[PATCH] serial: 8250_pnp: Support configurable reg shift property

[PATCH] serial: 8250_pnp: Support configurable reg shift property

[GuanBing Huang]

From: albanhuang <albanhuang@tencent.com>

The 16550a serial port based on the ACPI table requires obtaining the
reg-shift attribute. In the ACPI scenario, If the reg-shift property
is not configured like in DTS, the 16550a serial driver cannot read or
write controller registers properly during initialization.

Signed-off-by: albanhuang <albanhuang@tencent.com>
Signed-off-by: tombinfan <tombinfan@tencent.com>
Signed-off-by: dylanlhdu <dylanlhdu@tencent.com>
---
 drivers/tty/serial/8250/8250_pnp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
index 1974bbadc975..25b4e41e9745 100644
--- a/drivers/tty/serial/8250/8250_pnp.c
+++ b/drivers/tty/serial/8250/8250_pnp.c
@@ -473,6 +473,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 		uart.port.flags |= UPF_SHARE_IRQ;
 	uart.port.uartclk = 1843200;
 	device_property_read_u32(&dev->dev, "clock-frequency", &uart.port.uartclk);
+	device_property_read_u8(&dev->dev, "reg-shift", &uart.port.regshift);
 	uart.port.dev = &dev->dev;
 
 	line = serial8250_register_8250_port(&uart);
-- 
2.17.1

Re: [PATCH] serial: 8250_pnp: Support configurable reg shift property

[Greg KH]

On Thu, Feb 29, 2024 at 07:51:54PM +0800, GuanBing Huang wrote:
> From: albanhuang <albanhuang@tencent.com>
> 
> The 16550a serial port based on the ACPI table requires obtaining the
> reg-shift attribute. In the ACPI scenario, If the reg-shift property
> is not configured like in DTS, the 16550a serial driver cannot read or
> write controller registers properly during initialization.
> 
> Signed-off-by: albanhuang <albanhuang@tencent.com>
> Signed-off-by: tombinfan <tombinfan@tencent.com>
> Signed-off-by: dylanlhdu <dylanlhdu@tencent.com>

"interesting" names, can you not just use your native encoding to make
this easier?

> ---
>  drivers/tty/serial/8250/8250_pnp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
> index 1974bbadc975..25b4e41e9745 100644
> --- a/drivers/tty/serial/8250/8250_pnp.c
> +++ b/drivers/tty/serial/8250/8250_pnp.c
> @@ -473,6 +473,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
>  		uart.port.flags |= UPF_SHARE_IRQ;
>  	uart.port.uartclk = 1843200;
>  	device_property_read_u32(&dev->dev, "clock-frequency", &uart.port.uartclk);
> +	device_property_read_u8(&dev->dev, "reg-shift", &uart.port.regshift);

Is this property documented somewhere?  What happens if the property
isn't there?

thanks,

greg k-h

Re: [PATCH] serial: 8250_pnp: Support configurable reg shift property

[GuanBing Huang]

在 2024/3/1 6:00, Greg KH 写道:

> On Thu, Feb 29, 2024 at 07:51:54PM +0800, GuanBing Huang wrote:
>> From: albanhuang <albanhuang@tencent.com>
>>
>> The 16550a serial port based on the ACPI table requires obtaining the
>> reg-shift attribute. In the ACPI scenario, If the reg-shift property
>> is not configured like in DTS, the 16550a serial driver cannot read or
>> write controller registers properly during initialization.
>>
>> Signed-off-by: albanhuang <albanhuang@tencent.com>
>> Signed-off-by: tombinfan <tombinfan@tencent.com>
>> Signed-off-by: dylanlhdu <dylanlhdu@tencent.com>
> "interesting" names, can you not just use your native encoding to make
> this easier?

> ->I'm sorry,this is my first time sending a patch.The names should be changed to the following. Do I need to resend a new patch?

Signed-off-by: Guanbing Huang <albanhuang@tencent.com>
Signed-off-by: Bing Fan <tombinfan@tencent.com>
Signed-off-by: Linheng Du <dylanlhdu@tencent.com>

>> ---
>>   drivers/tty/serial/8250/8250_pnp.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
>> index 1974bbadc975..25b4e41e9745 100644
>> --- a/drivers/tty/serial/8250/8250_pnp.c
>> +++ b/drivers/tty/serial/8250/8250_pnp.c
>> @@ -473,6 +473,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
>>   		uart.port.flags |= UPF_SHARE_IRQ;
>>   	uart.port.uartclk = 1843200;
>>   	device_property_read_u32(&dev->dev, "clock-frequency", &uart.port.uartclk);
>> +	device_property_read_u8(&dev->dev, "reg-shift", &uart.port.regshift);
> Is this property documented somewhere?  What happens if the property
> isn't there?
> ->
> 1、In the DTS scenario, certain chips use the reg-shift property, such as:
>
> arch/riscv/boot/dts/microchip/mpfs.dtsi:
>
> 294                 mmuart0: serial@20000000 {
> 295                         compatible = "ns16550a";
> 296                         reg = <0x0 0x20000000 0x0 0x400>;
> 297                         reg-io-width = <4>;
> 298                         reg-shift = <2>;
> 299                         interrupt-parent = <&plic>;
> 300                         interrupts = <90>;
> 301                         current-speed = <115200>;
> 302                         clocks = <&clkcfg CLK_MMUART0>;
> 303                         status = "disabled"; /* Reserved for the HSS */
> 304                 };
>
> drivers/tty/serial/8250/8250_of.c:
>
> of_platform_serial_probe->of_platform_serial_setup:
>
> 150         /* Check for registers offset within the devices address range */
> 151         if (of_property_read_u32(np, "reg-shift", &prop) == 0)
> 152                 port->regshift = prop;
> 2、In the ACPI scenario, 16550a serial port initialization code execution process:
> ->serial_pnp_probe
> ->serial8250_register_8250_port
> ->uart_add_one_port
> ->serial_ctrl_register_port
> ->serial_core_register_port
> ->serial_core_add_one_port
> ->uart_configure_port
> ->serial8250_config_port
> ->autoconfig:
>
> 1194                 scratch = serial_in(up, UART_IER);
> 1195                 serial_out(up, UART_IER, 0);
> 1196 #ifdef __i386__
> 1197                 outb(0xff, 0x080);
> 1198 #endif
> 1199                 /*
> 1200                  * Mask out IER[7:4] bits for test as some UARTs (e.g. TL
> 1201                  * 16C754B) allow only to modify them if an EFR bit is set.
> 1202                  */
> 1203                 scratch2 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
> 1204                 serial_out(up, UART_IER, UART_IER_ALL_INTR);
> 1205 #ifdef __i386__
> 1206                 outb(0, 0x080);
> 1207 #endif
> 1208                 scratch3 = serial_in(up, UART_IER) & UART_IER_ALL_INTR;
> 1209                 serial_out(up, UART_IER, scratch);
> 1210                 if (scratch2 != 0 || scratch3 != UART_IER_ALL_INTR) {
> 1211                         /*
> 1212                          * We failed; there's nothing here
> 1213                          */
> 1214                         spin_unlock_irqrestore(&port->lock, flags);
> 1215                         DEBUG_AUTOCONF("IER test failed (%02x, %02x) ",
> 1216                                        scratch2, scratch3);
> 1217                         goto out;
> 1218                 }
>
> static unsigned int mem_serial_in(struct uart_port *p, int offset)
> {
>          offset = offset << p->regshift;
>          return readb(p->membase + offset);
> }
>
> static void mem_serial_out(struct uart_port *p, int offset, int value)
> {
>          offset = offset << p->regshift;
>          writeb(value, p->membase + offset);
> }
> The kernel will execute the serial_pnp_probe function to initialize the 16550a serial port during startup.
> When executing the autoconfig function, the serial_in/serial_out function failed to read and write to the UART_IER register,
> causing an abnormal branch entry at line 1210 and an abnormal exit at line 1217, preventing the subsequent 16550a initialization
> process from being executed properly.
> mem_serial_in/mem_serial_out will perform a regshift offset when reading and writing registers,
> because serial_pnp_probe does not have a configured regshift, which is 0 (regshift needs to be configured as 2),
> resulting in invalid register reading and writing.
> Without the reg-shift attribute, print the kernel boot log and identify the serial port as "unknown":
>
> [    1.458722] riscv-plic riscv-plic.0: mapped 64 interrupts with 1 handlers for 1 contexts.
> [    5.342472] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [    6.038702] 00:02: ttyS0 at MMIO 0x310b0000 (irq = 12, base_baud = 3125000) is a unknown
> Configure the value of the reg-shift attribute to 2, print the kernel boot log, and correctly recognize 16550a:
>
> [    1.459948] riscv-plic riscv-plic.0: mapped 64 interrupts with 1 handlers for 1 contexts.
> [    5.317550] Serial: 8250/16550 driver, 4 ports, IRQ sharing disabled
> [    5.864828] 00:02: ttyS0 at MMIO 0x310b0000 (irq = 12, base_baud = 3125000) is a 16550A
> thanks,
> Guanbing Huang
> thanks,
>
> greg k-h

Re: [PATCH] serial: 8250_pnp: Support configurable reg shift property

[Greg KH]

On Tue, Mar 05, 2024 at 11:24:08AM +0800, GuanBing Huang wrote:
> 在 2024/3/1 6:00, Greg KH 写道:
> 
> > On Thu, Feb 29, 2024 at 07:51:54PM +0800, GuanBing Huang wrote:
> > > From: albanhuang <albanhuang@tencent.com>
> > > 
> > > The 16550a serial port based on the ACPI table requires obtaining the
> > > reg-shift attribute. In the ACPI scenario, If the reg-shift property
> > > is not configured like in DTS, the 16550a serial driver cannot read or
> > > write controller registers properly during initialization.
> > > 
> > > Signed-off-by: albanhuang <albanhuang@tencent.com>
> > > Signed-off-by: tombinfan <tombinfan@tencent.com>
> > > Signed-off-by: dylanlhdu <dylanlhdu@tencent.com>
> > "interesting" names, can you not just use your native encoding to make
> > this easier?
> 
> > ->I'm sorry,this is my first time sending a patch.The names should be changed to the following. Do I need to resend a new patch?
> 
> Signed-off-by: Guanbing Huang <albanhuang@tencent.com>
> Signed-off-by: Bing Fan <tombinfan@tencent.com>
> Signed-off-by: Linheng Du <dylanlhdu@tencent.com>

Yes please, I can not take it in the original format.

thanks,

greg k-h

[PATCH v2] serial: 8250_pnp: Support configurable reg shift property

[Guanbing Huang]

From: Guanbing Huang <albanhuang@tencent.com>

The 16550a serial port based on the ACPI table requires obtaining the
reg-shift attribute. In the ACPI scenario, If the reg-shift property
is not configured like in DTS, the 16550a serial driver cannot read or
write controller registers properly during initialization.

Signed-off-by: Guanbing Huang <albanhuang@tencent.com>
Signed-off-by: Bing Fan <tombinfan@tencent.com>
Signed-off-by: Linheng Du <dylanlhdu@tencent.com>
---
v2: change the names after "Signed off by" to the real names

 drivers/tty/serial/8250/8250_pnp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/8250/8250_pnp.c b/drivers/tty/serial/8250/8250_pnp.c
index 1974bbadc975..25b4e41e9745 100644
--- a/drivers/tty/serial/8250/8250_pnp.c
+++ b/drivers/tty/serial/8250/8250_pnp.c
@@ -473,6 +473,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
 		uart.port.flags |= UPF_SHARE_IRQ;
 	uart.port.uartclk = 1843200;
 	device_property_read_u32(&dev->dev, "clock-frequency", &uart.port.uartclk);
+	device_property_read_u8(&dev->dev, "reg-shift", &uart.port.regshift);
 	uart.port.dev = &dev->dev;
 
 	line = serial8250_register_8250_port(&uart);
-- 
2.17.1

Re: [PATCH v2] serial: 8250_pnp: Support configurable reg shift property

[Andy Shevchenko]

On Wed, Mar 06, 2024 at 07:42:27PM +0800, Guanbing Huang wrote:
> From: Guanbing Huang <albanhuang@tencent.com>

Thanks for your contribution!
My comments below.

...


First of all, always start a new email thread when sending a new version of the
patch (i.o.w. no In-Reply-to email header).

> The 16550a serial port based on the ACPI table requires obtaining the
> reg-shift attribute. In the ACPI scenario, If the reg-shift property
> is not configured like in DTS, the 16550a serial driver cannot read or
> write controller registers properly during initialization.
> 
> Signed-off-by: Guanbing Huang <albanhuang@tencent.com>


> Signed-off-by: Bing Fan <tombinfan@tencent.com>
> Signed-off-by: Linheng Du <dylanlhdu@tencent.com>

This chain, as described in Submitting Patches documentation [1], should go
accordingly.

...

> @@ -473,6 +473,7 @@ serial_pnp_probe(struct pnp_dev *dev, const struct pnp_device_id *dev_id)
>  		uart.port.flags |= UPF_SHARE_IRQ;
>  	uart.port.uartclk = 1843200;
>  	device_property_read_u32(&dev->dev, "clock-frequency", &uart.port.uartclk);
> +	device_property_read_u8(&dev->dev, "reg-shift", &uart.port.regshift);
>  	uart.port.dev = &dev->dev;

Instead, it may make sense to switch to use uart_read_port_properties() which
has been recently introduced and dozen of drivers converted.

Ex. e6a46d073e11 ("serial: 8250_dw: Switch to use uart_read_port_properties()")

Yes, it assumes that you always need to base your changes on the latest
available changes in the certain subsystem (here it is tty-next branch
in Greg's tty tree, see git.kernel.org for the details).

>  	line = serial8250_register_8250_port(&uart);

[1]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

-- 
With Best Regards,
Andy Shevchenko