[PATCH V2] clk: scmi: add is_prepared hook

[PATCH V2] 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
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

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

[Dhruva Gole]

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

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

[Cristian Marussi]

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

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

[Sudeep Holla]

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

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

[Dhruva Gole]

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>

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

[Sudeep Holla]

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

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

[Peng Fan]

Hi Sudeep, Dhruva,

> Subject: Re: [PATCH V2] clk: scmi: add is_prepared hook
> 
> 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.
> 

I will use below in V3:
"
Some clocks maybe default enabled by hardwar. For clocks that not 
have users, they will be left in hardware default state, because prepare
count and enable count is zero,if there is no is_prepared hook to get
the hardware state. So add is_prepared hook to detect the hardware
state. Then when disabling the unused clocks, they can be simply
turned OFF to save power during kernel boot.
"

I will post out v3 in later next week, waiting to see any more comments.

Thanks,
Peng.

> --
> Regards,
> Sudeep

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

[Dhruva Gole]

Hi Peng,

On Aug 01, 2024 at 03:35:37 +0000, Peng Fan wrote:
> Hi Sudeep, Dhruva,
> 
> > Subject: Re: [PATCH V2] clk: scmi: add is_prepared hook
> > 
> > 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.
> > 
> 
> I will use below in V3:
> "
> Some clocks maybe default enabled by hardwar. For clocks that not 

s/not/don't

> have users, they will be left in hardware default state, because prepare

s/they/that, will be left in default hardware state.

> count and enable count is zero,if there is no is_prepared hook to get
> the hardware state. So add is_prepared hook to detect the hardware
> state. Then when disabling the unused clocks, they can be simply
> turned OFF to save power during kernel boot.
> "

Thanks, rest looks better.

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