[U-Boot] [PATCH] FSL PCI: Configure PCIe reference ratio
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 */
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
-----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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
participants (4)
-
Joakim Tjernlund -
Joakim Tjernlund
-
Mingkai Hu -
York Sun