[v3,1/5] spi: introduce mmap read support for spi flash devices
diff mbox

Message ID 1447133399-25658-2-git-send-email-vigneshr@ti.com
State New
Headers show

Commit Message

Vignesh Raghavendra Nov. 10, 2015, 5:29 a.m. UTC
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(+)

Comments

Brian Norris Nov. 10, 2015, 11:23 p.m. UTC | #1
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-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vignesh Raghavendra Nov. 11, 2015, 6:50 a.m. UTC | #2
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-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Norris Nov. 11, 2015, 7:20 a.m. UTC | #3
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-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Brian Norris Nov. 11, 2015, 7:24 p.m. UTC | #4
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-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vignesh Raghavendra Nov. 12, 2015, 4:32 a.m. UTC | #5
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!
Mike Looijmans Nov. 13, 2015, 2:06 p.m. UTC | #6
?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-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Cyrille Pitchen Nov. 13, 2015, 4:05 p.m. UTC | #7
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-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vignesh Raghavendra Nov. 17, 2015, 6:32 a.m. UTC | #8
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?
Brian Norris Nov. 17, 2015, 5:48 p.m. UTC | #9
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-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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);
+
 /*---------------------------------------------------------------------------*/
 
 /*