[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"
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(-)
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);
This reverts commit 82de42fa1468 ("dm: core: Allocate parent data separate from probing parent") as a being a culprit for Apollo Lake breakage.
Suggested-by: Wolfgang Wallner wolfgang.wallner@br-automation.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- drivers/core/device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/core/device.c b/drivers/core/device.c index 89ea820d48..400767d9bd 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -392,10 +392,6 @@ int device_probe(struct udevice *dev) drv = dev->driver; assert(drv);
- ret = device_ofdata_to_platdata(dev); - if (ret) - goto fail; - /* Ensure all parents are probed */ if (dev->parent) { ret = device_probe(dev->parent); @@ -412,6 +408,10 @@ int device_probe(struct udevice *dev) return 0; }
+ ret = device_ofdata_to_platdata(dev); + if (ret) + goto fail; + seq = uclass_resolve_seq(dev); if (seq < 0) { ret = seq;
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: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
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?
- Fix in the driver (broken by this commit: 82de42fa1468) by moving some code from ofdata_to_platdata() to probe().
- 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?
- Fix in the driver (broken by this commit: 82de42fa1468) by moving some
code from ofdata_to_platdata() to probe().
- 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?
- Fix in the driver (broken by this commit: 82de42fa1468) by moving some
code from ofdata_to_platdata() to probe().
- 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 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?
- Fix in the driver (broken by this commit: 82de42fa1468) by moving some
code from ofdata_to_platdata() to probe().
- 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
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!
+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
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 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.
Regards, Simon
participants (7)
-
Andy Shevchenko -
Andy Shevchenko -
Ang, Chee Hong -
Bin Meng -
Simon Glass -
Tom Rini -
Wolfgang Wallner