On Thu, Feb 10, 2022 at 03:41:41PM +0000, Zhou, Yun wrote:
Please don't top post, reply in line with needed context. This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.
> I am sorry that I can not agree with you.
> If there are multiple messages, and each message only has one xfer,
> and the cs_change of each xfer is 1, during the transmission of the
> messages, the CS will keep active even until at the end. This must be
> unreasonable.
This is not something that most drivers are expected to use, cs_change
should only be being used at all for very unusual hardware and it should
be used even less frequently for the last transfer in a message. It is
fragile and anyone using it really needs to know what they're doing but
the feature is there.
> I can't understand why it have to keep CS active after the
> transmission is completed. Could you please explain this in detail?
The feature predates me working on the SPI stack, the obvious examples
would be a device that doesn't actually use chip select where you want
to avoid all chip select changes or if you need to do some other actions
in the middle of a SPI transaction for some reason (which would need a
bunch of system level considerations to actually be safe/sensible like
making sure you're not sharing the SPI bus).
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.
Hi Brown,
>> If there are multiple messages, and each message only has one xfer,
>> and the cs_change of each xfer is 1, during the transmission of the
>> messages, the CS will keep active even until at the end. This must be
>> unreasonable.
> This is not something that most drivers are expected to use, cs_change
> should only be being used at all for very unusual hardware and it should
> be used even less frequently for the last transfer in a message. It is
> fragile and anyone using it really needs to know what they're doing but
> the feature is there.
Maybe it's not normal to set "cs_change" in the last xfer. However, in
most cases, SPI messages come from user space, and these messages may
come from multiple different applications. We can't make the whole
controller fail to work normally due to an inappropriate message of one
application.
>
>> I can't understand why it have to keep CS active after the
>> transmission is completed. Could you please explain this in detail?
> The feature predates me working on the SPI stack, the obvious examples
> would be a device that doesn't actually use chip select where you want
> to avoid all chip select changes or if you need to do some other actions
> in the middle of a SPI transaction for some reason (which would need a
> bunch of system level considerations to actually be safe/sensible like
> making sure you're not sharing the SPI bus).
At present, if "cs_change" is not set, CS will be changed back to inactive
after the transmission is completed. If "cs_change" is set, CS will not
be changed. This obviously violates the definition of "cs_change".
Therefore, I think my patch does not break the support of "cs_change",
but consolidates the support of "cs_change". It also ensures that the whole
SPI controller will not be abnormal due to a message from userspace.
Regards,
Yun
On Fri, Feb 11, 2022 at 01:01:20AM +0800, Yun Zhou wrote:
> > > If there are multiple messages, and each message only has one xfer,
> > > and the cs_change of each xfer is 1, during the transmission of the
> > > messages, the CS will keep active even until at the end. This must be
> > > unreasonable.
> > This is not something that most drivers are expected to use, cs_change
> > should only be being used at all for very unusual hardware and it should
> > be used even less frequently for the last transfer in a message. It is
> > fragile and anyone using it really needs to know what they're doing but
> > the feature is there.
> Maybe it's not normal to set "cs_change" in the last xfer. However, in
> most cases, SPI messages come from user space, and these messages may
I would question your use of "most" here...
> come from multiple different applications. We can't make the whole
> controller fail to work normally due to an inappropriate message of one
> application.
This is one of the many hazards of using spidev, it is not an especially
safe or robust interface. To the extent that there's an issue here it's
something that should be addressed at the spidev level, though I expect
that there will be some users who want this facility and would want a
way to disable any access controls. I recommend writing device drivers
in kernel.
> > The feature predates me working on the SPI stack, the obvious examples
> > would be a device that doesn't actually use chip select where you want
> > to avoid all chip select changes or if you need to do some other actions
> > in the middle of a SPI transaction for some reason (which would need a
> > bunch of system level considerations to actually be safe/sensible like
> > making sure you're not sharing the SPI bus).
> At present, if "cs_change" is not set, CS will be changed back to inactive
> after the transmission is completed. If "cs_change" is set, CS will not
> be changed. This obviously violates the definition of "cs_change".
No, it is exactly the specified behaviour of cs_change. Please see
spi.h.
Hi Brown,
The focus of our differences is what the role of cs_change is.
I think the direct effect of cs_change is to change CS to inactive.
Here are some comments from spi.h:
1. Line 573, comments of 'transfer' operation in struct spi_controller:
?572????? * + Chipselect stays active during the entire message
?573????? *?? (unless modified by spi_transfer.cs_change != 0).
It means that if cs_change != 0, chipselect will be changed to inactive.
2. Line 820, comments for struct spi_transfer:
?820? * @cs_change: affects chipselect after this transfer completes
?821? * @cs_change_delay: delay between cs deassert and assert when
?822? *????? @cs_change is set and @spi_transfer is not the last in
@spi_message
It says that the chipselect will be changed after this transfer completes.
3. Line 885, also the comments for struct spi_transfer:
?883? * All SPI transfers start with the relevant chipselect active.?
Normally
?884? * it stays selected until after the last transfer in a message.?
Drivers
?885? * can affect the chipselect signal using cs_change.
It means that we can use cs_change to make chipselect not active.
In the whole SPI subsystem, cs_change is written in several places, but
only read
in one place: in function spi_transfer_one_message(), which is the only
place
where cs_change comes into play.
1426???? list_for_each_entry(xfer, &msg->transfers, transfer_list) {
...
1441???????????? ret = ctlr->transfer_one(ctlr, msg->spi, xfer);
...
1484???????? if (xfer->cs_change) {
1485???????????? if (list_is_last(&xfer->transfer_list,
1486????????????????????? &msg->transfers)) {
1487???????????????? keep_cs = true;
1488???????????? } else {
1489???????????????? spi_set_cs(msg->spi, false, false);
1490???????????????? _spi_transfer_cs_change_delay(msg, xfer);
1491???????????????? spi_set_cs(msg->spi, true, false);
1492???????????? }
1493???????? }
1495???????? msg->actual_length += xfer->len;
1496???? }
spi_transfer_one_message() will process one or more transfers in the message
one by one. During this period, if cs_change is true, chipselect will be
changed
to inactive and then be changed back to active(for next transfer),
except the
last transfer as we no need to change chipselect back to active after
the last
transfer. In other words, if cs_change is true, we will make an independent
transmission for each transfer.
I also investigated the role of cs_change in several drivers, the result is
similar, e.g. spi-mpc52xx-psc.c, spi-fsl-spi.c, spi-bcm63xx.c,
spi-mpc52xx.c, etc.
To sum up, there is no indication and no requirement that when cs_change is
true, we need to keep chipselect active.
I hope you can seriously consider my analysis. If what I said is wrong,
please
correct it.
Regards,
Yun
On 2/11/22 1:14 AM, Mark Brown wrote:
> On Fri, Feb 11, 2022 at 01:01:20AM +0800, Yun Zhou wrote:
>
>>>> If there are multiple messages, and each message only has one xfer,
>>>> and the cs_change of each xfer is 1, during the transmission of the
>>>> messages, the CS will keep active even until at the end. This must be
>>>> unreasonable.
>>> This is not something that most drivers are expected to use, cs_change
>>> should only be being used at all for very unusual hardware and it should
>>> be used even less frequently for the last transfer in a message. It is
>>> fragile and anyone using it really needs to know what they're doing but
>>> the feature is there.
>> Maybe it's not normal to set "cs_change" in the last xfer. However, in
>> most cases, SPI messages come from user space, and these messages may
> I would question your use of "most" here...
>
>> come from multiple different applications. We can't make the whole
>> controller fail to work normally due to an inappropriate message of one
>> application.
> This is one of the many hazards of using spidev, it is not an especially
> safe or robust interface. To the extent that there's an issue here it's
> something that should be addressed at the spidev level, though I expect
> that there will be some users who want this facility and would want a
> way to disable any access controls. I recommend writing device drivers
> in kernel.
>
>>> The feature predates me working on the SPI stack, the obvious examples
>>> would be a device that doesn't actually use chip select where you want
>>> to avoid all chip select changes or if you need to do some other actions
>>> in the middle of a SPI transaction for some reason (which would need a
>>> bunch of system level considerations to actually be safe/sensible like
>>> making sure you're not sharing the SPI bus).
>> At present, if "cs_change" is not set, CS will be changed back to inactive
>> after the transmission is completed. If "cs_change" is set, CS will not
>> be changed. This obviously violates the definition of "cs_change".
> No, it is exactly the specified behaviour of cs_change. Please see
> spi.h.
On Mon, Feb 14, 2022 at 08:35:33PM +0800, Yun Zhou wrote:
Please don't top post, reply in line with needed context. This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.
> The focus of our differences is what the role of cs_change is.
> I think the direct effect of cs_change is to change CS to inactive.
During a message. At the end of the message it does the opposite.
> I also investigated the role of cs_change in several drivers, the result is
> similar, e.g. spi-mpc52xx-psc.c, spi-fsl-spi.c, spi-bcm63xx.c,
> spi-mpc52xx.c, etc.
Individual drivers may not respect chip select at all, never mind in
this specific way - in general any driver actively managing chip select
is going to be unable to implement the full semnatics of cs_change,
possibly even most of the semantics of cs_change, due to hardware
limiations. If a driver has full control over chip select it'll
normally be able to implement a set_cs() operation and just use the core
handling.
> To sum up, there is no indication and no requirement that when cs_change is
> true, we need to keep chipselect active.
>
> I hope you can seriously consider my analysis. If what I said is wrong,
> please
> correct it.
* All SPI transfers start with the relevant chipselect active. Normally
* it stays selected until after the last transfer in a message. Drivers
* can affect the chipselect signal using cs_change.
*
* (i) If the transfer isn't the last one in the message, this flag is
* used to make the chipselect briefly go inactive in the middle of the
* message. Toggling chipselect in this way may be needed to terminate
* a chip command, letting a single spi_message perform all of group of
* chip transactions together.
*
* (ii) When the transfer is the last one in the message, the chip may
* stay selected until the next transfer. On multi-device SPI busses
* with nothing blocking messages going to other devices, this is just
* a performance hint; starting a message to another device deselects
* this one. But in other cases, this can be used to ensure correctness.
* Some devices need protocol transactions to be built from a series of
* spi_message submissions, where the content of one message is determined
* by the results of previous messages and where the whole transaction
* ends when the chipselect goes intactive.
especially (ii).