Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
On Oct 7, 2010, at 9:15 PM, Hu Mingkai-B21284 wrote: Yes, I agree with David on this. If large transfers don't work, then it is the SPI master driver that is buggy. >>> >>> By the way, does this fix your problem? >>> >>> https://patchwork.kernel.org/patch/184752/ >> >> It shouldn't. AFAIK, eSPI is PIO-only controller, and the overrun fix is for >> the >> DMA mode. >> >> Thanks, >> >> p.s. Btw, in patch 3/7, is_dma_mapped argument of fsl_espi_bufs() is >> unneeded. >> > > Yes, the is_dma_mapped isn't needed, I'll remove it. > > Thanks, > Mingkai I'd be really nice if we could close on this patchset in time for .37 acceptance. I'm guessing that cutoff is quickly approaching. - k ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
> -Original Message- > From: Anton Vorontsov [mailto:cbouatmai...@gmail.com] > Sent: Thursday, September 30, 2010 11:07 PM > To: Grant Likely > Cc: David Brownell; linuxppc-...@ozlabs.org; Hu Mingkai-B21284; linux- > m...@lists.infradead.org; Gala Kumar-B11780; spi-devel- > gene...@lists.sourceforge.net > Subject: Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by > page > > On Thu, Sep 30, 2010 at 11:41:40PM +0900, Grant Likely wrote: > > On Thu, Sep 30, 2010 at 11:16 PM, Grant Likely > > wrote: > > > On Thu, Sep 30, 2010 at 7:46 PM, David Brownell > > > wrote: > > >> > > >> --- On Thu, 9/30/10, Mingkai Hu wrote: > > >> > > >>> From: Mingkai Hu > > >>> Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read > > >>> page by page > > >> > > >> NAK. > > >> > > >> We went over this before. > > > > > > Yes, I agree with David on this. If large transfers don't work, > > > then it is the SPI master driver that is buggy. > > > > By the way, does this fix your problem? > > > > https://patchwork.kernel.org/patch/184752/ > > It shouldn't. AFAIK, eSPI is PIO-only controller, and the overrun fix is for > the > DMA mode. > > Thanks, > > p.s. Btw, in patch 3/7, is_dma_mapped argument of fsl_espi_bufs() is unneeded. > Yes, the is_dma_mapped isn't needed, I'll remove it. Thanks, Mingkai ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
> -Original Message- > From: David Brownell [mailto:davi...@pacbell.net] > Sent: Thursday, September 30, 2010 6:46 PM > To: linuxppc-...@ozlabs.org; spi-devel-gene...@lists.sourceforge.net; linux- > m...@lists.infradead.org; Hu Mingkai-B21284 > Cc: Gala Kumar-B11780; Zang Roy-R61911; Hu Mingkai-B21284 > Subject: Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by > page > > > --- On Thu, 9/30/10, Mingkai Hu wrote: > > > From: Mingkai Hu > > Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page > > by page > > NAK. > > We went over this before. > > The bug is in your SPI master controller driver, and the fix there involves > mapping large reads into multiple smaller reads. (Example, 128K read as two > 64K reads instead of one of 128K. > > It's *NEVER* appropriate to commit to patching all upper level drivers in > order > to work around bugs in lower level ones. The set of such upper level drivers > that may need bugfixing is quite large, most will never be used with your > buggy > controller driver, and all such patches will need testing (but the test > resources are probably not available). > > Whatever SPI controller driver you're working with is clearly buggy ... but > not > unfixably so. > > DO NOT head down the path of requiring every SPI device driver to include > workarounds for this odd little SPI master driver bug. > > - Dave > Thanks for your comments, the controller driver is the proper place to handle this, I'll fix it. Thanks, Mingkai ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
Hmmm for some reason the previous replies didn't get picked up by
patchwork, so I'm replying with my comment again for the public
record.
In this case the eSPI controller driver is buggy and needs to be
fixed. If the hardware can only support small transfers, then it is
the responsibilty of the driver to chain up smaller chunks into one
big transfer, and make sure that the CS line doesn't go low in the
middle of it.
g.
On Thu, Sep 30, 2010 at 5:00 PM, Mingkai Hu wrote:
> For Freescale's eSPI controller, the max transaction length one time
> is limitted by the SPCOM[TRANSLEN] field which is 0x. When used
> mkfs.ext2 command to create ext2 filesystem on the flash, the read
> length will exceed the max value of the SPCOM[TRANSLEN] field, so
> change the read function to read page by page.
>
> For other SPI flash driver, also needed to supply the read function
> if used the eSPI controller.
>
> Signed-off-by: Mingkai Hu
> ---
> v3:
> - Add a quirks member for the SPI master to handle the contrains of the
> SPI controller. I can't think of other method. :-(
>
> drivers/mtd/devices/m25p80.c | 78
> ++
> drivers/spi/spi_fsl_lib.c | 4 ++
> include/linux/spi/spi.h | 5 +++
> 3 files changed, 87 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 47d53c7..f65cca8 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -377,6 +377,81 @@ static int m25p80_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> }
>
> /*
> + * Read an address range from the flash chip page by page.
> + * Some controller has transaction length limitation such as the
> + * Freescale's eSPI controller can only trasmit 0x bytes one
> + * time, so we have to read page by page if the len is more than
> + * the limitation.
> + */
> +static int m25p80_page_read(struct mtd_info *mtd, loff_t from, size_t len,
> + size_t *retlen, u_char *buf)
> +{
> + struct m25p *flash = mtd_to_m25p(mtd);
> + struct spi_transfer t[2];
> + struct spi_message m;
> + u32 i, page_size = 0;
> +
> + DEBUG(MTD_DEBUG_LEVEL2, "%s: %s %s 0x%08x, len %zd\n",
> + dev_name(&flash->spi->dev), __func__, "from",
> + (u32)from, len);
> +
> + /* sanity checks */
> + if (!len)
> + return 0;
> +
> + if (from + len > flash->mtd.size)
> + return -EINVAL;
> +
> + spi_message_init(&m);
> + memset(t, 0, (sizeof t));
> +
> + /* NOTE:
> + * OPCODE_FAST_READ (if available) is faster.
> + * Should add 1 byte DUMMY_BYTE.
> + */
> + t[0].tx_buf = flash->command;
> + t[0].len = m25p_cmdsz(flash) + FAST_READ_DUMMY_BYTE;
> + spi_message_add_tail(&t[0], &m);
> +
> + t[1].rx_buf = buf;
> + spi_message_add_tail(&t[1], &m);
> +
> + /* Byte count starts at zero. */
> + if (retlen)
> + *retlen = 0;
> +
> + mutex_lock(&flash->lock);
> +
> + /* Wait till previous write/erase is done. */
> + if (wait_till_ready(flash)) {
> + /* REVISIT status return?? */
> + mutex_unlock(&flash->lock);
> + return 1;
> + }
> +
> + /* Set up the write data buffer. */
> + flash->command[0] = OPCODE_READ;
> +
> + for (i = page_size; i < len; i += page_size) {
> + page_size = len - i;
> + if (page_size > flash->page_size)
> + page_size = flash->page_size;
> + m25p_addr2cmd(flash, from + i, flash->command);
> + t[1].len = page_size;
> + t[1].rx_buf = buf + i;
> +
> + spi_sync(flash->spi, &m);
> +
> + *retlen += m.actual_length - m25p_cmdsz(flash)
> + - FAST_READ_DUMMY_BYTE;
> + }
> +
> + mutex_unlock(&flash->lock);
> +
> + 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.
> @@ -874,6 +949,9 @@ static int __devinit m25p_probe(struct spi_device *spi)
> flash->mtd.erase = m25p80_erase;
> flash->mtd.read = m25p80_read;
>
> + if (spi->master->quirks & SPI_QUIRK_TRANS_LEN_LIMIT)
> + flash->mtd.read = m25p80_page_read;
> +
> /* sst flash chips use AAI word program */
> if (info->jedec_id >> 16 == 0xbf)
> flash->mtd.write = sst_write;
> diff --git a/drivers/spi/spi_fsl_lib.c b/drivers/spi/spi_fsl_lib.c
> index 5cd741f..c8d8c2d 100644
> --- a/drivers/spi/spi_fsl_lib.c
> +++ b/drivers/spi/spi_fsl_lib.c
> @@ -135,6 +135,10 @@ int mpc8xxx_spi_probe(struct device *dev, struct
> resource *mem,
> master->cleanup = mpc8xxx_spi_cleanup;
> master->dev.of_node = dev->of_node;
>
> +
Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
On Fri, Oct 1, 2010 at 12:06 AM, Anton Vorontsov wrote: > On Thu, Sep 30, 2010 at 11:41:40PM +0900, Grant Likely wrote: >> On Thu, Sep 30, 2010 at 11:16 PM, Grant Likely >> wrote: >> > On Thu, Sep 30, 2010 at 7:46 PM, David Brownell >> > wrote: >> >> >> >> --- On Thu, 9/30/10, Mingkai Hu wrote: >> >> >> >>> From: Mingkai Hu >> >>> Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by >> >>> page >> >> >> >> NAK. >> >> >> >> We went over this before. >> > >> > Yes, I agree with David on this. If large transfers don't work, then >> > it is the SPI master driver that is buggy. >> >> By the way, does this fix your problem? >> >> https://patchwork.kernel.org/patch/184752/ > > It shouldn't. AFAIK, eSPI is PIO-only controller, and the overrun > fix is for the DMA mode. > > Thanks, > > p.s. Btw, in patch 3/7, is_dma_mapped argument of fsl_espi_bufs() > is unneeded. Thanks Anton. Please reply to that patch with this comment so that patchwork records it and I don't forget about it. Thanks, g. > > -- > Anton Vorontsov > email: cbouatmai...@gmail.com > irc://irc.freenode.net/bd2 > -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
On Thu, Sep 30, 2010 at 11:41:40PM +0900, Grant Likely wrote: > On Thu, Sep 30, 2010 at 11:16 PM, Grant Likely > wrote: > > On Thu, Sep 30, 2010 at 7:46 PM, David Brownell wrote: > >> > >> --- On Thu, 9/30/10, Mingkai Hu wrote: > >> > >>> From: Mingkai Hu > >>> Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by > >>> page > >> > >> NAK. > >> > >> We went over this before. > > > > Yes, I agree with David on this. If large transfers don't work, then > > it is the SPI master driver that is buggy. > > By the way, does this fix your problem? > > https://patchwork.kernel.org/patch/184752/ It shouldn't. AFAIK, eSPI is PIO-only controller, and the overrun fix is for the DMA mode. Thanks, p.s. Btw, in patch 3/7, is_dma_mapped argument of fsl_espi_bufs() is unneeded. -- Anton Vorontsov email: cbouatmai...@gmail.com irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
On Thu, Sep 30, 2010 at 11:16 PM, Grant Likely wrote: > On Thu, Sep 30, 2010 at 7:46 PM, David Brownell wrote: >> >> --- On Thu, 9/30/10, Mingkai Hu wrote: >> >>> From: Mingkai Hu >>> Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by >>> page >> >> NAK. >> >> We went over this before. > > Yes, I agree with David on this. If large transfers don't work, then > it is the SPI master driver that is buggy. By the way, does this fix your problem? https://patchwork.kernel.org/patch/184752/ g. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
On Thu, Sep 30, 2010 at 7:46 PM, David Brownell wrote: > > --- On Thu, 9/30/10, Mingkai Hu wrote: > >> From: Mingkai Hu >> Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page > > NAK. > > We went over this before. Yes, I agree with David on this. If large transfers don't work, then it is the SPI master driver that is buggy. g. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
--- On Thu, 9/30/10, Mingkai Hu wrote: > From: Mingkai Hu > Subject: [PATCH v3 6/7] mtd: m25p80: add a read function to read page by page NAK. We went over this before. The bug is in your SPI master controller driver, and the fix there involves mapping large reads into multiple smaller reads. (Example, 128K read as two 64K reads instead of one of 128K. It's *NEVER* appropriate to commit to patching all upper level drivers in order to work around bugs in lower level ones. The set of such upper level drivers that may need bugfixing is quite large, most will never be used with your buggy controller driver, and all such patches will need testing (but the test resources are probably not available). Whatever SPI controller driver you're working with is clearly buggy ... but not unfixably so. DO NOT head down the path of requiring every SPI device driver to include workarounds for this odd little SPI master driver bug. - Dave ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH v3 6/7] mtd: m25p80: add a read function to read page by page
For Freescale's eSPI controller, the max transaction length one time
is limitted by the SPCOM[TRANSLEN] field which is 0x. When used
mkfs.ext2 command to create ext2 filesystem on the flash, the read
length will exceed the max value of the SPCOM[TRANSLEN] field, so
change the read function to read page by page.
For other SPI flash driver, also needed to supply the read function
if used the eSPI controller.
Signed-off-by: Mingkai Hu
---
v3:
- Add a quirks member for the SPI master to handle the contrains of the
SPI controller. I can't think of other method. :-(
drivers/mtd/devices/m25p80.c | 78 ++
drivers/spi/spi_fsl_lib.c|4 ++
include/linux/spi/spi.h |5 +++
3 files changed, 87 insertions(+), 0 deletions(-)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 47d53c7..f65cca8 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -377,6 +377,81 @@ static int m25p80_read(struct mtd_info *mtd, loff_t from,
size_t len,
}
/*
+ * Read an address range from the flash chip page by page.
+ * Some controller has transaction length limitation such as the
+ * Freescale's eSPI controller can only trasmit 0x bytes one
+ * time, so we have to read page by page if the len is more than
+ * the limitation.
+ */
+static int m25p80_page_read(struct mtd_info *mtd, loff_t from, size_t len,
+ size_t *retlen, u_char *buf)
+{
+ struct m25p *flash = mtd_to_m25p(mtd);
+ struct spi_transfer t[2];
+ struct spi_message m;
+ u32 i, page_size = 0;
+
+ DEBUG(MTD_DEBUG_LEVEL2, "%s: %s %s 0x%08x, len %zd\n",
+ dev_name(&flash->spi->dev), __func__, "from",
+ (u32)from, len);
+
+ /* sanity checks */
+ if (!len)
+ return 0;
+
+ if (from + len > flash->mtd.size)
+ return -EINVAL;
+
+ spi_message_init(&m);
+ memset(t, 0, (sizeof t));
+
+ /* NOTE:
+* OPCODE_FAST_READ (if available) is faster.
+* Should add 1 byte DUMMY_BYTE.
+*/
+ t[0].tx_buf = flash->command;
+ t[0].len = m25p_cmdsz(flash) + FAST_READ_DUMMY_BYTE;
+ spi_message_add_tail(&t[0], &m);
+
+ t[1].rx_buf = buf;
+ spi_message_add_tail(&t[1], &m);
+
+ /* Byte count starts at zero. */
+ if (retlen)
+ *retlen = 0;
+
+ mutex_lock(&flash->lock);
+
+ /* Wait till previous write/erase is done. */
+ if (wait_till_ready(flash)) {
+ /* REVISIT status return?? */
+ mutex_unlock(&flash->lock);
+ return 1;
+ }
+
+ /* Set up the write data buffer. */
+ flash->command[0] = OPCODE_READ;
+
+ for (i = page_size; i < len; i += page_size) {
+ page_size = len - i;
+ if (page_size > flash->page_size)
+ page_size = flash->page_size;
+ m25p_addr2cmd(flash, from + i, flash->command);
+ t[1].len = page_size;
+ t[1].rx_buf = buf + i;
+
+ spi_sync(flash->spi, &m);
+
+ *retlen += m.actual_length - m25p_cmdsz(flash)
+ - FAST_READ_DUMMY_BYTE;
+ }
+
+ mutex_unlock(&flash->lock);
+
+ 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.
@@ -874,6 +949,9 @@ static int __devinit m25p_probe(struct spi_device *spi)
flash->mtd.erase = m25p80_erase;
flash->mtd.read = m25p80_read;
+ if (spi->master->quirks & SPI_QUIRK_TRANS_LEN_LIMIT)
+ flash->mtd.read = m25p80_page_read;
+
/* sst flash chips use AAI word program */
if (info->jedec_id >> 16 == 0xbf)
flash->mtd.write = sst_write;
diff --git a/drivers/spi/spi_fsl_lib.c b/drivers/spi/spi_fsl_lib.c
index 5cd741f..c8d8c2d 100644
--- a/drivers/spi/spi_fsl_lib.c
+++ b/drivers/spi/spi_fsl_lib.c
@@ -135,6 +135,10 @@ int mpc8xxx_spi_probe(struct device *dev, struct resource
*mem,
master->cleanup = mpc8xxx_spi_cleanup;
master->dev.of_node = dev->of_node;
+ if (of_get_property(dev->of_node,
+ "fsl,spi-quirk-trans-len-limit", NULL))
+ master->quirks |= SPI_QUIRK_TRANS_LEN_LIMIT;
+
mpc8xxx_spi = spi_master_get_devdata(master);
mpc8xxx_spi->dev = dev;
mpc8xxx_spi->get_rx = mpc8xxx_spi_rx_buf_u8;
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 92e52a1..4234dfd 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -304,6 +304,11 @@ struct spi_master {
/* called on release() to free memory provided by spi_master */
void(*cleanup)(struct spi_device *spi);
+
+ /* some constraints of the controller */
+ u16
