* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 2:18 [v6 0/4] Support pwm/tach driver for aspeed ast26xx Billy Tsai
@ 2023-06-08 2:18 ` Billy Tsai
2023-06-08 6:39 ` Krzysztof Kozlowski
2023-06-08 6:46 ` Krzysztof Kozlowski
2023-06-08 2:18 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH " Billy Tsai
` (3 subsequent siblings)
4 siblings, 2 replies; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 2:18 UTC (permalink / raw)
To: linux-aspeed
Document the compatible for aspeed,ast2600-pwm device.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
.../bindings/pwm/aspeed,ast2600-pwm.yaml | 38 +++++++++++++++++++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
new file mode 100644
index 000000000000..a9e040263578
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 Aspeed, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/aspeed,ast2600-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed Ast2600 PWM controller
+
+maintainers:
+ - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+ The Aspeed PWM controller supports up to 1 PWM outputs.
+
+allOf:
+ - $ref: pwm.yaml#
+
+properties:
+ compatible:
+ enum:
+ - aspeed,ast2600-pwm
+
+ "#pwm-cells":
+ const: 3
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+required:
+ - compatible
+ - clocks
+ - resets
+
+additionalProperties: false
--
2.25.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 2:18 ` [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation Billy Tsai
@ 2023-06-08 6:39 ` Krzysztof Kozlowski
2023-06-08 6:46 ` Krzysztof Kozlowski
1 sibling, 0 replies; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 6:39 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-pwm device.
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.
Thank you.
I don't understand why you make the same mistakes, even though I pointed
them out two times already.
I am not going to point third time. Sorry, it's a waste of my time.
NAK.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 2:18 ` [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation Billy Tsai
2023-06-08 6:39 ` Krzysztof Kozlowski
@ 2023-06-08 6:46 ` Krzysztof Kozlowski
2023-06-08 7:47 ` Billy Tsai
1 sibling, 1 reply; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 6:46 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-pwm device.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> .../bindings/pwm/aspeed,ast2600-pwm.yaml | 38 +++++++++++++++++++
> 1 file changed, 38 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
>
> diff --git a/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> new file mode 100644
> index 000000000000..a9e040263578
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/aspeed,ast2600-pwm.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/aspeed,ast2600-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 PWM controller
> +
> +maintainers:
> + - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> + The Aspeed PWM controller supports up to 1 PWM outputs.
This does not look right.
> +
> +allOf:
> + - $ref: pwm.yaml#
> +
> +properties:
> + compatible:
> + enum:
> + - aspeed,ast2600-pwm
> +
> + "#pwm-cells":
> + const: 3
3 cells? For one PWM? What are they?
> +
> + clocks:
> + maxItems: 1
> +
> + resets:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - clocks
> + - resets
> +
> +additionalProperties: false
Missing examples. All bindings need examples.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 6:46 ` Krzysztof Kozlowski
@ 2023-06-08 7:47 ` Billy Tsai
2023-06-08 7:54 ` Krzysztof Kozlowski
0 siblings, 1 reply; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 7:47 UTC (permalink / raw)
To: linux-aspeed
>> +
>> +allOf:
>> + - $ref: pwm.yaml#
>> +
>> +properties:
>> + compatible:
>> + enum:
>> + - aspeed,ast2600-pwm
>> +
>> + "#pwm-cells":
>> + const: 3
> 3 cells? For one PWM? What are they?
channel, period and polarity.
>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + resets:
>> + maxItems: 1
>> +
>> +required:
>> + - compatible
>> + - clocks
>> + - resets
>> +
>> +additionalProperties: false
> Missing examples. All bindings need examples.
The examples will looks like following:
pwm0: pwm0 at 1e610000 {
compatible = "aspeed,ast2600-pwm";
reg = <0x1e610000 0x8>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm0_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
pwm1: pwm1 at 1e610010 {
compatible = "aspeed,ast2600-pwm";
reg = <0x1e610010 0x8>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm1_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
...
Thanks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20230608/1cd9195e/attachment-0001.htm>
^ permalink raw reply [flat|nested] 41+ messages in thread* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 7:47 ` Billy Tsai
@ 2023-06-08 7:54 ` Krzysztof Kozlowski
2023-06-08 8:21 ` Billy Tsai
0 siblings, 1 reply; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 7:54 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 09:47, Billy Tsai wrote:
>
> >> +
> >> +allOf:
> >> + - $ref: pwm.yaml#
> >> +
> >> +properties:
> >> + compatible:
> >> + enum:
> >> + - aspeed,ast2600-pwm
> >> +
> >> + "#pwm-cells":
> >> + const: 3
>
> > 3 cells? For one PWM? What are they?
>
> channel, period and polarity.
Don't cut my responses. You wrote you have one PWM output, so only one
channel. What do you put then in the channel?
I will start NAKing such patches without DTS user. It's like reviewing
fake code for some unknown solution and trying to get from you piece of
answers one by one, because you do not want to share entire part.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 7:54 ` Krzysztof Kozlowski
@ 2023-06-08 8:21 ` Billy Tsai
2023-06-08 8:36 ` Krzysztof Kozlowski
2023-06-08 8:39 ` Krzysztof Kozlowski
0 siblings, 2 replies; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 8:21 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 09:47, Billy Tsai wrote:
>>
>> >> +
>> >> +allOf:
>> >> + - $ref: pwm.yaml#
>> >> +
>> >> +properties:
>> >> + compatible:
>> >> + enum:
>> >> + - aspeed,ast2600-pwm
>> >> +
>> >> + "#pwm-cells":
>> >> + const: 3
>>
>> > 3 cells? For one PWM? What are they?
>>
>> channel, period and polarity.
> Don't cut my responses. You wrote you have one PWM output, so only one
> channel. What do you put then in the channel?
You need to put 0 in the cell of the channel, the example of the dts usage will like following:
pwm0: pwm0 at 1e610000 {
compatible = "aspeed,ast2600-pwm";
reg = <0x1e610000 0x8>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm0_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "okay";
};
pwm1: pwm1 at 1e610010 {
compatible = "aspeed,ast2600-pwm";
reg = <0x1e610010 0x8>;
#pwm-cells = <3>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm1_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "okay";
};
fan0: pwm-fan0 {
compatible = "pwm-fan";
pwms = <&pwm0 0 40000 0>; /* Target freq:25 kHz */
cooling-min-state = <0>;
cooling-max-state = <3>;
#cooling-cells = <2>;
cooling-levels = <0 15 128 255>;
};
fan1: pwm-fan1 {
compatible = "pwm-fan";
pwms = <&pwm1 0 40000 0>; /* Target freq:25 kHz */
cooling-min-state = <0>;
cooling-max-state = <3>;
#cooling-cells = <2>;
cooling-levels = <0 15 128 255>;
};
> I will start NAKing such patches without DTS user. It's like reviewing
> fake code for some unknown solution and trying to get from you piece of
> answers one by one, because you do not want to share entire part.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20230608/91403601/attachment-0001.htm>
^ permalink raw reply [flat|nested] 41+ messages in thread* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 8:21 ` Billy Tsai
@ 2023-06-08 8:36 ` Krzysztof Kozlowski
2023-06-08 8:57 ` Billy Tsai
2023-06-08 8:39 ` Krzysztof Kozlowski
1 sibling, 1 reply; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 8:36 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 10:21, Billy Tsai wrote:
> On 08/06/2023 09:47, Billy Tsai wrote:
> >>
> >> >> +
> >> >> +allOf:
> >> >> + - $ref: pwm.yaml#
> >> >> +
> >> >> +properties:
> >> >> + compatible:
> >> >> + enum:
> >> >> + - aspeed,ast2600-pwm
> >> >> +
> >> >> + "#pwm-cells":
> >> >> + const: 3
> >>
> >> > 3 cells? For one PWM? What are they?
> >>
> >> channel, period and polarity.
>
> > Don't cut my responses. You wrote you have one PWM output, so only one
> > channel. What do you put then in the channel?
>
> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
If you always put 0 isn't this a proof that it's wrong?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 8:36 ` Krzysztof Kozlowski
@ 2023-06-08 8:57 ` Billy Tsai
2023-06-08 9:26 ` Krzysztof Kozlowski
0 siblings, 1 reply; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 8:57 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 10:21, Billy Tsai wrote:
>> On 08/06/2023 09:47, Billy Tsai wrote:
>> >>
>> >> >> +
>> >> >> +allOf:
>> >> >> + - $ref: pwm.yaml#
>> >> >> +
>> >> >> +properties:
>> >> >> + compatible:
>> >> >> + enum:
>> >> >> + - aspeed,ast2600-pwm
>> >> >> +
>> >> >> + "#pwm-cells":
>> >> >> + const: 3
>> >>
>> >> > 3 cells? For one PWM? What are they?
>> >>
>> >> channel, period and polarity.
>>
>> > Don't cut my responses. You wrote you have one PWM output, so only one
>> > channel. What do you put then in the channel?
>>
>> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
> If you always put 0 isn't this a proof that it's wrong?
No, if your PWM controller only has one pwm output, then it should only be configured as 0.
This is the usage of the pwm-cells property.
https://github.com/torvalds/linux/blob/master/drivers/pwm/core.c#L129-L158
https://github.com/torvalds/linux/blob/master/include/linux/pwm.h#LL299C20-L299C20
All of the pwm driver with npwm = 1 will has the same usage.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20230608/93652b02/attachment-0001.htm>
^ permalink raw reply [flat|nested] 41+ messages in thread* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 8:57 ` Billy Tsai
@ 2023-06-08 9:26 ` Krzysztof Kozlowski
0 siblings, 0 replies; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 9:26 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 10:57, Billy Tsai wrote:
> On 08/06/2023 10:21, Billy Tsai wrote:
> >> On 08/06/2023 09:47, Billy Tsai wrote:
> >> >>
> >> >> >> +
> >> >> >> +allOf:
> >> >> >> + - $ref: pwm.yaml#
> >> >> >> +
> >> >> >> +properties:
> >> >> >> + compatible:
> >> >> >> + enum:
> >> >> >> + - aspeed,ast2600-pwm
> >> >> >> +
> >> >> >> + "#pwm-cells":
> >> >> >> + const: 3
> >> >>
> >> >> > 3 cells? For one PWM? What are they?
> >> >>
> >> >> channel, period and polarity.
> >>
> >> > Don't cut my responses. You wrote you have one PWM output, so only one
> >> > channel. What do you put then in the channel?
> >>
> >> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
>
> > If you always put 0 isn't this a proof that it's wrong?
>
> No, if your PWM controller only has one pwm output, then it should only be configured as 0.
> This is the usage of the pwm-cells property.
> https://github.com/torvalds/linux/blob/master/drivers/pwm/core.c#L129-L158
This is only when you use generic of_xlate. You do not have to use
generic of_xlate if it does not suite you. Again you speak about the
drivers, but we talk about bindings:
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/pwm/pwm.txt#L13
"controller specific"
> https://github.com/torvalds/linux/blob/master/include/linux/pwm.h#LL299C20-L299C20
> All of the pwm driver with npwm = 1 will has the same usage.
So it seems many simplified their drivers...
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 8:21 ` Billy Tsai
2023-06-08 8:36 ` Krzysztof Kozlowski
@ 2023-06-08 8:39 ` Krzysztof Kozlowski
2023-06-08 9:15 ` Billy Tsai
1 sibling, 1 reply; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 8:39 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 10:21, Billy Tsai wrote:
> On 08/06/2023 09:47, Billy Tsai wrote:
> >>
> >> >> +
> >> >> +allOf:
> >> >> + - $ref: pwm.yaml#
> >> >> +
> >> >> +properties:
> >> >> + compatible:
> >> >> + enum:
> >> >> + - aspeed,ast2600-pwm
> >> >> +
> >> >> + "#pwm-cells":
> >> >> + const: 3
> >>
> >> > 3 cells? For one PWM? What are they?
> >>
> >> channel, period and polarity.
>
> > Don't cut my responses. You wrote you have one PWM output, so only one
> > channel. What do you put then in the channel?
>
> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
>
> pwm0: pwm0 at 1e610000 {
> compatible = "aspeed,ast2600-pwm";
> reg = <0x1e610000 0x8>;
> #pwm-cells = <3>;
> #address-cells = <1>;
> #size-cells = <0>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_pwm0_default>;
> clocks = <&syscon ASPEED_CLK_AHB>;
> resets = <&syscon ASPEED_RESET_PWM>;
> status = "okay";
> };
>
> pwm1: pwm1 at 1e610010 {
> compatible = "aspeed,ast2600-pwm";
> reg = <0x1e610010 0x8>;
> #pwm-cells = <3>;
> #address-cells = <1>;
> #size-cells = <0>;
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_pwm1_default>;
> clocks = <&syscon ASPEED_CLK_AHB>;
> resets = <&syscon ASPEED_RESET_PWM>;
> status = "okay";
BTW, these are not two PWM devices but one. I don't understand why you
changed previous design into something like this, but this is not
representing your hardware.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 8:39 ` Krzysztof Kozlowski
@ 2023-06-08 9:15 ` Billy Tsai
2023-06-08 9:27 ` Krzysztof Kozlowski
0 siblings, 1 reply; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 9:15 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 10:21, Billy Tsai wrote:
>> On 08/06/2023 09:47, Billy Tsai wrote:
>> >>
>> >> >> +
>> >> >> +allOf:
>> >> >> + - $ref: pwm.yaml#
>> >> >> +
>> >> >> +properties:
>> >> >> + compatible:
>> >> >> + enum:
>> >> >> + - aspeed,ast2600-pwm
>> >> >> +
>> >> >> + "#pwm-cells":
>> >> >> + const: 3
>> >>
>> >> > 3 cells? For one PWM? What are they?
>> >>
>> >> channel, period and polarity.
>>
>> > Don't cut my responses. You wrote you have one PWM output, so only one
>> > channel. What do you put then in the channel?
>>
>> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
>>
>> pwm0: pwm0 at 1e610000 {
>> compatible = "aspeed,ast2600-pwm";
>> reg = <0x1e610000 0x8>;
>> #pwm-cells = <3>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_pwm0_default>;
>> clocks = <&syscon ASPEED_CLK_AHB>;
>> resets = <&syscon ASPEED_RESET_PWM>;
>> status = "okay";
>> };
>>
>> pwm1: pwm1 at 1e610010 {
>> compatible = "aspeed,ast2600-pwm";
>> reg = <0x1e610010 0x8>;
>> #pwm-cells = <3>;
>> #address-cells = <1>;
>> #size-cells = <0>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&pinctrl_pwm1_default>;
>> clocks = <&syscon ASPEED_CLK_AHB>;
>> resets = <&syscon ASPEED_RESET_PWM>;
>> status = "okay";
> BTW, these are not two PWM devices but one. I don't understand why you
> changed previous design into something like this, but this is not
> representing your hardware.
The previous design of my patch treated our PWM controller as having 16 PWM channels.
However, from a hardware perspective, it consists of 16 individual PWM chips, each
with its own set of two 4-byte control registers. These chips operate independently
and are not affected by each other.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20230608/e4ae7379/attachment.htm>
^ permalink raw reply [flat|nested] 41+ messages in thread* [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation
2023-06-08 9:15 ` Billy Tsai
@ 2023-06-08 9:27 ` Krzysztof Kozlowski
0 siblings, 0 replies; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 9:27 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 11:15, Billy Tsai wrote:
> On 08/06/2023 10:21, Billy Tsai wrote:
> >> On 08/06/2023 09:47, Billy Tsai wrote:
> >> >>
> >> >> >> +
> >> >> >> +allOf:
> >> >> >> + - $ref: pwm.yaml#
> >> >> >> +
> >> >> >> +properties:
> >> >> >> + compatible:
> >> >> >> + enum:
> >> >> >> + - aspeed,ast2600-pwm
> >> >> >> +
> >> >> >> + "#pwm-cells":
> >> >> >> + const: 3
> >> >>
> >> >> > 3 cells? For one PWM? What are they?
> >> >>
> >> >> channel, period and polarity.
> >>
> >> > Don't cut my responses. You wrote you have one PWM output, so only one
> >> > channel. What do you put then in the channel?
> >>
> >> You need to put 0 in the cell of the channel, the example of the dts usage will like following:
> >>
> >> pwm0: pwm0 at 1e610000 {
> >> compatible = "aspeed,ast2600-pwm";
> >> reg = <0x1e610000 0x8>;
> >> #pwm-cells = <3>;
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&pinctrl_pwm0_default>;
> >> clocks = <&syscon ASPEED_CLK_AHB>;
> >> resets = <&syscon ASPEED_RESET_PWM>;
> >> status = "okay";
> >> };
> >>
> >> pwm1: pwm1 at 1e610010 {
> >> compatible = "aspeed,ast2600-pwm";
> >> reg = <0x1e610010 0x8>;
> >> #pwm-cells = <3>;
> >> #address-cells = <1>;
> >> #size-cells = <0>;
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&pinctrl_pwm1_default>;
> >> clocks = <&syscon ASPEED_CLK_AHB>;
> >> resets = <&syscon ASPEED_RESET_PWM>;
> >> status = "okay";
>
> > BTW, these are not two PWM devices but one. I don't understand why you
> > changed previous design into something like this, but this is not
> > representing your hardware.
>
> The previous design of my patch treated our PWM controller as having 16 PWM channels.
> However, from a hardware perspective, it consists of 16 individual PWM chips, each
> with its own set of two 4-byte control registers. These chips operate independently
> and are not affected by each other.
They are affected by each other - you use the same clock and reset line.
I really doubt you have 16 PWM controllers. Anyway, I cannot judge.
Either your previous submissions were totally bogus or this one is.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
2023-06-08 2:18 [v6 0/4] Support pwm/tach driver for aspeed ast26xx Billy Tsai
2023-06-08 2:18 ` [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation Billy Tsai
@ 2023-06-08 2:18 ` Billy Tsai
2023-06-08 4:58 ` Guenter Roeck
2023-06-08 6:40 ` Krzysztof Kozlowski
2023-06-08 2:18 ` [v6 3/4] pwm: Add Aspeed ast2600 PWM support Billy Tsai
` (2 subsequent siblings)
4 siblings, 2 replies; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 2:18 UTC (permalink / raw)
To: linux-aspeed
Document the compatible for aspeed,ast2600-tach device.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
.../bindings/hwmon/aspeed,ast2600-tach.yaml | 32 +++++++++++++++++++
1 file changed, 32 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
new file mode 100644
index 000000000000..627aa00f2e92
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
@@ -0,0 +1,32 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2021 Aspeed, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed Ast2600 Tach controller
+
+maintainers:
+ - Billy Tsai <billy_tsai@aspeedtech.com>
+
+description: |
+ The Aspeed Tach controller can support upto 1 fan input.
+
+properties:
+ compatible:
+ enum:
+ - aspeed,ast2600-tach
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+required:
+ - compatible
+ - clocks
+ - resets
+
+additionalProperties: false
--
2.25.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
2023-06-08 2:18 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH " Billy Tsai
@ 2023-06-08 4:58 ` Guenter Roeck
2023-06-08 6:21 ` Billy Tsai
2023-06-08 6:40 ` Krzysztof Kozlowski
1 sibling, 1 reply; 41+ messages in thread
From: Guenter Roeck @ 2023-06-08 4:58 UTC (permalink / raw)
To: linux-aspeed
On 6/7/23 19:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> .../bindings/hwmon/aspeed,ast2600-tach.yaml | 32 +++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> + - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> + The Aspeed Tach controller can support upto 1 fan input.
> +
The code says:
In Aspeed AST2600 SoC features 16 TACH controllers, with each
controller capable of supporting up to 1 input.
which is a bit different. I guess there are no examples anymore,
but I'd really like to see how this looks like in the devicetree file,
and how the driver is supposed to distinguish/select the 16 inputs.
> +properties:
> + compatible:
> + enum:
> + - aspeed,ast2600-tach
> +
> + clocks:
> + maxItems: 1
> +
> + resets:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - clocks
> + - resets
> +
> +additionalProperties: false
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
2023-06-08 4:58 ` Guenter Roeck
@ 2023-06-08 6:21 ` Billy Tsai
2023-06-08 13:18 ` Guenter Roeck
0 siblings, 1 reply; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 6:21 UTC (permalink / raw)
To: linux-aspeed
> The code says:
> In Aspeed AST2600 SoC features 16 TACH controllers, with each
> controller capable of supporting up to 1 input.
> which is a bit different. I guess there are no examples anymore,
> but I'd really like to see how this looks like in the devicetree file,
> and how the driver is supposed to distinguish/select the 16 inputs.
Hi Roeck,
The node in the devicetree file will looks like following:
tach0: tach0 at 1e610008 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610008 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach0_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach1: tach1 at 1e610018 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610018 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach1_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach2: tach2 at 1e610028 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610028 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach2_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach3: tach3 at 1e610038 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610038 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach3_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach4: tach4 at 1e610048 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610048 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach4_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach5: tach5 at 1e610058 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610058 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach5_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach6: tach6 at 1e610068 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610068 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach6_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach7: tach7 at 1e610078 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610078 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach7_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach8: tach8 at 1e610088 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610088 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach8_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach9: tach9 at 1e610098 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e610098 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach9_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach10: tach10 at 1e6100A8 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e6100A8 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach10_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach11: tach11 at 1e6100B8 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e6100B8 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach11_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach12: tach12 at 1e6100C8 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e6100C8 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach12_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach13: tach13 at 1e6100D8 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e6100D8 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach13_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach14: tach14 at 1e6100E8 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e6100E8 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach14_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
tach15: tach15 at 1e6100F8 {
compatible = "aspeed,ast2600-tach";
reg = <0x1e6100F8 0x8>;
#address-cells = <1>;
#size-cells = <0>;
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_tach15_default>;
clocks = <&syscon ASPEED_CLK_AHB>;
resets = <&syscon ASPEED_RESET_PWM>;
status = "disabled";
};
Thanks
Best Regards,
Billy Tsai
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20230608/84e95682/attachment-0001.htm>
^ permalink raw reply [flat|nested] 41+ messages in thread* [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
2023-06-08 6:21 ` Billy Tsai
@ 2023-06-08 13:18 ` Guenter Roeck
2023-06-28 5:30 ` Billy Tsai
0 siblings, 1 reply; 41+ messages in thread
From: Guenter Roeck @ 2023-06-08 13:18 UTC (permalink / raw)
To: linux-aspeed
On 6/7/23 23:21, Billy Tsai wrote:
> ??????? > The code says:
>
> ??????? > In Aspeed AST2600 SoC features 16 TACH controllers, with each
>
> ??????? > controller capable of supporting up to 1 input.
>
> ??????? > which is a bit different. I guess there are no examples anymore,
>
> ??????? > but I'd really like to see how this looks like in the devicetree file,
>
> ??????? > and how the driver is supposed to distinguish/select the 16 inputs.
>
> Hi Roeck,
>
> The node in the devicetree file will looks like following:
>
> tach0: tach0 at 1e610008 {
>
> ??????? compatible = "aspeed,ast2600-tach";
>
> ??????? reg = <0x1e610008 0x8>;
>
> ??????? #address-cells = <1>;
>
> ??????? #size-cells = <0>;
>
> ??????? pinctrl-names = "default";
>
> ??????? pinctrl-0 = <&pinctrl_tach0_default>;
>
> ??????? clocks = <&syscon ASPEED_CLK_AHB>;
>
> ??????? resets = <&syscon ASPEED_RESET_PWM>;
>
> ??????? status = "disabled";
>
> };
>
Neither reg nor pinctrl is mentioned in the bindings. Maybe that is not needed nowadays,
but I find it confusing.
Either case, it is highly unusual that there would be 16 instances of this device
instead of one. Why is this done ? It doesn't really make sense to me.
Guenter
^ permalink raw reply [flat|nested] 41+ messages in thread* [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
2023-06-08 13:18 ` Guenter Roeck
@ 2023-06-28 5:30 ` Billy Tsai
0 siblings, 0 replies; 41+ messages in thread
From: Billy Tsai @ 2023-06-28 5:30 UTC (permalink / raw)
To: linux-aspeed
On 6/7/23 23:21, Billy Tsai wrote:
> > > The code says:
> >
> > > In Aspeed AST2600 SoC features 16 TACH controllers, with each
> >
> > > controller capable of supporting up to 1 input.
> >
> > > which is a bit different. I guess there are no examples anymore,
> >
> > > but I'd really like to see how this looks like in the devicetree file,
> >
> > > and how the driver is supposed to distinguish/select the 16 inputs.
> >
> > Hi Roeck,
> >
> > The node in the devicetree file will looks like following:
> >
> > tach0: tach0 at 1e610008 {
> >
> > compatible = "aspeed,ast2600-tach";
> >
> > reg = <0x1e610008 0x8>;
> >
> > #address-cells = <1>;
> >
> > #size-cells = <0>;
> >
> > pinctrl-names = "default";
> >
> > pinctrl-0 = <&pinctrl_tach0_default>;
> >
> > clocks = <&syscon ASPEED_CLK_AHB>;
> >
> > resets = <&syscon ASPEED_RESET_PWM>;
> >
> > status = "disabled";
> >
> > };
> >
> Neither reg nor pinctrl is mentioned in the bindings. Maybe that is not needed nowadays,
> but I find it confusing.
> Either case, it is highly unusual that there would be 16 instances of this device
> instead of one. Why is this done ? It doesn't really make sense to me.
Having 16 instances of PWM and Tach is more compatible with our hardware design.
This is because our register layout for PWM and Tach is not continuous.
PWM0 used 0x0 0x4, Tach0 used 0x8 0xc
PWM1 used 0x10 0x14, Tach1 used 0x18 0x1c
...
Each PWM/Tach instance has its own controller register and is not dependent on others.
Thanks
Best Regards,
Billy Tsai
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20230628/02c54aee/attachment.htm>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH Control documentation
2023-06-08 2:18 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH " Billy Tsai
2023-06-08 4:58 ` Guenter Roeck
@ 2023-06-08 6:40 ` Krzysztof Kozlowski
1 sibling, 0 replies; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 6:40 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 04:18, Billy Tsai wrote:
> Document the compatible for aspeed,ast2600-tach device.
This is a friendly reminder during the review process.
It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.
Thank you.
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> .../bindings/hwmon/aspeed,ast2600-tach.yaml | 32 +++++++++++++++++++
> 1 file changed, 32 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
>
> diff --git a/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> new file mode 100644
> index 000000000000..627aa00f2e92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/aspeed,ast2600-tach.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2021 Aspeed, Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwmon/aspeed,ast2600-tach.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed Ast2600 Tach controller
> +
> +maintainers:
> + - Billy Tsai <billy_tsai@aspeedtech.com>
> +
> +description: |
> + The Aspeed Tach controller can support upto 1 fan input.
> +
> +properties:
> + compatible:
> + enum:
> + - aspeed,ast2600-tach
> +
> + clocks:
> + maxItems: 1
> +
> + resets:
> + maxItems: 1
NAK, not true based on previous discussions. Device does not come with
resets and clocks.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 3/4] pwm: Add Aspeed ast2600 PWM support
2023-06-08 2:18 [v6 0/4] Support pwm/tach driver for aspeed ast26xx Billy Tsai
2023-06-08 2:18 ` [v6 1/4] dt-bindings: pwm: Add ASPEED PWM Control documentation Billy Tsai
2023-06-08 2:18 ` [v6 2/4] dt-bindings: hwmon: Add ASPEED TACH " Billy Tsai
@ 2023-06-08 2:18 ` Billy Tsai
2023-07-18 7:08 ` Thierry Reding
2023-06-08 2:18 ` [v6 4/4] hwmon: Add Aspeed ast2600 TACH support Billy Tsai
2023-06-08 6:37 ` [v6 0/4] Support pwm/tach driver for aspeed ast26xx Krzysztof Kozlowski
4 siblings, 1 reply; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 2:18 UTC (permalink / raw)
To: linux-aspeed
Add the support of PWM controller which can be found in the Aspeed AST2600
SoC. This particular SoC features 16 PWM controllers, with each controller
capable of supporting up to 1 PWM output.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
Reviewed-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-aspeed-ast2600.c | 309 +++++++++++++++++++++++++++++++
3 files changed, 320 insertions(+)
create mode 100644 drivers/pwm/pwm-aspeed-ast2600.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..54915185d918 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -51,6 +51,16 @@ config PWM_AB8500
To compile this driver as a module, choose M here: the module
will be called pwm-ab8500.
+config PWM_ASPEED_AST2600
+ tristate "Aspeed ast2600 PWM support"
+ depends on ARCH_ASPEED || COMPILE_TEST
+ depends on HAVE_CLK && HAS_IOMEM
+ help
+ This driver provides support for Aspeed ast2600 PWM controllers.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-aspeed-ast2600.
+
config PWM_ATMEL
tristate "Atmel PWM support"
depends on ARCH_AT91 || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..5169c34056e6 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -2,6 +2,7 @@
obj-$(CONFIG_PWM) += core.o
obj-$(CONFIG_PWM_SYSFS) += sysfs.o
obj-$(CONFIG_PWM_AB8500) += pwm-ab8500.o
+obj-$(CONFIG_PWM_ASPEED_AST2600) += pwm-aspeed-ast2600.o
obj-$(CONFIG_PWM_ATMEL) += pwm-atmel.o
obj-$(CONFIG_PWM_ATMEL_HLCDC_PWM) += pwm-atmel-hlcdc.o
obj-$(CONFIG_PWM_ATMEL_TCB) += pwm-atmel-tcb.o
diff --git a/drivers/pwm/pwm-aspeed-ast2600.c b/drivers/pwm/pwm-aspeed-ast2600.c
new file mode 100644
index 000000000000..ae6eb197e473
--- /dev/null
+++ b/drivers/pwm/pwm-aspeed-ast2600.c
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2021 Aspeed Technology Inc.
+ *
+ * PWM controller driver for Aspeed ast2600 SoCs.
+ * This drivers doesn't support earlier version of the IP.
+ *
+ * The hardware operates in time quantities of length
+ * Q := (DIV_L + 1) << DIV_H / input-clk
+ * The length of a PWM period is (DUTY_CYCLE_PERIOD + 1) * Q.
+ * The maximal value for DUTY_CYCLE_PERIOD is used here to provide
+ * a fine grained selection for the duty cycle.
+ *
+ * This driver uses DUTY_CYCLE_RISING_POINT = 0, so from the start of a
+ * period the output is active until DUTY_CYCLE_FALLING_POINT * Q. Note
+ * that if DUTY_CYCLE_RISING_POINT = DUTY_CYCLE_FALLING_POINT the output is
+ * always active.
+ *
+ * Register usage:
+ * PIN_ENABLE: When it is unset the pwm controller will emit inactive level to the external.
+ * Use to determine whether the PWM channel is enabled or disabled
+ * CLK_ENABLE: When it is unset the pwm controller will assert the duty counter reset and
+ * emit inactive level to the PIN_ENABLE mux after that the driver can still change the pwm period
+ * and duty and the value will apply when CLK_ENABLE be set again.
+ * Use to determine whether duty_cycle bigger than 0.
+ * PWM_ASPEED_CTRL_INVERSE: When it is toggled the output value will inverse immediately.
+ * PWM_ASPEED_DUTY_CYCLE_FALLING_POINT/PWM_ASPEED_DUTY_CYCLE_RISING_POINT: When these two
+ * values are equal it means the duty cycle = 100%.
+ *
+ * The glitch may generate at:
+ * - Enabled changing when the duty_cycle bigger than 0% and less than 100%.
+ * - Polarity changing when the duty_cycle bigger than 0% and less than 100%.
+ *
+ * Limitations:
+ * - When changing both duty cycle and period, we cannot prevent in
+ * software that the output might produce a period with mixed
+ * settings.
+ * - Disabling the PWM doesn't complete the current period.
+ *
+ * Improvements:
+ * - When only changing one of duty cycle or period, our pwm controller will not
+ * generate the glitch, the configure will change@next cycle of pwm.
+ * This improvement can disable/enable through PWM_ASPEED_CTRL_DUTY_SYNC_DISABLE.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/sysfs.h>
+
+/* PWM Control Register */
+#define PWM_ASPEED_CTRL (0x00)
+#define PWM_ASPEED_CTRL_LOAD_SEL_RISING_AS_WDT BIT(19)
+#define PWM_ASPEED_CTRL_DUTY_LOAD_AS_WDT_ENABLE BIT(18)
+#define PWM_ASPEED_CTRL_DUTY_SYNC_DISABLE BIT(17)
+#define PWM_ASPEED_CTRL_CLK_ENABLE BIT(16)
+#define PWM_ASPEED_CTRL_LEVEL_OUTPUT BIT(15)
+#define PWM_ASPEED_CTRL_INVERSE BIT(14)
+#define PWM_ASPEED_CTRL_OPEN_DRAIN_ENABLE BIT(13)
+#define PWM_ASPEED_CTRL_PIN_ENABLE BIT(12)
+#define PWM_ASPEED_CTRL_CLK_DIV_H GENMASK(11, 8)
+#define PWM_ASPEED_CTRL_CLK_DIV_L GENMASK(7, 0)
+
+/* PWM Duty Cycle Register */
+#define PWM_ASPEED_DUTY_CYCLE (0x04)
+#define PWM_ASPEED_DUTY_CYCLE_PERIOD GENMASK(31, 24)
+#define PWM_ASPEED_DUTY_CYCLE_POINT_AS_WDT GENMASK(23, 16)
+#define PWM_ASPEED_DUTY_CYCLE_FALLING_POINT GENMASK(15, 8)
+#define PWM_ASPEED_DUTY_CYCLE_RISING_POINT GENMASK(7, 0)
+
+/* PWM fixed value */
+#define PWM_ASPEED_FIXED_PERIOD FIELD_MAX(PWM_ASPEED_DUTY_CYCLE_PERIOD)
+
+struct aspeed_pwm_data {
+ struct pwm_chip chip;
+ struct clk *clk;
+ void __iomem *base;
+ struct reset_control *reset;
+ unsigned long clk_source;
+};
+
+static inline struct aspeed_pwm_data *
+aspeed_pwm_chip_to_data(struct pwm_chip *chip)
+{
+ return container_of(chip, struct aspeed_pwm_data, chip);
+}
+
+static int aspeed_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
+ bool polarity, pin_en, clk_en;
+ u32 duty_pt, val;
+ u64 div_h, div_l, duty_cycle_period, dividend;
+
+ val = readl(priv->base + PWM_ASPEED_CTRL);
+ polarity = FIELD_GET(PWM_ASPEED_CTRL_INVERSE, val);
+ pin_en = FIELD_GET(PWM_ASPEED_CTRL_PIN_ENABLE, val);
+ clk_en = FIELD_GET(PWM_ASPEED_CTRL_CLK_ENABLE, val);
+ div_h = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_H, val);
+ div_l = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_L, val);
+ val = readl(priv->base + PWM_ASPEED_DUTY_CYCLE);
+ duty_pt = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT, val);
+ duty_cycle_period = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_PERIOD, val);
+
+ /*
+ * This multiplication doesn't overflow, the upper bound is
+ * 1000000000 * 256 * 256 << 15 = 0x1dcd650000000000
+ */
+ dividend = (u64)NSEC_PER_SEC * (div_l + 1) * (duty_cycle_period + 1)
+ << div_h;
+ state->period = DIV_ROUND_UP_ULL(dividend, priv->clk_source);
+
+ if (clk_en && duty_pt) {
+ dividend = (u64)NSEC_PER_SEC * (div_l + 1) * duty_pt
+ << div_h;
+ state->duty_cycle =
+ DIV_ROUND_UP_ULL(dividend, priv->clk_source);
+ } else {
+ state->duty_cycle = clk_en ? state->period : 0;
+ }
+ state->polarity = polarity ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+ state->enabled = pin_en;
+ dev_dbg(dev, "get period: %lldns, duty_cycle: %lldns", state->period,
+ state->duty_cycle);
+ return 0;
+}
+
+static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct device *dev = chip->dev;
+ struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
+ u32 duty_pt;
+ u64 div_h, div_l, divisor, expect_period;
+ bool clk_en;
+
+ expect_period = min(div64_u64(ULLONG_MAX, (u64)priv->clk_source),
+ state->period);
+ dev_dbg(dev, "expect period: %lldns, duty_cycle: %lldns", expect_period,
+ state->duty_cycle);
+ /*
+ * Pick the smallest value for div_h so that div_l can be the biggest
+ * which results in a finer resolution near the target period value.
+ */
+ divisor = (u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1) *
+ (FIELD_MAX(PWM_ASPEED_CTRL_CLK_DIV_L) + 1);
+ div_h = order_base_2(DIV64_U64_ROUND_UP(priv->clk_source * expect_period, divisor));
+ if (div_h > 0xf)
+ div_h = 0xf;
+
+ divisor = ((u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1)) << div_h;
+ div_l = div64_u64(priv->clk_source * expect_period, divisor);
+
+ if (div_l == 0)
+ return -ERANGE;
+
+ div_l -= 1;
+
+ if (div_l > 255)
+ div_l = 255;
+
+ dev_dbg(dev, "clk source: %ld div_h %lld, div_l : %lld\n",
+ priv->clk_source, div_h, div_l);
+ /* duty_pt = duty_cycle * (PERIOD + 1) / period */
+ duty_pt = div64_u64(state->duty_cycle * priv->clk_source,
+ (u64)NSEC_PER_SEC * (div_l + 1) << div_h);
+ dev_dbg(dev, "duty_cycle = %lld, duty_pt = %d\n", state->duty_cycle,
+ duty_pt);
+
+ /*
+ * Fixed DUTY_CYCLE_PERIOD to its max value to get a
+ * fine-grained resolution for duty_cycle at the expense of a
+ * coarser period resolution.
+ */
+ writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
+ ~(PWM_ASPEED_DUTY_CYCLE_PERIOD)) |
+ FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_PERIOD,
+ PWM_ASPEED_FIXED_PERIOD),
+ priv->base + PWM_ASPEED_DUTY_CYCLE);
+
+ if (duty_pt == 0) {
+ /* emit inactive level and assert the duty counter reset */
+ clk_en = 0;
+ } else {
+ clk_en = 1;
+ if (duty_pt >= (PWM_ASPEED_FIXED_PERIOD + 1))
+ duty_pt = 0;
+ writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
+ ~(PWM_ASPEED_DUTY_CYCLE_RISING_POINT |
+ PWM_ASPEED_DUTY_CYCLE_FALLING_POINT)) |
+ FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT,
+ duty_pt),
+ priv->base + PWM_ASPEED_DUTY_CYCLE);
+ }
+
+ writel((readl(priv->base + PWM_ASPEED_CTRL) &
+ ~(PWM_ASPEED_CTRL_CLK_DIV_H | PWM_ASPEED_CTRL_CLK_DIV_L |
+ PWM_ASPEED_CTRL_PIN_ENABLE | PWM_ASPEED_CTRL_CLK_ENABLE |
+ PWM_ASPEED_CTRL_INVERSE)) |
+ FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_H, div_h) |
+ FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_L, div_l) |
+ FIELD_PREP(PWM_ASPEED_CTRL_PIN_ENABLE, state->enabled) |
+ FIELD_PREP(PWM_ASPEED_CTRL_CLK_ENABLE, clk_en) |
+ FIELD_PREP(PWM_ASPEED_CTRL_INVERSE, state->polarity),
+ priv->base + PWM_ASPEED_CTRL);
+
+ return 0;
+}
+
+static const struct pwm_ops aspeed_pwm_ops = {
+ .apply = aspeed_pwm_apply,
+ .get_state = aspeed_pwm_get_state,
+ .owner = THIS_MODULE,
+};
+
+static void aspeed_pwm_reset_assert(void *data)
+{
+ struct reset_control *rst = data;
+
+ reset_control_assert(rst);
+}
+
+static void aspeed_pwm_chip_remove(void *data)
+{
+ struct pwm_chip *chip = data;
+
+ pwmchip_remove(chip);
+}
+
+static int aspeed_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ int ret;
+ struct aspeed_pwm_data *priv;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ priv->clk = devm_clk_get_enabled(dev, NULL);
+ if (IS_ERR(priv->clk))
+ return dev_err_probe(dev, PTR_ERR(priv->clk),
+ "Couldn't get clock\n");
+ priv->clk_source = clk_get_rate(priv->clk);
+ priv->reset = devm_reset_control_get_shared(dev, NULL);
+ if (IS_ERR(priv->reset))
+ return dev_err_probe(dev, PTR_ERR(priv->reset),
+ "Couldn't get reset control\n");
+
+ ret = reset_control_deassert(priv->reset);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Couldn't deassert reset control\n");
+
+ ret = devm_add_action_or_reset(dev, aspeed_pwm_reset_assert,
+ priv->reset);
+ if (ret)
+ return ret;
+
+ priv->chip.dev = dev;
+ priv->chip.ops = &aspeed_pwm_ops;
+ priv->chip.npwm = 1;
+
+ ret = pwmchip_add(&priv->chip);
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
+ ret = devm_add_action_or_reset(dev, aspeed_pwm_chip_remove,
+ &priv->chip);
+ if (ret)
+ return ret;
+ return 0;
+}
+
+static const struct of_device_id of_pwm_match_table[] = {
+ {
+ .compatible = "aspeed,ast2600-pwm",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_pwm_match_table);
+
+static struct platform_driver aspeed_pwm_driver = {
+ .probe = aspeed_pwm_probe,
+ .driver = {
+ .name = "aspeed-pwm",
+ .of_match_table = of_pwm_match_table,
+ },
+};
+
+module_platform_driver(aspeed_pwm_driver);
+
+MODULE_AUTHOR("Billy Tsai <billy_tsai@aspeedtech.com>");
+MODULE_DESCRIPTION("Aspeed ast2600 PWM device driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [v6 3/4] pwm: Add Aspeed ast2600 PWM support
2023-06-08 2:18 ` [v6 3/4] pwm: Add Aspeed ast2600 PWM support Billy Tsai
@ 2023-07-18 7:08 ` Thierry Reding
0 siblings, 0 replies; 41+ messages in thread
From: Thierry Reding @ 2023-07-18 7:08 UTC (permalink / raw)
To: linux-aspeed
On Thu, Jun 08, 2023 at 10:18:38AM +0800, Billy Tsai wrote:
[...]
> diff --git a/drivers/pwm/pwm-aspeed-ast2600.c b/drivers/pwm/pwm-aspeed-ast2600.c
[...]
> +/* PWM Control Register */
> +#define PWM_ASPEED_CTRL (0x00)
[...]
> +#define PWM_ASPEED_DUTY_CYCLE (0x04)
Guenther already mentioned this, but these parentheses are unnecessary.
> +struct aspeed_pwm_data {
> + struct pwm_chip chip;
> + struct clk *clk;
> + void __iomem *base;
> + struct reset_control *reset;
> + unsigned long clk_source;
The name is a bit unfortunate. Looking at the code this represent the
rate of the parent clock, so something like clk_rate would be more
appropriate.
> +};
> +
> +static inline struct aspeed_pwm_data *
> +aspeed_pwm_chip_to_data(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct aspeed_pwm_data, chip);
> +}
> +
> +static int aspeed_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct device *dev = chip->dev;
You seem to use this exactly once, in a debug message, so having the
extra local variable seems a bit overkill. No strong objection, though.
> + struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
> + bool polarity, pin_en, clk_en;
A tab seems to have snuck in here.
> + u32 duty_pt, val;
> + u64 div_h, div_l, duty_cycle_period, dividend;
> +
> + val = readl(priv->base + PWM_ASPEED_CTRL);
> + polarity = FIELD_GET(PWM_ASPEED_CTRL_INVERSE, val);
> + pin_en = FIELD_GET(PWM_ASPEED_CTRL_PIN_ENABLE, val);
> + clk_en = FIELD_GET(PWM_ASPEED_CTRL_CLK_ENABLE, val);
> + div_h = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_H, val);
> + div_l = FIELD_GET(PWM_ASPEED_CTRL_CLK_DIV_L, val);
> + val = readl(priv->base + PWM_ASPEED_DUTY_CYCLE);
> + duty_pt = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT, val);
> + duty_cycle_period = FIELD_GET(PWM_ASPEED_DUTY_CYCLE_PERIOD, val);
> +
> + /*
> + * This multiplication doesn't overflow, the upper bound is
> + * 1000000000 * 256 * 256 << 15 = 0x1dcd650000000000
> + */
> + dividend = (u64)NSEC_PER_SEC * (div_l + 1) * (duty_cycle_period + 1)
> + << div_h;
> + state->period = DIV_ROUND_UP_ULL(dividend, priv->clk_source);
> +
> + if (clk_en && duty_pt) {
> + dividend = (u64)NSEC_PER_SEC * (div_l + 1) * duty_pt
> + << div_h;
> + state->duty_cycle =
> + DIV_ROUND_UP_ULL(dividend, priv->clk_source);
> + } else {
> + state->duty_cycle = clk_en ? state->period : 0;
> + }
> + state->polarity = polarity ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> + state->enabled = pin_en;
> + dev_dbg(dev, "get period: %lldns, duty_cycle: %lldns", state->period,
> + state->duty_cycle);
How likely are you to ever use this again? And how useful will that be?
We've got debugfs support that will show this information and more.
> + return 0;
> +}
> +
> +static int aspeed_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct device *dev = chip->dev;
> + struct aspeed_pwm_data *priv = aspeed_pwm_chip_to_data(chip);
> + u32 duty_pt;
> + u64 div_h, div_l, divisor, expect_period;
> + bool clk_en;
> +
> + expect_period = min(div64_u64(ULLONG_MAX, (u64)priv->clk_source),
> + state->period);
> + dev_dbg(dev, "expect period: %lldns, duty_cycle: %lldns", expect_period,
> + state->duty_cycle);
> + /*
> + * Pick the smallest value for div_h so that div_l can be the biggest
> + * which results in a finer resolution near the target period value.
> + */
> + divisor = (u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1) *
> + (FIELD_MAX(PWM_ASPEED_CTRL_CLK_DIV_L) + 1);
> + div_h = order_base_2(DIV64_U64_ROUND_UP(priv->clk_source * expect_period, divisor));
> + if (div_h > 0xf)
> + div_h = 0xf;
> +
> + divisor = ((u64)NSEC_PER_SEC * (PWM_ASPEED_FIXED_PERIOD + 1)) << div_h;
> + div_l = div64_u64(priv->clk_source * expect_period, divisor);
> +
> + if (div_l == 0)
> + return -ERANGE;
> +
> + div_l -= 1;
> +
> + if (div_l > 255)
> + div_l = 255;
> +
> + dev_dbg(dev, "clk source: %ld div_h %lld, div_l : %lld\n",
> + priv->clk_source, div_h, div_l);
> + /* duty_pt = duty_cycle * (PERIOD + 1) / period */
> + duty_pt = div64_u64(state->duty_cycle * priv->clk_source,
> + (u64)NSEC_PER_SEC * (div_l + 1) << div_h);
> + dev_dbg(dev, "duty_cycle = %lld, duty_pt = %d\n", state->duty_cycle,
> + duty_pt);
> +
> + /*
> + * Fixed DUTY_CYCLE_PERIOD to its max value to get a
> + * fine-grained resolution for duty_cycle at the expense of a
> + * coarser period resolution.
> + */
> + writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
> + ~(PWM_ASPEED_DUTY_CYCLE_PERIOD)) |
> + FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_PERIOD,
> + PWM_ASPEED_FIXED_PERIOD),
> + priv->base + PWM_ASPEED_DUTY_CYCLE);
This is completely unreadable. Use a temporary variable to split this
up.
> +
> + if (duty_pt == 0) {
> + /* emit inactive level and assert the duty counter reset */
> + clk_en = 0;
> + } else {
> + clk_en = 1;
> + if (duty_pt >= (PWM_ASPEED_FIXED_PERIOD + 1))
> + duty_pt = 0;
> + writel((readl(priv->base + PWM_ASPEED_DUTY_CYCLE) &
> + ~(PWM_ASPEED_DUTY_CYCLE_RISING_POINT |
> + PWM_ASPEED_DUTY_CYCLE_FALLING_POINT)) |
> + FIELD_PREP(PWM_ASPEED_DUTY_CYCLE_FALLING_POINT,
> + duty_pt),
> + priv->base + PWM_ASPEED_DUTY_CYCLE);
Same here ...
> + }
> +
> + writel((readl(priv->base + PWM_ASPEED_CTRL) &
> + ~(PWM_ASPEED_CTRL_CLK_DIV_H | PWM_ASPEED_CTRL_CLK_DIV_L |
> + PWM_ASPEED_CTRL_PIN_ENABLE | PWM_ASPEED_CTRL_CLK_ENABLE |
> + PWM_ASPEED_CTRL_INVERSE)) |
> + FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_H, div_h) |
> + FIELD_PREP(PWM_ASPEED_CTRL_CLK_DIV_L, div_l) |
> + FIELD_PREP(PWM_ASPEED_CTRL_PIN_ENABLE, state->enabled) |
> + FIELD_PREP(PWM_ASPEED_CTRL_CLK_ENABLE, clk_en) |
> + FIELD_PREP(PWM_ASPEED_CTRL_INVERSE, state->polarity),
> + priv->base + PWM_ASPEED_CTRL);
... and here.
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops aspeed_pwm_ops = {
> + .apply = aspeed_pwm_apply,
> + .get_state = aspeed_pwm_get_state,
> + .owner = THIS_MODULE,
> +};
> +
> +static void aspeed_pwm_reset_assert(void *data)
> +{
> + struct reset_control *rst = data;
> +
> + reset_control_assert(rst);
> +}
> +
> +static void aspeed_pwm_chip_remove(void *data)
> +{
> + struct pwm_chip *chip = data;
> +
> + pwmchip_remove(chip);
> +}
Erm... no.
> +static int aspeed_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + int ret;
> + struct aspeed_pwm_data *priv;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + priv->clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(priv->clk))
> + return dev_err_probe(dev, PTR_ERR(priv->clk),
> + "Couldn't get clock\n");
> + priv->clk_source = clk_get_rate(priv->clk);
> + priv->reset = devm_reset_control_get_shared(dev, NULL);
> + if (IS_ERR(priv->reset))
> + return dev_err_probe(dev, PTR_ERR(priv->reset),
> + "Couldn't get reset control\n");
> +
> + ret = reset_control_deassert(priv->reset);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Couldn't deassert reset control\n");
> +
> + ret = devm_add_action_or_reset(dev, aspeed_pwm_reset_assert,
> + priv->reset);
> + if (ret)
> + return ret;
So now you need that extra callback that you defined earlier plus these
four lines of code in order to ...
> +
> + priv->chip.dev = dev;
> + priv->chip.ops = &aspeed_pwm_ops;
> + priv->chip.npwm = 1;
> +
> + ret = pwmchip_add(&priv->chip);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
... avoid calling reset_control_assert() once here? These device-
managed functions are meant to help simplify things, but there's nothing
complicated about it in this driver, so don't do it.
> + ret = devm_add_action_or_reset(dev, aspeed_pwm_chip_remove,
> + &priv->chip);
> + if (ret)
> + return ret;
Why not just use the driver's .remove() callback? There's nothing here
that would fail afterwards, so this will effectively get called only
during driver removal, so might as well use the idiomatic infrastructure
that exists for this.
> + return 0;
> +}
> +
> +static const struct of_device_id of_pwm_match_table[] = {
That's a suboptimal name. Use a driver-specific prefix.
> + {
> + .compatible = "aspeed,ast2600-pwm",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_match_table);
> +
> +static struct platform_driver aspeed_pwm_driver = {
> + .probe = aspeed_pwm_probe,
> + .driver = {
There's another tab that doesn't belong here.
Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20230718/97a7f270/attachment.sig>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 4/4] hwmon: Add Aspeed ast2600 TACH support
2023-06-08 2:18 [v6 0/4] Support pwm/tach driver for aspeed ast26xx Billy Tsai
` (2 preceding siblings ...)
2023-06-08 2:18 ` [v6 3/4] pwm: Add Aspeed ast2600 PWM support Billy Tsai
@ 2023-06-08 2:18 ` Billy Tsai
2023-06-08 5:01 ` Guenter Roeck
2023-06-08 6:37 ` [v6 0/4] Support pwm/tach driver for aspeed ast26xx Krzysztof Kozlowski
4 siblings, 1 reply; 41+ messages in thread
From: Billy Tsai @ 2023-06-08 2:18 UTC (permalink / raw)
To: linux-aspeed
Add the support of Tachometer which can use to monitor the frequency of
the input. In Aspeed AST2600 SoC features 16 TACH controllers, with each
controller capable of supporting up to 1 input.
Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/tach-aspeed-ast2600.rst | 25 ++
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/tach-aspeed-ast2600.c | 305 ++++++++++++++++++++
5 files changed, 342 insertions(+)
create mode 100644 Documentation/hwmon/tach-aspeed-ast2600.rst
create mode 100644 drivers/hwmon/tach-aspeed-ast2600.c
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index ddff3c5713d7..4c3dd74675ef 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -194,6 +194,7 @@ Hardware Monitoring Kernel Drivers
sparx5-temp
stpddc60
sy7636a-hwmon
+ tach-aspeed-ast2600
tc654
tc74
thmc50
diff --git a/Documentation/hwmon/tach-aspeed-ast2600.rst b/Documentation/hwmon/tach-aspeed-ast2600.rst
new file mode 100644
index 000000000000..b08c73a4237f
--- /dev/null
+++ b/Documentation/hwmon/tach-aspeed-ast2600.rst
@@ -0,0 +1,25 @@
+Kernel driver tach-aspeed-ast2600
+=================================
+
+Supported chips:
+ ASPEED AST2600
+
+Authors:
+ <billy_tsai@aspeedtech.com>
+
+Description:
+------------
+This driver implements support for ASPEED AST2600 Fan Tacho controller.
+The controller supports up to 1 tachometer inputs.
+
+The driver provides the following sensor accesses in sysfs:
+
+=============== ======= ======================================================
+fanX_input ro provide current fan rotation value in RPM as reported
+ by the fan to the device.
+fanX_div rw Fan divisor: Supported value are power of 4 (1, 4, 16
+ 64, ... 4194304)
+ The larger divisor, the less rpm accuracy and the less
+ affected by fan signal glitch.
+fanX_pulses rw Fan pulses per resolution.
+=============== ======= ======================================================
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index a5253abb7ea7..5948a63e44e7 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -411,6 +411,16 @@ config SENSORS_ASPEED
This driver can also be built as a module. If so, the module
will be called aspeed_pwm_tacho.
+config SENSORS_TACH_ASPEED_AST2600
+ tristate "ASPEED ast2600 Tachometer support"
+ depends on ARCH_ASPEED || COMPILE_TEST
+ depends on HAVE_CLK && HAS_IOMEM
+ help
+ This driver provides support for Aspeed ast2600 Tachometer.
+
+ To compile this driver as a module, choose M here: the module
+ will be called tach-aspeed-ast2600.
+
config SENSORS_ATXP1
tristate "Attansic ATXP1 VID controller"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index c5cd7e3a67ff..a3bf5b438e0f 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o
obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o
obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o
+obj-$(CONFIG_SENSORS_TACH_ASPEED_AST2600) += tach-aspeed-ast2600.o
obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o
obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o
obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
diff --git a/drivers/hwmon/tach-aspeed-ast2600.c b/drivers/hwmon/tach-aspeed-ast2600.c
new file mode 100644
index 000000000000..8be66ee25a31
--- /dev/null
+++ b/drivers/hwmon/tach-aspeed-ast2600.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) ASPEED Technology Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/hwmon.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/sysfs.h>
+
+/* TACH Control Register */
+#define TACH_ASPEED_CTRL (0x00)
+#define TACH_ASPEED_IER BIT(31)
+#define TACH_ASPEED_INVERS_LIMIT BIT(30)
+#define TACH_ASPEED_LOOPBACK BIT(29)
+#define TACH_ASPEED_ENABLE BIT(28)
+#define TACH_ASPEED_DEBOUNCE_MASK GENMASK(27, 26)
+#define TACH_ASPEED_DEBOUNCE_BIT 26
+#define TACH_ASPEED_IO_EDGE_MASK GENMASK(25, 24)
+#define TACH_ASPEED_IO_EDGE_BIT 24
+#define TACH_ASPEED_CLK_DIV_T_MASK GENMASK(23, 20)
+#define TACH_ASPEED_CLK_DIV_BIT 20
+#define TACH_ASPEED_THRESHOLD_MASK GENMASK(19, 0)
+/* [27:26] */
+#define DEBOUNCE_3_CLK 0x00
+#define DEBOUNCE_2_CLK 0x01
+#define DEBOUNCE_1_CLK 0x02
+#define DEBOUNCE_0_CLK 0x03
+/* [25:24] */
+#define F2F_EDGES 0x00
+#define R2R_EDGES 0x01
+#define BOTH_EDGES 0x02
+/* [23:20] */
+/* divisor = 4 to the nth power, n = register value */
+#define DEFAULT_TACH_DIV 1024
+#define DIV_TO_REG(divisor) (ilog2(divisor) >> 1)
+
+/* TACH Status Register */
+#define TACH_ASPEED_STS (0x04)
+
+/*PWM_TACH_STS */
+#define TACH_ASPEED_ISR BIT(31)
+#define TACH_ASPEED_PWM_OUT BIT(25)
+#define TACH_ASPEED_PWM_OEN BIT(24)
+#define TACH_ASPEED_DEB_INPUT BIT(23)
+#define TACH_ASPEED_RAW_INPUT BIT(22)
+#define TACH_ASPEED_VALUE_UPDATE BIT(21)
+#define TACH_ASPEED_FULL_MEASUREMENT BIT(20)
+#define TACH_ASPEED_VALUE_MASK GENMASK(19, 0)
+/**********************************************************
+ * Software setting
+ *********************************************************/
+#define DEFAULT_FAN_PULSE_PR 2
+
+struct aspeed_tach_channel_params {
+ u8 pulse_pr;
+ u32 divisor;
+};
+
+struct aspeed_tach_data {
+ struct device *dev;
+ void __iomem *base;
+ struct clk *clk;
+ struct reset_control *reset;
+ bool tach_present;
+ struct aspeed_tach_channel_params tach_channel;
+ unsigned long clk_source;
+};
+
+static void aspeed_tach_ch_enable(struct aspeed_tach_data *priv, bool enable)
+{
+ if (enable)
+ writel(readl(priv->base + TACH_ASPEED_CTRL) |
+ (TACH_ASPEED_ENABLE),
+ priv->base + TACH_ASPEED_CTRL);
+ else
+ writel(readl(priv->base + TACH_ASPEED_CTRL) &
+ ~(TACH_ASPEED_ENABLE),
+ priv->base + TACH_ASPEED_CTRL);
+}
+
+static u64 aspeed_tach_val_to_rpm(struct aspeed_tach_data *priv, u32 tach_val)
+{
+ u64 rpm;
+ u32 tach_div;
+
+ tach_div = tach_val * (priv->tach_channel.divisor) *
+ (priv->tach_channel.pulse_pr);
+
+ dev_dbg(priv->dev, "clk %ld, tach_val %d , tach_div %d\n",
+ priv->clk_source, tach_val, tach_div);
+
+ rpm = (u64)priv->clk_source * 60;
+ do_div(rpm, tach_div);
+
+ return rpm;
+}
+
+static int aspeed_get_fan_tach_ch_rpm(struct aspeed_tach_data *priv)
+{
+ u32 val;
+ u64 rpm;
+
+ val = readl(priv->base + TACH_ASPEED_STS);
+
+ if (!(val & TACH_ASPEED_FULL_MEASUREMENT))
+ return 0;
+ rpm = aspeed_tach_val_to_rpm(priv, val & TACH_ASPEED_VALUE_MASK);
+
+ return rpm;
+}
+
+static int aspeed_tach_hwmon_read(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long *val)
+{
+ struct aspeed_tach_data *priv = dev_get_drvdata(dev);
+ u32 reg_val;
+ int ret;
+
+ switch (attr) {
+ case hwmon_fan_input:
+ ret = aspeed_get_fan_tach_ch_rpm(priv);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ break;
+ case hwmon_fan_div:
+ reg_val = readl(priv->base + TACH_ASPEED_CTRL);
+ reg_val = FIELD_GET(TACH_ASPEED_CLK_DIV_T_MASK, reg_val);
+ *val = BIT(reg_val << 1);
+ break;
+ case hwmon_fan_pulses:
+ *val = priv->tach_channel.pulse_pr;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+ return 0;
+}
+
+static int aspeed_tach_hwmon_write(struct device *dev,
+ enum hwmon_sensor_types type, u32 attr,
+ int channel, long val)
+{
+ struct aspeed_tach_data *priv = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_fan_div:
+ if (!is_power_of_2(val) || (ilog2(val) % 2))
+ return -EINVAL;
+ else if (DIV_TO_REG(val) > 0xb)
+ return -ERANGE;
+ priv->tach_channel.divisor = val;
+ writel((readl(priv->base + TACH_ASPEED_CTRL) &
+ ~(TACH_ASPEED_CLK_DIV_T_MASK)) |
+ (DIV_TO_REG(priv->tach_channel.divisor)
+ << TACH_ASPEED_CLK_DIV_BIT),
+ priv->base + TACH_ASPEED_CTRL);
+ break;
+ case hwmon_fan_pulses:
+ priv->tach_channel.pulse_pr = val;
+ break;
+ default:
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static umode_t aspeed_tach_dev_is_visible(const void *drvdata,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ const struct aspeed_tach_data *priv = drvdata;
+
+ if (!priv->tach_present)
+ return 0;
+ switch (attr) {
+ case hwmon_fan_input:
+ return 0444;
+ case hwmon_fan_div:
+ case hwmon_fan_pulses:
+ return 0644;
+ }
+ return 0;
+}
+
+static const struct hwmon_ops aspeed_tach_ops = {
+ .is_visible = aspeed_tach_dev_is_visible,
+ .read = aspeed_tach_hwmon_read,
+ .write = aspeed_tach_hwmon_write,
+};
+
+static const struct hwmon_channel_info *aspeed_tach_info[] = {
+ HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_PULSES),
+ NULL
+};
+
+static const struct hwmon_chip_info aspeed_tach_chip_info = {
+ .ops = &aspeed_tach_ops,
+ .info = aspeed_tach_info,
+};
+
+static void aspeed_present_fan_tach(struct aspeed_tach_data *priv)
+{
+ priv->tach_present = true;
+ priv->tach_channel.divisor = DEFAULT_TACH_DIV;
+ priv->tach_channel.pulse_pr = DEFAULT_FAN_PULSE_PR;
+
+ writel((readl(priv->base + TACH_ASPEED_CTRL) &
+ ~(TACH_ASPEED_INVERS_LIMIT | TACH_ASPEED_DEBOUNCE_MASK |
+ TACH_ASPEED_IO_EDGE_MASK | TACH_ASPEED_CLK_DIV_T_MASK |
+ TACH_ASPEED_THRESHOLD_MASK)) |
+ ((DEBOUNCE_3_CLK << TACH_ASPEED_DEBOUNCE_BIT) |
+ F2F_EDGES |
+ (DIV_TO_REG(priv->tach_channel.divisor)
+ << TACH_ASPEED_CLK_DIV_BIT)),
+ priv->base + TACH_ASPEED_CTRL);
+
+ aspeed_tach_ch_enable(priv, true);
+}
+
+static void aspeed_tach_reset_assert(void *data)
+{
+ struct reset_control *rst = data;
+
+ reset_control_assert(rst);
+}
+
+static int aspeed_tach_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct aspeed_tach_data *priv;
+ struct device *hwmon;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+ priv->dev = &pdev->dev;
+ priv->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+ priv->clk = devm_clk_get_enabled(dev, NULL);
+ if (IS_ERR(priv->clk))
+ return dev_err_probe(dev, PTR_ERR(priv->clk),
+ "Couldn't get clock\n");
+
+ priv->clk_source = clk_get_rate(priv->clk);
+ priv->reset = devm_reset_control_get_shared(dev, NULL);
+ if (IS_ERR(priv->reset))
+ return dev_err_probe(dev, PTR_ERR(priv->reset),
+ "Couldn't get reset control\n");
+
+ ret = reset_control_deassert(priv->reset);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Couldn't deassert reset control\n");
+
+ ret = devm_add_action_or_reset(dev, aspeed_tach_reset_assert,
+ priv->reset);
+ if (ret)
+ return ret;
+
+ aspeed_present_fan_tach(priv);
+
+ hwmon = devm_hwmon_device_register_with_info(dev, "aspeed_tach", priv,
+ &aspeed_tach_chip_info, NULL);
+ ret = PTR_ERR_OR_ZERO(hwmon);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to register hwmon device\n");
+ return 0;
+}
+
+static const struct of_device_id of_stach_match_table[] = {
+ {
+ .compatible = "aspeed,ast2600-tach",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, of_stach_match_table);
+
+static struct platform_driver aspeed_tach_driver = {
+ .probe = aspeed_tach_probe,
+ .driver = {
+ .name = "aspeed_tach",
+ .of_match_table = of_stach_match_table,
+ },
+};
+
+module_platform_driver(aspeed_tach_driver);
+
+MODULE_AUTHOR("Billy Tsai <billy_tsai@aspeedtech.com>");
+MODULE_DESCRIPTION("Aspeed ast2600 TACH device driver");
+MODULE_LICENSE("GPL");
--
2.25.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* [v6 4/4] hwmon: Add Aspeed ast2600 TACH support
2023-06-08 2:18 ` [v6 4/4] hwmon: Add Aspeed ast2600 TACH support Billy Tsai
@ 2023-06-08 5:01 ` Guenter Roeck
0 siblings, 0 replies; 41+ messages in thread
From: Guenter Roeck @ 2023-06-08 5:01 UTC (permalink / raw)
To: linux-aspeed
On 6/7/23 19:18, Billy Tsai wrote:
> Add the support of Tachometer which can use to monitor the frequency of
> the input. In Aspeed AST2600 SoC features 16 TACH controllers, with each
> controller capable of supporting up to 1 input.
>
You mean there will be 16 instances of this driver ? That is weird,
confusing, and warrants a detailed explanation.
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
> Documentation/hwmon/index.rst | 1 +
> Documentation/hwmon/tach-aspeed-ast2600.rst | 25 ++
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/tach-aspeed-ast2600.c | 305 ++++++++++++++++++++
> 5 files changed, 342 insertions(+)
> create mode 100644 Documentation/hwmon/tach-aspeed-ast2600.rst
> create mode 100644 drivers/hwmon/tach-aspeed-ast2600.c
>
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index ddff3c5713d7..4c3dd74675ef 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -194,6 +194,7 @@ Hardware Monitoring Kernel Drivers
> sparx5-temp
> stpddc60
> sy7636a-hwmon
> + tach-aspeed-ast2600
> tc654
> tc74
> thmc50
> diff --git a/Documentation/hwmon/tach-aspeed-ast2600.rst b/Documentation/hwmon/tach-aspeed-ast2600.rst
> new file mode 100644
> index 000000000000..b08c73a4237f
> --- /dev/null
> +++ b/Documentation/hwmon/tach-aspeed-ast2600.rst
> @@ -0,0 +1,25 @@
> +Kernel driver tach-aspeed-ast2600
> +=================================
> +
> +Supported chips:
> + ASPEED AST2600
> +
> +Authors:
> + <billy_tsai@aspeedtech.com>
> +
> +Description:
> +------------
> +This driver implements support for ASPEED AST2600 Fan Tacho controller.
> +The controller supports up to 1 tachometer inputs.
> +
> +The driver provides the following sensor accesses in sysfs:
> +
> +=============== ======= ======================================================
> +fanX_input ro provide current fan rotation value in RPM as reported
> + by the fan to the device.
> +fanX_div rw Fan divisor: Supported value are power of 4 (1, 4, 16
> + 64, ... 4194304)
> + The larger divisor, the less rpm accuracy and the less
> + affected by fan signal glitch.
> +fanX_pulses rw Fan pulses per resolution.
Presumably, as implemented, that would be 16 instances, each with fan1_{input,div}.
> +=============== ======= ======================================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index a5253abb7ea7..5948a63e44e7 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -411,6 +411,16 @@ config SENSORS_ASPEED
> This driver can also be built as a module. If so, the module
> will be called aspeed_pwm_tacho.
>
> +config SENSORS_TACH_ASPEED_AST2600
> + tristate "ASPEED ast2600 Tachometer support"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + depends on HAVE_CLK && HAS_IOMEM
> + help
> + This driver provides support for Aspeed ast2600 Tachometer.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called tach-aspeed-ast2600.
> +
> config SENSORS_ATXP1
> tristate "Attansic ATXP1 VID controller"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index c5cd7e3a67ff..a3bf5b438e0f 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -53,6 +53,7 @@ obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o
> obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
> obj-$(CONFIG_SENSORS_AS370) += as370-hwmon.o
> obj-$(CONFIG_SENSORS_ASC7621) += asc7621.o
> +obj-$(CONFIG_SENSORS_TACH_ASPEED_AST2600) += tach-aspeed-ast2600.o
> obj-$(CONFIG_SENSORS_ASPEED) += aspeed-pwm-tacho.o
> obj-$(CONFIG_SENSORS_ATXP1) += atxp1.o
> obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> diff --git a/drivers/hwmon/tach-aspeed-ast2600.c b/drivers/hwmon/tach-aspeed-ast2600.c
> new file mode 100644
> index 000000000000..8be66ee25a31
> --- /dev/null
> +++ b/drivers/hwmon/tach-aspeed-ast2600.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) ASPEED Technology Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/sysfs.h>
> +
> +/* TACH Control Register */
> +#define TACH_ASPEED_CTRL (0x00)
> +#define TACH_ASPEED_IER BIT(31)
> +#define TACH_ASPEED_INVERS_LIMIT BIT(30)
> +#define TACH_ASPEED_LOOPBACK BIT(29)
> +#define TACH_ASPEED_ENABLE BIT(28)
> +#define TACH_ASPEED_DEBOUNCE_MASK GENMASK(27, 26)
> +#define TACH_ASPEED_DEBOUNCE_BIT 26
> +#define TACH_ASPEED_IO_EDGE_MASK GENMASK(25, 24)
> +#define TACH_ASPEED_IO_EDGE_BIT 24
> +#define TACH_ASPEED_CLK_DIV_T_MASK GENMASK(23, 20)
> +#define TACH_ASPEED_CLK_DIV_BIT 20
> +#define TACH_ASPEED_THRESHOLD_MASK GENMASK(19, 0)
> +/* [27:26] */
> +#define DEBOUNCE_3_CLK 0x00
> +#define DEBOUNCE_2_CLK 0x01
> +#define DEBOUNCE_1_CLK 0x02
> +#define DEBOUNCE_0_CLK 0x03
> +/* [25:24] */
> +#define F2F_EDGES 0x00
> +#define R2R_EDGES 0x01
> +#define BOTH_EDGES 0x02
> +/* [23:20] */
> +/* divisor = 4 to the nth power, n = register value */
> +#define DEFAULT_TACH_DIV 1024
> +#define DIV_TO_REG(divisor) (ilog2(divisor) >> 1)
> +
> +/* TACH Status Register */
> +#define TACH_ASPEED_STS (0x04)
Unnecesarry ()
> +
> +/*PWM_TACH_STS */
> +#define TACH_ASPEED_ISR BIT(31)
> +#define TACH_ASPEED_PWM_OUT BIT(25)
> +#define TACH_ASPEED_PWM_OEN BIT(24)
> +#define TACH_ASPEED_DEB_INPUT BIT(23)
> +#define TACH_ASPEED_RAW_INPUT BIT(22)
> +#define TACH_ASPEED_VALUE_UPDATE BIT(21)
> +#define TACH_ASPEED_FULL_MEASUREMENT BIT(20)
> +#define TACH_ASPEED_VALUE_MASK GENMASK(19, 0)
> +/**********************************************************
> + * Software setting
> + *********************************************************/
> +#define DEFAULT_FAN_PULSE_PR 2
> +
> +struct aspeed_tach_channel_params {
> + u8 pulse_pr;
> + u32 divisor;
> +};
> +
> +struct aspeed_tach_data {
> + struct device *dev;
> + void __iomem *base;
> + struct clk *clk;
> + struct reset_control *reset;
> + bool tach_present;
> + struct aspeed_tach_channel_params tach_channel;
Assuming that all channels have the same divisor, this structure
is really unnecessary (especially since pulse_pr should be dropped -
see below).
> + unsigned long clk_source;
> +};
> +
> +static void aspeed_tach_ch_enable(struct aspeed_tach_data *priv, bool enable)
> +{
> + if (enable)
> + writel(readl(priv->base + TACH_ASPEED_CTRL) |
> + (TACH_ASPEED_ENABLE),
> + priv->base + TACH_ASPEED_CTRL);
> + else
> + writel(readl(priv->base + TACH_ASPEED_CTRL) &
> + ~(TACH_ASPEED_ENABLE),
Unnecessary () around TACH_ASPEED_ENABLE.
> + priv->base + TACH_ASPEED_CTRL);
> +}
> +
> +static u64 aspeed_tach_val_to_rpm(struct aspeed_tach_data *priv, u32 tach_val)
> +{
> + u64 rpm;
> + u32 tach_div;
> +
> + tach_div = tach_val * (priv->tach_channel.divisor) *
> + (priv->tach.channel.pulse_pr);
> +
Unnecessary () around priv->tach_channel.pulse_pr and priv->tach_channel.divisor.
I am not going to look further for those. Please make sure you drop them all.
> + dev_dbg(priv->dev, "clk %ld, tach_val %d , tach_div %d\n",
> + priv->clk_source, tach_val, tach_div);
> +
> + rpm = (u64)priv->clk_source * 60;
> + do_div(rpm, tach_div);
> +
> + return rpm;
This function should really return an integer.
> +}
> +
> +static int aspeed_get_fan_tach_ch_rpm(struct aspeed_tach_data *priv)
> +{
> + u32 val;
> + u64 rpm;
> +
> + val = readl(priv->base + TACH_ASPEED_STS);
> +
> + if (!(val & TACH_ASPEED_FULL_MEASUREMENT))
> + return 0;
> + rpm = aspeed_tach_val_to_rpm(priv, val & TACH_ASPEED_VALUE_MASK);
> +
> + return rpm;
The 'rpm' variable is unnecessary.
> +}
> +
> +static int aspeed_tach_hwmon_read(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct aspeed_tach_data *priv = dev_get_drvdata(dev);
> + u32 reg_val;
> + int ret;
> +
> + switch (attr) {
> + case hwmon_fan_input:
> + ret = aspeed_get_fan_tach_ch_rpm(priv);
> + if (ret < 0)
> + return ret;
Why ? The called code never returns a negative value.
> + *val = ret;
> + break;
> + case hwmon_fan_div:
> + reg_val = readl(priv->base + TACH_ASPEED_CTRL);
> + reg_val = FIELD_GET(TACH_ASPEED_CLK_DIV_T_MASK, reg_val);
> + *val = BIT(reg_val << 1);
> + break;
> + case hwmon_fan_pulses:
> + *val = priv->tach_channel.pulse_pr;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> + return 0;
> +}
> +
> +static int aspeed_tach_hwmon_write(struct device *dev,
> + enum hwmon_sensor_types type, u32 attr,
> + int channel, long val)
> +{
> + struct aspeed_tach_data *priv = dev_get_drvdata(dev);
> +
> + switch (attr) {
> + case hwmon_fan_div:
> + if (!is_power_of_2(val) || (ilog2(val) % 2))
> + return -EINVAL;
> + else if (DIV_TO_REG(val) > 0xb)
Else after return is unnecessary
> + return -ERANGE;
Math result not representable ? Not really.
> + priv->tach_channel.divisor = val;
> + writel((readl(priv->base + TACH_ASPEED_CTRL) &
> + ~(TACH_ASPEED_CLK_DIV_T_MASK)) |
Unnecessary () around TACH_ASPEED_CLK_DIV_T_MASK.
> + (DIV_TO_REG(priv->tach_channel.divisor)
> + << TACH_ASPEED_CLK_DIV_BIT),
> + priv->base + TACH_ASPEED_CTRL);
> + break;
> + case hwmon_fan_pulses:
> + priv->tach_channel.pulse_pr = val;
The ABI says for the pulses atribute:
Should only be created if the chip has a register to configure
the number of pulses. In the absence of such a register (and
thus attribute) the value assumed by all devices is 2 pulses
per fan revolution.
I see no evidence that there is a chip register for the number of pulses.
Please drop this attribute.
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static umode_t aspeed_tach_dev_is_visible(const void *drvdata,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + const struct aspeed_tach_data *priv = drvdata;
> +
> + if (!priv->tach_present)
> + return 0;
What is the point of instantiating the driver in this case ?
> + switch (attr) {
> + case hwmon_fan_input:
> + return 0444;
> + case hwmon_fan_div:
> + case hwmon_fan_pulses:
> + return 0644;
> + }
> + return 0;
> +}
> +
> +static const struct hwmon_ops aspeed_tach_ops = {
> + .is_visible = aspeed_tach_dev_is_visible,
> + .read = aspeed_tach_hwmon_read,
> + .write = aspeed_tach_hwmon_write,
> +};
> +
> +static const struct hwmon_channel_info *aspeed_tach_info[] = {
> + HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_DIV | HWMON_F_PULSES),
> + NULL
> +};
> +
> +static const struct hwmon_chip_info aspeed_tach_chip_info = {
> + .ops = &aspeed_tach_ops,
> + .info = aspeed_tach_info,
> +};
> +
> +static void aspeed_present_fan_tach(struct aspeed_tach_data *priv)
> +{
> + priv->tach_present = true;
> + priv->tach_channel.divisor = DEFAULT_TACH_DIV;
> + priv->tach_channel.pulse_pr = DEFAULT_FAN_PULSE_PR;
> +
> + writel((readl(priv->base + TACH_ASPEED_CTRL) &
> + ~(TACH_ASPEED_INVERS_LIMIT | TACH_ASPEED_DEBOUNCE_MASK |
> + TACH_ASPEED_IO_EDGE_MASK | TACH_ASPEED_CLK_DIV_T_MASK |
> + TACH_ASPEED_THRESHOLD_MASK)) |
> + ((DEBOUNCE_3_CLK << TACH_ASPEED_DEBOUNCE_BIT) |
> + F2F_EDGES |
> + (DIV_TO_REG(priv->tach_channel.divisor)
> + << TACH_ASPEED_CLK_DIV_BIT)),
> + priv->base + TACH_ASPEED_CTRL);
> +
> + aspeed_tach_ch_enable(priv, true);
> +}
> +
> +static void aspeed_tach_reset_assert(void *data)
> +{
> + struct reset_control *rst = data;
> +
> + reset_control_assert(rst);
> +}
> +
> +static int aspeed_tach_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct aspeed_tach_data *priv;
> + struct device *hwmon;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> + priv->dev = &pdev->dev;
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> + priv->clk = devm_clk_get_enabled(dev, NULL);
> + if (IS_ERR(priv->clk))
> + return dev_err_probe(dev, PTR_ERR(priv->clk),
> + "Couldn't get clock\n");
> +
> + priv->clk_source = clk_get_rate(priv->clk);
> + priv->reset = devm_reset_control_get_shared(dev, NULL);
> + if (IS_ERR(priv->reset))
> + return dev_err_probe(dev, PTR_ERR(priv->reset),
> + "Couldn't get reset control\n");
> +
> + ret = reset_control_deassert(priv->reset);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Couldn't deassert reset control\n");
> +
> + ret = devm_add_action_or_reset(dev, aspeed_tach_reset_assert,
> + priv->reset);
> + if (ret)
> + return ret;
> +
> + aspeed_present_fan_tach(priv);
> +
> + hwmon = devm_hwmon_device_register_with_info(dev, "aspeed_tach", priv,
> + &aspeed_tach_chip_info, NULL);
> + ret = PTR_ERR_OR_ZERO(hwmon);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to register hwmon device\n");
> + return 0;
> +}
> +
> +static const struct of_device_id of_stach_match_table[] = {
> + {
> + .compatible = "aspeed,ast2600-tach",
> + },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, of_stach_match_table);
> +
> +static struct platform_driver aspeed_tach_driver = {
> + .probe = aspeed_tach_probe,
> + .driver = {
> + .name = "aspeed_tach",
> + .of_match_table = of_stach_match_table,
> + },
> +};
> +
> +module_platform_driver(aspeed_tach_driver);
> +
> +MODULE_AUTHOR("Billy Tsai <billy_tsai@aspeedtech.com>");
> +MODULE_DESCRIPTION("Aspeed ast2600 TACH device driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 41+ messages in thread
* [v6 0/4] Support pwm/tach driver for aspeed ast26xx
2023-06-08 2:18 [v6 0/4] Support pwm/tach driver for aspeed ast26xx Billy Tsai
` (3 preceding siblings ...)
2023-06-08 2:18 ` [v6 4/4] hwmon: Add Aspeed ast2600 TACH support Billy Tsai
@ 2023-06-08 6:37 ` Krzysztof Kozlowski
4 siblings, 0 replies; 41+ messages in thread
From: Krzysztof Kozlowski @ 2023-06-08 6:37 UTC (permalink / raw)
To: linux-aspeed
On 08/06/2023 04:18, Billy Tsai wrote:
> Unlike the old design that the register setting of the TACH should based
> on the configure of the PWM. In ast26xx, the dependency between pwm and
> tach controller is eliminated and becomes a separate hardware block. One
> is used to provide pwm output and another is used to monitor the frequency
> of the input. Therefore, this patch serials implements them by writing the
> two driver "pwm-aspeed-ast2600.c" and "tach-aspeed-ast2600.c". The former
> is following the pwm subsystem which can apply the existed driver to
> controller the fan(pwm-fan.c), beeper(pwm-beeper.c) and so on. The latter
> is following the sysfs interface of hwmon to creat the node for fan
> monitor.
You like to ignore my comments... How did you implement them? Go one by
one - answer to v4 emails.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 41+ messages in thread