fdt: pci: Permit use of reg property for setting device address
PATCH 1/1 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.
---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
lib/fdtdec.c +8 -2
@@ -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 ;
Signature · 1 line
2.2.0.rc0.207.ga3a616c
PATCH 1/1 fdt: pci: Permit use of reg property for setting device address
Hi Simon,
Quoted 24 lines · Simon Glass
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 <[email protected]>---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/fdtdec.c +2
-2@@ -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 */
lib/fdtdec.c int fdtdec_get_pci_bdf(const void *blob, int node,
Quoted 10 lines
lib/fdtdec.c +6
-1+ 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
PATCH 1/1 fdt: pci: Permit use of reg property for setting device address
+Thierry
Hi Bin,
Quoted 34 lines · Bin Meng
Hi Simon,
Quoted 31 lines · Simon Glass
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 <[email protected]>---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/fdtdec.c +8
-2@@ -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 ; + }
Quoted 6 lines
lib/fdtdec.c int fdtdec_get_pci_bdf(const void *blob, int node,
Quoted 3 lines
lib/fdtdec.c +0
-1/* 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
PATCH 1/1 fdt: pci: Permit use of reg property for setting device address
Hi Simon,
Quoted 46 lines · Simon Glass
+Thierry
Hi Bin,
Quoted 34 lines · Bin Meng
Hi Simon,
Quoted 31 lines · Simon Glass
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 <[email protected]>---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/fdtdec.c +8
-2@@ -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 ; + }Quoted 6 lines
lib/fdtdec.c int fdtdec_get_pci_bdf(const void *blob, int node,
Quoted 3 lines
lib/fdtdec.c +0
-1/* 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.
Quoted 4 lines
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
PATCH 1/1 fdt: pci: Permit use of reg property for setting device address
Quoted 54 lines · Bin Meng
Hi Simon,
Quoted 46 lines · Simon Glass
+Thierry
Hi Bin,
Quoted 34 lines · Bin Meng
Hi Simon,
Quoted 31 lines · Simon Glass
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 <[email protected]>---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/fdtdec.c +8
-2@@ -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 ; + }Quoted 6 lines
lib/fdtdec.c int fdtdec_get_pci_bdf(const void *blob, int node,
Quoted 3 lines
lib/fdtdec.c +0
-1/* 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.
Quoted 4 lines
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.
Quoted 4 lines
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150121/196b66cb/attachment.pgp>
PATCH 1/1 fdt: pci: Permit use of reg property for setting device address
Hi Thierry,
Quoted 66 lines · Thierry Reding
Quoted 54 lines · Bin Meng
Hi Simon,
Quoted 46 lines · Simon Glass
+Thierry
Hi Bin,
Quoted 34 lines · Bin Meng
Hi Simon,
Quoted 31 lines · Simon Glass
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 <[email protected]>---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/fdtdec.c +8
-2@@ -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 ; + }Quoted 6 lines
lib/fdtdec.c int fdtdec_get_pci_bdf(const void *blob, int node,
Quoted 3 lines
lib/fdtdec.c +0
-1/* 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.
Quoted 4 lines
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.
Quoted 4 lines
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?
Quoted 4 lines
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.
Quoted 6 lines
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'.
Quoted 12 lines
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_hibecause 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;
Quoted 5 lines
Which would avoid parsing the reg property twice. Also I think addr should be const struct
fdt_pci_addrto give a hint that it's an input parameter.Thierry
Regards,
Bin
PATCH 1/1 fdt: pci: Permit use of reg property for setting device address
Quoted 74 lines · Bin Meng
Hi Thierry,
Quoted 66 lines · Thierry Reding
Quoted 54 lines · Bin Meng
Hi Simon,
Quoted 46 lines · Simon Glass
+Thierry
Hi Bin,
Quoted 34 lines · Bin Meng
Hi Simon,
Quoted 31 lines · Simon Glass
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 <[email protected]>---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/fdtdec.c +8
-2@@ -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 ; + }Quoted 6 lines
lib/fdtdec.c int fdtdec_get_pci_bdf(const void *blob, int node,
Quoted 3 lines
lib/fdtdec.c +0
-1/* 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.
Quoted 4 lines
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.
Quoted 4 lines
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?Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 18 lines
Quoted 6 lines
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
Quoted 8 lines
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150121/dd48e022/attachment.pgp>
PATCH 1/1 fdt: pci: Permit use of reg property for setting device address
In reply to Simon GlassQuoted 20 lines · Simon Glass
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 <[email protected]>---
lib/fdtdec.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)lib/fdtdec.c +0
-0@@ -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);
lib/fdtdec.c int fdtdec_get_pci_bdf(const void *blob, int node, · line 229
Quoted 10 lines
lib/fdtdec.c +8
-2-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 ?
Signature · 9 lines
Sjoerd Simons <[email protected]> Collabora Ltd. -------------- next part -------------- A non-text attachment was scrubbed... Name: smime.p7s Type: application/x-pkcs7-signature Size: 6170 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150121/a49942fd/attachment.bin>