From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bo Shen Date: Thu, 14 Nov 2013 14:53:10 +0800 Subject: [U-Boot] [PATCH v3 6/6] arm: atmel: sama5d3: spl boot from fat fs SD card In-Reply-To: <52846D72.3010108@googlemail.com> References: <1383715757-20170-1-git-send-email-voice.shen@atmel.com> <1383715757-20170-7-git-send-email-voice.shen@atmel.com> <52837FEF.3070703@gmail.com> <5284651F.8060208@denx.de> <52846D72.3010108@googlemail.com> Message-ID: <52847356.20607@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Andreas, On 11/14/2013 02:28 PM, Andreas Bie?mann wrote: > Hello Heiko, > > On 14.11.13 06:52, Heiko Schocher wrote: >> Am 13.11.2013 14:34, schrieb Andreas Bie?mann: >>> Hi Bo, >>> >>> On 11/06/2013 06:29 AM, Bo Shen wrote: >>>> Enable Atmel sama5d3xek boart spl boot support, which can load u-boot >>>> from SD card with FAT file system. >>>> >>>> Signed-off-by: Bo Shen >>>> >>>> --- > > > >>>> +static void at91_disable_wdt(void) >>> >>> Why should we disable the WDT in SPL? I think it would be better to >>> configure a working timer value than just disable it. >> >> This is currently done in the at91bootstrap code too... > > I know ... >> >>> Well it's easy and works, but for the future I think it would be good to >>> let it run while in SPL and u-boot. >> >> We should have the option to enable/disable it ... The watchdog is one time configurable. If configurabled, can not change anymore, so we disable it by default. > I just like to mention that this should be changed in near future. > >>>> +{ >>>> + struct at91_wdt *wdt = (struct at91_wdt *)ATMEL_BASE_WDT; >>>> + >>>> + writel(AT91_WDT_MR_WDDIS,&wdt->mr); >>>> +} >>>> + >>>> +void at91_plla_init(u32 pllar) >>>> +{ >>>> + struct at91_pmc *pmc = (struct at91_pmc *)ATMEL_BASE_PMC; >>>> + >>> >>> We should ensure bit 29 to be '1' here. >> >> What is bit 29 ? > > 'ONE' ;) Spec says it must be written to '1' OK, I will add a check here. >>>> + writel(pllar,&pmc->pllar); >>>> + while (!(readl(&pmc->sr)& (AT91_PMC_LOCKA | AT91_PMC_MCKRDY))) >>>> + ; >>> >>> Especially for doing such things it would be best handled by the WDT on >>> error. As watchdog is disabled, we can not use it here. >>>> +} >>>> + >>>> +void at91_mck_init(u32 mckr) >>>> +{ >>>> + 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); >>> >>> Why gets the at91_mck_init() just some parts of the MCK register (some >>> fields are preserved here) while the at91_plla_init() just rewrites the >>> PLLA register? > > Don't you see the error in my sentence here? ;) I thought some parts of > the register content where preserved. The other way round is true, we > need to clean up relevant parts (PRES, MDIV, and PLLADIV Bit(s)). Sorry, > my fault here. So, keep it as is(?) >>> I think it is not much more than hiding the writel() register access. I >>> think a better API would be to request some specific frequency and we >>> calculate the register values with that input. >>> Please let us discuss this. >> >> Such a function would be nice, indeed. But I would accept this function, >> to get SPL code into mainline... (I have the same function ;-) > > Yea, no problem with this function. I just wonder why the > at91_plla_init() is a plain writel() (plus waiting for lock) and this > one does a bit more (preserve CSS... We could also declare the whole > register content when calling at91_mckr_init(), don't we? at91_plla_init()'s main purpose is to wait the lock. > This is no change request in general, but please lets discuss why we not > just write the given register content. This make we focus on what we need to configure. Best Regards, Bo Shen