| Message ID | 20231026034125.1823954-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] firmware: arm_scmi: support clock denied attributes | expand |
On Thu, Oct 26, 2023 at 11:41:25AM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > This is not in SPEC and is just defined by NXP. This patch is > to start the discussion to start include them in SPEC. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- Hi Peng, thanks for validating this scenario. [CC Souvik] So at the end, you are returning -EACCESS anyway, it is just that you are avoiding to send any SCMI message at all if the flag reports that you cannot touch this and you will get back a DENY. Does this solve your usecase where your other drivers (callers) were failing to probe due to such error being reported from the server ? From our offline discussions my understanding was that, beside un-needed SCMI msg exchanges, your main issue was receiving a DENY error from the server when trying to modify a clock, how does this solves that ? Basically You are just returning the same error from the clk driver, just avoiding (rightly so) needless SCMI exchanges. (..in your last RFC patch you attempt was indeed different, to refrain from registering any clk framework callbacks at all for untocuhable clocks...) Does this work with upstream drivers, or just with some downstream solution properly crafted to handle the EACCESS ? Anyway IMO, these changes in this scenario are certainly valuable in general since they avoid needless exchanges with the server around clocks that we know upfront we cannot touch. BUT, hvaing said that, if this series goes further as it is and the spec is changed accordingly, please move all of this logic inside the protocol layer: there is no reason to change the clk-scmi driver at all for this. You can just check that same new flags (that you set in clk discovery) upfront inside the related clock operations in drivers/firmware/arm/clock.c and just return EACCESS from there avoiding any SCMI exchanges if the flags are set. This way you dont either need to re-define and expose such new flags in scmi_protocol.h, it can just all handled inside the SCMI protocol layer. Thank, Cristian > drivers/clk/clk-scmi.c | 39 +++++++++++++++++++++++++------ > drivers/firmware/arm_scmi/clock.c | 9 +++++++ > include/linux/scmi_protocol.h | 4 ++++ > 3 files changed, 45 insertions(+), 7 deletions(-) > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > index 8cbe24789c24..303f8a8ec8e0 100644 > --- a/drivers/clk/clk-scmi.c > +++ b/drivers/clk/clk-scmi.c > @@ -75,15 +75,13 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, > unsigned long parent_rate) > { > struct scmi_clk *clk = to_scmi_clk(hw); > + const struct scmi_clock_info *info = clk->info; > + u64 rate1 = 0; > > - return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > -} > - > -static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) > -{ > - struct scmi_clk *clk = to_scmi_clk(hw); > + if (info->flags & SCMI_CLOCK_SET_RATE_DENIED) > + return -EACCES; > > - return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); > + return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > } > > static u8 scmi_clk_get_parent(struct clk_hw *hw) > @@ -107,6 +105,17 @@ static u8 scmi_clk_get_parent(struct clk_hw *hw) > return p_idx; > } > > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + const struct scmi_clock_info *info = clk->info; > + > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > + return -EACCES; > + > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); > +} > + > static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) > { > /* > @@ -119,6 +128,10 @@ static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *r > static int scmi_clk_enable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > + const struct scmi_clock_info *info = clk->info; > + > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > + return 0; > > return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); > } > @@ -126,6 +139,10 @@ static int scmi_clk_enable(struct clk_hw *hw) > static void scmi_clk_disable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > + const struct scmi_clock_info *info = clk->info; > + > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > + return; > > scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC); > } > @@ -133,6 +150,10 @@ static void scmi_clk_disable(struct clk_hw *hw) > static int scmi_clk_atomic_enable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > + const struct scmi_clock_info *info = clk->info; > + > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > + return 0; > > return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC); > } > @@ -140,6 +161,10 @@ static int scmi_clk_atomic_enable(struct clk_hw *hw) > static void scmi_clk_atomic_disable(struct clk_hw *hw) > { > struct scmi_clk *clk = to_scmi_clk(hw); > + const struct scmi_clock_info *info = clk->info; > + > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > + return; > > scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC); > } > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c > index 42b81c181d68..1a62e3b82d34 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 SETS_ENABLE_DENIED(x) ((x) & BIT(15)) > +#define SETS_RATE_DENIED(x) ((x) & BIT(14)) > +#define SETS_PARENT_DENIED(x) ((x) & BIT(13)) > u8 name[SCMI_SHORT_NAME_MAX_SIZE]; > __le32 clock_enable_latency; > }; > @@ -327,6 +330,12 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph, > clk->rate_change_requested_notifications = true; > if (SUPPORTS_PARENT_CLOCK(attributes)) > scmi_clock_possible_parents(ph, clk_id, clk); > + if (SETS_PARENT_DENIED(attributes)) > + clk->flags |= SCMI_CLOCK_SET_PARENT_DENIED; > + if (SETS_RATE_DENIED(attributes)) > + clk->flags |= SCMI_CLOCK_SET_RATE_DENIED; > + if (SETS_ENABLE_DENIED(attributes)) > + clk->flags |= SCMI_CLOCK_SET_ENABLE_DENIED; > } > > return ret; > diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h > index f2f05fb42d28..71911dcd8117 100644 > --- a/include/linux/scmi_protocol.h > +++ b/include/linux/scmi_protocol.h > @@ -41,12 +41,16 @@ struct scmi_revision_info { > char sub_vendor_id[SCMI_SHORT_NAME_MAX_SIZE]; > }; > > +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13) > +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14) > +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15) > struct scmi_clock_info { > char name[SCMI_MAX_STR_SIZE]; > unsigned int enable_latency; > bool rate_discrete; > bool rate_changed_notifications; > bool rate_change_requested_notifications; > + unsigned int flags; > union { > struct { > int num_rates; > -- > 2.37.1 >
Hi Cristian > Subject: Re: [RFC] firmware: arm_scmi: support clock denied attributes > > On Thu, Oct 26, 2023 at 11:41:25AM +0800, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > This is not in SPEC and is just defined by NXP. This patch is to start > > the discussion to start include them in SPEC. > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > Hi Peng, > > thanks for validating this scenario. > > [CC Souvik] > > So at the end, you are returning -EACCESS anyway, it is just that you are > avoiding to send any SCMI message at all if the flag reports that you cannot > touch this and you will get back a DENY. Yes, except enable/disable, rate/parent both return DENY. > > Does this solve your usecase where your other drivers (callers) were failing to > probe due to such error being reported from the server ? We removed the assigned-clock-rates and assigned-clock-parents from device tree nodes to avoid failure. The peripheral driver call clk_enable/disable, since it return success, so issue resolved. > > From our offline discussions my understanding was that, beside un-needed > SCMI msg exchanges, your main issue was receiving a DENY error from the > server when trying to modify a clock, how does this solves that ? See above, removed the assigned-clock-parents and assigned-clock-rates. The scmi firmware configured the parent and rate ready. > Basically You are just returning the same error from the clk driver, just > avoiding (rightly so) needless SCMI exchanges. > (..in your last RFC patch you attempt was indeed different, to refrain from > registering any clk framework callbacks at all for untocuhable > clocks...) The last RFC was to register clocks as fixed clock, so no parent/rate call needed. But actually we may need get new rate because the scmi firmware could update rate per device performance level setting. > > Does this work with upstream drivers, or just with some downstream solution > properly crafted to handle the EACCESS ? The patch tested in downstream repo, 6.6. > > Anyway IMO, these changes in this scenario are certainly valuable in general > since they avoid needless exchanges with the server around clocks that we > know upfront we cannot touch. > > BUT, hvaing said that, if this series goes further as it is and the spec is > changed accordingly, please move all of this logic inside the protocol layer: > there is no reason to change the clk-scmi driver at all for this. > > You can just check that same new flags (that you set in clk discovery) upfront > inside the related clock operations in drivers/firmware/arm/clock.c and just > return EACCESS from there avoiding any SCMI exchanges if the flags are set. > > This way you dont either need to re-define and expose such new flags in > scmi_protocol.h, it can just all handled inside the SCMI protocol layer. Thanks for your suggestions, I will give a look and prepare a new version patch. Thanks, Peng. > > Thank, > Cristian > > > drivers/clk/clk-scmi.c | 39 +++++++++++++++++++++++++------ > > drivers/firmware/arm_scmi/clock.c | 9 +++++++ > > include/linux/scmi_protocol.h | 4 ++++ > > 3 files changed, 45 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index > > 8cbe24789c24..303f8a8ec8e0 100644 > > --- a/drivers/clk/clk-scmi.c > > +++ b/drivers/clk/clk-scmi.c > > @@ -75,15 +75,13 @@ static int scmi_clk_set_rate(struct clk_hw *hw, > unsigned long rate, > > unsigned long parent_rate) > > { > > struct scmi_clk *clk = to_scmi_clk(hw); > > + const struct scmi_clock_info *info = clk->info; > > + u64 rate1 = 0; > > > > - return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > > -} > > - > > -static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) -{ > > - struct scmi_clk *clk = to_scmi_clk(hw); > > + if (info->flags & SCMI_CLOCK_SET_RATE_DENIED) > > + return -EACCES; > > > > - return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, > parent_index); > > + return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); > > } > > > > static u8 scmi_clk_get_parent(struct clk_hw *hw) @@ -107,6 +105,17 @@ > > static u8 scmi_clk_get_parent(struct clk_hw *hw) > > return p_idx; > > } > > > > +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) { > > + struct scmi_clk *clk = to_scmi_clk(hw); > > + const struct scmi_clock_info *info = clk->info; > > + > > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > > + return -EACCES; > > + > > + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, > > +parent_index); } > > + > > static int scmi_clk_determine_rate(struct clk_hw *hw, struct > > clk_rate_request *req) { > > /* > > @@ -119,6 +128,10 @@ static int scmi_clk_determine_rate(struct clk_hw > > *hw, struct clk_rate_request *r static int scmi_clk_enable(struct > > clk_hw *hw) { > > struct scmi_clk *clk = to_scmi_clk(hw); > > + const struct scmi_clock_info *info = clk->info; > > + > > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > > + return 0; > > > > return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); } > > @@ -126,6 +139,10 @@ static int scmi_clk_enable(struct clk_hw *hw) > > static void scmi_clk_disable(struct clk_hw *hw) { > > struct scmi_clk *clk = to_scmi_clk(hw); > > + const struct scmi_clock_info *info = clk->info; > > + > > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > > + return; > > > > scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC); } @@ > > -133,6 +150,10 @@ static void scmi_clk_disable(struct clk_hw *hw) > > static int scmi_clk_atomic_enable(struct clk_hw *hw) { > > struct scmi_clk *clk = to_scmi_clk(hw); > > + const struct scmi_clock_info *info = clk->info; > > + > > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > > + return 0; > > > > return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC); } @@ > > -140,6 +161,10 @@ static int scmi_clk_atomic_enable(struct clk_hw *hw) > > static void scmi_clk_atomic_disable(struct clk_hw *hw) { > > struct scmi_clk *clk = to_scmi_clk(hw); > > + const struct scmi_clock_info *info = clk->info; > > + > > + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) > > + return; > > > > scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC); } diff --git > > a/drivers/firmware/arm_scmi/clock.c > > b/drivers/firmware/arm_scmi/clock.c > > index 42b81c181d68..1a62e3b82d34 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 SETS_ENABLE_DENIED(x) ((x) & BIT(15)) > > +#define SETS_RATE_DENIED(x) ((x) & BIT(14)) > > +#define SETS_PARENT_DENIED(x) ((x) & BIT(13)) > > u8 name[SCMI_SHORT_NAME_MAX_SIZE]; > > __le32 clock_enable_latency; > > }; > > @@ -327,6 +330,12 @@ static int scmi_clock_attributes_get(const struct > scmi_protocol_handle *ph, > > clk->rate_change_requested_notifications = true; > > if (SUPPORTS_PARENT_CLOCK(attributes)) > > scmi_clock_possible_parents(ph, clk_id, clk); > > + if (SETS_PARENT_DENIED(attributes)) > > + clk->flags |= SCMI_CLOCK_SET_PARENT_DENIED; > > + if (SETS_RATE_DENIED(attributes)) > > + clk->flags |= SCMI_CLOCK_SET_RATE_DENIED; > > + if (SETS_ENABLE_DENIED(attributes)) > > + clk->flags |= SCMI_CLOCK_SET_ENABLE_DENIED; > > } > > > > return ret; > > diff --git a/include/linux/scmi_protocol.h > > b/include/linux/scmi_protocol.h index f2f05fb42d28..71911dcd8117 > > 100644 > > --- a/include/linux/scmi_protocol.h > > +++ b/include/linux/scmi_protocol.h > > @@ -41,12 +41,16 @@ struct scmi_revision_info { > > char sub_vendor_id[SCMI_SHORT_NAME_MAX_SIZE]; > > }; > > > > +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13) > > +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14) > > +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15) > > struct scmi_clock_info { > > char name[SCMI_MAX_STR_SIZE]; > > unsigned int enable_latency; > > bool rate_discrete; > > bool rate_changed_notifications; > > bool rate_change_requested_notifications; > > + unsigned int flags; > > union { > > struct { > > int num_rates; > > -- > > 2.37.1 > >
diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 8cbe24789c24..303f8a8ec8e0 100644 --- a/drivers/clk/clk-scmi.c +++ b/drivers/clk/clk-scmi.c @@ -75,15 +75,13 @@ static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate) { struct scmi_clk *clk = to_scmi_clk(hw); + const struct scmi_clock_info *info = clk->info; + u64 rate1 = 0; - return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); -} - -static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) -{ - struct scmi_clk *clk = to_scmi_clk(hw); + if (info->flags & SCMI_CLOCK_SET_RATE_DENIED) + return -EACCES; - return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); + return scmi_proto_clk_ops->rate_set(clk->ph, clk->id, rate); } static u8 scmi_clk_get_parent(struct clk_hw *hw) @@ -107,6 +105,17 @@ static u8 scmi_clk_get_parent(struct clk_hw *hw) return p_idx; } +static int scmi_clk_set_parent(struct clk_hw *hw, u8 parent_index) +{ + struct scmi_clk *clk = to_scmi_clk(hw); + const struct scmi_clock_info *info = clk->info; + + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) + return -EACCES; + + return scmi_proto_clk_ops->parent_set(clk->ph, clk->id, parent_index); +} + static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *req) { /* @@ -119,6 +128,10 @@ static int scmi_clk_determine_rate(struct clk_hw *hw, struct clk_rate_request *r static int scmi_clk_enable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); + const struct scmi_clock_info *info = clk->info; + + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) + return 0; return scmi_proto_clk_ops->enable(clk->ph, clk->id, NOT_ATOMIC); } @@ -126,6 +139,10 @@ static int scmi_clk_enable(struct clk_hw *hw) static void scmi_clk_disable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); + const struct scmi_clock_info *info = clk->info; + + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) + return; scmi_proto_clk_ops->disable(clk->ph, clk->id, NOT_ATOMIC); } @@ -133,6 +150,10 @@ static void scmi_clk_disable(struct clk_hw *hw) static int scmi_clk_atomic_enable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); + const struct scmi_clock_info *info = clk->info; + + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) + return 0; return scmi_proto_clk_ops->enable(clk->ph, clk->id, ATOMIC); } @@ -140,6 +161,10 @@ static int scmi_clk_atomic_enable(struct clk_hw *hw) static void scmi_clk_atomic_disable(struct clk_hw *hw) { struct scmi_clk *clk = to_scmi_clk(hw); + const struct scmi_clock_info *info = clk->info; + + if (info->flags & SCMI_CLOCK_SET_ENABLE_DENIED) + return; scmi_proto_clk_ops->disable(clk->ph, clk->id, ATOMIC); } diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index 42b81c181d68..1a62e3b82d34 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 SETS_ENABLE_DENIED(x) ((x) & BIT(15)) +#define SETS_RATE_DENIED(x) ((x) & BIT(14)) +#define SETS_PARENT_DENIED(x) ((x) & BIT(13)) u8 name[SCMI_SHORT_NAME_MAX_SIZE]; __le32 clock_enable_latency; }; @@ -327,6 +330,12 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph, clk->rate_change_requested_notifications = true; if (SUPPORTS_PARENT_CLOCK(attributes)) scmi_clock_possible_parents(ph, clk_id, clk); + if (SETS_PARENT_DENIED(attributes)) + clk->flags |= SCMI_CLOCK_SET_PARENT_DENIED; + if (SETS_RATE_DENIED(attributes)) + clk->flags |= SCMI_CLOCK_SET_RATE_DENIED; + if (SETS_ENABLE_DENIED(attributes)) + clk->flags |= SCMI_CLOCK_SET_ENABLE_DENIED; } return ret; diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index f2f05fb42d28..71911dcd8117 100644 --- a/include/linux/scmi_protocol.h +++ b/include/linux/scmi_protocol.h @@ -41,12 +41,16 @@ struct scmi_revision_info { char sub_vendor_id[SCMI_SHORT_NAME_MAX_SIZE]; }; +#define SCMI_CLOCK_SET_PARENT_DENIED BIT(13) +#define SCMI_CLOCK_SET_RATE_DENIED BIT(14) +#define SCMI_CLOCK_SET_ENABLE_DENIED BIT(15) struct scmi_clock_info { char name[SCMI_MAX_STR_SIZE]; unsigned int enable_latency; bool rate_discrete; bool rate_changed_notifications; bool rate_change_requested_notifications; + unsigned int flags; union { struct { int num_rates;