[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 at 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