mtd: m25p80: consider max_transfer_size when reading

Some controllers have transfer size limits. To allow to deal with this max_transfer_size was introduced in the SPI core recently. Use this new feature to read in chunks if needed.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/mtd/devices/m25p80.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

drivers/mtd/devices/m25p80.c +7 -1

index c2d1f65..69f3acf 100644
@@ -119,7 +119,7 @@ static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
  * Read an address range from the nor chip.  The address range
  * may be any size provided it is within the physical boundaries.
  */
-static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
+static int _m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 			size_t *retlen, u_char *buf)
 {
 	struct m25p *flash = nor->priv;

@@ -153,6 +153,34 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	return ret;
 }
 
+/* Read in max_read_len chunks if len > max_read_len */
+static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
+		       size_t *retlen, u_char *buf)
+{
+	struct m25p *flash = nor->priv;
+	/*

drivers/mtd/devices/m25p80.c +12 -0

+		read_len = min(len, max_read_len);
+		ret = _m25p80_read(nor, from, read_len, retlen, buf);
+		if (ret)
+			return ret;
+		from += read_len;
+		buf += read_len;
+		len -= read_len;
+	}
+
+	return 0;
+}
+
 /*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver

Signature · 1 line

2.7.1

+ others

Hi Heiner,

Quoted 5 lines · Heiner Kallweit

Some controllers have transfer size limits. To allow to deal with this max_transfer_size was introduced in the SPI core recently. Use this new feature to read in chunks if needed.

Signed-off-by Heiner Kallweit <[email protected]>

Michal has been working on a similar series, with some differences (I'll comment below). I think his latest work is here:

http://lists.infradead.org/pipermail/linux-mtd/2015-December/063865.html

Is there a particular reason you've continued on your series instead of reviewing/fixing his?

Quoted 27 lines

---
 drivers/mtd/devices/m25p80.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

drivers/mtd/devices/m25p80.c +7 -1

index c2d1f65..69f3acf 100644
@@ -119,7 +119,7 @@ static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
  * Read an address range from the nor chip.  The address range
  * may be any size provided it is within the physical boundaries.
  */
-static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
+static int _m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 			size_t *retlen, u_char *buf)
 {
 	struct m25p *flash = nor->priv;

@@ -153,6 +153,34 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	return ret;
 }
 
+/* Read in max_read_len chunks if len > max_read_len */
+static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
+		       size_t *retlen, u_char *buf)
+{
+	struct m25p *flash = nor->priv;
+	/*

drivers/mtd/devices/m25p80.c static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 171

Quoted 10 lines

drivers/mtd/devices/m25p80.c +10 -0

+	 * The controller transfer size limit refers to the overall transfer
+	 * including read command + actually read data. Therefore subtract
+	 * the command size when calculating the max read length.
+	 */
+	size_t max_read_len = spi_max_transfer_size(flash->spi) -
+			      (m25p_cmdsz(nor) + nor->read_dummy / 8);
+	size_t read_len;
+	int ret;
+
+	while (len > 0) {

^^ I really don't want this loop to be in m25p80.c. This should be in the core spi-nor.c, and then the driver (like m25p80.c, but also possibly other non-SPI drivers that also use spi-nor.c) can simply pass its limitation up the spi-nor.c. (See Michal's patch 10.)

So I plan to take some form of Michal's and not yours. This will require a bit of rebasing.

Brian

Quoted 18 lines

drivers/mtd/devices/m25p80.c +12 -0

+		read_len = min(len, max_read_len);
+		ret = _m25p80_read(nor, from, read_len, retlen, buf);
+		if (ret)
+			return ret;
+		from += read_len;
+		buf += read_len;
+		len -= read_len;
+	}
+
+	return 0;
+}
+
 /*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver

Signature · 1 line

2.7.1

Am 05.04.2016 um 21:39 schrieb Brian Norris:

Quoted 10 lines · Heiner Kallweit

Quoted 5 lines

Some controllers have transfer size limits. To allow to deal with this max_transfer_size was introduced in the SPI core recently. Use this new feature to read in chunks if needed.

Signed-off-by Heiner Kallweit <[email protected]>

Michal has been working on a similar series, with some differences (I'll comment below). I think his latest work is here:

Quoted 5 lines

http://lists.infradead.org/pipermail/linux-mtd/2015-December/063865.html

Is there a particular reason you've continued on your series instead of reviewing/fixing his?

Latest version of Michal's patch set was sent beginning of December and has received no comment since then, so I assumed it's dead.
If that's not the case, totally fine with me and I'll have a further look at it.

Quoted 16 lines

Am 05.04.2016 um 21:39 schrieb Brian Norris:

Quoted 15 lines · Heiner Kallweit

Quoted 5 lines

Some controllers have transfer size limits. To allow to deal with this max_transfer_size was introduced in the SPI core recently. Use this new feature to read in chunks if needed.

Signed-off-by Heiner Kallweit <[email protected]>

Michal has been working on a similar series, with some differences (I'll comment below). I think his latest work is here:

http://lists.infradead.org/pipermail/linux-mtd/2015-December/063865.html

Is there a particular reason you've continued on your series instead of reviewing/fixing his?

Quoted 3 lines

Latest version of Michal's patch set was sent beginning of December and has received no comment since then, so I assumed it's dead.
If that's not the case, totally fine with me and I'll have a further look at it.

There was a comment about the loop bounds changing away from page alignment. I think otherwise it's about ready to go.

If you or Michal don't take a look at it, I'll rebase and resubmit it.

Brian

Am 05.04.2016 um 23:07 schrieb Brian Norris:

Quoted 7 lines · Heiner Kallweit

Quoted 6 lines

Am 05.04.2016 um 21:39 schrieb Brian Norris:

Quoted 5 lines

Michal has been working on a similar series, with some differences (I'll comment below). I think his latest work is here:

http://lists.infradead.org/pipermail/linux-mtd/2015-December/063865.html

I had a closer look at Michal's patch set, few remarks:

Patch 2 changes the semantics of the return value of m25p80_read/write and the related change to spi_nor_read/write is part of patch 4.
Means if the first patches are applied only we get a faulty behavior. Usually this is undesirable, not sure whether it's acceptable here.

Patch 2

(quoted diff) +3 -0

+	ret = m.actual_length - cmd_sz;
+	if (ret < 0)
+		return -EIO;

I think we should add special handling for the case ret == 0. Usually this would indicate an error however there might be intentional zero-length read's (not sure about that). Therefore I'd propose to change the error condition to if (ret < 0 || (!ret && len))
If zero-length reads are not possible we can simply change it to if (ret <= 0)

Patch 7
W/o the proposed change to patch 2 the case that nor->read() returns 0 isn't handled correctly.
We'd bail out from the read loop but return 0. Instead we should return an error in this case.

With the change to patch 2 the case that nor->read() returns 0 can't happen and we should change the error condition to if (ret < 0) for the sake of clarity.

Patch 8
Made it to mainline already, can be removed.

Patch 10
1. min_t isn't needed here because both arguments are of type size_t.

2. At least in the case of fsl-espi the size limit refers to one physical transfer (including the command) and therefore to the sum of all transfers.
We should change

(quoted diff) +1 -0

+	t[1].len = min_t(size_t, len, spi_max_transfer_size(spi));

to

(quoted diff) +1 -0

+	t[1].len = min(len, spi_max_transfer_size(spi) - t[0].len);

Apart from that the patch set looks good to me.

Quoted 9 lines · Heiner Kallweit

2. At least in the case of fsl-espi the size limit refers to one physical transfer (including the command) and therefore to the sum of all transfers.
We should change

(quoted diff) +1 -0

+	t[1].len = min_t(size_t, len, spi_max_transfer_size(spi));

to

(quoted diff) +1 -0

+	t[1].len = min(len, spi_max_transfer_size(spi) - t[0].len);

Apart from that the patch set looks good to me.

That's not what Mark specified here:

http://lists.infradead.org/pipermail/linux-mtd/2015-November/063616.html

and that's not what the API's very *name* means; it says max transfer size (where a spi_transfer is a very well-defined concept). You need to fix the driver or take up the API issues with Mark if you want to suggest we interpret this differently.

Quoted 15 lines · Heiner Kallweit

Quoted 10 lines

2. At least in the case of fsl-espi the size limit refers to one physical transfer (including the command) and therefore to the sum of all transfers.
We should change

(quoted diff) +1 -0

+	t[1].len = min_t(size_t, len, spi_max_transfer_size(spi));

to

(quoted diff) +1 -0

+	t[1].len = min(len, spi_max_transfer_size(spi) - t[0].len);

Apart from that the patch set looks good to me.

That's not what Mark specified here:

http://lists.infradead.org/pipermail/linux-mtd/2015-November/063616.html

Quoted 4 lines

and that's not what the API's very *name* means; it says max transfer size (where a spi_transfer is a very well-defined concept). You need to fix the driver or take up the API issues with Mark if you want to suggest we interpret this differently.

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 2 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

Quoted 4 lines · Heiner Kallweit

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 2 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

Quoted 4 lines

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

Quoted 6 lines · Mark Brown

Quoted 4 lines · Heiner Kallweit

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

Quoted 2 lines

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

I believe Heiner has an (unstated here, but stated elsewhere?) assumption that his driver has a maximum *message* size, not *transfer* size. So he's explaining how to work around that by implicitly figuring out what the message size would be based on a transfer size, I think.

Quoted 10 lines · Brian Norris

Quoted 9 lines · Mark Brown

Quoted 4 lines · Heiner Kallweit

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

Quoted 4 lines

I believe Heiner has an (unstated here, but stated elsewhere?) assumption that his driver has a maximum *message* size, not *transfer* size. So he's explaining how to work around that by implicitly figuring out what the message size would be based on a transfer size, I think.

That still wouldn't explain how this could depend on the connected device, the message size isn't going to change any more than the transfer size is.

message size / transfer size patterns for arbitrary users. I suppose we really just need to figure out what his actual problem is, so we can address it generically.

Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.

Quoted 5 lines

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 3 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

Quoted 4 lines

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 12 lines · Heiner Kallweit

Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.

Quoted 5 lines

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 3 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

Quoted 8 lines

Quoted 4 lines

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

If deactivating and reactivating CS between two transfers doesn't affect the functionality of a spi device then we can go with the full max transfer size.

In case of SPI NOR CS needs to stay active between command and data read. Therefore the two transfers in the m25p80 read SPI message need to be combined to one physical transfer by the controller driver. Result is that the max read size is effectively reduced by the length of the m25p80 read command.

Quoted 29 lines

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 12 lines · Heiner Kallweit

Please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.

Quoted 5 lines

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 3 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

Quoted 8 lines

Quoted 4 lines

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

If deactivating and reactivating CS between two transfers doesn't affect the functionality of a spi device then we can go with the full max transfer size.

Quoted 5 lines

In case of SPI NOR CS needs to stay active between command and data read. Therefore the two transfers in the m25p80 read SPI message need to be combined to one physical transfer by the controller driver. Result is that the max read size is effectively reduced by the length of the m25p80 read command.

Which is what the dummy bytes are used for AFAIK.
They are received while you are sending the command and white the flash is getting ready to send data and after the transfer are deducted from the received data in m25p80.

If this does not work for fsl-espi for some reason it should be corrected. I am working with a controller that can transfer 63 bytes at once and I am quite positive it does not do any magic transfer coalescing. Still it can read a 4Mbyte flash chip.

However, spi-sun4 implements transfer_one and fsl-espi implements transfer_one_message so the different interface may work out differently.

Indeed, the spi.c transfer_one_message calls set_cs and then calls transfer_one repeatedly which is exactly what fsl-espi cannot do due to automagic CS handling. So the fsl-espi limit is probably on message size.

Maybe adding another quirk the m25p80 driver can query to know to deduct the command size from the maximum data size would resolve both cases.

Thanks

Michal

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 11 lines · Heiner Kallweit

Quoted 9 lines

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 3 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

Quoted 3 lines

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

Hi Heiner,

Quoted 15 lines · Heiner Kallweit

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 14 lines · Heiner Kallweit

Quoted 9 lines

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 3 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

Quoted 3 lines

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

And you can't use pinmux to configure the pin used for hardware CS to become a GPIO?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

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

Am 06.06.2016 um 21:46 schrieb Geert Uytterhoeven:

Quoted 22 lines

Hi Heiner,

Quoted 18 lines · Heiner Kallweit

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 6 lines · Heiner Kallweit

Quoted 4 lines

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 3 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

Quoted 8 lines

Quoted 4 lines

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

Quoted 3 lines

And you can't use pinmux to configure the pin used for hardware CS to become a GPIO?

Sadly enough, no. Only the complete block of SPI signals can be switched to GPIO mode. But then we'd have to use bitbanging and would loose all features of the integrated SPI controller (FIFO's etc.)

Quoted 23 lines

Am 06.06.2016 um 21:46 schrieb Geert Uytterhoeven:

Quoted 22 lines

Hi Heiner,

Quoted 18 lines · Heiner Kallweit

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 6 lines · Heiner Kallweit

Quoted 4 lines

Am 06.05.2016 um 14:14 schrieb Mark Brown:

Quoted 3 lines

Yes, it's called the maximum transfer size because it is the maximum size of a transfer, not because it's the maximum size of a message.

Quoted 8 lines

Quoted 4 lines

I'd like to come back to this discussion. You said best would be to fix the chip driver. To do this and calculate an appropriate value for max_transfer_size the chip driver would have to know that the spi_device is a spi-nor device.

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

Quoted 6 lines

Quoted 3 lines

And you can't use pinmux to configure the pin used for hardware CS to become a GPIO?

Sadly enough, no. Only the complete block of SPI signals can be switched to GPIO mode. But then we'd have to use bitbanging and would loose all features of the integrated SPI controller (FIFO's etc.)

Just for completeness, which SoC are we talking about here ?

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 2 lines

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

In case of SPI NOR CS needs to stay active between command and data

Well, we could reduce the max_transfer_size exposed by the driver by the max length of a m25p80 read command in general.

Then we might be too strict for other SPI devices, but this may be acceptable as the current fsl-espi driver is usable with SPI NOR devices only anyway.

Quoted 8 lines

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 2 lines

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

There's no pinmux to fix the broken behaviour? The Freescale SPI controllers really are terrible :(

Quoted 1 line

In case of SPI NOR CS needs to stay active between command and data

In the case of *all* SPI transactions this needs to happen. This is *not* optional, it's not some weird requirement of NOR, it's one of the most basic requirements for a SPI driver on Linux.

Quoted 2 lines

Well, we could reduce the max_transfer_size exposed by the driver by the max length of a m25p80 read command in general.

No! Apart from anything else this would be a complete and total abstraction failure. This is a driver for a SPI controller, not a driver for a flash controller, which means that it should not have flash specific hacks in it.

Quoted 3 lines

Then we might be too strict for other SPI devices, but this may be acceptable as the current fsl-espi driver is usable with SPI NOR devices only anyway.

What makes you say this? I'm guessing that the driver is just broken but it would be good to understand the specifics.

Am 07.06.2016 um 01:07 schrieb Mark Brown:

Quoted 15 lines · Heiner Kallweit

Quoted 4 lines

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Well, we could reduce the max_transfer_size exposed by the driver by the max length of a m25p80 read command in general.

No! Apart from anything else this would be a complete and total abstraction failure. This is a driver for a SPI controller, not a driver for a flash controller, which means that it should not have flash specific hacks in it.

Quoted 3 lines

Then we might be too strict for other SPI devices, but this may be acceptable as the current fsl-espi driver is usable with SPI NOR devices only anyway.

Quoted 3 lines

What makes you say this? I'm guessing that the driver is just broken but it would be good to understand the specifics.

See fsl_espi_rw_trans():
In case of transfers > SPCOM_TRANLEN_MAX it loops and reads in SPCOM_TRANLEN_MAX chunks whilst replacing bytes 2-4 of the message buffer with the new start address to read from.

Quoted 19 lines

Am 07.06.2016 um 01:07 schrieb Mark Brown:

Quoted 18 lines · Heiner Kallweit

Quoted 4 lines

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Well, we could reduce the max_transfer_size exposed by the driver by the max length of a m25p80 read command in general.

No! Apart from anything else this would be a complete and total abstraction failure. This is a driver for a SPI controller, not a driver for a flash controller, which means that it should not have flash specific hacks in it.

Quoted 3 lines

Then we might be too strict for other SPI devices, but this may be acceptable as the current fsl-espi driver is usable with SPI NOR devices only anyway.

What makes you say this? I'm guessing that the driver is just broken but it would be good to understand the specifics.

Quoted 4 lines

See fsl_espi_rw_trans():
In case of transfers > SPCOM_TRANLEN_MAX it loops and reads in SPCOM_TRANLEN_MAX chunks whilst replacing bytes 2-4 of the message buffer with the new start address to read from.

That's obviously bogus. Just reject anything too large and let the upper layer deal with it.

Given m25p80 and fbtft would handle this can you use DMA to gather buffers from multiple transfers (ie command and data) or does the buffer have to be copied together for the transfer?

Thanks

Michal

Am 07.06.2016 um 01:07 schrieb Mark Brown:

Quoted 27 lines · Heiner Kallweit

Quoted 8 lines

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 2 lines

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

There's no pinmux to fix the broken behaviour? The Freescale SPI controllers really are terrible :(

Quoted 1 line

In case of SPI NOR CS needs to stay active between command and data

In the case of *all* SPI transactions this needs to happen. This is *not* optional, it's not some weird requirement of NOR, it's one of the most basic requirements for a SPI driver on Linux.

Quoted 2 lines

Well, we could reduce the max_transfer_size exposed by the driver by the max length of a m25p80 read command in general.

No! Apart from anything else this would be a complete and total abstraction failure. This is a driver for a SPI controller, not a driver for a flash controller, which means that it should not have flash specific hacks in it.

Quoted 7 lines

Quoted 3 lines

Then we might be too strict for other SPI devices, but this may be acceptable as the current fsl-espi driver is usable with SPI NOR devices only anyway.

What makes you say this? I'm guessing that the driver is just broken but it would be good to understand the specifics.

See fsl_espi_rw_trans():
In case of transfers > SPCOM_TRANLEN_MAX it loops and reads in SPCOM_TRANLEN_MAX chunks whilst replacing bytes 2-4 of the message buffer with the new start address to read from. Having said that it implicitely assumes that the first transfer in the message is a m25p80 read command with a 3 byte address.

And in fsl_espi_do_one_msg() it assumes that each message includes max one r/o transfer (else rx_buf is overwritten).

I'm not 100% sure whether any arbitrary combination of r/o, r/w, w/o transfers in a SPI message is acceptable. But this controller driver supports just the transfer combinations generated by the m25p80 protocol driver.

Am 07.06.2016 um 08:03 schrieb Heiner Kallweit:

Quoted 42 lines

Am 07.06.2016 um 01:07 schrieb Mark Brown:

Quoted 27 lines · Heiner Kallweit

Quoted 8 lines

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 2 lines

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

There's no pinmux to fix the broken behaviour? The Freescale SPI controllers really are terrible :(

Quoted 1 line

In case of SPI NOR CS needs to stay active between command and data

In the case of *all* SPI transactions this needs to happen. This is *not* optional, it's not some weird requirement of NOR, it's one of the most basic requirements for a SPI driver on Linux.

Quoted 2 lines

Well, we could reduce the max_transfer_size exposed by the driver by the max length of a m25p80 read command in general.

No! Apart from anything else this would be a complete and total abstraction failure. This is a driver for a SPI controller, not a driver for a flash controller, which means that it should not have flash specific hacks in it.

Quoted 7 lines

Quoted 3 lines

Then we might be too strict for other SPI devices, but this may be acceptable as the current fsl-espi driver is usable with SPI NOR devices only anyway.

What makes you say this? I'm guessing that the driver is just broken but it would be good to understand the specifics.

See fsl_espi_rw_trans():
In case of transfers > SPCOM_TRANLEN_MAX it loops and reads in SPCOM_TRANLEN_MAX chunks whilst replacing bytes 2-4 of the message buffer with the new start address to read from. Having said that it implicitely assumes that the first transfer in the message is a m25p80 read command with a 3 byte address.

Quoted 8 lines

And in fsl_espi_do_one_msg() it assumes that each message includes max one r/o transfer (else rx_buf is overwritten).

I'm not 100% sure whether any arbitrary combination of r/o, r/w, w/o transfers in a SPI message is acceptable. But this controller driver supports just the transfer combinations generated by the m25p80 protocol driver.

I think in this discussion there was the idea already to introduce a message size limit complementing the transfer size limit. Could this be an acceptable way to go?
If yes I'd prepare a patch as basis for further discussion.

Quoted 51 lines

Am 07.06.2016 um 08:03 schrieb Heiner Kallweit:

Quoted 42 lines

Am 07.06.2016 um 01:07 schrieb Mark Brown:

Quoted 27 lines · Heiner Kallweit

Quoted 8 lines

Am 06.06.2016 um 19:40 schrieb Mark Brown:

Quoted 2 lines

That doesn't make any sense, the controller hardware doesn't magically change based on what is connected to it.

The issue with fsl-espi is that the controller deactivates CS after each physical transfer. And a lot of HW designs use the hardware CS, therefore the advise to use a GPIO instead doesn't really help.

There's no pinmux to fix the broken behaviour? The Freescale SPI controllers really are terrible :(

Quoted 1 line

In case of SPI NOR CS needs to stay active between command and data

In the case of *all* SPI transactions this needs to happen. This is *not* optional, it's not some weird requirement of NOR, it's one of the most basic requirements for a SPI driver on Linux.

Quoted 2 lines

Well, we could reduce the max_transfer_size exposed by the driver by the max length of a m25p80 read command in general.

No! Apart from anything else this would be a complete and total abstraction failure. This is a driver for a SPI controller, not a driver for a flash controller, which means that it should not have flash specific hacks in it.

Quoted 7 lines

Quoted 3 lines

Then we might be too strict for other SPI devices, but this may be acceptable as the current fsl-espi driver is usable with SPI NOR devices only anyway.

What makes you say this? I'm guessing that the driver is just broken but it would be good to understand the specifics.

See fsl_espi_rw_trans():
In case of transfers > SPCOM_TRANLEN_MAX it loops and reads in SPCOM_TRANLEN_MAX chunks whilst replacing bytes 2-4 of the message buffer with the new start address to read from. Having said that it implicitely assumes that the first transfer in the message is a m25p80 read command with a 3 byte address.

Quoted 8 lines

And in fsl_espi_do_one_msg() it assumes that each message includes max one r/o transfer (else rx_buf is overwritten).

I'm not 100% sure whether any arbitrary combination of r/o, r/w, w/o transfers in a SPI message is acceptable. But this controller driver supports just the transfer combinations generated by the m25p80 protocol driver.

Quoted 5 lines

I think in this discussion there was the idea already to introduce a message size limit complementing the transfer size limit. Could this be an acceptable way to go?
If yes I'd prepare a patch as basis for further discussion.

I think it would be better to introduce a flag saying that the transfer size limit is also a message size limit. We don't have a controller which has different limit for both and protocol drivers that only send one transfer in the message do not need to care that way.

Also it might be nice to add functionality to spi core that detects drivers that don't have neither set_cs nor cs gpio and do message coalescing for them in the core transfer_one_message.

You could allocate two buffers of the same size, copy the transfer bits into the transfer buffer, do a transfer, and copy the receive bits out of the receive buffer.

Thanks

Michal

+ others

Hi Heiner,

Quoted 5 lines · Heiner Kallweit

Some controllers have transfer size limits. To allow to deal with this max_transfer_size was introduced in the SPI core recently. Use this new feature to read in chunks if needed.

Signed-off-by Heiner Kallweit <[email protected]>

Michal has been working on a similar series, with some differences (I'll comment below). I think his latest work is here:

http://lists.infradead.org/pipermail/linux-mtd/2015-December/063865.html

Is there a particular reason you've continued on your series instead of reviewing/fixing his?

Quoted 34 lines

---
 drivers/mtd/devices/m25p80.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

drivers/mtd/devices/m25p80.c +14 -1

index c2d1f65..69f3acf 100644
@@ -119,7 +119,7 @@ static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
  * Read an address range from the nor chip.  The address range
  * may be any size provided it is within the physical boundaries.
  */
-static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
+static int _m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 			size_t *retlen, u_char *buf)
 {
 	struct m25p *flash = nor->priv;

@@ -153,6 +153,34 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	return ret;
 }
 
+/* Read in max_read_len chunks if len > max_read_len */
+static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
+		       size_t *retlen, u_char *buf)
+{
+	struct m25p *flash = nor->priv;
+	/*
+	 * The controller transfer size limit refers to the overall transfer
+	 * including read command + actually read data. Therefore subtract
+	 * the command size when calculating the max read length.
+	 */
+	size_t max_read_len = spi_max_transfer_size(flash->spi) -
+			      (m25p_cmdsz(nor) + nor->read_dummy / 8);
+	size_t read_len;

drivers/mtd/devices/m25p80.c static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len, · line 171

Quoted 3 lines

drivers/mtd/devices/m25p80.c +3 -0

+	int ret;
+
+	while (len > 0) {

^^ I really don't want this loop to be in m25p80.c. This should be in the core spi-nor.c, and then the driver (like m25p80.c, but also possibly other non-SPI drivers that also use spi-nor.c) can simply pass its limitation up the spi-nor.c. (See Michal's patch 10.)