Re: [U-Boot] [PATCH v2] arm: socfpga: Move Stratix 10 SDRAM driver to DM

26 Apr 2019

On Wed, Apr 24, 2019 at 11:01 PM Marek Vasut marex@denx.de wrote:
...
On 4/24/19 8:21 AM, Ley Foon Tan wrote:
...
Convert Stratix 10 SDRAM driver to device model.
Get rid of call to socfpga_per_reset() and use reset
framework.
SPL is changed from calling function in SDRAM driver
directly to just probing UCLASS_RAM.
Move sdram_s10.h from arch to driver/ddr/altera directory.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com
v1->v2:

Change sdr device tree node enabled by default.
Probe UCLASS_RAM only if CONFIG_ALTERA_SDRAM is enabled.


arch/arm/dts/socfpga_stratix10.dtsi           |   9 +
 arch/arm/mach-socfpga/spl_s10.c               |  11 +-
 configs/socfpga_stratix10_defconfig           |   1 +
 drivers/ddr/altera/Kconfig                    |   6 +-
 drivers/ddr/altera/sdram_s10.c                | 246 ++++++++++++------
 .../mach => drivers/ddr/altera}/sdram_s10.h   |   4 -
 include/configs/socfpga_stratix10_socdk.h     |   5 -
 7 files changed, 192 insertions(+), 90 deletions(-)
 rename {arch/arm/mach-socfpga/include/mach => drivers/ddr/altera}/sdram_s10.h (97%)

diff --git a/arch/arm/dts/socfpga_stratix10.dtsi b/arch/arm/dts/socfpga_stratix10.dtsi
index d1ae2fabae..bd68a78a37 100755
--- a/arch/arm/dts/socfpga_stratix10.dtsi
+++ b/arch/arm/dts/socfpga_stratix10.dtsi
@@ -258,6 +258,15 @@
                      u-boot,dm-pre-reloc;
              };

        sdr: sdr@f8000400 {


                 compatible = "altr,sdr-ctl-s10";


                 reg = <0xf8000400 0x80>,


                       <0xf8010000 0x190>,


                       <0xf8011000 0x500>;


                 resets = <&rst DDRSCH_RESET>;


                 u-boot,dm-pre-reloc;


         };


        spi0: spi@ffda4000 {
                compatible = "snps,dw-apb-ssi";
                #address-cells = <1>;



Separate patch please
Okay.
...
...
diff --git a/arch/arm/mach-socfpga/spl_s10.c b/arch/arm/mach-socfpga/spl_s10.c
index a141ffe82a..3d44eabf91 100644
--- a/arch/arm/mach-socfpga/spl_s10.c
+++ b/arch/arm/mach-socfpga/spl_s10.c
@@ -15,9 +15,9 @@
 #include <asm/arch/firewall_s10.h>
 #include <asm/arch/mailbox_s10.h>
 #include <asm/arch/reset_manager.h>
-#include <asm/arch/sdram_s10.h>
 #include <asm/arch/system_manager.h>
 #include <watchdog.h>
+#include <dm/uclass.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -119,6 +119,7 @@ void board_init_f(ulong dummy)
 {
      const struct cm_config *cm_default_cfg = cm_get_default_config();
      int ret;

struct udevice *dev;




#ifdef CONFIG_HW_WATCHDOG
      /* Ensure watchdog is paused when debugging is happening */
@@ -175,11 +176,13 @@ void board_init_f(ulong dummy)
      clrbits_le32(CCU_REG_ADDR(CCU_IOM_MPRT_ADMASK_MEM_RAM0),
                   CCU_ADMASK_P_MASK | CCU_ADMASK_NS_MASK);

debug("DDR: Initializing Hard Memory Controller\n");


if (sdram_mmr_init_full(0)) {


        puts("DDR: Initialization failed.\n");



+#ifdef CONFIG_ALTERA_SDRAM
#if CONFIG_IS_ENABLED(ALTERA_SDRAM) (really, altera sdram ? How will
this work with other SDRAM controllers in the FPGA ? I thought that was
already discussed too)
Tried change to  #if CONFIG_IS_ENABLED(ALTERA_SDRAM), but it is not
get compiled in SPL.
Found that it is only working if CONFIG consist of _SPL_ keyword, i.e:
CONFIG_SPL_ALTERA_SDRAM.
FPGA SDRAM doesn't really need a driver. So, we don't need to probe
UCLASS_RAM here.
...
...

ret = uclass_get_device(UCLASS_RAM, 0, &dev);


if (ret) {


        debug("DRAM init failed: %d\n", ret);
        hang();
}



+#endif /* CONFIG_ALTERA_SDRAM */
  mbox_init();


diff --git a/configs/socfpga_stratix10_defconfig b/configs/socfpga_stratix10_defconfig
index 4848013b21..bda6ab6637 100644
--- a/configs/socfpga_stratix10_defconfig
+++ b/configs/socfpga_stratix10_defconfig
@@ -32,6 +32,7 @@ CONFIG_ENV_IS_IN_MMC=y
 CONFIG_NET_RANDOM_ETHADDR=y
 CONFIG_SPL_DM=y
 CONFIG_SPL_DM_SEQ_ALIAS=y
+CONFIG_ALTERA_SDRAM=y
 CONFIG_DM_GPIO=y
 CONFIG_DWAPB_GPIO=y
 CONFIG_DM_I2C=y
diff --git a/drivers/ddr/altera/Kconfig b/drivers/ddr/altera/Kconfig
index 8f60b56eb8..112c4ad7c3 100644
--- a/drivers/ddr/altera/Kconfig
+++ b/drivers/ddr/altera/Kconfig
@@ -1,7 +1,7 @@
 config ALTERA_SDRAM
      bool "SoCFPGA DDR SDRAM driver"

depends on TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10


select RAM if TARGET_SOCFPGA_GEN5


select SPL_RAM if TARGET_SOCFPGA_GEN5




depends on TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_ARRIA10 || TARGET_SOCFPGA_STRATIX10


select RAM if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_STRATIX10


select SPL_RAM if TARGET_SOCFPGA_GEN5 || TARGET_SOCFPGA_STRATIX10
help
  Enable DDR SDRAM controller for the SoCFPGA devices.



diff --git a/drivers/ddr/altera/sdram_s10.c b/drivers/ddr/altera/sdram_s10.c
index e4d4a02ca2..d2f3272609 100644
--- a/drivers/ddr/altera/sdram_s10.c
+++ b/drivers/ddr/altera/sdram_s10.c
@@ -5,17 +5,32 @@
  */
#include <common.h>
+#include <dm.h>
 #include <errno.h>
 #include <div64.h>
 #include <fdtdec.h>
-#include <asm/io.h>
+#include <ram.h>
+#include <reset.h>
+#include "sdram_s10.h"
 #include <wait_bit.h>
 #include <asm/arch/firewall_s10.h>
-#include <asm/arch/sdram_s10.h>
 #include <asm/arch/system_manager.h>
 #include <asm/arch/reset_manager.h>
+#include <asm/io.h>
 #include <linux/sizes.h>
+#ifdef CONFIG_SPL_BUILD
Is this ifdef really needed ?
Yes, these code will compile into Uboot image as well.
...
...
+struct altera_sdram_priv {

struct ram_info info;



+};



+struct altera_sdram_platdata {

void __iomem *hmc;


void __iomem *ddr_sch;


void __iomem *iomhc;



+};



DECLARE_GLOBAL_DATA_PTR;
static const struct socfpga_system_manager *sysmgr_regs =
@@ -51,25 +66,26 @@ u32 ddr_config[] = {
      DDR_CONFIG(1, 4, 10, 17),
 };
-static u32 hmc_readl(u32 reg)
+static u32 hmc_readl(struct altera_sdram_platdata *plat, u32 reg)
 {

return readl(((void __iomem *)SOCFPGA_HMC_MMR_IO48_ADDRESS + (reg)));




return readl(plat->iomhc + (reg));



Superfluous parenthesis around (reg) , drop them, fix globally.
Okay.
...
[...]
...
+/**


sdram_calculate_size() - Calculate SDRAM size







Calculate SDRAM device size based on SDRAM controller parameters.



Size is specified in bytes.


*/

+static phys_size_t sdram_calculate_size(struct altera_sdram_platdata *plat)
+{

u32 dramaddrw = hmc_readl(plat, DRAMADDRW);



Is this a new piece of code ?
Not new. Just move this function to earlier of the file, before call
to this function.
...
...

phys_size_t size = 1 << (DRAMADDRW_CFG_CS_ADDR_WIDTH(dramaddrw) +


                 DRAMADDRW_CFG_BANK_GRP_ADDR_WIDTH(dramaddrw) +


                 DRAMADDRW_CFG_BANK_ADDR_WIDTH(dramaddrw) +


                 DRAMADDRW_CFG_ROW_ADDR_WIDTH(dramaddrw) +


                 DRAMADDRW_CFG_COL_ADDR_WIDTH(dramaddrw));



size *= (2 << (hmc_ecc_readl(plat, DDRIOCTRL) &


                DDR_HMC_DDRIOCTRL_IOSIZE_MSK));



return size;



+}



[...]
...
+static int altera_sdram_probe(struct udevice *dev)
+{

int ret;


struct reset_ctl_bulk resets;



ret = reset_get_bulk(dev, &resets);


if (ret) {


        dev_err(dev, "Can't get reset: %d\n", ret);


        return -ENODEV;


}


reset_deassert_bulk(&resets);



if (sdram_mmr_init_full(dev) != 0) {


        puts("SDRAM init failed.\n");


        goto failed;


}



return 0;




+failed:

reset_release_bulk(&resets);


return -ENODEV;



}
Are you missing altera_sdram_remove() , which would assert reset here ?
Will add it.
...
[...]
--

    