RE: [PATCH v5 1/3] omap3 gpmc: functionality enhancement
> -Original Message- > From: Tony Lindgren [mailto:t...@atomide.com] > Sent: Wednesday, July 07, 2010 6:32 PM > To: Ghorai, Sukumar > Cc: linux-omap@vger.kernel.org; linux-...@lists.infradead.org; > m...@compulab.co.il > Subject: Re: [PATCH v5 1/3] omap3 gpmc: functionality enhancement > > * Ghorai, Sukumar [100707 15:26]: > > > From: Tony Lindgren [mailto:t...@atomide.com] > > > > > > You should just replace this function with simple functions like we > > > already > > > have in gpmc.c rather than trying to pack everything into one function. > > > Just add various gpmc_xxx_get/set functions rather than pass int *rval. > > > > [Ghorai] So I was having the same query very 1st time. > > So we need to implement 15 separate functions to do the same as you > suggested. And in my approach it's very easy to enhance the functionally > in future, say to add new set/get. E.g. we need the similar cleanup for > OneNAND code too. > > So, would you please confirm once again with one is the best and should > follow? > > In general, we should have separate read and write functions. > > Maybe you can group them a little bit? Some of them need the chip > select, and some of them are generic. Then some of them are NAND > specific. [Ghorai] ok. And let me re-work this for next version. > > Regards, > > Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] omap3 gpmc: functionality enhancement
* Ghorai, Sukumar [100707 15:26]: > > From: Tony Lindgren [mailto:t...@atomide.com] > > > > You should just replace this function with simple functions like we > > already > > have in gpmc.c rather than trying to pack everything into one function. > > Just add various gpmc_xxx_get/set functions rather than pass int *rval. > > [Ghorai] So I was having the same query very 1st time. > So we need to implement 15 separate functions to do the same as you > suggested. And in my approach it's very easy to enhance the functionally in > future, say to add new set/get. E.g. we need the similar cleanup for OneNAND > code too. > So, would you please confirm once again with one is the best and should > follow? In general, we should have separate read and write functions. Maybe you can group them a little bit? Some of them need the chip select, and some of them are generic. Then some of them are NAND specific. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v5 1/3] omap3 gpmc: functionality enhancement
Tony,
> -Original Message-
> From: Tony Lindgren [mailto:t...@atomide.com]
> Sent: Wednesday, July 07, 2010 3:49 PM
> To: Ghorai, Sukumar
> Cc: linux-omap@vger.kernel.org; linux-...@lists.infradead.org;
> m...@compulab.co.il
> Subject: Re: [PATCH v5 1/3] omap3 gpmc: functionality enhancement
>
> * Sukumar Ghorai [100604 10:34]:
> > few functions added in gpmc module and to be used by other drivers like
> NAND.
> > E.g.: - ioctl function
> > - ecc functions
>
> Uhh, let's leave out ioctl from the description.. Otherwise people
> think you're adding new ioctls in this patch.
[Ghorai] I am agree.
>
> > @@ -46,8 +46,9 @@
> > #define GPMC_ECC_CONFIG0x1f4
> > #define GPMC_ECC_CONTROL 0x1f8
> > #define GPMC_ECC_SIZE_CONFIG 0x1fc
> > +#define GPMC_ECC1_RESULT0x200
> >
> > -#define GPMC_CS0 0x60
> > +#define GPMC_CS0_BASE 0x60
> > #define GPMC_CS_SIZE 0x30
> >
> > #define GPMC_MEM_START 0x
>
> Why changing GPMC_CS0 to GPMC_CS0_BASE? Should it rather be
> GPMC_CS0_OFFSET?
[Ghorai] I am agree with your input.
>
> > @@ -419,8 +437,100 @@ void gpmc_cs_free(int cs)
> > EXPORT_SYMBOL(gpmc_cs_free);
> >
> > /**
> > + * gpmc_hwcontrol - hardware specific access (read/ write) control
> > + * @cs: chip select number
> > + * @cmd: command type
> > + * @write: 1 for write; 0 for read
> > + * @wval: value to write
> > + * @rval: read pointer
> > + */
> > +int gpmc_hwcontrol(int cs, int cmd, int write, int wval, int *rval)
> > +{
> > + u32 regval = 0;
> > +
> > + if (!write && !rval)
> > + return -EINVAL;
>
> You pass int write, then return immediately if it's not set?
[Ghorai] This is just to check if argument passed correctly either for read or
write functionally to do. We can remove this checking.
>
> > + switch (cmd) {
> > + case GPMC_STATUS_BUFFER:
> > + regval = gpmc_read_reg(GPMC_STATUS);
> > + /* 1 : buffer is available to write */
> > + *rval = regval & GPMC_STATUS_BUFF_EMPTY;
> > + break;
> > +
> > + case GPMC_GET_SET_IRQ_STATUS:
> > + if (write)
> > + gpmc_write_reg(GPMC_IRQSTATUS, wval);
> > + else
> > + *rval = gpmc_read_reg(GPMC_IRQSTATUS);
> > + break;
> > +
> > + case GPMC_PREFETCH_FIFO_CNT:
> > + regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
> > + *rval = GPMC_PREFETCH_STATUS_FIFO_CNT(regval);
> > + break;
> > +
> > + case GPMC_PREFETCH_COUNT:
> > + regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
> > + *rval = GPMC_PREFETCH_STATUS_COUNT(regval);
> > + break;
> > +
> > + case GPMC_CONFIG_WP:
> > + regval = gpmc_read_reg(GPMC_CONFIG);
> > + if (wval)
> > + regval &= ~GPMC_CONFIG_WRITEPROTECT; /* WP is ON */
> > + else
> > + regval |= GPMC_CONFIG_WRITEPROTECT; /* WP is OFF */
> > + gpmc_write_reg(GPMC_CONFIG, regval);
> > + break;
> > +
> > + case GPMC_CONFIG_RDY_BSY:
> > + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> > + regval |= WR_RD_PIN_MONITORING;
> > + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> > + break;
> > +
> > + case GPMC_CONFIG_DEV_SIZE:
> > + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> > + regval |= GPMC_CONFIG1_DEVICESIZE(wval);
> > + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> > + break;
> > +
> > + case GPMC_CONFIG_DEV_TYPE:
> > + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> > + regval |= GPMC_CONFIG1_DEVICETYPE(wval);
> > + if (wval == GPMC_DEVICETYPE_NOR)
> > + regval |= GPMC_CONFIG1_MUXADDDATA;
> > + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> > + break;
> > +
> > + case GPMC_NAND_COMMAND:
> > + gpmc_cs_write_byte(cs, GPMC_CS_NAND_COMMAND, wval);
> > + break;
> > +
> > + case GPMC_NAND_ADDRESS:
> > + gpmc_cs_write_byte(cs, GPMC_CS_NAND_ADDRESS, wval);
> > + break;
> > +
> > + case GPMC_NAND_DATA:
> > + if (write)
> > + gpmc_cs_write_byte(cs, GPMC_CS_NAND_DATA, wval);
> > + els
Re: [PATCH v5 1/3] omap3 gpmc: functionality enhancement
* Sukumar Ghorai [100604 10:34]:
> few functions added in gpmc module and to be used by other drivers like NAND.
> E.g.: - ioctl function
> - ecc functions
Uhh, let's leave out ioctl from the description.. Otherwise people
think you're adding new ioctls in this patch.
> @@ -46,8 +46,9 @@
> #define GPMC_ECC_CONFIG 0x1f4
> #define GPMC_ECC_CONTROL 0x1f8
> #define GPMC_ECC_SIZE_CONFIG 0x1fc
> +#define GPMC_ECC1_RESULT0x200
>
> -#define GPMC_CS0 0x60
> +#define GPMC_CS0_BASE0x60
> #define GPMC_CS_SIZE 0x30
>
> #define GPMC_MEM_START 0x
Why changing GPMC_CS0 to GPMC_CS0_BASE? Should it rather be
GPMC_CS0_OFFSET?
> @@ -419,8 +437,100 @@ void gpmc_cs_free(int cs)
> EXPORT_SYMBOL(gpmc_cs_free);
>
> /**
> + * gpmc_hwcontrol - hardware specific access (read/ write) control
> + * @cs: chip select number
> + * @cmd: command type
> + * @write: 1 for write; 0 for read
> + * @wval: value to write
> + * @rval: read pointer
> + */
> +int gpmc_hwcontrol(int cs, int cmd, int write, int wval, int *rval)
> +{
> + u32 regval = 0;
> +
> + if (!write && !rval)
> + return -EINVAL;
You pass int write, then return immediately if it's not set?
> + switch (cmd) {
> + case GPMC_STATUS_BUFFER:
> + regval = gpmc_read_reg(GPMC_STATUS);
> + /* 1 : buffer is available to write */
> + *rval = regval & GPMC_STATUS_BUFF_EMPTY;
> + break;
> +
> + case GPMC_GET_SET_IRQ_STATUS:
> + if (write)
> + gpmc_write_reg(GPMC_IRQSTATUS, wval);
> + else
> + *rval = gpmc_read_reg(GPMC_IRQSTATUS);
> + break;
> +
> + case GPMC_PREFETCH_FIFO_CNT:
> + regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
> + *rval = GPMC_PREFETCH_STATUS_FIFO_CNT(regval);
> + break;
> +
> + case GPMC_PREFETCH_COUNT:
> + regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
> + *rval = GPMC_PREFETCH_STATUS_COUNT(regval);
> + break;
> +
> + case GPMC_CONFIG_WP:
> + regval = gpmc_read_reg(GPMC_CONFIG);
> + if (wval)
> + regval &= ~GPMC_CONFIG_WRITEPROTECT; /* WP is ON */
> + else
> + regval |= GPMC_CONFIG_WRITEPROTECT; /* WP is OFF */
> + gpmc_write_reg(GPMC_CONFIG, regval);
> + break;
> +
> + case GPMC_CONFIG_RDY_BSY:
> + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + regval |= WR_RD_PIN_MONITORING;
> + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> + break;
> +
> + case GPMC_CONFIG_DEV_SIZE:
> + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + regval |= GPMC_CONFIG1_DEVICESIZE(wval);
> + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> + break;
> +
> + case GPMC_CONFIG_DEV_TYPE:
> + regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + regval |= GPMC_CONFIG1_DEVICETYPE(wval);
> + if (wval == GPMC_DEVICETYPE_NOR)
> + regval |= GPMC_CONFIG1_MUXADDDATA;
> + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
> + break;
> +
> + case GPMC_NAND_COMMAND:
> + gpmc_cs_write_byte(cs, GPMC_CS_NAND_COMMAND, wval);
> + break;
> +
> + case GPMC_NAND_ADDRESS:
> + gpmc_cs_write_byte(cs, GPMC_CS_NAND_ADDRESS, wval);
> + break;
> +
> + case GPMC_NAND_DATA:
> + if (write)
> + gpmc_cs_write_byte(cs, GPMC_CS_NAND_DATA, wval);
> + else
> + *rval = gpmc_cs_read_byte(cs, GPMC_CS_NAND_DATA);
> + break;
> +
> + default:
> + printk(KERN_ERR "gpmc_hwcontrol: Not supported\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(gpmc_hwcontrol);
You should just replace this function with simple functions like we already
have in gpmc.c rather than trying to pack everything into one function.
Just add various gpmc_xxx_get/set functions rather than pass int *rval.
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/3] omap3 gpmc: functionality enhancement
few functions added in gpmc module and to be used by other drivers like NAND.
E.g.: - ioctl function
- ecc functions
Signed-off-by: Sukumar Ghorai
---
arch/arm/mach-omap2/gpmc.c | 219 ++--
arch/arm/plat-omap/include/plat/gpmc.h | 33 +-
drivers/mtd/nand/omap2.c |4 +-
3 files changed, 239 insertions(+), 17 deletions(-)
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5bc3ca0..48b5af0
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -46,8 +46,9 @@
#define GPMC_ECC_CONFIG0x1f4
#define GPMC_ECC_CONTROL 0x1f8
#define GPMC_ECC_SIZE_CONFIG 0x1fc
+#define GPMC_ECC1_RESULT0x200
-#define GPMC_CS0 0x60
+#define GPMC_CS0_BASE 0x60
#define GPMC_CS_SIZE 0x30
#define GPMC_MEM_START 0x
@@ -92,7 +93,8 @@ struct omap3_gpmc_regs {
static struct resource gpmc_mem_root;
static struct resource gpmc_cs_mem[GPMC_CS_NUM];
static DEFINE_SPINLOCK(gpmc_mem_lock);
-static unsignedgpmc_cs_map;
+static unsigned int gpmc_cs_map; /* flag for cs which are initialized */
+static int gpmc_ecc_used = -EINVAL;/* cs using ecc engine */
static void __iomem *gpmc_base;
@@ -108,11 +110,27 @@ static u32 gpmc_read_reg(int idx)
return __raw_readl(gpmc_base + idx);
}
+static void gpmc_cs_write_byte(int cs, int idx, u8 val)
+{
+ void __iomem *reg_addr;
+
+ reg_addr = gpmc_base + GPMC_CS0_BASE + (cs * GPMC_CS_SIZE) + idx;
+ __raw_writeb(val, reg_addr);
+}
+
+static u8 gpmc_cs_read_byte(int cs, int idx)
+{
+ void __iomem *reg_addr;
+
+ reg_addr = gpmc_base + GPMC_CS0_BASE + (cs * GPMC_CS_SIZE) + idx;
+ return __raw_readb(reg_addr);
+}
+
void gpmc_cs_write_reg(int cs, int idx, u32 val)
{
void __iomem *reg_addr;
- reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
+ reg_addr = gpmc_base + GPMC_CS0_BASE + (cs * GPMC_CS_SIZE) + idx;
__raw_writel(val, reg_addr);
}
@@ -120,7 +138,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
{
void __iomem *reg_addr;
- reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
+ reg_addr = gpmc_base + GPMC_CS0_BASE + (cs * GPMC_CS_SIZE) + idx;
return __raw_readl(reg_addr);
}
@@ -419,8 +437,100 @@ void gpmc_cs_free(int cs)
EXPORT_SYMBOL(gpmc_cs_free);
/**
+ * gpmc_hwcontrol - hardware specific access (read/ write) control
+ * @cs: chip select number
+ * @cmd: command type
+ * @write: 1 for write; 0 for read
+ * @wval: value to write
+ * @rval: read pointer
+ */
+int gpmc_hwcontrol(int cs, int cmd, int write, int wval, int *rval)
+{
+ u32 regval = 0;
+
+ if (!write && !rval)
+ return -EINVAL;
+
+ switch (cmd) {
+ case GPMC_STATUS_BUFFER:
+ regval = gpmc_read_reg(GPMC_STATUS);
+ /* 1 : buffer is available to write */
+ *rval = regval & GPMC_STATUS_BUFF_EMPTY;
+ break;
+
+ case GPMC_GET_SET_IRQ_STATUS:
+ if (write)
+ gpmc_write_reg(GPMC_IRQSTATUS, wval);
+ else
+ *rval = gpmc_read_reg(GPMC_IRQSTATUS);
+ break;
+
+ case GPMC_PREFETCH_FIFO_CNT:
+ regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
+ *rval = GPMC_PREFETCH_STATUS_FIFO_CNT(regval);
+ break;
+
+ case GPMC_PREFETCH_COUNT:
+ regval = gpmc_read_reg(GPMC_PREFETCH_STATUS);
+ *rval = GPMC_PREFETCH_STATUS_COUNT(regval);
+ break;
+
+ case GPMC_CONFIG_WP:
+ regval = gpmc_read_reg(GPMC_CONFIG);
+ if (wval)
+ regval &= ~GPMC_CONFIG_WRITEPROTECT; /* WP is ON */
+ else
+ regval |= GPMC_CONFIG_WRITEPROTECT; /* WP is OFF */
+ gpmc_write_reg(GPMC_CONFIG, regval);
+ break;
+
+ case GPMC_CONFIG_RDY_BSY:
+ regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+ regval |= WR_RD_PIN_MONITORING;
+ gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
+ break;
+
+ case GPMC_CONFIG_DEV_SIZE:
+ regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+ regval |= GPMC_CONFIG1_DEVICESIZE(wval);
+ gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
+ break;
+
+ case GPMC_CONFIG_DEV_TYPE:
+ regval = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+ regval |= GPMC_CONFIG1_DEVICETYPE(wval);
+ if (wval == GPMC_DEVICETYPE_NOR)
+ regval |= GPMC_CONFIG1_MUXADDDATA;
+ gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, regval);
+ break;
+
+ case GPMC_NAND_COMMAND:
+ gpmc_cs_write_byte(cs, GPMC_CS_NAND_COMMAND, wval);
+ break;
+
+ cas
