[PATCH] clk: scmi: add is_prepared hook

[PATCH] clk: scmi: add is_prepared hook

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Some clks maybe default enabled by hardware, so add is_prepared hook
to get the status of the clk. Then when disabling unused clks, those
unused clks but default hardware on clks could be in off state to save
power.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/clk-scmi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index d86a02563f6c..d2d370337ba5 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -142,6 +142,20 @@ static void scmi_clk_disable(struct clk_hw *hw)
 	scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC);
 }
 
+static int scmi_clk_is_enabled(struct clk_hw *hw)
+{
+	int ret;
+	bool enabled = false;
+	struct scmi_clk *clk = to_scmi_clk(hw);
+
+	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled, NOT_ATOMIC);
+	if (ret)
+		dev_warn(clk->dev,
+			 "Failed to get state for clock ID %d\n", clk->id);
+
+	return !!enabled;
+}
+
 static int scmi_clk_atomic_enable(struct clk_hw *hw)
 {
 	struct scmi_clk *clk = to_scmi_clk(hw);
@@ -280,6 +294,7 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
 		} else {
 			ops->prepare = scmi_clk_enable;
 			ops->unprepare = scmi_clk_disable;
+			ops->is_prepared = scmi_clk_is_enabled;
 		}
 	}
 
-- 
2.37.1

Re: [PATCH] clk: scmi: add is_prepared hook

[Dhruva Gole]

On Jul 25, 2024 at 17:07:41 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Some clks maybe default enabled by hardware, so add is_prepared hook

Why is_prepared when there is an is_enabled hook?
See in the atomic case we already have something similar:

ops->is_enabled = scmi_clk_atomic_is_enabled;

> to get the status of the clk. Then when disabling unused clks, those
> unused clks but default hardware on clks could be in off state to save
> power.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/clk-scmi.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index d86a02563f6c..d2d370337ba5 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -142,6 +142,20 @@ static void scmi_clk_disable(struct clk_hw *hw)
>  	scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC);
>  }
>  
> +static int scmi_clk_is_enabled(struct clk_hw *hw)
> +{
> +	int ret;
> +	bool enabled = false;
> +	struct scmi_clk *clk = to_scmi_clk(hw);
> +
> +	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled, NOT_ATOMIC);
> +	if (ret)
> +		dev_warn(clk->dev,
> +			 "Failed to get state for clock ID %d\n", clk->id);
> +
> +	return !!enabled;
> +}
> +
>  static int scmi_clk_atomic_enable(struct clk_hw *hw)
>  {
>  	struct scmi_clk *clk = to_scmi_clk(hw);
> @@ -280,6 +294,7 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>  		} else {
>  			ops->prepare = scmi_clk_enable;
>  			ops->unprepare = scmi_clk_disable;
> +			ops->is_prepared = scmi_clk_is_enabled;

IMO from the decription and what the function is doing is_enabled makes
more sense here to me, unless there's a better explanation.

Ref: linux/clk-provider.h
is_prepared: Queries the hardware to determine if the clock is prepared
vs
is_enabled: Queries the hardware to determine if the clock is enabled

-- 
Best regards,
Dhruva Gole <d-gole@ti.com>

RE: [PATCH] clk: scmi: add is_prepared hook

[Peng Fan]

> Subject: Re: [PATCH] clk: scmi: add is_prepared hook
> 
> On Jul 25, 2024 at 17:07:41 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > Some clks maybe default enabled by hardware, so add is_prepared
> hook
> 
> Why is_prepared when there is an is_enabled hook?

This patch is for non-atomic clk ops. The is_enabled hook is
In atomic clk ops.

> See in the atomic case we already have something similar:
> 
> ops->is_enabled = scmi_clk_atomic_is_enabled;
> 
> > to get the status of the clk. Then when disabling unused clks, those
> > unused clks but default hardware on clks could be in off state to save
> > power.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/clk/clk-scmi.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index
> > d86a02563f6c..d2d370337ba5 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -142,6 +142,20 @@ static void scmi_clk_disable(struct clk_hw
> *hw)
> >  	scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC);  }
> >
> > +static int scmi_clk_is_enabled(struct clk_hw *hw) {
> > +	int ret;
> > +	bool enabled = false;
> > +	struct scmi_clk *clk = to_scmi_clk(hw);
> > +
> > +	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled,
> NOT_ATOMIC);
> > +	if (ret)
> > +		dev_warn(clk->dev,
> > +			 "Failed to get state for clock ID %d\n", clk-
> >id);
> > +
> > +	return !!enabled;
> > +}
> > +
> >  static int scmi_clk_atomic_enable(struct clk_hw *hw)  {
> >  	struct scmi_clk *clk = to_scmi_clk(hw); @@ -280,6 +294,7
> @@
> > scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
> >  		} else {
> >  			ops->prepare = scmi_clk_enable;
> >  			ops->unprepare = scmi_clk_disable;
> > +			ops->is_prepared = scmi_clk_is_enabled;
> 
> IMO from the decription and what the function is doing is_enabled
> makes
> more sense here to me, unless there's a better explanation.
> 
> Ref: linux/clk-provider.h
> is_prepared: Queries the hardware to determine if the clock is prepared
> vs
> is_enabled: Queries the hardware to determine if the clock is enabled

SCMI firmware has no knowledge of prepare/unprepared.

As wrote in the beginning, this patch is for non atomic clk ops.

Regards,
Peng.

> 
> --
> Best regards,
> Dhruva Gole <d-gole@ti.com>

Re: [PATCH] clk: scmi: add is_prepared hook

[Dhruva Gole]

On Jul 26, 2024 at 09:28:52 +0000, Peng Fan wrote:
> > Subject: Re: [PATCH] clk: scmi: add is_prepared hook
> > 
> > On Jul 25, 2024 at 17:07:41 +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > Some clks maybe default enabled by hardware, so add is_prepared
> > hook
> > 
> > Why is_prepared when there is an is_enabled hook?
> 
> This patch is for non-atomic clk ops. The is_enabled hook is
> In atomic clk ops.
> 
> > See in the atomic case we already have something similar:
> > 
> > ops->is_enabled = scmi_clk_atomic_is_enabled;
> > 
> > > to get the status of the clk. Then when disabling unused clks, those
> > > unused clks but default hardware on clks could be in off state to save
> > > power.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/clk/clk-scmi.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index
> > > d86a02563f6c..d2d370337ba5 100644
> > > --- a/drivers/clk/clk-scmi.c
> > > +++ b/drivers/clk/clk-scmi.c
> > > @@ -142,6 +142,20 @@ static void scmi_clk_disable(struct clk_hw
> > *hw)
> > >  	scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC);  }
> > >
> > > +static int scmi_clk_is_enabled(struct clk_hw *hw) {
> > > +	int ret;
> > > +	bool enabled = false;
> > > +	struct scmi_clk *clk = to_scmi_clk(hw);
> > > +
> > > +	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled,
> > NOT_ATOMIC);
> > > +	if (ret)
> > > +		dev_warn(clk->dev,
> > > +			 "Failed to get state for clock ID %d\n", clk-
> > >id);
> > > +
> > > +	return !!enabled;
> > > +}
> > > +
> > >  static int scmi_clk_atomic_enable(struct clk_hw *hw)  {
> > >  	struct scmi_clk *clk = to_scmi_clk(hw); @@ -280,6 +294,7
> > @@
> > > scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
> > >  		} else {
> > >  			ops->prepare = scmi_clk_enable;
> > >  			ops->unprepare = scmi_clk_disable;
> > > +			ops->is_prepared = scmi_clk_is_enabled;
> > 
> > IMO from the decription and what the function is doing is_enabled
> > makes
> > more sense here to me, unless there's a better explanation.
> > 
> > Ref: linux/clk-provider.h
> > is_prepared: Queries the hardware to determine if the clock is prepared
> > vs
> > is_enabled: Queries the hardware to determine if the clock is enabled
> 
> SCMI firmware has no knowledge of prepare/unprepared.
> 
> As wrote in the beginning, this patch is for non atomic clk ops.

OK, I got carried away with the wording of is_prepared a bit but it
seems like prepare/unprepare are inter changeably used to enable/disable
in non atomic cases and so it makes sense to follow suit with is_prepared.

Thanks for clarifying,

Reviewed-by: Dhruva Gole <d-gole@ti.com>


-- 
Best regards,
Dhruva

Re: [PATCH] clk: scmi: add is_prepared hook

[Cristian Marussi]

On Thu, Jul 25, 2024 at 05:07:41PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Some clks maybe default enabled by hardware, so add is_prepared hook
> to get the status of the clk. Then when disabling unused clks, those
> unused clks but default hardware on clks could be in off state to save
> power.
> 

Hi Peng,

seems a good addition to me, I forgot to add supporrt for non atomic
scenarios like for clk enable/disable....

...having said that, though, you basically copied the original ATOMIC
version of this function and changed only the ATOMIC -> NON_ATOMIC param
for the state_get() call...

...unless I am missing something, this sounds to me as needless duplication
of code...please rework the original existing function to be an internal helper
acceppting an additinal parameter (ATOMIC, NON_ATOMIC) and then build 2 wrappers
omn top of it for the original and your new function...

Thanks,
Cristian

Re: [PATCH] clk: scmi: add is_prepared hook

[Cristian Marussi]

On Thu, Jul 25, 2024 at 05:07:41PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 

.... one more remark..

> Some clks maybe default enabled by hardware, so add is_prepared hook
> to get the status of the clk. Then when disabling unused clks, those
> unused clks but default hardware on clks could be in off state to save
> power.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/clk-scmi.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index d86a02563f6c..d2d370337ba5 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -142,6 +142,20 @@ static void scmi_clk_disable(struct clk_hw *hw)
>  	scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC);
>  }
>  
> +static int scmi_clk_is_enabled(struct clk_hw *hw)
> +{
> +	int ret;
> +	bool enabled = false;
> +	struct scmi_clk *clk = to_scmi_clk(hw);
> +
> +	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled, NOT_ATOMIC);
> +	if (ret)
> +		dev_warn(clk->dev,
> +			 "Failed to get state for clock ID %d\n", clk->id);
> +
> +	return !!enabled;
> +}
> +
>  static int scmi_clk_atomic_enable(struct clk_hw *hw)
>  {
>  	struct scmi_clk *clk = to_scmi_clk(hw);
> @@ -280,6 +294,7 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>  		} else {
>  			ops->prepare = scmi_clk_enable;
>  			ops->unprepare = scmi_clk_disable;
> +			ops->is_prepared = scmi_clk_is_enabled;

... you should NOT add the is_prepared ops here, since this would mean
that you will have the is_prepared ops available only when
SCMI_CLK_STATE_CTRL_SUPPORTED, WHILE you most probably want to be able
to check (non atomically) the current enabled-status for a clock EVEN IF
you are allowed to change its state...please add the is_prepared in the
else-branch down below where the ATOMIC is_enabled is added

Thanks,
Cristian

RE: [PATCH] clk: scmi: add is_prepared hook

[Peng Fan]

> Subject: Re: [PATCH] clk: scmi: add is_prepared hook
> 
> On Thu, Jul 25, 2024 at 05:07:41PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> 
> .... one more remark..
> 
> > Some clks maybe default enabled by hardware, so add is_prepared
> hook
> > to get the status of the clk. Then when disabling unused clks, those
> > unused clks but default hardware on clks could be in off state to save
> > power.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/clk/clk-scmi.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index
> > d86a02563f6c..d2d370337ba5 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -142,6 +142,20 @@ static void scmi_clk_disable(struct clk_hw
> *hw)
> >  	scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC);  }
> >
> > +static int scmi_clk_is_enabled(struct clk_hw *hw) {
> > +	int ret;
> > +	bool enabled = false;
> > +	struct scmi_clk *clk = to_scmi_clk(hw);
> > +
> > +	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled,
> NOT_ATOMIC);
> > +	if (ret)
> > +		dev_warn(clk->dev,
> > +			 "Failed to get state for clock ID %d\n", clk-
> >id);
> > +
> > +	return !!enabled;
> > +}
> > +
> >  static int scmi_clk_atomic_enable(struct clk_hw *hw)  {
> >  	struct scmi_clk *clk = to_scmi_clk(hw); @@ -280,6 +294,7
> @@
> > scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
> >  		} else {
> >  			ops->prepare = scmi_clk_enable;
> >  			ops->unprepare = scmi_clk_disable;
> > +			ops->is_prepared = scmi_clk_is_enabled;
> 
> ... you should NOT add the is_prepared ops here, since this would
> mean
> that you will have the is_prepared ops available only when
> SCMI_CLK_STATE_CTRL_SUPPORTED, WHILE you most probably want
> to be able
> to check (non atomically) the current enabled-status for a clock EVEN
> IF
> you are allowed to change its state...please add the is_prepared in the
> else-branch down below where the ATOMIC is_enabled is added

Will use this in v2. And together try to avoid duplicate code.
        if (feats_key & BIT(SCMI_CLK_ATOMIC_SUPPORTED))
                ops->is_enabled = scmi_clk_atomic_is_enabled;
+       else
+               ops->is_prepared = scmi_clk_is_enabled;
 
        /* Rate ops */

Thanks,
Peng.

> 
> Thanks,
> Cristian