Hi Pragnesh, On Sat, Apr 25, 2020 at 12:27 AM Pragnesh Patel <pragnesh.pa...@sifive.com> wrote: > > Hi Bin, > > >-----Original Message----- > >From: Bin Meng <bmeng...@gmail.com> > >Sent: 24 April 2020 19:25 > >To: Pragnesh Patel <pragnesh.pa...@sifive.com> > >Cc: Jagan Teki <ja...@amarulasolutions.com>; U-Boot-Denx <u- > >b...@lists.denx.de>; Atish Patra <atish.pa...@wdc.com>; Palmer Dabbelt > ><palmerdabb...@google.com>; Paul Walmsley <paul.walms...@sifive.com>; > >Troy Benjegerdes <troy.benjeger...@sifive.com>; Anup Patel > ><anup.pa...@wdc.com>; Sagar Kadam <sagar.ka...@sifive.com>; Rick Chen > ><r...@andestech.com>; Lukasz Majewski <lu...@denx.de>; Simon Glass > ><s...@chromium.org> > >Subject: Re: [PATCH v6 09/17] clk: sifive: fu540-prci: Add clock > >initialization for > >SPL > > > >[External Email] Do not click links or attachments unless you recognize the > >sender and know the content is safe > > > >Hi Pragnesh, > > > >On Fri, Apr 24, 2020 at 9:34 PM Pragnesh Patel <pragnesh.pa...@sifive.com> > >wrote: > >> > >> > >> > >> Hi Bin, > >> > >> >-----Original Message----- > >> >From: Bin Meng <bmeng...@gmail.com> > >> >Sent: 24 April 2020 18:31 > >> >To: Pragnesh Patel <pragnesh.pa...@sifive.com> > >> >Cc: Jagan Teki <ja...@amarulasolutions.com>; U-Boot-Denx <u- > >> >b...@lists.denx.de>; Atish Patra <atish.pa...@wdc.com>; Palmer > >> >Dabbelt <palmerdabb...@google.com>; Paul Walmsley > >> ><paul.walms...@sifive.com>; Troy Benjegerdes > >> ><troy.benjeger...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; Sagar > >> >Kadam <sagar.ka...@sifive.com>; Rick Chen <r...@andestech.com>; > >> >Lukasz Majewski <lu...@denx.de>; Simon Glass <s...@chromium.org> > >> >Subject: Re: [PATCH v6 09/17] clk: sifive: fu540-prci: Add clock > >> >initialization for SPL > >> > > >> >[External Email] Do not click links or attachments unless you > >> >recognize the sender and know the content is safe > >> > > >> >Hi Pragnesh, > >> > > >> >On Fri, Apr 24, 2020 at 6:08 PM Pragnesh Patel > >> ><pragnesh.pa...@sifive.com> > >> >wrote: > >> >> > >> >> Hi Bin, Jagan > >> >> > >> >> >-----Original Message----- > >> >> >From: Bin Meng <bmeng...@gmail.com> > >> >> >Sent: 20 April 2020 14:30 > >> >> >To: Jagan Teki <ja...@amarulasolutions.com> > >> >> >Cc: Pragnesh Patel <pragnesh.pa...@sifive.com>; U-Boot-Denx <u- > >> >> >b...@lists.denx.de>; Atish Patra <atish.pa...@wdc.com>; Palmer > >> >> >Dabbelt <palmerdabb...@google.com>; Paul Walmsley > >> >> ><paul.walms...@sifive.com>; Troy Benjegerdes > >> >> ><troy.benjeger...@sifive.com>; Anup Patel <anup.pa...@wdc.com>; > >> >> >Sagar Kadam <sagar.ka...@sifive.com>; Rick Chen > >> >> ><r...@andestech.com>; Lukasz Majewski <lu...@denx.de>; Simon > >Glass > >> >> ><s...@chromium.org> > >> >> >Subject: Re: [PATCH v6 09/17] clk: sifive: fu540-prci: Add clock > >> >> >initialization for SPL > >> >> > > >> >> >[External Email] Do not click links or attachments unless you > >> >> >recognize the sender and know the content is safe > >> >> > > >> >> >Hi Jagan, Pragnesh, > >> >> > > >> >> >On Tue, Apr 7, 2020 at 3:35 AM Jagan Teki > >> >> ><ja...@amarulasolutions.com> > >> >> >wrote: > >> >> >> > >> >> >> On Sun, Mar 29, 2020 at 10:37 PM Pragnesh Patel > >> >> >> <pragnesh.pa...@sifive.com> wrote: > >> >> >> > > >> >> >> > Set corepll, ddrpll and ethernet PLL for u-boot-spl > >> >> >> > > >> >> >> > Signed-off-by: Pragnesh Patel <pragnesh.pa...@sifive.com> > >> >> >> > --- > >> >> >> > drivers/clk/sifive/fu540-prci.c | 118 > >> >> >> > ++++++++++++++++++++++++++++++++ > >> >> >> > 1 file changed, 118 insertions(+) > >> >> >> > > >> >> >> > diff --git a/drivers/clk/sifive/fu540-prci.c > >> >> >> > b/drivers/clk/sifive/fu540-prci.c index e6214cd821..3a73c2c8d1 > >> >> >> > 100644 > >> >> >> > --- a/drivers/clk/sifive/fu540-prci.c > >> >> >> > +++ b/drivers/clk/sifive/fu540-prci.c > >> >> >> > @@ -41,6 +41,10 @@ > >> >> >> > #include <linux/clk/analogbits-wrpll-cln28hpc.h> > >> >> >> > #include <dt-bindings/clock/sifive-fu540-prci.h> > >> >> >> > > >> >> >> > +#define DDRCTLPLL_F 55 > >> >> >> > +#define DDRCTLPLL_Q 2 > >> >> >> > +#define MHz 1000000 > >> >> >> > + > >> >> >> > /* > >> >> >> > * EXPECTED_CLK_PARENT_COUNT: how many parent clocks this > >> >> >> > driver > >> >> >expects: > >> >> >> > * hfclk and rtcclk > >> >> >> > @@ -152,6 +156,27 @@ > >> >> >> > #define PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK \ > >> >> >> > (0x1 << > >> >> >> > PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_SHIFT) > >> >> >> > > >> >> >> > +/* PROCMONCFG */ > >> >> >> > +#define PRCI_PROCMONCFG_OFFSET 0xF0 > >> >> >> > +#define PRCI_PROCMONCFG_CORE_CLOCK_SHIFT 24 > >> >> >> > +#define PRCI_PROCMONCFG_CORE_CLOCK_MASK \ > >> >> >> > + (0x1 << > >> >> >> > +PRCI_PROCMONCFG_CORE_CLOCK_SHIFT) > >> >> >> > + > >> >> >> > +#define PLL_R(x) \ > >> >> >> > + ((x) << PRCI_DDRPLLCFG0_DIVR_SHIFT) & > >> >> >> > +PRCI_DDRPLLCFG0_DIVR_MASK #define PLL_F(x) \ > >> >> >> > + ((x) << PRCI_DDRPLLCFG0_DIVF_SHIFT) & > >> >> >> > +PRCI_DDRPLLCFG0_DIVF_MASK #define PLL_Q(x) \ > >> >> >> > + ((x) << PRCI_DDRPLLCFG0_DIVQ_SHIFT) & > >> >> >> > +PRCI_DDRPLLCFG0_DIVQ_MASK #define PLL_RANGE(x) \ > >> >> >> > + ((x) << PRCI_DDRPLLCFG0_RANGE_SHIFT) & > >> >> >> > +PRCI_DDRPLLCFG0_RANGE_MASK #define PLL_BYPASS(x) \ > >> >> >> > + ((x) << PRCI_DDRPLLCFG0_BYPASS_SHIFT) & > >> >> >> > +PRCI_DDRPLLCFG0_BYPASS_MASK #define PLL_FSE(x) \ > >> >> >> > + ((x) << PRCI_DDRPLLCFG0_FSE_SHIFT) & > >> >> >> > +PRCI_DDRPLLCFG0_FSE_MASK #define PLL_LOCK(x) \ > >> >> >> > + ((x) << PRCI_DDRPLLCFG0_LOCK_SHIFT) & > >> >> >> > +PRCI_DDRPLLCFG0_LOCK_MASK > >> >> >> > + > >> >> >> > /* > >> >> >> > * Private structures > >> >> >> > */ > >> >> >> > @@ -654,6 +679,93 @@ static int > >> >> >> > sifive_fu540_prci_disable(struct clk > >> >*clk) > >> >> >> > return pc->ops->enable_clk(pc, 0); } > >> >> >> > > >> >> >> > +#ifdef CONFIG_SPL_BUILD > >> >> >> > +static void corepll_init(struct udevice *dev) { > >> >> >> > + u32 v; > >> >> >> > + struct clk clock; > >> >> >> > + struct __prci_data *pd = dev_get_priv(dev); > >> >> >> > + > >> >> >> > + v = __prci_readl(pd, PRCI_CLKMUXSTATUSREG_OFFSET); > >> >> >> > + v &= PRCI_CLKMUXSTATUSREG_TLCLKSEL_STATUS_MASK; > >> >> >> > + > >> >> >> > + clock.id = PRCI_CLK_COREPLL; > >> >> >> > + > >> >> >> > + if (v) { > >> >> >> > + /* corepll 500 Mhz */ > >> >> >> > + sifive_fu540_prci_set_rate(&clock, 500UL * MHz); > >> >> >> > + } else { > >> >> >> > + /* corepll 1 Ghz */ > >> >> >> > + sifive_fu540_prci_set_rate(&clock, 1000UL * MHz); > >> >> >> > + } > >> >> >> > + > >> >> >> > + > >> >> >> > +sifive_fu540_prci_clock_enable(&__prci_init_clocks[clock.id], > >> >> >> > +1); } > >> >> >> > + > >> >> >> > +static void ddr_init(struct udevice *dev) { > >> >> >> > + u32 v; > >> >> >> > + struct clk clock; > >> >> >> > + struct __prci_data *pd = dev_get_priv(dev); > >> >> >> > + > >> >> >> > + //DDR init > >> >> >> > + u32 ddrctlmhz = > >> >> >> > + (PLL_R(0)) | > >> >> >> > + (PLL_F(DDRCTLPLL_F)) | > >> >> >> > + (PLL_Q(DDRCTLPLL_Q)) | > >> >> >> > + (PLL_RANGE(0x4)) | > >> >> >> > + (PLL_BYPASS(0)) | > >> >> >> > + (PLL_FSE(1)); > >> >> >> > + __prci_writel(ddrctlmhz, PRCI_DDRPLLCFG0_OFFSET, pd); > >> >> >> > + > >> >> >> > + clock.id = PRCI_CLK_DDRPLL; > >> >> >> > + > >> >> >> > + sifive_fu540_prci_clock_enable(&__prci_init_clocks[clock.id] > >> >> >> > + , > >> >> >> > + 1); > >> >> >> > + > >> >> >> > + /* Release DDR reset */ > >> >> >> > + v = __prci_readl(pd, PRCI_DEVICESRESETREG_OFFSET); > >> >> >> > + v |= PRCI_DEVICESRESETREG_DDR_CTRL_RST_N_MASK; > >> >> >> > + __prci_writel(v, PRCI_DEVICESRESETREG_OFFSET, pd); > >> >> >> > + > >> >> >> > + // HACK to get the '1 full controller clock cycle'. > >> >> >> > + asm volatile ("fence"); > >> >> >> > + v = __prci_readl(pd, PRCI_DEVICESRESETREG_OFFSET); > >> >> >> > + v |= (PRCI_DEVICESRESETREG_DDR_AXI_RST_N_MASK | > >> >> >> > + PRCI_DEVICESRESETREG_DDR_AHB_RST_N_MASK | > >> >> >> > + PRCI_DEVICESRESETREG_DDR_PHY_RST_N_MASK); > >> >> >> > + __prci_writel(v, PRCI_DEVICESRESETREG_OFFSET, pd); > >> >> >> > + // HACK to get the '1 full controller clock cycle'. > >> >> >> > + asm volatile ("fence"); > >> >> >> > + > >> >> >> > + /* These take like 16 cycles to actually propagate. We > >> >> >> > + can't go > >> >sending > >> >> >> > + * stuff before they come out of reset. So wait. > >> >> >> > + (TODO: Add a > >> >register > >> >> >> > + * to read the current reset states, or DDR Control > >> >> >> > device?) > >> >> >> > + */ > >> >> >> > + for (int i = 0; i < 256; i++) > >> >> >> > + asm volatile ("nop"); } > >> >> >> > + > >> >> >> > +static void ethernet_init(struct udevice *dev) { > >> >> >> > + u32 v; > >> >> >> > + struct clk clock; > >> >> >> > + struct __prci_data *pd = dev_get_priv(dev); > >> >> >> > + > >> >> >> > + /* GEMGXL init */ > >> >> >> > + clock.id = PRCI_CLK_GEMGXLPLL; > >> >> >> > + sifive_fu540_prci_set_rate(&clock, 125UL * MHz); > >> >> >> > + > >> >> >> > + sifive_fu540_prci_clock_enable(&__prci_init_clocks[clock.id] > >> >> >> > + , > >> >> >> > + 1); > >> >> >> > + > >> >> >> > + /* Release GEMGXL reset */ > >> >> >> > + v = __prci_readl(pd, PRCI_DEVICESRESETREG_OFFSET); > >> >> >> > + v |= PRCI_DEVICESRESETREG_GEMGXL_RST_N_MASK; > >> >> >> > + __prci_writel(v, PRCI_DEVICESRESETREG_OFFSET, pd); > >> >> >> > + > >> >> >> > + /* Procmon => core clock */ > >> >> >> > + __prci_writel(PRCI_PROCMONCFG_CORE_CLOCK_MASK, > >> >> >PRCI_PROCMONCFG_OFFSET, > >> >> >> > + pd); > >> >> >> > +} > >> >> >> > +#endif > >> >> >> > + > >> >> >> > static int sifive_fu540_prci_probe(struct udevice *dev) { > >> >> >> > int i, err; > >> >> >> > @@ -679,6 +791,12 @@ static int sifive_fu540_prci_probe(struct > >> >> >> > udevice > >> >> >*dev) > >> >> >> > __prci_wrpll_read_cfg0(pd, pc->pwd); > >> >> >> > } > >> >> >> > > >> >> >> > +#ifdef CONFIG_SPL_BUILD > >> >> >> > + corepll_init(dev); > >> >> >> > + ddr_init(dev); > >> >> >> > + ethernet_init(dev); > >> >> >> > +#endif > >> >> >> > >> >> >> 1. Why would ethernet clocks require for SPL 2. Why these clocks > >> >> >> are special for SPL, can't we use it for U-Boot proper 3. This > >> >> >> look like raw clock initialization instead of having in conventional > >> >> >> dm > >way. > >> >> >> since these are here just to use pd. May be reuse the required > >> >> >> clock of what u-boot proper using or move them into spl code. > >> >> > >> >> 1. ethernet clock is not necessary for SPL but SPL performs > >> >> ethernet PHY reset sequence (board/sifive/fu540/spl.c - > >> >> "gem_phy_reset") and for > >> >that ethernet clock needs to be enabled. > >> > > >> >Can we delay that for U-Boot proper to enable PRCI ethernet clock and > >> >reset the PHY ? > >> > >> We can but what happens if someone wants to skip U-Boot (SPL -> OpenSBI > >+ Linux) ? > >> > > > >I am not sure if that's a desired boot flow. If that's something we want to > >support, yes we will have to do it in SPL. > > > >> > > >> >> > >> >> 2. There is nothing special of this clocks for SPL, we can use this for > >> >> U- > >Boot. > >> >> I am planning to add this clocks in device tree but I am not sure > >> >> how to add coreclk (corepll) in device tree Do you guys have any > >> >> suggestion for > >> >this ? > >> >> > >> > > >> >Does the example in > >> > >>http://patchwork.ozlabs.org/project/uboot/patch/20200423023320.138009 > >> >0- > >> >20-sean...@gmail.com/ > >> >help? > >> > > >> >+ cpu0: cpu@0 { > >> >+ device_type = "cpu"; > >> >+ compatible = "kendryte,k210", "sifive,rocket0", "riscv"; reg = <0>; > >> >+ riscv,isa = "rv64imafdgc"; mmu-type = "sv39"; i-cache-block-size = > >> >+ <64>; i-cache-size = <0x8000>; d-cache-block-size = <64>; > >> >+ d-cache-size = <0x8000>; clocks = <&sysclk K210_CLK_CPU>; > >> > > >> >You need this patch to get clock in the CPU node. > >> > > >> > >>http://patchwork.ozlabs.org/project/uboot/patch/20200423023320.138009 > >> >0- > >> >19-sean...@gmail.com/ > > This patch helps but I also need to do clk_set_rate() for coreclk. > > static void corepll_init(struct udevice *dev) > { > ..... > if (v) { > /* corepll 500 Mhz */ > sifive_fu540_prci_set_rate(&clock, 500UL * MHz); > } else { > /* corepll 1 Ghz */ > sifive_fu540_prci_set_rate(&clock, 1000UL * MHz); > } > } > > I think, if "clock-frequency" is provided in dts then it's better to call > clk_set_rate() >
I think so. > Any suggestions are welcome. Regards, Bin