RE: [PATCH] drivers: clk: Update clock driver to handle clock attribute

[Jolly Shah]

Hi Michael,

> -----Original Message-----
> From: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
> Sent: Friday, April 12, 2019 2:01 AM
> To: Jolly Shah <JOLLYS@xxxxxxxxxx>
> Cc: mturquette@xxxxxxxxxxxx; sboyd@xxxxxxxxxxxxxx; Michal Simek
> <michals@xxxxxxxxxx>; linux-clk@xxxxxxxxxxxxxxx; Tejas Patel
> <TEJASP@xxxxxxxxxx>; Rajan Vaja <RAJANV@xxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; Jolly Shah <JOLLYS@xxxxxxxxxx>; Rajan Vaja
> <RAJANV@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> kernel@xxxxxxxxxxxxxx
> Subject: Re: [PATCH] drivers: clk: Update clock driver to handle clock attribute
> 
> On Mon, 04 Mar 2019 15:19:10 -0800, Jolly Shah wrote:
> > From: Rajan Vaja <rajan.vaja@xxxxxxxxxx>
> >
> > Versal EEMI APIs uses clock device ID which is combination of class,
> > subclass, type and clock index (e.g. 0x8104006 in which 0-13 bits are
> > for index(6 in given example), 14-19 bits are for clock type (i.e pll,
> > out or ref, 1 in given example), 20-25 bits are for subclass which is
> > nothing but clock type only), 26-32 bits are for device class, which
> > is clock(0x2) for all clocks) while zynqmp firmware uses clock ID
> > which is index only (e.g 0, 1, to n, where n is max_clock id).
> >
> > To use zynqmp clock driver for versal platform also, extend use
> > of QueryAttribute API to fetch device class, subclass and clock type
> > to create clock device ID. In case of zynqmp this attributes would be
> > 0 only, so there won't be any effect on clock id as it would use
> > clock index only.
> >
> > Signed-off-by: Tejas Patel <tejas.patel@xxxxxxxxxx>
> > Signed-off-by: Rajan Vaja <rajan.vaja@xxxxxxxxxx>
> > Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
> > Signed-off-by: Jolly Shah <jollys@xxxxxxxxxx>
> > ---
> >  drivers/clk/zynqmp/clkc.c | 42 +++++++++++++++++++++++++++++-------------
> >  1 file changed, 29 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> > index f65cc0f..c13b014 100644
> > --- a/drivers/clk/zynqmp/clkc.c
> > +++ b/drivers/clk/zynqmp/clkc.c
> > @@ -53,6 +53,10 @@
> >  #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
> >
> >  enum clk_type {
> >  	CLK_TYPE_OUTPUT,
> > @@ -80,6 +84,7 @@ struct clock_parent {
> >   * @num_nodes:		Number of nodes present in topology
> >   * @parent:		Parent of clock
> >   * @num_parents:	Number of parents of clock
> > + * @clk_id:		Clock id
> >   */
> >  struct zynqmp_clock {
> >  	char clk_name[MAX_NAME_LEN];
> > @@ -89,6 +94,7 @@ struct zynqmp_clock {
> >  	u32 num_nodes;
> >  	struct clock_parent parent[MAX_PARENT];
> >  	u32 num_parents;
> > +	u32 clk_id;
> >  };
> >
> >  static const char clk_type_postfix[][10] = {
> > @@ -396,7 +402,8 @@ static int zynqmp_clock_get_topology(u32 clk_id,
> >
> >  	*num_nodes = 0;
> >  	for (j = 0; j <= MAX_NODES; j += 3) {
> > -		ret = zynqmp_pm_clock_get_topology(clk_id, j, pm_resp);
> > +		ret = zynqmp_pm_clock_get_topology(clock[clk_id].clk_id, j,
> > +						   pm_resp);
> 
> I think, having clk_id as the index in the array of clock descriptors
> and each descriptor having a clk_id, which might be equal to the clk_id
> (on zynqmp), but might be different from the index (versal) is really
> confusing. It would be better if there would be a clear separation
> between the driver internal id and the id that is used at the interface
> with the firmware.

If we use different ids, we will need to hard code some mappings to convert them to one being used by firmware. For user, both are clock ids but id values are different compared to zynqmp where it was sequential starting from 0. 

> 
> >  		if (ret)
> >  			return ret;
> >  		ret = __zynqmp_clock_get_topology(topology, pm_resp,
> num_nodes);
> > @@ -459,7 +466,8 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct
> clock_parent *parents,
> >  	*num_parents = 0;
> >  	do {
> >  		/* Get parents from firmware */
> > -		ret = zynqmp_pm_clock_get_parents(clk_id, j, pm_resp);
> > +		ret = zynqmp_pm_clock_get_parents(clock[clk_id].clk_id, j,
> > +						  pm_resp);
> >  		if (ret)
> >  			return ret;
> >
> > @@ -528,13 +536,14 @@ static struct clk_hw
> *zynqmp_register_clk_topology(int clk_id, char *clk_name,
> >  						   const char **parent_names)
> >  {
> >  	int j;
> > -	u32 num_nodes;
> > +	u32 num_nodes, clk_dev_id;
> >  	char *clk_out = NULL;
> >  	struct clock_topology *nodes;
> >  	struct clk_hw *hw = NULL;
> >
> >  	nodes = clock[clk_id].node;
> >  	num_nodes = clock[clk_id].num_nodes;
> > +	clk_dev_id = clock[clk_id].clk_id;
> >
> >  	for (j = 0; j < num_nodes; j++) {
> >  		/*
> > @@ -551,13 +560,14 @@ static struct clk_hw
> *zynqmp_register_clk_topology(int clk_id, char *clk_name,
> >  		if (!clk_topology[nodes[j].type])
> >  			continue;
> >
> > -		hw = (*clk_topology[nodes[j].type])(clk_out, clk_id,
> > +		hw = (*clk_topology[nodes[j].type])(clk_out, clk_dev_id,
> >  						    parent_names,
> >  						    num_parents,
> >  						    &nodes[j]);
> >  		if (IS_ERR(hw))
> > -			pr_warn_once("%s() %s register fail with %ld\n",
> > -				     __func__, clk_name, PTR_ERR(hw));
> > +			pr_warn_once("%s() 0x%x: %s register fail with %ld\n",
> > +				     __func__,  clk_dev_id, clk_name,
> > +				     PTR_ERR(hw));
> >
> >  		parent_names[0] = clk_out;
> >  	}
> > @@ -621,20 +631,26 @@ static int zynqmp_register_clocks(struct
> device_node *np)
> >  static void zynqmp_get_clock_info(void)
> >  {
> >  	int i, ret;
> > -	u32 attr, type = 0;
> > +	u32 attr, type = 0, nodetype, subclass, class;
> >
> >  	for (i = 0; i < clock_max_idx; i++) {
> > -		zynqmp_pm_clock_get_name(i, clock[i].clk_name);
> > -		if (!strcmp(clock[i].clk_name, RESERVED_CLK_NAME))
> > -			continue;
> > -
> >  		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 ? CLK_TYPE_EXTERNAL :
> > -							CLK_TYPE_OUTPUT;
> > +		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].clk_id = (class << NODE_CLASS_SHIFT) |
> > +				  (subclass << NODE_SUBCLASS_SHIFT) |
> > +				  (nodetype << NODE_TYPE_SHIFT) |
> > +				  (i << NODE_INDEX_SHIFT);
> 
> In the commit message you write that on versal the index is returned in
> bits 13..0 of the get_attr response from the firmware. However, the code uses
> the index that is used in the get_attr call and ignores the index in
> the response.
> 

Yes index is from bit 0:13. Attributes response doesn't contain index as it is same for what attribute is being queried for which is i. 

> Moreover, on zynqmp bits 0 and 2 of the response are already in use,
> but would be part of the index on versal. Therefore, as I understand,
> the response formats of zynqmp and versal are actually different
> formats and should be distinguished more clearly.
> 

Bits 0 to 2 are same bot Zynqmp and Versal as versal doesn't contain index in attribute response. Only new attribute fields for versal are class, subclass and type. Driver reconstructs clock id using those value and index as i.

Thanks,
Jolly Shah


> Michael
> 
> > +
> > +		zynqmp_pm_clock_get_name(clock[i].clk_id,
> clock[i].clk_name);
> >  	}
> >
> >  	/* Get topology of all clock */