[U-Boot] [PATCH v2] pci/fsl_pci_init: Enable inbound PCI config cycles

[U-Boot] [PATCH v2] pci/fsl_pci_init: Enable inbound PCI config cycles

[Peter Tyser]

Set CFG_READY bit in Configuration Ready register for PCIe
interfaces and clear ACL bit in PBFR register for PCI
interfaces to allow devices to respond to incoming PCI
configuration cycles.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
Changes since v1:
- Anal retentive update to the commit message:)

 drivers/pci/fsl_pci_init.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
index 7625ccc..6da9c22 100644
--- a/drivers/pci/fsl_pci_init.c
+++ b/drivers/pci/fsl_pci_init.c
@@ -37,6 +37,11 @@ DECLARE_GLOBAL_DATA_PTR;
 #include <pci.h>
 #include <asm/immap_fsl_pci.h>
 
+/* Freescale-specific PCI config registers */
+#define FSL_PCI_PBFR		0x44
+#define FSL_PCIE_CAP_ID		0x4c
+#define FSL_PCIE_CFG_RDY	0x4b0
+
 void pciauto_prescan_setup_bridge(struct pci_controller *hose,
 				pci_dev_t dev, int sub_bus);
 void pciauto_postscan_setup_bridge(struct pci_controller *hose,
@@ -302,6 +307,18 @@ void fsl_pci_init(struct pci_controller *hose)
 	if (temp16) {
 		pci_hose_write_config_word(hose, dev, PCI_SEC_STATUS, 0xffff);
 	}
+
+	/* Enable inbound PCI config cycles */
+	pci_hose_read_config_byte(hose, dev, FSL_PCIE_CAP_ID, &temp8);
+	if (temp8 != 0x0) {
+		/* PCIe - set CFG_READY bit of Configuration Ready Register */
+		pci_hose_write_config_byte(hose, dev, FSL_PCIE_CFG_RDY, 0x1);
+	} else {
+		/* PCI - clear ACL bit of PBFR */
+		pci_hose_read_config_word(hose, dev, FSL_PCI_PBFR, &temp16);
+		temp16 &= ~0x20;
+		pci_hose_write_config_byte(hose, dev, FSL_PCI_PBFR, temp16);
+	}
 }
 
 #ifdef CONFIG_OF_BOARD_SETUP
-- 
1.6.0.2.GIT

[U-Boot] [PATCH v2] pci/fsl_pci_init: Enable inbound PCI config cycles

[Kumar Gala]

On Oct 28, 2008, at 7:24 PM, Peter Tyser wrote:

> Set CFG_READY bit in Configuration Ready register for PCIe
> interfaces and clear ACL bit in PBFR register for PCI
> interfaces to allow devices to respond to incoming PCI
> configuration cycles.
>
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> ---
> Changes since v1:
> - Anal retentive update to the commit message:)
>
> drivers/pci/fsl_pci_init.c |   17 +++++++++++++++++
> 1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> index 7625ccc..6da9c22 100644
> --- a/drivers/pci/fsl_pci_init.c
> +++ b/drivers/pci/fsl_pci_init.c
> @@ -37,6 +37,11 @@ DECLARE_GLOBAL_DATA_PTR;
> #include <pci.h>
> #include <asm/immap_fsl_pci.h>
>
> +/* Freescale-specific PCI config registers */
> +#define FSL_PCI_PBFR		0x44
> +#define FSL_PCIE_CAP_ID		0x4c
> +#define FSL_PCIE_CFG_RDY	0x4b0
> +
> void pciauto_prescan_setup_bridge(struct pci_controller *hose,
> 				pci_dev_t dev, int sub_bus);
> void pciauto_postscan_setup_bridge(struct pci_controller *hose,
> @@ -302,6 +307,18 @@ void fsl_pci_init(struct pci_controller *hose)
> 	if (temp16) {
> 		pci_hose_write_config_word(hose, dev, PCI_SEC_STATUS, 0xffff);
> 	}
> +
> +	/* Enable inbound PCI config cycles */
> +	pci_hose_read_config_byte(hose, dev, FSL_PCIE_CAP_ID, &temp8);
> +	if (temp8 != 0x0) {
> +		/* PCIe - set CFG_READY bit of Configuration Ready Register */
> +		pci_hose_write_config_byte(hose, dev, FSL_PCIE_CFG_RDY, 0x1);
> +	} else {
> +		/* PCI - clear ACL bit of PBFR */
> +		pci_hose_read_config_word(hose, dev, FSL_PCI_PBFR, &temp16);
> +		temp16 &= ~0x20;
> +		pci_hose_write_config_byte(hose, dev, FSL_PCI_PBFR, temp16);
> +	}
> }

Shouldn't we only be doing this for an agent?  Also is the right place  
to enable it?  Just wondering if board code should have more  
flexibility here.

- k

[U-Boot] [PATCH v2] pci/fsl_pci_init: Enable inbound PCI config cycles

[Peter Tyser]

Hi Kumar,

> On Oct 28, 2008, at 7:24 PM, Peter Tyser wrote:
> 
>> Set CFG_READY bit in Configuration Ready register for PCIe
>> interfaces and clear ACL bit in PBFR register for PCI
>> interfaces to allow devices to respond to incoming PCI
>> configuration cycles.
>>
>> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
>> ---
>> Changes since v1:
>> - Anal retentive update to the commit message:)
>>
>> drivers/pci/fsl_pci_init.c |   17 +++++++++++++++++
>> 1 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>> index 7625ccc..6da9c22 100644
>> --- a/drivers/pci/fsl_pci_init.c
>> +++ b/drivers/pci/fsl_pci_init.c
>> @@ -37,6 +37,11 @@ DECLARE_GLOBAL_DATA_PTR;
>> #include <pci.h>
>> #include <asm/immap_fsl_pci.h>
>>
>> +/* Freescale-specific PCI config registers */
>> +#define FSL_PCI_PBFR        0x44
>> +#define FSL_PCIE_CAP_ID        0x4c
>> +#define FSL_PCIE_CFG_RDY    0x4b0
>> +
>> void pciauto_prescan_setup_bridge(struct pci_controller *hose,
>>                 pci_dev_t dev, int sub_bus);
>> void pciauto_postscan_setup_bridge(struct pci_controller *hose,
>> @@ -302,6 +307,18 @@ void fsl_pci_init(struct pci_controller *hose)
>>     if (temp16) {
>>         pci_hose_write_config_word(hose, dev, PCI_SEC_STATUS, 0xffff);
>>     }
>> +
>> +    /* Enable inbound PCI config cycles */
>> +    pci_hose_read_config_byte(hose, dev, FSL_PCIE_CAP_ID, &temp8);
>> +    if (temp8 != 0x0) {
>> +        /* PCIe - set CFG_READY bit of Configuration Ready Register */
>> +        pci_hose_write_config_byte(hose, dev, FSL_PCIE_CFG_RDY, 0x1);
>> +    } else {
>> +        /* PCI - clear ACL bit of PBFR */
>> +        pci_hose_read_config_word(hose, dev, FSL_PCI_PBFR, &temp16);
>> +        temp16 &= ~0x20;
>> +        pci_hose_write_config_byte(hose, dev, FSL_PCI_PBFR, temp16);
>> +    }
>> }
> 
> Shouldn't we only be doing this for an agent?  Also is the right place 
> to enable it?  Just wondering if board code should have more flexibility 
> here.

I was under the impression that if a PCIe interface was configured as 
root complex its CFG_READY bit would still need to be set in order for 
an endpoint device to perform config cycles to it (eg to see if the root 
complex device was sharing its memory, or if the root complex's shared 
memory was prefetchable, etc).  So I thought it made sense to 
unconditionally set the CFG_READY bit for PCIe interfaces.  Let me know 
if I'm missing something here (I thought some non-fsl chips could 
generate config cycles in endpoint mode)...

Looking at section 4.4.3.6 of the MPC8548 manual I thought that the ACL 
bit could be set on a host interface based on the value of the 
cfg_cpu_boot reset configuration bit.  Looking over the manual a bit 
more closely, it looks like if a PCI interface is configured as host it 
can't respond to inbound config cycles period (Table 16-65 of the 8548 
manual).  So for PCI hosts, ACL does not need to be cleared as you 
suggested.

As far as having more flexibility, I couldn't think of a reason that 
boards would want to have more fine grained control of this.  Is there 
an example you can think of where a board would want to do it differently?

Thanks for the comments,
Peter

[U-Boot] [PATCH v2] pci/fsl_pci_init: Enable inbound PCI config cycles

[Kumar Gala]

On Oct 28, 2008, at 11:03 PM, Peter Tyser wrote:

> Hi Kumar,
>
>> On Oct 28, 2008, at 7:24 PM, Peter Tyser wrote:
>>> Set CFG_READY bit in Configuration Ready register for PCIe
>>> interfaces and clear ACL bit in PBFR register for PCI
>>> interfaces to allow devices to respond to incoming PCI
>>> configuration cycles.
>>>
>>> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
>>> ---
>>> Changes since v1:
>>> - Anal retentive update to the commit message:)
>>>
>>> drivers/pci/fsl_pci_init.c |   17 +++++++++++++++++
>>> 1 files changed, 17 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>> index 7625ccc..6da9c22 100644
>>> --- a/drivers/pci/fsl_pci_init.c
>>> +++ b/drivers/pci/fsl_pci_init.c
>>> @@ -37,6 +37,11 @@ DECLARE_GLOBAL_DATA_PTR;
>>> #include <pci.h>
>>> #include <asm/immap_fsl_pci.h>
>>>
>>> +/* Freescale-specific PCI config registers */
>>> +#define FSL_PCI_PBFR        0x44
>>> +#define FSL_PCIE_CAP_ID        0x4c
>>> +#define FSL_PCIE_CFG_RDY    0x4b0
>>> +
>>> void pciauto_prescan_setup_bridge(struct pci_controller *hose,
>>>                pci_dev_t dev, int sub_bus);
>>> void pciauto_postscan_setup_bridge(struct pci_controller *hose,
>>> @@ -302,6 +307,18 @@ void fsl_pci_init(struct pci_controller *hose)
>>>    if (temp16) {
>>>        pci_hose_write_config_word(hose, dev, PCI_SEC_STATUS,  
>>> 0xffff);
>>>    }
>>> +
>>> +    /* Enable inbound PCI config cycles */
>>> +    pci_hose_read_config_byte(hose, dev, FSL_PCIE_CAP_ID, &temp8);
>>> +    if (temp8 != 0x0) {
>>> +        /* PCIe - set CFG_READY bit of Configuration Ready  
>>> Register */
>>> +        pci_hose_write_config_byte(hose, dev, FSL_PCIE_CFG_RDY,  
>>> 0x1);
>>> +    } else {
>>> +        /* PCI - clear ACL bit of PBFR */
>>> +        pci_hose_read_config_word(hose, dev, FSL_PCI_PBFR,  
>>> &temp16);
>>> +        temp16 &= ~0x20;
>>> +        pci_hose_write_config_byte(hose, dev, FSL_PCI_PBFR,  
>>> temp16);
>>> +    }
>>> }
>> Shouldn't we only be doing this for an agent?  Also is the right  
>> place to enable it?  Just wondering if board code should have more  
>> flexibility here.
>
> I was under the impression that if a PCIe interface was configured  
> as root complex its CFG_READY bit would still need to be set in  
> order for an endpoint device to perform config cycles to it (eg to  
> see if the root complex device was sharing its memory, or if the  
> root complex's shared memory was prefetchable, etc).  So I thought  
> it made sense to unconditionally set the CFG_READY bit for PCIe  
> interfaces.  Let me know if I'm missing something here (I thought  
> some non-fsl chips could generate config cycles in endpoint mode)...

No Freescale part that I'm aware of (ppc based) has ever allowed  
inbound config cycles if the device is in host mode/root complex.

> Looking at section 4.4.3.6 of the MPC8548 manual I thought that the  
> ACL bit could be set on a host interface based on the value of the  
> cfg_cpu_boot reset configuration bit.  Looking over the manual a bit  
> more closely, it looks like if a PCI interface is configured as host  
> it can't respond to inbound config cycles period (Table 16-65 of the  
> 8548 manual).  So for PCI hosts, ACL does not need to be cleared as  
> you suggested.
>
> As far as having more flexibility, I couldn't think of a reason that  
> boards would want to have more fine grained control of this.  Is  
> there an example you can think of where a board would want to do it  
> differently?

Its a timing issue if we are talking about using this for "agent"  
support.  The idea is once any cpu local software has run and setup  
things than it sets the ACL bit to allow an external master access.   
Its not clear when that point might be as we can't predicate what the  
local CPU SW wants setup before allowing access.

If you are only running your devices/boards in host mode this patch is  
not needed.

- k

[U-Boot] [PATCH v2] pci/fsl_pci_init: Enable inbound PCI config cycles

[Peter Tyser]

> >> Shouldn't we only be doing this for an agent?  Also is the right  
> >> place to enable it?  Just wondering if board code should have more  
> >> flexibility here.
> >
> > I was under the impression that if a PCIe interface was configured  
> > as root complex its CFG_READY bit would still need to be set in  
> > order for an endpoint device to perform config cycles to it (eg to  
> > see if the root complex device was sharing its memory, or if the  
> > root complex's shared memory was prefetchable, etc).  So I thought  
> > it made sense to unconditionally set the CFG_READY bit for PCIe  
> > interfaces.  Let me know if I'm missing something here (I thought  
> > some non-fsl chips could generate config cycles in endpoint mode)...
> 
> No Freescale part that I'm aware of (ppc based) has ever allowed  
> inbound config cycles if the device is in host mode/root complex.

Alright.

> > Looking at section 4.4.3.6 of the MPC8548 manual I thought that the  
> > ACL bit could be set on a host interface based on the value of the  
> > cfg_cpu_boot reset configuration bit.  Looking over the manual a bit  
> > more closely, it looks like if a PCI interface is configured as host  
> > it can't respond to inbound config cycles period (Table 16-65 of the  
> > 8548 manual).  So for PCI hosts, ACL does not need to be cleared as  
> > you suggested.
> >
> > As far as having more flexibility, I couldn't think of a reason that  
> > boards would want to have more fine grained control of this.  Is  
> > there an example you can think of where a board would want to do it  
> > differently?
> 
> Its a timing issue if we are talking about using this for "agent"  
> support.  The idea is once any cpu local software has run and setup  
> things than it sets the ACL bit to allow an external master access.   
> Its not clear when that point might be as we can't predicate what the  
> local CPU SW wants setup before allowing access.

It seems like 99% of the users could do any custom configuration of
inbound windows between the calls to fsl_pci_setup_inbound_windows() and
fsl_pci_init() in pci_init_board().  If its a concern I can pull the
configuration unlocking into a separate function to to allow for odd
situations.  That would allow a board to call it at any point in the
boot process (eg last_stage_init()).

> If you are only running your devices/boards in host mode this patch is  
> not needed.

Most of our Freescale boards require this change as they are PMCs, XMCs,
cPCI, etc boards which can be configured as host/agent/root
complex/endpoint dynamically depending on a carrier card, chassis, etc.
We also have a card with 2 Freescale CPUs connected via PCIe which
requires the change.

How about I pull the changes into a separate function and add checks to
ensure the the interface is host before allowing inbound config cycles?
I'd only add a call to the new function to the two boards I recently
added support for (assuming they are accepted:) and leave it to other
boards to add a call to it if they need it. 

Thanks,
Peter

[U-Boot] [PATCH v2] pci/fsl_pci_init: Enable inbound PCI config cycles

[Kumar Gala]

On Oct 29, 2008, at 10:05 AM, Peter Tyser wrote:

>
>>>> Shouldn't we only be doing this for an agent?  Also is the right
>>>> place to enable it?  Just wondering if board code should have more
>>>> flexibility here.
>>>
>>> I was under the impression that if a PCIe interface was configured
>>> as root complex its CFG_READY bit would still need to be set in
>>> order for an endpoint device to perform config cycles to it (eg to
>>> see if the root complex device was sharing its memory, or if the
>>> root complex's shared memory was prefetchable, etc).  So I thought
>>> it made sense to unconditionally set the CFG_READY bit for PCIe
>>> interfaces.  Let me know if I'm missing something here (I thought
>>> some non-fsl chips could generate config cycles in endpoint mode)...
>>
>> No Freescale part that I'm aware of (ppc based) has ever allowed
>> inbound config cycles if the device is in host mode/root complex.
>
> Alright.
>
>>> Looking at section 4.4.3.6 of the MPC8548 manual I thought that the
>>> ACL bit could be set on a host interface based on the value of the
>>> cfg_cpu_boot reset configuration bit.  Looking over the manual a bit
>>> more closely, it looks like if a PCI interface is configured as host
>>> it can't respond to inbound config cycles period (Table 16-65 of the
>>> 8548 manual).  So for PCI hosts, ACL does not need to be cleared as
>>> you suggested.
>>>
>>> As far as having more flexibility, I couldn't think of a reason that
>>> boards would want to have more fine grained control of this.  Is
>>> there an example you can think of where a board would want to do it
>>> differently?
>>
>> Its a timing issue if we are talking about using this for "agent"
>> support.  The idea is once any cpu local software has run and setup
>> things than it sets the ACL bit to allow an external master access.
>> Its not clear when that point might be as we can't predicate what the
>> local CPU SW wants setup before allowing access.
>
> It seems like 99% of the users could do any custom configuration of
> inbound windows between the calls to fsl_pci_setup_inbound_windows()  
> and
> fsl_pci_init() in pci_init_board().  If its a concern I can pull the
> configuration unlocking into a separate function to to allow for odd
> situations.  That would allow a board to call it at any point in the
> boot process (eg last_stage_init()).
>
>> If you are only running your devices/boards in host mode this patch  
>> is
>> not needed.
>
> Most of our Freescale boards require this change as they are PMCs,  
> XMCs,
> cPCI, etc boards which can be configured as host/agent/root
> complex/endpoint dynamically depending on a carrier card, chassis,  
> etc.
> We also have a card with 2 Freescale CPUs connected via PCIe which
> requires the change.
>
> How about I pull the changes into a separate function and add checks  
> to
> ensure the the interface is host before allowing inbound config  
> cycles?
> I'd only add a call to the new function to the two boards I recently
> added support for (assuming they are accepted:) and leave it to other
> boards to add a call to it if they need it.

perfect.

- k