Re: [PATCH v4 07/10] memory: tegra: Introduce Tegra30 EMC driver

From: Dmitry Osipenko
Date: Mon Jun 17 2019 - 11:08:02 EST

17.06.2019 12:50, Thierry Reding ÐÐÑÐÑ:
> On Mon, Jun 17, 2019 at 02:35:48AM +0300, Dmitry Osipenko wrote:
>> Introduce driver for the External Memory Controller (EMC) found on Tegra30
>> chips, 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: Dmitry Osipenko <digetx@xxxxxxxxx>
>> ---
>> drivers/memory/tegra/Kconfig | 10 +
>> drivers/memory/tegra/Makefile | 1 +
>> drivers/memory/tegra/mc.c | 9 +-
>> drivers/memory/tegra/mc.h | 30 +-
>> drivers/memory/tegra/tegra30-emc.c | 1197 ++++++++++++++++++++++++++++
>> drivers/memory/tegra/tegra30.c | 44 +
>> include/soc/tegra/mc.h | 2 +-
>> 7 files changed, 1278 insertions(+), 15 deletions(-)
>> create mode 100644 drivers/memory/tegra/tegra30-emc.c
>>
>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
>> index 4680124ddcab..fbfbaada61a2 100644
>> --- a/drivers/memory/tegra/Kconfig
>> +++ b/drivers/memory/tegra/Kconfig
>> @@ -17,6 +17,16 @@ config TEGRA20_EMC
>> This driver is required to change memory timings / clock rate for
>> external memory.
>>
>> +config TEGRA30_EMC
>> + bool "NVIDIA Tegra30 External Memory Controller driver"
>> + default y
>> + depends on TEGRA_MC && ARCH_TEGRA_3x_SOC
>> + help
>> + This driver is for the External Memory Controller (EMC) found on
>> + Tegra30 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
>> bool "NVIDIA Tegra124 External Memory Controller driver"
>> default y
>> diff --git a/drivers/memory/tegra/Makefile b/drivers/memory/tegra/Makefile
>> index 3971a6b7c487..3d23c4261104 100644
>> --- a/drivers/memory/tegra/Makefile
>> +++ b/drivers/memory/tegra/Makefile
>> @@ -11,5 +11,6 @@ tegra-mc-$(CONFIG_ARCH_TEGRA_210_SOC) += tegra210.o
>> obj-$(CONFIG_TEGRA_MC) += tegra-mc.o
>>
>> obj-$(CONFIG_TEGRA20_EMC) += tegra20-emc.o
>> +obj-$(CONFIG_TEGRA30_EMC) += tegra30-emc.o
>> obj-$(CONFIG_TEGRA124_EMC) += tegra124-emc.o
>> obj-$(CONFIG_ARCH_TEGRA_186_SOC) += tegra186.o
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index 163b6c69e651..eaebe371625c 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -51,9 +51,6 @@
>> #define MC_EMEM_ADR_CFG 0x54
>> #define MC_EMEM_ADR_CFG_EMEM_NUMDEV BIT(0)
>>
>> -#define MC_TIMING_CONTROL 0xfc
>> -#define MC_TIMING_UPDATE BIT(0)
>> -
>> static const struct of_device_id tegra_mc_of_match[] = {
>> #ifdef CONFIG_ARCH_TEGRA_2x_SOC
>> { .compatible = "nvidia,tegra20-mc-gart", .data = &tegra20_mc_soc },
>> @@ -310,7 +307,7 @@ static int tegra_mc_setup_latency_allowance(struct tegra_mc *mc)
>> return 0;
>> }
>>
>> -void tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate)
>> +int tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate)
>> {
>> unsigned int i;
>> struct tegra_mc_timing *timing = NULL;
>> @@ -325,11 +322,13 @@ void tegra_mc_write_emem_configuration(struct tegra_mc *mc, unsigned long rate)
>> if (!timing) {
>> dev_err(mc->dev, "no memory timing registered for rate %lu\n",
>> rate);
>> - return;
>> + return -EINVAL;
>> }
>>
>> for (i = 0; i < mc->soc->num_emem_regs; ++i)
>> mc_writel(mc, timing->emem_data[i], mc->soc->emem_regs[i]);
>> +
>> + return 0;
>> }
>>
>> unsigned int tegra_mc_get_emem_device_count(struct tegra_mc *mc)
>> diff --git a/drivers/memory/tegra/mc.h b/drivers/memory/tegra/mc.h
>> index 392993955c93..0720a1d2023e 100644
>> --- a/drivers/memory/tegra/mc.h
>> +++ b/drivers/memory/tegra/mc.h
>> @@ -9,20 +9,32 @@
>> #ifndef MEMORY_TEGRA_MC_H
>> #define MEMORY_TEGRA_MC_H
>>
>> +#include <linux/bits.h>
>> #include <linux/io.h>
>> #include <linux/types.h>
>>
>> #include <soc/tegra/mc.h>
>>
>> -#define MC_INT_DECERR_MTS (1 << 16)
>> -#define MC_INT_SECERR_SEC (1 << 13)
>> -#define MC_INT_DECERR_VPR (1 << 12)
>> -#define MC_INT_INVALID_APB_ASID_UPDATE (1 << 11)
>> -#define MC_INT_INVALID_SMMU_PAGE (1 << 10)
>> -#define MC_INT_ARBITRATION_EMEM (1 << 9)
>> -#define MC_INT_SECURITY_VIOLATION (1 << 8)
>> -#define MC_INT_INVALID_GART_PAGE (1 << 7)
>> -#define MC_INT_DECERR_EMEM (1 << 6)
>> +#define MC_INT_DECERR_MTS BIT(16)
>> +#define MC_INT_SECERR_SEC BIT(13)
>> +#define MC_INT_DECERR_VPR BIT(12)
>> +#define MC_INT_INVALID_APB_ASID_UPDATE BIT(11)
>> +#define MC_INT_INVALID_SMMU_PAGE BIT(10)
>> +#define MC_INT_ARBITRATION_EMEM BIT(9)
>> +#define MC_INT_SECURITY_VIOLATION BIT(8)
>> +#define MC_INT_INVALID_GART_PAGE BIT(7)
>> +#define MC_INT_DECERR_EMEM BIT(6)
>
> This /could/ be a separate patch, with it being unrelated to the EMC
> support, but probably not worth it.

I had the same feeling about this change and decided that it's not really worth it.

>> +#define MC_EMEM_ARB_OUTSTANDING_REQ 0x94
>> +#define MC_EMEM_ARB_OUTSTANDING_REQ_MAX_MASK 0x1ff
>> +#define MC_EMEM_ARB_OUTSTANDING_REQ_HOLDOFF_OVERRIDE BIT(30)
>> +#define MC_EMEM_ARB_OUTSTANDING_REQ_LIMIT_ENABLE BIT(31)
>> +
>> +#define MC_EMEM_ARB_OVERRIDE 0xe8
>> +#define MC_EMEM_ARB_OVERRIDE_EACK_MASK 0x3
>> +
>> +#define MC_TIMING_CONTROL 0xfc
>> +#define MC_TIMING_UPDATE BIT(0)
>>
>> static inline u32 mc_readl(struct tegra_mc *mc, unsigned long offset)
>> {
>> diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
>> new file mode 100644
>> index 000000000000..4700f7c8022e
>> --- /dev/null
>> +++ b/drivers/memory/tegra/tegra30-emc.c
>> @@ -0,0 +1,1197 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Tegra30 External Memory Controller driver
>> + *
>> + * Author: Dmitry Osipenko <digetx@xxxxxxxxx>
>> + */
>
> Copyright?
>
> Otherwise looks good to me.

Okay!