* [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations
2023-08-11 16:14 [PATCH 0/6] Add SCMI v3.2 Clock new CONFIGs support Cristian Marussi
@ 2023-08-11 16:14 ` Cristian Marussi
2023-08-22 20:17 ` Stephen Boyd
2023-08-11 16:14 ` [PATCH 2/6] firmware: arm_scmi: Add Clock v3.2 CONFIG_SET support Cristian Marussi
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Cristian Marussi @ 2023-08-11 16:14 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi, Michael Turquette,
Stephen Boyd, linux-clk
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(-)
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);
};
/**
--
2.41.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations
2023-08-11 16:14 ` [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations Cristian Marussi
@ 2023-08-22 20:17 ` Stephen Boyd
2023-08-23 9:02 ` Cristian Marussi
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2023-08-22 20:17 UTC (permalink / raw)
To: Cristian Marussi, linux-arm-kernel, linux-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi, Michael Turquette, linux-clk
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);
> }
>
> /*
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations
2023-08-22 20:17 ` Stephen Boyd
@ 2023-08-23 9:02 ` Cristian Marussi
2023-08-23 18:01 ` Stephen Boyd
0 siblings, 1 reply; 14+ messages in thread
From: Cristian Marussi @ 2023-08-23 9:02 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-kernel, linux-kernel, sudeep.holla, james.quinlan,
f.fainelli, vincent.guittot, etienne.carriere, peng.fan,
chuck.cannon, souvik.chakravarty, nicola.mazzucato,
Michael Turquette, linux-clk
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations
2023-08-23 9:02 ` Cristian Marussi
@ 2023-08-23 18:01 ` Stephen Boyd
2023-08-24 14:25 ` Cristian Marussi
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2023-08-23 18:01 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-arm-kernel, linux-kernel, sudeep.holla, james.quinlan,
f.fainelli, vincent.guittot, etienne.carriere, peng.fan,
chuck.cannon, souvik.chakravarty, nicola.mazzucato,
Michael Turquette, linux-clk
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations
2023-08-23 18:01 ` Stephen Boyd
@ 2023-08-24 14:25 ` Cristian Marussi
2023-08-24 18:43 ` Stephen Boyd
0 siblings, 1 reply; 14+ messages in thread
From: Cristian Marussi @ 2023-08-24 14:25 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-kernel, linux-kernel, sudeep.holla, james.quinlan,
f.fainelli, vincent.guittot, etienne.carriere, peng.fan,
chuck.cannon, souvik.chakravarty, nicola.mazzucato,
Michael Turquette, linux-clk
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations
2023-08-24 14:25 ` Cristian Marussi
@ 2023-08-24 18:43 ` Stephen Boyd
2023-08-26 12:50 ` Cristian Marussi
0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2023-08-24 18:43 UTC (permalink / raw)
To: Cristian Marussi
Cc: linux-arm-kernel, linux-kernel, sudeep.holla, james.quinlan,
f.fainelli, vincent.guittot, etienne.carriere, peng.fan,
chuck.cannon, souvik.chakravarty, nicola.mazzucato,
Michael Turquette, linux-clk
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?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations
2023-08-24 18:43 ` Stephen Boyd
@ 2023-08-26 12:50 ` Cristian Marussi
0 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-08-26 12:50 UTC (permalink / raw)
To: Stephen Boyd
Cc: linux-arm-kernel, linux-kernel, sudeep.holla, james.quinlan,
f.fainelli, vincent.guittot, etienne.carriere, peng.fan,
chuck.cannon, souvik.chakravarty, nicola.mazzucato,
Michael Turquette, linux-clk
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/6] firmware: arm_scmi: Add Clock v3.2 CONFIG_SET support
2023-08-11 16:14 [PATCH 0/6] Add SCMI v3.2 Clock new CONFIGs support Cristian Marussi
2023-08-11 16:14 ` [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations Cristian Marussi
@ 2023-08-11 16:14 ` Cristian Marussi
2023-08-11 16:14 ` [PATCH 3/6] firmware: arm_scmi: Add v3.2 Clock CONFIG_GET support Cristian Marussi
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-08-11 16:14 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi
SCMI v3.2 introduces a new Clock CONFIG_SET message format that can
optionally carry also OEM specific configuration values beside the usual
clock enable/disable requests.
Refactor internal helpers and add support to use such new format when
talking to a v3.2 compliant SCMI platform.
Support existing enable/disable operations across different Clock protocol
versions: this patch still does not add protocol operations to support the
new OEM specific optional configuration capabilities.
No functional change for the SCMI drivers users of the related enable and
disable clock operations.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/clock.c | 88 ++++++++++++++++++++++++++++---
1 file changed, 80 insertions(+), 8 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 447d29b5fc72..63bd043365cd 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -23,6 +23,13 @@ enum scmi_clock_protocol_cmd {
CLOCK_RATE_CHANGE_REQUESTED_NOTIFY = 0xA,
};
+enum clk_state {
+ CLK_STATE_DISABLE,
+ CLK_STATE_ENABLE,
+ CLK_STATE_RESERVED,
+ CLK_STATE_UNCHANGED,
+};
+
struct scmi_msg_resp_clock_protocol_attributes {
__le16 num_clocks;
u8 max_async_req;
@@ -31,7 +38,6 @@ struct scmi_msg_resp_clock_protocol_attributes {
struct scmi_msg_resp_clock_attributes {
__le32 attributes;
-#define CLOCK_ENABLE BIT(0)
#define SUPPORTS_RATE_CHANGED_NOTIF(x) ((x) & BIT(31))
#define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) & BIT(30))
#define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
@@ -39,9 +45,18 @@ struct scmi_msg_resp_clock_attributes {
__le32 clock_enable_latency;
};
-struct scmi_clock_set_config {
+struct scmi_msg_clock_config_set_v2 {
+ __le32 id;
+ __le32 attributes;
+};
+
+struct scmi_msg_clock_config_set_v21 {
__le32 id;
__le32 attributes;
+#define NULL_OEM_TYPE 0
+#define REGMASK_OEM_TYPE_SET GENMASK(23, 16)
+#define REGMASK_CLK_STATE GENMASK(1, 0)
+ __le32 oem_config_val;
};
struct scmi_msg_clock_describe_rates {
@@ -100,6 +115,9 @@ struct clock_info {
int max_async_req;
atomic_t cur_async_req;
struct scmi_clock_info *clk;
+ int (*clock_config_set)(const struct scmi_protocol_handle *ph,
+ u32 clk_id, enum clk_state state,
+ u8 oem_type, u32 oem_val, bool atomic);
};
static enum scmi_clock_protocol_cmd evt_2_cmd[] = {
@@ -394,12 +412,47 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
}
static int
-scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
- u32 config, bool atomic)
+scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
+ enum clk_state state, u8 __unused0, u32 __unused1,
+ bool atomic)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_clock_config_set_v2 *cfg;
+
+ if (state >= CLK_STATE_RESERVED)
+ return -EINVAL;
+
+ ret = ph->xops->xfer_get_init(ph, CLOCK_CONFIG_SET,
+ sizeof(*cfg), 0, &t);
+ if (ret)
+ return ret;
+
+ t->hdr.poll_completion = atomic;
+
+ cfg = t->tx.buf;
+ cfg->id = cpu_to_le32(clk_id);
+ cfg->attributes = cpu_to_le32(state);
+
+ ret = ph->xops->do_xfer(ph, t);
+
+ ph->xops->xfer_put(ph, t);
+ return ret;
+}
+
+static int
+scmi_clock_config_set_v21(const struct scmi_protocol_handle *ph, u32 clk_id,
+ enum clk_state state, u8 oem_type, u32 oem_val,
+ bool atomic)
{
int ret;
+ u32 attrs;
struct scmi_xfer *t;
- struct scmi_clock_set_config *cfg;
+ struct scmi_msg_clock_config_set_v21 *cfg;
+
+ if (state == CLK_STATE_RESERVED ||
+ (!oem_type && state == CLK_STATE_UNCHANGED))
+ return -EINVAL;
ret = ph->xops->xfer_get_init(ph, CLOCK_CONFIG_SET,
sizeof(*cfg), 0, &t);
@@ -408,9 +461,16 @@ scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id,
t->hdr.poll_completion = atomic;
+ attrs = FIELD_PREP(REGMASK_OEM_TYPE_SET, oem_type) |
+ FIELD_PREP(REGMASK_CLK_STATE, state);
+
cfg = t->tx.buf;
cfg->id = cpu_to_le32(clk_id);
- cfg->attributes = cpu_to_le32(config);
+ cfg->attributes = cpu_to_le32(attrs);
+ /* Clear in any case */
+ cfg->oem_config_val = cpu_to_le32(0);
+ if (oem_type)
+ cfg->oem_config_val = cpu_to_le32(oem_val);
ret = ph->xops->do_xfer(ph, t);
@@ -421,13 +481,19 @@ scmi_clock_config_set(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, atomic);
+ struct clock_info *ci = ph->get_priv(ph);
+
+ return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
+ NULL_OEM_TYPE, 0, atomic);
}
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, atomic);
+ struct clock_info *ci = ph->get_priv(ph);
+
+ return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
+ NULL_OEM_TYPE, 0, atomic);
}
static int scmi_clock_count_get(const struct scmi_protocol_handle *ph)
@@ -592,6 +658,12 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
scmi_clock_describe_rates_get(ph, clkid, clk);
}
+ if (PROTOCOL_REV_MAJOR(version) >= 0x2 &&
+ PROTOCOL_REV_MINOR(version) >= 0x1)
+ cinfo->clock_config_set = scmi_clock_config_set_v21;
+ else
+ cinfo->clock_config_set = scmi_clock_config_set_v2;
+
cinfo->version = version;
return ph->set_priv(ph, cinfo);
}
--
2.41.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 3/6] firmware: arm_scmi: Add v3.2 Clock CONFIG_GET support
2023-08-11 16:14 [PATCH 0/6] Add SCMI v3.2 Clock new CONFIGs support Cristian Marussi
2023-08-11 16:14 ` [PATCH 1/6] firmware: arm_scmi: Simplify enable/disable Clock operations Cristian Marussi
2023-08-11 16:14 ` [PATCH 2/6] firmware: arm_scmi: Add Clock v3.2 CONFIG_SET support Cristian Marussi
@ 2023-08-11 16:14 ` Cristian Marussi
2023-08-11 16:14 ` [PATCH 4/6] firmware: arm_scmi: Add Clock .state_get support to pre-v3.2 Cristian Marussi
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-08-11 16:14 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi
Add support for v3.2 Clock CONFIG_GET command and related new clock
protocol operation state_get() to retrieve the status of a clock.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/clock.c | 64 +++++++++++++++++++++++++++++++
include/linux/scmi_protocol.h | 3 ++
2 files changed, 67 insertions(+)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 63bd043365cd..aaa95624493d 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -21,6 +21,7 @@ enum scmi_clock_protocol_cmd {
CLOCK_NAME_GET = 0x8,
CLOCK_RATE_NOTIFY = 0x9,
CLOCK_RATE_CHANGE_REQUESTED_NOTIFY = 0xA,
+ CLOCK_CONFIG_GET = 0xB,
};
enum clk_state {
@@ -59,6 +60,19 @@ struct scmi_msg_clock_config_set_v21 {
__le32 oem_config_val;
};
+struct scmi_msg_clock_config_get {
+ __le32 id;
+ __le32 flags;
+#define REGMASK_OEM_TYPE_GET GENMASK(7, 0)
+};
+
+struct scmi_msg_resp_clock_config_get {
+ __le32 attributes;
+ __le32 config;
+#define IS_CLK_ENABLED(x) le32_get_bits((x), BIT(0))
+ __le32 oem_config_val;
+};
+
struct scmi_msg_clock_describe_rates {
__le32 id;
__le32 rate_index;
@@ -496,6 +510,55 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
NULL_OEM_TYPE, 0, atomic);
}
+static int
+scmi_clock_config_get(const struct scmi_protocol_handle *ph, u32 clk_id,
+ u8 oem_type, u32 *attributes, bool *enabled,
+ u32 *oem_val, bool atomic)
+{
+ int ret;
+ u32 flags;
+ struct scmi_xfer *t;
+ struct scmi_msg_clock_config_get *cfg;
+
+ ret = ph->xops->xfer_get_init(ph, CLOCK_CONFIG_GET,
+ sizeof(*cfg), 0, &t);
+ if (ret)
+ return ret;
+
+ t->hdr.poll_completion = atomic;
+
+ flags = FIELD_PREP(REGMASK_OEM_TYPE_GET, oem_type);
+
+ cfg = t->tx.buf;
+ cfg->id = cpu_to_le32(clk_id);
+ cfg->flags = cpu_to_le32(flags);
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret) {
+ struct scmi_msg_resp_clock_config_get *resp = t->rx.buf;
+
+ if (attributes)
+ *attributes = le32_to_cpu(resp->attributes);
+
+ if (enabled)
+ *enabled = IS_CLK_ENABLED(resp->config);
+
+ if (oem_val && oem_type)
+ *oem_val = le32_to_cpu(resp->oem_config_val);
+ }
+
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
+static int scmi_clock_state_get(const struct scmi_protocol_handle *ph,
+ u32 clk_id, bool *enabled, bool atomic)
+{
+ return scmi_clock_config_get(ph, clk_id, NULL_OEM_TYPE, NULL,
+ enabled, NULL, atomic);
+}
+
static int scmi_clock_count_get(const struct scmi_protocol_handle *ph)
{
struct clock_info *ci = ph->get_priv(ph);
@@ -526,6 +589,7 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
.rate_set = scmi_clock_rate_set,
.enable = scmi_clock_enable,
.disable = scmi_clock_disable,
+ .state_get = scmi_clock_state_get,
};
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 b4c631a8d0ac..d11ca4286d57 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -80,6 +80,7 @@ struct scmi_protocol_handle;
* @rate_set: set the clock rate of a clock
* @enable: enables the specified clock
* @disable: disables the specified clock
+ * @state_get: get the status of the specified clock
*/
struct scmi_clk_proto_ops {
int (*count_get)(const struct scmi_protocol_handle *ph);
@@ -94,6 +95,8 @@ struct scmi_clk_proto_ops {
bool atomic);
int (*disable)(const struct scmi_protocol_handle *ph, u32 clk_id,
bool atomic);
+ int (*state_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
+ bool *enabled, bool atomic);
};
/**
--
2.41.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 4/6] firmware: arm_scmi: Add Clock .state_get support to pre-v3.2
2023-08-11 16:14 [PATCH 0/6] Add SCMI v3.2 Clock new CONFIGs support Cristian Marussi
` (2 preceding siblings ...)
2023-08-11 16:14 ` [PATCH 3/6] firmware: arm_scmi: Add v3.2 Clock CONFIG_GET support Cristian Marussi
@ 2023-08-11 16:14 ` Cristian Marussi
2023-08-11 16:14 ` [PATCH 5/6] clk: scmi: Add support for .is_enabled clk_ops Cristian Marussi
2023-08-11 16:14 ` [PATCH 6/6] [RFC] firmware: arm_scmi: Add Clock OEM config clock operations Cristian Marussi
5 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-08-11 16:14 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi
Support Clock .state_get operation against SCMI platform servers that do
not support v3.2 CONFIG_GET dedicated command: while talking with these
platforms the command CLOCK_ATTRIBUTES can be used to gather the current
clock states.
Note that, in case of shared resources, the retrieved clock state 'flavour'
(virtual vs physical) depends on the backend SCMI platform server specific
kind of implementation.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/clock.c | 52 ++++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 7 deletions(-)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index aaa95624493d..333d08822f77 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -132,6 +132,9 @@ struct clock_info {
int (*clock_config_set)(const struct scmi_protocol_handle *ph,
u32 clk_id, enum clk_state state,
u8 oem_type, u32 oem_val, bool atomic);
+ int (*clock_config_get)(const struct scmi_protocol_handle *ph,
+ u32 clk_id, u8 oem_type, u32 *attributes,
+ bool *enabled, u32 *oem_val, bool atomic);
};
static enum scmi_clock_protocol_cmd evt_2_cmd[] = {
@@ -511,9 +514,9 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
}
static int
-scmi_clock_config_get(const struct scmi_protocol_handle *ph, u32 clk_id,
- u8 oem_type, u32 *attributes, bool *enabled,
- u32 *oem_val, bool atomic)
+scmi_clock_config_get_v21(const struct scmi_protocol_handle *ph, u32 clk_id,
+ u8 oem_type, u32 *attributes, bool *enabled,
+ u32 *oem_val, bool atomic)
{
int ret;
u32 flags;
@@ -552,11 +555,43 @@ scmi_clock_config_get(const struct scmi_protocol_handle *ph, u32 clk_id,
return ret;
}
+static int
+scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id,
+ u8 oem_type, u32 *attributes, bool *enabled,
+ u32 *oem_val, bool atomic)
+{
+ int ret;
+ struct scmi_xfer *t;
+ struct scmi_msg_resp_clock_attributes *resp;
+
+ if (!enabled)
+ return -EINVAL;
+
+ ret = ph->xops->xfer_get_init(ph, CLOCK_ATTRIBUTES,
+ sizeof(clk_id), sizeof(*resp), &t);
+ if (ret)
+ return ret;
+
+ t->hdr.poll_completion = atomic;
+ put_unaligned_le32(clk_id, t->tx.buf);
+ resp = t->rx.buf;
+
+ ret = ph->xops->do_xfer(ph, t);
+ if (!ret)
+ *enabled = IS_CLK_ENABLED(resp->attributes);
+
+ ph->xops->xfer_put(ph, t);
+
+ return ret;
+}
+
static int scmi_clock_state_get(const struct scmi_protocol_handle *ph,
u32 clk_id, bool *enabled, bool atomic)
{
- return scmi_clock_config_get(ph, clk_id, NULL_OEM_TYPE, NULL,
- enabled, NULL, atomic);
+ struct clock_info *ci = ph->get_priv(ph);
+
+ return ci->clock_config_get(ph, clk_id, NULL_OEM_TYPE, NULL,
+ enabled, NULL, atomic);
}
static int scmi_clock_count_get(const struct scmi_protocol_handle *ph)
@@ -723,10 +758,13 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
}
if (PROTOCOL_REV_MAJOR(version) >= 0x2 &&
- PROTOCOL_REV_MINOR(version) >= 0x1)
+ PROTOCOL_REV_MINOR(version) >= 0x1) {
cinfo->clock_config_set = scmi_clock_config_set_v21;
- else
+ cinfo->clock_config_get = scmi_clock_config_get_v21;
+ } else {
cinfo->clock_config_set = scmi_clock_config_set_v2;
+ cinfo->clock_config_get = scmi_clock_config_get_v2;
+ }
cinfo->version = version;
return ph->set_priv(ph, cinfo);
--
2.41.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 5/6] clk: scmi: Add support for .is_enabled clk_ops
2023-08-11 16:14 [PATCH 0/6] Add SCMI v3.2 Clock new CONFIGs support Cristian Marussi
` (3 preceding siblings ...)
2023-08-11 16:14 ` [PATCH 4/6] firmware: arm_scmi: Add Clock .state_get support to pre-v3.2 Cristian Marussi
@ 2023-08-11 16:14 ` Cristian Marussi
2023-08-22 20:22 ` Stephen Boyd
2023-08-11 16:14 ` [PATCH 6/6] [RFC] firmware: arm_scmi: Add Clock OEM config clock operations Cristian Marussi
5 siblings, 1 reply; 14+ messages in thread
From: Cristian Marussi @ 2023-08-11 16:14 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi, Michael Turquette,
Stephen Boyd, linux-clk
Add support for .is_enabled atomic clk_ops using the related SCMI Clock
operation in atomic mode, if available.
Note that the .is_enabled callback will be supported by this SCMI Clock
driver only if the configured underlying SCMI transport does support atomic
operations.
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 | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index ff003083e592..3770b58cc882 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -17,6 +17,7 @@ static const struct scmi_clk_proto_ops *scmi_proto_clk_ops;
struct scmi_clk {
u32 id;
+ struct device *dev;
struct clk_hw hw;
const struct scmi_clock_info *info;
const struct scmi_protocol_handle *ph;
@@ -102,10 +103,24 @@ static void scmi_clk_atomic_disable(struct clk_hw *hw)
scmi_proto_clk_ops->disable(clk->ph, clk->id, true);
}
+static int scmi_clk_atomic_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, true);
+ if (ret)
+ dev_warn(clk->dev,
+ "Failed to get state for clock ID %d\n", clk->id);
+
+ return !!enabled;
+}
+
/*
- * We can provide enable/disable atomic callbacks only if the underlying SCMI
- * transport for an SCMI instance is configured to handle SCMI commands in an
- * atomic manner.
+ * We can provide enable/disable/is_enabled atomic callbacks only if the
+ * underlying SCMI transport for an SCMI instance is configured to handle
+ * SCMI commands in an atomic manner.
*
* When no SCMI atomic transport support is available we instead provide only
* the prepare/unprepare API, as allowed by the clock framework when atomic
@@ -129,6 +144,7 @@ static const struct clk_ops scmi_atomic_clk_ops = {
.set_rate = scmi_clk_set_rate,
.enable = scmi_clk_atomic_enable,
.disable = scmi_clk_atomic_disable,
+ .is_enabled = scmi_clk_atomic_is_enabled,
};
static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk,
@@ -218,6 +234,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
sclk->id = idx;
sclk->ph = ph;
+ sclk->dev = dev;
/*
* Note that when transport is atomic but SCMI protocol did not
--
2.41.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 5/6] clk: scmi: Add support for .is_enabled clk_ops
2023-08-11 16:14 ` [PATCH 5/6] clk: scmi: Add support for .is_enabled clk_ops Cristian Marussi
@ 2023-08-22 20:22 ` Stephen Boyd
0 siblings, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2023-08-22 20:22 UTC (permalink / raw)
To: Cristian Marussi, linux-arm-kernel, linux-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi, Michael Turquette, linux-clk
Quoting Cristian Marussi (2023-08-11 09:14:45)
> Add support for .is_enabled atomic clk_ops using the related SCMI Clock
> operation in atomic mode, if available.
>
> Note that the .is_enabled callback will be supported by this SCMI Clock
> driver only if the configured underlying SCMI transport does support atomic
> operations.
>
> 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>
> ---
Acked-by: Stephen Boyd <sboyd@kernel.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 6/6] [RFC] firmware: arm_scmi: Add Clock OEM config clock operations
2023-08-11 16:14 [PATCH 0/6] Add SCMI v3.2 Clock new CONFIGs support Cristian Marussi
` (4 preceding siblings ...)
2023-08-11 16:14 ` [PATCH 5/6] clk: scmi: Add support for .is_enabled clk_ops Cristian Marussi
@ 2023-08-11 16:14 ` Cristian Marussi
5 siblings, 0 replies; 14+ messages in thread
From: Cristian Marussi @ 2023-08-11 16:14 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: sudeep.holla, james.quinlan, f.fainelli, vincent.guittot,
etienne.carriere, peng.fan, chuck.cannon, souvik.chakravarty,
nicola.mazzucato, Cristian Marussi
Expose a couple of new SCMI Clock operations to get and set OEM specific
clock configurations when talking to an SCMI v3.2 compliant.
Issuing such requests against an SCMI platform server not supporting v3.2
extension for OEM specific clock configurations will fail.
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
drivers/firmware/arm_scmi/clock.c | 22 ++++++++++++++++++++++
include/linux/scmi_protocol.h | 7 +++++++
2 files changed, 29 insertions(+)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 333d08822f77..d18bf789fc24 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -594,6 +594,26 @@ static int scmi_clock_state_get(const struct scmi_protocol_handle *ph,
enabled, NULL, atomic);
}
+static int scmi_clock_config_oem_set(const struct scmi_protocol_handle *ph,
+ u32 clk_id, u8 oem_type, u32 oem_val,
+ bool atomic)
+{
+ struct clock_info *ci = ph->get_priv(ph);
+
+ return ci->clock_config_set(ph, clk_id, CLK_STATE_UNCHANGED,
+ oem_type, oem_val, atomic);
+}
+
+static int scmi_clock_config_oem_get(const struct scmi_protocol_handle *ph,
+ u32 clk_id, u8 oem_type, u32 *oem_val,
+ u32 *attributes, bool atomic)
+{
+ struct clock_info *ci = ph->get_priv(ph);
+
+ return ci->clock_config_get(ph, clk_id, oem_type, attributes,
+ NULL, oem_val, atomic);
+}
+
static int scmi_clock_count_get(const struct scmi_protocol_handle *ph)
{
struct clock_info *ci = ph->get_priv(ph);
@@ -625,6 +645,8 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
.enable = scmi_clock_enable,
.disable = scmi_clock_disable,
.state_get = scmi_clock_state_get,
+ .config_oem_get = scmi_clock_config_oem_get,
+ .config_oem_set = scmi_clock_config_oem_set,
};
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 d11ca4286d57..e09ac428fa1b 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -81,6 +81,8 @@ struct scmi_protocol_handle;
* @enable: enables the specified clock
* @disable: disables the specified clock
* @state_get: get the status of the specified clock
+ * @config_oem_get: get the value of an OEM specific clock config
+ * @config_oem_set: set the value of an OEM specific clock config
*/
struct scmi_clk_proto_ops {
int (*count_get)(const struct scmi_protocol_handle *ph);
@@ -97,6 +99,11 @@ struct scmi_clk_proto_ops {
bool atomic);
int (*state_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
bool *enabled, bool atomic);
+ int (*config_oem_get)(const struct scmi_protocol_handle *ph, u32 clk_id,
+ u8 oem_type, u32 *oem_val, u32 *attributes,
+ bool atomic);
+ int (*config_oem_set)(const struct scmi_protocol_handle *ph, u32 clk_id,
+ u8 oem_type, u32 oem_val, bool atomic);
};
/**
--
2.41.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 14+ messages in thread