diff mbox series

[v1,12/29] interconnect: Add memory interconnection providers for NVIDIA Tegra SoCs

Message ID 20191118200247.3567-13-digetx@gmail.com (mailing list archive)
State Not Applicable, archived
Headers show
Series Introduce memory interconnect for NVIDIA Tegra SoCs | expand

Commit Message

Dmitry Osipenko Nov. 18, 2019, 8:02 p.m. UTC 
All NVIDIA Tegra SoCs have identical topology in regards to memory
interconnection between memory clients and memory controllers.
The memory controller (MC) and external memory controller (EMC) are
providing memory clients with required memory bandwidth. The memory
controller performs arbitration between memory clients, while the
external memory controller transfers data from/to DRAM and pipes that
data from/to memory controller. Memory controller interconnect provider
aggregates bandwidth requests from memory clients and sends the aggregated
request to EMC provider that scales DRAM frequency in order to satisfy the
bandwidth requirement. Memory controller provider could adjust hardware
configuration for a more optimal arbitration depending on bandwidth
requirements from memory clients, but this is unimplemented for now.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/interconnect/Kconfig               |   1 +
 drivers/interconnect/Makefile              |   1 +
 drivers/interconnect/tegra/Kconfig         |   6 +
 drivers/interconnect/tegra/Makefile        |   4 +
 drivers/interconnect/tegra/tegra-icc-emc.c | 138 +++++++++++++++++++++
 drivers/interconnect/tegra/tegra-icc-mc.c  | 130 +++++++++++++++++++
 include/soc/tegra/mc.h                     |  26 ++++
 7 files changed, 306 insertions(+)
 create mode 100644 drivers/interconnect/tegra/Kconfig
 create mode 100644 drivers/interconnect/tegra/Makefile
 create mode 100644 drivers/interconnect/tegra/tegra-icc-emc.c
 create mode 100644 drivers/interconnect/tegra/tegra-icc-mc.c

Comments

Thierry Reding Nov. 19, 2019, 6:30 a.m. UTC | #1 
On Mon, Nov 18, 2019 at 11:02:30PM +0300, Dmitry Osipenko wrote:
> All NVIDIA Tegra SoCs have identical topology in regards to memory
> interconnection between memory clients and memory controllers.
> The memory controller (MC) and external memory controller (EMC) are
> providing memory clients with required memory bandwidth. The memory
> controller performs arbitration between memory clients, while the
> external memory controller transfers data from/to DRAM and pipes that
> data from/to memory controller. Memory controller interconnect provider
> aggregates bandwidth requests from memory clients and sends the aggregated
> request to EMC provider that scales DRAM frequency in order to satisfy the
> bandwidth requirement. Memory controller provider could adjust hardware
> configuration for a more optimal arbitration depending on bandwidth
> requirements from memory clients, but this is unimplemented for now.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/interconnect/Kconfig               |   1 +
>  drivers/interconnect/Makefile              |   1 +
>  drivers/interconnect/tegra/Kconfig         |   6 +
>  drivers/interconnect/tegra/Makefile        |   4 +
>  drivers/interconnect/tegra/tegra-icc-emc.c | 138 +++++++++++++++++++++
>  drivers/interconnect/tegra/tegra-icc-mc.c  | 130 +++++++++++++++++++
>  include/soc/tegra/mc.h                     |  26 ++++
>  7 files changed, 306 insertions(+)
>  create mode 100644 drivers/interconnect/tegra/Kconfig
>  create mode 100644 drivers/interconnect/tegra/Makefile
>  create mode 100644 drivers/interconnect/tegra/tegra-icc-emc.c
>  create mode 100644 drivers/interconnect/tegra/tegra-icc-mc.c

Why does this have to be separate from the memory controller driver in
drivers/memory/tegra? It seems like this requires a bunch of boilerplate
just so that this code can live in the drivers/interconnect directory.
If Georgi doesn't insist, I'd prefer if we carried this code directly in
the drivers/memory/tegra directory so that we don't have so many
indirections.

Also, and I already briefly mentioned this in another reply, I think we
don't need two providers here. The only one we're really interested in
is the memory-client to memory-controller paths. The MC to EMC path is
static.

Thierry

> diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
> index bfa4ca3ab7a9..b11ca09665bb 100644
> --- a/drivers/interconnect/Kconfig
> +++ b/drivers/interconnect/Kconfig
> @@ -12,5 +12,6 @@ menuconfig INTERCONNECT
>  if INTERCONNECT
>  
>  source "drivers/interconnect/qcom/Kconfig"
> +source "drivers/interconnect/tegra/Kconfig"
>  
>  endif
> diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
> index 28f2ab0824d5..a37d419e262c 100644
> --- a/drivers/interconnect/Makefile
> +++ b/drivers/interconnect/Makefile
> @@ -4,3 +4,4 @@ icc-core-objs				:= core.o
>  
>  obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
>  obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
> +obj-$(CONFIG_INTERCONNECT_TEGRA)	+= tegra/
> diff --git a/drivers/interconnect/tegra/Kconfig b/drivers/interconnect/tegra/Kconfig
> new file mode 100644
> index 000000000000..b724781da71e
> --- /dev/null
> +++ b/drivers/interconnect/tegra/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config INTERCONNECT_TEGRA
> +	bool "NVIDIA Tegra interconnect drivers"
> +	depends on ARCH_TEGRA || COMPILE_TEST
> +	help
> +	  Say Y here to enable support for NVIDIA Tegra interconnect drivers.
> diff --git a/drivers/interconnect/tegra/Makefile b/drivers/interconnect/tegra/Makefile
> new file mode 100644
> index 000000000000..74ff2e53dbdc
> --- /dev/null
> +++ b/drivers/interconnect/tegra/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +obj-$(CONFIG_INTERCONNECT_TEGRA) += tegra-icc-mc.o
> +obj-$(CONFIG_INTERCONNECT_TEGRA) += tegra-icc-emc.o
> diff --git a/drivers/interconnect/tegra/tegra-icc-emc.c b/drivers/interconnect/tegra/tegra-icc-emc.c
> new file mode 100644
> index 000000000000..b594ce811153
> --- /dev/null
> +++ b/drivers/interconnect/tegra/tegra-icc-emc.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Author: Dmitry Osipenko <digetx@gmail.com>
> + * Copyright (C) 2019 GRATE-DRIVER project
> + */
> +
> +#include <dt-bindings/interconnect/tegra-icc.h>
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/of.h>
> +
> +#include <soc/tegra/mc.h>
> +
> +struct tegra_emc_provider {
> +	struct icc_provider provider;
> +	struct clk *clk;
> +	unsigned int dram_data_bus_width_bytes;
> +};
> +
> +static inline struct tegra_emc_provider *
> +to_tegra_emc_provider(struct icc_provider *provider)
> +{
> +	return container_of(provider, struct tegra_emc_provider, provider);
> +}
> +
> +static struct icc_node *
> +tegra_emc_of_icc_xlate_onecell(struct of_phandle_args *spec, void *data)
> +{
> +	struct icc_provider *provider = data;
> +	struct icc_node *node;
> +
> +	list_for_each_entry(node, &provider->nodes, node_list) {
> +		if (node->id == spec->args[0])
> +			return node;
> +	}
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static int tegra_emc_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	struct tegra_emc_provider *emc = to_tegra_emc_provider(dst->provider);
> +	unsigned long long rate = icc_units_to_bps(dst->avg_bw);
> +	unsigned int ddr = 2;
> +	int err;
> +
> +	do_div(rate, ddr * emc->dram_data_bus_width_bytes);
> +	rate = min_t(u64, rate, U32_MAX);
> +
> +	err = clk_set_min_rate(emc->clk, rate);
> +	if (err)
> +		return err;
> +
> +	err = clk_set_rate(emc->clk, rate);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int tegra_emc_icc_aggregate(struct icc_node *node,
> +				   u32 tag, u32 avg_bw, u32 peak_bw,
> +				   u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_avg = min((u64)avg_bw + (*agg_avg), (u64)U32_MAX);
> +	*agg_peak = max(*agg_peak, peak_bw);
> +
> +	return 0;
> +}
> +
> +int tegra_icc_emc_setup_interconnect(struct device *emc_dev,
> +				     unsigned int dram_data_bus_width_bytes)
> +{
> +	struct tegra_emc_provider *emc;
> +	struct icc_node *node, *tmp;
> +	int err;
> +
> +	emc = devm_kzalloc(emc_dev, sizeof(*emc), GFP_KERNEL);
> +	if (!emc)
> +		return -ENOMEM;
> +
> +	emc->clk = devm_clk_get(emc_dev, "emc");
> +	err = PTR_ERR_OR_ZERO(emc->clk);
> +	if (err)
> +		return err;
> +
> +	emc->dram_data_bus_width_bytes = dram_data_bus_width_bytes;
> +
> +	emc->provider.dev = emc_dev;
> +	emc->provider.set = tegra_emc_icc_set;
> +	emc->provider.data = &emc->provider;
> +	emc->provider.xlate = tegra_emc_of_icc_xlate_onecell;
> +	emc->provider.aggregate = tegra_emc_icc_aggregate;
> +
> +	err = icc_provider_add(&emc->provider);
> +	if (err)
> +		return err;
> +
> +	/* create External Memory Controller node */
> +	node = icc_node_create(TEGRA_ICC_EMC);
> +	err = PTR_ERR_OR_ZERO(node);
> +	if (err)
> +		goto del_provider;
> +
> +	node->name = "EMC";
> +	icc_node_add(node, &emc->provider);
> +
> +	/* link External Memory Controller with External Memory */
> +	err = icc_link_create(node, TEGRA_ICC_EMEM);
> +	if (err)
> +		goto destroy_nodes;
> +
> +	/* create External Memory node */
> +	node = icc_node_create(TEGRA_ICC_EMEM);
> +	err = PTR_ERR_OR_ZERO(node);
> +	if (err)
> +		goto destroy_nodes;
> +
> +	node->name = "EMEM";
> +	icc_node_add(node, &emc->provider);
> +
> +	return 0;
> +
> +destroy_nodes:
> +	list_for_each_entry_safe(node, tmp, &emc->provider.nodes, node_list) {
> +		icc_node_del(node);
> +		icc_node_destroy(node->id);
> +	}
> +
> +del_provider:
> +	icc_provider_del(&emc->provider);
> +
> +	return err;
> +}
> diff --git a/drivers/interconnect/tegra/tegra-icc-mc.c b/drivers/interconnect/tegra/tegra-icc-mc.c
> new file mode 100644
> index 000000000000..f1ff8f98def3
> --- /dev/null
> +++ b/drivers/interconnect/tegra/tegra-icc-mc.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Author: Dmitry Osipenko <digetx@gmail.com>
> + * Copyright (C) 2019 GRATE-DRIVER project
> + */
> +
> +#include <dt-bindings/interconnect/tegra-icc.h>
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/interconnect-provider.h>
> +#include <linux/of.h>
> +
> +#include <soc/tegra/mc.h>
> +
> +static struct icc_node *
> +tegra_mc_of_icc_xlate_onecell(struct of_phandle_args *spec, void *data)
> +{
> +	struct icc_provider *provider = data;
> +	struct icc_node *node;
> +
> +	list_for_each_entry(node, &provider->nodes, node_list) {
> +		if (node->id == spec->args[0])
> +			return node;
> +	}
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +static int tegra_mc_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	return 0;
> +}
> +
> +static int tegra_mc_icc_aggregate(struct icc_node *node,
> +				  u32 tag, u32 avg_bw, u32 peak_bw,
> +				  u32 *agg_avg, u32 *agg_peak)
> +{
> +	*agg_avg = min((u64)avg_bw + (*agg_avg), (u64)U32_MAX);
> +	*agg_peak = max(*agg_peak, peak_bw);
> +
> +	return 0;
> +}
> +
> +/*
> + * 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.
> + *
> + * Memory interconnect topology:
> + *
> + *               +----+
> + *   +-----+     |    |
> + *   | GPU +---->+    |
> + *   +-----+     |    |
> + *               |    |     +-----+     +------+
> + *    ...        | MC +---->+ EMC +---->+ EMEM |
> + *               |    |     +-----+     +------+
> + *  +------+     |    |
> + *  | DISP +---->+    |
> + *  +------+     |    |
> + *               +----+
> + */
> +int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc)
> +{
> +	struct icc_provider *provider;
> +	struct icc_node *node, *tmp;
> +	unsigned int i;
> +	int err;
> +
> +	provider = devm_kzalloc(mc->dev, sizeof(*provider), GFP_KERNEL);
> +	if (!provider)
> +		return -ENOMEM;
> +
> +	provider->dev = mc->dev;
> +	provider->set = tegra_mc_icc_set;
> +	provider->data = provider;
> +	provider->xlate = tegra_mc_of_icc_xlate_onecell;
> +	provider->aggregate = tegra_mc_icc_aggregate;
> +
> +	err = icc_provider_add(provider);
> +	if (err)
> +		return err;
> +
> +	/* create Memory Controller node */
> +	node = icc_node_create(TEGRA_ICC_MC);
> +	err = PTR_ERR_OR_ZERO(node);
> +	if (err)
> +		goto del_provider;
> +
> +	node->name = "MC";
> +	icc_node_add(node, provider);
> +
> +	/* link Memory Controller with External Memory Controller */
> +	err = icc_link_create(node, TEGRA_ICC_EMC);
> +	if (err)
> +		goto destroy_nodes;
> +
> +	for (i = 0; i < mc->soc->num_icc_nodes; i++) {
> +		/* create MC client node */
> +		node = icc_node_create(mc->soc->icc_nodes[i].id);
> +		err = PTR_ERR_OR_ZERO(node);
> +		if (err)
> +			goto destroy_nodes;
> +
> +		node->name = mc->soc->icc_nodes[i].name;
> +		icc_node_add(node, provider);
> +
> +		/* link Memory Client with Memory Controller */
> +		err = icc_link_create(node, TEGRA_ICC_MC);
> +		if (err)
> +			goto destroy_nodes;
> +	}
> +
> +	return 0;
> +
> +destroy_nodes:
> +	list_for_each_entry_safe(node, tmp, &provider->nodes, node_list) {
> +		icc_node_del(node);
> +		icc_node_destroy(node->id);
> +	}
> +
> +del_provider:
> +	icc_provider_del(provider);
> +
> +	return err;
> +}
> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> index 1238e35653d1..593954324259 100644
> --- a/include/soc/tegra/mc.h
> +++ b/include/soc/tegra/mc.h
> @@ -141,6 +141,11 @@ struct tegra_mc_reset_ops {
>  			    const struct tegra_mc_reset *rst);
>  };
>  
> +struct tegra_mc_icc_node {
> +	const char *name;
> +	unsigned int id;
> +};
> +
>  struct tegra_mc_soc {
>  	const struct tegra_mc_client *clients;
>  	unsigned int num_clients;
> @@ -160,6 +165,9 @@ struct tegra_mc_soc {
>  	const struct tegra_mc_reset_ops *reset_ops;
>  	const struct tegra_mc_reset *resets;
>  	unsigned int num_resets;
> +
> +	const struct tegra_mc_icc_node *icc_nodes;
> +	unsigned int num_icc_nodes;
>  };
>  
>  struct tegra_mc {
> @@ -184,4 +192,22 @@ struct tegra_mc {
>  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
>  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
>  
> +#ifdef CONFIG_INTERCONNECT_TEGRA
> +int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
> +int tegra_icc_emc_setup_interconnect(struct device *emc_dev,
> +				     unsigned int dram_data_bus_width_bytes);
> +#else
> +static inline int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
> +{
> +	return 0;
> +}
> +
> +static inline int
> +tegra_icc_emc_setup_interconnect(struct device *emc_dev,
> +				 unsigned int dram_data_bus_width_bytes)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #endif /* __SOC_TEGRA_MC_H__ */
> -- 
> 2.23.0
>
Thierry Reding Nov. 19, 2019, 6:31 a.m. UTC | #2 
On Mon, Nov 18, 2019 at 11:02:30PM +0300, Dmitry Osipenko wrote:
[...]
> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
> index 1238e35653d1..593954324259 100644
> --- a/include/soc/tegra/mc.h
> +++ b/include/soc/tegra/mc.h
> @@ -141,6 +141,11 @@ struct tegra_mc_reset_ops {
>  			    const struct tegra_mc_reset *rst);
>  };
>  
> +struct tegra_mc_icc_node {
> +	const char *name;
> +	unsigned int id;
> +};
> +
>  struct tegra_mc_soc {
>  	const struct tegra_mc_client *clients;
>  	unsigned int num_clients;
> @@ -160,6 +165,9 @@ struct tegra_mc_soc {
>  	const struct tegra_mc_reset_ops *reset_ops;
>  	const struct tegra_mc_reset *resets;
>  	unsigned int num_resets;
> +
> +	const struct tegra_mc_icc_node *icc_nodes;
> +	unsigned int num_icc_nodes;
>  };
>  
>  struct tegra_mc {
> @@ -184,4 +192,22 @@ struct tegra_mc {
>  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
>  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
>  
> +#ifdef CONFIG_INTERCONNECT_TEGRA
> +int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
> +int tegra_icc_emc_setup_interconnect(struct device *emc_dev,
> +				     unsigned int dram_data_bus_width_bytes);
> +#else
> +static inline int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
> +{
> +	return 0;
> +}
> +
> +static inline int
> +tegra_icc_emc_setup_interconnect(struct device *emc_dev,
> +				 unsigned int dram_data_bus_width_bytes)
> +{
> +	return 0;
> +}
> +#endif

Is there really any reason why we should make this support optional? It
seems to me like we would always want this enabled once it's tested and
known to work.

Thierry
Dmitry Osipenko Nov. 19, 2019, 4:58 p.m. UTC | #3 
19.11.2019 09:30, Thierry Reding пишет:
> On Mon, Nov 18, 2019 at 11:02:30PM +0300, Dmitry Osipenko wrote:
>> All NVIDIA Tegra SoCs have identical topology in regards to memory
>> interconnection between memory clients and memory controllers.
>> The memory controller (MC) and external memory controller (EMC) are
>> providing memory clients with required memory bandwidth. The memory
>> controller performs arbitration between memory clients, while the
>> external memory controller transfers data from/to DRAM and pipes that
>> data from/to memory controller. Memory controller interconnect provider
>> aggregates bandwidth requests from memory clients and sends the aggregated
>> request to EMC provider that scales DRAM frequency in order to satisfy the
>> bandwidth requirement. Memory controller provider could adjust hardware
>> configuration for a more optimal arbitration depending on bandwidth
>> requirements from memory clients, but this is unimplemented for now.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/interconnect/Kconfig               |   1 +
>>  drivers/interconnect/Makefile              |   1 +
>>  drivers/interconnect/tegra/Kconfig         |   6 +
>>  drivers/interconnect/tegra/Makefile        |   4 +
>>  drivers/interconnect/tegra/tegra-icc-emc.c | 138 +++++++++++++++++++++
>>  drivers/interconnect/tegra/tegra-icc-mc.c  | 130 +++++++++++++++++++
>>  include/soc/tegra/mc.h                     |  26 ++++
>>  7 files changed, 306 insertions(+)
>>  create mode 100644 drivers/interconnect/tegra/Kconfig
>>  create mode 100644 drivers/interconnect/tegra/Makefile
>>  create mode 100644 drivers/interconnect/tegra/tegra-icc-emc.c
>>  create mode 100644 drivers/interconnect/tegra/tegra-icc-mc.c
> 
> Why does this have to be separate from the memory controller driver in
> drivers/memory/tegra? It seems like this requires a bunch of boilerplate
> just so that this code can live in the drivers/interconnect directory.

It fits with the IOMMU separation. To me that it's a bit nicer to have
the separation for the ICC as well, but having ICC within memory
controller driver also will be fine.

Indeed it looks like there is not much in the MC's provider code right
now, but maybe more stuff will be added later on.

> If Georgi doesn't insist, I'd prefer if we carried this code directly in
> the drivers/memory/tegra directory so that we don't have so many
> indirections.
> 
> Also, and I already briefly mentioned this in another reply, I think we
> don't need two providers here. The only one we're really interested in
> is the memory-client to memory-controller paths. The MC to EMC path is
> static.

Perhaps it is fine to drop EMC path, I'll revisit it.

[snip]
Dmitry Osipenko Nov. 19, 2019, 4:59 p.m. UTC | #4 
19.11.2019 09:31, Thierry Reding пишет:
> On Mon, Nov 18, 2019 at 11:02:30PM +0300, Dmitry Osipenko wrote:
> [...]
>> diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
>> index 1238e35653d1..593954324259 100644
>> --- a/include/soc/tegra/mc.h
>> +++ b/include/soc/tegra/mc.h
>> @@ -141,6 +141,11 @@ struct tegra_mc_reset_ops {
>>  			    const struct tegra_mc_reset *rst);
>>  };
>>  
>> +struct tegra_mc_icc_node {
>> +	const char *name;
>> +	unsigned int id;
>> +};
>> +
>>  struct tegra_mc_soc {
>>  	const struct tegra_mc_client *clients;
>>  	unsigned int num_clients;
>> @@ -160,6 +165,9 @@ struct tegra_mc_soc {
>>  	const struct tegra_mc_reset_ops *reset_ops;
>>  	const struct tegra_mc_reset *resets;
>>  	unsigned int num_resets;
>> +
>> +	const struct tegra_mc_icc_node *icc_nodes;
>> +	unsigned int num_icc_nodes;
>>  };
>>  
>>  struct tegra_mc {
>> @@ -184,4 +192,22 @@ struct tegra_mc {
>>  int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
>>  unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
>>  
>> +#ifdef CONFIG_INTERCONNECT_TEGRA
>> +int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
>> +int tegra_icc_emc_setup_interconnect(struct device *emc_dev,
>> +				     unsigned int dram_data_bus_width_bytes);
>> +#else
>> +static inline int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
>> +{
>> +	return 0;
>> +}
>> +
>> +static inline int
>> +tegra_icc_emc_setup_interconnect(struct device *emc_dev,
>> +				 unsigned int dram_data_bus_width_bytes)
>> +{
>> +	return 0;
>> +}
>> +#endif
> 
> Is there really any reason why we should make this support optional? It
> seems to me like we would always want this enabled once it's tested and
> known to work.

There is always some room for bugs, should be better to have an option
to disable ICC entirely (IMO).
Dmitry Osipenko Nov. 21, 2019, 5:33 p.m. UTC | #5 
19.11.2019 19:58, Dmitry Osipenko пишет:
> 19.11.2019 09:30, Thierry Reding пишет:
>> On Mon, Nov 18, 2019 at 11:02:30PM +0300, Dmitry Osipenko wrote:
>>> All NVIDIA Tegra SoCs have identical topology in regards to memory
>>> interconnection between memory clients and memory controllers.
>>> The memory controller (MC) and external memory controller (EMC) are
>>> providing memory clients with required memory bandwidth. The memory
>>> controller performs arbitration between memory clients, while the
>>> external memory controller transfers data from/to DRAM and pipes that
>>> data from/to memory controller. Memory controller interconnect provider
>>> aggregates bandwidth requests from memory clients and sends the aggregated
>>> request to EMC provider that scales DRAM frequency in order to satisfy the
>>> bandwidth requirement. Memory controller provider could adjust hardware
>>> configuration for a more optimal arbitration depending on bandwidth
>>> requirements from memory clients, but this is unimplemented for now.
>>>
>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>> ---
>>>  drivers/interconnect/Kconfig               |   1 +
>>>  drivers/interconnect/Makefile              |   1 +
>>>  drivers/interconnect/tegra/Kconfig         |   6 +
>>>  drivers/interconnect/tegra/Makefile        |   4 +
>>>  drivers/interconnect/tegra/tegra-icc-emc.c | 138 +++++++++++++++++++++
>>>  drivers/interconnect/tegra/tegra-icc-mc.c  | 130 +++++++++++++++++++
>>>  include/soc/tegra/mc.h                     |  26 ++++
>>>  7 files changed, 306 insertions(+)
>>>  create mode 100644 drivers/interconnect/tegra/Kconfig
>>>  create mode 100644 drivers/interconnect/tegra/Makefile
>>>  create mode 100644 drivers/interconnect/tegra/tegra-icc-emc.c
>>>  create mode 100644 drivers/interconnect/tegra/tegra-icc-mc.c
>>
>> Why does this have to be separate from the memory controller driver in
>> drivers/memory/tegra? It seems like this requires a bunch of boilerplate
>> just so that this code can live in the drivers/interconnect directory.
> 
> It fits with the IOMMU separation. To me that it's a bit nicer to have
> the separation for the ICC as well, but having ICC within memory
> controller driver also will be fine.
> 
> Indeed it looks like there is not much in the MC's provider code right
> now, but maybe more stuff will be added later on.
> 
>> If Georgi doesn't insist, I'd prefer if we carried this code directly in
>> the drivers/memory/tegra directory so that we don't have so many
>> indirections.
>>
>> Also, and I already briefly mentioned this in another reply, I think we
>> don't need two providers here. The only one we're really interested in
>> is the memory-client to memory-controller paths. The MC to EMC path is
>> static.
> 
> Perhaps it is fine to drop EMC path, I'll revisit it.
> 
> [snip]

One advantage of having both MC and EMC as ICC providers is that there
won't be a need to mess with a custom coupling of MC-EMC drivers
together because interconnect API naturally takes care of the coupling
for us by telling ICC users to defer until both providers are registered.

I'll take another look at this over the weekend, but for now my v1
variant looks appropriate in terms of a better hardware description and
implementation in the code.
diff mbox series

Patch

diff --git a/drivers/interconnect/Kconfig b/drivers/interconnect/Kconfig
index bfa4ca3ab7a9..b11ca09665bb 100644
--- a/drivers/interconnect/Kconfig
+++ b/drivers/interconnect/Kconfig
@@ -12,5 +12,6 @@  menuconfig INTERCONNECT
 if INTERCONNECT
 
 source "drivers/interconnect/qcom/Kconfig"
+source "drivers/interconnect/tegra/Kconfig"
 
 endif
diff --git a/drivers/interconnect/Makefile b/drivers/interconnect/Makefile
index 28f2ab0824d5..a37d419e262c 100644
--- a/drivers/interconnect/Makefile
+++ b/drivers/interconnect/Makefile
@@ -4,3 +4,4 @@  icc-core-objs				:= core.o
 
 obj-$(CONFIG_INTERCONNECT)		+= icc-core.o
 obj-$(CONFIG_INTERCONNECT_QCOM)		+= qcom/
+obj-$(CONFIG_INTERCONNECT_TEGRA)	+= tegra/
diff --git a/drivers/interconnect/tegra/Kconfig b/drivers/interconnect/tegra/Kconfig
new file mode 100644
index 000000000000..b724781da71e
--- /dev/null
+++ b/drivers/interconnect/tegra/Kconfig
@@ -0,0 +1,6 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+config INTERCONNECT_TEGRA
+	bool "NVIDIA Tegra interconnect drivers"
+	depends on ARCH_TEGRA || COMPILE_TEST
+	help
+	  Say Y here to enable support for NVIDIA Tegra interconnect drivers.
diff --git a/drivers/interconnect/tegra/Makefile b/drivers/interconnect/tegra/Makefile
new file mode 100644
index 000000000000..74ff2e53dbdc
--- /dev/null
+++ b/drivers/interconnect/tegra/Makefile
@@ -0,0 +1,4 @@ 
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_INTERCONNECT_TEGRA) += tegra-icc-mc.o
+obj-$(CONFIG_INTERCONNECT_TEGRA) += tegra-icc-emc.o
diff --git a/drivers/interconnect/tegra/tegra-icc-emc.c b/drivers/interconnect/tegra/tegra-icc-emc.c
new file mode 100644
index 000000000000..b594ce811153
--- /dev/null
+++ b/drivers/interconnect/tegra/tegra-icc-emc.c
@@ -0,0 +1,138 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Author: Dmitry Osipenko <digetx@gmail.com>
+ * Copyright (C) 2019 GRATE-DRIVER project
+ */
+
+#include <dt-bindings/interconnect/tegra-icc.h>
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interconnect-provider.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+
+#include <soc/tegra/mc.h>
+
+struct tegra_emc_provider {
+	struct icc_provider provider;
+	struct clk *clk;
+	unsigned int dram_data_bus_width_bytes;
+};
+
+static inline struct tegra_emc_provider *
+to_tegra_emc_provider(struct icc_provider *provider)
+{
+	return container_of(provider, struct tegra_emc_provider, provider);
+}
+
+static struct icc_node *
+tegra_emc_of_icc_xlate_onecell(struct of_phandle_args *spec, void *data)
+{
+	struct icc_provider *provider = data;
+	struct icc_node *node;
+
+	list_for_each_entry(node, &provider->nodes, node_list) {
+		if (node->id == spec->args[0])
+			return node;
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
+static int tegra_emc_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	struct tegra_emc_provider *emc = to_tegra_emc_provider(dst->provider);
+	unsigned long long rate = icc_units_to_bps(dst->avg_bw);
+	unsigned int ddr = 2;
+	int err;
+
+	do_div(rate, ddr * emc->dram_data_bus_width_bytes);
+	rate = min_t(u64, rate, U32_MAX);
+
+	err = clk_set_min_rate(emc->clk, rate);
+	if (err)
+		return err;
+
+	err = clk_set_rate(emc->clk, rate);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int tegra_emc_icc_aggregate(struct icc_node *node,
+				   u32 tag, u32 avg_bw, u32 peak_bw,
+				   u32 *agg_avg, u32 *agg_peak)
+{
+	*agg_avg = min((u64)avg_bw + (*agg_avg), (u64)U32_MAX);
+	*agg_peak = max(*agg_peak, peak_bw);
+
+	return 0;
+}
+
+int tegra_icc_emc_setup_interconnect(struct device *emc_dev,
+				     unsigned int dram_data_bus_width_bytes)
+{
+	struct tegra_emc_provider *emc;
+	struct icc_node *node, *tmp;
+	int err;
+
+	emc = devm_kzalloc(emc_dev, sizeof(*emc), GFP_KERNEL);
+	if (!emc)
+		return -ENOMEM;
+
+	emc->clk = devm_clk_get(emc_dev, "emc");
+	err = PTR_ERR_OR_ZERO(emc->clk);
+	if (err)
+		return err;
+
+	emc->dram_data_bus_width_bytes = dram_data_bus_width_bytes;
+
+	emc->provider.dev = emc_dev;
+	emc->provider.set = tegra_emc_icc_set;
+	emc->provider.data = &emc->provider;
+	emc->provider.xlate = tegra_emc_of_icc_xlate_onecell;
+	emc->provider.aggregate = tegra_emc_icc_aggregate;
+
+	err = icc_provider_add(&emc->provider);
+	if (err)
+		return err;
+
+	/* create External Memory Controller node */
+	node = icc_node_create(TEGRA_ICC_EMC);
+	err = PTR_ERR_OR_ZERO(node);
+	if (err)
+		goto del_provider;
+
+	node->name = "EMC";
+	icc_node_add(node, &emc->provider);
+
+	/* link External Memory Controller with External Memory */
+	err = icc_link_create(node, TEGRA_ICC_EMEM);
+	if (err)
+		goto destroy_nodes;
+
+	/* create External Memory node */
+	node = icc_node_create(TEGRA_ICC_EMEM);
+	err = PTR_ERR_OR_ZERO(node);
+	if (err)
+		goto destroy_nodes;
+
+	node->name = "EMEM";
+	icc_node_add(node, &emc->provider);
+
+	return 0;
+
+destroy_nodes:
+	list_for_each_entry_safe(node, tmp, &emc->provider.nodes, node_list) {
+		icc_node_del(node);
+		icc_node_destroy(node->id);
+	}
+
+del_provider:
+	icc_provider_del(&emc->provider);
+
+	return err;
+}
diff --git a/drivers/interconnect/tegra/tegra-icc-mc.c b/drivers/interconnect/tegra/tegra-icc-mc.c
new file mode 100644
index 000000000000..f1ff8f98def3
--- /dev/null
+++ b/drivers/interconnect/tegra/tegra-icc-mc.c
@@ -0,0 +1,130 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Author: Dmitry Osipenko <digetx@gmail.com>
+ * Copyright (C) 2019 GRATE-DRIVER project
+ */
+
+#include <dt-bindings/interconnect/tegra-icc.h>
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/interconnect-provider.h>
+#include <linux/of.h>
+
+#include <soc/tegra/mc.h>
+
+static struct icc_node *
+tegra_mc_of_icc_xlate_onecell(struct of_phandle_args *spec, void *data)
+{
+	struct icc_provider *provider = data;
+	struct icc_node *node;
+
+	list_for_each_entry(node, &provider->nodes, node_list) {
+		if (node->id == spec->args[0])
+			return node;
+	}
+
+	return ERR_PTR(-EINVAL);
+}
+
+static int tegra_mc_icc_set(struct icc_node *src, struct icc_node *dst)
+{
+	return 0;
+}
+
+static int tegra_mc_icc_aggregate(struct icc_node *node,
+				  u32 tag, u32 avg_bw, u32 peak_bw,
+				  u32 *agg_avg, u32 *agg_peak)
+{
+	*agg_avg = min((u64)avg_bw + (*agg_avg), (u64)U32_MAX);
+	*agg_peak = max(*agg_peak, peak_bw);
+
+	return 0;
+}
+
+/*
+ * 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.
+ *
+ * Memory interconnect topology:
+ *
+ *               +----+
+ *   +-----+     |    |
+ *   | GPU +---->+    |
+ *   +-----+     |    |
+ *               |    |     +-----+     +------+
+ *    ...        | MC +---->+ EMC +---->+ EMEM |
+ *               |    |     +-----+     +------+
+ *  +------+     |    |
+ *  | DISP +---->+    |
+ *  +------+     |    |
+ *               +----+
+ */
+int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc)
+{
+	struct icc_provider *provider;
+	struct icc_node *node, *tmp;
+	unsigned int i;
+	int err;
+
+	provider = devm_kzalloc(mc->dev, sizeof(*provider), GFP_KERNEL);
+	if (!provider)
+		return -ENOMEM;
+
+	provider->dev = mc->dev;
+	provider->set = tegra_mc_icc_set;
+	provider->data = provider;
+	provider->xlate = tegra_mc_of_icc_xlate_onecell;
+	provider->aggregate = tegra_mc_icc_aggregate;
+
+	err = icc_provider_add(provider);
+	if (err)
+		return err;
+
+	/* create Memory Controller node */
+	node = icc_node_create(TEGRA_ICC_MC);
+	err = PTR_ERR_OR_ZERO(node);
+	if (err)
+		goto del_provider;
+
+	node->name = "MC";
+	icc_node_add(node, provider);
+
+	/* link Memory Controller with External Memory Controller */
+	err = icc_link_create(node, TEGRA_ICC_EMC);
+	if (err)
+		goto destroy_nodes;
+
+	for (i = 0; i < mc->soc->num_icc_nodes; i++) {
+		/* create MC client node */
+		node = icc_node_create(mc->soc->icc_nodes[i].id);
+		err = PTR_ERR_OR_ZERO(node);
+		if (err)
+			goto destroy_nodes;
+
+		node->name = mc->soc->icc_nodes[i].name;
+		icc_node_add(node, provider);
+
+		/* link Memory Client with Memory Controller */
+		err = icc_link_create(node, TEGRA_ICC_MC);
+		if (err)
+			goto destroy_nodes;
+	}
+
+	return 0;
+
+destroy_nodes:
+	list_for_each_entry_safe(node, tmp, &provider->nodes, node_list) {
+		icc_node_del(node);
+		icc_node_destroy(node->id);
+	}
+
+del_provider:
+	icc_provider_del(provider);
+
+	return err;
+}
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1238e35653d1..593954324259 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -141,6 +141,11 @@  struct tegra_mc_reset_ops {
 			    const struct tegra_mc_reset *rst);
 };
 
+struct tegra_mc_icc_node {
+	const char *name;
+	unsigned int id;
+};
+
 struct tegra_mc_soc {
 	const struct tegra_mc_client *clients;
 	unsigned int num_clients;
@@ -160,6 +165,9 @@  struct tegra_mc_soc {
 	const struct tegra_mc_reset_ops *reset_ops;
 	const struct tegra_mc_reset *resets;
 	unsigned int num_resets;
+
+	const struct tegra_mc_icc_node *icc_nodes;
+	unsigned int num_icc_nodes;
 };
 
 struct tegra_mc {
@@ -184,4 +192,22 @@  struct tegra_mc {
 int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
 unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc);
 
+#ifdef CONFIG_INTERCONNECT_TEGRA
+int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
+int tegra_icc_emc_setup_interconnect(struct device *emc_dev,
+				     unsigned int dram_data_bus_width_bytes);
+#else
+static inline int tegra_icc_mc_setup_interconnect(struct tegra_mc *mc);
+{
+	return 0;
+}
+
+static inline int
+tegra_icc_emc_setup_interconnect(struct device *emc_dev,
+				 unsigned int dram_data_bus_width_bytes)
+{
+	return 0;
+}
+#endif
+
 #endif /* __SOC_TEGRA_MC_H__ */