Re: [Patch v7 1/8] memory: tegra: add interconnect support for DRAM scaling in Tegra234

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 24/04/2023 15:13, Sumit Gupta wrote:
> Add Interconnect framework support to dynamically set the DRAM
> bandwidth from different clients. Both the MC and EMC drivers are
> added as ICC providers. The path for any request is:
>  MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM
> 
Thank you for your patch. There is something to discuss/improve.


> +static int tegra_emc_interconnect_init(struct tegra186_emc *emc)
> +{
> +	struct tegra_mc *mc = dev_get_drvdata(emc->dev->parent);
> +	const struct tegra_mc_soc *soc = mc->soc;
> +	struct icc_node *node;
> +	int err;
> +
> +	emc->provider.dev = emc->dev;
> +	emc->provider.set = tegra_emc_icc_set_bw;
> +	emc->provider.data = &emc->provider;
> +	emc->provider.aggregate = soc->icc_ops->aggregate;
> +	emc->provider.xlate = tegra_emc_of_icc_xlate;
> +	emc->provider.get_bw = tegra_emc_icc_get_init_bw;
> +
> +	icc_provider_init(&emc->provider);
> +
> +	/* create External Memory Controller node */
> +	node = icc_node_create(TEGRA_ICC_EMC);
> +	if (IS_ERR(node)) {
> +		err = PTR_ERR(node);
> +		goto err_msg;
> +	}
> +
> +	node->name = "External Memory Controller";
> +	icc_node_add(node, &emc->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->provider);
> +
> +	err = icc_provider_register(&emc->provider);
> +	if (err)
> +		goto remove_nodes;
> +
> +	return 0;
> +
> +remove_nodes:
> +	icc_nodes_remove(&emc->provider);
> +err_msg:
> +	dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
> +
> +	return err;
> +}
> +
>  static int tegra186_emc_probe(struct platform_device *pdev)
>  {
> +	struct tegra_mc *mc = dev_get_drvdata(pdev->dev.parent);
>  	struct mrq_emc_dvfs_latency_response response;
>  	struct tegra_bpmp_message msg;
>  	struct tegra186_emc *emc;
> @@ -236,6 +339,29 @@ static int tegra186_emc_probe(struct platform_device *pdev)
>  	debugfs_create_file("max_rate", S_IRUGO | S_IWUSR, emc->debugfs.root,
>  			    emc, &tegra186_emc_debug_max_rate_fops);
>  
> +	if (mc && mc->soc->icc_ops) {
> +		/*
> +		 * Initialize the ICC even if BPMP-FW doesn't support 'MRQ_BWMGR_INT'.
> +		 * Use the flag 'mc->bwmgr_mrq_supported' within MC driver and return
> +		 * EINVAL instead of passing the request to BPMP-FW later when the BW
> +		 * request is made by client with 'icc_set_bw()' call.
> +		 */
> +		err = tegra_emc_interconnect_init(emc);
> +		if (err)
> +			goto put_bpmp;
> +
> +		if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
> +			mc->bwmgr_mrq_supported = true;
> +
> +			/*
> +			 * MC driver probe can't get BPMP reference as it gets probed
> +			 * earlier than BPMP. So, save the BPMP ref got from the EMC
> +			 * DT node in the mc->bpmp and use it in MC's icc_set hook.
> +			 */
> +			mc->bpmp = emc->bpmp;

This (and ()) are called without any locking. You register first the
interconnect, so set() callback can be used, right? Then set() could be
called anytime between tegra_emc_interconnect_init() and assignment
above. How do you synchronize these?

Best regards,
Krzysztof