| Message ID | 20231214091101.45713-1-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix Qcom UFS PHY clocks | expand |
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? 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? 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") Johan
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? > 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. Unfortunately, this information is not depicted correctly in the downstream devicetrees. - Mani > Johan >
[ +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@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. > > 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
+ 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: > > > > > > > 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. > PHY is indeed directly consuming CXO. That's why I included it in the binding. > 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. > > > 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. > 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. 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. > > 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. :) > Unfortunately, you got inaccurate information. But that's very common, since these kind of info are buried down in some docs and people's mind :) - Mani > Johan
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
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