Re: [U-Boot] [PATCHv4 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller
Hi! Comment what kind of errata this is working around? I'll have to ask around. It is to workaround the computational of SDRAM rows. The info is then used to calculate the SDRAM size. By doing this, we can remove from hardcoding the SDRAM size into the code. More info at https://github.com/altera-opensource/u-boot-socfpga/commit/93815696dce132ff8abc4ab2f4c195339ff821a0. Hope this explains. Ok, can you add a comment into the sources explaining what is going on? Normally, chip errata are documented somewhere and have numbers, that would be best thing to link there. (But the github link is also better than nothing...) Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv4 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller
Hi,
On Tue, 2015-06-09 at 10:51 -0500, Dinh Nguyen wrote:
On 6/9/15 6:55 AM, Pavel Machek wrote:
Hi!
+struct sdram_prot_rule {
+ uint64_tsdram_start; /* SDRAM start address */
+ uint64_tsdram_end; /* SDRAM end address */
+ uint32_trule; /* SDRAM protection rule number: 0-19 */
+ int valid; /* Rule valid or not? 1 - valid, 0 not*/
There should be space before */.
Ok...
diff --git a/arch/arm/include/asm/arch-socfpga/sdram_config.h
b/arch/arm/include/asm/arch-socfpga/sdram_config.h
new file mode 100644
index 000..f6d51ca
--- /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
+ */
+
+/* This file is autogenerated from tools provided by Altera.*/
Here too.
Ok...
+#endif/*#ifndef__SDRAM_CONFIG_H*/
You should not need to comment for include guards... (and comment
style).
+static int compute_errata_rows(unsigned long long memsize, int cs, int
width,
+ int rows, int banks, int cols)
+{
Comment what kind of errata this is working around?
I'll have to ask around.
It is to workaround the computational of SDRAM rows. The info is then
used to calculate the SDRAM size. By doing this, we can remove from
hardcoding the SDRAM size into the code. More info at
https://github.com/altera-opensource/u-boot-socfpga/commit/93815696dce132ff8abc4ab2f4c195339ff821a0.
Hope this explains.
+#if defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS)
+
Hmm? Is this really neccessary? Is it valid to provide configuration
w/o those defines?
These defines are necessary as I want to keep some level of continuity
with the Altera tools that generates these config files to this.
+ writel(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS,
+ sysmgr_regs-iswgrp_handoff[4]);
+#endif
+
+ /* Restore the SDR PHY Register if valid */
+ if (sdr_phy_reg != 0x)
+ writel(sdr_phy_reg, sdr_ctrl-phy_ctrl0);
+
+/* Final step - apply configuration changes */
Comment style...
Ok..
+/*
+ * To calculate SDRAM device size based on SDRAM controller parameters.
+ * 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.
+ */
+unsigned long sdram_calculate_size(void)
+{
+ unsigned long temp;
+ unsigned long row, bank, col, cs, width;
+
+ temp = readl(sdr_ctrl-dram_addrw);
+ col = (temp SDR_CTRLGRP_DRAMADDRW_COLBITS_MASK)
+ SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB;
+
+ /* SDRAM Failure When Accessing Non-Existent Memory
+ * Use ROWBITS from Quartus/QSys to calculate SDRAM size
+ * since the FB specifies we modify ROWBITs to work around SDRAM
+ * controller issue.
+ *
+ * If the stored handoff value for rows is 0, it probably means
+ * the preloader is older than UBoot. Use the
+ * #define from the SOCEDS Tools per Crucible review
+ * uboot-socfpga-204. Note that this is not a supported
+ * configuration and is not tested. The customer
+ * should be using preloader and uboot built from the
+ * same tag.
+ */
U-Boot is normally spelled U-Boot. You have two different variants
in comments here.
Thanks for the comment here, and will be more cognizant in the future on
this fact.
Second part of the comment is probably not relevant any more?
removed...
Acked-by: Pavel Machek pa...@denx.de
Pavel
Thanks,
Dinh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv4 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller
Hi!
+struct sdram_prot_rule {
+ uint64_tsdram_start; /* SDRAM start address */
+ uint64_tsdram_end; /* SDRAM end address */
+ uint32_trule; /* SDRAM protection rule number: 0-19 */
+ int valid; /* Rule valid or not? 1 - valid, 0 not*/
There should be space before */.
diff --git a/arch/arm/include/asm/arch-socfpga/sdram_config.h
b/arch/arm/include/asm/arch-socfpga/sdram_config.h
new file mode 100644
index 000..f6d51ca
--- /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
+ */
+
+/* This file is autogenerated from tools provided by Altera.*/
Here too.
+#endif /*#ifndef__SDRAM_CONFIG_H*/
You should not need to comment for include guards... (and comment
style).
+static int compute_errata_rows(unsigned long long memsize, int cs, int width,
+int rows, int banks, int cols)
+{
Comment what kind of errata this is working around?
+#if defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS)
+
Hmm? Is this really neccessary? Is it valid to provide configuration
w/o those defines?
+ writel(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS,
+sysmgr_regs-iswgrp_handoff[4]);
+#endif
+
+ /* Restore the SDR PHY Register if valid */
+ if (sdr_phy_reg != 0x)
+ writel(sdr_phy_reg, sdr_ctrl-phy_ctrl0);
+
+/* Final step - apply configuration changes */
Comment style...
+/*
+ * To calculate SDRAM device size based on SDRAM controller parameters.
+ * 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.
+ */
+unsigned long sdram_calculate_size(void)
+{
+ unsigned long temp;
+ unsigned long row, bank, col, cs, width;
+
+ temp = readl(sdr_ctrl-dram_addrw);
+ col = (temp SDR_CTRLGRP_DRAMADDRW_COLBITS_MASK)
+ SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB;
+
+ /* SDRAM Failure When Accessing Non-Existent Memory
+ * Use ROWBITS from Quartus/QSys to calculate SDRAM size
+ * since the FB specifies we modify ROWBITs to work around SDRAM
+ * controller issue.
+ *
+ * If the stored handoff value for rows is 0, it probably means
+ * the preloader is older than UBoot. Use the
+ * #define from the SOCEDS Tools per Crucible review
+ * uboot-socfpga-204. Note that this is not a supported
+ * configuration and is not tested. The customer
+ * should be using preloader and uboot built from the
+ * same tag.
+ */
U-Boot is normally spelled U-Boot. You have two different variants
in comments here.
Second part of the comment is probably not relevant any more?
Acked-by: Pavel Machek pa...@denx.de
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv4 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller
On 6/9/15 6:55 AM, Pavel Machek wrote:
Hi!
+struct sdram_prot_rule {
+uint64_tsdram_start; /* SDRAM start address */
+uint64_tsdram_end; /* SDRAM end address */
+uint32_trule; /* SDRAM protection rule number: 0-19 */
+int valid; /* Rule valid or not? 1 - valid, 0 not*/
There should be space before */.
Ok...
diff --git a/arch/arm/include/asm/arch-socfpga/sdram_config.h
b/arch/arm/include/asm/arch-socfpga/sdram_config.h
new file mode 100644
index 000..f6d51ca
--- /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
+ */
+
+/* This file is autogenerated from tools provided by Altera.*/
Here too.
Ok...
+#endif /*#ifndef__SDRAM_CONFIG_H*/
You should not need to comment for include guards... (and comment
style).
+static int compute_errata_rows(unsigned long long memsize, int cs, int
width,
+ int rows, int banks, int cols)
+{
Comment what kind of errata this is working around?
I'll have to ask around.
+#if defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_CSBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_BANKBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_COLBITS) \
+defined(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS)
+
Hmm? Is this really neccessary? Is it valid to provide configuration
w/o those defines?
These defines are necessary as I want to keep some level of continuity
with the Altera tools that generates these config files to this.
+writel(CONFIG_HPS_SDR_CTRLCFG_DRAMADDRW_ROWBITS,
+ sysmgr_regs-iswgrp_handoff[4]);
+#endif
+
+/* Restore the SDR PHY Register if valid */
+if (sdr_phy_reg != 0x)
+writel(sdr_phy_reg, sdr_ctrl-phy_ctrl0);
+
+/* Final step - apply configuration changes */
Comment style...
Ok..
+/*
+ * To calculate SDRAM device size based on SDRAM controller parameters.
+ * 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.
+ */
+unsigned long sdram_calculate_size(void)
+{
+unsigned long temp;
+unsigned long row, bank, col, cs, width;
+
+temp = readl(sdr_ctrl-dram_addrw);
+col = (temp SDR_CTRLGRP_DRAMADDRW_COLBITS_MASK)
+SDR_CTRLGRP_DRAMADDRW_COLBITS_LSB;
+
+/* SDRAM Failure When Accessing Non-Existent Memory
+ * Use ROWBITS from Quartus/QSys to calculate SDRAM size
+ * since the FB specifies we modify ROWBITs to work around SDRAM
+ * controller issue.
+ *
+ * If the stored handoff value for rows is 0, it probably means
+ * the preloader is older than UBoot. Use the
+ * #define from the SOCEDS Tools per Crucible review
+ * uboot-socfpga-204. Note that this is not a supported
+ * configuration and is not tested. The customer
+ * should be using preloader and uboot built from the
+ * same tag.
+ */
U-Boot is normally spelled U-Boot. You have two different variants
in comments here.
Thanks for the comment here, and will be more cognizant in the future on
this fact.
Second part of the comment is probably not relevant any more?
removed...
Acked-by: Pavel Machek pa...@denx.de
Pavel
Thanks,
Dinh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCHv4 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller
Dear Pavel, In message 20150609115532.GA29408@amd you wrote: U-Boot is normally spelled U-Boot. You have two different variants in comments here. Thanks for pointing out. This is actually quite important as U-Boot is _not_ a copyrighted name, while some other spellings are (search for text u-boot at [1]). For example, uboot is a registered trade mark of the uboot.com mobile internet services gmbh in Vienna (who actively pursuit violations). [1] http://www.wipo.int/branddb/en/ Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Given a choice between two theories, take the one which is funnier. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCHv4 1/3] driver/ddr/altera: Add DDR driver for Altera's SDRAM controller
From: Dinh Nguyen dingu...@opensource.altera.com
This patch enables the SDRAM controller that is used on Altera's SoCFPGA
family. This patch configures the SDRAM controller based on a configuration
file that is generated from the Quartus tool, sdram_config.h.
Signed-off-by: Dinh Nguyen dingu...@opensource.altera.com
---
v4: none
v3: none
v2: clean up from comments from Pavel
---
Makefile | 1 +
arch/arm/include/asm/arch-socfpga/sdram.h| 306 +
arch/arm/include/asm/arch-socfpga/sdram_config.h | 100 +++
drivers/ddr/altera/sdram.c | 799 +++
scripts/Makefile.spl | 1 +
5 files changed, 1207 insertions(+)
create mode 100644 arch/arm/include/asm/arch-socfpga/sdram.h
create mode 100644 arch/arm/include/asm/arch-socfpga/sdram_config.h
create mode 100644 drivers/ddr/altera/sdram.c
diff --git a/Makefile b/Makefile
index 0f7d583..163d530 100644
--- a/Makefile
+++ b/Makefile
@@ -649,6 +649,7 @@ libs-y += drivers/power/ \
libs-y += drivers/spi/
libs-$(CONFIG_FMAN_ENET) += drivers/net/fm/
libs-$(CONFIG_SYS_FSL_DDR) += drivers/ddr/fsl/
+libs-$(CONFIG_ALTERA_SDRAM) += drivers/ddr/altera/
libs-y += drivers/serial/
libs-y += drivers/usb/dwc3/
libs-y += drivers/usb/eth/
diff --git a/arch/arm/include/asm/arch-socfpga/sdram.h
b/arch/arm/include/asm/arch-socfpga/sdram.h
new file mode 100644
index 000..b4c1a2f
--- /dev/null
+++ b/arch/arm/include/asm/arch-socfpga/sdram.h
@@ -0,0 +1,306 @@
+/*
+ * Copyright Altera Corporation (C) 2014-2015
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+#ifndef_SDRAM_H_
+#define_SDRAM_H_
+
+#ifndef __ASSEMBLY__
+
+unsigned long sdram_calculate_size(void);
+unsigned sdram_mmr_init_full(unsigned int sdr_phy_reg);
+int sdram_calibration_full(void);
+
+extern int sdram_calibration(void);
+
+#define SDR_CTRLGRP_ADDRESS 0x5000
+
+struct socfpga_sdr_ctrl {
+ u32 ctrl_cfg;
+ u32 dram_timing1;
+ u32 dram_timing2;
+ u32 dram_timing3;
+ u32 dram_timing4; /* 0x10 */
+ u32 lowpwr_timing;
+ u32 dram_odt;
+ u32 __padding0[4];
+ u32 dram_addrw; /* 0x2c */
+ u32 dram_if_width; /* 0x30 */
+ u32 dram_dev_width;
+ u32 dram_sts;
+ u32 dram_intr;
+ u32 sbe_count; /* 0x40 */
+ u32 dbe_count;
+ u32 err_addr;
+ u32 drop_count;
+ u32 drop_addr; /* 0x50 */
+ u32 lowpwr_eq;
+ u32 lowpwr_ack;
+ u32 static_cfg;
+ u32 ctrl_width; /* 0x60 */
+ u32 cport_width;
+ u32 cport_wmap;
+ u32 cport_rmap;
+ u32 rfifo_cmap; /* 0x70 */
+ u32 wfifo_cmap;
+ u32 cport_rdwr;
+ u32 port_cfg;
+ u32 fpgaport_rst; /* 0x80 */
+ u32 __padding1;
+ u32 fifo_cfg;
+ u32 protport_default;
+ u32 prot_rule_addr; /* 0x90 */
+ u32 prot_rule_id;
+ u32 prot_rule_data;
+ u32 prot_rule_rdwr;
+ u32 __padding2[3];
+ u32 mp_priority;/* 0xac */
+ u32 mp_weight0; /* 0xb0 */
+ u32 mp_weight1;
+ u32 mp_weight2;
+ u32 mp_weight3;
+ u32 mp_pacing0; /* 0xc0 */
+ u32 mp_pacing1;
+ u32 mp_pacing2;
+ u32 mp_pacing3;
+ u32 mp_threshold0; /* 0xd0 */
+ u32 mp_threshold1;
+ u32 mp_threshold2;
+ u32 __padding3[29];
+ u32 phy_ctrl0; /* 0x150 */
+ u32 phy_ctrl1;
+ u32 phy_ctrl2;
+};
+
+struct sdram_prot_rule {
+ uint64_tsdram_start; /* SDRAM start address */
+ uint64_tsdram_end; /* SDRAM end address */
+ uint32_trule; /* SDRAM protection rule number: 0-19 */
+ int valid; /* Rule valid or not? 1 - valid, 0 not*/
+
+ uint32_tsecurity;
+ uint32_tportmask;
+ uint32_tresult;
+ uint32_tlo_prot_id;
+ uint32_thi_prot_id;
+};
+
+#define SDR_CTRLGRP_CTRLCFG_NODMPINS_LSB 23
+#define SDR_CTRLGRP_CTRLCFG_NODMPINS_MASK 0x0080
+#define SDR_CTRLGRP_CTRLCFG_DQSTRKEN_LSB 22
+#define SDR_CTRLGRP_CTRLCFG_DQSTRKEN_MASK 0x0040
+#define SDR_CTRLGRP_CTRLCFG_STARVELIMIT_LSB 16
+#define SDR_CTRLGRP_CTRLCFG_STARVELIMIT_MASK 0x003f
+#define SDR_CTRLGRP_CTRLCFG_REORDEREN_LSB 15
+#define SDR_CTRLGRP_CTRLCFG_REORDEREN_MASK 0x8000
+#define SDR_CTRLGRP_CTRLCFG_ECCCORREN_LSB 11
+#define SDR_CTRLGRP_CTRLCFG_ECCCORREN_MASK 0x0800
+#define SDR_CTRLGRP_CTRLCFG_ECCEN_LSB 10
+#define SDR_CTRLGRP_CTRLCFG_ECCEN_MASK 0x0400
+#define SDR_CTRLGRP_CTRLCFG_ADDRORDER_LSB 8
+#define SDR_CTRLGRP_CTRLCFG_ADDRORDER_MASK 0x0300
+#define SDR_CTRLGRP_CTRLCFG_MEMBL_LSB 3
+#define SDR_CTRLGRP_CTRLCFG_MEMBL_MASK 0x00f8
+#define
