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

[Yuanjie Yang <quic_yuanjiey@quicinc.com>]

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