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

[Manivannan Sadhasivam <mani@kernel.org>]

On Thu, Dec 14, 2023 at 12:30:45PM +0100, Johan Hovold wrote:
> 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.
>  

I can understand 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/

To clarify further, what Shazad said about GCC_UFS_REF_CLKREF_CLK is correct.
This clock is not directly sourced by CXO, so it should be voted by the
_PHY_ driver separately along with CXO (which still feeds PHY). That's what I
represented in the binding.

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

That's the life for most of us :) Even with access to internal docs, it is
difficult to find the information we are looking for. Because, a very few people
know where the information is buried.

- Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்