[U-Boot] [PATCH 1/6] ofnode: Add missing address translation into ofnode_get_addr_size()

[U-Boot] [PATCH 1/6] ofnode: Add missing address translation into ofnode_get_addr_size()

[Marek Vasut]

Of CONFIG_OF_TRANSLATE is enabled, this function still returns
untranslated bogus results. Add the missing translation.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 drivers/core/ofnode.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
index a7e1927723..035023ca91 100644
--- a/drivers/core/ofnode.c
+++ b/drivers/core/ofnode.c
@@ -542,8 +542,15 @@ fdt_addr_t ofnode_get_addr_size(ofnode node, const char *property,
 			return FDT_ADDR_T_NONE;
 		na = of_n_addr_cells(np);
 		ns = of_n_addr_cells(np);
+
 		*sizep = of_read_number(prop + na, ns);
-		return of_read_number(prop, na);
+
+		if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
+			return of_translate_address(ofnode_to_np(node), prop);
+		} else {
+			na = of_n_addr_cells(ofnode_to_np(node));
+			return of_read_number(prop, na);
+		}
 	} else {
 		return fdtdec_get_addr_size(gd->fdt_blob,
 					    ofnode_to_offset(node), property,
-- 
2.18.0

[U-Boot] [PATCH 2/6] pci: Check ops before using them for config space access

[Marek Vasut]

The code fails to check if ops is non-NULL before using it's members.
Add the missing check and while at it, flip the condition to make it
more obvious what is actually happening.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 drivers/pci/pci-uclass.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index eb118f3496..de523a76ad 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -241,9 +241,9 @@ int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset,
 	struct dm_pci_ops *ops;
 
 	ops = pci_get_ops(bus);
-	if (!ops->write_config)
-		return -ENOSYS;
-	return ops->write_config(bus, bdf, offset, value, size);
+	if (ops && ops->write_config)
+		return ops->write_config(bus, bdf, offset, value, size);
+	return -ENOSYS;
 }
 
 int pci_bus_clrset_config32(struct udevice *bus, pci_dev_t bdf, int offset,
@@ -321,9 +321,9 @@ int pci_bus_read_config(struct udevice *bus, pci_dev_t bdf, int offset,
 	struct dm_pci_ops *ops;
 
 	ops = pci_get_ops(bus);
-	if (!ops->read_config)
-		return -ENOSYS;
-	return ops->read_config(bus, bdf, offset, valuep, size);
+	if (ops && ops->read_config)
+		return ops->read_config(bus, bdf, offset, valuep, size);
+	return -ENOSYS;
 }
 
 int pci_read_config(pci_dev_t bdf, int offset, unsigned long *valuep,
-- 
2.18.0

[U-Boot] [PATCH 3/6] pci: Support parsing PCI controller DT subnodes

[Marek Vasut]

The PCI controller can have DT subnodes describing extra properties
of particular PCI devices, ie. a PHY attached to an EHCI controller
on a PCI bus. This patch parses those DT subnodes and assigns a node
to the PCI device instance, so that the driver can extract details
from that node and ie. configure the PHY using the PHY subsystem.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 drivers/pci/pci-uclass.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index de523a76ad..e274632428 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -90,6 +90,25 @@ int pci_get_ff(enum pci_size_t size)
 	}
 }
 
+static void pci_dev_find_ofnode(struct udevice *bus, phys_addr_t bdf,
+				ofnode *rnode)
+{
+	ofnode node;
+
+	dev_for_each_subnode(node, bus) {
+		phys_addr_t df, size;
+		df = ofnode_get_addr_size(node, "reg", &size);
+		if (df == FDT_ADDR_T_NONE)
+			continue;
+
+		if (PCI_MASK_BUS(df) != PCI_MASK_BUS(bdf))
+			continue;
+
+		*rnode = node;
+		break;
+	}
+};
+
 int pci_bus_find_devfn(struct udevice *bus, pci_dev_t find_devfn,
 		       struct udevice **devp)
 {
@@ -641,6 +660,7 @@ static int pci_find_and_bind_driver(struct udevice *parent,
 				    pci_dev_t bdf, struct udevice **devp)
 {
 	struct pci_driver_entry *start, *entry;
+	ofnode node = ofnode_null();
 	const char *drv;
 	int n_ents;
 	int ret;
@@ -651,6 +671,10 @@ static int pci_find_and_bind_driver(struct udevice *parent,
 
 	debug("%s: Searching for driver: vendor=%x, device=%x\n", __func__,
 	      find_id->vendor, find_id->device);
+
+	/* Determine optional OF node */
+	pci_dev_find_ofnode(parent, bdf, &node);
+
 	start = ll_entry_start(struct pci_driver_entry, pci_driver_entry);
 	n_ents = ll_entry_count(struct pci_driver_entry, pci_driver_entry);
 	for (entry = start; entry != start + n_ents; entry++) {
@@ -684,8 +708,8 @@ static int pci_find_and_bind_driver(struct udevice *parent,
 			 * find another driver. For now this doesn't seem
 			 * necesssary, so just bind the first match.
 			 */
-			ret = device_bind(parent, drv, drv->name, NULL, -1,
-					  &dev);
+			ret = device_bind_ofnode(parent, drv, drv->name, NULL,
+						 node, &dev);
 			if (ret)
 				goto error;
 			debug("%s: Match found: %s\n", __func__, drv->name);
@@ -712,7 +736,7 @@ static int pci_find_and_bind_driver(struct udevice *parent,
 		return -ENOMEM;
 	drv = bridge ? "pci_bridge_drv" : "pci_generic_drv";
 
-	ret = device_bind_driver(parent, drv, str, devp);
+	ret = device_bind_driver_to_node(parent, drv, str, node, devp);
 	if (ret) {
 		debug("%s: Failed to bind generic driver: %d\n", __func__, ret);
 		free(str);
-- 
2.18.0

[U-Boot] [PATCH 4/6] pci: Update documentation to make 'compatible' string optional

[Marek Vasut]

Reword the documentation to make it clear the compatible string is now
optional, yet still matching on it takes precedence over PCI IDs and
PCI classes.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 doc/driver-model/pci-info.txt | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/doc/driver-model/pci-info.txt b/doc/driver-model/pci-info.txt
index e1701d1fbc..14364c5c75 100644
--- a/doc/driver-model/pci-info.txt
+++ b/doc/driver-model/pci-info.txt
@@ -34,11 +34,15 @@ under that bus.
 Note that this is all done on a lazy basis, as needed, so until something is
 touched on PCI (eg: a call to pci_find_devices()) it will not be probed.
 
-PCI devices can appear in the flattened device tree. If they do this serves to
-specify the driver to use for the device. In this case they will be bound at
-first. Each PCI device node must have a compatible string list as well as a
-<reg> property, as defined by the IEEE Std 1275-1994 PCI bus binding document
-v2.1. Note we must describe PCI devices with the same bus hierarchy as the
+PCI devices can appear in the flattened device tree. If they do, their node
+often contains extra information which cannot be derived from the PCI IDs or
+PCI class of the device. Each PCI device node must have a <reg> property, as
+defined by the IEEE Std 1275-1994 PCI bus binding document v2.1. Compatible
+string list is optional and generally not needed, since PCI is discoverable
+bus, albeit there are justified exceptions. If the compatible string is
+present, matching on it takes precedence over PCI IDs and PCI classes.
+
+Note we must describe PCI devices with the same bus hierarchy as the
 hardware, otherwise driver model cannot detect the correct parent/children
 relationship during PCI bus enumeration thus PCI devices won't be bound to
 their drivers accordingly. A working example like below:
-- 
2.18.0

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Marek Vasut]

Add PCI entry without compatible string and with a DT node only with
reg = <...> property into the DT. This is needed for the tests to
verify whether such a setup creates an U-Boot PCI device with the
DT node associated with it in udevice.node.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 arch/sandbox/dts/test.dts | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index b8524e3b7d..c13a270c2e 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -354,9 +354,14 @@
 		#address-cells = <3>;
 		#size-cells = <2>;
 		ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
-				0x01000000 0 0x40000000 0x40000000 0 0x2000>;
+			  0x01000000 0 0x40000000 0x40000000 0 0x2000
+			  0x00008000 0 0x00000000 0x00008000 0 0x2000>;
 		sandbox,dev-info = <0x08 0x00 0x1234 0x5678
-				    0x0c 0x00 0x1234 0x5678>;
+				    0x0c 0x00 0x1234 0x5678
+				    0x10 0x00 0x1234 0x5678>;
+		pci at 10,0 {
+			reg = <0x8000 0 0 0 0>;
+		};
 	};
 
 	pci2: pci-controller2 {
-- 
2.18.0

[U-Boot] [PATCH 6/6] test: Add test for PCI device without compat string and with DT node

[Marek Vasut]

Add test which checks if a PCI device described in DT with an
entry and reg = <...> property, but without compatible string
results in a valid U-Boot PCI udevice with the udevice.node
populated with reference to this DT node. Also check if the
other PCI device without a DT node does not contain any bogus
udevice.node.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 test/dm/pci.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/test/dm/pci.c b/test/dm/pci.c
index 869970072d..a1dedd84a7 100644
--- a/test/dm/pci.c
+++ b/test/dm/pci.c
@@ -119,8 +119,13 @@ static int dm_test_pci_drvdata(struct unit_test_state *uts)
 
 	ut_assertok(dm_pci_bus_find_bdf(PCI_BDF(1, 0x08, 0), &swap));
 	ut_asserteq(SWAP_CASE_DRV_DATA, swap->driver_data);
+	ut_assertok(dev_of_valid(swap));
 	ut_assertok(dm_pci_bus_find_bdf(PCI_BDF(1, 0x0c, 0), &swap));
 	ut_asserteq(SWAP_CASE_DRV_DATA, swap->driver_data);
+	ut_assertok(dev_of_valid(swap));
+	ut_assertok(dm_pci_bus_find_bdf(PCI_BDF(1, 0x10, 0), &swap));
+	ut_asserteq(SWAP_CASE_DRV_DATA, swap->driver_data);
+	ut_assertok(!dev_of_valid(swap));
 
 	return 0;
 }
-- 
2.18.0

[U-Boot] [PATCH 1/6] ofnode: Add missing address translation into ofnode_get_addr_size()

[Bin Meng]

Hi Marek,

On Sat, Sep 22, 2018 at 6:59 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> Of CONFIG_OF_TRANSLATE is enabled, this function still returns
> untranslated bogus results. Add the missing translation.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  drivers/core/ofnode.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> index a7e1927723..035023ca91 100644
> --- a/drivers/core/ofnode.c
> +++ b/drivers/core/ofnode.c
> @@ -542,8 +542,15 @@ fdt_addr_t ofnode_get_addr_size(ofnode node, const char *property,
>                         return FDT_ADDR_T_NONE;
>                 na = of_n_addr_cells(np);
>                 ns = of_n_addr_cells(np);

There is an unrelated bug. It should be:

ns = of_n_size_cells(np);

> +
>                 *sizep = of_read_number(prop + na, ns);
> -               return of_read_number(prop, na);
> +
> +               if (IS_ENABLED(CONFIG_OF_TRANSLATE) && ns > 0) {
> +                       return of_translate_address(ofnode_to_np(node), prop);

Just use np instead of ofnode_to_np(node)

> +               } else {
> +                       na = of_n_addr_cells(ofnode_to_np(node));

This line is unnecessary.

> +                       return of_read_number(prop, na);
> +               }
>         } else {
>                 return fdtdec_get_addr_size(gd->fdt_blob,
>                                             ofnode_to_offset(node), property,
> --

However, I have to point out that this patch (along with the unrelated
bug I pointed out above) is not related to "support parsing PCI
controller DT subnodes". Both of_translate_address() and
of_read_number() are not designed to handle PCI-specific address
formats. See my comments in patch 3 and 5.

Regards,
Bin

[U-Boot] [PATCH 2/6] pci: Check ops before using them for config space access

[Bin Meng]

Hi Marek,

On Sat, Sep 22, 2018 at 7:00 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> The code fails to check if ops is non-NULL before using it's members.
> Add the missing check and while at it, flip the condition to make it
> more obvious what is actually happening.
>

Though adding the NULL pointer check makes the codes more robust, I
would like to know with what calling sequence current codes are
broken? I did a grep and looks every PCI controller driver registers a
non-NULL dm_pci_ops so the ops can't be NULL, which means adding the
check seems unnecessary.

> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  drivers/pci/pci-uclass.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index eb118f3496..de523a76ad 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -241,9 +241,9 @@ int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset,
>         struct dm_pci_ops *ops;
>
>         ops = pci_get_ops(bus);
> -       if (!ops->write_config)
> -               return -ENOSYS;
> -       return ops->write_config(bus, bdf, offset, value, size);
> +       if (ops && ops->write_config)
> +               return ops->write_config(bus, bdf, offset, value, size);
> +       return -ENOSYS;
>  }
>
>  int pci_bus_clrset_config32(struct udevice *bus, pci_dev_t bdf, int offset,
> @@ -321,9 +321,9 @@ int pci_bus_read_config(struct udevice *bus, pci_dev_t bdf, int offset,
>         struct dm_pci_ops *ops;
>
>         ops = pci_get_ops(bus);
> -       if (!ops->read_config)
> -               return -ENOSYS;
> -       return ops->read_config(bus, bdf, offset, valuep, size);
> +       if (ops && ops->read_config)
> +               return ops->read_config(bus, bdf, offset, valuep, size);
> +       return -ENOSYS;
>  }
>
>  int pci_read_config(pci_dev_t bdf, int offset, unsigned long *valuep,
> --

Regards,
Bin

[U-Boot] [PATCH 3/6] pci: Support parsing PCI controller DT subnodes

[Bin Meng]

Hi Marek,

On Sat, Sep 22, 2018 at 7:00 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> The PCI controller can have DT subnodes describing extra properties
> of particular PCI devices, ie. a PHY attached to an EHCI controller
> on a PCI bus. This patch parses those DT subnodes and assigns a node
> to the PCI device instance, so that the driver can extract details
> from that node and ie. configure the PHY using the PHY subsystem.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  drivers/pci/pci-uclass.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index de523a76ad..e274632428 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -90,6 +90,25 @@ int pci_get_ff(enum pci_size_t size)
>         }
>  }
>
> +static void pci_dev_find_ofnode(struct udevice *bus, phys_addr_t bdf,
> +                               ofnode *rnode)
> +{
> +       ofnode node;
> +
> +       dev_for_each_subnode(node, bus) {
> +               phys_addr_t df, size;
> +               df = ofnode_get_addr_size(node, "reg", &size);

Using API ofnode_get_addr_size() is wrong. It cannot handle
PCI-specific address formats. I understand why you added "0x00008000 0
0x00000000 0x00008000 0 0x2000" to the bus ranges property in patch 5,
is to make ofnode_get_addr_size() work, but that's the wrong approach.
The correct API should be ofnode_read_pci_addr(). To call it like
this:

ret = ofnode_read_pci_addr(node, FDT_PCI_SPACE_CONFIG, "reg", &addr);
if (!ret)
    df = addr.phys_hi & 0xff00;

> +               if (df == FDT_ADDR_T_NONE)
> +                       continue;
> +
> +               if (PCI_MASK_BUS(df) != PCI_MASK_BUS(bdf))
> +                       continue;
> +
> +               *rnode = node;
> +               break;
> +       }
> +};
> +
>  int pci_bus_find_devfn(struct udevice *bus, pci_dev_t find_devfn,
>                        struct udevice **devp)
>  {
> @@ -641,6 +660,7 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>                                     pci_dev_t bdf, struct udevice **devp)
>  {
>         struct pci_driver_entry *start, *entry;
> +       ofnode node = ofnode_null();
>         const char *drv;
>         int n_ents;
>         int ret;
> @@ -651,6 +671,10 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>
>         debug("%s: Searching for driver: vendor=%x, device=%x\n", __func__,
>               find_id->vendor, find_id->device);
> +
> +       /* Determine optional OF node */
> +       pci_dev_find_ofnode(parent, bdf, &node);
> +
>         start = ll_entry_start(struct pci_driver_entry, pci_driver_entry);
>         n_ents = ll_entry_count(struct pci_driver_entry, pci_driver_entry);
>         for (entry = start; entry != start + n_ents; entry++) {
> @@ -684,8 +708,8 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>                          * find another driver. For now this doesn't seem
>                          * necesssary, so just bind the first match.
>                          */
> -                       ret = device_bind(parent, drv, drv->name, NULL, -1,
> -                                         &dev);
> +                       ret = device_bind_ofnode(parent, drv, drv->name, NULL,
> +                                                node, &dev);
>                         if (ret)
>                                 goto error;
>                         debug("%s: Match found: %s\n", __func__, drv->name);
> @@ -712,7 +736,7 @@ static int pci_find_and_bind_driver(struct udevice *parent,
>                 return -ENOMEM;
>         drv = bridge ? "pci_bridge_drv" : "pci_generic_drv";
>
> -       ret = device_bind_driver(parent, drv, str, devp);
> +       ret = device_bind_driver_to_node(parent, drv, str, node, devp);
>         if (ret) {
>                 debug("%s: Failed to bind generic driver: %d\n", __func__, ret);
>                 free(str);
> --

Regards,
Bin

[U-Boot] [PATCH 4/6] pci: Update documentation to make 'compatible' string optional

[Bin Meng]

On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> Reword the documentation to make it clear the compatible string is now
> optional, yet still matching on it takes precedence over PCI IDs and
> PCI classes.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  doc/driver-model/pci-info.txt | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
>

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

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Bin Meng]

Hi Marek,

On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> Add PCI entry without compatible string and with a DT node only with
> reg = <...> property into the DT. This is needed for the tests to
> verify whether such a setup creates an U-Boot PCI device with the
> DT node associated with it in udevice.node.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  arch/sandbox/dts/test.dts | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> index b8524e3b7d..c13a270c2e 100644
> --- a/arch/sandbox/dts/test.dts
> +++ b/arch/sandbox/dts/test.dts
> @@ -354,9 +354,14 @@
>                 #address-cells = <3>;
>                 #size-cells = <2>;
>                 ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
> -                               0x01000000 0 0x40000000 0x40000000 0 0x2000>;
> +                         0x01000000 0 0x40000000 0x40000000 0 0x2000
> +                         0x00008000 0 0x00000000 0x00008000 0 0x2000>;

Adding this line makes no sense. You can't translate a PCI bus
configuration space address (0x8000) to something in its parent bus's
(MMIO) address space. See my related comments in patch 1 and 3.

>                 sandbox,dev-info = <0x08 0x00 0x1234 0x5678
> -                                   0x0c 0x00 0x1234 0x5678>;
> +                                   0x0c 0x00 0x1234 0x5678
> +                                   0x10 0x00 0x1234 0x5678>;
> +               pci at 10,0 {
> +                       reg = <0x8000 0 0 0 0>;
> +               };
>         };
>
>         pci2: pci-controller2 {
> --

Regards,
Bin

[U-Boot] [PATCH 6/6] test: Add test for PCI device without compat string and with DT node

[Bin Meng]

On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> Add test which checks if a PCI device described in DT with an
> entry and reg = <...> property, but without compatible string
> results in a valid U-Boot PCI udevice with the udevice.node
> populated with reference to this DT node. Also check if the
> other PCI device without a DT node does not contain any bogus
> udevice.node.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  test/dm/pci.c | 5 +++++
>  1 file changed, 5 insertions(+)
>

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

[U-Boot] [PATCH 2/6] pci: Check ops before using them for config space access

[Simon Glass]

Hi Marek,

On 21 September 2018 at 16:59, Marek Vasut <marek.vasut@gmail.com> wrote:
> The code fails to check if ops is non-NULL before using it's members.
> Add the missing check and while at it, flip the condition to make it
> more obvious what is actually happening.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  drivers/pci/pci-uclass.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index eb118f3496..de523a76ad 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -241,9 +241,9 @@ int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset,
>         struct dm_pci_ops *ops;
>
>         ops = pci_get_ops(bus);
> -       if (!ops->write_config)
> -               return -ENOSYS;
> -       return ops->write_config(bus, bdf, offset, value, size);
> +       if (ops && ops->write_config)
> +               return ops->write_config(bus, bdf, offset, value, size);

I'd like to avoid this if possible, since it bloats code. If you don't
provide operations you are on your own!

Ideas:
- add it behind DEBUG
- check it once in the uclass when binding - e.g. in uclass_add() -
and print a warning?

I have pushed back pretty hard against people adding checks for things
which should not be true in normal systems. Partly it is just for the
confusion it adds, partly for efficiency. Perhaps we should document
the pre-conditions that DM guarantees somewhere?

Regards,
Simon

[U-Boot] [PATCH 2/6] pci: Check ops before using them for config space access

[Marek Vasut]

On 09/26/2018 07:42 AM, Simon Glass wrote:
> Hi Marek,
> 
> On 21 September 2018 at 16:59, Marek Vasut <marek.vasut@gmail.com> wrote:
>> The code fails to check if ops is non-NULL before using it's members.
>> Add the missing check and while at it, flip the condition to make it
>> more obvious what is actually happening.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>>  drivers/pci/pci-uclass.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> index eb118f3496..de523a76ad 100644
>> --- a/drivers/pci/pci-uclass.c
>> +++ b/drivers/pci/pci-uclass.c
>> @@ -241,9 +241,9 @@ int pci_bus_write_config(struct udevice *bus, pci_dev_t bdf, int offset,
>>         struct dm_pci_ops *ops;
>>
>>         ops = pci_get_ops(bus);
>> -       if (!ops->write_config)
>> -               return -ENOSYS;
>> -       return ops->write_config(bus, bdf, offset, value, size);
>> +       if (ops && ops->write_config)
>> +               return ops->write_config(bus, bdf, offset, value, size);
> 
> I'd like to avoid this if possible, since it bloats code. If you don't
> provide operations you are on your own!
> 
> Ideas:
> - add it behind DEBUG
> - check it once in the uclass when binding - e.g. in uclass_add() -
> and print a warning?
> 
> I have pushed back pretty hard against people adding checks for things
> which should not be true in normal systems. Partly it is just for the
> confusion it adds, partly for efficiency. Perhaps we should document
> the pre-conditions that DM guarantees somewhere?

Seems unneeded, dropped.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Marek Vasut]

On 09/25/2018 05:26 PM, Bin Meng wrote:
> Hi Marek,
> 
> On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> Add PCI entry without compatible string and with a DT node only with
>> reg = <...> property into the DT. This is needed for the tests to
>> verify whether such a setup creates an U-Boot PCI device with the
>> DT node associated with it in udevice.node.
>>
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Tom Rini <trini@konsulko.com>
>> ---
>>  arch/sandbox/dts/test.dts | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
>> index b8524e3b7d..c13a270c2e 100644
>> --- a/arch/sandbox/dts/test.dts
>> +++ b/arch/sandbox/dts/test.dts
>> @@ -354,9 +354,14 @@
>>                 #address-cells = <3>;
>>                 #size-cells = <2>;
>>                 ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
>> -                               0x01000000 0 0x40000000 0x40000000 0 0x2000>;
>> +                         0x01000000 0 0x40000000 0x40000000 0 0x2000
>> +                         0x00008000 0 0x00000000 0x00008000 0 0x2000>;
> 
> Adding this line makes no sense. You can't translate a PCI bus
> configuration space address (0x8000) to something in its parent bus's
> (MMIO) address space. See my related comments in patch 1 and 3.
> 

So what should be in that line ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Bin Meng]

Hi Marek,

On Mon, Oct 1, 2018 at 7:44 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 09/25/2018 05:26 PM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> Add PCI entry without compatible string and with a DT node only with
> >> reg = <...> property into the DT. This is needed for the tests to
> >> verify whether such a setup creates an U-Boot PCI device with the
> >> DT node associated with it in udevice.node.
> >>
> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >> Cc: Simon Glass <sjg@chromium.org>
> >> Cc: Tom Rini <trini@konsulko.com>
> >> ---
> >>  arch/sandbox/dts/test.dts | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> >> index b8524e3b7d..c13a270c2e 100644
> >> --- a/arch/sandbox/dts/test.dts
> >> +++ b/arch/sandbox/dts/test.dts
> >> @@ -354,9 +354,14 @@
> >>                 #address-cells = <3>;
> >>                 #size-cells = <2>;
> >>                 ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
> >> -                               0x01000000 0 0x40000000 0x40000000 0 0x2000>;
> >> +                         0x01000000 0 0x40000000 0x40000000 0 0x2000
> >> +                         0x00008000 0 0x00000000 0x00008000 0 0x2000>;
> >
> > Adding this line makes no sense. You can't translate a PCI bus
> > configuration space address (0x8000) to something in its parent bus's
> > (MMIO) address space. See my related comments in patch 1 and 3.
> >
>
> So what should be in that line ?
>

There is no need to add that line in the ranges property.

Regards,
Bin

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Marek Vasut]

On 10/01/2018 01:44 PM, Marek Vasut wrote:
> On 09/25/2018 05:26 PM, Bin Meng wrote:
>> Hi Marek,
>>
>> On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>
>>> Add PCI entry without compatible string and with a DT node only with
>>> reg = <...> property into the DT. This is needed for the tests to
>>> verify whether such a setup creates an U-Boot PCI device with the
>>> DT node associated with it in udevice.node.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Tom Rini <trini@konsulko.com>
>>> ---
>>>  arch/sandbox/dts/test.dts | 9 +++++++--
>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
>>> index b8524e3b7d..c13a270c2e 100644
>>> --- a/arch/sandbox/dts/test.dts
>>> +++ b/arch/sandbox/dts/test.dts
>>> @@ -354,9 +354,14 @@
>>>                 #address-cells = <3>;
>>>                 #size-cells = <2>;
>>>                 ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
>>> -                               0x01000000 0 0x40000000 0x40000000 0 0x2000>;
>>> +                         0x01000000 0 0x40000000 0x40000000 0 0x2000
>>> +                         0x00008000 0 0x00000000 0x00008000 0 0x2000>;
>>
>> Adding this line makes no sense. You can't translate a PCI bus
>> configuration space address (0x8000) to something in its parent bus's
>> (MMIO) address space. See my related comments in patch 1 and 3.
>>
> 
> So what should be in that line ?

Ping ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Bin Meng]

Hi Marek,

On Sun, Oct 7, 2018 at 8:12 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 10/01/2018 01:44 PM, Marek Vasut wrote:
> > On 09/25/2018 05:26 PM, Bin Meng wrote:
> >> Hi Marek,
> >>
> >> On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>
> >>> Add PCI entry without compatible string and with a DT node only with
> >>> reg = <...> property into the DT. This is needed for the tests to
> >>> verify whether such a setup creates an U-Boot PCI device with the
> >>> DT node associated with it in udevice.node.
> >>>
> >>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>> Cc: Simon Glass <sjg@chromium.org>
> >>> Cc: Tom Rini <trini@konsulko.com>
> >>> ---
> >>>  arch/sandbox/dts/test.dts | 9 +++++++--
> >>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> >>> index b8524e3b7d..c13a270c2e 100644
> >>> --- a/arch/sandbox/dts/test.dts
> >>> +++ b/arch/sandbox/dts/test.dts
> >>> @@ -354,9 +354,14 @@
> >>>                 #address-cells = <3>;
> >>>                 #size-cells = <2>;
> >>>                 ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
> >>> -                               0x01000000 0 0x40000000 0x40000000 0 0x2000>;
> >>> +                         0x01000000 0 0x40000000 0x40000000 0 0x2000
> >>> +                         0x00008000 0 0x00000000 0x00008000 0 0x2000>;
> >>
> >> Adding this line makes no sense. You can't translate a PCI bus
> >> configuration space address (0x8000) to something in its parent bus's
> >> (MMIO) address space. See my related comments in patch 1 and 3.
> >>
> >
> > So what should be in that line ?
>
> Ping ?

Looks you missed my response ?
https://lists.denx.de/pipermail/u-boot/2018-October/342820.html

Regards,
Bin

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Marek Vasut]

On 10/07/2018 02:16 PM, Bin Meng wrote:
> Hi Marek,
> 
> On Sun, Oct 7, 2018 at 8:12 PM Marek Vasut <marek.vasut@gmail.com> wrote:
>>
>> On 10/01/2018 01:44 PM, Marek Vasut wrote:
>>> On 09/25/2018 05:26 PM, Bin Meng wrote:
>>>> Hi Marek,
>>>>
>>>> On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>>>>>
>>>>> Add PCI entry without compatible string and with a DT node only with
>>>>> reg = <...> property into the DT. This is needed for the tests to
>>>>> verify whether such a setup creates an U-Boot PCI device with the
>>>>> DT node associated with it in udevice.node.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Tom Rini <trini@konsulko.com>
>>>>> ---
>>>>>  arch/sandbox/dts/test.dts | 9 +++++++--
>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
>>>>> index b8524e3b7d..c13a270c2e 100644
>>>>> --- a/arch/sandbox/dts/test.dts
>>>>> +++ b/arch/sandbox/dts/test.dts
>>>>> @@ -354,9 +354,14 @@
>>>>>                 #address-cells = <3>;
>>>>>                 #size-cells = <2>;
>>>>>                 ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
>>>>> -                               0x01000000 0 0x40000000 0x40000000 0 0x2000>;
>>>>> +                         0x01000000 0 0x40000000 0x40000000 0 0x2000
>>>>> +                         0x00008000 0 0x00000000 0x00008000 0 0x2000>;
>>>>
>>>> Adding this line makes no sense. You can't translate a PCI bus
>>>> configuration space address (0x8000) to something in its parent bus's
>>>> (MMIO) address space. See my related comments in patch 1 and 3.
>>>>
>>>
>>> So what should be in that line ?
>>
>> Ping ?
> 
> Looks you missed my response ?
> https://lists.denx.de/pipermail/u-boot/2018-October/342820.html

I don't have that mail, sorry.

Can you explain your reply ? Why "There is no need to add that line in
the ranges property." ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH 5/6] test: Add PCI device entry without compat string and with DT node

[Bin Meng]

Hi Marek,

On Mon, Oct 8, 2018 at 12:00 AM Marek Vasut <marek.vasut@gmail.com> wrote:
>
> On 10/07/2018 02:16 PM, Bin Meng wrote:
> > Hi Marek,
> >
> > On Sun, Oct 7, 2018 at 8:12 PM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>
> >> On 10/01/2018 01:44 PM, Marek Vasut wrote:
> >>> On 09/25/2018 05:26 PM, Bin Meng wrote:
> >>>> Hi Marek,
> >>>>
> >>>> On Sat, Sep 22, 2018 at 7:02 AM Marek Vasut <marek.vasut@gmail.com> wrote:
> >>>>>
> >>>>> Add PCI entry without compatible string and with a DT node only with
> >>>>> reg = <...> property into the DT. This is needed for the tests to
> >>>>> verify whether such a setup creates an U-Boot PCI device with the
> >>>>> DT node associated with it in udevice.node.
> >>>>>
> >>>>> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> >>>>> Cc: Simon Glass <sjg@chromium.org>
> >>>>> Cc: Tom Rini <trini@konsulko.com>
> >>>>> ---
> >>>>>  arch/sandbox/dts/test.dts | 9 +++++++--
> >>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> >>>>> index b8524e3b7d..c13a270c2e 100644
> >>>>> --- a/arch/sandbox/dts/test.dts
> >>>>> +++ b/arch/sandbox/dts/test.dts
> >>>>> @@ -354,9 +354,14 @@
> >>>>>                 #address-cells = <3>;
> >>>>>                 #size-cells = <2>;
> >>>>>                 ranges = <0x02000000 0 0x30000000 0x30000000 0 0x2000
> >>>>> -                               0x01000000 0 0x40000000 0x40000000 0 0x2000>;
> >>>>> +                         0x01000000 0 0x40000000 0x40000000 0 0x2000
> >>>>> +                         0x00008000 0 0x00000000 0x00008000 0 0x2000>;
> >>>>
> >>>> Adding this line makes no sense. You can't translate a PCI bus
> >>>> configuration space address (0x8000) to something in its parent bus's
> >>>> (MMIO) address space. See my related comments in patch 1 and 3.
> >>>>
> >>>
> >>> So what should be in that line ?
> >>
> >> Ping ?
> >
> > Looks you missed my response ?
> > https://lists.denx.de/pipermail/u-boot/2018-October/342820.html
>
> I don't have that mail, sorry.
>
> Can you explain your reply ? Why "There is no need to add that line in
> the ranges property." ?

I mentioned "See my related comments in patch 1 and 3.", and in the
patch 3 review comments [1] I said:

"Using API ofnode_get_addr_size() is wrong. It cannot handle
PCI-specific address formats. I understand why you added "0x00008000 0
0x00000000 0x00008000 0 0x2000" to the bus ranges property in patch 5,
is to make ofnode_get_addr_size() work, but that's the wrong approach.
The correct API should be ofnode_read_pci_addr(). To call it like
this:

ret = ofnode_read_pci_addr(node, FDT_PCI_SPACE_CONFIG, "reg", &addr);
if (!ret)
    df = addr.phys_hi & 0xff00;"

So all we need do is to use ofnode_read_pci_addr() to get the PCI bdf,
and there is no need to add any line in the ranges property.

BTW: changing ranges property also violates the rules of using Linux
DT out of the box :)

[1] https://lists.denx.de/pipermail/u-boot/2018-September/341814.html

Regards,
Bin