Messages in this thread Patch in this message | | | Date | Thu, 20 Mar 2014 00:27:44 -0700 | From | Brian Norris <> | Subject | Re: [PATCH 27/35] mtd: st_spi_fsm: Supply a busy wait for post-write status |
| |
On Tue, Feb 18, 2014 at 02:55:54PM +0000, Lee Jones wrote: > When we write data to the Serial Flash chip we'll wait a predetermined > period of time before giving up. During that period of time we poll the > status register until completion. > > Acked-by Angus Clark <angus.clark@st.com> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/mtd/devices/st_spi_fsm.c | 223 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 223 insertions(+) > > diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c > index ea55548..0eacfbf 100644 > --- a/drivers/mtd/devices/st_spi_fsm.c > +++ b/drivers/mtd/devices/st_spi_fsm.c > @@ -238,8 +238,18 @@ > #define FLASH_CMD_READ4_1_1_4 0x6c > #define FLASH_CMD_READ4_1_4_4 0xec > > +/* Status register */ > +#define FLASH_STATUS_BUSY 0x01 > +#define FLASH_STATUS_WEL 0x02 > +#define FLASH_STATUS_BP0 0x04 > +#define FLASH_STATUS_BP1 0x08 > +#define FLASH_STATUS_BP2 0x10 > +#define FLASH_STATUS_SRWP0 0x80 > +#define FLASH_STATUS_TIMEOUT 0xff > + > #define FLASH_PAGESIZE 256 /* In Bytes */ > #define FLASH_PAGESIZE_32 (FLASH_PAGESIZE / 4) /* In uint32_t */ > +#define FLASH_MAX_BUSY_WAIT (300 * HZ) /* Maximum 'CHIPERASE' time */ > > /* > * Flags to tweak operation of default read/write/erase routines > @@ -519,6 +529,22 @@ static struct stfsm_seq stfsm_seq_read_jedec = { > SEQ_CFG_STARTSEQ), > }; > > +static struct stfsm_seq stfsm_seq_read_status_fifo = { > + .data_size = TRANSFER_SIZE(4), > + .seq_opc[0] = (SEQ_OPC_PADS_1 | > + SEQ_OPC_CYCLES(8) | > + SEQ_OPC_OPCODE(FLASH_CMD_RDSR)), > + .seq = { > + STFSM_INST_CMD1, > + STFSM_INST_DATA_READ, > + STFSM_INST_STOP, > + }, > + .seq_cfg = (SEQ_CFG_PADS_1 | > + SEQ_CFG_READNOTWRITE | > + SEQ_CFG_CSDEASSERT | > + SEQ_CFG_STARTSEQ), > +}; > + > static struct stfsm_seq stfsm_seq_erase_sector = { > /* 'addr_cfg' configured during initialisation */ > .seq_opc = { > @@ -699,6 +725,53 @@ static int stfsm_enter_32bit_addr(struct stfsm *fsm, int enter) > return 0; > } > > +static uint8_t stfsm_wait_busy(struct stfsm *fsm) > +{ > + struct stfsm_seq *seq = &stfsm_seq_read_status_fifo; > + unsigned long deadline; > + uint32_t status; > + int timeout = 0; > + > + /* Use RDRS1 */ > + seq->seq_opc[0] = (SEQ_OPC_PADS_1 | > + SEQ_OPC_CYCLES(8) | > + SEQ_OPC_OPCODE(FLASH_CMD_RDSR)); > + > + /* Load read_status sequence */ > + stfsm_load_seq(fsm, seq); > + > + /* > + * Repeat until busy bit is deasserted, or timeout, or error (S25FLxxxS) > + */ > + deadline = jiffies + FLASH_MAX_BUSY_WAIT; > + while (!timeout) { > + cond_resched();
Do you really want to give up the CPU right from the start? Shouldn't you run through the loop once first? Maybe move this to the end of the while loop.
> + > + if (time_after_eq(jiffies, deadline)) > + timeout = 1; > + > + stfsm_wait_seq(fsm); > + > + stfsm_read_fifo(fsm, &status, 4); > + > + if ((status & FLASH_STATUS_BUSY) == 0) > + return 0; > + > + if ((fsm->configuration & CFG_S25FL_CHECK_ERROR_FLAGS) && > + ((status & S25FL_STATUS_P_ERR) || > + (status & S25FL_STATUS_E_ERR))) > + return (uint8_t)(status & 0xff); > + > + if (!timeout) > + /* Restart */ > + writel(seq->seq_cfg, fsm->base + SPI_FAST_SEQ_CFG); > + } > + > + dev_err(fsm->dev, "timeout on wait_busy\n"); > + > + return FLASH_STATUS_TIMEOUT; > +} > + > static int stfsm_wrvcr(struct stfsm *fsm, uint8_t data) > { > struct stfsm_seq *seq = &stfsm_seq_wrvcr; > @@ -1020,6 +1093,98 @@ static int stfsm_read(struct stfsm *fsm, uint8_t *buf, uint32_t size, > return 0; > } > > +static int stfsm_write(struct stfsm *fsm, const uint8_t *const buf, > + const uint32_t size, const uint32_t offset)
I'm still not sure about addint the 'const' label to these local parameters. It doesn't add any value to the callers (they are passed-by-value anyway), nor are they very useful locally. I mentioned this in my first review, and you never commented. I won't insist you drop the 'const', if you really prefer it...
My suggestion:
const uint8_t *const buf => const uint8_t *buf const uint32_t size => uint32_t size const uint32_t offset => uint32_t offset
(For the first one: a pointer to constant data is a useful type annotation; a constant pointer to data is not.)
Similar patterns apply elsewhere.
> +{ > + struct stfsm_seq *seq = &stfsm_seq_write; > + uint32_t data_pads; > + uint32_t write_mask; > + uint32_t size_ub; > + uint32_t size_lb; > + uint32_t size_mop; > + uint32_t tmp[4]; > + uint32_t page_buf[FLASH_PAGESIZE_32]; > + uint8_t *t = (uint8_t *)&tmp; > + const uint8_t *p; > + int ret; > + int i; > + > + dev_dbg(fsm->dev, "writing %d bytes to 0x%08x\n", size, offset); > + > + /* Enter 32-bit address mode, if required */ > + if (fsm->configuration & CFG_WRITE_TOGGLE_32BIT_ADDR) > + stfsm_enter_32bit_addr(fsm, 1); > + > + /* Must write in multiples of 32 cycles (or 32*pads/8 bytes) */ > + data_pads = ((seq->seq_cfg >> 16) & 0x3) + 1; > + write_mask = (data_pads << 2) - 1; > + > + /* Handle non-aligned buf */ > + if ((uint32_t)buf & 0x3) { > + memcpy(page_buf, buf, size); > + p = (uint8_t *)page_buf; > + } else { > + p = buf; > + } > + > + /* Handle non-aligned size */ > + size_ub = (size + write_mask) & ~write_mask; > + size_lb = size & ~write_mask; > + size_mop = size & write_mask; > + > + seq->data_size = TRANSFER_SIZE(size_ub); > + seq->addr1 = (offset >> 16) & 0xffff; > + seq->addr2 = offset & 0xffff; > + > + /* Need to set FIFO to write mode, before writing data to FIFO (see > + * GNBvb79594) > + */ > + writel(0x00040000, fsm->base + SPI_FAST_SEQ_CFG); > + > + /* > + * Before writing data to the FIFO, apply a small delay to allow a > + * potential change of FIFO direction to complete. > + */ > + if (fsm->fifo_dir_delay == 0) > + readl(fsm->base + SPI_FAST_SEQ_CFG); > + else > + udelay(fsm->fifo_dir_delay); > + > + > + /* Write data to FIFO, before starting sequence (see GNBvd79593) */ > + if (size_lb) { > + stfsm_write_fifo(fsm, (uint32_t *)p, size_lb); > + p += size_lb; > + } > + > + /* Handle non-aligned size */ > + if (size_mop) { > + memset(t, 0xff, write_mask + 1); /* fill with 0xff's */ > + for (i = 0; i < size_mop; i++) > + t[i] = *p++; > + > + stfsm_write_fifo(fsm, tmp, write_mask + 1); > + } > + > + /* Start sequence */ > + stfsm_load_seq(fsm, seq); > + > + /* Wait for sequence to finish */ > + stfsm_wait_seq(fsm); > + > + /* Wait for completion */ > + ret = stfsm_wait_busy(fsm); > + > + /* Exit 32-bit address mode, if required */ > + if (fsm->configuration & CFG_WRITE_TOGGLE_32BIT_ADDR) { > + stfsm_enter_32bit_addr(fsm, 0); > + if (fsm->configuration & CFG_WRITE_EX_32BIT_ADDR_DELAY) > + udelay(1); > + } > + > + return 0; > +} > + > /* > * Read an address range from the flash chip. The address range > * may be any size provided it is within the physical boundaries. > @@ -1053,6 +1218,63 @@ static int stfsm_mtd_read(struct mtd_info *mtd, loff_t from, size_t len, > return 0; > } > > +/* > + * Write an address range to the flash chip. Data must be written in > + * FLASH_PAGESIZE chunks. The address range may be any size provided > + * it is within the physical boundaries. > + */ > +static int stfsm_mtd_write(struct mtd_info *mtd, loff_t to, size_t len, > + size_t *retlen, const u_char *buf) > +{ > + struct stfsm *fsm = dev_get_drvdata(mtd->dev.parent); > + > + u32 page_offs; > + u32 bytes; > + uint8_t *b = (uint8_t *)buf; > + int ret = 0; > + > + dev_dbg(fsm->dev, "%s to 0x%08x, len %zd\n", __func__, (u32)to, len); > + > + if (retlen) > + *retlen = 0;
You're still duplicating the code from mtd_write(). Please kill the above.
> + > + if (!len) > + return 0;
Ditto.
> + > + if (to + len > mtd->size) > + return -EINVAL;
Ditto.
> + > + /* Offset within page */ > + page_offs = to % FLASH_PAGESIZE; > + > + mutex_lock(&fsm->lock); > + > + while (len) { > + /* Write up to page boundary */ > + bytes = min(FLASH_PAGESIZE - page_offs, len); > + > + ret = stfsm_write(fsm, b, bytes, to); > + if (ret) > + goto out1; > + > + b += bytes; > + len -= bytes; > + to += bytes; > + > + /* We are now page-aligned */ > + page_offs = 0; > + > + if (retlen) You can assume retlen is always non-NULL. Drop the 'if (retlen)'. > + *retlen += bytes; > + > + } > + > +out1: > + mutex_unlock(&fsm->lock); > + > + return ret; > +} > + > static void stfsm_read_jedec(struct stfsm *fsm, uint8_t *const jedec) > { > const struct stfsm_seq *seq = &stfsm_seq_read_jedec; > @@ -1310,6 +1532,7 @@ static int stfsm_probe(struct platform_device *pdev) > fsm->mtd.erasesize = info->sector_size; > > fsm->mtd._read = stfsm_mtd_read; > + fsm->mtd._write = stfsm_mtd_write; > > dev_err(&pdev->dev, > "Found serial flash device: %s\n"
Brian
| |