diff mbox series

[v3] clk: zynqmp: use structs for clk query responses

Message ID 20190412095220.22855-1-m.tretter@pengutronix.de (mailing list archive)
State Accepted, archived
Headers show
Series [v3] clk: zynqmp: use structs for clk query responses | expand

Commit Message

Michael Tretter April 12, 2019, 9:52 a.m. UTC
The driver retrieves the clock try by querying the ATF for the clock
names, the clock topology, the parents and other attributes. The driver
needs to unmarshal the responses.

The definition of the fields in the firmware responses to the queries is
inconsistent. Some are specified as a mask, some as a shift, and by the
length of the previous field.

Define C structs for the entire firmware responses to avoid passing
pointers to arrays of an implicit size and make the format of the
responses to the queries obvious.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
v2 -> v3:
- fix conflicts with new format of get_attr response

v1 -> v2:
- define structs for firmware responses
- fix size of CLK_TYPOLOGY_FLAGS
---
 drivers/clk/zynqmp/clk-zynqmp.h |   6 --
 drivers/clk/zynqmp/clkc.c       | 170 +++++++++++++++++++-------------
 2 files changed, 99 insertions(+), 77 deletions(-)

Comments

Jolly Shah April 18, 2019, 8:01 p.m. UTC | #1
Hi,

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: Friday, April 12, 2019 2:52 AM
> To: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>; Jolly
> Shah <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> Subject: [PATCH v3] clk: zynqmp: use structs for clk query responses
> 
> The driver retrieves the clock try by querying the ATF for the clock

*tree

Thanks,
Jolly Shah

> names, the clock topology, the parents and other attributes. The driver
> needs to unmarshal the responses.
> 
> The definition of the fields in the firmware responses to the queries is
> inconsistent. Some are specified as a mask, some as a shift, and by the
> length of the previous field.
> 
> Define C structs for the entire firmware responses to avoid passing
> pointers to arrays of an implicit size and make the format of the
> responses to the queries obvious.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> v2 -> v3:
> - fix conflicts with new format of get_attr response
> 
> v1 -> v2:
> - define structs for firmware responses
> - fix size of CLK_TYPOLOGY_FLAGS
> ---
>  drivers/clk/zynqmp/clk-zynqmp.h |   6 --
>  drivers/clk/zynqmp/clkc.c       | 170 +++++++++++++++++++-------------
>  2 files changed, 99 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/clk/zynqmp/clk-zynqmp.h b/drivers/clk/zynqmp/clk-zynqmp.h
> index 7ab163b67249..fec9a15c8786 100644
> --- a/drivers/clk/zynqmp/clk-zynqmp.h
> +++ b/drivers/clk/zynqmp/clk-zynqmp.h
> @@ -10,12 +10,6 @@
> 
>  #include <linux/firmware/xlnx-zynqmp.h>
> 
> -/* Clock APIs payload parameters */
> -#define CLK_GET_NAME_RESP_LEN				16
> -#define CLK_GET_TOPOLOGY_RESP_WORDS			3
> -#define CLK_GET_PARENTS_RESP_WORDS			3
> -#define CLK_GET_ATTR_RESP_WORDS				1
> -
>  enum topology_type {
>  	TYPE_INVALID,
>  	TYPE_MUX,
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index 8b9d94a2e775..fb2dbd556f9e 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -21,24 +21,6 @@
>  #define MAX_NODES			6
>  #define MAX_NAME_LEN			50
> 
> -#define CLK_TYPE_SHIFT			2
> -
> -#define PM_API_PAYLOAD_LEN		3
> -
> -#define NA_PARENT			0xFFFFFFFF
> -#define DUMMY_PARENT			0xFFFFFFFE
> -
> -#define CLK_TYPE_FIELD_LEN		4
> -#define CLK_TOPOLOGY_NODE_OFFSET	16
> -#define NODES_PER_RESP			3
> -
> -#define CLK_TYPE_FIELD_MASK		0xF
> -#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
> -#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
> -
> -#define CLK_PARENTS_ID_LEN		16
> -#define CLK_PARENTS_ID_MASK		0xFFFF
> -
>  /* Flags for parents */
>  #define PARENT_CLK_SELF			0
>  #define PARENT_CLK_NODE1		1
> @@ -52,11 +34,10 @@
>  #define END_OF_PARENTS			1
>  #define RESERVED_CLK_NAME		""
> 
> -#define CLK_VALID_MASK			0x1
> -#define NODE_CLASS_SHIFT		26U
> -#define NODE_SUBCLASS_SHIFT		20U
> -#define NODE_TYPE_SHIFT			14U
> -#define NODE_INDEX_SHIFT		0U
> +#define CLK_GET_NAME_RESP_LEN		16
> +#define CLK_GET_TOPOLOGY_RESP_WORDS	3
> +#define CLK_GET_PARENTS_RESP_WORDS	3
> +#define CLK_GET_ATTR_RESP_WORDS		1
> 
>  enum clk_type {
>  	CLK_TYPE_OUTPUT,
> @@ -97,6 +78,35 @@ struct zynqmp_clock {
>  	u32 clk_id;
>  };
> 
> +struct name_resp {
> +	char name[CLK_GET_NAME_RESP_LEN];
> +};
> +
> +struct topology_resp {
> +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> +#define CLK_TOPOLOGY_FLAGS		GENMASK(23, 8)
> +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
> +	u32 topology[CLK_GET_TOPOLOGY_RESP_WORDS];
> +};
> +
> +struct parents_resp {
> +#define NA_PARENT			0xFFFFFFFF
> +#define DUMMY_PARENT			0xFFFFFFFE
> +#define CLK_PARENTS_ID			GENMASK(15, 0)
> +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
> +	u32 parents[CLK_GET_PARENTS_RESP_WORDS];
> +};
> +
> +struct attr_resp {
> +#define CLK_ATTR_VALID			BIT(0)
> +#define CLK_ATTR_TYPE			BIT(2)
> +#define CLK_ATTR_NODE_INDEX		GENMASK(13, 0)
> +#define CLK_ATTR_NODE_TYPE		GENMASK(19, 14)
> +#define CLK_ATTR_NODE_SUBCLASS		GENMASK(25, 20)
> +#define CLK_ATTR_NODE_CLASS		GENMASK(31, 26)
> +	u32 attr[CLK_GET_ATTR_RESP_WORDS];
> +};
> +
>  static const char clk_type_postfix[][10] = {
>  	[TYPE_INVALID] = "",
>  	[TYPE_MUX] = "_mux",
> @@ -205,14 +215,15 @@ static int zynqmp_pm_clock_get_num_clocks(u32
> *nclocks)
>  /**
>   * zynqmp_pm_clock_get_name() - Get the name of clock for given id
>   * @clock_id:	ID of the clock to be queried
> - * @name:	Name of given clock
> + * @response:	Name of the clock with the given id
>   *
>   * This function is used to get name of clock specified by given
>   * clock ID.
>   *
> - * Return: Returns 0, in case of error name would be 0
> + * Return: Returns 0
>   */
> -static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
> +static int zynqmp_pm_clock_get_name(u32 clock_id,
> +				    struct name_resp *response)
>  {
>  	struct zynqmp_pm_query_data qdata = {0};
>  	u32 ret_payload[PAYLOAD_ARG_CNT];
> @@ -221,7 +232,7 @@ static int zynqmp_pm_clock_get_name(u32 clock_id,
> char *name)
>  	qdata.arg1 = clock_id;
> 
>  	eemi_ops->query_data(qdata, ret_payload);
> -	memcpy(name, ret_payload, CLK_GET_NAME_RESP_LEN);
> +	memcpy(response, ret_payload, sizeof(*response));
> 
>  	return 0;
>  }
> @@ -230,7 +241,7 @@ static int zynqmp_pm_clock_get_name(u32 clock_id,
> char *name)
>   * zynqmp_pm_clock_get_topology() - Get the topology of clock for given id
>   * @clock_id:	ID of the clock to be queried
>   * @index:	Node index of clock topology
> - * @topology:	Buffer to store nodes in topology and flags
> + * @response:	Buffer used for the topology response
>   *
>   * This function is used to get topology information for the clock
>   * specified by given clock ID.
> @@ -243,7 +254,8 @@ static int zynqmp_pm_clock_get_name(u32 clock_id,
> char *name)
>   *
>   * Return: 0 on success else error+reason
>   */
> -static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32
> *topology)
> +static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index,
> +					struct topology_resp *response)
>  {
>  	struct zynqmp_pm_query_data qdata = {0};
>  	u32 ret_payload[PAYLOAD_ARG_CNT];
> @@ -254,7 +266,7 @@ static int zynqmp_pm_clock_get_topology(u32 clock_id,
> u32 index, u32 *topology)
>  	qdata.arg2 = index;
> 
>  	ret = eemi_ops->query_data(qdata, ret_payload);
> -	memcpy(topology, &ret_payload[1],
> CLK_GET_TOPOLOGY_RESP_WORDS * 4);
> +	memcpy(response, &ret_payload[1], sizeof(*response));
> 
>  	return ret;
>  }
> @@ -303,7 +315,7 @@ struct clk_hw *zynqmp_clk_register_fixed_factor(const
> char *name, u32 clk_id,
>   * zynqmp_pm_clock_get_parents() - Get the first 3 parents of clock for given id
>   * @clock_id:	Clock ID
>   * @index:	Parent index
> - * @parents:	3 parents of the given clock
> + * @response:	Parents of the given clock
>   *
>   * This function is used to get 3 parents for the clock specified by
>   * given clock ID.
> @@ -316,7 +328,8 @@ struct clk_hw *zynqmp_clk_register_fixed_factor(const
> char *name, u32 clk_id,
>   *
>   * Return: 0 on success else error+reason
>   */
> -static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32
> *parents)
> +static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index,
> +				       struct parents_resp *response)
>  {
>  	struct zynqmp_pm_query_data qdata = {0};
>  	u32 ret_payload[PAYLOAD_ARG_CNT];
> @@ -327,7 +340,7 @@ static int zynqmp_pm_clock_get_parents(u32 clock_id,
> u32 index, u32 *parents)
>  	qdata.arg2 = index;
> 
>  	ret = eemi_ops->query_data(qdata, ret_payload);
> -	memcpy(parents, &ret_payload[1], CLK_GET_PARENTS_RESP_WORDS *
> 4);
> +	memcpy(response, &ret_payload[1], sizeof(*response));
> 
>  	return ret;
>  }
> @@ -335,13 +348,14 @@ static int zynqmp_pm_clock_get_parents(u32
> clock_id, u32 index, u32 *parents)
>  /**
>   * zynqmp_pm_clock_get_attributes() - Get the attributes of clock for given id
>   * @clock_id:	Clock ID
> - * @attr:	Clock attributes
> + * @response:	Clock attributes response
>   *
>   * This function is used to get clock's attributes(e.g. valid, clock type, etc).
>   *
>   * Return: 0 on success else error+reason
>   */
> -static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
> +static int zynqmp_pm_clock_get_attributes(u32 clock_id,
> +					  struct attr_resp *response)
>  {
>  	struct zynqmp_pm_query_data qdata = {0};
>  	u32 ret_payload[PAYLOAD_ARG_CNT];
> @@ -351,7 +365,7 @@ static int zynqmp_pm_clock_get_attributes(u32
> clock_id, u32 *attr)
>  	qdata.arg1 = clock_id;
> 
>  	ret = eemi_ops->query_data(qdata, ret_payload);
> -	memcpy(attr, &ret_payload[1], CLK_GET_ATTR_RESP_WORDS * 4);
> +	memcpy(response, &ret_payload[1], sizeof(*response));
> 
>  	return ret;
>  }
> @@ -360,24 +374,28 @@ static int zynqmp_pm_clock_get_attributes(u32
> clock_id, u32 *attr)
>   * __zynqmp_clock_get_topology() - Get topology data of clock from firmware
>   *				   response data
>   * @topology:		Clock topology
> - * @data:		Clock topology data received from firmware
> + * @response:		Clock topology data received from firmware
>   * @nnodes:		Number of nodes
>   *
>   * Return: 0 on success else error+reason
>   */
>  static int __zynqmp_clock_get_topology(struct clock_topology *topology,
> -				       u32 *data, u32 *nnodes)
> +				       struct topology_resp *response,
> +				       u32 *nnodes)
>  {
>  	int i;
> +	u32 type;
> 
> -	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
> -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> +	for (i = 0; i < ARRAY_SIZE(response->topology); i++) {
> +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, response-
> >topology[i]);
> +		if (type == TYPE_INVALID)
>  			return END_OF_TOPOLOGY_NODE;
> -		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
> -		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
> -						   data[i]);
> +		topology[*nnodes].type = type;
> +		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS,
> +						   response->topology[i]);
>  		topology[*nnodes].type_flag =
> -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK,
> data[i]);
> +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS,
> +					  response->topology[i]);
>  		(*nnodes)++;
>  	}
> 
> @@ -398,15 +416,16 @@ static int zynqmp_clock_get_topology(u32 clk_id,
>  				     u32 *num_nodes)
>  {
>  	int j, ret;
> -	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
> +	struct topology_resp response = {0};
> 
>  	*num_nodes = 0;
> -	for (j = 0; j <= MAX_NODES; j += 3) {
> +	for (j = 0; j <= MAX_NODES; j += ARRAY_SIZE(response.topology)) {
>  		ret = zynqmp_pm_clock_get_topology(clock[clk_id].clk_id, j,
> -						   pm_resp);
> +						   &response);
>  		if (ret)
>  			return ret;
> -		ret = __zynqmp_clock_get_topology(topology, pm_resp,
> num_nodes);
> +		ret = __zynqmp_clock_get_topology(topology, &response,
> +						  num_nodes);
>  		if (ret == END_OF_TOPOLOGY_NODE)
>  			return 0;
>  	}
> @@ -418,28 +437,30 @@ static int zynqmp_clock_get_topology(u32 clk_id,
>   * __zynqmp_clock_get_parents() - Get parents info of clock from firmware
>   *				   response data
>   * @parents:		Clock parents
> - * @data:		Clock parents data received from firmware
> + * @response:		Clock parents data received from firmware
>   * @nparent:		Number of parent
>   *
>   * Return: 0 on success else error+reason
>   */
> -static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32
> *data,
> +static int __zynqmp_clock_get_parents(struct clock_parent *parents,
> +				      struct parents_resp *response,
>  				      u32 *nparent)
>  {
>  	int i;
>  	struct clock_parent *parent;
> 
> -	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
> -		if (data[i] == NA_PARENT)
> +	for (i = 0; i < ARRAY_SIZE(response->parents); i++) {
> +		if (response->parents[i] == NA_PARENT)
>  			return END_OF_PARENTS;
> 
>  		parent = &parents[i];
> -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> -		if (data[i] == DUMMY_PARENT) {
> +		parent->id = FIELD_GET(CLK_PARENTS_ID, response-
> >parents[i]);
> +		if (response->parents[i] == DUMMY_PARENT) {
>  			strcpy(parent->name, "dummy_name");
>  			parent->flag = 0;
>  		} else {
> -			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
> +			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS,
> +						 response->parents[i]);
>  			if (zynqmp_get_clock_name(parent->id, parent-
> >name))
>  				continue;
>  		}
> @@ -461,21 +482,21 @@ static int zynqmp_clock_get_parents(u32 clk_id,
> struct clock_parent *parents,
>  				    u32 *num_parents)
>  {
>  	int j = 0, ret;
> -	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
> +	struct parents_resp response = {0};
> 
>  	*num_parents = 0;
>  	do {
>  		/* Get parents from firmware */
>  		ret = zynqmp_pm_clock_get_parents(clock[clk_id].clk_id, j,
> -						  pm_resp);
> +						  &response);
>  		if (ret)
>  			return ret;
> 
> -		ret = __zynqmp_clock_get_parents(&parents[j], pm_resp,
> +		ret = __zynqmp_clock_get_parents(&parents[j], &response,
>  						 num_parents);
>  		if (ret == END_OF_PARENTS)
>  			return 0;
> -		j += PM_API_PAYLOAD_LEN;
> +		j += ARRAY_SIZE(response.parents);
>  	} while (*num_parents <= MAX_PARENT);
> 
>  	return 0;
> @@ -631,26 +652,33 @@ static int zynqmp_register_clocks(struct device_node
> *np)
>  static void zynqmp_get_clock_info(void)
>  {
>  	int i, ret;
> -	u32 attr, type = 0, nodetype, subclass, class;
> +	u32 type = 0;
> +	u32 nodetype, subclass, class;
> +	struct attr_resp attr;
> +	struct name_resp name;
> 
>  	for (i = 0; i < clock_max_idx; i++) {
>  		ret = zynqmp_pm_clock_get_attributes(i, &attr);
>  		if (ret)
>  			continue;
> 
> -		clock[i].valid = attr & CLK_VALID_MASK;
> -		clock[i].type = ((attr >> CLK_TYPE_SHIFT) & 0x1) ?
> -				CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
> -		nodetype = (attr >> NODE_TYPE_SHIFT) & 0x3F;
> -		subclass = (attr >> NODE_SUBCLASS_SHIFT) & 0x3F;
> -		class = (attr >> NODE_CLASS_SHIFT) & 0x3F;
> +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr.attr[0]);
> +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE, attr.attr[0]) ?
> +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
> 
> -		clock[i].clk_id = (class << NODE_CLASS_SHIFT) |
> -				  (subclass << NODE_SUBCLASS_SHIFT) |
> -				  (nodetype << NODE_TYPE_SHIFT) |
> -				  (i << NODE_INDEX_SHIFT);
> +		nodetype = FIELD_GET(CLK_ATTR_NODE_TYPE, attr.attr[0]);
> +		subclass = FIELD_GET(CLK_ATTR_NODE_SUBCLASS,
> attr.attr[0]);
> +		class = FIELD_GET(CLK_ATTR_NODE_CLASS, attr.attr[0]);
> 
> -		zynqmp_pm_clock_get_name(clock[i].clk_id,
> clock[i].clk_name);
> +		clock[i].clk_id = FIELD_PREP(CLK_ATTR_NODE_CLASS, class) |
> +				  FIELD_PREP(CLK_ATTR_NODE_SUBCLASS,
> subclass) |
> +				  FIELD_PREP(CLK_ATTR_NODE_TYPE,
> nodetype) |
> +				  FIELD_PREP(CLK_ATTR_NODE_INDEX, i);
> +
> +		zynqmp_pm_clock_get_name(clock[i].clk_id, &name);
> +		if (!strcmp(name.name, RESERVED_CLK_NAME))
> +			continue;
> +		strncpy(clock[i].clk_name, name.name, MAX_NAME_LEN);
>  	}
> 
>  	/* Get topology of all clock */
> --
> 2.20.1
Stephen Boyd April 18, 2019, 8:39 p.m. UTC | #2
Quoting Jolly Shah (2019-04-18 13:01:32)
> Hi,
> 
> > -----Original Message-----
> > From: Michael Tretter <m.tretter@pengutronix.de>
> > Sent: Friday, April 12, 2019 2:52 AM
> > To: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> > Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>; Jolly
> > Shah <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> > Subject: [PATCH v3] clk: zynqmp: use structs for clk query responses
> > 
> > The driver retrieves the clock try by querying the ATF for the clock
> 
> *tree
> 
> Thanks,
> Jolly Shah

Is that a Reviewed-by?
Jolly Shah April 18, 2019, 9:15 p.m. UTC | #3
Hi Stephan,

> -----Original Message-----
> From: Stephen Boyd <sboyd@kernel.org>
> Sent: Thursday, April 18, 2019 1:39 PM
> To: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; Jolly Shah
> <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> Michal Simek <michals@xilinx.com>
> Subject: RE: [PATCH v3] clk: zynqmp: use structs for clk query responses
> 
> Quoting Jolly Shah (2019-04-18 13:01:32)
> > Hi,
> >
> > > -----Original Message-----
> > > From: Michael Tretter <m.tretter@pengutronix.de>
> > > Sent: Friday, April 12, 2019 2:52 AM
> > > To: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > > Cc: kernel@pengutronix.de; Michael Turquette
> <mturquette@baylibre.com>;
> > > Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>;
> Jolly
> > > Shah <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> > > Subject: [PATCH v3] clk: zynqmp: use structs for clk query responses
> > >
> > > The driver retrieves the clock try by querying the ATF for the clock
> >
> > *tree
> >
> > Thanks,
> > Jolly Shah
> 
> Is that a Reviewed-by?

Commit message has a typo, "try" instead of "tree". Rest looks good.

Thanks,
Jolly Shah
Stephen Boyd April 18, 2019, 10:02 p.m. UTC | #4
Quoting Jolly Shah (2019-04-18 14:15:24)
> Hi Stephan,
> 
> > -----Original Message-----
> > From: Stephen Boyd <sboyd@kernel.org>
> > Sent: Thursday, April 18, 2019 1:39 PM
> > To: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org; Jolly Shah
> > <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> > Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> > Michal Simek <michals@xilinx.com>
> > Subject: RE: [PATCH v3] clk: zynqmp: use structs for clk query responses
> > 
> > Quoting Jolly Shah (2019-04-18 13:01:32)
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: Michael Tretter <m.tretter@pengutronix.de>
> > > > Sent: Friday, April 12, 2019 2:52 AM
> > > > To: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > > > Cc: kernel@pengutronix.de; Michael Turquette
> > <mturquette@baylibre.com>;
> > > > Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>;
> > Jolly
> > > > Shah <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> > > > Subject: [PATCH v3] clk: zynqmp: use structs for clk query responses
> > > >
> > > > The driver retrieves the clock try by querying the ATF for the clock
> > >
> > > *tree
> > >
> > > Thanks,
> > > Jolly Shah
> > 
> > Is that a Reviewed-by?
> 
> Commit message has a typo, "try" instead of "tree". Rest looks good.
> 

Ok. Same question. With typo fixed can you provide your reviewed-by tag?
Jolly Shah April 19, 2019, 4:59 p.m. UTC | #5
With typo fix mentioned below,
Reviewed-by: Jolly Shah <jolly.shah@xilinx.com>

Thanks,
Jolly Shah


> -----Original Message-----
> From: Jolly Shah
> Sent: Thursday, April 18, 2019 1:02 PM
> To: Michael Tretter <m.tretter@pengutronix.de>; linux-clk@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>
> Subject: RE: [PATCH v3] clk: zynqmp: use structs for clk query responses
> 
> Hi,
> 
> > -----Original Message-----
> > From: Michael Tretter <m.tretter@pengutronix.de>
> > Sent: Friday, April 12, 2019 2:52 AM
> > To: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> > Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> > Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>; Jolly
> > Shah <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> > Subject: [PATCH v3] clk: zynqmp: use structs for clk query responses
> >
> > The driver retrieves the clock try by querying the ATF for the clock
> 
> *tree
> 
> Thanks,
> Jolly Shah
> 
> > names, the clock topology, the parents and other attributes. The driver
> > needs to unmarshal the responses.
> >
> > The definition of the fields in the firmware responses to the queries is
> > inconsistent. Some are specified as a mask, some as a shift, and by the
> > length of the previous field.
> >
> > Define C structs for the entire firmware responses to avoid passing
> > pointers to arrays of an implicit size and make the format of the
> > responses to the queries obvious.
> >
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> > v2 -> v3:
> > - fix conflicts with new format of get_attr response
> >
> > v1 -> v2:
> > - define structs for firmware responses
> > - fix size of CLK_TYPOLOGY_FLAGS
> > ---
> >  drivers/clk/zynqmp/clk-zynqmp.h |   6 --
> >  drivers/clk/zynqmp/clkc.c       | 170 +++++++++++++++++++-------------
> >  2 files changed, 99 insertions(+), 77 deletions(-)
> >
> > diff --git a/drivers/clk/zynqmp/clk-zynqmp.h b/drivers/clk/zynqmp/clk-
> zynqmp.h
> > index 7ab163b67249..fec9a15c8786 100644
> > --- a/drivers/clk/zynqmp/clk-zynqmp.h
> > +++ b/drivers/clk/zynqmp/clk-zynqmp.h
> > @@ -10,12 +10,6 @@
> >
> >  #include <linux/firmware/xlnx-zynqmp.h>
> >
> > -/* Clock APIs payload parameters */
> > -#define CLK_GET_NAME_RESP_LEN				16
> > -#define CLK_GET_TOPOLOGY_RESP_WORDS			3
> > -#define CLK_GET_PARENTS_RESP_WORDS			3
> > -#define CLK_GET_ATTR_RESP_WORDS				1
> > -
> >  enum topology_type {
> >  	TYPE_INVALID,
> >  	TYPE_MUX,
> > diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> > index 8b9d94a2e775..fb2dbd556f9e 100644
> > --- a/drivers/clk/zynqmp/clkc.c
> > +++ b/drivers/clk/zynqmp/clkc.c
> > @@ -21,24 +21,6 @@
> >  #define MAX_NODES			6
> >  #define MAX_NAME_LEN			50
> >
> > -#define CLK_TYPE_SHIFT			2
> > -
> > -#define PM_API_PAYLOAD_LEN		3
> > -
> > -#define NA_PARENT			0xFFFFFFFF
> > -#define DUMMY_PARENT			0xFFFFFFFE
> > -
> > -#define CLK_TYPE_FIELD_LEN		4
> > -#define CLK_TOPOLOGY_NODE_OFFSET	16
> > -#define NODES_PER_RESP			3
> > -
> > -#define CLK_TYPE_FIELD_MASK		0xF
> > -#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
> > -#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
> > -
> > -#define CLK_PARENTS_ID_LEN		16
> > -#define CLK_PARENTS_ID_MASK		0xFFFF
> > -
> >  /* Flags for parents */
> >  #define PARENT_CLK_SELF			0
> >  #define PARENT_CLK_NODE1		1
> > @@ -52,11 +34,10 @@
> >  #define END_OF_PARENTS			1
> >  #define RESERVED_CLK_NAME		""
> >
> > -#define CLK_VALID_MASK			0x1
> > -#define NODE_CLASS_SHIFT		26U
> > -#define NODE_SUBCLASS_SHIFT		20U
> > -#define NODE_TYPE_SHIFT			14U
> > -#define NODE_INDEX_SHIFT		0U
> > +#define CLK_GET_NAME_RESP_LEN		16
> > +#define CLK_GET_TOPOLOGY_RESP_WORDS	3
> > +#define CLK_GET_PARENTS_RESP_WORDS	3
> > +#define CLK_GET_ATTR_RESP_WORDS		1
> >
> >  enum clk_type {
> >  	CLK_TYPE_OUTPUT,
> > @@ -97,6 +78,35 @@ struct zynqmp_clock {
> >  	u32 clk_id;
> >  };
> >
> > +struct name_resp {
> > +	char name[CLK_GET_NAME_RESP_LEN];
> > +};
> > +
> > +struct topology_resp {
> > +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> > +#define CLK_TOPOLOGY_FLAGS		GENMASK(23, 8)
> > +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
> > +	u32 topology[CLK_GET_TOPOLOGY_RESP_WORDS];
> > +};
> > +
> > +struct parents_resp {
> > +#define NA_PARENT			0xFFFFFFFF
> > +#define DUMMY_PARENT			0xFFFFFFFE
> > +#define CLK_PARENTS_ID			GENMASK(15, 0)
> > +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
> > +	u32 parents[CLK_GET_PARENTS_RESP_WORDS];
> > +};
> > +
> > +struct attr_resp {
> > +#define CLK_ATTR_VALID			BIT(0)
> > +#define CLK_ATTR_TYPE			BIT(2)
> > +#define CLK_ATTR_NODE_INDEX		GENMASK(13, 0)
> > +#define CLK_ATTR_NODE_TYPE		GENMASK(19, 14)
> > +#define CLK_ATTR_NODE_SUBCLASS		GENMASK(25, 20)
> > +#define CLK_ATTR_NODE_CLASS		GENMASK(31, 26)
> > +	u32 attr[CLK_GET_ATTR_RESP_WORDS];
> > +};
> > +
> >  static const char clk_type_postfix[][10] = {
> >  	[TYPE_INVALID] = "",
> >  	[TYPE_MUX] = "_mux",
> > @@ -205,14 +215,15 @@ static int zynqmp_pm_clock_get_num_clocks(u32
> > *nclocks)
> >  /**
> >   * zynqmp_pm_clock_get_name() - Get the name of clock for given id
> >   * @clock_id:	ID of the clock to be queried
> > - * @name:	Name of given clock
> > + * @response:	Name of the clock with the given id
> >   *
> >   * This function is used to get name of clock specified by given
> >   * clock ID.
> >   *
> > - * Return: Returns 0, in case of error name would be 0
> > + * Return: Returns 0
> >   */
> > -static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
> > +static int zynqmp_pm_clock_get_name(u32 clock_id,
> > +				    struct name_resp *response)
> >  {
> >  	struct zynqmp_pm_query_data qdata = {0};
> >  	u32 ret_payload[PAYLOAD_ARG_CNT];
> > @@ -221,7 +232,7 @@ static int zynqmp_pm_clock_get_name(u32 clock_id,
> > char *name)
> >  	qdata.arg1 = clock_id;
> >
> >  	eemi_ops->query_data(qdata, ret_payload);
> > -	memcpy(name, ret_payload, CLK_GET_NAME_RESP_LEN);
> > +	memcpy(response, ret_payload, sizeof(*response));
> >
> >  	return 0;
> >  }
> > @@ -230,7 +241,7 @@ static int zynqmp_pm_clock_get_name(u32 clock_id,
> > char *name)
> >   * zynqmp_pm_clock_get_topology() - Get the topology of clock for given id
> >   * @clock_id:	ID of the clock to be queried
> >   * @index:	Node index of clock topology
> > - * @topology:	Buffer to store nodes in topology and flags
> > + * @response:	Buffer used for the topology response
> >   *
> >   * This function is used to get topology information for the clock
> >   * specified by given clock ID.
> > @@ -243,7 +254,8 @@ static int zynqmp_pm_clock_get_name(u32 clock_id,
> > char *name)
> >   *
> >   * Return: 0 on success else error+reason
> >   */
> > -static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32
> > *topology)
> > +static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index,
> > +					struct topology_resp *response)
> >  {
> >  	struct zynqmp_pm_query_data qdata = {0};
> >  	u32 ret_payload[PAYLOAD_ARG_CNT];
> > @@ -254,7 +266,7 @@ static int zynqmp_pm_clock_get_topology(u32
> clock_id,
> > u32 index, u32 *topology)
> >  	qdata.arg2 = index;
> >
> >  	ret = eemi_ops->query_data(qdata, ret_payload);
> > -	memcpy(topology, &ret_payload[1],
> > CLK_GET_TOPOLOGY_RESP_WORDS * 4);
> > +	memcpy(response, &ret_payload[1], sizeof(*response));
> >
> >  	return ret;
> >  }
> > @@ -303,7 +315,7 @@ struct clk_hw
> *zynqmp_clk_register_fixed_factor(const
> > char *name, u32 clk_id,
> >   * zynqmp_pm_clock_get_parents() - Get the first 3 parents of clock for given
> id
> >   * @clock_id:	Clock ID
> >   * @index:	Parent index
> > - * @parents:	3 parents of the given clock
> > + * @response:	Parents of the given clock
> >   *
> >   * This function is used to get 3 parents for the clock specified by
> >   * given clock ID.
> > @@ -316,7 +328,8 @@ struct clk_hw
> *zynqmp_clk_register_fixed_factor(const
> > char *name, u32 clk_id,
> >   *
> >   * Return: 0 on success else error+reason
> >   */
> > -static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32
> > *parents)
> > +static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index,
> > +				       struct parents_resp *response)
> >  {
> >  	struct zynqmp_pm_query_data qdata = {0};
> >  	u32 ret_payload[PAYLOAD_ARG_CNT];
> > @@ -327,7 +340,7 @@ static int zynqmp_pm_clock_get_parents(u32
> clock_id,
> > u32 index, u32 *parents)
> >  	qdata.arg2 = index;
> >
> >  	ret = eemi_ops->query_data(qdata, ret_payload);
> > -	memcpy(parents, &ret_payload[1], CLK_GET_PARENTS_RESP_WORDS *
> > 4);
> > +	memcpy(response, &ret_payload[1], sizeof(*response));
> >
> >  	return ret;
> >  }
> > @@ -335,13 +348,14 @@ static int zynqmp_pm_clock_get_parents(u32
> > clock_id, u32 index, u32 *parents)
> >  /**
> >   * zynqmp_pm_clock_get_attributes() - Get the attributes of clock for given id
> >   * @clock_id:	Clock ID
> > - * @attr:	Clock attributes
> > + * @response:	Clock attributes response
> >   *
> >   * This function is used to get clock's attributes(e.g. valid, clock type, etc).
> >   *
> >   * Return: 0 on success else error+reason
> >   */
> > -static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
> > +static int zynqmp_pm_clock_get_attributes(u32 clock_id,
> > +					  struct attr_resp *response)
> >  {
> >  	struct zynqmp_pm_query_data qdata = {0};
> >  	u32 ret_payload[PAYLOAD_ARG_CNT];
> > @@ -351,7 +365,7 @@ static int zynqmp_pm_clock_get_attributes(u32
> > clock_id, u32 *attr)
> >  	qdata.arg1 = clock_id;
> >
> >  	ret = eemi_ops->query_data(qdata, ret_payload);
> > -	memcpy(attr, &ret_payload[1], CLK_GET_ATTR_RESP_WORDS * 4);
> > +	memcpy(response, &ret_payload[1], sizeof(*response));
> >
> >  	return ret;
> >  }
> > @@ -360,24 +374,28 @@ static int zynqmp_pm_clock_get_attributes(u32
> > clock_id, u32 *attr)
> >   * __zynqmp_clock_get_topology() - Get topology data of clock from
> firmware
> >   *				   response data
> >   * @topology:		Clock topology
> > - * @data:		Clock topology data received from firmware
> > + * @response:		Clock topology data received from firmware
> >   * @nnodes:		Number of nodes
> >   *
> >   * Return: 0 on success else error+reason
> >   */
> >  static int __zynqmp_clock_get_topology(struct clock_topology *topology,
> > -				       u32 *data, u32 *nnodes)
> > +				       struct topology_resp *response,
> > +				       u32 *nnodes)
> >  {
> >  	int i;
> > +	u32 type;
> >
> > -	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
> > -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> > +	for (i = 0; i < ARRAY_SIZE(response->topology); i++) {
> > +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, response-
> > >topology[i]);
> > +		if (type == TYPE_INVALID)
> >  			return END_OF_TOPOLOGY_NODE;
> > -		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
> > -		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
> > -						   data[i]);
> > +		topology[*nnodes].type = type;
> > +		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS,
> > +						   response->topology[i]);
> >  		topology[*nnodes].type_flag =
> > -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK,
> > data[i]);
> > +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS,
> > +					  response->topology[i]);
> >  		(*nnodes)++;
> >  	}
> >
> > @@ -398,15 +416,16 @@ static int zynqmp_clock_get_topology(u32 clk_id,
> >  				     u32 *num_nodes)
> >  {
> >  	int j, ret;
> > -	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
> > +	struct topology_resp response = {0};
> >
> >  	*num_nodes = 0;
> > -	for (j = 0; j <= MAX_NODES; j += 3) {
> > +	for (j = 0; j <= MAX_NODES; j += ARRAY_SIZE(response.topology)) {
> >  		ret = zynqmp_pm_clock_get_topology(clock[clk_id].clk_id, j,
> > -						   pm_resp);
> > +						   &response);
> >  		if (ret)
> >  			return ret;
> > -		ret = __zynqmp_clock_get_topology(topology, pm_resp,
> > num_nodes);
> > +		ret = __zynqmp_clock_get_topology(topology, &response,
> > +						  num_nodes);
> >  		if (ret == END_OF_TOPOLOGY_NODE)
> >  			return 0;
> >  	}
> > @@ -418,28 +437,30 @@ static int zynqmp_clock_get_topology(u32 clk_id,
> >   * __zynqmp_clock_get_parents() - Get parents info of clock from firmware
> >   *				   response data
> >   * @parents:		Clock parents
> > - * @data:		Clock parents data received from firmware
> > + * @response:		Clock parents data received from firmware
> >   * @nparent:		Number of parent
> >   *
> >   * Return: 0 on success else error+reason
> >   */
> > -static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32
> > *data,
> > +static int __zynqmp_clock_get_parents(struct clock_parent *parents,
> > +				      struct parents_resp *response,
> >  				      u32 *nparent)
> >  {
> >  	int i;
> >  	struct clock_parent *parent;
> >
> > -	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
> > -		if (data[i] == NA_PARENT)
> > +	for (i = 0; i < ARRAY_SIZE(response->parents); i++) {
> > +		if (response->parents[i] == NA_PARENT)
> >  			return END_OF_PARENTS;
> >
> >  		parent = &parents[i];
> > -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> > -		if (data[i] == DUMMY_PARENT) {
> > +		parent->id = FIELD_GET(CLK_PARENTS_ID, response-
> > >parents[i]);
> > +		if (response->parents[i] == DUMMY_PARENT) {
> >  			strcpy(parent->name, "dummy_name");
> >  			parent->flag = 0;
> >  		} else {
> > -			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
> > +			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS,
> > +						 response->parents[i]);
> >  			if (zynqmp_get_clock_name(parent->id, parent-
> > >name))
> >  				continue;
> >  		}
> > @@ -461,21 +482,21 @@ static int zynqmp_clock_get_parents(u32 clk_id,
> > struct clock_parent *parents,
> >  				    u32 *num_parents)
> >  {
> >  	int j = 0, ret;
> > -	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
> > +	struct parents_resp response = {0};
> >
> >  	*num_parents = 0;
> >  	do {
> >  		/* Get parents from firmware */
> >  		ret = zynqmp_pm_clock_get_parents(clock[clk_id].clk_id, j,
> > -						  pm_resp);
> > +						  &response);
> >  		if (ret)
> >  			return ret;
> >
> > -		ret = __zynqmp_clock_get_parents(&parents[j], pm_resp,
> > +		ret = __zynqmp_clock_get_parents(&parents[j], &response,
> >  						 num_parents);
> >  		if (ret == END_OF_PARENTS)
> >  			return 0;
> > -		j += PM_API_PAYLOAD_LEN;
> > +		j += ARRAY_SIZE(response.parents);
> >  	} while (*num_parents <= MAX_PARENT);
> >
> >  	return 0;
> > @@ -631,26 +652,33 @@ static int zynqmp_register_clocks(struct
> device_node
> > *np)
> >  static void zynqmp_get_clock_info(void)
> >  {
> >  	int i, ret;
> > -	u32 attr, type = 0, nodetype, subclass, class;
> > +	u32 type = 0;
> > +	u32 nodetype, subclass, class;
> > +	struct attr_resp attr;
> > +	struct name_resp name;
> >
> >  	for (i = 0; i < clock_max_idx; i++) {
> >  		ret = zynqmp_pm_clock_get_attributes(i, &attr);
> >  		if (ret)
> >  			continue;
> >
> > -		clock[i].valid = attr & CLK_VALID_MASK;
> > -		clock[i].type = ((attr >> CLK_TYPE_SHIFT) & 0x1) ?
> > -				CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
> > -		nodetype = (attr >> NODE_TYPE_SHIFT) & 0x3F;
> > -		subclass = (attr >> NODE_SUBCLASS_SHIFT) & 0x3F;
> > -		class = (attr >> NODE_CLASS_SHIFT) & 0x3F;
> > +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr.attr[0]);
> > +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE, attr.attr[0]) ?
> > +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
> >
> > -		clock[i].clk_id = (class << NODE_CLASS_SHIFT) |
> > -				  (subclass << NODE_SUBCLASS_SHIFT) |
> > -				  (nodetype << NODE_TYPE_SHIFT) |
> > -				  (i << NODE_INDEX_SHIFT);
> > +		nodetype = FIELD_GET(CLK_ATTR_NODE_TYPE, attr.attr[0]);
> > +		subclass = FIELD_GET(CLK_ATTR_NODE_SUBCLASS,
> > attr.attr[0]);
> > +		class = FIELD_GET(CLK_ATTR_NODE_CLASS, attr.attr[0]);
> >
> > -		zynqmp_pm_clock_get_name(clock[i].clk_id,
> > clock[i].clk_name);
> > +		clock[i].clk_id = FIELD_PREP(CLK_ATTR_NODE_CLASS, class) |
> > +				  FIELD_PREP(CLK_ATTR_NODE_SUBCLASS,
> > subclass) |
> > +				  FIELD_PREP(CLK_ATTR_NODE_TYPE,
> > nodetype) |
> > +				  FIELD_PREP(CLK_ATTR_NODE_INDEX, i);
> > +
> > +		zynqmp_pm_clock_get_name(clock[i].clk_id, &name);
> > +		if (!strcmp(name.name, RESERVED_CLK_NAME))
> > +			continue;
> > +		strncpy(clock[i].clk_name, name.name, MAX_NAME_LEN);
> >  	}
> >
> >  	/* Get topology of all clock */
> > --
> > 2.20.1
Stephen Boyd April 19, 2019, 9:22 p.m. UTC | #6
Quoting Michael Tretter (2019-04-12 02:52:20)
> The driver retrieves the clock try by querying the ATF for the clock
> names, the clock topology, the parents and other attributes. The driver
> needs to unmarshal the responses.
> 
> The definition of the fields in the firmware responses to the queries is
> inconsistent. Some are specified as a mask, some as a shift, and by the
> length of the previous field.
> 
> Define C structs for the entire firmware responses to avoid passing
> pointers to arrays of an implicit size and make the format of the
> responses to the queries obvious.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---

Applied to clk-next but I had to apply this to make sparse happy.

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index fb2dbd556f9e..8febd2431545 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -416,7 +416,7 @@ static int zynqmp_clock_get_topology(u32 clk_id,
 				     u32 *num_nodes)
 {
 	int j, ret;
-	struct topology_resp response = {0};
+	struct topology_resp response = { };
 
 	*num_nodes = 0;
 	for (j = 0; j <= MAX_NODES; j += ARRAY_SIZE(response.topology)) {
@@ -482,7 +482,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
 				    u32 *num_parents)
 {
 	int j = 0, ret;
-	struct parents_resp response = {0};
+	struct parents_resp response = { };
 
 	*num_parents = 0;
 	do {


Also, I was hoping to see the patch go further and make structures
instead of doing FIELD_GET on u32s. For example:

#define CLK_PARENTS_ID                  GENMASK(15, 0)
#define CLK_PARENTS_FLAGS               GENMASK(31, 16)

This could be a struct of two u16 in them (and quite possibly __le16).

	u16 id;
	u16 flags;


Similarly:

#define CLK_TOPOLOGY_TYPE               GENMASK(3, 0)
#define CLK_TOPOLOGY_FLAGS              GENMASK(23, 8)
#define CLK_TOPOLOGY_TYPE_FLAGS         GENMASK(31, 24)

	u8 type;
	u16 flags;
	u8 typeflags;

with __packed on it. This one is weird though:

#define CLK_ATTR_NODE_INDEX             GENMASK(13, 0)
#define CLK_ATTR_NODE_TYPE              GENMASK(19, 14)
#define CLK_ATTR_NODE_SUBCLASS          GENMASK(25, 20)
#define CLK_ATTR_NODE_CLASS             GENMASK(31, 26)

The bits are really packed in there. I guess we can't win this one.
Michael Tretter April 23, 2019, 1:54 p.m. UTC | #7
On Fri, 19 Apr 2019 14:22:19 -0700, Stephen Boyd wrote:
> Quoting Michael Tretter (2019-04-12 02:52:20)
> > The driver retrieves the clock try by querying the ATF for the clock
> > names, the clock topology, the parents and other attributes. The driver
> > needs to unmarshal the responses.
> > 
> > The definition of the fields in the firmware responses to the queries is
> > inconsistent. Some are specified as a mask, some as a shift, and by the
> > length of the previous field.
> > 
> > Define C structs for the entire firmware responses to avoid passing
> > pointers to arrays of an implicit size and make the format of the
> > responses to the queries obvious.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---  
> 
> Applied to clk-next but I had to apply this to make sparse happy.

Thanks.

> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index fb2dbd556f9e..8febd2431545 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -416,7 +416,7 @@ static int zynqmp_clock_get_topology(u32 clk_id,
>  				     u32 *num_nodes)
>  {
>  	int j, ret;
> -	struct topology_resp response = {0};
> +	struct topology_resp response = { };
>  
>  	*num_nodes = 0;
>  	for (j = 0; j <= MAX_NODES; j += ARRAY_SIZE(response.topology)) {
> @@ -482,7 +482,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
>  				    u32 *num_parents)
>  {
>  	int j = 0, ret;
> -	struct parents_resp response = {0};
> +	struct parents_resp response = { };
>  
>  	*num_parents = 0;
>  	do {
> 
> 
> Also, I was hoping to see the patch go further and make structures
> instead of doing FIELD_GET on u32s. For example:
> 
> #define CLK_PARENTS_ID                  GENMASK(15, 0)
> #define CLK_PARENTS_FLAGS               GENMASK(31, 16)
> 
> This could be a struct of two u16 in them (and quite possibly __le16).
> 
> 	u16 id;
> 	u16 flags;

The error codes also do not fit into this scheme. For example, if no
further parents are available, the response will contain the value
0xFFFFFFFF in the response. The value may occur at any position in the
parents array and should not be interpreted as a struct with two u16 in
it, but should be seen as a single value on its own.

Therefore, I think the abstraction using a u32[] and bitfields is better
than defining a struct for unmarshalling the response.

> 
> Similarly:
> 
> #define CLK_TOPOLOGY_TYPE               GENMASK(3, 0)
> #define CLK_TOPOLOGY_FLAGS              GENMASK(23, 8)
> #define CLK_TOPOLOGY_TYPE_FLAGS         GENMASK(31, 24)
> 
> 	u8 type;
> 	u16 flags;
> 	u8 typeflags;
> 
> with __packed on it. This one is weird though:
> 
> #define CLK_ATTR_NODE_INDEX             GENMASK(13, 0)
> #define CLK_ATTR_NODE_TYPE              GENMASK(19, 14)
> #define CLK_ATTR_NODE_SUBCLASS          GENMASK(25, 20)
> #define CLK_ATTR_NODE_CLASS             GENMASK(31, 26)
> 
> The bits are really packed in there. I guess we can't win this one.

Actually, the CLK_ATTR_NODE_INDEX is not a field in the response, but a
field in the clock id. The driver needs to use the index from the query
to set the index field in the clock id. Only the bits 2 and 0 in bits
13..0 carry actual meaning.

Not sure if I should amend this patch or have a separate patch for
that, because this is in fact an issue in the "drivers: clk: Update
clock driver to handle clock attribute" patch.

Michael
Stephen Boyd April 23, 2019, 5:38 p.m. UTC | #8
Quoting Michael Tretter (2019-04-23 06:54:18)
> On Fri, 19 Apr 2019 14:22:19 -0700, Stephen Boyd wrote:
> > Also, I was hoping to see the patch go further and make structures
> > instead of doing FIELD_GET on u32s. For example:
> > 
> > #define CLK_PARENTS_ID                  GENMASK(15, 0)
> > #define CLK_PARENTS_FLAGS               GENMASK(31, 16)
> > 
> > This could be a struct of two u16 in them (and quite possibly __le16).
> > 
> >       u16 id;
> >       u16 flags;
> 
> The error codes also do not fit into this scheme. For example, if no
> further parents are available, the response will contain the value
> 0xFFFFFFFF in the response. The value may occur at any position in the
> parents array and should not be interpreted as a struct with two u16 in
> it, but should be seen as a single value on its own.
> 
> Therefore, I think the abstraction using a u32[] and bitfields is better
> than defining a struct for unmarshalling the response.

We have union for that?

	union {
		struct resp {
			u16 id;
			u16 flags;
		};
		u32 error;
	};

> 
> > 
> > with __packed on it. This one is weird though:
> > 
> > #define CLK_ATTR_NODE_INDEX             GENMASK(13, 0)
> > #define CLK_ATTR_NODE_TYPE              GENMASK(19, 14)
> > #define CLK_ATTR_NODE_SUBCLASS          GENMASK(25, 20)
> > #define CLK_ATTR_NODE_CLASS             GENMASK(31, 26)
> > 
> > The bits are really packed in there. I guess we can't win this one.
> 
> Actually, the CLK_ATTR_NODE_INDEX is not a field in the response, but a
> field in the clock id. The driver needs to use the index from the query
> to set the index field in the clock id. Only the bits 2 and 0 in bits
> 13..0 carry actual meaning.
> 
> Not sure if I should amend this patch or have a separate patch for
> that, because this is in fact an issue in the "drivers: clk: Update
> clock driver to handle clock attribute" patch.
> 

I'd just send a follow-up patch if something is wrong.
diff mbox series

Patch

diff --git a/drivers/clk/zynqmp/clk-zynqmp.h b/drivers/clk/zynqmp/clk-zynqmp.h
index 7ab163b67249..fec9a15c8786 100644
--- a/drivers/clk/zynqmp/clk-zynqmp.h
+++ b/drivers/clk/zynqmp/clk-zynqmp.h
@@ -10,12 +10,6 @@ 
 
 #include <linux/firmware/xlnx-zynqmp.h>
 
-/* Clock APIs payload parameters */
-#define CLK_GET_NAME_RESP_LEN				16
-#define CLK_GET_TOPOLOGY_RESP_WORDS			3
-#define CLK_GET_PARENTS_RESP_WORDS			3
-#define CLK_GET_ATTR_RESP_WORDS				1
-
 enum topology_type {
 	TYPE_INVALID,
 	TYPE_MUX,
diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index 8b9d94a2e775..fb2dbd556f9e 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -21,24 +21,6 @@ 
 #define MAX_NODES			6
 #define MAX_NAME_LEN			50
 
-#define CLK_TYPE_SHIFT			2
-
-#define PM_API_PAYLOAD_LEN		3
-
-#define NA_PARENT			0xFFFFFFFF
-#define DUMMY_PARENT			0xFFFFFFFE
-
-#define CLK_TYPE_FIELD_LEN		4
-#define CLK_TOPOLOGY_NODE_OFFSET	16
-#define NODES_PER_RESP			3
-
-#define CLK_TYPE_FIELD_MASK		0xF
-#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
-#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
-
-#define CLK_PARENTS_ID_LEN		16
-#define CLK_PARENTS_ID_MASK		0xFFFF
-
 /* Flags for parents */
 #define PARENT_CLK_SELF			0
 #define PARENT_CLK_NODE1		1
@@ -52,11 +34,10 @@ 
 #define END_OF_PARENTS			1
 #define RESERVED_CLK_NAME		""
 
-#define CLK_VALID_MASK			0x1
-#define NODE_CLASS_SHIFT		26U
-#define NODE_SUBCLASS_SHIFT		20U
-#define NODE_TYPE_SHIFT			14U
-#define NODE_INDEX_SHIFT		0U
+#define CLK_GET_NAME_RESP_LEN		16
+#define CLK_GET_TOPOLOGY_RESP_WORDS	3
+#define CLK_GET_PARENTS_RESP_WORDS	3
+#define CLK_GET_ATTR_RESP_WORDS		1
 
 enum clk_type {
 	CLK_TYPE_OUTPUT,
@@ -97,6 +78,35 @@  struct zynqmp_clock {
 	u32 clk_id;
 };
 
+struct name_resp {
+	char name[CLK_GET_NAME_RESP_LEN];
+};
+
+struct topology_resp {
+#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
+#define CLK_TOPOLOGY_FLAGS		GENMASK(23, 8)
+#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
+	u32 topology[CLK_GET_TOPOLOGY_RESP_WORDS];
+};
+
+struct parents_resp {
+#define NA_PARENT			0xFFFFFFFF
+#define DUMMY_PARENT			0xFFFFFFFE
+#define CLK_PARENTS_ID			GENMASK(15, 0)
+#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
+	u32 parents[CLK_GET_PARENTS_RESP_WORDS];
+};
+
+struct attr_resp {
+#define CLK_ATTR_VALID			BIT(0)
+#define CLK_ATTR_TYPE			BIT(2)
+#define CLK_ATTR_NODE_INDEX		GENMASK(13, 0)
+#define CLK_ATTR_NODE_TYPE		GENMASK(19, 14)
+#define CLK_ATTR_NODE_SUBCLASS		GENMASK(25, 20)
+#define CLK_ATTR_NODE_CLASS		GENMASK(31, 26)
+	u32 attr[CLK_GET_ATTR_RESP_WORDS];
+};
+
 static const char clk_type_postfix[][10] = {
 	[TYPE_INVALID] = "",
 	[TYPE_MUX] = "_mux",
@@ -205,14 +215,15 @@  static int zynqmp_pm_clock_get_num_clocks(u32 *nclocks)
 /**
  * zynqmp_pm_clock_get_name() - Get the name of clock for given id
  * @clock_id:	ID of the clock to be queried
- * @name:	Name of given clock
+ * @response:	Name of the clock with the given id
  *
  * This function is used to get name of clock specified by given
  * clock ID.
  *
- * Return: Returns 0, in case of error name would be 0
+ * Return: Returns 0
  */
-static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
+static int zynqmp_pm_clock_get_name(u32 clock_id,
+				    struct name_resp *response)
 {
 	struct zynqmp_pm_query_data qdata = {0};
 	u32 ret_payload[PAYLOAD_ARG_CNT];
@@ -221,7 +232,7 @@  static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
 	qdata.arg1 = clock_id;
 
 	eemi_ops->query_data(qdata, ret_payload);
-	memcpy(name, ret_payload, CLK_GET_NAME_RESP_LEN);
+	memcpy(response, ret_payload, sizeof(*response));
 
 	return 0;
 }
@@ -230,7 +241,7 @@  static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
  * zynqmp_pm_clock_get_topology() - Get the topology of clock for given id
  * @clock_id:	ID of the clock to be queried
  * @index:	Node index of clock topology
- * @topology:	Buffer to store nodes in topology and flags
+ * @response:	Buffer used for the topology response
  *
  * This function is used to get topology information for the clock
  * specified by given clock ID.
@@ -243,7 +254,8 @@  static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
  *
  * Return: 0 on success else error+reason
  */
-static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32 *topology)
+static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index,
+					struct topology_resp *response)
 {
 	struct zynqmp_pm_query_data qdata = {0};
 	u32 ret_payload[PAYLOAD_ARG_CNT];
@@ -254,7 +266,7 @@  static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32 *topology)
 	qdata.arg2 = index;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(topology, &ret_payload[1], CLK_GET_TOPOLOGY_RESP_WORDS * 4);
+	memcpy(response, &ret_payload[1], sizeof(*response));
 
 	return ret;
 }
@@ -303,7 +315,7 @@  struct clk_hw *zynqmp_clk_register_fixed_factor(const char *name, u32 clk_id,
  * zynqmp_pm_clock_get_parents() - Get the first 3 parents of clock for given id
  * @clock_id:	Clock ID
  * @index:	Parent index
- * @parents:	3 parents of the given clock
+ * @response:	Parents of the given clock
  *
  * This function is used to get 3 parents for the clock specified by
  * given clock ID.
@@ -316,7 +328,8 @@  struct clk_hw *zynqmp_clk_register_fixed_factor(const char *name, u32 clk_id,
  *
  * Return: 0 on success else error+reason
  */
-static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32 *parents)
+static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index,
+				       struct parents_resp *response)
 {
 	struct zynqmp_pm_query_data qdata = {0};
 	u32 ret_payload[PAYLOAD_ARG_CNT];
@@ -327,7 +340,7 @@  static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32 *parents)
 	qdata.arg2 = index;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(parents, &ret_payload[1], CLK_GET_PARENTS_RESP_WORDS * 4);
+	memcpy(response, &ret_payload[1], sizeof(*response));
 
 	return ret;
 }
@@ -335,13 +348,14 @@  static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32 *parents)
 /**
  * zynqmp_pm_clock_get_attributes() - Get the attributes of clock for given id
  * @clock_id:	Clock ID
- * @attr:	Clock attributes
+ * @response:	Clock attributes response
  *
  * This function is used to get clock's attributes(e.g. valid, clock type, etc).
  *
  * Return: 0 on success else error+reason
  */
-static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
+static int zynqmp_pm_clock_get_attributes(u32 clock_id,
+					  struct attr_resp *response)
 {
 	struct zynqmp_pm_query_data qdata = {0};
 	u32 ret_payload[PAYLOAD_ARG_CNT];
@@ -351,7 +365,7 @@  static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
 	qdata.arg1 = clock_id;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(attr, &ret_payload[1], CLK_GET_ATTR_RESP_WORDS * 4);
+	memcpy(response, &ret_payload[1], sizeof(*response));
 
 	return ret;
 }
@@ -360,24 +374,28 @@  static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
  * __zynqmp_clock_get_topology() - Get topology data of clock from firmware
  *				   response data
  * @topology:		Clock topology
- * @data:		Clock topology data received from firmware
+ * @response:		Clock topology data received from firmware
  * @nnodes:		Number of nodes
  *
  * Return: 0 on success else error+reason
  */
 static int __zynqmp_clock_get_topology(struct clock_topology *topology,
-				       u32 *data, u32 *nnodes)
+				       struct topology_resp *response,
+				       u32 *nnodes)
 {
 	int i;
+	u32 type;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
-		if (!(data[i] & CLK_TYPE_FIELD_MASK))
+	for (i = 0; i < ARRAY_SIZE(response->topology); i++) {
+		type = FIELD_GET(CLK_TOPOLOGY_TYPE, response->topology[i]);
+		if (type == TYPE_INVALID)
 			return END_OF_TOPOLOGY_NODE;
-		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
-		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
-						   data[i]);
+		topology[*nnodes].type = type;
+		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS,
+						   response->topology[i]);
 		topology[*nnodes].type_flag =
-				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK, data[i]);
+				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS,
+					  response->topology[i]);
 		(*nnodes)++;
 	}
 
@@ -398,15 +416,16 @@  static int zynqmp_clock_get_topology(u32 clk_id,
 				     u32 *num_nodes)
 {
 	int j, ret;
-	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+	struct topology_resp response = {0};
 
 	*num_nodes = 0;
-	for (j = 0; j <= MAX_NODES; j += 3) {
+	for (j = 0; j <= MAX_NODES; j += ARRAY_SIZE(response.topology)) {
 		ret = zynqmp_pm_clock_get_topology(clock[clk_id].clk_id, j,
-						   pm_resp);
+						   &response);
 		if (ret)
 			return ret;
-		ret = __zynqmp_clock_get_topology(topology, pm_resp, num_nodes);
+		ret = __zynqmp_clock_get_topology(topology, &response,
+						  num_nodes);
 		if (ret == END_OF_TOPOLOGY_NODE)
 			return 0;
 	}
@@ -418,28 +437,30 @@  static int zynqmp_clock_get_topology(u32 clk_id,
  * __zynqmp_clock_get_parents() - Get parents info of clock from firmware
  *				   response data
  * @parents:		Clock parents
- * @data:		Clock parents data received from firmware
+ * @response:		Clock parents data received from firmware
  * @nparent:		Number of parent
  *
  * Return: 0 on success else error+reason
  */
-static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
+static int __zynqmp_clock_get_parents(struct clock_parent *parents,
+				      struct parents_resp *response,
 				      u32 *nparent)
 {
 	int i;
 	struct clock_parent *parent;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
-		if (data[i] == NA_PARENT)
+	for (i = 0; i < ARRAY_SIZE(response->parents); i++) {
+		if (response->parents[i] == NA_PARENT)
 			return END_OF_PARENTS;
 
 		parent = &parents[i];
-		parent->id = data[i] & CLK_PARENTS_ID_MASK;
-		if (data[i] == DUMMY_PARENT) {
+		parent->id = FIELD_GET(CLK_PARENTS_ID, response->parents[i]);
+		if (response->parents[i] == DUMMY_PARENT) {
 			strcpy(parent->name, "dummy_name");
 			parent->flag = 0;
 		} else {
-			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
+			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS,
+						 response->parents[i]);
 			if (zynqmp_get_clock_name(parent->id, parent->name))
 				continue;
 		}
@@ -461,21 +482,21 @@  static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
 				    u32 *num_parents)
 {
 	int j = 0, ret;
-	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+	struct parents_resp response = {0};
 
 	*num_parents = 0;
 	do {
 		/* Get parents from firmware */
 		ret = zynqmp_pm_clock_get_parents(clock[clk_id].clk_id, j,
-						  pm_resp);
+						  &response);
 		if (ret)
 			return ret;
 
-		ret = __zynqmp_clock_get_parents(&parents[j], pm_resp,
+		ret = __zynqmp_clock_get_parents(&parents[j], &response,
 						 num_parents);
 		if (ret == END_OF_PARENTS)
 			return 0;
-		j += PM_API_PAYLOAD_LEN;
+		j += ARRAY_SIZE(response.parents);
 	} while (*num_parents <= MAX_PARENT);
 
 	return 0;
@@ -631,26 +652,33 @@  static int zynqmp_register_clocks(struct device_node *np)
 static void zynqmp_get_clock_info(void)
 {
 	int i, ret;
-	u32 attr, type = 0, nodetype, subclass, class;
+	u32 type = 0;
+	u32 nodetype, subclass, class;
+	struct attr_resp attr;
+	struct name_resp name;
 
 	for (i = 0; i < clock_max_idx; i++) {
 		ret = zynqmp_pm_clock_get_attributes(i, &attr);
 		if (ret)
 			continue;
 
-		clock[i].valid = attr & CLK_VALID_MASK;
-		clock[i].type = ((attr >> CLK_TYPE_SHIFT) & 0x1) ?
-				CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
-		nodetype = (attr >> NODE_TYPE_SHIFT) & 0x3F;
-		subclass = (attr >> NODE_SUBCLASS_SHIFT) & 0x3F;
-		class = (attr >> NODE_CLASS_SHIFT) & 0x3F;
+		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr.attr[0]);
+		clock[i].type = FIELD_GET(CLK_ATTR_TYPE, attr.attr[0]) ?
+			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
 
-		clock[i].clk_id = (class << NODE_CLASS_SHIFT) |
-				  (subclass << NODE_SUBCLASS_SHIFT) |
-				  (nodetype << NODE_TYPE_SHIFT) |
-				  (i << NODE_INDEX_SHIFT);
+		nodetype = FIELD_GET(CLK_ATTR_NODE_TYPE, attr.attr[0]);
+		subclass = FIELD_GET(CLK_ATTR_NODE_SUBCLASS, attr.attr[0]);
+		class = FIELD_GET(CLK_ATTR_NODE_CLASS, attr.attr[0]);
 
-		zynqmp_pm_clock_get_name(clock[i].clk_id, clock[i].clk_name);
+		clock[i].clk_id = FIELD_PREP(CLK_ATTR_NODE_CLASS, class) |
+				  FIELD_PREP(CLK_ATTR_NODE_SUBCLASS, subclass) |
+				  FIELD_PREP(CLK_ATTR_NODE_TYPE, nodetype) |
+				  FIELD_PREP(CLK_ATTR_NODE_INDEX, i);
+
+		zynqmp_pm_clock_get_name(clock[i].clk_id, &name);
+		if (!strcmp(name.name, RESERVED_CLK_NAME))
+			continue;
+		strncpy(clock[i].clk_name, name.name, MAX_NAME_LEN);
 	}
 
 	/* Get topology of all clock */