diff mbox series

[RESEND,v10,2/3] dt-bindings: hwmon: Support Aspeed g6 PWM TACH Control

Message ID 20231107105025.1480561-3-billy_tsai@aspeedtech.com
State Superseded
Headers show
Series Support pwm/tach driver for aspeed ast26xx | expand

Commit Message

Billy Tsai Nov. 7, 2023, 10:50 a.m. UTC
Document the compatible for aspeed,ast2600-pwm-tach device, which can
support up to 16 PWM outputs and 16 fan tach input.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml

Comments

Billy Tsai Nov. 14, 2023, 3:11 a.m. UTC | #1
> > Document the compatible for aspeed,ast2600-pwm-tach device, which can
> > support up to 16 PWM outputs and 16 fan tach input.
> >
> > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > ---
> >  .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 69 +++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > new file mode 100644
> > index 000000000000..c615fb10705c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (C) 2023 Aspeed, Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/hwmon/aspeed,g6-pwm-tach.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ASPEED G6 PWM and Fan Tach controller
> > +
> > +maintainers:
> > +  - Billy Tsai <billy_tsai@aspeedtech.com>
> > +
> > +description: |
> > +  The ASPEED PWM controller can support up to 16 PWM outputs.
> > +  The ASPEED Fan Tacho controller can support up to 16 fan tach input.
> > +  They are independent hardware blocks, which are different from the
> > +  previous version of the ASPEED chip.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - aspeed,ast2600-pwm-tach
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 1
> > +
> > +  "#pwm-cells":
> > +    const: 3
> > +
> > +patternProperties:
> > +  "^fan-[0-9]+$":
> > +    $ref: fan-common.yaml#
> > +    unevaluatedProperties: false
> > +    required:
> > +      - tach-ch
> > +
> > +required:
> > +  - reg
> > +  - clocks
> > +  - resets
> > +  - "#pwm-cells"
> > +  - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/aspeed-clock.h>
> > +    pwm_tach: pwm-tach-controller@1e610000 {
> > +      compatible = "aspeed,ast2600-pwm-tach";
> > +      reg = <0x1e610000 0x100>;
> > +      clocks = <&syscon ASPEED_CLK_AHB>;
> > +      resets = <&syscon ASPEED_RESET_PWM>;
> > +      #pwm-cells = <3>;
> > +
> > +      fan-0 {

> I assume there's a PWM connection here? How do you know which PWM? You
> said the tach channel is independent, so it is not that.

> It should not be 0 from 'fan-0' because that's just a meaningless index.

> You either need 'pwms' here or you can use 'reg' and the reg value is
> the PWM channel.

Hi Rob, this binding is used to export the PWM provider and the Fan monitor (i.e., Tach).
If the user wants to add the PWM connection for the fan, it can be done as follows:

fan0: pwm-fan0 {
        compatible = "pwm-fan";
        pwms = <&pwm_tach 0 40000 0>; 
        cooling-min-state = <0>;
        cooling-max-state = <3>;
        #cooling-cells = <2>;
        cooling-levels = <0 15 128 255>;
};

This will reuse the existing PWM fan driver (e.g., pwm-fan.c).

Thanks

Billy

> > +        tach-ch = /bits/ 8 <0x0>;
> > +      };
> > +
> > +      fan-1 {
> > +        tach-ch = /bits/ 8 <0x1 0x2>;
> > +      };
> > +    };
> > --
> > 2.25.1
> >
Rob Herring (Arm) Nov. 22, 2023, 8:47 p.m. UTC | #2
On Mon, Nov 13, 2023 at 8:11 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote:
>
> > > Document the compatible for aspeed,ast2600-pwm-tach device, which can
> > > support up to 16 PWM outputs and 16 fan tach input.
> > >
> > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > > ---
> > >  .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 69 +++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > > new file mode 100644
> > > index 000000000000..c615fb10705c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > > @@ -0,0 +1,69 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +# Copyright (C) 2023 Aspeed, Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/hwmon/aspeed,g6-pwm-tach.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ASPEED G6 PWM and Fan Tach controller
> > > +
> > > +maintainers:
> > > +  - Billy Tsai <billy_tsai@aspeedtech.com>
> > > +
> > > +description: |
> > > +  The ASPEED PWM controller can support up to 16 PWM outputs.
> > > +  The ASPEED Fan Tacho controller can support up to 16 fan tach input.
> > > +  They are independent hardware blocks, which are different from the
> > > +  previous version of the ASPEED chip.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - aspeed,ast2600-pwm-tach
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    maxItems: 1
> > > +
> > > +  resets:
> > > +    maxItems: 1
> > > +
> > > +  "#pwm-cells":
> > > +    const: 3
> > > +
> > > +patternProperties:
> > > +  "^fan-[0-9]+$":
> > > +    $ref: fan-common.yaml#
> > > +    unevaluatedProperties: false
> > > +    required:
> > > +      - tach-ch
> > > +
> > > +required:
> > > +  - reg
> > > +  - clocks
> > > +  - resets
> > > +  - "#pwm-cells"
> > > +  - compatible
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/aspeed-clock.h>
> > > +    pwm_tach: pwm-tach-controller@1e610000 {
> > > +      compatible = "aspeed,ast2600-pwm-tach";
> > > +      reg = <0x1e610000 0x100>;
> > > +      clocks = <&syscon ASPEED_CLK_AHB>;
> > > +      resets = <&syscon ASPEED_RESET_PWM>;
> > > +      #pwm-cells = <3>;
> > > +
> > > +      fan-0 {
>
> > I assume there's a PWM connection here? How do you know which PWM? You
> > said the tach channel is independent, so it is not that.
>
> > It should not be 0 from 'fan-0' because that's just a meaningless index.
>
> > You either need 'pwms' here or you can use 'reg' and the reg value is
> > the PWM channel.
>
> Hi Rob, this binding is used to export the PWM provider and the Fan monitor (i.e., Tach).
> If the user wants to add the PWM connection for the fan, it can be done as follows:
>
> fan0: pwm-fan0 {
>         compatible = "pwm-fan";
>         pwms = <&pwm_tach 0 40000 0>;
>         cooling-min-state = <0>;
>         cooling-max-state = <3>;
>         #cooling-cells = <2>;
>         cooling-levels = <0 15 128 255>;
> };
>
> This will reuse the existing PWM fan driver (e.g., pwm-fan.c).

I'm confused now. So what are the child nodes you have? You are
defining the fan in 2 places? The "pwm-fan" driver supports a tach via
an interrupt, so how would this work in your case?

Rob
Rob Herring (Arm) Jan. 13, 2024, 1:53 a.m. UTC | #3
On Sun, Nov 26, 2023 at 11:45 PM Billy Tsai <billy_tsai@aspeedtech.com> wrote:
>
> > > > > Document the compatible for aspeed,ast2600-pwm-tach device, which can
> > > > > support up to 16 PWM outputs and 16 fan tach input.
> > > > >
> > > > > Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> > > > > ---
> > > > >  .../bindings/hwmon/aspeed,g6-pwm-tach.yaml    | 69 +++++++++++++++++++
> > > > >  1 file changed, 69 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..c615fb10705c
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
> > > > > @@ -0,0 +1,69 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +# Copyright (C) 2023 Aspeed, Inc.
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/hwmon/aspeed,g6-pwm-tach.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: ASPEED G6 PWM and Fan Tach controller
> > > > > +
> > > > > +maintainers:
> > > > > +  - Billy Tsai <billy_tsai@aspeedtech.com>
> > > > > +
> > > > > +description: |
> > > > > +  The ASPEED PWM controller can support up to 16 PWM outputs.
> > > > > +  The ASPEED Fan Tacho controller can support up to 16 fan tach input.
> > > > > +  They are independent hardware blocks, which are different from the
> > > > > +  previous version of the ASPEED chip.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - aspeed,ast2600-pwm-tach
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  clocks:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  resets:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  "#pwm-cells":
> > > > > +    const: 3
> > > > > +
> > > > > +patternProperties:
> > > > > +  "^fan-[0-9]+$":
> > > > > +    $ref: fan-common.yaml#
> > > > > +    unevaluatedProperties: false
> > > > > +    required:
> > > > > +      - tach-ch
> > > > > +
> > > > > +required:
> > > > > +  - reg
> > > > > +  - clocks
> > > > > +  - resets
> > > > > +  - "#pwm-cells"
> > > > > +  - compatible
> > > > > +
> > > > > +additionalProperties: false
> > > > > +
> > > > > +examples:
> > > > > +  - |
> > > > > +    #include <dt-bindings/clock/aspeed-clock.h>
> > > > > +    pwm_tach: pwm-tach-controller@1e610000 {
> > > > > +      compatible = "aspeed,ast2600-pwm-tach";
> > > > > +      reg = <0x1e610000 0x100>;
> > > > > +      clocks = <&syscon ASPEED_CLK_AHB>;
> > > > > +      resets = <&syscon ASPEED_RESET_PWM>;
> > > > > +      #pwm-cells = <3>;
> > > > > +
> > > > > +      fan-0 {
> > >
> > > > I assume there's a PWM connection here? How do you know which PWM? You
> > > > said the tach channel is independent, so it is not that.
> > >
> > > > It should not be 0 from 'fan-0' because that's just a meaningless index.
> > >
> > > > You either need 'pwms' here or you can use 'reg' and the reg value is
> > > > the PWM channel.
> > >
> > > Hi Rob, this binding is used to export the PWM provider and the Fan monitor (i.e., Tach).
> > > If the user wants to add the PWM connection for the fan, it can be done as follows:
> > >
> > > fan0: pwm-fan0 {
> > >         compatible = "pwm-fan";
> > >         pwms = <&pwm_tach 0 40000 0>;
> > >         cooling-min-state = <0>;
> > >         cooling-max-state = <3>;
> > >         #cooling-cells = <2>;
> > >         cooling-levels = <0 15 128 255>;
> > > };
> > >
> > > This will reuse the existing PWM fan driver (e.g., pwm-fan.c).
>
> > I'm confused now. So what are the child nodes you have? You are
> > defining the fan in 2 places? The "pwm-fan" driver supports a tach via
> > an interrupt, so how would this work in your case?
>
> Hi Rob,
>
> The tach interrupt for the pwm-fan is option. In our case, the dts just reuse the pwm control function
> of the pwm-fan, and the part of the tach monitor will be created by our fan child nodes.
> So the dts will like followings:
>
> // Use to declare the tach monitor for fan.
> &pwm_tach {
>         fan-0 {
>                 tach-ch = /bits/ 8 <0x0>;
>         };
>         fan-1 {
>                 tach-ch = /bits/ 8 <0x1>;
>         };
>         ...
> }
>
> // Reuse the pwm-fan.c to control the behavior of the PWM for fan.
> fan0: pwm-fan0 {
>         compatible = "pwm-fan";
>         pwms = <&pwm_tach 0 40000 0>;   /* Target freq:25 kHz */
>         cooling-min-state = <0>;
>         cooling-max-state = <3>;
>         #cooling-cells = <2>;
>         cooling-levels = <0 15 128 255>;
> };

No, you can't have a fan described by 2 different nodes. Can't you
just merge everything from the pwm-fan0 node into the fan-0 node?

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
new file mode 100644
index 000000000000..c615fb10705c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,g6-pwm-tach.yaml
@@ -0,0 +1,69 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2023 Aspeed, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/aspeed,g6-pwm-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ASPEED G6 PWM and Fan Tach controller
+
+maintainers:
+  - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+  The ASPEED PWM controller can support up to 16 PWM outputs.
+  The ASPEED Fan Tacho controller can support up to 16 fan tach input.
+  They are independent hardware blocks, which are different from the
+  previous version of the ASPEED chip.
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-pwm-tach
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+patternProperties:
+  "^fan-[0-9]+$":
+    $ref: fan-common.yaml#
+    unevaluatedProperties: false
+    required:
+      - tach-ch
+
+required:
+  - reg
+  - clocks
+  - resets
+  - "#pwm-cells"
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/aspeed-clock.h>
+    pwm_tach: pwm-tach-controller@1e610000 {
+      compatible = "aspeed,ast2600-pwm-tach";
+      reg = <0x1e610000 0x100>;
+      clocks = <&syscon ASPEED_CLK_AHB>;
+      resets = <&syscon ASPEED_RESET_PWM>;
+      #pwm-cells = <3>;
+
+      fan-0 {
+        tach-ch = /bits/ 8 <0x0>;
+      };
+
+      fan-1 {
+        tach-ch = /bits/ 8 <0x1 0x2>;
+      };
+    };