[U-Boot] [PATCH v2 2/4] arm, at91: add function for waiting if reset ends

Heiko Schocher
2013-11-04T09:11:58Z

Hello Wolfgang,

Am 04.11.2013 10:03, schrieb Wolfgang Denk:
> Dear Heiko,
>
> In message<1383547247-7017-3-git-send-email-hs@denx.de> you wrote:
>> add function for waiting if reset ends. If reset never ends,
>> timeout and print an error message.
>
> I think this patch needs some rework.
>
> First, I think we should point out in the commit mnessage that we're
> not talking about a general hardware reset here (how could the code
> be running if the CPU was helt in reset?), but that we are actually
> talking about the PHY reset.
>
>> arch/arm/cpu/arm926ejs/at91/reset.c | 15 +++++++++++++++
>> arch/arm/include/asm/arch-at91/at91_rstc.h | 1 +
>> 2 files changed, 16 insertions(+)
>
> Second, while I highly appreciate your effort to identify and factor
> out common code, we should then actually use this new common function
> to replace all the occurrences of that common code - i. e. I would
> expect to see modifications at least in the following files:
>
> board/BuS/vl_ma2sc/vl_ma2sc.c
> board/afeb9260/afeb9260.c
> board/atmel/at91sam9260ek/at91sam9260ek.c
> board/atmel/at91sam9263ek/at91sam9263ek.c
> board/atmel/at91sam9m10g45ek/at91sam9m10g45ek.c
> board/bluewater/snapper9260/snapper9260.c
> board/calao/sbc35_a9g20/sbc35_a9g20.c
> board/eukrea/cpu9260/cpu9260.c
> board/taskit/stamp9g20/stamp9g20.c

Yep, full ack, do this in the next version

> Third, I think we should not only replace the waiting for the end og
> the PHY reset loop (and add a timeout to it), but instead we should
> factor out the whole block of code performing the PHY reset. From
> what I've seen, the following piece of code is repeated identical
> (except for formatting) in all these files:
>
> erstl = readl(&rstc->mr)& AT91_RSTC_MR_ERSTL_MASK;
>
> /* Need to reset PHY -> 500ms reset */
> writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
> AT91_RSTC_MR_URSTEN,&rstc->mr);
>
> writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST,&rstc->cr);
>
> /* Wait for end hardware reset */
> while (!(readl(&rstc->sr)& AT91_RSTC_SR_NRSTL))
> ;
>
> /* Restore NRST value */
> writel(AT91_RSTC_KEY | erstl | AT91_RSTC_MR_URSTEN,
> &rstc->mr);
>
> So we should factor out all of this (and probably call the function
> at91_phy_reset() then?).

Yes.

> Are thwere any AT91 experts out there who actually understand this
> code? In my (very limited) understanding, NRST is the "microcon-
> troller reset pin", so "Wait for end hardware reset" (and polling
> AT91_RSTC_SR_NRSTL) does not make much sense - how could this code be
> running if the microprocessor was helt in reset? After asserting the
> AT91_RSTC_CR_EXTRST (external reset) we are probably waiting for
> something else?

Good question!

>> +void at91_wait_for_reset(int timeout)
>> +{
>> + struct at91_rstc *rstc = (struct at91_rstc *)ATMEL_BASE_RSTC;
>> + int count = 0;
>> +
>> + while (!(readl(&rstc->sr)& AT91_RSTC_SR_NRSTL)) {
>> + if (count>= timeout) {
>> + printf("reset timeout.\n");
>> + return;
>> + }
>> + udelay(10);
>> + timeout++;
>> + }
>> +}
>
> Finally, you should fix the code so that it really times out - this
> code will not, as you initialize count as zero and then wait for
> "count>= timeout", but you never change "count"; instead you
> increment "timeout", so you might have to wait for up to INT_MAX * 10
> us or about 6 hours...

Good catch! Thanks

bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany