Patchwork [U-Boot,02/13] S3C64XX: Switch to use readl/writel to operate nand flash

login
register
mail settings
Submitter seedshope
Date July 7, 2012, 9:57 a.m.
Message ID <1341655032-30201-3-git-send-email-bocui107@gmail.com>
Download mbox | patch
Permalink /patch/169574/
State Superseded
Headers show

Comments

seedshope - July 7, 2012, 9:57 a.m.
From: Zhong Hongbo <bocui107@gmail.com>

Signed-off-by: Zhong Hongbo <bocui107@gmail.com>
---
 arch/arm/include/asm/arch-s3c64xx/nand.h    |   69 +++++++++++++++++++++++
 arch/arm/include/asm/arch-s3c64xx/s3c6400.h |   79 ++-------------------------
 board/samsung/smdk6400/lowlevel_init.S      |    8 ++--
 drivers/mtd/nand/s3c64xx.c                  |   58 +++++++++++++-------
 4 files changed, 116 insertions(+), 98 deletions(-)
 create mode 100644 arch/arm/include/asm/arch-s3c64xx/nand.h
Scott Wood - July 9, 2012, 10:19 p.m.
On 07/07/2012 04:57 AM, Zhong Hongbo wrote:
> +static inline unsigned int s3c64xx_get_base_nand(void)
> +{
> +	return ELFIN_NAND_BASE;
> +}

unsigned long or uintptr_t would be more appropriate, even if U-Boot is
unlikely to be 64-bit any time soon.

Or better, "struct s3c64xx_nand *".

> @@ -89,15 +96,16 @@ static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
>   */
>  static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>  {
> +	struct s3c64xx_nand *const nand = s3c_get_base_nand();

Is there any benefit to declaring local variables const like this?  Why
this one and not all the others that never get altered?

-Scott
seedshope - July 10, 2012, 1 p.m.
On 07/10/2012 08:29 PM, Zhong Hongbo wrote:
> On 07/10/2012 06:19 AM, Scott Wood wrote:
>> On 07/07/2012 04:57 AM, Zhong Hongbo wrote:
>>> +static inline unsigned int s3c64xx_get_base_nand(void)
>>> +{
>>> +	return ELFIN_NAND_BASE;
>>> +}
>>
>> unsigned long or uintptr_t would be more appropriate, even if U-Boot is
>> unlikely to be 64-bit any time soon.
> 
> Ok, I will fix it in V2.
> 
> Thanks,
> hongbo
>>
>> Or better, "struct s3c64xx_nand *".
>>
>>> @@ -89,15 +96,16 @@ static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
>>>   */
>>>  static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>>  {
>>> +	struct s3c64xx_nand *const nand = s3c_get_base_nand();
>>
>> Is there any benefit to declaring local variables const like this?
> 
> I reference the nand driver of S5PXX CPU. So ...
Sorry, I make a mistake, The S5PXX have not nand flash support. When i
do the patch, I use the format as following:

struct s3c64xx_nand *nand = s3c_get_base_nand();

But when I use checkpatch.pl script to check the patch. more and more
waring about the line, it said that you should add 'const' before nand
variable.

Thanks,
hongbo
> 
>   Why
>> this one and not all the others that never get altered?
> 
> Ok, I will change it. And i just found the S3c64XX is orphaned board.
> So Thanks you for the foucus it!
> 
> Thanks,
> hongbo
>>
>> -Scott
>>
>>
> 
> 
> 
>
Scott Wood - July 10, 2012, 3:36 p.m.
On 07/10/2012 08:00 AM, Zhong Hongbo wrote:
> On 07/10/2012 08:29 PM, Zhong Hongbo wrote:
>> On 07/10/2012 06:19 AM, Scott Wood wrote:
>>> On 07/07/2012 04:57 AM, Zhong Hongbo wrote:
>>>> @@ -89,15 +96,16 @@ static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
>>>>   */
>>>>  static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>>>  {
>>>> +	struct s3c64xx_nand *const nand = s3c_get_base_nand();
>>>
>>> Is there any benefit to declaring local variables const like this?
>>
>> I reference the nand driver of S5PXX CPU. So ...
> Sorry, I make a mistake, The S5PXX have not nand flash support. When i
> do the patch, I use the format as following:
> 
> struct s3c64xx_nand *nand = s3c_get_base_nand();
> 
> But when I use checkpatch.pl script to check the patch. more and more
> waring about the line, it said that you should add 'const' before nand
> variable.

Could you paste the exact output from checkpatch.pl?

-Scott
seedshope - July 11, 2012, 12:34 p.m.
Hi Scott,

On 07/10/2012 11:36 PM, Scott Wood wrote:
> On 07/10/2012 08:00 AM, Zhong Hongbo wrote:
>> On 07/10/2012 08:29 PM, Zhong Hongbo wrote:
>>> On 07/10/2012 06:19 AM, Scott Wood wrote:
>>>> On 07/07/2012 04:57 AM, Zhong Hongbo wrote:
>>>>> @@ -89,15 +96,16 @@ static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
>>>>>   */
>>>>>  static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>>>>>  {
>>>>> +	struct s3c64xx_nand *const nand = s3c_get_base_nand();
>>>>
>>>> Is there any benefit to declaring local variables const like this?
>>>
>>> I reference the nand driver of S5PXX CPU. So ...
>> Sorry, I make a mistake, The S5PXX have not nand flash support. When i
>> do the patch, I use the format as following:
>>
>> struct s3c64xx_nand *nand = s3c_get_base_nand();
>>
>> But when I use checkpatch.pl script to check the patch. more and more
>> waring about the line, it said that you should add 'const' before nand
>> variable.
> 
> Could you paste the exact output from checkpatch.pl?

Just redo the patch, I can not reproduce the issue. Thank you for your
point.

Thanks,
hongbo
> 
> -Scott
> 
> 
>

Patch

diff --git a/arch/arm/include/asm/arch-s3c64xx/nand.h b/arch/arm/include/asm/arch-s3c64xx/nand.h
new file mode 100644
index 0000000..51e4d34
--- /dev/null
+++ b/arch/arm/include/asm/arch-s3c64xx/nand.h
@@ -0,0 +1,69 @@ 
+/*
+ * (C) Copyright 2012
+ * Zhong Hongbo <bocui107@gmail.com>
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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
+ */
+
+#ifndef __ASM_ARCH_NAND_H__
+#define __ASM_ARCH_NAND_H__
+
+#define NFCONF_ECC_4BIT		(1<<24)
+
+#define NFCONT_ECC_ENC		(1<<18)
+#define NFCONT_WP		(1<<16)
+#define NFCONT_MECCLOCK		(1<<7)
+#define NFCONT_SECCLOCK		(1<<6)
+#define NFCONT_INITMECC		(1<<5)
+#define NFCONT_INITSECC		(1<<4)
+#define NFCONT_INITECC		(NFCONT_INITMECC | NFCONT_INITSECC)
+#define NFCONT_CS_ALT		(1<<2)
+#define NFCONT_CS		(1<<1)
+#define NFCONT_ENABLE		(1<<0)
+
+#define NFSTAT_ECCENCDONE	(1<<7)
+#define NFSTAT_ECCDECDONE	(1<<6)
+#define NFSTAT_RnB		(1<<0)
+
+#define NFESTAT0_ECCBUSY	(1<<31)
+
+#ifndef __ASSEMBLY__
+/* NAND FLASH */
+struct s3c64xx_nand {
+	u32	nfconf;
+	u32	nfcont;
+	u32	nfcmmd;
+	u32	nfaddr;
+	u32	nfdata;
+	u32	nfmeccdata0;
+	u32	nfmeccdata1;
+	u32	nfseccdata0;
+	u32	nfsblk;
+	u32	nfeblk;
+	u32	nfstat;
+	u32	nfestat0;
+	u32	nfestat1;
+	u32	nfmecc0;
+	u32	nfmecc1;
+	u32	nfsecc;
+	u32	nfmlcbitpt;
+};
+#endif
+
+#endif
diff --git a/arch/arm/include/asm/arch-s3c64xx/s3c6400.h b/arch/arm/include/asm/arch-s3c64xx/s3c6400.h
index 10b3324..5cb8976 100644
--- a/arch/arm/include/asm/arch-s3c64xx/s3c6400.h
+++ b/arch/arm/include/asm/arch-s3c64xx/s3c6400.h
@@ -556,80 +556,6 @@ 
  */
 #define ELFIN_NAND_BASE		0x70200000
 
-#define NFCONF_OFFSET		0x00
-#define NFCONT_OFFSET		0x04
-#define NFCMMD_OFFSET		0x08
-#define NFADDR_OFFSET		0x0c
-#define NFDATA_OFFSET		0x10
-#define NFMECCDATA0_OFFSET	0x14
-#define NFMECCDATA1_OFFSET	0x18
-#define NFSECCDATA0_OFFSET	0x1c
-#define NFSBLK_OFFSET		0x20
-#define NFEBLK_OFFSET		0x24
-#define NFSTAT_OFFSET		0x28
-#define NFESTAT0_OFFSET		0x2c
-#define NFESTAT1_OFFSET		0x30
-#define NFMECC0_OFFSET		0x34
-#define NFMECC1_OFFSET		0x38
-#define NFSECC_OFFSET		0x3c
-#define NFMLCBITPT_OFFSET	0x40
-
-#define NFCONF			(ELFIN_NAND_BASE + NFCONF_OFFSET)
-#define NFCONT			(ELFIN_NAND_BASE + NFCONT_OFFSET)
-#define NFCMMD			(ELFIN_NAND_BASE + NFCMMD_OFFSET)
-#define NFADDR			(ELFIN_NAND_BASE + NFADDR_OFFSET)
-#define NFDATA			(ELFIN_NAND_BASE + NFDATA_OFFSET)
-#define NFMECCDATA0		(ELFIN_NAND_BASE + NFMECCDATA0_OFFSET)
-#define NFMECCDATA1		(ELFIN_NAND_BASE + NFMECCDATA1_OFFSET)
-#define NFSECCDATA0		(ELFIN_NAND_BASE + NFSECCDATA0_OFFSET)
-#define NFSBLK			(ELFIN_NAND_BASE + NFSBLK_OFFSET)
-#define NFEBLK			(ELFIN_NAND_BASE + NFEBLK_OFFSET)
-#define NFSTAT			(ELFIN_NAND_BASE + NFSTAT_OFFSET)
-#define NFESTAT0		(ELFIN_NAND_BASE + NFESTAT0_OFFSET)
-#define NFESTAT1		(ELFIN_NAND_BASE + NFESTAT1_OFFSET)
-#define NFMECC0			(ELFIN_NAND_BASE + NFMECC0_OFFSET)
-#define NFMECC1			(ELFIN_NAND_BASE + NFMECC1_OFFSET)
-#define NFSECC			(ELFIN_NAND_BASE + NFSECC_OFFSET)
-#define NFMLCBITPT		(ELFIN_NAND_BASE + NFMLCBITPT_OFFSET)
-
-#define NFCONF_REG		__REG(ELFIN_NAND_BASE + NFCONF_OFFSET)
-#define NFCONT_REG		__REG(ELFIN_NAND_BASE + NFCONT_OFFSET)
-#define NFCMD_REG		__REG(ELFIN_NAND_BASE + NFCMMD_OFFSET)
-#define NFADDR_REG		__REG(ELFIN_NAND_BASE + NFADDR_OFFSET)
-#define NFDATA_REG		__REG(ELFIN_NAND_BASE + NFDATA_OFFSET)
-#define NFDATA8_REG		__REGb(ELFIN_NAND_BASE + NFDATA_OFFSET)
-#define NFMECCDATA0_REG		__REG(ELFIN_NAND_BASE + NFMECCDATA0_OFFSET)
-#define NFMECCDATA1_REG		__REG(ELFIN_NAND_BASE + NFMECCDATA1_OFFSET)
-#define NFSECCDATA0_REG		__REG(ELFIN_NAND_BASE + NFSECCDATA0_OFFSET)
-#define NFSBLK_REG		__REG(ELFIN_NAND_BASE + NFSBLK_OFFSET)
-#define NFEBLK_REG		__REG(ELFIN_NAND_BASE + NFEBLK_OFFSET)
-#define NFSTAT_REG		__REG(ELFIN_NAND_BASE + NFSTAT_OFFSET)
-#define NFESTAT0_REG		__REG(ELFIN_NAND_BASE + NFESTAT0_OFFSET)
-#define NFESTAT1_REG		__REG(ELFIN_NAND_BASE + NFESTAT1_OFFSET)
-#define NFMECC0_REG		__REG(ELFIN_NAND_BASE + NFMECC0_OFFSET)
-#define NFMECC1_REG		__REG(ELFIN_NAND_BASE + NFMECC1_OFFSET)
-#define NFSECC_REG		__REG(ELFIN_NAND_BASE + NFSECC_OFFSET)
-#define NFMLCBITPT_REG		__REG(ELFIN_NAND_BASE + NFMLCBITPT_OFFSET)
-
-#define NFCONF_ECC_4BIT		(1<<24)
-
-#define NFCONT_ECC_ENC		(1<<18)
-#define NFCONT_WP		(1<<16)
-#define NFCONT_MECCLOCK		(1<<7)
-#define NFCONT_SECCLOCK		(1<<6)
-#define NFCONT_INITMECC		(1<<5)
-#define NFCONT_INITSECC		(1<<4)
-#define NFCONT_INITECC		(NFCONT_INITMECC | NFCONT_INITSECC)
-#define NFCONT_CS_ALT		(1<<2)
-#define NFCONT_CS		(1<<1)
-#define NFCONT_ENABLE		(1<<0)
-
-#define NFSTAT_ECCENCDONE	(1<<7)
-#define NFSTAT_ECCDECDONE	(1<<6)
-#define NFSTAT_RnB		(1<<0)
-
-#define NFESTAT0_ECCBUSY	(1<<31)
-
 /*
  * Interrupt
  */
@@ -890,6 +816,11 @@  static inline s3c64xx_uart *s3c64xx_get_base_uart(enum s3c64xx_uarts_nr nr)
 {
 	return (s3c64xx_uart *)(ELFIN_UART_BASE + (nr * 0x400));
 }
+
+static inline unsigned int s3c64xx_get_base_nand(void)
+{
+	return ELFIN_NAND_BASE;
+}
 #endif
 
 #endif /*__S3C6400_H__*/
diff --git a/board/samsung/smdk6400/lowlevel_init.S b/board/samsung/smdk6400/lowlevel_init.S
index f7ce176..4a82e4d 100644
--- a/board/samsung/smdk6400/lowlevel_init.S
+++ b/board/samsung/smdk6400/lowlevel_init.S
@@ -269,14 +269,14 @@  uart_asm_init:
  */
 nand_asm_init:
 	ldr	r0, =ELFIN_NAND_BASE
-	ldr	r1, [r0, #NFCONF_OFFSET]
+	ldr	r1, [r0, #0x0]	@NFCONF_OFFSET
 	orr	r1, r1, #0x70
 	orr	r1, r1, #0x7700
-	str	r1, [r0, #NFCONF_OFFSET]
+	str	r1, [r0, #0x0]
 
-	ldr	r1, [r0, #NFCONT_OFFSET]
+	ldr	r1, [r0, #0x04]	@NFCONT_OFFSET
 	orr	r1, r1, #0x07
-	str	r1, [r0, #NFCONT_OFFSET]
+	str	r1, [r0, #0x04]
 
 	mov	pc, lr
 #endif
diff --git a/drivers/mtd/nand/s3c64xx.c b/drivers/mtd/nand/s3c64xx.c
index 87f0341..1d6bd67 100644
--- a/drivers/mtd/nand/s3c64xx.c
+++ b/drivers/mtd/nand/s3c64xx.c
@@ -31,6 +31,7 @@ 
 #include <linux/mtd/nand.h>
 
 #include <asm/arch/s3c6400.h>
+#include <asm/arch/nand.h>
 
 #include <asm/io.h>
 #include <asm/errno.h>
@@ -42,6 +43,11 @@  static int nand_cs[MAX_CHIPS] = {0, 1};
 #define printf(arg...) do {} while (0)
 #endif
 
+static inline struct s3c64xx_nand *s3c_get_base_nand(void)
+{
+	return (struct s3c64xx_nand *)s3c64xx_get_base_nand();
+}
+
 /* Nand flash definition values by jsgood */
 #ifdef S3C_NAND_DEBUG
 /*
@@ -64,7 +70,8 @@  static void print_oob(const char *header, struct mtd_info *mtd)
 
 static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
 {
-	int ctrl = readl(NFCONT);
+	struct s3c64xx_nand *const nand = s3c_get_base_nand();
+	int ctrl = readl(&nand->nfcont);
 
 	switch (chip) {
 	case -1:
@@ -80,7 +87,7 @@  static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
 		return;
 	}
 
-	writel(ctrl, NFCONT);
+	writel(ctrl, &nand->nfcont);
 }
 
 /*
@@ -89,15 +96,16 @@  static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
  */
 static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
 {
+	struct s3c64xx_nand *const nand = s3c_get_base_nand();
 	struct nand_chip *this = mtd->priv;
 
 	if (ctrl & NAND_CTRL_CHANGE) {
 		if (ctrl & NAND_CLE)
-			this->IO_ADDR_W = (void __iomem *)NFCMMD;
+			this->IO_ADDR_W = (void *)&nand->nfcmmd;
 		else if (ctrl & NAND_ALE)
-			this->IO_ADDR_W = (void __iomem *)NFADDR;
+			this->IO_ADDR_W = (void *)&nand->nfaddr;
 		else
-			this->IO_ADDR_W = (void __iomem *)NFDATA;
+			this->IO_ADDR_W = (void *)&nand->nfdata;
 		if (ctrl & NAND_NCE)
 			s3c_nand_select_chip(mtd, *(int *)this->priv);
 		else
@@ -114,7 +122,9 @@  static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
  */
 static int s3c_nand_device_ready(struct mtd_info *mtdinfo)
 {
-	return !!(readl(NFSTAT) & NFSTAT_RnB);
+	struct s3c64xx_nand *const nand = s3c_get_base_nand();
+
+	return !!(readl(&nand->nfstat) & NFSTAT_RnB);
 }
 
 #ifdef CONFIG_SYS_S3C_NAND_HWECC
@@ -124,6 +134,7 @@  static int s3c_nand_device_ready(struct mtd_info *mtdinfo)
  */
 static void s3c_nand_enable_hwecc(struct mtd_info *mtd, int mode)
 {
+	struct s3c64xx_nand *const nand = s3c_get_base_nand();
 	u_long nfcont, nfconf;
 
 	/*
@@ -131,12 +142,12 @@  static void s3c_nand_enable_hwecc(struct mtd_info *mtd, int mode)
 	 * those with non-zero ID[3][3:2], which anyway only holds for ST
 	 * (Numonyx) chips
 	 */
-	nfconf = readl(NFCONF) & ~NFCONF_ECC_4BIT;
+	nfconf = readl(&nand->nfconf) & ~NFCONF_ECC_4BIT;
 
-	writel(nfconf, NFCONF);
+	writel(nfconf, &nand->nfconf);
 
 	/* Initialize & unlock */
-	nfcont = readl(NFCONT);
+	nfcont = readl(&nand->nfcont);
 	nfcont |= NFCONT_INITECC;
 	nfcont &= ~NFCONT_MECCLOCK;
 
@@ -145,7 +156,7 @@  static void s3c_nand_enable_hwecc(struct mtd_info *mtd, int mode)
 	else if (mode == NAND_ECC_READ)
 		nfcont &= ~NFCONT_ECC_ENC;
 
-	writel(nfcont, NFCONT);
+	writel(nfcont, &nand->nfcont);
 }
 
 /*
@@ -156,14 +167,15 @@  static void s3c_nand_enable_hwecc(struct mtd_info *mtd, int mode)
 static int s3c_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
 				  u_char *ecc_code)
 {
+	struct s3c64xx_nand *const nand = s3c_get_base_nand();
 	u_long nfcont, nfmecc0;
 
 	/* Lock */
-	nfcont = readl(NFCONT);
+	nfcont = readl(&nand->nfcont);
 	nfcont |= NFCONT_MECCLOCK;
-	writel(nfcont, NFCONT);
+	writel(nfcont, &nand->nfcont);
 
-	nfmecc0 = readl(NFMECC0);
+	nfmecc0 = readl(&nand->nfmecc0);
 
 	ecc_code[0] = nfmecc0 & 0xff;
 	ecc_code[1] = (nfmecc0 >> 8) & 0xff;
@@ -185,18 +197,19 @@  static int s3c_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
 static int s3c_nand_correct_data(struct mtd_info *mtd, u_char *dat,
 				 u_char *read_ecc, u_char *calc_ecc)
 {
-	int ret = -1;
+	struct s3c64xx_nand *const nand = s3c_get_base_nand();
 	u_long nfestat0, nfmeccdata0, nfmeccdata1, err_byte_addr;
 	u_char err_type, repaired;
+	int ret = -1;
 
 	/* SLC: Write ecc to compare */
 	nfmeccdata0 = (calc_ecc[1] << 16) | calc_ecc[0];
 	nfmeccdata1 = (calc_ecc[3] << 16) | calc_ecc[2];
-	writel(nfmeccdata0, NFMECCDATA0);
-	writel(nfmeccdata1, NFMECCDATA1);
+	writel(nfmeccdata0, &nand->nfmeccdata0);
+	writel(nfmeccdata1, &nand->nfmeccdata1);
 
 	/* Read ecc status */
-	nfestat0 = readl(NFESTAT0);
+	nfestat0 = readl(&nand->nfestat0);
 	err_type = nfestat0 & 0x3;
 
 	switch (err_type) {
@@ -254,15 +267,20 @@  static int s3c_nand_correct_data(struct mtd_info *mtd, u_char *dat,
  */
 int board_nand_init(struct nand_chip *nand)
 {
+	struct s3c64xx_nand *const nand_reg = s3c_get_base_nand();
 	static int chip_n;
+	unsigned int nfcont;
 
 	if (chip_n >= MAX_CHIPS)
 		return -ENODEV;
 
-	NFCONT_REG = (NFCONT_REG & ~NFCONT_WP) | NFCONT_ENABLE | 0x6;
+	nfcont = readl(&nand_reg->nfcont);
+	nfcont &= ~NFCONT_WP;
+	nfcont |= NFCONT_ENABLE | 0x6;
+	writel(nfcont, &nand_reg->nfcont);
 
-	nand->IO_ADDR_R		= (void __iomem *)NFDATA;
-	nand->IO_ADDR_W		= (void __iomem *)NFDATA;
+	nand->IO_ADDR_R		= (void *)&nand_reg->nfdata;
+	nand->IO_ADDR_W		= (void *)&nand_reg->nfdata;
 	nand->cmd_ctrl		= s3c_nand_hwcontrol;
 	nand->dev_ready		= s3c_nand_device_ready;
 	nand->select_chip	= s3c_nand_select_chip;