Re: [PATCH V2 1/2] arm64: dts: qcom: Add eMMC support for qcs8300

[Sayali Lokhande <quic_sayalil@quicinc.com>]

On 6/23/2025 7:39 PM, Konrad Dybcio wrote:
> On 6/19/25 9:02 AM, Sayali Lokhande wrote:
>> Add eMMC support for qcs8300 board.
>>
>> Signed-off-by: Sayali Lokhande <quic_sayalil@quicinc.com>
>> ---
> It's customary to split board and SoC changes into separate commits
>
> [...]
>
>> +		sdhc_1: mmc@87C4000 {
> Please use lowercase hex across DT, in all places
>
>> +			compatible = "qcom,qcs8300-sdhci", "qcom,sdhci-msm-v5";
>> +			reg = <0x0 0x87C4000 0x0 0x1000>,
> To make it easier to compare by eye, we tend to pad the address
> values to 8 hex digits with leading zeroes, please do so as well


Sure will do


>> +			      <0x0 0x87C5000 0x0 0x1000>;
>> +			reg-names = "hc",
>> +				    "cqhci";
>> +
>> +			interrupts = <GIC_SPI 383 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 521 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>;
>> +			clock-names = "iface",
>> +				      "core",
>> +				      "xo";
>> +
>> +			resets = <&gcc GCC_SDCC1_BCR>;
>> +
>> +			power-domains = <&rpmhpd RPMHPD_CX>;
>> +			operating-points-v2 = <&sdhc1_opp_table>;
>> +			iommus = <&apps_smmu 0x0 0x0>;
> In case anyone's wondering, this is actually the correct value
>
>> +			interconnects = <&aggre1_noc MASTER_SDC QCOM_ICC_TAG_ALWAYS
>> +					 &mc_virt SLAVE_EBI1 QCOM_ICC_TAG_ALWAYS>,
>> +					<&gem_noc MASTER_APPSS_PROC QCOM_ICC_TAG_ACTIVE_ONLY
>> +					 &config_noc SLAVE_SDCC_1 QCOM_ICC_TAG_ACTIVE_ONLY>;
>> +			interconnect-names = "sdhc-ddr",
>> +					     "cpu-sdhc";
> [...]
>
>> +			sdc1_state_on: sdc1-on-state {
>> +				clk-pins {
>> +					pins = "sdc1_clk";
>> +					bias-disable;
>> +					drive-strength = <16>;
> Please move bias properties under drive-strength for consistency


Will do


>
>> +				};
>> +
>> +				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-bus-hold;
> Is bus-hold what we want here? Other platforms do a pull-up/down
> or disable bias altogether


Yes we want bias-bus-hold, same as other qc platforms for example like 
sc7280.dtsi


>
> Konrad