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

[Jolly Shah <JOLLYS@xilinx.com>]

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