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

Some clks maybe default enabled by hardware, so add is_prepared hook
for non-atomic clk_ops 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>
---

V2:
 Provider helper __scmi_clk_is_enabled for atomic and non-atomic usage
 Move is_prepared hook out of SCMI_CLK_STATE_CTRL_SUPPORTED

 drivers/clk/clk-scmi.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index d86a02563f6c..15510c2ff21c 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -156,13 +156,13 @@ static void scmi_clk_atomic_disable(struct clk_hw *hw)
 	scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC);
 }
 
-static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
+static int __scmi_clk_is_enabled(struct clk_hw *hw, bool atomic)
 {
 	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, ATOMIC);
+	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled, atomic);
 	if (ret)
 		dev_warn(clk->dev,
 			 "Failed to get state for clock ID %d\n", clk->id);
@@ -170,6 +170,16 @@ static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
 	return !!enabled;
 }
 
+static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
+{
+	return __scmi_clk_is_enabled(hw, ATOMIC);
+}
+
+static int scmi_clk_is_enabled(struct clk_hw *hw)
+{
+	return __scmi_clk_is_enabled(hw, NOT_ATOMIC);
+}
+
 static int scmi_clk_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
 {
 	int ret;
@@ -285,6 +295,8 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
 
 	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 */
 	ops->recalc_rate = scmi_clk_recalc_rate;
-- 
2.37.1

On Jul 26, 2024 at 21:10:07 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Some clks maybe default enabled by hardware, so add is_prepared hook
> for non-atomic clk_ops 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.

Just a nit - reword the commit message as:
Then when disabling the unused clocks, they can be simply turned OFF to
save power.

Also if you can make it still verbose, explain when you expect this
disabling of unused clks to take place exactly? During boot?  Driver probe sequence?
or By some user commands?

> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  Provider helper __scmi_clk_is_enabled for atomic and non-atomic usage
>  Move is_prepared hook out of SCMI_CLK_STATE_CTRL_SUPPORTED
[...]


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

On Fri, Jul 26, 2024 at 07:22:31PM +0530, Dhruva Gole wrote:
> On Jul 26, 2024 at 21:10:07 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > Some clks maybe default enabled by hardware, so add is_prepared hook
> > for non-atomic clk_ops 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.
> 
> Just a nit - reword the commit message as:
> Then when disabling the unused clocks, they can be simply turned OFF to
> save power.
>

Ah this was what it meant. I couldn't parse the original text and was about
to ask.

> Also if you can make it still verbose, explain when you expect this
> disabling of unused clks to take place exactly? During boot?  Driver probe sequence?
> or By some user commands?
>

Agreed. Being little more verbose here would be beneficial IMO.

-- 
Regards,
Sudeep

On Jul 26, 2024 at 21:10:07 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Some clks maybe default enabled by hardware, so add is_prepared hook
> for non-atomic clk_ops 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>
> ---
> 
> V2:
>  Provider helper __scmi_clk_is_enabled for atomic and non-atomic usage
>  Move is_prepared hook out of SCMI_CLK_STATE_CTRL_SUPPORTED
> 
>  drivers/clk/clk-scmi.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> index d86a02563f6c..15510c2ff21c 100644
> --- a/drivers/clk/clk-scmi.c
> +++ b/drivers/clk/clk-scmi.c
> @@ -156,13 +156,13 @@ static void scmi_clk_atomic_disable(struct clk_hw *hw)
>  	scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC);
>  }
>  
> -static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
> +static int __scmi_clk_is_enabled(struct clk_hw *hw, bool atomic)

I think we can combine other atomic/non atomic in the same way no?
Let me know if I should send a follow up patch based on this to make
__scmi_clk_enable(hw,atomic) and __scmi_clk_disable(hw,atomic)

I'd be more than happy to do so.

>  {
>  	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, ATOMIC);
> +	ret = scmi_proto_clk_ops->state_get(clk->ph, clk->id, &enabled, atomic);
>  	if (ret)
>  		dev_warn(clk->dev,
>  			 "Failed to get state for clock ID %d\n", clk->id);
> @@ -170,6 +170,16 @@ static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
>  	return !!enabled;
>  }
>  
> +static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
> +{
> +	return __scmi_clk_is_enabled(hw, ATOMIC);
> +}
> +
> +static int scmi_clk_is_enabled(struct clk_hw *hw)
> +{
> +	return __scmi_clk_is_enabled(hw, NOT_ATOMIC);
> +}
> +
>  static int scmi_clk_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
>  {
>  	int ret;
> @@ -285,6 +295,8 @@ scmi_clk_ops_alloc(struct device *dev, unsigned long feats_key)
>  
>  	if (feats_key & BIT(SCMI_CLK_ATOMIC_SUPPORTED))
>  		ops->is_enabled = scmi_clk_atomic_is_enabled;
> +	else
> +		ops->is_prepared = scmi_clk_is_enabled;

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


-- 
Best regards,
Dhruva

On Fri, Jul 26, 2024 at 07:14:14PM +0530, Dhruva Gole wrote:
> On Jul 26, 2024 at 21:10:07 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > Some clks maybe default enabled by hardware, so add is_prepared hook
> > for non-atomic clk_ops 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>
> > ---
> > 
> > V2:
> >  Provider helper __scmi_clk_is_enabled for atomic and non-atomic usage
> >  Move is_prepared hook out of SCMI_CLK_STATE_CTRL_SUPPORTED
> > 
> >  drivers/clk/clk-scmi.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > index d86a02563f6c..15510c2ff21c 100644
> > --- a/drivers/clk/clk-scmi.c
> > +++ b/drivers/clk/clk-scmi.c
> > @@ -156,13 +156,13 @@ static void scmi_clk_atomic_disable(struct clk_hw *hw)
> >  	scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC);
> >  }
> >  
> > -static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
> > +static int __scmi_clk_is_enabled(struct clk_hw *hw, bool atomic)
> 
> I think we can combine other atomic/non atomic in the same way no?
> Let me know if I should send a follow up patch based on this to make
> __scmi_clk_enable(hw,atomic) and __scmi_clk_disable(hw,atomic)

I dont think that it is worth unifying also the disable/enable atomic and
non_atomic versions because if you look at their implementations they are
indeed already wrappers around the state_get()....this new is_prepared/is_enabled
were more 'thick' and so there was  a lot of duplicated code.

Thanks
Cristian

On Fri, Jul 26, 2024 at 03:11:08PM +0100, Cristian Marussi wrote:
> On Fri, Jul 26, 2024 at 07:14:14PM +0530, Dhruva Gole wrote:
> > On Jul 26, 2024 at 21:10:07 +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > > 
> > > Some clks maybe default enabled by hardware, so add is_prepared hook
> > > for non-atomic clk_ops 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>
> > > ---
> > > 
> > > V2:
> > >  Provider helper __scmi_clk_is_enabled for atomic and non-atomic usage
> > >  Move is_prepared hook out of SCMI_CLK_STATE_CTRL_SUPPORTED
> > > 
> > >  drivers/clk/clk-scmi.c | 16 ++++++++++++++--
> > >  1 file changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
> > > index d86a02563f6c..15510c2ff21c 100644
> > > --- a/drivers/clk/clk-scmi.c
> > > +++ b/drivers/clk/clk-scmi.c
> > > @@ -156,13 +156,13 @@ static void scmi_clk_atomic_disable(struct clk_hw *hw)
> > >  	scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC);
> > >  }
> > >  
> > > -static int scmi_clk_atomic_is_enabled(struct clk_hw *hw)
> > > +static int __scmi_clk_is_enabled(struct clk_hw *hw, bool atomic)
> > 
> > I think we can combine other atomic/non atomic in the same way no?
> > Let me know if I should send a follow up patch based on this to make
> > __scmi_clk_enable(hw,atomic) and __scmi_clk_disable(hw,atomic)
> 
> I dont think that it is worth unifying also the disable/enable atomic and
> non_atomic versions because if you look at their implementations they are
> indeed already wrappers around the state_get()....this new is_prepared/is_enabled
> were more 'thick' and so there was  a lot of duplicated code.
> 

+1, was planning to respond with similar message. Just jumped now as you
made it easier for me 😁.

-- 
Regards,
Sudeep