WATCHDOG: use base address from platform resources and make 2430 boot again

PATCH In review nishanth menon 5 participants 11 messages Apr 5, 2007 – Apr 26, 2007

Subsystems (3)

Arm (32-bit) Arm sub-architectures Char and misc drivers

lore

Share & tools

lore

nishanth menon

author

2007-04-05 01:55 UTC #0 lore raw

PATCH 1/1 WATCHDOG: use base address from platform resources and make 2430 boot again

Hi All,
Here is a patch for making 2430 bark again. In the process cleaned up the watchdog timer driver a bit.
Details:
Watchdog fixes for 2430
* modfied watchdog driver to use base address provided in platform defn
* in the process cleaned up a bunch of statics used.
* added 2430 baseaddress defn
Regards,
Nishanth Menon

Watchdog fixes for 2430
* modfied watchdog driver to use base address provided in platform defn
* in the process cleaned up a bunch of statics used.
* added 2430 baseaddress defn

Signed-off-by Nishanth Menon <[email protected]>

---
 arch/arm/plat-omap/devices.c     |    7 +
 drivers/char/watchdog/omap_wdt.c |  255 ++++++++++++++++++++++++---------------
 drivers/char/watchdog/omap_wdt.h |   28 +---
 3 files changed, 175 insertions(+), 115 deletions(-)

arch/arm/plat-omap/devices.c +7 -0

index f4d04d8..56690ac 100644
@@ -424,7 +424,14 @@ static inline void omap_init_uwire(void) {}
 #if	defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
 
 #ifdef CONFIG_ARCH_OMAP24XX
+
+#ifdef CONFIG_ARCH_OMAP2430
+/* WDT2 */
+#define	OMAP_WDT_BASE		0x49016000
+#else
 #define	OMAP_WDT_BASE		0x48022000
+#endif
+
 #else
 #define	OMAP_WDT_BASE		0xfffeb000
 #endif

drivers/char/watchdog/omap_wdt.c +159 ~~-96~~

439 lines not on this page · raw message

drivers/char/watchdog/omap_wdt.h +9 ~~-19~~

index 52a532a..511135d 100644
@@ -30,25 +30,15 @@
 #ifndef _OMAP_WATCHDOG_H
 #define _OMAP_WATCHDOG_H
 
-#define OMAP1610_WATCHDOG_BASE		0xfffeb000
-#define OMAP2420_WATCHDOG_BASE		0x48022000	/*WDT Timer 2 */
-
-#ifdef CONFIG_ARCH_OMAP24XX
-#define OMAP_WATCHDOG_BASE 		OMAP2420_WATCHDOG_BASE
-#else
-#define OMAP_WATCHDOG_BASE 		OMAP1610_WATCHDOG_BASE
-#define RM_RSTST_WKUP			0
-#endif
-
-#define OMAP_WATCHDOG_REV		(OMAP_WATCHDOG_BASE + 0x00)
-#define OMAP_WATCHDOG_SYS_CONFIG	(OMAP_WATCHDOG_BASE + 0x10)
-#define OMAP_WATCHDOG_STATUS		(OMAP_WATCHDOG_BASE + 0x14)
-#define OMAP_WATCHDOG_CNTRL		(OMAP_WATCHDOG_BASE + 0x24)
-#define OMAP_WATCHDOG_CRR		(OMAP_WATCHDOG_BASE + 0x28)
-#define OMAP_WATCHDOG_LDR		(OMAP_WATCHDOG_BASE + 0x2c)
-#define OMAP_WATCHDOG_TGR		(OMAP_WATCHDOG_BASE + 0x30)
-#define OMAP_WATCHDOG_WPS		(OMAP_WATCHDOG_BASE + 0x34)
-#define OMAP_WATCHDOG_SPR		(OMAP_WATCHDOG_BASE + 0x48)
+#define OMAP_WATCHDOG_REV			(0x00)
+#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
+#define OMAP_WATCHDOG_STATUS		(0x14)
+#define OMAP_WATCHDOG_CNTRL			(0x24)
+#define OMAP_WATCHDOG_CRR			(0x28)
+#define OMAP_WATCHDOG_LDR			(0x2c)
+#define OMAP_WATCHDOG_TGR			(0x30)
+#define OMAP_WATCHDOG_WPS			(0x34)
+#define OMAP_WATCHDOG_SPR			(0x48)
 
 /* Using the prescaler, the OMAP watchdog could go for many
  * months before firing.  These limits work without scaling,

[email protected]

2007-04-05 03:55 UTC #1 lore raw

OMAP2430 NAND Boot

Hi All,

I am trying to do a NAND boot on OMAP2430 SDP Board. I have created X-load.bin and converted it into x-load.raw. But I donot know how to convert x-load.raw into .ift format. Can some body help me out to do this.

1. What kind of tool is to be used to convert x-load.raw into x-load.raw.ift ?
2. Where can I get this tool ?
3. Or is it just editing some data in x-load.raw file or something else ?

I have used u-boot to burn itself back into NAND flash and now trying to program Xloader with CSST.

Regards
Sathish Kumar

Nishanth Menon

author

2007-04-05 11:16 UTC #2 lore raw

Re: OMAP2430 NAND Boot

[email protected] stated on 4/4/2007 10:55 PM:

Quoted 3 lines

1. What kind of tool is to be used to convert x-load.raw into x-load.raw.ift ?

CSST can help here..

Quoted 2 lines

2. Where can I get this tool ?

http://focus.ti.com/general/docs/wtbu/wtbugencontent.tsp?contentId=14645&navigationId=12013&templateId=6123

Quoted 3 lines

3. Or is it just editing some data in x-load.raw file or something else ?

no.. it is a matter of signint it.. read thru the quick start guide.. also the release notes.

Quoted 3 lines

I have used u-boot to burn itself back into NAND flash and now trying to program Xloader with CSST.

:) U knew it :).... u can actually do flashing of all using csst.. not just signing.. in fact there are stuff in the csst tool that will allow u to do board diagnostics also..
Regards,
Nishanth Menon

Trilok Soni

2007-04-05 10:20 UTC #3 lore raw

In reply to nishanth menon

Hi Nishanth,

Quoted 8 lines · nishanth menon

Hi All,
Here is a patch for making 2430 bark again. In the process cleaned up the watchdog timer driver a bit.
Details:
Watchdog fixes for 2430
* modfied watchdog driver to use base address provided in platform defn
* in the process cleaned up a bunch of statics used.
* added 2430 baseaddress defn

Few comments:

+ * ***
+ * History:
+ * --------
+ * 2007-04-04 Nishanth Menon - move to depend on platform resource and clean
+ * statics
*/

Changelog goes in the SCM metadata, not source files. Please avoid this.

-static void omap_wdt_ping(void)
+static void omap_wdt_ping(struct omap_wdt_dev *wdev) {
+ void __iomem *base=wdev->base;

space between *base and wdev->base is required for readability.

+ void __iomem *base;
+ base = wdev->base;

Please add atleast one empty line between above sentences and same with all other places in the driver.

+ wdev = (struct omap_wdt_dev *) platform_get_drvdata(omap_wdt_dev);

You don't need casting here, I think.

+ file->private_data = (void *) wdev;
+ wdev = (struct omap_wdt_dev *) file->private_data;

No need of casting.

You should also set drvdata to NULL on wdt remove and error recovery path in wdt_probe.

+ platform_set_drvdata(pdev, NULL);

Signature · 1 line

--Trilok Soni

Nishanth Menon

author

2007-04-05 12:43 UTC #4 lore raw

Hi Trilok,
Thanks for your time in helping review the patch. :)

Trilok Soni stated on 4/5/2007 5:20 AM:

Quoted 6 lines

Few comments:
+ * statics
*/
Changelog goes in the SCM metadata, not source files. Please avoid this.

Is this a new convention? No offence intended, but I like my signature on the files I touch extensively.. since it is not denied by coding_standards.txt, kinda pulling on leeway here.. :)

Quoted 8 lines

(quoted diff) +1 -1
-static void omap_wdt_ping(void)
+static void omap_wdt_ping(struct omap_wdt_dev *wdev)
{
+ void __iomem *base=wdev->base;
space between *base and wdev->base is required for readability.

yup.. typie typie....

Quoted 3 lines

+ wdev = (struct omap_wdt_dev *) platform_get_drvdata(omap_wdt_dev);
You don't need casting here, I think.

cleaner.. since get_drvdata returns void *.. makes sense to explain what form i am casting to-helps novice readers of code.. readability..

Quoted 5 lines

(quoted diff) +2 -0

+    file->private_data = (void *) wdev;
+    wdev = (struct omap_wdt_dev *) file->private_data;

No need of casting.

same reasoning..readability

Quoted 6 lines

You should also set drvdata to NULL on wdt remove and error recovery path in wdt_probe.
+ platform_set_drvdata(pdev, NULL);

good catch.. missed that one.

I have attached a follow up patch fixing the above mentioned issues..

Regards,
Nishanth Menon

Watchdog fixes for 2430
* modfied watchdog driver to use base address provided in platform defn
* in the process cleaned up a bunch of statics used.
* added 2430 baseaddress defn
* Addressed review reported by Trilok Soni.

Signed-off-by Nishanth Menon <[email protected]>

---
 arch/arm/plat-omap/devices.c     |    7 +
 drivers/char/watchdog/omap_wdt.c |  255 ++++++++++++++++++++++++---------------
 drivers/char/watchdog/omap_wdt.h |   28 +---
 3 files changed, 175 insertions(+), 115 deletions(-)

arch/arm/plat-omap/devices.c +7 -0

index f4d04d8..56690ac 100644
@@ -424,7 +424,14 @@ static inline void omap_init_uwire(void) {}
 #if	defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
 
 #ifdef CONFIG_ARCH_OMAP24XX
+
+#ifdef CONFIG_ARCH_OMAP2430
+/* WDT2 */
+#define	OMAP_WDT_BASE		0x49016000
+#else
 #define	OMAP_WDT_BASE		0x48022000
+#endif
+
 #else
 #define	OMAP_WDT_BASE		0xfffeb000
 #endif

drivers/char/watchdog/omap_wdt.c +160 ~~-96~~

440 lines not on this page · raw message

drivers/char/watchdog/omap_wdt.h +9 ~~-19~~

index 52a532a..511135d 100644
@@ -30,25 +30,15 @@
 #ifndef _OMAP_WATCHDOG_H
 #define _OMAP_WATCHDOG_H
 
-#define OMAP1610_WATCHDOG_BASE		0xfffeb000
-#define OMAP2420_WATCHDOG_BASE		0x48022000	/*WDT Timer 2 */
-
-#ifdef CONFIG_ARCH_OMAP24XX
-#define OMAP_WATCHDOG_BASE 		OMAP2420_WATCHDOG_BASE
-#else
-#define OMAP_WATCHDOG_BASE 		OMAP1610_WATCHDOG_BASE
-#define RM_RSTST_WKUP			0
-#endif
-
-#define OMAP_WATCHDOG_REV		(OMAP_WATCHDOG_BASE + 0x00)
-#define OMAP_WATCHDOG_SYS_CONFIG	(OMAP_WATCHDOG_BASE + 0x10)
-#define OMAP_WATCHDOG_STATUS		(OMAP_WATCHDOG_BASE + 0x14)
-#define OMAP_WATCHDOG_CNTRL		(OMAP_WATCHDOG_BASE + 0x24)
-#define OMAP_WATCHDOG_CRR		(OMAP_WATCHDOG_BASE + 0x28)
-#define OMAP_WATCHDOG_LDR		(OMAP_WATCHDOG_BASE + 0x2c)
-#define OMAP_WATCHDOG_TGR		(OMAP_WATCHDOG_BASE + 0x30)
-#define OMAP_WATCHDOG_WPS		(OMAP_WATCHDOG_BASE + 0x34)
-#define OMAP_WATCHDOG_SPR		(OMAP_WATCHDOG_BASE + 0x48)
+#define OMAP_WATCHDOG_REV			(0x00)
+#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
+#define OMAP_WATCHDOG_STATUS		(0x14)
+#define OMAP_WATCHDOG_CNTRL			(0x24)
+#define OMAP_WATCHDOG_CRR			(0x28)
+#define OMAP_WATCHDOG_LDR			(0x2c)
+#define OMAP_WATCHDOG_TGR			(0x30)
+#define OMAP_WATCHDOG_WPS			(0x34)
+#define OMAP_WATCHDOG_SPR			(0x48)
 
 /* Using the prescaler, the OMAP watchdog could go for many
  * months before firing.  These limits work without scaling,

Trilok Soni

2007-04-05 13:00 UTC #5 lore raw

Quoted 20 lines · Nishanth Menon

Hi Trilok,
Thanks for your time in helping review the patch. :)
Trilok Soni stated on 4/5/2007 5:20 AM:
Quoted 6 lines
Few comments:
+ * statics
*/
Changelog goes in the SCM metadata, not source files. Please avoid this.
Is this a new convention? No offence intended, but I like my signature on the files I touch extensively.. since it is not denied by coding_standards.txt, kinda pulling on leeway here.. :)
Quoted 7 lines
drivers/char/watchdog/omap_wdt.c +1 -1
-static void omap_wdt_ping(void)
+static void omap_wdt_ping(struct omap_wdt_dev *wdev)
{
+ void __iomem *base=wdev->base;

Quoted 7 lines

Quoted 1 line
space between *base and wdev->base is required for readability.
yup.. typie typie....
Quoted 3 lines
+ wdev = (struct omap_wdt_dev *) platform_get_drvdata(omap_wdt_dev);
You don't need casting here, I think.
cleaner.. since get_drvdata returns void *.. makes sense to explain what form i am casting to-helps novice readers of code.. readability..

But the compiler is intelligent. So, I don't think this unnecessary castings is needed at all.

Signature · 1 line

--Trilok Soni

Tony Lindgren

2007-04-05 13:30 UTC #6 lore raw

In reply to Nishanth Menon

Quoted 9 lines · Nishanth Menon

Hi Trilok,
Thanks for your time in helping review the patch. :)
Trilok Soni stated on 4/5/2007 5:20 AM:
Quoted 2 lines
Changelog goes in the SCM metadata, not source files. Please avoid this.
Is this a new convention? No offence intended, but I like my signature on the files I touch extensively.. since it is not denied by coding_standards.txt, kinda pulling on leeway here.. :)

The problem with this is that we will always get nailed on that trying to send the patches upstream, see current omap update on LKML right now for example. So we should generate patches that do not leave any reason for anybody to complain.

Regards,

Tony

Trilok Soni

2007-04-05 13:42 UTC #7 lore raw

In reply to Nishanth Menon

Hi Nishanth,

Quoted 2 lines · Nishanth Menon

Quoted 1 line
good catch.. missed that one.

Nop. You have only made change in wdt_probe, but missed wdt_remove for setting drvdata to NULL. Please do the same there.

Quoted 28 lines

static int omap_wdt_remove(struct platform_device *pdev) {

drivers/char/watchdog/omap_wdt.c +13 -9

-       struct resource *mem = platform_get_drvdata(pdev);
-       misc_deregister(&omap_wdt_miscdev);
-       release_resource(mem);
-       if (armwdt_ck)
-               clk_put(armwdt_ck);
-       if (mpu_wdt_ick)
-               clk_put(mpu_wdt_ick);
-       if (mpu_wdt_fck)
-               clk_put(mpu_wdt_fck);
+       struct omap_wdt_dev *wdev;
+       wdev = (struct omap_wdt_dev *) platform_get_drvdata(pdev);
+
+       misc_deregister(&(wdev->omap_wdt_miscdev));
+       release_resource(wdev->mem);
+       if (wdev->armwdt_ck)
+               clk_put(wdev->armwdt_ck);
+       if (wdev->mpu_wdt_ick)
+               clk_put(wdev->mpu_wdt_ick);
+       if (wdev->mpu_wdt_fck)
+               clk_put(wdev->mpu_wdt_fck);
+       kfree(wdev);
+       omap_wdt_dev = NULL;
        return 0;
 }

Signature · 1 line

--Trilok Soni

Nishanth Menon

author

2007-04-05 17:27 UTC #8 lore raw

PATCH 1/1 WATCHDOG: use base address from platform resources and make 2430 boot again

Hi Trilok and Tony,
Thanks a zillion for helping out with the patch
Trilok Soni stated on 4/5/2007 8:42 AM:

Quoted 2 lines

Nop. You have only made change in wdt_probe, but missed wdt_remove for setting drvdata to NULL. Please do the same there.

Done. Thanks for catching it - boy am I sleepy in the morning...

Trilok Soni stated on 4/5/2007 8:00 AM:

Quoted 2 lines

But the compiler is intelligent. So, I don't think this unnecessary castings is needed at all.

Yup.. accepted this too.
Tony Lindgren stated on 4/5/2007 8:30 AM:

Quoted 6 lines

The problem with this is that we will always get nailed on that trying to send the patches upstream, see current omap update on LKML right now for example. So we should generate patches that do not leave any reason for anybody to complain.

My hopes of eternal notoriety is dashed to pieces :(... I saw the discussion on LKML... ouch.... next time i put an '||' i will be careful :)

Hope the attached patch takes care of all concerns and i can listen to my 2430 bark again :) ..my expertise with generating git patches and signing off seems pretty basic.. so apologies in advance on it..

Regards,
Nishanth Menon

WATCHDOG: use base address from platform resources and make 2430 boot again Modfied watchdog driver to use base address provided in platform defn Cleaned up a bunch of statics used.
Added 2430 baseaddress defn
Addressed review comments for patch1 and 2 reported by Trilok Soni and Tony

Signed-off-by Nishanth Menon <[email protected]>

---
commit 4ddf95d4786b58c219a3713e17532e8fbf0b389f
tree 724a70469173fa6b7fe2f620f55874423408f3c0
parent 80564e176c4b6458940530b6e131b536aeab8b62
author Nishanth Menon <[email protected]> Thu, 05 Apr 2007 10:41:50 -0500 committer Nishanth Menon <[email protected]> Thu, 05 Apr 2007 10:41:50 -0500

 arch/arm/plat-omap/devices.c     |    7 +
 drivers/char/watchdog/omap_wdt.c |  252 ++++++++++++++++++++++++--------------
 drivers/char/watchdog/omap_wdt.h |   28 +---
 3 files changed, 172 insertions(+), 115 deletions(-)

arch/arm/plat-omap/devices.c +7 -0

index 32e4ec3..b8a0fcb 100644
@@ -420,7 +420,14 @@ static inline void omap_init_uwire(void) {}
 #if	defined(CONFIG_OMAP_WATCHDOG) || defined(CONFIG_OMAP_WATCHDOG_MODULE)
 
 #ifdef CONFIG_ARCH_OMAP24XX
+
+#ifdef CONFIG_ARCH_OMAP2430
+/* WDT2 */
+#define	OMAP_WDT_BASE		0x49016000
+#else
 #define	OMAP_WDT_BASE		0x48022000
+#endif
+
 #else
 #define	OMAP_WDT_BASE		0xfffeb000
 #endif

drivers/char/watchdog/omap_wdt.c +156 ~~-96~~

430 lines not on this page · raw message

drivers/char/watchdog/omap_wdt.h +9 ~~-19~~

index 52a532a..511135d 100644
@@ -30,25 +30,15 @@
 #ifndef _OMAP_WATCHDOG_H
 #define _OMAP_WATCHDOG_H
 
-#define OMAP1610_WATCHDOG_BASE		0xfffeb000
-#define OMAP2420_WATCHDOG_BASE		0x48022000	/*WDT Timer 2 */
-
-#ifdef CONFIG_ARCH_OMAP24XX
-#define OMAP_WATCHDOG_BASE 		OMAP2420_WATCHDOG_BASE
-#else
-#define OMAP_WATCHDOG_BASE 		OMAP1610_WATCHDOG_BASE
-#define RM_RSTST_WKUP			0
-#endif
-
-#define OMAP_WATCHDOG_REV		(OMAP_WATCHDOG_BASE + 0x00)
-#define OMAP_WATCHDOG_SYS_CONFIG	(OMAP_WATCHDOG_BASE + 0x10)
-#define OMAP_WATCHDOG_STATUS		(OMAP_WATCHDOG_BASE + 0x14)
-#define OMAP_WATCHDOG_CNTRL		(OMAP_WATCHDOG_BASE + 0x24)
-#define OMAP_WATCHDOG_CRR		(OMAP_WATCHDOG_BASE + 0x28)
-#define OMAP_WATCHDOG_LDR		(OMAP_WATCHDOG_BASE + 0x2c)
-#define OMAP_WATCHDOG_TGR		(OMAP_WATCHDOG_BASE + 0x30)
-#define OMAP_WATCHDOG_WPS		(OMAP_WATCHDOG_BASE + 0x34)
-#define OMAP_WATCHDOG_SPR		(OMAP_WATCHDOG_BASE + 0x48)
+#define OMAP_WATCHDOG_REV			(0x00)
+#define OMAP_WATCHDOG_SYS_CONFIG	(0x10)
+#define OMAP_WATCHDOG_STATUS		(0x14)
+#define OMAP_WATCHDOG_CNTRL			(0x24)
+#define OMAP_WATCHDOG_CRR			(0x28)
+#define OMAP_WATCHDOG_LDR			(0x2c)
+#define OMAP_WATCHDOG_TGR			(0x30)
+#define OMAP_WATCHDOG_WPS			(0x34)
+#define OMAP_WATCHDOG_SPR			(0x48)
 
 /* Using the prescaler, the OMAP watchdog could go for many
  * months before firing.  These limits work without scaling,

Tony Lindgren

2007-04-26 18:09 UTC #9 lore raw

Quoted 5 lines · Nishanth Menon

WATCHDOG: use base address from platform resources and make 2430 boot again Modfied watchdog driver to use base address provided in platform defn Cleaned up a bunch of statics used.
Added 2430 baseaddress defn
Addressed review comments for patch1 and 2 reported by Trilok Soni and Tony

Pushing today, I've split out the devices.c part into a separate patch.

Tony

David Brownell

2007-04-05 20:13 UTC #10 lore raw

In reply to Trilok Soni

Quoted 8 lines · Trilok Soni

Hi Nishanth,
Quoted 2 lines · Nishanth Menon
Quoted 1 line
good catch.. missed that one.
Nop. You have only made change in wdt_probe, but missed wdt_remove for setting drvdata to NULL. Please do the same there.

Actually drvata is undefined when there's no driver bound to a device. So nulling it is not needed. It may make you worry less about memory leakage (needless worries, right?), and isn't wrong -- not needed though.