On 08/19/2016 11:32 AM, Thierry Reding wrote:
From: Thierry Reding <treding@xxxxxxxxxx>
This driver uses the services provided by the BPMP firmware driver to
implement a clock driver based on the MRQ_CLK request. This part of the
BPMP ABI provides a means to enumerate and control clocks and should
allow the driver to work on any chip that supports this ABI.
diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
+#define TEGRA_BPMP_CLK_HAS_MUX BIT(0)
+#define TEGRA_BPMP_CLK_HAS_SET_RATE BIT(1)
+#define TEGRA_BPMP_CLK_IS_ROOT BIT(2)
Shouldn't those be defined in the BPMP ABI header?
+static int
+tegra_bpmp_clk_transfer_atomic(struct tegra_bpmp *bpmp,
+ const struct tegra_bpmp_clk_message *clk)
+{
+ struct mrq_clk_request request;
+ struct tegra_bpmp_message msg;
+ void *req = (void *)&request;
+
+ memset(&request, 0, sizeof(request));
+ request.cmd_and_id = (clk->cmd << 24) | clk->clk;
+ memcpy(req + 4, clk->tx.data, clk->tx.size);
So the TX payload gets memcpy()d once here to combine it with the
request header, and again inside the BPMP driver to copy it to the IVC
shared memory. Does it make sense to eliminate the copy here, and
require callers to allocate and fill in the entire TX structure? The
"(clk->cmd << 24) | clk->clk" could be wrapped in a static inline
function to avoid any duplication of logic.
+static int tegra_bpmp_clk_transfer(struct tegra_bpmp *bpmp,
+ const struct tegra_bpmp_clk_message *clk)
...
+ err = tegra_bpmp_transfer(bpmp, &msg);
+ if (err != -EAGAIN)
+ return err;
+
+ return __tegra_bpmp_clk_transfer_atomic(bpmp, &msg);
+}
This seems odd; can you add some comments indicating why there's a need
for a retry, and why it falls back to the _atomic() function rather than
just retrying?
Nit: Perhaps s/tegra_bpmp_clk/tegra_clk_bpmp/ for all symbols
implemented in this driver; it can be a little hard to tell which
function calls are to the Tegra BPMP driver (tegra_bpmp_*), and which
are to the Tegra clock driver that's implemented using BPMP
(tegra_bpmp_clk_*).
+static int tegra_bpmp_clk_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ return 0;
+}
+
+static long tegra_bpmp_clk_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ return 0;
+}
+
+static unsigned long tegra_bpmp_clk_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ return 0;
+}
Aren't those all missing an implementation?
+static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
+ struct tegra_bpmp_clk_info **clocksp)
+ for (id = 0; id <= max_id; id++) {
+ struct tegra_bpmp_clk_info *info = &clocks[count];
+#if 0
+ const char *prefix = "";
+ struct seq_buf buf;
+ unsigned int i;
+ char flags[64];
+#endif
Should the #if 0 be removed? checkpatch would warn about this; was it
run and if not would it find other things to complain about?
+static struct clk_hw *
+tegra_bpmp_clk_register(struct tegra_bpmp *bpmp,
+ const struct tegra_bpmp_clk_info *info,
+ const struct tegra_bpmp_clk_info *clocks,
+ unsigned int num_clocks)
+{
+ struct tegra_bpmp_clk *priv;
+ struct clk_init_data init;
...
+ /* hardware clock initialization */
+ priv->hw.init = &init;
+ init.name = info->name;
...
+ clk = clk_register(bpmp->dev, &priv->hw);
Is priv->hw.init guaranteed to only be used inside the call to
clk_register()? If not, it's storing a pointer to the stack for longer
than it's valid.
+ parents = kcalloc(info->num_parents, sizeof(*parents), GFP_KERNEL);
+ if (!parents)
+ return ERR_PTR(-ENOMEM);
That needs to free priv->parents which was allocated earlier this function.
+static struct clk_hw *tegra_bpmp_clk_of_xlate(struct of_phandle_args *clkspec,
+ void *data)
+{
+ unsigned int id = clkspec->args[0], i;
Should this function validate the cell count first? Too small means
args[0] doesn't contain valid data, and too large means the DT is bogus,
and we should at least warn the user they've included extra cruft, since
it could in theory gain additional meaning later on if the binding gets
extended.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
