| Message ID | 20200824091013.20640-2-matthias.schiffer@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ARM: dts: imx6qdl: tqma6: fix indentation | expand |
Hi Matthias, On Mon, Aug 24, 2020 at 6:10 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi > index 9513020ddd1a..7aaae83c1fae 100644 > --- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi > @@ -20,7 +20,7 @@ > &ecspi1 { > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_ecspi1>; > - fsl,spi-num-chipselects = <1>; > + num-cs = <1>; You could simply remove fsl,spi-num-chipselects without passing num-cs. The spi core is able to count the number of chipselects passed via cs-gpios in the device tree.
On Mon, 2020-08-24 at 18:36 -0300, Fabio Estevam wrote: > Hi Matthias, > > On Mon, Aug 24, 2020 at 6:10 AM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi > > b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi > > index 9513020ddd1a..7aaae83c1fae 100644 > > --- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi > > +++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi > > @@ -20,7 +20,7 @@ > > &ecspi1 { > > pinctrl-names = "default"; > > pinctrl-0 = <&pinctrl_ecspi1>; > > - fsl,spi-num-chipselects = <1>; > > + num-cs = <1>; > > You could simply remove fsl,spi-num-chipselects without passing num- > cs. > > The spi core is able to count the number of chipselects passed via > cs-gpios in the device tree. Hmm, unless I'm overlooking something, this is not going to work: - spi_get_gpio_descs() sets num_chipselect to the maximum of the num_chipselect set in the driver and the number of cs-gpios - spi_imx_probe() sets num_chipselect to 3 if not specified in the device tree So I think we would end up with 3 instead of 1 chipselect. Kind regards, Matthias
Hi Matthias, On Tue, Aug 25, 2020 at 4:22 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > Hmm, unless I'm overlooking something, this is not going to work: > > - spi_get_gpio_descs() sets num_chipselect to the maximum of the > num_chipselect set in the driver and the number of cs-gpios > > - spi_imx_probe() sets num_chipselect to 3 if not specified in the > device tree > > So I think we would end up with 3 instead of 1 chipselect. Oh, this has changed recently in 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert to GPIO descriptors"): .... - } else { - u32 num_cs; - - if (!of_property_read_u32(np, "num-cs", &num_cs)) - master->num_chipselect = num_cs; - /* If not preset, default value of 1 is used */ Initially, if num-cs was not present the default value for num_chipselect was 1. - } + /* + * 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; Now it became 3. I think this is a driver issue and we should fix the driver instead of requiring to pass num-cs to the device tree. num-cs is not even documented in the spi-imx binding.
On Tue, 2020-08-25 at 11:24 -0300, Fabio Estevam wrote: > Hi Matthias, > > On Tue, Aug 25, 2020 at 4:22 AM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > Hmm, unless I'm overlooking something, this is not going to work: > > > > - spi_get_gpio_descs() sets num_chipselect to the maximum of the > > num_chipselect set in the driver and the number of cs-gpios > > > > - spi_imx_probe() sets num_chipselect to 3 if not specified in the > > device tree > > > > So I think we would end up with 3 instead of 1 chipselect. > > Oh, this has changed recently in 8cdcd8aeee281 ("spi: imx/fsl-lpspi: > Convert to GPIO descriptors"): > .... > > - } else { > - u32 num_cs; > - > - if (!of_property_read_u32(np, "num-cs", &num_cs)) > - master->num_chipselect = num_cs; > - /* If not preset, default value of 1 is used */ > > Initially, if num-cs was not present the default value for > num_chipselect was 1. > > - } > + /* > + * 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; > > Now it became 3. > > I think this is a driver issue and we should fix the driver instead > of > requiring to pass num-cs to the device tree. > > > num-cs is not even documented in the spi-imx binding. Makes sense. Does the following logic sound correct? - If num-cs is set, use that (and add it to the docs) - If num-cs is unset, use the number of cs-gpios - If num-cs is unset and no cs-gpios are defined, use a driver-provided default I'm not sure if 3 is a particularly useful default either, but it seems it was chosen to accommodate boards that previously set this via platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7) have 4 internal CS pins per ECSPI instance, so maybe the driver should use that as its default instead?
On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > Makes sense. Does the following logic sound correct? > > - If num-cs is set, use that (and add it to the docs) I would not add num-cs to the docs. As far as I can see there is no imx dts that uses num-cs currently. > - If num-cs is unset, use the number of cs-gpios > - If num-cs is unset and no cs-gpios are defined, use a driver-provided > default > > > I'm not sure if 3 is a particularly useful default either, but it seems > it was chosen to accommodate boards that previously set this via > platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7) have 4 > internal CS pins per ECSPI instance, so maybe the driver should use > that as its default instead? I think it is time to get rid of i.MX board files. I will try to work on this when I have a chance. bout using 4 as default chip select number, please also check some older SoCs like imx25, imx35, imx51, imx53, etc
On Tue, 2020-08-25 at 14:16 -0300, Fabio Estevam wrote: > On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > Makes sense. Does the following logic sound correct? > > > > - If num-cs is set, use that (and add it to the docs) > > I would not add num-cs to the docs. As far as I can see there is no > imx dts that uses num-cs currently. But the previous platform data that was removed in 8cdcd8aeee281 ("spi: imx/fsl-lpspi: Convert to GPIO descriptors") set different values for different boards. So maybe some DTS should be using num-cs? > > > - If num-cs is unset, use the number of cs-gpios > > - If num-cs is unset and no cs-gpios are defined, use a driver- > > provided > > default > > > > > > I'm not sure if 3 is a particularly useful default either, but it > > seems > > it was chosen to accommodate boards that previously set this via > > platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7) > > have 4 > > internal CS pins per ECSPI instance, so maybe the driver should use > > that as its default instead? > > I think it is time to get rid of i.MX board files. I will try to work > on this when I have a chance. > > bout using 4 as default chip select number, please also check some > older SoCs like imx25, imx35, imx51, imx53, etc Hmm, I just checked i.MX28, and it has only 3 chip selects per instance.
On Wed, 2020-08-26 at 12:32 +0200, Matthias Schiffer wrote: > On Tue, 2020-08-25 at 14:16 -0300, Fabio Estevam wrote: > > On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer > > <matthias.schiffer@ew.tq-group.com> wrote: > > > > > Makes sense. Does the following logic sound correct? > > > > > > - If num-cs is set, use that (and add it to the docs) > > > > I would not add num-cs to the docs. As far as I can see there is no > > imx dts that uses num-cs currently. > > But the previous platform data that was removed in 8cdcd8aeee281 > ("spi: > imx/fsl-lpspi: Convert to GPIO descriptors") set different values for > different boards. So maybe some DTS should be using num-cs? > > > > > > > - If num-cs is unset, use the number of cs-gpios > > > - If num-cs is unset and no cs-gpios are defined, use a driver- > > > provided > > > default > > > > > > > > > I'm not sure if 3 is a particularly useful default either, but it > > > seems > > > it was chosen to accommodate boards that previously set this via > > > platform data. All SoCs I've checked (i.MX6Q/DL, i.MX6UL, i.MX7) > > > have 4 > > > internal CS pins per ECSPI instance, so maybe the driver should > > > use > > > that as its default instead? > > > > I think it is time to get rid of i.MX board files. I will try to > > work > > on this when I have a chance. > > > > bout using 4 as default chip select number, please also check some > > older SoCs like imx25, imx35, imx51, imx53, etc > > Hmm, I just checked i.MX28, and it has only 3 chip selects per > instance. Ah sorry, I got confused, the i.MX28 has a different SPI IP.
Hi Matthias, On Wed, Aug 26, 2020 at 7:32 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > But the previous platform data that was removed in 8cdcd8aeee281 ("spi: > imx/fsl-lpspi: Convert to GPIO descriptors") set different values for > different boards. So maybe some DTS should be using num-cs? Could you provide an example of an imx dts that should use num-cs?
On Wed, 2020-08-26 at 07:59 -0300, Fabio Estevam wrote: > Hi Matthias, > > On Wed, Aug 26, 2020 at 7:32 AM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > But the previous platform data that was removed in 8cdcd8aeee281 > > ("spi: > > imx/fsl-lpspi: Convert to GPIO descriptors") set different values > > for > > different boards. So maybe some DTS should be using num-cs? > > Could you provide an example of an imx dts that should use num-cs? I'm not sure, does anything break when num_chipselect is set too high? Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1 for spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that it would make sense to add this as num-cs to arch/arm/boot/dts/imx31-lite.dts.
On Wed, Aug 26, 2020 at 8:54 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1 for > spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that it > would make sense to add this as num-cs to > arch/arm/boot/dts/imx31-lite.dts. Or just pass cs-gpios instead?
On Wed, 2020-08-26 at 10:01 -0300, Fabio Estevam wrote: > On Wed, Aug 26, 2020 at 8:54 AM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > Before 8cdcd8aeee281, num_chipselect was set to 3 for spi0 and to 1 > > for > > spi1 in arch/arm/mach-imx/mach-mx31lite.c. My understanding is that > > it > > would make sense to add this as num-cs to > > arch/arm/boot/dts/imx31-lite.dts. > > Or just pass cs-gpios instead? Using GPIOs for chipselect would require different pinmuxing. Also, why use GPIOs, when the SPI controller has this built in?
On Wed, Aug 26, 2020 at 10:13 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > Using GPIOs for chipselect would require different pinmuxing. Also, why > use GPIOs, when the SPI controller has this built in? In the initial chips with the ECSPI controller there was a bug with the native chipselect controller and we had to use GPIO.
On Wed, 2020-08-26 at 10:49 -0300, Fabio Estevam wrote: > On Wed, Aug 26, 2020 at 10:13 AM Matthias Schiffer > <matthias.schiffer@ew.tq-group.com> wrote: > > > Using GPIOs for chipselect would require different pinmuxing. Also, > > why > > use GPIOs, when the SPI controller has this built in? > > In the initial chips with the ECSPI controller there was a bug with > the native chipselect controller and we had to use GPIO. Ah, I see. Nevertheless, hardware that uses the native chipselects of newer chips exists (for example our TQ-Systems starterkit mainboards, the DTS of which we're currently preparing for mainline submission). Shouldn't num-cs be set for boards (or SoM DTSI) where not all CS pins of the SoC are usable? In any case, my original question was about the intended logic for num_chipselects for SPI drivers. My proposal was: - If num-cs is set, use that - If num-cs is unset, use the number of cs-gpios - If num-cs is unset and no cs-gpios are defined, use a driver-provided default How do other SPI controller drivers deal with this?
Hi Matthias, (Your mailer is messing the threading.) On Thu, Aug 27, 2020 at 4:31 AM Matthias Schiffer <matthias.schiffer@ew.tq-group.com> wrote: > Ah, I see. > > Nevertheless, hardware that uses the native chipselects of newer chips > exists (for example our TQ-Systems starterkit mainboards, the DTS of > which we're currently preparing for mainline submission). Shouldn't > num-cs be set for boards (or SoM DTSI) where not all CS pins of the SoC > are usable? > > In any case, my original question was about the intended logic for > num_chipselects for SPI drivers. My proposal was: > > - If num-cs is set, use that > - If num-cs is unset, use the number of cs-gpios > - If num-cs is unset and no cs-gpios are defined, use a driver-provided > default > > How do other SPI controller drivers deal with this? I think it would be better to discuss this in the spi mailing list with Mark Brown and the dt maintainer, Rob Herring.
diff --git a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi index 9513020ddd1a..7aaae83c1fae 100644 --- a/arch/arm/boot/dts/imx6qdl-tqma6.dtsi +++ b/arch/arm/boot/dts/imx6qdl-tqma6.dtsi @@ -20,7 +20,7 @@ &ecspi1 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_ecspi1>; - fsl,spi-num-chipselects = <1>; + num-cs = <1>; cs-gpios = <&gpio3 19 GPIO_ACTIVE_HIGH>; status = "okay"; diff --git a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi index c18a06cf7929..b679bec78e6c 100644 --- a/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi +++ b/arch/arm/boot/dts/imx6qdl-tqma6a.dtsi @@ -16,7 +16,7 @@ }; sensor@48 { - compatible = "lm75"; + compatible = "national,lm75"; reg = <0x48>; }; diff --git a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi index a7460075f517..49c472285c06 100644 --- a/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi +++ b/arch/arm/boot/dts/imx6qdl-tqma6b.dtsi @@ -16,7 +16,7 @@ }; sensor@48 { - compatible = "lm75"; + compatible = "national,lm75"; reg = <0x48>; };
- Fix national,lm75 compatible string - Replace obsolete fsl,spi-num-chipselects with num-cs Fixes: cac849e9bbc8 ("ARM: dts: imx6qdl: add TQMa6{S,Q,QP} SoM") Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com> --- arch/arm/boot/dts/imx6qdl-tqma6.dtsi | 2 +- arch/arm/boot/dts/imx6qdl-tqma6a.dtsi | 2 +- arch/arm/boot/dts/imx6qdl-tqma6b.dtsi | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)