Re: [PATCH 00/16] Fix Qcom UFS PHY clocks

[Johan Hovold]

[ +CC: Shazad ]

On Thu, Dec 14, 2023 at 04:09:07PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Dec 14, 2023 at 11:15:34AM +0100, Johan Hovold wrote:
> > On Thu, Dec 14, 2023 at 02:40:45PM +0530, Manivannan Sadhasivam wrote:
> > 
> > > This series fixes the clocks supplied to QMP PHY IPs in the Qcom SoCs. All
> > > of the Qcom SoCs except MSM8996 require 3 clocks for QMP UFS:
> > > 
> > > * ref - 19.2MHz reference clock from RPM/RPMh
> > > * ref_aux - Auxiliary reference clock from GCC
> > > * qref - QREF clock from GCC or TCSR (TCSR since SM8550)
> > > 
> > > MSM8996 only requires 'ref' and 'qref' clocks.
> > > 
> > > Hence, this series fixes the binding, DT and GCC driver to reflect the
> > > actual clock topology.
> > 
> > Is this based on documentation for all the SoCs or on inference from the
> > current (upstream and downstream) devicetrees?
> 
> It is based on the internal documentation. Even downstream devicetrees are
> wrong. I should've mentioned it in the cover letter.
> 
> > Are you sure that you should not just describe that some of these UFS
> > reference clocks are sourced from CXO in the clock driver instead?
> 
> I don't get your comment fully. Could you please elaborate?

Unless the PHY consumes CXO directly, it should not be included in the
binding as you are suggesting here.

We discussed this at some length at the time with Bjorn and Shazad who
had access to the documentation and the conclusion was that, at least on
sc8280xp, the PHY does not use CXO directly and instead it should be
described as a parent to the UFS refclocks in the clock driver:

	https://lore.kernel.org/lkml/Y2OEjNAPXg5BfOxH@xxxxxxxxxxxxxxxxxxxx/

The downstream devicetrees have a bad habit of including parent clocks
directly in the consumer node instead of modelling this in clock driver
also for other peripherals.
 
> > Take a look at commits
> > 
> > 	f446022b932a ("arm64: dts: qcom: sc8280xp: fix UFS reference clocks")
> > 	f6abcc21d943 ("clk: qcom: gcc-sc8280xp: add cxo as parent for three ufs ref clks")
> 
> Btw, these commits are not accurate. In all the SoCs before SM8550, reference
> clock for the UFS device comes from the UFS controller. There is a dedicated
> register in UFSHC memory map that is being toggled by the driver to
> enable/disable reference clock for the UFS device.
>
> Since SM8550, reference clock is directly sourced from RPMh. I'm preparing a
> series to fix it.

What exactly is wrong with those commits? We know that the controller
does not consume GCC_UFS_REF_CLKREF_CLK directly, but describing that as
such for now was a deliberate choice:

	GCC_UFS_REF_CLKREF_CLK is the clock to the devices, but as we
	don't represent the memory device explicitly it seems suitable
	to use as "ref_clk" in the ufshc nodes - which would then match
	the special handling of the "link clock" in the UFS driver.
 
> Unfortunately, this information is not depicted correctly in the downstream
> devicetrees.

I was hoping the information that those commits are based on would be
correct as it came from Qualcomm and Bjorn. I have no illusions about
the downstream devicetrees being correct. :)

Johan