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

[Zhi Li]

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?

best regards
Frank Li

>>  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.