| Message ID | 20201202214935.1114381-1-swboyd@chromium.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3b25f337929e73232f0aa990cd68a129f53652e2 |
| Headers | show |
| Series | spi: spi-geni-qcom: Use the new method of gpio CS control | expand |
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)
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.
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.
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/
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/
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/
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 --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);