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

[Wolfgang Denk]

Dear Heiko,

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

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


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?

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