diff mbox series

[v2] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe()

Message ID 20200302134114.2044855-1-wolfgang.wallner@br-automation.com
State Accepted
Commit 720f9e1fdb0c92d3fd16e1bfc25bcbd35612675c
Headers show
Series [v2] serial: ns16550: Move PCI access from ofdata_to_platdata() to probe() | expand

Commit Message

Wolfgang Wallner March 2, 2020, 1:41 p.m. UTC
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 its own function,
which is then called from 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 at 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

I have moved the code from probe() to a new function called
ns1655_serial_set_base_addr() to keep the probe() simple.

Changes in v2:
 - Fixed build guards
 - Moved code from probe() to a new function called
   ns1655_serial_set_base_addr() to keep the probe() simple
 - Removed RFC tag

 drivers/serial/ns16550.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

Comments

Simon Glass March 2, 2020, 7:47 p.m. UTC | #1
Hi Wolfgang,

On Mon, 2 Mar 2020 at 06:41, Wolfgang Wallner
<wolfgang.wallner at 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 its own function,
> which is then called from 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 at 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
>
> I have moved the code from probe() to a new function called
> ns1655_serial_set_base_addr() to keep the probe() simple.
>
> Changes in v2:
>  - Fixed build guards
>  - Moved code from probe() to a new function called
>    ns1655_serial_set_base_addr() to keep the probe() simple
>  - Removed RFC tag
>
>  drivers/serial/ns16550.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)

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

it would be nice i the build guards could use if() instead of #if but
it may not be possible.


- Simon
Bin Meng March 5, 2020, 10:23 a.m. UTC | #2
On Mon, Mar 2, 2020 at 9:41 PM Wolfgang Wallner
<wolfgang.wallner at 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 its own function,
> which is then called from 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 at 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
>
> I have moved the code from probe() to a new function called
> ns1655_serial_set_base_addr() to keep the probe() simple.
>
> Changes in v2:
>  - Fixed build guards
>  - Moved code from probe() to a new function called
>    ns1655_serial_set_base_addr() to keep the probe() simple
>  - Removed RFC tag
>
>  drivers/serial/ns16550.c | 40 ++++++++++++++++++++++++++++------------
>  1 file changed, 28 insertions(+), 12 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
Tested-by: Bin Meng <bmeng.cn at gmail.com> # Tested on Intel Galileo
diff mbox series

Patch

diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 1fcbc35015..c1b303ffcb 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -479,12 +479,40 @@  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);
@@ -507,21 +535,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);