[PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible

[PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible

[Baruch Siach]

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 Documentation/devicetree/bindings/mfd/qcom,tcsr.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
index e90519d566a3..c5f4f0ddfcc3 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
@@ -6,6 +6,7 @@ registers via syscon.
 
 Required properties:
 - compatible:	Should contain:
+		"qcom,tcsr-ipq6018", "syscon", "simple-mfd" for IPQ6018
 		"qcom,tcsr-ipq8064", "syscon" for IPQ8064
 		"qcom,tcsr-apq8064", "syscon" for APQ8064
 		"qcom,tcsr-msm8660", "syscon" for MSM8660
-- 
2.33.0

[PATCH v8 1/4] dt-bindings: mfd: qcom, tcsr: document ipq6018 compatible

[Baruch Siach]

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 Documentation/devicetree/bindings/mfd/qcom,tcsr.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
index e90519d566a3..c5f4f0ddfcc3 100644
--- a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
+++ b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
@@ -6,6 +6,7 @@ registers via syscon.
 
 Required properties:
 - compatible:	Should contain:
+		"qcom,tcsr-ipq6018", "syscon", "simple-mfd" for IPQ6018
 		"qcom,tcsr-ipq8064", "syscon" for IPQ8064
 		"qcom,tcsr-apq8064", "syscon" for APQ8064
 		"qcom,tcsr-msm8660", "syscon" for MSM8660
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block

[Baruch Siach]

Driver for the PWM block in Qualcomm IPQ6018 line of SoCs. Based on
driver from downstream Codeaurora kernel tree. Removed support for older
(V1) variants because I have no access to that hardware.

Tested on IPQ6010 based hardware.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v7:

  Change 'offset' to 'reg' for the tcsr offset (Rob)

  Drop clock name; there is only one clock (Bjorn)

  Simplify probe failure code path (Bjorn)

v6:

Address Uwe Kleine-König review comments:

  Drop IPQ_PWM_MAX_DEVICES

  Rely on assigned-clock-rates; drop IPQ_PWM_CLK_SRC_FREQ

  Simplify register offset calculation

  Calculate duty cycle more precisely

  Refuse to set inverted polarity

  Drop redundant IPQ_PWM_REG1_ENABLE bit clear

  Remove x1000 factor in pwm_div calculation, use rate directly, and round up

  Choose initial pre_div such that pwm_div < IPQ_PWM_MAX_DIV

  Ensure pre_div <= pwm_div

  Rename close_ to best_

  Explain in comment why effective_div doesn't overflow

  Limit pwm_div to IPQ_PWM_MAX_DIV - 1 to allow 100% duty cycle

  Disable clock only after pwmchip_remove()

  const pwm_ops

Other changes:

  Add missing linux/bitfield.h header include (kernel test robot)

  Adjust code for PWM device node under TCSR (Rob Herring)

v5:

Use &tcsr_q6 syscon to access registers (Bjorn Andersson)

Address Uwe Kleine-König review comments:

  Implement .get_state()

  Add IPQ_PWM_ prefix to local macros

  Use GENMASK/BIT/FIELD_PREP for register fields access

  Make type of config_div_and_duty() parameters consistent

  Derive IPQ_PWM_MIN_PERIOD_NS from IPQ_PWM_CLK_SRC_FREQ

  Integrate enable/disable into config_div_and_duty() to save register read,
  and reduce frequency glitch on update

  Use min() instead of min_t()

  Fix comment format

  Use dev_err_probe() to indicate probe step failure

  Add missing clk_disable_unprepare() in .remove

  Don't set .owner

v4:

  Use div64_u64() to fix link for 32-bit targets ((kernel test robot
  <lkp@intel.com>, Uwe Kleine-König)

v3:

  s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)

  Fix integer overflow on 32-bit targets (kernel test robot <lkp@intel.com>)

v2:

Address Uwe Kleine-König review comments:

  Fix period calculation when out of range

  Don't set period larger than requested

  Remove PWM disable on configuration change

  Implement .apply instead of non-atomic .config/.enable/.disable

  Don't modify PWM on .request/.free

  Check pwm_div underflow

  Fix various code and comment formatting issues

Other changes:

  Use u64 divisor safe division

  Remove now empty .request/.free
---
 drivers/pwm/Kconfig   |  12 ++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-ipq.c | 275 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/pwm/pwm-ipq.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c76adedd58c9..08add845596f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -260,6 +260,18 @@ config PWM_INTEL_LGM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-intel-lgm.
 
+config PWM_IPQ
+	tristate "IPQ PWM support"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on HAVE_CLK && HAS_IOMEM
+	help
+	  Generic PWM framework driver for IPQ PWM block which supports
+	  4 pwm channels. Each of the these channels can be configured
+	  independent of each other.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ipq.
+
 config PWM_IQS620A
 	tristate "Azoteq IQS620A PWM support"
 	depends on MFD_IQS62X || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 708840b7fba8..7402feae4b36 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
 obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
 obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
 obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
+obj-$(CONFIG_PWM_IPQ)		+= pwm-ipq.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
new file mode 100644
index 000000000000..8405d0554951
--- /dev/null
+++ b/drivers/pwm/pwm-ipq.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/math64.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/bitfield.h>
+
+/* The frequency range supported is 1 Hz to clock rate */
+#define IPQ_PWM_MAX_PERIOD_NS	((u64)NSEC_PER_SEC)
+
+/*
+ * The max value specified for each field is based on the number of bits
+ * in the pwm control register for that field
+ */
+#define IPQ_PWM_MAX_DIV		0xFFFF
+
+/*
+ * Two 32-bit registers for each PWM: REG0, and REG1.
+ * Base offset for PWM #i is at 8 * #i.
+ */
+#define IPQ_PWM_CFG_REG0 0 /*PWM_DIV PWM_HI*/
+#define IPQ_PWM_REG0_PWM_DIV		GENMASK(15, 0)
+#define IPQ_PWM_REG0_HI_DURATION	GENMASK(31, 16)
+
+#define IPQ_PWM_CFG_REG1 4 /*ENABLE UPDATE PWM_PRE_DIV*/
+#define IPQ_PWM_REG1_PRE_DIV		GENMASK(15, 0)
+/*
+ * Enable bit is set to enable output toggling in pwm device.
+ * Update bit is set to reflect the changed divider and high duration
+ * values in register.
+ */
+#define IPQ_PWM_REG1_UPDATE		BIT(30)
+#define IPQ_PWM_REG1_ENABLE		BIT(31)
+
+
+struct ipq_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	struct regmap *regmap;
+	u32 regmap_off;
+};
+
+static struct ipq_pwm_chip *to_ipq_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ipq_pwm_chip, chip);
+}
+
+static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned reg)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
+	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
+	unsigned int val;
+
+	regmap_read(ipq_chip->regmap, off, &val);
+
+	return val;
+}
+
+static void ipq_pwm_reg_write(struct pwm_device *pwm, unsigned reg,
+		unsigned val)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
+	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
+
+	regmap_write(ipq_chip->regmap, off, val);
+}
+
+static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
+			unsigned int pwm_div, unsigned long rate, u64 duty_ns,
+			bool enable)
+{
+	unsigned long hi_dur;
+	unsigned long val = 0;
+
+	/*
+	 * high duration = pwm duty * (pwm div + 1)
+	 * pwm duty = duty_ns / period_ns
+	 */
+	hi_dur = div64_u64(duty_ns * rate, (pre_div + 1) * NSEC_PER_SEC);
+
+	val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
+		FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
+	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val);
+
+	val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
+	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
+
+	/* Enable needs a separate write to REG1 */
+	val |= IPQ_PWM_REG1_UPDATE;
+	if (enable)
+		val |= IPQ_PWM_REG1_ENABLE;
+	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
+}
+
+static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
+	unsigned long freq;
+	unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div;
+	long long diff;
+	unsigned long rate = clk_get_rate(ipq_chip->clk);
+	unsigned long min_diff = rate;
+	u64 period_ns, duty_ns;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (state->period < div64_u64(NSEC_PER_SEC, rate))
+		return -ERANGE;
+
+	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
+	duty_ns = min(state->duty_cycle, period_ns);
+
+	/* freq in Hz for period in nano second */
+	freq = div64_u64(NSEC_PER_SEC, period_ns);
+	best_pre_div = IPQ_PWM_MAX_DIV;
+	best_pwm_div = IPQ_PWM_MAX_DIV;
+	/* Initial pre_div value such that pwm_div < IPQ_PWM_MAX_DIV */
+	pre_div = DIV64_U64_ROUND_UP(period_ns * rate,
+			(u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));
+
+	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
+		pwm_div = DIV64_U64_ROUND_UP(period_ns * rate,
+				(u64)NSEC_PER_SEC * (pre_div + 1));
+		pwm_div--;
+
+		if (pre_div > pwm_div)
+			break;
+
+		/*
+		 * Make sure we can do 100% duty cycle where
+		 * hi_dur == pwm_div + 1
+		 */
+		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
+			continue;
+
+		diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
+			- (uint64_t)rate;
+
+		if (diff < 0) /* period larger than requested */
+			continue;
+		if (diff == 0) { /* bingo */
+			best_pre_div = pre_div;
+			best_pwm_div = pwm_div;
+			break;
+		}
+		if (diff < min_diff) {
+			min_diff = diff;
+			best_pre_div = pre_div;
+			best_pwm_div = pwm_div;
+		}
+	}
+
+	/* config divider values for the closest possible frequency */
+	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
+			    rate, duty_ns, state->enabled);
+
+	return 0;
+}
+
+static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
+	unsigned long rate = clk_get_rate(ipq_chip->clk);
+	unsigned int pre_div, pwm_div, hi_dur;
+	u64 effective_div, hi_div;
+	u32 reg0, reg1;
+
+	reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0);
+	reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1);
+
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
+
+	pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
+	hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
+	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
+
+	/* No overflow here, both pre_div and pwm_div <= 0xffff */
+	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
+	state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
+
+	hi_div = hi_dur * (pre_div + 1);
+	state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
+}
+
+static const struct pwm_ops ipq_pwm_ops = {
+	.apply = ipq_pwm_apply,
+	.get_state = ipq_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int ipq_pwm_probe(struct platform_device *pdev)
+{
+	struct ipq_pwm_chip *pwm;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pwm);
+
+	pwm->regmap = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(pwm->regmap))
+		return dev_err_probe(dev, PTR_ERR(pwm->regmap),
+				"regs map failed");
+
+	ret = of_property_read_u32(dev->of_node, "reg", &pwm->regmap_off);
+	if (ret)
+		return dev_err_probe(dev, ret, "error reading 'reg'");
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk),
+				"failed to get clock");
+
+	ret = clk_prepare_enable(pwm->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "clock enable failed");
+
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &ipq_pwm_ops;
+	pwm->chip.npwm = 4;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "pwmchip_add() failed\n");
+		clk_disable_unprepare(pwm->clk);
+	}
+
+	return ret;
+}
+
+static int ipq_pwm_remove(struct platform_device *pdev)
+{
+	struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&pwm->chip);
+	clk_disable_unprepare(pwm->clk);
+
+	return 0;
+}
+
+static const struct of_device_id pwm_ipq_dt_match[] = {
+	{ .compatible = "qcom,ipq6018-pwm", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pwm_ipq_dt_match);
+
+static struct platform_driver ipq_pwm_driver = {
+	.driver = {
+		.name = "ipq-pwm",
+		.of_match_table = pwm_ipq_dt_match,
+	},
+	.probe = ipq_pwm_probe,
+	.remove = ipq_pwm_remove,
+};
+
+module_platform_driver(ipq_pwm_driver);
+
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.33.0

[PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block

[Baruch Siach]

Driver for the PWM block in Qualcomm IPQ6018 line of SoCs. Based on
driver from downstream Codeaurora kernel tree. Removed support for older
(V1) variants because I have no access to that hardware.

Tested on IPQ6010 based hardware.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v7:

  Change 'offset' to 'reg' for the tcsr offset (Rob)

  Drop clock name; there is only one clock (Bjorn)

  Simplify probe failure code path (Bjorn)

v6:

Address Uwe Kleine-König review comments:

  Drop IPQ_PWM_MAX_DEVICES

  Rely on assigned-clock-rates; drop IPQ_PWM_CLK_SRC_FREQ

  Simplify register offset calculation

  Calculate duty cycle more precisely

  Refuse to set inverted polarity

  Drop redundant IPQ_PWM_REG1_ENABLE bit clear

  Remove x1000 factor in pwm_div calculation, use rate directly, and round up

  Choose initial pre_div such that pwm_div < IPQ_PWM_MAX_DIV

  Ensure pre_div <= pwm_div

  Rename close_ to best_

  Explain in comment why effective_div doesn't overflow

  Limit pwm_div to IPQ_PWM_MAX_DIV - 1 to allow 100% duty cycle

  Disable clock only after pwmchip_remove()

  const pwm_ops

Other changes:

  Add missing linux/bitfield.h header include (kernel test robot)

  Adjust code for PWM device node under TCSR (Rob Herring)

v5:

Use &tcsr_q6 syscon to access registers (Bjorn Andersson)

Address Uwe Kleine-König review comments:

  Implement .get_state()

  Add IPQ_PWM_ prefix to local macros

  Use GENMASK/BIT/FIELD_PREP for register fields access

  Make type of config_div_and_duty() parameters consistent

  Derive IPQ_PWM_MIN_PERIOD_NS from IPQ_PWM_CLK_SRC_FREQ

  Integrate enable/disable into config_div_and_duty() to save register read,
  and reduce frequency glitch on update

  Use min() instead of min_t()

  Fix comment format

  Use dev_err_probe() to indicate probe step failure

  Add missing clk_disable_unprepare() in .remove

  Don't set .owner

v4:

  Use div64_u64() to fix link for 32-bit targets ((kernel test robot
  <lkp@intel.com>, Uwe Kleine-König)

v3:

  s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)

  Fix integer overflow on 32-bit targets (kernel test robot <lkp@intel.com>)

v2:

Address Uwe Kleine-König review comments:

  Fix period calculation when out of range

  Don't set period larger than requested

  Remove PWM disable on configuration change

  Implement .apply instead of non-atomic .config/.enable/.disable

  Don't modify PWM on .request/.free

  Check pwm_div underflow

  Fix various code and comment formatting issues

Other changes:

  Use u64 divisor safe division

  Remove now empty .request/.free
---
 drivers/pwm/Kconfig   |  12 ++
 drivers/pwm/Makefile  |   1 +
 drivers/pwm/pwm-ipq.c | 275 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 288 insertions(+)
 create mode 100644 drivers/pwm/pwm-ipq.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c76adedd58c9..08add845596f 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -260,6 +260,18 @@ config PWM_INTEL_LGM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-intel-lgm.
 
+config PWM_IPQ
+	tristate "IPQ PWM support"
+	depends on ARCH_QCOM || COMPILE_TEST
+	depends on HAVE_CLK && HAS_IOMEM
+	help
+	  Generic PWM framework driver for IPQ PWM block which supports
+	  4 pwm channels. Each of the these channels can be configured
+	  independent of each other.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ipq.
+
 config PWM_IQS620A
 	tristate "Azoteq IQS620A PWM support"
 	depends on MFD_IQS62X || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 708840b7fba8..7402feae4b36 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
 obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
 obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
 obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
+obj-$(CONFIG_PWM_IPQ)		+= pwm-ipq.o
 obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
new file mode 100644
index 000000000000..8405d0554951
--- /dev/null
+++ b/drivers/pwm/pwm-ipq.c
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
+/*
+ * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/math64.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/bitfield.h>
+
+/* The frequency range supported is 1 Hz to clock rate */
+#define IPQ_PWM_MAX_PERIOD_NS	((u64)NSEC_PER_SEC)
+
+/*
+ * The max value specified for each field is based on the number of bits
+ * in the pwm control register for that field
+ */
+#define IPQ_PWM_MAX_DIV		0xFFFF
+
+/*
+ * Two 32-bit registers for each PWM: REG0, and REG1.
+ * Base offset for PWM #i is at 8 * #i.
+ */
+#define IPQ_PWM_CFG_REG0 0 /*PWM_DIV PWM_HI*/
+#define IPQ_PWM_REG0_PWM_DIV		GENMASK(15, 0)
+#define IPQ_PWM_REG0_HI_DURATION	GENMASK(31, 16)
+
+#define IPQ_PWM_CFG_REG1 4 /*ENABLE UPDATE PWM_PRE_DIV*/
+#define IPQ_PWM_REG1_PRE_DIV		GENMASK(15, 0)
+/*
+ * Enable bit is set to enable output toggling in pwm device.
+ * Update bit is set to reflect the changed divider and high duration
+ * values in register.
+ */
+#define IPQ_PWM_REG1_UPDATE		BIT(30)
+#define IPQ_PWM_REG1_ENABLE		BIT(31)
+
+
+struct ipq_pwm_chip {
+	struct pwm_chip chip;
+	struct clk *clk;
+	struct regmap *regmap;
+	u32 regmap_off;
+};
+
+static struct ipq_pwm_chip *to_ipq_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ipq_pwm_chip, chip);
+}
+
+static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned reg)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
+	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
+	unsigned int val;
+
+	regmap_read(ipq_chip->regmap, off, &val);
+
+	return val;
+}
+
+static void ipq_pwm_reg_write(struct pwm_device *pwm, unsigned reg,
+		unsigned val)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
+	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
+
+	regmap_write(ipq_chip->regmap, off, val);
+}
+
+static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
+			unsigned int pwm_div, unsigned long rate, u64 duty_ns,
+			bool enable)
+{
+	unsigned long hi_dur;
+	unsigned long val = 0;
+
+	/*
+	 * high duration = pwm duty * (pwm div + 1)
+	 * pwm duty = duty_ns / period_ns
+	 */
+	hi_dur = div64_u64(duty_ns * rate, (pre_div + 1) * NSEC_PER_SEC);
+
+	val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
+		FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
+	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val);
+
+	val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
+	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
+
+	/* Enable needs a separate write to REG1 */
+	val |= IPQ_PWM_REG1_UPDATE;
+	if (enable)
+		val |= IPQ_PWM_REG1_ENABLE;
+	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
+}
+
+static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
+	unsigned long freq;
+	unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div;
+	long long diff;
+	unsigned long rate = clk_get_rate(ipq_chip->clk);
+	unsigned long min_diff = rate;
+	u64 period_ns, duty_ns;
+
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
+
+	if (state->period < div64_u64(NSEC_PER_SEC, rate))
+		return -ERANGE;
+
+	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
+	duty_ns = min(state->duty_cycle, period_ns);
+
+	/* freq in Hz for period in nano second */
+	freq = div64_u64(NSEC_PER_SEC, period_ns);
+	best_pre_div = IPQ_PWM_MAX_DIV;
+	best_pwm_div = IPQ_PWM_MAX_DIV;
+	/* Initial pre_div value such that pwm_div < IPQ_PWM_MAX_DIV */
+	pre_div = DIV64_U64_ROUND_UP(period_ns * rate,
+			(u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));
+
+	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
+		pwm_div = DIV64_U64_ROUND_UP(period_ns * rate,
+				(u64)NSEC_PER_SEC * (pre_div + 1));
+		pwm_div--;
+
+		if (pre_div > pwm_div)
+			break;
+
+		/*
+		 * Make sure we can do 100% duty cycle where
+		 * hi_dur == pwm_div + 1
+		 */
+		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
+			continue;
+
+		diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
+			- (uint64_t)rate;
+
+		if (diff < 0) /* period larger than requested */
+			continue;
+		if (diff == 0) { /* bingo */
+			best_pre_div = pre_div;
+			best_pwm_div = pwm_div;
+			break;
+		}
+		if (diff < min_diff) {
+			min_diff = diff;
+			best_pre_div = pre_div;
+			best_pwm_div = pwm_div;
+		}
+	}
+
+	/* config divider values for the closest possible frequency */
+	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
+			    rate, duty_ns, state->enabled);
+
+	return 0;
+}
+
+static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
+	unsigned long rate = clk_get_rate(ipq_chip->clk);
+	unsigned int pre_div, pwm_div, hi_dur;
+	u64 effective_div, hi_div;
+	u32 reg0, reg1;
+
+	reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0);
+	reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1);
+
+	state->polarity = PWM_POLARITY_NORMAL;
+	state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
+
+	pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
+	hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
+	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
+
+	/* No overflow here, both pre_div and pwm_div <= 0xffff */
+	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
+	state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
+
+	hi_div = hi_dur * (pre_div + 1);
+	state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
+}
+
+static const struct pwm_ops ipq_pwm_ops = {
+	.apply = ipq_pwm_apply,
+	.get_state = ipq_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int ipq_pwm_probe(struct platform_device *pdev)
+{
+	struct ipq_pwm_chip *pwm;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, pwm);
+
+	pwm->regmap = syscon_node_to_regmap(dev->parent->of_node);
+	if (IS_ERR(pwm->regmap))
+		return dev_err_probe(dev, PTR_ERR(pwm->regmap),
+				"regs map failed");
+
+	ret = of_property_read_u32(dev->of_node, "reg", &pwm->regmap_off);
+	if (ret)
+		return dev_err_probe(dev, ret, "error reading 'reg'");
+
+	pwm->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk),
+				"failed to get clock");
+
+	ret = clk_prepare_enable(pwm->clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "clock enable failed");
+
+	pwm->chip.dev = dev;
+	pwm->chip.ops = &ipq_pwm_ops;
+	pwm->chip.npwm = 4;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "pwmchip_add() failed\n");
+		clk_disable_unprepare(pwm->clk);
+	}
+
+	return ret;
+}
+
+static int ipq_pwm_remove(struct platform_device *pdev)
+{
+	struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&pwm->chip);
+	clk_disable_unprepare(pwm->clk);
+
+	return 0;
+}
+
+static const struct of_device_id pwm_ipq_dt_match[] = {
+	{ .compatible = "qcom,ipq6018-pwm", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, pwm_ipq_dt_match);
+
+static struct platform_driver ipq_pwm_driver = {
+	.driver = {
+		.name = "ipq-pwm",
+		.of_match_table = pwm_ipq_dt_match,
+	},
+	.probe = ipq_pwm_probe,
+	.remove = ipq_pwm_remove,
+};
+
+module_platform_driver(ipq_pwm_driver);
+
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 3/4] dt-bindings: pwm: add IPQ6018 binding

[Baruch Siach]

DT binding for the PWM block in Qualcomm IPQ6018 SoC.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
This series does not convert the TCSR binding documentation to YAML. As
a result, this commit adds new a dt_binding_check warning:

/example-0/syscon@1937000: failed to match any schema with compatible: ['qcom,tcsr-ipq 6018', 'syscon', 'simple-mfd']

If that is a blocker to IPQ6018 PWM support, so be it. Patches will wait
for someone else to push them further.

v8:

  Add size cell to 'reg' (Rob)

v7:

  Use 'reg' instead of 'offset' (Rob)

  Drop 'clock-names' and 'assigned-clock*' (Bjorn)

  Use single cell address/size in example node (Bjorn)

  Move '#pwm-cells' lower in example node (Bjorn)

  List 'reg' as required

v6:

  Device node is child of TCSR; remove phandle (Rob Herring)

  Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)

v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
    Andersson, Kathiravan T)

v4: Update the binding example node as well (Rob Herring's bot)

v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)

v2: Make #pwm-cells const (Rob Herring)
---
 .../devicetree/bindings/pwm/ipq-pwm.yaml      | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
new file mode 100644
index 000000000000..96bc63e336bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm IPQ6018 PWM controller
+
+maintainers:
+  - Baruch Siach <baruch@tkos.co.il>
+
+properties:
+  "#pwm-cells":
+    const: 2
+
+  compatible:
+    const: qcom,ipq6018-pwm
+
+  reg:
+    description: Offset of PWM register in the TCSR block.
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#pwm-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
+
+    tcsr: syscon@1937000 {
+        compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
+        reg = <0x01937000 0x21000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        pwm: pwm@a010 {
+            compatible = "qcom,ipq6018-pwm";
+            reg = <0xa010 0x20>;
+            clocks = <&gcc GCC_ADSS_PWM_CLK>;
+            assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+            assigned-clock-rates = <100000000>;
+            #pwm-cells = <2>;
+        };
+    };
-- 
2.33.0

[PATCH v8 3/4] dt-bindings: pwm: add IPQ6018 binding

[Baruch Siach]

DT binding for the PWM block in Qualcomm IPQ6018 SoC.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
This series does not convert the TCSR binding documentation to YAML. As
a result, this commit adds new a dt_binding_check warning:

/example-0/syscon@1937000: failed to match any schema with compatible: ['qcom,tcsr-ipq 6018', 'syscon', 'simple-mfd']

If that is a blocker to IPQ6018 PWM support, so be it. Patches will wait
for someone else to push them further.

v8:

  Add size cell to 'reg' (Rob)

v7:

  Use 'reg' instead of 'offset' (Rob)

  Drop 'clock-names' and 'assigned-clock*' (Bjorn)

  Use single cell address/size in example node (Bjorn)

  Move '#pwm-cells' lower in example node (Bjorn)

  List 'reg' as required

v6:

  Device node is child of TCSR; remove phandle (Rob Herring)

  Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)

v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
    Andersson, Kathiravan T)

v4: Update the binding example node as well (Rob Herring's bot)

v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)

v2: Make #pwm-cells const (Rob Herring)
---
 .../devicetree/bindings/pwm/ipq-pwm.yaml      | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
new file mode 100644
index 000000000000..96bc63e336bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm IPQ6018 PWM controller
+
+maintainers:
+  - Baruch Siach <baruch@tkos.co.il>
+
+properties:
+  "#pwm-cells":
+    const: 2
+
+  compatible:
+    const: qcom,ipq6018-pwm
+
+  reg:
+    description: Offset of PWM register in the TCSR block.
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - "#pwm-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
+
+    tcsr: syscon@1937000 {
+        compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
+        reg = <0x01937000 0x21000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        pwm: pwm@a010 {
+            compatible = "qcom,ipq6018-pwm";
+            reg = <0xa010 0x20>;
+            clocks = <&gcc GCC_ADSS_PWM_CLK>;
+            assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+            assigned-clock-rates = <100000000>;
+            #pwm-cells = <2>;
+        };
+    };
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v8 4/4] arm64: dts: ipq6018: add pwm node

[Baruch Siach]

Describe the PWM block on IPQ6018.

The PWM is in the TCSR area. Make &tcsr "simple-mfd" compatible, and add
&pwm as child of &tcsr.

Add also ipq6018 specific compatible string.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v8:

  Add size cell to 'reg' (Rob)

v7:

  Use 'reg' instead of 'offset' (Rob)

  Add qcom,tcsr-ipq6018 (Rob)

  Drop clock-names (Bjorn)

v6:

  Make the PWM node child of TCSR (Rob Herring)

  Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)

v5: Use qcom,pwm-regs for TCSR phandle instead of direct regs

v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)
---
 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index a31df7d154db..8e8f72d9c172 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -259,8 +259,20 @@ tcsr_mutex_regs: syscon@1905000 {
 		};
 
 		tcsr: syscon@1937000 {
-			compatible = "syscon";
+			compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
 			reg = <0x0 0x01937000 0x0 0x21000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			pwm: pwm@a010 {
+				compatible = "qcom,ipq6018-pwm";
+				reg = <0xa010 0x20>;
+				clocks = <&gcc GCC_ADSS_PWM_CLK>;
+				assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+				assigned-clock-rates = <100000000>;
+				#pwm-cells = <2>;
+				status = "disabled";
+			};
 		};
 
 		blsp_dma: dma-controller@7884000 {
-- 
2.33.0

[PATCH v8 4/4] arm64: dts: ipq6018: add pwm node

[Baruch Siach]

Describe the PWM block on IPQ6018.

The PWM is in the TCSR area. Make &tcsr "simple-mfd" compatible, and add
&pwm as child of &tcsr.

Add also ipq6018 specific compatible string.

Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v8:

  Add size cell to 'reg' (Rob)

v7:

  Use 'reg' instead of 'offset' (Rob)

  Add qcom,tcsr-ipq6018 (Rob)

  Drop clock-names (Bjorn)

v6:

  Make the PWM node child of TCSR (Rob Herring)

  Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)

v5: Use qcom,pwm-regs for TCSR phandle instead of direct regs

v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)
---
 arch/arm64/boot/dts/qcom/ipq6018.dtsi | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index a31df7d154db..8e8f72d9c172 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -259,8 +259,20 @@ tcsr_mutex_regs: syscon@1905000 {
 		};
 
 		tcsr: syscon@1937000 {
-			compatible = "syscon";
+			compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
 			reg = <0x0 0x01937000 0x0 0x21000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			pwm: pwm@a010 {
+				compatible = "qcom,ipq6018-pwm";
+				reg = <0xa010 0x20>;
+				clocks = <&gcc GCC_ADSS_PWM_CLK>;
+				assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
+				assigned-clock-rates = <100000000>;
+				#pwm-cells = <2>;
+				status = "disabled";
+			};
 		};
 
 		blsp_dma: dma-controller@7884000 {
-- 
2.33.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 3/4] dt-bindings: pwm: add IPQ6018 binding

[Rob Herring]

On Mon, Aug 30, 2021 at 02:46:26PM +0300, Baruch Siach wrote:
> DT binding for the PWM block in Qualcomm IPQ6018 SoC.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> This series does not convert the TCSR binding documentation to YAML. As
> a result, this commit adds new a dt_binding_check warning:
> 
> /example-0/syscon@1937000: failed to match any schema with compatible: ['qcom,tcsr-ipq 6018', 'syscon', 'simple-mfd']
> 
> If that is a blocker to IPQ6018 PWM support, so be it. Patches will wait
> for someone else to push them further.
> 
> v8:
> 
>   Add size cell to 'reg' (Rob)
> 
> v7:
> 
>   Use 'reg' instead of 'offset' (Rob)
> 
>   Drop 'clock-names' and 'assigned-clock*' (Bjorn)
> 
>   Use single cell address/size in example node (Bjorn)
> 
>   Move '#pwm-cells' lower in example node (Bjorn)
> 
>   List 'reg' as required
> 
> v6:
> 
>   Device node is child of TCSR; remove phandle (Rob Herring)
> 
>   Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)
> 
> v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
>     Andersson, Kathiravan T)
> 
> v4: Update the binding example node as well (Rob Herring's bot)
> 
> v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)
> 
> v2: Make #pwm-cells const (Rob Herring)
> ---
>  .../devicetree/bindings/pwm/ipq-pwm.yaml      | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> new file mode 100644
> index 000000000000..96bc63e336bf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm IPQ6018 PWM controller
> +
> +maintainers:
> +  - Baruch Siach <baruch@tkos.co.il>
> +
> +properties:
> +  "#pwm-cells":
> +    const: 2
> +
> +  compatible:
> +    const: qcom,ipq6018-pwm
> +
> +  reg:
> +    description: Offset of PWM register in the TCSR block.
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - "#pwm-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
> +
> +    tcsr: syscon@1937000 {

Drop unused (by the example) labels.

> +        compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
> +        reg = <0x01937000 0x21000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;

           ranges = <0 0x1937000 0x21000>;

Otherwise, the address is not translatable.

> +
> +        pwm: pwm@a010 {
> +            compatible = "qcom,ipq6018-pwm";
> +            reg = <0xa010 0x20>;
> +            clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +            assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +            assigned-clock-rates = <100000000>;
> +            #pwm-cells = <2>;
> +        };
> +    };
> -- 
> 2.33.0
> 
> 

Re: [PATCH v8 3/4] dt-bindings: pwm: add IPQ6018 binding

[Rob Herring]

On Mon, Aug 30, 2021 at 02:46:26PM +0300, Baruch Siach wrote:
> DT binding for the PWM block in Qualcomm IPQ6018 SoC.
> 
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> This series does not convert the TCSR binding documentation to YAML. As
> a result, this commit adds new a dt_binding_check warning:
> 
> /example-0/syscon@1937000: failed to match any schema with compatible: ['qcom,tcsr-ipq 6018', 'syscon', 'simple-mfd']
> 
> If that is a blocker to IPQ6018 PWM support, so be it. Patches will wait
> for someone else to push them further.
> 
> v8:
> 
>   Add size cell to 'reg' (Rob)
> 
> v7:
> 
>   Use 'reg' instead of 'offset' (Rob)
> 
>   Drop 'clock-names' and 'assigned-clock*' (Bjorn)
> 
>   Use single cell address/size in example node (Bjorn)
> 
>   Move '#pwm-cells' lower in example node (Bjorn)
> 
>   List 'reg' as required
> 
> v6:
> 
>   Device node is child of TCSR; remove phandle (Rob Herring)
> 
>   Add assigned-clocks/assigned-clock-rates (Uwe Kleine-König)
> 
> v5: Use qcom,pwm-regs for phandle instead of direct regs (Bjorn
>     Andersson, Kathiravan T)
> 
> v4: Update the binding example node as well (Rob Herring's bot)
> 
> v3: s/qcom,pwm-ipq6018/qcom,ipq6018-pwm/ (Rob Herring)
> 
> v2: Make #pwm-cells const (Rob Herring)
> ---
>  .../devicetree/bindings/pwm/ipq-pwm.yaml      | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> new file mode 100644
> index 000000000000..96bc63e336bf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/ipq-pwm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm IPQ6018 PWM controller
> +
> +maintainers:
> +  - Baruch Siach <baruch@tkos.co.il>
> +
> +properties:
> +  "#pwm-cells":
> +    const: 2
> +
> +  compatible:
> +    const: qcom,ipq6018-pwm
> +
> +  reg:
> +    description: Offset of PWM register in the TCSR block.
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - "#pwm-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
> +
> +    tcsr: syscon@1937000 {

Drop unused (by the example) labels.

> +        compatible = "qcom,tcsr-ipq6018", "syscon", "simple-mfd";
> +        reg = <0x01937000 0x21000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;

           ranges = <0 0x1937000 0x21000>;

Otherwise, the address is not translatable.

> +
> +        pwm: pwm@a010 {
> +            compatible = "qcom,ipq6018-pwm";
> +            reg = <0xa010 0x20>;
> +            clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +            assigned-clocks = <&gcc GCC_ADSS_PWM_CLK>;
> +            assigned-clock-rates = <100000000>;
> +            #pwm-cells = <2>;
> +        };
> +    };
> -- 
> 2.33.0
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 9525 bytes --]

Hello Baruch,

On Mon, Aug 30, 2021 at 02:46:25PM +0300, Baruch Siach wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c76adedd58c9..08add845596f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -260,6 +260,18 @@ config PWM_INTEL_LGM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-intel-lgm.
>  
> +config PWM_IPQ
> +	tristate "IPQ PWM support"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on HAVE_CLK && HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for IPQ PWM block which supports
> +	  4 pwm channels. Each of the these channels can be configured
> +	  independent of each other.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-ipq.
> +
>  config PWM_IQS620A
>  	tristate "Azoteq IQS620A PWM support"
>  	depends on MFD_IQS62X || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 708840b7fba8..7402feae4b36 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
>  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
>  obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
>  obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
> +obj-$(CONFIG_PWM_IPQ)		+= pwm-ipq.o
>  obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
> diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
> new file mode 100644
> index 000000000000..8405d0554951
> --- /dev/null
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/math64.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/bitfield.h>
> +
> +/* The frequency range supported is 1 Hz to clock rate */
> +#define IPQ_PWM_MAX_PERIOD_NS	((u64)NSEC_PER_SEC)
> +
> +/*
> + * The max value specified for each field is based on the number of bits
> + * in the pwm control register for that field
> + */
> +#define IPQ_PWM_MAX_DIV		0xFFFF
> +
> +/*
> + * Two 32-bit registers for each PWM: REG0, and REG1.
> + * Base offset for PWM #i is at 8 * #i.
> + */
> +#define IPQ_PWM_CFG_REG0 0 /*PWM_DIV PWM_HI*/
> +#define IPQ_PWM_REG0_PWM_DIV		GENMASK(15, 0)
> +#define IPQ_PWM_REG0_HI_DURATION	GENMASK(31, 16)

Assuming that IPQ_PWM_REG0_PWM_DIV is a field in IPQ_PWM_CFG_REG0: I
wonder why the former has not "CFG" in it's name?! Ditto below.

> +#define IPQ_PWM_CFG_REG1 4 /*ENABLE UPDATE PWM_PRE_DIV*/
> +#define IPQ_PWM_REG1_PRE_DIV		GENMASK(15, 0)
> +/*
> + * Enable bit is set to enable output toggling in pwm device.
> + * Update bit is set to reflect the changed divider and high duration
> + * values in register.
> + */
> +#define IPQ_PWM_REG1_UPDATE		BIT(30)
> +#define IPQ_PWM_REG1_ENABLE		BIT(31)
> +
> +
> +struct ipq_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	struct regmap *regmap;
> +	u32 regmap_off;
> +};
> +
> +static struct ipq_pwm_chip *to_ipq_pwm_chip(struct pwm_chip *chip)

I would have called this ipq_pwm_from_chip() to have this function's
name use the common prefix, too. (But note that Thierry might not agree
here.)

> +{
> +	return container_of(chip, struct ipq_pwm_chip, chip);
> +}
> +
> +static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned reg)

checkpatch warns about this line

> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
> +	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
> +	unsigned int val;
> +
> +	regmap_read(ipq_chip->regmap, off, &val);

You don't expect regmap_read returning an error? Maybe note that in a
comment to prevent someone preparing patches checking the error. Or
alternatively add a WARN_ONCE when this fails?

> +	return val;
> +}
> +
> +static void ipq_pwm_reg_write(struct pwm_device *pwm, unsigned reg,
> +		unsigned val)

I expected that checkpatch warns here, too, and advises to align follow
up lines to the opening ( in the previous line. So it's me who has to
criticize that.

> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
> +	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
> +
> +	regmap_write(ipq_chip->regmap, off, val);
> +}
> +
> +static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
> +			unsigned int pwm_div, unsigned long rate, u64 duty_ns,
> +			bool enable)
> +{
> +	unsigned long hi_dur;
> +	unsigned long val = 0;
> +
> +	/*
> +	 * high duration = pwm duty * (pwm div + 1)
> +	 * pwm duty = duty_ns / period_ns
> +	 */
> +	hi_dur = div64_u64(duty_ns * rate, (pre_div + 1) * NSEC_PER_SEC);
> +
> +	val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
> +		FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
> +	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val);
> +
> +	val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
> +	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
> +
> +	/* Enable needs a separate write to REG1 */

s/Enable/Updating REG1/ ?

> +	val |= IPQ_PWM_REG1_UPDATE;
> +	if (enable)
> +		val |= IPQ_PWM_REG1_ENABLE;
> +	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
> +}
> +
> +static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
> +	unsigned long freq;
> +	unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div;
> +	long long diff;
> +	unsigned long rate = clk_get_rate(ipq_chip->clk);
> +	unsigned long min_diff = rate;
> +	u64 period_ns, duty_ns;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (state->period < div64_u64(NSEC_PER_SEC, rate))
> +		return -ERANGE;
> +
> +	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
> +	duty_ns = min(state->duty_cycle, period_ns);
> +
> +	/* freq in Hz for period in nano second */
> +	freq = div64_u64(NSEC_PER_SEC, period_ns);

You're loosing quite some precision here. Consider a clock rate of
266666667 Hz and period = 500000001 ns.

Then we end up with freq = 1 (while the exact result is nearly 2) which
results in diff below being too small.

> +	best_pre_div = IPQ_PWM_MAX_DIV;
> +	best_pwm_div = IPQ_PWM_MAX_DIV;
> +	/* Initial pre_div value such that pwm_div < IPQ_PWM_MAX_DIV */
> +	pre_div = DIV64_U64_ROUND_UP(period_ns * rate,
> +			(u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));

This is wrong, you need to round down here. (Consider cases where you
need pre_div = 0.)

> +
> +	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
> +		pwm_div = DIV64_U64_ROUND_UP(period_ns * rate,
> +				(u64)NSEC_PER_SEC * (pre_div + 1));
> +		pwm_div--;

Can it happen that pwm_div is zero before it is decreased by one? Also
you need to round down here; with rounding up the resulting period is
bigger than the requested period (unless the division yields an exact
integer).

> +		if (pre_div > pwm_div)
> +			break;

A comment here why we can end the search would be good.

> +		/*
> +		 * Make sure we can do 100% duty cycle where
> +		 * hi_dur == pwm_div + 1
> +		 */
> +		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
> +			continue;
> +
> +		diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
> +			- (uint64_t)rate;
> +
> +		if (diff < 0) /* period larger than requested */
> +			continue;
> +		if (diff == 0) { /* bingo */
> +			best_pre_div = pre_div;
> +			best_pwm_div = pwm_div;
> +			break;
> +		}
> +		if (diff < min_diff) {
> +			min_diff = diff;
> +			best_pre_div = pre_div;
> +			best_pwm_div = pwm_div;
> +		}
> +	}
> +
> +	/* config divider values for the closest possible frequency */
> +	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
> +			    rate, duty_ns, state->enabled);
> +
> +	return 0;
> +}
> +
> +static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      struct pwm_state *state)
> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
> +	unsigned long rate = clk_get_rate(ipq_chip->clk);
> +	unsigned int pre_div, pwm_div, hi_dur;
> +	u64 effective_div, hi_div;
> +	u32 reg0, reg1;
> +
> +	reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0);
> +	reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1);
> +
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
> +
> +	pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
> +	hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
> +	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
> +
> +	/* No overflow here, both pre_div and pwm_div <= 0xffff */
> +	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
> +	state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);

You have to round up here to make apply . get_state idempotent.

> +	hi_div = hi_dur * (pre_div + 1);
> +	state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
> +}

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 9525 bytes --]

Hello Baruch,

On Mon, Aug 30, 2021 at 02:46:25PM +0300, Baruch Siach wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index c76adedd58c9..08add845596f 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -260,6 +260,18 @@ config PWM_INTEL_LGM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-intel-lgm.
>  
> +config PWM_IPQ
> +	tristate "IPQ PWM support"
> +	depends on ARCH_QCOM || COMPILE_TEST
> +	depends on HAVE_CLK && HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for IPQ PWM block which supports
> +	  4 pwm channels. Each of the these channels can be configured
> +	  independent of each other.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-ipq.
> +
>  config PWM_IQS620A
>  	tristate "Azoteq IQS620A PWM support"
>  	depends on MFD_IQS62X || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 708840b7fba8..7402feae4b36 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
>  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
>  obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
>  obj-$(CONFIG_PWM_INTEL_LGM)	+= pwm-intel-lgm.o
> +obj-$(CONFIG_PWM_IPQ)		+= pwm-ipq.o
>  obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
>  obj-$(CONFIG_PWM_KEEMBAY)	+= pwm-keembay.o
> diff --git a/drivers/pwm/pwm-ipq.c b/drivers/pwm/pwm-ipq.c
> new file mode 100644
> index 000000000000..8405d0554951
> --- /dev/null
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -0,0 +1,275 @@
> +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> +/*
> + * Copyright (c) 2016-2017, 2020 The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/math64.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/bitfield.h>
> +
> +/* The frequency range supported is 1 Hz to clock rate */
> +#define IPQ_PWM_MAX_PERIOD_NS	((u64)NSEC_PER_SEC)
> +
> +/*
> + * The max value specified for each field is based on the number of bits
> + * in the pwm control register for that field
> + */
> +#define IPQ_PWM_MAX_DIV		0xFFFF
> +
> +/*
> + * Two 32-bit registers for each PWM: REG0, and REG1.
> + * Base offset for PWM #i is at 8 * #i.
> + */
> +#define IPQ_PWM_CFG_REG0 0 /*PWM_DIV PWM_HI*/
> +#define IPQ_PWM_REG0_PWM_DIV		GENMASK(15, 0)
> +#define IPQ_PWM_REG0_HI_DURATION	GENMASK(31, 16)

Assuming that IPQ_PWM_REG0_PWM_DIV is a field in IPQ_PWM_CFG_REG0: I
wonder why the former has not "CFG" in it's name?! Ditto below.

> +#define IPQ_PWM_CFG_REG1 4 /*ENABLE UPDATE PWM_PRE_DIV*/
> +#define IPQ_PWM_REG1_PRE_DIV		GENMASK(15, 0)
> +/*
> + * Enable bit is set to enable output toggling in pwm device.
> + * Update bit is set to reflect the changed divider and high duration
> + * values in register.
> + */
> +#define IPQ_PWM_REG1_UPDATE		BIT(30)
> +#define IPQ_PWM_REG1_ENABLE		BIT(31)
> +
> +
> +struct ipq_pwm_chip {
> +	struct pwm_chip chip;
> +	struct clk *clk;
> +	struct regmap *regmap;
> +	u32 regmap_off;
> +};
> +
> +static struct ipq_pwm_chip *to_ipq_pwm_chip(struct pwm_chip *chip)

I would have called this ipq_pwm_from_chip() to have this function's
name use the common prefix, too. (But note that Thierry might not agree
here.)

> +{
> +	return container_of(chip, struct ipq_pwm_chip, chip);
> +}
> +
> +static unsigned int ipq_pwm_reg_read(struct pwm_device *pwm, unsigned reg)

checkpatch warns about this line

> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
> +	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
> +	unsigned int val;
> +
> +	regmap_read(ipq_chip->regmap, off, &val);

You don't expect regmap_read returning an error? Maybe note that in a
comment to prevent someone preparing patches checking the error. Or
alternatively add a WARN_ONCE when this fails?

> +	return val;
> +}
> +
> +static void ipq_pwm_reg_write(struct pwm_device *pwm, unsigned reg,
> +		unsigned val)

I expected that checkpatch warns here, too, and advises to align follow
up lines to the opening ( in the previous line. So it's me who has to
criticize that.

> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(pwm->chip);
> +	unsigned int off = ipq_chip->regmap_off + 8 * pwm->hwpwm + reg;
> +
> +	regmap_write(ipq_chip->regmap, off, val);
> +}
> +
> +static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
> +			unsigned int pwm_div, unsigned long rate, u64 duty_ns,
> +			bool enable)
> +{
> +	unsigned long hi_dur;
> +	unsigned long val = 0;
> +
> +	/*
> +	 * high duration = pwm duty * (pwm div + 1)
> +	 * pwm duty = duty_ns / period_ns
> +	 */
> +	hi_dur = div64_u64(duty_ns * rate, (pre_div + 1) * NSEC_PER_SEC);
> +
> +	val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
> +		FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
> +	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val);
> +
> +	val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
> +	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
> +
> +	/* Enable needs a separate write to REG1 */

s/Enable/Updating REG1/ ?

> +	val |= IPQ_PWM_REG1_UPDATE;
> +	if (enable)
> +		val |= IPQ_PWM_REG1_ENABLE;
> +	ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
> +}
> +
> +static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
> +	unsigned long freq;
> +	unsigned int pre_div, pwm_div, best_pre_div, best_pwm_div;
> +	long long diff;
> +	unsigned long rate = clk_get_rate(ipq_chip->clk);
> +	unsigned long min_diff = rate;
> +	u64 period_ns, duty_ns;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
> +
> +	if (state->period < div64_u64(NSEC_PER_SEC, rate))
> +		return -ERANGE;
> +
> +	period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
> +	duty_ns = min(state->duty_cycle, period_ns);
> +
> +	/* freq in Hz for period in nano second */
> +	freq = div64_u64(NSEC_PER_SEC, period_ns);

You're loosing quite some precision here. Consider a clock rate of
266666667 Hz and period = 500000001 ns.

Then we end up with freq = 1 (while the exact result is nearly 2) which
results in diff below being too small.

> +	best_pre_div = IPQ_PWM_MAX_DIV;
> +	best_pwm_div = IPQ_PWM_MAX_DIV;
> +	/* Initial pre_div value such that pwm_div < IPQ_PWM_MAX_DIV */
> +	pre_div = DIV64_U64_ROUND_UP(period_ns * rate,
> +			(u64)NSEC_PER_SEC * (IPQ_PWM_MAX_DIV + 1));

This is wrong, you need to round down here. (Consider cases where you
need pre_div = 0.)

> +
> +	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
> +		pwm_div = DIV64_U64_ROUND_UP(period_ns * rate,
> +				(u64)NSEC_PER_SEC * (pre_div + 1));
> +		pwm_div--;

Can it happen that pwm_div is zero before it is decreased by one? Also
you need to round down here; with rounding up the resulting period is
bigger than the requested period (unless the division yields an exact
integer).

> +		if (pre_div > pwm_div)
> +			break;

A comment here why we can end the search would be good.

> +		/*
> +		 * Make sure we can do 100% duty cycle where
> +		 * hi_dur == pwm_div + 1
> +		 */
> +		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
> +			continue;
> +
> +		diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
> +			- (uint64_t)rate;
> +
> +		if (diff < 0) /* period larger than requested */
> +			continue;
> +		if (diff == 0) { /* bingo */
> +			best_pre_div = pre_div;
> +			best_pwm_div = pwm_div;
> +			break;
> +		}
> +		if (diff < min_diff) {
> +			min_diff = diff;
> +			best_pre_div = pre_div;
> +			best_pwm_div = pwm_div;
> +		}
> +	}
> +
> +	/* config divider values for the closest possible frequency */
> +	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
> +			    rate, duty_ns, state->enabled);
> +
> +	return 0;
> +}
> +
> +static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      struct pwm_state *state)
> +{
> +	struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
> +	unsigned long rate = clk_get_rate(ipq_chip->clk);
> +	unsigned int pre_div, pwm_div, hi_dur;
> +	u64 effective_div, hi_div;
> +	u32 reg0, reg1;
> +
> +	reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0);
> +	reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1);
> +
> +	state->polarity = PWM_POLARITY_NORMAL;
> +	state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
> +
> +	pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
> +	hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
> +	pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
> +
> +	/* No overflow here, both pre_div and pwm_div <= 0xffff */
> +	effective_div = (u64)(pre_div + 1) * (pwm_div + 1);
> +	state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);

You have to round up here to make apply . get_state idempotent.

> +	hi_div = hi_dur * (pre_div + 1);
> +	state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
> +}

Best regards
Uwe

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible

[Lee Jones]

On Mon, 30 Aug 2021, Baruch Siach wrote:

> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  Documentation/devicetree/bindings/mfd/qcom,tcsr.txt | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible

[Lee Jones]

On Mon, 30 Aug 2021, Baruch Siach wrote:

> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  Documentation/devicetree/bindings/mfd/qcom,tcsr.txt | 1 +
>  1 file changed, 1 insertion(+)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1232 bytes --]

On Mon, Aug 30, 2021 at 02:46:24PM +0300, Baruch Siach wrote:
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  Documentation/devicetree/bindings/mfd/qcom,tcsr.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
> index e90519d566a3..c5f4f0ddfcc3 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
> @@ -6,6 +6,7 @@ registers via syscon.
>  
>  Required properties:
>  - compatible:	Should contain:
> +		"qcom,tcsr-ipq6018", "syscon", "simple-mfd" for IPQ6018
>  		"qcom,tcsr-ipq8064", "syscon" for IPQ8064
>  		"qcom,tcsr-apq8064", "syscon" for APQ8064
>  		"qcom,tcsr-msm8660", "syscon" for MSM8660

I wonder about why qcom,tcsr-ipq6018 should use simple-mfd, but the
others don't. Assuming there is a deeper reason, mentioning that in the
commit log (or depending on the reason maybe even in the binding?)
sounds sensible.

Best regards
Uwe


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v8 1/4] dt-bindings: mfd: qcom,tcsr: document ipq6018 compatible

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1232 bytes --]

On Mon, Aug 30, 2021 at 02:46:24PM +0300, Baruch Siach wrote:
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
>  Documentation/devicetree/bindings/mfd/qcom,tcsr.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
> index e90519d566a3..c5f4f0ddfcc3 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom,tcsr.txt
> @@ -6,6 +6,7 @@ registers via syscon.
>  
>  Required properties:
>  - compatible:	Should contain:
> +		"qcom,tcsr-ipq6018", "syscon", "simple-mfd" for IPQ6018
>  		"qcom,tcsr-ipq8064", "syscon" for IPQ8064
>  		"qcom,tcsr-apq8064", "syscon" for APQ8064
>  		"qcom,tcsr-msm8660", "syscon" for MSM8660

I wonder about why qcom,tcsr-ipq6018 should use simple-mfd, but the
others don't. Assuming there is a deeper reason, mentioning that in the
commit log (or depending on the reason maybe even in the binding?)
sounds sensible.

Best regards
Uwe


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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block

[Baruch Siach]

Hi Uwe,

On Tue, Sep 14 2021, Uwe Kleine-König wrote:
> On Mon, Aug 30, 2021 at 02:46:25PM +0300, Baruch Siach wrote:
>> +	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
>> +		pwm_div = DIV64_U64_ROUND_UP(period_ns * rate,
>> +				(u64)NSEC_PER_SEC * (pre_div + 1));
>> +		pwm_div--;
>
> Can it happen that pwm_div is zero before it is decreased by one? Also
> you need to round down here; with rounding up the resulting period is
> bigger than the requested period (unless the division yields an exact
> integer).

I followed your round down advice on v9, but it turned out to be
wrong. Round down means that the divider is smaller so the period is
larger. This means that 'diff' below can not be positive. So only exact
match (diff == 0) works. When there is no exact match, best_* values are
left in their initial setting.

I'll fix that in v10 along with another bug I introduced in v9.

baruch

>> +		if (pre_div > pwm_div)
>> +			break;
>
> A comment here why we can end the search would be good.
>
>> +		/*
>> +		 * Make sure we can do 100% duty cycle where
>> +		 * hi_dur == pwm_div + 1
>> +		 */
>> +		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
>> +			continue;
>> +
>> +		diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
>> +			- (uint64_t)rate;
>> +
>> +		if (diff < 0) /* period larger than requested */
>> +			continue;
>> +		if (diff == 0) { /* bingo */
>> +			best_pre_div = pre_div;
>> +			best_pwm_div = pwm_div;
>> +			break;
>> +		}
>> +		if (diff < min_diff) {
>> +			min_diff = diff;
>> +			best_pre_div = pre_div;
>> +			best_pwm_div = pwm_div;
>> +		}
>> +	}
>> +
>> +	/* config divider values for the closest possible frequency */
>> +	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
>> +			    rate, duty_ns, state->enabled);
>> +
>> +	return 0;
>> +}

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v8 2/4] pwm: driver for qualcomm ipq6018 pwm block

[Baruch Siach]

Hi Uwe,

On Tue, Sep 14 2021, Uwe Kleine-König wrote:
> On Mon, Aug 30, 2021 at 02:46:25PM +0300, Baruch Siach wrote:
>> +	for (; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
>> +		pwm_div = DIV64_U64_ROUND_UP(period_ns * rate,
>> +				(u64)NSEC_PER_SEC * (pre_div + 1));
>> +		pwm_div--;
>
> Can it happen that pwm_div is zero before it is decreased by one? Also
> you need to round down here; with rounding up the resulting period is
> bigger than the requested period (unless the division yields an exact
> integer).

I followed your round down advice on v9, but it turned out to be
wrong. Round down means that the divider is smaller so the period is
larger. This means that 'diff' below can not be positive. So only exact
match (diff == 0) works. When there is no exact match, best_* values are
left in their initial setting.

I'll fix that in v10 along with another bug I introduced in v9.

baruch

>> +		if (pre_div > pwm_div)
>> +			break;
>
> A comment here why we can end the search would be good.
>
>> +		/*
>> +		 * Make sure we can do 100% duty cycle where
>> +		 * hi_dur == pwm_div + 1
>> +		 */
>> +		if (pwm_div > IPQ_PWM_MAX_DIV - 1)
>> +			continue;
>> +
>> +		diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
>> +			- (uint64_t)rate;
>> +
>> +		if (diff < 0) /* period larger than requested */
>> +			continue;
>> +		if (diff == 0) { /* bingo */
>> +			best_pre_div = pre_div;
>> +			best_pwm_div = pwm_div;
>> +			break;
>> +		}
>> +		if (diff < min_diff) {
>> +			min_diff = diff;
>> +			best_pre_div = pre_div;
>> +			best_pwm_div = pwm_div;
>> +		}
>> +	}
>> +
>> +	/* config divider values for the closest possible frequency */
>> +	config_div_and_duty(pwm, best_pre_div, best_pwm_div,
>> +			    rate, duty_ns, state->enabled);
>> +
>> +	return 0;
>> +}

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -