[U-Boot] [PATCH] fdt: pci: Permit use of reg property for setting device address
Thierry Reding
treding at nvidia.com
Wed Jan 21 10:33:15 CET 2015
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 at 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 at chromium.org> wrote:
> >> > +Thierry
> >> >
> >> > Hi Bin,
> >> >
> >> > On 20 January 2015 at 05:59, Bin Meng <bmeng.cn at gmail.com> wrote:
> >> >> Hi Simon,
> >> >>
> >> >> On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass <sjg at 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 at 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
-------------- 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>
More information about the U-Boot
mailing list