Re: [PATCH v3 11/12] clk: tegra: Add BPMP clock driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [ARM Kernel]     [Linux ARM]     [Linux ARM MSM]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux