[U-Boot] [PATCH] fdt: pci: Permit use of reg property for setting device address

[Thierry Reding]

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.

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>