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 <tred...@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 <s...@chromium.org> wrote:
> >> > +Thierry
> >> >
> >> > Hi Bin,
> >> >
> >> > On 20 January 2015 at 05:59, Bin Meng <bmeng...@gmail.com> wrote:
> >> >> Hi Simon,
> >> >>
> >> >> On Tue, Jan 20, 2015 at 11:19 AM, Simon Glass <s...@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 <s...@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
pgpu4leldeIMq.pgp
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
