[U-Boot] [PATCH] pci: tegra: Fix port information parsing

[Stephen Warren <swarren@wwwdotorg.org>]

On 01/23/2015 03:19 AM, Thierry Reding wrote:
> On Thu, Jan 22, 2015 at 12:04:06AM +0800, Bin Meng wrote:
>> Hi Thierry,
>>
>> On Wed, Jan 21, 2015 at 5:40 PM, Thierry Reding <treding@nvidia.com> wrote:
>>> On Wed, Jan 21, 2015 at 05:15:42PM +0800, Bin Meng wrote:
>>>> Hi Thierry,
>>>>
>>>> On Wed, Jan 21, 2015 at 4:24 PM, Thierry Reding <treding@nvidia.com> wrote:
>>>>> On Wed, Jan 21, 2015 at 10:37:07AM +0800, Bin Meng wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Wed, Jan 21, 2015 at 3:05 AM, Simon Glass <sjg@chromium.org> wrote:
>>>>>>> Hi Sjoerd,
>>>>>>>
>>>>>>> On 20 January 2015 at 10:06, Sjoerd Simons
>>>>>>> <sjoerd.simons@collabora.co.uk> wrote:
>>>>>>>> commit a62e84d7b1824a202dd incorrectly changed the tegra pci code to the
>>>>>>>> new fdtdec pci helpers. To get the device index of the root port, the
>>>>>>>> "reg" property should be parsed from the dtb (as was previously the
>>>>>>>> case).
>>>>>>>>
>>>>>>>> With this patch i can successfully network boot my jetson tk1
>>>>>>>>
>>>>>>>> Signed-off-by: Sjoerd Simons <sjoerd.simons@collabora.co.uk>
>>>>>>>> ---
>>>>>>>>   drivers/pci/pci_tegra.c | 5 ++---
>>>>>>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> Can you also please take a look at this patch?
>>>>>>>
>>>>>>> http://patchwork.ozlabs.org/patch/430815/
>>>>>>>
>>>>>>> It tries to support both options.
>>>>>>
>>>>>> Although I still don't see how the Tegra's dts is written, I feel this
>>>>>> patch is doing correctly.
>>>>>
>>>>> It's in the U-Boot tree, look at arch/arm/dts/tegra124.dtsi for an
>>>>> example.
>>>>
>>>> Got it. I see:
>>>>
>>>>                  pci at 1,0 {
>>>>                          device_type = "pci";
>>>>                          assigned-addresses = <0x82000800 0 0x01000000 0 0x1000>;
>>>>                          reg = <0x000800 0 0 0 0>;
>>>>                          status = "disabled";
>>>>
>>>>                          #address-cells = <3>;
>>>>                          #size-cells = <2>;
>>>>                          ranges;
>>>>
>>>>                          nvidia,num-lanes = <2>;
>>>>                  };
>>>>
>>>> So I would read this 'reg = <0x000800 0 0 0 0>' as this is a
>>>> downstream port with device number 1 of the root complex.
>>>
>>> Correct. Note that these root ports don't appear on the bus using the
>>> regular configuration space accesses, so the definition here is
>>> arbitrary, though in a way to mirror what PCI would typically look like
>>> (host bridge 00:00.0, root ports 00:01.0..00:0N.0).
>>>
>>> The Linux kernel driver (and the U-Boot driver for that matter) rely on
>>> this numbering, though, for some aspects of configuration of the root
>>> ports.
>>>
>>>>>>>> diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c
>>>>>>>> index f9e05ad..67b5fdf 100644
>>>>>>>> --- a/drivers/pci/pci_tegra.c
>>>>>>>> +++ b/drivers/pci/pci_tegra.c
>>>>>>>> @@ -459,7 +459,6 @@ static int tegra_pcie_parse_port_info(const void *fdt, int node,
>>>>>>>>                                        unsigned int *lanes)
>>>>>>>>   {
>>>>>>>>          struct fdt_pci_addr addr;
>>>>>>>> -       pci_dev_t bdf;
>>>>>>>>          int err;
>>>>>>>>
>>>>>>>>          err = fdtdec_get_int(fdt, node, "nvidia,num-lanes", 0);
>>>>>>>> @@ -470,13 +469,13 @@ static int tegra_pcie_parse_port_info(const void *fdt, int node,
>>>>>>>>
>>>>>>>>          *lanes = err;
>>>>>>>>
>>>>>>>> -       err = fdtdec_get_pci_bdf(fdt, node, &addr, &bdf);
>>>>>>>> +       err = fdtdec_get_pci_addr(fdt, node, 0, "reg", &addr);
>>>>>>
>>>>>> I suggest replace 0 to FDT_PCI_SPACE_CONFIG.
>>>>>
>>>>> I do like how 0 actually transports the meaning of "don't care" here.
>>>>> The reg property encodes only the BDF, whereas the configuration space
>>>>> region for the root ports is encoded in the assigned-addresses property.
>>>>>
>>>>> Looking at the fdtdec_get_pci_addr() implementation I notice that it
>>>>> uses the type parameter to match on the type of region. Devices can have
>>>>> more than one region of the same type. How is that supposed to work with
>>>>> this function. Perhaps it's nothing we care about for the fdtdec API
>>>>> since we don't access those regions anyway from FDT code?
>>>>
>>>> Ah, yes, some devices may have multiple regions of the same type.
>>>> Perhaps we need another parameter bar_index for this api? So far this
>>>> API is not used by FDT codes. It is used by the ns16550 driver where
>>>> pci ns16550 normally has two bars, one memory and one i/o.
>>>
>>> Why not use the BARs directly in the ns16550 driver rather than looking
>>> it up from the device tree? I assume the device will have to be
>>> enumerated anyway to make it work properly, at which point addresses
>>> should've been assigned to the memory and I/O BARs.
>>>
>>
>> It is because we cannot predict which bar to look up if we hardcod
>> that in the generic ns16550 driver. Normally PCI ns16550 registers can
>> be memory-mapped or I/O mapped and it could use any of the 6 BARs.
>> What's more, on x86 for memory-mapped and I/O mapped they use
>> different instructions to access the registers, and there is one build
>> time macro (CONFIG_SYS_NS16550_PORT_MAPPED) to control this.

Surely the vendor/device ID (or perhaps subvendor/device ID) directly 
imply which BAR is used for this purpose? It's really part of the 
specification of the interface to HW, so a particular bit of HW 
shouldn't be randomly deciding to use a different BAR register each 
power-on.

> That sounds like pretty arbitrary restrictions. For one you can detect
> invalid BARs, so selecting the right one should be easy. Also it seems
> like adding support for both I/O and memory BARs in the same binary
> should be relatively easy to do and not add a whole lot of code to the
> driver. But even if you only want one variant you can still select the
> correct port and not rely on the region defined in DTB.
>
> Like you said, bus number and addresses are assigned at runtime, so if
> you rely on DTB you also need code to make sure the DTB matches reality
> to prevent breakage. That sounds a lot more complicated than simply
> using what's there in the PCI configuration space already.
>
> Thierry
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>