Re: [PATCH 11/17] spi: dw: Fix native CS being unset

On Fri, May 8, 2020 at 3:31 PM Serge Semin
[off-list ref] wrote:

Commit 6e0a32d6f376 ("spi: dw: Fix default polarity of native
chipselect") attempted to fix the problem when GPIO active-high
chip-select is utilized to communicate with some SPI slave. It fixed
the problem, but broke the normal native CS support. At the same time
the reversion commit ada9e3fcc175 ("spi: dw: Correct handling of native
chipselect") didn't solve the problem either, since it just inverted
the set_cs() polarity perception without taking into account that
CS-high might be applicable. Here is what is done to finally fix the
problem.

I'm not sure this is the whole story.

I think Charles' fix made it work, and then commit
3e5ec1db8bfee845d9f8560d1c64aeaccd586398
"spi: Fix SPI_CS_HIGH setting when using native and GPIO CS"
fixed it broken again.

This commit will make sure only set SPI_CS_HIGH on a
spi_device if it is using a GPIO as CS. Before this change,
the core would set that on everything, and expect the
.set_cs() callback to cope.

I think we fixed that and that fix should have been undone
when applying commit 3e5ec1db8bfe.

So possibly Fixes: should be set only to this commit, so
that the fix is not backported to kernels without it.

DW SPI controller demands any native CS being set in order to proceed
with data transfer. So in order to activate the SPI communications we
must set any bit in the Slave Select DW SPI controller register no
matter whether the platform requests the GPIO- or native CS.

Ah-ha! Maybe we should even add a comment explaining that.
And that is why SPI_MASTER_GPIO_SS is set.

I suppose my naive understanding was:
"bit set to 1" = CS asserted (driven low)
"bit set to 0" = CS de-asserted  (driven high)
So that is not how this register works at all.

This commit fixes the problem for all described cases. So no matter
whether an SPI slave needs GPIO- or native-based CS with active-high
or low signal the corresponding bit will be set in SER.

Makes sense.

        struct dw_spi *dws = spi_controller_get_devdata(spi->controller);
        struct chip_data *chip = spi_get_ctldata(spi);
+       bool cs_high = !!(spi->mode & SPI_CS_HIGH);

        /* Chip select logic is inverted from spi_set_cs() */
        if (chip && chip->cs_control)
                chip->cs_control(!enable);

-       if (!enable)
+       if (cs_high == enable)
                dw_writel(dws, DW_SPI_SER, BIT(spi->chip_select));

This is the correct fix now but I an afraid not correct before
commit 3e5ec1db8bfe.

What I can't help but asking is: can the native chip select even
handle active high chip select if not backed by a GPIO?
Which register would set that polarity?

Yours,
Linus Walleij