[PATCH 0/2] watchdog: jz4740: Add DT support

[PATCH 0/2] watchdog: jz4740: Add DT support

[Zubair Lutfullah Kakakhel]

Hi,

Here are two simple patches that add DT support to the jz4740 watchdog driver.

Patches are based on 3.19-rc6. Quite disjoint and stay within jz4740
so should apply easily on other trees.

If you would like to have them rebased to a different tree, please tell.

Thank-you

ZubairLK

Zubair Lutfullah Kakakhel (2):
  dt: watchdog: Add DT binding documentation for jz47xx watchdog timer
  watchdog: jz4740: Add DT support

 .../devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt | 17 +++++++++++++++++
 drivers/watchdog/jz4740_wdt.c                           |  9 +++++++++
 2 files changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt

-- 
1.9.1

[PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Zubair Lutfullah Kakakhel]

Add binding for jz47xx watchdog timer. It is a simple watchdog timer.
Needs rtc clock and register addresses

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>

---
The jz4740 is platform only at the moment.

But DT support is being added

See http://patchwork.linux-mips.org/bundle/paulburton/ci20-v3.20/

jz47xx is used because jz4780 will also use this driver
---
 .../devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt

diff --git a/Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt b/Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt
new file mode 100644
index 0000000..bf4c404
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt
@@ -0,0 +1,17 @@
+Ingenic Watchdog Timer (WDT) Controller for JZ47XX
+
+Required properties:
+compatible: "ingenic,jz4740-watchdog"
+reg: Register address and length for watchdog registers
+clocks: phandle to rtcclk
+clock-names: must be "rtc"
+
+Example:
+
+watchdog: jz47xx-watchdog@0x10002000 {
+	compatible = "ingenic,jz4780-watchdog";
+	reg = <0x10002000 0x100>;
+
+	clocks = <&rtclk>;
+	clock-names = "rtc";
+};
-- 
1.9.1

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Guenter Roeck]

On Tue, Jan 27, 2015 at 03:11:29PM +0000, Zubair Lutfullah Kakakhel wrote:
> Add binding for jz47xx watchdog timer. It is a simple watchdog timer.
> Needs rtc clock and register addresses
> 
> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
> 
> ---
> The jz4740 is platform only at the moment.
> 
> But DT support is being added
> 
> See http://patchwork.linux-mips.org/bundle/paulburton/ci20-v3.20/
> 
> jz47xx is used because jz4780 will also use this driver

Not a valid argument. Please use jz4740, per convention.

xx implies anything from 00 to 99, which is not necessarily correct.
If support for other chips is added, that information can be mentioned
in the file itself, as it is done for many other drivers.

> ---
>  .../devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt b/Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt
> new file mode 100644
> index 0000000..bf4c404
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/ingenic,jz47xx-wdt.txt
> @@ -0,0 +1,17 @@
> +Ingenic Watchdog Timer (WDT) Controller for JZ47XX
> +
> +Required properties:
> +compatible: "ingenic,jz4740-watchdog"
> +reg: Register address and length for watchdog registers
> +clocks: phandle to rtcclk
> +clock-names: must be "rtc"
> +
> +Example:
> +
> +watchdog: jz47xx-watchdog@0x10002000 {
> +	compatible = "ingenic,jz4780-watchdog";

"ingenic,jz4780-watchdog" is not listed as possible value above,
nor is it listed in the patch itself, so you can not use it here.

> +	reg = <0x10002000 0x100>;
> +
> +	clocks = <&rtclk>;
> +	clock-names = "rtc";
> +};
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Arnd Bergmann]

On Tuesday 27 January 2015 15:11:29 Zubair Lutfullah Kakakhel wrote:
> +clocks: phandle to rtcclk
> +clock-names: must be "rtc"
> +
> +Example:
> +
> +watchdog: jz47xx-watchdog@0x10002000 {
> +       compatible = "ingenic,jz4780-watchdog";
> +       reg = <0x10002000 0x100>;
> +
> +       clocks = <&rtclk>;
> +       clock-names = "rtc";
> +};

Why is the name "rtc"? are you sure you are not confusing the output
name of the clock controller with the input of the watchdog device?

It's suspicious that both have similar names.

	Arnd

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Guenter Roeck]

On Tue, Jan 27, 2015 at 09:16:45PM +0100, Arnd Bergmann wrote:
> On Tuesday 27 January 2015 15:11:29 Zubair Lutfullah Kakakhel wrote:
> > +clocks: phandle to rtcclk
> > +clock-names: must be "rtc"
> > +
> > +Example:
> > +
> > +watchdog: jz47xx-watchdog@0x10002000 {
> > +       compatible = "ingenic,jz4780-watchdog";
> > +       reg = <0x10002000 0x100>;
> > +
> > +       clocks = <&rtclk>;
> > +       clock-names = "rtc";
> > +};
> 
> Why is the name "rtc"? are you sure you are not confusing the output
> name of the clock controller with the input of the watchdog device?
> 
Driver does this (today):

	 drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");

Isn't that the name to use ? Just wondering.

Thanks,
Guenter

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Arnd Bergmann]

On Tuesday 27 January 2015 12:52:29 Guenter Roeck wrote:
> Driver does this (today):
> 
>          drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> 
> Isn't that the name to use ? Just wondering.

Just because the driver uses it at the moment does not mean it's the name
that the IP block uses.

clk_get() has the unpleasant property of doing fuzzy matching
on the name that is passed. It first tries to use the string
as the name of the clock input of the device, but if that is
not there, it falls back to looking for a global clk with a con_id.

In DT, we only support the first kind, but if a driver currently
uses the second, you get the wrong name.

Looking at arch/mips/jz4740/clock.c now, this seems to be exactly
what is going on here: there is no clkdev_add call to associate
the device clocks, so it can only match a global clock entry. :(

	Arnd

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Guenter Roeck]

On Tue, Jan 27, 2015 at 10:29:49PM +0100, Arnd Bergmann wrote:
> On Tuesday 27 January 2015 12:52:29 Guenter Roeck wrote:
> > Driver does this (today):
> > 
> >          drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> > 
> > Isn't that the name to use ? Just wondering.
> 
> Just because the driver uses it at the moment does not mean it's the name
> that the IP block uses.
> 
> clk_get() has the unpleasant property of doing fuzzy matching
> on the name that is passed. It first tries to use the string
> as the name of the clock input of the device, but if that is
> not there, it falls back to looking for a global clk with a con_id.
> 
> In DT, we only support the first kind, but if a driver currently
> uses the second, you get the wrong name.
> 
> Looking at arch/mips/jz4740/clock.c now, this seems to be exactly
> what is going on here: there is no clkdev_add call to associate
> the device clocks, so it can only match a global clock entry. :(
> 
Me confused :-(.

Does that mean the driver needs to be fixed, that the DT property
needs to change (to what ?), or both ?

Thanks,
Guenter

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Arnd Bergmann]

On Tuesday 27 January 2015 14:19:09 Guenter Roeck wrote:
> On Tue, Jan 27, 2015 at 10:29:49PM +0100, Arnd Bergmann wrote:
> > On Tuesday 27 January 2015 12:52:29 Guenter Roeck wrote:
> > > Driver does this (today):
> > > 
> > >          drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> > > 
> > > Isn't that the name to use ? Just wondering.
> > 
> > Just because the driver uses it at the moment does not mean it's the name
> > that the IP block uses.
> > 
> > clk_get() has the unpleasant property of doing fuzzy matching
> > on the name that is passed. It first tries to use the string
> > as the name of the clock input of the device, but if that is
> > not there, it falls back to looking for a global clk with a con_id.
> > 
> > In DT, we only support the first kind, but if a driver currently
> > uses the second, you get the wrong name.
> > 
> > Looking at arch/mips/jz4740/clock.c now, this seems to be exactly
> > what is going on here: there is no clkdev_add call to associate
> > the device clocks, so it can only match a global clock entry. 
> > 
> Me confused :-(.
> 
> Does that mean the driver needs to be fixed, that the DT property
> needs to change (to what ?), or both ?

Both.

The jz47xx clock driver should register a clkdev lookup table with
proper clock input names for each clock that is referenced by a
device, and then the drivers can use the right names.

In a lot of cases, the best name for a clock is no name so you
just use an anonymous clock like

	clk = clk_get(dev, NULL);

but this still requires a clock lookup table.

	Arnd

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Arnd Bergmann]

On Tuesday 27 January 2015 23:36:36 Arnd Bergmann wrote:
> On Tuesday 27 January 2015 14:19:09 Guenter Roeck wrote:
> > On Tue, Jan 27, 2015 at 10:29:49PM +0100, Arnd Bergmann wrote:
> > > On Tuesday 27 January 2015 12:52:29 Guenter Roeck wrote:
> > > > Driver does this (today):
> > > > 
> > > >          drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
> > > > 
> > > > Isn't that the name to use ? Just wondering.
> > > 
> > > Just because the driver uses it at the moment does not mean it's the name
> > > that the IP block uses.
> > > 
> > > clk_get() has the unpleasant property of doing fuzzy matching
> > > on the name that is passed. It first tries to use the string
> > > as the name of the clock input of the device, but if that is
> > > not there, it falls back to looking for a global clk with a con_id.
> > > 
> > > In DT, we only support the first kind, but if a driver currently
> > > uses the second, you get the wrong name.
> > > 
> > > Looking at arch/mips/jz4740/clock.c now, this seems to be exactly
> > > what is going on here: there is no clkdev_add call to associate
> > > the device clocks, so it can only match a global clock entry. 
> > > 
> > Me confused :-(.
> > 
> > Does that mean the driver needs to be fixed, that the DT property
> > needs to change (to what ?), or both ?
> 
> Both.
> 
> The jz47xx clock driver should register a clkdev lookup table with
> proper clock input names for each clock that is referenced by a
> device, and then the drivers can use the right names.
> 
> In a lot of cases, the best name for a clock is no name so you
> just use an anonymous clock like
> 
>         clk = clk_get(dev, NULL);
> 
> but this still requires a clock lookup table.

To illustrate why the current approach is wrong, think of a driver
that handles a device that can be used on two different SoCs.

The name used by the clock provider is SoC specific, so the driver
would need to know which SoC it's being used on in order to pass
the right clock signal name. clkdev is meant to solve this by providing
a lookup between the device/clock-input pair and the actual clock.

Apparently jz4740 does not share any devices with another SoC at
the moment, so this has not been a problem, but if jz4780 has
a slightly different clock tree, it's already broken.

	Arnd

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Zubair Lutfullah Kakakhel]

On 27/01/15 22:42, Arnd Bergmann wrote:
> On Tuesday 27 January 2015 23:36:36 Arnd Bergmann wrote:
>> On Tuesday 27 January 2015 14:19:09 Guenter Roeck wrote:
>>> On Tue, Jan 27, 2015 at 10:29:49PM +0100, Arnd Bergmann wrote:
>>>> On Tuesday 27 January 2015 12:52:29 Guenter Roeck wrote:
>>>>> Driver does this (today):
>>>>>
>>>>>          drvdata->rtc_clk = clk_get(&pdev->dev, "rtc");
>>>>>
>>>>> Isn't that the name to use ? Just wondering.
>>>>
>>>> Just because the driver uses it at the moment does not mean it's the name
>>>> that the IP block uses.
>>>>
>>>> clk_get() has the unpleasant property of doing fuzzy matching
>>>> on the name that is passed. It first tries to use the string
>>>> as the name of the clock input of the device, but if that is
>>>> not there, it falls back to looking for a global clk with a con_id.
>>>>
>>>> In DT, we only support the first kind, but if a driver currently
>>>> uses the second, you get the wrong name.
>>>>
>>>> Looking at arch/mips/jz4740/clock.c now, this seems to be exactly
>>>> what is going on here: there is no clkdev_add call to associate
>>>> the device clocks, so it can only match a global clock entry. 
>>>>
>>> Me confused :-(.
>>>
>>> Does that mean the driver needs to be fixed, that the DT property
>>> needs to change (to what ?), or both ?
>>
>> Both.
>>
>> The jz47xx clock driver should register a clkdev lookup table with
>> proper clock input names for each clock that is referenced by a
>> device, and then the drivers can use the right names.
>>
>> In a lot of cases, the best name for a clock is no name so you
>> just use an anonymous clock like
>>
>>         clk = clk_get(dev, NULL);
>>
>> but this still requires a clock lookup table.
> 
> To illustrate why the current approach is wrong, think of a driver
> that handles a device that can be used on two different SoCs.
> 
> The name used by the clock provider is SoC specific, so the driver
> would need to know which SoC it's being used on in order to pass
> the right clock signal name. clkdev is meant to solve this by providing
> a lookup between the device/clock-input pair and the actual clock.
> 
> Apparently jz4740 does not share any devices with another SoC at
> the moment, so this has not been a problem, but if jz4780 has
> a slightly different clock tree, it's already broken.
> 
> 	Arnd
> 

There is on-going work to fix jz4740, add jz4780 and shake the entire clock tree as well.

Patch 14 onwards in this series
http://patchwork.linux-mips.org/bundle/paulburton/ci20-v3.20/

Instead of lumping things out in huge changesets, I intended to push out the minor patches that are disjoint.

That was the purpose of sending these two patches.

Current binding requires the clock-name to be "rtc". Hence the name at the moment.

Regards,
ZubairLK

Re: [PATCH 1/2] dt: watchdog: Add DT binding documentation for jz47xx watchdog timer

[Arnd Bergmann]

On Wednesday 28 January 2015 10:27:00 Zubair Lutfullah Kakakhel wrote:
> 
> There is on-going work to fix jz4740, add jz4780 and shake the entire clock tree as well.
> 
> Patch 14 onwards in this series
> http://patchwork.linux-mips.org/bundle/paulburton/ci20-v3.20/

Right, that looks good.
 
> Instead of lumping things out in huge changesets, I intended to push out the minor patches that are disjoint.
> 
> That was the purpose of sending these two patches.
> 
> Current binding requires the clock-name to be "rtc". Hence the name at the moment.

The problem with the binding is that once you've established it, it
is very hard to change. One possible solution would be to try
'of_clk_get(dev->of_node, 0)' in the driver first, and only then
call clk_get(dev, "rtc"). Doing that, you can have an anonymous clock
in the DT without waiting for the clock driver to get merged first
or breaking the existing setup.

	Arnd

[PATCH 2/2] watchdog: jz4740: Add DT support

[Zubair Lutfullah Kakakhel]

Add DT support to the jz4740 driver. Simple of_match_ptr. No other
modification for probe needed

Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>
---
 drivers/watchdog/jz4740_wdt.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
index 18e41af..4420ec3 100644
--- a/drivers/watchdog/jz4740_wdt.c
+++ b/drivers/watchdog/jz4740_wdt.c
@@ -24,6 +24,7 @@
 #include <linux/clk.h>
 #include <linux/slab.h>
 #include <linux/err.h>
+#include <linux/of.h>
 
 #include <asm/mach-jz4740/timer.h>
 
@@ -142,6 +143,13 @@ static const struct watchdog_ops jz4740_wdt_ops = {
 	.set_timeout = jz4740_wdt_set_timeout,
 };
 
+#ifdef CONFIG_OF
+static const struct of_device_id jz4740_of_matches[] = {
+	{ .compatible = "ingenic,jz4740-watchdog", },
+	{ /* sentinel */ }
+};
+#endif
+
 static int jz4740_wdt_probe(struct platform_device *pdev)
 {
 	struct jz4740_wdt_drvdata *drvdata;
@@ -211,6 +219,7 @@ static struct platform_driver jz4740_wdt_driver = {
 	.remove = jz4740_wdt_remove,
 	.driver = {
 		.name = "jz4740-wdt",
+		.of_match_table = of_match_ptr(jz4740_of_matches),
 	},
 };
 
-- 
1.9.1

Re: [PATCH 2/2] watchdog: jz4740: Add DT support

[Guenter Roeck]

On Tue, Jan 27, 2015 at 03:11:30PM +0000, Zubair Lutfullah Kakakhel wrote:
> Add DT support to the jz4740 driver. Simple of_match_ptr. No other
> modification for probe needed
> 
> Signed-off-by: Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com>

Reviewed-by: Guenetr Roeck <linux@roeck-us.net>

Re: [PATCH 2/2] watchdog: jz4740: Add DT support

[Arnd Bergmann]

On Tuesday 27 January 2015 15:11:30 Zubair Lutfullah Kakakhel wrote:
>  };
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id jz4740_of_matches[] = {
> +       { .compatible = "ingenic,jz4740-watchdog", },
> +       { /* sentinel */ }
> +};
> +#endif
> +
>  static int jz4740_wdt_probe(struct platform_device *pdev)
>  {
>         struct jz4740_wdt_drvdata *drvdata;
> @@ -211,6 +219,7 @@ static struct platform_driver jz4740_wdt_driver = {
>         .remove = jz4740_wdt_remove,
>         .driver = {
>                 .name = "jz4740-wdt",
> +               .of_match_table = of_match_ptr(jz4740_of_matches),
>         },
> 

Why the #ifdef? Presumably we want to move over jz4740 to DT-only in the
long run, so you could just remove the #ifdef and the of_match_ptr()
wrapper.

Also, you should add a MODULE_DEVICE_TABLE() entry.

	Arnd

Re: [PATCH 2/2] watchdog: jz4740: Add DT support

[Guenter Roeck]

On Tue, Jan 27, 2015 at 09:15:37PM +0100, Arnd Bergmann wrote:
> On Tuesday 27 January 2015 15:11:30 Zubair Lutfullah Kakakhel wrote:
> >  };
> >  
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id jz4740_of_matches[] = {
> > +       { .compatible = "ingenic,jz4740-watchdog", },
> > +       { /* sentinel */ }
> > +};
> > +#endif
> > +
> >  static int jz4740_wdt_probe(struct platform_device *pdev)
> >  {
> >         struct jz4740_wdt_drvdata *drvdata;
> > @@ -211,6 +219,7 @@ static struct platform_driver jz4740_wdt_driver = {
> >         .remove = jz4740_wdt_remove,
> >         .driver = {
> >                 .name = "jz4740-wdt",
> > +               .of_match_table = of_match_ptr(jz4740_of_matches),
> >         },
> > 
> 
> Why the #ifdef? Presumably we want to move over jz4740 to DT-only in the
> long run, so you could just remove the #ifdef and the of_match_ptr()
> wrapper.
> 
Only if "depends on OF" is added to its Kconfig entry, or am I missing
something ?

Guenter

Re: [PATCH 2/2] watchdog: jz4740: Add DT support

[Arnd Bergmann]

On Tuesday 27 January 2015 12:54:30 Guenter Roeck wrote:
> On Tue, Jan 27, 2015 at 09:15:37PM +0100, Arnd Bergmann wrote:
> > On Tuesday 27 January 2015 15:11:30 Zubair Lutfullah Kakakhel wrote:
> > >  };
> > >  
> > > +#ifdef CONFIG_OF
> > > +static const struct of_device_id jz4740_of_matches[] = {
> > > +       { .compatible = "ingenic,jz4740-watchdog", },
> > > +       { /* sentinel */ }
> > > +};
> > > +#endif
> > > +
> > >  static int jz4740_wdt_probe(struct platform_device *pdev)
> > >  {
> > >         struct jz4740_wdt_drvdata *drvdata;
> > > @@ -211,6 +219,7 @@ static struct platform_driver jz4740_wdt_driver = {
> > >         .remove = jz4740_wdt_remove,
> > >         .driver = {
> > >                 .name = "jz4740-wdt",
> > > +               .of_match_table = of_match_ptr(jz4740_of_matches),
> > >         },
> > > 
> > 
> > Why the #ifdef? Presumably we want to move over jz4740 to DT-only in the
> > long run, so you could just remove the #ifdef and the of_match_ptr()
> > wrapper.
> > 
> Only if "depends on OF" is added to its Kconfig entry, or am I missing
> something ?
> 

I'm pretty sure you don't need that, it will still compile and link
fine, the only downside being a few wasted bytes to store the compatible
string and the array.

	Arnd