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

Hello Wolfgang,

Am 04.11.2013 10:03, schrieb Wolfgang Denk:

Dear Heiko,

In message1383547247-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