| Message ID | 20191126164140.6240-1-ckeepax@opensource.cirrus.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 61acd19f9c56fa0809285346bd0bd4a926ab0da0 |
| Headers | show |
| Series | spi: cadence: Correct handling of native chipselect | expand |
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
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
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
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
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 {
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(-)