| Message ID | 1438072876-16338-2-git-send-email-vigneshr@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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?
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
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. Thanks Michal
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.
On 5 August 2015 at 07:35, Vignesh R <vigneshr@ti.com> wrote: > > > 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: >>>> >>> >>> 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
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.
On 5 August 2015 at 13:50, Mark Brown <broonie@kernel.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
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.
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
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.
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
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.
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.
On 6 August 2015 at 12:22, Russell King - ARM Linux <linux@arm.linux.org.uk> 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
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.
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
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
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.
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.
On Thu, Aug 6, 2015 at 3:51 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> 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@linux-m68k.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
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.
On 6 August 2015 at 18:14, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Aug 6, 2015 at 3:51 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> 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
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.
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@arm.linux.org.uk> 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.
On 6 August 2015 at 23:33, Russell King - ARM Linux <linux@arm.linux.org.uk> 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@arm.linux.org.uk> 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
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.
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.
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
On 08/07/2015 03:46 PM, Michal Suchanek wrote: [snip] > 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. >> > I will re-submit patches based on the above idea, if there are no further comments..
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
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(+)