| Message ID | 1407374222-8448-7-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Thursday, August 07, 2014 at 03:17:00 AM, Brian Norris wrote: > We don't need to expose a 'wait-till-ready' interface to drivers. Status > register polling should be handled by the core spi-nor.c library, and as > of now, I see no need to provide a special driver-specific hook for it. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> Very nice, Reviewed-by: Marek Vasut <marex@denx.de> Best regards, Marek Vasut
On Wed, Aug 06, 2014 at 06:17:00PM -0700, Brian Norris wrote: > We don't need to expose a 'wait-till-ready' interface to drivers. Status > register polling should be handled by the core spi-nor.c library, and as > of now, I see no need to provide a special driver-specific hook for it. Please do not drop this hook, please see why we add this hook: http://lists.infradead.org/pipermail/linux-mtd/2013-December/050561.html " The default implementation would do just as you suggest, and I would expect most H/W drivers to use the default implementation. However, some H/W Controllers can automate the polling of status registers, so having a hook allows the driver override the default behaviour. " The spi-nor framework will used by more drivers besides the m25p80 and fsl-quadspi. Some NOR flash controller may be very strange. thanks Huang Shijie
On Sat, Aug 09, 2014 at 05:53:03PM +0800, Huang Shijie wrote: > On Wed, Aug 06, 2014 at 06:17:00PM -0700, Brian Norris wrote: > > We don't need to expose a 'wait-till-ready' interface to drivers. Status > > register polling should be handled by the core spi-nor.c library, and as > > of now, I see no need to provide a special driver-specific hook for it. > > Please do not drop this hook, please see why we add this hook: > http://lists.infradead.org/pipermail/linux-mtd/2013-December/050561.html > > " > The default implementation would do just as you suggest, and I would > expect most H/W drivers to use the default implementation. However, some H/W > Controllers can automate the polling of status registers, so having a hook allows > the driver override the default behaviour. > " > > The spi-nor framework will used by more drivers besides the m25p80 and > fsl-quadspi. Some NOR flash controller may be very strange. OK, but the wait-till-ready hook should not look like the current one. If it is used as an optimization of sorts, then we can provide it in *addition* to the checks we do, but not *instead of*. I sincerely doubt that any specialized SPI NOR controller will know how to check both SR and FSR where appropriate, and it probably won't understand other specialized sequences as they are developed / thrown on our lap by flash vendors. So, the spi-nor.c "wait until ready" might have a sequence like this: 1. (Optionally) use low-level driver's "wait" function; skip if not present 2. use the read register hooks to check SR/FSR to confirm completion I do not want to toss #2 into the low-level driver, so if there is a need for #1, it should be done in addition to our spi-nor.c code, not instead. (To this end, I believe Marek also complained about this when we were adding the FSR-checking code; we should not have drivers and spi-nor.c fighting over callback functions.) So I'm inclined to at most address #1 through an optional callback, and possibly even to ignore that until we see an actual driver that needs it. Brian
On 7 August 2014 03:17, Brian Norris <computersforpeace@gmail.com> wrote: > We don't need to expose a 'wait-till-ready' interface to drivers. Status > register polling should be handled by the core spi-nor.c library, and as > of now, I see no need to provide a special driver-specific hook for it. Do you think we could use spi-nor for Broadcom's SPI Atmel flashes? If so, we could need this. Broadcom's code includes support for two Atmel flashes: AT25F512 and AT25F2048. From what I can see, Atmel-specific code uses command 0xD7 (checking for bit 0x80 to be unset). That is a bit different from "standard" flashes using command 0xD7 (checking for bit 0x80 to be unset).
On 12 August 2014 07:13, Rafał Miłecki <zajec5@gmail.com> wrote: > On 7 August 2014 03:17, Brian Norris <computersforpeace@gmail.com> wrote: >> We don't need to expose a 'wait-till-ready' interface to drivers. Status >> register polling should be handled by the core spi-nor.c library, and as >> of now, I see no need to provide a special driver-specific hook for it. > > Do you think we could use spi-nor for Broadcom's SPI Atmel flashes? If > so, we could need this. > > Broadcom's code includes support for two Atmel flashes: AT25F512 and AT25F2048. > > From what I can see, Atmel-specific code uses command 0xD7 (checking > for bit 0x80 to be unset). That is a bit different from "standard" > flashes using command 0xD7 (checking for bit 0x80 to be unset). I meant standard code using command 0x05 and checking for 0x01 to be unset.
On Tue, Aug 12, 2014 at 09:16:07AM +0800, Huang Shijie wrote: > On Mon, Aug 11, 2014 at 11:43:11AM -0700, Brian Norris wrote: > > On Sat, Aug 09, 2014 at 05:53:03PM +0800, Huang Shijie wrote: > > > On Wed, Aug 06, 2014 at 06:17:00PM -0700, Brian Norris wrote: > > > > We don't need to expose a 'wait-till-ready' interface to drivers. Status > > > > register polling should be handled by the core spi-nor.c library, and as > > > > of now, I see no need to provide a special driver-specific hook for it. > > > > > > Please do not drop this hook, please see why we add this hook: > > > http://lists.infradead.org/pipermail/linux-mtd/2013-December/050561.html > > > > > > " > > > The default implementation would do just as you suggest, and I would > > > expect most H/W drivers to use the default implementation. However, some H/W > > > Controllers can automate the polling of status registers, so having a hook allows > > > the driver override the default behaviour. > > > " > > > > > > The spi-nor framework will used by more drivers besides the m25p80 and > > > fsl-quadspi. Some NOR flash controller may be very strange. > > > > OK, but the wait-till-ready hook should not look like the current one. > > If it is used as an optimization of sorts, then we can provide it in > > *addition* to the checks we do, but not *instead of*. I sincerely doubt > > that any specialized SPI NOR controller will know how to check both SR > > and FSR where appropriate, and it probably won't understand other > > specialized sequences as they are developed / thrown on our lap by flash > > vendors. > > > > So, the spi-nor.c "wait until ready" might have a sequence like this: > > > > 1. (Optionally) use low-level driver's "wait" function; skip if not > > present > > > > 2. use the read register hooks to check SR/FSR to confirm completion > > > > I do not want to toss #2 into the low-level driver, so if there is a > > need for #1, it should be done in addition to our spi-nor.c code, not > > instead. (To this end, I believe Marek also complained about this when > > we were adding the FSR-checking code; we should not have drivers and > > spi-nor.c fighting over callback functions.) > > > > So I'm inclined to at most address #1 through an optional callback, and > > but where to put the optional callback? in the spi_nor{} ? Probably. > > possibly even to ignore that until we see an actual driver that needs > > it. > If you insist to remove it, i will be okay too. > anyway, we can add it back when an actual driver appears. Yeah. The point of this series is that without users, the current callback is both ambiguously defined (it was allegedly serving both a flash-specific and a controller-specific purpose) and unncecessary. It's easier to add back once we have a clear purpose. Brian
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 227e2743203e..874e6d9a0b02 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -193,6 +193,10 @@ static int spi_nor_ready(struct spi_nor *nor) return sr && fsr; } +/* + * Service routine to read status register until ready, or timeout occurs. + * Returns non-zero if error. + */ static int spi_nor_wait_till_ready(struct spi_nor *nor) { unsigned long deadline; @@ -214,15 +218,6 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor) } /* - * Service routine to read status register until ready, or timeout occurs. - * Returns non-zero if error. - */ -static int wait_till_ready(struct spi_nor *nor) -{ - return nor->wait_till_ready(nor); -} - -/* * Erase the whole flash memory * * Returns 0 if successful, non-zero otherwise. @@ -712,7 +707,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, /* write one byte. */ nor->write(nor, to, 1, retlen, buf); - ret = wait_till_ready(nor); + ret = spi_nor_wait_till_ready(nor); if (ret) goto time_out; } @@ -724,7 +719,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, /* write two bytes. */ nor->write(nor, to, 2, retlen, buf + actual); - ret = wait_till_ready(nor); + ret = spi_nor_wait_till_ready(nor); if (ret) goto time_out; to += 2; @@ -733,7 +728,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, nor->sst_write_second = false; write_disable(nor); - ret = wait_till_ready(nor); + ret = spi_nor_wait_till_ready(nor); if (ret) goto time_out; @@ -744,7 +739,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len, nor->program_opcode = SPINOR_OP_BP; nor->write(nor, to, 1, retlen, buf + actual); - ret = wait_till_ready(nor); + ret = spi_nor_wait_till_ready(nor); if (ret) goto time_out; write_disable(nor); @@ -790,7 +785,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, if (page_size > nor->page_size) page_size = nor->page_size; - ret = wait_till_ready(nor); + ret = spi_nor_wait_till_ready(nor); if (ret) goto write_err; @@ -816,7 +811,7 @@ static int macronix_quad_enable(struct spi_nor *nor) nor->cmd_buf[0] = val | SR_QUAD_EN_MX; nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1, 0); - if (wait_till_ready(nor)) + if (spi_nor_wait_till_ready(nor)) return 1; ret = read_sr(nor); @@ -898,8 +893,6 @@ static int spi_nor_check(struct spi_nor *nor) if (!nor->read_id) nor->read_id = spi_nor_read_id; - if (!nor->wait_till_ready) - nor->wait_till_ready = spi_nor_wait_till_ready; return 0; } diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index 603ac0306663..d7c12506d7ed 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -146,7 +146,6 @@ enum spi_nor_option_flags { * @write_reg: [DRIVER-SPECIFIC] write data to the register * @read_id: [REPLACEABLE] read out the ID data, and find * the proper spi_device_id - * @wait_till_ready: [REPLACEABLE] wait till the NOR becomes ready * @read: [DRIVER-SPECIFIC] read data from the SPI NOR * @write: [DRIVER-SPECIFIC] write data to the SPI NOR * @erase: [DRIVER-SPECIFIC] erase a sector of the SPI NOR @@ -179,7 +178,6 @@ struct spi_nor { int (*write_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len, int write_enable); const struct spi_device_id *(*read_id)(struct spi_nor *nor); - int (*wait_till_ready)(struct spi_nor *nor); int (*read)(struct spi_nor *nor, loff_t from, size_t len, size_t *retlen, u_char *read_buf);
We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- drivers/mtd/spi-nor/spi-nor.c | 27 ++++++++++----------------- include/linux/mtd/spi-nor.h | 2 -- 2 files changed, 10 insertions(+), 19 deletions(-)