| Message ID | 20240725090741.1039642-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | clk: scmi: add is_prepared hook | expand |
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
> 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>
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>
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 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
> 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
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; } }