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

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1d36d1a4-9d1e-576e-336a-1ea990f92725@gmail.com>
Date:   Mon, 17 Jun 2019 18:03:02 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Joseph Lo <josephl@...dia.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Peter De Schrijver <pdeschrijver@...dia.com>,
        Prashant Gaikwad <pgaikwad@...dia.com>,
        Stephen Boyd <sboyd@...nel.org>, devicetree@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 07/10] memory: tegra: Introduce Tegra30 EMC driver

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@...il.com>
>> ---
>>  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@...il.com>
>> + */
> 
> Copyright?
> 
> Otherwise looks good to me.

Okay!