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

[Andreas Bießmann]

Dear Wolfgang,

On 11/04/2013 10:03 AM, Wolfgang Denk wrote:
> In message <1383547247-7017-3-git-send-email-hs at 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

Full ACK.

> 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;

Get and save the length of reset pulse.

> 
>         /* Need to reset PHY -> 500ms reset */
>         writel(AT91_RSTC_KEY | AT91_RSTC_MR_ERSTL(13) |
>                 AT91_RSTC_MR_URSTEN, &rstc->mr);

Setup reset pulse for nearly 500ms (2^(13 + 1) slow clock cycles),
disable user reset interrupt but enable the user reset.

> 
>         writel(AT91_RSTC_KEY | AT91_RSTC_CR_EXTRST, &rstc->cr);

perform the external reset (but no processor nor internal peripherial)
IOW pull the NRST line low.

> 
>         /* Wait for end hardware reset */
>         while (!(readl(&rstc->sr) & AT91_RSTC_SR_NRSTL))
>                 ;

Read the status register until the reset line NRST has high level.

> 
>         /* Restore NRST value */
>         writel(AT91_RSTC_KEY | erstl | AT91_RSTC_MR_URSTEN,
>                 &rstc->mr);

restore pulse length

> 
> So we should factor out all of this (and probably call the function
> at91_phy_reset() then?).

Full ACK.

> 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?

Please read above. AT91 can setup the reset target (core, peripherial,
external pin). We do only pull the line but do not reset the CPU nor
internal peripherial.

> 
>> +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!

Best regards

Andreas Bießmann