Re: [U-Boot] [PATCH 16/28] ddr: altera: sdram: Clean up sdram_mmr_init_full() part 4

[Dinh Nguyen]

On 8/1/15 4:34 PM, Marek Vasut wrote:
 Merge sdr_set_*() functions which are just setting registers among
 the sea of register setting in sdram_mmr_init_full(). There is no
 need to keep them separate this way, there is nothing special about
 them.
 
 Signed-off-by: Marek Vasut ma...@denx.de
 ---
  drivers/ddr/altera/sdram.c | 98 
 +-
  1 file changed, 36 insertions(+), 62 deletions(-)
 
 diff --git a/drivers/ddr/altera/sdram.c b/drivers/ddr/altera/sdram.c
 index 595f2a4..199e8b8 100644
 --- a/drivers/ddr/altera/sdram.c
 +++ b/drivers/ddr/altera/sdram.c
 @@ -501,24 +501,6 @@ static void set_sdr_ctrlcfg(struct socfpga_sdram_config 
 *cfg)
   writel(ctrl_cfg, sdr_ctrl-ctrl_cfg);
  }


snip

 @@ -586,7 +530,22 @@ unsigned sdram_mmr_init_full(unsigned int sdr_phy_reg)
   writel(rows, sysmgr_regs-iswgrp_handoff[4]);
  
   set_sdr_ctrlcfg(cfg);
 - set_sdr_dram_timing(cfg);
 +
 + debug(Configuring DRAMTIMING1\n);
 + writel(cfg-dram_timing1, sdr_ctrl-dram_timing1);
 +
 + debug(Configuring DRAMTIMING2\n);
 + writel(cfg-dram_timing2, sdr_ctrl-dram_timing2);
 +
 + debug(Configuring DRAMTIMING3\n);
 + writel(cfg-dram_timing3, sdr_ctrl-dram_timing3);
 +
 + debug(Configuring DRAMTIMING4\n);
 + writel(cfg-dram_timing4, sdr_ctrl-dram_timing4);
 +
 + debug(Configuring LOWPWRTIMING\n);
 + writel(cfg-lowpwr_timing, sdr_ctrl-lowpwr_timing);
 +

I don't think we need all of these debug prints?

Dinh
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 16/28] ddr: altera: sdram: Clean up sdram_mmr_init_full() part 4

[Marek Vasut]

On Wednesday, August 05, 2015 at 05:59:56 PM, Dinh Nguyen wrote:
 On 8/1/15 4:34 PM, Marek Vasut wrote:
  Merge sdr_set_*() functions which are just setting registers among
  the sea of register setting in sdram_mmr_init_full(). There is no
  need to keep them separate this way, there is nothing special about
  them.
  
  Signed-off-by: Marek Vasut ma...@denx.de
  ---
  
   drivers/ddr/altera/sdram.c | 98
   +- 1 file changed, 36
   insertions(+), 62 deletions(-)
  
  diff --git a/drivers/ddr/altera/sdram.c b/drivers/ddr/altera/sdram.c
  index 595f2a4..199e8b8 100644
  --- a/drivers/ddr/altera/sdram.c
  +++ b/drivers/ddr/altera/sdram.c
  @@ -501,24 +501,6 @@ static void set_sdr_ctrlcfg(struct
  socfpga_sdram_config *cfg)
  
  writel(ctrl_cfg, sdr_ctrl-ctrl_cfg);
   
   }
 
 snip
 
  @@ -586,7 +530,22 @@ unsigned sdram_mmr_init_full(unsigned int
  sdr_phy_reg)
  
  writel(rows, sysmgr_regs-iswgrp_handoff[4]);
  
  set_sdr_ctrlcfg(cfg);
  
  -   set_sdr_dram_timing(cfg);
  +
  +   debug(Configuring DRAMTIMING1\n);
  +   writel(cfg-dram_timing1, sdr_ctrl-dram_timing1);
  +
  +   debug(Configuring DRAMTIMING2\n);
  +   writel(cfg-dram_timing2, sdr_ctrl-dram_timing2);
  +
  +   debug(Configuring DRAMTIMING3\n);
  +   writel(cfg-dram_timing3, sdr_ctrl-dram_timing3);
  +
  +   debug(Configuring DRAMTIMING4\n);
  +   writel(cfg-dram_timing4, sdr_ctrl-dram_timing4);
  +
  +   debug(Configuring LOWPWRTIMING\n);
  +   writel(cfg-lowpwr_timing, sdr_ctrl-lowpwr_timing);
  +
 
 I don't think we need all of these debug prints?

Me neither ;-) I think there's wy too many (useless) debug() prints
throughout the entire DDR driver, not only here. I tried to preserve the
debug prints in their pristine state so far.

I think that in sequencer.c, the debug prints cause even more mess,
since there is a lot of code only to cater for printing debug stuff.
All kinda of variables get computed only to be used in some debug()
print and then discarded. I think a lot of code could be removed from
there if we discard those debug() prints.

I'd be happy if someone does a debug() print cleanup once things settle.
Would you like to prepare the patches ? :-)

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 16/28] ddr: altera: sdram: Clean up sdram_mmr_init_full() part 4

[Marek Vasut]

Merge sdr_set_*() functions which are just setting registers among
the sea of register setting in sdram_mmr_init_full(). There is no
need to keep them separate this way, there is nothing special about
them.

Signed-off-by: Marek Vasut ma...@denx.de
---
 drivers/ddr/altera/sdram.c | 98 +-
 1 file changed, 36 insertions(+), 62 deletions(-)

diff --git a/drivers/ddr/altera/sdram.c b/drivers/ddr/altera/sdram.c
index 595f2a4..199e8b8 100644
--- a/drivers/ddr/altera/sdram.c
+++ b/drivers/ddr/altera/sdram.c
@@ -501,24 +501,6 @@ static void set_sdr_ctrlcfg(struct socfpga_sdram_config 
*cfg)
writel(ctrl_cfg, sdr_ctrl-ctrl_cfg);
 }
 
-static void set_sdr_dram_timing(struct socfpga_sdram_config *cfg)
-{
-   debug(Configuring DRAMTIMING1\n);
-   writel(cfg-dram_timing1, sdr_ctrl-dram_timing1);
-
-   debug(Configuring DRAMTIMING2\n);
-   writel(cfg-dram_timing2, sdr_ctrl-dram_timing2);
-
-   debug(Configuring DRAMTIMING3\n);
-   writel(cfg-dram_timing3, sdr_ctrl-dram_timing3);
-
-   debug(Configuring DRAMTIMING4\n);
-   writel(cfg-dram_timing4, sdr_ctrl-dram_timing4);
-
-   debug(Configuring LOWPWRTIMING\n);
-   writel(cfg-lowpwr_timing, sdr_ctrl-lowpwr_timing);
-}
-
 static void set_sdr_addr_rw(struct socfpga_sdram_config *cfg)
 {
/*
@@ -536,44 +518,6 @@ static void set_sdr_addr_rw(struct socfpga_sdram_config 
*cfg)
   sdr_ctrl-dram_addrw);
 }
 
-static void set_sdr_static_cfg(struct socfpga_sdram_config *cfg)
-{
-   debug(Configuring STATICCFG\n);
-   writel(cfg-static_cfg, sdr_ctrl-static_cfg);
-}
-
-static void set_sdr_fifo_cfg(struct socfpga_sdram_config *cfg)
-{
-   debug(Configuring FIFOCFG\n);
-   writel(cfg-fifo_cfg, sdr_ctrl-fifo_cfg);
-}
-
-static void set_sdr_mp_weight(struct socfpga_sdram_config *cfg)
-{
-   debug(Configuring MPWEIGHT_MPWEIGHT_0\n);
-   writel(cfg-mp_weight0, sdr_ctrl-mp_weight0);
-   writel(cfg-mp_weight1, sdr_ctrl-mp_weight1);
-   writel(cfg-mp_weight2, sdr_ctrl-mp_weight2);
-   writel(cfg-mp_weight3, sdr_ctrl-mp_weight3);
-}
-
-static void set_sdr_mp_pacing(struct socfpga_sdram_config *cfg)
-{
-   debug(Configuring MPPACING_MPPACING_0\n);
-   writel(cfg-mp_pacing0, sdr_ctrl-mp_pacing0);
-   writel(cfg-mp_pacing1, sdr_ctrl-mp_pacing1);
-   writel(cfg-mp_pacing2, sdr_ctrl-mp_pacing2);
-   writel(cfg-mp_pacing3, sdr_ctrl-mp_pacing3);
-}
-
-static void set_sdr_mp_threshold(struct socfpga_sdram_config *cfg)
-{
-   debug(Configuring MPTHRESHOLDRST_MPTHRESHOLDRST_0\n);
-   writel(cfg-mp_threshold0, sdr_ctrl-mp_threshold0);
-   writel(cfg-mp_threshold1, sdr_ctrl-mp_threshold1);
-   writel(cfg-mp_threshold2, sdr_ctrl-mp_threshold2);
-}
-
 /* Function to initialize SDRAM MMR */
 unsigned sdram_mmr_init_full(unsigned int sdr_phy_reg)
 {
@@ -586,7 +530,22 @@ unsigned sdram_mmr_init_full(unsigned int sdr_phy_reg)
writel(rows, sysmgr_regs-iswgrp_handoff[4]);
 
set_sdr_ctrlcfg(cfg);
-   set_sdr_dram_timing(cfg);
+
+   debug(Configuring DRAMTIMING1\n);
+   writel(cfg-dram_timing1, sdr_ctrl-dram_timing1);
+
+   debug(Configuring DRAMTIMING2\n);
+   writel(cfg-dram_timing2, sdr_ctrl-dram_timing2);
+
+   debug(Configuring DRAMTIMING3\n);
+   writel(cfg-dram_timing3, sdr_ctrl-dram_timing3);
+
+   debug(Configuring DRAMTIMING4\n);
+   writel(cfg-dram_timing4, sdr_ctrl-dram_timing4);
+
+   debug(Configuring LOWPWRTIMING\n);
+   writel(cfg-lowpwr_timing, sdr_ctrl-lowpwr_timing);
+
set_sdr_addr_rw(cfg);
 
debug(Configuring DRAMIFWIDTH\n);
@@ -601,7 +560,8 @@ unsigned sdram_mmr_init_full(unsigned int sdr_phy_reg)
debug(Configuring DRAMINTR\n);
writel(cfg-dram_intr, sdr_ctrl-dram_intr);
 
-   set_sdr_static_cfg(cfg);
+   debug(Configuring STATICCFG\n);
+   writel(cfg-static_cfg, sdr_ctrl-static_cfg);
 
debug(Configuring CTRLWIDTH\n);
writel(cfg-ctrl_width, sdr_ctrl-ctrl_width);
@@ -609,14 +569,28 @@ unsigned sdram_mmr_init_full(unsigned int sdr_phy_reg)
debug(Configuring PORTCFG\n);
writel(cfg-port_cfg, sdr_ctrl-port_cfg);
 
-   set_sdr_fifo_cfg(cfg);
+   debug(Configuring FIFOCFG\n);
+   writel(cfg-fifo_cfg, sdr_ctrl-fifo_cfg);
 
debug(Configuring MPPRIORITY\n);
writel(cfg-mp_priority, sdr_ctrl-mp_priority);
 
-   set_sdr_mp_weight(cfg);
-   set_sdr_mp_pacing(cfg);
-   set_sdr_mp_threshold(cfg);
+   debug(Configuring MPWEIGHT_MPWEIGHT_0\n);
+   writel(cfg-mp_weight0, sdr_ctrl-mp_weight0);
+   writel(cfg-mp_weight1, sdr_ctrl-mp_weight1);
+   writel(cfg-mp_weight2, sdr_ctrl-mp_weight2);
+   writel(cfg-mp_weight3, sdr_ctrl-mp_weight3);
+
+   debug(Configuring MPPACING_MPPACING_0\n);
+   writel(cfg-mp_pacing0, sdr_ctrl-mp_pacing0);
+   writel(cfg-mp_pacing1, sdr_ctrl-mp_pacing1);
+   writel(cfg-mp_pacing2,