[U-Boot] [PATCH] fdt: pci: Permit use of reg property for setting device address
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org ---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device); - if (ret) - return ret; + if (ret) { + /* Fall back to using the 'reg' property */ + ret = fdtdec_get_int(blob, node, "reg", -1); + if (ret == -1) + return -ENOENT; + *bdf = ret & 0xffffff; + return 0; + }
/* extract the bdf from fdt_pci_addr */ *bdf = addr->phys_hi & 0xffff00;
Hi Simon,
On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass sjg@chromium.org wrote:
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);
if (ret)return ret;
if (ret) {/* Fall back to using the 'reg' property */ret = fdtdec_get_int(blob, node, "reg", -1);if (ret == -1)return -ENOENT;*bdf = ret & 0xffffff;return 0;} /* extract the bdf from fdt_pci_addr */ *bdf = addr->phys_hi & 0xffff00;--
How is 'reg' encodeded in Tegra's dts? I feel we should start using standard bindings instead of custom one.
Regards, Bin
+Thierry
Hi Bin,
On 20 January 2015 at 05:59, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass sjg@chromium.org wrote:
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);
if (ret)return ret;
if (ret) {/* Fall back to using the 'reg' property */ret = fdtdec_get_int(blob, node, "reg", -1);if (ret == -1)return -ENOENT;*bdf = ret & 0xffffff;return 0;} /* extract the bdf from fdt_pci_addr */ *bdf = addr->phys_hi & 0xffff00;--
How is 'reg' encodeded in Tegra's dts? I feel we should start using standard bindings instead of custom one.
This is as per the 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1). It seems just as valid as the textual ones.
Regards, Simon
Hi Simon,
On Tue, Jan 20, 2015 at 10:31 PM, Simon Glass sjg@chromium.org wrote:
+Thierry
Hi Bin,
On 20 January 2015 at 05:59, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass sjg@chromium.org wrote:
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);
if (ret)return ret;
if (ret) {/* Fall back to using the 'reg' property */ret = fdtdec_get_int(blob, node, "reg", -1);if (ret == -1)return -ENOENT;*bdf = ret & 0xffffff;return 0;} /* extract the bdf from fdt_pci_addr */ *bdf = addr->phys_hi & 0xffff00;--
How is 'reg' encodeded in Tegra's dts? I feel we should start using standard bindings instead of custom one.
This is as per the 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1). It seems just as valid as the textual ones.
I still don't get it. The 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1) is already supported in commit a62e84d. That's why I want to see how the Tegra's dts is written and how commit a62e84d could not handle that case.
Regards, Bin
On Wed, Jan 21, 2015 at 10:00:39AM +0800, Bin Meng wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 10:31 PM, Simon Glass sjg@chromium.org wrote:
+Thierry
Hi Bin,
On 20 January 2015 at 05:59, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass sjg@chromium.org wrote:
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);
if (ret)return ret;
if (ret) {/* Fall back to using the 'reg' property */ret = fdtdec_get_int(blob, node, "reg", -1);if (ret == -1)return -ENOENT;*bdf = ret & 0xffffff;return 0;} /* extract the bdf from fdt_pci_addr */ *bdf = addr->phys_hi & 0xffff00;--
How is 'reg' encodeded in Tegra's dts? I feel we should start using standard bindings instead of custom one.
This is as per the 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1). It seems just as valid as the textual ones.
I still don't get it. The 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1) is already supported in commit a62e84d. That's why I want to see how the Tegra's dts is written and how commit a62e84d could not handle that case.
Tegra's DTS doesn't have a compatible string for PCI devices, that's why fdtdec_get_pci_bdf() fails. Also it looks like the big comment in that function is a little overzealous. If you rely solely on matching the compatible string to the vendor and device IDs to get the BDF triplet, what if you have two devices with the same vendor and device IDs?
According to the OpenFirmware PCI bus binding, both of the reg and compatible properties should be constructed by OpenFirmware, so it's not really specified which one is mandatory if you provide them in a DTS. Or which one is authoritative for that matter.
In addition to the potential problem with two identical devices that I mentioned above, fdtdec_get_pci_bdf() ignores the BDF encoded in the DTB completely if vendor and device IDs don't match. So the assumption there is that compatible overrides reg. I don't think that's a good assumption because the compatible is just as likely to be wrong. At the very least I think it should trigger an error message that the DTB is inconsistent.
All of that said, if we absolutely must support this behaviour then I think Simon's patch makes perfect sense. I guess perhaps it shouldn't include the lower 8 bits of phys_hi because they designate the register number and that's not part of the BDF. Perhaps a simpler approach would be:
*bdf = addr->phys_hi & 0xffff00;
ret = fdtdec_get_pci_vendev(...); if (ret) return 0;
Which would avoid parsing the reg property twice. Also I think addr should be const struct fdt_pci_addr to give a hint that it's an input parameter.
Thierry
Hi Thierry,
On Wed, Jan 21, 2015 at 4:05 PM, Thierry Reding treding@nvidia.com wrote:
On Wed, Jan 21, 2015 at 10:00:39AM +0800, Bin Meng wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 10:31 PM, Simon Glass sjg@chromium.org wrote:
+Thierry
Hi Bin,
On 20 January 2015 at 05:59, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass sjg@chromium.org wrote:
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);
if (ret)return ret;
if (ret) {/* Fall back to using the 'reg' property */ret = fdtdec_get_int(blob, node, "reg", -1);if (ret == -1)return -ENOENT;*bdf = ret & 0xffffff;return 0;} /* extract the bdf from fdt_pci_addr */ *bdf = addr->phys_hi & 0xffff00;--
How is 'reg' encodeded in Tegra's dts? I feel we should start using standard bindings instead of custom one.
This is as per the 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1). It seems just as valid as the textual ones.
I still don't get it. The 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1) is already supported in commit a62e84d. That's why I want to see how the Tegra's dts is written and how commit a62e84d could not handle that case.
Tegra's DTS doesn't have a compatible string for PCI devices, that's why fdtdec_get_pci_bdf() fails. Also it looks like the big comment in that function is a little overzealous. If you rely solely on matching the compatible string to the vendor and device IDs to get the BDF triplet, what if you have two devices with the same vendor and device IDs?
According to the OpenFirmware PCI bus binding, both of the reg and compatible properties should be constructed by OpenFirmware, so it's not really specified which one is mandatory if you provide them in a DTS. Or which one is authoritative for that matter.
Yes, both 'reg' and 'compatible' are valid, and current fdtdec pci helper apis can support 'reg' via fdtdec_get_pci_addr(). IMHO this patch http://patchwork.ozlabs.org/patch/431183/ makes better sense to me, if we want to just fix the Tegra issue.
In addition to the potential problem with two identical devices that I mentioned above, fdtdec_get_pci_bdf() ignores the BDF encoded in the DTB completely if vendor and device IDs don't match. So the assumption there is that compatible overrides reg. I don't think that's a good assumption because the compatible is just as likely to be wrong. At the very least I think it should trigger an error message that the DTB is inconsistent.
Yes, current assumption in fdtdec_get_pci_bdf() is that compatible overrides reg. The reason is documented in that big comment block. You cannot use 'reg' to describe devices's bdf behind multiple bridges as bdf are assigned during pci enumeration. Yes, such assumption could not handle the cases like two devices with the same vendor and device IDs. I don't think we have a perfect solution now since in the original OF spec these encoding like 'compatible' and 'reg' are filled in by the firmware (the producer knows everything so that reg encoding would be unique in a system) while today U-Boot is actually the consumer of these encoding which is generated externally (by us when adding a new board support). To resolve this, I believe we may have to either: 1) manually fix up the bdf encodings in 'reg' for devices behind multiple bridges by bringing up U-Boot first and seeing what bdf is assigned to that device, then updating the dts to use this one. 2) let U-Boot fix up the bdf encodings in 'reg' for each device in dtb automatically as part of the pci enumeration process (maybe in the pci dm work). If we can always get correct bdf in 'reg', I think we can use 'reg' directly instead of bothering ourselves to parse 'compatible'.
All of that said, if we absolutely must support this behaviour then I think Simon's patch makes perfect sense. I guess perhaps it shouldn't include the lower 8 bits of phys_hi because they designate the register number and that's not part of the BDF. Perhaps a simpler approach would be:
*bdf = addr->phys_hi & 0xffff00; ret = fdtdec_get_pci_vendev(...); if (ret) return 0;Which would avoid parsing the reg property twice. Also I think addr should be const struct fdt_pci_addr to give a hint that it's an input parameter.
Thierry
Regards, Bin
On Wed, Jan 21, 2015 at 04:46:42PM +0800, Bin Meng wrote:
Hi Thierry,
On Wed, Jan 21, 2015 at 4:05 PM, Thierry Reding treding@nvidia.com wrote:
On Wed, Jan 21, 2015 at 10:00:39AM +0800, Bin Meng wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 10:31 PM, Simon Glass sjg@chromium.org wrote:
+Thierry
Hi Bin,
On 20 January 2015 at 05:59, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass sjg@chromium.org wrote:
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);
if (ret)return ret;
if (ret) {/* Fall back to using the 'reg' property */ret = fdtdec_get_int(blob, node, "reg", -1);if (ret == -1)return -ENOENT;*bdf = ret & 0xffffff;return 0;} /* extract the bdf from fdt_pci_addr */ *bdf = addr->phys_hi & 0xffff00;--
How is 'reg' encodeded in Tegra's dts? I feel we should start using standard bindings instead of custom one.
This is as per the 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1). It seems just as valid as the textual ones.
I still don't get it. The 'Numerical Representation' of the Physical Address Formats (in pci supplement v2.1) is already supported in commit a62e84d. That's why I want to see how the Tegra's dts is written and how commit a62e84d could not handle that case.
Tegra's DTS doesn't have a compatible string for PCI devices, that's why fdtdec_get_pci_bdf() fails. Also it looks like the big comment in that function is a little overzealous. If you rely solely on matching the compatible string to the vendor and device IDs to get the BDF triplet, what if you have two devices with the same vendor and device IDs?
According to the OpenFirmware PCI bus binding, both of the reg and compatible properties should be constructed by OpenFirmware, so it's not really specified which one is mandatory if you provide them in a DTS. Or which one is authoritative for that matter.
Yes, both 'reg' and 'compatible' are valid, and current fdtdec pci helper apis can support 'reg' via fdtdec_get_pci_addr(). IMHO this patch http://patchwork.ozlabs.org/patch/431183/ makes better sense to me, if we want to just fix the Tegra issue.
I don't have a strong opinion, so either way is fine with me. However if you choose to go with special-casing Tegra, then I suggest that the documentation of fdtdec_get_pci_bdf() is updated because currently the function doesn't do what the documentation says.
In addition to the potential problem with two identical devices that I mentioned above, fdtdec_get_pci_bdf() ignores the BDF encoded in the DTB completely if vendor and device IDs don't match. So the assumption there is that compatible overrides reg. I don't think that's a good assumption because the compatible is just as likely to be wrong. At the very least I think it should trigger an error message that the DTB is inconsistent.
Yes, current assumption in fdtdec_get_pci_bdf() is that compatible overrides reg. The reason is documented in that big comment block. You cannot use 'reg' to describe devices's bdf behind multiple bridges as bdf are assigned during pci enumeration. Yes, such assumption could not handle the cases like two devices with the same vendor and device IDs. I don't think we have a perfect solution now since in the original OF spec these encoding like 'compatible' and 'reg' are filled in by the firmware (the producer knows everything so that reg encoding would be unique in a system) while today U-Boot is actually the consumer of these encoding which is generated externally (by us when adding a new board support). To resolve this, I believe we may have to either: 1) manually fix up the bdf encodings in 'reg' for devices behind multiple bridges by bringing up U-Boot first and seeing what bdf is assigned to that device, then updating the dts to use this one. 2) let U-Boot fix up the bdf encodings in 'reg' for each device in dtb automatically as part of the pci enumeration process (maybe in the pci dm work). If we can always get correct bdf in 'reg', I think we can use 'reg' directly instead of bothering ourselves to parse 'compatible'.
That sounds like a good solution in the longer term. One of the issues is that the bus number still depends on the enumeration order. Earlier Linux kernels for example used breadth-first sorting and IIRC so does the BIOS on various x86 processors.
U-Boot and recent Linux kernels use depth-first sorting, so maybe this isn't an issue if we restrict ourselves to that combination.
Thierry
On Mon, 2015-01-19 at 20:19 -0700, Simon Glass wrote:
In commit a62e84d the old functionality of obtaining a PCI address from the 'reg' property was lost. Add it back, so we can support both a compatible string list and a 'reg' property.
This patch fixes PCIe ethernet on Tegra boards.
Signed-off-by: Simon Glass sjg@chromium.org
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 89dac4c..0488607 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -219,8 +219,14 @@ int fdtdec_get_pci_bdf(const void *blob, int node,
/* get vendor id & device id from the compatible string */ ret = fdtdec_get_pci_vendev(blob, node, &dt_vendor, &dt_device);
- if (ret)
return ret;
- if (ret) {
/* Fall back to using the 'reg' property */ret = fdtdec_get_int(blob, node, "reg", -1);if (ret == -1)return -ENOENT;*bdf = ret & 0xffffff;return 0;- }
Unless I'm missing something, this makes no sense. the 'reg' property should contain the physical address of the PCI device which this function already gets as one of its input parameters. Unless there is some valid case where the passed address isn't correct for the node ?
participants (4)
-
Bin Meng -
Simon Glass -
Sjoerd Simons -
Thierry Reding