| Message ID | 1330086194-21762-4-git-send-email-amit.virdi@st.com |
|---|---|
| State | Accepted |
| Commit | 5c16c54124ba7a21979e1eb656e696b7b6d881ec |
| Delegated to: | Stefan Roese |
| Headers | show |
On Friday 24 February 2012 13:23:06 Amit Virdi wrote: > Curently the code makes wrong assumption that the Transfer finished flag > shall be set within the stipulated time. However, there may occur a > scenario in which the TFF flag is not set. Return error in that case. > > Signed-off-by: Vipin Kumar <vipin.kumar@st.com> > Signed-off-by: Amit Virdi <amit.virdi@st.com> > --- > drivers/mtd/st_smi.c | 22 ++++++++++++++-------- > 1 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/mtd/st_smi.c b/drivers/mtd/st_smi.c > index 82f1fe1..ec19b0d 100644 > --- a/drivers/mtd/st_smi.c > +++ b/drivers/mtd/st_smi.c > @@ -58,13 +58,15 @@ static struct flash_dev flash_ids[] = { > * > * Wait until TFF is set in status register > */ > -static void smi_wait_xfer_finish(int timeout) > +static int smi_wait_xfer_finish(int timeout) > { > - while (timeout--) { > + do { > if (readl(&smicntl->smi_sr) & TFF) > - break; > + return 0; > udelay(1000); > - } > + } while (timeout--); > + > + return -1; Somewhat unrelated to the patch topic, but I don't really like this kind of timeout loops. Since it always adds at least 1ms delay after initial failing test. Better use something like this: static int smi_wait_xfer_finish(int timeout) { ulong start = get_timer(0); while (get_timer(start) < timeout) { if (readl(&smicntl->smi_sr) & TFF) return 0; /* Try again after 100usec */ udelay(100); } return -1; } You could tune this udelay(100) down more or even remove it completely. But such a change could be done in a addon cleanup patch, changing all timeout loops this way. I suggest you take a look at my version, here these loops are changes. In the designware networking driver as well, iirc. Thanks, Stefan -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Hello Stefan, On 2/27/2012 3:56 PM, Stefan Roese wrote: > On Friday 24 February 2012 13:23:06 Amit Virdi wrote: >> Curently the code makes wrong assumption that the Transfer finished flag >> shall be set within the stipulated time. However, there may occur a >> scenario in which the TFF flag is not set. Return error in that case. >> >> Signed-off-by: Vipin Kumar<vipin.kumar@st.com> >> Signed-off-by: Amit Virdi<amit.virdi@st.com> >> --- >> drivers/mtd/st_smi.c | 22 ++++++++++++++-------- >> 1 files changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/mtd/st_smi.c b/drivers/mtd/st_smi.c >> index 82f1fe1..ec19b0d 100644 >> --- a/drivers/mtd/st_smi.c >> +++ b/drivers/mtd/st_smi.c >> @@ -58,13 +58,15 @@ static struct flash_dev flash_ids[] = { >> * >> * Wait until TFF is set in status register >> */ >> -static void smi_wait_xfer_finish(int timeout) >> +static int smi_wait_xfer_finish(int timeout) >> { >> - while (timeout--) { >> + do { >> if (readl(&smicntl->smi_sr)& TFF) >> - break; >> + return 0; >> udelay(1000); >> - } >> + } while (timeout--); >> + >> + return -1; > > Somewhat unrelated to the patch topic, but I don't really like this kind of > timeout loops. Since it always adds at least 1ms delay after initial failing > test. Better use something like this: > > static int smi_wait_xfer_finish(int timeout) > { > ulong start = get_timer(0); > > while (get_timer(start)< timeout) { > if (readl(&smicntl->smi_sr)& TFF) > return 0; > > /* Try again after 100usec */ > udelay(100); > } > > return -1; > } > > You could tune this udelay(100) down more or even remove it completely. > > But such a change could be done in a addon cleanup patch, changing all timeout > loops this way. I suggest you take a look at my version, here these loops are > changes. In the designware networking driver as well, iirc. > Thanks for pointing out. In a separate cleanup patch, I shall be changing all the timeout loops for smi driver. I'll check other ST drivers too and change the timer implementation there too. Regards Amit Virdi
Hi Amit, On Wednesday 29 February 2012 10:10:20 Amit Virdi wrote: > > You could tune this udelay(100) down more or even remove it completely. > > > > But such a change could be done in a addon cleanup patch, changing all > > timeout loops this way. I suggest you take a look at my version, here > > these loops are changes. In the designware networking driver as well, > > iirc. > > Thanks for pointing out. In a separate cleanup patch, I shall be > changing all the timeout loops for smi driver. I'll check other ST > drivers too and change the timer implementation there too. Good. Thanks, Stefan -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
diff --git a/drivers/mtd/st_smi.c b/drivers/mtd/st_smi.c index 82f1fe1..ec19b0d 100644 --- a/drivers/mtd/st_smi.c +++ b/drivers/mtd/st_smi.c @@ -58,13 +58,15 @@ static struct flash_dev flash_ids[] = { * * Wait until TFF is set in status register */ -static void smi_wait_xfer_finish(int timeout) +static int smi_wait_xfer_finish(int timeout) { - while (timeout--) { + do { if (readl(&smicntl->smi_sr) & TFF) - break; + return 0; udelay(1000); - } + } while (timeout--); + + return -1; } /* @@ -83,7 +85,8 @@ static unsigned int smi_read_id(flash_info_t *info, int banknum) writel((banknum << BANKSEL_SHIFT) | SEND | TX_LEN_1 | RX_LEN_3, &smicntl->smi_cr2); - smi_wait_xfer_finish(XFER_FINISH_TOUT); + if (smi_wait_xfer_finish(XFER_FINISH_TOUT)) + return -EIO; value = (readl(&smicntl->smi_rr) & 0x00FFFFFF); @@ -151,7 +154,8 @@ static unsigned int smi_read_sr(int bank) /* Performing a RSR instruction in HW mode */ writel((bank << BANKSEL_SHIFT) | RD_STATUS_REG, &smicntl->smi_cr2); - smi_wait_xfer_finish(XFER_FINISH_TOUT); + if (smi_wait_xfer_finish(XFER_FINISH_TOUT)) + return -1; /* Restore the CTRL REG1 state */ writel(ctrlreg1, &smicntl->smi_cr1); @@ -211,7 +215,8 @@ static int smi_write_enable(int bank) /* Give the Flash, Write Enable command */ writel((bank << BANKSEL_SHIFT) | WE, &smicntl->smi_cr2); - smi_wait_xfer_finish(XFER_FINISH_TOUT); + if (smi_wait_xfer_finish(XFER_FINISH_TOUT)) + return -1; /* Restore the CTRL REG1 state */ writel(ctrlreg1, &smicntl->smi_cr1); @@ -292,7 +297,8 @@ static int smi_sector_erase(flash_info_t *info, unsigned int sector) writel(instruction, &smicntl->smi_tr); writel((bank << BANKSEL_SHIFT) | SEND | TX_LEN_4, &smicntl->smi_cr2); - smi_wait_xfer_finish(XFER_FINISH_TOUT); + if (smi_wait_xfer_finish(XFER_FINISH_TOUT)) + return -EIO; if (smi_wait_till_ready(bank, CONFIG_SYS_FLASH_ERASE_TOUT)) return -EBUSY;