| Message ID | 20200401135759.13197-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2,1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()" | expand |
Hi Andy, 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! Regards, Bin
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.
Hi Andy, 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. Does it enable the debug UART? I have one but have not powered it on for a while, nor added it to my lab. Regards, Simon
On Wed, Apr 01, 2020 at 10:56:26AM -0600, Simon Glass wrote: > Hi Andy, > > 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.
Hi Andy, 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: > > Hi Andy, > > > > 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. - Simon
Hi Simon, Andy, On Thu, Apr 2, 2020 at 1:55 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Andy, > > 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: > > > Hi Andy, > > > > > > 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. Regards, Bin
On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng wrote: > Hi Simon, Andy, > > On Thu, Apr 2, 2020 at 1:55 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Andy, > > > > 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: > > > > Hi Andy, > > > > > > > > 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. Adding SoCFPGA folks to this thread as the first commit (82de42fa1468) is also breaking platforms there and then their fix for that problem is also causing problems, if I follow right.
> On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng wrote: > > Hi Simon, Andy, > > > > On Thu, Apr 2, 2020 at 1:55 AM Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Andy, > > > > > > 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: > > > > > Hi Andy, > > > > > > > > > > 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. > > Adding SoCFPGA folks to this thread as the first commit (82de42fa1468) is also > breaking platforms there and then their fix for that problem is also causing > problems, if I follow right. > > -- > Tom Yes. This commit (82de42fa1468) breaks our A10 platform. I did submit similar patch to move some init code from ofdata_to_platdata() to probe() for our clock driver as well. Check out the thread here: https://lists.denx.de/pipermail/u-boot/2020-April/405252.html
On Thu, Apr 2, 2020 at 7:28 PM Ang, Chee Hong <chee.hong.ang@intel.com> wrote: > > On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng 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. > > > > Adding SoCFPGA folks to this thread as the first commit (82de42fa1468) is also > > breaking platforms there and then their fix for that problem is also causing > > problems, if I follow right. > Yes. This commit (82de42fa1468) breaks our A10 platform. > I did submit similar patch to move some init code from ofdata_to_platdata() to probe() for our clock driver as well. > Check out the thread here: > https://lists.denx.de/pipermail/u-boot/2020-April/405252.html I see half or less of the messages in the thread by above link. Can you summarize what should have been done in order to fix this?
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. 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!
> On Thu, Apr 2, 2020 at 7:28 PM Ang, Chee Hong <chee.hong.ang@intel.com> > wrote: > > > On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng 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. > > > > > > Adding SoCFPGA folks to this thread as the first commit > > > (82de42fa1468) is also breaking platforms there and then their fix > > > for that problem is also causing problems, if I follow right. > > > Yes. This commit (82de42fa1468) breaks our A10 platform. > > I did submit similar patch to move some init code from ofdata_to_platdata() to > probe() for our clock driver as well. > > Check out the thread here: > > https://lists.denx.de/pipermail/u-boot/2020-April/405252.html > > I see half or less of the messages in the thread by above link. > Can you summarize what should have been done in order to fix this? 1) Fix in the driver (broken by this commit: 82de42fa1468) by moving some code from ofdata_to_platdata() to probe(). 2) Fix the DM core. Simon and Marek were discussing about these 2 options. Perhaps Simon can help shed some light here. > > -- > With Best Regards, > Andy Shevchenko
+Cc: Chee Hong On Thu, Apr 2, 2020 at 10:09 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > 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. > > 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! > > -- > With Best Regards, > Andy Shevchenko
On Fri, Apr 3, 2020 at 6:56 AM Ang, Chee Hong <chee.hong.ang@intel.com> wrote: > > > On Thu, Apr 2, 2020 at 7:28 PM Ang, Chee Hong <chee.hong.ang@intel.com> > > wrote: > > > > On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng 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 > > > > > > > > > > > This reverts commit > > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > > > Adding SoCFPGA folks to this thread as the first commit > > > > (82de42fa1468) is also breaking platforms there and then their fix > > > > for that problem is also causing problems, if I follow right. > > > > > Yes. This commit (82de42fa1468) breaks our A10 platform. > > > I did submit similar patch to move some init code from ofdata_to_platdata() to > > probe() for our clock driver as well. > > > Check out the thread here: > > > https://lists.denx.de/pipermail/u-boot/2020-April/405252.html > > > > I see half or less of the messages in the thread by above link. > > Can you summarize what should have been done in order to fix this? > 1) Fix in the driver (broken by this commit: 82de42fa1468) by moving some code from ofdata_to_platdata() to probe(). > 2) Fix the DM core. > Simon and Marek were discussing about these 2 options. > Perhaps Simon can help shed some light here. I have a question. Does revert of 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c makes any difference to SoCFPGA?
> On Fri, Apr 3, 2020 at 6:56 AM Ang, Chee Hong <chee.hong.ang@intel.com> > wrote: > > > > > On Thu, Apr 2, 2020 at 7:28 PM Ang, Chee Hong > > > <chee.hong.ang@intel.com> > > > wrote: > > > > > On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng 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 > > > > > > > > > > > > > This reverts commit > > > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > > > > > Adding SoCFPGA folks to this thread as the first commit > > > > > (82de42fa1468) is also breaking platforms there and then their > > > > > fix for that problem is also causing problems, if I follow right. > > > > > > > Yes. This commit (82de42fa1468) breaks our A10 platform. > > > > I did submit similar patch to move some init code from > > > > ofdata_to_platdata() to > > > probe() for our clock driver as well. > > > > Check out the thread here: > > > > https://lists.denx.de/pipermail/u-boot/2020-April/405252.html > > > > > > I see half or less of the messages in the thread by above link. > > > Can you summarize what should have been done in order to fix this? > > 1) Fix in the driver (broken by this commit: 82de42fa1468) by moving some > code from ofdata_to_platdata() to probe(). > > 2) Fix the DM core. > > Simon and Marek were discussing about these 2 options. > > Perhaps Simon can help shed some light here. > > I have a question. Does revert of > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c makes any difference to > SoCFPGA? This commit: 82de42fa1468 break our SoCFPGA A10 clock driver. This commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c does not affect our SoCFPGA platforms. We just want to know which way to go. Fixing our A10 clock driver or fix the DM core. Seems like this commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c go with option 1 (fixing the driver). > > -- > With Best Regards, > Andy Shevchenko
On Fri, Apr 3, 2020 at 10:56 AM Ang, Chee Hong <chee.hong.ang@intel.com> wrote: > > On Fri, Apr 3, 2020 at 6:56 AM Ang, Chee Hong <chee.hong.ang@intel.com> > > wrote: > > > > > > > On Thu, Apr 2, 2020 at 7:28 PM Ang, Chee Hong > > > > <chee.hong.ang@intel.com> > > > > wrote: > > > > > > On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng 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 > > > > > > > > > > > > > > > This reverts commit > > > > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > > > > > > > Adding SoCFPGA folks to this thread as the first commit > > > > > > (82de42fa1468) is also breaking platforms there and then their > > > > > > fix for that problem is also causing problems, if I follow right. > > > > > > > > > Yes. This commit (82de42fa1468) breaks our A10 platform. > > > > > I did submit similar patch to move some init code from > > > > > ofdata_to_platdata() to > > > > probe() for our clock driver as well. > > > > > Check out the thread here: > > > > > https://lists.denx.de/pipermail/u-boot/2020-April/405252.html > > > > > > > > I see half or less of the messages in the thread by above link. > > > > Can you summarize what should have been done in order to fix this? > > > 1) Fix in the driver (broken by this commit: 82de42fa1468) by moving some > > code from ofdata_to_platdata() to probe(). > > > 2) Fix the DM core. > > > Simon and Marek were discussing about these 2 options. > > > Perhaps Simon can help shed some light here. > > > > I have a question. Does revert of > > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c makes any difference to > > SoCFPGA? > This commit: 82de42fa1468 break our SoCFPGA A10 clock driver. > This commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c does not affect our SoCFPGA platforms. > We just want to know which way to go. Fixing our A10 clock driver or fix the DM core. > Seems like this commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c go with option 1 (fixing the driver). I dunno what 720f9e is actually fixing, it breaks definitely the ordering. So, I'm going to send a v3 of revert of 720f9e since it is not related to SoCFPGA.
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." @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
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
On Fri, Apr 3, 2020 at 11:35 AM Bin Meng <bmeng.cn@gmail.com> wrote: > On Fri, Apr 3, 2020 at 4:26 PM Wolfgang Wallner > <wolfgang.wallner@br-automation.com> wrote: > > > -----"Andy Shevchenko" <andy.shevchenko@gmail.com> schrieb: ----- > > > On Thu, Apr 2, 2020 at 7:55 AM Bin Meng <bmeng.cn@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 <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. 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.
Hi Chee Hong, On Fri, 3 Apr 2020 at 01:56, Ang, Chee Hong <chee.hong.ang@intel.com> wrote: > > > On Fri, Apr 3, 2020 at 6:56 AM Ang, Chee Hong <chee.hong.ang@intel.com> > > wrote: > > > > > > > On Thu, Apr 2, 2020 at 7:28 PM Ang, Chee Hong > > > > <chee.hong.ang@intel.com> > > > > wrote: > > > > > > On Thu, Apr 02, 2020 at 12:55:14PM +0800, Bin Meng 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 > > > > > > > > > > > > > > > This reverts commit > > > > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c. > > > > > > > > Adding SoCFPGA folks to this thread as the first commit > > > > > > (82de42fa1468) is also breaking platforms there and then their > > > > > > fix for that problem is also causing problems, if I follow right. > > > > > > > > > Yes. This commit (82de42fa1468) breaks our A10 platform. > > > > > I did submit similar patch to move some init code from > > > > > ofdata_to_platdata() to > > > > probe() for our clock driver as well. > > > > > Check out the thread here: > > > > > https://lists.denx.de/pipermail/u-boot/2020-April/405252.html > > > > > > > > I see half or less of the messages in the thread by above link. > > > > Can you summarize what should have been done in order to fix this? > > > 1) Fix in the driver (broken by this commit: 82de42fa1468) by moving some > > code from ofdata_to_platdata() to probe(). > > > 2) Fix the DM core. > > > Simon and Marek were discussing about these 2 options. > > > Perhaps Simon can help shed some light here. > > > > I have a question. Does revert of > > 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c makes any difference to > > SoCFPGA? > This commit: 82de42fa1468 break our SoCFPGA A10 clock driver. > This commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c does not affect our SoCFPGA platforms. > We just want to know which way to go. Fixing our A10 clock driver or fix the DM core. > Seems like this commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c go with option 1 (fixing the driver). For now you should fix your clock driver as it is too late to make a change to the DM core. For your particular case, running ofdata_to_platdata() on parent devices might be enough to get your clock driver running, so after the release we can figure that out. Regards, SImon
Hi Bin, On Fri, 3 Apr 2020 at 02:35, Bin Meng <bmeng.cn@gmail.com> wrote: > > 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? Yes, please see the cover letter for the series: https://www.mail-archive.com/u-boot@lists.denx.de/msg352249.html > > 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 Regards, Simon
Hi Simon, On Mon, Apr 6, 2020 at 11:43 AM Simon Glass <sjg@chromium.org> wrote: > > Hi Bin, > > On Fri, 3 Apr 2020 at 02:35, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > 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? > > 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
Hi Bin, On Sun, 5 Apr 2020 at 22:13, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Simon, > > On Mon, Apr 6, 2020 at 11:43 AM Simon Glass <sjg@chromium.org> wrote: > > > > Hi Bin, > > > > On Fri, 3 Apr 2020 at 02:35, Bin Meng <bmeng.cn@gmail.com> wrote: > > > > > > 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? > > > > 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
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index c1b303ffcb..1fcbc35015 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -479,40 +479,12 @@ static int ns16550_serial_getinfo(struct udevice *dev, return 0; } -#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) -static int ns1655_serial_set_base_addr(struct udevice *dev) -{ - fdt_addr_t addr; - struct ns16550_platdata *plat; - - plat = dev_get_platdata(dev); - - addr = dev_read_addr_pci(dev); - if (addr == FDT_ADDR_T_NONE) - return -EINVAL; - -#ifdef CONFIG_SYS_NS16550_PORT_MAPPED - plat->base = addr; -#else - plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE); -#endif - - return 0; -} -#endif - int ns16550_serial_probe(struct udevice *dev) { struct NS16550 *const com_port = dev_get_priv(dev); struct reset_ctl_bulk reset_bulk; int ret; -#if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA) - ret = ns1655_serial_set_base_addr(dev); - if (ret) - return ret; -#endif - ret = reset_get_bulk(dev, &reset_bulk); if (!ret) reset_deassert_bulk(&reset_bulk); @@ -535,9 +507,21 @@ int ns16550_serial_ofdata_to_platdata(struct udevice *dev) { struct ns16550_platdata *plat = dev->platdata; const u32 port_type = dev_get_driver_data(dev); + fdt_addr_t addr; struct clk clk; int err; + /* try Processor Local Bus device first */ + addr = dev_read_addr_pci(dev); + if (addr == FDT_ADDR_T_NONE) + return -EINVAL; + +#ifdef CONFIG_SYS_NS16550_PORT_MAPPED + plat->base = addr; +#else + plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE); +#endif + plat->reg_offset = dev_read_u32_default(dev, "reg-offset", 0); plat->reg_shift = dev_read_u32_default(dev, "reg-shift", 0); plat->reg_width = dev_read_u32_default(dev, "reg-io-width", 1);
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(-)