From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from metis.ext.pengutronix.de ([2001:6f8:1178:4:290:27ff:fe1d:cc33]) by bombadil.infradead.org with esmtps (Exim 4.69 #1 (Red Hat Linux)) id 1OBjuP-0006Zz-Px for barebox@lists.infradead.org; Tue, 11 May 2010 07:29:44 +0000 Date: Tue, 11 May 2010 09:29:38 +0200 From: Sascha Hauer Message-ID: <20100511072938.GV31199@pengutronix.de> References: <1273284563-28645-1-git-send-email-marc@cpdesign.com.au> <1273284563-28645-2-git-send-email-marc@cpdesign.com.au> <1273284563-28645-3-git-send-email-marc@cpdesign.com.au> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1273284563-28645-3-git-send-email-marc@cpdesign.com.au> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: barebox-bounces@lists.infradead.org Errors-To: barebox-bounces+u.kleine-koenig=pengutronix.de@lists.infradead.org Subject: Re: [PATCH 2/3] Detect CPU and board revision for freescale-mx35-3-stack and add to boot parameters To: Marc Reilly Cc: barebox@lists.infradead.org, Marc Reilly Hi Marc, Your patch has some coding style issues. We write function calls like x(y), not x( y ). Also there are several trailing whitespaces in the patch. Some other comments inline. On Sat, May 08, 2010 at 12:09:22PM +1000, Marc Reilly wrote: > From: Marc Reilly > > --- > arch/arm/mach-imx/include/mach/generic.h | 5 ++ > arch/arm/mach-imx/include/mach/imx35-regs.h | 18 +++++++ > board/freescale-mx35-3-stack/3stack.c | 71 +++++++++++++++++++++++++-- > 3 files changed, 90 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-imx/include/mach/generic.h b/arch/arm/mach-imx/include/mach/generic.h > index 99a53a4..926e553 100644 > --- a/arch/arm/mach-imx/include/mach/generic.h > +++ b/arch/arm/mach-imx/include/mach/generic.h > @@ -3,6 +3,11 @@ int imx_silicon_revision(void); > #define IMX27_CHIP_REVISION_1_0 0 > #define IMX27_CHIP_REVISION_2_0 1 > > +#define IMX35_CHIP_REVISION_1_0 0x10 > +#define IMX35_CHIP_REVISION_2_0 0x20 > + > + > + > #ifdef CONFIG_ARCH_IMX1 > #define cpu_is_mx1() (1) > #else > diff --git a/arch/arm/mach-imx/include/mach/imx35-regs.h b/arch/arm/mach-imx/include/mach/imx35-regs.h > index c394a2a..899e57b 100644 > --- a/arch/arm/mach-imx/include/mach/imx35-regs.h > +++ b/arch/arm/mach-imx/include/mach/imx35-regs.h > @@ -76,6 +76,24 @@ > #define PDR0_AUTO_CON (1 << 0) > #define PDR0_PER_SEL (1 << 26) > > + > +#define IIM_STAT 0x0000 > +#define IIM_STATM 0x0004 > +#define IIM_ERR 0x0008 > +#define IIM_EMASK 0x000C > +#define IIM_FCTL 0x0010 > +#define IIM_UA 0x0014 > +#define IIM_LA 0x0018 > +#define IIM_SDAT 0x001C > +#define IIM_PREV 0x0020 > +#define IIM_SREV 0x0024 > +#define IIM_PREG_P 0x0028 > +#define IIM_SCS0 0x002C > +#define IIM_SCS1 0x0030 > +#define IIM_SCS2 0x0034 > +#define IIM_SCS3 0x0038 > + > + > /* > * Adresses and ranges of the external chip select lines > */ > diff --git a/board/freescale-mx35-3-stack/3stack.c b/board/freescale-mx35-3-stack/3stack.c > index fcb87cf..cee0e75 100644 > --- a/board/freescale-mx35-3-stack/3stack.c > +++ b/board/freescale-mx35-3-stack/3stack.c > @@ -47,6 +47,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -144,6 +145,58 @@ static struct device_d imxfb_dev = { > .platform_data = &ipu_fb_data, > }; > > + > +/* Board rev for the PDK 3stack */ > +#define MX35PDK_BOARD_REV_1 0 > +#define MX35PDK_BOARD_REV_2 1 > + > + > +/* > + * Revision to be passed to kernel. The kernel provided > + * by freescale relies on this. > + * > + * C --> CPU type > + * S --> Silicon revision > + * B --> Board rev > + * > + * 31 20 16 12 8 4 0 > + * | Cmaj | Cmin | B | Smaj | Smin| > + * > + * e.g 0x00035120 --> i.MX35, Cpu silicon rev 2.0, Board rev 2 > +*/ > +static unsigned int system_rev; > + > +unsigned int get_board_rev(void) > +{ > + return system_rev; > +} This shouldn't be exported, at least not with such a generic name and without a declaration in a header file. > + > +/* > + * Setup system_rev with the MX35 cpu and the silicon revision > + * Doesn't set up the board. > + * > + * TODO: look up the rompatch > + */ > +static inline void setup_soc_rev(void) > +{ > + uint32_t reg; > + reg = readl(IMX_IIM_BASE + IIM_SREV); > + reg += IMX35_CHIP_REVISION_1_0; > + > + system_rev = 0x35000 + (reg & 0xFF); > +} This function could be useful elsewhere. We already have a imx_silicon_revision() for i.MX27. Can you use the same for i.MX35 please? > + > +static inline void set_board_rev(int rev) > +{ > + system_rev = (system_rev & ~(0xF << 8)) | (rev & 0xF) << 8; > +} > + > +int is_soc_rev(int rev) > +{ > + return (system_rev & 0xFF) - rev; > +} > + > + > static int f3s_devices_init(void) > { > uint32_t reg; > @@ -181,6 +234,8 @@ static int f3s_devices_init(void) > break; > } > > + setup_soc_rev(); I think it's easier to read just to do a system_rev = 0x35000 | imx_silicon_revision(); here, and a > + > i2c_register_board_info(0, i2c_devices, ARRAY_SIZE(i2c_devices)); > register_device(&i2c_dev); > > @@ -374,13 +429,16 @@ static int f3s_get_rev(struct mc13892 *mc13892) > if (rev == 0x00ffffff) > return -ENODEV; > > - return ((rev >> 6) & 0x7) ? 20 : 10; > + return ((rev >> 6) & 0x7) ? MX35PDK_BOARD_REV_2 : MX35PDK_BOARD_REV_1; > } > > static int f3s_pmic_init_v2(struct mc13892 *mc13892) > { > int err = 0; > > + /* COMPARE pin (GPIO1_5) as output and set high */ > + gpio_direction_output( 32*0 + 5 , 1); > + > err |= mc13892_set_bits(mc13892, MC13892_REG_SETTING_0, 0x03, 0x03); > err |= mc13892_set_bits(mc13892, MC13892_REG_MODE_0, 0x01, 0x01); > if (err) > @@ -421,16 +479,18 @@ static int f3s_pmic_init(void) > > rev = f3s_get_rev(mc13892); > switch (rev) { > - case 10: > + case MX35PDK_BOARD_REV_1: > break; > - case 20: > + case MX35PDK_BOARD_REV_2: > f3s_pmic_init_v2(mc13892); > break; > default: > printf("FAILED to identify board revision!\n"); > return 0; > } > - printf("i.MX35 PDK CPU board version %d.%d\n", rev / 10, rev % 10); > + > + set_board_rev( rev ); system_rev |= rev << 8 here. Also, imx35_3ds_system_rev might be a better name to prevent confusion with other system_rev variables. Sascha > + printf("i.MX35 PDK CPU board version %d.\n", rev ); > > mc9sdz60 = mc9sdz60_get(); > if (!mc9sdz60) { > @@ -439,6 +499,9 @@ static int f3s_pmic_init(void) > } > > f3s_pmic_init_all(mc9sdz60); > + > + printf("system_rev is 0x%08x.\n", system_rev ); > + armlinux_set_revision( system_rev ); > > return 0; > } > -- > 1.6.4.2 > > > _______________________________________________ > barebox mailing list > barebox@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/barebox > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox