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

Andreas Bießmann
2013-11-04T10:35:13Z

Dear Wolfgang,

On 11/04/2013 10:03 AM, Wolfgang Denk wrote:
> 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

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