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@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. > > > > > > > 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@xxxxxxxxxxxxxxxxxxxx/ 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 -- மணிவண்ணன் சதாசிவம்
