diff mbox series

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

Message ID 1556086869-13501-1-git-send-email-ley.foon.tan@intel.com
State Superseded
Delegated to: Marek Vasut
Headers show
Series [U-Boot,v2] arm: socfpga: Move Stratix 10 SDRAM driver to DM | expand

Commit Message

Ley Foon Tan April 24, 2019, 6:21 a.m. UTC
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%)

Comments

Marek Vasut April 24, 2019, 2:43 p.m. UTC | #1
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

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

> +	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 ?

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

[...]

> +/**
> + * 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 ?

> +	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 ?

[...]
Ley Foon Tan April 26, 2019, 8:23 a.m. UTC | #2
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.
>
> [...]
>
> --
Marek Vasut April 29, 2019, 8:32 a.m. UTC | #3
On 4/26/19 10:23 AM, Ley Foon Tan wrote:
> 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.

Isn't that exactly what you want -- compile the DRAM driver only into SPL ?

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

No, it won't, the code will only be compiled into the SPL with this
ifdef. But that's something you should solve on the Kconfig/Kbuild level.

The way forward here is to create a new Kconfig symbol, some
CONFIG_SPL_ALTERA_STRATIX10_DRAM or somesuch and then add a new entry
into drivers/ddr/altera/Makefile to only compile this entire code when
that symbol is enabled.

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

OK

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

But won't that prevent the DRAM controller from working ?
Ley Foon Tan April 30, 2019, 7:40 a.m. UTC | #4
On Mon, Apr 29, 2019 at 5:55 PM Marek Vasut <marex@denx.de> wrote:
>
> On 4/26/19 10:23 AM, Ley Foon Tan wrote:
> > 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.
>
> Isn't that exactly what you want -- compile the DRAM driver only into SPL ?
Yes. Will change to 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.
>
> No, it won't, the code will only be compiled into the SPL with this
> ifdef. But that's something you should solve on the Kconfig/Kbuild level.
>
> The way forward here is to create a new Kconfig symbol, some
> CONFIG_SPL_ALTERA_STRATIX10_DRAM or somesuch and then add a new entry
> into drivers/ddr/altera/Makefile to only compile this entire code when
> that symbol is enabled.

Will change ALTERA_SDRAM to SPL_ALTERA_SDRAM and compile in SPL only.

>
> >>
> >>> +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.
>
> OK
>
> >>> +     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.
>
> But won't that prevent the DRAM controller from working ?
Added assert reset in _remove, SDRAM controller still can work after
boot to Uboot.
Seem _remove is not called when boot to Uboot.

Regards
Ley Foon
Marek Vasut April 30, 2019, 9:56 a.m. UTC | #5
On 4/30/19 9:40 AM, Ley Foon Tan wrote:
[...]
>>>>> +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.
>>
>> But won't that prevent the DRAM controller from working ?
> Added assert reset in _remove, SDRAM controller still can work after
> boot to Uboot.
> Seem _remove is not called when boot to Uboot.

Look at DM_FLAG_OS_PREPARE
Ley Foon Tan May 2, 2019, 3:46 a.m. UTC | #6
On Tue, Apr 30, 2019 at 5:56 PM Marek Vasut <marex@denx.de> wrote:
>
> On 4/30/19 9:40 AM, Ley Foon Tan wrote:
> [...]
> >>>>> +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.
> >>
> >> But won't that prevent the DRAM controller from working ?
> > Added assert reset in _remove, SDRAM controller still can work after
> > boot to Uboot.
> > Seem _remove is not called when boot to Uboot.
>
> Look at DM_FLAG_OS_PREPARE
Tested add DM_FLAG_OS_PREPARE flag, but still didn't see it call to _remove().

BTW, I think we shouldn't assert SDRAM controller. Otherwise, SDRAM is
not working in next boot stage.

Regards
Ley Foon
Marek Vasut May 2, 2019, 10:17 a.m. UTC | #7
On 5/2/19 5:46 AM, Ley Foon Tan wrote:
> On Tue, Apr 30, 2019 at 5:56 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 4/30/19 9:40 AM, Ley Foon Tan wrote:
>> [...]
>>>>>>> +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.
>>>>
>>>> But won't that prevent the DRAM controller from working ?
>>> Added assert reset in _remove, SDRAM controller still can work after
>>> boot to Uboot.
>>> Seem _remove is not called when boot to Uboot.
>>
>> Look at DM_FLAG_OS_PREPARE
> Tested add DM_FLAG_OS_PREPARE flag, but still didn't see it call to _remove().
> 
> BTW, I think we shouldn't assert SDRAM controller. Otherwise, SDRAM is
> not working in next boot stage.

Hence my question above -- "But won't that prevent the DRAM controller
from working ?"

I think you're right.
diff mbox series

Patch

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>;
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
+	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
+
+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));
 }
 
-static u32 hmc_ecc_readl(u32 reg)
+static u32 hmc_ecc_readl(struct altera_sdram_platdata *plat, u32 reg)
 {
-	return readl((void __iomem *)SOCFPGA_SDR_ADDRESS + (reg));
+	return readl(plat->hmc + (reg));
 }
 
-static u32 hmc_ecc_writel(u32 data, u32 reg)
+static u32 hmc_ecc_writel(struct altera_sdram_platdata *plat,
+			  u32 data, u32 reg)
 {
-	return writel(data, (void __iomem *)SOCFPGA_SDR_ADDRESS + (reg));
+	return writel(data, plat->hmc + (reg));
 }
 
-static u32 ddr_sch_writel(u32 data, u32 reg)
+static u32 ddr_sch_writel(struct altera_sdram_platdata *plat, u32 data,
+			  u32 reg)
 {
-	return writel(data,
-		      (void __iomem *)SOCFPGA_SDR_SCHEDULER_ADDRESS + (reg));
+	return writel(data, plat->ddr_sch + (reg));
 }
 
 int match_ddr_conf(u32 ddr_conf)
@@ -83,37 +99,38 @@  int match_ddr_conf(u32 ddr_conf)
 	return 0;
 }
 
-static int emif_clear(void)
+static int emif_clear(struct altera_sdram_platdata *plat)
 {
-	hmc_ecc_writel(0, RSTHANDSHAKECTRL);
+	hmc_ecc_writel(plat, 0, RSTHANDSHAKECTRL);
 
-	return wait_for_bit_le32((const void *)(SOCFPGA_SDR_ADDRESS +
+	return wait_for_bit_le32((const void *)(plat->hmc +
 				 RSTHANDSHAKESTAT),
 				 DDR_HMC_RSTHANDSHAKE_MASK,
 				 false, 1000, false);
 }
 
-static int emif_reset(void)
+static int emif_reset(struct altera_sdram_platdata *plat)
 {
 	u32 c2s, s2c, ret;
 
-	c2s = hmc_ecc_readl(RSTHANDSHAKECTRL) & DDR_HMC_RSTHANDSHAKE_MASK;
-	s2c = hmc_ecc_readl(RSTHANDSHAKESTAT) & DDR_HMC_RSTHANDSHAKE_MASK;
+	c2s = hmc_ecc_readl(plat, RSTHANDSHAKECTRL) & DDR_HMC_RSTHANDSHAKE_MASK;
+	s2c = hmc_ecc_readl(plat, RSTHANDSHAKESTAT) & DDR_HMC_RSTHANDSHAKE_MASK;
 
 	debug("DDR: c2s=%08x s2c=%08x nr0=%08x nr1=%08x nr2=%08x dst=%08x\n",
-	      c2s, s2c, hmc_readl(NIOSRESERVED0), hmc_readl(NIOSRESERVED1),
-	      hmc_readl(NIOSRESERVED2), hmc_readl(DRAMSTS));
+	      c2s, s2c, hmc_readl(plat, NIOSRESERVED0),
+	      hmc_readl(plat, NIOSRESERVED1), hmc_readl(plat, NIOSRESERVED2),
+	      hmc_readl(plat, DRAMSTS));
 
-	if (s2c && emif_clear()) {
+	if (s2c && emif_clear(plat)) {
 		printf("DDR: emif_clear() failed\n");
 		return -1;
 	}
 
 	debug("DDR: Triggerring emif reset\n");
-	hmc_ecc_writel(DDR_HMC_CORE2SEQ_INT_REQ, RSTHANDSHAKECTRL);
+	hmc_ecc_writel(plat, DDR_HMC_CORE2SEQ_INT_REQ, RSTHANDSHAKECTRL);
 
 	/* if seq2core[3] = 0, we are good */
-	ret = wait_for_bit_le32((const void *)(SOCFPGA_SDR_ADDRESS +
+	ret = wait_for_bit_le32((const void *)(plat->hmc +
 				 RSTHANDSHAKESTAT),
 				 DDR_HMC_SEQ2CORE_INT_RESP_MASK,
 				 false, 1000, false);
@@ -122,7 +139,7 @@  static int emif_reset(void)
 		return ret;
 	}
 
-	ret = emif_clear();
+	ret = emif_clear(plat);
 	if (ret) {
 		printf("DDR: emif_clear() failed\n");
 		return ret;
@@ -240,13 +257,37 @@  static void sdram_size_check(bd_t *bd)
 	debug("DDR: SDRAM size check passed!\n");
 }
 
+/**
+ * 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);
+
+	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;
+}
+
 /**
  * sdram_mmr_init_full() - Function to initialize SDRAM MMR
  *
  * Initialize the SDRAM MMR.
  */
-int sdram_mmr_init_full(unsigned int unused)
+static int sdram_mmr_init_full(struct udevice *dev)
 {
+	struct altera_sdram_platdata *plat = dev->platdata;
+	struct altera_sdram_priv *priv = dev_get_priv(dev);
 	u32 update_value, io48_value, ddrioctl;
 	u32 i;
 	int ret;
@@ -303,19 +344,16 @@  int sdram_mmr_init_full(unsigned int unused)
 		return -1;
 	}
 
-	/* release DDR scheduler from reset */
-	socfpga_per_reset(SOCFPGA_RESET(SDR), 0);
-
 	/* Try 3 times to do a calibration */
 	for (i = 0; i < 3; i++) {
-		ret = wait_for_bit_le32((const void *)(SOCFPGA_SDR_ADDRESS +
+		ret = wait_for_bit_le32((const void *)(plat->hmc +
 					DDRCALSTAT),
 					DDR_HMC_DDRCALSTAT_CAL_MSK, true, 1000,
 					false);
 		if (!ret)
 			break;
 
-		emif_reset();
+		emif_reset(plat);
 	}
 
 	if (ret) {
@@ -324,16 +362,16 @@  int sdram_mmr_init_full(unsigned int unused)
 	}
 	debug("DDR: Calibration success\n");
 
-	u32 ctrlcfg0 = hmc_readl(CTRLCFG0);
-	u32 ctrlcfg1 = hmc_readl(CTRLCFG1);
-	u32 dramaddrw = hmc_readl(DRAMADDRW);
-	u32 dramtim0 = hmc_readl(DRAMTIMING0);
-	u32 caltim0 = hmc_readl(CALTIMING0);
-	u32 caltim1 = hmc_readl(CALTIMING1);
-	u32 caltim2 = hmc_readl(CALTIMING2);
-	u32 caltim3 = hmc_readl(CALTIMING3);
-	u32 caltim4 = hmc_readl(CALTIMING4);
-	u32 caltim9 = hmc_readl(CALTIMING9);
+	u32 ctrlcfg0 = hmc_readl(plat, CTRLCFG0);
+	u32 ctrlcfg1 = hmc_readl(plat, CTRLCFG1);
+	u32 dramaddrw = hmc_readl(plat, DRAMADDRW);
+	u32 dramtim0 = hmc_readl(plat, DRAMTIMING0);
+	u32 caltim0 = hmc_readl(plat, CALTIMING0);
+	u32 caltim1 = hmc_readl(plat, CALTIMING1);
+	u32 caltim2 = hmc_readl(plat, CALTIMING2);
+	u32 caltim3 = hmc_readl(plat, CALTIMING3);
+	u32 caltim4 = hmc_readl(plat, CALTIMING4);
+	u32 caltim9 = hmc_readl(plat, CALTIMING9);
 
 	/*
 	 * Configure the DDR IO size [0xFFCFB008]
@@ -349,12 +387,12 @@  int sdram_mmr_init_full(unsigned int unused)
 	 *	bit[9:6] = Minor Release #
 	 *	bit[14:10] = Major Release #
 	 */
-	update_value = hmc_readl(NIOSRESERVED0);
-	hmc_ecc_writel(((update_value & 0xFF) >> 5), DDRIOCTRL);
-	ddrioctl = hmc_ecc_readl(DDRIOCTRL);
+	update_value = hmc_readl(plat, NIOSRESERVED0);
+	hmc_ecc_writel(plat, ((update_value & 0xFF) >> 5), DDRIOCTRL);
+	ddrioctl = hmc_ecc_readl(plat, DDRIOCTRL);
 
 	/* enable HPS interface to HMC */
-	hmc_ecc_writel(DDR_HMC_HPSINTFCSEL_ENABLE_MASK, HPSINTFCSEL);
+	hmc_ecc_writel(plat, DDR_HMC_HPSINTFCSEL_ENABLE_MASK, HPSINTFCSEL);
 
 	/* Set the DDR Configuration */
 	io48_value = DDR_CONFIG(CTRLCFG1_CFG_ADDR_ORDER(ctrlcfg1),
@@ -365,10 +403,10 @@  int sdram_mmr_init_full(unsigned int unused)
 
 	update_value = match_ddr_conf(io48_value);
 	if (update_value)
-		ddr_sch_writel(update_value, DDR_SCH_DDRCONF);
+		ddr_sch_writel(plat, update_value, DDR_SCH_DDRCONF);
 
 	/* Configure HMC dramaddrw */
-	hmc_ecc_writel(hmc_readl(DRAMADDRW), DRAMADDRWIDTH);
+	hmc_ecc_writel(plat, hmc_readl(plat, DRAMADDRW), DRAMADDRWIDTH);
 
 	/*
 	 * Configure DDR timing
@@ -392,7 +430,7 @@  int sdram_mmr_init_full(unsigned int unused)
 		      CALTIMING0_CFG_ACT_TO_RDWR(caltim0) +
 		      CALTIMING4_CFG_PCH_TO_VALID(caltim4));
 
-	ddr_sch_writel(((CALTIMING0_CFG_ACT_TO_ACT(caltim0) <<
+	ddr_sch_writel(plat, ((CALTIMING0_CFG_ACT_TO_ACT(caltim0) <<
 			 DDR_SCH_DDRTIMING_ACTTOACT_OFF) |
 			(update_value << DDR_SCH_DDRTIMING_RDTOMISS_OFF) |
 			(io48_value << DDR_SCH_DDRTIMING_WRTOMISS_OFF) |
@@ -406,12 +444,12 @@  int sdram_mmr_init_full(unsigned int unused)
 			DDR_SCH_DDRTIMING);
 
 	/* Configure DDR mode [precharge = 0] */
-	ddr_sch_writel(((ddrioctl ? 0 : 1) <<
+	ddr_sch_writel(plat, ((ddrioctl ? 0 : 1) <<
 			 DDR_SCH_DDRMOD_BWRATIOEXTENDED_OFF),
 			DDR_SCH_DDRMODE);
 
 	/* Configure the read latency */
-	ddr_sch_writel((DRAMTIMING0_CFG_TCL(dramtim0) >> 1) +
+	ddr_sch_writel(plat, (DRAMTIMING0_CFG_TCL(dramtim0) >> 1) +
 			DDR_READ_LATENCY_DELAY,
 			DDR_SCH_READ_LATENCY);
 
@@ -419,7 +457,7 @@  int sdram_mmr_init_full(unsigned int unused)
 	 * Configuring timing values concerning activate commands
 	 * [FAWBANK alway 1 because always 4 bank DDR]
 	 */
-	ddr_sch_writel(((CALTIMING0_CFG_ACT_TO_ACT_DB(caltim0) <<
+	ddr_sch_writel(plat, ((CALTIMING0_CFG_ACT_TO_ACT_DB(caltim0) <<
 			 DDR_SCH_ACTIVATE_RRD_OFF) |
 			(CALTIMING9_CFG_4_ACT_TO_ACT(caltim9) <<
 			 DDR_SCH_ACTIVATE_FAW_OFF) |
@@ -431,7 +469,7 @@  int sdram_mmr_init_full(unsigned int unused)
 	 * Configuring timing values concerning device to device data bus
 	 * ownership change
 	 */
-	ddr_sch_writel(((CALTIMING1_CFG_RD_TO_RD_DC(caltim1) <<
+	ddr_sch_writel(plat, ((CALTIMING1_CFG_RD_TO_RD_DC(caltim1) <<
 			 DDR_SCH_DEVTODEV_BUSRDTORD_OFF) |
 			(CALTIMING1_CFG_RD_TO_WR_DC(caltim1) <<
 			 DDR_SCH_DEVTODEV_BUSRDTOWR_OFF) |
@@ -440,7 +478,7 @@  int sdram_mmr_init_full(unsigned int unused)
 			DDR_SCH_DEVTODEV);
 
 	/* assigning the SDRAM size */
-	unsigned long long size = sdram_calculate_size();
+	unsigned long long size = sdram_calculate_size(plat);
 	/* If the size is invalid, use default Config size */
 	if (size <= 0)
 		hw_size = PHYS_SDRAM_1_SIZE;
@@ -462,18 +500,17 @@  int sdram_mmr_init_full(unsigned int unused)
 
 	/* Enable or disable the SDRAM ECC */
 	if (CTRLCFG1_CFG_CTRL_EN_ECC(ctrlcfg1)) {
-		setbits_le32(SOCFPGA_SDR_ADDRESS + ECCCTRL1,
+		setbits_le32(plat->hmc + ECCCTRL1,
 			     (DDR_HMC_ECCCTL_AWB_CNT_RST_SET_MSK |
 			      DDR_HMC_ECCCTL_CNT_RST_SET_MSK |
 			      DDR_HMC_ECCCTL_ECC_EN_SET_MSK));
-		clrbits_le32(SOCFPGA_SDR_ADDRESS + ECCCTRL1,
+		clrbits_le32(plat->hmc + ECCCTRL1,
 			     (DDR_HMC_ECCCTL_AWB_CNT_RST_SET_MSK |
 			      DDR_HMC_ECCCTL_CNT_RST_SET_MSK));
-		setbits_le32(SOCFPGA_SDR_ADDRESS + ECCCTRL2,
+		setbits_le32(plat->hmc + ECCCTRL2,
 			     (DDR_HMC_ECCCTL2_RMW_EN_SET_MSK |
 			      DDR_HMC_ECCCTL2_AWB_EN_SET_MSK));
-		writel(DDR_HMC_ERRINTEN_INTMASK,
-		       SOCFPGA_SDR_ADDRESS + ERRINTENS);
+		hmc_ecc_writel(plat, DDR_HMC_ERRINTEN_INTMASK, ERRINTENS);
 
 		/* Enable non-secure writes to HMC Adapter for SDRAM ECC */
 		writel(FW_HMC_ADAPTOR_MPU_MASK, FW_HMC_ADAPTOR_REG_ADDR);
@@ -482,39 +519,100 @@  int sdram_mmr_init_full(unsigned int unused)
 		if (!cpu_has_been_warmreset())
 			sdram_init_ecc_bits(&bd);
 	} else {
-		clrbits_le32(SOCFPGA_SDR_ADDRESS + ECCCTRL1,
+		clrbits_le32(plat->hmc + ECCCTRL1,
 			     (DDR_HMC_ECCCTL_AWB_CNT_RST_SET_MSK |
 			      DDR_HMC_ECCCTL_CNT_RST_SET_MSK |
 			      DDR_HMC_ECCCTL_ECC_EN_SET_MSK));
-		clrbits_le32(SOCFPGA_SDR_ADDRESS + ECCCTRL2,
+		clrbits_le32(plat->hmc + ECCCTRL2,
 			     (DDR_HMC_ECCCTL2_RMW_EN_SET_MSK |
 			      DDR_HMC_ECCCTL2_AWB_EN_SET_MSK));
 	}
 
 	sdram_size_check(&bd);
 
+	priv->info.base = bd.bi_dram[0].start;
+	priv->info.size = gd->ram_size;
+
 	debug("DDR: HMC init success\n");
 	return 0;
 }
 
-/**
- * sdram_calculate_size() - Calculate SDRAM size
- *
- * Calculate SDRAM device size based on SDRAM controller parameters.
- * Size is specified in bytes.
- */
-phys_size_t sdram_calculate_size(void)
+static int altera_sdram_ofdata_to_platdata(struct udevice *dev)
 {
-	u32 dramaddrw = hmc_readl(DRAMADDRW);
+	struct altera_sdram_platdata *plat = dev->platdata;
+	fdt_addr_t addr;
 
-	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));
+	addr = dev_read_addr_index(dev, 0);
+	if (addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+	plat->ddr_sch = (void __iomem *)addr;
 
-	size *= (2 << (hmc_ecc_readl(DDRIOCTRL) &
-			DDR_HMC_DDRIOCTRL_IOSIZE_MSK));
+	addr = dev_read_addr_index(dev, 1);
+	if (addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+	plat->iomhc = (void __iomem *)addr;
 
-	return size;
+	addr = dev_read_addr_index(dev, 2);
+	if (addr == FDT_ADDR_T_NONE)
+		return -EINVAL;
+	plat->hmc = (void __iomem *)addr;
+
+	return 0;
+}
+
+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;
 }
+
+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-s10" },
+	{ /* 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),
+};
+
+#endif /* CONFIG_SPL_BUILD */
diff --git a/arch/arm/mach-socfpga/include/mach/sdram_s10.h b/drivers/ddr/altera/sdram_s10.h
similarity index 97%
rename from arch/arm/mach-socfpga/include/mach/sdram_s10.h
rename to drivers/ddr/altera/sdram_s10.h
index f39206ca1e..096c06cba2 100644
--- a/arch/arm/mach-socfpga/include/mach/sdram_s10.h
+++ b/drivers/ddr/altera/sdram_s10.h
@@ -7,10 +7,6 @@ 
 #ifndef	_SDRAM_S10_H_
 #define	_SDRAM_S10_H_
 
-phys_size_t sdram_calculate_size(void);
-int sdram_mmr_init_full(unsigned int sdr_phy_reg);
-int sdram_calibration_full(void);
-
 #define DDR_TWR				15
 #define DDR_READ_LATENCY_DELAY		40
 #define DDR_ACTIVATE_FAWBANK		0x1
diff --git a/include/configs/socfpga_stratix10_socdk.h b/include/configs/socfpga_stratix10_socdk.h
index 31c267f55d..2af808cadf 100644
--- a/include/configs/socfpga_stratix10_socdk.h
+++ b/include/configs/socfpga_stratix10_socdk.h
@@ -129,11 +129,6 @@  unsigned int cm_get_qspi_controller_clk_hz(void);
 #define CONFIG_SYS_MEMTEST_START	0
 #define CONFIG_SYS_MEMTEST_END		PHYS_SDRAM_1_SIZE - 0x200000
 
-/*
- * SDRAM controller
- */
-#define CONFIG_ALTERA_SDRAM
-
 /*
  * Serial / UART configurations
  */