[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Rostislav Lisovy]

Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
Reviewed-by: Yijing Wang <wangyijing@huawei.com>
---
The header file include/linux/pci_ids.h defines
#define PCI_CLASS_BRIDGE_OTHER          0x0680
#define PCI_CLASS_SYSTEM_DMA            0x0801

((struct pci_dev*)dev)->class
corresponds to the 3 bytes Class code in the PCI Configuration space
header -- 1B Base class, 1B Sub-class, 1B Reg-level interface.

In that case
(PCI_CLASS_BRIDGE_OTHER << 8)
is equivalent to 0x68000 (imagine the leading zero)
and
((PCI_CLASS_SYSTEM_DMA << 8) | 0x03))
is equivalent to 0x80103.


 arch/arm/kernel/bios32.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c1..e511ad1 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -255,10 +255,9 @@ static void pci_fixup_it8152(struct pci_dev *dev)
 {
 	int i;
 	/* fixup for ITE 8152 devices */
-	/* FIXME: add defines for class 0x68000 and 0x80103 */
 	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST ||
-	    dev->class == 0x68000 ||
-	    dev->class == 0x80103) {
+	    dev->class == (PCI_CLASS_BRIDGE_OTHER << 8) ||
+	    dev->class == ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03)) {
 		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
 			dev->resource[i].start = 0;
 			dev->resource[i].end   = 0;
-- 
1.7.10.4

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Rostislav Lisovy]

Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
Reviewed-by: Yijing Wang <wangyijing@huawei.com>
---
The header file include/linux/pci_ids.h defines
#define PCI_CLASS_BRIDGE_OTHER????????? 0x0680
#define PCI_CLASS_SYSTEM_DMA??????????? 0x0801

((struct pci_dev*)dev)->class
corresponds to the 3 bytes Class code in the PCI Configuration space
header -- 1B Base class, 1B Sub-class, 1B Reg-level interface.

In that case
(PCI_CLASS_BRIDGE_OTHER << 8)
is equivalent to 0x68000 (imagine the leading zero)
and
((PCI_CLASS_SYSTEM_DMA << 8) | 0x03))
is equivalent to 0x80103.


 arch/arm/kernel/bios32.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index 17a26c1..e511ad1 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -255,10 +255,9 @@ static void pci_fixup_it8152(struct pci_dev *dev)
 {
 	int i;
 	/* fixup for ITE 8152 devices */
-	/* FIXME: add defines for class 0x68000 and 0x80103 */
 	if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST ||
-	    dev->class == 0x68000 ||
-	    dev->class == 0x80103) {
+	    dev->class == (PCI_CLASS_BRIDGE_OTHER << 8) ||
+	    dev->class == ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03)) {
 		for (i = 0; i < PCI_NUM_RESOURCES; i++) {
 			dev->resource[i].start = 0;
 			dev->resource[i].end   = 0;
-- 
1.7.10.4

Re: [PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Bjorn Helgaas]

[+cc Mike]

This doesn't touch drivers/pci or conflict with any PCI-related
changes, so I'll leave this up to Russell.  My comments on this are
just kibbitzing.

On Mon, Sep 8, 2014 at 2:05 AM, Rostislav Lisovy <lisovy@gmail.com> wrote:
> Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
> Reviewed-by: Yijing Wang <wangyijing@huawei.com>
> ---
> The header file include/linux/pci_ids.h defines
> #define PCI_CLASS_BRIDGE_OTHER          0x0680
> #define PCI_CLASS_SYSTEM_DMA            0x0801
>
> ((struct pci_dev*)dev)->class
> corresponds to the 3 bytes Class code in the PCI Configuration space
> header -- 1B Base class, 1B Sub-class, 1B Reg-level interface.
>
> In that case
> (PCI_CLASS_BRIDGE_OTHER << 8)
> is equivalent to 0x68000 (imagine the leading zero)
> and
> ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03))
> is equivalent to 0x80103.

This part looks like it's supposed to be the changelog, which should
be above the Signed-off-by.

>  arch/arm/kernel/bios32.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..e511ad1 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -255,10 +255,9 @@ static void pci_fixup_it8152(struct pci_dev *dev)
>  {
>         int i;
>         /* fixup for ITE 8152 devices */
> -       /* FIXME: add defines for class 0x68000 and 0x80103 */
>         if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST ||
> -           dev->class == 0x68000 ||
> -           dev->class == 0x80103) {
> +           dev->class == (PCI_CLASS_BRIDGE_OTHER << 8) ||
> +           dev->class == ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03)) {
>                 for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>                         dev->resource[i].start = 0;
>                         dev->resource[i].end   = 0;

The interesting part to me is the part you didn't change, i.e.,
setting the resource start/end/flags to 0.  That's all from
a8fc0789558d ("[ARM] 4577/1: ITE 8152 PCI bridge support"), which
unfortunately doesn't have any more details about *why* this is
needed.

Presumably these devices have PCI BARs (since that's how dev->resource
got filled in), and we want to somehow ignore them, not assign
resources to them, and not allow drivers to use whatever is mapped by
the BARs.  But clearing out the struct resources isn't really safe by
itself.  The hardware BAR still contains some value, and may still be
enabled, which means that it may conflict with other devices.

I guess you're not making a functional change, so maybe we don't need
to worry about this.  But it does leave me scratching my head a bit.

Bjorn

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Bjorn Helgaas]

[+cc Mike]

This doesn't touch drivers/pci or conflict with any PCI-related
changes, so I'll leave this up to Russell.  My comments on this are
just kibbitzing.

On Mon, Sep 8, 2014 at 2:05 AM, Rostislav Lisovy <lisovy@gmail.com> wrote:
> Signed-off-by: Rostislav Lisovy <lisovy@gmail.com>
> Reviewed-by: Yijing Wang <wangyijing@huawei.com>
> ---
> The header file include/linux/pci_ids.h defines
> #define PCI_CLASS_BRIDGE_OTHER          0x0680
> #define PCI_CLASS_SYSTEM_DMA            0x0801
>
> ((struct pci_dev*)dev)->class
> corresponds to the 3 bytes Class code in the PCI Configuration space
> header -- 1B Base class, 1B Sub-class, 1B Reg-level interface.
>
> In that case
> (PCI_CLASS_BRIDGE_OTHER << 8)
> is equivalent to 0x68000 (imagine the leading zero)
> and
> ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03))
> is equivalent to 0x80103.

This part looks like it's supposed to be the changelog, which should
be above the Signed-off-by.

>  arch/arm/kernel/bios32.c |    5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
> index 17a26c1..e511ad1 100644
> --- a/arch/arm/kernel/bios32.c
> +++ b/arch/arm/kernel/bios32.c
> @@ -255,10 +255,9 @@ static void pci_fixup_it8152(struct pci_dev *dev)
>  {
>         int i;
>         /* fixup for ITE 8152 devices */
> -       /* FIXME: add defines for class 0x68000 and 0x80103 */
>         if ((dev->class >> 8) == PCI_CLASS_BRIDGE_HOST ||
> -           dev->class == 0x68000 ||
> -           dev->class == 0x80103) {
> +           dev->class == (PCI_CLASS_BRIDGE_OTHER << 8) ||
> +           dev->class == ((PCI_CLASS_SYSTEM_DMA << 8) | 0x03)) {
>                 for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>                         dev->resource[i].start = 0;
>                         dev->resource[i].end   = 0;

The interesting part to me is the part you didn't change, i.e.,
setting the resource start/end/flags to 0.  That's all from
a8fc0789558d ("[ARM] 4577/1: ITE 8152 PCI bridge support"), which
unfortunately doesn't have any more details about *why* this is
needed.

Presumably these devices have PCI BARs (since that's how dev->resource
got filled in), and we want to somehow ignore them, not assign
resources to them, and not allow drivers to use whatever is mapped by
the BARs.  But clearing out the struct resources isn't really safe by
itself.  The hardware BAR still contains some value, and may still be
enabled, which means that it may conflict with other devices.

I guess you're not making a functional change, so maybe we don't need
to worry about this.  But it does leave me scratching my head a bit.

Bjorn

Re: [PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Jason Gunthorpe]

On Mon, Sep 08, 2014 at 10:36:36AM -0600, Bjorn Helgaas wrote:

> Presumably these devices have PCI BARs (since that's how dev->resource
> got filled in), and we want to somehow ignore them, not assign
> resources to them, and not allow drivers to use whatever is mapped by
> the BARs.  But clearing out the struct resources isn't really safe by
> itself.  The hardware BAR still contains some value, and may still be
> enabled, which means that it may conflict with other devices.

I've seen this pattern before in the old mvebu stuff.

Essentially for that case the mvebu HW is a repurposed end port core,
so when working as a root port it presents an end port config space
(not a root port bridge config space). The BAR in that config space is
not relevant, so the old mvebu used code like this to hide it from the
PCI core. Otherwise the core will try to program it from the aperture,
and the default BAR size is really big, so it runs out of space.

Not sure if this is the same for this driver, but it looks very
similar..

Jason

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Jason Gunthorpe]

On Mon, Sep 08, 2014 at 10:36:36AM -0600, Bjorn Helgaas wrote:

> Presumably these devices have PCI BARs (since that's how dev->resource
> got filled in), and we want to somehow ignore them, not assign
> resources to them, and not allow drivers to use whatever is mapped by
> the BARs.  But clearing out the struct resources isn't really safe by
> itself.  The hardware BAR still contains some value, and may still be
> enabled, which means that it may conflict with other devices.

I've seen this pattern before in the old mvebu stuff.

Essentially for that case the mvebu HW is a repurposed end port core,
so when working as a root port it presents an end port config space
(not a root port bridge config space). The BAR in that config space is
not relevant, so the old mvebu used code like this to hide it from the
PCI core. Otherwise the core will try to program it from the aperture,
and the default BAR size is really big, so it runs out of space.

Not sure if this is the same for this driver, but it looks very
similar..

Jason

Re: [PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Bjorn Helgaas]

[-cc Mike's old email address]

On Mon, Sep 8, 2014 at 12:03 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Mon, Sep 08, 2014 at 10:36:36AM -0600, Bjorn Helgaas wrote:
>
>> Presumably these devices have PCI BARs (since that's how dev->resource
>> got filled in), and we want to somehow ignore them, not assign
>> resources to them, and not allow drivers to use whatever is mapped by
>> the BARs.  But clearing out the struct resources isn't really safe by
>> itself.  The hardware BAR still contains some value, and may still be
>> enabled, which means that it may conflict with other devices.
>
> I've seen this pattern before in the old mvebu stuff.
>
> Essentially for that case the mvebu HW is a repurposed end port core,
> so when working as a root port it presents an end port config space
> (not a root port bridge config space). The BAR in that config space is
> not relevant, so the old mvebu used code like this to hide it from the
> PCI core. Otherwise the core will try to program it from the aperture,
> and the default BAR size is really big, so it runs out of space.

How does mvebu deal with this now?  I don't see anything similar in pci-mvebu.c.

If the BAR is still a functioning PCI BAR, i.e., it decodes address
space and potentially responds to accesses there, we really should pay
attention to it.  That doesn't mean we have to assign space to it, but
for example, we might want to make sure we have PCI_COMMAND_MEM turned
off.  If we just clear out the resource start/end/flags, the PCI core
doesn't have enough information to do that.

Bjorn

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Bjorn Helgaas]

[-cc Mike's old email address]

On Mon, Sep 8, 2014 at 12:03 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Mon, Sep 08, 2014 at 10:36:36AM -0600, Bjorn Helgaas wrote:
>
>> Presumably these devices have PCI BARs (since that's how dev->resource
>> got filled in), and we want to somehow ignore them, not assign
>> resources to them, and not allow drivers to use whatever is mapped by
>> the BARs.  But clearing out the struct resources isn't really safe by
>> itself.  The hardware BAR still contains some value, and may still be
>> enabled, which means that it may conflict with other devices.
>
> I've seen this pattern before in the old mvebu stuff.
>
> Essentially for that case the mvebu HW is a repurposed end port core,
> so when working as a root port it presents an end port config space
> (not a root port bridge config space). The BAR in that config space is
> not relevant, so the old mvebu used code like this to hide it from the
> PCI core. Otherwise the core will try to program it from the aperture,
> and the default BAR size is really big, so it runs out of space.

How does mvebu deal with this now?  I don't see anything similar in pci-mvebu.c.

If the BAR is still a functioning PCI BAR, i.e., it decodes address
space and potentially responds to accesses there, we really should pay
attention to it.  That doesn't mean we have to assign space to it, but
for example, we might want to make sure we have PCI_COMMAND_MEM turned
off.  If we just clear out the resource start/end/flags, the PCI core
doesn't have enough information to do that.

Bjorn

Re: [PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Jason Gunthorpe]

On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote:

> > Essentially for that case the mvebu HW is a repurposed end port core,
> > so when working as a root port it presents an end port config space
> > (not a root port bridge config space). The BAR in that config space is
> > not relevant, so the old mvebu used code like this to hide it from the
> > PCI core. Otherwise the core will try to program it from the aperture,
> > and the default BAR size is really big, so it runs out of space.
> 
> How does mvebu deal with this now?  I don't see anything similar in
> pci-mvebu.c.

The new driver doesn't allow the core directly access to the end point
config space registers, the entire config space is managed by the
driver directly, exactly how the HW requires it to be set for root
port operation. The core sees the config space of a root port bridge
that is created by the driver.

> If the BAR is still a functioning PCI BAR, i.e., it decodes address
> space and potentially responds to accesses there, we really should pay
> attention to it.  That doesn't mean we have to assign space to it, but
> for example, we might want to make sure we have PCI_COMMAND_MEM turned
> off.  If we just clear out the resource start/end/flags, the PCI core
> doesn't have enough information to do that.

In mvebu land, the BAR does need to have a proper value. IIRC, with
the old driver the bootloader was expected to configure PCI to an
operational state, and the kernel was expected not to touch that BAR
register at all.

For mvebu the new driver explicitly sets that BAR properly like this:

        /* Setup BAR[1] to all DRAM banks. */
        mvebu_writel(port, dram->cs[0].base, PCIE_BAR_LO_OFF(1));
        mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
        mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
                     PCIE_BAR_CTRL_OFF(1));

[Note: mvebu has a memory mapped alias for the end port config space,
 which is what PCIE_BAR_LO_OFF is referencing.]

Eg, it is matching the DRAM. This is because in an end port the BAR
claims incoming MemRd/MemWr TLPs, and for root port operation you want
the HW to claim all TLPs related to system memory.

The old driver (linux/arch/arm/mach-orion5x/pci.c) did this:

static void rc_pci_fixup(struct pci_dev *dev)
{
        /*
         * Prevent enumeration of root complex.
         */
        if (dev->bus->parent == NULL && dev->devfn == 0) {
                int i;

                for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
                        dev->resource[i].start = 0;
                        dev->resource[i].end   = 0;
                        dev->resource[i].flags = 0;
                }
        }
}
DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);

Which seems very similar to what you quoted?

Jason

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Jason Gunthorpe]

On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote:

> > Essentially for that case the mvebu HW is a repurposed end port core,
> > so when working as a root port it presents an end port config space
> > (not a root port bridge config space). The BAR in that config space is
> > not relevant, so the old mvebu used code like this to hide it from the
> > PCI core. Otherwise the core will try to program it from the aperture,
> > and the default BAR size is really big, so it runs out of space.
> 
> How does mvebu deal with this now?  I don't see anything similar in
> pci-mvebu.c.

The new driver doesn't allow the core directly access to the end point
config space registers, the entire config space is managed by the
driver directly, exactly how the HW requires it to be set for root
port operation. The core sees the config space of a root port bridge
that is created by the driver.

> If the BAR is still a functioning PCI BAR, i.e., it decodes address
> space and potentially responds to accesses there, we really should pay
> attention to it.  That doesn't mean we have to assign space to it, but
> for example, we might want to make sure we have PCI_COMMAND_MEM turned
> off.  If we just clear out the resource start/end/flags, the PCI core
> doesn't have enough information to do that.

In mvebu land, the BAR does need to have a proper value. IIRC, with
the old driver the bootloader was expected to configure PCI to an
operational state, and the kernel was expected not to touch that BAR
register at all.

For mvebu the new driver explicitly sets that BAR properly like this:

        /* Setup BAR[1] to all DRAM banks. */
        mvebu_writel(port, dram->cs[0].base, PCIE_BAR_LO_OFF(1));
        mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
        mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
                     PCIE_BAR_CTRL_OFF(1));

[Note: mvebu has a memory mapped alias for the end port config space,
 which is what PCIE_BAR_LO_OFF is referencing.]

Eg, it is matching the DRAM. This is because in an end port the BAR
claims incoming MemRd/MemWr TLPs, and for root port operation you want
the HW to claim all TLPs related to system memory.

The old driver (linux/arch/arm/mach-orion5x/pci.c) did this:

static void rc_pci_fixup(struct pci_dev *dev)
{
        /*
         * Prevent enumeration of root complex.
         */
        if (dev->bus->parent == NULL && dev->devfn == 0) {
                int i;

                for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
                        dev->resource[i].start = 0;
                        dev->resource[i].end   = 0;
                        dev->resource[i].flags = 0;
                }
        }
}
DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);

Which seems very similar to what you quoted?

Jason

Re: [PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Bjorn Helgaas]

On Mon, Sep 8, 2014 at 2:04 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote:
>
>> > Essentially for that case the mvebu HW is a repurposed end port core,
>> > so when working as a root port it presents an end port config space
>> > (not a root port bridge config space). The BAR in that config space is
>> > not relevant, so the old mvebu used code like this to hide it from the
>> > PCI core. Otherwise the core will try to program it from the aperture,
>> > and the default BAR size is really big, so it runs out of space.

This is a bit of a tangent, but an endpoint has a type 0 config
header, while a root port is a bridge and has a type 1 config header.
I don't understand how the PCI core can operate a root port correctly
if that port has a type 0 config header.

>> How does mvebu deal with this now?  I don't see anything similar in
>> pci-mvebu.c.
>
> The new driver doesn't allow the core directly access to the end point
> config space registers, the entire config space is managed by the
> driver directly, exactly how the HW requires it to be set for root
> port operation. The core sees the config space of a root port bridge
> that is created by the driver.
>
>> If the BAR is still a functioning PCI BAR, i.e., it decodes address
>> space and potentially responds to accesses there, we really should pay
>> attention to it.  That doesn't mean we have to assign space to it, but
>> for example, we might want to make sure we have PCI_COMMAND_MEM turned
>> off.  If we just clear out the resource start/end/flags, the PCI core
>> doesn't have enough information to do that.
>
> In mvebu land, the BAR does need to have a proper value. IIRC, with
> the old driver the bootloader was expected to configure PCI to an
> operational state, and the kernel was expected not to touch that BAR
> register at all.
>
> For mvebu the new driver explicitly sets that BAR properly like this:
>
>         /* Setup BAR[1] to all DRAM banks. */
>         mvebu_writel(port, dram->cs[0].base, PCIE_BAR_LO_OFF(1));
>         mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
>         mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
>                      PCIE_BAR_CTRL_OFF(1));
>
> [Note: mvebu has a memory mapped alias for the end port config space,
>  which is what PCIE_BAR_LO_OFF is referencing.]
>
> Eg, it is matching the DRAM. This is because in an end port the BAR
> claims incoming MemRd/MemWr TLPs, and for root port operation you want
> the HW to claim all TLPs related to system memory.
>
> The old driver (linux/arch/arm/mach-orion5x/pci.c) did this:
>
> static void rc_pci_fixup(struct pci_dev *dev)
> {
>         /*
>          * Prevent enumeration of root complex.
>          */
>         if (dev->bus->parent == NULL && dev->devfn == 0) {
>                 int i;
>
>                 for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>                         dev->resource[i].start = 0;
>                         dev->resource[i].end   = 0;
>                         dev->resource[i].flags = 0;
>                 }
>         }
> }
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);
>
> Which seems very similar to what you quoted?

I haven't worked with devices like this before, so pardon my ignorance.

It sounds like when these devices are configured as root ports, they
have BARs visible to the host, which is how dev->resource[] would get
filled in to begin with.  We manage normal BARs so the CPU can perform
MMIO accesses to the device.  In this case, it sounds like the BARs
are more for the benefit of devices below the bridge that want to
perform DMA.  The PCI core certainly wouldn't know what to do about
something like that, so I agree the best thing to do is to clear out
the resource completely and pretend it doesn't exist.  I guess a new
(not repurposed) design would probably put stuff like this in
device-specific config space, not in architected BARs.

Bjorn

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Bjorn Helgaas]

On Mon, Sep 8, 2014 at 2:04 PM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote:
>
>> > Essentially for that case the mvebu HW is a repurposed end port core,
>> > so when working as a root port it presents an end port config space
>> > (not a root port bridge config space). The BAR in that config space is
>> > not relevant, so the old mvebu used code like this to hide it from the
>> > PCI core. Otherwise the core will try to program it from the aperture,
>> > and the default BAR size is really big, so it runs out of space.

This is a bit of a tangent, but an endpoint has a type 0 config
header, while a root port is a bridge and has a type 1 config header.
I don't understand how the PCI core can operate a root port correctly
if that port has a type 0 config header.

>> How does mvebu deal with this now?  I don't see anything similar in
>> pci-mvebu.c.
>
> The new driver doesn't allow the core directly access to the end point
> config space registers, the entire config space is managed by the
> driver directly, exactly how the HW requires it to be set for root
> port operation. The core sees the config space of a root port bridge
> that is created by the driver.
>
>> If the BAR is still a functioning PCI BAR, i.e., it decodes address
>> space and potentially responds to accesses there, we really should pay
>> attention to it.  That doesn't mean we have to assign space to it, but
>> for example, we might want to make sure we have PCI_COMMAND_MEM turned
>> off.  If we just clear out the resource start/end/flags, the PCI core
>> doesn't have enough information to do that.
>
> In mvebu land, the BAR does need to have a proper value. IIRC, with
> the old driver the bootloader was expected to configure PCI to an
> operational state, and the kernel was expected not to touch that BAR
> register at all.
>
> For mvebu the new driver explicitly sets that BAR properly like this:
>
>         /* Setup BAR[1] to all DRAM banks. */
>         mvebu_writel(port, dram->cs[0].base, PCIE_BAR_LO_OFF(1));
>         mvebu_writel(port, 0, PCIE_BAR_HI_OFF(1));
>         mvebu_writel(port, ((size - 1) & 0xffff0000) | 1,
>                      PCIE_BAR_CTRL_OFF(1));
>
> [Note: mvebu has a memory mapped alias for the end port config space,
>  which is what PCIE_BAR_LO_OFF is referencing.]
>
> Eg, it is matching the DRAM. This is because in an end port the BAR
> claims incoming MemRd/MemWr TLPs, and for root port operation you want
> the HW to claim all TLPs related to system memory.
>
> The old driver (linux/arch/arm/mach-orion5x/pci.c) did this:
>
> static void rc_pci_fixup(struct pci_dev *dev)
> {
>         /*
>          * Prevent enumeration of root complex.
>          */
>         if (dev->bus->parent == NULL && dev->devfn == 0) {
>                 int i;
>
>                 for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
>                         dev->resource[i].start = 0;
>                         dev->resource[i].end   = 0;
>                         dev->resource[i].flags = 0;
>                 }
>         }
> }
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_MARVELL, PCI_ANY_ID, rc_pci_fixup);
>
> Which seems very similar to what you quoted?

I haven't worked with devices like this before, so pardon my ignorance.

It sounds like when these devices are configured as root ports, they
have BARs visible to the host, which is how dev->resource[] would get
filled in to begin with.  We manage normal BARs so the CPU can perform
MMIO accesses to the device.  In this case, it sounds like the BARs
are more for the benefit of devices below the bridge that want to
perform DMA.  The PCI core certainly wouldn't know what to do about
something like that, so I agree the best thing to do is to clear out
the resource completely and pretend it doesn't exist.  I guess a new
(not repurposed) design would probably put stuff like this in
device-specific config space, not in architected BARs.

Bjorn

Re: [PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Jason Gunthorpe]

On Tue, Sep 09, 2014 at 01:31:46PM -0600, Bjorn Helgaas wrote:
> On Mon, Sep 8, 2014 at 2:04 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote:
> >
> >> > Essentially for that case the mvebu HW is a repurposed end port core,
> >> > so when working as a root port it presents an end port config space
> >> > (not a root port bridge config space). The BAR in that config space is
> >> > not relevant, so the old mvebu used code like this to hide it from the
> >> > PCI core. Otherwise the core will try to program it from the aperture,
> >> > and the default BAR size is really big, so it runs out of space.
> 
> This is a bit of a tangent, but an endpoint has a type 0 config
> header, while a root port is a bridge and has a type 1 config header.
> I don't understand how the PCI core can operate a root port correctly
> if that port has a type 0 config header.

Well, it cannot operate 'correctly' but something gets bodged
together enough to pass basic scenarios..

Everything gets flattened down to bus 0, so the end port config space
gets to be device 0x10, and then config access to other device numbers
are forwarded out as TLPs to whatever is connected, so typically the
connected device shows up as device 0x0.

> It sounds like when these devices are configured as root ports, they
> have BARs visible to the host, which is how dev->resource[] would get
> filled in to begin with.  We manage normal BARs so the CPU can perform
> MMIO accesses to the device.  In this case, it sounds like the BARs
> are more for the benefit of devices below the bridge that want to
> perform DMA.  The PCI core certainly wouldn't know what to do about
> something like that, so I agree the best thing to do is to clear out
> the resource completely and pretend it doesn't exist.  I guess a new
> (not repurposed) design would probably put stuff like this in
> device-specific config space, not in architected BARs.

Yes to all of this.

Jason

[PATCH RESEND] ARM: PCI: Use PCI_CLASS_* defines for PCI class

[Jason Gunthorpe]

On Tue, Sep 09, 2014 at 01:31:46PM -0600, Bjorn Helgaas wrote:
> On Mon, Sep 8, 2014 at 2:04 PM, Jason Gunthorpe
> <jgunthorpe@obsidianresearch.com> wrote:
> > On Mon, Sep 08, 2014 at 01:39:40PM -0600, Bjorn Helgaas wrote:
> >
> >> > Essentially for that case the mvebu HW is a repurposed end port core,
> >> > so when working as a root port it presents an end port config space
> >> > (not a root port bridge config space). The BAR in that config space is
> >> > not relevant, so the old mvebu used code like this to hide it from the
> >> > PCI core. Otherwise the core will try to program it from the aperture,
> >> > and the default BAR size is really big, so it runs out of space.
> 
> This is a bit of a tangent, but an endpoint has a type 0 config
> header, while a root port is a bridge and has a type 1 config header.
> I don't understand how the PCI core can operate a root port correctly
> if that port has a type 0 config header.

Well, it cannot operate 'correctly' but something gets bodged
together enough to pass basic scenarios..

Everything gets flattened down to bus 0, so the end port config space
gets to be device 0x10, and then config access to other device numbers
are forwarded out as TLPs to whatever is connected, so typically the
connected device shows up as device 0x0.

> It sounds like when these devices are configured as root ports, they
> have BARs visible to the host, which is how dev->resource[] would get
> filled in to begin with.  We manage normal BARs so the CPU can perform
> MMIO accesses to the device.  In this case, it sounds like the BARs
> are more for the benefit of devices below the bridge that want to
> perform DMA.  The PCI core certainly wouldn't know what to do about
> something like that, so I agree the best thing to do is to clear out
> the resource completely and pretend it doesn't exist.  I guess a new
> (not repurposed) design would probably put stuff like this in
> device-specific config space, not in architected BARs.

Yes to all of this.

Jason