Date:   Mon, 15 Apr 2019 19:34:08 +0200
From:   Michael Tretter <m.tretter@...gutronix.de>
To:     Jolly Shah <JOLLYS@...inx.com>
Cc:     "mturquette@...libre.com" <mturquette@...libre.com>,
        "sboyd@...eaurora.org" <sboyd@...eaurora.org>,
        Michal Simek <michals@...inx.com>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        Tejas Patel <TEJASP@...inx.com>,
        Rajan Vaja <RAJANV@...inx.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>
Subject: Re: [PATCH] drivers: clk: Update clock driver to handle clock
 attribute

On Fri, 12 Apr 2019 17:50:12 +0000, Jolly Shah wrote:
> Hi Michael,
> 
> > -----Original Message-----
> > From: Michael Tretter <m.tretter@...gutronix.de>
> > Sent: Friday, April 12, 2019 2:01 AM
> > To: Jolly Shah <JOLLYS@...inx.com>
> > Cc: mturquette@...libre.com; sboyd@...eaurora.org; Michal Simek
> > <michals@...inx.com>; linux-clk@...r.kernel.org; Tejas Patel
> > <TEJASP@...inx.com>; Rajan Vaja <RAJANV@...inx.com>; linux-
> > kernel@...r.kernel.org; Jolly Shah <JOLLYS@...inx.com>; Rajan Vaja
> > <RAJANV@...inx.com>; linux-arm-kernel@...ts.infradead.org;
> > kernel@...gutronix.de
> > 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@...inx.com>
> > >
> > > 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@...inx.com>
> > > Signed-off-by: Rajan Vaja <rajan.vaja@...inx.com>
> > > Signed-off-by: Michal Simek <michal.simek@...inx.com>
> > > Signed-off-by: Jolly Shah <jollys@...inx.com>
> > > ---
> > >  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. 

We don't need to hardcode the mapping, because this patch already adds
a mapping by adding the clk_id as a field of zynqmp_clock. What I am
suggesting in to never refer to the index in clock[] as clk_id, but as
index, i or whatever. This would emphasize when we are dealing with a
clock id that can be used to communicate with the firmware and an index
that can be used within the driver.

> 
> >   
> > >  		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.

If the index i is sufficient for retrieving the class, subclass and
type which will be used to construct the clk_id, then why do you need
the class, subclass and type in the clk_id anyway?

> 
> > 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.

OK. So the get_attributes call will never contain the clock index, but
only the class, subclass and type fields. On the other hand, the clk_id
contains the same class, subclass and type fields, but also the index
of the clock. Therefore, the clk_id and the get_attr formats differ and
e.g. NODE_CLASS_SHIFT, which is used to read from get_attr shouldn't be
reused to write to the clk_id, because the clk_id and the get_attr
response essentially have different formats.

Maybe have a look at my other patch [0], which changes how the firmware
responses are unmarshaled and makes the difference between the
attributes and the clock id more obvious.

Michael

[0] https://lore.kernel.org/linux-clk/20190412095220.22855-1-m.tretter@pengutronix.de/T/#u

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

Powered by blists - more mailing lists