From mboxrd@z Thu Jan 1 00:00:00 1970 From: York Sun Date: Tue, 21 Nov 2017 18:04:10 +0000 Subject: [U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio References: <20170912175641.23175-1-joakim.tjernlund@infinera.com> <1511284718.4775.139.camel@infinera.com> <1511285373.4775.143.camel@infinera.com> <1511286092.4775.148.camel@infinera.com> <1511286718.4775.153.camel@infinera.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>>>> --- >>>>>>>> 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