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

Ley Foon Tan lftan.linux at gmail.com
Tue Apr 30 07:40:54 UTC 2019


On Mon, Apr 29, 2019 at 5:55 PM Marek Vasut <marex at 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 at 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 at 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 at f8000400 {
> >>> +                      compatible = "altr,sdr-ctl-s10";
> >>> +                      reg = <0xf8000400 0x80>,
> >>> +                            <0xf8010000 0x190>,
> >>> +                            <0xf8011000 0x500>;
> >>> +                      resets = <&rst DDRSCH_RESET>;
> >>> +                      u-boot,dm-pre-reloc;
> >>> +              };
> >>> +
> >>>               spi0: spi at 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


More information about the U-Boot mailing list