| Message ID | 9c721b89ed2ceb6997809bb3363f852277e67dc2.1391177880.git.michal.simek@xilinx.com |
|---|---|
| State | Accepted, archived |
| Commit | a8b84b01e4af2baad3cef46bc86dcb2718c79e72 |
| Headers | show |
On Friday 31 January 2014, Michal Simek wrote: > +Optional properties: > +- clock-frequency : Frequency of clock in Hz > +- xlnx,wdt-enable-once : 0 - Watchdog can be restarted > + 1 - Watchdog can be enabled just once > +- xlnx,wdt-interval : Watchdog timeout interval in 2^<val> clock cycles, > + <val> is integer from 8 to 31. > + The latter two don't really seem to be xilinx specific, it would be reasonable to have a standard watchdog binding that mandates a common format for them. I'm not sure about the enable-once flag, which seems to just map to the "nowayout" watchdog option that is not a hardware feature at all and should probably be kept as a software setting only, rather than settable through DT. If it is kept, it should have a standard name and get turned into a boolean (present/absent) property rather than a 0/1 integer property. The interval should really be specified in terms of seconds or miliseconds, not in clock cycles. 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 02/03/2014 04:06 PM, Arnd Bergmann wrote: > On Friday 31 January 2014, Michal Simek wrote: >> +Optional properties: >> +- clock-frequency : Frequency of clock in Hz >> +- xlnx,wdt-enable-once : 0 - Watchdog can be restarted >> + 1 - Watchdog can be enabled just once >> +- xlnx,wdt-interval : Watchdog timeout interval in 2^<val> clock cycles, >> + <val> is integer from 8 to 31. >> + > > The latter two don't really seem to be xilinx specific, it would be > reasonable to have a standard watchdog binding that mandates a common > format for them. > > I'm not sure about the enable-once flag, which seems to just map to the > "nowayout" watchdog option that is not a hardware feature at all > and should probably be kept as a software setting only, rather than > settable through DT. If it is kept, it should have a standard name and > get turned into a boolean (present/absent) property rather than a > 0/1 integer property. > > The interval should really be specified in terms of seconds or miliseconds, > not in clock cycles. Intention wasn't to fix binding but document current one which is in mainline for a long time. Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout is seconds, and clock-frequency should go out and use CCF for getting clock. Thanks, Michal -- 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 Monday 03 February 2014 16:13:47 Michal Simek wrote: > Intention wasn't to fix binding but document current one > which is in mainline for a long time. Ok, I see. > Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout > is seconds, and clock-frequency should go out and use CCF for getting clock. Could we make a common binding then, and document that the xilinx watchdog can optionally provide either one? 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 02/03/2014 04:32 PM, Arnd Bergmann wrote: > On Monday 03 February 2014 16:13:47 Michal Simek wrote: >> Intention wasn't to fix binding but document current one >> which is in mainline for a long time. > > Ok, I see. > >> Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout >> is seconds, and clock-frequency should go out and use CCF for getting clock. > > Could we make a common binding then, and document that the xilinx > watchdog can optionally provide either one? Do you mean to have 2 DT bindings? This binding is used from 2011-07. It means it was generated for all hw designs at least from this time. I would say from DT usage on Microblaze because it is not special case in our dt generator. xlnx,XXX are XXX parameters which you have to setup in tools and get synthesized. This is valid for all xilinx IPs. We have full IP description by generating xlnx,XXX parameters directly from tools because we know all variants which can happen. Just back to your previous post: "I'm not sure about the enable-once flag, which seems to just map to the "nowayout" watchdog option that is not a hardware feature at all" this is hw feature which you can select in tools because this is fpga. :-) Thanks, Michal -- 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 Monday 03 February 2014, Michal Simek wrote: > On 02/03/2014 04:32 PM, Arnd Bergmann wrote: > > On Monday 03 February 2014 16:13:47 Michal Simek wrote: > >> Intention wasn't to fix binding but document current one > >> which is in mainline for a long time. > > > > Ok, I see. > > > >> Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout > >> is seconds, and clock-frequency should go out and use CCF for getting clock. > > > > Could we make a common binding then, and document that the xilinx > > watchdog can optionally provide either one? > > Do you mean to have 2 DT bindings? > > This binding is used from 2011-07. > It means it was generated for all hw designs at least from this time. > I would say from DT usage on Microblaze because it is not special case > in our dt generator. I certainly wasn't suggesting to break the binding, quite the contrary. What I tried to say is that the properties look like they should be useful for different kinds of watchdogs, not just xilinx, so it would be good to have a common definition using generic strings. The xilinx driver would definitely have to keep supporting the traditional property names, but it could also support the generic names in the future. > xlnx,XXX are XXX parameters which you have to setup in tools > and get synthesized. This is valid for all xilinx IPs. We have full > IP description by generating xlnx,XXX parameters directly from tools > because we know all variants which can happen. > > Just back to your previous post: > "I'm not sure about the enable-once flag, which seems to just map to the > "nowayout" watchdog option that is not a hardware feature at all" > this is hw feature which you can select in tools because this is fpga. :-) Ah, so you mean the properties are not settings that the driver programs into the hardware, but they are hardware properties that the driver reports to user space? 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 02/04/2014 08:27 PM, Arnd Bergmann wrote: > On Monday 03 February 2014, Michal Simek wrote: >> On 02/03/2014 04:32 PM, Arnd Bergmann wrote: >>> On Monday 03 February 2014 16:13:47 Michal Simek wrote: >>>> Intention wasn't to fix binding but document current one >>>> which is in mainline for a long time. >>> >>> Ok, I see. >>> >>>> Apart of this - yes, wdt-enable-once is nowayout and wdt-interval should be timeout >>>> is seconds, and clock-frequency should go out and use CCF for getting clock. >>> >>> Could we make a common binding then, and document that the xilinx >>> watchdog can optionally provide either one? >> >> Do you mean to have 2 DT bindings? >> >> This binding is used from 2011-07. >> It means it was generated for all hw designs at least from this time. >> I would say from DT usage on Microblaze because it is not special case >> in our dt generator. > > I certainly wasn't suggesting to break the binding, quite the contrary. > What I tried to say is that the properties look like they should be > useful for different kinds of watchdogs, not just xilinx, so it would > be good to have a common definition using generic strings. > > The xilinx driver would definitely have to keep supporting the traditional > property names, but it could also support the generic names in the > future. No problem with to do in future. >> xlnx,XXX are XXX parameters which you have to setup in tools >> and get synthesized. This is valid for all xilinx IPs. We have full >> IP description by generating xlnx,XXX parameters directly from tools >> because we know all variants which can happen. >> >> Just back to your previous post: >> "I'm not sure about the enable-once flag, which seems to just map to the >> "nowayout" watchdog option that is not a hardware feature at all" >> this is hw feature which you can select in tools because this is fpga. :-) > > Ah, so you mean the properties are not settings that the driver > programs into the hardware, but they are hardware properties that the > driver reports to user space? yes, they are hardware properties which you can choose based on your configuration. Every user just decides if this watchdog can be started just once and how long it takes in synthesis time. There is no option to program this by software. I am not quite sure what you mean by reports to user space. If you mean to get timeout through ioctl for example - then yes it is working through standard watchdog ioctl interface and timeout is calculated from hardware setup. Thanks, Michal
On Wednesday 05 February 2014, Michal Simek wrote: > I am not quite sure what you mean by reports to user space. > If you mean to get timeout through ioctl for example - then yes it is working > through standard watchdog ioctl interface and timeout is calculated > from hardware setup. Yes, that is what I meant. I believe most other watchdogs let you program the timeout, but I don't see anything wrong with having that fixed in the FPGA in your case. Still, the choice of putting the timeout into DT in terms of cycles rather than miliseconds wasn't ideal from an interface perspective and we should change that if/when we do a generic binding. I can definitely see where it's coming from for your case, as the cycle count totally makes sense from an FPGA tool perspective... 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 02/05/2014 10:36 AM, Arnd Bergmann wrote: > On Wednesday 05 February 2014, Michal Simek wrote: >> I am not quite sure what you mean by reports to user space. >> If you mean to get timeout through ioctl for example - then yes it is working >> through standard watchdog ioctl interface and timeout is calculated >> from hardware setup. > > Yes, that is what I meant. I believe most other watchdogs let > you program the timeout, but I don't see anything wrong with > having that fixed in the FPGA in your case. > > Still, the choice of putting the timeout into DT in terms of > cycles rather than miliseconds wasn't ideal from an interface > perspective and we should change that if/when we do a generic > binding. I can definitely see where it's coming from for your > case, as the cycle count totally makes sense from an FPGA > tool perspective... Thanks. I take this like ACK for this current binding description. Thanks, Michal
On Wednesday 05 February 2014, Michal Simek wrote: > On 02/05/2014 10:36 AM, Arnd Bergmann wrote: > > On Wednesday 05 February 2014, Michal Simek wrote: > > Still, the choice of putting the timeout into DT in terms of > > cycles rather than miliseconds wasn't ideal from an interface > > perspective and we should change that if/when we do a generic > > binding. I can definitely see where it's coming from for your > > case, as the cycle count totally makes sense from an FPGA > > tool perspective... > > Thanks. I take this like ACK for this current binding description. > Yes, please add my 'Acked-by'. 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/of-xilinx-wdt.txt b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt new file mode 100644 index 0000000..6d63782 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt @@ -0,0 +1,23 @@ +Xilinx AXI/PLB soft-core watchdog Device Tree Bindings +--------------------------------------------------------- + +Required properties: +- compatible : Should be "xlnx,xps-timebase-wdt-1.00.a" or + "xlnx,xps-timebase-wdt-1.01.a". +- reg : Physical base address and size + +Optional properties: +- clock-frequency : Frequency of clock in Hz +- xlnx,wdt-enable-once : 0 - Watchdog can be restarted + 1 - Watchdog can be enabled just once +- xlnx,wdt-interval : Watchdog timeout interval in 2^<val> clock cycles, + <val> is integer from 8 to 31. + +Example: +axi-timebase-wdt@40100000 { + clock-frequency = <50000000>; + compatible = "xlnx,xps-timebase-wdt-1.00.a"; + reg = <0x40100000 0x10000>; + xlnx,wdt-enable-once = <0x0>; + xlnx,wdt-interval = <0x1b>; +} ;
Document current driver binding. Signed-off-by: Michal Simek <michal.simek@xilinx.com> --- .../devicetree/bindings/watchdog/of-xilinx-wdt.txt | 23 ++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/of-xilinx-wdt.txt -- 1.8.2.3