Hi Simon, On Mon, Apr 6, 2020 at 11:43 AM Simon Glass <s...@chromium.org> wrote: > > Hi Bin, > > On Fri, 3 Apr 2020 at 02:35, Bin Meng <bmeng...@gmail.com> wrote: > > > > Hi Wolfgang, > > > > On Fri, Apr 3, 2020 at 4:26 PM Wolfgang Wallner > > <wolfgang.wall...@br-automation.com> wrote: > > > > > > > > > Hi Andy, Bin, > > > > > > > -----"Andy Shevchenko" <andy.shevche...@gmail.com> schrieb: ----- > > > > On Thu, Apr 2, 2020 at 7:55 AM Bin Meng <bmeng...@gmail.com> wrote: > > > > > On Thu, Apr 2, 2020 at 1:55 AM Simon Glass <s...@chromium.org> wrote: > > > > > > On Wed, 1 Apr 2020 at 11:39, Andy Shevchenko > > > > > > <andy.shevche...@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.shevche...@gmail.com> wrote: > > > > > > > > > On Wed, Apr 1, 2020 at 5:32 PM Bin Meng <bmeng...@gmail.com> > > > > > > > > > wrote: > > > > > > > > > > On Wed, Apr 1, 2020 at 9:58 PM Andy Shevchenko > > > > > > > > > > <andriy.shevche...@linux.intel.com> wrote: > > > > > > > > > > > > > > > > > > > > > > The commit breaks serial console on the Intel Edison. > > > > > > > > > > > > > > > > > > > > > > This reverts commit > > > > > > > > > > > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Andy Shevchenko > > > > > > > > > > > <andriy.shevche...@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? [1] http://patchwork.ozlabs.org/patch/1266326/ Regards, Bin