Patchwork mtd: nand: davinci: Add cpufreq support

login
register
mail settings
Submitter Rajashekhara, Sudhakar
Date July 21, 2011, 9:39 a.m.
Message ID <1311241163-4576-1-git-send-email-sudhakar.raj@ti.com>
Download mbox | patch
Permalink /patch/106018/
State New
Headers show

Comments

Rajashekhara, Sudhakar - July 21, 2011, 9:39 a.m.
This patch adds cpufreq support for NAND driver. During cpufreq
transition, 'davinci_aemif_setup_timing()' function will be called
to reconfigure that AEMIF timings for the new frequency.

Tested on TI DA850/OMAP-L138 EVM.

Signed-off-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   54 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)
Rajashekhara, Sudhakar - Aug. 2, 2011, 6:33 a.m.
Hi David,

On Thu, Jul 21, 2011 at 15:09:23, Rajashekhara, Sudhakar wrote:
> This patch adds cpufreq support for NAND driver. During cpufreq
> transition, 'davinci_aemif_setup_timing()' function will be called
> to reconfigure that AEMIF timings for the new frequency.
> 
> Tested on TI DA850/OMAP-L138 EVM.
> 
> Signed-off-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>

I have not seen any comments on this patch. Can you pull it in into your tree?

Thanks,
Sudhakar
Ben Gardiner - Aug. 2, 2011, 1:02 p.m.
Hi Sudhakar,

On Thu, Jul 21, 2011 at 5:39 AM, Rajashekhara, Sudhakar
<sudhakar.raj@ti.com> wrote:
> This patch adds cpufreq support for NAND driver. During cpufreq
> transition, 'davinci_aemif_setup_timing()' function will be called
> to reconfigure that AEMIF timings for the new frequency.

I can see that this will work fine for dacinci_nand -- and it is a
welcomed feature also -- thank you for the effort to push this
upstream.

Seeing this support added directly to davinci_nand driver made me
think -- now this driver has another reference to platform-specific
functions in arch/arm/mach-davinci/aemif.c. Recently the c6x project
pushed a fix to remove references to a platform specific include. Not
that you are re-introducing this include but in a more general sense:
perhaps the features/functions of the AEMIF _bus_ should be removed
from the davinci_nand driver?

As for an in-tree driver example: how would support for cpufreq
transitions be added to the UI-board's NOR device which is currently
using "physmap-flash" driver?

> Tested on TI DA850/OMAP-L138 EVM.
>
> Signed-off-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   54 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 54 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 1f34951..7e764af 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -33,6 +33,7 @@
>  #include <linux/mtd/nand.h>
>  #include <linux/mtd/partitions.h>
>  #include <linux/slab.h>
> +#include <linux/cpufreq.h>
>
>  #include <mach/nand.h>
>  #include <mach/aemif.h>
> @@ -74,6 +75,9 @@ struct davinci_nand_info {
>        uint32_t                core_chipsel;
>
>        struct davinci_aemif_timing     *timing;
> +#ifdef CONFIG_CPU_FREQ
> +       struct notifier_block   freq_transition;
> +#endif
>  };
>
>  static DEFINE_SPINLOCK(davinci_nand_lock);
> @@ -519,6 +523,47 @@ static struct nand_ecclayout hwecc4_2048 __initconst = {
>        },
>  };
>
> +#ifdef CONFIG_CPU_FREQ
> +static int nand_davinci_cpufreq_transition(struct notifier_block *nb,
> +               unsigned long val, void *data)
> +{
> +       struct davinci_nand_info *info;
> +
> +       info = container_of(nb, struct davinci_nand_info, freq_transition);
> +
> +       if (val == CPUFREQ_POSTCHANGE)
> +               davinci_aemif_setup_timing(info->timing, info->base,
> +                               info->core_chipsel);
> +
> +       return 0;
> +}
> +
> +static inline int nand_davinci_cpufreq_register(struct davinci_nand_info *info)
> +{
> +       info->freq_transition.notifier_call = nand_davinci_cpufreq_transition;
> +
> +       return cpufreq_register_notifier(&info->freq_transition,
> +                       CPUFREQ_TRANSITION_NOTIFIER);
> +}
> +
> +static inline void nand_davinci_cpufreq_deregister(struct davinci_nand_info
> +               *info)
> +{
> +       cpufreq_unregister_notifier(&info->freq_transition,
> +                       CPUFREQ_TRANSITION_NOTIFIER);
> +}
> +#else
> +static inline int nand_davinci_cpufreq_register(struct davinci_nand_info *info)
> +{
> +       return 0;
> +}
> +
> +static inline void nand_davinci_cpufreq_deregister(struct davinci_nand_info
> +               *info)
> +{
> +}
> +#endif
> +
>  static int __init nand_davinci_probe(struct platform_device *pdev)
>  {
>        struct davinci_nand_pdata       *pdata = pdev->dev.platform_data;
> @@ -782,12 +827,19 @@ syndrome_done:
>        if (ret < 0)
>                goto err_scan;
>
> +       ret = nand_davinci_cpufreq_register(info);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to register cpufreq\n");
> +               goto err_cpu_freq_fail;
> +       }
> +
>        val = davinci_nand_readl(info, NRCSR_OFFSET);
>        dev_info(&pdev->dev, "controller rev. %d.%d\n",
>               (val >> 8) & 0xff, val & 0xff);
>
>        return 0;
>
> +err_cpu_freq_fail:
>  err_scan:
>  err_timing:
>        clk_disable(info->clk);
> @@ -818,6 +870,8 @@ static int __exit nand_davinci_remove(struct platform_device *pdev)
>        struct davinci_nand_info *info = platform_get_drvdata(pdev);
>        int status;
>
> +       nand_davinci_cpufreq_deregister(info);
> +
>        status = mtd_device_unregister(&info->mtd);
>
>        spin_lock_irq(&davinci_nand_lock);
> --
> 1.7.1
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>
Sekhar Nori - Aug. 2, 2011, 4 p.m.
Hi Ben,

On Tue, Aug 02, 2011 at 18:32:39, Ben Gardiner wrote:
> Hi Sudhakar,
> 
> On Thu, Jul 21, 2011 at 5:39 AM, Rajashekhara, Sudhakar
> <sudhakar.raj@ti.com> wrote:
> > This patch adds cpufreq support for NAND driver. During cpufreq
> > transition, 'davinci_aemif_setup_timing()' function will be called
> > to reconfigure that AEMIF timings for the new frequency.
> 
> I can see that this will work fine for dacinci_nand -- and it is a
> welcomed feature also -- thank you for the effort to push this
> upstream.
> 
> Seeing this support added directly to davinci_nand driver made me
> think -- now this driver has another reference to platform-specific
> functions in arch/arm/mach-davinci/aemif.c. Recently the c6x project
> pushed a fix to remove references to a platform specific include. Not
> that you are re-introducing this include but in a more general sense:
> perhaps the features/functions of the AEMIF _bus_ should be removed
> from the davinci_nand driver?

Good point! arch/arm/mach-davinci/aemif.c should be moved to drivers/
Because AEMIF is present on C6x and OMAP architectures as well and
not limited to DaVinci.

I am not sure if there are any existing "memory interface" drivers.
Moving to drivers/misc/ may be a good point to start.

Making AEMIF a new bus type is an interesting idea as well.

Thanks,
Sekhar
Ben Gardiner - Aug. 2, 2011, 4:13 p.m.
Hi Sekhar,

On Tue, Aug 2, 2011 at 12:00 PM, Nori, Sekhar <nsekhar@ti.com> wrote:
> Hi Ben,
>
> On Tue, Aug 02, 2011 at 18:32:39, Ben Gardiner wrote:
>> Hi Sudhakar,
>>
>> On Thu, Jul 21, 2011 at 5:39 AM, Rajashekhara, Sudhakar
>> <sudhakar.raj@ti.com> wrote:
>> > This patch adds cpufreq support for NAND driver. During cpufreq
>> > transition, 'davinci_aemif_setup_timing()' function will be called
>> > to reconfigure that AEMIF timings for the new frequency.
>>
>> I can see that this will work fine for dacinci_nand -- and it is a
>> welcomed feature also -- thank you for the effort to push this
>> upstream.
>>
>> Seeing this support added directly to davinci_nand driver made me
>> think -- now this driver has another reference to platform-specific
>> functions in arch/arm/mach-davinci/aemif.c. Recently the c6x project
>> pushed a fix to remove references to a platform specific include. Not
>> that you are re-introducing this include but in a more general sense:
>> perhaps the features/functions of the AEMIF _bus_ should be removed
>> from the davinci_nand driver?
>
> Good point! arch/arm/mach-davinci/aemif.c should be moved to drivers/
> Because AEMIF is present on C6x and OMAP architectures as well and
> not limited to DaVinci.

Thanks, glad to help.

> I am not sure if there are any existing "memory interface" drivers.
> Moving to drivers/misc/ may be a good point to start.
>
> Making AEMIF a new bus type is an interesting idea as well.

I don't really know what the best fit is; but I imagine a bus type
driver is suitable since it best reflects the actual hierarchy of
devices and hence should yield the best runtime power management...

Whether or not it is a bus type driver, I think the AEMIF driver when
migrated should ideally be taking the timings and other AEMIF
configuration parameters (bus width, extended wait, stobe mode) as
platform data instead of the direct register writes from
board-da850-evm.c. This suggests one driver instance per chip-select.

Note that I am not volunteering to do the AEMIF driver development
here :) Although I do have some patches in development that would be
affected by the merge of such a feature.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca
Artem Bityutskiy - Aug. 15, 2011, 3:20 p.m.
On Tue, 2011-08-02 at 12:03 +0530, Rajashekhara, Sudhakar wrote:
> Hi David,
> 
> On Thu, Jul 21, 2011 at 15:09:23, Rajashekhara, Sudhakar wrote:
> > This patch adds cpufreq support for NAND driver. During cpufreq
> > transition, 'davinci_aemif_setup_timing()' function will be called
> > to reconfigure that AEMIF timings for the new frequency.
> > 
> > Tested on TI DA850/OMAP-L138 EVM.
> > 
> > Signed-off-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>
> 
> I have not seen any comments on this patch. Can you pull it in into your tree?

Would be nice to get a Tested-by or Acked-by from someone.
Artem Bityutskiy - Aug. 15, 2011, 3:21 p.m.
On Thu, 2011-07-21 at 15:09 +0530, Rajashekhara, Sudhakar wrote:
> This patch adds cpufreq support for NAND driver. During cpufreq
> transition, 'davinci_aemif_setup_timing()' function will be called
> to reconfigure that AEMIF timings for the new frequency.
> 
> Tested on TI DA850/OMAP-L138 EVM.
> 
> Signed-off-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   54 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 54 insertions(+), 0 deletions(-)

Please, include re-send the patch with linux-omap in CC.
Rajashekhara, Sudhakar - Aug. 16, 2011, 5:05 a.m.
Hi,

On Mon, Aug 15, 2011 at 20:50:07, Artem Bityutskiy wrote:
> On Tue, 2011-08-02 at 12:03 +0530, Rajashekhara, Sudhakar wrote:
> > Hi David,
> > 
> > On Thu, Jul 21, 2011 at 15:09:23, Rajashekhara, Sudhakar wrote:
> > > This patch adds cpufreq support for NAND driver. During cpufreq 
> > > transition, 'davinci_aemif_setup_timing()' function will be called 
> > > to reconfigure that AEMIF timings for the new frequency.
> > > 
> > > Tested on TI DA850/OMAP-L138 EVM.
> > > 
> > > Signed-off-by: Rajashekhara, Sudhakar <sudhakar.raj@ti.com>
> > 
> > I have not seen any comments on this patch. Can you pull it in into your tree?
> 
> Would be nice to get a Tested-by or Acked-by from someone.
> 

There were few comments on this patch series from Ben Gardiner. I am working on them.
Will post an updated patch set to the list.

Thanks,
Sudhakar

Patch

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 1f34951..7e764af 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -33,6 +33,7 @@ 
 #include <linux/mtd/nand.h>
 #include <linux/mtd/partitions.h>
 #include <linux/slab.h>
+#include <linux/cpufreq.h>
 
 #include <mach/nand.h>
 #include <mach/aemif.h>
@@ -74,6 +75,9 @@  struct davinci_nand_info {
 	uint32_t		core_chipsel;
 
 	struct davinci_aemif_timing	*timing;
+#ifdef CONFIG_CPU_FREQ
+	struct notifier_block	freq_transition;
+#endif
 };
 
 static DEFINE_SPINLOCK(davinci_nand_lock);
@@ -519,6 +523,47 @@  static struct nand_ecclayout hwecc4_2048 __initconst = {
 	},
 };
 
+#ifdef CONFIG_CPU_FREQ
+static int nand_davinci_cpufreq_transition(struct notifier_block *nb,
+		unsigned long val, void *data)
+{
+	struct davinci_nand_info *info;
+
+	info = container_of(nb, struct davinci_nand_info, freq_transition);
+
+	if (val == CPUFREQ_POSTCHANGE)
+		davinci_aemif_setup_timing(info->timing, info->base,
+				info->core_chipsel);
+
+	return 0;
+}
+
+static inline int nand_davinci_cpufreq_register(struct davinci_nand_info *info)
+{
+	info->freq_transition.notifier_call = nand_davinci_cpufreq_transition;
+
+	return cpufreq_register_notifier(&info->freq_transition,
+			CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+static inline void nand_davinci_cpufreq_deregister(struct davinci_nand_info
+		*info)
+{
+	cpufreq_unregister_notifier(&info->freq_transition,
+			CPUFREQ_TRANSITION_NOTIFIER);
+}
+#else
+static inline int nand_davinci_cpufreq_register(struct davinci_nand_info *info)
+{
+	return 0;
+}
+
+static inline void nand_davinci_cpufreq_deregister(struct davinci_nand_info
+		*info)
+{
+}
+#endif
+
 static int __init nand_davinci_probe(struct platform_device *pdev)
 {
 	struct davinci_nand_pdata	*pdata = pdev->dev.platform_data;
@@ -782,12 +827,19 @@  syndrome_done:
 	if (ret < 0)
 		goto err_scan;
 
+	ret = nand_davinci_cpufreq_register(info);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register cpufreq\n");
+		goto err_cpu_freq_fail;
+	}
+
 	val = davinci_nand_readl(info, NRCSR_OFFSET);
 	dev_info(&pdev->dev, "controller rev. %d.%d\n",
 	       (val >> 8) & 0xff, val & 0xff);
 
 	return 0;
 
+err_cpu_freq_fail:
 err_scan:
 err_timing:
 	clk_disable(info->clk);
@@ -818,6 +870,8 @@  static int __exit nand_davinci_remove(struct platform_device *pdev)
 	struct davinci_nand_info *info = platform_get_drvdata(pdev);
 	int status;
 
+	nand_davinci_cpufreq_deregister(info);
+
 	status = mtd_device_unregister(&info->mtd);
 
 	spin_lock_irq(&davinci_nand_lock);