arm64: dts: qcom: sm6115: Move SDHC node(s)'s 'pinctrl' properties to dts
Commit Message
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).
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
.../boot/dts/qcom/sm4250-oneplus-billie2.dts | 10 +++++++++
arch/arm64/boot/dts/qcom/sm6115.dtsi | 22 -------------------
2 files changed, 10 insertions(+), 22 deletions(-)
Comments
On 2022-12-20 17:06:16, 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).
I always thought it was okay to give the `sdcX_*` pins to the sdhc_X
node unconditionally in SoC DTSI, and leave board-dependent card-detect
(if this SDHCI slot is even used for removable cards) pins to the board
DTS.
> So, move the same from the sm6115 dtsi file to the respective
> board file(s).
>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
Thanks for cleaning this up, we're already using this pattern in quite a
few SoCs now.
Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
Though...
> ---
> .../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>;
>
> 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;
Note that SoC DTSI uses bias-disable in the off state.
> + };
> };
>
> &ufs_mem_hc {
> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> index 572bf04adf906..6be763d39870d 100644
> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
> @@ -518,13 +518,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 {
> @@ -545,13 +538,6 @@ data-pins {
> bias-pull-up;
> drive-strength = <2>;
> };
> -
> - sd-cd-pins {
> - pins = "gpio88";
> - function = "gpio";
> - bias-disable;
> - drive-strength = <2>;
> - };
> };
> };
>
> @@ -652,10 +638,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";
> -
You can probably leave these and below? Only boards needing to extend
pinctrl with board-specific SDCard pins would have to update/override
these properties that way?
- Marijn
> bus-width = <8>;
> status = "disabled";
> };
> @@ -672,10 +654,6 @@ sdhc_2: mmc@4784000 {
> clocks = <&gcc GCC_SDCC2_AHB_CLK>, <&gcc GCC_SDCC2_APPS_CLK>, <&xo_board>;
> 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>;
> --
> 2.38.1
>
Thanks Marjin for the review. Sorry for the delay in replying to your
comments.
On 12/22/22 3:40 AM, Marijn Suijten wrote:
> On 2022-12-20 17:06:16, 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).
>
> I always thought it was okay to give the `sdcX_*` pins to the sdhc_X
> node unconditionally in SoC DTSI, and leave board-dependent card-detect
> (if this SDHCI slot is even used for removable cards) pins to the board
> DTS.
>
>> So, move the same from the sm6115 dtsi file to the respective
>> board file(s).
>>
>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>
> Thanks for cleaning this up, we're already using this pattern in quite a
> few SoCs now.
Sure. But since they are being used in other SoC DTSIs doesn't guarantee
the correct demarcation between SoC DTSI and Board DTS :)
> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>
> Though...
>
>> ---
>> .../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>;
>>
>> 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;
>
> Note that SoC DTSI uses bias-disable in the off state.
The downstream uses bias-pull-up instead, as is the case with other
upstream qcom dtsi (see [1] and [2]).
[1].
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts#L1242
[2].
https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/sdm845-mtp.dts#L783
>> + };
>> };
>>
>> &ufs_mem_hc {
>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>> index 572bf04adf906..6be763d39870d 100644
>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>> @@ -518,13 +518,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 {
>> @@ -545,13 +538,6 @@ data-pins {
>> bias-pull-up;
>> drive-strength = <2>;
>> };
>> -
>> - sd-cd-pins {
>> - pins = "gpio88";
>> - function = "gpio";
>> - bias-disable;
>> - drive-strength = <2>;
>> - };
>> };
>> };
>>
>> @@ -652,10 +638,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";
>> -
>
> You can probably leave these and below? Only boards needing to extend
> pinctrl with board-specific SDCard pins would have to update/override
> these properties that way?
I disagree on this. But let's defer to @Bjorn for further inputs on the
same.
Thanks,
Bhupesh
>> bus-width = <8>;
>> status = "disabled";
>> };
>> @@ -672,10 +654,6 @@ sdhc_2: mmc@4784000 {
>> clocks = <&gcc GCC_SDCC2_AHB_CLK>, <&gcc GCC_SDCC2_APPS_CLK>, <&xo_board>;
>> 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>;
>> --
>> 2.38.1
>>
@@ -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 {
@@ -518,13 +518,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 {
@@ -545,13 +538,6 @@ data-pins {
bias-pull-up;
drive-strength = <2>;
};
-
- sd-cd-pins {
- pins = "gpio88";
- function = "gpio";
- bias-disable;
- drive-strength = <2>;
- };
};
};
@@ -652,10 +638,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";
};
@@ -672,10 +654,6 @@ sdhc_2: mmc@4784000 {
clocks = <&gcc GCC_SDCC2_AHB_CLK>, <&gcc GCC_SDCC2_APPS_CLK>, <&xo_board>;
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>;