On Fri, Apr 3, 2020 at 11:35 AM Bin Meng <bmeng...@gmail.com> wrote: > On Fri, Apr 3, 2020 at 4:26 PM Wolfgang Wallner > <wolfgang.wall...@br-automation.com> wrote: > > > -----"Andy Shevchenko" <andy.shevche...@gmail.com> schrieb: ----- > > > On Thu, Apr 2, 2020 at 7:55 AM Bin Meng <bmeng...@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 <s...@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 <s...@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