RfC: Handle SPI controller limitations like maximum message length
Subsystems (2)
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.
include/linux/spi/spi.h +15 -0
@@ -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;
Signature · 8 lines
2.6.2 --- drivers/mtd/devices/m25p80.c | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-)
drivers/mtd/devices/m25p80.c +37 -5
53 lines not on this page · raw message
struct m25p *flash = nor->priv;
Signature · 7 lines
2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 3 lines · 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.
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.
Quoted 2 lines
Maybe a better approach would be to introduce a new member of
spi_masterdealing with controller limitations.
That's what we've been doing...
include/linux/spi/spi.h
Quoted 6 lines
I also add an example how a protocol driver could use this extension. Appreciate any comment.
include/linux/spi/spi.h +0
-0
Please don't send patches without a signoff, see SubmittingPatches.
Am 18.11.2015 um 22:57 schrieb Mark Brown:
Quoted 9 lines · Heiner Kallweit
Quoted 3 lines
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_masterstruct 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.
Quoted 5 lines
Quoted 2 lines
Maybe a better approach would be to introduce a new member of
spi_masterdealing 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.
Quoted 9 lines
include/linux/spi/spi.h
Quoted 6 lines
I also add an example how a protocol driver could use this extension. Appreciate any comment.
include/linux/spi/spi.h +0
-0Please 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 7 lines · Heiner Kallweit
Am 18.11.2015 um 22:57 schrieb Mark Brown:
Quoted 5 lines · Heiner Kallweit
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
No, they've resulted in people writing code. We've got a bunch of things in the
spi_masterstruct 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.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Maybe a better approach would be to introduce a new member of
spi_masterdealing 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.
Quoted 3 lines
In case there should be more need to reflect such limitations in
spi_masterit might be good to encapsulate them in a separate struct instead of adding more such members tospi_masterdirectly.
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.
Quoted 34 lines · Mark Brown
Quoted 7 lines · Heiner Kallweit
Am 18.11.2015 um 22:57 schrieb Mark Brown:
Quoted 5 lines · Heiner Kallweit
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
No, they've resulted in people writing code. We've got a bunch of things in the
spi_masterstruct 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.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Maybe a better approach would be to introduce a new member of
spi_masterdealing 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.
Quoted 7 lines
Quoted 3 lines
In case there should be more need to reflect such limitations in
spi_masterit might be good to encapsulate them in a separate struct instead of adding more such members tospi_masterdirectly.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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerOn 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.
Quoted 2 lines
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.
Quoted 7 lines
I am in the progress of writing a (minimal) spi-core extension, that would allow a
spi_masterto have aprepare_messagefunction that would call some “message translation functions”.The ones I have currently come up with are:
intspi_split_transfer_alignment(struct spi_message *msg, int alignment); intspi_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.
Quoted 4 lines
I guess this is a more transparent approach that does not require the individual device drivers to query the
spi_masterabout 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.
Quoted 4 lines · Mark Brown
Quoted 2 lines · Martin Sperl
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.
Quoted 8 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 8 lines · Brian Norris
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 aboutspi_messagelimitations, 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.
Quoted 14 lines · Mark Brown
Quoted 8 lines · Brian Norris
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 aboutspi_messagelimitations, then this would be more exposed to the protocol driver.
Quoted 6 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · Mark Brown
Quoted 8 lines
I am in the progress of writing a (minimal) spi-core extension, that would allow a
spi_masterto have aprepare_messagefunction that would call some “message translation functions”.The ones I have currently come up with are:
intspi_split_transfer_alignment(struct spi_message *msg, int alignment); intspi_split_transfer_maxsize(struct spi_message *msg, size_t max_size);
Quoted 6 lines
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...
Quoted 9 lines
Quoted 4 lines
I guess this is a more transparent approach that does not require the individual device drivers to query the
spi_masterabout 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 8 lines · Martin Sperl
Quoted 7 lines · Mark Brown
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.
Quoted 5 lines
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.
Quoted 4 lines
Also the bus master driver knows what its limitations are, so putting them inside
prepare_messageseems reasonable to me - that is unless you really havespi_devicedrivers 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.
Quoted 8 lines
Quoted 4 lines
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_driverwanting 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.
Quoted 14 lines · Mark Brown
Quoted 9 lines · Martin Sperl
Quoted 7 lines · Mark Brown
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 10 lines
Quoted 4 lines
Also the bus master driver knows what its limitations are, so putting them inside
prepare_messageseems reasonable to me - that is unless you really havespi_devicedrivers 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.
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
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.
Quoted 7 lines
Quoted 3 lines
I wonder if such a variant-construct does not create more headaches in the long run as each
spi_driverwanting 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 9 lines · Martin Sperl
Quoted 5 lines
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.
Quoted 15 lines
Quoted 7 lines
Quoted 3 lines
I wonder if such a variant-construct does not create more headaches in the long run as each
spi_driverwanting 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
Quoted 3 lines
All the other possible constraints parameters should be opaque to a
spi_devicedriver, so I would not want to include those inside thespi_masterstructure, 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.
Quoted 3 lines
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?
Am 21.11.2015 um 14:49 schrieb Mark Brown:
Quoted 34 lines · Martin Sperl
Quoted 10 lines
Quoted 6 lines
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.
Quoted 15 lines
Quoted 7 lines
Quoted 3 lines
I wonder if such a variant-construct does not create more headaches in the long run as each
spi_driverwanting 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.lengthQuoted 3 lines
All the other possible constraints parameters should be opaque to a
spi_devicedriver, so I would not want to include those inside thespi_masterstructure, as _then_ these get used.
Quoted 6 lines
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?
Quoted 6 lines
Quoted 3 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Brian Norris
Quoted 5 lines · Michal Suchanek
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.
Quoted 6 lines
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.
Quoted 13 lines · Brian Norris
Quoted 3 lines
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.
Quoted 6 lines
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.
Quoted 5 lines
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.
Quoted 41 lines · Heiner Kallweit
Am 21.11.2015 um 14:49 schrieb Mark Brown:
Quoted 34 lines · Martin Sperl
Quoted 10 lines
Quoted 6 lines
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.
Quoted 15 lines
Quoted 7 lines
Quoted 3 lines
I wonder if such a variant-construct does not create more headaches in the long run as each
spi_driverwanting 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.lengthQuoted 3 lines
All the other possible constraints parameters should be opaque to a
spi_devicedriver, so I would not want to include those inside thespi_masterstructure, as _then_ these get used.Quoted 6 lines
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.
Quoted 9 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
cover spi: mtd: Handle HW message length restrictions
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(-)
Signature · 7 lines
2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/3 spi: core: add max_msg_size to spi_master
In reply to Michal SuchanekAdd a member to spi_master allowing to better handle SPI chips with a message size HW limit.
--- include/linux/spi/spi.h | 4 ++++ 1 file changed, 4 insertions(+)
include/linux/spi/spi.h +4 -0
@@ -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 */
Signature · 7 lines
2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH 1/3] spi: core: add max_msg_size to spi_master
Quoted 2 lines · Heiner Kallweit
Add a member to
spi_masterallowing 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.
Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master
Am 22.11.2015 um 14:16 schrieb Mark Brown:
Quoted 9 lines · Heiner Kallweit
Quoted 2 lines
Add a member to
spi_masterallowing 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".
Quoted 5 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH 1/3] spi: core: add max_msg_size to spi_master
Quoted 9 lines · Heiner Kallweit
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.
Quoted 8 lines
Quoted 4 lines
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_masterso that each controller driver can provide its own implementation of e.g. something likeget_max_msg_size()?
No, for the clients so they aren't peering at the struct.
Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master
Am 23.11.2015 um 12:38 schrieb Mark Brown:
Quoted 11 lines · Heiner Kallweit
Quoted 9 lines
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".
Quoted 6 lines
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.
Quoted 11 lines
Quoted 8 lines
Quoted 4 lines
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_masterso that each controller driver can provide its own implementation of e.g. something likeget_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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH 1/3] spi: core: add max_msg_size to spi_master
Quoted 10 lines · Heiner Kallweit
Am 23.11.2015 um 12:38 schrieb Mark Brown:
Quoted 1 line
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_tspi_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.
Re: [PATCH 1/3] spi: core: add max_msg_size to spi_master
Am 30.11.2015 um 17:42 schrieb Mark Brown:
Quoted 7 lines · Heiner Kallweit
Quoted 5 lines
Am 23.11.2015 um 12:38 schrieb Mark Brown:
Quoted 1 line
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?
Quoted 8 lines
Quoted 4 lines
size_tspi_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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/3 mtd: m25p80: handle HW message size restrictions
In reply to Michal SuchanekExtend m25p80_read allowing to read in chunks in case the SPI HW has a max supported message size.
--- drivers/mtd/devices/m25p80.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)
drivers/mtd/devices/m25p80.c +34 -5
@@ -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).
Signature · 7 lines
2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] mtd: m25p80: handle HW message size restrictions
Hello,
Quoted 2 lines · Heiner Kallweit
Extend
m25p80_readallowing 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 [email protected] 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
In reply to Michal SuchanekPropagate the 64K message size limitation allowing client drivers to deal properly with this limitation.
--- drivers/spi/spi-fsl-espi.c | 1 + 1 file changed, 1 insertion(+)
drivers/spi/spi-fsl-espi.c +1 -0
@@ -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);
Signature · 7 lines
2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/2 spi: core: add max_msg_size to spi_master
In reply to Michal SuchanekAdd a member to spi_master allowing to better handle SPI chips with a message size HW limit.
---
v2: add accessor
include/linux/spi/spi.h | 9 +++++++++ 1 file changed, 9 insertions(+)
include/linux/spi/spi.h +9 -0
@@ -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);
Signature · 6 lines
2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/2 spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions
In reply to Michal SuchanekPropagate the 64K message size limitation allowing client drivers to deal properly with this limitation.
--- drivers/spi/spi-fsl-espi.c | 1 + 1 file changed, 1 insertion(+)
drivers/spi/spi-fsl-espi.c +1 -0
@@ -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);
Signature · 7 lines
2.6.2 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerRe: [PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions
Quoted 2 lines · Heiner Kallweit
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).
Re: [PATCH resubmit 2/2] spi: fsl-espi: make use of max_msg_size in spi_master to handle HW restrictions
Am 01.12.2015 um 15:19 schrieb Mark Brown:
Quoted 15 lines · Heiner Kallweit
Quoted 2 lines
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
Quoted 5 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 4 lines · Heiner Kallweit
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.
Quoted 8 lines · 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.
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
Quoted 5 lines
Maybe a better approach would be to introduce a new member of
spi_masterdealing with controller limitations.
My issue is just the message size limitation but most likely there are more and different limitations in other controllers.
Quoted 8 lines
I'd introduce a struct
spi_controller_restrictionsand add a member tospi_masterpointing 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.
Quoted 2 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 20.11.2015 um 01:02 schrieb Brian Norris:
Quoted 25 lines · Heiner Kallweit
Quoted 8 lines
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_lengthinforms thespi_norlayer 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.htmlQuoted 8 lines
Maybe a better approach would be to introduce a new member of
spi_masterdealing 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_restrictionsand add a member tospi_masterpointing to such a struct. Then a controller driver could do something like this:
Quoted 9 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 2 lines
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.
Quoted 7 lines
Then I have another one: is the '
actual_length' field really insufficient? For instance, is it non-kosher for aspi_masterto 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 itsspi_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).
Quoted 4 lines
[snip example]
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 52 lines · Heiner Kallweit
Am 20.11.2015 um 01:02 schrieb Brian Norris:
Quoted 25 lines · Heiner Kallweit
Quoted 8 lines
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_lengthinforms thespi_norlayer 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.htmlQuoted 8 lines
Maybe a better approach would be to introduce a new member of
spi_masterdealing 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_restrictionsand add a member tospi_masterpointing to such a struct. Then a controller driver could do something like this:Quoted 9 lines
Quoted 5 lines
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.
Quoted 8 lines
Quoted 2 lines
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.Quoted 7 lines
Then I have another one: is the '
actual_length' field really insufficient? For instance, is it non-kosher for aspi_masterto 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 itsspi_master?
Quoted 3 lines
It would be sufficient if it's a valid case that
spi_masterreturns 0 and anactual_length<requested_lengthas 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.
Quoted 12 lines
I could also fully understand if
spi_masterdoesn't return 0 but-EMSGSIZEin 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 thespi_synccall inm25p80_read).Quoted 4 lines
[snip example]
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 10 lines · Heiner Kallweit
Quoted 3 lines · Heiner Kallweit
It would be sufficient if it's a valid case that
spi_masterreturns 0 and anactual_length<requested_lengthas this is some kind of error situation.I had one more look at the SPI core and e.g.
spi_write_then_readcallsspi_syncw/o checkingactual_lengthafterwards.
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.
Quoted 4 lines
If the discussed case is valid a clear hint to all users of
spi_syncand friends should be added that the caller can not rely on status code 0 only but must checkactual_lengthto verify that the complete message was transferred.
You'll get an error on truncation. It may be possible to recover.
Am 20.11.2015 um 13:35 schrieb Mark Brown:
Quoted 18 lines · Heiner Kallweit
Quoted 10 lines · Heiner Kallweit
Quoted 4 lines
It would be sufficient if it's a valid case that
spi_masterreturns 0 and anactual_length<requested_lengthas this is some kind of error situation.I had one more look at the SPI core and e.g.
spi_write_then_readcallsspi_syncw/o checkingactual_lengthafterwards.
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.
Quoted 7 lines
Quoted 4 lines
If the discussed case is valid a clear hint to all users of
spi_syncand friends should be added that the caller can not rely on status code 0 only but must checkactual_lengthto 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 32 lines · Heiner Kallweit
Am 20.11.2015 um 13:35 schrieb Mark Brown:
Quoted 18 lines · Heiner Kallweit
Quoted 10 lines · Heiner Kallweit
Quoted 4 lines
It would be sufficient if it's a valid case that
spi_masterreturns 0 and anactual_length<requested_lengthas this is some kind of error situation.I had one more look at the SPI core and e.g.
spi_write_then_readcallsspi_syncw/o checkingactual_lengthafterwards.
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.Quoted 7 lines
Quoted 4 lines
If the discussed case is valid a clear hint to all users of
spi_syncand friends should be added that the caller can not rely on status code 0 only but must checkactual_lengthto 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 setactual_lengthto what was transferred successfully.
Quoted 9 lines
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 inspi_nor_readIt loops over
nor->readand 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 7 lines · Michal Suchanek
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.
Quoted 10 lines · Mark Brown
Quoted 7 lines · Michal Suchanek
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.
Quoted 5 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 10 lines · Michal Suchanek
Quoted 9 lines · Heiner Kallweit
Am 20.11.2015 um 13:35 schrieb Mark Brown:
Quoted 8 lines · Heiner Kallweit
Quoted 4 lines
If the discussed case is valid a clear hint to all users of
spi_syncand friends should be added that the caller can not rely on status code 0 only but must checkactual_lengthto verify that the complete message was transferred.You'll get an error on truncation. It may be possible to recover.
Quoted 8 lines
Quoted 8 lines
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 setactual_lengthto 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).
Quoted 2 lines
Quoted 2 lines
Instead something like the spi-nor patch I sent on Oct 6th would be needed:
[PATCH] mtd: spi-nor: handle controller driver limitations inspi_nor_read
I don't think your patch is good either...
Quoted 5 lines
Quoted 3 lines
It loops over
nor->readand 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.
Quoted 5 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 21.11.2015 um 00:22 schrieb Brian Norris:
Quoted 46 lines · Michal Suchanek
Quoted 10 lines · Heiner Kallweit
Quoted 9 lines
Am 20.11.2015 um 13:35 schrieb Mark Brown:
Quoted 8 lines · Heiner Kallweit
Quoted 4 lines
If the discussed case is valid a clear hint to all users of
spi_syncand friends should be added that the caller can not rely on status code 0 only but must checkactual_lengthto verify that the complete message was transferred.You'll get an error on truncation. It may be possible to recover.
Quoted 8 lines
Quoted 8 lines
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 setactual_lengthto 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).
Quoted 2 lines
Quoted 2 lines
Instead something like the spi-nor patch I sent on Oct 6th would be needed:
[PATCH] mtd: spi-nor: handle controller driver limitations inspi_nor_readI don't think your patch is good either...
Quoted 5 lines
Quoted 3 lines
It loops over
nor->readand 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.
Quoted 5 lines
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.
Quoted 4 lines
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
Quoted 5 lines
Sound OK?
Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 3 lines · Heiner Kallweit
OK, I interpret this as:
Controller drivers shall return 0 only if the complete message was transferred successfully.
Yes.
Quoted 2 lines
If a controller driver returns an error it has the option to set
actual_lengthto what was transferred successfully.
It should always do this.
Quoted 2 lines
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.
Am 20.11.2015 um 20:18 schrieb Mark Brown:
Quoted 11 lines · Heiner Kallweit
Quoted 4 lines
OK, I interpret this as:
Controller drivers shall return 0 only if the complete message was transferred successfully.Yes.
Quoted 2 lines
If a controller driver returns an error it has the option to set
actual_lengthto what was transferred successfully.
Quoted 7 lines
It should always do this.
Quoted 2 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 9 lines · Brian Norris
Quoted 2 lines · Heiner Kallweit
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.
Quoted 6 lines
Then I have another one: is the '
actual_length' field really insufficient? For instance, is it non-kosher for aspi_masterto 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 itsspi_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).
Hello,
Quoted 6 lines · 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.
Quoted 8 lines
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_masterdealing 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Michal,
Quoted 9 lines · Michal Suchanek
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.
Quoted 3 lines
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.
Quoted 3 lines
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.
Quoted 6 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html