| Message ID | 1437479828-30826-1-git-send-email-B48286@freescale.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Shijie and Brian, Do you have any comment? > -----Original Message----- > From: Zhiqiang Hou [mailto:B48286@freescale.com] > Sent: 2015年7月21日 19:57 > To: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > dwmw2@infradead.org > Cc: Hu Mingkai-B21284; Hou Zhiqiang-B48286 > Subject: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read > > From: Hou Zhiqiang <B48286@freescale.com> > > Compare the var retlen with len to ensure the read operation successful. > > There are some SPI controllers that have a maximum transfer length, e.g. > Freescale eSPI controller is able to transfer 0x10000 Bytes each time. > If the spi_nor_read diliver a transaction length that excced the ability > of the SPI controller, there should be multiple read operation to make > sure this transaction completion. > > Signed-off-by: Hou Zhiqiang <B48286@freescale.com> > --- > drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi- > nor.c index fbc5035..ec10cd1 100644 > --- a/drivers/mtd/spi-nor/spi-nor.c > +++ b/drivers/mtd/spi-nor/spi-nor.c > @@ -715,6 +715,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t > from, size_t len, { > struct spi_nor *nor = mtd_to_spi_nor(mtd); > int ret; > + size_t cnt = 0; > + u_char *cur = buf; > > dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > > @@ -722,8 +724,16 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t > from, size_t len, > if (ret) > return ret; > > - ret = nor->read(nor, from, len, retlen, buf); > - > + *retlen = 0; > + while (len > 0) { > + ret = nor->read(nor, from, len, &cnt, cur); > + if (ret < 0) > + break; > + len -= cnt; > + cur += cnt; > + *retlen += cnt; > + from += cnt; > + } > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ); > return ret; > } > -- > 2.1.0.27.g96db324 Thanks, Zhiqiang
Hello, On 31 July 2015 at 12:35, Hou Zhiqiang <B48286@freescale.com> wrote: > Hi Shijie and Brian, > > Do you have any comment? > >> -----Original Message----- >> From: Zhiqiang Hou [mailto:B48286@freescale.com] >> Sent: 2015年7月21日 19:57 >> To: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; >> dwmw2@infradead.org >> Cc: Hu Mingkai-B21284; Hou Zhiqiang-B48286 >> Subject: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read >> >> From: Hou Zhiqiang <B48286@freescale.com> >> >> Compare the var retlen with len to ensure the read operation successful. >> >> There are some SPI controllers that have a maximum transfer length, e.g. >> Freescale eSPI controller is able to transfer 0x10000 Bytes each time. >> If the spi_nor_read diliver a transaction length that excced the ability >> of the SPI controller, there should be multiple read operation to make >> sure this transaction completion. >> >> Signed-off-by: Hou Zhiqiang <B48286@freescale.com> >> --- >> drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++-- >> 1 file changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi- >> nor.c index fbc5035..ec10cd1 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -715,6 +715,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t >> from, size_t len, { >> struct spi_nor *nor = mtd_to_spi_nor(mtd); >> int ret; >> + size_t cnt = 0; >> + u_char *cur = buf; >> >> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); >> >> @@ -722,8 +724,16 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t >> from, size_t len, >> if (ret) >> return ret; >> >> - ret = nor->read(nor, from, len, retlen, buf); >> - >> + *retlen = 0; >> + while (len > 0) { >> + ret = nor->read(nor, from, len, &cnt, cur); >> + if (ret < 0) >> + break; >> + len -= cnt; >> + cur += cnt; >> + *retlen += cnt; >> + from += cnt; >> + } >> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ); >> return ret; >> } >> -- >> 2.1.0.27.g96db324 > > Thanks, > Zhiqiang > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ It appears that nor->read() always returns 0. Also see https://www.mail-archive.com/linux-kernel%40vger.kernel.org/msg942729.html Thanks Michal
On 31 July 2015 at 14:13, Michal Suchanek <hramrach@gmail.com> wrote: > Hello, > > On 31 July 2015 at 12:35, Hou Zhiqiang <B48286@freescale.com> wrote: >> Hi Shijie and Brian, >> >> Do you have any comment? >> >>> -----Original Message----- >>> From: Zhiqiang Hou [mailto:B48286@freescale.com] >>> Sent: 2015年7月21日 19:57 >>> To: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; >>> dwmw2@infradead.org >>> Cc: Hu Mingkai-B21284; Hou Zhiqiang-B48286 >>> Subject: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read >>> >>> From: Hou Zhiqiang <B48286@freescale.com> >>> >>> Compare the var retlen with len to ensure the read operation successful. >>> >>> There are some SPI controllers that have a maximum transfer length, e.g. >>> Freescale eSPI controller is able to transfer 0x10000 Bytes each time. >>> If the spi_nor_read diliver a transaction length that excced the ability >>> of the SPI controller, there should be multiple read operation to make >>> sure this transaction completion. >>> >>> Signed-off-by: Hou Zhiqiang <B48286@freescale.com> >>> --- >>> drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++-- >>> 1 file changed, 12 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi- >>> nor.c index fbc5035..ec10cd1 100644 >>> --- a/drivers/mtd/spi-nor/spi-nor.c >>> +++ b/drivers/mtd/spi-nor/spi-nor.c >>> @@ -715,6 +715,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t >>> from, size_t len, { >>> struct spi_nor *nor = mtd_to_spi_nor(mtd); >>> int ret; >>> + size_t cnt = 0; >>> + u_char *cur = buf; >>> >>> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); >>> >>> @@ -722,8 +724,16 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t >>> from, size_t len, >>> if (ret) >>> return ret; >>> >>> - ret = nor->read(nor, from, len, retlen, buf); >>> - >>> + *retlen = 0; >>> + while (len > 0) { >>> + ret = nor->read(nor, from, len, &cnt, cur); >>> + if (ret < 0) >>> + break; >>> + len -= cnt; >>> + cur += cnt; >>> + *retlen += cnt; >>> + from += cnt; >>> + } >>> spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ); >>> return ret; >>> } >>> -- >>> 2.1.0.27.g96db324 >> >> Thanks, >> Zhiqiang >> ______________________________________________________ >> Linux MTD discussion mailing list >> http://lists.infradead.org/mailman/listinfo/linux-mtd/ > > > It appears that nor->read() always returns 0. > > Also see https://www.mail-archive.com/linux-kernel%40vger.kernel.org/msg942729.html > Sorry, there is some e-mail thread connectivity problem. This is a link to the actual patch http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html
Hello, I want to add the 4-Byte addressing flashes support for the Freescale eSPI controller dirver. Appreciate your suggestions. Background: In the Freescale eSPI controller dirver, if the transaction length exceed 64KiB, the contents of the transaction was touched to update the command with assumed the 3-Byte address width. It is a workaround, due to the Freescale eSPI controller have a hardware limit that the maximum one-time transaction length is 64KiB, that isn't consistent with many other vendors' SPI controllers, and so far the SPI flash driver assume the SPI controller have the ability to complete the specified transaction length at one-time. But this workaround is only suit for 3-Byte addressing slaves, as time goes by, there are 4-Byte address width SPI flashes, so this assumption isn't correct and this workaround discombobulate the controller driver layer and protocol driver layer. So, this workaround should be removed and the SPI client driver should ensure the transaction completion. There are two solutions: 1. In spi-nor framework, compare the retlen with the transaction length specified, if the retlen less than the transaction length and with no error number returned, re-initiate the transaction with the updated address. Advantage: It won't affect other SPI controllers without this limit. Disadvantage: It is not a systematic solution. 2. Add quirk support, something like i2c quirk. Advantage: It is a systematic solution. Disadvantage: The quirk mechanism is only useful for Freescale eSPI controller, but it will affect whole SPI framework. > -----Original Message----- > From: Hou Zhiqiang-B48286 > Sent: 2015年7月31日 18:36 > To: Hou Zhiqiang-B48286; linux-mtd@lists.infradead.org; > computersforpeace@gmail.com; dwmw2@infradead.org; shijie.huang@intel.com > Cc: Hu Mingkai-B21284 > Subject: RE: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read > > Hi Shijie and Brian, > > Do you have any comment? > > > -----Original Message----- > > From: Zhiqiang Hou [mailto:B48286@freescale.com] > > Sent: 2015年7月21日 19:57 > > To: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > > dwmw2@infradead.org > > Cc: Hu Mingkai-B21284; Hou Zhiqiang-B48286 > > Subject: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read > > > > From: Hou Zhiqiang <B48286@freescale.com> > > > > Compare the var retlen with len to ensure the read operation successful. > > > > There are some SPI controllers that have a maximum transfer length, e.g. > > Freescale eSPI controller is able to transfer 0x10000 Bytes each time. > > If the spi_nor_read diliver a transaction length that excced the > > ability of the SPI controller, there should be multiple read operation > > to make sure this transaction completion. > > > > Signed-off-by: Hou Zhiqiang <B48286@freescale.com> > > --- > > drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++-- > > 1 file changed, 12 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi- > > nor.c index fbc5035..ec10cd1 100644 > > --- a/drivers/mtd/spi-nor/spi-nor.c > > +++ b/drivers/mtd/spi-nor/spi-nor.c > > @@ -715,6 +715,8 @@ static int spi_nor_read(struct mtd_info *mtd, > > loff_t from, size_t len, { > > struct spi_nor *nor = mtd_to_spi_nor(mtd); > > int ret; > > + size_t cnt = 0; > > + u_char *cur = buf; > > > > dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > > > > @@ -722,8 +724,16 @@ static int spi_nor_read(struct mtd_info *mtd, > > loff_t from, size_t len, > > if (ret) > > return ret; > > > > - ret = nor->read(nor, from, len, retlen, buf); > > - > > + *retlen = 0; > > + while (len > 0) { > > + ret = nor->read(nor, from, len, &cnt, cur); > > + if (ret < 0) > > + break; > > + len -= cnt; > > + cur += cnt; > > + *retlen += cnt; > > + from += cnt; > > + } > > spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ); > > return ret; > > } > > -- > > 2.1.0.27.g96db324 > > Thanks, > Zhiqiang Thanks, Zhiqiang
On 13 August 2015 at 11:03, Hou Zhiqiang <B48286@freescale.com> wrote: > Hello, > > I want to add the 4-Byte addressing flashes support for the Freescale eSPI > controller dirver. Appreciate your suggestions. > > Background: > In the Freescale eSPI controller dirver, if the transaction length exceed > 64KiB, the contents of the transaction was touched to update the command > with assumed the 3-Byte address width. It is a workaround, due to the > Freescale eSPI controller have a hardware limit that the maximum one-time > transaction length is 64KiB, that isn't consistent with many other vendors' > SPI controllers, and so far the SPI flash driver assume the SPI controller > have the ability to complete the specified transaction length at one-time. > > But this workaround is only suit for 3-Byte addressing slaves, as time goes > by, there are 4-Byte address width SPI flashes, so this assumption isn't > correct and this workaround discombobulate the controller driver layer and > protocol driver layer. So, this workaround should be removed and the SPI > client driver should ensure the transaction completion. > > There are two solutions: > 1. In spi-nor framework, compare the retlen with the transaction length > specified, if the retlen less than the transaction length and with no error > number returned, re-initiate the transaction with the updated address. > Advantage: > It won't affect other SPI controllers without this limit. > Disadvantage: > It is not a systematic solution. This can probably break transactions that are performed on non-flash slaves but look like flash read command. > > 2. Add quirk support, something like i2c quirk. > Advantage: > It is a systematic solution. > Disadvantage: > The quirk mechanism is only useful for Freescale eSPI controller, but it will > affect whole SPI framework. I added this quirk in m25p80.c since my current SPI master driver is just broken so I tried to work around that. While the quirk itself is rejected as non-syetematic this series that adds the required support for checking the return value of the SPI master driver transfer functions. So you have options: 2a: add SPI master quirk and check it in m25p80.c and truncate the transfer in m25p80.c 2b: truncate the transfer in SPI master driver and just return the number of bytes transferred This patch series should handle fragmenting the transfers in spi-nor.c http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html 3: if your controller has manual CS driving support fragment the transfer in your SPI master driver into chunks the controler can transfer in one go. Thanks Michal
Hi, Is it convenient to tell me that which vendor's spi nor? For current spi nor structure, write operation has been already split transaction length By page size(256 Bytes). But seems "retlen" not reasonable. Spi nor read operation indeed does not take spi controller's transaction ability into Account.but for spi nor, one-time read length is non-restrictive. --Beanhuo
On 13 August 2015 at 17:38, Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote: > > Hi, > Is it convenient to tell me that which vendor's spi nor? > For current spi nor structure, write operation has been already split transaction length > By page size(256 Bytes). But seems "retlen" not reasonable. > I was working with SPI controller which had limit of 63 bytes per transfer which is smaller than the page size. This limitation was due to driver deficiency. Adding the limit to m25p80 allowed full access to the flash content, though. Thanks Michal
> -----Original Message----- > From: Bean Huo 霍斌斌 (beanhuo) [mailto:beanhuo@micron.com] > Sent: 2015年8月13日 23:39 > To: Hou Zhiqiang-B48286; linux-mtd@lists.infradead.org; > computersforpeace@gmail.com; dwmw2@infradead.org; > jonatas.rech@datacom.ind.br; Jane.Wan@gainspeed.com; > shijie.huang@intel.com; valentin.longchamp@keymile.com; > hkallweit1@gmail.com; hramrach@gmail.com > Cc: Hu Mingkai-B21284 > Subject: RE: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read > > > Hi, > Is it convenient to tell me that which vendor's spi nor? > For current spi nor structure, write operation has been already split > transaction length By page size(256 Bytes). But seems "retlen" not > reasonable. > > Spi nor read operation indeed does not take spi controller's transaction > ability into Account.but for spi nor, one-time read length is non- > restrictive. > I'm sorry for my unclear explanation. Yes, for SPI NOR Flash, the one-time read length is non-restrictive, but for the SPI Controller there are some limits, e.g. 64KiB one-time transfer length limit for Freescale eSPI controller, and as you known the spi nor read operation indeed does not take spi controller's transaction ability into account. > --Beanhuo B.R Zhiqiang
> -----Original Message----- > From: Michal Suchanek [mailto:hramrach@gmail.com] > Sent: 2015年8月13日 17:42 > To: Hou Zhiqiang-B48286 > Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > dwmw2@infradead.org; jonatas.rech@datacom.ind.br; Jane.Wan@gainspeed.com; > shijie.huang@intel.com; valentin.longchamp@keymile.com; > hkallweit1@gmail.com; beanhuo@micron.com; Hu Mingkai-B21284 > Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read > > On 13 August 2015 at 11:03, Hou Zhiqiang <B48286@freescale.com> wrote: > > Hello, > > > > I want to add the 4-Byte addressing flashes support for the Freescale > > eSPI controller dirver. Appreciate your suggestions. > > > > Background: > > In the Freescale eSPI controller dirver, if the transaction length > > exceed 64KiB, the contents of the transaction was touched to update > > the command with assumed the 3-Byte address width. It is a workaround, > > due to the Freescale eSPI controller have a hardware limit that the > > maximum one-time transaction length is 64KiB, that isn't consistent > with many other vendors' > > SPI controllers, and so far the SPI flash driver assume the SPI > > controller have the ability to complete the specified transaction > length at one-time. > > > > But this workaround is only suit for 3-Byte addressing slaves, as time > > goes by, there are 4-Byte address width SPI flashes, so this > > assumption isn't correct and this workaround discombobulate the > > controller driver layer and protocol driver layer. So, this workaround > > should be removed and the SPI client driver should ensure the > transaction completion. > > > > There are two solutions: > > 1. In spi-nor framework, compare the retlen with the transaction > > length specified, if the retlen less than the transaction length and > > with no error number returned, re-initiate the transaction with the > updated address. > > Advantage: > > It won't affect other SPI controllers without this limit. > > Disadvantage: > > It is not a systematic solution. > > This can probably break transactions that are performed on non-flash > slaves but look like flash read command. > > > > > 2. Add quirk support, something like i2c quirk. > > Advantage: > > It is a systematic solution. > > Disadvantage: > > The quirk mechanism is only useful for Freescale eSPI controller, but > > it will affect whole SPI framework. > > I added this quirk in m25p80.c since my current SPI master driver is just > broken so I tried to work around that. While the quirk itself is rejected > as non-syetematic this series that adds the required support for checking > the return value of the SPI master driver transfer functions. > > So you have options: > > 2a: add SPI master quirk and check it in m25p80.c and truncate the > transfer in m25p80.c > > 2b: truncate the transfer in SPI master driver and just return the number > of bytes transferred > I expect this way, so the spi slave driver should check the retlen with the transaction length and do some loop transfer, and make sure getting all data this transaction wanted. > This patch series should handle fragmenting the transfers in spi-nor.c > > http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html > Please correct me if I'm wrong, AFAIU the mtd layer will check the retlen and if it isn't equal to the transaction length, the EIO will be returned. So, I'm afraid this patch series can't handle the fragmenting the transfers. Because it cannot ensure all the data the this transaction specified has been read back. > 3: if your controller has manual CS driving support fragment the transfer > in your SPI master driver into chunks the controler can transfer in one > go. > > Thanks > > Michal
On 14 August 2015 at 05:40, Hou Zhiqiang <B48286@freescale.com> wrote: > > >> -----Original Message----- >> From: Michal Suchanek [mailto:hramrach@gmail.com] >> Sent: 2015年8月13日 17:42 >> To: Hou Zhiqiang-B48286 >> Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; >> dwmw2@infradead.org; jonatas.rech@datacom.ind.br; Jane.Wan@gainspeed.com; >> shijie.huang@intel.com; valentin.longchamp@keymile.com; >> hkallweit1@gmail.com; beanhuo@micron.com; Hu Mingkai-B21284 >> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read >> >> On 13 August 2015 at 11:03, Hou Zhiqiang <B48286@freescale.com> wrote: >> > Hello, >> > >> > I want to add the 4-Byte addressing flashes support for the Freescale >> > eSPI controller dirver. Appreciate your suggestions. >> > >> > Background: >> > In the Freescale eSPI controller dirver, if the transaction length >> > exceed 64KiB, the contents of the transaction was touched to update >> > the command with assumed the 3-Byte address width. It is a workaround, >> > due to the Freescale eSPI controller have a hardware limit that the >> > maximum one-time transaction length is 64KiB, that isn't consistent >> with many other vendors' >> > SPI controllers, and so far the SPI flash driver assume the SPI >> > controller have the ability to complete the specified transaction >> length at one-time. >> > >> > But this workaround is only suit for 3-Byte addressing slaves, as time >> > goes by, there are 4-Byte address width SPI flashes, so this >> > assumption isn't correct and this workaround discombobulate the >> > controller driver layer and protocol driver layer. So, this workaround >> > should be removed and the SPI client driver should ensure the >> transaction completion. >> > >> > There are two solutions: >> > 1. In spi-nor framework, compare the retlen with the transaction >> > length specified, if the retlen less than the transaction length and >> > with no error number returned, re-initiate the transaction with the >> updated address. >> > Advantage: >> > It won't affect other SPI controllers without this limit. >> > Disadvantage: >> > It is not a systematic solution. >> >> This can probably break transactions that are performed on non-flash >> slaves but look like flash read command. >> >> > >> > 2. Add quirk support, something like i2c quirk. >> > Advantage: >> > It is a systematic solution. >> > Disadvantage: >> > The quirk mechanism is only useful for Freescale eSPI controller, but >> > it will affect whole SPI framework. >> >> I added this quirk in m25p80.c since my current SPI master driver is just >> broken so I tried to work around that. While the quirk itself is rejected >> as non-syetematic this series that adds the required support for checking >> the return value of the SPI master driver transfer functions. >> >> So you have options: >> >> 2a: add SPI master quirk and check it in m25p80.c and truncate the >> transfer in m25p80.c >> >> 2b: truncate the transfer in SPI master driver and just return the number >> of bytes transferred >> > > I expect this way, so the spi slave driver should check the retlen with the > transaction length and do some loop transfer, and make sure getting all data > this transaction wanted. > >> This patch series should handle fragmenting the transfers in spi-nor.c >> >> http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html >> > > Please correct me if I'm wrong, AFAIU the mtd layer will check the retlen and > if it isn't equal to the transaction length, the EIO will be returned. I never had this happen because with this patch series spi-nor checks the amount of data transferred and continues until the whole length is transferred. Without it just sets retlen to length regardless of amount of data transferred. Thanks Michal
On 14 August 2015 at 06:30, Michal Suchanek <hramrach@gmail.com> wrote: > On 14 August 2015 at 05:40, Hou Zhiqiang <B48286@freescale.com> wrote: >> >> >>> -----Original Message----- >>> From: Michal Suchanek [mailto:hramrach@gmail.com] >>> Sent: 2015年8月13日 17:42 >>> To: Hou Zhiqiang-B48286 >>> Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; >>> dwmw2@infradead.org; jonatas.rech@datacom.ind.br; Jane.Wan@gainspeed.com; >>> shijie.huang@intel.com; valentin.longchamp@keymile.com; >>> hkallweit1@gmail.com; beanhuo@micron.com; Hu Mingkai-B21284 >>> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read >>> >>> On 13 August 2015 at 11:03, Hou Zhiqiang <B48286@freescale.com> wrote: >>> > Hello, >>> > >>> > I want to add the 4-Byte addressing flashes support for the Freescale >>> > eSPI controller dirver. Appreciate your suggestions. >>> > >>> > Background: >>> > In the Freescale eSPI controller dirver, if the transaction length >>> > exceed 64KiB, the contents of the transaction was touched to update >>> > the command with assumed the 3-Byte address width. It is a workaround, >>> > due to the Freescale eSPI controller have a hardware limit that the >>> > maximum one-time transaction length is 64KiB, that isn't consistent >>> with many other vendors' >>> > SPI controllers, and so far the SPI flash driver assume the SPI >>> > controller have the ability to complete the specified transaction >>> length at one-time. >>> > >>> > But this workaround is only suit for 3-Byte addressing slaves, as time >>> > goes by, there are 4-Byte address width SPI flashes, so this >>> > assumption isn't correct and this workaround discombobulate the >>> > controller driver layer and protocol driver layer. So, this workaround >>> > should be removed and the SPI client driver should ensure the >>> transaction completion. >>> > >>> > There are two solutions: >>> > 1. In spi-nor framework, compare the retlen with the transaction >>> > length specified, if the retlen less than the transaction length and >>> > with no error number returned, re-initiate the transaction with the >>> updated address. >>> > Advantage: >>> > It won't affect other SPI controllers without this limit. >>> > Disadvantage: >>> > It is not a systematic solution. >>> >>> This can probably break transactions that are performed on non-flash >>> slaves but look like flash read command. >>> >>> > >>> > 2. Add quirk support, something like i2c quirk. >>> > Advantage: >>> > It is a systematic solution. >>> > Disadvantage: >>> > The quirk mechanism is only useful for Freescale eSPI controller, but >>> > it will affect whole SPI framework. >>> >>> I added this quirk in m25p80.c since my current SPI master driver is just >>> broken so I tried to work around that. While the quirk itself is rejected >>> as non-syetematic this series that adds the required support for checking >>> the return value of the SPI master driver transfer functions. >>> >>> So you have options: >>> >>> 2a: add SPI master quirk and check it in m25p80.c and truncate the >>> transfer in m25p80.c >>> >>> 2b: truncate the transfer in SPI master driver and just return the number >>> of bytes transferred >>> >> >> I expect this way, so the spi slave driver should check the retlen with the >> transaction length and do some loop transfer, and make sure getting all data >> this transaction wanted. >> >>> This patch series should handle fragmenting the transfers in spi-nor.c >>> >>> http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html >>> >> >> Please correct me if I'm wrong, AFAIU the mtd layer will check the retlen and >> if it isn't equal to the transaction length, the EIO will be returned. > > I never had this happen because with this patch series spi-nor checks > the amount of data transferred and continues until the whole length is > transferred. Without it just sets retlen to length regardless of > amount of data transferred. > Looking at the code closely mtdcore just invokes _read in mtd_read and passes the result on. mtdchar does the same. ubi has assert that read amount of data is equal requested so if you used ubi you whould have to fix it or add read loop in spi-nor to work around ubi deficiency. Thanks Michal
Hi Michal, > -----Original Message----- > From: Michal Suchanek [mailto:hramrach@gmail.com] > Sent: 2015年8月14日 12:43 > To: Hou Zhiqiang-B48286 > Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > dwmw2@infradead.org; jonatas.rech@datacom.ind.br; Jane.Wan@gainspeed.com; > shijie.huang@intel.com; valentin.longchamp@keymile.com; > hkallweit1@gmail.com; beanhuo@micron.com; Hu Mingkai-B21284 > Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read > > On 14 August 2015 at 06:30, Michal Suchanek <hramrach@gmail.com> wrote: > > On 14 August 2015 at 05:40, Hou Zhiqiang <B48286@freescale.com> wrote: > >> > >> > >>> -----Original Message----- > >>> From: Michal Suchanek [mailto:hramrach@gmail.com] > >>> Sent: 2015年8月13日 17:42 > >>> To: Hou Zhiqiang-B48286 > >>> Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > >>> dwmw2@infradead.org; jonatas.rech@datacom.ind.br; > >>> Jane.Wan@gainspeed.com; shijie.huang@intel.com; > >>> valentin.longchamp@keymile.com; hkallweit1@gmail.com; > >>> beanhuo@micron.com; Hu Mingkai-B21284 > >>> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the > >>> spi_nor_read > >>> > >>> On 13 August 2015 at 11:03, Hou Zhiqiang <B48286@freescale.com> wrote: > >>> > Hello, > >>> > > >>> > I want to add the 4-Byte addressing flashes support for the > >>> > Freescale eSPI controller dirver. Appreciate your suggestions. > >>> > > >>> > Background: > >>> > In the Freescale eSPI controller dirver, if the transaction length > >>> > exceed 64KiB, the contents of the transaction was touched to > >>> > update the command with assumed the 3-Byte address width. It is a > >>> > workaround, due to the Freescale eSPI controller have a hardware > >>> > limit that the maximum one-time transaction length is 64KiB, that > >>> > isn't consistent > >>> with many other vendors' > >>> > SPI controllers, and so far the SPI flash driver assume the SPI > >>> > controller have the ability to complete the specified transaction > >>> length at one-time. > >>> > > >>> > But this workaround is only suit for 3-Byte addressing slaves, as > >>> > time goes by, there are 4-Byte address width SPI flashes, so this > >>> > assumption isn't correct and this workaround discombobulate the > >>> > controller driver layer and protocol driver layer. So, this > >>> > workaround should be removed and the SPI client driver should > >>> > ensure the > >>> transaction completion. > >>> > > >>> > There are two solutions: > >>> > 1. In spi-nor framework, compare the retlen with the transaction > >>> > length specified, if the retlen less than the transaction length > >>> > and with no error number returned, re-initiate the transaction > >>> > with the > >>> updated address. > >>> > Advantage: > >>> > It won't affect other SPI controllers without this limit. > >>> > Disadvantage: > >>> > It is not a systematic solution. > >>> > >>> This can probably break transactions that are performed on non-flash > >>> slaves but look like flash read command. > >>> > >>> > > >>> > 2. Add quirk support, something like i2c quirk. > >>> > Advantage: > >>> > It is a systematic solution. > >>> > Disadvantage: > >>> > The quirk mechanism is only useful for Freescale eSPI controller, > >>> > but it will affect whole SPI framework. > >>> > >>> I added this quirk in m25p80.c since my current SPI master driver is > >>> just broken so I tried to work around that. While the quirk itself > >>> is rejected as non-syetematic this series that adds the required > >>> support for checking the return value of the SPI master driver > transfer functions. > >>> > >>> So you have options: > >>> > >>> 2a: add SPI master quirk and check it in m25p80.c and truncate the > >>> transfer in m25p80.c > >>> > >>> 2b: truncate the transfer in SPI master driver and just return the > >>> number of bytes transferred > >>> > >> > >> I expect this way, so the spi slave driver should check the retlen > >> with the transaction length and do some loop transfer, and make sure > >> getting all data this transaction wanted. > >> > >>> This patch series should handle fragmenting the transfers in > >>> spi-nor.c > >>> > >>> http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html > >>> > >> > >> Please correct me if I'm wrong, AFAIU the mtd layer will check the > >> retlen and if it isn't equal to the transaction length, the EIO will > be returned. > > > > I never had this happen because with this patch series spi-nor checks > > the amount of data transferred and continues until the whole length is > > transferred. Without it just sets retlen to length regardless of > > amount of data transferred. > > > > Looking at the code closely mtdcore just invokes _read in mtd_read and > passes the result on. mtdchar does the same. ubi has assert that read > amount of data is equal requested so if you used ubi you whould have to > fix it or add read loop in spi-nor to work around ubi deficiency. > But in mtdblock.c, mtd_read is invoked by mtdblock_readsect->do_cached_read without read loop. And just compare the retlen with requested data size, if not equal, the EIO will be reported. 243 ret = mtd_read(mtd, pos, size, &retlen, buf); 244 if (ret) 245 return ret; 246 if (retlen != size) 247 return -EIO; > Thanks > > Michal
On 14 August 2015 at 09:26, Hou Zhiqiang <B48286@freescale.com> wrote: > Hi Michal, > >> -----Original Message----- >> From: Michal Suchanek [mailto:hramrach@gmail.com] >> Sent: 2015年8月14日 12:43 >> To: Hou Zhiqiang-B48286 >> Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; >> dwmw2@infradead.org; jonatas.rech@datacom.ind.br; Jane.Wan@gainspeed.com; >> shijie.huang@intel.com; valentin.longchamp@keymile.com; >> hkallweit1@gmail.com; beanhuo@micron.com; Hu Mingkai-B21284 >> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read >> >> On 14 August 2015 at 06:30, Michal Suchanek <hramrach@gmail.com> wrote: >> > On 14 August 2015 at 05:40, Hou Zhiqiang <B48286@freescale.com> wrote: >> >> >> >> >> >>> -----Original Message----- >> >>> From: Michal Suchanek [mailto:hramrach@gmail.com] >> >>> Sent: 2015年8月13日 17:42 >> >>> To: Hou Zhiqiang-B48286 >> >>> Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; >> >>> dwmw2@infradead.org; jonatas.rech@datacom.ind.br; >> >>> Jane.Wan@gainspeed.com; shijie.huang@intel.com; >> >>> valentin.longchamp@keymile.com; hkallweit1@gmail.com; >> >>> beanhuo@micron.com; Hu Mingkai-B21284 >> >>> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the >> >>> spi_nor_read >> >>> >> >>> On 13 August 2015 at 11:03, Hou Zhiqiang <B48286@freescale.com> wrote: >> >>> > Hello, >> >>> > >> >>> > I want to add the 4-Byte addressing flashes support for the >> >>> > Freescale eSPI controller dirver. Appreciate your suggestions. >> >>> > >> >>> > Background: >> >>> > In the Freescale eSPI controller dirver, if the transaction length >> >>> > exceed 64KiB, the contents of the transaction was touched to >> >>> > update the command with assumed the 3-Byte address width. It is a >> >>> > workaround, due to the Freescale eSPI controller have a hardware >> >>> > limit that the maximum one-time transaction length is 64KiB, that >> >>> > isn't consistent >> >>> with many other vendors' >> >>> > SPI controllers, and so far the SPI flash driver assume the SPI >> >>> > controller have the ability to complete the specified transaction >> >>> length at one-time. >> >>> > >> >>> > But this workaround is only suit for 3-Byte addressing slaves, as >> >>> > time goes by, there are 4-Byte address width SPI flashes, so this >> >>> > assumption isn't correct and this workaround discombobulate the >> >>> > controller driver layer and protocol driver layer. So, this >> >>> > workaround should be removed and the SPI client driver should >> >>> > ensure the >> >>> transaction completion. >> >>> > >> >>> > There are two solutions: >> >>> > 1. In spi-nor framework, compare the retlen with the transaction >> >>> > length specified, if the retlen less than the transaction length >> >>> > and with no error number returned, re-initiate the transaction >> >>> > with the >> >>> updated address. >> >>> > Advantage: >> >>> > It won't affect other SPI controllers without this limit. >> >>> > Disadvantage: >> >>> > It is not a systematic solution. >> >>> >> >>> This can probably break transactions that are performed on non-flash >> >>> slaves but look like flash read command. >> >>> >> >>> > >> >>> > 2. Add quirk support, something like i2c quirk. >> >>> > Advantage: >> >>> > It is a systematic solution. >> >>> > Disadvantage: >> >>> > The quirk mechanism is only useful for Freescale eSPI controller, >> >>> > but it will affect whole SPI framework. >> >>> >> >>> I added this quirk in m25p80.c since my current SPI master driver is >> >>> just broken so I tried to work around that. While the quirk itself >> >>> is rejected as non-syetematic this series that adds the required >> >>> support for checking the return value of the SPI master driver >> transfer functions. >> >>> >> >>> So you have options: >> >>> >> >>> 2a: add SPI master quirk and check it in m25p80.c and truncate the >> >>> transfer in m25p80.c >> >>> >> >>> 2b: truncate the transfer in SPI master driver and just return the >> >>> number of bytes transferred >> >>> >> >> >> >> I expect this way, so the spi slave driver should check the retlen >> >> with the transaction length and do some loop transfer, and make sure >> >> getting all data this transaction wanted. >> >> >> >>> This patch series should handle fragmenting the transfers in >> >>> spi-nor.c >> >>> >> >>> http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.html >> >>> >> >> >> >> Please correct me if I'm wrong, AFAIU the mtd layer will check the >> >> retlen and if it isn't equal to the transaction length, the EIO will >> be returned. >> > >> > I never had this happen because with this patch series spi-nor checks >> > the amount of data transferred and continues until the whole length is >> > transferred. Without it just sets retlen to length regardless of >> > amount of data transferred. >> > >> >> Looking at the code closely mtdcore just invokes _read in mtd_read and >> passes the result on. mtdchar does the same. ubi has assert that read >> amount of data is equal requested so if you used ubi you whould have to >> fix it or add read loop in spi-nor to work around ubi deficiency. >> > > But in mtdblock.c, mtd_read is invoked by mtdblock_readsect->do_cached_read > without read loop. And just compare the retlen with requested data size, if > not equal, the EIO will be reported. > > 243 ret = mtd_read(mtd, pos, size, &retlen, buf); > 244 if (ret) > 245 return ret; > 246 if (retlen != size) > 247 return -EIO; > Size is set to cache_size which is set to erasesize which is typically 4k/32k/64k on spi nor so you should not hit this in practice. It should be fixed to work with exotic erase sizes as well, though. Thanks Michal
> -----Original Message----- > From: Michal Suchanek [mailto:hramrach@gmail.com] > Sent: 2015年8月14日 15:34 > To: Hou Zhiqiang-B48286 > Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > dwmw2@infradead.org; jonatas.rech@datacom.ind.br; Jane.Wan@gainspeed.com; > shijie.huang@intel.com; valentin.longchamp@keymile.com; > hkallweit1@gmail.com; beanhuo@micron.com; Hu Mingkai-B21284 > Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read > > On 14 August 2015 at 09:26, Hou Zhiqiang <B48286@freescale.com> wrote: > > Hi Michal, > > > >> -----Original Message----- > >> From: Michal Suchanek [mailto:hramrach@gmail.com] > >> Sent: 2015年8月14日 12:43 > >> To: Hou Zhiqiang-B48286 > >> Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > >> dwmw2@infradead.org; jonatas.rech@datacom.ind.br; > >> Jane.Wan@gainspeed.com; shijie.huang@intel.com; > >> valentin.longchamp@keymile.com; hkallweit1@gmail.com; > >> beanhuo@micron.com; Hu Mingkai-B21284 > >> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the > >> spi_nor_read > >> > >> On 14 August 2015 at 06:30, Michal Suchanek <hramrach@gmail.com> wrote: > >> > On 14 August 2015 at 05:40, Hou Zhiqiang <B48286@freescale.com> > wrote: > >> >> > >> >> > >> >>> -----Original Message----- > >> >>> From: Michal Suchanek [mailto:hramrach@gmail.com] > >> >>> Sent: 2015年8月13日 17:42 > >> >>> To: Hou Zhiqiang-B48286 > >> >>> Cc: linux-mtd@lists.infradead.org; computersforpeace@gmail.com; > >> >>> dwmw2@infradead.org; jonatas.rech@datacom.ind.br; > >> >>> Jane.Wan@gainspeed.com; shijie.huang@intel.com; > >> >>> valentin.longchamp@keymile.com; hkallweit1@gmail.com; > >> >>> beanhuo@micron.com; Hu Mingkai-B21284 > >> >>> Subject: Re: [PATCH] RFC: mtd/spi-nor: add checking for the > >> >>> spi_nor_read > >> >>> > >> >>> On 13 August 2015 at 11:03, Hou Zhiqiang <B48286@freescale.com> > wrote: > >> >>> > Hello, > >> >>> > > >> >>> > I want to add the 4-Byte addressing flashes support for the > >> >>> > Freescale eSPI controller dirver. Appreciate your suggestions. > >> >>> > > >> >>> > Background: > >> >>> > In the Freescale eSPI controller dirver, if the transaction > >> >>> > length exceed 64KiB, the contents of the transaction was > >> >>> > touched to update the command with assumed the 3-Byte address > >> >>> > width. It is a workaround, due to the Freescale eSPI controller > >> >>> > have a hardware limit that the maximum one-time transaction > >> >>> > length is 64KiB, that isn't consistent > >> >>> with many other vendors' > >> >>> > SPI controllers, and so far the SPI flash driver assume the SPI > >> >>> > controller have the ability to complete the specified > >> >>> > transaction > >> >>> length at one-time. > >> >>> > > >> >>> > But this workaround is only suit for 3-Byte addressing slaves, > >> >>> > as time goes by, there are 4-Byte address width SPI flashes, so > >> >>> > this assumption isn't correct and this workaround > >> >>> > discombobulate the controller driver layer and protocol driver > >> >>> > layer. So, this workaround should be removed and the SPI client > >> >>> > driver should ensure the > >> >>> transaction completion. > >> >>> > > >> >>> > There are two solutions: > >> >>> > 1. In spi-nor framework, compare the retlen with the > >> >>> > transaction length specified, if the retlen less than the > >> >>> > transaction length and with no error number returned, > >> >>> > re-initiate the transaction with the > >> >>> updated address. > >> >>> > Advantage: > >> >>> > It won't affect other SPI controllers without this limit. > >> >>> > Disadvantage: > >> >>> > It is not a systematic solution. > >> >>> > >> >>> This can probably break transactions that are performed on > >> >>> non-flash slaves but look like flash read command. > >> >>> > >> >>> > > >> >>> > 2. Add quirk support, something like i2c quirk. > >> >>> > Advantage: > >> >>> > It is a systematic solution. > >> >>> > Disadvantage: > >> >>> > The quirk mechanism is only useful for Freescale eSPI > >> >>> > controller, but it will affect whole SPI framework. > >> >>> > >> >>> I added this quirk in m25p80.c since my current SPI master driver > >> >>> is just broken so I tried to work around that. While the quirk > >> >>> itself is rejected as non-syetematic this series that adds the > >> >>> required support for checking the return value of the SPI master > >> >>> driver > >> transfer functions. > >> >>> > >> >>> So you have options: > >> >>> > >> >>> 2a: add SPI master quirk and check it in m25p80.c and truncate > >> >>> the transfer in m25p80.c > >> >>> > >> >>> 2b: truncate the transfer in SPI master driver and just return > >> >>> the number of bytes transferred > >> >>> > >> >> > >> >> I expect this way, so the spi slave driver should check the retlen > >> >> with the transaction length and do some loop transfer, and make > >> >> sure getting all data this transaction wanted. > >> >> > >> >>> This patch series should handle fragmenting the transfers in > >> >>> spi-nor.c > >> >>> > >> >>> http://lists.infradead.org/pipermail/linux-mtd/2015-July/060466.h > >> >>> tml > >> >>> > >> >> > >> >> Please correct me if I'm wrong, AFAIU the mtd layer will check the > >> >> retlen and if it isn't equal to the transaction length, the EIO > >> >> will > >> be returned. > >> > > >> > I never had this happen because with this patch series spi-nor > >> > checks the amount of data transferred and continues until the whole > >> > length is transferred. Without it just sets retlen to length > >> > regardless of amount of data transferred. > >> > > >> > >> Looking at the code closely mtdcore just invokes _read in mtd_read > >> and passes the result on. mtdchar does the same. ubi has assert that > >> read amount of data is equal requested so if you used ubi you whould > >> have to fix it or add read loop in spi-nor to work around ubi > deficiency. > >> > > > > But in mtdblock.c, mtd_read is invoked by > > mtdblock_readsect->do_cached_read without read loop. And just compare > > the retlen with requested data size, if not equal, the EIO will be > reported. > > > > 243 ret = mtd_read(mtd, pos, size, &retlen, > buf); > > 244 if (ret) > > 245 return ret; > > 246 if (retlen != size) > > 247 return -EIO; > > > > Size is set to cache_size which is set to erasesize which is typically > 4k/32k/64k on spi nor so you should not hit this in practice. > If the size is 64K and the spi controller have the one-time transfer limit Less than 64K, then the (retlen != size) is hited and EIO will be returned. According to this code section, do_cached_read require the mtd_read should ensure the amount of data transferred completion. And spi nor read ignores hardware limits of spi controllers and assumes spi controllers have the ability to transfer any length transaction one-time. > It should be fixed to work with exotic erase sizes as well, though. > > Thanks > > Michal Thanks, Zhiqiang
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index fbc5035..ec10cd1 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -715,6 +715,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, { struct spi_nor *nor = mtd_to_spi_nor(mtd); int ret; + size_t cnt = 0; + u_char *cur = buf; dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); @@ -722,8 +724,16 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, if (ret) return ret; - ret = nor->read(nor, from, len, retlen, buf); - + *retlen = 0; + while (len > 0) { + ret = nor->read(nor, from, len, &cnt, cur); + if (ret < 0) + break; + len -= cnt; + cur += cnt; + *retlen += cnt; + from += cnt; + } spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ); return ret; }