* [PATCH v5 1/4] arm64: dts: ipq6018: correct TCSR block area
@ 2021-07-13 11:35 ` Baruch Siach
0 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
According to Bjorn Andersson[1], &tcsr_q6 base is 0x01937000 with size
0x21000. Adjust qcom,halt-regs offsets (add 0x8000) to match the new
syscon base.
[1] https://lore.kernel.org/r/YLgO0Aj1d4w9EcPv@yoga
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v5: New patch in this series
---
arch/arm64/boot/dts/qcom/ipq6018.dtsi | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index 6ee7b99c21ec..72ac36c1be57 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -270,9 +270,9 @@ tcsr_mutex_regs: syscon@1905000 {
reg = <0x0 0x01905000 0x0 0x8000>;
};
- tcsr_q6: syscon@1945000 {
+ tcsr_q6: syscon@1937000 {
compatible = "syscon";
- reg = <0x0 0x01945000 0x0 0xe000>;
+ reg = <0x0 0x01937000 0x0 0x21000>;
};
blsp_dma: dma-controller@7884000 {
@@ -615,7 +615,7 @@ q6v5_wcss: remoteproc@cd00000 {
clocks = <&gcc GCC_PRNG_AHB_CLK>;
clock-names = "prng";
- qcom,halt-regs = <&tcsr_q6 0xa000 0xd000 0x0>;
+ qcom,halt-regs = <&tcsr_q6 0x12000 0x15000 0x8000>;
qcom,smem-states = <&wcss_smp2p_out 0>,
<&wcss_smp2p_out 1>;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v5 1/4] arm64: dts: ipq6018: correct TCSR block area
@ 2021-07-13 11:35 ` Baruch Siach
0 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
According to Bjorn Andersson[1], &tcsr_q6 base is 0x01937000 with size
0x21000. Adjust qcom,halt-regs offsets (add 0x8000) to match the new
syscon base.
[1] https://lore.kernel.org/r/YLgO0Aj1d4w9EcPv@yoga
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
v5: New patch in this series
---
arch/arm64/boot/dts/qcom/ipq6018.dtsi | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index 6ee7b99c21ec..72ac36c1be57 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -270,9 +270,9 @@ tcsr_mutex_regs: syscon@1905000 {
reg = <0x0 0x01905000 0x0 0x8000>;
};
- tcsr_q6: syscon@1945000 {
+ tcsr_q6: syscon@1937000 {
compatible = "syscon";
- reg = <0x0 0x01945000 0x0 0xe000>;
+ reg = <0x0 0x01937000 0x0 0x21000>;
};
blsp_dma: dma-controller@7884000 {
@@ -615,7 +615,7 @@ q6v5_wcss: remoteproc@cd00000 {
clocks = <&gcc GCC_PRNG_AHB_CLK>;
clock-names = "prng";
- qcom,halt-regs = <&tcsr_q6 0xa000 0xd000 0x0>;
+ qcom,halt-regs = <&tcsr_q6 0x12000 0x15000 0x8000>;
qcom,smem-states = <&wcss_smp2p_out 0>,
<&wcss_smp2p_out 1>;
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
2021-07-13 11:35 ` Baruch Siach
@ 2021-07-13 11:35 ` Baruch Siach
-1 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
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>
---
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 | 278 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 291 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..ddfbe95816a4
--- /dev/null
+++ b/drivers/pwm/pwm-ipq.c
@@ -0,0 +1,278 @@
+// 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/math64.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#define IPQ_PWM_MAX_DEVICES 4
+
+/* The frequency range supported is 1Hz to 100MHz */
+#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
+#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
+#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
+
+#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 1 /*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 ipq_pwm_reg_offset(struct pwm_device *pwm, unsigned reg)
+{
+ return ((pwm->hwpwm * 2) + reg) * 4;
+}
+
+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 + ipq_pwm_reg_offset(pwm, 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
+ bool enable)
+{
+ unsigned long hi_dur;
+ unsigned long long quotient;
+ unsigned long val = 0;
+
+ /*
+ * high duration = pwm duty * (pwm div + 1)
+ * pwm duty = duty_ns / period_ns
+ */
+ quotient = (pwm_div + 1) * duty_ns;
+ hi_dur = div64_u64(quotient, period_ns);
+
+ 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;
+ else
+ 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, close_pre_div, close_pwm_div;
+ long long diff;
+ unsigned long rate = clk_get_rate(ipq_chip->clk);
+ unsigned long min_diff = rate;
+ uint64_t fin_ps;
+ u64 period_ns, duty_ns;
+
+ if (state->period < IPQ_PWM_MIN_PERIOD_NS)
+ 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);
+ fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
+ close_pre_div = IPQ_PWM_MAX_DIV;
+ close_pwm_div = IPQ_PWM_MAX_DIV;
+
+ for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
+ pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
+ fin_ps * (pre_div + 1));
+ pwm_div--;
+ if (pwm_div > IPQ_PWM_MAX_DIV)
+ 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 */
+ close_pre_div = pre_div;
+ close_pwm_div = pwm_div;
+ break;
+ }
+ if (diff < min_diff) {
+ min_diff = diff;
+ close_pre_div = pre_div;
+ close_pwm_div = pwm_div;
+ }
+ }
+
+ /* config divider values for the closest possible frequency */
+ config_div_and_duty(pwm, close_pre_div, close_pwm_div,
+ period_ns, 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);
+
+ effective_div = (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 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;
+ struct of_phandle_args args;
+ int ret;
+
+ pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, pwm);
+
+ ret = of_parse_phandle_with_fixed_args(dev->of_node, "qcom,pwm-regs",
+ 1, 0, &args);
+ if (ret)
+ return dev_err_probe(dev, ret, "regs parse failed");
+
+ pwm->regmap = syscon_node_to_regmap(args.np);
+ of_node_put(args.np);
+ if (IS_ERR(pwm->regmap))
+ return dev_err_probe(dev, PTR_ERR(pwm->regmap),
+ "regs map failed");
+ pwm->regmap_off = args.args[0];
+
+ pwm->clk = devm_clk_get(dev, "core");
+ if (IS_ERR(pwm->clk))
+ return dev_err_probe(dev, PTR_ERR(pwm->clk),
+ "failed to get core clock");
+
+ ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
+ if (ret)
+ return dev_err_probe(dev, ret, "clock rate set failed");
+
+ 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 = IPQ_PWM_MAX_DEVICES;
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "pwmchip_add() failed\n");
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ipq_pwm_remove(struct platform_device *pdev)
+{
+ struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(pwm->clk);
+ pwmchip_remove(&pwm->chip);
+
+ 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.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
@ 2021-07-13 11:35 ` Baruch Siach
0 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
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>
---
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 | 278 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 291 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..ddfbe95816a4
--- /dev/null
+++ b/drivers/pwm/pwm-ipq.c
@@ -0,0 +1,278 @@
+// 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/math64.h>
+#include <linux/of_device.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#define IPQ_PWM_MAX_DEVICES 4
+
+/* The frequency range supported is 1Hz to 100MHz */
+#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
+#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
+#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
+
+#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 1 /*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 ipq_pwm_reg_offset(struct pwm_device *pwm, unsigned reg)
+{
+ return ((pwm->hwpwm * 2) + reg) * 4;
+}
+
+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 + ipq_pwm_reg_offset(pwm, 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
+ bool enable)
+{
+ unsigned long hi_dur;
+ unsigned long long quotient;
+ unsigned long val = 0;
+
+ /*
+ * high duration = pwm duty * (pwm div + 1)
+ * pwm duty = duty_ns / period_ns
+ */
+ quotient = (pwm_div + 1) * duty_ns;
+ hi_dur = div64_u64(quotient, period_ns);
+
+ 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;
+ else
+ 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, close_pre_div, close_pwm_div;
+ long long diff;
+ unsigned long rate = clk_get_rate(ipq_chip->clk);
+ unsigned long min_diff = rate;
+ uint64_t fin_ps;
+ u64 period_ns, duty_ns;
+
+ if (state->period < IPQ_PWM_MIN_PERIOD_NS)
+ 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);
+ fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
+ close_pre_div = IPQ_PWM_MAX_DIV;
+ close_pwm_div = IPQ_PWM_MAX_DIV;
+
+ for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
+ pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
+ fin_ps * (pre_div + 1));
+ pwm_div--;
+ if (pwm_div > IPQ_PWM_MAX_DIV)
+ 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 */
+ close_pre_div = pre_div;
+ close_pwm_div = pwm_div;
+ break;
+ }
+ if (diff < min_diff) {
+ min_diff = diff;
+ close_pre_div = pre_div;
+ close_pwm_div = pwm_div;
+ }
+ }
+
+ /* config divider values for the closest possible frequency */
+ config_div_and_duty(pwm, close_pre_div, close_pwm_div,
+ period_ns, 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);
+
+ effective_div = (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 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;
+ struct of_phandle_args args;
+ int ret;
+
+ pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+ if (!pwm)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, pwm);
+
+ ret = of_parse_phandle_with_fixed_args(dev->of_node, "qcom,pwm-regs",
+ 1, 0, &args);
+ if (ret)
+ return dev_err_probe(dev, ret, "regs parse failed");
+
+ pwm->regmap = syscon_node_to_regmap(args.np);
+ of_node_put(args.np);
+ if (IS_ERR(pwm->regmap))
+ return dev_err_probe(dev, PTR_ERR(pwm->regmap),
+ "regs map failed");
+ pwm->regmap_off = args.args[0];
+
+ pwm->clk = devm_clk_get(dev, "core");
+ if (IS_ERR(pwm->clk))
+ return dev_err_probe(dev, PTR_ERR(pwm->clk),
+ "failed to get core clock");
+
+ ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
+ if (ret)
+ return dev_err_probe(dev, ret, "clock rate set failed");
+
+ 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 = IPQ_PWM_MAX_DEVICES;
+
+ ret = pwmchip_add(&pwm->chip);
+ if (ret < 0) {
+ dev_err_probe(dev, ret, "pwmchip_add() failed\n");
+ clk_disable_unprepare(pwm->clk);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ipq_pwm_remove(struct platform_device *pdev)
+{
+ struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
+
+ clk_disable_unprepare(pwm->clk);
+ pwmchip_remove(&pwm->chip);
+
+ 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.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v5 3/4] dt-bindings: pwm: add IPQ6018 binding
2021-07-13 11:35 ` Baruch Siach
@ 2021-07-13 11:35 ` Baruch Siach
-1 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
DT binding for the PWM block in Qualcomm IPQ6018 SoC.
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
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 | 55 +++++++++++++++++++
1 file changed, 55 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..a07bfe63dc1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
@@ -0,0 +1,55 @@
+# 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
+
+ qcom,pwm-regs:
+ $ref: '/schemas/types.yaml#/definitions/phandle-array'
+ maxItems: 1
+ description: |
+ phandle link and offset to TCSR block
+
+ clocks:
+ maxItems: 1
+
+ clock-names:
+ const: core
+
+required:
+ - "#pwm-cells"
+ - compatible
+ - qcom,pwm-regs
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ pwm {
+ #pwm-cells = <2>;
+ compatible = "qcom,ipq6018-pwm";
+ qcom,pwm-regs = <&tcsr_q6 0xa010>;
+ clocks = <&gcc GCC_ADSS_PWM_CLK>;
+ clock-names = "core";
+ };
+ };
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v5 3/4] dt-bindings: pwm: add IPQ6018 binding
@ 2021-07-13 11:35 ` Baruch Siach
0 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
DT binding for the PWM block in Qualcomm IPQ6018 SoC.
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
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 | 55 +++++++++++++++++++
1 file changed, 55 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..a07bfe63dc1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
@@ -0,0 +1,55 @@
+# 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
+
+ qcom,pwm-regs:
+ $ref: '/schemas/types.yaml#/definitions/phandle-array'
+ maxItems: 1
+ description: |
+ phandle link and offset to TCSR block
+
+ clocks:
+ maxItems: 1
+
+ clock-names:
+ const: core
+
+required:
+ - "#pwm-cells"
+ - compatible
+ - qcom,pwm-regs
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ pwm {
+ #pwm-cells = <2>;
+ compatible = "qcom,ipq6018-pwm";
+ qcom,pwm-regs = <&tcsr_q6 0xa010>;
+ clocks = <&gcc GCC_ADSS_PWM_CLK>;
+ clock-names = "core";
+ };
+ };
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v5 4/4] arm64: dts: ipq6018: add pwm node
2021-07-13 11:35 ` Baruch Siach
@ 2021-07-13 11:35 ` Baruch Siach
-1 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
Describe the PWM block on IPQ6018.
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
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 | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index 72ac36c1be57..06b6676097e8 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -355,6 +355,15 @@ i2c_1: i2c@78b7000 { /* BLSP1 QUP2 */
status = "disabled";
};
+ pwm: pwm {
+ #pwm-cells = <2>;
+ compatible = "qcom,ipq6018-pwm";
+ qcom,pwm-regs = <&tcsr_q6 0xa010>;
+ clocks = <&gcc GCC_ADSS_PWM_CLK>;
+ clock-names = "core";
+ status = "disabled";
+ };
+
qpic_bam: dma-controller@7984000 {
compatible = "qcom,bam-v1.7.0";
reg = <0x0 0x07984000 0x0 0x1a000>;
--
2.30.2
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v5 4/4] arm64: dts: ipq6018: add pwm node
@ 2021-07-13 11:35 ` Baruch Siach
0 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-13 11:35 UTC (permalink / raw)
To: Thierry Reding, Uwe Kleine-König, Lee Jones
Cc: Baruch Siach, Andy Gross, Bjorn Andersson, Balaji Prakash J,
Rob Herring, Robert Marko, Kathiravan T, linux-pwm, devicetree,
linux-arm-msm, linux-arm-kernel
Describe the PWM block on IPQ6018.
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
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 | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
index 72ac36c1be57..06b6676097e8 100644
--- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
@@ -355,6 +355,15 @@ i2c_1: i2c@78b7000 { /* BLSP1 QUP2 */
status = "disabled";
};
+ pwm: pwm {
+ #pwm-cells = <2>;
+ compatible = "qcom,ipq6018-pwm";
+ qcom,pwm-regs = <&tcsr_q6 0xa010>;
+ clocks = <&gcc GCC_ADSS_PWM_CLK>;
+ clock-names = "core";
+ status = "disabled";
+ };
+
qpic_bam: dma-controller@7984000 {
compatible = "qcom,bam-v1.7.0";
reg = <0x0 0x07984000 0x0 0x1a000>;
--
2.30.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
2021-07-13 11:35 ` Baruch Siach
(?)
@ 2021-07-13 18:07 ` kernel test robot
-1 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2021-07-13 18:07 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6370 bytes --]
Hi Baruch,
I love your patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on robh/for-next v5.14-rc1 next-20210713]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Baruch-Siach/arm64-dts-ipq6018-correct-TCSR-block-area/20210713-193616
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/3215e41e0c2fbd26202f21458ea6f1993f90e126
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Baruch-Siach/arm64-dts-ipq6018-correct-TCSR-block-area/20210713-193616
git checkout 3215e41e0c2fbd26202f21458ea6f1993f90e126
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers/pwm/pwm-ipq.c: In function 'config_div_and_duty':
>> drivers/pwm/pwm-ipq.c:96:8: error: implicit declaration of function 'FIELD_PREP' [-Werror=implicit-function-declaration]
96 | val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
| ^~~~~~~~~~
drivers/pwm/pwm-ipq.c: In function 'ipq_pwm_get_state':
>> drivers/pwm/pwm-ipq.c:182:12: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
182 | pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
| ^~~~~~~~~
cc1: some warnings being treated as errors
vim +/FIELD_PREP +96 drivers/pwm/pwm-ipq.c
80
81 static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
82 unsigned int pwm_div, u64 period_ns, u64 duty_ns,
83 bool enable)
84 {
85 unsigned long hi_dur;
86 unsigned long long quotient;
87 unsigned long val = 0;
88
89 /*
90 * high duration = pwm duty * (pwm div + 1)
91 * pwm duty = duty_ns / period_ns
92 */
93 quotient = (pwm_div + 1) * duty_ns;
94 hi_dur = div64_u64(quotient, period_ns);
95
> 96 val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
97 FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
98 ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val);
99
100 val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
101 ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
102
103 /* Enable needs a separate write to REG1 */
104 val |= IPQ_PWM_REG1_UPDATE;
105 if (enable)
106 val |= IPQ_PWM_REG1_ENABLE;
107 else
108 val &= ~IPQ_PWM_REG1_ENABLE;
109 ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
110 }
111
112 static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
113 const struct pwm_state *state)
114 {
115 struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
116 unsigned long freq;
117 unsigned int pre_div, pwm_div, close_pre_div, close_pwm_div;
118 long long diff;
119 unsigned long rate = clk_get_rate(ipq_chip->clk);
120 unsigned long min_diff = rate;
121 uint64_t fin_ps;
122 u64 period_ns, duty_ns;
123
124 if (state->period < IPQ_PWM_MIN_PERIOD_NS)
125 return -ERANGE;
126
127 period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
128 duty_ns = min(state->duty_cycle, period_ns);
129
130 /* freq in Hz for period in nano second */
131 freq = div64_u64(NSEC_PER_SEC, period_ns);
132 fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
133 close_pre_div = IPQ_PWM_MAX_DIV;
134 close_pwm_div = IPQ_PWM_MAX_DIV;
135
136 for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
137 pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
138 fin_ps * (pre_div + 1));
139 pwm_div--;
140 if (pwm_div > IPQ_PWM_MAX_DIV)
141 continue;
142
143 diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
144 - (uint64_t)rate;
145
146 if (diff < 0) /* period larger than requested */
147 continue;
148 if (diff == 0) { /* bingo */
149 close_pre_div = pre_div;
150 close_pwm_div = pwm_div;
151 break;
152 }
153 if (diff < min_diff) {
154 min_diff = diff;
155 close_pre_div = pre_div;
156 close_pwm_div = pwm_div;
157 }
158 }
159
160 /* config divider values for the closest possible frequency */
161 config_div_and_duty(pwm, close_pre_div, close_pwm_div,
162 period_ns, duty_ns, state->enabled);
163
164 return 0;
165 }
166
167 static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
168 struct pwm_state *state)
169 {
170 struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
171 unsigned long rate = clk_get_rate(ipq_chip->clk);
172 unsigned int pre_div, pwm_div, hi_dur;
173 u64 effective_div, hi_div;
174 u32 reg0, reg1;
175
176 reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0);
177 reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1);
178
179 state->polarity = PWM_POLARITY_NORMAL;
180 state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
181
> 182 pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
183 hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
184 pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
185
186 effective_div = (pre_div + 1) * (pwm_div + 1);
187 state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
188
189 hi_div = hi_dur * (pre_div + 1);
190 state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
191 }
192
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 60273 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
2021-07-13 11:35 ` Baruch Siach
(?)
(?)
@ 2021-07-13 21:26 ` kernel test robot
-1 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2021-07-13 21:26 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6452 bytes --]
Hi Baruch,
I love your patch! Yet something to improve:
[auto build test ERROR on pwm/for-next]
[also build test ERROR on robh/for-next v5.14-rc1 next-20210713]
[cannot apply to agross-msm/qcom/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Baruch-Siach/arm64-dts-ipq6018-correct-TCSR-block-area/20210713-193616
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: riscv-randconfig-r032-20210713 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 8d69635ed9ecf36fd0ca85906bfde17949671cbe)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install riscv cross compiling tool for clang build
# apt-get install binutils-riscv64-linux-gnu
# https://github.com/0day-ci/linux/commit/3215e41e0c2fbd26202f21458ea6f1993f90e126
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Baruch-Siach/arm64-dts-ipq6018-correct-TCSR-block-area/20210713-193616
git checkout 3215e41e0c2fbd26202f21458ea6f1993f90e126
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/pwm/pwm-ipq.c:96:8: error: implicit declaration of function 'FIELD_PREP' [-Werror,-Wimplicit-function-declaration]
val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
^
>> drivers/pwm/pwm-ipq.c:182:12: error: implicit declaration of function 'FIELD_GET' [-Werror,-Wimplicit-function-declaration]
pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
^
2 errors generated.
vim +/FIELD_PREP +96 drivers/pwm/pwm-ipq.c
80
81 static void config_div_and_duty(struct pwm_device *pwm, unsigned int pre_div,
82 unsigned int pwm_div, u64 period_ns, u64 duty_ns,
83 bool enable)
84 {
85 unsigned long hi_dur;
86 unsigned long long quotient;
87 unsigned long val = 0;
88
89 /*
90 * high duration = pwm duty * (pwm div + 1)
91 * pwm duty = duty_ns / period_ns
92 */
93 quotient = (pwm_div + 1) * duty_ns;
94 hi_dur = div64_u64(quotient, period_ns);
95
> 96 val = FIELD_PREP(IPQ_PWM_REG0_HI_DURATION, hi_dur) |
97 FIELD_PREP(IPQ_PWM_REG0_PWM_DIV, pwm_div);
98 ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG0, val);
99
100 val = FIELD_PREP(IPQ_PWM_REG1_PRE_DIV, pre_div);
101 ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
102
103 /* Enable needs a separate write to REG1 */
104 val |= IPQ_PWM_REG1_UPDATE;
105 if (enable)
106 val |= IPQ_PWM_REG1_ENABLE;
107 else
108 val &= ~IPQ_PWM_REG1_ENABLE;
109 ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
110 }
111
112 static int ipq_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
113 const struct pwm_state *state)
114 {
115 struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
116 unsigned long freq;
117 unsigned int pre_div, pwm_div, close_pre_div, close_pwm_div;
118 long long diff;
119 unsigned long rate = clk_get_rate(ipq_chip->clk);
120 unsigned long min_diff = rate;
121 uint64_t fin_ps;
122 u64 period_ns, duty_ns;
123
124 if (state->period < IPQ_PWM_MIN_PERIOD_NS)
125 return -ERANGE;
126
127 period_ns = min(state->period, IPQ_PWM_MAX_PERIOD_NS);
128 duty_ns = min(state->duty_cycle, period_ns);
129
130 /* freq in Hz for period in nano second */
131 freq = div64_u64(NSEC_PER_SEC, period_ns);
132 fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
133 close_pre_div = IPQ_PWM_MAX_DIV;
134 close_pwm_div = IPQ_PWM_MAX_DIV;
135
136 for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
137 pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
138 fin_ps * (pre_div + 1));
139 pwm_div--;
140 if (pwm_div > IPQ_PWM_MAX_DIV)
141 continue;
142
143 diff = ((uint64_t)freq * (pre_div + 1) * (pwm_div + 1))
144 - (uint64_t)rate;
145
146 if (diff < 0) /* period larger than requested */
147 continue;
148 if (diff == 0) { /* bingo */
149 close_pre_div = pre_div;
150 close_pwm_div = pwm_div;
151 break;
152 }
153 if (diff < min_diff) {
154 min_diff = diff;
155 close_pre_div = pre_div;
156 close_pwm_div = pwm_div;
157 }
158 }
159
160 /* config divider values for the closest possible frequency */
161 config_div_and_duty(pwm, close_pre_div, close_pwm_div,
162 period_ns, duty_ns, state->enabled);
163
164 return 0;
165 }
166
167 static void ipq_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
168 struct pwm_state *state)
169 {
170 struct ipq_pwm_chip *ipq_chip = to_ipq_pwm_chip(chip);
171 unsigned long rate = clk_get_rate(ipq_chip->clk);
172 unsigned int pre_div, pwm_div, hi_dur;
173 u64 effective_div, hi_div;
174 u32 reg0, reg1;
175
176 reg0 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG0);
177 reg1 = ipq_pwm_reg_read(pwm, IPQ_PWM_CFG_REG1);
178
179 state->polarity = PWM_POLARITY_NORMAL;
180 state->enabled = reg1 & IPQ_PWM_REG1_ENABLE;
181
> 182 pwm_div = FIELD_GET(IPQ_PWM_REG0_PWM_DIV, reg0);
183 hi_dur = FIELD_GET(IPQ_PWM_REG0_HI_DURATION, reg0);
184 pre_div = FIELD_GET(IPQ_PWM_REG1_PRE_DIV, reg1);
185
186 effective_div = (pre_div + 1) * (pwm_div + 1);
187 state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
188
189 hi_div = hi_dur * (pre_div + 1);
190 state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
191 }
192
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29453 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 3/4] dt-bindings: pwm: add IPQ6018 binding
2021-07-13 11:35 ` Baruch Siach
@ 2021-07-14 2:40 ` Rob Herring
-1 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2021-07-14 2:40 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, Andy Gross,
Bjorn Andersson, Balaji Prakash J, Robert Marko, Kathiravan T,
linux-pwm, devicetree, linux-arm-msm, linux-arm-kernel
On Tue, Jul 13, 2021 at 02:35:44PM +0300, Baruch Siach wrote:
> DT binding for the PWM block in Qualcomm IPQ6018 SoC.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> 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 | 55 +++++++++++++++++++
> 1 file changed, 55 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..a07bfe63dc1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> @@ -0,0 +1,55 @@
> +# 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
> +
> + qcom,pwm-regs:
> + $ref: '/schemas/types.yaml#/definitions/phandle-array'
> + maxItems: 1
> + description: |
> + phandle link and offset to TCSR block
This binding should be a child of the TCSR I think as Bjorn asked.
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + const: core
> +
> +required:
> + - "#pwm-cells"
> + - compatible
> + - qcom,pwm-regs
> + - clocks
> + - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + pwm {
> + #pwm-cells = <2>;
> + compatible = "qcom,ipq6018-pwm";
> + qcom,pwm-regs = <&tcsr_q6 0xa010>;
> + clocks = <&gcc GCC_ADSS_PWM_CLK>;
> + clock-names = "core";
> + };
> + };
> --
> 2.30.2
>
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 3/4] dt-bindings: pwm: add IPQ6018 binding
@ 2021-07-14 2:40 ` Rob Herring
0 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2021-07-14 2:40 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, Andy Gross,
Bjorn Andersson, Balaji Prakash J, Robert Marko, Kathiravan T,
linux-pwm, devicetree, linux-arm-msm, linux-arm-kernel
On Tue, Jul 13, 2021 at 02:35:44PM +0300, Baruch Siach wrote:
> DT binding for the PWM block in Qualcomm IPQ6018 SoC.
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> 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 | 55 +++++++++++++++++++
> 1 file changed, 55 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..a07bfe63dc1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/ipq-pwm.yaml
> @@ -0,0 +1,55 @@
> +# 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
> +
> + qcom,pwm-regs:
> + $ref: '/schemas/types.yaml#/definitions/phandle-array'
> + maxItems: 1
> + description: |
> + phandle link and offset to TCSR block
This binding should be a child of the TCSR I think as Bjorn asked.
> +
> + clocks:
> + maxItems: 1
> +
> + clock-names:
> + const: core
> +
> +required:
> + - "#pwm-cells"
> + - compatible
> + - qcom,pwm-regs
> + - clocks
> + - clock-names
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/qcom,gcc-ipq6018.h>
> +
> + soc {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + pwm {
> + #pwm-cells = <2>;
> + compatible = "qcom,ipq6018-pwm";
> + qcom,pwm-regs = <&tcsr_q6 0xa010>;
> + clocks = <&gcc GCC_ADSS_PWM_CLK>;
> + clock-names = "core";
> + };
> + };
> --
> 2.30.2
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 1/4] arm64: dts: ipq6018: correct TCSR block area
2021-07-13 11:35 ` Baruch Siach
` (3 preceding siblings ...)
(?)
@ 2021-07-14 16:13 ` Kathiravan T
-1 siblings, 0 replies; 21+ messages in thread
From: Kathiravan T @ 2021-07-14 16:13 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Uwe Kleine-König, Lee Jones, Andy Gross,
Bjorn Andersson, Balaji Prakash J, Rob Herring, Robert Marko,
linux-pwm, devicetree, linux-arm-msm, linux-arm-kernel
On 2021-07-13 17:05, Baruch Siach wrote:
> According to Bjorn Andersson[1], &tcsr_q6 base is 0x01937000 with size
> 0x21000. Adjust qcom,halt-regs offsets (add 0x8000) to match the new
> syscon base.
>
> [1] https://lore.kernel.org/r/YLgO0Aj1d4w9EcPv@yoga
>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> ---
> v5: New patch in this series
> ---
> arch/arm64/boot/dts/qcom/ipq6018.dtsi | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> index 6ee7b99c21ec..72ac36c1be57 100644
> --- a/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq6018.dtsi
> @@ -270,9 +270,9 @@ tcsr_mutex_regs: syscon@1905000 {
> reg = <0x0 0x01905000 0x0 0x8000>;
> };
>
> - tcsr_q6: syscon@1945000 {
> + tcsr_q6: syscon@1937000 {
We can remove the q6 reference and make it as just 'tcsr'?
> compatible = "syscon";
> - reg = <0x0 0x01945000 0x0 0xe000>;
> + reg = <0x0 0x01937000 0x0 0x21000>;
> };
>
> blsp_dma: dma-controller@7884000 {
> @@ -615,7 +615,7 @@ q6v5_wcss: remoteproc@cd00000 {
> clocks = <&gcc GCC_PRNG_AHB_CLK>;
> clock-names = "prng";
>
> - qcom,halt-regs = <&tcsr_q6 0xa000 0xd000 0x0>;
> + qcom,halt-regs = <&tcsr_q6 0x12000 0x15000 0x8000>;
This seems to be not correct. 0x01945000 - 0x01937000 = 0xE000 but here
the values are adjusted with 0x8000 not with 0xE000.
>
> qcom,smem-states = <&wcss_smp2p_out 0>,
> <&wcss_smp2p_out 1>;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
2021-07-13 11:35 ` Baruch Siach
@ 2021-07-14 20:18 ` Uwe Kleine-König
-1 siblings, 0 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2021-07-14 20:18 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Lee Jones, Andy Gross, Bjorn Andersson,
Balaji Prakash J, Rob Herring, Robert Marko, Kathiravan T,
linux-pwm, devicetree, linux-arm-msm, linux-arm-kernel, kernel
[-- Attachment #1: Type: text/plain, Size: 12218 bytes --]
Hello Baruch,
On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
> --- /dev/null
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -0,0 +1,278 @@
> +// 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/math64.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +#define IPQ_PWM_MAX_DEVICES 4
This is only used once. Just doing
pwm->chip.npwm = 4;
is better in my book. Does "MAX" suggest that there are variants with
less PWMs?
> +/* The frequency range supported is 1Hz to 100MHz */
A space between number and unit is usual and makes this better readable.
> +#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
> +#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
You're assuming here that the parent clock runs at exactly the set rate.
Is this a sensible assumption? If this division didn't have an integer
result there would be rounding issues.
> +#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
> +
> +#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 1 /*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 ipq_pwm_reg_offset(struct pwm_device *pwm, unsigned reg)
> +{
> + return ((pwm->hwpwm * 2) + reg) * 4;
> +}
> +
> +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 + ipq_pwm_reg_offset(pwm, reg);
I already stumbled about this in v4 but thought I'd let you do it. As
I stumbled again I'll say something now:
I would do the register stuff as follows:
/* Each PWM has two registers, the offset for PWM #i is at 8 * #i */
#define IPQ_PWM_CFG_REG0 0
#define IPQ_PWM_CFG_REG1 4
and then do:
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;
...
this is a bit easier to understand IMHO, but might be subjective. I let
you decide if you want to change that or stay with your approach.
> + 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
> + bool enable)
> +{
> + unsigned long hi_dur;
> + unsigned long long quotient;
> + unsigned long val = 0;
> +
> + /*
> + * high duration = pwm duty * (pwm div + 1)
> + * pwm duty = duty_ns / period_ns
> + */
> + quotient = (pwm_div + 1) * duty_ns;
> + hi_dur = div64_u64(quotient, period_ns);
this division should use the actual period, not the target period.
Otherwise the result might be to small.
> + 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;
Setting this bit results in the two writes above being configured
atomically so that no mixed settings happen to the output, right?
Does the hardware complete the currently running cycle on
reconfiguration?
> + if (enable)
> + val |= IPQ_PWM_REG1_ENABLE;
> + else
> + val &= ~IPQ_PWM_REG1_ENABLE;
The else branch has no effect as val is initialized as zero above, so
please drop it.
> + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
How does the hardware behave with the ENABLE bit unset? Does it drive
the pin to zero?
> +}
> +
> +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, close_pre_div, close_pwm_div;
> + long long diff;
> + unsigned long rate = clk_get_rate(ipq_chip->clk);
> + unsigned long min_diff = rate;
> + uint64_t fin_ps;
> + u64 period_ns, duty_ns;
You have to refuse the request if state->polarity !=
PWM_POLARITY_NORMAL.
> +
> + if (state->period < IPQ_PWM_MIN_PERIOD_NS)
It's strange that you assume here the hardcoded 100 MHz but below you
use clk_get_rate(ipq_chip->clk).
> + 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);
> + fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
I don't understand that factor 1000. This just cancels with the 1000 in
the calculation of pwm_div below?! Maybe this is to soften the precision
loss?
> + close_pre_div = IPQ_PWM_MAX_DIV;
> + close_pwm_div = IPQ_PWM_MAX_DIV;
> +
> + for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
> + pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
> + fin_ps * (pre_div + 1));
Having fin_ps in the divisor results in loss of precision. When ever the
closest rounding division rounds down diff becomes negative below. So
you should round up here.
Also if you do:
pwm_div = round_up((period_ns * rate) / (NSEC_PER_SEC * (pre_div + 1)))
there is no relevant loss of precision. (You might have to care for
period_ns * rate overflowing though or argue why it doesn't overflow.)
> + pwm_div--;
> + if (pwm_div > IPQ_PWM_MAX_DIV)
> + continue;
This check can be dropped if the loop (depending on the other parameters)
does not start with pre_div = 0 but some bigger number.
> + 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 */
> + close_pre_div = pre_div;
> + close_pwm_div = pwm_div;
> + break;
> + }
> + if (diff < min_diff) {
> + min_diff = diff;
> + close_pre_div = pre_div;
> + close_pwm_div = pwm_div;
I would call these best_..._div, not close_..._div which makes the
purpose clearer.
A big pre_div results in a coarse resolution for duty_cycle. This makes
other similar drivers chose to hardcode pwm_div to its max value. At
least you should ensure that pre_div <= pwm_div.
> + }
> + }
> +
> + /* config divider values for the closest possible frequency */
> + config_div_and_duty(pwm, close_pre_div, close_pwm_div,
> + period_ns, 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);
> + effective_div = (pre_div + 1) * (pwm_div + 1);
Please add a comment here that with pre_div and pwm_div <= 0xffff the
multiplication below doesn't overflow
> + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
> +
> + hi_div = hi_dur * (pre_div + 1);
This suggests that the hardware cannot do 100% relative duty cycle if
pwm_div == 0xffff? I suggest to clamp pwm_div to 0xfffe then.
> + state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
> +}
> +
> +static struct pwm_ops ipq_pwm_ops = {
const please
> + .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;
> + struct of_phandle_args args;
> + int ret;
> +
> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, pwm);
> +
> + ret = of_parse_phandle_with_fixed_args(dev->of_node, "qcom,pwm-regs",
> + 1, 0, &args);
> + if (ret)
> + return dev_err_probe(dev, ret, "regs parse failed");
> +
> + pwm->regmap = syscon_node_to_regmap(args.np);
> + of_node_put(args.np);
> + if (IS_ERR(pwm->regmap))
> + return dev_err_probe(dev, PTR_ERR(pwm->regmap),
> + "regs map failed");
> + pwm->regmap_off = args.args[0];
Does this have to be so complicated? Why doesn't the normal approach
with the pwm being a child of the syscon device and reg = <...> work
here?
> + pwm->clk = devm_clk_get(dev, "core");
> + if (IS_ERR(pwm->clk))
> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
> + "failed to get core clock");
> +
> + ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
> + if (ret)
> + return dev_err_probe(dev, ret, "clock rate set failed");
Would it make more sense to set this in the device tree using
assigned-clock-rate?
> + 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 = IPQ_PWM_MAX_DEVICES;
> +
> + ret = pwmchip_add(&pwm->chip);
> + if (ret < 0) {
> + dev_err_probe(dev, ret, "pwmchip_add() failed\n");
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ipq_pwm_remove(struct platform_device *pdev)
> +{
> + struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(pwm->clk);
> + pwmchip_remove(&pwm->chip);
This is the wrong order. Until pwmchip_remove() returns the PWM must stay
functional, so disable the clock only after pwmchip_remove().
> +
> + 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");
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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
@ 2021-07-14 20:18 ` Uwe Kleine-König
0 siblings, 0 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2021-07-14 20:18 UTC (permalink / raw)
To: Baruch Siach
Cc: Thierry Reding, Lee Jones, Andy Gross, Bjorn Andersson,
Balaji Prakash J, Rob Herring, Robert Marko, Kathiravan T,
linux-pwm, devicetree, linux-arm-msm, linux-arm-kernel, kernel
[-- Attachment #1.1: Type: text/plain, Size: 12218 bytes --]
Hello Baruch,
On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
> --- /dev/null
> +++ b/drivers/pwm/pwm-ipq.c
> @@ -0,0 +1,278 @@
> +// 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/math64.h>
> +#include <linux/of_device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +
> +#define IPQ_PWM_MAX_DEVICES 4
This is only used once. Just doing
pwm->chip.npwm = 4;
is better in my book. Does "MAX" suggest that there are variants with
less PWMs?
> +/* The frequency range supported is 1Hz to 100MHz */
A space between number and unit is usual and makes this better readable.
> +#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
> +#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
You're assuming here that the parent clock runs at exactly the set rate.
Is this a sensible assumption? If this division didn't have an integer
result there would be rounding issues.
> +#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
> +
> +#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 1 /*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 ipq_pwm_reg_offset(struct pwm_device *pwm, unsigned reg)
> +{
> + return ((pwm->hwpwm * 2) + reg) * 4;
> +}
> +
> +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 + ipq_pwm_reg_offset(pwm, reg);
I already stumbled about this in v4 but thought I'd let you do it. As
I stumbled again I'll say something now:
I would do the register stuff as follows:
/* Each PWM has two registers, the offset for PWM #i is at 8 * #i */
#define IPQ_PWM_CFG_REG0 0
#define IPQ_PWM_CFG_REG1 4
and then do:
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;
...
this is a bit easier to understand IMHO, but might be subjective. I let
you decide if you want to change that or stay with your approach.
> + 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
> + bool enable)
> +{
> + unsigned long hi_dur;
> + unsigned long long quotient;
> + unsigned long val = 0;
> +
> + /*
> + * high duration = pwm duty * (pwm div + 1)
> + * pwm duty = duty_ns / period_ns
> + */
> + quotient = (pwm_div + 1) * duty_ns;
> + hi_dur = div64_u64(quotient, period_ns);
this division should use the actual period, not the target period.
Otherwise the result might be to small.
> + 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;
Setting this bit results in the two writes above being configured
atomically so that no mixed settings happen to the output, right?
Does the hardware complete the currently running cycle on
reconfiguration?
> + if (enable)
> + val |= IPQ_PWM_REG1_ENABLE;
> + else
> + val &= ~IPQ_PWM_REG1_ENABLE;
The else branch has no effect as val is initialized as zero above, so
please drop it.
> + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
How does the hardware behave with the ENABLE bit unset? Does it drive
the pin to zero?
> +}
> +
> +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, close_pre_div, close_pwm_div;
> + long long diff;
> + unsigned long rate = clk_get_rate(ipq_chip->clk);
> + unsigned long min_diff = rate;
> + uint64_t fin_ps;
> + u64 period_ns, duty_ns;
You have to refuse the request if state->polarity !=
PWM_POLARITY_NORMAL.
> +
> + if (state->period < IPQ_PWM_MIN_PERIOD_NS)
It's strange that you assume here the hardcoded 100 MHz but below you
use clk_get_rate(ipq_chip->clk).
> + 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);
> + fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
I don't understand that factor 1000. This just cancels with the 1000 in
the calculation of pwm_div below?! Maybe this is to soften the precision
loss?
> + close_pre_div = IPQ_PWM_MAX_DIV;
> + close_pwm_div = IPQ_PWM_MAX_DIV;
> +
> + for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
> + pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
> + fin_ps * (pre_div + 1));
Having fin_ps in the divisor results in loss of precision. When ever the
closest rounding division rounds down diff becomes negative below. So
you should round up here.
Also if you do:
pwm_div = round_up((period_ns * rate) / (NSEC_PER_SEC * (pre_div + 1)))
there is no relevant loss of precision. (You might have to care for
period_ns * rate overflowing though or argue why it doesn't overflow.)
> + pwm_div--;
> + if (pwm_div > IPQ_PWM_MAX_DIV)
> + continue;
This check can be dropped if the loop (depending on the other parameters)
does not start with pre_div = 0 but some bigger number.
> + 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 */
> + close_pre_div = pre_div;
> + close_pwm_div = pwm_div;
> + break;
> + }
> + if (diff < min_diff) {
> + min_diff = diff;
> + close_pre_div = pre_div;
> + close_pwm_div = pwm_div;
I would call these best_..._div, not close_..._div which makes the
purpose clearer.
A big pre_div results in a coarse resolution for duty_cycle. This makes
other similar drivers chose to hardcode pwm_div to its max value. At
least you should ensure that pre_div <= pwm_div.
> + }
> + }
> +
> + /* config divider values for the closest possible frequency */
> + config_div_and_duty(pwm, close_pre_div, close_pwm_div,
> + period_ns, 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);
> + effective_div = (pre_div + 1) * (pwm_div + 1);
Please add a comment here that with pre_div and pwm_div <= 0xffff the
multiplication below doesn't overflow
> + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
> +
> + hi_div = hi_dur * (pre_div + 1);
This suggests that the hardware cannot do 100% relative duty cycle if
pwm_div == 0xffff? I suggest to clamp pwm_div to 0xfffe then.
> + state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
> +}
> +
> +static struct pwm_ops ipq_pwm_ops = {
const please
> + .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;
> + struct of_phandle_args args;
> + int ret;
> +
> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
> + if (!pwm)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, pwm);
> +
> + ret = of_parse_phandle_with_fixed_args(dev->of_node, "qcom,pwm-regs",
> + 1, 0, &args);
> + if (ret)
> + return dev_err_probe(dev, ret, "regs parse failed");
> +
> + pwm->regmap = syscon_node_to_regmap(args.np);
> + of_node_put(args.np);
> + if (IS_ERR(pwm->regmap))
> + return dev_err_probe(dev, PTR_ERR(pwm->regmap),
> + "regs map failed");
> + pwm->regmap_off = args.args[0];
Does this have to be so complicated? Why doesn't the normal approach
with the pwm being a child of the syscon device and reg = <...> work
here?
> + pwm->clk = devm_clk_get(dev, "core");
> + if (IS_ERR(pwm->clk))
> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
> + "failed to get core clock");
> +
> + ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
> + if (ret)
> + return dev_err_probe(dev, ret, "clock rate set failed");
Would it make more sense to set this in the device tree using
assigned-clock-rate?
> + 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 = IPQ_PWM_MAX_DEVICES;
> +
> + ret = pwmchip_add(&pwm->chip);
> + if (ret < 0) {
> + dev_err_probe(dev, ret, "pwmchip_add() failed\n");
> + clk_disable_unprepare(pwm->clk);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ipq_pwm_remove(struct platform_device *pdev)
> +{
> + struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
> +
> + clk_disable_unprepare(pwm->clk);
> + pwmchip_remove(&pwm->chip);
This is the wrong order. Until pwmchip_remove() returns the PWM must stay
functional, so disable the clock only after pwmchip_remove().
> +
> + 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");
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
2021-07-14 20:18 ` Uwe Kleine-König
@ 2021-07-16 5:51 ` Baruch Siach
-1 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-16 5:51 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Thierry Reding, Lee Jones, Andy Gross, Bjorn Andersson,
Balaji Prakash J, Rob Herring, Robert Marko, Kathiravan T,
linux-pwm, devicetree, linux-arm-msm, linux-arm-kernel, kernel
Hi Uwe,
Thanks again for your detailed review.
I have a few comments and questions below.
On Wed, Jul 14 2021, Uwe Kleine-König wrote:
> On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-ipq.c
>> @@ -0,0 +1,278 @@
>> +// 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/math64.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +
>> +#define IPQ_PWM_MAX_DEVICES 4
>
> This is only used once. Just doing
>
> pwm->chip.npwm = 4;
>
> is better in my book. Does "MAX" suggest that there are variants with
> less PWMs?
I have no idea. I guess not. I'll drop this macro in v6.
>
>> +/* The frequency range supported is 1Hz to 100MHz */
>
> A space between number and unit is usual and makes this better readable.
Quick 'git grep' indicates that '[[:digit:]]\+MHz' is a little more
popular than '[[:digit:]]\+ MHz' in kernel code. But OK, not a big deal.
>> +#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
>> +#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
>
> You're assuming here that the parent clock runs at exactly the set rate.
> Is this a sensible assumption? If this division didn't have an integer
> result there would be rounding issues.
The code only uses this for period validity check. It saves us some code
for run-time division.
>> +#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
>> +
>> +#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 1 /*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 ipq_pwm_reg_offset(struct pwm_device *pwm, unsigned reg)
>> +{
>> + return ((pwm->hwpwm * 2) + reg) * 4;
>> +}
>> +
>> +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 + ipq_pwm_reg_offset(pwm, reg);
>
> I already stumbled about this in v4 but thought I'd let you do it. As
> I stumbled again I'll say something now:
>
> I would do the register stuff as follows:
>
> /* Each PWM has two registers, the offset for PWM #i is at 8 * #i */
> #define IPQ_PWM_CFG_REG0 0
> #define IPQ_PWM_CFG_REG1 4
>
> and then do:
>
> 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;
>
> ...
>
> this is a bit easier to understand IMHO, but might be subjective. I let
> you decide if you want to change that or stay with your approach.
>
>> + 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
>> + bool enable)
>> +{
>> + unsigned long hi_dur;
>> + unsigned long long quotient;
>> + unsigned long val = 0;
>> +
>> + /*
>> + * high duration = pwm duty * (pwm div + 1)
>> + * pwm duty = duty_ns / period_ns
>> + */
>> + quotient = (pwm_div + 1) * duty_ns;
>> + hi_dur = div64_u64(quotient, period_ns);
>
> this division should use the actual period, not the target period.
> Otherwise the result might be to small.
>
>> + 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;
>
> Setting this bit results in the two writes above being configured
> atomically so that no mixed settings happen to the output, right?
I guess so. I have no access to hardware documentation, mind you. I
first tried to do only one write to REG1, but it had no effect. The
existence of the UPDATE bit also indicates that hardware works as you
suggest.
> Does the hardware complete the currently running cycle on
> reconfiguration?
No idea.
>> + if (enable)
>> + val |= IPQ_PWM_REG1_ENABLE;
>> + else
>> + val &= ~IPQ_PWM_REG1_ENABLE;
>
> The else branch has no effect as val is initialized as zero above, so
> please drop it.
>
>> + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
>
> How does the hardware behave with the ENABLE bit unset? Does it drive
> the pin to zero?
Yes. That's what experimentation here shows. The pin is pulled up, but
the PWM keeps it low.
>> +}
>> +
>> +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, close_pre_div, close_pwm_div;
>> + long long diff;
>> + unsigned long rate = clk_get_rate(ipq_chip->clk);
>> + unsigned long min_diff = rate;
>> + uint64_t fin_ps;
>> + u64 period_ns, duty_ns;
>
> You have to refuse the request if state->polarity !=
> PWM_POLARITY_NORMAL.
>
>> +
>> + if (state->period < IPQ_PWM_MIN_PERIOD_NS)
>
> It's strange that you assume here the hardcoded 100 MHz but below you
> use clk_get_rate(ipq_chip->clk).
As I said above, this is meant to save code for the less critical
case. Should I use clk_get_rate() here as well? If we go with
assigned-clock-rates, as you suggest below, we'll have to do that
anyway.
>
>> + 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);
>> + fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
>
> I don't understand that factor 1000. This just cancels with the 1000 in
> the calculation of pwm_div below?! Maybe this is to soften the precision
> loss?
That is my understanding of the code intent.
>> + close_pre_div = IPQ_PWM_MAX_DIV;
>> + close_pwm_div = IPQ_PWM_MAX_DIV;
>> +
>> + for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
>> + pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
>> + fin_ps * (pre_div + 1));
>
> Having fin_ps in the divisor results in loss of precision. When ever the
> closest rounding division rounds down diff becomes negative below. So
> you should round up here.
>
> Also if you do:
>
> pwm_div = round_up((period_ns * rate) / (NSEC_PER_SEC * (pre_div + 1)))
>
> there is no relevant loss of precision. (You might have to care for
> period_ns * rate overflowing though or argue why it doesn't overflow.)
Looks better.
>
>> + pwm_div--;
>> + if (pwm_div > IPQ_PWM_MAX_DIV)
>> + continue;
>
> This check can be dropped if the loop (depending on the other parameters)
> does not start with pre_div = 0 but some bigger number.
That is, calculate the minimum pre_div value for which the division
above always produces pwm_div in range, right?
>
>> + 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 */
>> + close_pre_div = pre_div;
>> + close_pwm_div = pwm_div;
>> + break;
>> + }
>> + if (diff < min_diff) {
>> + min_diff = diff;
>> + close_pre_div = pre_div;
>> + close_pwm_div = pwm_div;
>
> I would call these best_..._div, not close_..._div which makes the
> purpose clearer.
>
> A big pre_div results in a coarse resolution for duty_cycle. This makes
> other similar drivers chose to hardcode pwm_div to its max value. At
> least you should ensure that pre_div <= pwm_div.
>
>> + }
>> + }
>> +
>> + /* config divider values for the closest possible frequency */
>> + config_div_and_duty(pwm, close_pre_div, close_pwm_div,
>> + period_ns, 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);
>> + effective_div = (pre_div + 1) * (pwm_div + 1);
>
> Please add a comment here that with pre_div and pwm_div <= 0xffff the
> multiplication below doesn't overflow
>
>> + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
>> +
>> + hi_div = hi_dur * (pre_div + 1);
>
> This suggests that the hardware cannot do 100% relative duty cycle if
> pwm_div == 0xffff? I suggest to clamp pwm_div to 0xfffe then.
What is "100% relative duty"? How does pwm_div clamping helps?
>> + state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
>> +}
>> +
>> +static struct pwm_ops ipq_pwm_ops = {
>
> const please
>
>> + .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;
>> + struct of_phandle_args args;
>> + int ret;
>> +
>> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + ret = of_parse_phandle_with_fixed_args(dev->of_node, "qcom,pwm-regs",
>> + 1, 0, &args);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "regs parse failed");
>> +
>> + pwm->regmap = syscon_node_to_regmap(args.np);
>> + of_node_put(args.np);
>> + if (IS_ERR(pwm->regmap))
>> + return dev_err_probe(dev, PTR_ERR(pwm->regmap),
>> + "regs map failed");
>> + pwm->regmap_off = args.args[0];
>
> Does this have to be so complicated? Why doesn't the normal approach
> with the pwm being a child of the syscon device and reg = <...> work
> here?
I'll do that in v6. That's what Bjorn originally suggested in response
to v2.
>
>> + pwm->clk = devm_clk_get(dev, "core");
>> + if (IS_ERR(pwm->clk))
>> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
>> + "failed to get core clock");
>> +
>> + ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "clock rate set failed");
>
> Would it make more sense to set this in the device tree using
> assigned-clock-rate?
That's 'assigned-clock-rates' I believe. I'll try that.
>
>> + 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 = IPQ_PWM_MAX_DEVICES;
>> +
>> + ret = pwmchip_add(&pwm->chip);
>> + if (ret < 0) {
>> + dev_err_probe(dev, ret, "pwmchip_add() failed\n");
>> + clk_disable_unprepare(pwm->clk);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ipq_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
>> +
>> + clk_disable_unprepare(pwm->clk);
>> + pwmchip_remove(&pwm->chip);
>
> This is the wrong order. Until pwmchip_remove() returns the PWM must stay
> functional, so disable the clock only after pwmchip_remove().
>
>> +
>> + 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");
>
> Best regards
> Uwe
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
@ 2021-07-16 5:51 ` Baruch Siach
0 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-16 5:51 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Thierry Reding, Lee Jones, Andy Gross, Bjorn Andersson,
Balaji Prakash J, Rob Herring, Robert Marko, Kathiravan T,
linux-pwm, devicetree, linux-arm-msm, linux-arm-kernel, kernel
Hi Uwe,
Thanks again for your detailed review.
I have a few comments and questions below.
On Wed, Jul 14 2021, Uwe Kleine-König wrote:
> On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-ipq.c
>> @@ -0,0 +1,278 @@
>> +// 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/math64.h>
>> +#include <linux/of_device.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/regmap.h>
>> +
>> +#define IPQ_PWM_MAX_DEVICES 4
>
> This is only used once. Just doing
>
> pwm->chip.npwm = 4;
>
> is better in my book. Does "MAX" suggest that there are variants with
> less PWMs?
I have no idea. I guess not. I'll drop this macro in v6.
>
>> +/* The frequency range supported is 1Hz to 100MHz */
>
> A space between number and unit is usual and makes this better readable.
Quick 'git grep' indicates that '[[:digit:]]\+MHz' is a little more
popular than '[[:digit:]]\+ MHz' in kernel code. But OK, not a big deal.
>> +#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
>> +#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
>
> You're assuming here that the parent clock runs at exactly the set rate.
> Is this a sensible assumption? If this division didn't have an integer
> result there would be rounding issues.
The code only uses this for period validity check. It saves us some code
for run-time division.
>> +#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
>> +
>> +#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 1 /*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 ipq_pwm_reg_offset(struct pwm_device *pwm, unsigned reg)
>> +{
>> + return ((pwm->hwpwm * 2) + reg) * 4;
>> +}
>> +
>> +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 + ipq_pwm_reg_offset(pwm, reg);
>
> I already stumbled about this in v4 but thought I'd let you do it. As
> I stumbled again I'll say something now:
>
> I would do the register stuff as follows:
>
> /* Each PWM has two registers, the offset for PWM #i is at 8 * #i */
> #define IPQ_PWM_CFG_REG0 0
> #define IPQ_PWM_CFG_REG1 4
>
> and then do:
>
> 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;
>
> ...
>
> this is a bit easier to understand IMHO, but might be subjective. I let
> you decide if you want to change that or stay with your approach.
>
>> + 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
>> + bool enable)
>> +{
>> + unsigned long hi_dur;
>> + unsigned long long quotient;
>> + unsigned long val = 0;
>> +
>> + /*
>> + * high duration = pwm duty * (pwm div + 1)
>> + * pwm duty = duty_ns / period_ns
>> + */
>> + quotient = (pwm_div + 1) * duty_ns;
>> + hi_dur = div64_u64(quotient, period_ns);
>
> this division should use the actual period, not the target period.
> Otherwise the result might be to small.
>
>> + 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;
>
> Setting this bit results in the two writes above being configured
> atomically so that no mixed settings happen to the output, right?
I guess so. I have no access to hardware documentation, mind you. I
first tried to do only one write to REG1, but it had no effect. The
existence of the UPDATE bit also indicates that hardware works as you
suggest.
> Does the hardware complete the currently running cycle on
> reconfiguration?
No idea.
>> + if (enable)
>> + val |= IPQ_PWM_REG1_ENABLE;
>> + else
>> + val &= ~IPQ_PWM_REG1_ENABLE;
>
> The else branch has no effect as val is initialized as zero above, so
> please drop it.
>
>> + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
>
> How does the hardware behave with the ENABLE bit unset? Does it drive
> the pin to zero?
Yes. That's what experimentation here shows. The pin is pulled up, but
the PWM keeps it low.
>> +}
>> +
>> +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, close_pre_div, close_pwm_div;
>> + long long diff;
>> + unsigned long rate = clk_get_rate(ipq_chip->clk);
>> + unsigned long min_diff = rate;
>> + uint64_t fin_ps;
>> + u64 period_ns, duty_ns;
>
> You have to refuse the request if state->polarity !=
> PWM_POLARITY_NORMAL.
>
>> +
>> + if (state->period < IPQ_PWM_MIN_PERIOD_NS)
>
> It's strange that you assume here the hardcoded 100 MHz but below you
> use clk_get_rate(ipq_chip->clk).
As I said above, this is meant to save code for the less critical
case. Should I use clk_get_rate() here as well? If we go with
assigned-clock-rates, as you suggest below, we'll have to do that
anyway.
>
>> + 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);
>> + fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
>
> I don't understand that factor 1000. This just cancels with the 1000 in
> the calculation of pwm_div below?! Maybe this is to soften the precision
> loss?
That is my understanding of the code intent.
>> + close_pre_div = IPQ_PWM_MAX_DIV;
>> + close_pwm_div = IPQ_PWM_MAX_DIV;
>> +
>> + for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
>> + pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
>> + fin_ps * (pre_div + 1));
>
> Having fin_ps in the divisor results in loss of precision. When ever the
> closest rounding division rounds down diff becomes negative below. So
> you should round up here.
>
> Also if you do:
>
> pwm_div = round_up((period_ns * rate) / (NSEC_PER_SEC * (pre_div + 1)))
>
> there is no relevant loss of precision. (You might have to care for
> period_ns * rate overflowing though or argue why it doesn't overflow.)
Looks better.
>
>> + pwm_div--;
>> + if (pwm_div > IPQ_PWM_MAX_DIV)
>> + continue;
>
> This check can be dropped if the loop (depending on the other parameters)
> does not start with pre_div = 0 but some bigger number.
That is, calculate the minimum pre_div value for which the division
above always produces pwm_div in range, right?
>
>> + 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 */
>> + close_pre_div = pre_div;
>> + close_pwm_div = pwm_div;
>> + break;
>> + }
>> + if (diff < min_diff) {
>> + min_diff = diff;
>> + close_pre_div = pre_div;
>> + close_pwm_div = pwm_div;
>
> I would call these best_..._div, not close_..._div which makes the
> purpose clearer.
>
> A big pre_div results in a coarse resolution for duty_cycle. This makes
> other similar drivers chose to hardcode pwm_div to its max value. At
> least you should ensure that pre_div <= pwm_div.
>
>> + }
>> + }
>> +
>> + /* config divider values for the closest possible frequency */
>> + config_div_and_duty(pwm, close_pre_div, close_pwm_div,
>> + period_ns, 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);
>> + effective_div = (pre_div + 1) * (pwm_div + 1);
>
> Please add a comment here that with pre_div and pwm_div <= 0xffff the
> multiplication below doesn't overflow
>
>> + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
>> +
>> + hi_div = hi_dur * (pre_div + 1);
>
> This suggests that the hardware cannot do 100% relative duty cycle if
> pwm_div == 0xffff? I suggest to clamp pwm_div to 0xfffe then.
What is "100% relative duty"? How does pwm_div clamping helps?
>> + state->duty_cycle = div64_u64(hi_div * NSEC_PER_SEC, rate);
>> +}
>> +
>> +static struct pwm_ops ipq_pwm_ops = {
>
> const please
>
>> + .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;
>> + struct of_phandle_args args;
>> + int ret;
>> +
>> + pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
>> + if (!pwm)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, pwm);
>> +
>> + ret = of_parse_phandle_with_fixed_args(dev->of_node, "qcom,pwm-regs",
>> + 1, 0, &args);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "regs parse failed");
>> +
>> + pwm->regmap = syscon_node_to_regmap(args.np);
>> + of_node_put(args.np);
>> + if (IS_ERR(pwm->regmap))
>> + return dev_err_probe(dev, PTR_ERR(pwm->regmap),
>> + "regs map failed");
>> + pwm->regmap_off = args.args[0];
>
> Does this have to be so complicated? Why doesn't the normal approach
> with the pwm being a child of the syscon device and reg = <...> work
> here?
I'll do that in v6. That's what Bjorn originally suggested in response
to v2.
>
>> + pwm->clk = devm_clk_get(dev, "core");
>> + if (IS_ERR(pwm->clk))
>> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
>> + "failed to get core clock");
>> +
>> + ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "clock rate set failed");
>
> Would it make more sense to set this in the device tree using
> assigned-clock-rate?
That's 'assigned-clock-rates' I believe. I'll try that.
>
>> + 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 = IPQ_PWM_MAX_DEVICES;
>> +
>> + ret = pwmchip_add(&pwm->chip);
>> + if (ret < 0) {
>> + dev_err_probe(dev, ret, "pwmchip_add() failed\n");
>> + clk_disable_unprepare(pwm->clk);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int ipq_pwm_remove(struct platform_device *pdev)
>> +{
>> + struct ipq_pwm_chip *pwm = platform_get_drvdata(pdev);
>> +
>> + clk_disable_unprepare(pwm->clk);
>> + pwmchip_remove(&pwm->chip);
>
> This is the wrong order. Until pwmchip_remove() returns the PWM must stay
> functional, so disable the clock only after pwmchip_remove().
>
>> +
>> + 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");
>
> Best regards
> Uwe
--
~. .~ 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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
2021-07-16 5:51 ` Baruch Siach
@ 2021-07-16 7:04 ` Uwe Kleine-König
-1 siblings, 0 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2021-07-16 7:04 UTC (permalink / raw)
To: Baruch Siach
Cc: linux-pwm, devicetree, linux-arm-msm, Andy Gross, Rob Herring,
Bjorn Andersson, Kathiravan T, Thierry Reding, kernel,
Robert Marko, Lee Jones, linux-arm-kernel, Balaji Prakash J
[-- Attachment #1: Type: text/plain, Size: 9815 bytes --]
Hello Baruch,
On Fri, Jul 16, 2021 at 08:51:20AM +0300, Baruch Siach wrote:
> On Wed, Jul 14 2021, Uwe Kleine-König wrote:
> > On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
> >> +/* The frequency range supported is 1Hz to 100MHz */
> >
> > A space between number and unit is usual and makes this better readable.
>
> Quick 'git grep' indicates that '[[:digit:]]\+MHz' is a little more
> popular than '[[:digit:]]\+ MHz' in kernel code. But OK, not a big deal.
"usual" was not meant in the sense "How it is used in the kernel" but
what the typesetting rules say. (Not 100% sure about English, but in
German you're supposed to add a space.)
> >> +#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
> >> +#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
> >
> > You're assuming here that the parent clock runs at exactly the set rate.
> > Is this a sensible assumption? If this division didn't have an integer
> > result there would be rounding issues.
>
> The code only uses this for period validity check. It saves us some code
> for run-time division.
This check is only completely right if the clock really runs at 100 MHz,
and I'd prefer correct over saving a division. (If you know the clock
will run at 100 MHz for sure, you can better hard code it everywhere
giving the compiler the opportunity to optimize.) So the TL;DR here is:
use one or the other and use that one consistently.
> >> + 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
> >> + bool enable)
> >> +{
> >> + unsigned long hi_dur;
> >> + unsigned long long quotient;
> >> + unsigned long val = 0;
> >> +
> >> + /*
> >> + * high duration = pwm duty * (pwm div + 1)
> >> + * pwm duty = duty_ns / period_ns
> >> + */
> >> + quotient = (pwm_div + 1) * duty_ns;
> >> + hi_dur = div64_u64(quotient, period_ns);
> >
> > this division should use the actual period, not the target period.
> > Otherwise the result might be to small.
I just noticed: Using the period here is also bad for precision as the
actual period is the result of a division.
> >> + 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;
> >
> > Setting this bit results in the two writes above being configured
> > atomically so that no mixed settings happen to the output, right?
>
> I guess so. I have no access to hardware documentation, mind you. I
> first tried to do only one write to REG1, but it had no effect. The
> existence of the UPDATE bit also indicates that hardware works as you
> suggest.
I wouldn't trust HW documentation here. If you have some means to
inspect the waveform this is easy to test. Depending on how long you can
make the periods an LED is enough. If you start with a slower parent
clk, a big pre_div and hi_dur = 0 the LED is supposed to be off. Then
set hi_dur = pwm_div/2 which either make the LED blink slowly or keeps
off. Then setting pre_div = 2 either increased the blink frequency or it
doesn't. ...
> > Does the hardware complete the currently running cycle on
> > reconfiguration?
>
> No idea.
This is easy to test, too. If you set a big period and duty_cycle and
immediately after that set a small period and duty.
> >> + if (enable)
> >> + val |= IPQ_PWM_REG1_ENABLE;
> >> + else
> >> + val &= ~IPQ_PWM_REG1_ENABLE;
> >
> > The else branch has no effect as val is initialized as zero above, so
> > please drop it.
> >
> >> + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
> >
> > How does the hardware behave with the ENABLE bit unset? Does it drive
> > the pin to zero?
>
> Yes. That's what experimentation here shows. The pin is pulled up, but
> the PWM keeps it low.
And with polarity set to inverted the PWM pulls the line up? As the
different hardwares behave differently and some consumers have
expectations here, having this documented would be great.
> >> +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, close_pre_div, close_pwm_div;
> >> + long long diff;
> >> + unsigned long rate = clk_get_rate(ipq_chip->clk);
> >> + unsigned long min_diff = rate;
> >> + uint64_t fin_ps;
> >> + u64 period_ns, duty_ns;
> >
> > You have to refuse the request if state->polarity !=
> > PWM_POLARITY_NORMAL.
> >
> >> +
> >> + if (state->period < IPQ_PWM_MIN_PERIOD_NS)
> >
> > It's strange that you assume here the hardcoded 100 MHz but below you
> > use clk_get_rate(ipq_chip->clk).
>
> As I said above, this is meant to save code for the less critical
> case. Should I use clk_get_rate() here as well? If we go with
> assigned-clock-rates, as you suggest below, we'll have to do that
> anyway.
Sounds right. (That is: use assigned-clock-rates + use clk_get_rate
consistently)
> >> + 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);
> >> + fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
> >
> > I don't understand that factor 1000. This just cancels with the 1000 in
> > the calculation of pwm_div below?! Maybe this is to soften the precision
> > loss?
>
> That is my understanding of the code intent.
>
> >> + close_pre_div = IPQ_PWM_MAX_DIV;
> >> + close_pwm_div = IPQ_PWM_MAX_DIV;
> >> +
> >> + for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
> >> + pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
> >> + fin_ps * (pre_div + 1));
> >
> > Having fin_ps in the divisor results in loss of precision. When ever the
> > closest rounding division rounds down diff becomes negative below. So
> > you should round up here.
> >
> > Also if you do:
> >
> > pwm_div = round_up((period_ns * rate) / (NSEC_PER_SEC * (pre_div + 1)))
> >
> > there is no relevant loss of precision. (You might have to care for
> > period_ns * rate overflowing though or argue why it doesn't overflow.)
>
> Looks better.
And doesn't need the factor 1000 to improve precision \o/
> >> + pwm_div--;
> >> + if (pwm_div > IPQ_PWM_MAX_DIV)
> >> + continue;
> >
> > This check can be dropped if the loop (depending on the other parameters)
> > does not start with pre_div = 0 but some bigger number.
>
> That is, calculate the minimum pre_div value for which the division
> above always produces pwm_div in range, right?
Yes, that was my idea. I didn't do the math but expect this not to be so
difficult.
> [...]
> >> +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);
> >> + effective_div = (pre_div + 1) * (pwm_div + 1);
> >
> > Please add a comment here that with pre_div and pwm_div <= 0xffff the
> > multiplication below doesn't overflow
> >
> >> + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
> >> +
> >> + hi_div = hi_dur * (pre_div + 1);
> >
> > This suggests that the hardware cannot do 100% relative duty cycle if
> > pwm_div == 0xffff? I suggest to clamp pwm_div to 0xfffe then.
>
> What is "100% relative duty"? How does pwm_div clamping helps?
relative duty = duty_cycle / period. So 100% relative duty means period ==
duty_cycle. With pwm_div == 0xffff period is
0x10000 * (pre_div + 1) / rate but duty_cycle cannot achieve that as the
maximum is 0xffff * (pre_div + 1) / rate.
> >> + pwm->clk = devm_clk_get(dev, "core");
> >> + if (IS_ERR(pwm->clk))
> >> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
> >> + "failed to get core clock");
> >> +
> >> + ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
> >> + if (ret)
> >> + return dev_err_probe(dev, ret, "clock rate set failed");
> >
> > Would it make more sense to set this in the device tree using
> > assigned-clock-rate?
>
> That's 'assigned-clock-rates' I believe. I'll try that.
Ah right, I missed the s.
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 --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
@ 2021-07-16 7:04 ` Uwe Kleine-König
0 siblings, 0 replies; 21+ messages in thread
From: Uwe Kleine-König @ 2021-07-16 7:04 UTC (permalink / raw)
To: Baruch Siach
Cc: linux-pwm, devicetree, linux-arm-msm, Andy Gross, Rob Herring,
Bjorn Andersson, Kathiravan T, Thierry Reding, kernel,
Robert Marko, Lee Jones, linux-arm-kernel, Balaji Prakash J
[-- Attachment #1.1: Type: text/plain, Size: 9815 bytes --]
Hello Baruch,
On Fri, Jul 16, 2021 at 08:51:20AM +0300, Baruch Siach wrote:
> On Wed, Jul 14 2021, Uwe Kleine-König wrote:
> > On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
> >> +/* The frequency range supported is 1Hz to 100MHz */
> >
> > A space between number and unit is usual and makes this better readable.
>
> Quick 'git grep' indicates that '[[:digit:]]\+MHz' is a little more
> popular than '[[:digit:]]\+ MHz' in kernel code. But OK, not a big deal.
"usual" was not meant in the sense "How it is used in the kernel" but
what the typesetting rules say. (Not 100% sure about English, but in
German you're supposed to add a space.)
> >> +#define IPQ_PWM_CLK_SRC_FREQ (100*1000*1000)
> >> +#define IPQ_PWM_MIN_PERIOD_NS (NSEC_PER_SEC / IPQ_PWM_CLK_SRC_FREQ)
> >
> > You're assuming here that the parent clock runs at exactly the set rate.
> > Is this a sensible assumption? If this division didn't have an integer
> > result there would be rounding issues.
>
> The code only uses this for period validity check. It saves us some code
> for run-time division.
This check is only completely right if the clock really runs at 100 MHz,
and I'd prefer correct over saving a division. (If you know the clock
will run at 100 MHz for sure, you can better hard code it everywhere
giving the compiler the opportunity to optimize.) So the TL;DR here is:
use one or the other and use that one consistently.
> >> + 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 + ipq_pwm_reg_offset(pwm, 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, u64 period_ns, u64 duty_ns,
> >> + bool enable)
> >> +{
> >> + unsigned long hi_dur;
> >> + unsigned long long quotient;
> >> + unsigned long val = 0;
> >> +
> >> + /*
> >> + * high duration = pwm duty * (pwm div + 1)
> >> + * pwm duty = duty_ns / period_ns
> >> + */
> >> + quotient = (pwm_div + 1) * duty_ns;
> >> + hi_dur = div64_u64(quotient, period_ns);
> >
> > this division should use the actual period, not the target period.
> > Otherwise the result might be to small.
I just noticed: Using the period here is also bad for precision as the
actual period is the result of a division.
> >> + 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;
> >
> > Setting this bit results in the two writes above being configured
> > atomically so that no mixed settings happen to the output, right?
>
> I guess so. I have no access to hardware documentation, mind you. I
> first tried to do only one write to REG1, but it had no effect. The
> existence of the UPDATE bit also indicates that hardware works as you
> suggest.
I wouldn't trust HW documentation here. If you have some means to
inspect the waveform this is easy to test. Depending on how long you can
make the periods an LED is enough. If you start with a slower parent
clk, a big pre_div and hi_dur = 0 the LED is supposed to be off. Then
set hi_dur = pwm_div/2 which either make the LED blink slowly or keeps
off. Then setting pre_div = 2 either increased the blink frequency or it
doesn't. ...
> > Does the hardware complete the currently running cycle on
> > reconfiguration?
>
> No idea.
This is easy to test, too. If you set a big period and duty_cycle and
immediately after that set a small period and duty.
> >> + if (enable)
> >> + val |= IPQ_PWM_REG1_ENABLE;
> >> + else
> >> + val &= ~IPQ_PWM_REG1_ENABLE;
> >
> > The else branch has no effect as val is initialized as zero above, so
> > please drop it.
> >
> >> + ipq_pwm_reg_write(pwm, IPQ_PWM_CFG_REG1, val);
> >
> > How does the hardware behave with the ENABLE bit unset? Does it drive
> > the pin to zero?
>
> Yes. That's what experimentation here shows. The pin is pulled up, but
> the PWM keeps it low.
And with polarity set to inverted the PWM pulls the line up? As the
different hardwares behave differently and some consumers have
expectations here, having this documented would be great.
> >> +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, close_pre_div, close_pwm_div;
> >> + long long diff;
> >> + unsigned long rate = clk_get_rate(ipq_chip->clk);
> >> + unsigned long min_diff = rate;
> >> + uint64_t fin_ps;
> >> + u64 period_ns, duty_ns;
> >
> > You have to refuse the request if state->polarity !=
> > PWM_POLARITY_NORMAL.
> >
> >> +
> >> + if (state->period < IPQ_PWM_MIN_PERIOD_NS)
> >
> > It's strange that you assume here the hardcoded 100 MHz but below you
> > use clk_get_rate(ipq_chip->clk).
>
> As I said above, this is meant to save code for the less critical
> case. Should I use clk_get_rate() here as well? If we go with
> assigned-clock-rates, as you suggest below, we'll have to do that
> anyway.
Sounds right. (That is: use assigned-clock-rates + use clk_get_rate
consistently)
> >> + 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);
> >> + fin_ps = div64_u64(NSEC_PER_SEC * 1000ULL, rate);
> >
> > I don't understand that factor 1000. This just cancels with the 1000 in
> > the calculation of pwm_div below?! Maybe this is to soften the precision
> > loss?
>
> That is my understanding of the code intent.
>
> >> + close_pre_div = IPQ_PWM_MAX_DIV;
> >> + close_pwm_div = IPQ_PWM_MAX_DIV;
> >> +
> >> + for (pre_div = 0; pre_div <= IPQ_PWM_MAX_DIV; pre_div++) {
> >> + pwm_div = DIV64_U64_ROUND_CLOSEST(period_ns * 1000,
> >> + fin_ps * (pre_div + 1));
> >
> > Having fin_ps in the divisor results in loss of precision. When ever the
> > closest rounding division rounds down diff becomes negative below. So
> > you should round up here.
> >
> > Also if you do:
> >
> > pwm_div = round_up((period_ns * rate) / (NSEC_PER_SEC * (pre_div + 1)))
> >
> > there is no relevant loss of precision. (You might have to care for
> > period_ns * rate overflowing though or argue why it doesn't overflow.)
>
> Looks better.
And doesn't need the factor 1000 to improve precision \o/
> >> + pwm_div--;
> >> + if (pwm_div > IPQ_PWM_MAX_DIV)
> >> + continue;
> >
> > This check can be dropped if the loop (depending on the other parameters)
> > does not start with pre_div = 0 but some bigger number.
>
> That is, calculate the minimum pre_div value for which the division
> above always produces pwm_div in range, right?
Yes, that was my idea. I didn't do the math but expect this not to be so
difficult.
> [...]
> >> +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);
> >> + effective_div = (pre_div + 1) * (pwm_div + 1);
> >
> > Please add a comment here that with pre_div and pwm_div <= 0xffff the
> > multiplication below doesn't overflow
> >
> >> + state->period = div64_u64(effective_div * NSEC_PER_SEC, rate);
> >> +
> >> + hi_div = hi_dur * (pre_div + 1);
> >
> > This suggests that the hardware cannot do 100% relative duty cycle if
> > pwm_div == 0xffff? I suggest to clamp pwm_div to 0xfffe then.
>
> What is "100% relative duty"? How does pwm_div clamping helps?
relative duty = duty_cycle / period. So 100% relative duty means period ==
duty_cycle. With pwm_div == 0xffff period is
0x10000 * (pre_div + 1) / rate but duty_cycle cannot achieve that as the
maximum is 0xffff * (pre_div + 1) / rate.
> >> + pwm->clk = devm_clk_get(dev, "core");
> >> + if (IS_ERR(pwm->clk))
> >> + return dev_err_probe(dev, PTR_ERR(pwm->clk),
> >> + "failed to get core clock");
> >> +
> >> + ret = clk_set_rate(pwm->clk, IPQ_PWM_CLK_SRC_FREQ);
> >> + if (ret)
> >> + return dev_err_probe(dev, ret, "clock rate set failed");
> >
> > Would it make more sense to set this in the device tree using
> > assigned-clock-rate?
>
> That's 'assigned-clock-rates' I believe. I'll try that.
Ah right, I missed the s.
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
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
2021-07-16 7:04 ` Uwe Kleine-König
@ 2021-07-22 10:08 ` Baruch Siach
-1 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-22 10:08 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, devicetree, linux-arm-msm, Andy Gross, Rob Herring,
Bjorn Andersson, Kathiravan T, Thierry Reding, kernel,
Robert Marko, Lee Jones, linux-arm-kernel, Balaji Prakash J
Hi Uwe,
On Fri, Jul 16 2021, Uwe Kleine-König wrote:
> On Fri, Jul 16, 2021 at 08:51:20AM +0300, Baruch Siach wrote:
>> On Wed, Jul 14 2021, Uwe Kleine-König wrote:
>> > On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
>> >> + 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;
>> >
>> > Setting this bit results in the two writes above being configured
>> > atomically so that no mixed settings happen to the output, right?
>>
>> I guess so. I have no access to hardware documentation, mind you. I
>> first tried to do only one write to REG1, but it had no effect. The
>> existence of the UPDATE bit also indicates that hardware works as you
>> suggest.
>
> I wouldn't trust HW documentation here. If you have some means to
> inspect the waveform this is easy to test. Depending on how long you can
> make the periods an LED is enough. If you start with a slower parent
> clk, a big pre_div and hi_dur = 0 the LED is supposed to be off. Then
> set hi_dur = pwm_div/2 which either make the LED blink slowly or keeps
> off. Then setting pre_div = 2 either increased the blink frequency or it
> doesn't. ...
I currently have only access to DVM to measure the PWM effect. I'll try
to do more measures when I have access to better equipment.
baruch
--
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block
@ 2021-07-22 10:08 ` Baruch Siach
0 siblings, 0 replies; 21+ messages in thread
From: Baruch Siach @ 2021-07-22 10:08 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, devicetree, linux-arm-msm, Andy Gross, Rob Herring,
Bjorn Andersson, Kathiravan T, Thierry Reding, kernel,
Robert Marko, Lee Jones, linux-arm-kernel, Balaji Prakash J
Hi Uwe,
On Fri, Jul 16 2021, Uwe Kleine-König wrote:
> On Fri, Jul 16, 2021 at 08:51:20AM +0300, Baruch Siach wrote:
>> On Wed, Jul 14 2021, Uwe Kleine-König wrote:
>> > On Tue, Jul 13, 2021 at 02:35:43PM +0300, Baruch Siach wrote:
>> >> + 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;
>> >
>> > Setting this bit results in the two writes above being configured
>> > atomically so that no mixed settings happen to the output, right?
>>
>> I guess so. I have no access to hardware documentation, mind you. I
>> first tried to do only one write to REG1, but it had no effect. The
>> existence of the UPDATE bit also indicates that hardware works as you
>> suggest.
>
> I wouldn't trust HW documentation here. If you have some means to
> inspect the waveform this is easy to test. Depending on how long you can
> make the periods an LED is enough. If you start with a slower parent
> clk, a big pre_div and hi_dur = 0 the LED is supposed to be off. Then
> set hi_dur = pwm_div/2 which either make the LED blink slowly or keeps
> off. Then setting pre_div = 2 either increased the blink frequency or it
> doesn't. ...
I currently have only access to DVM to measure the PWM effect. I'll try
to do more measures when I have access to better equipment.
baruch
--
~. .~ 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
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-07-22 10:14 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-13 11:35 [PATCH v5 1/4] arm64: dts: ipq6018: correct TCSR block area Baruch Siach
2021-07-13 11:35 ` Baruch Siach
2021-07-13 11:35 ` [PATCH v5 2/4] pwm: driver for qualcomm ipq6018 pwm block Baruch Siach
2021-07-13 11:35 ` Baruch Siach
2021-07-13 18:07 ` kernel test robot
2021-07-13 21:26 ` kernel test robot
2021-07-14 20:18 ` Uwe Kleine-König
2021-07-14 20:18 ` Uwe Kleine-König
2021-07-16 5:51 ` Baruch Siach
2021-07-16 5:51 ` Baruch Siach
2021-07-16 7:04 ` Uwe Kleine-König
2021-07-16 7:04 ` Uwe Kleine-König
2021-07-22 10:08 ` Baruch Siach
2021-07-22 10:08 ` Baruch Siach
2021-07-13 11:35 ` [PATCH v5 3/4] dt-bindings: pwm: add IPQ6018 binding Baruch Siach
2021-07-13 11:35 ` Baruch Siach
2021-07-14 2:40 ` Rob Herring
2021-07-14 2:40 ` Rob Herring
2021-07-13 11:35 ` [PATCH v5 4/4] arm64: dts: ipq6018: add pwm node Baruch Siach
2021-07-13 11:35 ` Baruch Siach
2021-07-14 16:13 ` [PATCH v5 1/4] arm64: dts: ipq6018: correct TCSR block area Kathiravan T
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.