serial: 8250_fintek: Add using BIOS IRQ default setting

[PATCH] serial: 8250_fintek: Add using BIOS IRQ default setting

Posted by Ji-Ze Hong (Peter Hong) 2 years, 6 months ago

In 8250_fintek.c probe_setup_port(), we'll detect the IRQ trigger mode by
irq_get_irq_data() and pass it to fintek_8250_set_irq_mode(). If detected
Edge mode, we'll set the UART with Edge/High mode, otherwise Level/Low.

But in some motherboard, The APIC maybe setting to Level/High. In this case
the driver will setting wrong configuration into UART. So we add a option
to kernel parameter to control the driver as following:

	fintek_uart_irq_mode_override= [SERIAL]
		{default, bios}
		If the parameter is "default", the driver will using
		former IRQ override methed(By IRQ trigger type).
		otherwise, we'll don't change the UART IRQ setting.

Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
---
 drivers/tty/serial/8250/8250_fintek.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
index dba5950b8d0e..c5fea0a7c79b 100644
--- a/drivers/tty/serial/8250/8250_fintek.c
+++ b/drivers/tty/serial/8250/8250_fintek.c
@@ -92,6 +92,9 @@
 #define F81866_UART_CLK_18_432MHZ BIT(0)
 #define F81866_UART_CLK_24MHZ BIT(1)

+#define FINTEK_IRQ_MODE_BY_DETECT	0
+#define FINTEK_IRQ_MODE_BY_BIOS		1
+
 struct fintek_8250 {
 	u16 pid;
 	u16 base_port;
@@ -99,6 +102,24 @@ struct fintek_8250 {
 	u8 key;
 };

+static int not_override_irq_mode = FINTEK_IRQ_MODE_BY_DETECT;
+
+static int __init parse_uart_irq_mode_override(char *arg)
+{
+	if (!arg)
+		return -EINVAL;
+
+	if (strcmp(arg, "bios") == 0)
+		not_override_irq_mode = FINTEK_IRQ_MODE_BY_BIOS;
+	else if (strcmp(arg, "default") == 0)
+		not_override_irq_mode = FINTEK_IRQ_MODE_BY_DETECT;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+early_param("fintek_uart_irq_mode_override", parse_uart_irq_mode_override);
+
 static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg)
 {
 	outb(reg, pdata->base_port + ADDR_PORT);
@@ -248,6 +269,12 @@ static int fintek_8250_rs485_config(struct uart_port *port,

 static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
 {
+	if (not_override_irq_mode == FINTEK_IRQ_MODE_BY_BIOS) {
+		pr_info("Fintek UART(%04x) irq mode is using BIOS default",
+				pdata->pid);
+		return;
+	}
+
 	sio_write_reg(pdata, LDN, pdata->index);

 	switch (pdata->pid) {
--
2.17.1

Re: [PATCH] serial: 8250_fintek: Add using BIOS IRQ default setting

Posted by kernel test robot 2 years, 6 months ago

Hi Ji-Ze,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.2-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ji-Ze-Hong-Peter-Hong/serial-8250_fintek-Add-using-BIOS-IRQ-default-setting/20230217-165155
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20230217084953.2580-1-hpeter%2Blinux_kernel%40gmail.com
patch subject: [PATCH] serial: 8250_fintek: Add using BIOS IRQ default setting
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20230217/202302172031.t1Y23hRe-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/fd786fba8247c675fb90d00d076235cbd85842e6
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Ji-Ze-Hong-Peter-Hong/serial-8250_fintek-Add-using-BIOS-IRQ-default-setting/20230217-165155
        git checkout fd786fba8247c675fb90d00d076235cbd85842e6
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/tty/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202302172031.t1Y23hRe-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/tty/serial/8250/8250_fintek.c:121:13: error: expected declaration specifiers or '...' before string constant
     121 | early_param("fintek_uart_irq_mode_override", parse_uart_irq_mode_override);
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/tty/serial/8250/8250_fintek.c:121:46: error: expected declaration specifiers or '...' before 'parse_uart_irq_mode_override'
     121 | early_param("fintek_uart_irq_mode_override", parse_uart_irq_mode_override);
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/tty/serial/8250/8250_fintek.c:107:19: warning: 'parse_uart_irq_mode_override' defined but not used [-Wunused-function]
     107 | static int __init parse_uart_irq_mode_override(char *arg)
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +121 drivers/tty/serial/8250/8250_fintek.c

   106	
   107	static int __init parse_uart_irq_mode_override(char *arg)
   108	{
   109		if (!arg)
   110			return -EINVAL;
   111	
   112		if (strcmp(arg, "bios") == 0)
   113			not_override_irq_mode = FINTEK_IRQ_MODE_BY_BIOS;
   114		else if (strcmp(arg, "default") == 0)
   115			not_override_irq_mode = FINTEK_IRQ_MODE_BY_DETECT;
   116		else
   117			return -EINVAL;
   118	
   119		return 0;
   120	}
 > 121	early_param("fintek_uart_irq_mode_override", parse_uart_irq_mode_override);
   122	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [PATCH] serial: 8250_fintek: Add using BIOS IRQ default setting

Posted by Greg KH 2 years, 6 months ago

On Fri, Feb 17, 2023 at 04:49:53PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> In 8250_fintek.c probe_setup_port(), we'll detect the IRQ trigger mode by
> irq_get_irq_data() and pass it to fintek_8250_set_irq_mode(). If detected
> Edge mode, we'll set the UART with Edge/High mode, otherwise Level/Low.
> 
> But in some motherboard, The APIC maybe setting to Level/High. In this case
> the driver will setting wrong configuration into UART. So we add a option
> to kernel parameter to control the driver as following:
> 
> 	fintek_uart_irq_mode_override= [SERIAL]
> 		{default, bios}
> 		If the parameter is "default", the driver will using
> 		former IRQ override methed(By IRQ trigger type).
> 		otherwise, we'll don't change the UART IRQ setting.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>

Also note, your From: line does not match this line, so I couldn't take
this patch anyway :(

And why are you sending patches through a random gmail account and not
your fintek.com.tw address?  Please use that one, so that we can easily
verify that this is coming from the proper address and no one is
impersonating you.

thanks,

greg k-h

Re: [PATCH] serial: 8250_fintek: Add using BIOS IRQ default setting

Posted by Greg KH 2 years, 6 months ago

On Fri, Feb 17, 2023 at 04:49:53PM +0800, Ji-Ze Hong (Peter Hong) wrote:
> In 8250_fintek.c probe_setup_port(), we'll detect the IRQ trigger mode by
> irq_get_irq_data() and pass it to fintek_8250_set_irq_mode(). If detected
> Edge mode, we'll set the UART with Edge/High mode, otherwise Level/Low.
> 
> But in some motherboard, The APIC maybe setting to Level/High. In this case
> the driver will setting wrong configuration into UART. So we add a option
> to kernel parameter to control the driver as following:
> 
> 	fintek_uart_irq_mode_override= [SERIAL]
> 		{default, bios}
> 		If the parameter is "default", the driver will using
> 		former IRQ override methed(By IRQ trigger type).
> 		otherwise, we'll don't change the UART IRQ setting.
> 
> Signed-off-by: Ji-Ze Hong (Peter Hong) <hpeter+linux_kernel@gmail.com>
> ---
>  drivers/tty/serial/8250/8250_fintek.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_fintek.c b/drivers/tty/serial/8250/8250_fintek.c
> index dba5950b8d0e..c5fea0a7c79b 100644
> --- a/drivers/tty/serial/8250/8250_fintek.c
> +++ b/drivers/tty/serial/8250/8250_fintek.c
> @@ -92,6 +92,9 @@
>  #define F81866_UART_CLK_18_432MHZ BIT(0)
>  #define F81866_UART_CLK_24MHZ BIT(1)
> 
> +#define FINTEK_IRQ_MODE_BY_DETECT	0
> +#define FINTEK_IRQ_MODE_BY_BIOS		1
> +
>  struct fintek_8250 {
>  	u16 pid;
>  	u16 base_port;
> @@ -99,6 +102,24 @@ struct fintek_8250 {
>  	u8 key;
>  };
> 
> +static int not_override_irq_mode = FINTEK_IRQ_MODE_BY_DETECT;
> +
> +static int __init parse_uart_irq_mode_override(char *arg)
> +{
> +	if (!arg)
> +		return -EINVAL;
> +
> +	if (strcmp(arg, "bios") == 0)
> +		not_override_irq_mode = FINTEK_IRQ_MODE_BY_BIOS;
> +	else if (strcmp(arg, "default") == 0)
> +		not_override_irq_mode = FINTEK_IRQ_MODE_BY_DETECT;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +early_param("fintek_uart_irq_mode_override", parse_uart_irq_mode_override);

Sorry, but no, this will not work with multiple devices.  Please fix the
bios to handle this all properly, do not require users to manually fix
up problems for you (i.e. fix it once, not in thousands of individual
systems.)

> +
>  static u8 sio_read_reg(struct fintek_8250 *pdata, u8 reg)
>  {
>  	outb(reg, pdata->base_port + ADDR_PORT);
> @@ -248,6 +269,12 @@ static int fintek_8250_rs485_config(struct uart_port *port,
> 
>  static void fintek_8250_set_irq_mode(struct fintek_8250 *pdata, bool is_level)
>  {
> +	if (not_override_irq_mode == FINTEK_IRQ_MODE_BY_BIOS) {
> +		pr_info("Fintek UART(%04x) irq mode is using BIOS default",
> +				pdata->pid);

Note, this is a driver, always use dev_*() calls, never pr_*() calls.

thanks,

greg k-h