[PATCH] RFC: mtd/spi-nor: add checking for the spi_nor_read

Fri Aug 14 01:45:39 PDT 2015

> -----Original Message-----
> From: Michal Suchanek [mailto:hramrach at gmail.com]
> Sent: 2015年8月14日 15:34
> To: Hou Zhiqiang-B48286
> Cc: linux-mtd at lists.infradead.org; computersforpeace at gmail.com;
> dwmw2 at infradead.org; jonatas.rech at datacom.ind.br; Jane.Wan at gainspeed.com;
> shijie.huang at intel.com; valentin.longchamp at keymile.com;
> hkallweit1 at gmail.com; beanhuo at 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 at freescale.com> wrote:
> > Hi Michal,
> >
> >> -----Original Message-----
> >> From: Michal Suchanek [mailto:hramrach at gmail.com]
> >> Sent: 2015年8月14日 12:43
> >> To: Hou Zhiqiang-B48286
> >> Cc: linux-mtd at lists.infradead.org; computersforpeace at gmail.com;
> >> dwmw2 at infradead.org; jonatas.rech at datacom.ind.br;
> >> Jane.Wan at gainspeed.com; shijie.huang at intel.com;
> >> valentin.longchamp at keymile.com; hkallweit1 at gmail.com;
> >> beanhuo at 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 at gmail.com> wrote:
> >> > On 14 August 2015 at 05:40, Hou Zhiqiang <B48286 at freescale.com>
> wrote:
> >> >>
> >> >>
> >> >>> -----Original Message-----
> >> >>> From: Michal Suchanek [mailto:hramrach at gmail.com]
> >> >>> Sent: 2015年8月13日 17:42
> >> >>> To: Hou Zhiqiang-B48286
> >> >>> Cc: linux-mtd at lists.infradead.org; computersforpeace at gmail.com;
> >> >>> dwmw2 at infradead.org; jonatas.rech at datacom.ind.br;
> >> >>> Jane.Wan at gainspeed.com; shijie.huang at intel.com;
> >> >>> valentin.longchamp at keymile.com; hkallweit1 at gmail.com;
> >> >>> beanhuo at 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 at 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