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

[Sjoerd Simons]

On Wed, 2015-01-21 at 09:24 +0100, Thierry Reding 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.

I don't think that patch makes sense to fix this particular issue. More
below

> > 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.
> 
> > >> 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);

for fdtdec_get_pci_bdf addr is an *in* parameter, not an out parameter.
So using it like this makes very little sense and will crash badly if
the dt does define a compatibility. 

Given this is tegra specific and the nvidia,tegra20-pcie.txt[0]
device-tree binding from linux defines the reg property as required just
checking that should be enough in all cases (as in potentailly also
using the compatibility string doesn't really add value)

> > 
> > 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?

The intention indeed was to have a "don't care", i guess a potential
cleanup might be to have add FDT_PCI_SPACE_ANY to the enum? 

However in that case fdtdec_get_pci_addr should probably be adapted to
check more carefully if the space match, given with the current code
specifying _SPACE_CONFIG can give you the address of any space...

> > >>         if (err < 0) {
> > >>                 error("failed to parse \"reg\" property");
> > >>                 return err;
> > >>         }
> > >>
> > >> -       *index = PCI_DEV(bdf) - 1;
> > >> +       *index = PCI_DEV(addr.phys_hi) - 1;
> > >>
> > >>         return 0;
> > >>  }
> > 
> > Based on this patch, I think Tegra's dts just uses "reg" to encode the
> > device number into fdt_pci_addr.phys_hi. The bus number and function
> > number are not there.
> 
> Both the bus and function numbers are 0. These reg properties describe
> the root ports, which are single-function devices and always on bus 0 by
> definition.
> 
> Thierry

0:
http://free-electrons.com/kerneldoc/latest/devicetree/bindings/pci/nvidia,tegra20-pcie.txt

-- 
Sjoerd Simons <sjoerd.simons at collabora.co.uk>
Collabora Ltd.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6170 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150121/43c5b44e/attachment.bin>