| Message ID | PSAPR06MB49522EB50BDE08A5D9D0DACEC95F2@PSAPR06MB4952.apcprd06.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | serial: 8250_pnp: Support configurable reg shift property | expand |
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
在 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
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
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);