[PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[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 <hkallweit1@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index c2d1f65..69f3acf 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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;
+	int ret;
+
+	while (len > 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
-- 
2.7.1

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Brian Norris]

+ others

Hi Heiner,

On Sat, Feb 27, 2016 at 11:50:11PM +0100, Heiner Kallweit wrote:
> 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 <hkallweit1@gmail.com>

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?

> ---
>  drivers/mtd/devices/m25p80.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index c2d1f65..69f3acf 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -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;
> +	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

> +		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
> -- 
> 2.7.1
> 

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 05.04.2016 um 21:39 schrieb Brian Norris:
> + others
> 
> Hi Heiner,
> 
> On Sat, Feb 27, 2016 at 11:50:11PM +0100, Heiner Kallweit wrote:
>> 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 <hkallweit1@gmail.com>
> 
> 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?
> 
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.

Regards, Heiner

>> ---
>>  drivers/mtd/devices/m25p80.c | 30 +++++++++++++++++++++++++++++-
>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index c2d1f65..69f3acf 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -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;
>> +	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
> 
>> +		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
>> -- 
>> 2.7.1
>>
> 

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Brian Norris]

On Tue, Apr 05, 2016 at 10:08:35PM +0200, Heiner Kallweit wrote:
> Am 05.04.2016 um 21:39 schrieb Brian Norris:
> > On Sat, Feb 27, 2016 at 11:50:11PM +0100, Heiner Kallweit wrote:
> >> 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 <hkallweit1@gmail.com>
> > 
> > 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?
> > 
> 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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 05.04.2016 um 23:07 schrieb Brian Norris:
> On Tue, Apr 05, 2016 at 10:08:35PM +0200, Heiner Kallweit wrote:
>> Am 05.04.2016 um 21:39 schrieb Brian Norris:
>>> On Sat, Feb 27, 2016 at 11:50:11PM +0100, Heiner Kallweit wrote:
>>>> 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 <hkallweit1@gmail.com>
>>>
>>> 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?
>>>
>> 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.
> 
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
+	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
+	t[1].len = min_t(size_t, len, spi_max_transfer_size(spi));
to
+	t[1].len = min(len, spi_max_transfer_size(spi) - t[0].len);

Apart from that the patch set looks good to me.

Heiner

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Brian Norris]

+ Mark, linux-spi (see the last bit)

Hi Heiner,

On Thu, Apr 07, 2016 at 09:09:56PM +0200, Heiner Kallweit wrote:
> Am 05.04.2016 um 23:07 schrieb Brian Norris:
> > On Tue, Apr 05, 2016 at 10:08:35PM +0200, Heiner Kallweit wrote:
> >> Am 05.04.2016 um 21:39 schrieb Brian Norris:
> >>>
> >>> 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:

I would have much preferred you send your reviews in reply to his patch
emails...

> 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.

I think I've fixed that up here. I'll resend.

> Patch 2
> +	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)

Why should we do this in m25p80? I'm doing it in spi-nor.c.

> 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.

Right. I've fixed this in patch 7, not in patch 2.

> 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.

Of course.

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

Fixed.

> 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
> +	t[1].len = min_t(size_t, len, spi_max_transfer_size(spi));
> to
> +	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.

I won't be changing this bit for now.

Brian

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Mark Brown]

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

On Thu, May 05, 2016 at 04:57:00PM -0700, Brian Norris wrote:
> On Thu, Apr 07, 2016 at 09:09:56PM +0200, Heiner Kallweit wrote:

> > 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
> > +	t[1].len = min_t(size_t, len, spi_max_transfer_size(spi));
> > to
> > +	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.

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.

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 06.05.2016 um 14:14 schrieb Mark Brown:
> On Thu, May 05, 2016 at 04:57:00PM -0700, Brian Norris wrote:
>> On Thu, Apr 07, 2016 at 09:09:56PM +0200, Heiner Kallweit wrote:
> 
>>> 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
>>> +	t[1].len = min_t(size_t, len, spi_max_transfer_size(spi));
>>> to
>>> +	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.
> 
> 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.
We could check that the name of the driver bound to the spi-device is m25p80.
This doesn't sound very nice.

Or maybe better, struct spi_device could be extended by a pointer to a struct spi_nor.
This comment in the definition of struct spi_device seems to point in this direction:
"likely need more hooks for more protocol options affecting how the controller
talks to each chip"
Just storing the length of the read command + dummy bytes would also be sufficient.
Could this be a feasible way?

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Mark Brown]

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

On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:

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.

> Am 06.05.2016 um 14:14 schrieb Mark Brown:

> > 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.

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Brian Norris]

On Mon, Jun 06, 2016 at 06:40:03PM +0100, Mark Brown wrote:
> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
> > Am 06.05.2016 um 14:14 schrieb Mark Brown:
> 
> > > 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.

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.

What I don't understand yet is whether there's some HW limitation, or
just a SW limitation, for handling *transfers* as large as
SPCOM_TRANLEN_MAX in drivers/spi/spi-fsl-espi.c, with potentially
multiple of those transfers in a single message. I would think that each
SPI transfer can mostly be handled individually. But if not, then as
mentioned previously, we'd need a new API for that: ->max_msg_size().

Brian

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Mark Brown]

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

On Mon, Jun 06, 2016 at 11:28:03AM -0700, Brian Norris wrote:
> On Mon, Jun 06, 2016 at 06:40:03PM +0100, Mark Brown wrote:
> > On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:

> > > 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.

> 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.

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Brian Norris]

On Mon, Jun 06, 2016 at 07:34:26PM +0100, Mark Brown wrote:
> On Mon, Jun 06, 2016 at 11:28:03AM -0700, Brian Norris wrote:
> > On Mon, Jun 06, 2016 at 06:40:03PM +0100, Mark Brown wrote:
> > > On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
> 
> > > > 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.
> 
> > 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.

Ah, well I bet Heiner's mostly just testing flash (m25p80) and/or m25p80
is one of the bigger stressors.

Anyway, you're definitely correct that Heiner's suggested approach is
not good. He's essentially implying his driver should be intuiting the
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.

Brian

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Mark Brown]

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

On Mon, Jun 06, 2016 at 11:43:48AM -0700, Brian Norris wrote:

> 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.

Yes, exactly.

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 06.06.2016 um 19:40 schrieb Mark Brown:
> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
> 
> 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.
> 
>> Am 06.05.2016 um 14:14 schrieb Mark Brown:
> 
>>> 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.
> 
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.

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.

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Michal Suchanek]

On 6 June 2016 at 20:53, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
>>
>> 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.
>>
>>> Am 06.05.2016 um 14:14 schrieb Mark Brown:
>>
>>>> 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.
>>
> 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.

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 06.06.2016 um 21:40 schrieb Michal Suchanek:
> On 6 June 2016 at 20:53, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
>>>
>>> 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.
>>>
>>>> Am 06.05.2016 um 14:14 schrieb Mark Brown:
>>>
>>>>> 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.
>>>
>> 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.
> 
> 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.
> 
Thanks for the feedback.
The dummy bytes are inserted *after* the read command.
Therefore in the low speed modes usually no dummy bytes are needed.

> 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
> 

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Geert Uytterhoeven]

Hi Heiner,

On Mon, Jun 6, 2016 at 8:53 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
>>> Am 06.05.2016 um 14:14 schrieb Mark Brown:
>>>> 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.
>>
> 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 -- 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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 06.06.2016 um 21:46 schrieb Geert Uytterhoeven:
> Hi Heiner,
> 
> On Mon, Jun 6, 2016 at 8:53 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
>>>> Am 06.05.2016 um 14:14 schrieb Mark Brown:
>>>>> 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.
>>>
>> 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?
> 
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.)

> 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
> 

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Marek Vasut]

On 06/06/2016 11:20 PM, Heiner Kallweit wrote:
> Am 06.06.2016 um 21:46 schrieb Geert Uytterhoeven:
>> Hi Heiner,
>>
>> On Mon, Jun 6, 2016 at 8:53 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>>> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
>>>>> Am 06.05.2016 um 14:14 schrieb Mark Brown:
>>>>>> 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.
>>>>
>>> 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?
>>
> 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 ?

-- 
Best regards,
Marek Vasut

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 07.06.2016 um 00:28 schrieb Marek Vasut:
> On 06/06/2016 11:20 PM, Heiner Kallweit wrote:
>> Am 06.06.2016 um 21:46 schrieb Geert Uytterhoeven:
>>> Hi Heiner,
>>>
>>> On Mon, Jun 6, 2016 at 8:53 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>>>> On Sat, Jun 04, 2016 at 12:22:37AM +0200, Heiner Kallweit wrote:
>>>>>> Am 06.05.2016 um 14:14 schrieb Mark Brown:
>>>>>>> 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.
>>>>>
>>>> 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?
>>>
>> 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 ?
> 
Freescale (now NXP) P1014

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Mark Brown]

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

On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
> Am 06.06.2016 um 19:40 schrieb Mark Brown:

> > 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 :(

> 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.

> 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.

> 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.

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 07.06.2016 um 01:07 schrieb Mark Brown:
> On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
> 
>>> 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 :(
> 
>> 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.
> 
>> 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.
> 
>> 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.

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Michal Suchanek]

On 7 June 2016 at 08:03, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 07.06.2016 um 01:07 schrieb Mark Brown:
>> On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
>>> 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.
>>
>>> 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.

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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 07.06.2016 um 10:10 schrieb Michal Suchanek:
> On 7 June 2016 at 08:03, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Am 07.06.2016 um 01:07 schrieb Mark Brown:
>>> On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
>>>> 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.
>>>
>>>> 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.
> 
> That's obviously bogus. Just reject anything too large and let the
> upper layer deal with it.
> 
So far the upper layer wasn't able to deal with it. Now, with the recent
change set initiated by you, we can clean up the driver a little.
This was also the motivation to involve myself in getting this upper
layer functionality into the mtd code.

> 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?
> 
The fsl-espi controller doesn't support DMA.
The driver copies all transfers to a local buffer.

There's just one nice feature of this controller: It supports a
flash-read-optimized transfer mode, where you can configure:
- first write x bytes
- then read y bytes in either single or dual mode
In tests with a modified driver the dual mode worked quite well.
But that's something for a future extension.

> Thanks
> 
> Michal
> 

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 07.06.2016 um 08:03 schrieb Heiner Kallweit:
> Am 07.06.2016 um 01:07 schrieb Mark Brown:
>> On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
>>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>
>>>> 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 :(
>>
>>> 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.
>>
>>> 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.
>>
>>> 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.
> 
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.

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Michal Suchanek]

On 8 June 2016 at 21:51, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 07.06.2016 um 08:03 schrieb Heiner Kallweit:
>> Am 07.06.2016 um 01:07 schrieb Mark Brown:
>>> On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
>>>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>>
>>>>> 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 :(
>>>
>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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.
>>
> 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

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Heiner Kallweit]

Am 09.06.2016 um 09:12 schrieb Michal Suchanek:
> On 8 June 2016 at 21:51, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> Am 07.06.2016 um 08:03 schrieb Heiner Kallweit:
>>> Am 07.06.2016 um 01:07 schrieb Mark Brown:
>>>> On Mon, Jun 06, 2016 at 08:53:10PM +0200, Heiner Kallweit wrote:
>>>>> Am 06.06.2016 um 19:40 schrieb Mark Brown:
>>>>
>>>>>> 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 :(
>>>>
>>>>> 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.
>>>>
>>>>> 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.
>>>>
>>>>> 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.
>>>
>> 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.
> 
I like this idea. In spi_master we could introduce a bool flag
max_message_size_flag. This would indicate that the max transfer
size is also the max message size. Then we could change
m25p80_read to:

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

max_read_len = spi_max_transfer_size(spi);
if (spi->master->max_message_size_flag)
	max_read_len -= t[0].len;

t[1].rx_buf = buf;
t[1].rx_nbits = m25p80_rx_nbits(nor);
t[1].len = min(len, max_read_len);

> 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
> 

Re: [PATCH 2/2] mtd: m25p80: consider max_transfer_size when reading

[Cyrille Pitchen]

Hi all,

Le 05/04/2016 21:39, Brian Norris a écrit :
> + others
> 
> Hi Heiner,
> 
> On Sat, Feb 27, 2016 at 11:50:11PM +0100, Heiner Kallweit wrote:
>> 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 <hkallweit1@gmail.com>
> 
> 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?
> 
>> ---
>>  drivers/mtd/devices/m25p80.c | 30 +++++++++++++++++++++++++++++-
>>  1 file changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
>> index c2d1f65..69f3acf 100644
>> --- a/drivers/mtd/devices/m25p80.c
>> +++ b/drivers/mtd/devices/m25p80.c
>> @@ -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;
>> +	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.)

I agree with Brian on this point: it would be better to move a generic
algorithm into spi-nor.c which is directly used by some (Q)SPI controller
drivers, hence by-passing the m25p80 driver.

Best regards,

Cyrille