[PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Vimal Singh]

From: Vimal Singh <vimalsingh@ti.com>
Date: Fri, 30 Oct 2009 11:32:17 +0530
Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module

Removing OMAP NAND driver, when loaded as a module, gives error and
does not get success. This fixes this and makes driver loadable and
removable run time.

Signed-off-by: Vimal Singh <vimalsingh@ti.com>
---
 drivers/mtd/nand/omap2.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 92573d5..ecc4d32 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1056,7 +1056,8 @@ out_free_info:
 static int omap_nand_remove(struct platform_device *pdev)
 {
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct omap_nand_info *info = mtd->priv;
+	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+							mtd);

 	platform_set_drvdata(pdev, NULL);
 	if (use_dma)
@@ -1064,7 +1065,9 @@ static int omap_nand_remove(struct platform_device *pdev)

 	/* Release NAND device, its internal structures and partitions */
 	nand_release(&info->mtd);
+	release_mem_region(info->phys_base, NAND_IO_SIZE);
 	iounmap(info->nand_pref_fifo_add);
+	gpmc_cs_free(info->gpmc_cs);
 	kfree(&info->mtd);
 	return 0;
 }
-- 
1.5.5

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Vimal Singh]

Looping Linux-omap mailing list.


From: Vimal Singh <vimalsingh@ti.com>
Date: Fri, 30 Oct 2009 11:32:17 +0530
Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module

Removing OMAP NAND driver, when loaded as a module, gives error and
does not get success. This fixes this and makes driver loadable and
removable run time.

Signed-off-by: Vimal Singh <vimalsingh@ti.com>
---
 drivers/mtd/nand/omap2.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 92573d5..ecc4d32 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1056,7 +1056,8 @@ out_free_info:
 static int omap_nand_remove(struct platform_device *pdev)
 {
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct omap_nand_info *info = mtd->priv;
+	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+							mtd);

 	platform_set_drvdata(pdev, NULL);
 	if (use_dma)
@@ -1064,7 +1065,9 @@ static int omap_nand_remove(struct platform_device *pdev)

 	/* Release NAND device, its internal structures and partitions */
 	nand_release(&info->mtd);
+	release_mem_region(info->phys_base, NAND_IO_SIZE);
 	iounmap(info->nand_pref_fifo_add);
+	gpmc_cs_free(info->gpmc_cs);
 	kfree(&info->mtd);
 	return 0;
 }
-- 
1.5.5

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Vimal Singh]

Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
Correct patch is below.

-vimal


From: Vimal Singh <vimalsingh@ti.com>
Date: Fri, 30 Oct 2009 14:54:29 +0530
Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module

Removing OMAP NAND driver, when loaded as a module, gives error and
does not get success. This fixes this and makes driver loadable and
removable run time.

Signed-off-by: Vimal Singh <vimalsingh@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |    2 ++
 drivers/mtd/nand/omap2.c   |    5 ++++-
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 1587682..1d10b7b 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
 	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
 	__raw_writel(val, reg_addr);
 }
+EXPORT_SYMBOL(gpmc_cs_write_reg);

 u32 gpmc_cs_read_reg(int cs, int idx)
 {
@@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
 	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
 	return __raw_readl(reg_addr);
 }
+EXPORT_SYMBOL(gpmc_cs_read_reg);

 /* TODO: Add support for gpmc_fck to clock framework and use it */
 unsigned long gpmc_get_fclk_period(void)
diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 92573d5..ecc4d32 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1056,7 +1056,8 @@ out_free_info:
 static int omap_nand_remove(struct platform_device *pdev)
 {
 	struct mtd_info *mtd = platform_get_drvdata(pdev);
-	struct omap_nand_info *info = mtd->priv;
+	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
+							mtd);

 	platform_set_drvdata(pdev, NULL);
 	if (use_dma)
@@ -1064,7 +1065,9 @@ static int omap_nand_remove(struct platform_device *pdev)

 	/* Release NAND device, its internal structures and partitions */
 	nand_release(&info->mtd);
+	release_mem_region(info->phys_base, NAND_IO_SIZE);
 	iounmap(info->nand_pref_fifo_add);
+	gpmc_cs_free(info->gpmc_cs);
 	kfree(&info->mtd);
 	return 0;
 }
-- 
1.5.5

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Artem Bityutskiy]

On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
> Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
> Correct patch is below.
> 
> -vimal
> 
> 
> From: Vimal Singh <vimalsingh@ti.com>
> Date: Fri, 30 Oct 2009 14:54:29 +0530
> Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
> 
> Removing OMAP NAND driver, when loaded as a module, gives error and
> does not get success. This fixes this and makes driver loadable and
> removable run time.
> 
> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc.c |    2 ++
>  drivers/mtd/nand/omap2.c   |    5 ++++-
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 1587682..1d10b7b 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
>  	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>  	__raw_writel(val, reg_addr);
>  }
> +EXPORT_SYMBOL(gpmc_cs_write_reg);
> 
>  u32 gpmc_cs_read_reg(int cs, int idx)
>  {
> @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
>  	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>  	return __raw_readl(reg_addr);
>  }
> +EXPORT_SYMBOL(gpmc_cs_read_reg);

Why do you export these?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Vimal Singh]

On Tue, Nov 3, 2009 at 1:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
>> Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
>> Correct patch is below.
>>
>> -vimal
>>
>>
>> From: Vimal Singh <vimalsingh@ti.com>
>> Date: Fri, 30 Oct 2009 14:54:29 +0530
>> Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
>>
>> Removing OMAP NAND driver, when loaded as a module, gives error and
>> does not get success. This fixes this and makes driver loadable and
>> removable run time.
>>
>> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> ---
>>  arch/arm/mach-omap2/gpmc.c |    2 ++
>>  drivers/mtd/nand/omap2.c   |    5 ++++-
>>  2 files changed, 6 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> index 1587682..1d10b7b 100644
>> --- a/arch/arm/mach-omap2/gpmc.c
>> +++ b/arch/arm/mach-omap2/gpmc.c
>> @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
>>       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>>       __raw_writel(val, reg_addr);
>>  }
>> +EXPORT_SYMBOL(gpmc_cs_write_reg);
>>
>>  u32 gpmc_cs_read_reg(int cs, int idx)
>>  {
>> @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
>>       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>>       return __raw_readl(reg_addr);
>>  }
>> +EXPORT_SYMBOL(gpmc_cs_read_reg);
>
> Why do you export these?

These functions are called during prob. So, if not exported and driver
is compiled as a module compilation will break with error saying these
are not defined or not found.


-- 
Regards,
Vimal Singh

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Artem Bityutskiy]

On Tue, 2009-11-03 at 14:05 +0530, Vimal Singh wrote:
> On Tue, Nov 3, 2009 at 1:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
> >> Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
> >> Correct patch is below.
> >>
> >> -vimal
> >>
> >>
> >> From: Vimal Singh <vimalsingh@ti.com>
> >> Date: Fri, 30 Oct 2009 14:54:29 +0530
> >> Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
> >>
> >> Removing OMAP NAND driver, when loaded as a module, gives error and
> >> does not get success. This fixes this and makes driver loadable and
> >> removable run time.
> >>
> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> >> ---
> >>  arch/arm/mach-omap2/gpmc.c |    2 ++
> >>  drivers/mtd/nand/omap2.c   |    5 ++++-
> >>  2 files changed, 6 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> >> index 1587682..1d10b7b 100644
> >> --- a/arch/arm/mach-omap2/gpmc.c
> >> +++ b/arch/arm/mach-omap2/gpmc.c
> >> @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
> >>       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> >>       __raw_writel(val, reg_addr);
> >>  }
> >> +EXPORT_SYMBOL(gpmc_cs_write_reg);
> >>
> >>  u32 gpmc_cs_read_reg(int cs, int idx)
> >>  {
> >> @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
> >>       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> >>       return __raw_readl(reg_addr);
> >>  }
> >> +EXPORT_SYMBOL(gpmc_cs_read_reg);
> >
> > Why do you export these?
> 
> These functions are called during prob. So, if not exported and driver
> is compiled as a module compilation will break with error saying these
> are not defined or not found.

Could you show the place where they are called from please. I tried to
compile the omap NAND driver as module and it compiled fine. Probably
I'm missing something?

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Vimal Singh]

On Tue, Nov 3, 2009 at 2:09 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Tue, 2009-11-03 at 14:05 +0530, Vimal Singh wrote:
>> On Tue, Nov 3, 2009 at 1:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
>> > On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
>> >> Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
>> >> Correct patch is below.
>> >>
>> >> -vimal
>> >>
>> >>
>> >> From: Vimal Singh <vimalsingh@ti.com>
>> >> Date: Fri, 30 Oct 2009 14:54:29 +0530
>> >> Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
>> >>
>> >> Removing OMAP NAND driver, when loaded as a module, gives error and
>> >> does not get success. This fixes this and makes driver loadable and
>> >> removable run time.
>> >>
>> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> >> ---
>> >>  arch/arm/mach-omap2/gpmc.c |    2 ++
>> >>  drivers/mtd/nand/omap2.c   |    5 ++++-
>> >>  2 files changed, 6 insertions(+), 1 deletions(-)
>> >>
>> >> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> >> index 1587682..1d10b7b 100644
>> >> --- a/arch/arm/mach-omap2/gpmc.c
>> >> +++ b/arch/arm/mach-omap2/gpmc.c
>> >> @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
>> >>       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>> >>       __raw_writel(val, reg_addr);
>> >>  }
>> >> +EXPORT_SYMBOL(gpmc_cs_write_reg);
>> >>
>> >>  u32 gpmc_cs_read_reg(int cs, int idx)
>> >>  {
>> >> @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
>> >>       reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>> >>       return __raw_readl(reg_addr);
>> >>  }
>> >> +EXPORT_SYMBOL(gpmc_cs_read_reg);
>> >
>> > Why do you export these?
>>
>> These functions are called during prob. So, if not exported and driver
>> is compiled as a module compilation will break with error saying these
>> are not defined or not found.
>
> Could you show the place where they are called from please. I tried to
> compile the omap NAND driver as module and it compiled fine. Probably
> I'm missing something?

Code snippet from omap2.c, in prob call: line 919
-----------------------------------------------------------
        /* Enable RD PIN Monitoring Reg */
        if (pdata->dev_ready) {
                val  = gpmc_cs_read_reg(info->gpmc_cs, GPMC_CS_CONFIG1);
                val |= WR_RD_PIN_MONITORING;
                gpmc_cs_write_reg(info->gpmc_cs, GPMC_CS_CONFIG1, val);
        }

        val  = gpmc_cs_read_reg(info->gpmc_cs, GPMC_CS_CONFIG7);
        val &= ~(0xf << 8);
        val |=  (0xc & 0xf) << 8;
        gpmc_cs_write_reg(info->gpmc_cs, GPMC_CS_CONFIG7, val);
-----------------------------------------------------------

-- 
Regards,
Vimal Singh

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Artem Bityutskiy]

On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
> Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
> Correct patch is below.
> 
> -vimal
> 
> 
> From: Vimal Singh <vimalsingh@ti.com>
> Date: Fri, 30 Oct 2009 14:54:29 +0530
> Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
> 
> Removing OMAP NAND driver, when loaded as a module, gives error and
> does not get success. This fixes this and makes driver loadable and
> removable run time.
> 
> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc.c |    2 ++
>  drivers/mtd/nand/omap2.c   |    5 ++++-
>  2 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 1587682..1d10b7b 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
>  	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>  	__raw_writel(val, reg_addr);
>  }
> +EXPORT_SYMBOL(gpmc_cs_write_reg);
> 
>  u32 gpmc_cs_read_reg(int cs, int idx)
>  {
> @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
>  	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>  	return __raw_readl(reg_addr);
>  }
> +EXPORT_SYMBOL(gpmc_cs_read_reg);

You should get Tony's ack for this. I do not know the code, but on
surface it looks strange. Exporting so low-level functions is bad in
general, IMO. These function should either be inlined, or you should
invent better abstraction, so that you would not need to ever call these
functions from omap2.c. 

> 
>  /* TODO: Add support for gpmc_fck to clock framework and use it */
>  unsigned long gpmc_get_fclk_period(void)
> diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
> index 92573d5..ecc4d32 100644
> --- a/drivers/mtd/nand/omap2.c
> +++ b/drivers/mtd/nand/omap2.c
> @@ -1056,7 +1056,8 @@ out_free_info:
>  static int omap_nand_remove(struct platform_device *pdev)
>  {
>  	struct mtd_info *mtd = platform_get_drvdata(pdev);
> -	struct omap_nand_info *info = mtd->priv;
> +	struct omap_nand_info *info = container_of(mtd, struct omap_nand_info,
> +							mtd);
> 
>  	platform_set_drvdata(pdev, NULL);
>  	if (use_dma)
> @@ -1064,7 +1065,9 @@ static int omap_nand_remove(struct platform_device *pdev)
> 
>  	/* Release NAND device, its internal structures and partitions */
>  	nand_release(&info->mtd);
> +	release_mem_region(info->phys_base, NAND_IO_SIZE);
>  	iounmap(info->nand_pref_fifo_add);
> +	gpmc_cs_free(info->gpmc_cs);
>  	kfree(&info->mtd);
>  	return 0;
>  }
-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Tony Lindgren]

* Artem Bityutskiy <dedekind1@gmail.com> [091110 06:22]:
> On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
> > Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
> > Correct patch is below.
> > 
> > -vimal
> > 
> > 
> > From: Vimal Singh <vimalsingh@ti.com>
> > Date: Fri, 30 Oct 2009 14:54:29 +0530
> > Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
> > 
> > Removing OMAP NAND driver, when loaded as a module, gives error and
> > does not get success. This fixes this and makes driver loadable and
> > removable run time.
> > 
> > Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> > ---
> >  arch/arm/mach-omap2/gpmc.c |    2 ++
> >  drivers/mtd/nand/omap2.c   |    5 ++++-
> >  2 files changed, 6 insertions(+), 1 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> > index 1587682..1d10b7b 100644
> > --- a/arch/arm/mach-omap2/gpmc.c
> > +++ b/arch/arm/mach-omap2/gpmc.c
> > @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
> >  	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> >  	__raw_writel(val, reg_addr);
> >  }
> > +EXPORT_SYMBOL(gpmc_cs_write_reg);
> > 
> >  u32 gpmc_cs_read_reg(int cs, int idx)
> >  {
> > @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
> >  	reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> >  	return __raw_readl(reg_addr);
> >  }
> > +EXPORT_SYMBOL(gpmc_cs_read_reg);
> 
> You should get Tony's ack for this. I do not know the code, but on
> surface it looks strange. Exporting so low-level functions is bad in
> general, IMO. These function should either be inlined, or you should
> invent better abstraction, so that you would not need to ever call these
> functions from omap2.c. 

NAK. We don't want the drivers to tinker with these registers
directly. And really, the drivers should be platform independent.

This seems like a quick hack to add back the missing functionality
we threw out of the linux-omap tree. It was thrown out because there
were the same cut and paste hacks duplicated all over the place
tinkering with the GPMC registers directly.

We've fixed a lot of this by creating gpmc-onenand.c and gpmc-smc91x.c,
and that's clearly the way to go.

So instead of trying to add back the same old hacks, how about rather
spend that time to create something that we can use for all boards
using GPMC?

To me it looks like platform init like this should be done in a
generic way in arch/arm/mach-omap2/gpmc-nand.c the same way we have
gpmc-onenand.c and gpmc-smc91x.c.

Also, you should calculate the GPMC timings dynamically as they
can change based on the L3 frequency. Just take a look at the
gpmc-onenand.c and gpmc-smc91x.c.

Cheers,

Tony

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Vimal Singh]

On Wed, Nov 11, 2009 at 12:26 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Artem Bityutskiy <dedekind1@gmail.com> [091110 06:22]:
>> On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
>> > Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
>> > Correct patch is below.
>> >
>> > -vimal
>> >
>> >
>> > From: Vimal Singh <vimalsingh@ti.com>
>> > Date: Fri, 30 Oct 2009 14:54:29 +0530
>> > Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
>> >
>> > Removing OMAP NAND driver, when loaded as a module, gives error and
>> > does not get success. This fixes this and makes driver loadable and
>> > removable run time.
>> >
>> > Signed-off-by: Vimal Singh <vimalsingh@ti.com>
>> > ---
>> >  arch/arm/mach-omap2/gpmc.c |    2 ++
>> >  drivers/mtd/nand/omap2.c   |    5 ++++-
>> >  2 files changed, 6 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> > index 1587682..1d10b7b 100644
>> > --- a/arch/arm/mach-omap2/gpmc.c
>> > +++ b/arch/arm/mach-omap2/gpmc.c
>> > @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
>> >     reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>> >     __raw_writel(val, reg_addr);
>> >  }
>> > +EXPORT_SYMBOL(gpmc_cs_write_reg);
>> >
>> >  u32 gpmc_cs_read_reg(int cs, int idx)
>> >  {
>> > @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
>> >     reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
>> >     return __raw_readl(reg_addr);
>> >  }
>> > +EXPORT_SYMBOL(gpmc_cs_read_reg);
>>
>> You should get Tony's ack for this. I do not know the code, but on
>> surface it looks strange. Exporting so low-level functions is bad in
>> general, IMO. These function should either be inlined, or you should
>> invent better abstraction, so that you would not need to ever call these
>> functions from omap2.c.
>
> NAK. We don't want the drivers to tinker with these registers
> directly. And really, the drivers should be platform independent.
>
> This seems like a quick hack to add back the missing functionality
> we threw out of the linux-omap tree. It was thrown out because there
> were the same cut and paste hacks duplicated all over the place
> tinkering with the GPMC registers directly.
>
> We've fixed a lot of this by creating gpmc-onenand.c and gpmc-smc91x.c,
> and that's clearly the way to go.
>
> So instead of trying to add back the same old hacks, how about rather
> spend that time to create something that we can use for all boards
> using GPMC?
>
> To me it looks like platform init like this should be done in a
> generic way in arch/arm/mach-omap2/gpmc-nand.c the same way we have
> gpmc-onenand.c and gpmc-smc91x.c.
>
> Also, you should calculate the GPMC timings dynamically as they
> can change based on the L3 frequency. Just take a look at the
> gpmc-onenand.c and gpmc-smc91x.c.

Ok, I'll look into these and will try to do something generic.

-- 
Regards,
Vimal Singh

Re: [PATCH 2/3]NAND: OMAP: Fixing omap nand driver, compiled as module

[Tony Lindgren]

* Vimal Singh <vimal.newwork@gmail.com> [091110 20:46]:
> On Wed, Nov 11, 2009 at 12:26 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Artem Bityutskiy <dedekind1@gmail.com> [091110 06:22]:
> >> On Fri, 2009-10-30 at 14:57 +0530, Vimal Singh wrote:
> >> > Last time I forgot to 'git add' for 'arch/arm/mach-omap2/gpmc.c'... My bad.
> >> > Correct patch is below.
> >> >
> >> > -vimal
> >> >
> >> >
> >> > From: Vimal Singh <vimalsingh@ti.com>
> >> > Date: Fri, 30 Oct 2009 14:54:29 +0530
> >> > Subject: [PATCH] NAND: OMAP: Fixing omap nand driver, compiled as module
> >> >
> >> > Removing OMAP NAND driver, when loaded as a module, gives error and
> >> > does not get success. This fixes this and makes driver loadable and
> >> > removable run time.
> >> >
> >> > Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> >> > ---
> >> >  arch/arm/mach-omap2/gpmc.c |    2 ++
> >> >  drivers/mtd/nand/omap2.c   |    5 ++++-
> >> >  2 files changed, 6 insertions(+), 1 deletions(-)
> >> >
> >> > diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> >> > index 1587682..1d10b7b 100644
> >> > --- a/arch/arm/mach-omap2/gpmc.c
> >> > +++ b/arch/arm/mach-omap2/gpmc.c
> >> > @@ -88,6 +88,7 @@ void gpmc_cs_write_reg(int cs, int idx, u32 val)
> >> >     reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> >> >     __raw_writel(val, reg_addr);
> >> >  }
> >> > +EXPORT_SYMBOL(gpmc_cs_write_reg);
> >> >
> >> >  u32 gpmc_cs_read_reg(int cs, int idx)
> >> >  {
> >> > @@ -96,6 +97,7 @@ u32 gpmc_cs_read_reg(int cs, int idx)
> >> >     reg_addr = gpmc_base + GPMC_CS0 + (cs * GPMC_CS_SIZE) + idx;
> >> >     return __raw_readl(reg_addr);
> >> >  }
> >> > +EXPORT_SYMBOL(gpmc_cs_read_reg);
> >>
> >> You should get Tony's ack for this. I do not know the code, but on
> >> surface it looks strange. Exporting so low-level functions is bad in
> >> general, IMO. These function should either be inlined, or you should
> >> invent better abstraction, so that you would not need to ever call these
> >> functions from omap2.c.
> >
> > NAK. We don't want the drivers to tinker with these registers
> > directly. And really, the drivers should be platform independent.
> >
> > This seems like a quick hack to add back the missing functionality
> > we threw out of the linux-omap tree. It was thrown out because there
> > were the same cut and paste hacks duplicated all over the place
> > tinkering with the GPMC registers directly.
> >
> > We've fixed a lot of this by creating gpmc-onenand.c and gpmc-smc91x.c,
> > and that's clearly the way to go.
> >
> > So instead of trying to add back the same old hacks, how about rather
> > spend that time to create something that we can use for all boards
> > using GPMC?
> >
> > To me it looks like platform init like this should be done in a
> > generic way in arch/arm/mach-omap2/gpmc-nand.c the same way we have
> > gpmc-onenand.c and gpmc-smc91x.c.
> >
> > Also, you should calculate the GPMC timings dynamically as they
> > can change based on the L3 frequency. Just take a look at the
> > gpmc-onenand.c and gpmc-smc91x.c.
> 
> Ok, I'll look into these and will try to do something generic.

Thanks!

Tony