Re: [PATCH v3 4/4] ARM: novena: Use DDR3 information from SPD EEPROM

[Marco Felsch <m.felsch@pengutronix.de>]

Hi John,

thanks for your patch.

On 23-01-30, John Watts wrote:
> The Novena supports swappable memory which means we must query the memory
> for configuration details. This patch uses values stored in the SPD EEPROM
> to configure the i.MX6 memory controller.
> 
> If the SPD can't be read, the default configuration for a 4GB stick is used.
> 
> Calibration for the installed stick is run unconditionally.
> 
> Verification that this works was done by comparing MMDC registers before and
> after and carefully reading other people's source code.
> 
> Signed-off-by: John Watts <contact@jookia.org>
> ---
>  arch/arm/boards/novena/lowlevel.c | 108 ++++++++++++++++++++++++++++++
>  arch/arm/mach-imx/Kconfig         |   2 +
>  2 files changed, 110 insertions(+)
> 
> diff --git a/arch/arm/boards/novena/lowlevel.c b/arch/arm/boards/novena/lowlevel.c
> index 3acf983ee0..54980bda4e 100644
> --- a/arch/arm/boards/novena/lowlevel.c
> +++ b/arch/arm/boards/novena/lowlevel.c
> @@ -3,6 +3,8 @@
>  
>  #include <asm/barebox-arm.h>
>  #include <common.h>
> +#include <ddr_dimms.h>
> +#include <ddr_spd.h>
>  #include <debug_ll.h>
>  #include <mach/esdctl.h>
>  #include <mach/generic.h>
> @@ -10,6 +12,7 @@
>  #include <mach/imx6-mmdc.h>
>  #include <mach/iomux-mx6.h>
>  #include <mach/xload.h>
> +#include <pbl/i2c.h>
>  #include <soc/fsl/fsl_udc.h>
>  #include "ddr_regs.h"
>  
> @@ -17,6 +20,106 @@
>  
>  extern char __dtb_imx6q_novena_start[];
>  
> +static struct spd_eeprom spd_eeprom;
> +static struct dimm_params dimm_params;
> +
> +static struct pbl_i2c *setup_spd_i2c(void)
> +{
> +	void __iomem *iomuxbase = IOMEM(MX6_IOMUXC_BASE_ADDR);
> +
> +	imx_setup_pad(iomuxbase, MX6Q_PAD_CSI0_DAT8__I2C1_SDA);
> +	imx_setup_pad(iomuxbase, MX6Q_PAD_CSI0_DAT9__I2C1_SCL);
> +
> +	return imx6_i2c_early_init(IOMEM(MX6_I2C1_BASE_ADDR));
> +}
> +
> +static struct spd_eeprom *read_spd(void)
> +{
> +	struct spd_eeprom *eeprom = &spd_eeprom;
> +	struct pbl_i2c *i2c = setup_spd_i2c();
> +	int rc;
> +
> +	rc = spd_read_eeprom(i2c, 0x50, eeprom, SPD_MEMTYPE_DDR3);
> +	if (rc < 0) {
> +		pr_err("Couldn't read SPD EEPROM: %i\n", rc);
> +		return NULL;
> +	}
> +
> +	rc = ddr3_spd_check(&eeprom->ddr3);
> +	if (rc < 0) {
> +		pr_err("Couldn't verify SPD data: %i\n", rc);
> +		return NULL;
> +	}
> +
> +	return eeprom;
> +}
> +
> +static bool check_spd_compatible(struct spd_eeprom *eeprom)
> +{
> +	if (eeprom->ddr3.mem_type != 0x0b) {

Is this parameter and value standarized?

> +		pr_err("SPD data is not DDR3\n");
> +		return false;
> +	}
> +
> +	if ((eeprom->ddr3.bus_width & 0x1f) != 0x03) {

Same question as above. Would be nice to avoid magic numbers if this is
the case.

Regards,
  Marco

> +		pr_err("SPD data is for a 64-bit bus\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void setup_dimm_settings(struct dimm_params *params,
> +				struct mx6_ddr_sysinfo *info,
> +				struct mx6_ddr3_cfg *cfg)
> +{
> +	int capacity_gbit = params->capacity / 0x8000000;
> +	int density_rank = capacity_gbit / params->n_ranks;
> +
> +	info->ncs = params->n_ranks;
> +	info->cs_density = density_rank;
> +	cfg->mem_speed = params->tckmin_x_ps;
> +	cfg->density = density_rank / params->n_banks_per_sdram_device;
> +	cfg->width = params->data_width;
> +	cfg->banks = params->n_banks_per_sdram_device;
> +	cfg->rowaddr = params->n_row_addr;
> +	cfg->coladdr = params->n_col_addr;
> +	cfg->trcd = params->trcd_ps / 10;
> +	cfg->trcmin = params->trc_ps / 10;
> +	cfg->trasmin = params->tras_ps / 10;
> +	cfg->SRT = params->extended_op_srt;
> +
> +	if (params->device_width >= 16)
> +		cfg->pagesz = 2;
> +}
> +
> +static void read_dimm_settings(void)
> +{
> +	struct spd_eeprom *eeprom = read_spd();
> +	struct dimm_params *params = &dimm_params;
> +	int rc;
> +
> +	if (!eeprom) {
> +		pr_err("Couldn't read SPD EEPROM, using default settings\n");
> +		return;
> +	}
> +
> +	if (!check_spd_compatible(eeprom)) {
> +		pr_err("DIMM stick incompatible\n");
> +		hang();
> +	}
> +
> +	rc = ddr3_compute_dimm_parameters(&eeprom->ddr3, params);
> +	if (rc < 0) {
> +		pr_err("Couldn't compute DIMM params: %i\n", rc);
> +		return;
> +	}
> +
> +	setup_dimm_settings(params, &novena_ddr_info, &novena_ddr_cfg);
> +
> +	pr_info("Found DIMM: %s\n", params->mpart);
> +}
> +
>  static bool running_from_ram(void)
>  {
>  	return (get_pc() >= MX6_MMDC_PORT01_BASE_ADDR);
> @@ -36,8 +139,13 @@ static void setup_uart(void)
>  
>  static void setup_ram(void)
>  {
> +	read_dimm_settings();
> +
>  	mx6dq_dram_iocfg(64, &novena_ddr_regs, &novena_grp_regs);
>  	mx6_dram_cfg(&novena_ddr_info, &novena_mmdc_calib, &novena_ddr_cfg);
> +
> +	mmdc_do_write_level_calibration();
> +	mmdc_do_dqs_calibration();
>  }
>  
>  static void load_barebox(void)
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index 7993835be7..aa7a86da66 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -437,6 +437,8 @@ config MACH_NITROGEN6
>  config MACH_NOVENA
>  	bool "Kosagi Novena board"
>  	select ARCH_IMX6
> +	select DDR_SPD
> +	select I2C_IMX_EARLY
>  	select MCI_IMX_ESDHC_PBL
>  	select USB_GADGET_DRIVER_ARC_PBL
>  
> -- 
> 2.39.1
> 
> 
>