Patchwork [v4,02/39] ARM: OMAP2+: gpmc: Adapt to HWMOD

login
register
mail settings
Submitter Mohammed Afzal
Date May 1, 2012, 12:19 p.m.
Message ID <e0cf9678cb9a44d2a16401383ab8496eb825c291.1335874494.git.afzal@ti.com>
Download mbox | patch
Permalink /patch/156092/
State Not Applicable
Headers show

Comments

Mohammed Afzal - May 1, 2012, 12:19 p.m.
Create API for platforms to adapt gpmc to HWMOD

Signed-off-by: Afzal Mohammed <afzal@ti.com>
---
 arch/arm/mach-omap2/gpmc.c             |   52 +++++++++++++++++++++++---------
 arch/arm/plat-omap/include/plat/gpmc.h |    1 +
 2 files changed, 38 insertions(+), 15 deletions(-)
Hunter, Jon - May 1, 2012, 8:41 p.m.
Hi Afzal,

On 05/01/2012 07:19 AM, Afzal Mohammed wrote:
> Create API for platforms to adapt gpmc to HWMOD
> 
> Signed-off-by: Afzal Mohammed <afzal@ti.com>
> ---
>  arch/arm/mach-omap2/gpmc.c             |   52 +++++++++++++++++++++++---------
>  arch/arm/plat-omap/include/plat/gpmc.h |    1 +
>  2 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 12916f3..c8d07bb 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -33,6 +33,8 @@
>  
>  #include <plat/sdrc.h>
>  
> +#include <plat/omap_device.h>
> +
>  /* GPMC register offsets */
>  #define GPMC_REVISION		0x00
>  #define GPMC_SYSCONFIG		0x10
> @@ -276,6 +278,31 @@ unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns)
>  	return ticks * gpmc_get_fclk_period() / 1000;
>  }
>  
> +int __init omap_init_gpmc(struct gpmc_pdata *pdata)
> +{
> +	struct omap_hwmod *oh;
> +	struct platform_device *pdev;
> +	char *name = "omap-gpmc";
> +	char *oh_name = "gpmc";
> +
> +	pdata->clk_prd = gpmc_get_fclk_period();

Does this need to be done here? May be this should be done in the probe
function. You could store the handle to the main clk in the pdata.

> +
> +	oh = omap_hwmod_lookup(oh_name);
> +	if (!oh) {
> +		pr_err("Could not look up %s\n", oh_name);
> +		return -ENODEV;
> +	}
> +
> +	pdev = omap_device_build(name, -1, oh, pdata,
> +					sizeof(*pdata), NULL, 0, 0);
> +	if (IS_ERR(pdev)) {
> +		WARN(1, "Can't build omap_device for %s:%s.\n",
> +						name, oh->name);
> +		return PTR_ERR(pdev);
> +	}
> +
> +	return 0;
> +}
>  #ifdef DEBUG
>  static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>  			       int time, const char *name)
> @@ -843,24 +870,19 @@ static __devinit void gpmc_mem_init(void)
>  
>  static int __init gpmc_init(void)
>  {
> -	int ret = -EINVAL;
> -	char *ck = NULL;
> -
> -	if (cpu_is_omap24xx()) {
> -		ck = "core_l3_ck";
> -	} else if (cpu_is_omap34xx()) {
> -		ck = "gpmc_fck";
> -	} else if (cpu_is_omap44xx()) {
> -		ck = "gpmc_ck";
> -	}
> +	char *oh_name = "gpmc";
> +	struct omap_hwmod *oh;
>  
> -	if (WARN_ON(!ck))
> -		return ret;
> +	oh = omap_hwmod_lookup(oh_name);
> +	if (!oh) {
> +		pr_err("Could not look up %s\n", oh_name);
> +		return -ENODEV;
> +	}
>  
> -	gpmc_l3_clk = clk_get(NULL, ck);
> +	gpmc_l3_clk = clk_get(NULL, oh->main_clk);
>  	if (IS_ERR(gpmc_l3_clk)) {
> -		printk(KERN_ERR "Could not get GPMC clock %s\n", ck);
> -		BUG();
> +		pr_err("error: clk_get on %s\n", oh->main_clk);
> +		return -EINVAL;
>  	}
>  
>  	clk_enable(gpmc_l3_clk);

I would have thought we should be able to remove the gpmc_init function
completely by now. Most of the code should be moved to the probe function.

Also now with hwmod in place, we should be able to remove the
clk_enable/disable functions and use the pm_runtime APIs instead.

Cheers
Jon
Mohammed Afzal - May 3, 2012, 8:37 a.m.
Hi Jon,

On Wed, May 02, 2012 at 02:11:48, Hunter, Jon wrote:
> > +
> > +	pdata->clk_prd = gpmc_get_fclk_period();
> 
> Does this need to be done here? May be this should be done in the probe
> function. You could store the handle to the main clk in the pdata.

This is done so that migration of gpmc driver to the drivers folder
would be smooth, remember that this function will still live here.
 
> > +		pr_err("error: clk_get on %s\n", oh->main_clk);
> > +		return -EINVAL;
> >  	}
> >  
> >  	clk_enable(gpmc_l3_clk);
> 
> I would have thought we should be able to remove the gpmc_init function
> completely by now. Most of the code should be moved to the probe function.
> 
> Also now with hwmod in place, we should be able to remove the
> clk_enable/disable functions and use the pm_runtime APIs instead.

There was no plan to add rpm in this series, but now that you have
brought it up, I will adapt the driver to rpm.

Regards
Afzal
Hunter, Jon - May 4, 2012, 4:30 p.m.
Hi Afzal,

On 05/03/2012 03:37 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Wed, May 02, 2012 at 02:11:48, Hunter, Jon wrote:
>>> +
>>> +	pdata->clk_prd = gpmc_get_fclk_period();
>>
>> Does this need to be done here? May be this should be done in the probe
>> function. You could store the handle to the main clk in the pdata.
> 
> This is done so that migration of gpmc driver to the drivers folder
> would be smooth, remember that this function will still live here.

Sure, but why call this here?

>>> +		pr_err("error: clk_get on %s\n", oh->main_clk);
>>> +		return -EINVAL;
>>>  	}
>>>  
>>>  	clk_enable(gpmc_l3_clk);
>>
>> I would have thought we should be able to remove the gpmc_init function
>> completely by now. Most of the code should be moved to the probe function.
>>
>> Also now with hwmod in place, we should be able to remove the
>> clk_enable/disable functions and use the pm_runtime APIs instead.
> 
> There was no plan to add rpm in this series, but now that you have
> brought it up, I will adapt the driver to rpm.

Ok, great.

Jon
Mohammed Afzal - May 7, 2012, 11:02 a.m.
Hi Jon,

On Fri, May 04, 2012 at 22:00:21, Hunter, Jon wrote:
> >>> +
> >>> +	pdata->clk_prd = gpmc_get_fclk_period();
> >>
> >> Does this need to be done here? May be this should be done in the probe
> >> function. You could store the handle to the main clk in the pdata.
> > 
> > This is done so that migration of gpmc driver to the drivers folder
> > would be smooth, remember that this function will still live here.
> 
> Sure, but why call this here?

Clk_prd is a platform data passed to the driver, so platform code
updates it, where else can it be done ?

Regards
Afzal
Hunter, Jon - May 7, 2012, 4:12 p.m.
Hi Afzal,

On 05/07/2012 06:02 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Fri, May 04, 2012 at 22:00:21, Hunter, Jon wrote:
>>>>> +
>>>>> +	pdata->clk_prd = gpmc_get_fclk_period();
>>>>
>>>> Does this need to be done here? May be this should be done in the probe
>>>> function. You could store the handle to the main clk in the pdata.
>>>
>>> This is done so that migration of gpmc driver to the drivers folder
>>> would be smooth, remember that this function will still live here.
>>
>> Sure, but why call this here?
> 
> Clk_prd is a platform data passed to the driver, so platform code
> updates it, where else can it be done ?

The point is that you can pass what ever you like. You do not need to
pass the frequency you can pass the clock handle instead.

What happens it the clk_get() of the gpmc_l3_clk fails during the init?

In fact if you migrate to runtime pm then we should not have the clk_get
in the gpmc_init any more.

Jon
Mohammed Afzal - May 8, 2012, 6:24 a.m.
Hi Jon,

On Mon, May 07, 2012 at 21:42:35, Hunter, Jon wrote:

> > Clk_prd is a platform data passed to the driver, so platform code
> > updates it, where else can it be done ?
> 
> The point is that you can pass what ever you like. You do not need to
> pass the frequency you can pass the clock handle instead.

As clk rate is required in platform code for timing calculation, and
already available, period was passed

> 
> What happens it the clk_get() of the gpmc_l3_clk fails during the init?

Thanks for bringing this point, invalid clk_prd has to be handled by driver.

> 
> In fact if you migrate to runtime pm then we should not have the clk_get
> in the gpmc_init any more.

Even if converted to RPM, to get clk rate, clk_get has to be done
somewhere, right ?

Regards
Afzal
Hunter, Jon - May 8, 2012, 3:13 p.m.
Hi Afzal,

On 05/08/2012 01:24 AM, Mohammed, Afzal wrote:
> Hi Jon,
> 
> On Mon, May 07, 2012 at 21:42:35, Hunter, Jon wrote:
> 
>>> Clk_prd is a platform data passed to the driver, so platform code
>>> updates it, where else can it be done ?
>>
>> The point is that you can pass what ever you like. You do not need to
>> pass the frequency you can pass the clock handle instead.
> 
> As clk rate is required in platform code for timing calculation, and
> already available, period was passed

Sure.

>>
>> What happens it the clk_get() of the gpmc_l3_clk fails during the init?
> 
> Thanks for bringing this point, invalid clk_prd has to be handled by driver.
> 
>>
>> In fact if you migrate to runtime pm then we should not have the clk_get
>> in the gpmc_init any more.
> 
> Even if converted to RPM, to get clk rate, clk_get has to be done
> somewhere, right ?

Yes exactly. However, you could now do this in the driver itself like
the probe function. Or may be just pass the frequency of the gpmc fclk
to the driver and let the driver convert to the period. No reason we
need to convert to the period outside of the driver. Hence, we can keep
the function to do the conversion static in the driver and don't need to
expose externally.

Jon
Mohammed Afzal - May 10, 2012, 5:17 a.m.
Hi Jon,

On Tue, May 08, 2012 at 20:43:19, Hunter, Jon wrote:

> >> In fact if you migrate to runtime pm then we should not have the clk_get
> >> in the gpmc_init any more.
> > 
> > Even if converted to RPM, to get clk rate, clk_get has to be done
> > somewhere, right ?
> 
> Yes exactly. However, you could now do this in the driver itself like
> the probe function. Or may be just pass the frequency of the gpmc fclk
> to the driver and let the driver convert to the period. No reason we
> need to convert to the period outside of the driver. Hence, we can keep
> the function to do the conversion static in the driver and don't need to
> expose externally.

But platform need period before driver is probed (for calculating
peripheral timings to be passed for driver)

Regards
Afzal

Patch

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 12916f3..c8d07bb 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -33,6 +33,8 @@ 
 
 #include <plat/sdrc.h>
 
+#include <plat/omap_device.h>
+
 /* GPMC register offsets */
 #define GPMC_REVISION		0x00
 #define GPMC_SYSCONFIG		0x10
@@ -276,6 +278,31 @@  unsigned int gpmc_round_ns_to_ticks(unsigned int time_ns)
 	return ticks * gpmc_get_fclk_period() / 1000;
 }
 
+int __init omap_init_gpmc(struct gpmc_pdata *pdata)
+{
+	struct omap_hwmod *oh;
+	struct platform_device *pdev;
+	char *name = "omap-gpmc";
+	char *oh_name = "gpmc";
+
+	pdata->clk_prd = gpmc_get_fclk_period();
+
+	oh = omap_hwmod_lookup(oh_name);
+	if (!oh) {
+		pr_err("Could not look up %s\n", oh_name);
+		return -ENODEV;
+	}
+
+	pdev = omap_device_build(name, -1, oh, pdata,
+					sizeof(*pdata), NULL, 0, 0);
+	if (IS_ERR(pdev)) {
+		WARN(1, "Can't build omap_device for %s:%s.\n",
+						name, oh->name);
+		return PTR_ERR(pdev);
+	}
+
+	return 0;
+}
 #ifdef DEBUG
 static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
 			       int time, const char *name)
@@ -843,24 +870,19 @@  static __devinit void gpmc_mem_init(void)
 
 static int __init gpmc_init(void)
 {
-	int ret = -EINVAL;
-	char *ck = NULL;
-
-	if (cpu_is_omap24xx()) {
-		ck = "core_l3_ck";
-	} else if (cpu_is_omap34xx()) {
-		ck = "gpmc_fck";
-	} else if (cpu_is_omap44xx()) {
-		ck = "gpmc_ck";
-	}
+	char *oh_name = "gpmc";
+	struct omap_hwmod *oh;
 
-	if (WARN_ON(!ck))
-		return ret;
+	oh = omap_hwmod_lookup(oh_name);
+	if (!oh) {
+		pr_err("Could not look up %s\n", oh_name);
+		return -ENODEV;
+	}
 
-	gpmc_l3_clk = clk_get(NULL, ck);
+	gpmc_l3_clk = clk_get(NULL, oh->main_clk);
 	if (IS_ERR(gpmc_l3_clk)) {
-		printk(KERN_ERR "Could not get GPMC clock %s\n", ck);
-		BUG();
+		pr_err("error: clk_get on %s\n", oh->main_clk);
+		return -EINVAL;
 	}
 
 	clk_enable(gpmc_l3_clk);
diff --git a/arch/arm/plat-omap/include/plat/gpmc.h b/arch/arm/plat-omap/include/plat/gpmc.h
index 2eedd99..c5cf020 100644
--- a/arch/arm/plat-omap/include/plat/gpmc.h
+++ b/arch/arm/plat-omap/include/plat/gpmc.h
@@ -217,6 +217,7 @@  struct gpmc_pdata {
 };
 
 extern int gpmc_cs_reconfigure(char *name, int id, struct gpmc_cs_data *cs);
+extern int omap_init_gpmc(struct gpmc_pdata *pdata);
 
 extern unsigned int gpmc_ns_to_ticks(unsigned int time_ns);
 extern unsigned int gpmc_ps_to_ticks(unsigned int time_ps);