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

[Johan Hovold <johan@kernel.org>]

On Thu, Dec 14, 2023 at 04:44:09PM +0530, Manivannan Sadhasivam wrote:
> + Can
> 
> On Thu, Dec 14, 2023 at 12:00:40PM +0100, Johan Hovold wrote:
> > [ +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:

> > Unless the PHY consumes CXO directly, it should not be included in the
> > binding as you are suggesting here.
> 
> PHY is indeed directly consuming CXO. That's why I included it in the binding.

Ok, good. It's a bit frustrating that people can even seem to agree on
answers to direct questions about that.
 
> > 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@hovoldconsulting.com/
> > 
> > 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.
> >  
> 
> No, I can assure that you got the wrong info. UFS PHY consumes the clock
> directly from RPMh. It took me several days to dig through the UFS and PHY docs
> and special thanks to Can Guo from UFS team, who provided much valuable
> information about these clocks.

Sounds like you've done your research.

> > 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.
> >  
> 
> No, GCC_UFS_REF_CLKREF_CLK is _not_ the clock to UFS devices. I haven't found
> information about this specific register in GCC. Initially I thought this is for
> enabling QREF clocks for the UFS MEM phy, but I haven't found the answer yet.

Just quoting Bjorn.

> But as I said earlier, reference clock to UFS devices comes directly from the
> controller and there is a specfic register for controlling that. Starting from
> SM8550, reference clock comes from RPMh.

Sure, but that was only part of what those commits did or claimed. Bjorn
also explicitly stated that those refclocks were sourced from CXO, even
though I now see a claim from Shazad in that thread claiming the
opposite:

	https://lore.kernel.org/all/Y2Imnf1+v5j5CH9r@hovoldconsulting.com/

Without access to docs I can only ask questions and try to do tedious
inferences from incomplete open sources (e.g. downstream devicetrees).

Johan