diff mbox series

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

Message ID 20201202214935.1114381-1-swboyd@chromium.org
State Superseded
Headers show
Series spi: spi-geni-qcom: Use the new method of gpio CS control | expand

Commit Message

Stephen Boyd Dec. 2, 2020, 9:49 p.m. UTC
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(+)


base-commit: b65054597872ce3aefbc6a666385eabdf9e288da

Comments

Stephen Boyd Dec. 2, 2020, 11:28 p.m. UTC | #1
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.
Stephen Boyd Dec. 3, 2020, 12:47 a.m. UTC | #2
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.
Doug Anderson Dec. 3, 2020, 8:06 p.m. UTC | #3
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/
Stephen Boyd Dec. 4, 2020, 12:10 a.m. UTC | #4
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/
Enric Balletbo i Serra Dec. 4, 2020, 9:13 a.m. UTC | #5
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/
Mark Brown Dec. 4, 2020, 5 p.m. UTC | #6
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?
diff mbox series

Patch

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);