[PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Aaron Sierra]

The following commit prevents the MPC8548E on the XPedite5200 PrPMC
module from enumerating its PCI/PCI-X bus, though it was previously
able to:

    powerpc/fsl-pci: use 'Header Type' to identify PCIE mode

The previous patch prevents any Freescale PCI-X bridge from enumerating
the bus, if it is hardware strapped into Agent mode.

In PCI-X, the Host is responsible for driving the PCI-X initialization
pattern to devices on the bus, so that they know whether to operate in
conventional PCI or PCI-X mode as well as what the bus timing will be.
For a PCI-X PrPMC, the pattern is driven by the mezzanine carrier it is
installed onto. Therefore, PrPMCs are PCI-X Agents, but they may still
enumerate the bus.

This patch depends on firmware to determine if the bridge should
perform enumeration based on factors other than the Host/Agent mode,
such as the state of the VITA 32-defined MONARCH# signal. If firmware
has determined that enumeration should be allowed, then it will set the
bridge's Bus Master bit in the Command register.

Without firmware intervention, the Bus Master bit defaults to 1 in Host
mode and 0 in Agent mode.

Cc: Minghuan Lian <Minghuan.Lian@freescale.com>
Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 arch/powerpc/sysdev/fsl_pci.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_pci.c b/arch/powerpc/sysdev/fsl_pci.c
index 4bd091a..88d8844 100644
--- a/arch/powerpc/sysdev/fsl_pci.c
+++ b/arch/powerpc/sysdev/fsl_pci.c
@@ -463,7 +463,7 @@ int fsl_add_bridge(struct platform_device *pdev, int is_primary)
 	struct pci_controller *hose;
 	struct resource rsrc;
 	const int *bus_range;
-	u8 hdr_type, progif;
+	u8 hdr_type;
 	struct device_node *dev;
 	struct ccsr_pci __iomem *pci;
 
@@ -520,9 +520,22 @@ int fsl_add_bridge(struct platform_device *pdev, int is_primary)
 			goto no_bridge;
 
 	} else {
-		/* For PCI read PROG to identify controller mode */
-		early_read_config_byte(hose, 0, 0, PCI_CLASS_PROG, &progif);
-		if ((progif & 1) == 1)
+		u16 master;
+
+		/*
+		 * If the controller is PCI-X, then Host mode refers to a
+		 * bridge that drives the PCI-X initialization pattern to
+		 * indicate bus operating mode/frequency to devices on the bus.
+		 * Some hardware (specifically PrPMC modules) are Agents, since
+		 * the mezzanine carrier is responsible for driving the
+		 * pattern, but they still may perform bus enumeration.
+		 *
+		 * Allow the bridge to be used for enumeration, if hardware
+		 * strapping (Host mode) or firmware (Agent mode) has enabled
+		 * bus mastering.
+		 */
+		early_read_config_word(hose, 0, 0, PCI_COMMAND, &master);
+		if (!(master & PCI_COMMAND_MASTER))
 			goto no_bridge;
 	}
 
-- 
1.9.1

Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Scott Wood]

On Wed, 2014-08-20 at 18:51 -0500, Aaron Sierra wrote:
> @@ -520,9 +520,22 @@ int fsl_add_bridge(struct platform_device *pdev, int is_primary)
>  			goto no_bridge;
>  
>  	} else {
> -		/* For PCI read PROG to identify controller mode */
> -		early_read_config_byte(hose, 0, 0, PCI_CLASS_PROG, &progif);
> -		if ((progif & 1) == 1)
> +		u16 master;
> +
> +		/*
> +		 * If the controller is PCI-X, then Host mode refers to a
> +		 * bridge that drives the PCI-X initialization pattern to
> +		 * indicate bus operating mode/frequency to devices on the bus.
> +		 * Some hardware (specifically PrPMC modules) are Agents, since
> +		 * the mezzanine carrier is responsible for driving the
> +		 * pattern, but they still may perform bus enumeration.
> +		 *
> +		 * Allow the bridge to be used for enumeration, if hardware
> +		 * strapping (Host mode) or firmware (Agent mode) has enabled
> +		 * bus mastering.
> +		 */
> +		early_read_config_word(hose, 0, 0, PCI_COMMAND, &master);
> +		if (!(master & PCI_COMMAND_MASTER))
>  			goto no_bridge;
>  	}

Why wouldn't a normal PCI agent be able to bus master?

-Scott

Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Aaron Sierra]

----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> Sent: Thursday, August 21, 2014 4:19:56 PM
> 
> On Wed, 2014-08-20 at 18:51 -0500, Aaron Sierra wrote:
> > @@ -520,9 +520,22 @@ int fsl_add_bridge(struct platform_device *pdev, int
> > is_primary)
> >  			goto no_bridge;
> >  
> >  	} else {
> > -		/* For PCI read PROG to identify controller mode */
> > -		early_read_config_byte(hose, 0, 0, PCI_CLASS_PROG, &progif);
> > -		if ((progif & 1) == 1)
> > +		u16 master;
> > +
> > +		/*
> > +		 * If the controller is PCI-X, then Host mode refers to a
> > +		 * bridge that drives the PCI-X initialization pattern to
> > +		 * indicate bus operating mode/frequency to devices on the bus.
> > +		 * Some hardware (specifically PrPMC modules) are Agents, since
> > +		 * the mezzanine carrier is responsible for driving the
> > +		 * pattern, but they still may perform bus enumeration.
> > +		 *
> > +		 * Allow the bridge to be used for enumeration, if hardware
> > +		 * strapping (Host mode) or firmware (Agent mode) has enabled
> > +		 * bus mastering.
> > +		 */
> > +		early_read_config_word(hose, 0, 0, PCI_COMMAND, &master);
> > +		if (!(master & PCI_COMMAND_MASTER))
> >  			goto no_bridge;
> >  	}
> 
> Why wouldn't a normal PCI agent be able to bus master?
> 
> -Scott
> 

Short answer:

Simply because the hardware strapping for Host/Agent determines the
default state of the Bus Master bit in the Command register. Without
that bit being set, an Agent won't be able to send the PCI cycles
necessary to enumerate the bus.


Long answer:

I think there was an assumption in the patch that introduced the
regression that Host and Agent in conventional PCI and PCI-X are more
equivalent to PCIe Root Complex and Endpoint than they really are.

I think the purpose of Minghuan's patch was to not attempt to enumerate
the bus with a bridge that simply cannot do it. A PCIe Endpoint cannot
enumerate the bus and a PCI/PCI-X device that cannot master cycles on the
bus will not be able to enumerate the bus either.

The hardware strapping for Host/Agent mode determines the default state
of the Bus Master bit in the Command register.

In our more specialized, but still industry standardized, environment our
firmware must detect whether our board should be the bus's sole enumerator
and set the Bus Master bit accordingly.

My comment in the code still mentions Host and Agent mode, simply because
the code it is replacing based its decision on the mode.

-Aaron

Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Scott Wood]

On Thu, 2014-08-21 at 16:54 -0500, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Scott Wood" <scottwood@freescale.com>
> > Sent: Thursday, August 21, 2014 4:19:56 PM
> > 
> > On Wed, 2014-08-20 at 18:51 -0500, Aaron Sierra wrote:
> > > @@ -520,9 +520,22 @@ int fsl_add_bridge(struct platform_device *pdev, int
> > > is_primary)
> > >  			goto no_bridge;
> > >  
> > >  	} else {
> > > -		/* For PCI read PROG to identify controller mode */
> > > -		early_read_config_byte(hose, 0, 0, PCI_CLASS_PROG, &progif);
> > > -		if ((progif & 1) == 1)
> > > +		u16 master;
> > > +
> > > +		/*
> > > +		 * If the controller is PCI-X, then Host mode refers to a
> > > +		 * bridge that drives the PCI-X initialization pattern to
> > > +		 * indicate bus operating mode/frequency to devices on the bus.
> > > +		 * Some hardware (specifically PrPMC modules) are Agents, since
> > > +		 * the mezzanine carrier is responsible for driving the
> > > +		 * pattern, but they still may perform bus enumeration.
> > > +		 *
> > > +		 * Allow the bridge to be used for enumeration, if hardware
> > > +		 * strapping (Host mode) or firmware (Agent mode) has enabled
> > > +		 * bus mastering.
> > > +		 */
> > > +		early_read_config_word(hose, 0, 0, PCI_COMMAND, &master);
> > > +		if (!(master & PCI_COMMAND_MASTER))
> > >  			goto no_bridge;
> > >  	}
> > 
> > Why wouldn't a normal PCI agent be able to bus master?
> > 
> > -Scott
> > 
> 
> Short answer:
> 
> Simply because the hardware strapping for Host/Agent determines the
> default state of the Bus Master bit in the Command register. Without
> that bit being set, an Agent won't be able to send the PCI cycles
> necessary to enumerate the bus.

But what if the host has already set that bit before Linux boots?

> Long answer:
> 
> I think there was an assumption in the patch that introduced the
> regression that Host and Agent in conventional PCI and PCI-X are more
> equivalent to PCIe Root Complex and Endpoint than they really are.
> 
> I think the purpose of Minghuan's patch was to not attempt to enumerate
> the bus with a bridge that simply cannot do it. A PCIe Endpoint cannot
> enumerate the bus and a PCI/PCI-X device that cannot master cycles on the
> bus will not be able to enumerate the bus either.

No, the point is also to not enumerate the bus in cases where we
shouldn't.  We don't want to step on the host's toes.

> The hardware strapping for Host/Agent mode determines the default state
> of the Bus Master bit in the Command register.
> 
> In our more specialized, but still industry standardized, environment our
> firmware must detect whether our board should be the bus's sole enumerator
> and set the Bus Master bit accordingly.
> 
> My comment in the code still mentions Host and Agent mode, simply because
> the code it is replacing based its decision on the mode.

I understand why you need to do this -- I just don't think this is a
reliable way of detecting that you're in that situation.  How about a
kernel command line setting?

-Scott

Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Aaron Sierra]

----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> Sent: Thursday, August 21, 2014 5:01:46 PM
> 
> On Thu, 2014-08-21 at 16:54 -0500, Aaron Sierra wrote:
> > ----- Original Message -----
> > > From: "Scott Wood" <scottwood@freescale.com>
> > > Sent: Thursday, August 21, 2014 4:19:56 PM
> > > 
> > > On Wed, 2014-08-20 at 18:51 -0500, Aaron Sierra wrote:
> > > > @@ -520,9 +520,22 @@ int fsl_add_bridge(struct platform_device *pdev,
> > > > int
> > > > is_primary)
> > > >  			goto no_bridge;
> > > >  
> > > >  	} else {
> > > > -		/* For PCI read PROG to identify controller mode */
> > > > -		early_read_config_byte(hose, 0, 0, PCI_CLASS_PROG, &progif);
> > > > -		if ((progif & 1) == 1)
> > > > +		u16 master;
> > > > +
> > > > +		/*
> > > > +		 * If the controller is PCI-X, then Host mode refers to a
> > > > +		 * bridge that drives the PCI-X initialization pattern to
> > > > +		 * indicate bus operating mode/frequency to devices on the bus.
> > > > +		 * Some hardware (specifically PrPMC modules) are Agents, since
> > > > +		 * the mezzanine carrier is responsible for driving the
> > > > +		 * pattern, but they still may perform bus enumeration.
> > > > +		 *
> > > > +		 * Allow the bridge to be used for enumeration, if hardware
> > > > +		 * strapping (Host mode) or firmware (Agent mode) has enabled
> > > > +		 * bus mastering.
> > > > +		 */
> > > > +		early_read_config_word(hose, 0, 0, PCI_COMMAND, &master);
> > > > +		if (!(master & PCI_COMMAND_MASTER))
> > > >  			goto no_bridge;
> > > >  	}
> > > 
> > > Why wouldn't a normal PCI agent be able to bus master?
> > > 
> > > -Scott
> > > 
> > 
> > Short answer:
> > 
> > Simply because the hardware strapping for Host/Agent determines the
> > default state of the Bus Master bit in the Command register. Without
> > that bit being set, an Agent won't be able to send the PCI cycles
> > necessary to enumerate the bus.
> 
> But what if the host has already set that bit before Linux boots?

That's a very good point. I think that concern can be addressed by looking
for another telltale sign of enumeration, whether an address has been
assigned to the bridge's BAR 0 (PCSRBAR).

> > Long answer:
> > 
> > I think there was an assumption in the patch that introduced the
> > regression that Host and Agent in conventional PCI and PCI-X are more
> > equivalent to PCIe Root Complex and Endpoint than they really are.
> > 
> > I think the purpose of Minghuan's patch was to not attempt to enumerate
> > the bus with a bridge that simply cannot do it. A PCIe Endpoint cannot
> > enumerate the bus and a PCI/PCI-X device that cannot master cycles on the
> > bus will not be able to enumerate the bus either.
> 
> No, the point is also to not enumerate the bus in cases where we
> shouldn't.  We don't want to step on the host's toes.

Agreed.
 
> > The hardware strapping for Host/Agent mode determines the default state
> > of the Bus Master bit in the Command register.
> > 
> > In our more specialized, but still industry standardized, environment our
> > firmware must detect whether our board should be the bus's sole enumerator
> > and set the Bus Master bit accordingly.
> > 
> > My comment in the code still mentions Host and Agent mode, simply because
> > the code it is replacing based its decision on the mode.
> 
> I understand why you need to do this -- I just don't think this is a
> reliable way of detecting that you're in that situation.  How about a
> kernel command line setting?

I'd like to avoid requiring a kernel command-line option for this.

Instead, I propose checking for Agent mode like Minghuan's patch, but
prohibiting enumeration only if Bus Master is not enabled OR the address
portion of PCSRBAR is non-zero (indicating another device has enumerated
the bus).

Pseudo code:
    if (agent && (!master || enumerated))
        goto no_bridge;

-Aaron

Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Scott Wood]

On Fri, 2014-08-22 at 12:54 -0500, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Scott Wood" <scottwood@freescale.com>
> > Sent: Thursday, August 21, 2014 5:01:46 PM
> > 
> > On Thu, 2014-08-21 at 16:54 -0500, Aaron Sierra wrote:
> > > ----- Original Message -----
> > > > From: "Scott Wood" <scottwood@freescale.com>
> > > > Sent: Thursday, August 21, 2014 4:19:56 PM
> > > > 
> > > > Why wouldn't a normal PCI agent be able to bus master?
> > > > 
> > > > -Scott
> > > > 
> > > 
> > > Short answer:
> > > 
> > > Simply because the hardware strapping for Host/Agent determines the
> > > default state of the Bus Master bit in the Command register. Without
> > > that bit being set, an Agent won't be able to send the PCI cycles
> > > necessary to enumerate the bus.
> > 
> > But what if the host has already set that bit before Linux boots?
> 
> That's a very good point. I think that concern can be addressed by looking
> for another telltale sign of enumeration, whether an address has been
> assigned to the bridge's BAR 0 (PCSRBAR).

I don't see how that's any different.  The host may or may not have
assigned an address.

> > I understand why you need to do this -- I just don't think this is a
> > reliable way of detecting that you're in that situation.  How about a
> > kernel command line setting?
> 
> I'd like to avoid requiring a kernel command-line option for this.

It's hardware description, so you could use a device tree property.

-Scott

Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Aaron Sierra]

----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> To: "Aaron Sierra" <asierra@xes-inc.com>
> Cc: linuxppc-dev@lists.ozlabs.org, "Minghuan Lian" <Minghuan.Lian@freescale.com>
> Sent: Friday, August 22, 2014 1:36:31 PM
> Subject: Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression
> 
> On Fri, 2014-08-22 at 12:54 -0500, Aaron Sierra wrote:
> > ----- Original Message -----
> > > From: "Scott Wood" <scottwood@freescale.com>
> > > Sent: Thursday, August 21, 2014 5:01:46 PM
> > > 
> > > On Thu, 2014-08-21 at 16:54 -0500, Aaron Sierra wrote:
> > > > ----- Original Message -----
> > > > > From: "Scott Wood" <scottwood@freescale.com>
> > > > > Sent: Thursday, August 21, 2014 4:19:56 PM
> > > > > 
> > > > > Why wouldn't a normal PCI agent be able to bus master?
> > > > > 
> > > > > -Scott
> > > > > 
> > > > 
> > > > Short answer:
> > > > 
> > > > Simply because the hardware strapping for Host/Agent determines the
> > > > default state of the Bus Master bit in the Command register. Without
> > > > that bit being set, an Agent won't be able to send the PCI cycles
> > > > necessary to enumerate the bus.
> > > 
> > > But what if the host has already set that bit before Linux boots?
> > 
> > That's a very good point. I think that concern can be addressed by looking
> > for another telltale sign of enumeration, whether an address has been
> > assigned to the bridge's BAR 0 (PCSRBAR).
> 
> I don't see how that's any different.  The host may or may not have
> assigned an address.

I don't agree with that. If the host has enabled bus mastering for the
device, then it also surely would also have assigned an address to the
always on PCSRBAR during enumeration.

In what sort of environment would a host have enabled bus mastering for
a peripheral device _before_ assigning addresses to its BARs?

> > > I understand why you need to do this -- I just don't think this is a
> > > reliable way of detecting that you're in that situation.  How about a
> > > kernel command line setting?
> > 
> > I'd like to avoid requiring a kernel command-line option for this.
> 
> It's hardware description, so you could use a device tree property.
> 

I like this better than a command-line option, but what are you
suggesting? Would you want a device-tree property to gate the logic that
I've proposed?

-Aaron

Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression

[Scott Wood]

On Fri, 2014-08-22 at 14:00 -0500, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Scott Wood" <scottwood@freescale.com>
> > To: "Aaron Sierra" <asierra@xes-inc.com>
> > Cc: linuxppc-dev@lists.ozlabs.org, "Minghuan Lian" <Minghuan.Lian@freescale.com>
> > Sent: Friday, August 22, 2014 1:36:31 PM
> > Subject: Re: [PATCH] powerpc: fsl_pci: Fix PCI/PCI-X regression
> > 
> > On Fri, 2014-08-22 at 12:54 -0500, Aaron Sierra wrote:
> > > ----- Original Message -----
> > > > From: "Scott Wood" <scottwood@freescale.com>
> > > > Sent: Thursday, August 21, 2014 5:01:46 PM
> > > > 
> > > > On Thu, 2014-08-21 at 16:54 -0500, Aaron Sierra wrote:
> > > > > ----- Original Message -----
> > > > > > From: "Scott Wood" <scottwood@freescale.com>
> > > > > > Sent: Thursday, August 21, 2014 4:19:56 PM
> > > > > > 
> > > > > > Why wouldn't a normal PCI agent be able to bus master?
> > > > > > 
> > > > > > -Scott
> > > > > > 
> > > > > 
> > > > > Short answer:
> > > > > 
> > > > > Simply because the hardware strapping for Host/Agent determines the
> > > > > default state of the Bus Master bit in the Command register. Without
> > > > > that bit being set, an Agent won't be able to send the PCI cycles
> > > > > necessary to enumerate the bus.
> > > > 
> > > > But what if the host has already set that bit before Linux boots?
> > > 
> > > That's a very good point. I think that concern can be addressed by looking
> > > for another telltale sign of enumeration, whether an address has been
> > > assigned to the bridge's BAR 0 (PCSRBAR).
> > 
> > I don't see how that's any different.  The host may or may not have
> > assigned an address.
> 
> I don't agree with that. If the host has enabled bus mastering for the
> device, then it also surely would also have assigned an address to the
> always on PCSRBAR during enumeration.
> 
> In what sort of environment would a host have enabled bus mastering for
> a peripheral device _before_ assigning addresses to its BARs?

It's unusual, but not illegal and doing so shouldn't cause a PCI device
to try to take over the system.

Plus, zero is (arguably) a valid BAR address.

> > > > I understand why you need to do this -- I just don't think this is a
> > > > reliable way of detecting that you're in that situation.  How about a
> > > > kernel command line setting?
> > > 
> > > I'd like to avoid requiring a kernel command-line option for this.
> > 
> > It's hardware description, so you could use a device tree property.
> > 
> 
> I like this better than a command-line option, but what are you
> suggesting? Would you want a device-tree property to gate the logic that
> I've proposed?

I don't think you need the logic you've proposed if you have a device
tree property -- the property would assert that you are the owner of the
bus despite being an agent.

-Scott