[U-Boot] [PATCH 1/6] ofnode: Add missing address translation into ofnode_get_addr_size()
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,
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,
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
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
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.
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);
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
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:
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
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@10,0 { + reg = <0x8000 0 0 0 0>; + }; };
pci2: pci-controller2 {
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 0x20000x00008000 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 0x56780x10 0x00 0x1234 0x5678>;pci@10,0 {reg = <0x8000 0 0 0 0>;}; }; pci2: pci-controller2 {--
Regards, Bin
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 0x20000x00008000 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 ?
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 0x20000x00008000 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
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 0x20000x00008000 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 ?
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 0x20000x00008000 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
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 0x20000x00008000 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." ?
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 0x20000x00008000 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
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; }
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
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
participants (3)
-
Bin Meng -
Marek Vasut -
Simon Glass