* Re: [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses [not found] ` <1374658699-3961-2-git-send-email-l.krishna@samsung.com> @ 2013-07-25 10:27 ` Tomasz Figa 2013-07-26 0:32 ` Doug Anderson 0 siblings, 1 reply; 3+ messages in thread From: Tomasz Figa @ 2013-07-25 10:27 UTC (permalink / raw) To: Leela Krishna Amudala Cc: linux-samsung-soc, kgene.kim, dianders, jg1.han, sachin.kamat, 'Wim Van Sebroeck', devicetree Hi Leela, On Wednesday 24 of July 2013 15:08:17 Leela Krishna Amudala wrote: > This patch parses the watchdog node to read pmu wdt sys registers > addresses and do mask/unmask enable/disable of WDT in probe and s2r > scenarios. > > Reviewed-by: Doug Anderson <dianders@chromium.org> > Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> > --- > .../devicetree/bindings/watchdog/samsung-wdt.txt | 14 ++++- > drivers/watchdog/s3c2410_wdt.c | 56 > ++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt index > 2aa486c..4c798e3 100644 > --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > @@ -7,8 +7,20 @@ occurred. > Required properties: > - compatible : should be "samsung,s3c2410-wdt" > - reg : base physical address of the controller and length of memory > mapped - region. > + region and the optional (addresses and length of memory mapped regions > + of) PMU registers for masking/unmasking WDT. > - interrupts : interrupt number to the cpu. I don't think PMU registers should be passed like this, even if USB PHY driver currently does it - it needs to be fixed. Every time you are mapping a single 4-byte register, you are creating new mapping for the whole page (4K) containing it. In addition, this makes the WDT driver map IO memory that does not belong to the IP block it handles, which is not nice. I would suggest looking into regmap helpers to implement a driver for the PMU that can share PMU registers with interested drivers. > Optional properties: > - timeout-sec : contains the watchdog timeout in seconds. > +- reset-mask-bit: bit number in the PMU registers to program mask/unmask > WDT. + > +Example: If WDT of Exynos 5420 needs special masking, it is no longer compatible with "samsung,s3c2410-wdt". You need to create separate compatible for it ("samsung,exynos5420-wdt") and do this special masking only for devices using it. > +watchdog { > + compatible = "samsung,s3c2410-wdt"; > + reg = <0x101D0000 0x100>, <0x10040408 0x4>, <0x1004040c 0x4>; > + interrupts = <0 42 0>; > + status = "disabled"; > + reset-mask-bit = <0>; > +}; By the way, you need to Cc devicetree@vger.kernel.org mailing if you modify device tree bindings. Best regards, Tomasz ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses 2013-07-25 10:27 ` [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses Tomasz Figa @ 2013-07-26 0:32 ` Doug Anderson 2013-08-10 21:32 ` Olof Johansson 0 siblings, 1 reply; 3+ messages in thread From: Doug Anderson @ 2013-07-26 0:32 UTC (permalink / raw) To: Tomasz Figa Cc: Leela Krishna Amudala, linux-samsung-soc, Kukjin Kim, Jingoo Han, Sachin Kamat, Wim Van Sebroeck, devicetree, Olof Johansson Tomasz, On Thu, Jul 25, 2013 at 3:27 AM, Tomasz Figa <t.figa@samsung.com> wrote: > Hi Leela, > > On Wednesday 24 of July 2013 15:08:17 Leela Krishna Amudala wrote: >> This patch parses the watchdog node to read pmu wdt sys registers >> addresses and do mask/unmask enable/disable of WDT in probe and s2r >> scenarios. >> >> Reviewed-by: Doug Anderson <dianders@chromium.org> >> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> >> --- >> .../devicetree/bindings/watchdog/samsung-wdt.txt | 14 ++++- >> drivers/watchdog/s3c2410_wdt.c | 56 >> ++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt index >> 2aa486c..4c798e3 100644 >> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >> @@ -7,8 +7,20 @@ occurred. >> Required properties: >> - compatible : should be "samsung,s3c2410-wdt" >> - reg : base physical address of the controller and length of memory >> mapped - region. >> + region and the optional (addresses and length of memory mapped regions >> + of) PMU registers for masking/unmasking WDT. >> - interrupts : interrupt number to the cpu. > > I don't think PMU registers should be passed like this, even if USB PHY > driver currently does it - it needs to be fixed. Every time you are mapping > a single 4-byte register, you are creating new mapping for the whole page > (4K) containing it. In addition, this makes the WDT driver map IO memory > that does not belong to the IP block it handles, which is not nice. > > I would suggest looking into regmap helpers to implement a driver for the > PMU that can share PMU registers with interested drivers. We've done this in some other drivers as well. I remember someone suggested that this was fine when I posted the change to the ADC driver for something similar. Do you really think it's worth adding a level of abstraction here? The argument I remember in the past was that it was fine as long as the register itself was used only by this driver. If we want to do as you say, we'll have to figure out whether you want to create a new generic interface for this or reuse an existing one. You could kinda think of these bits as enabling power, so you could use the regulator framework. ...but that framework often gets abused. Olof: I think I got your advice when I did the ADC work. Do you have any advice here? Thanks! -Doug ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses 2013-07-26 0:32 ` Doug Anderson @ 2013-08-10 21:32 ` Olof Johansson 0 siblings, 0 replies; 3+ messages in thread From: Olof Johansson @ 2013-08-10 21:32 UTC (permalink / raw) To: Doug Anderson Cc: Tomasz Figa, Leela Krishna Amudala, linux-samsung-soc, Kukjin Kim, Jingoo Han, Sachin Kamat, Wim Van Sebroeck, devicetree@vger.kernel.org On Thu, Jul 25, 2013 at 5:32 PM, Doug Anderson <dianders@chromium.org> wrote: > Tomasz, > > On Thu, Jul 25, 2013 at 3:27 AM, Tomasz Figa <t.figa@samsung.com> wrote: >> Hi Leela, >> >> On Wednesday 24 of July 2013 15:08:17 Leela Krishna Amudala wrote: >>> This patch parses the watchdog node to read pmu wdt sys registers >>> addresses and do mask/unmask enable/disable of WDT in probe and s2r >>> scenarios. >>> >>> Reviewed-by: Doug Anderson <dianders@chromium.org> >>> Signed-off-by: Leela Krishna Amudala <l.krishna@samsung.com> >>> --- >>> .../devicetree/bindings/watchdog/samsung-wdt.txt | 14 ++++- >>> drivers/watchdog/s3c2410_wdt.c | 56 >>> ++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) >>> >>> diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt index >>> 2aa486c..4c798e3 100644 >>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt >>> @@ -7,8 +7,20 @@ occurred. >>> Required properties: >>> - compatible : should be "samsung,s3c2410-wdt" >>> - reg : base physical address of the controller and length of memory >>> mapped - region. >>> + region and the optional (addresses and length of memory mapped regions >>> + of) PMU registers for masking/unmasking WDT. >>> - interrupts : interrupt number to the cpu. >> >> I don't think PMU registers should be passed like this, even if USB PHY >> driver currently does it - it needs to be fixed. Every time you are mapping >> a single 4-byte register, you are creating new mapping for the whole page >> (4K) containing it. In addition, this makes the WDT driver map IO memory >> that does not belong to the IP block it handles, which is not nice. >> >> I would suggest looking into regmap helpers to implement a driver for the >> PMU that can share PMU registers with interested drivers. > > We've done this in some other drivers as well. I remember someone > suggested that this was fine when I posted the change to the ADC > driver for something similar. > > Do you really think it's worth adding a level of abstraction here? > The argument I remember in the past was that it was fine as long as > the register itself was used only by this driver. > > If we want to do as you say, we'll have to figure out whether you want > to create a new generic interface for this or reuse an existing one. > You could kinda think of these bits as enabling power, so you could > use the regulator framework. ...but that framework often gets abused. > > Olof: I think I got your advice when I did the ADC work. Do you have > any advice here? These tradeoffs are always tricky since it's hard to decide what corners are OK to cut a bit off of, and when you have to be a stickler for really following hard strict rules and stick to abstraction models. I think the more reasonable approach is the one you've taken here, Doug. I think we risk over-abstracting this otherwise, to little actual gain. The register is not shared so there's no conflict in that area. The argument that it wastes a 4K page isn't a big deal either, since we don't do this everywhere all the time. I'd rather have the register described in the reg range like this, than having the driver go out and find the PMU node and find a magic offset into that, with or without an abstraction layer for the PMU register access. -Olof ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-08-10 21:32 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1374658699-3961-1-git-send-email-l.krishna@samsung.com>
[not found] ` <1374658699-3961-2-git-send-email-l.krishna@samsung.com>
2013-07-25 10:27 ` [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses Tomasz Figa
2013-07-26 0:32 ` Doug Anderson
2013-08-10 21:32 ` Olof Johansson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).