[U-Boot] [PATCH 6/6] ram: stm32: add stm32h7 support

[Patrice CHOTARD]

Hi Simon

On 07/18/2017 04:01 PM, Simon Glass wrote:
> On 13 July 2017 at 06:49,  <patrice.chotard at st.com> wrote:
>> From: Patrice Chotard <patrice.chotard at st.com>
>>
>> STM32F7 and H7 shared the same SDRAM control block.
>> On STM32H7 few control bits has been added.
>> The current driver need some minor adaptation as FMC block
>> enable/disable for H7.
>>
>> Signed-off-by: Patrice Chotard <patrice.chotard at st.com>
>> Acked-by: Vikas MANOCHA <vikas.manocha at st.com>
>> ---
>>   drivers/ram/stm32_sdram.c | 23 ++++++++++++++++++++++-
>>   1 file changed, 22 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> nit / question below
> 
>>
>> diff --git a/drivers/ram/stm32_sdram.c b/drivers/ram/stm32_sdram.c
>> index 5e7539c..e27c6a8 100644
>> --- a/drivers/ram/stm32_sdram.c
>> +++ b/drivers/ram/stm32_sdram.c
>> @@ -54,6 +54,10 @@ struct stm32_fmc_regs {
>>          u32 sdsr;       /* SDRAM Status register */
>>   };
>>
>> +/* NOR/PSRAM Control register BCR1 */
>> +#define FMC_BCR1_FMCEN         31      /* FMC controller Enable, only availabe
>> +                                          for H7*/
> 
> Use the normal comment style for multi-line comments. Here I think you
> should merge these two comments and put them on the line before the
> #define.

ok

> 
>> +
>>   /* Control register SDCR */
>>   #define FMC_SDCR_RPIPE_SHIFT   13      /* RPIPE bit shift */
>>   #define FMC_SDCR_RBURST_SHIFT  12      /* RBURST bit shift */
>> @@ -123,6 +127,11 @@ enum stm32_fmc_bank {
>>          MAX_SDRAM_BANK,
>>   };
>>
>> +enum stm32_fmc_family {
>> +       STM32F7_FMC,
>> +       STM32H7_FMC,
>> +};
>> +
>>   struct bank_params {
>>          struct stm32_sdram_control *sdram_control;
>>          struct stm32_sdram_timing *sdram_timing;
>> @@ -134,6 +143,7 @@ struct stm32_sdram_params {
>>          struct stm32_fmc_regs *base;
>>          u8 no_sdram_banks;
>>          struct bank_params bank_params[MAX_SDRAM_BANK];
>> +       enum stm32_fmc_family family;
>>   };
>>
>>   #define SDRAM_MODE_BL_SHIFT    0
>> @@ -151,6 +161,10 @@ int stm32_sdram_init(struct udevice *dev)
>>          u32 ref_count;
>>          u8 i;
>>
>> +       /* disable the FMC controller */
>> +       if (params->family == STM32H7_FMC)
>> +               generic_clear_bit(FMC_BCR1_FMCEN, (unsigned long *)&regs->bcr1);
> 
> I'm not sure what generic_clear_bit() is...is it different from
> clrbits_le32()? But why the cast to unsigned long *? Shouldn't the

cast (unsigned long *) is imposed by generic_clear_bit()

> function handle the register (which is presumably something like u32)
> correctly?

But ok i will use clrbits_le32() which fit also my needs and avoid to 
cast register address.

> 
>> +
>>          for (i = 0; i < params->no_sdram_banks; i++) {
>>                  control = params->bank_params[i].sdram_control;
>>                  timing = params->bank_params[i].sdram_timing;
>> @@ -193,6 +207,7 @@ int stm32_sdram_init(struct udevice *dev)
>>                                  | timing->txsr << FMC_SDTR_TXSR_SHIFT
>>                                  | timing->tmrd << FMC_SDTR_TMRD_SHIFT,
>>                                  &regs->sdtr2);
>> +
>>                  if (target_bank == SDRAM_BANK1)
>>                          ctb = FMC_SDCMR_BANK_1;
>>                  else
>> @@ -225,6 +240,10 @@ int stm32_sdram_init(struct udevice *dev)
>>                  writel(ref_count << 1, &regs->sdrtr);
>>          }
>>
>> +       /* enable the FMC controller */
>> +       if (params->family == STM32H7_FMC)
>> +               generic_set_bit(FMC_BCR1_FMCEN, (unsigned long *)&regs->bcr1);
>> +
>>          return 0;
>>   }
>>
>> @@ -305,6 +324,7 @@ static int stm32_fmc_probe(struct udevice *dev)
>>                  return -EINVAL;
>>
>>          params->base = (struct stm32_fmc_regs *)addr;
>> +       params->family = dev_get_driver_data(dev);
>>
>>   #ifdef CONFIG_CLK
>>          struct clk clk;
>> @@ -337,7 +357,8 @@ static struct ram_ops stm32_fmc_ops = {
>>   };
>>
>>   static const struct udevice_id stm32_fmc_ids[] = {
>> -       { .compatible = "st,stm32-fmc" },
>> +       { .compatible = "st,stm32-fmc", .data = STM32F7_FMC },
>> +       { .compatible = "st,stm32h7-fmc", .data = STM32H7_FMC },
>>          { }
>>   };
>>
>> --
>> 1.9.1
>>
> 
> Regards,
> Simon
>