[U-Boot] [PATCH] arm: at91: clock: Add the generated clock support

[Yang, Wenyou]

Hi Bo Shen,

Thank you for your review.

> -----Original Message-----
> From: Bo Shen [mailto:voice.shen at gmail.com]
> Sent: 2015年9月10日 6:51
> To: Yang, Wenyou; U-Boot Mailing List
> Subject: Re: [U-Boot] [PATCH] arm: at91: clock: Add the generated clock support
> 
> Hi Wenyou,
> 
> On 09/09/2015 10:29 AM, Wenyou Yang wrote:
> > Some peripherals may need a second clock source that may be different
> > from the system clock. This second clock is the generated clock (GCK)
> > and is managed by the PMC via PMC_PCR.
> >
> > For simplicity, the source of the GCK is fixed to PLLA_CLK.
> >
> > Signed-off-by: Wenyou Yang <wenyou.yang at atmel.com>
> > ---
> >
> >   arch/arm/mach-at91/armv7/clock.c           |   57
> ++++++++++++++++++++++++++++
> >   arch/arm/mach-at91/include/mach/at91_pmc.h |   13 +++++++
> >   arch/arm/mach-at91/include/mach/clk.h      |    3 ++
> >   3 files changed, 73 insertions(+)
> >
> > diff --git a/arch/arm/mach-at91/armv7/clock.c
> > b/arch/arm/mach-at91/armv7/clock.c
> > index 0bf453e..84418a3 100644
> > --- a/arch/arm/mach-at91/armv7/clock.c
> > +++ b/arch/arm/mach-at91/armv7/clock.c
> > @@ -5,6 +5,7 @@
> >    * Copyright (C) 2005 Ivan Kokshaysky
> >    * Copyright (C) 2009 Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj at jcrosoft.com>
> >    * Copyright (C) 2013 Bo Shen <voice.shen at atmel.com>
> > + * Copyright (C) 2015 Wenyou Yang <wenyou.yang at atmel.com>
> >    *
> >    * SPDX-License-Identifier:	GPL-2.0+
> >    */
> > @@ -173,3 +174,59 @@ void at91_periph_clk_disable(int id)
> >
> >   	writel(regval, &pmc->pcr);
> >   }
> > +
> > +void at91_enable_periph_generated_clk(u32 id) {
> > +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
> > +	u32 regval;
> > +
> > +	if (id > AT91_PMC_PCR_PID_MASK)
> > +		return;
> > +
> > +	writel(id, &pmc->pcr);
> > +	regval = readl(&pmc->pcr);
> > +	regval &= ~AT91_PMC_PCR_GCKCSS;
> > +	regval &= ~AT91_PMC_PCR_GCKDIV;
> > +	regval |= AT91_PMC_PCR_GCKCSS_PLLA_CLK |
> > +		  AT91_PMC_PCR_CMD_WRITE |
> > +		  AT91_PMC_PCR_GCKDIV_(1) |
> > +		  AT91_PMC_PCR_GCKEN;
> > +
> > +	writel(regval, &pmc->pcr);
> > +
> > +	while (!(readl(&pmc->sr) & AT91_PMC_GCKRDY))
> > +		;
> 
> Here, do we need to hang the whole system?
Do you mean, add the timeout to while()? 

But we think if the clock can't reach to a stable state, the system must be in wrong condition.
So, I don't think this timeout is necessary.

Thanks.

> 
> > +}
> > +
> > +u32 at91_get_periph_generated_clk(u32 id) {
> > +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
> > +	u32 regval, clk_source, div;
> > +	u32 freq = 0;
> > +
> > +	if (id > AT91_PMC_PCR_PID_MASK)
> > +		return 0;
> > +
> > +	writel(id, &pmc->pcr);
> > +	regval = readl(&pmc->pcr);
> > +
> > +	clk_source = regval & AT91_PMC_PCR_GCKCSS;
> > +	switch (clk_source) {
> > +	case AT91_PMC_PCR_GCKCSS_SLOW_CLK:
> > +		freq = CONFIG_SYS_AT91_SLOW_CLOCK;
> > +		break;
> > +	case AT91_PMC_PCR_GCKCSS_MAIN_CLK:
> > +		freq = gd->arch.main_clk_rate_hz;
> > +		break;
> > +	case AT91_PMC_PCR_GCKCSS_PLLA_CLK:
> > +		freq = gd->arch.plla_rate_hz;
> > +		break;
> > +	default:
> > +		break;
> 
> For the default, is it valuable to add error information? Or return an invalid value?
Error information is useful, it will be added in next version. Thanks

If reach to default, "freq" will use the initial value(i.e, 0), it can be as an invalid value.

> 
> > +	}
> > +
> > +	div = ((regval & AT91_PMC_PCR_GCKDIV) >>
> AT91_PMC_PCR_GCKDIV_OFFSET);
> > +	div += 1;
> > +
> > +	return freq / div;
> > +}
> > diff --git a/arch/arm/mach-at91/include/mach/at91_pmc.h
> > b/arch/arm/mach-at91/include/mach/at91_pmc.h
> > index 8a3fb94..dcd6e36 100644
> > --- a/arch/arm/mach-at91/include/mach/at91_pmc.h
> > +++ b/arch/arm/mach-at91/include/mach/at91_pmc.h
> > @@ -153,8 +153,20 @@ typedef struct at91_pmc {
> >   #define AT91_PMC_IXR_MOSCSELS		0x00010000
> >
> >   #define AT91_PMC_PCR_PID_MASK		(0x3f)
> > +#define AT91_PMC_PCR_GCKCSS		(0x7 << 8)
> > +#define		AT91_PMC_PCR_GCKCSS_SLOW_CLK	(0x0 << 8)
> > +#define		AT91_PMC_PCR_GCKCSS_MAIN_CLK	(0x1 << 8)
> > +#define		AT91_PMC_PCR_GCKCSS_PLLA_CLK	(0x2 << 8)
> > +#define		AT91_PMC_PCR_GCKCSS_UPLL_CLK	(0x3 << 8)
> > +#define		AT91_PMC_PCR_GCKCSS_MCK_CLK	(0x4 << 8)
> > +#define		AT91_PMC_PCR_GCKCSS_AUDIO_CLK	(0x5 << 8)
> >   #define AT91_PMC_PCR_CMD_WRITE		(0x1 << 12)
> > +#define AT91_PMC_PCR_DIV		(0x3 << 16)
> > +#define AT91_PMC_PCR_GCKDIV		(0xff << 20)
> > +#define		AT91_PMC_PCR_GCKDIV_(x)		((x) << 20)
> 
> It is dangerous here, if "x = 0xfff", then what will happen?
You mean add "& 0xff" after (x). it will added in next version.

Thanks.

> 
> > +#define		AT91_PMC_PCR_GCKDIV_OFFSET	20
> >   #define AT91_PMC_PCR_EN			(0x1 << 28)
> > +#define AT91_PMC_PCR_GCKEN		(0x1 << 29)
> >
> >   #define		AT91_PMC_PCK		(1 <<  0)		/*
> Processor Clock */
> >   #define		AT91RM9200_PMC_UDP	(1 <<  1)		/*
> USB Devcice Port Clock [AT91RM9200 only] */
> > @@ -236,6 +248,7 @@ typedef struct at91_pmc {
> >   #define		AT91_PMC_PCK1RDY	(1 <<  9)		/*
> Programmable Clock 1 */
> >   #define		AT91_PMC_PCK2RDY	(1 << 10)		/*
> Programmable Clock 2 */
> >   #define		AT91_PMC_PCK3RDY	(1 << 11)		/*
> Programmable Clock 3 */
> > +#define		AT91_PMC_GCKRDY		(1 << 24)
> >
> >   #define		AT91_PMC_PROTKEY	0x504d4301	/* Activation Code
> */
> >   #endif
> > diff --git a/arch/arm/mach-at91/include/mach/clk.h
> > b/arch/arm/mach-at91/include/mach/clk.h
> > index 1d45e2d..dd1ed96 100644
> > --- a/arch/arm/mach-at91/include/mach/clk.h
> > +++ b/arch/arm/mach-at91/include/mach/clk.h
> > @@ -119,4 +119,7 @@ static inline unsigned long get_pit_clk_rate(void)
> >   int at91_clock_init(unsigned long main_clock);
> >   void at91_periph_clk_enable(int id);
> >   void at91_periph_clk_disable(int id);
> > +void at91_enable_periph_generated_clk(u32 id);
> > +u32 at91_get_periph_generated_clk(u32 id);
> > +
> >   #endif /* __ASM_ARM_ARCH_CLK_H__ */
> >
> 
> Best Regards,
> Bo Shen

Wenyou Yang