From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Sumit Gupta <sumitg@nvidia.com>,
treding@nvidia.com, dmitry.osipenko@collabora.com,
viresh.kumar@linaro.org, rafael@kernel.org, jonathanh@nvidia.com,
robh+dt@kernel.org, lpieralisi@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
Subject: Re: [Patch v4 03/10] memory: tegra: add interconnect support for DRAM scaling in Tegra234
Date: Tue, 28 Mar 2023 09:31:58 +0200 [thread overview]
Message-ID: <8b04a266-20eb-f1c1-278f-764b1b06b78b@linaro.org> (raw)
In-Reply-To: <20230327161426.32639-4-sumitg@nvidia.com>
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@nvidia.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.
> +
> 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.
> + }
> +
> 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.
> */
>
> #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...
> +#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
next prev parent reply other threads:[~2023-03-28 7:32 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-27 16:14 [Patch v4 00/10] Tegra234 Memory interconnect support Sumit Gupta
2023-03-27 16:14 ` [Patch v4 01/10] dt-bindings: memory: tegra: add bpmp ref in tegra234-mc node Sumit Gupta
2023-03-28 7:23 ` Krzysztof Kozlowski
2023-03-28 10:48 ` Thierry Reding
2023-03-28 11:22 ` Krzysztof Kozlowski
2023-03-28 12:48 ` Thierry Reding
2023-03-29 17:12 ` Sumit Gupta
2023-04-02 10:47 ` Krzysztof Kozlowski
2023-04-04 11:44 ` Thierry Reding
2023-03-28 10:42 ` Thierry Reding
2023-03-27 16:14 ` [Patch v4 02/10] arm64: " Sumit Gupta
2023-03-28 7:21 ` Krzysztof Kozlowski
2023-03-28 12:41 ` Sumit Gupta
2023-03-27 16:14 ` [Patch v4 03/10] memory: tegra: add interconnect support for DRAM scaling in Tegra234 Sumit Gupta
2023-03-28 7:31 ` Krzysztof Kozlowski [this message]
2023-03-28 11:05 ` Thierry Reding
2023-03-28 12:30 ` Sumit Gupta
2023-03-28 12:34 ` Sumit Gupta
2023-03-27 16:14 ` [Patch v4 04/10] memory: tegra: add mc clients for Tegra234 Sumit Gupta
2023-03-27 16:14 ` [Patch v4 05/10] memory: tegra: add software mc clients in Tegra234 Sumit Gupta
2023-03-27 16:14 ` [Patch v4 06/10] dt-bindings: tegra: add icc ids for dummy MC clients Sumit Gupta
2023-03-27 16:14 ` [Patch v4 07/10] arm64: tegra: Add cpu OPP tables and interconnects property Sumit Gupta
2023-03-27 16:14 ` [Patch v4 08/10] cpufreq: tegra194: add OPP support and set bandwidth Sumit Gupta
2023-03-27 16:14 ` [Patch v4 09/10] memory: tegra: make cpu cluster bw request a multiple of mc channels Sumit Gupta
2023-03-27 16:14 ` [Patch v4 10/10] PCI: tegra194: add interconnect support in Tegra234 Sumit Gupta
2023-03-28 17:53 ` Bjorn Helgaas
2023-03-29 9:14 ` Sumit Gupta
2023-03-29 16:59 ` Bjorn Helgaas
2023-03-29 17:58 ` Sumit Gupta
2023-03-29 19:17 ` Bjorn Helgaas
2023-03-30 8:00 ` Sumit Gupta
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8b04a266-20eb-f1c1-278f-764b1b06b78b@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=bbasu@nvidia.com \
--cc=bhelgaas@google.com \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.osipenko@collabora.com \
--cc=ishah@nvidia.com \
--cc=jonathanh@nvidia.com \
--cc=ksitaraman@nvidia.com \
--cc=kw@linux.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-tegra@vger.kernel.org \
--cc=lpieralisi@kernel.org \
--cc=mmaddireddy@nvidia.com \
--cc=rafael@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sanjayc@nvidia.com \
--cc=sumitg@nvidia.com \
--cc=treding@nvidia.com \
--cc=vidyas@nvidia.com \
--cc=viresh.kumar@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).