| Message ID | 20230811161446.636253-2-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add SCMI v3.2 Clock new CONFIGs support | expand |
Quoting Cristian Marussi (2023-08-11 09:14:41) > Add a param to Clock enable/disable operation to ask for atomic operation > and remove _atomic version of such operations. Why? > > No functional change. > > CC: Michael Turquette <mturquette@baylibre.com> > CC: Stephen Boyd <sboyd@kernel.org> > CC: linux-clk@vger.kernel.org > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > drivers/clk/clk-scmi.c | 8 ++++---- > drivers/firmware/arm_scmi/clock.c | 24 ++++++------------------ > include/linux/scmi_protocol.h | 9 ++++----- > 3 files changed, 14 insertions(+), 27 deletions(-) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 2c7a830ce308..ff003083e592 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -78,28 +78,28 @@ static int scmi_clk_enable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > > - return scmi_proto_clk_ops->enable(clk->ph, clk->id); > + return scmi_proto_clk_ops->enable(clk->ph, clk->id, false); > } > > static void scmi_clk_disable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > > - scmi_proto_clk_ops->disable(clk->ph, clk->id); > + scmi_proto_clk_ops->disable(clk->ph, clk->id, false); I enjoyed how it was before because I don't know what 'false' means without looking at the ops now. > } > > static int scmi_clk_atomic_enable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > > - return scmi_proto_clk_ops->enable_atomic(clk->ph, clk->id); > + return scmi_proto_clk_ops->enable(clk->ph, clk->id, true); > } > > static void scmi_clk_atomic_disable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > > - scmi_proto_clk_ops->disable_atomic(clk->ph, clk->id); > + scmi_proto_clk_ops->disable(clk->ph, clk->id, true); > } > > /*
On Tue, Aug 22, 2023 at 01:17:15PM -0700, Stephen Boyd wrote: > Quoting Cristian Marussi (2023-08-11 09:14:41) > > Add a param to Clock enable/disable operation to ask for atomic operation > > and remove _atomic version of such operations. > Hi, > Why? > :D, given that the 2 flavours of SCMI enable/disable ops (and the upcoming state_get) just differ in their operating mode (atomic or not) and the Clock framework in turn wrap such calls into 4 related and explicitly named clk_ops (scmi_clock_enable/scmi_clock_atomic_enable etc) that hint at what is being done, seemed to me reasonable to reduce the churn and remove a bit of code wrappers in favour of a param. > > > > No functional change. > > > > CC: Michael Turquette <mturquette@baylibre.com> > > CC: Stephen Boyd <sboyd@kernel.org> > > CC: linux-clk@vger.kernel.org > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > --- > > drivers/clk/clk-scmi.c | 8 ++++---- > > drivers/firmware/arm_scmi/clock.c | 24 ++++++------------------ > > include/linux/scmi_protocol.h | 9 ++++----- > > 3 files changed, 14 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > > index 2c7a830ce308..ff003083e592 100644 > > --- a/drivers/clk/clk-scmi.c > > +++ b/drivers/clk/clk-scmi.c > > @@ -78,28 +78,28 @@ static int scmi_clk_enable(struct clk_hw *hw) > > { > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > - return scmi_proto_clk_ops->enable(clk->ph, clk->id); > > + return scmi_proto_clk_ops->enable(clk->ph, clk->id, false); > > } > > > > static void scmi_clk_disable(struct clk_hw *hw) > > { > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > - scmi_proto_clk_ops->disable(clk->ph, clk->id); > > + scmi_proto_clk_ops->disable(clk->ph, clk->id, false); > > I enjoyed how it was before because I don't know what 'false' means > without looking at the ops now. > Yes indeed, I can drop this and rework if you prefer to maintain the old API calls, but this would mean that whenever we'll add new atomic flavour to some new SCMI clk operations we'll have to add 2 ops instead of a parametrized one...this is what would happen also in this series with state_get (and what really triggered this refactor) (and please consider that on the SCMI side, for testing purposes, I would prefer to expose always both atomic and non-atomic flavours even if NOT both actively used by the Clock framework...like state_get() that can only be atomic for Clock frmwk...) Thanks, Cristian
Quoting Cristian Marussi (2023-08-23 02:02:46) > On Tue, Aug 22, 2023 at 01:17:15PM -0700, Stephen Boyd wrote: > > Quoting Cristian Marussi (2023-08-11 09:14:41) > > > Add a param to Clock enable/disable operation to ask for atomic operation > > > and remove _atomic version of such operations. > > > > Hi, Yo > > > Why? > > > > :D, given that the 2 flavours of SCMI enable/disable ops (and the upcoming > state_get) just differ in their operating mode (atomic or not) and the > Clock framework in turn wrap such calls into 4 related and explicitly > named clk_ops (scmi_clock_enable/scmi_clock_atomic_enable etc) that hint > at what is being done, seemed to me reasonable to reduce the churn and > remove a bit of code wrappers in favour of a param. Please add these extra details to the commit text about why we're making the change. > > > > > > > No functional change. > > > > > > CC: Michael Turquette <mturquette@baylibre.com> > > > CC: Stephen Boyd <sboyd@kernel.org> > > > CC: linux-clk@vger.kernel.org > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > --- > > > drivers/clk/clk-scmi.c | 8 ++++---- > > > drivers/firmware/arm_scmi/clock.c | 24 ++++++------------------ > > > include/linux/scmi_protocol.h | 9 ++++----- > > > 3 files changed, 14 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > > > index 2c7a830ce308..ff003083e592 100644 > > > --- a/drivers/clk/clk-scmi.c > > > +++ b/drivers/clk/clk-scmi.c > > > @@ -78,28 +78,28 @@ static int scmi_clk_enable(struct clk_hw *hw) > > > { > > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > > > - return scmi_proto_clk_ops->enable(clk->ph, clk->id); > > > + return scmi_proto_clk_ops->enable(clk->ph, clk->id, false); > > > } > > > > > > static void scmi_clk_disable(struct clk_hw *hw) > > > { > > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > > > - scmi_proto_clk_ops->disable(clk->ph, clk->id); > > > + scmi_proto_clk_ops->disable(clk->ph, clk->id, false); > > > > I enjoyed how it was before because I don't know what 'false' means > > without looking at the ops now. > > > > Yes indeed, I can drop this and rework if you prefer to maintain the old > API calls, but this would mean that whenever we'll add new atomic > flavour to some new SCMI clk operations we'll have to add 2 ops instead > of a parametrized one...this is what would happen also in this series > with state_get (and what really triggered this refactor) > > (and please consider that on the SCMI side, for testing purposes, I would > prefer to expose always both atomic and non-atomic flavours even if NOT > both actively used by the Clock framework...like state_get() that can only > be atomic for Clock frmwk...) > Perhaps we need a local variable to make it more readable. static int scmi_clk_enable(struct clk_hw *hw) { bool can_sleep = false; struct scmi_clk *clk = to_scmi_clk(hw); return scmi_proto_clk_ops->enable(clk->ph, clk->id, can_sleep); } This let's the reader quickly understand what the parameter means. I'm OK with adding the function parameter, but a plain 'true' or 'false' doesn't help with clarity.
On Wed, Aug 23, 2023 at 11:01:17AM -0700, Stephen Boyd wrote: > Quoting Cristian Marussi (2023-08-23 02:02:46) > > On Tue, Aug 22, 2023 at 01:17:15PM -0700, Stephen Boyd wrote: > > > Quoting Cristian Marussi (2023-08-11 09:14:41) > > > > Add a param to Clock enable/disable operation to ask for atomic operation > > > > and remove _atomic version of such operations. > > > > > > > Hi, > > Yo > > > > > > Why? > > > > > > > :D, given that the 2 flavours of SCMI enable/disable ops (and the upcoming > > state_get) just differ in their operating mode (atomic or not) and the > > Clock framework in turn wrap such calls into 4 related and explicitly > > named clk_ops (scmi_clock_enable/scmi_clock_atomic_enable etc) that hint > > at what is being done, seemed to me reasonable to reduce the churn and > > remove a bit of code wrappers in favour of a param. > > Please add these extra details to the commit text about why we're making > the change. > Sure I'll do. > > > > > > > > > > No functional change. > > > > > > > > CC: Michael Turquette <mturquette@baylibre.com> > > > > CC: Stephen Boyd <sboyd@kernel.org> > > > > CC: linux-clk@vger.kernel.org > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > > --- > > > > drivers/clk/clk-scmi.c | 8 ++++---- > > > > drivers/firmware/arm_scmi/clock.c | 24 ++++++------------------ > > > > include/linux/scmi_protocol.h | 9 ++++----- > > > > 3 files changed, 14 insertions(+), 27 deletions(-) > > > > > > > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > > > > index 2c7a830ce308..ff003083e592 100644 > > > > --- a/drivers/clk/clk-scmi.c > > > > +++ b/drivers/clk/clk-scmi.c > > > > @@ -78,28 +78,28 @@ static int scmi_clk_enable(struct clk_hw *hw) > > > > { > > > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > > > > > - return scmi_proto_clk_ops->enable(clk->ph, clk->id); > > > > + return scmi_proto_clk_ops->enable(clk->ph, clk->id, false); > > > > } > > > > > > > > static void scmi_clk_disable(struct clk_hw *hw) > > > > { > > > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > > > > > - scmi_proto_clk_ops->disable(clk->ph, clk->id); > > > > + scmi_proto_clk_ops->disable(clk->ph, clk->id, false); > > > > > > I enjoyed how it was before because I don't know what 'false' means > > > without looking at the ops now. > > > > > > > Yes indeed, I can drop this and rework if you prefer to maintain the old > > API calls, but this would mean that whenever we'll add new atomic > > flavour to some new SCMI clk operations we'll have to add 2 ops instead > > of a parametrized one...this is what would happen also in this series > > with state_get (and what really triggered this refactor) > > > > (and please consider that on the SCMI side, for testing purposes, I would > > prefer to expose always both atomic and non-atomic flavours even if NOT > > both actively used by the Clock framework...like state_get() that can only > > be atomic for Clock frmwk...) > > > > Perhaps we need a local variable to make it more readable. > > static int scmi_clk_enable(struct clk_hw *hw) > { > bool can_sleep = false; > struct scmi_clk *clk = to_scmi_clk(hw); > > return scmi_proto_clk_ops->enable(clk->ph, clk->id, can_sleep); > } > > This let's the reader quickly understand what the parameter means. I'm > OK with adding the function parameter, but a plain 'true' or 'false' > doesn't help with clarity. Thanks for the suggestion, it would help definitely making it more readable, maybe a local define or enum could make it without even putting anything on the stack. Thanks, Cristian
Quoting Cristian Marussi (2023-08-24 07:25:21) > On Wed, Aug 23, 2023 at 11:01:17AM -0700, Stephen Boyd wrote: > > > > Perhaps we need a local variable to make it more readable. > > > > static int scmi_clk_enable(struct clk_hw *hw) > > { > > bool can_sleep = false; > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > return scmi_proto_clk_ops->enable(clk->ph, clk->id, can_sleep); > > } > > > > This let's the reader quickly understand what the parameter means. I'm > > OK with adding the function parameter, but a plain 'true' or 'false' > > doesn't help with clarity. > > Thanks for the suggestion, it would help definitely making it more > readable, maybe a local define or enum could make it without even > putting anything on the stack. > Surely the compiler can optimize that so there isn't stack local storage for a local variable used as an argument to a function call?
On Thu, Aug 24, 2023 at 11:43:35AM -0700, Stephen Boyd wrote: > Quoting Cristian Marussi (2023-08-24 07:25:21) > > On Wed, Aug 23, 2023 at 11:01:17AM -0700, Stephen Boyd wrote: > > > > > > Perhaps we need a local variable to make it more readable. > > > > > > static int scmi_clk_enable(struct clk_hw *hw) > > > { > > > bool can_sleep = false; > > > struct scmi_clk *clk = to_scmi_clk(hw); > > > > > > return scmi_proto_clk_ops->enable(clk->ph, clk->id, can_sleep); > > > } > > > > > > This let's the reader quickly understand what the parameter means. I'm > > > OK with adding the function parameter, but a plain 'true' or 'false' > > > doesn't help with clarity. > > > > Thanks for the suggestion, it would help definitely making it more > > readable, maybe a local define or enum could make it without even > > putting anything on the stack. > > > > Surely the compiler can optimize that so there isn't stack local > storage for a local variable used as an argument to a function call? Yes indeed the compiler will certainly drop anything at the end, but still I'd have to fill with local vars definitions all the related functions just to be able to make them more readable while I can improve the readability also by just adding a pair descriptive defines to use all over. I'll send a V2 and then you tell if it is fine for you. Thanks Cristian
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 2c7a830ce308..ff003083e592 100644 --- a/drivers/clk/clk-scmi.c +++ b/drivers/clk/clk-scmi.c @@ -78,28 +78,28 @@ static int scmi_clk_enable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); - return scmi_proto_clk_ops->enable(clk->ph, clk->id); + return scmi_proto_clk_ops->enable(clk->ph, clk->id, false); } static void scmi_clk_disable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); - scmi_proto_clk_ops->disable(clk->ph, clk->id); + scmi_proto_clk_ops->disable(clk->ph, clk->id, false); } static int scmi_clk_atomic_enable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); - return scmi_proto_clk_ops->enable_atomic(clk->ph, clk->id); + return scmi_proto_clk_ops->enable(clk->ph, clk->id, true); } static void scmi_clk_atomic_disable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); - scmi_proto_clk_ops->disable_atomic(clk->ph, clk->id); + scmi_proto_clk_ops->disable(clk->ph, clk->id, true); } /* diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index 96060bf90a24..447d29b5fc72 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c @@ -418,26 +418,16 @@ scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id, return ret; } -static int scmi_clock_enable(const struct scmi_protocol_handle *ph, u32 clk_id) +static int scmi_clock_enable(const struct scmi_protocol_handle *ph, u32 clk_id, + bool atomic) { - return scmi_clock_config_set(ph, clk_id, CLOCK_ENABLE, false); + return scmi_clock_config_set(ph, clk_id, CLOCK_ENABLE, atomic); } -static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id) +static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id, + bool atomic) { - return scmi_clock_config_set(ph, clk_id, 0, false); -} - -static int scmi_clock_enable_atomic(const struct scmi_protocol_handle *ph, - u32 clk_id) -{ - return scmi_clock_config_set(ph, clk_id, CLOCK_ENABLE, true); -} - -static int scmi_clock_disable_atomic(const struct scmi_protocol_handle *ph, - u32 clk_id) -{ - return scmi_clock_config_set(ph, clk_id, 0, true); + return scmi_clock_config_set(ph, clk_id, 0, atomic); } static int scmi_clock_count_get(const struct scmi_protocol_handle *ph) @@ -470,8 +460,6 @@ static const struct scmi_clk_proto_ops clk_proto_ops = { .rate_set = scmi_clock_rate_set, .enable = scmi_clock_enable, .disable = scmi_clock_disable, - .enable_atomic = scmi_clock_enable_atomic, - .disable_atomic = scmi_clock_disable_atomic, }; static int scmi_clk_rate_notify(const struct scmi_protocol_handle *ph, diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index e6fe4f73ffe6..b4c631a8d0ac 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h @@ -90,11 +90,10 @@ struct scmi_clk_proto_ops { u64 *rate); int (*rate_set)(const struct scmi_protocol_handle *ph, u32 clk_id, u64 rate); - int (*enable)(const struct scmi_protocol_handle *ph, u32 clk_id); - int (*disable)(const struct scmi_protocol_handle *ph, u32 clk_id); - int (*enable_atomic)(const struct scmi_protocol_handle *ph, u32 clk_id); - int (*disable_atomic)(const struct scmi_protocol_handle *ph, - u32 clk_id); + int (*enable)(const struct scmi_protocol_handle *ph, u32 clk_id, + bool atomic); + int (*disable)(const struct scmi_protocol_handle *ph, u32 clk_id, + bool atomic); }; /**
Add a param to Clock enable/disable operation to ask for atomic operation and remove _atomic version of such operations. No functional change. CC: Michael Turquette <mturquette@baylibre.com> CC: Stephen Boyd <sboyd@kernel.org> CC: linux-clk@vger.kernel.org Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/clk/clk-scmi.c | 8 ++++---- drivers/firmware/arm_scmi/clock.c | 24 ++++++------------------ include/linux/scmi_protocol.h | 9 ++++----- 3 files changed, 14 insertions(+), 27 deletions(-)