From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bo Shen <voice.shen@atmel.com>
Date: Thu, 31 Oct 2013 09:38:24 +0800
Subject: [U-Boot] [RFC PATCH 5/7] arm: atmel: add plla and mck
	initialize function
In-Reply-To: <5270EBD5.4010909@gmail.com>
References: <1383124508-8413-1-git-send-email-voice.shen@atmel.com>
	<1383124508-8413-6-git-send-email-voice.shen@atmel.com>
	<5270EBD5.4010909@gmail.com>
Message-ID: <5271B490.3010307@atmel.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Andreas,

On 10/30/2013 19:21, Andreas Bie?mann wrote:
> Hi Bo,
>
> On 10/30/2013 10:15 AM, Bo Shen wrote:
>> Add plla and mck initialize function.
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>>   arch/arm/cpu/armv7/at91/clock.c              |   27 ++++++++++++++++++++++++++
>>   arch/arm/include/asm/arch-at91/at91_common.h |    2 ++
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv7/at91/clock.c b/arch/arm/cpu/armv7/at91/clock.c
>> index 1588e0c..ec92950 100644
>> --- a/arch/arm/cpu/armv7/at91/clock.c
>> +++ b/arch/arm/cpu/armv7/at91/clock.c
>> @@ -120,3 +120,30 @@ void at91_periph_clk_enable(int id)
>>   	else
>>   		writel(1 << id, &pmc->pcer);
>>   }
>> +
>> +void at91_plla_init(u32 pllar, int timeout)
>> +{
>> +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
>> +
>> +	writel(pllar, &pmc->pllar);
>> +	while (!(readl(&pmc->sr) & (AT91_PMC_LOCKA | AT91_PMC_MCKRDY)))
>> +		timeout--;
>
> I do not understand use of this timeout here ... You do not check it nor
> return something ... It would make sense to add an check into the wile()
> and return upon the state of timeout true/false.
> This however is not needed here cause we either succeed to setup the PLL
> or not. If not we could not proceed anyways, so why check it here?

OK, I will remove this timeout here.

> In connection with a watchdog it could make sense to kick it while
> waiting for PLL initialisation.

As watchdog is disabled, so can not use it.

>> +}
>> +
>> +void at91_mck_init(u32 mckr, int timeout)
>
> I wonder if it would make sense to abstract the functionality of mckr to
> the function parameters. Something like 'I want to set 400MHz, do it for
> me'. Maybe this doesn't make any sense, lets discuss.

As the SoC can run in 132MHz * (2, 3, 4), if it runs in different 
frequency, the mckr configuration is different, so we need this function,

>> +{
>> +	struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC;
>> +	u32 tmp;
>> +
>> +	tmp = readl(&pmc->mckr);
>> +	tmp &= ~(AT91_PMC_MCKR_PRES_MASK |
>> +		 AT91_PMC_MCKR_MDIV_MASK |
>> +		 AT91_PMC_MCKR_PLLADIV_2);
>> +	tmp |= mckr & (AT91_PMC_MCKR_PRES_MASK |
>> +		       AT91_PMC_MCKR_MDIV_MASK |
>> +		       AT91_PMC_MCKR_PLLADIV_2);
>> +	writel(tmp, &pmc->mckr);
>> +
>> +	while (!(readl(&pmc->sr) & AT91_PMC_MCKRDY))
>> +		timeout--;
>> +}
>> diff --git a/arch/arm/include/asm/arch-at91/at91_common.h b/arch/arm/include/asm/arch-at91/at91_common.h
>> index abcb97d..a692275 100644
>> --- a/arch/arm/include/asm/arch-at91/at91_common.h
>> +++ b/arch/arm/include/asm/arch-at91/at91_common.h
>> @@ -22,5 +22,7 @@ void at91_spi1_hw_init(unsigned long cs_mask);
>>   void at91_udp_hw_init(void);
>>   void at91_uhp_hw_init(void);
>>   void at91_lcd_hw_init(void);
>> +void at91_plla_init(u32 pllar, int timeout);
>> +void at91_mck_init(u32 mckr, int timeout);
>>
>>   #endif /* AT91_COMMON_H */
>>
>

Best Regards,
Bo Shen