[PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Baolin Wang]

Add Spreadtrum PWM controller documentation.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v1:
 - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
---
 Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   38 ++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
new file mode 100644
index 0000000..e6cf312
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
@@ -0,0 +1,38 @@
+Spreadtrum PWM controller
+
+Spreadtrum SoCs PWM controller provides 4 PWM channels.
+
+Required porperties:
+- compatible : Should be "sprd,ums512-pwm".
+- reg: Physical base address and length of the controller's registers.
+- clocks: The phandle and specifier referencing the controller's clocks.
+- clock-names: Should contain following entries:
+  "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
+  "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
+- assigned-clocks: Reference to the PWM clock entroes.
+- assigned-clock-parents: The phandle of the parent clock of PWM clock.
+- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
+  the cells format.
+
+Example:
+	pwms: pwm@32260000 {
+		compatible = "sprd,ums512-pwm";
+		reg = <0 0x32260000 0 0x10000>;
+		clock-names = "pwm0", "enable0",
+			"pwm1", "enable1",
+			"pwm2", "enable2",
+			"pwm3", "enable3";
+		clocks = <&aon_clk CLK_PWM0>, <&aonapb_gate CLK_PWM0_EB>,
+		       <&aon_clk CLK_PWM1>, <&aonapb_gate CLK_PWM1_EB>,
+		       <&aon_clk CLK_PWM2>, <&aonapb_gate CLK_PWM2_EB>,
+		       <&aon_clk CLK_PWM3>, <&aonapb_gate CLK_PWM3_EB>;
+		assigned-clocks = <&aon_clk CLK_PWM0>,
+			<&aon_clk CLK_PWM1>,
+			<&aon_clk CLK_PWM2>,
+			<&aon_clk CLK_PWM3>;
+		assigned-clock-parents = <&ext_26m>,
+			<&ext_26m>,
+			<&ext_26m>,
+			<&ext_26m>;
+		#pwm-cells = <2>;
+	};
-- 
1.7.9.5

[PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

This patch adds the Spreadtrum PWM support, which provides maximum 4
channels.

Signed-off-by: Neo Hou <neo.hou@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v1:
 - Add depending on HAS_IOMEM.
 - Rename parameters' names.
 - Implement .apply() instead of .config(), .enable() and .disable().
 - Use NSEC_PER_SEC instead of 1000000000ULL.
 - Add some comments to make code more readable.
 - Remove some redundant operation.
 - Use standard clock properties to set clock parent.
 - Other coding style optimization.
---
 drivers/pwm/Kconfig    |   11 ++
 drivers/pwm/Makefile   |    1 +
 drivers/pwm/pwm-sprd.c |  307 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 319 insertions(+)
 create mode 100644 drivers/pwm/pwm-sprd.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a7e5751..31dfc88 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -423,6 +423,17 @@ config PWM_SPEAR
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-spear.
 
+config PWM_SPRD
+	tristate "Spreadtrum PWM support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	depends on HAS_IOMEM
+	help
+	  Generic PWM framework driver for the PWM controller on
+	  Spreadtrum SoCs.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sprd.
+
 config PWM_STI
 	tristate "STiH4xx PWM support"
 	depends on ARCH_STI
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 76b555b..26326ad 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
+obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
 obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
 obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
new file mode 100644
index 0000000..067e711
--- /dev/null
+++ b/drivers/pwm/pwm-sprd.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Spreadtrum Communications Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+#define SPRD_PWM_PRESCALE	0x0
+#define SPRD_PWM_MOD		0x4
+#define SPRD_PWM_DUTY		0x8
+#define SPRD_PWM_ENABLE		0x18
+
+#define SPRD_PWM_MOD_MAX	GENMASK(7, 0)
+#define SPRD_PWM_DUTY_MSK	GENMASK(15, 0)
+#define SPRD_PWM_PRESCALE_MSK	GENMASK(7, 0)
+#define SPRD_PWM_ENABLE_BIT	BIT(0)
+
+#define SPRD_PWM_NUM		4
+#define SPRD_PWM_REGS_SHIFT	5
+#define SPRD_PWM_NUM_CLKS	2
+#define SPRD_PWM_OUTPUT_CLK	1
+
+struct sprd_pwm_chn {
+	struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
+	u32 clk_rate;
+};
+
+struct sprd_pwm_chip {
+	void __iomem *base;
+	struct device *dev;
+	struct pwm_chip chip;
+	int num_pwms;
+	struct sprd_pwm_chn chn[SPRD_PWM_NUM];
+};
+
+/*
+ * The list of clocks required by PWM channels, and each channel has 2 clocks:
+ * enable clock and pwm clock.
+ */
+static const char * const sprd_pwm_clks[] = {
+	"enable0", "pwm0",
+	"enable1", "pwm1",
+	"enable2", "pwm2",
+	"enable3", "pwm3",
+};
+
+static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
+{
+	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
+
+	return readl_relaxed(spc->base + offset);
+}
+
+static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
+			   u32 reg, u32 val)
+{
+	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
+
+	writel_relaxed(val, spc->base + offset);
+}
+
+static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct sprd_pwm_chip *spc =
+		container_of(chip, struct sprd_pwm_chip, chip);
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	u32 val, duty, prescale;
+	u64 tmp;
+	int ret;
+
+	/*
+	 * The clocks to PWM channel has to be enabled first before
+	 * reading to the registers.
+	 */
+	ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
+	if (ret) {
+		dev_err(spc->dev, "failed to enable pwm%u clocks\n",
+			pwm->hwpwm);
+		return;
+	}
+
+	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE);
+	if (val & SPRD_PWM_ENABLE_BIT)
+		state->enabled = true;
+	else
+		state->enabled = false;
+
+	/*
+	 * The hardware provides a counter that is feed by the source clock.
+	 * The period length is (PRESCALE + 1) * MOD counter steps.
+	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
+	 * Thus the period_ns and duty_ns calculation formula should be:
+	 * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
+	 * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
+	 */
+	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
+	prescale = val & SPRD_PWM_PRESCALE_MSK;
+	tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
+	state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
+
+	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
+	duty = val & SPRD_PWM_DUTY_MSK;
+	tmp = (prescale + 1) * NSEC_PER_SEC * duty;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
+
+	/* Disable PWM clocks if the PWM channel is not in enable state. */
+	if (!state->enabled)
+		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
+}
+
+static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
+			   int duty_ns, int period_ns)
+{
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	u64 div, tmp;
+	u32 prescale, duty;
+
+	/*
+	 * The hardware provides a counter that is feed by the source clock.
+	 * The period length is (PRESCALE + 1) * MOD counter steps.
+	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
+	 *
+	 * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
+	 * The value for PRESCALE is selected such that the resulting period
+	 * gets the maximal length not bigger than the requested one with the
+	 * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
+	 */
+	duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
+
+	tmp = (u64)chn->clk_rate * period_ns;
+	div = NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
+	prescale = div64_u64(tmp, div) - 1;
+	if (prescale > SPRD_PWM_PRESCALE_MSK)
+		prescale = SPRD_PWM_PRESCALE_MSK;
+
+	/*
+	 * Note: The MOD must be configured before DUTY, and the hardware can
+	 * ensure current running period is completed before changing a new
+	 * configuration to avoid mixed settings.
+	 */
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
+
+	return 0;
+}
+
+static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			  struct pwm_state *state)
+{
+	struct sprd_pwm_chip *spc =
+		container_of(chip, struct sprd_pwm_chip, chip);
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	struct pwm_state cstate;
+	int ret;
+
+	pwm_get_state(pwm, &cstate);
+
+	if (state->enabled) {
+		if (!cstate.enabled) {
+			/*
+			 * The clocks to PWM channel has to be enabled first
+			 * before writing to the registers.
+			 */
+			ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
+						      chn->clks);
+			if (ret) {
+				dev_err(spc->dev,
+					"failed to enable pwm%u clocks\n",
+					pwm->hwpwm);
+				return ret;
+			}
+		}
+
+		if (state->period != cstate.period ||
+		    state->duty_cycle != cstate.duty_cycle) {
+			ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
+					      state->period);
+			if (ret)
+				return ret;
+		}
+
+		sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
+	} else if (cstate.enabled) {
+		sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
+
+		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
+	}
+
+	return 0;
+}
+
+static const struct pwm_ops sprd_pwm_ops = {
+	.apply = sprd_pwm_apply,
+	.get_state = sprd_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
+{
+	struct clk *clk_pwm;
+	int ret, i, clk_index = 0;
+
+	for (i = 0; i < SPRD_PWM_NUM; i++) {
+		struct sprd_pwm_chn *chn = &spc->chn[i];
+		int j;
+
+		for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
+			chn->clks[j].id = sprd_pwm_clks[clk_index++];
+
+		ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks);
+		if (ret) {
+			if (ret == -ENOENT)
+				break;
+
+			dev_err(spc->dev, "failed to get channel clocks\n");
+			return ret;
+		}
+
+		clk_pwm = chn->clks[SPRD_PWM_OUTPUT_CLK].clk;
+		chn->clk_rate = clk_get_rate(clk_pwm);
+	}
+
+	if (!i) {
+		dev_err(spc->dev, "no available PWM channels\n");
+		return -EINVAL;
+	}
+
+	spc->num_pwms = i;
+
+	return 0;
+}
+
+static int sprd_pwm_probe(struct platform_device *pdev)
+{
+	struct sprd_pwm_chip *spc;
+	int ret;
+
+	spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL);
+	if (!spc)
+		return -ENOMEM;
+
+	spc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(spc->base))
+		return PTR_ERR(spc->base);
+
+	spc->dev = &pdev->dev;
+	platform_set_drvdata(pdev, spc);
+
+	ret = sprd_pwm_clk_init(spc);
+	if (ret)
+		return ret;
+
+	spc->chip.dev = &pdev->dev;
+	spc->chip.ops = &sprd_pwm_ops;
+	spc->chip.base = -1;
+	spc->chip.npwm = spc->num_pwms;
+
+	ret = pwmchip_add(&spc->chip);
+	if (ret)
+		dev_err(&pdev->dev, "failed to add PWM chip\n");
+
+	return ret;
+}
+
+static int sprd_pwm_remove(struct platform_device *pdev)
+{
+	struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
+	int ret, i;
+
+	ret = pwmchip_remove(&spc->chip);
+
+	for (i = 0; i < spc->num_pwms; i++) {
+		struct sprd_pwm_chn *chn = &spc->chn[i];
+
+		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
+	}
+
+	return ret;
+}
+
+static const struct of_device_id sprd_pwm_of_match[] = {
+	{ .compatible = "sprd,ums512-pwm", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sprd_pwm_of_match);
+
+static struct platform_driver sprd_pwm_driver = {
+	.driver = {
+		.name = "sprd-pwm",
+		.of_match_table = sprd_pwm_of_match,
+	},
+	.probe = sprd_pwm_probe,
+	.remove = sprd_pwm_remove,
+};
+
+module_platform_driver(sprd_pwm_driver);
+
+MODULE_DESCRIPTION("Spreadtrum PWM Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Uwe Kleine-König]

On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> Add Spreadtrum PWM controller documentation.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
> ---
>  Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   38 ++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> new file mode 100644
> index 0000000..e6cf312
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> @@ -0,0 +1,38 @@
> +Spreadtrum PWM controller
> +
> +Spreadtrum SoCs PWM controller provides 4 PWM channels.
> +
> +Required porperties:

s/porperties/properties/

> +- compatible : Should be "sprd,ums512-pwm".
> +- reg: Physical base address and length of the controller's registers.
> +- clocks: The phandle and specifier referencing the controller's clocks.
> +- clock-names: Should contain following entries:
> +  "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> +  "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
> +- assigned-clocks: Reference to the PWM clock entroes.

s/entroes/entries/

> +- assigned-clock-parents: The phandle of the parent clock of PWM clock.

I'm not sure you need to point out assigned-clocks and
assigned-clock-parents as this is general clk stuff. Also I wonder if
these should be "required properties".

> +- #pwm-cells: Should be 2. See pwm.txt in this directory for a description of
> +  the cells format.

Best regards
Uwe

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

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello,

On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> This patch adds the Spreadtrum PWM support, which provides maximum 4
> channels.
> 
> Signed-off-by: Neo Hou <neo.hou@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Add depending on HAS_IOMEM.
>  - Rename parameters' names.
>  - Implement .apply() instead of .config(), .enable() and .disable().
>  - Use NSEC_PER_SEC instead of 1000000000ULL.
>  - Add some comments to make code more readable.
>  - Remove some redundant operation.
>  - Use standard clock properties to set clock parent.
>  - Other coding style optimization.
> ---
>  drivers/pwm/Kconfig    |   11 ++
>  drivers/pwm/Makefile   |    1 +
>  drivers/pwm/pwm-sprd.c |  307 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 319 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sprd.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7e5751..31dfc88 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -423,6 +423,17 @@ config PWM_SPEAR
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-spear.
>  
> +config PWM_SPRD
> +	tristate "Spreadtrum PWM support"
> +	depends on ARCH_SPRD || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Generic PWM framework driver for the PWM controller on
> +	  Spreadtrum SoCs.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-sprd.
> +
>  config PWM_STI
>  	tristate "STiH4xx PWM support"
>  	depends on ARCH_STI
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 76b555b..26326ad 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
>  obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
>  obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
>  obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
> +obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
>  obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
>  obj-$(CONFIG_PWM_STM32)		+= pwm-stm32.o
>  obj-$(CONFIG_PWM_STM32_LP)	+= pwm-stm32-lp.o
> diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> new file mode 100644
> index 0000000..067e711
> --- /dev/null
> +++ b/drivers/pwm/pwm-sprd.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Spreadtrum Communications Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +#define SPRD_PWM_PRESCALE	0x0
> +#define SPRD_PWM_MOD		0x4
> +#define SPRD_PWM_DUTY		0x8
> +#define SPRD_PWM_ENABLE		0x18
> +
> +#define SPRD_PWM_MOD_MAX	GENMASK(7, 0)
> +#define SPRD_PWM_DUTY_MSK	GENMASK(15, 0)
> +#define SPRD_PWM_PRESCALE_MSK	GENMASK(7, 0)
> +#define SPRD_PWM_ENABLE_BIT	BIT(0)
> +
> +#define SPRD_PWM_NUM		4
> +#define SPRD_PWM_REGS_SHIFT	5
> +#define SPRD_PWM_NUM_CLKS	2
> +#define SPRD_PWM_OUTPUT_CLK	1

These definitions could benefit from some explaining comments. Just from
looking at the names it is for example not obvious what is the
difference between SPRD_PWM_NUM and SPRD_PWM_NUM_CLKS is.

> +struct sprd_pwm_chn {
> +	struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
> +	u32 clk_rate;
> +};
> +
> +struct sprd_pwm_chip {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct pwm_chip chip;
> +	int num_pwms;
> +	struct sprd_pwm_chn chn[SPRD_PWM_NUM];
> +};
> +
> +/*
> + * The list of clocks required by PWM channels, and each channel has 2 clocks:
> + * enable clock and pwm clock.
> + */
> +static const char * const sprd_pwm_clks[] = {
> +	"enable0", "pwm0",
> +	"enable1", "pwm1",
> +	"enable2", "pwm2",
> +	"enable3", "pwm3",
> +};
> +
> +static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
> +{
> +	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> +
> +	return readl_relaxed(spc->base + offset);
> +}
> +
> +static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
> +			   u32 reg, u32 val)
> +{
> +	u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> +
> +	writel_relaxed(val, spc->base + offset);
> +}
> +
> +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       struct pwm_state *state)
> +{
> +	struct sprd_pwm_chip *spc =
> +		container_of(chip, struct sprd_pwm_chip, chip);
> +	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> +	u32 val, duty, prescale;
> +	u64 tmp;
> +	int ret;
> +
> +	/*
> +	 * The clocks to PWM channel has to be enabled first before
> +	 * reading to the registers.
> +	 */
> +	ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> +	if (ret) {
> +		dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> +			pwm->hwpwm);
> +		return;
> +	}
> +
> +	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE);
> +	if (val & SPRD_PWM_ENABLE_BIT)
> +		state->enabled = true;
> +	else
> +		state->enabled = false;
> +
> +	/*
> +	 * The hardware provides a counter that is feed by the source clock.
> +	 * The period length is (PRESCALE + 1) * MOD counter steps.
> +	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> +	 * Thus the period_ns and duty_ns calculation formula should be:
> +	 * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> +	 * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> +	 */
> +	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> +	prescale = val & SPRD_PWM_PRESCALE_MSK;
> +	tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +
> +	val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> +	duty = val & SPRD_PWM_DUTY_MSK;
> +	tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> +	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> +
> +	/* Disable PWM clocks if the PWM channel is not in enable state. */
> +	if (!state->enabled)
> +		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> +}
> +
> +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)
> +{
> +	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> +	u64 div, tmp;
> +	u32 prescale, duty;
> +
> +	/*
> +	 * The hardware provides a counter that is feed by the source clock.
> +	 * The period length is (PRESCALE + 1) * MOD counter steps.
> +	 * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> +	 *
> +	 * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> +	 * The value for PRESCALE is selected such that the resulting period
> +	 * gets the maximal length not bigger than the requested one with the
> +	 * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> +	 */
> +	duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> +
> +	tmp = (u64)chn->clk_rate * period_ns;
> +	div = NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> +	prescale = div64_u64(tmp, div) - 1;
> +	if (prescale > SPRD_PWM_PRESCALE_MSK)
> +		prescale = SPRD_PWM_PRESCALE_MSK;

This isn't the inverse of .get_state(). Consider:

	clk_rate = 3333333
	SPRD_PWM_PRESCALE = 15
	
then you calculate in .get_state():

	period = 1224000

If you then call apply with this value you calulate:

	prescale = 14

.

> +
> +	/*
> +	 * Note: The MOD must be configured before DUTY, and the hardware can
> +	 * ensure current running period is completed before changing a new
> +	 * configuration to avoid mixed settings.

You write "the hardware can ensure ..". Does that actually means "The
hardware ensures that ..." or is there some additional condition? Maybe
you mean:

	/*
	 * Writing DUTY triggers the hardware to actually apply the
	 * values written to MOD and DUTY to the output. So write DUTY
	 * last.
	 */

> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);

If writing DUTY triggers the hardware to sample DUTY and MOD, what about
PRESCALE?

> +	return 0;
> +}
> +
> +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  struct pwm_state *state)
> +{
> +	struct sprd_pwm_chip *spc =
> +		container_of(chip, struct sprd_pwm_chip, chip);
> +	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> +	struct pwm_state cstate;
> +	int ret;
> +
> +	pwm_get_state(pwm, &cstate);

I don't like it when pwm drivers call pwm_get_state(). If ever
pwm_get_state would take a lock, this would deadlock as the lock is
probably already taken when your .apply() callback is running. Moreover
the (expensive) calculations are not used appropriately. See below.

> +	if (state->enabled) {
> +		if (!cstate.enabled) {

To just know the value of cstate.enabled you only need to read the
register with the ENABLE flag. That is cheaper than calling get_state.

> +			/*
> +			 * The clocks to PWM channel has to be enabled first
> +			 * before writing to the registers.
> +			 */
> +			ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> +						      chn->clks);
> +			if (ret) {
> +				dev_err(spc->dev,
> +					"failed to enable pwm%u clocks\n",
> +					pwm->hwpwm);
> +				return ret;
> +			}
> +		}
> +
> +		if (state->period != cstate.period ||
> +		    state->duty_cycle != cstate.duty_cycle) {

This is a coarse check. If state->period and cstate.period only differ
by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
state->period) probably results in a noop. So you're doing an expensive
division to get an unreliable check. It would be better to calculate the
register values from the requested state and compare the register
values. The costs are more or less the same than calling .get_state and
the check is reliable. And you don't need to spend another division to
calculate the new register values.

> +			ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> +					      state->period);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> +	} else if (cstate.enabled) {
> +		sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> +
> +		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);

Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
currently running period and the write doesn't block that long: Does
disabling the clocks interfere with completing the period?

> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops sprd_pwm_ops = {
> +	.apply = sprd_pwm_apply,
> +	.get_state = sprd_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> +{
> +	struct clk *clk_pwm;
> +	int ret, i, clk_index = 0;
> +
> +	for (i = 0; i < SPRD_PWM_NUM; i++) {
> +		struct sprd_pwm_chn *chn = &spc->chn[i];
> +		int j;
> +
> +		for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
> +			chn->clks[j].id = sprd_pwm_clks[clk_index++];

I think this would be more understandable when written as:

	for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
		chn->clks[j].id = sprd_pwm_clks[i * SPRD_PWM_NUM_CLKS + j];

but I'm not sure I'm objective here.

> +
> +		ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks);
> +		if (ret) {
> +			if (ret == -ENOENT)
> +				break;
> +
> +			dev_err(spc->dev, "failed to get channel clocks\n");

if ret == -EPROBE_DEFER you shouldn't issue an error message.

> +			return ret;
> +		}
> +
> +		clk_pwm = chn->clks[SPRD_PWM_OUTPUT_CLK].clk;
> +		chn->clk_rate = clk_get_rate(clk_pwm);
> +	}
> +
> +	if (!i) {
> +		dev_err(spc->dev, "no available PWM channels\n");
> +		return -EINVAL;

ENODEV?

> +	}
> +
> +	spc->num_pwms = i;
> +
> +	return 0;
> +}
> +
> +static int sprd_pwm_probe(struct platform_device *pdev)
> +{
> +	struct sprd_pwm_chip *spc;
> +	int ret;
> +
> +	spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL);
> +	if (!spc)
> +		return -ENOMEM;
> +
> +	spc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(spc->base))
> +		return PTR_ERR(spc->base);
> +
> +	spc->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, spc);
> +
> +	ret = sprd_pwm_clk_init(spc);
> +	if (ret)
> +		return ret;
> +
> +	spc->chip.dev = &pdev->dev;
> +	spc->chip.ops = &sprd_pwm_ops;
> +	spc->chip.base = -1;
> +	spc->chip.npwm = spc->num_pwms;
> +
> +	ret = pwmchip_add(&spc->chip);
> +	if (ret)
> +		dev_err(&pdev->dev, "failed to add PWM chip\n");
> +
> +	return ret;
> +}
> +
> +static int sprd_pwm_remove(struct platform_device *pdev)
> +{
> +	struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> +	int ret, i;
> +
> +	ret = pwmchip_remove(&spc->chip);
> +
> +	for (i = 0; i < spc->num_pwms; i++) {
> +		struct sprd_pwm_chn *chn = &spc->chn[i];
> +
> +		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);

If a PWM was still running you're effectively stopping it here, right?
Are you sure you don't disable once more than you enabled?

> +	}
> +
> +	return ret;
> +}

Best regards
Uwe

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

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Baolin Wang]

Hi Uwe,

On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > Add Spreadtrum PWM controller documentation.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > Changes from v1:
> >  - Use assigned-clock-parents and assigned-clocks to set PWM clock parent.
> > ---
> >  Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   38 ++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/pwm-sprd.txt b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> > new file mode 100644
> > index 0000000..e6cf312
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
> > @@ -0,0 +1,38 @@
> > +Spreadtrum PWM controller
> > +
> > +Spreadtrum SoCs PWM controller provides 4 PWM channels.
> > +
> > +Required porperties:
>
> s/porperties/properties/

Sorry for typos, will fix in next version.

>
> > +- compatible : Should be "sprd,ums512-pwm".
> > +- reg: Physical base address and length of the controller's registers.
> > +- clocks: The phandle and specifier referencing the controller's clocks.
> > +- clock-names: Should contain following entries:
> > +  "pwmn": used to derive the functional clock for PWM channel n (n range: 0 ~ 3).
> > +  "enablen": for PWM channel n enable clock (n range: 0 ~ 3).
> > +- assigned-clocks: Reference to the PWM clock entroes.
>
> s/entroes/entries/

Sure.

>
> > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
>
> I'm not sure you need to point out assigned-clocks and
> assigned-clock-parents as this is general clk stuff. Also I wonder if
> these should be "required properties".

I think I should describe any properties used by PWM node, like
'clocks' and 'clock-names' properties, though they are common clock
properties.
Yes, they are required. Thanks for your comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Uwe Kleine-König]

Hello Baolin,

On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> >
> > I'm not sure you need to point out assigned-clocks and
> > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > these should be "required properties".
> 
> I think I should describe any properties used by PWM node, like
> 'clocks' and 'clock-names' properties, though they are common clock
> properties.

Then you might want to describe also "status", "assigned-clock-rates",
"pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names" and
probably another dozen I'm not aware of.

> Yes, they are required. Thanks for your comments.

required in which sense? Why can a Spreadtrum PWM not work when the
clock parents are unspecified?

Best regards
Uwe

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

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Baolin Wang]

Hi Uwe,

On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> > >
> > > I'm not sure you need to point out assigned-clocks and
> > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > > these should be "required properties".
> >
> > I think I should describe any properties used by PWM node, like
> > 'clocks' and 'clock-names' properties, though they are common clock
> > properties.
>
> Then you might want to describe also "status", "assigned-clock-rates",
> "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names" and
> probably another dozen I'm not aware of.

We usually do not describe 'status', but if your device node used
"pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
describe them to let users know what is the purpose of these
properties. That's also asked by DT maintainer Rob.

>
> > Yes, they are required. Thanks for your comments.
>
> required in which sense? Why can a Spreadtrum PWM not work when the
> clock parents are unspecified?

On some Spreadtrum platforms, the default source clock of PWM may not
be enabled, so we should force users to select one available source
clock for PWM output clock.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Uwe Kleine-König]

On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> Hi Uwe,
> 
> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello Baolin,
> >
> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM clock.
> > > >
> > > > I'm not sure you need to point out assigned-clocks and
> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > > > these should be "required properties".
> > >
> > > I think I should describe any properties used by PWM node, like
> > > 'clocks' and 'clock-names' properties, though they are common clock
> > > properties.
> >
> > Then you might want to describe also "status", "assigned-clock-rates",
> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names" and
> > probably another dozen I'm not aware of.
> 
> We usually do not describe 'status', but if your device node used
> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> describe them to let users know what is the purpose of these
> properties. That's also asked by DT maintainer Rob.

Does this convince you that you should also describe "pinctrl-$n" and
the others? If not, why not? We also usually don't describe
assigned-clock-parents. For me these are all in the same catagory:
Common properties supported for each devicetree node that represents a
device. The only difference is that on your board you make use of some
but not some others.

> > > Yes, they are required. Thanks for your comments.
> >
> > required in which sense? Why can a Spreadtrum PWM not work when the
> > clock parents are unspecified?
> 
> On some Spreadtrum platforms, the default source clock of PWM may not
> be enabled, so we should force users to select one available source
> clock for PWM output clock.

Sounds like a bug in the clk tree of your SoC that shouldn't affect how
the PWM is described in the device tree. After all a parent of a clock
is supposed to become enabled when the clock gets enabled.

Best regards
Uwe

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

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Baolin Wang]

Hi Uwe,

On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
>> Hi Uwe,
>>
>> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>> >
>> > Hello Baolin,
>> >
>> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
>> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
>> > > <u.kleine-koenig@pengutronix.de> wrote:
>> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
>> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
>> > > > > clock.
>> > > >
>> > > > I'm not sure you need to point out assigned-clocks and
>> > > > assigned-clock-parents as this is general clk stuff. Also I wonder
>> > > > if
>> > > > these should be "required properties".
>> > >
>> > > I think I should describe any properties used by PWM node, like
>> > > 'clocks' and 'clock-names' properties, though they are common clock
>> > > properties.
>> >
>> > Then you might want to describe also "status", "assigned-clock-rates",
>> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
>> > and
>> > probably another dozen I'm not aware of.
>>
>> We usually do not describe 'status', but if your device node used
>> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
>> describe them to let users know what is the purpose of these
>> properties. That's also asked by DT maintainer Rob.
>
> Does this convince you that you should also describe "pinctrl-$n" and
> the others? If not, why not? We also usually don't describe

Our PWM device node did not use "pinctrl-$n" things, why I should
describe "pinctrl-$n"?
If some devices use pinctrl, yes, they should describe what is the
purpose of pinctrl, see:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt

> assigned-clock-parents. For me these are all in the same catagory:

Lots of dt bindings describe 'assigned-clock-parents',:
./Documentation/devicetree/bindings/display/msm/dsi.txt
./Documentation/devicetree/bindings/display/msm/dsi.txt
./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
......

> Common properties supported for each devicetree node that represents a
> device. The only difference is that on your board you make use of some
> but not some others.

Fine, let's decide this by PWM maintainer or DT maintainer Rob.

>
>> > > Yes, they are required. Thanks for your comments.
>> >
>> > required in which sense? Why can a Spreadtrum PWM not work when the
>> > clock parents are unspecified?
>>
>> On some Spreadtrum platforms, the default source clock of PWM may not
>> be enabled, so we should force users to select one available source
>> clock for PWM output clock.
>
> Sounds like a bug in the clk tree of your SoC that shouldn't affect how
> the PWM is described in the device tree. After all a parent of a clock
> is supposed to become enabled when the clock gets enabled.

That's not a bug, that's like a MUX, the default source clock of PWM
can be disabled, since we usually do not use the default source clock.
Then we can select a available source clock by the MUX.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

Hi Uwe,

On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > This patch adds the Spreadtrum PWM support, which provides maximum 4
> > channels.
> >
> > Signed-off-by: Neo Hou <neo.hou@unisoc.com>
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> > Changes from v1:
> >  - Add depending on HAS_IOMEM.
> >  - Rename parameters' names.
> >  - Implement .apply() instead of .config(), .enable() and .disable().
> >  - Use NSEC_PER_SEC instead of 1000000000ULL.
> >  - Add some comments to make code more readable.
> >  - Remove some redundant operation.
> >  - Use standard clock properties to set clock parent.
> >  - Other coding style optimization.
> > ---
> >  drivers/pwm/Kconfig    |   11 ++
> >  drivers/pwm/Makefile   |    1 +
> >  drivers/pwm/pwm-sprd.c |  307 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 319 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sprd.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a7e5751..31dfc88 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -423,6 +423,17 @@ config PWM_SPEAR
> >         To compile this driver as a module, choose M here: the module
> >         will be called pwm-spear.
> >
> > +config PWM_SPRD
> > +     tristate "Spreadtrum PWM support"
> > +     depends on ARCH_SPRD || COMPILE_TEST
> > +     depends on HAS_IOMEM
> > +     help
> > +       Generic PWM framework driver for the PWM controller on
> > +       Spreadtrum SoCs.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called pwm-sprd.
> > +
> >  config PWM_STI
> >       tristate "STiH4xx PWM support"
> >       depends on ARCH_STI
> > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > index 76b555b..26326ad 100644
> > --- a/drivers/pwm/Makefile
> > +++ b/drivers/pwm/Makefile
> > @@ -41,6 +41,7 @@ obj-$(CONFIG_PWM_ROCKCHIP)  += pwm-rockchip.o
> >  obj-$(CONFIG_PWM_SAMSUNG)    += pwm-samsung.o
> >  obj-$(CONFIG_PWM_SIFIVE)     += pwm-sifive.o
> >  obj-$(CONFIG_PWM_SPEAR)              += pwm-spear.o
> > +obj-$(CONFIG_PWM_SPRD)               += pwm-sprd.o
> >  obj-$(CONFIG_PWM_STI)                += pwm-sti.o
> >  obj-$(CONFIG_PWM_STM32)              += pwm-stm32.o
> >  obj-$(CONFIG_PWM_STM32_LP)   += pwm-stm32-lp.o
> > diff --git a/drivers/pwm/pwm-sprd.c b/drivers/pwm/pwm-sprd.c
> > new file mode 100644
> > index 0000000..067e711
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sprd.c
> > @@ -0,0 +1,307 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Spreadtrum Communications Inc.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +
> > +#define SPRD_PWM_PRESCALE    0x0
> > +#define SPRD_PWM_MOD         0x4
> > +#define SPRD_PWM_DUTY                0x8
> > +#define SPRD_PWM_ENABLE              0x18
> > +
> > +#define SPRD_PWM_MOD_MAX     GENMASK(7, 0)
> > +#define SPRD_PWM_DUTY_MSK    GENMASK(15, 0)
> > +#define SPRD_PWM_PRESCALE_MSK        GENMASK(7, 0)
> > +#define SPRD_PWM_ENABLE_BIT  BIT(0)
> > +
> > +#define SPRD_PWM_NUM         4
> > +#define SPRD_PWM_REGS_SHIFT  5
> > +#define SPRD_PWM_NUM_CLKS    2
> > +#define SPRD_PWM_OUTPUT_CLK  1
>
> These definitions could benefit from some explaining comments. Just from
> looking at the names it is for example not obvious what is the
> difference between SPRD_PWM_NUM and SPRD_PWM_NUM_CLKS is.

Sure, I will optimize the macros' names to make them understandable.

>
> > +struct sprd_pwm_chn {
> > +     struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
> > +     u32 clk_rate;
> > +};
> > +
> > +struct sprd_pwm_chip {
> > +     void __iomem *base;
> > +     struct device *dev;
> > +     struct pwm_chip chip;
> > +     int num_pwms;
> > +     struct sprd_pwm_chn chn[SPRD_PWM_NUM];
> > +};
> > +
> > +/*
> > + * The list of clocks required by PWM channels, and each channel has 2 clocks:
> > + * enable clock and pwm clock.
> > + */
> > +static const char * const sprd_pwm_clks[] = {
> > +     "enable0", "pwm0",
> > +     "enable1", "pwm1",
> > +     "enable2", "pwm2",
> > +     "enable3", "pwm3",
> > +};
> > +
> > +static u32 sprd_pwm_read(struct sprd_pwm_chip *spc, u32 hwid, u32 reg)
> > +{
> > +     u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> > +
> > +     return readl_relaxed(spc->base + offset);
> > +}
> > +
> > +static void sprd_pwm_write(struct sprd_pwm_chip *spc, u32 hwid,
> > +                        u32 reg, u32 val)
> > +{
> > +     u32 offset = reg + (hwid << SPRD_PWM_REGS_SHIFT);
> > +
> > +     writel_relaxed(val, spc->base + offset);
> > +}
> > +
> > +static void sprd_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                            struct pwm_state *state)
> > +{
> > +     struct sprd_pwm_chip *spc =
> > +             container_of(chip, struct sprd_pwm_chip, chip);
> > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > +     u32 val, duty, prescale;
> > +     u64 tmp;
> > +     int ret;
> > +
> > +     /*
> > +      * The clocks to PWM channel has to be enabled first before
> > +      * reading to the registers.
> > +      */
> > +     ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > +     if (ret) {
> > +             dev_err(spc->dev, "failed to enable pwm%u clocks\n",
> > +                     pwm->hwpwm);
> > +             return;
> > +     }
> > +
> > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE);
> > +     if (val & SPRD_PWM_ENABLE_BIT)
> > +             state->enabled = true;
> > +     else
> > +             state->enabled = false;
> > +
> > +     /*
> > +      * The hardware provides a counter that is feed by the source clock.
> > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > +      * Thus the period_ns and duty_ns calculation formula should be:
> > +      * period_ns = NSEC_PER_SEC * (prescale + 1) * mod / clk_rate
> > +      * duty_ns = NSEC_PER_SEC * (prescale + 1) * duty / clk_rate
> > +      */
> > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE);
> > +     prescale = val & SPRD_PWM_PRESCALE_MSK;
> > +     tmp = (prescale + 1) * NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > +     state->period = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > +     val = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY);
> > +     duty = val & SPRD_PWM_DUTY_MSK;
> > +     tmp = (prescale + 1) * NSEC_PER_SEC * duty;
> > +     state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, chn->clk_rate);
> > +
> > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > +     if (!state->enabled)
> > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > +}
> > +
> > +static int sprd_pwm_config(struct sprd_pwm_chip *spc, struct pwm_device *pwm,
> > +                        int duty_ns, int period_ns)
> > +{
> > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > +     u64 div, tmp;
> > +     u32 prescale, duty;
> > +
> > +     /*
> > +      * The hardware provides a counter that is feed by the source clock.
> > +      * The period length is (PRESCALE + 1) * MOD counter steps.
> > +      * The duty cycle length is (PRESCALE + 1) * DUTY counter steps.
> > +      *
> > +      * To keep the maths simple we're always using MOD = SPRD_PWM_MOD_MAX.
> > +      * The value for PRESCALE is selected such that the resulting period
> > +      * gets the maximal length not bigger than the requested one with the
> > +      * given settings (MOD = SPRD_PWM_MOD_MAX and input clock).
> > +      */
> > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > +
> > +     tmp = (u64)chn->clk_rate * period_ns;
> > +     div = NSEC_PER_SEC * SPRD_PWM_MOD_MAX;
> > +     prescale = div64_u64(tmp, div) - 1;
> > +     if (prescale > SPRD_PWM_PRESCALE_MSK)
> > +             prescale = SPRD_PWM_PRESCALE_MSK;
>
> This isn't the inverse of .get_state(). Consider:
>
>         clk_rate = 3333333
>         SPRD_PWM_PRESCALE = 15
>
> then you calculate in .get_state():
>
>         period = 1224000
>
> If you then call apply with this value you calulate:
>
>         prescale = 14

OK. I will optimize the logics to fix this issue in next version.

> > +
> > +     /*
> > +      * Note: The MOD must be configured before DUTY, and the hardware can
> > +      * ensure current running period is completed before changing a new
> > +      * configuration to avoid mixed settings.
>
> You write "the hardware can ensure ..". Does that actually means "The
> hardware ensures that ..." or is there some additional condition? Maybe

Yes, will update the comments.

> you mean:
>
>         /*
>          * Writing DUTY triggers the hardware to actually apply the
>          * values written to MOD and DUTY to the output. So write DUTY
>          * last.
>          */

Not really, our hardware's method is, when you changed a new
configuration (MOD or duty is changed) , the hardware will wait for a
while to complete current period, then change to the new period.

>
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
>
> If writing DUTY triggers the hardware to sample DUTY and MOD, what about
> PRESCALE?

See above comments.

>
> > +     return 0;
> > +}
> > +
> > +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                       struct pwm_state *state)
> > +{
> > +     struct sprd_pwm_chip *spc =
> > +             container_of(chip, struct sprd_pwm_chip, chip);
> > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > +     struct pwm_state cstate;
> > +     int ret;
> > +
> > +     pwm_get_state(pwm, &cstate);
>
> I don't like it when pwm drivers call pwm_get_state(). If ever
> pwm_get_state would take a lock, this would deadlock as the lock is
> probably already taken when your .apply() callback is running. Moreover
> the (expensive) calculations are not used appropriately. See below.

I do not think so, see:

static inline void pwm_get_state(const struct pwm_device *pwm, struct
pwm_state *state)
{
        *state = pwm->state;
}

>
> > +     if (state->enabled) {
> > +             if (!cstate.enabled) {
>
> To just know the value of cstate.enabled you only need to read the
> register with the ENABLE flag. That is cheaper than calling get_state.
>
> > +                     /*
> > +                      * The clocks to PWM channel has to be enabled first
> > +                      * before writing to the registers.
> > +                      */
> > +                     ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> > +                                                   chn->clks);
> > +                     if (ret) {
> > +                             dev_err(spc->dev,
> > +                                     "failed to enable pwm%u clocks\n",
> > +                                     pwm->hwpwm);
> > +                             return ret;
> > +                     }
> > +             }
> > +
> > +             if (state->period != cstate.period ||
> > +                 state->duty_cycle != cstate.duty_cycle) {
>
> This is a coarse check. If state->period and cstate.period only differ
> by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
> state->period) probably results in a noop. So you're doing an expensive
> division to get an unreliable check. It would be better to calculate the
> register values from the requested state and compare the register
> values. The costs are more or less the same than calling .get_state and
> the check is reliable. And you don't need to spend another division to
> calculate the new register values.

Same as above comment.

>
> > +                     ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> > +                                           state->period);
> > +                     if (ret)
> > +                             return ret;
> > +             }
> > +
> > +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> > +     } else if (cstate.enabled) {
> > +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> > +
> > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
>
> Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
> currently running period and the write doesn't block that long: Does
> disabling the clocks interfere with completing the period?

Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
waiting for completing the currently period.

>
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct pwm_ops sprd_pwm_ops = {
> > +     .apply = sprd_pwm_apply,
> > +     .get_state = sprd_pwm_get_state,
> > +     .owner = THIS_MODULE,
> > +};
> > +
> > +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> > +{
> > +     struct clk *clk_pwm;
> > +     int ret, i, clk_index = 0;
> > +
> > +     for (i = 0; i < SPRD_PWM_NUM; i++) {
> > +             struct sprd_pwm_chn *chn = &spc->chn[i];
> > +             int j;
> > +
> > +             for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
> > +                     chn->clks[j].id = sprd_pwm_clks[clk_index++];
>
> I think this would be more understandable when written as:
>
>         for (j = 0; j < SPRD_PWM_NUM_CLKS; ++j)
>                 chn->clks[j].id = sprd_pwm_clks[i * SPRD_PWM_NUM_CLKS + j];
>
> but I'm not sure I'm objective here.

Sure.

>
> > +
> > +             ret = devm_clk_bulk_get(spc->dev, SPRD_PWM_NUM_CLKS, chn->clks);
> > +             if (ret) {
> > +                     if (ret == -ENOENT)
> > +                             break;
> > +
> > +                     dev_err(spc->dev, "failed to get channel clocks\n");
>
> if ret == -EPROBE_DEFER you shouldn't issue an error message.

Yes.

>
> > +                     return ret;
> > +             }
> > +
> > +             clk_pwm = chn->clks[SPRD_PWM_OUTPUT_CLK].clk;
> > +             chn->clk_rate = clk_get_rate(clk_pwm);
> > +     }
> > +
> > +     if (!i) {
> > +             dev_err(spc->dev, "no available PWM channels\n");
> > +             return -EINVAL;
>
> ENODEV?

Yes

>
> > +     }
> > +
> > +     spc->num_pwms = i;
> > +
> > +     return 0;
> > +}
> > +
> > +static int sprd_pwm_probe(struct platform_device *pdev)
> > +{
> > +     struct sprd_pwm_chip *spc;
> > +     int ret;
> > +
> > +     spc = devm_kzalloc(&pdev->dev, sizeof(*spc), GFP_KERNEL);
> > +     if (!spc)
> > +             return -ENOMEM;
> > +
> > +     spc->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(spc->base))
> > +             return PTR_ERR(spc->base);
> > +
> > +     spc->dev = &pdev->dev;
> > +     platform_set_drvdata(pdev, spc);
> > +
> > +     ret = sprd_pwm_clk_init(spc);
> > +     if (ret)
> > +             return ret;
> > +
> > +     spc->chip.dev = &pdev->dev;
> > +     spc->chip.ops = &sprd_pwm_ops;
> > +     spc->chip.base = -1;
> > +     spc->chip.npwm = spc->num_pwms;
> > +
> > +     ret = pwmchip_add(&spc->chip);
> > +     if (ret)
> > +             dev_err(&pdev->dev, "failed to add PWM chip\n");
> > +
> > +     return ret;
> > +}
> > +
> > +static int sprd_pwm_remove(struct platform_device *pdev)
> > +{
> > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > +     int ret, i;
> > +
> > +     ret = pwmchip_remove(&spc->chip);
> > +
> > +     for (i = 0; i < spc->num_pwms; i++) {
> > +             struct sprd_pwm_chn *chn = &spc->chn[i];
> > +
> > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
>
> If a PWM was still running you're effectively stopping it here, right?
> Are you sure you don't disable once more than you enabled?

Yes, you are right. I should check current enable status of the PWM channel.
Thanks for your comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Uwe Kleine-König]

Hello,

On Wed, Aug 14, 2019 at 03:52:08PM +0800, Baolin Wang wrote:
> On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> >> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> >> <u.kleine-koenig@pengutronix.de> wrote:
> >> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> >> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> >> > > <u.kleine-koenig@pengutronix.de> wrote:
> >> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> >> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
> >> > > > > clock.
> >> > > >
> >> > > > I'm not sure you need to point out assigned-clocks and
> >> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> >> > > > these should be "required properties".
> >> > >
> >> > > I think I should describe any properties used by PWM node, like
> >> > > 'clocks' and 'clock-names' properties, though they are common clock
> >> > > properties.
> >> >
> >> > Then you might want to describe also "status", "assigned-clock-rates",
> >> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
> >> > and
> >> > probably another dozen I'm not aware of.
> >>
> >> We usually do not describe 'status', but if your device node used
> >> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> >> describe them to let users know what is the purpose of these
> >> properties. That's also asked by DT maintainer Rob.
> >
> > Does this convince you that you should also describe "pinctrl-$n" and
> > the others? If not, why not? We also usually don't describe
> 
> Our PWM device node did not use "pinctrl-$n" things, why I should
> describe "pinctrl-$n"?

The binding you implemented supports "pinctrl-$n". And this is described
somewhere in the generic pinctrl binding docs. The same applies to
"assigned-clock-parents".

That you happen to use assigned-clock-parents but not pinctrl-$n on the
board you used to develop your driver is a detail that IMHO shouldn't
decide about being listed in the binding doc for your PWM type.

> If some devices use pinctrl, yes, they should describe what is the
> purpose of pinctrl, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt

I agree that if the driver assumes special pinctrl names this is worth
mentioning. If however there is nothing special and just some generic
stuff is used that is described in some other location then I'd chose to
not add this redundant information to the binding document. So if I
reviewed the patch that created
Documentation/devicetree/bindings/mmc/sdhci-sprd.txt I would have
suggested to drop "assigned-clocks" and "assigned-clock-parents" there,
too.

> > assigned-clock-parents. For me these are all in the same catagory:
> 
> Lots of dt bindings describe 'assigned-clock-parents',:
> ./Documentation/devicetree/bindings/display/msm/dsi.txt
> ./Documentation/devicetree/bindings/display/msm/dsi.txt
> ./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
> ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> ./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
> ./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
> ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> ......

I didn't check each of them, but I assume the same applies here, too.
Please don't copy blindly but think before using other people's stuff as
reference. Even in the Linux kernel where reviews seem and are told to
catch non-optimal stuff, there are places where this doesn't work. IMHO
the key question is: Does it add value to describe "assigned-clocks" in
the binding for your PWM device given that you're only using this
generic and well documented construct?

> > Common properties supported for each devicetree node that represents a
> > device. The only difference is that on your board you make use of some
> > but not some others.
> 
> Fine, let's decide this by PWM maintainer or DT maintainer Rob.

Fine for me.

> >> > > Yes, they are required. Thanks for your comments.
> >> >
> >> > required in which sense? Why can a Spreadtrum PWM not work when the
> >> > clock parents are unspecified?
> >>
> >> On some Spreadtrum platforms, the default source clock of PWM may not
> >> be enabled, so we should force users to select one available source
> >> clock for PWM output clock.
> >
> > Sounds like a bug in the clk tree of your SoC that shouldn't affect how
> > the PWM is described in the device tree. After all a parent of a clock
> > is supposed to become enabled when the clock gets enabled.
> 
> That's not a bug, that's like a MUX, the default source clock of PWM
> can be disabled, since we usually do not use the default source clock.
> Then we can select a available source clock by the MUX.

In my eyes there is a difference between a) The way the clocks are
implemented in the XZ SoC implies that to actually use the PWM you need
to reparent some clock; and b) Each "sprd,ums512-pwm" device really
needs an "assigned-clock" property, otherwise it cannot work.

If you write "required" in the binding doc the semantic should be b) but
the motivation here seems to be a). Legal questions aside someone could
implement a PWM that has the same register layout and behaviour as the
PWM in your SoC but with a different clock tree. Should they use a
different compatible just because they don't need "assigned-clock"?

Best regards
Uwe

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

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello Baolin,

On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > [...]
> Not really, our hardware's method is, when you changed a new
> configuration (MOD or duty is changed) , the hardware will wait for a
> while to complete current period, then change to the new period.

Can you describe that in more detail? This doesn't explain why MOD must be
configured before DUTY. Is there another reason for that?

> > > +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +                       struct pwm_state *state)
> > > +{
> > > +     struct sprd_pwm_chip *spc =
> > > +             container_of(chip, struct sprd_pwm_chip, chip);
> > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > +     struct pwm_state cstate;
> > > +     int ret;
> > > +
> > > +     pwm_get_state(pwm, &cstate);
> >
> > I don't like it when pwm drivers call pwm_get_state(). If ever
> > pwm_get_state would take a lock, this would deadlock as the lock is
> > probably already taken when your .apply() callback is running. Moreover
> > the (expensive) calculations are not used appropriately. See below.
> 
> I do not think so, see:
> 
> static inline void pwm_get_state(const struct pwm_device *pwm, struct
> pwm_state *state)
> {
>         *state = pwm->state;
> }

OK, the PWM framework currently caches this for you. Still I would
prefer if you didn't call PWM API functions in your lowlevel driver.
There is (up to now) nothing bad that will happen if you do it anyhow,
but when the PWM framework evolves it might (and I want to work on such
an evolution). You must not call clk_get_rate() in a .set_rate()
callback of a clock either.
 
> > > +     if (state->enabled) {
> > > +             if (!cstate.enabled) {
> >
> > To just know the value of cstate.enabled you only need to read the
> > register with the ENABLE flag. That is cheaper than calling get_state.
> >
> > > +                     /*
> > > +                      * The clocks to PWM channel has to be enabled first
> > > +                      * before writing to the registers.
> > > +                      */
> > > +                     ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> > > +                                                   chn->clks);
> > > +                     if (ret) {
> > > +                             dev_err(spc->dev,
> > > +                                     "failed to enable pwm%u clocks\n",
> > > +                                     pwm->hwpwm);
> > > +                             return ret;
> > > +                     }
> > > +             }
> > > +
> > > +             if (state->period != cstate.period ||
> > > +                 state->duty_cycle != cstate.duty_cycle) {
> >
> > This is a coarse check. If state->period and cstate.period only differ
> > by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
> > state->period) probably results in a noop. So you're doing an expensive
> > division to get an unreliable check. It would be better to calculate the
> > register values from the requested state and compare the register
> > values. The costs are more or less the same than calling .get_state and
> > the check is reliable. And you don't need to spend another division to
> > calculate the new register values.
> 
> Same as above comment.

When taking the caching into account (which I wouldn't) the check is
still incomplete. OK, you could argue avoiding the recalculation in 90%
(to just say some number) of the cases where it is unnecessary is good.
 
> >
> > > +                     ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> > > +                                           state->period);
> > > +                     if (ret)
> > > +                             return ret;
> > > +             }
> > > +
> > > +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> > > +     } else if (cstate.enabled) {
> > > +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> > > +
> > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> >
> > Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
> > currently running period and the write doesn't block that long: Does
> > disabling the clocks interfere with completing the period?
> 
> Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
> waiting for completing the currently period.

The currently active period is supposed to be completed. If you cannot
implement this please point this out as limitation at the top of the
driver.

Honestly I think most pwm users won't mind and they should get the
possibility to tell they prefer pwm_apply_state to return immediately
even if this could result in a spike. But we're not there yet.
(Actually there are three things a PWM consumer might want:

 a) stop immediately;
 b) complete the currently running period, then stop and return only
    when the period is completed; or
 c) complete the currently running period and then stop, return immediately if
    possible.

Currently the expected semantic is b).

> > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > +{
> > > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > +     int ret, i;
> > > +
> > > +     ret = pwmchip_remove(&spc->chip);
> > > +
> > > +     for (i = 0; i < spc->num_pwms; i++) {
> > > +             struct sprd_pwm_chn *chn = &spc->chn[i];
> > > +
> > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> >
> > If a PWM was still running you're effectively stopping it here, right?
> > Are you sure you don't disable once more than you enabled?
> 
> Yes, you are right. I should check current enable status of the PWM channel.
> Thanks for your comments.

I didn't recheck, but I think the right approach is to not fiddle with
the clocks at all and rely on the PWM framework to not let someone call
sprd_pwm_remove when a PWM is still in use.

Best regards
Uwe

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

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Baolin Wang]

On Wed, 14 Aug 2019 at 16:49, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Wed, Aug 14, 2019 at 03:52:08PM +0800, Baolin Wang wrote:
> > On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > > On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> > >> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> > >> <u.kleine-koenig@pengutronix.de> wrote:
> > >> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > >> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > >> > > <u.kleine-koenig@pengutronix.de> wrote:
> > >> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > >> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
> > >> > > > > clock.
> > >> > > >
> > >> > > > I'm not sure you need to point out assigned-clocks and
> > >> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > >> > > > these should be "required properties".
> > >> > >
> > >> > > I think I should describe any properties used by PWM node, like
> > >> > > 'clocks' and 'clock-names' properties, though they are common clock
> > >> > > properties.
> > >> >
> > >> > Then you might want to describe also "status", "assigned-clock-rates",
> > >> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
> > >> > and
> > >> > probably another dozen I'm not aware of.
> > >>
> > >> We usually do not describe 'status', but if your device node used
> > >> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> > >> describe them to let users know what is the purpose of these
> > >> properties. That's also asked by DT maintainer Rob.
> > >
> > > Does this convince you that you should also describe "pinctrl-$n" and
> > > the others? If not, why not? We also usually don't describe
> >
> > Our PWM device node did not use "pinctrl-$n" things, why I should
> > describe "pinctrl-$n"?
>
> The binding you implemented supports "pinctrl-$n". And this is described
> somewhere in the generic pinctrl binding docs. The same applies to
> "assigned-clock-parents".
>
> That you happen to use assigned-clock-parents but not pinctrl-$n on the
> board you used to develop your driver is a detail that IMHO shouldn't
> decide about being listed in the binding doc for your PWM type.
>
> > If some devices use pinctrl, yes, they should describe what is the
> > purpose of pinctrl, see:
> > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt
>
> I agree that if the driver assumes special pinctrl names this is worth
> mentioning. If however there is nothing special and just some generic
> stuff is used that is described in some other location then I'd chose to
> not add this redundant information to the binding document. So if I
> reviewed the patch that created
> Documentation/devicetree/bindings/mmc/sdhci-sprd.txt I would have
> suggested to drop "assigned-clocks" and "assigned-clock-parents" there,
> too.
>
> > > assigned-clock-parents. For me these are all in the same catagory:
> >
> > Lots of dt bindings describe 'assigned-clock-parents',:
> > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > ./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
> > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > ./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
> > ./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
> > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > ......
>
> I didn't check each of them, but I assume the same applies here, too.
> Please don't copy blindly but think before using other people's stuff as

I did not  copy blindly.

> reference. Even in the Linux kernel where reviews seem and are told to
> catch non-optimal stuff, there are places where this doesn't work. IMHO
> the key question is: Does it add value to describe "assigned-clocks" in
> the binding for your PWM device given that you're only using this
> generic and well documented construct?

I just want to remind users that they should set the parent clock for
PWMs, otherwise the PWM source clock can be not available.

>
> > > Common properties supported for each devicetree node that represents a
> > > device. The only difference is that on your board you make use of some
> > > but not some others.
> >
> > Fine, let's decide this by PWM maintainer or DT maintainer Rob.
>
> Fine for me.
>
> > >> > > Yes, they are required. Thanks for your comments.
> > >> >
> > >> > required in which sense? Why can a Spreadtrum PWM not work when the
> > >> > clock parents are unspecified?
> > >>
> > >> On some Spreadtrum platforms, the default source clock of PWM may not
> > >> be enabled, so we should force users to select one available source
> > >> clock for PWM output clock.
> > >
> > > Sounds like a bug in the clk tree of your SoC that shouldn't affect how
> > > the PWM is described in the device tree. After all a parent of a clock
> > > is supposed to become enabled when the clock gets enabled.
> >
> > That's not a bug, that's like a MUX, the default source clock of PWM
> > can be disabled, since we usually do not use the default source clock.
> > Then we can select a available source clock by the MUX.
>
> In my eyes there is a difference between a) The way the clocks are
> implemented in the XZ SoC implies that to actually use the PWM you need
> to reparent some clock; and b) Each "sprd,ums512-pwm" device really
> needs an "assigned-clock" property, otherwise it cannot work.
>
> If you write "required" in the binding doc the semantic should be b) but
> the motivation here seems to be a). Legal questions aside someone could
> implement a PWM that has the same register layout and behaviour as the
> PWM in your SoC but with a different clock tree. Should they use a
> different compatible just because they don't need "assigned-clock"?

Fair enough, I move them to be optional.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/2] dt-bindings: pwm: sprd: Add Spreadtrum PWM documentation

[Uwe Kleine-König]

Hello,

On Wed, Aug 14, 2019 at 05:33:25PM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 16:49, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Aug 14, 2019 at 03:52:08PM +0800, Baolin Wang wrote:
> > > On 14/08/2019, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Wed, Aug 14, 2019 at 03:25:53PM +0800, Baolin Wang wrote:
> > > >> On Wed, 14 Aug 2019 at 15:01, Uwe Kleine-König
> > > >> <u.kleine-koenig@pengutronix.de> wrote:
> > > >> > On Wed, Aug 14, 2019 at 09:51:34AM +0800, Baolin Wang wrote:
> > > >> > > On Tue, 13 Aug 2019 at 22:13, Uwe Kleine-König
> > > >> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >> > > > On Tue, Aug 13, 2019 at 09:46:40PM +0800, Baolin Wang wrote:
> > > >> > > > > +- assigned-clock-parents: The phandle of the parent clock of PWM
> > > >> > > > > clock.
> > > >> > > >
> > > >> > > > I'm not sure you need to point out assigned-clocks and
> > > >> > > > assigned-clock-parents as this is general clk stuff. Also I wonder if
> > > >> > > > these should be "required properties".
> > > >> > >
> > > >> > > I think I should describe any properties used by PWM node, like
> > > >> > > 'clocks' and 'clock-names' properties, though they are common clock
> > > >> > > properties.
> > > >> >
> > > >> > Then you might want to describe also "status", "assigned-clock-rates",
> > > >> > "pinctrl-$n", "pinctrl-names", "power-domains", "power-domain-names"
> > > >> > and
> > > >> > probably another dozen I'm not aware of.
> > > >>
> > > >> We usually do not describe 'status', but if your device node used
> > > >> "pinctrl-$n", "pinctrl-names" ... common properties, yes, you should
> > > >> describe them to let users know what is the purpose of these
> > > >> properties. That's also asked by DT maintainer Rob.
> > > >
> > > > Does this convince you that you should also describe "pinctrl-$n" and
> > > > the others? If not, why not? We also usually don't describe
> > >
> > > Our PWM device node did not use "pinctrl-$n" things, why I should
> > > describe "pinctrl-$n"?
> >
> > The binding you implemented supports "pinctrl-$n". And this is described
> > somewhere in the generic pinctrl binding docs. The same applies to
> > "assigned-clock-parents".
> >
> > That you happen to use assigned-clock-parents but not pinctrl-$n on the
> > board you used to develop your driver is a detail that IMHO shouldn't
> > decide about being listed in the binding doc for your PWM type.
> >
> > > If some devices use pinctrl, yes, they should describe what is the
> > > purpose of pinctrl, see:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mmc/sdhci-sprd.txt
> >
> > I agree that if the driver assumes special pinctrl names this is worth
> > mentioning. If however there is nothing special and just some generic
> > stuff is used that is described in some other location then I'd chose to
> > not add this redundant information to the binding document. So if I
> > reviewed the patch that created
> > Documentation/devicetree/bindings/mmc/sdhci-sprd.txt I would have
> > suggested to drop "assigned-clocks" and "assigned-clock-parents" there,
> > too.
> >
> > > > assigned-clock-parents. For me these are all in the same catagory:
> > >
> > > Lots of dt bindings describe 'assigned-clock-parents',:
> > > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > > ./Documentation/devicetree/bindings/display/msm/dsi.txt
> > > ./Documentation/devicetree/bindings/display/mediatek/mediatek,hdmi.txt
> > > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > > ./Documentation/devicetree/bindings/rtc/st,stm32-rtc.txt
> > > ./Documentation/devicetree/bindings/pci/rockchip-pcie-host.txt
> > > ./Documentation/devicetree/bindings/sound/mt2701-afe-pcm.txt
> > > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > > ./Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt
> > > ......
> >
> > I didn't check each of them, but I assume the same applies here, too.
> > Please don't copy blindly but think before using other people's stuff as
> 
> I did not  copy blindly.

OK, there was no offence intended.

> > reference. Even in the Linux kernel where reviews seem and are told to
> > catch non-optimal stuff, there are places where this doesn't work. IMHO
> > the key question is: Does it add value to describe "assigned-clocks" in
> > the binding for your PWM device given that you're only using this
> > generic and well documented construct?
> 
> I just want to remind users that they should set the parent clock for
> PWMs, otherwise the PWM source clock can be not available.

Probably it is just subjective where to draw the line here. There are a
thousand and one things that can go wrong when the PWM should be used.
To me it seems artificial to pick one of these and mention it in a
document that is supposed to describe how to formalize such a device.

But given that we're going in cycles, I will stop trying to convince you
now.

Best regards
Uwe

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

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

Hi Uwe,

On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > [...]
> > Not really, our hardware's method is, when you changed a new
> > configuration (MOD or duty is changed) , the hardware will wait for a
> > while to complete current period, then change to the new period.
>
> Can you describe that in more detail? This doesn't explain why MOD must be
> configured before DUTY. Is there another reason for that?

Sorry, I did not explain this explicitly. When we change a new PWM
configuration, the PWM controller will make sure the current period is
completed before changing to a new period. Once setting the MOD
register (since we always set MOD firstly), that will tell the
hardware that a new period need to change.

The reason MOD must be configured before DUTY is that, if we
configured DUTY firstly, the PWM can work abnormally if the current
DUTY is larger than previous MOD. That is also our hardware's
limitation.

>
> > > > +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +                       struct pwm_state *state)
> > > > +{
> > > > +     struct sprd_pwm_chip *spc =
> > > > +             container_of(chip, struct sprd_pwm_chip, chip);
> > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > +     struct pwm_state cstate;
> > > > +     int ret;
> > > > +
> > > > +     pwm_get_state(pwm, &cstate);
> > >
> > > I don't like it when pwm drivers call pwm_get_state(). If ever
> > > pwm_get_state would take a lock, this would deadlock as the lock is
> > > probably already taken when your .apply() callback is running. Moreover
> > > the (expensive) calculations are not used appropriately. See below.
> >
> > I do not think so, see:
> >
> > static inline void pwm_get_state(const struct pwm_device *pwm, struct
> > pwm_state *state)
> > {
> >         *state = pwm->state;
> > }
>
> OK, the PWM framework currently caches this for you. Still I would
> prefer if you didn't call PWM API functions in your lowlevel driver.
> There is (up to now) nothing bad that will happen if you do it anyhow,
> but when the PWM framework evolves it might (and I want to work on such
> an evolution). You must not call clk_get_rate() in a .set_rate()
> callback of a clock either.

Make sense, I will change to:

struct pwm_state *cstate = pwm->state;

>
> > > > +     if (state->enabled) {
> > > > +             if (!cstate.enabled) {
> > >
> > > To just know the value of cstate.enabled you only need to read the
> > > register with the ENABLE flag. That is cheaper than calling get_state.
> > >
> > > > +                     /*
> > > > +                      * The clocks to PWM channel has to be enabled first
> > > > +                      * before writing to the registers.
> > > > +                      */
> > > > +                     ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
> > > > +                                                   chn->clks);
> > > > +                     if (ret) {
> > > > +                             dev_err(spc->dev,
> > > > +                                     "failed to enable pwm%u clocks\n",
> > > > +                                     pwm->hwpwm);
> > > > +                             return ret;
> > > > +                     }
> > > > +             }
> > > > +
> > > > +             if (state->period != cstate.period ||
> > > > +                 state->duty_cycle != cstate.duty_cycle) {
> > >
> > > This is a coarse check. If state->period and cstate.period only differ
> > > by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
> > > state->period) probably results in a noop. So you're doing an expensive
> > > division to get an unreliable check. It would be better to calculate the
> > > register values from the requested state and compare the register
> > > values. The costs are more or less the same than calling .get_state and
> > > the check is reliable. And you don't need to spend another division to
> > > calculate the new register values.
> >
> > Same as above comment.
>
> When taking the caching into account (which I wouldn't) the check is
> still incomplete. OK, you could argue avoiding the recalculation in 90%
> (to just say some number) of the cases where it is unnecessary is good.

Yes :)

>
> > >
> > > > +                     ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
> > > > +                                           state->period);
> > > > +                     if (ret)
> > > > +                             return ret;
> > > > +             }
> > > > +
> > > > +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
> > > > +     } else if (cstate.enabled) {
> > > > +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
> > > > +
> > > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > >
> > > Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
> > > currently running period and the write doesn't block that long: Does
> > > disabling the clocks interfere with completing the period?
> >
> > Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
> > waiting for completing the currently period.
>
> The currently active period is supposed to be completed. If you cannot
> implement this please point this out as limitation at the top of the
> driver.

Sure.

>
> Honestly I think most pwm users won't mind and they should get the
> possibility to tell they prefer pwm_apply_state to return immediately
> even if this could result in a spike. But we're not there yet.
> (Actually there are three things a PWM consumer might want:
>
>  a) stop immediately;
>  b) complete the currently running period, then stop and return only
>     when the period is completed; or
>  c) complete the currently running period and then stop, return immediately if
>     possible.
>
> Currently the expected semantic is b).

Make sense.

>
> > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > +     int ret, i;
> > > > +
> > > > +     ret = pwmchip_remove(&spc->chip);
> > > > +
> > > > +     for (i = 0; i < spc->num_pwms; i++) {
> > > > +             struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > +
> > > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > >
> > > If a PWM was still running you're effectively stopping it here, right?
> > > Are you sure you don't disable once more than you enabled?
> >
> > Yes, you are right. I should check current enable status of the PWM channel.
> > Thanks for your comments.
>
> I didn't recheck, but I think the right approach is to not fiddle with
> the clocks at all and rely on the PWM framework to not let someone call
> sprd_pwm_remove when a PWM is still in use.

So you mean just return pwmchip_remove(&spc->chip); ?

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello Baolin,

On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > > [...]
> > > Not really, our hardware's method is, when you changed a new
> > > configuration (MOD or duty is changed) , the hardware will wait for a
> > > while to complete current period, then change to the new period.
> >
> > Can you describe that in more detail? This doesn't explain why MOD must be
> > configured before DUTY. Is there another reason for that?
> 
> Sorry, I did not explain this explicitly. When we change a new PWM
> configuration, the PWM controller will make sure the current period is
> completed before changing to a new period. Once setting the MOD
> register (since we always set MOD firstly), that will tell the
> hardware that a new period need to change.

So if the current period just ended after you reconfigured MOD but
before you wrote to DUTY we'll see a bogus period, right? I assume the
same holds true for writing the prescale value?

> The reason MOD must be configured before DUTY is that, if we
> configured DUTY firstly, the PWM can work abnormally if the current
> DUTY is larger than previous MOD. That is also our hardware's
> limitation.

OK, so you must not get into a situation where DUTY > MOD, right?

Now if the hardware was configured for

	period = 8s, duty = 4s

and now you are supposed to change to

	period = 2s, duty = 1s

you'd need to write DUTY first, don't you?

> > > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > > +{
> > > > > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > > +     int ret, i;
> > > > > +
> > > > > +     ret = pwmchip_remove(&spc->chip);
> > > > > +
> > > > > +     for (i = 0; i < spc->num_pwms; i++) {
> > > > > +             struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > > +
> > > > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > >
> > > > If a PWM was still running you're effectively stopping it here, right?
> > > > Are you sure you don't disable once more than you enabled?
> > >
> > > Yes, you are right. I should check current enable status of the PWM channel.
> > > Thanks for your comments.
> >
> > I didn't recheck, but I think the right approach is to not fiddle with
> > the clocks at all and rely on the PWM framework to not let someone call
> > sprd_pwm_remove when a PWM is still in use.
> 
> So you mean just return pwmchip_remove(&spc->chip); ?

right.

I just rechecked: If there is still a pwm in use, pwmchip_remove returns
-EBUSY. So this should be safe.

Best regards
Uwe

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

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Baolin Wang]

Hi Uwe,

On Wed, 14 Aug 2019 at 18:55, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote:
> > On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > > > [...]
> > > > Not really, our hardware's method is, when you changed a new
> > > > configuration (MOD or duty is changed) , the hardware will wait for a
> > > > while to complete current period, then change to the new period.
> > >
> > > Can you describe that in more detail? This doesn't explain why MOD must be
> > > configured before DUTY. Is there another reason for that?
> >
> > Sorry, I did not explain this explicitly. When we change a new PWM
> > configuration, the PWM controller will make sure the current period is
> > completed before changing to a new period. Once setting the MOD
> > register (since we always set MOD firstly), that will tell the
> > hardware that a new period need to change.
>
> So if the current period just ended after you reconfigured MOD but
> before you wrote to DUTY we'll see a bogus period, right? I assume the
> same holds true for writing the prescale value?

I confirmed again, I am sorry I missed something before. Yes, like you
said before, writing DUTY triggers the hardware to actually apply the
values written to MOD and DUTY to the output. So write DUTY last. I
will update the comments and change the PWM configure like:

sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);

>
> > The reason MOD must be configured before DUTY is that, if we
> > configured DUTY firstly, the PWM can work abnormally if the current
> > DUTY is larger than previous MOD. That is also our hardware's
> > limitation.
>
> OK, so you must not get into a situation where DUTY > MOD, right?
>
> Now if the hardware was configured for
>
>         period = 8s, duty = 4s
>
> and now you are supposed to change to
>
>         period = 2s, duty = 1s
>
> you'd need to write DUTY first, don't you?
>
> > > > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > > > +{
> > > > > > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > > > +     int ret, i;
> > > > > > +
> > > > > > +     ret = pwmchip_remove(&spc->chip);
> > > > > > +
> > > > > > +     for (i = 0; i < spc->num_pwms; i++) {
> > > > > > +             struct sprd_pwm_chn *chn = &spc->chn[i];
> > > > > > +
> > > > > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > > >
> > > > > If a PWM was still running you're effectively stopping it here, right?
> > > > > Are you sure you don't disable once more than you enabled?
> > > >
> > > > Yes, you are right. I should check current enable status of the PWM channel.
> > > > Thanks for your comments.
> > >
> > > I didn't recheck, but I think the right approach is to not fiddle with
> > > the clocks at all and rely on the PWM framework to not let someone call
> > > sprd_pwm_remove when a PWM is still in use.
> >
> > So you mean just return pwmchip_remove(&spc->chip); ?
>
> right.
>
> I just rechecked: If there is still a pwm in use, pwmchip_remove returns
> -EBUSY. So this should be safe.

Yes. Thanks for your comments.


--
Baolin Wang
Best Regards

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

Hello,

On Wed, Aug 14, 2019 at 08:23:37PM +0800, Baolin Wang wrote:
> On Wed, 14 Aug 2019 at 18:55, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Aug 14, 2019 at 06:01:50PM +0800, Baolin Wang wrote:
> > > On Wed, 14 Aug 2019 at 17:23, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
> > > > > On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
> > > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > > On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
> > > > > > [...]
> > > > > Not really, our hardware's method is, when you changed a new
> > > > > configuration (MOD or duty is changed) , the hardware will wait for a
> > > > > while to complete current period, then change to the new period.
> > > >
> > > > Can you describe that in more detail? This doesn't explain why MOD must be
> > > > configured before DUTY. Is there another reason for that?
> > >
> > > Sorry, I did not explain this explicitly. When we change a new PWM
> > > configuration, the PWM controller will make sure the current period is
> > > completed before changing to a new period. Once setting the MOD
> > > register (since we always set MOD firstly), that will tell the
> > > hardware that a new period need to change.
> >
> > So if the current period just ended after you reconfigured MOD but
> > before you wrote to DUTY we'll see a bogus period, right? I assume the
> > same holds true for writing the prescale value?
> 
> I confirmed again, I am sorry I missed something before. Yes, like you
> said before, writing DUTY triggers the hardware to actually apply the
> values written to MOD and DUTY to the output. So write DUTY last. I
> will update the comments and change the PWM configure like:
> 
> sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);

So PRESCALE is independent and it can still happen that writing PRESCALE
affects the output before MOD and DUTY do?

Best regards
Uwe

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

Re: [PATCH v2 2/2] pwm: sprd: Add Spreadtrum PWM support

[Michal Vokáč]

On 14. 08. 19 11:23, Uwe Kleine-König wrote:
> Hello Baolin,
> 
> On Wed, Aug 14, 2019 at 04:42:28PM +0800, Baolin Wang wrote:
>> On Tue, 13 Aug 2019 at 23:16, Uwe Kleine-König
>> <u.kleine-koenig@pengutronix.de> wrote:
>>> On Tue, Aug 13, 2019 at 09:46:41PM +0800, Baolin Wang wrote:
>>> [...]
>> Not really, our hardware's method is, when you changed a new
>> configuration (MOD or duty is changed) , the hardware will wait for a
>> while to complete current period, then change to the new period.
> 
> Can you describe that in more detail? This doesn't explain why MOD must be
> configured before DUTY. Is there another reason for that?
> 
>>>> +static int sprd_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>>> +                       struct pwm_state *state)
>>>> +{
>>>> +     struct sprd_pwm_chip *spc =
>>>> +             container_of(chip, struct sprd_pwm_chip, chip);
>>>> +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
>>>> +     struct pwm_state cstate;
>>>> +     int ret;
>>>> +
>>>> +     pwm_get_state(pwm, &cstate);
>>>
>>> I don't like it when pwm drivers call pwm_get_state(). If ever
>>> pwm_get_state would take a lock, this would deadlock as the lock is
>>> probably already taken when your .apply() callback is running. Moreover
>>> the (expensive) calculations are not used appropriately. See below.
>>
>> I do not think so, see:
>>
>> static inline void pwm_get_state(const struct pwm_device *pwm, struct
>> pwm_state *state)
>> {
>>          *state = pwm->state;
>> }
> 
> OK, the PWM framework currently caches this for you. Still I would
> prefer if you didn't call PWM API functions in your lowlevel driver.
> There is (up to now) nothing bad that will happen if you do it anyhow,
> but when the PWM framework evolves it might (and I want to work on such
> an evolution). You must not call clk_get_rate() in a .set_rate()
> callback of a clock either.
>   
>>>> +     if (state->enabled) {
>>>> +             if (!cstate.enabled) {
>>>
>>> To just know the value of cstate.enabled you only need to read the
>>> register with the ENABLE flag. That is cheaper than calling get_state.
>>>
>>>> +                     /*
>>>> +                      * The clocks to PWM channel has to be enabled first
>>>> +                      * before writing to the registers.
>>>> +                      */
>>>> +                     ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS,
>>>> +                                                   chn->clks);
>>>> +                     if (ret) {
>>>> +                             dev_err(spc->dev,
>>>> +                                     "failed to enable pwm%u clocks\n",
>>>> +                                     pwm->hwpwm);
>>>> +                             return ret;
>>>> +                     }
>>>> +             }
>>>> +
>>>> +             if (state->period != cstate.period ||
>>>> +                 state->duty_cycle != cstate.duty_cycle) {
>>>
>>> This is a coarse check. If state->period and cstate.period only differ
>>> by one calling sprd_pwm_config(spc, pwm, state->duty_cycle,
>>> state->period) probably results in a noop. So you're doing an expensive
>>> division to get an unreliable check. It would be better to calculate the
>>> register values from the requested state and compare the register
>>> values. The costs are more or less the same than calling .get_state and
>>> the check is reliable. And you don't need to spend another division to
>>> calculate the new register values.
>>
>> Same as above comment.
> 
> When taking the caching into account (which I wouldn't) the check is
> still incomplete. OK, you could argue avoiding the recalculation in 90%
> (to just say some number) of the cases where it is unnecessary is good.
>   
>>>
>>>> +                     ret = sprd_pwm_config(spc, pwm, state->duty_cycle,
>>>> +                                           state->period);
>>>> +                     if (ret)
>>>> +                             return ret;
>>>> +             }
>>>> +
>>>> +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
>>>> +     } else if (cstate.enabled) {
>>>> +             sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
>>>> +
>>>> +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
>>>
>>> Assuming writing SPRD_PWM_ENABLE = 0 to the hardware completes the
>>> currently running period and the write doesn't block that long: Does
>>> disabling the clocks interfere with completing the period?
>>
>> Writing SPRD_PWM_ENABLE = 0 will stop the PWM immediately, will not
>> waiting for completing the currently period.
> 
> The currently active period is supposed to be completed. If you cannot
> implement this please point this out as limitation at the top of the
> driver.
> 
> Honestly I think most pwm users won't mind and they should get the
> possibility to tell they prefer pwm_apply_state to return immediately
> even if this could result in a spike. But we're not there yet.
> (Actually there are three things a PWM consumer might want:
> 
>   a) stop immediately;
>   b) complete the currently running period, then stop and return only
>      when the period is completed; or
>   c) complete the currently running period and then stop, return immediately if
>      possible.
> 
> Currently the expected semantic is b).

Hi Uwe et all,

I am sorry for interrupting your discussion. From my (last year or so)
observation of the PWM mailing list I see this as a common pattern in
almost every submission of a new PWM driver. That is PWM driver authors
keep submitting solution a) and you tell them the expected behavior
is b).

Why is that? I hope that the fact that implementing a) is simpler
than b) is not the main reason. I believe that people think a)
is the correct solution.

I agree that smooth transition from one period/duty setting to
different setting makes sense. But I also believe the expected
behavior of setting enabled=0 is different. That is to stop
immediately the technology that is fed by the PWM signal.
Otherwise the concept of enabled/disabled does not make sense
because setting duty=0 would have the same effect.

So I still wonder where the expectation for b) is coming from.
What would be the physical/electrical reasoning for b)?
Why/how it can be dangerous/harmful for any device to stop the
PWM signal immediately?

Would be great to finally reach consensus on that so the expected
behavior can be documented. I know you already attempted that [1].
I think you have done a great job but I still consider the part
about state changes little controversial and unresolved.

Best regards,
Michal

[1] http://patchwork.ozlabs.org/patch/1021566/

Are the requirements of the PWM API sensible?

[Uwe Kleine-König]

Hello Michal,

[adapted the Subject to the actual topic of this mail]

On Wed, Aug 14, 2019 at 04:07:21PM +0200, Michal Vokáč wrote:
> > Honestly I think most pwm users won't mind and they should get the
> > possibility to tell they prefer pwm_apply_state to return immediately
> > even if this could result in a spike. But we're not there yet.
> > (Actually there are three things a PWM consumer might want:
> > 
> >   a) stop immediately;
> >   b) complete the currently running period, then stop and return
> >   only
> >      when the period is completed; or
> >   c) complete the currently running period and then stop, return
> >   immediately if
> >      possible.
> > 
> > Currently the expected semantic is b).
> 
> I am sorry for interrupting your discussion. From my (last year or so)
> observation of the PWM mailing list I see this as a common pattern in
> almost every submission of a new PWM driver. That is PWM driver authors
> keep submitting solution a) and you tell them the expected behavior
> is b).

The history is that there was a discussion about something related to
the pwm-imx27 or the pwm-mxs driver between Thierry and me. I don't
remember the exact issue though. My impression back then was that
Thierry had a conception of how a PWM should work that was idealised and
hard to implement with a big part of the actually available hardware
implementations. In the end Thierry ended the discussion saying
something like: "I'm not aware that the available drivers don't fulfill
the requirements so I must assume they are right. If you know otherwise,
please tell us. I'm not willing to change the API's requirements just
because of one or two drivers having problems." (I didn't look up the
mail thread, so this is not an exact quote.)

One of these requirements was b) from above. So if you notice a common
pattern here on the list that most new drivers have problems to
implement b) I'm happy. I let you decide what this probably means for
the existing drivers.

> Why is that? I hope that the fact that implementing a) is simpler
> than b) is not the main reason. I believe that people think a)
> is the correct solution.

Honestly I think people are not aware of the requirements and/or don't
care enough about these details instead of actively being convinced that
a) is right. Their test cases are most of the time quite limited and if
the LED's brightness reacts according to the expectations the driver is
considered done.

And I agree with Thierry that in some situations it is sane to require
that periods are completed to prevent spikes. (I'm not a hardware guy,
but I think controlling a step motor is such a situation where you'd want
to prevent spikes.)

> I agree that smooth transition from one period/duty setting to
> different setting makes sense. But I also believe the expected
> behavior of setting enabled=0 is different. That is to stop
> immediately the technology that is fed by the PWM signal.
> Otherwise the concept of enabled/disabled does not make sense
> because setting duty=0 would have the same effect.

That's approximately my argumentation from back then. I asked several
times what was the difference between duty=0 and enabled=0. Thierry was
unable to come up with a better explanation but insisted there was a
difference. (Later I became aware that there is a difference, i.e. with
duty=0 the inactive time still has to be a multiple of the configured
period. So disabled is actually equivalent to duty=0 with an
infinitesimal period. Unless I missed something of course.)

I even questioned if consumers really care about a PWM going to it's
inactive level on disable and suggested to introduce something to the
PWM API to let consumers express they don't care. But the discussion
led nowhere. In my eyes the PWM framework would become better if
consumers were able to specify their intentions in a more detailed way.
Thierry didn't agree.

I think allowing lowlevel drivers to act immediately on enabled=0 would
be fine. Consumers who'd want to disable only after completion of the
currently running period could then do:

	pwm_apply(pwm, duty=0, period=dontcare, enabled=1);
	pwm_apply(pwm, duty=0, period=dontcare, enabled=0);

to achieve this completion. So after all consumers are converted the API
is more powerful than before because consumers have the choice between
the above and a single

	pwm_apply(pwm, duty=0, period=dontcare, enabled=0);

The net benefit is that consumers don't have to wait if they don't
actually care about completing the period. The downside is that all
consumers must be reviewed and maybe fixed and maybe that people's
expectations are not met anymore. (But I doubt the latter a bit because
I'm not sure that most people actually expect that pwm_apply() blocks
until the output reached its disabled state.)

And we could even go further and tell consumers who care about the
output's level should not disable the pwm but instead do

	pwm_apply(pwm, duty=0, period=1, enabled=1);

which has effectively the same semantic as enabled=0 today. This would
give at least two (probably more) PWM drivers the freedom to really
disable the hardware which might result in something different than
"inactive level" without taking the possibility from consumers to
actually request what they want.

> So I still wonder where the expectation for b) is coming from.
> What would be the physical/electrical reasoning for b)?
> Why/how it can be dangerous/harmful for any device to stop the
> PWM signal immediately?

Maybe you can get Thierry to answer this. He is the source of this
requirement.

> Would be great to finally reach consensus on that so the expected
> behavior can be documented. I know you already attempted that [1].
> I think you have done a great job but I still consider the part
> about state changes little controversial and unresolved.

I'm completely open to document the expectations. I'm also open to shift
the expectations to something that is saner and/or gives consumers more
possibilities. But it's Thierry's responsibility to go forward here or
at least stop the blockade.

Best regards
Uwe

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