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 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.2.
William Qiu (2):
dt-bindings: PWM: Add StarFive PWM module
pwm: starfive: Add PWM driver support
.../bindings/pwm/starfive,jh7110-pwm.yaml | 53 ++++
MAINTAINERS | 7 +
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-starfive-ptc.c | 245 ++++++++++++++++++
5 files changed, 316 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml
create mode 100644 drivers/pwm/pwm-starfive-ptc.c
--
2.34.1
Add documentation to describe StarFive Pulse Width Modulation
controller driver.
Signed-off-by: William Qiu <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
---
.../bindings/pwm/starfive,jh7110-pwm.yaml | 53 +++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml
diff --git a/Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml b/Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml
new file mode 100644
index 000000000000..082b3779fa61
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.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/starfive,jh7110-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive PWM controller
+
+maintainers:
+ - William Qiu <[email protected]>
+
+description:
+ StarFive SoCs contain PWM and when operating in PWM mode, the PTC core generates
+ binary signal with user-programmable low and high periods. Clock source for the
+ PWM can be either system clockor external clock. Each PWM timer block provides 8
+ PWM channels.
+
+allOf:
+ - $ref: pwm.yaml#
+
+properties:
+ compatible:
+ const: starfive,jh7110-pwm
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+ "#pwm-cells":
+ const: 3
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - resets
+
+additionalProperties: false
+
+examples:
+ - |
+ pwm@120d0000 {
+ compatible = "starfive,jh7110-pwm";
+ reg = <0x120d0000 0x10000>;
+ clocks = <&syscrg 121>;
+ resets = <&syscrg 108>;
+ #pwm-cells = <3>;
+ };
--
2.34.1
Add Pulse Width Modulation driver support for StarFive
JH7110 soc.
Signed-off-by: Hal Feng <[email protected]>
Signed-off-by: William Qiu <[email protected]>
---
MAINTAINERS | 7 +
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-starfive-ptc.c | 245 +++++++++++++++++++++++++++++++++
4 files changed, 263 insertions(+)
create mode 100644 drivers/pwm/pwm-starfive-ptc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index ac151975d0d3..efe1811f9501 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19929,6 +19929,13 @@ F: drivers/pinctrl/starfive/pinctrl-starfive-jh71*
F: include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h
F: include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
+STARFIVE JH71X0 PWM DRIVERS
+M: William Qiu <[email protected]>
+M: Hal Feng <[email protected]>
+S: Supported
+F: Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml
+F: drivers/pwm/pwm-starfive-ptc.c
+
STARFIVE JH71X0 RESET CONTROLLER DRIVERS
M: Emil Renner Berthing <[email protected]>
M: Hal Feng <[email protected]>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index dae023d783a2..2307a0099994 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -536,6 +536,16 @@ config PWM_SPRD
To compile this driver as a module, choose M here: the module
will be called pwm-sprd.
+config PWM_STARFIVE_PTC
+ tristate "StarFive PWM PTC support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for StarFive SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-starfive-ptc.
+
config PWM_STI
tristate "STiH4xx PWM support"
depends on ARCH_STI || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..577f69904baa 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
+obj-$(CONFIG_PWM_STARFIVE_PTC) += pwm-starfive-ptc.o
obj-$(CONFIG_PWM_STI) += pwm-sti.o
obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
diff --git a/drivers/pwm/pwm-starfive-ptc.c b/drivers/pwm/pwm-starfive-ptc.c
new file mode 100644
index 000000000000..239df796d240
--- /dev/null
+++ b/drivers/pwm/pwm-starfive-ptc.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM driver for the StarFive JH7110 SoC
+ *
+ * Copyright (C) 2018 StarFive Technology Co., Ltd.
+ */
+
+#include <dt-bindings/pwm/pwm.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/io.h>
+
+/* how many parameters can be transferred to ptc */
+#define OF_PWM_N_CELLS 3
+
+/* PTC Register offsets */
+#define REG_RPTC_CNTR 0x0
+#define REG_RPTC_HRC 0x4
+#define REG_RPTC_LRC 0x8
+#define REG_RPTC_CTRL 0xC
+
+/* Bit for PWM clock */
+#define BIT_PWM_CLOCK_EN 31
+
+/* Bit for clock gen soft reset */
+#define BIT_CLK_GEN_SOFT_RESET 13
+
+#define NS_PER_SECOND 1000000000
+
+/*
+ * Access PTC register (cntr hrc lrc and ctrl),
+ * need to replace PWM_BASE_ADDR
+ */
+#define REG_PTC_BASE_ADDR_SUB(base, N) \
+((base) + (((N) > 3) ? (((N) % 4) * 0x10 + (1 << 15)) : ((N) * 0x10)))
+#define REG_PTC_RPTC_CNTR(base, N) (REG_PTC_BASE_ADDR_SUB(base, N))
+#define REG_PTC_RPTC_HRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x4)
+#define REG_PTC_RPTC_LRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x8)
+#define REG_PTC_RPTC_CTRL(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0xC)
+
+/* PTC_RPTC_CTRL */
+#define PTC_EN BIT(0)
+#define PTC_ECLK BIT(1)
+#define PTC_NEC BIT(2)
+#define PTC_OE BIT(3)
+#define PTC_SIGNLE BIT(4)
+#define PTC_INTE BIT(5)
+#define PTC_INT BIT(6)
+#define PTC_CNTRRST BIT(7)
+#define PTC_CAPTE BIT(8)
+
+struct starfive_pwm_ptc_device {
+ struct pwm_chip chip;
+ struct clk *clk;
+ struct reset_control *rst;
+ void __iomem *regs;
+ int irq;
+ unsigned int approx_freq;/*pwm apb clock frequency*/
+};
+
+static inline
+struct starfive_pwm_ptc_device *chip_to_starfive_ptc(struct pwm_chip *c)
+{
+ return container_of(c, struct starfive_pwm_ptc_device, chip);
+}
+
+static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
+ struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
+ u32 data_lrc, data_hrc;
+ u32 pwm_clk_ns = 0;
+
+ data_lrc = ioread32(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
+ data_hrc = ioread32(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
+
+ pwm_clk_ns = NS_PER_SECOND / pwm->approx_freq;
+
+ state->period = data_lrc * pwm_clk_ns;
+ state->duty_cycle = data_hrc * pwm_clk_ns;
+ state->polarity = PWM_POLARITY_NORMAL;
+ state->enabled = 1;
+
+ return 0;
+}
+
+static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
+ struct pwm_device *dev,
+ struct pwm_state *state)
+{
+ struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
+ u32 data_hrc = 0;
+ u32 data_lrc = 0;
+ u32 period_data = 0;
+ u32 duty_data = 0;
+ s64 multi = pwm->approx_freq;
+ s64 div = NS_PER_SECOND;
+ void __iomem *reg_addr;
+
+ if (state->duty_cycle > state->period)
+ state->duty_cycle = state->period;
+
+ while (multi % 10 == 0 && div % 10 == 0 && multi > 0 && div > 0) {
+ multi /= 10;
+ div /= 10;
+ }
+
+ period_data = (u32)(state->period * multi / div);
+ if (abs(period_data * div / multi - state->period)
+ > abs((period_data + 1) * div / multi - state->period) ||
+ (state->period > 0 && period_data == 0))
+ period_data += 1;
+
+ if (state->enabled) {
+ duty_data = (u32)(state->duty_cycle * multi / div);
+ if (abs(duty_data * div / multi - state->duty_cycle)
+ > abs((duty_data + 1) * div / multi - state->duty_cycle) ||
+ (state->duty_cycle > 0 && duty_data == 0))
+ duty_data += 1;
+ } else {
+ duty_data = 0;
+ }
+
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ data_hrc = period_data - duty_data;
+ else
+ data_hrc = duty_data;
+
+ data_lrc = period_data;
+
+ reg_addr = REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm);
+ iowrite32(data_hrc, reg_addr);
+
+ reg_addr = REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm);
+ iowrite32(data_lrc, reg_addr);
+
+ reg_addr = REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm);
+ iowrite32(0, reg_addr);
+
+ reg_addr = REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm);
+ iowrite32(PTC_EN | PTC_OE, reg_addr);
+
+ return 0;
+}
+
+static const struct pwm_ops starfive_pwm_ptc_ops = {
+ .get_state = starfive_pwm_ptc_get_state,
+ .apply = (void *)starfive_pwm_ptc_apply,
+ .owner = THIS_MODULE,
+};
+
+static int starfive_pwm_ptc_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct starfive_pwm_ptc_device *pwm;
+ struct pwm_chip *chip;
+ int ret;
+
+ pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ chip = &pwm->chip;
+ chip->dev = dev;
+ chip->ops = &starfive_pwm_ptc_ops;
+ chip->npwm = 8;
+
+ chip->of_pwm_n_cells = OF_PWM_N_CELLS;
+ chip->base = -1;
+
+ 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(dev, NULL);
+ if (IS_ERR(pwm->clk))
+ return dev_err_probe(dev, PTR_ERR(pwm->clk),
+ "Unable to get pwm clock\n");
+
+ pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
+ if (IS_ERR(pwm->rst))
+ return dev_err_probe(dev, PTR_ERR(pwm->rst),
+ "Unable to get pwm reset\n");
+
+ ret = clk_prepare_enable(pwm->clk);
+ if (ret) {
+ dev_err(dev,
+ "Failed to enable pwm clock, %d\n", ret);
+ return ret;
+ }
+
+ reset_control_deassert(pwm->rst);
+
+ pwm->approx_freq = (unsigned int)clk_get_rate(pwm->clk);
+ if (!pwm->approx_freq)
+ dev_err(dev, "get pwm apb clock rate failed.\n");
+
+ ret = devm_pwmchip_add(dev, chip);
+ if (ret < 0) {
+ dev_err(dev, "cannot register PTC: %d\n", ret);
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+ }
+
+ platform_set_drvdata(pdev, pwm);
+
+ return 0;
+}
+
+static int starfive_pwm_ptc_remove(struct platform_device *dev)
+{
+ struct starfive_pwm_ptc_device *pwm = platform_get_drvdata(dev);
+ struct pwm_chip *chip = &pwm->chip;
+
+ pwmchip_remove(chip);
+
+ return 0;
+}
+
+static const struct of_device_id starfive_pwm_ptc_of_match[] = {
+ { .compatible = "starfive,jh7110-pwm" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, starfive_pwm_ptc_of_match);
+
+static struct platform_driver starfive_pwm_ptc_driver = {
+ .probe = starfive_pwm_ptc_probe,
+ .remove = starfive_pwm_ptc_remove,
+ .driver = {
+ .name = "pwm-starfive-ptc",
+ .of_match_table = starfive_pwm_ptc_of_match,
+ },
+};
+module_platform_driver(starfive_pwm_ptc_driver);
+
+MODULE_AUTHOR("Jenny Zhang <[email protected]>");
+MODULE_AUTHOR("Hal Feng <[email protected]>");
+MODULE_DESCRIPTION("StarFive PWM PTC driver");
+MODULE_LICENSE("GPL");
--
2.34.1
On Tue, Mar 21, 2023 at 01:52:28PM +0800, William Qiu wrote:
> Add Pulse Width Modulation driver support for StarFive
> JH7110 soc.
s/soc/SoC/
>
> Signed-off-by: Hal Feng <[email protected]>
> Signed-off-by: William Qiu <[email protected]>
> ---
> MAINTAINERS | 7 +
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-starfive-ptc.c | 245 +++++++++++++++++++++++++++++++++
> 4 files changed, 263 insertions(+)
> create mode 100644 drivers/pwm/pwm-starfive-ptc.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ac151975d0d3..efe1811f9501 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19929,6 +19929,13 @@ F: drivers/pinctrl/starfive/pinctrl-starfive-jh71*
> F: include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h
> F: include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
>
> +STARFIVE JH71X0 PWM DRIVERS
> +M: William Qiu <[email protected]>
> +M: Hal Feng <[email protected]>
> +S: Supported
> +F: Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml
> +F: drivers/pwm/pwm-starfive-ptc.c
> +
> STARFIVE JH71X0 RESET CONTROLLER DRIVERS
> M: Emil Renner Berthing <[email protected]>
> M: Hal Feng <[email protected]>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index dae023d783a2..2307a0099994 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -536,6 +536,16 @@ config PWM_SPRD
> To compile this driver as a module, choose M here: the module
> will be called pwm-sprd.
>
> +config PWM_STARFIVE_PTC
> + tristate "StarFive PWM PTC support"
> + depends on OF
> + depends on COMMON_CLK
You probably want HAS_IOMEM here as well, otherwise this will likely
fail to build on some architectures.
> + help
> + Generic PWM framework driver for StarFive SoCs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-starfive-ptc.
> +
> config PWM_STI
> tristate "STiH4xx PWM support"
> depends on ARCH_STI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 7bf1a29f02b8..577f69904baa 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
> obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
> obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
> +obj-$(CONFIG_PWM_STARFIVE_PTC) += pwm-starfive-ptc.o
> obj-$(CONFIG_PWM_STI) += pwm-sti.o
> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
> diff --git a/drivers/pwm/pwm-starfive-ptc.c b/drivers/pwm/pwm-starfive-ptc.c
> new file mode 100644
> index 000000000000..239df796d240
> --- /dev/null
> +++ b/drivers/pwm/pwm-starfive-ptc.c
> @@ -0,0 +1,245 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PWM driver for the StarFive JH7110 SoC
> + *
> + * Copyright (C) 2018 StarFive Technology Co., Ltd.
> + */
> +
> +#include <dt-bindings/pwm/pwm.h>
You don't use anything from this, nor should you. Just drop it.
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/io.h>
These should be in alphabetic order.
> +
> +/* how many parameters can be transferred to ptc */
> +#define OF_PWM_N_CELLS 3
You use this exactly once, so this definition is useless.
> +
> +/* PTC Register offsets */
> +#define REG_RPTC_CNTR 0x0
> +#define REG_RPTC_HRC 0x4
> +#define REG_RPTC_LRC 0x8
> +#define REG_RPTC_CTRL 0xC
These seem to have been replaced by the REG_PTC_RPTC_* definitions
below. Pick one and drop the other.
> +/* Bit for PWM clock */
> +#define BIT_PWM_CLOCK_EN 31
> +
> +/* Bit for clock gen soft reset */
> +#define BIT_CLK_GEN_SOFT_RESET 13
These two bit definitions seem to be completely unused.
> +
> +#define NS_PER_SECOND 1000000000
Use the standard NSEC_PER_SEC.
> +
> +/*
> + * Access PTC register (cntr hrc lrc and ctrl),
> + * need to replace PWM_BASE_ADDR
> + */
> +#define REG_PTC_BASE_ADDR_SUB(base, N) \
> +((base) + (((N) > 3) ? (((N) % 4) * 0x10 + (1 << 15)) : ((N) * 0x10)))
> +#define REG_PTC_RPTC_CNTR(base, N) (REG_PTC_BASE_ADDR_SUB(base, N))
> +#define REG_PTC_RPTC_HRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x4)
> +#define REG_PTC_RPTC_LRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x8)
> +#define REG_PTC_RPTC_CTRL(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0xC)
> +
> +/* PTC_RPTC_CTRL */
> +#define PTC_EN BIT(0)
> +#define PTC_ECLK BIT(1)
> +#define PTC_NEC BIT(2)
> +#define PTC_OE BIT(3)
> +#define PTC_SIGNLE BIT(4)
> +#define PTC_INTE BIT(5)
> +#define PTC_INT BIT(6)
> +#define PTC_CNTRRST BIT(7)
> +#define PTC_CAPTE BIT(8)
> +
> +struct starfive_pwm_ptc_device {
> + struct pwm_chip chip;
> + struct clk *clk;
> + struct reset_control *rst;
> + void __iomem *regs;
> + int irq;
> + unsigned int approx_freq;/*pwm apb clock frequency*/
No need for aligning these with tabs. Single space is enough for each of
these.
Also, you're dealing with potentially large numbers here, so best to
make the approx_freq unsigned long or perhaps even u64.
> +};
> +
> +static inline
> +struct starfive_pwm_ptc_device *chip_to_starfive_ptc(struct pwm_chip *c)
> +{
> + return container_of(c, struct starfive_pwm_ptc_device, chip);
> +}
> +
> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
> + struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
> + u32 data_lrc, data_hrc;
> + u32 pwm_clk_ns = 0;
> +
> + data_lrc = ioread32(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
> + data_hrc = ioread32(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
Why ioread32()? It doesn't look like this is getting used in some sort
of I/O port setup, so you probably want readl()/writel() instead.
> +
> + pwm_clk_ns = NS_PER_SECOND / pwm->approx_freq;
> +
> + state->period = data_lrc * pwm_clk_ns;
> + state->duty_cycle = data_hrc * pwm_clk_ns;
Again, you want data_lrc, data_hrc and pwm_clk_ns to be unsigned long
or u64 to avoid overflow.
> + state->polarity = PWM_POLARITY_NORMAL;
> + state->enabled = 1;
So these cannot be turned off? You seem to emulate enabled = false by
setting duty cycle to 0 in starfive_pwm_ptc_apply(), so it's probably
best to mirror that here.
> +
> + return 0;
> +}
> +
> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
> + struct pwm_device *dev,
> + struct pwm_state *state)
> +{
> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
> + u32 data_hrc = 0;
> + u32 data_lrc = 0;
> + u32 period_data = 0;
> + u32 duty_data = 0;
Some of these can be condensed into a single line.
> + s64 multi = pwm->approx_freq;
> + s64 div = NS_PER_SECOND;
NSEC_PER_SEC
> + void __iomem *reg_addr;
> +
> + if (state->duty_cycle > state->period)
> + state->duty_cycle = state->period;
> +
> + while (multi % 10 == 0 && div % 10 == 0 && multi > 0 && div > 0) {
> + multi /= 10;
> + div /= 10;
> + }
> +
> + period_data = (u32)(state->period * multi / div);
You're doing 64-bit multiplications and divisions here, which will
likely trigger a build error on some platforms (typically 32-bit ARM).
You should look using at the various helpers in linux/math64.h.
> + if (abs(period_data * div / multi - state->period)
> + > abs((period_data + 1) * div / multi - state->period) ||
> + (state->period > 0 && period_data == 0))
> + period_data += 1;
We typically write this as period_data++;
> +
> + if (state->enabled) {
> + duty_data = (u32)(state->duty_cycle * multi / div);
> + if (abs(duty_data * div / multi - state->duty_cycle)
> + > abs((duty_data + 1) * div / multi - state->duty_cycle) ||
> + (state->duty_cycle > 0 && duty_data == 0))
> + duty_data += 1;
Same here. You may also want to create temporary variables for those
abs() parameters (or the result) to make this a bit more readable.
> + } else {
> + duty_data = 0;
> + }
> +
> + if (state->polarity == PWM_POLARITY_NORMAL)
> + data_hrc = period_data - duty_data;
> + else
> + data_hrc = duty_data;
That's not how we do polarity inversion. If you need period - duty for
normal polarity, that probably indicates that your PWM supports inverse
polarity natively. Furthermore the above seems to only consider the
output power when reversing polarity, which is not correct. If you need
this for the likes of pwm-fan, then you should look at inverting the
polarity in those drivers.
> +
> + data_lrc = period_data;
> +
> + reg_addr = REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm);
> + iowrite32(data_hrc, reg_addr);
> +
> + reg_addr = REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm);
> + iowrite32(data_lrc, reg_addr);
> +
> + reg_addr = REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm);
> + iowrite32(0, reg_addr);
> +
> + reg_addr = REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm);
> + iowrite32(PTC_EN | PTC_OE, reg_addr);
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops starfive_pwm_ptc_ops = {
> + .get_state = starfive_pwm_ptc_get_state,
> + .apply = (void *)starfive_pwm_ptc_apply,
Why do you need to cast this? Also, drop the extra padding.
> + .owner = THIS_MODULE,
> +};
> +
> +static int starfive_pwm_ptc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct starfive_pwm_ptc_device *pwm;
> + struct pwm_chip *chip;
> + int ret;
> +
> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + chip = &pwm->chip;
> + chip->dev = dev;
> + chip->ops = &starfive_pwm_ptc_ops;
> + chip->npwm = 8;
> +
> + chip->of_pwm_n_cells = OF_PWM_N_CELLS;
Simply use the literal "3" here. It's sufficiently clear from the
context what this means, so it's not a "magic" value or anything.
> + chip->base = -1;
This is no longer needed.
> +
> + 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");
The string on the second line should be aligned with "dev" from the
first line. Same for the errors below.
> +
> + pwm->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(pwm->clk))
> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
> + "Unable to get pwm clock\n");
> +
> + pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
> + if (IS_ERR(pwm->rst))
> + return dev_err_probe(dev, PTR_ERR(pwm->rst),
> + "Unable to get pwm reset\n");
> +
> + ret = clk_prepare_enable(pwm->clk);
> + if (ret) {
> + dev_err(dev,
> + "Failed to enable pwm clock, %d\n", ret);
s/pwm/PWM/ in the strings above. And why not use dev_err_probe() here as
well?
> + return ret;
> + }
> +
> + reset_control_deassert(pwm->rst);
> +
> + pwm->approx_freq = (unsigned int)clk_get_rate(pwm->clk);
Drop the cast. It's not needed.
> + if (!pwm->approx_freq)
> + dev_err(dev, "get pwm apb clock rate failed.\n");
Don't you want to make this fatal? If not, you'll end up dividing by
zero in ->get_state(). Also, you may want to reword the error message to
be more in line with the others in this function. Perhaps something
like:
dev_err(dev, "failed to get APB clock rate\n");
> +
> + ret = devm_pwmchip_add(dev, chip);
> + if (ret < 0) {
> + dev_err(dev, "cannot register PTC: %d\n", ret);
> + clk_disable_unprepare(pwm->clk);
Maybe reset_control_assert() here as well?
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, pwm);
> +
> + return 0;
> +}
> +
> +static int starfive_pwm_ptc_remove(struct platform_device *dev)
> +{
> + struct starfive_pwm_ptc_device *pwm = platform_get_drvdata(dev);
> + struct pwm_chip *chip = &pwm->chip;
> +
> + pwmchip_remove(chip);
No need for the temporary variable, you can pass &pwm->chip directly to
pwmchip_remove().
> +
> + return 0;
> +}
You may want to use the ->remove_new() callback instead since the error
code return is misleading.
Although, I just noticed that you use devm_pwmchip_add(), so there
should be no need for the remove callback at all.
Thierry
> +
> +static const struct of_device_id starfive_pwm_ptc_of_match[] = {
> + { .compatible = "starfive,jh7110-pwm" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, starfive_pwm_ptc_of_match);
> +
> +static struct platform_driver starfive_pwm_ptc_driver = {
> + .probe = starfive_pwm_ptc_probe,
> + .remove = starfive_pwm_ptc_remove,
> + .driver = {
> + .name = "pwm-starfive-ptc",
> + .of_match_table = starfive_pwm_ptc_of_match,
> + },
> +};
> +module_platform_driver(starfive_pwm_ptc_driver);
> +
> +MODULE_AUTHOR("Jenny Zhang <[email protected]>");
> +MODULE_AUTHOR("Hal Feng <[email protected]>");
> +MODULE_DESCRIPTION("StarFive PWM PTC driver");
> +MODULE_LICENSE("GPL");
> --
> 2.34.1
>
On 2023/4/6 22:19, Thierry Reding wrote:
> On Tue, Mar 21, 2023 at 01:52:28PM +0800, William Qiu wrote:
>> Add Pulse Width Modulation driver support for StarFive
>> JH7110 soc.
>
> s/soc/SoC/
>
Will update.
>>
>> Signed-off-by: Hal Feng <[email protected]>
>> Signed-off-by: William Qiu <[email protected]>
>> ---
>> MAINTAINERS | 7 +
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-starfive-ptc.c | 245 +++++++++++++++++++++++++++++++++
>> 4 files changed, 263 insertions(+)
>> create mode 100644 drivers/pwm/pwm-starfive-ptc.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ac151975d0d3..efe1811f9501 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19929,6 +19929,13 @@ F: drivers/pinctrl/starfive/pinctrl-starfive-jh71*
>> F: include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h
>> F: include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
>>
>> +STARFIVE JH71X0 PWM DRIVERS
>> +M: William Qiu <[email protected]>
>> +M: Hal Feng <[email protected]>
>> +S: Supported
>> +F: Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml
>> +F: drivers/pwm/pwm-starfive-ptc.c
>> +
>> STARFIVE JH71X0 RESET CONTROLLER DRIVERS
>> M: Emil Renner Berthing <[email protected]>
>> M: Hal Feng <[email protected]>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index dae023d783a2..2307a0099994 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -536,6 +536,16 @@ config PWM_SPRD
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-sprd.
>>
>> +config PWM_STARFIVE_PTC
>> + tristate "StarFive PWM PTC support"
>> + depends on OF
>> + depends on COMMON_CLK
>
> You probably want HAS_IOMEM here as well, otherwise this will likely
> fail to build on some architectures.
>
Will update.
>> + help
>> + Generic PWM framework driver for StarFive SoCs.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-starfive-ptc.
>> +
>> config PWM_STI
>> tristate "STiH4xx PWM support"
>> depends on ARCH_STI || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 7bf1a29f02b8..577f69904baa 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
>> obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
>> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
>> obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
>> +obj-$(CONFIG_PWM_STARFIVE_PTC) += pwm-starfive-ptc.o
>> obj-$(CONFIG_PWM_STI) += pwm-sti.o
>> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
>> diff --git a/drivers/pwm/pwm-starfive-ptc.c b/drivers/pwm/pwm-starfive-ptc.c
>> new file mode 100644
>> index 000000000000..239df796d240
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-starfive-ptc.c
>> @@ -0,0 +1,245 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PWM driver for the StarFive JH7110 SoC
>> + *
>> + * Copyright (C) 2018 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <dt-bindings/pwm/pwm.h>
>
> You don't use anything from this, nor should you. Just drop it.
>
Will drop.
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/reset.h>
>> +#include <linux/io.h>
>
> These should be in alphabetic order.
>
Will update.
>> +
>> +/* how many parameters can be transferred to ptc */
>> +#define OF_PWM_N_CELLS 3
>
> You use this exactly once, so this definition is useless.
>
Will drop.
>> +
>> +/* PTC Register offsets */
>> +#define REG_RPTC_CNTR 0x0
>> +#define REG_RPTC_HRC 0x4
>> +#define REG_RPTC_LRC 0x8
>> +#define REG_RPTC_CTRL 0xC
>
> These seem to have been replaced by the REG_PTC_RPTC_* definitions
> below. Pick one and drop the other.
>
Will drop one of it.
>> +/* Bit for PWM clock */
>> +#define BIT_PWM_CLOCK_EN 31
>> +
>> +/* Bit for clock gen soft reset */
>> +#define BIT_CLK_GEN_SOFT_RESET 13
>
> These two bit definitions seem to be completely unused.
>
Will drop.
>> +
>> +#define NS_PER_SECOND 1000000000
>
> Use the standard NSEC_PER_SEC.
>
Will drop.
>> +
>> +/*
>> + * Access PTC register (cntr hrc lrc and ctrl),
>> + * need to replace PWM_BASE_ADDR
>> + */
>> +#define REG_PTC_BASE_ADDR_SUB(base, N) \
>> +((base) + (((N) > 3) ? (((N) % 4) * 0x10 + (1 << 15)) : ((N) * 0x10)))
>> +#define REG_PTC_RPTC_CNTR(base, N) (REG_PTC_BASE_ADDR_SUB(base, N))
>> +#define REG_PTC_RPTC_HRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x4)
>> +#define REG_PTC_RPTC_LRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x8)
>> +#define REG_PTC_RPTC_CTRL(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0xC)
>> +
>> +/* PTC_RPTC_CTRL */
>> +#define PTC_EN BIT(0)
>> +#define PTC_ECLK BIT(1)
>> +#define PTC_NEC BIT(2)
>> +#define PTC_OE BIT(3)
>> +#define PTC_SIGNLE BIT(4)
>> +#define PTC_INTE BIT(5)
>> +#define PTC_INT BIT(6)
>> +#define PTC_CNTRRST BIT(7)
>> +#define PTC_CAPTE BIT(8)
>> +
>> +struct starfive_pwm_ptc_device {
>> + struct pwm_chip chip;
>> + struct clk *clk;
>> + struct reset_control *rst;
>> + void __iomem *regs;
>> + int irq;
>> + unsigned int approx_freq;/*pwm apb clock frequency*/
>
> No need for aligning these with tabs. Single space is enough for each of
> these.
>
> Also, you're dealing with potentially large numbers here, so best to
> make the approx_freq unsigned long or perhaps even u64.
>
Will update.
>> +};
>> +
>> +static inline
>> +struct starfive_pwm_ptc_device *chip_to_starfive_ptc(struct pwm_chip *c)
>> +{
>> + return container_of(c, struct starfive_pwm_ptc_device, chip);
>> +}
>> +
>> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
>> + struct pwm_device *dev,
>> + struct pwm_state *state)
>> +{
>> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> + u32 data_lrc, data_hrc;
>> + u32 pwm_clk_ns = 0;
>> +
>> + data_lrc = ioread32(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
>> + data_hrc = ioread32(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
>
> Why ioread32()? It doesn't look like this is getting used in some sort
> of I/O port setup, so you probably want readl()/writel() instead.
>
Will fix.
>> +
>> + pwm_clk_ns = NS_PER_SECOND / pwm->approx_freq;
>> +
>> + state->period = data_lrc * pwm_clk_ns;
>> + state->duty_cycle = data_hrc * pwm_clk_ns;
>
> Again, you want data_lrc, data_hrc and pwm_clk_ns to be unsigned long
> or u64 to avoid overflow.
>
Will fix.
>> + state->polarity = PWM_POLARITY_NORMAL;
>> + state->enabled = 1;
>
> So these cannot be turned off? You seem to emulate enabled = false by
> setting duty cycle to 0 in starfive_pwm_ptc_apply(), so it's probably
> best to mirror that here.
>
Will update.
>> +
>> + return 0;
>> +}
>> +
>> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
>> + struct pwm_device *dev,
>> + struct pwm_state *state)
>> +{
>> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> + u32 data_hrc = 0;
>> + u32 data_lrc = 0;
>> + u32 period_data = 0;
>> + u32 duty_data = 0;
>
> Some of these can be condensed into a single line.
>
Will update.
>> + s64 multi = pwm->approx_freq;
>> + s64 div = NS_PER_SECOND;
>
> NSEC_PER_SEC
>
Will update.
>> + void __iomem *reg_addr;
>> +
>> + if (state->duty_cycle > state->period)
>> + state->duty_cycle = state->period;
>> +
>> + while (multi % 10 == 0 && div % 10 == 0 && multi > 0 && div > 0) {
>> + multi /= 10;
>> + div /= 10;
>> + }
>> +
>> + period_data = (u32)(state->period * multi / div);
>
> You're doing 64-bit multiplications and divisions here, which will
> likely trigger a build error on some platforms (typically 32-bit ARM).
> You should look using at the various helpers in linux/math64.h.
>
Will update.
>> + if (abs(period_data * div / multi - state->period)
>> + > abs((period_data + 1) * div / multi - state->period) ||
>> + (state->period > 0 && period_data == 0))
>> + period_data += 1;
>
> We typically write this as period_data++;
>
Will fix.
>> +
>> + if (state->enabled) {
>> + duty_data = (u32)(state->duty_cycle * multi / div);
>> + if (abs(duty_data * div / multi - state->duty_cycle)
>> + > abs((duty_data + 1) * div / multi - state->duty_cycle) ||
>> + (state->duty_cycle > 0 && duty_data == 0))
>> + duty_data += 1;
>
> Same here. You may also want to create temporary variables for those
> abs() parameters (or the result) to make this a bit more readable.
>
>> + } else {
>> + duty_data = 0;
>> + }
>> +
>> + if (state->polarity == PWM_POLARITY_NORMAL)
>> + data_hrc = period_data - duty_data;
>> + else
>> + data_hrc = duty_data;
>
> That's not how we do polarity inversion. If you need period - duty for
> normal polarity, that probably indicates that your PWM supports inverse
> polarity natively. Furthermore the above seems to only consider the
> output power when reversing polarity, which is not correct. If you need
> this for the likes of pwm-fan, then you should look at inverting the
> polarity in those drivers.
>
In JH7110 SoC, the PWM support inverse polarity natively, so I want to ask
how to achieve polarity inversion.
>> +
>> + data_lrc = period_data;
>> +
>> + reg_addr = REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm);
>> + iowrite32(data_hrc, reg_addr);
>> +
>> + reg_addr = REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm);
>> + iowrite32(data_lrc, reg_addr);
>> +
>> + reg_addr = REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm);
>> + iowrite32(0, reg_addr);
>> +
>> + reg_addr = REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm);
>> + iowrite32(PTC_EN | PTC_OE, reg_addr);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct pwm_ops starfive_pwm_ptc_ops = {
>> + .get_state = starfive_pwm_ptc_get_state,
>> + .apply = (void *)starfive_pwm_ptc_apply,
>
> Why do you need to cast this? Also, drop the extra padding.
>
Will fix and drop the extra padding.
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int starfive_pwm_ptc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct starfive_pwm_ptc_device *pwm;
>> + struct pwm_chip *chip;
>> + int ret;
>> +
>> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + chip = &pwm->chip;
>> + chip->dev = dev;
>> + chip->ops = &starfive_pwm_ptc_ops;
>> + chip->npwm = 8;
>> +
>> + chip->of_pwm_n_cells = OF_PWM_N_CELLS;
>
> Simply use the literal "3" here. It's sufficiently clear from the
> context what this means, so it's not a "magic" value or anything.
>
Will update.
>> + chip->base = -1;
>
> This is no longer needed.
>
Will drop
>> +
>> + 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");
>
> The string on the second line should be aligned with "dev" from the
> first line. Same for the errors below.
>
Will fix.
>> +
>> + pwm->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(pwm->clk))
>> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
>> + "Unable to get pwm clock\n");
>> +
>> + pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
>> + if (IS_ERR(pwm->rst))
>> + return dev_err_probe(dev, PTR_ERR(pwm->rst),
>> + "Unable to get pwm reset\n");
>> +
>> + ret = clk_prepare_enable(pwm->clk);
>> + if (ret) {
>> + dev_err(dev,
>> + "Failed to enable pwm clock, %d\n", ret);
>
> s/pwm/PWM/ in the strings above. And why not use dev_err_probe() here as
> well?
>
Will update. I think, in this case is doing "enable", not probe, so dev_err()
is a better fit.
>> + return ret;
>> + }
>> +
>> + reset_control_deassert(pwm->rst);
>> +
>> + pwm->approx_freq = (unsigned int)clk_get_rate(pwm->clk);
>
> Drop the cast. It's not needed.
>
Will drop.
>> + if (!pwm->approx_freq)
>> + dev_err(dev, "get pwm apb clock rate failed.\n");
>
> Don't you want to make this fatal? If not, you'll end up dividing by
> zero in ->get_state(). Also, you may want to reword the error message to
> be more in line with the others in this function. Perhaps something
> like:
>
> dev_err(dev, "failed to get APB clock rate\n");
>
Will update.
>> +
>> + ret = devm_pwmchip_add(dev, chip);
>> + if (ret < 0) {
>> + dev_err(dev, "cannot register PTC: %d\n", ret);
>> + clk_disable_unprepare(pwm->clk);
>
> Maybe reset_control_assert() here as well?
>
Will add.
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + return 0;
>> +}
>> +
>> +static int starfive_pwm_ptc_remove(struct platform_device *dev)
>> +{
>> + struct starfive_pwm_ptc_device *pwm = platform_get_drvdata(dev);
>> + struct pwm_chip *chip = &pwm->chip;
>> +
>> + pwmchip_remove(chip);
>
> No need for the temporary variable, you can pass &pwm->chip directly to
> pwmchip_remove().
>
>> +
>> + return 0;
>> +}
>
> You may want to use the ->remove_new() callback instead since the error
> code return is misleading.
>
> Although, I just noticed that you use devm_pwmchip_add(), so there
> should be no need for the remove callback at all.
>
Will drop remove callback.
Thanks for taking time to review this patch series and giving useful suggestions.
Best regards,
William
> Thierry
>
>> +
>> +static const struct of_device_id starfive_pwm_ptc_of_match[] = {
>> + { .compatible = "starfive,jh7110-pwm" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, starfive_pwm_ptc_of_match);
>> +
>> +static struct platform_driver starfive_pwm_ptc_driver = {
>> + .probe = starfive_pwm_ptc_probe,
>> + .remove = starfive_pwm_ptc_remove,
>> + .driver = {
>> + .name = "pwm-starfive-ptc",
>> + .of_match_table = starfive_pwm_ptc_of_match,
>> + },
>> +};
>> +module_platform_driver(starfive_pwm_ptc_driver);
>> +
>> +MODULE_AUTHOR("Jenny Zhang <[email protected]>");
>> +MODULE_AUTHOR("Hal Feng <[email protected]>");
>> +MODULE_DESCRIPTION("StarFive PWM PTC driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
On 2023/4/6 22:19, Thierry Reding wrote:
> On Tue, Mar 21, 2023 at 01:52:28PM +0800, William Qiu wrote:
>> Add Pulse Width Modulation driver support for StarFive
>> JH7110 soc.
>
> s/soc/SoC/
>
>>
>> Signed-off-by: Hal Feng <[email protected]>
>> Signed-off-by: William Qiu <[email protected]>
>> ---
>> MAINTAINERS | 7 +
>> drivers/pwm/Kconfig | 10 ++
>> drivers/pwm/Makefile | 1 +
>> drivers/pwm/pwm-starfive-ptc.c | 245 +++++++++++++++++++++++++++++++++
>> 4 files changed, 263 insertions(+)
>> create mode 100644 drivers/pwm/pwm-starfive-ptc.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ac151975d0d3..efe1811f9501 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19929,6 +19929,13 @@ F: drivers/pinctrl/starfive/pinctrl-starfive-jh71*
>> F: include/dt-bindings/pinctrl/pinctrl-starfive-jh7100.h
>> F: include/dt-bindings/pinctrl/starfive,jh7110-pinctrl.h
>>
>> +STARFIVE JH71X0 PWM DRIVERS
>> +M: William Qiu <[email protected]>
>> +M: Hal Feng <[email protected]>
>> +S: Supported
>> +F: Documentation/devicetree/bindings/pwm/starfive,jh7110-pwm.yaml
>> +F: drivers/pwm/pwm-starfive-ptc.c
>> +
>> STARFIVE JH71X0 RESET CONTROLLER DRIVERS
>> M: Emil Renner Berthing <[email protected]>
>> M: Hal Feng <[email protected]>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index dae023d783a2..2307a0099994 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -536,6 +536,16 @@ config PWM_SPRD
>> To compile this driver as a module, choose M here: the module
>> will be called pwm-sprd.
>>
>> +config PWM_STARFIVE_PTC
>> + tristate "StarFive PWM PTC support"
>> + depends on OF
>> + depends on COMMON_CLK
>
> You probably want HAS_IOMEM here as well, otherwise this will likely
> fail to build on some architectures.
>
>> + help
>> + Generic PWM framework driver for StarFive SoCs.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called pwm-starfive-ptc.
>> +
>> config PWM_STI
>> tristate "STiH4xx PWM support"
>> depends on ARCH_STI || COMPILE_TEST
>> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
>> index 7bf1a29f02b8..577f69904baa 100644
>> --- a/drivers/pwm/Makefile
>> +++ b/drivers/pwm/Makefile
>> @@ -49,6 +49,7 @@ obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
>> obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
>> obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
>> obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
>> +obj-$(CONFIG_PWM_STARFIVE_PTC) += pwm-starfive-ptc.o
>> obj-$(CONFIG_PWM_STI) += pwm-sti.o
>> obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
>> obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
>> diff --git a/drivers/pwm/pwm-starfive-ptc.c b/drivers/pwm/pwm-starfive-ptc.c
>> new file mode 100644
>> index 000000000000..239df796d240
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-starfive-ptc.c
>> @@ -0,0 +1,245 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * PWM driver for the StarFive JH7110 SoC
>> + *
>> + * Copyright (C) 2018 StarFive Technology Co., Ltd.
>> + */
>> +
>> +#include <dt-bindings/pwm/pwm.h>
>
> You don't use anything from this, nor should you. Just drop it.
>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/slab.h>
>> +#include <linux/clk.h>
>> +#include <linux/reset.h>
>> +#include <linux/io.h>
>
> These should be in alphabetic order.
>
>> +
>> +/* how many parameters can be transferred to ptc */
>> +#define OF_PWM_N_CELLS 3
>
> You use this exactly once, so this definition is useless.
>
>> +
>> +/* PTC Register offsets */
>> +#define REG_RPTC_CNTR 0x0
>> +#define REG_RPTC_HRC 0x4
>> +#define REG_RPTC_LRC 0x8
>> +#define REG_RPTC_CTRL 0xC
>
> These seem to have been replaced by the REG_PTC_RPTC_* definitions
> below. Pick one and drop the other.
>
>> +/* Bit for PWM clock */
>> +#define BIT_PWM_CLOCK_EN 31
>> +
>> +/* Bit for clock gen soft reset */
>> +#define BIT_CLK_GEN_SOFT_RESET 13
>
> These two bit definitions seem to be completely unused.
>
>> +
>> +#define NS_PER_SECOND 1000000000
>
> Use the standard NSEC_PER_SEC.
>
>> +
>> +/*
>> + * Access PTC register (cntr hrc lrc and ctrl),
>> + * need to replace PWM_BASE_ADDR
>> + */
>> +#define REG_PTC_BASE_ADDR_SUB(base, N) \
>> +((base) + (((N) > 3) ? (((N) % 4) * 0x10 + (1 << 15)) : ((N) * 0x10)))
>> +#define REG_PTC_RPTC_CNTR(base, N) (REG_PTC_BASE_ADDR_SUB(base, N))
>> +#define REG_PTC_RPTC_HRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x4)
>> +#define REG_PTC_RPTC_LRC(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0x8)
>> +#define REG_PTC_RPTC_CTRL(base, N) (REG_PTC_BASE_ADDR_SUB(base, N) + 0xC)
>> +
>> +/* PTC_RPTC_CTRL */
>> +#define PTC_EN BIT(0)
>> +#define PTC_ECLK BIT(1)
>> +#define PTC_NEC BIT(2)
>> +#define PTC_OE BIT(3)
>> +#define PTC_SIGNLE BIT(4)
>> +#define PTC_INTE BIT(5)
>> +#define PTC_INT BIT(6)
>> +#define PTC_CNTRRST BIT(7)
>> +#define PTC_CAPTE BIT(8)
>> +
>> +struct starfive_pwm_ptc_device {
>> + struct pwm_chip chip;
>> + struct clk *clk;
>> + struct reset_control *rst;
>> + void __iomem *regs;
>> + int irq;
>> + unsigned int approx_freq;/*pwm apb clock frequency*/
>
> No need for aligning these with tabs. Single space is enough for each of
> these.
>
> Also, you're dealing with potentially large numbers here, so best to
> make the approx_freq unsigned long or perhaps even u64.
>
>> +};
>> +
>> +static inline
>> +struct starfive_pwm_ptc_device *chip_to_starfive_ptc(struct pwm_chip *c)
>> +{
>> + return container_of(c, struct starfive_pwm_ptc_device, chip);
>> +}
>> +
>> +static int starfive_pwm_ptc_get_state(struct pwm_chip *chip,
>> + struct pwm_device *dev,
>> + struct pwm_state *state)
>> +{
>> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> + u32 data_lrc, data_hrc;
>> + u32 pwm_clk_ns = 0;
>> +
>> + data_lrc = ioread32(REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm));
>> + data_hrc = ioread32(REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm));
>
> Why ioread32()? It doesn't look like this is getting used in some sort
> of I/O port setup, so you probably want readl()/writel() instead.
>
>> +
>> + pwm_clk_ns = NS_PER_SECOND / pwm->approx_freq;
>> +
>> + state->period = data_lrc * pwm_clk_ns;
>> + state->duty_cycle = data_hrc * pwm_clk_ns;
>
> Again, you want data_lrc, data_hrc and pwm_clk_ns to be unsigned long
> or u64 to avoid overflow.
>
>> + state->polarity = PWM_POLARITY_NORMAL;
>> + state->enabled = 1;
>
> So these cannot be turned off? You seem to emulate enabled = false by
> setting duty cycle to 0 in starfive_pwm_ptc_apply(), so it's probably
> best to mirror that here.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int starfive_pwm_ptc_apply(struct pwm_chip *chip,
>> + struct pwm_device *dev,
>> + struct pwm_state *state)
>> +{
>> + struct starfive_pwm_ptc_device *pwm = chip_to_starfive_ptc(chip);
>> + u32 data_hrc = 0;
>> + u32 data_lrc = 0;
>> + u32 period_data = 0;
>> + u32 duty_data = 0;
>
> Some of these can be condensed into a single line.
>
>> + s64 multi = pwm->approx_freq;
>> + s64 div = NS_PER_SECOND;
>
> NSEC_PER_SEC
>
>> + void __iomem *reg_addr;
>> +
>> + if (state->duty_cycle > state->period)
>> + state->duty_cycle = state->period;
>> +
>> + while (multi % 10 == 0 && div % 10 == 0 && multi > 0 && div > 0) {
>> + multi /= 10;
>> + div /= 10;
>> + }
>> +
>> + period_data = (u32)(state->period * multi / div);
>
> You're doing 64-bit multiplications and divisions here, which will
> likely trigger a build error on some platforms (typically 32-bit ARM).
> You should look using at the various helpers in linux/math64.h.
>
>> + if (abs(period_data * div / multi - state->period)
>> + > abs((period_data + 1) * div / multi - state->period) ||
>> + (state->period > 0 && period_data == 0))
>> + period_data += 1;
>
> We typically write this as period_data++;
>
>> +
>> + if (state->enabled) {
>> + duty_data = (u32)(state->duty_cycle * multi / div);
>> + if (abs(duty_data * div / multi - state->duty_cycle)
>> + > abs((duty_data + 1) * div / multi - state->duty_cycle) ||
>> + (state->duty_cycle > 0 && duty_data == 0))
>> + duty_data += 1;
>
> Same here. You may also want to create temporary variables for those
> abs() parameters (or the result) to make this a bit more readable.
>
>> + } else {
>> + duty_data = 0;
>> + }
>> +
>> + if (state->polarity == PWM_POLARITY_NORMAL)
>> + data_hrc = period_data - duty_data;
>> + else
>> + data_hrc = duty_data;
>
> That's not how we do polarity inversion. If you need period - duty for
> normal polarity, that probably indicates that your PWM supports inverse
> polarity natively. Furthermore the above seems to only consider the
> output power when reversing polarity, which is not correct. If you need
> this for the likes of pwm-fan, then you should look at inverting the
> polarity in those drivers.
>
>> +
>> + data_lrc = period_data;
>> +
>> + reg_addr = REG_PTC_RPTC_HRC(pwm->regs, dev->hwpwm);
>> + iowrite32(data_hrc, reg_addr);
>> +
>> + reg_addr = REG_PTC_RPTC_LRC(pwm->regs, dev->hwpwm);
>> + iowrite32(data_lrc, reg_addr);
>> +
>> + reg_addr = REG_PTC_RPTC_CNTR(pwm->regs, dev->hwpwm);
>> + iowrite32(0, reg_addr);
>> +
>> + reg_addr = REG_PTC_RPTC_CTRL(pwm->regs, dev->hwpwm);
>> + iowrite32(PTC_EN | PTC_OE, reg_addr);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct pwm_ops starfive_pwm_ptc_ops = {
>> + .get_state = starfive_pwm_ptc_get_state,
>> + .apply = (void *)starfive_pwm_ptc_apply,
>
> Why do you need to cast this? Also, drop the extra padding.
>
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int starfive_pwm_ptc_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct starfive_pwm_ptc_device *pwm;
>> + struct pwm_chip *chip;
>> + int ret;
>> +
>> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + chip = &pwm->chip;
>> + chip->dev = dev;
>> + chip->ops = &starfive_pwm_ptc_ops;
>> + chip->npwm = 8;
>> +
>> + chip->of_pwm_n_cells = OF_PWM_N_CELLS;
>
> Simply use the literal "3" here. It's sufficiently clear from the
> context what this means, so it's not a "magic" value or anything.
>
>> + chip->base = -1;
>
> This is no longer needed.
>
>> +
>> + 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");
>
> The string on the second line should be aligned with "dev" from the
> first line. Same for the errors below.
>
>> +
>> + pwm->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(pwm->clk))
>> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
>> + "Unable to get pwm clock\n");
>> +
>> + pwm->rst = devm_reset_control_get_exclusive(dev, NULL);
>> + if (IS_ERR(pwm->rst))
>> + return dev_err_probe(dev, PTR_ERR(pwm->rst),
>> + "Unable to get pwm reset\n");
>> +
>> + ret = clk_prepare_enable(pwm->clk);
>> + if (ret) {
>> + dev_err(dev,
>> + "Failed to enable pwm clock, %d\n", ret);
>
> s/pwm/PWM/ in the strings above. And why not use dev_err_probe() here as
> well?
>
>> + return ret;
>> + }
>> +
>> + reset_control_deassert(pwm->rst);
>> +
>> + pwm->approx_freq = (unsigned int)clk_get_rate(pwm->clk);
>
> Drop the cast. It's not needed.
>
>> + if (!pwm->approx_freq)
>> + dev_err(dev, "get pwm apb clock rate failed.\n");
>
> Don't you want to make this fatal? If not, you'll end up dividing by
> zero in ->get_state(). Also, you may want to reword the error message to
> be more in line with the others in this function. Perhaps something
> like:
>
> dev_err(dev, "failed to get APB clock rate\n");
>
>> +
>> + ret = devm_pwmchip_add(dev, chip);
>> + if (ret < 0) {
>> + dev_err(dev, "cannot register PTC: %d\n", ret);
>> + clk_disable_unprepare(pwm->clk);
>
> Maybe reset_control_assert() here as well?
>
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + return 0;
>> +}
>> +
>> +static int starfive_pwm_ptc_remove(struct platform_device *dev)
>> +{
>> + struct starfive_pwm_ptc_device *pwm = platform_get_drvdata(dev);
>> + struct pwm_chip *chip = &pwm->chip;
>> +
>> + pwmchip_remove(chip);
>
> No need for the temporary variable, you can pass &pwm->chip directly to
> pwmchip_remove().
>
>> +
>> + return 0;
>> +}
>
> You may want to use the ->remove_new() callback instead since the error
> code return is misleading.
>
> Although, I just noticed that you use devm_pwmchip_add(), so there
> should be no need for the remove callback at all.
>
> Thierry
>
>> +
>> +static const struct of_device_id starfive_pwm_ptc_of_match[] = {
>> + { .compatible = "starfive,jh7110-pwm" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, starfive_pwm_ptc_of_match);
>> +
>> +static struct platform_driver starfive_pwm_ptc_driver = {
>> + .probe = starfive_pwm_ptc_probe,
>> + .remove = starfive_pwm_ptc_remove,
>> + .driver = {
>> + .name = "pwm-starfive-ptc",
>> + .of_match_table = starfive_pwm_ptc_of_match,
>> + },
>> +};
>> +module_platform_driver(starfive_pwm_ptc_driver);
>> +
>> +MODULE_AUTHOR("Jenny Zhang <[email protected]>");
>> +MODULE_AUTHOR("Hal Feng <[email protected]>");
>> +MODULE_DESCRIPTION("StarFive PWM PTC driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.34.1
>>
Hi Thierry,
I'm sorry to bother you. I wonder if you have seen the previous email.
Regarding the realization of polarity inversion, I wonder if you have
any suggestions for me.
Thanks for taking time to give useful suggestions.
Best regards,
William