Re: [PATCH v6, 5/8] mtd: m25p80: Let m25p80_read() fallback to spi transfer

From: Marek Vasut <marek.vasut-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
To: Cyrille Pitchen
	<cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>,
	Florian Fainelli
	<f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Kamal Dasu <kamal.dasu-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>
Cc: vigneshr-l0cyMroinI0@public.gmane.org,
	Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Yendapally Reddy Dhananjaya Reddy
	<yendapally.reddy-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w@public.gmane.org,
	Jon Mason <jon.mason-dY08KVG/lbpWk0Htik3J/w@public.gmane.org>,
	Brian Norris
	<computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v6, 5/8] mtd: m25p80: Let m25p80_read() fallback to spi transfer
Date: Thu, 1 Dec 2016 16:45:01 +0100	[thread overview]
Message-ID: <3260e7e5-fcf1-599a-e7b4-fd7aaef5be2e@gmail.com> (raw)
In-Reply-To: <a0179d25-71a5-b47f-4272-49aa2a346b54-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>

On 11/29/2016 03:06 PM, Cyrille Pitchen wrote:
> Hi all,
> 
> +Marek
> 
> Le 29/11/2016 à 02:32, Florian Fainelli a écrit :
>> On 10/14/2016 06:17 AM, Cyrille Pitchen wrote:
>>> Le 13/10/2016 à 23:15, Kamal Dasu a écrit :
>>>> On Mon, Oct 10, 2016 at 4:29 AM, Cyrille Pitchen
>>>> <cyrille.pitchen-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org> wrote:
>>>>> Hi all,
>>>>>
>>>>>
>>>>> Le 10/10/2016 à 10:04, Florian Fainelli a écrit :
>>>>>> On 08/24/2016 03:04 PM, Kamal Dasu wrote:
>>>>>>> In m25p80_read() even though spi_flash_read() is supported
>>>>>>> by some drivers, under certain circumstances like unaligned
>>>>>>> buffer, address or address range limitations on certain SoCs
>>>>>>> let it fallback to core spi reads. Such drivers are expected
>>>>>>> to return -EAGAIN so that the m25p80_read() uses standard
>>>>>>> spi transfer.
>>>>>>>
>>>>>>> Signed-off-by: Kamal Dasu <kdasu.kdev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>
>>>>>> MTD folks, any comments on this?
>>>>>>
>>>>>>> ---
>>>>>>>  drivers/mtd/devices/m25p80.c | 11 +++++++++--
>>>>>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>>>>>>> index 9cf7fcd..77c2d2c 100644
>>>>>>> --- a/drivers/mtd/devices/m25p80.c
>>>>>>> +++ b/drivers/mtd/devices/m25p80.c
>>>>>>> @@ -155,9 +155,16 @@ static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
>>>>>>>              msg.data_nbits = m25p80_rx_nbits(nor);
>>>>>>>
>>>>>>>              ret = spi_flash_read(spi, &msg);
>>>>>>> -            if (ret < 0)
>>>>>>> +
>>>>>>> +            if (ret >= 0)
>>>>>>> +                    return msg.retlen;
>>>>>>> +
>>>>>>> +            /*
>>>>>>> +             * some spi master drivers might need to fallback to
>>>>>>> +             * normal spi transfer
>>>>>>> +             */
>>>>>>> +            if (ret != -EAGAIN)
>>>>> I just wonder whether EINVAL would be a better choice.
>>>>
>>>>  spi_flash_read calls the down stream controller driver with all
>>>> params addresses however  accelerated transfer is not possible by the
>>>> controller due to alignment issues, it needs to indicate to m25p call
>>>> to try the normal transfer, hence use of EAGAIN seemed appropriate.
>>>>
>>>>
>>>
>>> Yes, I think I've understood the purpose of this patch. In the example you
>>> gave, the actual implementation of spi_flash_read() works fine with aligned
>>> addresses but doesn't support unaligned addresses. Hence, such unaligned
>>> addresses are invalid argument for spi_flash_read() and we should fall back
>>> to the legacy implementation of m25p80_read().
>>>
>>> My point is just that EINVAL clearly tells that the SPI controller driver
>>> implementation of spi_flash_read() doesn't support the given input
>>> parameters, here an unaligned address, whereas EAGAIN suggests that some
>>> hardware resource is temporarily unavailable and we could call spi_flash_read()
>>> again later. However, in this case, spi_flash_read() would still fail even if
>>> called later.
>>>
>>> That's why I've suggested EINVAL might have been a better choice than EAGAIN,
>>> but honestly it's not a big deal, only a detail. So if most people prefer to
>>> keep EAGAIN, I'm perfectly fine with it! :)
>>>
>>> I don't want my comment to delay the integration of this patch.
>>
>> So what are we going to do now, should Kamal resubmit and
>> s/EGAIN/EINVAL/ or is EAGAIN good enough? If EINVAL needs to be used,
>> which I agree with you seems like a valid error code to return, this
>> does imply changing the Broadcom QSPI driver though... so we have to
>> coordinate the two changes to be merged through the same cycle to avoid
>> regressions.
>>
> 
> Please replace the EAGAIN error code by EINVAL then I agree to merge this
> patch in the github spi-nor tree.
> 
> I know the EAGAIN error code has already been introduced in spi-bcm-qspi.c
> through the spi tree but since currently m25p280.c handles neither EAGAIN
> nor EINVAL as the returned code of spi_flash_read(), I guess no regression
> will be introduced. The feature will work as expected once both the m25p80.c
> and spi-bcm-qspi.c use the very same error code.
> 
> Marek, any comment?

I'm fine with this patch, but a patch for the broadcom controller would
be great.

-- 
Best regards,
Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html