Hi Wolfgang,
On Fri, Apr 3, 2020 at 4:26 PM Wolfgang Wallner wolfgang.wallner@br-automation.com wrote:
Hi Andy, Bin,
-----"Andy Shevchenko" andy.shevchenko@gmail.com schrieb: ----- On Thu, Apr 2, 2020 at 7:55 AM Bin Meng bmeng.cn@gmail.com wrote:
On Thu, Apr 2, 2020 at 1:55 AM Simon Glass sjg@chromium.org wrote:
On Wed, 1 Apr 2020 at 11:39, Andy Shevchenko andy.shevchenko@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@gmail.com wrote: > On Wed, Apr 1, 2020 at 5:32 PM Bin Meng bmeng.cn@gmail.com wrote: > > On Wed, Apr 1, 2020 at 9:58 PM Andy Shevchenko > > andriy.shevchenko@linux.intel.com wrote: > > > > > > The commit breaks serial console on the Intel Edison. > > > > > > This reverts commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > > > > > Signed-off-by: Andy Shevchenko andriy.shevchenko@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?
commit 82de42fa14682d408da935adfb0f935354c5008f Author: Simon Glass sjg@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@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.
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.
Regards, Bin