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

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

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

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>