[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Andy Shevchenko]

On Fri, Apr 3, 2020 at 11:35 AM Bin Meng <bmeng.cn at gmail.com> wrote:
> On Fri, Apr 3, 2020 at 4:26 PM Wolfgang Wallner
> <wolfgang.wallner at br-automation.com> wrote:
> > > -----"Andy Shevchenko" <andy.shevchenko at gmail.com> schrieb: -----
> > > On Thu, Apr 2, 2020 at 7:55 AM Bin Meng <bmeng.cn at gmail.com> wrote:

...

> > > I think I understand what happened, and Wolfgang's patch *is* a culprit.
> > >
> > > In serial_intel_mid.c we setup a divisor before probing the actual
> > > device. Now, since the address retrieving has been moved further in
> > > the initialization, we are writing to unknown registers and thus don't
> > > properly initialize hardware.
> >
> > Yes, you are right, mid_serial_probe() relies on plat->base being set by
> > ns16550_serial_ofdata_to_platdata().
> >
> > I was not aware that other drivers rely on ns16550 in this way.
> > And now that I know I see that there are few more:
> >
> >   $ grep -r -l --include='*.c' -e ns16550_serial_probe -e ns16550_serial_ofdata_to_platdata
> >   drivers/serial/serial_intel_mid.c
> >   drivers/serial/serial_rockchip.c
> >   drivers/serial/serial_omap.c
> >   drivers/serial/ns16550.c
> >   arch/x86/cpu/apollolake/uart.c
> >   arch/x86/cpu/slimbootloader/serial.c
> >
> > This means my patch has a wider impact than what I have taken care of.
> >
> > @Bin: I'm fine with reverting 720f9e1 for now. I will come back with a
> > new revision of this patch when I had the time to look at the other impacted
> > drivers.
> >
> > But reverting 720f9e1 is IMHO only a temporary workaround, as the underlying
> > problem (possible PCI access in ofdata_to_platdata()) should be fixed anyway,
> > at least my interpretation of the comment in drivers/pci/pci-uclass.c is that
> > this PCI access should not be there:
> >
> >     "A common cause of this problem is that this function is called in the
> >     ofdata_to_platdata() method of @dev. Accessing the PCI bus in that
> >     method is not allowed, since it has not yet been probed. To fix this,
> >     move that access to the probe() method of @dev instead."
> >
>
> Yes, when I looked at this, I wonder why the first commit was
> introduced. Simon, could you please explain more about the DM changes
> below?
>
> commit 82de42fa14682d408da935adfb0f935354c5008f
> Author: Simon Glass <sjg at chromium.org>
> Date:   Sun Dec 29 21:19:18 2019 -0700
>
>     dm: core: Allocate parent data separate from probing parent
>
>     At present the parent is probed before the child's ofdata_to_platdata()
>     method is called. Adjust the logic slightly so that probing parents is
>     not done until afterwards.
>
>     Signed-off-by: Simon Glass <sjg at chromium.org>
>
> > @Andy: Regarding serial_intel_mid: I don't know the hardware, but would it be
> > possible to move the register accesses after the call to
> > ns16550_serial_probe()?
> >
>
> I suspect this does not work.

I didn't check the details, but have same feelings.

> >    mid_writel(plat, UART_MUL, 96);
> >    mid_writel(plat, UART_DIV, 125);
> >    mid_writel(plat, UART_PS, 16);
> >
> >    return ns16550_serial_probe(dev);
> >
> > > So, the proper fix would be in my opinion to introduce a call back or
> > > some other way to make ordering possible, like registering hw_init
> > > callback in probe which will be called in the ns16550, or even do this
> > > as a callback for all serial class drivers.
> > >
> > > I don't dare to dive into this anticipating that crap will hit the fan.
> > > So, please, who is knowing serial code, fix this asap!
> >
>
> I am currently working on a patch. Will send out for Andy and you for
> testing soon.

I just sent a v3 of revert, but I'll glad to test better solution.

-- 
With Best Regards,
Andy Shevchenko