LinuxLists.cc - [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

2018-11-25 16:26:18

Subject: [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

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 504d252..6105ac8 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -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
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9c676a0..32c8d2d 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -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
diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
new file mode 100644
index 0000000..d8597e4
--- /dev/null
+++ b/drivers/pwm/pwm-sun8i.c
@@ -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);
+
+ 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;
+ }
+ }
+
+ 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),
+};
+
+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,
+ },
+ {},
+};
+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;
+ 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;
+ }
+
+ 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);
+
+ 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)) {
+ 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) {
+ dev_err(&pdev->dev, "Can't get pwm-channels\n");
+ return ret;
+ }
+
+ dev_info(&pdev->dev, "pwm-channels:%d\n", pwm->chip.npwm);
+ pwm->data = match->data;
+ pwm->chip.dev = &pdev->dev;
+ pwm->chip.ops = &sun8i_pwm_ops;
+ pwm->chip.base = -1;
+ 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);
+
+ return 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);
+}
+
+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");
+MODULE_AUTHOR("Hao Zhang <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun8i PWM driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4

2018-11-26 21:33:27

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote:
> 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
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 504d252..6105ac8 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -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
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0..32c8d2d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -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
> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> new file mode 100644
> index 0000000..d8597e4
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun8i.c
> @@ -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;

This member is only written to, but never used.

> + 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);
> +}

This function is only used once. Maybe drop the function and put the if
to the caller.

> +
> +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");

This error message looks wrong, several others, too.

> + return -EINVAL;

return PTR_ERR(clk)?

> + }
> +
> + 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;

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.

> +
> + 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?

> + }
> +
> + 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.

> +
> + /* 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;
> + }
> + }
> + }

Noting the underlying formula for the calculation and the bitwidth for
the related register fields above would be good.

> +
> + 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);

Why "<< 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;
> + }
> + }

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.

> + 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));
> + }

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?)

> +
> + 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)

This looks strange to me. While syntactically equivalent it is more
usual to write this as

if (val & PWM_ACT_STA)

> + 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?

> +
> + 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),
> +};
> +
> +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.

> + },
> + {},
> +};
> +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;
> + 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.

> +
> + 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);
> +
> + 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.

> + 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.

> + 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?

> + pwm->data = match->data;
> + pwm->chip.dev = &pdev->dev;
> + pwm->chip.ops = &sun8i_pwm_ops;
> + pwm->chip.base = -1;
> + 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;

> +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.

> +}
> +
> +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.

> +MODULE_AUTHOR("Hao Zhang <[email protected]>");
> +MODULE_DESCRIPTION("Allwinner sun8i PWM driver");
> +MODULE_LICENSE("GPL v2");

Best regards
Uwe

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

2018-11-27 10:58:19

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

Hi!

On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote:
> 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
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 504d252..6105ac8 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -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.)

> 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.
> +

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).

> config PWM_TEGRA
> tristate "NVIDIA Tegra PWM support"
> depends on ARCH_TEGRA
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 9c676a0..32c8d2d 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -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
> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> new file mode 100644
> index 0000000..d8597e4
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun8i.c
> @@ -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);
> +
> + 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.

> + /* 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),
> +};
> +
> +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

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (12.58 kB)
signature.asc (235.00 B)
Download all attachments

2018-11-27 18:47:36

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

Hello,

On Mon, Nov 26, 2018 at 10:31:58PM +0100, Uwe Kleine-K?nig wrote:
> On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote:
> > 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
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 504d252..6105ac8 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -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
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 9c676a0..32c8d2d 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -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
> > diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> > new file mode 100644
> > index 0000000..d8597e4
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sun8i.c
> > @@ -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)

> > + /* 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;
> > + }
> > + }
> > + }
>
> Noting the underlying formula for the calculation and the bitwidth for
> the related register fields above would be good.
>
> > +
> > + 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);
>
> Why "<< 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;
> > + }
> > + }
>
> 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.

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

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

2018-12-03 09:51:53

by Uwe Kleine-König

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

Hello,

On Mon, Nov 26, 2018 at 10:31:58PM +0100, Uwe Kleine-K?nig wrote:
> > +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;
> > + }
> > +
> > + /* 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?

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

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

2018-12-03 10:40:55

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

Hi,

On Sat, Dec 01, 2018 at 11:23:40PM +0800, Hao Zhang wrote:
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index 504d252..6105ac8 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -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.)
>
> 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.

> > > config PWM_TEGRA
> > > tristate "NVIDIA Tegra PWM support"
> > > depends on ARCH_TEGRA
> > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > index 9c676a0..32c8d2d 100644
> > > --- a/drivers/pwm/Makefile
> > > +++ b/drivers/pwm/Makefile
> > > @@ -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
> > > diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> > > new file mode 100644
> > > index 0000000..d8597e4
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sun8i.c
> > > @@ -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);
> > > +
> > > + 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.
>
> 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

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Attachments:

(No filename) (9.72 kB)
signature.asc (235.00 B)
Download all attachments

2018-12-21 00:32:09

by Thierry Reding

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

Attachments:

(No filename) (10.39 kB)
signature.asc (849.00 B)
Download all attachments

2019-03-12 05:43:55

by Hao Zhang

[permalink] [raw]

Subject: Re: [PATCH v3 6/6] ARM: PWM: add allwinner sun8i R40/T3/V40 PWM support.

Thierry Reding <[email protected]> 于2018年12月21日周五上午1:57写道：
>
> On Mon, Nov 26, 2018 at 12:23:19AM +0800, Hao Zhang wrote:
> > 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
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index 504d252..6105ac8 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -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
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 9c676a0..32c8d2d 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -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
> > diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> > new file mode 100644
> > index 0000000..d8597e4
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sun8i.c
> > @@ -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;
> > + }
>
> 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 :)

> > +
> > + 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;
> > + }
> > + }
> > +
> > + 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.
>

Got it, it is useful for me !

> 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-----