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

[Michal Suchanek]

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.

Thanks

Michal