Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Wed, Aug 26, 2020 at 10:13 AM Matthias Schiffer 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.
Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Wed, 2020-08-26 at 10:01 -0300, Fabio Estevam wrote: > On Wed, Aug 26, 2020 at 8:54 AM Matthias Schiffer > 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?
Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Wed, Aug 26, 2020 at 8:54 AM Matthias Schiffer 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?
Re: (EXT) Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
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 > 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.
Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
Hi Matthias, On Wed, Aug 26, 2020 at 7:32 AM Matthias Schiffer 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?
Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
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 > > 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.
Re: (EXT) Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Tue, 2020-08-25 at 14:16 -0300, Fabio Estevam wrote: > On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer > 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.
Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
On Tue, Aug 25, 2020 at 11:40 AM Matthias Schiffer 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
Re: (EXT) Re: (EXT) Re: [PATCH 2/2] ARM: dts: imx6qdl: tqma6: minor fixes
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 > 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?