[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