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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130921133141.GL12758@n2100.arm.linux.org.uk>
Date:	Sat, 21 Sep 2013 14:31:42 +0100
From:	Russell King - ARM Linux <linux@....linux.org.uk>
To:	Fan Rong <cinifr@...il.com>
Cc:	coosty@....com, maxime.ripard@...e-electrons.com,
	daniel.lezcano@...aro.org, tglx@...utronix.de,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	mark.rutland@....com, pawel.moll@....co, rob.herring@...xeda.com,
	linux-sunxi@...glegroups.com
Subject: Re: [PATCH 1/3] Add smp support for Allwinner A20(sunxi 7i).

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@...winnertech.com>
> + *
> + * 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/