Re: [PATCH 00/10] Orion Watchdog fixes
On Tue, Jul 16, 2013 at 11:04:36AM -0300, Ezequiel Garcia wrote: > On Tue, Jul 16, 2013 at 09:44:22AM -0400, Jason Cooper wrote: > > On Tue, Jul 16, 2013 at 09:14:33AM -0300, Ezequiel Garcia wrote: > > > On the other side, I'm much interested in knowing if you are OK with > > > breaking the watchdog DT compatibility. If you NACK this, then I'll > > > start preparing a different watchdog driver for 370/XP, since I don't > > > want to extend a driver that is a bit dirty. > > > > Apparently there is some agreement that the bindings are still in flux > > and that they need to be for a bit longer in order to hammer out > > problems such as this. > > > > Arnd and Olof both mentioned that something (a doc, and email?) is > > forthcoming about marking some bindings as stable. Whatever form that > > takes, this one wouldn't get the stable marking yet. ;-) > > > > Yup, that's my understanding as well. But on the other side, I don't > want to break possible users out there. I agree, but living with a sub-standard binding is worse imho. > So, just to check, you say it's early enough to safely do such change? Well, it's not *safe*. But most consumers on mainline kernels are currently developer/hackers/distro maintainers. eg, folks who can handle it. It's better to do it now. > In that case, I'll extend this patchset to include Armada 370/XP support > and post it as soon as Sebastian's clocksource stuff gets in. Cool. thx, Jason. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
On Tue, Jul 16, 2013 at 09:44:22AM -0400, Jason Cooper wrote: > On Tue, Jul 16, 2013 at 09:14:33AM -0300, Ezequiel Garcia wrote: > > On the other side, I'm much interested in knowing if you are OK with > > breaking the watchdog DT compatibility. If you NACK this, then I'll > > start preparing a different watchdog driver for 370/XP, since I don't > > want to extend a driver that is a bit dirty. > > Apparently there is some agreement that the bindings are still in flux > and that they need to be for a bit longer in order to hammer out > problems such as this. > > Arnd and Olof both mentioned that something (a doc, and email?) is > forthcoming about marking some bindings as stable. Whatever form that > takes, this one wouldn't get the stable marking yet. ;-) > Yup, that's my understanding as well. But on the other side, I don't want to break possible users out there. So, just to check, you say it's early enough to safely do such change? In that case, I'll extend this patchset to include Armada 370/XP support and post it as soon as Sebastian's clocksource stuff gets in. > Oh, and one more nit. The work 'fix' triggers a whole bunch of "get on > this right away, does it need to go to stable? Has anyone confirmed it? > Which commit caused the regression? etc." Although I hate the word, I > think 'refactoring' is much more appropriate description for this series. > Oh, good observation. I wrote the cover letter at 8 PM, after ten long hours (*) of hacking and smashing this into something easy to review, and that's the best title I could come up with. I'll change it on v2. Thanks, (*) yes, I have another pair of eyes, in case these wear out. -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
On Tue, Jul 16, 2013 at 09:14:33AM -0300, Ezequiel Garcia wrote: > On the other side, I'm much interested in knowing if you are OK with > breaking the watchdog DT compatibility. If you NACK this, then I'll > start preparing a different watchdog driver for 370/XP, since I don't > want to extend a driver that is a bit dirty. Apparently there is some agreement that the bindings are still in flux and that they need to be for a bit longer in order to hammer out problems such as this. Arnd and Olof both mentioned that something (a doc, and email?) is forthcoming about marking some bindings as stable. Whatever form that takes, this one wouldn't get the stable marking yet. ;-) Oh, and one more nit. The work 'fix' triggers a whole bunch of "get on this right away, does it need to go to stable? Has anyone confirmed it? Which commit caused the regression? etc." Although I hate the word, I think 'refactoring' is much more appropriate description for this series. thx, Jason. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
Hi Sebastian, On Tue, Jul 16, 2013 at 09:48:56AM +0200, Sebastian Hesselbarth wrote: > > In the discussion about orion clocksource Russell was proposing a generic > thread-safe write. That puts a single lock around all those writes. Of > course, it will also blocked by totally unrelated thread-safe register > accesses but should prevent us from having dozens of locks and solves the > symbol issue. > Thanks for the insight! I'll try to dig this suggestion in the clocksource discussion. Do you have any plans for posting the clocksource soon? This way I can base this series on something that's in Jason's trees... Thanks! -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
Hi Thomas, Andrew: Thanks for looking at this! On Tue, Jul 16, 2013 at 09:31:01AM +0200, Andrew Lunn wrote: > On Tue, Jul 16, 2013 at 09:20:59AM +0200, Thomas Petazzoni wrote: > > > > On Tue, 16 Jul 2013 08:59:52 +0200, Andrew Lunn wrote: > > > > > Maybe i'm missing something here. You are making use of > > > orion_timer_ctrl_clrset() from time-orion.c. How will this work on > > > 370/XP which has a different clocksource driver? > > > > I *think* the idea is that the Armada 370/XP driver will expose the > > same function, so from the point of view of the watchdog driver, it > > will just work. Indeed that was one of the ideas. As Thomas said, this was just preparation work. > > That was what i was thinking would happen. And then i started to > wonder how well the kernel linker deals with multiple definitions of > the same symbol. Dove and 370/XP can end up in the same kernel. So we > need to have both orion-timer and the 370/XP timer in the same kernel, > so we end up with the same symbol in the kernel twice... > Yeah, well... I wasn't sure about using the same name, so another approach would be adding a new compatible to the driver and then make it use the appropriate function in the 370/XP clocksource driver (with a different name). And, yet another approach, is what Sebastian just said, although I'm not sure I understood it :). In any case, we have already several solutions, which is why I'm not too worried about this particular issue. On the other side, I'm much interested in knowing if you are OK with breaking the watchdog DT compatibility. If you NACK this, then I'll start preparing a different watchdog driver for 370/XP, since I don't want to extend a driver that is a bit dirty. -- Ezequiel García, Free Electrons Embedded Linux, Kernel and Android Engineering http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
Andrew, Thomas, In the discussion about orion clocksource Russell was proposing a generic thread-safe write. That puts a single lock around all those writes. Of course, it will also blocked by totally unrelated thread-safe register accesses but should prevent us from having dozens of locks and solves the symbol issue. Sebastian On July 16, 2013 9:31:01 AM Andrew Lunn wrote: On Tue, Jul 16, 2013 at 09:20:59AM +0200, Thomas Petazzoni wrote: > Dear Andrew Lunn, > On Tue, 16 Jul 2013 08:59:52 +0200, Andrew Lunn wrote: > > Maybe i'm missing something here. You are making use of > > orion_timer_ctrl_clrset() from time-orion.c. How will this work on > > 370/XP which has a different clocksource driver? > I *think* the idea is that the Armada 370/XP driver will expose the > same function, so from the point of view of the watchdog driver, it > will just work. Hi Thomas That was what i was thinking would happen. And then i started to wonder how well the kernel linker deals with multiple definitions of the same symbol. Dove and 370/XP can end up in the same kernel. So we need to have both orion-timer and the 370/XP timer in the same kernel, so we end up with the same symbol in the kernel twice... Lets wait for Ezequiel to answer. Andrew ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
On Tue, Jul 16, 2013 at 09:20:59AM +0200, Thomas Petazzoni wrote: > Dear Andrew Lunn, > > On Tue, 16 Jul 2013 08:59:52 +0200, Andrew Lunn wrote: > > > Maybe i'm missing something here. You are making use of > > orion_timer_ctrl_clrset() from time-orion.c. How will this work on > > 370/XP which has a different clocksource driver? > > I *think* the idea is that the Armada 370/XP driver will expose the > same function, so from the point of view of the watchdog driver, it > will just work. Hi Thomas That was what i was thinking would happen. And then i started to wonder how well the kernel linker deals with multiple definitions of the same symbol. Dove and 370/XP can end up in the same kernel. So we need to have both orion-timer and the 370/XP timer in the same kernel, so we end up with the same symbol in the kernel twice... Lets wait for Ezequiel to answer. Andrew ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
Dear Andrew Lunn, On Tue, 16 Jul 2013 08:59:52 +0200, Andrew Lunn wrote: > Maybe i'm missing something here. You are making use of > orion_timer_ctrl_clrset() from time-orion.c. How will this work on > 370/XP which has a different clocksource driver? I *think* the idea is that the Armada 370/XP driver will expose the same function, so from the point of view of the watchdog driver, it will just work. This set of patches is just some preparation patches on the Orion watchdog driver, to later make it usable on Armada 370/XP. The patches enabling its usage on Armada 370/XP will follow. But Ezequiel will confirm all that, we had a discussion together about this yesterday, so I guess what I said should be the plan, but it's better if Ezequiel confirms, obviously :) Thanks! Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 00/10] Orion Watchdog fixes
On Mon, Jul 15, 2013 at 08:32:33PM -0300, Ezequiel Garcia wrote: > This patchset introduces a bunch of fixes that removes the direct use of > the shared timer control register, and also removes the need to include > a mach-specific header. > > With this changes the driver can be included in multiplatforms builds, > and in particular be used for the Armada 370/XP SoC. Hi Ezequiel Maybe i'm missing something here. You are making use of orion_timer_ctrl_clrset() from time-orion.c. How will this work on 370/XP which has a different clocksource driver? Thanks Andrew ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
[PATCH 00/10] Orion Watchdog fixes
This patchset introduces a bunch of fixes that removes the direct use of
the shared timer control register, and also removes the need to include
a mach-specific header.
With this changes the driver can be included in multiplatforms builds,
and in particular be used for the Armada 370/XP SoC.
I'm sending this early patchset mainly to discuss the possibility of
introducing this changes. As cleared stated in each patch, this series
breaks device-tree compatilibity by changing the meaning of the watchdog
node 'reg' property.
I'm aware of the implications of this, but I'm not sure how 'stable' the
Kirkwood DT is considered. So, given we're currently moving things around,
maybe there is still a chance to do this.
This patchset applies on top of Sebastian Hesselbarth's branch:
git://github.com/shesselba/linux-dove.git orion-irqchip-for-v3.11_v4
Ezequiel Garcia (10):
clocksource: orion: Add thread-safe API header
watchdog: orion: Use thread-safe clocksource API
watchdog: orion: Rename device-tree binding documentation
watchdog: orion: Use the proper watchdog register
watchdog: orion: Add a memory resource for RSTOUT register
watchdog: orion: Update device-tree binding documentation
watchdog: orion: Remove unneeded BRIDGE_CAUSE clear
watchdog: orion: Remove mach-specific unneeded header
watchdog: orion: Use BIT()
ARM: kirkwood: Fix the device-tree watchdog's node reg property
.../watchdog/{marvel.txt => orion-wdt.txt} | 8 ++--
arch/arm/boot/dts/kirkwood.dtsi| 3 +-
arch/arm/mach-kirkwood/include/mach/bridge-regs.h | 2 +
arch/arm/mach-orion5x/include/mach/bridge-regs.h | 2 +
arch/arm/plat-orion/common.c | 10 +++--
drivers/watchdog/orion_wdt.c | 46 +-
include/linux/time-orion.h | 7
7 files changed, 42 insertions(+), 36 deletions(-)
rename Documentation/devicetree/bindings/watchdog/{marvel.txt =>
orion-wdt.txt} (58%)
create mode 100644 include/linux/time-orion.h
--
1.8.1.5
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
