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

[Marek Vasut <marex@denx.de>]

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

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

-- 
Best regards,
Marek Vasut