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

[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

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

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

[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

[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

[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

[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

[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

[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

[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

[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