Messages in this thread | | | Date | Wed, 8 Feb 2023 15:21:48 +0530 | Subject | Re: [PATCH] arm64: dts: qcom: sm6115: Move SDHC node(s)'s 'pinctrl' properties to dts | From | Bhupesh Sharma <> |
| |
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 >>
| |