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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux