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