[PATCH 0/3] Add QCM2290 RPM clocks support

[PATCH 0/3] Add QCM2290 RPM clocks support

[Shawn Guo]

The series adds RPM clocks support for QCM2290.

Shawn Guo (3):
  clk: qcom: smd-rpm: Add rate hooks for clk_smd_rpm_branch_ops
  dt-bindings: clk: qcom,rpmcc: Document QCM2290 compatible
  clk: qcom: smd-rpm: Add QCM2290 RPM clock support

 .../devicetree/bindings/clock/qcom,rpmcc.txt  |  1 +
 drivers/clk/qcom/clk-smd-rpm.c                | 62 +++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmcc.h        |  6 ++
 include/linux/soc/qcom/smd-rpm.h              |  2 +
 4 files changed, 71 insertions(+)

-- 
2.17.1

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

[Shawn Guo]

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.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 66d7807ee38e..2380e45b6247 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -416,6 +416,9 @@ static const struct clk_ops clk_smd_rpm_ops = {
 static const struct clk_ops clk_smd_rpm_branch_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
+	.set_rate	= clk_smd_rpm_set_rate,
+	.round_rate	= clk_smd_rpm_round_rate,
+	.recalc_rate	= clk_smd_rpm_recalc_rate,
 };
 
 DEFINE_CLK_SMD_RPM(msm8916, pcnoc_clk, pcnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
-- 
2.17.1

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

[Stephen Boyd]

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.

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

[Shawn Guo]

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@linaro.org>
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.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 66d7807ee38e..66ef0d3795fd 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -66,13 +66,14 @@
 	}
 
 #define __DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type, r_id,    \
-				    stat_id, r, key)			      \
+				    stat_id, r, key, _div)		      \
 	static struct clk_smd_rpm _platform##_##_active;		      \
 	static struct clk_smd_rpm _platform##_##_name = {		      \
 		.rpm_res_type = (type),					      \
 		.rpm_clk_id = (r_id),					      \
 		.rpm_status_id = (stat_id),				      \
 		.rpm_key = (key),					      \
+		.div = (_div),						      \
 		.branch = true,						      \
 		.peer = &_platform##_##_active,				      \
 		.rate = (r),						      \
@@ -92,6 +93,7 @@
 		.rpm_status_id = (stat_id),				      \
 		.active_only = true,					      \
 		.rpm_key = (key),					      \
+		.div = (_div),						      \
 		.branch = true,						      \
 		.peer = &_platform##_##_name,				      \
 		.rate = (r),						      \
@@ -112,7 +114,12 @@
 
 #define DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type, r_id, r)   \
 		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type,  \
-		r_id, 0, r, QCOM_RPM_SMD_KEY_ENABLE)
+		r_id, 0, r, QCOM_RPM_SMD_KEY_ENABLE, 0)
+
+#define DEFINE_CLK_SMD_RPM_BRANCH_DIV(_platform, _name, _active, type, r_id,  \
+				      _div)				      \
+		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type,  \
+		r_id, 0, 0, QCOM_RPM_SMD_KEY_ENABLE, _div)
 
 #define DEFINE_CLK_SMD_RPM_QDSS(_platform, _name, _active, type, r_id)	      \
 		__DEFINE_CLK_SMD_RPM(_platform, _name, _active, type, r_id,   \
@@ -121,12 +128,12 @@
 #define DEFINE_CLK_SMD_RPM_XO_BUFFER(_platform, _name, _active, r_id)	      \
 		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
 		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
-		QCOM_RPM_KEY_SOFTWARE_ENABLE)
+		QCOM_RPM_KEY_SOFTWARE_ENABLE, 0)
 
 #define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_platform, _name, _active, r_id) \
 		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
 		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
-		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY)
+		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY, 0)
 
 #define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)
 
@@ -140,6 +147,7 @@ struct clk_smd_rpm {
 	bool branch;
 	struct clk_smd_rpm *peer;
 	struct clk_hw hw;
+	u8 div;
 	unsigned long rate;
 	struct qcom_smd_rpm *rpm;
 };
@@ -370,10 +378,10 @@ static unsigned long clk_smd_rpm_recalc_rate(struct clk_hw *hw,
 
 	/*
 	 * RPM handles rate rounding and we don't have a way to
-	 * know what the rate will be, so just return whatever
-	 * rate was set.
+	 * know what the rate will be, so just return divided parent
+	 * rate or whatever rate was set.
 	 */
-	return r->rate;
+	return r->div ? parent_rate / r->div : r->rate;
 }
 
 static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
@@ -416,6 +424,7 @@ static const struct clk_ops clk_smd_rpm_ops = {
 static const struct clk_ops clk_smd_rpm_branch_ops = {
 	.prepare	= clk_smd_rpm_prepare,
 	.unprepare	= clk_smd_rpm_unprepare,
+	.recalc_rate	= clk_smd_rpm_recalc_rate,
 };
 
 DEFINE_CLK_SMD_RPM(msm8916, pcnoc_clk, pcnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
-- 
2.17.1

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

[Bjorn Andersson]

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@linaro.org>
> 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.

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).

Regards,
Bjorn

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/clk/qcom/clk-smd-rpm.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 66d7807ee38e..66ef0d3795fd 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -66,13 +66,14 @@
>  	}
>  
>  #define __DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type, r_id,    \
> -				    stat_id, r, key)			      \
> +				    stat_id, r, key, _div)		      \
>  	static struct clk_smd_rpm _platform##_##_active;		      \
>  	static struct clk_smd_rpm _platform##_##_name = {		      \
>  		.rpm_res_type = (type),					      \
>  		.rpm_clk_id = (r_id),					      \
>  		.rpm_status_id = (stat_id),				      \
>  		.rpm_key = (key),					      \
> +		.div = (_div),						      \
>  		.branch = true,						      \
>  		.peer = &_platform##_##_active,				      \
>  		.rate = (r),						      \
> @@ -92,6 +93,7 @@
>  		.rpm_status_id = (stat_id),				      \
>  		.active_only = true,					      \
>  		.rpm_key = (key),					      \
> +		.div = (_div),						      \
>  		.branch = true,						      \
>  		.peer = &_platform##_##_name,				      \
>  		.rate = (r),						      \
> @@ -112,7 +114,12 @@
>  
>  #define DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type, r_id, r)   \
>  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type,  \
> -		r_id, 0, r, QCOM_RPM_SMD_KEY_ENABLE)
> +		r_id, 0, r, QCOM_RPM_SMD_KEY_ENABLE, 0)
> +
> +#define DEFINE_CLK_SMD_RPM_BRANCH_DIV(_platform, _name, _active, type, r_id,  \
> +				      _div)				      \
> +		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type,  \
> +		r_id, 0, 0, QCOM_RPM_SMD_KEY_ENABLE, _div)
>  
>  #define DEFINE_CLK_SMD_RPM_QDSS(_platform, _name, _active, type, r_id)	      \
>  		__DEFINE_CLK_SMD_RPM(_platform, _name, _active, type, r_id,   \
> @@ -121,12 +128,12 @@
>  #define DEFINE_CLK_SMD_RPM_XO_BUFFER(_platform, _name, _active, r_id)	      \
>  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
>  		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> -		QCOM_RPM_KEY_SOFTWARE_ENABLE)
> +		QCOM_RPM_KEY_SOFTWARE_ENABLE, 0)
>  
>  #define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_platform, _name, _active, r_id) \
>  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
>  		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> -		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY)
> +		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY, 0)
>  
>  #define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)
>  
> @@ -140,6 +147,7 @@ struct clk_smd_rpm {
>  	bool branch;
>  	struct clk_smd_rpm *peer;
>  	struct clk_hw hw;
> +	u8 div;
>  	unsigned long rate;
>  	struct qcom_smd_rpm *rpm;
>  };
> @@ -370,10 +378,10 @@ static unsigned long clk_smd_rpm_recalc_rate(struct clk_hw *hw,
>  
>  	/*
>  	 * RPM handles rate rounding and we don't have a way to
> -	 * know what the rate will be, so just return whatever
> -	 * rate was set.
> +	 * know what the rate will be, so just return divided parent
> +	 * rate or whatever rate was set.
>  	 */
> -	return r->rate;
> +	return r->div ? parent_rate / r->div : r->rate;
>  }
>  
>  static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
> @@ -416,6 +424,7 @@ static const struct clk_ops clk_smd_rpm_ops = {
>  static const struct clk_ops clk_smd_rpm_branch_ops = {
>  	.prepare	= clk_smd_rpm_prepare,
>  	.unprepare	= clk_smd_rpm_unprepare,
> +	.recalc_rate	= clk_smd_rpm_recalc_rate,
>  };
>  
>  DEFINE_CLK_SMD_RPM(msm8916, pcnoc_clk, pcnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
> -- 
> 2.17.1
> 

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

[Bjorn Andersson]

On Wed 15 Sep 10:23 PDT 2021, 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@linaro.org>
> > 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.
> 
> 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).
> 

And as I sent that and was going to close the editor...

In contrast to clk-rpmh we already have a "rate" defined for each
clock in clk-smd-rpm, it's just that we don't let anyone know what it is
for branches.

But with your change of defining recalc_rate() for the branches we would
return 19.2MHz for the tcxo - without specifying a divider.

So your patch can be reduced to simply specifying recalc_rate() - i.e.
the last line in your patch.

I tested this on my sdm660 board, by updating xo_board and ensuring that
all nodes referring to "xo" refers to rpmcc tcxo intead and it boots
just fine. And it's backwards compatible with existing DT!



However, as we specify recalc_rate(), we'd start reporting that all the
buffered XO clocks are ticking at 1kHz, so I suspect that there might be
things that doesn't work. This is done so that rate/1000 becomes 0 or 1
for the branches.

But that will happen with the patch as you have given it as well, until
we've defined all the divs.

In other words, I don't think we should introduce the div, but rather
just rely on the "rate". And for the buffered clocks we need to correct
the rate - while ensuring that we pass a binary enable/rate to the
interface.

Regards,
Bjorn

> Regards,
> Bjorn
> 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/clk/qcom/clk-smd-rpm.c | 23 ++++++++++++++++-------
> >  1 file changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> > index 66d7807ee38e..66ef0d3795fd 100644
> > --- a/drivers/clk/qcom/clk-smd-rpm.c
> > +++ b/drivers/clk/qcom/clk-smd-rpm.c
> > @@ -66,13 +66,14 @@
> >  	}
> >  
> >  #define __DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type, r_id,    \
> > -				    stat_id, r, key)			      \
> > +				    stat_id, r, key, _div)		      \
> >  	static struct clk_smd_rpm _platform##_##_active;		      \
> >  	static struct clk_smd_rpm _platform##_##_name = {		      \
> >  		.rpm_res_type = (type),					      \
> >  		.rpm_clk_id = (r_id),					      \
> >  		.rpm_status_id = (stat_id),				      \
> >  		.rpm_key = (key),					      \
> > +		.div = (_div),						      \
> >  		.branch = true,						      \
> >  		.peer = &_platform##_##_active,				      \
> >  		.rate = (r),						      \
> > @@ -92,6 +93,7 @@
> >  		.rpm_status_id = (stat_id),				      \
> >  		.active_only = true,					      \
> >  		.rpm_key = (key),					      \
> > +		.div = (_div),						      \
> >  		.branch = true,						      \
> >  		.peer = &_platform##_##_name,				      \
> >  		.rate = (r),						      \
> > @@ -112,7 +114,12 @@
> >  
> >  #define DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type, r_id, r)   \
> >  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type,  \
> > -		r_id, 0, r, QCOM_RPM_SMD_KEY_ENABLE)
> > +		r_id, 0, r, QCOM_RPM_SMD_KEY_ENABLE, 0)
> > +
> > +#define DEFINE_CLK_SMD_RPM_BRANCH_DIV(_platform, _name, _active, type, r_id,  \
> > +				      _div)				      \
> > +		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active, type,  \
> > +		r_id, 0, 0, QCOM_RPM_SMD_KEY_ENABLE, _div)
> >  
> >  #define DEFINE_CLK_SMD_RPM_QDSS(_platform, _name, _active, type, r_id)	      \
> >  		__DEFINE_CLK_SMD_RPM(_platform, _name, _active, type, r_id,   \
> > @@ -121,12 +128,12 @@
> >  #define DEFINE_CLK_SMD_RPM_XO_BUFFER(_platform, _name, _active, r_id)	      \
> >  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
> >  		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> > -		QCOM_RPM_KEY_SOFTWARE_ENABLE)
> > +		QCOM_RPM_KEY_SOFTWARE_ENABLE, 0)
> >  
> >  #define DEFINE_CLK_SMD_RPM_XO_BUFFER_PINCTRL(_platform, _name, _active, r_id) \
> >  		__DEFINE_CLK_SMD_RPM_BRANCH(_platform, _name, _active,	      \
> >  		QCOM_SMD_RPM_CLK_BUF_A, r_id, 0, 1000,			      \
> > -		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY)
> > +		QCOM_RPM_KEY_PIN_CTRL_CLK_BUFFER_ENABLE_KEY, 0)
> >  
> >  #define to_clk_smd_rpm(_hw) container_of(_hw, struct clk_smd_rpm, hw)
> >  
> > @@ -140,6 +147,7 @@ struct clk_smd_rpm {
> >  	bool branch;
> >  	struct clk_smd_rpm *peer;
> >  	struct clk_hw hw;
> > +	u8 div;
> >  	unsigned long rate;
> >  	struct qcom_smd_rpm *rpm;
> >  };
> > @@ -370,10 +378,10 @@ static unsigned long clk_smd_rpm_recalc_rate(struct clk_hw *hw,
> >  
> >  	/*
> >  	 * RPM handles rate rounding and we don't have a way to
> > -	 * know what the rate will be, so just return whatever
> > -	 * rate was set.
> > +	 * know what the rate will be, so just return divided parent
> > +	 * rate or whatever rate was set.
> >  	 */
> > -	return r->rate;
> > +	return r->div ? parent_rate / r->div : r->rate;
> >  }
> >  
> >  static int clk_smd_rpm_enable_scaling(struct qcom_smd_rpm *rpm)
> > @@ -416,6 +424,7 @@ static const struct clk_ops clk_smd_rpm_ops = {
> >  static const struct clk_ops clk_smd_rpm_branch_ops = {
> >  	.prepare	= clk_smd_rpm_prepare,
> >  	.unprepare	= clk_smd_rpm_unprepare,
> > +	.recalc_rate	= clk_smd_rpm_recalc_rate,
> >  };
> >  
> >  DEFINE_CLK_SMD_RPM(msm8916, pcnoc_clk, pcnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
> > -- 
> > 2.17.1
> > 

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@linaro.org>
> > 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).

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

[Bjorn Andersson]

On Mon 13 Sep 19:55 PDT 2021, Shawn Guo wrote:

> 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.
> 

Most platforms has a crystal oscillator ticking at 38.4MHz feeding the
PMIC (represented by the rpmcc and its "xo" parent) and out comes the
bi_tcxo with a fixed 19.2MHz rate.

Is there a problem with the way sdm660_bi_tcxo is defined in this
regard?

Regards,
Bjorn

> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/clk/qcom/clk-smd-rpm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
> index 66d7807ee38e..2380e45b6247 100644
> --- a/drivers/clk/qcom/clk-smd-rpm.c
> +++ b/drivers/clk/qcom/clk-smd-rpm.c
> @@ -416,6 +416,9 @@ static const struct clk_ops clk_smd_rpm_ops = {
>  static const struct clk_ops clk_smd_rpm_branch_ops = {
>  	.prepare	= clk_smd_rpm_prepare,
>  	.unprepare	= clk_smd_rpm_unprepare,
> +	.set_rate	= clk_smd_rpm_set_rate,
> +	.round_rate	= clk_smd_rpm_round_rate,
> +	.recalc_rate	= clk_smd_rpm_recalc_rate,
>  };
>  
>  DEFINE_CLK_SMD_RPM(msm8916, pcnoc_clk, pcnoc_a_clk, QCOM_SMD_RPM_BUS_CLK, 0);
> -- 
> 2.17.1
> 

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

[Shawn Guo]

On Tue, Sep 14, 2021 at 07:55:21PM -0700, Bjorn Andersson wrote:
> On Mon 13 Sep 19:55 PDT 2021, Shawn Guo wrote:
> 
> > 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.
> > 
> 
> Most platforms has a crystal oscillator ticking at 38.4MHz feeding the
> PMIC (represented by the rpmcc and its "xo" parent) and out comes the
> bi_tcxo with a fixed 19.2MHz rate.

Yeah, but all those platforms are running clk-rpmh driver, I think.

> Is there a problem with the way sdm660_bi_tcxo is defined in this
> regard?

There is no problem if xo clock is 19.2MHz, but for platforms with
38.4MHz xo, bi_tcxo will be seen as 38.4MHz in clock tree, while we
expect it to be 19.2MHz.

Shawn

[PATCH 2/3] dt-bindings: clk: qcom,rpmcc: Document QCM2290 compatible

[Shawn Guo]

Add compatible for the RPM Clock Controller on the QCM2290 SoC.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 Documentation/devicetree/bindings/clock/qcom,rpmcc.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
index a4877881f1d8..da295c3c004b 100644
--- a/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
+++ b/Documentation/devicetree/bindings/clock/qcom,rpmcc.txt
@@ -25,6 +25,7 @@ Required properties :
 			"qcom,rpmcc-msm8994",·"qcom,rpmcc"
 			"qcom,rpmcc-msm8996", "qcom,rpmcc"
 			"qcom,rpmcc-msm8998", "qcom,rpmcc"
+			"qcom,rpmcc-qcm2290", "qcom,rpmcc"
 			"qcom,rpmcc-qcs404", "qcom,rpmcc"
 			"qcom,rpmcc-sdm660", "qcom,rpmcc"
 			"qcom,rpmcc-sm6115", "qcom,rpmcc"
-- 
2.17.1

[PATCH 3/3] clk: qcom: smd-rpm: Add QCM2290 RPM clock support

[Shawn Guo]

Add support for RPM-managed clocks on the QCM2290 platform.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/clk/qcom/clk-smd-rpm.c         | 59 ++++++++++++++++++++++++++
 include/dt-bindings/clock/qcom,rpmcc.h |  6 +++
 include/linux/soc/qcom/smd-rpm.h       |  2 +
 3 files changed, 67 insertions(+)

diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c
index 2380e45b6247..428830d800f6 100644
--- a/drivers/clk/qcom/clk-smd-rpm.c
+++ b/drivers/clk/qcom/clk-smd-rpm.c
@@ -1070,6 +1070,64 @@ static const struct rpm_smd_clk_desc rpm_clk_sm6115 = {
 	.num_clks = ARRAY_SIZE(sm6115_clks),
 };
 
+/* QCM2290 */
+DEFINE_CLK_SMD_RPM_XO_BUFFER(qcm2290, ln_bb_clk2, ln_bb_clk2_a, 0x2);
+DEFINE_CLK_SMD_RPM_XO_BUFFER(qcm2290, rf_clk3, rf_clk3_a, 6);
+
+DEFINE_CLK_SMD_RPM(qcm2290, qpic_clk, qpic_a_clk, QCOM_SMD_RPM_QPIC_CLK, 0);
+DEFINE_CLK_SMD_RPM(qcm2290, hwkm_clk, hwkm_a_clk, QCOM_SMD_RPM_HWKM_CLK, 0);
+DEFINE_CLK_SMD_RPM(qcm2290, pka_clk, pka_a_clk, QCOM_SMD_RPM_PKA_CLK, 0);
+DEFINE_CLK_SMD_RPM(qcm2290, cpuss_gnoc_clk, cpuss_gnoc_a_clk,
+		   QCOM_SMD_RPM_MEM_CLK, 1);
+DEFINE_CLK_SMD_RPM(qcm2290, bimc_gpu_clk, bimc_gpu_a_clk,
+		   QCOM_SMD_RPM_MEM_CLK, 2);
+
+static struct clk_smd_rpm *qcm2290_clks[] = {
+	[RPM_SMD_XO_CLK_SRC] = &sdm660_bi_tcxo,
+	[RPM_SMD_XO_A_CLK_SRC] = &sdm660_bi_tcxo_a,
+	[RPM_SMD_SNOC_CLK] = &sm6125_snoc_clk,
+	[RPM_SMD_SNOC_A_CLK] = &sm6125_snoc_a_clk,
+	[RPM_SMD_BIMC_CLK] = &msm8916_bimc_clk,
+	[RPM_SMD_BIMC_A_CLK] = &msm8916_bimc_a_clk,
+	[RPM_SMD_QDSS_CLK] = &sm6125_qdss_clk,
+	[RPM_SMD_QDSS_A_CLK] = &sm6125_qdss_a_clk,
+	[RPM_SMD_LN_BB_CLK2] = &qcm2290_ln_bb_clk2,
+	[RPM_SMD_LN_BB_CLK2_A] = &qcm2290_ln_bb_clk2_a,
+	[RPM_SMD_RF_CLK3] = &qcm2290_rf_clk3,
+	[RPM_SMD_RF_CLK3_A] = &qcm2290_rf_clk3_a,
+	[RPM_SMD_CNOC_CLK] = &sm6125_cnoc_clk,
+	[RPM_SMD_CNOC_A_CLK] = &sm6125_cnoc_a_clk,
+	[RPM_SMD_IPA_CLK] = &msm8976_ipa_clk,
+	[RPM_SMD_IPA_A_CLK] = &msm8976_ipa_a_clk,
+	[RPM_SMD_QUP_CLK] = &sm6125_qup_clk,
+	[RPM_SMD_QUP_A_CLK] = &sm6125_qup_a_clk,
+	[RPM_SMD_MMRT_CLK] = &sm6125_mmrt_clk,
+	[RPM_SMD_MMRT_A_CLK] = &sm6125_mmrt_a_clk,
+	[RPM_SMD_MMNRT_CLK] = &sm6125_mmnrt_clk,
+	[RPM_SMD_MMNRT_A_CLK] = &sm6125_mmnrt_a_clk,
+	[RPM_SMD_SNOC_PERIPH_CLK] = &sm6125_snoc_periph_clk,
+	[RPM_SMD_SNOC_PERIPH_A_CLK] = &sm6125_snoc_periph_a_clk,
+	[RPM_SMD_SNOC_LPASS_CLK] = &sm6125_snoc_lpass_clk,
+	[RPM_SMD_SNOC_LPASS_A_CLK] = &sm6125_snoc_lpass_a_clk,
+	[RPM_SMD_CE1_CLK] = &msm8992_ce1_clk,
+	[RPM_SMD_CE1_A_CLK] = &msm8992_ce1_a_clk,
+	[RPM_SMD_QPIC_CLK] = &qcm2290_qpic_clk,
+	[RPM_SMD_QPIC_CLK_A] = &qcm2290_qpic_a_clk,
+	[RPM_SMD_HWKM_CLK] = &qcm2290_hwkm_clk,
+	[RPM_SMD_HWKM_A_CLK] = &qcm2290_hwkm_a_clk,
+	[RPM_SMD_PKA_CLK] = &qcm2290_pka_clk,
+	[RPM_SMD_PKA_A_CLK] = &qcm2290_pka_a_clk,
+	[RPM_SMD_BIMC_GPU_CLK] = &qcm2290_bimc_gpu_clk,
+	[RPM_SMD_BIMC_GPU_A_CLK] = &qcm2290_bimc_gpu_a_clk,
+	[RPM_SMD_CPUSS_GNOC_CLK] = &qcm2290_cpuss_gnoc_clk,
+	[RPM_SMD_CPUSS_GNOC_A_CLK] = &qcm2290_cpuss_gnoc_a_clk,
+};
+
+static const struct rpm_smd_clk_desc rpm_clk_qcm2290 = {
+	.clks = qcm2290_clks,
+	.num_clks = ARRAY_SIZE(qcm2290_clks),
+};
+
 static const struct of_device_id rpm_smd_clk_match_table[] = {
 	{ .compatible = "qcom,rpmcc-mdm9607", .data = &rpm_clk_mdm9607 },
 	{ .compatible = "qcom,rpmcc-msm8226", .data = &rpm_clk_msm8974 },
@@ -1082,6 +1140,7 @@ static const struct of_device_id rpm_smd_clk_match_table[] = {
 	{ .compatible = "qcom,rpmcc-msm8994", .data = &rpm_clk_msm8994 },
 	{ .compatible = "qcom,rpmcc-msm8996", .data = &rpm_clk_msm8996 },
 	{ .compatible = "qcom,rpmcc-msm8998", .data = &rpm_clk_msm8998 },
+	{ .compatible = "qcom,rpmcc-qcm2290", .data = &rpm_clk_qcm2290 },
 	{ .compatible = "qcom,rpmcc-qcs404",  .data = &rpm_clk_qcs404  },
 	{ .compatible = "qcom,rpmcc-sdm660",  .data = &rpm_clk_sdm660  },
 	{ .compatible = "qcom,rpmcc-sm6115",  .data = &rpm_clk_sm6115  },
diff --git a/include/dt-bindings/clock/qcom,rpmcc.h b/include/dt-bindings/clock/qcom,rpmcc.h
index aa834d516234..fb624ff39273 100644
--- a/include/dt-bindings/clock/qcom,rpmcc.h
+++ b/include/dt-bindings/clock/qcom,rpmcc.h
@@ -159,5 +159,11 @@
 #define RPM_SMD_SNOC_PERIPH_A_CLK		113
 #define RPM_SMD_SNOC_LPASS_CLK			114
 #define RPM_SMD_SNOC_LPASS_A_CLK		115
+#define RPM_SMD_HWKM_CLK			116
+#define RPM_SMD_HWKM_A_CLK			117
+#define RPM_SMD_PKA_CLK				118
+#define RPM_SMD_PKA_A_CLK			119
+#define RPM_SMD_CPUSS_GNOC_CLK			120
+#define RPM_SMD_CPUSS_GNOC_A_CLK		121
 
 #endif
diff --git a/include/linux/soc/qcom/smd-rpm.h b/include/linux/soc/qcom/smd-rpm.h
index 60e66fc9b6bf..860dd8cdf9f3 100644
--- a/include/linux/soc/qcom/smd-rpm.h
+++ b/include/linux/soc/qcom/smd-rpm.h
@@ -38,6 +38,8 @@ struct qcom_smd_rpm;
 #define QCOM_SMD_RPM_IPA_CLK	0x617069
 #define QCOM_SMD_RPM_CE_CLK	0x6563
 #define QCOM_SMD_RPM_AGGR_CLK	0x72676761
+#define QCOM_SMD_RPM_HWKM_CLK	0x6d6b7768
+#define QCOM_SMD_RPM_PKA_CLK	0x616b70
 
 int qcom_rpm_smd_write(struct qcom_smd_rpm *rpm,
 		       int state,
-- 
2.17.1