Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
From:   Ranjani Vaidyanathan <ranjani.vaidyanathan@nxp.com>
To:     Sudeep Holla <sudeep.holla@arm.com>,
        Cristian Marussi <cristian.marussi@arm.com>
CC:     "Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "souvik.chakravarty@arm.com" <souvik.chakravarty@arm.com>,
        Glen G Wienecke <glen.wienecke@nxp.com>,
        Peng Fan <peng.fan@nxp.com>
Subject: RE: [EXT] Re: [RFC] firmware: arm_scmi: clock: add fixed clock
 attribute support
Thread-Topic: [EXT] Re: [RFC] firmware: arm_scmi: clock: add fixed clock
 attribute support
Thread-Index: AQHZ+yDsjwKkBHlR5kuYildRcF7ttbBCvYCAgAACs4CAAAOpgIABD3+g
Date:   Wed, 11 Oct 2023 03:54:59 +0000
Message-ID: <PA4PR04MB94859C7729B19C8B88541F8692CCA@PA4PR04MB9485.eurprd04.prod.outlook.com>
References: <20231010022911.4106863-1-peng.fan@oss.nxp.com>
 <20231010091223.rvcyrgbjcrmjzmvp@bogus> <ZSUXu65bOYVG689E@pluto>
 <20231010093509.ddy75og4jd72n6cq@bogus>
In-Reply-To: <20231010093509.ddy75og4jd72n6cq@bogus>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?DJZIRw4cNlCAuQ2UBCy2itds4qOC9nTlGuA8QCBXNVzA3eZBCFa1YPHG2oQT?=
 =?us-ascii?Q?TUZ7/5zRJDx28F1PTCjCELfe4GzKodOk/AkzElsrNKIlLst6QO6qf1eqwHsp?=
 =?us-ascii?Q?v/tLf5hzNsAsL6PCv6mgD8CmgM1r5iKcK0BuqZ+iGRna3jAE3ryBuLnMUbeE?=
 =?us-ascii?Q?iifESW6zkHrtC/3mT9RDqDri+NlQg2z8x4GXucg6PH66a0MuwZ1X9JcuCcn+?=
 =?us-ascii?Q?ZDsD1TDzxWhR/13BmkA1fJq52TsKXeAMJfLH1cHCOBF+qZxR1FNZxFtCfhJi?=
 =?us-ascii?Q?lSy7hpvuGua0qltudr9Ngv3yYUJNXEBzQIR6gKI0Na0mN9qBRGDCIPHwyako?=
 =?us-ascii?Q?zOL2k72JnwP5ZCnlurSa36s8V+1MJpGAp5il5N89GlatkPpnoVS7Nq4DGLgU?=
 =?us-ascii?Q?zp69dVEwiXJd+gWVrdwXVFee8zzsyXVRKarFh0puVIuiEl+K9FgrY1UoqvL8?=
 =?us-ascii?Q?q6YTrPLyq9mBklw+MIGKWBsb2mjWz4WWzKj2KPJkmdIOKldAacKQlLcDyOUi?=
 =?us-ascii?Q?h0BPJxchESNic2NItl5spzH3uQWZoHdxg6Esvve/3cfRfu8SBNjxBn/0JfK5?=
 =?us-ascii?Q?eUOkTeqBKlIWCHKQc18Q5ctuz55SpzAssMFPsnKlI9y5/xEeWf7vSNR6eqVr?=
 =?us-ascii?Q?8ou8QmSQHgNr7V20AZL3GePM3Zcj7GP7BGAGqvvUNepiiGWY2UlLkO7oSG+Z?=
 =?us-ascii?Q?gkZPyKdvBs9UvlHXlllutbbCQ3A2W0XSue1ObFY1mLaocm0LKfb0b62NabE4?=
 =?us-ascii?Q?lbCyLJMNgA9puQwFzn05MaRuT+KaF4lORPAi459rupciwKXdy1IcxVCTYbWL?=
 =?us-ascii?Q?Ohg8Z/IPejOrgS2dN1mS+VuaNqcSBAhYNnqAkBVfR9TDFJcaPTIAwrRqAcQB?=
 =?us-ascii?Q?Lq93ZKWoz5rgXCOGmX4F3VE2xxpD8mTpfTJPUgnhU2cv3DF4KKptNJ8cJiGA?=
 =?us-ascii?Q?C36jCpozOZxjVoNvSj18cPO+YSrkLSQOyPMxzwPUOVhVJcb9vU0gOx0N3M97?=
 =?us-ascii?Q?U6z6lyiyjlaSyJqaqcjly6Er2awryAslfjt7Yse7pSzlYtlXdGSNTPDUeFRK?=
 =?us-ascii?Q?Vxqod2PD8b1UoSisCxJKnIQiGQEwOWQmTscmgq+f2pDnnNXFoYetQWqa6oF1?=
 =?us-ascii?Q?AYJQUejeiJiqmT6QFtjpIE4DO/B5/of7CFPdoMkrZl7c+oHtDaoI1roNnJT7?=
 =?us-ascii?Q?OttcTty7Scv1tsBsQYNXw2Wz/5vCq9HSca+ofF06XFIwGhviXkvy7cYA67Js?=
 =?us-ascii?Q?7+ECMnCE6Lkc+ULq50i63ozfgbU0Ft3pvu9uQ1Trs883qri+8PDQragepAga?=
 =?us-ascii?Q?e4Q5BXVspfTm/WU1kCXuF5zgUbOfiNXvtful6yFBk6GSV1r0NA5TfzaQW7yJ?=
 =?us-ascii?Q?AV3h2GsjKL+5/foFys8664QFfKm6rCG4biEFNMy6JwiPnHyeXIPYFZVEPJZX?=
 =?us-ascii?Q?MJCSpO8JjARmGawRdN4dfm1OAx1wmtNziCKDz2CA6Irqfy5tCwSO5Q5TRKfb?=
 =?us-ascii?Q?JCR8HLsuSQCSWF4YyqIjsXaG1f+dbls/DwIvUmhUNSD2rlhAM9HhTsM5Hjo1?=
 =?us-ascii?Q?BXLYKa42lb7WqkdkobcfifGVFqPVvUNSCrbtzhJNT4Lw8GqmDQ4CHFL9fotZ?=
 =?us-ascii?Q?UA=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PA4PR04MB9485.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 86be3a1b-f3bd-4137-e24c-08dbca0dd6ef
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Oct 2023 03:54:59.2252
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: QpuMwpNlccJEXQWPSEM03bR+qTWgyS8h9iwzlnctdkHcM40mgmnxtCaP4NL2rvlDZJOCBA8nIpW6uQm1MT/dY0pZCGpnuj5wiLReCaa2nS8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR04MB7593
X-Spam-Status: No, score=2.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on groat.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (groat.vger.email [0.0.0.0]); Tue, 10 Oct 2023 20:55:37 -0700 (PDT)
X-Spam-Level: **

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

In the case of Linux, perhaps this should be handled by the SCMI clock driv=
er, 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 di=
fficult and having a "DENIED" error code not very useful.

Regards,
Ranjani

-----Original Message-----
From: Sudeep Holla <sudeep.holla@arm.com>=20
Sent: Tuesday, October 10, 2023 4:35 AM
To: Cristian Marussi <cristian.marussi@arm.com>
Cc: Peng Fan (OSS) <peng.fan@oss.nxp.com>; linux-arm-kernel@lists.infradead=
.org; linux-clk@vger.kernel.org; linux-kernel@vger.kernel.org; Ranjani Vaid=
yanathan <ranjani.vaidyanathan@nxp.com>; souvik.chakravarty@arm.com; Glen G=
 Wienecke <glen.wienecke@nxp.com>; Peng Fan <peng.fan@nxp.com>
Subject: [EXT] Re: [RFC] firmware: arm_scmi: clock: add fixed clock attribu=
te 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 t=
his 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@nxp.com>
> > >
> > > There are clocks:
> > >  system critical, not allow linux to disable, change rate  allow=20
> > > linux to get rate, because some periphals will use the frequency =20
> > > to configure periphals.
> > >
> > >  So introduce an attribute to indicated FIXED clock
> > >
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  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=20
> > > 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 =3D {
> > >   .determine_rate =3D scmi_clk_determine_rate,  };
> > >
> > > +static const struct clk_ops scmi_fixed_rate_clk_ops =3D { =20
> > > +.recalc_rate =3D scmi_clk_recalc_rate, };
> > > +
> > >  static const struct clk_ops scmi_atomic_clk_ops =3D {
> > >   .recalc_rate =3D scmi_clk_recalc_rate,
> > >   .round_rate =3D scmi_clk_round_rate, @@ -293,6 +297,8 @@ static=20
> > > int scmi_clocks_probe(struct scmi_device *sdev)
> > >           if (is_atomic &&
> > >               sclk->info->enable_latency <=3D atomic_threshold)
> > >                   scmi_ops =3D &scmi_atomic_clk_ops;
> > > +         else if (sclk->info->rate_fixed)
> > > +                 scmi_ops =3D &scmi_fixed_rate_clk_ops;
> > >           else
> > >                   scmi_ops =3D &scmi_clk_ops;
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c=20
> > > 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=20
> > > 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=20
> > clock rate ? Or step clock with both lowest and highest the same and st=
ep being 0.
> > At-least I don't see the need to change the spec for this and hence=20
> > 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=20
> rate BUT such clock cannot be changed by Linux, only other agents can=20
> enable/disable/set_rate BUT they still want to be able to query the=20
> 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