[linux,v1,1/2] Documentation: dt-bindings: Document bindings for ASPEED AST2400/AST2500 pwm and fan tach controller device driver
diff mbox

Message ID 20170109215935.30067-2-jaghu@google.com
State Awaiting Upstream
Headers show

Commit Message

Jaghathiswari Rankappagounder Natarajan Jan. 9, 2017, 9:59 p.m. UTC
This binding provides interface for adding values related to ASPEED
AST2400/2500 PWM and Fan tach controller support.
The PWM controller can support upto 8 PWM output ports.
The Fan tach controller can support upto 16 tachometer inputs.
PWM clock types M, N and 0 are three types just to have three independent
PWM sources.

Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
---
 .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 153 +++++++++++++++++++++
 1 file changed, 153 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt

--
2.11.0.390.gc69c2f50cf-goog

Comments

Rob Herring Jan. 13, 2017, 4:21 p.m. UTC | #1
On Mon, Jan 09, 2017 at 01:59:34PM -0800, Jaghathiswari Rankappagounder Natarajan wrote:
> This binding provides interface for adding values related to ASPEED
> AST2400/2500 PWM and Fan tach controller support.
> The PWM controller can support upto 8 PWM output ports.
> The Fan tach controller can support upto 16 tachometer inputs.
> PWM clock types M, N and 0 are three types just to have three independent
> PWM sources.
> 
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com>
> ---
>  .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 153 +++++++++++++++++++++

Perhaps bindings/pwm/... even though this is more than just PWM.

>  1 file changed, 153 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> new file mode 100644
> index 000000000000..8f346409ee8c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> @@ -0,0 +1,153 @@
> +ASPEED AST2400/AST2500 PWM and Fan Tacho controller device driver
> +
> +The ASPEED PWM controller can support upto 8 PWM outputs. The ASPEED Fan Tacho
> +controller can support upto 16 tachometer inputs. The PWM controller supports
> +3 types of frequency mode PWM for fan speed control. PWM clock types M, N and 0
> +are 3 types of frequency mode PWM just to have 3 independent PWM sources.
> +
> +Required properties for pwm_tacho node:
> +- #address-cells : should be 1.
> +
> +- #size-cells : should be 1.
> +
> +- reg : address and length of the register set for the device.
> +
> +- pinctrl-names : a pinctrl state named "default" must be defined.
> +
> +- pinctrl-0 : phandle referencing pin configuration of the AST2400/AST2500 PWM
> +	      ports.
> +
> +- compatible : should be "aspeed,aspeed2400-pwm-tacho" for AST2400 or
> +	       "aspeed,aspeed2500-pwm-tacho" for AST2500.
> +
> +- clocks : a fixed clock providing input clock frequency(PWM
> +	   and Fan Tach clock)
> +
> +type_values subnode format:

Don't use '_' in node or property names.

> +===========================
> +Under type_values subnode there can be upto 3 child nodes indicating type M/N/O
> +values. Atleast one child node is required.
> +
> +Required properties for the child node(type M/N/O):
> +- pwm_period : indicates type M/N/O PWM period, as per the AST2400/AST2500
> +	       datasheet. integer value in the range 0 to 255.


> +
> +- pwm_clock_division_l : indicates type M/N/O PWM clock division L value,
> +			 as per the AST2400/AST2500 datasheet.
> +			 integer value in the range 0 to 15.
> +			 0 here indicates divide 1, 1 indicates divide 2,
> +			 2 indicates divide 4, 3 indicates divide 6, and so on
> +			 till 15 indicates divide 30.
> +
> +- pwm_clock_division_h : indicates type M/N/O PWM clock division H value,
> +			 as per the AST2400/AST2500 datasheet.
> +			 integer value in the range 0 to 15.
> +			 0 here indicates divide 1, 1 indicates divide 2,
> +			 2 indicates divide 4, 3 indicates divide 8, and so on
> +			 till 15 indicates divide 32768.

Can't you have a single divider value and driver convert to register 
values? Really, you should specify the PWM period in ns/us and calculate 
the divider based on the input clock freq. (i.e. use the clock binding). 
There's already PWM binding to specify the period.

I think you should have a node for the fan using the PWM binding and 
perhaps moving some of these properties to the fan node.

> +
> +- fan_tach_enable : indicates fan tach enable of type M/N/O as per the
> +		    AST2400/AST2500 datasheet. boolean value.
> +
> +- fan_tach_clock_division : indicates fan tach clock division as per the
> +			    AST2400/AST2500 datasheet.
> +			    integer value in the range 0 to 7.
> +			    0 indicates divide 4, 1 indicates divide 16,
> +			    2 indicates divide 64, 3 indicates divide 256
> +			    and so on till 7 indicates divide 65536.
> +
> +- fan_tach_mode_selection : indicates fan tach mode mode selection as per the
> +			    AST2400/AST2500 datasheet. integer value in the
> +			    range 0 to 2. 0 indicates falling edge, 1 indicates
> +			    rising edge and 2 indicates both edges.
> +
> +- fan_tach_period : indicates fan tach period as per the AST2400/AST2500
> +		    datasheet. integer value (can be upto 16 bits long).
> +
> +pwm_port subnode format:
> +========================
> +Under pwm_port subnode there can upto 8 child nodes each indicating values
> +for one of the 8 PWM output ports.
> +
> +Required properties for each child node(starting from PWM A through PWM H):
> +- enable : enable PWM #X port, X ranges from A through H. boolean value.

A connection in the PWM binding would imply this.

> +
> +- type : indicates type selection value of PWM #X port, X ranges from A
> +	 through H. integer value in the range 0 to 2;
> +	 0 indicates type M, 1 indicates type N, 2 indicates type O.

This is meaningless to me.

> +- fan_ctrl : set the PWM fan control initial value. integer value between
> +	     0(off) and 255(full speed).
> +
> +fan_tach_channel subnode format:
> +================================
> +Under fan_tach_channel subnode there can be upto 16 child nodes each indicating
> +values for one of the 16 fan tach channels.
> +
> +Required properties for each child node(starting from fan tach #0 through
> +fan tach #16):
> +- fan-ctrl-gpios : should specify the tachometer input pin on the hardware.

I don't understand the connection here. You have a tach block with 16 
inputs on the SoC, so why the GPIOs? If the GPIOs go to the fan, then a 
fan node should have the GPIO properties.

> +
> +- enable : enable fan tach #X, X ranges from 0 through 16. boolean value.

'status' property is the standard way to enable a node or not. Not sure 
if that makes sense here though.

> +
> +- pwm_source : indicates PWM source of fan tach #X, X ranges from 0 through 16.
> +	       integer value in the range 0 to 7. 0 indicates PWM port A,
> +	       1 indicates PWM port B and so on till 7 indicates PWM port H.
Jaghathiswari Rankappagounder Natarajan Jan. 27, 2017, 9:32 a.m. UTC | #2
On Fri, Jan 13, 2017 at 8:21 AM, Rob Herring <robh@kernel.org> wrote:

> On Mon, Jan 09, 2017 at 01:59:34PM -0800, Jaghathiswari Rankappagounder
> Natarajan wrote:
> > This binding provides interface for adding values related to ASPEED
> > AST2400/2500 PWM and Fan tach controller support.
> > The PWM controller can support upto 8 PWM output ports.
> > The Fan tach controller can support upto 16 tachometer inputs.
> > PWM clock types M, N and 0 are three types just to have three independent
> > PWM sources.
> >
> > Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu@google.com
> >
> > ---
> >  .../devicetree/bindings/hwmon/aspeed-pwm-tacho.txt | 153
> +++++++++++++++++++++
>
> Perhaps bindings/pwm/... even though this is more than just PWM.
>
The corresponding hwmon driver is present in /drivers/hwmon/. So would it
make more

sense to include the devicetree document for that driver in
/devicetree/bindings/hwmon/.

In future if there any more hwmon related functionalities are added to this
driver then /devicetree/bindings/hwmon/ would be more relevant.


>
> >  1 file changed, 153 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed-pwm-
> tacho.txt
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> > new file mode 100644
> > index 000000000000..8f346409ee8c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
> > @@ -0,0 +1,153 @@
> > +ASPEED AST2400/AST2500 PWM and Fan Tacho controller device driver
> > +
> > +The ASPEED PWM controller can support upto 8 PWM outputs. The ASPEED
> Fan Tacho
> > +controller can support upto 16 tachometer inputs. The PWM controller
> supports
> > +3 types of frequency mode PWM for fan speed control. PWM clock types M,
> N and 0
> > +are 3 types of frequency mode PWM just to have 3 independent PWM
> sources.
> > +
> > +Required properties for pwm_tacho node:
> > +- #address-cells : should be 1.
> > +
> > +- #size-cells : should be 1.
> > +
> > +- reg : address and length of the register set for the device.
> > +
> > +- pinctrl-names : a pinctrl state named "default" must be defined.
> > +
> > +- pinctrl-0 : phandle referencing pin configuration of the
> AST2400/AST2500 PWM
> > +           ports.
> > +
> > +- compatible : should be "aspeed,aspeed2400-pwm-tacho" for AST2400 or
> > +            "aspeed,aspeed2500-pwm-tacho" for AST2500.
> > +
> > +- clocks : a fixed clock providing input clock frequency(PWM
> > +        and Fan Tach clock)
> > +
> > +type_values subnode format:
>
> Don't use '_' in node or property names.
>
Done.

>
> > +===========================
> > +Under type_values subnode there can be upto 3 child nodes indicating
> type M/N/O
> > +values. Atleast one child node is required.
> > +
> > +Required properties for the child node(type M/N/O):
> > +- pwm_period : indicates type M/N/O PWM period, as per the
> AST2400/AST2500
> > +            datasheet. integer value in the range 0 to 255.
>
>
> > +
> > +- pwm_clock_division_l : indicates type M/N/O PWM clock division L
> value,
> > +                      as per the AST2400/AST2500 datasheet.
> > +                      integer value in the range 0 to 15.
> > +                      0 here indicates divide 1, 1 indicates divide 2,
> > +                      2 indicates divide 4, 3 indicates divide 6, and
> so on
> > +                      till 15 indicates divide 30.
> > +
> > +- pwm_clock_division_h : indicates type M/N/O PWM clock division H
> value,
> > +                      as per the AST2400/AST2500 datasheet.
> > +                      integer value in the range 0 to 15.
> > +                      0 here indicates divide 1, 1 indicates divide 2,
> > +                      2 indicates divide 4, 3 indicates divide 8, and
> so on
> > +                      till 15 indicates divide 32768.
>
> Can't you have a single divider value and driver convert to register
> values? Really, you should specify the PWM period in ns/us and calculate
> the divider based on the input clock freq. (i.e. use the clock binding).
> There's already PWM binding to specify the period.
>
> I think you should have a node for the fan using the PWM binding and
> perhaps moving some of these properties to the fan node.
>

There are three different types M, N, O. These three types are just to have


different PWM/Fan Tach settings. Each type can have the following settings:

PWM related:

              1) PWM period


              2) PWM clock division high


              3) PWM clock division low


Fan Tach related:

              1) Fan Tach type enable


              2) Fan Tach clock division


              3) Fan Tach mode selection


              4) Fan Tach period




Each PWM port should be assigned a type which can be type M, N or O.




Each Fan Tach channel should be assigned a PWM source, which can be PWM
port A
through H. The Fan Tach channel implicitly gets the type (M, N or O), from
the type assigned to its PWM source.

If the clock type is type M then :
The PWM frequency = 24MHz / (type M clock division L value * type M
clock division H value *

(type M PWM period value + 1))

Because of this formula, I thought I could use properties to specify
type clock division High and Low and

 type period values so that the desired PWM frequency be achieved.


> > +
> > +- fan_tach_enable : indicates fan tach enable of type M/N/O as per the
> > +                 AST2400/AST2500 datasheet. boolean value.
> > +
> > +- fan_tach_clock_division : indicates fan tach clock division as per the
> > +                         AST2400/AST2500 datasheet.
> > +                         integer value in the range 0 to 7.
> > +                         0 indicates divide 4, 1 indicates divide 16,
> > +                         2 indicates divide 64, 3 indicates divide 256
> > +                         and so on till 7 indicates divide 65536.
> > +
> > +- fan_tach_mode_selection : indicates fan tach mode mode selection as
> per the
> > +                         AST2400/AST2500 datasheet. integer value in the
> > +                         range 0 to 2. 0 indicates falling edge, 1
> indicates
> > +                         rising edge and 2 indicates both edges.
> > +
> > +- fan_tach_period : indicates fan tach period as per the AST2400/AST2500
> > +                 datasheet. integer value (can be upto 16 bits long).
> > +
> > +pwm_port subnode format:
> > +========================
> > +Under pwm_port subnode there can upto 8 child nodes each indicating
> values
> > +for one of the 8 PWM output ports.
> > +
> > +Required properties for each child node(starting from PWM A through PWM
> H):
> > +- enable : enable PWM #X port, X ranges from A through H. boolean value.
>
> A connection in the PWM binding would imply this.
>
Had to change this property to indicate PWM port number.


>
> > +
> > +- type : indicates type selection value of PWM #X port, X ranges from A
> > +      through H. integer value in the range 0 to 2;
> > +      0 indicates type M, 1 indicates type N, 2 indicates type O.
>
> This is meaningless to me.
>

Each PWM port should be assigned a type which can be type M, N or O.



>
> > +- fan_ctrl : set the PWM fan control initial value. integer value
> between
> > +          0(off) and 255(full speed).
> > +
> > +fan_tach_channel subnode format:
> > +================================
> > +Under fan_tach_channel subnode there can be upto 16 child nodes each
> indicating
> > +values for one of the 16 fan tach channels.
> > +
> > +Required properties for each child node(starting from fan tach #0
> through
> > +fan tach #16):
> > +- fan-ctrl-gpios : should specify the tachometer input pin on the
> hardware.
>
> I don't understand the connection here. You have a tach block with 16
> inputs on the SoC, so why the GPIOs? If the GPIOs go to the fan, then a
> fan node should have the GPIO properties.
>
This property specifies the tachometer input GPIO pin on the hardware.
Fan Tachometer function

 can only work when GPIO is in ”input mode” .


> > +
> > +- enable : enable fan tach #X, X ranges from 0 through 16. boolean
> value.
>
> 'status' property is the standard way to enable a node or not. Not sure
> if that makes sense here though.
>
Had to change this property to indicate Fan tach channel number.


>
> > +
> > +- pwm_source : indicates PWM source of fan tach #X, X ranges from 0
> through 16.
> > +            integer value in the range 0 to 7. 0 indicates PWM port A,
> > +            1 indicates PWM port B and so on till 7 indicates PWM port
> H.
>

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
new file mode 100644
index 000000000000..8f346409ee8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed-pwm-tacho.txt
@@ -0,0 +1,153 @@ 
+ASPEED AST2400/AST2500 PWM and Fan Tacho controller device driver
+
+The ASPEED PWM controller can support upto 8 PWM outputs. The ASPEED Fan Tacho
+controller can support upto 16 tachometer inputs. The PWM controller supports
+3 types of frequency mode PWM for fan speed control. PWM clock types M, N and 0
+are 3 types of frequency mode PWM just to have 3 independent PWM sources.
+
+Required properties for pwm_tacho node:
+- #address-cells : should be 1.
+
+- #size-cells : should be 1.
+
+- reg : address and length of the register set for the device.
+
+- pinctrl-names : a pinctrl state named "default" must be defined.
+
+- pinctrl-0 : phandle referencing pin configuration of the AST2400/AST2500 PWM
+	      ports.
+
+- compatible : should be "aspeed,aspeed2400-pwm-tacho" for AST2400 or
+	       "aspeed,aspeed2500-pwm-tacho" for AST2500.
+
+- clocks : a fixed clock providing input clock frequency(PWM
+	   and Fan Tach clock)
+
+type_values subnode format:
+===========================
+Under type_values subnode there can be upto 3 child nodes indicating type M/N/O
+values. Atleast one child node is required.
+
+Required properties for the child node(type M/N/O):
+- pwm_period : indicates type M/N/O PWM period, as per the AST2400/AST2500
+	       datasheet. integer value in the range 0 to 255.
+
+- pwm_clock_division_l : indicates type M/N/O PWM clock division L value,
+			 as per the AST2400/AST2500 datasheet.
+			 integer value in the range 0 to 15.
+			 0 here indicates divide 1, 1 indicates divide 2,
+			 2 indicates divide 4, 3 indicates divide 6, and so on
+			 till 15 indicates divide 30.
+
+- pwm_clock_division_h : indicates type M/N/O PWM clock division H value,
+			 as per the AST2400/AST2500 datasheet.
+			 integer value in the range 0 to 15.
+			 0 here indicates divide 1, 1 indicates divide 2,
+			 2 indicates divide 4, 3 indicates divide 8, and so on
+			 till 15 indicates divide 32768.
+
+- fan_tach_enable : indicates fan tach enable of type M/N/O as per the
+		    AST2400/AST2500 datasheet. boolean value.
+
+- fan_tach_clock_division : indicates fan tach clock division as per the
+			    AST2400/AST2500 datasheet.
+			    integer value in the range 0 to 7.
+			    0 indicates divide 4, 1 indicates divide 16,
+			    2 indicates divide 64, 3 indicates divide 256
+			    and so on till 7 indicates divide 65536.
+
+- fan_tach_mode_selection : indicates fan tach mode mode selection as per the
+			    AST2400/AST2500 datasheet. integer value in the
+			    range 0 to 2. 0 indicates falling edge, 1 indicates
+			    rising edge and 2 indicates both edges.
+
+- fan_tach_period : indicates fan tach period as per the AST2400/AST2500
+		    datasheet. integer value (can be upto 16 bits long).
+
+pwm_port subnode format:
+========================
+Under pwm_port subnode there can upto 8 child nodes each indicating values
+for one of the 8 PWM output ports.
+
+Required properties for each child node(starting from PWM A through PWM H):
+- enable : enable PWM #X port, X ranges from A through H. boolean value.
+
+- type : indicates type selection value of PWM #X port, X ranges from A
+	 through H. integer value in the range 0 to 2;
+	 0 indicates type M, 1 indicates type N, 2 indicates type O.
+
+- fan_ctrl : set the PWM fan control initial value. integer value between
+	     0(off) and 255(full speed).
+
+fan_tach_channel subnode format:
+================================
+Under fan_tach_channel subnode there can be upto 16 child nodes each indicating
+values for one of the 16 fan tach channels.
+
+Required properties for each child node(starting from fan tach #0 through
+fan tach #16):
+- fan-ctrl-gpios : should specify the tachometer input pin on the hardware.
+
+- enable : enable fan tach #X, X ranges from 0 through 16. boolean value.
+
+- pwm_source : indicates PWM source of fan tach #X, X ranges from 0 through 16.
+	       integer value in the range 0 to 7. 0 indicates PWM port A,
+	       1 indicates PWM port B and so on till 7 indicates PWM port H.
+
+Examples:
+
+pwm_tacho_fixed_clk: fixedclk {
+	compatible = "fixed-clock";
+	#clock-cells = <0>;
+	clock-frequency = <24000000>;
+}
+
+pwm_tacho: pwm-tacho-controller@1e786000 {
+	#address-cells = <1>;
+	#size-cells = <1>;
+	reg = <0x1E786000 0x1000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm0_default &pinctrl_pwm1_default>;
+	compatible = "aspeed,aspeed2500-pwm-tacho";
+	clocks = <&pwm_tacho_fixed_clk>;
+	type_values {
+		typem {
+			pwm_period = /bits/ 8 <0x5F>;
+			pwm_clock_division_h = /bits/ 8 <0x00>;
+			pwm_clock_division_l = /bits/ 8 <0x05>;
+			fan_tach_enable;
+			fan_tach_clock_division = /bits/ 8 <0x00>;
+			fan_tach_mode_selection = /bits/ 8 <0x00>;
+			fan_tach_period = /bits/ 16 <0x1000>;
+		};
+	};
+
+	pwm_port {
+		pwm_port0 {
+			enable;
+			type = /bits/ 8 <0x00>;
+			fan_ctrl = /bits/ 8 <0xFF>;
+		};
+
+		pwm_port1 {
+			enable;
+			type = /bits/ 8 <0x00>;
+			fan_ctrl = /bits/ 8 <0xFF>;
+		};
+        };
+
+	fan_tach_channel {
+		fan_tach0 {
+			fan-ctrl-gpios = <&gpio ASPEED_GPIO(O, 0) GPIO_ACTIVE_HIGH>;
+			enable;
+			pwm_source = /bits/ 8 <0x00>;
+		};
+
+		fan_tach1 {
+			fan-ctrl-gpios = <&gpio ASPEED_GPIO(O, 1) GPIO_ACTIVE_HIGH>;
+			enable;
+			pwm_source = /bits/ 8 <0x01>;
+		};
+
+	};
+};