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

[Thierry Reding]

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 at 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 at chromium.org> wrote:
> >> > Hi Sjoerd,
> >> >
> >> > On 20 January 2015 at 10:06, Sjoerd Simons
> >> > <sjoerd.simons at 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 at 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.

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/33f66dff/attachment.pgp>