Behavior of CS signal in combination of spi-cs-high and gpio descriptors
is not clearly defined and documented. So clarify the documentation
Cc: [email protected]
Cc: [email protected]
Signed-off-by: H. Nikolaus Schaller <[email protected]>
---
.../bindings/spi/spi-controller.yaml | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 1b56d5e40f1fc..5f505810104dd 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -42,6 +42,33 @@ properties:
cs2 : &gpio1 1 0
cs3 : &gpio1 2 0
+ The second flag of a gpio descriptor can be GPIO_ACTIVE_HIGH (0)
+ or GPIO_ACTIVE_LOW(1). Legacy device trees often use 0.
+
+ There is a special rule set for combining the second flag of an
+ cs-gpio with the optional spi-cs-high flag for SPI slaves.
+
+ Each table entry defines how the CS pin is to be physically
+ driven (not considering potential gpio inversions by pinmux):
+
+ device node | cs-gpio | CS pin state active | Note
+ ================+===============+=====================+=====
+ spi-cs-high | - | H |
+ - | - | L |
+ spi-cs-high | ACTIVE_HIGH | H |
+ - | ACTIVE_HIGH | L | 1
+ spi-cs-high | ACTIVE_LOW | H | 2
+ - | ACTIVE_LOW | L |
+
+ Notes:
+ 1) Should print a warning about polarity inversion.
+ Here it would be wise to avoid and define the gpio as
+ ACTIVE_LOW.
+ 2) Should print a warning about polarity inversion
+ because ACTIVE_LOW is overridden by spi-cs-high.
+ Should be generally avoided and be replaced by
+ spi-cs-high + ACTIVE_HIGH.
+
num-cs:
$ref: /schemas/types.yaml#/definitions/uint32
description:
--
2.26.2
On Wed, Dec 9, 2020 at 4:57 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> +
> + device node | cs-gpio | CS pin state active | Note
> + ================+===============+=====================+=====
> + spi-cs-high | - | H |
> + - | - | L |
> + spi-cs-high | ACTIVE_HIGH | H |
> + - | ACTIVE_HIGH | L | 1
> + spi-cs-high | ACTIVE_LOW | H | 2
> + - | ACTIVE_LOW | L |
> +
Doesn't this table simply say:
- specify 'spi-cs-high' for an active-high chip select
- leave out 'spi-cs-high' for an active-low chip select
- the gpio active high/active low consumer flags are ignored
?
If so, then I would simply document it that way.
Simple is beautiful.
> Am 09.12.2020 um 18:36 schrieb Sven Van Asbroeck <[email protected]>:
>
> On Wed, Dec 9, 2020 at 4:57 AM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> +
>> + device node | cs-gpio | CS pin state active | Note
>> + ================+===============+=====================+=====
>> + spi-cs-high | - | H |
>> + - | - | L |
>> + spi-cs-high | ACTIVE_HIGH | H |
>> + - | ACTIVE_HIGH | L | 1
>> + spi-cs-high | ACTIVE_LOW | H | 2
>> + - | ACTIVE_LOW | L |
>> +
>
> Doesn't this table simply say:
> - specify 'spi-cs-high' for an active-high chip select
> - leave out 'spi-cs-high' for an active-low chip select
> - the gpio active high/active low consumer flags are ignored
> ?
Yes it does, but I don't know if it is what we want to have. Linus confirmed
and you also seem to agree. Let's wait for other verdicts.
This is also what made me wonder if that is really intended because then
the whole discussion about the cs-gpio-flags and inversion and the fixes
would not have been needed. The current code and fixes are all about
not ignoring the flags...
And I am sure the code would be much simpler than currently for treating
special cases. Code would simply be: make any spi driver look at the gpio
descriptor and undo any inversion that gpiod_set_value() will do in
gpiod_set_value_nocheck() so that we can really control the physical
state by spi-cs-high instead of the logical gpio activity.
Something like:
static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
{
struct spi_gpio *spi_gpio = spi_to_spi_gpio(spi);
/* set initial clock line level */
if (is_active)
gpiod_set_value_cansleep(spi_gpio->sck, spi->mode & SPI_CPOL);
/* Drive chip select line, if we have one */
if (spi_gpio->cs_gpios) {
struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select];
/* check if gpiod_set_value_nocheck() will invert */
if (test_bit(FLAG_ACTIVE_LOW, &cs->flags)
is_active = !is_active;
/* SPI chip selects are normally active-low */
gpiod_set_value_cansleep(cs, (spi->mode & SPI_CS_HIGH) ? is_active : !is_active);
}
}
There would be no need to detect spi-cs-high etc. in gpio-lib or
elsewhere. Only for printing warnings as suggested by Notes 1 and 2.
>
> If so, then I would simply document it that way.
> Simple is beautiful.
Firstly, I would only think about collapsing the table if we agree that
it is correct. Beauty is IMHO not a reason to declare the table to be
correct.
Secondly, please imagine some reader of a device tree who finds
cs-gpios = <&gpio 7 ACTIVE_LOW>;
spi-cs-high;
Documentation should work well and be helpful especially in such a case.
Otherwise you don't need documentation.
Saying that the gpio flags are ignored would be helpful but a full table
with Notes and recommendations how to resolve is even more helpful and
unambiguous - even if it tells the same.
BR and thanks,
Nikolaus
On Wed, Dec 9, 2020 at 1:16 PM H. Nikolaus Schaller <[email protected]> wrote:
>
> This is also what made me wonder if that is really intended because then
> the whole discussion about the cs-gpio-flags and inversion and the fixes
> would not have been needed. The current code and fixes are all about
> not ignoring the flags...
The inversion you witnessed was a bug caused by spi client drivers that
simply "plow over" the SPI_CS_HIGH mode flag. This includes the panel driver
you're using, see:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c?h=v5.10-rc6#n337
You responded to this inversion bug by inverting your chip select, which made
things work again. Now that the bug is gone, you can indicate the correct
polarity in your devicetree, i.e. add 'spi-cs-high' for an active-high
CS, and leave it out for an active-low CS.
Your panel's CS is active-low, so it should not contain spi-cs-high.
> Secondly, please imagine some reader of a device tree who finds
>
> cs-gpios = <&gpio 7 ACTIVE_LOW>;
> spi-cs-high;
That reader looks at the rules, sees that:
- the ACTIVE_LOW is ignored,
- presence of spi-cs-high means active-high
and concludes this chip-select is active-high.
> Am 09.12.2020 um 20:04 schrieb Sven Van Asbroeck <[email protected]>:
>
> On Wed, Dec 9, 2020 at 1:16 PM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> This is also what made me wonder if that is really intended because then
>> the whole discussion about the cs-gpio-flags and inversion and the fixes
>> would not have been needed. The current code and fixes are all about
>> not ignoring the flags...
>
> The inversion you witnessed was a bug caused by spi client drivers that
The inversion we witnessed came from:
commit 6953c57ab172 "gpio: of: Handle SPI chipselect legacy bindings"
There, I read a verbal description of the table I want to formalize
with this patch, because natural language is not as precise as the language
of logic.
This has nothing to do with driver code, which remained and remains unchanged
for long time.
>
>> Secondly, please imagine some reader of a device tree who finds
>>
>> cs-gpios = <&gpio 7 ACTIVE_LOW>;
>> spi-cs-high;
>
> That reader looks at the rules, sees that:
> - the ACTIVE_LOW is ignored,
> - presence of spi-cs-high means active-high
> and concludes this chip-select is active-high.
This misses information what the reader should do to resolve the
obviously missing beauty of the DT.
a) remove spi-cs-high;
b) change to ACTIVE_HIGH
Both appear valid in first place. But one is preferred. This is
again nowhere documented if you simplify the table.
On Wed, 9 Dec 2020 14:04:26 -0500
Sven Van Asbroeck <[email protected]> wrote:
> On Wed, Dec 9, 2020 at 1:16 PM H. Nikolaus Schaller <[email protected]> wrote:
> >
> > This is also what made me wonder if that is really intended because then
> > the whole discussion about the cs-gpio-flags and inversion and the fixes
> > would not have been needed. The current code and fixes are all about
> > not ignoring the flags...
>
> The inversion you witnessed was a bug caused by spi client drivers that
> simply "plow over" the SPI_CS_HIGH mode flag. This includes the panel driver
> you're using, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c?h=v5.10-rc6#n337
>
ah, it would be set in spi->mode and is cleared by
spi->mode = SPI_MODE_3;
Hmm, but we have
spi-cpol;
spi-cpha;
in devicetree. Why do we need that spi->mode line at all?
Regards,
Andreas
Hi Andreas,
> Am 09.12.2020 um 21:01 schrieb Andreas Kemnade <[email protected]>:
>
> On Wed, 9 Dec 2020 14:04:26 -0500
> Sven Van Asbroeck <[email protected]> wrote:
>
>> On Wed, Dec 9, 2020 at 1:16 PM H. Nikolaus Schaller <[email protected]> wrote:
>>>
>>> This is also what made me wonder if that is really intended because then
>>> the whole discussion about the cs-gpio-flags and inversion and the fixes
>>> would not have been needed. The current code and fixes are all about
>>> not ignoring the flags...
>>
>> The inversion you witnessed was a bug caused by spi client drivers that
>> simply "plow over" the SPI_CS_HIGH mode flag. This includes the panel driver
>> you're using, see:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-tpo-td028ttec1.c?h=v5.10-rc6#n337
>>
> ah, it would be set in spi->mode and is cleared by
>
> spi->mode = SPI_MODE_3;
>
>
> Hmm, but we have
> spi-cpol;
> spi-cpha;
> in devicetree. Why do we need that spi->mode line at all?
Because it is there in almost all or at least many drivers.
But I have tested with
> spi->mode |= SPI_MODE_3;
which should keep the mode intact. Right? That did not work either.
So let's not derail the discussion by moving to the code of some
specific driver. Even if that is wrong it does not solve what
this patch wants to solve.
BR and thanks,
Nikolaus
On Wed, Dec 9, 2020 at 3:08 PM H. Nikolaus Schaller <[email protected]> wrote:
>
> But I have tested with
>
> > spi->mode |= SPI_MODE_3;
>
> which should keep the mode intact. Right? That did not work either.
>
- make sure ("spi: fix client driver breakages when using GPIO descriptors")
is in your tree
- your panel's CS is active-low, so 'spi-cs-high' should be removed from its
devicetree entry. In accordance with the rules as explained in commit
message of 6953c57ab172. Also in accordance with the table you posted
in this patch.
When these two changes in place, your panel should work. I have tested this
by mirroring your setup on my board:
spi5-gpio {
compatible = "spi-gpio";
#address-cells = <0x1>;
#size-cells = <0x0>;
pinctrl-names = "default";
pinctrl-0 = <&...>;
sck-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
miso-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
mosi-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
cs-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
num-chipselects = <1>;
ethernet-switch@0 { /* active low cs */
compatible = "micrel,ksz8795";
spi-max-frequency = <1000000>;
reg = <0>;
};
};
If this does not work for you, then what are we missing?
> Am 09.12.2020 um 22:28 schrieb Sven Van Asbroeck <[email protected]>:
>
> On Wed, Dec 9, 2020 at 3:08 PM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> But I have tested with
>>
>>> spi->mode |= SPI_MODE_3;
>>
>> which should keep the mode intact. Right? That did not work either.
>>
>
> - make sure ("spi: fix client driver breakages when using GPIO descriptors")
> is in your tree
Well, if you remember, the panel did work *before* this patch was in my tree
and I found this patch as the reason of the break...
> - your panel's CS is active-low, so 'spi-cs-high' should be removed from its
> devicetree entry. In accordance with the rules as explained in commit
> message of 6953c57ab172. Also in accordance with the table you posted
> in this patch.
It could not have been different because the table was the result of
experimentally checking all possible combinations...
>
> When these two changes in place, your panel should work. I have tested this
> by mirroring your setup on my board:
>
> spi5-gpio {
> compatible = "spi-gpio";
> #address-cells = <0x1>;
> #size-cells = <0x0>;
> pinctrl-names = "default";
> pinctrl-0 = <&...>;
>
> sck-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
> miso-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
> mosi-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
> cs-gpios = <&gpio... GPIO_ACTIVE_HIGH>;
BTW: exactly this choice is questionable ^^^ if you have an active low CS
and it needs an explanation.
> num-chipselects = <1>;
>
> ethernet-switch@0 { /* active low cs */
> compatible = "micrel,ksz8795";
> spi-max-frequency = <1000000>;
> reg = <0>;
> };
> };
>
> If this does not work for you, then what are we missing?
I am missing that you notice that we are not discussing what I should
do with the panel driver or my device tree. I have these patches laying around
for a while (which exactly do what you try to convince me about - except that
I would apply an GPIO_ACTIVE_LOW). Just not submitted because I want to
have a clear definition agreed on first. For a simple reason: reviewers
of my patch should know what to check for.
In this thread we discuss a patch for the SPI bindings documentation which
is something different. See subject and the file the patch affects.
And I am looking for an ack and merge by maintainers of the affected subsystems
that the table is ok. Nothing else.
Please let's stay on topic and please cooperate.
On Wed, Dec 9, 2020 at 11:01 AM H. Nikolaus Schaller <[email protected]> wrote:
> Behavior of CS signal in combination of spi-cs-high and gpio descriptors
> is not clearly defined and documented. So clarify the documentation
>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: H. Nikolaus Schaller <[email protected]>
This is good because it is helpful to users.
Reviewed-by: Linus Walleij <[email protected]>
In cases like this I would actually consider to write a bit in the
bindings saying "this is inconsistent because we screwed up
so be careful", standard bodies usually try to avoid that kind of
statements because they have all kind of prestige involved
with their work, but we don't so we can just as well admit it.
Yours,
Linus Walleij
On Fri, Dec 11, 2020 at 8:18 AM Mark Brown <[email protected]> wrote:
>
> Yeah, it'd definitely be easier to read and clearer what people should
> actually do.
I think it would be beneficial if this consisted of two very clearly
separated parts:
1. the actual recommended binding - so people writing new
devicetrees know what to do
2. the legacy bindings which "also work", which is important
to know when working with legacy devicetrees
At least, that's what I would want if I put myself in a user's
shoes.
On Wed, 9 Dec 2020 10:57:44 +0100, H. Nikolaus Schaller wrote:
> Behavior of CS signal in combination of spi-cs-high and gpio descriptors
> is not clearly defined and documented. So clarify the documentation
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
Thanks!
[1/1] spi: dt-bindings: clarify CS behavior for spi-cs-high and gpio descriptors
commit: 2fee9583198eb97b5351feda7bd825e0f778385c
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
On Wed, Dec 09, 2020 at 12:36:40PM -0500, Sven Van Asbroeck wrote:
> On Wed, Dec 9, 2020 at 4:57 AM H. Nikolaus Schaller <[email protected]> wrote:
> > + device node | cs-gpio | CS pin state active | Note
> > + ================+===============+=====================+=====
> > + spi-cs-high | - | H |
> > + - | - | L |
> > + spi-cs-high | ACTIVE_HIGH | H |
> > + - | ACTIVE_HIGH | L | 1
> > + spi-cs-high | ACTIVE_LOW | H | 2
> > + - | ACTIVE_LOW | L |
> > +
> Doesn't this table simply say:
> - specify 'spi-cs-high' for an active-high chip select
> - leave out 'spi-cs-high' for an active-low chip select
> - the gpio active high/active low consumer flags are ignored
> ?
It seems to, yes.
> If so, then I would simply document it that way.
> Simple is beautiful.
Yeah, it'd definitely be easier to read and clearer what people should
actually do. As Linus said it'd also be a good idea to explicitly say
that this is not great design or particularly intentional since it could
be pretty confusing for someone trying to understand why the bindings
are the way they are.
I'm going to apply this anyway to make sure we get this documentated but
some incremental improvements along these lines would be good.