[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Andy Shevchenko]

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);
-- 
2.25.1

[PATCH v2 2/2] dm: core: Partial revert of "Allocate parent data separate from probing parent"

[Andy Shevchenko]

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;
-- 
2.25.1

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Bin Meng]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Simon Glass]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Simon Glass]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Bin Meng]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Tom Rini]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200402/a337622c/attachment.sig>

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Ang, Chee Hong]

> 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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Ang, Chee Hong]

> 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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[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?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Ang, Chee Hong]

> 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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Simon Glass]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Andy Shevchenko]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[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



-- 
With Best Regards,
Andy Shevchenko

Antwort: Re: [PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Wolfgang Wallner]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Bin Meng]

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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Simon Glass]

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 at 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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Bin Meng]

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 at 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

[PATCH v2 1/2] serial: ns16550: Revert "Move PCI access from ofdata_to_platdata() to probe()"

[Simon Glass]

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 at 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