On Fri, Apr 03, 2020 at 05:46:19AM -0700, Bin Meng wrote:
> 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 <[email protected]>
> Signed-off-by: Bin Meng <[email protected]>
> Tested-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Wolfgang Wallner <[email protected]>
> Tested-by: Wolfgang Wallner <[email protected]>
...
> - 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;
(1) for below.
...
> + addr = devfdt_get_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
And now is the question why we can't use above as a helper in both cases
(either with check for address correctness or without)?
> + }
...
> + 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;
> + }
Something like
addr = dev_read_addr(dev);
ret = ns16550_assign_base(addr, plat);
if (ret && !device_is_on_pci_bus(...))
return ret;
--
With Best Regards,
Andy Shevchenko
