[PATCH 0/7] ddr: altera: arria10: Convert SDRAM driver to DM
This patchset mainly to covert Arria 10 SDRAM driver to device model and fixes few bugs in driver. It also added RAM size check function to check valid RAM.
Ley Foon Tan (7): ddr: altera: arria10: Fix incorrect address for mpu1 ddr: altera: arria10: Move SDRAM driver to DM ddr: altera: arria10: Change to use reset DM function arm: socfpga: arria10: Move sdram_arria10.h to drivers/ddr/altera ddr: altera: arria10: Add RAM size check ddr: altera: arria10: Change %i to %u for printf ddr: altera: arria10: Remove call to dram_init_banksize()
arch/arm/dts/socfpga_arria10-u-boot.dtsi | 8 + .../include/mach/reset_manager_arria10.h | 1 - arch/arm/mach-socfpga/include/mach/sdram.h | 2 +- arch/arm/mach-socfpga/misc_arria10.c | 2 +- arch/arm/mach-socfpga/reset_manager_arria10.c | 7 - arch/arm/mach-socfpga/spl_a10.c | 14 +- drivers/ddr/altera/Kconfig | 4 +- drivers/ddr/altera/sdram_arria10.c | 387 ++++++++++-------- .../ddr/altera}/sdram_arria10.h | 244 +++-------- 9 files changed, 309 insertions(+), 360 deletions(-) rename {arch/arm/mach-socfpga/include/mach => drivers/ddr/altera}/sdram_arria10.h (72%)
Remove extra "SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS" in mpu1 address.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com --- drivers/ddr/altera/sdram_arria10.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index 2fd50b7ae550..e0779b810fdc 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -470,7 +470,6 @@ const struct firewall_entry firewall_table[] = { }, { "mpu1", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(mpuregion1addr), SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), ALT_NOC_FW_DDR_SCR_EN_MPUREG1EN_SET_MSK
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Remove extra "SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS" in mpu1 address.
I see what the patch does from the patch itself, but the commit message does not explain _why_ the patch does it. Can you add it ?
[...]
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, April 15, 2020 8:38 PM To: Tan, Ley Foon ley.foon.tan@intel.com; u-boot@lists.denx.de Cc: Ley Foon Tan lftan.linux@gmail.com; See, Chin Liang chin.liang.see@intel.com; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; Chee, Tien Fong tien.fong.chee@intel.com Subject: Re: [PATCH 1/7] ddr: altera: arria10: Fix incorrect address for mpu1
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Remove extra "SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS" in mpu1
address.
I see what the patch does from the patch itself, but the commit message does not explain _why_ the patch does it. Can you add it ?
[...]
Okay.
Regards Ley Foon
Convert Arria 10 SDRAM driver to device model.
SPL is changed from calling function in SDRAM driver directly to just probing UCLASS_RAM.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com --- arch/arm/dts/socfpga_arria10-u-boot.dtsi | 8 + .../mach-socfpga/include/mach/sdram_arria10.h | 244 ++++--------- arch/arm/mach-socfpga/spl_a10.c | 14 +- drivers/ddr/altera/Kconfig | 4 +- drivers/ddr/altera/sdram_arria10.c | 334 ++++++++++-------- 5 files changed, 270 insertions(+), 334 deletions(-)
diff --git a/arch/arm/dts/socfpga_arria10-u-boot.dtsi b/arch/arm/dts/socfpga_arria10-u-boot.dtsi index 6ff1ea6e5eb7..cb576f5288ea 100644 --- a/arch/arm/dts/socfpga_arria10-u-boot.dtsi +++ b/arch/arm/dts/socfpga_arria10-u-boot.dtsi @@ -133,6 +133,14 @@ u-boot,dm-pre-reloc; };
+&sdr { + reg = <0xffcfa000 0x11C>, + <0xffcfb000 0x180>, + <0xffd12400 0x1030>; + resets = <&rst DDRSCH_RESET>; + u-boot,dm-pre-reloc; +}; + &sysmgr { u-boot,dm-pre-reloc; }; diff --git a/arch/arm/mach-socfpga/include/mach/sdram_arria10.h b/arch/arm/mach-socfpga/include/mach/sdram_arria10.h index 25b82fb28583..71889ecde9f7 100644 --- a/arch/arm/mach-socfpga/include/mach/sdram_arria10.h +++ b/arch/arm/mach-socfpga/include/mach/sdram_arria10.h @@ -6,76 +6,65 @@ #ifndef _SOCFPGA_SDRAM_ARRIA10_H_ #define _SOCFPGA_SDRAM_ARRIA10_H_
-#ifndef __ASSEMBLY__ -int ddr_calibration_sequence(void); - -struct socfpga_ecc_hmc { - u32 ip_rev_id; - u32 _pad_0x4_0x7; - u32 ddrioctrl; - u32 ddrcalstat; - u32 mpr_0beat1; - u32 mpr_1beat1; - u32 mpr_2beat1; - u32 mpr_3beat1; - u32 mpr_4beat1; - u32 mpr_5beat1; - u32 mpr_6beat1; - u32 mpr_7beat1; - u32 mpr_8beat1; - u32 mpr_0beat2; - u32 mpr_1beat2; - u32 mpr_2beat2; - u32 mpr_3beat2; - u32 mpr_4beat2; - u32 mpr_5beat2; - u32 mpr_6beat2; - u32 mpr_7beat2; - u32 mpr_8beat2; - u32 _pad_0x58_0x5f[2]; - u32 auto_precharge; - u32 _pad_0x64_0xff[39]; - u32 eccctrl; - u32 eccctrl2; - u32 _pad_0x108_0x10f[2]; - u32 errinten; - u32 errintens; - u32 errintenr; - u32 intmode; - u32 intstat; - u32 diaginttest; - u32 modstat; - u32 derraddra; - u32 serraddra; - u32 _pad_0x134_0x137; - u32 autowb_corraddr; - u32 serrcntreg; - u32 autowb_drop_cntreg; - u32 _pad_0x144_0x147; - u32 ecc_reg2wreccdatabus; - u32 ecc_rdeccdata2regbus; - u32 ecc_reg2rdeccdatabus; - u32 _pad_0x154_0x15f[3]; - u32 ecc_diagon; - u32 ecc_decstat; - u32 _pad_0x168_0x16f[2]; - u32 ecc_errgenaddr_0; - u32 ecc_errgenaddr_1; - u32 ecc_errgenaddr_2; - u32 ecc_errgenaddr_3; -}; - -struct socfpga_noc_ddr_scheduler { - u32 ddr_t_main_scheduler_id_coreid; - u32 ddr_t_main_scheduler_id_revisionid; - u32 ddr_t_main_scheduler_ddrconf; - u32 ddr_t_main_scheduler_ddrtiming; - u32 ddr_t_main_scheduler_ddrmode; - u32 ddr_t_main_scheduler_readlatency; - u32 _pad_0x20_0x34[8]; - u32 ddr_t_main_scheduler_activate; - u32 ddr_t_main_scheduler_devtodev; -}; +/* IO48 HMC */ +#define CTRLCFG0 0x28 +#define CTRLCFG1 0x2c +#define DRAMTIMING0 0x50 +#define CALTIMING0 0x7c +#define CALTIMING1 0x80 +#define CALTIMING2 0x84 +#define CALTIMING3 0x88 +#define CALTIMING4 0x8c +#define CALTIMING9 0xa0 +#define DRAMADDRW 0xa8 +#define DRAMSTS 0xec +#define NIOSRESERVE0 0x110 +#define NIOSRESERVE1 0x114 +#define NIOSRESERVE2 0x118 + +/* HMC */ +#define DDRIOCTRL 0x8 +#define DDRCALSTAT 0xc +#define ECCCTRL1 0x100 +#define ECCCTRL2 0x104 + +/* DDR scheduler */ +#define DDRCONF 0x08 +#define DDRTIMING 0x0c +#define DDRMODE 0x10 +#define DDRREADLATENCY 0x14 +#define DDRACTIVATE 0x38 +#define DDRDEVTODEV 0x3c + +/* FW DDR MPU F2S SCR */ +#define FW_DDR_MPU_F2S_SCR_EN 0x00 +#define FW_DDR_MPU_F2S_SCR_MPUR0 0x10 +#define FW_DDR_MPU_F2S_SCR_MPUR1 0x14 +#define FW_DDR_MPU_F2S_SCR_MPUR2 0x18 +#define FW_DDR_MPU_F2S_SCR_MPUR3 0x1c +#define FW_DDR_MPU_F2S_SCR_F2S0_R0 0x20 +#define FW_DDR_MPU_F2S_SCR_F2S0_R1 0x24 +#define FW_DDR_MPU_F2S_SCR_F2S0_R2 0x28 +#define FW_DDR_MPU_F2S_SCR_F2S0_R3 0x2c +#define FW_DDR_MPU_F2S_SCR_F2S1_R0 0x30 +#define FW_DDR_MPU_F2S_SCR_F2S1_R1 0x34 +#define FW_DDR_MPU_F2S_SCR_F2S1_R2 0x38 +#define FW_DDR_MPU_F2S_SCR_F2S1_R3 0x3c +#define FW_DDR_MPU_F2S_SCR_F2S2_R0 0x40 +#define FW_DDR_MPU_F2S_SCR_F2S2_R1 0x44 +#define FW_DDR_MPU_F2S_SCR_F2S2_R2 0x48 +#define FW_DDR_MPU_F2S_SCR_F2S2_R3 0x4c + +/* FW DDR L3 DDR SCR */ +#define FW_DDR_L3_SCR_EN 0x00 +#define FW_DDR_L3_SCR_HPS_R0_ADDR 0x0c +#define FW_DDR_L3_SCR_HPS_R1_ADDR 0x10 +#define FW_DDR_L3_SCR_HPS_R2_ADDR 0x14 +#define FW_DDR_L3_SCR_HPS_R3_ADDR 0x18 +#define FW_DDR_L3_SCR_HPS_R4_ADDR 0x1c +#define FW_DDR_L3_SCR_HPS_R5_ADDR 0x20 +#define FW_DDR_L3_SCR_HPS_R6_ADDR 0x24 +#define FW_DDR_L3_SCR_HPS_R7_ADDR 0x28
/* * OCRAM firewall @@ -92,121 +81,6 @@ struct socfpga_noc_fw_ocram { u32 region5; };
-/* for master such as MPU and FPGA */ -struct socfpga_noc_fw_ddr_mpu_fpga2sdram { - u32 enable; - u32 enable_set; - u32 enable_clear; - u32 _pad_0xc_0xf; - u32 mpuregion0addr; - u32 mpuregion1addr; - u32 mpuregion2addr; - u32 mpuregion3addr; - u32 fpga2sdram0region0addr; - u32 fpga2sdram0region1addr; - u32 fpga2sdram0region2addr; - u32 fpga2sdram0region3addr; - u32 fpga2sdram1region0addr; - u32 fpga2sdram1region1addr; - u32 fpga2sdram1region2addr; - u32 fpga2sdram1region3addr; - u32 fpga2sdram2region0addr; - u32 fpga2sdram2region1addr; - u32 fpga2sdram2region2addr; - u32 fpga2sdram2region3addr; -}; - -/* for L3 master */ -struct socfpga_noc_fw_ddr_l3 { - u32 enable; - u32 enable_set; - u32 enable_clear; - u32 hpsregion0addr; - u32 hpsregion1addr; - u32 hpsregion2addr; - u32 hpsregion3addr; - u32 hpsregion4addr; - u32 hpsregion5addr; - u32 hpsregion6addr; - u32 hpsregion7addr; -}; - -struct socfpga_io48_mmr { - u32 dbgcfg0; - u32 dbgcfg1; - u32 dbgcfg2; - u32 dbgcfg3; - u32 dbgcfg4; - u32 dbgcfg5; - u32 dbgcfg6; - u32 reserve0; - u32 reserve1; - u32 reserve2; - u32 ctrlcfg0; - u32 ctrlcfg1; - u32 ctrlcfg2; - u32 ctrlcfg3; - u32 ctrlcfg4; - u32 ctrlcfg5; - u32 ctrlcfg6; - u32 ctrlcfg7; - u32 ctrlcfg8; - u32 ctrlcfg9; - u32 dramtiming0; - u32 dramodt0; - u32 dramodt1; - u32 sbcfg0; - u32 sbcfg1; - u32 sbcfg2; - u32 sbcfg3; - u32 sbcfg4; - u32 sbcfg5; - u32 sbcfg6; - u32 sbcfg7; - u32 caltiming0; - u32 caltiming1; - u32 caltiming2; - u32 caltiming3; - u32 caltiming4; - u32 caltiming5; - u32 caltiming6; - u32 caltiming7; - u32 caltiming8; - u32 caltiming9; - u32 caltiming10; - u32 dramaddrw; - u32 sideband0; - u32 sideband1; - u32 sideband2; - u32 sideband3; - u32 sideband4; - u32 sideband5; - u32 sideband6; - u32 sideband7; - u32 sideband8; - u32 sideband9; - u32 sideband10; - u32 sideband11; - u32 sideband12; - u32 sideband13; - u32 sideband14; - u32 sideband15; - u32 dramsts; - u32 dbgdone; - u32 dbgsignals; - u32 dbgreset; - u32 dbgmatch; - u32 counter0mask; - u32 counter1mask; - u32 counter0match; - u32 counter1match; - u32 niosreserve0; - u32 niosreserve1; - u32 niosreserve2; -}; - -#endif /*__ASSEMBLY__*/ - #define IO48_MMR_CTRLCFG0_DB2_BURST_LENGTH_MASK 0x1F000000 #define IO48_MMR_CTRLCFG0_DB2_BURST_LENGTH_SHIFT 24 #define IO48_MMR_CTRLCFG0_DB1_BURST_LENGTH_MASK 0x00F80000 diff --git a/arch/arm/mach-socfpga/spl_a10.c b/arch/arm/mach-socfpga/spl_a10.c index b10be3326803..3a2080af3aa6 100644 --- a/arch/arm/mach-socfpga/spl_a10.c +++ b/arch/arm/mach-socfpga/spl_a10.c @@ -28,6 +28,7 @@ #include <asm/arch/fpga_manager.h> #include <mmc.h> #include <memalign.h> +#include <dm/uclass.h>
#define FPGA_BUFSIZ 16 * 1024
@@ -104,6 +105,8 @@ u32 spl_boot_mode(const u32 boot_device)
void spl_board_init(void) { + int ret; + struct udevice *dev; ALLOC_CACHE_ALIGN_BUFFER(char, buf, FPGA_BUFSIZ);
/* enable console uart printing */ @@ -127,9 +130,16 @@ void spl_board_init(void) fpgamgr_program(buf, FPGA_BUFSIZ, 0); }
+#if CONFIG_IS_ENABLED(ALTERA_SDRAM) /* If the IOSSM/full FPGA is already loaded, start DDR */ - if (is_fpgamgr_early_user_mode() || is_fpgamgr_user_mode()) - ddr_calibration_sequence(); + if (is_fpgamgr_early_user_mode() || is_fpgamgr_user_mode()) { + ret = uclass_get_device(UCLASS_RAM, 0, &dev); + if (ret) { + debug("DRAM init failed: %d\n", ret); + hang(); + } + } +#endif
if (!is_fpgamgr_user_mode()) fpgamgr_program(buf, FPGA_BUFSIZ, 0); diff --git a/drivers/ddr/altera/Kconfig b/drivers/ddr/altera/Kconfig index 8f590dc5f611..30ee884dcf47 100644 --- a/drivers/ddr/altera/Kconfig +++ b/drivers/ddr/altera/Kconfig @@ -2,7 +2,7 @@ config SPL_ALTERA_SDRAM bool "SoCFPGA DDR SDRAM driver in SPL" depends on SPL depends on TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10 || TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX - select RAM if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX - select SPL_RAM if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX + select RAM + select SPL_RAM help Enable DDR SDRAM controller for the SoCFPGA devices. diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e0779b810fdc..a31d45a5bb8e 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -5,9 +5,11 @@
#include <common.h> #include <cpu_func.h> +#include <dm.h> #include <errno.h> #include <fdtdec.h> #include <malloc.h> +#include <ram.h> #include <wait_bit.h> #include <watchdog.h> #include <asm/io.h> @@ -19,8 +21,15 @@
DECLARE_GLOBAL_DATA_PTR;
-static void sdram_mmr_init(void); -static u64 sdram_size_calc(void); +struct altera_sdram_priv { + struct ram_info info; +}; + +struct altera_sdram_platdata { + void __iomem *iohmc; + void __iomem *hmc; + void __iomem *ddr_sch; +};
/* FAWBANK - Number of Bank of a given device involved in the FAW period. */ #define ARRIA10_SDR_ACTIVATE_FAWBANK (0x1) @@ -33,27 +42,10 @@ static u64 sdram_size_calc(void); #define DDR_READ_LATENCY_DELAY 40 #define DDR_SIZE_2GB_HEX 0x80000000
-#define IO48_MMR_DRAMSTS 0xFFCFA0EC -#define IO48_MMR_NIOS2_RESERVE0 0xFFCFA110 -#define IO48_MMR_NIOS2_RESERVE1 0xFFCFA114 -#define IO48_MMR_NIOS2_RESERVE2 0xFFCFA118 - #define SEQ2CORE_MASK 0xF #define CORE2SEQ_INT_REQ 0xF #define SEQ2CORE_INT_RESP_BIT 3
-static const struct socfpga_ecc_hmc *socfpga_ecc_hmc_base = - (void *)SOCFPGA_SDR_ADDRESS; -static const struct socfpga_noc_ddr_scheduler *socfpga_noc_ddr_scheduler_base = - (void *)SOCFPGA_SDR_SCHEDULER_ADDRESS; -static const struct socfpga_noc_fw_ddr_mpu_fpga2sdram - *socfpga_noc_fw_ddr_mpu_fpga2sdram_base = - (void *)SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS; -static const struct socfpga_noc_fw_ddr_l3 *socfpga_noc_fw_ddr_l3_base = - (void *)SOCFPGA_SDR_FIREWALL_L3_ADDRESS; -static const struct socfpga_io48_mmr *socfpga_io48_mmr_base = - (void *)SOCFPGA_HMC_MMR_IO48_ADDRESS; - /* The following are the supported configurations */ static u32 ddr_config[] = { /* Chip - Row - Bank - Column Style */ @@ -111,7 +103,7 @@ static int emif_clear(void) SEQ2CORE_MASK, 0, 1000, 0); }
-static int emif_reset(void) +static int emif_reset(struct altera_sdram_platdata *plat) { u32 c2s, s2c; int ret; @@ -120,10 +112,10 @@ static int emif_reset(void) s2c = readl(DDR_REG_SEQ2CORE);
debug("c2s=%08x s2c=%08x nr0=%08x nr1=%08x nr2=%08x dst=%08x\n", - c2s, s2c, readl(IO48_MMR_NIOS2_RESERVE0), - readl(IO48_MMR_NIOS2_RESERVE1), - readl(IO48_MMR_NIOS2_RESERVE2), - readl(IO48_MMR_DRAMSTS)); + c2s, s2c, readl(plat->iohmc + NIOSRESERVE0), + readl(plat->iohmc + NIOSRESERVE1), + readl(plat->iohmc + NIOSRESERVE2), + readl(plat->iohmc + DRAMSTS));
if (s2c & SEQ2CORE_MASK) { ret = emif_clear(); @@ -153,26 +145,27 @@ static int emif_reset(void) debug("emif_reset interrupt cleared\n");
debug("nr0=%08x nr1=%08x nr2=%08x\n", - readl(IO48_MMR_NIOS2_RESERVE0), - readl(IO48_MMR_NIOS2_RESERVE1), - readl(IO48_MMR_NIOS2_RESERVE2)); + readl(plat->iohmc + NIOSRESERVE0), + readl(plat->iohmc + NIOSRESERVE1), + readl(plat->iohmc + NIOSRESERVE2));
return 0; }
-static int ddr_setup(void) +static int ddr_setup(struct altera_sdram_platdata *plat) { int i, ret;
/* Try 32 times to do a calibration */ for (i = 0; i < 32; i++) { mdelay(500); - ret = wait_for_bit_le32(&socfpga_ecc_hmc_base->ddrcalstat, - BIT(0), true, 500, false); + ret = wait_for_bit_le32((const void *)(plat->hmc + + DDRCALSTAT), BIT(0), + true, 500, false); if (!ret) return 0;
- ret = emif_reset(); + ret = emif_reset(plat); if (ret) puts("Error: Failed to reset EMIF\n"); } @@ -181,9 +174,9 @@ static int ddr_setup(void) return -EPERM; }
-static int sdram_is_ecc_enabled(void) +static int sdram_is_ecc_enabled(struct altera_sdram_platdata *plat) { - return !!(readl(&socfpga_ecc_hmc_base->eccctrl) & + return !!(readl(plat->hmc + ECCCTRL1) & ALT_ECC_HMC_OCP_ECCCTL_ECC_EN_SET_MSK); }
@@ -206,18 +199,18 @@ static void sdram_init_ecc_bits(u32 size) }
/* Function to startup the SDRAM*/ -static int sdram_startup(void) +static int sdram_startup(struct altera_sdram_platdata *plat) { /* Release NOC ddr scheduler from reset */ socfpga_reset_deassert_noc_ddr_scheduler();
/* Bringup the DDR (calibration and configuration) */ - return ddr_setup(); + return ddr_setup(plat); }
-static u64 sdram_size_calc(void) +static u64 sdram_size_calc(struct altera_sdram_platdata *plat) { - u32 dramaddrw = readl(&socfpga_io48_mmr_base->dramaddrw); + u32 dramaddrw = readl(plat->iohmc + DRAMADDRW);
u64 size = BIT(((dramaddrw & IO48_MMR_DRAMADDRW_CFG_CS_ADDR_WIDTH_MASK) >> @@ -233,7 +226,7 @@ static u64 sdram_size_calc(void) IO48_MMR_DRAMADDRW_CFG_ROW_ADDR_WIDTH_SHIFT) + (dramaddrw & IO48_MMR_DRAMADDRW_CFG_COL_ADDR_WIDTH_MASK));
- size *= (2 << (readl(&socfpga_ecc_hmc_base->ddrioctrl) & + size *= (2 << (readl(plat->hmc + DDRIOCTRL) & ALT_ECC_HMC_OCP_DDRIOCTRL_IO_SIZE_MSK));
debug("SDRAM size=%llu\n", size); @@ -242,18 +235,18 @@ static u64 sdram_size_calc(void) }
/* Function to initialize SDRAM MMR and NOC DDR scheduler*/ -static void sdram_mmr_init(void) +static void sdram_mmr_init(struct altera_sdram_platdata *plat) { u32 update_value, io48_value; - u32 ctrlcfg0 = readl(&socfpga_io48_mmr_base->ctrlcfg0); - u32 ctrlcfg1 = readl(&socfpga_io48_mmr_base->ctrlcfg1); - u32 dramaddrw = readl(&socfpga_io48_mmr_base->dramaddrw); - u32 caltim0 = readl(&socfpga_io48_mmr_base->caltiming0); - u32 caltim1 = readl(&socfpga_io48_mmr_base->caltiming1); - u32 caltim2 = readl(&socfpga_io48_mmr_base->caltiming2); - u32 caltim3 = readl(&socfpga_io48_mmr_base->caltiming3); - u32 caltim4 = readl(&socfpga_io48_mmr_base->caltiming4); - u32 caltim9 = readl(&socfpga_io48_mmr_base->caltiming9); + u32 ctrlcfg0 = readl(plat->iohmc + CTRLCFG0); + u32 ctrlcfg1 = readl(plat->iohmc + CTRLCFG1); + u32 dramaddrw = readl(plat->iohmc + DRAMADDRW); + u32 caltim0 = readl(plat->iohmc + CALTIMING0); + u32 caltim1 = readl(plat->iohmc + CALTIMING1); + u32 caltim2 = readl(plat->iohmc + CALTIMING2); + u32 caltim3 = readl(plat->iohmc + CALTIMING3); + u32 caltim4 = readl(plat->iohmc + CALTIMING4); + u32 caltim9 = readl(plat->iohmc + CALTIMING9); u32 ddrioctl;
/* @@ -270,13 +263,13 @@ static void sdram_mmr_init(void) * bit[9:6] = Minor Release # * bit[14:10] = Major Release # */ - if ((readl(&socfpga_io48_mmr_base->niosreserve1) >> 6) & 0x1FF) { - update_value = readl(&socfpga_io48_mmr_base->niosreserve0); + if ((readl(plat->iohmc + NIOSRESERVE1) >> 6) & 0x1FF) { + update_value = readl(plat->iohmc + NIOSRESERVE0); writel(((update_value & 0xFF) >> 5), - &socfpga_ecc_hmc_base->ddrioctrl); + plat->hmc + DDRIOCTRL); }
- ddrioctl = readl(&socfpga_ecc_hmc_base->ddrioctrl); + ddrioctl = readl(plat->hmc + DDRIOCTRL);
/* Set the DDR Configuration [0xFFD12400] */ io48_value = ARRIA_DDR_CONFIG( @@ -297,8 +290,7 @@ static void sdram_mmr_init(void)
update_value = match_ddr_conf(io48_value); if (update_value) - writel(update_value, - &socfpga_noc_ddr_scheduler_base->ddr_t_main_scheduler_ddrconf); + writel(update_value, plat->ddr_sch + DDRCONF);
/* * Configure DDR timing [0xFFD1240C] @@ -360,7 +352,7 @@ static void sdram_mmr_init(void) caltim0_cfg_act_to_rdwr - (ctrlcfg0_cfg_ctrl_burst_len >> 2));
- io48_value = ((((readl(&socfpga_io48_mmr_base->dramtiming0) & + io48_value = ((((readl(plat->iohmc + DRAMTIMING0) & ALT_IO48_DRAMTIME_MEM_READ_LATENCY_MASK) + 2 + 15 + (ctrlcfg0_cfg_ctrl_burst_len >> 1)) >> 1) - /* Up to here was in memory cycles so divide by 2 */ @@ -381,20 +373,18 @@ static void sdram_mmr_init(void) ALT_NOC_MPU_DDR_T_SCHED_DDRTIMING_WRTORD_LSB) | (((ddrioctl == 1) ? 1 : 0) << ALT_NOC_MPU_DDR_T_SCHED_DDRTIMING_BWRATIO_LSB)), - &socfpga_noc_ddr_scheduler_base-> - ddr_t_main_scheduler_ddrtiming); + plat->ddr_sch + DDRTIMING);
/* Configure DDR mode [0xFFD12410] [precharge = 0] */ writel(((ddrioctl ? 0 : 1) << ALT_NOC_MPU_DDR_T_SCHED_DDRMOD_BWRATIOEXTENDED_LSB), - &socfpga_noc_ddr_scheduler_base->ddr_t_main_scheduler_ddrmode); + plat->ddr_sch + DDRMODE);
/* Configure the read latency [0xFFD12414] */ - writel(((readl(&socfpga_io48_mmr_base->dramtiming0) & + writel(((readl(plat->iohmc + DRAMTIMING0) & ALT_IO48_DRAMTIME_MEM_READ_LATENCY_MASK) >> 1) + DDR_READ_LATENCY_DELAY, - &socfpga_noc_ddr_scheduler_base-> - ddr_t_main_scheduler_readlatency); + plat->ddr_sch + DDRREADLATENCY);
/* * Configuring timing values concerning activate commands @@ -406,7 +396,7 @@ static void sdram_mmr_init(void) ALT_NOC_MPU_DDR_T_SCHED_ACTIVATE_FAW_LSB) | (ARRIA10_SDR_ACTIVATE_FAWBANK << ALT_NOC_MPU_DDR_T_SCHED_ACTIVATE_FAWBANK_LSB)), - &socfpga_noc_ddr_scheduler_base->ddr_t_main_scheduler_activate); + plat->ddr_sch + DDRACTIVATE);
/* * Configuring timing values concerning device to device data bus @@ -418,26 +408,26 @@ static void sdram_mmr_init(void) ALT_NOC_MPU_DDR_T_SCHED_DEVTODEV_BUSRDTOWR_LSB) | (caltim3_cfg_wr_to_rd_dc << ALT_NOC_MPU_DDR_T_SCHED_DEVTODEV_BUSWRTORD_LSB)), - &socfpga_noc_ddr_scheduler_base->ddr_t_main_scheduler_devtodev); + plat->ddr_sch + DDRDEVTODEV);
/* Enable or disable the SDRAM ECC */ if (ctrlcfg1 & IO48_MMR_CTRLCFG1_CTRL_ENABLE_ECC) { - setbits_le32(&socfpga_ecc_hmc_base->eccctrl, + setbits_le32(plat->hmc + ECCCTRL1, (ALT_ECC_HMC_OCP_ECCCTL_AWB_CNT_RST_SET_MSK | ALT_ECC_HMC_OCP_ECCCTL_CNT_RST_SET_MSK | ALT_ECC_HMC_OCP_ECCCTL_ECC_EN_SET_MSK)); - clrbits_le32(&socfpga_ecc_hmc_base->eccctrl, + clrbits_le32(plat->hmc + ECCCTRL1, (ALT_ECC_HMC_OCP_ECCCTL_AWB_CNT_RST_SET_MSK | ALT_ECC_HMC_OCP_ECCCTL_CNT_RST_SET_MSK)); - setbits_le32(&socfpga_ecc_hmc_base->eccctrl2, + setbits_le32(plat->hmc + ECCCTRL2, (ALT_ECC_HMC_OCP_ECCCTL2_RMW_EN_SET_MSK | ALT_ECC_HMC_OCP_ECCCTL2_AWB_EN_SET_MSK)); } else { - clrbits_le32(&socfpga_ecc_hmc_base->eccctrl, + clrbits_le32(plat->hmc + ECCCTRL1, (ALT_ECC_HMC_OCP_ECCCTL_AWB_CNT_RST_SET_MSK | ALT_ECC_HMC_OCP_ECCCTL_CNT_RST_SET_MSK | ALT_ECC_HMC_OCP_ECCCTL_ECC_EN_SET_MSK)); - clrbits_le32(&socfpga_ecc_hmc_base->eccctrl2, + clrbits_le32(plat->hmc + ECCCTRL2, (ALT_ECC_HMC_OCP_ECCCTL2_RMW_EN_SET_MSK | ALT_ECC_HMC_OCP_ECCCTL2_AWB_EN_SET_MSK)); } @@ -449,173 +439,156 @@ struct firewall_entry { const u32 en_addr; const u32 en_bit; }; -#define FW_MPU_FPGA_ADDRESS \ - ((const struct socfpga_noc_fw_ddr_mpu_fpga2sdram *)\ - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS)
-#define SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(ADDR) \ - (SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS + \ - offsetof(struct socfpga_noc_fw_ddr_mpu_fpga2sdram, ADDR)) +#define SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(off) \ + (SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS + (off))
-#define SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(ADDR) \ - (SOCFPGA_SDR_FIREWALL_L3_ADDRESS + \ - offsetof(struct socfpga_noc_fw_ddr_l3, ADDR)) +#define SOCFPGA_SDR_FIREWALL_L3_ADDR(off) \ + (SOCFPGA_SDR_FIREWALL_L3_ADDRESS + (off))
const struct firewall_entry firewall_table[] = { { "mpu0", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(mpuregion0addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_MPUR0), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_MPUREG0EN_SET_MSK }, { "mpu1", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(mpuregion1addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_MPUR1), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_MPUREG1EN_SET_MSK }, { "mpu2", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(mpuregion2addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_MPUR2), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_MPUREG2EN_SET_MSK }, { "mpu3", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(mpuregion3addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_MPUR3), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_MPUREG3EN_SET_MSK }, { "l3-0", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion0addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R0_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG0EN_SET_MSK }, { "l3-1", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion1addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R1_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG1EN_SET_MSK }, { "l3-2", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion2addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R2_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG2EN_SET_MSK }, { "l3-3", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion3addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R3_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG3EN_SET_MSK }, { "l3-4", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion4addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R4_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG4EN_SET_MSK }, { "l3-5", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion5addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R5_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG5EN_SET_MSK }, { "l3-6", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion6addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R6_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG6EN_SET_MSK }, { "l3-7", - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(hpsregion7addr), - SOCFPGA_SDR_FIREWALL_L3_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_HPS_R7_ADDR), + SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_HPSREG7EN_SET_MSK }, { "fpga2sdram0-0", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram0region0addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S0_R0), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR0REG0EN_SET_MSK }, { "fpga2sdram0-1", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram0region1addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S0_R1), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR0REG1EN_SET_MSK }, { "fpga2sdram0-2", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram0region2addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S0_R2), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR0REG2EN_SET_MSK }, { "fpga2sdram0-3", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram0region3addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S0_R3), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR0REG3EN_SET_MSK }, { "fpga2sdram1-0", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram1region0addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S1_R0), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR1REG0EN_SET_MSK }, { "fpga2sdram1-1", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram1region1addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S1_R1), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR1REG1EN_SET_MSK }, { "fpga2sdram1-2", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram1region2addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S1_R2), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR1REG2EN_SET_MSK }, { "fpga2sdram1-3", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram1region3addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S1_R3), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR1REG3EN_SET_MSK }, { "fpga2sdram2-0", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram2region0addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S2_R0), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR2REG0EN_SET_MSK }, { "fpga2sdram2-1", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram2region1addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S2_R1), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR2REG1EN_SET_MSK }, { "fpga2sdram2-2", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram2region2addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S2_R2), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR2REG2EN_SET_MSK }, { "fpga2sdram2-3", - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET - (fpga2sdram2region3addr), - SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDRESS_OFFSET(enable), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_F2S2_R3), + SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN), ALT_NOC_FW_DDR_SCR_EN_F2SDR2REG3EN_SET_MSK },
@@ -636,9 +609,8 @@ static int of_sdram_firewall_setup(const void *blob) return -ENXIO;
/* set to default state */ - writel(0, &socfpga_noc_fw_ddr_mpu_fpga2sdram_base->enable); - writel(0, &socfpga_noc_fw_ddr_l3_base->enable); - + writel(0, SOCFPGA_SDR_FIREWALL_MPU_FPGA_ADDR(FW_DDR_MPU_F2S_SCR_EN)); + writel(0, SOCFPGA_SDR_FIREWALL_L3_ADDR(FW_DDR_L3_SCR_EN));
for (i = 0; i < ARRAY_SIZE(firewall_table); i++) { sprintf(name, "%s", firewall_table[i].prop_name); @@ -662,12 +634,12 @@ static int of_sdram_firewall_setup(const void *blob) return 0; }
-int ddr_calibration_sequence(void) +static int ddr_calibration_sequence(struct altera_sdram_platdata *plat) { WATCHDOG_RESET();
/* Check to see if SDRAM cal was success */ - if (sdram_startup()) { + if (sdram_startup(plat)) { puts("DDRCAL: Failed\n"); return -EPERM; } @@ -677,10 +649,10 @@ int ddr_calibration_sequence(void) WATCHDOG_RESET();
/* initialize the MMR register */ - sdram_mmr_init(); + sdram_mmr_init(plat);
/* assigning the SDRAM size */ - u64 size = sdram_size_calc(); + u64 size = sdram_size_calc(plat);
/* * If size is less than zero, this is invalid/weird value from @@ -700,8 +672,80 @@ int ddr_calibration_sequence(void) if (of_sdram_firewall_setup(gd->fdt_blob)) puts("FW: Error Configuring Firewall\n");
- if (sdram_is_ecc_enabled()) + if (sdram_is_ecc_enabled(plat)) sdram_init_ecc_bits(gd->ram_size);
return 0; } + +static int altera_sdram_ofdata_to_platdata(struct udevice *dev) +{ + struct altera_sdram_platdata *plat = dev->platdata; + fdt_addr_t addr; + + addr = dev_read_addr_index(dev, 0); + if (addr == FDT_ADDR_T_NONE) + return -EINVAL; + plat->iohmc = (void __iomem *)addr; + + addr = dev_read_addr_index(dev, 1); + if (addr == FDT_ADDR_T_NONE) + return -EINVAL; + plat->hmc = (void __iomem *)addr; + + addr = dev_read_addr_index(dev, 2); + if (addr == FDT_ADDR_T_NONE) + return -EINVAL; + plat->ddr_sch = (void __iomem *)addr; + + return 0; +} + +static int altera_sdram_probe(struct udevice *dev) +{ + struct altera_sdram_priv *priv = dev_get_priv(dev); + + if (ddr_calibration_sequence(dev->platdata) != 0) { + puts("SDRAM init failed.\n"); + goto failed; + } + + priv->info.base = gd->bd->bi_dram[0].start; + priv->info.size = gd->ram_size; + + return 0; + +failed: + return -ENODEV; +} + +static int altera_sdram_get_info(struct udevice *dev, + struct ram_info *info) +{ + struct altera_sdram_priv *priv = dev_get_priv(dev); + + info->base = priv->info.base; + info->size = priv->info.size; + + return 0; +} + +static struct ram_ops altera_sdram_ops = { + .get_info = altera_sdram_get_info, +}; + +static const struct udevice_id altera_sdram_ids[] = { + { .compatible = "altr,sdr-ctl" }, + { /* sentinel */ } +}; + +U_BOOT_DRIVER(altera_sdram) = { + .name = "altr_sdr_ctl", + .id = UCLASS_RAM, + .of_match = altera_sdram_ids, + .ops = &altera_sdram_ops, + .ofdata_to_platdata = altera_sdram_ofdata_to_platdata, + .platdata_auto_alloc_size = sizeof(struct altera_sdram_platdata), + .probe = altera_sdram_probe, + .priv_auto_alloc_size = sizeof(struct altera_sdram_priv), +};
On 4/15/20 11:00 AM, Ley Foon Tan wrote: [...]
+++ b/arch/arm/mach-socfpga/include/mach/sdram_arria10.h @@ -6,76 +6,65 @@
[...]
+/* FW DDR MPU F2S SCR */ +#define FW_DDR_MPU_F2S_SCR_EN 0x00 +#define FW_DDR_MPU_F2S_SCR_MPUR0 0x10 +#define FW_DDR_MPU_F2S_SCR_MPUR1 0x14 +#define FW_DDR_MPU_F2S_SCR_MPUR2 0x18 +#define FW_DDR_MPU_F2S_SCR_MPUR3 0x1c
These can be: #define FW_DDR_MPU_F2S_SCR_MPUR(n) (0x10 + (n) * 4)
and the other macros can be reduced in a similar manner.
+#define FW_DDR_MPU_F2S_SCR_F2S0_R0 0x20 +#define FW_DDR_MPU_F2S_SCR_F2S0_R1 0x24 +#define FW_DDR_MPU_F2S_SCR_F2S0_R2 0x28 +#define FW_DDR_MPU_F2S_SCR_F2S0_R3 0x2c +#define FW_DDR_MPU_F2S_SCR_F2S1_R0 0x30 +#define FW_DDR_MPU_F2S_SCR_F2S1_R1 0x34 +#define FW_DDR_MPU_F2S_SCR_F2S1_R2 0x38 +#define FW_DDR_MPU_F2S_SCR_F2S1_R3 0x3c +#define FW_DDR_MPU_F2S_SCR_F2S2_R0 0x40 +#define FW_DDR_MPU_F2S_SCR_F2S2_R1 0x44 +#define FW_DDR_MPU_F2S_SCR_F2S2_R2 0x48 +#define FW_DDR_MPU_F2S_SCR_F2S2_R3 0x4c
+/* FW DDR L3 DDR SCR */ +#define FW_DDR_L3_SCR_EN 0x00 +#define FW_DDR_L3_SCR_HPS_R0_ADDR 0x0c +#define FW_DDR_L3_SCR_HPS_R1_ADDR 0x10 +#define FW_DDR_L3_SCR_HPS_R2_ADDR 0x14 +#define FW_DDR_L3_SCR_HPS_R3_ADDR 0x18 +#define FW_DDR_L3_SCR_HPS_R4_ADDR 0x1c +#define FW_DDR_L3_SCR_HPS_R5_ADDR 0x20 +#define FW_DDR_L3_SCR_HPS_R6_ADDR 0x24 +#define FW_DDR_L3_SCR_HPS_R7_ADDR 0x28
[...]
+#if CONFIG_IS_ENABLED(ALTERA_SDRAM) /* If the IOSSM/full FPGA is already loaded, start DDR */
- if (is_fpgamgr_early_user_mode() || is_fpgamgr_user_mode())
ddr_calibration_sequence();
- if (is_fpgamgr_early_user_mode() || is_fpgamgr_user_mode()) {
ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {debug("DRAM init failed: %d\n", ret);
This should be printf(), since it's an error, no ?
hang();}- }
+#endif
if (!is_fpgamgr_user_mode()) fpgamgr_program(buf, FPGA_BUFSIZ, 0); diff --git a/drivers/ddr/altera/Kconfig b/drivers/ddr/altera/Kconfig index 8f590dc5f611..30ee884dcf47 100644 --- a/drivers/ddr/altera/Kconfig +++ b/drivers/ddr/altera/Kconfig @@ -2,7 +2,7 @@ config SPL_ALTERA_SDRAM bool "SoCFPGA DDR SDRAM driver in SPL" depends on SPL depends on TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10 || TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select RAM if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select SPL_RAM if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select RAM
- select SPL_RAM
Shouldn't this be selected for Arria 10 only ?
[...]
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, April 15, 2020 8:42 PM To: Tan, Ley Foon ley.foon.tan@intel.com; u-boot@lists.denx.de Cc: Ley Foon Tan lftan.linux@gmail.com; See, Chin Liang chin.liang.see@intel.com; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; Chee, Tien Fong tien.fong.chee@intel.com Subject: Re: [PATCH 2/7] ddr: altera: arria10: Move SDRAM driver to DM
On 4/15/20 11:00 AM, Ley Foon Tan wrote: [...]
+++ b/arch/arm/mach-socfpga/include/mach/sdram_arria10.h @@ -6,76 +6,65 @@
[...]
+/* FW DDR MPU F2S SCR */ +#define FW_DDR_MPU_F2S_SCR_EN 0x00 +#define FW_DDR_MPU_F2S_SCR_MPUR0 0x10 +#define FW_DDR_MPU_F2S_SCR_MPUR1 0x14 +#define FW_DDR_MPU_F2S_SCR_MPUR2 0x18 +#define FW_DDR_MPU_F2S_SCR_MPUR3 0x1c
These can be: #define FW_DDR_MPU_F2S_SCR_MPUR(n) (0x10 + (n) * 4)
and the other macros can be reduced in a similar manner.
Okay.
[...]
+#if CONFIG_IS_ENABLED(ALTERA_SDRAM) /* If the IOSSM/full FPGA is already loaded, start DDR */
- if (is_fpgamgr_early_user_mode() || is_fpgamgr_user_mode())
ddr_calibration_sequence();
- if (is_fpgamgr_early_user_mode() || is_fpgamgr_user_mode()) {
ret = uclass_get_device(UCLASS_RAM, 0, &dev);if (ret) {debug("DRAM init failed: %d\n", ret);This should be printf(), since it's an error, no ?
Yes, should use printf().
hang();}- }
+#endif
if (!is_fpgamgr_user_mode()) fpgamgr_program(buf, FPGA_BUFSIZ, 0); diff --git a/drivers/ddr/altera/Kconfig b/drivers/ddr/altera/Kconfig index 8f590dc5f611..30ee884dcf47 100644 --- a/drivers/ddr/altera/Kconfig +++ b/drivers/ddr/altera/Kconfig @@ -2,7 +2,7 @@ config SPL_ALTERA_SDRAM bool "SoCFPGA DDR SDRAM driver in SPL" depends on SPL depends on TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
|| TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select RAM if TARGET_SOCFPGA_GEN5 ||
TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select SPL_RAM if TARGET_SOCFPGA_GEN5 ||
TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select RAM
- select SPL_RAM
Shouldn't this be selected for Arria 10 only ?
Previously, we select SPL_RAM and RAM for Gen5, S10 and Agilex only. They support device model. Now, we convert A10 SDRAM driver support device model, so we just need to select SPL_RAM and RAM always, regardless of TARGET.
Regards Ley Foon
On 4/16/20 3:41 AM, Tan, Ley Foon wrote: [...]
a/drivers/ddr/altera/Kconfig b/drivers/ddr/altera/Kconfig index 8f590dc5f611..30ee884dcf47 100644 --- a/drivers/ddr/altera/Kconfig +++ b/drivers/ddr/altera/Kconfig @@ -2,7 +2,7 @@ config SPL_ALTERA_SDRAM bool "SoCFPGA DDR SDRAM driver in SPL" depends on SPL depends on TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10
|| TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select RAM if TARGET_SOCFPGA_GEN5 ||
TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select SPL_RAM if TARGET_SOCFPGA_GEN5 ||
TARGET_SOCFPGA_STRATIX10 || TARGET_SOCFPGA_AGILEX
- select RAM
- select SPL_RAM
Shouldn't this be selected for Arria 10 only ?
Previously, we select SPL_RAM and RAM for Gen5, S10 and Agilex only. They support device model. Now, we convert A10 SDRAM driver support device model, so we just need to select SPL_RAM and RAM always, regardless of TARGET.
All right, I see.
Change to use reset DM function and remove unused socfpga_reset_deassert_noc_ddr_scheduler().
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com --- .../include/mach/reset_manager_arria10.h | 1 - arch/arm/mach-socfpga/reset_manager_arria10.c | 7 ------ drivers/ddr/altera/sdram_arria10.c | 25 ++++++++++--------- 3 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h index 22e4eb33de88..a0fad7c1e2fc 100644 --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h @@ -9,7 +9,6 @@ #include <dt-bindings/reset/altr,rst-mgr-a10.h>
void socfpga_watchdog_disable(void); -void socfpga_reset_deassert_noc_ddr_scheduler(void); int socfpga_reset_deassert_bridges_handoff(void); void socfpga_reset_deassert_osc1wd0(void); int socfpga_bridges_reset(void); diff --git a/arch/arm/mach-socfpga/reset_manager_arria10.c b/arch/arm/mach-socfpga/reset_manager_arria10.c index aa5299415a74..edfe250ec0bc 100644 --- a/arch/arm/mach-socfpga/reset_manager_arria10.c +++ b/arch/arm/mach-socfpga/reset_manager_arria10.c @@ -62,13 +62,6 @@ void socfpga_watchdog_disable(void) ALT_RSTMGR_PER1MODRST_WD0_SET_MSK); }
-/* Release NOC ddr scheduler from reset */ -void socfpga_reset_deassert_noc_ddr_scheduler(void) -{ - clrbits_le32(socfpga_get_rstmgr_addr() + RSTMGR_A10_BRGMODRST, - ALT_RSTMGR_BRGMODRST_DDRSCH_SET_MSK); -} - static int get_bridge_init_val(const void *blob, int compat_id) { int node; diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index a31d45a5bb8e..794c13acfa93 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -10,19 +10,21 @@ #include <fdtdec.h> #include <malloc.h> #include <ram.h> +#include <reset.h> #include <wait_bit.h> #include <watchdog.h> #include <asm/io.h> #include <asm/arch/fpga_manager.h> #include <asm/arch/misc.h> -#include <asm/arch/reset_manager.h> #include <asm/arch/sdram.h> +#include <dm/device_compat.h> #include <linux/kernel.h>
DECLARE_GLOBAL_DATA_PTR;
struct altera_sdram_priv { struct ram_info info; + struct reset_ctl_bulk resets; };
struct altera_sdram_platdata { @@ -152,7 +154,7 @@ static int emif_reset(struct altera_sdram_platdata *plat) return 0; }
-static int ddr_setup(struct altera_sdram_platdata *plat) +static int sdram_startup(struct altera_sdram_platdata *plat) { int i, ret;
@@ -198,16 +200,6 @@ static void sdram_init_ecc_bits(u32 size) dcache_disable(); }
-/* Function to startup the SDRAM*/ -static int sdram_startup(struct altera_sdram_platdata *plat) -{ - /* Release NOC ddr scheduler from reset */ - socfpga_reset_deassert_noc_ddr_scheduler(); - - /* Bringup the DDR (calibration and configuration) */ - return ddr_setup(plat); -} - static u64 sdram_size_calc(struct altera_sdram_platdata *plat) { u32 dramaddrw = readl(plat->iohmc + DRAMADDRW); @@ -703,8 +695,17 @@ static int altera_sdram_ofdata_to_platdata(struct udevice *dev)
static int altera_sdram_probe(struct udevice *dev) { + int ret; struct altera_sdram_priv *priv = dev_get_priv(dev);
+ ret = reset_get_bulk(dev, &priv->resets); + if (ret) { + dev_err(dev, "Can't get reset: %d\n", ret); + return -ENODEV; + } + + reset_deassert_bulk(&priv->resets); + if (ddr_calibration_sequence(dev->platdata) != 0) { puts("SDRAM init failed.\n"); goto failed;
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Change to use reset DM function and remove unused socfpga_reset_deassert_noc_ddr_scheduler().
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
.../include/mach/reset_manager_arria10.h | 1 - arch/arm/mach-socfpga/reset_manager_arria10.c | 7 ------ drivers/ddr/altera/sdram_arria10.c | 25 ++++++++++--------- 3 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h index 22e4eb33de88..a0fad7c1e2fc 100644 --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h @@ -9,7 +9,6 @@ #include <dt-bindings/reset/altr,rst-mgr-a10.h>
void socfpga_watchdog_disable(void); -void socfpga_reset_deassert_noc_ddr_scheduler(void); int socfpga_reset_deassert_bridges_handoff(void); void socfpga_reset_deassert_osc1wd0(void); int socfpga_bridges_reset(void); diff --git a/arch/arm/mach-socfpga/reset_manager_arria10.c b/arch/arm/mach-socfpga/reset_manager_arria10.c index aa5299415a74..edfe250ec0bc 100644 --- a/arch/arm/mach-socfpga/reset_manager_arria10.c +++ b/arch/arm/mach-socfpga/reset_manager_arria10.c @@ -62,13 +62,6 @@ void socfpga_watchdog_disable(void) ALT_RSTMGR_PER1MODRST_WD0_SET_MSK); }
-/* Release NOC ddr scheduler from reset */ -void socfpga_reset_deassert_noc_ddr_scheduler(void) -{
- clrbits_le32(socfpga_get_rstmgr_addr() + RSTMGR_A10_BRGMODRST,
ALT_RSTMGR_BRGMODRST_DDRSCH_SET_MSK);-}
static int get_bridge_init_val(const void *blob, int compat_id) { int node; diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index a31d45a5bb8e..794c13acfa93 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -10,19 +10,21 @@ #include <fdtdec.h> #include <malloc.h> #include <ram.h> +#include <reset.h> #include <wait_bit.h> #include <watchdog.h> #include <asm/io.h> #include <asm/arch/fpga_manager.h> #include <asm/arch/misc.h> -#include <asm/arch/reset_manager.h> #include <asm/arch/sdram.h> +#include <dm/device_compat.h> #include <linux/kernel.h>
DECLARE_GLOBAL_DATA_PTR;
struct altera_sdram_priv { struct ram_info info;
- struct reset_ctl_bulk resets;
};
struct altera_sdram_platdata { @@ -152,7 +154,7 @@ static int emif_reset(struct altera_sdram_platdata *plat) return 0; }
-static int ddr_setup(struct altera_sdram_platdata *plat) +static int sdram_startup(struct altera_sdram_platdata *plat) { int i, ret;
@@ -198,16 +200,6 @@ static void sdram_init_ecc_bits(u32 size) dcache_disable(); }
-/* Function to startup the SDRAM*/ -static int sdram_startup(struct altera_sdram_platdata *plat) -{
- /* Release NOC ddr scheduler from reset */
- socfpga_reset_deassert_noc_ddr_scheduler();
- /* Bringup the DDR (calibration and configuration) */
- return ddr_setup(plat);
-}
static u64 sdram_size_calc(struct altera_sdram_platdata *plat) { u32 dramaddrw = readl(plat->iohmc + DRAMADDRW); @@ -703,8 +695,17 @@ static int altera_sdram_ofdata_to_platdata(struct udevice *dev)
static int altera_sdram_probe(struct udevice *dev) {
int ret; struct altera_sdram_priv *priv = dev_get_priv(dev);
ret = reset_get_bulk(dev, &priv->resets);
if (ret) {
dev_err(dev, "Can't get reset: %d\n", ret);return -ENODEV;}
reset_deassert_bulk(&priv->resets);
if (ddr_calibration_sequence(dev->platdata) != 0) { puts("SDRAM init failed.\n"); goto failed;
I think you need to re-assert the reset in the failed: fail path.
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, April 15, 2020 8:43 PM To: Tan, Ley Foon ley.foon.tan@intel.com; u-boot@lists.denx.de Cc: Ley Foon Tan lftan.linux@gmail.com; See, Chin Liang chin.liang.see@intel.com; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; Chee, Tien Fong tien.fong.chee@intel.com Subject: Re: [PATCH 3/7] ddr: altera: arria10: Change to use reset DM function
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Change to use reset DM function and remove unused socfpga_reset_deassert_noc_ddr_scheduler().
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
.../include/mach/reset_manager_arria10.h | 1 - arch/arm/mach-socfpga/reset_manager_arria10.c | 7 ------ drivers/ddr/altera/sdram_arria10.c | 25 ++++++++++--------- 3 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h index 22e4eb33de88..a0fad7c1e2fc 100644 --- a/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h +++ b/arch/arm/mach-socfpga/include/mach/reset_manager_arria10.h @@ -9,7 +9,6 @@ static int altera_sdram_ofdata_to_platdata(struct udevice *dev)
static int altera_sdram_probe(struct udevice *dev) {
int ret; struct altera_sdram_priv *priv = dev_get_priv(dev);
ret = reset_get_bulk(dev, &priv->resets);
if (ret) {
dev_err(dev, "Can't get reset: %d\n", ret);return -ENODEV;}
reset_deassert_bulk(&priv->resets);
if (ddr_calibration_sequence(dev->platdata) != 0) { puts("SDRAM init failed.\n"); goto failed;
I think you need to re-assert the reset in the failed: fail path.
Okay.
Thanks. Regards Ley Foon
Move sdram_arria10.h to drivers/ddr/altera directory. No functionality change.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com --- arch/arm/mach-socfpga/include/mach/sdram.h | 2 +- arch/arm/mach-socfpga/misc_arria10.c | 2 +- drivers/ddr/altera/sdram_arria10.c | 3 ++- .../include/mach => drivers/ddr/altera}/sdram_arria10.h | 0 4 files changed, 4 insertions(+), 3 deletions(-) rename {arch/arm/mach-socfpga/include/mach => drivers/ddr/altera}/sdram_arria10.h (100%)
diff --git a/arch/arm/mach-socfpga/include/mach/sdram.h b/arch/arm/mach-socfpga/include/mach/sdram.h index 79cb9e6064a2..0a52c1078b5f 100644 --- a/arch/arm/mach-socfpga/include/mach/sdram.h +++ b/arch/arm/mach-socfpga/include/mach/sdram.h @@ -10,7 +10,7 @@ #if defined(CONFIG_TARGET_SOCFPGA_GEN5) #include <asm/arch/sdram_gen5.h> #elif defined(CONFIG_TARGET_SOCFPGA_ARRIA10) -#include <asm/arch/sdram_arria10.h> +#include "../../../../../drivers/ddr/altera/sdram_arria10.h" #endif
#endif diff --git a/arch/arm/mach-socfpga/misc_arria10.c b/arch/arm/mach-socfpga/misc_arria10.c index d56349b7f3ea..1ddffaa0ac55 100644 --- a/arch/arm/mach-socfpga/misc_arria10.c +++ b/arch/arm/mach-socfpga/misc_arria10.c @@ -15,7 +15,7 @@ #include <asm/arch/pinmux.h> #include <asm/arch/reset_manager.h> #include <asm/arch/reset_manager_arria10.h> -#include <asm/arch/sdram_arria10.h> +#include <asm/arch/sdram.h> #include <asm/arch/system_manager.h> #include <asm/arch/nic301.h> #include <asm/io.h> diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index 794c13acfa93..6b74423ea789 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -16,10 +16,11 @@ #include <asm/io.h> #include <asm/arch/fpga_manager.h> #include <asm/arch/misc.h> -#include <asm/arch/sdram.h> #include <dm/device_compat.h> #include <linux/kernel.h>
+#include "sdram_arria10.h" + DECLARE_GLOBAL_DATA_PTR;
struct altera_sdram_priv { diff --git a/arch/arm/mach-socfpga/include/mach/sdram_arria10.h b/drivers/ddr/altera/sdram_arria10.h similarity index 100% rename from arch/arm/mach-socfpga/include/mach/sdram_arria10.h rename to drivers/ddr/altera/sdram_arria10.h
Add call to get_ram_size() function to check memory range for valid RAM.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com --- drivers/ddr/altera/sdram_arria10.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index 6b74423ea789..e3f11984a978 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -8,6 +8,7 @@ #include <dm.h> #include <errno.h> #include <fdtdec.h> +#include <hang.h> #include <malloc.h> #include <ram.h> #include <reset.h> @@ -17,7 +18,7 @@ #include <asm/arch/fpga_manager.h> #include <asm/arch/misc.h> #include <dm/device_compat.h> -#include <linux/kernel.h> +#include <linux/sizes.h>
#include "sdram_arria10.h"
@@ -671,6 +672,27 @@ static int ddr_calibration_sequence(struct altera_sdram_platdata *plat) return 0; }
+static void sdram_size_check(struct ram_info *ram) +{ + phys_size_t ram_check = 0; + phys_size_t size = ram->size; + phys_addr_t base = ram->base; + + debug("DDR: Running SDRAM size sanity check\n"); + + while (ram_check < size) { + ram_check += get_ram_size((void *)(base + ram_check), + (phys_size_t)SZ_1G); + } + + if (ram_check != size) { + puts("DDR: SDRAM size check failed!\n"); + hang(); + } + + debug("DDR: SDRAM size check passed!\n"); +} + static int altera_sdram_ofdata_to_platdata(struct udevice *dev) { struct altera_sdram_platdata *plat = dev->platdata; @@ -715,6 +737,7 @@ static int altera_sdram_probe(struct udevice *dev) priv->info.base = gd->bd->bi_dram[0].start; priv->info.size = gd->ram_size;
+ sdram_size_check(&priv->info); return 0;
failed:
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Add call to get_ram_size() function to check memory range for valid RAM.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index 6b74423ea789..e3f11984a978 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -8,6 +8,7 @@ #include <dm.h> #include <errno.h> #include <fdtdec.h> +#include <hang.h> #include <malloc.h> #include <ram.h> #include <reset.h> @@ -17,7 +18,7 @@ #include <asm/arch/fpga_manager.h> #include <asm/arch/misc.h> #include <dm/device_compat.h> -#include <linux/kernel.h> +#include <linux/sizes.h>
#include "sdram_arria10.h"
@@ -671,6 +672,27 @@ static int ddr_calibration_sequence(struct altera_sdram_platdata *plat) return 0; }
+static void sdram_size_check(struct ram_info *ram) +{
- phys_size_t ram_check = 0;
- phys_size_t size = ram->size;
- phys_addr_t base = ram->base;
- debug("DDR: Running SDRAM size sanity check\n");
- while (ram_check < size) {
ram_check += get_ram_size((void *)(base + ram_check),(phys_size_t)SZ_1G);
Why is it running in 1 GiB steps ?
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Wednesday, April 15, 2020 8:44 PM To: Tan, Ley Foon ley.foon.tan@intel.com; u-boot@lists.denx.de Cc: Ley Foon Tan lftan.linux@gmail.com; See, Chin Liang chin.liang.see@intel.com; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; Chee, Tien Fong tien.fong.chee@intel.com Subject: Re: [PATCH 5/7] ddr: altera: arria10: Add RAM size check
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Add call to get_ram_size() function to check memory range for valid RAM.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index 6b74423ea789..e3f11984a978 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -8,6 +8,7 @@ #include <dm.h> #include <errno.h> #include <fdtdec.h> +#include <hang.h> #include <malloc.h> #include <ram.h> #include <reset.h> @@ -17,7 +18,7 @@ #include <asm/arch/fpga_manager.h> #include <asm/arch/misc.h> #include <dm/device_compat.h> -#include <linux/kernel.h> +#include <linux/sizes.h>
#include "sdram_arria10.h"
@@ -671,6 +672,27 @@ static int ddr_calibration_sequence(struct
altera_sdram_platdata *plat)
return 0; }
+static void sdram_size_check(struct ram_info *ram) {
- phys_size_t ram_check = 0;
- phys_size_t size = ram->size;
- phys_addr_t base = ram->base;
- debug("DDR: Running SDRAM size sanity check\n");
- while (ram_check < size) {
ram_check += get_ram_size((void *)(base + ram_check),(phys_size_t)SZ_1G);Why is it running in 1 GiB steps ?
Don't have any special reason. I'm following S10/Agilex's implementation. Do you prefer use smaller step size?
Regards Ley Foon
On 4/16/20 3:34 AM, Tan, Ley Foon wrote: [...]
+static void sdram_size_check(struct ram_info *ram) {
- phys_size_t ram_check = 0;
- phys_size_t size = ram->size;
- phys_addr_t base = ram->base;
- debug("DDR: Running SDRAM size sanity check\n");
- while (ram_check < size) {
ram_check += get_ram_size((void *)(base + ram_check),(phys_size_t)SZ_1G);Why is it running in 1 GiB steps ?
Don't have any special reason. I'm following S10/Agilex's implementation. Do you prefer use smaller step size?
Rather, why doesn't it use the entire DRAM size, without the while loop?
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Thursday, April 16, 2020 4:52 PM To: Tan, Ley Foon ley.foon.tan@intel.com; u-boot@lists.denx.de Cc: Ley Foon Tan lftan.linux@gmail.com; See, Chin Liang chin.liang.see@intel.com; Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; Chee, Tien Fong tien.fong.chee@intel.com Subject: Re: [PATCH 5/7] ddr: altera: arria10: Add RAM size check
On 4/16/20 3:34 AM, Tan, Ley Foon wrote: [...]
+static void sdram_size_check(struct ram_info *ram) {
- phys_size_t ram_check = 0;
- phys_size_t size = ram->size;
- phys_addr_t base = ram->base;
- debug("DDR: Running SDRAM size sanity check\n");
- while (ram_check < size) {
ram_check += get_ram_size((void *)(base + ram_check),(phys_size_t)SZ_1G);Why is it running in 1 GiB steps ?
Don't have any special reason. I'm following S10/Agilex's implementation. Do you prefer use smaller step size?
Rather, why doesn't it use the entire DRAM size, without the while loop?
Yes, can change to entire DRAM size. Will change it. S10/Agilex have 2 banks of memory, that's why we have the while loop for 2 banks checking. But, A10 only have 1 bank.
Regards Ley Foon
Tiny printf doesn't support %i, change to %u.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com --- drivers/ddr/altera/sdram_arria10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e3f11984a978..8acf324117af 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
dcache_enable();
- printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20); + printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); memset((void *)0x8000, 0, size - 0x8000); flush_dcache_all(); printf("DDRCAL: Scrubbing ECC RAM done.\n");
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Tiny printf doesn't support %i, change to %u.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e3f11984a978..8acf324117af 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
dcache_enable();
- printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20);
- printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); memset((void *)0x8000, 0, size - 0x8000); flush_dcache_all(); printf("DDRCAL: Scrubbing ECC RAM done.\n");
Yes, sadly, tiny printf is broken so we need to patch code to work around that breakage.
On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote:
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Tiny printf doesn't support %i, change to %u.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e3f11984a978..8acf324117af 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
dcache_enable();
- printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20);
- printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); memset((void *)0x8000, 0, size - 0x8000); flush_dcache_all(); printf("DDRCAL: Scrubbing ECC RAM done.\n");
Yes, sadly, tiny printf is broken so we need to patch code to work around that breakage.
Yes, limited by design, thanks for changing.
Reviewed-by: Tom Rini trini@konsulko.com
On 4/15/20 4:56 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote:
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Tiny printf doesn't support %i, change to %u.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e3f11984a978..8acf324117af 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
dcache_enable();
- printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20);
- printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); memset((void *)0x8000, 0, size - 0x8000); flush_dcache_all(); printf("DDRCAL: Scrubbing ECC RAM done.\n");
Yes, sadly, tiny printf is broken so we need to patch code to work around that breakage.
Yes, limited by design, thanks for changing.
This code could be used without tiny printf, so this change is unnecessary.
On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote:
On 4/15/20 4:56 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote:
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Tiny printf doesn't support %i, change to %u.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e3f11984a978..8acf324117af 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
dcache_enable();
- printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20);
- printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); memset((void *)0x8000, 0, size - 0x8000); flush_dcache_all(); printf("DDRCAL: Scrubbing ECC RAM done.\n");
Yes, sadly, tiny printf is broken so we need to patch code to work around that breakage.
Yes, limited by design, thanks for changing.
This code could be used without tiny printf, so this change is unnecessary.
You've got it backwards. Code that could be used by tiny printf needs to use the more limited set of formats. But this should have been using %u all along? %i is for int, %u is unsigned int.
On 4/15/20 5:14 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote:
On 4/15/20 4:56 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote:
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Tiny printf doesn't support %i, change to %u.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e3f11984a978..8acf324117af 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
dcache_enable();
- printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20);
- printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); memset((void *)0x8000, 0, size - 0x8000); flush_dcache_all(); printf("DDRCAL: Scrubbing ECC RAM done.\n");
Yes, sadly, tiny printf is broken so we need to patch code to work around that breakage.
Yes, limited by design, thanks for changing.
This code could be used without tiny printf, so this change is unnecessary.
You've got it backwards. Code that could be used by tiny printf needs to use the more limited set of formats. But this should have been using %u all along? %i is for int, %u is unsigned int.
That would mean most of U-Boot needs to be limited to the subset of formatting characters supported by tiny printf, which is unrealistic.
On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote:
On 4/15/20 5:14 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote:
On 4/15/20 4:56 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote:
On 4/15/20 11:00 AM, Ley Foon Tan wrote:
Tiny printf doesn't support %i, change to %u.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
drivers/ddr/altera/sdram_arria10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index e3f11984a978..8acf324117af 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size)
dcache_enable();
- printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20);
- printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); memset((void *)0x8000, 0, size - 0x8000); flush_dcache_all(); printf("DDRCAL: Scrubbing ECC RAM done.\n");
Yes, sadly, tiny printf is broken so we need to patch code to work around that breakage.
Yes, limited by design, thanks for changing.
This code could be used without tiny printf, so this change is unnecessary.
You've got it backwards. Code that could be used by tiny printf needs to use the more limited set of formats. But this should have been using %u all along? %i is for int, %u is unsigned int.
That would mean most of U-Boot needs to be limited to the subset of formatting characters supported by tiny printf, which is unrealistic.
Not at all. Only the code that's used in SPL and in tiny printf situations needs to be limited to reduced set. Which is why we're at 4.5 years in and just now "oh, %i doesn't work?".
On 4/15/20 7:44 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote:
On 4/15/20 5:14 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote:
On 4/15/20 4:56 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote:
On 4/15/20 11:00 AM, Ley Foon Tan wrote: > Tiny printf doesn't support %i, change to %u. > > Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com > --- > drivers/ddr/altera/sdram_arria10.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c > index e3f11984a978..8acf324117af 100644 > --- a/drivers/ddr/altera/sdram_arria10.c > +++ b/drivers/ddr/altera/sdram_arria10.c > @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size) > > dcache_enable(); > > - printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20); > + printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); > memset((void *)0x8000, 0, size - 0x8000); > flush_dcache_all(); > printf("DDRCAL: Scrubbing ECC RAM done.\n");
Yes, sadly, tiny printf is broken so we need to patch code to work around that breakage.
Yes, limited by design, thanks for changing.
This code could be used without tiny printf, so this change is unnecessary.
You've got it backwards. Code that could be used by tiny printf needs to use the more limited set of formats. But this should have been using %u all along? %i is for int, %u is unsigned int.
That would mean most of U-Boot needs to be limited to the subset of formatting characters supported by tiny printf, which is unrealistic.
Not at all. Only the code that's used in SPL and in tiny printf situations needs to be limited to reduced set. Which is why we're at 4.5 years in and just now "oh, %i doesn't work?".
I keep running into "oh, %i, such a basic C printf() feature, doesn't work" again and again, and it makes my work with U-Boot real annoying. This should be fixed in the printf implementation, not all over the code base. Also, it prevents sane code reuse, unless we start adding #ifdef TINY_PRINTF all over the place, which is just ew ...
On Wed, Apr 15, 2020 at 08:06:45PM +0200, Marek Vasut wrote:
On 4/15/20 7:44 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote:
On 4/15/20 5:14 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote:
On 4/15/20 4:56 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote: > On 4/15/20 11:00 AM, Ley Foon Tan wrote: >> Tiny printf doesn't support %i, change to %u. >> >> Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com >> --- >> drivers/ddr/altera/sdram_arria10.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c >> index e3f11984a978..8acf324117af 100644 >> --- a/drivers/ddr/altera/sdram_arria10.c >> +++ b/drivers/ddr/altera/sdram_arria10.c >> @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size) >> >> dcache_enable(); >> >> - printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20); >> + printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); >> memset((void *)0x8000, 0, size - 0x8000); >> flush_dcache_all(); >> printf("DDRCAL: Scrubbing ECC RAM done.\n"); > > Yes, sadly, tiny printf is broken so we need to patch code to work > around that breakage.
Yes, limited by design, thanks for changing.
This code could be used without tiny printf, so this change is unnecessary.
You've got it backwards. Code that could be used by tiny printf needs to use the more limited set of formats. But this should have been using %u all along? %i is for int, %u is unsigned int.
That would mean most of U-Boot needs to be limited to the subset of formatting characters supported by tiny printf, which is unrealistic.
Not at all. Only the code that's used in SPL and in tiny printf situations needs to be limited to reduced set. Which is why we're at 4.5 years in and just now "oh, %i doesn't work?".
I keep running into "oh, %i, such a basic C printf() feature, doesn't work" again and again, and it makes my work with U-Boot real annoying. This should be fixed in the printf implementation, not all over the code base. Also, it prevents sane code reuse, unless we start adding #ifdef TINY_PRINTF all over the place, which is just ew ...
I hear you saying "I type %i not %d without thinking about it", but I'm telling you, think about it. I will not grow 200+ boards when there's an easy way not to.
On 4/16/20 2:55 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 08:06:45PM +0200, Marek Vasut wrote:
On 4/15/20 7:44 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote:
On 4/15/20 5:14 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote:
On 4/15/20 4:56 PM, Tom Rini wrote: > On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote: >> On 4/15/20 11:00 AM, Ley Foon Tan wrote: >>> Tiny printf doesn't support %i, change to %u. >>> >>> Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com >>> --- >>> drivers/ddr/altera/sdram_arria10.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c >>> index e3f11984a978..8acf324117af 100644 >>> --- a/drivers/ddr/altera/sdram_arria10.c >>> +++ b/drivers/ddr/altera/sdram_arria10.c >>> @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size) >>> >>> dcache_enable(); >>> >>> - printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20); >>> + printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); >>> memset((void *)0x8000, 0, size - 0x8000); >>> flush_dcache_all(); >>> printf("DDRCAL: Scrubbing ECC RAM done.\n"); >> >> Yes, sadly, tiny printf is broken so we need to patch code to work >> around that breakage. > > Yes, limited by design, thanks for changing.
This code could be used without tiny printf, so this change is unnecessary.
You've got it backwards. Code that could be used by tiny printf needs to use the more limited set of formats. But this should have been using %u all along? %i is for int, %u is unsigned int.
That would mean most of U-Boot needs to be limited to the subset of formatting characters supported by tiny printf, which is unrealistic.
Not at all. Only the code that's used in SPL and in tiny printf situations needs to be limited to reduced set. Which is why we're at 4.5 years in and just now "oh, %i doesn't work?".
I keep running into "oh, %i, such a basic C printf() feature, doesn't work" again and again, and it makes my work with U-Boot real annoying. This should be fixed in the printf implementation, not all over the code base. Also, it prevents sane code reuse, unless we start adding #ifdef TINY_PRINTF all over the place, which is just ew ...
I hear you saying "I type %i not %d without thinking about it", but I'm telling you, think about it.
No, it works everywhere else just fine, except U-Boot SPL is special. This is a tiny-printf bug, period.
I will not grow 200+ boards when there's an easy way not to.
By ~6 bytes, which happens with almost every DM patch. I am not buying the size argument.
On Thu, Apr 16, 2020 at 03:11:45PM +0200, Marek Vasut wrote:
On 4/16/20 2:55 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 08:06:45PM +0200, Marek Vasut wrote:
On 4/15/20 7:44 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote:
On 4/15/20 5:14 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote: > On 4/15/20 4:56 PM, Tom Rini wrote: >> On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote: >>> On 4/15/20 11:00 AM, Ley Foon Tan wrote: >>>> Tiny printf doesn't support %i, change to %u. >>>> >>>> Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com >>>> --- >>>> drivers/ddr/altera/sdram_arria10.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c >>>> index e3f11984a978..8acf324117af 100644 >>>> --- a/drivers/ddr/altera/sdram_arria10.c >>>> +++ b/drivers/ddr/altera/sdram_arria10.c >>>> @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size) >>>> >>>> dcache_enable(); >>>> >>>> - printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20); >>>> + printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); >>>> memset((void *)0x8000, 0, size - 0x8000); >>>> flush_dcache_all(); >>>> printf("DDRCAL: Scrubbing ECC RAM done.\n"); >>> >>> Yes, sadly, tiny printf is broken so we need to patch code to work >>> around that breakage. >> >> Yes, limited by design, thanks for changing. > > This code could be used without tiny printf, so this change is unnecessary.
You've got it backwards. Code that could be used by tiny printf needs to use the more limited set of formats. But this should have been using %u all along? %i is for int, %u is unsigned int.
That would mean most of U-Boot needs to be limited to the subset of formatting characters supported by tiny printf, which is unrealistic.
Not at all. Only the code that's used in SPL and in tiny printf situations needs to be limited to reduced set. Which is why we're at 4.5 years in and just now "oh, %i doesn't work?".
I keep running into "oh, %i, such a basic C printf() feature, doesn't work" again and again, and it makes my work with U-Boot real annoying. This should be fixed in the printf implementation, not all over the code base. Also, it prevents sane code reuse, unless we start adding #ifdef TINY_PRINTF all over the place, which is just ew ...
I hear you saying "I type %i not %d without thinking about it", but I'm telling you, think about it.
No, it works everywhere else just fine, except U-Boot SPL is special.
It works fine in SPL when we use the full printf, which is still a good percent of the boards.
This is a tiny-printf bug, period.
It's a feature of the explicitly reduced functionality printf. It's the whole point of the feature, provide as much output with as little code as we can.
I will not grow 200+ boards when there's an easy way not to.
By ~6 bytes, which happens with almost every DM patch. I am not buying the size argument.
Nope, not true. Boards with tiny printf rarely grow their SPL size from general changes (SoC trees only get scrutiny over growth in platforms outside their area) because I get this annoying about why they grow in size.
On 4/16/20 3:21 PM, Tom Rini wrote:
On Thu, Apr 16, 2020 at 03:11:45PM +0200, Marek Vasut wrote:
On 4/16/20 2:55 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 08:06:45PM +0200, Marek Vasut wrote:
On 4/15/20 7:44 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote:
On 4/15/20 5:14 PM, Tom Rini wrote: > On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote: >> On 4/15/20 4:56 PM, Tom Rini wrote: >>> On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote: >>>> On 4/15/20 11:00 AM, Ley Foon Tan wrote: >>>>> Tiny printf doesn't support %i, change to %u. >>>>> >>>>> Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com >>>>> --- >>>>> drivers/ddr/altera/sdram_arria10.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c >>>>> index e3f11984a978..8acf324117af 100644 >>>>> --- a/drivers/ddr/altera/sdram_arria10.c >>>>> +++ b/drivers/ddr/altera/sdram_arria10.c >>>>> @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size) >>>>> >>>>> dcache_enable(); >>>>> >>>>> - printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20); >>>>> + printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); >>>>> memset((void *)0x8000, 0, size - 0x8000); >>>>> flush_dcache_all(); >>>>> printf("DDRCAL: Scrubbing ECC RAM done.\n"); >>>> >>>> Yes, sadly, tiny printf is broken so we need to patch code to work >>>> around that breakage. >>> >>> Yes, limited by design, thanks for changing. >> >> This code could be used without tiny printf, so this change is unnecessary. > > You've got it backwards. Code that could be used by tiny printf needs > to use the more limited set of formats. But this should have been using > %u all along? %i is for int, %u is unsigned int.
That would mean most of U-Boot needs to be limited to the subset of formatting characters supported by tiny printf, which is unrealistic.
Not at all. Only the code that's used in SPL and in tiny printf situations needs to be limited to reduced set. Which is why we're at 4.5 years in and just now "oh, %i doesn't work?".
I keep running into "oh, %i, such a basic C printf() feature, doesn't work" again and again, and it makes my work with U-Boot real annoying. This should be fixed in the printf implementation, not all over the code base. Also, it prevents sane code reuse, unless we start adding #ifdef TINY_PRINTF all over the place, which is just ew ...
I hear you saying "I type %i not %d without thinking about it", but I'm telling you, think about it.
No, it works everywhere else just fine, except U-Boot SPL is special.
It works fine in SPL when we use the full printf, which is still a good percent of the boards.
This is a tiny-printf bug, period.
It's a feature of the explicitly reduced functionality printf. It's the whole point of the feature, provide as much output with as little code as we can.
Not supporting standard features is a bug, because it makes SPL behavior non-standard.
I will not grow 200+ boards when there's an easy way not to.
By ~6 bytes, which happens with almost every DM patch. I am not buying the size argument.
Nope, not true. Boards with tiny printf rarely grow their SPL size from general changes (SoC trees only get scrutiny over growth in platforms outside their area) because I get this annoying about why they grow in size.
OK, so look at socfpga_cyclone5_defconfig for example, which grew by 32 bytes between 2020.04 and u-boot/master thanks to:
commit 0486497e2b5f4d36fa968a1a60fea358cbf70b65 lib: Improve _parse_integer_fixup_radix base 16 detection
It uses tiny-printf, it grew from general change, and it came from SoC tree: https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze/-/commits/xilinx-...
It didn't take too long to find a counter-example ...
On Thu, Apr 16, 2020 at 03:39:19PM +0200, Marek Vasut wrote:
On 4/16/20 3:21 PM, Tom Rini wrote:
On Thu, Apr 16, 2020 at 03:11:45PM +0200, Marek Vasut wrote:
On 4/16/20 2:55 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 08:06:45PM +0200, Marek Vasut wrote:
On 4/15/20 7:44 PM, Tom Rini wrote:
On Wed, Apr 15, 2020 at 05:16:52PM +0200, Marek Vasut wrote: > On 4/15/20 5:14 PM, Tom Rini wrote: >> On Wed, Apr 15, 2020 at 04:58:31PM +0200, Marek Vasut wrote: >>> On 4/15/20 4:56 PM, Tom Rini wrote: >>>> On Wed, Apr 15, 2020 at 02:45:08PM +0200, Marek Vasut wrote: >>>>> On 4/15/20 11:00 AM, Ley Foon Tan wrote: >>>>>> Tiny printf doesn't support %i, change to %u. >>>>>> >>>>>> Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com >>>>>> --- >>>>>> drivers/ddr/altera/sdram_arria10.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c >>>>>> index e3f11984a978..8acf324117af 100644 >>>>>> --- a/drivers/ddr/altera/sdram_arria10.c >>>>>> +++ b/drivers/ddr/altera/sdram_arria10.c >>>>>> @@ -195,7 +195,7 @@ static void sdram_init_ecc_bits(u32 size) >>>>>> >>>>>> dcache_enable(); >>>>>> >>>>>> - printf("DDRCAL: Scrubbing ECC RAM (%i MiB).\n", size >> 20); >>>>>> + printf("DDRCAL: Scrubbing ECC RAM (%u MiB).\n", size >> 20); >>>>>> memset((void *)0x8000, 0, size - 0x8000); >>>>>> flush_dcache_all(); >>>>>> printf("DDRCAL: Scrubbing ECC RAM done.\n"); >>>>> >>>>> Yes, sadly, tiny printf is broken so we need to patch code to work >>>>> around that breakage. >>>> >>>> Yes, limited by design, thanks for changing. >>> >>> This code could be used without tiny printf, so this change is unnecessary. >> >> You've got it backwards. Code that could be used by tiny printf needs >> to use the more limited set of formats. But this should have been using >> %u all along? %i is for int, %u is unsigned int. > > That would mean most of U-Boot needs to be limited to the subset of > formatting characters supported by tiny printf, which is unrealistic.
Not at all. Only the code that's used in SPL and in tiny printf situations needs to be limited to reduced set. Which is why we're at 4.5 years in and just now "oh, %i doesn't work?".
I keep running into "oh, %i, such a basic C printf() feature, doesn't work" again and again, and it makes my work with U-Boot real annoying. This should be fixed in the printf implementation, not all over the code base. Also, it prevents sane code reuse, unless we start adding #ifdef TINY_PRINTF all over the place, which is just ew ...
I hear you saying "I type %i not %d without thinking about it", but I'm telling you, think about it.
No, it works everywhere else just fine, except U-Boot SPL is special.
It works fine in SPL when we use the full printf, which is still a good percent of the boards.
This is a tiny-printf bug, period.
It's a feature of the explicitly reduced functionality printf. It's the whole point of the feature, provide as much output with as little code as we can.
Not supporting standard features is a bug, because it makes SPL behavior non-standard.
It's a non-standard function. There's all sorts of standard printf formats it does not support, on purpose.
I will not grow 200+ boards when there's an easy way not to.
By ~6 bytes, which happens with almost every DM patch. I am not buying the size argument.
Nope, not true. Boards with tiny printf rarely grow their SPL size from general changes (SoC trees only get scrutiny over growth in platforms outside their area) because I get this annoying about why they grow in size.
OK, so look at socfpga_cyclone5_defconfig for example, which grew by 32 bytes between 2020.04 and u-boot/master thanks to:
commit 0486497e2b5f4d36fa968a1a60fea358cbf70b65 lib: Improve _parse_integer_fixup_radix base 16 detection
It uses tiny-printf, it grew from general change, and it came from SoC tree: https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze/-/commits/xilinx-...
It didn't take too long to find a counter-example ...
Yup, it grew for a bugfix. The full growth is: spl-u-boot-spl: add: 0/0, grow: 3/0 bytes: 56/0 (56) function old new delta _parse_integer_fixup_radix 92 120 +28 ns16550_serial_ofdata_to_platdata 104 128 +24 ns16550_serial_probe 76 80 +4
For other bugfixes too. Any of those bugfixes that can be done with zero size growth would be appreciated.
On 4/16/20 8:02 PM, Tom Rini wrote: [...]
I will not grow 200+ boards when there's an easy way not to.
By ~6 bytes, which happens with almost every DM patch. I am not buying the size argument.
Nope, not true. Boards with tiny printf rarely grow their SPL size from general changes (SoC trees only get scrutiny over growth in platforms outside their area) because I get this annoying about why they grow in size.
OK, so look at socfpga_cyclone5_defconfig for example, which grew by 32 bytes between 2020.04 and u-boot/master thanks to:
commit 0486497e2b5f4d36fa968a1a60fea358cbf70b65 lib: Improve _parse_integer_fixup_radix base 16 detection
It uses tiny-printf, it grew from general change, and it came from SoC tree: https://gitlab.denx.de/u-boot/custodians/u-boot-microblaze/-/commits/xilinx-...
It didn't take too long to find a counter-example ...
Yup, it grew for a bugfix. The full growth is: spl-u-boot-spl: add: 0/0, grow: 3/0 bytes: 56/0 (56) function old new delta _parse_integer_fixup_radix 92 120 +28 ns16550_serial_ofdata_to_platdata 104 128 +24 ns16550_serial_probe 76 80 +4
For other bugfixes too. Any of those bugfixes that can be done with zero size growth would be appreciated.
So, how is this bugfix applicable, while a bugfix which fixes tiny printf to behave sane is not ? I don't really see a distinction here, sorry.
And you should have rejected the above and asked for a more optimized version, based on 'I get this annoying about why they grow in size.'
dram_init_banksize() is called in board_init_f() boot sequences in Uboot, remove it from SDRAM driver.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com --- drivers/ddr/altera/sdram_arria10.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/ddr/altera/sdram_arria10.c b/drivers/ddr/altera/sdram_arria10.c index 8acf324117af..a7d0eefe9195 100644 --- a/drivers/ddr/altera/sdram_arria10.c +++ b/drivers/ddr/altera/sdram_arria10.c @@ -660,9 +660,6 @@ static int ddr_calibration_sequence(struct altera_sdram_platdata *plat) else gd->ram_size = (u32)size;
- /* setup the dram info within bd */ - dram_init_banksize(); - if (of_sdram_firewall_setup(gd->fdt_blob)) puts("FW: Error Configuring Firewall\n");
participants (4)
-
Ley Foon Tan -
Marek Vasut -
Tan, Ley Foon
-
Tom Rini