From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ley Foon Tan Date: Tue, 30 Apr 2019 15:40:54 +0800 Subject: [U-Boot] [PATCH v2] arm: socfpga: Move Stratix 10 SDRAM driver to DM In-Reply-To: <9fdf5c74-cfd8-b551-9f6b-313a799c5a4c@denx.de> References: <1556086869-13501-1-git-send-email-ley.foon.tan@intel.com> <9fdf5c74-cfd8-b551-9f6b-313a799c5a4c@denx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Mon, Apr 29, 2019 at 5:55 PM Marek Vasut wrote: > > On 4/26/19 10:23 AM, Ley Foon Tan wrote: > > On Wed, Apr 24, 2019 at 11:01 PM Marek Vasut 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 > >>> --- > >>> 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 > >>> #include > >>> #include > >>> -#include > >>> #include > >>> #include > >>> +#include > >>> > >>> 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 > >>> +#include > >>> #include > >>> #include > >>> #include > >>> -#include > >>> +#include > >>> +#include > >>> +#include "sdram_s10.h" > >>> #include > >>> #include > >>> -#include > >>> #include > >>> #include > >>> +#include > >>> #include > >>> > >>> +#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