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

[Sumit Gupta <sumitg@nvidia.com>]

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