diff mbox series

[5/5] clk: zynqmp: make field definitions of query responses consistent

Message ID 20190312110016.29174-6-m.tretter@pengutronix.de (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: zynqmp: fix CLK_FRAC and various cleanups | expand

Commit Message

Michael Tretter March 12, 2019, 11 a.m. UTC 
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 some by
the length of the previous field.

Specify all of them as bit masks and access the fields using the
FIELD_GET macro.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

Comments

Michael Tretter March 12, 2019, 11:19 a.m. UTC | #1 
Oops, somehow this previous incomplete version of the patch slipped through.

I will send a v2 after getting some feedback on the whole series.

Michael

On Tue, 12 Mar 2019 12:00:16 +0100, Michael Tretter wrote:
> 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 some by
> the length of the previous field.
> 
> Specify all of them as bit masks and access the fields using the
> FIELD_GET macro.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index 573c3c58dbbc..b4e13b8618b2 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -21,20 +21,18 @@
>  #define MAX_NODES			6
>  #define MAX_NAME_LEN			50
>  
> -#define CLK_TYPE_SHIFT			2
> -
>  #define NA_PARENT			0xFFFFFFFF
>  #define DUMMY_PARENT			0xFFFFFFFE
>  
> -#define CLK_TYPE_FIELD_LEN		4
> -#define CLK_TOPOLOGY_NODE_OFFSET	16
> +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> +#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
> +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
>  
> -#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			GENMASK(15, 0)
> +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
>  
> -#define CLK_PARENTS_ID_LEN		16
> -#define CLK_PARENTS_ID_MASK		0xFFFF
> +#define CLK_ATTR_VALID			BIT(0)
> +#define CLK_ATTR_TYPE			BIT(2)
>  
>  /* Flags for parents */
>  #define PARENT_CLK_SELF			0
> @@ -49,8 +47,6 @@
>  #define END_OF_PARENTS			1
>  #define RESERVED_CLK_NAME		""
>  
> -#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
> @@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct clock_topology *topology,
>  				       u32 *data, u32 *nnodes)
>  {
>  	int i;
> +	u32 type;
>  
>  	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
> -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[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

Missing semicolon.

> +		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS, data[i]);
>  		topology[*nnodes].type_flag =
> -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK, data[i]);
> +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS, data[i]);
>  		(*nnodes)++;
>  	}
>  
> @@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
>  			return END_OF_PARENTS;
>  
>  		parent = &parents[i];
> -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> +		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
>  		if (data[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, data[i]);
>  			if (zynqmp_get_clock_name(parent->id, parent->name))
>  				continue;
>  		}
> @@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
>  		if (ret)
>  			continue;
>  
> -		clock[i].valid = attr & CLK_VALID_MASK;
> -		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
> -							CLK_TYPE_OUTPUT;
> +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
> +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?

		clock[i].type = FIELD_GET(CLK_ATTR_TYPE, attr) ?

> +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
>  	}
>  
>  	/* Get topology of all clock */
Jolly Shah March 19, 2019, 12:47 a.m. UTC | #2 
Hi Michael,

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: Tuesday, March 12, 2019 4:00 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 5/5] clk: zynqmp: make field definitions of query responses
> consistent
> 
> 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 some by
> the length of the previous field.
> 
> Specify all of them as bit masks and access the fields using the
> FIELD_GET macro.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index 573c3c58dbbc..b4e13b8618b2 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -21,20 +21,18 @@
>  #define MAX_NODES			6
>  #define MAX_NAME_LEN			50
> 
> -#define CLK_TYPE_SHIFT			2
> -
>  #define NA_PARENT			0xFFFFFFFF
>  #define DUMMY_PARENT			0xFFFFFFFE
> 
> -#define CLK_TYPE_FIELD_LEN		4
> -#define CLK_TOPOLOGY_NODE_OFFSET	16
> +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> +#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
> +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
> 
> -#define CLK_TYPE_FIELD_MASK		0xF
> -#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)

GENMASK(23,8) here.

Thanks,
Jolly Shah

> -#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
> +#define CLK_PARENTS_ID			GENMASK(15, 0)
> +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
> 
> -#define CLK_PARENTS_ID_LEN		16
> -#define CLK_PARENTS_ID_MASK		0xFFFF
> +#define CLK_ATTR_VALID			BIT(0)
> +#define CLK_ATTR_TYPE			BIT(2)
> 
>  /* Flags for parents */
>  #define PARENT_CLK_SELF			0
> @@ -49,8 +47,6 @@
>  #define END_OF_PARENTS			1
>  #define RESERVED_CLK_NAME		""
> 
> -#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
> @@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct
> clock_topology *topology,
>  				       u32 *data, u32 *nnodes)
>  {
>  	int i;
> +	u32 type;
> 
>  	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
> -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[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,
> data[i]);
>  		topology[*nnodes].type_flag =
> -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK,
> data[i]);
> +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS,
> data[i]);
>  		(*nnodes)++;
>  	}
> 
> @@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct
> clock_parent *parents, u32 *data,
>  			return END_OF_PARENTS;
> 
>  		parent = &parents[i];
> -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> +		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
>  		if (data[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,
> data[i]);
>  			if (zynqmp_get_clock_name(parent->id, parent-
> >name))
>  				continue;
>  		}
> @@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
>  		if (ret)
>  			continue;
> 
> -		clock[i].valid = attr & CLK_VALID_MASK;
> -		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
> -							CLK_TYPE_OUTPUT;
> +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
> +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?
> +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
>  	}
> 
>  	/* Get topology of all clock */
> --
> 2.20.1
diff mbox series

Patch

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index 573c3c58dbbc..b4e13b8618b2 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -21,20 +21,18 @@ 
 #define MAX_NODES			6
 #define MAX_NAME_LEN			50
 
-#define CLK_TYPE_SHIFT			2
-
 #define NA_PARENT			0xFFFFFFFF
 #define DUMMY_PARENT			0xFFFFFFFE
 
-#define CLK_TYPE_FIELD_LEN		4
-#define CLK_TOPOLOGY_NODE_OFFSET	16
+#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
+#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
+#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
 
-#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			GENMASK(15, 0)
+#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
 
-#define CLK_PARENTS_ID_LEN		16
-#define CLK_PARENTS_ID_MASK		0xFFFF
+#define CLK_ATTR_VALID			BIT(0)
+#define CLK_ATTR_TYPE			BIT(2)
 
 /* Flags for parents */
 #define PARENT_CLK_SELF			0
@@ -49,8 +47,6 @@ 
 #define END_OF_PARENTS			1
 #define RESERVED_CLK_NAME		""
 
-#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
@@ -369,15 +365,16 @@  static int __zynqmp_clock_get_topology(struct clock_topology *topology,
 				       u32 *data, u32 *nnodes)
 {
 	int i;
+	u32 type;
 
 	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
-		if (!(data[i] & CLK_TYPE_FIELD_MASK))
+		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[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, data[i]);
 		topology[*nnodes].type_flag =
-				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK, data[i]);
+				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS, data[i]);
 		(*nnodes)++;
 	}
 
@@ -433,12 +430,12 @@  static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
 			return END_OF_PARENTS;
 
 		parent = &parents[i];
-		parent->id = data[i] & CLK_PARENTS_ID_MASK;
+		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
 		if (data[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, data[i]);
 			if (zynqmp_get_clock_name(parent->id, parent->name))
 				continue;
 		}
@@ -638,9 +635,9 @@  static void zynqmp_get_clock_info(void)
 		if (ret)
 			continue;
 
-		clock[i].valid = attr & CLK_VALID_MASK;
-		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
-							CLK_TYPE_OUTPUT;
+		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
+		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?
+			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
 	}
 
 	/* Get topology of all clock */