ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.
Subsystems (3)
PATCH v3 6/6 ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.
The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, each PWM pair built-in 1 clock module, 2 timer logic module and 1 programmable dead-time generator, it also support waveform capture. It has 2 clock sources OSC24M and APB1, it is different with the sun4i-pwm driver, Therefore add a new driver for it.
Signed-off-by Hao Zhang <[email protected]>
--- drivers/pwm/Kconfig | 12 +- drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 drivers/pwm/pwm-sun8i.c
drivers/pwm/Kconfig +11 -1
@@ -426,7 +426,7 @@ config PWM_STMPE expanders. config PWM_SUN4I - tristate "Allwinner PWM support" + tristate "Allwinner SUN4I PWM support" depends on ARCH_SUNXI || COMPILE_TEST depends on HAS_IOMEM && COMMON_CLK help
@@ -435,6 +435,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN8I + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on HAS_IOMEM && COMMON_CLK + help + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun8i. + config PWM_TEGRA tristate "NVIDIA Tegra PWM support" depends on ARCH_TEGRA
drivers/pwm/Makefile +1 -0
@@ -43,6 +43,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_SUN8I) += pwm-sun8i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
drivers/pwm/pwm-sun8i.c new file +418 -0
418 lines not on this page · raw message
Signature · 1 line
2.7.4
Quoted 132 lines · Hao Zhang
The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, each PWM pair built-in 1 clock module, 2 timer logic module and 1 programmable dead-time generator, it also support waveform capture. It has 2 clock sources OSC24M and APB1, it is different with the sun4i-pwm driver, Therefore add a new driver for it.
Signed-off-by Hao Zhang <[email protected]>--- drivers/pwm/Kconfig | 12 +- drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 drivers/pwm/pwm-sun8i.cdrivers/pwm/Kconfig +11
-1@@ -426,7 +426,7 @@ config PWM_STMPE expanders. config PWM_SUN4I - tristate "Allwinner PWM support" + tristate "Allwinner SUN4I PWM support" depends on ARCH_SUNXI || COMPILE_TEST depends on HAS_IOMEM && COMMON_CLK help@@ -435,6 +435,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN8I + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on HAS_IOMEM && COMMON_CLK + help + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun8i. + config PWM_TEGRA tristate "NVIDIA Tegra PWM support" depends on ARCH_TEGRAdrivers/pwm/Makefile +1
-0@@ -43,6 +43,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_SUN8I) += pwm-sun8i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.odrivers/pwm/pwm-sun8i.c new file +70
-0@@ -0,0 +1,418 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Hao Zhang <[email protected]> + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/time.h> +#include <linux/regmap.h> + +#define PWM_IRQ_ENABLE_REG 0x0000 +#define PCIE (ch) BIT(ch) + +#define PWM_IRQ_STATUS_REG 0x0004 +#define PIS (ch) BIT(ch) + +#define CAPTURE_IRQ_ENABLE_REG 0x0010 +#define CRIE (ch) BIT((ch) * 2) +#define CFIE (ch) BIT((ch) * 2 + 1) + +#define CAPTURE_IRQ_STATUS_REG 0x0014 +#define CRIS (ch) BIT((ch) * 2) +#define CFIS (ch) BIT((ch) * 2 + 1) + +#define CLK_CFG_REG (ch) (0x0020 + ((ch) >> 1) * 4) +#define CLK_SRC_SEL GENMASK(8, 7) +#define CLK_SRC_BYPASS_SEC BIT(6) +#define CLK_SRC_BYPASS_FIR BIT(5) +#define CLK_GATING BIT(4) +#define CLK_DIV_M GENMASK(3, 0) + +#define PWM_DZ_CTR_REG (ch) (0x0030 + ((ch) >> 1) * 4) +#define PWM_DZ_INTV GENMASK(15, 8) +#define PWM_DZ_EN BIT(0) + +#define PWM_ENABLE_REG 0x0040 +#define PWM_EN (ch) BIT(ch) + +#define CAPTURE_ENABLE_REG 0x0044 +#define CAP_EN (ch) BIT(ch) + +#define PWM_CTR_REG (ch) (0x0060 + (ch) * 0x20) +#define PWM_PERIOD_RDY BIT(11) +#define PWM_PUL_START BIT(10) +#define PWM_MODE BIT(9) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (ch) (0x0064 + (ch) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0) + +#define PWM_CNT_REG (ch) (0x0068 + (ch) * 0x20) +#define PWM_CNT_VAL GENMASK(15, 0) + +#define CAPTURE_CTR_REG (ch) (0x006c + (ch) * 0x20) +#define CAPTURE_CRLF BIT(2) +#define CAPTURE_CFLF BIT(1) +#define CAPINV BIT(0) + +#define CAPTURE_RISE_REG (ch) (0x0070 + (ch) * 0x20)
drivers/pwm/pwm-sun8i.c line 80
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+#define CAPTURE_CRLR GENMASK(15, 0) + +#define CAPTURE_FALL_REG (ch) (0x0074 + (ch) * 0x20) +#define CAPTURE_CFLR GENMASK(15, 0) + +struct sun8i_pwm_chip { +struct pwm_chip chip; +struct clk *clk; +void __iomem *base; +const struct sun8i_pwm_data *data;
This member is only written to, but never used.
Quoted 33 lines
drivers/pwm/pwm-sun8i.c new file +33
-0+struct regmap *regmap; +}; + +static struct sun8i_pwm_chip *to_sun8i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun8i_pwm_chip, chip); +} + +static u32 sun8i_pwm_read (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long offset) +{ +u32 val; + +regmap_read (sun8i_pwm->regmap, offset, &val); +return val; +} + +static void sun8i_pwm_set_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit, bit); +} + +static void sun8i_pwm_clear_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit,0 ); +} + +static void sun8i_pwm_set_value (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 mask,u32 val) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, mask, val);
drivers/pwm/pwm-sun8i.c line 123
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+} + +static void sun8i_pwm_set_polarity (struct sun8i_pwm_chip *chip,u32 ch, +enum pwm_polarity polarity) +{ +if (polarity ==PWM_POLARITY_NORMAL ) +sun8i_pwm_set_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +else +sun8i_pwm_clear_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +}
This function is only used once. Maybe drop the function and put the if to the caller.
Quoted 12 lines
drivers/pwm/pwm-sun8i.c new file +12
-0+ +static int sun8i_pwm_config (struct sun8i_pwm_chip *sun8i_pwm,u8 ch, +struct pwm_state *state) +{ +u64 clk_rate, clk_div, val; +u16 prescaler =0 ; +u16 div_id =0 ; +struct clk *clk; +bool is_clk; +int ret; + + clk_rate =clk_get_rate (sun8i_pwm->clk);
drivers/pwm/pwm-sun8i.c line 145
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ +/* check period and select clock source */ + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (val <=1 && is_clk) { + clk =devm_clk_get (sun8i_pwm->chip.dev,"mux-1" ); +if (IS_ERR (clk)) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" );
This error message looks wrong, several others, too.
drivers/pwm/pwm-sun8i.c line 146
Quoted 1 line
drivers/pwm/pwm-sun8i.c new file +1
-0+return -EINVAL ;
return PTR_ERR(clk)?
Quoted 5 lines
drivers/pwm/pwm-sun8i.c new file +5
-0+ } + + clk_rate =clk_get_rate (clk); + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC );
drivers/pwm/pwm-sun8i.c line 161
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+if (val <=1 ) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + +/* change clock source to "mux-1" */ +clk_disable_unprepare (sun8i_pwm->clk); +devm_clk_put (sun8i_pwm->chip.dev, sun8i_pwm->clk); + sun8i_pwm->clk = clk;
sun8i_pwm is shared for all 8 PWMs, right? So if you assign mux-1 here for the second mux, how does this influence the first PWM?
mux-0 might already be enabled, it is then never disabled.
drivers/pwm/pwm-sun8i.c line 173
Quoted 12 lines
drivers/pwm/pwm-sun8i.c new file +12
-0+ + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (sun8i_pwm->chip.dev, +"Failed to enable PWM clock\n" ); +return ret; + } + + }else { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ;
This looks wrong. If val is > 1 there shouldn't be a reason to abort?
drivers/pwm/pwm-sun8i.c line 184
Quoted 11 lines
drivers/pwm/pwm-sun8i.c new file +11
-0+ } + + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (is_clk) +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,0 <<7 ); +else +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,1 <<7 ); + +dev_info (sun8i_pwm->chip.dev,"clock source freq:%lldHz\n" , clk_rate);
I'd degrade this to dev_dbg.
Quoted 9 lines
drivers/pwm/pwm-sun8i.c new file +9
-0+ +/* calculate and set prescaler, div table, PWM entire cycle */ + clk_div = val; +while (clk_div >65535 ) { + prescaler++; + clk_div = val; +do_div (clk_div,1U << div_id); +do_div (clk_div, prescaler +1 ); +
drivers/pwm/pwm-sun8i.c line 203
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+if (prescaler ==255 ) { + prescaler =0 ; + div_id++; +if (div_id ==9 ) { +dev_err (sun8i_pwm->chip.dev, +"unsupport period value\n" ); +return -EINVAL ; + } + } + }
Noting the underlying formula for the calculation and the bitwidth for the related register fields above would be good.
Quoted 8 lines
drivers/pwm/pwm-sun8i.c new file +8
-0+ +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ENTIRE_CYCLE , clk_div <<16 ); +sun8i_pwm_set_value (sun8i_pwm,PWM_CTR_REG (ch), +PWM_PRESCAL_K , prescaler <<0 ); +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_DIV_M , div_id <<0 ); +
drivers/pwm/pwm-sun8i.c line 221
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+/* set duty cycle */ + val = state->period; +do_div (val, clk_div); + clk_div = state->duty_cycle; +do_div (clk_div, val); +if (clk_div >65535 ) + clk_div =65535 ; + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ACT_CYCLE , clk_div <<0 );
Why "<< 0"?
Quoted 17 lines
drivers/pwm/pwm-sun8i.c new file +17
-0+return 0 ; +} + +static int sun8i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +int ret; +struct sun8i_pwm_chip *sun8i_pwm =to_sun8i_pwm_chip (chip); +struct pwm_state cstate; + +pwm_get_state (pwm, &cstate); +if (!cstate.enabled) { + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (chip->dev,"Failed to enable PWM clock\n" ); +return ret; + }
drivers/pwm/pwm-sun8i.c line 249
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ } + +if ((cstate.period != state->period) || + (cstate.duty_cycle != state->duty_cycle)) { + ret =sun8i_pwm_config (sun8i_pwm, pwm->hwpwm, state); +if (ret) { +dev_err (chip->dev,"Failed to config PWM\n" ); +return ret; + } + }
sun8i_pwm_config writes the registers that are relevant for period and duty_cycle. When do these values take effect? If it's already here, switching the polarity below might introduce a glitch.
Quoted 6 lines
drivers/pwm/pwm-sun8i.c new file +6
-0+if (state->polarity != cstate.polarity) +sun8i_pwm_set_polarity (sun8i_pwm, pwm->hwpwm, state->polarity); + +if (state->enabled) { +sun8i_pwm_set_bit (sun8i_pwm, +CLK_CFG_REG (pwm->hwpwm),CLK_GATING );
drivers/pwm/pwm-sun8i.c line 266
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ +sun8i_pwm_set_bit (sun8i_pwm, +PWM_ENABLE_REG ,PWM_EN (pwm->hwpwm)); + }else { +sun8i_pwm_clear_bit (sun8i_pwm, +CLK_CFG_REG (pwm->hwpwm),CLK_GATING ); + +sun8i_pwm_clear_bit (sun8i_pwm, +PWM_ENABLE_REG ,PWM_EN (pwm->hwpwm)); + }
Please document how the hardware behaves when being disabled. (Does it drive a 0? Does it drive a 1 when inverted? Or is the output high-z?)
Quoted 8 lines
drivers/pwm/pwm-sun8i.c new file +8
-0+ +return 0 ; +} + +static void sun8i_pwm_get_state (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +struct sun8i_pwm_chip *sun8i_pwm =to_sun8i_pwm_chip (chip);
drivers/pwm/pwm-sun8i.c line 284
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+u64 clk_rate, tmp; +u32 val; +u16 clk_div, act_cycle; +u8 prescal, div_id; +u8 chn = pwm->hwpwm; + + clk_rate =clk_get_rate (sun8i_pwm->clk); + + val =sun8i_pwm_read (sun8i_pwm,PWM_CTR_REG (chn)); +if (PWM_ACT_STA & val)
This looks strange to me. While syntactically equivalent it is more usual to write this as
if (val & PWM_ACT_STA)
drivers/pwm/pwm-sun8i.c line 295
Quoted 11 lines
drivers/pwm/pwm-sun8i.c new file +11
-0+ state->polarity =PWM_POLARITY_NORMAL ; + else + state->polarity = PWM_POLARITY_INVERSED; + + prescal =PWM_PRESCAL_K & val; + + val =sun8i_pwm_read (sun8i_pwm,PWM_ENABLE_REG ); +if (PWM_EN (chn) & val) + state->enabled = true; +else + state->enabled = false;
When the PWM should be enabled, you also set the CLK_GATING bit. Should this better be checked for in .get_state, too?
Quoted 21 lines
drivers/pwm/pwm-sun8i.c new file +21
-0+ + val =sun8i_pwm_read (sun8i_pwm,PWM_PERIOD_REG (chn)); + act_cycle =PWM_ACT_CYCLE & val; + clk_div = val >>16 ; + + val =sun8i_pwm_read (sun8i_pwm,CLK_CFG_REG (chn)); + div_id =CLK_DIV_M & val; + + tmp = act_cycle * prescal * (1U << div_id) *NSEC_PER_SEC ; + state->duty_cycle =DIV_ROUND_CLOSEST_ULL (tmp, clk_rate); + tmp = clk_div * prescal * (1U << div_id) *NSEC_PER_SEC ; + state->period =DIV_ROUND_CLOSEST_ULL (tmp, clk_rate); +} + +static const struct regmap_config sun8i_pwm_regmap_config = { + .reg_bits =32 , + .reg_stride =4 , + .val_bits =32 , + .max_register =CAPTURE_FALL_REG (7 ), +}; +
drivers/pwm/pwm-sun8i.c line 326
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+static const struct pwm_ops sun8i_pwm_ops = { + .apply = sun8i_pwm_apply, + .get_state = sun8i_pwm_get_state, + .owner =THIS_MODULE , +}; + +static const struct of_device_id sun8i_pwm_dt_ids[] = { + { + .compatible ="allwinner,sun8i-r40-pwm" , + .data =NULL ,
.data doesn't need to be specified.
Quoted 8 lines
drivers/pwm/pwm-sun8i.c new file +8
-0+ }, + {}, +}; +MODULE_DEVICE_TABLE (of, sun8i_pwm_dt_ids); + +static int sun8i_pwm_probe (struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.of_node;
drivers/pwm/pwm-sun8i.c line 344
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+struct sun8i_pwm_chip *pwm; +struct resource *res; +int ret; +const struct of_device_id *match; + + match =of_match_device (sun8i_pwm_dt_ids, &pdev->dev); +if (!match) { +dev_err (&pdev->dev,"Error: No device match found\n" ); +return -ENODEV ; + }
This prevents a match by driver-name, right? Other than that match is only used to assign pwm->data below to NULL.
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ + pwm =devm_kzalloc (&pdev->dev,sizeof (*pwm),GFP_KERNEL ); +if (!pwm) +return -ENOMEM ; + + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); + pwm->base =devm_ioremap_resource (&pdev->dev, res); +if (IS_ERR (pwm->base)) +return PTR_ERR (pwm->base); +
drivers/pwm/pwm-sun8i.c line 364
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ pwm->regmap =devm_regmap_init_mmio (&pdev->dev, pwm->base, + &sun8i_pwm_regmap_config); +if (IS_ERR (pwm->regmap)) { +dev_err (&pdev->dev,"Failed to create regmap\n" ); +return PTR_ERR (pwm->regmap); + } + +/* we use mux-0 as default clock source */ + pwm->clk =devm_clk_get (&pdev->dev,"mux-0" ); +if (IS_ERR (pwm->clk)) {
You might want to handle pwm->clk == ERR_PTR(-EPROBE_DEFER) without falling back to mux-1 and without printing an error.
drivers/pwm/pwm-sun8i.c line 373
Quoted 9 lines
drivers/pwm/pwm-sun8i.c new file +9
-0+ pwm->clk =devm_clk_get (&pdev->dev,"mux-1" ); +if (IS_ERR (pwm->clk)) { +dev_err (&pdev->dev,"Failed to get PWM clock\n" ); +return PTR_ERR (pwm->clk); + } + } + + ret =of_property_read_u32 (np,"pwm-channels" , &pwm->chip.npwm); +if (ret && !pwm->chip.npwm) {
This is equivalent to
if (ret)
because &pwm->chip.npwm is only modified if of_property_read_u32 returns 0 and the variable holds a 0 before.
drivers/pwm/pwm-sun8i.c line 378
Quoted 5 lines
drivers/pwm/pwm-sun8i.c new file +5
-0+dev_err (&pdev->dev,"Can't get pwm-channels\n" ); +return ret; + } + +dev_info (&pdev->dev,"pwm-channels:%d\n" , pwm->chip.npwm);
dev_dbg?
Quoted 4 lines
drivers/pwm/pwm-sun8i.c new file +4
-0+ pwm->data = match->data; + pwm->chip.dev = &pdev->dev; + pwm->chip.ops = &sun8i_pwm_ops; + pwm->chip.base =-1 ;
drivers/pwm/pwm-sun8i.c line 392
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ pwm->chip.of_xlate = of_pwm_xlate_with_flags; + pwm->chip.of_pwm_n_cells =3 ; + + ret =pwmchip_add (&pwm->chip); +if (ret <0 ) { +dev_err (&pdev->dev,"Failed to add PWM chip: %d\n" , ret); +return ret; + } + +platform_set_drvdata (pdev, pwm);
If you do this earlier (typically after the allocation succeeded) you can simplify the last few lines to:
ret = pwmchip_add(&pwm->chip); if (ret < 0) dev_err(&pdev->dev, ...); return ret;
drivers/pwm/pwm-sun8i.c line 402
Quoted 6 lines
drivers/pwm/pwm-sun8i.c new file +6
-0+static int sun8i_pwm_remove (struct platform_device *pdev) +{ +struct sun8i_pwm_chip *pwm =platform_get_drvdata (pdev); + +clk_disable_unprepare (pwm->clk); +return pwmchip_remove (&pwm->chip);
This is at least unusual (and maybe broken). Please call pwmchip_remove before clk_disable_unprepare.
drivers/pwm/pwm-sun8i.c line 415
Quoted 13 lines
drivers/pwm/pwm-sun8i.c new file +13
-0+} + +static struct platform_driver sun8i_pwm_driver = { + .driver = { + .name ="sun8i-pwm" , + .of_match_table = sun8i_pwm_dt_ids, + }, + .probe = sun8i_pwm_probe, + .remove = sun8i_pwm_remove, +}; +module_platform_driver (sun8i_pwm_driver); + +MODULE_ALIAS ("platform: sun8i-pwm" );
I think the space in the alias must be dropped. Giving that the driver doesn't bind by driver-name I suggest to drop this completely.
drivers/pwm/pwm-sun8i.c line 418
Quoted 3 lines
drivers/pwm/pwm-sun8i.c new file +3
-0+MODULE_AUTHOR ("Hao Zhang <[email protected]>" ); +MODULE_DESCRIPTION ("Allwinner sun8i PWM driver" ); +MODULE_LICENSE ("GPL v2" );
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello,
Quoted 57 lines · Uwe Kleine-K?nig
Quoted 56 lines · Hao Zhang
The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, each PWM pair built-in 1 clock module, 2 timer logic module and 1 programmable dead-time generator, it also support waveform capture. It has 2 clock sources OSC24M and APB1, it is different with the sun4i-pwm driver, Therefore add a new driver for it.
Signed-off-by Hao Zhang <[email protected]>--- drivers/pwm/Kconfig | 12 +- drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 drivers/pwm/pwm-sun8i.cdrivers/pwm/Kconfig +11
-1@@ -426,7 +426,7 @@ config PWM_STMPE expanders. config PWM_SUN4I - tristate "Allwinner PWM support" + tristate "Allwinner SUN4I PWM support" depends on ARCH_SUNXI || COMPILE_TEST depends on HAS_IOMEM && COMMON_CLK help@@ -435,6 +435,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN8I + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on HAS_IOMEM && COMMON_CLK + help + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun8i. + config PWM_TEGRA tristate "NVIDIA Tegra PWM support" depends on ARCH_TEGRAdrivers/pwm/Makefile +1
-0@@ -43,6 +43,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_SUN8I) += pwm-sun8i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.o
Quoted 8 lines
drivers/pwm/pwm-sun8i.c line 2
Quoted 8 lines
drivers/pwm/pwm-sun8i.c new file +2
-0@@ -0,0 +1,418 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Hao Zhang <[email protected]>
Given that the documentation is publically available, I suggest to add a link to it in a comment here.
(http://linux-sunxi.org/File:Allwinner_R40_User_Manual_V1.0.pdf)
Quoted 66 lines
Quoted 8 lines
drivers/pwm/pwm-sun8i.c new file +8
-0+/* calculate and set prescaler, div table, PWM entire cycle */ + clk_div = val; +while (clk_div >65535 ) { + prescaler++; + clk_div = val; +do_div (clk_div,1U << div_id); +do_div (clk_div, prescaler +1 ); +drivers/pwm/pwm-sun8i.c line 203
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+if (prescaler ==255 ) { + prescaler =0 ; + div_id++; +if (div_id ==9 ) { +dev_err (sun8i_pwm->chip.dev, +"unsupport period value\n" ); +return -EINVAL ; + } + } + }Noting the underlying formula for the calculation and the bitwidth for the related register fields above would be good.
Quoted 8 lines
drivers/pwm/pwm-sun8i.c new file +8
-0+ +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ENTIRE_CYCLE , clk_div <<16 ); +sun8i_pwm_set_value (sun8i_pwm,PWM_CTR_REG (ch), +PWM_PRESCAL_K , prescaler <<0 ); +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_DIV_M , div_id <<0 ); +drivers/pwm/pwm-sun8i.c line 221
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+/* set duty cycle */ + val = state->period; +do_div (val, clk_div); + clk_div = state->duty_cycle; +do_div (clk_div, val); +if (clk_div >65535 ) + clk_div =65535 ; + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ACT_CYCLE , clk_div <<0 );Why "<< 0"?
Quoted 23 lines
drivers/pwm/pwm-sun8i.c new file +23
-0+return 0 ; +} + +static int sun8i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +int ret; +struct sun8i_pwm_chip *sun8i_pwm =to_sun8i_pwm_chip (chip); +struct pwm_state cstate; + +pwm_get_state (pwm, &cstate); +if (!cstate.enabled) { + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (chip->dev,"Failed to enable PWM clock\n" ); +return ret; + } + } + +if ((cstate.period != state->period) || + (cstate.duty_cycle != state->duty_cycle)) { + ret =sun8i_pwm_config (sun8i_pwm, pwm->hwpwm, state); +if (ret) {
Quoted 8 lines
drivers/pwm/pwm-sun8i.c line 249
Quoted 4 lines
drivers/pwm/pwm-sun8i.c new file +4
-0+dev_err (chip->dev,"Failed to config PWM\n" ); +return ret; + } + }
sun8i_pwm_configwrites the registers that are relevant for period andduty_cycle. When do these values take effect? If it's already here, switching the polarity below might introduce a glitch.
I think this is the case after taking a look into the reference manual.
There are two 16 bit fields in the PWM_PERIOD_REG. One specifies the number of clock ticks defining the period ("PWM_ENTIRE_CYCLE") and the other the duty cycle ("PWM_ACT_CYCLE").
So if you go from duty_cycle=5 + period=10 + POLARITY_NORMAL to duty_cycle=3 + period=10 + POLARITY_INVERTED this might generate:
____ __ ______
/ \____/ \_________/ \__/
^ ^ ^ ^
Also there is a PWM_PERIOD_RDY bit field that probably has to be consulted before writing to the PWM_PERIOD_REG register.
It's not entirely clear to me if the PWM_ACT_STA bit that is used for inversion is shadowed until the next period, too. That's what I assumed above. If it's not the wave might look as follows:
____ __ _____ ______
/ \____/ \/ \__/ \__/ ^ ^ * ^ ^
Where * marks the point where the inversion starts to take effect.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-K?nig | Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello,
Quoted 12 lines · Uwe Kleine-König
Quoted 12 lines
drivers/pwm/pwm-sun8i.c new file +12
-0+static int sun8i_pwm_config (struct sun8i_pwm_chip *sun8i_pwm,u8 ch, +struct pwm_state *state) +{ +[...] + clk_rate =clk_get_rate (clk); + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + }
Quoted 8 lines
drivers/pwm/pwm-sun8i.c line 161
Quoted 5 lines
drivers/pwm/pwm-sun8i.c new file +5
-0+ +/* change clock source to "mux-1" */ +clk_disable_unprepare (sun8i_pwm->clk); +devm_clk_put (sun8i_pwm->chip.dev, sun8i_pwm->clk); + sun8i_pwm->clk = clk;
sun8i_pwmis shared for all 8 PWMs, right? So if you assign mux-1 here for the second mux, how does this influence the first PWM?
To clearify my question:
after the first pwm is used and enabled (maybe using mux-0) changing sun8i_pwm->clk for the second pwm is broken because then when the first pwm is disabled the wrong clock is stopped.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ |
Hi!
Quoted 19 lines · Hao Zhang
The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, each PWM pair built-in 1 clock module, 2 timer logic module and 1 programmable dead-time generator, it also support waveform capture. It has 2 clock sources OSC24M and APB1, it is different with the sun4i-pwm driver, Therefore add a new driver for it.
Signed-off-by Hao Zhang <[email protected]>--- drivers/pwm/Kconfig | 12 +- drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 drivers/pwm/pwm-sun8i.cdrivers/pwm/Kconfig +0
-0
drivers/pwm/Kconfig config PWM_STMPE · line 429
Quoted 5 lines
drivers/pwm/Kconfig +1
-1@@ -426,7 +426,7 @@ config PWM_STMPE expanders. config PWM_SUN4I - tristate "Allwinner PWM support" + tristate "Allwinner SUN4I PWM support"
That should be a separate patch.
(also, your patch series don't seem to have the threading properly configured, you might want to fix that.)
Quoted 7 lines
drivers/pwm/Kconfig +0
-0depends on ARCH_SUNXI || COMPILE_TEST depends on HAS_IOMEM && COMMON_CLK help@@ -435,6 +435,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i.
drivers/pwm/Kconfig config PWM_SUN4I · line 447
Quoted 10 lines
drivers/pwm/Kconfig +10
-0+config PWM_SUN8I + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on HAS_IOMEM && COMMON_CLK + help + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun8i. +
sun8i here (and in the rest of the driver) is too vague. There's already plenty of SoCs part of the sun8i family that are supported by the other driver. sun8i-r40 would be a better fit (and there's no need to mention all the rebranding that allwinner has done with the R40, just use R40).
Quoted 195 lines
drivers/pwm/Kconfig +0
-0config PWM_TEGRA tristate "NVIDIA Tegra PWM support" depends on ARCH_TEGRAdrivers/pwm/Makefile +1
-0@@ -43,6 +43,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_SUN8I) += pwm-sun8i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.odrivers/pwm/pwm-sun8i.c new file +174
-0@@ -0,0 +1,418 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Hao Zhang <[email protected]> + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/time.h> +#include <linux/regmap.h> + +#define PWM_IRQ_ENABLE_REG 0x0000 +#define PCIE (ch) BIT(ch) + +#define PWM_IRQ_STATUS_REG 0x0004 +#define PIS (ch) BIT(ch) + +#define CAPTURE_IRQ_ENABLE_REG 0x0010 +#define CRIE (ch) BIT((ch) * 2) +#define CFIE (ch) BIT((ch) * 2 + 1) + +#define CAPTURE_IRQ_STATUS_REG 0x0014 +#define CRIS (ch) BIT((ch) * 2) +#define CFIS (ch) BIT((ch) * 2 + 1) + +#define CLK_CFG_REG (ch) (0x0020 + ((ch) >> 1) * 4) +#define CLK_SRC_SEL GENMASK(8, 7) +#define CLK_SRC_BYPASS_SEC BIT(6) +#define CLK_SRC_BYPASS_FIR BIT(5) +#define CLK_GATING BIT(4) +#define CLK_DIV_M GENMASK(3, 0) + +#define PWM_DZ_CTR_REG (ch) (0x0030 + ((ch) >> 1) * 4) +#define PWM_DZ_INTV GENMASK(15, 8) +#define PWM_DZ_EN BIT(0) + +#define PWM_ENABLE_REG 0x0040 +#define PWM_EN (ch) BIT(ch) + +#define CAPTURE_ENABLE_REG 0x0044 +#define CAP_EN (ch) BIT(ch) + +#define PWM_CTR_REG (ch) (0x0060 + (ch) * 0x20) +#define PWM_PERIOD_RDY BIT(11) +#define PWM_PUL_START BIT(10) +#define PWM_MODE BIT(9) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (ch) (0x0064 + (ch) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0) + +#define PWM_CNT_REG (ch) (0x0068 + (ch) * 0x20) +#define PWM_CNT_VAL GENMASK(15, 0) + +#define CAPTURE_CTR_REG (ch) (0x006c + (ch) * 0x20) +#define CAPTURE_CRLF BIT(2) +#define CAPTURE_CFLF BIT(1) +#define CAPINV BIT(0) + +#define CAPTURE_RISE_REG (ch) (0x0070 + (ch) * 0x20) +#define CAPTURE_CRLR GENMASK(15, 0) + +#define CAPTURE_FALL_REG (ch) (0x0074 + (ch) * 0x20) +#define CAPTURE_CFLR GENMASK(15, 0) + +struct sun8i_pwm_chip { +struct pwm_chip chip; +struct clk *clk; +void __iomem *base; +const struct sun8i_pwm_data *data; +struct regmap *regmap; +}; + +static struct sun8i_pwm_chip *to_sun8i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun8i_pwm_chip, chip); +} + +static u32 sun8i_pwm_read (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long offset) +{ +u32 val; + +regmap_read (sun8i_pwm->regmap, offset, &val); +return val; +} + +static void sun8i_pwm_set_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit, bit); +} + +static void sun8i_pwm_clear_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit,0 ); +} + +static void sun8i_pwm_set_value (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 mask,u32 val) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, mask, val); +} + +static void sun8i_pwm_set_polarity (struct sun8i_pwm_chip *chip,u32 ch, +enum pwm_polarity polarity) +{ +if (polarity ==PWM_POLARITY_NORMAL ) +sun8i_pwm_set_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +else +sun8i_pwm_clear_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +} + +static int sun8i_pwm_config (struct sun8i_pwm_chip *sun8i_pwm,u8 ch, +struct pwm_state *state) +{ +u64 clk_rate, clk_div, val; +u16 prescaler =0 ; +u16 div_id =0 ; +struct clk *clk; +bool is_clk; +int ret; + + clk_rate =clk_get_rate (sun8i_pwm->clk); + +/* check period and select clock source */ + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (val <=1 && is_clk) { + clk =devm_clk_get (sun8i_pwm->chip.dev,"mux-1" ); +if (IS_ERR (clk)) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + + clk_rate =clk_get_rate (clk); + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + +/* change clock source to "mux-1" */ +clk_disable_unprepare (sun8i_pwm->clk); +devm_clk_put (sun8i_pwm->chip.dev, sun8i_pwm->clk); + sun8i_pwm->clk = clk; + + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (sun8i_pwm->chip.dev, +"Failed to enable PWM clock\n" ); +return ret; + } + + }else { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + }
drivers/pwm/pwm-sun8i.c line 184
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (is_clk) +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,0 <<7 ); +else +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,1 <<7 ); + +dev_info (sun8i_pwm->chip.dev,"clock source freq:%lldHz\n" , clk_rate);
This is pretty much reimplementing the clock framework. I guess you'd be better off just modeling this clock as a clock registered in the framework. It will take care by itself of the combination of muxing and rate, and making sure the parent clocks are properly enabled when needed.
Quoted 131 lines
drivers/pwm/pwm-sun8i.c new file +131
-0+/* calculate and set prescaler, div table, PWM entire cycle */ + clk_div = val; +while (clk_div >65535 ) { + prescaler++; + clk_div = val; +do_div (clk_div,1U << div_id); +do_div (clk_div, prescaler +1 ); + +if (prescaler ==255 ) { + prescaler =0 ; + div_id++; +if (div_id ==9 ) { +dev_err (sun8i_pwm->chip.dev, +"unsupport period value\n" ); +return -EINVAL ; + } + } + } + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ENTIRE_CYCLE , clk_div <<16 ); +sun8i_pwm_set_value (sun8i_pwm,PWM_CTR_REG (ch), +PWM_PRESCAL_K , prescaler <<0 ); +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_DIV_M , div_id <<0 ); + +/* set duty cycle */ + val = state->period; +do_div (val, clk_div); + clk_div = state->duty_cycle; +do_div (clk_div, val); +if (clk_div >65535 ) + clk_div =65535 ; + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ACT_CYCLE , clk_div <<0 ); + +return 0 ; +} + +static int sun8i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +int ret; +struct sun8i_pwm_chip *sun8i_pwm =to_sun8i_pwm_chip (chip); +struct pwm_state cstate; + +pwm_get_state (pwm, &cstate); +if (!cstate.enabled) { + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (chip->dev,"Failed to enable PWM clock\n" ); +return ret; + } + } + +if ((cstate.period != state->period) || + (cstate.duty_cycle != state->duty_cycle)) { + ret =sun8i_pwm_config (sun8i_pwm, pwm->hwpwm, state); +if (ret) { +dev_err (chip->dev,"Failed to config PWM\n" ); +return ret; + } + } + +if (state->polarity != cstate.polarity) +sun8i_pwm_set_polarity (sun8i_pwm, pwm->hwpwm, state->polarity); + +if (state->enabled) { +sun8i_pwm_set_bit (sun8i_pwm, +CLK_CFG_REG (pwm->hwpwm),CLK_GATING ); + +sun8i_pwm_set_bit (sun8i_pwm, +PWM_ENABLE_REG ,PWM_EN (pwm->hwpwm)); + }else { +sun8i_pwm_clear_bit (sun8i_pwm, +CLK_CFG_REG (pwm->hwpwm),CLK_GATING ); + +sun8i_pwm_clear_bit (sun8i_pwm, +PWM_ENABLE_REG ,PWM_EN (pwm->hwpwm)); + } + +return 0 ; +} + +static void sun8i_pwm_get_state (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +struct sun8i_pwm_chip *sun8i_pwm =to_sun8i_pwm_chip (chip); +u64 clk_rate, tmp; +u32 val; +u16 clk_div, act_cycle; +u8 prescal, div_id; +u8 chn = pwm->hwpwm; + + clk_rate =clk_get_rate (sun8i_pwm->clk); + + val =sun8i_pwm_read (sun8i_pwm,PWM_CTR_REG (chn)); +if (PWM_ACT_STA & val) + state->polarity =PWM_POLARITY_NORMAL ; +else + state->polarity =PWM_POLARITY_INVERSED ; + + prescal =PWM_PRESCAL_K & val; + + val =sun8i_pwm_read (sun8i_pwm,PWM_ENABLE_REG ); +if (PWM_EN (chn) & val) + state->enabled = true; +else + state->enabled = false; + + val =sun8i_pwm_read (sun8i_pwm,PWM_PERIOD_REG (chn)); + act_cycle =PWM_ACT_CYCLE & val; + clk_div = val >>16 ; + + val =sun8i_pwm_read (sun8i_pwm,CLK_CFG_REG (chn)); + div_id =CLK_DIV_M & val; + + tmp = act_cycle * prescal * (1U << div_id) *NSEC_PER_SEC ; + state->duty_cycle =DIV_ROUND_CLOSEST_ULL (tmp, clk_rate); + tmp = clk_div * prescal * (1U << div_id) *NSEC_PER_SEC ; + state->period =DIV_ROUND_CLOSEST_ULL (tmp, clk_rate); +} + +static const struct regmap_config sun8i_pwm_regmap_config = { + .reg_bits =32 , + .reg_stride =4 , + .val_bits =32 , + .max_register =CAPTURE_FALL_REG (7 ), +}; +
drivers/pwm/pwm-sun8i.c line 326
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+static const struct pwm_ops sun8i_pwm_ops = { + .apply = sun8i_pwm_apply, + .get_state = sun8i_pwm_get_state, + .owner =THIS_MODULE , +}; + +static const struct of_device_id sun8i_pwm_dt_ids[] = { + { + .compatible ="allwinner,sun8i-r40-pwm" , + .data =NULL ,
Do you really need that field if you leave it NULL?
Thanks!
Maxime
Signature · 10 lines
Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 228 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20181127/c76c58d1/attachment-0001.sig>
A message that is not in the archive (sent off-list or not yet received): CAJeuY7-U=EF_FCe2jmBB-Otuemp_rcMPs=g7ipPi=YP4qy5mtg@mail.gmail.com
Hi,
Quoted 13 lines · Hao Zhang
Quoted 13 lines
drivers/pwm/Kconfig config PWM_STMPE · line 429
Quoted 10 lines
drivers/pwm/Kconfig +1
-1@@ -426,7 +426,7 @@ config PWM_STMPE expanders. config PWM_SUN4I - tristate "Allwinner PWM support" + tristate "Allwinner SUN4I PWM support"That should be a separate patch.
Quoted 8 lines
Quoted 2 lines
(also, your patch series don't seem to have the threading properly configured, you might want to fix that.)
Do you mean i should fix it like this ?
config sunxi pwm| |---->config PWM_SUN4I |---->config PWM_SUN8I_R40
No, I meant for your mails, sorry. Each patch should be sent in reply to your cover letter, and they are all sent as separate mails, which makes it hard to track.
I'm not sure how you're sending your patches, but using git send-email this would be using --no-chain-reply-to --thread if I remember well.
Quoted 212 lines
Quoted 203 lines
Quoted 203 lines
drivers/pwm/Kconfig +0
-0config PWM_TEGRA tristate "NVIDIA Tegra PWM support" depends on ARCH_TEGRAdrivers/pwm/Makefile +1
-0@@ -43,6 +43,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_SUN8I) += pwm-sun8i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.odrivers/pwm/pwm-sun8i.c new file +182
-0@@ -0,0 +1,418 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Hao Zhang <[email protected]> + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/time.h> +#include <linux/regmap.h> + +#define PWM_IRQ_ENABLE_REG 0x0000 +#define PCIE (ch) BIT(ch) + +#define PWM_IRQ_STATUS_REG 0x0004 +#define PIS (ch) BIT(ch) + +#define CAPTURE_IRQ_ENABLE_REG 0x0010 +#define CRIE (ch) BIT((ch) * 2) +#define CFIE (ch) BIT((ch) * 2 + 1) + +#define CAPTURE_IRQ_STATUS_REG 0x0014 +#define CRIS (ch) BIT((ch) * 2) +#define CFIS (ch) BIT((ch) * 2 + 1) + +#define CLK_CFG_REG (ch) (0x0020 + ((ch) >> 1) * 4) +#define CLK_SRC_SEL GENMASK(8, 7) +#define CLK_SRC_BYPASS_SEC BIT(6) +#define CLK_SRC_BYPASS_FIR BIT(5) +#define CLK_GATING BIT(4) +#define CLK_DIV_M GENMASK(3, 0) + +#define PWM_DZ_CTR_REG (ch) (0x0030 + ((ch) >> 1) * 4) +#define PWM_DZ_INTV GENMASK(15, 8) +#define PWM_DZ_EN BIT(0) + +#define PWM_ENABLE_REG 0x0040 +#define PWM_EN (ch) BIT(ch) + +#define CAPTURE_ENABLE_REG 0x0044 +#define CAP_EN (ch) BIT(ch) + +#define PWM_CTR_REG (ch) (0x0060 + (ch) * 0x20) +#define PWM_PERIOD_RDY BIT(11) +#define PWM_PUL_START BIT(10) +#define PWM_MODE BIT(9) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (ch) (0x0064 + (ch) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0) + +#define PWM_CNT_REG (ch) (0x0068 + (ch) * 0x20) +#define PWM_CNT_VAL GENMASK(15, 0) + +#define CAPTURE_CTR_REG (ch) (0x006c + (ch) * 0x20) +#define CAPTURE_CRLF BIT(2) +#define CAPTURE_CFLF BIT(1) +#define CAPINV BIT(0) + +#define CAPTURE_RISE_REG (ch) (0x0070 + (ch) * 0x20) +#define CAPTURE_CRLR GENMASK(15, 0) + +#define CAPTURE_FALL_REG (ch) (0x0074 + (ch) * 0x20) +#define CAPTURE_CFLR GENMASK(15, 0) + +struct sun8i_pwm_chip { +struct pwm_chip chip; +struct clk *clk; +void __iomem *base; +const struct sun8i_pwm_data *data; +struct regmap *regmap; +}; + +static struct sun8i_pwm_chip *to_sun8i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun8i_pwm_chip, chip); +} + +static u32 sun8i_pwm_read (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long offset) +{ +u32 val; + +regmap_read (sun8i_pwm->regmap, offset, &val); +return val; +} + +static void sun8i_pwm_set_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit, bit); +} + +static void sun8i_pwm_clear_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit,0 ); +} + +static void sun8i_pwm_set_value (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 mask,u32 val) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, mask, val); +} + +static void sun8i_pwm_set_polarity (struct sun8i_pwm_chip *chip,u32 ch, +enum pwm_polarity polarity) +{ +if (polarity ==PWM_POLARITY_NORMAL ) +sun8i_pwm_set_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +else +sun8i_pwm_clear_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +} + +static int sun8i_pwm_config (struct sun8i_pwm_chip *sun8i_pwm,u8 ch, +struct pwm_state *state) +{ +u64 clk_rate, clk_div, val; +u16 prescaler =0 ; +u16 div_id =0 ; +struct clk *clk; +bool is_clk; +int ret; + + clk_rate =clk_get_rate (sun8i_pwm->clk); + +/* check period and select clock source */ + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (val <=1 && is_clk) { + clk =devm_clk_get (sun8i_pwm->chip.dev,"mux-1" ); +if (IS_ERR (clk)) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + + clk_rate =clk_get_rate (clk); + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + +/* change clock source to "mux-1" */ +clk_disable_unprepare (sun8i_pwm->clk); +devm_clk_put (sun8i_pwm->chip.dev, sun8i_pwm->clk); + sun8i_pwm->clk = clk; + + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (sun8i_pwm->chip.dev, +"Failed to enable PWM clock\n" ); +return ret; + } + + }else { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (is_clk) +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,0 <<7 ); +else +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,1 <<7 );Quoted 8 lines
drivers/pwm/pwm-sun8i.c line 184
Quoted 2 lines
drivers/pwm/pwm-sun8i.c new file +2
-0+ +dev_info (sun8i_pwm->chip.dev,"clock source freq:%lldHz\n" , clk_rate);This is pretty much reimplementing the clock framework. I guess you'd be better off just modeling this clock as a clock registered in the framework. It will take care by itself of the combination of muxing and rate, and making sure the parent clocks are properly enabled when needed.
Quoted 1 line
Do you mean i should move it to ccu clock framwork?
You don't need to move it anywhere, you can declare a clock in a driver, without being in drivers/clk. We're doing that in the DRM or the RTC drivers for example.
Maxime
Signature · 3 lines
Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Quoted 199 lines · Hao Zhang
The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, each PWM pair built-in 1 clock module, 2 timer logic module and 1 programmable dead-time generator, it also support waveform capture. It has 2 clock sources OSC24M and APB1, it is different with the sun4i-pwm driver, Therefore add a new driver for it.
Signed-off-by Hao Zhang <[email protected]>--- drivers/pwm/Kconfig | 12 +- drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 drivers/pwm/pwm-sun8i.cdrivers/pwm/Kconfig +11
-1@@ -426,7 +426,7 @@ config PWM_STMPE expanders. config PWM_SUN4I - tristate "Allwinner PWM support" + tristate "Allwinner SUN4I PWM support" depends on ARCH_SUNXI || COMPILE_TEST depends on HAS_IOMEM && COMMON_CLK help@@ -435,6 +435,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN8I + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on HAS_IOMEM && COMMON_CLK + help + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun8i. + config PWM_TEGRA tristate "NVIDIA Tegra PWM support" depends on ARCH_TEGRAdrivers/pwm/Makefile +1
-0@@ -43,6 +43,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_SUN8I) += pwm-sun8i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.odrivers/pwm/pwm-sun8i.c new file +137
-0@@ -0,0 +1,418 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Hao Zhang <[email protected]> + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/time.h> +#include <linux/regmap.h> + +#define PWM_IRQ_ENABLE_REG 0x0000 +#define PCIE (ch) BIT(ch) + +#define PWM_IRQ_STATUS_REG 0x0004 +#define PIS (ch) BIT(ch) + +#define CAPTURE_IRQ_ENABLE_REG 0x0010 +#define CRIE (ch) BIT((ch) * 2) +#define CFIE (ch) BIT((ch) * 2 + 1) + +#define CAPTURE_IRQ_STATUS_REG 0x0014 +#define CRIS (ch) BIT((ch) * 2) +#define CFIS (ch) BIT((ch) * 2 + 1) + +#define CLK_CFG_REG (ch) (0x0020 + ((ch) >> 1) * 4) +#define CLK_SRC_SEL GENMASK(8, 7) +#define CLK_SRC_BYPASS_SEC BIT(6) +#define CLK_SRC_BYPASS_FIR BIT(5) +#define CLK_GATING BIT(4) +#define CLK_DIV_M GENMASK(3, 0) + +#define PWM_DZ_CTR_REG (ch) (0x0030 + ((ch) >> 1) * 4) +#define PWM_DZ_INTV GENMASK(15, 8) +#define PWM_DZ_EN BIT(0) + +#define PWM_ENABLE_REG 0x0040 +#define PWM_EN (ch) BIT(ch) + +#define CAPTURE_ENABLE_REG 0x0044 +#define CAP_EN (ch) BIT(ch) + +#define PWM_CTR_REG (ch) (0x0060 + (ch) * 0x20) +#define PWM_PERIOD_RDY BIT(11) +#define PWM_PUL_START BIT(10) +#define PWM_MODE BIT(9) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (ch) (0x0064 + (ch) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0) + +#define PWM_CNT_REG (ch) (0x0068 + (ch) * 0x20) +#define PWM_CNT_VAL GENMASK(15, 0) + +#define CAPTURE_CTR_REG (ch) (0x006c + (ch) * 0x20) +#define CAPTURE_CRLF BIT(2) +#define CAPTURE_CFLF BIT(1) +#define CAPINV BIT(0) + +#define CAPTURE_RISE_REG (ch) (0x0070 + (ch) * 0x20) +#define CAPTURE_CRLR GENMASK(15, 0) + +#define CAPTURE_FALL_REG (ch) (0x0074 + (ch) * 0x20) +#define CAPTURE_CFLR GENMASK(15, 0) + +struct sun8i_pwm_chip { +struct pwm_chip chip; +struct clk *clk; +void __iomem *base; +const struct sun8i_pwm_data *data; +struct regmap *regmap; +}; + +static struct sun8i_pwm_chip *to_sun8i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun8i_pwm_chip, chip); +} + +static u32 sun8i_pwm_read (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long offset) +{ +u32 val; + +regmap_read (sun8i_pwm->regmap, offset, &val); +return val; +} + +static void sun8i_pwm_set_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit, bit); +} + +static void sun8i_pwm_clear_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit,0 ); +} + +static void sun8i_pwm_set_value (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 mask,u32 val) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, mask, val); +} + +static void sun8i_pwm_set_polarity (struct sun8i_pwm_chip *chip,u32 ch, +enum pwm_polarity polarity) +{ +if (polarity ==PWM_POLARITY_NORMAL ) +sun8i_pwm_set_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +else +sun8i_pwm_clear_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +} + +static int sun8i_pwm_config (struct sun8i_pwm_chip *sun8i_pwm,u8 ch, +struct pwm_state *state) +{ +u64 clk_rate, clk_div, val; +u16 prescaler =0 ; +u16 div_id =0 ; +struct clk *clk; +bool is_clk; +int ret; + + clk_rate =clk_get_rate (sun8i_pwm->clk); + +/* check period and select clock source */
drivers/pwm/pwm-sun8i.c line 147
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (val <=1 && is_clk) { + clk =devm_clk_get (sun8i_pwm->chip.dev,"mux-1" ); +if (IS_ERR (clk)) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + }
You should never try to get resources at this point. You should have requested them already at ->probe() time. Otherwise, how are you going to handle failures here?
Quoted 92 lines
drivers/pwm/pwm-sun8i.c new file +92
-0+ + clk_rate =clk_get_rate (clk); + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + +/* change clock source to "mux-1" */ +clk_disable_unprepare (sun8i_pwm->clk); +devm_clk_put (sun8i_pwm->chip.dev, sun8i_pwm->clk); + sun8i_pwm->clk = clk; + + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (sun8i_pwm->chip.dev, +"Failed to enable PWM clock\n" ); +return ret; + } + + }else { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (is_clk) +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,0 <<7 ); +else +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,1 <<7 ); + +dev_info (sun8i_pwm->chip.dev,"clock source freq:%lldHz\n" , clk_rate); + +/* calculate and set prescaler, div table, PWM entire cycle */ + clk_div = val; +while (clk_div >65535 ) { + prescaler++; + clk_div = val; +do_div (clk_div,1U << div_id); +do_div (clk_div, prescaler +1 ); + +if (prescaler ==255 ) { + prescaler =0 ; + div_id++; +if (div_id ==9 ) { +dev_err (sun8i_pwm->chip.dev, +"unsupport period value\n" ); +return -EINVAL ; + } + } + } + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ENTIRE_CYCLE , clk_div <<16 ); +sun8i_pwm_set_value (sun8i_pwm,PWM_CTR_REG (ch), +PWM_PRESCAL_K , prescaler <<0 ); +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_DIV_M , div_id <<0 ); + +/* set duty cycle */ + val = state->period; +do_div (val, clk_div); + clk_div = state->duty_cycle; +do_div (clk_div, val); +if (clk_div >65535 ) + clk_div =65535 ; + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ACT_CYCLE , clk_div <<0 ); + +return 0 ; +} + +static int sun8i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +int ret; +struct sun8i_pwm_chip *sun8i_pwm =to_sun8i_pwm_chip (chip); +struct pwm_state cstate; + +pwm_get_state (pwm, &cstate); +if (!cstate.enabled) { + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (chip->dev,"Failed to enable PWM clock\n" ); +return ret; + }
drivers/pwm/pwm-sun8i.c line 249
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ } + +if ((cstate.period != state->period) || + (cstate.duty_cycle != state->duty_cycle)) { + ret =sun8i_pwm_config (sun8i_pwm, pwm->hwpwm, state); +if (ret) { +dev_err (chip->dev,"Failed to config PWM\n" ); +return ret; + } + }
So this isn't really how atomic is supposed to work. The whole point of the single callback is to allow the driver to apply the changes in an atomic way, which means that either everything is applied or nothing is applied.
That's not what you do here. In the above you can end up with an enabled clock but the settings not being applied. Similarly sun8i_pwm_config() can abort in a number of places, which would leave you with a half- configured PWM channel.
Instead, what you should be doing is precompute everything and check that the configuration can be applied before touching any registers or enabling clocks. Once you've validate the new state, you need to write everything and there should be no more risk of failure.
Thierry
Quoted 207 lines · Thierry Reding <[email protected]>…
Quoted 205 lines · Hao Zhang
The sun8i R40/T3/V40 PWM has 8 PWM channals and divides to 4 PWM pairs, each PWM pair built-in 1 clock module, 2 timer logic module and 1 programmable dead-time generator, it also support waveform capture. It has 2 clock sources OSC24M and APB1, it is different with the sun4i-pwm driver, Therefore add a new driver for it.
Signed-off-by Hao Zhang <[email protected]>--- drivers/pwm/Kconfig | 12 +- drivers/pwm/Makefile | 1 + drivers/pwm/pwm-sun8i.c | 418 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 430 insertions(+), 1 deletion(-) create mode 100644 drivers/pwm/pwm-sun8i.cdrivers/pwm/Kconfig +11
-1@@ -426,7 +426,7 @@ config PWM_STMPE expanders. config PWM_SUN4I - tristate "Allwinner PWM support" + tristate "Allwinner SUN4I PWM support" depends on ARCH_SUNXI || COMPILE_TEST depends on HAS_IOMEM && COMMON_CLK help@@ -435,6 +435,16 @@ config PWM_SUN4I To compile this driver as a module, choose M here: the module will be called pwm-sun4i. +config PWM_SUN8I + tristate "Allwinner SUN8I (R40/V40/T3) PWM support" + depends on ARCH_SUNXI || COMPILE_TEST + depends on HAS_IOMEM && COMMON_CLK + help + Generic PWM framework driver for Allwinner R40/V40/T3 SoCs. + + To compile this driver as a module, choose M here: the module + will be called pwm-sun8i. + config PWM_TEGRA tristate "NVIDIA Tegra PWM support" depends on ARCH_TEGRAdrivers/pwm/Makefile +1
-0@@ -43,6 +43,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_SUN8I) += pwm-sun8i.o obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o obj-$(CONFIG_PWM_TIEHRPWM) += pwm-tiehrpwm.odrivers/pwm/pwm-sun8i.c new file +143
-0@@ -0,0 +1,418 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (C) 2018 Hao Zhang <[email protected]> + +#include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/slab.h> +#include <linux/spinlock.h> +#include <linux/time.h> +#include <linux/regmap.h> + +#define PWM_IRQ_ENABLE_REG 0x0000 +#define PCIE (ch) BIT(ch) + +#define PWM_IRQ_STATUS_REG 0x0004 +#define PIS (ch) BIT(ch) + +#define CAPTURE_IRQ_ENABLE_REG 0x0010 +#define CRIE (ch) BIT((ch) * 2) +#define CFIE (ch) BIT((ch) * 2 + 1) + +#define CAPTURE_IRQ_STATUS_REG 0x0014 +#define CRIS (ch) BIT((ch) * 2) +#define CFIS (ch) BIT((ch) * 2 + 1) + +#define CLK_CFG_REG (ch) (0x0020 + ((ch) >> 1) * 4) +#define CLK_SRC_SEL GENMASK(8, 7) +#define CLK_SRC_BYPASS_SEC BIT(6) +#define CLK_SRC_BYPASS_FIR BIT(5) +#define CLK_GATING BIT(4) +#define CLK_DIV_M GENMASK(3, 0) + +#define PWM_DZ_CTR_REG (ch) (0x0030 + ((ch) >> 1) * 4) +#define PWM_DZ_INTV GENMASK(15, 8) +#define PWM_DZ_EN BIT(0) + +#define PWM_ENABLE_REG 0x0040 +#define PWM_EN (ch) BIT(ch) + +#define CAPTURE_ENABLE_REG 0x0044 +#define CAP_EN (ch) BIT(ch) + +#define PWM_CTR_REG (ch) (0x0060 + (ch) * 0x20) +#define PWM_PERIOD_RDY BIT(11) +#define PWM_PUL_START BIT(10) +#define PWM_MODE BIT(9) +#define PWM_ACT_STA BIT(8) +#define PWM_PRESCAL_K GENMASK(7, 0) + +#define PWM_PERIOD_REG (ch) (0x0064 + (ch) * 0x20) +#define PWM_ENTIRE_CYCLE GENMASK(31, 16) +#define PWM_ACT_CYCLE GENMASK(15, 0) + +#define PWM_CNT_REG (ch) (0x0068 + (ch) * 0x20) +#define PWM_CNT_VAL GENMASK(15, 0) + +#define CAPTURE_CTR_REG (ch) (0x006c + (ch) * 0x20) +#define CAPTURE_CRLF BIT(2) +#define CAPTURE_CFLF BIT(1) +#define CAPINV BIT(0) + +#define CAPTURE_RISE_REG (ch) (0x0070 + (ch) * 0x20) +#define CAPTURE_CRLR GENMASK(15, 0) + +#define CAPTURE_FALL_REG (ch) (0x0074 + (ch) * 0x20) +#define CAPTURE_CFLR GENMASK(15, 0) + +struct sun8i_pwm_chip { +struct pwm_chip chip; +struct clk *clk; +void __iomem *base; +const struct sun8i_pwm_data *data; +struct regmap *regmap; +}; + +static struct sun8i_pwm_chip *to_sun8i_pwm_chip (struct pwm_chip *chip) +{ + return container_of(chip, struct sun8i_pwm_chip, chip); +} + +static u32 sun8i_pwm_read (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long offset) +{ +u32 val; + +regmap_read (sun8i_pwm->regmap, offset, &val); +return val; +} + +static void sun8i_pwm_set_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit, bit); +} + +static void sun8i_pwm_clear_bit (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 bit) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, bit,0 ); +} + +static void sun8i_pwm_set_value (struct sun8i_pwm_chip *sun8i_pwm, +unsigned long reg,u32 mask,u32 val) +{ +regmap_update_bits (sun8i_pwm->regmap, reg, mask, val); +} + +static void sun8i_pwm_set_polarity (struct sun8i_pwm_chip *chip,u32 ch, +enum pwm_polarity polarity) +{ +if (polarity ==PWM_POLARITY_NORMAL ) +sun8i_pwm_set_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +else +sun8i_pwm_clear_bit (chip,PWM_CTR_REG (ch),PWM_ACT_STA ); +} + +static int sun8i_pwm_config (struct sun8i_pwm_chip *sun8i_pwm,u8 ch, +struct pwm_state *state) +{ +u64 clk_rate, clk_div, val; +u16 prescaler =0 ; +u16 div_id =0 ; +struct clk *clk; +bool is_clk; +int ret; + + clk_rate =clk_get_rate (sun8i_pwm->clk); + +/* check period and select clock source */ + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (val <=1 && is_clk) { + clk =devm_clk_get (sun8i_pwm->chip.dev,"mux-1" ); +if (IS_ERR (clk)) {
Quoted 9 lines
drivers/pwm/pwm-sun8i.c line 147
Quoted 4 lines
drivers/pwm/pwm-sun8i.c new file +4
-0+dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + }You should never try to get resources at this point. You should have requested them already at ->probe() time. Otherwise, how are you going to handle failures here?
Something wrong here, i will fix it, thanks :)
Quoted 113 lines
Quoted 92 lines
drivers/pwm/pwm-sun8i.c new file +92
-0+ + clk_rate =clk_get_rate (clk); + val = state->period * clk_rate; +do_div (val,NSEC_PER_SEC ); +if (val <=1 ) { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + +/* change clock source to "mux-1" */ +clk_disable_unprepare (sun8i_pwm->clk); +devm_clk_put (sun8i_pwm->chip.dev, sun8i_pwm->clk); + sun8i_pwm->clk = clk; + + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (sun8i_pwm->chip.dev, +"Failed to enable PWM clock\n" ); +return ret; + } + + }else { +dev_err (sun8i_pwm->chip.dev, +"Period expects a larger value\n" ); +return -EINVAL ; + } + + is_clk =devm_clk_name_match (sun8i_pwm->clk,"mux-0" ); +if (is_clk) +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,0 <<7 ); +else +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_SRC_SEL ,1 <<7 ); + +dev_info (sun8i_pwm->chip.dev,"clock source freq:%lldHz\n" , clk_rate); + +/* calculate and set prescaler, div table, PWM entire cycle */ + clk_div = val; +while (clk_div >65535 ) { + prescaler++; + clk_div = val; +do_div (clk_div,1U << div_id); +do_div (clk_div, prescaler +1 ); + +if (prescaler ==255 ) { + prescaler =0 ; + div_id++; +if (div_id ==9 ) { +dev_err (sun8i_pwm->chip.dev, +"unsupport period value\n" ); +return -EINVAL ; + } + } + } + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ENTIRE_CYCLE , clk_div <<16 ); +sun8i_pwm_set_value (sun8i_pwm,PWM_CTR_REG (ch), +PWM_PRESCAL_K , prescaler <<0 ); +sun8i_pwm_set_value (sun8i_pwm,CLK_CFG_REG (ch), +CLK_DIV_M , div_id <<0 ); + +/* set duty cycle */ + val = state->period; +do_div (val, clk_div); + clk_div = state->duty_cycle; +do_div (clk_div, val); +if (clk_div >65535 ) + clk_div =65535 ; + +sun8i_pwm_set_value (sun8i_pwm,PWM_PERIOD_REG (ch), +PWM_ACT_CYCLE , clk_div <<0 ); + +return 0 ; +} + +static int sun8i_pwm_apply (struct pwm_chip *chip,struct pwm_device *pwm, +struct pwm_state *state) +{ +int ret; +struct sun8i_pwm_chip *sun8i_pwm =to_sun8i_pwm_chip (chip); +struct pwm_state cstate; + +pwm_get_state (pwm, &cstate); +if (!cstate.enabled) { + ret =clk_prepare_enable (sun8i_pwm->clk); +if (ret) { +dev_err (chip->dev,"Failed to enable PWM clock\n" ); +return ret; + }drivers/pwm/pwm-sun8i.c line 249
Quoted 10 lines
drivers/pwm/pwm-sun8i.c new file +10
-0+ } + +if ((cstate.period != state->period) || + (cstate.duty_cycle != state->duty_cycle)) { + ret =sun8i_pwm_config (sun8i_pwm, pwm->hwpwm, state); +if (ret) { +dev_err (chip->dev,"Failed to config PWM\n" ); +return ret; + } + }So this isn't really how atomic is supposed to work. The whole point of the single callback is to allow the driver to apply the changes in an atomic way, which means that either everything is applied or nothing is applied.
That's not what you do here. In the above you can end up with an enabled clock but the settings not being applied. Similarly
sun8i_pwm_config()can abort in a number of places, which would leave you with a half- configured PWM channel.
Quoted 5 lines
Instead, what you should be doing is precompute everything and check that the configuration can be applied before touching any registers or enabling clocks. Once you've validate the new state, you need to write everything and there should be no more risk of failure.
Got it, it is useful for me !
Quoted 17 lines
Thierry
-----BEGIN PGP SIGNATURE-----iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlwb2B4ACgkQ3SOs138+ s6GmgRAAwvTyhaFa0jWmUftz+0FS5JTXADm9VusyhbSlmHV7UNkqkQuLnaT1WoGi gGW5ZMLA+wSPWrXqGsw1HuzLc57c1/eDQdxLmlqBz0Lwribn75+0kisEvV3CRCEL P9UiTgMualhTe0DGKZmcHJvV56sEhx7b2WQ81sOn5nZvCP/iFTURp5WSyk+phAyb ss9i5bjwuxC34wLC0wCVNvr2XlpdD7CTy0R/nap1Za2nf6Cm0TnzGGPIYki1/gM2 fUtYT/zCvfGp+JG5+R2755XdomXzj0vmXD7Omv75eKzMb+HivolmLPJEfUiP8c+x SElT/F/jIkwvdBzyeQ/sA8ybh4N0DVapnUxBjrUBLOMlrNDlh+ApjUOOpwylQBXQ 6JKmjoHoBg31lK9QEPxoIoNP0FdZBr6+lAaZeQNx95PeICxD21IvPtuLEp4ksbX7 fJsOxbkBzbIMpKuxLM3vkl89/O9IhPbVoyvMASCPMmdwAIH+LSy3SvIFWbGQ0nXS gwjTzN4r1AWmdXIN+faT6khnNY64WoPd896AyJSQ5mQsl42IMKRi42kpfeN3/f8y x4d07WSTCRB4v+OehJEVP2WJNsbI3EvtClbngn66xSac7kGSbg4cpbW0NzkrFj+T dOG2p9wUMibPBXzS+V7JzTsDgR9JG2fzxG8PhEesaG/Bt32Hg0Y= =S1Wz
-----END PGP SIGNATURE-----
Signature · 3 lines
You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/[email protected] For more options, visit https://groups.google.com/d/optout.