| Message ID | 1390295561-3466-6-git-send-email-ezequiel.garcia@free-electrons.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, Jan 21, 2014 at 06:12:31AM -0300, Ezequiel Garcia wrote: > In order to support other SoC, it's required to distinguish > the 'control' timer register, from the 'rstout' register > that enables system reset on watchdog expiration. > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (!res) > + return -ENODEV; ^^^^^^^^^^^^^^^^^^^^^^ This change seems to break compatibility with existing DT files that have only a single entry in reg? Can the value be defaulted some how if missing? Regards, Jason -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 21 January 2014 16:33:21 Jason Gunthorpe wrote: > On Tue, Jan 21, 2014 at 06:12:31AM -0300, Ezequiel Garcia wrote: > > In order to support other SoC, it's required to distinguish > > the 'control' timer register, from the 'rstout' register > > that enables system reset on watchdog expiration. > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > + if (!res) > > + return -ENODEV; > ^^^^^^^^^^^^^^^^^^^^^^ > > This change seems to break compatibility with existing DT files that > have only a single entry in reg? > > Can the value be defaulted some how if missing? I think this is a direct consequence of the attempt to remove the header file dependency, since the RSTOUTn_MASK macro is defined in mach/bridge-regs.h. I don't see a good way out that would preserve backwards compatibility, other than hardcoding the physical address in the driver, which seems just as bad as breaking compatibility. That said, it is always the same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding a fallback would technically work, but we should print a fat warning at boot time if we actually fall back to that. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jason, Arnd: On Wed, Jan 22, 2014 at 10:48:37AM +0100, Arnd Bergmann wrote: > On Tuesday 21 January 2014 16:33:21 Jason Gunthorpe wrote: > > On Tue, Jan 21, 2014 at 06:12:31AM -0300, Ezequiel Garcia wrote: > > > In order to support other SoC, it's required to distinguish > > > the 'control' timer register, from the 'rstout' register > > > that enables system reset on watchdog expiration. > > > > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > > + if (!res) > > > + return -ENODEV; > > ^^^^^^^^^^^^^^^^^^^^^^ > > > > This change seems to break compatibility with existing DT files that > > have only a single entry in reg? > > > > Can the value be defaulted some how if missing? > > I think this is a direct consequence of the attempt to remove the > header file dependency, since the RSTOUTn_MASK macro is defined > in mach/bridge-regs.h. > Exactly. Just for the record, I warned about this a while back: http://www.spinics.net/lists/arm-kernel/msg270163.html That said, it truly sucks to break compatibility so finding a way to keep backwards compatibility would be certainly desirable. > I don't see a good way out that would preserve backwards compatibility, > other than hardcoding the physical address in the driver, which seems > just as bad as breaking compatibility. That said, it is always the > same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and > Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding > a fallback would technically work, but we should print a fat warning at > boot time if we actually fall back to that. > Yes, I was thinking just about this. Namely: If the second resource is missing *and* the compatible-string is "orion-watchdog" (or it's not DT-registered), then use a hardcoded RSTOUTn_MASK address. And pr_warn() something. Thanks for the feedback,
On Wed, Jan 22, 2014 at 03:01:01PM -0300, Ezequiel Garcia wrote: > On Wed, Jan 22, 2014 at 01:21:36PM -0300, Ezequiel Garcia wrote: > > > I don't see a good way out that would preserve backwards compatibility, > > > other than hardcoding the physical address in the driver, which seems > > > just as bad as breaking compatibility. That said, it is always the > > > same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and > > > Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding > > > a fallback would technically work, but we should print a fat warning at > > > boot time if we actually fall back to that. > > > > > > > Yes, I was thinking just about this. Namely: > > > [..] > > How about something like this? I liked Arnd's idea to use an offset from the first register. With the mbus driver we can now actually change the 0xF1.. prefix via the DT. > + dev_warn(&pdev->dev, "falling back to harcoded RSTOUT reg 0x%x\n", > + ORION_RSTOUT_MASK); Maybe: dev_warn(&pdev->dev, FW_BUG "falling back to harcoded RSTOUT reg 0x%x\n", ? > @@ -139,10 +166,7 @@ static int orion_wdt_probe(struct platform_device *pdev) > if (!wdt_reg) > return -ENOMEM; > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > - if (!res) > - return -ENODEV; > - wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res)); > + wdt_rstout = try_compat_rstout_ioremap(pdev); > if (!wdt_rstout) > return -ENOMEM; ENOMEM is probably not the right errno? Is there a standard errno for malformed DT? Jason -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 22 January 2014 11:14:09 Jason Gunthorpe wrote: > On Wed, Jan 22, 2014 at 03:01:01PM -0300, Ezequiel Garcia wrote: > > On Wed, Jan 22, 2014 at 01:21:36PM -0300, Ezequiel Garcia wrote: > > > > I don't see a good way out that would preserve backwards compatibility, > > > > other than hardcoding the physical address in the driver, which seems > > > > just as bad as breaking compatibility. That said, it is always the > > > > same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and > > > > Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding > > > > a fallback would technically work, but we should print a fat warning at > > > > boot time if we actually fall back to that. > > > > > > > > > > Yes, I was thinking just about this. Namely: > > > > > [..] > > > > How about something like this? > > I liked Arnd's idea to use an offset from the first register. With the > mbus driver we can now actually change the 0xF1.. prefix via the DT. That wasn't actually my idea, but it does sound reasonable. > > + dev_warn(&pdev->dev, "falling back to harcoded RSTOUT reg 0x%x\n", > > + ORION_RSTOUT_MASK); > > Maybe: > dev_warn(&pdev->dev, FW_BUG "falling back to harcoded RSTOUT reg 0x%x\n", I was thinking of WARN_ON(), i.e. something that users will actually notice ;-) > > @@ -139,10 +166,7 @@ static int orion_wdt_probe(struct platform_device *pdev) > > if (!wdt_reg) > > return -ENOMEM; > > > > - res = platform_get_resource(pdev, IORESOURCE_MEM, 1); > > - if (!res) > > - return -ENODEV; > > - wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res)); > > + wdt_rstout = try_compat_rstout_ioremap(pdev); > > if (!wdt_rstout) > > return -ENOMEM; > > ENOMEM is probably not the right errno? Is there a standard errno for > malformed DT? I don't think so. I'd probably use ENODEV, but it's not ideal. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 22, 2014 at 11:14:09AM -0700, Jason Gunthorpe wrote: > On Wed, Jan 22, 2014 at 03:01:01PM -0300, Ezequiel Garcia wrote: > > On Wed, Jan 22, 2014 at 01:21:36PM -0300, Ezequiel Garcia wrote: > > > > I don't see a good way out that would preserve backwards compatibility, > > > > other than hardcoding the physical address in the driver, which seems > > > > just as bad as breaking compatibility. That said, it is always the > > > > same constant (0xf1000000 + 0x20000 + 0x0108) on Dove, Kirkwood and > > > > Orion5x (not on mv78xx0, but that doesn't use the wdt), so hardcoding > > > > a fallback would technically work, but we should print a fat warning at > > > > boot time if we actually fall back to that. > > > > > > > > > > Yes, I was thinking just about this. Namely: > > > > > [..] > > > > How about something like this? > > I liked Arnd's idea to use an offset from the first register. With the > mbus driver we can now actually change the 0xF1.. prefix via the DT. > Good idea. Maybe extracting the base address from the timer control register: wdt_reg & 0xff000000 ?
diff --git a/Documentation/devicetree/bindings/watchdog/marvel.txt b/Documentation/devicetree/bindings/watchdog/marvel.txt index 0731fbd..1544fe9 100644 --- a/Documentation/devicetree/bindings/watchdog/marvel.txt +++ b/Documentation/devicetree/bindings/watchdog/marvel.txt @@ -3,7 +3,9 @@ Required Properties: - Compatibility : "marvell,orion-wdt" -- reg : Address of the timer registers +- reg : Should contain two entries: first one with the + timer control address, second one with the + rstout enable address. Optional properties: @@ -14,7 +16,7 @@ Example: wdt@20300 { compatible = "marvell,orion-wdt"; - reg = <0x20300 0x28>; + reg = <0x20300 0x28>, <0x20108 0x4>; interrupts = <3>; timeout-sec = <10>; status = "okay"; diff --git a/arch/arm/mach-dove/include/mach/bridge-regs.h b/arch/arm/mach-dove/include/mach/bridge-regs.h index 5362df3..f4a5b34 100644 --- a/arch/arm/mach-dove/include/mach/bridge-regs.h +++ b/arch/arm/mach-dove/include/mach/bridge-regs.h @@ -21,6 +21,7 @@ #define CPU_CTRL_PCIE1_LINK 0x00000008 #define RSTOUTn_MASK (BRIDGE_VIRT_BASE + 0x0108) +#define RSTOUTn_MASK_PHYS (BRIDGE_PHYS_BASE + 0x0108) #define SOFT_RESET_OUT_EN 0x00000004 #define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c) diff --git a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h index 8b9d1c9..60f6421 100644 --- a/arch/arm/mach-kirkwood/include/mach/bridge-regs.h +++ b/arch/arm/mach-kirkwood/include/mach/bridge-regs.h @@ -21,6 +21,7 @@ #define CPU_RESET 0x00000002 #define RSTOUTn_MASK (BRIDGE_VIRT_BASE + 0x0108) +#define RSTOUTn_MASK_PHYS (BRIDGE_PHYS_BASE + 0x0108) #define SOFT_RESET_OUT_EN 0x00000004 #define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c) diff --git a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h index 5f03484..e20d6da 100644 --- a/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h +++ b/arch/arm/mach-mv78xx0/include/mach/bridge-regs.h @@ -15,6 +15,7 @@ #define L2_WRITETHROUGH 0x00020000 #define RSTOUTn_MASK (BRIDGE_VIRT_BASE + 0x0108) +#define RSTOUTn_MASK_PHYS (BRIDGE_PHYS_BASE + 0x0108) #define SOFT_RESET_OUT_EN 0x00000004 #define SYSTEM_SOFT_RESET (BRIDGE_VIRT_BASE + 0x010c) diff --git a/arch/arm/mach-orion5x/include/mach/bridge-regs.h b/arch/arm/mach-orion5x/include/mach/bridge-regs.h index f727d03..5766e3f 100644 --- a/arch/arm/mach-orion5x/include/mach/bridge-regs.h +++ b/arch/arm/mach-orion5x/include/mach/bridge-regs.h @@ -18,6 +18,7 @@ #define CPU_CTRL (ORION5X_BRIDGE_VIRT_BASE + 0x104) #define RSTOUTn_MASK (ORION5X_BRIDGE_VIRT_BASE + 0x108) +#define RSTOUTn_MASK_PHYS (ORION5X_BRIDGE_PHYS_BASE + 0x108) #define CPU_SOFT_RESET (ORION5X_BRIDGE_VIRT_BASE + 0x10c) diff --git a/arch/arm/plat-orion/common.c b/arch/arm/plat-orion/common.c index c66d163..3375037 100644 --- a/arch/arm/plat-orion/common.c +++ b/arch/arm/plat-orion/common.c @@ -594,14 +594,16 @@ void __init orion_spi_1_init(unsigned long mapbase) /***************************************************************************** * Watchdog ****************************************************************************/ -static struct resource orion_wdt_resource = - DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x28); +static struct resource orion_wdt_resource[] = { + DEFINE_RES_MEM(TIMER_PHYS_BASE, 0x04), + DEFINE_RES_MEM(RSTOUTn_MASK_PHYS, 0x04), +}; static struct platform_device orion_wdt_device = { .name = "orion_wdt", .id = -1, - .num_resources = 1, - .resource = &orion_wdt_resource, + .num_resources = ARRAY_SIZE(orion_wdt_resource), + .resource = orion_wdt_resource, }; void __init orion_wdt_init(void) diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c index 06e766f..9c7d695 100644 --- a/drivers/watchdog/orion_wdt.c +++ b/drivers/watchdog/orion_wdt.c @@ -47,6 +47,7 @@ static unsigned int wdt_max_duration; /* (seconds) */ static struct clk *clk; static unsigned int wdt_tclk; static void __iomem *wdt_reg; +static void __iomem *wdt_rstout; static DEFINE_SPINLOCK(wdt_lock); static int orion_wdt_ping(struct watchdog_device *wdt_dev) @@ -74,7 +75,7 @@ static int orion_wdt_start(struct watchdog_device *wdt_dev) atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, WDT_EN); /* Enable reset on watchdog */ - atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN); + atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, WDT_RESET_OUT_EN); spin_unlock(&wdt_lock); return 0; @@ -85,7 +86,7 @@ static int orion_wdt_stop(struct watchdog_device *wdt_dev) spin_lock(&wdt_lock); /* Disable reset on watchdog */ - atomic_io_modify(RSTOUTn_MASK, WDT_RESET_OUT_EN, 0); + atomic_io_modify(wdt_rstout, WDT_RESET_OUT_EN, 0); /* Disable watchdog timer */ atomic_io_modify(wdt_reg + TIMER_CTRL, WDT_EN, 0); @@ -158,6 +159,13 @@ static int orion_wdt_probe(struct platform_device *pdev) if (!wdt_reg) return -ENOMEM; + res = platform_get_resource(pdev, IORESOURCE_MEM, 1); + if (!res) + return -ENODEV; + wdt_rstout = devm_ioremap(&pdev->dev, res->start, resource_size(res)); + if (!wdt_rstout) + return -ENOMEM; + irq = platform_get_irq(pdev, 0); if (irq > 0) { /*
In order to support other SoC, it's required to distinguish the 'control' timer register, from the 'rstout' register that enables system reset on watchdog expiration. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- Documentation/devicetree/bindings/watchdog/marvel.txt | 6 ++++-- arch/arm/mach-dove/include/mach/bridge-regs.h | 1 + arch/arm/mach-kirkwood/include/mach/bridge-regs.h | 1 + arch/arm/mach-mv78xx0/include/mach/bridge-regs.h | 1 + arch/arm/mach-orion5x/include/mach/bridge-regs.h | 1 + arch/arm/plat-orion/common.c | 10 ++++++---- drivers/watchdog/orion_wdt.c | 12 ++++++++++-- 7 files changed, 24 insertions(+), 8 deletions(-)