Re: [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

Re: [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Tomasz Figa]

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

Re: [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Doug Anderson]

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

Re: [PATCH 1/3] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses

[Olof Johansson]

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