| Message ID | 1305751670-30808-3-git-send-email-fabio.estevam@freescale.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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(-)