diff mbox

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

Message ID 1499953762-3597-7-git-send-email-patrice.chotard@st.com
State Superseded
Headers show

Commit Message

Patrice CHOTARD July 13, 2017, 1:49 p.m. UTC 
From: Patrice Chotard <patrice.chotard@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@st.com>
Acked-by: Vikas MANOCHA <vikas.manocha@st.com>
---
 drivers/ram/stm32_sdram.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

Simon Glass July 18, 2017, 2:01 p.m. UTC | #1 
On 13 July 2017 at 06:49,  <patrice.chotard@st.com> wrote:
> From: Patrice Chotard <patrice.chotard@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@st.com>
> Acked-by: Vikas MANOCHA <vikas.manocha@st.com>
> ---
>  drivers/ram/stm32_sdram.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)

Reviewed-by: Simon Glass <sjg@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.

> +
>  /* 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
function handle the register (which is presumably something like u32)
correctly?

> +
>         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
Patrice CHOTARD July 18, 2017, 2:48 p.m. UTC | #2 
Hi Simon

On 07/18/2017 04:01 PM, Simon Glass wrote:
> On 13 July 2017 at 06:49,  <patrice.chotard@st.com> wrote:

>> From: Patrice Chotard <patrice.chotard@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@st.com>

>> Acked-by: Vikas MANOCHA <vikas.manocha@st.com>

>> ---

>>   drivers/ram/stm32_sdram.c | 23 ++++++++++++++++++++++-

>>   1 file changed, 22 insertions(+), 1 deletion(-)

> 

> Reviewed-by: Simon Glass <sjg@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

>
diff mbox

Patch

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*/
+
 /* 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);
+
 	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 },
 	{ }
 };