2023-05-09 03:16:24

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH 1/2] dt-bindings: mmc: sdhci-msm: Document SC8280XP SDHCI

Add compatible for the SDHCI block found in SC8280XP.

Signed-off-by: Bjorn Andersson <[email protected]>
---
Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 1 +
1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
index 4f2d9e8127dd..9a87c03937c7 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
+++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
@@ -41,6 +41,7 @@ properties:
- qcom,qcs404-sdhci
- qcom,sc7180-sdhci
- qcom,sc7280-sdhci
+ - qcom,sc8280xp-sdhci
- qcom,sdm630-sdhci
- qcom,sdm670-sdhci
- qcom,sdm845-sdhci
--
2.25.1


2023-05-09 03:32:17

by Bjorn Andersson

[permalink] [raw]
Subject: [PATCH 2/2] arm64: dts: qcom: sc8280xp: Add SDC2 and enable on CRD

The CRD has Micro SD slot, introduce the necessary DeviceTree nodes for
enabling this.

Signed-off-by: Bjorn Andersson <[email protected]>
---
arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 80 +++++++++++++++++++++++
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 39 +++++++++++
2 files changed, 119 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index 5b25d54b9591..f83411e0e7f8 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -308,6 +308,13 @@ vreg_l1c: ldo1 {
regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
};

+ vreg_l6c: ldo6 {
+ regulator-name = "vreg_l6c";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <2960000>;
+ regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+ };
+
vreg_l7c: ldo7 {
regulator-name = "vreg_l7c";
regulator-min-microvolt = <2504000>;
@@ -318,6 +325,13 @@ vreg_l7c: ldo7 {
RPMH_REGULATOR_MODE_HPM>;
};

+ vreg_l9c: ldo9 {
+ regulator-name = "vreg_l9c";
+ regulator-min-microvolt = <2960000>;
+ regulator-max-microvolt = <2960000>;
+ regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
+ };
+
vreg_l13c: ldo13 {
regulator-name = "vreg_l13c";
regulator-min-microvolt = <3072000>;
@@ -600,6 +614,18 @@ &remoteproc_nsp0 {
status = "okay";
};

+&sdc2 {
+ cd-gpios = <&tlmm 131 GPIO_ACTIVE_LOW>;
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&sdc2_default_state>;
+ pinctrl-1 = <&sdc2_sleep_state>;
+ vmmc-supply = <&vreg_l9c>;
+ vqmmc-supply = <&vreg_l6c>;
+ no-sdio;
+ no-mmc;
+ status = "okay";
+};
+
&uart17 {
compatible = "qcom,geni-debug-uart";

@@ -842,6 +868,60 @@ wake-n-pins {
};
};

+ sdc2_default_state: sdc2-default-state {
+ clk-pins {
+ pins = "sdc2_clk";
+ drive-strength = <16>;
+ bias-disable;
+ };
+
+ cmd-pins {
+ pins = "sdc2_cmd";
+ drive-strength = <16>;
+ bias-pull-up;
+ };
+
+ data-pins {
+ pins = "sdc2_data";
+ drive-strength = <16>;
+ bias-pull-up;
+ };
+
+ card-detect-pins {
+ pins = "gpio131";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-disable;
+ };
+ };
+
+ sdc2_sleep_state: sdc2-sleep-state {
+ clk-pins {
+ pins = "sdc2_clk";
+ drive-strength = <2>;
+ bias-disable;
+ };
+
+ cmd-pins {
+ pins = "sdc2_cmd";
+ drive-strength = <2>;
+ bias-pull-up;
+ };
+
+ data-pins {
+ pins = "sdc2_data";
+ drive-strength = <2>;
+ bias-pull-up;
+ };
+
+ card-detect-pins {
+ pins = "gpio131";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-disable;
+ };
+ };
+
tpad_default: tpad-default-state {
int-n-pins {
pins = "gpio182";
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 8fa9fbfe5d00..21dfb48d923c 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -2815,6 +2815,45 @@ data-pins {
};
};

+ sdc2: mmc@8804000 {
+ compatible = "qcom,sc8280xp-sdhci", "qcom,sdhci-msm-v5";
+ reg = <0 0x08804000 0 0x1000>;
+
+ interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "hc_irq", "pwr_irq";
+
+ clocks = <&gcc GCC_SDCC2_AHB_CLK>,
+ <&gcc GCC_SDCC2_APPS_CLK>,
+ <&rpmhcc RPMH_CXO_CLK>;
+ clock-names = "iface", "core", "xo";
+ resets = <&gcc GCC_SDCC4_BCR>;
+ interconnects = <&aggre2_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_SDCC_2 0>;
+ interconnect-names = "sdhc-ddr","cpu-sdhc";
+ iommus = <&apps_smmu 0x4e0 0x0>;
+ power-domains = <&rpmhpd SC8280XP_CX>;
+ operating-points-v2 = <&sdc2_opp_table>;
+ bus-width = <4>;
+ dma-coherent;
+
+ status = "disabled";
+
+ sdc2_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ opp-100000000 {
+ opp-hz = /bits/ 64 <100000000>;
+ required-opps = <&rpmhpd_opp_low_svs>;
+ };
+
+ opp-202000000 {
+ opp-hz = /bits/ 64 <202000000>;
+ required-opps = <&rpmhpd_opp_svs_l1>;
+ };
+ };
+ };
+
usb_0_qmpphy: phy@88eb000 {
compatible = "qcom,sc8280xp-qmp-usb43dp-phy";
reg = <0 0x088eb000 0 0x4000>;
--
2.25.1

2023-05-09 06:25:39

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: mmc: sdhci-msm: Document SC8280XP SDHCI

On 09/05/2023 05:01, Bjorn Andersson wrote:
> Add compatible for the SDHCI block found in SC8280XP.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 1 +
> 1 file changed, 1 insertion(+)


Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

2023-05-09 06:55:38

by Manivannan Sadhasivam

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp: Add SDC2 and enable on CRD

On Mon, May 08, 2023 at 08:01:36PM -0700, Bjorn Andersson wrote:
> The CRD has Micro SD slot, introduce the necessary DeviceTree nodes for
> enabling this.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 80 +++++++++++++++++++++++
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 39 +++++++++++
> 2 files changed, 119 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index 5b25d54b9591..f83411e0e7f8 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -308,6 +308,13 @@ vreg_l1c: ldo1 {
> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> };
>
> + vreg_l6c: ldo6 {
> + regulator-name = "vreg_l6c";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <2960000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
> +
> vreg_l7c: ldo7 {
> regulator-name = "vreg_l7c";
> regulator-min-microvolt = <2504000>;
> @@ -318,6 +325,13 @@ vreg_l7c: ldo7 {
> RPMH_REGULATOR_MODE_HPM>;
> };
>
> + vreg_l9c: ldo9 {
> + regulator-name = "vreg_l9c";
> + regulator-min-microvolt = <2960000>;
> + regulator-max-microvolt = <2960000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
> +
> vreg_l13c: ldo13 {
> regulator-name = "vreg_l13c";
> regulator-min-microvolt = <3072000>;
> @@ -600,6 +614,18 @@ &remoteproc_nsp0 {
> status = "okay";
> };
>
> +&sdc2 {
> + cd-gpios = <&tlmm 131 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&sdc2_default_state>;
> + pinctrl-1 = <&sdc2_sleep_state>;
> + vmmc-supply = <&vreg_l9c>;
> + vqmmc-supply = <&vreg_l6c>;
> + no-sdio;
> + no-mmc;
> + status = "okay";
> +};
> +
> &uart17 {
> compatible = "qcom,geni-debug-uart";
>
> @@ -842,6 +868,60 @@ wake-n-pins {
> };
> };
>
> + sdc2_default_state: sdc2-default-state {
> + clk-pins {
> + pins = "sdc2_clk";
> + drive-strength = <16>;
> + bias-disable;
> + };
> +
> + cmd-pins {
> + pins = "sdc2_cmd";
> + drive-strength = <16>;
> + bias-pull-up;
> + };
> +
> + data-pins {
> + pins = "sdc2_data";
> + drive-strength = <16>;
> + bias-pull-up;
> + };
> +
> + card-detect-pins {
> + pins = "gpio131";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;

Since the card detect is active low, shouldn't the pin be pulled high to avoid
floating? Or is there an external pull up available on the board?

- Mani

> + };
> + };
> +
> + sdc2_sleep_state: sdc2-sleep-state {
> + clk-pins {
> + pins = "sdc2_clk";
> + drive-strength = <2>;
> + bias-disable;
> + };
> +
> + cmd-pins {
> + pins = "sdc2_cmd";
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> +
> + data-pins {
> + pins = "sdc2_data";
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> +
> + card-detect-pins {
> + pins = "gpio131";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + };
> + };
> +
> tpad_default: tpad-default-state {
> int-n-pins {
> pins = "gpio182";
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 8fa9fbfe5d00..21dfb48d923c 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -2815,6 +2815,45 @@ data-pins {
> };
> };
>
> + sdc2: mmc@8804000 {
> + compatible = "qcom,sc8280xp-sdhci", "qcom,sdhci-msm-v5";
> + reg = <0 0x08804000 0 0x1000>;
> +
> + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "hc_irq", "pwr_irq";
> +
> + clocks = <&gcc GCC_SDCC2_AHB_CLK>,
> + <&gcc GCC_SDCC2_APPS_CLK>,
> + <&rpmhcc RPMH_CXO_CLK>;
> + clock-names = "iface", "core", "xo";
> + resets = <&gcc GCC_SDCC4_BCR>;
> + interconnects = <&aggre2_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_SDCC_2 0>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> + iommus = <&apps_smmu 0x4e0 0x0>;
> + power-domains = <&rpmhpd SC8280XP_CX>;
> + operating-points-v2 = <&sdc2_opp_table>;
> + bus-width = <4>;
> + dma-coherent;
> +
> + status = "disabled";
> +
> + sdc2_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-100000000 {
> + opp-hz = /bits/ 64 <100000000>;
> + required-opps = <&rpmhpd_opp_low_svs>;
> + };
> +
> + opp-202000000 {
> + opp-hz = /bits/ 64 <202000000>;
> + required-opps = <&rpmhpd_opp_svs_l1>;
> + };
> + };
> + };
> +
> usb_0_qmpphy: phy@88eb000 {
> compatible = "qcom,sc8280xp-qmp-usb43dp-phy";
> reg = <0 0x088eb000 0 0x4000>;
> --
> 2.25.1
>

--
மணிவண்ணன் சதாசிவம்

2023-05-09 09:00:57

by Bhupesh Sharma

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: mmc: sdhci-msm: Document SC8280XP SDHCI

On Tue, 9 May 2023 at 08:31, Bjorn Andersson <[email protected]> wrote:
>
> Add compatible for the SDHCI block found in SC8280XP.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> index 4f2d9e8127dd..9a87c03937c7 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> @@ -41,6 +41,7 @@ properties:
> - qcom,qcs404-sdhci
> - qcom,sc7180-sdhci
> - qcom,sc7280-sdhci
> + - qcom,sc8280xp-sdhci
> - qcom,sdm630-sdhci
> - qcom,sdm670-sdhci
> - qcom,sdm845-sdhci
> --
> 2.25.1

Reviewed-by: Bhupesh Sharma <[email protected]>

Thanks.

2023-05-09 10:19:38

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH 1/2] dt-bindings: mmc: sdhci-msm: Document SC8280XP SDHCI

On Tue, 9 May 2023 at 05:01, Bjorn Andersson <[email protected]> wrote:
>
> Add compatible for the SDHCI block found in SC8280XP.
>
> Signed-off-by: Bjorn Andersson <[email protected]>

Applied for next, thanks!

Kind regards
Uffe


> ---
> Documentation/devicetree/bindings/mmc/sdhci-msm.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> index 4f2d9e8127dd..9a87c03937c7 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-msm.yaml
> @@ -41,6 +41,7 @@ properties:
> - qcom,qcs404-sdhci
> - qcom,sc7180-sdhci
> - qcom,sc7280-sdhci
> + - qcom,sc8280xp-sdhci
> - qcom,sdm630-sdhci
> - qcom,sdm670-sdhci
> - qcom,sdm845-sdhci
> --
> 2.25.1
>

2023-05-09 16:30:55

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp: Add SDC2 and enable on CRD

On Tue, May 09, 2023 at 12:02:05PM +0530, Manivannan Sadhasivam wrote:
> On Mon, May 08, 2023 at 08:01:36PM -0700, Bjorn Andersson wrote:
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
[..]
> > + sdc2_default_state: sdc2-default-state {
[..]
> > + card-detect-pins {
> > + pins = "gpio131";
> > + function = "gpio";
> > + drive-strength = <2>;
> > + bias-disable;
>
> Since the card detect is active low, shouldn't the pin be pulled high to avoid
> floating? Or is there an external pull up available on the board?
>

There's an external pull up resistor to vreg_s10b, so there shouldn't be
any reason to pull it internally as well.

Regards,
Bjorn

2023-05-16 01:36:19

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp: Add SDC2 and enable on CRD



On 9.05.2023 05:01, Bjorn Andersson wrote:
> The CRD has Micro SD slot, introduce the necessary DeviceTree nodes for
> enabling this.
>
> Signed-off-by: Bjorn Andersson <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 80 +++++++++++++++++++++++
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 39 +++++++++++
> 2 files changed, 119 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index 5b25d54b9591..f83411e0e7f8 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -308,6 +308,13 @@ vreg_l1c: ldo1 {
> regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> };
>
> + vreg_l6c: ldo6 {
> + regulator-name = "vreg_l6c";
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <2960000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> + };
> +
> vreg_l7c: ldo7 {
> regulator-name = "vreg_l7c";
> regulator-min-microvolt = <2504000>;
> @@ -318,6 +325,13 @@ vreg_l7c: ldo7 {
> RPMH_REGULATOR_MODE_HPM>;
> };
>
> + vreg_l9c: ldo9 {
> + regulator-name = "vreg_l9c";
> + regulator-min-microvolt = <2960000>;
> + regulator-max-microvolt = <2960000>;
> + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
Generally I ask people to add the missing regulator-allow-set-load,
but in case of the RPMh driver, should we also consider allowing LPM?

> + };
> +
> vreg_l13c: ldo13 {
> regulator-name = "vreg_l13c";
> regulator-min-microvolt = <3072000>;
> @@ -600,6 +614,18 @@ &remoteproc_nsp0 {
> status = "okay";
> };
>
> +&sdc2 {
> + cd-gpios = <&tlmm 131 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&sdc2_default_state>;
> + pinctrl-1 = <&sdc2_sleep_state>;
pinctrl-n
pinctrl-names

please
> + vmmc-supply = <&vreg_l9c>;
> + vqmmc-supply = <&vreg_l6c>;
> + no-sdio;
> + no-mmc;
> + status = "okay";
> +};
> +
> &uart17 {
> compatible = "qcom,geni-debug-uart";
>
> @@ -842,6 +868,60 @@ wake-n-pins {
> };
> };
>
> + sdc2_default_state: sdc2-default-state {
> + clk-pins {
> + pins = "sdc2_clk";
> + drive-strength = <16>;
> + bias-disable;
> + };
> +
> + cmd-pins {
> + pins = "sdc2_cmd";
> + drive-strength = <16>;
> + bias-pull-up;
> + };
> +
> + data-pins {
> + pins = "sdc2_data";
> + drive-strength = <16>;
> + bias-pull-up;
> + };
> +
> + card-detect-pins {
> + pins = "gpio131";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + };
> + };
> +
> + sdc2_sleep_state: sdc2-sleep-state {
> + clk-pins {
> + pins = "sdc2_clk";
> + drive-strength = <2>;
> + bias-disable;
> + };
> +
> + cmd-pins {
> + pins = "sdc2_cmd";
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> +
> + data-pins {
> + pins = "sdc2_data";
> + drive-strength = <2>;
> + bias-pull-up;
> + };
> +
> + card-detect-pins {
> + pins = "gpio131";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + };
> + };
That's totally SoC-specific, modulo the CD pin which can have
its own separate node and label

> +
> tpad_default: tpad-default-state {
> int-n-pins {
> pins = "gpio182";
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 8fa9fbfe5d00..21dfb48d923c 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -2815,6 +2815,45 @@ data-pins {
> };
> };
>
> + sdc2: mmc@8804000 {
> + compatible = "qcom,sc8280xp-sdhci", "qcom,sdhci-msm-v5";
> + reg = <0 0x08804000 0 0x1000>;
> +
> + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "hc_irq", "pwr_irq";
> +
> + clocks = <&gcc GCC_SDCC2_AHB_CLK>,
> + <&gcc GCC_SDCC2_APPS_CLK>,
> + <&rpmhcc RPMH_CXO_CLK>;
> + clock-names = "iface", "core", "xo";
> + resets = <&gcc GCC_SDCC4_BCR>;
4?

> + interconnects = <&aggre2_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_SDCC_2 0>;
> + interconnect-names = "sdhc-ddr","cpu-sdhc";
> + iommus = <&apps_smmu 0x4e0 0x0>;
> + power-domains = <&rpmhpd SC8280XP_CX>;
> + operating-points-v2 = <&sdc2_opp_table>;
> + bus-width = <4>;
> + dma-coherent;
> +
> + status = "disabled";
> +
> + sdc2_opp_table: opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-100000000 {
> + opp-hz = /bits/ 64 <100000000>;
> + required-opps = <&rpmhpd_opp_low_svs>;
You specified interconnects, but no bw values.. was that on purpose?

Other than these nits, lgtm
(generally, my dt sources don't even have sdhci to compare)

Konrad
> + };
> +
> + opp-202000000 {
> + opp-hz = /bits/ 64 <202000000>;
> + required-opps = <&rpmhpd_opp_svs_l1>;
> + };
> + };
> + };
> +
> usb_0_qmpphy: phy@88eb000 {
> compatible = "qcom,sc8280xp-qmp-usb43dp-phy";
> reg = <0 0x088eb000 0 0x4000>;

2023-05-16 16:45:47

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 2/2] arm64: dts: qcom: sc8280xp: Add SDC2 and enable on CRD

On Tue, May 16, 2023 at 03:22:41AM +0200, Konrad Dybcio wrote:
>
>
> On 9.05.2023 05:01, Bjorn Andersson wrote:
> > The CRD has Micro SD slot, introduce the necessary DeviceTree nodes for
> > enabling this.
> >
> > Signed-off-by: Bjorn Andersson <[email protected]>
> > ---
> > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 80 +++++++++++++++++++++++
> > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 39 +++++++++++
> > 2 files changed, 119 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > index 5b25d54b9591..f83411e0e7f8 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> > @@ -308,6 +308,13 @@ vreg_l1c: ldo1 {
> > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > };
> >
> > + vreg_l6c: ldo6 {
> > + regulator-name = "vreg_l6c";
> > + regulator-min-microvolt = <1800000>;
> > + regulator-max-microvolt = <2960000>;
> > + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > + };
> > +
> > vreg_l7c: ldo7 {
> > regulator-name = "vreg_l7c";
> > regulator-min-microvolt = <2504000>;
> > @@ -318,6 +325,13 @@ vreg_l7c: ldo7 {
> > RPMH_REGULATOR_MODE_HPM>;
> > };
> >
> > + vreg_l9c: ldo9 {
> > + regulator-name = "vreg_l9c";
> > + regulator-min-microvolt = <2960000>;
> > + regulator-max-microvolt = <2960000>;
> > + regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> Generally I ask people to add the missing regulator-allow-set-load,
> but in case of the RPMh driver, should we also consider allowing LPM?
>

I prefer to avoid LPM and dynamic load management, because I don't think
we're doing a good enough job across the kernel to ensure we get back to
HPM when needed.

At some point this needs to be re-evaluated though.

> > + };
> > +
> > vreg_l13c: ldo13 {
> > regulator-name = "vreg_l13c";
> > regulator-min-microvolt = <3072000>;
> > @@ -600,6 +614,18 @@ &remoteproc_nsp0 {
> > status = "okay";
> > };
> >
> > +&sdc2 {
> > + cd-gpios = <&tlmm 131 GPIO_ACTIVE_LOW>;
> > + pinctrl-names = "default", "sleep";
> > + pinctrl-0 = <&sdc2_default_state>;
> > + pinctrl-1 = <&sdc2_sleep_state>;
> pinctrl-n
> pinctrl-names
>
> please

That's ugly, but I see the symmetry to other -# vs -names

> > + vmmc-supply = <&vreg_l9c>;
> > + vqmmc-supply = <&vreg_l6c>;
> > + no-sdio;
> > + no-mmc;
> > + status = "okay";
> > +};
> > +
> > &uart17 {
> > compatible = "qcom,geni-debug-uart";
> >
> > @@ -842,6 +868,60 @@ wake-n-pins {
> > };
> > };
> >
> > + sdc2_default_state: sdc2-default-state {
> > + clk-pins {
> > + pins = "sdc2_clk";
> > + drive-strength = <16>;
> > + bias-disable;
> > + };
> > +
> > + cmd-pins {
> > + pins = "sdc2_cmd";
> > + drive-strength = <16>;
> > + bias-pull-up;
> > + };
> > +
> > + data-pins {
> > + pins = "sdc2_data";
> > + drive-strength = <16>;
> > + bias-pull-up;
> > + };
> > +
> > + card-detect-pins {
> > + pins = "gpio131";
> > + function = "gpio";
> > + drive-strength = <2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > + sdc2_sleep_state: sdc2-sleep-state {
> > + clk-pins {
> > + pins = "sdc2_clk";
> > + drive-strength = <2>;
> > + bias-disable;
> > + };
> > +
> > + cmd-pins {
> > + pins = "sdc2_cmd";
> > + drive-strength = <2>;
> > + bias-pull-up;
> > + };
> > +
> > + data-pins {
> > + pins = "sdc2_data";
> > + drive-strength = <2>;
> > + bias-pull-up;
> > + };
> > +
> > + card-detect-pins {
> > + pins = "gpio131";
> > + function = "gpio";
> > + drive-strength = <2>;
> > + bias-disable;
> > + };
> > + };
> That's totally SoC-specific, modulo the CD pin which can have
> its own separate node and label
>

The drive-strength and bias properties are board specific. Also, at this
time the CRD is the only board I'm aware of having the SD-card slot.

I suggest that we move it out of here once there's another user...

> > +
> > tpad_default: tpad-default-state {
> > int-n-pins {
> > pins = "gpio182";
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index 8fa9fbfe5d00..21dfb48d923c 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -2815,6 +2815,45 @@ data-pins {
> > };
> > };
> >
> > + sdc2: mmc@8804000 {
> > + compatible = "qcom,sc8280xp-sdhci", "qcom,sdhci-msm-v5";
> > + reg = <0 0x08804000 0 0x1000>;
> > +
> > + interrupts = <GIC_SPI 207 IRQ_TYPE_LEVEL_HIGH>,
> > + <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "hc_irq", "pwr_irq";
> > +
> > + clocks = <&gcc GCC_SDCC2_AHB_CLK>,
> > + <&gcc GCC_SDCC2_APPS_CLK>,
> > + <&rpmhcc RPMH_CXO_CLK>;
> > + clock-names = "iface", "core", "xo";
> > + resets = <&gcc GCC_SDCC4_BCR>;
> 4?
>

That's a typo. Thanks.

> > + interconnects = <&aggre2_noc MASTER_SDCC_2 0 &mc_virt SLAVE_EBI1 0>,
> > + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_SDCC_2 0>;
> > + interconnect-names = "sdhc-ddr","cpu-sdhc";
> > + iommus = <&apps_smmu 0x4e0 0x0>;
> > + power-domains = <&rpmhpd SC8280XP_CX>;
> > + operating-points-v2 = <&sdc2_opp_table>;
> > + bus-width = <4>;
> > + dma-coherent;
> > +
> > + status = "disabled";
> > +
> > + sdc2_opp_table: opp-table {
> > + compatible = "operating-points-v2";
> > +
> > + opp-100000000 {
> > + opp-hz = /bits/ 64 <100000000>;
> > + required-opps = <&rpmhpd_opp_low_svs>;
> You specified interconnects, but no bw values.. was that on purpose?
>

Assumed the driver did something clever, when I didn't see anything for
the other boards I looked at either. Will reconsider.

> Other than these nits, lgtm
> (generally, my dt sources don't even have sdhci to compare)
>

Thanks,
Bjorn

> Konrad
> > + };
> > +
> > + opp-202000000 {
> > + opp-hz = /bits/ 64 <202000000>;
> > + required-opps = <&rpmhpd_opp_svs_l1>;
> > + };
> > + };
> > + };
> > +
> > usb_0_qmpphy: phy@88eb000 {
> > compatible = "qcom,sc8280xp-qmp-usb43dp-phy";
> > reg = <0 0x088eb000 0 0x4000>;