[PATCH 1/2] staging: tidspbridge: fix breakages due to CM reorganization

[PATCH 1/2] staging: tidspbridge: fix breakages due to CM reorganization

[Omar Ramirez Luna]

3.8-rc1 introduced changes in the clock management header files,
this resulted in compilation breakages for this driver.

Define this locally while APIs are made available, given that driver
code shouldn't include mach header files.

This fixes:
drivers/staging/tidspbridge/core/tiomap3430.c:550:13: error:
'OMAP3430_CM_AUTOIDLE_PLL' undeclared (first use in this function)
drivers/staging/tidspbridge/core/tiomap_io.c:416:13: error:
'OMAP3430_CM_CLKEN_PLL' undeclared (first use in this function)

Reported-by: Chen Gang <gang.chen@asianux.com>
Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
---
 drivers/staging/tidspbridge/core/_tiomap.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/_tiomap.h b/drivers/staging/tidspbridge/core/_tiomap.h
index 543a127..61ea135 100644
--- a/drivers/staging/tidspbridge/core/_tiomap.h
+++ b/drivers/staging/tidspbridge/core/_tiomap.h
@@ -40,6 +40,14 @@
 #include <dspbridge/sync.h>
 #include <dspbridge/clk.h>
 
+/*
+ * XXX These mach-omap2/ defines are wrong and should be removed.  No
+ * driver should read or write to PRM/CM registers directly; they
+ * should rely on OMAP core code to do this.
+ */
+#define OMAP3430_CM_AUTOIDLE_PLL	0x0034
+#define OMAP3430_CM_CLKEN_PLL		0x0004
+
 struct map_l4_peripheral {
 	u32 phys_addr;
 	u32 dsp_virt_addr;
-- 
1.7.4.4

[PATCH 2/2] staging: tidspbridge: use prepare/unprepare on dsp clocks

[Omar Ramirez Luna]

This solves runtime failures while trying to enable WDT3 related
functionality on firmware load, however it does affect other clocks
controlled by the driver. Seen on 3.8-rc1.

CCF provides clk_prepare and clk_unprepare for enable and disable
operations respectively, this needs to be called in the correct
order while handling clocks.

Code path to enable/disable dsp clocks can still be reached from an
atomic context, hence we can't use clk_prepare_enable and
clk_disable_unprepare yet.

Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
---
 drivers/staging/tidspbridge/core/dsp-clock.c |   13 ++++++++++++-
 drivers/staging/tidspbridge/core/wdt.c       |   12 ++++++++++--
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/tidspbridge/core/dsp-clock.c b/drivers/staging/tidspbridge/core/dsp-clock.c
index b647207..2f084e18 100644
--- a/drivers/staging/tidspbridge/core/dsp-clock.c
+++ b/drivers/staging/tidspbridge/core/dsp-clock.c
@@ -121,9 +121,13 @@ void dsp_clk_exit(void)
 	for (i = 0; i < DM_TIMER_CLOCKS; i++)
 		omap_dm_timer_free(timer[i]);
 
+	clk_unprepare(iva2_clk);
 	clk_put(iva2_clk);
+	clk_unprepare(ssi.sst_fck);
 	clk_put(ssi.sst_fck);
+	clk_unprepare(ssi.ssr_fck);
 	clk_put(ssi.ssr_fck);
+	clk_unprepare(ssi.ick);
 	clk_put(ssi.ick);
 }
 
@@ -145,14 +149,21 @@ void dsp_clk_init(void)
 	iva2_clk = clk_get(&dspbridge_device.dev, "iva2_ck");
 	if (IS_ERR(iva2_clk))
 		dev_err(bridge, "failed to get iva2 clock %p\n", iva2_clk);
+	else
+		clk_prepare(iva2_clk);
 
 	ssi.sst_fck = clk_get(&dspbridge_device.dev, "ssi_sst_fck");
 	ssi.ssr_fck = clk_get(&dspbridge_device.dev, "ssi_ssr_fck");
 	ssi.ick = clk_get(&dspbridge_device.dev, "ssi_ick");
 
-	if (IS_ERR(ssi.sst_fck) || IS_ERR(ssi.ssr_fck) || IS_ERR(ssi.ick))
+	if (IS_ERR(ssi.sst_fck) || IS_ERR(ssi.ssr_fck) || IS_ERR(ssi.ick)) {
 		dev_err(bridge, "failed to get ssi: sst %p, ssr %p, ick %p\n",
 					ssi.sst_fck, ssi.ssr_fck, ssi.ick);
+	} else {
+		clk_prepare(ssi.sst_fck);
+		clk_prepare(ssi.ssr_fck);
+		clk_prepare(ssi.ick);
+	}
 }
 
 /**
diff --git a/drivers/staging/tidspbridge/core/wdt.c b/drivers/staging/tidspbridge/core/wdt.c
index 1dce36f..7ff0e6c 100644
--- a/drivers/staging/tidspbridge/core/wdt.c
+++ b/drivers/staging/tidspbridge/core/wdt.c
@@ -63,11 +63,15 @@ int dsp_wdt_init(void)
 	dsp_wdt.fclk = clk_get(NULL, "wdt3_fck");
 
 	if (!IS_ERR(dsp_wdt.fclk)) {
+		clk_prepare(dsp_wdt.fclk);
+
 		dsp_wdt.iclk = clk_get(NULL, "wdt3_ick");
 		if (IS_ERR(dsp_wdt.iclk)) {
 			clk_put(dsp_wdt.fclk);
 			dsp_wdt.fclk = NULL;
 			ret = -EFAULT;
+		} else {
+			clk_prepare(dsp_wdt.iclk);
 		}
 	} else
 		ret = -EFAULT;
@@ -95,10 +99,14 @@ void dsp_wdt_exit(void)
 	free_irq(INT_34XX_WDT3_IRQ, &dsp_wdt);
 	tasklet_kill(&dsp_wdt.wdt3_tasklet);
 
-	if (dsp_wdt.fclk)
+	if (dsp_wdt.fclk) {
+		clk_unprepare(dsp_wdt.fclk);
 		clk_put(dsp_wdt.fclk);
-	if (dsp_wdt.iclk)
+	}
+	if (dsp_wdt.iclk) {
+		clk_unprepare(dsp_wdt.iclk);
 		clk_put(dsp_wdt.iclk);
+	}
 
 	dsp_wdt.fclk = NULL;
 	dsp_wdt.iclk = NULL;
-- 
1.7.4.4

Re: [PATCH 1/2] staging: tidspbridge: fix breakages due to CM reorganization

[Greg Kroah-Hartman]

On Mon, Dec 24, 2012 at 08:10:24AM -0600, Omar Ramirez Luna wrote:
> 3.8-rc1 introduced changes in the clock management header files,
> this resulted in compilation breakages for this driver.
> 
> Define this locally while APIs are made available, given that driver
> code shouldn't include mach header files.
> 
> This fixes:
> drivers/staging/tidspbridge/core/tiomap3430.c:550:13: error:
> 'OMAP3430_CM_AUTOIDLE_PLL' undeclared (first use in this function)
> drivers/staging/tidspbridge/core/tiomap_io.c:416:13: error:
> 'OMAP3430_CM_CLKEN_PLL' undeclared (first use in this function)
> 
> Reported-by: Chen Gang <gang.chen@asianux.com>
> Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>

Enric sent me a patch that just includes the proper .h file, which
should be better than doing this:

> --- a/drivers/staging/tidspbridge/core/_tiomap.h
> +++ b/drivers/staging/tidspbridge/core/_tiomap.h
> @@ -40,6 +40,14 @@
>  #include <dspbridge/sync.h>
>  #include <dspbridge/clk.h>
>  
> +/*
> + * XXX These mach-omap2/ defines are wrong and should be removed.  No
> + * driver should read or write to PRM/CM registers directly; they
> + * should rely on OMAP core code to do this.
> + */
> +#define OMAP3430_CM_AUTOIDLE_PLL	0x0034
> +#define OMAP3430_CM_CLKEN_PLL		0x0004

Don't define things that are already defined elsewhere...

I'll not apply this.

thanks,

greg k-h

Re: [PATCH 1/2] staging: tidspbridge: fix breakages due to CM reorganization

[Omar Ramirez Luna]

On Mon, Jan 7, 2013 at 5:03 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Dec 24, 2012 at 08:10:24AM -0600, Omar Ramirez Luna wrote:
>> 3.8-rc1 introduced changes in the clock management header files,
>> this resulted in compilation breakages for this driver.
>>
>> Define this locally while APIs are made available, given that driver
>> code shouldn't include mach header files.
>>
>> This fixes:
>> drivers/staging/tidspbridge/core/tiomap3430.c:550:13: error:
>> 'OMAP3430_CM_AUTOIDLE_PLL' undeclared (first use in this function)
>> drivers/staging/tidspbridge/core/tiomap_io.c:416:13: error:
>> 'OMAP3430_CM_CLKEN_PLL' undeclared (first use in this function)
>>
>> Reported-by: Chen Gang <gang.chen@asianux.com>
>> Signed-off-by: Omar Ramirez Luna <omar.ramirez@copitl.com>
>
> Enric sent me a patch that just includes the proper .h file, which
> should be better than doing this:

It looks better because the driver is already including related
headers in a similar fashion, but in reality those headers are under
arch/arm/mach-omap2 and the driver shouldn't have any business in
including headers from there.

>> --- a/drivers/staging/tidspbridge/core/_tiomap.h
>> +++ b/drivers/staging/tidspbridge/core/_tiomap.h
>> @@ -40,6 +40,14 @@
>>  #include <dspbridge/sync.h>
>>  #include <dspbridge/clk.h>
>>
>> +/*
>> + * XXX These mach-omap2/ defines are wrong and should be removed.  No
>> + * driver should read or write to PRM/CM registers directly; they
>> + * should rely on OMAP core code to do this.
>> + */
>> +#define OMAP3430_CM_AUTOIDLE_PLL     0x0034
>> +#define OMAP3430_CM_CLKEN_PLL                0x0004
>
> Don't define things that are already defined elsewhere...
>
> I'll not apply this.

Ok, not a problem, I'll be working on the real fix which is to get
APIs from the core code for the driver to use.

Cheers,

Omar