[U-Boot] [RFC PATCH 3/9] x86: quark: Add Memory Reference Code (MRC) main routines
Simon Glass
sjg at chromium.org
Wed Feb 4 17:24:47 CET 2015
Hi Bin,
On 3 February 2015 at 04:45, Bin Meng <bmeng.cn at gmail.com> wrote:
> Add the main routines for Quark Memory Reference Code (MRC).
>
> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>
> ---
> The are 24 checkpatch warnings in this patch, which is:
>
> warning: arch/x86/cpu/quark/mrc.c,43: line over 80 characters
> ...
>
> I intentionally leave it as is now, as fixing these warnings
> make the mrc initialization table a little bit harder to read.
>
> arch/x86/cpu/quark/mrc.c | 206 ++++++++++++++++++++++++++++++++++
> arch/x86/include/asm/arch-quark/mrc.h | 189 +++++++++++++++++++++++++++++++
> 2 files changed, 395 insertions(+)
> create mode 100644 arch/x86/cpu/quark/mrc.c
> create mode 100644 arch/x86/include/asm/arch-quark/mrc.h
>
> diff --git a/arch/x86/cpu/quark/mrc.c b/arch/x86/cpu/quark/mrc.c
> new file mode 100644
> index 0000000..6a82519
> --- /dev/null
> +++ b/arch/x86/cpu/quark/mrc.c
> @@ -0,0 +1,206 @@
> +/*
> + * Copyright (C) 2013, Intel Corporation
> + * Copyright (C) 2015, Bin Meng <bmeng.cn at gmail.com>
> + *
> + * Ported from Intel released Quark UEFI BIOS
> + * QuarkSocPkg/QuarkNorthCluster/MemoryInit/Pei/
> + *
> + * SPDX-License-Identifier: Intel
> + */
> +
> +/*
> + * This is the main Quark Memory Reference Code (MRC)
> + *
> + * These functions are generic and should work for any Quark based board.
Quark-based
> + *
> + * MRC requires two data structures to be passed in which are initialized by
> + * mrc_adjust_params().
> + *
> + * The basic flow is as follows:
> + * 01) Check for supported DDR speed configuration
> + * 02) Set up Memory Manager buffer as pass-through (POR)
> + * 03) Set Channel Interleaving Mode and Channel Stride to the most aggressive
> + * setting possible
> + * 04) Set up the Memory Controller logic
> + * 05) Set up the DDR_PHY logic
> + * 06) Initialise the DRAMs (JEDEC)
> + * 07) Perform the Receive Enable Calibration algorithm
> + * 08) Perform the Write Leveling algorithm
> + * 09) Perform the Read Training algorithm (includes internal Vref)
> + * 10) Perform the Write Training algorithm
> + * 11) Set Channel Interleaving Mode and Channel Stride to the desired settings
> + *
> + * Dunit configuration based on Valleyview MRC.
What is Dunit?
> + */
> +
> +#include <common.h>
> +#include <asm/arch/mrc.h>
> +#include <asm/arch/msg_port.h>
> +#include "mrc_util.h"
> +#include "smc.h"
> +
> +static const struct mem_init init[] = {
> + { 0x0101, BM_COLD | BM_FAST | BM_WARM | BM_S3, clear_self_refresh },
> + { 0x0200, BM_COLD | BM_FAST | BM_WARM | BM_S3, prog_ddr_timing_control },
> + { 0x0103, BM_COLD | BM_FAST , prog_decode_before_jedec },
> + { 0x0104, BM_COLD | BM_FAST , perform_ddr_reset },
> + { 0x0300, BM_COLD | BM_FAST | BM_S3, ddrphy_init },
> + { 0x0400, BM_COLD | BM_FAST , perform_jedec_init },
> + { 0x0105, BM_COLD | BM_FAST , set_ddr_init_complete },
> + { 0x0106, BM_FAST | BM_WARM | BM_S3, restore_timings },
> + { 0x0106, BM_COLD , default_timings },
> + { 0x0500, BM_COLD , rcvn_cal },
> + { 0x0600, BM_COLD , wr_level },
> + { 0x0120, BM_COLD , prog_page_ctrl },
> + { 0x0700, BM_COLD , rd_train },
> + { 0x0800, BM_COLD , wr_train },
> + { 0x010B, BM_COLD , store_timings },
> + { 0x010C, BM_COLD | BM_FAST | BM_WARM | BM_S3, enable_scrambling },
> + { 0x010D, BM_COLD | BM_FAST | BM_WARM | BM_S3, prog_ddr_control },
> + { 0x010E, BM_COLD | BM_FAST | BM_WARM | BM_S3, prog_dra_drb },
> + { 0x010F, BM_WARM | BM_S3, perform_wake },
> + { 0x0110, BM_COLD | BM_FAST | BM_WARM | BM_S3, change_refresh_period },
> + { 0x0111, BM_COLD | BM_FAST | BM_WARM | BM_S3, set_auto_refresh },
> + { 0x0112, BM_COLD | BM_FAST | BM_WARM | BM_S3, ecc_enable },
> + { 0x0113, BM_COLD | BM_FAST , memory_test },
> + { 0x0114, BM_COLD | BM_FAST | BM_WARM | BM_S3, lock_registers }
What are the hex codes at the start? Ah I see they are post codes (we
don't particularly need them, I'm just asking). Should there be
#defines for these? Also how come they use all 16 bits?
> +};
> +
> +/* Adjust configuration parameters before initialization sequence */
> +static void mrc_adjust_params(struct mrc_params *mrc_params)
> +{
> + const struct dram_params *dram_params;
> + uint8_t dram_width;
> + uint32_t rank_enables;
> + uint32_t channel_width;
> +
> + ENTERFN();
What is this?
> +
> + /* initially expect success */
> + mrc_params->status = MRC_SUCCESS;
> +
> + dram_width = mrc_params->dram_width;
> + rank_enables = mrc_params->rank_enables;
> + channel_width = mrc_params->channel_width;
> +
> + /*
> + * Setup board layout (must be reviewed as is selecting static timings)
> + * 0 == R0 (DDR3 x16), 1 == R1 (DDR3 x16),
> + * 2 == DV (DDR3 x8), 3 == SV (DDR3 x8).
> + */
> + if (dram_width == X8)
> + mrc_params->board_id = 2; /* select x8 layout */
> + else
> + mrc_params->board_id = 0; /* select x16 layout */
> +
> + /* initially no memory */
> + mrc_params->mem_size = 0;
> +
> + /* begin of channel settings */
> + dram_params = &mrc_params->params;
> +
> + /*
> + * Determine Column Bits:
> + *
> + * Column: 11 for 8Gbx8, else 10
> + */
> + mrc_params->column_bits[0] =
> + ((dram_params[0].density == 4) &&
> + (dram_width == X8)) ? (11) : (10);
> +
> + /*
> + * Determine Row Bits:
Can we capitalise only the first word in these comments?
> + *
> + * 512Mbx16=12 512Mbx8=13
> + * 1Gbx16=13 1Gbx8=14
> + * 2Gbx16=14 2Gbx8=15
> + * 4Gbx16=15 4Gbx8=16
> + * 8Gbx16=16 8Gbx8=16
> + */
> + mrc_params->row_bits[0] = 12 + (dram_params[0].density) +
> + (((dram_params[0].density < 4) &&
> + (dram_width == X8)) ? (1) : (0));
> +
> + /*
> + * Determine Per Channel Memory Size:
per-channel
> + *
> + * (For 2 RANKs, multiply by 2)
> + * (For 16 bit data bus, divide by 2)
> + *
> + * DENSITY WIDTH MEM_AVAILABLE
> + * 512Mb x16 0x008000000 ( 128MB)
> + * 512Mb x8 0x010000000 ( 256MB)
> + * 1Gb x16 0x010000000 ( 256MB)
> + * 1Gb x8 0x020000000 ( 512MB)
> + * 2Gb x16 0x020000000 ( 512MB)
> + * 2Gb x8 0x040000000 (1024MB)
> + * 4Gb x16 0x040000000 (1024MB)
> + * 4Gb x8 0x080000000 (2048MB)
> + */
> + mrc_params->channel_size[0] = (1 << dram_params[0].density);
> + mrc_params->channel_size[0] *= (dram_width == X8) ? (2) : (1);
> + mrc_params->channel_size[0] *= (rank_enables == 0x3) ? (2) : (1);
> + mrc_params->channel_size[0] *= (channel_width == X16) ? (1) : (2);
Remove () around 2 and 1.
> +
> + /* Determine memory size (convert number of 64MB/512Mb units) */
> + mrc_params->mem_size += mrc_params->channel_size[0] << 26;
> +
> + LEAVEFN();
?
> +}
> +
> +static void mrc_init(struct mrc_params *mrc_params)
> +{
> + int i;
> +
> + ENTERFN();
> +
> + DPF(D_INFO, "mrc_init build %s %s\n", __DATE__, __TIME__);
debug() I think, and below.
> +
> + /* MRC started */
> + mrc_post_code(0x01, 0x00);
> +
> + if (mrc_params->boot_mode != BM_COLD) {
> + if (mrc_params->ddr_speed != mrc_params->timings.ddr_speed) {
> + /* full training required as frequency changed */
> + mrc_params->boot_mode = BM_COLD;
> + }
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(init); i++) {
> + uint64_t my_tsc;
> +
> + if (mrc_params->boot_mode & init[i].boot_path) {
> + uint8_t major = init[i].post_code >> 8 & 0xFF;
> + uint8_t minor = init[i].post_code >> 0 & 0xFF;
Can we stick with lower case hex, and below?
> + mrc_post_code(major, minor);
> +
> + my_tsc = rdtsc();
> + init[i].init_fn(mrc_params);
> + DPF(D_TIME, "Execution time %llx", rdtsc() - my_tsc);
> + }
> + }
> +
> + /* display the timings */
> + print_timings(mrc_params);
> +
> + /* MRC complete */
> + mrc_post_code(0x01, 0xFF);
> +
> + LEAVEFN();
> +}
> +
> +void mrc(struct mrc_params *mrc_params)
> +{
> + ENTERFN();
> +
> + DPF(D_INFO, "MRC Version %04x %s %s\n",
> + MRC_VERSION, __DATE__, __TIME__);
Can you reformat so more args on first line?
> +
> + /* Set up the data structures used by mrc_init() */
> + mrc_adjust_params(mrc_params);
> +
> + /* Initialize system memory */
> + mrc_init(mrc_params);
> +
> + LEAVEFN();
> +}
> diff --git a/arch/x86/include/asm/arch-quark/mrc.h b/arch/x86/include/asm/arch-quark/mrc.h
> new file mode 100644
> index 0000000..690a800
> --- /dev/null
> +++ b/arch/x86/include/asm/arch-quark/mrc.h
> @@ -0,0 +1,189 @@
> +/*
> + * Copyright (C) 2013, Intel Corporation
> + * Copyright (C) 2015, Bin Meng <bmeng.cn at gmail.com>
> + *
> + * Ported from Intel released Quark UEFI BIOS
> + * QuarkSocPkg/QuarkNorthCluster/MemoryInit/Pei/
> + *
> + * SPDX-License-Identifier: Intel
> + */
> +
> +#ifndef _MRC_H_
> +#define _MRC_H_
> +
> +/* MRC Version */
I think you can drop that comment!
> +#define MRC_VERSION 0x0111
> +
> +/* architectural definitions */
> +#define NUM_CHANNELS 1 /* number of channels */
> +#define NUM_RANKS 2 /* number of ranks per channel */
> +#define NUM_BYTE_LANES 4 /* number of byte lanes per channel */
> +
> +/* software limitations */
> +#define MAX_CHANNELS 1
> +#define MAX_RANKS 2
> +#define MAX_BYTE_LANES 4
> +
> +/* only to mock MrcWrapper */
What does this mean?
> +#define MAX_SOCKETS 1
> +#define MAX_SIDES 1
> +#define MAX_ROWS (MAX_SIDES * MAX_SOCKETS)
> +
> +/* Specify DRAM of nenory channel width */
memory
Also this doesn't quite make sense - can you please reword it?
> +enum {
> + X8, /* DRAM width */
> + X16, /* DRAM width & Channel Width */
> + X32 /* Channel Width */
> +};
> +
> +/* Specify DRAM speed */
> +enum {
> + DDRFREQ_800,
> + DDRFREQ_1066
> +};
> +
> +/* Specify DRAM type */
> +enum {
> + DDR3,
> + DDR3L
> +};
> +
> +/*
> + * density: 0=512Mb, 1=Gb, 2=2Gb, 3=4Gb
should either have @density in this header and all the others here
too. Or move this comment below above density.
> + * cl is DRAM CAS Latency in clocks
> + * All other timings are in picoseconds
> + *
> + * Refer to JEDEC spec (or DRAM datasheet) when changing these values.
> + */
> +struct dram_params {
> + uint8_t density;
> + /* CAS latency in clocks */
> + uint8_t cl;
> + /* ACT to PRE command period */
> + uint32_t ras;
> + /*
> + * Delay from start of internal write transaction to
> + * internal read command
> + */
> + uint32_t wtr;
> + /* ACT to ACT command period (JESD79 specific to page size 1K/2K) */
> + uint32_t rrd;
> + /* Four activate window (JESD79 specific to page size 1K/2K) */
> + uint32_t faw;
> +};
> +
> +/*
> + * Delay configuration for individual signals
> + * Vref setting
> + * Scrambler seed
What do the above two lines mean?
> + */
> +struct mrc_timings {
> + uint32_t rcvn[NUM_CHANNELS][NUM_RANKS][NUM_BYTE_LANES];
> + uint32_t rdqs[NUM_CHANNELS][NUM_RANKS][NUM_BYTE_LANES];
> + uint32_t wdqs[NUM_CHANNELS][NUM_RANKS][NUM_BYTE_LANES];
> + uint32_t wdq[NUM_CHANNELS][NUM_RANKS][NUM_BYTE_LANES];
> + uint32_t vref[NUM_CHANNELS][NUM_BYTE_LANES];
> + uint32_t wctl[NUM_CHANNELS][NUM_RANKS];
> + uint32_t wcmd[NUM_CHANNELS];
> + uint32_t scrambler_seed;
Comments for the above?
> + /* need to save for the case of frequency change */
> + uint8_t ddr_speed;
> +};
> +
> +/* Boot mode defined as bit mask (1<<n) */
> +enum {
> + BM_UNKNOWN,
> + BM_COLD = 1, /* full training */
> + BM_FAST = 2, /* restore timing parameters */
> + BM_S3 = 4, /* resume from S3 */
> + BM_WARM = 8
> +};
> +
> +/* MRC execution status */
> +#define MRC_SUCCESS 0 /* initialization ok */
> +#define MRC_E_MEMTEST 1 /* memtest failed */
> +
> +/* Input/output/context parameters for Memory Reference Code */
> +struct mrc_params {
> + /* Global Settings */
> +
> + /* BM_COLD, BM_FAST, BM_WARM, BM_S3 */
> + uint32_t boot_mode;
> + uint8_t first_run;
> +
> + /* DRAM Parameters */
> +
Remove blank line
> + uint8_t dram_width; /* x8, x16 */
> + uint8_t ddr_speed; /* DDRFREQ_800, DDRFREQ_1066 */
> + uint8_t ddr_type; /* DDR3, DDR3L */
> + uint8_t ecc_enables; /* 0, 1 (memory size reduced to 7/8) */
> + uint8_t scrambling_enables; /* 0, 1 */
> + /* 1, 3 (1'st rank has to be populated if 2'nd rank present) */
> + uint32_t rank_enables;
> + uint32_t channel_enables; /* 1 only */
> + uint32_t channel_width; /* x16 only */
> + /* 0, 1, 2 (mode 2 forced if ecc enabled) */
> + uint32_t address_mode;
> + /* REFRESH_RATE: 1=1.95us, 2=3.9us, 3=7.8us, others=RESERVED */
> + uint8_t refresh_rate;
> + /* SR_TEMP_RANGE: 0=normal, 1=extended, others=RESERVED */
> + uint8_t sr_temp_range;
> + /*
> + * RON_VALUE: 0=34ohm, 1=40ohm, others=RESERVED
> + * (select MRS1.DIC driver impedance control)
> + */
> + uint8_t ron_value;
> + /* RTT_NOM_VALUE: 0=40ohm, 1=60ohm, 2=120ohm, others=RESERVED */
> + uint8_t rtt_nom_value;
> + /* RD_ODT_VALUE: 0=off, 1=60ohm, 2=120ohm, 3=180ohm, others=RESERVED */
> + uint8_t rd_odt_value;
> + struct dram_params params;
> +
> + /* Internally Used */
I think I know what this means? It's unfortunate to have
input/output/working data in the same structure but this seems to be
the approach taken, so let's keep it. But can you add a comment above
the struct saying how it is split into multiple parts?
> +
> + /* internally used for board layout (use x8 or x16 memory) */
> + uint32_t board_id;
> + /* when set hte reconfiguration requested */
> + uint32_t hte_setup:1;
> + uint32_t menu_after_mrc:1;
> + uint32_t power_down_disable:1;
> + uint32_t tune_rcvn:1;
Should these be bool? I'm not sure the :1 helps much - are you trying
to save memory?
> + uint32_t channel_size[NUM_CHANNELS];
> + uint32_t column_bits[NUM_CHANNELS];
> + uint32_t row_bits[NUM_CHANNELS];
> + /* register content saved during training */
> + uint32_t mrs1;
> +
> + /* Output */
> +
> + /* initialization result (non zero specifies error code) */
> + uint32_t status;
> + /* total memory size in bytes (excludes ECC banks) */
> + uint32_t mem_size;
> + /* training results (also used on input) */
> + struct mrc_timings timings;
> +};
> +
This one needs comments:
> +struct mem_init {
> + uint16_t post_code;
> + uint16_t boot_path;
> + void (*init_fn)(struct mrc_params *mrc_params);
> +};
> +
> +/* MRC platform data flags */
> +#define MRC_FLAG_ECC_EN 0x00000001
> +#define MRC_FLAG_SCRAMBLE_EN 0x00000002
> +#define MRC_FLAG_MEMTEST_EN 0x00000004
> +/* 0b DDR "fly-by" topology else 1b DDR "tree" topology */
> +#define MRC_FLAG_TOP_TREE_EN 0x00000008
> +/* If set ODR signal is asserted to DRAM devices on writes */
> +#define MRC_FLAG_WR_ODT_EN 0x00000010
> +
> +/**
> + * mrc - Memory Reference Code entry routine
> + *
> + * @mrc_params: parameters for MRC
> + */
> +void mrc(struct mrc_params *mrc_params);
How about sdram_init() or mrc_init()?
> +
> +#endif /* _MRC_H_ */
> --
> 1.8.2.1
>
Regards,
Simon
More information about the U-Boot
mailing list