[U-Boot] [PATCH RESEND 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller

[Pavel Machek <pavel@denx.de>]

Hi!

> +#ifndef	_SDRAM_H_
> +#define	_SDRAM_H_
> +
> +#ifndef __ASSEMBLY__
> +
> +/* function declaration */

You can delete this comment.

> +#define \
> +SDR_CTRLGRP_MPTHRESHOLDRST_0_THRESHOLDRSTCYCLES_31_0_LSB 0
> +#define  \
> +SDR_CTRLGRP_MPTHRESHOLDRST_0_THRESHOLDRSTCYCLES_31_0_MASK \
> +0xffffffff
> +/* Register template: sdr::ctrlgrp::mpthresholdrst::mpthresholdrst_1       */
> +#define \
> +SDR_CTRLGRP_MPTHRESHOLDRST_1_THRESHOLDRSTCYCLES_63_32_LSB 0
> +#define \
> +SDR_CTRLGRP_MPTHRESHOLDRST_1_THRESHOLDRSTCYCLES_63_32_MASK \
> +0xffffffff
> +/* Register template: sdr::ctrlgrp::mpthresholdrst::mpthresholdrst_2       */
> +#define \
> +SDR_CTRLGRP_MPTHRESHOLDRST_2_THRESHOLDRSTCYCLES_79_64_LSB 0
> +#define \
> +SDR_CTRLGRP_MPTHRESHOLDRST_2_THRESHOLDRSTCYCLES_79_64_MASK \
> +0x0000ffff

Can we get slightly shorter define names?

> +/* Register template: sdr::ctrlgrp::remappriority                          */
> +#define SDR_CTRLGRP_REMAPPRIORITY_PRIORITYREMAP_LSB 0
> +#define SDR_CTRLGRP_REMAPPRIORITY_PRIORITYREMAP_MASK 0x000000ff
> +/* Register template: sdr::ctrlgrp::phyctrl::phyctrl_0                     */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_SAMPLECOUNT_19_0_LSB 12
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_SAMPLECOUNT_19_0_WIDTH 20
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_SAMPLECOUNT_19_0_SET(x) \
> + (((x) << 12) & 0xfffff000)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_ADDLATSEL_SET(x) \
> + (((x) << 10) & 0x00000c00)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_DQSLOGICDELAYEN_SET(x) \
> + (((x) << 6) & 0x000000c0)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_RESETDELAYEN_SET(x) \
> + (((x) << 8) & 0x00000100)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_LPDDRDIS_SET(x) \
> + (((x) << 9) & 0x00000200)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_DQSDELAYEN_SET(x) \
> + (((x) << 4) & 0x00000030)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_DQDELAYEN_SET(x) \
> + (((x) << 2) & 0x0000000c)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_0_ACDELAYEN_SET(x) \
> + (((x) << 0) & 0x00000003)
> +/* Register template: sdr::ctrlgrp::phyctrl::phyctrl_1                     */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_1_LONGIDLESAMPLECOUNT_19_0_WIDTH 20
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_1_LONGIDLESAMPLECOUNT_19_0_SET(x) \
> + (((x) << 12) & 0xfffff000)
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_1_SAMPLECOUNT_31_20_SET(x) \
> + (((x) << 0) & 0x00000fff)
> +/* Register template: sdr::ctrlgrp::phyctrl::phyctrl_2                     */
> +#define SDR_CTRLGRP_PHYCTRL_PHYCTRL_2_LONGIDLESAMPLECOUNT_31_20_SET(x) \
> + (((x) << 0) & 0x00000fff)

Too long names here, too..

> --- /dev/null
> +++ b/arch/arm/include/asm/arch-socfpga/sdram_config.h
> @@ -0,0 +1,100 @@
> +/*
> + * Copyright Altera Corporation (C) 2012-2015
> + *
> + * SPDX-License-Identifier:    BSD-3-Clause
> + */
> +

If this file is autogenerated, you should mention it here.

> +#ifdef CONFIG_SOCFPGA_ARRIA5
> +/* The if..else... is not required if generated by tools */

What does this comment say?

> +#define CONFIG_HPS_SDR_CTRLCFG_MPPACING_1_THRESHOLD2_3_0	0
> +#define CONFIG_HPS_SDR_CTRLCFG_MPPACING_2_THRESHOLD2_35_4	0x41041041
> +#define CONFIG_HPS_SDR_CTRLCFG_MPPACING_3_THRESHOLD2_59_36	0x410410
> +#define CONFIG_HPS_SDR_CTRLCFG_MPTHRESHOLDRST_0_THRESHOLDRSTCYCLES_31_0 \
> +0x01010101
> +#define CONFIG_HPS_SDR_CTRLCFG_MPTHRESHOLDRST_1_THRESHOLDRSTCYCLES_63_32 \
> +0x01010101
> +#define CONFIG_HPS_SDR_CTRLCFG_MPTHRESHOLDRST_2_THRESHOLDRSTCYCLES_79_64 \
> +0x0101

Drop "HPS" and "CTRLCFG" from the config names... they should still be
unique and you'll not hit 80 column limits with just the name?

> +#define COMPARE_FAIL_ACTION	return 1;

Macros that change control flow are nasty.

> +/* Below function only applicable for SPL */

"Function below"?

Add ifdef so that it is not available for u-boot proper?

> +typedef struct _sdram_prot_rule {
> +	uint64_t	sdram_start;	/* SDRAM start address */
> +	uint64_t	sdram_end;	/* SDRAM end address */
> +	uint32_t	rule;		/* SDRAM protection rule number: 0-19 */
> +	int		valid;		/* Rule valid or not? 1 - valid, 0 not*/
> +
> +	uint32_t	security;
> +	uint32_t	portmask;
> +	uint32_t	result;
> +	uint32_t	lo_prot_id;
> +	uint32_t	hi_prot_id;
> +} sdram_prot_rule, *psdram_prot_rule;

Struct typedefs are nasty. Just use "struct sdram_prot_rule"?

> +static void sdram_get_rule(psdram_prot_rule prule)
> +{
> +	uint32_t protruleaddr;
> +	uint32_t protruleid;
> +	uint32_t protruledata;

Remove "protrule" from local variables, as it is clear from context?

> +static void sdram_set_protection_config(uint64_t sdram_start, uint64_t sdram_end)
> +{
> +	sdram_prot_rule rule;
> +	int rules;
> +
> +	/* Start with accepting all SDRAM transaction */
> +	writel(0x0, &sdr_ctrl->protport_default);
> +
> +	/* Clear all protection rules for warm boot case */
> +
> +	rule.sdram_start = 0;

Kill last empty line. And actually... maybe memset?

> +static void set_sdr_addr_rw(void)
> +{
> +	int cs = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS;
> +	int width = 8;
> +	int rows = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS;
> +	int banks = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS;
> +	int cols = CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS;
> +	unsigned long long workaround_memsize = MEMSIZE_4G;
> +
> +	debug("Configuring DRAMADDRW\n");
> +	clrsetbits_le32(&sdr_ctrl->dram_addrw, SDR_CTRLGRP_DRAMADDRW_COLBITS_MASK,
> +			CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS <<
> +			SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB);
> +	/* SDRAM Failure When Accessing Non-Existent Memory
> +	 * Update Preloader to artificially increase the number of rows so
> +	 * that the memory thinks it has 4GB of RAM.
> +	 */

Comment style, "rows, so"?


> +/* To calculate SDRAM device size based on SDRAM controller parameters.

Drop "To".

> + * Size is specified in bytes.
> + *
> + * NOTE!!!!
> + * This function is compiled and linked into the preloader and
> + * Uboot (there may be others). So if this function changes, the Preloader
> + * and UBoot must be updated simultaneously.
> + */

Is that worth big note and four exclamation marks? Compiler should
take care of recompilation...

Ok, this starts to look like something that we could actually merge.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html