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

From: Martin Sperl <martin@sperl.org>
To: Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Vignesh R <vigneshr@ti.com>, Michal Suchanek <hramrach@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	Brian Norris <computersforpeace@gmail.com>,
	Tony Lindgren <tony@atomide.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	MTD Maling List <linux-mtd@lists.infradead.org>,
	"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
	David Woodhouse <dwmw2@infradead.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [RFC PATCH 1/5] spi: introduce flag for memory mapped read
Date: Fri, 07 Aug 2015 10:25:40 +0200	[thread overview]
Message-ID: <55C46B84.3010304@sperl.org> (raw)
In-Reply-To: <20150806213340.GK7576@n2100.arm.linux.org.uk>

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.