Re: [PATCH v3 5/9] memory: tegra210: Support interconnect framework

[Krzysztof Kozlowski <krzk@kernel.org>]

On 06/09/2025 22:16, Aaron Kling via B4 Relay wrote:
> +
> +static int tegra_emc_interconnect_init(struct tegra210_emc *emc)
> +{
> +	const struct tegra_mc_soc *soc = emc->mc->soc;
> +	struct icc_node *node;
> +	int err;
> +
> +	emc->icc_provider.dev = emc->dev;
> +	emc->icc_provider.set = emc_icc_set;
> +	emc->icc_provider.data = &emc->icc_provider;
> +	emc->icc_provider.aggregate = soc->icc_ops->aggregate;
> +	emc->icc_provider.xlate_extended = emc_of_icc_xlate_extended;
> +	emc->icc_provider.get_bw = tegra_emc_icc_get_init_bw;
> +
> +	icc_provider_init(&emc->icc_provider);
> +
> +	/* create External Memory Controller node */
> +	node = icc_node_create(TEGRA_ICC_EMC);
> +	if (IS_ERR(node)) {
> +		err = PTR_ERR(node);
> +		goto err_msg;

return dev_err_probe

> +	}
> +
> +	node->name = "External Memory Controller";
> +	icc_node_add(node, &emc->icc_provider);
> +
> +	/* link External Memory Controller to External Memory (DRAM) */
> +	err = icc_link_create(node, TEGRA_ICC_EMEM);
> +	if (err)
> +		goto remove_nodes;
> +
> +	/* create External Memory node */
> +	node = icc_node_create(TEGRA_ICC_EMEM);
> +	if (IS_ERR(node)) {
> +		err = PTR_ERR(node);
> +		goto remove_nodes;
> +	}
> +
> +	node->name = "External Memory (DRAM)";
> +	icc_node_add(node, &emc->icc_provider);
> +
> +	err = icc_provider_register(&emc->icc_provider);
> +	if (err)
> +		goto remove_nodes;
> +
> +	return 0;
> +
> +remove_nodes:
> +	icc_nodes_remove(&emc->icc_provider);
> +err_msg:
> +	dev_err(emc->dev, "failed to initialize ICC: %d\n", err);

Write descriptive error messages or skip it. It's printing errors also
on ENOMEM.

I will send a patch to fix existing code.

> +
> +	return err;
> +}
> +
> +static int tegra_emc_opp_table_init(struct tegra210_emc *emc)
> +{
> +	u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> +	struct dev_pm_opp *opp;
> +	unsigned long rate;
> +	int opp_token, err, max_opps, i;
> +
> +	err = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
> +	if (err < 0) {
> +		dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
> +		return err;

return dev_err_probe

> +	}
> +	opp_token = err;
> +
> +	err = dev_pm_opp_of_add_table(emc->dev);
> +	if (err) {
> +		if (err == -ENODEV)
> +			dev_warn(emc->dev, "OPP table not found, please update your device tree\n");
> +		else
> +			dev_err(emc->dev, "failed to add OPP table: %d\n", err);
> +
> +		goto put_hw_table;
> +	}
> +
> +	max_opps = dev_pm_opp_get_opp_count(emc->dev);
> +	if (max_opps <= 0) {
> +		dev_err(emc->dev, "Failed to add OPPs\n");
> +		goto remove_table;
> +	}
> +
> +	if (emc->num_timings != max_opps) {
> +		dev_err(emc->dev, "OPP table does not match emc table\n");
> +		goto remove_table;
> +	}
> +
> +	for (i = 0; i < emc->num_timings; i++) {
> +		rate = emc->timings[i].rate * 1000;
> +		opp = dev_pm_opp_find_freq_exact(emc->dev, rate, true);
> +		if (IS_ERR(opp)) {
> +			dev_err(emc->dev, "Rate %lu not found in OPP table\n", rate);
> +			goto remove_table;
> +		}
> +
> +		dev_pm_opp_put(opp);
> +	}
> +
> +	dev_info_once(emc->dev, "OPP HW ver. 0x%x, current clock rate %lu MHz\n",
> +		      hw_version, clk_get_rate(emc->clk) / 1000000);
> +
> +	return 0;
> +
> +remove_table:
> +	dev_pm_opp_of_remove_table(emc->dev);
> +put_hw_table:
> +	dev_pm_opp_put_supported_hw(opp_token);
> +
> +	return err;
> +}
> +

...

> diff --git a/drivers/memory/tegra/tegra210.c b/drivers/memory/tegra/tegra210.c
> index cfa61dd885577a8fbd79c396a1316101197ca1f2..20828a07d2d0cafa739b534c20c12f065b276669 100644
> --- a/drivers/memory/tegra/tegra210.c
> +++ b/drivers/memory/tegra/tegra210.c
> @@ -3,6 +3,9 @@
>   * Copyright (C) 2015 NVIDIA CORPORATION.  All rights reserved.
>   */
>  
> +#include <linux/of.h>
> +#include <linux/device.h>
> +
>  #include <dt-bindings/memory/tegra210-mc.h>
>  
>  #include "mc.h"
> @@ -1273,6 +1276,83 @@ static const struct tegra_mc_reset tegra210_mc_resets[] = {
>  	TEGRA210_MC_RESET(TSECB,     0x970, 0x974, 13),
>  };
>  
> +static int tegra210_mc_icc_set(struct icc_node *src, struct icc_node *dst)
> +{
> +	/* TODO: program PTSA */
> +	return 0;
> +}
> +
> +static int tegra210_mc_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
> +				     u32 peak_bw, u32 *agg_avg, u32 *agg_peak)
> +{
> +	/*
> +	 * ISO clients need to reserve extra bandwidth up-front because
> +	 * there could be high bandwidth pressure during initial filling
> +	 * of the client's FIFO buffers.  Secondly, we need to take into
> +	 * account impurities of the memory subsystem.
> +	 */
> +	if (tag & TEGRA_MC_ICC_TAG_ISO)
> +		peak_bw = tegra_mc_scale_percents(peak_bw, 400);
> +
> +	*agg_avg += avg_bw;
> +	*agg_peak = max(*agg_peak, peak_bw);
> +
> +	return 0;
> +}
> +
> +static struct icc_node_data *
> +tegra210_mc_of_icc_xlate_extended(const struct of_phandle_args *spec, void *data)
> +{
> +	struct tegra_mc *mc = icc_provider_to_tegra_mc(data);
> +	const struct tegra_mc_client *client;
> +	unsigned int i, idx = spec->args[0];
> +	struct icc_node_data *ndata;
> +	struct icc_node *node;
> +
> +	list_for_each_entry(node, &mc->provider.nodes, node_list) {
> +		if (node->id != idx)
> +			continue;
> +
> +		ndata = kzalloc(sizeof(*ndata), GFP_KERNEL);
> +		if (!ndata)
> +			return ERR_PTR(-ENOMEM);
> +
> +		client = &mc->soc->clients[idx];
> +		ndata->node = node;
> +
> +		switch (client->swgroup) {
> +		case TEGRA_SWGROUP_DC:
> +		case TEGRA_SWGROUP_DCB:
> +		case TEGRA_SWGROUP_PTC:
> +		case TEGRA_SWGROUP_VI:
> +			/* these clients are isochronous by default */
> +			ndata->tag = TEGRA_MC_ICC_TAG_ISO;
> +			break;
> +
> +		default:
> +			ndata->tag = TEGRA_MC_ICC_TAG_DEFAULT;
> +			break;
> +		}
> +
> +		return ndata;
> +	}
> +
> +	for (i = 0; i < mc->soc->num_clients; i++) {
> +		if (mc->soc->clients[i].id == idx)
> +			return ERR_PTR(-EPROBE_DEFER);

This feels like more reasonable use of deferred probe, but I wonder how
is this condition possible.

This xlate is called at earliest after MC ICC is registered. At this
tage all nodes are created, so previous list_for_each_entry() should
succeed and return. If previous list_for_each_entry() did not return,
how can you find a matching client for which the node will be added
later, thus deferred probe?

This is again existing code, of course :)



Best regards,
Krzysztof