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<voice.s...@atmel.com>
---
<snip>
+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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
