| Message ID | 1484949023-2085-1-git-send-email-kdasu.kdev@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Boris Brezillon |
| Headers | show |
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); >
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
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 ?
"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
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 ?
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
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.
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 >
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
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
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
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
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/ >
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
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);
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(-)