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

[Wolfgang Wallner]

Hi Andy, Bin,

> -----"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:
> > On Thu, Apr 2, 2020 at 1:55 AM Simon Glass <sjg at chromium.org> wrote:
> > > On Wed, 1 Apr 2020 at 11:39, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > > > On Wed, Apr 01, 2020 at 10:56:26AM -0600, Simon Glass wrote:
> > > > > On Wed, 1 Apr 2020 at 08:45, Andy Shevchenko <andy.shevchenko at gmail.com> wrote:
> > > > > > On Wed, Apr 1, 2020 at 5:32 PM Bin Meng <bmeng.cn at gmail.com> wrote:
> > > > > > > On Wed, Apr 1, 2020 at 9:58 PM Andy Shevchenko
> > > > > > > <andriy.shevchenko at linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > The commit breaks serial console on the Intel Edison.
> > > > > > > >
> > > > > > > > This reverts commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c.
> > > > > > > >
> > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko at linux.intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/serial/ns16550.c | 40 ++++++++++++----------------------------
> > > > > > > >  1 file changed, 12 insertions(+), 28 deletions(-)
> > > > > > > >
> > > > > > >
> > > > > > > Could you please spend some time to investigate why this breaks Intel Edison?
> > > > > > >
> > > > > > > Reverting this patch would mean we break other boards too as
> > > > > > > Wolfgang's patch wanted to fix the breakage in the first place. Much
> > > > > > > appreciated!
> > > > > >
> > > > > > I guess I'm wrong person here. The DM code is a complete black box to me.
> > > > > > Nevertheless, I may test any provided fix / debug / etc patch by request.
> > > > > >
> > > > > > And I think it's fair to investigate by the one who made a regression
> > > > > > in the first place.
> > > > >
> > > > > Given that we have conflicting breakages, we need to debug Edison.
> > > >
> > > > I would glad to test any suggested change or debug patch!
> > > >
> > > > > Does it enable the debug UART?
> > > >
> > > > If I am not mistaken, it does not.
> > >
> > > Looks like you are right. If you know the address you could do that -
> > > see minnowmax for an example.
> >
> > Please suggest what's the best approach to proceed.
> 
> 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."

@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()?

   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!

regards, Wolfgang