* [PATCH V1] firmware: arm_scmi: clock: support clock denied flags
@ 2023-10-31 12:27 Peng Fan (OSS)
2023-10-31 12:30 ` Cristian Marussi
2023-10-31 13:03 ` Cristian Marussi
0 siblings, 2 replies; 5+ messages in thread
From: Peng Fan (OSS) @ 2023-10-31 12:27 UTC (permalink / raw)
To: sudeep.holla, cristian.marussi
Cc: linux-arm-kernel, linux-clk, linux-kernel, ranjani.vaidyanathan,
glen.wienecke, nitin.garg, chuck.cannon, Souvik.Chakravarty,
Peng Fan
From: Peng Fan <peng.fan@nxp.com>
The firmware may export flags to indicate whether the clock
is allowed to set rate, set parent, enable/disable from the Agent.
If Agent is not allowed to enable/disable, directly return success.
If Agent is not allowed to set rate/parent, directly return -EACCES to
avoid SCMI RPC calls.
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
V1:
drop the changes in clock.c, add an attribute entry in clock info which
may be easy for extending new flag.
SPEC still not have such support, this is for discussion
drivers/firmware/arm_scmi/clock.c | 19 +++++++++++++++++++
include/linux/scmi_protocol.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 42b81c181d68..fad4329a21fc 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -46,6 +46,9 @@ struct scmi_msg_resp_clock_attributes {
#define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) & BIT(30))
#define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
#define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
+#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15)
+#define SCMI_CLOCK_SET_RATE_DENIED BIT(14)
+#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13)
u8 name[SCMI_SHORT_NAME_MAX_SIZE];
__le32 clock_enable_latency;
};
@@ -307,6 +310,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
if (PROTOCOL_REV_MAJOR(version) >= 0x2)
latency = le32_to_cpu(attr->clock_enable_latency);
clk->enable_latency = latency ? : U32_MAX;
+ clk->attributes = attributes;
}
ph->xops->xfer_put(ph, t);
@@ -499,6 +503,10 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
struct scmi_xfer *t;
struct scmi_clock_set_rate *cfg;
struct clock_info *ci = ph->get_priv(ph);
+ struct scmi_clock_info *clk = ci->clk + clk_id;
+
+ if (clk->attributes & SCMI_CLOCK_SET_RATE_DENIED)
+ return -EACCES;
ret = ph->xops->xfer_get_init(ph, CLOCK_RATE_SET, sizeof(*cfg), 0, &t);
if (ret)
@@ -585,6 +593,9 @@ scmi_clock_set_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
if (parent_id >= clk->num_parents)
return -EINVAL;
+ if (clk->attributes & SCMI_CLOCK_SET_PARENT_DENIED)
+ return -EACCES;
+
ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
sizeof(*cfg), 0, &t);
if (ret)
@@ -668,6 +679,10 @@ static int scmi_clock_enable(const struct scmi_protocol_handle *ph, u32 clk_id,
bool atomic)
{
struct clock_info *ci = ph->get_priv(ph);
+ struct scmi_clock_info *clk = ci->clk + clk_id;
+
+ if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
+ return 0;
return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
NULL_OEM_TYPE, 0, atomic);
@@ -677,6 +692,10 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
bool atomic)
{
struct clock_info *ci = ph->get_priv(ph);
+ struct scmi_clock_info *clk = ci->clk + clk_id;
+
+ if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
+ return 0;
return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
NULL_OEM_TYPE, 0, atomic);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f2f05fb42d28..ddf5363c8cfd 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -58,6 +58,7 @@ struct scmi_clock_info {
u64 step_size;
} range;
};
+ u32 attributes;
int num_parents;
u32 *parents;
};
--
2.37.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH V1] firmware: arm_scmi: clock: support clock denied flags
2023-10-31 12:27 [PATCH V1] firmware: arm_scmi: clock: support clock denied flags Peng Fan (OSS)
@ 2023-10-31 12:30 ` Cristian Marussi
2023-10-31 13:03 ` Cristian Marussi
1 sibling, 0 replies; 5+ messages in thread
From: Cristian Marussi @ 2023-10-31 12:30 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: sudeep.holla, linux-arm-kernel, linux-clk, linux-kernel,
ranjani.vaidyanathan, glen.wienecke, nitin.garg, chuck.cannon,
Souvik.Chakravarty, Peng Fan
On Tue, Oct 31, 2023 at 08:27:34PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> The firmware may export flags to indicate whether the clock
> is allowed to set rate, set parent, enable/disable from the Agent.
>
> If Agent is not allowed to enable/disable, directly return success.
> If Agent is not allowed to set rate/parent, directly return -EACCES to
> avoid SCMI RPC calls.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
Hi Peng,
thanks for this, this is what I meant indeed.
I suppose soon-ish a spec proposal will be circulated about this and
we'll move forward from there when we have details.
Thanks,
Cristian
>
> V1:
> drop the changes in clock.c, add an attribute entry in clock info which
> may be easy for extending new flag.
>
> SPEC still not have such support, this is for discussion
>
> drivers/firmware/arm_scmi/clock.c | 19 +++++++++++++++++++
> include/linux/scmi_protocol.h | 1 +
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 42b81c181d68..fad4329a21fc 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -46,6 +46,9 @@ struct scmi_msg_resp_clock_attributes {
> #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) & BIT(30))
> #define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
> #define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
> +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15)
> +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14)
> +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13)
> u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> __le32 clock_enable_latency;
> };
> @@ -307,6 +310,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> if (PROTOCOL_REV_MAJOR(version) >= 0x2)
> latency = le32_to_cpu(attr->clock_enable_latency);
> clk->enable_latency = latency ? : U32_MAX;
> + clk->attributes = attributes;
> }
>
> ph->xops->xfer_put(ph, t);
> @@ -499,6 +503,10 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
> struct scmi_xfer *t;
> struct scmi_clock_set_rate *cfg;
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk = ci->clk + clk_id;
> +
> + if (clk->attributes & SCMI_CLOCK_SET_RATE_DENIED)
> + return -EACCES;
>
> ret = ph->xops->xfer_get_init(ph, CLOCK_RATE_SET, sizeof(*cfg), 0, &t);
> if (ret)
> @@ -585,6 +593,9 @@ scmi_clock_set_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
> if (parent_id >= clk->num_parents)
> return -EINVAL;
>
> + if (clk->attributes & SCMI_CLOCK_SET_PARENT_DENIED)
> + return -EACCES;
> +
> ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
> sizeof(*cfg), 0, &t);
> if (ret)
> @@ -668,6 +679,10 @@ static int scmi_clock_enable(const struct scmi_protocol_handle *ph, u32 clk_id,
> bool atomic)
> {
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk = ci->clk + clk_id;
> +
> + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return 0;
>
> return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
> NULL_OEM_TYPE, 0, atomic);
> @@ -677,6 +692,10 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
> bool atomic)
> {
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk = ci->clk + clk_id;
> +
> + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return 0;
>
> return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
> NULL_OEM_TYPE, 0, atomic);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index f2f05fb42d28..ddf5363c8cfd 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -58,6 +58,7 @@ struct scmi_clock_info {
> u64 step_size;
> } range;
> };
> + u32 attributes;
> int num_parents;
> u32 *parents;
> };
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V1] firmware: arm_scmi: clock: support clock denied flags
2023-10-31 12:27 [PATCH V1] firmware: arm_scmi: clock: support clock denied flags Peng Fan (OSS)
2023-10-31 12:30 ` Cristian Marussi
@ 2023-10-31 13:03 ` Cristian Marussi
2023-10-31 13:33 ` Peng Fan
1 sibling, 1 reply; 5+ messages in thread
From: Cristian Marussi @ 2023-10-31 13:03 UTC (permalink / raw)
To: Peng Fan (OSS)
Cc: sudeep.holla, linux-arm-kernel, linux-clk, linux-kernel,
ranjani.vaidyanathan, glen.wienecke, nitin.garg, chuck.cannon,
Souvik.Chakravarty, Peng Fan
On Tue, Oct 31, 2023 at 08:27:34PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> The firmware may export flags to indicate whether the clock
> is allowed to set rate, set parent, enable/disable from the Agent.
>
> If Agent is not allowed to enable/disable, directly return success.
... even though, this success on deny built-in policy could be
controversial...Have you tried the different approach of not
registerig the enable/prepare callbacks in clk-scmi instead ?
From the Clk framework code it seems could be handled gracefully.
(I have not tried though...)
Thanks,
Cristian
> If Agent is not allowed to set rate/parent, directly return -EACCES to
> avoid SCMI RPC calls.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V1:
> drop the changes in clock.c, add an attribute entry in clock info which
> may be easy for extending new flag.
>
> SPEC still not have such support, this is for discussion
>
> drivers/firmware/arm_scmi/clock.c | 19 +++++++++++++++++++
> include/linux/scmi_protocol.h | 1 +
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 42b81c181d68..fad4329a21fc 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -46,6 +46,9 @@ struct scmi_msg_resp_clock_attributes {
> #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) & BIT(30))
> #define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
> #define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
> +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15)
> +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14)
> +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13)
> u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> __le32 clock_enable_latency;
> };
> @@ -307,6 +310,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
> if (PROTOCOL_REV_MAJOR(version) >= 0x2)
> latency = le32_to_cpu(attr->clock_enable_latency);
> clk->enable_latency = latency ? : U32_MAX;
> + clk->attributes = attributes;
> }
>
> ph->xops->xfer_put(ph, t);
> @@ -499,6 +503,10 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
> struct scmi_xfer *t;
> struct scmi_clock_set_rate *cfg;
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk = ci->clk + clk_id;
> +
> + if (clk->attributes & SCMI_CLOCK_SET_RATE_DENIED)
> + return -EACCES;
>
> ret = ph->xops->xfer_get_init(ph, CLOCK_RATE_SET, sizeof(*cfg), 0, &t);
> if (ret)
> @@ -585,6 +593,9 @@ scmi_clock_set_parent(const struct scmi_protocol_handle *ph, u32 clk_id,
> if (parent_id >= clk->num_parents)
> return -EINVAL;
>
> + if (clk->attributes & SCMI_CLOCK_SET_PARENT_DENIED)
> + return -EACCES;
> +
> ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
> sizeof(*cfg), 0, &t);
> if (ret)
> @@ -668,6 +679,10 @@ static int scmi_clock_enable(const struct scmi_protocol_handle *ph, u32 clk_id,
> bool atomic)
> {
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk = ci->clk + clk_id;
> +
> + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return 0;
>
> return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
> NULL_OEM_TYPE, 0, atomic);
> @@ -677,6 +692,10 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id,
> bool atomic)
> {
> struct clock_info *ci = ph->get_priv(ph);
> + struct scmi_clock_info *clk = ci->clk + clk_id;
> +
> + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> + return 0;
>
> return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
> NULL_OEM_TYPE, 0, atomic);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index f2f05fb42d28..ddf5363c8cfd 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -58,6 +58,7 @@ struct scmi_clock_info {
> u64 step_size;
> } range;
> };
> + u32 attributes;
> int num_parents;
> u32 *parents;
> };
> --
> 2.37.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH V1] firmware: arm_scmi: clock: support clock denied flags
2023-10-31 13:03 ` Cristian Marussi
@ 2023-10-31 13:33 ` Peng Fan
2023-10-31 13:35 ` Cristian Marussi
0 siblings, 1 reply; 5+ messages in thread
From: Peng Fan @ 2023-10-31 13:33 UTC (permalink / raw)
To: Cristian Marussi, Peng Fan (OSS)
Cc: sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org,
linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org,
Ranjani Vaidyanathan, Glen G Wienecke, nitin.garg@nxp.com,
Chuck Cannon, Souvik.Chakravarty@arm.com
> Subject: Re: [PATCH V1] firmware: arm_scmi: clock: support clock denied
> flags
>
> On Tue, Oct 31, 2023 at 08:27:34PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > The firmware may export flags to indicate whether the clock is allowed
> > to set rate, set parent, enable/disable from the Agent.
> >
> > If Agent is not allowed to enable/disable, directly return success.
>
> ... even though, this success on deny built-in policy could be
> controversial..
Yeah.
.Have you tried the different approach of not registerig the
> enable/prepare callbacks in clk-scmi instead ?
Not yet, I could give a look. But not have much time
in the following week.
> From the Clk framework code it seems could be handled gracefully.
> (I have not tried though...)
If there is no enable/disable/prepare/unprepared hooks, the enable
counter or prepare counter would be used.
Thanks,
Peng.
>
> Thanks,
> Cristian
>
> > If Agent is not allowed to set rate/parent, directly return -EACCES to
> > avoid SCMI RPC calls.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V1:
> > drop the changes in clock.c, add an attribute entry in clock info
> > which may be easy for extending new flag.
> >
> > SPEC still not have such support, this is for discussion
> >
> > drivers/firmware/arm_scmi/clock.c | 19 +++++++++++++++++++
> > include/linux/scmi_protocol.h | 1 +
> > 2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c
> > b/drivers/firmware/arm_scmi/clock.c
> > index 42b81c181d68..fad4329a21fc 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -46,6 +46,9 @@ struct scmi_msg_resp_clock_attributes {
> > #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) &
> BIT(30))
> > #define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
> > #define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
> > +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15)
> > +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14)
> > +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13)
> > u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > __le32 clock_enable_latency;
> > };
> > @@ -307,6 +310,7 @@ static int scmi_clock_attributes_get(const struct
> scmi_protocol_handle *ph,
> > if (PROTOCOL_REV_MAJOR(version) >= 0x2)
> > latency = le32_to_cpu(attr->clock_enable_latency);
> > clk->enable_latency = latency ? : U32_MAX;
> > + clk->attributes = attributes;
> > }
> >
> > ph->xops->xfer_put(ph, t);
> > @@ -499,6 +503,10 @@ static int scmi_clock_rate_set(const struct
> scmi_protocol_handle *ph,
> > struct scmi_xfer *t;
> > struct scmi_clock_set_rate *cfg;
> > struct clock_info *ci = ph->get_priv(ph);
> > + struct scmi_clock_info *clk = ci->clk + clk_id;
> > +
> > + if (clk->attributes & SCMI_CLOCK_SET_RATE_DENIED)
> > + return -EACCES;
> >
> > ret = ph->xops->xfer_get_init(ph, CLOCK_RATE_SET, sizeof(*cfg), 0,
> &t);
> > if (ret)
> > @@ -585,6 +593,9 @@ scmi_clock_set_parent(const struct
> scmi_protocol_handle *ph, u32 clk_id,
> > if (parent_id >= clk->num_parents)
> > return -EINVAL;
> >
> > + if (clk->attributes & SCMI_CLOCK_SET_PARENT_DENIED)
> > + return -EACCES;
> > +
> > ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
> > sizeof(*cfg), 0, &t);
> > if (ret)
> > @@ -668,6 +679,10 @@ static int scmi_clock_enable(const struct
> scmi_protocol_handle *ph, u32 clk_id,
> > bool atomic)
> > {
> > struct clock_info *ci = ph->get_priv(ph);
> > + struct scmi_clock_info *clk = ci->clk + clk_id;
> > +
> > + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> > + return 0;
> >
> > return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
> > NULL_OEM_TYPE, 0, atomic);
> > @@ -677,6 +692,10 @@ static int scmi_clock_disable(const struct
> scmi_protocol_handle *ph, u32 clk_id,
> > bool atomic)
> > {
> > struct clock_info *ci = ph->get_priv(ph);
> > + struct scmi_clock_info *clk = ci->clk + clk_id;
> > +
> > + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> > + return 0;
> >
> > return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
> > NULL_OEM_TYPE, 0, atomic);
> > diff --git a/include/linux/scmi_protocol.h
> > b/include/linux/scmi_protocol.h index f2f05fb42d28..ddf5363c8cfd
> > 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -58,6 +58,7 @@ struct scmi_clock_info {
> > u64 step_size;
> > } range;
> > };
> > + u32 attributes;
> > int num_parents;
> > u32 *parents;
> > };
> > --
> > 2.37.1
> >
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH V1] firmware: arm_scmi: clock: support clock denied flags
2023-10-31 13:33 ` Peng Fan
@ 2023-10-31 13:35 ` Cristian Marussi
0 siblings, 0 replies; 5+ messages in thread
From: Cristian Marussi @ 2023-10-31 13:35 UTC (permalink / raw)
To: Peng Fan
Cc: Peng Fan (OSS), sudeep.holla@arm.com,
linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
linux-kernel@vger.kernel.org, Ranjani Vaidyanathan,
Glen G Wienecke, nitin.garg@nxp.com, Chuck Cannon,
Souvik.Chakravarty@arm.com
On Tue, Oct 31, 2023 at 01:33:17PM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH V1] firmware: arm_scmi: clock: support clock denied
> > flags
> >
> > On Tue, Oct 31, 2023 at 08:27:34PM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > >
> > > The firmware may export flags to indicate whether the clock is allowed
> > > to set rate, set parent, enable/disable from the Agent.
> > >
> > > If Agent is not allowed to enable/disable, directly return success.
> >
> > ... even though, this success on deny built-in policy could be
> > controversial..
> Yeah.
>
> .Have you tried the different approach of not registerig the
> > enable/prepare callbacks in clk-scmi instead ?
>
> Not yet, I could give a look. But not have much time
> in the following week.
>
>
> > From the Clk framework code it seems could be handled gracefully.
> > (I have not tried though...)
>
> If there is no enable/disable/prepare/unprepared hooks, the enable
> counter or prepare counter would be used.
>
Yes this is what I could understand.
Thanks,
Cristian
> Thanks,
> Peng.
> >
> > Thanks,
> > Cristian
> >
> > > If Agent is not allowed to set rate/parent, directly return -EACCES to
> > > avoid SCMI RPC calls.
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >
> > > V1:
> > > drop the changes in clock.c, add an attribute entry in clock info
> > > which may be easy for extending new flag.
> > >
> > > SPEC still not have such support, this is for discussion
> > >
> > > drivers/firmware/arm_scmi/clock.c | 19 +++++++++++++++++++
> > > include/linux/scmi_protocol.h | 1 +
> > > 2 files changed, 20 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index 42b81c181d68..fad4329a21fc 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -46,6 +46,9 @@ struct scmi_msg_resp_clock_attributes {
> > > #define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x) ((x) &
> > BIT(30))
> > > #define SUPPORTS_EXTENDED_NAMES(x) ((x) & BIT(29))
> > > #define SUPPORTS_PARENT_CLOCK(x) ((x) & BIT(28))
> > > +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15)
> > > +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14)
> > > +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13)
> > > u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> > > __le32 clock_enable_latency;
> > > };
> > > @@ -307,6 +310,7 @@ static int scmi_clock_attributes_get(const struct
> > scmi_protocol_handle *ph,
> > > if (PROTOCOL_REV_MAJOR(version) >= 0x2)
> > > latency = le32_to_cpu(attr->clock_enable_latency);
> > > clk->enable_latency = latency ? : U32_MAX;
> > > + clk->attributes = attributes;
> > > }
> > >
> > > ph->xops->xfer_put(ph, t);
> > > @@ -499,6 +503,10 @@ static int scmi_clock_rate_set(const struct
> > scmi_protocol_handle *ph,
> > > struct scmi_xfer *t;
> > > struct scmi_clock_set_rate *cfg;
> > > struct clock_info *ci = ph->get_priv(ph);
> > > + struct scmi_clock_info *clk = ci->clk + clk_id;
> > > +
> > > + if (clk->attributes & SCMI_CLOCK_SET_RATE_DENIED)
> > > + return -EACCES;
> > >
> > > ret = ph->xops->xfer_get_init(ph, CLOCK_RATE_SET, sizeof(*cfg), 0,
> > &t);
> > > if (ret)
> > > @@ -585,6 +593,9 @@ scmi_clock_set_parent(const struct
> > scmi_protocol_handle *ph, u32 clk_id,
> > > if (parent_id >= clk->num_parents)
> > > return -EINVAL;
> > >
> > > + if (clk->attributes & SCMI_CLOCK_SET_PARENT_DENIED)
> > > + return -EACCES;
> > > +
> > > ret = ph->xops->xfer_get_init(ph, CLOCK_PARENT_SET,
> > > sizeof(*cfg), 0, &t);
> > > if (ret)
> > > @@ -668,6 +679,10 @@ static int scmi_clock_enable(const struct
> > scmi_protocol_handle *ph, u32 clk_id,
> > > bool atomic)
> > > {
> > > struct clock_info *ci = ph->get_priv(ph);
> > > + struct scmi_clock_info *clk = ci->clk + clk_id;
> > > +
> > > + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> > > + return 0;
> > >
> > > return ci->clock_config_set(ph, clk_id, CLK_STATE_ENABLE,
> > > NULL_OEM_TYPE, 0, atomic);
> > > @@ -677,6 +692,10 @@ static int scmi_clock_disable(const struct
> > scmi_protocol_handle *ph, u32 clk_id,
> > > bool atomic)
> > > {
> > > struct clock_info *ci = ph->get_priv(ph);
> > > + struct scmi_clock_info *clk = ci->clk + clk_id;
> > > +
> > > + if (clk->attributes & SCMI_CLOCK_SET_ENABLE_DENIED)
> > > + return 0;
> > >
> > > return ci->clock_config_set(ph, clk_id, CLK_STATE_DISABLE,
> > > NULL_OEM_TYPE, 0, atomic);
> > > diff --git a/include/linux/scmi_protocol.h
> > > b/include/linux/scmi_protocol.h index f2f05fb42d28..ddf5363c8cfd
> > > 100644
> > > --- a/include/linux/scmi_protocol.h
> > > +++ b/include/linux/scmi_protocol.h
> > > @@ -58,6 +58,7 @@ struct scmi_clock_info {
> > > u64 step_size;
> > > } range;
> > > };
> > > + u32 attributes;
> > > int num_parents;
> > > u32 *parents;
> > > };
> > > --
> > > 2.37.1
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-31 13:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-31 12:27 [PATCH V1] firmware: arm_scmi: clock: support clock denied flags Peng Fan (OSS)
2023-10-31 12:30 ` Cristian Marussi
2023-10-31 13:03 ` Cristian Marussi
2023-10-31 13:33 ` Peng Fan
2023-10-31 13:35 ` Cristian Marussi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).