[PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Matthias Schiffer]

The num-cs property is not considered useful, and no in-tree Device
Trees define it for spi-imx.

The default value to be used when no cs-gpios are defined is set to 4 to
give access to all native CS pins of modern i.MX SoCs (i.MX6 and newer).

In older SoCs, the number of CS pins varies (for example the i.MX27 has 3
CS pins on CSPI1 and CSPI2, and only a single CS on CSPI3). Attempting
to use the nonexisting CS pin would be an easy to notice DT
misconfiguration; making the driver catch this doesn't seem worthwhile.

Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/spi/spi-imx.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 197f60632072..aece8482739b 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1581,7 +1581,6 @@ static int spi_imx_probe(struct platform_device *pdev)
 	const struct spi_imx_devtype_data *devtype_data = of_id ? of_id->data :
 		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
 	bool slave_mode;
-	u32 val;
 
 	slave_mode = devtype_data->has_slavemode &&
 			of_property_read_bool(np, "spi-slave");
@@ -1605,6 +1604,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
 	master->bus_num = np ? -1 : pdev->id;
 	master->use_gpio_descriptors = true;
+	master->num_chipselect = 4;
 
 	spi_imx = spi_master_get_devdata(master);
 	spi_imx->bitbang.master = master;
@@ -1613,17 +1613,6 @@ static int spi_imx_probe(struct platform_device *pdev)
 
 	spi_imx->devtype_data = devtype_data;
 
-	/*
-	 * Get number of chip selects from device properties. This can be
-	 * coming from device tree or boardfiles, if it is not defined,
-	 * a default value of 3 chip selects will be used, as all the legacy
-	 * board files have <= 3 chip selects.
-	 */
-	if (!device_property_read_u32(&pdev->dev, "num-cs", &val))
-		master->num_chipselect = val;
-	else
-		master->num_chipselect = 3;
-
 	spi_imx->bitbang.setup_transfer = spi_imx_setupxfer;
 	spi_imx->bitbang.txrx_bufs = spi_imx_transfer;
 	spi_imx->bitbang.master->setup = spi_imx_setup;
-- 
2.17.1

Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Fabio Estevam]

Hi Matthias,

On Thu, Sep 3, 2020 at 11:40 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:
>
> The num-cs property is not considered useful, and no in-tree Device
> Trees define it for spi-imx.
>
> The default value to be used when no cs-gpios are defined is set to 4 to
> give access to all native CS pins of modern i.MX SoCs (i.MX6 and newer).
>
> In older SoCs, the number of CS pins varies (for example the i.MX27 has 3
> CS pins on CSPI1 and CSPI2, and only a single CS on CSPI3). Attempting
> to use the nonexisting CS pin would be an easy to notice DT
> misconfiguration; making the driver catch this doesn't seem worthwhile.
>
> Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> ---
>  drivers/spi/spi-imx.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 197f60632072..aece8482739b 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1581,7 +1581,6 @@ static int spi_imx_probe(struct platform_device *pdev)
>         const struct spi_imx_devtype_data *devtype_data = of_id ? of_id->data :
>                 (struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
>         bool slave_mode;
> -       u32 val;
>
>         slave_mode = devtype_data->has_slavemode &&
>                         of_property_read_bool(np, "spi-slave");
> @@ -1605,6 +1604,7 @@ static int spi_imx_probe(struct platform_device *pdev)
>         master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
>         master->bus_num = np ? -1 : pdev->id;
>         master->use_gpio_descriptors = true;
> +       master->num_chipselect = 4;

On an imx6q-sabresd, which only has one SPI chip-select via GPIO, this
makes the SPI core to understand that it has 4 chip selects.

From spi_get_gpio_descs() in drivers/spi/spi.c:

ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);

It is 4 now after your patch, it was 3 after 8cdcd8aeee28 ("spi:
imx/fsl-lpspi: Convert to GPIO descriptors") and 1 before such commit.

Couldn't we just remove master->num_chipselect from the spi-imx.c driver?

Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Matthias Schiffer]

On Fri, 2020-09-04 at 09:35 -0300, Fabio Estevam wrote:
> Hi Matthias,
> 
> On Thu, Sep 3, 2020 at 11:40 AM Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> > 
> > The num-cs property is not considered useful, and no in-tree Device
> > Trees define it for spi-imx.
> > 
> > The default value to be used when no cs-gpios are defined is set to
> > 4 to
> > give access to all native CS pins of modern i.MX SoCs (i.MX6 and
> > newer).
> > 
> > In older SoCs, the number of CS pins varies (for example the i.MX27
> > has 3
> > CS pins on CSPI1 and CSPI2, and only a single CS on CSPI3).
> > Attempting
> > to use the nonexisting CS pin would be an easy to notice DT
> > misconfiguration; making the driver catch this doesn't seem
> > worthwhile.
> > 
> > Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com
> > >
> > ---
> >  drivers/spi/spi-imx.c | 13 +------------
> >  1 file changed, 1 insertion(+), 12 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> > index 197f60632072..aece8482739b 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -1581,7 +1581,6 @@ static int spi_imx_probe(struct
> > platform_device *pdev)
> >         const struct spi_imx_devtype_data *devtype_data = of_id ?
> > of_id->data :
> >                 (struct spi_imx_devtype_data *)pdev->id_entry-
> > >driver_data;
> >         bool slave_mode;
> > -       u32 val;
> > 
> >         slave_mode = devtype_data->has_slavemode &&
> >                         of_property_read_bool(np, "spi-slave");
> > @@ -1605,6 +1604,7 @@ static int spi_imx_probe(struct
> > platform_device *pdev)
> >         master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32);
> >         master->bus_num = np ? -1 : pdev->id;
> >         master->use_gpio_descriptors = true;
> > +       master->num_chipselect = 4;
> 
> On an imx6q-sabresd, which only has one SPI chip-select via GPIO,
> this
> makes the SPI core to understand that it has 4 chip selects.
> 
> From spi_get_gpio_descs() in drivers/spi/spi.c:
> 
> ctlr->num_chipselect = max_t(int, nb, ctlr->num_chipselect);
> 
> It is 4 now after your patch, it was 3 after 8cdcd8aeee28 ("spi:
> imx/fsl-lpspi: Convert to GPIO descriptors") and 1 before such
> commit.
> 
> Couldn't we just remove master->num_chipselect from the spi-imx.c
> driver?

This would make num_chipselect default to 1 again (set by
__spi_alloc_controller()), breaking every i.MX board that uses more
than 1 native CS.

I'm aware that using cs-gpios instead of native CS is probably a good
idea in any case, as the native CS of this SPI controller is kinda
flaky (and at a glance it looks like all in-tree DTs do this; not sure
about board files that don't use DTs?), but I'm not convinced that
breaking native CS support completely is desirable either.

Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

On Fri, Sep 04, 2020 at 09:35:38AM -0300, Fabio Estevam wrote:
> On Thu, Sep 3, 2020 at 11:40 AM Matthias Schiffer

> > +       master->num_chipselect = 4;

> On an imx6q-sabresd, which only has one SPI chip-select via GPIO, this
> makes the SPI core to understand that it has 4 chip selects.

We shouldn't do anything with the extra chip selects though?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Fabio Estevam]

On Fri, Sep 4, 2020 at 10:02 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:

> This would make num_chipselect default to 1 again (set by
> __spi_alloc_controller()), breaking every i.MX board that uses more
> than 1 native CS.

Which boards are that? Are you referring to non-DT i.MX boards?

If so, I have sent a patch removing all non-DT i.MX boards.

> I'm aware that using cs-gpios instead of native CS is probably a good
> idea in any case, as the native CS of this SPI controller is kinda
> flaky (and at a glance it looks like all in-tree DTs do this; not sure
> about board files that don't use DTs?), but I'm not convinced that
> breaking native CS support completely is desirable either.

Initial i.MX chips with this SPI IP had issues in using chip-select in
native mode and GPIO chip-select has been used since them.

Do we have i.MX dts that make use of native chip select?

Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Matthias Schiffer]

On Fri, 2020-09-04 at 10:57 -0300, Fabio Estevam wrote:
> On Fri, Sep 4, 2020 at 10:02 AM Matthias Schiffer
> <matthias.schiffer@ew.tq-group.com> wrote:
> 
> > This would make num_chipselect default to 1 again (set by
> > __spi_alloc_controller()), breaking every i.MX board that uses more
> > than 1 native CS.
> 
> Which boards are that? Are you referring to non-DT i.MX boards?
> 
> If so, I have sent a patch removing all non-DT i.MX boards.
> 
> > I'm aware that using cs-gpios instead of native CS is probably a
> > good
> > idea in any case, as the native CS of this SPI controller is kinda
> > flaky (and at a glance it looks like all in-tree DTs do this; not
> > sure
> > about board files that don't use DTs?), but I'm not convinced that
> > breaking native CS support completely is desirable either.
> 
> Initial i.MX chips with this SPI IP had issues in using chip-select
> in
> native mode and GPIO chip-select has been used since them.
> 
> Do we have i.MX dts that make use of native chip select?

As mentioned in my previous mail, all in-tree DTS use cs-gpios (unless
I've overlooked something - I grepped for CSPIn_SSm pinmuxings), so
removing support for native CS should not break anything we know of.

Nevertheless, I don't see why we should deliberately remove the native
CS support - my understanding was that we try to avoid breaking changes
to DT interpretation even for unknown/out-of-tree DTS.

Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

On Fri, Sep 04, 2020 at 04:34:43PM +0200, Matthias Schiffer wrote:

> Nevertheless, I don't see why we should deliberately remove the native
> CS support - my understanding was that we try to avoid breaking changes
> to DT interpretation even for unknown/out-of-tree DTS.

Yes, we should try to maintain compatibility for anyone that was using
it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Fabio Estevam]

Hi Mark,

On Fri, Sep 4, 2020 at 12:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Fri, Sep 04, 2020 at 04:34:43PM +0200, Matthias Schiffer wrote:
>
> > Nevertheless, I don't see why we should deliberately remove the native
> > CS support - my understanding was that we try to avoid breaking changes
> > to DT interpretation even for unknown/out-of-tree DTS.
>
> Yes, we should try to maintain compatibility for anyone that was using
> it.

We are not breaking compatibility.

Prior to 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert to GPIO
descriptors")  num_chipselect was 1 for all device tree users.
i.MX board files will be removed, so we don't need to worry about them.

What will cause breakage is to say that the driver supports the native
chip-select.

I have just done a quick test on an imx6q-sabresd.

With the original dts that uses cs-gpios the SPI NOR is correctly probed:

[    5.402627] spi-nor spi0.0: m25p32 (4096 Kbytes)

However, using native chip select with this dts change:

--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -189,7 +189,6 @@
 };

 &ecspi1 {
-       cs-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_ecspi1>;
        status = "okay";
@@ -506,7 +505,7 @@
                                MX6QDL_PAD_KEY_COL1__ECSPI1_MISO        0x100b1
                                MX6QDL_PAD_KEY_ROW0__ECSPI1_MOSI        0x100b1
                                MX6QDL_PAD_KEY_COL0__ECSPI1_SCLK        0x100b1
-                               MX6QDL_PAD_KEY_ROW1__GPIO4_IO09         0x1b0b0
+                               MX6QDL_PAD_KEY_ROW1__ECSPI1_SS0         0x1b0b0
                        >;
                };

Causes SPI NOR probe to fail:

[    5.388704] spi-nor spi0.0: unrecognized JEDEC id bytes: 00 00 00 00 00 00

That's why I prefer we do not advertise that we can use the native
chip-select with this driver.

Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Matthias Schiffer]

On Fri, 2020-09-04 at 12:42 -0300, Fabio Estevam wrote:
> Hi Mark,
> 
> On Fri, Sep 4, 2020 at 12:05 PM Mark Brown <broonie@kernel.org>
> wrote:
> > 
> > On Fri, Sep 04, 2020 at 04:34:43PM +0200, Matthias Schiffer wrote:
> > 
> > > Nevertheless, I don't see why we should deliberately remove the
> > > native
> > > CS support - my understanding was that we try to avoid breaking
> > > changes
> > > to DT interpretation even for unknown/out-of-tree DTS.
> > 
> > Yes, we should try to maintain compatibility for anyone that was
> > using
> > it.
> 
> We are not breaking compatibility.
> 
> Prior to 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert to GPIO
> descriptors")  num_chipselect was 1 for all device tree users.
> i.MX board files will be removed, so we don't need to worry about
> them.
> 
> What will cause breakage is to say that the driver supports the
> native
> chip-select.
> 
> I have just done a quick test on an imx6q-sabresd.
> 
> With the original dts that uses cs-gpios the SPI NOR is correctly
> probed:
> 
> [    5.402627] spi-nor spi0.0: m25p32 (4096 Kbytes)
> 
> However, using native chip select with this dts change:
> 
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -189,7 +189,6 @@
>  };
> 
>  &ecspi1 {
> -       cs-gpios = <&gpio4 9 GPIO_ACTIVE_LOW>;
>         pinctrl-names = "default";
>         pinctrl-0 = <&pinctrl_ecspi1>;
>         status = "okay";
> @@ -506,7 +505,7 @@
>                                 MX6QDL_PAD_KEY_COL1__ECSPI1_MISO     
>    0x100b1
>                                 MX6QDL_PAD_KEY_ROW0__ECSPI1_MOSI     
>    0x100b1
>                                 MX6QDL_PAD_KEY_COL0__ECSPI1_SCLK     
>    0x100b1
> -                               MX6QDL_PAD_KEY_ROW1__GPIO4_IO09      
>    0x1b0b0
> +                               MX6QDL_PAD_KEY_ROW1__ECSPI1_SS0      
>    0x1b0b0
>                         >;
>                 };
> 
> Causes SPI NOR probe to fail:
> 
> [    5.388704] spi-nor spi0.0: unrecognized JEDEC id bytes: 00 00 00
> 00 00 00
> 
> That's why I prefer we do not advertise that we can use the native
> chip-select with this driver.


My rationale here is the following: As broken as the native CS of these
controllers is, it isn't an unreasonable assumption that it is working
fine with *some* devices or for some usecases - after all the support
was implemented at some point, and has existed for a long time now. If
we really want to remove this feature, a deprecation period with a
warning message seems like the proper way to deal with this.

Hypothetically, existing out-of-tree DTS could have used the native CS
with num-cs set to 4. Always setting num_chipselect to 4 ensures that
we don't break such DTS with the num-cs removal.

Kind regards,
Matthias

Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4

[Fabio Estevam]

Hi Matthias,

On Mon, Sep 7, 2020 at 4:40 AM Matthias Schiffer
<matthias.schiffer@ew.tq-group.com> wrote:

> My rationale here is the following: As broken as the native CS of these
> controllers is, it isn't an unreasonable assumption that it is working
> fine with *some* devices or for some usecases - after all the support
> was implemented at some point, and has existed for a long time now. If
> we really want to remove this feature, a deprecation period with a
> warning message seems like the proper way to deal with this.
>
> Hypothetically, existing out-of-tree DTS could have used the native CS
> with num-cs set to 4. Always setting num_chipselect to 4 ensures that
> we don't break such DTS with the num-cs removal.

I still think this is more of a theoretical issue rather than a real
use case one.

Anyway, I have a proposal that I think will make both of us happy :-)

I will submit a patch shortly.