Hi,
starting from v5.10-rc5 the SPI based panel of the GTA04 device is broken. It uses
compatible = "spi-gpio"; [1] i.e. gpio descriptors very indirectly.
Bisect shows that it is commit
766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
The commit description tells about a problematic pattern and indeed the driver is
using it - like ca. 15 other spi based panel drivers in drivers/gpu/drm/panel/
I understood that it wants to fix the spi system to handle that correctly again.
But reverting your patch brings back the display. So it appears as if it does not
fix a breakage, rather breaks a previously working setup.
What should we do?
BR and thanks,
Nikolaus
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/omap3-gta04.dtsi?h=v5.10-rc6#n107
Hi Nikolaus, thank you for reaching out !
On Mon, Nov 30, 2020 at 2:06 PM H. Nikolaus Schaller <[email protected]> wrote:
>
> But reverting your patch brings back the display. So it appears as if it does not
> fix a breakage, rather breaks a previously working setup.
The patch in question fixes an important breakage: before the patch, literally
hundreds of SPI drivers no longer worked - only if the SPI bus master
driver was using gpio descriptors.
We knew that there was a chance that our fix would break something else.
But hopefully "it fixes more than it breaks"
>
> What should we do?
>
Can you try the following patch ?
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index 7ceb0ba27b75..c173d7de73b3 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -208,8 +208,8 @@ static void spi_gpio_chipselect(struct spi_device
*spi, int is_active)
if (spi_gpio->cs_gpios) {
struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select];
- /* SPI chip selects are normally active-low */
- gpiod_set_value_cansleep(cs, (spi->mode & SPI_CS_HIGH)
? is_active : !is_active);
+ /* SPI chip select polarity handled by gpiolib*/
+ gpiod_set_value_cansleep(cs, is_active);
}
}
And probably also:
@@ -226,8 +226,7 @@ static int spi_gpio_setup(struct spi_device *spi)
if (spi_gpio->cs_gpios) {
cs = spi_gpio->cs_gpios[spi->chip_select];
if (!spi->controller_state && cs)
- status = gpiod_direction_output(cs,
- !(spi->mode & SPI_CS_HIGH));
+ status = gpiod_direction_output(cs, false);
}
if (!status)
Hi Sven,
> Am 30.11.2020 um 21:13 schrieb Sven Van Asbroeck <[email protected]>:
>
> Hi Nikolaus, thank you for reaching out !
>
> On Mon, Nov 30, 2020 at 2:06 PM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> But reverting your patch brings back the display. So it appears as if it does not
>> fix a breakage, rather breaks a previously working setup.
>
> The patch in question fixes an important breakage: before the patch, literally
> hundreds of SPI drivers no longer worked - only if the SPI bus master
> driver was using gpio descriptors.
>
> We knew that there was a chance that our fix would break something else.
> But hopefully "it fixes more than it breaks"
Then it should not have been applied to mainline but fully worked out and tested.
>
>>
>> What should we do?
>>
>
> Can you try the following patch ?
Unfortunately it doesn't seem to fix it. And combined with the second proposed fix also not.
BR and thanks,
Nikolaus
my combined change:
diff --git a/drivers/spi/spi-gpio.c b/drivers/spi/spi-gpio.c
index 7ceb0ba27b755c..ec2da62716a279 100644
--- a/drivers/spi/spi-gpio.c
+++ b/drivers/spi/spi-gpio.c
@@ -208,8 +208,8 @@ static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
if (spi_gpio->cs_gpios) {
struct gpio_desc *cs = spi_gpio->cs_gpios[spi->chip_select];
- /* SPI chip selects are normally active-low */
- gpiod_set_value_cansleep(cs, (spi->mode & SPI_CS_HIGH) ? is_active : !is_active);
+ /* SPI chip select polarity handled by gpiolib*/
+ gpiod_set_value_cansleep(cs, is_active);
}
}
@@ -226,8 +226,7 @@ static int spi_gpio_setup(struct spi_device *spi)
if (spi_gpio->cs_gpios) {
cs = spi_gpio->cs_gpios[spi->chip_select];
if (!spi->controller_state && cs)
- status = gpiod_direction_output(cs,
- !(spi->mode & SPI_CS_HIGH));
+ status = gpiod_direction_output(cs, false);
}
if (!status)
On Tue, Dec 01, 2020 at 09:59:43AM +0100, H. Nikolaus Schaller wrote:
> > Am 30.11.2020 um 21:13 schrieb Sven Van Asbroeck <[email protected]>:
> > We knew that there was a chance that our fix would break something else.
> > But hopefully "it fixes more than it breaks"
> Then it should not have been applied to mainline but fully worked out
> and tested.
If you want to see better testing of things in mainline please
contribute to the various kernel testing efforts that are out there,
there's a huge range of systems out there using the kernel and it's
simply not realistic to expect that they'll all be covered, the testing
effort for the kernel is very much a community effort. If there are
things that are particularly important to you the best way to ensure
that they are tested is to provide that testing yourself.
Hi Sven,
> Am 01.12.2020 um 13:41 schrieb Sven Van Asbroeck <[email protected]>:
>
> On Tue, Dec 1, 2020 at 4:04 AM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> Then it should not have been applied to mainline but fully worked out and tested.
Well I only complain because you wrote that you knew that it may
break something else. So it is known to induces a regression.
But I did not know until I got a report from our own testing effort (we
run a vendor kernel and do tests all days over the week).
So I had to bisect what was going on. There could have been 100 reasons.
Took me several hours because I had to look at the display to see if
it is on or off after boot. There was no simple shell command to find out
and let the bisect run over night.
Maybe printing a "please check your spi setup" in spi_setup() with
a comment hinting at your patch would have saved me a lot of time.
>
> That would be a reasonable expectation of a product. But Linux
> isn't a product, it's a hugely complex, shared system, which may
> form the basis of your product. The core maintainers aren't
> superhuman, nor do they have access to the 1000s of configurations
> and devices where Linux runs or will run. They do their very best,
> but if every change had to be 100% tested in every possible
> configuration, then few things could ever change, and Linux
> would slow down to a snail's pace.
> When your product is based on Linux and you pull a newer version
> off kernel.org, it's not unreasonable to expect the occasional
> breakage. In my case, when I moved from 5.7 to 5.9, some of the
> things that broke were my network chip, and most SPI drivers. That
> was a bad day, most pulls are trouble-free.
If it were occasional it would be good.... It is not the first time
that I have to do bisect tests to find what did go wrong upstream.
Even in LTS kernels and some of my fixes were backported later.
> I believe LTSes are more stable than 'stable releases' which are in
> turn more stable than RCs. The choice involves a trade-off between
> features, security and stability.
>
> When you do run into a breakage, complaining on the mailing list
> is good, but posting a fix is better :)
I usually do - if I have the time to go the extra mile to fix something
what somebody else did break. But I need to have a lot of spare time
if I have no idea what made the regression for a device driver where I
know that it was not touched and have to study the details.
And for me it is much less effort (hours vs. seconds) to do a revert...
I could submit that as a quick fix :) But then you are not happy.
> This is my layman's understanding of the situation, I'm just a user
> and not a maintainer.
Me too...
Well, I am sort of maintainer of a vendor kernel that tries to
follow linus/master and fix things before we release an LTS.
That is why I need a solution and can not go back to some LTS or wait
for the next one...
Anyways, there is still time until v5.10.0 to fix it better than by
a revert.
>
>>>
>>>>
>>>> What should we do?
>
> Hopefully I have some time this week to look into your breakage,
> I may get overtaken by someone much more knowledgeable than
> me on spi-gpio.
Hm. Then we are already two of us who have not much knowledge about
spi-gpio...
Hope that you have an idea soon. I am happy to test any suggestions/patches/alternatives
better than a simple revert.
BR and thanks,
Nikolaus
> Am 01.12.2020 um 13:16 schrieb Mark Brown <[email protected]>:
>
> On Tue, Dec 01, 2020 at 09:59:43AM +0100, H. Nikolaus Schaller wrote:
>>> Am 30.11.2020 um 21:13 schrieb Sven Van Asbroeck <[email protected]>:
>
>>> We knew that there was a chance that our fix would break something else.
>>> But hopefully "it fixes more than it breaks"
>
>> Then it should not have been applied to mainline but fully worked out
>> and tested.
>
> If you want to see better testing of things in mainline please
> contribute to the various kernel testing efforts that are out there,
> there's a huge range of systems out there using the kernel and it's
> simply not realistic to expect that they'll all be covered, the testing
> effort for the kernel is very much a community effort. If there are
> things that are particularly important to you the best way to ensure
> that they are tested is to provide that testing yourself.
Well, having a working display of a portable device that has mainline
Linux support for many years is particularly important...
BTW, I am already part of this testing efforts out there. Because I test
almost all -rc when they arrive. So I just did what you propose,
And if I can locate the commit of a regression I write bug reports
like this one.
The only thing I could have done differently is to always test based
on linux-next but that is a less structured testing environment and has
its own pitfalls. But that would have revealed the issue only earlier
but not with less effort or with a quicker fix.
I am not sure if ít is realistic to dream of some way of informing/contacting
the potentially affected driver authors/maintainers to run a quick test
before it is merged upstream.
To be clear: I do not expect that there are no bugs at all (for that I know
Linux and software far too long). But I do not expect regression of the
type hopefully "it fixes more than it breaks".
Well, for me it (apparently) breaks more than it fixes. So the easiest fix
for me would be to revert the patch. But I am sure that then you are not
happy with it...
So let's set out for a better solution.
BR and thanks,
Nikolaus
On Tue, Dec 1, 2020 at 8:36 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> Well I only complain because you wrote that you knew that it may
> break something else. So it is known to induces a regression.
We knew that it would fix an important, common problem, but
we also knew that there is always a possibility of breaking
something else when making a change to the core.
>
> Maybe printing a "please check your spi setup" in spi_setup() with
> a comment hinting at your patch would have saved me a lot of time.
>
You could ask the maintainer for such a policy, but I fear that soon
the code would emit too many "please check" messages.
>
> Well, I am sort of maintainer of a vendor kernel that tries to
> follow linus/master and fix things before we release an LTS.
Makes sense, I understand your situation better now.
>
> Anyways, there is still time until v5.10.0 to fix it better than by
> a revert.
When we find a fix, it'll have a Fixes: tag, which means it'll
automatically be applied to every supported kernel, including
v5.10 even if already released.
>
> Hope that you have an idea soon. I am happy to test any suggestions/patches/alternatives
> better than a simple revert.
>
Thank you, that's great. I may come back with a few suggestions
for you to test this week.
On Tue, Dec 01, 2020 at 09:08:34AM -0500, Sven Van Asbroeck wrote:
> On Tue, Dec 1, 2020 at 8:36 AM H. Nikolaus Schaller <[email protected]> wrote:
> > Well I only complain because you wrote that you knew that it may
> > break something else. So it is known to induces a regression.
> We knew that it would fix an important, common problem, but
> we also knew that there is always a possibility of breaking
> something else when making a change to the core.
It's worth pointing out that this is an exceptionally fragile area of
the code thanks to some regrettable historical decisions with regard to
how GPIOs are managed so this assessment is based on repeated past
experiences with changes that look sensible, fix real problems for real
systems and yet cause problems to crop up elsewhere due to unforseen
interactions elsewhere. Eventually we'll shake out all these issues and
end up with something that's more understandable and hence managable but
clearly we aren't there yet.
Nikolaus,
On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> Let's work on a fix for the fix now.
I tested spi-gpio on my system, by converting a built-in or hardware spi,
to a spi-gpio. Interestingly, the patch has the opposite effect on my system:
before the patch, spi-gpio did not work, but after it's applied, it does work.
Can you tell me the idle status of your chip-select gpio in debugfs?
# mount -t debugfs none /sys/kernel/debug
# cat /sys/kernel/debug/gpio
Look for something like this:
gpiochip0: GPIOs 0-31, parent: platform/209c000.gpio, 209c000.gpio:
gpio-17 ( |spi5 CS0 ) out hi ACTIVE LOW
Also, apply the following patch, and tell me
a) does this dev_err() get called on your system, and
b) what is the value when your chip is de-selected
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 7e8804b02be9..b2f4cf5c9ffb 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -813,11 +813,12 @@ static void spi_set_cs(struct spi_device *spi,
bool enable)
if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
if (!(spi->mode & SPI_NO_CS)) {
- if (spi->cs_gpiod)
+ if (spi->cs_gpiod) {
+ dev_err(&spi->dev, "gpiod %s", enable1
? "enable" : "disable");
/* polarity handled by gpiolib */
gpiod_set_value_cansleep(spi->cs_gpiod,
enable1);
- else
+ } else
/*
* invert the enable line, as active low is
* default for SPI.
Nikolaus,
On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> Let's work on a fix for the fix now.
>
Are you quite sure the chip-select of the tpo,td028ttec1 panel
is active-high? A quick google produced a datasheet which
seems to indicate that XCS is active-low?
See page 17 here:
http://www.lcd-source.com/datasheet/TPO/TD028TTEC1.pdf
It is of course possible that you are driving that line behind
some inverting circuitry. Hardware designers seem to do that
all the time, if they need to go from one voltage domain to
the other, etc.
On Tue, Dec 01, 2020 at 03:20:12PM +0100, Linus Walleij wrote:
> The reason why I shoot in the dark to convert all SPI
> drivers to use GPIO descriptors instead of the global
> GPIO numberspace is detailed in drivers/gpio/TODO
> so I will not repeat it here.
> I don't know if much can be done about it other than
> having better programmers than me at the task. Or
> less tired when they write the patch. etc.
I think the problem here is more to do with where we started than where
we're going or how we got there - things have been glued together or
happened to work in ways that mean I'm not sure we reasonably understand
the situation we started from or all the requirements it has. As you
say I'm not sure anything beyond throwing the API away and starting
afresh would really help here, but that's not really how we tend to do
things for a bunch of very good reasons.
> Am 01.12.2020 um 17:10 schrieb Sven Van Asbroeck <[email protected]>:
>
> Nikolaus,
>
> On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> Let's work on a fix for the fix now.
>>
>
> Are you quite sure the chip-select of the tpo,td028ttec1 panel
> is active-high? A quick google produced a datasheet which
> seems to indicate that XCS is active-low?
>
> See page 17 here:
> http://www.lcd-source.com/datasheet/TPO/TD028TTEC1.pdf
>
> It is of course possible that you are driving that line behind
> some inverting circuitry. Hardware designers seem to do that
> all the time, if they need to go from one voltage domain to
> the other, etc.
You are right. It is active low.
But that is the start of a long story...
It was introduced in DT by c2e138bc8ed80ae
with cs-gpios = <&gpio1 19 0>;
because the third parameter did not have any meaning
back then, AFAIR. It was ignored and drivers did initialize
gpios with inversion if needed.
Missing spi-cs-high; did define active low back then for spi-gpio.
In 3a637e008e542b8ebdc2ceed616b229af0462b14
the "0" was replaced by the constant
cs-gpios = <&gpio1 19 GPIO_ACTIVE_HIGH>;
keeping the DTB unchanged and compatible.
This did work because there was still no spi-cs-high;
Then came 6953c57ab172 which mixed cs-gpios and spi-cs-high;
and made use of the now wrong GPIO_ACTIVE_HIGH;
A special logic converted GPIO_ACTIVE_HIGH to GPIO_ACTIVE_LOW
in case of spi-cs-high; being present.
A long discussion revealed that the only solution to
stay compatible with old and new DT/kernels was to
add spi-cs-high; and keep the wrong <&gpio1 19 GPIO_ACTIVE_HIGH>;
f1f028ff89cb0d3 did "fix" this by adding spi-cs-high to the DT.
Please note that apparently we were already confused about what
the right value should be (ACTIVE_HIGH or ACTIVE_LOW) in 2019 but
the result was working until your new patch appeared.
Yes, it could be that the problem introduced by 6953c57ab172
is just coming up again with your new change.
On the other hand I remember from the old discussion that changing
a DT is "forbidden" if it is not upwards compatible with earlier
kernels. The driver must take what it gets from (legacy) DT.
BR,
Nikolaus
> Am 01.12.2020 um 17:20 schrieb Mark Brown <[email protected]>:
>
> On Tue, Dec 01, 2020 at 03:20:12PM +0100, Linus Walleij wrote:
>
>> The reason why I shoot in the dark to convert all SPI
>> drivers to use GPIO descriptors instead of the global
>> GPIO numberspace is detailed in drivers/gpio/TODO
>> so I will not repeat it here.
>
>> I don't know if much can be done about it other than
>> having better programmers than me at the task. Or
>> less tired when they write the patch. etc.
>
> I think the problem here is more to do with where we started than where
> we're going or how we got there - things have been glued together or
> happened to work in ways that mean I'm not sure we reasonably understand
> the situation we started from or all the requirements it has. As you
> say I'm not sure anything beyond throwing the API away and starting
> afresh would really help here, but that's not really how we tend to do
> things for a bunch of very good reasons.
I think the key problem is GPIO_ACTIVE_HIGH 0 and GPIO_ACTIVE_LOW 1
in device tree blobs. But that is so fundamental that we have to live with it.
So I guess that even a new API from scratch wouldn't improve that.
On Tue, 1 Dec 2020 11:10:49 -0500
Sven Van Asbroeck <[email protected]> wrote:
> Nikolaus,
>
> On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <[email protected]> wrote:
> >
> > Let's work on a fix for the fix now.
> >
>
> Are you quite sure the chip-select of the tpo,td028ttec1 panel
> is active-high? A quick google produced a datasheet which
> seems to indicate that XCS is active-low?
>
Schematics is here:
https://projects.goldelico.com/p/gta04-main/downloads/48/
The display connector P204-LCD indicates some inversion at the XCS and
XRES pins.
This patch fixes things for a boot where the display was not
touched by the bootloader
diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
index c8745bc800f7..003202d12990 100644
--- a/arch/arm/boot/dts/omap3-gta04.dtsi
+++ b/arch/arm/boot/dts/omap3-gta04.dtsi
@@ -124,7 +124,6 @@
spi-max-frequency = <100000>;
spi-cpol;
spi-cpha;
- spi-cs-high;
backlight= <&backlight>;
label = "lcd";
So if that one is really active-low, why did it ever work?!
Regards,
Andreas
> Am 01.12.2020 um 17:44 schrieb Andreas Kemnade <[email protected]>:
>
> On Tue, 1 Dec 2020 11:10:49 -0500
> Sven Van Asbroeck <[email protected]> wrote:
>
>> Nikolaus,
>>
>> On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <[email protected]> wrote:
>>>
>>> Let's work on a fix for the fix now.
>>>
>>
>> Are you quite sure the chip-select of the tpo,td028ttec1 panel
>> is active-high? A quick google produced a datasheet which
>> seems to indicate that XCS is active-low?
>>
> Schematics is here:
> https://projects.goldelico.com/p/gta04-main/downloads/48/
>
> The display connector P204-LCD indicates some inversion at the XCS and
> XRES pins.
>
> This patch fixes things for a boot where the display was not
> touched by the bootloader
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index c8745bc800f7..003202d12990 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -124,7 +124,6 @@
> spi-max-frequency = <100000>;
> spi-cpol;
> spi-cpha;
> - spi-cs-high;
>
> backlight= <&backlight>;
> label = "lcd";
>
> So if that one is really active-low, why did it ever work?!
Apparently, because there was patch f1f028ff89cb0d3 to fix 6953c57ab172 ...
BR,
Nikolaus
> Am 01.12.2020 um 17:44 schrieb Andreas Kemnade <[email protected]>:
>
> On Tue, 1 Dec 2020 11:10:49 -0500
> Sven Van Asbroeck <[email protected]> wrote:
>
>> Nikolaus,
>>
>> On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <[email protected]> wrote:
>>>
>>> Let's work on a fix for the fix now.
>>>
>>
>> Are you quite sure the chip-select of the tpo,td028ttec1 panel
>> is active-high? A quick google produced a datasheet which
>> seems to indicate that XCS is active-low?
>>
> Schematics is here:
> https://projects.goldelico.com/p/gta04-main/downloads/48/
>
> The display connector P204-LCD indicates some inversion at the XCS and
> XRES pins.
>
> This patch fixes things for a boot where the display was not
> touched by the bootloader
> diff --git a/arch/arm/boot/dts/omap3-gta04.dtsi b/arch/arm/boot/dts/omap3-gta04.dtsi
> index c8745bc800f7..003202d12990 100644
> --- a/arch/arm/boot/dts/omap3-gta04.dtsi
> +++ b/arch/arm/boot/dts/omap3-gta04.dtsi
> @@ -124,7 +124,6 @@
> spi-max-frequency = <100000>;
> spi-cpol;
> spi-cpha;
> - spi-cs-high;
BTW: that is what I had planned to try next...
>
> backlight= <&backlight>;
> label = "lcd";
>
> So if that one is really active-low, why did it ever work?!
>
> Regards,
> Andreas
Hi Nikolaus,
On Tue, Dec 1, 2020 at 11:43 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> You are right. It is active low.
>
In that case, we have a very simple solution, just remove the spi-cs-high,
and things will work.
In case of SPI CS gpios, the current kernel ignores all
GPIO_ACTIVE_HIGH/LOW
flags, and uses the presence/absence of spi-cs-high instead, to
determine active high / active low.
Hi Nikolaus,
On Tue, Dec 1, 2020 at 12:13 PM H. Nikolaus Schaller <[email protected]> wrote:
> > Am 01.12.2020 um 17:53 schrieb Sven Van Asbroeck <[email protected]>:
> > On Tue, Dec 1, 2020 at 11:43 AM H. Nikolaus Schaller <[email protected]> wrote:
> >>
> >> You are right. It is active low.
> >>
> >
> > In that case, we have a very simple solution, just remove the spi-cs-high,
> > and things will work.
>
> We originally had it that way and because there was a change in gpiolib we had
> to introduce it.
The current rules re. spi chip-selects in devicetrees are here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib-of.c?h=v5.10-rc6#n191
This is the way I see things:
- according to the current rules, your devicetree describes a spi panel with
an active-high chip select
- the actual chip select of your panel is active-low
- a spi/gpiod bug inverted the chip-select in many instances
- because of this bug, your devicetree happened to work before 766c6b63aa04
- 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
fixes this chip-select polarity bug
- you now need to remove your devicetree work-around for this bug by reverting
f1f028ff89cb0d3
>
> I am not sure if DT maintainers accept that we revert a DT change just to
> handle some change in a driver. Usually they insist on fixing a driver and
> live with the DT. DT is carved in stone or could be ROM...
This is above my paygrade, but I've always assumed that the devicetree ABI
is an in-kernel ABI, i.e. not a userspace ABI. Meaning that it is flexible and
there is no obligation to keep it 100% backwards compatible. Of course Rob
Herring may want to keep it as backwards-compatible as possible, but that's
an altogether different thing from having a userspace-type ABI.
>
> So you could try to submit a revert of f1f028ff89cb0d3 with a description
> why it is needed. And please make sure that it is also applied where your
> patch is backported to stable. So it should have some
>
> Fixes: 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
I have no insight in your devicetrees, and no hardware to test it out either.
As a user of these trees, you are best placed to make the change and test it
out. I invite you to submit a patch (revert of f1f028ff89cb0d3) to the
mailing list.
>
>
> So you mean you are just restoring the behaviour before
>
> 6953c57ab172
>
> was introduced?
>
That was based on my incorrect interpretation of the devicetree spi cs
rules, my apologies. I have linked to the correct rules in the link above.
Hi Nikolaus, Sven,
the fault that the thing broke in the first place is all mine.
Credit where credit is due!
The reason why I shoot in the dark to convert all SPI
drivers to use GPIO descriptors instead of the global
GPIO numberspace is detailed in drivers/gpio/TODO
so I will not repeat it here.
I don't know if much can be done about it other than
having better programmers than me at the task. Or
less tired when they write the patch. etc.
What other operating systems do to get around the same
type of refactoring problem is to aggressively
deprecate and delete code that does not follow the
latest ideas of the driver subsystem developer. This
is not an option on Linux because we don't like to leave
working hardware and users behind so I am painstakingly
fixing it all over the place, with a little help from my
friends. Sometimes it blows up in my face, sometimes
in other people faces too, sorry about that.
Yours,
Linus Walleij
Hi Linus,
On Tue, Dec 1, 2020 at 9:20 AM Linus Walleij <[email protected]> wrote:
>
> I don't know if much can be done about it other than
> having better programmers than me at the task. Or
> less tired when they write the patch. etc.
I don't think that we have many programmers that are
better than you :)
IMHO the fundamental dilemma is between features,
security and agility on the one hand, and stability on
the other. Too much emphasis on stability, and one
ends up with a system which is hard to change, i.e.
improve or keep secure. A system like that runs the
real risk of being rapidly overtaken by a more nimble
alternative.
More testing is good and will make breakages rarer,
but I guess we need to be realistic here and realize
that even with a huge community effort, we might
never get 100% or even 80% test coverage.
But to be honest these sound like questions for the
Greg KHs and Torvaldses of this world, not for me.
On Tue, Dec 1, 2020 at 4:04 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> Then it should not have been applied to mainline but fully worked out and tested.
>
That would be a reasonable expectation of a product. But Linux
isn't a product, it's a hugely complex, shared system, which may
form the basis of your product. The core maintainers aren't
superhuman, nor do they have access to the 1000s of configurations
and devices where Linux runs or will run. They do their very best,
but if every change had to be 100% tested in every possible
configuration, then few things could ever change, and Linux
would slow down to a snail's pace.
When your product is based on Linux and you pull a newer version
off kernel.org, it's not unreasonable to expect the occasional
breakage. In my case, when I moved from 5.7 to 5.9, some of the
things that broke were my network chip, and most SPI drivers. That
was a bad day, most pulls are trouble-free.
I believe LTSes are more stable than 'stable releases' which are in
turn more stable than RCs. The choice involves a trade-off between
features, security and stability.
When you do run into a breakage, complaining on the mailing list
is good, but posting a fix is better :)
This is my layman's understanding of the situation, I'm just a user
and not a maintainer.
> >
> >>
> >> What should we do?
Hopefully I have some time this week to look into your breakage,
I may get overtaken by someone much more knowledgeable than
me on spi-gpio.
Hi Andreas,
On Tue, Dec 1, 2020 at 11:44 AM Andreas Kemnade <[email protected]> wrote:
>
> So if that one is really active-low, why did it ever work?!
Because the spi gpio chipselect was broken (inverted), and
766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
fixes it.
Hi Linus,
> Am 01.12.2020 um 15:20 schrieb Linus Walleij <[email protected]>:
>
> Hi Nikolaus, Sven,
>
> the fault that the thing broke in the first place is all mine.
> Credit where credit is due!
>
> The reason why I shoot in the dark to convert all SPI
> drivers to use GPIO descriptors instead of the global
> GPIO numberspace is detailed in drivers/gpio/TODO
> so I will not repeat it here.
>
> I don't know if much can be done about it other than
> having better programmers than me at the task. Or
> less tired when they write the patch. etc.
>
> What other operating systems do to get around the same
> type of refactoring problem is to aggressively
> deprecate and delete code that does not follow the
> latest ideas of the driver subsystem developer. This
> is not an option on Linux because we don't like to leave
> working hardware and users behind so I am painstakingly
> fixing it all over the place, with a little help from my
> friends. Sometimes it blows up in my face, sometimes
> in other people faces too, sorry about that.
Everyone has such a patch written at the wrong time of the day...
See for example: f069b5a0b27a
Let's work on a fix for the fix now.
BR and thanks,
Nikolaus
On Tue, Dec 01, 2020 at 05:41:54PM +0100, H. Nikolaus Schaller wrote:
> > Am 01.12.2020 um 17:20 schrieb Mark Brown <[email protected]>:
> > I think the problem here is more to do with where we started than where
> > we're going or how we got there - things have been glued together or
> > happened to work in ways that mean I'm not sure we reasonably understand
> > the situation we started from or all the requirements it has. As you
> > say I'm not sure anything beyond throwing the API away and starting
> > afresh would really help here, but that's not really how we tend to do
> > things for a bunch of very good reasons.
> I think the key problem is GPIO_ACTIVE_HIGH 0 and GPIO_ACTIVE_LOW 1
> in device tree blobs. But that is so fundamental that we have to live with it.
> So I guess that even a new API from scratch wouldn't improve that.
Yeah, that's definitely part of it - more generally there's multiple
places trying to determine if the signal is inverted with different
interactions/expectations. Having it in an ABI definitely contributes a
lot to causing trouble though.
> Am 01.12.2020 um 16:52 schrieb Sven Van Asbroeck <[email protected]>:
>
> Nikolaus,
>
> On Tue, Dec 1, 2020 at 9:38 AM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> Let's work on a fix for the fix now.
>
> I tested spi-gpio on my system, by converting a built-in or hardware spi,
> to a spi-gpio. Interestingly, the patch has the opposite effect on my system:
> before the patch, spi-gpio did not work, but after it's applied, it does work.
>
> Can you tell me the idle status of your chip-select gpio in debugfs?
> # mount -t debugfs none /sys/kernel/debug
> # cat /sys/kernel/debug/gpio
> Look for something like this:
> gpiochip0: GPIOs 0-31, parent: platform/209c000.gpio, 209c000.gpio:
> gpio-17 ( |spi5 CS0 ) out hi ACTIVE LOW
root@letux:~# mount -t debugfs none /sys/kernel/debug
root@letux:~# cat /sys/kernel/debug/gpio|fgrep spi
gpio-179 ( |spi4 CS0 ) out lo
root@letux:~#
That is after the panel driver did send the commands.
>
> Also, apply the following patch, and tell me
> a) does this dev_err() get called on your system, and
yes. Many times.
> b) what is the value when your chip is de-selected
root@letux:~# dmesg|fgrep td028
[ 14.530456] panel-tpo-td028ttec1 spi4.0: spi->mode = 00000003
[ 14.599212] panel-tpo-td028ttec1 spi4.0: gpiod disable
[ 14.817871] panel-tpo-td028ttec1 spi4.0: spi->mode = 00000003
[ 14.817871] panel-tpo-td028ttec1 spi4.0: gpiod enable
So it is disabled first and then enabled. Which appears to be the opposite
of what it should be.
BTW: I have added another dev_err to print the spi->mode and like
you describe it is (overwritten? by SPI_MODE_3.
I can check what value it has in the driver before it is set to SPI_MODE_3.
Maybe, there the spi-cs-high gets lost?
BR and thanks,
Nikolaus
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
> index 7e8804b02be9..b2f4cf5c9ffb 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -813,11 +813,12 @@ static void spi_set_cs(struct spi_device *spi,
> bool enable)
>
> if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
> if (!(spi->mode & SPI_NO_CS)) {
> - if (spi->cs_gpiod)
> + if (spi->cs_gpiod) {
> + dev_err(&spi->dev, "gpiod %s", enable1
> ? "enable" : "disable");
> /* polarity handled by gpiolib */
> gpiod_set_value_cansleep(spi->cs_gpiod,
> enable1);
> - else
> + } else
> /*
> * invert the enable line, as active low is
> * default for SPI.
Hi Sven,
> Am 01.12.2020 um 17:53 schrieb Sven Van Asbroeck <[email protected]>:
>
> Hi Nikolaus,
>
> On Tue, Dec 1, 2020 at 11:43 AM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> You are right. It is active low.
>>
>
> In that case, we have a very simple solution, just remove the spi-cs-high,
> and things will work.
We originally had it that way and because there was a change in gpiolib we had
to introduce it.
See: f1f028ff89cb0d3
where we were forced to introduce it although I had preferred to not change DT.
I am not sure if DT maintainers accept that we revert a DT change just to
handle some change in a driver. Usually they insist on fixing a driver and
live with the DT. DT is carved in stone or could be ROM...
So you could try to submit a revert of f1f028ff89cb0d3 with a description
why it is needed. And please make sure that it is also applied where your
patch is backported to stable. So it should have some
Fixes: 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
>
> In case of SPI CS gpios, the current kernel ignores all
> GPIO_ACTIVE_HIGH/LOW
> flags, and uses the presence/absence of spi-cs-high instead, to
> determine active high / active low.
So you mean you are just restoring the behaviour before
6953c57ab172
was introduced?
The alternative is that you restore the cs-gpios + spi-cs-high semantics
as defined by 6953c57ab172 and everything is fine without touching (our)
DTS (again).
BR and thanks,
Nikolaus
On Tue, Dec 1, 2020 at 6:13 PM H. Nikolaus Schaller <[email protected]> wrote:
> I am not sure if DT maintainers accept that we revert a DT change just to
> handle some change in a driver. Usually they insist on fixing a driver and
> live with the DT. DT is carved in stone or could be ROM...
I usually use this rough consensus: is the DTB flashed into millions of
devices and supplied to the kernel using some bootloader, and
is the kernel upgraded on the device without also upgrading the
DTB?
And I mean in practice, not in theory.
So whether the DTB ABI can be changed or not is a practical
deployment question, not a religious sacrament. It came from systems
such as Sun machines where the DTB was, indeed, in a PROM,
and indeed intended for SunOS so Linux had no control over
it. We had to just treat it as static ABI.
If the actual situation is different, sucn as kernel and DTB are
always updated together, or those are a few custom systems in
a factory floor (not millions of mobile phones or laptops) then
it is fine to change it occasionally even if it is seen as "bad".
Yours,
Linus Walleij
On Tue, Dec 01, 2020 at 01:43:36PM -0500, Sven Van Asbroeck wrote:
> On Tue, Dec 1, 2020 at 12:13 PM H. Nikolaus Schaller <[email protected]> wrote:
> > I am not sure if DT maintainers accept that we revert a DT change just to
> > handle some change in a driver. Usually they insist on fixing a driver and
> > live with the DT. DT is carved in stone or could be ROM...
> This is above my paygrade, but I've always assumed that the devicetree ABI
> is an in-kernel ABI, i.e. not a userspace ABI. Meaning that it is flexible and
> there is no obligation to keep it 100% backwards compatible. Of course Rob
> Herring may want to keep it as backwards-compatible as possible, but that's
> an altogether different thing from having a userspace-type ABI.
It's supposed to be an ABI, though sometimes that gets broken - in a
case like this if there's only a very limited set of users relying on
something that's going to make things harder to maintain and which don't
in practice distribute the kernel separately to the DT then it can make
sense to just break the DT since realistically nobody's going to
actually notice. On the other hand if people are distributing the
kernel separately to the DT then compatibility definitely has to be
maintained.
Hi Sven and Linux,
> Am 01.12.2020 um 19:43 schrieb Sven Van Asbroeck <[email protected]>:
>
> Hi Nikolaus,
I was about to submit two patches. One that reverts our "spi-cs-high"
and one that actively sets the GPIO_ACTIVE_LOW because that seems
to be the right thing. It would work with v5.10, but is it really
the right thing?
Before submitting I would like to clarify the rules.
Especially since it is the same discussion as before:
https://lkml.org/lkml/2019/3/23/131
https://lkml.org/lkml/2019/3/24/2
> On Tue, Dec 1, 2020 at 12:13 PM H. Nikolaus Schaller <[email protected]> wrote:
>>> Am 01.12.2020 um 17:53 schrieb Sven Van Asbroeck <[email protected]>:
>>> On Tue, Dec 1, 2020 at 11:43 AM H. Nikolaus Schaller <[email protected]> wrote:
>>>>
>>>> You are right. It is active low.
>>>>
>>>
>>> In that case, we have a very simple solution, just remove the spi-cs-high,
>>> and things will work.
>>
>> We originally had it that way and because there was a change in gpiolib we had
>> to introduce it.
>
> The current rules re. spi chip-selects in devicetrees are here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib-of.c?h=v5.10-rc6#n191
Yes, there is
/*
* SPI children have active low chip selects
* by default. This can be specified negatively
* by just omitting "spi-cs-high" in the
* device node, or actively by tagging on
* GPIO_ACTIVE_LOW as flag in the device
* tree. If the line is simultaneously
* tagged as active low in the device tree
* and has the "spi-cs-high" set, we get a
* conflict and the "spi-cs-high" flag will
* take precedence.
*/
It is not complete and a formulated a little in reverse.
A table would would be more precise.
And it is not the complete rule:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib-of.c?h=v5.10-rc6#n175
/*
* Legacy handling of SPI active high chip select. If we have a
* property named "cs-gpios" we need to inspect the child node
* to determine if the flags should have inverted semantics.
*/
This seems to indicate that the gpio flags can invert spi-cs-high. There are
only two options: invert if they are GPIO_ACTIVE_LOW or GPIO_ACTIVE_HIGH.
This makes IMHO much sense. But it seems that it was not implemented that way.
>
> This is the way I see things:
> - according to the current rules, your devicetree describes a spi panel with
> an active-high chip select
I have now tested all 4 combinations of GPIO_ACTIVE_LOW/HIGH and missing/present
"spi-cs-high" with v5.4.79.
This leads to the following definition:
a) general SPI controller or if no cs-gpios available in spi-gpio
device node | CS pin state active
================+==================
spi-cs-high | H
- | L
This was the single (legacy) rule until v4.18 effectively ignoring any gpio flags.
b) with v4.19 and 6953c57ab172 ("gpio: of: Handle SPI chipselect legacy bindings") was introduced
it was apparently extended to (by code inspection of the patch):
device node | cs-gpio | CS pin state active
================+===============+====================
spi-cs-high | - | H
- | - | L
spi-cs-high | ACTIVE_HIGH | L (polarity inversion, no warning)
- | ACTIVE_HIGH | L + "enforce active low on chipselect handle"
spi-cs-high | ACTIVE_LOW | H + "GPIO handle specifies active low - ignored"
- | ACTIVE_LOW | H (no warning)
But I did test all 4 combinations with v5.4.79 and assuming CS pin
state L if the panel works gave me:
device node | cs-gpio | CS pin state active
================+===============+====================
spi-cs-high | ACTIVE_HIGH | L (panel works)
- | ACTIVE_HIGH | H (panel fails) + "enforce active low on chipselect handle"
spi-cs-high | ACTIVE_LOW | L (panel works) + "GPIO handle specifies active low - ignored"
- | ACTIVE_LOW | H (panel fails without comment)
This means there was some additional tweak to the rules effectively ignoring
the cs-gpio. Like before - but with opposite polarity.
Note that some legacy gpio flags are 0 (ACTIVE_HIGH) so it seems to make sense
for the ACTIVE_HIGH case like ours.
Therefore, only GPIO_ACTIVE_HIGH + "spi-cs-high" works without side-effects which
is the reason why we had modified our device tree and defined the wrong
"spi-cs-high" as a solution.
Anyways it is debatable if this is a bug at all. It is just a definition.
Which is not well documented anywhere.
c) now with v5.10 and 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
the spi driver does no longer rely on gpiolib to do the right thing but adds
its own rule effectively inverting things again. I hope I got it right because
the logic is now spread over several files:
device node | cs-gpio | CS pin state active
================+===============+====================
spi-cs-high | - | H
- | - | L
spi-cs-high | ACTIVE_HIGH | H (breaks our panel with current DT)
- | ACTIVE_HIGH | L (should print a warning about polarity inversion - because here it would be wise to switch to ACTIVE_LOW)
spi-cs-high | ACTIVE_LOW | H (could print a warning about polarity inversion - because ACTIVE_LOW is overridden by spi-cs-high)
- | ACTIVE_LOW | L
The implementation with your fix now seems to completely ignore the gpio
definition since in the absence of spi-cs-high the panel works with either
ACTIVE_ definition.
Basically, we are back at a) i.e. the same as it was up to v4.18.
So in total there should be no automatic polarity inversion of the flags
by gpiolib at all. Only the spi driver should know that it has to invert
the parameter for ACTIVE_LOW to gpiod_set_value_cansleep() to really control
the CS pin state (H/L).
> - the actual chip select of your panel is active-low
Yes.
> - a spi/gpiod bug inverted the chip-select in many instances
Ok, there was another patch in between:
138c9c32f090 ("spi: spidev: Fix CS polarity if GPIO descriptors are used")
Maybe this has introduced the bug - and we just didn't notice.
This is why I have added Lukas Wunner <[email protected]> to the discussion.
> - because of this bug, your devicetree happened to work before 766c6b63aa04
IMHO it is debatable if it is a bug or what is the bug. In any case it
is a gap in clear and binding documentation.
> - 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
> fixes this chip-select polarity bug
> - you now need to remove your devicetree work-around for this bug by reverting
> f1f028ff89cb0d3
Before doing this I would like to see a table (like mine above) how it
should be. Definitively and finally. Agreed and approved by maintainers.
This table should then go somewhere to the .yaml definitions of gpiod or
spi or whereever it belongs to.
Then you can check if your code does what the table mandates and I can
check if our device tree does the right thing.
Otherwise we risk that I change the DT now and in 2 years someone else
finds that the definition is still not sane and tweaks SPI or gpiolib
again and I have to modify our DT once more (which costs a lot of time
to find the reasons of breakage and takes away a lot of work-time from
other nice things I would like to add to Linux).
What I especially wonder is why you fix that at all in drivers/spi/spi.c
if polarity inversion is handled in gpiolib.
BR and thanks,
Nikolaus
On Fri, Dec 4, 2020 at 5:11 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> Anyways it is debatable if this is a bug at all. It is just a definition.
I respectfully disagree. Prior to the fix, your panel's active-low chip select
needed to be described in the devicetree with 'spi-cs-high'. That sounds
very much like a bug to me.
> Which is not well documented anywhere.
I agree that documentation can be improved here.
Would you like to submit a patch that improves:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml?h=v5.10-rc6#n28
?
This way, we also get Rob Herring involved, which may lead to more elegant
documentation. He is more likely to respond to a patch than to a question.
>
> What I especially wonder is why you fix that at all in drivers/spi/spi.c
> if polarity inversion is handled in gpiolib.
The reason for that is described in the commit message of
766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
Hi Sven,
> Am 04.12.2020 um 14:46 schrieb Sven Van Asbroeck <[email protected]>:
>
> On Fri, Dec 4, 2020 at 5:11 AM H. Nikolaus Schaller <[email protected]> wrote:
>>
>> Anyways it is debatable if this is a bug at all. It is just a definition.
>
> I respectfully disagree. Prior to the fix, your panel's active-low chip select
> needed to be described in the devicetree with 'spi-cs-high'. That sounds
> very much like a bug to me.
It could have been described by ACTIVE_LOW without spi-cs-high but that did
emit a nasty and not helpful warning on each boot.
Well, there are of course better and worse definitions and I agree that
spi-cs-high to define an active-low chip select sounds strange. Still it
is just a definition.
But what I don't know is if I can omit spi-cs-high and have to keep
ACTIVE_HIGH (my revert patch) or also change to ACTIVE_LOW (my additional
patch). This is arbitrary and someone has to decide what it should be.
Then, the gpiolib / spi driver code should be adapted if necessary and a
unit-test or real-hardware test with all possible combinations would prove
it for all other devices as well. So testing against a specification does
not need /my/ hardware.
>
>> Which is not well documented anywhere.
>
> I agree that documentation can be improved here.
> Would you like to submit a patch that improves:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/spi/spi-controller.yaml?h=v5.10-rc6#n28
> ?
Yes, that is the right location.
Basically it involves adding a table like in my previous mail to the comment
so that it also covers all cases where the second parameter is not 0.
I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.
The reason is that I am neither involved in gpiolib nor spi subsystem development
so I neither know what your intended setup is. I may define a wrong table.
I just need a stable definition by the subsystem maintainers so that I can
finish the device tree I am responsible for.
What I can do is to provide just a skeleton for the table that you or Linus
can fix/fill in and make a patch out of it. Is attached and the ??? is
something you should discuss and define.
> This way, we also get Rob Herring involved, which may lead to more elegant
> documentation. He is more likely to respond to a patch than to a question.
>
>>
>> What I especially wonder is why you fix that at all in drivers/spi/spi.c
>> if polarity inversion is handled in gpiolib.
>
> The reason for that is described in the commit message of
> 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
IMHO it could have been fixed in the gpiolib alone. In my assumption, gpiolib
would know (or be informed) that the gpio is used by spi and could invert gpio_set_value()
if needed. Then any SPI code would simply use gpio_set_value(true) if spi-cs-high
is defined and gpio_set_value(false) if not to enable the chip.
The alternative would be that it is only done centrally in one place in the
spi subsystem.
But I may be wrong, because the real architecture of the spi and gpiolib code
is quite new to me. My focus is on very different things (e.g. camera driver,
drm panel drivers) which is already complex enough.
BR and thanks,
Nikolaus
diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 1b56d5e40f1f..4f8755dabecc 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -42,6 +42,30 @@ properties:
cs2 : &gpio1 1 0
cs3 : &gpio1 2 0
+ The second flag can be GPIO_ACTIVE_HIGH/0 or GPIO_ACTIVE_LOW/1.
+
+ There is 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 physically driven
+ (not considering 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 (or H ???) | 1
+ spi-cs-high | ACTIVE_LOW | H (or L ???) | 2
+ - | ACTIVE_LOW | L |
+
+ Notes:
+ 1) should print a warning about polarity inversion
+ because here it would be wise to define the gpio as ACTIVE_LOW
+ 2) could print a warning about polarity inversion
+ because ACTIVE_LOW is overridden by spi-cs-high
+ 3) Effectively this rule defines that the ACTIVE level of the
+ gpio has to be ignored
+
num-cs:
$ref: /schemas/types.yaml#/definitions/uint32
description:
On Fri, Dec 4, 2020 at 11:52 AM H. Nikolaus Schaller <[email protected]> wrote:
>
> >> Anyways it is debatable if this is a bug at all. It is just a definition.
> >
> > I respectfully disagree. Prior to the fix, your panel's active-low chip select
> > needed to be described in the devicetree with 'spi-cs-high'. That sounds
> > very much like a bug to me.
>
> It could have been described by ACTIVE_LOW without spi-cs-high but that did
> emit a nasty and not helpful warning on each boot.
That will not work, try it out. You will see that without the bugfix, your chip
select is consistently inverted, no matter how you formulate it in the
devicetree.
>
> I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.
I cannot help you with that, I'm sorry.
> Am 04.12.2020 um 20:19 schrieb Sven Van Asbroeck <[email protected]>:
>
> On Fri, Dec 4, 2020 at 11:52 AM H. Nikolaus Schaller <[email protected]> wrote:
>>
>>>> Anyways it is debatable if this is a bug at all. It is just a definition.
>>>
>>> I respectfully disagree. Prior to the fix, your panel's active-low chip select
>>> needed to be described in the devicetree with 'spi-cs-high'. That sounds
>>> very much like a bug to me.
>>
>> It could have been described by ACTIVE_LOW without spi-cs-high but that did
>> emit a nasty and not helpful warning on each boot.
>
> That will not work, try it out. You will see that without the bugfix, your chip
> select is consistently inverted, no matter how you formulate it in the
> devicetree.
I have.
But please show me which line in my analyses table of my mail 12 hours ago
is wrong. Then I can repeat the test and we can discuss the reasons.
>
>>
>> I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.
>
> I cannot help you with that, I'm sorry.
Come on...
BR and thanks,
Nikolaus
On Fri, Dec 4, 2020 at 5:52 PM H. Nikolaus Schaller <[email protected]> wrote:
> It could have been described by ACTIVE_LOW without spi-cs-high but that did
> emit a nasty and not helpful warning on each boot.
>
> Well, there are of course better and worse definitions and I agree that
> spi-cs-high to define an active-low chip select sounds strange. Still it
> is just a definition.
This has an historical background which is why we have the
problem in the first place. We ended up with two different
ways of doing polarity inversion in some subsystems because
polarity flags *and* local polarity flags such as this existed.
So the semantics became ambiguous.
commit f618ebfcbf9616a0fa9a78f5ecb69762f0fa3c59
"of/spi: Support specifying chip select as active high via device tree"
created spi-cs-high
October 2008
commit b908b53d580c3e9aba81ebe3339c5b7b4fa8031d
"of/gpio: Implement of_get_gpio_flags()"
Created of_get_gpio_flags() and OF_GPIO_ACTIVE_LOW
as an optional but strongly encouraged cell.
December 2008
What we are seeing
is the consequence of a formal language with ambiguous
semantics. It's no-ones fault other than the human error of
allowing both to exist simultaneously in the first place.
Both ways of doing things existed before I took over as GPIO
maintainer and before Mark took over as SPI maintainer.
What we try to do is contain the situation. My attempt was
to hide it inside the GPIO descriptor, which we still do,
if and only if GPIO descriptors are used.
> But what I don't know is if I can omit spi-cs-high and have to keep
> ACTIVE_HIGH (my revert patch) or also change to ACTIVE_LOW (my additional
> patch). This is arbitrary and someone has to decide what it should be.
(...)
> I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.
It seems really ill-advised to have me do that since I have not
managed very well to deal with this. Clearly better developers
are needed. But I can review a patch and see if it makes me
smarter :)
> > The reason for that is described in the commit message of
> > 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")
>
> IMHO it could have been fixed in the gpiolib alone. In my assumption, gpiolib
> would know (or be informed) that the gpio is used by spi and could invert gpio_set_value()
> if needed. Then any SPI code would simply use gpio_set_value(true) if spi-cs-high
> is defined and gpio_set_value(false) if not to enable the chip.
That was the intention behind my code in
of_gpio_flags_quirks().
And I suppose how it finally works after the recent patches
as well, since we now pass enable1 (the non-inverted
assertion parameter) to gpiod_set_value_cansleep()
if and only if you are using GPIO descriptors.
The reason it didn't work and a lot of ill-advised patches
(mostly mine) has a lot to do with native chip selects
which both listened to the same flag "spi-cs-high"
and expected specific semantics from the spi core.
For native chip selects there is no ambiguity: they can
only be made active high using "spi-cs-high".
> The alternative would be that it is only done centrally in one place in the
> spi subsystem.
FWIW I think GPIO CS is fine to handle in the gpiolib
but then spi-cs-high also applies to native chip selects
and that makes it necessary for the SPI core to also parse
for spi-cs-high sometimes, for something that is not
a GPIO, setting SPI_CS_HIGH and calling
spi->controller->set_cs() on the controller with 0 for
active low and 1 for active high.
I think we are there now?
Yours,
Linus Walleij
Hi Linus,
> Am 05.12.2020 um 01:25 schrieb Linus Walleij <[email protected]>:
>
> On Fri, Dec 4, 2020 at 5:52 PM H. Nikolaus Schaller <[email protected]> wrote:
>
>> But what I don't know is if I can omit spi-cs-high and have to keep
>> ACTIVE_HIGH (my revert patch) or also change to ACTIVE_LOW (my additional
>> patch). This is arbitrary and someone has to decide what it should be.
> (...)
>> I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.
>
> It seems really ill-advised to have me do that since I have not
> managed very well to deal with this. Clearly better developers
> are needed. But I can review a patch and see if it makes me
> smarter :)
I find it interesting that so far nobody wants to take responsibility
for a decision and to write down the behaviour really should be. Coding
is the second step then.
Anyways you did not cite the really important part of my mail. So let me
copy it back. Here it is again:
> What I can do is to provide just a skeleton for the table that you or Linus
> can fix/fill in and make a patch out of it. Is attached and the ??? is
> something you should discuss and define.
Please take the attached diff, comment it here and define the question marks
according to your intention and then make a patch for the YAML bindings out
of it. (I can't do because I don't know your intentions and what to write into
the commit message).
As soon as we have settled this, we can check if code is correct and really
define if my device tree fits and which change it needs exactly.
BR and thanks,
Nikolaus
[slightly edited]
diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 1b56d5e40f1f..4f8755dabecc 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -42,6 +42,30 @@ 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.
+
+ 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 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 (or H ???) | 1
+ spi-cs-high | ACTIVE_LOW | H (or L ???) | 2
+ - | ACTIVE_LOW | L |
+
+ Notes:
+ 1) should print a warning about polarity inversion
+ because here it would be wise to define the gpio as ACTIVE_LOW
+ 2) could print a warning about polarity inversion
+ because ACTIVE_LOW is overridden by spi-cs-high
+ 3) Effectively this rule defines that the ACTIVE level of the
+ gpio has to be ignored
+
num-cs:
$ref: /schemas/types.yaml#/definitions/uint32
description:
On Mon 2020-11-30 15:13:02, Sven Van Asbroeck wrote:
> Hi Nikolaus, thank you for reaching out !
>
> On Mon, Nov 30, 2020 at 2:06 PM H. Nikolaus Schaller <[email protected]> wrote:
> >
> > But reverting your patch brings back the display. So it appears as if it does not
> > fix a breakage, rather breaks a previously working setup.
>
> The patch in question fixes an important breakage: before the patch, literally
> hundreds of SPI drivers no longer worked - only if the SPI bus master
> driver was using gpio descriptors.
>
> We knew that there was a chance that our fix would break something else.
> But hopefully "it fixes more than it breaks"
If the patch causes regression it will ultimately need to be
reverted... no matter how much it fixes.
Pavel
--
http://www.livejournal.com/~pavelmachek
On Sat, Dec 05, 2020 at 09:57:44PM +0100, Pavel Machek wrote:
> If the patch causes regression it will ultimately need to be
> reverted... no matter how much it fixes.
It's regression fixes all the way down...
On Sat, Dec 5, 2020 at 8:07 AM H. Nikolaus Schaller <[email protected]> wrote:
> I find it interesting that so far nobody wants to take responsibility
> for a decision
(...)
What causes some consternation in this discussion is the appeal
to higher authority. The kernel community in general does not like
authority/responsibility by way of formal hierarchy.
Have you read this document? Especially point 1) Decisions:
https://www.kernel.org/doc/html/latest/process/management-style.html
(We can have a meta-discussion about this but it is not really your
point I believe.)
> > What I can do is to provide just a skeleton for the table that you or Linus
> > can fix/fill in and make a patch out of it. Is attached and the ??? is
> > something you should discuss and define.
>
> Please take the attached diff, comment it here and define the question marks
> according to your intention and then make a patch for the YAML bindings out
> of it. (I can't do because I don't know your intentions and what to write into
> the commit message).
I'll comment what I know, then you can send a proper patch to
Mark. But you really need more people than me to look at this.
> + device node | cs-gpio | CS pin state active | Note
> + ================+===============+=====================+=====
> + spi-cs-high | - | H |
> + - | - | L |
> + spi-cs-high | ACTIVE_HIGH | H |
> + - | ACTIVE_HIGH | L (or H ???) | 1
When using GPIO descriptors it will be enforced to ACTIVE_LOW (L) with an
explicit warning in dmesg, see drivers/gpio/gpiolib-of.c
When using legacy GPIOs, will be enforced ACTIVE_LOW by the SPI
core.
> + spi-cs-high | ACTIVE_LOW | H (or L ???) | 2
When using GPIO descriptors it will be enforced to ACTIVE_HIGH (H) with an
explicit warning in dmesg, see drivers/gpio/gpiolib-of.c
> + 3) Effectively this rule defines that the ACTIVE level of the
> + gpio has to be ignored
Nr 3 isn't tagged in the table.
Yours,
Linus Walleij
Hi Andreas,
> Am 09.12.2020 um 09:04 schrieb Andreas Kemnade <[email protected]>:
>
> Hi,
>
> On Sat, 5 Dec 2020 08:04:25 +0100
> "H. Nikolaus Schaller" <[email protected]> wrote:
>
>> Hi Linus,
>>
>>> Am 05.12.2020 um 01:25 schrieb Linus Walleij <[email protected]>:
>>>
>>> On Fri, Dec 4, 2020 at 5:52 PM H. Nikolaus Schaller <[email protected]> wrote:
>>>
>>>> But what I don't know is if I can omit spi-cs-high and have to keep
>>>> ACTIVE_HIGH (my revert patch) or also change to ACTIVE_LOW (my additional
>>>> patch). This is arbitrary and someone has to decide what it should be.
>>> (...)
>>>> I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.
>>>
>>> It seems really ill-advised to have me do that since I have not
>>> managed very well to deal with this. Clearly better developers
>>> are needed. But I can review a patch and see if it makes me
>>> smarter :)
>
> Hmm, if those developers are not available, then probably finding
> those bugs has to be time-optimized, like establishing better automatic
> display testing.
Well, I don't think we need automatic display testing.
We just need to test if any SPI CS behaves correctly according to some
specification. Then all displays and other chips will work - unless
they have a bug in their DT.
Basically it would need a software unit-test going through all 6 variants
of having spi-cs-high and gpiod and parameters and looking if the chip
(maybe some SPI EEPROM with known SPI polarity) responds or not. This
can be done with hardware SPI controllers and spi-gpio.
And it can be re-run each time something significant in gpiolib or spi-gpio
is changed.
>
>>
>> I find it interesting that so far nobody wants to take responsibility
>> for a decision and to write down the behaviour really should be. Coding
>> is the second step then.
>>
> well, the interesting people are not involved yet (DTML) because no
> patch is sent.
Well, I think we (gpiolib maintainers, spi maintainers and users of it) are
the right ones to define the truth table.
This is not primarily a DT issue, it is a matter of what we want to have
and then it is cast it into DT (documentation). So I am not sure if delegation
to someone else solves it.
>
>> Anyways you did not cite the really important part of my mail. So let me
>> copy it back. Here it is again:
>>
>>> What I can do is to provide just a skeleton for the table that you or Linus
>>> can fix/fill in and make a patch out of it. Is attached and the ??? is
>>> something you should discuss and define.
>>
>> Please take the attached diff, comment it here and define the question marks
>> according to your intention and then make a patch for the YAML bindings out
>> of it. (I can't do because I don't know your intentions and what to write into
>> the commit message).
>>
> Well, I the easiest step forward is just to document clearer how things
> behave now, so the commit message is just something like
>
> "Behavior of CS signal is not clearly documented, clarify the
> documentation". And then send the patch to the proper mailing lists
> including devicetree folks.
Ok, that looks like a good solution to get out of the deadlock.
BR and thanks,
Nikolaus
>
> Regards,
> Andreas
>
>> As soon as we have settled this, we can check if code is correct and really
>> define if my device tree fits and which change it needs exactly.
>>
>> BR and thanks,
>> Nikolaus
>>
>> [slightly edited]
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
>> index 1b56d5e40f1f..4f8755dabecc 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
>> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
>> @@ -42,6 +42,30 @@ 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.
>> +
>> + 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 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 (or H ???) | 1
>> + spi-cs-high | ACTIVE_LOW | H (or L ???) | 2
>> + - | ACTIVE_LOW | L |
>> +
>> + Notes:
>> + 1) should print a warning about polarity inversion
>> + because here it would be wise to define the gpio as ACTIVE_LOW
>> + 2) could print a warning about polarity inversion
>> + because ACTIVE_LOW is overridden by spi-cs-high
>> + 3) Effectively this rule defines that the ACTIVE level of the
>> + gpio has to be ignored
>> +
>> num-cs:
>> $ref: /schemas/types.yaml#/definitions/uint32
>> description:
>>
>>
>>
>
Hi Linus,
> Am 09.12.2020 um 09:38 schrieb Linus Walleij <[email protected]>:
>
> On Sat, Dec 5, 2020 at 8:07 AM H. Nikolaus Schaller <[email protected]> wrote:
>
>> I find it interesting that so far nobody wants to take responsibility
>> for a decision
> (...)
>
>
>>> What I can do is to provide just a skeleton for the table that you or Linus
>>> can fix/fill in and make a patch out of it. Is attached and the ??? is
>>> something you should discuss and define.
>>
>> Please take the attached diff, comment it here and define the question marks
>> according to your intention and then make a patch for the YAML bindings out
>> of it. (I can't do because I don't know your intentions and what to write into
>> the commit message).
>
> I'll comment what I know, then you can send a proper patch to
> Mark. But you really need more people than me to look at this.
>
>> + device node | cs-gpio | CS pin state active | Note
>> + ================+===============+=====================+=====
>> + spi-cs-high | - | H |
>> + - | - | L |
>> + spi-cs-high | ACTIVE_HIGH | H |
>> + - | ACTIVE_HIGH | L (or H ???) | 1
>
> When using GPIO descriptors it will be enforced to ACTIVE_LOW (L) with an
> explicit warning in dmesg, see drivers/gpio/gpiolib-of.c
Ok, so in this line the L is ok.
>
> When using legacy GPIOs, will be enforced ACTIVE_LOW by the SPI
> core.
>
>> + spi-cs-high | ACTIVE_LOW | H (or L ???) | 2
>
> When using GPIO descriptors it will be enforced to ACTIVE_HIGH (H) with an
> explicit warning in dmesg, see drivers/gpio/gpiolib-of.c
Ok, so my assumption about H is right and not the part in parenthesis with ???.
>
>> + 3) Effectively this rule defines that the ACTIVE level of the
>> + gpio has to be ignored
>
> Nr 3 isn't tagged in the table.
Well, it was thought as a third but general note. Maybe should have been a *)
or omitted since the table stands for itself.
>
> Yours,
> Linus Walleij
So let me prepare a patch with fixes for this.
BR and thanks,
Nikolaus
Hi,
On Sat, 5 Dec 2020 08:04:25 +0100
"H. Nikolaus Schaller" <[email protected]> wrote:
> Hi Linus,
>
> > Am 05.12.2020 um 01:25 schrieb Linus Walleij <[email protected]>:
> >
> > On Fri, Dec 4, 2020 at 5:52 PM H. Nikolaus Schaller <[email protected]> wrote:
> >
> >> But what I don't know is if I can omit spi-cs-high and have to keep
> >> ACTIVE_HIGH (my revert patch) or also change to ACTIVE_LOW (my additional
> >> patch). This is arbitrary and someone has to decide what it should be.
> > (...)
> >> I'd prefer if you or maybe Linus could submit such a patch and I am happy to review it.
> >
> > It seems really ill-advised to have me do that since I have not
> > managed very well to deal with this. Clearly better developers
> > are needed. But I can review a patch and see if it makes me
> > smarter :)
Hmm, if those developers are not available, then probably finding
those bugs has to be time-optimized, like establishing better automatic
display testing.
>
> I find it interesting that so far nobody wants to take responsibility
> for a decision and to write down the behaviour really should be. Coding
> is the second step then.
>
well, the interesting people are not involved yet (DTML) because no
patch is sent.
> Anyways you did not cite the really important part of my mail. So let me
> copy it back. Here it is again:
>
> > What I can do is to provide just a skeleton for the table that you or Linus
> > can fix/fill in and make a patch out of it. Is attached and the ??? is
> > something you should discuss and define.
>
> Please take the attached diff, comment it here and define the question marks
> according to your intention and then make a patch for the YAML bindings out
> of it. (I can't do because I don't know your intentions and what to write into
> the commit message).
>
Well, I the easiest step forward is just to document clearer how things
behave now, so the commit message is just something like
"Behavior of CS signal is not clearly documented, clarify the
documentation". And then send the patch to the proper mailing lists
including devicetree folks.
Regards,
Andreas
> As soon as we have settled this, we can check if code is correct and really
> define if my device tree fits and which change it needs exactly.
>
> BR and thanks,
> Nikolaus
>
> [slightly edited]
>
> diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> index 1b56d5e40f1f..4f8755dabecc 100644
> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> @@ -42,6 +42,30 @@ 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.
> +
> + 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 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 (or H ???) | 1
> + spi-cs-high | ACTIVE_LOW | H (or L ???) | 2
> + - | ACTIVE_LOW | L |
> +
> + Notes:
> + 1) should print a warning about polarity inversion
> + because here it would be wise to define the gpio as ACTIVE_LOW
> + 2) could print a warning about polarity inversion
> + because ACTIVE_LOW is overridden by spi-cs-high
> + 3) Effectively this rule defines that the ACTIVE level of the
> + gpio has to be ignored
> +
> num-cs:
> $ref: /schemas/types.yaml#/definitions/uint32
> description:
>
>
>