From: sbhanu@codeaurora.org
To: Konrad Dybcio <konrad.dybcio@somainline.org>
Cc: adrian.hunter@intel.com, ulf.hansson@linaro.org,
robh+dt@kernel.org, asutoshd@codeaurora.org,
stummala@codeaurora.org, vbadigan@codeaurora.org,
rampraka@codeaurora.org, sayalil@codeaurora.org,
sartgarg@codeaurora.org, rnayak@codeaurora.org,
saiprakash.ranjan@codeaurora.org, sibis@codeaurora.org,
okukatla@codeaurora.org, djakov@kernel.org, cang@codeaurora.org,
pragalla@codeaurora.org, nitirawa@codeaurora.org,
linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
agross@kernel.org, bjorn.andersson@linaro.org
Subject: Re: [PATCH V3] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card
Date: Mon, 14 Jun 2021 16:55:17 +0530 [thread overview]
Message-ID: <1c22c4f5ca372988491181b3d0bd55e8@codeaurora.org> (raw)
In-Reply-To: <793c9596-73f3-42b2-291e-1c728e279e28@somainline.org>
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
next prev parent reply other threads:[~2021-06-14 11:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 15:20 [PATCH V3] arm64: dts: qcom: sc7280: Add nodes for eMMC and SD card Shaik Sajida Bhanu
2021-06-09 20:45 ` Konrad Dybcio
2021-06-14 11:25 ` sbhanu [this message]
2021-06-11 3:42 ` Bjorn Andersson
2021-06-14 11:30 ` sbhanu
2021-06-15 8:56 ` sbhanu
2021-06-16 0:08 ` Bjorn Andersson
2021-06-16 5:54 ` sbhanu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c22c4f5ca372988491181b3d0bd55e8@codeaurora.org \
--to=sbhanu@codeaurora.org \
--cc=adrian.hunter@intel.com \
--cc=agross@kernel.org \
--cc=asutoshd@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=cang@codeaurora.org \
--cc=devicetree@vger.kernel.org \
--cc=djakov@kernel.org \
--cc=konrad.dybcio@somainline.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=nitirawa@codeaurora.org \
--cc=okukatla@codeaurora.org \
--cc=pragalla@codeaurora.org \
--cc=rampraka@codeaurora.org \
--cc=rnayak@codeaurora.org \
--cc=robh+dt@kernel.org \
--cc=saiprakash.ranjan@codeaurora.org \
--cc=sartgarg@codeaurora.org \
--cc=sayalil@codeaurora.org \
--cc=sibis@codeaurora.org \
--cc=stummala@codeaurora.org \
--cc=ulf.hansson@linaro.org \
--cc=vbadigan@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).