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

Sumit Gupta
2023-04-24T15:01:00Z

On 24/04/23 19:18, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> 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
>

Currently, the tegra234_mc_icc_set() has NULL check. So, it will give
this error.
if (!mc->bpmp) {
dev_err(mc->dev, "BPMP reference NULL\n");
return -ENOENT;
}

To fix, we can do the below change and swap the order. Please let me
know if this sounds fine ?
if (mc && mc->soc->icc_ops) {
if (tegra_bpmp_mrq_is_supported(emc->bpmp, MRQ_BWMGR_INT)) {
mc->bwmgr_mrq_supported = true;
mc->bpmp = emc->bpmp;
}

err = tegra_emc_interconnect_init(emc);
if (err)
goto put_bpmp;

}

Thank you,
Sumit Gupta