[PATCH v2 5/9] ARM: imx: add msl support for imx7d

[Russell King - ARM Linux]

On Mon, Apr 20, 2015 at 10:55:12AM -0500, Zhi Li wrote:
> On Fri, Apr 17, 2015 at 6:04 PM, Russell King - ARM Linux
> <linux at arm.linux.org.uk> wrote:
> > On Fri, Apr 17, 2015 at 03:17:36AM +0800, Frank.Li at freescale.com wrote:
> >> From: Anson Huang <b20788 at freescale.com>
> >>
> >> Add i.MX7D MSL support.
> >>
> >> Signed-off-by: Anson Huang <b20788 at freescale.com>
> >> Signed-off-by: Frank Li <Frank.Li at freescale.com>
> >
> > I'm not happy with this.
> >
> 
> Sorry, what cause your unhappy?
> Can you tell me detail?

Please see comments in the rest of the email.

> >>  void imx_anatop_pre_suspend(void)
> >>  {
> >> +     if (cpu_is_imx7d()) {
> >> +             /* PLL overwrite set */
> >> +             regmap_write(anatop, ANADIG_ARM_PLL + REG_SET, 1 << 20);
> >> +             regmap_write(anatop, ANADIG_DDR_PLL + REG_SET, 1 << 19);
> >> +             regmap_write(anatop, ANADIG_SYS_PLL + REG_SET, 1 << 17);
> >> +             regmap_write(anatop, ANADIG_ENET_PLL + REG_SET, 1 << 13);
> >> +             regmap_write(anatop, ANADIG_AUDIO_PLL + REG_SET, 1 << 24);
> >> +             regmap_write(anatop, ANADIG_VIDEO_PLL + REG_SET, 1 << 24);
> >> +             return;
> >> +     }
> >> +
> >>       if (imx_mmdc_get_ddr_type() == IMX_DDR_TYPE_LPDDR2)
> >>               imx_anatop_enable_2p5_pulldown(true);
> >>       else
> >> @@ -86,6 +104,17 @@ void imx_anatop_pre_suspend(void)
> >>
> >>  void imx_anatop_post_resume(void)
> >>  {
> >> +     if (cpu_is_imx7d()) {
> >> +             /* PLL overwrite clear */
> >> +             regmap_write(anatop, ANADIG_ARM_PLL + REG_CLR, 1 << 20);
> >> +             regmap_write(anatop, ANADIG_DDR_PLL + REG_CLR, 1 << 19);
> >> +             regmap_write(anatop, ANADIG_SYS_PLL + REG_CLR, 1 << 17);
> >> +             regmap_write(anatop, ANADIG_ENET_PLL + REG_CLR, 1 << 13);
> >> +             regmap_write(anatop, ANADIG_AUDIO_PLL + REG_CLR, 1 << 24);
> >> +             regmap_write(anatop, ANADIG_VIDEO_PLL + REG_CLR, 1 << 24);
> >> +             return;
> >> +     }
> >
> > Shouldn't both of these be determined by the compatible string rather
> > than checking __mxc_cpu_type?
> >
> >> diff --git a/arch/arm/mach-imx/cpu.c b/arch/arm/mach-imx/cpu.c
> >> index df42c14..4e84a7a 100644
> >> --- a/arch/arm/mach-imx/cpu.c
> >> +++ b/arch/arm/mach-imx/cpu.c
> >> @@ -11,6 +11,8 @@
> >>
> >>  unsigned int __mxc_cpu_type;
> >>  EXPORT_SYMBOL(__mxc_cpu_type);
> >> +unsigned int __mxc_arch_type;
> >> +EXPORT_SYMBOL(__mxc_arch_type);
> >>
> >>  static unsigned int imx_soc_revision;
> >>
> >> @@ -19,6 +21,11 @@ void mxc_set_cpu_type(unsigned int type)
> >>       __mxc_cpu_type = type;
> >>  }
> >>
> >> +void mxc_set_arch_type(unsigned int type)
> >> +{
> >> +     __mxc_arch_type = type;
> >> +}
> >
> > You're re-inventing the wheel here.  Please get rid of __mxc_arch_type
> > and mxc_set_arch_type() entirely.
> >
> >> @@ -38,6 +38,8 @@
> >>  #define MXC_CPU_IMX6DL               0x61
> >>  #define MXC_CPU_IMX6SX               0x62
> >>  #define MXC_CPU_IMX6Q                0x63
> >> +#define MXC_CPU_IMX7D                0x72
> >> +#define MXC_ARCH_CA7         0xc07
> >
> > Get rid of MXC_ARCH_CA7.
> >
> >> +static inline bool arm_is_ca7(void)
> >> +{
> >> +     return __mxc_arch_type == MXC_ARCH_CA7;
> >> +}
> >
> > And this function.
> >
> >> diff --git a/arch/arm/mach-imx/platsmp.c b/arch/arm/mach-imx/platsmp.c
> >> index 7f27001..2ca4a41 100644
> >> --- a/arch/arm/mach-imx/platsmp.c
> >> +++ b/arch/arm/mach-imx/platsmp.c
> >> @@ -60,11 +60,33 @@ static int imx_boot_secondary(unsigned int cpu, struct task_struct *idle)
> >>  static void __init imx_smp_init_cpus(void)
> >>  {
> >>       int i, ncores;
> >> +     unsigned long arch_type;
> >>
> >> -     ncores = scu_get_core_count(scu_base);
> >> +     asm volatile(
> >> +             ".align 4\n"
> >> +             "mrc p15, 0, %0, c0, c0, 0\n"
> >> +             : "=r" (arch_type)
> >> +     );
> >> +
> >> +     /* MIDR[15:4] defines ARCH type */
> >> +     mxc_set_arch_type((arch_type >> 4) & 0xfff);
> >> +
> >> +     if (arm_is_ca7()) {
> >
> > Rather than the above, use:
> >
> >         if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A7) {
> >
> > here.
> >
> > Even better would be to _not_ re-use imx_smp_init_cpus(), but instead
> > declare a separate smp_operations structure for the CA7 iMX7 which
> > takes account of this difference by calling a seperate .smp_init_cpus
> > function.
> >
> > Thanks.
> >
> > --
> > FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
> > according to speedtest.net.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.