| Message ID | 20240221183442.4124354-11-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | serial: Add a helper to parse device properties and more | expand |
On Wed, 2024-02-21 at 20:31 +0200, Andy Shevchenko wrote: > Since we have now a common helper to read port properties > use it instead of sparse home grown solution. I did some brief testing of the series for the Aspeed machines under qemu, building them on top of v6.8-rc5: export ARCH=arm export CROSS_COMPILE=arm-linux-gnueabihf- make aspeed_g5_defconfig make -j$(nproc) qemu-system-arm -M rainier-bmc -nographic -no-reboot -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb -initrd ... I got an oops during boot, which bisected to this change: [ 0.314946] 8<--- cut here --- [ 0.315051] Unable to handle kernel paging request at virtual address fee00000 when write [ 0.315201] [fee00000] *pgd=00000000 [ 0.315593] Internal error: Oops: 805 [#1] SMP ARM [ 0.315847] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 6.8.0-rc5-00010-g8a2c8fe174cf #13 [ 0.316071] Hardware name: Generic DT based system [ 0.316216] PC is at io_serial_out+0x18/0x20 [ 0.316677] LR is at serial8250_do_set_mctrl+0x54/0x90 [ 0.316781] pc : [<8060eea8>] lr : [<806108b0>] psr: 40000093 [ 0.316891] sp : bf815b08 ip : 00000000 fp : 00000026 [ 0.316987] r10: 81698240 r9 : 40000013 r8 : 81cae600 [ 0.317087] r7 : 81d7d1a8 r6 : 81d7d110 r5 : 81008158 r4 : 00000000 [ 0.317197] r3 : fee00000 r2 : 00000000 r1 : 00000004 r0 : 81008158 [ 0.317350] Flags: nZcv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none [ 0.317471] Control: 10c5387d Table: 8000406a DAC: 00000051 [ 0.317593] Register r0 information: non-slab/vmalloc memory [ 0.317892] Register r1 information: non-paged memory [ 0.317996] Register r2 information: NULL pointer [ 0.318080] Register r3 information: vmalloc memory [ 0.318176] Register r4 information: NULL pointer [ 0.318264] Register r5 information: non-slab/vmalloc memory [ 0.318362] Register r6 information: slab kmalloc-2k start 81d7d000 pointer offset 272 size 2048 [ 0.318701] Register r7 information: slab kmalloc-2k start 81d7d000 pointer offset 424 size 2048 [ 0.318860] Register r8 information: slab kmalloc-512 start 81cae600 pointer offset 0 size 512 [ 0.319095] Register r9 information: non-paged memory [ 0.319194] Register r10 information: slab kmalloc-64 start 81698240 pointer offset 0 size 64 [ 0.319266] Freeing initrd memory: 13684K [ 0.319384] Register r11 information: non-paged memory [ 0.319593] Register r12 information: NULL pointer [ 0.319703] Process swapper/0 (pid: 1, stack limit = 0x(ptrval)) [ 0.320006] Stack: (0xbf815b08 to 0xbf816000) [ 0.320157] 5b00: 81008158 80f85a88 81d7d110 8060cb78 bf815b34 00000026 [ 0.320313] 5b20: 0016e360 80cba110 81e65e80 80cfcdf4 00000003 204f2f49 00307830 00000000 [ 0.320457] 5b40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.320600] 5b60: 00000000 00000000 00000000 00000000 00000000 ed1677db 81008158 81008158 [ 0.320744] 5b80: bf815c00 81e5c5c0 81008304 81007f58 bf815d2c bf815dac 00000000 8060e1f4 [ 0.320890] 5ba0: 80cba4ec 8081e2c4 bf815dfc 00000001 00000000 81cf5400 81cf5410 81e65e00 [ 0.321030] 5bc0: 00000004 00000000 00000001 80616538 00000000 00000000 00000000 00000000 [ 0.321176] 5be0: 1e784000 1e784fff bd7c1a94 00000200 00000000 00000000 00000000 00000000 [ 0.321325] 5c00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.321469] 5c20: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.321624] 5c40: 00000000 00000000 8060fb34 00000000 00000000 00000000 00000026 00000000 [ 0.321777] 5c60: 016e3600 00000000 00000200 00000000 00000000 00000000 00000000 00000000 [ 0.321920] 5c80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.322063] 5ca0: 00000000 00000000 b9000040 00000000 00000000 00000000 00000000 00000000 [ 0.322204] 5cc0: 00000004 00000000 00000000 00000004 00000000 1e784000 00001000 81cf5410 [ 0.322347] 5ce0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.322492] 5d00: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000037 [ 0.322640] 5d20: 00000001 00000001 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.322800] 5d40: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.322957] 5d60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.323114] 5d80: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.323271] 5da0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.323422] 5dc0: 00000000 00000000 806111c8 80610eb4 00000000 00000000 00000000 00000000 [ 0.323573] 5de0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.323723] 5e00: 00000000 ed1677db 00000000 81cf5410 80f85cf8 00000000 00000000 81e5c638 [ 0.323878] 5e20: 80e66f48 8067f888 81cf5410 00000000 80f85cf8 00000000 00000000 8067ca08 [ 0.324029] 5e40: 81cf5410 00000000 81cf5410 81cf5410 80f85cf8 81cf5454 81cf5410 8067cda8 [ 0.324181] 5e60: 60000013 81e5c638 81008d4c 81008d54 81cf5454 81cf5410 00000000 8067cf3c [ 0.324337] 5e80: 81cf5410 80f85cf8 81cf5454 814cec00 00000000 8067d21c 00000000 80f85cf8 [ 0.324494] 5ea0: 8067d11c 8067aa04 814cec00 814cec58 816a4bb4 ed1677db 814cec00 81e5c600 [ 0.324646] 5ec0: 00000000 80f85cf8 814cec00 8067bc6c 80cba524 00000000 00000006 80f85cf8 [ 0.324795] 5ee0: 8158b480 00000006 bf815f14 00000000 80d19438 8067e284 80e221c4 8158b480 [ 0.324945] 5f00: 00000006 80e01414 80d2d3b0 000000db 8173ad17 00000000 00000000 00000000 [ 0.325096] 5f20: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.325247] 5f40: 00000000 00000000 00000000 00000000 00000000 ed1677db 8173ad00 00000020 [ 0.325403] 5f60: 00000006 80e3b83c 80e3b860 80e01750 00000006 00000006 00000000 80e004f8 [ 0.325553] 5f80: 80f05cc0 80a50e18 00000000 00000000 00000000 00000000 00000000 80a50e34 [ 0.325699] 5fa0: 00000000 80a50e18 00000000 8010016c 00000000 00000000 00000000 00000000 [ 0.325848] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.325995] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [ 0.326531] io_serial_out from serial8250_do_set_mctrl+0x54/0x90 [ 0.326761] serial8250_do_set_mctrl from serial_core_register_port+0x4c4/0x694 [ 0.326917] serial_core_register_port from serial8250_register_8250_port+0x310/0x4bc [ 0.327063] serial8250_register_8250_port from of_platform_serial_probe+0x300/0x45c [ 0.327242] of_platform_serial_probe from platform_probe+0x60/0xb8 [ 0.327367] platform_probe from really_probe+0xd4/0x3e4 [ 0.327471] really_probe from __driver_probe_device+0x90/0x1ec [ 0.327568] __driver_probe_device from driver_probe_device+0x38/0xd0 [ 0.327674] driver_probe_device from __driver_attach+0x100/0x1dc [ 0.327793] __driver_attach from bus_for_each_dev+0x84/0xd4 [ 0.327906] bus_for_each_dev from bus_add_driver+0xec/0x1f0 [ 0.328015] bus_add_driver from driver_register+0x84/0x11c [ 0.328126] driver_register from do_one_initcall+0x84/0x1c8 [ 0.328297] do_one_initcall from kernel_init_freeable+0x19c/0x22c [ 0.328419] kernel_init_freeable from kernel_init+0x1c/0x138 [ 0.328534] kernel_init from ret_from_fork+0x14/0x28 [ 0.328656] Exception stack(0xbf815fb0 to 0xbf815ff8) [ 0.328755] 5fa0: 00000000 00000000 00000000 00000000 [ 0.328901] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 0.329112] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 0.329413] Code: e3a03000 ee073f9a e2433612 e6ef2072 (e5c32000) [ 0.329824] ---[ end trace 0000000000000000 ]--- [ 0.336692] Kernel panic - not syncing: Fatal exception Andrew
On Thu, Feb 22, 2024 at 11:07:05AM +1030, Andrew Jeffery wrote: > On Wed, 2024-02-21 at 20:31 +0200, Andy Shevchenko wrote: > > Since we have now a common helper to read port properties > > use it instead of sparse home grown solution. > > I did some brief testing of the series for the Aspeed machines under > qemu, building them on top of v6.8-rc5: > > export ARCH=arm > export CROSS_COMPILE=arm-linux-gnueabihf- > make aspeed_g5_defconfig > make -j$(nproc) > qemu-system-arm -M rainier-bmc -nographic -no-reboot -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb -initrd ... > > I got an oops during boot, which bisected to this change: Thank you for prompt testing! I will look at it.
On Thu, Feb 22, 2024 at 03:23:24PM +0200, Andy Shevchenko wrote: > On Thu, Feb 22, 2024 at 11:07:05AM +1030, Andrew Jeffery wrote: > > On Wed, 2024-02-21 at 20:31 +0200, Andy Shevchenko wrote: > > > Since we have now a common helper to read port properties > > > use it instead of sparse home grown solution. > > > > I did some brief testing of the series for the Aspeed machines under > > qemu, building them on top of v6.8-rc5: > > > > export ARCH=arm > > export CROSS_COMPILE=arm-linux-gnueabihf- > > make aspeed_g5_defconfig > > make -j$(nproc) > > qemu-system-arm -M rainier-bmc -nographic -no-reboot -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb -initrd ... > > > > I got an oops during boot, which bisected to this change: > > Thank you for prompt testing! I will look at it. I found the issue, will be fixed in next version.
On Thu, Feb 22, 2024 at 06:43:08PM +0200, Andy Shevchenko wrote: > On Thu, Feb 22, 2024 at 03:23:24PM +0200, Andy Shevchenko wrote: > > On Thu, Feb 22, 2024 at 11:07:05AM +1030, Andrew Jeffery wrote: > > > On Wed, 2024-02-21 at 20:31 +0200, Andy Shevchenko wrote: > > > > Since we have now a common helper to read port properties > > > > use it instead of sparse home grown solution. > > > > > > I did some brief testing of the series for the Aspeed machines under > > > qemu, building them on top of v6.8-rc5: > > > > > > export ARCH=arm > > > export CROSS_COMPILE=arm-linux-gnueabihf- > > > make aspeed_g5_defconfig > > > make -j$(nproc) > > > qemu-system-arm -M rainier-bmc -nographic -no-reboot -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb -initrd ... > > > > > > I got an oops during boot, which bisected to this change: > > > > Thank you for prompt testing! I will look at it. > > I found the issue, will be fixed in next version. Whoever is going to test this series, the - port->iotype = use_defaults ? UPIO_MEM : port->iotype; + port->iotype = UPIO_MEM; should be applied to uart_read_port_properties() implementation.
On 2/22/24 08:47, Andy Shevchenko wrote: > On Thu, Feb 22, 2024 at 06:43:08PM +0200, Andy Shevchenko wrote: >> On Thu, Feb 22, 2024 at 03:23:24PM +0200, Andy Shevchenko wrote: >>> On Thu, Feb 22, 2024 at 11:07:05AM +1030, Andrew Jeffery wrote: >>>> On Wed, 2024-02-21 at 20:31 +0200, Andy Shevchenko wrote: >>>>> Since we have now a common helper to read port properties >>>>> use it instead of sparse home grown solution. >>>> >>>> I did some brief testing of the series for the Aspeed machines under >>>> qemu, building them on top of v6.8-rc5: >>>> >>>> export ARCH=arm >>>> export CROSS_COMPILE=arm-linux-gnueabihf- >>>> make aspeed_g5_defconfig >>>> make -j$(nproc) >>>> qemu-system-arm -M rainier-bmc -nographic -no-reboot -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb -initrd ... >>>> >>>> I got an oops during boot, which bisected to this change: >>> >>> Thank you for prompt testing! I will look at it. >> >> I found the issue, will be fixed in next version. > > Whoever is going to test this series, the > > - port->iotype = use_defaults ? UPIO_MEM : port->iotype; > + port->iotype = UPIO_MEM; > > should be applied to uart_read_port_properties() implementation. > Thanks, on 8250_bcm7271.c with the above hunk applied, I did not spot any differences between the values returned by stty or a cat /sys/class/tty/ttyS0/* before or after, the console remained fully functional. I will see if I can run an additional test where I removed the DT's "clocks" property and confirm that the fall back to "clock-frequency" works. Thanks Andy!
On 2/22/24 09:39, Florian Fainelli wrote: > On 2/22/24 08:47, Andy Shevchenko wrote: >> On Thu, Feb 22, 2024 at 06:43:08PM +0200, Andy Shevchenko wrote: >>> On Thu, Feb 22, 2024 at 03:23:24PM +0200, Andy Shevchenko wrote: >>>> On Thu, Feb 22, 2024 at 11:07:05AM +1030, Andrew Jeffery wrote: >>>>> On Wed, 2024-02-21 at 20:31 +0200, Andy Shevchenko wrote: >>>>>> Since we have now a common helper to read port properties >>>>>> use it instead of sparse home grown solution. >>>>> >>>>> I did some brief testing of the series for the Aspeed machines under >>>>> qemu, building them on top of v6.8-rc5: >>>>> >>>>> export ARCH=arm >>>>> export CROSS_COMPILE=arm-linux-gnueabihf- >>>>> make aspeed_g5_defconfig >>>>> make -j$(nproc) >>>>> qemu-system-arm -M rainier-bmc -nographic -no-reboot -kernel >>>>> arch/arm/boot/zImage -dtb >>>>> arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb -initrd ... >>>>> >>>>> I got an oops during boot, which bisected to this change: >>>> >>>> Thank you for prompt testing! I will look at it. >>> >>> I found the issue, will be fixed in next version. >> >> Whoever is going to test this series, the >> >> - port->iotype = use_defaults ? UPIO_MEM : port->iotype; >> + port->iotype = UPIO_MEM; >> >> should be applied to uart_read_port_properties() implementation. >> > > Thanks, on 8250_bcm7271.c with the above hunk applied, I did not spot > any differences between the values returned by stty or a cat > /sys/class/tty/ttyS0/* before or after, the console remained fully > functional. I will see if I can run an additional test where I removed > the DT's "clocks" property and confirm that the fall back to > "clock-frequency" works. > > Thanks Andy! Also appears to work properly on a Raspberry Pi 4 with the console using the bcm2835-aux driver, will provide Tested-by tags on the next submission, thanks!
On Thu, Feb 22, 2024 at 11:54:46AM -0800, Florian Fainelli wrote: > On 2/22/24 09:39, Florian Fainelli wrote: > > On 2/22/24 08:47, Andy Shevchenko wrote: ... > > Thanks, on 8250_bcm7271.c with the above hunk applied, I did not spot > > any differences between the values returned by stty or a cat > > /sys/class/tty/ttyS0/* before or after, the console remained fully > > functional. I will see if I can run an additional test where I removed > > the DT's "clocks" property and confirm that the fall back to > > "clock-frequency" works. > > Also appears to work properly on a Raspberry Pi 4 with the console using the > bcm2835-aux driver, will provide Tested-by tags on the next submission, > thanks! Thank you for prompt testing on real HW!
On Thu, 2024-02-22 at 18:47 +0200, Andy Shevchenko wrote: > On Thu, Feb 22, 2024 at 06:43:08PM +0200, Andy Shevchenko wrote: > > On Thu, Feb 22, 2024 at 03:23:24PM +0200, Andy Shevchenko wrote: > > > On Thu, Feb 22, 2024 at 11:07:05AM +1030, Andrew Jeffery wrote: > > > > On Wed, 2024-02-21 at 20:31 +0200, Andy Shevchenko wrote: > > > > > Since we have now a common helper to read port properties > > > > > use it instead of sparse home grown solution. > > > > > > > > I did some brief testing of the series for the Aspeed machines under > > > > qemu, building them on top of v6.8-rc5: > > > > > > > > export ARCH=arm > > > > export CROSS_COMPILE=arm-linux-gnueabihf- > > > > make aspeed_g5_defconfig > > > > make -j$(nproc) > > > > qemu-system-arm -M rainier-bmc -nographic -no-reboot -kernel arch/arm/boot/zImage -dtb arch/arm/boot/dts/aspeed/aspeed-bmc-ibm-rainier.dtb -initrd ... > > > > > > > > I got an oops during boot, which bisected to this change: > > > > > > Thank you for prompt testing! I will look at it. > > > > I found the issue, will be fixed in next version. > > Whoever is going to test this series, the > > - port->iotype = use_defaults ? UPIO_MEM : port->iotype; > + port->iotype = UPIO_MEM; > > should be applied to uart_read_port_properties() implementation. > Thanks, with that fix applied it works fine for me also. Andrew
diff --git a/drivers/tty/serial/8250/8250_of.c b/drivers/tty/serial/8250/8250_of.c index 9dcc17e33269..1a699ce2e812 100644 --- a/drivers/tty/serial/8250/8250_of.c +++ b/drivers/tty/serial/8250/8250_of.c @@ -69,37 +69,22 @@ static int of_platform_serial_setup(struct platform_device *ofdev, struct device *dev = &ofdev->dev; struct device_node *np = dev->of_node; struct uart_port *port = &up->port; - u32 clk, spd, prop; - int ret, irq; + u32 spd; + int ret; memset(port, 0, sizeof *port); pm_runtime_enable(&ofdev->dev); pm_runtime_get_sync(&ofdev->dev); - if (of_property_read_u32(np, "clock-frequency", &clk)) { - - /* Get clk rate through clk driver if present */ - info->clk = devm_clk_get_enabled(dev, NULL); - if (IS_ERR(info->clk)) { - ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n"); - goto err_pmruntime; - } - - clk = clk_get_rate(info->clk); - } - /* If current-speed was set, then try not to change it. */ - if (of_property_read_u32(np, "current-speed", &spd) == 0) - port->custom_divisor = clk / (16 * spd); - ret = of_address_to_resource(np, 0, &resource); if (ret) { dev_err_probe(dev, ret, "invalid address\n"); goto err_pmruntime; } - port->flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | - UPF_FIXED_TYPE; + port->dev = &ofdev->dev; + port->flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE; spin_lock_init(&port->lock); if (resource_type(&resource) == IORESOURCE_IO) { @@ -108,70 +93,31 @@ static int of_platform_serial_setup(struct platform_device *ofdev, } else { port->mapbase = resource.start; port->mapsize = resource_size(&resource); - - /* Check for shifted address mapping */ - if (of_property_read_u32(np, "reg-offset", &prop) == 0) { - if (prop >= port->mapsize) { - ret = dev_err_probe(dev, -EINVAL, "reg-offset %u exceeds region size %pa\n", - prop, &port->mapsize); - goto err_pmruntime; - } - - port->mapbase += prop; - port->mapsize -= prop; - } - - port->iotype = UPIO_MEM; - if (of_property_read_u32(np, "reg-io-width", &prop) == 0) { - switch (prop) { - case 1: - port->iotype = UPIO_MEM; - break; - case 2: - port->iotype = UPIO_MEM16; - break; - case 4: - port->iotype = of_device_is_big_endian(np) ? - UPIO_MEM32BE : UPIO_MEM32; - break; - default: - ret = dev_err_probe(dev, -EINVAL, "unsupported reg-io-width (%u)\n", - prop); - goto err_pmruntime; - } - } port->flags |= UPF_IOREMAP; } + ret = uart_read_port_properties(port, false); + if (ret) + goto err_pmruntime; + + /* Get clk rate through clk driver if present */ + if (!port->uartclk) { + info->clk = devm_clk_get_enabled(dev, NULL); + if (IS_ERR(info->clk)) { + ret = dev_err_probe(dev, PTR_ERR(info->clk), "failed to get clock\n"); + goto err_pmruntime; + } + + port->uartclk = clk_get_rate(info->clk); + } + /* If current-speed was set, then try not to change it. */ + if (of_property_read_u32(np, "current-speed", &spd) == 0) + port->custom_divisor = port->uartclk / (16 * spd); + /* Compatibility with the deprecated pxa driver and 8250_pxa drivers. */ if (of_device_is_compatible(np, "mrvl,mmp-uart")) port->regshift = 2; - /* Check for registers offset within the devices address range */ - if (of_property_read_u32(np, "reg-shift", &prop) == 0) - port->regshift = prop; - - /* Check for fifo size */ - if (of_property_read_u32(np, "fifo-size", &prop) == 0) - port->fifosize = prop; - - /* Check for a fixed line number */ - ret = of_alias_get_id(np, "serial"); - if (ret >= 0) - port->line = ret; - - irq = of_irq_get(np, 0); - if (irq < 0) { - if (irq == -EPROBE_DEFER) { - ret = -EPROBE_DEFER; - goto err_pmruntime; - } - /* IRQ support not mandatory */ - irq = 0; - } - - port->irq = irq; - info->rst = devm_reset_control_get_optional_shared(&ofdev->dev, NULL); if (IS_ERR(info->rst)) { ret = PTR_ERR(info->rst); @@ -183,12 +129,6 @@ static int of_platform_serial_setup(struct platform_device *ofdev, goto err_pmruntime; port->type = type; - port->uartclk = clk; - - if (of_property_read_bool(np, "no-loopback-test")) - port->flags |= UPF_SKIP_TEST; - - port->dev = &ofdev->dev; port->rs485_config = serial8250_em485_config; port->rs485_supported = serial8250_em485_supported; up->rs485_start_tx = serial8250_em485_start_tx; @@ -280,7 +220,6 @@ static int of_platform_serial_probe(struct platform_device *ofdev) platform_set_drvdata(ofdev, info); return 0; err_dispose: - irq_dispose_mapping(port8250.port.irq); pm_runtime_put_sync(&ofdev->dev); pm_runtime_disable(&ofdev->dev); err_free: