[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

Most FSL PCIe controllers expects 333 MHz PCI reference clock.
This clock is derived from the CCB but in many cases the ref.
clock is not 333 MHz and a divisor needs to be configured.

This adds PEX_CCB_DIV #define which can be defined for each
type of CPU/platform.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/pci/fsl_pci_init.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
index 52792dcd59..4d00b3f26c 100644
--- a/drivers/pci/fsl_pci_init.c
+++ b/drivers/pci/fsl_pci_init.c
@@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
 
 	pci_setup_indirect(hose, cfg_addr, cfg_data);
 
+#ifdef PEX_CCB_DIV
+	/* Configure the PCIE controller core clock ratio */
+	pci_hose_write_config_dword(hose, dev, 0x440,
+				    ((gd->bus_clk / 1000000) *
+				     (16 / PEX_CCB_DIV)) / 333);
+#endif
 	block_rev = in_be32(&pci->block_rev1);
 	if (PEX_IP_BLK_REV_2_2 <= block_rev) {
 		pi = &pci->pit[2];	/* 0xDC0 */
-- 
2.13.5

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 09/12/2017 10:56 AM, Joakim Tjernlund wrote:
> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> This clock is derived from the CCB but in many cases the ref.
> clock is not 333 MHz and a divisor needs to be configured.
> 
> This adds PEX_CCB_DIV #define which can be defined for each
> type of CPU/platform.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
>   drivers/pci/fsl_pci_init.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> index 52792dcd59..4d00b3f26c 100644
> --- a/drivers/pci/fsl_pci_init.c
> +++ b/drivers/pci/fsl_pci_init.c
> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>   
>   	pci_setup_indirect(hose, cfg_addr, cfg_data);
>   
> +#ifdef PEX_CCB_DIV
> +	/* Configure the PCIE controller core clock ratio */
> +	pci_hose_write_config_dword(hose, dev, 0x440,
> +				    ((gd->bus_clk / 1000000) *
> +				     (16 / PEX_CCB_DIV)) / 333);
> +#endif
>   	block_rev = in_be32(&pci->block_rev1);
>   	if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>   		pi = &pci->pit[2];	/* 0xDC0 */
> 

Mingkai,

Do you ack this change? This presumes the PCIe clock derives from CCB 
bus clock.

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Mingkai Hu]

> -----Original Message-----
> From: York Sun
> Sent: Friday, September 15, 2017 5:15 AM
> To: Joakim Tjernlund <joakim.tjernlund@infinera.com>; Mingkai Hu
> <mingkai.hu@nxp.com>; u-boot @ lists . denx . de <u-boot@lists.denx.de>;
> Roy Zang <roy.zang@nxp.com>
> Subject: Re: [PATCH] FSL PCI: Configure PCIe reference ratio
> 
> On 09/12/2017 10:56 AM, Joakim Tjernlund wrote:
> > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > This clock is derived from the CCB but in many cases the ref.
> > clock is not 333 MHz and a divisor needs to be configured.
> >
> > This adds PEX_CCB_DIV #define which can be defined for each type of
> > CPU/platform.
> >
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> >   drivers/pci/fsl_pci_init.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > index 52792dcd59..4d00b3f26c 100644
> > --- a/drivers/pci/fsl_pci_init.c
> > +++ b/drivers/pci/fsl_pci_init.c
> > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose,
> > struct fsl_pci_info *pci_info)
> >
> >   	pci_setup_indirect(hose, cfg_addr, cfg_data);
> >
> > +#ifdef PEX_CCB_DIV
> > +	/* Configure the PCIE controller core clock ratio */
> > +	pci_hose_write_config_dword(hose, dev, 0x440,
> > +				    ((gd->bus_clk / 1000000) *
> > +				     (16 / PEX_CCB_DIV)) / 333);
> > +#endif
> >   	block_rev = in_be32(&pci->block_rev1);
> >   	if (PEX_IP_BLK_REV_2_2 <= block_rev) {
> >   		pi = &pci->pit[2];	/* 0xDC0 */
> >
> 
> Mingkai,
> 
> Do you ack this change? This presumes the PCIe clock derives from CCB bus
> clock.
> 

gd->bus_clk indicates the platform clock while PCIe clock could be CCB or CCB/2.
For example, it's CCB/2 for T1040/T1020, CCB for P2020.

I suggest to add the PCIe clock in gd to handle this difference, just like what we've done for other IP clocks.

Thanks,
Mingkai

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> This clock is derived from the CCB but in many cases the ref.
> clock is not 333 MHz and a divisor needs to be configured.
> 
> This adds PEX_CCB_DIV #define which can be defined for each
> type of CPU/platform.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
>  drivers/pci/fsl_pci_init.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> index 52792dcd59..4d00b3f26c 100644
> --- a/drivers/pci/fsl_pci_init.c
> +++ b/drivers/pci/fsl_pci_init.c
> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>  
>  	pci_setup_indirect(hose, cfg_addr, cfg_data);
>  
> +#ifdef PEX_CCB_DIV
> +	/* Configure the PCIE controller core clock ratio */
> +	pci_hose_write_config_dword(hose, dev, 0x440,
> +				    ((gd->bus_clk / 1000000) *
> +				     (16 / PEX_CCB_DIV)) / 333);
> +#endif
>  	block_rev = in_be32(&pci->block_rev1);
>  	if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>  		pi = &pci->pit[2];	/* 0xDC0 */

Ping? 

 Jocke

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>> This clock is derived from the CCB but in many cases the ref.
>> clock is not 333 MHz and a divisor needs to be configured.
>>
>> This adds PEX_CCB_DIV #define which can be defined for each
>> type of CPU/platform.
>>
>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>> ---
>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>> index 52792dcd59..4d00b3f26c 100644
>> --- a/drivers/pci/fsl_pci_init.c
>> +++ b/drivers/pci/fsl_pci_init.c
>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>  
>>  	pci_setup_indirect(hose, cfg_addr, cfg_data);
>>  
>> +#ifdef PEX_CCB_DIV
>> +	/* Configure the PCIE controller core clock ratio */
>> +	pci_hose_write_config_dword(hose, dev, 0x440,
>> +				    ((gd->bus_clk / 1000000) *
>> +				     (16 / PEX_CCB_DIV)) / 333);
>> +#endif
>>  	block_rev = in_be32(&pci->block_rev1);
>>  	if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>>  		pi = &pci->pit[2];	/* 0xDC0 */
> 
> Ping? 
> 
>  Jocke
> 

I believe Mingkai's last comment was "to add the PCIe clock in gd".

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> > On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> > > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > > This clock is derived from the CCB but in many cases the ref.
> > > clock is not 333 MHz and a divisor needs to be configured.
> > > 
> > > This adds PEX_CCB_DIV #define which can be defined for each
> > > type of CPU/platform.
> > > 
> > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > ---
> > >  drivers/pci/fsl_pci_init.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > > index 52792dcd59..4d00b3f26c 100644
> > > --- a/drivers/pci/fsl_pci_init.c
> > > +++ b/drivers/pci/fsl_pci_init.c
> > > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
> > > 
> > >      pci_setup_indirect(hose, cfg_addr, cfg_data);
> > > 
> > > +#ifdef PEX_CCB_DIV
> > > +    /* Configure the PCIE controller core clock ratio */
> > > +    pci_hose_write_config_dword(hose, dev, 0x440,
> > > +                                ((gd->bus_clk / 1000000) *
> > > +                                 (16 / PEX_CCB_DIV)) / 333);
> > > +#endif
> > >      block_rev = in_be32(&pci->block_rev1);
> > >      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
> > >              pi = &pci->pit[2];      /* 0xDC0 */
> > 
> > Ping?
> > 
> >  Jocke
> > 
> 
> I believe Mingkai's last comment was "to add the PCIe clock in gd".

Right, I seem to have forgotten to comment on that ...
Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
it makes perfect sense to just #define it.

 Jocke

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
> On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>
>>
>> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
>>> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>>>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>>>> This clock is derived from the CCB but in many cases the ref.
>>>> clock is not 333 MHz and a divisor needs to be configured.
>>>>
>>>> This adds PEX_CCB_DIV #define which can be defined for each
>>>> type of CPU/platform.
>>>>
>>>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>>>> ---
>>>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>>> index 52792dcd59..4d00b3f26c 100644
>>>> --- a/drivers/pci/fsl_pci_init.c
>>>> +++ b/drivers/pci/fsl_pci_init.c
>>>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>>>
>>>>      pci_setup_indirect(hose, cfg_addr, cfg_data);
>>>>
>>>> +#ifdef PEX_CCB_DIV
>>>> +    /* Configure the PCIE controller core clock ratio */
>>>> +    pci_hose_write_config_dword(hose, dev, 0x440,
>>>> +                                ((gd->bus_clk / 1000000) *
>>>> +                                 (16 / PEX_CCB_DIV)) / 333);
>>>> +#endif
>>>>      block_rev = in_be32(&pci->block_rev1);
>>>>      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>>>>              pi = &pci->pit[2];      /* 0xDC0 */
>>>
>>> Ping?
>>>
>>>  Jocke
>>>
>>
>> I believe Mingkai's last comment was "to add the PCIe clock in gd".
> 
> Right, I seem to have forgotten to comment on that ...
> Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
> it makes perfect sense to just #define it.
> 

I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
Would it be better if you use a Kconfig option and select the divisor by
SoC?

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
> > On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > 
> > > 
> > > On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> > > > On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> > > > > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > > > > This clock is derived from the CCB but in many cases the ref.
> > > > > clock is not 333 MHz and a divisor needs to be configured.
> > > > > 
> > > > > This adds PEX_CCB_DIV #define which can be defined for each
> > > > > type of CPU/platform.
> > > > > 
> > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > > ---
> > > > >  drivers/pci/fsl_pci_init.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > > > > index 52792dcd59..4d00b3f26c 100644
> > > > > --- a/drivers/pci/fsl_pci_init.c
> > > > > +++ b/drivers/pci/fsl_pci_init.c
> > > > > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
> > > > > 
> > > > >      pci_setup_indirect(hose, cfg_addr, cfg_data);
> > > > > 
> > > > > +#ifdef PEX_CCB_DIV
> > > > > +    /* Configure the PCIE controller core clock ratio */
> > > > > +    pci_hose_write_config_dword(hose, dev, 0x440,
> > > > > +                                ((gd->bus_clk / 1000000) *
> > > > > +                                 (16 / PEX_CCB_DIV)) / 333);
> > > > > +#endif
> > > > >      block_rev = in_be32(&pci->block_rev1);
> > > > >      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
> > > > >              pi = &pci->pit[2];      /* 0xDC0 */
> > > > 
> > > > Ping?
> > > > 
> > > >  Jocke
> > > > 
> > > 
> > > I believe Mingkai's last comment was "to add the PCIe clock in gd".
> > 
> > Right, I seem to have forgotten to comment on that ...
> > Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
> > it makes perfect sense to just #define it.
> > 
> 
> I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
> Would it be better if you use a Kconfig option and select the divisor by
> SoC?

No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
in relevant PPC CPU, like in config_mpc85xx.h 

 Jocke

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
> On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>
>>
>> On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
>>> On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>
>>>>
>>>> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
>>>>> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>>>>>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>>>>>> This clock is derived from the CCB but in many cases the ref.
>>>>>> clock is not 333 MHz and a divisor needs to be configured.
>>>>>>
>>>>>> This adds PEX_CCB_DIV #define which can be defined for each
>>>>>> type of CPU/platform.
>>>>>>
>>>>>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>>>>>> ---
>>>>>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>>>>>  1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>>>>> index 52792dcd59..4d00b3f26c 100644
>>>>>> --- a/drivers/pci/fsl_pci_init.c
>>>>>> +++ b/drivers/pci/fsl_pci_init.c
>>>>>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>>>>>
>>>>>>      pci_setup_indirect(hose, cfg_addr, cfg_data);
>>>>>>
>>>>>> +#ifdef PEX_CCB_DIV
>>>>>> +    /* Configure the PCIE controller core clock ratio */
>>>>>> +    pci_hose_write_config_dword(hose, dev, 0x440,
>>>>>> +                                ((gd->bus_clk / 1000000) *
>>>>>> +                                 (16 / PEX_CCB_DIV)) / 333);
>>>>>> +#endif
>>>>>>      block_rev = in_be32(&pci->block_rev1);
>>>>>>      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>>>>>>              pi = &pci->pit[2];      /* 0xDC0 */
>>>>>
>>>>> Ping?
>>>>>
>>>>>  Jocke
>>>>>
>>>>
>>>> I believe Mingkai's last comment was "to add the PCIe clock in gd".
>>>
>>> Right, I seem to have forgotten to comment on that ...
>>> Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
>>> it makes perfect sense to just #define it.
>>>
>>
>> I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
>> Would it be better if you use a Kconfig option and select the divisor by
>> SoC?
> 
> No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
> in relevant PPC CPU, like in config_mpc85xx.h 
> 

So what should be in Kconfig, and what shouldn't? This is per SoC macro.
Sounds like CONFIG_SYS_ in old days.

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
> 
> On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
> > On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
> > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > 
> > > 
> > > On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
> > > > On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
> > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > 
> > > > > 
> > > > > On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> > > > > > On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> > > > > > > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > > > > > > This clock is derived from the CCB but in many cases the ref.
> > > > > > > clock is not 333 MHz and a divisor needs to be configured.
> > > > > > > 
> > > > > > > This adds PEX_CCB_DIV #define which can be defined for each
> > > > > > > type of CPU/platform.
> > > > > > > 
> > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > > > > ---
> > > > > > >  drivers/pci/fsl_pci_init.c | 6 ++++++
> > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > > > > > > index 52792dcd59..4d00b3f26c 100644
> > > > > > > --- a/drivers/pci/fsl_pci_init.c
> > > > > > > +++ b/drivers/pci/fsl_pci_init.c
> > > > > > > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
> > > > > > > 
> > > > > > >      pci_setup_indirect(hose, cfg_addr, cfg_data);
> > > > > > > 
> > > > > > > +#ifdef PEX_CCB_DIV
> > > > > > > +    /* Configure the PCIE controller core clock ratio */
> > > > > > > +    pci_hose_write_config_dword(hose, dev, 0x440,
> > > > > > > +                                ((gd->bus_clk / 1000000) *
> > > > > > > +                                 (16 / PEX_CCB_DIV)) / 333);
> > > > > > > +#endif
> > > > > > >      block_rev = in_be32(&pci->block_rev1);
> > > > > > >      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
> > > > > > >              pi = &pci->pit[2];      /* 0xDC0 */
> > > > > > 
> > > > > > Ping?
> > > > > > 
> > > > > >  Jocke
> > > > > > 
> > > > > 
> > > > > I believe Mingkai's last comment was "to add the PCIe clock in gd".
> > > > 
> > > > Right, I seem to have forgotten to comment on that ...
> > > > Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
> > > > it makes perfect sense to just #define it.
> > > > 
> > > 
> > > I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
> > > Would it be better if you use a Kconfig option and select the divisor by
> > > SoC?
> > 
> > No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
> > in relevant PPC CPU, like in config_mpc85xx.h
> > 
> 
> So what should be in Kconfig, and what shouldn't? This is per SoC macro.
> Sounds like CONFIG_SYS_ in old days.

I must confess, I am not up to date with Kconfig stuff. I based my suggestion on
what already exists in config_mpc85xx.h:
#elif defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042) ||\
defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
#define CONFIG_E5500
#define CONFIG_FSL_CORENET		/* Freescale CoreNet platform */
#define CONFIG_SYS_FSL_QORIQ_CHASSIS2	/* Freescale Chassis generation 2 */
#define CONFIG_SYS_FSL_CORES_PER_CLUSTER 1
#define CONFIG_SYS_FSL_QMAN_V3		/* QMAN version 3 */
#ifdef CONFIG_SYS_FSL_DDR4
#define CONFIG_SYS_FSL_DDRC_GEN4
#endif
#if defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042)
#define CONFIG_MAX_CPUS			4
#elif defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
#define CONFIG_MAX_CPUS			2
#endif
#define CONFIG_SYS_FSL_NUM_CC_PLLS	2
#define CONFIG_SYS_FSL_CLUSTER_CLOCKS   { 1, 1, 1, 1 }
#define CONFIG_SYS_FSL_NUM_LAWS		16
#define CONFIG_SYS_FSL_SRDS_1
#define CONFIG_SYS_FSL_SEC_COMPAT	5
#define CONFIG_SYS_NUM_FMAN		1
#define CONFIG_SYS_NUM_FM1_DTSEC	5
....
etc.

Seems like a good place to put another CPU constant.

Anyhow, that patch stands of its own I think. Where to put all constants 
is another patch for NXP.

 Jocke

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
> On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
>>
>> On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
>>> On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>
>>>>
>>>> On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
>>>>> On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>
>>>>>>
>>>>>> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
>>>>>>> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>>>>>>>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>>>>>>>> This clock is derived from the CCB but in many cases the ref.
>>>>>>>> clock is not 333 MHz and a divisor needs to be configured.
>>>>>>>>
>>>>>>>> This adds PEX_CCB_DIV #define which can be defined for each
>>>>>>>> type of CPU/platform.
>>>>>>>>
>>>>>>>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>>>>>>>> ---
>>>>>>>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>>>>>>> index 52792dcd59..4d00b3f26c 100644
>>>>>>>> --- a/drivers/pci/fsl_pci_init.c
>>>>>>>> +++ b/drivers/pci/fsl_pci_init.c
>>>>>>>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>>>>>>>
>>>>>>>>      pci_setup_indirect(hose, cfg_addr, cfg_data);
>>>>>>>>
>>>>>>>> +#ifdef PEX_CCB_DIV
>>>>>>>> +    /* Configure the PCIE controller core clock ratio */
>>>>>>>> +    pci_hose_write_config_dword(hose, dev, 0x440,
>>>>>>>> +                                ((gd->bus_clk / 1000000) *
>>>>>>>> +                                 (16 / PEX_CCB_DIV)) / 333);
>>>>>>>> +#endif



I took another look at this patch. Would it be appropriate to alway
write to this register with correct clock?



>>>>>>>>      block_rev = in_be32(&pci->block_rev1);
>>>>>>>>      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>>>>>>>>              pi = &pci->pit[2];      /* 0xDC0 */
>>>>>>>
>>>>>>> Ping?
>>>>>>>
>>>>>>>  Jocke
>>>>>>>
>>>>>>
>>>>>> I believe Mingkai's last comment was "to add the PCIe clock in gd".
>>>>>
>>>>> Right, I seem to have forgotten to comment on that ...
>>>>> Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
>>>>> it makes perfect sense to just #define it.
>>>>>
>>>>
>>>> I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
>>>> Would it be better if you use a Kconfig option and select the divisor by
>>>> SoC?
>>>
>>> No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
>>> in relevant PPC CPU, like in config_mpc85xx.h
>>>
>>
>> So what should be in Kconfig, and what shouldn't? This is per SoC macro.
>> Sounds like CONFIG_SYS_ in old days.
> 
> I must confess, I am not up to date with Kconfig stuff. I based my suggestion on
> what already exists in config_mpc85xx.h:
> #elif defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042) ||\
> defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> #define CONFIG_E5500
> #define CONFIG_FSL_CORENET		/* Freescale CoreNet platform */
> #define CONFIG_SYS_FSL_QORIQ_CHASSIS2	/* Freescale Chassis generation 2 */
> #define CONFIG_SYS_FSL_CORES_PER_CLUSTER 1
> #define CONFIG_SYS_FSL_QMAN_V3		/* QMAN version 3 */
> #ifdef CONFIG_SYS_FSL_DDR4
> #define CONFIG_SYS_FSL_DDRC_GEN4
> #endif
> #if defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042)
> #define CONFIG_MAX_CPUS			4
> #elif defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> #define CONFIG_MAX_CPUS			2
> #endif
> #define CONFIG_SYS_FSL_NUM_CC_PLLS	2
> #define CONFIG_SYS_FSL_CLUSTER_CLOCKS   { 1, 1, 1, 1 }
> #define CONFIG_SYS_FSL_NUM_LAWS		16
> #define CONFIG_SYS_FSL_SRDS_1
> #define CONFIG_SYS_FSL_SEC_COMPAT	5
> #define CONFIG_SYS_NUM_FMAN		1
> #define CONFIG_SYS_NUM_FM1_DTSEC	5
> ....
> etc.
> 
> Seems like a good place to put another CPU constant.
> 

Yeah. We have been moving things out of header files and into Kconfig. I
would avoid adding new macro to header files.

> Anyhow, that patch stands of its own I think. Where to put all constants 
> is another patch for NXP.

Yes, we can add another patch to define this option for SoCs.

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
> 
> 
> On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
> > On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
> > > 
> > > On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
> > > > On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
> > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > 
> > > > > 
> > > > > On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
> > > > > > On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
> > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > > > 
> > > > > > > 
> > > > > > > On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> > > > > > > > On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> > > > > > > > > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > > > > > > > > This clock is derived from the CCB but in many cases the ref.
> > > > > > > > > clock is not 333 MHz and a divisor needs to be configured.
> > > > > > > > > 
> > > > > > > > > This adds PEX_CCB_DIV #define which can be defined for each
> > > > > > > > > type of CPU/platform.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/pci/fsl_pci_init.c | 6 ++++++
> > > > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > > > > > > > > index 52792dcd59..4d00b3f26c 100644
> > > > > > > > > --- a/drivers/pci/fsl_pci_init.c
> > > > > > > > > +++ b/drivers/pci/fsl_pci_init.c
> > > > > > > > > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
> > > > > > > > > 
> > > > > > > > >      pci_setup_indirect(hose, cfg_addr, cfg_data);
> > > > > > > > > 
> > > > > > > > > +#ifdef PEX_CCB_DIV
> > > > > > > > > +    /* Configure the PCIE controller core clock ratio */
> > > > > > > > > +    pci_hose_write_config_dword(hose, dev, 0x440,
> > > > > > > > > +                                ((gd->bus_clk / 1000000) *
> > > > > > > > > +                                 (16 / PEX_CCB_DIV)) / 333);
> > > > > > > > > +#endif
> 
> 
> 
> I took another look at this patch. Would it be appropriate to alway
> write to this register with correct clock?

I sure hope so, the docs I have only mentions this reg having a default value which
needs to be changed in most cases I guess.

> 
> 
> 
> > > > > > > > >      block_rev = in_be32(&pci->block_rev1);
> > > > > > > > >      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
> > > > > > > > >              pi = &pci->pit[2];      /* 0xDC0 */
> > > > > > > > 
> > > > > > > > Ping?
> > > > > > > > 
> > > > > > > >  Jocke
> > > > > > > > 
> > > > > > > 
> > > > > > > I believe Mingkai's last comment was "to add the PCIe clock in gd".
> > > > > > 
> > > > > > Right, I seem to have forgotten to comment on that ...
> > > > > > Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
> > > > > > it makes perfect sense to just #define it.
> > > > > > 
> > > > > 
> > > > > I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
> > > > > Would it be better if you use a Kconfig option and select the divisor by
> > > > > SoC?
> > > > 
> > > > No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
> > > > in relevant PPC CPU, like in config_mpc85xx.h
> > > > 
> > > 
> > > So what should be in Kconfig, and what shouldn't? This is per SoC macro.
> > > Sounds like CONFIG_SYS_ in old days.
> > 
> > I must confess, I am not up to date with Kconfig stuff. I based my suggestion on
> > what already exists in config_mpc85xx.h:
> > #elif defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042) ||\
> > defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> > #define CONFIG_E5500
> > #define CONFIG_FSL_CORENET            /* Freescale CoreNet platform */
> > #define CONFIG_SYS_FSL_QORIQ_CHASSIS2 /* Freescale Chassis generation 2 */
> > #define CONFIG_SYS_FSL_CORES_PER_CLUSTER 1
> > #define CONFIG_SYS_FSL_QMAN_V3                /* QMAN version 3 */
> > #ifdef CONFIG_SYS_FSL_DDR4
> > #define CONFIG_SYS_FSL_DDRC_GEN4
> > #endif
> > #if defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042)
> > #define CONFIG_MAX_CPUS                       4
> > #elif defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> > #define CONFIG_MAX_CPUS                       2
> > #endif
> > #define CONFIG_SYS_FSL_NUM_CC_PLLS    2
> > #define CONFIG_SYS_FSL_CLUSTER_CLOCKS   { 1, 1, 1, 1 }
> > #define CONFIG_SYS_FSL_NUM_LAWS               16
> > #define CONFIG_SYS_FSL_SRDS_1
> > #define CONFIG_SYS_FSL_SEC_COMPAT     5
> > #define CONFIG_SYS_NUM_FMAN           1
> > #define CONFIG_SYS_NUM_FM1_DTSEC      5
> > ....
> > etc.
> > 
> > Seems like a good place to put another CPU constant.
> > 
> 
> Yeah. We have been moving things out of header files and into Kconfig. I
> would avoid adding new macro to header files.

OK, I am happy if don't have to manually select the constant.

> 
> > Anyhow, that patch stands of its own I think. Where to put all constants
> > is another patch for NXP.
> 
> Yes, we can add another patch to define this option for SoCs.

 :)

 Jocke

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 11/21/2017 10:20 AM, Joakim Tjernlund wrote:
> On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
>>
>>
>> On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
>>> On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
>>>>
>>>> On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
>>>>> On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>
>>>>>>
>>>>>> On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
>>>>>>> On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
>>>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
>>>>>>>>> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>>>>>>>>>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>>>>>>>>>> This clock is derived from the CCB but in many cases the ref.
>>>>>>>>>> clock is not 333 MHz and a divisor needs to be configured.
>>>>>>>>>>
>>>>>>>>>> This adds PEX_CCB_DIV #define which can be defined for each
>>>>>>>>>> type of CPU/platform.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>>>>>>>>> index 52792dcd59..4d00b3f26c 100644
>>>>>>>>>> --- a/drivers/pci/fsl_pci_init.c
>>>>>>>>>> +++ b/drivers/pci/fsl_pci_init.c
>>>>>>>>>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>>>>>>>>>
>>>>>>>>>>      pci_setup_indirect(hose, cfg_addr, cfg_data);
>>>>>>>>>>
>>>>>>>>>> +#ifdef PEX_CCB_DIV
>>>>>>>>>> +    /* Configure the PCIE controller core clock ratio */
>>>>>>>>>> +    pci_hose_write_config_dword(hose, dev, 0x440,
>>>>>>>>>> +                                ((gd->bus_clk / 1000000) *
>>>>>>>>>> +                                 (16 / PEX_CCB_DIV)) / 333);
>>>>>>>>>> +#endif
>>
>>
>>
>> I took another look at this patch. Would it be appropriate to alway
>> write to this register with correct clock?
> 
> I sure hope so, the docs I have only mentions this reg having a default value which
> needs to be changed in most cases I guess.


The only case we don't need to set this register is when the actual
clock is 333MHz. I wonder why we didn't have a problem so far. Maybe we
just didn't realize it.

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

On Tue, 2017-11-21 at 18:35 +0000, York Sun wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> 
> 
> On 11/21/2017 10:20 AM, Joakim Tjernlund wrote:
> > On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
> > > 
> > > 
> > > On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
> > > > On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
> > > > > 
> > > > > On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
> > > > > > On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
> > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > > > 
> > > > > > > 
> > > > > > > On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
> > > > > > > > On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
> > > > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> > > > > > > > > > On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> > > > > > > > > > > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > > > > > > > > > > This clock is derived from the CCB but in many cases the ref.
> > > > > > > > > > > clock is not 333 MHz and a divisor needs to be configured.
> > > > > > > > > > > 
> > > > > > > > > > > This adds PEX_CCB_DIV #define which can be defined for each
> > > > > > > > > > > type of CPU/platform.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/pci/fsl_pci_init.c | 6 ++++++
> > > > > > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > index 52792dcd59..4d00b3f26c 100644
> > > > > > > > > > > --- a/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > +++ b/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
> > > > > > > > > > > 
> > > > > > > > > > >      pci_setup_indirect(hose, cfg_addr, cfg_data);
> > > > > > > > > > > 
> > > > > > > > > > > +#ifdef PEX_CCB_DIV
> > > > > > > > > > > +    /* Configure the PCIE controller core clock ratio */
> > > > > > > > > > > +    pci_hose_write_config_dword(hose, dev, 0x440,
> > > > > > > > > > > +                                ((gd->bus_clk / 1000000) *
> > > > > > > > > > > +                                 (16 / PEX_CCB_DIV)) / 333);
> > > > > > > > > > > +#endif
> > > 
> > > 
> > > 
> > > I took another look at this patch. Would it be appropriate to alway
> > > write to this register with correct clock?
> > 
> > I sure hope so, the docs I have only mentions this reg having a default value which
> > needs to be changed in most cases I guess.
> 
> 
> The only case we don't need to set this register is when the actual
> clock is 333MHz. I wonder why we didn't have a problem so far. Maybe we
> just didn't realize it.

Have you tried higher PCI speeds? I remember a board we had didn't work with higher
speeds so we settled for less than max. Don't hav the board handy now.

 Jocke

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 11/21/2017 10:40 AM, Joakim Tjernlund wrote:
> On Tue, 2017-11-21 at 18:35 +0000, York Sun wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>
>>
>> On 11/21/2017 10:20 AM, Joakim Tjernlund wrote:
>>> On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
>>>>
>>>>
>>>> On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
>>>>> On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
>>>>>>
>>>>>> On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
>>>>>>> On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
>>>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
>>>>>>>>> On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
>>>>>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
>>>>>>>>>>> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>>>>>>>>>>>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>>>>>>>>>>>> This clock is derived from the CCB but in many cases the ref.
>>>>>>>>>>>> clock is not 333 MHz and a divisor needs to be configured.
>>>>>>>>>>>>
>>>>>>>>>>>> This adds PEX_CCB_DIV #define which can be defined for each
>>>>>>>>>>>> type of CPU/platform.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>>>>>>>>>>> index 52792dcd59..4d00b3f26c 100644
>>>>>>>>>>>> --- a/drivers/pci/fsl_pci_init.c
>>>>>>>>>>>> +++ b/drivers/pci/fsl_pci_init.c
>>>>>>>>>>>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>>>>>>>>>>>
>>>>>>>>>>>>      pci_setup_indirect(hose, cfg_addr, cfg_data);
>>>>>>>>>>>>
>>>>>>>>>>>> +#ifdef PEX_CCB_DIV
>>>>>>>>>>>> +    /* Configure the PCIE controller core clock ratio */
>>>>>>>>>>>> +    pci_hose_write_config_dword(hose, dev, 0x440,
>>>>>>>>>>>> +                                ((gd->bus_clk / 1000000) *
>>>>>>>>>>>> +                                 (16 / PEX_CCB_DIV)) / 333);
>>>>>>>>>>>> +#endif
>>>>
>>>>
>>>>
>>>> I took another look at this patch. Would it be appropriate to alway
>>>> write to this register with correct clock?
>>>
>>> I sure hope so, the docs I have only mentions this reg having a default value which
>>> needs to be changed in most cases I guess.
>>
>>
>> The only case we don't need to set this register is when the actual
>> clock is 333MHz. I wonder why we didn't have a problem so far. Maybe we
>> just didn't realize it.
> 
> Have you tried higher PCI speeds? I remember a board we had didn't work with higher
> speeds so we settled for less than max. Don't hav the board handy now.
> 

No, I haven't.

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 11/21/2017 10:20 AM, Joakim Tjernlund wrote:
> On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
>>
>>
>> On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
>>> On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
>>>>
>>>> On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
>>>>> On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>
>>>>>>
>>>>>> On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
>>>>>>> On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
>>>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
>>>>>>>>> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>>>>>>>>>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>>>>>>>>>> This clock is derived from the CCB but in many cases the ref.
>>>>>>>>>> clock is not 333 MHz and a divisor needs to be configured.
>>>>>>>>>>
>>>>>>>>>> This adds PEX_CCB_DIV #define which can be defined for each
>>>>>>>>>> type of CPU/platform.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>>>>>>>>> index 52792dcd59..4d00b3f26c 100644
>>>>>>>>>> --- a/drivers/pci/fsl_pci_init.c
>>>>>>>>>> +++ b/drivers/pci/fsl_pci_init.c
>>>>>>>>>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>>>>>>>>>
>>>>>>>>>>      pci_setup_indirect(hose, cfg_addr, cfg_data);
>>>>>>>>>>
>>>>>>>>>> +#ifdef PEX_CCB_DIV
>>>>>>>>>> +    /* Configure the PCIE controller core clock ratio */
>>>>>>>>>> +    pci_hose_write_config_dword(hose, dev, 0x440,
>>>>>>>>>> +                                ((gd->bus_clk / 1000000) *
>>>>>>>>>> +                                 (16 / PEX_CCB_DIV)) / 333);
>>>>>>>>>> +#endif
>>
>>
>>
>> I took another look at this patch. Would it be appropriate to alway
>> write to this register with correct clock?
> 
> I sure hope so, the docs I have only mentions this reg having a default value which
> needs to be changed in most cases I guess.
> 
>>
>>
>>
>>>>>>>>>>      block_rev = in_be32(&pci->block_rev1);
>>>>>>>>>>      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>>>>>>>>>>              pi = &pci->pit[2];      /* 0xDC0 */
>>>>>>>>>
>>>>>>>>> Ping?
>>>>>>>>>
>>>>>>>>>  Jocke
>>>>>>>>>
>>>>>>>>
>>>>>>>> I believe Mingkai's last comment was "to add the PCIe clock in gd".
>>>>>>>
>>>>>>> Right, I seem to have forgotten to comment on that ...
>>>>>>> Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
>>>>>>> it makes perfect sense to just #define it.
>>>>>>>
>>>>>>
>>>>>> I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
>>>>>> Would it be better if you use a Kconfig option and select the divisor by
>>>>>> SoC?
>>>>>
>>>>> No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
>>>>> in relevant PPC CPU, like in config_mpc85xx.h
>>>>>
>>>>
>>>> So what should be in Kconfig, and what shouldn't? This is per SoC macro.
>>>> Sounds like CONFIG_SYS_ in old days.
>>>
>>> I must confess, I am not up to date with Kconfig stuff. I based my suggestion on
>>> what already exists in config_mpc85xx.h:
>>> #elif defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042) ||\
>>> defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
>>> #define CONFIG_E5500
>>> #define CONFIG_FSL_CORENET            /* Freescale CoreNet platform */
>>> #define CONFIG_SYS_FSL_QORIQ_CHASSIS2 /* Freescale Chassis generation 2 */
>>> #define CONFIG_SYS_FSL_CORES_PER_CLUSTER 1
>>> #define CONFIG_SYS_FSL_QMAN_V3                /* QMAN version 3 */
>>> #ifdef CONFIG_SYS_FSL_DDR4
>>> #define CONFIG_SYS_FSL_DDRC_GEN4
>>> #endif
>>> #if defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042)
>>> #define CONFIG_MAX_CPUS                       4
>>> #elif defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
>>> #define CONFIG_MAX_CPUS                       2
>>> #endif
>>> #define CONFIG_SYS_FSL_NUM_CC_PLLS    2
>>> #define CONFIG_SYS_FSL_CLUSTER_CLOCKS   { 1, 1, 1, 1 }
>>> #define CONFIG_SYS_FSL_NUM_LAWS               16
>>> #define CONFIG_SYS_FSL_SRDS_1
>>> #define CONFIG_SYS_FSL_SEC_COMPAT     5
>>> #define CONFIG_SYS_NUM_FMAN           1
>>> #define CONFIG_SYS_NUM_FM1_DTSEC      5
>>> ....
>>> etc.
>>>
>>> Seems like a good place to put another CPU constant.
>>>
>>
>> Yeah. We have been moving things out of header files and into Kconfig. I
>> would avoid adding new macro to header files.
> 
> OK, I am happy if don't have to manually select the constant.
> 
>>
>>> Anyhow, that patch stands of its own I think. Where to put all constants
>>> is another patch for NXP.
>>
>> Yes, we can add another patch to define this option for SoCs.
> 
>  :)
> 

Jocke,

Where are we on this patch? Do we have any patch to define PEX_CCB_DIV
to activate this new code?

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

On Tue, 2018-02-27 at 19:30 +0000, York Sun wrote:
> 
> On 11/21/2017 10:20 AM, Joakim Tjernlund wrote:
> > On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
> > > 
> > > 
> > > On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
> > > > On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
> > > > > 
> > > > > On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
> > > > > > On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
> > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > > > 
> > > > > > > 
> > > > > > > On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
> > > > > > > > On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
> > > > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> > > > > > > > > > On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> > > > > > > > > > > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > > > > > > > > > > This clock is derived from the CCB but in many cases the ref.
> > > > > > > > > > > clock is not 333 MHz and a divisor needs to be configured.
> > > > > > > > > > > 
> > > > > > > > > > > This adds PEX_CCB_DIV #define which can be defined for each
> > > > > > > > > > > type of CPU/platform.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/pci/fsl_pci_init.c | 6 ++++++
> > > > > > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > index 52792dcd59..4d00b3f26c 100644
> > > > > > > > > > > --- a/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > +++ b/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
> > > > > > > > > > > 
> > > > > > > > > > >      pci_setup_indirect(hose, cfg_addr, cfg_data);
> > > > > > > > > > > 
> > > > > > > > > > > +#ifdef PEX_CCB_DIV
> > > > > > > > > > > +    /* Configure the PCIE controller core clock ratio */
> > > > > > > > > > > +    pci_hose_write_config_dword(hose, dev, 0x440,
> > > > > > > > > > > +                                ((gd->bus_clk / 1000000) *
> > > > > > > > > > > +                                 (16 / PEX_CCB_DIV)) / 333);
> > > > > > > > > > > +#endif
> > > 
> > > 
> > > 
> > > I took another look at this patch. Would it be appropriate to alway
> > > write to this register with correct clock?
> > 
> > I sure hope so, the docs I have only mentions this reg having a default value which
> > needs to be changed in most cases I guess.
> > 
> > > 
> > > 
> > > 
> > > > > > > > > > >      block_rev = in_be32(&pci->block_rev1);
> > > > > > > > > > >      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
> > > > > > > > > > >              pi = &pci->pit[2];      /* 0xDC0 */
> > > > > > > > > > 
> > > > > > > > > > Ping?
> > > > > > > > > > 
> > > > > > > > > >  Jocke
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I believe Mingkai's last comment was "to add the PCIe clock in gd".
> > > > > > > > 
> > > > > > > > Right, I seem to have forgotten to comment on that ...
> > > > > > > > Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
> > > > > > > > it makes perfect sense to just #define it.
> > > > > > > > 
> > > > > > > 
> > > > > > > I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
> > > > > > > Would it be better if you use a Kconfig option and select the divisor by
> > > > > > > SoC?
> > > > > > 
> > > > > > No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
> > > > > > in relevant PPC CPU, like in config_mpc85xx.h
> > > > > > 
> > > > > 
> > > > > So what should be in Kconfig, and what shouldn't? This is per SoC macro.
> > > > > Sounds like CONFIG_SYS_ in old days.
> > > > 
> > > > I must confess, I am not up to date with Kconfig stuff. I based my suggestion on
> > > > what already exists in config_mpc85xx.h:
> > > > #elif defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042) ||\
> > > > defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> > > > #define CONFIG_E5500
> > > > #define CONFIG_FSL_CORENET            /* Freescale CoreNet platform */
> > > > #define CONFIG_SYS_FSL_QORIQ_CHASSIS2 /* Freescale Chassis generation 2 */
> > > > #define CONFIG_SYS_FSL_CORES_PER_CLUSTER 1
> > > > #define CONFIG_SYS_FSL_QMAN_V3                /* QMAN version 3 */
> > > > #ifdef CONFIG_SYS_FSL_DDR4
> > > > #define CONFIG_SYS_FSL_DDRC_GEN4
> > > > #endif
> > > > #if defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042)
> > > > #define CONFIG_MAX_CPUS                       4
> > > > #elif defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> > > > #define CONFIG_MAX_CPUS                       2
> > > > #endif
> > > > #define CONFIG_SYS_FSL_NUM_CC_PLLS    2
> > > > #define CONFIG_SYS_FSL_CLUSTER_CLOCKS   { 1, 1, 1, 1 }
> > > > #define CONFIG_SYS_FSL_NUM_LAWS               16
> > > > #define CONFIG_SYS_FSL_SRDS_1
> > > > #define CONFIG_SYS_FSL_SEC_COMPAT     5
> > > > #define CONFIG_SYS_NUM_FMAN           1
> > > > #define CONFIG_SYS_NUM_FM1_DTSEC      5
> > > > ....
> > > > etc.
> > > > 
> > > > Seems like a good place to put another CPU constant.
> > > > 
> > > 
> > > Yeah. We have been moving things out of header files and into Kconfig. I
> > > would avoid adding new macro to header files.
> > 
> > OK, I am happy if don't have to manually select the constant.
> > 
> > > 
> > > > Anyhow, that patch stands of its own I think. Where to put all constants
> > > > is another patch for NXP.
> > > 
> > > Yes, we can add another patch to define this option for SoCs.
> > 
> >  :)
> > 
> 
> Jocke,
> 
> Where are we on this patch? Do we have any patch to define PEX_CCB_DIV
> to activate this new code?

I think we are at me thinking we should add my simple patch:
+#ifdef PEX_CCB_DIV
+    /* Configure the PCIE controller core clock ratio */
+    pci_hose_write_config_dword(hose, dev, 0x440,>
+                                ((gd->bus_clk / 1000000) *
+                                 (16 / PEX_CCB_DIV)) / 333);
+#endif
and then NXP find a suitable place to add PEX_CCB_DIV #define for each CPU/SOC.
I am not working on such a patch anyhow.

 Jocke

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 02/27/2018 11:52 AM, Joakim Tjernlund wrote:
> On Tue, 2018-02-27 at 19:30 +0000, York Sun wrote:
>>
>> On 11/21/2017 10:20 AM, Joakim Tjernlund wrote:
>>> On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
>>>>
>>>>
>>>> On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
>>>>> On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
>>>>>>
>>>>>> On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
>>>>>>> On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
>>>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
>>>>>>>>> On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
>>>>>>>>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
>>>>>>>>>>> On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
>>>>>>>>>>>> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
>>>>>>>>>>>> This clock is derived from the CCB but in many cases the ref.
>>>>>>>>>>>> clock is not 333 MHz and a divisor needs to be configured.
>>>>>>>>>>>>
>>>>>>>>>>>> This adds PEX_CCB_DIV #define which can be defined for each
>>>>>>>>>>>> type of CPU/platform.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  drivers/pci/fsl_pci_init.c | 6 ++++++
>>>>>>>>>>>>  1 file changed, 6 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
>>>>>>>>>>>> index 52792dcd59..4d00b3f26c 100644
>>>>>>>>>>>> --- a/drivers/pci/fsl_pci_init.c
>>>>>>>>>>>> +++ b/drivers/pci/fsl_pci_init.c
>>>>>>>>>>>> @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
>>>>>>>>>>>>
>>>>>>>>>>>>      pci_setup_indirect(hose, cfg_addr, cfg_data);
>>>>>>>>>>>>
>>>>>>>>>>>> +#ifdef PEX_CCB_DIV
>>>>>>>>>>>> +    /* Configure the PCIE controller core clock ratio */
>>>>>>>>>>>> +    pci_hose_write_config_dword(hose, dev, 0x440,
>>>>>>>>>>>> +                                ((gd->bus_clk / 1000000) *
>>>>>>>>>>>> +                                 (16 / PEX_CCB_DIV)) / 333);
>>>>>>>>>>>> +#endif
>>>>
>>>>
>>>>
>>>> I took another look at this patch. Would it be appropriate to alway
>>>> write to this register with correct clock?
>>>
>>> I sure hope so, the docs I have only mentions this reg having a default value which
>>> needs to be changed in most cases I guess.
>>>
>>>>
>>>>
>>>>
>>>>>>>>>>>>      block_rev = in_be32(&pci->block_rev1);
>>>>>>>>>>>>      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
>>>>>>>>>>>>              pi = &pci->pit[2];      /* 0xDC0 */
>>>>>>>>>>>
>>>>>>>>>>> Ping?
>>>>>>>>>>>
>>>>>>>>>>>  Jocke
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I believe Mingkai's last comment was "to add the PCIe clock in gd".
>>>>>>>>>
>>>>>>>>> Right, I seem to have forgotten to comment on that ...
>>>>>>>>> Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
>>>>>>>>> it makes perfect sense to just #define it.
>>>>>>>>>
>>>>>>>>
>>>>>>>> I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
>>>>>>>> Would it be better if you use a Kconfig option and select the divisor by
>>>>>>>> SoC?
>>>>>>>
>>>>>>> No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
>>>>>>> in relevant PPC CPU, like in config_mpc85xx.h
>>>>>>>
>>>>>>
>>>>>> So what should be in Kconfig, and what shouldn't? This is per SoC macro.
>>>>>> Sounds like CONFIG_SYS_ in old days.
>>>>>
>>>>> I must confess, I am not up to date with Kconfig stuff. I based my suggestion on
>>>>> what already exists in config_mpc85xx.h:
>>>>> #elif defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042) ||\
>>>>> defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
>>>>> #define CONFIG_E5500
>>>>> #define CONFIG_FSL_CORENET            /* Freescale CoreNet platform */
>>>>> #define CONFIG_SYS_FSL_QORIQ_CHASSIS2 /* Freescale Chassis generation 2 */
>>>>> #define CONFIG_SYS_FSL_CORES_PER_CLUSTER 1
>>>>> #define CONFIG_SYS_FSL_QMAN_V3                /* QMAN version 3 */
>>>>> #ifdef CONFIG_SYS_FSL_DDR4
>>>>> #define CONFIG_SYS_FSL_DDRC_GEN4
>>>>> #endif
>>>>> #if defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042)
>>>>> #define CONFIG_MAX_CPUS                       4
>>>>> #elif defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
>>>>> #define CONFIG_MAX_CPUS                       2
>>>>> #endif
>>>>> #define CONFIG_SYS_FSL_NUM_CC_PLLS    2
>>>>> #define CONFIG_SYS_FSL_CLUSTER_CLOCKS   { 1, 1, 1, 1 }
>>>>> #define CONFIG_SYS_FSL_NUM_LAWS               16
>>>>> #define CONFIG_SYS_FSL_SRDS_1
>>>>> #define CONFIG_SYS_FSL_SEC_COMPAT     5
>>>>> #define CONFIG_SYS_NUM_FMAN           1
>>>>> #define CONFIG_SYS_NUM_FM1_DTSEC      5
>>>>> ....
>>>>> etc.
>>>>>
>>>>> Seems like a good place to put another CPU constant.
>>>>>
>>>>
>>>> Yeah. We have been moving things out of header files and into Kconfig. I
>>>> would avoid adding new macro to header files.
>>>
>>> OK, I am happy if don't have to manually select the constant.
>>>
>>>>
>>>>> Anyhow, that patch stands of its own I think. Where to put all constants
>>>>> is another patch for NXP.
>>>>
>>>> Yes, we can add another patch to define this option for SoCs.
>>>
>>>  :)
>>>
>>
>> Jocke,
>>
>> Where are we on this patch? Do we have any patch to define PEX_CCB_DIV
>> to activate this new code?
> 
> I think we are at me thinking we should add my simple patch:
> +#ifdef PEX_CCB_DIV
> +    /* Configure the PCIE controller core clock ratio */
> +    pci_hose_write_config_dword(hose, dev, 0x440,>
> +                                ((gd->bus_clk / 1000000) *
> +                                 (16 / PEX_CCB_DIV)) / 333);
> +#endif
> and then NXP find a suitable place to add PEX_CCB_DIV #define for each CPU/SOC.
> I am not working on such a patch anyhow.
> 

OK. I tend to accept this patch and have Mingkai to follow up on
defining PEX_CCB_DIV for SoCs in need.

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[Joakim Tjernlund]

York, did this go anywhere?

 Jocke

On Tue, 2018-02-27 at 19:54 +0000, York Sun wrote:
> 
> On 02/27/2018 11:52 AM, Joakim Tjernlund wrote:
> > On Tue, 2018-02-27 at 19:30 +0000, York Sun wrote:
> > > 
> > > On 11/21/2017 10:20 AM, Joakim Tjernlund wrote:
> > > > On Tue, 2017-11-21 at 18:04 +0000, York Sun wrote:
> > > > > 
> > > > > 
> > > > > On 11/21/2017 09:52 AM, Joakim Tjernlund wrote:
> > > > > > On Tue, 2017-11-21 at 17:45 +0000, York Sun wrote:
> > > > > > > 
> > > > > > > On 11/21/2017 09:41 AM, Joakim Tjernlund wrote:
> > > > > > > > On Tue, 2017-11-21 at 17:32 +0000, York Sun wrote:
> > > > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 11/21/2017 09:29 AM, Joakim Tjernlund wrote:
> > > > > > > > > > On Tue, 2017-11-21 at 17:23 +0000, York Sun wrote:
> > > > > > > > > > > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > On 11/21/2017 09:18 AM, Joakim Tjernlund wrote:
> > > > > > > > > > > > On Tue, 2017-09-12 at 19:56 +0200, Joakim Tjernlund wrote:
> > > > > > > > > > > > > Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> > > > > > > > > > > > > This clock is derived from the CCB but in many cases the ref.
> > > > > > > > > > > > > clock is not 333 MHz and a divisor needs to be configured.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > This adds PEX_CCB_DIV #define which can be defined for each
> > > > > > > > > > > > > type of CPU/platform.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > > > > > > > > > > ---
> > > > > > > > > > > > >  drivers/pci/fsl_pci_init.c | 6 ++++++
> > > > > > > > > > > > >  1 file changed, 6 insertions(+)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > diff --git a/drivers/pci/fsl_pci_init.c b/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > > > index 52792dcd59..4d00b3f26c 100644
> > > > > > > > > > > > > --- a/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > > > +++ b/drivers/pci/fsl_pci_init.c
> > > > > > > > > > > > > @@ -322,6 +322,12 @@ void fsl_pci_init(struct pci_controller *hose, struct fsl_pci_info *pci_info)
> > > > > > > > > > > > > 
> > > > > > > > > > > > >      pci_setup_indirect(hose, cfg_addr, cfg_data);
> > > > > > > > > > > > > 
> > > > > > > > > > > > > +#ifdef PEX_CCB_DIV
> > > > > > > > > > > > > +    /* Configure the PCIE controller core clock ratio */
> > > > > > > > > > > > > +    pci_hose_write_config_dword(hose, dev, 0x440,
> > > > > > > > > > > > > +                                ((gd->bus_clk / 1000000) *
> > > > > > > > > > > > > +                                 (16 / PEX_CCB_DIV)) / 333);
> > > > > > > > > > > > > +#endif
> > > > > 
> > > > > 
> > > > > 
> > > > > I took another look at this patch. Would it be appropriate to alway
> > > > > write to this register with correct clock?
> > > > 
> > > > I sure hope so, the docs I have only mentions this reg having a default value which
> > > > needs to be changed in most cases I guess.
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > > > > > > > > >      block_rev = in_be32(&pci->block_rev1);
> > > > > > > > > > > > >      if (PEX_IP_BLK_REV_2_2 <= block_rev) {
> > > > > > > > > > > > >              pi = &pci->pit[2];      /* 0xDC0 */
> > > > > > > > > > > > 
> > > > > > > > > > > > Ping?
> > > > > > > > > > > > 
> > > > > > > > > > > >  Jocke
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > I believe Mingkai's last comment was "to add the PCIe clock in gd".
> > > > > > > > > > 
> > > > > > > > > > Right, I seem to have forgotten to comment on that ...
> > > > > > > > > > Why add that complexity/bloat? This is a constant defined by the CPU/SOC so
> > > > > > > > > > it makes perfect sense to just #define it.
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I am leaning to agree with you. The clock is either CCB clock, or CCB/2.
> > > > > > > > > Would it be better if you use a Kconfig option and select the divisor by
> > > > > > > > > SoC?
> > > > > > > > 
> > > > > > > > No, this is not something that needs Kconfig, just add the PEX_CCB_DIV #define
> > > > > > > > in relevant PPC CPU, like in config_mpc85xx.h
> > > > > > > > 
> > > > > > > 
> > > > > > > So what should be in Kconfig, and what shouldn't? This is per SoC macro.
> > > > > > > Sounds like CONFIG_SYS_ in old days.
> > > > > > 
> > > > > > I must confess, I am not up to date with Kconfig stuff. I based my suggestion on
> > > > > > what already exists in config_mpc85xx.h:
> > > > > > #elif defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042) ||\
> > > > > > defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> > > > > > #define CONFIG_E5500
> > > > > > #define CONFIG_FSL_CORENET            /* Freescale CoreNet platform */
> > > > > > #define CONFIG_SYS_FSL_QORIQ_CHASSIS2 /* Freescale Chassis generation 2 */
> > > > > > #define CONFIG_SYS_FSL_CORES_PER_CLUSTER 1
> > > > > > #define CONFIG_SYS_FSL_QMAN_V3                /* QMAN version 3 */
> > > > > > #ifdef CONFIG_SYS_FSL_DDR4
> > > > > > #define CONFIG_SYS_FSL_DDRC_GEN4
> > > > > > #endif
> > > > > > #if defined(CONFIG_PPC_T1040) || defined(CONFIG_PPC_T1042)
> > > > > > #define CONFIG_MAX_CPUS                       4
> > > > > > #elif defined(CONFIG_PPC_T1020) || defined(CONFIG_PPC_T1022)
> > > > > > #define CONFIG_MAX_CPUS                       2
> > > > > > #endif
> > > > > > #define CONFIG_SYS_FSL_NUM_CC_PLLS    2
> > > > > > #define CONFIG_SYS_FSL_CLUSTER_CLOCKS   { 1, 1, 1, 1 }
> > > > > > #define CONFIG_SYS_FSL_NUM_LAWS               16
> > > > > > #define CONFIG_SYS_FSL_SRDS_1
> > > > > > #define CONFIG_SYS_FSL_SEC_COMPAT     5
> > > > > > #define CONFIG_SYS_NUM_FMAN           1
> > > > > > #define CONFIG_SYS_NUM_FM1_DTSEC      5
> > > > > > ....
> > > > > > etc.
> > > > > > 
> > > > > > Seems like a good place to put another CPU constant.
> > > > > > 
> > > > > 
> > > > > Yeah. We have been moving things out of header files and into Kconfig. I
> > > > > would avoid adding new macro to header files.
> > > > 
> > > > OK, I am happy if don't have to manually select the constant.
> > > > 
> > > > > 
> > > > > > Anyhow, that patch stands of its own I think. Where to put all constants
> > > > > > is another patch for NXP.
> > > > > 
> > > > > Yes, we can add another patch to define this option for SoCs.
> > > > 
> > > >  :)
> > > > 
> > > 
> > > Jocke,
> > > 
> > > Where are we on this patch? Do we have any patch to define PEX_CCB_DIV
> > > to activate this new code?
> > 
> > I think we are at me thinking we should add my simple patch:
> > +#ifdef PEX_CCB_DIV
> > +    /* Configure the PCIE controller core clock ratio */
> > +    pci_hose_write_config_dword(hose, dev, 0x440,>
> > +                                ((gd->bus_clk / 1000000) *
> > +                                 (16 / PEX_CCB_DIV)) / 333);
> > +#endif
> > and then NXP find a suitable place to add PEX_CCB_DIV #define for each CPU/SOC.
> > I am not working on such a patch anyhow.
> > 
> 
> OK. I tend to accept this patch and have Mingkai to follow up on
> defining PEX_CCB_DIV for SoCs in need.
> 
> York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 08/02/2018 03:38 PM, Joakim Tjernlund wrote:
> York, did this go anywhere?

No, I didn't hear from Mingkai. I am OK with this patch and will merge it.

York

[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio

[York Sun]

On 09/12/2017 10:56 AM, Joakim Tjernlund wrote:
> Most FSL PCIe controllers expects 333 MHz PCI reference clock.
> This clock is derived from the CCB but in many cases the ref.
> clock is not 333 MHz and a divisor needs to be configured.
> 
> This adds PEX_CCB_DIV #define which can be defined for each
> type of CPU/platform.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---

Applied to fsl-qoriq master, awaiting upstream.
Thanks.

York