Message-ID: <nr4xt5pefd3jngml6bkbgrfhsuxmre44v3qs6uyxz7qp5dzqad@6dss6lwhb35n>
Date: Fri, 15 Nov 2024 09:03:28 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Tingwei Zhang <quic_tingweiz@...cinc.com>
Cc: Konrad Dybcio <konrad.dybcio@....qualcomm.com>, 
	Ziyue Zhang <quic_ziyuzhan@...cinc.com>, vkoul@...nel.org, kishon@...nel.org, robh+dt@...nel.org, 
	manivannan.sadhasivam@...aro.org, bhelgaas@...gle.com, kw@...ux.com, lpieralisi@...nel.org, 
	quic_qianyu@...cinc.com, conor+dt@...nel.org, neil.armstrong@...aro.org, 
	andersson@...nel.org, konradybcio@...nel.org, quic_shashim@...cinc.com, 
	quic_kaushalk@...cinc.com, quic_tdas@...cinc.com, quic_aiquny@...cinc.com, 
	kernel@...cinc.com, linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-phy@...ts.infradead.org
Subject: Re: [PATCH 4/5] arm64: dts: qcom: qcs8300: enable pcie0 for QCS8300

On Fri, Nov 15, 2024 at 02:42:47PM +0800, Tingwei Zhang wrote:
> On 11/15/2024 2:26 PM, Dmitry Baryshkov wrote:
> > On Fri, Nov 15, 2024 at 12:59:12PM +0800, Tingwei Zhang wrote:
> > > On 11/14/2024 9:03 PM, Konrad Dybcio wrote:
> > > > On 14.11.2024 1:10 PM, Dmitry Baryshkov wrote:
> > > > > On Thu, Nov 14, 2024 at 05:54:08PM +0800, Ziyue Zhang wrote:
> > > > > > Add configurations in devicetree for PCIe0, including registers, clocks,
> > > > > > interrupts and phy setting sequence.
> > > > > > 
> > > > > > Signed-off-by: Ziyue Zhang <quic_ziyuzhan@...cinc.com>
> > > > > > ---
> > > > > >    arch/arm64/boot/dts/qcom/qcs8300-ride.dts |  44 +++++-
> > > > > >    arch/arm64/boot/dts/qcom/qcs8300.dtsi     | 176 ++++++++++++++++++++++
> > > > > >    2 files changed, 219 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs8300-ride.dts b/arch/arm64/boot/dts/qcom/qcs8300-ride.dts
> > > > > > index 7eed19a694c3..9d7c8555ed38 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/qcs8300-ride.dts
> > > > > > +++ b/arch/arm64/boot/dts/qcom/qcs8300-ride.dts
> > > > > > @@ -213,7 +213,7 @@ vreg_l9c: ldo9 {
> > > > > >    &gcc {
> > > > > 
> > > > > The patch doesn't seem to update the gcc node in qcs8300.dtsi. Is there
> > > > > any reason to have the clocks property in the board data file?
> > > > 
> > > > Definitely not. Ziyue, please move that change to the soc dtsi
> > > 
> > > Gcc node is updated in board device tree due to sleep_clk is defined in
> > > board device tree. Sleep_clk is from PMIC instead SoC so we were requested
> > > to move sleep_clk to board device tree in previous review [1].
> > 
> > Note, the review doesn't talk about sleep_clk at all. The recent
> > examples (sm8650, x1e80100, sa8775p) still pull the clocks into the SoC
> > dtsi, but without the freq.
> > 
> It's begining of the discussion of the PMIC clock for SoC. Sleep clock
> specific discussion is here [2].
> [2]https://lore.kernel.org/all/be8b573c-db4e-4eec-a9a6-3cd83d04156d@kernel.org/

Please note how the recent platforms describe those clocks: the node in
the SoC dtsi, the frequency in the board dtsi. X1E80100 is a step
backwards, the clock are completely defined in the x1e80100.dtsi. There
seems to be no strict rule on how to handle board clocks. I've sent an
RFC patchset, trying to move them to a single logical location. Let's
see what kind of response it will get. We probably need to define and
follow a common rule for all Qualcomm platforms. Please give it a couple
of days for the dust to settle. However, I think there should be no
reason to keep GCC's clock definitions in the board DTS.

> > > 
> > > [1]https://lore.kernel.org/all/10914199-1e86-4a2e-aec8-2a48cc49ef14@kernel.org/
> > > > 
> > > > Konrad
> > > 
> > > 
> > > -- 
> > > Thanks,
> > > Tingwei
> > > 
> > > -- 
> > > linux-phy mailing list
> > > linux-phy@...ts.infradead.org
> > > https://lists.infradead.org/mailman/listinfo/linux-phy
> > 
> 
> 
> -- 
> Thanks,
> Tingwei

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists