Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
Date:   Thu, 14 Dec 2023 17:19:59 +0530
From:   Manivannan Sadhasivam <mani@kernel.org>
To:     Johan Hovold <johan@kernel.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        andersson@kernel.org, konrad.dybcio@linaro.org, vkoul@kernel.org,
        sboyd@kernel.org, mturquette@baylibre.com, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
        devicetree@vger.kernel.org,
        Shazad Hussain <quic_shazhuss@quicinc.com>,
        quic_cang@quicinc.com
Subject: Re: [PATCH 00/16] Fix Qcom UFS PHY clocks
Message-ID: <20231214114959.GC48078@thinkpad>
References: <20231214091101.45713-1-manivannan.sadhasivam@linaro.org>
 <ZXrVxmxY6wZprbBa@hovoldconsulting.com>
 <20231214103907.GL2938@thinkpad>
 <ZXrgWK5wZz6dAkKP@hovoldconsulting.com>
 <20231214111409.GB48078@thinkpad>
 <ZXrnZeDYOsteY5zT@hovoldconsulting.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZXrnZeDYOsteY5zT@hovoldconsulting.com>
Precedence: bulk

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

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