Messages in this thread | | | Date | Mon, 24 Apr 2023 20:30:15 +0530 | Subject | Re: [Patch v7 1/8] memory: tegra: add interconnect support for DRAM scaling in Tegra234 | From | Sumit Gupta <> |
| |
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
| |