[PATCH v3 1/3] dm: core: Bind another driver with the same compatible string

[PATCH v3 1/3] dm: core: Bind another driver with the same compatible string

[Michal Simek]

When one IP can have multiple configurations (like timer and PWM) covered
by multiple drivers. Because it is the same IP it should also have the same
compatible string.
Current code look for the first driver which matches compatible string and
call bind function. If this is not the right driver which is express by
returning -ENODEV ("Driver XYZ refuses to bind") there is no reason not to
continue over compatible list to try to find out different driver with the
same compatible string which match it.

When the first compatible string is found core looks for driver bind()
function. If if assigned driver refuse to bind, core continue in a loop to
check if there is another driver which match compatible string.

This driver is going to be used with cdnc,ttc driver which has driver as
timer and also can be used as PWM. The difference is done via #pwm-cells
property in DT.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3:
- New patch in series

 drivers/core/lists.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/core/lists.c b/drivers/core/lists.c
index 350b9d32687c..199ee3a8e0fc 100644
--- a/drivers/core/lists.c
+++ b/drivers/core/lists.c
@@ -221,12 +221,14 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
 		compat = compat_list + i;
 		log_debug("   - attempt to match compatible string '%s'\n",
 			  compat);
-
-		for (entry = driver; entry != driver + n_ents; entry++) {
+		entry = driver;
+next:
+		for (; entry != driver + n_ents; entry++) {
 			ret = driver_check_compatible(entry->of_match, &id,
 						      compat);
 			if (!ret)
 				break;
+
 		}
 		if (entry == driver + n_ents)
 			continue;
@@ -246,7 +248,8 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
 						   id->data, node, &dev);
 		if (ret == -ENODEV) {
 			log_debug("Driver '%s' refuses to bind\n", entry->name);
-			continue;
+			entry++;
+			goto next;
 		}
 		if (ret) {
 			dm_warn("Error binding driver '%s': %d\n", entry->name,
-- 
2.33.0

[PATCH v3 2/3] timer: cadence: Add bind function to driver

[Michal Simek]

When DT node has pwm-cells property it shouldn't be bind as timer driver
but as PWM driver. That's why make sure that this property is checked.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3:
- New patch in series

 drivers/timer/cadence-ttc.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
index 2f95d45ecd7a..2eff45060ad6 100644
--- a/drivers/timer/cadence-ttc.c
+++ b/drivers/timer/cadence-ttc.c
@@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev)
 	return 0;
 }
 
+static int cadence_ttc_bind(struct udevice *dev)
+{
+	const char *cells;
+
+	cells = dev_read_prop(dev, "#pwm-cells", NULL);
+	if (cells)
+		return -ENODEV;
+
+	return 0;
+}
+
 static const struct timer_ops cadence_ttc_ops = {
 	.get_count = cadence_ttc_get_count,
 };
@@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = {
 	.priv_auto	= sizeof(struct cadence_ttc_priv),
 	.probe = cadence_ttc_probe,
 	.ops = &cadence_ttc_ops,
+	.bind = cadence_ttc_bind,
 };
-- 
2.33.0

[PATCH v3 3/3] pwm: Add driver for cadence TTC

[Michal Simek]

TTC has three modes of operations. Timer, PWM and input counters.

There is already driver for timer under CADENCE_TTC_TIMER which is used for
ZynqMP R5 configuration.
This driver is targeting PWM which is for example configuration which can
be used for fan control.
The driver has been tested on Xilinx Kria SOM platform where fan is
connected to one PL pin. When TTC output is connected via EMIO to PL pin
TTC pwm can be configured and tested for example like this:
pwm config 0 0 10000 1200
pwm enable 0 0
pwm config 0 0 10000 1400
pwm config 0 0 10000 1600

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v3:
- Add bind function to check pwm-cells to recognize PWM driver
- Use timer-width in prescaler calculation
- Record timer width in platdata instead of timer mask
- Also disable wave out in config function

Changes in v2:
- Detect pwm-cells property for PWM driver
- Fix all macro names
- Use BIT and GENMASK macros
- Introduce TTC_REG macro for reg offsets
- Use FIELD_PREP
- Move cadence_ttc_pwm_of_to_plat() below probe
- Introduce struct cadence_ttc_pwm_plat
- Read timer-width from DT
- Use NSEC_PER_SEC macro
- Use clock_ctrl variable instead of x - all reported by Sean

 MAINTAINERS                   |   1 +
 drivers/pwm/Kconfig           |   7 +
 drivers/pwm/Makefile          |   1 +
 drivers/pwm/pwm-cadence-ttc.c | 261 ++++++++++++++++++++++++++++++++++
 4 files changed, 270 insertions(+)
 create mode 100644 drivers/pwm/pwm-cadence-ttc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 31b49c0a95f0..d0b1845384bf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -599,6 +599,7 @@ F:	drivers/mmc/zynq_sdhci.c
 F:	drivers/mtd/nand/raw/zynq_nand.c
 F:	drivers/net/phy/xilinx_phy.c
 F:	drivers/net/zynq_gem.c
+F:	drivers/pwm/pwm-cadence-ttc.c
 F:	drivers/serial/serial_zynq.c
 F:	drivers/reset/reset-zynqmp.c
 F:	drivers/rtc/zynqmp_rtc.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index cf7f4c6840ce..176e703c8fbb 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -9,6 +9,13 @@ config DM_PWM
 	  frequency/period can be controlled along with the proportion of that
 	  time that the signal is high.
 
+config PWM_CADENCE_TTC
+	bool "Enable support for the Cadence TTC PWM"
+	depends on DM_PWM && !CADENCE_TTC_TIMER
+	help
+	  Cadence TTC can be configured as timer which is done via
+	  CONFIG_CADENCE_TTC_TIMER or as PWM. This is covering only PWM now.
+
 config PWM_CROS_EC
 	bool "Enable support for the Chrome OS EC PWM"
 	depends on DM_PWM
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 10d244bfb79d..abf5af41d2cc 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -10,6 +10,7 @@
 
 obj-$(CONFIG_DM_PWM)		+= pwm-uclass.o
 
+obj-$(CONFIG_PWM_CADENCE_TTC)	+= pwm-cadence-ttc.o
 obj-$(CONFIG_PWM_CROS_EC)	+= cros_ec_pwm.o
 obj-$(CONFIG_PWM_EXYNOS)	+= exynos_pwm.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o pwm-imx-util.o
diff --git a/drivers/pwm/pwm-cadence-ttc.c b/drivers/pwm/pwm-cadence-ttc.c
new file mode 100644
index 000000000000..dc3b314b0cce
--- /dev/null
+++ b/drivers/pwm/pwm-cadence-ttc.c
@@ -0,0 +1,261 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * (C) Copyright 2021 Xilinx, Inc. Michal Simek
+ */
+
+#define LOG_CATEGORY UCLASS_PWM
+
+#include <clk.h>
+#include <common.h>
+#include <div64.h>
+#include <dm.h>
+#include <log.h>
+#include <pwm.h>
+#include <asm/io.h>
+#include <log.h>
+#include <div64.h>
+#include <linux/bitfield.h>
+#include <linux/math64.h>
+#include <linux/log2.h>
+#include <dm/device_compat.h>
+
+#define CLOCK_CONTROL		0
+#define COUNTER_CONTROL		0xc
+#define INTERVAL_COUNTER	0x24
+#define MATCH_1_COUNTER		0x30
+
+#define CLK_FALLING_EDGE	BIT(6)
+#define CLK_SRC_EXTERNAL	BIT(5)
+#define CLK_PRESCALE_MASK	GENMASK(4, 1)
+#define CLK_PRESCALE_ENABLE	BIT(0)
+
+#define COUNTER_WAVE_POL		BIT(6)
+#define COUNTER_WAVE_DISABLE		BIT(5)
+#define COUNTER_RESET			BIT(4)
+#define COUNTER_MATCH_ENABLE		BIT(3)
+#define COUNTER_DECREMENT_ENABLE	BIT(2)
+#define COUNTER_INTERVAL_ENABLE		BIT(1)
+#define COUNTER_COUNTING_DISABLE	BIT(0)
+
+#define NSEC_PER_SEC	1000000000L
+
+#define TTC_REG(reg, channel) ((reg) + (channel) * sizeof(u32))
+#define TTC_CLOCK_CONTROL(reg, channel) \
+	TTC_REG((reg) + CLOCK_CONTROL, (channel))
+#define TTC_COUNTER_CONTROL(reg, channel) \
+	TTC_REG((reg) + COUNTER_CONTROL, (channel))
+#define TTC_INTERVAL_COUNTER(reg, channel) \
+	TTC_REG((reg) + INTERVAL_COUNTER, (channel))
+#define TTC_MATCH_1_COUNTER(reg, channel) \
+	TTC_REG((reg) + MATCH_1_COUNTER, (channel))
+
+struct cadence_ttc_pwm_plat {
+	u8 *regs;
+	u32 timer_width;
+};
+
+struct cadence_ttc_pwm_priv {
+	u8 *regs;
+	u32 timer_width;
+	u32 timer_mask;
+	unsigned long frequency;
+	bool invert[2];
+};
+
+static int cadence_ttc_pwm_set_invert(struct udevice *dev, uint channel,
+				      bool polarity)
+{
+	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
+
+	if (channel > 2) {
+		dev_err(dev, "Unsupported channel number %d(max 2)\n", channel);
+		return -EINVAL;
+	}
+
+	priv->invert[channel] = polarity;
+
+	dev_dbg(dev, "polarity=%u. Please config PWM again\n", polarity);
+
+	return 0;
+}
+
+static int cadence_ttc_pwm_set_config(struct udevice *dev, uint channel,
+				      uint period_ns, uint duty_ns)
+{
+	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
+	u32 counter_ctrl, clock_ctrl;
+	int period_clocks, duty_clocks, prescaler;
+
+	dev_dbg(dev, "channel %d, duty %d/period %d ns\n", channel,
+		duty_ns, period_ns);
+
+	if (channel > 2) {
+		dev_err(dev, "Unsupported channel number %d(max 2)\n", channel);
+		return -EINVAL;
+	}
+
+	/* Make sure counter is stopped */
+	counter_ctrl = readl(TTC_COUNTER_CONTROL(priv->regs, channel));
+	setbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
+		     COUNTER_COUNTING_DISABLE | COUNTER_WAVE_DISABLE);
+
+	/* Calculate period, prescaler and set clock control register */
+	period_clocks = div64_u64(((int64_t)period_ns * priv->frequency),
+				  NSEC_PER_SEC);
+
+	prescaler = ilog2(period_clocks) + 1 - priv->timer_width;
+	if (prescaler < 0)
+		prescaler = 0;
+
+	clock_ctrl = readl(TTC_CLOCK_CONTROL(priv->regs, channel));
+
+	if (!prescaler) {
+		clock_ctrl &= ~(CLK_PRESCALE_ENABLE | CLK_PRESCALE_MASK);
+	} else {
+		clock_ctrl &= ~CLK_PRESCALE_MASK;
+		clock_ctrl |= CLK_PRESCALE_ENABLE;
+		clock_ctrl |= FIELD_PREP(CLK_PRESCALE_MASK, prescaler - 1);
+	};
+
+	/* External source is not handled by this driver now */
+	clock_ctrl &= ~CLK_SRC_EXTERNAL;
+
+	writel(clock_ctrl, TTC_CLOCK_CONTROL(priv->regs, channel));
+
+	/* Calculate interval and set counter control value */
+	duty_clocks = div64_u64(((int64_t)duty_ns * priv->frequency),
+				NSEC_PER_SEC);
+
+	writel((period_clocks >> prescaler) & priv->timer_mask,
+	       TTC_INTERVAL_COUNTER(priv->regs, channel));
+	writel((duty_clocks >> prescaler) & priv->timer_mask,
+	       TTC_MATCH_1_COUNTER(priv->regs, channel));
+
+	/* Restore/reset counter */
+	counter_ctrl &= ~COUNTER_DECREMENT_ENABLE;
+	counter_ctrl |= COUNTER_INTERVAL_ENABLE |
+			COUNTER_RESET |
+			COUNTER_MATCH_ENABLE;
+
+	if (priv->invert[channel])
+		counter_ctrl |= COUNTER_WAVE_POL;
+	else
+		counter_ctrl &= ~COUNTER_WAVE_POL;
+
+	writel(counter_ctrl, TTC_COUNTER_CONTROL(priv->regs, channel));
+
+	dev_dbg(dev, "%d/%d clocks, prescaler 2^%d\n", duty_clocks,
+		period_clocks, prescaler);
+
+	return 0;
+};
+
+static int cadence_ttc_pwm_set_enable(struct udevice *dev, uint channel,
+				      bool enable)
+{
+	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
+
+	if (channel > 2) {
+		dev_err(dev, "Unsupported channel number %d(max 2)\n", channel);
+		return -EINVAL;
+	}
+
+	dev_dbg(dev, "Enable: %d, channel %d\n", enable, channel);
+
+	if (enable) {
+		clrbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
+			     COUNTER_COUNTING_DISABLE |
+			     COUNTER_WAVE_DISABLE);
+		setbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
+			     COUNTER_RESET);
+	} else {
+		setbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
+			     COUNTER_COUNTING_DISABLE |
+			     COUNTER_WAVE_DISABLE);
+	}
+
+	return 0;
+};
+
+static int cadence_ttc_pwm_probe(struct udevice *dev)
+{
+	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
+	struct cadence_ttc_pwm_plat *plat = dev_get_plat(dev);
+	struct clk clk;
+	int ret;
+
+	priv->regs = plat->regs;
+	priv->timer_width = plat->timer_width;
+	priv->timer_mask = GENMASK(priv->timer_width - 1, 0);
+
+	ret = clk_get_by_index(dev, 0, &clk);
+	if (ret < 0) {
+		dev_err(dev, "failed to get clock\n");
+		return ret;
+	}
+
+	priv->frequency = clk_get_rate(&clk);
+	if (IS_ERR_VALUE(priv->frequency)) {
+		dev_err(dev, "failed to get rate\n");
+		return priv->frequency;
+	}
+	dev_dbg(dev, "Clk frequency: %ld\n", priv->frequency);
+
+	ret = clk_enable(&clk);
+	if (ret) {
+		dev_err(dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cadence_ttc_pwm_of_to_plat(struct udevice *dev)
+{
+	struct cadence_ttc_pwm_plat *plat = dev_get_plat(dev);
+	const char *cells;
+
+	cells = dev_read_prop(dev, "#pwm-cells", NULL);
+	if (!cells)
+		return -EINVAL;
+
+	plat->regs = dev_read_addr_ptr(dev);
+
+	plat->timer_width = dev_read_u32_default(dev, "timer-width", 16);
+
+	return 0;
+}
+
+static int cadence_ttc_pwm_bind(struct udevice *dev)
+{
+	const char *cells;
+
+	cells = dev_read_prop(dev, "#pwm-cells", NULL);
+	if (!cells)
+		return -ENODEV;
+
+	return 0;
+}
+
+static const struct pwm_ops cadence_ttc_pwm_ops = {
+	.set_invert = cadence_ttc_pwm_set_invert,
+	.set_config = cadence_ttc_pwm_set_config,
+	.set_enable = cadence_ttc_pwm_set_enable,
+};
+
+static const struct udevice_id cadence_ttc_pwm_ids[] = {
+	{ .compatible = "cdns,ttc" },
+	{ }
+};
+
+U_BOOT_DRIVER(cadence_ttc_pwm) = {
+	.name = "cadence_ttc_pwm",
+	.id = UCLASS_PWM,
+	.of_match = cadence_ttc_pwm_ids,
+	.ops = &cadence_ttc_pwm_ops,
+	.bind = cadence_ttc_pwm_bind,
+	.of_to_plat = cadence_ttc_pwm_of_to_plat,
+	.probe = cadence_ttc_pwm_probe,
+	.priv_auto = sizeof(struct cadence_ttc_pwm_priv),
+	.plat_auto = sizeof(struct cadence_ttc_pwm_plat),
+};
-- 
2.33.0

Re: [PATCH v3 2/3] timer: cadence: Add bind function to driver

[Sean Anderson]

On 10/6/21 10:19 AM, Michal Simek wrote:
> When DT node has pwm-cells property it shouldn't be bind as timer driver
> but as PWM driver. That's why make sure that this property is checked.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v3:
> - New patch in series
> 
>   drivers/timer/cadence-ttc.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
> index 2f95d45ecd7a..2eff45060ad6 100644
> --- a/drivers/timer/cadence-ttc.c
> +++ b/drivers/timer/cadence-ttc.c
> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev)
>   	return 0;
>   }
>   
> +static int cadence_ttc_bind(struct udevice *dev)
> +{
> +	const char *cells;
> +
> +	cells = dev_read_prop(dev, "#pwm-cells", NULL);
> +	if (cells)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
>   static const struct timer_ops cadence_ttc_ops = {
>   	.get_count = cadence_ttc_get_count,
>   };
> @@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = {
>   	.priv_auto	= sizeof(struct cadence_ttc_priv),
>   	.probe = cadence_ttc_probe,
>   	.ops = &cadence_ttc_ops,
> +	.bind = cadence_ttc_bind,
>   };
> 

Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Re: [PATCH v3 3/3] pwm: Add driver for cadence TTC

[Sean Anderson]

On 10/6/21 10:19 AM, Michal Simek wrote:
> TTC has three modes of operations. Timer, PWM and input counters.
> 
> There is already driver for timer under CADENCE_TTC_TIMER which is used for
> ZynqMP R5 configuration.
> This driver is targeting PWM which is for example configuration which can
> be used for fan control.
> The driver has been tested on Xilinx Kria SOM platform where fan is
> connected to one PL pin. When TTC output is connected via EMIO to PL pin
> TTC pwm can be configured and tested for example like this:
> pwm config 0 0 10000 1200
> pwm enable 0 0
> pwm config 0 0 10000 1400
> pwm config 0 0 10000 1600
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
> Changes in v3:
> - Add bind function to check pwm-cells to recognize PWM driver
> - Use timer-width in prescaler calculation
> - Record timer width in platdata instead of timer mask
> - Also disable wave out in config function
> 
> Changes in v2:
> - Detect pwm-cells property for PWM driver
> - Fix all macro names
> - Use BIT and GENMASK macros
> - Introduce TTC_REG macro for reg offsets
> - Use FIELD_PREP
> - Move cadence_ttc_pwm_of_to_plat() below probe
> - Introduce struct cadence_ttc_pwm_plat
> - Read timer-width from DT
> - Use NSEC_PER_SEC macro
> - Use clock_ctrl variable instead of x - all reported by Sean
> 
>   MAINTAINERS                   |   1 +
>   drivers/pwm/Kconfig           |   7 +
>   drivers/pwm/Makefile          |   1 +
>   drivers/pwm/pwm-cadence-ttc.c | 261 ++++++++++++++++++++++++++++++++++
>   4 files changed, 270 insertions(+)
>   create mode 100644 drivers/pwm/pwm-cadence-ttc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 31b49c0a95f0..d0b1845384bf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -599,6 +599,7 @@ F:	drivers/mmc/zynq_sdhci.c
>   F:	drivers/mtd/nand/raw/zynq_nand.c
>   F:	drivers/net/phy/xilinx_phy.c
>   F:	drivers/net/zynq_gem.c
> +F:	drivers/pwm/pwm-cadence-ttc.c
>   F:	drivers/serial/serial_zynq.c
>   F:	drivers/reset/reset-zynqmp.c
>   F:	drivers/rtc/zynqmp_rtc.c
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index cf7f4c6840ce..176e703c8fbb 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -9,6 +9,13 @@ config DM_PWM
>   	  frequency/period can be controlled along with the proportion of that
>   	  time that the signal is high.
>   
> +config PWM_CADENCE_TTC
> +	bool "Enable support for the Cadence TTC PWM"
> +	depends on DM_PWM && !CADENCE_TTC_TIMER
> +	help
> +	  Cadence TTC can be configured as timer which is done via
> +	  CONFIG_CADENCE_TTC_TIMER or as PWM. This is covering only PWM now.
> +
>   config PWM_CROS_EC
>   	bool "Enable support for the Chrome OS EC PWM"
>   	depends on DM_PWM
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 10d244bfb79d..abf5af41d2cc 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -10,6 +10,7 @@
>   
>   obj-$(CONFIG_DM_PWM)		+= pwm-uclass.o
>   
> +obj-$(CONFIG_PWM_CADENCE_TTC)	+= pwm-cadence-ttc.o
>   obj-$(CONFIG_PWM_CROS_EC)	+= cros_ec_pwm.o
>   obj-$(CONFIG_PWM_EXYNOS)	+= exynos_pwm.o
>   obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o pwm-imx-util.o
> diff --git a/drivers/pwm/pwm-cadence-ttc.c b/drivers/pwm/pwm-cadence-ttc.c
> new file mode 100644
> index 000000000000..dc3b314b0cce
> --- /dev/null
> +++ b/drivers/pwm/pwm-cadence-ttc.c
> @@ -0,0 +1,261 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * (C) Copyright 2021 Xilinx, Inc. Michal Simek
> + */
> +
> +#define LOG_CATEGORY UCLASS_PWM
> +
> +#include <clk.h>
> +#include <common.h>
> +#include <div64.h>
> +#include <dm.h>
> +#include <log.h>
> +#include <pwm.h>
> +#include <asm/io.h>
> +#include <log.h>
> +#include <div64.h>
> +#include <linux/bitfield.h>
> +#include <linux/math64.h>
> +#include <linux/log2.h>
> +#include <dm/device_compat.h>
> +
> +#define CLOCK_CONTROL		0
> +#define COUNTER_CONTROL		0xc
> +#define INTERVAL_COUNTER	0x24
> +#define MATCH_1_COUNTER		0x30
> +
> +#define CLK_FALLING_EDGE	BIT(6)
> +#define CLK_SRC_EXTERNAL	BIT(5)
> +#define CLK_PRESCALE_MASK	GENMASK(4, 1)
> +#define CLK_PRESCALE_ENABLE	BIT(0)
> +
> +#define COUNTER_WAVE_POL		BIT(6)
> +#define COUNTER_WAVE_DISABLE		BIT(5)
> +#define COUNTER_RESET			BIT(4)
> +#define COUNTER_MATCH_ENABLE		BIT(3)
> +#define COUNTER_DECREMENT_ENABLE	BIT(2)
> +#define COUNTER_INTERVAL_ENABLE		BIT(1)
> +#define COUNTER_COUNTING_DISABLE	BIT(0)
> +
> +#define NSEC_PER_SEC	1000000000L
> +
> +#define TTC_REG(reg, channel) ((reg) + (channel) * sizeof(u32))
> +#define TTC_CLOCK_CONTROL(reg, channel) \
> +	TTC_REG((reg) + CLOCK_CONTROL, (channel))
> +#define TTC_COUNTER_CONTROL(reg, channel) \
> +	TTC_REG((reg) + COUNTER_CONTROL, (channel))
> +#define TTC_INTERVAL_COUNTER(reg, channel) \
> +	TTC_REG((reg) + INTERVAL_COUNTER, (channel))
> +#define TTC_MATCH_1_COUNTER(reg, channel) \
> +	TTC_REG((reg) + MATCH_1_COUNTER, (channel))
> +
> +struct cadence_ttc_pwm_plat {
> +	u8 *regs;
> +	u32 timer_width;
> +};
> +
> +struct cadence_ttc_pwm_priv {
> +	u8 *regs;
> +	u32 timer_width;
> +	u32 timer_mask;
> +	unsigned long frequency;
> +	bool invert[2];
> +};
> +
> +static int cadence_ttc_pwm_set_invert(struct udevice *dev, uint channel,
> +				      bool polarity)
> +{
> +	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
> +
> +	if (channel > 2) {
> +		dev_err(dev, "Unsupported channel number %d(max 2)\n", channel);
> +		return -EINVAL;
> +	}
> +
> +	priv->invert[channel] = polarity;
> +
> +	dev_dbg(dev, "polarity=%u. Please config PWM again\n", polarity);
> +
> +	return 0;
> +}
> +
> +static int cadence_ttc_pwm_set_config(struct udevice *dev, uint channel,
> +				      uint period_ns, uint duty_ns)
> +{
> +	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
> +	u32 counter_ctrl, clock_ctrl;
> +	int period_clocks, duty_clocks, prescaler;
> +
> +	dev_dbg(dev, "channel %d, duty %d/period %d ns\n", channel,
> +		duty_ns, period_ns);
> +
> +	if (channel > 2) {
> +		dev_err(dev, "Unsupported channel number %d(max 2)\n", channel);
> +		return -EINVAL;
> +	}
> +
> +	/* Make sure counter is stopped */
> +	counter_ctrl = readl(TTC_COUNTER_CONTROL(priv->regs, channel));
> +	setbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
> +		     COUNTER_COUNTING_DISABLE | COUNTER_WAVE_DISABLE);
> +
> +	/* Calculate period, prescaler and set clock control register */
> +	period_clocks = div64_u64(((int64_t)period_ns * priv->frequency),
> +				  NSEC_PER_SEC);
> +
> +	prescaler = ilog2(period_clocks) + 1 - priv->timer_width;
> +	if (prescaler < 0)
> +		prescaler = 0;
> +
> +	clock_ctrl = readl(TTC_CLOCK_CONTROL(priv->regs, channel));
> +
> +	if (!prescaler) {
> +		clock_ctrl &= ~(CLK_PRESCALE_ENABLE | CLK_PRESCALE_MASK);
> +	} else {
> +		clock_ctrl &= ~CLK_PRESCALE_MASK;
> +		clock_ctrl |= CLK_PRESCALE_ENABLE;
> +		clock_ctrl |= FIELD_PREP(CLK_PRESCALE_MASK, prescaler - 1);
> +	};
> +
> +	/* External source is not handled by this driver now */
> +	clock_ctrl &= ~CLK_SRC_EXTERNAL;
> +
> +	writel(clock_ctrl, TTC_CLOCK_CONTROL(priv->regs, channel));
> +
> +	/* Calculate interval and set counter control value */
> +	duty_clocks = div64_u64(((int64_t)duty_ns * priv->frequency),
> +				NSEC_PER_SEC);
> +
> +	writel((period_clocks >> prescaler) & priv->timer_mask,
> +	       TTC_INTERVAL_COUNTER(priv->regs, channel));
> +	writel((duty_clocks >> prescaler) & priv->timer_mask,
> +	       TTC_MATCH_1_COUNTER(priv->regs, channel));
> +
> +	/* Restore/reset counter */
> +	counter_ctrl &= ~COUNTER_DECREMENT_ENABLE;
> +	counter_ctrl |= COUNTER_INTERVAL_ENABLE |
> +			COUNTER_RESET |
> +			COUNTER_MATCH_ENABLE;
> +
> +	if (priv->invert[channel])
> +		counter_ctrl |= COUNTER_WAVE_POL;
> +	else
> +		counter_ctrl &= ~COUNTER_WAVE_POL;
> +
> +	writel(counter_ctrl, TTC_COUNTER_CONTROL(priv->regs, channel));
> +
> +	dev_dbg(dev, "%d/%d clocks, prescaler 2^%d\n", duty_clocks,
> +		period_clocks, prescaler);
> +
> +	return 0;
> +};
> +
> +static int cadence_ttc_pwm_set_enable(struct udevice *dev, uint channel,
> +				      bool enable)
> +{
> +	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
> +
> +	if (channel > 2) {
> +		dev_err(dev, "Unsupported channel number %d(max 2)\n", channel);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(dev, "Enable: %d, channel %d\n", enable, channel);
> +
> +	if (enable) {
> +		clrbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
> +			     COUNTER_COUNTING_DISABLE |
> +			     COUNTER_WAVE_DISABLE);
> +		setbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
> +			     COUNTER_RESET);
> +	} else {
> +		setbits_le32(TTC_COUNTER_CONTROL(priv->regs, channel),
> +			     COUNTER_COUNTING_DISABLE |
> +			     COUNTER_WAVE_DISABLE);
> +	}
> +
> +	return 0;
> +};
> +
> +static int cadence_ttc_pwm_probe(struct udevice *dev)
> +{
> +	struct cadence_ttc_pwm_priv *priv = dev_get_priv(dev);
> +	struct cadence_ttc_pwm_plat *plat = dev_get_plat(dev);
> +	struct clk clk;
> +	int ret;
> +
> +	priv->regs = plat->regs;
> +	priv->timer_width = plat->timer_width;
> +	priv->timer_mask = GENMASK(priv->timer_width - 1, 0);
> +
> +	ret = clk_get_by_index(dev, 0, &clk);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to get clock\n");
> +		return ret;
> +	}
> +
> +	priv->frequency = clk_get_rate(&clk);
> +	if (IS_ERR_VALUE(priv->frequency)) {
> +		dev_err(dev, "failed to get rate\n");
> +		return priv->frequency;
> +	}
> +	dev_dbg(dev, "Clk frequency: %ld\n", priv->frequency);
> +
> +	ret = clk_enable(&clk);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cadence_ttc_pwm_of_to_plat(struct udevice *dev)
> +{
> +	struct cadence_ttc_pwm_plat *plat = dev_get_plat(dev);
> +	const char *cells;
> +
> +	cells = dev_read_prop(dev, "#pwm-cells", NULL);
> +	if (!cells)
> +		return -EINVAL;
> +
> +	plat->regs = dev_read_addr_ptr(dev);
> +
> +	plat->timer_width = dev_read_u32_default(dev, "timer-width", 16);
> +
> +	return 0;
> +}
> +
> +static int cadence_ttc_pwm_bind(struct udevice *dev)
> +{
> +	const char *cells;
> +
> +	cells = dev_read_prop(dev, "#pwm-cells", NULL);
> +	if (!cells)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops cadence_ttc_pwm_ops = {
> +	.set_invert = cadence_ttc_pwm_set_invert,
> +	.set_config = cadence_ttc_pwm_set_config,
> +	.set_enable = cadence_ttc_pwm_set_enable,
> +};
> +
> +static const struct udevice_id cadence_ttc_pwm_ids[] = {
> +	{ .compatible = "cdns,ttc" },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(cadence_ttc_pwm) = {
> +	.name = "cadence_ttc_pwm",
> +	.id = UCLASS_PWM,
> +	.of_match = cadence_ttc_pwm_ids,
> +	.ops = &cadence_ttc_pwm_ops,
> +	.bind = cadence_ttc_pwm_bind,
> +	.of_to_plat = cadence_ttc_pwm_of_to_plat,
> +	.probe = cadence_ttc_pwm_probe,
> +	.priv_auto = sizeof(struct cadence_ttc_pwm_priv),
> +	.plat_auto = sizeof(struct cadence_ttc_pwm_plat),
> +};
> 

Reviewed-by: Sean Anderson <sean.anderson@seco.com>

Re: [PATCH v3 1/3] dm: core: Bind another driver with the same compatible string

[Simon Glass]

Hi Michal,

On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote:
>
> When one IP can have multiple configurations (like timer and PWM) covered
> by multiple drivers. Because it is the same IP it should also have the same
> compatible string.
> Current code look for the first driver which matches compatible string and
> call bind function. If this is not the right driver which is express by
> returning -ENODEV ("Driver XYZ refuses to bind") there is no reason not to
> continue over compatible list to try to find out different driver with the
> same compatible string which match it.
>
> When the first compatible string is found core looks for driver bind()
> function. If if assigned driver refuse to bind, core continue in a loop to
> check if there is another driver which match compatible string.
>
> This driver is going to be used with cdnc,ttc driver which has driver as
> timer and also can be used as PWM. The difference is done via #pwm-cells
> property in DT.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v3:
> - New patch in series
>
>  drivers/core/lists.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Can you refactor this to avoid the 'goto'? E.g. put the loop in a new function?

Also, if pre_reloc_only, it still quits. Don't you want it to continue
in that case too?

Finally, can you add such a driver to sandbox test.dts and add a check
in test/dm/core.c or similar so that this behaviour is tested?

>
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index 350b9d32687c..199ee3a8e0fc 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -221,12 +221,14 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>                 compat = compat_list + i;
>                 log_debug("   - attempt to match compatible string '%s'\n",
>                           compat);
> -
> -               for (entry = driver; entry != driver + n_ents; entry++) {
> +               entry = driver;
> +next:
> +               for (; entry != driver + n_ents; entry++) {
>                         ret = driver_check_compatible(entry->of_match, &id,
>                                                       compat);
>                         if (!ret)
>                                 break;
> +
>                 }
>                 if (entry == driver + n_ents)
>                         continue;
> @@ -246,7 +248,8 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>                                                    id->data, node, &dev);
>                 if (ret == -ENODEV) {
>                         log_debug("Driver '%s' refuses to bind\n", entry->name);
> -                       continue;
> +                       entry++;
> +                       goto next;
>                 }
>                 if (ret) {
>                         dm_warn("Error binding driver '%s': %d\n", entry->name,
> --
> 2.33.0
>

Regards,
Simon

Re: [PATCH v3 2/3] timer: cadence: Add bind function to driver

[Simon Glass]

Hi Michal,

On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote:
>
> When DT node has pwm-cells property it shouldn't be bind as timer driver
> but as PWM driver. That's why make sure that this property is checked.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> Changes in v3:
> - New patch in series
>
>  drivers/timer/cadence-ttc.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>

Why not have two compatible strings?

> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
> index 2f95d45ecd7a..2eff45060ad6 100644
> --- a/drivers/timer/cadence-ttc.c
> +++ b/drivers/timer/cadence-ttc.c
> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev)
>         return 0;
>  }
>
> +static int cadence_ttc_bind(struct udevice *dev)
> +{
> +       const char *cells;
> +
> +       cells = dev_read_prop(dev, "#pwm-cells", NULL);
> +       if (cells)
> +               return -ENODEV;

We can read properties in bind() when necessary, but this is very
strange...it seems like the bindings are messed up?

> +
> +       return 0;
> +}
> +
>  static const struct timer_ops cadence_ttc_ops = {
>         .get_count = cadence_ttc_get_count,
>  };
> @@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = {
>         .priv_auto      = sizeof(struct cadence_ttc_priv),
>         .probe = cadence_ttc_probe,
>         .ops = &cadence_ttc_ops,
> +       .bind = cadence_ttc_bind,
>  };
> --
> 2.33.0
>

Regards,
Simon

Re: [PATCH v3 2/3] timer: cadence: Add bind function to driver

[Sean Anderson]

On 10/14/21 11:09 AM, Simon Glass wrote:
> Hi Michal,
>
> On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> When DT node has pwm-cells property it shouldn't be bind as timer driver
>> but as PWM driver. That's why make sure that this property is checked.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Changes in v3:
>> - New patch in series
>>
>>  drivers/timer/cadence-ttc.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>
> Why not have two compatible strings?

As I understand it, because the hardware is the same, it should have the
same compatible string.

>> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
>> index 2f95d45ecd7a..2eff45060ad6 100644
>> --- a/drivers/timer/cadence-ttc.c
>> +++ b/drivers/timer/cadence-ttc.c
>> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev)
>>         return 0;
>>  }
>>
>> +static int cadence_ttc_bind(struct udevice *dev)
>> +{
>> +       const char *cells;
>> +
>> +       cells = dev_read_prop(dev, "#pwm-cells", NULL);
>> +       if (cells)
>> +               return -ENODEV;
>
> We can read properties in bind() when necessary, but this is very
> strange...it seems like the bindings are messed up?

This is the preferred way to select between a PWM and a timer driver.
See e.g. Rob's comment for xlnx,pwm at [1].

--Sean

[1] https://lore.kernel.org/linux-pwm/20210513021631.GA878860@robh.at.kernel.org/

>> +
>> +       return 0;
>> +}
>> +
>>  static const struct timer_ops cadence_ttc_ops = {
>>         .get_count = cadence_ttc_get_count,
>>  };
>> @@ -114,4 +125,5 @@ U_BOOT_DRIVER(cadence_ttc) = {
>>         .priv_auto      = sizeof(struct cadence_ttc_priv),
>>         .probe = cadence_ttc_probe,
>>         .ops = &cadence_ttc_ops,
>> +       .bind = cadence_ttc_bind,
>>  };
>> --
>> 2.33.0
>>
>
> Regards,
> Simon
>

Re: [PATCH v3 2/3] timer: cadence: Add bind function to driver

[Simon Glass]

Hi Sean,

On Thu, 14 Oct 2021 at 09:36, Sean Anderson <sean.anderson@seco.com> wrote:
>
>
>
> On 10/14/21 11:09 AM, Simon Glass wrote:
> > Hi Michal,
> >
> > On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote:
> >>
> >> When DT node has pwm-cells property it shouldn't be bind as timer driver
> >> but as PWM driver. That's why make sure that this property is checked.
> >>
> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> >> ---
> >>
> >> Changes in v3:
> >> - New patch in series
> >>
> >>  drivers/timer/cadence-ttc.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >
> > Why not have two compatible strings?
>
> As I understand it, because the hardware is the same, it should have the
> same compatible string.
>
> >> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
> >> index 2f95d45ecd7a..2eff45060ad6 100644
> >> --- a/drivers/timer/cadence-ttc.c
> >> +++ b/drivers/timer/cadence-ttc.c
> >> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev)
> >>         return 0;
> >>  }
> >>
> >> +static int cadence_ttc_bind(struct udevice *dev)
> >> +{
> >> +       const char *cells;
> >> +
> >> +       cells = dev_read_prop(dev, "#pwm-cells", NULL);
> >> +       if (cells)
> >> +               return -ENODEV;
> >
> > We can read properties in bind() when necessary, but this is very
> > strange...it seems like the bindings are messed up?
>
> This is the preferred way to select between a PWM and a timer driver.
> See e.g. Rob's comment for xlnx,pwm at [1].

I actually don't understand his comment. Is it the 'No...' one?

Anyway, it seems you understand what you are doing, and this is
supposed to be supported by driver model.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH v3 2/3] timer: cadence: Add bind function to driver

[Sean Anderson]

On 10/14/21 2:24 PM, Simon Glass wrote:
> Hi Sean,
>
> On Thu, 14 Oct 2021 at 09:36, Sean Anderson <sean.anderson@seco.com> wrote:
>>
>>
>>
>> On 10/14/21 11:09 AM, Simon Glass wrote:
>> > Hi Michal,
>> >
>> > On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote:
>> >>
>> >> When DT node has pwm-cells property it shouldn't be bind as timer driver
>> >> but as PWM driver. That's why make sure that this property is checked.
>> >>
>> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> >> ---
>> >>
>> >> Changes in v3:
>> >> - New patch in series
>> >>
>> >>  drivers/timer/cadence-ttc.c | 12 ++++++++++++
>> >>  1 file changed, 12 insertions(+)
>> >>
>> >
>> > Why not have two compatible strings?
>>
>> As I understand it, because the hardware is the same, it should have the
>> same compatible string.
>>
>> >> diff --git a/drivers/timer/cadence-ttc.c b/drivers/timer/cadence-ttc.c
>> >> index 2f95d45ecd7a..2eff45060ad6 100644
>> >> --- a/drivers/timer/cadence-ttc.c
>> >> +++ b/drivers/timer/cadence-ttc.c
>> >> @@ -97,6 +97,17 @@ static int cadence_ttc_of_to_plat(struct udevice *dev)
>> >>         return 0;
>> >>  }
>> >>
>> >> +static int cadence_ttc_bind(struct udevice *dev)
>> >> +{
>> >> +       const char *cells;
>> >> +
>> >> +       cells = dev_read_prop(dev, "#pwm-cells", NULL);
>> >> +       if (cells)
>> >> +               return -ENODEV;
>> >
>> > We can read properties in bind() when necessary, but this is very
>> > strange...it seems like the bindings are messed up?
>>
>> This is the preferred way to select between a PWM and a timer driver.
>> See e.g. Rob's comment for xlnx,pwm at [1].
>
> I actually don't understand his comment. Is it the 'No...' one?

No, it's the

> If a PWM, perhaps you want a '#pwm-cells' property which can serve as
> the hint to configure as a PWM.

--Sean

>
> Anyway, it seems you understand what you are doing, and this is
> supposed to be supported by driver model.
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Regards,
> Simon
>

Re: [PATCH v3 1/3] dm: core: Bind another driver with the same compatible string

[Michal Simek]

On 10/14/21 17:09, Simon Glass wrote:
> Hi Michal,
> 
> On Wed, 6 Oct 2021 at 08:19, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> When one IP can have multiple configurations (like timer and PWM) covered
>> by multiple drivers. Because it is the same IP it should also have the same
>> compatible string.
>> Current code look for the first driver which matches compatible string and
>> call bind function. If this is not the right driver which is express by
>> returning -ENODEV ("Driver XYZ refuses to bind") there is no reason not to
>> continue over compatible list to try to find out different driver with the
>> same compatible string which match it.
>>
>> When the first compatible string is found core looks for driver bind()
>> function. If if assigned driver refuse to bind, core continue in a loop to
>> check if there is another driver which match compatible string.
>>
>> This driver is going to be used with cdnc,ttc driver which has driver as
>> timer and also can be used as PWM. The difference is done via #pwm-cells
>> property in DT.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> Changes in v3:
>> - New patch in series
>>
>>   drivers/core/lists.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> Can you refactor this to avoid the 'goto'? E.g. put the loop in a new function?

Sent v4 with adding one more loop. There are so many parameters use that 
new function won't really help.

> 
> Also, if pre_reloc_only, it still quits. Don't you want it to continue
> in that case too?

Also fixed in v4.

> 
> Finally, can you add such a driver to sandbox test.dts and add a check
> in test/dm/core.c or similar so that this behaviour is tested?

When the patch is acked I will take a look at writing test case for it.
I have tested it with pwm driver to see proper behavior.

Thanks,
Michal