Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
Message-ID: <14438cf9-ec78-afb5-107a-4ed954ac0eb7@nvidia.com>
Date:   Tue, 9 May 2023 18:47:47 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [Patch v7 1/8] memory: tegra: add interconnect support for DRAM
 scaling in Tegra234
Content-Language: en-US
To:     Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <treding@nvidia.com>, <dmitry.osipenko@collabora.com>,
        <viresh.kumar@linaro.org>, <rafael@kernel.org>,
        <jonathanh@nvidia.com>, <robh+dt@kernel.org>,
        <lpieralisi@kernel.org>, <helgaas@kernel.org>
CC:     <linux-kernel@vger.kernel.org>, <linux-tegra@vger.kernel.org>,
        <linux-pm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-pci@vger.kernel.org>, <mmaddireddy@nvidia.com>,
        <kw@linux.com>, <bhelgaas@google.com>, <vidyas@nvidia.com>,
        <sanjayc@nvidia.com>, <ksitaraman@nvidia.com>, <ishah@nvidia.com>,
        <bbasu@nvidia.com>, Sumit Gupta <sumitg@nvidia.com>
References: <20230424131337.20151-1-sumitg@nvidia.com>
 <20230424131337.20151-2-sumitg@nvidia.com>
 <7c6c6584-204a-ada1-d669-2e8bef50e5e5@linaro.org>
 <3071273b-b03b-5fc8-ffa1-9b18311a3a5d@nvidia.com>
 <5ab9687e-756d-f94b-b085-931d4ea534c1@nvidia.com>
 <10b32e55-4d28-5405-035e-c73a514c95e4@linaro.org>
From:   Sumit Gupta <sumitg@nvidia.com>
In-Reply-To: <10b32e55-4d28-5405-035e-c73a514c95e4@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 May 2023 13:18:14.1047
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 0827f75c-2ac9-48b5-df63-08db508fd84b
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CY4PEPF0000E9D8.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR12MB6594
X-Spam-Status: No, score=-1.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 07/05/23 14:41, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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?
> 

Now, the "mc->bpmp" is set before tegra_emc_interconnect_init().
So, until the EMC interconnect initializes, set() won't be
called as the devm_of_icc_get() call will fail.

Added the change in v8.

Thank you,
Sumit Gupta

>>>            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
>