| Message ID | 20230323173019.3706069-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | Control Quad SPI pinctrl better on Qualcomm Chromebooks | expand |
On Thu, Mar 23, 2023 at 10:30:12AM -0700, Douglas Anderson wrote: > The Qualcomm pinctrl driver has been violating the documented meaning > of PIN_CONFIG_INPUT_ENABLE. That documentation says: > > Note that this does not affect the pin's ability to drive output. > > ...yet the Qualcomm driver's sole action when asked to "enable input" > on a pin is to disable its output. > Seemed like a good idea at the time... > The Qualcomm driver's implementation stems from the fact that > "output-disable" is a "new" property from 2017. It was introduced in > commit 425562429d4f ("pinctrl: generic: Add output-enable > property"). The "input-enable" handling in Qualcomm drivers is from > 2015 introduced in commit 407f5e392f9c ("pinctrl: qcom: handle > input-enable pinconf property"). > > Let's change the Qualcomm driver to move us in the right direction. As > part of this: > 1. We'll now support PIN_CONFIG_OUTPUT_ENABLE > 2. We'll still support using PIN_CONFIG_INPUT_ENABLE to disable a > pin's output (in violation of the docs) with a big comment in the > code. This is needed because old device trees have "input-enable" > in them and, in some cases, people might need the old > behavior. While we could programmatically change all old device > trees, it doesn't really hurt to keep supporting the old behavior > and we're _supposed_ to try to be compatible with old device trees > anyway. > > It can also be noted that the PIN_CONFIG_INPUT_ENABLE handling code > seems to have purposefully ignored its argument. That means that old > boards that had _either_ "input-disable" or "input-enable" in them > would have had the effect of disabling a pin's output. While we could > change this behavior, since we're only leaving the > PIN_CONFIG_INPUT_ENABLE there for backward compatibility we might as > well be fully backward compatible. > It made total sense to to spell input-disable as output-{high,low} back then, but we're wiser now. Thanks for fixing it. > NOTE: despite the fact that we'll still support > PIN_CONFIG_INPUT_ENABLE for _setting_ config, we take it away from > msm_config_group_get(). This appears to be only used for populating > debugfs and fixing debugfs to "output enabled" where relevant instead > of "input enabled" makes more sense and has more truthiness. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > --- > > drivers/pinctrl/qcom/pinctrl-msm.c | 36 +++++++++++++++++++++++++----- > 1 file changed, 31 insertions(+), 5 deletions(-) > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > index daeb79a9a602..4515f375c5e8 100644 > --- a/drivers/pinctrl/qcom/pinctrl-msm.c > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -323,6 +323,7 @@ static int msm_config_reg(struct msm_pinctrl *pctrl, > break; > case PIN_CONFIG_OUTPUT: > case PIN_CONFIG_INPUT_ENABLE: > + case PIN_CONFIG_OUTPUT_ENABLE: > *bit = g->oe_bit; > *mask = 1; > break; > @@ -414,11 +415,9 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev, > val = msm_readl_io(pctrl, g); > arg = !!(val & BIT(g->in_bit)); > break; > - case PIN_CONFIG_INPUT_ENABLE: > - /* Pin is output */ > - if (arg) > + case PIN_CONFIG_OUTPUT_ENABLE: > + if (!arg) > return -EINVAL; > - arg = 1; > break; > default: > return -ENOTSUPP; > @@ -502,9 +501,36 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev, > arg = 1; > break; > case PIN_CONFIG_INPUT_ENABLE: > - /* disable output */ > + /* > + * According to pinctrl documentation this should > + * actually be a no-op. > + * > + * The docs are explicit that "this does not affect > + * the pin's ability to drive output" but what we do > + * here is to modify the output enable bit. Thus, to > + * follow the docs we should remove that. > + * > + * The docs say that we should enable any relevant > + * input buffer, but TLMM there is no input buffer that > + * can be enabled/disabled. It's always on. > + * > + * The points above, explain why this _should_ be a > + * no-op. However, for historical reasons and to > + * support old device trees, we'll violate the docs > + * still affect the output. > + * > + * It should further be noted that this old historical > + * behavior actually overrides arg to 0. That means > + * that "input-enable" and "input-disable" in a device > + * tree would _both_ disable the output. We'll > + * continue to preserve this behavior as well since > + * we have no other use for this attribute. > + */ > arg = 0; > break; > + case PIN_CONFIG_OUTPUT_ENABLE: > + arg = !!arg; > + break; > default: > dev_err(pctrl->dev, "Unsupported config parameter: %x\n", > param); > -- > 2.40.0.348.gf938b09366-goog >
On Thu, Mar 23, 2023 at 6:31 PM Douglas Anderson <dianders@chromium.org> wrote: > The main goal of this series is to do a better job of cI can apply ontroling the > pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This > is essentially 'v2' of my previous attempt in the patch ("arm64: dts: > qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's > spiraled out a bit and there are no patches that are exactly the same > I've reset to v1. > > The early patches in this series are just no-op cleanup patches that > can be applied. They're not terribly critical but since they are > "Fixes" I've listed them first. > > The next patch in the series is a very simple and (hopefully) > non-controversial SPI patch. It can be applied independently if > anything else. > > Next, we have a bunch of pinctrl patches (including the device tree > bindings related to them). I dunno what folks are going to think about > these. If everyone hates them, we can drop them and just change the > later patches in the series to use "input-enable" instead of > "output-disable". It feels ugly to me, but it maybe less upheval. > > Next I removed the now-deprecated "input-enable" property from all > Chromebooks. None of them were necessary. > > Finally, I did what I really wanted to do in the first place: attempt > to cleanup the pinctrl states of the Quad SPI. These patches have a > hard requirement on the pinctrl change. This looks good to me (TM) Do you have a merge plan? I can queue the pinctrl patch into the pinctrl tree, and the pinctrl binding patches. Will you take the rest to the SPI and SoC tree? Acked-by: Linus Walleij <linus.walleij@linaro.org> FWIW Yours, Linus Walleij
Hi, On Mon, Mar 27, 2023 at 2:45 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Mar 23, 2023 at 6:31 PM Douglas Anderson <dianders@chromium.org> wrote: > > > The main goal of this series is to do a better job of cI can apply ontroling the > > pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This > > is essentially 'v2' of my previous attempt in the patch ("arm64: dts: > > qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's > > spiraled out a bit and there are no patches that are exactly the same > > I've reset to v1. > > > > The early patches in this series are just no-op cleanup patches that > > can be applied. They're not terribly critical but since they are > > "Fixes" I've listed them first. > > > > The next patch in the series is a very simple and (hopefully) > > non-controversial SPI patch. It can be applied independently if > > anything else. > > > > Next, we have a bunch of pinctrl patches (including the device tree > > bindings related to them). I dunno what folks are going to think about > > these. If everyone hates them, we can drop them and just change the > > later patches in the series to use "input-enable" instead of > > "output-disable". It feels ugly to me, but it maybe less upheval. > > > > Next I removed the now-deprecated "input-enable" property from all > > Chromebooks. None of them were necessary. > > > > Finally, I did what I really wanted to do in the first place: attempt > > to cleanup the pinctrl states of the Quad SPI. These patches have a > > hard requirement on the pinctrl change. > > This looks good to me (TM) > > Do you have a merge plan? > I can queue the pinctrl patch into the pinctrl tree, and > the pinctrl binding patches. > > Will you take the rest to the SPI and SoC tree? > > Acked-by: Linus Walleij <linus.walleij@linaro.org> My thoughts: 1. Mark could land the SPI patch at any time, assuming he's OK with it. It can land totally independently. 2. First 7 dts patches could land in the Qualcomm tree. There are no dependencies on these ones other than the commit message of some of the later dts patches talking about the pinctrl patches. ...then... Option A: 3. You land the pinctrl and binding patches in an immutable branch and merge into pinctrl. 4. Bjorn merges the immutable branch into the Qulacomm tree and places the last 3 dts patches atop. Option B: 3. You Ack the pinctrl patches and Bjorn lands them all, plus the last 3 dts patches. Option C: 3. You land the pinctrl patches, then we want a few months and land the last 3 dts patches.
On Thu, 23 Mar 2023 10:30:04 -0700, Douglas Anderson wrote: > The main goal of this series is to do a better job of controling the > pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This > is essentially 'v2' of my previous attempt in the patch ("arm64: dts: > qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's > spiraled out a bit and there are no patches that are exactly the same > I've reset to v1. > > [...] Applied to broonie/spi.git for-next Thanks! [05/14] spi: spi-qcom-qspi: Support pinctrl sleep states commit: 0098c52745112c4387942a37559ababeaf072f0c All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Mon, Mar 27, 2023 at 11:51 PM Doug Anderson <dianders@chromium.org> wrote: > 1. Mark could land the SPI patch at any time, assuming he's OK with > it. It can land totally independently. OK this happened. > Option A: > > 3. You land the pinctrl and binding patches in an immutable branch and > merge into pinctrl. > > 4. Bjorn merges the immutable branch into the Qulacomm tree and places > the last 3 dts patches atop. Looks most appetizing. I have applied patches 6,7,8 to this immutable branch: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-qcom-quad-spi and I merged that into my "devel" branch for v6.4. Bjorn can grab the branch if he wants it. Yours, Linus Walleij
On Wed, Mar 29, 2023 at 10:50:34AM +0200, Linus Walleij wrote: > On Mon, Mar 27, 2023 at 11:51 PM Doug Anderson <dianders@chromium.org> wrote: > > > 1. Mark could land the SPI patch at any time, assuming he's OK with > > it. It can land totally independently. > > OK this happened. > > > Option A: > > > > 3. You land the pinctrl and binding patches in an immutable branch and > > merge into pinctrl. > > > > 4. Bjorn merges the immutable branch into the Qulacomm tree and places > > the last 3 dts patches atop. > > Looks most appetizing. > > I have applied patches 6,7,8 to this immutable branch: > https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/log/?h=ib-qcom-quad-spi > > and I merged that into my "devel" branch for v6.4. > > Bjorn can grab the branch if he wants it. > Thank you, Bjorn > Yours, > Linus Walleij
On Thu, 23 Mar 2023 10:30:04 -0700, Douglas Anderson wrote: > The main goal of this series is to do a better job of controling the > pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This > is essentially 'v2' of my previous attempt in the patch ("arm64: dts: > qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's > spiraled out a bit and there are no patches that are exactly the same > I've reset to v1. > > [...] Applied, thanks! [01/14] arm64: dts: sc7180: Rename qspi data12 as data23 commit: d84f8f2687bdc67f20262e822b206419bcfd0038 [02/14] arm64: dts: sc7280: Rename qspi data12 as data23 commit: 14acf21c0d3f7b7298ffcd2e5b5db4a476ec6202 [03/14] arm64: dts: sdm845: Rename qspi data12 as data23 commit: 37f7349b56decc91c66f8039712e63740b1f25f9 [04/14] arm64: dts: qcom: sc7180: Annotate l13a on trogdor to always-on commit: ced32c299e5d6c447ad0b80d7a16b44e0e72e8e0 [09/14] arm64: dts: qcom: sc7180: Remove superfluous "input-enable"s from trogdor commit: e8df226339fa032c49f8db4281903930d018a22c [10/14] arm64: dts: qcom: sc7280: Remove superfluous "input-enable"s from idp-ec-h1 commit: 6d4794d658a0967a7f257f16d6a7a48afb8c8e05 [11/14] arm64: dts: qcom: sdm845: Remove superfluous "input-enable"s from cheza commit: 406fed87083578d07c7cea9483b85b51469594e0 [12/14] arm64: dts: qcom: sc7180: Fix trogdor qspi pin config commit: ab752f03e2feb3323dfd9c1ce161ac759ce09634 [13/14] arm64: dts: qcom: sc7280: Fix qspi pin config commit: 5f89df31096d67c244d8f36502f651ce701ddcde [14/14] arm64: dts: qcom: sdm845: Fix cheza qspi pin config commit: 9f5cdeb7031062a36e135ebb88bd99c03f32e5ee Best regards,
On 23/03/2023 18:30, Douglas Anderson wrote: > In commit 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial > trogdor and lazor dt") we specified the pull settings on the boot SPI > (the qspi) data lines as pullups to "park" the lines. This seemed like > the right thing to do, but I never really probed the lines to confirm. > > &qup_i2c2_default { > @@ -1336,6 +1340,22 @@ p_sensor_int_l: p-sensor-int-l-state { > bias-disable; > }; > > + qspi_sleep: qspi-sleep-state { > + pins = "gpio63", "gpio64", "gpio65", "gpio68"; > + > + /* > + * When we're not actively transferring we want pins as GPIOs > + * with output disabled so that the quad SPI IP block stops > + * driving them. We rely on the normal pulls configured in > + * the active state and don't redefine them here. Also note > + * that we don't need the reverse (output-enable) in the > + * normal mode since the "output-enable" only matters for > + * GPIO function. > + */ > + function = "gpio"; > + output-disable; Doug, I acked some of your patches, but I assumed you tested all this. It turns out you never run dtbs_check on the patches you sent. Best regards, Krzysztof
Hi, On Fri, Apr 7, 2023 at 11:11 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 23/03/2023 18:30, Douglas Anderson wrote: > > In commit 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial > > trogdor and lazor dt") we specified the pull settings on the boot SPI > > (the qspi) data lines as pullups to "park" the lines. This seemed like > > the right thing to do, but I never really probed the lines to confirm. > > > > > > &qup_i2c2_default { > > @@ -1336,6 +1340,22 @@ p_sensor_int_l: p-sensor-int-l-state { > > bias-disable; > > }; > > > > + qspi_sleep: qspi-sleep-state { > > + pins = "gpio63", "gpio64", "gpio65", "gpio68"; > > + > > + /* > > + * When we're not actively transferring we want pins as GPIOs > > + * with output disabled so that the quad SPI IP block stops > > + * driving them. We rely on the normal pulls configured in > > + * the active state and don't redefine them here. Also note > > + * that we don't need the reverse (output-enable) in the > > + * normal mode since the "output-enable" only matters for > > + * GPIO function. > > + */ > > + function = "gpio"; > > + output-disable; > > Doug, > > I acked some of your patches, but I assumed you tested all this. It > turns out you never run dtbs_check on the patches you sent. I'm fairly certain that I ran dtbs_check and confirmed that no new errors were introduced on the device tree files that this patch series cleaned up. Did I miss one? I did not try to go through and fix all examples of people using "input-enable" across all Qualcomm device trees, though. Those old device trees still work even if they're using the now-deprecated bindings. When deprecating something my understanding is that it's not required to go back and immediately transition all old device tree files. If having the "input-enable: false" in the bindings is causing huge problems we could do a blank search-and-replace to rename it to "output-disable", at least for places under "tlmm". Even if there are cases where it's superfluous it would at least make the bindings validate. -Doug
On 07/04/2023 21:53, Doug Anderson wrote: > Hi, > > On Fri, Apr 7, 2023 at 11:11 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 23/03/2023 18:30, Douglas Anderson wrote: >>> In commit 7ec3e67307f8 ("arm64: dts: qcom: sc7180-trogdor: add initial >>> trogdor and lazor dt") we specified the pull settings on the boot SPI >>> (the qspi) data lines as pullups to "park" the lines. This seemed like >>> the right thing to do, but I never really probed the lines to confirm. >>> >> >> >>> &qup_i2c2_default { >>> @@ -1336,6 +1340,22 @@ p_sensor_int_l: p-sensor-int-l-state { >>> bias-disable; >>> }; >>> >>> + qspi_sleep: qspi-sleep-state { >>> + pins = "gpio63", "gpio64", "gpio65", "gpio68"; >>> + >>> + /* >>> + * When we're not actively transferring we want pins as GPIOs >>> + * with output disabled so that the quad SPI IP block stops >>> + * driving them. We rely on the normal pulls configured in >>> + * the active state and don't redefine them here. Also note >>> + * that we don't need the reverse (output-enable) in the >>> + * normal mode since the "output-enable" only matters for >>> + * GPIO function. >>> + */ >>> + function = "gpio"; >>> + output-disable; >> >> Doug, >> >> I acked some of your patches, but I assumed you tested all this. It >> turns out you never run dtbs_check on the patches you sent. > > I'm fairly certain that I ran dtbs_check and confirmed that no new > errors were introduced on the device tree files that this patch series > cleaned up. Did I miss one? You missed everything. Before the patchset almost all pinctrl bindings were passing on arm64 DTS. Just one or two things to fix. After the patchset: many new warnings. > I did not try to go through and fix all > examples of people using "input-enable" across all Qualcomm device > trees, though. You introduced new warnings, so it is expected to do. > Those old device trees still work even if they're using > the now-deprecated bindings. When deprecating something my > understanding is that it's not required to go back and immediately > transition all old device tree files. You did not deprecate anything. You disallowed property causing many new warnings to pop up. > > If having the "input-enable: false" in the bindings is causing huge > problems we could do a blank search-and-replace to rename it to > "output-disable", at least for places under "tlmm". Even if there are > cases where it's superfluous it would at least make the bindings > validate. There are different ways to fix it, the point is that none of the ways were used. I fixed it up: https://lore.kernel.org/linux-arm-msm/574d3aa5-21f4-014a-8cc7-7549df59ff3c@linaro.org/ https://lore.kernel.org/linux-arm-msm/20230407180655.128771-1-krzysztof.kozlowski@linaro.org/ https://lore.kernel.org/linux-arm-msm/20230407180045.126952-1-krzysztof.kozlowski@linaro.org/ https://lore.kernel.org/linux-arm-msm/20230407175807.124394-1-krzysztof.kozlowski@linaro.org/ Best regards, Krzysztof
The main goal of this series is to do a better job of controling the pins related to the "Quad SPI" IP block on Qualcomm Chromebooks. This is essentially 'v2' of my previous attempt in the patch ("arm64: dts: qcom: sc7180: Fix trogdor qspi pull direction") [1] but since it's spiraled out a bit and there are no patches that are exactly the same I've reset to v1. The early patches in this series are just no-op cleanup patches that can be applied. They're not terribly critical but since they are "Fixes" I've listed them first. The next patch in the series is a very simple and (hopefully) non-controversial SPI patch. It can be applied independently if anything else. Next, we have a bunch of pinctrl patches (including the device tree bindings related to them). I dunno what folks are going to think about these. If everyone hates them, we can drop them and just change the later patches in the series to use "input-enable" instead of "output-disable". It feels ugly to me, but it maybe less upheval. Next I removed the now-deprecated "input-enable" property from all Chromebooks. None of them were necessary. Finally, I did what I really wanted to do in the first place: attempt to cleanup the pinctrl states of the Quad SPI. These patches have a hard requirement on the pinctrl change. [1] https://lore.kernel.org/r/20230213165743.1.I6f03f86546e6ce9abb1d24fd9ece663c3a5b950c@changeid Douglas Anderson (14): arm64: dts: sc7180: Rename qspi data12 as data23 arm64: dts: sc7280: Rename qspi data12 as data23 arm64: dts: sdm845: Rename qspi data12 as data23 arm64: dts: qcom: sc7180: Annotate l13a on trogdor to always-on spi: spi-qcom-qspi: Support pinctrl sleep states dt-bindings: pinctrl: qcom: tlmm should use output-disable, not input-enable dt-bindings: pinctrl: qcom: Add output-enable pinctrl: qcom: Support OUTPUT_ENABLE; deprecate INPUT_ENABLE arm64: dts: qcom: sc7180: Remove superfluous "input-enable"s from trogdor arm64: dts: qcom: sc7280: Remove superfluous "input-enable"s from idp-ec-h1 arm64: dts: qcom: sdm845: Remove superfluous "input-enable"s from cheza arm64: dts: qcom: sc7180: Fix trogdor qspi pin config arm64: dts: qcom: sc7280: Fix qspi pin config arm64: dts: qcom: sdm845: Fix cheza qspi pin config .../bindings/pinctrl/qcom,tlmm-common.yaml | 4 +- arch/arm64/boot/dts/qcom/sc7180-idp.dts | 9 ++-- arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 41 +++++++++++++------ arch/arm64/boot/dts/qcom/sc7180.dtsi | 11 +++-- .../boot/dts/qcom/sc7280-chrome-common.dtsi | 25 ++++++++++- .../arm64/boot/dts/qcom/sc7280-herobrine.dtsi | 14 ++++--- .../arm64/boot/dts/qcom/sc7280-idp-ec-h1.dtsi | 2 - arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 13 +++--- arch/arm64/boot/dts/qcom/sc7280.dtsi | 11 +++-- arch/arm64/boot/dts/qcom/sdm845-cheza.dtsi | 37 ++++++++++++----- arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 +++-- drivers/pinctrl/qcom/pinctrl-msm.c | 36 +++++++++++++--- drivers/spi/spi-qcom-qspi.c | 5 +++ 13 files changed, 165 insertions(+), 54 deletions(-)