Message-ID: <5462407.z4eX3u12aQ@wuerfel>
Date:	Tue, 22 Jul 2014 23:11:14 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Bjorn Helgaas <bhelgaas@...gle.com>
Cc:	Kishon Vijay Abraham I <kishon@...com>, devicetree@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-pci@...r.kernel.org,
	jg1.han@...sung.com, mohit.kumar@...com,
	linux-kernel@...r.kernel.org, grant.likely@...aro.org,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Marek Vasut <marex@...x.de>
Subject: Re: [PATCH v3 1/4] PCI: designware: Configuration space should be specified in 'reg'

On Tuesday 22 July 2014 14:53:26 Bjorn Helgaas wrote:
> > @@ -2,6 +2,10 @@
> >  
> >  Required properties:
> >  - compatible: should contain "snps,dw-pcie" to identify the core.
> > +- reg: Should contain the configuration address space.
> > +- reg-names: Must be "config" for the PCIe configuration space.
> > +    (The old way of getting the configuration address space from "ranges"
> > +    is deprecated and should be avoided.)
> 
> I'm not a devicetree person, so please excuse my floundering here.
> There doesn't seem to be consistent usage of "reg" or "reg-names".

I think the above is ok, given the constraints we hae.

> Here's what I found in the existing Documentation/devicetree/bindings/pci
> stuff:
>
>   83xx-512x-pci.txt
>     - reg: should contain two address length tuples
>         The first is for the internal pci bridge registers
>         The second is for the pci config space access registers

This is fine

>   fsl,imx6q-pcie.txt
>     - reg: base addresse and length of the pcie controller

This should have included an entry for the config space, it was a
mistake

>   host-generic-pci.txt
>     - reg            : The Configuration Space base address and size, as accessed
>                        from the parent bus.

This is fine.

>   nvidia,tegra20-pcie.txt
>     - reg: A list of physical base address and length for each set of controller
>       registers. Must contain an entry for each entry in the reg-names property.
>     - reg-names: Must include the following entries:
>       "pads": PADS registers
>       "afi": AFI registers
>       "cs": configuration space region

The names are a bit strange, but it's also ok. In general we should mandate
the order of the entries as well.

>   pci-rcar-gen2.txt
>     - reg:  A list of physical regions to access the device: the first is
>             the operational registers for the OHCI/EHCI controllers and the
>             second is for the bridge configuration and control registers.

This one is wrong, as previously discussed. The first region should
have been listed in the 'ranges' property, because these are the registers
of the child nodes (the actual PCI devices) rather than the host controller
itself.

>   ralink,rt3883-pci.txt
>      - reg: specifies the physical base address of the controller and
>        the length of the memory mapped region.

The wording is a bit strange but it seems otherwise correct.

>   rcar-pci.txt
>     - reg: base address and length of the pcie controller registers.

This is ok

>   samsung,exynos5440-pcie.txt
>     - reg: base addresses and lengths of the pcie controller,
>           the phy controller, additional register for the phy controller.

Could be a bit more verbose, but is not wrong. In order to work
with the "snps,dw-pcie" binding, this will need a reg-names property.

>   v3-v360epc-pci.txt
>     - reg: should contain the base address of the V3 adapter.

This has the same mistake that the pcie-dw driver has and should
be fixed by putting the config space in there.

> So my questions are:
> 
> 1) Is "reg" the right place for configuration space?  I assume this is
> CAM or ECAM.  Only 83xx-512x-pci.txt, host-generic-pci.txt, and
> nvidia,tegra20-pcie.txt mention using "reg" for config space; all the
> others use "reg" for register space for the PCIe controller itself.

We had long debates about this. The conclusion was that we should
have put them in 'reg' from the start and not used 'ranges' here.

> 2) Is "config" the correct value for "reg-names"?  Only
> nvidia,tegra20-pcie.txt has something similar, and it wants "cs".
> Should these be the same?

It would be better if they were the same, but it's not important.
The names are private to the device driver, and the register layout
within this region is driver specific, so we can't use common code
to parse them anyway.

> 3) Doesn't this add a revlock (kernels with this patch require a new
> device tree that adds "reg" and "reg-names"?  I know both Mohit and
> Jingoo acked this [1,2], so I'm just double-checking that this is
> expected and desired.

My understanding is that the patch still allows the old method as
a fallback if no reg with the name "config" is present.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists