On 24/04/2023 17:21, Sumit Gupta wrote:
>
>
> 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) {
How does it solve concurrent accesses and re-ordering of instructions by
compiler or CPU?
>> 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;
Sorry, I didn't check the code. I assume you should do it...
Best regards,
Krzysztof
