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