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

[Bin Meng]

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.

> 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