Hi Wolfgang,
On Fri, Apr 3, 2020 at 7:47 PM Wolfgang Wallner wolfgang.wallner@br-automation.com wrote:
Hi Bin,
Thanks for taking care of this!
-----"Bin Meng" bmeng.cn@gmail.com schrieb: -----
An: "Simon Glass" sjg@chromium.org, "Tom Rini" trini@konsulko.com, "Andy Shevchenko" andriy.shevchenko@linux.intel.com, "Wolfgang Wallner" wolfgang.wallner@br-automation.com, "Chee Hong Ang" chee.hong.ang@intel.com, "U-Boot Mailing List" u-boot@lists.denx.de Von: "Bin Meng" bmeng.cn@gmail.com Datum: 03.04.2020 11:58 Betreff: [PATCH] serial: ns16550: Fix ordering of getting base address
Currently the driver gets ns16550 base address in the driver probe() routine, which may potentially break any ns16550 wrapper driver that does additional initialization before calling ns16550_serial_probe().
Things are complicated that we need consider ns16550 devices on both simple-bus and PCI bus. To fix the issue we move the base address assignment for simple-bus ns16550 device back to the ofdata_to_platdata(), and assign base address for PCI ns16550 device in ns16550_serial_probe().
This is still not perfect. Ideally if any PCI bus based ns16550 wrapper driver tries to access plat->base before calling probe(), it is subject to break.
Fixes: 720f9e1fdb0c9 ("serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()") Reported-by: Andy Shevchenko andriy.shevchenko@linux.intel.com Signed-off-by: Bin Meng bmeng.cn@gmail.com
drivers/serial/ns16550.c | 51 +++++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 27 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index c1b303f..5e3cd1c 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -479,39 +479,24 @@ static int ns16550_serial_getinfo(struct udevice *dev, return 0; }
-#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) -static int ns1655_serial_set_base_addr(struct udevice *dev) -{
fdt_addr_t addr;struct ns16550_platdata *plat;plat = dev_get_platdata(dev);addr = dev_read_addr_pci(dev);if (addr == FDT_ADDR_T_NONE)return -EINVAL;-#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
plat->base = addr;-#else
plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE);-#endif
return 0;-} -#endif
int ns16550_serial_probe(struct udevice *dev) {
struct ns16550_platdata *plat = dev->platdata; struct NS16550 *const com_port = dev_get_priv(dev); struct reset_ctl_bulk reset_bulk;fdt_addr_t addr; int ret;-#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
ret = ns1655_serial_set_base_addr(dev);if (ret)return ret;-#endif
/** If we are on PCI bus, either directly attached to a PCI rootport,
* or via a PCI bridge, assign platdata->base before probinghardware.
*/if (device_is_on_pci_bus(dev)) {addr = devfdt_get_addr_pci(dev);if (addr == FDT_ADDR_T_NONE)return -EINVAL;plat->base = addr;The assignment here to plat->base does not distinguish any more based on CONFIG_SYS_NS16550_PORT_MAPPED. Is it not necessary any more in this case?
Yep, you are right. Both two cases should be handled consistently. I will spin a v2.
} ret = reset_get_bulk(dev, &reset_bulk); if (!ret)@@ -535,9 +520,21 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) { struct ns16550_platdata *plat = dev->platdata; const u32 port_type = dev_get_driver_data(dev);
fdt_addr_t addr; struct clk clk; int err;addr = dev_read_addr(dev);if (addr != FDT_ADDR_T_NONE) {+#ifdef CONFIG_SYS_NS16550_PORT_MAPPED
plat->base = addr;+#else
plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE);+#endif
} else if (!device_is_on_pci_bus(dev)) {return -EINVAL;}plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0); plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0); plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);-- 2.7.4
Tested-by: Wolfgang Wallner wolfgang.wallner@br-automation.com Tested on an Intel Apollo Lake based board booting with Coreboot and using U-Boot as payload.
Thanks for testing!
Reviewed-by: Wolfgang Wallner wolfgang.wallner@br-automation.com
Regards, Bin