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

From: Thierry Reding <treding@nvidia.com>

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.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/clk/tegra/Makefile   |   1 +
 drivers/clk/tegra/clk-bpmp.c | 565 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 566 insertions(+)
 create mode 100644 drivers/clk/tegra/clk-bpmp.c

On 19/08/16 18:32, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> 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.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/clk/tegra/Makefile   |   1 +
>  drivers/clk/tegra/clk-bpmp.c | 565 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 566 insertions(+)
>  create mode 100644 drivers/clk/tegra/clk-bpmp.c
> 
> diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> index 33fd0938d79e..130df5685d21 100644
> --- a/drivers/clk/tegra/Makefile
> +++ b/drivers/clk/tegra/Makefile
> @@ -22,3 +22,4 @@ obj-$(CONFIG_ARCH_TEGRA_124_SOC)	+= clk-tegra124-dfll-fcpu.o
>  obj-$(CONFIG_ARCH_TEGRA_132_SOC)	+= clk-tegra124.o
>  obj-y					+= cvb.o
>  obj-$(CONFIG_ARCH_TEGRA_210_SOC)	+= clk-tegra210.o
> +obj-$(CONFIG_ARCH_TEGRA_186_SOC)	+= clk-bpmp.o
> diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
> new file mode 100644
> index 000000000000..96cd6becf73e
> --- /dev/null
> +++ b/drivers/clk/tegra/clk-bpmp.c
> @@ -0,0 +1,565 @@
> +#define DEBUG

Do we want DEBUG by default?

> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/seq_buf.h>
> +#include <linux/slab.h>
> +
> +#include <soc/tegra/bpmp.h>
> +#include <soc/tegra/bpmp-abi.h>
> +
> +#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)
> +
> +struct tegra_bpmp_clk_info {
> +	unsigned int id;
> +	char name[MRQ_CLK_NAME_MAXLEN];
> +	unsigned int parents[MRQ_CLK_MAX_PARENTS];
> +	unsigned int num_parents;
> +	unsigned long flags;
> +};
> +
> +struct tegra_bpmp_clk {
> +	struct clk_hw hw;
> +
> +	struct tegra_bpmp *bpmp;
> +	unsigned int id;
> +
> +	unsigned int num_parents;
> +	unsigned int *parents;
> +};
> +
> +static inline struct tegra_bpmp_clk *to_tegra_bpmp_clk(struct clk_hw *hw)
> +{
> +	return container_of(hw, struct tegra_bpmp_clk, hw);
> +}
> +
> +struct tegra_bpmp_clk_message {
> +	unsigned int cmd;
> +	unsigned int clk;
> +
> +	struct {
> +		const void *data;
> +		size_t size;
> +	} tx;
> +
> +	struct {
> +		void *data;
> +		size_t size;
> +	} rx;
> +};
> +
> +static int
> +__tegra_bpmp_clk_transfer_atomic(struct tegra_bpmp *bpmp,
> +				 struct tegra_bpmp_message *msg)
> +{
> +	unsigned long flags;
> +	int err;
> +
> +	local_irq_save(flags);
> +	err = tegra_bpmp_transfer_atomic(bpmp, msg);
> +	local_irq_restore(flags);
> +
> +	return err;
> +}
> +
> +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);
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_CLK;
> +	msg.tx.data = &request;
> +	msg.tx.size = sizeof(request);
> +	msg.rx.data = clk->rx.data;
> +	msg.rx.size = clk->rx.size;
> +
> +	return __tegra_bpmp_clk_transfer_atomic(bpmp, &msg);;
> +}
> +
> +static int tegra_bpmp_clk_transfer(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;
> +	int err;
> +
> +	memset(&request, 0, sizeof(request));
> +	request.cmd_and_id = (clk->cmd << 24) | clk->clk;
> +	memcpy(req + 4, clk->tx.data, clk->tx.size);
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_CLK;
> +	msg.tx.data = &request;
> +	msg.tx.size = sizeof(request);
> +	msg.rx.data = clk->rx.data;
> +	msg.rx.size = clk->rx.size;
> +
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err != -EAGAIN)
> +		return err;
> +
> +	return __tegra_bpmp_clk_transfer_atomic(bpmp, &msg);
> +}
> +
> +static int tegra_bpmp_clk_enable(struct clk_hw *hw)
> +{
> +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> +	struct tegra_bpmp_clk_message msg;
> +	int err;
> +
> +	dev_dbg(clk->bpmp->dev, "> %s(hw=%p)\n", __func__, hw);
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.cmd = CMD_CLK_ENABLE;
> +	msg.clk = clk->id;
> +
> +	err = tegra_bpmp_clk_transfer(clk->bpmp, &msg);
> +
> +	dev_dbg(clk->bpmp->dev, "< %s() = %d\n", __func__, err);
> +	return err;
> +}
> +
> +static void tegra_bpmp_clk_disable(struct clk_hw *hw)
> +{
> +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> +	struct tegra_bpmp_clk_message msg;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.cmd = CMD_CLK_DISABLE;
> +	msg.clk = clk->id;
> +
> +	err = tegra_bpmp_clk_transfer(clk->bpmp, &msg);
> +	if (err < 0)
> +		dev_err(clk->bpmp->dev, "failed to disable clock %s: %d\n",
> +			clk_hw_get_name(hw), err);
> +}
> +
> +static int tegra_bpmp_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> +	struct cmd_clk_is_enabled_response response;
> +	struct tegra_bpmp_clk_message msg;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.cmd = CMD_CLK_IS_ENABLED;
> +	msg.clk = clk->id;
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(response);
> +
> +	err = tegra_bpmp_clk_transfer_atomic(clk->bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +
> +	return response.state;
> +}
> +
> +static u8 tegra_bpmp_clk_get_parent(struct clk_hw *hw)
> +{
> +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> +	struct cmd_clk_get_parent_response response;
> +	struct tegra_bpmp_clk_message msg;
> +	unsigned int i;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.cmd = CMD_CLK_GET_PARENT;
> +	msg.clk = clk->id;
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(&response);
> +
> +	err = tegra_bpmp_clk_transfer(clk->bpmp, &msg);
> +	if (err < 0)
> +		return err;

tegra_bpmp_clk_transfer returns an int, but this function returns a u8.

> +
> +	for (i = 0; i < clk->num_parents; i++)
> +		if (clk->parents[i] == response.parent_id)
> +			return i;
> +
> +	return U8_MAX;

Is there any chance the U8_MAX == num_parents? Should we warn here
somewhere?

> +}
> +
> +static int tegra_bpmp_clk_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> +	struct cmd_clk_set_parent_response response;
> +	struct cmd_clk_set_parent_request request;
> +	struct tegra_bpmp_clk_message msg;
> +	int err;
> +
> +	if (index >= clk->num_parents)
> +		return -EINVAL;
> +
> +	memset(&request, 0, sizeof(request));
> +	request.parent_id = clk->parents[index];
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.cmd = CMD_CLK_SET_PARENT;
> +	msg.clk = clk->id;
> +	msg.tx.data = &request;
> +	msg.tx.size = sizeof(request);
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(response);
> +
> +	err = tegra_bpmp_clk_transfer(clk->bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +
> +	/* XXX check parent ID in response */
> +
> +	return 0;
> +}
> +
> +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;
> +}
> +
> +static const struct clk_ops tegra_bpmp_clk_gate_ops = {
> +	.is_enabled = tegra_bpmp_clk_is_enabled,
> +	.prepare = tegra_bpmp_clk_enable,
> +	.unprepare = tegra_bpmp_clk_disable,
> +};
> +
> +static const struct clk_ops tegra_bpmp_clk_mux_ops = {
> +	.get_parent = tegra_bpmp_clk_get_parent,
> +	.set_parent = tegra_bpmp_clk_set_parent,
> +	.is_enabled = tegra_bpmp_clk_is_enabled,
> +	.prepare = tegra_bpmp_clk_enable,
> +	.unprepare = tegra_bpmp_clk_disable,
> +};
> +
> +static const struct clk_ops tegra_bpmp_clk_rate_ops = {
> +	.is_enabled = tegra_bpmp_clk_is_enabled,
> +	.prepare = tegra_bpmp_clk_enable,
> +	.unprepare = tegra_bpmp_clk_disable,
> +	.set_rate = tegra_bpmp_clk_set_rate,
> +	.round_rate = tegra_bpmp_clk_round_rate,
> +	.recalc_rate = tegra_bpmp_clk_recalc_rate,
> +};
> +
> +static const struct clk_ops tegra_bpmp_clk_mux_rate_ops = {
> +	.get_parent = tegra_bpmp_clk_get_parent,
> +	.set_parent = tegra_bpmp_clk_set_parent,
> +	.is_enabled = tegra_bpmp_clk_is_enabled,
> +	.prepare = tegra_bpmp_clk_enable,
> +	.unprepare = tegra_bpmp_clk_disable,
> +	.set_rate = tegra_bpmp_clk_set_rate,
> +	.round_rate = tegra_bpmp_clk_round_rate,
> +	.recalc_rate = tegra_bpmp_clk_recalc_rate,
> +};
> +
> +static int tegra_bpmp_clk_get_max_id(struct tegra_bpmp *bpmp)
> +{
> +	struct cmd_clk_get_max_clk_id_response response;
> +	struct tegra_bpmp_clk_message msg;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.cmd = CMD_CLK_GET_MAX_CLK_ID;
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(response);
> +
> +	err = tegra_bpmp_clk_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +
> +	return response.max_id;

response.max_id is a uint32
> +}
> +
> +static int tegra_bpmp_clk_get_info(struct tegra_bpmp *bpmp, unsigned int id,
> +				   struct tegra_bpmp_clk_info *info)
> +{
> +	struct cmd_clk_get_all_info_response response;
> +	struct tegra_bpmp_clk_message msg;
> +	unsigned int i;
> +	int err;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.cmd = CMD_CLK_GET_ALL_INFO;
> +	msg.clk = id;
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(response);
> +
> +	err = tegra_bpmp_clk_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +
> +	strlcpy(info->name, response.name, MRQ_CLK_NAME_MAXLEN);
> +	info->num_parents = response.num_parents;
> +
> +	for (i = 0; i < info->num_parents; i++)
> +		info->parents[i] = response.parents[i];
> +
> +	info->flags = response.flags;
> +
> +	return 0;
> +}
> +
> +static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
> +				   struct tegra_bpmp_clk_info **clocksp)
> +{
> +	struct tegra_bpmp_clk_info *clocks;
> +	unsigned int max_id, id, count = 0;
> +	int err;
> +
> +	err = tegra_bpmp_clk_get_max_id(bpmp);
> +	if (err < 0)
> +		return err;
> +
> +	max_id = err;
> +
> +	dev_dbg(bpmp->dev, "maximum clock ID: %u\n", max_id);
> +
> +	clocks = kcalloc(max_id + 1, sizeof(*clocks), GFP_KERNEL);
> +	if (!clocks)
> +		return -ENOMEM;
> +
> +	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
> +
> +		err = tegra_bpmp_clk_get_info(bpmp, id, info);
> +		if (err < 0) {
> +			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
> +				id, err);
> +			continue;
> +		}
> +
> +#if 0
> +		seq_buf_init(&buf, flags, sizeof(flags));
> +
> +		if (info->flags)
> +			seq_buf_printf(&buf, "(");
> +
> +		if (info->flags & TEGRA_BPMP_CLK_HAS_MUX) {
> +			seq_buf_printf(&buf, "%smux", prefix);
> +			prefix = ", ";
> +		}
> +
> +		if ((info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE) == 0) {
> +			seq_buf_printf(&buf, "%sfixed", prefix);
> +			prefix = ", ";
> +		}
> +
> +		if (info->flags & TEGRA_BPMP_CLK_IS_ROOT) {
> +			seq_buf_printf(&buf, "%sroot", prefix);
> +			prefix = ", ";
> +		}
> +
> +		if (info->flags)
> +			seq_buf_printf(&buf, ")");
> +
> +		dev_dbg(bpmp->dev, "  %03u: %s\n", id, info->name);
> +		dev_dbg(bpmp->dev, "    flags: %lx %s\n", info->flags, flags);
> +		dev_dbg(bpmp->dev, "    parents: %u\n", info->num_parents);
> +
> +		for (i = 0; i < info->num_parents; i++)
> +			dev_dbg(bpmp->dev, "      %03u\n", info->parents[i]);
> +#endif
> +
> +		/* clock not exposed by BPMP */
> +		if (info->name[0] == '\0')
> +			continue;
> +
> +		info->id = id;
> +		count++;
> +	}
> +
> +	*clocksp = clocks;

Nit we could just return the pointer.

> +
> +	return count;

We return unsigned int here and not int. Why do we bother returning
count and just store it directly in the bpmp->num_clocks here (see
tegra_bpmp_register_clocks)?

> +}
> +
> +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;
> +	const char **parents;
> +	unsigned int i, j;
> +	struct clk *clk;
> +
> +	dev_dbg(bpmp->dev, "registering clock %u (%s)\n", info->id, info->name);
> +
> +	priv = devm_kzalloc(bpmp->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv->bpmp = bpmp;
> +	priv->id = info->id;
> +
> +	priv->parents = devm_kcalloc(bpmp->dev, info->num_parents,
> +				     sizeof(*priv->parents), GFP_KERNEL);
> +	if (!priv->parents)
> +		return ERR_PTR(-ENOMEM);
> +
> +	priv->num_parents = info->num_parents;
> +
> +	/* hardware clock initialization */
> +	priv->hw.init = &init;
> +	init.name = info->name;
> +
> +	if (info->flags & TEGRA_BPMP_CLK_HAS_MUX) {
> +		if (info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE)
> +			init.ops = &tegra_bpmp_clk_mux_rate_ops;
> +		else
> +			init.ops = &tegra_bpmp_clk_mux_ops;
> +	} else {
> +		if (info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE)
> +			init.ops = &tegra_bpmp_clk_rate_ops;
> +		else
> +			init.ops = &tegra_bpmp_clk_gate_ops;
> +	}
> +
> +	init.num_parents = info->num_parents;
> +
> +	parents = kcalloc(info->num_parents, sizeof(*parents), GFP_KERNEL);
> +	if (!parents)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < info->num_parents; i++) {
> +		/* keep a private copy of the ID to parent index map */
> +		priv->parents[i] = info->parents[i];
> +
> +		for (j = 0; j < num_clocks; j++) {
> +			const struct tegra_bpmp_clk_info *parent = &clocks[j];
> +
> +			if (parent->id == info->parents[i]) {
> +				parents[i] = parent->name;
> +				break;
> +			}
> +		}

Is it necessary to loop through all the clocks again here? This function
is called from tegra_bpmp_register_clocks() which is already looping
through all the clocks. So for each clock we loop through all the clocks
again.

> +		if (!parents[i])
> +			dev_err(bpmp->dev, "no parent %u found for %u\n",
> +				info->parents[i], info->id);
> +	}
> +
> +	init.parent_names = parents;
> +
> +	clk = clk_register(bpmp->dev, &priv->hw);

IS_ERR?

> +
> +	kfree(parents);
> +
> +	return &priv->hw;
> +}
> +
> +static int tegra_bpmp_register_clocks(struct tegra_bpmp *bpmp,
> +				      struct tegra_bpmp_clk_info *clocks,
> +				      unsigned int count)
> +{
> +	struct clk_hw *hw;
> +	unsigned int i;
> +
> +	bpmp->num_clocks = count;
> +
> +	bpmp->clocks = devm_kcalloc(bpmp->dev, count, sizeof(hw), GFP_KERNEL);
> +	if (!bpmp->clocks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < count; i++) {
> +		struct tegra_bpmp_clk_info *info = &clocks[i];
> +
> +		hw = tegra_bpmp_clk_register(bpmp, info, clocks, count);
> +		if (IS_ERR(hw)) {
> +			dev_err(bpmp->dev,
> +				"failed to register clock %u (%s): %ld\n",
> +				info->id, info->name, PTR_ERR(hw));
> +			continue;
> +		}
> +
> +		bpmp->clocks[i] = hw;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct clk_hw *tegra_bpmp_clk_of_xlate(struct of_phandle_args *clkspec,
> +					      void *data)
> +{
> +	unsigned int id = clkspec->args[0], i;
> +	struct tegra_bpmp *bpmp = data;
> +	struct clk_hw *hw = NULL;
> +
> +	dev_dbg(bpmp->dev, "> %s(clkspec=%p, data=%p)\n", __func__, clkspec,
> +		data);
> +
> +	for (i = 0; i < bpmp->num_clocks; i++) {
> +		struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(bpmp->clocks[i]);
> +
> +		if (clk->id == id) {
> +			hw = bpmp->clocks[i];
> +			dev_dbg(bpmp->dev, "  found %u: %s\n", clk->id, clk_hw_get_name(hw));
> +			break;
> +		}
> +	}
> +
> +	dev_dbg(bpmp->dev, "< %s() = %p\n", __func__, hw);
> +
> +	return hw;
> +}
> +
> +int tegra_bpmp_init_clocks(struct tegra_bpmp *bpmp)
> +{
> +	struct tegra_bpmp_clk_info *clocks;
> +	unsigned int count;
> +	int err;
> +
> +	dev_dbg(bpmp->dev, "> %s(bpmp=%p)\n", __func__, bpmp);
> +
> +	err = tegra_bpmp_probe_clocks(bpmp, &clocks);
> +	if (err < 0)
> +		return err;
> +
> +	count = err;
> +
> +	dev_dbg(bpmp->dev, "%u clocks probed\n", count);
> +
> +	err = tegra_bpmp_register_clocks(bpmp, clocks, count);
> +	if (err < 0) {
> +		kfree(clocks);
> +		return err;
> +	}
> +
> +	kfree(clocks);
> +
> +	of_clk_add_hw_provider(bpmp->dev->of_node, tegra_bpmp_clk_of_xlate,
> +			       bpmp);

We should check the return code.

> +	dev_dbg(bpmp->dev, "< %s()\n", __func__);
> +	return 0;
> +}
> 

Cheers
Jon

On Mon, Aug 22, 2016 at 11:11:57AM +0100, Jon Hunter wrote:
> 
> On 19/08/16 18:32, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > 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.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/clk/tegra/Makefile   |   1 +
> >  drivers/clk/tegra/clk-bpmp.c | 565 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 566 insertions(+)
> >  create mode 100644 drivers/clk/tegra/clk-bpmp.c
> > 
> > diff --git a/drivers/clk/tegra/Makefile b/drivers/clk/tegra/Makefile
> > index 33fd0938d79e..130df5685d21 100644
> > --- a/drivers/clk/tegra/Makefile
> > +++ b/drivers/clk/tegra/Makefile
> > @@ -22,3 +22,4 @@ obj-$(CONFIG_ARCH_TEGRA_124_SOC)	+= clk-tegra124-dfll-fcpu.o
> >  obj-$(CONFIG_ARCH_TEGRA_132_SOC)	+= clk-tegra124.o
> >  obj-y					+= cvb.o
> >  obj-$(CONFIG_ARCH_TEGRA_210_SOC)	+= clk-tegra210.o
> > +obj-$(CONFIG_ARCH_TEGRA_186_SOC)	+= clk-bpmp.o
> > diff --git a/drivers/clk/tegra/clk-bpmp.c b/drivers/clk/tegra/clk-bpmp.c
> > new file mode 100644
> > index 000000000000..96cd6becf73e
> > --- /dev/null
> > +++ b/drivers/clk/tegra/clk-bpmp.c
> > @@ -0,0 +1,565 @@
> > +#define DEBUG
> 
> Do we want DEBUG by default?

That was left-over from debugging. I've removed it.

> > +static u8 tegra_bpmp_clk_get_parent(struct clk_hw *hw)
> > +{
> > +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> > +	struct cmd_clk_get_parent_response response;
> > +	struct tegra_bpmp_clk_message msg;
> > +	unsigned int i;
> > +	int err;
> > +
> > +	memset(&msg, 0, sizeof(msg));
> > +	msg.cmd = CMD_CLK_GET_PARENT;
> > +	msg.clk = clk->id;
> > +	msg.rx.data = &response;
> > +	msg.rx.size = sizeof(&response);
> > +
> > +	err = tegra_bpmp_clk_transfer(clk->bpmp, &msg);
> > +	if (err < 0)
> > +		return err;
> 
> tegra_bpmp_clk_transfer returns an int, but this function returns a u8.

I've added an error message containing the error code and made this
return U8_MAX.

> > +
> > +	for (i = 0; i < clk->num_parents; i++)
> > +		if (clk->parents[i] == response.parent_id)
> > +			return i;
> > +
> > +	return U8_MAX;
> 
> Is there any chance the U8_MAX == num_parents? Should we warn here
> somewhere?

I've added a check to the function registration code that will refuse to
add clocks with num_parents >= U8_MAX. This should never happen because
the MRQ_CLK_MAX_PARENTS is the upper bound for this, and it's currently
very small.

> > +static int tegra_bpmp_clk_get_max_id(struct tegra_bpmp *bpmp)
> > +{
> > +	struct cmd_clk_get_max_clk_id_response response;
> > +	struct tegra_bpmp_clk_message msg;
> > +	int err;
> > +
> > +	memset(&msg, 0, sizeof(msg));
> > +	msg.cmd = CMD_CLK_GET_MAX_CLK_ID;
> > +	msg.rx.data = &response;
> > +	msg.rx.size = sizeof(response);
> > +
> > +	err = tegra_bpmp_clk_transfer(bpmp, &msg);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	return response.max_id;
> 
> response.max_id is a uint32

I've added a check to make sure this doesn't overflow.

> > +}
> > +
> > +static int tegra_bpmp_clk_get_info(struct tegra_bpmp *bpmp, unsigned int id,
> > +				   struct tegra_bpmp_clk_info *info)
> > +{
> > +	struct cmd_clk_get_all_info_response response;
> > +	struct tegra_bpmp_clk_message msg;
> > +	unsigned int i;
> > +	int err;
> > +
> > +	memset(&msg, 0, sizeof(msg));
> > +	msg.cmd = CMD_CLK_GET_ALL_INFO;
> > +	msg.clk = id;
> > +	msg.rx.data = &response;
> > +	msg.rx.size = sizeof(response);
> > +
> > +	err = tegra_bpmp_clk_transfer(bpmp, &msg);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	strlcpy(info->name, response.name, MRQ_CLK_NAME_MAXLEN);
> > +	info->num_parents = response.num_parents;
> > +
> > +	for (i = 0; i < info->num_parents; i++)
> > +		info->parents[i] = response.parents[i];
> > +
> > +	info->flags = response.flags;
> > +
> > +	return 0;
> > +}
> > +
> > +static int tegra_bpmp_probe_clocks(struct tegra_bpmp *bpmp,
> > +				   struct tegra_bpmp_clk_info **clocksp)
> > +{
> > +	struct tegra_bpmp_clk_info *clocks;
> > +	unsigned int max_id, id, count = 0;
> > +	int err;
> > +
> > +	err = tegra_bpmp_clk_get_max_id(bpmp);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	max_id = err;
> > +
> > +	dev_dbg(bpmp->dev, "maximum clock ID: %u\n", max_id);
> > +
> > +	clocks = kcalloc(max_id + 1, sizeof(*clocks), GFP_KERNEL);
> > +	if (!clocks)
> > +		return -ENOMEM;
> > +
> > +	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
> > +
> > +		err = tegra_bpmp_clk_get_info(bpmp, id, info);
> > +		if (err < 0) {
> > +			dev_err(bpmp->dev, "failed to query clock %u: %d\n",
> > +				id, err);
> > +			continue;
> > +		}
> > +
> > +#if 0
> > +		seq_buf_init(&buf, flags, sizeof(flags));
> > +
> > +		if (info->flags)
> > +			seq_buf_printf(&buf, "(");
> > +
> > +		if (info->flags & TEGRA_BPMP_CLK_HAS_MUX) {
> > +			seq_buf_printf(&buf, "%smux", prefix);
> > +			prefix = ", ";
> > +		}
> > +
> > +		if ((info->flags & TEGRA_BPMP_CLK_HAS_SET_RATE) == 0) {
> > +			seq_buf_printf(&buf, "%sfixed", prefix);
> > +			prefix = ", ";
> > +		}
> > +
> > +		if (info->flags & TEGRA_BPMP_CLK_IS_ROOT) {
> > +			seq_buf_printf(&buf, "%sroot", prefix);
> > +			prefix = ", ";
> > +		}
> > +
> > +		if (info->flags)
> > +			seq_buf_printf(&buf, ")");
> > +
> > +		dev_dbg(bpmp->dev, "  %03u: %s\n", id, info->name);
> > +		dev_dbg(bpmp->dev, "    flags: %lx %s\n", info->flags, flags);
> > +		dev_dbg(bpmp->dev, "    parents: %u\n", info->num_parents);
> > +
> > +		for (i = 0; i < info->num_parents; i++)
> > +			dev_dbg(bpmp->dev, "      %03u\n", info->parents[i]);
> > +#endif
> > +
> > +		/* clock not exposed by BPMP */
> > +		if (info->name[0] == '\0')
> > +			continue;
> > +
> > +		info->id = id;
> > +		count++;
> > +	}
> > +
> > +	*clocksp = clocks;
> 
> Nit we could just return the pointer.
> 
> > +
> > +	return count;
> 
> We return unsigned int here and not int. Why do we bother returning
> count and just store it directly in the bpmp->num_clocks here (see
> tegra_bpmp_register_clocks)?

Hm... I'm not sure but I think I had initially imagined this to be used
in two steps: call tegra_bpmp_probe_clocks() without passing in a clocks
buffer to get a count for how many clocks to allocate, allocate a buffer
with the proper size in the caller and call again, this time passing in
the new buffer. It looks like halfway through the code I changed my mind
and it's no longer consistent with what I had intended originally.

I'll rewrite this so that it (hopefully) makes more sense.

> > +	for (i = 0; i < info->num_parents; i++) {
> > +		/* keep a private copy of the ID to parent index map */
> > +		priv->parents[i] = info->parents[i];
> > +
> > +		for (j = 0; j < num_clocks; j++) {
> > +			const struct tegra_bpmp_clk_info *parent = &clocks[j];
> > +
> > +			if (parent->id == info->parents[i]) {
> > +				parents[i] = parent->name;
> > +				break;
> > +			}
> > +		}
> 
> Is it necessary to loop through all the clocks again here? This function
> is called from tegra_bpmp_register_clocks() which is already looping
> through all the clocks. So for each clock we loop through all the clocks
> again.

Yes, it's unfortunately necessary because clocks aren't topologically
sorted. That is, in the earlier loop we may encounter clocks for which a
parent hasn't been probed yet. That's not a problem for the common clock
framework because it supports orphan clocks and reparenting when their
parent becomes available. However, given that the driver queries all the
clock names from BPMP, we first need to get all names before we can save
the parent names for the CCFs consumption.

> 
> > +		if (!parents[i])
> > +			dev_err(bpmp->dev, "no parent %u found for %u\n",
> > +				info->parents[i], info->id);
> > +	}
> > +
> > +	init.parent_names = parents;
> > +
> > +	clk = clk_register(bpmp->dev, &priv->hw);
> 
> IS_ERR?

Yes, I've added a check.

> > +int tegra_bpmp_init_clocks(struct tegra_bpmp *bpmp)
> > +{
> > +	struct tegra_bpmp_clk_info *clocks;
> > +	unsigned int count;
> > +	int err;
> > +
> > +	dev_dbg(bpmp->dev, "> %s(bpmp=%p)\n", __func__, bpmp);
> > +
> > +	err = tegra_bpmp_probe_clocks(bpmp, &clocks);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	count = err;
> > +
> > +	dev_dbg(bpmp->dev, "%u clocks probed\n", count);
> > +
> > +	err = tegra_bpmp_register_clocks(bpmp, clocks, count);
> > +	if (err < 0) {
> > +		kfree(clocks);
> > +		return err;
> > +	}
> > +
> > +	kfree(clocks);
> > +
> > +	of_clk_add_hw_provider(bpmp->dev->of_node, tegra_bpmp_clk_of_xlate,
> > +			       bpmp);
> 
> We should check the return code.

Yes, I suppose we should also make sure to remove all clocks if this
ever fails. I'll see how difficult it is to implement that.

Thierry

On 08/19/2016 11:32 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 22/08/16 14:28, Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 11:11:57AM +0100, Jon Hunter wrote:
>>
>> On 19/08/16 18:32, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> 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.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>>  drivers/clk/tegra/Makefile   |   1 +
>>>  drivers/clk/tegra/clk-bpmp.c | 565 +++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 566 insertions(+)
>>>  create mode 100644 drivers/clk/tegra/clk-bpmp.c

[snip]

>>> +int tegra_bpmp_init_clocks(struct tegra_bpmp *bpmp)
>>> +{
>>> +	struct tegra_bpmp_clk_info *clocks;
>>> +	unsigned int count;
>>> +	int err;
>>> +
>>> +	dev_dbg(bpmp->dev, "> %s(bpmp=%p)\n", __func__, bpmp);
>>> +
>>> +	err = tegra_bpmp_probe_clocks(bpmp, &clocks);
>>> +	if (err < 0)
>>> +		return err;
>>> +
>>> +	count = err;
>>> +
>>> +	dev_dbg(bpmp->dev, "%u clocks probed\n", count);
>>> +
>>> +	err = tegra_bpmp_register_clocks(bpmp, clocks, count);
>>> +	if (err < 0) {
>>> +		kfree(clocks);
>>> +		return err;
>>> +	}
>>> +
>>> +	kfree(clocks);
>>> +
>>> +	of_clk_add_hw_provider(bpmp->dev->of_node, tegra_bpmp_clk_of_xlate,
>>> +			       bpmp);
>>
>> We should check the return code.
> 
> Yes, I suppose we should also make sure to remove all clocks if this
> ever fails. I'll see how difficult it is to implement that.

In the unlikely event it fails, not much is going to work and so may be
warning or printing an error is enough. It seems that we don't clean-up
in the bpmp firmware driver (that calls this) and so not cleaning up
here will not make much difference.

Cheers
Jon