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