[4/5] clk: zynqmp: cleanup sizes of firmware responses

Commit Message

Michael Tretter March 12, 2019, 11 a.m. UTC

The queries for the clock name, clock topology, clock parents, and clock
attributes return a specified count of values, whose type and number
depends on the query. Properly separate the number of values per query,
make it dependent on the returned type values and get rid of leftover
hard coded sizes.

Signed-off-by: Michael Tretter <[e-mail]>
---
 drivers/clk/zynqmp/clk-zynqmp.h |  6 ------
 drivers/clk/zynqmp/clkc.c       | 32 +++++++++++++++++++-------------
 2 files changed, 19 insertions(+), 19 deletions(-)

Comments

Stephen Boyd March 13, 2019, 4:37 p.m. UTC | #1

Quoting Michael Tretter (2019-03-12 04:00:15)
> @@ -248,7 +251,8 @@ 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(topology, &ret_payload[1],
> +              CLK_GET_TOPOLOGY_RESP_WORDS * sizeof(*topology));

It would be even clearer to have a set of structs that encoded what the
responses looked like, instead of having to unmarshal the response
somewhat manually on top of a u32 buffer.

So something like

	struct topology_resp {
		u32 node[3];
	};

and then just use sizeof(*topology_resp). Also, it's odd that we make a
stack local variable for the response in this function and then copy it
onto the stack again for the caller of this function
(zynqmp_clock_get_topology()) and then pass it to the unmarshal function
that copies it the third time into the struct clk_topology. It would be
better to do the copy at most once into some structure that has the data
all packed together and then use the macros to access the fields
wherever the response is used.

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 d3d4ce305e71..573c3c58dbbc 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -23,14 +23,11 @@ 
 
 #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)
@@ -54,6 +51,11 @@ 
 
 #define CLK_VALID_MASK			0x1
 
+#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,
 	CLK_TYPE_EXTERNAL,
@@ -215,7 +217,8 @@  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(name, ret_payload,
+	       CLK_GET_NAME_RESP_LEN * sizeof(*name));
 
 	return 0;
 }
@@ -248,7 +251,8 @@  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(topology, &ret_payload[1],
+	       CLK_GET_TOPOLOGY_RESP_WORDS * sizeof(*topology));
 
 	return ret;
 }
@@ -321,7 +325,8 @@  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(parents, &ret_payload[1],
+	       CLK_GET_PARENTS_RESP_WORDS * sizeof(*parents));
 
 	return ret;
 }
@@ -345,7 +350,8 @@  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(attr, &ret_payload[1],
+	       CLK_GET_ATTR_RESP_WORDS * sizeof(*attr));
 
 	return ret;
 }
@@ -364,7 +370,7 @@  static int __zynqmp_clock_get_topology(struct clock_topology *topology,
 {
 	int i;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
+	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
 		if (!(data[i] & CLK_TYPE_FIELD_MASK))
 			return END_OF_TOPOLOGY_NODE;
 		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
@@ -392,10 +398,10 @@  static int zynqmp_clock_get_topology(u32 clk_id,
 				     u32 *num_nodes)
 {
 	int j, ret;
-	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+	u32 pm_resp[CLK_GET_TOPOLOGY_RESP_WORDS] = {0};
 
 	*num_nodes = 0;
-	for (j = 0; j <= MAX_NODES; j += 3) {
+	for (j = 0; j <= MAX_NODES; j += CLK_GET_TOPOLOGY_RESP_WORDS) {
 		ret = zynqmp_pm_clock_get_topology(clk_id, j, pm_resp);
 		if (ret)
 			return ret;
@@ -422,7 +428,7 @@  static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
 	int i;
 	struct clock_parent *parent;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
+	for (i = 0; i < CLK_GET_PARENTS_RESP_WORDS; i++) {
 		if (data[i] == NA_PARENT)
 			return END_OF_PARENTS;
 
@@ -454,7 +460,7 @@  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};
+	u32 pm_resp[CLK_GET_PARENTS_RESP_WORDS] = {0};
 
 	*num_parents = 0;
 	do {
@@ -467,7 +473,7 @@  static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
 						 num_parents);
 		if (ret == END_OF_PARENTS)
 			return 0;
-		j += PM_API_PAYLOAD_LEN;
+		j += CLK_GET_PARENTS_RESP_WORDS;
 	} while (*num_parents <= MAX_PARENT);
 
 	return 0;