mbox series

[v3,0/4] tegra20-emc: Identify memory chip by LPDDR configuration

Message ID 20211003013235.2357-1-digetx@gmail.com
Headers show
Series tegra20-emc: Identify memory chip by LPDDR configuration | expand

Message

Dmitry Osipenko Oct. 3, 2021, 1:32 a.m. UTC
Support memory chip identification by LPDDR2 configuration, which is
needed by ASUS Transformer TF101 tablet device that doesn't store RAMCODE
in Tegra's NVMEM.

Changelog:

v3: - Corrected sub-node name in tegra20-emc.yaml.

v2: - Added separate binding for standard LPDDR2 properties, like it
      was suggested by Krzysztof Kozlowski.

    - Switched Tegra binding to use new lpddr2-configuration sub-node
      that contains the standard properties.

    - Extended commit message of the "emc: Document new LPDDR2 sub-node"
      patch, telling how the properties are supposed to be used, which
      was requested by Krzysztof Kozlowski.

    - Added new common helpers for parsing LPDDR2 properties and made
      tegra20-emc driver to use these helpers.

Dmitry Osipenko (4):
  dt-bindings: memory: Add LPDDR2 binding
  dt-bindings: memory: tegra20: emc: Document new LPDDR2 sub-node
  memory: Add LPDDR2 configuration helpers
  memory: tegra20-emc: Support matching timings by LPDDR2 configuration

 .../memory-controllers/jedec,lpddr2.yaml      |  80 ++++++++
 .../nvidia,tegra20-emc.yaml                   |  17 +-
 drivers/memory/jedec_ddr.h                    |  21 ++
 drivers/memory/jedec_ddr_data.c               |  42 ++++
 drivers/memory/of_memory.c                    |  34 ++++
 drivers/memory/of_memory.h                    |   9 +
 drivers/memory/tegra/Kconfig                  |   1 +
 drivers/memory/tegra/tegra20-emc.c            | 191 ++++++++++++++++--
 include/dt-bindings/memory/lpddr2.h           |  25 +++
 9 files changed, 404 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/jedec,lpddr2.yaml
 create mode 100644 include/dt-bindings/memory/lpddr2.h

Comments

Krzysztof Kozlowski Oct. 4, 2021, 8:53 a.m. UTC | #1
On 03/10/2021 03:32, Dmitry Osipenko wrote:
> Add helpers for reading and parsing standard LPDDR2 properties.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/memory/jedec_ddr.h      | 21 +++++++++++++++++
>  drivers/memory/jedec_ddr_data.c | 42 +++++++++++++++++++++++++++++++++
>  drivers/memory/of_memory.c      | 34 ++++++++++++++++++++++++++
>  drivers/memory/of_memory.h      |  9 +++++++
>  4 files changed, 106 insertions(+)
> 
> diff --git a/drivers/memory/jedec_ddr.h b/drivers/memory/jedec_ddr.h
> index e59ccbd982d0..14cef272559e 100644
> --- a/drivers/memory/jedec_ddr.h
> +++ b/drivers/memory/jedec_ddr.h
> @@ -230,4 +230,25 @@ struct lpddr3_min_tck {
>  	u32 tMRD;
>  };
>  
> +union lpddr2_basic_config4 {
> +	u32 value;
> +
> +	struct {
> +		unsigned int arch_type : 2;
> +		unsigned int density : 4;
> +		unsigned int io_width : 2;
> +	} __packed;
> +};
> +
> +struct lpddr2_configuration {
> +	int arch_type;
> +	int density;
> +	int io_width;
> +	int manufacturer_id;
> +	int revision_id1;
> +	int revision_id2;
> +};
> +
> +const char *lpddr2_jedec_manufacturer(unsigned int manufacturer_id);
> +
>  #endif /* __JEDEC_DDR_H */
> diff --git a/drivers/memory/jedec_ddr_data.c b/drivers/memory/jedec_ddr_data.c
> index ed601d813175..1f214716ac45 100644
> --- a/drivers/memory/jedec_ddr_data.c
> +++ b/drivers/memory/jedec_ddr_data.c
> @@ -7,6 +7,7 @@
>   * Aneesh V <aneesh@ti.com>
>   */
>  
> +#include <dt-bindings/memory/lpddr2.h>
>  #include <linux/export.h>
>  
>  #include "jedec_ddr.h"
> @@ -131,3 +132,44 @@ const struct lpddr2_min_tck lpddr2_jedec_min_tck = {
>  	.tFAW		= 8
>  };
>  EXPORT_SYMBOL_GPL(lpddr2_jedec_min_tck);
> +
> +const char *lpddr2_jedec_manufacturer(unsigned int manufacturer_id)
> +{
> +	switch (manufacturer_id) {
> +	case LPDDR2_MANID_SAMSUNG:
> +		return "Samsung";
> +	case LPDDR2_MANID_QIMONDA:
> +		return "Qimonda";
> +	case LPDDR2_MANID_ELPIDA:
> +		return "Elpida";
> +	case LPDDR2_MANID_ETRON:
> +		return "Etron";
> +	case LPDDR2_MANID_NANYA:
> +		return "Nanya";
> +	case LPDDR2_MANID_HYNIX:
> +		return "Hynix";
> +	case LPDDR2_MANID_MOSEL:
> +		return "Mosel";
> +	case LPDDR2_MANID_WINBOND:
> +		return "Winbond";
> +	case LPDDR2_MANID_ESMT:
> +		return "ESMT";
> +	case LPDDR2_MANID_SPANSION:
> +		return "Spansion";
> +	case LPDDR2_MANID_SST:
> +		return "SST";
> +	case LPDDR2_MANID_ZMOS:
> +		return "ZMOS";
> +	case LPDDR2_MANID_INTEL:
> +		return "Intel";
> +	case LPDDR2_MANID_NUMONYX:
> +		return "Numonyx";
> +	case LPDDR2_MANID_MICRON:
> +		return "Micron";
> +	default:
> +		break;
> +	}
> +
> +	return "invalid";
> +}
> +EXPORT_SYMBOL_GPL(lpddr2_jedec_manufacturer);
> diff --git a/drivers/memory/of_memory.c b/drivers/memory/of_memory.c
> index d9f5437d3bce..8aa777f2a090 100644
> --- a/drivers/memory/of_memory.c
> +++ b/drivers/memory/of_memory.c
> @@ -298,3 +298,37 @@ const struct lpddr3_timings
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_lpddr3_get_ddr_timings);
> +
> +/**
> + * of_lpddr2_get_config() - extracts the lpddr2 chip configuration.
> + * @np: Pointer to device tree node containing configuration
> + * @conf: Configuration updated by this function
> + *
> + * Populates lpddr2_configuration structure by extracting data from device
> + * tree node. Returns 0 on success or error code on failure. If property
> + * is missing in device-tree, then the corresponding @conf value is set to
> + * -ENOENT.
> + */
> +int of_lpddr2_get_config(struct device_node *np,
> +			 struct lpddr2_configuration *conf)
> +{

Interface should be rather like of_get_ddr_timings() - allocate memory
for structure and return it. It's less error-prone.

> +	int err, ret = -ENOENT;
> +
> +#define OF_LPDDR2_READ_U32(prop, dtprop) \
> +	err = of_property_read_u32(np, dtprop, &conf->prop); \
> +	if (err) \
> +		conf->prop = -ENOENT; \
> +	else \
> +		ret = 0
> +
> +	/* at least one property should be parsed */
> +	OF_LPDDR2_READ_U32(manufacturer_id, "jedec,lpddr2-manufacturer-id");
> +	OF_LPDDR2_READ_U32(revision_id1, "jedec,lpddr2-revision-id1");
> +	OF_LPDDR2_READ_U32(revision_id2, "jedec,lpddr2-revision-id2");
> +	OF_LPDDR2_READ_U32(io_width, "jedec,lpddr2-io-width-bits");
> +	OF_LPDDR2_READ_U32(density, "jedec,lpddr2-density-mbits");
> +	OF_LPDDR2_READ_U32(arch_type, "jedec,lpddr2-type");

density and io-width are required properties in existing bindings, so
return code should not be overridden.

> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(of_lpddr2_get_config);
> diff --git a/drivers/memory/of_memory.h b/drivers/memory/of_memory.h
> index 4a99b232ab0a..95eccc251b04 100644
> --- a/drivers/memory/of_memory.h
> +++ b/drivers/memory/of_memory.h
> @@ -20,6 +20,9 @@ const struct lpddr3_min_tck *of_lpddr3_get_min_tck(struct device_node *np,
>  const struct lpddr3_timings *
>  of_lpddr3_get_ddr_timings(struct device_node *np_ddr,
>  			  struct device *dev, u32 device_type, u32 *nr_frequencies);
> +
> +int of_lpddr2_get_config(struct device_node *np,
> +			 struct lpddr2_configuration *conf);
>  #else
>  static inline const struct lpddr2_min_tck
>  	*of_get_min_tck(struct device_node *np, struct device *dev)
> @@ -46,6 +49,12 @@ static inline const struct lpddr3_timings
>  {
>  	return NULL;
>  }
> +
> +static int of_lpddr2_get_config(struct device_node *np,
> +				struct lpddr2_configuration *conf)
> +{
> +	return -ENOENT;
> +}
>  #endif /* CONFIG_OF && CONFIG_DDR */
>  
>  #endif /* __LINUX_MEMORY_OF_REG_ */
> 


Best regards,
Krzysztof
Krzysztof Kozlowski Oct. 4, 2021, 9:09 a.m. UTC | #2
On 03/10/2021 03:32, Dmitry Osipenko wrote:
> Asus Transformer TF101 doesn't provide RAM code and in this case memory
> timings should be selected based on identity information read out from
> SDRAM chip. Support matching timings by LPDDR2 configuration.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/memory/tegra/Kconfig       |   1 +
>  drivers/memory/tegra/tegra20-emc.c | 191 +++++++++++++++++++++++++++--
>  2 files changed, 179 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/memory/tegra/Kconfig b/drivers/memory/tegra/Kconfig
> index f9bae36c03a3..7951764b4efe 100644
> --- a/drivers/memory/tegra/Kconfig
> +++ b/drivers/memory/tegra/Kconfig
> @@ -16,6 +16,7 @@ config TEGRA20_EMC
>  	depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
>  	select DEVFREQ_GOV_SIMPLE_ONDEMAND
>  	select PM_DEVFREQ
> +	select DDR
>  	help
>  	  This driver is for the External Memory Controller (EMC) found on
>  	  Tegra20 chips. The EMC controls the external DRAM on the board.
> diff --git a/drivers/memory/tegra/tegra20-emc.c b/drivers/memory/tegra/tegra20-emc.c
> index c3462dbc8c22..8965cdff43b9 100644
> --- a/drivers/memory/tegra/tegra20-emc.c
> +++ b/drivers/memory/tegra/tegra20-emc.c
> @@ -5,6 +5,7 @@
>   * Author: Dmitry Osipenko <digetx@gmail.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/clk/tegra.h>
>  #include <linux/debugfs.h>
> @@ -27,11 +28,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_0				0x010
>  #define EMC_TIMING_CONTROL			0x028
>  #define EMC_RC					0x02c
>  #define EMC_RFC					0x030
> @@ -68,6 +73,7 @@
>  #define EMC_QUSE_EXTRA				0x0ac
>  #define EMC_ODT_WRITE				0x0b0
>  #define EMC_ODT_READ				0x0b4
> +#define EMC_MRR					0x0ec
>  #define EMC_FBIO_CFG5				0x104
>  #define EMC_FBIO_CFG6				0x114
>  #define EMC_STAT_CONTROL			0x160
> @@ -94,6 +100,7 @@
>  
>  #define EMC_REFRESH_OVERFLOW_INT		BIT(3)
>  #define EMC_CLKCHANGE_COMPLETE_INT		BIT(4)
> +#define EMC_MRR_DIVLD_INT			BIT(5)
>  
>  #define EMC_DBG_READ_MUX_ASSEMBLY		BIT(0)
>  #define EMC_DBG_WRITE_MUX_ACTIVE		BIT(1)
> @@ -102,11 +109,25 @@
>  #define EMC_DBG_CFG_PRIORITY			BIT(24)
>  
>  #define EMC_FBIO_CFG5_DRAM_WIDTH_X16		BIT(4)
> +#define EMC_FBIO_CFG5_DRAM_TYPE			GENMASK(1, 0)
> +
> +#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_0_EMEM_NUMDEV		GENMASK(25, 24)
>  
>  #define EMC_PWR_GATHER_CLEAR			(1 << 8)
>  #define EMC_PWR_GATHER_DISABLE			(2 << 8)
>  #define EMC_PWR_GATHER_ENABLE			(3 << 8)
>  
> +enum emc_dram_type {
> +	DRAM_TYPE_RESERVED,
> +	DRAM_TYPE_DDR1,
> +	DRAM_TYPE_LPDDR2,
> +	DRAM_TYPE_DDR2,
> +};
> +
>  static const u16 emc_timing_registers[] = {
>  	EMC_RC,
>  	EMC_RFC,
> @@ -201,6 +222,12 @@ struct tegra_emc {
>  	struct mutex rate_lock;
>  
>  	struct devfreq_simple_ondemand_data ondemand_data;
> +
> +	/* memory chip identity information */
> +	union lpddr2_basic_config4 basic_conf4;
> +	unsigned int manufacturer_id;
> +	unsigned int revision_id1;
> +	unsigned int revision_id2;
>  };
>  
>  static irqreturn_t tegra_emc_isr(int irq, void *data)
> @@ -401,6 +428,9 @@ static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
>  	timing = emc->timings;
>  
>  	for_each_child_of_node(node, child) {
> +		if (of_node_name_eq(child, "lpddr2-configuration"))
> +			continue;
> +
>  		err = load_one_timing_from_dt(emc, timing++, child);
>  		if (err) {
>  			of_node_put(child);
> @@ -422,8 +452,9 @@ static int tegra_emc_load_timings_from_dt(struct tegra_emc *emc,
>  }
>  
>  static struct device_node *
> -tegra_emc_find_node_by_ram_code(struct device *dev)
> +tegra_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;
> @@ -442,8 +473,53 @@ tegra_emc_find_node_by_ram_code(struct device *dev)
>  	     np = of_find_node_by_name(np, "emc-tables")) {
>  		err = of_property_read_u32(np, "nvidia,ram-code", &value);
>  		if (err || value != ram_code) {
> -			of_node_put(np);
> -			continue;
> +			struct device_node *lpddr2_np;
> +			bool conf_mismatches = false;
> +
> +			lpddr2_np = of_find_node_by_name(np, "lpddr2-configuration");
> +			if (lpddr2_np) {
> +				struct lpddr2_configuration cfg;
> +
> +				err = of_lpddr2_get_config(lpddr2_np, &cfg);
> +				if (!err) {
> +					if (cfg.manufacturer_id >= 0 &&
> +					    cfg.manufacturer_id != emc->manufacturer_id)
> +						conf_mismatches = true;
> +
> +					if (cfg.revision_id1 >= 0 &&
> +					    cfg.revision_id1 != emc->revision_id1)
> +						conf_mismatches = true;
> +
> +					if (cfg.revision_id2 >= 0 &&
> +					    cfg.revision_id2 != emc->revision_id2)
> +						conf_mismatches = true;
> +
> +					if (cfg.density >= 0 &&
> +					    cfg.density != 64 << emc->basic_conf4.density)
> +						conf_mismatches = true;
> +
> +					if (cfg.io_width >= 0 &&
> +					    cfg.io_width != 32 >> emc->basic_conf4.io_width)
> +						conf_mismatches = true;
> +
> +					if (cfg.arch_type >= 0 &&
> +					    cfg.arch_type != emc->basic_conf4.arch_type)
> +						conf_mismatches = true;
> +				} else {
> +					dev_err(emc->dev, "failed to parse %pOF: %d\n",
> +						lpddr2_np, err);
> +					conf_mismatches = true;
> +				}
> +
> +				of_node_put(lpddr2_np);
> +			} else {
> +				conf_mismatches = true;
> +			}
> +
> +			if (conf_mismatches) {
> +				of_node_put(np);
> +				continue;
> +			}
>  		}
>  
>  		return np;
> @@ -455,10 +531,70 @@ tegra_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 val, memory_dev = emem_dev + 1, mr_mask = 0xff;

One initialization per line, in reversed christmas tree (which matches
rest of the file, I thnk).

> +	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);
> +		return err;
> +	}
> +
> +	/* read out 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,
> +				      bool print_out)
> +{
> +	/* these registers are standard for all LPDDR JEDEC memory chips */
> +	emc_read_lpddr_mode_register(emc, emem_dev, 5, &emc->manufacturer_id);
> +	emc_read_lpddr_mode_register(emc, emem_dev, 6, &emc->revision_id1);
> +	emc_read_lpddr_mode_register(emc, emem_dev, 7, &emc->revision_id2);
> +	emc_read_lpddr_mode_register(emc, emem_dev, 8, &emc->basic_conf4.value);

You ignore the return codes but you should not try to load timings in
such case. The DT could (by mistake or on purpose) have values '0' for
the fields you compare.

> +
> +	if (!print_out)
> +		return;
> +
> +	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, emc->manufacturer_id,
> +		 lpddr2_jedec_manufacturer(emc->manufacturer_id),
> +		 emc->revision_id1, emc->revision_id2,
> +		 4 >> emc->basic_conf4.arch_type,
> +		 64 << emc->basic_conf4.density,
> +		 32 >> emc->basic_conf4.io_width);
> +}
> +
>  static int emc_setup_hw(struct tegra_emc *emc)
>  {
> +	u32 emc_cfg, emc_dbg, emc_fbio, emc_adr_cfg;
>  	u32 intmask = EMC_REFRESH_OVERFLOW_INT;
> -	u32 emc_cfg, emc_dbg, emc_fbio;
> +	static bool print_sdram_info_once;

How about moving print_sdram_info_once to emc_read_lpddr_sdram_info()?
Less code here.

> +	enum emc_dram_type dram_type;
> +	const char *dram_type_str;
> +	unsigned int emem_numdev;
>  
>  	emc_cfg = readl_relaxed(emc->regs + EMC_CFG_2);
>  
> @@ -496,7 +632,36 @@ static int emc_setup_hw(struct tegra_emc *emc)
>  	else
>  		emc->dram_bus_width = 32;
>  

Best regards,
Krzysztof
Dmitry Osipenko Oct. 4, 2021, 5:05 p.m. UTC | #3
04.10.2021 12:09, Krzysztof Kozlowski пишет:
>>  static int emc_setup_hw(struct tegra_emc *emc)
>>  {
>> +	u32 emc_cfg, emc_dbg, emc_fbio, emc_adr_cfg;
>>  	u32 intmask = EMC_REFRESH_OVERFLOW_INT;
>> -	u32 emc_cfg, emc_dbg, emc_fbio;
>> +	static bool print_sdram_info_once;
> How about moving print_sdram_info_once to emc_read_lpddr_sdram_info()?
> Less code here.
> 

The SDRAM info is printed out for each attached SDRAM chip. There are
two prints if two chips are attached to memory controller. Hence this
print_once flag should cover both prints.
Dmitry Osipenko Oct. 5, 2021, 3:51 p.m. UTC | #4
04.10.2021 12:09, Krzysztof Kozlowski пишет:
>> +static void emc_read_lpddr_sdram_info(struct tegra_emc *emc,
>> +				      unsigned int emem_dev,
>> +				      bool print_out)
>> +{
>> +	/* these registers are standard for all LPDDR JEDEC memory chips */
>> +	emc_read_lpddr_mode_register(emc, emem_dev, 5, &emc->manufacturer_id);
>> +	emc_read_lpddr_mode_register(emc, emem_dev, 6, &emc->revision_id1);
>> +	emc_read_lpddr_mode_register(emc, emem_dev, 7, &emc->revision_id2);
>> +	emc_read_lpddr_mode_register(emc, emem_dev, 8, &emc->basic_conf4.value);
> You ignore the return codes but you should not try to load timings in
> such case. The DT could (by mistake or on purpose) have values '0' for
> the fields you compare.
> 

Good suggestion. I'll add a flag that will prevent loading timings if
there was MRR error, thanks.