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

[Krzysztof Kozlowski]

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?

> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/interconnect-provider.h>

Where is it used?

> +#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.

> +#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?

> +
> +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?

> +		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