[U-Boot,RFC,4/5] ram: add SDRAM driver for i.MXRT SoCs

Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
---
 drivers/ram/Kconfig                      |   8 +
 drivers/ram/Makefile                     |   2 +
 drivers/ram/imxrt_sdram.c                | 406 +++++++++++++++++++++++
 include/dt-bindings/memory/imxrt-sdram.h | 168 ++++++++++
 4 files changed, 584 insertions(+)
 create mode 100644 drivers/ram/imxrt_sdram.c
 create mode 100644 include/dt-bindings/memory/imxrt-sdram.h

Hi Stefano, Fabio and All,

I have some question below:

On 10/30/19 10:09 PM, Giulio Benetti wrote:
> Signed-off-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> ---
>   drivers/ram/Kconfig                      |   8 +
>   drivers/ram/Makefile                     |   2 +
>   drivers/ram/imxrt_sdram.c                | 406 +++++++++++++++++++++++
>   include/dt-bindings/memory/imxrt-sdram.h | 168 ++++++++++
>   4 files changed, 584 insertions(+)
>   create mode 100644 drivers/ram/imxrt_sdram.c
>   create mode 100644 include/dt-bindings/memory/imxrt-sdram.h
> 
> diff --git a/drivers/ram/Kconfig b/drivers/ram/Kconfig
> index 568d8f2c6a..69b0233a48 100644
> --- a/drivers/ram/Kconfig
> +++ b/drivers/ram/Kconfig
> @@ -54,5 +54,13 @@ config K3_AM654_DDRSS
>   	  config add support for the initialization of the external
>   	  SDRAM devices connected to DDR subsystem.
>   
> +config IMXRT_SDRAM
> +	bool "Enable i.MXRT SDRAM support"
> +	depends on RAM
> +	help
> +	  i.MXRT family devices support smart external memory controller(SEMC)
> +	  to support external memories like sdram, psram & nand.
> +	  This driver is for the sdram memory interface with the SEMC.
> +
>   source "drivers/ram/rockchip/Kconfig"
>   source "drivers/ram/stm32mp1/Kconfig"
> diff --git a/drivers/ram/Makefile b/drivers/ram/Makefile
> index 976ec66df7..d3813e37e4 100644
> --- a/drivers/ram/Makefile
> +++ b/drivers/ram/Makefile
> @@ -14,3 +14,5 @@ obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip/
>   
>   obj-$(CONFIG_K3_AM654_DDRSS) += k3-am654-ddrss.o
>   obj-$(CONFIG_ARCH_MEDIATEK) += mediatek/
> +
> +obj-$(CONFIG_IMXRT_SDRAM) += imxrt_sdram.o
> diff --git a/drivers/ram/imxrt_sdram.c b/drivers/ram/imxrt_sdram.c
> new file mode 100644
> index 0000000000..7c157f60e2
> --- /dev/null
> +++ b/drivers/ram/imxrt_sdram.c
> @@ -0,0 +1,406 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019
> + * Author(s): Giulio Benetti <giulio.benetti@benettiengineering.com>
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <ram.h>
> +#include <asm/io.h>
> +
> +/* SDRAM Command Code */
> +#define SD_CC_ARD		0x0     /* Master Bus (AXI) command - Read */
> +#define SD_CC_AWR		0x1     /* Master Bus (AXI) command - Write */
> +#define SD_CC_IRD		0x8     /* IP command - Read */
> +#define SD_CC_IWR		0x9     /* IP command - Write */
> +#define SD_CC_IMS		0xA     /* IP command - Set Mode Register */
> +#define SD_CC_IACT		0xB     /* IP command - ACTIVE */
> +#define SD_CC_IAF		0xC     /* IP command - Auto Refresh */
> +#define SD_CC_ISF		0xD     /* IP Command - Self Refresh */
> +#define SD_CC_IPRE		0xE     /* IP command - Precharge */
> +#define SD_CC_IPREA		0xF     /* IP command - Precharge ALL */
> +
> +#define SEMC_MCR_MDIS		BIT(1)
> +#define SEMC_MCR_DQSMD		BIT(2)
> +
> +struct imxrt_semc_regs {
> +	/* 0x0 */
> +	u32 mcr;
> +	u32 iocr;
> +	u32 bmcr0;
> +	u32 bmcr1;
> +	u32 br[9];
> +
> +	/* 0x34 */
> +	u32 res1;
> +	u32 inten;
> +	u32 intr;
> +	/* 0x40 */
> +	u32 sdramcr0;
> +	u32 sdramcr1;
> +	u32 sdramcr2;
> +	u32 sdramcr3;
> +	/* 0x50 */
> +	u32 nandcr0;
> +	u32 nandcr1;
> +	u32 nandcr2;
> +	u32 nandcr3;
> +	/* 0x60 */
> +	u32 norcr0;
> +	u32 norcr1;
> +	u32 norcr2;
> +	u32 norcr3;
> +	/* 0x70 */
> +	u32 sramcr0;
> +	u32 sramcr1;
> +	u32 sramcr2;
> +	u32 sramcr3;
> +	/* 0x80 */
> +	u32 dbicr0;
> +	u32 dbicr1;
> +	u32 res2[2];
> +	/* 0x90 */
> +	u32 ipcr0;
> +	u32 ipcr1;
> +	u32 ipcr2;
> +	u32 ipcmd;
> +	/* 0xA0 */
> +	u32 iptxdat;
> +	u32 res3[3];
> +	/* 0xB0 */
> +	u32 iprxdat;
> +	u32 res4[3];
> +	/* 0xC0 */
> +	u32 sts[16];
> +};
> +
> +#define SEMC_IOCR_MUX_A8_SHIFT		0
> +#define SEMC_IOCR_MUX_CSX0_SHIFT	1
> +#define SEMC_IOCR_MUX_CSX1_SHIFT	2
> +#define SEMC_IOCR_MUX_CSX2_SHIFT	3
> +#define SEMC_IOCR_MUX_CSX3_SHIFT	4
> +#define SEMC_IOCR_MUX_RDY_SHIFT		5
> +
> +struct imxrt_sdram_mux {
> +	u8 a8;
> +	u8 csx0;
> +	u8 csx1;
> +	u8 csx2;
> +	u8 csx3;
> +	u8 rdy;
> +};
> +
> +#define SEMC_SDRAMCR0_PS_SHIFT		0
> +#define SEMC_SDRAMCR0_BL_SHIFT		4
> +#define SEMC_SDRAMCR0_COL_SHIFT		8
> +#define SEMC_SDRAMCR0_CL_SHIFT		10
> +
> +struct imxrt_sdram_control {
> +	u8 memory_width;
> +	u8 burst_len;
> +	u8 no_columns;
> +	u8 cas_latency;
> +};
> +
> +#define SEMC_SDRAMCR1_PRE2ACT_SHIFT	0
> +#define SEMC_SDRAMCR1_ACT2RW_SHIFT	4
> +#define SEMC_SDRAMCR1_RFRC_SHIFT	8
> +#define SEMC_SDRAMCR1_WRC_SHIFT		12
> +
> +struct imxrt_sdram_timing {
> +	u8 trp;
> +	u8 trcd;
> +	u8 trfc;
> +	u8 twr;
> +#if TODO
> +	u8 tmrd;
> +	u8 txsr;
> +	u8 tras;
> +	u8 trc;
> +#endif
> +};
> +
> +enum imxrt_semc_bank {
> +	SDRAM_BANK1,
> +	SDRAM_BANK2,
> +	SDRAM_BANK3,
> +	SDRAM_BANK4,
> +	MAX_SDRAM_BANK,
> +};
> +
> +#define SEMC_BR_VLD_MASK		1
> +#define SEMC_BR_MS_SHIFT		1
> +
> +struct bank_params {
> +	enum imxrt_semc_bank target_bank;
> +	u32 base_address;
> +	u32 memory_size;
> +};
> +
> +struct imxrt_sdram_params {
> +	struct imxrt_semc_regs *base;
> +
> +	struct imxrt_sdram_mux *sdram_mux;
> +	struct imxrt_sdram_control *sdram_control;
> +	struct imxrt_sdram_timing *sdram_timing;
> +
> +	struct bank_params bank_params[MAX_SDRAM_BANK];
> +	u8 no_sdram_banks;
> +};
> +
> +void imxrt_sdram_wait_ipcmd_done(struct imxrt_semc_regs *regs)
> +{
> +	do {
> +		mdelay(50);
> +		/* check for IPCMDDONE or IPCMERR */
> +		/* TODO: handle error */
> +	} while (!(readl(&regs->intr) & 0x3));
> +}
> +
> +u32 imxrt_sdram_ipcmd(struct imxrt_semc_regs *regs, u32 mem_addr, u32 ipcmd,
> +		      u32 wd)
> +{
> +	u32 rd = 0x0;
> +
> +	if (ipcmd == SD_CC_IWR || ((ipcmd & 0x0000FFFF) == SD_CC_IMS))
> +		writel(wd & 0x0000FFFF, &regs->iptxdat);
> +
> +	/* set slave address for every command */
> +	writel(mem_addr, &regs->ipcr0);
> +
> +	/* execute command */
> +	writel(0xA55A0000 | (ipcmd & 0x0000FFFF), &regs->ipcmd);
> +
> +	imxrt_sdram_wait_ipcmd_done(regs);
> +
> +	if (ipcmd == SD_CC_IRD)
> +		rd = readl(&regs->iprxdat);
> +
> +	return rd;
> +}
> +
> +int imxrt_sdram_init(struct udevice *dev)
> +{
> +	struct imxrt_sdram_params *params = dev_get_platdata(dev);
> +	struct imxrt_sdram_mux *mux = params->sdram_mux;
> +	struct imxrt_sdram_control *ctrl = params->sdram_control;
> +	struct imxrt_sdram_timing *time = params->sdram_timing;
> +	struct imxrt_semc_regs *regs = params->base;
> +	struct bank_params *bank_params = params->bank_params;
> +	int i;
> +
> +	/* enable the SEMC controller */
> +	clrbits_le32(&regs->mcr, SEMC_MCR_MDIS);
> +	/* set DQS mode from DQS pad */
> +	setbits_le32(&regs->mcr, SEMC_MCR_DQSMD);
> +
> +	for (i = 0; i < params->no_sdram_banks; bank_params++, i++)
> +		writel((bank_params->base_address & 0xfffff000)
> +		       | bank_params->memory_size << SEMC_BR_MS_SHIFT
> +		       | SEMC_BR_VLD_MASK,
> +		       &regs->br[bank_params->target_bank]);
> +
> +	writel(mux->a8 << SEMC_IOCR_MUX_A8_SHIFT
> +		| mux->csx0 << SEMC_IOCR_MUX_CSX0_SHIFT
> +		| mux->csx1 << SEMC_IOCR_MUX_CSX1_SHIFT
> +		| mux->csx2 << SEMC_IOCR_MUX_CSX2_SHIFT
> +		| mux->csx3 << SEMC_IOCR_MUX_CSX3_SHIFT
> +		| mux->rdy << SEMC_IOCR_MUX_RDY_SHIFT,
> +		&regs->iocr);
> +
> +	writel(ctrl->memory_width << SEMC_SDRAMCR0_PS_SHIFT
> +		| ctrl->burst_len << SEMC_SDRAMCR0_BL_SHIFT
> +		| ctrl->no_columns << SEMC_SDRAMCR0_COL_SHIFT
> +		| ctrl->cas_latency << SEMC_SDRAMCR0_CL_SHIFT,
> +		&regs->sdramcr0);
> +
> +	writel(time->trp << SEMC_SDRAMCR1_PRE2ACT_SHIFT
> +		| time->trcd << SEMC_SDRAMCR1_ACT2RW_SHIFT
> +		| time->trfc << SEMC_SDRAMCR1_RFRC_SHIFT
> +		| time->twr << SEMC_SDRAMCR1_WRC_SHIFT,
> +		&regs->sdramcr1);

Here I've tried to use classic sdram timing as parameters but...

> +	/*
> +	 *	SDRAMCR1:
> +	 *	PRE2ACT => tRP(0x02)
> +	 *	ACT2RW => tRCD(0x02)
> +	 *	RFRC => tRFC(0x09)
> +	 *	WRC => tWR(0x01)
> +	 *	TODO: CKEOFF => CKE Off minimum time(0x05)
> +	 *	TODO: ACT2PRE => ACT to Precharge minimum time(0x06)
> +	 */
> +	writel(0x00010920, &regs->sdramcr2);

...in this register and the next one there are parameters like CKEOFF 
and ACT2PRE impossible to lead to classic tXXX sdram timing parameters.
I've done this way inspired by stm32-sdram, but maybe U should go back 
using i.MXRT names(i.e. PRE2ACT instead of tRP) and list all possible 
useful registers in DM as I've done for tRP etc.
What do you think?

And...

> +	/*
> +	 *	SDRAMCR2:
> +	 *	SRRC => Self Refresh Recovery Time(0x20)
> +	 *	REF2REF => tRFC(0x09)
> +	 *	ACT2ACT => tRRD(0x01)
> +	 *	ITO => SDRAM idle timeout(0x00)
> +	 */
> +	writel(0x50210A09, &regs->sdramcr3);
> +	/*
> +	 *	SDRAMCR3:
> +	 *	REN => Refresh Enable(0x01)
> +	 *	REBL => Refresh Burst Length(0x04)
> +	 *	PRESCALE => Prescaler timer pediod(0x0A)
> +	 *	RT => Refresh timer period(0x21)
> +	 *	UT => Refresh urgent threshold(0x50)
> +	 */ > +	mdelay(250);
> +	imxrt_sdram_ipcmd(regs, bank_params->base_address, SD_CC_IPREA,
> +			  0x0000);
> +	imxrt_sdram_ipcmd(regs, bank_params->base_address, SD_CC_IAF,
> +			  0x0000);
> +	imxrt_sdram_ipcmd(regs, bank_params->base_address, SD_CC_IAF,
> +			  0x0000);
> +	imxrt_sdram_ipcmd(regs, bank_params->base_address, SD_CC_IMS,
> +			  ctrl->burst_len | (ctrl->cas_latency << 4));
> +	mdelay(250);
> +
> +	return 0;
> +}
> +
> +static int imxrt_semc_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct imxrt_sdram_params *params = dev_get_platdata(dev);
> +	ofnode bank_node;
> +	u8 bank = 0;
> +
> +	params->sdram_mux =
> +		(struct imxrt_sdram_mux *)
> +		 dev_read_u8_array_ptr(dev,
> +				       "fsl,sdram-mux",
> +				       sizeof(struct imxrt_sdram_mux));
> +	if (!params->sdram_mux) {
> +		pr_err("fsl,sdram-mux not found");
> +		return -EINVAL;
> +	}
> +
> +	params->sdram_control =
> +		(struct imxrt_sdram_control *)
> +		 dev_read_u8_array_ptr(dev,
> +				       "fsl,sdram-control",
> +				       sizeof(struct imxrt_sdram_control));
> +	if (!params->sdram_control) {
> +		pr_err("fsl,sdram-control not found");
> +		return -EINVAL;
> +	}
> +
> +	params->sdram_timing =
> +		(struct imxrt_sdram_timing *)
> +		 dev_read_u8_array_ptr(dev,
> +				       "fsl,sdram-timing",
> +				       sizeof(struct imxrt_sdram_timing));
> +	if (!params->sdram_timing) {
> +		pr_err("fsl,sdram-timing not found");
> +		return -EINVAL;
> +	}
> +
> +	dev_for_each_subnode(bank_node, dev) {
> +		struct bank_params *bank_params;
> +		char *bank_name;
> +		int ret;
> +
> +		/* extract the bank index from DT */
> +		bank_name = (char *)ofnode_get_name(bank_node);
> +		strsep(&bank_name, "@");
> +		if (!bank_name) {
> +			pr_err("missing sdram bank index");
> +			return -EINVAL;
> +		}
> +
> +		bank_params = &params->bank_params[bank];
> +		strict_strtoul(bank_name, 10,
> +			       (unsigned long int *)&bank_params->target_bank);
> +		if (bank_params->target_bank >= MAX_SDRAM_BANK) {
> +			pr_err("Found bank %d , but only bank 0,1,2,3 are supported",
> +			       bank_params->target_bank);
> +			return -EINVAL;
> +		}
> +
> +		ret = ofnode_read_u32(bank_node,
> +				      "fsl,memory-size",
> +				      &bank_params->memory_size);
> +		if (ret < 0) {
> +			pr_err("fsl,memory-size not found");
> +			return -EINVAL;
> +		}
> +
> +		ret = ofnode_read_u32(bank_node,
> +				      "fsl,base-address",
> +				      &bank_params->base_address);
> +		if (ret < 0) {
> +			pr_err("fsl,base-address not found");
> +			return -EINVAL;
> +		}
> +
> +		debug("Found bank %s %u\n", bank_name,
> +		      bank_params->target_bank);
> +		bank++;
> +	}
> +
> +	params->no_sdram_banks = bank;
> +	debug("%s, no of banks = %d\n", __func__, params->no_sdram_banks);
> +
> +	return 0;
> +}
> +
> +static int imxrt_semc_probe(struct udevice *dev)
> +{
> +	struct imxrt_sdram_params *params = dev_get_platdata(dev);
> +	int ret;
> +	fdt_addr_t addr;
> +
> +	addr = dev_read_addr(dev);
> +	if (addr == FDT_ADDR_T_NONE)
> +		return -EINVAL;
> +
> +	params->base = (struct imxrt_semc_regs *)addr;
> +
> +#ifdef CONFIG_CLK
> +	struct clk clk;
> +
> +	ret = clk_get_by_index(dev, 0, &clk);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = clk_enable(&clk);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +#endif
> +	ret = imxrt_sdram_init(dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int imxrt_semc_get_info(struct udevice *dev, struct ram_info *info)
> +{
> +	return 0;
> +}
> +
> +static struct ram_ops imxrt_semc_ops = {
> +	.get_info = imxrt_semc_get_info,
> +};
> +
> +static const struct udevice_id imxrt_semc_ids[] = {
> +	{ .compatible = "fsl,imxrt-semc", .data = 0 },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(imxrt_semc) = {
> +	.name = "imxrt_semc",
> +	.id = UCLASS_RAM,
> +	.of_match = imxrt_semc_ids,
> +	.ops = &imxrt_semc_ops,
> +	.ofdata_to_platdata = imxrt_semc_ofdata_to_platdata,
> +	.probe = imxrt_semc_probe,
> +	.platdata_auto_alloc_size = sizeof(struct imxrt_sdram_params),
> +};
> diff --git a/include/dt-bindings/memory/imxrt-sdram.h b/include/dt-bindings/memory/imxrt-sdram.h
> new file mode 100644
> index 0000000000..dff6d4401e
> --- /dev/null
> +++ b/include/dt-bindings/memory/imxrt-sdram.h
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2019
> + * Author(s): Giulio Benetti <giulio.benetti@benettiengineering.com>
> + */
> +
> +#ifndef DT_BINDINGS_IMXRT_SDRAM_H
> +#define DT_BINDINGS_IMXRT_SDRAM_H
> +
> +#define MEM_SIZE_4K		0x00
> +#define MEM_SIZE_8K		0x01
> +#define MEM_SIZE_16K		0x02
> +#define MEM_SIZE_32K		0x03
> +#define MEM_SIZE_64K		0x04
> +#define MEM_SIZE_128K		0x05
> +#define MEM_SIZE_256K		0x06
> +#define MEM_SIZE_512K		0x07
> +#define MEM_SIZE_1M		0x08
> +#define MEM_SIZE_2M		0x09
> +#define MEM_SIZE_4M		0x0A
> +#define MEM_SIZE_8M		0x0B
> +#define MEM_SIZE_16M		0x0C
> +#define MEM_SIZE_32M		0x0D
> +#define MEM_SIZE_64M		0x0E
> +#define MEM_SIZE_128M		0x0F
> +#define MEM_SIZE_256M		0x10
> +#define MEM_SIZE_512M		0x11
> +#define MEM_SIZE_1G		0x12
> +#define MEM_SIZE_2G		0x13
> +#define MEM_SIZE_4G		0x14
> +
> +#define MUX_A8_SDRAM_A8		0x0
> +#define MUX_A8_NAND_CE		0x1
> +#define MUX_A8_NOR_CE		0x2
> +#define MUX_A8_PSRAM_CE		0x3
> +#define MUX_A8_DBI_CSX		0x4
> +
> +#define MUX_CSX0_NOR_PSRAM_A24	0x0
> +#define MUX_CSX0_SDRAM_CS1	0x1
> +#define MUX_CSX0_SDRAM_CS2	0x2
> +#define MUX_CSX0_SDRAM_CS3	0x3
> +#define MUX_CSX0_NAND_CE	0x4
> +#define MUX_CSX0_NOR_CE		0x5
> +#define MUX_CSX0_PSRAM_CE	0x6
> +#define MUX_CSX0_DBI_CSX	0x7
> +
> +#define MUX_CSX1_NOR_PSRAM_A25	0x0
> +#define MUX_CSX1_SDRAM_CS1	0x1
> +#define MUX_CSX1_SDRAM_CS2	0x2
> +#define MUX_CSX1_SDRAM_CS3	0x3
> +#define MUX_CSX1_NAND_CE	0x4
> +#define MUX_CSX1_NOR_CE		0x5
> +#define MUX_CSX1_PSRAM_CE	0x6
> +#define MUX_CSX1_DBI_CSX	0x7
> +
> +#define MUX_CSX2_NOR_PSRAM_A26	0x0
> +#define MUX_CSX2_SDRAM_CS1	0x1
> +#define MUX_CSX2_SDRAM_CS2	0x2
> +#define MUX_CSX2_SDRAM_CS3	0x3
> +#define MUX_CSX2_NAND_CE	0x4
> +#define MUX_CSX2_NOR_CE		0x5
> +#define MUX_CSX2_PSRAM_CE	0x6
> +#define MUX_CSX2_DBI_CSX	0x7
> +
> +#define MUX_CSX3_NOR_PSRAM_A27	0x0
> +#define MUX_CSX3_SDRAM_CS1	0x1
> +#define MUX_CSX3_SDRAM_CS2	0x2
> +#define MUX_CSX3_SDRAM_CS3	0x3
> +#define MUX_CSX3_NAND_CE	0x4
> +#define MUX_CSX3_NOR_CE		0x5
> +#define MUX_CSX3_PSRAM_CE	0x6
> +#define MUX_CSX3_DBI_CSX	0x7
> +
> +#define MUX_RDY_NAND_RDY_WAIT	0x0
> +#define MUX_RDY_SDRAM_CS1	0x1
> +#define MUX_RDY_SDRAM_CS2	0x2
> +#define MUX_RDY_SDRAM_CS3	0x3
> +#define MUX_RDY_NOR_CE		0x4
> +#define MUX_RDY_PSRAM_CE	0x5
> +#define MUX_RDY_DBI_CSX		0x6
> +#define MUX_RDY_NOR_PSRAM_A27	0x7
> +
> +#define MEM_WIDTH_8BITS		0x0
> +#define MEM_WIDTH_16BITS	0x1
> +
> +#define BL_1			0x0
> +#define BL_2			0x1
> +#define BL_4			0x2
> +#define BL_8			0x3
> +
> +#define COL_12BITS		0x0
> +#define COL_11BITS		0x1
> +#define COL_10BITS		0x2
> +#define COL_9BITS		0x3
> +
> +#define CL_1			0x0
> +#define CL_2			0x2
> +#define CL_3			0x3
> +
> +#define TRP_1			0x0
> +#define TRP_2			0x1
> +#define TRP_3			0x2
> +#define TRP_4			0x3
> +#define TRP_5			0x4
> +#define TRP_6			0x5
> +#define TRP_7			0x6
> +#define TRP_8			0x7
> +#define TRP_9			0x8
> +#define TRP_10			0x9
> +#define TRP_11			0xA
> +#define TRP_12			0xB
> +#define TRP_13			0xC
> +#define TRP_14			0xD
> +#define TRP_15			0xE
> +#define TRP_16			0xF
> +
> +#define TRCD_1			0x0
> +#define TRCD_2			0x1
> +#define TRCD_3			0x2
> +#define TRCD_4			0x3
> +#define TRCD_5			0x4
> +#define TRCD_6			0x5
> +#define TRCD_7			0x6
> +#define TRCD_8			0x7
> +#define TRCD_9			0x8
> +#define TRCD_10			0x9
> +#define TRCD_11			0xA
> +#define TRCD_12			0xB
> +#define TRCD_13			0xC
> +#define TRCD_14			0xD
> +#define TRCD_15			0xE
> +#define TRCD_16			0xF
> +
> +#define TRFC_1			0x0
> +#define TRFC_2			0x1
> +#define TRFC_3			0x2
> +#define TRFC_4			0x3
> +#define TRFC_5			0x4
> +#define TRFC_6			0x5
> +#define TRFC_7			0x6
> +#define TRFC_8			0x7
> +#define TRFC_9			0x8
> +#define TRFC_10			0x9
> +#define TRFC_11			0xA
> +#define TRFC_12			0xB
> +#define TRFC_13			0xC
> +#define TRFC_14			0xD
> +#define TRFC_15			0xE
> +#define TRFC_16			0xF
> +
> +#define TWR_1			0x0
> +#define TWR_2			0x1
> +#define TWR_3			0x2
> +#define TWR_4			0x3
> +#define TWR_5			0x4
> +#define TWR_6			0x5
> +#define TWR_7			0x6
> +#define TWR_8			0x7
> +#define TWR_9			0x8
> +#define TWR_10			0x9
> +#define TWR_11			0xA
> +#define TWR_12			0xB
> +#define TWR_13			0xC
> +#define TWR_14			0xD
> +#define TWR_15			0xE
> +#define TWR_16			0xF
> +
> +#endif
> 

...is it ok listing every possible value for DM parameter?
I mean, some of them would be 8-bit, this would mean having 256 cases, 
that sound very ugly to me.

Thanks in advance

Kind regards

Hi Giulio,

On Thu, Oct 31, 2019 at 9:11 AM Giulio Benetti
<giulio.benetti@benettiengineering.com> wrote:

> ...in this register and the next one there are parameters like CKEOFF
> and ACT2PRE impossible to lead to classic tXXX sdram timing parameters.
> I've done this way inspired by stm32-sdram, but maybe U should go back
> using i.MXRT names(i.e. PRE2ACT instead of tRP) and list all possible
> useful registers in DM as I've done for tRP etc.
> What do you think?

I think it is OK to use the i.MXRT names.

#define TWR_1                        0x0
> > +#define TWR_2                        0x1
> > +#define TWR_3                        0x2
> > +#define TWR_4                        0x3
> > +#define TWR_5                        0x4
> > +#define TWR_6                        0x5
> > +#define TWR_7                        0x6
> > +#define TWR_8                        0x7
> > +#define TWR_9                        0x8
> > +#define TWR_10                       0x9
> > +#define TWR_11                       0xA
> > +#define TWR_12                       0xB
> > +#define TWR_13                       0xC
> > +#define TWR_14                       0xD
> > +#define TWR_15                       0xE
> > +#define TWR_16                       0xF
> > +
> > +#endif
> >
>
> ...is it ok listing every possible value for DM parameter?
> I mean, some of them would be 8-bit, this would mean having 256 cases,
> that sound very ugly to me.

It doesn't seem necessary to write all 256 cases. I would suggest to
write the direct hex number.

On 11/4/19 5:33 PM, Fabio Estevam wrote:
> Hi Giulio,
> 
> On Thu, Oct 31, 2019 at 9:11 AM Giulio Benetti
> <giulio.benetti@benettiengineering.com> wrote:
> 
>> ...in this register and the next one there are parameters like CKEOFF
>> and ACT2PRE impossible to lead to classic tXXX sdram timing parameters.
>> I've done this way inspired by stm32-sdram, but maybe U should go back
>> using i.MXRT names(i.e. PRE2ACT instead of tRP) and list all possible
>> useful registers in DM as I've done for tRP etc.
>> What do you think?
> 
> I think it is OK to use the i.MXRT names.

Ok

> #define TWR_1                        0x0
>>> +#define TWR_2                        0x1
>>> +#define TWR_3                        0x2
>>> +#define TWR_4                        0x3
>>> +#define TWR_5                        0x4
>>> +#define TWR_6                        0x5
>>> +#define TWR_7                        0x6
>>> +#define TWR_8                        0x7
>>> +#define TWR_9                        0x8
>>> +#define TWR_10                       0x9
>>> +#define TWR_11                       0xA
>>> +#define TWR_12                       0xB
>>> +#define TWR_13                       0xC
>>> +#define TWR_14                       0xD
>>> +#define TWR_15                       0xE
>>> +#define TWR_16                       0xF
>>> +
>>> +#endif
>>>
>>
>> ...is it ok listing every possible value for DM parameter?
>> I mean, some of them would be 8-bit, this would mean having 256 cases,
>> that sound very ugly to me.
> 
> It doesn't seem necessary to write all 256 cases. I would suggest to
> write the direct hex number.
> 

Well, thanks a lot!

Best regards