[PATCH v17] pwm: opencores: Add PWM driver support

[PATCH v17] pwm: opencores: Add PWM driver support

[William Qiu]

Add driver for OpenCores PWM Controller. And add compatibility code
which based on StarFive SoC.

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      |  12 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-ocores.c | 238 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 258 insertions(+)
 create mode 100644 drivers/pwm/pwm-ocores.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 910305c11e8a..e0b130e0dc54 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17537,6 +17537,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.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 0915c1e7df16..33819cb585be 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -471,6 +471,18 @@ config PWM_NTXEC
 	  controller found in certain e-book readers designed by the original
 	  design manufacturer Netronix.
 
+config PWM_OCORES
+	tristate "OpenCores PTC PWM support"
+	depends on HAS_IOMEM && OF
+	depends on COMMON_CLK
+	depends on ARCH_STARFIVE || COMPILE_TEST
+	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 9081e0c0e9e0..e55997490dcb 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -42,6 +42,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..bc957830017f
--- /dev/null
+++ b/drivers/pwm/pwm-ocores.c
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * OpenCores PWM Driver
+ *
+ * https://opencores.org/projects/ptc
+ *
+ * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
+ *
+ * Limitations:
+ * - The hardware only supports inverted polarity.
+ * - The hardware minimum period / duty_cycle is (1 / pwm_apb clock frequency).
+ * - The hardware maximum period / duty_cycle is (U32_MAX / pwm_apb clock frequency).
+ * - The output is set to a low level immediately when disabled.
+ * - When configuration changes are done, they get active immediately without resetting
+ *   the counter. This might result in one period affected by both old and new settings.
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+/* OpenCores Register offsets */
+#define REG_OCPWM_CNTR    0x0
+#define REG_OCPWM_HRC     0x4
+#define REG_OCPWM_LRC     0x8
+#define REG_OCPWM_CTRL    0xC
+
+/* OCPWM_CTRL register bits*/
+#define REG_OCPWM_CNTR_EN      BIT(0)
+#define REG_OCPWM_CNTR_ECLK    BIT(1)
+#define REG_OCPWM_CNTR_NEC     BIT(2)
+#define REG_OCPWM_CNTR_OE      BIT(3)
+#define REG_OCPWM_CNTR_SIGNLE  BIT(4)
+#define REG_OCPWM_CNTR_INTE    BIT(5)
+#define REG_OCPWM_CNTR_INT     BIT(6)
+#define REG_OCPWM_CNTR_RST     BIT(7)
+#define REG_OCPWM_CNTR_CAPTE   BIT(8)
+
+struct ocores_pwm_data {
+	void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel);
+};
+
+struct ocores_pwm_device {
+	const struct ocores_pwm_data *data;
+	void __iomem *regs;
+	u32 clk_rate; /* PWM APB clock frequency */
+};
+
+static inline u32 ocores_pwm_readl(struct ocores_pwm_device *ddata,
+				   unsigned int channel,
+				   unsigned int offset)
+{
+	void __iomem *base = ddata->data->get_ch_base ?
+			     ddata->data->get_ch_base(ddata->regs, channel) : ddata->regs;
+
+	return readl(base + offset);
+}
+
+static inline void ocores_pwm_writel(struct ocores_pwm_device *ddata,
+				     unsigned int channel,
+				     unsigned int offset, u32 val)
+{
+	void __iomem *base = ddata->data->get_ch_base ?
+			     ddata->data->get_ch_base(ddata->regs, channel) : ddata->regs;
+
+	writel(val, base + offset);
+}
+
+static inline struct ocores_pwm_device *chip_to_ocores(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static void __iomem *ocores_pwm_get_ch_base(void __iomem *base,
+					    unsigned int channel)
+{
+	unsigned int offset = (channel & 4) << 13 | (channel & 3) << 4;
+
+	return base + offset;
+}
+
+static int ocores_pwm_get_state(struct pwm_chip *chip,
+				struct pwm_device *pwm,
+				struct pwm_state *state)
+{
+	struct ocores_pwm_device *ddata = chip_to_ocores(chip);
+	u32 period_data, duty_data, ctrl_data;
+
+	period_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_LRC);
+	duty_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_HRC);
+	ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL);
+
+	state->period = DIV_ROUND_UP_ULL((u64)period_data * NSEC_PER_SEC, ddata->clk_rate);
+	state->duty_cycle = DIV_ROUND_UP_ULL((u64)duty_data * NSEC_PER_SEC, ddata->clk_rate);
+	state->polarity = PWM_POLARITY_INVERSED;
+	state->enabled = (ctrl_data & REG_OCPWM_CNTR_EN) ? true : false;
+
+	return 0;
+}
+
+static int ocores_pwm_apply(struct pwm_chip *chip,
+			    struct pwm_device *pwm,
+			    const struct pwm_state *state)
+{
+	struct ocores_pwm_device *ddata = chip_to_ocores(chip);
+	u32 ctrl_data = 0;
+	u64 period_data, duty_data;
+
+	if (state->polarity != PWM_POLARITY_INVERSED)
+		return -EINVAL;
+
+	period_data = mul_u64_u32_div(state->period, ddata->clk_rate, NSEC_PER_SEC);
+	if (!period_data)
+		return -EINVAL;
+
+	if (period_data > U32_MAX)
+		period_data = U32_MAX;
+
+	ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_LRC, period_data);
+
+	duty_data = mul_u64_u32_div(state->duty_cycle, ddata->clk_rate, NSEC_PER_SEC);
+	if (duty_data > U32_MAX)
+		duty_data = U32_MAX;
+
+	ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, duty_data);
+
+	ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL);
+	if (state->enabled)
+		ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL,
+				  ctrl_data | REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE);
+	else
+		ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL,
+				  ctrl_data & ~(REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE));
+
+	return 0;
+}
+
+static const struct pwm_ops ocores_pwm_ops = {
+	.get_state = ocores_pwm_get_state,
+	.apply = ocores_pwm_apply,
+};
+
+static const struct ocores_pwm_data starfive_pwm_data = {
+	.get_ch_base = ocores_pwm_get_ch_base,
+};
+
+static const struct of_device_id ocores_pwm_of_match[] = {
+	{ .compatible = "opencores,pwm-v1" },
+	{ .compatible = "starfive,jh7100-pwm", .data = &starfive_pwm_data},
+	{ .compatible = "starfive,jh7110-pwm", .data = &starfive_pwm_data},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ocores_pwm_of_match);
+
+static void ocores_pwm_reset_control_assert(void *data)
+{
+	reset_control_assert(data);
+}
+
+static int ocores_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct ocores_pwm_device *ddata;
+	struct pwm_chip *chip;
+	struct clk *clk;
+	struct reset_control *rst;
+	int ret;
+
+	chip = devm_pwmchip_alloc(&pdev->dev, 8, sizeof(*ddata));
+	if (IS_ERR(chip))
+		return -ENOMEM;
+
+	ddata = chip_to_ocores(chip);
+	ddata->data = device_get_match_data(&pdev->dev);
+	chip->ops = &ocores_pwm_ops;
+
+	ddata->regs = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(ddata->regs))
+		return dev_err_probe(dev, PTR_ERR(ddata->regs),
+				     "Failed to map IO resources\n");
+
+	clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk),
+				     "Failed to get pwm's clock\n");
+
+	ret = devm_clk_rate_exclusive_get(dev, clk);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to lock clock rate\n");
+
+	rst = devm_reset_control_get_optional_exclusive(dev, NULL);
+	if (IS_ERR(rst))
+		return dev_err_probe(dev, PTR_ERR(rst),
+				     "Failed to get pwm's reset\n");
+
+	ret = reset_control_deassert(rst);
+	if (ret) {
+		dev_err(dev, "Failed to deassert pwm's reset\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, ocores_pwm_reset_control_assert, rst);
+	if (ret) {
+		dev_err(dev, "Failed to register assert devm action\n");
+		return ret;
+	}
+
+	ddata->clk_rate = clk_get_rate(clk);
+	if (ddata->clk_rate > NSEC_PER_SEC) {
+		dev_err(dev, "Failed to get clock frequency\n");
+		return -EINVAL;
+	}
+
+	ret = devm_pwmchip_add(dev, chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
+
+	return 0;
+}
+
+static struct platform_driver ocores_pwm_driver = {
+	.probe = ocores_pwm_probe,
+	.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 PTC PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.43.0

Re: [PATCH v17] pwm: opencores: Add PWM driver support

[Maud Spierings]

Hello William,

I've once again put the patch to the test, and it seems the oops is 
resolved.

I did notice something odd though, when controlling the backlight  
bl_power 0 means the backlight is on and controllable, 1 seems like off, 
but instead sets the screen to maximum brightness and then stops 
listening to any value echoed into brightness.

The brightness is also reversed from what would be logical, so 255 is 
off and 0 is maximum.

Now the little text at the top specifies that the hardware only does 
inverted polarity, which I guess explains this, but I don't understand 
it. I also encountered this when I got an error to start with so I had 
to add PWM_POLARITY_INVERTED to my pwm-backlight definition.

But I don't understand why it isn't supported. Wouldn't supporting non 
inverted polarity be a very simple calculation? 40% negative duty cycle 
is of course equal to 60% positive duty cycle, 20% N == 80% P etc. I 
don't see why the hardware would specifically have to support this.

Anyways it does seem to work now so if others approve:

Tested-by: Maud Spierings <maud_spierings@hotmail.com>

Kind regards,

Maud

Re: [PATCH v17] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

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

Hello Maud,

On Mon, Jan 06, 2025 at 07:59:23PM +0100, Maud Spierings wrote:
> Hello William,
> 
> I've once again put the patch to the test, and it seems the oops is
> resolved.
> 
> I did notice something odd though, when controlling the backlight  bl_power
> 0 means the backlight is on and controllable, 1 seems like off, but instead
> sets the screen to maximum brightness and then stops listening to any value
> echoed into brightness.

Note that for bl_power 0 is on and 4 is off. Still the behaviour you
report sounds wrong. Quickly looking in the pwm_bl driver, I don't spot
something obvious.

> 
> The brightness is also reversed from what would be logical, so 255 is off
> and 0 is maximum.
> 
> Now the little text at the top specifies that the hardware only does
> inverted polarity, which I guess explains this, but I don't understand it.

The backlight's operation should still be fine, its usage be independent
of the PWM's details.

> I also encountered this when I got an error to start with so I had to add
> PWM_POLARITY_INVERTED to my pwm-backlight definition.

That makes me suspect the problem is on your end. If you add
PWM_POLARITY_INVERTED the result is that the pwm_bl driver still
configures duty_cycle=0 for backlight off, but you then get a constant
high output.

So with the hardware capabilities (i.e. not being able to emit a
constant low output) I think you need to not use PWM_POLARITY_INVERTED
and accept that completely off doesn't work (unless you have an
additional GPIO or regulator to disable the backlight).

> But I don't understand why it isn't supported. Wouldn't supporting non
> inverted polarity be a very simple calculation? 40% negative duty cycle is
> of course equal to 60% positive duty cycle, 20% N == 80% P etc. I don't see
> why the hardware would specifically have to support this.

If you only care about the mean voltage level (as is the case for a
backlight), that's right. But only then. And if the hardware cannot emit
a constant low signal, this doesn't help you.

Best regards
Uwe

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

Re: [PATCH v17] pwm: opencores: Add PWM driver support

[Maud Spierings]

Hello Uwe,

On 1/7/25 6:07 AM, Uwe Kleine-König wrote:
> Hello Maud,
>
> On Mon, Jan 06, 2025 at 07:59:23PM +0100, Maud Spierings wrote:
>> Hello William,
>>
>> I've once again put the patch to the test, and it seems the oops is
>> resolved.
>>
>> I did notice something odd though, when controlling the backlight  bl_power
>> 0 means the backlight is on and controllable, 1 seems like off, but instead
>> sets the screen to maximum brightness and then stops listening to any value
>> echoed into brightness.
> Note that for bl_power 0 is on and 4 is off. Still the behaviour you
> report sounds wrong. Quickly looking in the pwm_bl driver, I don't spot
> something obvious.

you are indeed correct thats alittle odd but ok.
>> The brightness is also reversed from what would be logical, so 255 is off
>> and 0 is maximum.
>>
>> Now the little text at the top specifies that the hardware only does
>> inverted polarity, which I guess explains this, but I don't understand it.
> The backlight's operation should still be fine, its usage be independent
> of the PWM's details.
>
>> I also encountered this when I got an error to start with so I had to add
>> PWM_POLARITY_INVERTED to my pwm-backlight definition.
> That makes me suspect the problem is on your end. If you add
> PWM_POLARITY_INVERTED the result is that the pwm_bl driver still
> configures duty_cycle=0 for backlight off, but you then get a constant
> high output.
>
> So with the hardware capabilities (i.e. not being able to emit a
> constant low output) I think you need to not use PWM_POLARITY_INVERTED
> and accept that completely off doesn't work (unless you have an
> additional GPIO or regulator to disable the backlight).

If I do not set PWM_POLARITY_INVERTED in the dts it will cause an EINVAL 
in ocores_pwm_apply

>> But I don't understand why it isn't supported. Wouldn't supporting non
>> inverted polarity be a very simple calculation? 40% negative duty cycle is
>> of course equal to 60% positive duty cycle, 20% N == 80% P etc. I don't see
>> why the hardware would specifically have to support this.
> If you only care about the mean voltage level (as is the case for a
> backlight), that's right. But only then. And if the hardware cannot emit
> a constant low signal, this doesn't help you.
>
> Best regards
> Uwe

I did some more digging, took a look at the pwm-backlight driver in the 
vendor kernel, and it seems there are some tweaks there [1] (all the way 
at the bottom). And it explains some things, first off why it didn't 
error before without the inverted polarity, because it sets this value 
in the pwm-backlight driver in that kernel.

but the interesting line is this one:
|brightness = (u8)~brightness;

Which is doing what I thought to do in the end of my previous email.

So maybe the hardware is indeed the odd one out here, sadly I have no 
access to the design of the board.
But there is definitely# something weird going on here, not quite sure 
where to fix it.
|

[1]: 
https://github.com/DC-DeepComputing/fml13v01-linux/commit/cc7131dd58e068c58e54f8c8fd23c5ac41c33f46

Kind regards,

Maud

Re: [PATCH v17] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

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

Hello Maud,

On Tue, Jan 07, 2025 at 07:37:13AM +0100, Maud Spierings wrote:
> I did some more digging, took a look at the pwm-backlight driver in the
> vendor kernel, and it seems there are some tweaks there [1] (all the way at
> the bottom). And it explains some things, first off why it didn't error
> before without the inverted polarity, because it sets this value in the
> pwm-backlight driver in that kernel.
> 
> but the interesting line is this one:
> |brightness = (u8)~brightness;

Huh, that confirms my prejudice that vendor trees are bad. Even in
combination with

	state.polarity = PWM_POLARITY_INVERSED;

in the probe function this is horrible. This effectively renders your
testing useless.

Best regards
Uwe

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

Re: [PATCH v17] pwm: opencores: Add PWM driver support

[Uwe Kleine-König]

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

Hello,

On Mon, Jan 06, 2025 at 06:35:40PM +0800, William Qiu wrote:
> diff --git a/drivers/pwm/pwm-ocores.c b/drivers/pwm/pwm-ocores.c
> new file mode 100644
> index 000000000000..bc957830017f
> --- /dev/null
> +++ b/drivers/pwm/pwm-ocores.c
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * OpenCores PWM Driver
> + *
> + * https://opencores.org/projects/ptc
> + *
> + * Copyright (C) 2018-2023 StarFive Technology Co., Ltd.
> + *
> + * Limitations:
> + * - The hardware only supports inverted polarity.
> + * - The hardware minimum period / duty_cycle is (1 / pwm_apb clock frequency).
> + * - The hardware maximum period / duty_cycle is (U32_MAX / pwm_apb clock frequency).
> + * - The output is set to a low level immediately when disabled.
> + * - When configuration changes are done, they get active immediately without resetting
> + *   the counter. This might result in one period affected by both old and new settings.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +/* OpenCores Register offsets */
> +#define REG_OCPWM_CNTR    0x0
> +#define REG_OCPWM_HRC     0x4
> +#define REG_OCPWM_LRC     0x8
> +#define REG_OCPWM_CTRL    0xC
> +
> +/* OCPWM_CTRL register bits*/
> +#define REG_OCPWM_CNTR_EN      BIT(0)
> +#define REG_OCPWM_CNTR_ECLK    BIT(1)
> +#define REG_OCPWM_CNTR_NEC     BIT(2)
> +#define REG_OCPWM_CNTR_OE      BIT(3)
> +#define REG_OCPWM_CNTR_SIGNLE  BIT(4)
> +#define REG_OCPWM_CNTR_INTE    BIT(5)
> +#define REG_OCPWM_CNTR_INT     BIT(6)
> +#define REG_OCPWM_CNTR_RST     BIT(7)
> +#define REG_OCPWM_CNTR_CAPTE   BIT(8)

I'm confused here. These register bits are used for REG_OCPWM_CTRL in
the code below. So their name is wrong and they all should change
s/CNTR/CTRL/? What is REG_OCPWM_CNTR which isn't used at all?

> +struct ocores_pwm_data {
> +	void __iomem *(*get_ch_base)(void __iomem *base, unsigned int channel);
> +};
> +
> +struct ocores_pwm_device {
> +	const struct ocores_pwm_data *data;
> +	void __iomem *regs;
> +	u32 clk_rate; /* PWM APB clock frequency */
> +};
> +
> +static inline u32 ocores_pwm_readl(struct ocores_pwm_device *ddata,
> +				   unsigned int channel,
> +				   unsigned int offset)
> +{
> +	void __iomem *base = ddata->data->get_ch_base ?
> +			     ddata->data->get_ch_base(ddata->regs, channel) : ddata->regs;

I suggest s/base/channel_base/

> +
> +	return readl(base + offset);
> +}
> +
> +static inline void ocores_pwm_writel(struct ocores_pwm_device *ddata,
> +				     unsigned int channel,
> +				     unsigned int offset, u32 val)
> +{
> +	void __iomem *base = ddata->data->get_ch_base ?
> +			     ddata->data->get_ch_base(ddata->regs, channel) : ddata->regs;
> +
> +	writel(val, base + offset);

So without a get_ch_base() function the register address is independent
from the channel. That's wrong isn't it? (Or alternatively
opencores,pwm-v1 should only support a single channel.)

> +}
> +
> +static inline struct ocores_pwm_device *chip_to_ocores(struct pwm_chip *chip)

Please rename this to also share the ocores_pwm_ prefix all other
functions use. ocores_pwm_from_chip() would be the usual choice.

> +{
> +	return pwmchip_get_drvdata(chip);
> +}
> +
> +static void __iomem *ocores_pwm_get_ch_base(void __iomem *base,
> +					    unsigned int channel)
> +{
> +	unsigned int offset = (channel & 4) << 13 | (channel & 3) << 4;
> +
> +	return base + offset;

I suggest s/offset/channel_offset/ in this function to differentiate it
from offset in the sense ocores_pwm_writel() uses.

With that strange register layout I wonder if that are really two
different cores with 4 channels each?

> +}
> +
> +static int ocores_pwm_get_state(struct pwm_chip *chip,
> +				struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct ocores_pwm_device *ddata = chip_to_ocores(chip);
> +	u32 period_data, duty_data, ctrl_data;
> +
> +	period_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_LRC);
> +	duty_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_HRC);
> +	ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL);
> +
> +	state->period = DIV_ROUND_UP_ULL((u64)period_data * NSEC_PER_SEC, ddata->clk_rate);
> +	state->duty_cycle = DIV_ROUND_UP_ULL((u64)duty_data * NSEC_PER_SEC, ddata->clk_rate);
> +	state->polarity = PWM_POLARITY_INVERSED;

That looks strange. From the register names I would expect that LRC
holds the low time and HRC holds the high time?

> +	state->enabled = (ctrl_data & REG_OCPWM_CNTR_EN) ? true : false;
> +
> +	return 0;
> +}
> +
> +static int ocores_pwm_apply(struct pwm_chip *chip,
> +			    struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	struct ocores_pwm_device *ddata = chip_to_ocores(chip);
> +	u32 ctrl_data = 0;
> +	u64 period_data, duty_data;
> +
> +	if (state->polarity != PWM_POLARITY_INVERSED)
> +		return -EINVAL;
> +
> +	period_data = mul_u64_u32_div(state->period, ddata->clk_rate, NSEC_PER_SEC);
> +	if (!period_data)
> +		return -EINVAL;
> +
> +	if (period_data > U32_MAX)
> +		period_data = U32_MAX;
> +
> +	ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_LRC, period_data);
> +
> +	duty_data = mul_u64_u32_div(state->duty_cycle, ddata->clk_rate, NSEC_PER_SEC);
> +	if (duty_data > U32_MAX)
> +		duty_data = U32_MAX;
> +
> +	ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_HRC, duty_data);
> +
> +	ctrl_data = ocores_pwm_readl(ddata, pwm->hwpwm, REG_OCPWM_CTRL);
> +	if (state->enabled)
> +		ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL,
> +				  ctrl_data | REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE);
> +	else
> +		ocores_pwm_writel(ddata, pwm->hwpwm, REG_OCPWM_CTRL,
> +				  ctrl_data & ~(REG_OCPWM_CNTR_EN | REG_OCPWM_CNTR_OE));

With REG_OCPWM_CNTR_EN and REG_OCPWM_CNTR_OE unset the values in
REG_OCPWM_LRC and REG_OCPWM_HRC don't matter, right? If so, you can skip
the calculations and register writes in the !state->enabled case.

What is the semantic of REG_OCPWM_CNTR_OE? If it's "output enable" the
claim "The output is set to a low level immediately when disabled."
sounds wrong in general.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops ocores_pwm_ops = {
> +	.get_state = ocores_pwm_get_state,
> +	.apply = ocores_pwm_apply,
> +};
> +
> +static const struct ocores_pwm_data starfive_pwm_data = {
> +	.get_ch_base = ocores_pwm_get_ch_base,
> +};
> +
> +static const struct of_device_id ocores_pwm_of_match[] = {
> +	{ .compatible = "opencores,pwm-v1" },
> +	{ .compatible = "starfive,jh7100-pwm", .data = &starfive_pwm_data},
> +	{ .compatible = "starfive,jh7110-pwm", .data = &starfive_pwm_data},

Is there a difference between starfive,jh7100-pwm and
starfive,jh7110-pwm?

> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ocores_pwm_of_match);
> +
> +static void ocores_pwm_reset_control_assert(void *data)
> +{
> +	reset_control_assert(data);
> +}
> +
> +static int ocores_pwm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ocores_pwm_device *ddata;
> +	struct pwm_chip *chip;
> +	struct clk *clk;
> +	struct reset_control *rst;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(&pdev->dev, 8, sizeof(*ddata));
> +	if (IS_ERR(chip))
> +		return -ENOMEM;
> +
> +	ddata = chip_to_ocores(chip);
> +	ddata->data = device_get_match_data(&pdev->dev);
> +	chip->ops = &ocores_pwm_ops;
> +
> +	ddata->regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(ddata->regs))
> +		return dev_err_probe(dev, PTR_ERR(ddata->regs),
> +				     "Failed to map IO resources\n");
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk),
> +				     "Failed to get pwm's clock\n");
> +
> +	ret = devm_clk_rate_exclusive_get(dev, clk);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to lock clock rate\n");
> +
> +	rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst),
> +				     "Failed to get pwm's reset\n");
> +
> +	ret = reset_control_deassert(rst);
> +	if (ret) {
> +		dev_err(dev, "Failed to deassert pwm's reset\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ocores_pwm_reset_control_assert, rst);
> +	if (ret) {
> +		dev_err(dev, "Failed to register assert devm action\n");
> +		return ret;
> +	}

This can be simplified in the meantime. Just use
devm_reset_control_get_optional_exclusive_deasserted().

> +	ddata->clk_rate = clk_get_rate(clk);
> +	if (ddata->clk_rate > NSEC_PER_SEC) {

clk_get_rate() returns an unsigned long but ddata->clk_rate is only a
u32, so you might miss that the clockrate is bigger than NSEC_PER_SEC.

> +		dev_err(dev, "Failed to get clock frequency\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> +
> +	return 0;
> +}

Best regards
Uwe

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