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: <841dd7b5-98b2-e1a5-2387-a48d7abf4f38@nvidia.com>
Date:   Tue, 7 Mar 2023 00:58:19 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [Patch v1 01/10] 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>,
        <linux-kernel@vger.kernel.org>, <linux-tegra@vger.kernel.org>,
        <linux-pm@vger.kernel.org>, <devicetree@vger.kernel.org>
CC:     <sanjayc@nvidia.com>, <ksitaraman@nvidia.com>, <ishah@nvidia.com>,
        <bbasu@nvidia.com>, Sumit Gupta <sumitg@nvidia.com>
References: <20221220160240.27494-1-sumitg@nvidia.com>
 <20221220160240.27494-2-sumitg@nvidia.com>
 <b4777025-0220-b1e4-f6f3-00d75ec8f0be@linaro.org>
From:   Sumit Gupta <sumitg@nvidia.com>
In-Reply-To: <b4777025-0220-b1e4-f6f3-00d75ec8f0be@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Mar 2023 19:28:41.5464
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: faf5ffa6-fff2-4530-dacd-08db1e78fe7b
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: BN8NAM11FT022.eop-nam11.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB4513
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 22/12/22 17:02, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 20/12/2022 17:02, Sumit Gupta wrote:
>> Adding 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 will be:
>>   MC-Client[1-n] -> MC -> EMC -> EMEM/DRAM
>>
>> MC clients will request for bandwidth to the MC driver which will
>> pass the tegra icc node having current request info to the EMC driver.
>> The EMC driver will send the BPMP Client ID, Client type and bandwidth
>> request info to the BPMP-FW where the final DRAM freq for achieving the
>> requested bandwidth is set based on the passed parameters.
>>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/memory/tegra/mc.c           |  18 ++-
>>   drivers/memory/tegra/tegra186-emc.c | 166 ++++++++++++++++++++++++++++
>>   drivers/memory/tegra/tegra234.c     | 101 ++++++++++++++++-
>>   include/soc/tegra/mc.h              |   7 ++
>>   include/soc/tegra/tegra-icc.h       |  72 ++++++++++++
>>   5 files changed, 362 insertions(+), 2 deletions(-)
>>   create mode 100644 include/soc/tegra/tegra-icc.h
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index 592907546ee6..ff887fb03bce 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -17,6 +17,7 @@
>>   #include <linux/sort.h>
>>
>>   #include <soc/tegra/fuse.h>
>> +#include <soc/tegra/tegra-icc.h>
>>
>>   #include "mc.h"
>>
>> @@ -779,6 +780,7 @@ const char *const tegra_mc_error_names[8] = {
>>    */
>>   static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
>>   {
>> +     struct tegra_icc_node *tnode;
>>        struct icc_node *node;
>>        unsigned int i;
>>        int err;
>> @@ -792,7 +794,11 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
>>        mc->provider.data = &mc->provider;
>>        mc->provider.set = mc->soc->icc_ops->set;
>>        mc->provider.aggregate = mc->soc->icc_ops->aggregate;
>> -     mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended;
>> +     mc->provider.get_bw = mc->soc->icc_ops->get_bw;
>> +     if (mc->soc->icc_ops->xlate)
>> +             mc->provider.xlate = mc->soc->icc_ops->xlate;
>> +     if (mc->soc->icc_ops->xlate_extended)
>> +             mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended;
>>
>>        err = icc_provider_add(&mc->provider);
>>        if (err)
>> @@ -814,6 +820,10 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
>>                goto remove_nodes;
>>
>>        for (i = 0; i < mc->soc->num_clients; i++) {
>> +             tnode = kzalloc(sizeof(*tnode), GFP_KERNEL);
>> +             if (!tnode)
>> +                     return -ENOMEM;
>> +
>>                /* create MC client node */
>>                node = icc_node_create(mc->soc->clients[i].id);
>>                if (IS_ERR(node)) {
>> @@ -828,6 +838,12 @@ static int tegra_mc_interconnect_setup(struct tegra_mc *mc)
>>                err = icc_link_create(node, TEGRA_ICC_MC);
>>                if (err)
>>                        goto remove_nodes;
>> +
>> +             node->data = tnode;
> 
> Where is it freed?
> 
> 
> (...)
> 
Have removed 'struct tegra_icc_node' in v2. Instead 'node->data'
now points to the entry of MC client in the static table 
'tegra234_mc_clients' as below. So, the old alloc of 'struct 
tegra_icc_node' is not required now.

  + node->data = (char *)&(mc->soc->clients[i]);

>>
>>   struct tegra_mc_ops {
>> @@ -238,6 +243,8 @@ struct tegra_mc {
>>        struct {
>>                struct dentry *root;
>>        } debugfs;
>> +
>> +     struct tegra_icc_node *curr_tnode;
>>   };
>>
>>   int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate);
>> diff --git a/include/soc/tegra/tegra-icc.h b/include/soc/tegra/tegra-icc.h
>> new file mode 100644
>> index 000000000000..3855d8571281
>> --- /dev/null
>> +++ b/include/soc/tegra/tegra-icc.h
> 
> Why not in linux?
> 
Moved the file to 'include/linux/tegra-icc.h' in v2.

>> @@ -0,0 +1,72 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2022-2023 NVIDIA CORPORATION.  All rights reserved.
>> + */
>> +
>> +#ifndef MEMORY_TEGRA_ICC_H
> 
> This does not match the path/name.
> 
Have changed path name to below in v2.

   +#ifndef LINUX_TEGRA_ICC_H
   +#define LINUX_TEGRA_ICC_H

>> +#define MEMORY_TEGRA_ICC_H
>> +
>> +enum tegra_icc_client_type {
>> +     TEGRA_ICC_NONE,
>> +     TEGRA_ICC_NISO,
>> +     TEGRA_ICC_ISO_DISPLAY,
>> +     TEGRA_ICC_ISO_VI,
>> +     TEGRA_ICC_ISO_AUDIO,
>> +     TEGRA_ICC_ISO_VIFAL,
>> +};
>> +
>> +struct tegra_icc_node {
>> +     struct icc_node *node;
>> +     struct tegra_mc *mc;
>> +     u32 bpmp_id;
>> +     u32 type;
>> +};
>> +
>> +/* ICC ID's for MC client's used in BPMP */
>> +#define TEGRA_ICC_BPMP_DEBUG         1
>> +#define TEGRA_ICC_BPMP_CPU_CLUSTER0  2
>> +#define TEGRA_ICC_BPMP_CPU_CLUSTER1  3
>> +#define TEGRA_ICC_BPMP_CPU_CLUSTER2  4
>> +#define TEGRA_ICC_BPMP_GPU           5
>> +#define TEGRA_ICC_BPMP_CACTMON               6
>> +#define TEGRA_ICC_BPMP_DISPLAY               7
>> +#define TEGRA_ICC_BPMP_VI            8
>> +#define TEGRA_ICC_BPMP_EQOS          9
>> +#define TEGRA_ICC_BPMP_PCIE_0                10
>> +#define TEGRA_ICC_BPMP_PCIE_1                11
>> +#define TEGRA_ICC_BPMP_PCIE_2                12
>> +#define TEGRA_ICC_BPMP_PCIE_3                13
>> +#define TEGRA_ICC_BPMP_PCIE_4                14
>> +#define TEGRA_ICC_BPMP_PCIE_5                15
>> +#define TEGRA_ICC_BPMP_PCIE_6                16
>> +#define TEGRA_ICC_BPMP_PCIE_7                17
>> +#define TEGRA_ICC_BPMP_PCIE_8                18
>> +#define TEGRA_ICC_BPMP_PCIE_9                19
>> +#define TEGRA_ICC_BPMP_PCIE_10               20
>> +#define TEGRA_ICC_BPMP_DLA_0         21
>> +#define TEGRA_ICC_BPMP_DLA_1         22
>> +#define TEGRA_ICC_BPMP_SDMMC_1               23
>> +#define TEGRA_ICC_BPMP_SDMMC_2               24
>> +#define TEGRA_ICC_BPMP_SDMMC_3               25
>> +#define TEGRA_ICC_BPMP_SDMMC_4               26
>> +#define TEGRA_ICC_BPMP_NVDEC         27
>> +#define TEGRA_ICC_BPMP_NVENC         28
>> +#define TEGRA_ICC_BPMP_NVJPG_0               29
>> +#define TEGRA_ICC_BPMP_NVJPG_1               30
>> +#define TEGRA_ICC_BPMP_OFAA          31
>> +#define TEGRA_ICC_BPMP_XUSB_HOST     32
>> +#define TEGRA_ICC_BPMP_XUSB_DEV              33
>> +#define TEGRA_ICC_BPMP_TSEC          34
>> +#define TEGRA_ICC_BPMP_VIC           35
>> +#define TEGRA_ICC_BPMP_APE           36
>> +#define TEGRA_ICC_BPMP_APEDMA                37
>> +#define TEGRA_ICC_BPMP_SE            38
>> +#define TEGRA_ICC_BPMP_ISP           39
>> +#define TEGRA_ICC_BPMP_HDA           40
>> +#define TEGRA_ICC_BPMP_VIFAL         41
>> +#define TEGRA_ICC_BPMP_VI2FAL                42
>> +#define TEGRA_ICC_BPMP_VI2           43
>> +#define TEGRA_ICC_BPMP_RCE           44
>> +#define TEGRA_ICC_BPMP_PVA           45
>> +
>> +#endif /* MEMORY_TEGRA_ICC_H */
> 
> Best regards,
> Krzysztof
>