[RFC PATCH 0/5] Add memory mapped read support for TI QSPI.

[RFC PATCH 0/5] Add memory mapped read support for TI QSPI.

[Vignesh R]

This patch series adds support for memory mapped reads for TI QSPI
driver.

TI QSPI controller has memory mapped port (SFI translator interface [1])
through which SPI flash memories can be read using memcpy call. SFI
translator takes care of generating appropriate SPI signals to read data
from flash. This interface works only with SPI flash memories and cannot
be used with other SPI devices. To use memory mapped port, the
controller is switched to memory mapped interface by writing to
QSPI_SPI_SWITCH_REG. The read_opcode, read mode, dummy bytes are set in
QSPI_SPI_SETUPx_REG. Once switched, the SPI flash is available to SoC to
read at specific address. This interface is disabled once memory mapped
read is complete. For write, erase and interaction with non-flash SPI
devices normal SPI interface is used. 

The m25p80 driver sets use_mmap_read flag in spi-message struct passed
to spi-ti-qspi so as to indicate the read request is from mtd layer.
spi-ti-qspi driver switches to memory mapped mode and does memcpy based
on use_mmap_read flag.

The read performace increased from ~100kB/s to ~2.5MB/s on DRA74 EVM.

Tested on DRA74 EVM with spansion S25FL256S flash.
Tested on AM437x sk evm with macronix MX66l51235l flash.

[1] http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf Section 24.5.4 QSPI
Functional Description

Vignesh R (5):
  spi: introduce flag for memory mapped read
  spi: spi-ti-qspi: Add memory mapped read support
  mtd: devices: m25p80: set flag to request memory mapped read
  ARM: dts: DRA7: Add memory map region entries for qspi
  ARM: dts: AM4372: Add memory map region entries for qspi

 arch/arm/boot/dts/am4372.dtsi |   4 +-
 arch/arm/boot/dts/dra7.dtsi   |   6 +-
 drivers/mtd/devices/m25p80.c  |   3 +
 drivers/spi/spi-ti-qspi.c     | 129 ++++++++++++++++++++++++++++++++++++++++--
 include/linux/spi/spi.h       |   3 +
 5 files changed, 138 insertions(+), 7 deletions(-)

-- 
2.4.6

[RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Vignesh R]

TI QSPI controller has SFI translator which exposes entire flash memory
as memory mapped region for read. With this interface, the CPU
can copy data from flash using normal memcpy call. SFI translator
takes care of generating appropriate SPI signals to read data from
flash. This interface works only with SPI flash memories and cannot be
used with other SPI devices.
Introduce use_mmap_read field in spi_message struct. This can be set by
mtd devices (m25p80) to indicate to spi-master (ti-qspi) to perform
memory mapped read. This helps to distinguish whether the spi-message is
from mtd layer(hence mmap read is possible) or by other spi devices.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 include/linux/spi/spi.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index d673072346f2..f1a0329ee63f 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -640,6 +640,8 @@ struct spi_transfer {
  * @actual_length: the total number of bytes that were transferred in all
  *	successful segments
  * @status: zero for success, else negative errno
+ * @use_mmap_mode: Indicate to spi master to perform memory mapped
+ *	read if possible.
  * @queue: for use by whichever driver currently owns the message
  * @state: for use by whichever driver currently owns the message
  *
@@ -681,6 +683,7 @@ struct spi_message {
 	unsigned		frame_length;
 	unsigned		actual_length;
 	int			status;
+	bool			use_mmap_mode;
 
 	/* for optional use by whatever driver currently owns the
 	 * spi_message ...  between calls to spi_async and then later
-- 
2.4.6

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 751 bytes --]

On Tue, Jul 28, 2015 at 02:11:12PM +0530, Vignesh R wrote:

> Introduce use_mmap_read field in spi_message struct. This can be set by
> mtd devices (m25p80) to indicate to spi-master (ti-qspi) to perform
> memory mapped read. This helps to distinguish whether the spi-message is
> from mtd layer(hence mmap read is possible) or by other spi devices.

Based on this description and...

> + * @use_mmap_mode: Indicate to spi master to perform memory mapped
> + *	read if possible.

...the internal documentation I unable to tell what is meant by "perform
a memory mapped read", at least to the extent where it is visible
outside of the driver.  This means we can't really use this as a generic
API since other people won't be able to tell what it does.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Vignesh R]

Hi,

On 7/31/2015 11:47 PM, Mark Brown wrote:
> On Tue, Jul 28, 2015 at 02:11:12PM +0530, Vignesh R wrote:
> 
>> Introduce use_mmap_read field in spi_message struct. This can be set by
>> mtd devices (m25p80) to indicate to spi-master (ti-qspi) to perform
>> memory mapped read. This helps to distinguish whether the spi-message is
>> from mtd layer(hence mmap read is possible) or by other spi devices.
> 
> Based on this description and...
> 
>> + * @use_mmap_mode: Indicate to spi master to perform memory mapped
>> + *	read if possible.
> 
> ...the internal documentation I unable to tell what is meant by "perform
> a memory mapped read", at least to the extent where it is visible
> outside of the driver.  This means we can't really use this as a generic
> API since other people won't be able to tell what it does.
> 


Will the following documentation provide better idea regarding the flag:

@use_mmap_mode: Some SPI controller chips are optimized for interacting
with serial flash memories. These chips have memory mapped interface,
through which entire serial flash memory slave can be read/written as if
though they are physical memories (like RAM). Using this interface,
flash can be accessed using memcpy() function and the spi controller
hardware will take care of communicating with serial flash over SPI.
Setting this flag will indicate the SPI controller driver that the
spi_message is from mtd layer to read from/write to flash. The SPI
master driver can then appropriately switch the controller to memory
mapped interface to read from/write to flash, based on this flag (See
drivers/spi/spi-ti-qspi.c for example).
NOTE: If the SPI controller chip lacks memory mapped interface, then the
driver will ignore this flag and use normal SPI protocol to read
from/write to flash. Communication with non-flash SPI devices is not
possible using the memory mapped interface.

I can update the patch commit message and documentation accordingly?

Regards
Vignesh

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1438 bytes --]

On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:

> @use_mmap_mode: Some SPI controller chips are optimized for interacting
> with serial flash memories. These chips have memory mapped interface,
> through which entire serial flash memory slave can be read/written as if
> though they are physical memories (like RAM). Using this interface,
> flash can be accessed using memcpy() function and the spi controller
> hardware will take care of communicating with serial flash over SPI.
> Setting this flag will indicate the SPI controller driver that the
> spi_message is from mtd layer to read from/write to flash. The SPI
> master driver can then appropriately switch the controller to memory
> mapped interface to read from/write to flash, based on this flag (See
> drivers/spi/spi-ti-qspi.c for example).
> NOTE: If the SPI controller chip lacks memory mapped interface, then the
> driver will ignore this flag and use normal SPI protocol to read
> from/write to flash. Communication with non-flash SPI devices is not
> possible using the memory mapped interface.

I still can't tell from the above what this interface is supposed to do.
It sounds like the use of memory mapped mode is supposed to be
transparent to users, it should just affect how the controller interacts
with the hardware, but if that's the case why do we need to expose it to
users at all?  Shouldn't the driver just use memory mapped mode if it's
faster?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[R, Vignesh]

On 8/4/2015 9:21 PM, Mark Brown wrote:
> On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:
> 
>> @use_mmap_mode: Some SPI controller chips are optimized for interacting
>> with serial flash memories. These chips have memory mapped interface,
>> through which entire serial flash memory slave can be read/written as if
>> though they are physical memories (like RAM). Using this interface,
>> flash can be accessed using memcpy() function and the spi controller
>> hardware will take care of communicating with serial flash over SPI.
>> Setting this flag will indicate the SPI controller driver that the
>> spi_message is from mtd layer to read from/write to flash. The SPI
>> master driver can then appropriately switch the controller to memory
>> mapped interface to read from/write to flash, based on this flag (See
>> drivers/spi/spi-ti-qspi.c for example).
>> NOTE: If the SPI controller chip lacks memory mapped interface, then the
>> driver will ignore this flag and use normal SPI protocol to read
>> from/write to flash. Communication with non-flash SPI devices is not
>> possible using the memory mapped interface.
> 
> I still can't tell from the above what this interface is supposed to do.
> It sounds like the use of memory mapped mode is supposed to be
> transparent to users, it should just affect how the controller interacts
> with the hardware, but if that's the case why do we need to expose it to
> users at all?  Shouldn't the driver just use memory mapped mode if it's
> faster?
> 

TI QSPI controller has two blocks:
1. SPI_CORE: This is generic(normal) spi mode. This can be used to
communicate with any SPI devices (serial flashes as well as non-flash
devices like touchscreen).
2. SFI_MM_IF(SPI memory mapped interface): The SFI_MM_IF block only
allows reading and writing to an SPI flash device only. Used to speed up
flash reads. It _cannot_ be used to communicate with non flash devices.
Now, the spi_message that ti-qspi receives in transfer_one() callback
can be from mtd device(in which case SFI_MM_IF can be used) or from any
other non flash SPI device (in which case SFI_MM_IF must not be used
instead SPI_CORE is to be used) but there is no way(is there?) to
distinguish where spi_message is from. Therefore I introduced flag
(use_mmap_mode) to struct spi_message. mtd driver will set flag to true,
this helps the ti-qspi driver to determine that the user is flash device
and thus can do read via SFI_MM_IF. If this flag is not set then the
user is assumed to be non flash SPI driver and will use SPI_CORE block
to communicate.

On the whole, I just need a way to determine that the user is a flash
device in order to switch to memory mapped interface.

Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

Hello,

On 4 August 2015 at 19:59, R, Vignesh <vigneshr-l0cyMroinI0@public.gmane.org> wrote:
>
>
> On 8/4/2015 9:21 PM, Mark Brown wrote:
>> On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:
>>
>>> @use_mmap_mode: Some SPI controller chips are optimized for interacting
>>> with serial flash memories. These chips have memory mapped interface,
>>> through which entire serial flash memory slave can be read/written as if
>>> though they are physical memories (like RAM). Using this interface,
>>> flash can be accessed using memcpy() function and the spi controller
>>> hardware will take care of communicating with serial flash over SPI.
>>> Setting this flag will indicate the SPI controller driver that the
>>> spi_message is from mtd layer to read from/write to flash. The SPI
>>> master driver can then appropriately switch the controller to memory
>>> mapped interface to read from/write to flash, based on this flag (See
>>> drivers/spi/spi-ti-qspi.c for example).
>>> NOTE: If the SPI controller chip lacks memory mapped interface, then the
>>> driver will ignore this flag and use normal SPI protocol to read
>>> from/write to flash. Communication with non-flash SPI devices is not
>>> possible using the memory mapped interface.
>>
>> I still can't tell from the above what this interface is supposed to do.
>> It sounds like the use of memory mapped mode is supposed to be
>> transparent to users, it should just affect how the controller interacts
>> with the hardware, but if that's the case why do we need to expose it to
>> users at all?  Shouldn't the driver just use memory mapped mode if it's
>> faster?
>>
>
> TI QSPI controller has two blocks:
> 1. SPI_CORE: This is generic(normal) spi mode. This can be used to
> communicate with any SPI devices (serial flashes as well as non-flash
> devices like touchscreen).
> 2. SFI_MM_IF(SPI memory mapped interface): The SFI_MM_IF block only
> allows reading and writing to an SPI flash device only. Used to speed up
> flash reads. It _cannot_ be used to communicate with non flash devices.
> Now, the spi_message that ti-qspi receives in transfer_one() callback
> can be from mtd device(in which case SFI_MM_IF can be used) or from any
> other non flash SPI device (in which case SFI_MM_IF must not be used
> instead SPI_CORE is to be used) but there is no way(is there?) to
> distinguish where spi_message is from. Therefore I introduced flag
> (use_mmap_mode) to struct spi_message. mtd driver will set flag to true,
> this helps the ti-qspi driver to determine that the user is flash device
> and thus can do read via SFI_MM_IF. If this flag is not set then the
> user is assumed to be non flash SPI driver and will use SPI_CORE block
> to communicate.
>
> On the whole, I just need a way to determine that the user is a flash
> device in order to switch to memory mapped interface.
>

Maybe it can be set on the SPI slave rather than each message.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Vignesh R]

On 08/05/2015 10:51 AM, Michal Suchanek wrote:
> Hello,
> 
> On 4 August 2015 at 19:59, R, Vignesh <vigneshr@ti.com> wrote:
>>
>>
>> On 8/4/2015 9:21 PM, Mark Brown wrote:
>>> On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:
>>>
>>>> @use_mmap_mode: Some SPI controller chips are optimized for interacting
>>>> with serial flash memories. These chips have memory mapped interface,
>>>> through which entire serial flash memory slave can be read/written as if
>>>> though they are physical memories (like RAM). Using this interface,
>>>> flash can be accessed using memcpy() function and the spi controller
>>>> hardware will take care of communicating with serial flash over SPI.
>>>> Setting this flag will indicate the SPI controller driver that the
>>>> spi_message is from mtd layer to read from/write to flash. The SPI
>>>> master driver can then appropriately switch the controller to memory
>>>> mapped interface to read from/write to flash, based on this flag (See
>>>> drivers/spi/spi-ti-qspi.c for example).
>>>> NOTE: If the SPI controller chip lacks memory mapped interface, then the
>>>> driver will ignore this flag and use normal SPI protocol to read
>>>> from/write to flash. Communication with non-flash SPI devices is not
>>>> possible using the memory mapped interface.
>>>
>>> I still can't tell from the above what this interface is supposed to do.
>>> It sounds like the use of memory mapped mode is supposed to be
>>> transparent to users, it should just affect how the controller interacts
>>> with the hardware, but if that's the case why do we need to expose it to
>>> users at all?  Shouldn't the driver just use memory mapped mode if it's
>>> faster?
>>>
>>
>> TI QSPI controller has two blocks:
>> 1. SPI_CORE: This is generic(normal) spi mode. This can be used to
>> communicate with any SPI devices (serial flashes as well as non-flash
>> devices like touchscreen).
>> 2. SFI_MM_IF(SPI memory mapped interface): The SFI_MM_IF block only
>> allows reading and writing to an SPI flash device only. Used to speed up
>> flash reads. It _cannot_ be used to communicate with non flash devices.
>> Now, the spi_message that ti-qspi receives in transfer_one() callback
>> can be from mtd device(in which case SFI_MM_IF can be used) or from any
>> other non flash SPI device (in which case SFI_MM_IF must not be used
>> instead SPI_CORE is to be used) but there is no way(is there?) to
>> distinguish where spi_message is from. Therefore I introduced flag
>> (use_mmap_mode) to struct spi_message. mtd driver will set flag to true,
>> this helps the ti-qspi driver to determine that the user is flash device
>> and thus can do read via SFI_MM_IF. If this flag is not set then the
>> user is assumed to be non flash SPI driver and will use SPI_CORE block
>> to communicate.
>>
>> On the whole, I just need a way to determine that the user is a flash
>> device in order to switch to memory mapped interface.
>>
> 
> Maybe it can be set on the SPI slave rather than each message.

You mean to add flag to spi_device struct? That's ok for me.

-- 
Regards
Vignesh

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 5 August 2015 at 07:35, Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org> wrote:
>
>
> On 08/05/2015 10:51 AM, Michal Suchanek wrote:
>> Hello,
>>
>> On 4 August 2015 at 19:59, R, Vignesh <vigneshr-l0cyMroinI0@public.gmane.org> wrote:
>>>
>>>
>>> On 8/4/2015 9:21 PM, Mark Brown wrote:
>>>> On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:
>>>>

>>>
>>> TI QSPI controller has two blocks:
>>> 1. SPI_CORE: This is generic(normal) spi mode. This can be used to
>>> communicate with any SPI devices (serial flashes as well as non-flash
>>> devices like touchscreen).
>>> 2. SFI_MM_IF(SPI memory mapped interface): The SFI_MM_IF block only
>>> allows reading and writing to an SPI flash device only. Used to speed up
>>> flash reads. It _cannot_ be used to communicate with non flash devices.
>>> Now, the spi_message that ti-qspi receives in transfer_one() callback
>>> can be from mtd device(in which case SFI_MM_IF can be used) or from any
>>> other non flash SPI device (in which case SFI_MM_IF must not be used
>>> instead SPI_CORE is to be used) but there is no way(is there?) to
>>> distinguish where spi_message is from. Therefore I introduced flag
>>> (use_mmap_mode) to struct spi_message. mtd driver will set flag to true,
>>> this helps the ti-qspi driver to determine that the user is flash device
>>> and thus can do read via SFI_MM_IF. If this flag is not set then the
>>> user is assumed to be non flash SPI driver and will use SPI_CORE block
>>> to communicate.
>>>
>>> On the whole, I just need a way to determine that the user is a flash
>>> device in order to switch to memory mapped interface.
>>>
>>
>> Maybe it can be set on the SPI slave rather than each message.
>
> You mean to add flag to spi_device struct? That's ok for me.
>

There are already mode flags so you can just add one more.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2402 bytes --]

On Tue, Aug 04, 2015 at 11:29:52PM +0530, R, Vignesh wrote:
> On 8/4/2015 9:21 PM, Mark Brown wrote:
> > On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:

> > I still can't tell from the above what this interface is supposed to do.
> > It sounds like the use of memory mapped mode is supposed to be
> > transparent to users, it should just affect how the controller interacts
> > with the hardware, but if that's the case why do we need to expose it to
> > users at all?  Shouldn't the driver just use memory mapped mode if it's
> > faster?

> 2. SFI_MM_IF(SPI memory mapped interface): The SFI_MM_IF block only
> allows reading and writing to an SPI flash device only. Used to speed up
> flash reads. It _cannot_ be used to communicate with non flash devices.
> Now, the spi_message that ti-qspi receives in transfer_one() callback
> can be from mtd device(in which case SFI_MM_IF can be used) or from any
> other non flash SPI device (in which case SFI_MM_IF must not be used
> instead SPI_CORE is to be used) but there is no way(is there?) to
> distinguish where spi_message is from. Therefore I introduced flag
> (use_mmap_mode) to struct spi_message. mtd driver will set flag to true,
> this helps the ti-qspi driver to determine that the user is flash device
> and thus can do read via SFI_MM_IF. If this flag is not set then the
> user is assumed to be non flash SPI driver and will use SPI_CORE block
> to communicate.

So if you're trying to do this you need to document it adequately so
that other people can understand what it is supposed to do and how to
use and implement it.  People can't really tell how the interface is
supposed to work based on what was in the patch and the above isn't
really helping.  For example, how does this change or restrict what the
contents of the spi_message are?

> On the whole, I just need a way to determine that the user is a flash
> device in order to switch to memory mapped interface.

As far as I can tell you want to set a per spi_message flag saying that
the message is a flash read command?  If that's what this is trying to
do then why do you need to set the flag at all?  If the message is in a
clearly defined format and it's more efficient to use this mmap mode
then surely the driver can just recognise that the format is approprate
and switch into mmap mode without being explicitly told - I'm not clear
what the flag adds here.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 5 August 2015 at 13:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Tue, Aug 04, 2015 at 11:29:52PM +0530, R, Vignesh wrote:
>> On 8/4/2015 9:21 PM, Mark Brown wrote:
>> > On Mon, Aug 03, 2015 at 10:27:19AM +0530, Vignesh R wrote:
>
>> > I still can't tell from the above what this interface is supposed to do.
>> > It sounds like the use of memory mapped mode is supposed to be
>> > transparent to users, it should just affect how the controller interacts
>> > with the hardware, but if that's the case why do we need to expose it to
>> > users at all?  Shouldn't the driver just use memory mapped mode if it's
>> > faster?
>
>> 2. SFI_MM_IF(SPI memory mapped interface): The SFI_MM_IF block only
>> allows reading and writing to an SPI flash device only. Used to speed up
>> flash reads. It _cannot_ be used to communicate with non flash devices.
>> Now, the spi_message that ti-qspi receives in transfer_one() callback
>> can be from mtd device(in which case SFI_MM_IF can be used) or from any
>> other non flash SPI device (in which case SFI_MM_IF must not be used
>> instead SPI_CORE is to be used) but there is no way(is there?) to
>> distinguish where spi_message is from. Therefore I introduced flag
>> (use_mmap_mode) to struct spi_message. mtd driver will set flag to true,
>> this helps the ti-qspi driver to determine that the user is flash device
>> and thus can do read via SFI_MM_IF. If this flag is not set then the
>> user is assumed to be non flash SPI driver and will use SPI_CORE block
>> to communicate.
>
> So if you're trying to do this you need to document it adequately so
> that other people can understand what it is supposed to do and how to
> use and implement it.  People can't really tell how the interface is
> supposed to work based on what was in the patch and the above isn't
> really helping.  For example, how does this change or restrict what the
> contents of the spi_message are?
>
>> On the whole, I just need a way to determine that the user is a flash
>> device in order to switch to memory mapped interface.
>
> As far as I can tell you want to set a per spi_message flag saying that
> the message is a flash read command?  If that's what this is trying to
> do then why do you need to set the flag at all?  If the message is in a
> clearly defined format and it's more efficient to use this mmap mode
> then surely the driver can just recognise that the format is approprate
> and switch into mmap mode without being explicitly told - I'm not clear
> what the flag adds here.

ehm, the read command is just one byte.

I don't think sending 03 or other random byte as the first byte of a
SPI transfer can be used as reliable detection that we are talking to
a SPI flash memory.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1079 bytes --]

On Wed, Aug 05, 2015 at 02:40:01PM +0200, Michal Suchanek wrote:
> On 5 August 2015 at 13:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > As far as I can tell you want to set a per spi_message flag saying that
> > the message is a flash read command?  If that's what this is trying to
> > do then why do you need to set the flag at all?  If the message is in a
> > clearly defined format and it's more efficient to use this mmap mode
> > then surely the driver can just recognise that the format is approprate
> > and switch into mmap mode without being explicitly told - I'm not clear
> > what the flag adds here.

> ehm, the read command is just one byte.

> I don't think sending 03 or other random byte as the first byte of a
> SPI transfer can be used as reliable detection that we are talking to
> a SPI flash memory.

Why care - if something is physically in the same format as a flash read
command how would a device be able to tell that it wasn't actually a
flash read command?  The signals sent on the bus are going to be
identical anyway.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 5 August 2015 at 14:44, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 05, 2015 at 02:40:01PM +0200, Michal Suchanek wrote:
>> On 5 August 2015 at 13:50, Mark Brown <broonie@kernel.org> wrote:
>
>> > As far as I can tell you want to set a per spi_message flag saying that
>> > the message is a flash read command?  If that's what this is trying to
>> > do then why do you need to set the flag at all?  If the message is in a
>> > clearly defined format and it's more efficient to use this mmap mode
>> > then surely the driver can just recognise that the format is approprate
>> > and switch into mmap mode without being explicitly told - I'm not clear
>> > what the flag adds here.
>
>> ehm, the read command is just one byte.
>
>> I don't think sending 03 or other random byte as the first byte of a
>> SPI transfer can be used as reliable detection that we are talking to
>> a SPI flash memory.
>
> Why care - if something is physically in the same format as a flash read
> command how would a device be able to tell that it wasn't actually a
> flash read command?  The signals sent on the bus are going to be
> identical anyway.

Not only must the command be the same but also the response must be tha same.

The flash chip responds by sending arbitrary amount of data. Given
that transfer_one gets only the part that sends the read command and
the part to do the actual read may or may not follow this is getting a
bit hairy. Add in dummy bytes due to fast-read lag and page write
wrap-around and you get something that you definitely do not want
unless you are really sure that there is a flash memory on the other
end of the wire.

Thanks

Michal

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

On Wed, Aug 05, 2015 at 02:56:09PM +0200, Michal Suchanek wrote:
> On 5 August 2015 at 14:44, Mark Brown <broonie@kernel.org> wrote:
> > On Wed, Aug 05, 2015 at 02:40:01PM +0200, Michal Suchanek wrote:

> >> I don't think sending 03 or other random byte as the first byte of a
> >> SPI transfer can be used as reliable detection that we are talking to
> >> a SPI flash memory.

> > Why care - if something is physically in the same format as a flash read
> > command how would a device be able to tell that it wasn't actually a
> > flash read command?  The signals sent on the bus are going to be
> > identical anyway.

> Not only must the command be the same but also the response must be tha same.

What difference would that make?  The caller is sending a single SPI
operation and this is a user visible thing...

> The flash chip responds by sending arbitrary amount of data. Given
> that transfer_one gets only the part that sends the read command and
> the part to do the actual read may or may not follow this is getting a
> bit hairy. Add in dummy bytes due to fast-read lag and page write
> wrap-around and you get something that you definitely do not want
> unless you are really sure that there is a flash memory on the other
> end of the wire.

So if you're doing this you may have a good reason to implement
transfer_one_message() instead.  Or perhaps implement it in the core and
provide operations to do the map and unmap.  And of course if this sort
of requirement exists that's an obvious thing that must be documented
in the interfaces but isn't.

We need a lot more thought about the interface here, the lack of any
explanation of what the interface is supposed to be and the fact that
all questions about it are being answered in terms of describing the
specific system are both a bit worrying.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 6 August 2015 at 11:02, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Aug 05, 2015 at 02:56:09PM +0200, Michal Suchanek wrote:
>> On 5 August 2015 at 14:44, Mark Brown <broonie@kernel.org> wrote:
>> > On Wed, Aug 05, 2015 at 02:40:01PM +0200, Michal Suchanek wrote:
>
>> >> I don't think sending 03 or other random byte as the first byte of a
>> >> SPI transfer can be used as reliable detection that we are talking to
>> >> a SPI flash memory.
>
>> > Why care - if something is physically in the same format as a flash read
>> > command how would a device be able to tell that it wasn't actually a
>> > flash read command?  The signals sent on the bus are going to be
>> > identical anyway.
>
>> Not only must the command be the same but also the response must be tha same.
>
> What difference would that make?  The caller is sending a single SPI
> operation and this is a user visible thing...
>
>> The flash chip responds by sending arbitrary amount of data. Given
>> that transfer_one gets only the part that sends the read command and
>> the part to do the actual read may or may not follow this is getting a
>> bit hairy. Add in dummy bytes due to fast-read lag and page write
>> wrap-around and you get something that you definitely do not want
>> unless you are really sure that there is a flash memory on the other
>> end of the wire.
>
> So if you're doing this you may have a good reason to implement
> transfer_one_message() instead.  Or perhaps implement it in the core and
> provide operations to do the map and unmap.  And of course if this sort
> of requirement exists that's an obvious thing that must be documented
> in the interfaces but isn't.
>
> We need a lot more thought about the interface here, the lack of any
> explanation of what the interface is supposed to be and the fact that
> all questions about it are being answered in terms of describing the
> specific system are both a bit worrying.

Disclaimer: I am not familiar with the hardware for which this patch
adds support.

However, I am familiar m25p80.c and as I understand it the controller
is basically supposed to implement m25p80.c in hardware when this flag
is set.

If I was using m25p80.c to talk to anything but an actual flash chip
it would get me quite worried.

Thanks

Michal

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Russell King - ARM Linux]

On Thu, Aug 06, 2015 at 12:01:37PM +0200, Michal Suchanek wrote:
> Disclaimer: I am not familiar with the hardware for which this patch
> adds support.
> 
> However, I am familiar m25p80.c and as I understand it the controller
> is basically supposed to implement m25p80.c in hardware when this flag
> is set.

That, to me, sounds like what you have is:

---m25p80 specific interface--->SPI bus--->m25p80 device

Where the m25p80 specific interface does not expose direct access to the
SPI bus?

If that's the case, then maybe you should consider whether using the SPI
bus infrastructure is really the best way forward.  Would it make more
sense instead to adopt a different software structure, something more
high-level like:

     +-------------------------------------------+
     |          m25p80 high-level driver         |
     +----------------------+--------------------+
     |   SPI m25p80 driver  |                    |
     +----------------------+                    |
     |      SPI layer       |  Special driver    |
     +----------------------+                    |
     |    SPI bus driver    |                    |
     +----------------------+--------------------+
     |     SPI hardware     |  Special hardware  |
     +----------------------+--------------------+

Rather than what you seem to be trying to do, which seems to be:

     +----------------------+
     |   SPI m25p80 driver  |
     +----------------------+
     |      SPI layer       |
     +----------------------+
     |  Translation driver  |
     +----------------------+
     |   Special hardware   |
     +----------------------+

where this requires M25P80 specific hacks to be introduced into the SPI
layer so that you can communicate additional information between the SPI
M25P80 driver and the translation driver.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1519 bytes --]

On Thu, Aug 06, 2015 at 11:22:25AM +0100, Russell King - ARM Linux wrote:

> If that's the case, then maybe you should consider whether using the SPI
> bus infrastructure is really the best way forward.  Would it make more
> sense instead to adopt a different software structure, something more
> high-level like:

>      +-------------------------------------------+
>      |          m25p80 high-level driver         |
>      +----------------------+--------------------+
>      |   SPI m25p80 driver  |                    |
>      +----------------------+                    |
>      |      SPI layer       |  Special driver    |
>      +----------------------+                    |
>      |    SPI bus driver    |                    |
>      +----------------------+--------------------+
>      |     SPI hardware     |  Special hardware  |
>      +----------------------+--------------------+

Yes, that's what's been talked about before - for some of these devices
they're sufficiently flash specialist that we just don't bother exposing
a SPI interface at all though AIUI they could be persuaded to do it.  It
isn't entirely clear that we want exactly that split, if the devices are
reasonable SPI controllers we will want to handle the case where they
have flash and non-flash devices on the same bus.  For that there is
going to be some generalisable work possible for managing switching
between memory mapped and SPI modes where those are mutually exclusive,
especially if the switch between them isn't free.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 6 August 2015 at 12:22, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> On Thu, Aug 06, 2015 at 12:01:37PM +0200, Michal Suchanek wrote:
>> Disclaimer: I am not familiar with the hardware for which this patch
>> adds support.
>>
>> However, I am familiar m25p80.c and as I understand it the controller
>> is basically supposed to implement m25p80.c in hardware when this flag
>> is set.
>
> That, to me, sounds like what you have is:
>
> ---m25p80 specific interface--->SPI bus--->m25p80 device
>
> Where the m25p80 specific interface does not expose direct access to the
> SPI bus?

The m25p80 specific hardware interface is presumably optional so you
can use it or not. The description is a bit vague, though.

In fsl-qspi the driver does not make it optional. I am not sure that
controller can be used for non-m25p80 slaves.


>
> If that's the case, then maybe you should consider whether using the SPI
> bus infrastructure is really the best way forward.  Would it make more
> sense instead to adopt a different software structure, something more
> high-level like:
>
>      +-------------------------------------------+
>      |          m25p80 high-level driver      =spi-nor   |
>      +----------------------+--------------------+
>      |   SPI m25p80 driver  |                    |
>      +----------------------+                    |
>      |      SPI layer       |  Special driver    =fsl-qspi|
>      +----------------------+                    |
>      |    SPI bus driver    |                    |
>      +----------------------+--------------------+
>      |     SPI hardware     |  Special hardware  |
>      +----------------------+--------------------+
>

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Vignesh R]

On 08/06/2015 03:52 PM, Russell King - ARM Linux wrote:
> On Thu, Aug 06, 2015 at 12:01:37PM +0200, Michal Suchanek wrote:
>> Disclaimer: I am not familiar with the hardware for which this patch
>> adds support.
>>
>> However, I am familiar m25p80.c and as I understand it the controller
>> is basically supposed to implement m25p80.c in hardware when this flag
>> is set.
> 
> That, to me, sounds like what you have is:
> 
> ---m25p80 specific interface--->SPI bus--->m25p80 device
> 
> Where the m25p80 specific interface does not expose direct access to the
> SPI bus?
> 

Let me give overview of ti-qspi controller:

There are two interfaces in the controller, one exposes direct access to
SPI bus and the other doesn't. It is possible to dynamically switch
between these ports by writing to QSPI_SPI_SWITCH_REG.

The two interface are [1]:
1) Generic SPI interface: (config port): with this interface, ti-qspi
controller can communicate with *any* spi device (flash and non-flash).
This interface is provides direct access to SPI bus.

2) SPI memory mapped interface (memory mapped port): This is m25p80
specific interface which can be use to read data from flash.
But if the flash has to be configured to some particular mode like QUAD
READ MODE, then the controller needs to be put in to config port to read
and modify serial flash's config register and then switch to memory
mapped port in order to read data stored on flash.

For example on DRA74 evm, if a 64 MB flash is connected as a slave,
entire flash memory is visible from 0x5C000000 to 0x5FFFFFFF L3_MAIN
address. In order to read using memory mapped port following will be the
sequence:

1.Write to flash config register via config port to switch to QUAD MODE
(or any mode that flash supports).
2. Populate QSPI_SPI_SETUP_REGx with flash read command, number of
address bytes to use and dummy bytes required.
3. Switch to memory mapped port by writing to QSPI_SPI_SWITCH_REG.
4. Now, its possible to perform read from 0x5C000000 to 0x5FFFFFFF using
memcpy. The qspi controller hardware will communicate over SPI bus and
get the data. This data is directly sent to RAM via SoC's interconnect.

Advantages of memory mapped port are: improved read performance,
MEM_TO_MEM DMA support can be added (ti-qspi hardware as such does not
provide DMA events).

Advantages of config port: can be used to communicate with *any* SPI
device, provides direct read/write access to SPI bus.

On the whole following are my requirements:
1. to be able to communicate with non -flash SPI devices via config port
( this functionality is supported by current driver, I dont want to
break it). Or pump any spi_message on to SPI bus directly.
2. take advantage of memory mapped port in order to increase read
throughput( and use dma in future) when the slave is a m25p80 type flash.
3. handle m25p80 as well as other slave on multiple chipselects.

I just need to know whether the user that requested the transfer is
m25p80 driver. If yes, ti-qspi driver can take advantage of memory
mapped interface, else just use config port to access SPI bus directly.

Writing separate driver based on spi-nor framework to interface with
m25p80 is not an option because, I would lose the ability to interface
with non-flash devices.

The spi_message that is received in transfer_one_message() is too
generic to imply the slave device that is on the other side of the wire.
IMO, the read command does not imply that the slave is m25p80 flash
(besides the read opcodes vary across vendors of m25p80 and across modes).

As Michal suggested, adding a flag to spi_device to distinguish whether
the slave is a m25p80 flash type will help spi master to handle
optimizations specific to m25p80s while being generic enough to handle
all other spi devices. Is that ok? Is there any other way to imply what
slave as at the other end?

[1] TRM: http://www.ti.com/lit/ug/spruhz6/spruhz6.pdf 24.5.4 QSPI
Functional Description
-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Russell King - ARM Linux]

On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:
> On the whole following are my requirements:
> 1. to be able to communicate with non -flash SPI devices via config port
> ( this functionality is supported by current driver, I dont want to
> break it). Or pump any spi_message on to SPI bus directly.
> 2. take advantage of memory mapped port in order to increase read
> throughput( and use dma in future) when the slave is a m25p80 type flash.
> 3. handle m25p80 as well as other slave on multiple chipselects.
> 
> I just need to know whether the user that requested the transfer is
> m25p80 driver. If yes, ti-qspi driver can take advantage of memory
> mapped interface, else just use config port to access SPI bus directly.

The problem with this approach is that it's an abomination.  It's adding
a SPI-user specific hack which is detected by a specific driver.  That's
really not sane - what happens when we have lots of these kinds of "I'm
an X SPI-user" with drivers detecting that?  It's not maintainable in the
long term.

Yes, your requirements _today_ seem simple and easy, but you're only
thinking about today, not tomorrow when you've moved on and someone else
has to maintain the mess left behind (or delete it from mainline because
they're sick of dealing with a hack.)

> The spi_message that is received in transfer_one_message() is too
> generic to imply the slave device that is on the other side of the wire.
> IMO, the read command does not imply that the slave is m25p80 flash
> (besides the read opcodes vary across vendors of m25p80 and across modes).

I can see both sides of the argument.

Mark is saying: if the SPI driver detects that the message to be transmitted
is a read command followed by the appropriate number of dummy bytes, and
then the data being read _and_ it's using quad-mode access, and the hardware
generates _exactly_ that in hardware using the memory mapped mode, there is
no reason _not_ to use the hardware to achieve that SPI transaction.  The
bus activity will be identical to what happens when the SPI controller is
used manually to achieve that bus sequence.

You're saying: but the documentation says you can't use it for anything
except m25p80.  If you look at 24.5.4.1.2, it tells you what the SFI
generates on the bus, which is:

1. CS active
2. Read command byte sent
3. 1-4 address bytes sent
4. 0-3 dummy bytes sent
5. data bytes read from bus
6. CS inactive

So, Mark's point is "if we can detect a transaction which fits _that_
bus activity, there's no reason not to use this acceleration for the
transaction."

What you're failing to counter with is: we don't have enough information
in the SPI driver to know how many dummy bytes there are between the
address bytes and the data read from the bus.

The M25P80 driver just appends additional bytes to the message to
achieve this:

        struct m25p *flash = nor->priv;
        unsigned int dummy = nor->read_dummy;

        /* convert the dummy cycles to the number of bytes */
        dummy /= 8;

        flash->command[0] = nor->read_opcode;
        m25p_addr2cmd(nor, from, flash->command);

        t[0].tx_buf = flash->command;
        t[0].len = m25p_cmdsz(nor) + dummy;
        spi_message_add_tail(&t[0], &m);

The reason that the number of dummy bytes can't be detected is because
it's all hidden in the first transaction as the total number of bytes to
be transmitted - and the dummy bytes are uninitialised, so you can't
make any assumptions what value they are.  There is no way for the SPI
driver to know whether these dummy bytes are dummy bytes or whether they
have an effect on the targetted device.

I think that comprehensively rules out Mark's idea with the SPI core as
it stands today: there is no way for the SPI driver to reliably detect
a message like a SFI read command by parsing the SPI transmitted message
prior to sending it as we can never be sure whether there are dummy bytes
to be transmitted.

What may make more sense from the SPI point of view is to communicate to
all SPI drivers how many dummy bytes are to be transferred.  I'm not fully
up on SPI, but maybe something like this:

	t[0].tx_buf = flash->command;
	t[0].len = m25p_cmdsz(nor);
	spi_message_add_tail(&t[0], &m);
	t[1].tx_buf = dummy_buffer;
	t[1].len = dummy;
	t[1].dummy = 1;
	spi_message_add_tail(&t[1], &m);

This way, we're describing the transfer to the SPI core, and explicitly
indicating that there are some dummy bytes.  The SPI driver can then
tell that these are dummy bytes, and if the SPI message consists of:

- transmit 2 to 5 bytes where the first byte is a recognised read command
- transmit 0 to 3 dummy bytes
- read some bytes

then it can make use of the SFI mode to accelerate the operation.

This would not be a hack to the SPI code: we're describing to the SPI
code what we want to achieve in terms of the activity on the bus, and
providing that level of description then allows the SPI driver to make
informed decisions on whether it can handle the transfer using some
non-standard feature.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Geert Uytterhoeven]

On Thu, Aug 6, 2015 at 3:51 PM, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:
>> On the whole following are my requirements:
>> 1. to be able to communicate with non -flash SPI devices via config port
>> ( this functionality is supported by current driver, I dont want to
>> break it). Or pump any spi_message on to SPI bus directly.
>> 2. take advantage of memory mapped port in order to increase read
>> throughput( and use dma in future) when the slave is a m25p80 type flash.
>> 3. handle m25p80 as well as other slave on multiple chipselects.
>>
>> I just need to know whether the user that requested the transfer is
>> m25p80 driver. If yes, ti-qspi driver can take advantage of memory
>> mapped interface, else just use config port to access SPI bus directly.
>
> The problem with this approach is that it's an abomination.  It's adding
> a SPI-user specific hack which is detected by a specific driver.  That's
> really not sane - what happens when we have lots of these kinds of "I'm
> an X SPI-user" with drivers detecting that?  It's not maintainable in the
> long term.
>
> Yes, your requirements _today_ seem simple and easy, but you're only
> thinking about today, not tomorrow when you've moved on and someone else
> has to maintain the mess left behind (or delete it from mainline because
> they're sick of dealing with a hack.)
>
>> The spi_message that is received in transfer_one_message() is too
>> generic to imply the slave device that is on the other side of the wire.
>> IMO, the read command does not imply that the slave is m25p80 flash
>> (besides the read opcodes vary across vendors of m25p80 and across modes).
>
> I can see both sides of the argument.
>
> Mark is saying: if the SPI driver detects that the message to be transmitted
> is a read command followed by the appropriate number of dummy bytes, and
> then the data being read _and_ it's using quad-mode access, and the hardware
> generates _exactly_ that in hardware using the memory mapped mode, there is
> no reason _not_ to use the hardware to achieve that SPI transaction.  The
> bus activity will be identical to what happens when the SPI controller is
> used manually to achieve that bus sequence.
>
> You're saying: but the documentation says you can't use it for anything
> except m25p80.  If you look at 24.5.4.1.2, it tells you what the SFI
> generates on the bus, which is:
>
> 1. CS active
> 2. Read command byte sent
> 3. 1-4 address bytes sent
> 4. 0-3 dummy bytes sent
> 5. data bytes read from bus
> 6. CS inactive
>
> So, Mark's point is "if we can detect a transaction which fits _that_
> bus activity, there's no reason not to use this acceleration for the
> transaction."
>
> What you're failing to counter with is: we don't have enough information
> in the SPI driver to know how many dummy bytes there are between the
> address bytes and the data read from the bus.

Irrespective of the dummy bytes.
What if the spi device is not a FLASH ROM, but some other device,
which receives a data packet that accidentally looks like an m25p80 READ
command?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 6 August 2015 at 18:14, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> On Thu, Aug 6, 2015 at 3:51 PM, Russell King - ARM Linux
> <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
>> On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:
>>> On the whole following are my requirements:
>>> 1. to be able to communicate with non -flash SPI devices via config port
>>> ( this functionality is supported by current driver, I dont want to
>>> break it). Or pump any spi_message on to SPI bus directly.
>>> 2. take advantage of memory mapped port in order to increase read
>>> throughput( and use dma in future) when the slave is a m25p80 type flash.
>>> 3. handle m25p80 as well as other slave on multiple chipselects.
>>>
>>> I just need to know whether the user that requested the transfer is
>>> m25p80 driver. If yes, ti-qspi driver can take advantage of memory
>>> mapped interface, else just use config port to access SPI bus directly.
>>
>> The problem with this approach is that it's an abomination.  It's adding
>> a SPI-user specific hack which is detected by a specific driver.  That's
>> really not sane - what happens when we have lots of these kinds of "I'm
>> an X SPI-user" with drivers detecting that?  It's not maintainable in the
>> long term.
>>
>> Yes, your requirements _today_ seem simple and easy, but you're only
>> thinking about today, not tomorrow when you've moved on and someone else
>> has to maintain the mess left behind (or delete it from mainline because
>> they're sick of dealing with a hack.)
>>
>>> The spi_message that is received in transfer_one_message() is too
>>> generic to imply the slave device that is on the other side of the wire.
>>> IMO, the read command does not imply that the slave is m25p80 flash
>>> (besides the read opcodes vary across vendors of m25p80 and across modes).
>>
>> I can see both sides of the argument.
>>
>> Mark is saying: if the SPI driver detects that the message to be transmitted
>> is a read command followed by the appropriate number of dummy bytes, and
>> then the data being read _and_ it's using quad-mode access, and the hardware
>> generates _exactly_ that in hardware using the memory mapped mode, there is
>> no reason _not_ to use the hardware to achieve that SPI transaction.  The
>> bus activity will be identical to what happens when the SPI controller is
>> used manually to achieve that bus sequence.
>>
>> You're saying: but the documentation says you can't use it for anything
>> except m25p80.  If you look at 24.5.4.1.2, it tells you what the SFI
>> generates on the bus, which is:
>>
>> 1. CS active
>> 2. Read command byte sent
>> 3. 1-4 address bytes sent
>> 4. 0-3 dummy bytes sent
>> 5. data bytes read from bus
>> 6. CS inactive
>>
>> So, Mark's point is "if we can detect a transaction which fits _that_
>> bus activity, there's no reason not to use this acceleration for the
>> transaction."
>>
>> What you're failing to counter with is: we don't have enough information
>> in the SPI driver to know how many dummy bytes there are between the
>> address bytes and the data read from the bus.
>
> Irrespective of the dummy bytes.
> What if the spi device is not a FLASH ROM, but some other device,
> which receives a data packet that accidentally looks like an m25p80 READ
> command?
>

Presumably the driver would interpret some random part of the message
as address and map the reply in your address space at that address
from the flash mmap base.

If you happen to overflow the flash memory mmap space the behaviour
will probably not be well defined.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Russell King - ARM Linux]

On Thu, Aug 06, 2015 at 06:14:00PM +0200, Geert Uytterhoeven wrote:
> On Thu, Aug 6, 2015 at 3:51 PM, Russell King - ARM Linux
> <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> > On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:
> >> On the whole following are my requirements:
> >> 1. to be able to communicate with non -flash SPI devices via config port
> >> ( this functionality is supported by current driver, I dont want to
> >> break it). Or pump any spi_message on to SPI bus directly.
> >> 2. take advantage of memory mapped port in order to increase read
> >> throughput( and use dma in future) when the slave is a m25p80 type flash.
> >> 3. handle m25p80 as well as other slave on multiple chipselects.
> >>
> >> I just need to know whether the user that requested the transfer is
> >> m25p80 driver. If yes, ti-qspi driver can take advantage of memory
> >> mapped interface, else just use config port to access SPI bus directly.
> >
> > The problem with this approach is that it's an abomination.  It's adding
> > a SPI-user specific hack which is detected by a specific driver.  That's
> > really not sane - what happens when we have lots of these kinds of "I'm
> > an X SPI-user" with drivers detecting that?  It's not maintainable in the
> > long term.
> >
> > Yes, your requirements _today_ seem simple and easy, but you're only
> > thinking about today, not tomorrow when you've moved on and someone else
> > has to maintain the mess left behind (or delete it from mainline because
> > they're sick of dealing with a hack.)
> >
> >> The spi_message that is received in transfer_one_message() is too
> >> generic to imply the slave device that is on the other side of the wire.
> >> IMO, the read command does not imply that the slave is m25p80 flash
> >> (besides the read opcodes vary across vendors of m25p80 and across modes).
> >
> > I can see both sides of the argument.
> >
> > Mark is saying: if the SPI driver detects that the message to be transmitted
> > is a read command followed by the appropriate number of dummy bytes, and
> > then the data being read _and_ it's using quad-mode access, and the hardware
> > generates _exactly_ that in hardware using the memory mapped mode, there is
> > no reason _not_ to use the hardware to achieve that SPI transaction.  The
> > bus activity will be identical to what happens when the SPI controller is
> > used manually to achieve that bus sequence.
> >
> > You're saying: but the documentation says you can't use it for anything
> > except m25p80.  If you look at 24.5.4.1.2, it tells you what the SFI
> > generates on the bus, which is:
> >
> > 1. CS active
> > 2. Read command byte sent
> > 3. 1-4 address bytes sent
> > 4. 0-3 dummy bytes sent
> > 5. data bytes read from bus
> > 6. CS inactive
> >
> > So, Mark's point is "if we can detect a transaction which fits _that_
> > bus activity, there's no reason not to use this acceleration for the
> > transaction."
> >
> > What you're failing to counter with is: we don't have enough information
> > in the SPI driver to know how many dummy bytes there are between the
> > address bytes and the data read from the bus.
> 
> Irrespective of the dummy bytes.
> What if the spi device is not a FLASH ROM, but some other device,
> which receives a data packet that accidentally looks like an m25p80 READ
> command?

Well, for the most part it looks like it should still work, but there
could be a gotcha, but first, let's get rid of a myth there.

The QSPI is _not_ specific to the M25P80.  The manual says nothing
about being specific to that device.  What it says is that it's for
SPI NOR memory.  It will work with bus widths of 1, 2 or 4 data lines,
so it probably works with non-M25P80 SPI NOR devices too - and the fact
that the read and write commands are completely programmable suggests
that using it with SPI NOR devices which do not use the M25P80 read
command value is intended.


The SFI is a state machine based translator which sits behind the SPI
interface (look at the manual).  It sequence sthe SPI bus through a
series of standard SPI states which happen to be the states I detailed
above.

Now, the first byte of the SFI-generated SPI message can be programmed
to any 8 bit value.  So the first byte of the SPI message is totally
under software control.  The next one to four bytes which comprise the
"address" can be controlled to by deciding where in the memory map to
start reading from.  Hence, the value of those bytes is also totally
under software control.  The number of dummy bytes can be programmed
too.  So far so good.

So, if we know that we have a SPI message which says "send 0x01 0x20
0x30, send one dummy byte, read 32 bytes", if we program the SFI to
send a read command as 0x01, program an address length of 2 bytes
with one dummy byte, and then read the next 32 bytes at the appropriate
offset in the memory mapping to cause the next two bytes to be 0x20,
0x30, then what we end up with on the bus is:

	send 0x01, 0x20, 0x30
	send one dummy byte

That much is good, but now is the problem - how does the SFI know that
we're going to require to read 32 bytes?  I think the answer to that
is that it doesn't know, so it probably just reads the number of bytes
which the access on the SoC bus is asking for, which makes it
indeterminant from a software point of view to control how many bytes
will be read without provoking another "send 0x01, next address, dummy
byte" sequence.

So, I'm now on the side of not parsing commands in the SPI driver, and
back on the idea that this needs to be handled in some other manner
which doesn't involve polluting the SPI core with flag-hacks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 6 August 2015 at 23:33, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> On Thu, Aug 06, 2015 at 06:14:00PM +0200, Geert Uytterhoeven wrote:
>> On Thu, Aug 6, 2015 at 3:51 PM, Russell King - ARM Linux
>> <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
>> > On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:
>> >> On the whole following are my requirements:
>> >> 1. to be able to communicate with non -flash SPI devices via config port
>> >> ( this functionality is supported by current driver, I dont want to
>> >> break it). Or pump any spi_message on to SPI bus directly.
>> >> 2. take advantage of memory mapped port in order to increase read
>> >> throughput( and use dma in future) when the slave is a m25p80 type flash.
>> >> 3. handle m25p80 as well as other slave on multiple chipselects.
>> >>
>> >> I just need to know whether the user that requested the transfer is
>> >> m25p80 driver. If yes, ti-qspi driver can take advantage of memory
>> >> mapped interface, else just use config port to access SPI bus directly.
>> >
>> > The problem with this approach is that it's an abomination.  It's adding
>> > a SPI-user specific hack which is detected by a specific driver.  That's
>> > really not sane - what happens when we have lots of these kinds of "I'm
>> > an X SPI-user" with drivers detecting that?  It's not maintainable in the
>> > long term.
>> >
>> > Yes, your requirements _today_ seem simple and easy, but you're only
>> > thinking about today, not tomorrow when you've moved on and someone else
>> > has to maintain the mess left behind (or delete it from mainline because
>> > they're sick of dealing with a hack.)
>> >
>> >> The spi_message that is received in transfer_one_message() is too
>> >> generic to imply the slave device that is on the other side of the wire.
>> >> IMO, the read command does not imply that the slave is m25p80 flash
>> >> (besides the read opcodes vary across vendors of m25p80 and across modes).
>> >
>> > I can see both sides of the argument.
>> >
>> > Mark is saying: if the SPI driver detects that the message to be transmitted
>> > is a read command followed by the appropriate number of dummy bytes, and
>> > then the data being read _and_ it's using quad-mode access, and the hardware
>> > generates _exactly_ that in hardware using the memory mapped mode, there is
>> > no reason _not_ to use the hardware to achieve that SPI transaction.  The
>> > bus activity will be identical to what happens when the SPI controller is
>> > used manually to achieve that bus sequence.
>> >
>> > You're saying: but the documentation says you can't use it for anything
>> > except m25p80.  If you look at 24.5.4.1.2, it tells you what the SFI
>> > generates on the bus, which is:
>> >
>> > 1. CS active
>> > 2. Read command byte sent
>> > 3. 1-4 address bytes sent
>> > 4. 0-3 dummy bytes sent
>> > 5. data bytes read from bus
>> > 6. CS inactive
>> >
>> > So, Mark's point is "if we can detect a transaction which fits _that_
>> > bus activity, there's no reason not to use this acceleration for the
>> > transaction."
>> >
>> > What you're failing to counter with is: we don't have enough information
>> > in the SPI driver to know how many dummy bytes there are between the
>> > address bytes and the data read from the bus.
>>
>> Irrespective of the dummy bytes.
>> What if the spi device is not a FLASH ROM, but some other device,
>> which receives a data packet that accidentally looks like an m25p80 READ
>> command?
>
> Well, for the most part it looks like it should still work, but there
> could be a gotcha, but first, let's get rid of a myth there.
>
> The QSPI is _not_ specific to the M25P80.  The manual says nothing
> about being specific to that device.  What it says is that it's for
> SPI NOR memory.  It will work with bus widths of 1, 2 or 4 data lines,
> so it probably works with non-M25P80 SPI NOR devices too - and the fact
> that the read and write commands are completely programmable suggests
> that using it with SPI NOR devices which do not use the M25P80 read
> command value is intended.
>
...
>
> That much is good, but now is the problem - how does the SFI know that
> we're going to require to read 32 bytes?  I think the answer to that
> is that it doesn't know, so it probably just reads the number of bytes
> which the access on the SoC bus is asking for, which makes it
> indeterminant from a software point of view to control how many bytes
> will be read without provoking another "send 0x01, next address, dummy
> byte" sequence.
>
> So, I'm now on the side of not parsing commands in the SPI driver, and
> back on the idea that this needs to be handled in some other manner
> which doesn't involve polluting the SPI core with flag-hacks.

OK, so we can agree that using this hardware acceleration for any kind
of transfer indiscriminately is not a very good idea.

Now since the description is clearer it's obvious that ti-qspi cannot
work fully mmapped as fsl-qspi does because the setup has to be done
over normal spi access and using non-m25p80 devices on the same bus is
a requirement.

The place where it is known if a transfer can use the mmap access is m25p80.c

So my suggestion is

 - add a new method for spi master that gets the read opcode, dummy
length, address, address length, buffer, buffer length and performs
read from the flash memory in a hardware-specific way

- add a check in m25p80.c that the master supports this feature and if
so use it (eg check that the method is non-null)

Presumably if some new SPI controllers with similar feature are
supported in the future they can use the same inteface because you
pass on everything the m25p80 read knows.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Vignesh R]

On 08/07/2015 01:08 PM, Michal Suchanek wrote:

> Now since the description is clearer it's obvious that ti-qspi cannot
> work fully mmapped as fsl-qspi does because the setup has to be done
> over normal spi access and using non-m25p80 devices on the same bus is
> a requirement.
> 
> The place where it is known if a transfer can use the mmap access is m25p80.c
> 
> So my suggestion is
> 
>  - add a new method for spi master that gets the read opcode, dummy
> length, address, address length, buffer, buffer length and performs
> read from the flash memory in a hardware-specific way
> 
> - add a check in m25p80.c that the master supports this feature and if
> so use it (eg check that the method is non-null)
> 
> Presumably if some new SPI controllers with similar feature are
> supported in the future they can use the same inteface because you
> pass on everything the m25p80 read knows.
> 

Ok... Do you mean something like this?

I will take m25p80 as example but can be expanded for any flash.

In include/linux/mtd.h:
	struct spi_mtd_config_info {
		struct spi_device 	*spi;
	        u32                     page_size;
	        u8                      addr_width;
       		u8                      erase_opcode;
	        u8                      read_opcode;
        	u8                      read_dummy;
	        u8                      program_opcode;
        	enum read_mode          flash_read;

	} /* subset of struct spi_nor */


In m25p80.c:

	static int m25p80_read(struct spi_nor *nor, loff_t from,
				size_t len, size_t *retlen,
				u_char *buf)
	{
		struct spi_mtd_config_info info;
		struct spi_device *spi;
		
		if (spi->master->spi_mtd_mmap_read) {
		  /* Populate spi_mtd_config_info */
		  spi->master->spi_mtd_mmap_read(&info, from, len,
						 retlen, buf);
		}
		else {
		/* no mtd specific acceleration supported try normal
		 * SPI way of communicating with flash
		 * continue with current code
		 * set up spi_message and call spi_sync()
		 */
	}

      }

In spi-ti-qspi.c:
Implement spi_mtd_mmap_read while holding master->bus_lock mutex.


-- 
Regards
Vignesh

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Martin Sperl]

On 8/6/2015 23:33, Russell King - ARM Linux wrote:
> On Thu, Aug 06, 2015 at 06:14:00PM +0200, Geert Uytterhoeven wrote:
>>
>> Irrespective of the dummy bytes.
>> What if the spi device is not a FLASH ROM, but some other device,
>> which receives a data packet that accidentally looks like an m25p80 READ
>> command?
> Well, for the most part it looks like it should still work, but there
> could be a gotcha, but first, let's get rid of a myth there.
>
> The QSPI is _not_ specific to the M25P80.  The manual says nothing
> about being specific to that device.  What it says is that it's for
> SPI NOR memory.  It will work with bus widths of 1, 2 or 4 data lines,
> so it probably works with non-M25P80 SPI NOR devices too - and the fact
> that the read and write commands are completely programmable suggests
> that using it with SPI NOR devices which do not use the M25P80 read
> command value is intended.
>
>
> The SFI is a state machine based translator which sits behind the SPI
> interface (look at the manual).  It sequence sthe SPI bus through a
> series of standard SPI states which happen to be the states I detailed
> above.
>
> Now, the first byte of the SFI-generated SPI message can be programmed
> to any 8 bit value.  So the first byte of the SPI message is totally
> under software control.  The next one to four bytes which comprise the
> "address" can be controlled to by deciding where in the memory map to
> start reading from.  Hence, the value of those bytes is also totally
> under software control.  The number of dummy bytes can be programmed
> too.  So far so good.
>
> So, if we know that we have a SPI message which says "send 0x01 0x20
> 0x30, send one dummy byte, read 32 bytes", if we program the SFI to
> send a read command as 0x01, program an address length of 2 bytes
> with one dummy byte, and then read the next 32 bytes at the appropriate
> offset in the memory mapping to cause the next two bytes to be 0x20,
> 0x30, then what we end up with on the bus is:
>
> 	send 0x01, 0x20, 0x30
> 	send one dummy byte
>
> That much is good, but now is the problem - how does the SFI know that
> we're going to require to read 32 bytes?  I think the answer to that
> is that it doesn't know, so it probably just reads the number of bytes
> which the access on the SoC bus is asking for, which makes it
> indeterminant from a software point of view to control how many bytes
> will be read without provoking another "send 0x01, next address, dummy
> byte" sequence.
>
> So, I'm now on the side of not parsing commands in the SPI driver, and
> back on the idea that this needs to be handled in some other manner
> which doesn't involve polluting the SPI core with flag-hacks.
>
So I see 2 distinct options:

Have the nor driver modified to run SPI commands and then ask the
SPI framework (and driver) to switch into mmap mode:

Would probably look something like this inside the nor driver:
    /* lock spi bus for other activities */
    spi_bus_lock(spi);
    /* send the "configuration" for mmap */
    t[0].tx_buf = flash->command;
    t[0].len = m25p_cmdsz(nor);
    spi_message_add_tail(&t[0], &m);
    t[1].tx_buf = dummy_buffer;
    t[1].len = dummy;
    spi_message_add_tail(&t[1], &m);
    spi_sync(spi, &m);
    /* switch to mmap mode */
    spi->mode |= SPI_MMAP;
    spi_setup(spi);
    /* run the mmapped transfers bypassing the spi-layer */
    memcpy(...)
    /* open questions here: which address range
     * and how to detect if transfer is done
*/
/* restore back to "normal" mode */
    spi->mode &= ~SPI_MMAP;
    spi_setup(spi);
    /* unlock spi bus for other activities */
    spi_bus_unlock(spi);

The downside is that it requires modification in several places
(nor-framework, spi-framework plus the driver) and it would not
be generic enough...

IMO such a situation is feasible if we only got a single device
on the spi-bus, but leaves a lot of questions open...
Alternatively we could create an additional api.

On the other end of spectrum could be a solution where the
spi-master driverwould have the opportunity to query the
device-tree for specific propertiesduring the spi_add_device
phase - in this case querying the followingproperty in the
device-tree:
   spi-master-XXX,use-mmap-cmd-mode = <0x08 0x38>;
to implement mmap-mode for commands 0x08 and 0x38.

Maybe we would want to also encode the number of address bytes
to send per command without hardcoding those values explicitly:
so maybe something like:
   spi-master-XXX,use-mmap-cmd-mode = <0x08 2> <0x38 3>;

Obviously these would need to get documented in the bindings
documentation of that driver.

Alternatively we could also introduce generic alternate modes
for the driver(similar to GPIO - ALT modes), but that would be
less transparent and more hard-coded...

In the end this would mean that from the nor framework side
therewould be no change at all - it still would be issuing
something like this:
    /* send the "normal" block read command */
    t[0].tx_buf = flash->command;
    /* note that the address would be encoded here */
    t[0].len = m25p_cmdsz(nor);
    spi_message_add_tail(&t[0], &m);
    /* dummy bytes could also get added to the above transfer */
    t[1].tx_buf = dummy_buffer;
    t[1].len = dummy;
    spi_message_add_tail(&t[1], &m);
    /* the real transfer */
    t[2].rx_buf = read_buffer;
    t[2].len = transfer_size;
    spi_message_add_tail(&t[2], &m);
    spi_sync(spi, &m);

On the spi-master side the driver would need to run:
*  if the spi-message (in this case the first byte) matches
    the "allowed" command pattern:
   * "setup" device in normal mode preparing the transfer engine
   * mode-switch to "mmapped" mode
   * copy via mmapped mode (via DMA to avoid blocking the CPU?)
   * return to "normal" mode
* else
   * run in "normal" spi mode.
(obviously it would be a bit more complicated that that).

Note that a discussion in regards to spi-master methods called
duringspi_add_device has just come up a few days ago in a
slightly differentcontext - forcing to always/never use DMA mode
for a specific spi-device on the bus (as well as tuning other
parameters per device).

Obviously the additional properties should describe required
HW behaviour and not tune the driver parameters - sys should be
used for those, but also for implementing those sys parameters we
would need those above "hooks"...

Just an idea.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 7 August 2015 at 10:25, Martin Sperl <martin@sperl.org> wrote:
> On 8/6/2015 23:33, Russell King - ARM Linux wrote:
>>
>> On Thu, Aug 06, 2015 at 06:14:00PM +0200, Geert Uytterhoeven wrote:
>>>
>>>
>>> Irrespective of the dummy bytes.
>>> What if the spi device is not a FLASH ROM, but some other device,
>>> which receives a data packet that accidentally looks like an m25p80 READ
>>> command?
>>
>> Well, for the most part it looks like it should still work, but there
>> could be a gotcha, but first, let's get rid of a myth there.
>>
>> The QSPI is _not_ specific to the M25P80.  The manual says nothing
>> about being specific to that device.  What it says is that it's for
>> SPI NOR memory.  It will work with bus widths of 1, 2 or 4 data lines,
>> so it probably works with non-M25P80 SPI NOR devices too - and the fact
>> that the read and write commands are completely programmable suggests
>> that using it with SPI NOR devices which do not use the M25P80 read
>> command value is intended.
>>
>>
>> The SFI is a state machine based translator which sits behind the SPI
>> interface (look at the manual).  It sequence sthe SPI bus through a
>> series of standard SPI states which happen to be the states I detailed
>> above.
>>
>> Now, the first byte of the SFI-generated SPI message can be programmed
>> to any 8 bit value.  So the first byte of the SPI message is totally
>> under software control.  The next one to four bytes which comprise the
>> "address" can be controlled to by deciding where in the memory map to
>> start reading from.  Hence, the value of those bytes is also totally
>> under software control.  The number of dummy bytes can be programmed
>> too.  So far so good.
>>
>> So, if we know that we have a SPI message which says "send 0x01 0x20
>> 0x30, send one dummy byte, read 32 bytes", if we program the SFI to
>> send a read command as 0x01, program an address length of 2 bytes
>> with one dummy byte, and then read the next 32 bytes at the appropriate
>> offset in the memory mapping to cause the next two bytes to be 0x20,
>> 0x30, then what we end up with on the bus is:
>>
>>         send 0x01, 0x20, 0x30
>>         send one dummy byte
>>
>> That much is good, but now is the problem - how does the SFI know that
>> we're going to require to read 32 bytes?  I think the answer to that
>> is that it doesn't know, so it probably just reads the number of bytes
>> which the access on the SoC bus is asking for, which makes it
>> indeterminant from a software point of view to control how many bytes
>> will be read without provoking another "send 0x01, next address, dummy
>> byte" sequence.
>>
>> So, I'm now on the side of not parsing commands in the SPI driver, and
>> back on the idea that this needs to be handled in some other manner
>> which doesn't involve polluting the SPI core with flag-hacks.
>>
> So I see 2 distinct options:
>
> Have the nor driver modified to run SPI commands and then ask the
> SPI framework (and driver) to switch into mmap mode:
>
> Would probably look something like this inside the nor driver:
>    /* lock spi bus for other activities */
>    spi_bus_lock(spi);
>    /* send the "configuration" for mmap */
>    t[0].tx_buf = flash->command;
>    t[0].len = m25p_cmdsz(nor);
>    spi_message_add_tail(&t[0], &m);
>    t[1].tx_buf = dummy_buffer;
>    t[1].len = dummy;
>    spi_message_add_tail(&t[1], &m);
>    spi_sync(spi, &m);

>    /* switch to mmap mode */
>    spi->mode |= SPI_MMAP;

Presumably you will do this *before* sending the read command so the
SPI driver can reverse-engineer it according to the spi-nor read
pattern.

Then the mmap mode can be set in spi_sync() but you also have to lock
the spi controller to prevent other transfers from resetting it.

Or you can pass the read buffer to the SPI master driver as well to do
the memcpy for you. Which you want to do anyway in case the SPI master
can use DMA to fill the buffer rather than memcpy.

>    spi_setup(spi);
>    /* run the mmapped transfers bypassing the spi-layer */
>    memcpy(...)
>    /* open questions here: which address range
>     * and how to detect if transfer is done
> */

Presumably when the memcpy ends the transfer is done.

> /* restore back to "normal" mode */
>    spi->mode &= ~SPI_MMAP;
>    spi_setup(spi);
>    /* unlock spi bus for other activities */
>    spi_bus_unlock(spi);

Presumably you have to restore to non-mmap mode only when you receive
a command that requires it. If the next command is read with same
configuration you can just keep reading.

>
> The downside is that it requires modification in several places
> (nor-framework, spi-framework plus the driver) and it would not
> be generic enough...

You only need modification to the SPI master driver and m25p80 driver.

The SPI master driver is where the hardware-specific operation is
executed and m25p80 is where you have the information so you can
decide to take advantage of the hardware acceleration.

>
> IMO such a situation is feasible if we only got a single device
> on the spi-bus, but leaves a lot of questions open...
> Alternatively we could create an additional api.

The spi master driver can track what mode is set on the controller and
change it as needed before each transfer. This is what is commonly
done for other SPI master hardware configuration options.

>
> On the other end of spectrum could be a solution where the
> spi-master driverwould have the opportunity to query the
> device-tree for specific propertiesduring the spi_add_device
> phase - in this case querying the followingproperty in the
> device-tree:
>   spi-master-XXX,use-mmap-cmd-mode = <0x08 0x38>;
> to implement mmap-mode for commands 0x08 and 0x38.

This is bogus.

Firstly this is per-slave and not per-master.

Secondly in devicetree you have jedec,spi-nor which says exactly that
the SPI slave is a device that responds to the JEDEC identify command.
Nothing more. The data returned from the identify command is used to
determine what the command opcodes are, what features are supported,
etc. So that is *not* in the dt and there is no place for hardwiring
opcodes in the dt.

> Alternatively we could also introduce generic alternate modes
> for the driver(similar to GPIO - ALT modes), but that would be
> less transparent and more hard-coded...
>
> In the end this would mean that from the nor framework side
> therewould be no change at all - it still would be issuing
> something like this:
>    /* send the "normal" block read command */
>    t[0].tx_buf = flash->command;
>    /* note that the address would be encoded here */
>    t[0].len = m25p_cmdsz(nor);
>    spi_message_add_tail(&t[0], &m);
>    /* dummy bytes could also get added to the above transfer */
>    t[1].tx_buf = dummy_buffer;
>    t[1].len = dummy;
>    spi_message_add_tail(&t[1], &m);
>    /* the real transfer */
>    t[2].rx_buf = read_buffer;
>    t[2].len = transfer_size;
>    spi_message_add_tail(&t[2], &m);
>    spi_sync(spi, &m);
>
> On the spi-master side the driver would need to run:
> *  if the spi-message (in this case the first byte) matches
>    the "allowed" command pattern:

There is no 'allowed' pattern. Any message like 1~7 bytes long can be
interpreted as read command. Unless you can tell the SPI master driver
it cannot know.

And you go the abominable route of the original patch that you compose
a SPI message in m25p80, and then decide in the SPI master driver that
you will in fact not send a SPI message and reverse-engineer what
m25p80 really meant.

On 7 August 2015 at 10:35, Vignesh R <vigneshr@ti.com> wrote:
>
>
> On 08/07/2015 01:08 PM, Michal Suchanek wrote:
>
>> Now since the description is clearer it's obvious that ti-qspi cannot
>> work fully mmapped as fsl-qspi does because the setup has to be done
>> over normal spi access and using non-m25p80 devices on the same bus is
>> a requirement.
>>
>> The place where it is known if a transfer can use the mmap access is m25p80.c
>>
>> So my suggestion is
>>
>>  - add a new method for spi master that gets the read opcode, dummy
>> length, address, address length, buffer, buffer length and performs
>> read from the flash memory in a hardware-specific way
>>
>> - add a check in m25p80.c that the master supports this feature and if
>> so use it (eg check that the method is non-null)
>>
>> Presumably if some new SPI controllers with similar feature are
>> supported in the future they can use the same inteface because you
>> pass on everything the m25p80 read knows.
>>
>
> Ok... Do you mean something like this?
>
> I will take m25p80 as example but can be expanded for any flash.
>
> In include/linux/mtd.h:
>         struct spi_mtd_config_info {
>                 struct spi_device       *spi;
>                 u32                     page_size;
>                 u8                      addr_width;
>                 u8                      erase_opcode;
>                 u8                      read_opcode;
>                 u8                      read_dummy;
>                 u8                      program_opcode;
>                 enum read_mode          flash_read;
>
>         } /* subset of struct spi_nor */
>

I would just pass these as separate arguments to the function but whatver.

> In m25p80.c:
>
>         static int m25p80_read(struct spi_nor *nor, loff_t from,
>                                 size_t len, size_t *retlen,
>                                 u_char *buf)
>         {
>                 struct spi_mtd_config_info info;
>                 struct spi_device *spi;
>
>                 if (spi->master->spi_mtd_mmap_read) {
>                   /* Populate spi_mtd_config_info */
>                   spi->master->spi_mtd_mmap_read(&info, from, len,
>                                                  retlen, buf);
>                 }
>                 else {
>                 /* no mtd specific acceleration supported try normal
>                  * SPI way of communicating with flash
>                  * continue with current code
>                  * set up spi_message and call spi_sync()
>                  */
>         }
>
>       }
>
> In spi-ti-qspi.c:
> Implement spi_mtd_mmap_read while holding master->bus_lock mutex.
>

Thanks

Michal

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Vignesh R]

On 08/07/2015 03:46 PM, Michal Suchanek wrote:
[snip]
> On 7 August 2015 at 10:35, Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org> wrote:
>>
>>
>> On 08/07/2015 01:08 PM, Michal Suchanek wrote:
>>
>>> Now since the description is clearer it's obvious that ti-qspi cannot
>>> work fully mmapped as fsl-qspi does because the setup has to be done
>>> over normal spi access and using non-m25p80 devices on the same bus is
>>> a requirement.
>>>
>>> The place where it is known if a transfer can use the mmap access is m25p80.c
>>>
>>> So my suggestion is
>>>
>>>  - add a new method for spi master that gets the read opcode, dummy
>>> length, address, address length, buffer, buffer length and performs
>>> read from the flash memory in a hardware-specific way
>>>
>>> - add a check in m25p80.c that the master supports this feature and if
>>> so use it (eg check that the method is non-null)
>>>
>>> Presumably if some new SPI controllers with similar feature are
>>> supported in the future they can use the same inteface because you
>>> pass on everything the m25p80 read knows.
>>>
>>
>> Ok... Do you mean something like this?
>>
>> I will take m25p80 as example but can be expanded for any flash.
>>
>> In include/linux/mtd.h:
>>         struct spi_mtd_config_info {
>>                 struct spi_device       *spi;
>>                 u32                     page_size;
>>                 u8                      addr_width;
>>                 u8                      erase_opcode;
>>                 u8                      read_opcode;
>>                 u8                      read_dummy;
>>                 u8                      program_opcode;
>>                 enum read_mode          flash_read;
>>
>>         } /* subset of struct spi_nor */
>>
> 
> I would just pass these as separate arguments to the function but whatver.
> 
>> In m25p80.c:
>>
>>         static int m25p80_read(struct spi_nor *nor, loff_t from,
>>                                 size_t len, size_t *retlen,
>>                                 u_char *buf)
>>         {
>>                 struct spi_mtd_config_info info;
>>                 struct spi_device *spi;
>>
>>                 if (spi->master->spi_mtd_mmap_read) {
>>                   /* Populate spi_mtd_config_info */
>>                   spi->master->spi_mtd_mmap_read(&info, from, len,
>>                                                  retlen, buf);
>>                 }
>>                 else {
>>                 /* no mtd specific acceleration supported try normal
>>                  * SPI way of communicating with flash
>>                  * continue with current code
>>                  * set up spi_message and call spi_sync()
>>                  */
>>         }
>>
>>       }
>>
>> In spi-ti-qspi.c:
>> Implement spi_mtd_mmap_read while holding master->bus_lock mutex.
>>
> 

I will re-submit patches based on the above idea, if there are no
further comments..

-- 
Thanks
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]

On Thu, Aug 06, 2015 at 02:51:29PM +0100, Russell King - ARM Linux wrote:

> The M25P80 driver just appends additional bytes to the message to
> achieve this:
> 
>         struct m25p *flash = nor->priv;
>         unsigned int dummy = nor->read_dummy;
> 
>         /* convert the dummy cycles to the number of bytes */
>         dummy /= 8;
> 
>         flash->command[0] = nor->read_opcode;
>         m25p_addr2cmd(nor, from, flash->command);
> 
>         t[0].tx_buf = flash->command;
>         t[0].len = m25p_cmdsz(nor) + dummy;
>         spi_message_add_tail(&t[0], &m);

> The reason that the number of dummy bytes can't be detected is because
> it's all hidden in the first transaction as the total number of bytes to
> be transmitted - and the dummy bytes are uninitialised, so you can't
> make any assumptions what value they are.  There is no way for the SPI
> driver to know whether these dummy bytes are dummy bytes or whether they
> have an effect on the targetted device.

We *could* (as you suggest below) indicate dummy transfers by having a
separate transfer which omits the transmit buffers though I'd expect
that normally that is going to be a small performance hit if interpreted
directly so we need to think what to do there.  We do get other devices
sending dummy bytes, it's sometimes a requirement for high speed
register access to give settling time for the device, so other things
would get some milage from it.

> What may make more sense from the SPI point of view is to communicate to
> all SPI drivers how many dummy bytes are to be transferred.  I'm not fully
> up on SPI, but maybe something like this:

> 	t[0].tx_buf = flash->command;
> 	t[0].len = m25p_cmdsz(nor);
> 	spi_message_add_tail(&t[0], &m);
> 	t[1].tx_buf = dummy_buffer;
> 	t[1].len = dummy;
> 	t[1].dummy = 1;
> 	spi_message_add_tail(&t[1], &m);

> This way, we're describing the transfer to the SPI core, and explicitly
> indicating that there are some dummy bytes.  The SPI driver can then
> tell that these are dummy bytes, and if the SPI message consists of:

That'd work as well, my first thought would to use NULL as a dummy
buffer pointer and let the core substitute in data for the drivers.  We
currently insist on having at least one buffer but that's fixable.

> This would not be a hack to the SPI code: we're describing to the SPI
> code what we want to achieve in terms of the activity on the bus, and
> providing that level of description then allows the SPI driver to make
> informed decisions on whether it can handle the transfer using some
> non-standard feature.

Yup.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

On Thu, Aug 06, 2015 at 05:55:23PM +0530, Vignesh R wrote:

> 1.Write to flash config register via config port to switch to QUAD MODE
> (or any mode that flash supports).
> 2. Populate QSPI_SPI_SETUP_REGx with flash read command, number of
> address bytes to use and dummy bytes required.

These things being constant for a given flash?

> 3. Switch to memory mapped port by writing to QSPI_SPI_SWITCH_REG.
> 4. Now, its possible to perform read from 0x5C000000 to 0x5FFFFFFF using
> memcpy. The qspi controller hardware will communicate over SPI bus and
> get the data. This data is directly sent to RAM via SoC's interconnect.

Presumably if it's done via memcpy() it won't go direct to RAM but
rather be bounced through the CPU which is a bit interesting for
performance (it might help, but it does mean that there's a core stalled
waiting for the flash which might not be the best use of resources if
there's other things it can be getting on with).  With DMA it'd be a
direct to memory transfer though.

> Advantages of memory mapped port are: improved read performance,
> MEM_TO_MEM DMA support can be added (ti-qspi hardware as such does not
> provide DMA events).

DMA would definitely help here.

> I just need to know whether the user that requested the transfer is
> m25p80 driver. If yes, ti-qspi driver can take advantage of memory
> mapped interface, else just use config port to access SPI bus directly.

You don't *really* care if it's that specific user so much as that it's
that particular pattern.

> Writing separate driver based on spi-nor framework to interface with
> m25p80 is not an option because, I would lose the ability to interface
> with non-flash devices.

You could, however, expose an explicit flash mapping interface for this
functionality.  That does seem like it's going to be an awful lot easier 
and help with keeping things like DMA support out of the driver and in
the core (which would be useful if there's other controllers with the
same functionality, I seem to recall that there are).

> The spi_message that is received in transfer_one_message() is too
> generic to imply the slave device that is on the other side of the wire.
> IMO, the read command does not imply that the slave is m25p80 flash
> (besides the read opcodes vary across vendors of m25p80 and across modes).

Again, it doesn't matter if it's actually a read command only that it's
got a compatible format on the bus.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On Thu, Aug 06, 2015 at 12:01:37PM +0200, Michal Suchanek wrote:

> However, I am familiar m25p80.c and as I understand it the controller
> is basically supposed to implement m25p80.c in hardware when this flag
> is set.

But what in concrete terms is that supposed to mean?  It's currently
just an essentially undocumented flag on a message rather than something
operating at the level of a flash chip.  That's pretty much where
Russell's comments come from.

> If I was using m25p80.c to talk to anything but an actual flash chip
> it would get me quite worried.

Sure, but at the end of the day it's just emitting standard SPI messages
which don't know anything about flash.  If those messages are a sensible
interface here then why bother with the flag, we can just pattern match
on the format of the message.  If that doesn't work then probably this
isn't a great interface and a separate, application specific interface
makes more sense.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Michal Suchanek]

On 6 August 2015 at 13:23, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Aug 06, 2015 at 12:01:37PM +0200, Michal Suchanek wrote:
>
>> However, I am familiar m25p80.c and as I understand it the controller
>> is basically supposed to implement m25p80.c in hardware when this flag
>> is set.
>
> But what in concrete terms is that supposed to mean?  It's currently
> just an essentially undocumented flag on a message rather than something
> operating at the level of a flash chip.  That's pretty much where
> Russell's comments come from.
>
>> If I was using m25p80.c to talk to anything but an actual flash chip
>> it would get me quite worried.
>
> Sure, but at the end of the day it's just emitting standard SPI messages
> which don't know anything about flash.  If those messages are a sensible
> interface here then why bother with the flag, we can just pattern match
> on the format of the message.  If that doesn't work then probably this
> isn't a great interface and a separate, application specific interface
> makes more sense.

The messages are sensible interface for communicating with a device
that interprets a particular part of the mesasge as address and
another particular part of the message as command and sends same
amount of junk before reply as the flash chip would. If your device
happens to send reply immediately part of it is trashed. If it happens
to interpret address differently the data ends up in random part of
your memory. So no, that is not something you can autodetect.

At the end of the day you have valid SPI messages but the m25p80 layer
adds interpretation to those messages which may not always give
correct result.

On the other hand, if you ever get to m25p80 or spi-nor you can assume
any message you send goes to a flash chip and insist that the
controller uses the flash-specific interface.

If there is possibility of connecting different kind of devices to
multiple chipselects on the same master then you probably want to
select this option per message or per slave.

Thanks

Michal

Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]

On Thu, Aug 06, 2015 at 01:42:32PM +0200, Michal Suchanek wrote:
> On 6 August 2015 at 13:23, Mark Brown <broonie@kernel.org> wrote:

> > Sure, but at the end of the day it's just emitting standard SPI messages
> > which don't know anything about flash.  If those messages are a sensible
> > interface here then why bother with the flag, we can just pattern match
> > on the format of the message.  If that doesn't work then probably this
> > isn't a great interface and a separate, application specific interface
> > makes more sense.

> The messages are sensible interface for communicating with a device
> that interprets a particular part of the mesasge as address and
> another particular part of the message as command and sends same
> amount of junk before reply as the flash chip would. If your device

That's just a statement that it's possible to do things this way, it's
not clear to me that it's an explanation as to why it is sensible to do
so - the obvious thing to do there would be to specify the flash
operation as a flash operation rather than reverse engineer the flash
operation from the wire format.

> happens to send reply immediately part of it is trashed. If it happens
> to interpret address differently the data ends up in random part of
> your memory. So no, that is not something you can autodetect.

If this stuff doesn't appear in the spi_message in some observable form
then I'd expect that any existing flash support is broken since a SPI
controller that doesn't have any acceleration is just going to do what
the message says.

> At the end of the day you have valid SPI messages but the m25p80 layer
> adds interpretation to those messages which may not always give
> correct result.

Which comes back to the thing I keep saying about needing some sort of
information about what the flag means and the questions about why this
is a good interface...

> On the other hand, if you ever get to m25p80 or spi-nor you can assume
> any message you send goes to a flash chip and insist that the
> controller uses the flash-specific interface.

There's an awful lot of flashes out there connected to controllers that
don't implement any sort of acceleration for flash, I'm not convinced
that your assumption there is valid.

> If there is possibility of connecting different kind of devices to
> multiple chipselects on the same master then you probably want to
> select this option per message or per slave.

We definitely need to account for that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[RFC PATCH 2/5] spi: spi-ti-qspi: Add memory mapped read support

[Vignesh R]

TI QSPI controller has memory mapped port through which SPI flash
memories can be read using memcpy call. This patch adds support for
memory mapped read based on use_mmap_read flag.
When use_mmap_read flag is set, the controller is switched to memory
mapped interface by writing to QSPI_SPI_SWITCH_REG. The read_opcode,
read mode, dummy bytes are configured in QSPI_SPI_SETUPx_REG, then
memcpy is called to copy the requested data from flash to the rx_buf.
With this patch, the read speed increased from ~100kB/s to ~2.5MB/s on
DRA74 EVM.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/spi/spi-ti-qspi.c | 129 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 125 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 5c0616870358..45844a227c5e 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -71,11 +71,8 @@ struct ti_qspi {
 #define QSPI_SPI_CMD_REG		(0x48)
 #define QSPI_SPI_STATUS_REG		(0x4c)
 #define QSPI_SPI_DATA_REG		(0x50)
-#define QSPI_SPI_SETUP0_REG		(0x54)
+#define QSPI_SPI_SETUP_REG(n)		(0x54 + 4 * n)
 #define QSPI_SPI_SWITCH_REG		(0x64)
-#define QSPI_SPI_SETUP1_REG		(0x58)
-#define QSPI_SPI_SETUP2_REG		(0x5c)
-#define QSPI_SPI_SETUP3_REG		(0x60)
 #define QSPI_SPI_DATA_REG_1		(0x68)
 #define QSPI_SPI_DATA_REG_2		(0x6c)
 #define QSPI_SPI_DATA_REG_3		(0x70)
@@ -118,6 +115,16 @@ struct ti_qspi {
 
 #define QSPI_AUTOSUSPEND_TIMEOUT         2000
 
+#define MEM_CS_EN(n)			((n + 1) << 8)
+
+#define MM_SWITCH			0x1
+
+#define QSPI_SETUP_RD_NORMAL		(0x0 << 12)
+#define QSPI_SETUP_RD_DUAL		(0x1 << 12)
+#define QSPI_SETUP_RD_QUAD		(0x3 << 12)
+#define QSPI_SETUP_ADDR_SHIFT		8
+#define QSPI_SETUP_DUMMY_SHIFT		10
+
 static inline unsigned long ti_qspi_read(struct ti_qspi *qspi,
 		unsigned long reg)
 {
@@ -335,6 +342,117 @@ static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	return 0;
 }
 
+static void ti_qspi_enable_memory_map(struct spi_device *spi)
+{
+	struct ti_qspi  *qspi = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	ti_qspi_write(qspi, MM_SWITCH, QSPI_SPI_SWITCH_REG);
+	if (qspi->ctrl_mod) {
+		val = readl(qspi->ctrl_base);
+		val |= MEM_CS_EN(spi->chip_select);
+		writel(val, qspi->ctrl_base);
+	}
+}
+
+static void ti_qspi_disable_memory_map(struct spi_device *spi)
+{
+	struct ti_qspi  *qspi = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	ti_qspi_write(qspi, 0, QSPI_SPI_SWITCH_REG);
+	if (qspi->ctrl_mod) {
+		val = readl(qspi->ctrl_base);
+		val &= ~MEM_CS_EN(spi->chip_select);
+		writel(val, qspi->ctrl_base);
+	}
+}
+
+static void ti_qspi_setup_mmap_read(struct spi_device *spi, u8
+				    read_opcode, u8 addr_width,
+				    u8 dummy_bytes)
+{
+	struct ti_qspi  *qspi = spi_master_get_devdata(spi->master);
+	u32 mode = spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD);
+	u32 memval = read_opcode;
+
+	switch (mode) {
+	case SPI_RX_QUAD:
+		memval |= QSPI_SETUP_RD_QUAD;
+		break;
+	case SPI_RX_DUAL:
+		memval |= QSPI_SETUP_RD_DUAL;
+		break;
+	default:
+		memval |= QSPI_SETUP_RD_NORMAL;
+		break;
+	}
+	memval |= ((addr_width - 1) << QSPI_SETUP_ADDR_SHIFT |
+		   dummy_bytes << QSPI_SETUP_DUMMY_SHIFT);
+	ti_qspi_write(qspi, memval,
+		      QSPI_SPI_SETUP_REG(spi->chip_select));
+}
+
+static unsigned int ti_qspi_cmd2addr(u8 *cmd, u8 addr_width)
+{
+	u32 addr;
+
+	/* cmd[0] is read opcode */
+	addr = cmd[1] << ((addr_width - 1) * 8);
+	addr |= cmd[2] << ((addr_width - 2) * 8);
+	addr |= cmd[3] << ((addr_width - 3) * 8);
+	addr |= cmd[4] << ((addr_width - 4) * 8);
+
+	return addr;
+}
+
+static int ti_qspi_mmap_read(struct spi_master *master,
+			     struct spi_message *m)
+{
+	struct ti_qspi *qspi = spi_master_get_devdata(master);
+	struct spi_device *spi = m->spi;
+	struct spi_transfer *t;
+	u8 read_opcode = 0x3;	/* Default normal read */
+	void *to = NULL;
+	u8 addr_width = 4, dummy_bytes = 0;
+	unsigned int len = 0, from = 0;
+	int status = 0;
+
+	mutex_lock(&qspi->list_lock);
+
+	/* disable WC interrupt during memcpy */
+	ti_qspi_write(qspi, QSPI_WC_INT_DISABLE, QSPI_INTR_ENABLE_CLEAR_REG);
+	ti_qspi_enable_memory_map(spi);
+	list_for_each_entry(t, &m->transfers, transfer_list) {
+		if (t->tx_buf) {
+			read_opcode = *((u8 *)t->tx_buf);
+			dummy_bytes = t->len - (addr_width + 1);
+			from = ti_qspi_cmd2addr((u8 *)t->tx_buf, addr_width);
+		}
+		if (t->rx_buf) {
+			to = ((void *)t->rx_buf);
+			len = t->len;
+		}
+		m->actual_length += t->len;
+	}
+	ti_qspi_setup_mmap_read(spi, read_opcode, addr_width,
+				dummy_bytes);
+
+	if (qspi_is_busy(qspi)) {
+		status = -EBUSY;
+		goto err;
+	}
+	memcpy(to, qspi->mmap_base + from, len);
+
+err:
+	ti_qspi_disable_memory_map(spi);
+	mutex_unlock(&qspi->list_lock);
+	m->status = status;
+	spi_finalize_current_message(master);
+
+	return status;
+}
+
 static int ti_qspi_start_transfer_one(struct spi_master *master,
 		struct spi_message *m)
 {
@@ -344,6 +462,9 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
 	int status = 0, ret;
 	int frame_length;
 
+	if (m->use_mmap_mode)
+		return ti_qspi_mmap_read(master, m);
+
 	/* setup device control reg */
 	qspi->dc = 0;
 
-- 
2.4.6

[RFC PATCH 3/5] mtd: devices: m25p80: set flag to request memory mapped read

[Vignesh R]

Set use_mmap_read flag to true, to indicate to spi-master that the
spi-message is from mtd layer. This helps spi-master to do memory
mapped reads over SPI flash memories, when hardware support is
available.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/devices/m25p80.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index d313f948b96c..aac2121c5454 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -148,6 +148,9 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	t[1].len = len;
 	spi_message_add_tail(&t[1], &m);
 
+	m.spi = spi;
+	m.use_mmap_mode = true;
+
 	spi_sync(spi, &m);
 
 	*retlen = m.actual_length - m25p_cmdsz(nor) - dummy;
-- 
2.4.6

[RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Vignesh R]

Add qspi memory mapped region entries for DRA7xx based SoCs.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 arch/arm/boot/dts/am4372.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index ade28c790f4b..5317a0f24ab9 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -902,7 +902,9 @@
 
 		qspi: qspi@47900000 {
 			compatible = "ti,am4372-qspi";
-			reg = <0x47900000 0x100>;
+			reg = <0x47900000 0x100>,
+			      <0x30000000 0x3ffffff>;
+			reg-names = "qspi_base", "qspi_mmap";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			ti,hwmods = "qspi";
-- 
2.4.6

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Sekhar Nori]

On Tuesday 28 July 2015 02:11 PM, Vignesh R wrote:
> Add qspi memory mapped region entries for DRA7xx based SoCs.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  arch/arm/boot/dts/am4372.dtsi | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> index ade28c790f4b..5317a0f24ab9 100644
> --- a/arch/arm/boot/dts/am4372.dtsi
> +++ b/arch/arm/boot/dts/am4372.dtsi

The patch talks about DRA7x in subject and description but you modify
AM437x here. You have got commit text mixed up between 4/5 and 5/5.

Probably not the kind of feedback you are looking for an RFC, but since
I noticed it..

Thanks,
Sekhar

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Vignesh R]

On 07/31/2015 07:18 PM, Sekhar Nori wrote:
> On Tuesday 28 July 2015 02:11 PM, Vignesh R wrote:
>> Add qspi memory mapped region entries for DRA7xx based SoCs.
>>
>> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/am4372.dtsi | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
>> index ade28c790f4b..5317a0f24ab9 100644
>> --- a/arch/arm/boot/dts/am4372.dtsi
>> +++ b/arch/arm/boot/dts/am4372.dtsi
> 
> The patch talks about DRA7x in subject and description but you modify
> AM437x here. You have got commit text mixed up between 4/5 and 5/5.
> 
> Probably not the kind of feedback you are looking for an RFC, but since
> I noticed it..

Oh, my bad.. I will correct $subject when I submit actual patch series.

-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 498 bytes --]

On Tue, Jul 28, 2015 at 02:11:15PM +0530, Vignesh R wrote:
> Add qspi memory mapped region entries for DRA7xx based SoCs.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>

>  		qspi: qspi@47900000 {
>  			compatible = "ti,am4372-qspi";
> -			reg = <0x47900000 0x100>;
> +			reg = <0x47900000 0x100>,
> +			      <0x30000000 0x3ffffff>;
> +			reg-names = "qspi_base", "qspi_mmap";

The DT binding document for the controller needs to be extended to
document this change in the binding.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Vignesh R]

On 07/31/2015 11:49 PM, Mark Brown wrote:
> On Tue, Jul 28, 2015 at 02:11:15PM +0530, Vignesh R wrote:
>> Add qspi memory mapped region entries for DRA7xx based SoCs.
>>
>> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
> 
>>  		qspi: qspi@47900000 {
>>  			compatible = "ti,am4372-qspi";
>> -			reg = <0x47900000 0x100>;
>> +			reg = <0x47900000 0x100>,
>> +			      <0x30000000 0x3ffffff>;
>> +			reg-names = "qspi_base", "qspi_mmap";
> 
> The DT binding document for the controller needs to be extended to
> document this change in the binding.
> 

DT bindings are already documented at
Documentation/devicetree/bindings/spi/ti_qspi.txt. Did you mean to add
this node as an example in that file?

-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Mon, Aug 03, 2015 at 10:32:09AM +0530, Vignesh R wrote:
> On 07/31/2015 11:49 PM, Mark Brown wrote:

> >> -			reg = <0x47900000 0x100>;
> >> +			reg = <0x47900000 0x100>,
> >> +			      <0x30000000 0x3ffffff>;
> >> +			reg-names = "qspi_base", "qspi_mmap";

> > The DT binding document for the controller needs to be extended to
> > document this change in the binding.

> DT bindings are already documented at
> Documentation/devicetree/bindings/spi/ti_qspi.txt. Did you mean to add
> this node as an example in that file?

No, I mean you're changing the binding to add this new memory region -
that new region needs to be added to the documentation.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Brian Norris]

On Tue, Jul 28, 2015 at 02:11:15PM +0530, Vignesh R wrote:
> Add qspi memory mapped region entries for DRA7xx based SoCs.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  arch/arm/boot/dts/am4372.dtsi | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> index ade28c790f4b..5317a0f24ab9 100644
> --- a/arch/arm/boot/dts/am4372.dtsi
> +++ b/arch/arm/boot/dts/am4372.dtsi
> @@ -902,7 +902,9 @@
>  
>  		qspi: qspi@47900000 {
>  			compatible = "ti,am4372-qspi";
> -			reg = <0x47900000 0x100>;
> +			reg = <0x47900000 0x100>,
> +			      <0x30000000 0x3ffffff>;

Are you sure this region is 1 byte shy of 64MB in length? Same question
for patch 5.

> +			reg-names = "qspi_base", "qspi_mmap";
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			ti,hwmods = "qspi";
> -- 
> 2.4.6
> 

Re: [RFC PATCH 4/5] ARM: dts: DRA7: Add memory map region entries for qspi

[Vignesh R]

On 08/01/2015 02:58 AM, Brian Norris wrote:
> On Tue, Jul 28, 2015 at 02:11:15PM +0530, Vignesh R wrote:
>> Add qspi memory mapped region entries for DRA7xx based SoCs.
>>
>> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/am4372.dtsi | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
>> index ade28c790f4b..5317a0f24ab9 100644
>> --- a/arch/arm/boot/dts/am4372.dtsi
>> +++ b/arch/arm/boot/dts/am4372.dtsi
>> @@ -902,7 +902,9 @@
>>  
>>  		qspi: qspi@47900000 {
>>  			compatible = "ti,am4372-qspi";
>> -			reg = <0x47900000 0x100>;
>> +			reg = <0x47900000 0x100>,
>> +			      <0x30000000 0x3ffffff>;
> 
> Are you sure this region is 1 byte shy of 64MB in length? Same question
> for patch 5.
> 

Oops, my bad... Its 64MB in length, I entered offset of last byte
instead of length. Will correct this in the actual patch.

>> +			reg-names = "qspi_base", "qspi_mmap";
>>  			#address-cells = <1>;
>>  			#size-cells = <0>;
>>  			ti,hwmods = "qspi";
>> -- 
>> 2.4.6
>>

-- 
Regards
Vignesh
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 5/5] ARM: dts: AM4372: Add memory map region entries for qspi

[Vignesh R]

Add qspi memory mapped region entries for AM43xx based SoCs.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 arch/arm/boot/dts/dra7.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 8f1e25bcecbd..75a17c78b0ad 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1103,8 +1103,10 @@
 
 		qspi: qspi@4b300000 {
 			compatible = "ti,dra7xxx-qspi";
-			reg = <0x4b300000 0x100>;
-			reg-names = "qspi_base";
+			reg = <0x4b300000 0x100>, <0x4a002558 0x4>,
+			      <0x5c000000 0x3ffffff>;
+			reg-names = "qspi_base", "qspi_ctrlmod",
+				    "qspi_mmap";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			ti,hwmods = "qspi";
-- 
2.4.6