| Message ID | 20221118164201.321147-1-krzysztof.kozlowski@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFT,v2,1/2] arm64: dts: qcom: sdm845-db845c: drop unneeded qup_spi0_default | expand |
Hi, On Fri, Nov 18, 2022 at 8:42 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > The qup_spi0_default pin override is exactly the same as one already in > sdm845.dtsi. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Cc: Doug Anderson <dianders@chromium.org> > > Changes since v1: > 1. New patch. > --- > arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts > index 02dcf75c0745..56a7afb697ed 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts > +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts > @@ -1274,11 +1274,3 @@ ov7251_ep: endpoint { > }; > }; > }; > - > -/* PINCTRL - additions to nodes defined in sdm845.dtsi */ > -&qup_spi0_default { > - config { > - drive-strength = <6>; > - bias-disable; > - }; > -}; I guess it's more of a question for what Bjorn thinks, but I view the fact that the drive-strength / bias are in the dtsi file to begin with as more as a bug in commit 8f6e20adaaf3 ("arm64: dts: qcom: sdm845: enable dma for spi"), which is where these properties were introduced to sdm845.dtsi. The historical guidance from Bjorn was that things like "drive-strength" and "bias" didn't belong in the SoC dtsi file. Later we came to an agreement that it could be OK to put drive-strength in the SoC dtsi file but that bias was still problematic because it meant ugly "/delete-property/" stuff in the board dtsi files [1]. [1] https://lore.kernel.org/r/YnSQvyAN3v69an8k@ripper
Hi, On Fri, Nov 18, 2022 at 8:42 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > DT schema expects TLMM pin configuration nodes to be named with > '-state' suffix and their optional children with '-pins' suffix. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Cc: Doug Anderson <dianders@chromium.org> > > Tested on Qualcomm RB3. Please kndly test a bit more on other devices. > This should not have an functional impact. Added Rob Clark and Fritz who are the only people I know that still do anything with cheza. > - wcd_intr_default: wcd_intr_default { > + wcd_intr_default: wcd-intr-default-state { > pins = <54>; Not new to your patch, but I'm surprised it truly works to use an integer for a pin? How does it know that 54 is an integer and not a string??? > &qup_uart3_default { > - pinmux { > - pins = "gpio41", "gpio42", "gpio43", "gpio44"; > + cts-rts-pins { > + pins = "gpio41", "gpio42"; > function = "qup3"; > }; > }; FWIW, I would have expected that the SoC dtsi file would get a "4-pin" definition (similar to what you did with qup_uart6_4pin) and then we'd use that here. > qup_uart6_4pin: qup-uart6-4pin-state { > - > - cts-pins { > + qup_uart6_4pin_cts: cts-pins { > pins = "gpio45"; > function = "qup6"; > - bias-pull-down; After your patch, where is the above bias set for cheza, db845c, oneplus, shift-axolotl, ...? > }; > > - rts-tx-pins { > + qup_uart6_4pin_rts_tx: rts-tx-pins { > pins = "gpio46", "gpio47"; > function = "qup6"; > - drive-strength = <2>; > - bias-disable; After your patch, where is the above bias / drive-strength set? > }; > > - rx-pins { > + qup_uart6_4pin_rx: rx-pins { > pins = "gpio48"; > function = "qup6"; > - bias-pull-up; After your patch, where is the above bias set?
On 02/12/2022 01:50, Doug Anderson wrote: > Hi, > > On Fri, Nov 18, 2022 at 8:42 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> DT schema expects TLMM pin configuration nodes to be named with >> '-state' suffix and their optional children with '-pins' suffix. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> Cc: Doug Anderson <dianders@chromium.org> >> >> Tested on Qualcomm RB3. Please kndly test a bit more on other devices. >> This should not have an functional impact. > > Added Rob Clark and Fritz who are the only people I know that still do > anything with cheza. > > >> - wcd_intr_default: wcd_intr_default { >> + wcd_intr_default: wcd-intr-default-state { >> pins = <54>; > > Not new to your patch, but I'm surprised it truly works to use an > integer for a pin? How does it know that 54 is an integer and not a > string??? Maybe it passes the checks as phandle... Anyway, I'll fix it in separate patch, assuming this is GPIO54. On schematics I see CODEC_INT1_N GPIO_54. > > >> &qup_uart3_default { >> - pinmux { >> - pins = "gpio41", "gpio42", "gpio43", "gpio44"; >> + cts-rts-pins { >> + pins = "gpio41", "gpio42"; >> function = "qup3"; >> }; >> }; > > FWIW, I would have expected that the SoC dtsi file would get a "4-pin" > definition (similar to what you did with qup_uart6_4pin) and then we'd > use that here. Sure. > > >> qup_uart6_4pin: qup-uart6-4pin-state { >> - >> - cts-pins { >> + qup_uart6_4pin_cts: cts-pins { >> pins = "gpio45"; >> function = "qup6"; >> - bias-pull-down; > > After your patch, where is the above bias set for cheza, db845c, > oneplus, shift-axolotl, ...? > > >> }; >> >> - rts-tx-pins { >> + qup_uart6_4pin_rts_tx: rts-tx-pins { >> pins = "gpio46", "gpio47"; >> function = "qup6"; >> - drive-strength = <2>; >> - bias-disable; > > After your patch, where is the above bias / drive-strength set? They don't use 4-pin setup. If they use, I would assume they will override the entries just like sdm850 boards (where I override it to set these). Alternatively I can keep it in DTSI, but it is not really property of the SoC. > > >> }; >> >> - rx-pins { >> + qup_uart6_4pin_rx: rx-pins { >> pins = "gpio48"; >> function = "qup6"; >> - bias-pull-up; > > After your patch, where is the above bias set? Best regards, Krzysztof
On 02/12/2022 01:49, Doug Anderson wrote: > Hi, > > On Fri, Nov 18, 2022 at 8:42 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> The qup_spi0_default pin override is exactly the same as one already in >> sdm845.dtsi. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> Cc: Doug Anderson <dianders@chromium.org> >> >> Changes since v1: >> 1. New patch. >> --- >> arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 8 -------- >> 1 file changed, 8 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts >> index 02dcf75c0745..56a7afb697ed 100644 >> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts >> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts >> @@ -1274,11 +1274,3 @@ ov7251_ep: endpoint { >> }; >> }; >> }; >> - >> -/* PINCTRL - additions to nodes defined in sdm845.dtsi */ >> -&qup_spi0_default { >> - config { >> - drive-strength = <6>; >> - bias-disable; >> - }; >> -}; > > I guess it's more of a question for what Bjorn thinks, but I view the > fact that the drive-strength / bias are in the dtsi file to begin with > as more as a bug in commit 8f6e20adaaf3 ("arm64: dts: qcom: sdm845: > enable dma for spi"), which is where these properties were introduced > to sdm845.dtsi. > > The historical guidance from Bjorn was that things like > "drive-strength" and "bias" didn't belong in the SoC dtsi file. Later > we came to an agreement that it could be OK to put drive-strength in > the SoC dtsi file but that bias was still problematic because it meant > ugly "/delete-property/" stuff in the board dtsi files [1]. So let's move it from DTSI to all boards? Although what if the board does not use SPI0? Best regards, Krzysztof
Hi, On Fri, Dec 2, 2022 at 12:15 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > >> qup_uart6_4pin: qup-uart6-4pin-state { > >> - > >> - cts-pins { > >> + qup_uart6_4pin_cts: cts-pins { > >> pins = "gpio45"; > >> function = "qup6"; > >> - bias-pull-down; > > > > After your patch, where is the above bias set for cheza, db845c, > > oneplus, shift-axolotl, ...? > > > > > >> }; > >> > >> - rts-tx-pins { > >> + qup_uart6_4pin_rts_tx: rts-tx-pins { > >> pins = "gpio46", "gpio47"; > >> function = "qup6"; > >> - drive-strength = <2>; > >> - bias-disable; > > > > After your patch, where is the above bias / drive-strength set? > > They don't use 4-pin setup. If they use, I would assume they will > override the entries just like sdm850 boards (where I override it to set > these). > > Alternatively I can keep it in DTSI, but it is not really property of > the SoC. I see things like: .../sdm845-cheza.dtsi: pinctrl-0 = <&qup_uart6_4pin>; ...before your patch that would get the bias/drive strength from the SoC dtsi, right? After your patch, you've removed it from the dtsi but not added it to the board. ...so I think it's a net change. Did I mess up / miss something?
Hi, On Fri, Dec 2, 2022 at 12:17 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 02/12/2022 01:49, Doug Anderson wrote: > > Hi, > > > > On Fri, Nov 18, 2022 at 8:42 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> The qup_spi0_default pin override is exactly the same as one already in > >> sdm845.dtsi. > >> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> > >> --- > >> > >> Cc: Doug Anderson <dianders@chromium.org> > >> > >> Changes since v1: > >> 1. New patch. > >> --- > >> arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 8 -------- > >> 1 file changed, 8 deletions(-) > >> > >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts > >> index 02dcf75c0745..56a7afb697ed 100644 > >> --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts > >> +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts > >> @@ -1274,11 +1274,3 @@ ov7251_ep: endpoint { > >> }; > >> }; > >> }; > >> - > >> -/* PINCTRL - additions to nodes defined in sdm845.dtsi */ > >> -&qup_spi0_default { > >> - config { > >> - drive-strength = <6>; > >> - bias-disable; > >> - }; > >> -}; > > > > I guess it's more of a question for what Bjorn thinks, but I view the > > fact that the drive-strength / bias are in the dtsi file to begin with > > as more as a bug in commit 8f6e20adaaf3 ("arm64: dts: qcom: sdm845: > > enable dma for spi"), which is where these properties were introduced > > to sdm845.dtsi. > > > > The historical guidance from Bjorn was that things like > > "drive-strength" and "bias" didn't belong in the SoC dtsi file. Later > > we came to an agreement that it could be OK to put drive-strength in > > the SoC dtsi file but that bias was still problematic because it meant > > ugly "/delete-property/" stuff in the board dtsi files [1]. > > So let's move it from DTSI to all boards? Although what if the board > does not use SPI0? You'd look for boards that set spi0's status to "okay" and those boards would be the ones to have it in their dtsi. -Doug
On 02/12/2022 15:36, Doug Anderson wrote: > Hi, > > On Fri, Dec 2, 2022 at 12:15 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >>>> qup_uart6_4pin: qup-uart6-4pin-state { >>>> - >>>> - cts-pins { >>>> + qup_uart6_4pin_cts: cts-pins { >>>> pins = "gpio45"; >>>> function = "qup6"; >>>> - bias-pull-down; >>> >>> After your patch, where is the above bias set for cheza, db845c, >>> oneplus, shift-axolotl, ...? >>> >>> >>>> }; >>>> >>>> - rts-tx-pins { >>>> + qup_uart6_4pin_rts_tx: rts-tx-pins { >>>> pins = "gpio46", "gpio47"; >>>> function = "qup6"; >>>> - drive-strength = <2>; >>>> - bias-disable; >>> >>> After your patch, where is the above bias / drive-strength set? >> >> They don't use 4-pin setup. If they use, I would assume they will >> override the entries just like sdm850 boards (where I override it to set >> these). >> >> Alternatively I can keep it in DTSI, but it is not really property of >> the SoC. > > I see things like: > > .../sdm845-cheza.dtsi: pinctrl-0 = <&qup_uart6_4pin>; > > ...before your patch that would get the bias/drive strength from the > SoC dtsi, right? After your patch, you've removed it from the dtsi but > not added it to the board. ...so I think it's a net change. Did I mess > up / miss something? I missed something or rather my git grep failed. I'll bring them back to DTSI. Best regards, Krzysztof
diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts index 02dcf75c0745..56a7afb697ed 100644 --- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts +++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts @@ -1274,11 +1274,3 @@ ov7251_ep: endpoint { }; }; }; - -/* PINCTRL - additions to nodes defined in sdm845.dtsi */ -&qup_spi0_default { - config { - drive-strength = <6>; - bias-disable; - }; -};
The qup_spi0_default pin override is exactly the same as one already in sdm845.dtsi. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Cc: Doug Anderson <dianders@chromium.org> Changes since v1: 1. New patch. --- arch/arm64/boot/dts/qcom/sdm845-db845c.dts | 8 -------- 1 file changed, 8 deletions(-)