[PATCH v6 0/4] StarFive's Pulse Width Modulation driver support

[PATCH v6 0/4] StarFive's Pulse Width Modulation driver support

[William Qiu]

Hi,

This patchset adds initial rudimentary support for the StarFive
Pulse Width Modulation controller driver. And this driver will
be used in StarFive's VisionFive 2 board.The first patch add
Documentations for the device and Patch 2 adds device probe for
the module.

Changes v5->v6:
- Rebased to v6.6rc5.
- Changed driver into a generic OpenCores driver.
- Modified dt-bindings description into OpenCores.
- Uesd the StarFive compatible string to parameterize.

Changes v4->v5:
- Rebased to v6.6rc2.
- Updated macro definition indent.
- Replaced the clock initializes the interface.
- Fixed patch description.

Changes v3->v4:
- Rebased to v6.5rc7.
- Sorted the header files in alphabetic order.
- Changed iowrite32() to writel().
- Added a way to turn off.
- Modified polarity inversion implementation.
- Added 7100 support.
- Added dts patches.
- Used the various helpers in linux/math.h.
- Corrected formatting problems.
- Renamed dtbinding  to 'starfive,jh7100-pwm.yaml'.
- Dropped the redundant code.

Changes v2->v3:
- Fixed some formatting issues.

Changes v1->v2:
- Renamed the dt-binding 'pwm-starfive.yaml' to 'starfive,jh7110-pwm.yaml'.
- Dropped the compatible's Items.
- Dropped the unuse defines.
- Modified the code to follow the Linux coding style.
- Changed return value to dev_err_probe.
- Dropped the unnecessary local variable.

The patch series is based on v6.6rc5.

William Qiu (4):
  dt-bindings: pwm: Add OpenCores PWM module
  pwm: opencores: Add PWM driver support
  riscv: dts: starfive: jh7110: Add PWM node and pins configuration
  riscv: dts: starfive: jh7100: Add PWM node and pins configuration

 .../bindings/pwm/opencores,pwm-ocores.yaml    |  53 +++++
 MAINTAINERS                                   |   7 +
 .../boot/dts/starfive/jh7100-common.dtsi      |  24 ++
 arch/riscv/boot/dts/starfive/jh7100.dtsi      |   9 +
 .../jh7110-starfive-visionfive-2.dtsi         |  22 ++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |   9 +
 drivers/pwm/Kconfig                           |  11 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-ocores.c                      | 211 ++++++++++++++++++
 9 files changed, 347 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
 create mode 100644 drivers/pwm/pwm-ocores.c

--
2.34.1

[PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module

[William Qiu]

Add documentation to describe OpenCores Pulse Width Modulation
controller driver.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../bindings/pwm/opencores,pwm-ocores.yaml    | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml

diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
new file mode 100644
index 000000000000..0f6a3434f155
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/opencores,pwm-ocores.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: OpenCores PWM controller
+
+maintainers:
+  - William Qiu <william.qiu@starfivetech.com>
+
+description:
+  OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
+  generates binary signal with user-programmable low and high periods. All PTC counters and
+  registers are 32-bit.
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    enum:
+      - opencores,pwm-ocores
+      - starfive,jh71x0-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    pwm@12490000 {
+        compatible = "opencores,pwm-ocores";
+        reg = <0x12490000 0x10000>;
+        clocks = <&clkgen 181>;
+        resets = <&rstgen 109>;
+        #pwm-cells = <3>;
+    };
-- 
2.34.1

Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

Krzysztof, William,

On Fri, Oct 20, 2023 at 06:37:38PM +0800, William Qiu wrote:
> Add documentation to describe OpenCores Pulse Width Modulation
> controller driver.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../bindings/pwm/opencores,pwm-ocores.yaml    | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> new file mode 100644
> index 000000000000..0f6a3434f155
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/opencores,pwm-ocores.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OpenCores PWM controller
> +
> +maintainers:
> +  - William Qiu <william.qiu@starfivetech.com>
> +
> +description:
> +  OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
> +  generates binary signal with user-programmable low and high periods. All PTC counters and
> +  registers are 32-bit.
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - opencores,pwm-ocores

What does the extra "ocores" suffix add, when it just repeats the vendor
prefix?

> +      - starfive,jh71x0-pwm

Krzysztof, did you approve this generic compatible?

And the whole thing looks like it should really be something like

items:
  - enum:
      - starfive,jh7100-pwm
      - starfive,jh7110-pwm
  - const: opencores,pwm

Cheers,
Conor.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 1
> +
> +  "#pwm-cells":
> +    const: 3
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pwm@12490000 {
> +        compatible = "opencores,pwm-ocores";
> +        reg = <0x12490000 0x10000>;
> +        clocks = <&clkgen 181>;
> +        resets = <&rstgen 109>;
> +        #pwm-cells = <3>;
> +    };
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2219 bytes --]

On Fri, Oct 20, 2023 at 03:21:15PM +0100, Conor Dooley wrote:
> Krzysztof, William,
> 
> On Fri, Oct 20, 2023 at 06:37:38PM +0800, William Qiu wrote:
> > Add documentation to describe OpenCores Pulse Width Modulation
> > controller driver.
> > 
> > Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> > ---
> >  .../bindings/pwm/opencores,pwm-ocores.yaml    | 53 +++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> > new file mode 100644
> > index 000000000000..0f6a3434f155
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> > @@ -0,0 +1,53 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pwm/opencores,pwm-ocores.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: OpenCores PWM controller
> > +
> > +maintainers:
> > +  - William Qiu <william.qiu@starfivetech.com>
> > +
> > +description:
> > +  OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
> > +  generates binary signal with user-programmable low and high periods. All PTC counters and
> > +  registers are 32-bit.
> > +
> > +allOf:
> > +  - $ref: pwm.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - opencores,pwm-ocores
> 
> What does the extra "ocores" suffix add, when it just repeats the vendor
> prefix?
> 
> > +      - starfive,jh71x0-pwm
> 
> Krzysztof, did you approve this generic compatible?
> 
> And the whole thing looks like it should really be something like
> 
> items:
>   - enum:
>       - starfive,jh7100-pwm
>       - starfive,jh7110-pwm
>   - const: opencores,pwm

(assuming that the opencores,pwm compatible represents a subset of what
is implemented on the jh7100 series)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module

[Krzysztof Kozlowski]

On 20/10/2023 16:22, Conor Dooley wrote:
> On Fri, Oct 20, 2023 at 03:21:15PM +0100, Conor Dooley wrote:
>> Krzysztof, William,
>>
>> On Fri, Oct 20, 2023 at 06:37:38PM +0800, William Qiu wrote:
>>> Add documentation to describe OpenCores Pulse Width Modulation
>>> controller driver.
>>>
>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>>> ---
>>>  .../bindings/pwm/opencores,pwm-ocores.yaml    | 53 +++++++++++++++++++
>>>  1 file changed, 53 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>>> new file mode 100644
>>> index 000000000000..0f6a3434f155
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>>> @@ -0,0 +1,53 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/pwm/opencores,pwm-ocores.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: OpenCores PWM controller
>>> +
>>> +maintainers:
>>> +  - William Qiu <william.qiu@starfivetech.com>
>>> +
>>> +description:
>>> +  OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
>>> +  generates binary signal with user-programmable low and high periods. All PTC counters and
>>> +  registers are 32-bit.
>>> +
>>> +allOf:
>>> +  - $ref: pwm.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - opencores,pwm-ocores
>>
>> What does the extra "ocores" suffix add, when it just repeats the vendor
>> prefix?
>>
>>> +      - starfive,jh71x0-pwm
>>
>> Krzysztof, did you approve this generic compatible?

Patch was quite different than reviewed by me. This obviously does not
make any sense. Thanks for spotting.

I guess carrying tags should not be trusted.

>>
>> And the whole thing looks like it should really be something like
>>
>> items:
>>   - enum:
>>       - starfive,jh7100-pwm
>>       - starfive,jh7110-pwm
>>   - const: opencores,pwm
> 
> (assuming that the opencores,pwm compatible represents a subset of what
> is implemented on the jh7100 series)



Best regards,
Krzysztof

Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module

[William Qiu]

On 2023/10/20 22:21, Conor Dooley wrote:
> Krzysztof, William,
> 
> On Fri, Oct 20, 2023 at 06:37:38PM +0800, William Qiu wrote:
>> Add documentation to describe OpenCores Pulse Width Modulation
>> controller driver.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  .../bindings/pwm/opencores,pwm-ocores.yaml    | 53 +++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> new file mode 100644
>> index 000000000000..0f6a3434f155
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> @@ -0,0 +1,53 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/opencores,pwm-ocores.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: OpenCores PWM controller
>> +
>> +maintainers:
>> +  - William Qiu <william.qiu@starfivetech.com>
>> +
>> +description:
>> +  OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
>> +  generates binary signal with user-programmable low and high periods. All PTC counters and
>> +  registers are 32-bit.
>> +
>> +allOf:
>> +  - $ref: pwm.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - opencores,pwm-ocores
> 
> What does the extra "ocores" suffix add, when it just repeats the vendor
> prefix?
> 
>> +      - starfive,jh71x0-pwm
> 
> Krzysztof, did you approve this generic compatible?
> 
> And the whole thing looks like it should really be something like
> 
> items:
>   - enum:
>       - starfive,jh7100-pwm
>       - starfive,jh7110-pwm
>   - const: opencores,pwm
> 
> Cheers,
> Conor.
> 
I'm going to use this format.
Thanks,
William
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    maxItems: 1
>> +
>> +  resets:
>> +    maxItems: 1
>> +
>> +  "#pwm-cells":
>> +    const: 3
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - clocks
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    pwm@12490000 {
>> +        compatible = "opencores,pwm-ocores";
>> +        reg = <0x12490000 0x10000>;
>> +        clocks = <&clkgen 181>;
>> +        resets = <&rstgen 109>;
>> +        #pwm-cells = <3>;
>> +    };
>> -- 
>> 2.34.1
>> 

Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module

[Krzysztof Kozlowski]

On 20/10/2023 12:37, William Qiu wrote:
> Add documentation to describe OpenCores Pulse Width Modulation
> controller driver.
> 
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Please point me where this patch got review?

> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../bindings/pwm/opencores,pwm-ocores.yaml    | 53 +++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> new file mode 100644
> index 000000000000..0f6a3434f155
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/opencores,pwm-ocores.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: OpenCores PWM controller
> +
> +maintainers:
> +  - William Qiu <william.qiu@starfivetech.com>
> +
> +description:
> +  OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
> +  generates binary signal with user-programmable low and high periods. All PTC counters and
> +  registers are 32-bit.
> +
> +allOf:
> +  - $ref: pwm.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - opencores,pwm-ocores

NAK. This is not something which received my review.


Best regards,
Krzysztof

Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module

[William Qiu]

On 2023/10/21 2:01, Krzysztof Kozlowski wrote:
> On 20/10/2023 12:37, William Qiu wrote:
>> Add documentation to describe OpenCores Pulse Width Modulation
>> controller driver.
>> 
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Please point me where this patch got review?
> 
This is my mistake. After making extensive changes, the tag should have been deleted

Best regards,
William
>> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
>> ---
>>  .../bindings/pwm/opencores,pwm-ocores.yaml    | 53 +++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> new file mode 100644
>> index 000000000000..0f6a3434f155
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> @@ -0,0 +1,53 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pwm/opencores,pwm-ocores.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: OpenCores PWM controller
>> +
>> +maintainers:
>> +  - William Qiu <william.qiu@starfivetech.com>
>> +
>> +description:
>> +  OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
>> +  generates binary signal with user-programmable low and high periods. All PTC counters and
>> +  registers are 32-bit.
>> +
>> +allOf:
>> +  - $ref: pwm.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - opencores,pwm-ocores
> 
> NAK. This is not something which received my review.
> 
> 
> Best regards,
> Krzysztof
> 

[PATCH v6 2/4] pwm: opencores: Add PWM driver support

[William Qiu]

Add Pulse Width Modulation driver support for OpenCores.

Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 MAINTAINERS              |   7 ++
 drivers/pwm/Kconfig      |  11 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-ocores.c | 211 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)
 create mode 100644 drivers/pwm/pwm-ocores.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 6c4cce45a09d..321af8fa7aad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16003,6 +16003,13 @@ F:	Documentation/i2c/busses/i2c-ocores.rst
 F:	drivers/i2c/busses/i2c-ocores.c
 F:	include/linux/platform_data/i2c-ocores.h

+OPENCORES PWM DRIVER
+M:	William Qiu <william.qiu@starfivetech.com>
+M:	Hal Feng <hal.feng@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
+F:	drivers/pwm/pwm-ocores.c
+
 OPENRISC ARCHITECTURE
 M:	Jonas Bonn <jonas@southpole.se>
 M:	Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ebcddf91f7b..cbfbf227d957 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -434,6 +434,17 @@ config PWM_NTXEC
 	  controller found in certain e-book readers designed by the original
 	  design manufacturer Netronix.

+config PWM_OCORES
+	tristate "Opencores PWM support"
+	depends on HAS_IOMEM && OF
+	depends on COMMON_CLK && RESET_CONTROLLER
+	help
+	  If you say yes to this option, support will be included for the
+	  OpenCores PWM. For details see https://opencores.org/projects/ptc.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ocores.
+
 config PWM_OMAP_DMTIMER
 	tristate "OMAP Dual-Mode Timer PWM support"
 	depends on OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c822389c2a24..542b98202153 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
 obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
+obj-$(CONFIG_PWM_OCORES)	+= pwm-ocores.o
 obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
new file mode 100644
index 000000000000..7a510de4e063
--- /dev/null
+++ b/drivers/pwm/pwm-ocores.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * OpenCores PWM Driver
+ *
+ * https://opencores.org/projects/ptc
+ *
+ * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#define REG_OCPWM_CNTR(base)	((base))
+#define REG_OCPWM_HRC(base)	((base) + 0x4)
+#define REG_OCPWM_LRC(base)	((base) + 0x8)
+#define REG_OCPWM_CTRL(base)	((base) + 0xC)
+
+/* OCPWM_CTRL register bits*/
+#define OCPWM_EN      BIT(0)
+#define OCPWM_ECLK    BIT(1)
+#define OCPWM_NEC     BIT(2)
+#define OCPWM_OE      BIT(3)
+#define OCPWM_SIGNLE  BIT(4)
+#define OCPWM_INTE    BIT(5)
+#define OCPWM_INT     BIT(6)
+#define OCPWM_CNTRRST BIT(7)
+#define OCPWM_CAPTE   BIT(8)
+
+struct ocores_pwm_device {
+	struct pwm_chip chip;
+	struct clk *clk;
+	struct reset_control *rst;
+	const struct ocores_pwm_data *data;
+	void __iomem *regs;
+	u32 clk_rate; /* PWM APB clock frequency */
+};
+
+struct ocores_pwm_data {
+	void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel);
+};
+
+static inline struct ocores_pwm_device *
+chip_to_ocores(struct pwm_chip *chip)
+
+{
+	return container_of(chip, struct ocores_pwm_device, chip);
+}
+
+void __iomem *starfive_jh71x0_get_ch_base(void __iomem *base,
+					  unsigned int channel)
+{
+	return base + (channel > 3 ? channel % 4 * 0x10 + (1 << 15) : channel * 0x10);
+}
+
+static int ocores_pwm_get_state(struct pwm_chip *chip,
+				struct pwm_device *dev,
+				struct pwm_state *state)
+{
+	struct ocores_pwm_device *pwm = chip_to_ocores(chip);
+	void __iomem *base = pwm->data->get_ch_base ?
+			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
+	u32 period_data, duty_data, ctrl_data;
+
+	period_data = readl(REG_OCPWM_LRC(base));
+	duty_data = readl(REG_OCPWM_HRC(base));
+	ctrl_data = readl(REG_OCPWM_CTRL(base));
+
+	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
+	state->polarity = PWM_POLARITY_INVERSED;
+	state->enabled = (ctrl_data & OCPWM_EN) ? true : false;
+
+	return 0;
+}
+
+static int ocores_pwm_apply(struct pwm_chip *chip,
+			    struct pwm_device *dev,
+			    const struct pwm_state *state)
+{
+	struct ocores_pwm_device *pwm = chip_to_ocores(chip);
+	void __iomem *base = pwm->data->get_ch_base ?
+			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
+	u32 period_data, duty_data, ctrl_data = 0;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
+					    NSEC_PER_SEC);
+	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
+					  NSEC_PER_SEC);
+
+	writel(period_data, REG_OCPWM_LRC(base));
+	writel(duty_data, REG_OCPWM_HRC(base));
+	writel(0,  REG_OCPWM_CNTR(base));
+
+	ctrl_data = readl(REG_OCPWM_CTRL(base));
+	if (state->enabled)
+		writel(ctrl_data | OCPWM_EN | OCPWM_OE, REG_OCPWM_CTRL(base));
+	else
+		writel(ctrl_data & ~(OCPWM_EN | OCPWM_OE), REG_OCPWM_CTRL(base));
+
+	return 0;
+}
+
+static const struct pwm_ops ocores_pwm_ops = {
+	.get_state	= ocores_pwm_get_state,
+	.apply		= ocores_pwm_apply,
+	.owner		= THIS_MODULE,
+};
+
+static const struct ocores_pwm_data jh71x0_pwm_data = {
+	.get_ch_base = starfive_jh71x0_get_ch_base,
+};
+
+static const struct of_device_id ocores_pwm_of_match[] = {
+	{ .compatible = "opencores,pwm-ocores" },
+	{ .compatible = "starfive,jh71x0-pwm", .data = &jh71x0_pwm_data},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ocores_pwm_of_match);
+
+static int ocores_pwm_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *id;
+	struct device *dev = &pdev->dev;
+	struct ocores_pwm_device *pwm;
+	struct pwm_chip *chip;
+	int ret;
+
+	id = of_match_device(ocores_pwm_of_match, dev);
+	if (!id)
+		return -EINVAL;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->data = id->data;
+	chip = &pwm->chip;
+	chip->dev = dev;
+	chip->ops = &ocores_pwm_ops;
+	chip->npwm = 8;
+	chip->of_pwm_n_cells = 3;
+
+	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pwm->regs))
+		return dev_err_probe(dev, PTR_ERR(pwm->regs),
+				     "Unable to map IO resources\n");
+
+	pwm->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk),
+				     "Unable to get pwm's clock\n");
+
+	pwm->rst = devm_reset_control_get_optional_exclusive(dev, NULL);
+	reset_control_deassert(pwm->rst);
+
+	pwm->clk_rate = clk_get_rate(pwm->clk);
+	if (pwm->clk_rate <= 0) {
+		dev_warn(dev, "Failed to get APB clock rate\n");
+		return -EINVAL;
+	}
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0) {
+		dev_err(dev, "Cannot register PTC: %d\n", ret);
+		clk_disable_unprepare(pwm->clk);
+		reset_control_assert(pwm->rst);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int ocores_pwm_remove(struct platform_device *dev)
+{
+	struct ocores_pwm_device *pwm = platform_get_drvdata(dev);
+
+	reset_control_assert(pwm->rst);
+	clk_disable_unprepare(pwm->clk);
+
+	return 0;
+}
+
+static struct platform_driver ocores_pwm_driver = {
+	.probe = ocores_pwm_probe,
+	.remove = ocores_pwm_remove,
+	.driver = {
+		.name = "ocores-pwm",
+		.of_match_table = ocores_pwm_of_match,
+	},
+};
+module_platform_driver(ocores_pwm_driver);
+
+MODULE_AUTHOR("Jieqin Chen");
+MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
+MODULE_DESCRIPTION("OpenCores PWM PTC driver");
+MODULE_LICENSE("GPL");
--
2.34.1

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 11870 bytes --]

Hello,

On Fri, Oct 20, 2023 at 06:37:39PM +0800, William Qiu wrote:
> Add Pulse Width Modulation driver support for OpenCores.
> 
> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> ---
>  MAINTAINERS              |   7 ++
>  drivers/pwm/Kconfig      |  11 ++
>  drivers/pwm/Makefile     |   1 +
>  drivers/pwm/pwm-ocores.c | 211 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 230 insertions(+)
>  create mode 100644 drivers/pwm/pwm-ocores.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 6c4cce45a09d..321af8fa7aad 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16003,6 +16003,13 @@ F:	Documentation/i2c/busses/i2c-ocores.rst
>  F:	drivers/i2c/busses/i2c-ocores.c
>  F:	include/linux/platform_data/i2c-ocores.h
> 
> +OPENCORES PWM DRIVER
> +M:	William Qiu <william.qiu@starfivetech.com>
> +M:	Hal Feng <hal.feng@starfivetech.com>
> +S:	Supported
> +F:	Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> +F:	drivers/pwm/pwm-ocores.c
> +
>  OPENRISC ARCHITECTURE
>  M:	Jonas Bonn <jonas@southpole.se>
>  M:	Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ebcddf91f7b..cbfbf227d957 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -434,6 +434,17 @@ config PWM_NTXEC
>  	  controller found in certain e-book readers designed by the original
>  	  design manufacturer Netronix.
> 
> +config PWM_OCORES
> +	tristate "Opencores PWM support"
> +	depends on HAS_IOMEM && OF
> +	depends on COMMON_CLK && RESET_CONTROLLER

Would it make sense to add something like:

	depends on ARCH_SOMETHING || COMPILE_TEST

here?

> +	help
> +	  If you say yes to this option, support will be included for the
> +	  OpenCores PWM. For details see https://opencores.org/projects/ptc.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-ocores.
> +
>  config PWM_OMAP_DMTIMER
>  	tristate "OMAP Dual-Mode Timer PWM support"
>  	depends on OF
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index c822389c2a24..542b98202153 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
>  obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>  obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
> +obj-$(CONFIG_PWM_OCORES)	+= pwm-ocores.o
>  obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
>  obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
> new file mode 100644
> index 000000000000..7a510de4e063
> --- /dev/null
> +++ b/drivers/pwm/pwm-ocores.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * OpenCores PWM Driver
> + *
> + * https://opencores.org/projects/ptc
> + *
> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
> + */

Please add a section here describing the hardware limitations. Please
stick to the format used e.g. in drivers/pwm/pwm-sl28cpld.c to make this
easy to grep for. It should mention for example that the hardware can
only do inverted polarity.

> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define REG_OCPWM_CNTR(base)	((base))
> +#define REG_OCPWM_HRC(base)	((base) + 0x4)
> +#define REG_OCPWM_LRC(base)	((base) + 0x8)
> +#define REG_OCPWM_CTRL(base)	((base) + 0xC)

This is unusual, I would skip base here and do the addition explicitly
in some static inline helpers like:

	static inline ocores_writel(struct ocores_pwm_device *, unsigned int offset, u32 val);

> +/* OCPWM_CTRL register bits*/
> +#define OCPWM_EN      BIT(0)
> +#define OCPWM_ECLK    BIT(1)
> +#define OCPWM_NEC     BIT(2)
> +#define OCPWM_OE      BIT(3)
> +#define OCPWM_SIGNLE  BIT(4)
> +#define OCPWM_INTE    BIT(5)
> +#define OCPWM_INT     BIT(6)
> +#define OCPWM_CNTRRST BIT(7)
> +#define OCPWM_CAPTE   BIT(8)

I like register bit fields being named with the register as prefix, so I
suggest:

	#define	REG_OCPWM_CTRL_EN	BIT(0)
	...

> +
> +struct ocores_pwm_device {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	const struct ocores_pwm_data *data;
> +	void __iomem *regs;
> +	u32 clk_rate; /* PWM APB clock frequency */
> +};
> +
> +struct ocores_pwm_data {
> +	void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel);

It might be worth to mark this with the function attribute const.

> +};
> +
> +static inline struct ocores_pwm_device *
> +chip_to_ocores(struct pwm_chip *chip)

These two lines can go in a single one.

> +

please drop this empty line.

> +{
> +	return container_of(chip, struct ocores_pwm_device, chip);
> +}
> +
> +void __iomem *starfive_jh71x0_get_ch_base(void __iomem *base,
> +					  unsigned int channel)
> +{
> +	return base + (channel > 3 ? channel % 4 * 0x10 + (1 << 15) : channel * 0x10);

Maybe make this:

	unsigned int offset =
		(channel > 3 ? 1 << 15 : 0) +
		(channel & 3) * 0x10
	...

or even:

	unsigned int offset = (channel & 4) << 13 + (channel & 3) * 0x10;

The former is easier to read, the latter might be compiled to faster
code.

Alternatively: Is it easier/sensible to model the jh71x0 hardware as two
PWM chips with 4 lines each?

> +}
> +
> +static int ocores_pwm_get_state(struct pwm_chip *chip,
> +				struct pwm_device *dev,
> +				struct pwm_state *state)
> +{
> +	struct ocores_pwm_device *pwm = chip_to_ocores(chip);

Please use "pwm" for variables of type struct pwm_device and pick
something different for ocores_pwm_device variables. I suggest something
like "ddata" or "opd".

> +	void __iomem *base = pwm->data->get_ch_base ?
> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
> +	u32 period_data, duty_data, ctrl_data;
> +
> +	period_data = readl(REG_OCPWM_LRC(base));
> +	duty_data = readl(REG_OCPWM_HRC(base));
> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
> +
> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);

Please test your driver with PWM_DEBUG enabled. The rounding is wrong
here.

> +	state->polarity = PWM_POLARITY_INVERSED;
> +	state->enabled = (ctrl_data & OCPWM_EN) ? true : false;
> +
> +	return 0;
> +}
> +
> +static int ocores_pwm_apply(struct pwm_chip *chip,
> +			    struct pwm_device *dev,
> +			    const struct pwm_state *state)
> +{
> +	struct ocores_pwm_device *pwm = chip_to_ocores(chip);
> +	void __iomem *base = pwm->data->get_ch_base ?
> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
> +	u32 period_data, duty_data, ctrl_data = 0;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,

this multiplication might overflow. And also wrong rounding. I didn't
check, but maybe DIV_ROUND_CLOSEST_ULL might return a value > U32_MAX?

> +					    NSEC_PER_SEC);
> +	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
> +					  NSEC_PER_SEC);
> +
> +	writel(period_data, REG_OCPWM_LRC(base));
> +	writel(duty_data, REG_OCPWM_HRC(base));
> +	writel(0,  REG_OCPWM_CNTR(base));

s/  / /

I assume this is "glitchy", i.e. after updating the REG_OCPWM_LRC and
before updating REG_OCPWM_HRC the signal emitted might be a mixture
between old and new state? This should be mentioned in the Limitations
section I mentioned above. Also mention that the currently running
period is not completed and how the output behave if the hardware is
disabled.

> +
> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
> +	if (state->enabled)
> +		writel(ctrl_data | OCPWM_EN | OCPWM_OE, REG_OCPWM_CTRL(base));
> +	else
> +		writel(ctrl_data & ~(OCPWM_EN | OCPWM_OE), REG_OCPWM_CTRL(base));
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops ocores_pwm_ops = {
> +	.get_state	= ocores_pwm_get_state,
> +	.apply		= ocores_pwm_apply,
> +	.owner		= THIS_MODULE,

The assignment to .owner should be dropped. (See commit
384461abcab6602abc06c2dfb8fb99beeeaa12b0)

> +};
> +
> +static const struct ocores_pwm_data jh71x0_pwm_data = {
> +	.get_ch_base = starfive_jh71x0_get_ch_base,
> +};
> +
> +static const struct of_device_id ocores_pwm_of_match[] = {
> +	{ .compatible = "opencores,pwm-ocores" },
> +	{ .compatible = "starfive,jh71x0-pwm", .data = &jh71x0_pwm_data},
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ocores_pwm_of_match);
> +
> +static int ocores_pwm_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *id;
> +	struct device *dev = &pdev->dev;
> +	struct ocores_pwm_device *pwm;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	id = of_match_device(ocores_pwm_of_match, dev);
> +	if (!id)
> +		return -EINVAL;
> +
> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->data = id->data;
> +	chip = &pwm->chip;
> +	chip->dev = dev;
> +	chip->ops = &ocores_pwm_ops;
> +	chip->npwm = 8;
> +	chip->of_pwm_n_cells = 3;
> +
> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(pwm->regs))
> +		return dev_err_probe(dev, PTR_ERR(pwm->regs),
> +				     "Unable to map IO resources\n");
> +
> +	pwm->clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(pwm->clk))
> +		return dev_err_probe(dev, PTR_ERR(pwm->clk),
> +				     "Unable to get pwm's clock\n");
> +
> +	pwm->rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	reset_control_deassert(pwm->rst);
> +
> +	pwm->clk_rate = clk_get_rate(pwm->clk);
> +	if (pwm->clk_rate <= 0) {
> +		dev_warn(dev, "Failed to get APB clock rate\n");
> +		return -EINVAL;

dev_err_probe() here, too? Missing call to reset_control_assert().

> +	}
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret < 0) {
> +		dev_err(dev, "Cannot register PTC: %d\n", ret);

dev_err_probe()

> +		clk_disable_unprepare(pwm->clk);

This is wrong, devm_clk_get_enabled() cares for that.

> +		reset_control_assert(pwm->rst);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pwm);
> +
> +	return 0;

If you call platform_set_drvdata() earlier you can just return ret here
and drop the return in the error path above.

> +}
> +
> +static int ocores_pwm_remove(struct platform_device *dev)
> +{
> +	struct ocores_pwm_device *pwm = platform_get_drvdata(dev);
> +
> +	reset_control_assert(pwm->rst);
> +	clk_disable_unprepare(pwm->clk);

Wrong in the same way as the call in .probe()'s error path.

> +
> +	return 0;
> +}
> +
> +static struct platform_driver ocores_pwm_driver = {
> +	.probe = ocores_pwm_probe,
> +	.remove = ocores_pwm_remove,

Please use .remove_new

> +	.driver = {
> +		.name = "ocores-pwm",
> +		.of_match_table = ocores_pwm_of_match,
> +	},
> +};
> +module_platform_driver(ocores_pwm_driver);
> +
> +MODULE_AUTHOR("Jieqin Chen");

Jieqin Chen != William Qiu ?

> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
> +MODULE_DESCRIPTION("OpenCores PWM PTC driver");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[William Qiu]

On 2023/10/20 19:25, Uwe Kleine-König wrote:
> Hello,
> 
> On Fri, Oct 20, 2023 at 06:37:39PM +0800, William Qiu wrote:
>> Add Pulse Width Modulation driver support for OpenCores.
>> 
>> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> ---
>>  MAINTAINERS              |   7 ++
>>  drivers/pwm/Kconfig      |  11 ++
>>  drivers/pwm/Makefile     |   1 +
>>  drivers/pwm/pwm-ocores.c | 211 +++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 230 insertions(+)
>>  create mode 100644 drivers/pwm/pwm-ocores.c
>> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6c4cce45a09d..321af8fa7aad 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -16003,6 +16003,13 @@ F:	Documentation/i2c/busses/i2c-ocores.rst
>>  F:	drivers/i2c/busses/i2c-ocores.c
>>  F:	include/linux/platform_data/i2c-ocores.h
>> 
>> +OPENCORES PWM DRIVER
>> +M:	William Qiu <william.qiu@starfivetech.com>
>> +M:	Hal Feng <hal.feng@starfivetech.com>
>> +S:	Supported
>> +F:	Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> +F:	drivers/pwm/pwm-ocores.c
>> +
>>  OPENRISC ARCHITECTURE
>>  M:	Jonas Bonn <jonas@southpole.se>
>>  M:	Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index 8ebcddf91f7b..cbfbf227d957 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -434,6 +434,17 @@ config PWM_NTXEC
>>  	  controller found in certain e-book readers designed by the original
>>  	  design manufacturer Netronix.
>> 
>> +config PWM_OCORES
>> +	tristate "Opencores PWM support"
>> +	depends on HAS_IOMEM && OF
>> +	depends on COMMON_CLK && RESET_CONTROLLER
> 
> Would it make sense to add something like:
> 
> 	depends on ARCH_SOMETHING || COMPILE_TEST
> 
> here?
> 
But there is no mention of architectural limitations in the OpenCores's
specification.
>> +	help
>> +	  If you say yes to this option, support will be included for the
>> +	  OpenCores PWM. For details see https://opencores.org/projects/ptc.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called pwm-ocores.
>> +
>>  config PWM_OMAP_DMTIMER
>>  	tristate "OMAP Dual-Mode Timer PWM support"
>>  	depends on OF
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index c822389c2a24..542b98202153 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -39,6 +39,7 @@ obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
>>  obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
>>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
>>  obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
>> +obj-$(CONFIG_PWM_OCORES)	+= pwm-ocores.o
>>  obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
>>  obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
>>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
>> diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
>> new file mode 100644
>> index 000000000000..7a510de4e063
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-ocores.c
>> @@ -0,0 +1,211 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * OpenCores PWM Driver
>> + *
>> + * https://opencores.org/projects/ptc
>> + *
>> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
>> + */
> 
> Please add a section here describing the hardware limitations. Please
> stick to the format used e.g. in drivers/pwm/pwm-sl28cpld.c to make this
> easy to grep for. It should mention for example that the hardware can
> only do inverted polarity.
> 
Will add.
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#define REG_OCPWM_CNTR(base)	((base))
>> +#define REG_OCPWM_HRC(base)	((base) + 0x4)
>> +#define REG_OCPWM_LRC(base)	((base) + 0x8)
>> +#define REG_OCPWM_CTRL(base)	((base) + 0xC)
> 
> This is unusual, I would skip base here and do the addition explicitly
> in some static inline helpers like:
> 
> 	static inline ocores_writel(struct ocores_pwm_device *, unsigned int offset, u32 val);
> 
Will update.
>> +/* OCPWM_CTRL register bits*/
>> +#define OCPWM_EN      BIT(0)
>> +#define OCPWM_ECLK    BIT(1)
>> +#define OCPWM_NEC     BIT(2)
>> +#define OCPWM_OE      BIT(3)
>> +#define OCPWM_SIGNLE  BIT(4)
>> +#define OCPWM_INTE    BIT(5)
>> +#define OCPWM_INT     BIT(6)
>> +#define OCPWM_CNTRRST BIT(7)
>> +#define OCPWM_CAPTE   BIT(8)
> 
> I like register bit fields being named with the register as prefix, so I
> suggest:
> 
> 	#define	REG_OCPWM_CTRL_EN	BIT(0)
> 	...
> 
Will update.
>> +
>> +struct ocores_pwm_device {
>> +	struct pwm_chip chip;
>> +	struct clk *clk;
>> +	struct reset_control *rst;
>> +	const struct ocores_pwm_data *data;
>> +	void __iomem *regs;
>> +	u32 clk_rate; /* PWM APB clock frequency */
>> +};
>> +
>> +struct ocores_pwm_data {
>> +	void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel);
> 
> It might be worth to mark this with the function attribute const.
> 
Will update.
>> +};
>> +
>> +static inline struct ocores_pwm_device *
>> +chip_to_ocores(struct pwm_chip *chip)
> 
> These two lines can go in a single one.
> 
>> +
Will update.
> 
> please drop this empty line.
> 
Will drop.
>> +{
>> +	return container_of(chip, struct ocores_pwm_device, chip);
>> +}
>> +
>> +void __iomem *starfive_jh71x0_get_ch_base(void __iomem *base,
>> +					  unsigned int channel)
>> +{
>> +	return base + (channel > 3 ? channel % 4 * 0x10 + (1 << 15) : channel * 0x10);
> 
> Maybe make this:
> 
> 	unsigned int offset =
> 		(channel > 3 ? 1 << 15 : 0) +
> 		(channel & 3) * 0x10
> 	...
> 
> or even:
> 
> 	unsigned int offset = (channel & 4) << 13 + (channel & 3) * 0x10;
> 
> The former is easier to read, the latter might be compiled to faster
> code.
> 
Will update.
> Alternatively: Is it easier/sensible to model the jh71x0 hardware as two
> PWM chips with 4 lines each?
> 
Maybe it's better to stick with the original.  
>> +}
>> +
>> +static int ocores_pwm_get_state(struct pwm_chip *chip,
>> +				struct pwm_device *dev,
>> +				struct pwm_state *state)
>> +{
>> +	struct ocores_pwm_device *pwm = chip_to_ocores(chip);
> 
> Please use "pwm" for variables of type struct pwm_device and pick
> something different for ocores_pwm_device variables. I suggest something
> like "ddata" or "opd".
> 
Will update.
>> +	void __iomem *base = pwm->data->get_ch_base ?
>> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
>> +	u32 period_data, duty_data, ctrl_data;
>> +
>> +	period_data = readl(REG_OCPWM_LRC(base));
>> +	duty_data = readl(REG_OCPWM_HRC(base));
>> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
>> +
>> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
>> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> 
> Please test your driver with PWM_DEBUG enabled. The rounding is wrong
> here.
> 
Will check
>> +	state->polarity = PWM_POLARITY_INVERSED;
>> +	state->enabled = (ctrl_data & OCPWM_EN) ? true : false;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ocores_pwm_apply(struct pwm_chip *chip,
>> +			    struct pwm_device *dev,
>> +			    const struct pwm_state *state)
>> +{
>> +	struct ocores_pwm_device *pwm = chip_to_ocores(chip);
>> +	void __iomem *base = pwm->data->get_ch_base ?
>> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
>> +	u32 period_data, duty_data, ctrl_data = 0;
>> +
>> +	if (state->polarity != PWM_POLARITY_INVERSED)
>> +		return -EINVAL;
>> +
>> +	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
> 
> this multiplication might overflow. And also wrong rounding. I didn't
> check, but maybe DIV_ROUND_CLOSEST_ULL might return a value > U32_MAX?
> 
Will check
>> +					    NSEC_PER_SEC);
>> +	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
>> +					  NSEC_PER_SEC);
>> +
>> +	writel(period_data, REG_OCPWM_LRC(base));
>> +	writel(duty_data, REG_OCPWM_HRC(base));
>> +	writel(0,  REG_OCPWM_CNTR(base));
> 
> s/  / /
> 
> I assume this is "glitchy", i.e. after updating the REG_OCPWM_LRC and
> before updating REG_OCPWM_HRC the signal emitted might be a mixture
> between old and new state? This should be mentioned in the Limitations
> section I mentioned above. Also mention that the currently running
> period is not completed and how the output behave if the hardware is
> disabled.
> 
Will check
>> +
>> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
>> +	if (state->enabled)
>> +		writel(ctrl_data | OCPWM_EN | OCPWM_OE, REG_OCPWM_CTRL(base));
>> +	else
>> +		writel(ctrl_data & ~(OCPWM_EN | OCPWM_OE), REG_OCPWM_CTRL(base));
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct pwm_ops ocores_pwm_ops = {
>> +	.get_state	= ocores_pwm_get_state,
>> +	.apply		= ocores_pwm_apply,
>> +	.owner		= THIS_MODULE,
> 
> The assignment to .owner should be dropped. (See commit
> 384461abcab6602abc06c2dfb8fb99beeeaa12b0)
> 
Will drop.
>> +};
>> +
>> +static const struct ocores_pwm_data jh71x0_pwm_data = {
>> +	.get_ch_base = starfive_jh71x0_get_ch_base,
>> +};
>> +
>> +static const struct of_device_id ocores_pwm_of_match[] = {
>> +	{ .compatible = "opencores,pwm-ocores" },
>> +	{ .compatible = "starfive,jh71x0-pwm", .data = &jh71x0_pwm_data},
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, ocores_pwm_of_match);
>> +
>> +static int ocores_pwm_probe(struct platform_device *pdev)
>> +{
>> +	const struct of_device_id *id;
>> +	struct device *dev = &pdev->dev;
>> +	struct ocores_pwm_device *pwm;
>> +	struct pwm_chip *chip;
>> +	int ret;
>> +
>> +	id = of_match_device(ocores_pwm_of_match, dev);
>> +	if (!id)
>> +		return -EINVAL;
>> +
>> +	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
>> +	if (!pwm)
>> +		return -ENOMEM;
>> +
>> +	pwm->data = id->data;
>> +	chip = &pwm->chip;
>> +	chip->dev = dev;
>> +	chip->ops = &ocores_pwm_ops;
>> +	chip->npwm = 8;
>> +	chip->of_pwm_n_cells = 3;
>> +
>> +	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(pwm->regs))
>> +		return dev_err_probe(dev, PTR_ERR(pwm->regs),
>> +				     "Unable to map IO resources\n");
>> +
>> +	pwm->clk = devm_clk_get_enabled(dev, NULL);
>> +	if (IS_ERR(pwm->clk))
>> +		return dev_err_probe(dev, PTR_ERR(pwm->clk),
>> +				     "Unable to get pwm's clock\n");
>> +
>> +	pwm->rst = devm_reset_control_get_optional_exclusive(dev, NULL);
>> +	reset_control_deassert(pwm->rst);
>> +
>> +	pwm->clk_rate = clk_get_rate(pwm->clk);
>> +	if (pwm->clk_rate <= 0) {
>> +		dev_warn(dev, "Failed to get APB clock rate\n");
>> +		return -EINVAL;
> 
> dev_err_probe() here, too? Missing call to reset_control_assert().
> 
Will update
>> +	}
>> +
>> +	ret = devm_pwmchip_add(dev, chip);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Cannot register PTC: %d\n", ret);
> 
> dev_err_probe()
> 
Will update
>> +		clk_disable_unprepare(pwm->clk);
> 
> This is wrong, devm_clk_get_enabled() cares for that.
> 
Will update
>> +		reset_control_assert(pwm->rst);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, pwm);
>> +
>> +	return 0;
> 
> If you call platform_set_drvdata() earlier you can just return ret here
> and drop the return in the error path above.
> 
Will drop.
>> +}
>> +
>> +static int ocores_pwm_remove(struct platform_device *dev)
>> +{
>> +	struct ocores_pwm_device *pwm = platform_get_drvdata(dev);
>> +
>> +	reset_control_assert(pwm->rst);
>> +	clk_disable_unprepare(pwm->clk);
> 
> Wrong in the same way as the call in .probe()'s error path.
> 
Will update.
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver ocores_pwm_driver = {
>> +	.probe = ocores_pwm_probe,
>> +	.remove = ocores_pwm_remove,
> 
> Please use .remove_new
> 
Will update.
>> +	.driver = {
>> +		.name = "ocores-pwm",
>> +		.of_match_table = ocores_pwm_of_match,
>> +	},
>> +};
>> +module_platform_driver(ocores_pwm_driver);
>> +
>> +MODULE_AUTHOR("Jieqin Chen");
> 
> Jieqin Chen != William Qiu ?
> 
This driver was originally written by Chen Jieqin, but she left, so I
just based on her driver to do upstream, so I think the author is
still her.


Thanks for taking time to review this patch series and give a lot of
useful suggestion,

Best regards,
William
>> +MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
>> +MODULE_DESCRIPTION("OpenCores PWM PTC driver");
>> +MODULE_LICENSE("GPL");
> 
> Best regards
> Uwe
> 

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4555 bytes --]

Hello William,

On Tue, Oct 24, 2023 at 05:16:49PM +0800, William Qiu wrote:
> On 2023/10/20 19:25, Uwe Kleine-König wrote:
> > Hello,
> > 
> > On Fri, Oct 20, 2023 at 06:37:39PM +0800, William Qiu wrote:
> >> Add Pulse Width Modulation driver support for OpenCores.
> >> 
> >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> >> ---
> >>  MAINTAINERS              |   7 ++
> >>  drivers/pwm/Kconfig      |  11 ++
> >>  drivers/pwm/Makefile     |   1 +
> >>  drivers/pwm/pwm-ocores.c | 211 +++++++++++++++++++++++++++++++++++++++
> >>  4 files changed, 230 insertions(+)
> >>  create mode 100644 drivers/pwm/pwm-ocores.c
> >> 
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 6c4cce45a09d..321af8fa7aad 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -16003,6 +16003,13 @@ F:	Documentation/i2c/busses/i2c-ocores.rst
> >>  F:	drivers/i2c/busses/i2c-ocores.c
> >>  F:	include/linux/platform_data/i2c-ocores.h
> >> 
> >> +OPENCORES PWM DRIVER
> >> +M:	William Qiu <william.qiu@starfivetech.com>
> >> +M:	Hal Feng <hal.feng@starfivetech.com>
> >> +S:	Supported
> >> +F:	Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
> >> +F:	drivers/pwm/pwm-ocores.c
> >> +
> >>  OPENRISC ARCHITECTURE
> >>  M:	Jonas Bonn <jonas@southpole.se>
> >>  M:	Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index 8ebcddf91f7b..cbfbf227d957 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -434,6 +434,17 @@ config PWM_NTXEC
> >>  	  controller found in certain e-book readers designed by the original
> >>  	  design manufacturer Netronix.
> >> 
> >> +config PWM_OCORES
> >> +	tristate "Opencores PWM support"
> >> +	depends on HAS_IOMEM && OF
> >> +	depends on COMMON_CLK && RESET_CONTROLLER
> > 
> > Would it make sense to add something like:
> > 
> > 	depends on ARCH_SOMETHING || COMPILE_TEST
> > 
> > here?
> > 
> But there is no mention of architectural limitations in the OpenCores's
> specification.

I already guessed that. Still it probably makes no sense to enable that
option on most machines. The PWM device found in i.MX SoCs can
theoretically also be implemented on AT91 or S390x. In practice it
isn't, so there is a dependency on ARCH_MXC || COMPILE_TEST.

Consider the role of someone who does a kernel bump for a certain
machine (on one end of the spectrum) or a distribution kernel (on the
other end).

If you take a 6.5 x86_64 allmodconfig + COMPILE_TEST=n and upgrade to
v6.6-rc7 and do an oldconfig, you get 90 questions[1].

Just looking quickly through this list, among them are:

	DRM support for Loongson Graphics (DRM_LOONGSON) [N/m/?] (NEW) 
	Xilinx AXI DMAS Engine (XILINX_DMA) [N/m/y/?] (NEW)
	Clock driver for Renesas VersaClock 3 devices (COMMON_CLK_VC3) [N/m/y/?] (NEW)
	Realtek RT1017 SDCA Codec - SDW (SND_SOC_RT1017_SDCA_SDW) [N/m/?] (NEW)

I didn't check in detail and maybe one or the other is valid on x86_64,
but I'd be surprised if you find two that are sensible to enable on
x86_64 to support a real machine.

While I think Kconfig cannot be held responsible to only allow
generating "real world sensible" configurations, we should work a bit
harder to rule out the obvious violators and make it easy for people
configuring the kernel where sensible.

In my book it's better to have a too strong dependency at first for a
new driver (but allow it with COMPILE_TEST). Someone who as a device
needing that driver will find it out and speak up. However if you allow
to enable the driver everywhere, many people will disable the driver
(maybe using yes '' | make oldconfig), some will spend time to research
about this option to find which machines actually have such a device and
if the machine(s) they care about are in this set. This is a waste of
time and opportunities. (And note, this isn't only about people spending
time to decide if they enable or disable PWM_OCORES, this is also about
people who use yes '' because there are too many questions and so they
might miss the handful of useful ones.)

Best regards
Uwe

[1] measured using

	yes '' | make oldconfig

and counting the occurrences of "(NEW)".

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[William Qiu]

On 2023/10/24 19:45, Uwe Kleine-König wrote:
> Hello William,
> 
> On Tue, Oct 24, 2023 at 05:16:49PM +0800, William Qiu wrote:
>> On 2023/10/20 19:25, Uwe Kleine-König wrote:
>> > Hello,
>> > 
>> > On Fri, Oct 20, 2023 at 06:37:39PM +0800, William Qiu wrote:
>> >> Add Pulse Width Modulation driver support for OpenCores.
>> >> 
>> >> Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
>> >> Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
>> >> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>> >> ---
>> >>  MAINTAINERS              |   7 ++
>> >>  drivers/pwm/Kconfig      |  11 ++
>> >>  drivers/pwm/Makefile     |   1 +
>> >>  drivers/pwm/pwm-ocores.c | 211 +++++++++++++++++++++++++++++++++++++++
>> >>  4 files changed, 230 insertions(+)
>> >>  create mode 100644 drivers/pwm/pwm-ocores.c
>> >> 
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index 6c4cce45a09d..321af8fa7aad 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -16003,6 +16003,13 @@ F:	Documentation/i2c/busses/i2c-ocores.rst
>> >>  F:	drivers/i2c/busses/i2c-ocores.c
>> >>  F:	include/linux/platform_data/i2c-ocores.h
>> >> 
>> >> +OPENCORES PWM DRIVER
>> >> +M:	William Qiu <william.qiu@starfivetech.com>
>> >> +M:	Hal Feng <hal.feng@starfivetech.com>
>> >> +S:	Supported
>> >> +F:	Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
>> >> +F:	drivers/pwm/pwm-ocores.c
>> >> +
>> >>  OPENRISC ARCHITECTURE
>> >>  M:	Jonas Bonn <jonas@southpole.se>
>> >>  M:	Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
>> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> >> index 8ebcddf91f7b..cbfbf227d957 100644
>> >> --- a/drivers/pwm/Kconfig
>> >> +++ b/drivers/pwm/Kconfig
>> >> @@ -434,6 +434,17 @@ config PWM_NTXEC
>> >>  	  controller found in certain e-book readers designed by the original
>> >>  	  design manufacturer Netronix.
>> >> 
>> >> +config PWM_OCORES
>> >> +	tristate "Opencores PWM support"
>> >> +	depends on HAS_IOMEM && OF
>> >> +	depends on COMMON_CLK && RESET_CONTROLLER
>> > 
>> > Would it make sense to add something like:
>> > 
>> > 	depends on ARCH_SOMETHING || COMPILE_TEST
>> > 
>> > here?
>> > 
>> But there is no mention of architectural limitations in the OpenCores's
>> specification.
> 
> I already guessed that. Still it probably makes no sense to enable that
> option on most machines. The PWM device found in i.MX SoCs can
> theoretically also be implemented on AT91 or S390x. In practice it
> isn't, so there is a dependency on ARCH_MXC || COMPILE_TEST.
> 
> Consider the role of someone who does a kernel bump for a certain
> machine (on one end of the spectrum) or a distribution kernel (on the
> other end).
> 
> If you take a 6.5 x86_64 allmodconfig + COMPILE_TEST=n and upgrade to
> v6.6-rc7 and do an oldconfig, you get 90 questions[1].
> 
> Just looking quickly through this list, among them are:
> 
> 	DRM support for Loongson Graphics (DRM_LOONGSON) [N/m/?] (NEW) 
> 	Xilinx AXI DMAS Engine (XILINX_DMA) [N/m/y/?] (NEW)
> 	Clock driver for Renesas VersaClock 3 devices (COMMON_CLK_VC3) [N/m/y/?] (NEW)
> 	Realtek RT1017 SDCA Codec - SDW (SND_SOC_RT1017_SDCA_SDW) [N/m/?] (NEW)
> 
> I didn't check in detail and maybe one or the other is valid on x86_64,
> but I'd be surprised if you find two that are sensible to enable on
> x86_64 to support a real machine.
> 
> While I think Kconfig cannot be held responsible to only allow
> generating "real world sensible" configurations, we should work a bit
> harder to rule out the obvious violators and make it easy for people
> configuring the kernel where sensible.
> 
> In my book it's better to have a too strong dependency at first for a
> new driver (but allow it with COMPILE_TEST). Someone who as a device
> needing that driver will find it out and speak up. However if you allow
> to enable the driver everywhere, many people will disable the driver
> (maybe using yes '' | make oldconfig), some will spend time to research
> about this option to find which machines actually have such a device and
> if the machine(s) they care about are in this set. This is a waste of
> time and opportunities. (And note, this isn't only about people spending
> time to decide if they enable or disable PWM_OCORES, this is also about
> people who use yes '' because there are too many questions and so they
> might miss the handful of useful ones.)
> 
> Best regards
> Uwe
> 
> [1] measured using
> 
> 	yes '' | make oldconfig
> 
> and counting the occurrences of "(NEW)".
> 
I see, I'll think about it.
Maybe depend on STARFIVE'S SoCs first?

Best regards,
William

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 390 bytes --]

Hello William,

On Fri, Oct 27, 2023 at 06:23:58PM +0800, William Qiu wrote:
> [...]
> Maybe depend on STARFIVE'S SoCs first?

If these are for now the only known implementers, that sounds about
right.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[William Qiu]

On 2023/10/20 19:25, Uwe Kleine-König wrote:
>> +	void __iomem *base = pwm->data->get_ch_base ?
>> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
>> +	u32 period_data, duty_data, ctrl_data;
>> +
>> +	period_data = readl(REG_OCPWM_LRC(base));
>> +	duty_data = readl(REG_OCPWM_HRC(base));
>> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
>> +
>> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
>> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> 
> Please test your driver with PWM_DEBUG enabled. The rounding is wrong
> here.
> 

Hi Uwe,

The conclusion after checking is: when the period or duty_cycle value set
by the user is not divisible (1000000000/49.5M), there will be an error.
This error is due to hardware accuracy. So why is rounding is wrong?
rockchip also has a similar implementation drivers/pwm/ pwm-rockchip.c

Best regards,
William

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2372 bytes --]

Hello William,

On Wed, Nov 01, 2023 at 10:22:44AM +0800, William Qiu wrote:
> 
> 
> On 2023/10/20 19:25, Uwe Kleine-König wrote:
> >> +	void __iomem *base = pwm->data->get_ch_base ?
> >> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
> >> +	u32 period_data, duty_data, ctrl_data;
> >> +
> >> +	period_data = readl(REG_OCPWM_LRC(base));
> >> +	duty_data = readl(REG_OCPWM_HRC(base));
> >> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
> >> +
> >> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
> >> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> > 
> > Please test your driver with PWM_DEBUG enabled. The rounding is wrong
> > here.
> 
> The conclusion after checking is: when the period or duty_cycle value set
> by the user is not divisible (1000000000/49.5M), there will be an error.
> This error is due to hardware accuracy. So why is rounding is wrong?
> rockchip also has a similar implementation drivers/pwm/ pwm-rockchip.c

I fail to follow. Where is an error?

The general policy (for new drivers at least) is to implement the
biggest period possible not bigger than the requested period. That means
that .apply must round down and to make .apply ∘ .get_state idempotent
.get_state must round up to match.

Assuming a clkrate of 49500000 Hz the actual period for REG_OCPWM_LRC =
400 is 8080.808ns and for REG_OCPWM_LRC = 401 is 8101.010.

So with REG_OCPWM_LRC = 401 .get_state should report state.period = 8102
[ns] because if you call .apply with .period = 8101 [ns] you're supposed
to use REG_OCPWM_LRC = 400.

Rounding using DIV_ROUND_CLOSEST doesn't give consistent behaviour in
some cases. Consider a PWM that can implement the following periods (and
none in between):

	20.1 ns
	20.4 ns
	21.7 ns

With round-to-nearest a request to configure 21 ns will yield 20.4 ns.
If you call .get_state there the driver will return 20 ns. However
configuring 20 ns results in a period of 20.1 ns.

With rounding as requested above you get a consistent behaviour. After
.apply_state(period=21) .get_state() returns period=21.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support

[William Qiu]

On 2023/11/2 19:30, Uwe Kleine-König wrote:
> Hello William,
> 
> On Wed, Nov 01, 2023 at 10:22:44AM +0800, William Qiu wrote:
>> 
>> 
>> On 2023/10/20 19:25, Uwe Kleine-König wrote:
>> >> +	void __iomem *base = pwm->data->get_ch_base ?
>> >> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
>> >> +	u32 period_data, duty_data, ctrl_data;
>> >> +
>> >> +	period_data = readl(REG_OCPWM_LRC(base));
>> >> +	duty_data = readl(REG_OCPWM_HRC(base));
>> >> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
>> >> +
>> >> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
>> >> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
>> > 
>> > Please test your driver with PWM_DEBUG enabled. The rounding is wrong
>> > here.
>> 
>> The conclusion after checking is: when the period or duty_cycle value set
>> by the user is not divisible (1000000000/49.5M), there will be an error.
>> This error is due to hardware accuracy. So why is rounding is wrong?
>> rockchip also has a similar implementation drivers/pwm/ pwm-rockchip.c
> 
> I fail to follow. Where is an error?
> 
> The general policy (for new drivers at least) is to implement the
> biggest period possible not bigger than the requested period. That means
> that .apply must round down and to make .apply ∘ .get_state idempotent
> .get_state must round up to match.
> 
> Assuming a clkrate of 49500000 Hz the actual period for REG_OCPWM_LRC =
> 400 is 8080.808ns and for REG_OCPWM_LRC = 401 is 8101.010.
> 
> So with REG_OCPWM_LRC = 401 .get_state should report state.period = 8102
> [ns] because if you call .apply with .period = 8101 [ns] you're supposed
> to use REG_OCPWM_LRC = 400.
> 
> Rounding using DIV_ROUND_CLOSEST doesn't give consistent behaviour in
> some cases. Consider a PWM that can implement the following periods (and
> none in between):
> 
> 	20.1 ns
> 	20.4 ns
> 	21.7 ns
> 
> With round-to-nearest a request to configure 21 ns will yield 20.4 ns.
> If you call .get_state there the driver will return 20 ns. However
> configuring 20 ns results in a period of 20.1 ns.
> 
> With rounding as requested above you get a consistent behaviour. After
> .apply_state(period=21) .get_state() returns period=21.
> 
> Best regards
> Uwe
> 
I see, then we'll use DIV_ROUND_DOWN_ULL for .apply() and DIV_ROUND_UP_ULL
for .get_state().
Thank you for your answer.

Best regards,
William

[PATCH v6 3/4] riscv: dts: starfive: jh7110: Add PWM node and pins configuration

[William Qiu]

Add OpenCores PWM controller node and add PWM pins configuration
on VisionFive 2 board.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../jh7110-starfive-visionfive-2.dtsi         | 22 +++++++++++++++++++
 arch/riscv/boot/dts/starfive/jh7110.dtsi      |  9 ++++++++
 2 files changed, 31 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
index 12ebe9792356..63d16a6a4e12 100644
--- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
@@ -268,6 +268,12 @@ reserved-data@600000 {
 	};
 };
 
+&pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pwm_pins>;
+	status = "okay";
+};
+
 &spi0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&spi0_pins>;
@@ -402,6 +408,22 @@ GPOEN_SYS_SDIO1_DATA3,
 		};
 	};
 
+	pwm_pins: pwm-0 {
+		pwm-pins {
+			pinmux = <GPIOMUX(46, GPOUT_SYS_PWM_CHANNEL0,
+					      GPOEN_SYS_PWM0_CHANNEL0,
+					      GPI_NONE)>,
+				 <GPIOMUX(59, GPOUT_SYS_PWM_CHANNEL1,
+					      GPOEN_SYS_PWM0_CHANNEL1,
+					      GPI_NONE)>;
+			bias-disable;
+			drive-strength = <12>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
 	spi0_pins: spi0-0 {
 		mosi-pins {
 			pinmux = <GPIOMUX(52, GPOUT_SYS_SPI0_TXD,
diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index e85464c328d0..4024165d4538 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -736,6 +736,15 @@ spi6: spi@120a0000 {
 			status = "disabled";
 		};
 
+		pwm: pwm@120d0000 {
+			compatible = "starfive,jh71x0-pwm";
+			reg = <0x0 0x120d0000 0x0 0x10000>;
+			clocks = <&syscrg JH7110_SYSCLK_PWM_APB>;
+			resets = <&syscrg JH7110_SYSRST_PWM_APB>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		sfctemp: temperature-sensor@120e0000 {
 			compatible = "starfive,jh7110-temp";
 			reg = <0x0 0x120e0000 0x0 0x10000>;
-- 
2.34.1

Re: [PATCH v6 3/4] riscv: dts: starfive: jh7110: Add PWM node and pins configuration

[Emil Renner Berthing]

William Qiu wrote:
> Add OpenCores PWM controller node and add PWM pins configuration
> on VisionFive 2 board.
>
> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
> Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
> ---
>  .../jh7110-starfive-visionfive-2.dtsi         | 22 +++++++++++++++++++
>  arch/riscv/boot/dts/starfive/jh7110.dtsi      |  9 ++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> index 12ebe9792356..63d16a6a4e12 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi
> @@ -268,6 +268,12 @@ reserved-data@600000 {
>  	};
>  };
>
> +&pwm {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pwm_pins>;
> +	status = "okay";
> +};
> +

Hi William,

I just noticed this node reference is out of order. The references should be
sorted alphabetically.

/Emil

>  &spi0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&spi0_pins>;
> @@ -402,6 +408,22 @@ GPOEN_SYS_SDIO1_DATA3,
>  		};
>  	};
>
> +	pwm_pins: pwm-0 {
> +		pwm-pins {
> +			pinmux = <GPIOMUX(46, GPOUT_SYS_PWM_CHANNEL0,
> +					      GPOEN_SYS_PWM0_CHANNEL0,
> +					      GPI_NONE)>,
> +				 <GPIOMUX(59, GPOUT_SYS_PWM_CHANNEL1,
> +					      GPOEN_SYS_PWM0_CHANNEL1,
> +					      GPI_NONE)>;
> +			bias-disable;
> +			drive-strength = <12>;
> +			input-disable;
> +			input-schmitt-disable;
> +			slew-rate = <0>;
> +		};
> +	};
> +
>  	spi0_pins: spi0-0 {
>  		mosi-pins {
>  			pinmux = <GPIOMUX(52, GPOUT_SYS_SPI0_TXD,
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index e85464c328d0..4024165d4538 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -736,6 +736,15 @@ spi6: spi@120a0000 {
>  			status = "disabled";
>  		};
>
> +		pwm: pwm@120d0000 {
> +			compatible = "starfive,jh71x0-pwm";
> +			reg = <0x0 0x120d0000 0x0 0x10000>;
> +			clocks = <&syscrg JH7110_SYSCLK_PWM_APB>;
> +			resets = <&syscrg JH7110_SYSRST_PWM_APB>;
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +
>  		sfctemp: temperature-sensor@120e0000 {
>  			compatible = "starfive,jh7110-temp";
>  			reg = <0x0 0x120e0000 0x0 0x10000>;
> --
> 2.34.1
>

[PATCH v6 4/4] riscv: dts: starfive: jh7100: Add PWM node and pins configuration

[William Qiu]

Add OpenCores PWM controller node and add PWM pins configuration
on VisionFive 1 board.

Signed-off-by: William Qiu <william.qiu@starfivetech.com>
Reviewed-by: Hal Feng <hal.feng@starfivetech.com>
---
 .../boot/dts/starfive/jh7100-common.dtsi      | 24 +++++++++++++++++++
 arch/riscv/boot/dts/starfive/jh7100.dtsi      |  9 +++++++
 2 files changed, 33 insertions(+)

diff --git a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
index b93ce351a90f..11876906cc05 100644
--- a/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100-common.dtsi
@@ -84,6 +84,24 @@ GPO_I2C2_PAD_SDA_OEN,
 		};
 	};
 
+	pwm_pins: pwm-0 {
+		pwm-pins {
+			pinmux = <GPIOMUX(7,
+				  GPO_PWM_PAD_OUT_BIT0,
+				  GPO_PWM_PAD_OE_N_BIT0,
+				  GPI_NONE)>,
+				 <GPIOMUX(5,
+				  GPO_PWM_PAD_OUT_BIT1,
+				  GPO_PWM_PAD_OE_N_BIT1,
+				  GPI_NONE)>;
+			bias-disable;
+			drive-strength = <35>;
+			input-disable;
+			input-schmitt-disable;
+			slew-rate = <0>;
+		};
+	};
+
 	uart3_pins: uart3-0 {
 		rx-pins {
 			pinmux = <GPIOMUX(13, GPO_LOW, GPO_DISABLE,
@@ -154,6 +172,12 @@ &osc_aud {
 	clock-frequency = <27000000>;
 };
 
+&pwm {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pwm_pins>;
+	status = "okay";
+};
+
 &uart3 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&uart3_pins>;
diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi
index 35ab54fb235f..0bb9e2e5ae68 100644
--- a/arch/riscv/boot/dts/starfive/jh7100.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi
@@ -274,6 +274,15 @@ watchdog@12480000 {
 				 <&rstgen JH7100_RSTN_WDT>;
 		};
 
+		pwm: pwm@12490000 {
+			compatible = "starfive,jh71x0-pwm";
+			reg = <0x0 0x12490000 0x0 0x10000>;
+			clocks = <&clkgen JH7100_CLK_PWM_APB>;
+			resets = <&rstgen JH7100_RSTN_PWM_APB>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		sfctemp: temperature-sensor@124a0000 {
 			compatible = "starfive,jh7100-temp";
 			reg = <0x0 0x124a0000 0x0 0x10000>;
-- 
2.34.1