diff mbox series

[v2,5/5] clk: scmi: register scmi clocks with CCF

Message ID 20220221082242.6349-5-etienne.carriere@linaro.org
State Accepted
Commit 7c33f78983c344c46d46d857fd1d5e2b5b95ad40
Headers show
Series [v2,1/5] doc: binding: scmi: link to latest Linux kernel binding | expand

Commit Message

Etienne Carriere Feb. 21, 2022, 8:22 a.m. UTC 
Implements SCMI APIs to retrieve the number exposed SCMI clocks using
SCMI_PROTOCOL_ATTRIBUTES messages and the names of the clocks using
SCMI_CLOCK_ATTRIBUTES messages.

This change updates sandbox SCMI clock test driver to manage these
2 new message IDs.

Cc: Lukasz Majewski <lukma@denx.de>
Cc: Sean Anderson <seanga2@gmail.com>
Cc: Clement Leger <clement.leger@bootlin.com>
Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
Changes since v1:
- Remove Series-links tag and apply R-b tag.
---
 drivers/clk/clk_scmi.c                     | 90 ++++++++++++++++++++++
 drivers/firmware/scmi/sandbox-scmi_agent.c | 53 +++++++++++++
 include/scmi_protocols.h                   | 43 +++++++++++
 3 files changed, 186 insertions(+)

Comments

Sean Anderson Feb. 25, 2022, 6:33 a.m. UTC | #1 
Hi Etienne,


On 2/21/22 3:22 AM, Etienne Carriere wrote:
> Implements SCMI APIs to retrieve the number exposed SCMI clocks using
> SCMI_PROTOCOL_ATTRIBUTES messages and the names of the clocks using
> SCMI_CLOCK_ATTRIBUTES messages.
> 
> This change updates sandbox SCMI clock test driver to manage these
> 2 new message IDs.
> 
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Sean Anderson <seanga2@gmail.com>
> Cc: Clement Leger <clement.leger@bootlin.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
> Changes since v1:
> - Remove Series-links tag and apply R-b tag.
> ---
>   drivers/clk/clk_scmi.c                     | 90 ++++++++++++++++++++++
>   drivers/firmware/scmi/sandbox-scmi_agent.c | 53 +++++++++++++
>   include/scmi_protocols.h                   | 43 +++++++++++
>   3 files changed, 186 insertions(+)
> 
> diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
> index 42fbab0d21..57022685e2 100644
> --- a/drivers/clk/clk_scmi.c
> +++ b/drivers/clk/clk_scmi.c
> @@ -11,6 +11,52 @@
>   #include <scmi_agent.h>
>   #include <scmi_protocols.h>
>   #include <asm/types.h>
> +#include <linux/clk-provider.h>
> +
> +static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
> +{
> +	struct scmi_clk_protocol_attr_out out;
> +	struct scmi_msg msg = {
> +		.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> +		.message_id = SCMI_PROTOCOL_ATTRIBUTES,
> +		.out_msg = (u8 *)&out,
> +		.out_msg_sz = sizeof(out),
> +	};
> +	int ret;
> +
> +	ret = devm_scmi_process_msg(dev, &msg);
> +	if (ret)
> +		return ret;
> +
> +	*num_clocks = out.attributes & SCMI_CLK_PROTO_ATTR_COUNT_MASK;
> +
> +	return 0;
> +}
> +
> +static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name)

nit: scmi_clk_get_attribute

> +{
> +	struct scmi_clk_attribute_in in = {
> +		.clock_id = clkid,
> +	};
> +	struct scmi_clk_attribute_out out;
> +	struct scmi_msg msg = {
> +		.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> +		.message_id = SCMI_CLOCK_ATTRIBUTES,
> +		.in_msg = (u8 *)&in,
> +		.in_msg_sz = sizeof(in),
> +		.out_msg = (u8 *)&out,
> +		.out_msg_sz = sizeof(out),
> +	};
> +	int ret;
> +
> +	ret = devm_scmi_process_msg(dev, &msg);
> +	if (ret)
> +		return ret;
> +
> +	*name = out.clock_name;
> +
> +	return 0;
> +}
>   
>   static int scmi_clk_gate(struct clk *clk, int enable)
>   {
> @@ -88,6 +134,49 @@ static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
>   	return scmi_clk_get_rate(clk);
>   }
>   
> +static int scmi_clk_probe(struct udevice *dev)
> +{
> +	struct clk *clk;
> +	size_t num_clocks, i;
> +	int ret;
> +
> +	if (!CONFIG_IS_ENABLED(CLK_CCF))
> +		return 0;
> +
> +	/* register CCF children: CLK UCLASS, no probed again */
> +	if (device_get_uclass_id(dev->parent) == UCLASS_CLK)
> +		return 0;
> +
> +	ret = scmi_clk_get_num_clock(dev, &num_clocks);
> +	if (ret)
> +		return ret;
> +
> +	for (i = 0; i < num_clocks; i++) {
> +		char *name;
> +
> +		if (!scmi_clk_get_attibute(dev, i, &name)) {
> +			char *clock_name = strdup(name);
> +
> +			clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +			if (!clk || !clock_name)
> +				ret = -ENOMEM;
> +			else
> +				ret = clk_register(clk, dev->driver->name,
> +						   clock_name, dev->name);
> +
> +			if (ret) {
> +				free(clk);
> +				free(clock_name);
> +				return ret;
> +			}
> +
> +			clk_dm(i, clk);
> +		}
> +	}
> +
> +	return 0;
> +}
> +

So why not call scmi_clk_get_num_clock during probe() and then check against
it in xlate? I don't really see why we need CCF. This also means you don't
have a driver with copies of itself as children, and is lighter on memory.

--Sean

>   static const struct clk_ops scmi_clk_ops = {
>   	.enable = scmi_clk_enable,
>   	.disable = scmi_clk_disable,
> @@ -99,4 +188,5 @@ U_BOOT_DRIVER(scmi_clock) = {
>   	.name = "scmi_clk",
>   	.id = UCLASS_CLK,
>   	.ops = &scmi_clk_ops,
> +	.probe = &scmi_clk_probe,
>   };
> diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
> index fc717a8aea..c555164d19 100644
> --- a/drivers/firmware/scmi/sandbox-scmi_agent.c
> +++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
> @@ -114,6 +114,55 @@ static struct sandbox_scmi_voltd *get_scmi_voltd_state(uint domain_id)
>    * Sandbox SCMI agent ops
>    */
>   
> +static int sandbox_scmi_clock_protocol_attribs(struct udevice *dev,
> +					       struct scmi_msg *msg)
> +{
> +	struct scmi_clk_protocol_attr_out *out = NULL;
> +
> +	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
> +		return -EINVAL;
> +
> +	out = (struct scmi_clk_protocol_attr_out *)msg->out_msg;
> +	out->attributes = ARRAY_SIZE(scmi_clk);
> +	out->status = SCMI_SUCCESS;
> +
> +	return 0;
> +}
> +
> +static int sandbox_scmi_clock_attribs(struct udevice *dev, struct scmi_msg *msg)
> +{
> +	struct scmi_clk_attribute_in *in = NULL;
> +	struct scmi_clk_attribute_out *out = NULL;
> +	struct sandbox_scmi_clk *clk_state = NULL;
> +	int ret;
> +
> +	if (!msg->in_msg || msg->in_msg_sz < sizeof(*in) ||
> +	    !msg->out_msg || msg->out_msg_sz < sizeof(*out))
> +		return -EINVAL;
> +
> +	in = (struct scmi_clk_attribute_in *)msg->in_msg;
> +	out = (struct scmi_clk_attribute_out *)msg->out_msg;
> +
> +	clk_state = get_scmi_clk_state(in->clock_id);
> +	if (!clk_state) {
> +		dev_err(dev, "Unexpected clock ID %u\n", in->clock_id);
> +
> +		out->status = SCMI_NOT_FOUND;
> +	} else {
> +		memset(out, 0, sizeof(*out));
> +
> +		if (clk_state->enabled)
> +			out->attributes = 1;
> +
> +		ret = snprintf(out->clock_name, sizeof(out->clock_name),
> +			       "clk%u", in->clock_id);
> +		assert(ret > 0 && ret < sizeof(out->clock_name));
> +
> +		out->status = SCMI_SUCCESS;
> +	}
> +
> +	return 0;
> +}
>   static int sandbox_scmi_clock_rate_set(struct udevice *dev,
>   				       struct scmi_msg *msg)
>   {
> @@ -427,6 +476,10 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
>   	switch (msg->protocol_id) {
>   	case SCMI_PROTOCOL_ID_CLOCK:
>   		switch (msg->message_id) {
> +		case SCMI_PROTOCOL_ATTRIBUTES:
> +			return sandbox_scmi_clock_protocol_attribs(dev, msg);
> +		case SCMI_CLOCK_ATTRIBUTES:
> +			return sandbox_scmi_clock_attribs(dev, msg);
>   		case SCMI_CLOCK_RATE_SET:
>   			return sandbox_scmi_clock_rate_set(dev, msg);
>   		case SCMI_CLOCK_RATE_GET:
> diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> index ef26e72176..a220cb2a91 100644
> --- a/include/scmi_protocols.h
> +++ b/include/scmi_protocols.h
> @@ -40,22 +40,65 @@ enum scmi_status_code {
>   	SCMI_PROTOCOL_ERROR = -10,
>   };
>   
> +/*
> + * Generic message IDs
> + */
> +enum scmi_discovery_id {
> +	SCMI_PROTOCOL_VERSION = 0x0,
> +	SCMI_PROTOCOL_ATTRIBUTES = 0x1,
> +	SCMI_PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
> +};
> +
>   /*
>    * SCMI Clock Protocol
>    */
>   
>   enum scmi_clock_message_id {
> +	SCMI_CLOCK_ATTRIBUTES = 0x3,
>   	SCMI_CLOCK_RATE_SET = 0x5,
>   	SCMI_CLOCK_RATE_GET = 0x6,
>   	SCMI_CLOCK_CONFIG_SET = 0x7,
>   };
>   
> +#define SCMI_CLK_PROTO_ATTR_COUNT_MASK	GENMASK(15, 0)
>   #define SCMI_CLK_RATE_ASYNC_NOTIFY	BIT(0)
>   #define SCMI_CLK_RATE_ASYNC_NORESP	(BIT(0) | BIT(1))
>   #define SCMI_CLK_RATE_ROUND_DOWN	0
>   #define SCMI_CLK_RATE_ROUND_UP		BIT(2)
>   #define SCMI_CLK_RATE_ROUND_CLOSEST	BIT(3)
>   
> +#define SCMI_CLOCK_NAME_LENGTH_MAX 16
> +
> +/**
> + * struct scmi_clk_get_nb_out - Response for SCMI_PROTOCOL_ATTRIBUTES command
> + * @status:	SCMI command status
> + * @attributes:	Attributes of the clock protocol, mainly number of clocks exposed
> + */
> +struct scmi_clk_protocol_attr_out {
> +	s32 status;
> +	u32 attributes;
> +};
> +
> +/**
> + * struct scmi_clk_attribute_in - Message payload for SCMI_CLOCK_ATTRIBUTES command
> + * @clock_id:	SCMI clock ID
> + */
> +struct scmi_clk_attribute_in {
> +	u32 clock_id;
> +};
> +
> +/**
> + * struct scmi_clk_get_nb_out - Response payload for SCMI_CLOCK_ATTRIBUTES command
> + * @status:	SCMI command status
> + * @attributes:	clock attributes
> + * @clock_name:	name of the clock
> + */
> +struct scmi_clk_attribute_out {
> +	s32 status;
> +	u32 attributes;
> +	char clock_name[SCMI_CLOCK_NAME_LENGTH_MAX];
> +};
> +
>   /**
>    * struct scmi_clk_state_in - Message payload for CLOCK_CONFIG_SET command
>    * @clock_id:	SCMI clock ID
>
Tom Rini March 3, 2022, 7:16 p.m. UTC | #2 
On Mon, Feb 21, 2022 at 09:22:42AM +0100, Etienne Carriere wrote:

> Implements SCMI APIs to retrieve the number exposed SCMI clocks using
> SCMI_PROTOCOL_ATTRIBUTES messages and the names of the clocks using
> SCMI_CLOCK_ATTRIBUTES messages.
> 
> This change updates sandbox SCMI clock test driver to manage these
> 2 new message IDs.
> 
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Sean Anderson <seanga2@gmail.com>
> Cc: Clement Leger <clement.leger@bootlin.com>
> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>

Applied to u-boot/next, thanks!
Etienne Carriere March 7, 2022, 10:17 a.m. UTC | #3 
Hello Sean,

Thanks for the feedback.
Sorry I missed your post end Feb.

On Fri, 25 Feb 2022 at 07:33, Sean Anderson <seanga2@gmail.com> wrote:
>
> Hi Etienne,
>
>
> On 2/21/22 3:22 AM, Etienne Carriere wrote:
> > Implements SCMI APIs to retrieve the number exposed SCMI clocks using
> > SCMI_PROTOCOL_ATTRIBUTES messages and the names of the clocks using
> > SCMI_CLOCK_ATTRIBUTES messages.
> >
> > This change updates sandbox SCMI clock test driver to manage these
> > 2 new message IDs.
> >
> > Cc: Lukasz Majewski <lukma@denx.de>
> > Cc: Sean Anderson <seanga2@gmail.com>
> > Cc: Clement Leger <clement.leger@bootlin.com>
> > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
> > Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> > Changes since v1:
> > - Remove Series-links tag and apply R-b tag.
> > ---
> >   drivers/clk/clk_scmi.c                     | 90 ++++++++++++++++++++++
> >   drivers/firmware/scmi/sandbox-scmi_agent.c | 53 +++++++++++++
> >   include/scmi_protocols.h                   | 43 +++++++++++
> >   3 files changed, 186 insertions(+)
> >
> > diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
> > index 42fbab0d21..57022685e2 100644
> > --- a/drivers/clk/clk_scmi.c
> > +++ b/drivers/clk/clk_scmi.c
> > @@ -11,6 +11,52 @@
> >   #include <scmi_agent.h>
> >   #include <scmi_protocols.h>
> >   #include <asm/types.h>
> > +#include <linux/clk-provider.h>
> > +
> > +static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
> > +{
> > +     struct scmi_clk_protocol_attr_out out;
> > +     struct scmi_msg msg = {
> > +             .protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> > +             .message_id = SCMI_PROTOCOL_ATTRIBUTES,
> > +             .out_msg = (u8 *)&out,
> > +             .out_msg_sz = sizeof(out),
> > +     };
> > +     int ret;
> > +
> > +     ret = devm_scmi_process_msg(dev, &msg);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *num_clocks = out.attributes & SCMI_CLK_PROTO_ATTR_COUNT_MASK;
> > +
> > +     return 0;
> > +}
> > +
> > +static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name)
>
> nit: scmi_clk_get_attribute

right!

>
> > +{
> > +     struct scmi_clk_attribute_in in = {
> > +             .clock_id = clkid,
> > +     };
> > +     struct scmi_clk_attribute_out out;
> > +     struct scmi_msg msg = {
> > +             .protocol_id = SCMI_PROTOCOL_ID_CLOCK,
> > +             .message_id = SCMI_CLOCK_ATTRIBUTES,
> > +             .in_msg = (u8 *)&in,
> > +             .in_msg_sz = sizeof(in),
> > +             .out_msg = (u8 *)&out,
> > +             .out_msg_sz = sizeof(out),
> > +     };
> > +     int ret;
> > +
> > +     ret = devm_scmi_process_msg(dev, &msg);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *name = out.clock_name;
> > +
> > +     return 0;
> > +}
> >
> >   static int scmi_clk_gate(struct clk *clk, int enable)
> >   {
> > @@ -88,6 +134,49 @@ static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
> >       return scmi_clk_get_rate(clk);
> >   }
> >
> > +static int scmi_clk_probe(struct udevice *dev)
> > +{
> > +     struct clk *clk;
> > +     size_t num_clocks, i;
> > +     int ret;
> > +
> > +     if (!CONFIG_IS_ENABLED(CLK_CCF))
> > +             return 0;
> > +
> > +     /* register CCF children: CLK UCLASS, no probed again */
> > +     if (device_get_uclass_id(dev->parent) == UCLASS_CLK)
> > +             return 0;
> > +
> > +     ret = scmi_clk_get_num_clock(dev, &num_clocks);
> > +     if (ret)
> > +             return ret;
> > +
> > +     for (i = 0; i < num_clocks; i++) {
> > +             char *name;
> > +
> > +             if (!scmi_clk_get_attibute(dev, i, &name)) {
> > +                     char *clock_name = strdup(name);
> > +
> > +                     clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > +                     if (!clk || !clock_name)
> > +                             ret = -ENOMEM;
> > +                     else
> > +                             ret = clk_register(clk, dev->driver->name,
> > +                                                clock_name, dev->name);
> > +
> > +                     if (ret) {
> > +                             free(clk);
> > +                             free(clock_name);
> > +                             return ret;
> > +                     }
> > +
> > +                     clk_dm(i, clk);
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
>
> So why not call scmi_clk_get_num_clock during probe() and then check against
> it in xlate? I don't really see why we need CCF. This also means you don't
> have a driver with copies of itself as children, and is lighter on memory.

There is no specific need for CCF for the scmi clock driver itself.
The goal of these changes is rather to allow platforms that do already
enable CCF (for an u-boot native clock driver)
to be able to also embed SCMI clocks support, for clocks controlled
from an external firmware.

Br,
etienne

>
> --Sean
>
> >   static const struct clk_ops scmi_clk_ops = {
> >       .enable = scmi_clk_enable,
> >       .disable = scmi_clk_disable,
> > @@ -99,4 +188,5 @@ U_BOOT_DRIVER(scmi_clock) = {
> >       .name = "scmi_clk",
> >       .id = UCLASS_CLK,
> >       .ops = &scmi_clk_ops,
> > +     .probe = &scmi_clk_probe,
> >   };
> > diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
> > index fc717a8aea..c555164d19 100644
> > --- a/drivers/firmware/scmi/sandbox-scmi_agent.c
> > +++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
> > @@ -114,6 +114,55 @@ static struct sandbox_scmi_voltd *get_scmi_voltd_state(uint domain_id)
> >    * Sandbox SCMI agent ops
> >    */
> >
> > +static int sandbox_scmi_clock_protocol_attribs(struct udevice *dev,
> > +                                            struct scmi_msg *msg)
> > +{
> > +     struct scmi_clk_protocol_attr_out *out = NULL;
> > +
> > +     if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
> > +             return -EINVAL;
> > +
> > +     out = (struct scmi_clk_protocol_attr_out *)msg->out_msg;
> > +     out->attributes = ARRAY_SIZE(scmi_clk);
> > +     out->status = SCMI_SUCCESS;
> > +
> > +     return 0;
> > +}
> > +
> > +static int sandbox_scmi_clock_attribs(struct udevice *dev, struct scmi_msg *msg)
> > +{
> > +     struct scmi_clk_attribute_in *in = NULL;
> > +     struct scmi_clk_attribute_out *out = NULL;
> > +     struct sandbox_scmi_clk *clk_state = NULL;
> > +     int ret;
> > +
> > +     if (!msg->in_msg || msg->in_msg_sz < sizeof(*in) ||
> > +         !msg->out_msg || msg->out_msg_sz < sizeof(*out))
> > +             return -EINVAL;
> > +
> > +     in = (struct scmi_clk_attribute_in *)msg->in_msg;
> > +     out = (struct scmi_clk_attribute_out *)msg->out_msg;
> > +
> > +     clk_state = get_scmi_clk_state(in->clock_id);
> > +     if (!clk_state) {
> > +             dev_err(dev, "Unexpected clock ID %u\n", in->clock_id);
> > +
> > +             out->status = SCMI_NOT_FOUND;
> > +     } else {
> > +             memset(out, 0, sizeof(*out));
> > +
> > +             if (clk_state->enabled)
> > +                     out->attributes = 1;
> > +
> > +             ret = snprintf(out->clock_name, sizeof(out->clock_name),
> > +                            "clk%u", in->clock_id);
> > +             assert(ret > 0 && ret < sizeof(out->clock_name));
> > +
> > +             out->status = SCMI_SUCCESS;
> > +     }
> > +
> > +     return 0;
> > +}
> >   static int sandbox_scmi_clock_rate_set(struct udevice *dev,
> >                                      struct scmi_msg *msg)
> >   {
> > @@ -427,6 +476,10 @@ static int sandbox_scmi_test_process_msg(struct udevice *dev,
> >       switch (msg->protocol_id) {
> >       case SCMI_PROTOCOL_ID_CLOCK:
> >               switch (msg->message_id) {
> > +             case SCMI_PROTOCOL_ATTRIBUTES:
> > +                     return sandbox_scmi_clock_protocol_attribs(dev, msg);
> > +             case SCMI_CLOCK_ATTRIBUTES:
> > +                     return sandbox_scmi_clock_attribs(dev, msg);
> >               case SCMI_CLOCK_RATE_SET:
> >                       return sandbox_scmi_clock_rate_set(dev, msg);
> >               case SCMI_CLOCK_RATE_GET:
> > diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
> > index ef26e72176..a220cb2a91 100644
> > --- a/include/scmi_protocols.h
> > +++ b/include/scmi_protocols.h
> > @@ -40,22 +40,65 @@ enum scmi_status_code {
> >       SCMI_PROTOCOL_ERROR = -10,
> >   };
> >
> > +/*
> > + * Generic message IDs
> > + */
> > +enum scmi_discovery_id {
> > +     SCMI_PROTOCOL_VERSION = 0x0,
> > +     SCMI_PROTOCOL_ATTRIBUTES = 0x1,
> > +     SCMI_PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
> > +};
> > +
> >   /*
> >    * SCMI Clock Protocol
> >    */
> >
> >   enum scmi_clock_message_id {
> > +     SCMI_CLOCK_ATTRIBUTES = 0x3,
> >       SCMI_CLOCK_RATE_SET = 0x5,
> >       SCMI_CLOCK_RATE_GET = 0x6,
> >       SCMI_CLOCK_CONFIG_SET = 0x7,
> >   };
> >
> > +#define SCMI_CLK_PROTO_ATTR_COUNT_MASK       GENMASK(15, 0)
> >   #define SCMI_CLK_RATE_ASYNC_NOTIFY  BIT(0)
> >   #define SCMI_CLK_RATE_ASYNC_NORESP  (BIT(0) | BIT(1))
> >   #define SCMI_CLK_RATE_ROUND_DOWN    0
> >   #define SCMI_CLK_RATE_ROUND_UP              BIT(2)
> >   #define SCMI_CLK_RATE_ROUND_CLOSEST BIT(3)
> >
> > +#define SCMI_CLOCK_NAME_LENGTH_MAX 16
> > +
> > +/**
> > + * struct scmi_clk_get_nb_out - Response for SCMI_PROTOCOL_ATTRIBUTES command
> > + * @status:  SCMI command status
> > + * @attributes:      Attributes of the clock protocol, mainly number of clocks exposed
> > + */
> > +struct scmi_clk_protocol_attr_out {
> > +     s32 status;
> > +     u32 attributes;
> > +};
> > +
> > +/**
> > + * struct scmi_clk_attribute_in - Message payload for SCMI_CLOCK_ATTRIBUTES command
> > + * @clock_id:        SCMI clock ID
> > + */
> > +struct scmi_clk_attribute_in {
> > +     u32 clock_id;
> > +};
> > +
> > +/**
> > + * struct scmi_clk_get_nb_out - Response payload for SCMI_CLOCK_ATTRIBUTES command
> > + * @status:  SCMI command status
> > + * @attributes:      clock attributes
> > + * @clock_name:      name of the clock
> > + */
> > +struct scmi_clk_attribute_out {
> > +     s32 status;
> > +     u32 attributes;
> > +     char clock_name[SCMI_CLOCK_NAME_LENGTH_MAX];
> > +};
> > +
> >   /**
> >    * struct scmi_clk_state_in - Message payload for CLOCK_CONFIG_SET command
> >    * @clock_id:       SCMI clock ID
> >
>
Sean Anderson March 16, 2022, 4:09 p.m. UTC | #4 
On 3/7/22 5:17 AM, Etienne Carriere wrote:
> Hello Sean,
> 
> Thanks for the feedback.
> Sorry I missed your post end Feb.
> 
> On Fri, 25 Feb 2022 at 07:33, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> Hi Etienne,
>>
>>
>> On 2/21/22 3:22 AM, Etienne Carriere wrote:
>>> Implements SCMI APIs to retrieve the number exposed SCMI clocks using
>>> SCMI_PROTOCOL_ATTRIBUTES messages and the names of the clocks using
>>> SCMI_CLOCK_ATTRIBUTES messages.
>>>
>>> This change updates sandbox SCMI clock test driver to manage these
>>> 2 new message IDs.
>>>
>>> Cc: Lukasz Majewski <lukma@denx.de>
>>> Cc: Sean Anderson <seanga2@gmail.com>
>>> Cc: Clement Leger <clement.leger@bootlin.com>
>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>> Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com>
>>> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@st.com>
>>> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
>>> ---
>>> Changes since v1:
>>> - Remove Series-links tag and apply R-b tag.
>>> ---
>>>    drivers/clk/clk_scmi.c                     | 90 ++++++++++++++++++++++
>>>    drivers/firmware/scmi/sandbox-scmi_agent.c | 53 +++++++++++++
>>>    include/scmi_protocols.h                   | 43 +++++++++++
>>>    3 files changed, 186 insertions(+)
>>>
>>> diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
>>> index 42fbab0d21..57022685e2 100644
>>> --- a/drivers/clk/clk_scmi.c
>>> +++ b/drivers/clk/clk_scmi.c
>>> @@ -11,6 +11,52 @@
>>>    #include <scmi_agent.h>
>>>    #include <scmi_protocols.h>
>>>    #include <asm/types.h>
>>> +#include <linux/clk-provider.h>
>>> +
>>> +static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
>>> +{
>>> +     struct scmi_clk_protocol_attr_out out;
>>> +     struct scmi_msg msg = {
>>> +             .protocol_id = SCMI_PROTOCOL_ID_CLOCK,
>>> +             .message_id = SCMI_PROTOCOL_ATTRIBUTES,
>>> +             .out_msg = (u8 *)&out,
>>> +             .out_msg_sz = sizeof(out),
>>> +     };
>>> +     int ret;
>>> +
>>> +     ret = devm_scmi_process_msg(dev, &msg);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     *num_clocks = out.attributes & SCMI_CLK_PROTO_ATTR_COUNT_MASK;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name)
>>
>> nit: scmi_clk_get_attribute
> 
> right!
> 
>>
>>> +{
>>> +     struct scmi_clk_attribute_in in = {
>>> +             .clock_id = clkid,
>>> +     };
>>> +     struct scmi_clk_attribute_out out;
>>> +     struct scmi_msg msg = {
>>> +             .protocol_id = SCMI_PROTOCOL_ID_CLOCK,
>>> +             .message_id = SCMI_CLOCK_ATTRIBUTES,
>>> +             .in_msg = (u8 *)&in,
>>> +             .in_msg_sz = sizeof(in),
>>> +             .out_msg = (u8 *)&out,
>>> +             .out_msg_sz = sizeof(out),
>>> +     };
>>> +     int ret;
>>> +
>>> +     ret = devm_scmi_process_msg(dev, &msg);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     *name = out.clock_name;
>>> +
>>> +     return 0;
>>> +}
>>>
>>>    static int scmi_clk_gate(struct clk *clk, int enable)
>>>    {
>>> @@ -88,6 +134,49 @@ static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
>>>        return scmi_clk_get_rate(clk);
>>>    }
>>>
>>> +static int scmi_clk_probe(struct udevice *dev)
>>> +{
>>> +     struct clk *clk;
>>> +     size_t num_clocks, i;
>>> +     int ret;
>>> +
>>> +     if (!CONFIG_IS_ENABLED(CLK_CCF))
>>> +             return 0;
>>> +
>>> +     /* register CCF children: CLK UCLASS, no probed again */
>>> +     if (device_get_uclass_id(dev->parent) == UCLASS_CLK)
>>> +             return 0;
>>> +
>>> +     ret = scmi_clk_get_num_clock(dev, &num_clocks);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     for (i = 0; i < num_clocks; i++) {
>>> +             char *name;
>>> +
>>> +             if (!scmi_clk_get_attibute(dev, i, &name)) {
>>> +                     char *clock_name = strdup(name);
>>> +
>>> +                     clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>>> +                     if (!clk || !clock_name)
>>> +                             ret = -ENOMEM;
>>> +                     else
>>> +                             ret = clk_register(clk, dev->driver->name,
>>> +                                                clock_name, dev->name);
>>> +
>>> +                     if (ret) {
>>> +                             free(clk);
>>> +                             free(clock_name);
>>> +                             return ret;
>>> +                     }
>>> +
>>> +                     clk_dm(i, clk);
>>> +             }
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>
>> So why not call scmi_clk_get_num_clock during probe() and then check against
>> it in xlate? I don't really see why we need CCF. This also means you don't
>> have a driver with copies of itself as children, and is lighter on memory.
> 
> There is no specific need for CCF for the scmi clock driver itself.
> The goal of these changes is rather to allow platforms that do already
> enable CCF (for an u-boot native clock driver)
> to be able to also embed SCMI clocks support, for clocks controlled
> from an external firmware.

Sure, but you can still use non-CCF clocks in a CCF clock.

--Sean
diff mbox series

Patch

diff --git a/drivers/clk/clk_scmi.c b/drivers/clk/clk_scmi.c
index 42fbab0d21..57022685e2 100644
--- a/drivers/clk/clk_scmi.c
+++ b/drivers/clk/clk_scmi.c
@@ -11,6 +11,52 @@ 
 #include <scmi_agent.h>
 #include <scmi_protocols.h>
 #include <asm/types.h>
+#include <linux/clk-provider.h>
+
+static int scmi_clk_get_num_clock(struct udevice *dev, size_t *num_clocks)
+{
+	struct scmi_clk_protocol_attr_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
+		.message_id = SCMI_PROTOCOL_ATTRIBUTES,
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, &msg);
+	if (ret)
+		return ret;
+
+	*num_clocks = out.attributes & SCMI_CLK_PROTO_ATTR_COUNT_MASK;
+
+	return 0;
+}
+
+static int scmi_clk_get_attibute(struct udevice *dev, int clkid, char **name)
+{
+	struct scmi_clk_attribute_in in = {
+		.clock_id = clkid,
+	};
+	struct scmi_clk_attribute_out out;
+	struct scmi_msg msg = {
+		.protocol_id = SCMI_PROTOCOL_ID_CLOCK,
+		.message_id = SCMI_CLOCK_ATTRIBUTES,
+		.in_msg = (u8 *)&in,
+		.in_msg_sz = sizeof(in),
+		.out_msg = (u8 *)&out,
+		.out_msg_sz = sizeof(out),
+	};
+	int ret;
+
+	ret = devm_scmi_process_msg(dev, &msg);
+	if (ret)
+		return ret;
+
+	*name = out.clock_name;
+
+	return 0;
+}
 
 static int scmi_clk_gate(struct clk *clk, int enable)
 {
@@ -88,6 +134,49 @@  static ulong scmi_clk_set_rate(struct clk *clk, ulong rate)
 	return scmi_clk_get_rate(clk);
 }
 
+static int scmi_clk_probe(struct udevice *dev)
+{
+	struct clk *clk;
+	size_t num_clocks, i;
+	int ret;
+
+	if (!CONFIG_IS_ENABLED(CLK_CCF))
+		return 0;
+
+	/* register CCF children: CLK UCLASS, no probed again */
+	if (device_get_uclass_id(dev->parent) == UCLASS_CLK)
+		return 0;
+
+	ret = scmi_clk_get_num_clock(dev, &num_clocks);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < num_clocks; i++) {
+		char *name;
+
+		if (!scmi_clk_get_attibute(dev, i, &name)) {
+			char *clock_name = strdup(name);
+
+			clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+			if (!clk || !clock_name)
+				ret = -ENOMEM;
+			else
+				ret = clk_register(clk, dev->driver->name,
+						   clock_name, dev->name);
+
+			if (ret) {
+				free(clk);
+				free(clock_name);
+				return ret;
+			}
+
+			clk_dm(i, clk);
+		}
+	}
+
+	return 0;
+}
+
 static const struct clk_ops scmi_clk_ops = {
 	.enable = scmi_clk_enable,
 	.disable = scmi_clk_disable,
@@ -99,4 +188,5 @@  U_BOOT_DRIVER(scmi_clock) = {
 	.name = "scmi_clk",
 	.id = UCLASS_CLK,
 	.ops = &scmi_clk_ops,
+	.probe = &scmi_clk_probe,
 };
diff --git a/drivers/firmware/scmi/sandbox-scmi_agent.c b/drivers/firmware/scmi/sandbox-scmi_agent.c
index fc717a8aea..c555164d19 100644
--- a/drivers/firmware/scmi/sandbox-scmi_agent.c
+++ b/drivers/firmware/scmi/sandbox-scmi_agent.c
@@ -114,6 +114,55 @@  static struct sandbox_scmi_voltd *get_scmi_voltd_state(uint domain_id)
  * Sandbox SCMI agent ops
  */
 
+static int sandbox_scmi_clock_protocol_attribs(struct udevice *dev,
+					       struct scmi_msg *msg)
+{
+	struct scmi_clk_protocol_attr_out *out = NULL;
+
+	if (!msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	out = (struct scmi_clk_protocol_attr_out *)msg->out_msg;
+	out->attributes = ARRAY_SIZE(scmi_clk);
+	out->status = SCMI_SUCCESS;
+
+	return 0;
+}
+
+static int sandbox_scmi_clock_attribs(struct udevice *dev, struct scmi_msg *msg)
+{
+	struct scmi_clk_attribute_in *in = NULL;
+	struct scmi_clk_attribute_out *out = NULL;
+	struct sandbox_scmi_clk *clk_state = NULL;
+	int ret;
+
+	if (!msg->in_msg || msg->in_msg_sz < sizeof(*in) ||
+	    !msg->out_msg || msg->out_msg_sz < sizeof(*out))
+		return -EINVAL;
+
+	in = (struct scmi_clk_attribute_in *)msg->in_msg;
+	out = (struct scmi_clk_attribute_out *)msg->out_msg;
+
+	clk_state = get_scmi_clk_state(in->clock_id);
+	if (!clk_state) {
+		dev_err(dev, "Unexpected clock ID %u\n", in->clock_id);
+
+		out->status = SCMI_NOT_FOUND;
+	} else {
+		memset(out, 0, sizeof(*out));
+
+		if (clk_state->enabled)
+			out->attributes = 1;
+
+		ret = snprintf(out->clock_name, sizeof(out->clock_name),
+			       "clk%u", in->clock_id);
+		assert(ret > 0 && ret < sizeof(out->clock_name));
+
+		out->status = SCMI_SUCCESS;
+	}
+
+	return 0;
+}
 static int sandbox_scmi_clock_rate_set(struct udevice *dev,
 				       struct scmi_msg *msg)
 {
@@ -427,6 +476,10 @@  static int sandbox_scmi_test_process_msg(struct udevice *dev,
 	switch (msg->protocol_id) {
 	case SCMI_PROTOCOL_ID_CLOCK:
 		switch (msg->message_id) {
+		case SCMI_PROTOCOL_ATTRIBUTES:
+			return sandbox_scmi_clock_protocol_attribs(dev, msg);
+		case SCMI_CLOCK_ATTRIBUTES:
+			return sandbox_scmi_clock_attribs(dev, msg);
 		case SCMI_CLOCK_RATE_SET:
 			return sandbox_scmi_clock_rate_set(dev, msg);
 		case SCMI_CLOCK_RATE_GET:
diff --git a/include/scmi_protocols.h b/include/scmi_protocols.h
index ef26e72176..a220cb2a91 100644
--- a/include/scmi_protocols.h
+++ b/include/scmi_protocols.h
@@ -40,22 +40,65 @@  enum scmi_status_code {
 	SCMI_PROTOCOL_ERROR = -10,
 };
 
+/*
+ * Generic message IDs
+ */
+enum scmi_discovery_id {
+	SCMI_PROTOCOL_VERSION = 0x0,
+	SCMI_PROTOCOL_ATTRIBUTES = 0x1,
+	SCMI_PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
+};
+
 /*
  * SCMI Clock Protocol
  */
 
 enum scmi_clock_message_id {
+	SCMI_CLOCK_ATTRIBUTES = 0x3,
 	SCMI_CLOCK_RATE_SET = 0x5,
 	SCMI_CLOCK_RATE_GET = 0x6,
 	SCMI_CLOCK_CONFIG_SET = 0x7,
 };
 
+#define SCMI_CLK_PROTO_ATTR_COUNT_MASK	GENMASK(15, 0)
 #define SCMI_CLK_RATE_ASYNC_NOTIFY	BIT(0)
 #define SCMI_CLK_RATE_ASYNC_NORESP	(BIT(0) | BIT(1))
 #define SCMI_CLK_RATE_ROUND_DOWN	0
 #define SCMI_CLK_RATE_ROUND_UP		BIT(2)
 #define SCMI_CLK_RATE_ROUND_CLOSEST	BIT(3)
 
+#define SCMI_CLOCK_NAME_LENGTH_MAX 16
+
+/**
+ * struct scmi_clk_get_nb_out - Response for SCMI_PROTOCOL_ATTRIBUTES command
+ * @status:	SCMI command status
+ * @attributes:	Attributes of the clock protocol, mainly number of clocks exposed
+ */
+struct scmi_clk_protocol_attr_out {
+	s32 status;
+	u32 attributes;
+};
+
+/**
+ * struct scmi_clk_attribute_in - Message payload for SCMI_CLOCK_ATTRIBUTES command
+ * @clock_id:	SCMI clock ID
+ */
+struct scmi_clk_attribute_in {
+	u32 clock_id;
+};
+
+/**
+ * struct scmi_clk_get_nb_out - Response payload for SCMI_CLOCK_ATTRIBUTES command
+ * @status:	SCMI command status
+ * @attributes:	clock attributes
+ * @clock_name:	name of the clock
+ */
+struct scmi_clk_attribute_out {
+	s32 status;
+	u32 attributes;
+	char clock_name[SCMI_CLOCK_NAME_LENGTH_MAX];
+};
+
 /**
  * struct scmi_clk_state_in - Message payload for CLOCK_CONFIG_SET command
  * @clock_id:	SCMI clock ID