mtd: m25p80: Let m25p80_read() fallback to spi transfer
Subsystems (3)
PATCH 1/2 mtd: m25p80: Let m25p80_read() fallback to spi transfer
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.
--- drivers/mtd/devices/m25p80.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
drivers/mtd/devices/m25p80.c +9 -2
@@ -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);
Signature · 1 line
1.9.1
PATCH 2/2 spi: bcm-qspi: Fix return code from bcm_qspi_flash_read()
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.
--- drivers/spi/spi-bcm-qspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/spi-bcm-qspi.c +1 -1
@@ -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;
Signature · 1 line
1.9.1
Re: [PATCH, 2/2] spi: bcm-qspi: Fix return code from bcm_qspi_flash_read()
Quoted 14 lines · Kamal Dasu
To properly fallback to normal mspi reads return appropriate return code to
m25p80_read()function. Changed return code to-EINVALfrom-EAGAINto match what is expected by the m25p80 driver.Signed-off-by Kamal Dasu <[email protected]>--- drivers/spi/spi-bcm-qspi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/spi/spi-bcm-qspi.c +0
-0
drivers/spi/spi-bcm-qspi.c static int bcm_qspi_flash_read(struct spi_device *spi, · line 922
Quoted 5 lines
drivers/spi/spi-bcm-qspi.c +1
-1@@ -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.
Quoted 3 lines
drivers/spi/spi-bcm-qspi.c +0
-0io_width = msg->data_nbits ? msg->data_nbits :SPI_NBITS_SINGLE ; addrlen = msg->addr_width;
Signature · 2 lines
Best regards, Marek Vasut
Quoted 17 lines · Kamal Dasu
In
m25p80_read()even thoughspi_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-EINVALso that them25p80_read()uses standard spi transfer.Signed-off-by Kamal Dasu <[email protected]>--- drivers/mtd/devices/m25p80.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/mtd/devices/m25p80.c +0
-0
drivers/mtd/devices/m25p80.c static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 160
Quoted 7 lines
drivers/mtd/devices/m25p80.c +3
-1@@ -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 .
drivers/mtd/devices/m25p80.c static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 167
Quoted 6 lines
drivers/mtd/devices/m25p80.c +5
-0+/* +* 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.
Quoted 5 lines
drivers/mtd/devices/m25p80.c +0
-1-return msg.retlen; }spi_message_init (&m);
Signature · 2 lines
Best regards, Marek Vasut
Quoted 35 lines · Marek Vasut
Quoted 17 lines · Kamal Dasu
In
m25p80_read()even thoughspi_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-EINVALso that them25p80_read()uses standard spi transfer.Signed-off-by Kamal Dasu <[email protected]>--- drivers/mtd/devices/m25p80.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/mtd/devices/m25p80.c +0
-0drivers/mtd/devices/m25p80.c static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 160
Quoted 7 lines
drivers/mtd/devices/m25p80.c +3
-1@@ -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 .
drivers/mtd/devices/m25p80.c static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 167
Quoted 6 lines
drivers/mtd/devices/m25p80.c +5
-0+/* +* some spi master drivers might need to fallback to +* normal spi transfer +*/ +if (ret != -EINVAL )return ret;
Quoted 6 lines
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
Quoted 43 lines · Michal Suchanek
Quoted 35 lines · Marek Vasut
Quoted 17 lines · Kamal Dasu
In
m25p80_read()even thoughspi_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-EINVALso that them25p80_read()uses standard spi transfer.Signed-off-by Kamal Dasu <[email protected]>--- drivers/mtd/devices/m25p80.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)drivers/mtd/devices/m25p80.c +0
-0drivers/mtd/devices/m25p80.c static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 160
Quoted 7 lines
drivers/mtd/devices/m25p80.c +3
-1@@ -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 .
drivers/mtd/devices/m25p80.c static ssize_t m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 167
Quoted 6 lines
drivers/mtd/devices/m25p80.c +5
-0+/* +* some spi master drivers might need to fallback to +* normal spi transfer +*/ +if (ret != -EINVAL )return ret;Quoted 6 lines
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.
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.
I think I see what you mean, but care to show an RFC patch ?
Signature · 2 lines
Best regards, Marek Vasut
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador 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.
Quoted 2 lines
Best regards,
Marek Vasut
Thanks
Kamal
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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 ?
Signature · 2 lines
Best regards, Marek Vasut
Quoted 27 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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.
Quoted 7 lines
Hmmm, I kinda see the problem. I was thinking splitting the
m25p80_readfunction 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
Quoted 33 lines · Michal Suchanek
Quoted 32 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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 .
Quoted 7 lines
Quoted 2 lines
Can you tell me about the conditions under which the bcm controller fails and should fall back to standard spi read ?
spi_flash_readis designed to perform whatm25p80_readdoes in a controller-specific way. So how can you get to a point when you are inspi_flash_read, it fails, and you do not know you if you can call the bottom half ofm25p80_readto 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.
Signature · 2 lines
Best regards, Marek Vasut
Hi all,
Quoted 33 lines · Michal Suchanek
Quoted 32 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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 .
Quoted 8 lines
Quoted 2 lines
Can you tell me about the conditions under which the bcm controller fails and should fall back to standard spi read ?
spi_flash_readis designed to perform whatm25p80_readdoes in a controller-specific way. So how can you get to a point when you are inspi_flash_read, it fails, and you do not know you if you can call the bottom half ofm25p80_readto 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
Quoted 4 lines
Thanks
Michal
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
Quoted 38 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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
Quoted 2 lines · Kamal Dasu
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 ?
Quoted 4 lines
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?
Quoted 3 lines
One other option is that controller divers implementation of
bcm_qspi_spi_flash_read()can return msg.retlen = 0 and them25p80_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
Quoted 43 lines
Kamal
Quoted 38 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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
Signature · 2 lines
Best regards, Marek Vasut
Kamal Dasu
maintainerQuoted 15 lines · Marek Vasut
Quoted 2 lines · Kamal Dasu
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 ?
Quoted 4 lines
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?
Quoted 8 lines
Quoted 3 lines
One other option is that controller divers implementation of
bcm_qspi_spi_flash_read()can return msg.retlen = 0 and them25p80_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.
Quoted 3 lines
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.
Quoted 48 lines
Quoted 4 lines
Kamal
Quoted 39 lines
Quoted 38 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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
Quoted 20 lines · Kamal Dasu
Quoted 19 lines · Marek Vasut
Quoted 2 lines · Kamal Dasu
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 ?
Quoted 4 lines
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?
Quoted 3 lines
One other option is that controller divers implementation of
bcm_qspi_spi_flash_read()can return msg.retlen = 0 and them25p80_read()can fallback to normal mspi read.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 9 lines
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 spitransfer_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 ?
Quoted 54 lines
Quoted 3 lines
btw please do NOT top-post:
http://www.arm.linux.org.uk/mailinglists/etiquette.php#e3Sorry about that, I will make sure from this point forward I do not top-post.
Quoted 48 lines
Quoted 4 lines
Kamal
Quoted 39 lines
Quoted 38 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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
Signature · 2 lines
Best regards, Marek Vasut
R, Vignesh
maintainerQuoted 2 lines · Kamal Dasu
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.
Quoted 5 lines
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
Quoted 52 lines
One other option is that controller divers implementation of
bcm_qspi_spi_flash_read()can return msg.retlen = 0 and them25p80_read()can fallback to normal mspi read.Kamal
Quoted 32 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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 .Quoted 6 lines
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/
Signature · 2 lines
Regards Vignesh
Quoted 15 lines · Vignesh
Quoted 2 lines · Kamal Dasu
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.
Quoted 5 lines
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.
Quoted 4 lines
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.
Quoted 3 lines
Regards
Vignesh
Thanks
Kamal
Quoted 57 lines
Quoted 45 lines
One other option is that controller divers implementation of
bcm_qspi_spi_flash_read()can return msg.retlen = 0 and them25p80_read()can fallback to normal mspi read.Kamal
Quoted 35 lines · Marek Vasut
Quoted 18 lines · Kamal Dasu
"ret can never be > 0 , it is only 0 or negative "
I can fix this.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
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.
Quoted 6 lines
Quoted 2 lines
I guess the other half of
m25p80_readcan be factored out and used as fallback from eitherm25p80_reador the controller driver.I think I see what you mean, but care to show an RFC patch ?
--
Quoted 7 lines
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_readfunction 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 ?
Quoted 8 lines
Quoted 3 lines
--
Best regards,
Marek Vasut______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/--
Regards
Vignesh