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

[Heiko Schocher <hs@denx.de>]

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