All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()
@ 2020-02-18 12:34 Wolfgang Wallner
  2020-02-18 18:31 ` Simon Glass
  2020-02-27  9:36 ` Bin Meng
  0 siblings, 2 replies; 6+ messages in thread
From: Wolfgang Wallner @ 2020-02-18 12:34 UTC (permalink / raw)
  To: u-boot

Currently the ofdata_to_platdata() method calls dev_read_addr_pci(),
which potentially accesses the parent PCI bus. If this happens before
the parent PCI bus is probed the resulting address will be wrong.

This behavior was triggered by commit 82de42fa1468 ("dm: core:
Allocate parent data separate from probing parent").

According to a comment in drivers/pci/pci-uclass.c [1] accessing
the PCI parent bus in ofdata_to_platdata() is not allowed, and the
access should be moved to the probe() function.

Move the call to dev_read_addr_pci() and the related handling of the
'addr' value from the ofdata_to_platdata() to the probe() method.

While moving the code, the comment /* try Processor Local Bus device
first */ was dropped. It was initially added with commit 3db886a5bf38
("serial: ns16550: Support ns16550 compatible pci uart devices") and
later made obsolete with commit 33c215af4b9d ("dm: pci: Add a function
to read a PCI BAR").

[1] Comment in drivers/pci/pci-uclass.c:
"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."

Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>

Fixes: 82de42fa1468 ("dm: core: Allocate parent data separate from
probing parent")

---
The discussion leading to this patch is located at
https://lists.denx.de/pipermail/u-boot/2020-February/399811.html

 drivers/serial/ns16550.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 1fcbc35015..07b32abd38 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -484,12 +484,24 @@ int ns16550_serial_probe(struct udevice *dev)
 	struct NS16550 *const com_port = dev_get_priv(dev);
 	struct reset_ctl_bulk reset_bulk;
 	int ret;
+	fdt_addr_t addr;
+
+	com_port->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
+	com_port->plat->base = addr;
+#else
+	com_port->plat->base = (unsigned long)map_physmem(addr, 0, MAP_NOCACHE);
+#endif
 
 	ret = reset_get_bulk(dev, &reset_bulk);
 	if (!ret)
 		reset_deassert_bulk(&reset_bulk);
 
-	com_port->plat = dev_get_platdata(dev);
 	NS16550_init(com_port, -1);
 
 	return 0;
@@ -507,21 +519,9 @@ 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.0

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()
  2020-02-18 12:34 [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe() Wolfgang Wallner
@ 2020-02-18 18:31 ` Simon Glass
  2020-02-27  9:36 ` Bin Meng
  1 sibling, 0 replies; 6+ messages in thread
From: Simon Glass @ 2020-02-18 18:31 UTC (permalink / raw)
  To: u-boot

On Tue, 18 Feb 2020 at 05:36, Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Currently the ofdata_to_platdata() method calls dev_read_addr_pci(),
> which potentially accesses the parent PCI bus. If this happens before
> the parent PCI bus is probed the resulting address will be wrong.
>
> This behavior was triggered by commit 82de42fa1468 ("dm: core:
> Allocate parent data separate from probing parent").
>
> According to a comment in drivers/pci/pci-uclass.c [1] accessing
> the PCI parent bus in ofdata_to_platdata() is not allowed, and the
> access should be moved to the probe() function.
>
> Move the call to dev_read_addr_pci() and the related handling of the
> 'addr' value from the ofdata_to_platdata() to the probe() method.
>
> While moving the code, the comment /* try Processor Local Bus device
> first */ was dropped. It was initially added with commit 3db886a5bf38
> ("serial: ns16550: Support ns16550 compatible pci uart devices") and
> later made obsolete with commit 33c215af4b9d ("dm: pci: Add a function
> to read a PCI BAR").
>
> [1] Comment in drivers/pci/pci-uclass.c:
> "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."
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
>
> Fixes: 82de42fa1468 ("dm: core: Allocate parent data separate from
> probing parent")
>
> ---
> The discussion leading to this patch is located at
> https://lists.denx.de/pipermail/u-boot/2020-February/399811.html
>
>  drivers/serial/ns16550.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()
  2020-02-18 12:34 [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe() Wolfgang Wallner
  2020-02-18 18:31 ` Simon Glass
@ 2020-02-27  9:36 ` Bin Meng
  2020-02-28  7:50   ` Bin Meng
  1 sibling, 1 reply; 6+ messages in thread
From: Bin Meng @ 2020-02-27  9:36 UTC (permalink / raw)
  To: u-boot

On Tue, Feb 18, 2020 at 8:36 PM Wolfgang Wallner
<wolfgang.wallner@br-automation.com> wrote:
>
> Currently the ofdata_to_platdata() method calls dev_read_addr_pci(),
> which potentially accesses the parent PCI bus. If this happens before
> the parent PCI bus is probed the resulting address will be wrong.
>
> This behavior was triggered by commit 82de42fa1468 ("dm: core:
> Allocate parent data separate from probing parent").
>
> According to a comment in drivers/pci/pci-uclass.c [1] accessing
> the PCI parent bus in ofdata_to_platdata() is not allowed, and the
> access should be moved to the probe() function.
>
> Move the call to dev_read_addr_pci() and the related handling of the
> 'addr' value from the ofdata_to_platdata() to the probe() method.
>
> While moving the code, the comment /* try Processor Local Bus device
> first */ was dropped. It was initially added with commit 3db886a5bf38
> ("serial: ns16550: Support ns16550 compatible pci uart devices") and
> later made obsolete with commit 33c215af4b9d ("dm: pci: Add a function
> to read a PCI BAR").
>
> [1] Comment in drivers/pci/pci-uclass.c:
> "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."
>
> Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
>
> Fixes: 82de42fa1468 ("dm: core: Allocate parent data separate from
> probing parent")
>
> ---
> The discussion leading to this patch is located at
> https://lists.denx.de/pipermail/u-boot/2020-February/399811.html
>
>  drivers/serial/ns16550.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

This fixed the boot failure of Intel Galileo

Tested on Intel Galileo
Tested-by: Bin Meng <bmeng.cn@gmail.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()
  2020-02-27  9:36 ` Bin Meng
@ 2020-02-28  7:50   ` Bin Meng
  2020-02-28 12:34     ` Bin Meng
  2020-02-28 13:59     ` Antwort: " Wolfgang Wallner
  0 siblings, 2 replies; 6+ messages in thread
From: Bin Meng @ 2020-02-28  7:50 UTC (permalink / raw)
  To: u-boot

On Thu, Feb 27, 2020 at 5:36 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Tue, Feb 18, 2020 at 8:36 PM Wolfgang Wallner
> <wolfgang.wallner@br-automation.com> wrote:
> >
> > Currently the ofdata_to_platdata() method calls dev_read_addr_pci(),
> > which potentially accesses the parent PCI bus. If this happens before
> > the parent PCI bus is probed the resulting address will be wrong.
> >
> > This behavior was triggered by commit 82de42fa1468 ("dm: core:
> > Allocate parent data separate from probing parent").
> >
> > According to a comment in drivers/pci/pci-uclass.c [1] accessing
> > the PCI parent bus in ofdata_to_platdata() is not allowed, and the
> > access should be moved to the probe() function.
> >
> > Move the call to dev_read_addr_pci() and the related handling of the
> > 'addr' value from the ofdata_to_platdata() to the probe() method.
> >
> > While moving the code, the comment /* try Processor Local Bus device
> > first */ was dropped. It was initially added with commit 3db886a5bf38
> > ("serial: ns16550: Support ns16550 compatible pci uart devices") and
> > later made obsolete with commit 33c215af4b9d ("dm: pci: Add a function
> > to read a PCI BAR").
> >
> > [1] Comment in drivers/pci/pci-uclass.c:
> > "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."
> >
> > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> >
> > Fixes: 82de42fa1468 ("dm: core: Allocate parent data separate from
> > probing parent")
> >
> > ---
> > The discussion leading to this patch is located at
> > https://lists.denx.de/pipermail/u-boot/2020-February/399811.html
> >
> >  drivers/serial/ns16550.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>
> This fixed the boot failure of Intel Galileo
>
> Tested on Intel Galileo
> Tested-by: Bin Meng <bmeng.cn@gmail.com>

applied to u-boot-x86, thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()
  2020-02-28  7:50   ` Bin Meng
@ 2020-02-28 12:34     ` Bin Meng
  2020-02-28 13:59     ` Antwort: " Wolfgang Wallner
  1 sibling, 0 replies; 6+ messages in thread
From: Bin Meng @ 2020-02-28 12:34 UTC (permalink / raw)
  To: u-boot

Hi Wolfgang,

On Fri, Feb 28, 2020 at 3:50 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Thu, Feb 27, 2020 at 5:36 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Tue, Feb 18, 2020 at 8:36 PM Wolfgang Wallner
> > <wolfgang.wallner@br-automation.com> wrote:
> > >
> > > Currently the ofdata_to_platdata() method calls dev_read_addr_pci(),
> > > which potentially accesses the parent PCI bus. If this happens before
> > > the parent PCI bus is probed the resulting address will be wrong.
> > >
> > > This behavior was triggered by commit 82de42fa1468 ("dm: core:
> > > Allocate parent data separate from probing parent").
> > >
> > > According to a comment in drivers/pci/pci-uclass.c [1] accessing
> > > the PCI parent bus in ofdata_to_platdata() is not allowed, and the
> > > access should be moved to the probe() function.
> > >
> > > Move the call to dev_read_addr_pci() and the related handling of the
> > > 'addr' value from the ofdata_to_platdata() to the probe() method.
> > >
> > > While moving the code, the comment /* try Processor Local Bus device
> > > first */ was dropped. It was initially added with commit 3db886a5bf38
> > > ("serial: ns16550: Support ns16550 compatible pci uart devices") and
> > > later made obsolete with commit 33c215af4b9d ("dm: pci: Add a function
> > > to read a PCI BAR").
> > >
> > > [1] Comment in drivers/pci/pci-uclass.c:
> > > "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."
> > >
> > > Signed-off-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> > >
> > > Fixes: 82de42fa1468 ("dm: core: Allocate parent data separate from
> > > probing parent")
> > >
> > > ---
> > > The discussion leading to this patch is located at
> > > https://lists.denx.de/pipermail/u-boot/2020-February/399811.html
> > >
> > >  drivers/serial/ns16550.c | 26 +++++++++++++-------------
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > >
> >
> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> >
> > This fixed the boot failure of Intel Galileo
> >
> > Tested on Intel Galileo
> > Tested-by: Bin Meng <bmeng.cn@gmail.com>
>
> applied to u-boot-x86, thanks!

Unfortunately this patch breaks several ARM board:

+drivers/built-in.o: In function `ofnode_get_parent':
+drivers/core/ofnode.c:235: undefined reference to `fdt_parent_offset'
+drivers/built-in.o: In function `ofnode_read_simple_addr_cells':
+drivers/core/ofnode.c:717: undefined reference to `fdt_address_cells'
+drivers/built-in.o: In function `ofnode_read_simple_size_cells':
+drivers/core/ofnode.c:725: undefined reference to `fdt_size_cells'
+drivers/built-in.o: In function `ofnode_get_addr_size_index':
+drivers/core/ofnode.c:294: undefined reference to `fdtdec_get_addr_size_fixed'

See https://dev.azure.com/bmeng/GitHub/_build/results?buildId=158&view=logs&j=25fc9316-c681-5af2-a03f-b888fd665e84&t=3af932b6-18ad-575e-b0ea-0b31bb7c6c26

Would you please send a v2 to address this? thanks!

Regards,
Bin

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Antwort: Re: [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()
  2020-02-28  7:50   ` Bin Meng
  2020-02-28 12:34     ` Bin Meng
@ 2020-02-28 13:59     ` Wolfgang Wallner
  1 sibling, 0 replies; 6+ messages in thread
From: Wolfgang Wallner @ 2020-02-28 13:59 UTC (permalink / raw)
  To: u-boot

Hi Bin,

-----"Bin Meng" <bmeng.cn@gmail.com> schrieb: -----
> Hi Wolfgang,
> 
> [snip]
> 
> Unfortunately this patch breaks several ARM board:
> 
> +drivers/built-in.o: In function `ofnode_get_parent':
> +drivers/core/ofnode.c:235: undefined reference to `fdt_parent_offset'
> +drivers/built-in.o: In function `ofnode_read_simple_addr_cells':
> +drivers/core/ofnode.c:717: undefined reference to `fdt_address_cells'
> +drivers/built-in.o: In function `ofnode_read_simple_size_cells':
> +drivers/core/ofnode.c:725: undefined reference to `fdt_size_cells'
> +drivers/built-in.o: In function `ofnode_get_addr_size_index':
> +drivers/core/ofnode.c:294: undefined reference to `fdtdec_get_addr_size_fixed'
> 
> See https://dev.azure.com/bmeng/GitHub/_build/results?buildId=158&view=logs&j=25fc9316-c681-5af2-a03f-b888fd665e84&t=3af932b6-18ad-575e-b0ea-0b31bb7c6c26

Thanks for pointing this out.

I have missed that the code was guarded with
 #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
before, and is not guarded anymore now.

> Would you please send a v2 to address this? thanks!

Yes, I will follow up with a v2.

regards, Wolfgang

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-28 13:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-18 12:34 [RFC PATCH] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe() Wolfgang Wallner
2020-02-18 18:31 ` Simon Glass
2020-02-27  9:36 ` Bin Meng
2020-02-28  7:50   ` Bin Meng
2020-02-28 12:34     ` Bin Meng
2020-02-28 13:59     ` Antwort: " Wolfgang Wallner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.