RE: [RFC] firmware: arm_scmi: support clock denied attributes

[Peng Fan]

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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> 
> 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
> >