StarFive's Pulse Width Modulation driver support
Subsystems (4)
v6 cover StarFive's Pulse Width Modulation driver support
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
Add documentation to describe OpenCores Pulse Width Modulation controller driver.
--- .../bindings/pwm/opencores,pwm-ocores.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +53 -0
@@ -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 <[email protected]> + +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>; + };
Signature · 1 line
2.34.1
Conor Dooley
maintainerRe: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module
Krzysztof, William,
Quoted 30 lines · William Qiu
Add documentation to describe OpenCores Pulse Width Modulation controller driver.
Signed-off-by William Qiu <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>Reviewed-by Hal Feng <[email protected]>--- .../bindings/pwm/opencores,pwm-ocores.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yamlDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +13
-0@@ -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 <[email protected]> + +description: + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +10
-0+ 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?
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 24
Quoted 1 line
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +1
-0+ - 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.
Quoted 32 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +29
-0+ + 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>; + };Signature · 1 line
2.34.1
Conor Dooley
maintainerRe: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module
Quoted 51 lines · Conor Dooley
Krzysztof, William,
Quoted 30 lines · William Qiu
Add documentation to describe OpenCores Pulse Width Modulation controller driver.
Signed-off-by William Qiu <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>Reviewed-by Hal Feng <[email protected]>--- .../bindings/pwm/opencores,pwm-ocores.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yamlDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +13
-0@@ -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 <[email protected]> + +description: + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC coreDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +10
-0+ 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-ocoresWhat does the extra "ocores" suffix add, when it just repeats the vendor prefix?
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 24
Quoted 1 line
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +1
-0+ - starfive,jh71x0-pwmKrzysztof, did you approve this generic compatible?
Quoted 7 lines
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)
Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module
Quoted 48 lines · Conor Dooley
Quoted 47 lines · Conor Dooley
Krzysztof, William,
Quoted 30 lines · William Qiu
Add documentation to describe OpenCores Pulse Width Modulation controller driver.
Signed-off-by William Qiu <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>Reviewed-by Hal Feng <[email protected]>--- .../bindings/pwm/opencores,pwm-ocores.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yamlDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +13
-0@@ -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 <[email protected]> + +description: + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC coreDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +10
-0+ 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-ocoresWhat does the extra "ocores" suffix add, when it just repeats the vendor prefix?
Quoted 3 lines
Quoted 3 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 24
Quoted 1 line
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +1
-0+ - starfive,jh71x0-pwmKrzysztof, 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.
Quoted 11 lines
Quoted 8 lines
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
In reply to Conor DooleyQuoted 53 lines · Conor Dooley
Krzysztof, William,
Quoted 30 lines · William Qiu
Add documentation to describe OpenCores Pulse Width Modulation controller driver.
Signed-off-by William Qiu <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>Reviewed-by Hal Feng <[email protected]>--- .../bindings/pwm/opencores,pwm-ocores.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yamlDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +13
-0@@ -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 <[email protected]> + +description: + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC coreDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +10
-0+ 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-ocoresWhat does the extra "ocores" suffix add, when it just repeats the vendor prefix?
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 24
Quoted 1 line
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +1
-0+ - starfive,jh71x0-pwmKrzysztof, did you approve this generic compatible?
And the whole thing looks like it should really be something like
Quoted 9 lines
items:
- enum:
- starfive,jh7100-pwm
- starfive,jh7110-pwm
- const: opencores,pwmCheers,
Conor.
I'm going to use this format.
Thanks,
William
Quoted 32 lines
Quoted 32 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +29
-0+ + 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>; + };Signature · 1 line
2.34.1
Re: [PATCH v6 1/4] dt-bindings: pwm: Add OpenCores PWM module
In reply to William QiuQuoted 5 lines · William Qiu
Add documentation to describe OpenCores Pulse Width Modulation controller driver.
Signed-off-by William Qiu <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>
Please point me where this patch got review?
Quoted 25 lines
Reviewed-by Hal Feng <[email protected]>--- .../bindings/pwm/opencores,pwm-ocores.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yamlDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +13
-0@@ -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 <[email protected]> + +description: + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC core
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +10
-0+ 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
Quoted 9 lines · Krzysztof Kozlowski
Quoted 5 lines · William Qiu
Add documentation to describe OpenCores Pulse Width Modulation controller driver.
Signed-off-by William Qiu <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>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
Quoted 42 lines
Quoted 25 lines
Reviewed-by Hal Feng <[email protected]>--- .../bindings/pwm/opencores,pwm-ocores.yaml | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yamlDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +13
-0@@ -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 <[email protected]> + +description: + OpenCores PTC ip core contains a PWM controller. When operating in PWM mode, the PTC coreDocumentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml line 23
Quoted 10 lines
Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml new file +10
-0+ 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-ocoresNAK. This is not something which received my review.
Best regards,
Krzysztof
PATCH v6 2/4 pwm: opencores: Add PWM driver support
In reply to William QiuAdd Pulse Width Modulation driver support for OpenCores.
--- 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
MAINTAINERS +7 -0
@@ -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 <[email protected]> +M: Hal Feng <[email protected]> +S: Supported +F: Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml +F: drivers/pwm/pwm-ocores.c + OPENRISC ARCHITECTURE M: Jonas Bonn <[email protected]> M: Stefan Kristiansson <[email protected]>
drivers/pwm/Kconfig +11 -0
@@ -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
drivers/pwm/Makefile +1 -0
@@ -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
drivers/pwm/pwm-ocores.c new file +208 -0
208 lines not on this page · raw message
--
2.34.1
Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support
Hello,
Quoted 36 lines · William Qiu
Add Pulse Width Modulation driver support for OpenCores.
Co-developed-by Hal Feng <[email protected]>Signed-off-by Hal Feng <[email protected]>Signed-off-by William Qiu <[email protected]>--- 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.cMAINTAINERS +7
-0@@ -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 <[email protected]> +M: Hal Feng <[email protected]> +S: Supported +F: Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml +F: drivers/pwm/pwm-ocores.c + OPENRISC ARCHITECTURE M: Jonas Bonn <[email protected]> M: Stefan Kristiansson <[email protected]>drivers/pwm/Kconfig +0
-0
drivers/pwm/Kconfig config PWM_NTXEC · line 440
Quoted 7 lines
drivers/pwm/Kconfig +4
-0@@ -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?
Quoted 28 lines
drivers/pwm/Kconfig +7
-0+ 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 OFdrivers/pwm/Makefile +1
-0@@ -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.odrivers/pwm/pwm-ocores.c new file +0
-0
drivers/pwm/pwm-ocores.c line 8
Quoted 8 lines
drivers/pwm/pwm-ocores.c new file +8
-0@@ -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.
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h>
drivers/pwm/pwm-ocores.c line 23
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+#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);
drivers/pwm/pwm-ocores.c line 34
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+/* 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) ...
drivers/pwm/pwm-ocores.c line 46
Quoted 12 lines
drivers/pwm/pwm-ocores.c new file +12
-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.
drivers/pwm/pwm-ocores.c line 50
Quoted 4 lines
drivers/pwm/pwm-ocores.c new file +4
-0+}; + +static inline struct ocores_pwm_device * +chip_to_ocores (struct pwm_chip *chip)
These two lines can go in a single one.
drivers/pwm/pwm-ocores.c line 50
Quoted 1 line
drivers/pwm/pwm-ocores.c new file +1
-0+
please drop this empty line.
drivers/pwm/pwm-ocores.c line 59
Quoted 8 lines
drivers/pwm/pwm-ocores.c new file +8
-0+{ + 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?
drivers/pwm/pwm-ocores.c line 66
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+} + +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".
drivers/pwm/pwm-ocores.c line 76
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+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.
Quoted 9 lines
drivers/pwm/pwm-ocores.c new file +9
-0+ 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)
drivers/pwm/pwm-ocores.c line 95
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+{ +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?
drivers/pwm/pwm-ocores.c line 102
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+ 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.
Quoted 4 lines
drivers/pwm/pwm-ocores.c new file +4
-0+ + ctrl_data =readl (REG_OCPWM_CTRL (base)); +if (state->enabled) +writel (ctrl_data |OCPWM_EN |OCPWM_OE ,REG_OCPWM_CTRL (base));
drivers/pwm/pwm-ocores.c line 116
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+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)
Quoted 43 lines
drivers/pwm/pwm-ocores.c new file +43
-0+}; + +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))
drivers/pwm/pwm-ocores.c line 169
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+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().
drivers/pwm/pwm-ocores.c line 174
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ } + + ret =devm_pwmchip_add (dev, chip); +if (ret <0 ) { +dev_err (dev,"Cannot register PTC: %d\n" , ret);
dev_err_probe()
drivers/pwm/pwm-ocores.c line 174
Quoted 1 line
drivers/pwm/pwm-ocores.c new file +1
-0+clk_disable_unprepare (pwm->clk);
This is wrong, devm_clk_get_enabled() cares for that.
drivers/pwm/pwm-ocores.c line 182
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+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.
drivers/pwm/pwm-ocores.c line 190
Quoted 8 lines
drivers/pwm/pwm-ocores.c new file +8
-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);
Wrong in the same way as the call in .probe()'s error path.
drivers/pwm/pwm-ocores.c line 197
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+ +return 0 ; +} + +static struct platform_driver ocores_pwm_driver = { + .probe = ocores_pwm_probe, + .remove = ocores_pwm_remove,
Please use .remove_new
drivers/pwm/pwm-ocores.c line 205
Quoted 8 lines
drivers/pwm/pwm-ocores.c new file +8
-0+ .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 ?
drivers/pwm/pwm-ocores.c line 208
Quoted 3 lines
drivers/pwm/pwm-ocores.c new file +3
-0+MODULE_AUTHOR ("Hal Feng <[email protected]>" ); +MODULE_DESCRIPTION ("OpenCores PWM PTC driver" ); +MODULE_LICENSE ("GPL" );
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support
Quoted 49 lines · Uwe Kleine-König
Hello,
Quoted 36 lines · William Qiu
Add Pulse Width Modulation driver support for OpenCores.
Co-developed-by Hal Feng <[email protected]>Signed-off-by Hal Feng <[email protected]>Signed-off-by William Qiu <[email protected]>--- 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.cMAINTAINERS +7
-0@@ -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 <[email protected]> +M: Hal Feng <[email protected]> +S: Supported +F: Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml +F: drivers/pwm/pwm-ocores.c + OPENRISC ARCHITECTURE M: Jonas Bonn <[email protected]> M: Stefan Kristiansson <[email protected]>drivers/pwm/Kconfig +0
-0drivers/pwm/Kconfig config PWM_NTXEC · line 440
Quoted 7 lines
drivers/pwm/Kconfig +4
-0@@ -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_CONTROLLERWould it make sense to add something like:
Quoted 4 lines
depends on ARCH_SOMETHING || COMPILE_TESThere?
But there is no mention of architectural limitations in the OpenCores's specification.
Quoted 33 lines
Quoted 33 lines
drivers/pwm/Kconfig +7
-0+ 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 OFdrivers/pwm/Makefile +1
-0@@ -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.odrivers/pwm/pwm-ocores.c new file +5
-0@@ -0,0 +1,211 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* OpenCores PWM Driver +* +* https://opencores.org/projects/ptc
Quoted 9 lines
drivers/pwm/pwm-ocores.c line 8
Quoted 3 lines
drivers/pwm/pwm-ocores.c new file +3
-0+* +* 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.cto make this easy to grep for. It should mention for example that the hardware can only do inverted polarity.
Will add.
Quoted 19 lines
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> drivers/pwm/pwm-ocores.c line 23
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+#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:
Quoted 2 lines
static inline ocores_writel(struct ocores_pwm_device *, unsigned int offset, u32 val);
Will update.
Quoted 14 lines
drivers/pwm/pwm-ocores.c line 34
Quoted 10 lines
drivers/pwm/pwm-ocores.c new file +10
-0+/* 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:
Quoted 3 lines
#define REG_OCPWM_CTRL_EN BIT(0) ...
Will update.
Quoted 6 lines
Quoted 6 lines
drivers/pwm/pwm-ocores.c new file +6
-0+ +struct ocores_pwm_device { +struct pwm_chip chip; +struct clk *clk; +struct reset_control *rst; +const struct ocores_pwm_data *data;
Quoted 9 lines
drivers/pwm/pwm-ocores.c line 46
Quoted 6 lines
drivers/pwm/pwm-ocores.c new file +6
-0+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.
Quoted 8 lines
drivers/pwm/pwm-ocores.c line 50
Quoted 4 lines
drivers/pwm/pwm-ocores.c new file +4
-0+}; + +static inline struct ocores_pwm_device * +chip_to_ocores (struct pwm_chip *chip)These two lines can go in a single one.
Quoted 1 line
drivers/pwm/pwm-ocores.c new file +1
-0+
Will update.
Quoted 3 lines
please drop this empty line.
Will drop.
Quoted 16 lines
drivers/pwm/pwm-ocores.c line 59
Quoted 8 lines
drivers/pwm/pwm-ocores.c new file +8
-0+{ + 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 ...
Quoted 7 lines
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.
Quoted 3 lines
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.
Quoted 12 lines
drivers/pwm/pwm-ocores.c line 66
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+} + +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_deviceand pick something different forocores_pwm_devicevariables. I suggest something like "ddata" or "opd".
Will update.
Quoted 5 lines
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+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));
Quoted 9 lines
drivers/pwm/pwm-ocores.c line 76
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ 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_DEBUGenabled. The rounding is wrong here.
Will check
Quoted 14 lines
Quoted 14 lines
drivers/pwm/pwm-ocores.c new file +14
-0+ 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 ;
Quoted 9 lines
drivers/pwm/pwm-ocores.c line 95
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-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_ULLmight return a value >U32_MAX?
Will check
Quoted 10 lines
drivers/pwm/pwm-ocores.c line 102
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+ 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/ / /
Quoted 7 lines
I assume this is "glitchy", i.e. after updating the
REG_OCPWM_LRCand before updatingREG_OCPWM_HRCthe 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
Quoted 9 lines
Quoted 9 lines
drivers/pwm/pwm-ocores.c new file +9
-0+ + 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 ; +}
Quoted 9 lines
drivers/pwm/pwm-ocores.c line 116
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-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.
Quoted 47 lines
Quoted 47 lines
drivers/pwm/pwm-ocores.c new file +47
-0+}; + +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 );
Quoted 9 lines
drivers/pwm/pwm-ocores.c line 169
Quoted 6 lines
drivers/pwm/pwm-ocores.c new file +6
-0+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 toreset_control_assert().
Will update
Quoted 8 lines
drivers/pwm/pwm-ocores.c line 174
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ } + + ret =devm_pwmchip_add (dev, chip); +if (ret <0 ) { +dev_err (dev,"Cannot register PTC: %d\n" , ret);
dev_err_probe()
Will update
Quoted 4 lines
drivers/pwm/pwm-ocores.c line 174
Quoted 1 line
drivers/pwm/pwm-ocores.c new file +1
-0+clk_disable_unprepare (pwm->clk);This is wrong,
devm_clk_get_enabled()cares for that.
Will update
Quoted 11 lines
drivers/pwm/pwm-ocores.c line 182
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+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.
Quoted 11 lines
drivers/pwm/pwm-ocores.c line 190
Quoted 8 lines
drivers/pwm/pwm-ocores.c new file +8
-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);Wrong in the same way as the call in .probe()'s error path.
Will update.
Quoted 10 lines
drivers/pwm/pwm-ocores.c line 197
Quoted 7 lines
drivers/pwm/pwm-ocores.c new file +7
-0+ +return 0 ; +} + +static struct platform_driver ocores_pwm_driver = { + .probe = ocores_pwm_probe, + .remove = ocores_pwm_remove,Please use .remove_new
Will update.
Quoted 11 lines
drivers/pwm/pwm-ocores.c line 205
Quoted 8 lines
drivers/pwm/pwm-ocores.c new file +8
-0+ .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
Quoted 7 lines
drivers/pwm/pwm-ocores.c line 208
Quoted 3 lines
drivers/pwm/pwm-ocores.c new file +3
-0+MODULE_AUTHOR ("Hal Feng <[email protected]>" ); +MODULE_DESCRIPTION ("OpenCores PWM PTC driver" ); +MODULE_LICENSE ("GPL" );Best regards
Uwe
Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support
Hello William,
Quoted 50 lines · William Qiu
Quoted 49 lines · Uwe Kleine-König
Hello,
Quoted 36 lines · William Qiu
Add Pulse Width Modulation driver support for OpenCores.
Co-developed-by Hal Feng <[email protected]>Signed-off-by Hal Feng <[email protected]>Signed-off-by William Qiu <[email protected]>--- 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.cMAINTAINERS +7
-0@@ -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 <[email protected]> +M: Hal Feng <[email protected]> +S: Supported +F: Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml +F: drivers/pwm/pwm-ocores.c + OPENRISC ARCHITECTURE M: Jonas Bonn <[email protected]> M: Stefan Kristiansson <[email protected]>drivers/pwm/Kconfig +0
-0drivers/pwm/Kconfig config PWM_NTXEC · line 440
Quoted 7 lines
drivers/pwm/Kconfig +4
-0@@ -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_CONTROLLERWould it make sense to add something like:
Quoted 6 lines
Quoted 4 lines
depends on ARCH_SOMETHING || COMPILE_TESThere?
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)".
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support
Quoted 100 lines · Uwe Kleine-König
Hello William,
Quoted 50 lines · William Qiu
Quoted 49 lines · Uwe Kleine-König
Hello,
Quoted 36 lines · William Qiu
Add Pulse Width Modulation driver support for OpenCores.
Co-developed-by Hal Feng <[email protected]>Signed-off-by Hal Feng <[email protected]>Signed-off-by William Qiu <[email protected]>--- 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.cMAINTAINERS +7
-0@@ -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 <[email protected]> +M: Hal Feng <[email protected]> +S: Supported +F: Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml +F: drivers/pwm/pwm-ocores.c + OPENRISC ARCHITECTURE M: Jonas Bonn <[email protected]> M: Stefan Kristiansson <[email protected]>drivers/pwm/Kconfig +0
-0drivers/pwm/Kconfig config PWM_NTXEC · line 440
Quoted 7 lines
drivers/pwm/Kconfig +4
-0@@ -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_CONTROLLERWould it make sense to add something like:
Quoted 6 lines
Quoted 4 lines
depends on ARCH_SOMETHING || COMPILE_TESThere?
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_64allmodconfig + 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 onx86_64to support a real machine.While I think
Kconfigcannot 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 disablePWM_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.)
Quoted 9 lines
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
Hello William,
Quoted 2 lines · William Qiu
[...]
Maybe depend on STARFIVE'S SoCs first?
If these are for now the only known implementers, that sounds about right.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support
In reply to Uwe Kleine-KönigQuoted 5 lines · Uwe Kleine-König
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+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));
Quoted 9 lines
drivers/pwm/pwm-ocores.c line 76
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ 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_DEBUGenabled. 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
Hello William,
Quoted 17 lines · William Qiu
Quoted 5 lines · Uwe Kleine-König
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+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));Quoted 8 lines
drivers/pwm/pwm-ocores.c line 76
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ 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_DEBUGenabled. The rounding is wrong here.
Quoted 4 lines
The conclusion after checking is: when the period or
duty_cyclevalue 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 implementationdrivers/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
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v6 2/4] pwm: opencores: Add PWM driver support
Quoted 51 lines · Uwe Kleine-König
Hello William,
Quoted 17 lines · William Qiu
Quoted 5 lines · Uwe Kleine-König
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+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));Quoted 8 lines
drivers/pwm/pwm-ocores.c line 76
Quoted 5 lines
drivers/pwm/pwm-ocores.c new file +5
-0+ 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_DEBUGenabled. The rounding is wrong here.Quoted 4 lines
The conclusion after checking is: when the period or
duty_cyclevalue 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 implementationdrivers/pwm/pwm-rockchip.cI 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 forREG_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 useREG_OCPWM_LRC= 400.Rounding using
DIV_ROUND_CLOSESTdoesn'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 nsWith 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.
Quoted 6 lines
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
In reply to William QiuAdd OpenCores PWM controller node and add PWM pins configuration on VisionFive 2 board.
--- .../jh7110-starfive-visionfive-2.dtsi | 22 +++++++++++++++++++ arch/riscv/boot/dts/starfive/jh7110.dtsi | 9 ++++++++ 2 files changed, 31 insertions(+)
arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi +22 -0
@@ -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,
arch/riscv/boot/dts/starfive/jh7110.dtsi +9 -0
@@ -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>;
Signature · 1 line
2.34.1
Re: [PATCH v6 3/4] riscv: dts: starfive: jh7110: Add PWM node and pins configuration
Quoted 15 lines · William Qiu
Add OpenCores PWM controller node and add PWM pins configuration on VisionFive 2 board.
Signed-off-by William Qiu <[email protected]>Reviewed-by Hal Feng <[email protected]>--- .../jh7110-starfive-visionfive-2.dtsi | 22 +++++++++++++++++++ arch/riscv/boot/dts/starfive/jh7110.dtsi | 9 ++++++++ 2 files changed, 31 insertions(+)arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi +0
-0
arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi reserved-data@600000 { · line 276
Quoted 9 lines
arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi +6
-0@@ -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
Quoted 49 lines
arch/riscv/boot/dts/starfive/jh7110-starfive-visionfive-2.dtsi +16
-0&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,arch/riscv/boot/dts/starfive/jh7110.dtsi +9
-1@@ -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
In reply to William QiuAdd OpenCores PWM controller node and add PWM pins configuration on VisionFive 1 board.
--- .../boot/dts/starfive/jh7100-common.dtsi | 24 +++++++++++++++++++ arch/riscv/boot/dts/starfive/jh7100.dtsi | 9 +++++++ 2 files changed, 33 insertions(+)
arch/riscv/boot/dts/starfive/jh7100-common.dtsi +24 -0
@@ -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>;
arch/riscv/boot/dts/starfive/jh7100.dtsi +9 -0
@@ -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>;
Signature · 1 line
2.34.1