Re: [PATCH v6 29/52] memory: tegra-mc: Add interconnect framework

[Thierry Reding]

On Mon, Oct 26, 2020 at 01:17:12AM +0300, Dmitry Osipenko wrote:
> Now Memory Controller is a memory interconnection provider. This allows
> us to use interconnect API for tuning of memory configuration. This patch
> adds common ICC core and adds hooks which should be implemented by the SoC
> drivers.
> 
> Tested-by: Peter Geis <pgwipeout@xxxxxxxxx>
> Tested-by: Nicolas Chauvet <kwizart@xxxxxxxxx>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  drivers/memory/tegra/Kconfig |   1 +
>  drivers/memory/tegra/mc.c    | 129 +++++++++++++++++++++++++++++++++++
>  drivers/memory/tegra/mc.h    |   8 +++
>  include/soc/tegra/mc.h       |  16 +++++
>  4 files changed, 154 insertions(+)
> 
> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
> index 9f0a96bf9ccc..b38e5255effe 100644
> --- a/drivers/memory/tegra/Kconfig
> +++ b/drivers/memory/tegra/Kconfig
> @@ -3,6 +3,7 @@ config TEGRA_MC
>  	bool "NVIDIA Tegra Memory Controller support"
>  	default y
>  	depends on ARCH_TEGRA
> +	select INTERCONNECT
>  	help
>  	  This driver supports the Memory Controller (MC) hardware found on
>  	  NVIDIA Tegra SoCs.
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index 12ea2c79205a..53d61b05ebf8 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -639,6 +639,133 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +static struct icc_node_data *
> +tegra_mc_of_icc_xlate_extended(struct of_phandle_args *spec, void *data)
> +{
> +	struct icc_provider *provider = data;
> +	unsigned int idx = spec->args[0];
> +	struct icc_node_data *ndata;
> +	struct icc_node *node;
> +
> +	list_for_each_entry(node, &provider->nodes, node_list) {
> +		if (node->id != idx)
> +			continue;
> +
> +		ndata = kzalloc(sizeof(*ndata), GFP_KERNEL);
> +		if (!ndata)
> +			return ERR_PTR(-ENOMEM);
> +
> +		ndata->node = node;
> +
> +		/* these clients are isochronous by default on all SoCs */
> +		if (strstarts(node->name, "display") ||
> +		    strstarts(node->name, "ptc") ||
> +		    strstarts(node->name, "vi"))
> +			ndata->tag = TEGRA_MC_ICC_TAG_ISO;

This looks like something that might be better left to the drivers to
decide. Doing this here seems okay for now, but I suspect that this will
get fairly complicated to keep accurate as we add more clients later on.

> +
> +		return ndata;
> +	}
> +
> +	pr_err("%s: invalid client index %u\n", __func__, idx);

Perhaps use "dev_err(provider->dev, ...);"?

> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +/*
> + * Memory Controller (MC) has few Memory Clients that are issuing memory
> + * bandwidth allocation requests to the MC interconnect provider. The MC
> + * provider aggregates the requests and then sends the aggregated request
> + * up to the External Memory Controller (EMC) interconnect provider which
> + * re-configures hardware interface to External Memory (EMEM) in accordance
> + * to the required bandwidth. Each MC interconnect node represents an
> + * individual Memory Client.
> + *
> + * Memory interconnect topology:
> + *
> + *               +----+
> + * +--------+    |    |
> + * | TEXSRD +--->+    |
> + * +--------+    |    |
> + *               |    |    +-----+    +------+
> + *    ...        | MC +--->+ EMC +--->+ EMEM |
> + *               |    |    +-----+    +------+
> + * +--------+    |    |
> + * | DISP.. +--->+    |
> + * +--------+    |    |
> + *               +----+
> + */
> +static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
> +{
> +	struct icc_node *node;
> +	unsigned int i;
> +	int err;
> +
> +	/* older device-trees don't have interconnect properties */
> +	if (!of_find_property(mc->dev->of_node, "#interconnect-cells", NULL) ||
> +	    !mc->soc->icc_ops)
> +		return 0;

This indicates that this property is indeed optional, so the bindings
should reflect that.

> +
> +	mc->provider.dev = mc->dev;
> +	mc->provider.data = &mc->provider;
> +	mc->provider.set = mc->soc->icc_ops->set;
> +	mc->provider.aggregate = mc->soc->icc_ops->aggregate;
> +	mc->provider.xlate_extended = tegra_mc_of_icc_xlate_extended;
> +
> +	err = icc_provider_add(&mc->provider);
> +	if (err)
> +		goto err_msg;
> +
> +	/* create Memory Controller node */
> +	node = icc_node_create(TEGRA_ICC_MC);
> +	err = PTR_ERR_OR_ZERO(node);
> +	if (err)
> +		goto del_provider;
> +
> +	node->name = "Memory Controller";
> +	icc_node_add(node, &mc->provider);
> +
> +	/* link Memory Controller to External Memory Controller */
> +	err = icc_link_create(node, TEGRA_ICC_EMC);
> +	if (err)
> +		goto remove_nodes;
> +
> +	for (i = 0; i < mc->soc->num_clients; i++) {
> +		/* create MC client node */
> +		node = icc_node_create(mc->soc->clients[i].id);
> +		err = PTR_ERR_OR_ZERO(node);
> +		if (err)
> +			goto remove_nodes;
> +
> +		node->name = mc->soc->clients[i].name;
> +		icc_node_add(node, &mc->provider);

I'm not fully familiar with how these nodes are set up, but would it be
possible to set the isochronous tag here already? I'd still prefer this
to be up to the drivers because I think that nicely localizes the
device-specific information in the driver, but if that's not an option,
then doing it here, based on lookup data from the MC clients table
sounds like the next best thing.

> +		/* link Memory Client to Memory Controller */
> +		err = icc_link_create(node, TEGRA_ICC_MC);
> +		if (err)
> +			goto remove_nodes;
> +	}
> +
> +	/*
> +	 * MC driver is registered too early, so early that generic driver
> +	 * syncing doesn't work for the MC. But it doesn't really matter
> +	 * since syncing works for the EMC drivers, hence the we can sync
> +	 * the MC driver by ourselves and then EMC will complete syncing of
> +	 * the whole ICC state.
> +	 */
> +	icc_sync_state(mc->dev);
> +
> +	return 0;
> +
> +remove_nodes:
> +	icc_nodes_remove(&mc->provider);
> +del_provider:
> +	icc_provider_del(&mc->provider);
> +err_msg:
> +	dev_err(mc->dev, "failed to initialize ICC: %d\n", err);
> +
> +	return err;
> +}
> +
>  static int tegra_mc_probe(struct platform_device *pdev)
>  {
>  	struct resource *res;
> @@ -747,6 +874,8 @@ static int tegra_mc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	tegra_mc_interconnect_setup(mc);

Do you want to check the return value here for errors? If not, might as
well make the function return void.

> +
>  	return 0;
>  }
>  
> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
> index afa3ba45c9e6..abeb6a2cc36a 100644
> --- a/drivers/memory/tegra/mc.h
> +++ b/drivers/memory/tegra/mc.h
> @@ -115,4 +115,12 @@ extern const struct tegra_mc_soc tegra132_mc_soc;
>  extern const struct tegra_mc_soc tegra210_mc_soc;
>  #endif
>  
> +/*
> + * These IDs are for internal use of Tegra's ICC, the values are chosen
> + * such that they don't conflict with the device-tree ICC node IDs.
> + */
> +#define TEGRA_ICC_EMC		1000
> +#define TEGRA_ICC_EMEM		2000
> +#define TEGRA_ICC_MC		3000

Sounds to me like these could equally well be 1000, 1001 and 1002. Why
leave these large holes in the number space?

Thierry

Attachment: signature.asc
Description: PGP signature