Hi Simon,
On Thu, Feb 5, 2015 at 12:24 AM, Simon Glass <s...@chromium.org> wrote:
> Hi Bin,
>
> On 3 February 2015 at 04:45, Bin Meng <bmeng...@gmail.com> wrote:
>> Add the main routines for Quark Memory Reference Code (MRC).
>>
>> Signed-off-by: Bin Meng <bmeng...@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...@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
Fixed
>> + *
>> + * 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?
Fixed. DRAM unit.
>> + */
>> +
>> +#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?
>
Looks Intel chose random numbers for these post codes. Changing them
to #define does not seem to work as I don't know how to name them. So
I keep these unchanged in v2.
>> +};
>> +
>> +/* 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?
Debug output for tracking function call.
>> +
>> + /* 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?
Fixed.
>> + *
>> + * 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
Fixed.
>> + *
>> + * (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.
Fixed
>> +
>> + /* Determine memory size (convert number of 64MB/512Mb units) */
>> + mrc_params->mem_size += mrc_params->channel_size[0] << 26;
>> +
>> + LEAVEFN();
>
> ?
Debug output for tracking function return.
>> +}
>> +
>> +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.
DPF is the MRC debug routine, and I wanted to keep using it for MRC
codes. And in v2, I removed this line completely.
>> +
>> + /* 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?
Fixed.
>> + 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);
>> +
Fixed, using lower case hex.
>> + 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?
Fixed.
>> +
>> + /* 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...@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!
Fixed.
>> +#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?
I don't know, just removed this line in v2.
>> +#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?
Changed the comment to: /* Specify DRAM and channel width */ in v2.
>> +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.
Fixed.
>> + * 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?
I think this is DDR technology term. I did not change this in v2.
>> + */
>> +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?
Again too DDR-specific terms. I did not add any comment to this.
>> + /* 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
Fixed.
>> + 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?
Fixed.
>> +
>> + /* 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?
I just removed the :1 in the v2.
>> + 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:
Fixed.
>> +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()?
Changed it to mrc_init() in v2.
>> +
>> +#endif /* _MRC_H_ */
>> --
>> 1.8.2.1
>>
>
> Regards,
> Simon
Regards,
Bin
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
