SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Christophe Leroy]

Hello,

My board has powerpc 885 microcontroler. Temperature sensor is an LM74.

Kernel has CONFIG_SPI_FSL_SPI and CONFIG_SPI_FSL_CPM and CONFIG_SENSORS_LM70.

Since kernel 5.10, 'sensors' reports temperature 0°C on my board:

	root@vgoip:~# sensors

	lm74-spi-0-5
	Adapter: SPI adapter
	temp1:         +0.0 C

	lm74-spi-0-1
	Adapter: SPI adapter
	temp1:         +0.0 C

With commit 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") reverted, 
it is back to work:

	root@vgoip:~# sensors

	lm74-spi-0-5
	Adapter: SPI adapter
	temp1:        +38.9 C

	lm74-spi-0-1
	Adapter: SPI adapter
	temp1:        +37.2 C


What shall I do ?

Thanks
Christophe

Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 445 bytes --]

On Wed, Jan 13, 2021 at 09:49:12AM +0100, Christophe Leroy wrote:

> With commit 766c6b63aa04 ("spi: fix client driver breakages when using GPIO
> descriptors") reverted, it is back to work:

...

> What shall I do ?

I would guess that there's an error with the chip select polarity
configuration on your system that just happened to work previously, I'd
suggest fixing this in the board configuration to bring it in line with
everything else.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Christophe Leroy]

Le 13/01/2021 à 13:33, Mark Brown a écrit :
> On Wed, Jan 13, 2021 at 09:49:12AM +0100, Christophe Leroy wrote:
> 
>> With commit 766c6b63aa04 ("spi: fix client driver breakages when using GPIO
>> descriptors") reverted, it is back to work:
> 
> ...
> 
>> What shall I do ?
> 
> I would guess that there's an error with the chip select polarity
> configuration on your system that just happened to work previously, I'd
> suggest fixing this in the board configuration to bring it in line with
> everything else.
> 

Not that easy.

Today I have in the DTS the CS GPIOs declared as ACTIVE_LOW.

If I declare them as ACTIVE_HIGH instead, then I also have to set spi-cs-high property, otherwise 
of_gpio_flags_quirks() is not happy and forces the GPIO ACTIVE LOW.

When I set spi-cs-high property, it sets the SPI_CS_HIGH bit in spi->mode.

In fsl_spi_chipselect(), we have

	bool pol = spi->mode & SPI_CS_HIGH

Then
	pdata->cs_control(spi, pol);

So changing the board config is compensated by the above, and at the end it still doesn't work.


Whereas reverting the above mentionned commit sets back SPI_CS_HIGH into spi->mode without changing 
the ACTIVE level of the GPIO, resulting in the correct polarity.


So, I'm a bit lost, where is the problem exactly ?

Thanks
Christophe

Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Thu, Jan 14, 2021 at 12:27:42PM +0100, Christophe Leroy wrote:

> Today I have in the DTS the CS GPIOs declared as ACTIVE_LOW.

> If I declare them as ACTIVE_HIGH instead, then I also have to set
> spi-cs-high property, otherwise of_gpio_flags_quirks() is not happy and
> forces the GPIO ACTIVE LOW.

> When I set spi-cs-high property, it sets the SPI_CS_HIGH bit in spi->mode.

OK, so it sounds like you want SPI_CS_HIGH and that is being set
correctly?

> In fsl_spi_chipselect(), we have
> 
> 	bool pol = spi->mode & SPI_CS_HIGH
> 
> Then
> 	pdata->cs_control(spi, pol);

> So changing the board config is compensated by the above, and at the end it still doesn't work.

This is a driver bug, the driver set_cs() operation should not be
modifying the value it is told to set.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Christophe Leroy]

Le 14/01/2021 à 12:59, Mark Brown a écrit :
> On Thu, Jan 14, 2021 at 12:27:42PM +0100, Christophe Leroy wrote:
> 
>> Today I have in the DTS the CS GPIOs declared as ACTIVE_LOW.
> 
>> If I declare them as ACTIVE_HIGH instead, then I also have to set
>> spi-cs-high property, otherwise of_gpio_flags_quirks() is not happy and
>> forces the GPIO ACTIVE LOW.
> 
>> When I set spi-cs-high property, it sets the SPI_CS_HIGH bit in spi->mode.
> 
> OK, so it sounds like you want SPI_CS_HIGH and that is being set
> correctly?
> 
>> In fsl_spi_chipselect(), we have
>>
>> 	bool pol = spi->mode & SPI_CS_HIGH
>>
>> Then
>> 	pdata->cs_control(spi, pol);
> 
>> So changing the board config is compensated by the above, and at the end it still doesn't work.
> 
> This is a driver bug, the driver set_cs() operation should not be
> modifying the value it is told to set.
> 

A driver bug ? Or maybe a change forgotten in commit  766c6b63aa04 ("spi: fix client driver 
breakages when using GPIO descriptors") ?

I'm almost sure it was not a bug, it is in line which what is said in the comment removed by the 
above mentionned commit.

I'll send a fix.

Christophe

Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Thu, Jan 14, 2021 at 01:33:50PM +0100, Christophe Leroy wrote:
> Le 14/01/2021 à 12:59, Mark Brown a écrit :
> > On Thu, Jan 14, 2021 at 12:27:42PM +0100, Christophe Leroy wrote:

> > > Today I have in the DTS the CS GPIOs declared as ACTIVE_LOW.

> > > If I declare them as ACTIVE_HIGH instead, then I also have to set
> > > spi-cs-high property, otherwise of_gpio_flags_quirks() is not happy and
> > > forces the GPIO ACTIVE LOW.

> > > When I set spi-cs-high property, it sets the SPI_CS_HIGH bit in spi->mode.

> > OK, so it sounds like you want SPI_CS_HIGH and that is being set
> > correctly?

> > > In fsl_spi_chipselect(), we have
> > > 
> > > 	bool pol = spi->mode & SPI_CS_HIGH
> > > 
> > > Then
> > > 	pdata->cs_control(spi, pol);

> > > So changing the board config is compensated by the above, and at the end it still doesn't work.

> > This is a driver bug, the driver set_cs() operation should not be
> > modifying the value it is told to set.

> A driver bug ? Or maybe a change forgotten in commit  766c6b63aa04 ("spi:
> fix client driver breakages when using GPIO descriptors") ?

The expectation that the driver will be using the chip select exactly as
passed in and not attempting to implement SPI_CS_HIGH itself when it has
set_cs() has been there for a considerable time now, that's not new with
the cleanup.  Drivers should only be paying attention to SPI_CS_HIGH in
cases where the hardware controls the chip select autonomously and so
set_cs() can't be provided.

> I'm almost sure it was not a bug, it is in line which what is said in
> the comment removed by the above mentionned commit.

Please take a look at the list archive discussions around this - there's
been a lot of confusion with GPIO descriptors in particular due to there
being multiple places where you can set the inversion.  Note that the
situation you describe above is that you end up with all the various
places other than your driver agreeing that the chip select is active
high as it (AFAICT from what you're saying) actually is.  

For GPIO chipselects you should really fix the driver to just hand the
GPIO off to the core rather than trying to implement this itself, that
will avoid driver specific differences like this.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Christophe Leroy]

Le 14/01/2021 à 14:22, Mark Brown a écrit :
> 
> For GPIO chipselects you should really fix the driver to just hand the
> GPIO off to the core rather than trying to implement this itself, that
> will avoid driver specific differences like this.
> 

IIUC, it is not trivial as it requires implementing transfer_one() instead of the existing 
transfer_one_message() in the driver. Am I right ?

What's the difference/benefit of transfer_one() compared to the existing transfer_one_message() ?

Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Thu, Jan 14, 2021 at 02:42:26PM +0100, Christophe Leroy wrote:
> Le 14/01/2021 à 14:22, Mark Brown a écrit :

> > For GPIO chipselects you should really fix the driver to just hand the
> > GPIO off to the core rather than trying to implement this itself, that
> > will avoid driver specific differences like this.

> IIUC, it is not trivial as it requires implementing transfer_one() instead
> of the existing transfer_one_message() in the driver. Am I right ?

Yes, that's a good idea in general though.  It should normally be pretty
simple since the conversion is mostly just deleting code doing things
which will be handled by the core.

> What's the difference/benefit of transfer_one() compared to the existing transfer_one_message() ?

It factors out all the handling of chip selects, including per-transfer
chip select inversion and so on, and also factors out all the handling
of in-message delays.  If nothing else it reduces the amount of
duplicated code that might require maintainance can have issues with
misaligned expectations from the core or client drivers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]