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

Michal Suchanek hramrach at gmail.com
Fri Aug 14 00:33:55 PDT 2015


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.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



More information about the linux-mtd mailing list