[PATCH] spi: cadence: Correct handling of native chipselect

[PATCH] spi: cadence: Correct handling of native chipselect

[Charles Keepax]

To fix a regression on the Cadence SPI driver, this patch reverts
commit 6046f5407ff0 ("spi: cadence: Fix default polarity of native
chipselect").

This patch was not the correct fix for the issue. The SPI framework
calls the set_cs line with the logic level it desires on the chip select
line, as such the old is_high handling was correct. However, this was
broken by the fact that before commit 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH
setting when using native and GPIO CS") all controllers that offered
the use of a GPIO chip select had SPI_CS_HIGH applied, even for hardware
chip selects. This caused the value passed into the driver to be inverted.
Which unfortunately makes it look like a logical enable the chip select
value.

Since the core was corrected to not unconditionally apply SPI_CS_HIGH,
the Cadence driver, whilst using the hardware chip select, will deselect
the chip select every time we attempt to communicate with the device,
which results in failed communications.

Fixes: 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH setting when using native and GPIO CS")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/spi/spi-cadence.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index c36587b42e951..82a0ee09cbe14 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -168,16 +168,16 @@ static void cdns_spi_init_hw(struct cdns_spi *xspi)
 /**
  * cdns_spi_chipselect - Select or deselect the chip select line
  * @spi:	Pointer to the spi_device structure
- * @enable:	Select (1) or deselect (0) the chip select line
+ * @is_high:	Select(0) or deselect (1) the chip select line
  */
-static void cdns_spi_chipselect(struct spi_device *spi, bool enable)
+static void cdns_spi_chipselect(struct spi_device *spi, bool is_high)
 {
 	struct cdns_spi *xspi = spi_master_get_devdata(spi->master);
 	u32 ctrl_reg;
 
 	ctrl_reg = cdns_spi_read(xspi, CDNS_SPI_CR);
 
-	if (!enable) {
+	if (is_high) {
 		/* Deselect the slave */
 		ctrl_reg |= CDNS_SPI_CR_SSCTRL;
 	} else {
-- 
2.11.0

Re: [PATCH] spi: cadence: Correct handling of native chipselect

[Linus Walleij]

Hi Charles!

Thanks for finding this!

On Tue, Nov 26, 2019 at 5:41 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:

> To fix a regression on the Cadence SPI driver, this patch reverts
> commit 6046f5407ff0 ("spi: cadence: Fix default polarity of native
> chipselect").
>
> This patch was not the correct fix for the issue. The SPI framework
> calls the set_cs line with the logic level it desires on the chip select
> line, as such the old is_high handling was correct. However, this was
> broken by the fact that before commit 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH
> setting when using native and GPIO CS") all controllers that offered
> the use of a GPIO chip select had SPI_CS_HIGH applied, even for hardware
> chip selects. This caused the value passed into the driver to be inverted.
> Which unfortunately makes it look like a logical enable the chip select
> value.
>
> Since the core was corrected to not unconditionally apply SPI_CS_HIGH,
> the Cadence driver, whilst using the hardware chip select, will deselect
> the chip select every time we attempt to communicate with the device,
> which results in failed communications.
>
> Fixes: 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH setting when using native and GPIO CS")
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>

I kind of get it... I think.

I see it fixes a patch that I was not CC:ed on, which is a bit unfortunate
as it tries to fix something I wrote, but such things happen.

The original patch
f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
came with the assumption that native chip select handler needed
was to be converted to always expect a true (1) value to their
->set_cs() callbacks for asserting chip select, and this was one of
the drivers augmented to expect that.

As
3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH setting when using native and GPIO CS")
essentially undo that semantic change and switches back to
the old semantic, all the drivers that were converted to expect
a high input to their ->set_cs() callbacks for asserting CS need
to be reverted back as well, but that didn't happen.

So we need to fix not just cadence but also any other driver setting
->use_gpio_descriptors() and also supplying their own
->set_cs() callback and expecting this behaviour, or the fix
will have fixed broken a bunch of drivers.

But we are lucky: there aren't many of them.
In addition to spi-cadence.c this seems to affect only spi-dw.c
and I suppose that is what Gregory was using? Or
something else?

Yours,
Linus Walleij

Re: [PATCH] spi: cadence: Correct handling of native chipselect

[Charles Keepax]

On Wed, Nov 27, 2019 at 11:42:47AM +0100, Linus Walleij wrote:
> On Tue, Nov 26, 2019 at 5:41 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> The original patch
> f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
> came with the assumption that native chip select handler needed
> was to be converted to always expect a true (1) value to their
> ->set_cs() callbacks for asserting chip select, and this was one of
> the drivers augmented to expect that.
> 

Which is fine, I am not greatly invested in either symantics
of the set_cs callback although if we were changing that we
should have probably updated the kerneldoc comments for it.

Although I do have a question if that is that case what is the
expected way to handle the polarity of the chip select? Because
it seems to me you would end up with each driver checking the
SPI_CS_HIGH flag in set_cs and doing the invert locally, whereas
with the pass the logic level system the core can centralise that
inversion.

> As
> 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH setting when using native and GPIO CS")
> essentially undo that semantic change and switches back to
> the old semantic, all the drivers that were converted to expect
> a high input to their ->set_cs() callbacks for asserting CS need
> to be reverted back as well, but that didn't happen.
> 
> So we need to fix not just cadence but also any other driver setting
> ->use_gpio_descriptors() and also supplying their own
> ->set_cs() callback and expecting this behaviour, or the fix
> will have fixed broken a bunch of drivers.
> 
> But we are lucky: there aren't many of them.
> In addition to spi-cadence.c this seems to affect only spi-dw.c
> and I suppose that is what Gregory was using? Or
> something else?
> 

I will go do some digging and see what I can find.

Thanks,
Charles

Re: [PATCH] spi: cadence: Correct handling of native chipselect

[Linus Walleij]

On Wed, Nov 27, 2019 at 12:54 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Nov 27, 2019 at 11:42:47AM +0100, Linus Walleij wrote:
> > On Tue, Nov 26, 2019 at 5:41 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > The original patch
> > f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
> > came with the assumption that native chip select handler needed
> > was to be converted to always expect a true (1) value to their
> > ->set_cs() callbacks for asserting chip select, and this was one of
> > the drivers augmented to expect that.
> >
>
> Which is fine, I am not greatly invested in either symantics
> of the set_cs callback although if we were changing that we
> should have probably updated the kerneldoc comments for it.
>
> Although I do have a question if that is that case what is the
> expected way to handle the polarity of the chip select? Because
> it seems to me you would end up with each driver checking the
> SPI_CS_HIGH flag in set_cs and doing the invert locally, whereas
> with the pass the logic level system the core can centralise that
> inversion.

I guess I thought it was logical (hah!) that the core provide
a signal that is true if a condition is asserted, and then the
driver decides whether that drives the line low or high.

But just saying that the callback sets the physical level out
to the device works too, so the patch as-is:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

> > As
> > 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH setting when using native and GPIO CS")
> > essentially undo that semantic change and switches back to
> > the old semantic, all the drivers that were converted to expect
> > a high input to their ->set_cs() callbacks for asserting CS need
> > to be reverted back as well, but that didn't happen.
> >
> > So we need to fix not just cadence but also any other driver setting
> > ->use_gpio_descriptors() and also supplying their own
> > ->set_cs() callback and expecting this behaviour, or the fix
> > will have fixed broken a bunch of drivers.
> >
> > But we are lucky: there aren't many of them.
> > In addition to spi-cadence.c this seems to affect only spi-dw.c
> > and I suppose that is what Gregory was using? Or
> > something else?
> >
>
> I will go do some digging and see what I can find.

Thanks.

Yours,
Linus Walleij

Re: [PATCH] spi: cadence: Correct handling of native chipselect

[Charles Keepax]

On Wed, Nov 27, 2019 at 12:59:34PM +0100, Linus Walleij wrote:
> On Wed, Nov 27, 2019 at 12:54 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Nov 27, 2019 at 11:42:47AM +0100, Linus Walleij wrote:
> > > On Tue, Nov 26, 2019 at 5:41 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > > But we are lucky: there aren't many of them.
> > > In addition to spi-cadence.c this seems to affect only spi-dw.c
> > > and I suppose that is what Gregory was using? Or
> > > something else?
> > >
> > I will go do some digging and see what I can find.

Yeah so looks to me like spi-dw is the only other part affected,
and it probably wants a similar revert done to fix it. It is a
little more complex as it has this additional cs_control
callback, but there don't appear to be any in tree users for that
(which I can find). So I am guessing any out of tree users
probably broke when the logic was first changed so the revert
probably helps them too, unless they have already changed there
callbacks in which case it will break them again.

Anyways I will send the revert and hopefully some people who use
the driver can test it for us.

Thanks,
Charles

Applied "spi: cadence: Correct handling of native chipselect" to the spi tree

[Mark Brown]

The patch

   spi: cadence: Correct handling of native chipselect

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-5.5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 61acd19f9c56fa0809285346bd0bd4a926ab0da0 Mon Sep 17 00:00:00 2001
From: Charles Keepax <ckeepax@opensource.cirrus.com>
Date: Tue, 26 Nov 2019 16:41:40 +0000
Subject: [PATCH] spi: cadence: Correct handling of native chipselect

To fix a regression on the Cadence SPI driver, this patch reverts
commit 6046f5407ff0 ("spi: cadence: Fix default polarity of native
chipselect").

This patch was not the correct fix for the issue. The SPI framework
calls the set_cs line with the logic level it desires on the chip select
line, as such the old is_high handling was correct. However, this was
broken by the fact that before commit 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH
setting when using native and GPIO CS") all controllers that offered
the use of a GPIO chip select had SPI_CS_HIGH applied, even for hardware
chip selects. This caused the value passed into the driver to be inverted.
Which unfortunately makes it look like a logical enable the chip select
value.

Since the core was corrected to not unconditionally apply SPI_CS_HIGH,
the Cadence driver, whilst using the hardware chip select, will deselect
the chip select every time we attempt to communicate with the device,
which results in failed communications.

Fixes: 3e5ec1db8bfe ("spi: Fix SPI_CS_HIGH setting when using native and GPIO CS")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Link: https://lore.kernel.org/r/20191126164140.6240-1-ckeepax@opensource.cirrus.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-cadence.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
index c36587b42e95..82a0ee09cbe1 100644
--- a/drivers/spi/spi-cadence.c
+++ b/drivers/spi/spi-cadence.c
@@ -168,16 +168,16 @@ static void cdns_spi_init_hw(struct cdns_spi *xspi)
 /**
  * cdns_spi_chipselect - Select or deselect the chip select line
  * @spi:	Pointer to the spi_device structure
- * @enable:	Select (1) or deselect (0) the chip select line
+ * @is_high:	Select(0) or deselect (1) the chip select line
  */
-static void cdns_spi_chipselect(struct spi_device *spi, bool enable)
+static void cdns_spi_chipselect(struct spi_device *spi, bool is_high)
 {
 	struct cdns_spi *xspi = spi_master_get_devdata(spi->master);
 	u32 ctrl_reg;
 
 	ctrl_reg = cdns_spi_read(xspi, CDNS_SPI_CR);
 
-	if (!enable) {
+	if (is_high) {
 		/* Deselect the slave */
 		ctrl_reg |= CDNS_SPI_CR_SSCTRL;
 	} else {
-- 
2.20.1