Re: [PATCH v4 28/37] memory: tegra: Register as interconnect provider

Hi Dmitry,

Thank you for updating the patches!

On 6/9/20 16:13, Dmitry Osipenko wrote:

Now memory controller is a memory interconnection provider. This allows us to use interconnect API in order to change memory configuration.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

drivers/memory/tegra/Kconfig | 1 + drivers/memory/tegra/mc.c | 114 +++++++++++++++++++++++++++++++++++ drivers/memory/tegra/mc.h | 8 +++ include/soc/tegra/mc.h | 3 + 4 files changed, 126 insertions(+)

diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig index 5bf75b316a2f..7055fdef2c32 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 772aa021b5f6..7ef7ac9e103e 100644 --- a/drivers/memory/tegra/mc.c +++ b/drivers/memory/tegra/mc.c @@ -594,6 +594,118 @@ static __maybe_unused irqreturn_t tegra20_mc_irq(int irq, void *data) return IRQ_HANDLED; }

+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. 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_onecell_data *data;
• struct icc_node *node;
• unsigned int num_nodes;
• unsigned int i;
• int err;
• /* older device-trees don't have interconnect properties */
• if (!of_find_property(mc->dev->of_node, "#interconnect-cells", NULL))

• return 0;
  

• num_nodes = mc->soc->num_clients;
• data = devm_kzalloc(mc->dev, struct_size(data, nodes, num_nodes),

• 	    GFP_KERNEL);
  

• if (!data)

• return -ENOMEM;
  

• mc->provider.dev = mc->dev;
• mc->provider.set = tegra_mc_icc_set;

Hmm, maybe the core should not require a set() implementation and we can just make it optional instead. Then the dummy function would not be needed.

• mc->provider.data = data;
• mc->provider.xlate = of_icc_xlate_onecell;
• mc->provider.aggregate = tegra_mc_icc_aggregate;
• err = icc_provider_add(&mc->provider);
• if (err)

• goto err_msg;
  

Nit: I am planning to re-organize some of the existing drivers to call icc_provider_add() after the topology is populated. Could you please move this after the nodes are created and linked.

• /* 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 < num_nodes; 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);
  

• /* link Memory Client to Memory Controller */
  

• err = icc_link_create(node, TEGRA_ICC_MC);
  

• if (err)
  

• 	goto remove_nodes;
  

• data->nodes[i] = node;
  

• }
• data->num_nodes = num_nodes;
• 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; @@ -702,6 +814,8 @@ static int tegra_mc_probe(struct platform_device *pdev) } }

• tegra_mc_interconnect_setup(mc);
• 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

#endif /* MEMORY_TEGRA_MC_H */ diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h index 1238e35653d1..71de023f9f47 100644 --- a/include/soc/tegra/mc.h +++ b/include/soc/tegra/mc.h @@ -7,6 +7,7 @@ #define __SOC_TEGRA_MC_H__

#include <linux/err.h> +#include <linux/interconnect-provider.h> #include <linux/reset-controller.h> #include <linux/types.h>

@@ -178,6 +179,8 @@ struct tegra_mc {

struct reset_controller_dev reset;

• struct icc_provider provider;
• spinlock_t lock;

};

The rest looks good to me!

Thanks, Georgi