| Message ID | 20230314074001.1873781-1-bhupesh.sharma@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 27ad7815cb70cbfdcbee4368f41033e83731479f |
| Headers | show |
| Series | [v2] arm64: dts: qcom: sm6115: Move SDHC node(s)'s 'pinctrl' properties to dts | expand |
On 14.03.2023 08:40, Bhupesh Sharma wrote: > Normally the 'pinctrl' properties of a SDHC controller and the > chip detect pin settings are dependent on the type of the slots > (for e.g uSD card slot), regulators and GPIO(s) available on the > board(s). > > So, move the same from the sm6115 dtsi file to the respective > board file(s). So, file or files? :D > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > Changes since v1: > - v1 can be seen here: https://lore.kernel.org/linux-arm-msm/20221220113616.1556097-1-bhupesh.sharma@linaro.org/ > - Colleted the R-B from Marijn. > - Rebased on linux-next/master > > .../boot/dts/qcom/sm4250-oneplus-billie2.dts | 10 +++++++++ > arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ------------------- > 2 files changed, 10 insertions(+), 22 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts > index a3f1c7c41fd73..329eb496bbc5f 100644 > --- a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts > +++ b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts > @@ -202,12 +202,22 @@ &sdhc_2 { > vqmmc-supply = <&vreg_l5a>; > > cd-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>; > + pinctrl-names = "default", "sleep"; > + pinctrl-0 = <&sdc2_state_on &sdc2_card_det_n>; > + pinctrl-1 = <&sdc2_state_off &sdc2_card_det_n>; This should have been pinctrl-n pinctrl-names I made a mistake in my lenovo dts if that was your reference.. You should also mention that the implicit removal of sdhci1's gpio properties from the lenovo j606f and oneplus billie2 is intentional as they both use UFS instead of eMMC. And one more thing, you missed bringing the CD pin back into pinctrl-0/1 in the tab dts. I'd really appreciate if you could fix up that ordering mess I mentioned above while at it. Konrad > > status = "okay"; > }; > > &tlmm { > gpio-reserved-ranges = <14 4>; > + > + sdc2_card_det_n: sd-card-det-n-state { > + pins = "gpio88"; > + function = "gpio"; > + drive-strength = <2>; > + bias-pull-up; > + }; > }; > > &ufs_mem_hc { > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > index fbd67d2c8d781..e8e5f2cafebb9 100644 > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > @@ -595,13 +595,6 @@ data-pins { > bias-pull-up; > drive-strength = <10>; > }; > - > - sd-cd-pins { > - pins = "gpio88"; > - function = "gpio"; > - bias-pull-up; > - drive-strength = <2>; > - }; > }; > > sdc2_state_off: sdc2-off-state { > @@ -622,13 +615,6 @@ data-pins { > bias-pull-up; > drive-strength = <2>; > }; > - > - sd-cd-pins { > - pins = "gpio88"; > - function = "gpio"; > - bias-disable; > - drive-strength = <2>; > - }; > }; > }; > > @@ -731,10 +717,6 @@ sdhc_1: mmc@4744000 { > <&gcc GCC_SDCC1_ICE_CORE_CLK>; > clock-names = "iface", "core", "xo", "ice"; > > - pinctrl-0 = <&sdc1_state_on>; > - pinctrl-1 = <&sdc1_state_off>; > - pinctrl-names = "default", "sleep"; > - > bus-width = <8>; > status = "disabled"; > }; > @@ -753,10 +735,6 @@ sdhc_2: mmc@4784000 { > <&rpmcc RPM_SMD_XO_CLK_SRC>; > clock-names = "iface", "core", "xo"; > > - pinctrl-0 = <&sdc2_state_on>; > - pinctrl-1 = <&sdc2_state_off>; > - pinctrl-names = "default", "sleep"; > - > power-domains = <&rpmpd SM6115_VDDCX>; > operating-points-v2 = <&sdhc2_opp_table>; > iommus = <&apps_smmu 0x00a0 0x0>;
On 2023-03-14 11:09:41, Konrad Dybcio wrote: > > > On 14.03.2023 08:40, Bhupesh Sharma wrote: > > Normally the 'pinctrl' properties of a SDHC controller and the > > chip detect pin settings are dependent on the type of the slots > > (for e.g uSD card slot), regulators and GPIO(s) available on the > > board(s). > > > > So, move the same from the sm6115 dtsi file to the respective > > board file(s). > So, file or files? :D > > > > > Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org> Not sure if I still stand by this... > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > Changes since v1: > > - v1 can be seen here: https://lore.kernel.org/linux-arm-msm/20221220113616.1556097-1-bhupesh.sharma@linaro.org/ > > - Colleted the R-B from Marijn. > > - Rebased on linux-next/master > > > > .../boot/dts/qcom/sm4250-oneplus-billie2.dts | 10 +++++++++ > > arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 ------------------- > > 2 files changed, 10 insertions(+), 22 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts > > index a3f1c7c41fd73..329eb496bbc5f 100644 > > --- a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts > > +++ b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts > > @@ -202,12 +202,22 @@ &sdhc_2 { > > vqmmc-supply = <&vreg_l5a>; > > > > cd-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>; > > + pinctrl-names = "default", "sleep"; > > + pinctrl-0 = <&sdc2_state_on &sdc2_card_det_n>; > > + pinctrl-1 = <&sdc2_state_off &sdc2_card_det_n>; > This should have been > > pinctrl-n > pinctrl-names > > I made a mistake in my lenovo dts if that was your reference.. > > You should also mention that the implicit removal of sdhci1's > gpio properties from the lenovo j606f and oneplus billie2 is intentional > as they both use UFS instead of eMMC. IMO we should keep the default sdc1_on/off_state in sdhci1 and sdhci2. That node is disabled anyway. Only removable sdhci's need the pinctrl extended with the CD pin (and whether that's done with a new pinctrl node, or by adding the pin to &sdcX_on/off_state varies per SoC). I mentioned this in: https://lore.kernel.org/linux-arm-msm/48607619-3a7b-a9d7-1e6a-c24f52539671@linaro.org/ But no-one gave their opinion. In other words: it seems odd to define the sdcX_on/off_state in SoC DTS, but then always require the board DTS to wire it up to the specific sdhc_X node (when that is only needed when extending pinctrl-X with an extra CD state). > And one more thing, you missed bringing the CD pin back into pinctrl-0/1 > in the tab dts. I'd really appreciate if you could fix up that ordering > mess I mentioned above while at it. > > Konrad > > > > status = "okay"; > > }; > > > > &tlmm { > > gpio-reserved-ranges = <14 4>; > > + > > + sdc2_card_det_n: sd-card-det-n-state { > > + pins = "gpio88"; > > + function = "gpio"; > > + drive-strength = <2>; > > + bias-pull-up; > > + }; > > }; > > > > &ufs_mem_hc { > > diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi > > index fbd67d2c8d781..e8e5f2cafebb9 100644 > > --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi > > @@ -595,13 +595,6 @@ data-pins { > > bias-pull-up; > > drive-strength = <10>; > > }; > > - > > - sd-cd-pins { > > - pins = "gpio88"; > > - function = "gpio"; > > - bias-pull-up; > > - drive-strength = <2>; > > - }; > > }; > > > > sdc2_state_off: sdc2-off-state { > > @@ -622,13 +615,6 @@ data-pins { > > bias-pull-up; > > drive-strength = <2>; > > }; > > - > > - sd-cd-pins { > > - pins = "gpio88"; > > - function = "gpio"; > > - bias-disable; > > - drive-strength = <2>; > > - }; > > }; > > }; > > > > @@ -731,10 +717,6 @@ sdhc_1: mmc@4744000 { > > <&gcc GCC_SDCC1_ICE_CORE_CLK>; > > clock-names = "iface", "core", "xo", "ice"; > > > > - pinctrl-0 = <&sdc1_state_on>; > > - pinctrl-1 = <&sdc1_state_off>; > > - pinctrl-names = "default", "sleep"; > > - In other words, IMO this should stay: - if it is not used for a removable card, this is adequate (only boards with a removable card on sdhc1 would have to update these properties or the referenced pinctrl state with CD pin); - the node is disabled by default anyway. > > bus-width = <8>; > > status = "disabled"; > > }; > > @@ -753,10 +735,6 @@ sdhc_2: mmc@4784000 { > > <&rpmcc RPM_SMD_XO_CLK_SRC>; > > clock-names = "iface", "core", "xo"; > > > > - pinctrl-0 = <&sdc2_state_on>; > > - pinctrl-1 = <&sdc2_state_off>; > > - pinctrl-names = "default", "sleep"; > > - And we could keep this for exactly the same reason. - Marijn > > power-domains = <&rpmpd SM6115_VDDCX>; > > operating-points-v2 = <&sdhc2_opp_table>; > > iommus = <&apps_smmu 0x00a0 0x0>;
On Tue, 14 Mar 2023 13:10:01 +0530, Bhupesh Sharma wrote: > Normally the 'pinctrl' properties of a SDHC controller and the > chip detect pin settings are dependent on the type of the slots > (for e.g uSD card slot), regulators and GPIO(s) available on the > board(s). > > So, move the same from the sm6115 dtsi file to the respective > board file(s). > > [...] Applied, thanks! [1/1] arm64: dts: qcom: sm6115: Move SDHC node(s)'s 'pinctrl' properties to dts commit: 27ad7815cb70cbfdcbee4368f41033e83731479f Best regards,
diff --git a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts index a3f1c7c41fd73..329eb496bbc5f 100644 --- a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts +++ b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts @@ -202,12 +202,22 @@ &sdhc_2 { vqmmc-supply = <&vreg_l5a>; cd-gpios = <&tlmm 88 GPIO_ACTIVE_HIGH>; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&sdc2_state_on &sdc2_card_det_n>; + pinctrl-1 = <&sdc2_state_off &sdc2_card_det_n>; status = "okay"; }; &tlmm { gpio-reserved-ranges = <14 4>; + + sdc2_card_det_n: sd-card-det-n-state { + pins = "gpio88"; + function = "gpio"; + drive-strength = <2>; + bias-pull-up; + }; }; &ufs_mem_hc { diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi index fbd67d2c8d781..e8e5f2cafebb9 100644 --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi @@ -595,13 +595,6 @@ data-pins { bias-pull-up; drive-strength = <10>; }; - - sd-cd-pins { - pins = "gpio88"; - function = "gpio"; - bias-pull-up; - drive-strength = <2>; - }; }; sdc2_state_off: sdc2-off-state { @@ -622,13 +615,6 @@ data-pins { bias-pull-up; drive-strength = <2>; }; - - sd-cd-pins { - pins = "gpio88"; - function = "gpio"; - bias-disable; - drive-strength = <2>; - }; }; }; @@ -731,10 +717,6 @@ sdhc_1: mmc@4744000 { <&gcc GCC_SDCC1_ICE_CORE_CLK>; clock-names = "iface", "core", "xo", "ice"; - pinctrl-0 = <&sdc1_state_on>; - pinctrl-1 = <&sdc1_state_off>; - pinctrl-names = "default", "sleep"; - bus-width = <8>; status = "disabled"; }; @@ -753,10 +735,6 @@ sdhc_2: mmc@4784000 { <&rpmcc RPM_SMD_XO_CLK_SRC>; clock-names = "iface", "core", "xo"; - pinctrl-0 = <&sdc2_state_on>; - pinctrl-1 = <&sdc2_state_off>; - pinctrl-names = "default", "sleep"; - power-domains = <&rpmpd SM6115_VDDCX>; operating-points-v2 = <&sdhc2_opp_table>; iommus = <&apps_smmu 0x00a0 0x0>;