| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <34631b30-7589-9b9d-4dd0-2e2606f62887@nvidia.com>
Date: Tue, 28 Mar 2023 18:04:37 +0530
From: Sumit Gupta <sumitg@...dia.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
<treding@...dia.com>, <dmitry.osipenko@...labora.com>,
<viresh.kumar@...aro.org>, <rafael@...nel.org>,
<jonathanh@...dia.com>, <robh+dt@...nel.org>,
<lpieralisi@...nel.org>
CC: <linux-kernel@...r.kernel.org>, <linux-tegra@...r.kernel.org>,
<linux-pm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-pci@...r.kernel.org>, <mmaddireddy@...dia.com>,
<kw@...ux.com>, <bhelgaas@...gle.com>, <vidyas@...dia.com>,
<sanjayc@...dia.com>, <ksitaraman@...dia.com>, <ishah@...dia.com>,
<bbasu@...dia.com>, Sumit Gupta <sumitg@...dia.com>
Subject: Re: [Patch v4 03/10] memory: tegra: add interconnect support for DRAM
scaling in Tegra234
On 28/03/23 13:01, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 27/03/2023 18:14, 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
>>
>> MC client's request for bandwidth will go to the MC driver which
>> passes the client request info like BPMP Client ID, Client type
>> and the Bandwidth to the BPMP-FW. The final DRAM freq to achieve
>> the requested bandwidth is set by the BPMP-FW based on the passed
>> parameters.
>>
>> Signed-off-by: Sumit Gupta <sumitg@...dia.com>
>> ---
>> drivers/memory/tegra/mc.c | 5 +
>> drivers/memory/tegra/tegra186-emc.c | 125 ++++++++++++++++++++++++
>> drivers/memory/tegra/tegra186.c | 3 +
>> drivers/memory/tegra/tegra234.c | 143 +++++++++++++++++++++++++++-
>> include/linux/tegra-icc.h | 65 +++++++++++++
>> include/soc/tegra/mc.h | 7 ++
>> 6 files changed, 347 insertions(+), 1 deletion(-)
>> create mode 100644 include/linux/tegra-icc.h
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index 9082b6c3763d..983455b1f98d 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -15,6 +15,7 @@
>> #include <linux/platform_device.h>
>> #include <linux/slab.h>
>> #include <linux/sort.h>
>> +#include <linux/tegra-icc.h>
>>
>> #include <soc/tegra/fuse.h>
>>
>> @@ -792,6 +793,8 @@ 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.get_bw = mc->soc->icc_ops->get_bw;
>> + mc->provider.xlate = mc->soc->icc_ops->xlate;
>> mc->provider.xlate_extended = mc->soc->icc_ops->xlate_extended;
>>
>> icc_provider_init(&mc->provider);
>> @@ -824,6 +827,8 @@ 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 = (struct tegra_mc_client *)&(mc->soc->clients[i]);
>> }
>>
>> err = icc_provider_register(&mc->provider);
>> diff --git a/drivers/memory/tegra/tegra186-emc.c b/drivers/memory/tegra/tegra186-emc.c
>> index e935ad4e95b6..1eefcf2ac0c7 100644
>> --- a/drivers/memory/tegra/tegra186-emc.c
>> +++ b/drivers/memory/tegra/tegra186-emc.c
>> @@ -7,9 +7,11 @@
>> #include <linux/debugfs.h>
>> #include <linux/module.h>
>> #include <linux/mod_devicetable.h>
>> +#include <linux/of_platform.h>
>> #include <linux/platform_device.h>
>>
>> #include <soc/tegra/bpmp.h>
>> +#include "mc.h"
>>
>> struct tegra186_emc_dvfs {
>> unsigned long latency;
>> @@ -29,8 +31,15 @@ struct tegra186_emc {
>> unsigned long min_rate;
>> unsigned long max_rate;
>> } debugfs;
>> +
>> + struct icc_provider provider;
>> };
>>
>> +static inline struct tegra186_emc *to_tegra186_emc(struct icc_provider *provider)
>> +{
>> + return container_of(provider, struct tegra186_emc, provider);
>> +}
>> +
>> /*
>> * debugfs interface
>> *
>> @@ -146,11 +155,104 @@ DEFINE_DEBUGFS_ATTRIBUTE(tegra186_emc_debug_max_rate_fops,
>> tegra186_emc_debug_max_rate_get,
>> tegra186_emc_debug_max_rate_set, "%llu\n");
>>
>> +/*
>> + * tegra_emc_icc_set_bw() - Set BW api for EMC provider
>> + * @src: ICC node for External Memory Controller (EMC)
>> + * @dst: ICC node for External Memory (DRAM)
>> + *
>> + * Do nothing here as info to BPMP-FW is now passed in the BW set function
>> + * of the MC driver. BPMP-FW sets the final Freq based on the passed values.
>> + */
>> +static int tegra_emc_icc_set_bw(struct icc_node *src, struct icc_node *dst)
>> +{
>> + return 0;
>> +}
>> +
>> +static struct icc_node *
>> +tegra_emc_of_icc_xlate(struct of_phandle_args *spec, void *data)
>> +{
>> + struct icc_provider *provider = data;
>> + struct icc_node *node;
>> +
>> + /* External Memory is the only possible ICC route */
>> + list_for_each_entry(node, &provider->nodes, node_list) {
>> + if (node->id != TEGRA_ICC_EMEM)
>> + continue;
>> +
>> + return node;
>> + }
>> +
>> + return ERR_PTR(-EPROBE_DEFER);
>> +}
>> +
>> +static int tegra_emc_icc_get_init_bw(struct icc_node *node, u32 *avg, u32 *peak)
>> +{
>> + *avg = 0;
>> + *peak = 0;
>> +
>> + return 0;
>> +}
>> +
>> +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;
>
> Blank line
>
>> +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 mrq_emc_dvfs_latency_response response;
>> struct tegra_bpmp_message msg;
>> struct tegra186_emc *emc;
>> + struct tegra_mc *mc;
>> unsigned int i;
>> int err;
>>
>> @@ -158,6 +260,9 @@ static int tegra186_emc_probe(struct platform_device *pdev)
>> if (!emc)
>> return -ENOMEM;
>>
>> + platform_set_drvdata(pdev, emc);
>> + emc->dev = &pdev->dev;
>
> This patch looks like stiched from two or more patches... emc->dev does
> not look like new member of emc, thus why do you set in exisitng
> function in this patch? Why it wasn't needed before?
>
> Same about line before.
>
Replied in other mail. will fix this.
>> +
>> emc->bpmp = tegra_bpmp_get(&pdev->dev);
>> if (IS_ERR(emc->bpmp))
>> return dev_err_probe(&pdev->dev, PTR_ERR(emc->bpmp), "failed to get BPMP\n");
>> @@ -236,6 +341,25 @@ 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);
>>
>> + mc = dev_get_drvdata(emc->dev->parent);
>> + 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;
>> + else
>> +
>
> Drop blank line.
>
>> + dev_info(&pdev->dev, "MRQ_BWMGR_INT not present\n");
>
> And what user is supposed to do with this? Either make it descriptive or
> drop.
>
Replied in other mail. will fix this.
>> + }
>> +
>> return 0;
>>
>> put_bpmp:
>> @@ -272,6 +396,7 @@ static struct platform_driver tegra186_emc_driver = {
>> .name = "tegra186-emc",
>> .of_match_table = tegra186_emc_of_match,
>> .suppress_bind_attrs = true,
>> + .sync_state = icc_sync_state,
>> },
>> .probe = tegra186_emc_probe,
>> .remove = tegra186_emc_remove,
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 7bb73f06fad3..386e029e41bb 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -10,6 +10,7 @@
>> #include <linux/of_device.h>
>> #include <linux/platform_device.h>
>>
>> +#include <soc/tegra/bpmp.h>
>> #include <soc/tegra/mc.h>
>>
>> #if defined(CONFIG_ARCH_TEGRA_186_SOC)
>> @@ -65,6 +66,8 @@ static int tegra186_mc_probe(struct tegra_mc *mc)
>> static void tegra186_mc_remove(struct tegra_mc *mc)
>> {
>> of_platform_depopulate(mc->dev);
>> +
>> + tegra_bpmp_put(mc->bpmp);
>> }
>>
>> #if IS_ENABLED(CONFIG_IOMMU_API)
>> diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
>> index 02dcc5748bba..4f34247c9bda 100644
>> --- a/drivers/memory/tegra/tegra234.c
>> +++ b/drivers/memory/tegra/tegra234.c
>> @@ -1,18 +1,24 @@
>> // SPDX-License-Identifier: GPL-2.0-only
>> /*
>> - * Copyright (C) 2021-2022, NVIDIA CORPORATION. All rights reserved.
>> + * Copyright (C) 20212-2023, NVIDIA CORPORATION. All rights reserved.
>
> Typo, 2021.
>
Will fix.
>> */
>>
>> #include <soc/tegra/mc.h>
>>
>> #include <dt-bindings/memory/tegra234-mc.h>
>> +#include <linux/interconnect.h>
>> +#include <linux/of_device.h>
>
> One more suprising change...
>
Will remove the header file "of_device.h".
>> +#include <linux/tegra-icc.h>
>>
>> +#include <soc/tegra/bpmp.h>
>> #include "mc.h"
>>
>> static const struct tegra_mc_client tegra234_mc_clients[] = {
>> {
>> .id = TEGRA234_MEMORY_CLIENT_MGBEARD,
>> .name = "mgbeard",
>> + .bpmp_id = TEGRA_ICC_BPMP_EQOS,
>> + .type = TEGRA_ICC_NISO,
>> .sid = TEGRA234_SID_MGBE,
>> .regs = {
>> .sid = {
>
>
> Best regards,
> Krzysztof
>
Powered by blists - more mailing lists