| 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: <CALHNRZ-A6L1s_Uc0cO-+akHyzHGkb4bkYd0pNKX96DqJfOBp9g@mail.gmail.com>
Date: Thu, 4 Sep 2025 12:28:40 -0500
From: Aaron Kling <webgeek1234@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Thierry Reding <thierry.reding@...il.com>, Jonathan Hunter <jonathanh@...dia.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, MyungJoo Ham <myungjoo.ham@...sung.com>,
Kyungmin Park <kyungmin.park@...sung.com>, Chanwoo Choi <cw00.choi@...sung.com>,
Dmitry Osipenko <digetx@...il.com>, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-tegra@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v2 5/8] memory: tegra210: Support interconnect framework
On Thu, Sep 4, 2025 at 3:17 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On Wed, Sep 03, 2025 at 02:50:11PM -0500, Aaron Kling wrote:
> > This makes mc and emc interconnect providers and allows for dynamic
> > memory clock scaling.
> >
> > Signed-off-by: Aaron Kling <webgeek1234@...il.com>
> > ---
> > drivers/memory/tegra/Kconfig | 1 +
> > drivers/memory/tegra/tegra210-emc-core.c | 274 ++++++++++++++++++++++++++++++-
> > drivers/memory/tegra/tegra210-emc.h | 23 +++
> > drivers/memory/tegra/tegra210.c | 81 +++++++++
> > 4 files changed, 377 insertions(+), 2 deletions(-)
>
> Patch #3 was memory, patch #4 was soc, patch #5 is memory, so that
> mixup pattern continues.
>
> Please address the earlier feedback.
Alright, I double check the docs and re-order this and my other series
for the next revisions. I had a misunderstanding how subsystems were
split up, which caused most but not all of the issues.
>
> >
> > diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
> > index fc5a277918267ee8240f9fb9efeb80275db4790b..2d0be29afe2b9ebf9a0630ef7fb6fb43ff359499 100644
> > --- a/drivers/memory/tegra/Kconfig
> > +++ b/drivers/memory/tegra/Kconfig
> > @@ -55,6 +55,7 @@ config TEGRA210_EMC
> > tristate "NVIDIA Tegra210 External Memory Controller driver"
> > depends on ARCH_TEGRA_210_SOC || COMPILE_TEST
> > select TEGRA210_EMC_TABLE
> > + select PM_OPP
> > help
> > This driver is for the External Memory Controller (EMC) found on
> > Tegra210 chips. The EMC controls the external DRAM on the board.
> > diff --git a/drivers/memory/tegra/tegra210-emc-core.c b/drivers/memory/tegra/tegra210-emc-core.c
> > index e96ca4157d48182574310f8caf72687bed7cc16a..f12e60b47fa87d629505cde57310d2bb68fc87f3 100644
> > --- a/drivers/memory/tegra/tegra210-emc-core.c
> > +++ b/drivers/memory/tegra/tegra210-emc-core.c
> > @@ -13,6 +13,7 @@
> > #include <linux/module.h>
> > #include <linux/of_reserved_mem.h>
> > #include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > #include <linux/slab.h>
> > #include <linux/thermal.h>
> > #include <soc/tegra/fuse.h>
> > @@ -1569,6 +1570,79 @@ static int tegra210_emc_set_rate(struct device *dev,
> > return 0;
> > }
> >
>
> ...
>
> > @@ -1758,6 +1832,193 @@ static void tegra210_emc_debugfs_init(struct tegra210_emc *emc)
> > &tegra210_emc_debug_temperature_fops);
> > }
> >
> > +static inline struct tegra210_emc *
> > +to_tegra210_emc_provider(struct icc_provider *provider)
> > +{
> > + return container_of(provider, struct tegra210_emc, icc_provider);
> > +}
> > +
> > +static struct icc_node_data *
> > +emc_of_icc_xlate_extended(const struct of_phandle_args *spec, void *data)
> > +{
> > + struct icc_provider *provider = data;
> > + struct icc_node_data *ndata;
> > + 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;
> > +
> > + ndata = kzalloc(sizeof(*ndata), GFP_KERNEL);
> > + if (!ndata)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + /*
> > + * SRC and DST nodes should have matching TAG in order to have
> > + * it set by default for a requested path.
> > + */
> > + ndata->tag = TEGRA_MC_ICC_TAG_ISO;
> > + ndata->node = node;
> > +
> > + return ndata;
> > + }
> > +
> > + return ERR_PTR(-EPROBE_DEFER);
> > +}
> > +
> > +static int emc_icc_set(struct icc_node *src, struct icc_node *dst)
> > +{
> > + struct tegra210_emc *emc = to_tegra210_emc_provider(dst->provider);
> > + unsigned long long peak_bw = icc_units_to_bps(dst->peak_bw);
> > + unsigned long long avg_bw = icc_units_to_bps(dst->avg_bw);
> > + unsigned long long rate = max(avg_bw, peak_bw);
> > + const unsigned int ddr = 2;
>
> Just use defines in top part for this.
>
> > + int err;
> > +
> > + /*
> > + * Tegra210 memory layout can be 1 channel at 64-bit or 2 channels
> > + * at 32-bit each. Either way, the total bus width will always be
> > + * 64-bit.
> > + */
> > + const unsigned int dram_data_bus_width_bytes = 64 / 8;
>
> Same here.
>
> > +
> > + /*
> > + * Tegra210 EMC runs on a clock rate of SDRAM bus. This means that
> > + * EMC clock rate is twice smaller than the peak data rate because
> > + * data is sampled on both EMC clock edges.
> > + */
> > + do_div(rate, ddr * dram_data_bus_width_bytes);
> > + rate = min_t(u64, rate, U32_MAX);
> > +
> > + err = emc_set_min_rate(emc, rate, EMC_RATE_ICC);
> > + if (err)
> > + return err;
> > +
> > + return 0;
> > +}
> > +
> > +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 tegra210_emc *emc)
> > +{
> > + const struct tegra_mc_soc *soc = emc->mc->soc;
> > + struct icc_node *node;
> > + int err;
> > +
> > + emc->icc_provider.dev = emc->dev;
> > + emc->icc_provider.set = emc_icc_set;
> > + emc->icc_provider.data = &emc->icc_provider;
> > + emc->icc_provider.aggregate = soc->icc_ops->aggregate;
> > + emc->icc_provider.xlate_extended = emc_of_icc_xlate_extended;
> > + emc->icc_provider.get_bw = tegra_emc_icc_get_init_bw;
> > +
> > + icc_provider_init(&emc->icc_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->icc_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->icc_provider);
> > +
> > + err = icc_provider_register(&emc->icc_provider);
> > + if (err)
> > + goto remove_nodes;
> > +
> > + return 0;
> > +
> > +remove_nodes:
> > + icc_nodes_remove(&emc->icc_provider);
> > +err_msg:
> > + dev_err(emc->dev, "failed to initialize ICC: %d\n", err);
> > +
> > + return err;
> > +}
> > +
> > +static int tegra_emc_opp_table_init(struct tegra210_emc *emc)
> > +{
> > + u32 hw_version = BIT(tegra_sku_info.soc_speedo_id);
> > + struct dev_pm_opp *opp;
> > + unsigned long rate;
> > + int opp_token, err, max_opps, i;
> > +
> > + err = dev_pm_opp_set_supported_hw(emc->dev, &hw_version, 1);
> > + if (err < 0) {
> > + dev_err(emc->dev, "failed to set OPP supported HW: %d\n", err);
> > + return err;
> > + }
> > + opp_token = err;
> > +
> > + err = dev_pm_opp_of_add_table(emc->dev);
> > + if (err) {
> > + if (err == -ENODEV)
> > + dev_err(emc->dev, "OPP table not found, please update your device tree\n");
>
> So this looks like the actual ABI break.
Okay, so let's discuss this. For reference, I based this patch off the
tegra124 change [0], which also caused an abi break. I know past
changes don't justify current mistakes, but this is the context. This
series adds all new required dt properties to the arch common dtsi, so
any newly compiled dtb will work. Any old dtb with a new kernel would
fail to probe, however. I think it would be safe to just skip the
interconnect init if the opp table init returns ENODEV, then let probe
succeed, but I would have to verify that. Do I need to do that and
drop the new requires from the binding?
Aaron
[0] https://github.com/torvalds/linux/commit/380def2d4cf257663de42618e57134afeded32dd#diff-3ff603a1ea7654928390eb213cea0424b6a12251bccbb5fd3b9720402a3c076aR1435
Powered by blists - more mailing lists