RfC: Handle SPI controller limitations like maximum message length

RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

There have been few discussions in the past about how to handle SPI controller
limitations like max message length. However they don't seem to have resulted
in accepted patches yet.
I also stumbled across this topic because I own a device using Freescale's
ESPI which has a 64K message size limitation.

At least one agreed fact is that silently assembling chunks in protocol
drivers is not the preferred approach.

Maybe a better approach would be to introduce a new member of spi_master
dealing with controller limitations.
My issue is just the message size limitation but most likely there are more
and different limitations in other controllers.

I'd introduce a struct spi_controller_restrictions and add a member to spi_master
pointing to such a struct. Then a controller driver could do something like this:

static const struct spi_controller_restrictions fsl_espi_restrictions = {
	.max_msg_size	= 0xffff,
};

master->restrictions = &fsl_espi_restrictions;

I also add an example how a protocol driver could use this extension.
Appreciate any comment.


diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 269e8af..8a27c66 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -276,6 +276,17 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
 			spi_unregister_driver)
 
 /**
+ * struct spi_controller_restrictions - restrictions of the controller
+ * @max_msg_size: maximum length of a SPI message
+ * SPI controllers can have different restrictions like a maximum
+ * supported message size.
+ * This struct most likely is going to be extended over time.
+ */
+struct spi_controller_restrictions {
+	size_t max_msg_size;
+};
+
+/**
  * struct spi_master - interface to SPI master controller
  * @dev: device interface to this driver
  * @list: link with the global spi_master list
@@ -295,6 +306,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @min_speed_hz: Lowest supported transfer speed
  * @max_speed_hz: Highest supported transfer speed
  * @flags: other constraints relevant to this driver
+ * @restrictions: controller restrictions like max supported message size
  * @bus_lock_spinlock: spinlock for SPI bus locking
  * @bus_lock_mutex: mutex for SPI bus locking
  * @bus_lock_flag: indicates that the SPI bus is locked for exclusive use
@@ -417,6 +429,9 @@ struct spi_master {
 #define SPI_MASTER_MUST_RX      BIT(3)		/* requires rx */
 #define SPI_MASTER_MUST_TX      BIT(4)		/* requires tx */
 
+	/* collection of restrictions like max supported message size */
+	struct spi_controller_restrictions *restrictions;
+
 	/* lock and mutex for SPI bus locking */
 	spinlock_t		bus_lock_spinlock;
 	struct mutex		bus_lock_mutex;
-- 
2.6.2




---
 drivers/mtd/devices/m25p80.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index f10daa8..0e46fb1f 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -115,11 +115,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;
@@ -160,6 +156,41 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	return ret;
 }
 
+/*
+ * 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,
+			size_t *retlen, u_char *buf)
+{
+	struct m25p *flash = nor->priv;
+	struct spi_controller_restrictions *restrictions =
+		flash->spi->master->restrictions;
+	size_t cmd_len, xfer_len, max_len;
+	int ret = 0;
+
+	/* convert the dummy cycles to the number of bytes */
+	cmd_len = m25p_cmdsz(nor) + nor->read_dummy / 8;
+
+	max_len = (restrictions && restrictions->max_msg_size) ?
+		  restrictions->max_msg_size : SIZE_MAX;
+
+	if (unlikely(max_len < cmd_len))
+		return -EINVAL;
+
+	max_len -= cmd_len;
+
+	while (len) {
+		xfer_len = min(len, max_len);
+		ret = _m25p80_read(nor, from, xfer_len, retlen, buf);
+		if (ret < 0)
+			break;
+		from += xfer_len;
+		len -= xfer_len;
+	}
+
+	return ret;
+}
+
 static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 {
 	struct m25p *flash = nor->priv;
-- 
2.6.2


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

There have been few discussions in the past about how to handle SPI controller
limitations like max message length. However they don't seem to have resulted
in accepted patches yet.
I also stumbled across this topic because I own a device using Freescale's
ESPI which has a 64K message size limitation.

At least one agreed fact is that silently assembling chunks in protocol
drivers is not the preferred approach.

Maybe a better approach would be to introduce a new member of spi_master
dealing with controller limitations.
My issue is just the message size limitation but most likely there are more
and different limitations in other controllers.

I'd introduce a struct spi_controller_restrictions and add a member to spi_master
pointing to such a struct. Then a controller driver could do something like this:

static const struct spi_controller_restrictions fsl_espi_restrictions = {
	.max_msg_size	= 0xffff,
};

master->restrictions = &fsl_espi_restrictions;

I also add an example how a protocol driver could use this extension.
Appreciate any comment.


diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 269e8af..8a27c66 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -276,6 +276,17 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
 			spi_unregister_driver)
 
 /**
+ * struct spi_controller_restrictions - restrictions of the controller
+ * @max_msg_size: maximum length of a SPI message
+ * SPI controllers can have different restrictions like a maximum
+ * supported message size.
+ * This struct most likely is going to be extended over time.
+ */
+struct spi_controller_restrictions {
+	size_t max_msg_size;
+};
+
+/**
  * struct spi_master - interface to SPI master controller
  * @dev: device interface to this driver
  * @list: link with the global spi_master list
@@ -295,6 +306,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @min_speed_hz: Lowest supported transfer speed
  * @max_speed_hz: Highest supported transfer speed
  * @flags: other constraints relevant to this driver
+ * @restrictions: controller restrictions like max supported message size
  * @bus_lock_spinlock: spinlock for SPI bus locking
  * @bus_lock_mutex: mutex for SPI bus locking
  * @bus_lock_flag: indicates that the SPI bus is locked for exclusive use
@@ -417,6 +429,9 @@ struct spi_master {
 #define SPI_MASTER_MUST_RX      BIT(3)		/* requires rx */
 #define SPI_MASTER_MUST_TX      BIT(4)		/* requires tx */
 
+	/* collection of restrictions like max supported message size */
+	struct spi_controller_restrictions *restrictions;
+
 	/* lock and mutex for SPI bus locking */
 	spinlock_t		bus_lock_spinlock;
 	struct mutex		bus_lock_mutex;
-- 
2.6.2




---
 drivers/mtd/devices/m25p80.c | 41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index f10daa8..0e46fb1f 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -115,11 +115,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;
@@ -160,6 +156,41 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	return ret;
 }
 
+/*
+ * 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,
+			size_t *retlen, u_char *buf)
+{
+	struct m25p *flash = nor->priv;
+	struct spi_controller_restrictions *restrictions =
+		flash->spi->master->restrictions;
+	size_t cmd_len, xfer_len, max_len;
+	int ret = 0;
+
+	/* convert the dummy cycles to the number of bytes */
+	cmd_len = m25p_cmdsz(nor) + nor->read_dummy / 8;
+
+	max_len = (restrictions && restrictions->max_msg_size) ?
+		  restrictions->max_msg_size : SIZE_MAX;
+
+	if (unlikely(max_len < cmd_len))
+		return -EINVAL;
+
+	max_len -= cmd_len;
+
+	while (len) {
+		xfer_len = min(len, max_len);
+		ret = _m25p80_read(nor, from, xfer_len, retlen, buf);
+		if (ret < 0)
+			break;
+		from += xfer_len;
+		len -= xfer_len;
+	}
+
+	return ret;
+}
+
 static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 {
 	struct m25p *flash = nor->priv;
-- 
2.6.2

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
> There have been few discussions in the past about how to handle SPI controller
> limitations like max message length. However they don't seem to have resulted
> in accepted patches yet.

No, they've resulted in people writing code.  We've got a bunch of
things in the spi_master struct like the limits on speeds and bits per
word.

> Maybe a better approach would be to introduce a new member of spi_master
> dealing with controller limitations.

That's what we've been doing...

> I also add an example how a protocol driver could use this extension.
> Appreciate any comment.
> 
> 
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 269e8af..8a27c66 100644

Please don't send patches without a signoff, see SubmittingPatches.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
> There have been few discussions in the past about how to handle SPI controller
> limitations like max message length. However they don't seem to have resulted
> in accepted patches yet.

No, they've resulted in people writing code.  We've got a bunch of
things in the spi_master struct like the limits on speeds and bits per
word.

> Maybe a better approach would be to introduce a new member of spi_master
> dealing with controller limitations.

That's what we've been doing...

> I also add an example how a protocol driver could use this extension.
> Appreciate any comment.
> 
> 
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index 269e8af..8a27c66 100644

Please don't send patches without a signoff, see SubmittingPatches.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 18.11.2015 um 22:57 schrieb Mark Brown:
> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
>> There have been few discussions in the past about how to handle SPI controller
>> limitations like max message length. However they don't seem to have resulted
>> in accepted patches yet.
> 
> No, they've resulted in people writing code.  We've got a bunch of
> things in the spi_master struct like the limits on speeds and bits per
> word.
> 
Sure, I'm aware of this. To you it might sound obvious to handle such
limitations in the SPI core, however there have been several attempts
to deal with such limitations in controller or protocol drivers.

>> Maybe a better approach would be to introduce a new member of spi_master
>> dealing with controller limitations.
> 
> That's what we've been doing...
> 
Primary addressees of my comment were users of the SPI core trying to deal
in their own code with things which might be better dealt with in the core.
Again, for you it's obvious ..
Just one more comment:
In case there should be more need to reflect such limitations in spi_master
it might be good to encapsulate them in a separate struct instead of adding
more such members to spi_master directly.

>> I also add an example how a protocol driver could use this extension.
>> Appreciate any comment.
>>
>>
>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>> index 269e8af..8a27c66 100644
> 
> Please don't send patches without a signoff, see SubmittingPatches.
> 
It wasn't meant to be actual patches, therefore I omitted some stuff
and didn't use [PATCH] in the subject.
It was meant just to be code snippets providing a little more detail.

Thanks for the comment anyway.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 18.11.2015 um 22:57 schrieb Mark Brown:
> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
>> There have been few discussions in the past about how to handle SPI controller
>> limitations like max message length. However they don't seem to have resulted
>> in accepted patches yet.
> 
> No, they've resulted in people writing code.  We've got a bunch of
> things in the spi_master struct like the limits on speeds and bits per
> word.
> 
Sure, I'm aware of this. To you it might sound obvious to handle such
limitations in the SPI core, however there have been several attempts
to deal with such limitations in controller or protocol drivers.

>> Maybe a better approach would be to introduce a new member of spi_master
>> dealing with controller limitations.
> 
> That's what we've been doing...
> 
Primary addressees of my comment were users of the SPI core trying to deal
in their own code with things which might be better dealt with in the core.
Again, for you it's obvious ..
Just one more comment:
In case there should be more need to reflect such limitations in spi_master
it might be good to encapsulate them in a separate struct instead of adding
more such members to spi_master directly.

>> I also add an example how a protocol driver could use this extension.
>> Appreciate any comment.
>>
>>
>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>> index 269e8af..8a27c66 100644
> 
> Please don't send patches without a signoff, see SubmittingPatches.
> 
It wasn't meant to be actual patches, therefore I omitted some stuff
and didn't use [PATCH] in the subject.
It was meant just to be code snippets providing a little more detail.

Thanks for the comment anyway.

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Wed, Nov 18, 2015 at 11:50:00PM +0100, Heiner Kallweit wrote:
> Am 18.11.2015 um 22:57 schrieb Mark Brown:
> > On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:

> >> There have been few discussions in the past about how to handle SPI controller
> >> limitations like max message length. However they don't seem to have resulted
> >> in accepted patches yet.

> > No, they've resulted in people writing code.  We've got a bunch of
> > things in the spi_master struct like the limits on speeds and bits per
> > word.

> Sure, I'm aware of this. To you it might sound obvious to handle such
> limitations in the SPI core, however there have been several attempts
> to deal with such limitations in controller or protocol drivers.

They're documented using terms like "limits" and "constraints" - it's
hard to see what we're missing there.

> >> Maybe a better approach would be to introduce a new member of spi_master
> >> dealing with controller limitations.

> > That's what we've been doing...

> Primary addressees of my comment were users of the SPI core trying to deal
> in their own code with things which might be better dealt with in the core.

Well, we do to the extent we can do anything useful in the core we have
code to deal with them - we constrain clocks and we have error checking
for the bits per word settings for example.

> In case there should be more need to reflect such limitations in spi_master
> it might be good to encapsulate them in a separate struct instead of adding
> more such members to spi_master directly.

It's going to be annoying to move everything over and TBH I'm not sure
it really helps much.  This is honestly the first time I can recall
anyone expressing any confusion here.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Wed, Nov 18, 2015 at 11:50:00PM +0100, Heiner Kallweit wrote:
> Am 18.11.2015 um 22:57 schrieb Mark Brown:
> > On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:

> >> There have been few discussions in the past about how to handle SPI controller
> >> limitations like max message length. However they don't seem to have resulted
> >> in accepted patches yet.

> > No, they've resulted in people writing code.  We've got a bunch of
> > things in the spi_master struct like the limits on speeds and bits per
> > word.

> Sure, I'm aware of this. To you it might sound obvious to handle such
> limitations in the SPI core, however there have been several attempts
> to deal with such limitations in controller or protocol drivers.

They're documented using terms like "limits" and "constraints" - it's
hard to see what we're missing there.

> >> Maybe a better approach would be to introduce a new member of spi_master
> >> dealing with controller limitations.

> > That's what we've been doing...

> Primary addressees of my comment were users of the SPI core trying to deal
> in their own code with things which might be better dealt with in the core.

Well, we do to the extent we can do anything useful in the core we have
code to deal with them - we constrain clocks and we have error checking
for the bits per word settings for example.

> In case there should be more need to reflect such limitations in spi_master
> it might be good to encapsulate them in a separate struct instead of adding
> more such members to spi_master directly.

It's going to be annoying to move everything over and TBH I'm not sure
it really helps much.  This is honestly the first time I can recall
anyone expressing any confusion here.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 19.11.2015, at 12:40, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Wed, Nov 18, 2015 at 11:50:00PM +0100, Heiner Kallweit wrote:
>> Am 18.11.2015 um 22:57 schrieb Mark Brown:
>>> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
> 
>>>> There have been few discussions in the past about how to handle SPI controller
>>>> limitations like max message length. However they don't seem to have resulted
>>>> in accepted patches yet.
> 
>>> No, they've resulted in people writing code.  We've got a bunch of
>>> things in the spi_master struct like the limits on speeds and bits per
>>> word.
> 
>> Sure, I'm aware of this. To you it might sound obvious to handle such
>> limitations in the SPI core, however there have been several attempts
>> to deal with such limitations in controller or protocol drivers.
> 
> They're documented using terms like "limits" and "constraints" - it's
> hard to see what we're missing there.
> 
>>>> Maybe a better approach would be to introduce a new member of spi_master
>>>> dealing with controller limitations.
> 
>>> That's what we've been doing...
> 
>> Primary addressees of my comment were users of the SPI core trying to deal
>> in their own code with things which might be better dealt with in the core.
> 
> Well, we do to the extent we can do anything useful in the core we have
> code to deal with them - we constrain clocks and we have error checking
> for the bits per word settings for example.
> 
>> In case there should be more need to reflect such limitations in spi_master
>> it might be good to encapsulate them in a separate struct instead of adding
>> more such members to spi_master directly.
> 
> It's going to be annoying to move everything over and TBH I'm not sure
> it really helps much.  This is honestly the first time I can recall
> anyone expressing any confusion here.

On the bcm2835 there are also some “limitations” (when transfers are not aligned
to word, transfers>65535 can’t DMA) which we work around right now inefficiently.

I am in the progress of writing a (minimal) spi-core extension, that would allow 
a spi_master to have a prepare_message function that would call some “message
translation functions”.

The ones I have currently come up with are:
int spi_split_transfer_alignment(struct spi_message *msg, int alignment);
int spi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);

These would make the spi message conform to the drivers requirements for
alignment and max_size splitting transfers in the appropriate way before they
are are dma-mapped.

I guess this is a more transparent approach that does not require the
individual device drivers to query the spi_master about limitations
and replicate code in several drivers - also there is no maintenance cost on
individual device drivers to track when there is a new limitation introduced.

This may not handle every possible case/limitation, but could help in some cases.

Thanks,
	Martin--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 19.11.2015, at 12:40, Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, Nov 18, 2015 at 11:50:00PM +0100, Heiner Kallweit wrote:
>> Am 18.11.2015 um 22:57 schrieb Mark Brown:
>>> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
> 
>>>> There have been few discussions in the past about how to handle SPI controller
>>>> limitations like max message length. However they don't seem to have resulted
>>>> in accepted patches yet.
> 
>>> No, they've resulted in people writing code.  We've got a bunch of
>>> things in the spi_master struct like the limits on speeds and bits per
>>> word.
> 
>> Sure, I'm aware of this. To you it might sound obvious to handle such
>> limitations in the SPI core, however there have been several attempts
>> to deal with such limitations in controller or protocol drivers.
> 
> They're documented using terms like "limits" and "constraints" - it's
> hard to see what we're missing there.
> 
>>>> Maybe a better approach would be to introduce a new member of spi_master
>>>> dealing with controller limitations.
> 
>>> That's what we've been doing...
> 
>> Primary addressees of my comment were users of the SPI core trying to deal
>> in their own code with things which might be better dealt with in the core.
> 
> Well, we do to the extent we can do anything useful in the core we have
> code to deal with them - we constrain clocks and we have error checking
> for the bits per word settings for example.
> 
>> In case there should be more need to reflect such limitations in spi_master
>> it might be good to encapsulate them in a separate struct instead of adding
>> more such members to spi_master directly.
> 
> It's going to be annoying to move everything over and TBH I'm not sure
> it really helps much.  This is honestly the first time I can recall
> anyone expressing any confusion here.

On the bcm2835 there are also some “limitations” (when transfers are not aligned
to word, transfers>65535 can’t DMA) which we work around right now inefficiently.

I am in the progress of writing a (minimal) spi-core extension, that would allow 
a spi_master to have a prepare_message function that would call some “message
translation functions”.

The ones I have currently come up with are:
int spi_split_transfer_alignment(struct spi_message *msg, int alignment);
int spi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);

These would make the spi message conform to the drivers requirements for
alignment and max_size splitting transfers in the appropriate way before they
are are dma-mapped.

I guess this is a more transparent approach that does not require the
individual device drivers to query the spi_master about limitations
and replicate code in several drivers - also there is no maintenance cost on
individual device drivers to track when there is a new limitation introduced.

This may not handle every possible case/limitation, but could help in some cases.

Thanks,
	Martin

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Thu, Nov 19, 2015 at 04:00:45PM +0100, Martin Sperl 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.

> On the bcm2835 there are also some “limitations” (when transfers are not aligned
> to word, transfers>65535 can’t DMA) which we work around right now inefficiently.

Alignment is a general issue which all clients should be trying to
ensure as a matter of course - never mind individual blocks of hardware,
some common CPU architectures suffer noticable penalties from unaligned
accesses so it's just generally good practice to try to avoid them.

You shouldn't be doing anything about transfer size limitations in your
driver, if you have this restriction you should be adding code to the
core and just flagging the limit in your driver.

> I am in the progress of writing a (minimal) spi-core extension, that would allow 
> a spi_master to have a prepare_message function that would call some “message
> translation functions”.

> The ones I have currently come up with are:
> int spi_split_transfer_alignment(struct spi_message *msg, int alignment);
> int spi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);

Please don't do things this way, please make this more data driven -
have the drivers declare their capabilities so the core can just do
these things based on those flags rather than requiring active code in
drivers.  This keeps the code central which in turn helps keep things
maintainable.

> I guess this is a more transparent approach that does not require the
> individual device drivers to query the spi_master about limitations
> and replicate code in several drivers - also there is no maintenance cost on
> individual device drivers to track when there is a new limitation introduced.

You've got this back to front - drivers are responsible for initialising
the master when they instantiate it.  There's nothing stopping them
using the data if they have variants to handle but they shouldn't be
speculatively looking at it on the off chance there's some limit.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Thu, Nov 19, 2015 at 04:00:45PM +0100, Martin Sperl 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.

> On the bcm2835 there are also some “limitations” (when transfers are not aligned
> to word, transfers>65535 can’t DMA) which we work around right now inefficiently.

Alignment is a general issue which all clients should be trying to
ensure as a matter of course - never mind individual blocks of hardware,
some common CPU architectures suffer noticable penalties from unaligned
accesses so it's just generally good practice to try to avoid them.

You shouldn't be doing anything about transfer size limitations in your
driver, if you have this restriction you should be adding code to the
core and just flagging the limit in your driver.

> I am in the progress of writing a (minimal) spi-core extension, that would allow 
> a spi_master to have a prepare_message function that would call some “message
> translation functions”.

> The ones I have currently come up with are:
> int spi_split_transfer_alignment(struct spi_message *msg, int alignment);
> int spi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);

Please don't do things this way, please make this more data driven -
have the drivers declare their capabilities so the core can just do
these things based on those flags rather than requiring active code in
drivers.  This keeps the code central which in turn helps keep things
maintainable.

> I guess this is a more transparent approach that does not require the
> individual device drivers to query the spi_master about limitations
> and replicate code in several drivers - also there is no maintenance cost on
> individual device drivers to track when there is a new limitation introduced.

You've got this back to front - drivers are responsible for initialising
the master when they instantiate it.  There's nothing stopping them
using the data if they have variants to handle but they shouldn't be
speculatively looking at it on the off chance there's some limit.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

On Thu, Nov 19, 2015 at 05:15:38PM +0000, Mark Brown wrote:
> On Thu, Nov 19, 2015 at 04:00:45PM +0100, Martin Sperl wrote:
> > On the bcm2835 there are also some “limitations” (when transfers are not aligned
> > to word, transfers>65535 can’t DMA) which we work around right now inefficiently.
> 
> Alignment is a general issue which all clients should be trying to
> ensure as a matter of course - never mind individual blocks of hardware,
> some common CPU architectures suffer noticable penalties from unaligned
> accesses so it's just generally good practice to try to avoid them.
> 
> You shouldn't be doing anything about transfer size limitations in your
> driver, if you have this restriction you should be adding code to the
> core and just flagging the limit in your driver.

So for transfer size limitations, are you speaking of the same thing as
Heiner (who began this post mentioning *message* size limitations)? I
read a difference between the two, where a transfer size limitation
might mean that one could improve the SPI core to just split transfers
up into multiple sub-transfers, and still complete the whole
spi_message (and therefore the protocol driver has less to worry about).
But if we're talking about spi_message limitations, then this would be
more exposed to the protocol driver.

Or maybe I'm just reading this all wrong and am confused. Please
enlighten.

Regards,
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

On Thu, Nov 19, 2015 at 05:15:38PM +0000, Mark Brown wrote:
> On Thu, Nov 19, 2015 at 04:00:45PM +0100, Martin Sperl wrote:
> > On the bcm2835 there are also some “limitations” (when transfers are not aligned
> > to word, transfers>65535 can’t DMA) which we work around right now inefficiently.
> 
> Alignment is a general issue which all clients should be trying to
> ensure as a matter of course - never mind individual blocks of hardware,
> some common CPU architectures suffer noticable penalties from unaligned
> accesses so it's just generally good practice to try to avoid them.
> 
> You shouldn't be doing anything about transfer size limitations in your
> driver, if you have this restriction you should be adding code to the
> core and just flagging the limit in your driver.

So for transfer size limitations, are you speaking of the same thing as
Heiner (who began this post mentioning *message* size limitations)? I
read a difference between the two, where a transfer size limitation
might mean that one could improve the SPI core to just split transfers
up into multiple sub-transfers, and still complete the whole
spi_message (and therefore the protocol driver has less to worry about).
But if we're talking about spi_message limitations, then this would be
more exposed to the protocol driver.

Or maybe I'm just reading this all wrong and am confused. Please
enlighten.

Regards,
Brian

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Thu, Nov 19, 2015 at 04:07:46PM -0800, Brian Norris wrote:

> So for transfer size limitations, are you speaking of the same thing as
> Heiner (who began this post mentioning *message* size limitations)? I
> read a difference between the two, where a transfer size limitation
> might mean that one could improve the SPI core to just split transfers
> up into multiple sub-transfers, and still complete the whole
> spi_message (and therefore the protocol driver has less to worry about).
> But if we're talking about spi_message limitations, then this would be
> more exposed to the protocol driver.

For almost all hardware these should be the same things - most drivers
shouldn't be working in terms of messages, they should be working in
terms of transferrs.  Anything that has to work at full message level
almost certainly has substantial other limitations in place given the
need to be able to change parameters arbatrarily in the middle of
messages.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Thu, Nov 19, 2015 at 04:07:46PM -0800, Brian Norris wrote:

> So for transfer size limitations, are you speaking of the same thing as
> Heiner (who began this post mentioning *message* size limitations)? I
> read a difference between the two, where a transfer size limitation
> might mean that one could improve the SPI core to just split transfers
> up into multiple sub-transfers, and still complete the whole
> spi_message (and therefore the protocol driver has less to worry about).
> But if we're talking about spi_message limitations, then this would be
> more exposed to the protocol driver.

For almost all hardware these should be the same things - most drivers
shouldn't be working in terms of messages, they should be working in
terms of transferrs.  Anything that has to work at full message level
almost certainly has substantial other limitations in place given the
need to be able to change parameters arbatrarily in the middle of
messages.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 20.11.2015, at 12:06, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Thu, Nov 19, 2015 at 04:07:46PM -0800, Brian Norris wrote:
> 
>> So for transfer size limitations, are you speaking of the same thing as
>> Heiner (who began this post mentioning *message* size limitations)? I
>> read a difference between the two, where a transfer size limitation
>> might mean that one could improve the SPI core to just split transfers
>> up into multiple sub-transfers, and still complete the whole
>> spi_message (and therefore the protocol driver has less to worry about).
>> But if we're talking about spi_message limitations, then this would be
>> more exposed to the protocol driver.
> 
> For almost all hardware these should be the same things - most drivers
> shouldn't be working in terms of messages, they should be working in
> terms of transferrs.  Anything that has to work at full message level
> almost certainly has substantial other limitations in place given the
> need to be able to change parameters arbatrarily in the middle of
> messages.

I will try to get a prototype out soon, so that we can talk actual code,
but for now I will leave out the automated core logic for transformations.

Martin
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 20.11.2015, at 12:06, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Nov 19, 2015 at 04:07:46PM -0800, Brian Norris wrote:
> 
>> So for transfer size limitations, are you speaking of the same thing as
>> Heiner (who began this post mentioning *message* size limitations)? I
>> read a difference between the two, where a transfer size limitation
>> might mean that one could improve the SPI core to just split transfers
>> up into multiple sub-transfers, and still complete the whole
>> spi_message (and therefore the protocol driver has less to worry about).
>> But if we're talking about spi_message limitations, then this would be
>> more exposed to the protocol driver.
> 
> For almost all hardware these should be the same things - most drivers
> shouldn't be working in terms of messages, they should be working in
> terms of transferrs.  Anything that has to work at full message level
> almost certainly has substantial other limitations in place given the
> need to be able to change parameters arbatrarily in the middle of
> messages.

I will try to get a prototype out soon, so that we can talk actual code,
but for now I will leave out the automated core logic for transformations.

Martin

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 19.11.2015, at 18:15, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>> I am in the progress of writing a (minimal) spi-core extension, that would allow 
>> a spi_master to have a prepare_message function that would call some “message
>> translation functions”.
> 
>> The ones I have currently come up with are:
>> int spi_split_transfer_alignment(struct spi_message *msg, int alignment);
>> int spi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);
> 
> Please don't do things this way, please make this more data driven -
> have the drivers declare their capabilities so the core can just do
> these things based on those flags rather than requiring active code in
> drivers.  This keeps the code central which in turn helps keep things
> maintainable.
> 

Well - I was thinking about starting with this approach for simplicity.

Making it automatic/data-driven by just defining limits that the core
can then handle transparently is something that could come as a next
step.

Also the bus master driver knows what its limitations are, so putting
them inside prepare_message seems reasonable to me - that is unless
you really have spi_device drivers that would handle those as separate
facts and not refuse to work.


So I wonder how much difference it makes:
int driver_prepare_message (struct spi_master *master, 
                            struct spi_message *msg)
{
  int ret;

  if (ret = spi_split_transfer_alignment(msg, 4))
	ret ret;
  if (ret = spi_split_transfer_maxsize(msg, 65534))
	ret ret
  return 0;
}

driver_probe() {
  ...
  master->prepare_message = driver_prepare_message;
  ...
}

or:
driver_probe() {
  ...
  master->transfer_alignment = 4;
  master->max_transfer_size = 65534; 
  ...
}

both work, but parametrizing may become a pain when more
limitations start to get created.

In my example when looking at the situation where we only care for
alignment when the transfer is valid for DMA (PIO does not care).

so you would need to add something like:
  master->transfer_alignment_min_size = 96;

to handle such “extra” requirements for the spi_master.
(Sometimes ordering of “rules” may be important as well)

So the function inside the core we could then look like this:

int spi_core_prepare_message (struct spi_master *master, 
                              struct spi_message *msg)
{
  int ret;

  if (master->transfer_alignment) {
    ret = spi_split_transfer_alignment(msg,
				       master->transfer_alignment,
				       master->transfer_alignment_min_size);
    if (ret)
      ret ret;
  }

  if (master->transfer_alignment) {
    ret = spi_split_transfer_maxsize(msg,
				     master->max_transfer_size);
    if (ret)
      ret ret;
  }
  return 0
}

Something like the above I envision as a second step,
but first start to make it work...

>> I guess this is a more transparent approach that does not require the
>> individual device drivers to query the spi_master about limitations
>> and replicate code in several drivers - also there is no maintenance cost on
>> individual device drivers to track when there is a new limitation introduced.
> 
> You've got this back to front - drivers are responsible for initialising
> the master when they instantiate it.  There's nothing stopping them
> using the data if they have variants to handle but they shouldn't be
> speculatively looking at it on the off chance there's some limit.

I wonder if such a variant-construct does not create more headaches in
the long run as each spi_driver wanting to do something “something”
special would then need to get updated for any additional constraint…

It seems as if we would need then to add 2 kinds of constraints:
* ones that may be of interest to spi_device to avoid unfavourable 
  situations created by the core implementation (max_transfer_sizes)
* ones that are handled by spi-core
  (like the alignment constraints)


Martin




--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 19.11.2015, at 18:15, Mark Brown <broonie@kernel.org> wrote:
> 
>> I am in the progress of writing a (minimal) spi-core extension, that would allow 
>> a spi_master to have a prepare_message function that would call some “message
>> translation functions”.
> 
>> The ones I have currently come up with are:
>> int spi_split_transfer_alignment(struct spi_message *msg, int alignment);
>> int spi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);
> 
> Please don't do things this way, please make this more data driven -
> have the drivers declare their capabilities so the core can just do
> these things based on those flags rather than requiring active code in
> drivers.  This keeps the code central which in turn helps keep things
> maintainable.
> 

Well - I was thinking about starting with this approach for simplicity.

Making it automatic/data-driven by just defining limits that the core
can then handle transparently is something that could come as a next
step.

Also the bus master driver knows what its limitations are, so putting
them inside prepare_message seems reasonable to me - that is unless
you really have spi_device drivers that would handle those as separate
facts and not refuse to work.


So I wonder how much difference it makes:
int driver_prepare_message (struct spi_master *master, 
                            struct spi_message *msg)
{
  int ret;

  if (ret = spi_split_transfer_alignment(msg, 4))
	ret ret;
  if (ret = spi_split_transfer_maxsize(msg, 65534))
	ret ret
  return 0;
}

driver_probe() {
  ...
  master->prepare_message = driver_prepare_message;
  ...
}

or:
driver_probe() {
  ...
  master->transfer_alignment = 4;
  master->max_transfer_size = 65534; 
  ...
}

both work, but parametrizing may become a pain when more
limitations start to get created.

In my example when looking at the situation where we only care for
alignment when the transfer is valid for DMA (PIO does not care).

so you would need to add something like:
  master->transfer_alignment_min_size = 96;

to handle such “extra” requirements for the spi_master.
(Sometimes ordering of “rules” may be important as well)

So the function inside the core we could then look like this:

int spi_core_prepare_message (struct spi_master *master, 
                              struct spi_message *msg)
{
  int ret;

  if (master->transfer_alignment) {
    ret = spi_split_transfer_alignment(msg,
				       master->transfer_alignment,
				       master->transfer_alignment_min_size);
    if (ret)
      ret ret;
  }

  if (master->transfer_alignment) {
    ret = spi_split_transfer_maxsize(msg,
				     master->max_transfer_size);
    if (ret)
      ret ret;
  }
  return 0
}

Something like the above I envision as a second step,
but first start to make it work...

>> I guess this is a more transparent approach that does not require the
>> individual device drivers to query the spi_master about limitations
>> and replicate code in several drivers - also there is no maintenance cost on
>> individual device drivers to track when there is a new limitation introduced.
> 
> You've got this back to front - drivers are responsible for initialising
> the master when they instantiate it.  There's nothing stopping them
> using the data if they have variants to handle but they shouldn't be
> speculatively looking at it on the off chance there's some limit.

I wonder if such a variant-construct does not create more headaches in
the long run as each spi_driver wanting to do something “something”
special would then need to get updated for any additional constraint…

It seems as if we would need then to add 2 kinds of constraints:
* ones that may be of interest to spi_device to avoid unfavourable 
  situations created by the core implementation (max_transfer_sizes)
* ones that are handled by spi-core
  (like the alignment constraints)


Martin

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 11:18:42AM +0100, Martin Sperl wrote:
> > On 19.11.2015, at 18:15, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > Please don't do things this way, please make this more data driven -
> > have the drivers declare their capabilities so the core can just do
> > these things based on those flags rather than requiring active code in
> > drivers.  This keeps the code central which in turn helps keep things
> > maintainable.

> Well - I was thinking about starting with this approach for simplicity.

> Making it automatic/data-driven by just defining limits that the core
> can then handle transparently is something that could come as a next
> step.

It makes things more complicated in the long run, people start open
coding things and then making any changes involves changing per-driver
code and we can't use the information in any way other than the one
specific way the transformation functions were written.

> Also the bus master driver knows what its limitations are, so putting
> them inside prepare_message seems reasonable to me - that is unless
> you really have spi_device drivers that would handle those as separate
> facts and not refuse to work.

Every line of code that's in a driver that could be in the core is a
maintainence burden, people will want to start doing things like
combining functions in fun ways and if we want to try to do things like
figure out if we can coalesce adjacent transfers in the core (which we
really ought to) it won't know what the limitations that exist are.

> > You've got this back to front - drivers are responsible for initialising
> > the master when they instantiate it.  There's nothing stopping them
> > using the data if they have variants to handle but they shouldn't be
> > speculatively looking at it on the off chance there's some limit.

> I wonder if such a variant-construct does not create more headaches in
> the long run as each spi_driver wanting to do something “something”
> special would then need to get updated for any additional constraint…

I'm sorry but I don't understand what you mean here.  However we
implement things anything that wants to know about constraints is going
to need to be updated to use them.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 11:18:42AM +0100, Martin Sperl wrote:
> > On 19.11.2015, at 18:15, Mark Brown <broonie@kernel.org> wrote:

> > Please don't do things this way, please make this more data driven -
> > have the drivers declare their capabilities so the core can just do
> > these things based on those flags rather than requiring active code in
> > drivers.  This keeps the code central which in turn helps keep things
> > maintainable.

> Well - I was thinking about starting with this approach for simplicity.

> Making it automatic/data-driven by just defining limits that the core
> can then handle transparently is something that could come as a next
> step.

It makes things more complicated in the long run, people start open
coding things and then making any changes involves changing per-driver
code and we can't use the information in any way other than the one
specific way the transformation functions were written.

> Also the bus master driver knows what its limitations are, so putting
> them inside prepare_message seems reasonable to me - that is unless
> you really have spi_device drivers that would handle those as separate
> facts and not refuse to work.

Every line of code that's in a driver that could be in the core is a
maintainence burden, people will want to start doing things like
combining functions in fun ways and if we want to try to do things like
figure out if we can coalesce adjacent transfers in the core (which we
really ought to) it won't know what the limitations that exist are.

> > You've got this back to front - drivers are responsible for initialising
> > the master when they instantiate it.  There's nothing stopping them
> > using the data if they have variants to handle but they shouldn't be
> > speculatively looking at it on the off chance there's some limit.

> I wonder if such a variant-construct does not create more headaches in
> the long run as each spi_driver wanting to do something “something”
> special would then need to get updated for any additional constraint…

I'm sorry but I don't understand what you mean here.  However we
implement things anything that wants to know about constraints is going
to need to be updated to use them.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 20.11.2015, at 13:05, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Fri, Nov 20, 2015 at 11:18:42AM +0100, Martin Sperl wrote:
>>> On 19.11.2015, at 18:15, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
>>> Please don't do things this way, please make this more data driven -
>>> have the drivers declare their capabilities so the core can just do
>>> these things based on those flags rather than requiring active code in
>>> drivers.  This keeps the code central which in turn helps keep things
>>> maintainable.
> 
>> Well - I was thinking about starting with this approach for simplicity.
> 
>> Making it automatic/data-driven by just defining limits that the core
>> can then handle transparently is something that could come as a next
>> step.
> 
> It makes things more complicated in the long run, people start open
> coding things and then making any changes involves changing per-driver
> code and we can't use the information in any way other than the one
> specific way the transformation functions were written.

As said: lets see if it works and passes some logical tests
and then address those things we may want to define on top.

Still there may be different policies that we would need to run for
different spi masters when the transfers are not aligned,
so I wonder if it is really sane to parametrize all those in the
spi_master structure.

>> Also the bus master driver knows what its limitations are, so putting
>> them inside prepare_message seems reasonable to me - that is unless
>> you really have spi_device drivers that would handle those as separate
>> facts and not refuse to work.
> 
> Every line of code that's in a driver that could be in the core is a
> maintainence burden, people will want to start doing things like
> combining functions in fun ways and if we want to try to do things like
> figure out if we can coalesce adjacent transfers in the core (which we
> really ought to) it won't know what the limitations that exist are.
this “colaesce” of transfers into one could be one of those 
“transformation” I am talking about - and this one would be implemented
in core making certain assumptions (like starting on a page, ...)

The way I think of it it is actually very simple to implement that.

>>> You've got this back to front - drivers are responsible for initialising
>>> the master when they instantiate it.  There's nothing stopping them
>>> using the data if they have variants to handle but they shouldn't be
>>> speculatively looking at it on the off chance there's some limit.
> 
>> I wonder if such a variant-construct does not create more headaches in
>> the long run as each spi_driver wanting to do something “something”
>> special would then need to get updated for any additional constraint…
> 
> I'm sorry but I don't understand what you mean here.  However we
> implement things anything that wants to know about constraints is going
> to need to be updated to use them.

That is what I want to avoid if possible - the one thing that may come
handy (at least from my perspective) could be to give some hints to
make optimal use of the HW
Say:
* preferred alignment on X byte boundry
* preferred max spi_transfer.length

All the other possible constraints parameters should be opaque to 
a spi_device driver, so I would not want to include those inside the
spi_master structure, as _then_ these get used.

That is why I thought of putting those “policies” inside
prepare_message on the driver side.

Also the “restrictions” on the spi HW need to get defined inside the
driver, so there will not be a new “restriction” that applies to
an existing piece of HW just because we incorporate new options.



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Martin Sperl]

> On 20.11.2015, at 13:05, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Nov 20, 2015 at 11:18:42AM +0100, Martin Sperl wrote:
>>> On 19.11.2015, at 18:15, Mark Brown <broonie@kernel.org> wrote:
> 
>>> Please don't do things this way, please make this more data driven -
>>> have the drivers declare their capabilities so the core can just do
>>> these things based on those flags rather than requiring active code in
>>> drivers.  This keeps the code central which in turn helps keep things
>>> maintainable.
> 
>> Well - I was thinking about starting with this approach for simplicity.
> 
>> Making it automatic/data-driven by just defining limits that the core
>> can then handle transparently is something that could come as a next
>> step.
> 
> It makes things more complicated in the long run, people start open
> coding things and then making any changes involves changing per-driver
> code and we can't use the information in any way other than the one
> specific way the transformation functions were written.

As said: lets see if it works and passes some logical tests
and then address those things we may want to define on top.

Still there may be different policies that we would need to run for
different spi masters when the transfers are not aligned,
so I wonder if it is really sane to parametrize all those in the
spi_master structure.

>> Also the bus master driver knows what its limitations are, so putting
>> them inside prepare_message seems reasonable to me - that is unless
>> you really have spi_device drivers that would handle those as separate
>> facts and not refuse to work.
> 
> Every line of code that's in a driver that could be in the core is a
> maintainence burden, people will want to start doing things like
> combining functions in fun ways and if we want to try to do things like
> figure out if we can coalesce adjacent transfers in the core (which we
> really ought to) it won't know what the limitations that exist are.
this “colaesce” of transfers into one could be one of those 
“transformation” I am talking about - and this one would be implemented
in core making certain assumptions (like starting on a page, ...)

The way I think of it it is actually very simple to implement that.

>>> You've got this back to front - drivers are responsible for initialising
>>> the master when they instantiate it.  There's nothing stopping them
>>> using the data if they have variants to handle but they shouldn't be
>>> speculatively looking at it on the off chance there's some limit.
> 
>> I wonder if such a variant-construct does not create more headaches in
>> the long run as each spi_driver wanting to do something “something”
>> special would then need to get updated for any additional constraint…
> 
> I'm sorry but I don't understand what you mean here.  However we
> implement things anything that wants to know about constraints is going
> to need to be updated to use them.

That is what I want to avoid if possible - the one thing that may come
handy (at least from my perspective) could be to give some hints to
make optimal use of the HW
Say:
* preferred alignment on X byte boundry
* preferred max spi_transfer.length

All the other possible constraints parameters should be opaque to 
a spi_device driver, so I would not want to include those inside the
spi_master structure, as _then_ these get used.

That is why I thought of putting those “policies” inside
prepare_message on the driver side.

Also the “restrictions” on the spi HW need to get defined inside the
driver, so there will not be a new “restriction” that applies to
an existing piece of HW just because we incorporate new options.

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 01:56:13PM +0100, Martin Sperl wrote:

> > Every line of code that's in a driver that could be in the core is a
> > maintainence burden, people will want to start doing things like
> > combining functions in fun ways and if we want to try to do things like
> > figure out if we can coalesce adjacent transfers in the core (which we
> > really ought to) it won't know what the limitations that exist are.

> this “colaesce” of transfers into one could be one of those 
> “transformation” I am talking about - and this one would be implemented
> in core making certain assumptions (like starting on a page, ...)

Why would we want to force that assumption?  It massively reduces the
utility for DMA controllers that don't have such limitations.

> >> I wonder if such a variant-construct does not create more headaches in
> >> the long run as each spi_driver wanting to do something “something”
> >> special would then need to get updated for any additional constraint…

> > I'm sorry but I don't understand what you mean here.  However we
> > implement things anything that wants to know about constraints is going
> > to need to be updated to use them.

> That is what I want to avoid if possible - the one thing that may come
> handy (at least from my perspective) could be to give some hints to
> make optimal use of the HW
> Say:
> * preferred alignment on X byte boundry
> * preferred max spi_transfer.length

> All the other possible constraints parameters should be opaque to 
> a spi_device driver, so I would not want to include those inside the
> spi_master structure, as _then_ these get used.

You're conflating two unrelated design decisions here.  Yes, it's
unfortunate that the SPI API was written to allow client drivers to see
the master structure but that doesn't mean that converting data into
code is a good idea, that's still a bad pattern independently of the
visibility issue.

> Also the “restrictions” on the spi HW need to get defined inside the
> driver, so there will not be a new “restriction” that applies to
> an existing piece of HW just because we incorporate new options.

That's what I was saying?

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 01:56:13PM +0100, Martin Sperl wrote:

> > Every line of code that's in a driver that could be in the core is a
> > maintainence burden, people will want to start doing things like
> > combining functions in fun ways and if we want to try to do things like
> > figure out if we can coalesce adjacent transfers in the core (which we
> > really ought to) it won't know what the limitations that exist are.

> this “colaesce” of transfers into one could be one of those 
> “transformation” I am talking about - and this one would be implemented
> in core making certain assumptions (like starting on a page, ...)

Why would we want to force that assumption?  It massively reduces the
utility for DMA controllers that don't have such limitations.

> >> I wonder if such a variant-construct does not create more headaches in
> >> the long run as each spi_driver wanting to do something “something”
> >> special would then need to get updated for any additional constraint…

> > I'm sorry but I don't understand what you mean here.  However we
> > implement things anything that wants to know about constraints is going
> > to need to be updated to use them.

> That is what I want to avoid if possible - the one thing that may come
> handy (at least from my perspective) could be to give some hints to
> make optimal use of the HW
> Say:
> * preferred alignment on X byte boundry
> * preferred max spi_transfer.length

> All the other possible constraints parameters should be opaque to 
> a spi_device driver, so I would not want to include those inside the
> spi_master structure, as _then_ these get used.

You're conflating two unrelated design decisions here.  Yes, it's
unfortunate that the SPI API was written to allow client drivers to see
the master structure but that doesn't mean that converting data into
code is a good idea, that's still a bad pattern independently of the
visibility issue.

> Also the “restrictions” on the spi HW need to get defined inside the
> driver, so there will not be a new “restriction” that applies to
> an existing piece of HW just because we incorporate new options.

That's what I was saying?

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 21.11.2015 um 14:49 schrieb Mark Brown:
> On Fri, Nov 20, 2015 at 01:56:13PM +0100, Martin Sperl wrote:
> 
>>> Every line of code that's in a driver that could be in the core is a
>>> maintainence burden, people will want to start doing things like
>>> combining functions in fun ways and if we want to try to do things like
>>> figure out if we can coalesce adjacent transfers in the core (which we
>>> really ought to) it won't know what the limitations that exist are.
> 
>> this “colaesce” of transfers into one could be one of those 
>> “transformation” I am talking about - and this one would be implemented
>> in core making certain assumptions (like starting on a page, ...)
> 
> Why would we want to force that assumption?  It massively reduces the
> utility for DMA controllers that don't have such limitations.
> 
>>>> I wonder if such a variant-construct does not create more headaches in
>>>> the long run as each spi_driver wanting to do something “something”
>>>> special would then need to get updated for any additional constraint…
> 
>>> I'm sorry but I don't understand what you mean here.  However we
>>> implement things anything that wants to know about constraints is going
>>> to need to be updated to use them.
> 
>> That is what I want to avoid if possible - the one thing that may come
>> handy (at least from my perspective) could be to give some hints to
>> make optimal use of the HW
>> Say:
>> * preferred alignment on X byte boundry
>> * preferred max spi_transfer.length
> 
>> All the other possible constraints parameters should be opaque to 
>> a spi_device driver, so I would not want to include those inside the
>> spi_master structure, as _then_ these get used.
> 
> You're conflating two unrelated design decisions here.  Yes, it's
> unfortunate that the SPI API was written to allow client drivers to see
> the master structure but that doesn't mean that converting data into
> code is a good idea, that's still a bad pattern independently of the
> visibility issue.
> 
Currently the only (?) way for a protocol driver like spi-nor to get
info about HW restrictions described in the controller driver
is via the master structure and its "constraint" members.

As you say this visibility of the master structure is unfortunate:
What could be a (maybe long-term) better way to propagate restrictions
that can't be handled transparently in the core like max message size
and SPI NOR to protocol drivers?

>> Also the “restrictions” on the spi HW need to get defined inside the
>> driver, so there will not be a new “restriction” that applies to
>> an existing piece of HW just because we incorporate new options.
> 
> That's what I was saying?
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 21.11.2015 um 14:49 schrieb Mark Brown:
> On Fri, Nov 20, 2015 at 01:56:13PM +0100, Martin Sperl wrote:
> 
>>> Every line of code that's in a driver that could be in the core is a
>>> maintainence burden, people will want to start doing things like
>>> combining functions in fun ways and if we want to try to do things like
>>> figure out if we can coalesce adjacent transfers in the core (which we
>>> really ought to) it won't know what the limitations that exist are.
> 
>> this “colaesce” of transfers into one could be one of those 
>> “transformation” I am talking about - and this one would be implemented
>> in core making certain assumptions (like starting on a page, ...)
> 
> Why would we want to force that assumption?  It massively reduces the
> utility for DMA controllers that don't have such limitations.
> 
>>>> I wonder if such a variant-construct does not create more headaches in
>>>> the long run as each spi_driver wanting to do something “something”
>>>> special would then need to get updated for any additional constraint…
> 
>>> I'm sorry but I don't understand what you mean here.  However we
>>> implement things anything that wants to know about constraints is going
>>> to need to be updated to use them.
> 
>> That is what I want to avoid if possible - the one thing that may come
>> handy (at least from my perspective) could be to give some hints to
>> make optimal use of the HW
>> Say:
>> * preferred alignment on X byte boundry
>> * preferred max spi_transfer.length
> 
>> All the other possible constraints parameters should be opaque to 
>> a spi_device driver, so I would not want to include those inside the
>> spi_master structure, as _then_ these get used.
> 
> You're conflating two unrelated design decisions here.  Yes, it's
> unfortunate that the SPI API was written to allow client drivers to see
> the master structure but that doesn't mean that converting data into
> code is a good idea, that's still a bad pattern independently of the
> visibility issue.
> 
Currently the only (?) way for a protocol driver like spi-nor to get
info about HW restrictions described in the controller driver
is via the master structure and its "constraint" members.

As you say this visibility of the master structure is unfortunate:
What could be a (maybe long-term) better way to propagate restrictions
that can't be handled transparently in the core like max message size
and SPI NOR to protocol drivers?

>> Also the “restrictions” on the spi HW need to get defined inside the
>> driver, so there will not be a new “restriction” that applies to
>> an existing piece of HW just because we incorporate new options.
> 
> That's what I was saying?
> 

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

On 21 November 2015 at 00:22, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:

>> If it's desirable that a partial transfer is reported as error then a
>> particular error value should be defined for this case and drivers
>> that can continue the transfer in a driver-specific way (such as
>> spi-nor) can check for this error and handle it appropriately and pass
>> through any other error.
>
> Based on Mark's further comments (and my own intuition), I'd rather not
> try to interpret different error codes to mean "truncated but keep
> going" vs. "truncated for other reason, stop now", unless we really have
> to.
>
> I think if we do what Heiner was proposing from the beginning -- expose
> a reasonable max SPI message length -- then I think we'll cover the bulk
> of what we want. SPI NOR drivers can then try "small enough" transfers,
> and if we see any errors, those are unexpected, and we abort.
>
> Sound OK?

It sounds ok.

We actually have use case for both cases even in spi-nor.

Write transfers a page at a time and when whole page cannot be written
an error should be reported and propagated. When the master controller
cannot write like 260 bytes at once the flash becomes effectively
read-only.

Read can do arbitrary size blocks so the limit should be checked and
the transfer done in appropriately sized chunks.

On 21 November 2015 at 00:07, Brian Norris <computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

>> 64k limit on the other hand is something more usable from driver
>> writer standpoint and some banked mmap access to flash memories would
>> have similar granularity.
>
> Right.
>
>> I would also like to point out that the limit depends on the transfer
>> settings. For example, a SPI controller can have no limit on transfer
>> size but when accessing a flash memory through mmap interface the mmap
>> window limits the amount of data you can transfer at once. This
>> particular case may be fixable by moving the mmap window transparently
>> inside the driver.
>
> Hmm, I'm not sure I have much opinion on that one without having a
> non-theoretical case. It seems like it'd be best if the SPI master
> driver can work as best as it can to respect a single reasonable "max
> mesage size", even if that means choosing the lowest common denominator
> of all limitations.

I don't have an actual example either. All the cases I can think of
fall into two categories

1) it can be handled transparently
2) something is broken

maybe master driver could recalculate the limit when the transfer
parameters are changed if really needed.

On 21 November 2015 at 15:10, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Am 21.11.2015 um 14:49 schrieb Mark Brown:
>> On Fri, Nov 20, 2015 at 01:56:13PM +0100, Martin Sperl wrote:
>>
>>>> Every line of code that's in a driver that could be in the core is a
>>>> maintainence burden, people will want to start doing things like
>>>> combining functions in fun ways and if we want to try to do things like
>>>> figure out if we can coalesce adjacent transfers in the core (which we
>>>> really ought to) it won't know what the limitations that exist are.
>>
>>> this “colaesce” of transfers into one could be one of those
>>> “transformation” I am talking about - and this one would be implemented
>>> in core making certain assumptions (like starting on a page, ...)
>>
>> Why would we want to force that assumption?  It massively reduces the
>> utility for DMA controllers that don't have such limitations.
>>
>>>>> I wonder if such a variant-construct does not create more headaches in
>>>>> the long run as each spi_driver wanting to do something “something”
>>>>> special would then need to get updated for any additional constraint…
>>
>>>> I'm sorry but I don't understand what you mean here.  However we
>>>> implement things anything that wants to know about constraints is going
>>>> to need to be updated to use them.
>>
>>> That is what I want to avoid if possible - the one thing that may come
>>> handy (at least from my perspective) could be to give some hints to
>>> make optimal use of the HW
>>> Say:
>>> * preferred alignment on X byte boundry
>>> * preferred max spi_transfer.length
>>
>>> All the other possible constraints parameters should be opaque to
>>> a spi_device driver, so I would not want to include those inside the
>>> spi_master structure, as _then_ these get used.
>>
>> You're conflating two unrelated design decisions here.  Yes, it's
>> unfortunate that the SPI API was written to allow client drivers to see
>> the master structure but that doesn't mean that converting data into
>> code is a good idea, that's still a bad pattern independently of the
>> visibility issue.
>>
> Currently the only (?) way for a protocol driver like spi-nor to get
> info about HW restrictions described in the controller driver
> is via the master structure and its "constraint" members.
>
> As you say this visibility of the master structure is unfortunate:
> What could be a (maybe long-term) better way to propagate restrictions
> that can't be handled transparently in the core like max message size
> and SPI NOR to protocol drivers?
>

Actually, the fact that the maximum size of SPI transfer can be
limited is a fact which was already observed by some protocol driver
writers and handled in a driver-specific way. So adding this to the
core makes sense.

In most cases you practically cannot hit the limit because it is very
large. In the cases when some practically feasible transfer can trip
the limit it should be reported as master limitation.

As the possibility to coalesce something is protocol-specific the
protocol driver is in the position to make decision how a limitation
should be handled and should know about the limitation.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

On 21 November 2015 at 00:22, Brian Norris <computersforpeace@gmail.com> wrote:
> On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:

>> If it's desirable that a partial transfer is reported as error then a
>> particular error value should be defined for this case and drivers
>> that can continue the transfer in a driver-specific way (such as
>> spi-nor) can check for this error and handle it appropriately and pass
>> through any other error.
>
> Based on Mark's further comments (and my own intuition), I'd rather not
> try to interpret different error codes to mean "truncated but keep
> going" vs. "truncated for other reason, stop now", unless we really have
> to.
>
> I think if we do what Heiner was proposing from the beginning -- expose
> a reasonable max SPI message length -- then I think we'll cover the bulk
> of what we want. SPI NOR drivers can then try "small enough" transfers,
> and if we see any errors, those are unexpected, and we abort.
>
> Sound OK?

It sounds ok.

We actually have use case for both cases even in spi-nor.

Write transfers a page at a time and when whole page cannot be written
an error should be reported and propagated. When the master controller
cannot write like 260 bytes at once the flash becomes effectively
read-only.

Read can do arbitrary size blocks so the limit should be checked and
the transfer done in appropriately sized chunks.

On 21 November 2015 at 00:07, Brian Norris <computersforpeace@gmail.com> wrote:

>> 64k limit on the other hand is something more usable from driver
>> writer standpoint and some banked mmap access to flash memories would
>> have similar granularity.
>
> Right.
>
>> I would also like to point out that the limit depends on the transfer
>> settings. For example, a SPI controller can have no limit on transfer
>> size but when accessing a flash memory through mmap interface the mmap
>> window limits the amount of data you can transfer at once. This
>> particular case may be fixable by moving the mmap window transparently
>> inside the driver.
>
> Hmm, I'm not sure I have much opinion on that one without having a
> non-theoretical case. It seems like it'd be best if the SPI master
> driver can work as best as it can to respect a single reasonable "max
> mesage size", even if that means choosing the lowest common denominator
> of all limitations.

I don't have an actual example either. All the cases I can think of
fall into two categories

1) it can be handled transparently
2) something is broken

maybe master driver could recalculate the limit when the transfer
parameters are changed if really needed.

On 21 November 2015 at 15:10, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 21.11.2015 um 14:49 schrieb Mark Brown:
>> On Fri, Nov 20, 2015 at 01:56:13PM +0100, Martin Sperl wrote:
>>
>>>> Every line of code that's in a driver that could be in the core is a
>>>> maintainence burden, people will want to start doing things like
>>>> combining functions in fun ways and if we want to try to do things like
>>>> figure out if we can coalesce adjacent transfers in the core (which we
>>>> really ought to) it won't know what the limitations that exist are.
>>
>>> this “colaesce” of transfers into one could be one of those
>>> “transformation” I am talking about - and this one would be implemented
>>> in core making certain assumptions (like starting on a page, ...)
>>
>> Why would we want to force that assumption?  It massively reduces the
>> utility for DMA controllers that don't have such limitations.
>>
>>>>> I wonder if such a variant-construct does not create more headaches in
>>>>> the long run as each spi_driver wanting to do something “something”
>>>>> special would then need to get updated for any additional constraint…
>>
>>>> I'm sorry but I don't understand what you mean here.  However we
>>>> implement things anything that wants to know about constraints is going
>>>> to need to be updated to use them.
>>
>>> That is what I want to avoid if possible - the one thing that may come
>>> handy (at least from my perspective) could be to give some hints to
>>> make optimal use of the HW
>>> Say:
>>> * preferred alignment on X byte boundry
>>> * preferred max spi_transfer.length
>>
>>> All the other possible constraints parameters should be opaque to
>>> a spi_device driver, so I would not want to include those inside the
>>> spi_master structure, as _then_ these get used.
>>
>> You're conflating two unrelated design decisions here.  Yes, it's
>> unfortunate that the SPI API was written to allow client drivers to see
>> the master structure but that doesn't mean that converting data into
>> code is a good idea, that's still a bad pattern independently of the
>> visibility issue.
>>
> Currently the only (?) way for a protocol driver like spi-nor to get
> info about HW restrictions described in the controller driver
> is via the master structure and its "constraint" members.
>
> As you say this visibility of the master structure is unfortunate:
> What could be a (maybe long-term) better way to propagate restrictions
> that can't be handled transparently in the core like max message size
> and SPI NOR to protocol drivers?
>

Actually, the fact that the maximum size of SPI transfer can be
limited is a fact which was already observed by some protocol driver
writers and handled in a driver-specific way. So adding this to the
core makes sense.

In most cases you practically cannot hit the limit because it is very
large. In the cases when some practically feasible transfer can trip
the limit it should be reported as master limitation.

As the possibility to coalesce something is protocol-specific the
protocol driver is in the position to make decision how a limitation
should be handled and should know about the limitation.

Thanks

Michal

[PATCH 0/3] spi: mtd: Handle HW message length restrictions

[Heiner Kallweit]

Based on the ongoing discussion how to handle HW restrictions
like max SPI message size this is a new proposal.

Heiner Kallweit (3):
  spi: core: add max_msg_size to spi_master
  mtd: m25p80: handle HW message size restrictions
  spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

 drivers/mtd/devices/m25p80.c | 39 ++++++++++++++++++++++++++++++++++-----
 drivers/spi/spi-fsl-espi.c   |  1 +
 include/linux/spi/spi.h      |  4 ++++
 3 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.6.2


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] spi: mtd: Handle HW message length restrictions

[Heiner Kallweit]

Based on the ongoing discussion how to handle HW restrictions
like max SPI message size this is a new proposal.

Heiner Kallweit (3):
  spi: core: add max_msg_size to spi_master
  mtd: m25p80: handle HW message size restrictions
  spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

 drivers/mtd/devices/m25p80.c | 39 ++++++++++++++++++++++++++++++++++-----
 drivers/spi/spi-fsl-espi.c   |  1 +
 include/linux/spi/spi.h      |  4 ++++
 3 files changed, 39 insertions(+), 5 deletions(-)

-- 
2.6.2

[PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Add a member to spi_master allowing to better handle
SPI chips with a message size HW limit.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 include/linux/spi/spi.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index cce80e6..23e259b 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -302,6 +302,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  *	and it's up to the individual driver to perform any validation.
  * @min_speed_hz: Lowest supported transfer speed
  * @max_speed_hz: Highest supported transfer speed
+ * @max_msg_size: maximum message size
  * @flags: other constraints relevant to this driver
  * @bus_lock_spinlock: spinlock for SPI bus locking
  * @bus_lock_mutex: mutex for SPI bus locking
@@ -417,6 +418,9 @@ struct spi_master {
 	u32			min_speed_hz;
 	u32			max_speed_hz;
 
+	/* maximum message size */
+	size_t			max_msg_size;
+
 	/* other constraints relevant to this driver */
 	u16			flags;
 #define SPI_MASTER_HALF_DUPLEX	BIT(0)		/* can't do full duplex */
-- 
2.6.2


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Add a member to spi_master allowing to better handle
SPI chips with a message size HW limit.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 include/linux/spi/spi.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index cce80e6..23e259b 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -302,6 +302,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  *	and it's up to the individual driver to perform any validation.
  * @min_speed_hz: Lowest supported transfer speed
  * @max_speed_hz: Highest supported transfer speed
+ * @max_msg_size: maximum message size
  * @flags: other constraints relevant to this driver
  * @bus_lock_spinlock: spinlock for SPI bus locking
  * @bus_lock_mutex: mutex for SPI bus locking
@@ -417,6 +418,9 @@ struct spi_master {
 	u32			min_speed_hz;
 	u32			max_speed_hz;
 
+	/* maximum message size */
+	size_t			max_msg_size;
+
 	/* other constraints relevant to this driver */
 	u16			flags;
 #define SPI_MASTER_HALF_DUPLEX	BIT(0)		/* can't do full duplex */
-- 
2.6.2

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Mark Brown]

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

On Sun, Nov 22, 2015 at 12:01:40AM +0100, Heiner Kallweit wrote:
> Add a member to spi_master allowing to better handle
> SPI chips with a message size HW limit.

How many devices have an actual message size limit (as opposed to a
transfer size limit)?  It seems like the restrictions here are really on
transfer sizes and that we probably want a second limit for devices that
aren't able to deal with multiple transfers independently.

For slightly more complex things like this it probably also makes sense
to use an accessor - I can see us wanting to combine restrictions from
DMA engines into restrictions for the SPI controller for example.  That
gives us a bit of insulation between the clients and the API.

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

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Mark Brown]

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

On Sun, Nov 22, 2015 at 12:01:40AM +0100, Heiner Kallweit wrote:
> Add a member to spi_master allowing to better handle
> SPI chips with a message size HW limit.

How many devices have an actual message size limit (as opposed to a
transfer size limit)?  It seems like the restrictions here are really on
transfer sizes and that we probably want a second limit for devices that
aren't able to deal with multiple transfers independently.

For slightly more complex things like this it probably also makes sense
to use an accessor - I can see us wanting to combine restrictions from
DMA engines into restrictions for the SPI controller for example.  That
gives us a bit of insulation between the clients and the API.

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

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Am 22.11.2015 um 14:16 schrieb Mark Brown:
> On Sun, Nov 22, 2015 at 12:01:40AM +0100, Heiner Kallweit wrote:
>> Add a member to spi_master allowing to better handle
>> SPI chips with a message size HW limit.
> 
> How many devices have an actual message size limit (as opposed to a
> transfer size limit)?  It seems like the restrictions here are really on
> transfer sizes and that we probably want a second limit for devices that
> aren't able to deal with multiple transfers independently.
> 
To avoid misunderstandings:
For fsl-espi the length of a SPI transfer has to be programmed (max 64K)
and after this number of bytes has been transferred CS is deselected
internally. There's no way to control CS directly.
Do you consider this a message or transfer size limit?
To me this seems to be exactly what you describe as "devices that aren't
able to deal with multiple transfers independently".

> For slightly more complex things like this it probably also makes sense
> to use an accessor - I can see us wanting to combine restrictions from
> DMA engines into restrictions for the SPI controller for example.  That
> gives us a bit of insulation between the clients and the API.
> 
When you talk about accessors do you think of hooks in spi_master so that
each controller driver can provide its own implementation of e.g.
something like get_max_msg_size()?

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Am 22.11.2015 um 14:16 schrieb Mark Brown:
> On Sun, Nov 22, 2015 at 12:01:40AM +0100, Heiner Kallweit wrote:
>> Add a member to spi_master allowing to better handle
>> SPI chips with a message size HW limit.
> 
> How many devices have an actual message size limit (as opposed to a
> transfer size limit)?  It seems like the restrictions here are really on
> transfer sizes and that we probably want a second limit for devices that
> aren't able to deal with multiple transfers independently.
> 
To avoid misunderstandings:
For fsl-espi the length of a SPI transfer has to be programmed (max 64K)
and after this number of bytes has been transferred CS is deselected
internally. There's no way to control CS directly.
Do you consider this a message or transfer size limit?
To me this seems to be exactly what you describe as "devices that aren't
able to deal with multiple transfers independently".

> For slightly more complex things like this it probably also makes sense
> to use an accessor - I can see us wanting to combine restrictions from
> DMA engines into restrictions for the SPI controller for example.  That
> gives us a bit of insulation between the clients and the API.
> 
When you talk about accessors do you think of hooks in spi_master so that
each controller driver can provide its own implementation of e.g.
something like get_max_msg_size()?

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Mark Brown]

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

On Sun, Nov 22, 2015 at 05:15:04PM +0100, Heiner Kallweit wrote:
> Am 22.11.2015 um 14:16 schrieb Mark Brown:

> To avoid misunderstandings:
> For fsl-espi the length of a SPI transfer has to be programmed (max 64K)
> and after this number of bytes has been transferred CS is deselected
> internally. There's no way to control CS directly.
> Do you consider this a message or transfer size limit?
> To me this seems to be exactly what you describe as "devices that aren't
> able to deal with multiple transfers independently".

Well, possibly.  What happens with a very large proportion of SPI
controllers is that we just ignore the /CS functionality of the
controller and use a GPIO instead, most SoC integrations also support
GPIO on the same pin and there's rarely any advantage in trying to use
the integrated support.

> > For slightly more complex things like this it probably also makes sense
> > to use an accessor - I can see us wanting to combine restrictions from
> > DMA engines into restrictions for the SPI controller for example.  That
> > gives us a bit of insulation between the clients and the API.

> When you talk about accessors do you think of hooks in spi_master so that
> each controller driver can provide its own implementation of e.g.
> something like get_max_msg_size()?

No, for the clients so they aren't peering at the struct.

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

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Mark Brown]

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

On Sun, Nov 22, 2015 at 05:15:04PM +0100, Heiner Kallweit wrote:
> Am 22.11.2015 um 14:16 schrieb Mark Brown:

> To avoid misunderstandings:
> For fsl-espi the length of a SPI transfer has to be programmed (max 64K)
> and after this number of bytes has been transferred CS is deselected
> internally. There's no way to control CS directly.
> Do you consider this a message or transfer size limit?
> To me this seems to be exactly what you describe as "devices that aren't
> able to deal with multiple transfers independently".

Well, possibly.  What happens with a very large proportion of SPI
controllers is that we just ignore the /CS functionality of the
controller and use a GPIO instead, most SoC integrations also support
GPIO on the same pin and there's rarely any advantage in trying to use
the integrated support.

> > For slightly more complex things like this it probably also makes sense
> > to use an accessor - I can see us wanting to combine restrictions from
> > DMA engines into restrictions for the SPI controller for example.  That
> > gives us a bit of insulation between the clients and the API.

> When you talk about accessors do you think of hooks in spi_master so that
> each controller driver can provide its own implementation of e.g.
> something like get_max_msg_size()?

No, for the clients so they aren't peering at the struct.

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

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Am 23.11.2015 um 12:38 schrieb Mark Brown:
> On Sun, Nov 22, 2015 at 05:15:04PM +0100, Heiner Kallweit wrote:
>> Am 22.11.2015 um 14:16 schrieb Mark Brown:
> 
>> To avoid misunderstandings:
>> For fsl-espi the length of a SPI transfer has to be programmed (max 64K)
>> and after this number of bytes has been transferred CS is deselected
>> internally. There's no way to control CS directly.
>> Do you consider this a message or transfer size limit?
>> To me this seems to be exactly what you describe as "devices that aren't
>> able to deal with multiple transfers independently".
> 
> Well, possibly.  What happens with a very large proportion of SPI
> controllers is that we just ignore the /CS functionality of the
> controller and use a GPIO instead, most SoC integrations also support
> GPIO on the same pin and there's rarely any advantage in trying to use
> the integrated support.
> 
Right, that's the case also for fsl-espi. The bad thing is that there's
no way to change only the CS pin to a GPIO.
Only the complete block of SPI signals can be switched to GPIO.

>>> For slightly more complex things like this it probably also makes sense
>>> to use an accessor - I can see us wanting to combine restrictions from
>>> DMA engines into restrictions for the SPI controller for example.  That
>>> gives us a bit of insulation between the clients and the API.
> 
>> When you talk about accessors do you think of hooks in spi_master so that
>> each controller driver can provide its own implementation of e.g.
>> something like get_max_msg_size()?
> 
> No, for the clients so they aren't peering at the struct.
> 
Sure, do you think of a simple getter like this or a more complex thing?

size_t spi_get_max_msg_size(struct spi_master *master)
{
	return master->max_msg_size;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Am 23.11.2015 um 12:38 schrieb Mark Brown:
> On Sun, Nov 22, 2015 at 05:15:04PM +0100, Heiner Kallweit wrote:
>> Am 22.11.2015 um 14:16 schrieb Mark Brown:
> 
>> To avoid misunderstandings:
>> For fsl-espi the length of a SPI transfer has to be programmed (max 64K)
>> and after this number of bytes has been transferred CS is deselected
>> internally. There's no way to control CS directly.
>> Do you consider this a message or transfer size limit?
>> To me this seems to be exactly what you describe as "devices that aren't
>> able to deal with multiple transfers independently".
> 
> Well, possibly.  What happens with a very large proportion of SPI
> controllers is that we just ignore the /CS functionality of the
> controller and use a GPIO instead, most SoC integrations also support
> GPIO on the same pin and there's rarely any advantage in trying to use
> the integrated support.
> 
Right, that's the case also for fsl-espi. The bad thing is that there's
no way to change only the CS pin to a GPIO.
Only the complete block of SPI signals can be switched to GPIO.

>>> For slightly more complex things like this it probably also makes sense
>>> to use an accessor - I can see us wanting to combine restrictions from
>>> DMA engines into restrictions for the SPI controller for example.  That
>>> gives us a bit of insulation between the clients and the API.
> 
>> When you talk about accessors do you think of hooks in spi_master so that
>> each controller driver can provide its own implementation of e.g.
>> something like get_max_msg_size()?
> 
> No, for the clients so they aren't peering at the struct.
> 
Sure, do you think of a simple getter like this or a more complex thing?

size_t spi_get_max_msg_size(struct spi_master *master)
{
	return master->max_msg_size;
}

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Mark Brown]

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

On Fri, Nov 27, 2015 at 08:26:52PM +0100, Heiner Kallweit wrote:
> Am 23.11.2015 um 12:38 schrieb Mark Brown:

> > No, for the clients so they aren't peering at the struct.

> Sure, do you think of a simple getter like this or a more complex thing?

> size_t spi_get_max_msg_size(struct spi_master *master)
> {
> 	return master->max_msg_size;
> }

That's fine, the point is to allow it to get complicated later rather
than that we have anything else to put in there right now.

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

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Mark Brown]

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

On Fri, Nov 27, 2015 at 08:26:52PM +0100, Heiner Kallweit wrote:
> Am 23.11.2015 um 12:38 schrieb Mark Brown:

> > No, for the clients so they aren't peering at the struct.

> Sure, do you think of a simple getter like this or a more complex thing?

> size_t spi_get_max_msg_size(struct spi_master *master)
> {
> 	return master->max_msg_size;
> }

That's fine, the point is to allow it to get complicated later rather
than that we have anything else to put in there right now.

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

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Am 30.11.2015 um 17:42 schrieb Mark Brown:
> On Fri, Nov 27, 2015 at 08:26:52PM +0100, Heiner Kallweit wrote:
>> Am 23.11.2015 um 12:38 schrieb Mark Brown:
> 
>>> No, for the clients so they aren't peering at the struct.
> 
>> Sure, do you think of a simple getter like this or a more complex thing?
> 
>> size_t spi_get_max_msg_size(struct spi_master *master)
>> {
>> 	return master->max_msg_size;
>> }
> 
> That's fine, the point is to allow it to get complicated later rather
> than that we have anything else to put in there right now.
> 
OK, then I'll submit an updated version of the patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Am 30.11.2015 um 17:42 schrieb Mark Brown:
> On Fri, Nov 27, 2015 at 08:26:52PM +0100, Heiner Kallweit wrote:
>> Am 23.11.2015 um 12:38 schrieb Mark Brown:
> 
>>> No, for the clients so they aren't peering at the struct.
> 
>> Sure, do you think of a simple getter like this or a more complex thing?
> 
>> size_t spi_get_max_msg_size(struct spi_master *master)
>> {
>> 	return master->max_msg_size;
>> }
> 
> That's fine, the point is to allow it to get complicated later rather
> than that we have anything else to put in there right now.
> 
OK, then I'll submit an updated version of the patch.

[PATCH 2/3] mtd: m25p80: handle HW message size restrictions

[Heiner Kallweit]

Extend m25p80_read allowing to read in chunks in case the
SPI HW has a max supported message size.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/mtd/devices/m25p80.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index c9c3b7f..df4c510 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -115,11 +115,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 +149,39 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 }
 
 /*
+ * 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,
+                       size_t *retlen, u_char *buf)
+{
+	struct m25p *flash = nor->priv;
+	size_t cmd_len, xfer_len, max_len;
+	int ret = 0;
+
+	/* convert the dummy cycles to the number of bytes */
+	cmd_len = m25p_cmdsz(nor) + nor->read_dummy / 8;
+
+	max_len = flash->spi->master->max_msg_size ?: SIZE_MAX;
+
+	if (unlikely(max_len < cmd_len))
+		return -EINVAL;
+
+	max_len -= cmd_len;
+
+	while (len) {
+		xfer_len = min(len, max_len);
+		ret = _m25p80_read(nor, from, xfer_len, retlen, buf);
+		if (ret < 0)
+			break;
+		from += xfer_len;
+		len -= xfer_len;
+	}
+
+	return ret;
+}
+
+/*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
  * understands FAST_READ (for clocks over 25 MHz).
-- 
2.6.2


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] mtd: m25p80: handle HW message size restrictions

[Heiner Kallweit]

Extend m25p80_read allowing to read in chunks in case the
SPI HW has a max supported message size.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 39 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index c9c3b7f..df4c510 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -115,11 +115,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 +149,39 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 }
 
 /*
+ * 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,
+                       size_t *retlen, u_char *buf)
+{
+	struct m25p *flash = nor->priv;
+	size_t cmd_len, xfer_len, max_len;
+	int ret = 0;
+
+	/* convert the dummy cycles to the number of bytes */
+	cmd_len = m25p_cmdsz(nor) + nor->read_dummy / 8;
+
+	max_len = flash->spi->master->max_msg_size ?: SIZE_MAX;
+
+	if (unlikely(max_len < cmd_len))
+		return -EINVAL;
+
+	max_len -= cmd_len;
+
+	while (len) {
+		xfer_len = min(len, max_len);
+		ret = _m25p80_read(nor, from, xfer_len, retlen, buf);
+		if (ret < 0)
+			break;
+		from += xfer_len;
+		len -= xfer_len;
+	}
+
+	return ret;
+}
+
+/*
  * board specific setup should have ensured the SPI clock used here
  * matches what the READ command supports, at least until this driver
  * understands FAST_READ (for clocks over 25 MHz).
-- 
2.6.2

Re: [PATCH 2/3] mtd: m25p80: handle HW message size restrictions

[Michal Suchanek]

Hello,

On 22 November 2015 at 00:08, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Extend m25p80_read allowing to read in chunks in case the
> SPI HW has a max supported message size.

I would prefer if this was handled as part of the patchset which adds
error checking to spi-nor.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] mtd: m25p80: handle HW message size restrictions

[Michal Suchanek]

Hello,

On 22 November 2015 at 00:08, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Extend m25p80_read allowing to read in chunks in case the
> SPI HW has a max supported message size.

I would prefer if this was handled as part of the patchset which adds
error checking to spi-nor.

Thanks

Michal

[PATCH 3/3] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Heiner Kallweit]

Propagate the 64K message size limitation allowing client drivers
to deal properly with this limitation.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi-fsl-espi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index c27124a..773bbab 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c
@@ -670,6 +670,7 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	master->cleanup = fsl_espi_cleanup;
 	master->transfer_one_message = fsl_espi_do_one_msg;
 	master->auto_runtime_pm = true;
+	master->max_msg_size = SPCOM_TRANLEN_MAX;
 
 	mpc8xxx_spi = spi_master_get_devdata(master);
 
-- 
2.6.2


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Heiner Kallweit]

Propagate the 64K message size limitation allowing client drivers
to deal properly with this limitation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/spi/spi-fsl-espi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index c27124a..773bbab 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c
@@ -670,6 +670,7 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	master->cleanup = fsl_espi_cleanup;
 	master->transfer_one_message = fsl_espi_do_one_msg;
 	master->auto_runtime_pm = true;
+	master->max_msg_size = SPCOM_TRANLEN_MAX;
 
 	mpc8xxx_spi = spi_master_get_devdata(master);
 
-- 
2.6.2

[PATCH v2 1/2] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Add a member to spi_master allowing to better handle
SPI chips with a message size HW limit.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
v2: add accessor

 include/linux/spi/spi.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index cce80e6..a8abd24 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -302,6 +302,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  *	and it's up to the individual driver to perform any validation.
  * @min_speed_hz: Lowest supported transfer speed
  * @max_speed_hz: Highest supported transfer speed
+ * @max_msg_size: maximum supported message size
  * @flags: other constraints relevant to this driver
  * @bus_lock_spinlock: spinlock for SPI bus locking
  * @bus_lock_mutex: mutex for SPI bus locking
@@ -417,6 +418,9 @@ struct spi_master {
 	u32			min_speed_hz;
 	u32			max_speed_hz;
 
+	/* maximum message size */
+	size_t			max_msg_size;
+
 	/* other constraints relevant to this driver */
 	u16			flags;
 #define SPI_MASTER_HALF_DUPLEX	BIT(0)		/* can't do full duplex */
@@ -556,6 +560,11 @@ static inline void spi_master_put(struct spi_master *master)
 		put_device(&master->dev);
 }
 
+static inline size_t spi_master_get_max_msg_size(struct spi_master *master)
+{
+	return master->max_msg_size;
+}
+
 /* PM calls that need to be issued by the driver */
 extern int spi_master_suspend(struct spi_master *master);
 extern int spi_master_resume(struct spi_master *master);
-- 
2.6.2

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/2] spi: core: add max_msg_size to spi_master

[Heiner Kallweit]

Add a member to spi_master allowing to better handle
SPI chips with a message size HW limit.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2: add accessor

 include/linux/spi/spi.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index cce80e6..a8abd24 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -302,6 +302,7 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  *	and it's up to the individual driver to perform any validation.
  * @min_speed_hz: Lowest supported transfer speed
  * @max_speed_hz: Highest supported transfer speed
+ * @max_msg_size: maximum supported message size
  * @flags: other constraints relevant to this driver
  * @bus_lock_spinlock: spinlock for SPI bus locking
  * @bus_lock_mutex: mutex for SPI bus locking
@@ -417,6 +418,9 @@ struct spi_master {
 	u32			min_speed_hz;
 	u32			max_speed_hz;
 
+	/* maximum message size */
+	size_t			max_msg_size;
+
 	/* other constraints relevant to this driver */
 	u16			flags;
 #define SPI_MASTER_HALF_DUPLEX	BIT(0)		/* can't do full duplex */
@@ -556,6 +560,11 @@ static inline void spi_master_put(struct spi_master *master)
 		put_device(&master->dev);
 }
 
+static inline size_t spi_master_get_max_msg_size(struct spi_master *master)
+{
+	return master->max_msg_size;
+}
+
 /* PM calls that need to be issued by the driver */
 extern int spi_master_suspend(struct spi_master *master);
 extern int spi_master_resume(struct spi_master *master);
-- 
2.6.2

[PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Heiner Kallweit]

Propagate the 64K message size limitation allowing client drivers
to deal properly with this limitation.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi-fsl-espi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index c27124a..773bbab 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c
@@ -670,6 +670,7 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	master->cleanup = fsl_espi_cleanup;
 	master->transfer_one_message = fsl_espi_do_one_msg;
 	master->auto_runtime_pm = true;
+	master->max_msg_size = SPCOM_TRANLEN_MAX;
 
 	mpc8xxx_spi = spi_master_get_devdata(master);
 
-- 
2.6.2


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Heiner Kallweit]

Propagate the 64K message size limitation allowing client drivers
to deal properly with this limitation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/spi/spi-fsl-espi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
index c27124a..773bbab 100644
--- a/drivers/spi/spi-fsl-espi.c
+++ b/drivers/spi/spi-fsl-espi.c
@@ -670,6 +670,7 @@ static struct spi_master * fsl_espi_probe(struct device *dev,
 	master->cleanup = fsl_espi_cleanup;
 	master->transfer_one_message = fsl_espi_do_one_msg;
 	master->auto_runtime_pm = true;
+	master->max_msg_size = SPCOM_TRANLEN_MAX;
 
 	mpc8xxx_spi = spi_master_get_devdata(master);
 
-- 
2.6.2

Re: [PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Mark Brown]

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

On Mon, Nov 30, 2015 at 09:25:36PM +0100, Heiner Kallweit wrote:
> Propagate the 64K message size limitation allowing client drivers
> to deal properly with this limitation.

Please don't submit patches into the middle of existing threads, it
makes it very confusing trying to work out what the currently proposed
patches are:

   8592 N T 11/22 Heiner Kallweit (2.1K) │                   │ ├─>[PATCH 2/3] mt
   8593 N C 11/22 Michal Suchanek (0.5K) │                   │ │ └─>
-> 8594 N T 11/22 Heiner Kallweit (0.9K) │                   │ ├─>[PATCH 3/3] sp
   8595 N T 11/30 Heiner Kallweit (1.7K) │                   │ ├─>[PATCH v2 1/2]
   8596 N T 11/30 Heiner Kallweit (0.9K) │                   │ └─>[PATCH resubmi
   8597 NsF 11/22 To Heiner Kallw (1.4K) │                   └─>Re: RfC: Handle

I *think* the two messages from yesterday are supposed to be a single
patch series but they're buried in the middle of a huge thread at the
same level as some older patches and for soem reason have different tags
(one of which is "resubmit" which is really just adding noise).

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

Re: [PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Mark Brown]

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

On Mon, Nov 30, 2015 at 09:25:36PM +0100, Heiner Kallweit wrote:
> Propagate the 64K message size limitation allowing client drivers
> to deal properly with this limitation.

Please don't submit patches into the middle of existing threads, it
makes it very confusing trying to work out what the currently proposed
patches are:

   8592 N T 11/22 Heiner Kallweit (2.1K) │                   │ ├─>[PATCH 2/3] mt
   8593 N C 11/22 Michal Suchanek (0.5K) │                   │ │ └─>
-> 8594 N T 11/22 Heiner Kallweit (0.9K) │                   │ ├─>[PATCH 3/3] sp
   8595 N T 11/30 Heiner Kallweit (1.7K) │                   │ ├─>[PATCH v2 1/2]
   8596 N T 11/30 Heiner Kallweit (0.9K) │                   │ └─>[PATCH resubmi
   8597 NsF 11/22 To Heiner Kallw (1.4K) │                   └─>Re: RfC: Handle

I *think* the two messages from yesterday are supposed to be a single
patch series but they're buried in the middle of a huge thread at the
same level as some older patches and for soem reason have different tags
(one of which is "resubmit" which is really just adding noise).

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

Re: [PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Heiner Kallweit]

Am 01.12.2015 um 15:19 schrieb Mark Brown:
> On Mon, Nov 30, 2015 at 09:25:36PM +0100, Heiner Kallweit wrote:
>> Propagate the 64K message size limitation allowing client drivers
>> to deal properly with this limitation.
> 
> Please don't submit patches into the middle of existing threads, it
> makes it very confusing trying to work out what the currently proposed
> patches are:
> 
>    8592 N T 11/22 Heiner Kallweit (2.1K) │                   │ ├─>[PATCH 2/3] mt
>    8593 N C 11/22 Michal Suchanek (0.5K) │                   │ │ └─>
> -> 8594 N T 11/22 Heiner Kallweit (0.9K) │                   │ ├─>[PATCH 3/3] sp
>    8595 N T 11/30 Heiner Kallweit (1.7K) │                   │ ├─>[PATCH v2 1/2]
>    8596 N T 11/30 Heiner Kallweit (0.9K) │                   │ └─>[PATCH resubmi
>    8597 NsF 11/22 To Heiner Kallw (1.4K) │                   └─>Re: RfC: Handle
> 
> I *think* the two messages from yesterday are supposed to be a single
> patch series but they're buried in the middle of a huge thread at the
> same level as some older patches and for soem reason have different tags
> (one of which is "resubmit" which is really just adding noise).
> 
Sorry, my mail template referred to the discussion thread.
I'll resubmit.



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions

[Heiner Kallweit]

Am 01.12.2015 um 15:19 schrieb Mark Brown:
> On Mon, Nov 30, 2015 at 09:25:36PM +0100, Heiner Kallweit wrote:
>> Propagate the 64K message size limitation allowing client drivers
>> to deal properly with this limitation.
> 
> Please don't submit patches into the middle of existing threads, it
> makes it very confusing trying to work out what the currently proposed
> patches are:
> 
>    8592 N T 11/22 Heiner Kallweit (2.1K) │                   │ ├─>[PATCH 2/3] mt
>    8593 N C 11/22 Michal Suchanek (0.5K) │                   │ │ └─>
> -> 8594 N T 11/22 Heiner Kallweit (0.9K) │                   │ ├─>[PATCH 3/3] sp
>    8595 N T 11/30 Heiner Kallweit (1.7K) │                   │ ├─>[PATCH v2 1/2]
>    8596 N T 11/30 Heiner Kallweit (0.9K) │                   │ └─>[PATCH resubmi
>    8597 NsF 11/22 To Heiner Kallw (1.4K) │                   └─>Re: RfC: Handle
> 
> I *think* the two messages from yesterday are supposed to be a single
> patch series but they're buried in the middle of a huge thread at the
> same level as some older patches and for soem reason have different tags
> (one of which is "resubmit" which is really just adding noise).
> 
Sorry, my mail template referred to the discussion thread.
I'll resubmit.

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Sat, Nov 21, 2015 at 03:10:03PM +0100, Heiner Kallweit wrote:

> As you say this visibility of the master structure is unfortunate:
> What could be a (maybe long-term) better way to propagate restrictions
> that can't be handled transparently in the core like max message size
> and SPI NOR to protocol drivers?

The point is that the spi_master structure is not opaque to users (with
a consumer/provider split like modern APIs have).  We'd have to
reorganise the headers and provide accessors in the client API to handle
this.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Sat, Nov 21, 2015 at 03:10:03PM +0100, Heiner Kallweit wrote:

> As you say this visibility of the master structure is unfortunate:
> What could be a (maybe long-term) better way to propagate restrictions
> that can't be handled transparently in the core like max message size
> and SPI NOR to protocol drivers?

The point is that the spi_master structure is not opaque to users (with
a consumer/provider split like modern APIs have).  We'd have to
reorganise the headers and provide accessors in the client API to handle
this.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
> There have been few discussions in the past about how to handle SPI controller
> limitations like max message length. However they don't seem to have resulted
> in accepted patches yet.
> I also stumbled across this topic because I own a device using Freescale's
> ESPI which has a 64K message size limitation.
> 
> At least one agreed fact is that silently assembling chunks in protocol
> drivers is not the preferred approach.

Hmm, are you referring to this sort of approach [1], where the
spi_message::acutal_length informs the spi_nor layer that the transfer
was truncated?

[1] [PATCH v4 7/7] mtd: spi-nor: add read loop
    http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html

> Maybe a better approach would be to introduce a new member of spi_master
> dealing with controller limitations.
> My issue is just the message size limitation but most likely there are more
> and different limitations in other controllers.
> 
> I'd introduce a struct spi_controller_restrictions and add a member to spi_master
> pointing to such a struct. Then a controller driver could do something like this:
> 
> static const struct spi_controller_restrictions fsl_espi_restrictions = {
> 	.max_msg_size	= 0xffff,
> };
> 
> master->restrictions = &fsl_espi_restrictions;

OK, so I think Mark suggested we not move to a 'restrictions' struct,
but otherwise it doesn't sound like he's opposed to this.

> I also add an example how a protocol driver could use this extension.
> Appreciate any comment.

One question I have: is it necessary to push the handling out into the
protocol driver? I feel like I've seen a partial answer to this: the
'actual_legnth' return field suggests that the protocol driver already
has to deal with shorter-than-desired transfers.

Then I have another one: is the 'actual_length' field really
insufficient? For instance, is it non-kosher for a spi_master to just
cutoff the message at (for instance) 64K, and expect the protocol
driver to handle that (e.g., with Michal's patch from [1])? And if that
is kosher, then is there a good reason for the protocol driver to know
the exact maximum for its spi_master?

[snip example]

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
> There have been few discussions in the past about how to handle SPI controller
> limitations like max message length. However they don't seem to have resulted
> in accepted patches yet.
> I also stumbled across this topic because I own a device using Freescale's
> ESPI which has a 64K message size limitation.
> 
> At least one agreed fact is that silently assembling chunks in protocol
> drivers is not the preferred approach.

Hmm, are you referring to this sort of approach [1], where the
spi_message::acutal_length informs the spi_nor layer that the transfer
was truncated?

[1] [PATCH v4 7/7] mtd: spi-nor: add read loop
    http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html

> Maybe a better approach would be to introduce a new member of spi_master
> dealing with controller limitations.
> My issue is just the message size limitation but most likely there are more
> and different limitations in other controllers.
> 
> I'd introduce a struct spi_controller_restrictions and add a member to spi_master
> pointing to such a struct. Then a controller driver could do something like this:
> 
> static const struct spi_controller_restrictions fsl_espi_restrictions = {
> 	.max_msg_size	= 0xffff,
> };
> 
> master->restrictions = &fsl_espi_restrictions;

OK, so I think Mark suggested we not move to a 'restrictions' struct,
but otherwise it doesn't sound like he's opposed to this.

> I also add an example how a protocol driver could use this extension.
> Appreciate any comment.

One question I have: is it necessary to push the handling out into the
protocol driver? I feel like I've seen a partial answer to this: the
'actual_legnth' return field suggests that the protocol driver already
has to deal with shorter-than-desired transfers.

Then I have another one: is the 'actual_length' field really
insufficient? For instance, is it non-kosher for a spi_master to just
cutoff the message at (for instance) 64K, and expect the protocol
driver to handle that (e.g., with Michal's patch from [1])? And if that
is kosher, then is there a good reason for the protocol driver to know
the exact maximum for its spi_master?

[snip example]

Brian

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 20.11.2015 um 01:02 schrieb Brian Norris:
> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
>> There have been few discussions in the past about how to handle SPI controller
>> limitations like max message length. However they don't seem to have resulted
>> in accepted patches yet.
>> I also stumbled across this topic because I own a device using Freescale's
>> ESPI which has a 64K message size limitation.
>>
>> At least one agreed fact is that silently assembling chunks in protocol
>> drivers is not the preferred approach.
> 
> Hmm, are you referring to this sort of approach [1], where the
> spi_message::acutal_length informs the spi_nor layer that the transfer
> was truncated?
> 
> [1] [PATCH v4 7/7] mtd: spi-nor: add read loop
>     http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html
> 
>> Maybe a better approach would be to introduce a new member of spi_master
>> dealing with controller limitations.
>> My issue is just the message size limitation but most likely there are more
>> and different limitations in other controllers.
>>
>> I'd introduce a struct spi_controller_restrictions and add a member to spi_master
>> pointing to such a struct. Then a controller driver could do something like this:
>>
>> static const struct spi_controller_restrictions fsl_espi_restrictions = {
>> 	.max_msg_size	= 0xffff,
>> };
>>
>> master->restrictions = &fsl_espi_restrictions;
> 
> OK, so I think Mark suggested we not move to a 'restrictions' struct,
> but otherwise it doesn't sound like he's opposed to this.
> 
That's how I read his comments too.

>> I also add an example how a protocol driver could use this extension.
>> Appreciate any comment.
> 
> One question I have: is it necessary to push the handling out into the
> protocol driver? I feel like I've seen a partial answer to this: the
> 'actual_legnth' return field suggests that the protocol driver already
> has to deal with shorter-than-desired transfers.
> 
> Then I have another one: is the 'actual_length' field really
> insufficient? For instance, is it non-kosher for a spi_master to just
> cutoff the message at (for instance) 64K, and expect the protocol
> driver to handle that (e.g., with Michal's patch from [1])? And if that
> is kosher, then is there a good reason for the protocol driver to know
> the exact maximum for its spi_master?
> 
It would be sufficient if it's a valid case that spi_master returns 0
and an actual_length < requested_length as this is some kind of error
situation.
I could also fully understand if spi_master doesn't return 0 but
-EMSGSIZE in such a case.
And the suggested patch would bail out of the chunk-assembling loop
once it get's an error from the SPI transfer
(after applying patch 2 of the series which introduces checking
the return code of the spi_sync call in m25p80_read).

> [snip example]
> 
> Brian
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 20.11.2015 um 01:02 schrieb Brian Norris:
> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
>> There have been few discussions in the past about how to handle SPI controller
>> limitations like max message length. However they don't seem to have resulted
>> in accepted patches yet.
>> I also stumbled across this topic because I own a device using Freescale's
>> ESPI which has a 64K message size limitation.
>>
>> At least one agreed fact is that silently assembling chunks in protocol
>> drivers is not the preferred approach.
> 
> Hmm, are you referring to this sort of approach [1], where the
> spi_message::acutal_length informs the spi_nor layer that the transfer
> was truncated?
> 
> [1] [PATCH v4 7/7] mtd: spi-nor: add read loop
>     http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html
> 
>> Maybe a better approach would be to introduce a new member of spi_master
>> dealing with controller limitations.
>> My issue is just the message size limitation but most likely there are more
>> and different limitations in other controllers.
>>
>> I'd introduce a struct spi_controller_restrictions and add a member to spi_master
>> pointing to such a struct. Then a controller driver could do something like this:
>>
>> static const struct spi_controller_restrictions fsl_espi_restrictions = {
>> 	.max_msg_size	= 0xffff,
>> };
>>
>> master->restrictions = &fsl_espi_restrictions;
> 
> OK, so I think Mark suggested we not move to a 'restrictions' struct,
> but otherwise it doesn't sound like he's opposed to this.
> 
That's how I read his comments too.

>> I also add an example how a protocol driver could use this extension.
>> Appreciate any comment.
> 
> One question I have: is it necessary to push the handling out into the
> protocol driver? I feel like I've seen a partial answer to this: the
> 'actual_legnth' return field suggests that the protocol driver already
> has to deal with shorter-than-desired transfers.
> 
> Then I have another one: is the 'actual_length' field really
> insufficient? For instance, is it non-kosher for a spi_master to just
> cutoff the message at (for instance) 64K, and expect the protocol
> driver to handle that (e.g., with Michal's patch from [1])? And if that
> is kosher, then is there a good reason for the protocol driver to know
> the exact maximum for its spi_master?
> 
It would be sufficient if it's a valid case that spi_master returns 0
and an actual_length < requested_length as this is some kind of error
situation.
I could also fully understand if spi_master doesn't return 0 but
-EMSGSIZE in such a case.
And the suggested patch would bail out of the chunk-assembling loop
once it get's an error from the SPI transfer
(after applying patch 2 of the series which introduces checking
the return code of the spi_sync call in m25p80_read).

> [snip example]
> 
> Brian
> 

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Am 20.11.2015 um 01:02 schrieb Brian Norris:
>> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
>>> There have been few discussions in the past about how to handle SPI controller
>>> limitations like max message length. However they don't seem to have resulted
>>> in accepted patches yet.
>>> I also stumbled across this topic because I own a device using Freescale's
>>> ESPI which has a 64K message size limitation.
>>>
>>> At least one agreed fact is that silently assembling chunks in protocol
>>> drivers is not the preferred approach.
>>
>> Hmm, are you referring to this sort of approach [1], where the
>> spi_message::acutal_length informs the spi_nor layer that the transfer
>> was truncated?
>>
>> [1] [PATCH v4 7/7] mtd: spi-nor: add read loop
>>     http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html
>>
>>> Maybe a better approach would be to introduce a new member of spi_master
>>> dealing with controller limitations.
>>> My issue is just the message size limitation but most likely there are more
>>> and different limitations in other controllers.
>>>
>>> I'd introduce a struct spi_controller_restrictions and add a member to spi_master
>>> pointing to such a struct. Then a controller driver could do something like this:
>>>
>>> static const struct spi_controller_restrictions fsl_espi_restrictions = {
>>>      .max_msg_size   = 0xffff,
>>> };
>>>
>>> master->restrictions = &fsl_espi_restrictions;
>>
>> OK, so I think Mark suggested we not move to a 'restrictions' struct,
>> but otherwise it doesn't sound like he's opposed to this.
>>
> That's how I read his comments too.
>
>>> I also add an example how a protocol driver could use this extension.
>>> Appreciate any comment.
>>
>> One question I have: is it necessary to push the handling out into the
>> protocol driver? I feel like I've seen a partial answer to this: the
>> 'actual_legnth' return field suggests that the protocol driver already
>> has to deal with shorter-than-desired transfers.
>>
>> Then I have another one: is the 'actual_length' field really
>> insufficient? For instance, is it non-kosher for a spi_master to just
>> cutoff the message at (for instance) 64K, and expect the protocol
>> driver to handle that (e.g., with Michal's patch from [1])? And if that
>> is kosher, then is there a good reason for the protocol driver to know
>> the exact maximum for its spi_master?
>>
> It would be sufficient if it's a valid case that spi_master returns 0
> and an actual_length < requested_length as this is some kind of error
> situation.

I had one more look at the SPI core and e.g. spi_write_then_read
calls spi_sync w/o checking actual_length afterwards.
This can mean the discussed case is not valid, however it also could be
simply a bug.

If the discussed case is valid a clear hint to all users of spi_sync and
friends should be added that the caller can not rely on status code 0
only but must check actual_length to verify that the complete message
was transferred.

It would be good to hear Mark's opinion on this.

> I could also fully understand if spi_master doesn't return 0 but
> -EMSGSIZE in such a case.
> And the suggested patch would bail out of the chunk-assembling loop
> once it get's an error from the SPI transfer
> (after applying patch 2 of the series which introduces checking
> the return code of the spi_sync call in m25p80_read).
>
>> [snip example]
>>
>> Brian
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 20.11.2015 um 01:02 schrieb Brian Norris:
>> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:
>>> There have been few discussions in the past about how to handle SPI controller
>>> limitations like max message length. However they don't seem to have resulted
>>> in accepted patches yet.
>>> I also stumbled across this topic because I own a device using Freescale's
>>> ESPI which has a 64K message size limitation.
>>>
>>> At least one agreed fact is that silently assembling chunks in protocol
>>> drivers is not the preferred approach.
>>
>> Hmm, are you referring to this sort of approach [1], where the
>> spi_message::acutal_length informs the spi_nor layer that the transfer
>> was truncated?
>>
>> [1] [PATCH v4 7/7] mtd: spi-nor: add read loop
>>     http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html
>>
>>> Maybe a better approach would be to introduce a new member of spi_master
>>> dealing with controller limitations.
>>> My issue is just the message size limitation but most likely there are more
>>> and different limitations in other controllers.
>>>
>>> I'd introduce a struct spi_controller_restrictions and add a member to spi_master
>>> pointing to such a struct. Then a controller driver could do something like this:
>>>
>>> static const struct spi_controller_restrictions fsl_espi_restrictions = {
>>>      .max_msg_size   = 0xffff,
>>> };
>>>
>>> master->restrictions = &fsl_espi_restrictions;
>>
>> OK, so I think Mark suggested we not move to a 'restrictions' struct,
>> but otherwise it doesn't sound like he's opposed to this.
>>
> That's how I read his comments too.
>
>>> I also add an example how a protocol driver could use this extension.
>>> Appreciate any comment.
>>
>> One question I have: is it necessary to push the handling out into the
>> protocol driver? I feel like I've seen a partial answer to this: the
>> 'actual_legnth' return field suggests that the protocol driver already
>> has to deal with shorter-than-desired transfers.
>>
>> Then I have another one: is the 'actual_length' field really
>> insufficient? For instance, is it non-kosher for a spi_master to just
>> cutoff the message at (for instance) 64K, and expect the protocol
>> driver to handle that (e.g., with Michal's patch from [1])? And if that
>> is kosher, then is there a good reason for the protocol driver to know
>> the exact maximum for its spi_master?
>>
> It would be sufficient if it's a valid case that spi_master returns 0
> and an actual_length < requested_length as this is some kind of error
> situation.

I had one more look at the SPI core and e.g. spi_write_then_read
calls spi_sync w/o checking actual_length afterwards.
This can mean the discussed case is not valid, however it also could be
simply a bug.

If the discussed case is valid a clear hint to all users of spi_sync and
friends should be added that the caller can not rely on status code 0
only but must check actual_length to verify that the complete message
was transferred.

It would be good to hear Mark's opinion on this.

> I could also fully understand if spi_master doesn't return 0 but
> -EMSGSIZE in such a case.
> And the suggested patch would bail out of the chunk-assembling loop
> once it get's an error from the SPI transfer
> (after applying patch 2 of the series which introduces checking
> the return code of the spi_sync call in m25p80_read).
>
>> [snip example]
>>
>> Brian
>>
>

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
> On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> > It would be sufficient if it's a valid case that spi_master returns 0
> > and an actual_length < requested_length as this is some kind of error
> > situation.

> I had one more look at the SPI core and e.g. spi_write_then_read
> calls spi_sync w/o checking actual_length afterwards.
> This can mean the discussed case is not valid, however it also could be
> simply a bug.

We can't assume that users of spi_write_then_read() will cope with a
restarted transfer - the usual use case is things like register I/O
where restarting a partial transfer wouldn't produce the desired result
so it's just a plain error for users of that interface.  Anything that
is able to cope needs to be using the core API directly.

> If the discussed case is valid a clear hint to all users of spi_sync and
> friends should be added that the caller can not rely on status code 0
> only but must check actual_length to verify that the complete message
> was transferred.

You'll get an error on truncation.  It may be possible to recover.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
> On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> > It would be sufficient if it's a valid case that spi_master returns 0
> > and an actual_length < requested_length as this is some kind of error
> > situation.

> I had one more look at the SPI core and e.g. spi_write_then_read
> calls spi_sync w/o checking actual_length afterwards.
> This can mean the discussed case is not valid, however it also could be
> simply a bug.

We can't assume that users of spi_write_then_read() will cope with a
restarted transfer - the usual use case is things like register I/O
where restarting a partial transfer wouldn't produce the desired result
so it's just a plain error for users of that interface.  Anything that
is able to cope needs to be using the core API directly.

> If the discussed case is valid a clear hint to all users of spi_sync and
> friends should be added that the caller can not rely on status code 0
> only but must check actual_length to verify that the complete message
> was transferred.

You'll get an error on truncation.  It may be possible to recover.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 20.11.2015 um 13:35 schrieb Mark Brown:
> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
>> On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
>>> It would be sufficient if it's a valid case that spi_master returns 0
>>> and an actual_length < requested_length as this is some kind of error
>>> situation.
> 
>> I had one more look at the SPI core and e.g. spi_write_then_read
>> calls spi_sync w/o checking actual_length afterwards.
>> This can mean the discussed case is not valid, however it also could be
>> simply a bug.
> 
> We can't assume that users of spi_write_then_read() will cope with a
> restarted transfer - the usual use case is things like register I/O
> where restarting a partial transfer wouldn't produce the desired result
> so it's just a plain error for users of that interface.  Anything that
> is able to cope needs to be using the core API directly.
> 
>> If the discussed case is valid a clear hint to all users of spi_sync and
>> friends should be added that the caller can not rely on status code 0
>> only but must check actual_length to verify that the complete message
>> was transferred.
> 
> You'll get an error on truncation.  It may be possible to recover.
> 
OK, I interpret this as:
Controller drivers shall return 0 only if the complete message was
transferred successfully.
If  a controller driver returns an error it has the option to set
actual_length to what was transferred successfully.

This means we can't use patch 4 from Michal because it bails out as soon
as the underlying SPI transfer returns an error.

Instead something like the spi-nor patch I sent on Oct 6th would be needed:
[PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read

It loops over nor->read and ignores errors as long as at least something
was read.

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 20.11.2015 um 13:35 schrieb Mark Brown:
> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
>> On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> 
>>> It would be sufficient if it's a valid case that spi_master returns 0
>>> and an actual_length < requested_length as this is some kind of error
>>> situation.
> 
>> I had one more look at the SPI core and e.g. spi_write_then_read
>> calls spi_sync w/o checking actual_length afterwards.
>> This can mean the discussed case is not valid, however it also could be
>> simply a bug.
> 
> We can't assume that users of spi_write_then_read() will cope with a
> restarted transfer - the usual use case is things like register I/O
> where restarting a partial transfer wouldn't produce the desired result
> so it's just a plain error for users of that interface.  Anything that
> is able to cope needs to be using the core API directly.
> 
>> If the discussed case is valid a clear hint to all users of spi_sync and
>> friends should be added that the caller can not rely on status code 0
>> only but must check actual_length to verify that the complete message
>> was transferred.
> 
> You'll get an error on truncation.  It may be possible to recover.
> 
OK, I interpret this as:
Controller drivers shall return 0 only if the complete message was
transferred successfully.
If  a controller driver returns an error it has the option to set
actual_length to what was transferred successfully.

This means we can't use patch 4 from Michal because it bails out as soon
as the underlying SPI transfer returns an error.

Instead something like the spi-nor patch I sent on Oct 6th would be needed:
[PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read

It loops over nor->read and ignores errors as long as at least something
was read.

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

On 20 November 2015 at 19:59, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Am 20.11.2015 um 13:35 schrieb Mark Brown:
>> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
>>> On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>>>> It would be sufficient if it's a valid case that spi_master returns 0
>>>> and an actual_length < requested_length as this is some kind of error
>>>> situation.
>>
>>> I had one more look at the SPI core and e.g. spi_write_then_read
>>> calls spi_sync w/o checking actual_length afterwards.
>>> This can mean the discussed case is not valid, however it also could be
>>> simply a bug.
>>
>> We can't assume that users of spi_write_then_read() will cope with a
>> restarted transfer - the usual use case is things like register I/O
>> where restarting a partial transfer wouldn't produce the desired result
>> so it's just a plain error for users of that interface.  Anything that
>> is able to cope needs to be using the core API directly.
>>
>>> If the discussed case is valid a clear hint to all users of spi_sync and
>>> friends should be added that the caller can not rely on status code 0
>>> only but must check actual_length to verify that the complete message
>>> was transferred.
>>
>> You'll get an error on truncation.  It may be possible to recover.
>>
> OK, I interpret this as:
> Controller drivers shall return 0 only if the complete message was
> transferred successfully.
> If  a controller driver returns an error it has the option to set
> actual_length to what was transferred successfully.
>
> This means we can't use patch 4 from Michal because it bails out as soon
> as the underlying SPI transfer returns an error.
>
> Instead something like the spi-nor patch I sent on Oct 6th would be needed:
> [PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read
>
> It loops over nor->read and ignores errors as long as at least something
> was read.
>

I don't think ignoring errors in general is good idea.

If it's desirable that a partial transfer is reported as error then a
particular error value should be defined for this case and drivers
that can continue the transfer in a driver-specific way (such as
spi-nor) can check for this error and handle it appropriately and pass
through any other error.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

On 20 November 2015 at 19:59, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 20.11.2015 um 13:35 schrieb Mark Brown:
>> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
>>> On Fri, Nov 20, 2015 at 7:59 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>
>>>> It would be sufficient if it's a valid case that spi_master returns 0
>>>> and an actual_length < requested_length as this is some kind of error
>>>> situation.
>>
>>> I had one more look at the SPI core and e.g. spi_write_then_read
>>> calls spi_sync w/o checking actual_length afterwards.
>>> This can mean the discussed case is not valid, however it also could be
>>> simply a bug.
>>
>> We can't assume that users of spi_write_then_read() will cope with a
>> restarted transfer - the usual use case is things like register I/O
>> where restarting a partial transfer wouldn't produce the desired result
>> so it's just a plain error for users of that interface.  Anything that
>> is able to cope needs to be using the core API directly.
>>
>>> If the discussed case is valid a clear hint to all users of spi_sync and
>>> friends should be added that the caller can not rely on status code 0
>>> only but must check actual_length to verify that the complete message
>>> was transferred.
>>
>> You'll get an error on truncation.  It may be possible to recover.
>>
> OK, I interpret this as:
> Controller drivers shall return 0 only if the complete message was
> transferred successfully.
> If  a controller driver returns an error it has the option to set
> actual_length to what was transferred successfully.
>
> This means we can't use patch 4 from Michal because it bails out as soon
> as the underlying SPI transfer returns an error.
>
> Instead something like the spi-nor patch I sent on Oct 6th would be needed:
> [PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read
>
> It loops over nor->read and ignores errors as long as at least something
> was read.
>

I don't think ignoring errors in general is good idea.

If it's desirable that a partial transfer is reported as error then a
particular error value should be defined for this case and drivers
that can continue the transfer in a driver-specific way (such as
spi-nor) can check for this error and handle it appropriately and pass
through any other error.

Thanks

Michal

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:

> I don't think ignoring errors in general is good idea.

> If it's desirable that a partial transfer is reported as error then a
> particular error value should be defined for this case and drivers
> that can continue the transfer in a driver-specific way (such as
> spi-nor) can check for this error and handle it appropriately and pass
> through any other error.

Fundamentally callers just shouldn't be trying to do excessively large
transfers in the first place, this is why we want capability reporting.
*Any* error code could indicate a truncation, we may have truncated due
to some length limitation but it could also be that there was some
hardware problem that occurred mid transfer or something similar.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:

> I don't think ignoring errors in general is good idea.

> If it's desirable that a partial transfer is reported as error then a
> particular error value should be defined for this case and drivers
> that can continue the transfer in a driver-specific way (such as
> spi-nor) can check for this error and handle it appropriately and pass
> through any other error.

Fundamentally callers just shouldn't be trying to do excessively large
transfers in the first place, this is why we want capability reporting.
*Any* error code could indicate a truncation, we may have truncated due
to some length limitation but it could also be that there was some
hardware problem that occurred mid transfer or something similar.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

On 20 November 2015 at 20:21, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:
>
>> I don't think ignoring errors in general is good idea.
>
>> If it's desirable that a partial transfer is reported as error then a
>> particular error value should be defined for this case and drivers
>> that can continue the transfer in a driver-specific way (such as
>> spi-nor) can check for this error and handle it appropriately and pass
>> through any other error.
>
> Fundamentally callers just shouldn't be trying to do excessively large
> transfers in the first place, this is why we want capability reporting.
> *Any* error code could indicate a truncation, we may have truncated due
> to some length limitation but it could also be that there was some
> hardware problem that occurred mid transfer or something similar.

Indeed, and in the case the SPI master driver truncated the message
due to known limitation before it even attempted a transfer it can
assure with as much certainty as is possible with SPI bus that this
much data was transferred, no more and no less. In contrast, a DMA
failure that is detected after the fact can leave the hardware in
unknown state. So I see a difference here.

And yes, for some driver protocols transferring less data than was
initially requested is not acceptable. For example, if you transferred
Ethernet frames to an Ethernet controller the frames must be
transferred fully.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

On 20 November 2015 at 20:21, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:
>
>> I don't think ignoring errors in general is good idea.
>
>> If it's desirable that a partial transfer is reported as error then a
>> particular error value should be defined for this case and drivers
>> that can continue the transfer in a driver-specific way (such as
>> spi-nor) can check for this error and handle it appropriately and pass
>> through any other error.
>
> Fundamentally callers just shouldn't be trying to do excessively large
> transfers in the first place, this is why we want capability reporting.
> *Any* error code could indicate a truncation, we may have truncated due
> to some length limitation but it could also be that there was some
> hardware problem that occurred mid transfer or something similar.

Indeed, and in the case the SPI master driver truncated the message
due to known limitation before it even attempted a transfer it can
assure with as much certainty as is possible with SPI bus that this
much data was transferred, no more and no less. In contrast, a DMA
failure that is detected after the fact can leave the hardware in
unknown state. So I see a difference here.

And yes, for some driver protocols transferring less data than was
initially requested is not acceptable. For example, if you transferred
Ethernet frames to an Ethernet controller the frames must be
transferred fully.

Thanks

Michal

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:
> On 20 November 2015 at 19:59, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Am 20.11.2015 um 13:35 schrieb Mark Brown:
> >> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
> >>> If the discussed case is valid a clear hint to all users of spi_sync and
> >>> friends should be added that the caller can not rely on status code 0
> >>> only but must check actual_length to verify that the complete message
> >>> was transferred.
> >>
> >> You'll get an error on truncation.  It may be possible to recover.
> >>
> > OK, I interpret this as:
> > Controller drivers shall return 0 only if the complete message was
> > transferred successfully.
> > If  a controller driver returns an error it has the option to set
> > actual_length to what was transferred successfully.
> >
> > This means we can't use patch 4 from Michal because it bails out as soon
> > as the underlying SPI transfer returns an error.

Right (although you meant patch 7).

> > Instead something like the spi-nor patch I sent on Oct 6th would be needed:
> > [PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read

I don't think your patch is good either...

> > It loops over nor->read and ignores errors as long as at least something
> > was read.
> >
> 
> I don't think ignoring errors in general is good idea.

...for this reason, at least.

> If it's desirable that a partial transfer is reported as error then a
> particular error value should be defined for this case and drivers
> that can continue the transfer in a driver-specific way (such as
> spi-nor) can check for this error and handle it appropriately and pass
> through any other error.

Based on Mark's further comments (and my own intuition), I'd rather not
try to interpret different error codes to mean "truncated but keep
going" vs. "truncated for other reason, stop now", unless we really have
to.

I think if we do what Heiner was proposing from the beginning -- expose
a reasonable max SPI message length -- then I think we'll cover the bulk
of what we want. SPI NOR drivers can then try "small enough" transfers,
and if we see any errors, those are unexpected, and we abort.

Sound OK?

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:
> On 20 November 2015 at 19:59, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> > Am 20.11.2015 um 13:35 schrieb Mark Brown:
> >> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
> >>> If the discussed case is valid a clear hint to all users of spi_sync and
> >>> friends should be added that the caller can not rely on status code 0
> >>> only but must check actual_length to verify that the complete message
> >>> was transferred.
> >>
> >> You'll get an error on truncation.  It may be possible to recover.
> >>
> > OK, I interpret this as:
> > Controller drivers shall return 0 only if the complete message was
> > transferred successfully.
> > If  a controller driver returns an error it has the option to set
> > actual_length to what was transferred successfully.
> >
> > This means we can't use patch 4 from Michal because it bails out as soon
> > as the underlying SPI transfer returns an error.

Right (although you meant patch 7).

> > Instead something like the spi-nor patch I sent on Oct 6th would be needed:
> > [PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read

I don't think your patch is good either...

> > It loops over nor->read and ignores errors as long as at least something
> > was read.
> >
> 
> I don't think ignoring errors in general is good idea.

...for this reason, at least.

> If it's desirable that a partial transfer is reported as error then a
> particular error value should be defined for this case and drivers
> that can continue the transfer in a driver-specific way (such as
> spi-nor) can check for this error and handle it appropriately and pass
> through any other error.

Based on Mark's further comments (and my own intuition), I'd rather not
try to interpret different error codes to mean "truncated but keep
going" vs. "truncated for other reason, stop now", unless we really have
to.

I think if we do what Heiner was proposing from the beginning -- expose
a reasonable max SPI message length -- then I think we'll cover the bulk
of what we want. SPI NOR drivers can then try "small enough" transfers,
and if we see any errors, those are unexpected, and we abort.

Sound OK?

Brian

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 21.11.2015 um 00:22 schrieb Brian Norris:
> On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:
>> On 20 November 2015 at 19:59, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>> Am 20.11.2015 um 13:35 schrieb Mark Brown:
>>>> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
>>>>> If the discussed case is valid a clear hint to all users of spi_sync and
>>>>> friends should be added that the caller can not rely on status code 0
>>>>> only but must check actual_length to verify that the complete message
>>>>> was transferred.
>>>>
>>>> You'll get an error on truncation.  It may be possible to recover.
>>>>
>>> OK, I interpret this as:
>>> Controller drivers shall return 0 only if the complete message was
>>> transferred successfully.
>>> If  a controller driver returns an error it has the option to set
>>> actual_length to what was transferred successfully.
>>>
>>> This means we can't use patch 4 from Michal because it bails out as soon
>>> as the underlying SPI transfer returns an error.
> 
> Right (although you meant patch 7).
> 
>>> Instead something like the spi-nor patch I sent on Oct 6th would be needed:
>>> [PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read
> 
> I don't think your patch is good either...
> 
>>> It loops over nor->read and ignores errors as long as at least something
>>> was read.
>>>
>>
>> I don't think ignoring errors in general is good idea.
> 
> ...for this reason, at least.
> 
>> If it's desirable that a partial transfer is reported as error then a
>> particular error value should be defined for this case and drivers
>> that can continue the transfer in a driver-specific way (such as
>> spi-nor) can check for this error and handle it appropriately and pass
>> through any other error.
> 
> Based on Mark's further comments (and my own intuition), I'd rather not
> try to interpret different error codes to mean "truncated but keep
> going" vs. "truncated for other reason, stop now", unless we really have
> to.
> 
> I think if we do what Heiner was proposing from the beginning -- expose
> a reasonable max SPI message length -- then I think we'll cover the bulk
> of what we want. SPI NOR drivers can then try "small enough" transfers,
> and if we see any errors, those are unexpected, and we abort.

Based on what was discussed so far I'll submit a patch series as basis
for further discussion.

Heiner
> 
> Sound OK?
> 
> Brian
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 21.11.2015 um 00:22 schrieb Brian Norris:
> On Fri, Nov 20, 2015 at 08:05:48PM +0100, Michal Suchanek wrote:
>> On 20 November 2015 at 19:59, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>> Am 20.11.2015 um 13:35 schrieb Mark Brown:
>>>> On Fri, Nov 20, 2015 at 11:06:47AM +0100, Heiner Kallweit wrote:
>>>>> If the discussed case is valid a clear hint to all users of spi_sync and
>>>>> friends should be added that the caller can not rely on status code 0
>>>>> only but must check actual_length to verify that the complete message
>>>>> was transferred.
>>>>
>>>> You'll get an error on truncation.  It may be possible to recover.
>>>>
>>> OK, I interpret this as:
>>> Controller drivers shall return 0 only if the complete message was
>>> transferred successfully.
>>> If  a controller driver returns an error it has the option to set
>>> actual_length to what was transferred successfully.
>>>
>>> This means we can't use patch 4 from Michal because it bails out as soon
>>> as the underlying SPI transfer returns an error.
> 
> Right (although you meant patch 7).
> 
>>> Instead something like the spi-nor patch I sent on Oct 6th would be needed:
>>> [PATCH] mtd: spi-nor: handle controller driver limitations in spi_nor_read
> 
> I don't think your patch is good either...
> 
>>> It loops over nor->read and ignores errors as long as at least something
>>> was read.
>>>
>>
>> I don't think ignoring errors in general is good idea.
> 
> ...for this reason, at least.
> 
>> If it's desirable that a partial transfer is reported as error then a
>> particular error value should be defined for this case and drivers
>> that can continue the transfer in a driver-specific way (such as
>> spi-nor) can check for this error and handle it appropriately and pass
>> through any other error.
> 
> Based on Mark's further comments (and my own intuition), I'd rather not
> try to interpret different error codes to mean "truncated but keep
> going" vs. "truncated for other reason, stop now", unless we really have
> to.
> 
> I think if we do what Heiner was proposing from the beginning -- expose
> a reasonable max SPI message length -- then I think we'll cover the bulk
> of what we want. SPI NOR drivers can then try "small enough" transfers,
> and if we see any errors, those are unexpected, and we abort.

Based on what was discussed so far I'll submit a patch series as basis
for further discussion.

Heiner
> 
> Sound OK?
> 
> Brian
> 

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 07:59:37PM +0100, Heiner Kallweit wrote:

> OK, I interpret this as:
> Controller drivers shall return 0 only if the complete message was
> transferred successfully.

Yes.

> If  a controller driver returns an error it has the option to set
> actual_length to what was transferred successfully.

It should always do this.

> This means we can't use patch 4 from Michal because it bails out as soon
> as the underlying SPI transfer returns an error.

I haven't seen that patch.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Fri, Nov 20, 2015 at 07:59:37PM +0100, Heiner Kallweit wrote:

> OK, I interpret this as:
> Controller drivers shall return 0 only if the complete message was
> transferred successfully.

Yes.

> If  a controller driver returns an error it has the option to set
> actual_length to what was transferred successfully.

It should always do this.

> This means we can't use patch 4 from Michal because it bails out as soon
> as the underlying SPI transfer returns an error.

I haven't seen that patch.

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 20.11.2015 um 20:18 schrieb Mark Brown:
> On Fri, Nov 20, 2015 at 07:59:37PM +0100, Heiner Kallweit wrote:
> 
>> OK, I interpret this as:
>> Controller drivers shall return 0 only if the complete message was
>> transferred successfully.
> 
> Yes.
> 
>> If  a controller driver returns an error it has the option to set
>> actual_length to what was transferred successfully.
> 
> It should always do this.
> 
>> This means we can't use patch 4 from Michal because it bails out as soon
>> as the underlying SPI transfer returns an error.
> 
> I haven't seen that patch.
> 
Sorry, this was addressed to Brian. It's about a mtd spi-nor patch.
http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html
Most likely neither you nor linux-spi was on cc.
(Oh, it was patch 7 of the series, not 4.)

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Heiner Kallweit]

Am 20.11.2015 um 20:18 schrieb Mark Brown:
> On Fri, Nov 20, 2015 at 07:59:37PM +0100, Heiner Kallweit wrote:
> 
>> OK, I interpret this as:
>> Controller drivers shall return 0 only if the complete message was
>> transferred successfully.
> 
> Yes.
> 
>> If  a controller driver returns an error it has the option to set
>> actual_length to what was transferred successfully.
> 
> It should always do this.
> 
>> This means we can't use patch 4 from Michal because it bails out as soon
>> as the underlying SPI transfer returns an error.
> 
> I haven't seen that patch.
> 
Sorry, this was addressed to Brian. It's about a mtd spi-nor patch.
http://lists.infradead.org/pipermail/linux-mtd/2015-August/061062.html
Most likely neither you nor linux-spi was on cc.
(Oh, it was patch 7 of the series, not 4.)

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Thu, Nov 19, 2015 at 04:02:26PM -0800, Brian Norris wrote:
> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:

> > I also add an example how a protocol driver could use this extension.
> > Appreciate any comment.

> One question I have: is it necessary to push the handling out into the
> protocol driver? I feel like I've seen a partial answer to this: the
> 'actual_legnth' return field suggests that the protocol driver already
> has to deal with shorter-than-desired transfers.

It should be possible to use actual_length if the hardware doesn't mind
the transfer being curtailed unexpectedly.

> Then I have another one: is the 'actual_length' field really
> insufficient? For instance, is it non-kosher for a spi_master to just
> cutoff the message at (for instance) 64K, and expect the protocol
> driver to handle that (e.g., with Michal's patch from [1])? And if that
> is kosher, then is there a good reason for the protocol driver to know
> the exact maximum for its spi_master?

It really depends if the hardware is able to cope with the error
handling (and ideally the SPI core will be able to transparently split
the transfer up into chunks the controller can cope with anyway).

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Mark Brown]

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

On Thu, Nov 19, 2015 at 04:02:26PM -0800, Brian Norris wrote:
> On Wed, Nov 18, 2015 at 10:19:29PM +0100, Heiner Kallweit wrote:

> > I also add an example how a protocol driver could use this extension.
> > Appreciate any comment.

> One question I have: is it necessary to push the handling out into the
> protocol driver? I feel like I've seen a partial answer to this: the
> 'actual_legnth' return field suggests that the protocol driver already
> has to deal with shorter-than-desired transfers.

It should be possible to use actual_length if the hardware doesn't mind
the transfer being curtailed unexpectedly.

> Then I have another one: is the 'actual_length' field really
> insufficient? For instance, is it non-kosher for a spi_master to just
> cutoff the message at (for instance) 64K, and expect the protocol
> driver to handle that (e.g., with Michal's patch from [1])? And if that
> is kosher, then is there a good reason for the protocol driver to know
> the exact maximum for its spi_master?

It really depends if the hardware is able to cope with the error
handling (and ideally the SPI core will be able to transparently split
the transfer up into chunks the controller can cope with anyway).

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

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

Hello,

On 18 November 2015 at 22:19, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> There have been few discussions in the past about how to handle SPI controller
> limitations like max message length. However they don't seem to have resulted
> in accepted patches yet.
> I also stumbled across this topic because I own a device using Freescale's
> ESPI which has a 64K message size limitation.
>
> At least one agreed fact is that silently assembling chunks in protocol
> drivers is not the preferred approach.
>
> Maybe a better approach would be to introduce a new member of spi_master
> dealing with controller limitations.
> My issue is just the message size limitation but most likely there are more
> and different limitations in other controllers.
>

There are multiple sides to this problem.

The first is the nature of the limitation of the driver. I was dealing
with a driver that can transfer up to 63 bytes because it has 64byte
fifo and the hardware locks up when it's full. The limitation is only
due to the driver cutting way too many corners. There is dmaengine
support available for the platform which has been merged recently so
the driver can use DMA for arbitrarily long transfers. Even without
DMA there is possibility to to drive CS manually and compose multiple
transfers into single logical message or whatever.

Either way, the limit of 63 bytes is very low and would actually cause
problems with many device drivers so it was agreed that fixing the
master one way or another is desirable.

64k limit on the other hand is something more usable from driver
writer standpoint and some banked mmap access to flash memories would
have similar granularity.

I would also like to point out that the limit depends on the transfer
settings. For example, a SPI controller can have no limit on transfer
size but when accessing a flash memory through mmap interface the mmap
window limits the amount of data you can transfer at once. This
particular case may be fixable by moving the mmap window transparently
inside the driver.

Thanks

Michal
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Michal Suchanek]

Hello,

On 18 November 2015 at 22:19, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> There have been few discussions in the past about how to handle SPI controller
> limitations like max message length. However they don't seem to have resulted
> in accepted patches yet.
> I also stumbled across this topic because I own a device using Freescale's
> ESPI which has a 64K message size limitation.
>
> At least one agreed fact is that silently assembling chunks in protocol
> drivers is not the preferred approach.
>
> Maybe a better approach would be to introduce a new member of spi_master
> dealing with controller limitations.
> My issue is just the message size limitation but most likely there are more
> and different limitations in other controllers.
>

There are multiple sides to this problem.

The first is the nature of the limitation of the driver. I was dealing
with a driver that can transfer up to 63 bytes because it has 64byte
fifo and the hardware locks up when it's full. The limitation is only
due to the driver cutting way too many corners. There is dmaengine
support available for the platform which has been merged recently so
the driver can use DMA for arbitrarily long transfers. Even without
DMA there is possibility to to drive CS manually and compose multiple
transfers into single logical message or whatever.

Either way, the limit of 63 bytes is very low and would actually cause
problems with many device drivers so it was agreed that fixing the
master one way or another is desirable.

64k limit on the other hand is something more usable from driver
writer standpoint and some banked mmap access to flash memories would
have similar granularity.

I would also like to point out that the limit depends on the transfer
settings. For example, a SPI controller can have no limit on transfer
size but when accessing a flash memory through mmap interface the mmap
window limits the amount of data you can transfer at once. This
particular case may be fixable by moving the mmap window transparently
inside the driver.

Thanks

Michal

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

Hi Michal,

On Fri, Nov 20, 2015 at 01:56:21PM +0100, Michal Suchanek wrote:
> I was dealing
> with a driver that can transfer up to 63 bytes because it has 64byte
> fifo and the hardware locks up when it's full. The limitation is only
> due to the driver cutting way too many corners. There is dmaengine
> support available for the platform which has been merged recently so
> the driver can use DMA for arbitrarily long transfers. Even without
> DMA there is possibility to to drive CS manually and compose multiple
> transfers into single logical message or whatever.
> 
> Either way, the limit of 63 bytes is very low and would actually cause
> problems with many device drivers so it was agreed that fixing the
> master one way or another is desirable.

Thanks for clarifying what your underlying SPI driver/controller
limitations are. I've probably heard this before, but it's hard to keep
track. This helps me review your MTD/SPI-NOR patches better.

All in all, I believe this is not acceptable for SPI NOR. Even if things
"mostly work" by limiting messages to 64 (or 63) bytes, I don't think
they are good in the long run, since the flash may respond differently
to sub-256-byte writes than to 256+ byte writes. I mentioned some of
this in reply to your other patches, but I think a SPI NOR driver would
just have to reject you if you can't even transfer one flash page of
data.

> 64k limit on the other hand is something more usable from driver
> writer standpoint and some banked mmap access to flash memories would
> have similar granularity.

Right.

> I would also like to point out that the limit depends on the transfer
> settings. For example, a SPI controller can have no limit on transfer
> size but when accessing a flash memory through mmap interface the mmap
> window limits the amount of data you can transfer at once. This
> particular case may be fixable by moving the mmap window transparently
> inside the driver.

Hmm, I'm not sure I have much opinion on that one without having a
non-theoretical case. It seems like it'd be best if the SPI master
driver can work as best as it can to respect a single reasonable "max
mesage size", even if that means choosing the lowest common denominator
of all limitations.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RfC: Handle SPI controller limitations like maximum message length

[Brian Norris]

Hi Michal,

On Fri, Nov 20, 2015 at 01:56:21PM +0100, Michal Suchanek wrote:
> I was dealing
> with a driver that can transfer up to 63 bytes because it has 64byte
> fifo and the hardware locks up when it's full. The limitation is only
> due to the driver cutting way too many corners. There is dmaengine
> support available for the platform which has been merged recently so
> the driver can use DMA for arbitrarily long transfers. Even without
> DMA there is possibility to to drive CS manually and compose multiple
> transfers into single logical message or whatever.
> 
> Either way, the limit of 63 bytes is very low and would actually cause
> problems with many device drivers so it was agreed that fixing the
> master one way or another is desirable.

Thanks for clarifying what your underlying SPI driver/controller
limitations are. I've probably heard this before, but it's hard to keep
track. This helps me review your MTD/SPI-NOR patches better.

All in all, I believe this is not acceptable for SPI NOR. Even if things
"mostly work" by limiting messages to 64 (or 63) bytes, I don't think
they are good in the long run, since the flash may respond differently
to sub-256-byte writes than to 256+ byte writes. I mentioned some of
this in reply to your other patches, but I think a SPI NOR driver would
just have to reject you if you can't even transfer one flash page of
data.

> 64k limit on the other hand is something more usable from driver
> writer standpoint and some banked mmap access to flash memories would
> have similar granularity.

Right.

> I would also like to point out that the limit depends on the transfer
> settings. For example, a SPI controller can have no limit on transfer
> size but when accessing a flash memory through mmap interface the mmap
> window limits the amount of data you can transfer at once. This
> particular case may be fixable by moving the mmap window transparently
> inside the driver.

Hmm, I'm not sure I have much opinion on that one without having a
non-theoretical case. It seems like it'd be best if the SPI master
driver can work as best as it can to respect a single reasonable "max
mesage size", even if that means choosing the lowest common denominator
of all limitations.

Brian