Adds initial TTC PWM driver. New driver would use same compatible string
as that of drivers/clocksource/timer-cadence-ttc.c.
Specific TTC device would be identified as clocksource/clockeven or PWM
device based on the pwm-cells poperty. Clocksource TTC driver checks
for pwm-cells property, if found, it just returns without acting on it.
Mubin Sayyed (3):
clocksource: timer-cadence-ttc: Do not probe TTC device configured as
PWM
dt-bindings: timer: Update device tree bindings for cadence TTC PWM
pwm: pwm-cadence: Add support for TTC PWM
.../devicetree/bindings/timer/cdns,ttc.yaml | 25 +-
drivers/clocksource/timer-cadence-ttc.c | 3 +
drivers/pwm/Kconfig | 11 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-cadence.c | 363 ++++++++++++++++++
5 files changed, 402 insertions(+), 1 deletion(-)
create mode 100644 drivers/pwm/pwm-cadence.c
--
2.25.1
TTC device can act either as clocksource/clockevent or
PWM generator, it would be decided by pwm-cells property.
TTC PWM feature would be supported through separate driver
based on PWM framework.
If pwm-cells property is present in TTC node, it would be
treated as PWM device, and clocksource driver should just
skip it.
Signed-off-by: Mubin Sayyed <[email protected]>
---
drivers/clocksource/timer-cadence-ttc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 4efd0cf3b602..ba46649148b1 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -476,6 +476,9 @@ static int __init ttc_timer_probe(struct platform_device *pdev)
u32 timer_width = 16;
struct device_node *timer = pdev->dev.of_node;
+ if (of_property_read_bool(timer, "#pwm-cells"))
+ return -ENODEV;
+
if (initialized)
return 0;
--
2.25.1
Cadence TTC can act as PWM device, it is supported through
separate PWM framework based driver. Decision to configure
specific TTC device as PWM or clocksource/clockevent would
be done based on presence of "#pwm-cells" property.
Also, interrupt property is not required for TTC PWM driver.
Updated bindings to support TTC PWM configuration.
Signed-off-by: Mubin Sayyed <[email protected]>
---
.../devicetree/bindings/timer/cdns,ttc.yaml | 25 ++++++++++++++++++-
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/timer/cdns,ttc.yaml b/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
index 7d821fd480f6..2855e92e02e3 100644
--- a/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
+++ b/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
@@ -32,12 +32,26 @@ properties:
description: |
Bit width of the timer, necessary if not 16.
+ "#pwm-cells":
+ description: |
+ Required to configure TTC as PWM device, supported cells are 0 to 3.
+ minimum: 0
+ maximum: 3
+
required:
- compatible
- reg
- - interrupts
- clocks
+allOf:
+ - if:
+ not:
+ required:
+ - "#pwm-cells"
+ then:
+ required:
+ - interrupts
+
additionalProperties: false
examples:
@@ -50,3 +64,12 @@ examples:
clocks = <&cpu_clk 3>;
timer-width = <32>;
};
+
+ - |
+ ttc1: ttc1@f8002000 {
+ compatible = "cdns,ttc";
+ reg = <0xF8002000 0x1000>;
+ clocks = <&cpu_clk 3>;
+ timer-width = <32>;
+ #pwm-cells = <3>;
+ };
--
2.25.1
Cadence TTC timer can be configured as clocksource/clockevent
or PWM device. Specific TTC device would be configured as PWM
device, if pwm-cells property is present in the device tree
node.
In case of Zynq, ZynqMP and Versal SoC's, each TTC device has 3
timers/counters, so maximum 3 PWM channels can be configured for
each TTC IP instance. Also, output of 0th PWM channel of each TTC
device can be routed to MIO or EMIO, and output of 2nd and 3rd
PWM channel can be routed only to EMIO.
Period for given PWM channel is configured through interval timer
and duty cycle through match counter.
Signed-off-by: Mubin Sayyed <[email protected]>
---
drivers/pwm/Kconfig | 11 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-cadence.c | 363 ++++++++++++++++++++++++++++++++++++++
3 files changed, 375 insertions(+)
create mode 100644 drivers/pwm/pwm-cadence.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index dae023d783a2..9e0f1fae4711 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -504,6 +504,17 @@ config PWM_SIFIVE
To compile this driver as a module, choose M here: the module
will be called pwm-sifive.
+config PWM_CADENCE
+ tristate "Cadence PWM support"
+ depends on OF
+ depends on COMMON_CLK
+ help
+ Generic PWM framework driver for cadence TTC IP found on
+ Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
+ channels. Output of 0th PWM channel of each TTC device can
+ be routed to MIO or EMIO, and output of 1st and 2nd PWM
+ channels can be routed only to EMIO.
+
config PWM_SL28CPLD
tristate "Kontron sl28cpld PWM support"
depends on MFD_SL28CPLD || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 7bf1a29f02b8..f744f021be0f 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o
obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o
obj-$(CONFIG_PWM_SAMSUNG) += pwm-samsung.o
obj-$(CONFIG_PWM_SIFIVE) += pwm-sifive.o
+obj-$(CONFIG_PWM_CADENCE) += pwm-cadence.o
obj-$(CONFIG_PWM_SL28CPLD) += pwm-sl28cpld.o
obj-$(CONFIG_PWM_SPEAR) += pwm-spear.o
obj-$(CONFIG_PWM_SPRD) += pwm-sprd.o
diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
new file mode 100644
index 000000000000..de981df3ec5a
--- /dev/null
+++ b/drivers/pwm/pwm-cadence.c
@@ -0,0 +1,363 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver to configure cadence TTC timer as PWM
+ * generator
+ *
+ * Copyright (C) 2022, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/of_address.h>
+
+#define TTC_CLK_CNTRL_OFFSET 0x00
+#define TTC_CNT_CNTRL_OFFSET 0x0C
+#define TTC_MATCH_CNT_VAL_OFFSET 0x30
+#define TTC_COUNT_VAL_OFFSET 0x18
+#define TTC_INTR_VAL_OFFSET 0x24
+#define TTC_ISR_OFFSET 0x54
+#define TTC_IER_OFFSET 0x60
+#define TTC_PWM_CHANNEL_OFFSET 0x4
+
+#define TTC_CLK_CNTRL_CSRC_MASK BIT(5)
+#define TTC_CLK_CNTRL_PSV_MASK GENMASK(4, 1)
+
+#define TTC_CNTR_CTRL_DIS_MASK BIT(0)
+#define TTC_CNTR_CTRL_INTR_MODE_EN_MASK BIT(1)
+#define TTC_CNTR_CTRL_MATCH_MODE_EN_MASK BIT(3)
+#define TTC_CNTR_CTRL_RST_MASK BIT(4)
+#define TTC_CNTR_CTRL_WAVE_EN_MASK BIT(5)
+#define TTC_CNTR_CTRL_WAVE_POL_MASK BIT(6)
+
+#define TTC_CLK_CNTRL_PSV_SHIFT 1
+
+#define TTC_PWM_MAX_CH 3
+
+/**
+ * struct ttc_pwm_priv - Private data for TTC PWM drivers
+ * @chip: PWM chip structure representing PWM controller
+ * @clk: TTC input clock
+ * @max: Maximum value of the counters
+ * @base: Base address of TTC instance
+ */
+struct ttc_pwm_priv {
+ struct pwm_chip chip;
+ struct clk *clk;
+ u32 max;
+ void __iomem *base;
+};
+
+static inline u32 ttc_pwm_readl(struct ttc_pwm_priv *priv,
+ unsigned long offset)
+{
+ return readl_relaxed(priv->base + offset);
+}
+
+static inline void ttc_pwm_writel(struct ttc_pwm_priv *priv,
+ unsigned long offset,
+ unsigned long val)
+{
+ writel_relaxed(val, priv->base + offset);
+}
+
+static inline u32 ttc_pwm_ch_readl(struct ttc_pwm_priv *priv,
+ struct pwm_device *pwm,
+ unsigned long offset)
+{
+ unsigned long pwm_ch_offset = offset +
+ (TTC_PWM_CHANNEL_OFFSET * pwm->hwpwm);
+
+ return ttc_pwm_readl(priv, pwm_ch_offset);
+}
+
+static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
+ struct pwm_device *pwm,
+ unsigned long offset,
+ unsigned long val)
+{
+ unsigned long pwm_ch_offset = offset +
+ (TTC_PWM_CHANNEL_OFFSET * pwm->hwpwm);
+
+ ttc_pwm_writel(priv, pwm_ch_offset, val);
+}
+
+static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
+{
+ return container_of(chip, struct ttc_pwm_priv, chip);
+}
+
+static void ttc_pwm_enable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
+{
+ u32 ctrl_reg;
+
+ ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
+ ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN_MASK
+ | TTC_CNTR_CTRL_MATCH_MODE_EN_MASK | TTC_CNTR_CTRL_RST_MASK);
+ ctrl_reg &= (~(TTC_CNTR_CTRL_DIS_MASK | TTC_CNTR_CTRL_WAVE_EN_MASK));
+ ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
+}
+
+static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
+{
+ u32 ctrl_reg;
+
+ ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
+ ctrl_reg |= TTC_CNTR_CTRL_DIS_MASK;
+
+ ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
+}
+
+static void ttc_pwm_rev_polarity(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
+{
+ u32 ctrl_reg;
+
+ ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
+ ctrl_reg ^= TTC_CNTR_CTRL_WAVE_POL_MASK;
+ ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
+}
+
+static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
+ struct pwm_device *pwm,
+ u32 div,
+ u32 period_cycles,
+ u32 duty_cycles)
+{
+ u32 clk_reg;
+
+ /* Set up prescalar */
+ clk_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CLK_CNTRL_OFFSET);
+ clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
+ clk_reg |= div;
+ ttc_pwm_ch_writel(priv, pwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
+
+ /* Set up period */
+ ttc_pwm_ch_writel(priv, pwm, TTC_INTR_VAL_OFFSET, period_cycles);
+
+ /* Set up duty cycle */
+ ttc_pwm_ch_writel(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET, duty_cycles);
+}
+
+static int ttc_pwm_apply(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ u32 div = 0;
+ u64 period_cycles;
+ u64 duty_cycles;
+ unsigned long rate;
+ struct pwm_state cstate;
+ struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
+
+ pwm_get_state(pwm, &cstate);
+
+ if (state->polarity != cstate.polarity) {
+ if (cstate.enabled)
+ ttc_pwm_disable(priv, pwm);
+
+ ttc_pwm_rev_polarity(priv, pwm);
+
+ if (cstate.enabled)
+ ttc_pwm_enable(priv, pwm);
+ }
+
+ if (state->period != cstate.period ||
+ state->duty_cycle != cstate.duty_cycle) {
+ rate = clk_get_rate(priv->clk);
+
+ /* Prevent overflow by limiting to the maximum possible period */
+ period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
+ period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate, NSEC_PER_SEC);
+
+ if (period_cycles > priv->max) {
+ /* prescale frequency to fit requested period cycles within limit */
+ div = 1;
+
+ while ((period_cycles > priv->max) && (div < 65536)) {
+ rate = DIV_ROUND_CLOSEST(rate, BIT(div));
+ period_cycles = DIV_ROUND_CLOSEST(state->period * rate,
+ NSEC_PER_SEC);
+ if (period_cycles < priv->max)
+ break;
+ div++;
+ }
+
+ if (period_cycles > priv->max)
+ return -ERANGE;
+ }
+
+ duty_cycles = DIV_ROUND_CLOSEST(state->duty_cycle * rate,
+ NSEC_PER_SEC);
+ if (cstate.enabled)
+ ttc_pwm_disable(priv, pwm);
+
+ ttc_pwm_set_counters(priv, pwm, div, period_cycles, duty_cycles);
+
+ if (cstate.enabled)
+ ttc_pwm_enable(priv, pwm);
+ }
+
+ if (state->enabled != cstate.enabled) {
+ if (state->enabled)
+ ttc_pwm_enable(priv, pwm);
+ else
+ ttc_pwm_disable(priv, pwm);
+ }
+
+ return 0;
+}
+
+static int ttc_pwm_get_state(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
+ unsigned long rate;
+ u32 value;
+ u64 tmp;
+
+ value = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
+
+ if (value & TTC_CNTR_CTRL_WAVE_POL_MASK)
+ state->polarity = PWM_POLARITY_INVERSED;
+ else
+ state->polarity = PWM_POLARITY_NORMAL;
+
+ if (value & TTC_CNTR_CTRL_DIS_MASK)
+ state->enabled = false;
+ else
+ state->enabled = true;
+
+ rate = clk_get_rate(priv->clk);
+
+ tmp = ttc_pwm_ch_readl(priv, pwm, TTC_INTR_VAL_OFFSET);
+ state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+
+ tmp = ttc_pwm_ch_readl(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET);
+ state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
+
+ return 0;
+}
+
+static struct pwm_device *
+ttc_pwm_of_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
+{
+ struct pwm_device *pwm;
+
+ if (args->args[0] >= TTC_PWM_MAX_CH)
+ return NULL;
+
+ pwm = pwm_request_from_chip(chip, args->args[0], NULL);
+ if (IS_ERR(pwm))
+ return pwm;
+
+ if (args->args[1])
+ pwm->args.period = args->args[1];
+
+ if (args->args[2])
+ pwm->args.polarity = args->args[2];
+
+ return pwm;
+}
+
+static const struct pwm_ops ttc_pwm_ops = {
+ .apply = ttc_pwm_apply,
+ .get_state = ttc_pwm_get_state,
+ .owner = THIS_MODULE,
+};
+
+static int ttc_pwm_probe(struct platform_device *pdev)
+{
+ int ret;
+ int clksel;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct ttc_pwm_priv *priv;
+ u32 pwm_cells;
+ u32 timer_width;
+ struct clk *clk_cs;
+
+ ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
+ if (ret == -EINVAL)
+ return -ENODEV;
+
+ if (ret)
+ return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(priv->base))
+ return PTR_ERR(priv->base);
+
+ ret = of_property_read_u32(np, "timer-width", &timer_width);
+ if (ret)
+ timer_width = 16;
+
+ priv->max = BIT(timer_width) - 1;
+
+ clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
+ clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
+ clk_cs = of_clk_get(np, clksel);
+ if (IS_ERR(clk_cs))
+ return dev_err_probe(dev, PTR_ERR(clk_cs),
+ "ERROR: timer input clock not found\n");
+
+ priv->clk = clk_cs;
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return dev_err_probe(dev, ret, "Clock enable failed\n");
+
+ clk_rate_exclusive_get(priv->clk);
+
+ priv->chip.dev = dev;
+ priv->chip.ops = &ttc_pwm_ops;
+ priv->chip.npwm = TTC_PWM_MAX_CH;
+ priv->chip.of_xlate = ttc_pwm_of_xlate;
+ ret = pwmchip_add(&priv->chip);
+ if (ret) {
+ clk_rate_exclusive_put(priv->clk);
+ clk_disable_unprepare(priv->clk);
+ return dev_err_probe(dev, ret, "Could not register PWM chip\n");
+ }
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static int ttc_pwm_remove(struct platform_device *pdev)
+{
+ struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
+
+ pwmchip_remove(&priv->chip);
+ clk_rate_exclusive_put(priv->clk);
+ clk_disable_unprepare(priv->clk);
+
+ return 0;
+}
+
+static const struct of_device_id ttc_pwm_of_match[] = {
+ { .compatible = "cdns,ttc"},
+ {},
+};
+MODULE_DEVICE_TABLE(of, ttc_pwm_of_match);
+
+static struct platform_driver ttc_pwm_driver = {
+ .probe = ttc_pwm_probe,
+ .remove = ttc_pwm_remove,
+ .driver = {
+ .name = "ttc-pwm",
+ .of_match_table = of_match_ptr(ttc_pwm_of_match),
+ },
+};
+module_platform_driver(ttc_pwm_driver);
+
+MODULE_AUTHOR("Mubin Sayyed <[email protected]>");
+MODULE_DESCRIPTION("Cadence TTC PWM driver");
+MODULE_LICENSE("GPL");
--
2.25.1
On 12/01/2023 08:15, Mubin Sayyed wrote:
> Cadence TTC can act as PWM device, it is supported through
Subject: drop second/last, redundant "device tree bindings". The
"dt-bindings" prefix is already stating that these are bindings.
Anyway subject is poor - every commit is an "update", so basically you
said there nothing...
> separate PWM framework based driver. Decision to configure
> specific TTC device as PWM or clocksource/clockevent would
> be done based on presence of "#pwm-cells" property.
>
> Also, interrupt property is not required for TTC PWM driver.
> Updated bindings to support TTC PWM configuration.
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.
>
> Signed-off-by: Mubin Sayyed <[email protected]>
> ---
> .../devicetree/bindings/timer/cdns,ttc.yaml | 25 ++++++++++++++++++-
> 1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/timer/cdns,ttc.yaml b/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
> index 7d821fd480f6..2855e92e02e3 100644
> --- a/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
> +++ b/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
> @@ -32,12 +32,26 @@ properties:
> description: |
> Bit width of the timer, necessary if not 16.
>
> + "#pwm-cells":
> + description: |
> + Required to configure TTC as PWM device, supported cells are 0 to 3.
> + minimum: 0
> + maximum: 3
Better make it const. What's the benefit of flexible cells? You also
should describe the arguments.
> +
> required:
> - compatible
> - reg
> - - interrupts
> - clocks
>
> +allOf:
> + - if:
> + not:
> + required:
> + - "#pwm-cells"
> + then:
> + required:
> + - interrupts
> +
> additionalProperties: false
>
> examples:
> @@ -50,3 +64,12 @@ examples:
> clocks = <&cpu_clk 3>;
> timer-width = <32>;
> };
> +
> + - |
> + ttc1: ttc1@f8002000 {
Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "cdns,ttc";
> + reg = <0xF8002000 0x1000>;
lowercase hex
> + clocks = <&cpu_clk 3>;
> + timer-width = <32>;
> + #pwm-cells = <3>;
> + };
Best regards,
Krzysztof
Hi Mubin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on robh/for-next tip/timers/core linus/master v6.2-rc3 next-20230112]
[cannot apply to xilinx-xlnx/master]
[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/Mubin-Sayyed/clocksource-timer-cadence-ttc-Do-not-probe-TTC-device-configured-as-PWM/20230112-151710
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
patch link: https://lore.kernel.org/r/20230112071526.3035949-4-mubin.sayyed%40amd.com
patch subject: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC PWM
config: m68k-allyesconfig
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c7f230f98c7cac02cea4ecd4237c1707344b99ef
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Mubin-Sayyed/clocksource-timer-cadence-ttc-Do-not-probe-TTC-device-configured-as-PWM/20230112-151710
git checkout c7f230f98c7cac02cea4ecd4237c1707344b99ef
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
m68k-linux-ld: drivers/pwm/pwm-cadence.o: in function `ttc_pwm_apply':
>> pwm-cadence.c:(.text+0x4e8): undefined reference to `__udivdi3'
>> m68k-linux-ld: pwm-cadence.c:(.text+0x53c): undefined reference to `__udivdi3'
m68k-linux-ld: pwm-cadence.c:(.text+0x592): undefined reference to `__udivdi3'
m68k-linux-ld: pwm-cadence.c:(.text+0x5fa): undefined reference to `__udivdi3'
`.exit.text' referenced in section `.data' of sound/soc/codecs/tlv320adc3xxx.o: defined in discarded section `.exit.text' of sound/soc/codecs/tlv320adc3xxx.o
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hello,
On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> Cadence TTC timer can be configured as clocksource/clockevent
> or PWM device. Specific TTC device would be configured as PWM
> device, if pwm-cells property is present in the device tree
> node.
>
> In case of Zynq, ZynqMP and Versal SoC's, each TTC device has 3
> timers/counters, so maximum 3 PWM channels can be configured for
> each TTC IP instance. Also, output of 0th PWM channel of each TTC
> device can be routed to MIO or EMIO, and output of 2nd and 3rd
> PWM channel can be routed only to EMIO.
>
> Period for given PWM channel is configured through interval timer
> and duty cycle through match counter.
>
> Signed-off-by: Mubin Sayyed <[email protected]>
> ---
> drivers/pwm/Kconfig | 11 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-cadence.c | 363 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 375 insertions(+)
> create mode 100644 drivers/pwm/pwm-cadence.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index dae023d783a2..9e0f1fae4711 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -504,6 +504,17 @@ config PWM_SIFIVE
> To compile this driver as a module, choose M here: the module
> will be called pwm-sifive.
>
> +config PWM_CADENCE
Please keep both Kconfig and Makefile sorted alphabetically.
> + tristate "Cadence PWM support"
> + depends on OF
> + depends on COMMON_CLK
> + help
> + Generic PWM framework driver for cadence TTC IP found on
> + Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
> + channels. Output of 0th PWM channel of each TTC device can
> + be routed to MIO or EMIO, and output of 1st and 2nd PWM
> + channels can be routed only to EMIO.
> +
> config PWM_SL28CPLD
> tristate "Kontron sl28cpld PWM support"
> depends on MFD_SL28CPLD || COMPILE_TEST
> [...]
> diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
> new file mode 100644
> index 000000000000..de981df3ec5a
> --- /dev/null
> +++ b/drivers/pwm/pwm-cadence.c
> @@ -0,0 +1,363 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver to configure cadence TTC timer as PWM
> + * generator
> + *
> + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> + */
Is there a public manual for the hardware? If yes, please mention a link
here.
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
clk-provider looks wronge here, your device is only a consumer, isn't
it?
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/of_address.h>
> +
> +#define TTC_CLK_CNTRL_OFFSET 0x00
> +#define TTC_CNT_CNTRL_OFFSET 0x0C
> +#define TTC_MATCH_CNT_VAL_OFFSET 0x30
> +#define TTC_COUNT_VAL_OFFSET 0x18
> +#define TTC_INTR_VAL_OFFSET 0x24
> +#define TTC_ISR_OFFSET 0x54
> +#define TTC_IER_OFFSET 0x60
> +#define TTC_PWM_CHANNEL_OFFSET 0x4
> +
> +#define TTC_CLK_CNTRL_CSRC_MASK BIT(5)
> +#define TTC_CLK_CNTRL_PSV_MASK GENMASK(4, 1)
> +
> +#define TTC_CNTR_CTRL_DIS_MASK BIT(0)
> +#define TTC_CNTR_CTRL_INTR_MODE_EN_MASK BIT(1)
> +#define TTC_CNTR_CTRL_MATCH_MODE_EN_MASK BIT(3)
> +#define TTC_CNTR_CTRL_RST_MASK BIT(4)
> +#define TTC_CNTR_CTRL_WAVE_EN_MASK BIT(5)
> +#define TTC_CNTR_CTRL_WAVE_POL_MASK BIT(6)
If you ask me do s/_OFFSET// and s/_MASK// on all these definitions.
> +#define TTC_CLK_CNTRL_PSV_SHIFT 1
This is unused.
> +
> +#define TTC_PWM_MAX_CH 3
> +
> +/**
> + * struct ttc_pwm_priv - Private data for TTC PWM drivers
> + * @chip: PWM chip structure representing PWM controller
> + * @clk: TTC input clock
> + * @max: Maximum value of the counters
> + * @base: Base address of TTC instance
> + */
> +struct ttc_pwm_priv {
> + struct pwm_chip chip;
> + struct clk *clk;
> + u32 max;
> + void __iomem *base;
> +};
> +
> +static inline u32 ttc_pwm_readl(struct ttc_pwm_priv *priv,
> + unsigned long offset)
> +{
> + return readl_relaxed(priv->base + offset);
> +}
> +
> +static inline void ttc_pwm_writel(struct ttc_pwm_priv *priv,
> + unsigned long offset,
> + unsigned long val)
> +{
> + writel_relaxed(val, priv->base + offset);
> +}
> +
> +static inline u32 ttc_pwm_ch_readl(struct ttc_pwm_priv *priv,
> + struct pwm_device *pwm,
> + unsigned long offset)
> +{
> + unsigned long pwm_ch_offset = offset +
> + (TTC_PWM_CHANNEL_OFFSET * pwm->hwpwm);
> +
> + return ttc_pwm_readl(priv, pwm_ch_offset);
> +}
> +
> +static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
> + struct pwm_device *pwm,
> + unsigned long offset,
> + unsigned long val)
> +{
> + unsigned long pwm_ch_offset = offset +
> + (TTC_PWM_CHANNEL_OFFSET * pwm->hwpwm);
> +
> + ttc_pwm_writel(priv, pwm_ch_offset, val);
> +}
> +
> +static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
> +{
> + return container_of(chip, struct ttc_pwm_priv, chip);
> +}
> +
> +static void ttc_pwm_enable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
> +{
> + u32 ctrl_reg;
> +
> + ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> + ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN_MASK
> + | TTC_CNTR_CTRL_MATCH_MODE_EN_MASK | TTC_CNTR_CTRL_RST_MASK);
> + ctrl_reg &= (~(TTC_CNTR_CTRL_DIS_MASK | TTC_CNTR_CTRL_WAVE_EN_MASK));
superflous parenthesis.
> + ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
> +{
> + u32 ctrl_reg;
> +
> + ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> + ctrl_reg |= TTC_CNTR_CTRL_DIS_MASK;
> +
> + ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
> +}
> +
> +static void ttc_pwm_rev_polarity(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
> +{
> + u32 ctrl_reg;
> +
> + ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> + ctrl_reg ^= TTC_CNTR_CTRL_WAVE_POL_MASK;
> + ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
I prefer to have this more explicit like
ttc_pwm_set_polarity(..., enum pwm_polarity polarity)
> +}
> +
> +static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
> + struct pwm_device *pwm,
> + u32 div,
> + u32 period_cycles,
> + u32 duty_cycles)
> +{
> + u32 clk_reg;
> +
> + /* Set up prescalar */
> + clk_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CLK_CNTRL_OFFSET);
> + clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
> + clk_reg |= div;
We have
#define TTC_CLK_CNTRL_PSV_MASK GENMASK(4, 1)
so the LSB of div sets a bit outside of TTC_CLK_CNTRL_PSV_MASK; that
looks wrong.
> + ttc_pwm_ch_writel(priv, pwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> +
> + /* Set up period */
> + ttc_pwm_ch_writel(priv, pwm, TTC_INTR_VAL_OFFSET, period_cycles);
> +
> + /* Set up duty cycle */
> + ttc_pwm_ch_writel(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET, duty_cycles);
how does the output pin behave between the writes in this function (and
the others in .apply())?
> +}
> +
> +static int ttc_pwm_apply(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + u32 div = 0;
> + u64 period_cycles;
> + u64 duty_cycles;
> + unsigned long rate;
> + struct pwm_state cstate;
> + struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +
> + pwm_get_state(pwm, &cstate);
Please don't use pwm consumer functions. You can use pwm->state
directly.
> + if (state->polarity != cstate.polarity) {
> + if (cstate.enabled)
> + ttc_pwm_disable(priv, pwm);
> +
> + ttc_pwm_rev_polarity(priv, pwm);
> +
> + if (cstate.enabled)
> + ttc_pwm_enable(priv, pwm);
It would (probably) be nice to delay that enable until you configured
duty_cycle and period to reduce the amount of wrong signalling.
> + }
> +
You can exit early here if state->enabled == false.
> + if (state->period != cstate.period ||
> + state->duty_cycle != cstate.duty_cycle) {
(With an early exit above this check becomes wrong, simply drop it.)
> + rate = clk_get_rate(priv->clk);
> +
> + /* Prevent overflow by limiting to the maximum possible period */
> + period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> + period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate, NSEC_PER_SEC);
DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
reason for the build bot report.)
The usual alternative trick here is to do:
if (rate > NSEC_PER_SEC)
return -EINVAL;
period_cycles = mul_u64_u64_div_u64(state->period, rate, NSEC_PER_SEC);
> + if (period_cycles > priv->max) {
> + /* prescale frequency to fit requested period cycles within limit */
> + div = 1;
> +
> + while ((period_cycles > priv->max) && (div < 65536)) {
s/ / /
Also this can be calculated without a loop. Maybe it's easier to store
the timer width in priv instead of max = BIT(width) - 1.
> + rate = DIV_ROUND_CLOSEST(rate, BIT(div));
> + period_cycles = DIV_ROUND_CLOSEST(state->period * rate,
> + NSEC_PER_SEC);
> + if (period_cycles < priv->max)
> + break;
> + div++;
> + }
> +
> + if (period_cycles > priv->max)
> + return -ERANGE;
> + }
> +
> + duty_cycles = DIV_ROUND_CLOSEST(state->duty_cycle * rate,
> + NSEC_PER_SEC);
Please ensure that you configure a period that is not bigger than
state->period. Ditto for duty_cycle.
> + if (cstate.enabled)
> + ttc_pwm_disable(priv, pwm);
> +
> + ttc_pwm_set_counters(priv, pwm, div, period_cycles, duty_cycles);
> +
> + if (cstate.enabled)
> + ttc_pwm_enable(priv, pwm);
Another possible glitch here.
> + }
> +
> + if (state->enabled != cstate.enabled) {
> + if (state->enabled)
> + ttc_pwm_enable(priv, pwm);
> + else
> + ttc_pwm_disable(priv, pwm);
> + }
> +
> + return 0;
> +}
> +
> +static int ttc_pwm_get_state(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + struct pwm_state *state)
> +{
> + struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> + unsigned long rate;
> + u32 value;
> + u64 tmp;
> +
> + value = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> +
> + if (value & TTC_CNTR_CTRL_WAVE_POL_MASK)
> + state->polarity = PWM_POLARITY_INVERSED;
> + else
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + if (value & TTC_CNTR_CTRL_DIS_MASK)
> + state->enabled = false;
> + else
> + state->enabled = true;
> +
> + rate = clk_get_rate(priv->clk);
> +
> + tmp = ttc_pwm_ch_readl(priv, pwm, TTC_INTR_VAL_OFFSET);
> + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> +
> + tmp = ttc_pwm_ch_readl(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET);
> + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
This looks wrong for several reasons. Have you tested with PWM_DEBUG
enabled? What is rate typically?
.get_state is supposed to round up in its divisions. Also I miss a
factor of NSEC_PER_SEC here, and also div doesn't appear here?!
> + return 0;
> +}
> +
> +static struct pwm_device *
> +ttc_pwm_of_xlate(struct pwm_chip *chip, const struct of_phandle_args *args)
> +{
> + struct pwm_device *pwm;
> +
> + if (args->args[0] >= TTC_PWM_MAX_CH)
> + return NULL;
> +
> + pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> + if (IS_ERR(pwm))
> + return pwm;
> +
> + if (args->args[1])
> + pwm->args.period = args->args[1];
> +
> + if (args->args[2])
> + pwm->args.polarity = args->args[2];
> +
> + return pwm;
> +}
I don't see an advantage in this function over the default
of_pwm_xlate_with_flags(). (There are downsides though I think.)
If you drop this function there is only one usage of TTC_PWM_MAX_CH left
and you can drop this symbol, too. (In favour of
priv->chip.npwm = 3;
.)
> +static const struct pwm_ops ttc_pwm_ops = {
> + .apply = ttc_pwm_apply,
> + .get_state = ttc_pwm_get_state,
> + .owner = THIS_MODULE,
> +};
> +
> +static int ttc_pwm_probe(struct platform_device *pdev)
> +{
> + int ret;
> + int clksel;
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct ttc_pwm_priv *priv;
> + u32 pwm_cells;
> + u32 timer_width;
> + struct clk *clk_cs;
> +
Please add a comment here about coexistence with the timer mode.
> + ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> + if (ret == -EINVAL)
> + return -ENODEV;
> +
> + if (ret)
> + return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(priv->base))
> + return PTR_ERR(priv->base);
> +
> + ret = of_property_read_u32(np, "timer-width", &timer_width);
> + if (ret)
> + timer_width = 16;
> +
> + priv->max = BIT(timer_width) - 1;
> +
> + clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
ttc_pwm_readl, is this correct?
> + clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
> + clk_cs = of_clk_get(np, clksel);
Can you use devm_clk_get_enabled here?
> + if (IS_ERR(clk_cs))
> + return dev_err_probe(dev, PTR_ERR(clk_cs),
> + "ERROR: timer input clock not found\n");
> +
> + priv->clk = clk_cs;
> + ret = clk_prepare_enable(priv->clk);
> + if (ret)
> + return dev_err_probe(dev, ret, "Clock enable failed\n");
> +
> + clk_rate_exclusive_get(priv->clk);
> +
> + priv->chip.dev = dev;
> + priv->chip.ops = &ttc_pwm_ops;
> + priv->chip.npwm = TTC_PWM_MAX_CH;
> + priv->chip.of_xlate = ttc_pwm_of_xlate;
> + ret = pwmchip_add(&priv->chip);
> + if (ret) {
> + clk_rate_exclusive_put(priv->clk);
> + clk_disable_unprepare(priv->clk);
> + return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> + }
> +
> + platform_set_drvdata(pdev, priv);
> +
> + return 0;
> +}
> +
> +static int ttc_pwm_remove(struct platform_device *pdev)
> +{
> + struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
> +
> + pwmchip_remove(&priv->chip);
> + clk_rate_exclusive_put(priv->clk);
> + clk_disable_unprepare(priv->clk);
missing clk_put() (unless you switch to a devm_clk_get variant as
suggested above).
> +
> + return 0;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, Jan 12, 2023 at 12:45:24PM +0530, Mubin Sayyed wrote:
> TTC device can act either as clocksource/clockevent or
> PWM generator, it would be decided by pwm-cells property.
> TTC PWM feature would be supported through separate driver
> based on PWM framework.
>
> If pwm-cells property is present in TTC node, it would be
> treated as PWM device, and clocksource driver should just
> skip it.
>
> Signed-off-by: Mubin Sayyed <[email protected]>
> ---
> drivers/clocksource/timer-cadence-ttc.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
> index 4efd0cf3b602..ba46649148b1 100644
> --- a/drivers/clocksource/timer-cadence-ttc.c
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -476,6 +476,9 @@ static int __init ttc_timer_probe(struct platform_device *pdev)
> u32 timer_width = 16;
> struct device_node *timer = pdev->dev.of_node;
>
While it's more obvious here than in the PWM driver, a comment here
would be good, too.
> + if (of_property_read_bool(timer, "#pwm-cells"))
> + return -ENODEV;
> +
> if (initialized)
> return 0;
>
> --
> 2.25.1
>
>
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On 12/01/2023 08:15, Mubin Sayyed wrote:
> Cadence TTC timer can be configured as clocksource/clockevent
> or PWM device. Specific TTC device would be configured as PWM
> device, if pwm-cells property is present in the device tree
> node.
>
(...)
> +
> +static const struct of_device_id ttc_pwm_of_match[] = {
> + { .compatible = "cdns,ttc"},
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ttc_pwm_of_match);
> +
> +static struct platform_driver ttc_pwm_driver = {
> + .probe = ttc_pwm_probe,
> + .remove = ttc_pwm_remove,
> + .driver = {
> + .name = "ttc-pwm",
> + .of_match_table = of_match_ptr(ttc_pwm_of_match),
This leads to warnings. Drop it or use maybe_unused.
Best regards,
Krzysztof
Hi Uwe,
> -----Original Message-----
> From: Uwe Kleine-K?nig <[email protected]>
> Sent: Friday, January 13, 2023 2:55 AM
> To: Sayyed, Mubin <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; git (AMD-Xilinx) <[email protected]>; Simek, Michal
> <[email protected]>; Paladugu, Siva Durga Prasad
> <[email protected]>; [email protected]
> Subject: Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC
> PWM
>
> Hello,
>
> On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > Cadence TTC timer can be configured as clocksource/clockevent or PWM
> > device. Specific TTC device would be configured as PWM device, if
> > pwm-cells property is present in the device tree node.
> >
> > In case of Zynq, ZynqMP and Versal SoC's, each TTC device has 3
> > timers/counters, so maximum 3 PWM channels can be configured for each
> > TTC IP instance. Also, output of 0th PWM channel of each TTC device
> > can be routed to MIO or EMIO, and output of 2nd and 3rd PWM channel
> > can be routed only to EMIO.
> >
> > Period for given PWM channel is configured through interval timer and
> > duty cycle through match counter.
> >
> > Signed-off-by: Mubin Sayyed <[email protected]>
> > ---
> > drivers/pwm/Kconfig | 11 ++
> > drivers/pwm/Makefile | 1 +
> > drivers/pwm/pwm-cadence.c | 363
> > ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 375 insertions(+)
> > create mode 100644 drivers/pwm/pwm-cadence.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index
> > dae023d783a2..9e0f1fae4711 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -504,6 +504,17 @@ config PWM_SIFIVE
> > To compile this driver as a module, choose M here: the module
> > will be called pwm-sifive.
> >
> > +config PWM_CADENCE
>
> Please keep both Kconfig and Makefile sorted alphabetically.
[Mubin]: OK
>
> > + tristate "Cadence PWM support"
> > + depends on OF
> > + depends on COMMON_CLK
> > + help
> > + Generic PWM framework driver for cadence TTC IP found on
> > + Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
> > + channels. Output of 0th PWM channel of each TTC device can
> > + be routed to MIO or EMIO, and output of 1st and 2nd PWM
> > + channels can be routed only to EMIO.
> > +
> > config PWM_SL28CPLD
> > tristate "Kontron sl28cpld PWM support"
> > depends on MFD_SL28CPLD || COMPILE_TEST [...] diff --git
> > a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c new file
> mode
> > 100644 index 000000000000..de981df3ec5a
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-cadence.c
> > @@ -0,0 +1,363 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver to configure cadence TTC timer as PWM
> > + * generator
> > + *
> > + * Copyright (C) 2022, Advanced Micro Devices, Inc.
> > + */
>
> Is there a public manual for the hardware? If yes, please mention a link here.
[Mubin]: I did not find any public manual from cadence. However, details can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available publicly.
>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
>
> clk-provider looks wronge here, your device is only a consumer, isn't it?
[Mubin]: will remove it
>
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/of_address.h>
> > +
> > +#define TTC_CLK_CNTRL_OFFSET 0x00
> > +#define TTC_CNT_CNTRL_OFFSET 0x0C
> > +#define TTC_MATCH_CNT_VAL_OFFSET 0x30
> > +#define TTC_COUNT_VAL_OFFSET 0x18
> > +#define TTC_INTR_VAL_OFFSET 0x24
> > +#define TTC_ISR_OFFSET 0x54
> > +#define TTC_IER_OFFSET 0x60
> > +#define TTC_PWM_CHANNEL_OFFSET 0x4
> > +
> > +#define TTC_CLK_CNTRL_CSRC_MASK BIT(5)
> > +#define TTC_CLK_CNTRL_PSV_MASK GENMASK(4, 1)
> > +
> > +#define TTC_CNTR_CTRL_DIS_MASK BIT(0)
> > +#define TTC_CNTR_CTRL_INTR_MODE_EN_MASK BIT(1)
> > +#define TTC_CNTR_CTRL_MATCH_MODE_EN_MASK BIT(3)
> > +#define TTC_CNTR_CTRL_RST_MASK BIT(4)
> > +#define TTC_CNTR_CTRL_WAVE_EN_MASK BIT(5)
> > +#define TTC_CNTR_CTRL_WAVE_POL_MASK BIT(6)
>
> If you ask me do s/_OFFSET// and s/_MASK// on all these definitions.
[Mubin]: will update them
>
> > +#define TTC_CLK_CNTRL_PSV_SHIFT 1
>
> This is unused.
[Mubin]: will remove it .
> > +
> > +#define TTC_PWM_MAX_CH 3
> > +
> > +/**
> > + * struct ttc_pwm_priv - Private data for TTC PWM drivers
> > + * @chip: PWM chip structure representing PWM controller
> > + * @clk: TTC input clock
> > + * @max: Maximum value of the counters
> > + * @base: Base address of TTC instance
> > + */
> > +struct ttc_pwm_priv {
> > + struct pwm_chip chip;
> > + struct clk *clk;
> > + u32 max;
> > + void __iomem *base;
> > +};
> > +
> > +static inline u32 ttc_pwm_readl(struct ttc_pwm_priv *priv,
> > + unsigned long offset)
> > +{
> > + return readl_relaxed(priv->base + offset); }
> > +
> > +static inline void ttc_pwm_writel(struct ttc_pwm_priv *priv,
> > + unsigned long offset,
> > + unsigned long val)
> > +{
> > + writel_relaxed(val, priv->base + offset); }
> > +
> > +static inline u32 ttc_pwm_ch_readl(struct ttc_pwm_priv *priv,
> > + struct pwm_device *pwm,
> > + unsigned long offset)
> > +{
> > + unsigned long pwm_ch_offset = offset +
> > + (TTC_PWM_CHANNEL_OFFSET * pwm-
> >hwpwm);
> > +
> > + return ttc_pwm_readl(priv, pwm_ch_offset); }
> > +
> > +static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
> > + struct pwm_device *pwm,
> > + unsigned long offset,
> > + unsigned long val)
> > +{
> > + unsigned long pwm_ch_offset = offset +
> > + (TTC_PWM_CHANNEL_OFFSET * pwm-
> >hwpwm);
> > +
> > + ttc_pwm_writel(priv, pwm_ch_offset, val); }
> > +
> > +static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct
> > +pwm_chip *chip) {
> > + return container_of(chip, struct ttc_pwm_priv, chip); }
> > +
> > +static void ttc_pwm_enable(struct ttc_pwm_priv *priv, struct
> > +pwm_device *pwm) {
> > + u32 ctrl_reg;
> > +
> > + ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> > + ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN_MASK
> > + |
> TTC_CNTR_CTRL_MATCH_MODE_EN_MASK | TTC_CNTR_CTRL_RST_MASK);
> > + ctrl_reg &= (~(TTC_CNTR_CTRL_DIS_MASK |
> > +TTC_CNTR_CTRL_WAVE_EN_MASK));
>
> superflous parenthesis.
[Mubin]: will remove it in v2
>
> > + ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg); }
> > +
> > +static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct
> > +pwm_device *pwm) {
> > + u32 ctrl_reg;
> > +
> > + ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> > + ctrl_reg |= TTC_CNTR_CTRL_DIS_MASK;
> > +
> > + ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg); }
> > +
> > +static void ttc_pwm_rev_polarity(struct ttc_pwm_priv *priv, struct
> > +pwm_device *pwm) {
> > + u32 ctrl_reg;
> > +
> > + ctrl_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> > + ctrl_reg ^= TTC_CNTR_CTRL_WAVE_POL_MASK;
> > + ttc_pwm_ch_writel(priv, pwm, TTC_CNT_CNTRL_OFFSET, ctrl_reg);
>
> I prefer to have this more explicit like
>
> ttc_pwm_set_polarity(..., enum pwm_polarity polarity)
[Mubin]: Agreed
>
> > +}
> > +
> > +static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
> > + struct pwm_device *pwm,
> > + u32 div,
> > + u32 period_cycles,
> > + u32 duty_cycles)
> > +{
> > + u32 clk_reg;
> > +
> > + /* Set up prescalar */
> > + clk_reg = ttc_pwm_ch_readl(priv, pwm, TTC_CLK_CNTRL_OFFSET);
> > + clk_reg &= ~TTC_CLK_CNTRL_PSV_MASK;
> > + clk_reg |= div;
>
> We have
>
> #define TTC_CLK_CNTRL_PSV_MASK GENMASK(4, 1)
>
> so the LSB of div sets a bit outside of TTC_CLK_CNTRL_PSV_MASK; that looks
> wrong.
[Mubin]: will fix it in v2
>
> > + ttc_pwm_ch_writel(priv, pwm, TTC_CLK_CNTRL_OFFSET, clk_reg);
> > +
> > + /* Set up period */
> > + ttc_pwm_ch_writel(priv, pwm, TTC_INTR_VAL_OFFSET,
> period_cycles);
> > +
> > + /* Set up duty cycle */
> > + ttc_pwm_ch_writel(priv, pwm, TTC_MATCH_CNT_VAL_OFFSET,
> duty_cycles);
>
> how does the output pin behave between the writes in this function (and the
> others in .apply())?
[Mubin]: ttc_pwm_apply is disabling counters before calling this function, and enabling it back immediately after it. So, output pin would be low during configuration.
>
> > +}
> > +
> > +static int ttc_pwm_apply(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + u32 div = 0;
> > + u64 period_cycles;
> > + u64 duty_cycles;
> > + unsigned long rate;
> > + struct pwm_state cstate;
> > + struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> > +
> > + pwm_get_state(pwm, &cstate);
>
> Please don't use pwm consumer functions. You can use pwm->state directly.
[Mubin]: will fix in v2
>
> > + if (state->polarity != cstate.polarity) {
> > + if (cstate.enabled)
> > + ttc_pwm_disable(priv, pwm);
> > +
> > + ttc_pwm_rev_polarity(priv, pwm);
> > +
> > + if (cstate.enabled)
> > + ttc_pwm_enable(priv, pwm);
>
> It would (probably) be nice to delay that enable until you configured
> duty_cycle and period to reduce the amount of wrong signalling.
[Mubin]: Agreed
>
> > + }
> > +
>
> You can exit early here if state->enabled == false.
>
> > + if (state->period != cstate.period ||
> > + state->duty_cycle != cstate.duty_cycle) {
>
> (With an early exit above this check becomes wrong, simply drop it.)
[Mubin]: OK
>
> > + rate = clk_get_rate(priv->clk);
> > +
> > + /* Prevent overflow by limiting to the maximum possible
> period */
> > + period_cycles = min_t(u64, state->period, ULONG_MAX *
> NSEC_PER_SEC);
> > + period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate,
> > +NSEC_PER_SEC);
>
> DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> reason for the build bot report.)
>
> The usual alternative trick here is to do:
>
> if (rate > NSEC_PER_SEC)
> return -EINVAL;
>
> period_cycles = mul_u64_u64_div_u64(state->period, rate,
> NSEC_PER_SEC);
[Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting accuracy while generating PWM signal of high frequency(output frequency above 5 MHZ, input 100 MHZ ). Usage of DIV_ROUND_CLOSEST improved accuracy. Can you please suggest better alternative for improving accuracy.
>
> > + if (period_cycles > priv->max) {
> > + /* prescale frequency to fit requested period cycles
> within limit */
> > + div = 1;
> > +
> > + while ((period_cycles > priv->max) && (div < 65536))
> {
>
> s/ / /
[Mubin]: OK
>
> Also this can be calculated without a loop. Maybe it's easier to store the
> timer width in priv instead of max = BIT(width) - 1.
>
> > + rate = DIV_ROUND_CLOSEST(rate, BIT(div));
> > + period_cycles = DIV_ROUND_CLOSEST(state-
> >period * rate,
> > +
> NSEC_PER_SEC);
[Mubin]: Agreed, will implement that logic in v2.
> > + if (period_cycles < priv->max)
> > + break;
> > + div++;
> > + }
> > +
> > + if (period_cycles > priv->max)
> > + return -ERANGE;
> > + }
> > +
> > + duty_cycles = DIV_ROUND_CLOSEST(state->duty_cycle * rate,
> > + NSEC_PER_SEC);
>
> Please ensure that you configure a period that is not bigger than
> state->period. Ditto for duty_cycle.
[Mubin]: OK
>
> > + if (cstate.enabled)
> > + ttc_pwm_disable(priv, pwm);
> > +
> > + ttc_pwm_set_counters(priv, pwm, div, period_cycles,
> duty_cycles);
> > +
> > + if (cstate.enabled)
> > + ttc_pwm_enable(priv, pwm);
>
> Another possible glitch here.
[Mubin]: Can you please elaborate.
>
> > + }
> > +
> > + if (state->enabled != cstate.enabled) {
> > + if (state->enabled)
> > + ttc_pwm_enable(priv, pwm);
> > + else
> > + ttc_pwm_disable(priv, pwm);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int ttc_pwm_get_state(struct pwm_chip *chip,
> > + struct pwm_device *pwm,
> > + struct pwm_state *state)
> > +{
> > + struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> > + unsigned long rate;
> > + u32 value;
> > + u64 tmp;
> > +
> > + value = ttc_pwm_ch_readl(priv, pwm, TTC_CNT_CNTRL_OFFSET);
> > +
> > + if (value & TTC_CNTR_CTRL_WAVE_POL_MASK)
> > + state->polarity = PWM_POLARITY_INVERSED;
> > + else
> > + state->polarity = PWM_POLARITY_NORMAL;
> > +
> > + if (value & TTC_CNTR_CTRL_DIS_MASK)
> > + state->enabled = false;
> > + else
> > + state->enabled = true;
> > +
> > + rate = clk_get_rate(priv->clk);
> > +
> > + tmp = ttc_pwm_ch_readl(priv, pwm, TTC_INTR_VAL_OFFSET);
> > + state->period = DIV_ROUND_CLOSEST_ULL(tmp, rate);
> > +
> > + tmp = ttc_pwm_ch_readl(priv, pwm,
> TTC_MATCH_CNT_VAL_OFFSET);
> > + state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, rate);
>
> This looks wrong for several reasons. Have you tested with PWM_DEBUG
> enabled? What is rate typically?
>
> .get_state is supposed to round up in its divisions. Also I miss a factor of
> NSEC_PER_SEC here, and also div doesn't appear here?!
[Mubin]: Did not tested it with PWM_DEBUG. Typical rate is 100 MHZ. I will fix it in v2.
>
> > + return 0;
> > +}
> > +
> > +static struct pwm_device *
> > +ttc_pwm_of_xlate(struct pwm_chip *chip, const struct of_phandle_args
> > +*args) {
> > + struct pwm_device *pwm;
> > +
> > + if (args->args[0] >= TTC_PWM_MAX_CH)
> > + return NULL;
> > +
> > + pwm = pwm_request_from_chip(chip, args->args[0], NULL);
> > + if (IS_ERR(pwm))
> > + return pwm;
> > +
> > + if (args->args[1])
> > + pwm->args.period = args->args[1];
> > +
> > + if (args->args[2])
> > + pwm->args.polarity = args->args[2];
> > +
> > + return pwm;
> > +}
>
> I don't see an advantage in this function over the default
> of_pwm_xlate_with_flags(). (There are downsides though I think.)
>
> If you drop this function there is only one usage of TTC_PWM_MAX_CH left
> and you can drop this symbol, too. (In favour of
>
> priv->chip.npwm = 3;
>
> .)
[Mubin]: Agreed
>
> > +static const struct pwm_ops ttc_pwm_ops = {
> > + .apply = ttc_pwm_apply,
> > + .get_state = ttc_pwm_get_state,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int ttc_pwm_probe(struct platform_device *pdev) {
> > + int ret;
> > + int clksel;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node;
> > + struct ttc_pwm_priv *priv;
> > + u32 pwm_cells;
> > + u32 timer_width;
> > + struct clk *clk_cs;
> > +
>
> Please add a comment here about coexistence with the timer mode.
[Mubin]: OK
>
> > + ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
> > + if (ret == -EINVAL)
> > + return -ENODEV;
> > +
> > + if (ret)
> > + return dev_err_probe(dev, ret, "could not read #pwm-
> cells\n");
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->base = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(priv->base))
> > + return PTR_ERR(priv->base);
> > +
> > + ret = of_property_read_u32(np, "timer-width", &timer_width);
> > + if (ret)
> > + timer_width = 16;
> > +
> > + priv->max = BIT(timer_width) - 1;
> > +
> > + clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
>
> TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
> ttc_pwm_readl, is this correct?
[Mubin]: Here TTC_CLK_CNTRL_OFFSET is being read only for 0th channel, so using ttc_pwm_readl does not impact offsets.
>
> > + clksel = !!(clksel & TTC_CLK_CNTRL_CSRC_MASK);
> > + clk_cs = of_clk_get(np, clksel);
>
> Can you use devm_clk_get_enabled here?
[Mubin]: Yes, will switch to devm_clk_get_enabled in v2.
>
> > + if (IS_ERR(clk_cs))
> > + return dev_err_probe(dev, PTR_ERR(clk_cs),
> > + "ERROR: timer input clock not found\n");
> > +
> > + priv->clk = clk_cs;
> > + ret = clk_prepare_enable(priv->clk);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Clock enable failed\n");
> > +
> > + clk_rate_exclusive_get(priv->clk);
> > +
> > + priv->chip.dev = dev;
> > + priv->chip.ops = &ttc_pwm_ops;
> > + priv->chip.npwm = TTC_PWM_MAX_CH;
> > + priv->chip.of_xlate = ttc_pwm_of_xlate;
> > + ret = pwmchip_add(&priv->chip);
> > + if (ret) {
> > + clk_rate_exclusive_put(priv->clk);
> > + clk_disable_unprepare(priv->clk);
> > + return dev_err_probe(dev, ret, "Could not register PWM
> chip\n");
> > + }
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int ttc_pwm_remove(struct platform_device *pdev) {
> > + struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
> > +
> > + pwmchip_remove(&priv->chip);
> > + clk_rate_exclusive_put(priv->clk);
> > + clk_disable_unprepare(priv->clk);
>
> missing clk_put() (unless you switch to a devm_clk_get variant as suggested
> above).
[Mubin] will be switching to devm_clk_get in next version.
Thanks,
Mubin
>
> > +
> > + return 0;
> > +}
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Mubin,
On Tue, Jan 17, 2023 at 09:58:10AM +0000, Sayyed, Mubin wrote:
> > On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > Is there a public manual for the hardware? If yes, please mention a link here.
> [Mubin]: I did not find any public manual from cadence. However, details can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available publicly.
Thenk please add a link to that one.
> > how does the output pin behave between the writes in this function (and the
> > others in .apply())?
> [Mubin]: ttc_pwm_apply is disabling counters before calling this
> function, and enabling it back immediately after it. So, output pin
> would be low during configuration.
Please document this behaviour (i.e. "A disabled PWM emits a zero") in a
paragraph at the top of the driver in the format that e.g.
drivers/pwm/pwm-pxa.c is using. Also please test if it emits a zero or
the inactive level, i.e. if the output depends on the polarity setting.
> > > + rate = clk_get_rate(priv->clk);
> > > +
> > > + /* Prevent overflow by limiting to the maximum possible period */
> > > + period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> > > + period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate, NSEC_PER_SEC);
> >
> > DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> > reason for the build bot report.)
> >
> > The usual alternative trick here is to do:
> >
> > if (rate > NSEC_PER_SEC)
> > return -EINVAL;
> >
> > period_cycles = mul_u64_u64_div_u64(state->period, rate,
> > NSEC_PER_SEC);
> [Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting
> accuracy while generating PWM signal of high frequency(output
> frequency above 5 MHZ, input 100 MHZ ). Usage of DIV_ROUND_CLOSEST
> improved accuracy. Can you please suggest better alternative for
> improving accuracy.
Unless I'm missing something, you have to adjust your definition of
accuracy :-)
If you request (say) a period of 149 ns and the nearest actual values
your hardware can provide left and right of that is 140 ns and 150 ns,
140ns is the one to select. That is pick the biggest possible period not
bigger than the requested period. And with that pick the biggest
possible duty_cycle not bigger than the requested duty_cycle. (i.e. it's
a bug to emit period=150ns if 149 was requested.)
There are ideas to implement something like
pwm_apply_nearest(...);
but that's still in the idea stage (and will depend on the lowlevel
drivers implementing the strategy described above).
> > Another possible glitch here.
> [Mubin]: Can you please elaborate.
If you switch polarity (say from normal to inverted) and duty/period you do (simplified)
ttc_pwm_disable(priv, pwm); // might yield a falling edge
set polarity // might yield a raising edge
ttc_pwm_enable(priv, pwm); // might yield a falling edge
... some calculations
ttc_pwm_disable(priv, pwm); // might yield a raising edge
setup counters
ttc_pwm_enable(priv, pwm); // might yield a falling edge
so during apply() the output might toggle several times at a high(?)
frequency. Depending on what you have connected to the PWM this is bad.
(A LED might blink, a backlight might flicker, a motor might stutter and
enter an error state or accelerate for a moment.)
> > > + clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
> >
> > TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
> > ttc_pwm_readl, is this correct?
> [Mubin]: Here TTC_CLK_CNTRL_OFFSET is being read only for 0th channel, so using ttc_pwm_readl does not impact offsets.
So which clk is selected (for all channels?) depends on how channel 0's
clock setting is setup by the bootloader (or bootrom)? Sounds strange.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
> -----Original Message-----
> From: Uwe Kleine-K?nig <[email protected]>
> Sent: Tuesday, January 17, 2023 4:57 PM
> To: Sayyed, Mubin <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; git (AMD-Xilinx) <[email protected]>; Simek, Michal
> <[email protected]>; Paladugu, Siva Durga Prasad
> <[email protected]>; [email protected];
> [email protected]
> Subject: Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC
> PWM
>
> Hello Mubin,
>
> On Tue, Jan 17, 2023 at 09:58:10AM +0000, Sayyed, Mubin wrote:
> > > On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > > Is there a public manual for the hardware? If yes, please mention a link
> here.
> > [Mubin]: I did not find any public manual from cadence. However, details
> can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available
> publicly.
>
> Thenk please add a link to that one.
>
> > > how does the output pin behave between the writes in this function
> > > (and the others in .apply())?
> > [Mubin]: ttc_pwm_apply is disabling counters before calling this
> > function, and enabling it back immediately after it. So, output pin
> > would be low during configuration.
>
> Please document this behaviour (i.e. "A disabled PWM emits a zero") in a
> paragraph at the top of the driver in the format that e.g.
> drivers/pwm/pwm-pxa.c is using. Also please test if it emits a zero or the
> inactive level, i.e. if the output depends on the polarity setting.
[Mubin]: will confirm behavior on hardware and document it.
>
> > > > + rate = clk_get_rate(priv->clk);
> > > > +
> > > > + /* Prevent overflow by limiting to the maximum possible
> period */
> > > > + period_cycles = min_t(u64, state->period, ULONG_MAX *
> NSEC_PER_SEC);
> > > > + period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate,
> > > > +NSEC_PER_SEC);
> > >
> > > DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> > > reason for the build bot report.)
> > >
> > > The usual alternative trick here is to do:
> > >
> > > if (rate > NSEC_PER_SEC)
> > > return -EINVAL;
> > >
> > > period_cycles = mul_u64_u64_div_u64(state->period, rate,
> > > NSEC_PER_SEC);
> > [Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting
> > accuracy while generating PWM signal of high frequency(output
> > frequency above 5 MHZ, input 100 MHZ ). Usage of DIV_ROUND_CLOSEST
> > improved accuracy. Can you please suggest better alternative for
> > improving accuracy.
>
> Unless I'm missing something, you have to adjust your definition of accuracy
> :-)
>
> If you request (say) a period of 149 ns and the nearest actual values your
> hardware can provide left and right of that is 140 ns and 150 ns, 140ns is the
> one to select. That is pick the biggest possible period not bigger than the
> requested period. And with that pick the biggest possible duty_cycle not
> bigger than the requested duty_cycle. (i.e. it's a bug to emit period=150ns if
> 149 was requested.)
>
> There are ideas to implement something like
>
> pwm_apply_nearest(...);
>
> but that's still in the idea stage (and will depend on the lowlevel drivers
> implementing the strategy described above).
[Mubin]: Thanks for explaining, will switch to mul_u64_u64_div_u64, though percentage of deviation would be more for PWM signal of high frequency. For example, requested period is 50 ns, requested duty cycle is 49 ns(98%), actual duty cycle set by driver would be 40ns (80%).
>
> > > Another possible glitch here.
> > [Mubin]: Can you please elaborate.
>
> If you switch polarity (say from normal to inverted) and duty/period you do
> (simplified)
>
> ttc_pwm_disable(priv, pwm); // might yield a falling edge
> set polarity // might yield a raising edge
> ttc_pwm_enable(priv, pwm); // might yield a falling edge
> ... some calculations
> ttc_pwm_disable(priv, pwm); // might yield a raising edge
> setup counters
> ttc_pwm_enable(priv, pwm); // might yield a falling edge
>
> so during apply() the output might toggle several times at a high(?)
> frequency. Depending on what you have connected to the PWM this is bad.
> (A LED might blink, a backlight might flicker, a motor might stutter and enter
> an error state or accelerate for a moment.)
[Mubin]: Agreed, will modify logic to avoid toggling
>
> > > > + clksel = ttc_pwm_readl(priv, TTC_CLK_CNTRL_OFFSET);
> > >
> > > TTC_CLK_CNTRL_OFFSET is as parameter for ttc_pwm_ch_readl and
> > > ttc_pwm_readl, is this correct?
> > [Mubin]: Here TTC_CLK_CNTRL_OFFSET is being read only for 0th channel,
> so using ttc_pwm_readl does not impact offsets.
>
> So which clk is selected (for all channels?) depends on how channel 0's clock
> setting is setup by the bootloader (or bootrom)? Sounds strange.
[Mubin]: I confirmed that each channel can have separate clk source. I will update it for all channels and modify ttc_pwm_priv structure accordingly.
Thanks,
Mubin
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-K?nig |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Jan 17, 2023 at 12:55:06PM +0000, Sayyed, Mubin wrote:
> Hi Uwe,
>
> > -----Original Message-----
> > From: Uwe Kleine-K?nig <[email protected]>
> > Sent: Tuesday, January 17, 2023 4:57 PM
> > To: Sayyed, Mubin <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]; git (AMD-Xilinx) <[email protected]>; Simek, Michal
> > <[email protected]>; Paladugu, Siva Durga Prasad
> > <[email protected]>; [email protected];
> > [email protected]
> > Subject: Re: [LINUX PATCH 3/3] pwm: pwm-cadence: Add support for TTC
> > PWM
> >
> > Hello Mubin,
> >
> > On Tue, Jan 17, 2023 at 09:58:10AM +0000, Sayyed, Mubin wrote:
> > > > On Thu, Jan 12, 2023 at 12:45:26PM +0530, Mubin Sayyed wrote:
> > > > Is there a public manual for the hardware? If yes, please mention a link
> > here.
> > > [Mubin]: I did not find any public manual from cadence. However, details
> > can be found in Zynq-7000/ Zynq UltraScale/Versal ACAP TRM available
> > publicly.
> >
> > Thenk please add a link to that one.
> >
> > > > how does the output pin behave between the writes in this function
> > > > (and the others in .apply())?
> > > [Mubin]: ttc_pwm_apply is disabling counters before calling this
> > > function, and enabling it back immediately after it. So, output pin
> > > would be low during configuration.
> >
> > Please document this behaviour (i.e. "A disabled PWM emits a zero") in a
> > paragraph at the top of the driver in the format that e.g.
> > drivers/pwm/pwm-pxa.c is using. Also please test if it emits a zero or the
> > inactive level, i.e. if the output depends on the polarity setting.
>
> [Mubin]: will confirm behavior on hardware and document it.
> >
> > > > > + rate = clk_get_rate(priv->clk);
> > > > > +
> > > > > + /* Prevent overflow by limiting to the maximum possible
> > period */
> > > > > + period_cycles = min_t(u64, state->period, ULONG_MAX *
> > NSEC_PER_SEC);
> > > > > + period_cycles = DIV_ROUND_CLOSEST(period_cycles * rate,
> > > > > +NSEC_PER_SEC);
> > > >
> > > > DIV_ROUND_CLOSEST isn't suiteable to work with u64. (That's also the
> > > > reason for the build bot report.)
> > > >
> > > > The usual alternative trick here is to do:
> > > >
> > > > if (rate > NSEC_PER_SEC)
> > > > return -EINVAL;
> > > >
> > > > period_cycles = mul_u64_u64_div_u64(state->period, rate,
> > > > NSEC_PER_SEC);
> > > [Mubin]: Initially I tried mul_u64_u64_div_u64, it was impacting
> > > accuracy while generating PWM signal of high frequency(output
> > > frequency above 5 MHZ, input 100 MHZ ). Usage of DIV_ROUND_CLOSEST
> > > improved accuracy. Can you please suggest better alternative for
> > > improving accuracy.
> >
> > Unless I'm missing something, you have to adjust your definition of accuracy
> > :-)
> >
> > If you request (say) a period of 149 ns and the nearest actual values your
> > hardware can provide left and right of that is 140 ns and 150 ns, 140ns is the
> > one to select. That is pick the biggest possible period not bigger than the
> > requested period. And with that pick the biggest possible duty_cycle not
> > bigger than the requested duty_cycle. (i.e. it's a bug to emit period=150ns if
> > 149 was requested.)
> >
> > There are ideas to implement something like
> >
> > pwm_apply_nearest(...);
> >
> > but that's still in the idea stage (and will depend on the lowlevel drivers
> > implementing the strategy described above).
> [Mubin]: Thanks for explaining, will switch to mul_u64_u64_div_u64,
> though percentage of deviation would be more for PWM signal of high
> frequency. For example, requested period is 50 ns, requested duty
> cycle is 49 ns(98%), actual duty cycle set by driver would be 40ns
> (80%).
Note it's a trade-off to let drivers round down and not nearest. The
motivating reasons are:
- division rounding down is the default in C, so it's a tad easier
- there are less corner cases
(There are two strange effects that I'm aware of:
- Consider for example a hardware that can implement periods of
length 49.2 ns, 49.4 ns and 50.7 ns (and nothing in-between). If
you request 50 ns, you get 49.4. .get_state would then tell you
that the hardware has configured 49 ns. But if you request 49
ns, you don't get 49.4 ns.
- Rounding to the nearest time is different to rounding to the
nearest frequency:
Consider a hardware than can configure a period of 200 ns (freq:
5 MHz) and 300 ns (freq: 3.33333 MHz). If you request 249 ns
(freq: 4.016 MHz) you'd get 200 ns if you round to the nearest
time, but 300 if you round to the nearest frequency.
Both problems don't happen with rounding down.)
- .get_state which is supposed to be the inverse of .apply() needs one
more distinction of cases.
(That is, if the nearest integer above or below the actual value
should be returned. And additionally another arbitraty choice what to
return for 50.5 ns)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |