Re: [PATCH v3 05/10] resource: add PCI configuration space support

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 5050 bytes --]

On Tue, Aug 14, 2012 at 10:38:08AM -0700, Bjorn Helgaas wrote:
> On Mon, Aug 13, 2012 at 10:55 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Mon, Aug 13, 2012 at 10:00:45PM -0700, Bjorn Helgaas wrote:
> >> On Thu, Jul 26, 2012 at 12:55 PM, Thierry Reding
> >> <thierry.reding@avionic-design.de> wrote:
> >> > This commit adds a new flag that allows marking resources as PCI
> >> > configuration space.
> >> >
> >> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> >> > ---
> >> > Changes in v3:
> >> > - new patch
> >> >
> >> >  include/linux/ioport.h | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> >> > index 589e0e7..3314843 100644
> >> > --- a/include/linux/ioport.h
> >> > +++ b/include/linux/ioport.h
> >> > @@ -102,7 +102,7 @@ struct resource {
> >> >
> >> >  /* PCI control bits.  Shares IORESOURCE_BITS with above PCI ROM.  */
> >> >  #define IORESOURCE_PCI_FIXED           (1<<4)  /* Do not move resource */
> >> > -
> >> > +#define IORESOURCE_PCI_CS              (1<<5)  /* PCI configuration space */
> >>
> >> What is the purpose of this?  It seems that you are marking regions
> >> that we call MMCONFIG on x86, or ECAM-type regions in the language of
> >> the PCIe spec.  I see that you set it in several places, but I don't
> >> see anything that ever looks for it.  Do you have plans to use it in
> >> the future?  If it really does correspond to MMCONFIG/ECAM, we should
> >> handle those regions consistently across all architectures.
> >
> > The purpose is ultimately to obtain the MMCONFIG/ECAM resources assigned
> > to a PCI host controller. I've used this in the of_pci_parse_ranges()
> > and in the static board setup code to mark ranges as such. Perhaps
> > IORESOURCE_ECAM or IORESOURCE_MMCONFIG might have been better names. I
> > also just noticed that I'm not using this anywhere, but the plan was to
> > eventually use it with platform_get_resource(). However that doesn't
> > seem to work either because the lower bits of the flags aren't use for
> > comparison in that function.
> >
> > Any other ideas how that could be handled? Basically what I need is a
> > way to mark a resource as an MMCONFIG/ECAM range so that it can be used
> > to program the PCI host controller accordingly. I don't know how these
> > are assigned on x86. I was under the impression that the MMCONFIG/ECAM
> > space was accessed through a single single address/data register pair.
> 
> The legacy config access mechanism (CF8h/CFCh registers described in
> PCI 3.0 spec sec 3.2.2.3.2) is a single address/data pair, but this is
> mostly x86-specific.  The ECAM mechanism (described in the PCIe 3.0
> spec sec 7.2.2) is not a single address/data pair; instead, each byte
> of config space is directly mapped into the host's MMIO space.
> 
> Here's what we do on x86 (omitting some historical grunge that
> complicates things):
> 
>   - Discover the host bridge via a PNP0A08 device in the ACPI namespace.
>   - Discover the bus number range behind the bridge using a _CRS
> method in the PNP0A08 device.
>   - Discover the ECAM space for those buses via a _CBA method in the
> PNP0A08 device.
>   - Tell the config accessors (struct pci_ops) that the ECAM space for
> buses A-B is at address X.
>   - Enumerate the devices behind the host bridge by calling
> pci_scan_root_bus(), passing the config accessors.
> 
> It sounds like you want a way to parse the resources at one point,
> saving them and marking the ECAM region, then at a later point, look
> up the ECAM from a saved list.  We don't do that on x86 because the
> config accessors keep an internal list of the ECAM area for each bus.
> 
> We do of course want to put this ECAM space in the IORESOURCE_MEM tree
> because it consumes address space and we have to make sure we don't
> put anything else on top of it.  But we don't have any reason to
> describe the MMIO -> config space function in that tree.  From the
> point of view of the rest of the system, it's just MMIO space that's
> consumed by the PCI host bridge, just like any other device-specific
> MMIO area.

What I currently do is pass the ECAM space as a resource to the PCI host
bridge platform device. Regular and extended configuration spaces are
given by the third and fourth resources, respectively. If I understand
correctly, you're saying that nothing beyond that needs to be encoded.
In other words it is enough for the PCI host bridge driver to know where
to take the data from.

I'll have to see what this means for the DT binding. There are other
issues that I need to think about, like for example how to pass the ECAM
space from the PCI host controller to each of the two bridges via the
ranges property. This no longer makes sense in the current form, as the
ECAM covers the configuration spaces for devices of both bridges and
cannot really be split among them.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]