Messages in this thread | | | Date | Mon, 24 Apr 2023 20:51:44 +0530 | Subject | Re: [Patch v7 1/8] memory: tegra: add interconnect support for DRAM scaling in Tegra234 | From | Sumit Gupta <> |
| |
On 24/04/23 20:30, Sumit Gupta wrote: > > > 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; > > } Sorry, replied too soon. Missed setting "mc->bpmp" to NULL. Sharing new proposed change. Please let me know if this looks OK.
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) { mc->bpmp = NULL; goto put_bpmp; } }
> > Thank you, > Sumit Gupta
| |