Re: [PATCH 1/3] Add smp support for Allwinner A20(sunxi 7i).

[Russell King - ARM Linux]

On Sat, Sep 21, 2013 at 09:05:14PM +0800, Fan Rong wrote:
> @@ -0,0 +1,12 @@
> +#include <linux/linkage.h>
> +#include <linux/init.h>
> +
> +        .section ".text.head", "ax"

Please use a tab instead of 8 spaces.

> +	__CPUINIT

__CPUINIT has been removed.

> +
> +ENTRY(sun7i_secondary_startup)
> +	msr	cpsr_fsxc, #0xd3
> +	mov r0, #0
> +	ldr r1, =0xffffffff

You don't  need to set r0 nor r1 here.

> +	b       secondary_startup
> +ENDPROC(sun7i_secondary_startup)
> diff --git a/arch/arm/mach-sunxi/platform.h b/arch/arm/mach-sunxi/platform.h
> new file mode 100644
> index 0000000..7e127c9
> --- /dev/null
> +++ b/arch/arm/mach-sunxi/platform.h
> @@ -0,0 +1,347 @@
> +/*
> + * arch/arm/plat-sunxi/include/plat/platform.h
> + *
> + * (C) Copyright 2007-2012
> + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
> + * Benn Huang <benn@xxxxxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA

Just the first paragraph is sufficient.  Also are you sure you want the
"or any later version"?  The kernel is predominantly a GPLv2 only project.

What are the origins of this file - do you have the ability to change this?

> +	/* step3: clear power-off gating */
> +	pwr_reg = readl(sunxi7i_cc_base + AW_CPU1_PWROFF_REG);
> +	pwr_reg &= ~(1);

I think I've said this before.  The parens around '1' are not required.
What do you think you gain by having the additional parens?

> +	writel(pwr_reg, sunxi7i_cc_base + AW_CPU1_PWROFF_REG);
> +	mdelay(1);
> +
> +	/* step4: de-assert core reset */
> +	writel(3, sunxi7i_cc_base + CPUX_RESET_CTL(cpu));
> +
> +	/* step5: assert DBGPWRDUP signal */
> +	pwr_reg = readl(sunxi7i_cc_base + AW_CPUCFG_DBGCTL1);
> +	pwr_reg |= (1<<cpu);

The parens around 1<<cpu are not required.  What do you think you gain
by having them?  Coding style also suggests placing a space before and
after the "<<" operation as well.  Again, I've said this in my previous
review.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/