Re: [PATCH V3] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card

[sbhanu]

On 2021-06-10 02:15, Konrad Dybcio wrote:
> Hi,
>
>
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts 
> > b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > index 3900cfc..8b159d1 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
> > @@ -11,6 +11,7 @@
> >  #include <dt-bindings/iio/qcom,spmi-adc7-pmr735b.h>
> >  #include <dt-bindings/iio/qcom,spmi-adc7-pm8350.h>
> >  #include <dt-bindings/iio/qcom,spmi-adc7-pmk8350.h>
> > +#include <dt-bindings/gpio/gpio.h>
> >  #include "sc7280.dtsi"
> >  #include "pm7325.dtsi"
> >  #include "pmr735a.dtsi"
> > @@ -272,6 +273,36 @@
> >  	status = "okay";
> >  };
> >
> > +&sdhc_1 {
> > +	status = "okay";
> > +
> > +	pinctrl-names = "default", "sleep";
> > +	pinctrl-0 = <&sdc1_clk &sdc1_cmd &sdc1_data &sdc1_rclk>;
> > +	pinctrl-1 = <&sdc1_clk_sleep &sdc1_cmd_sleep &sdc1_data_sleep 
> > &sdc1_rclk_sleep>;
>
> Please condense these pins into a since sdc1_on_state/sdc1_off_state
> (check sdc1_state_on in [1])
Sure
> > +
> > +	non-removable;
> > +	no-sd;
> > +	no-sdio;
> > +
> > +	vmmc-supply = <&vreg_l7b_2p9>;
> > +	vqmmc-supply = <&vreg_l19b_1p8>;
> > +
> > +};
> > +
> > +&sdhc_2 {
> > +	status = "okay";
> > +
> > +	pinctrl-names = "default", "sleep";
> > +	pinctrl-0 = <&sdc2_clk &sdc2_cmd &sdc2_data &sd_cd>;
> > +	pinctrl-1 = <&sdc2_clk_sleep &sdc2_cmd_sleep &sdc2_data_sleep>;
>
> Ditto
Sure
> > +&tlmm {
> > +	sdc1_clk: sdc1-clk {
> > +		pins = "sdc1_clk";
> > +		bias-disable;
> > +		drive-strength = <16>;
> > +	};
> > +
> > +	sdc1_cmd: sdc1-cmd {
> > +		pins = "sdc1_cmd";
> > +		bias-pull-up;
> > +		drive-strength = <10>;
> > +	};
> > +
> > +	sdc1_data: sdc1-data {
> > +		pins = "sdc1_data";
> > +		bias-pull-up;
> > +		drive-strength = <10>;
> > +	};
> > +	sdc1_rclk: sdc1-rclk {
> > +		pins = "sdc1_rclk";
> > +		bias-pull-down;
> > +	};
> > +
> > +	sdc2_clk: sdc2-clk {
> > +		pins = "sdc2_clk";
> > +		bias-disable;
> > +		drive-strength = <16>;
> > +	};
> > +
> > +	sdc2_cmd: sdc2-cmd {
> > +		pins = "sdc2_cmd";
> > +		bias-pull-up;
> > +		drive-strength = <10>;
> > +	};
> > +
> > +	sdc2_data: sdc2-data {
> > +		pins = "sdc2_data";
> > +		bias-pull-up;
> > +		drive-strength = <10>;
> > +	};
> > +
> > +	sd_cd: sd-cd {
>
> Please make it sdc2 to keep things coherent.
Sure
> > +		pins = "gpio91";
> > +		bias-pull-up;
> > +	};
> > +
> > +};
>
> Why are you defining on_state pins in the device dt and sleep state in
> the SoC one?
>
> Most devices share a common config for these, often coming from MTP or
> QRD boards
>
> and it makes little to no sense to define these separately every time,
> because if you hit the
>
> rare case of needing to make a change against that, it's probably just
> drive-strength.
I have made this change as per Doug Anderson comment on patch V2 
(https://lore.kernel.org/patchwork/patch/1399453/#1598871)
And along with drive-strength change, on some of boards Sd card gpio pin 
also may change right.
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
> > b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > index 0b6f119..eab6f7b 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > @@ -24,6 +24,11 @@
> >
> >  	chosen { };
> >
> > +	aliases {
> > +		mmc1 = &sdhc_1;
> > +		mmc2 = &sdhc_2;
> > +	};
>
> This is board specific. Something might have a SDIO Wi-Fi card on it.
we are assuming eMMC and SD support available on all boards and if any 
board supports SDIO Wi-Fi card
they can add aliases for SDIO Wi-Fi card in that particular board device 
DT file.
> > +			mmc-ddr-1_8v;
> > +			mmc-hs200-1_8v;
> > +			mmc-hs400-1_8v;
> > +			mmc-hs400-enhanced-strobe;
>
> These properties should probably be in the device DT, unless the SoC 
> controller
>
> can only support these speeds and only at 1.8v
we are keeping these flags in SOC file because all platforms would 
support these modes,
and yes we support only these and at 1.8V.
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=35a4a8b6e9b133cf3a7d059ad4cf0e24cb4bd029
>
>
> Konrad