[PATCH v3 0/2] Enable SDHC1 and SDHC2 on QCS615

[PATCH v3 0/2] Enable SDHC1 and SDHC2 on QCS615

[Yuanjie Yang]

Add SDHC1 and SDHC2 support to the QCS615 Ride platform. The
SDHC1 and SDHC2 of QCS615 are derived from SM6115. Include
the configuration of SDHC1-related and SDHC2-related opp,
power, and interconnect settings in the device tree.

Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
---
This patch series depends on below patch series:
https://lore.kernel.org/all/20241104-add_initial_support_for_qcs615-v5-0-9dde8d7b80b0@quicinc.com/
https://lore.kernel.org/all/20241105032107.9552-1-quic_qqzhou@quicinc.com/

Changes in v3:
- Improve the commit messages and cover letter
- Link to v2: https://lore.kernel.org/all/20241106072343.2070933-1-quic_yuanjiey@quicinc.com/

Changes in v2:
- Improve the commit messages and cover letter
- Remove applied patches 1
- Pad sdhc_1 node and sdhc_2 node register addresses to 8 hex digits
- Adjust sdhc_1 node and sdhc_2 node register addresses to hexadecimal
- Modify sdhc_2 vqmmc-supply incorrect power configuration
- Link to v1: https://lore.kernel.org/all/20241023092708.604195-1-quic_yuanjiey@quicinc.com/

---
Yuanjie Yang (2):
  arm64: dts: qcom: qcs615: add SDHC1 and SDHC2
  arm64: dts: qcom: qcs615-ride: Enable SDHC1 and SDHC2

 arch/arm64/boot/dts/qcom/qcs615-ride.dts |  31 ++++
 arch/arm64/boot/dts/qcom/qcs615.dtsi     | 198 +++++++++++++++++++++++
 2 files changed, 229 insertions(+)

-- 
2.34.1

[PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Yuanjie Yang]

Add SDHC1 and SDHC2 support to the QCS615 Ride platform.

Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
index 590beb37f441..37c6ab217c96 100644
--- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
@@ -399,6 +399,65 @@ qfprom: efuse@780000 {
 			#size-cells = <1>;
 		};
 
+		sdhc_1: mmc@7c4000 {
+			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
+			reg = <0x0 0x007c4000 0x0 0x1000>,
+			      <0x0 0x007c5000 0x0 0x1000>;
+			reg-names = "hc",
+				    "cqhci";
+
+			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "hc_irq",
+					  "pwr_irq";
+
+			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
+				 <&gcc GCC_SDCC1_APPS_CLK>,
+				 <&rpmhcc RPMH_CXO_CLK>,
+				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
+			clock-names = "iface",
+				      "core",
+				      "xo",
+				      "ice";
+
+			resets = <&gcc GCC_SDCC1_BCR>;
+
+			power-domains = <&rpmhpd RPMHPD_CX>;
+			operating-points-v2 = <&sdhc1_opp_table>;
+			iommus = <&apps_smmu 0x02c0 0x0>;
+			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
+					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
+			interconnect-names = "sdhc-ddr",
+					     "cpu-sdhc";
+
+			bus-width = <8>;
+			qcom,dll-config = <0x000f642c>;
+			qcom,ddr-config = <0x80040868>;
+			supports-cqe;
+			dma-coherent;
+			mmc-ddr-1_8v;
+			mmc-hs200-1_8v;
+			mmc-hs400-1_8v;
+			mmc-hs400-enhanced-strobe;
+			status = "disabled";
+
+			sdhc1_opp_table: opp-table {
+				compatible = "operating-points-v2";
+
+				opp-100000000 {
+					opp-hz = /bits/ 64 <100000000>;
+					required-opps = <&rpmhpd_opp_svs>;
+				};
+
+				opp-384000000 {
+					opp-hz = /bits/ 64 <384000000>;
+					required-opps = <&rpmhpd_opp_nom>;
+				};
+			};
+		};
+
 		qupv3_id_0: geniqup@8c0000 {
 			compatible = "qcom,geni-se-qup";
 			reg = <0x0 0x008c0000 0x0 0x6000>;
@@ -494,6 +553,145 @@ qup_uart0_rx: qup-uart0-rx-state {
 				pins = "gpio17";
 				function = "qup0";
 			};
+
+			sdc1_state_on: sdc1-on-state {
+				clk-pins {
+					pins = "sdc1_clk";
+					bias-disable;
+					drive-strength = <16>;
+				};
+
+				cmd-pins {
+					pins = "sdc1_cmd";
+					bias-pull-up;
+					drive-strength = <10>;
+				};
+
+				data-pins {
+					pins = "sdc1_data";
+					bias-pull-up;
+					drive-strength = <10>;
+				};
+
+				rclk-pins {
+					pins = "sdc1_rclk";
+					bias-pull-down;
+				};
+			};
+
+			sdc1_state_off: sdc1-off-state {
+				clk-pins {
+					pins = "sdc1_clk";
+					bias-disable;
+					drive-strength = <2>;
+				};
+
+				cmd-pins {
+					pins = "sdc1_cmd";
+					bias-pull-up;
+					drive-strength = <2>;
+				};
+
+				data-pins {
+					pins = "sdc1_data";
+					bias-pull-up;
+					drive-strength = <2>;
+				};
+
+				rclk-pins {
+					pins = "sdc1_rclk";
+					bias-pull-down;
+				};
+			};
+
+			sdc2_state_on: sdc2-on-state {
+				clk-pins {
+					pins = "sdc2_clk";
+					bias-disable;
+					drive-strength = <16>;
+				};
+
+				cmd-pins {
+					pins = "sdc2_cmd";
+					bias-pull-up;
+					drive-strength = <10>;
+				};
+
+				data-pins {
+					pins = "sdc2_data";
+					bias-pull-up;
+					drive-strength = <10>;
+				};
+			};
+
+			sdc2_state_off: sdc2-off-state {
+				clk-pins {
+					pins = "sdc2_clk";
+					bias-disable;
+					drive-strength = <2>;
+				};
+
+				cmd-pins {
+					pins = "sdc2_cmd";
+					bias-pull-up;
+					drive-strength = <2>;
+				};
+
+				data-pins {
+					pins = "sdc2_data";
+					bias-pull-up;
+					drive-strength = <2>;
+				};
+			};
+		};
+
+		sdhc_2: mmc@8804000 {
+			compatible = "qcom,qcs615-sdhci","qcom,sdhci-msm-v5";
+			reg = <0x0 0x08804000 0x0 0x1000>;
+			reg-names = "hc";
+
+			interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 222 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";
+
+			power-domains = <&rpmhpd RPMHPD_CX>;
+			operating-points-v2 = <&sdhc2_opp_table>;
+			iommus = <&apps_smmu 0x02a0 0x0>;
+			resets = <&gcc GCC_SDCC2_BCR>;
+			interconnects = <&aggre1_noc MASTER_SDCC_2 QCOM_ICC_TAG_ALWAYS
+					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
+					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
+					 &config_noc SLAVE_SDCC_2 QCOM_ICC_TAG_ALWAYS>;
+			interconnect-names = "sdhc-ddr",
+					     "cpu-sdhc";
+
+			bus-width = <4>;
+			qcom,dll-config = <0x0007642c>;
+			qcom,ddr-config = <0x80040868>;
+			dma-coherent;
+			status = "disabled";
+
+			sdhc2_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_nom>;
+				};
+			};
 		};
 
 		dc_noc: interconnect@9160000 {
-- 
2.34.1

[PATCH v3 2/2] arm64: dts: qcom: qcs615-ride: enable SDHC1 and SDHC2

[Yuanjie Yang]

Enable SDHC1 and SDHC2 on the Qualcomm QCS615 Ride platform.

Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615-ride.dts | 31 ++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
index ee6cab3924a6..308fd741a467 100644
--- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
+++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
@@ -5,6 +5,7 @@
 /dts-v1/;
 
 #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
+#include <dt-bindings/gpio/gpio.h>
 #include "qcs615.dtsi"
 / {
 	model = "Qualcomm Technologies, Inc. QCS615 Ride";
@@ -12,6 +13,8 @@ / {
 	chassis-type = "embedded";
 
 	aliases {
+		mmc0 = &sdhc_1;
+		mmc1 = &sdhc_2;
 		serial0 = &uart0;
 	};
 
@@ -210,6 +213,34 @@ &rpmhcc {
 	clocks = <&xo_board_clk>;
 };
 
+&sdhc_1 {
+	pinctrl-0 = <&sdc1_state_on>;
+	pinctrl-1 = <&sdc1_state_off>;
+	pinctrl-names = "default", "sleep";
+
+	vmmc-supply = <&vreg_l17a>;
+	vqmmc-supply = <&vreg_s4a>;
+
+	non-removable;
+	no-sd;
+	no-sdio;
+
+	status = "okay";
+};
+
+&sdhc_2 {
+	pinctrl-0 = <&sdc2_state_on>;
+	pinctrl-1 = <&sdc2_state_off>;
+	pinctrl-names = "default", "sleep";
+
+	cd-gpios = <&tlmm 99 GPIO_ACTIVE_LOW>;
+
+	vmmc-supply = <&vreg_l10a>;
+	vqmmc-supply = <&vreg_s4a>;
+
+	status = "okay";
+};
+
 &uart0 {
 	status = "okay";
 };
-- 
2.34.1

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Krzysztof Kozlowski]

On 22/11/2024 07:51, Yuanjie Yang wrote:
> Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
> 
> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
>  1 file changed, 198 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> index 590beb37f441..37c6ab217c96 100644
> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
>  			#size-cells = <1>;
>  		};
>  
> +		sdhc_1: mmc@7c4000 {
> +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
> +			reg = <0x0 0x007c4000 0x0 0x1000>,
> +			      <0x0 0x007c5000 0x0 0x1000>;
> +			reg-names = "hc",
> +				    "cqhci";
> +
> +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "hc_irq",
> +					  "pwr_irq";
> +
> +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
> +				 <&gcc GCC_SDCC1_APPS_CLK>,
> +				 <&rpmhcc RPMH_CXO_CLK>,
> +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
> +			clock-names = "iface",
> +				      "core",
> +				      "xo",
> +				      "ice";
> +
> +			resets = <&gcc GCC_SDCC1_BCR>;
> +
> +			power-domains = <&rpmhpd RPMHPD_CX>;
> +			operating-points-v2 = <&sdhc1_opp_table>;
> +			iommus = <&apps_smmu 0x02c0 0x0>;
> +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "sdhc-ddr",
> +					     "cpu-sdhc";
> +
> +			bus-width = <8>;
> +			qcom,dll-config = <0x000f642c>;
> +			qcom,ddr-config = <0x80040868>;
> +			supports-cqe;
> +			dma-coherent;
> +			mmc-ddr-1_8v;
> +			mmc-hs200-1_8v;
> +			mmc-hs400-1_8v;
> +			mmc-hs400-enhanced-strobe;

These are properties of memory, not SoC. If the node is disabled, means
memory is not attached to the SoC, right?

> +			status = "disabled";




...

> +
> +		sdhc_2: mmc@8804000 {
> +			compatible = "qcom,qcs615-sdhci","qcom,sdhci-msm-v5";
> +			reg = <0x0 0x08804000 0x0 0x1000>;
> +			reg-names = "hc";
> +
> +			interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 222 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";
> +
> +			power-domains = <&rpmhpd RPMHPD_CX>;
> +			operating-points-v2 = <&sdhc2_opp_table>;
> +			iommus = <&apps_smmu 0x02a0 0x0>;
> +			resets = <&gcc GCC_SDCC2_BCR>;
> +			interconnects = <&aggre1_noc MASTER_SDCC_2 QCOM_ICC_TAG_ALWAYS
> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> +					 &config_noc SLAVE_SDCC_2 QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "sdhc-ddr",
> +					     "cpu-sdhc";
> +
> +			bus-width = <4>;

Same comments.

> +			qcom,dll-config = <0x0007642c>;
> +			qcom,ddr-config = <0x80040868>;
> +			dma-coherent;
> +			status = "disabled";
> +
> +			sdhc2_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_nom>;
> +				};
> +			};
>  		};
>  
>  		dc_noc: interconnect@9160000 {


Best regards,
Krzysztof

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Yuanjie Yang]

On Fri, Nov 22, 2024 at 08:04:31AM +0100, Krzysztof Kozlowski wrote:
> On 22/11/2024 07:51, Yuanjie Yang wrote:
> > Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
> > 
> > Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
> >  1 file changed, 198 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > index 590beb37f441..37c6ab217c96 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
> >  			#size-cells = <1>;
> >  		};
> >  
> > +		sdhc_1: mmc@7c4000 {
> > +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
> > +			reg = <0x0 0x007c4000 0x0 0x1000>,
> > +			      <0x0 0x007c5000 0x0 0x1000>;
> > +			reg-names = "hc",
> > +				    "cqhci";
> > +
> > +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "hc_irq",
> > +					  "pwr_irq";
> > +
> > +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
> > +				 <&gcc GCC_SDCC1_APPS_CLK>,
> > +				 <&rpmhcc RPMH_CXO_CLK>,
> > +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
> > +			clock-names = "iface",
> > +				      "core",
> > +				      "xo",
> > +				      "ice";
> > +
> > +			resets = <&gcc GCC_SDCC1_BCR>;
> > +
> > +			power-domains = <&rpmhpd RPMHPD_CX>;
> > +			operating-points-v2 = <&sdhc1_opp_table>;
> > +			iommus = <&apps_smmu 0x02c0 0x0>;
> > +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
> > +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> > +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> > +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
> > +			interconnect-names = "sdhc-ddr",
> > +					     "cpu-sdhc";
> > +
> > +			bus-width = <8>;
> > +			qcom,dll-config = <0x000f642c>;
> > +			qcom,ddr-config = <0x80040868>;
> > +			supports-cqe;
> > +			dma-coherent;
> > +			mmc-ddr-1_8v;
> > +			mmc-hs200-1_8v;
> > +			mmc-hs400-1_8v;
> > +			mmc-hs400-enhanced-strobe;
> 
> These are properties of memory, not SoC. If the node is disabled, means
> memory is not attached to the SoC, right?
> 
> > +			status = "disabled";
Thanks, I think qcom,dll-config and qcom,ddr-config are properties of Soc,
they are memory configurations that need to be written to the ioaddr.
And mmc-ddr-1_8v,mmc-hs200-1_8v,mmc-hs400-1_8v are bus speed config,
they indicate the bus speed at which the host contoller can operate.
If the node is disabled, which means Soc don't support these properties.

> 
> 
> ...
> 
> > +
> > +		sdhc_2: mmc@8804000 {
> > +			compatible = "qcom,qcs615-sdhci","qcom,sdhci-msm-v5";
> > +			reg = <0x0 0x08804000 0x0 0x1000>;
> > +			reg-names = "hc";
> > +
> > +			interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 222 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";
> > +
> > +			power-domains = <&rpmhpd RPMHPD_CX>;
> > +			operating-points-v2 = <&sdhc2_opp_table>;
> > +			iommus = <&apps_smmu 0x02a0 0x0>;
> > +			resets = <&gcc GCC_SDCC2_BCR>;
> > +			interconnects = <&aggre1_noc MASTER_SDCC_2 QCOM_ICC_TAG_ALWAYS
> > +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> > +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> > +					 &config_noc SLAVE_SDCC_2 QCOM_ICC_TAG_ALWAYS>;
> > +			interconnect-names = "sdhc-ddr",
> > +					     "cpu-sdhc";
> > +
> > +			bus-width = <4>;
> 
> Same comments.

Thanks, I think qcom,dll-config and qcom,ddr-config are properties of Soc,
they are memory configurations that need to be written to the ioaddr.
And mmc-ddr-1_8v,mmc-hs200-1_8v,mmc-hs400-1_8v are bus speed config,
they indicate the bus speed at which the host controller can operate.
If the node is disabled, which means Soc don't support these properties.


> > +			qcom,dll-config = <0x0007642c>;
> > +			qcom,ddr-config = <0x80040868>;
> > +			dma-coherent;
> > +			status = "disabled";
> > +
> > +			sdhc2_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_nom>;
> > +				};
> > +			};
> >  		};
> >  
> >  		dc_noc: interconnect@9160000 {
> 
> 
> Best regards,
> Krzysztof

Thanks,
Yuanjie

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Krzysztof Kozlowski]

On 22/11/2024 09:40, Yuanjie Yang wrote:
> On Fri, Nov 22, 2024 at 08:04:31AM +0100, Krzysztof Kozlowski wrote:
>> On 22/11/2024 07:51, Yuanjie Yang wrote:
>>> Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
>>>
>>> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
>>>  1 file changed, 198 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>> index 590beb37f441..37c6ab217c96 100644
>>> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>> @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
>>>  			#size-cells = <1>;
>>>  		};
>>>  
>>> +		sdhc_1: mmc@7c4000 {
>>> +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
>>> +			reg = <0x0 0x007c4000 0x0 0x1000>,
>>> +			      <0x0 0x007c5000 0x0 0x1000>;
>>> +			reg-names = "hc",
>>> +				    "cqhci";
>>> +
>>> +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
>>> +			interrupt-names = "hc_irq",
>>> +					  "pwr_irq";
>>> +
>>> +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
>>> +				 <&gcc GCC_SDCC1_APPS_CLK>,
>>> +				 <&rpmhcc RPMH_CXO_CLK>,
>>> +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
>>> +			clock-names = "iface",
>>> +				      "core",
>>> +				      "xo",
>>> +				      "ice";
>>> +
>>> +			resets = <&gcc GCC_SDCC1_BCR>;
>>> +
>>> +			power-domains = <&rpmhpd RPMHPD_CX>;
>>> +			operating-points-v2 = <&sdhc1_opp_table>;
>>> +			iommus = <&apps_smmu 0x02c0 0x0>;
>>> +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
>>> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>>> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
>>> +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
>>> +			interconnect-names = "sdhc-ddr",
>>> +					     "cpu-sdhc";
>>> +
>>> +			bus-width = <8>;
>>> +			qcom,dll-config = <0x000f642c>;
>>> +			qcom,ddr-config = <0x80040868>;
>>> +			supports-cqe;
>>> +			dma-coherent;
>>> +			mmc-ddr-1_8v;
>>> +			mmc-hs200-1_8v;
>>> +			mmc-hs400-1_8v;
>>> +			mmc-hs400-enhanced-strobe;
>>
>> These are properties of memory, not SoC. If the node is disabled, means
>> memory is not attached to the SoC, right?
>>
>>> +			status = "disabled";
> Thanks, I think qcom,dll-config and qcom,ddr-config are properties of Soc,
> they are memory configurations that need to be written to the ioaddr.
> And mmc-ddr-1_8v,mmc-hs200-1_8v,mmc-hs400-1_8v are bus speed config,
> they indicate the bus speed at which the host contoller can operate.
> If the node is disabled, which means Soc don't support these properties.
No, that is not the meaning of node is disabled. When node is disabled,
it means board does not have attached memory.

Move the memory related properties  to the board.
Best regards,
Krzysztof

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Yuanjie Yang]

On Fri, Nov 22, 2024 at 01:35:28PM +0100, Krzysztof Kozlowski wrote:
> On 22/11/2024 09:40, Yuanjie Yang wrote:
> > On Fri, Nov 22, 2024 at 08:04:31AM +0100, Krzysztof Kozlowski wrote:
> >> On 22/11/2024 07:51, Yuanjie Yang wrote:
> >>> Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
> >>>
> >>> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> >>> ---
> >>>  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
> >>>  1 file changed, 198 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> >>> index 590beb37f441..37c6ab217c96 100644
> >>> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> >>> @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
> >>>  			#size-cells = <1>;
> >>>  		};
> >>>  
> >>> +		sdhc_1: mmc@7c4000 {
> >>> +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
> >>> +			reg = <0x0 0x007c4000 0x0 0x1000>,
> >>> +			      <0x0 0x007c5000 0x0 0x1000>;
> >>> +			reg-names = "hc",
> >>> +				    "cqhci";
> >>> +
> >>> +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> >>> +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
> >>> +			interrupt-names = "hc_irq",
> >>> +					  "pwr_irq";
> >>> +
> >>> +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
> >>> +				 <&gcc GCC_SDCC1_APPS_CLK>,
> >>> +				 <&rpmhcc RPMH_CXO_CLK>,
> >>> +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
> >>> +			clock-names = "iface",
> >>> +				      "core",
> >>> +				      "xo",
> >>> +				      "ice";
> >>> +
> >>> +			resets = <&gcc GCC_SDCC1_BCR>;
> >>> +
> >>> +			power-domains = <&rpmhpd RPMHPD_CX>;
> >>> +			operating-points-v2 = <&sdhc1_opp_table>;
> >>> +			iommus = <&apps_smmu 0x02c0 0x0>;
> >>> +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
> >>> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> >>> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> >>> +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
> >>> +			interconnect-names = "sdhc-ddr",
> >>> +					     "cpu-sdhc";
> >>> +
> >>> +			bus-width = <8>;
> >>> +			qcom,dll-config = <0x000f642c>;
> >>> +			qcom,ddr-config = <0x80040868>;
> >>> +			supports-cqe;
> >>> +			dma-coherent;
> >>> +			mmc-ddr-1_8v;
> >>> +			mmc-hs200-1_8v;
> >>> +			mmc-hs400-1_8v;
> >>> +			mmc-hs400-enhanced-strobe;
> >>
> >> These are properties of memory, not SoC. If the node is disabled, means
> >> memory is not attached to the SoC, right?
> >>
> >>> +			status = "disabled";
> > Thanks, I think qcom,dll-config and qcom,ddr-config are properties of Soc,
> > they are memory configurations that need to be written to the ioaddr.
> > And mmc-ddr-1_8v,mmc-hs200-1_8v,mmc-hs400-1_8v are bus speed config,
> > they indicate the bus speed at which the host contoller can operate.
> > If the node is disabled, which means Soc don't support these properties.
> No, that is not the meaning of node is disabled. When node is disabled,
> it means board does not have attached memory.
> 
> Move the memory related properties  to the board.

Thanks, Ok I understand, I will move the memory related
properties(qcom,dll-config and qcom,ddr-config) to the
board dts in next version.

> Best regards,
> Krzysztof

Thanks,
Yuanjie

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Krzysztof Kozlowski]

On 25/11/2024 03:20, Yuanjie Yang wrote:
> On Fri, Nov 22, 2024 at 01:35:28PM +0100, Krzysztof Kozlowski wrote:
>> On 22/11/2024 09:40, Yuanjie Yang wrote:
>>> On Fri, Nov 22, 2024 at 08:04:31AM +0100, Krzysztof Kozlowski wrote:
>>>> On 22/11/2024 07:51, Yuanjie Yang wrote:
>>>>> Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
>>>>>
>>>>> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
>>>>>  1 file changed, 198 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>>> index 590beb37f441..37c6ab217c96 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>>> @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
>>>>>  			#size-cells = <1>;
>>>>>  		};
>>>>>  
>>>>> +		sdhc_1: mmc@7c4000 {
>>>>> +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
>>>>> +			reg = <0x0 0x007c4000 0x0 0x1000>,
>>>>> +			      <0x0 0x007c5000 0x0 0x1000>;
>>>>> +			reg-names = "hc",
>>>>> +				    "cqhci";
>>>>> +
>>>>> +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
>>>>> +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
>>>>> +			interrupt-names = "hc_irq",
>>>>> +					  "pwr_irq";
>>>>> +
>>>>> +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
>>>>> +				 <&gcc GCC_SDCC1_APPS_CLK>,
>>>>> +				 <&rpmhcc RPMH_CXO_CLK>,
>>>>> +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
>>>>> +			clock-names = "iface",
>>>>> +				      "core",
>>>>> +				      "xo",
>>>>> +				      "ice";
>>>>> +
>>>>> +			resets = <&gcc GCC_SDCC1_BCR>;
>>>>> +
>>>>> +			power-domains = <&rpmhpd RPMHPD_CX>;
>>>>> +			operating-points-v2 = <&sdhc1_opp_table>;
>>>>> +			iommus = <&apps_smmu 0x02c0 0x0>;
>>>>> +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
>>>>> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>>>>> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
>>>>> +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
>>>>> +			interconnect-names = "sdhc-ddr",
>>>>> +					     "cpu-sdhc";
>>>>> +
>>>>> +			bus-width = <8>;
>>>>> +			qcom,dll-config = <0x000f642c>;
>>>>> +			qcom,ddr-config = <0x80040868>;
>>>>> +			supports-cqe;
>>>>> +			dma-coherent;
>>>>> +			mmc-ddr-1_8v;
>>>>> +			mmc-hs200-1_8v;
>>>>> +			mmc-hs400-1_8v;
>>>>> +			mmc-hs400-enhanced-strobe;
>>>>
>>>> These are properties of memory, not SoC. If the node is disabled, means
>>>> memory is not attached to the SoC, right?
>>>>
>>>>> +			status = "disabled";
>>> Thanks, I think qcom,dll-config and qcom,ddr-config are properties of Soc,
>>> they are memory configurations that need to be written to the ioaddr.
>>> And mmc-ddr-1_8v,mmc-hs200-1_8v,mmc-hs400-1_8v are bus speed config,
>>> they indicate the bus speed at which the host contoller can operate.
>>> If the node is disabled, which means Soc don't support these properties.
>> No, that is not the meaning of node is disabled. When node is disabled,
>> it means board does not have attached memory.
>>
>> Move the memory related properties  to the board.
> 
> Thanks, Ok I understand, I will move the memory related
> properties(qcom,dll-config and qcom,ddr-config) to the
> board dts in next version.

What? Why are you talking about these properties? My comment was not
under these!

Best regards,
Krzysztof

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Yuanjie Yang]

On Mon, Nov 25, 2024 at 09:16:02AM +0100, Krzysztof Kozlowski wrote:
> On 25/11/2024 09:06, Yuanjie Yang wrote:
> >>>>>>> +
> >>>>>>> +			resets = <&gcc GCC_SDCC1_BCR>;
> >>>>>>> +
> >>>>>>> +			power-domains = <&rpmhpd RPMHPD_CX>;
> >>>>>>> +			operating-points-v2 = <&sdhc1_opp_table>;
> >>>>>>> +			iommus = <&apps_smmu 0x02c0 0x0>;
> >>>>>>> +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
> >>>>>>> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> >>>>>>> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> >>>>>>> +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
> >>>>>>> +			interconnect-names = "sdhc-ddr",
> >>>>>>> +					     "cpu-sdhc";
> >>>>>>> +
> >>>>>>> +			bus-width = <8>;
> >>>>>>> +			qcom,dll-config = <0x000f642c>;
> >>>>>>> +			qcom,ddr-config = <0x80040868>;
> >>>>>>> +			supports-cqe;
> >>>>>>> +			dma-coherent;
> >>>>>>> +			mmc-ddr-1_8v;
> >>>>>>> +			mmc-hs200-1_8v;
> >>>>>>> +			mmc-hs400-1_8v;
> >>>>>>> +			mmc-hs400-enhanced-strobe;
> >>>>>>
> >>>>>> These are properties of memory, not SoC. If the node is disabled, means
> >>>>>> memory is not attached to the SoC, right?
> >>>>>>
> >>>>>>> +			status = "disabled";
> >>>>> Thanks, I think qcom,dll-config and qcom,ddr-config are properties of Soc,
> >>>>> they are memory configurations that need to be written to the ioaddr.
> >>>>> And mmc-ddr-1_8v,mmc-hs200-1_8v,mmc-hs400-1_8v are bus speed config,
> >>>>> they indicate the bus speed at which the host contoller can operate.
> >>>>> If the node is disabled, which means Soc don't support these properties.
> >>>> No, that is not the meaning of node is disabled. When node is disabled,
> >>>> it means board does not have attached memory.
> >>>>
> >>>> Move the memory related properties  to the board.
> >>>
> >>> Thanks, Ok I understand, I will move the memory related
> >>> properties(qcom,dll-config and qcom,ddr-config) to the
> >>> board dts in next version.
> >>
> >> What? Why are you talking about these properties? My comment was not
> >> under these!
> > Thanks, Sorry, I may have misunderstood your meaning.
> > Do you mean I need move memory realted properties(bus-width, dma-coherent)
> > to the board dts?
> > When this node's status is okay, then board can set these memory config.
> > I will fix it in next version.
> 
> Keep all discussions public. Where was my comment? Under dma-coherent?
> No. Each comment is in very specific place. I asked about memory
> specific properties.
> 
> I also rephrased it differently already, but maybe not clear enough: you
> cannot have here properties which are not properties of the SoC.
> 
> I am not going to discuss it more in private. Read the netiquette.
> 
> https://people.kernel.org/tglx/notes-about-netiquette

Thanks, Sorry, I accidentally sent the email just now; I didn't mean to send
it privately.

Ok, I agree with your idea. properties which are not of Soc should move
to board dts.

I double check my dts, dtsi. I think I should move properties(bus-width,
mmc-ddr-1_8v, mmc-hs200-1_8v, mmc-hs400-1_8v, mmc-hs400-enhanced-strobe)
to board dts, these properties are just to config Soc. Do you agree my
option?

Thanks again for your time to point out my mistake.

> Best regards,
> Krzysztof

Thanks,
Yuanjie

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Konrad Dybcio]

On 25.11.2024 3:20 AM, Yuanjie Yang wrote:
> On Fri, Nov 22, 2024 at 01:35:28PM +0100, Krzysztof Kozlowski wrote:
>> On 22/11/2024 09:40, Yuanjie Yang wrote:
>>> On Fri, Nov 22, 2024 at 08:04:31AM +0100, Krzysztof Kozlowski wrote:
>>>> On 22/11/2024 07:51, Yuanjie Yang wrote:
>>>>> Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
>>>>>
>>>>> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
>>>>> ---

[...]

>>>>> +			bus-width = <8>;
>>>>> +			qcom,dll-config = <0x000f642c>;
>>>>> +			qcom,ddr-config = <0x80040868>;
>>>>> +			supports-cqe;
>>>>> +			dma-coherent;
>>>>> +			mmc-ddr-1_8v;
>>>>> +			mmc-hs200-1_8v;
>>>>> +			mmc-hs400-1_8v;
>>>>> +			mmc-hs400-enhanced-strobe;
>>>>
>>>> These are properties of memory, not SoC. If the node is disabled, means
>>>> memory is not attached to the SoC, right?
>>>>
>>>>> +			status = "disabled";
>>> Thanks, I think qcom,dll-config and qcom,ddr-config are properties of Soc,
>>> they are memory configurations that need to be written to the ioaddr.
>>> And mmc-ddr-1_8v,mmc-hs200-1_8v,mmc-hs400-1_8v are bus speed config,
>>> they indicate the bus speed at which the host contoller can operate.
>>> If the node is disabled, which means Soc don't support these properties.
>> No, that is not the meaning of node is disabled. When node is disabled,
>> it means board does not have attached memory.
>>
>> Move the memory related properties  to the board.
> 
> Thanks, Ok I understand, I will move the memory related
> properties(qcom,dll-config and qcom,ddr-config) to the
> board dts in next version.

DDR/DLL tuning seem to be done per SoC and not per board.

Konrad

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Konrad Dybcio]

On 22.11.2024 7:51 AM, Yuanjie Yang wrote:
> Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
> 
> Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
>  1 file changed, 198 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> index 590beb37f441..37c6ab217c96 100644
> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
>  			#size-cells = <1>;
>  		};
>  
> +		sdhc_1: mmc@7c4000 {
> +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
> +			reg = <0x0 0x007c4000 0x0 0x1000>,
> +			      <0x0 0x007c5000 0x0 0x1000>;
> +			reg-names = "hc",
> +				    "cqhci";

There's an "ice" region at 0x007c8000

> +
> +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "hc_irq",
> +					  "pwr_irq";
> +
> +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
> +				 <&gcc GCC_SDCC1_APPS_CLK>,
> +				 <&rpmhcc RPMH_CXO_CLK>,
> +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
> +			clock-names = "iface",
> +				      "core",
> +				      "xo",
> +				      "ice";
> +
> +			resets = <&gcc GCC_SDCC1_BCR>;
> +
> +			power-domains = <&rpmhpd RPMHPD_CX>;
> +			operating-points-v2 = <&sdhc1_opp_table>;
> +			iommus = <&apps_smmu 0x02c0 0x0>;
> +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "sdhc-ddr",
> +					     "cpu-sdhc";
> +
> +			bus-width = <8>;
> +			qcom,dll-config = <0x000f642c>;
> +			qcom,ddr-config = <0x80040868>;
> +			supports-cqe;
> +			dma-coherent;
> +			mmc-ddr-1_8v;
> +			mmc-hs200-1_8v;
> +			mmc-hs400-1_8v;
> +			mmc-hs400-enhanced-strobe;
> +			status = "disabled";
> +
> +			sdhc1_opp_table: opp-table {
> +				compatible = "operating-points-v2";
> +
> +				opp-100000000 {
> +					opp-hz = /bits/ 64 <100000000>;
> +					required-opps = <&rpmhpd_opp_svs>;
> +				};

I'm seeing 25/50 MHz OPPs in the docs as well

[...]

> +
> +		sdhc_2: mmc@8804000 {
> +			compatible = "qcom,qcs615-sdhci","qcom,sdhci-msm-v5";

Missing space 

> +			reg = <0x0 0x08804000 0x0 0x1000>;
> +			reg-names = "hc";
> +
> +			interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 222 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";
> +
> +			power-domains = <&rpmhpd RPMHPD_CX>;
> +			operating-points-v2 = <&sdhc2_opp_table>;
> +			iommus = <&apps_smmu 0x02a0 0x0>;
> +			resets = <&gcc GCC_SDCC2_BCR>;
> +			interconnects = <&aggre1_noc MASTER_SDCC_2 QCOM_ICC_TAG_ALWAYS
> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> +					 &config_noc SLAVE_SDCC_2 QCOM_ICC_TAG_ALWAYS>;
> +			interconnect-names = "sdhc-ddr",
> +					     "cpu-sdhc";
> +
> +			bus-width = <4>;
> +			qcom,dll-config = <0x0007642c>;
> +			qcom,ddr-config = <0x80040868>;
> +			dma-coherent;
> +			status = "disabled";
> +
> +			sdhc2_opp_table: opp-table {
> +				compatible = "operating-points-v2";
> +

Similarly, it can operate at 25/50 MHz too

Konrad

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Dmitry Baryshkov]

On Mon, Nov 25, 2024 at 02:13:22PM +0100, Konrad Dybcio wrote:
> On 22.11.2024 7:51 AM, Yuanjie Yang wrote:
> > Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
> > 
> > Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
> >  1 file changed, 198 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > index 590beb37f441..37c6ab217c96 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
> >  			#size-cells = <1>;
> >  		};
> >  
> > +		sdhc_1: mmc@7c4000 {
> > +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
> > +			reg = <0x0 0x007c4000 0x0 0x1000>,
> > +			      <0x0 0x007c5000 0x0 0x1000>;
> > +			reg-names = "hc",
> > +				    "cqhci";
> 
> There's an "ice" region at 0x007c8000

Shouldn't ice now be handled by a separate device?


-- 
With best wishes
Dmitry

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Yuanjie Yang]

On Mon, Nov 25, 2024 at 02:13:22PM +0100, Konrad Dybcio wrote:
> On 22.11.2024 7:51 AM, Yuanjie Yang wrote:
> > Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
> > 
> > Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> > ---
> >  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
> >  1 file changed, 198 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > index 590beb37f441..37c6ab217c96 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
> >  			#size-cells = <1>;
> >  		};
> >  
> > +		sdhc_1: mmc@7c4000 {
> > +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
> > +			reg = <0x0 0x007c4000 0x0 0x1000>,
> > +			      <0x0 0x007c5000 0x0 0x1000>;
> > +			reg-names = "hc",
> > +				    "cqhci";
> 
> There's an "ice" region at 0x007c8000
Thanks, I check doc again, I miss "ice" region at 0x007c8000.

> > +
> > +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
> > +			interrupt-names = "hc_irq",
> > +					  "pwr_irq";
> > +
> > +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
> > +				 <&gcc GCC_SDCC1_APPS_CLK>,
> > +				 <&rpmhcc RPMH_CXO_CLK>,
> > +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
> > +			clock-names = "iface",
> > +				      "core",
> > +				      "xo",
> > +				      "ice";
> > +
> > +			resets = <&gcc GCC_SDCC1_BCR>;
> > +
> > +			power-domains = <&rpmhpd RPMHPD_CX>;
> > +			operating-points-v2 = <&sdhc1_opp_table>;
> > +			iommus = <&apps_smmu 0x02c0 0x0>;
> > +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
> > +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> > +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> > +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
> > +			interconnect-names = "sdhc-ddr",
> > +					     "cpu-sdhc";
> > +
> > +			bus-width = <8>;
> > +			qcom,dll-config = <0x000f642c>;
> > +			qcom,ddr-config = <0x80040868>;
> > +			supports-cqe;
> > +			dma-coherent;
> > +			mmc-ddr-1_8v;
> > +			mmc-hs200-1_8v;
> > +			mmc-hs400-1_8v;
> > +			mmc-hs400-enhanced-strobe;
> > +			status = "disabled";
> > +
> > +			sdhc1_opp_table: opp-table {
> > +				compatible = "operating-points-v2";
> > +
> > +				opp-100000000 {
> > +					opp-hz = /bits/ 64 <100000000>;
> > +					required-opps = <&rpmhpd_opp_svs>;
> > +				};
> 
> I'm seeing 25/50 MHz OPPs in the docs as well
Thanks, I check doc again, I miss 50MHz OPPs, but I don't find 25MHz.

> [...]
> 
> > +
> > +		sdhc_2: mmc@8804000 {
> > +			compatible = "qcom,qcs615-sdhci","qcom,sdhci-msm-v5";
> 
> Missing space 
> 
> > +			reg = <0x0 0x08804000 0x0 0x1000>;
> > +			reg-names = "hc";
> > +
> > +			interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
> > +				     <GIC_SPI 222 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";
> > +
> > +			power-domains = <&rpmhpd RPMHPD_CX>;
> > +			operating-points-v2 = <&sdhc2_opp_table>;
> > +			iommus = <&apps_smmu 0x02a0 0x0>;
> > +			resets = <&gcc GCC_SDCC2_BCR>;
> > +			interconnects = <&aggre1_noc MASTER_SDCC_2 QCOM_ICC_TAG_ALWAYS
> > +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> > +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> > +					 &config_noc SLAVE_SDCC_2 QCOM_ICC_TAG_ALWAYS>;
> > +			interconnect-names = "sdhc-ddr",
> > +					     "cpu-sdhc";
> > +
> > +			bus-width = <4>;
> > +			qcom,dll-config = <0x0007642c>;
> > +			qcom,ddr-config = <0x80040868>;
> > +			dma-coherent;
> > +			status = "disabled";
> > +
> > +			sdhc2_opp_table: opp-table {
> > +				compatible = "operating-points-v2";
> > +
> 
> Similarly, it can operate at 25/50 MHz too
Thanks, I check doc again, I miss 50MHz OPPs, but I don't find 25MHz.

> 
> Konrad

Thanks,
Yuanjie

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Yuanjie Yang]

On Tue, Nov 26, 2024 at 05:07:11PM +0800, Yuanjie Yang wrote:
> On Mon, Nov 25, 2024 at 02:13:22PM +0100, Konrad Dybcio wrote:
> > On 22.11.2024 7:51 AM, Yuanjie Yang wrote:
> > > Add SDHC1 and SDHC2 support to the QCS615 Ride platform.
> > > 
> > > Signed-off-by: Yuanjie Yang <quic_yuanjiey@quicinc.com>
> > > ---
> > >  arch/arm64/boot/dts/qcom/qcs615.dtsi | 198 +++++++++++++++++++++++++++
> > >  1 file changed, 198 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > > index 590beb37f441..37c6ab217c96 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
> > > @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
> > >  			#size-cells = <1>;
> > >  		};
> > >  
> > > +		sdhc_1: mmc@7c4000 {
> > > +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
> > > +			reg = <0x0 0x007c4000 0x0 0x1000>,
> > > +			      <0x0 0x007c5000 0x0 0x1000>;
> > > +			reg-names = "hc",
> > > +				    "cqhci";
> > 
> > There's an "ice" region at 0x007c8000
> Thanks, I check doc again, I miss "ice" region at 0x007c8000.
> 
> > > +
> > > +			interrupts = <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>,
> > > +				     <GIC_SPI 644 IRQ_TYPE_LEVEL_HIGH>;
> > > +			interrupt-names = "hc_irq",
> > > +					  "pwr_irq";
> > > +
> > > +			clocks = <&gcc GCC_SDCC1_AHB_CLK>,
> > > +				 <&gcc GCC_SDCC1_APPS_CLK>,
> > > +				 <&rpmhcc RPMH_CXO_CLK>,
> > > +				 <&gcc GCC_SDCC1_ICE_CORE_CLK>;
> > > +			clock-names = "iface",
> > > +				      "core",
> > > +				      "xo",
> > > +				      "ice";
> > > +
> > > +			resets = <&gcc GCC_SDCC1_BCR>;
> > > +
> > > +			power-domains = <&rpmhpd RPMHPD_CX>;
> > > +			operating-points-v2 = <&sdhc1_opp_table>;
> > > +			iommus = <&apps_smmu 0x02c0 0x0>;
> > > +			interconnects = <&aggre1_noc MASTER_SDCC_1 QCOM_ICC_TAG_ALWAYS
> > > +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> > > +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> > > +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ALWAYS>;
> > > +			interconnect-names = "sdhc-ddr",
> > > +					     "cpu-sdhc";
> > > +
> > > +			bus-width = <8>;
> > > +			qcom,dll-config = <0x000f642c>;
> > > +			qcom,ddr-config = <0x80040868>;
> > > +			supports-cqe;
> > > +			dma-coherent;
> > > +			mmc-ddr-1_8v;
> > > +			mmc-hs200-1_8v;
> > > +			mmc-hs400-1_8v;
> > > +			mmc-hs400-enhanced-strobe;
> > > +			status = "disabled";
> > > +
> > > +			sdhc1_opp_table: opp-table {
> > > +				compatible = "operating-points-v2";
> > > +
> > > +				opp-100000000 {
> > > +					opp-hz = /bits/ 64 <100000000>;
> > > +					required-opps = <&rpmhpd_opp_svs>;
> > > +				};
> > 
> > I'm seeing 25/50 MHz OPPs in the docs as well
> Thanks, I check doc again, I miss 50MHz OPPs, but I don't find 25MHz.
> 
> > [...]
> > 
> > > +
> > > +		sdhc_2: mmc@8804000 {
> > > +			compatible = "qcom,qcs615-sdhci","qcom,sdhci-msm-v5";
> > 
> > Missing space 
Thanks, I will add space in next version.

> > > +			reg = <0x0 0x08804000 0x0 0x1000>;
> > > +			reg-names = "hc";
> > > +
> > > +			interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>,
> > > +				     <GIC_SPI 222 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";
> > > +
> > > +			power-domains = <&rpmhpd RPMHPD_CX>;
> > > +			operating-points-v2 = <&sdhc2_opp_table>;
> > > +			iommus = <&apps_smmu 0x02a0 0x0>;
> > > +			resets = <&gcc GCC_SDCC2_BCR>;
> > > +			interconnects = <&aggre1_noc MASTER_SDCC_2 QCOM_ICC_TAG_ALWAYS
> > > +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
> > > +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ALWAYS
> > > +					 &config_noc SLAVE_SDCC_2 QCOM_ICC_TAG_ALWAYS>;
> > > +			interconnect-names = "sdhc-ddr",
> > > +					     "cpu-sdhc";
> > > +
> > > +			bus-width = <4>;
> > > +			qcom,dll-config = <0x0007642c>;
> > > +			qcom,ddr-config = <0x80040868>;
> > > +			dma-coherent;
> > > +			status = "disabled";
> > > +
> > > +			sdhc2_opp_table: opp-table {
> > > +				compatible = "operating-points-v2";
> > > +
> > 
> > Similarly, it can operate at 25/50 MHz too
> Thanks, I check doc again, I miss 50MHz OPPs, but I don't find 25MHz.
> 
> > 
> > Konrad
> 
> Thanks,
> Yuanjie
> 
Thanks,
Yuanjie

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Krzysztof Kozlowski]

On 26/11/2024 01:07, Dmitry Baryshkov wrote:
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>> index 590beb37f441..37c6ab217c96 100644
>>> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>> @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
>>>  			#size-cells = <1>;
>>>  		};
>>>  
>>> +		sdhc_1: mmc@7c4000 {
>>> +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
>>> +			reg = <0x0 0x007c4000 0x0 0x1000>,
>>> +			      <0x0 0x007c5000 0x0 0x1000>;
>>> +			reg-names = "hc",
>>> +				    "cqhci";
>>
>> There's an "ice" region at 0x007c8000
> 
> Shouldn't ice now be handled by a separate device?
It should and UFS bindings expect that. However I am not sure if MMC was
improved to support external ICE device.  Also for example on SM8550 the
ICE has entirely different (further) address space, so it also suggests
it is separate device. Here address space looks almost continuous.


Best regards,
Krzysztof

Re: [PATCH v3 1/2] arm64: dts: qcom: qcs615: add SDHC1 and SDHC2

[Konrad Dybcio]

On 26.11.2024 10:26 AM, Krzysztof Kozlowski wrote:
> On 26/11/2024 01:07, Dmitry Baryshkov wrote:
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/qcs615.dtsi b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>> index 590beb37f441..37c6ab217c96 100644
>>>> --- a/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/qcs615.dtsi
>>>> @@ -399,6 +399,65 @@ qfprom: efuse@780000 {
>>>>  			#size-cells = <1>;
>>>>  		};
>>>>  
>>>> +		sdhc_1: mmc@7c4000 {
>>>> +			compatible = "qcom,qcs615-sdhci", "qcom,sdhci-msm-v5";
>>>> +			reg = <0x0 0x007c4000 0x0 0x1000>,
>>>> +			      <0x0 0x007c5000 0x0 0x1000>;
>>>> +			reg-names = "hc",
>>>> +				    "cqhci";
>>>
>>> There's an "ice" region at 0x007c8000
>>
>> Shouldn't ice now be handled by a separate device?
> It should and UFS bindings expect that. However I am not sure if MMC was
> improved to support external ICE device.  Also for example on SM8550 the
> ICE has entirely different (further) address space, so it also suggests
> it is separate device. Here address space looks almost continuous.

Some SoCs have two ICEs (one for UFS and one for SDHCI) - seems to be
mainly the case on platforms where there's "sdhc1" (intended for eMMC)
*and* a UFS host.

The commit message that introduced a separate driver says:

"""
The reason for this is because, staring with SM8550, the ICE IP block
is shared between UFS and SDCC, which means we need to probe a dedicated
device and share it between those two consumers.
"""

but:

* in sm8550.dtsi, only UFS has a qcom,ice reference (like other device
   trees using that binding)
* I can't find anything that would back this internally

I'm not sure how this is supposed to work, especially on SoCs with two
instances

Konrad