| Message ID | 1422371490-44402-2-git-send-email-Zubair.Kakakhel@imgtec.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"; +};
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