[PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Kamal Dasu]

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 -EINVAL so that the m25p80_read() uses standard
spi transfer.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 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..7b7f2cc 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 != -EINVAL)
 			return ret;
-		return msg.retlen;
 	}
 
 	spi_message_init(&m);
-- 
1.9.1

[PATCH, 2/2] spi: bcm-qspi: Fix return code from bcm_qspi_flash_read()

[Kamal Dasu]

To properly fallback to normal mspi reads return appropriate return code to
m25p80_read() function. Changed return code to -EINVAL from -EAGAIN to match
what is expected by the m25p80 driver.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/spi/spi-bcm-qspi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
index 14f9dea..3e37b6b 100644
--- a/drivers/spi/spi-bcm-qspi.c
+++ b/drivers/spi/spi-bcm-qspi.c
@@ -919,7 +919,7 @@ static int bcm_qspi_flash_read(struct spi_device *spi,
 
 	if (mspi_read)
 		/* this will make the m25p80 read to fallback to mspi read */
-		return -EAGAIN;
+		return -EINVAL;
 
 	io_width = msg->data_nbits ? msg->data_nbits : SPI_NBITS_SINGLE;
 	addrlen = msg->addr_width;
-- 
1.9.1

Re: [PATCH, 2/2] spi: bcm-qspi: Fix return code from bcm_qspi_flash_read()

[Marek Vasut]

On 01/20/2017 10:50 PM, Kamal Dasu wrote:
> To properly fallback to normal mspi reads return appropriate return code to
> m25p80_read() function. Changed return code to -EINVAL from -EAGAIN to match
> what is expected by the m25p80 driver.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/spi/spi-bcm-qspi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
> index 14f9dea..3e37b6b 100644
> --- a/drivers/spi/spi-bcm-qspi.c
> +++ b/drivers/spi/spi-bcm-qspi.c
> @@ -919,7 +919,7 @@ static int bcm_qspi_flash_read(struct spi_device *spi,
>  
>  	if (mspi_read)
>  		/* this will make the m25p80 read to fallback to mspi read */
> -		return -EAGAIN;
> +		return -EINVAL;

This looks wrong, twice. First, why is the m25q80 driver mentioned in
this driver at all ? Second, see my comment for 1/2 , I think if the
read fails and it's because of some condition the block cannot handle,
but can recover from, the driver should deal with that.

>  	io_width = msg->data_nbits ? msg->data_nbits : SPI_NBITS_SINGLE;
>  	addrlen = msg->addr_width;
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Marek Vasut]

On 01/20/2017 10:50 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 -EINVAL so that the m25p80_read() uses standard
> spi transfer.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  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..7b7f2cc 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;

ret can never be > 0 , it is only 0 or negative .

> +		/*
> +		 * some spi master drivers might need to fallback to
> +		 * normal spi transfer
> +		 */
> +		if (ret != -EINVAL)
>  			return ret;

This looks really fragile and special-casing EINVAL here doesn't scale.
But still, if your controller driver is buggy, fix the driver, do not
pollute core code with workarounds. If you do support this sort of
accelerated read and it fails, it means something is seriously wrong.
If you need to invoke regular SPI reads to complete under some obscure
circumstances, do it from the driver, not here.

> -		return msg.retlen;
>  	}
>  
>  	spi_message_init(&m);
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Michal Suchanek]

On 20 January 2017 at 23:38, Marek Vasut <marex@denx.de> wrote:
> On 01/20/2017 10:50 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 -EINVAL so that the m25p80_read() uses standard
>> spi transfer.
>>
>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>> ---
>>  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..7b7f2cc 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;
>
> ret can never be > 0 , it is only 0 or negative .
>
>> +             /*
>> +              * some spi master drivers might need to fallback to
>> +              * normal spi transfer
>> +              */
>> +             if (ret != -EINVAL)
>>                       return ret;
>
> This looks really fragile and special-casing EINVAL here doesn't scale.
> But still, if your controller driver is buggy, fix the driver, do not
> pollute core code with workarounds. If you do support this sort of
> accelerated read and it fails, it means something is seriously wrong.
> If you need to invoke regular SPI reads to complete under some obscure
> circumstances, do it from the driver, not here.

I guess the other half of m25p80_read can be factored out and used as
fallback from either m25p80_read or the controller driver.

Thanks

Michal

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Marek Vasut]

On 01/21/2017 12:53 AM, Michal Suchanek wrote:
> On 20 January 2017 at 23:38, Marek Vasut <marex@denx.de> wrote:
>> On 01/20/2017 10:50 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 -EINVAL so that the m25p80_read() uses standard
>>> spi transfer.
>>>
>>> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
>>> ---
>>>  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..7b7f2cc 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;
>>
>> ret can never be > 0 , it is only 0 or negative .
>>
>>> +             /*
>>> +              * some spi master drivers might need to fallback to
>>> +              * normal spi transfer
>>> +              */
>>> +             if (ret != -EINVAL)
>>>                       return ret;
>>
>> This looks really fragile and special-casing EINVAL here doesn't scale.
>> But still, if your controller driver is buggy, fix the driver, do not
>> pollute core code with workarounds. If you do support this sort of
>> accelerated read and it fails, it means something is seriously wrong.
>> If you need to invoke regular SPI reads to complete under some obscure
>> circumstances, do it from the driver, not here.
> 
> I guess the other half of m25p80_read can be factored out and used as
> fallback from either m25p80_read or the controller driver.

I think I see what you mean, but care to show an RFC patch ?

-- 
Best regards,
Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Kamal Dasu]

"ret can never be > 0 , it is only 0 or negative "

I can fix this.

>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>> But still, if your controller driver is buggy, fix the driver, do not
>>> pollute core code with workarounds. If you do support this sort of
>>> accelerated read and it fails, it means something is seriously wrong.
>>> If you need to invoke regular SPI reads to complete under some obscure
>>> circumstances, do it from the driver, not here.
>>
>> I guess the other half of m25p80_read can be factored out and used as
>> fallback from either m25p80_read or the controller driver.
>
> I think I see what you mean, but care to show an RFC patch ?
>
> --

Its not the controller driver, but he hardware limitation with older
controller version. I have tried to see how I can do this better,
however when spi_flash_read() is called  cannot handle it within my
driver without returning from the function. I went over this with Mark
previously and this current solution seemed reasonable. Any other
solution outside of the generic driver would replicate a lot of code
unnecessarily.

> Best regards,
> Marek Vasut


Thanks
Kamal

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Marek Vasut]

On 01/24/2017 12:41 AM, Kamal Dasu wrote:
> "ret can never be > 0 , it is only 0 or negative "
> 
> I can fix this.
> 
>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>> pollute core code with workarounds. If you do support this sort of
>>>> accelerated read and it fails, it means something is seriously wrong.
>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>> circumstances, do it from the driver, not here.
>>>
>>> I guess the other half of m25p80_read can be factored out and used as
>>> fallback from either m25p80_read or the controller driver.
>>
>> I think I see what you mean, but care to show an RFC patch ?
>>
>> --
> 
> Its not the controller driver, but he hardware limitation with older
> controller version. I have tried to see how I can do this better,
> however when spi_flash_read() is called  cannot handle it within my
> driver without returning from the function. I went over this with Mark
> previously and this current solution seemed reasonable. Any other
> solution outside of the generic driver would replicate a lot of code
> unnecessarily.

Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
function could be the solution and invoking the second part from the
driver if applicable, but this cannot work because the driver does not
know when it's interacting with SPI NOR and when with something else .

Can you tell me about the conditions under which the bcm controller
fails and should fall back to standard spi read ?

-- 
Best regards,
Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Michal Suchanek]

On 25 January 2017 at 03:08, Marek Vasut <marex@denx.de> wrote:
> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>> "ret can never be > 0 , it is only 0 or negative "
>>
>> I can fix this.
>>
>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>> pollute core code with workarounds. If you do support this sort of
>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>> circumstances, do it from the driver, not here.
>>>>
>>>> I guess the other half of m25p80_read can be factored out and used as
>>>> fallback from either m25p80_read or the controller driver.
>>>
>>> I think I see what you mean, but care to show an RFC patch ?
>>>
>>> --
>>
>> Its not the controller driver, but he hardware limitation with older
>> controller version. I have tried to see how I can do this better,
>> however when spi_flash_read() is called  cannot handle it within my
>> driver without returning from the function. I went over this with Mark
>> previously and this current solution seemed reasonable. Any other
>> solution outside of the generic driver would replicate a lot of code
>> unnecessarily.
>
> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
> function could be the solution and invoking the second part from the
> driver if applicable, but this cannot work because the driver does not
> know when it's interacting with SPI NOR and when with something else .
>
> Can you tell me about the conditions under which the bcm controller
> fails and should fall back to standard spi read ?

spi_flash_read is designed to perform what m25p80_read does in a
controller-specific way. So how can you get to a point when you are
in spi_flash_read, it fails, and you do not know you if you can call
the bottom half of m25p80_read to finish the job?

Thanks

Michal

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Marek Vasut]

On 01/25/2017 07:29 AM, Michal Suchanek wrote:
> On 25 January 2017 at 03:08, Marek Vasut <marex@denx.de> wrote:
>> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>>> "ret can never be > 0 , it is only 0 or negative "
>>>
>>> I can fix this.
>>>
>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>>> pollute core code with workarounds. If you do support this sort of
>>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>>> circumstances, do it from the driver, not here.
>>>>>
>>>>> I guess the other half of m25p80_read can be factored out and used as
>>>>> fallback from either m25p80_read or the controller driver.
>>>>
>>>> I think I see what you mean, but care to show an RFC patch ?
>>>>
>>>> --
>>>
>>> Its not the controller driver, but he hardware limitation with older
>>> controller version. I have tried to see how I can do this better,
>>> however when spi_flash_read() is called  cannot handle it within my
>>> driver without returning from the function. I went over this with Mark
>>> previously and this current solution seemed reasonable. Any other
>>> solution outside of the generic driver would replicate a lot of code
>>> unnecessarily.
>>
>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
>> function could be the solution and invoking the second part from the
>> driver if applicable, but this cannot work because the driver does not
>> know when it's interacting with SPI NOR and when with something else .
>>
>> Can you tell me about the conditions under which the bcm controller
>> fails and should fall back to standard spi read ?
> 
> spi_flash_read is designed to perform what m25p80_read does in a
> controller-specific way. So how can you get to a point when you are
> in spi_flash_read, it fails, and you do not know you if you can call
> the bottom half of m25p80_read to finish the job?

Huh ? You can't, but that's not what my question is about. I'd like to
understand what's the problem with the bcm controller in the first place.

-- 
Best regards,
Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Cyrille Pitchen]

Hi all,

Le 25/01/2017 à 07:29, Michal Suchanek a écrit :
> On 25 January 2017 at 03:08, Marek Vasut <marex@denx.de> wrote:
>> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>>> "ret can never be > 0 , it is only 0 or negative "
>>>
>>> I can fix this.
>>>
>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>>> pollute core code with workarounds. If you do support this sort of
>>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>>> circumstances, do it from the driver, not here.
>>>>>
>>>>> I guess the other half of m25p80_read can be factored out and used as
>>>>> fallback from either m25p80_read or the controller driver.
>>>>
>>>> I think I see what you mean, but care to show an RFC patch ?
>>>>
>>>> --
>>>
>>> Its not the controller driver, but he hardware limitation with older
>>> controller version. I have tried to see how I can do this better,
>>> however when spi_flash_read() is called  cannot handle it within my
>>> driver without returning from the function. I went over this with Mark
>>> previously and this current solution seemed reasonable. Any other
>>> solution outside of the generic driver would replicate a lot of code
>>> unnecessarily.
>>
>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
>> function could be the solution and invoking the second part from the
>> driver if applicable, but this cannot work because the driver does not
>> know when it's interacting with SPI NOR and when with something else .
>>
>> Can you tell me about the conditions under which the bcm controller
>> fails and should fall back to standard spi read ?
> 
> spi_flash_read is designed to perform what m25p80_read does in a
> controller-specific way. So how can you get to a point when you are
> in spi_flash_read, it fails, and you do not know you if you can call
> the bottom half of m25p80_read to finish the job?
>

Indeed, spi_flash_read() was designed so the m25p80.c driver can bypass all
the regular SPI API and access directly the SPI controller driver.

When using the regular SPI API with spi_transfer and spi_message
structures, some pieces of information are lost at the level of the SPI
controller driver. This driver only sees streams of bytes to be read or
written on the SPI bus but no longer knows the protocol split between op
code | address/dummy | data bytes. Actually in this scenario, the SPI
driver should not even know whether the SPI device is a memory or not.

Then for "hybrid" SPI controllers, which can be connected at the same time
to SPI NOR and other SPI devices, the spi-nor framework API is not suited
as it is fully oriented to SPI memory devices and other SPI devices can't
be used. That's why spi_flash_read() was introduced in the API of the SPI
subsystem. This new hook allows the SPI controller driver to fully take
advantage of the hardware capabilities mainly to read from flash memory,
mapped in the system memory by the SPI controller.
To do so, the spi_flash_read_message structure was designed so the pieces
of informations (op code, address, address width, number of dummy bytes,
...) that were previously lost by the regular SPI API are now still
available to the SPI controller.

All this long introduction (sorry for that), just to say that, yes, in the
regular case, the SPI controller driver has no mean to know whether the SPI
device is a memory or not so some workarounds can only be done in upper
layers where the protocol details are still known. However we are not
in such a case here: we are using spi_flash_read(), so the SPI controller
driver knows without any doubt that it is talking to a SPI memory device
and this SPI controller driver still has all the protocol details it needs
to build some spi_message/spi_transfer structures like the m25p80.c driver
does when spi_flash_read_supported() returns false.

So I see many solutions to handle the unaligned data issue or implement
other workarounds:

1 - do everything in the SPI controller driver specific implementation of
spi_flash_read(). This is Marek's approach

1 bis - extract the half-bottom of m25p80_read in some exported function to
be called from the SPI controller driver specific implementation of
spi_flash_read() if we want an easy way to add a fallback implementation
without duplicating code. That's what Michal suggests I think.

2 - check the return code of spi_flash_read() and in the case of a specific
value (-EINVAL), m25p80_read() executes the same code as if
spi_flash_read_supported() returned false: That's what Kamal proposes with
this patch.


After reading this thread, IMHO solution 1 (including 1 bis) seems cleaner
than solution 2. Solution 2 gives a special meaning to some error code
value to ask m25p80_read() to retry with the legacy implementation.
However some other SPI controller drivers could return the very same (and
very common) error code without expecting m25p80_read() to send them a
spi_message structure as a fallback.

When I say a "very common error code", I don't change my mind and still
think that -EINVAL is better than -EAGAIN if finally the solution 2 is
chosen. Solution 2 is not that bad but I personally have a small preference
for solution 1 / 1 bis.

Best regards,

Cyrille



> Thanks
> 
> Michal
> 

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Kamal Dasu]

If the transfers are short and dest buffer or the flash address are
unaligned. Also in case of older version of the controller there are
some address mapping limitations when a transfer crosses 4MB window
(addr + len).  So in such cases  need to fallback to normal MSPI
reads.

One other option is that controller divers implementation of
bcm_qspi_spi_flash_read() can return msg.retlen = 0 and the
m25p80_read() can fallback to normal mspi read.

Kamal



On Tue, Jan 24, 2017 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>> "ret can never be > 0 , it is only 0 or negative "
>>
>> I can fix this.
>>
>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>> pollute core code with workarounds. If you do support this sort of
>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>> circumstances, do it from the driver, not here.
>>>>
>>>> I guess the other half of m25p80_read can be factored out and used as
>>>> fallback from either m25p80_read or the controller driver.
>>>
>>> I think I see what you mean, but care to show an RFC patch ?
>>>
>>> --
>>
>> Its not the controller driver, but he hardware limitation with older
>> controller version. I have tried to see how I can do this better,
>> however when spi_flash_read() is called  cannot handle it within my
>> driver without returning from the function. I went over this with Mark
>> previously and this current solution seemed reasonable. Any other
>> solution outside of the generic driver would replicate a lot of code
>> unnecessarily.
>
> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
> function could be the solution and invoking the second part from the
> driver if applicable, but this cannot work because the driver does not
> know when it's interacting with SPI NOR and when with something else .
>
> Can you tell me about the conditions under which the bcm controller
> fails and should fall back to standard spi read ?
>
> --
> Best regards,
> Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Marek Vasut]

On 01/25/2017 05:28 PM, Kamal Dasu wrote:
> If the transfers are short and dest buffer or the flash address are
> unaligned.

That sounds like a DMA problem where you're trying to fall back to PIO ?

> Also in case of older version of the controller there are
> some address mapping limitations when a transfer crosses 4MB window
> (addr + len).  So in such cases  need to fallback to normal MSPI
> reads.

But the driver can also detect this mode of failure before doing the
transfer and call it's internal functions to perform the transfer as
needed, right?

> One other option is that controller divers implementation of
> bcm_qspi_spi_flash_read() can return msg.retlen = 0 and the
> m25p80_read() can fallback to normal mspi read.

I'd much rather see the driver handling such detail internally instead
of patching the core code. Moreover, if you patch the core code, the SF
read will go - in case of a failure- all the way through the SPI
framework only to land in the same driver, which doesn't make much sense.

btw please do NOT top-post:
http://www.arm.linux.org.uk/mailinglists/etiquette.php#e3

> Kamal
> 
> 
> 
> On Tue, Jan 24, 2017 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
>> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>>> "ret can never be > 0 , it is only 0 or negative "
>>>
>>> I can fix this.
>>>
>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>>> pollute core code with workarounds. If you do support this sort of
>>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>>> circumstances, do it from the driver, not here.
>>>>>
>>>>> I guess the other half of m25p80_read can be factored out and used as
>>>>> fallback from either m25p80_read or the controller driver.
>>>>
>>>> I think I see what you mean, but care to show an RFC patch ?
>>>>
>>>> --
>>>
>>> Its not the controller driver, but he hardware limitation with older
>>> controller version. I have tried to see how I can do this better,
>>> however when spi_flash_read() is called  cannot handle it within my
>>> driver without returning from the function. I went over this with Mark
>>> previously and this current solution seemed reasonable. Any other
>>> solution outside of the generic driver would replicate a lot of code
>>> unnecessarily.
>>
>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
>> function could be the solution and invoking the second part from the
>> driver if applicable, but this cannot work because the driver does not
>> know when it's interacting with SPI NOR and when with something else .
>>
>> Can you tell me about the conditions under which the bcm controller
>> fails and should fall back to standard spi read ?
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Kamal Dasu]

On Wed, Jan 25, 2017 at 11:39 AM, Marek Vasut <marex@denx.de> wrote:
> On 01/25/2017 05:28 PM, Kamal Dasu wrote:
>> If the transfers are short and dest buffer or the flash address are
>> unaligned.
>
> That sounds like a DMA problem where you're trying to fall back to PIO ?
>
>> Also in case of older version of the controller there are
>> some address mapping limitations when a transfer crosses 4MB window
>> (addr + len).  So in such cases  need to fallback to normal MSPI
>> reads.
>
> But the driver can also detect this mode of failure before doing the
> transfer and call it's internal functions to perform the transfer as
> needed, right?
>
>> One other option is that controller divers implementation of
>> bcm_qspi_spi_flash_read() can return msg.retlen = 0 and the
>> m25p80_read() can fallback to normal mspi read.
>
> I'd much rather see the driver handling such detail internally instead
> of patching the core code. Moreover, if you patch the core code, the SF
> read will go - in case of a failure- all the way through the SPI
> framework only to land in the same driver, which doesn't make much sense.

Yes this is how the code was  organized before when I was making
initial commits. However I had to change it so that spi_flash_read()
can be exploited based on the review comments.  I was  handling code
internally using spi generic msg handling code for mspi transfer
fallback, but I was told that this code did not belong in the
controller driver. Hence the current implementation is geared towards
using spi transfer_one() in case of mspi transfers, without bothering
with how the messages are formed and pumped by the spi layer. If I
reorganize I am back to where I was before. Yes the code lands in the
same driver but it returns back to the m25p80 and uses the  normal
mspi reads.

> btw please do NOT top-post:
> http://www.arm.linux.org.uk/mailinglists/etiquette.php#e3
>

Sorry about that, I will make sure from this point forward I do not top-post.

>> Kamal
>>
>>
>>
>> On Tue, Jan 24, 2017 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>>>> "ret can never be > 0 , it is only 0 or negative "
>>>>
>>>> I can fix this.
>>>>
>>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>>>> pollute core code with workarounds. If you do support this sort of
>>>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>>>> circumstances, do it from the driver, not here.
>>>>>>
>>>>>> I guess the other half of m25p80_read can be factored out and used as
>>>>>> fallback from either m25p80_read or the controller driver.
>>>>>
>>>>> I think I see what you mean, but care to show an RFC patch ?
>>>>>
>>>>> --
>>>>
>>>> Its not the controller driver, but he hardware limitation with older
>>>> controller version. I have tried to see how I can do this better,
>>>> however when spi_flash_read() is called  cannot handle it within my
>>>> driver without returning from the function. I went over this with Mark
>>>> previously and this current solution seemed reasonable. Any other
>>>> solution outside of the generic driver would replicate a lot of code
>>>> unnecessarily.
>>>
>>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
>>> function could be the solution and invoking the second part from the
>>> driver if applicable, but this cannot work because the driver does not
>>> know when it's interacting with SPI NOR and when with something else .
>>>
>>> Can you tell me about the conditions under which the bcm controller
>>> fails and should fall back to standard spi read ?
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
>
>
> --
> Best regards,
> Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Marek Vasut]

On 01/25/2017 06:10 PM, Kamal Dasu wrote:
> On Wed, Jan 25, 2017 at 11:39 AM, Marek Vasut <marex@denx.de> wrote:
>> On 01/25/2017 05:28 PM, Kamal Dasu wrote:
>>> If the transfers are short and dest buffer or the flash address are
>>> unaligned.
>>
>> That sounds like a DMA problem where you're trying to fall back to PIO ?
>>
>>> Also in case of older version of the controller there are
>>> some address mapping limitations when a transfer crosses 4MB window
>>> (addr + len).  So in such cases  need to fallback to normal MSPI
>>> reads.
>>
>> But the driver can also detect this mode of failure before doing the
>> transfer and call it's internal functions to perform the transfer as
>> needed, right?
>>
>>> One other option is that controller divers implementation of
>>> bcm_qspi_spi_flash_read() can return msg.retlen = 0 and the
>>> m25p80_read() can fallback to normal mspi read.
>>
>> I'd much rather see the driver handling such detail internally instead
>> of patching the core code. Moreover, if you patch the core code, the SF
>> read will go - in case of a failure- all the way through the SPI
>> framework only to land in the same driver, which doesn't make much sense.
> 
> Yes this is how the code was  organized before when I was making
> initial commits. However I had to change it so that spi_flash_read()
> can be exploited based on the review comments.

This is OK, using spi_flash_read() is no problem.

> I was  handling code
> internally using spi generic msg handling code for mspi transfer
> fallback, but I was told that this code did not belong in the
> controller driver. Hence the current implementation is geared towards
> using spi transfer_one() in case of mspi transfers, without bothering
> with how the messages are formed and pumped by the spi layer. If I
> reorganize I am back to where I was before. Yes the code lands in the
> same driver but it returns back to the m25p80 and uses the  normal
> mspi reads.

Another option would be to split bcm_qspi_transfer_one into function
which does the actual CS manipulation and data transfer AND the SPI
interface shim , then invoke the data transfer bit from
bcm_qspi_flash_read () in case mspi_read = true , no ?

>> btw please do NOT top-post:
>> http://www.arm.linux.org.uk/mailinglists/etiquette.php#e3
>>
> 
> Sorry about that, I will make sure from this point forward I do not top-post.
> 
>>> Kamal
>>>
>>>
>>>
>>> On Tue, Jan 24, 2017 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
>>>> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>>>>> "ret can never be > 0 , it is only 0 or negative "
>>>>>
>>>>> I can fix this.
>>>>>
>>>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>>>>> pollute core code with workarounds. If you do support this sort of
>>>>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>>>>> circumstances, do it from the driver, not here.
>>>>>>>
>>>>>>> I guess the other half of m25p80_read can be factored out and used as
>>>>>>> fallback from either m25p80_read or the controller driver.
>>>>>>
>>>>>> I think I see what you mean, but care to show an RFC patch ?
>>>>>>
>>>>>> --
>>>>>
>>>>> Its not the controller driver, but he hardware limitation with older
>>>>> controller version. I have tried to see how I can do this better,
>>>>> however when spi_flash_read() is called  cannot handle it within my
>>>>> driver without returning from the function. I went over this with Mark
>>>>> previously and this current solution seemed reasonable. Any other
>>>>> solution outside of the generic driver would replicate a lot of code
>>>>> unnecessarily.
>>>>
>>>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
>>>> function could be the solution and invoking the second part from the
>>>> driver if applicable, but this cannot work because the driver does not
>>>> know when it's interacting with SPI NOR and when with something else .
>>>>
>>>> Can you tell me about the conditions under which the bcm controller
>>>> fails and should fall back to standard spi read ?
>>>>
>>>> --
>>>> Best regards,
>>>> Marek Vasut
>>
>>
>> --
>> Best regards,
>> Marek Vasut


-- 
Best regards,
Marek Vasut

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[R, Vignesh]

On 1/25/2017 9:58 PM, Kamal Dasu wrote:
> If the transfers are short and dest buffer or the flash address are
> unaligned. 

How about using using bounce buffer here? Using bounce buffer and doing
double copy might still be faster than PIO.

> Also in case of older version of the controller there are
> some address mapping limitations when a transfer crosses 4MB window
> (addr + len).  So in such cases  need to fallback to normal MSPI
> reads.
> 

At least, this can be handled by breaking transfers and handle what
falls within 4MB range. And then update msg.retlen equal to number of
bytes read.

Regards
Vignesh


> One other option is that controller divers implementation of
> bcm_qspi_spi_flash_read() can return msg.retlen = 0 and the
> m25p80_read() can fallback to normal mspi read.
> 
> Kamal
> 
> 
> 
> On Tue, Jan 24, 2017 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
>> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>>> "ret can never be > 0 , it is only 0 or negative "
>>>
>>> I can fix this.
>>>
>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>>> pollute core code with workarounds. If you do support this sort of
>>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>>> circumstances, do it from the driver, not here.
>>>>>
>>>>> I guess the other half of m25p80_read can be factored out and used as
>>>>> fallback from either m25p80_read or the controller driver.
>>>>
>>>> I think I see what you mean, but care to show an RFC patch ?
>>>>
>>>> --
>>>
>>> Its not the controller driver, but he hardware limitation with older
>>> controller version. I have tried to see how I can do this better,
>>> however when spi_flash_read() is called  cannot handle it within my
>>> driver without returning from the function. I went over this with Mark
>>> previously and this current solution seemed reasonable. Any other
>>> solution outside of the generic driver would replicate a lot of code
>>> unnecessarily.
>>
>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
>> function could be the solution and invoking the second part from the
>> driver if applicable, but this cannot work because the driver does not
>> know when it's interacting with SPI NOR and when with something else .
>>
>> Can you tell me about the conditions under which the bcm controller
>> fails and should fall back to standard spi read ?
>>
>> --
>> Best regards,
>> Marek Vasut
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

-- 
Regards
Vignesh

Re: [PATCH, 1/2] mtd: m25p80: Let m25p80_read() fallback to spi transfer

[Kamal Dasu]

On Sun, Jan 29, 2017 at 6:16 AM, R, Vignesh <vigneshr@ti.com> wrote:
>
>
> On 1/25/2017 9:58 PM, Kamal Dasu wrote:
>> If the transfers are short and dest buffer or the flash address are
>> unaligned.
>
> How about using using bounce buffer here? Using bounce buffer and doing
> double copy might still be faster than PIO.
>
>> Also in case of older version of the controller there are
>> some address mapping limitations when a transfer crosses 4MB window
>> (addr + len).  So in such cases  need to fallback to normal MSPI
>> reads.
>>
>
> At least, this can be handled by breaking transfers and handle what
> falls within 4MB range. And then update msg.retlen equal to number of
> bytes read.
>

 I did give a thought that idea as well. In bcm driver  these are just
exception and a rare cases mostly with tests that issue short reads
like mtd_debug read etc. So I think I will go for reverting to mspi
reads using existing framework. Its easier this way. So will go with
Marek's suggestion.

> Regards
> Vignesh
>
Thanks
Kamal
>
>> One other option is that controller divers implementation of
>> bcm_qspi_spi_flash_read() can return msg.retlen = 0 and the
>> m25p80_read() can fallback to normal mspi read.
>>
>> Kamal
>>
>>
>>
>> On Tue, Jan 24, 2017 at 9:08 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 01/24/2017 12:41 AM, Kamal Dasu wrote:
>>>> "ret can never be > 0 , it is only 0 or negative "
>>>>
>>>> I can fix this.
>>>>
>>>>>>> This looks really fragile and special-casing EINVAL here doesn't scale.
>>>>>>> But still, if your controller driver is buggy, fix the driver, do not
>>>>>>> pollute core code with workarounds. If you do support this sort of
>>>>>>> accelerated read and it fails, it means something is seriously wrong.
>>>>>>> If you need to invoke regular SPI reads to complete under some obscure
>>>>>>> circumstances, do it from the driver, not here.
>>>>>>
>>>>>> I guess the other half of m25p80_read can be factored out and used as
>>>>>> fallback from either m25p80_read or the controller driver.
>>>>>
>>>>> I think I see what you mean, but care to show an RFC patch ?
>>>>>
>>>>> --
>>>>
>>>> Its not the controller driver, but he hardware limitation with older
>>>> controller version. I have tried to see how I can do this better,
>>>> however when spi_flash_read() is called  cannot handle it within my
>>>> driver without returning from the function. I went over this with Mark
>>>> previously and this current solution seemed reasonable. Any other
>>>> solution outside of the generic driver would replicate a lot of code
>>>> unnecessarily.
>>>
>>> Hmmm, I kinda see the problem. I was thinking splitting the m25p80_read
>>> function could be the solution and invoking the second part from the
>>> driver if applicable, but this cannot work because the driver does not
>>> know when it's interacting with SPI NOR and when with something else .
>>>
>>> Can you tell me about the conditions under which the bcm controller
>>> fails and should fall back to standard spi read ?
>>>
>>> --
>>> Best regards,
>>> Marek Vasut
>>
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>>
>
> --
> Regards
> Vignesh