[PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Stephen Boyd]

Let's set the 'use_gpio_descriptors' field so that we use the new way of
requesting the CS GPIOs in the core. This allows us to avoid having to
configure the CS pins in "output" mode with an 'output-enable' pinctrl
setting.

Cc: Akash Asthana <akashast@codeaurora.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Alexandru M Stan <amstan@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/spi/spi-geni-qcom.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 25810a7eef10..c4c88984abc9 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -636,6 +636,7 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spi->auto_runtime_pm = true;
 	spi->handle_err = handle_fifo_timeout;
 	spi->set_cs = spi_geni_set_cs;
+	spi->use_gpio_descriptors = true;
 
 	init_completion(&mas->cs_done);
 	init_completion(&mas->cancel_done);

base-commit: b65054597872ce3aefbc6a666385eabdf9e288da
-- 
https://chromeos.dev

Re: [PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Alexandru M Stan]

On Wed, Dec 2, 2020 at 1:49 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Let's set the 'use_gpio_descriptors' field so that we use the new way of
> requesting the CS GPIOs in the core. This allows us to avoid having to
> configure the CS pins in "output" mode with an 'output-enable' pinctrl
> setting.
>
> Cc: Akash Asthana <akashast@codeaurora.org>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Acked-by: Alexandru M Stan <amstan@chromium.org>
I meant this as a joke in chat. It doesn't really mean anything in any capacity.

> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/spi/spi-geni-qcom.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 25810a7eef10..c4c88984abc9 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -636,6 +636,7 @@ static int spi_geni_probe(struct platform_device *pdev)
>         spi->auto_runtime_pm = true;
>         spi->handle_err = handle_fifo_timeout;
>         spi->set_cs = spi_geni_set_cs;
> +       spi->use_gpio_descriptors = true;
>
>         init_completion(&mas->cs_done);
>         init_completion(&mas->cancel_done);
>
> base-commit: b65054597872ce3aefbc6a666385eabdf9e288da
> --
> https://chromeos.dev
>

Unfortunately this patch makes my cros-ec (the main EC that used to
work even before my debugging) also fail to probe:
[    0.839533] cros-ec-spi spi6.0: EC failed to respond in time
[    1.040453] cros-ec-spi spi6.0: EC failed to respond in time
[    1.040852] cros-ec-spi spi6.0: Cannot identify the EC: error -110
[    1.040855] cros-ec-spi spi6.0: cannot register EC, fallback to spidev
[    1.040942] cros-ec-spi: probe of spi6.0 failed with error -110

I wasn't closely looking at this part closely when I was using my
other spi port with spidev, so this is why I haven't noticed it
before.
Doug suggests this might be a polarity issue. More scoping to be had.

On the other hand my gpioinfo output is better with this patch:
       line  86: "AP_SPI_FP_MISO" unused input active-high
       line  87: "AP_SPI_FP_MOSI" unused input active-high
       line  88: "AP_SPI_FP_CLK" unused input active-high
       line  89: "AP_SPI_FP_CS_L" "spi10 CS0" output active-low [used]

Previously they were:
       line  86: "AP_SPI_FP_MISO" unused input active-high
       line  87: "AP_SPI_FP_MOSI" unused input active-high
       line  88: "AP_SPI_FP_CLK" unused input active-high
       line  89: "AP_SPI_FP_CS_L" unused output active-high

But I'm still disappointed the rest of the pins (CLK MISO MOSI) are
still "unused", but I was told that's just an artifact of those pins
not being gpios (but remuxed to spi).

Alexandru Stan (amstan)

Re: [PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Stephen Boyd]

Quoting Alexandru M Stan (2020-12-02 14:18:20)
> On Wed, Dec 2, 2020 at 1:49 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Let's set the 'use_gpio_descriptors' field so that we use the new way of
> > requesting the CS GPIOs in the core. This allows us to avoid having to
> > configure the CS pins in "output" mode with an 'output-enable' pinctrl
> > setting.
> >
> > Cc: Akash Asthana <akashast@codeaurora.org>
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Acked-by: Alexandru M Stan <amstan@chromium.org>
> I meant this as a joke in chat. It doesn't really mean anything in any capacity.

Sorry! It can be removed when applying.

> 
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/spi/spi-geni-qcom.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > index 25810a7eef10..c4c88984abc9 100644
> > --- a/drivers/spi/spi-geni-qcom.c
> > +++ b/drivers/spi/spi-geni-qcom.c
> > @@ -636,6 +636,7 @@ static int spi_geni_probe(struct platform_device *pdev)
> >         spi->auto_runtime_pm = true;
> >         spi->handle_err = handle_fifo_timeout;
> >         spi->set_cs = spi_geni_set_cs;
> > +       spi->use_gpio_descriptors = true;
> >
> >         init_completion(&mas->cs_done);
> >         init_completion(&mas->cancel_done);
> >
> > base-commit: b65054597872ce3aefbc6a666385eabdf9e288da
> > --
> > https://chromeos.dev
> >
> 
> Unfortunately this patch makes my cros-ec (the main EC that used to
> work even before my debugging) also fail to probe:
> [    0.839533] cros-ec-spi spi6.0: EC failed to respond in time
> [    1.040453] cros-ec-spi spi6.0: EC failed to respond in time
> [    1.040852] cros-ec-spi spi6.0: Cannot identify the EC: error -110
> [    1.040855] cros-ec-spi spi6.0: cannot register EC, fallback to spidev
> [    1.040942] cros-ec-spi: probe of spi6.0 failed with error -110
> 
> I wasn't closely looking at this part closely when I was using my
> other spi port with spidev, so this is why I haven't noticed it
> before.
> Doug suggests this might be a polarity issue. More scoping to be had.
> 

Ah I see. It looks like the cs-gpios polarity is wrong for the DTS on
sc7180. That's a patch that Doug has sent in for the qcom tree, commit
37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to use
GPIO for CS") and it is pending for the next release (v5.11). Doug says
he will send in a fix for the DTS side, but this patch is still "good"
as far as I can tell. It moves us to use gpio descriptors and also finds
bugs like this in the DTS file that we would have missed otherwise
because the legacy mode doesn't look at the polarity flags in DT.

Re: [PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Stephen Boyd]

Quoting Stephen Boyd (2020-12-02 15:28:45)
> Quoting Alexandru M Stan (2020-12-02 14:18:20)
> > Unfortunately this patch makes my cros-ec (the main EC that used to
> > work even before my debugging) also fail to probe:
> > [    0.839533] cros-ec-spi spi6.0: EC failed to respond in time
> > [    1.040453] cros-ec-spi spi6.0: EC failed to respond in time
> > [    1.040852] cros-ec-spi spi6.0: Cannot identify the EC: error -110
> > [    1.040855] cros-ec-spi spi6.0: cannot register EC, fallback to spidev
> > [    1.040942] cros-ec-spi: probe of spi6.0 failed with error -110
> > 
> > I wasn't closely looking at this part closely when I was using my
> > other spi port with spidev, so this is why I haven't noticed it
> > before.
> > Doug suggests this might be a polarity issue. More scoping to be had.
> > 
> 
> Ah I see. It looks like the cs-gpios polarity is wrong for the DTS on
> sc7180. That's a patch that Doug has sent in for the qcom tree, commit
> 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to use
> GPIO for CS") and it is pending for the next release (v5.11). Doug says
> he will send in a fix for the DTS side, but this patch is still "good"
> as far as I can tell. It moves us to use gpio descriptors and also finds
> bugs like this in the DTS file that we would have missed otherwise
> because the legacy mode doesn't look at the polarity flags in DT.

And that is wrong. With even more investigation and Doug's eagle eyes it
seems that the cros-ec driver is overriding the spi::mode to clear out
the SPI_CS_HIGH bit that the spi core sets in there when using the gpio
descriptors. I'll send a patch for cros-ec-spi shortly.

Re: [PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Doug Anderson]

Hi,

On Wed, Dec 2, 2020 at 4:47 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Stephen Boyd (2020-12-02 15:28:45)
> > Quoting Alexandru M Stan (2020-12-02 14:18:20)
> > > Unfortunately this patch makes my cros-ec (the main EC that used to
> > > work even before my debugging) also fail to probe:
> > > [    0.839533] cros-ec-spi spi6.0: EC failed to respond in time
> > > [    1.040453] cros-ec-spi spi6.0: EC failed to respond in time
> > > [    1.040852] cros-ec-spi spi6.0: Cannot identify the EC: error -110
> > > [    1.040855] cros-ec-spi spi6.0: cannot register EC, fallback to spidev
> > > [    1.040942] cros-ec-spi: probe of spi6.0 failed with error -110
> > >
> > > I wasn't closely looking at this part closely when I was using my
> > > other spi port with spidev, so this is why I haven't noticed it
> > > before.
> > > Doug suggests this might be a polarity issue. More scoping to be had.
> > >
> >
> > Ah I see. It looks like the cs-gpios polarity is wrong for the DTS on
> > sc7180. That's a patch that Doug has sent in for the qcom tree, commit
> > 37dd4b777942 ("arm64: dts: qcom: sc7180: Provide pinconf for SPI to use
> > GPIO for CS") and it is pending for the next release (v5.11). Doug says
> > he will send in a fix for the DTS side, but this patch is still "good"
> > as far as I can tell. It moves us to use gpio descriptors and also finds
> > bugs like this in the DTS file that we would have missed otherwise
> > because the legacy mode doesn't look at the polarity flags in DT.
>
> And that is wrong. With even more investigation and Doug's eagle eyes it
> seems that the cros-ec driver is overriding the spi::mode to clear out
> the SPI_CS_HIGH bit that the spi core sets in there when using the gpio
> descriptors. I'll send a patch for cros-ec-spi shortly.

So do we need any coordinating here, are we OK w/ trogdor devices
being broken for a short period of time?

I think the device tree changes switching to use GPIO for chip select
is already queued in linux-next.  That means if we land this patch
before the fix to cros_ec [1] then we'll end up in a broken state.
Would we be able to do some quick landing to get the cros-ec fix into
v5.10 and then target the SPI patch for 5.11?

-Doug

[1] https://lore.kernel.org/r/20201203011649.1405292-2-swboyd@chromium.org/

Re: [PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Stephen Boyd]

Quoting Doug Anderson (2020-12-03 12:06:10)
> On Wed, Dec 2, 2020 at 4:47 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > And that is wrong. With even more investigation and Doug's eagle eyes it
> > seems that the cros-ec driver is overriding the spi::mode to clear out
> > the SPI_CS_HIGH bit that the spi core sets in there when using the gpio
> > descriptors. I'll send a patch for cros-ec-spi shortly.
> 
> So do we need any coordinating here, are we OK w/ trogdor devices
> being broken for a short period of time?
> 
> I think the device tree changes switching to use GPIO for chip select
> is already queued in linux-next.  That means if we land this patch
> before the fix to cros_ec [1] then we'll end up in a broken state.
> Would we be able to do some quick landing to get the cros-ec fix into
> v5.10 and then target the SPI patch for 5.11?

I don't think it really matters if the two patches meet up in linux-next
or cros-ec is fast tracked, but it would be bad if this patch was merged
without the cros-ec one. One option would be to apply the cros-ec fix to
the spi tree along with this patch (or vice versa) so that a bisection
hole isn't created. Or this patch can wait for a while until cros-ec is
fixed. I'm not the maintainer here so it's really up to Mark and
Enric/Benson.

> 
> [1] https://lore.kernel.org/r/20201203011649.1405292-2-swboyd@chromium.org/

Re: [PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Enric Balletbo i Serra]

Hi,

On 4/12/20 1:10, Stephen Boyd wrote:
> Quoting Doug Anderson (2020-12-03 12:06:10)
>> On Wed, Dec 2, 2020 at 4:47 PM Stephen Boyd <swboyd@chromium.org> wrote:
>>>
>>> And that is wrong. With even more investigation and Doug's eagle eyes it
>>> seems that the cros-ec driver is overriding the spi::mode to clear out
>>> the SPI_CS_HIGH bit that the spi core sets in there when using the gpio
>>> descriptors. I'll send a patch for cros-ec-spi shortly.
>>
>> So do we need any coordinating here, are we OK w/ trogdor devices
>> being broken for a short period of time?
>>
>> I think the device tree changes switching to use GPIO for chip select
>> is already queued in linux-next.  That means if we land this patch
>> before the fix to cros_ec [1] then we'll end up in a broken state.
>> Would we be able to do some quick landing to get the cros-ec fix into
>> v5.10 and then target the SPI patch for 5.11?
> 
> I don't think it really matters if the two patches meet up in linux-next
> or cros-ec is fast tracked, but it would be bad if this patch was merged
> without the cros-ec one. One option would be to apply the cros-ec fix to
> the spi tree along with this patch (or vice versa) so that a bisection
> hole isn't created. Or this patch can wait for a while until cros-ec is
> fixed. I'm not the maintainer here so it's really up to Mark and
> Enric/Benson.
> 

I am fine either way. I'm fine with pick all the patches and go through the
chrome/platform tree if Mark is agree (I think this patch has no other
dependencies and the patch applies cleanly to my tree) or all can go through the
Mark's tree. If I need to an IB I can also do it without problems.

I'll leave Mark to decide who has much experience solving this kind of problems.

Thanks,
 Enric


>>
>> [1] https://lore.kernel.org/r/20201203011649.1405292-2-swboyd@chromium.org/

Re: [PATCH] spi: spi-geni-qcom: Use the new method of gpio CS control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

On Fri, Dec 04, 2020 at 10:13:52AM +0100, Enric Balletbo i Serra wrote:

> I am fine either way. I'm fine with pick all the patches and go through the
> chrome/platform tree if Mark is agree (I think this patch has no other
> dependencies and the patch applies cleanly to my tree) or all can go through the
> Mark's tree. If I need to an IB I can also do it without problems.
> 
> I'll leave Mark to decide who has much experience solving this kind of problems.

I'm happy to take both patches, there are some others in this area in
the SPI tree so it might minimise bisection problems if I take them but
not sure if there's any actual overlap there.  If someone could resend
them as a series?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]