| Message ID | 1447133399-25658-2-git-send-email-vigneshr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vignesh, Sorry for the late review. I did not have time to review much back when you submitted your first RFCs for this. On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote: > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index cce80e6dc7d1..2f2c431b8917 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -361,6 +361,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) > * @handle_err: the subsystem calls the driver to handle an error that occurs > * in the generic implementation of transfer_one_message(). > * @unprepare_message: undo any work done by prepare_message(). > + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory. > + * Flash drivers (like m25p80) can request memory > + * mapped read via this method. This interface > + * should only be used by mtd flashes and cannot be > + * used by other spi devices. > * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS > * number. Any individual value may be -ENOENT for CS lines that > * are not GPIOs (driven by the SPI controller itself). > @@ -507,6 +512,11 @@ struct spi_master { > struct spi_message *message); > int (*unprepare_message)(struct spi_master *master, > struct spi_message *message); > + int (*spi_mtd_mmap_read)(struct spi_device *spi, > + loff_t from, size_t len, > + size_t *retlen, u_char *buf, > + u8 read_opcode, u8 addr_width, > + u8 dummy_bytes); Is this API really sufficient? There are actually quite a few other flash-related parameters that might be relevant to a controller. I presume you happen not hit them because of the particular cases you're using this for right now, but: * How many I/O lines are being used? These can vary depending on the type of flash and the number of I/O lines supported by the controller and connected on the board. * The previous point can vary across parts of the message. There are various combinations of 1/2/4 lines used for opcode/address/data. We only support a few of those combinations in m25p80 right now, but you're not specifying any of that in this API. I guess you're just making assumptions? (BTW, I think there are others having problems with the difference between different "quad" modes on Micron flash; I haven't sorted through all the discussion there.) There are typically both flash device and SPI controller constraints on this question, so there needs to be some kind of negotiation involved, I expect. Or at least, the SPI master needs to expose which modes it can support with this flash-read API. Also, this API doesn't actually have anything to do with memory mapping. It has to do with the de facto standard flash protocol. So I don't think mmap belongs in the name; it should be something about flash. (I know of at least one other controller that could probably use this API, excpet it doesn't use memory mapping to accomplish the accelerated flash read.) > > /* > * These hooks are for drivers that use a generic implementation Brian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brain, On 11/11/2015 4:53 AM, Brian Norris wrote: > Hi Vignesh, > > Sorry for the late review. I did not have time to review much back when > you submitted your first RFCs for this. > > On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote: >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h >> index cce80e6dc7d1..2f2c431b8917 100644Hi >> --- a/include/linux/spi/spi.h >> +++ b/include/linux/spi/spi.h >> @@ -361,6 +361,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) >> * @handle_err: the subsystem calls the driver to handle an error that occurs >> * in the generic implementation of transfer_one_message(). >> * @unprepare_message: undo any work done by prepare_message(). >> + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory. >> + * Flash drivers (like m25p80) can request memory >> + * mapped read via this method. This interface >> + * should only be used by mtd flashes and cannot be >> + * used by other spi devices. >> * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS >> * number. Any individual value may be -ENOENT for CS lines that >> * are not GPIOs (driven by the SPI controller itself). >> @@ -507,6 +512,11 @@ struct spi_master { >> struct spi_message *message); >> int (*unprepare_message)(struct spi_master *master, >> struct spi_message *message); >> + int (*spi_mtd_mmap_read)(struct spi_device *spi, >> + loff_t from, size_t len, >> + size_t *retlen, u_char *buf, >> + u8 read_opcode, u8 addr_width, >> + u8 dummy_bytes); > > Is this API really sufficient? There are actually quite a few other > flash-related parameters that might be relevant to a controller. I > presume you happen not hit them because of the particular cases you're > using this for right now, but: > > * How many I/O lines are being used? These can vary depending on the > type of flash and the number of I/O lines supported by the controller > and connected on the board. > This API communicates whatever data is currently communicated via spi_message through spi_sync/transfer_one interfaces. > * The previous point can vary across parts of the message. There are > various combinations of 1/2/4 lines used for opcode/address/data. We > only support a few of those combinations in m25p80 right now, but > you're not specifying any of that in this API. I guess you're just > making assumptions? (BTW, I think there are others having problems > with the difference between different "quad" modes on Micron flash; I > haven't sorted through all the discussion there.) > How is the spi controller currently being made aware of this via m25p80_read / spi_sync() interface? AFAIK, mode field of spi_device struct tell whether to do normal/dual/quad read but there is no info communicated wrt 1/2/4 opcode/address/data combinations. And there is no info indicating capabilities of spi-master wrt no of IO lines for opcode/address/data that it can support. > There are typically both flash device and SPI controller constraints > on this question, so there needs to be some kind of negotiation > involved, I expect. Or at least, the SPI master needs to expose which > modes it can support with this flash-read API. > If spi-master capabilities are known then spi_mmap_read_supported() (or a new function perhaps) can be used for negotiation. These capabilities can be added incrementally once ability to specify spi-master capabilities are in place. > Also, this API doesn't actually have anything to do with memory mapping. > It has to do with the de facto standard flash protocol. So I don't think > mmap belongs in the name; it should be something about flash. (I know of > at least one other controller that could probably use this API, excpet > it doesn't use memory mapping to accomplish the accelerated flash read.) > As far as TI QSPI controller is concerned, the accelerated read happens via mmap port whereby a predefined memory address space of SoC is exposed as QSPI mmap region. This region can be accessed like normal RAM(via memcpy()) and the QSPI controller interface takes care of fetching data from flash on SPI bus automatically hence, I named it as above. But, I have no hard feelings if it needs to be generalized to spi_mtd_read() or something else. Regards Vignesh -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Nov 11, 2015 at 12:20:46PM +0530, R, Vignesh wrote: > On 11/11/2015 4:53 AM, Brian Norris wrote: > > On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote: > >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > >> index cce80e6dc7d1..2f2c431b8917 100644Hi > >> --- a/include/linux/spi/spi.h > >> +++ b/include/linux/spi/spi.h > >> @@ -361,6 +361,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) > >> * @handle_err: the subsystem calls the driver to handle an error that occurs > >> * in the generic implementation of transfer_one_message(). > >> * @unprepare_message: undo any work done by prepare_message(). > >> + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory. > >> + * Flash drivers (like m25p80) can request memory > >> + * mapped read via this method. This interface > >> + * should only be used by mtd flashes and cannot be > >> + * used by other spi devices. > >> * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS > >> * number. Any individual value may be -ENOENT for CS lines that > >> * are not GPIOs (driven by the SPI controller itself). > >> @@ -507,6 +512,11 @@ struct spi_master { > >> struct spi_message *message); > >> int (*unprepare_message)(struct spi_master *master, > >> struct spi_message *message); > >> + int (*spi_mtd_mmap_read)(struct spi_device *spi, > >> + loff_t from, size_t len, > >> + size_t *retlen, u_char *buf, > >> + u8 read_opcode, u8 addr_width, > >> + u8 dummy_bytes); > > > > Is this API really sufficient? There are actually quite a few other > > flash-related parameters that might be relevant to a controller. I > > presume you happen not hit them because of the particular cases you're > > using this for right now, but: > > > > * How many I/O lines are being used? These can vary depending on the > > type of flash and the number of I/O lines supported by the controller > > and connected on the board. > > > > This API communicates whatever data is currently communicated via > spi_message through spi_sync/transfer_one interfaces. No it doesn't. A spi_message consists of a list of spi_transfer's, and each spi_transfer has tx_nbits and rx_nbits fields. > > * The previous point can vary across parts of the message. There are > > various combinations of 1/2/4 lines used for opcode/address/data. We > > only support a few of those combinations in m25p80 right now, but > > you're not specifying any of that in this API. I guess you're just > > making assumptions? (BTW, I think there are others having problems > > with the difference between different "quad" modes on Micron flash; I > > haven't sorted through all the discussion there.) > > > > How is the spi controller currently being made aware of this via > m25p80_read / spi_sync() interface? AFAIK, mode field of spi_device > struct tell whether to do normal/dual/quad read but there is no info > communicated wrt 1/2/4 opcode/address/data combinations. Yes there is. m25p80 fills out spi_transfer::rx_nbits. Currently, we only use this for the data portion, but it's possible to support more lines for the address and opcode portions too, using the rx_nbits for the opcode and address spi_transfer struct(s) (currently, m25p80_read() uses 2 spi_transfers per message, where the first one contains opcode + address + dummy on a single line, and the second transfer receives the data on 1, 2, or 4 lines). > And there is no > info indicating capabilities of spi-master wrt no of IO lines for > opcode/address/data that it can support. For a true SPI controller, there is no need to specify something different for opcode/address/data, since all those are treated the same; they're just bits on 1, 2, or 4 lines. So the SPI_{TX,RX}_{DUAL,QUAD} mode flags in struct spi_master tell m25p80 all it needs to know. > > There are typically both flash device and SPI controller constraints > > on this question, so there needs to be some kind of negotiation > > involved, I expect. Or at least, the SPI master needs to expose which > > modes it can support with this flash-read API. > > > > If spi-master capabilities are known For generic SPI handling, these are already known. But now you're adding flash-specific capabilities, and I'm not going to assume that all accelerated-read (e.g., your TI mmap'ed flash read) support all the same modes as your generic modes. So, which modes does your mmap'ed read handle? 1/1/1? 1/1/2? 1/1/4? 4/4/4? (where x/y/z means x lines for opcode, y lines for address, and z lines for data) > then spi_mmap_read_supported() (or > a new function perhaps) can be used for negotiation. These capabilities > can be added incrementally once ability to specify spi-master > capabilities are in place. spi_master capabilities are already in place. So now you need to describe them for your new interface too. > > Also, this API doesn't actually have anything to do with memory mapping. > > It has to do with the de facto standard flash protocol. So I don't think > > mmap belongs in the name; it should be something about flash. (I know of > > at least one other controller that could probably use this API, excpet > > it doesn't use memory mapping to accomplish the accelerated flash read.) > > > > As far as TI QSPI controller is concerned, the accelerated read happens > via mmap port whereby a predefined memory address space of SoC is > exposed as QSPI mmap region. This region can be accessed like normal > RAM(via memcpy()) and the QSPI controller interface takes care of > fetching data from flash on SPI bus automatically I understand all that, but the API as it currently stands is not tied to that implementation at all. > hence, I named it as > above. But, I have no hard feelings if it needs to be generalized to > spi_mtd_read() or something else. Maybe spi_flash_read()? It's technically not limited to MTD, though it probably would only be used there. (And please, don't tread on the 'spi_nor_' prefix, as we already have a related, but distinct, framework using that.) Brian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
In addition to my other comments: On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote: > In addition to providing direct access to SPI bus, some spi controller > hardwares (like ti-qspi) provide special memory mapped port > to accesses SPI flash devices in order to increase read performance. > This means the controller can automatically send the SPI signals > required to read data from the SPI flash device. > For this, spi controller needs to know flash specific information like > read command to use, dummy bytes and address width. Once these settings > are populated in hardware registers, any read accesses to flash's memory > map region(SoC specific) through memcpy (or mem-to mem DMA copy) will be > handled by controller hardware. The hardware will automatically generate > SPI signals required to read data from flash and present it to CPU/DMA. > > Introduce spi_mtd_mmap_read() interface to support memory mapped read > over SPI flash devices. SPI master drivers can implement this callback to > support memory mapped read interfaces. m25p80 flash driver and other > flash drivers can call this to request memory mapped read. The interface > should only be used MTD flashes and cannot be used with other SPI devices. > > Signed-off-by: Vignesh R <vigneshr@ti.com> > --- ... > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index cce80e6dc7d1..2f2c431b8917 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -361,6 +361,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) > * @handle_err: the subsystem calls the driver to handle an error that occurs > * in the generic implementation of transfer_one_message(). > * @unprepare_message: undo any work done by prepare_message(). > + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory. > + * Flash drivers (like m25p80) can request memory > + * mapped read via this method. This interface > + * should only be used by mtd flashes and cannot be > + * used by other spi devices. > * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS > * number. Any individual value may be -ENOENT for CS lines that > * are not GPIOs (driven by the SPI controller itself). > @@ -507,6 +512,11 @@ struct spi_master { > struct spi_message *message); > int (*unprepare_message)(struct spi_master *master, > struct spi_message *message); > + int (*spi_mtd_mmap_read)(struct spi_device *spi, > + loff_t from, size_t len, > + size_t *retlen, u_char *buf, > + u8 read_opcode, u8 addr_width, > + u8 dummy_bytes); This is seeming to be a longer and longer list of arguments. I know MTD has a bad habit of long argument lists (which then cause a ton of unnecessary churn when things need changed in the API), but perhaps we can limit the damage to the SPI layer. Perhaps this deserves a struct to encapsulate all the flash read arguments? Like: struct spi_flash_read_message { loff_t from; size_t len; size_t *retlen; void *buf; u8 read_opcode; u8 addr_width; u8 dummy_bits; // additional fields to describe rx_nbits for opcode/addr/data }; struct spi_master { ... int (*spi_flash_read)(struct spi_device *spi, struct spi_flash_message *msg); }; > > /* > * These hooks are for drivers that use a generic implementation ... Brian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, On 11/12/2015 12:54 AM, Brian Norris wrote: > In addition to my other comments: > [...] >> + int (*spi_mtd_mmap_read)(struct spi_device *spi, >> + loff_t from, size_t len, >> + size_t *retlen, u_char *buf, >> + u8 read_opcode, u8 addr_width, >> + u8 dummy_bytes); > > This is seeming to be a longer and longer list of arguments. I know MTD > has a bad habit of long argument lists (which then cause a ton of > unnecessary churn when things need changed in the API), but perhaps we > can limit the damage to the SPI layer. Perhaps this deserves a struct to > encapsulate all the flash read arguments? Like: > > struct spi_flash_read_message { > loff_t from; > size_t len; > size_t *retlen; > void *buf; > u8 read_opcode; > u8 addr_width; > u8 dummy_bits; > // additional fields to describe rx_nbits for opcode/addr/data > }; > > struct spi_master { > ... > int (*spi_flash_read)(struct spi_device *spi, > struct spi_flash_message *msg); > }; Yeah.. I think struct encapsulation helps, this can also be used to pass sg lists for dma in future. I will rework the series with your suggestion to include nbits for opcode/addr/data. Also, will add validation logic (similar to __spi_validate()) to check whether master supports dual/quad mode for opcode/addr/data. I am planning to add this validation code to spi_flash_read_validate(in place of spi_mmap_read_supported()) Thanks!
?On 11-11-15 07:50, R, Vignesh wrote: > Hi Brain, > > On 11/11/2015 4:53 AM, Brian Norris wrote: >> Hi Vignesh, ... >> Also, this API doesn't actually have anything to do with memory mapping. >> It has to do with the de facto standard flash protocol. So I don't think >> mmap belongs in the name; it should be something about flash. (I know of >> at least one other controller that could probably use this API, excpet >> it doesn't use memory mapping to accomplish the accelerated flash read.) The Zynq has a similar way of accessing QSPI flash through a memory map. The memory window is only 16MB, so some form of paging would be needed. It's why I have been following this thread with great interest, since the QSPI performance on the Zynq is way below what it could potentially be. > As far as TI QSPI controller is concerned, the accelerated read happens > via mmap port whereby a predefined memory address space of SoC is > exposed as QSPI mmap region. This region can be accessed like normal > RAM(via memcpy()) and the QSPI controller interface takes care of > fetching data from flash on SPI bus automatically hence, I named it as > above. But, I have no hard feelings if it needs to be generalized to > spi_mtd_read() or something else. I know that on the Zynq, you can even let the DMA controller access the QSPI flash via this memory mapping. The QSPI controller itself doesn't support any DMA at all. If something similar applies to the TI platform (most of the TI procs have nice DMA controllers) one could go one step further and implement a generic DMA-through-mmap access to QSPI flash. Mike. Kind regards, Mike Looijmans System Expert TOPIC Embedded Products Eindhovenseweg 32-C, NL-5683 KH Best Postbus 440, NL-5680 AK Best Telefoon: +31 (0) 499 33 69 79 Telefax: +31 (0) 499 33 69 70 E-mail: mike.looijmans@topicproducts.com Website: www.topicproducts.com Please consider the environment before printing this e-mail Visit us at : Aerospace Electrical Systems Expo Europe which will be held from 17.11.2015 till 19.11.2015, Findorffstrasse 101 Bremen, Germany, Hall 5, stand number C65 http://www.aesexpo.eu -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, Le 11/11/2015 08:20, Brian Norris a écrit : > Hi, > > On Wed, Nov 11, 2015 at 12:20:46PM +0530, R, Vignesh wrote: >> On 11/11/2015 4:53 AM, Brian Norris wrote: >>> On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote: >>>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h >>>> index cce80e6dc7d1..2f2c431b8917 100644Hi >>>> --- a/include/linux/spi/spi.h >>>> +++ b/include/linux/spi/spi.h >>>> @@ -361,6 +361,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) >>>> * @handle_err: the subsystem calls the driver to handle an error that occurs >>>> * in the generic implementation of transfer_one_message(). >>>> * @unprepare_message: undo any work done by prepare_message(). >>>> + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory. >>>> + * Flash drivers (like m25p80) can request memory >>>> + * mapped read via this method. This interface >>>> + * should only be used by mtd flashes and cannot be >>>> + * used by other spi devices. >>>> * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS >>>> * number. Any individual value may be -ENOENT for CS lines that >>>> * are not GPIOs (driven by the SPI controller itself). >>>> @@ -507,6 +512,11 @@ struct spi_master { >>>> struct spi_message *message); >>>> int (*unprepare_message)(struct spi_master *master, >>>> struct spi_message *message); >>>> + int (*spi_mtd_mmap_read)(struct spi_device *spi, >>>> + loff_t from, size_t len, >>>> + size_t *retlen, u_char *buf, >>>> + u8 read_opcode, u8 addr_width, >>>> + u8 dummy_bytes); >>> >>> Is this API really sufficient? There are actually quite a few other >>> flash-related parameters that might be relevant to a controller. I >>> presume you happen not hit them because of the particular cases you're >>> using this for right now, but: >>> >>> * How many I/O lines are being used? These can vary depending on the >>> type of flash and the number of I/O lines supported by the controller >>> and connected on the board. >>> >> >> This API communicates whatever data is currently communicated via >> spi_message through spi_sync/transfer_one interfaces. > > No it doesn't. A spi_message consists of a list of spi_transfer's, and > each spi_transfer has tx_nbits and rx_nbits fields. > >>> * The previous point can vary across parts of the message. There are >>> various combinations of 1/2/4 lines used for opcode/address/data. We >>> only support a few of those combinations in m25p80 right now, but >>> you're not specifying any of that in this API. I guess you're just >>> making assumptions? (BTW, I think there are others having problems >>> with the difference between different "quad" modes on Micron flash; I >>> haven't sorted through all the discussion there.) >>> >> >> How is the spi controller currently being made aware of this via >> m25p80_read / spi_sync() interface? AFAIK, mode field of spi_device >> struct tell whether to do normal/dual/quad read but there is no info >> communicated wrt 1/2/4 opcode/address/data combinations. > > Yes there is. m25p80 fills out spi_transfer::rx_nbits. Currently, we > only use this for the data portion, but it's possible to support more > lines for the address and opcode portions too, using the rx_nbits for > the opcode and address spi_transfer struct(s) (currently, m25p80_read() > uses 2 spi_transfers per message, where the first one contains opcode + > address + dummy on a single line, and the second transfer receives the > data on 1, 2, or 4 lines). > In September I've sent a series of patches to enhance the support of QSPI flash memories. Patch 4 was dedicated to the m25p80 driver and set the rx_nbits / tx_nbits fields of spi_transfer struct(s) in order to configure the number of I/O lines independently for the opcode, address and data parts. The work was done for m25p80_read() but also for _read_reg(), _write_reg() and _write(). The patched m25p80 driver was then tested with an at25 memory to check non- regression. This series of patches also added 4 enum spi_protocol fields inside struct spi_nor so the spi-nor framework can tell the (Q)SPI controller driver what SPI protocol should be use for erase, read, write and register read/write operations, depending on the memory manufacturer and the command opcode. This was done to better support Micron, Spansion and Macronix QSPI memories. I have tested the series with Micron QSPI memories and Atmel QSPI controller and I guess Marek also tested it on his side with Spansion QSPI memories and another QSPI controller. So if it can help other developers to develop QSPI controller drivers, the series is still available there: for the whole series: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371170.html for patch 4 (depends on patch 2 for enum spi_protocol): http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371173.html Best Regards, Cyrille -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Brian, On 11/13/2015 09:35 PM, Cyrille Pitchen wrote: [...] > > In September I've sent a series of patches to enhance the support of QSPI flash > memories. Patch 4 was dedicated to the m25p80 driver and set the > rx_nbits / tx_nbits fields of spi_transfer struct(s) in order to configure the > number of I/O lines independently for the opcode, address and data parts. > The work was done for m25p80_read() but also for _read_reg(), _write_reg() and > _write(). > The patched m25p80 driver was then tested with an at25 memory to check non- > regression. > > This series of patches also added 4 enum spi_protocol fields inside struct > spi_nor so the spi-nor framework can tell the (Q)SPI controller driver what SPI > protocol should be use for erase, read, write and register read/write > operations, depending on the memory manufacturer and the command opcode. > This was done to better support Micron, Spansion and Macronix QSPI memories. > > I have tested the series with Micron QSPI memories and Atmel QSPI controller > and I guess Marek also tested it on his side with Spansion QSPI memories and > another QSPI controller. > > So if it can help other developers to develop QSPI controller drivers, the > series is still available there: > > for the whole series: > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371170.html > > for patch 4 (depends on patch 2 for enum spi_protocol): > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371173.html > Should I rebase my next version on top of above patches by Cyrille or shall I post on top of 4.4-rc1?
Hi, On Tue, Nov 17, 2015 at 12:02:29PM +0530, Vignesh R wrote: > On 11/13/2015 09:35 PM, Cyrille Pitchen wrote: > > > > In September I've sent a series of patches to enhance the support of QSPI flash > > memories. Patch 4 was dedicated to the m25p80 driver and set the > > rx_nbits / tx_nbits fields of spi_transfer struct(s) in order to configure the > > number of I/O lines independently for the opcode, address and data parts. > > The work was done for m25p80_read() but also for _read_reg(), _write_reg() and > > _write(). > > The patched m25p80 driver was then tested with an at25 memory to check non- > > regression. > > > > This series of patches also added 4 enum spi_protocol fields inside struct > > spi_nor so the spi-nor framework can tell the (Q)SPI controller driver what SPI > > protocol should be use for erase, read, write and register read/write > > operations, depending on the memory manufacturer and the command opcode. > > This was done to better support Micron, Spansion and Macronix QSPI memories. > > > > I have tested the series with Micron QSPI memories and Atmel QSPI controller > > and I guess Marek also tested it on his side with Spansion QSPI memories and > > another QSPI controller. > > > > So if it can help other developers to develop QSPI controller drivers, the > > series is still available there: > > > > for the whole series: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371170.html > > > > for patch 4 (depends on patch 2 for enum spi_protocol): > > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371173.html > > > > Should I rebase my next version on top of above patches by Cyrille or > shall I post on top of 4.4-rc1? I'm sorry to say I really haven't had the time to review that properly. I'm also not sure it is a true dependency for your series, as you're tackling different pieces of the puzzle. So it's mostly just a conflict, not a real direct help. So unless I'm misunderstanding, I'd suggest submitting MTD stuff against the latest l2-mtd.git (or linux-next.git; l2-mtd.git is included there), and I'll let you know if conflicts come up that need fixing. Brian -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index e2415be209d5..0448d29fefc8 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -1134,6 +1134,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) } } + mutex_lock(&master->bus_lock_mutex); trace_spi_message_start(master->cur_msg); if (master->prepare_message) { @@ -1143,6 +1144,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) "failed to prepare message: %d\n", ret); master->cur_msg->status = ret; spi_finalize_current_message(master); + mutex_unlock(&master->bus_lock_mutex); return; } master->cur_msg_prepared = true; @@ -1152,6 +1154,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) if (ret) { master->cur_msg->status = ret; spi_finalize_current_message(master); + mutex_unlock(&master->bus_lock_mutex); return; } @@ -1159,8 +1162,10 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread) if (ret) { dev_err(&master->dev, "failed to transfer one message from queue\n"); + mutex_unlock(&master->bus_lock_mutex); return; } + mutex_unlock(&master->bus_lock_mutex); } /** @@ -2327,6 +2332,35 @@ int spi_async_locked(struct spi_device *spi, struct spi_message *message) EXPORT_SYMBOL_GPL(spi_async_locked); +int spi_mtd_mmap_read(struct spi_device *spi, loff_t from, size_t len, + size_t *retlen, u_char *buf, u8 read_opcode, + u8 addr_width, u8 dummy_bytes) + +{ + struct spi_master *master = spi->master; + int ret; + + if (master->auto_runtime_pm) { + ret = pm_runtime_get_sync(master->dev.parent); + if (ret < 0) { + dev_err(&master->dev, "Failed to power device: %d\n", + ret); + goto err; + } + } + mutex_lock(&master->bus_lock_mutex); + ret = master->spi_mtd_mmap_read(spi, from, len, retlen, buf, + read_opcode, addr_width, + dummy_bytes); + mutex_unlock(&master->bus_lock_mutex); + if (master->auto_runtime_pm) + pm_runtime_put(master->dev.parent); + +err: + return ret; +} +EXPORT_SYMBOL_GPL(spi_mtd_mmap_read); + /*-------------------------------------------------------------------------*/ /* Utility methods for SPI master protocol drivers, layered on diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index cce80e6dc7d1..2f2c431b8917 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -361,6 +361,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv) * @handle_err: the subsystem calls the driver to handle an error that occurs * in the generic implementation of transfer_one_message(). * @unprepare_message: undo any work done by prepare_message(). + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory. + * Flash drivers (like m25p80) can request memory + * mapped read via this method. This interface + * should only be used by mtd flashes and cannot be + * used by other spi devices. * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS * number. Any individual value may be -ENOENT for CS lines that * are not GPIOs (driven by the SPI controller itself). @@ -507,6 +512,11 @@ struct spi_master { struct spi_message *message); int (*unprepare_message)(struct spi_master *master, struct spi_message *message); + int (*spi_mtd_mmap_read)(struct spi_device *spi, + loff_t from, size_t len, + size_t *retlen, u_char *buf, + u8 read_opcode, u8 addr_width, + u8 dummy_bytes); /* * These hooks are for drivers that use a generic implementation @@ -999,6 +1009,16 @@ static inline ssize_t spi_w8r16be(struct spi_device *spi, u8 cmd) return be16_to_cpu(result); } +/* SPI core interface for memory mapped read support */ +static inline bool spi_mmap_read_supported(struct spi_device *spi) +{ + return spi->master->spi_mtd_mmap_read ? true : false; +} + +int spi_mtd_mmap_read(struct spi_device *spi, loff_t from, size_t len, + size_t *retlen, u_char *buf, u8 read_opcode, + u8 addr_width, u8 dummy_bytes); + /*---------------------------------------------------------------------------*/ /*
In addition to providing direct access to SPI bus, some spi controller hardwares (like ti-qspi) provide special memory mapped port to accesses SPI flash devices in order to increase read performance. This means the controller can automatically send the SPI signals required to read data from the SPI flash device. For this, spi controller needs to know flash specific information like read command to use, dummy bytes and address width. Once these settings are populated in hardware registers, any read accesses to flash's memory map region(SoC specific) through memcpy (or mem-to mem DMA copy) will be handled by controller hardware. The hardware will automatically generate SPI signals required to read data from flash and present it to CPU/DMA. Introduce spi_mtd_mmap_read() interface to support memory mapped read over SPI flash devices. SPI master drivers can implement this callback to support memory mapped read interfaces. m25p80 flash driver and other flash drivers can call this to request memory mapped read. The interface should only be used MTD flashes and cannot be used with other SPI devices. Signed-off-by: Vignesh R <vigneshr@ti.com> --- v3: * Remove use of mmap_lock_mutex, use bus_lock_mutex instead. drivers/spi/spi.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/spi/spi.h | 20 ++++++++++++++++++++ 2 files changed, 54 insertions(+)