RE: [EXT] Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribute support

[Ranjani Vaidyanathan]

>From what I see SCMI clock protocol could benefit from an attribute for the clock that describes what operations are possible on a clock. 
There are many bus clocks that only the SCMI server manages and the error code DENIED should be handled gracefully by the agent.

In the case of Linux, perhaps this should be handled by the SCMI clock driver, instead of allowing the error to propagate up the Linux clock framework? It seems strange that the SCMI server should swallow the error (silently fail) only for certain agents. I would think this would make debug quite difficult and having a "DENIED" error code not very useful.

Regards,
Ranjani

-----Original Message-----
From: Sudeep Holla <sudeep.holla@xxxxxxx> 
Sent: Tuesday, October 10, 2023 4:35 AM
To: Cristian Marussi <cristian.marussi@xxxxxxx>
Cc: Peng Fan (OSS) <peng.fan@xxxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Ranjani Vaidyanathan <ranjani.vaidyanathan@xxxxxxx>; souvik.chakravarty@xxxxxxx; Glen G Wienecke <glen.wienecke@xxxxxxx>; Peng Fan <peng.fan@xxxxxxx>
Subject: [EXT] Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribute support

Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button


On Tue, Oct 10, 2023 at 10:22:03AM +0100, Cristian Marussi wrote:
> On Tue, Oct 10, 2023 at 10:12:23AM +0100, Sudeep Holla wrote:
> > On Tue, Oct 10, 2023 at 10:29:11AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@xxxxxxx>
> > >
> > > There are clocks:
> > >  system critical, not allow linux to disable, change rate  allow 
> > > linux to get rate, because some periphals will use the frequency  
> > > to configure periphals.
> > >
> > >  So introduce an attribute to indicated FIXED clock
> > >
> > > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > > ---
> > >  drivers/clk/clk-scmi.c            | 6 ++++++
> > >  drivers/firmware/arm_scmi/clock.c | 5 ++++-
> > >  include/linux/scmi_protocol.h     | 1 +
> > >  3 files changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c index 
> > > 8cbe24789c24..a539a35bd45a 100644
> > > --- a/drivers/clk/clk-scmi.c
> > > +++ b/drivers/clk/clk-scmi.c
> > > @@ -182,6 +182,10 @@ static const struct clk_ops scmi_clk_ops = {
> > >   .determine_rate = scmi_clk_determine_rate,  };
> > >
> > > +static const struct clk_ops scmi_fixed_rate_clk_ops = {  
> > > +.recalc_rate = scmi_clk_recalc_rate, };
> > > +
> > >  static const struct clk_ops scmi_atomic_clk_ops = {
> > >   .recalc_rate = scmi_clk_recalc_rate,
> > >   .round_rate = scmi_clk_round_rate, @@ -293,6 +297,8 @@ static 
> > > int scmi_clocks_probe(struct scmi_device *sdev)
> > >           if (is_atomic &&
> > >               sclk->info->enable_latency <= atomic_threshold)
> > >                   scmi_ops = &scmi_atomic_clk_ops;
> > > +         else if (sclk->info->rate_fixed)
> > > +                 scmi_ops = &scmi_fixed_rate_clk_ops;
> > >           else
> > >                   scmi_ops = &scmi_clk_ops;
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c 
> > > b/drivers/firmware/arm_scmi/clock.c
> > > index ddaef34cd88b..8c52db539e54 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -46,6 +46,7 @@ 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 SUPPORTS_FIXED_RATE_CLOCK(x)             ((x) & BIT(27))
> >
> > I don't see this in the specification, am I missing something ?
> >
> > And why do we need it. Can't this be discrete clock with only one 
> > clock rate ? Or step clock with both lowest and highest the same and step being 0.
> > At-least I don't see the need to change the spec for this and hence 
> > no need to assign any attribute bit-field to represent the same.
> >
>
> No this is not in the spec, it would require a spec change.
>
> My understanding is that they have clocks that CAN have more than one 
> rate BUT such clock cannot be changed by Linux, only other agents can 
> enable/disable/set_rate BUT they still want to be able to query the 
> current rate for configuration purposes.
>

Fair enough. As I mentioned to Peng on the other thread, it is *not a fixed* clock. It is read only for this agent.

--
Regards,
Sudeep