Re: [PATCH v2 5/8] MIPS: jz4740: dts: Add bindings for the jz4740-wdt driver

[Paul Cercueil <paul@crapouillou.net>]

Le mer. 3 janv. 2018 à 5:46, Guenter Roeck <linux@roeck-us.net> a 
écrit :
> On 01/02/2018 08:48 AM, Paul Cercueil wrote:
>> Hi PrasannaKumar,
>> 
>> Le mar. 2 janv. 2018 à 17:37, PrasannaKumar Muralidharan 
>> <prasannatsmkumar@gmail.com> a écrit :
>>> Hi Paul,
>>> 
>>> On 30 December 2017 at 19:21, Paul Cercueil <paul@crapouillou.net> 
>>> wrote:
>>>>  Also remove the watchdog platform_device from platform.c, since it
>>>>  wasn't used anywhere anyway.
>>>> 
>>>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>>>  ---
>>>>   arch/mips/boot/dts/ingenic/jz4740.dtsi |  8 ++++++++
>>>>   arch/mips/jz4740/platform.c            | 16 ----------------
>>>>   2 files changed, 8 insertions(+), 16 deletions(-)
>>>> 
>>>>   v2: No change
>>>> 
>>>>  diff --git a/arch/mips/boot/dts/ingenic/jz4740.dtsi 
>>>> b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  index cd5185bb90ae..26c6b561d6f7 100644
>>>>  --- a/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  +++ b/arch/mips/boot/dts/ingenic/jz4740.dtsi
>>>>  @@ -45,6 +45,14 @@
>>>>                  #clock-cells = <1>;
>>>>          };
>>>> 
>>>>  +       watchdog: watchdog@10002000 {
>>>>  +               compatible = "ingenic,jz4740-watchdog";
>>>>  +               reg = <0x10002000 0x10>;
>>>>  +
>>>>  +               clocks = <&cgu JZ4740_CLK_RTC>;
>>>>  +               clock-names = "rtc";
>>>>  +       };
>>>>  +
>>> 
>>> The watchdog driver calls jz4740_timer_enable_watchdog and
>>> jz4740_timer_disable_watchdog which defined in
>>> arch/mips/jz4740/timer.c. It accesses registers iomapped by timer
>>> code. Declaring register size as 0x10 does not show the real 
>>> picture.
>>> Better use register size as 0x100 and let timer, wdt, pwm drivers to
>>> share them.
>> 
>> As you said, it accesses registers iomapped by timer code. So the 
>> watchdog
>> driver doesn't need to iomap them.
>> 
>>> Code from one of your branches
>>> (https://github.com/OpenDingux/linux/blob/for-upstream-clocksource/arch/mips/boot/dts/ingenic/jz4740.dtsi)
>>> does it. Can you prepare a patch series and send it?
>>> I have a patch set that moves timer code out of arch/mips/jz4740/ 
>>> and
>>> does a similar thing for watchdog and pwm. As your new timer driver 
>>> is
>>> better than the existing one I have not sent my patches yet. I would
>>> like to see it getting mainlined as it paves way for removing most 
>>> of
>>> code in arch/mips/jz4740.
>> 
>> The whole 'for-upstream-clocksource' branch is supposed to go 
>> upstream,
>> but I can't do it in one big patchset without having lots of 
>> breakages with
>> my other patchsets (jz4770 SoC support, and jz4740 watchdog updates)
>> currently under review. That also makes it simpler to upstream than 
>> having
>> one single patchset that touches 6 different frameworks (MIPS, irq, 
>> clocks,
>> clocksource, watchdog, PWM).
>> 
>> So I will submit it in two steps, first the irq/clocks/clocksource 
>> drivers
>> (this patchset) hopefully for 4.16, and then the 
>> platform/watchdog/PWM fixes
>> for 4.17.
>> 
> 
> I kind of lost it in this exchange, sorry. At this point I don't know 
> if something
> is wrong with the watchdog patches, and I have no clue what the 
> upstream path
> is supposed to be. My working assumption is that 1) something may be 
> wrong with
> the current version of the patches, and, 2), even if not, none of the 
> patches
> is expected to find its way upstream through the watchdog subsystem. 
> Plus, 3),
> even if some of the patches are supposed to go upstream through the 
> watchdog
> subsystem, that won't happen in 4.16, and the patches will be 
> resubmitted later
> when they are ready [and will hopefully marked clearly for submission 
> through
> the watchdog subsystem].
> 
> With that in mind, I'll mark the series for my reference as "not 
> applicable".
> If this is wrong please let me know.
> 
> Guenter

Sorry, my fault, PrasannaKumar mentionned my 'for-upstream-clocksource' 
branch requesting
me to submit it upstream, which I am doing in parallel of this one. I 
thought I was
answering him in the other patchset's thread, hence the confusion.

There is nothing wrong with these watchdog patches. Upstream path is 
through the MIPS tree.

Paul