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

[Dinh Nguyen <dinguyen@opensource.altera.com>]

Hi Pavel,

On 04/17/2015 07:31 AM, Pavel Machek wrote:
> Hi!
> 
>> +#ifndef	_SDRAM_H_
>> +#define	_SDRAM_H_
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +/* function declaration */
> 
> You can delete this comment.
> 

Ok...

>> +#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?

I did think about shortening these defines a bit, but came to this
reason that I should leave these alone. These defines are generated from
the tools AFAICT. I don't think any sane person would try to have
defines this long. So I still want to try to save the use case that the
driver can still be used with the autogenerated header file from the
tools in some form.

> 
>> +/* 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.
> 

Ok...

>> +#ifdef CONFIG_SOCFPGA_ARRIA5
>> +/* The if..else... is not required if generated by tools */
> 
> What does this comment say?
> 

I have no idea, but will clean up.

>> +#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?
> 

Same reasoning as I had for previous comment regarding long define names.

>> +#define COMPARE_FAIL_ACTION	return 1;
> 
> Macros that change control flow are nasty.
> 

Will remove...

>> +/* Below function only applicable for SPL */
> 
> "Function below"?

Will clean..

> 
> 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"?

Yeah...will clean up...

> 
>> +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?
> 

Ok...

>> +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?

Ok...

> 
>> +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"?
> 

Will clean up...

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

Ok...

>> + * 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...

Yeah, I'll clean up...

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

Almost...

thanks,
Dinh