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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <793c9596-73f3-42b2-291e-1c728e279e28@somainline.org>
Date:   Wed, 9 Jun 2021 22:45:43 +0200
From:   Konrad Dybcio <konrad.dybcio@...ainline.org>
To:     Shaik Sajida Bhanu <sbhanu@...eaurora.org>,
        adrian.hunter@...el.com, ulf.hansson@...aro.org, robh+dt@...nel.org
Cc:     asutoshd@...eaurora.org, stummala@...eaurora.org,
        vbadigan@...eaurora.org, rampraka@...eaurora.org,
        sayalil@...eaurora.org, sartgarg@...eaurora.org,
        rnayak@...eaurora.org, saiprakash.ranjan@...eaurora.org,
        sibis@...eaurora.org, okukatla@...eaurora.org, djakov@...nel.org,
        cang@...eaurora.org, pragalla@...eaurora.org,
        nitirawa@...eaurora.org, linux-mmc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        devicetree@...r.kernel.org, agross@...nel.org,
        bjorn.andersson@...aro.org
Subject: Re: [PATCH V3] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD
 card

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])



> +
> +	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



> +&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.



> +		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.



> 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.



> +			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



[1] https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/commit/?h=for-next&id=35a4a8b6e9b133cf3a7d059ad4cf0e24cb4bd029


Konrad