diff mbox

[U-Boot,RFC,2/5] driver: ddr: fsl_mmdc: Pass board parameters through data structure

Message ID 1474498278-1637-3-git-send-email-york.sun@nxp.com
State Accepted
Commit 1fdcc8dfc7612acc765cd483051dcfaac399f4f1
Delegated to: York Sun
Headers show

Commit Message

York Sun Sept. 21, 2016, 10:51 p.m. UTC 
Instead of using multiple macros, a data structure is used to pass
board-specific parameters to MMDC DDR driver.

Signed-off-by: York Sun <york.sun@nxp.com>
CC: Shengzhou Liu <Shengzhou.Liu@nxp.com>
---
 board/freescale/ls1012afrdm/ls1012afrdm.c | 18 ++++++++++++++-
 board/freescale/ls1012aqds/ls1012aqds.c   | 18 ++++++++++++++-
 board/freescale/ls1012ardb/ls1012ardb.c   | 18 ++++++++++++++-
 drivers/ddr/fsl/fsl_mmdc.c                | 38 +++++++++++++++----------------
 include/configs/ls1012afrdm.h             | 16 -------------
 include/configs/ls1012aqds.h              | 16 -------------
 include/configs/ls1012ardb.h              | 15 ------------
 include/fsl_mmdc.h                        | 21 +++++++++++++----
 8 files changed, 87 insertions(+), 73 deletions(-)

Comments

Prabhakar Kushwaha Sept. 22, 2016, 3:04 a.m. UTC | #1 
> -----Original Message-----
> From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of York Sun
> Sent: Thursday, September 22, 2016 4:21 AM
> To: trini@konsulko.com
> Cc: u-boot@lists.denx.de
> Subject: [U-Boot] [RFC Patch 2/5] driver: ddr: fsl_mmdc: Pass board parameters
> through data structure
> 
> Instead of using multiple macros, a data structure is used to pass
> board-specific parameters to MMDC DDR driver.
> 
> Signed-off-by: York Sun <york.sun@nxp.com>
> CC: Shengzhou Liu <Shengzhou.Liu@nxp.com>


I doubt if it is one of the reason for root-cause.
May not be part of this patch set


> ---
>  board/freescale/ls1012afrdm/ls1012afrdm.c | 18 ++++++++++++++-
>  board/freescale/ls1012aqds/ls1012aqds.c   | 18 ++++++++++++++-
>  board/freescale/ls1012ardb/ls1012ardb.c   | 18 ++++++++++++++-
>  drivers/ddr/fsl/fsl_mmdc.c                | 38 +++++++++++++++----------------
>  include/configs/ls1012afrdm.h             | 16 -------------
>  include/configs/ls1012aqds.h              | 16 -------------
>  include/configs/ls1012ardb.h              | 15 ------------
>  include/fsl_mmdc.h                        | 21 +++++++++++++----
>  8 files changed, 87 insertions(+), 73 deletions(-)
> 
> diff --git a/board/freescale/ls1012afrdm/ls1012afrdm.c
> b/board/freescale/ls1012afrdm/ls1012afrdm.c
> index d644e94..b03bdb8 100644
> --- a/board/freescale/ls1012afrdm/ls1012afrdm.c
> +++ b/board/freescale/ls1012afrdm/ls1012afrdm.c
> @@ -26,7 +26,23 @@ int checkboard(void)
> 
>  int dram_init(void)
>  {
> -	mmdc_init();
> +	static const struct fsl_mmdc_info mparam = {
> +		0x04180000,	/* mdctl */
> +		0x00030035,	/* mdpdc */
> +		0x12554000,	/* mdotc */
> +		0xbabf7954,	/* mdcfg0 */
> +		0xdb328f64,	/* mdcfg1 */
> +		0x01ff00db,	/* mdcfg2 */
> +		0x00001680,	/* mdmisc */
> +		0x0f3c8000,	/* mdref */
> +		0x00002000,	/* mdrwd */
> +		0x00bf1023,	/* mdor */
> +		0x0000003f,	/* mdasp */
> +		0x0000022a,	/* mpodtctrl */
> +		0xa1390003,	/* mpzqhwctrl */
> +	};
> +
> +	mmdc_init(&mparam);
> 

Why cannot #define directly be used in fsl_mmdc.c.

If objective is to remove #define from board file, they even be required to avoid magic numbers. 

-prabhakar
Tom Rini Sept. 22, 2016, 11:19 a.m. UTC | #2 
On Thu, Sep 22, 2016 at 03:04:31AM +0000, Prabhakar Kushwaha wrote:
> 
> > -----Original Message-----
> > From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of York Sun
> > Sent: Thursday, September 22, 2016 4:21 AM
> > To: trini@konsulko.com
> > Cc: u-boot@lists.denx.de
> > Subject: [U-Boot] [RFC Patch 2/5] driver: ddr: fsl_mmdc: Pass board parameters
> > through data structure
> > 
> > Instead of using multiple macros, a data structure is used to pass
> > board-specific parameters to MMDC DDR driver.
> > 
> > Signed-off-by: York Sun <york.sun@nxp.com>
> > CC: Shengzhou Liu <Shengzhou.Liu@nxp.com>
> 
> 
> I doubt if it is one of the reason for root-cause.
> May not be part of this patch set
> 
> 
> > ---
> >  board/freescale/ls1012afrdm/ls1012afrdm.c | 18 ++++++++++++++-
> >  board/freescale/ls1012aqds/ls1012aqds.c   | 18 ++++++++++++++-
> >  board/freescale/ls1012ardb/ls1012ardb.c   | 18 ++++++++++++++-
> >  drivers/ddr/fsl/fsl_mmdc.c                | 38 +++++++++++++++----------------
> >  include/configs/ls1012afrdm.h             | 16 -------------
> >  include/configs/ls1012aqds.h              | 16 -------------
> >  include/configs/ls1012ardb.h              | 15 ------------
> >  include/fsl_mmdc.h                        | 21 +++++++++++++----
> >  8 files changed, 87 insertions(+), 73 deletions(-)
> > 
> > diff --git a/board/freescale/ls1012afrdm/ls1012afrdm.c
> > b/board/freescale/ls1012afrdm/ls1012afrdm.c
> > index d644e94..b03bdb8 100644
> > --- a/board/freescale/ls1012afrdm/ls1012afrdm.c
> > +++ b/board/freescale/ls1012afrdm/ls1012afrdm.c
> > @@ -26,7 +26,23 @@ int checkboard(void)
> > 
> >  int dram_init(void)
> >  {
> > -	mmdc_init();
> > +	static const struct fsl_mmdc_info mparam = {
> > +		0x04180000,	/* mdctl */
> > +		0x00030035,	/* mdpdc */
> > +		0x12554000,	/* mdotc */
> > +		0xbabf7954,	/* mdcfg0 */
> > +		0xdb328f64,	/* mdcfg1 */
> > +		0x01ff00db,	/* mdcfg2 */
> > +		0x00001680,	/* mdmisc */
> > +		0x0f3c8000,	/* mdref */
> > +		0x00002000,	/* mdrwd */
> > +		0x00bf1023,	/* mdor */
> > +		0x0000003f,	/* mdasp */
> > +		0x0000022a,	/* mpodtctrl */
> > +		0xa1390003,	/* mpzqhwctrl */
> > +	};
> > +
> > +	mmdc_init(&mparam);
> > 
> 
> Why cannot #define directly be used in fsl_mmdc.c.
> 
> If objective is to remove #define from board file, they even be required to avoid magic numbers. 

Please see the previous thread about this between York and I.  The end
goal is not to have no magic numbers, the end goal is to make it clear
what the magic numbers are doing.  I still wish there was also a link to
a tech note / manual / whatever to decode each of these values and a
comment about what DDR part is being used on the board, but this is a
step in the right direction.

And as a side bonus, this moves things one step closer to being able to
have more than one board supported by a given binary.
diff mbox

Patch

diff --git a/board/freescale/ls1012afrdm/ls1012afrdm.c b/board/freescale/ls1012afrdm/ls1012afrdm.c
index d644e94..b03bdb8 100644
--- a/board/freescale/ls1012afrdm/ls1012afrdm.c
+++ b/board/freescale/ls1012afrdm/ls1012afrdm.c
@@ -26,7 +26,23 @@  int checkboard(void)
 
 int dram_init(void)
 {
-	mmdc_init();
+	static const struct fsl_mmdc_info mparam = {
+		0x04180000,	/* mdctl */
+		0x00030035,	/* mdpdc */
+		0x12554000,	/* mdotc */
+		0xbabf7954,	/* mdcfg0 */
+		0xdb328f64,	/* mdcfg1 */
+		0x01ff00db,	/* mdcfg2 */
+		0x00001680,	/* mdmisc */
+		0x0f3c8000,	/* mdref */
+		0x00002000,	/* mdrwd */
+		0x00bf1023,	/* mdor */
+		0x0000003f,	/* mdasp */
+		0x0000022a,	/* mpodtctrl */
+		0xa1390003,	/* mpzqhwctrl */
+	};
+
+	mmdc_init(&mparam);
 
 	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
 
diff --git a/board/freescale/ls1012aqds/ls1012aqds.c b/board/freescale/ls1012aqds/ls1012aqds.c
index 188b6bc..94440b3 100644
--- a/board/freescale/ls1012aqds/ls1012aqds.c
+++ b/board/freescale/ls1012aqds/ls1012aqds.c
@@ -54,7 +54,23 @@  int checkboard(void)
 
 int dram_init(void)
 {
-	mmdc_init();
+	static const struct fsl_mmdc_info mparam = {
+		0x05180000,	/* mdctl */
+		0x00030035,	/* mdpdc */
+		0x12554000,	/* mdotc */
+		0xbabf7954,	/* mdcfg0 */
+		0xdb328f64,	/* mdcfg1 */
+		0x01ff00db,	/* mdcfg2 */
+		0x00001680,	/* mdmisc */
+		0x0f3c8000,	/* mdref */
+		0x00002000,	/* mdrwd */
+		0x00bf1023,	/* mdor */
+		0x0000003f,	/* mdasp */
+		0x0000022a,	/* mpodtctrl */
+		0xa1390003,	/* mpzqhwctrl */
+	};
+
+	mmdc_init(&mparam);
 
 	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
 
diff --git a/board/freescale/ls1012ardb/ls1012ardb.c b/board/freescale/ls1012ardb/ls1012ardb.c
index 50f9187..778434d 100644
--- a/board/freescale/ls1012ardb/ls1012ardb.c
+++ b/board/freescale/ls1012ardb/ls1012ardb.c
@@ -58,7 +58,23 @@  int checkboard(void)
 
 int dram_init(void)
 {
-	mmdc_init();
+	static const struct fsl_mmdc_info mparam = {
+		0x05180000,	/* mdctl */
+		0x00030035,	/* mdpdc */
+		0x12554000,	/* mdotc */
+		0xbabf7954,	/* mdcfg0 */
+		0xdb328f64,	/* mdcfg1 */
+		0x01ff00db,	/* mdcfg2 */
+		0x00001680,	/* mdmisc */
+		0x0f3c8000,	/* mdref */
+		0x00002000,	/* mdrwd */
+		0x00bf1023,	/* mdor */
+		0x0000003f,	/* mdasp */
+		0x0000022a,	/* mpodtctrl */
+		0xa1390003,	/* mpzqhwctrl */
+	};
+
+	mmdc_init(&mparam);
 
 	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;
 
diff --git a/drivers/ddr/fsl/fsl_mmdc.c b/drivers/ddr/fsl/fsl_mmdc.c
index 1e35967..52eec0f 100644
--- a/drivers/ddr/fsl/fsl_mmdc.c
+++ b/drivers/ddr/fsl/fsl_mmdc.c
@@ -26,7 +26,7 @@  static void set_wait_for_bits_clear(void *ptr, u32 value, u32 bits)
 		printf("Error: %p wait for clear timeout.\n", ptr);
 }
 
-void mmdc_init(void)
+void mmdc_init(const struct fsl_mmdc_info *priv)
 {
 	struct mmdc_regs *mmdc = (struct mmdc_regs *)CONFIG_SYS_FSL_DDR_ADDR;
 	unsigned int tmp;
@@ -35,26 +35,26 @@  void mmdc_init(void)
 	out_be32(&mmdc->mdscr, MDSCR_ENABLE_CON_REQ);
 
 	/* 2. configure the desired timing parameters */
-	out_be32(&mmdc->mdotc,  CONFIG_MMDC_MDOTC);
-	out_be32(&mmdc->mdcfg0, CONFIG_MMDC_MDCFG0);
-	out_be32(&mmdc->mdcfg1, CONFIG_MMDC_MDCFG1);
-	out_be32(&mmdc->mdcfg2, CONFIG_MMDC_MDCFG2);
+	out_be32(&mmdc->mdotc, priv->mdotc);
+	out_be32(&mmdc->mdcfg0, priv->mdcfg0);
+	out_be32(&mmdc->mdcfg1, priv->mdcfg1);
+	out_be32(&mmdc->mdcfg2, priv->mdcfg2);
 
 	/* 3. configure DDR type and other miscellaneous parameters */
-	out_be32(&mmdc->mdmisc, CONFIG_MMDC_MDMISC);
+	out_be32(&mmdc->mdmisc, priv->mdmisc);
 	out_be32(&mmdc->mpmur0,	MMDC_MPMUR0_FRC_MSR);
-	out_be32(&mmdc->mdrwd,  CONFIG_MMDC_MDRWD);
-	out_be32(&mmdc->mpodtctrl, CONFIG_MMDC_MPODTCTRL);
+	out_be32(&mmdc->mdrwd, priv->mdrwd);
+	out_be32(&mmdc->mpodtctrl, priv->mpodtctrl);
 
 	/* 4. configure the required delay while leaving reset */
-	out_be32(&mmdc->mdor,  CONFIG_MMDC_MDOR);
+	out_be32(&mmdc->mdor, priv->mdor);
 
 	/* 5. configure DDR physical parameters */
 	/* set row/column address width, burst length, data bus width */
-	tmp = CONFIG_MMDC_MDCTL & ~(MDCTL_SDE0 | MDCTL_SDE1);
+	tmp = priv->mdctl & ~(MDCTL_SDE0 | MDCTL_SDE1);
 	out_be32(&mmdc->mdctl, tmp);
 	/* configure address space partition */
-	out_be32(&mmdc->mdasp, CONFIG_MMDC_MDASP);
+	out_be32(&mmdc->mdasp, priv->mdasp);
 
 	/* 6. perform a ZQ calibration - not needed here, doing in #8b */
 
@@ -84,7 +84,7 @@  void mmdc_init(void)
 	out_be32(&mmdc->mdscr,  CMD_ADDR_MSB_MR_OP(0x4) | MDSCR_ENABLE_CON_REQ |
 				CMD_ZQ_CALIBRATION | CMD_BANK_ADDR_0);
 
-	set_wait_for_bits_clear(&mmdc->mpzqhwctrl, CONFIG_MMDC_MPZQHWCTRL,
+	set_wait_for_bits_clear(&mmdc->mpzqhwctrl, priv->mpzqhwctrl,
 				MPZQHWCTRL_ZQ_HW_FORCE);
 
 	/* 9a. calibrations now, wr lvl */
@@ -116,11 +116,11 @@  void mmdc_init(void)
 	out_be32(&mmdc->mppdcmpr2, MPPDCMPR2_MPR_COMPARE_EN);
 
 	/* set absolute read delay offset */
-#if defined(CONFIG_MMDC_MPRDDLCTL)
-	out_be32(&mmdc->mprddlctl, CONFIG_MMDC_MPRDDLCTL);
-#else
-	out_be32(&mmdc->mprddlctl, MMDC_MPRDDLCTL_DEFAULT_DELAY);
-#endif
+	if (priv->mprddlctl)
+		out_be32(&mmdc->mprddlctl, priv->mprddlctl);
+	else
+		out_be32(&mmdc->mprddlctl, MMDC_MPRDDLCTL_DEFAULT_DELAY);
+
 	set_wait_for_bits_clear(&mmdc->mpdgctrl0,
 				AUTO_RD_DQS_GATING_CALIBRATION_EN,
 				AUTO_RD_DQS_GATING_CALIBRATION_EN);
@@ -142,13 +142,13 @@  void mmdc_init(void)
 				CMD_BANK_ADDR_3);
 
 	/* 10. configure power-down, self-refresh entry, exit parameters */
-	out_be32(&mmdc->mdpdc, CONFIG_MMDC_MDPDC);
+	out_be32(&mmdc->mdpdc, priv->mdpdc);
 	out_be32(&mmdc->mapsr, MMDC_MAPSR_PWR_SAV_CTRL_STAT);
 
 	/* 11. ZQ config again? do nothing here */
 
 	/* 12. refresh scheme */
-	set_wait_for_bits_clear(&mmdc->mdref, CONFIG_MMDC_MDREF,
+	set_wait_for_bits_clear(&mmdc->mdref, priv->mdref,
 				MDREF_START_REFRESH);
 
 	/* 13. disable CON_REQ */
diff --git a/include/configs/ls1012afrdm.h b/include/configs/ls1012afrdm.h
index 136d648..612f243 100644
--- a/include/configs/ls1012afrdm.h
+++ b/include/configs/ls1012afrdm.h
@@ -20,22 +20,6 @@ 
 #define CONFIG_SYS_MEMTEST_START	0x80000000
 #define CONFIG_SYS_MEMTEST_END		0x9fffffff
 
-/* DDR board-specific timing parameters */
-#define CONFIG_MMDC_MDCTL	0x04180000
-#define CONFIG_MMDC_MDPDC	0x00030035
-#define CONFIG_MMDC_MDOTC	0x12554000
-#define CONFIG_MMDC_MDCFG0	0xbabf7954
-#define CONFIG_MMDC_MDCFG1	0xdb328f64
-#define CONFIG_MMDC_MDCFG2	0x01ff00db
-#define CONFIG_MMDC_MDMISC	0x00001680
-#define CONFIG_MMDC_MDREF	0x0f3c8000
-#define CONFIG_MMDC_MDRWD	0x00002000
-#define CONFIG_MMDC_MDOR	0x00bf1023
-#define CONFIG_MMDC_MDASP	0x0000003f
-#define CONFIG_MMDC_MPODTCTRL	0x0000022a
-#define CONFIG_MMDC_MPZQHWCTRL	0xa1390003
-
-
 /*
 * USB
 */
diff --git a/include/configs/ls1012aqds.h b/include/configs/ls1012aqds.h
index b6d12dd..54abf30 100644
--- a/include/configs/ls1012aqds.h
+++ b/include/configs/ls1012aqds.h
@@ -19,22 +19,6 @@ 
 #define CONFIG_SYS_MEMTEST_START	0x80000000
 #define CONFIG_SYS_MEMTEST_END		0x9fffffff
 
-/* DDR board-specific timing parameters */
-#define CONFIG_MMDC_MDCTL	0x05180000
-#define CONFIG_MMDC_MDPDC	0x00030035
-#define CONFIG_MMDC_MDOTC	0x12554000
-#define CONFIG_MMDC_MDCFG0	0xbabf7954
-#define CONFIG_MMDC_MDCFG1	0xdb328f64
-#define CONFIG_MMDC_MDCFG2	0x01ff00db
-#define CONFIG_MMDC_MDMISC	0x00001680
-#define CONFIG_MMDC_MDREF	0x0f3c8000
-#define CONFIG_MMDC_MDRWD	0x00002000
-#define CONFIG_MMDC_MDOR	0x00bf1023
-#define CONFIG_MMDC_MDASP	0x0000003f
-#define CONFIG_MMDC_MPODTCTRL	0x0000022a
-#define CONFIG_MMDC_MPZQHWCTRL	0xa1390003
-
-
 /*
  * QIXIS Definitions
  */
diff --git a/include/configs/ls1012ardb.h b/include/configs/ls1012ardb.h
index 2076ce5..0c13dde 100644
--- a/include/configs/ls1012ardb.h
+++ b/include/configs/ls1012ardb.h
@@ -19,21 +19,6 @@ 
 #define CONFIG_SYS_MEMTEST_START	0x80000000
 #define CONFIG_SYS_MEMTEST_END		0x9fffffff
 
-/* DDR board-specific timing parameters */
-#define CONFIG_MMDC_MDCTL	0x05180000
-#define CONFIG_MMDC_MDPDC	0x00030035
-#define CONFIG_MMDC_MDOTC	0x12554000
-#define CONFIG_MMDC_MDCFG0	0xbabf7954
-#define CONFIG_MMDC_MDCFG1	0xdb328f64
-#define CONFIG_MMDC_MDCFG2	0x01ff00db
-#define CONFIG_MMDC_MDMISC	0x00001680
-#define CONFIG_MMDC_MDREF	0x0f3c8000
-#define CONFIG_MMDC_MDRWD	0x00002000
-#define CONFIG_MMDC_MDOR	0x00bf1023
-#define CONFIG_MMDC_MDASP	0x0000003f
-#define CONFIG_MMDC_MPODTCTRL	0x0000022a
-#define CONFIG_MMDC_MPZQHWCTRL	0xa1390003
-
 /*
 * USB
 */
diff --git a/include/fsl_mmdc.h b/include/fsl_mmdc.h
index 1d09ff4..d5c4f8d 100644
--- a/include/fsl_mmdc.h
+++ b/include/fsl_mmdc.h
@@ -150,10 +150,23 @@  struct mmdc_regs {
 	u32 mpdccr;
 };
 
-void mmdc_init(void);
+struct fsl_mmdc_info {
+	u32 mdctl;
+	u32 mdpdc;
+	u32 mdotc;
+	u32 mdcfg0;
+	u32 mdcfg1;
+	u32 mdcfg2;
+	u32 mdmisc;
+	u32 mdref;
+	u32 mdrwd;
+	u32 mdor;
+	u32 mdasp;
+	u32 mpodtctrl;
+	u32 mpzqhwctrl;
+	u32 mprddlctl;
+};
 
-#if !defined(CONFIG_MMDC_MDCTL)
-#error Must configure board-specific timing CONFIG_MMDC_* in <board>.h for MMDC
-#endif
+void mmdc_init(const struct fsl_mmdc_info *);
 
 #endif /* FSL_MMDC_H */