diff mbox series

[v6,2/4] pwm: opencores: Add PWM driver support

Message ID 20231020103741.557735-3-william.qiu@starfivetech.com
State New
Headers show
Series [v6,1/4] dt-bindings: pwm: Add OpenCores PWM module | expand

Commit Message

William Qiu Oct. 20, 2023, 10:37 a.m. UTC
Add Pulse Width Modulation driver support for OpenCores.

Co-developed-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: Hal Feng <hal.feng@starfivetech.com>
Signed-off-by: William Qiu <william.qiu@starfivetech.com>
---
 MAINTAINERS              |   7 ++
 drivers/pwm/Kconfig      |  11 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-ocores.c | 211 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 230 insertions(+)
 create mode 100644 drivers/pwm/pwm-ocores.c

Comments

Uwe Kleine-König Oct. 27, 2023, 1:23 p.m. UTC | #1
Hello William,

On Fri, Oct 27, 2023 at 06:23:58PM +0800, William Qiu wrote:
> [...]
> Maybe depend on STARFIVE'S SoCs first?

If these are for now the only known implementers, that sounds about
right.

Best regards
Uwe
William Qiu Nov. 1, 2023, 2:22 a.m. UTC | #2
On 2023/10/20 19:25, Uwe Kleine-König wrote:
>> +	void __iomem *base = pwm->data->get_ch_base ?
>> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
>> +	u32 period_data, duty_data, ctrl_data;
>> +
>> +	period_data = readl(REG_OCPWM_LRC(base));
>> +	duty_data = readl(REG_OCPWM_HRC(base));
>> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
>> +
>> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
>> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> 
> Please test your driver with PWM_DEBUG enabled. The rounding is wrong
> here.
> 

Hi Uwe,

The conclusion after checking is: when the period or duty_cycle value set
by the user is not divisible (1000000000/49.5M), there will be an error.
This error is due to hardware accuracy. So why is rounding is wrong?
rockchip also has a similar implementation drivers/pwm/ pwm-rockchip.c

Best regards,
William
Uwe Kleine-König Nov. 2, 2023, 11:30 a.m. UTC | #3
Hello William,

On Wed, Nov 01, 2023 at 10:22:44AM +0800, William Qiu wrote:
> 
> 
> On 2023/10/20 19:25, Uwe Kleine-König wrote:
> >> +	void __iomem *base = pwm->data->get_ch_base ?
> >> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
> >> +	u32 period_data, duty_data, ctrl_data;
> >> +
> >> +	period_data = readl(REG_OCPWM_LRC(base));
> >> +	duty_data = readl(REG_OCPWM_HRC(base));
> >> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
> >> +
> >> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
> >> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
> > 
> > Please test your driver with PWM_DEBUG enabled. The rounding is wrong
> > here.
> 
> The conclusion after checking is: when the period or duty_cycle value set
> by the user is not divisible (1000000000/49.5M), there will be an error.
> This error is due to hardware accuracy. So why is rounding is wrong?
> rockchip also has a similar implementation drivers/pwm/ pwm-rockchip.c

I fail to follow. Where is an error?

The general policy (for new drivers at least) is to implement the
biggest period possible not bigger than the requested period. That means
that .apply must round down and to make .apply ∘ .get_state idempotent
.get_state must round up to match.

Assuming a clkrate of 49500000 Hz the actual period for REG_OCPWM_LRC =
400 is 8080.808ns and for REG_OCPWM_LRC = 401 is 8101.010.

So with REG_OCPWM_LRC = 401 .get_state should report state.period = 8102
[ns] because if you call .apply with .period = 8101 [ns] you're supposed
to use REG_OCPWM_LRC = 400.

Rounding using DIV_ROUND_CLOSEST doesn't give consistent behaviour in
some cases. Consider a PWM that can implement the following periods (and
none in between):

	20.1 ns
	20.4 ns
	21.7 ns

With round-to-nearest a request to configure 21 ns will yield 20.4 ns.
If you call .get_state there the driver will return 20 ns. However
configuring 20 ns results in a period of 20.1 ns.

With rounding as requested above you get a consistent behaviour. After
.apply_state(period=21) .get_state() returns period=21.

Best regards
Uwe
William Qiu Nov. 6, 2023, 7:26 a.m. UTC | #4
On 2023/11/2 19:30, Uwe Kleine-König wrote:
> Hello William,
> 
> On Wed, Nov 01, 2023 at 10:22:44AM +0800, William Qiu wrote:
>> 
>> 
>> On 2023/10/20 19:25, Uwe Kleine-König wrote:
>> >> +	void __iomem *base = pwm->data->get_ch_base ?
>> >> +			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
>> >> +	u32 period_data, duty_data, ctrl_data;
>> >> +
>> >> +	period_data = readl(REG_OCPWM_LRC(base));
>> >> +	duty_data = readl(REG_OCPWM_HRC(base));
>> >> +	ctrl_data = readl(REG_OCPWM_CTRL(base));
>> >> +
>> >> +	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
>> >> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
>> > 
>> > Please test your driver with PWM_DEBUG enabled. The rounding is wrong
>> > here.
>> 
>> The conclusion after checking is: when the period or duty_cycle value set
>> by the user is not divisible (1000000000/49.5M), there will be an error.
>> This error is due to hardware accuracy. So why is rounding is wrong?
>> rockchip also has a similar implementation drivers/pwm/ pwm-rockchip.c
> 
> I fail to follow. Where is an error?
> 
> The general policy (for new drivers at least) is to implement the
> biggest period possible not bigger than the requested period. That means
> that .apply must round down and to make .apply ∘ .get_state idempotent
> .get_state must round up to match.
> 
> Assuming a clkrate of 49500000 Hz the actual period for REG_OCPWM_LRC =
> 400 is 8080.808ns and for REG_OCPWM_LRC = 401 is 8101.010.
> 
> So with REG_OCPWM_LRC = 401 .get_state should report state.period = 8102
> [ns] because if you call .apply with .period = 8101 [ns] you're supposed
> to use REG_OCPWM_LRC = 400.
> 
> Rounding using DIV_ROUND_CLOSEST doesn't give consistent behaviour in
> some cases. Consider a PWM that can implement the following periods (and
> none in between):
> 
> 	20.1 ns
> 	20.4 ns
> 	21.7 ns
> 
> With round-to-nearest a request to configure 21 ns will yield 20.4 ns.
> If you call .get_state there the driver will return 20 ns. However
> configuring 20 ns results in a period of 20.1 ns.
> 
> With rounding as requested above you get a consistent behaviour. After
> .apply_state(period=21) .get_state() returns period=21.
> 
> Best regards
> Uwe
> 
I see, then we'll use DIV_ROUND_DOWN_ULL for .apply() and DIV_ROUND_UP_ULL
for .get_state().
Thank you for your answer.

Best regards,
William
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 6c4cce45a09d..321af8fa7aad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16003,6 +16003,13 @@  F:	Documentation/i2c/busses/i2c-ocores.rst
 F:	drivers/i2c/busses/i2c-ocores.c
 F:	include/linux/platform_data/i2c-ocores.h

+OPENCORES PWM DRIVER
+M:	William Qiu <william.qiu@starfivetech.com>
+M:	Hal Feng <hal.feng@starfivetech.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/pwm/opencores,pwm-ocores.yaml
+F:	drivers/pwm/pwm-ocores.c
+
 OPENRISC ARCHITECTURE
 M:	Jonas Bonn <jonas@southpole.se>
 M:	Stefan Kristiansson <stefan.kristiansson@saunalahti.fi>
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ebcddf91f7b..cbfbf227d957 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -434,6 +434,17 @@  config PWM_NTXEC
 	  controller found in certain e-book readers designed by the original
 	  design manufacturer Netronix.

+config PWM_OCORES
+	tristate "Opencores PWM support"
+	depends on HAS_IOMEM && OF
+	depends on COMMON_CLK && RESET_CONTROLLER
+	help
+	  If you say yes to this option, support will be included for the
+	  OpenCores PWM. For details see https://opencores.org/projects/ptc.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ocores.
+
 config PWM_OMAP_DMTIMER
 	tristate "OMAP Dual-Mode Timer PWM support"
 	depends on OF
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c822389c2a24..542b98202153 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -39,6 +39,7 @@  obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
 obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
 obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
 obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
+obj-$(CONFIG_PWM_OCORES)	+= pwm-ocores.o
 obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
 obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
 obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
new file mode 100644
index 000000000000..7a510de4e063
--- /dev/null
+++ b/drivers/pwm/pwm-ocores.c
@@ -0,0 +1,211 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * OpenCores PWM Driver
+ *
+ * https://opencores.org/projects/ptc
+ *
+ * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#define REG_OCPWM_CNTR(base)	((base))
+#define REG_OCPWM_HRC(base)	((base) + 0x4)
+#define REG_OCPWM_LRC(base)	((base) + 0x8)
+#define REG_OCPWM_CTRL(base)	((base) + 0xC)
+
+/* OCPWM_CTRL register bits*/
+#define OCPWM_EN      BIT(0)
+#define OCPWM_ECLK    BIT(1)
+#define OCPWM_NEC     BIT(2)
+#define OCPWM_OE      BIT(3)
+#define OCPWM_SIGNLE  BIT(4)
+#define OCPWM_INTE    BIT(5)
+#define OCPWM_INT     BIT(6)
+#define OCPWM_CNTRRST BIT(7)
+#define OCPWM_CAPTE   BIT(8)
+
+struct ocores_pwm_device {
+	struct pwm_chip chip;
+	struct clk *clk;
+	struct reset_control *rst;
+	const struct ocores_pwm_data *data;
+	void __iomem *regs;
+	u32 clk_rate; /* PWM APB clock frequency */
+};
+
+struct ocores_pwm_data {
+	void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel);
+};
+
+static inline struct ocores_pwm_device *
+chip_to_ocores(struct pwm_chip *chip)
+
+{
+	return container_of(chip, struct ocores_pwm_device, chip);
+}
+
+void __iomem *starfive_jh71x0_get_ch_base(void __iomem *base,
+					  unsigned int channel)
+{
+	return base + (channel > 3 ? channel % 4 * 0x10 + (1 << 15) : channel * 0x10);
+}
+
+static int ocores_pwm_get_state(struct pwm_chip *chip,
+				struct pwm_device *dev,
+				struct pwm_state *state)
+{
+	struct ocores_pwm_device *pwm = chip_to_ocores(chip);
+	void __iomem *base = pwm->data->get_ch_base ?
+			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
+	u32 period_data, duty_data, ctrl_data;
+
+	period_data = readl(REG_OCPWM_LRC(base));
+	duty_data = readl(REG_OCPWM_HRC(base));
+	ctrl_data = readl(REG_OCPWM_CTRL(base));
+
+	state->period = DIV_ROUND_CLOSEST_ULL((u64)period_data * NSEC_PER_SEC, pwm->clk_rate);
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL((u64)duty_data * NSEC_PER_SEC, pwm->clk_rate);
+	state->polarity = PWM_POLARITY_INVERSED;
+	state->enabled = (ctrl_data & OCPWM_EN) ? true : false;
+
+	return 0;
+}
+
+static int ocores_pwm_apply(struct pwm_chip *chip,
+			    struct pwm_device *dev,
+			    const struct pwm_state *state)
+{
+	struct ocores_pwm_device *pwm = chip_to_ocores(chip);
+	void __iomem *base = pwm->data->get_ch_base ?
+			     pwm->data->get_ch_base(pwm->regs, dev->hwpwm) : pwm->regs;
+	u32 period_data, duty_data, ctrl_data = 0;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	period_data = DIV_ROUND_CLOSEST_ULL(state->period * pwm->clk_rate,
+					    NSEC_PER_SEC);
+	duty_data = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pwm->clk_rate,
+					  NSEC_PER_SEC);
+
+	writel(period_data, REG_OCPWM_LRC(base));
+	writel(duty_data, REG_OCPWM_HRC(base));
+	writel(0,  REG_OCPWM_CNTR(base));
+
+	ctrl_data = readl(REG_OCPWM_CTRL(base));
+	if (state->enabled)
+		writel(ctrl_data | OCPWM_EN | OCPWM_OE, REG_OCPWM_CTRL(base));
+	else
+		writel(ctrl_data & ~(OCPWM_EN | OCPWM_OE), REG_OCPWM_CTRL(base));
+
+	return 0;
+}
+
+static const struct pwm_ops ocores_pwm_ops = {
+	.get_state	= ocores_pwm_get_state,
+	.apply		= ocores_pwm_apply,
+	.owner		= THIS_MODULE,
+};
+
+static const struct ocores_pwm_data jh71x0_pwm_data = {
+	.get_ch_base = starfive_jh71x0_get_ch_base,
+};
+
+static const struct of_device_id ocores_pwm_of_match[] = {
+	{ .compatible = "opencores,pwm-ocores" },
+	{ .compatible = "starfive,jh71x0-pwm", .data = &jh71x0_pwm_data},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ocores_pwm_of_match);
+
+static int ocores_pwm_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *id;
+	struct device *dev = &pdev->dev;
+	struct ocores_pwm_device *pwm;
+	struct pwm_chip *chip;
+	int ret;
+
+	id = of_match_device(ocores_pwm_of_match, dev);
+	if (!id)
+		return -EINVAL;
+
+	pwm = devm_kzalloc(dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->data = id->data;
+	chip = &pwm->chip;
+	chip->dev = dev;
+	chip->ops = &ocores_pwm_ops;
+	chip->npwm = 8;
+	chip->of_pwm_n_cells = 3;
+
+	pwm->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pwm->regs))
+		return dev_err_probe(dev, PTR_ERR(pwm->regs),
+				     "Unable to map IO resources\n");
+
+	pwm->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(pwm->clk))
+		return dev_err_probe(dev, PTR_ERR(pwm->clk),
+				     "Unable to get pwm's clock\n");
+
+	pwm->rst = devm_reset_control_get_optional_exclusive(dev, NULL);
+	reset_control_deassert(pwm->rst);
+
+	pwm->clk_rate = clk_get_rate(pwm->clk);
+	if (pwm->clk_rate <= 0) {
+		dev_warn(dev, "Failed to get APB clock rate\n");
+		return -EINVAL;
+	}
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0) {
+		dev_err(dev, "Cannot register PTC: %d\n", ret);
+		clk_disable_unprepare(pwm->clk);
+		reset_control_assert(pwm->rst);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int ocores_pwm_remove(struct platform_device *dev)
+{
+	struct ocores_pwm_device *pwm = platform_get_drvdata(dev);
+
+	reset_control_assert(pwm->rst);
+	clk_disable_unprepare(pwm->clk);
+
+	return 0;
+}
+
+static struct platform_driver ocores_pwm_driver = {
+	.probe = ocores_pwm_probe,
+	.remove = ocores_pwm_remove,
+	.driver = {
+		.name = "ocores-pwm",
+		.of_match_table = ocores_pwm_of_match,
+	},
+};
+module_platform_driver(ocores_pwm_driver);
+
+MODULE_AUTHOR("Jieqin Chen");
+MODULE_AUTHOR("Hal Feng <hal.feng@starfivetech.com>");
+MODULE_DESCRIPTION("OpenCores PWM PTC driver");
+MODULE_LICENSE("GPL");
--
2.34.1