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.tjernl...@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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
