[RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers

Wed Feb 24 19:01:19 PST 2016

Hi Bjorn

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas at kernel.org]
> Sent: 24 February 2016 23:26
> To: Gabriele Paoloni
> Cc: 'Mark Rutland'; Guohanjun (Hanjun Guo); Wangzhou (B); liudongdong
> (C); Linuxarm; qiujiang; 'bhelgaas at google.com'; 'arnd at arndb.de';
> 'Lorenzo.Pieralisi at arm.com'; 'tn at semihalf.com'; 'linux-
> pci at vger.kernel.org'; 'linux-kernel at vger.kernel.org'; xuwei (O);
> 'linux-acpi at vger.kernel.org'; 'jcm at redhat.com'; zhangjukuo; Liguozhu
> (Kenneth); 'linux-arm-kernel at lists.infradead.org'
> Subject: Re: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support for
> HiSilicon SoCs Host Controllers
> 
> On Wed, Feb 24, 2016 at 06:46:09AM +0000, Gabriele Paoloni wrote:
> >
> > Hi Bjorn, many thanks for replying
> >
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:helgaas at kernel.org]
> > > Sent: 24 February 2016 09:14
> > > To: Gabriele Paoloni
> > > Cc: 'Mark Rutland'; Guohanjun (Hanjun Guo); Wangzhou (B);
> liudongdong
> > > (C); Linuxarm; qiujiang; 'bhelgaas at google.com'; 'arnd at arndb.de';
> > > 'Lorenzo.Pieralisi at arm.com'; 'tn at semihalf.com'; 'linux-
> > > pci at vger.kernel.org'; 'linux-kernel at vger.kernel.org'; xuwei (O);
> > > 'linux-acpi at vger.kernel.org'; 'jcm at redhat.com'; zhangjukuo;
> Liguozhu
> > > (Kenneth); 'linux-arm-kernel at lists.infradead.org'
> > > Subject: Re: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI support
> for
> > > HiSilicon SoCs Host Controllers
> > >
> > > On Tue, Feb 23, 2016 at 02:47:22AM +0000, Gabriele Paoloni wrote:
> > > > > -----Original Message-----
> > > > > From: Gabriele Paoloni
> > > > > Sent: 10 February 2016 22:45
> > > > > To: Mark Rutland
> > > > > Cc: Guohanjun (Hanjun Guo); Wangzhou (B); liudongdong (C);
> Linuxarm;
> > > > > qiujiang; bhelgaas at google.com; arnd at arndb.de;
> > > Lorenzo.Pieralisi at arm.com;
> > > > > tn at semihalf.com; linux-pci at vger.kernel.org; linux-
> > > > > kernel at vger.kernel.org; xuwei (O); linux-acpi at vger.kernel.org;
> > > > > jcm at redhat.com; zhangjukuo; Liguozhu (Kenneth); linux-arm-
> > > > > kernel at lists.infradead.org
> > > > > Subject: RE: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI
> support
> > > for
> > > > > HiSilicon SoCs Host Controllers
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: linux-kernel-owner at vger.kernel.org [mailto:linux-
> kernel-
> > > > > > owner at vger.kernel.org] On Behalf Of Mark Rutland
> > > > > > Sent: 10 February 2016 11:13
> > > > > > To: Gabriele Paoloni
> > > > > > Cc: Guohanjun (Hanjun Guo); Wangzhou (B); liudongdong (C);
> > > Linuxarm;
> > > > > > qiujiang; bhelgaas at google.com; arnd at arndb.de;
> > > > > > Lorenzo.Pieralisi at arm.com; tn at semihalf.com; linux-
> > > pci at vger.kernel.org;
> > > > > > linux-kernel at vger.kernel.org; xuwei (O); linux-
> > > acpi at vger.kernel.org;
> > > > > > jcm at redhat.com; zhangjukuo; Liguozhu (Kenneth); linux-arm-
> > > > > > kernel at lists.infradead.org
> > > > > > Subject: Re: [RFC PATCH v3 3/3] PCI/ACPI: hisi: Add ACPI
> support
> > > for
> > > > > > HiSilicon SoCs Host Controllers
> > > > > >
> > > > > > On Wed, Feb 10, 2016 at 09:52:36AM +0000, Gabriele Paoloni
> wrote:
> > > > > > > Hi Mark
> > > > > > >
> > > > > > > > On Tue, Feb 09, 2016 at 05:34:20PM +0000, Gabriele
> Paoloni
> > > wrote:
> > > > > > > > > From: gabriele paoloni <gabriele.paoloni at huawei.com>
> > > > > > > > > +/*
> > > > > > > > > + * Retrieve rc_dbi base and size from _DSD
> > > > > > > > > + * Name (_DSD, Package () {
> > > > > > > > > + *	ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > > > > > > > + *	Package () {
> > > > > > > > > + *	Package () {"rc-dbi", Package () { 0x0,
> 0xb0080000,
> > > 0x0,
> > > > > > 0x10000
> > > > > > > > }},
> > > > > > > > > + *	}
> > > > > > > > > + *	})
> > > > > > > > > + */
> > > > > > > >
> > > > > > > > As above, this does not look right. ACPI has standard
> > > mechanisms
> > > > > > for
> > > > > > > > describing addresses. Making something up like this is
> not a
> > > good
> > > > > > idea.
> > > > > > >
> > > > > > > I am quite new to ACPI, may I ask you to explain a bit?
> > > > > >
> > > > > > ACPI has standard mechanisms for describing certain resources,
> > > and
> > > > > > these
> > > > > > should not be described in _DSD. Memory or IO address regions
> are
> > > > > such
> > > > > > resources (in _CRS, IIRC), and should not be described in
> _DSD.
> > > > >
> > > > > Hi Mark,
> > > > >
> > > > > In my case I think in need to look into the MCFG object as the
> > > problem
> > > > > I have is RC using a different range than the rest of the
> hierarchy.
> > > > >
> > > > > I'll investigate this and try to come with a solution in v4
> > > >
> > > > I have looked into this and in our case we cannot use the
> > > > standard MCFG object to pass the RC config space addresses.
> > > >
> > > > The reason is that in our HW we have the config base addresses of
> the
> > > > root complex ports that are less than 0x100000 byte distant one
> from
> > > > the other as we only map the first 0x10000 bytes.
> > >
> > > The ECAM spec requires 4096 bytes per function, 8 functions per
> > > device, 32 devices per bus, which means you need 0x100000 bytes of
> > > address space per bus.  If your device doesn't supply that, it
> doesn't
> > > really implement ECAM, and you probably can't use the standard ways
> of
> > > describing ECAM (MCFG, _CBA).
> >
> > Correct, our host bridge is non ECAM only for the RC bus config space;
> > for any other bus underneath the root bus we support ECAM access, so
> > we need a quirk for the RC config rd/wr
> 
> MCFG can specify a bus number range, so you might be able to use it
> to describe the ECAM space for buses below the root bus, e.g.,
> [bus 01-ff].

Yes, for this patchset we use MCFG for buses [01-ff] and _DSD for bus 0

> 
> > > > Now the MCFG acpi framework always fix the MCFG resource size to
> > > 0x100000
> > > > for each bus; therefore if we pass our RC addresses through MCFG
> we
> > > end
> > > > up with a resource conflict.
> > > >
> > > > To give you a practical example we are in a situation where we
> have:
> > > >
> > > > port0: [0x00000000b0080000 - 0x00000000b0080000 + 0x10000]
> > > > port1: [0x00000000b0090000 - 0x00000000b0090000 + 0x10000]
> > > > port2: [0x00000000b00A0000 - 0x00000000b00A0000 + 0x10000]
> > > > port3: [0x00000000b00B0000 - 0x00000000b00B0000 + 0x10000]
> > > >
> > > > So if we pass the base addresses through MCFG the resources
> > > > will overlap as MCFG will consider 0x100000 size for each base
> > > > address of the root complex (only the RC bus uses that address)
> > > >
> > > > So far I do not see many option other than using _DSD to pass
> > > > these RC config base addresses.
> > >
> > > I don't want to take over Mark's discussion, but I really do not
> think
> > > _DSD is the correct way to fix this.  _CRS is like a generalized
> PCI
> > > BAR.  A PCI device is only allowed to respond to address space
> > > reported via a BAR.  An ACPI device is only allowed to respond to
> > > address space reported via _CRS.  Those are important rules because
> > > they mean we can manage address space with generic code instead of
> > > device-specific code.
> >
> > From what I see in
> > http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L723
> > acpi_pci_probe_root_resources() parses the _CRS method and
> > it only works for MEM and IO resources, so I don't think it is
> > right to pass a config space address by _CRS or to modify the
> > current ACPI framework to support this.
> 
> Config space addresses are made up of [bus, device, function, register
> offset], and you're right that _CRS doesn't contain those addresses
> directly; _CRS only describes memory, I/O, and bus number ranges.
> 
> But part of the function of a host bridge is to convert memory or I/O
> accesses on the primary (CPU) side to PCI config accesses on the
> secondary (PCI) side, and this CPU-side memory or I/O region should be
> reported via _CRS.

Yes the memory and I/O ranges are specified in the _CRS method of the
root complex PCI device

> 
> I think the relevant spec is the PCI Firmware Spec, r3.0, sec 4.1.2.
> Note 2 in that section says the address range of an MMCFG region
> must be reserved by declaring a motherboard resource, i.e., included
> in the _CRS of a PNP0C02 or similar device.

I had a look a this. So yes the specs says that we should use the 
PNP0C02 device if MCFG is not supported. 
So probably I can use acpi_get_devices("PNP0C02",...) to retrieve it
from the quirk match function, I will look into this... 

> 
> > On the other side, since this is an exception only for the config
> > space address of our host controller (as said before all the buses
> > below the root one support ECAM), I think that it is right to put
> > this address as a device specific data (in fact the rest of the
> > config space addresses will be parsed from MCFG).
> 
> A kernel with no support for your host controller (and thus no
> knowledge of its _DSD) should still be able to operate the rest of the
> system correctly.  That means we must have a generic way to learn what
> address space is consumed by the host controller so we don't try to
> assign it to other devices.

This is something I don't understand much...
Are you talking about a scenario where we have a Kernel image compiled
without our host controller support and running on our platform?

In this case the quirk is not called and the standard acpi pci root
driver will run and probably the config rd/wr for bus 0 will fail...

I don't see in this case how the physical address that we specify in the
_DSD can be assigned to other devices...

maybe I misunderstood something here...

Many Thanks

Gab 

> 
> Bjorn