Re: [PATCHv3 1/2] spi: imx: fix use of native chip-selects with devicetree

Greg Ungerer <gerg@xxxxxxxxxxxxxx> · Thu, 10 Aug 2017 21:35:15 +1000

On 10/08/17 19:35, Vladimir Zapolskiy wrote:
Hi Oleksij,

On 08/10/2017 09:09 AM, Oleksij Rempel wrote:
Hi Greg,

On 09.08.2017 15:00, Greg Ungerer wrote:
Hi Qleksij,

On 24/07/17 16:21, Oleksij Rempel wrote:
On Thu, Jul 20, 2017 at 11:00:49PM +1000, Greg Ungerer wrote:
Hi Oleksij,

On 20/07/17 16:34, Oleksij Rempel wrote:
Hi Greg,

On Tue, Jul 18, 2017 at 09:53:58PM -0300, Fabio Estevam wrote:
Adding Pengutronix folks on Cc.

On Tue, Jul 11, 2017 at 1:22 AM, Greg Ungerer <gerg@xxxxxxxxxxxxxx>
wrote:
The commonly used mechanism of specifying the hardware or native
chip-select on an SPI device in devicetree (that is "cs-gpios = <0>")
does not result in the native chip-select being configured for use.
So external SPI devices that require use of the native chip-select
will not work.

You can successfully specify native chip-selects if using a platform
setup by specifying the cs-gpio as negative offset by 32. And that
works correctly. You cannot use the same method in devicetree.

The logic in the spi-imx.c driver during probe uses core spi function
of_spi_register_master() in spi.c to parse the "cs-gpios"
devicetree tag.
For valid GPIO values that will be recorded for use, all other
entries in
the cs_gpios list will be set to -ENOENT. So entries like "<0>"
will be
set to -ENOENT in the cs_gpios list.

When the SPI device registers are setup the code will use the GPIO
listed in the cs_gpios list for the desired chip-select. If the
cs_gpio
is less then 0 then it is intended to be for a native chip-select,
and
its cs_gpio value is added to 32 to get the chipselect number to use.
Problem is that with devicetree this can only ever be -ENOENT (which
is -2), and that alone results in an invalid chip-select number.
But also
doesn't allow selection of the native chip-select at all.

To fix, if the cs_gpio specified for this spi device is not a
valid GPIO then use the "chip_select" (that is the native chip-select
number) for hardware setup.

Signed-off-by: Greg Ungerer <gerg@xxxxxxxxxxxxxx>
---
    drivers/spi/spi-imx.c | 8 ++++----
    1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index b402530..f4fe66c 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -524,8 +524,8 @@ static int mx31_config(struct spi_device *spi,
struct spi_imx_config *config)
                   reg |= MX31_CSPICTRL_POL;
           if (spi->mode & SPI_CS_HIGH)
                   reg |= MX31_CSPICTRL_SSPOL;
-       if (spi->cs_gpio < 0)
-               reg |= (spi->cs_gpio + 32) <<
+       if (!gpio_is_valid(spi->cs_gpio))
+               reg |= (spi->chip_select) <<
                           (is_imx35_cspi(spi_imx) ?
MX35_CSPICTRL_CS_SHIFT :

MX31_CSPICTRL_CS_SHIFT);

@@ -616,8 +616,8 @@ static int mx21_config(struct spi_device *spi,
struct spi_imx_config *config)
                   reg |= MX21_CSPICTRL_POL;
           if (spi->mode & SPI_CS_HIGH)
                   reg |= MX21_CSPICTRL_SSPOL;
-       if (spi->cs_gpio < 0)
-               reg |= (spi->cs_gpio + 32) << MX21_CSPICTRL_CS_SHIFT;
+       if (!gpio_is_valid(spi->cs_gpio))
+               reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT;

           writel(reg, spi_imx->base + MXC_CSPICTRL);


hm... do I see this correctly, all native chip_selects should
be registered before gpio based CS?

I don't follow. The "<0>" must be in the position in the list where
you want to use the native chip select. You can't arbitrarily change
the order.


For example like this?
cs-gpios = <0>, <&gpio1 1 0>, <&gpio1 2 0>;

Looks like we don't have any sanity checks for this kind of
configuration:
cs-gpios = <&gpio1 1 0>, <&gpio1 2 0>, <0>;

The chip_select is sanity checked in spi_add_device().


We may shift some wired numbers here:
reg |= spi->chip_select << MX21_CSPICTRL_CS_SHIFT;

I am not sure I see how that can be the case?

old and new version of iMX have different amount of native CS.
I can't find the code which is actually checking if we use right native
CS-index.
May be i'm blind :)

I don't think I entirely understand what you are saying. The code at the
top of spi_add_device() [drivers/spi/spi.c] looks like this:

         /* Chipselects are numbered 0..max; validate. */
         if (spi->chip_select >= ctlr->num_chipselect) {
                 dev_err(dev, "cs%d >= max %d\n", spi->chip_select,
                         ctlr->num_chipselect);
                 return -EINVAL;
         }

So it will range check the spi device (spi->chip_select) to be within
the range valid for this SPI controller. That is the very same
spi->chip_select that is used in spi-imx.c to set the register bits
when using a native chip select.

Correct. This is how ctlr->num_chipselect initialized:

          nb = of_gpio_named_count(np, "cs-gpios");
          ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);

it will take the count of cs-gpios.

The i.MX233 has 3 native CSs (SSn) and i.MX6D/Q has 4 CSs - controlled
by 2 bits. Lets assume in both cases we wish to use 5CSs, some of them
are GPIOs.

We will use same line for devicetree on i.MX233 and i.MX6D/Q:
cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <0>, <&gpio1 4 0>;

If i see it correctly, spi.c and imx-spi.c will just take it. But in
case of i.MX6 it should work and on i.MX233 it should silently fail.

Errors in DTB (or platform data) may confuse a driver and lead to runtime
misbehaviour. You describe an error in a board DTB, which is definitely
better to handle in the SPI driver, but I don't think it is strictly
mandatory to do it, because DTB errors are supposed to be fixed in DTB.

May be one day a formal check of DTBs against Documentation/devicetree
descriptions will be added and such DTB errors could be captured on DTB
compilation stage.

I completely agree with Vladmir here. Since "cs-gpios" defines the
number of chip selects, as per the code you point out, it is the range
limit. So if a DTB defines it wrongly then you can expect some things
not to work right. The spi code quite rightly relies on the DTB
definitions to be correct for proper operation.


And in this case:
cs-gpios = <&gpio1 0 0>, <&gpio1 1 0>, <&gpio1 2 0>, <&gpio1 3 0>, <0>;

we should produce a 3 bit value b100 which will be shifted left and
"or"-ed with other ctrl bits.

So the register settings will be wrong and the device will not work.
You can't really expect any other behavior from an incorrect DTB.

Regards
Greg



--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html






