Add support for Allwinner PWM on D1/T113s/R329 SoCs
Subsystems (4)
v3 cover Add support for Allwinner PWM on D1/T113s/R329 SoCs
Hi,
This series adds support for PWM controller on new Allwinner's SoCs, such as D1, T113s and R329. The implemented driver provides basic functionality for control PWM channels.
v2:
- fix dt-bindings
- fix a remark in the driver
v3:
- fix dt-bindings
- fix sunxi-d1s-t113.dtsi
Aleksandr Shubin (3):
dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
controller
pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
riscv: dts: allwinner: d1: Add pwm node
.../bindings/pwm/allwinner,sun20i-pwm.yaml | 86 +++++ .../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 11 + drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 ++++++++++++++++++ 5 files changed, 430 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml create mode 100644 drivers/pwm/pwm-sun20i.c
Signature · 1 line
2.25.1
PATCH v3 1/3 dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM controller
Allwinner's D1, T113-S3 and R329 SoCs have a new pwm controller witch is different from the previous pwm-sun4i.
The D1 and T113 are identical in terms of peripherals, they differ only in the architecture of the CPU core, and even share the majority of their DT. Because of that, using the same compatible makes sense.
The R329 is a different SoC though, and should have a different compatible string added, especially as there is a difference in the number of channels.
D1 and T113s SoCs have one PWM controller with 8 channels. R329 SoC has two PWM controllers in both power domains, one of them has 9 channels (CPUX one) and the other has 6 (CPUS one).
Add a device tree binding for them.
--- .../bindings/pwm/allwinner,sun20i-pwm.yaml | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml new file +86 -0
@@ -0,0 +1,86 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/pwm/allwinner,sun20i-pwm.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Allwinner D1, T113-S3 and R329 PWM + +maintainers: + - Aleksandr Shubin <[email protected]> + +properties: + compatible: + oneOf: + - const: allwinner,sun20i-d1-pwm + - items: + - const: allwinner,sun20i-r329-pwm + - const: allwinner,sun20i-d1-pwm + + reg: + maxItems: 1 + + "#pwm-cells": + const: 3 + + clocks: + items: + - description: 24 MHz oscillator + - description: Bus Clock + + clock-names: + items: + - const: hosc + - const: bus + + resets: + maxItems: 1 + + allwinner,pwm-channels: + $ref: /schemas/types.yaml#/definitions/uint32 + description: The number of PWM channels configured for this instance + enum: [6, 9] + +allOf: + - $ref: pwm.yaml# + + - if: + properties: + compatible: + contains: + const: allwinner,sun20i-r329-pwm + + then: + required: + - allwinner,pwm-channels + + else: + not: + required: + - allwinner,pwm-channels + +unevaluatedProperties: false + +required: + - compatible + - reg + - "#pwm-cells" + - clocks + - clock-names + - resets + +examples: + - | + #include <dt-bindings/clock/sun20i-d1-ccu.h> + #include <dt-bindings/reset/sun20i-d1-ccu.h> + + pwm: pwm@2000c00 { + compatible = "allwinner,sun20i-d1-pwm"; + reg = <0x02000c00 0x400>; + clocks = <&dcxo>, <&ccu CLK_BUS_PWM>; + clock-names = "hosc", "bus"; + resets = <&ccu RST_BUS_PWM>; + #pwm-cells = <0x3>; + }; + +...
Signature · 1 line
2.25.1
Re: [PATCH v3 1/3] dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM controller
Quoted 11 lines · Aleksandr Shubin
Allwinner's D1, T113-S3 and R329 SoCs have a new pwm controller witch is different from the previous pwm-sun4i.
The D1 and T113 are identical in terms of peripherals, they differ only in the architecture of the CPU core, and even share the majority of their DT. Because of that, using the same compatible makes sense.
The R329 is a different SoC though, and should have a different compatible string added, especially as there is a difference in the number of channels.
Quoted 7 lines
D1 and T113s SoCs have one PWM controller with 8 channels. R329 SoC has two PWM controllers in both power domains, one of them has 9 channels (CPUX one) and the other has 6 (CPUS one).
Add a device tree binding for them.
Signed-off-by Aleksandr Shubin <[email protected]>
Cheers,
Conor.
PATCH v3 2/3 pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
In reply to Aleksandr ShubinAllwinner's D1, T113-S3 and R329 SoCs have a quite different PWM controllers with ones supported by pwm-sun4i driver.
This patch adds a PWM controller driver for Allwinner's D1, T113-S3 and R329 SoCs. The main difference between these SoCs is the number of channels defined by the DT property.
--- drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 333 insertions(+) create mode 100644 drivers/pwm/pwm-sun20i.c
drivers/pwm/Kconfig +10 -0
@@ -594,6 +594,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN20I + tristate "Allwinner D1/T113s/R329 PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on COMMON_CLK + help + Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun20i. + config PWM_SUNPLUS tristate "Sunplus PWM support" depends on ARCH_SUNPLUS || COMPILE_TEST
drivers/pwm/Makefile +1 -0
@@ -55,6 +55,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o +obj-$(CONFIG_PWM_SUN20I) += pwm-sun20i.o obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
drivers/pwm/pwm-sun20i.c new file +322 -0
322 lines not on this page · raw message
Signature · 1 line
2.25.1
Re: [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
On Di, 2023-06-27 at 11:23 +0300, Aleksandr Shubin wrote:
[...]
Quoted 2 lines
(quoted diff) +2
-0+static int sun20i_pwm_probe(struct platform_device *pdev) +{
[...]
drivers/pwm/pwm-sun20i.c line 270
Quoted 1 line
drivers/pwm/pwm-sun20i.c new file +1
-0+ sun20i_chip->rst =devm_reset_control_get (&pdev->dev,NULL );
Please use devm_reset_control_get_exclusive() directly.
regards
Philipp
Re: [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
In reply to Aleksandr ShubinHello,
Quoted 83 lines · Aleksandr Shubin
Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM controllers with ones supported by pwm-sun4i driver.
This patch adds a PWM controller driver for Allwinner's D1, T113-S3 and R329 SoCs. The main difference between these SoCs is the number of channels defined by the DT property.
Signed-off-by Aleksandr Shubin <[email protected]>--- drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 333 insertions(+) create mode 100644 drivers/pwm/pwm-sun20i.cdrivers/pwm/Kconfig +10
-0@@ -594,6 +594,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN20I + tristate "Allwinner D1/T113s/R329 PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on COMMON_CLK + help + Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun20i. + config PWM_SUNPLUS tristate "Sunplus PWM support" depends on ARCH_SUNPLUS || COMPILE_TESTdrivers/pwm/Makefile +1
-0@@ -55,6 +55,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o +obj-$(CONFIG_PWM_SUN20I) += pwm-sun20i.o obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.odrivers/pwm/pwm-sun20i.c new file +29
-0@@ -0,0 +1,322 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329) +* +* Limitations: +* - When the parameters change, current running period will not be completed +* and run new settings immediately. +* - It output HIGH-Z state when PWM channel disabled. +* +* Copyright (c) 2023 Aleksandr Shubin <[email protected]> +*/ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pwm.h> +#include <linux/reset.h> + +#define PWM_CLK_CFG_REG (chan) (0x20 + (((chan) >> 1) * 0x4)) +#define PWM_CLK_SRC GENMASK(8, 7) +#define PWM_CLK_DIV_M GENMASK(3, 0) + +#define PWM_CLK_GATE_REG 0x40 +#define PWM_CLK_BYPASS (chan) BIT((chan) - 16) +#define PWM_CLK_GATING (chan) BIT(chan) +
drivers/pwm/pwm-sun20i.c line 39
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+#define PWM_ENABLE_REG 0x80 +#define PWM_EN (chan) BIT(chan) + +#define PWM_CTL_REG (chan) (0x100 + (chan) * 0x20) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (chan) (0x104 + (chan) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0)
Can you please adapt the register field names to include the register name? I'd use:
#define PWM_CTL(chan) (0x100 + (chan) * 0x20)
#define PWM_CTL_ACT_STA BIT(8)
#define PWM_CTL_PRESCAL_K GENMASK(7, 0)
then you get a chance to spot when PWM_CLK_BYPASS(x) is written to PWM_CLK_CFG.
Quoted 39 lines
drivers/pwm/pwm-sun20i.c new file +39
-0+struct sun20i_pwm_chip { +struct pwm_chip chip; +struct clk *clk_bus, *clk_hosc; +struct reset_control *rst; +void __iomem *base; +/* Mutex to protect pwm apply state */ +struct mutex mutex; +}; + +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun20i_pwm_chip, chip); +} + +static inline u32 sun20i_pwm_readl (struct sun20i_pwm_chip *chip, +unsigned long offset) +{ +return readl (chip->base + offset); +} + +static inline void sun20i_pwm_writel (struct sun20i_pwm_chip *chip, +u32 val,unsigned long offset) +{ +writel (val, chip->base + offset); +} + +static int sun20i_pwm_get_state (struct pwm_chip *chip, +struct pwm_device *pwm, +struct pwm_state *state) +{ +struct sun20i_pwm_chip *sun20i_chip =to_sun20i_pwm_chip (chip); +u64 clk_rate, tmp; +u32 val; +u16 ent_cycle, act_cycle; +u8 prescal, div_id; + +mutex_lock (&sun20i_chip->mutex); + + val =sun20i_pwm_readl (sun20i_chip,PWM_CLK_CFG_REG (pwm->hwpwm));
drivers/pwm/pwm-sun20i.c line 89
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ div_id =FIELD_GET (PWM_CLK_DIV_M , val); +if (FIELD_GET (PWM_CLK_SRC , val) ==0 ) + clk_rate =clk_get_rate (sun20i_chip->clk_hosc); +else + clk_rate =clk_get_rate (sun20i_chip->clk_bus); + + val =sun20i_pwm_readl (sun20i_chip,PWM_CTL_REG (pwm->hwpwm)); + state->polarity = (PWM_ACT_STA & val) ?PWM_POLARITY_NORMAL :PWM_POLARITY_INVERSED ; + + prescal =FIELD_GET (PWM_PRESCAL_K , val) +1 ;
If PWM_PRESCAL_K is 0xff, prescal ends up being 0. This isn't right, is it?
drivers/pwm/pwm-sun20i.c line 99
Quoted 9 lines
drivers/pwm/pwm-sun20i.c new file +9
-0+ val =sun20i_pwm_readl (sun20i_chip,PWM_ENABLE_REG ); + state->enabled = (PWM_EN (pwm->hwpwm) & val) ? true : false; + + val =sun20i_pwm_readl (sun20i_chip,PWM_PERIOD_REG (pwm->hwpwm)); + act_cycle =FIELD_GET (PWM_ACT_CYCLE , val); + ent_cycle =FIELD_GET (PWM_ENTIRE_CYCLE , val); +if (act_cycle > ent_cycle) + act_cycle = ent_cycle; +
A comment that with the width of the used factors this cannot overflow would be nice here.
drivers/pwm/pwm-sun20i.c line 100
Quoted 1 line
drivers/pwm/pwm-sun20i.c new file +1
-0+ tmp = (u64)(act_cycle) * prescal * (1U << div_id) *NSEC_PER_SEC ;
Can be simplified to:
tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;
Quoted 66 lines
drivers/pwm/pwm-sun20i.c new file +66
-0+ state->duty_cycle =DIV_ROUND_UP_ULL (tmp, clk_rate); + tmp = (u64)(ent_cycle) * prescal * (1U << div_id) *NSEC_PER_SEC ; + state->period =DIV_ROUND_UP_ULL (tmp, clk_rate); +mutex_unlock (&sun20i_chip->mutex); + +return 0 ; +} + +static int sun20i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +int ret =0 ; +u32 clk_gate, clk_cfg, pwm_en, ctl, period; +u64 bus_rate, hosc_rate, clk_div, val; +u16 prescaler, div_m; +bool use_bus_clk, calc_div_m; +struct sun20i_pwm_chip *sun20i_chip =to_sun20i_pwm_chip (chip); + +mutex_lock (&sun20i_chip->mutex); + + pwm_en =sun20i_pwm_readl (sun20i_chip,PWM_ENABLE_REG ); + +if (state->enabled != pwm->state.enabled) + clk_gate =sun20i_pwm_readl (sun20i_chip,PWM_CLK_GATE_REG ); + +if (state->enabled != pwm->state.enabled && !state->enabled) { + clk_gate &= ~PWM_CLK_GATING (pwm->hwpwm); + pwm_en &= ~PWM_EN (pwm->hwpwm); +sun20i_pwm_writel (sun20i_chip, pwm_en,PWM_ENABLE_REG ); +sun20i_pwm_writel (sun20i_chip, clk_gate,PWM_CLK_GATE_REG ); + } + +if (state->polarity != pwm->state.polarity || + state->duty_cycle != pwm->state.duty_cycle || + state->period != pwm->state.period) { + ctl =sun20i_pwm_readl (sun20i_chip,PWM_CTL_REG (pwm->hwpwm)); + clk_cfg =sun20i_pwm_readl (sun20i_chip,PWM_CLK_CFG_REG (pwm->hwpwm)); + hosc_rate =clk_get_rate (sun20i_chip->clk_hosc); + bus_rate =clk_get_rate (sun20i_chip->clk_bus); +if (pwm_en &PWM_EN (pwm->hwpwm ^1 )) { +/* if the neighbor channel is enable, check period only */ + use_bus_clk =FIELD_GET (PWM_CLK_SRC , clk_cfg) !=0 ; +if (use_bus_clk) + val = state->period * bus_rate; +else + val = state->period * hosc_rate; +do_div (val,NSEC_PER_SEC ); + + div_m =FIELD_GET (PWM_CLK_DIV_M , clk_cfg); + calc_div_m = false; + }else { +/* check period and select clock source */ + use_bus_clk = false; + val = state->period * hosc_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { + use_bus_clk = true; + val = state->period * bus_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { + ret = -EINVAL ; + goto unlock_mutex; + } + } + div_m =0 ; + calc_div_m = true;
drivers/pwm/pwm-sun20i.c line 176
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ +/* set up the CLK_DIV_M and clock CLK_SRC */ + clk_cfg =FIELD_PREP (PWM_CLK_DIV_M , div_m); + clk_cfg |=FIELD_PREP (PWM_CLK_SRC , use_bus_clk ?1 :0 ); + +sun20i_pwm_writel (sun20i_chip, clk_cfg,PWM_CLK_CFG_REG (pwm->hwpwm)); + } + +/* calculate prescaler, M factor, PWM entire cycle */ + clk_div = val;
This assignment is useless as it is overwritten in the loop below, isn't it?
Quoted 9 lines
drivers/pwm/pwm-sun20i.c new file +9
-0+for (prescaler =0 ;; prescaler++) { +if (prescaler >=256 ) { +if (calc_div_m) { + prescaler =0 ; + div_m++; +if (div_m >=9 ) { + ret = -EINVAL ; + goto unlock_mutex; + }
drivers/pwm/pwm-sun20i.c line 195
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ }else { + ret = -EINVAL ; + goto unlock_mutex; + } + } + + clk_div = val >> div_m; +do_div (clk_div, prescaler +1 ); +if (clk_div <=65534 ) +break ;
This can be calculated without a loop.
Quoted 5 lines
drivers/pwm/pwm-sun20i.c new file +5
-0+ } + + period =FIELD_PREP (PWM_ENTIRE_CYCLE , clk_div); + +/* set duty cycle */
drivers/pwm/pwm-sun20i.c line 210
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+if (use_bus_clk) + val = state->duty_cycle * bus_rate; +else + val = state->duty_cycle * hosc_rate; +do_div (val,NSEC_PER_SEC ); + clk_div = val >> div_m; +do_div (clk_div, prescaler +1 ); + +if (state->duty_cycle == state->period) + clk_div++;
I don't understand that one. Can you explain that in a comment please?
drivers/pwm/pwm-sun20i.c line 218
Quoted 8 lines
drivers/pwm/pwm-sun20i.c new file +8
-0+ period |=FIELD_PREP (PWM_ACT_CYCLE , clk_div); +sun20i_pwm_writel (sun20i_chip, period,PWM_PERIOD_REG (pwm->hwpwm)); + + ctl =FIELD_PREP (PWM_PRESCAL_K , prescaler); +if (state->polarity ==PWM_POLARITY_NORMAL ) + ctl |=PWM_ACT_STA ; + +sun20i_pwm_writel (sun20i_chip, ctl,PWM_CTL_REG (pwm->hwpwm));
Is this racy? I.e. does the write to PWM_PERIOD_REG(pwm->hwpwm) above already has an effect before PWM_CTL_REG(pwm->hwpwm) is written?
drivers/pwm/pwm-sun20i.c line 226
Quoted 8 lines
drivers/pwm/pwm-sun20i.c new file +8
-0+ } + +if (state->enabled != pwm->state.enabled && state->enabled) { + clk_gate &= ~PWM_CLK_BYPASS (pwm->hwpwm); + clk_gate |=PWM_CLK_GATING (pwm->hwpwm); + pwm_en |=PWM_EN (pwm->hwpwm); +sun20i_pwm_writel (sun20i_chip, pwm_en,PWM_ENABLE_REG ); +sun20i_pwm_writel (sun20i_chip, clk_gate,PWM_CLK_GATE_REG );
This is (I guess) racy. If your PWM is running with
.period = 10000 .duty_cyle = 0 .enabled = true
and you configure it to
.period = 10000 .duty_cyle = 10000 .enabled = false
you get a short spike. For a enabled=true -> enabled=false transition you should disable first before configuring duty+period (or skip the latter completely).
drivers/pwm/pwm-sun20i.c line 233
Quoted 7 lines
drivers/pwm/pwm-sun20i.c new file +7
-0+ } + +unlock_mutex: +mutex_unlock (&sun20i_chip->mutex); + +return ret; +}
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
Hi Uwe,
thanks for having a look!
Sorry for replying so late.
пн, 10 июл. 2023 г. в 12:14, Uwe Kleine-König <[email protected]>:
Quoted 281 lines
Hello,
Quoted 83 lines · Aleksandr Shubin
Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM controllers with ones supported by pwm-sun4i driver.
This patch adds a PWM controller driver for Allwinner's D1, T113-S3 and R329 SoCs. The main difference between these SoCs is the number of channels defined by the DT property.
Signed-off-by Aleksandr Shubin <[email protected]>--- drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 333 insertions(+) create mode 100644 drivers/pwm/pwm-sun20i.cdrivers/pwm/Kconfig +10
-0@@ -594,6 +594,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN20I + tristate "Allwinner D1/T113s/R329 PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on COMMON_CLK + help + Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun20i. + config PWM_SUNPLUS tristate "Sunplus PWM support" depends on ARCH_SUNPLUS || COMPILE_TESTdrivers/pwm/Makefile +1
-0@@ -55,6 +55,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o +obj-$(CONFIG_PWM_SUN20I) += pwm-sun20i.o obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.odrivers/pwm/pwm-sun20i.c new file +29
-0@@ -0,0 +1,322 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329) +* +* Limitations: +* - When the parameters change, current running period will not be completed +* and run new settings immediately. +* - It output HIGH-Z state when PWM channel disabled. +* +* Copyright (c) 2023 Aleksandr Shubin <[email protected]> +*/ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pwm.h> +#include <linux/reset.h> + +#define PWM_CLK_CFG_REG (chan) (0x20 + (((chan) >> 1) * 0x4)) +#define PWM_CLK_SRC GENMASK(8, 7) +#define PWM_CLK_DIV_M GENMASK(3, 0) + +#define PWM_CLK_GATE_REG 0x40 +#define PWM_CLK_BYPASS (chan) BIT((chan) - 16) +#define PWM_CLK_GATING (chan) BIT(chan) +drivers/pwm/pwm-sun20i.c line 39
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+#define PWM_ENABLE_REG 0x80 +#define PWM_EN (chan) BIT(chan) + +#define PWM_CTL_REG (chan) (0x100 + (chan) * 0x20) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (chan) (0x104 + (chan) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0)Can you please adapt the register field names to include the register name? I'd use:
#define
PWM_CTL(chan)(0x100+ (chan) *0x20)
#definePWM_CTL_ACT_STABIT(8)
#definePWM_CTL_PRESCAL_KGENMASK(7, 0)then you get a chance to spot when
PWM_CLK_BYPASS(x)is written toPWM_CLK_CFG.Quoted 39 lines
drivers/pwm/pwm-sun20i.c new file +39
-0+struct sun20i_pwm_chip { +struct pwm_chip chip; +struct clk *clk_bus, *clk_hosc; +struct reset_control *rst; +void __iomem *base; +/* Mutex to protect pwm apply state */ +struct mutex mutex; +}; + +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun20i_pwm_chip, chip); +} + +static inline u32 sun20i_pwm_readl (struct sun20i_pwm_chip *chip, +unsigned long offset) +{ +return readl (chip->base + offset); +} + +static inline void sun20i_pwm_writel (struct sun20i_pwm_chip *chip, +u32 val,unsigned long offset) +{ +writel (val, chip->base + offset); +} + +static int sun20i_pwm_get_state (struct pwm_chip *chip, +struct pwm_device *pwm, +struct pwm_state *state) +{ +struct sun20i_pwm_chip *sun20i_chip =to_sun20i_pwm_chip (chip); +u64 clk_rate, tmp; +u32 val; +u16 ent_cycle, act_cycle; +u8 prescal, div_id; + +mutex_lock (&sun20i_chip->mutex); + + val =sun20i_pwm_readl (sun20i_chip,PWM_CLK_CFG_REG (pwm->hwpwm));drivers/pwm/pwm-sun20i.c line 89
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ div_id =FIELD_GET (PWM_CLK_DIV_M , val); +if (FIELD_GET (PWM_CLK_SRC , val) ==0 ) + clk_rate =clk_get_rate (sun20i_chip->clk_hosc); +else + clk_rate =clk_get_rate (sun20i_chip->clk_bus); + + val =sun20i_pwm_readl (sun20i_chip,PWM_CTL_REG (pwm->hwpwm)); + state->polarity = (PWM_ACT_STA & val) ?PWM_POLARITY_NORMAL :PWM_POLARITY_INVERSED ; + + prescal =FIELD_GET (PWM_PRESCAL_K , val) +1 ;If
PWM_PRESCAL_Kis0xff, prescal ends up being 0. This isn't right, is it?drivers/pwm/pwm-sun20i.c line 99
Quoted 9 lines
drivers/pwm/pwm-sun20i.c new file +9
-0+ val =sun20i_pwm_readl (sun20i_chip,PWM_ENABLE_REG ); + state->enabled = (PWM_EN (pwm->hwpwm) & val) ? true : false; + + val =sun20i_pwm_readl (sun20i_chip,PWM_PERIOD_REG (pwm->hwpwm)); + act_cycle =FIELD_GET (PWM_ACT_CYCLE , val); + ent_cycle =FIELD_GET (PWM_ENTIRE_CYCLE , val); +if (act_cycle > ent_cycle) + act_cycle = ent_cycle; +A comment that with the width of the used factors this cannot overflow would be nice here.
drivers/pwm/pwm-sun20i.c line 100
Quoted 1 line
drivers/pwm/pwm-sun20i.c new file +1
-0+ tmp = (u64)(act_cycle) * prescal * (1U << div_id) *NSEC_PER_SEC ;Can be simplified to:
tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;Quoted 66 lines
drivers/pwm/pwm-sun20i.c new file +66
-0+ state->duty_cycle =DIV_ROUND_UP_ULL (tmp, clk_rate); + tmp = (u64)(ent_cycle) * prescal * (1U << div_id) *NSEC_PER_SEC ; + state->period =DIV_ROUND_UP_ULL (tmp, clk_rate); +mutex_unlock (&sun20i_chip->mutex); + +return 0 ; +} + +static int sun20i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +int ret =0 ; +u32 clk_gate, clk_cfg, pwm_en, ctl, period; +u64 bus_rate, hosc_rate, clk_div, val; +u16 prescaler, div_m; +bool use_bus_clk, calc_div_m; +struct sun20i_pwm_chip *sun20i_chip =to_sun20i_pwm_chip (chip); + +mutex_lock (&sun20i_chip->mutex); + + pwm_en =sun20i_pwm_readl (sun20i_chip,PWM_ENABLE_REG ); + +if (state->enabled != pwm->state.enabled) + clk_gate =sun20i_pwm_readl (sun20i_chip,PWM_CLK_GATE_REG ); + +if (state->enabled != pwm->state.enabled && !state->enabled) { + clk_gate &= ~PWM_CLK_GATING (pwm->hwpwm); + pwm_en &= ~PWM_EN (pwm->hwpwm); +sun20i_pwm_writel (sun20i_chip, pwm_en,PWM_ENABLE_REG ); +sun20i_pwm_writel (sun20i_chip, clk_gate,PWM_CLK_GATE_REG ); + } + +if (state->polarity != pwm->state.polarity || + state->duty_cycle != pwm->state.duty_cycle || + state->period != pwm->state.period) { + ctl =sun20i_pwm_readl (sun20i_chip,PWM_CTL_REG (pwm->hwpwm)); + clk_cfg =sun20i_pwm_readl (sun20i_chip,PWM_CLK_CFG_REG (pwm->hwpwm)); + hosc_rate =clk_get_rate (sun20i_chip->clk_hosc); + bus_rate =clk_get_rate (sun20i_chip->clk_bus); +if (pwm_en &PWM_EN (pwm->hwpwm ^1 )) { +/* if the neighbor channel is enable, check period only */ + use_bus_clk =FIELD_GET (PWM_CLK_SRC , clk_cfg) !=0 ; +if (use_bus_clk) + val = state->period * bus_rate; +else + val = state->period * hosc_rate; +do_div (val,NSEC_PER_SEC ); + + div_m =FIELD_GET (PWM_CLK_DIV_M , clk_cfg); + calc_div_m = false; + }else { +/* check period and select clock source */ + use_bus_clk = false; + val = state->period * hosc_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { + use_bus_clk = true; + val = state->period * bus_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { + ret = -EINVAL ; + goto unlock_mutex; + } + } + div_m =0 ; + calc_div_m = true;drivers/pwm/pwm-sun20i.c line 176
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ +/* set up the CLK_DIV_M and clock CLK_SRC */ + clk_cfg =FIELD_PREP (PWM_CLK_DIV_M , div_m); + clk_cfg |=FIELD_PREP (PWM_CLK_SRC , use_bus_clk ?1 :0 ); + +sun20i_pwm_writel (sun20i_chip, clk_cfg,PWM_CLK_CFG_REG (pwm->hwpwm)); + } + +/* calculate prescaler, M factor, PWM entire cycle */ + clk_div = val;This assignment is useless as it is overwritten in the loop below, isn't it?
Quoted 9 lines
drivers/pwm/pwm-sun20i.c new file +9
-0+for (prescaler =0 ;; prescaler++) { +if (prescaler >=256 ) { +if (calc_div_m) { + prescaler =0 ; + div_m++; +if (div_m >=9 ) { + ret = -EINVAL ; + goto unlock_mutex; + }drivers/pwm/pwm-sun20i.c line 195
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ }else { + ret = -EINVAL ; + goto unlock_mutex; + } + } + + clk_div = val >> div_m; +do_div (clk_div, prescaler +1 ); +if (clk_div <=65534 ) +break ;
Quoted 1 line
This can be calculated without a loop.
Point me please where can I see the calculation of two divisors without a loop?
Quoted 10 lines
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +9
-0+ } + + period =FIELD_PREP (PWM_ENTIRE_CYCLE , clk_div); + +/* set duty cycle */ +if (use_bus_clk) + val = state->duty_cycle * bus_rate; +else + val = state->duty_cycle * hosc_rate;
Quoted 8 lines
drivers/pwm/pwm-sun20i.c line 210
Quoted 6 lines
drivers/pwm/pwm-sun20i.c new file +6
-0+do_div (val,NSEC_PER_SEC ); + clk_div = val >> div_m; +do_div (clk_div, prescaler +1 ); + +if (state->duty_cycle == state->period) + clk_div++;I don't understand that one. Can you explain that in a comment please?
The formula of the output period and the duty-cycle for PWM are as follows. T period = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1) T high-level = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE Duty-cycle = T high-level / T period
In accordance with this formula, in order to set the duty-cycle to 100%, it is necessary that PWM_ACT_CYCLE >= PWM_ENTIRE_CYCLE + 1
Quoted 30 lines
drivers/pwm/pwm-sun20i.c line 218
Quoted 9 lines
drivers/pwm/pwm-sun20i.c new file +8
-0+ period |=FIELD_PREP (PWM_ACT_CYCLE , clk_div); +sun20i_pwm_writel (sun20i_chip, period,PWM_PERIOD_REG (pwm->hwpwm)); + + ctl =FIELD_PREP (PWM_PRESCAL_K , prescaler); +if (state->polarity ==PWM_POLARITY_NORMAL ) + ctl |=PWM_ACT_STA ; + +sun20i_pwm_writel (sun20i_chip, ctl,PWM_CTL_REG (pwm->hwpwm));Is this racy? I.e. does the write to
PWM_PERIOD_REG(pwm->hwpwm)above already has an effect beforePWM_CTL_REG(pwm->hwpwm)is written?drivers/pwm/pwm-sun20i.c line 226
Quoted 8 lines
drivers/pwm/pwm-sun20i.c new file +8
-0+ } + +if (state->enabled != pwm->state.enabled && state->enabled) { + clk_gate &= ~PWM_CLK_BYPASS (pwm->hwpwm); + clk_gate |=PWM_CLK_GATING (pwm->hwpwm); + pwm_en |=PWM_EN (pwm->hwpwm); +sun20i_pwm_writel (sun20i_chip, pwm_en,PWM_ENABLE_REG ); +sun20i_pwm_writel (sun20i_chip, clk_gate,PWM_CLK_GATE_REG );This is (I guess) racy. If your PWM is running with
.period = 10000 .duty_cyle = 0 .enabled = trueand you configure it to
Quoted 7 lines
.period = 10000 .duty_cyle = 10000 .enabled = falseyou get a short spike. For a enabled=true -> enabled=false transition you should disable first before configuring duty+period (or skip the latter completely).
When switching enabled=true -> enabled=false then before setting the period, the enable register will be written false and the pvm will become inactive. this is the place:
if (state->enabled != pwm->state.enabled && !state->enabled) {
Quoted 12 lines
drivers/pwm/pwm-sun20i.c line 233
Quoted 8 lines
drivers/pwm/pwm-sun20i.c new file +7
-0+ } + +unlock_mutex: +mutex_unlock (&sun20i_chip->mutex); + +return ret; +}Best regards
Uwe
Quoted 3 lines
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Cheers,
Aleksandr
Re: [PATCH v3 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
Hello,
Quoted 279 lines · Александр Шубин
пн, 10 июл. 2023 г. в 12:14, Uwe Kleine-König <[email protected]>:
Quoted 278 lines · Aleksandr Shubin
Quoted 83 lines
Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM controllers with ones supported by pwm-sun4i driver.
This patch adds a PWM controller driver for Allwinner's D1, T113-S3 and R329 SoCs. The main difference between these SoCs is the number of channels defined by the DT property.
Signed-off-by Aleksandr Shubin <[email protected]>--- drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 333 insertions(+) create mode 100644 drivers/pwm/pwm-sun20i.cdrivers/pwm/Kconfig +10
-0@@ -594,6 +594,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN20I + tristate "Allwinner D1/T113s/R329 PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on COMMON_CLK + help + Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun20i. + config PWM_SUNPLUS tristate "Sunplus PWM support" depends on ARCH_SUNPLUS || COMPILE_TESTdrivers/pwm/Makefile +1
-0@@ -55,6 +55,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o +obj-$(CONFIG_PWM_SUN20I) += pwm-sun20i.o obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.odrivers/pwm/pwm-sun20i.c new file +29
-0@@ -0,0 +1,322 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329) +* +* Limitations: +* - When the parameters change, current running period will not be completed +* and run new settings immediately. +* - It output HIGH-Z state when PWM channel disabled. +* +* Copyright (c) 2023 Aleksandr Shubin <[email protected]> +*/ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/pwm.h> +#include <linux/reset.h> + +#define PWM_CLK_CFG_REG (chan) (0x20 + (((chan) >> 1) * 0x4)) +#define PWM_CLK_SRC GENMASK(8, 7) +#define PWM_CLK_DIV_M GENMASK(3, 0) + +#define PWM_CLK_GATE_REG 0x40 +#define PWM_CLK_BYPASS (chan) BIT((chan) - 16) +#define PWM_CLK_GATING (chan) BIT(chan) +drivers/pwm/pwm-sun20i.c line 39
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+#define PWM_ENABLE_REG 0x80 +#define PWM_EN (chan) BIT(chan) + +#define PWM_CTL_REG (chan) (0x100 + (chan) * 0x20) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (chan) (0x104 + (chan) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0)Can you please adapt the register field names to include the register name? I'd use:
#define
PWM_CTL(chan)(0x100+ (chan) *0x20)
#definePWM_CTL_ACT_STABIT(8)
#definePWM_CTL_PRESCAL_KGENMASK(7, 0)then you get a chance to spot when
PWM_CLK_BYPASS(x)is written toPWM_CLK_CFG.Quoted 39 lines
drivers/pwm/pwm-sun20i.c new file +39
-0+struct sun20i_pwm_chip { +struct pwm_chip chip; +struct clk *clk_bus, *clk_hosc; +struct reset_control *rst; +void __iomem *base; +/* Mutex to protect pwm apply state */ +struct mutex mutex; +}; + +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun20i_pwm_chip, chip); +} + +static inline u32 sun20i_pwm_readl (struct sun20i_pwm_chip *chip, +unsigned long offset) +{ +return readl (chip->base + offset); +} + +static inline void sun20i_pwm_writel (struct sun20i_pwm_chip *chip, +u32 val,unsigned long offset) +{ +writel (val, chip->base + offset); +} + +static int sun20i_pwm_get_state (struct pwm_chip *chip, +struct pwm_device *pwm, +struct pwm_state *state) +{ +struct sun20i_pwm_chip *sun20i_chip =to_sun20i_pwm_chip (chip); +u64 clk_rate, tmp; +u32 val; +u16 ent_cycle, act_cycle; +u8 prescal, div_id; + +mutex_lock (&sun20i_chip->mutex); + + val =sun20i_pwm_readl (sun20i_chip,PWM_CLK_CFG_REG (pwm->hwpwm));drivers/pwm/pwm-sun20i.c line 89
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ div_id =FIELD_GET (PWM_CLK_DIV_M , val); +if (FIELD_GET (PWM_CLK_SRC , val) ==0 ) + clk_rate =clk_get_rate (sun20i_chip->clk_hosc); +else + clk_rate =clk_get_rate (sun20i_chip->clk_bus); + + val =sun20i_pwm_readl (sun20i_chip,PWM_CTL_REG (pwm->hwpwm)); + state->polarity = (PWM_ACT_STA & val) ?PWM_POLARITY_NORMAL :PWM_POLARITY_INVERSED ; + + prescal =FIELD_GET (PWM_PRESCAL_K , val) +1 ;If
PWM_PRESCAL_Kis0xff, prescal ends up being 0. This isn't right, is it?drivers/pwm/pwm-sun20i.c line 99
Quoted 9 lines
drivers/pwm/pwm-sun20i.c new file +9
-0+ val =sun20i_pwm_readl (sun20i_chip,PWM_ENABLE_REG ); + state->enabled = (PWM_EN (pwm->hwpwm) & val) ? true : false; + + val =sun20i_pwm_readl (sun20i_chip,PWM_PERIOD_REG (pwm->hwpwm)); + act_cycle =FIELD_GET (PWM_ACT_CYCLE , val); + ent_cycle =FIELD_GET (PWM_ENTIRE_CYCLE , val); +if (act_cycle > ent_cycle) + act_cycle = ent_cycle; +A comment that with the width of the used factors this cannot overflow would be nice here.
drivers/pwm/pwm-sun20i.c line 100
Quoted 1 line
drivers/pwm/pwm-sun20i.c new file +1
-0+ tmp = (u64)(act_cycle) * prescal * (1U << div_id) *NSEC_PER_SEC ;Can be simplified to:
tmp = (u64)act_cycle * prescal << div_id * NSEC_PER_SEC;Quoted 66 lines
drivers/pwm/pwm-sun20i.c new file +66
-0+ state->duty_cycle =DIV_ROUND_UP_ULL (tmp, clk_rate); + tmp = (u64)(ent_cycle) * prescal * (1U << div_id) *NSEC_PER_SEC ; + state->period =DIV_ROUND_UP_ULL (tmp, clk_rate); +mutex_unlock (&sun20i_chip->mutex); + +return 0 ; +} + +static int sun20i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +const struct pwm_state *state) +{ +int ret =0 ; +u32 clk_gate, clk_cfg, pwm_en, ctl, period; +u64 bus_rate, hosc_rate, clk_div, val; +u16 prescaler, div_m; +bool use_bus_clk, calc_div_m; +struct sun20i_pwm_chip *sun20i_chip =to_sun20i_pwm_chip (chip); + +mutex_lock (&sun20i_chip->mutex); + + pwm_en =sun20i_pwm_readl (sun20i_chip,PWM_ENABLE_REG ); + +if (state->enabled != pwm->state.enabled) + clk_gate =sun20i_pwm_readl (sun20i_chip,PWM_CLK_GATE_REG ); + +if (state->enabled != pwm->state.enabled && !state->enabled) { + clk_gate &= ~PWM_CLK_GATING (pwm->hwpwm); + pwm_en &= ~PWM_EN (pwm->hwpwm); +sun20i_pwm_writel (sun20i_chip, pwm_en,PWM_ENABLE_REG ); +sun20i_pwm_writel (sun20i_chip, clk_gate,PWM_CLK_GATE_REG ); + } + +if (state->polarity != pwm->state.polarity || + state->duty_cycle != pwm->state.duty_cycle || + state->period != pwm->state.period) { + ctl =sun20i_pwm_readl (sun20i_chip,PWM_CTL_REG (pwm->hwpwm)); + clk_cfg =sun20i_pwm_readl (sun20i_chip,PWM_CLK_CFG_REG (pwm->hwpwm)); + hosc_rate =clk_get_rate (sun20i_chip->clk_hosc); + bus_rate =clk_get_rate (sun20i_chip->clk_bus); +if (pwm_en &PWM_EN (pwm->hwpwm ^1 )) { +/* if the neighbor channel is enable, check period only */ + use_bus_clk =FIELD_GET (PWM_CLK_SRC , clk_cfg) !=0 ; +if (use_bus_clk) + val = state->period * bus_rate; +else + val = state->period * hosc_rate; +do_div (val,NSEC_PER_SEC ); + + div_m =FIELD_GET (PWM_CLK_DIV_M , clk_cfg); + calc_div_m = false; + }else { +/* check period and select clock source */ + use_bus_clk = false; + val = state->period * hosc_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { + use_bus_clk = true; + val = state->period * bus_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { + ret = -EINVAL ; + goto unlock_mutex; + } + } + div_m =0 ; + calc_div_m = true;drivers/pwm/pwm-sun20i.c line 176
Quoted 10 lines
drivers/pwm/pwm-sun20i.c new file +10
-0+ +/* set up the CLK_DIV_M and clock CLK_SRC */ + clk_cfg =FIELD_PREP (PWM_CLK_DIV_M , div_m); + clk_cfg |=FIELD_PREP (PWM_CLK_SRC , use_bus_clk ?1 :0 ); + +sun20i_pwm_writel (sun20i_chip, clk_cfg,PWM_CLK_CFG_REG (pwm->hwpwm)); + } + +/* calculate prescaler, M factor, PWM entire cycle */ + clk_div = val;This assignment is useless as it is overwritten in the loop below, isn't it?
Quoted 19 lines
drivers/pwm/pwm-sun20i.c new file +19
-0+for (prescaler =0 ;; prescaler++) { +if (prescaler >=256 ) { +if (calc_div_m) { + prescaler =0 ; + div_m++; +if (div_m >=9 ) { + ret = -EINVAL ; + goto unlock_mutex; + } + }else { + ret = -EINVAL ; + goto unlock_mutex; + } + } + + clk_div = val >> div_m; +do_div (clk_div, prescaler +1 ); +if (clk_div <=65534 ) +break ;
Quoted 3 lines
Quoted 1 line
This can be calculated without a loop.
Point me please where can I see the calculation of two divisors without a loop?
for a given value of div_m you search the smallest prescaler such that
(val >> div_m) // (prescaler + 1) ≤ 65534
(Using Python syntax where // denotes the usual round-down-to-next-integer division and / normal exact division.)
This is equivalent to:
(val >> div_m) // (prescaler + 1) ≤ 65534 ⟺ (val >> div_m) ≤ 65534 * (prescaler + 1) + prescaler ⟺ (val >> div_m) ≤ 65535 * prescaler + 65534 ⟺ (val >> div_m) - 65534 ≤ 65535 * prescaler ⟺ ((val >> div_m) - 65534) / 65535 ≤ prescaler
And as prescaler is integer, this is
... ⟺ ((val >> div_m) - 65534) // 65535 ≤ prescaler
So the prescaler value you're looking for is:
((val >> div_m) - 65534) // 65535
And then you have to pick the smallest div_m such that prescaler ≤ 255:
((val >> div_m) - 65534) // 65535 ≤ 255 ⟺ (val >> div_m) - 65534 ≤ 255 * 65535 + 65534 ⟺ val >> div_m ≤ 255 * 65535 + 2 * 65534 ⟺ val >> div_m ≤ 16842493 ⟺ val >> div_m < 16842494
so div_m is fls((val) / 16842494).
You might want to double check this and explain the algorithm in a comment similar to the above calculation.
Quoted 9 lines
Quoted 9 lines
drivers/pwm/pwm-sun20i.c line 204
Quoted 9 lines
drivers/pwm/pwm-sun20i.c new file +9
-0+ } + + period =FIELD_PREP (PWM_ENTIRE_CYCLE , clk_div); + +/* set duty cycle */ +if (use_bus_clk) + val = state->duty_cycle * bus_rate; +else + val = state->duty_cycle * hosc_rate;
maybe better use:
if (use_bus_clk) rate = bus_rate; else rate = hosc_rate; val = state->duty_cycle * rate
Quoted 11 lines
Quoted 8 lines
drivers/pwm/pwm-sun20i.c line 210
Quoted 6 lines
drivers/pwm/pwm-sun20i.c new file +6
-0+do_div (val,NSEC_PER_SEC ); + clk_div = val >> div_m; +do_div (clk_div, prescaler +1 ); + +if (state->duty_cycle == state->period) + clk_div++;I don't understand that one. Can you explain that in a comment please?
The formula of the output period and the duty-cycle for PWM are as follows. T period = (
PWM01_CLK/ PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1)
That can be simpler written as:
Tperiod = PWM0_PRESCALE_K / PWM01_CLK * (PPR0.PWM_ENTIRE_CYCLE + 1)
right?
Quoted 1 line
T high-level = (
PWM01_CLK/ PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE
simlar:
Thigh = PWM0_PRESCALE_K / PWM01_CLK * PPR0.PWM_ACT_CYCLE
Quoted 3 lines
Duty-cycle = T high-level / T period
In accordance with this formula, in order to set the duty-cycle to 100%, it is necessary thatPWM_ACT_CYCLE>=PWM_ENTIRE_CYCLE+ 1
The +1 is also relevant for duty_cycles other than state->period, right?
Quoted 33 lines
Quoted 33 lines
drivers/pwm/pwm-sun20i.c line 218
Quoted 8 lines
drivers/pwm/pwm-sun20i.c new file +8
-0+ period |=FIELD_PREP (PWM_ACT_CYCLE , clk_div); +sun20i_pwm_writel (sun20i_chip, period,PWM_PERIOD_REG (pwm->hwpwm)); + + ctl =FIELD_PREP (PWM_PRESCAL_K , prescaler); +if (state->polarity ==PWM_POLARITY_NORMAL ) + ctl |=PWM_ACT_STA ; + +sun20i_pwm_writel (sun20i_chip, ctl,PWM_CTL_REG (pwm->hwpwm));Is this racy? I.e. does the write to
PWM_PERIOD_REG(pwm->hwpwm)above already has an effect beforePWM_CTL_REG(pwm->hwpwm)is written?drivers/pwm/pwm-sun20i.c line 226
Quoted 8 lines
drivers/pwm/pwm-sun20i.c new file +8
-0+ } + +if (state->enabled != pwm->state.enabled && state->enabled) { + clk_gate &= ~PWM_CLK_BYPASS (pwm->hwpwm); + clk_gate |=PWM_CLK_GATING (pwm->hwpwm); + pwm_en |=PWM_EN (pwm->hwpwm); +sun20i_pwm_writel (sun20i_chip, pwm_en,PWM_ENABLE_REG ); +sun20i_pwm_writel (sun20i_chip, clk_gate,PWM_CLK_GATE_REG );This is (I guess) racy. If your PWM is running with
.period = 10000 .duty_cyle = 0 .enabled = trueand you configure it to
.period = 10000 .duty_cyle = 10000 .enabled = false
Quoted 8 lines
Quoted 3 lines
you get a short spike. For a enabled=true -> enabled=false transition you should disable first before configuring duty+period (or skip the latter completely).
When switching enabled=true -> enabled=false then before setting the period, the enable register will be written false and the pvm will become inactive. this is the place:
if (state->enabled!=pwm->state.enabled&& !state->enabled) {
Ah, it seems I missed that (or I just don't understand any more what meant back then :-)
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
PATCH v3 3/3 riscv: dts: allwinner: d1: Add pwm node
In reply to Aleksandr ShubinD1 and T113s contain a pwm controller with 8 channels. This controller is supported by the sun20i-pwm driver.
Add a device tree node for it.
--- arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi | 11 +++++++++++ 1 file changed, 11 insertions(+)
arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi +11 -0
@@ -127,6 +127,17 @@ uart3_pb_pins: uart3-pb-pins { }; }; + pwm: pwm@2000c00 { + compatible = "allwinner,sun20i-d1-pwm"; + reg = <0x02000c00 0x400>; + clocks = <&dcxo>, + <&ccu CLK_BUS_PWM>; + clock-names = "hosc", "bus"; + resets = <&ccu RST_BUS_PWM>; + status = "disabled"; + #pwm-cells = <0x3>; + }; + ccu: clock-controller@2001000 { compatible = "allwinner,sun20i-d1-ccu"; reg = <0x2001000 0x1000>;
Signature · 1 line
2.25.1
Quoted 20 lines · On Tue, 27 Jun 2023 01:23:23 PDT (-0700), [email protected] wrote:
Hi,
This series adds support for PWM controller on new Allwinner's SoCs, such as D1, T113s and R329. The implemented driver provides basic functionality for control PWM channels.
v2:
- fix dt-bindings
- fix a remark in the driverv3:
- fix dt-bindings
- fixsunxi-d1s-t113.dtsiAleksandr Shubin (3):
dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
controller
pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
riscv: dts: allwinner: d1: Add pwm node
Quoted 8 lines
.../bindings/pwm/allwinner,sun20i-pwm.yaml | 86 +++++ .../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 11 + drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 ++++++++++++++++++ 5 files changed, 430 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml create mode 100644 drivers/pwm/pwm-sun20i.c
I'm not sure if this ended up somewhere, but I'm assuming it's not aimed at my tree. LMK if you guys want me to pick it up, I'm dropping it from patchwork.
Hello,
Quoted 30 lines · Palmer Dabbelt
Quoted 20 lines · On Tue, 27 Jun 2023 01:23:23 PDT (-0700), [email protected] wrote:
Hi,
This series adds support for PWM controller on new Allwinner's SoCs, such as D1, T113s and R329. The implemented driver provides basic functionality for control PWM channels.
v2:
- fix dt-bindings
- fix a remark in the driverv3:
- fix dt-bindings
- fixsunxi-d1s-t113.dtsiAleksandr Shubin (3):
dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
controller
pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
riscv: dts: allwinner: d1: Add pwm nodeQuoted 8 lines
.../bindings/pwm/allwinner,sun20i-pwm.yaml | 86 +++++ .../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 11 + drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 ++++++++++++++++++ 5 files changed, 430 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml create mode 100644 drivers/pwm/pwm-sun20i.c
Quoted 5 lines
Acked-by Palmer Dabbelt <[email protected]>I'm not sure if this ended up somewhere, but I'm assuming it's not aimed at my tree. LMK if you guys want me to pick it up, I'm dropping it from patchwork.
At least patch #2 should go via the pwm tree. It's on my list to review.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Jernej Škrabec
maintainerDne četrtek, 06. julij 2023 ob 22:20:57 CEST je Uwe Kleine-König napisal(a):
Quoted 36 lines
Hello,
Quoted 33 lines · Palmer Dabbelt
Quoted 31 lines · On Tue, 27 Jun 2023 01:23:23 PDT (-0700), [email protected] wrote:
Hi,
This series adds support for PWM controller on new Allwinner's SoCs, such as D1, T113s and R329. The implemented driver provides basic functionality for control PWM channels.
v2:
- fix dt-bindings
- fix a remark in the driverv3:
- fix dt-bindings
- fixsunxi-d1s-t113.dtsiAleksandr Shubin (3):
dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWMcontroller
pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
riscv: dts: allwinner: d1: Add pwm node.../bindings/pwm/allwinner,sun20i-pwm.yaml | 86 +++++ .../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 11 + drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun20i.c | 322 ++++++++++++++++++ 5 files changed, 430 insertions(+) create mode 100644
Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yamlcreate mode 100644drivers/pwm/pwm-sun20i.c
Quoted 8 lines
Quoted 6 lines
Acked-by Palmer Dabbelt <[email protected]>I'm not sure if this ended up somewhere, but I'm assuming it's not aimed at
my tree. LMK if you guys want me to pick it up, I'm dropping it from patchwork.At least patch #2 should go via the pwm tree. It's on my list to review.
Rest will go through sunxi tree, although DT bindings patches are usually picked together with driver patches. I'm fine either way.
Best regards,
Jernej