Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
Hi Matthias, On Mon, Sep 7, 2020 at 4:40 AM Matthias Schiffer 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.
Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
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
> 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: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
Hi Mark,
On Fri, Sep 4, 2020 at 12:05 PM Mark Brown 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_MISO0x100b1
MX6QDL_PAD_KEY_ROW0__ECSPI1_MOSI0x100b1
MX6QDL_PAD_KEY_COL0__ECSPI1_SCLK0x100b1
- 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: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
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. signature.asc Description: PGP signature
Re: (EXT) Re: (EXT) Re: [PATCH] spi-imx: remove num-cs support, set num_chipselect to 4
On Fri, 2020-09-04 at 10:57 -0300, Fabio Estevam wrote: > On Fri, Sep 4, 2020 at 10:02 AM Matthias Schiffer > 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.
