[PATCH 0/7] ddr: altera: arria10: Convert SDRAM driver to DM

[PATCH 0/7] ddr: altera: arria10: Convert SDRAM driver to DM

[Ley Foon Tan]

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%)

-- 
2.19.0

[PATCH 1/7] ddr: altera: arria10: Fix incorrect address for mpu1

[Ley Foon Tan]

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
-- 
2.19.0

[PATCH 1/7] ddr: altera: arria10: Fix incorrect address for mpu1

[Marek Vasut]

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 ?

[...]

[PATCH 1/7] ddr: altera: arria10: Fix incorrect address for mpu1

[Tan, Ley Foon]

> -----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 at 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

[PATCH 2/7] ddr: altera: arria10: Move SDRAM driver to DM

[Ley Foon Tan]

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),
+};
-- 
2.19.0

[PATCH 2/7] ddr: altera: arria10: Move SDRAM driver to DM

[Marek Vasut]

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 ?

[...]

[PATCH 2/7] ddr: altera: arria10: Move SDRAM driver to DM

[Tan, Ley Foon]

> -----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 at 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

[PATCH 2/7] ddr: altera: arria10: Move SDRAM driver to DM

[Marek Vasut]

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.

[PATCH 3/7] ddr: altera: arria10: Change to use reset DM function

[Ley Foon Tan]

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;
-- 
2.19.0

[PATCH 3/7] ddr: altera: arria10: Change to use reset DM function

[Marek Vasut]

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.

[PATCH 3/7] ddr: altera: arria10: Change to use reset DM function

[Tan, Ley Foon]

> -----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 at 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

[PATCH 4/7] arm: socfpga: arria10: Move sdram_arria10.h to drivers/ddr/altera

[Ley Foon Tan]

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
-- 
2.19.0

[PATCH 5/7] ddr: altera: arria10: Add RAM size check

[Ley Foon Tan]

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:
-- 
2.19.0

[PATCH 5/7] ddr: altera: arria10: Add RAM size check

[Marek Vasut]

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 ?

[PATCH 5/7] ddr: altera: arria10: Add RAM size check

[Tan, Ley Foon]

> -----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 at 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

[PATCH 5/7] ddr: altera: arria10: Add RAM size check

[Marek Vasut]

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?

[PATCH 5/7] ddr: altera: arria10: Add RAM size check

[Tan, Ley Foon]

> -----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 at 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

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Ley Foon Tan]

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");
-- 
2.19.0

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Marek Vasut]

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.

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Tom Rini]

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>

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200415/574b3cf1/attachment.sig>

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Marek Vasut]

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.

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200415/4402a4de/attachment.sig>

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Marek Vasut]

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.

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200415/4ac1811b/attachment.sig>

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Marek Vasut]

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

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200416/d0897cfd/attachment.sig>

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Marek Vasut]

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.

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200416/ccb10a68/attachment.sig>

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Marek Vasut]

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-for-v2020.07

It didn't take too long to find a counter-example ...

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Tom Rini]

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-for-v2020.07
> 
> 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. 

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200416/42e83785/attachment.sig>

[PATCH 6/7] ddr: altera: arria10: Change %i to %u for printf

[Marek Vasut]

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-for-v2020.07
>>
>> 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.'

[PATCH 7/7] ddr: altera: arria10: Remove call to dram_init_banksize()

[Ley Foon Tan]

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");
 
-- 
2.19.0