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

From: Michael Tretter
Date: Mon Apr 15 2019 - 13:34:20 EST

Next message: Sean Christopherson: "Re: [PATCH] KVM: nVMX: always use early vmcs check when EPT is disabled"
Previous message: Nick Desaulniers: "Re: [PATCH] x86: fix .bss with -fdata-sections"
In reply to: Jolly Shah: "RE: [PATCH] drivers: clk: Update clock driver to handle clock attribute"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, 12 Apr 2019 17:50:12 +0000, Jolly Shah wrote:
> 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.

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@xxxxxxxxxxxxxx/T/#u

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

Next message: Sean Christopherson: "Re: [PATCH] KVM: nVMX: always use early vmcs check when EPT is disabled"
Previous message: Nick Desaulniers: "Re: [PATCH] x86: fix .bss with -fdata-sections"
In reply to: Jolly Shah: "RE: [PATCH] drivers: clk: Update clock driver to handle clock attribute"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]