Re: [PATCH v1 5/5] memory: tegra: Add Tegra114 EMC driver

[Svyatoslav Ryhel]

вт, 17 лют. 2026 р. о 09:21 Krzysztof Kozlowski <krzk@xxxxxxxxxx> пише:
>
> On 26/01/2026 20:07, Svyatoslav Ryhel wrote:
> > Introduce driver for the External Memory Controller (EMC) found in
> > Tegra114 SoC. It controls the external DRAM on the board. The purpose of
> > this driver is to program memory timing for external memory on the EMC
> > clock rate change.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > Reviewed-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > ---
> >  drivers/memory/tegra/Kconfig        |   12 +
> >  drivers/memory/tegra/Makefile       |    1 +
> >  drivers/memory/tegra/tegra114-emc.c | 1463 +++++++++++++++++++++++++++
>
> Please rebase on top of Mikko's patch removing duplicated code.
>
> >  3 files changed, 1476 insertions(+)
> >  create mode 100644 drivers/memory/tegra/tegra114-emc.c
> >
> > diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
> > index fc5a27791826..11e7cc357d39 100644
> > --- a/drivers/memory/tegra/Kconfig
> > +++ b/drivers/memory/tegra/Kconfig
> > @@ -35,6 +35,18 @@ config TEGRA30_EMC
> >         This driver is required to change memory timings / clock rate for
> >         external memory.
> >
> > +config TEGRA114_EMC
> > +     tristate "NVIDIA Tegra114 External Memory Controller driver"
> > +     default y
> > +     depends on ARCH_TEGRA_114_SOC || COMPILE_TEST
> > +     select TEGRA124_CLK_EMC if ARCH_TEGRA
> > +     select PM_OPP
> > +     help
> > +       This driver is for the External Memory Controller (EMC) found on
> > +       Tegra114 chips. The EMC controls the external DRAM on the board.
> > +       This driver is required to change memory timings / clock rate for
> > +       external memory.
> > +
> >  config TEGRA124_EMC
> >       tristate "NVIDIA Tegra124 External Memory Controller driver"
> >       default ARCH_TEGRA_124_SOC
> > diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
> > index 6334601e6120..6b9156de4b66 100644
> > --- a/drivers/memory/tegra/Makefile
> > +++ b/drivers/memory/tegra/Makefile
> > @@ -16,6 +16,7 @@ obj-$(CONFIG_TEGRA_MC) += tegra-mc.o
> >
> >  obj-$(CONFIG_TEGRA20_EMC)  += tegra20-emc.o
> >  obj-$(CONFIG_TEGRA30_EMC)  += tegra30-emc.o
> > +obj-$(CONFIG_TEGRA114_EMC) += tegra114-emc.o
> >  obj-$(CONFIG_TEGRA124_EMC) += tegra124-emc.o
> >  obj-$(CONFIG_TEGRA210_EMC_TABLE) += tegra210-emc-table.o
> >  obj-$(CONFIG_TEGRA210_EMC) += tegra210-emc.o
> > diff --git a/drivers/memory/tegra/tegra114-emc.c b/drivers/memory/tegra/tegra114-emc.c
> > new file mode 100644
> > index 000000000000..789b8e959a68
> > --- /dev/null
> > +++ b/drivers/memory/tegra/tegra114-emc.c
> > @@ -0,0 +1,1463 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Tegra114 External Memory Controller driver
> > + *
> > + * Based on downstream driver from NVIDIA and tegra124-emc.c
> > + * Copyright (C) 2011-2014 NVIDIA Corporation
> > + *
> > + * Copyright (C) 2024 Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > + */
> > +
> > +#include <linux/clk-provider.h>
>
> Where is it used?
>
> > +#include <linux/clk.h>
> > +#include <linux/clkdev.h>
>
> Where is it used?
>
> > +#include <linux/clk/tegra.h>
>
> Where is it used?
>

All 4 by tegra124_clk_set_emc_callbacks

> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/interconnect-provider.h>
>
> Where is it used?
>

tegra_emc_interconnect_init

> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
>
> I don't see these used. OTOH, I see you use other of_ which needs their
> header.
>

fair

> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/sort.h>
> > +#include <linux/string.h>
> > +
> > +#include <soc/tegra/common.h>
> > +#include <soc/tegra/fuse.h>
> > +#include <soc/tegra/mc.h>
> > +
> > +#include "mc.h"
> > +
> > +#define EMC_INTSTATUS                                0x0
> > +#define EMC_INTSTATUS_REFRESH_OVERFLOW               BIT(3)
> > +#define EMC_INTSTATUS_CLKCHANGE_COMPLETE     BIT(4)
> > +
> > +#define EMC_INTMASK                          0x4
> > +
> > +#define EMC_DBG                                      0x8
> > +#define EMC_DBG_READ_MUX_ASSEMBLY            BIT(0)
> > +#define EMC_DBG_WRITE_MUX_ACTIVE             BIT(1)
> > +#define EMC_DBG_FORCE_UPDATE                 BIT(2)
> > +#define EMC_DBG_CFG_PRIORITY                 BIT(24)
> > +
>
> ...
>
> > +
> > +static int tegra_emc_debug_available_rates_show(struct seq_file *s,
> > +                                             void *data)
> > +{
> > +     struct tegra_emc *emc = s->private;
> > +     const char *prefix = "";
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < emc->num_timings; i++) {
> > +             seq_printf(s, "%s%lu", prefix, emc->timings[i].rate);
> > +             prefix = " ";
> > +     }
> > +
> > +     seq_puts(s, "\n");
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(tegra_emc_debug_available_rates);
>
> Where is the ABI documented for all these?
>

Debugfs does not require ABI documentation, provided comment in the
driver is sufficient.

> > +
> > +static int tegra_emc_debug_min_rate_get(void *data, u64 *rate)
> > +{
> > +     struct tegra_emc *emc = data;
> > +
> > +     *rate = emc->debugfs.min_rate;
> > +
> > +     return 0;
> > +}
> > +
> > +static int tegra_emc_debug_min_rate_set(void *data, u64 rate)
> > +{
> > +     struct tegra_emc *emc = data;
> > +     int err;
> > +
> > +     if (!tegra_emc_validate_rate(emc, rate))
> > +             return -EINVAL;
> > +
> > +     err = emc_set_min_rate(emc, rate, EMC_RATE_DEBUG);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     emc->debugfs.min_rate = rate;
> > +
> > +     return 0;
> > +}
> > +
> > +DEFINE_DEBUGFS_ATTRIBUTE(tegra_emc_debug_min_rate_fops,
> > +                      tegra_emc_debug_min_rate_get,
> > +                      tegra_emc_debug_min_rate_set, "%llu\n");
> > +
>
> ...
>
> > +
> > +static int tegra_emc_probe(struct platform_device *pdev)
> > +{
> > +     struct tegra_core_opp_params opp_params = {};
> > +     struct device_node *np;
> > +     struct tegra_emc *emc;
> > +     u32 ram_code;
> > +     int err;
> > +
> > +     emc = devm_kzalloc(&pdev->dev, sizeof(*emc), GFP_KERNEL);
> > +     if (!emc)
> > +             return -ENOMEM;
> > +
> > +     mutex_init(&emc->rate_lock);
> > +     emc->dev = &pdev->dev;
> > +
> > +     emc->regs = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(emc->regs))
> > +             return PTR_ERR(emc->regs);
> > +
> > +     emc->mc = devm_tegra_memory_controller_get(&pdev->dev);
> > +     if (IS_ERR(emc->mc))
> > +             return PTR_ERR(emc->mc);
> > +
> > +     ram_code = tegra_read_ram_code();
> > +
> > +     np = tegra_emc_find_node_by_ram_code(pdev->dev.of_node, ram_code);
> > +     if (np) {
> > +             err = tegra_emc_load_timings_from_dt(emc, np);
> > +             of_node_put(np);
> > +             if (err)
> > +                     return err;
> > +     } else {
> > +             dev_info_once(&pdev->dev,
> > +                           "no memory timings for RAM code %u found in DT\n",
> > +                           ram_code);
> > +     }
> > +
> > +     err = emc_init(emc);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "EMC initialization failed: %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, emc);
> > +
> > +     tegra124_clk_set_emc_callbacks(tegra_emc_prepare_timing_change,
> > +                                    tegra_emc_complete_timing_change);
> > +
> > +     err = devm_add_action_or_reset(&pdev->dev, devm_tegra_emc_unset_callback,
> > +                                    NULL);
> > +     if (err)
> > +             return err;
> > +
> > +     err = platform_get_irq(pdev, 0);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     emc->irq = err;
> > +
> > +     err = devm_request_irq(&pdev->dev, emc->irq, tegra_emc_isr, 0,
> > +                            dev_name(&pdev->dev), emc);
> > +     if (err) {
> > +             dev_err(&pdev->dev, "failed to request irq: %d\n", err);
> > +             return err;
> > +     }
> > +
> > +     emc->clk = devm_clk_get(&pdev->dev, "emc");
> > +     if (IS_ERR(emc->clk)) {
> > +             err = PTR_ERR(emc->clk);
> > +             dev_err(&pdev->dev, "failed to get EMC clock: %d\n", err);
>
> Syntax is return dev_err_probe. Since some years and all the existing
> code was already fixed, no?
>

fair


> > +             return err;
> > +     }
> > +
> > +     opp_params.init_state = true;
> > +
> > +     err = devm_tegra_core_dev_init_opp_table(&pdev->dev, &opp_params);
> > +     if (err)
> > +             return err;
> > +
> > +     tegra_emc_rate_requests_init(emc);
> > +
> > +     if (IS_ENABLED(CONFIG_DEBUG_FS))
> > +             emc_debugfs_init(&pdev->dev, emc);
> > +
> > +     tegra_emc_interconnect_init(emc);
> > +
> > +     /*
> > +      * Don't allow the kernel module to be unloaded. Unloading adds some
> > +      * extra complexity which doesn't really worth the effort in a case of
> > +      * this driver.
> > +      */
> > +     try_module_get(THIS_MODULE);
> > +
> > +     return 0;
> > +};
> > +
> > +static const struct of_device_id tegra_emc_of_match[] = {
> > +     { .compatible = "nvidia,tegra114-emc" },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, tegra_emc_of_match);
> > +
> > +static struct platform_driver tegra_emc_driver = {
> > +     .probe = tegra_emc_probe,
> > +     .driver = {
> > +             .name = "tegra114-emc",
> > +             .of_match_table = tegra_emc_of_match,
> > +             .suppress_bind_attrs = true,
> > +             .sync_state = icc_sync_state,
> > +     },
> > +};
> > +module_platform_driver(tegra_emc_driver);
> > +
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("NVIDIA Tegra114 EMC driver");
> > +MODULE_LICENSE("GPL");
>
>
> Best regards,
> Krzysztof