Hi,
On Wed, Nov 11, 2015 at 12:20:46PM +0530, R, Vignesh wrote:
> On 11/11/2015 4:53 AM, Brian Norris wrote:
> > On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote:
> >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> >> index cce80e6dc7d1..2f2c431b8917 100644Hi
> >> --- a/include/linux/spi/spi.h
> >> +++ b/include/linux/spi/spi.h
> >> @@ -361,6 +361,11 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
> >> * @handle_err: the subsystem calls the driver to handle an error that occurs
> >> * in the generic implementation of transfer_one_message().
> >> * @unprepare_message: undo any work done by prepare_message().
> >> + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory.
> >> + * Flash drivers (like m25p80) can request memory
> >> + * mapped read via this method. This interface
> >> + * should only be used by mtd flashes and cannot be
> >> + * used by other spi devices.
> >> * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS
> >> * number. Any individual value may be -ENOENT for CS lines that
> >> * are not GPIOs (driven by the SPI controller itself).
> >> @@ -507,6 +512,11 @@ struct spi_master {
> >> struct spi_message *message);
> >> int (*unprepare_message)(struct spi_master *master,
> >> struct spi_message *message);
> >> + int (*spi_mtd_mmap_read)(struct spi_device *spi,
> >> + loff_t from, size_t len,
> >> + size_t *retlen, u_char *buf,
> >> + u8 read_opcode, u8 addr_width,
> >> + u8 dummy_bytes);
> >
> > Is this API really sufficient? There are actually quite a few other
> > flash-related parameters that might be relevant to a controller. I
> > presume you happen not hit them because of the particular cases you're
> > using this for right now, but:
> >
> > * How many I/O lines are being used? These can vary depending on the
> > type of flash and the number of I/O lines supported by the controller
> > and connected on the board.
> >
>
> This API communicates whatever data is currently communicated via
> spi_message through spi_sync/transfer_one interfaces.
No it doesn't. A spi_message consists of a list of spi_transfer's, and
each spi_transfer has tx_nbits and rx_nbits fields.
> > * The previous point can vary across parts of the message. There are
> > various combinations of 1/2/4 lines used for opcode/address/data. We
> > only support a few of those combinations in m25p80 right now, but
> > you're not specifying any of that in this API. I guess you're just
> > making assumptions? (BTW, I think there are others having problems
> > with the difference between different "quad" modes on Micron flash; I
> > haven't sorted through all the discussion there.)
> >
>
> How is the spi controller currently being made aware of this via
> m25p80_read / spi_sync() interface? AFAIK, mode field of spi_device
> struct tell whether to do normal/dual/quad read but there is no info
> communicated wrt 1/2/4 opcode/address/data combinations.
Yes there is. m25p80 fills out spi_transfer::rx_nbits. Currently, we
only use this for the data portion, but it's possible to support more
lines for the address and opcode portions too, using the rx_nbits for
the opcode and address spi_transfer struct(s) (currently, m25p80_read()
uses 2 spi_transfers per message, where the first one contains opcode +
address + dummy on a single line, and the second transfer receives the
data on 1, 2, or 4 lines).
> And there is no
> info indicating capabilities of spi-master wrt no of IO lines for
> opcode/address/data that it can support.
For a true SPI controller, there is no need to specify something
different for opcode/address/data, since all those are treated the same;
they're just bits on 1, 2, or 4 lines. So the SPI_{TX,RX}_{DUAL,QUAD}
mode flags in struct spi_master tell m25p80 all it needs to know.
> > There are typically both flash device and SPI controller constraints
> > on this question, so there needs to be some kind of negotiation
> > involved, I expect. Or at least, the SPI master needs to expose which
> > modes it can support with this flash-read API.
> >
>
> If spi-master capabilities are known
For generic SPI handling, these are already known. But now you're adding
flash-specific capabilities, and I'm not going to assume that all
accelerated-read (e.g., your TI mmap'ed flash read) support all the same
modes as your generic modes.
So, which modes does your mmap'ed read handle? 1/1/1? 1/1/2? 1/1/4?
4/4/4? (where x/y/z means x lines for opcode, y lines for address, and z
lines for data)
> then spi_mmap_read_supported() (or
> a new function perhaps) can be used for negotiation. These capabilities
> can be added incrementally once ability to specify spi-master
> capabilities are in place.
spi_master capabilities are already in place. So now you need to
describe them for your new interface too.
> > Also, this API doesn't actually have anything to do with memory mapping.
> > It has to do with the de facto standard flash protocol. So I don't think
> > mmap belongs in the name; it should be something about flash. (I know of
> > at least one other controller that could probably use this API, excpet
> > it doesn't use memory mapping to accomplish the accelerated flash read.)
> >
>
> As far as TI QSPI controller is concerned, the accelerated read happens
> via mmap port whereby a predefined memory address space of SoC is
> exposed as QSPI mmap region. This region can be accessed like normal
> RAM(via memcpy()) and the QSPI controller interface takes care of
> fetching data from flash on SPI bus automatically
I understand all that, but the API as it currently stands is not tied to
that implementation at all.
> hence, I named it as
> above. But, I have no hard feelings if it needs to be generalized to
> spi_mtd_read() or something else.
Maybe spi_flash_read()? It's technically not limited to MTD, though it
probably would only be used there. (And please, don't tread on the
'spi_nor_' prefix, as we already have a related, but distinct, framework
using that.)
Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
