* SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") @ 2021-01-13 8:49 Christophe Leroy 2021-01-13 12:33 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Christophe Leroy @ 2021-01-13 8:49 UTC (permalink / raw) To: Sven Van Asbroeck, Linus Walleij, Mark Brown Cc: linux-spi, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") 2021-01-13 8:49 SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") Christophe Leroy @ 2021-01-13 12:33 ` Mark Brown 2021-01-14 11:27 ` Christophe Leroy 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2021-01-13 12:33 UTC (permalink / raw) To: Christophe Leroy Cc: Sven Van Asbroeck, Linus Walleij, linux-spi, linux-kernel, linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") 2021-01-13 12:33 ` Mark Brown @ 2021-01-14 11:27 ` Christophe Leroy 2021-01-14 11:59 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Christophe Leroy @ 2021-01-14 11:27 UTC (permalink / raw) To: Mark Brown Cc: Sven Van Asbroeck, Linus Walleij, linux-spi, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") 2021-01-14 11:27 ` Christophe Leroy @ 2021-01-14 11:59 ` Mark Brown 2021-01-14 12:33 ` Christophe Leroy 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2021-01-14 11:59 UTC (permalink / raw) To: Christophe Leroy Cc: Sven Van Asbroeck, Linus Walleij, linux-spi, linux-kernel, linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") 2021-01-14 11:59 ` Mark Brown @ 2021-01-14 12:33 ` Christophe Leroy 2021-01-14 13:22 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Christophe Leroy @ 2021-01-14 12:33 UTC (permalink / raw) To: Mark Brown Cc: Sven Van Asbroeck, Linus Walleij, linux-spi, linux-kernel, linuxppc-dev 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") 2021-01-14 12:33 ` Christophe Leroy @ 2021-01-14 13:22 ` Mark Brown 2021-01-14 13:42 ` Christophe Leroy 0 siblings, 1 reply; 8+ messages in thread From: Mark Brown @ 2021-01-14 13:22 UTC (permalink / raw) To: Christophe Leroy Cc: Sven Van Asbroeck, Linus Walleij, linux-spi, linux-kernel, linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") 2021-01-14 13:22 ` Mark Brown @ 2021-01-14 13:42 ` Christophe Leroy 2021-01-14 13:59 ` Mark Brown 0 siblings, 1 reply; 8+ messages in thread From: Christophe Leroy @ 2021-01-14 13:42 UTC (permalink / raw) To: Mark Brown Cc: Sven Van Asbroeck, Linus Walleij, linux-spi, linux-kernel, linuxppc-dev 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() ? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") 2021-01-14 13:42 ` Christophe Leroy @ 2021-01-14 13:59 ` Mark Brown 0 siblings, 0 replies; 8+ messages in thread From: Mark Brown @ 2021-01-14 13:59 UTC (permalink / raw) To: Christophe Leroy Cc: Sven Van Asbroeck, Linus Walleij, linux-spi, linux-kernel, linuxppc-dev [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-14 14:00 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-13 8:49 SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors") Christophe Leroy 2021-01-13 12:33 ` Mark Brown 2021-01-14 11:27 ` Christophe Leroy 2021-01-14 11:59 ` Mark Brown 2021-01-14 12:33 ` Christophe Leroy 2021-01-14 13:22 ` Mark Brown 2021-01-14 13:42 ` Christophe Leroy 2021-01-14 13:59 ` Mark Brown
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).