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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
