| Message ID | 20200903144028.20416-1-matthias.schiffer@ew.tq-group.com |
|---|---|
| State | New |
| Headers | show |
| Series | spi-imx: remove num-cs support, set num_chipselect to 4 | expand |
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.
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?
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.
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.
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.
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;
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(-)