Patchwork [U-Boot,v3,3/4] MX5: Introduce a function for setting the chip select size

login
register
mail settings
Submitter Fabio Estevam
Date May 18, 2011, 8:47 p.m.
Message ID <1305751670-30808-3-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/96226/
State Changes Requested
Headers show

Comments

Fabio Estevam - May 18, 2011, 8:47 p.m.
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/cpu/armv7/mx5/soc.c              |   30 +++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-mx5/imx-regs.h  |    5 ++++
 arch/arm/include/asm/arch-mx5/sys_proto.h |    2 +-
 3 files changed, 36 insertions(+), 1 deletions(-)
Stefano Babic - May 19, 2011, 8:46 a.m.
On 05/18/2011 10:47 PM, Fabio Estevam wrote:
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---

Hi Fabio,

> +void set_chipselect_size(int const cs_size)
> +{
> +	unsigned int reg;
> +	struct iomuxc *iomuxc_regs = (struct weim *)IOMUXC_BASE_ADDR;
> +	reg = readl(&iomuxc_regs->gpr1);
> +
> +	switch (cs_size) {
> +	case CS0_128:
> +		reg &= ~0x7;	/* CS0=128MB, CS1=0, CS2=0, CS3=0 */
> +		reg |= 0x5;
> +		break;
> +	case CS0_64M_CS1_64M:
> +		reg &= ~0x3F;	/* CS0=64MB, CS1=64MB, CS2=0, CS3=0 */
> +		reg |= 0x1B;
> +		break;
> +	case CS0_64M_CS1_32M_CS2_32M:
> +		reg &= ~0x1FF;	/* CS0=64MB, CS1=32MB, CS2=32MB, CS3=0 */
> +		reg |= 0x4B;
> +		break;
> +	case CS0_32M_CS1_32M_CS2_32M_CS3_32M:
> +		reg &= ~0xFFF;  /* CS0=32MB, CS1=32MB, CS2=32MB, CS3=32MB */
> +		reg |= 0x249;
> +		break;
> +	default:
> +		printf("Unknown chip select size\n");
> +		break;
> +	}
> +
> +	writel(reg, &iomuxc_regs->gpr1);
> +}

mmmhhh...it seems to me not complete, because not all combinations are
covered. And setting fixed values in the switch constraints us to have
very long defines, as CS0_32M_CS1_32M_CS2_32M_CS3_32M.

What about to do in another way ? In the register, there are four bits
for each chip select, and the value to be set can then easy shifted to
the right place. You could define an enum with

CS_SIZE_32M = 0,
CS_SIZE_64M,
CS_SIZE_128

and use it as size. The function could take the chipselect as parameter,
and you could set the register with something like (size | ACT_CS) <<
(CS_SHIFT * chipselect), with CS_SHIFT=3. Then all cases are covered.

Best regards,
Stefano Babic
Fabio Estevam - May 19, 2011, 11:49 a.m.
Hi Stefano,

On Thu, May 19, 2011 at 5:46 AM, Stefano Babic <sbabic@denx.de> wrote:
> On 05/18/2011 10:47 PM, Fabio Estevam wrote:
>> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
>> ---
>
> Hi Fabio,
>
>> +void set_chipselect_size(int const cs_size)
>> +{
>> +     unsigned int reg;
>> +     struct iomuxc *iomuxc_regs = (struct weim *)IOMUXC_BASE_ADDR;
>> +     reg = readl(&iomuxc_regs->gpr1);
>> +
>> +     switch (cs_size) {
>> +     case CS0_128:
>> +             reg &= ~0x7;    /* CS0=128MB, CS1=0, CS2=0, CS3=0 */
>> +             reg |= 0x5;
>> +             break;
>> +     case CS0_64M_CS1_64M:
>> +             reg &= ~0x3F;   /* CS0=64MB, CS1=64MB, CS2=0, CS3=0 */
>> +             reg |= 0x1B;
>> +             break;
>> +     case CS0_64M_CS1_32M_CS2_32M:
>> +             reg &= ~0x1FF;  /* CS0=64MB, CS1=32MB, CS2=32MB, CS3=0 */
>> +             reg |= 0x4B;
>> +             break;
>> +     case CS0_32M_CS1_32M_CS2_32M_CS3_32M:
>> +             reg &= ~0xFFF;  /* CS0=32MB, CS1=32MB, CS2=32MB, CS3=32MB */
>> +             reg |= 0x249;
>> +             break;
>> +     default:
>> +             printf("Unknown chip select size\n");
>> +             break;
>> +     }
>> +
>> +     writel(reg, &iomuxc_regs->gpr1);
>> +}
>
> mmmhhh...it seems to me not complete, because not all combinations are
> covered.

Yes, it is complete. Only these four combinations are allowed as per
the MX53 Reference Manual.

>And setting fixed values in the switch constraints us to have
> very long defines, as CS0_32M_CS1_32M_CS2_32M_CS3_32M.

I can change the very long defines if you want.

I thought initially on doing the generic function as you described,
but then we would need to check for only the 4 valid combinations.
Then I came with this implementation that only handle the 4 possible
cases.

Let me know what you think.

Regards,

Fabio Estevam
Stefano Babic - May 19, 2011, 12:04 p.m.
On 05/19/2011 01:49 PM, Fabio Estevam wrote:
> Hi Stefano,
> 

Hi Fabio,

>>
>> mmmhhh...it seems to me not complete, because not all combinations are
>> covered.
> 
> Yes, it is complete. Only these four combinations are allowed as per
> the MX53 Reference Manual.
> 
>> And setting fixed values in the switch constraints us to have
>> very long defines, as CS0_32M_CS1_32M_CS2_32M_CS3_32M.
> 
> I can change the very long defines if you want.
> 
> I thought initially on doing the generic function as you described,
> but then we would need to check for only the 4 valid combinations.
> Then I came with this implementation that only handle the 4 possible
> cases.

I made the same mistake and I was convinced that all combinations are
possible. I understand why you changed in this way and I agree with this
implementation, thanks for clarification.

Acked-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic
Wolfgang Denk - May 19, 2011, 7:02 p.m.
Dear Fabio Estevam,

In message <1305751670-30808-3-git-send-email-fabio.estevam@freescale.com> you wrote:
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/cpu/armv7/mx5/soc.c              |   30 +++++++++++++++++++++++++++++
>  arch/arm/include/asm/arch-mx5/imx-regs.h  |    5 ++++
>  arch/arm/include/asm/arch-mx5/sys_proto.h |    2 +-
>  3 files changed, 36 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/mx5/soc.c b/arch/arm/cpu/armv7/mx5/soc.c
> index 40b8b56..e599df8 100644
> --- a/arch/arm/cpu/armv7/mx5/soc.c
> +++ b/arch/arm/cpu/armv7/mx5/soc.c
> @@ -163,6 +163,36 @@ int cpu_mmc_init(bd_t *bis)
>  #endif
>  }
>  
> +void set_chipselect_size(int const cs_size)
> +{
> +	unsigned int reg;
> +	struct iomuxc *iomuxc_regs = (struct weim *)IOMUXC_BASE_ADDR;
> +	reg = readl(&iomuxc_regs->gpr1);
> +
> +	switch (cs_size) {
> +	case CS0_128:
> +		reg &= ~0x7;	/* CS0=128MB, CS1=0, CS2=0, CS3=0 */
> +		reg |= 0x5;
> +		break;
> +	case CS0_64M_CS1_64M:
> +		reg &= ~0x3F;	/* CS0=64MB, CS1=64MB, CS2=0, CS3=0 */
> +		reg |= 0x1B;
> +		break;
> +	case CS0_64M_CS1_32M_CS2_32M:
> +		reg &= ~0x1FF;	/* CS0=64MB, CS1=32MB, CS2=32MB, CS3=0 */
> +		reg |= 0x4B;
> +		break;
> +	case CS0_32M_CS1_32M_CS2_32M_CS3_32M:
> +		reg &= ~0xFFF;  /* CS0=32MB, CS1=32MB, CS2=32MB, CS3=32MB */
> +		reg |= 0x249;
> +		break;
> +	default:
> +		printf("Unknown chip select size\n");

In cases like this, please _always_ print _what_ the unknown chip size
was.  Please fix.

Best regards,

Wolfgang Denk

Patch

diff --git a/arch/arm/cpu/armv7/mx5/soc.c b/arch/arm/cpu/armv7/mx5/soc.c
index 40b8b56..e599df8 100644
--- a/arch/arm/cpu/armv7/mx5/soc.c
+++ b/arch/arm/cpu/armv7/mx5/soc.c
@@ -163,6 +163,36 @@  int cpu_mmc_init(bd_t *bis)
 #endif
 }
 
+void set_chipselect_size(int const cs_size)
+{
+	unsigned int reg;
+	struct iomuxc *iomuxc_regs = (struct weim *)IOMUXC_BASE_ADDR;
+	reg = readl(&iomuxc_regs->gpr1);
+
+	switch (cs_size) {
+	case CS0_128:
+		reg &= ~0x7;	/* CS0=128MB, CS1=0, CS2=0, CS3=0 */
+		reg |= 0x5;
+		break;
+	case CS0_64M_CS1_64M:
+		reg &= ~0x3F;	/* CS0=64MB, CS1=64MB, CS2=0, CS3=0 */
+		reg |= 0x1B;
+		break;
+	case CS0_64M_CS1_32M_CS2_32M:
+		reg &= ~0x1FF;	/* CS0=64MB, CS1=32MB, CS2=32MB, CS3=0 */
+		reg |= 0x4B;
+		break;
+	case CS0_32M_CS1_32M_CS2_32M_CS3_32M:
+		reg &= ~0xFFF;  /* CS0=32MB, CS1=32MB, CS2=32MB, CS3=32MB */
+		reg |= 0x249;
+		break;
+	default:
+		printf("Unknown chip select size\n");
+		break;
+	}
+
+	writel(reg, &iomuxc_regs->gpr1);
+}
 
 void reset_cpu(ulong addr)
 {
diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h b/arch/arm/include/asm/arch-mx5/imx-regs.h
index 9d2046a..5163614 100644
--- a/arch/arm/include/asm/arch-mx5/imx-regs.h
+++ b/arch/arm/include/asm/arch-mx5/imx-regs.h
@@ -214,6 +214,11 @@ 
 #define WDOG_EN(x)	((x) << 8)
 #define WDOG_LIMIT(x)	(((x) & 0x3) << 9)
 
+#define CS0_128					0
+#define CS0_64M_CS1_64M				1
+#define CS0_64M_CS1_32M_CS2_32M			2
+#define CS0_32M_CS1_32M_CS2_32M_CS3_32M		3
+
 /*
  * Number of GPIO pins per port
  */
diff --git a/arch/arm/include/asm/arch-mx5/sys_proto.h b/arch/arm/include/asm/arch-mx5/sys_proto.h
index f687503..ce63675 100644
--- a/arch/arm/include/asm/arch-mx5/sys_proto.h
+++ b/arch/arm/include/asm/arch-mx5/sys_proto.h
@@ -27,5 +27,5 @@ 
 u32 get_cpu_rev(void);
 #define is_soc_rev(rev)	((get_cpu_rev() & 0xFF) - rev)
 void sdelay(unsigned long);
-
+void set_chipselect_size(int const);
 #endif