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

Mon Apr 6 16:24:39 CEST 2020

Hi Bin,

On Sun, 5 Apr 2020 at 22:13, Bin Meng <bmeng.cn at gmail.com> wrote:
>
> Hi Simon,
>
> On Mon, Apr 6, 2020 at 11:43 AM Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Bin,
> >
> > On Fri, 3 Apr 2020 at 02:35, Bin Meng <bmeng.cn at gmail.com> wrote:
> > >
> > > Hi Wolfgang,
> > >
> > > On Fri, Apr 3, 2020 at 4:26 PM Wolfgang Wallner
> > > <wolfgang.wallner at br-automation.com> wrote:
> > > >
> > > >
> > > > 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."
> > > >
> > >
> > > 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?
> >
> > Yes, please see the cover letter for the series:
> >
> > https://www.mail-archive.com/u-boot@lists.denx.de/msg352249.html
> >
>
> Thanks. But still, as I described in my fix commit [1], there are
> chances that any PCI based ns16550 driver might break due to the
> ordering changes. Any ideas to improve that?

Hmm it is not very nice.

But I am not sure what the solution can be. With PCI we do need to
probe the bus before we can access devices on it, right?

Would it help to split up ns16550 probe into two functions that people can call?

Another thought is that we could have a driver flag for PCI which
tells DM to probe it if any devices reads its ofdata? I'm not sure how
I could justify that, but it might work.

>
> [1] http://patchwork.ozlabs.org/patch/1266326/

Regards,
Simon