| 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: <84487ed8-2f9d-c178-012b-8407e5083b87@canonical.com>
Date: Tue, 21 Dec 2021 09:14:56 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To: Dmitry Osipenko <digetx@...il.com>,
Thierry Reding <thierry.reding@...il.com>,
Jonathan Hunter <jonathanh@...dia.com>,
Svyatoslav Ryhel <clamor95@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH v1] memory: tegra30-emc: Print additional memory info
On 20/12/2021 14:44, Dmitry Osipenko wrote:
> 20.12.2021 14:03, Krzysztof Kozlowski пишет:
>> On 18/12/2021 00:47, Dmitry Osipenko wrote:
>>> Print out memory type and LPDDR2 configuration on Tegra30, making it
>>> similar to the memory info printed by the Tegra20 memory driver. This
>>> info is useful for debugging purposes.
>>>
>>> Tested-by: Svyatoslav Ryhel <clamor95@...il.com> # T30 ASUS TF201 LPDDR2
>>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>>> ---
>>> drivers/memory/tegra/Kconfig | 1 +
>>> drivers/memory/tegra/tegra30-emc.c | 131 ++++++++++++++++++++++++++---
>>> 2 files changed, 122 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
>>> index 7951764b4efe..3fe83d7c2bf8 100644
>>> --- a/drivers/memory/tegra/Kconfig
>>> +++ b/drivers/memory/tegra/Kconfig
>>> @@ -28,6 +28,7 @@ config TEGRA30_EMC
>>> default y
>>> depends on ARCH_TEGRA_3x_SOC || COMPILE_TEST
>>> select PM_OPP
>>> + select DDR
>>> help
>>> This driver is for the External Memory Controller (EMC) found on
>>> Tegra30 chips. The EMC controls the external DRAM on the board.
>>> diff --git a/drivers/memory/tegra/tegra30-emc.c b/drivers/memory/tegra/tegra30-emc.c
>>> index 80f98d717e13..4c0432704f46 100644
>>> --- a/drivers/memory/tegra/tegra30-emc.c
>>> +++ b/drivers/memory/tegra/tegra30-emc.c
>>> @@ -9,6 +9,7 @@
>>> * Copyright (C) 2019 GRATE-DRIVER project
>>> */
>>>
>>> +#include <linux/bitfield.h>
>>> #include <linux/clk.h>
>>> #include <linux/clk/tegra.h>
>>> #include <linux/debugfs.h>
>>> @@ -31,11 +32,15 @@
>>> #include <soc/tegra/common.h>
>>> #include <soc/tegra/fuse.h>
>>>
>>> +#include "../jedec_ddr.h"
>>> +#include "../of_memory.h"
>>> +
>>> #include "mc.h"
>>>
>>> #define EMC_INTSTATUS 0x000
>>> #define EMC_INTMASK 0x004
>>> #define EMC_DBG 0x008
>>> +#define EMC_ADR_CFG 0x010
>>> #define EMC_CFG 0x00c
>>> #define EMC_REFCTRL 0x020
>>> #define EMC_TIMING_CONTROL 0x028
>>> @@ -81,6 +86,7 @@
>>> #define EMC_EMRS 0x0d0
>>> #define EMC_SELF_REF 0x0e0
>>> #define EMC_MRW 0x0e8
>>> +#define EMC_MRR 0x0ec
>>> #define EMC_XM2DQSPADCTRL3 0x0f8
>>> #define EMC_FBIO_SPARE 0x100
>>> #define EMC_FBIO_CFG5 0x104
>>> @@ -208,6 +214,13 @@
>>>
>>> #define EMC_REFRESH_OVERFLOW_INT BIT(3)
>>> #define EMC_CLKCHANGE_COMPLETE_INT BIT(4)
>>> +#define EMC_MRR_DIVLD_INT BIT(5)
>>> +
>>> +#define EMC_MRR_DEV_SELECTN GENMASK(31, 30)
>>> +#define EMC_MRR_MRR_MA GENMASK(23, 16)
>>> +#define EMC_MRR_MRR_DATA GENMASK(15, 0)
>>> +
>>> +#define EMC_ADR_CFG_EMEM_NUMDEV BIT(0)
>>>
>>> enum emc_dram_type {
>>> DRAM_TYPE_DDR3,
>>> @@ -378,6 +391,8 @@ struct tegra_emc {
>>>
>>> /* protect shared rate-change code path */
>>> struct mutex rate_lock;
>>> +
>>> + bool mrr_error;
>>> };
>>>
>>> static int emc_seq_update_timing(struct tegra_emc *emc)
>>> @@ -1008,12 +1023,18 @@ static int emc_load_timings_from_dt(struct tegra_emc *emc,
>>> return 0;
>>> }
>>>
>>> -static struct device_node *emc_find_node_by_ram_code(struct device *dev)
>>> +static struct device_node *emc_find_node_by_ram_code(struct tegra_emc *emc)
>>> {
>>> + struct device *dev = emc->dev;
>>> struct device_node *np;
>>> u32 value, ram_code;
>>> int err;
>>>
>>> + if (emc->mrr_error) {
>>> + dev_warn(dev, "memory timings skipped due to MRR error\n");
>>> + return NULL;
>>> + }
>>> +
>>> if (of_get_child_count(dev->of_node) == 0) {
>>> dev_info_once(dev, "device-tree doesn't have memory timings\n");
>>> return NULL;
>>> @@ -1035,11 +1056,73 @@ static struct device_node *emc_find_node_by_ram_code(struct device *dev)
>>> return NULL;
>>> }
>>>
>>> +static int emc_read_lpddr_mode_register(struct tegra_emc *emc,
>>> + unsigned int emem_dev,
>>> + unsigned int register_addr,
>>> + unsigned int *register_data)
>>> +{
>>> + u32 memory_dev = emem_dev + 1;
>>> + u32 val, mr_mask = 0xff;
>>> + int err;
>>> +
>>> + /* clear data-valid interrupt status */
>>> + writel_relaxed(EMC_MRR_DIVLD_INT, emc->regs + EMC_INTSTATUS);
>>> +
>>> + /* issue mode register read request */
>>> + val = FIELD_PREP(EMC_MRR_DEV_SELECTN, memory_dev);
>>> + val |= FIELD_PREP(EMC_MRR_MRR_MA, register_addr);
>>> +
>>> + writel_relaxed(val, emc->regs + EMC_MRR);
>>> +
>>> + /* wait for the LPDDR2 data-valid interrupt */
>>> + err = readl_relaxed_poll_timeout_atomic(emc->regs + EMC_INTSTATUS, val,
>>> + val & EMC_MRR_DIVLD_INT,
>>> + 1, 100);
>>> + if (err) {
>>> + dev_err(emc->dev, "mode register %u read failed: %d\n",
>>> + register_addr, err);
>>> + emc->mrr_error = true;
>>> + return err;
>>> + }
>>> +
>>> + /* read out mode register data */
>>> + val = readl_relaxed(emc->regs + EMC_MRR);
>>> + *register_data = FIELD_GET(EMC_MRR_MRR_DATA, val) & mr_mask;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void emc_read_lpddr_sdram_info(struct tegra_emc *emc,
>>> + unsigned int emem_dev)
>>> +{
>>> + union lpddr2_basic_config4 basic_conf4;
>>> + unsigned int manufacturer_id;
>>> + unsigned int revision_id1;
>>> + unsigned int revision_id2;
>>> +
>>> + /* these registers are standard for all LPDDR JEDEC memory chips */
>>> + emc_read_lpddr_mode_register(emc, emem_dev, 5, &manufacturer_id);
>>> + emc_read_lpddr_mode_register(emc, emem_dev, 6, &revision_id1);
>>> + emc_read_lpddr_mode_register(emc, emem_dev, 7, &revision_id2);
>>> + emc_read_lpddr_mode_register(emc, emem_dev, 8, &basic_conf4.value);
>>> +
>>> + dev_info(emc->dev, "SDRAM[dev%u]: manufacturer: 0x%x (%s) rev1: 0x%x rev2: 0x%x prefetch: S%u density: %uMbit iowidth: %ubit\n",
>>> + emem_dev, manufacturer_id,
>>> + lpddr2_jedec_manufacturer(manufacturer_id),
>>> + revision_id1, revision_id2,
>>> + 4 >> basic_conf4.arch_type,
>>> + 64 << basic_conf4.density,
>>> + 32 >> basic_conf4.io_width);
>>> +}
>>> +
>>
>> Quickly looking, these two functions are exactly the same as ones in
>> tegra20-emc.c
>> . Later you might come up with another set for other SoCs, so it looks
>> it is worth to share these.
>
> Should be too much trouble for not much gain, IMO. How many bytes will
> be shared in the end? There is no much code here, we may lose more than
> win. All these Tegra EMC drivers can be compiled as a loadable modules,
> that's what distro kernels usually do. There are no plans for other SoCs
> for today.
It's not about the bytes but source code lines to maintain and fix (if
there is something to fix). But if you don't plan to make a third copy
of it, it is okay.
>
> I don't see how that sharing could be done easily and nicely. Please
> tell if you see.
Since it is not about duplicated object code, but code for review, it is
pretty straightforward:
1. Create tegra-emc-common.[ch]
2. In Makefile:
+tegra20_emc-y += tegra20-emc.o tegra-emc-common.o
+obj-$(CONFIG_TEGRA20_EMC) += tegra20_emc.o
+
+tegra30_emc-y += tegra30-emc.o tegra-emc-common.o
+obj-$(CONFIG_TEGRA30_EMC) += tegra30_emc.o
Best regards,
Krzysztof
Powered by blists - more mailing lists