| Message ID | 20200403084014.51960-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()" | expand |
On Fri, Apr 3, 2020 at 11:40 AM Andy Shevchenko <andriy.shevchenko at linux.intel.com> wrote: > > The commit 720f9e1fdb0c ("Move PCI access from ofdata_to_platdata() to > probe()") while doing formally a right thing, actually brings a regression > to the drivers that would like to pre-initialize hardware before calling > ns16550_serial_probe(). In particular, the code, which gets moved out, > is responsible for getting the address of the hardware. > > The commit breaks serial console on the Intel Edison, for example. > > Since we are very close to the release, the quick fix is to revert the > culprit commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > Note, it doesn't affect SoCFPGA case. > As mentioned in another email, I'll glad to test a better solution if it pops up in time. > Cc: Wolfgang Wallner <wolfgang.wallner at br-automation.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com> > --- > v3: drop wrong patch, better explanation in the commit message > drivers/serial/ns16550.c | 40 ++++++++++++---------------------------- > 1 file changed, 12 insertions(+), 28 deletions(-) > > diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c > index c1b303ffcb..1fcbc35015 100644 > --- a/drivers/serial/ns16550.c > +++ b/drivers/serial/ns16550.c > @@ -479,40 +479,12 @@ 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 *const com_port = dev_get_priv(dev); > struct reset_ctl_bulk reset_bulk; > 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 > - > ret = reset_get_bulk(dev, &reset_bulk); > if (!ret) > reset_deassert_bulk(&reset_bulk); > @@ -535,9 +507,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; > > + /* try Processor Local Bus device first */ > + 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 > + > 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.25.1 >
Hi Andy,
-----"Andy Shevchenko" <andriy.shevchenko at linux.intel.com> schrieb: -----
The commit 720f9e1fdb0c ("Move PCI access from ofdata_to_platdata() to
probe()") while doing formally a right thing, actually brings a regression
to the drivers that would like to pre-initialize hardware before calling
ns16550_serial_probe(). In particular, the code, which gets moved out,
is responsible for getting the address of the hardware.
The commit breaks serial console on the Intel Edison, for example.
Since we are very close to the release, the quick fix is to revert the
culprit commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c.
Note, it doesn't affect SoCFPGA case.
Cc: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
---
v3: drop wrong patch, better explanation in the commit message
drivers/serial/ns16550.c | 40 ++++++++++++----------------------------
1 file changed, 12 insertions(+), 28 deletions(-)
Reviewed-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com>
Hi Wolfgang, On Fri, Apr 3, 2020 at 5:02 PM Wolfgang Wallner <wolfgang.wallner at br-automation.com> wrote: > > Hi Andy, > > -----"Andy Shevchenko" <andriy.shevchenko at linux.intel.com> schrieb: ----- > > The commit 720f9e1fdb0c ("Move PCI access from ofdata_to_platdata() to > probe()") while doing formally a right thing, actually brings a regression > to the drivers that would like to pre-initialize hardware before calling > ns16550_serial_probe(). In particular, the code, which gets moved out, > is responsible for getting the address of the hardware. > > The commit breaks serial console on the Intel Edison, for example. > > Since we are very close to the release, the quick fix is to revert the > culprit commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > Note, it doesn't affect SoCFPGA case. > > Cc: Wolfgang Wallner <wolfgang.wallner at br-automation.com> > Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com> > --- > v3: drop wrong patch, better explanation in the commit message > drivers/serial/ns16550.c | 40 ++++++++++++---------------------------- > 1 file changed, 12 insertions(+), 28 deletions(-) > > Reviewed-by: Wolfgang Wallner <wolfgang.wallner at br-automation.com> > We can't revert as that will put PCI based ns16550 in a broken state again. I've sent a patch to fix it. Please have a try. Regards, Bin
On Fri, Apr 3, 2020 at 1:08 PM Bin Meng <bmeng.cn at gmail.com> wrote: > On Fri, Apr 3, 2020 at 5:02 PM Wolfgang Wallner > <wolfgang.wallner at br-automation.com> wrote: > We can't revert as that will put PCI based ns16550 in a broken state again. > > I've sent a patch to fix it. Please have a try. Just did, thank you!
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index c1b303ffcb..1fcbc35015 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -479,40 +479,12 @@ 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 *const com_port = dev_get_priv(dev); struct reset_ctl_bulk reset_bulk; 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 - ret = reset_get_bulk(dev, &reset_bulk); if (!ret) reset_deassert_bulk(&reset_bulk); @@ -535,9 +507,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; + /* try Processor Local Bus device first */ + 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 + 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);
The commit 720f9e1fdb0c ("Move PCI access from ofdata_to_platdata() to probe()") while doing formally a right thing, actually brings a regression to the drivers that would like to pre-initialize hardware before calling ns16550_serial_probe(). In particular, the code, which gets moved out, is responsible for getting the address of the hardware. The commit breaks serial console on the Intel Edison, for example. Since we are very close to the release, the quick fix is to revert the culprit commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. Note, it doesn't affect SoCFPGA case. Cc: Wolfgang Wallner <wolfgang.wallner at br-automation.com> Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com> --- v3: drop wrong patch, better explanation in the commit message drivers/serial/ns16550.c | 40 ++++++++++++---------------------------- 1 file changed, 12 insertions(+), 28 deletions(-)