v2:
- fix dt-bindings
- fix a remark in the driver
v3:
- fix dt-bindings
- fix sunxi-d1s-t113.dtsi
v4:
- fix a remark in the driver
v5:
- dropped unused varibale in the driver
- fix dt-bindings
v6:
- add apb0 clock
v7:
- fix a remark in the driver
- add maintainer
v8:
- fix compile driver for 6.8-rc
Aleksandr Shubin (3):
dt-bindings: pwm: Add binding for Allwinner D1/T113-S3/R329 PWM
controller
pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
riscv: dts: allwinner: d1: Add pwm node
.../bindings/pwm/allwinner,sun20i-pwm.yaml | 88 ++++
.../boot/dts/allwinner/sunxi-d1s-t113.dtsi | 12 +
drivers/pwm/Kconfig | 10 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sun20i.c | 380 ++++++++++++++++++
5 files changed, 491 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml
create mode 100644 drivers/pwm/pwm-sun20i.c
--
2.25.1
Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM
controllers with ones supported by pwm-sun4i driver.
This patch adds a PWM controller driver for Allwinner's D1,
T113-S3 and R329 SoCs. The main difference between these SoCs
is the number of channels defined by the DT property.
Co-developed-by: Brandon Cheo Fusi <[email protected]>
Signed-off-by: Brandon Cheo Fusi <[email protected]>
Signed-off-by: Aleksandr Shubin <[email protected]>
---
drivers/pwm/Kconfig | 10 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-sun20i.c | 380 +++++++++++++++++++++++++++++++++++++++
3 files changed, 391 insertions(+)
create mode 100644 drivers/pwm/pwm-sun20i.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..d2e0a080eb3c 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -625,6 +625,16 @@ config PWM_SUN4I
To compile this driver as a module, choose M here: the module
will be called pwm-sun4i.
+config PWM_SUN20I
+ tristate "Allwinner D1/T113s/R329 PWM support"
+ depends on ARCH_SUNXI || COMPILE_TEST
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for Allwinner D1/T113s/R329 SoCs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-sun20i.
+
config PWM_SUNPLUS
tristate "Sunplus PWM support"
depends on ARCH_SUNPLUS || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c5ec9e168ee7..dcad0d5a2430 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_PWM_STM32) += pwm-stm32.o
obj-$(CONFIG_PWM_STM32_LP) += pwm-stm32-lp.o
obj-$(CONFIG_PWM_STMPE) += pwm-stmpe.o
obj-$(CONFIG_PWM_SUN4I) += pwm-sun4i.o
+obj-$(CONFIG_PWM_SUN20I) += pwm-sun20i.o
obj-$(CONFIG_PWM_SUNPLUS) += pwm-sunplus.o
obj-$(CONFIG_PWM_TEGRA) += pwm-tegra.o
obj-$(CONFIG_PWM_TIECAP) += pwm-tiecap.o
diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
new file mode 100644
index 000000000000..19bf3f495155
--- /dev/null
+++ b/drivers/pwm/pwm-sun20i.c
@@ -0,0 +1,380 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PWM Controller Driver for sunxi platforms (D1, T113-S3 and R329)
+ *
+ * Limitations:
+ * - When the parameters change, current running period will not be completed
+ * and run new settings immediately.
+ * - It output HIGH-Z state when PWM channel disabled.
+ *
+ * Copyright (c) 2023 Aleksandr Shubin <[email protected]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+
+#define SUN20I_PWM_CLK_CFG(chan) (0x20 + (((chan) >> 1) * 0x4))
+#define SUN20I_PWM_CLK_CFG_SRC GENMASK(8, 7)
+#define SUN20I_PWM_CLK_CFG_DIV_M GENMASK(3, 0)
+#define SUN20I_PWM_CLK_DIV_M_MAX 8
+
+#define SUN20I_PWM_CLK_GATE 0x40
+#define SUN20I_PWM_CLK_GATE_BYPASS(chan) BIT((chan) + 16)
+#define SUN20I_PWM_CLK_GATE_GATING(chan) BIT(chan)
+
+#define SUN20I_PWM_ENABLE 0x80
+#define SUN20I_PWM_ENABLE_EN(chan) BIT(chan)
+
+#define SUN20I_PWM_CTL(chan) (0x100 + (chan) * 0x20)
+#define SUN20I_PWM_CTL_ACT_STA BIT(8)
+#define SUN20I_PWM_CTL_PRESCAL_K GENMASK(7, 0)
+#define SUN20I_PWM_CTL_PRESCAL_K_MAX 0xff
+
+#define SUN20I_PWM_PERIOD(chan) (0x104 + (chan) * 0x20)
+#define SUN20I_PWM_PERIOD_ENTIRE_CYCLE GENMASK(31, 16)
+#define SUN20I_PWM_PERIOD_ACT_CYCLE GENMASK(15, 0)
+
+#define SUN20I_PWM_PCNTR_SIZE BIT(16)
+
+/**
+ * SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
+ * div_m (SUN20I_PWM_CLK_CFG_DIV_M) & prescale_k (SUN20I_PWM_CTL_PRESCAL_K)
+ * without using a loop. These dividers limit the # of cycles in a period
+ * to SUN20I_PWM_PCNTR_SIZE by applying a scaling factor of
+ * 1/(div_m * (prescale_k + 1)) to the clock source.
+ *
+ * SUN20I_PWM_MAGIC is derived by solving for div_m and prescale_k
+ * such that for a given requested period,
+ *
+ * i) div_m is minimized for any prescale_k ≤ SUN20I_PWM_CTL_PRESCAL_K_MAX,
+ * ii) prescale_k is minimized.
+ *
+ * The derivation proceeds as follows, with val = # of cycles for reqested
+ * period:
+ *
+ * for a given value of div_m we want the smallest prescale_k such that
+ *
+ * (val >> div_m) // (prescale_k + 1) ≤ 65536 (SUN20I_PWM_PCNTR_SIZE)
+ *
+ * This is equivalent to:
+ *
+ * (val >> div_m) ≤ 65536 * (prescale_k + 1) + prescale_k
+ * ⟺ (val >> div_m) ≤ 65537 * prescale_k + 65536
+ * ⟺ (val >> div_m) - 65536 ≤ 65537 * prescale_k
+ * ⟺ ((val >> div_m) - 65536) / 65537 ≤ prescale_k
+ *
+ * As prescale_k is integer, this becomes
+ *
+ * ((val >> div_m) - 65536) // 65537 ≤ prescale_k
+ *
+ * And is minimized at
+ *
+ * ((val >> div_m) - 65536) // 65537
+ *
+ * Now we pick the smallest div_m that satifies prescale_k ≤ 255
+ * (i.e SUN20I_PWM_CTL_PRESCAL_K_MAX),
+ *
+ * ((val >> div_m) - 65536) // 65537 ≤ 255
+ * ⟺ (val >> div_m) - 65536 ≤ 255 * 65537 + 65536
+ * ⟺ val >> div_m ≤ 255 * 65537 + 2 * 65536
+ * ⟺ val >> div_m < (255 * 65537 + 2 * 65536 + 1)
+ * ⟺ div_m = fls((val) / (255 * 65537 + 2 * 65536 + 1))
+ *
+ * Suggested by Uwe Kleine-König
+ */
+#define SUN20I_PWM_MAGIC (255 * 65537 + 2 * 65536 + 1)
+
+struct sun20i_pwm_chip {
+ struct clk *clk_bus, *clk_hosc, *clk_apb0;
+ struct reset_control *rst;
+ struct pwm_chip chip;
+ void __iomem *base;
+ /* Mutex to protect pwm apply state */
+ struct mutex mutex;
+};
+
+static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
+{
+ return container_of(chip, struct sun20i_pwm_chip, chip);
+}
+
+static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
+ unsigned long offset)
+{
+ return readl(chip->base + offset);
+}
+
+static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
+ u32 val, unsigned long offset)
+{
+ writel(val, chip->base + offset);
+}
+
+static int sun20i_pwm_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+ u16 ent_cycle, act_cycle, prescale_k;
+ u64 clk_rate, tmp;
+ u8 div_m;
+ u32 val;
+
+ mutex_lock(&sun20i_chip->mutex);
+
+ val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+ div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, val);
+ if (div_m > SUN20I_PWM_CLK_DIV_M_MAX)
+ div_m = SUN20I_PWM_CLK_DIV_M_MAX;
+
+ if (FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, val) == 0)
+ clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
+ else
+ clk_rate = clk_get_rate(sun20i_chip->clk_apb0);
+
+ val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+ state->polarity = (SUN20I_PWM_CTL_ACT_STA & val) ?
+ PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
+
+ prescale_k = FIELD_GET(SUN20I_PWM_CTL_PRESCAL_K, val) + 1;
+
+ val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+ state->enabled = (SUN20I_PWM_ENABLE_EN(pwm->hwpwm) & val) ? true : false;
+
+ val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_PERIOD(pwm->hwpwm));
+
+ mutex_unlock(&sun20i_chip->mutex);
+
+ act_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ACT_CYCLE, val);
+ ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val);
+
+ /*
+ * The duration of the active phase should not be longer
+ * than the duration of the period
+ */
+ if (act_cycle > ent_cycle)
+ act_cycle = ent_cycle;
+
+ tmp = ((u64)(act_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
+ state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
+ tmp = ((u64)(ent_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
+ state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
+
+ return 0;
+}
+
+static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
+ u64 bus_rate, hosc_rate, val, ent_cycle, act_cycle;
+ u32 clk_gate, clk_cfg, pwm_en, ctl, reg_period;
+ u32 prescale_k, div_m;
+ bool use_bus_clk;
+ int ret = 0;
+
+ mutex_lock(&sun20i_chip->mutex);
+
+ pwm_en = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
+
+ if (state->enabled != pwm->state.enabled) {
+ clk_gate = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_GATE);
+
+ if (!state->enabled) {
+ clk_gate &= ~SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+ pwm_en &= ~SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
+ sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
+ sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+ }
+ }
+
+ if (state->polarity != pwm->state.polarity ||
+ state->duty_cycle != pwm->state.duty_cycle ||
+ state->period != pwm->state.period) {
+ ctl = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
+ clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+ hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
+ bus_rate = clk_get_rate(sun20i_chip->clk_apb0);
+ if (pwm_en & SUN20I_PWM_ENABLE_EN(pwm->hwpwm ^ 1)) {
+ /* if the neighbor channel is enable, check period only */
+ use_bus_clk = FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, clk_cfg) != 0;
+ val = mul_u64_u64_div_u64(state->period,
+ (use_bus_clk ? bus_rate : hosc_rate),
+ NSEC_PER_SEC);
+
+ div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, clk_cfg);
+ } else {
+ /* check period and select clock source */
+ use_bus_clk = false;
+ val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
+ if (val <= 1) {
+ use_bus_clk = true;
+ val = mul_u64_u64_div_u64(state->period, bus_rate, NSEC_PER_SEC);
+ if (val <= 1) {
+ ret = -EINVAL;
+ goto unlock_mutex;
+ }
+ }
+ div_m = fls(DIV_ROUND_DOWN_ULL(val, SUN20I_PWM_MAGIC));
+ if (div_m > SUN20I_PWM_CLK_DIV_M_MAX) {
+ ret = -EINVAL;
+ goto unlock_mutex;
+ }
+
+ /* set up the CLK_DIV_M and clock CLK_SRC */
+ clk_cfg = FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m);
+ clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_SRC, use_bus_clk);
+
+ sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
+ }
+
+ /* calculate prescale_k, PWM entire cycle */
+ ent_cycle = val >> div_m;
+ prescale_k = DIV_ROUND_DOWN_ULL(ent_cycle, 65537);
+ if (prescale_k > SUN20I_PWM_CTL_PRESCAL_K_MAX)
+ prescale_k = SUN20I_PWM_CTL_PRESCAL_K_MAX;
+
+ do_div(ent_cycle, prescale_k + 1);
+
+ /* for N cycles, PPRx.PWM_ENTIRE_CYCLE = (N-1) */
+ reg_period = FIELD_PREP(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, ent_cycle - 1);
+
+ /* set duty cycle */
+ val = mul_u64_u64_div_u64(state->duty_cycle,
+ (use_bus_clk ? bus_rate : hosc_rate),
+ NSEC_PER_SEC);
+ act_cycle = val >> div_m;
+ do_div(act_cycle, prescale_k + 1);
+
+ /*
+ * The formula of the output period and the duty-cycle for PWM are as follows.
+ * T period = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * (PPR0.PWM_ENTIRE_CYCLE + 1)
+ * T high-level = (PWM01_CLK / PWM0_PRESCALE_K)^-1 * PPR0.PWM_ACT_CYCLE
+ * Duty-cycle = T high-level / T period
+ */
+ reg_period |= FIELD_PREP(SUN20I_PWM_PERIOD_ACT_CYCLE, act_cycle);
+ sun20i_pwm_writel(sun20i_chip, reg_period, SUN20I_PWM_PERIOD(pwm->hwpwm));
+
+ ctl = FIELD_PREP(SUN20I_PWM_CTL_PRESCAL_K, prescale_k);
+ if (state->polarity == PWM_POLARITY_NORMAL)
+ ctl |= SUN20I_PWM_CTL_ACT_STA;
+
+ sun20i_pwm_writel(sun20i_chip, ctl, SUN20I_PWM_CTL(pwm->hwpwm));
+ }
+
+ if (state->enabled != pwm->state.enabled && state->enabled) {
+ clk_gate &= ~SUN20I_PWM_CLK_GATE_BYPASS(pwm->hwpwm);
+ clk_gate |= SUN20I_PWM_CLK_GATE_GATING(pwm->hwpwm);
+ pwm_en |= SUN20I_PWM_ENABLE_EN(pwm->hwpwm);
+ sun20i_pwm_writel(sun20i_chip, pwm_en, SUN20I_PWM_ENABLE);
+ sun20i_pwm_writel(sun20i_chip, clk_gate, SUN20I_PWM_CLK_GATE);
+ }
+
+unlock_mutex:
+ mutex_unlock(&sun20i_chip->mutex);
+
+ return ret;
+}
+
+static const struct pwm_ops sun20i_pwm_ops = {
+ .apply = sun20i_pwm_apply,
+ .get_state = sun20i_pwm_get_state,
+};
+
+static const struct of_device_id sun20i_pwm_dt_ids[] = {
+ { .compatible = "allwinner,sun20i-d1-pwm" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, sun20i_pwm_dt_ids);
+
+static int sun20i_pwm_probe(struct platform_device *pdev)
+{
+ struct sun20i_pwm_chip *sun20i_chip;
+ int ret;
+
+ sun20i_chip = devm_kzalloc(&pdev->dev, sizeof(*sun20i_chip), GFP_KERNEL);
+ if (!sun20i_chip)
+ return -ENOMEM;
+
+ sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(sun20i_chip->base))
+ return PTR_ERR(sun20i_chip->base);
+
+ sun20i_chip->clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");
+ if (IS_ERR(sun20i_chip->clk_bus))
+ return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_bus),
+ "failed to get bus clock\n");
+
+ sun20i_chip->clk_hosc = devm_clk_get_enabled(&pdev->dev, "hosc");
+ if (IS_ERR(sun20i_chip->clk_hosc))
+ return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_hosc),
+ "failed to get hosc clock\n");
+
+ sun20i_chip->clk_apb0 = devm_clk_get_enabled(&pdev->dev, "apb0");
+ if (IS_ERR(sun20i_chip->clk_apb0))
+ return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_apb0),
+ "failed to get apb0 clock\n");
+
+ sun20i_chip->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+ if (IS_ERR(sun20i_chip->rst))
+ return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->rst),
+ "failed to get bus reset\n");
+
+ ret = of_property_read_u32(pdev->dev.of_node, "allwinner,pwm-channels",
+ &sun20i_chip->chip.npwm);
+ if (ret)
+ sun20i_chip->chip.npwm = 8;
+
+ if (sun20i_chip->chip.npwm > 16)
+ sun20i_chip->chip.npwm = 16;
+
+ /* Deassert reset */
+ ret = reset_control_deassert(sun20i_chip->rst);
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret, "failed to deassert reset\n");
+
+ sun20i_chip->chip.dev = &pdev->dev;
+ sun20i_chip->chip.ops = &sun20i_pwm_ops;
+
+ mutex_init(&sun20i_chip->mutex);
+
+ ret = pwmchip_add(&sun20i_chip->chip);
+ if (ret < 0) {
+ reset_control_assert(sun20i_chip->rst);
+ return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
+ }
+
+ platform_set_drvdata(pdev, sun20i_chip);
+
+ return 0;
+}
+
+static void sun20i_pwm_remove(struct platform_device *pdev)
+{
+ struct sun20i_pwm_chip *sun20i_chip = platform_get_drvdata(pdev);
+
+ pwmchip_remove(&sun20i_chip->chip);
+
+ reset_control_assert(sun20i_chip->rst);
+}
+
+static struct platform_driver sun20i_pwm_driver = {
+ .driver = {
+ .name = "sun20i-pwm",
+ .of_match_table = sun20i_pwm_dt_ids,
+ },
+ .probe = sun20i_pwm_probe,
+ .remove_new = sun20i_pwm_remove,
+};
+module_platform_driver(sun20i_pwm_driver);
+
+MODULE_AUTHOR("Aleksandr Shubin <[email protected]>");
+MODULE_DESCRIPTION("Allwinner sun20i PWM driver");
+MODULE_LICENSE("GPL");
--
2.25.1
On Mi, 2024-01-31 at 15:59 +0300, Aleksandr Shubin wrote:
> Allwinner's D1, T113-S3 and R329 SoCs have a quite different PWM
> controllers with ones supported by pwm-sun4i driver.
>
> This patch adds a PWM controller driver for Allwinner's D1,
> T113-S3 and R329 SoCs. The main difference between these SoCs
> is the number of channels defined by the DT property.
>
> Co-developed-by: Brandon Cheo Fusi <[email protected]>
> Signed-off-by: Brandon Cheo Fusi <[email protected]>
> Signed-off-by: Aleksandr Shubin <[email protected]>
> ---
> drivers/pwm/Kconfig | 10 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-sun20i.c | 380 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 391 insertions(+)
> create mode 100644 drivers/pwm/pwm-sun20i.c
>
[...]
> diff --git a/drivers/pwm/pwm-sun20i.c b/drivers/pwm/pwm-sun20i.c
> new file mode 100644
> index 000000000000..19bf3f495155
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun20i.c
> @@ -0,0 +1,380 @@
[...]
> +static int sun20i_pwm_probe(struct platform_device *pdev)
> +{
[...]
> + sun20i_chip->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(sun20i_chip->rst))
> + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->rst),
> + "failed to get bus reset\n");
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "allwinner,pwm-channels",
> + &sun20i_chip->chip.npwm);
> + if (ret)
> + sun20i_chip->chip.npwm = 8;
> +
> + if (sun20i_chip->chip.npwm > 16)
> + sun20i_chip->chip.npwm = 16;
> +
> + /* Deassert reset */
> + ret = reset_control_deassert(sun20i_chip->rst);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret, "failed to deassert reset\n");
Consider using devm_add_action_or_reset() to automatically assert the
reset control again on error or driver unbind ...
> +
> + sun20i_chip->chip.dev = &pdev->dev;
> + sun20i_chip->chip.ops = &sun20i_pwm_ops;
> +
> + mutex_init(&sun20i_chip->mutex);
> +
> + ret = pwmchip_add(&sun20i_chip->chip);
.. and devm_pwmchip_add() here. Together, this would allow to drop
sun20i_pwm_remove().
regards
Philipp
hello Aleksandr,
On Wed, Jan 31, 2024 at 03:59:15PM +0300, Aleksandr Shubin wrote:
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
Some time ago there was some effort by Rob Herring to detangle the
headers platform_device.h, of_device.h and of.h. See for example commit
87e51b76c9db8c29cde573af0faf5a3e13e23960. I think you should use
linux/of.h instead of linux/of_device.h.
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +
> +#define SUN20I_PWM_CLK_CFG(chan) (0x20 + (((chan) >> 1) * 0x4))
> +#define SUN20I_PWM_CLK_CFG_SRC GENMASK(8, 7)
> +#define SUN20I_PWM_CLK_CFG_DIV_M GENMASK(3, 0)
> +#define SUN20I_PWM_CLK_DIV_M_MAX 8
SUN20I_PWM_CLK_CFG_DIV_M_MAX?
> +#define SUN20I_PWM_CLK_GATE 0x40
> +#define SUN20I_PWM_CLK_GATE_BYPASS(chan) BIT((chan) + 16)
> +#define SUN20I_PWM_CLK_GATE_GATING(chan) BIT(chan)
> +
> +#define SUN20I_PWM_ENABLE 0x80
> +#define SUN20I_PWM_ENABLE_EN(chan) BIT(chan)
> +
> +#define SUN20I_PWM_CTL(chan) (0x100 + (chan) * 0x20)
> +#define SUN20I_PWM_CTL_ACT_STA BIT(8)
> +#define UN20I_PWM_CTL_PRESCAL_K GENMASK(7, 0)
> +#define SUN20I_PWM_CTL_PRESCAL_K_MAX 0xff
This matches the theoretical maximum for GENMASK(7,0), so you could make
use of field_max(SUN20I_PWM_CTL_PRESCAL_K) here.
> +#define SUN20I_PWM_PERIOD(chan) (0x104 + (chan) * 0x20)
> +#define SUN20I_PWM_PERIOD_ENTIRE_CYCLE GENMASK(31, 16)
> +#define SUN20I_PWM_PERIOD_ACT_CYCLE GENMASK(15, 0)
> +
> +#define SUN20I_PWM_PCNTR_SIZE BIT(16)
> +
> +/**
> + * SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
> + * div_m (SUN20I_PWM_CLK_CFG_DIV_M) & prescale_k (SUN20I_PWM_CTL_PRESCAL_K)
> + * without using a loop. These dividers limit the # of cycles in a period
> + * to SUN20I_PWM_PCNTR_SIZE by applying a scaling factor of
> + * 1/(div_m * (prescale_k + 1)) to the clock source.
> + *
> + * SUN20I_PWM_MAGIC is derived by solving for div_m and prescale_k
> + * such that for a given requested period,
> + *
> + * i) div_m is minimized for any prescale_k ≤ SUN20I_PWM_CTL_PRESCAL_K_MAX,
> + * ii) prescale_k is minimized.
> + *
> + * The derivation proceeds as follows, with val = # of cycles for reqested
s/reqested/requested/
> + * period:
> + *
> + * for a given value of div_m we want the smallest prescale_k such that
> + *
> + * (val >> div_m) // (prescale_k + 1) ≤ 65536 (SUN20I_PWM_PCNTR_SIZE)
> + *
> + * This is equivalent to:
> + *
> + * (val >> div_m) ≤ 65536 * (prescale_k + 1) + prescale_k
> + * ⟺ (val >> div_m) ≤ 65537 * prescale_k + 65536
> + * ⟺ (val >> div_m) - 65536 ≤ 65537 * prescale_k
> + * ⟺ ((val >> div_m) - 65536) / 65537 ≤ prescale_k
> + *
> + * As prescale_k is integer, this becomes
> + *
> + * ((val >> div_m) - 65536) // 65537 ≤ prescale_k
> + *
> + * And is minimized at
> + *
> + * ((val >> div_m) - 65536) // 65537
> + *
> + * Now we pick the smallest div_m that satifies prescale_k ≤ 255
> + * (i.e SUN20I_PWM_CTL_PRESCAL_K_MAX),
> + *
> + * ((val >> div_m) - 65536) // 65537 ≤ 255
> + * ⟺ (val >> div_m) - 65536 ≤ 255 * 65537 + 65536
> + * ⟺ val >> div_m ≤ 255 * 65537 + 2 * 65536
> + * ⟺ val >> div_m < (255 * 65537 + 2 * 65536 + 1)
> + * ⟺ div_m = fls((val) / (255 * 65537 + 2 * 65536 + 1))
> + *
> + * Suggested by Uwe Kleine-König
Good man, I assume this is all sane then :-)
> + */
> +#define SUN20I_PWM_MAGIC (255 * 65537 + 2 * 65536 + 1)
> +
> +struct sun20i_pwm_chip {
> + struct clk *clk_bus, *clk_hosc, *clk_apb0;
> + struct reset_control *rst;
> + struct pwm_chip chip;
> + void __iomem *base;
> + /* Mutex to protect pwm apply state */
> + struct mutex mutex;
> +};
> +
> +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct sun20i_pwm_chip, chip);
> +}
> +
> +static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
> + unsigned long offset)
> +{
> + return readl(chip->base + offset);
> +}
> +
> +static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
> + u32 val, unsigned long offset)
> +{
> + writel(val, chip->base + offset);
> +}
> +
> +static int sun20i_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> + u16 ent_cycle, act_cycle, prescale_k;
> + u64 clk_rate, tmp;
> + u8 div_m;
> + u32 val;
> +
> + mutex_lock(&sun20i_chip->mutex);
> +
> + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
> + div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, val);
> + if (div_m > SUN20I_PWM_CLK_DIV_M_MAX)
> + div_m = SUN20I_PWM_CLK_DIV_M_MAX;
> +
> + if (FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, val) == 0)
> + clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
> + else
> + clk_rate = clk_get_rate(sun20i_chip->clk_apb0);
> +
> + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
> + state->polarity = (SUN20I_PWM_CTL_ACT_STA & val) ?
> + PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> +
> + prescale_k = FIELD_GET(SUN20I_PWM_CTL_PRESCAL_K, val) + 1;
> +
> + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
> + state->enabled = (SUN20I_PWM_ENABLE_EN(pwm->hwpwm) & val) ? true : false;
> +
> + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_PERIOD(pwm->hwpwm));
> +
> + mutex_unlock(&sun20i_chip->mutex);
> +
> + act_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ACT_CYCLE, val);
> + ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val);
> +
> + /*
> + * The duration of the active phase should not be longer
> + * than the duration of the period
> + */
> + if (act_cycle > ent_cycle)
> + act_cycle = ent_cycle;
> +
> + tmp = ((u64)(act_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
> + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> + tmp = ((u64)(ent_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
> + state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
Please add a comment above this block that justifies assuming that the
multiplication doesn't overflow. Something like:
We have act_cycle <= ent_cycle <= 0xffff, prescale_k <= 0x100,
div_m <= 8. So the multiplication fits into an u64 without
overflow.
> +
> + return 0;
> +}
> +
> +static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> +...
> +}
I didn't recheck all the logic in .apply in detail and will assume it is
sane for this round.
> +static const struct pwm_ops sun20i_pwm_ops = {
> + .apply = sun20i_pwm_apply,
> + .get_state = sun20i_pwm_get_state,
> +};
> +
> +static const struct of_device_id sun20i_pwm_dt_ids[] = {
> + { .compatible = "allwinner,sun20i-d1-pwm" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, sun20i_pwm_dt_ids);
> +
> +static int sun20i_pwm_probe(struct platform_device *pdev)
> +{
> + struct sun20i_pwm_chip *sun20i_chip;
> + int ret;
> +
> + sun20i_chip = devm_kzalloc(&pdev->dev, sizeof(*sun20i_chip), GFP_KERNEL);
> + if (!sun20i_chip)
> + return -ENOMEM;
> +
> + sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(sun20i_chip->base))
> + return PTR_ERR(sun20i_chip->base);
> +
> + sun20i_chip->clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");
> + if (IS_ERR(sun20i_chip->clk_bus))
> + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_bus),
> + "failed to get bus clock\n");
> +
> + sun20i_chip->clk_hosc = devm_clk_get_enabled(&pdev->dev, "hosc");
> + if (IS_ERR(sun20i_chip->clk_hosc))
> + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_hosc),
> + "failed to get hosc clock\n");
> +
> + sun20i_chip->clk_apb0 = devm_clk_get_enabled(&pdev->dev, "apb0");
> + if (IS_ERR(sun20i_chip->clk_apb0))
> + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_apb0),
> + "failed to get apb0 clock\n");
> +
> + sun20i_chip->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> + if (IS_ERR(sun20i_chip->rst))
> + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->rst),
> + "failed to get bus reset\n");
> +
> + ret = of_property_read_u32(pdev->dev.of_node, "allwinner,pwm-channels",
> + &sun20i_chip->chip.npwm);
> + if (ret)
> + sun20i_chip->chip.npwm = 8;
> +
> + if (sun20i_chip->chip.npwm > 16)
> + sun20i_chip->chip.npwm = 16;
Is it worth to emit an error message here? Something like:
Limiting number of PWM lines from %u to 16
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Feb 1, 2024 at 9:49 AM Uwe Kleine-König <[email protected]> wrote:
>
> hello Aleksandr,
>
> On Wed, Jan 31, 2024 at 03:59:15PM +0300, Aleksandr Shubin wrote:
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
>
> Some time ago there was some effort by Rob Herring to detangle the
> headers platform_device.h, of_device.h and of.h. See for example commit
> 87e51b76c9db8c29cde573af0faf5a3e13e23960. I think you should use
> linux/of.h instead of linux/of_device.h.
>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/reset.h>
> > +
> > +#define SUN20I_PWM_CLK_CFG(chan) (0x20 + (((chan) >> 1) * 0x4))
> > +#define SUN20I_PWM_CLK_CFG_SRC GENMASK(8, 7)
> > +#define SUN20I_PWM_CLK_CFG_DIV_M GENMASK(3, 0)
> > +#define SUN20I_PWM_CLK_DIV_M_MAX 8
>
> SUN20I_PWM_CLK_CFG_DIV_M_MAX?
>
Yes. The manuals mark [0x9, 0xF] as reserved
> > +#define SUN20I_PWM_CLK_GATE 0x40
> > +#define SUN20I_PWM_CLK_GATE_BYPASS(chan) BIT((chan) + 16)
> > +#define SUN20I_PWM_CLK_GATE_GATING(chan) BIT(chan)
> > +
> > +#define SUN20I_PWM_ENABLE 0x80
> > +#define SUN20I_PWM_ENABLE_EN(chan) BIT(chan)
> > +
> > +#define SUN20I_PWM_CTL(chan) (0x100 + (chan) * 0x20)
> > +#define SUN20I_PWM_CTL_ACT_STA BIT(8)
> > +#define UN20I_PWM_CTL_PRESCAL_K GENMASK(7, 0)
> > +#define SUN20I_PWM_CTL_PRESCAL_K_MAX 0xff
>
> This matches the theoretical maximum for GENMASK(7,0), so you could make
> use of field_max(SUN20I_PWM_CTL_PRESCAL_K) here.
>
> > +#define SUN20I_PWM_PERIOD(chan) (0x104 + (chan) * 0x20)
> > +#define SUN20I_PWM_PERIOD_ENTIRE_CYCLE GENMASK(31, 16)
> > +#define SUN20I_PWM_PERIOD_ACT_CYCLE GENMASK(15, 0)
> > +
> > +#define SUN20I_PWM_PCNTR_SIZE BIT(16)
> > +
> > +/**
> > + * SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
> > + * div_m (SUN20I_PWM_CLK_CFG_DIV_M) & prescale_k (SUN20I_PWM_CTL_PRESCAL_K)
> > + * without using a loop. These dividers limit the # of cycles in a period
> > + * to SUN20I_PWM_PCNTR_SIZE by applying a scaling factor of
> > + * 1/(div_m * (prescale_k + 1)) to the clock source.
> > + *
> > + * SUN20I_PWM_MAGIC is derived by solving for div_m and prescale_k
> > + * such that for a given requested period,
> > + *
> > + * i) div_m is minimized for any prescale_k ≤ SUN20I_PWM_CTL_PRESCAL_K_MAX,
> > + * ii) prescale_k is minimized.
> > + *
> > + * The derivation proceeds as follows, with val = # of cycles for reqested
>
> s/reqested/requested/
Nice catch.
> > + * period:
> > + *
> > + * for a given value of div_m we want the smallest prescale_k such that
> > + *
> > + * (val >> div_m) // (prescale_k + 1) ≤ 65536 (SUN20I_PWM_PCNTR_SIZE)
> > + *
> > + * This is equivalent to:
> > + *
> > + * (val >> div_m) ≤ 65536 * (prescale_k + 1) + prescale_k
> > + * ⟺ (val >> div_m) ≤ 65537 * prescale_k + 65536
> > + * ⟺ (val >> div_m) - 65536 ≤ 65537 * prescale_k
> > + * ⟺ ((val >> div_m) - 65536) / 65537 ≤ prescale_k
> > + *
> > + * As prescale_k is integer, this becomes
> > + *
> > + * ((val >> div_m) - 65536) // 65537 ≤ prescale_k
> > + *
> > + * And is minimized at
> > + *
> > + * ((val >> div_m) - 65536) // 65537
> > + *
> > + * Now we pick the smallest div_m that satifies prescale_k ≤ 255
> > + * (i.e SUN20I_PWM_CTL_PRESCAL_K_MAX),
> > + *
> > + * ((val >> div_m) - 65536) // 65537 ≤ 255
> > + * ⟺ (val >> div_m) - 65536 ≤ 255 * 65537 + 65536
> > + * ⟺ val >> div_m ≤ 255 * 65537 + 2 * 65536
> > + * ⟺ val >> div_m < (255 * 65537 + 2 * 65536 + 1)
> > + * ⟺ div_m = fls((val) / (255 * 65537 + 2 * 65536 + 1))
> > + *
> > + * Suggested by Uwe Kleine-König
>
> Good man, I assume this is all sane then :-)
Credit should be given where it is due :-)
> > + */
> > +#define SUN20I_PWM_MAGIC (255 * 65537 + 2 * 65536 + 1)
> > +
> > +struct sun20i_pwm_chip {
> > + struct clk *clk_bus, *clk_hosc, *clk_apb0;
> > + struct reset_control *rst;
> > + struct pwm_chip chip;
> > + void __iomem *base;
> > + /* Mutex to protect pwm apply state */
> > + struct mutex mutex;
> > +};
> > +
> > +static inline struct sun20i_pwm_chip *to_sun20i_pwm_chip(struct pwm_chip *chip)
> > +{
> > + return container_of(chip, struct sun20i_pwm_chip, chip);
> > +}
> > +
> > +static inline u32 sun20i_pwm_readl(struct sun20i_pwm_chip *chip,
> > + unsigned long offset)
> > +{
> > + return readl(chip->base + offset);
> > +}
> > +
> > +static inline void sun20i_pwm_writel(struct sun20i_pwm_chip *chip,
> > + u32 val, unsigned long offset)
> > +{
> > + writel(val, chip->base + offset);
> > +}
> > +
> > +static int sun20i_pwm_get_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct sun20i_pwm_chip *sun20i_chip = to_sun20i_pwm_chip(chip);
> > + u16 ent_cycle, act_cycle, prescale_k;
> > + u64 clk_rate, tmp;
> > + u8 div_m;
> > + u32 val;
> > +
> > + mutex_lock(&sun20i_chip->mutex);
> > +
> > + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
> > + div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, val);
> > + if (div_m > SUN20I_PWM_CLK_DIV_M_MAX)
> > + div_m = SUN20I_PWM_CLK_DIV_M_MAX;
> > +
> > + if (FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, val) == 0)
> > + clk_rate = clk_get_rate(sun20i_chip->clk_hosc);
> > + else
> > + clk_rate = clk_get_rate(sun20i_chip->clk_apb0);
> > +
> > + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
> > + state->polarity = (SUN20I_PWM_CTL_ACT_STA & val) ?
> > + PWM_POLARITY_NORMAL : PWM_POLARITY_INVERSED;
> > +
> > + prescale_k = FIELD_GET(SUN20I_PWM_CTL_PRESCAL_K, val) + 1;
> > +
> > + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_ENABLE);
> > + state->enabled = (SUN20I_PWM_ENABLE_EN(pwm->hwpwm) & val) ? true : false;
> > +
> > + val = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_PERIOD(pwm->hwpwm));
> > +
> > + mutex_unlock(&sun20i_chip->mutex);
> > +
> > + act_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ACT_CYCLE, val);
> > + ent_cycle = FIELD_GET(SUN20I_PWM_PERIOD_ENTIRE_CYCLE, val);
> > +
> > + /*
> > + * The duration of the active phase should not be longer
> > + * than the duration of the period
> > + */
> > + if (act_cycle > ent_cycle)
> > + act_cycle = ent_cycle;
> > +
> > + tmp = ((u64)(act_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
> > + state->duty_cycle = DIV_ROUND_UP_ULL(tmp, clk_rate);
> > + tmp = ((u64)(ent_cycle) * prescale_k << div_m) * NSEC_PER_SEC;
> > + state->period = DIV_ROUND_UP_ULL(tmp, clk_rate);
>
> Please add a comment above this block that justifies assuming that the
> multiplication doesn't overflow. Something like:
>
> We have act_cycle <= ent_cycle <= 0xffff, prescale_k <= 0x100,
> div_m <= 8. So the multiplication fits into an u64 without
> overflow.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int sun20i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > +...
> > +}
>
> I didn't recheck all the logic in .apply in detail and will assume it is
> sane for this round.
Please do recheck. This thing is already on v8 and we want to make sure
everyone is happy with v9.
> > +static const struct pwm_ops sun20i_pwm_ops = {
> > + .apply = sun20i_pwm_apply,
> > + .get_state = sun20i_pwm_get_state,
> > +};
> > +
> > +static const struct of_device_id sun20i_pwm_dt_ids[] = {
> > + { .compatible = "allwinner,sun20i-d1-pwm" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, sun20i_pwm_dt_ids);
> > +
> > +static int sun20i_pwm_probe(struct platform_device *pdev)
> > +{
> > + struct sun20i_pwm_chip *sun20i_chip;
> > + int ret;
> > +
> > + sun20i_chip = devm_kzalloc(&pdev->dev, sizeof(*sun20i_chip), GFP_KERNEL);
> > + if (!sun20i_chip)
> > + return -ENOMEM;
> > +
> > + sun20i_chip->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(sun20i_chip->base))
> > + return PTR_ERR(sun20i_chip->base);
> > +
> > + sun20i_chip->clk_bus = devm_clk_get_enabled(&pdev->dev, "bus");
> > + if (IS_ERR(sun20i_chip->clk_bus))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_bus),
> > + "failed to get bus clock\n");
> > +
> > + sun20i_chip->clk_hosc = devm_clk_get_enabled(&pdev->dev, "hosc");
> > + if (IS_ERR(sun20i_chip->clk_hosc))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_hosc),
> > + "failed to get hosc clock\n");
> > +
> > + sun20i_chip->clk_apb0 = devm_clk_get_enabled(&pdev->dev, "apb0");
> > + if (IS_ERR(sun20i_chip->clk_apb0))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->clk_apb0),
> > + "failed to get apb0 clock\n");
> > +
> > + sun20i_chip->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> > + if (IS_ERR(sun20i_chip->rst))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(sun20i_chip->rst),
> > + "failed to get bus reset\n");
> > +
> > + ret = of_property_read_u32(pdev->dev.of_node, "allwinner,pwm-channels",
> > + &sun20i_chip->chip.npwm);
> > + if (ret)
> > + sun20i_chip->chip.npwm = 8;
> > +
> > + if (sun20i_chip->chip.npwm > 16)
> > + sun20i_chip->chip.npwm = 16;
>
> Is it worth to emit an error message here? Something like:
>
> Limiting number of PWM lines from %u to 16
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Brandon.
Hi Aleksandr,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on sunxi/sunxi/for-next linus/master v6.8-rc2 next-20240202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Aleksandr-Shubin/dt-bindings-pwm-Add-binding-for-Allwinner-D1-T113-S3-R329-PWM-controller/20240131-210313
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240131125920.2879433-3-privatesub2%40gmail.com
patch subject: [PATCH v8 2/3] pwm: Add Allwinner's D1/T113-S3/R329 SoCs PWM support
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240203/[email protected]/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240203/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/pwm/pwm-sun20i.c:47: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
vim +47 drivers/pwm/pwm-sun20i.c
45
46 /**
> 47 * SUN20I_PWM_MAGIC is used to quickly compute the values of the clock dividers
48 * div_m (SUN20I_PWM_CLK_CFG_DIV_M) & prescale_k (SUN20I_PWM_CTL_PRESCAL_K)
49 * without using a loop. These dividers limit the # of cycles in a period
50 * to SUN20I_PWM_PCNTR_SIZE by applying a scaling factor of
51 * 1/(div_m * (prescale_k + 1)) to the clock source.
52 *
53 * SUN20I_PWM_MAGIC is derived by solving for div_m and prescale_k
54 * such that for a given requested period,
55 *
56 * i) div_m is minimized for any prescale_k ≤ SUN20I_PWM_CTL_PRESCAL_K_MAX,
57 * ii) prescale_k is minimized.
58 *
59 * The derivation proceeds as follows, with val = # of cycles for reqested
60 * period:
61 *
62 * for a given value of div_m we want the smallest prescale_k such that
63 *
64 * (val >> div_m) // (prescale_k + 1) ≤ 65536 (SUN20I_PWM_PCNTR_SIZE)
65 *
66 * This is equivalent to:
67 *
68 * (val >> div_m) ≤ 65536 * (prescale_k + 1) + prescale_k
69 * ⟺ (val >> div_m) ≤ 65537 * prescale_k + 65536
70 * ⟺ (val >> div_m) - 65536 ≤ 65537 * prescale_k
71 * ⟺ ((val >> div_m) - 65536) / 65537 ≤ prescale_k
72 *
73 * As prescale_k is integer, this becomes
74 *
75 * ((val >> div_m) - 65536) // 65537 ≤ prescale_k
76 *
77 * And is minimized at
78 *
79 * ((val >> div_m) - 65536) // 65537
80 *
81 * Now we pick the smallest div_m that satifies prescale_k ≤ 255
82 * (i.e SUN20I_PWM_CTL_PRESCAL_K_MAX),
83 *
84 * ((val >> div_m) - 65536) // 65537 ≤ 255
85 * ⟺ (val >> div_m) - 65536 ≤ 255 * 65537 + 65536
86 * ⟺ val >> div_m ≤ 255 * 65537 + 2 * 65536
87 * ⟺ val >> div_m < (255 * 65537 + 2 * 65536 + 1)
88 * ⟺ div_m = fls((val) / (255 * 65537 + 2 * 65536 + 1))
89 *
90 * Suggested by Uwe Kleine-König
91 */
92 #define SUN20I_PWM_MAGIC (255 * 65537 + 2 * 65536 + 1)
93
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi,
On Wed, Jan 31, 2024 at 03:59:15PM +0300, Aleksandr Shubin wrote:
> + if (state->polarity != pwm->state.polarity ||
> + state->duty_cycle != pwm->state.duty_cycle ||
> + state->period != pwm->state.period) {
> + ctl = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CTL(pwm->hwpwm));
> + clk_cfg = sun20i_pwm_readl(sun20i_chip, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
> + hosc_rate = clk_get_rate(sun20i_chip->clk_hosc);
> + bus_rate = clk_get_rate(sun20i_chip->clk_apb0);
> + if (pwm_en & SUN20I_PWM_ENABLE_EN(pwm->hwpwm ^ 1)) {
> + /* if the neighbor channel is enable, check period only */
> + use_bus_clk = FIELD_GET(SUN20I_PWM_CLK_CFG_SRC, clk_cfg) != 0;
> + val = mul_u64_u64_div_u64(state->period,
> + (use_bus_clk ? bus_rate : hosc_rate),
> + NSEC_PER_SEC);
> +
> + div_m = FIELD_GET(SUN20I_PWM_CLK_CFG_DIV_M, clk_cfg);
> + } else {
> + /* check period and select clock source */
> + use_bus_clk = false;
> + val = mul_u64_u64_div_u64(state->period, hosc_rate, NSEC_PER_SEC);
> + if (val <= 1) {
> + use_bus_clk = true;
> + val = mul_u64_u64_div_u64(state->period, bus_rate, NSEC_PER_SEC);
> + if (val <= 1) {
> + ret = -EINVAL;
> + goto unlock_mutex;
> + }
> + }
> + div_m = fls(DIV_ROUND_DOWN_ULL(val, SUN20I_PWM_MAGIC));
> + if (div_m > SUN20I_PWM_CLK_DIV_M_MAX) {
> + ret = -EINVAL;
> + goto unlock_mutex;
> + }
> +
> + /* set up the CLK_DIV_M and clock CLK_SRC */
> + clk_cfg = FIELD_PREP(SUN20I_PWM_CLK_CFG_DIV_M, div_m);
> + clk_cfg |= FIELD_PREP(SUN20I_PWM_CLK_CFG_SRC, use_bus_clk);
> +
> + sun20i_pwm_writel(sun20i_chip, clk_cfg, SUN20I_PWM_CLK_CFG(pwm->hwpwm));
> + }
If I'm reading this correctly, for each PWM pair you set the clock once.
Wouldn't this mean that the order of setting PWMs would affect the accuracy?
It would be good to note this down perhaps?
John.
> --
> 2.25.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel