| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190412110044.194666db@litschi.hi.pengutronix.de>
Date: Fri, 12 Apr 2019 11:00:44 +0200
From: Michael Tretter <m.tretter@...gutronix.de>
To: Jolly Shah <jolly.shah@...inx.com>
Cc: <mturquette@...libre.com>, <sboyd@...eaurora.org>,
<michal.simek@...inx.com>, <linux-clk@...r.kernel.org>,
Tejas Patel <tejas.patel@...inx.com>,
Rajan Vaja <rajan.vaja@...inx.com>,
linux-kernel@...r.kernel.org, Jolly Shah <jollys@...inx.com>,
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 (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.
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.
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