Quoting Jolly Shah (2018-02-28 14:27:41)

This patch adds CCF compliant clock driver for ZynqMP.
Clock driver queries supported clock information from
firmware and regiters pll and output clocks with CCF.

Signed-off-by: Jolly Shah <redacted>
Signed-off-by: Rajan Vaja <redacted>
Signed-off-by: Tejas Patel <redacted>
Signed-off-by: Shubhrajyoti Datta <redacted>

Your signoff should go last.

So there should be a depends on for that?

Are you using these flags?

+ * @clk_id:    Id of clock
+ */
+struct zynqmp_clk_gate {
+       struct clk_hw hw;
+       u8 flags;
+       u32 clk_id;
+};
+
+#define to_zynqmp_clk_gate(_hw) container_of(_hw, struct zynqmp_clk_gate, hw)
+
+/**
+ * zynqmp_clk_gate_enable - Enable clock
+ * @hw: handle between common and hardware-specific interfaces
+ *
+ * Return: 0 always
+ */
+static int zynqmp_clk_gate_enable(struct clk_hw *hw)
+{
+       struct zynqmp_clk_gate *gate = to_zynqmp_clk_gate(hw);
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 clk_id = gate->clk_id;
+       int ret = 0;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_enable)
+               return -ENXIO;
+
+       ret = eemi_ops->clock_enable(clk_id);
+
+       if (ret)
+               pr_warn_once("%s() clock enabled failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+
+       return 0;

But we don't return failure if it fails?

+}
+
+/*
+ * zynqmp_clk_gate_disable - Disable clock
+ * @hw: handle between common and hardware-specific interfaces
+ */
+static void zynqmp_clk_gate_disable(struct clk_hw *hw)
+{
+       struct zynqmp_clk_gate *gate = to_zynqmp_clk_gate(hw);
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 clk_id = gate->clk_id;
+       int ret = 0;

Please don't assign things and then reassign them without using them
in between the two.

+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_disable)
+               return;
+
+       ret = eemi_ops->clock_disable(clk_id);
+
+       if (ret)
+               pr_warn_once("%s() clock disable failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+}
+
+/**
+ * zynqmp_clk_gate_is_enable - Check clock state
+ * @hw: handle between common and hardware-specific interfaces
+ *
+ * Return: 1 if enabled, 0 if disabled
+ */
+static int zynqmp_clk_gate_is_enabled(struct clk_hw *hw)
+{
+       struct zynqmp_clk_gate *gate = to_zynqmp_clk_gate(hw);
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 clk_id = gate->clk_id;
+       int state, ret;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_getstate)
+               return 0;
+
+       ret = eemi_ops->clock_getstate(clk_id, &state);
+       if (ret)
+               pr_warn_once("%s() clock get state failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+
+       return state ? 1 : 0;

If it fails to read does state get set, or we just return junk state?

+}
+
+const struct clk_ops zynqmp_clk_gate_ops = {
+       .enable = zynqmp_clk_gate_enable,
+       .disable = zynqmp_clk_gate_disable,
+       .is_enabled = zynqmp_clk_gate_is_enabled,
+};
+EXPORT_SYMBOL_GPL(zynqmp_clk_gate_ops);
+
+/**
+ * zynqmp_clk_register_gate - register a gate clock with the clock framework
+ * @dev: device that is registering this clock
+ * @name: name of this clock
+ * @clk_id: Id of this clock
+ * @parents: name of this clock's parents
+ * @num_parents: number of parents
+ * @flags: framework-specific flags for this clock
+ * @clk_gate_flags: gate-specific flags for this clock
+ *
+ * Return: clock handle of the registered clock gate
+ */
+struct clk *zynqmp_clk_register_gate(struct device *dev, const char *name,
+                                    u32 clk_id, const char * const *parents,
+                                    u8 num_parents, unsigned long flags,
+                                    u8 clk_gate_flags)
+{
+       struct zynqmp_clk_gate *gate;
+       struct clk *clk;
+       struct clk_init_data init;
+
+       /* allocate the gate */
+       gate = kzalloc(sizeof(*gate), GFP_KERNEL);
+       if (!gate)
+               return ERR_PTR(-ENOMEM);
+
+       init.name = name;
+       init.ops = &zynqmp_clk_gate_ops;
+       init.flags = flags;
+       init.parent_names = parents;

This could be a string that we create a pointer to right here because...

+       init.num_parents = num_parents;

this should always be 1?

+
+       /* struct clk_gate assignments */
+       gate->flags = clk_gate_flags;
+       gate->hw.init = &init;
+       gate->clk_id = clk_id;
+
+       clk = clk_register(dev, &gate->hw);

Please use clk_hw_register().

Are you using the CLK_MUX_INDEX_BIT or CLK_MUX_INDEX_ONE flags? If not,
drop them.

+
+       ret = eemi_ops->clock_setparent(clk_id, index);
+
+       if (ret)
+               pr_warn_once("%s() set parent failed for clock: %s, ret = %d\n",
+                            __func__, clk_name, ret);
+
+       return 0;
+}
+
+const struct clk_ops zynqmp_clk_mux_ops = {
+       .get_parent = zynqmp_clk_mux_get_parent,
+       .set_parent = zynqmp_clk_mux_set_parent,
+       .determine_rate = __clk_mux_determine_rate,
+};
+EXPORT_SYMBOL_GPL(zynqmp_clk_mux_ops);
+
+const struct clk_ops zynqmp_clk_mux_ro_ops = {
+       .get_parent = zynqmp_clk_mux_get_parent,
+};
+EXPORT_SYMBOL_GPL(zynqmp_clk_mux_ro_ops);
+
+/**
+ * zynqmp_clk_register_mux_table - register a mux table with the clock framework
+ * @dev: device that is registering this clock
+ * @name: name of this clock
+ * @clk_id: Id of this clock
+ * @parent_names: name of this clock's parents
+ * @num_parents: number of parents
+ * @flags: framework-specific flags for this clock
+ * @clk_mux_flags: mux-specific flags for this clock
+ *
+ * Return: clock handle of the registered clock mux
+ */
+struct clk *zynqmp_clk_register_mux_table(struct device *dev, const char *name,

Is this API used by anyone besides the mux code? It looks like clk-mux.c
was copied and then hacked up and this got left around with no user.

These two look odd.

+
+#define CLK_VALID_MASK                 0x1
+#define CLK_INIT_ENABLE_MASK           (0x1 << CLK_INIT_ENABLE_SHIFT)
+
+enum clk_type {
+       CLK_TYPE_OUTPUT,
+       CLK_TYPE_EXTERNAL,
+};
+
+/**
+ * struct clock_parent - Structure for parent of clock
+ * @name:      Parent name
+ * @id:                Parent clock ID
+ * @flag:      Parent flags
+ */
+struct clock_parent {
+       char name[MAX_NAME_LEN];
+       int id;
+       u32 flag;
+};
+
+/**
+ * struct clock_topology - Structure for topology of clock
+ * @type:      Type of topology
+ * @flag:      Topology flags
+ * @type_flag: Topology type specific flag
+ */
+struct clock_topology {
+       u32 type;
+       u32 flag;
+       u32 type_flag;
+};
+
+/**
+ * struct zynqmp_clock - Structure for clock
+ * @clk_name:          Clock name
+ * @valid:             Validity flag of clock
+ * @init_enable:       init_enable flag of clock
+ * @type:              Clock type (Output/External)
+ * @node:              Clock tolology nodes
+ * @num_nodes:         Number of nodes present in topology
+ * @parent:            structure of parent of clock
+ * @num_parents:       Number of parents of clock
+ */
+struct zynqmp_clock {
+       char clk_name[MAX_NAME_LEN];
+       u32 valid;
+       u32 init_enable;
+       enum clk_type type;

Is this ever assigned?

+       struct clock_topology node[MAX_NODES];
+       u32 num_nodes;
+       struct clock_parent parent[MAX_PARENT];
+       u32 num_parents;
+};
+
+static const char clk_type_postfix[][10] = {
+       [TYPE_INVALID] = "",
+       [TYPE_MUX] = "_mux",
+       [TYPE_GATE] = "",
+       [TYPE_DIV1] = "_div1",
+       [TYPE_DIV2] = "_div2",
+       [TYPE_FIXEDFACTOR] = "_ff",
+       [TYPE_PLL] = ""
+};
+
+static struct zynqmp_clock clock[MAX_CLOCK];
+static struct clk_onecell_data zynqmp_clk_data;
+static struct clk *zynqmp_clks[MAX_CLOCK];
+static unsigned int clock_max_idx;
+static const struct zynqmp_eemi_ops *eemi_ops;
+
+/**
+ * is_valid_clock() - Check whether clock is valid or not
+ * @clk_id:    Clock index.
+ * @valid:     1: if clock is valid.
+ *             0: invalid clock.
+ *
+ * Return: 0 on success else error code.
+ */
+static int is_valid_clock(u32 clk_id, u32 *valid)
+{
+       if (clk_id < 0 || clk_id > clock_max_idx)

clk_id is unsigned, this is impossible.

+               return -ENODEV;
+
+       *valid = clock[clk_id].valid;
+
+       return *valid ? 0 : -EINVAL;
+}
+
+/**
+ * zynqmp_get_clock_name() - Get name of clock from Clock index
+ * @clk_id:    Clock index.
+ * @clk_name:  Name of clock.
+ *
+ * Return: 0 on success else error code.
+ */
+static int zynqmp_get_clock_name(u32 clk_id, char *clk_name)

Please add zynqmp_ prefix to all these APIs, not just this one.

+{
+       int ret;
+       u32 valid;
+
+       ret = is_valid_clock(clk_id, &valid);
+       if (!ret && valid) {
+               strncpy(clk_name, clock[clk_id].clk_name, MAX_NAME_LEN);
+               return 0;
+       } else {
+               return ret;
+       }
+}
+
+/**
+ * get_clock_type() - Get type of clock
+ * @clk_id:    Clock index.
+ * @type:      Clock type: CLK_TYPE_OUTPUT or CLK_TYPE_EXTERNAL.
+ *
+ * Return: 0 on success else error code.
+ */
+static int get_clock_type(u32 clk_id, u32 *type)
+{
+       int ret;
+       u32 valid;
+
+       ret = is_valid_clock(clk_id, &valid);
+       if (!ret && valid) {
+               *type = clock[clk_id].type;
+               return 0;
+       } else {
+               return ret;
+       }

replace with return ret;

+}
+

[...]

+
+/**
+ * zynqmp_pm_clock_get_fixedfactor_params() - Get clock's fixed factor params
+ * @clock_id:  Clock ID.
+ * @mul:       Multiplication value.
+ * @div:       Divisor value.
+ *
+ * This function is used to get fixed factor parameers for the fixed
+ * clock. This API is application only for the fixed clock.

That last sentence doesn't make sense. s/application/applicable/
perhaps?

+ *
+ * Return: Returns status, either success or error+reason.
+ */
+static int zynqmp_pm_clock_get_fixedfactor_params(u32 clock_id,
+                                                 u32 *mul,
+                                                 u32 *div)
+{
+       struct zynqmp_pm_query_data qdata = {0};
+       u32 ret_payload[PAYLOAD_ARG_CNT];
+       int ret;
+
+       qdata.qid = PM_QID_CLOCK_GET_FIXEDFACTOR_PARAMS;
+       qdata.arg1 = clock_id;
+
+       ret = eemi_ops->query_data(qdata, ret_payload);
+       *mul = ret_payload[1];
+       *div = ret_payload[2];
+
+       return ret;
+}
+
+/**
+ * zynqmp_pm_clock_get_parents() - Get the first 3 parents of clock for given id
+ * @clock_id:  Clock ID.
+ * @index:     Parent index.
+ * @parents:   3 parents of the given clock.
+ *
+ * This function is used to get 3 parents for the clock specified by
+ * given clock ID.
+ *
+ * This API will return 3 parents with a single response. To get
+ * other parents, master should call same API in loop with new
+ * parent index till error is returned. E.g First call should have
+ * index 0 which will return parents 0,1 and 2. Next call, index
+ * should be 3 which will return parent 3,4 and 5 and so on.
+ *
+ * Return: Returns status, either success or error+reason.
+ */
+static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32 *parents)
+{
+       struct zynqmp_pm_query_data qdata = {0};
+       u32 ret_payload[PAYLOAD_ARG_CNT];
+       int ret;
+
+       qdata.qid = PM_QID_CLOCK_GET_PARENTS;
+       qdata.arg1 = clock_id;
+       qdata.arg2 = index;
+
+       ret = eemi_ops->query_data(qdata, ret_payload);
+       memcpy(parents, &ret_payload[1], CLK_GET_PARENTS_RESP_WORDS * 4);

Is 4 sizeof(u32)? Or is it supposed to be 3 for the 3 parents returned?

+
+       return ret;
+}
+
+/**
+ * zynqmp_pm_clock_get_attributes() - Get the attributes of clock for given id
+ * @clock_id:  Clock ID.
+ * @attr:      Clock attributes.
+ *
+ * This function is used to get clock's attributes(e.g. valid, clock type, etc).
+ *
+ * Return: Returns status, either success or error+reason.
+ */
+static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
+{
+       struct zynqmp_pm_query_data qdata = {0};
+       u32 ret_payload[PAYLOAD_ARG_CNT];
+       int ret;
+
+       qdata.qid = PM_QID_CLOCK_GET_ATTRIBUTES;
+       qdata.arg1 = clock_id;
+
+       ret = eemi_ops->query_data(qdata, ret_payload);
+       memcpy(attr, &ret_payload[1], CLK_GET_ATTR_RESP_WORDS * 4);

Can this just be sizeof(*attr)?

+
+       return ret;
+}
+
+/**
+ * clock_get_topology() - Get topology of clock from firmware using PM_API
+ * @clk_id:    Clock index.
+ * @clk_topology: Structure of clock topology.
+ * @num_nodes: number of nodes.
+ *
+ * Return: Returns status, either success or error+reason.
+ */
+static int clock_get_topology(u32 clk_id, struct clock_topology *clk_topology,
+                             u32 *num_nodes)
+{
+       int j, k = 0, ret;
+       u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+
+       *num_nodes = 0;
+       for (j = 0; j <= MAX_NODES; j += 3) {
+               ret = zynqmp_pm_clock_get_topology(clk_id, j, pm_resp);
+               if (ret)
+                       return ret;
+               for (k = 0; k < PM_API_PAYLOAD_LEN; k++) {
+                       if (!(pm_resp[k] & CLK_TYPE_FIELD_MASK))
+                               goto done;

return 0;

+                       clk_topology[*num_nodes].type = pm_resp[k] &
+                                                       CLK_TYPE_FIELD_MASK;
+                       clk_topology[*num_nodes].flag =
+                                       (pm_resp[k] >> CLK_FLAG_FIELD_SHIFT) &
+                                       CLK_FLAG_FIELD_MASK;
+                       clk_topology[*num_nodes].type_flag =
+                               (pm_resp[k] >> CLK_TYPE_FLAG_FIELD_SHIFT) &
+                               CLK_TYPE_FLAG_FIELD_MASK;

Use FIELD_GET() for these?

+                       (*num_nodes)++;
+               }
+       }
+done:

Drop label

+       return 0;
+}
+
+/**
+ * clock_get_parents() - Get parents info from firmware using PM_API
+ * @clk_id:            Clock index.
+ * @parents:           Structure of parent information.
+ * @num_parents:       Total number of parents.
+ *
+ * Return: Returns status, either success or error+reason.
+ */
+static int clock_get_parents(u32 clk_id, struct clock_parent *parents,
+                            u32 *num_parents)
+{
+       int j = 0, k, ret, total_parents = 0;
+       u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+
+       do {
+               /* Get parents from firmware */
+               ret = zynqmp_pm_clock_get_parents(clk_id, j, pm_resp);
+               if (ret)
+                       return ret;
+
+               for (k = 0; k < PM_API_PAYLOAD_LEN; k++) {
+                       if (pm_resp[k] == (u32)NA_PARENT) {

Make pm_resp signed? Or specify *_PARENT in hex form.

+                               *num_parents = total_parents;
+                               return 0;
+                       }
+
+                       parents[k + j].id = pm_resp[k] & CLK_PARENTS_ID_MASK;

Please grow a local variable for parents[k + j].

+                       if (pm_resp[k] == (u32)DUMMY_PARENT) {
+                               strncpy(parents[k + j].name,
+                                       "dummy_name", MAX_NAME_LEN);
+                               parents[k + j].flag = 0;
+                       } else {
+                               parents[k + j].flag = pm_resp[k] >>
+                                                       CLK_PARENTS_ID_LEN;
+                               if (zynqmp_get_clock_name(parents[k + j].id,
+                                                         parents[k + j].name))
+                                       continue;
+                       }
+                       total_parents++;
+               }
+               j += PM_API_PAYLOAD_LEN;
+       } while (total_parents <= MAX_PARENT);
+       return 0;
+}
+
+/**
+ * get_parent_list() - Create list of parents name
+ * @np:                        Device node.
+ * @clk_id:            Clock index.
+ * @parent_list:       List of parent's name.
+ * @num_parents:       Total number of parents.

Please drop full stop on kernel doc descriptions of variables.

+ *
+ * Return: Returns status, either success or error+reason.
+ */
+static int get_parent_list(struct device_node *np, u32 clk_id,
+                          const char **parent_list, u32 *num_parents)
+{
+       int i = 0, ret;
+       u32 total_parents = clock[clk_id].num_parents;
+       struct clock_topology *clk_nodes;
+       struct clock_parent *parents;
+
+       clk_nodes = clock[clk_id].node;
+       parents = clock[clk_id].parent;
+
+       for (i = 0; i < total_parents; i++) {
+               if (!parents[i].flag) {
+                       parent_list[i] = parents[i].name;
+               } else if (parents[i].flag == PARENT_CLK_EXTERNAL) {
+                       ret = of_property_match_string(np, "clock-names",
+                                                      parents[i].name);
+                       if (ret < 0)
+                               strncpy(parents[i].name,
+                                       "dummy_name", MAX_NAME_LEN);
+                       parent_list[i] = parents[i].name;
+               } else {
+                       strcat(parents[i].name,
+                              clk_type_postfix[clk_nodes[parents[i].flag - 1].
+                              type]);
+                       parent_list[i] = parents[i].name;
+               }
+       }
+
+       *num_parents = total_parents;
+       return 0;
+}
+
+/**
+ * zynqmp_register_clk_topology() - Register clock topology
+ * @clk_id:            Clock index.
+ * @clk_name:          Clock Name.
+ * @num_parents:       Total number of parents.
+ * @parent_names:      List of parents name.
+ *
+ * Return: Returns status, either success or error+reason.
+ */
+static struct clk *zynqmp_register_clk_topology(int clk_id, char *clk_name,
+                                               int num_parents,
+                                               const char **parent_names)
+{
+       int j, ret;
+       u32 num_nodes, mult, div;
+       char *clk_out = NULL;
+       struct clock_topology *nodes;
+       struct clk *clk = NULL;
+
+       nodes = clock[clk_id].node;
+       num_nodes = clock[clk_id].num_nodes;
+
+       for (j = 0; j < num_nodes; j++) {
+               if (j != (num_nodes - 1)) {

Why is last node special? Add a comment to the code.

+                       clk_out = kasprintf(GFP_KERNEL, "%s%s", clk_name,
+                                           clk_type_postfix[nodes[j].type]);
+               } else {
+                       clk_out = kasprintf(GFP_KERNEL, "%s", clk_name);
+               }
+
+               switch (nodes[j].type) {
+               case TYPE_MUX:
+                       clk = zynqmp_clk_register_mux(NULL, clk_out,
+                                                     clk_id, parent_names,
+                                                     num_parents,
+                                                     nodes[j].flag,
+                                                     nodes[j].type_flag);
+                       break;
+               case TYPE_PLL:
+                       clk = clk_register_zynqmp_pll(clk_out, clk_id,
+                                                     parent_names, 1,
+                                                     nodes[j].flag);
+                       break;
+               case TYPE_FIXEDFACTOR:
+                       ret = zynqmp_pm_clock_get_fixedfactor_params(clk_id,
+                                                                    &mult,
+                                                                    &div);
+                       clk = clk_register_fixed_factor(NULL, clk_out,
+                                                       parent_names[0],
+                                                       nodes[j].flag, mult,
+                                                       div);
+                       break;
+               case TYPE_DIV1:
+               case TYPE_DIV2:
+                       clk = zynqmp_clk_register_divider(NULL, clk_out, clk_id,
+                                                         nodes[j].type,
+                                                         parent_names, 1,
+                                                         nodes[j].flag,
+                                                         nodes[j].type_flag);
+                       break;
+               case TYPE_GATE:
+                       clk = zynqmp_clk_register_gate(NULL, clk_out, clk_id,
+                                                      parent_names, 1,
+                                                      nodes[j].flag,
+                                                      nodes[j].type_flag);
+                       break;
+               default:
+                       pr_err("%s() Unknown topology for %s\n",
+                              __func__, clk_out);
+                       break;
+               }
+               if (IS_ERR(clk))
+                       pr_warn_once("%s() %s register fail with %ld\n",
+                                    __func__, clk_name, PTR_ERR(clk));
+
+               parent_names[0] = clk_out;
+       }
+       kfree(clk_out);
+       return clk;
+}
+
+/**
+ * zynqmp_register_clocks() - Register clocks
+ * @np:                Device node.
+ *
+ * Return: 0 on success else error code
+ */
+static int zynqmp_register_clocks(struct device_node *np)
+{
+       int ret;
+       u32 i, total_parents = 0, type = 0;
+       const char *parent_names[MAX_PARENT];
+
+       for (i = 0; i < clock_max_idx; i++) {
+               char clk_name[MAX_NAME_LEN];
+
+               /* get clock name, continue to next clock if name not found */
+               if (zynqmp_get_clock_name(i, clk_name))
+                       continue;
+
+               /* Check if clock is valid and output clock.
+                * Do not regiter invalid or external clock.
+                */
+               ret = get_clock_type(i, &type);
+               if (ret || type != CLK_TYPE_OUTPUT)
+                       continue;
+
+               /* Get parents of clock*/
+               if (get_parent_list(np, i, parent_names, &total_parents)) {
+                       WARN_ONCE(1, "No parents found for %s\n",
+                                 clock[i].clk_name);
+                       continue;
+               }
+
+               zynqmp_clks[i] = zynqmp_register_clk_topology(i, clk_name,
+                                                             total_parents,
+                                                             parent_names);
+
+               /* Enable clock if init_enable flag is 1 */
+               if (clock[i].init_enable)
+                       clk_prepare_enable(zynqmp_clks[i]);

Use critical clock flag instead?

+       }
+
+       for (i = 0; i < clock_max_idx; i++) {
+               if (IS_ERR(zynqmp_clks[i])) {
+                       pr_err("Zynq Ultrascale+ MPSoC clk %s: register failed with %ld\n",
+                              clock[i].clk_name, PTR_ERR(zynqmp_clks[i]));
+                       WARN_ON(1);
+               }
+       }
+       return 0;
+}
+
+/**
+ * zynqmp_get_clock_info() - Get clock information from firmware using PM_API
+ */
+static void zynqmp_get_clock_info(void)
+{
+       int i, ret;
+       u32 attr, type = 0;
+
+       memset(clock, 0, sizeof(clock));
+       for (i = 0; i < MAX_CLOCK; i++) {
+               zynqmp_pm_clock_get_name(i, clock[i].clk_name);
+               if (!strncmp(clock[i].clk_name, END_OF_CLK_NAME,
+                            MAX_NAME_LEN)) {

Just strcmp? END_OF_CLK_NAME has a known length.

+                       clock_max_idx = i;
+                       break;
+               } else if (!strncmp(clock[i].clk_name, RESERVED_CLK_NAME,
+                                   MAX_NAME_LEN)) {
+                       continue;
+               }
+
+               ret = zynqmp_pm_clock_get_attributes(i, &attr);
+               if (ret)
+                       continue;
+
+               clock[i].valid = attr & CLK_VALID_MASK;
+               clock[i].init_enable = !!(attr & CLK_INIT_ENABLE_MASK);
+               clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
+                                                       CLK_TYPE_OUTPUT;
+       }
+
+       /* Get topology of all clock */
+       for (i = 0; i < clock_max_idx; i++) {
+               ret = get_clock_type(i, &type);
+               if (ret || type != CLK_TYPE_OUTPUT)
+                       continue;
+
+               ret = clock_get_topology(i, clock[i].node, &clock[i].num_nodes);
+               if (ret)
+                       continue;
+
+               ret = clock_get_parents(i, clock[i].parent,
+                                       &clock[i].num_parents);
+               if (ret)
+                       continue;
+       }
+}
+
+/**
+ * zynqmp_clk_setup() - Setup the clock framework and register clocks
+ * @np:                Device node
+ */
+static void __init zynqmp_clk_setup(struct device_node *np)
+{
+       int idx;
+
+       idx = of_property_match_string(np, "clock-names", "pss_ref_clk");
+       if (idx < 0) {
+               pr_err("pss_ref_clk not provided\n");
+               return;
+       }
+       idx = of_property_match_string(np, "clock-names", "video_clk");
+       if (idx < 0) {
+               pr_err("video_clk not provided\n");
+               return;
+       }
+       idx = of_property_match_string(np, "clock-names", "pss_alt_ref_clk");
+       if (idx < 0) {
+               pr_err("pss_alt_ref_clk not provided\n");
+               return;
+       }
+       idx = of_property_match_string(np, "clock-names", "aux_ref_clk");
+       if (idx < 0) {
+               pr_err("aux_ref_clk not provided\n");
+               return;
+       }
+       idx = of_property_match_string(np, "clock-names", "gt_crx_ref_clk");
+       if (idx < 0) {
+               pr_err("aux_ref_clk not provided\n");
+               return;
+       }

Why are we doing all this? Please don't verify DT contents in driver
code.

+
+       zynqmp_get_clock_info();
+       zynqmp_register_clocks(np);
+
+       zynqmp_clk_data.clks = zynqmp_clks;
+       zynqmp_clk_data.clk_num = clock_max_idx;
+       of_clk_add_provider(np, of_clk_src_onecell_get, &zynqmp_clk_data);

Use the clk_hw provider registration method please.

+}
+
+/**
+ * zynqmp_clock_init() - Initialize zynqmp clocks
+ *
+ * Return: 0 always

Why not error too?

+ */
+static int __init zynqmp_clock_init(void)
+{
+       struct device_node *np;
+
+       np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
+       if (!np)
+               return 0;
+       of_node_put(np);
+
+       np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-clkc");
+       if (np)
+               panic("%s: %s binding is deprecated, please use new DT binding\n",
+                      __func__, np->name);

Is this already present in mainline? Drop this check?

+
+       np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-clk");
+       if (!np) {
+               pr_err("%s: clk node not found\n", __func__);
+               of_node_put(np);
+               return 0;
+       }
+
+       eemi_ops = zynqmp_pm_get_eemi_ops();
+       if (!eemi_ops || !eemi_ops->query_data) {
+               pr_err("%s: clk data not found\n", __func__);
+               of_node_put(np);
+               return 0;
+       }
+
+       zynqmp_clk_setup(np);

Preferably this all moves to a platform driver that gets registered by
the eemi_ops code.

Used?

+
+/*
+ * DOC: basic adjustable divider clock that cannot gate
+ *
+ * Traits of this clock:
+ * prepare - clk_prepare only ensures that parents are prepared
+ * enable - clk_enable only ensures that parents are enabled
+ * rate - rate is adjustable.  clk->rate = ceiling(parent->rate / divisor)
+ * parent - fixed parent.  No clk_set_parent support
+ */
+
+#define to_zynqmp_clk_divider(_hw)             \
+       container_of(_hw, struct zynqmp_clk_divider, hw)
+
+/**
+ * struct zynqmp_clk_divider - adjustable divider clock
+ *
+ * @hw:        handle between common and hardware-specific interfaces
+ * @flags: Hardware specific flags
+ * @clk_id: Id of clock
+ * @div_type: divisor type (TYPE_DIV1 or TYPE_DIV2)
+ */
+struct zynqmp_clk_divider {
+       struct clk_hw hw;
+       u8 flags;
+       u32 clk_id;
+       u32 div_type;
+};
+
+static int zynqmp_divider_get_val(unsigned long parent_rate, unsigned long rate)
+{
+       return DIV_ROUND_CLOSEST(parent_rate, rate);
+}
+
+static unsigned long zynqmp_clk_divider_recalc_rate(struct clk_hw *hw,
+                                                   unsigned long parent_rate)
+{
+       struct zynqmp_clk_divider *divider = to_zynqmp_clk_divider(hw);
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 clk_id = divider->clk_id;
+       u32 div_type = divider->div_type;
+       u32 div, value;
+       int ret;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_getdivider)
+               return -ENXIO;

Can we just not register these clks or something if the eemi_ops or type
of ops isn't present? Checking this all the time is not good.

+
+       ret = eemi_ops->clock_getdivider(clk_id, &div);
+
+       if (ret)
+               pr_warn_once("%s() get divider failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+
+       if (div_type == TYPE_DIV1)
+               value = div & 0xFFFF;
+       else
+               value = (div >> 16) & 0xFFFF;
+
+       if (!value) {
+               WARN(!(divider->flags & CLK_DIVIDER_ALLOW_ZERO),
+                    "%s: Zero divisor and CLK_DIVIDER_ALLOW_ZERO not set\n",
+                    clk_name);

Do you use this flag? Copy-paste?

+               return parent_rate;
+       }
+
+       return DIV_ROUND_UP_ULL(parent_rate, value);
+}
+
+static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
+                                         unsigned long rate,
+                                         unsigned long *prate)
+{
+       struct zynqmp_clk_divider *divider = to_zynqmp_clk_divider(hw);
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 clk_id = divider->clk_id;
+       u32 div_type = divider->div_type;
+       u32 bestdiv;
+       int ret;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_getdivider)
+               return -ENXIO;
+
+       /* if read only, just return current value */
+       if (divider->flags & CLK_DIVIDER_READ_ONLY) {
+               ret = eemi_ops->clock_getdivider(clk_id, &bestdiv);
+
+               if (ret)
+                       pr_warn_once("%s() get divider failed for %s, ret = %d\n",
+                                    __func__, clk_name, ret);
+               if (div_type == TYPE_DIV1)
+                       bestdiv = bestdiv & 0xFFFF;
+               else
+                       bestdiv  = (bestdiv >> 16) & 0xFFFF;
+
+               return DIV_ROUND_UP_ULL((u64)*prate, bestdiv);
+       }
+
+       bestdiv = zynqmp_divider_get_val(*prate, rate);
+
+       if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
+           ((clk_hw_get_flags(hw) & CLK_FRAC)))

Too many parenthesis here.

+               bestdiv = rate % *prate ? 1 : bestdiv;
+       *prate = rate * bestdiv;
+
+       return rate;
+}
+
+/**
+ * zynqmp_clk_divider_set_rate - Set rate of divider clock
+ * @hw:        handle between common and hardware-specific interfaces
+ * @rate: rate of clock to be set
+ * @parent_rate: rate of parent clock
+ *
+ * Return: 0 always
+ */
+static int zynqmp_clk_divider_set_rate(struct clk_hw *hw, unsigned long rate,
+                                      unsigned long parent_rate)
+{
+       struct zynqmp_clk_divider *divider = to_zynqmp_clk_divider(hw);
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 clk_id = divider->clk_id;
+       u32 div_type = divider->div_type;
+       u32 value, div;
+       int ret;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_setdivider)
+               return -ENXIO;
+
+       value = zynqmp_divider_get_val(parent_rate, rate);
+       if (div_type == TYPE_DIV1) {
+               div = value & 0xFFFF;
+               div |= ((u16)-1) << 16;

div |= 0xffff << 16;

+       } else {
+               div = ((u16)-1);

div = 0xffff;

+               div |= value << 16;
+       }
+
+       ret = eemi_ops->clock_setdivider(clk_id, div);
+
+       if (ret)
+               pr_warn_once("%s() set divider failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+
+       return 0;
+}
+
+static const struct clk_ops zynqmp_clk_divider_ops = {
+       .recalc_rate = zynqmp_clk_divider_recalc_rate,
+       .round_rate = zynqmp_clk_divider_round_rate,
+       .set_rate = zynqmp_clk_divider_set_rate,
+};
+
+/**
+ * _register_divider - register a divider clock
+ * @dev: device registering this clock
+ * @name: name of this clock
+ * @clk_id: Id of clock
+ * @div_type: Type of divisor
+ * @parents: name of clock's parents
+ * @num_parents: number of parents
+ * @flags: framework-specific flags
+ * @clk_divider_flags: divider-specific flags for this clock
+ *
+ * Return: handle to registered clock divider
+ */
+static struct clk *_register_divider(struct device *dev, const char *name,
+                                    u32 clk_id, u32 div_type,
+                                    const char * const *parents,
+                                    u8 num_parents, unsigned long flags,
+                                    u8 clk_divider_flags)
+{
+       struct zynqmp_clk_divider *div;
+       struct clk *clk;
+       struct clk_init_data init;
+
+       /* allocate the divider */
+       div = kzalloc(sizeof(*div), GFP_KERNEL);
+       if (!div)
+               return ERR_PTR(-ENOMEM);
+
+       init.name = name;
+       init.ops = &zynqmp_clk_divider_ops;
+       init.flags = flags;
+       init.parent_names = parents;
+       init.num_parents = num_parents;
+
+       /* struct clk_divider assignments */
+       div->flags = clk_divider_flags;
+       div->hw.init = &init;
+       div->clk_id = clk_id;
+       div->div_type = div_type;
+
+       /* register the clock */
+       clk = clk_register(dev, &div->hw);

Please use clk_hw_register().

Could be 1 << 16 then?

+
+/**
+ * pll_get_mode - Get mode of PLL
+ * @hw: Handle between common and hardware-specific interfaces
+ *
+ * Return: Mode of PLL
+ */
+static inline enum pll_mode pll_get_mode(struct clk_hw *hw)
+{
+       struct zynqmp_pll *clk = to_zynqmp_pll(hw);
+       u32 clk_id = clk->clk_id;
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 ret_payload[PAYLOAD_ARG_CNT];

How big is PAYLOAD_ARG_CNT?

+       int ret;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->ioctl)
+               return -ENXIO;
+
+       ret = eemi_ops->ioctl(0, IOCTL_GET_PLL_FRAC_MODE, clk_id, 0,
+                             ret_payload);
+       if (ret)
+               pr_warn_once("%s() PLL get frac mode failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+
+       return ret_payload[1];
+}
+
+/**
+ * pll_set_mode - Set the PLL mode
+ * @hw:                Handle between common and hardware-specific interfaces
+ * @on:                Flag to determine the mode
+ */
+static inline void pll_set_mode(struct clk_hw *hw, bool on)
+{
+       struct zynqmp_pll *clk = to_zynqmp_pll(hw);
+       u32 clk_id = clk->clk_id;
+       const char *clk_name = clk_hw_get_name(hw);
+       int ret;
+       u32 mode;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->ioctl) {
+               pr_warn_once("eemi_ops not found\n");
+               return;
+       }
+
+       if (on)
+               mode = PLL_MODE_FRAC;
+       else
+               mode = PLL_MODE_INT;
+
+       ret = eemi_ops->ioctl(0, IOCTL_SET_PLL_FRAC_MODE, clk_id, mode, NULL);
+       if (ret)
+               pr_warn_once("%s() PLL set frac mode failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+}
+
+/**
+ * zynqmp_pll_round_rate - Round a clock frequency
+ * @hw:                Handle between common and hardware-specific interfaces
+ * @rate:      Desired clock frequency
+ * @prate:     Clock frequency of parent clock
+ *
+ * Return:     Frequency closest to @rate the hardware can generate
+ */
+static long zynqmp_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+                                 unsigned long *prate)
+{
+       u32 fbdiv;
+       long rate_div, f;
+
+       /* Enable the fractional mode if needed */
+       rate_div = ((rate * FRAC_DIV) / *prate);

Drop useless parenthesis.

+       f = rate_div % FRAC_DIV;
+       pll_set_mode(hw, !!f);
+
+       if (pll_get_mode(hw) == PLL_MODE_FRAC) {
+               if (rate > PS_PLL_VCO_MAX) {
+                       fbdiv = rate / PS_PLL_VCO_MAX;
+                       rate = rate / (fbdiv + 1);
+               }
+               if (rate < PS_PLL_VCO_MIN) {
+                       fbdiv = DIV_ROUND_UP(PS_PLL_VCO_MIN, rate);
+                       rate = rate * fbdiv;
+               }
+               return rate;
+       }
+
+       fbdiv = DIV_ROUND_CLOSEST(rate, *prate);
+       fbdiv = clamp_t(u32, fbdiv, PLL_FBDIV_MIN, PLL_FBDIV_MAX);
+       return *prate * fbdiv;
+}
+
+/**
+ * zynqmp_pll_recalc_rate - Recalculate clock frequency
+ * @hw:                        Handle between common and hardware-specific interfaces
+ * @parent_rate:       Clock frequency of parent clock
+ * Return:             Current clock frequency
+ */
+static unsigned long zynqmp_pll_recalc_rate(struct clk_hw *hw,
+                                           unsigned long parent_rate)
+{
+       struct zynqmp_pll *clk = to_zynqmp_pll(hw);
+       u32 clk_id = clk->clk_id;
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 fbdiv, data;
+       unsigned long rate, frac;
+       u32 ret_payload[PAYLOAD_ARG_CNT];
+       int ret;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_getdivider)
+               return 0;
+
+       /*
+        * makes probably sense to redundantly save fbdiv in the struct
+        * zynqmp_pll to save the IO access.
+        */
+       ret = eemi_ops->clock_getdivider(clk_id, &fbdiv);
+       if (ret)
+               pr_warn_once("%s() get divider failed for %s, ret = %d\n",
+                            __func__, clk_name, ret);
+
+       rate =  parent_rate * fbdiv;
+       if (pll_get_mode(hw) == PLL_MODE_FRAC) {
+               eemi_ops->ioctl(0, IOCTL_GET_PLL_FRAC_DATA, clk_id, 0,
+                               ret_payload);
+               data = ret_payload[1];
+               frac = (parent_rate * data) / FRAC_DIV;
+               rate = rate + frac;
+       }
+
+       return rate;
+}
+
+/**
+ * zynqmp_pll_set_rate - Set rate of PLL
+ * @hw:                        Handle between common and hardware-specific interfaces
+ * @rate:              Frequency of clock to be set
+ * @parent_rate:       Clock frequency of parent clock
+ */
+static int zynqmp_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+                              unsigned long parent_rate)
+{
+       struct zynqmp_pll *clk = to_zynqmp_pll(hw);
+       u32 clk_id = clk->clk_id;
+       const char *clk_name = clk_hw_get_name(hw);
+       u32 fbdiv, data;
+       long rate_div, frac, m, f;
+       int ret;
+       const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops();
+
+       if (!eemi_ops || !eemi_ops->clock_setdivider)
+               return -ENXIO;
+
+       if (pll_get_mode(hw) == PLL_MODE_FRAC) {
+               unsigned int children;
+
+               /*
+                * We're running on a ZynqMP compatible machine, make sure the
+                * VPLL only has one child.
+                */
+               children = clk_get_children("vpll");
+
+               /* Account for vpll_to_lpd and dp_video_ref */
+               if (children > 2)
+                       WARN(1, "Two devices are using vpll which is forbidden\n");

I suppose a clk_hw_count_children() API would be OK, but I don't know if
we really care. It looks like more firmware validation code that I'd
prefer we don't carry around.

+
+               rate_div = ((rate * FRAC_DIV) / parent_rate);
+               m = rate_div / FRAC_DIV;
+               f = rate_div % FRAC_DIV;
+               m = clamp_t(u32, m, (PLL_FBDIV_MIN), (PLL_FBDIV_MAX));
+               rate = parent_rate * m;
+               frac = (parent_rate * f) / FRAC_DIV;
+
+               ret = eemi_ops->clock_setdivider(clk_id, m);
+               if (ret)
+                       pr_warn_once("%s() set divider failed for %s, ret = %d\n",
+                                    __func__, clk_name, ret);
+
+               data = (FRAC_DIV * f) / FRAC_DIV;
+               eemi_ops->ioctl(0, IOCTL_SET_PLL_FRAC_DATA, clk_id, data, NULL);
+
+               return (rate + frac);

Drop useless parenthesis. 

Why is this here and not local to the zynqmp clk driver?  

Please move this to the one file that uses it. And does anyone use it?

+
+struct device;
+
+struct clk *clk_register_zynqmp_pll(const char *name, u32 clk_id,
+                                   const char * const *parent, u8 num_parents,
+                                   unsigned long flag);
+
+struct clk *zynqmp_clk_register_gate(struct device *dev, const char *name,
+                                    u32 clk_id,
+                                    const char * const *parent_name,
+                                    u8 num_parents, unsigned long flags,
+                                    u8 clk_gate_flags);
+
+struct clk *zynqmp_clk_register_divider(struct device *dev, const char *name,
+                                       u32 clk_id, u32 div_type,
+                                       const char * const *parent_name,
+                                       u8 num_parents,
+                                       unsigned long flags,
+                                       u8 clk_divider_flags);
+
+struct clk *zynqmp_clk_register_mux(struct device *dev, const char *name,
+                                   u32 clk_id,
+                                   const char **parent_names,
+                                   u8 num_parents, unsigned long flags,
+                                   u8 clk_mux_flags);
+
+struct clk *zynqmp_clk_register_mux_table(struct device *dev, const char *name,
+                                         u32 clk_id,
+                                         const char * const *parent_names,
+                                         u8 num_parents, unsigned long flags,
+                                         u8 clk_mux_flags);
+