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", _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(>dev, "num-cs", ))
> + 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?
