Re: [PATCH 1/3] clk: qcom: smd-rpm: Add rate hooks for clk_smd_rpm_branch_ops

[Shawn Guo]

On Wed, Sep 15, 2021 at 10:23:11AM -0700, Bjorn Andersson wrote:
> On Wed 15 Sep 08:05 PDT 2021, Shawn Guo wrote:
> 
> > On Tue, Sep 14, 2021 at 02:56:28PM -0700, Stephen Boyd wrote:
> > > Quoting Shawn Guo (2021-09-13 19:55:52)
> > > > On QCM2290 platform, the clock xo_board runs at 38400000, while the
> > > > child clock bi_tcxo needs to run at 19200000.  That said,
> > > > clk_smd_rpm_branch_ops needs the capability of setting rate. Add rate
> > > > hooks into clk_smd_rpm_branch_ops to make it possible.
> > > 
> > > This doesn't sound right. The branch is a simple on/off. If xo_board is
> > > 38.4MHz, then there is an internal divider in the SoC that makes bi_tcxo
> > > (i.e. the root of the entire clk tree) be 19.2MHz. We don't model the
> > > divider, I guess because it isn't very important to. Instead, we tack on
> > > a divider field and implement recalc_rate op. See clk-rpmh.c in the qcom
> > > directory for this.
> > 
> > Thanks for the comment, Stephen!  To be honest, I copied the
> > implementation from vendor kernel, and wasn't really sure if it's
> > correct or the best.
> > 
> > So here is what I get based on your suggestion.  Let's me know if
> > it's how you wanted it to be.  Thanks!
> > 
> > Shawn
> > 
> > ----8<---------
> > 
> > From 23dda79fee412738f046b89bdd20ef95a24c35cc Mon Sep 17 00:00:00 2001
> > From: Shawn Guo <shawn.guo@xxxxxxxxxx>
> > Date: Wed, 15 Sep 2021 22:00:32 +0800
> > Subject: [PATCH] clk: qcom: smd-rpm: Add a divider field for branch clock
> > 
> > Similar to clk-rpmh, clk-smd-rpm has the same need to handle the case
> > where an internal divider is there between xo_board and bi_tcxo.  The
> > change is made in the a back compatible way below.
> > 
> >  - Add div field to struct clk_smd_rpm, and have
> >    __DEFINE_CLK_SMD_RPM_BRANCH() assign it.
> > 
> >  - Update all existing __DEFINE_CLK_SMD_RPM_BRANCH() wrappers to pass a
> >    zero div.
> > 
> >  - Add DEFINE_CLK_SMD_RPM_BRANCH_DIV() which doesn't take rate argument
> >    but div.
> > 
> >  - Update clk_smd_rpm_recalc_rate() to handle div and add it as
> >    .recalc_rate of clk_smd_rpm_branch_ops.
> > 
> 
> This looks good to me.
> 
> And the confirmed that the xo_board in sdm630.dtsi (and hence SDM660) is
> wrong, it should be 38.4MHz as well.

Hmm, I see CAF kernel has 19.2MHz for SDM630/660 xo_board clock.  Or am
I looking at the wrong place?

Shawn

> Unfortunately adding the appropriate divider to the sdm660 bcxo would
> break existing .dtsi (but we can probably convince the community that it
> would be ok, if we do it now).