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

[PATCH 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>
---
 Documentation/devicetree/bindings/pwm/pwm-sprd.txt |   31 ++++++++++++++++++++
 1 file changed, 31 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..e8e0d5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-sprd.txt
@@ -0,0 +1,31 @@
+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:
+  "source": for PWM source (parent) clock.
+  "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).
+- #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 = "source",
+			"pwm0", "enable0",
+			"pwm1", "enable1",
+			"pwm2", "enable2",
+			"pwm3", "enable3";
+		clocks = <&ext_26m>,
+		       <&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>;
+		#pwm-cells = <2>;
+	};
-- 
1.7.9.5

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

[Baolin Wang]

From: Neo Hou <neo.hou@unisoc.com>

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

Signed-off-by: Neo Hou <neo.hou@unisoc.com>
Co-developed-by: Baolin Wang <baolin.wang@linaro.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/pwm/Kconfig    |   10 ++
 drivers/pwm/Makefile   |    1 +
 drivers/pwm/pwm-sprd.c |  311 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 322 insertions(+)
 create mode 100644 drivers/pwm/pwm-sprd.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index a7e5751..4963b4d 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -423,6 +423,16 @@ 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
+	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..f6fc793
--- /dev/null
+++ b/drivers/pwm/pwm-sprd.c
@@ -0,0 +1,311 @@
+// 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_DIV		0xc
+#define SPRD_PWM_PAT_LOW	0x10
+#define SPRD_PWM_PAT_HIGH	0x14
+#define SPRD_PWM_ENABLE		0x18
+
+#define SPRD_PWM_MOD_MAX	GENMASK(7, 0)
+#define SPRD_PWM_REG_MSK	GENMASK(15, 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_DEFAULT_CLK	26000000UL
+
+struct sprd_pwm_chn {
+	struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
+	unsigned long clk_rate;
+	bool clk_enabled;
+};
+
+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];
+};
+
+/* list of clocks required by PWM channels */
+static const char * const sprd_pwm_clks[] = {
+	"enable0", "pwm0",
+	"enable1", "pwm1",
+	"enable2", "pwm2",
+	"enable3", "pwm3",
+};
+
+static u32 sprd_pwm_read(struct sprd_pwm_chip *chip, u32 num, u32 reg)
+{
+	u32 offset = reg + (num << SPRD_PWM_REGS_SHIFT);
+
+	return readl_relaxed(chip->base + offset);
+}
+
+static void sprd_pwm_write(struct sprd_pwm_chip *chip, u32 num,
+			   u32 reg, u32 val)
+{
+	u32 offset = reg + (num << SPRD_PWM_REGS_SHIFT);
+
+	writel_relaxed(val, chip->base + offset);
+}
+
+static int sprd_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			   int duty_ns, int period_ns)
+{
+	struct sprd_pwm_chip *spc =
+		container_of(chip, struct sprd_pwm_chip, chip);
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	u64 div, tmp;
+	u32 prescale, duty;
+	int ret;
+
+	/*
+	 * NOTE: the clocks to PWM channel has to be enabled first before
+	 * writing to the registers.
+	 */
+	if (!chn->clk_enabled) {
+		ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
+		if (ret) {
+			dev_err(spc->dev, "failed to enable pwm%u clock\n",
+				pwm->hwpwm);
+			return ret;
+		}
+
+		chn->clk_enabled = true;
+	}
+
+	duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
+
+	/*
+	 * According to the datasheet, the period_ns calculation formula
+	 * should be:
+	 * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
+	 *
+	 * Then we can get the prescale formula:
+	 * prescale = (period_ns * clk_rate) / (10^9 * mod) -1
+	 */
+	tmp = chn->clk_rate * period_ns;
+	div = 1000000000ULL * SPRD_PWM_MOD_MAX;
+	prescale = div64_u64(tmp, div) - 1;
+
+	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_PAT_LOW, SPRD_PWM_REG_MSK);
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_HIGH, SPRD_PWM_REG_MSK);
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
+
+	return 0;
+}
+
+static int sprd_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct sprd_pwm_chip *spc =
+		container_of(chip, struct sprd_pwm_chip, chip);
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+	int ret;
+
+	if (!chn->clk_enabled) {
+		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;
+		}
+
+		chn->clk_enabled = true;
+	}
+
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 1);
+
+	return 0;
+}
+
+static void sprd_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct sprd_pwm_chip *spc =
+		container_of(chip, struct sprd_pwm_chip, chip);
+	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
+
+	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_ENABLE, 0);
+
+	if (chn->clk_enabled) {
+		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
+		chn->clk_enabled = false;
+	}
+}
+
+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 enabled, duty, prescale;
+	u64 tmp;
+	int ret;
+
+	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;
+	}
+
+	chn->clk_enabled = true;
+
+	duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
+	prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
+	enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
+
+	/*
+	 * According to the datasheet, the period_ns and duty_ns calculation
+	 * formula should be:
+	 * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
+	 * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
+	 */
+	tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
+	state->period = div64_u64(tmp, chn->clk_rate);
+
+	tmp = (prescale + 1) * 1000000000ULL * duty;
+	state->duty_cycle = div64_u64(tmp, chn->clk_rate);
+
+	state->enabled = !!enabled;
+
+	/* Disable PWM clocks if the PWM channel is not in enable state. */
+	if (!enabled) {
+		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
+		chn->clk_enabled = false;
+	}
+}
+
+static const struct pwm_ops sprd_pwm_ops = {
+	.config = sprd_pwm_config,
+	.enable = sprd_pwm_enable,
+	.disable = sprd_pwm_disable,
+	.get_state = sprd_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
+{
+	struct clk *clk_parent, *clk_pwm;
+	int ret, i, clk_index = 0;
+
+	clk_parent = devm_clk_get(spc->dev, "source");
+	if (IS_ERR(clk_parent)) {
+		dev_err(spc->dev, "failed to get source clock\n");
+		return PTR_ERR(clk_parent);
+	}
+
+	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[1].clk;
+		if (!clk_set_parent(clk_pwm, clk_parent))
+			chn->clk_rate = clk_get_rate(clk_pwm);
+		else
+			chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
+	}
+
+	if (!i) {
+		dev_err(spc->dev, "no availbale 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;
+	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;
+	}
+
+	platform_set_drvdata(pdev, spc);
+	return 0;
+}
+
+static int sprd_pwm_remove(struct platform_device *pdev)
+{
+	struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < spc->num_pwms; i++)
+		pwm_disable(&spc->chip.pwms[i]);
+
+	return pwmchip_remove(&spc->chip);
+}
+
+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 2/2] pwm: sprd: Add Spreadtrum PWM support

[Uwe Kleine-König]

On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> From: Neo Hou <neo.hou@unisoc.com>
> 
> This patch adds the Spreadtrum PWM support, which provides maximum 4
> channels.
> 
> Signed-off-by: Neo Hou <neo.hou@unisoc.com>
> Co-developed-by: Baolin Wang <baolin.wang@linaro.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/pwm/Kconfig    |   10 ++
>  drivers/pwm/Makefile   |    1 +
>  drivers/pwm/pwm-sprd.c |  311 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 322 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sprd.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a7e5751..4963b4d 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -423,6 +423,16 @@ 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

I think you need

	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..f6fc793
> --- /dev/null
> +++ b/drivers/pwm/pwm-sprd.c
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 Spreadtrum Communications Inc.

If there is a publicly available reference manual available, please add
a link to it here.

> + */
> +
> +#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_DIV		0xc
> +#define SPRD_PWM_PAT_LOW	0x10
> +#define SPRD_PWM_PAT_HIGH	0x14
> +#define SPRD_PWM_ENABLE		0x18
> +
> +#define SPRD_PWM_MOD_MAX	GENMASK(7, 0)
> +#define SPRD_PWM_REG_MSK	GENMASK(15, 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_DEFAULT_CLK	26000000UL
> +
> +struct sprd_pwm_chn {
> +	struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
> +	unsigned long clk_rate;
> +	bool clk_enabled;
> +};
> +
> +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];
> +};
> +
> +/* list of clocks required by PWM channels */
> +static const char * const sprd_pwm_clks[] = {
> +	"enable0", "pwm0",
> +	"enable1", "pwm1",
> +	"enable2", "pwm2",
> +	"enable3", "pwm3",
> +};
> +
> +static u32 sprd_pwm_read(struct sprd_pwm_chip *chip, u32 num, u32 reg)

num is the channel id here? Then maybe "hwid" or "chanid" would be a
better name. Or pass struct pwm_chip only?

Please (if you keep sprd_pwm_chip) rename chip to spc which is the name
used in other places for structures of this type. "chip" is for struct
pwm_chip.

> +{
> +	u32 offset = reg + (num << SPRD_PWM_REGS_SHIFT);
> +
> +	return readl_relaxed(chip->base + offset);
> +}
> +
> +static void sprd_pwm_write(struct sprd_pwm_chip *chip, u32 num,
> +			   u32 reg, u32 val)
> +{
> +	u32 offset = reg + (num << SPRD_PWM_REGS_SHIFT);
> +
> +	writel_relaxed(val, chip->base + offset);
> +}
> +
> +static int sprd_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   int duty_ns, int period_ns)

Please implement .apply() instead of .config(), .enable() and
.disable().

> +{
> +	struct sprd_pwm_chip *spc =
> +		container_of(chip, struct sprd_pwm_chip, chip);
> +	struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> +	u64 div, tmp;
> +	u32 prescale, duty;
> +	int ret;
> +
> +	/*
> +	 * NOTE: the clocks to PWM channel has to be enabled first before
> +	 * writing to the registers.
> +	 */
> +	if (!chn->clk_enabled) {
> +		ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);

Do you really need to enable all 8 clocks to configure a single channel?

> +		if (ret) {
> +			dev_err(spc->dev, "failed to enable pwm%u clock\n",
> +				pwm->hwpwm);
> +			return ret;
> +		}
> +
> +		chn->clk_enabled = true;
> +	}
> +
> +	duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;

@Baolin: until we're there that there are framework requirements how to
round, please document at least how you do it here. Also describing the
underlying concepts would be good for the driver reader.

Something like:

/*
 * 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 = 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 = MOD_MAX and input clock).
 */

@Thierry: having a framework guideline here would be good. Or still
better a guideline and a debug setting that notices drivers stepping out
of line.

I assume using MOD = 0 is forbidden?

> +	/*
> +	 * According to the datasheet, the period_ns calculation formula
> +	 * should be:
> +	 * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> +	 *
> +	 * Then we can get the prescale formula:
> +	 * prescale = (period_ns * clk_rate) / (10^9 * mod) -1
> +	 */
> +	tmp = chn->clk_rate * period_ns;
> +	div = 1000000000ULL * SPRD_PWM_MOD_MAX;

Please use NSEC_PER_SEC instead of 1000000000ULL.

> +	prescale = div64_u64(tmp, div) - 1;

If tmp is smaller than div you end up with prescale = 0xffffffff which
should be catched. What if prescale > 0xffff?

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

You're losing precision here as you always use SPRD_PWM_MOD_MAX, right?
(Just for my understanding, if this simpler approach is good enough
here that's fine.)

> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_LOW, SPRD_PWM_REG_MSK);
> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_HIGH, SPRD_PWM_REG_MSK);

Please describe these two registers in a short comment.

> +	sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);

Is the configuration here atomic in the sense that the write of DUTY
above only gets effective when PRESCALE is written? If not, please add
a describing paragraph at the top of the driver similar to what is
written in pwm-sifive.c. When the PWM is already running before, how
does a configuration change effects the output? Is the currently running
period completed?

> +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 enabled, duty, prescale;
> +	u64 tmp;
> +	int ret;
> +
> +	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;
> +	}
> +
> +	chn->clk_enabled = true;
> +
> +	duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> +	prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> +	enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> +
> +	/*
> +	 * According to the datasheet, the period_ns and duty_ns calculation
> +	 * formula should be:
> +	 * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> +	 * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> +	 */
> +	tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> +	state->period = div64_u64(tmp, chn->clk_rate);

This is not idempotent. If you apply the configuration that is returned
here this shouldn't result in a reconfiguration.

> +	tmp = (prescale + 1) * 1000000000ULL * duty;
> +	state->duty_cycle = div64_u64(tmp, chn->clk_rate);
> +
> +	state->enabled = !!enabled;
> +
> +	/* Disable PWM clocks if the PWM channel is not in enable state. */
> +	if (!enabled) {
> +		clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> +		chn->clk_enabled = false;
> +	}
> +}
> +
> +static const struct pwm_ops sprd_pwm_ops = {
> +	.config = sprd_pwm_config,
> +	.enable = sprd_pwm_enable,
> +	.disable = sprd_pwm_disable,
> +	.get_state = sprd_pwm_get_state,
> +	.owner = THIS_MODULE,
> +};
> +
> +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> +{
> +	struct clk *clk_parent, *clk_pwm;
> +	int ret, i, clk_index = 0;
> +
> +	clk_parent = devm_clk_get(spc->dev, "source");
> +	if (IS_ERR(clk_parent)) {
> +		dev_err(spc->dev, "failed to get source clock\n");
> +		return PTR_ERR(clk_parent);
> +	}
> +
> +	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;

devm_clk_bulk_get_optional ? I'm sure you don't want to get all 8 clocks
8 times.

> +
> +			dev_err(spc->dev, "failed to get channel clocks\n");
> +			return ret;
> +		}
> +
> +		clk_pwm = chn->clks[1].clk;

This 1 looks suspicious. Are you using all clocks provided in the dtb at
all? You're not using i in the loop at all, this doesn't look right.

> +		if (!clk_set_parent(clk_pwm, clk_parent))
> +			chn->clk_rate = clk_get_rate(clk_pwm);
> +		else
> +			chn->clk_rate = SPRD_PWM_DEFAULT_CLK;

I don't know all the clock framework details, but I think there are
better ways to ensure that a given clock is used as parent for another
given clock. Please read the chapter about "Assigned clock parents and
rates" in the clock bindings and check if this could be used for the
purpose here and so simplify the driver.

> +	}
> +
> +	if (!i) {
> +		dev_err(spc->dev, "no availbale PWM channels\n");

s/availbale/available/

> +		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;
> +	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;
> +	}
> +
> +	platform_set_drvdata(pdev, spc);

If you do this earlier you can simplify the last part of the driver to:

	ret = pwmchip_add(&spc->chip);
	if (ret)
		dev_err(&pdev->dev, "failed to add PWM chip\n");

	return ret;

> +	return 0;
> +}
> +
> +static int sprd_pwm_remove(struct platform_device *pdev)
> +{
> +	struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < spc->num_pwms; i++)
> +		pwm_disable(&spc->chip.pwms[i]);

This is wrong. You must not call pwm_disable here. It's the consumer's
responsibility to disable the hardware.

> +
> +	return pwmchip_remove(&spc->chip);
> +}

Best regards
Uwe

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

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

[Baolin Wang]

 Hi Uwe,

On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > From: Neo Hou <neo.hou@unisoc.com>
> >
> > This patch adds the Spreadtrum PWM support, which provides maximum 4
> > channels.
> >
> > Signed-off-by: Neo Hou <neo.hou@unisoc.com>
> > Co-developed-by: Baolin Wang <baolin.wang@linaro.org>
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > ---
> >  drivers/pwm/Kconfig    |   10 ++
> >  drivers/pwm/Makefile   |    1 +
> >  drivers/pwm/pwm-sprd.c |  311 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 322 insertions(+)
> >  create mode 100644 drivers/pwm/pwm-sprd.c
> >
> > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > index a7e5751..4963b4d 100644
> > --- a/drivers/pwm/Kconfig
> > +++ b/drivers/pwm/Kconfig
> > @@ -423,6 +423,16 @@ 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
>
> I think you need
>
>         depends on HAS_IOMEM

OK.

>
> > +     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..f6fc793
> > --- /dev/null
> > +++ b/drivers/pwm/pwm-sprd.c
> > @@ -0,0 +1,311 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2019 Spreadtrum Communications Inc.
>
> If there is a publicly available reference manual available, please add
> a link to it here.

Sure.

>
> > + */
> > +
> > +#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_DIV         0xc
> > +#define SPRD_PWM_PAT_LOW     0x10
> > +#define SPRD_PWM_PAT_HIGH    0x14
> > +#define SPRD_PWM_ENABLE              0x18
> > +
> > +#define SPRD_PWM_MOD_MAX     GENMASK(7, 0)
> > +#define SPRD_PWM_REG_MSK     GENMASK(15, 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_DEFAULT_CLK 26000000UL
> > +
> > +struct sprd_pwm_chn {
> > +     struct clk_bulk_data clks[SPRD_PWM_NUM_CLKS];
> > +     unsigned long clk_rate;
> > +     bool clk_enabled;
> > +};
> > +
> > +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];
> > +};
> > +
> > +/* list of clocks required by PWM channels */
> > +static const char * const sprd_pwm_clks[] = {
> > +     "enable0", "pwm0",
> > +     "enable1", "pwm1",
> > +     "enable2", "pwm2",
> > +     "enable3", "pwm3",
> > +};
> > +
> > +static u32 sprd_pwm_read(struct sprd_pwm_chip *chip, u32 num, u32 reg)
>
> num is the channel id here? Then maybe "hwid" or "chanid" would be a
> better name. Or pass struct pwm_chip only?

Yes, will change to 'hwid'.

>
> Please (if you keep sprd_pwm_chip) rename chip to spc which is the name
> used in other places for structures of this type. "chip" is for struct
> pwm_chip.

Yes, sure.

>
> > +{
> > +     u32 offset = reg + (num << SPRD_PWM_REGS_SHIFT);
> > +
> > +     return readl_relaxed(chip->base + offset);
> > +}
> > +
> > +static void sprd_pwm_write(struct sprd_pwm_chip *chip, u32 num,
> > +                        u32 reg, u32 val)
> > +{
> > +     u32 offset = reg + (num << SPRD_PWM_REGS_SHIFT);
> > +
> > +     writel_relaxed(val, chip->base + offset);
> > +}
> > +
> > +static int sprd_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                        int duty_ns, int period_ns)
>
> Please implement .apply() instead of .config(), .enable() and
> .disable().

OK.

>
> > +{
> > +     struct sprd_pwm_chip *spc =
> > +             container_of(chip, struct sprd_pwm_chip, chip);
> > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > +     u64 div, tmp;
> > +     u32 prescale, duty;
> > +     int ret;
> > +
> > +     /*
> > +      * NOTE: the clocks to PWM channel has to be enabled first before
> > +      * writing to the registers.
> > +      */
> > +     if (!chn->clk_enabled) {
> > +             ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
>
> Do you really need to enable all 8 clocks to configure a single channel?

We have 4 channels, and each channel use 2 clocks, so here only enable
2 clocks, see SPRD_PWM_NUM_CLKS.

>
> > +             if (ret) {
> > +                     dev_err(spc->dev, "failed to enable pwm%u clock\n",
> > +                             pwm->hwpwm);
> > +                     return ret;
> > +             }
> > +
> > +             chn->clk_enabled = true;
> > +     }
> > +
> > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
>
> @Baolin: until we're there that there are framework requirements how to
> round, please document at least how you do it here. Also describing the
> underlying concepts would be good for the driver reader.
>
> Something like:
>
> /*
>  * 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 = 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 = MOD_MAX and input clock).
>  */

Yes, totally agree with you. I will add some documentation for our
controller's setting.

>
> @Thierry: having a framework guideline here would be good. Or still
> better a guideline and a debug setting that notices drivers stepping out
> of line.
>
> I assume using MOD = 0 is forbidden?
>
> > +     /*
> > +      * According to the datasheet, the period_ns calculation formula
> > +      * should be:
> > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > +      *
> > +      * Then we can get the prescale formula:
> > +      * prescale = (period_ns * clk_rate) / (10^9 * mod) -1
> > +      */
> > +     tmp = chn->clk_rate * period_ns;
> > +     div = 1000000000ULL * SPRD_PWM_MOD_MAX;
>
> Please use NSEC_PER_SEC instead of 1000000000ULL.

Sure.

>
> > +     prescale = div64_u64(tmp, div) - 1;
>
> If tmp is smaller than div you end up with prescale = 0xffffffff which
> should be catched. What if prescale > 0xffff?

Usually we can not meet this case, but you are right, the prescale has
a limit according to the register definition. So will add a validation
here.

>
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
>
> You're losing precision here as you always use SPRD_PWM_MOD_MAX, right?
> (Just for my understanding, if this simpler approach is good enough
> here that's fine.)

Yes, SPRD_PWM_MOD_MAX is good enough.

>
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_LOW, SPRD_PWM_REG_MSK);
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_HIGH, SPRD_PWM_REG_MSK);
>
> Please describe these two registers in a short comment.

Sure.

>
> > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
>
> Is the configuration here atomic in the sense that the write of DUTY
> above only gets effective when PRESCALE is written? If not, please add

Yes.

> a describing paragraph at the top of the driver similar to what is
> written in pwm-sifive.c. When the PWM is already running before, how
> does a configuration change effects the output? Is the currently running
> period completed?

Anyway, I'll add some comments to explain how it works.

>
> > +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 enabled, duty, prescale;
> > +     u64 tmp;
> > +     int ret;
> > +
> > +     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;
> > +     }
> > +
> > +     chn->clk_enabled = true;
> > +
> > +     duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > +     prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > +     enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > +
> > +     /*
> > +      * According to the datasheet, the period_ns and duty_ns calculation
> > +      * formula should be:
> > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > +      * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > +      */
> > +     tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > +     state->period = div64_u64(tmp, chn->clk_rate);
>
> This is not idempotent. If you apply the configuration that is returned
> here this shouldn't result in a reconfiguration.

Since we may configure the  PWM in bootloader, so in kernel part we
should get current PWM state to avoid reconfiguration if state
configuration are same.

>
> > +     tmp = (prescale + 1) * 1000000000ULL * duty;
> > +     state->duty_cycle = div64_u64(tmp, chn->clk_rate);
> > +
> > +     state->enabled = !!enabled;
> > +
> > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > +     if (!enabled) {
> > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > +             chn->clk_enabled = false;
> > +     }
> > +}
> > +
> > +static const struct pwm_ops sprd_pwm_ops = {
> > +     .config = sprd_pwm_config,
> > +     .enable = sprd_pwm_enable,
> > +     .disable = sprd_pwm_disable,
> > +     .get_state = sprd_pwm_get_state,
> > +     .owner = THIS_MODULE,
> > +};
> > +
> > +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> > +{
> > +     struct clk *clk_parent, *clk_pwm;
> > +     int ret, i, clk_index = 0;
> > +
> > +     clk_parent = devm_clk_get(spc->dev, "source");
> > +     if (IS_ERR(clk_parent)) {
> > +             dev_err(spc->dev, "failed to get source clock\n");
> > +             return PTR_ERR(clk_parent);
> > +     }
> > +
> > +     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;
>
> devm_clk_bulk_get_optional ? I'm sure you don't want to get all 8 clocks
> 8 times.

This is not optional, each channel has 2 required clocks, and we have
4 channels. (SPRD_PWM_NUM_CLKS == 2)

>
> > +
> > +                     dev_err(spc->dev, "failed to get channel clocks\n");
> > +                     return ret;
> > +             }
> > +
> > +             clk_pwm = chn->clks[1].clk;
>
> This 1 looks suspicious. Are you using all clocks provided in the dtb at
> all? You're not using i in the loop at all, this doesn't look right.

Like I said above, each channel has 2 clocks: enable clock and pwm
clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
which is used to set the source clock. I know this's not easy to read,
so do you have any good suggestion?

>
> > +             if (!clk_set_parent(clk_pwm, clk_parent))
> > +                     chn->clk_rate = clk_get_rate(clk_pwm);
> > +             else
> > +                     chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
>
> I don't know all the clock framework details, but I think there are
> better ways to ensure that a given clock is used as parent for another
> given clock. Please read the chapter about "Assigned clock parents and
> rates" in the clock bindings and check if this could be used for the
> purpose here and so simplify the driver.

Actually there are many other drivers set the parent clock like this,
and we want a default clock if failed to set the parent clock.

>
> > +     }
> > +
> > +     if (!i) {
> > +             dev_err(spc->dev, "no availbale PWM channels\n");
>
> s/availbale/available/

Sure.

>
> > +             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;
> > +     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;
> > +     }
> > +
> > +     platform_set_drvdata(pdev, spc);
>
> If you do this earlier you can simplify the last part of the driver to:
>
>         ret = pwmchip_add(&spc->chip);
>         if (ret)
>                 dev_err(&pdev->dev, "failed to add PWM chip\n");
>
>         return ret;

OK.

>
> > +     return 0;
> > +}
> > +
> > +static int sprd_pwm_remove(struct platform_device *pdev)
> > +{
> > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > +     int i;
> > +
> > +     for (i = 0; i < spc->num_pwms; i++)
> > +             pwm_disable(&spc->chip.pwms[i]);
>
> This is wrong. You must not call pwm_disable here. It's the consumer's
> responsibility to disable the hardware.

Emm, I saw some drivers did like this, like pwm-spear.c.
Could you elaborate on what's the problem if the driver issues pwm_disable?

Very appreciated for your comments.

-- 
Baolin Wang
Best Regards

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

[Uwe Kleine-König]

Hello Baolin,

On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > +{
> > > +     struct sprd_pwm_chip *spc =
> > > +             container_of(chip, struct sprd_pwm_chip, chip);
> > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > +     u64 div, tmp;
> > > +     u32 prescale, duty;
> > > +     int ret;
> > > +
> > > +     /*
> > > +      * NOTE: the clocks to PWM channel has to be enabled first before
> > > +      * writing to the registers.
> > > +      */
> > > +     if (!chn->clk_enabled) {
> > > +             ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> >
> > Do you really need to enable all 8 clocks to configure a single channel?
> 
> We have 4 channels, and each channel use 2 clocks, so here only enable
> 2 clocks, see SPRD_PWM_NUM_CLKS.

Ah, I thought SPRD_PWM_NUM_CLKS was 8.

> > > +             if (ret) {
> > > +                     dev_err(spc->dev, "failed to enable pwm%u clock\n",
> > > +                             pwm->hwpwm);
> > > +                     return ret;
> > > +             }
> > > +
> > > +             chn->clk_enabled = true;
> > > +     }
> > > +
> > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> >
> > @Baolin: until we're there that there are framework requirements how to
> > round, please document at least how you do it here. Also describing the
> > underlying concepts would be good for the driver reader.
> >
> > Something like:
> >
> > /*
> >  * 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 = 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 = MOD_MAX and input clock).
> >  */
> 
> Yes, totally agree with you. I will add some documentation for our
> controller's setting.
> 
> >
> > @Thierry: having a framework guideline here would be good. Or still
> > better a guideline and a debug setting that notices drivers stepping out
> > of line.
> >
> > I assume using MOD = 0 is forbidden?
> >
> > > +     /*
> > > +      * According to the datasheet, the period_ns calculation formula
> > > +      * should be:
> > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > +      *
> > > +      * Then we can get the prescale formula:
> > > +      * prescale = (period_ns * clk_rate) / (10^9 * mod) -1
> > > +      */
> > > +     tmp = chn->clk_rate * period_ns;
> > > +     div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> >
> > Please use NSEC_PER_SEC instead of 1000000000ULL.
> 
> Sure.
> 
> >
> > > +     prescale = div64_u64(tmp, div) - 1;
> >
> > If tmp is smaller than div you end up with prescale = 0xffffffff which
> > should be catched. What if prescale > 0xffff?
> 
> Usually we can not meet this case, but you are right, the prescale has
> a limit according to the register definition. So will add a validation
> here.
> 
> >
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> >
> > You're losing precision here as you always use SPRD_PWM_MOD_MAX, right?
> > (Just for my understanding, if this simpler approach is good enough
> > here that's fine.)
> 
> Yes, SPRD_PWM_MOD_MAX is good enough.

ok.
 
> >
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_LOW, SPRD_PWM_REG_MSK);
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_HIGH, SPRD_PWM_REG_MSK);
> >
> > Please describe these two registers in a short comment.
> 
> Sure.
> 
> >
> > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> >
> > Is the configuration here atomic in the sense that the write of DUTY
> > above only gets effective when PRESCALE is written? If not, please add
> 
> Yes.
> 
> > a describing paragraph at the top of the driver similar to what is
> > written in pwm-sifive.c. When the PWM is already running before, how
> > does a configuration change effects the output? Is the currently running
> > period completed?
> 
> Anyway, I'll add some comments to explain how it works.

I'd apreciate if you'd stick to the format in pwm-sifive.c to make it
easier to grep for that.
 
> > > +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 enabled, duty, prescale;
> > > +     u64 tmp;
> > > +     int ret;
> > > +
> > > +     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;
> > > +     }
> > > +
> > > +     chn->clk_enabled = true;
> > > +
> > > +     duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > +     prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > +     enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > +
> > > +     /*
> > > +      * According to the datasheet, the period_ns and duty_ns calculation
> > > +      * formula should be:
> > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > +      * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > +      */
> > > +     tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > +     state->period = div64_u64(tmp, chn->clk_rate);
> >
> > This is not idempotent. If you apply the configuration that is returned
> > here this shouldn't result in a reconfiguration.
> 
> Since we may configure the  PWM in bootloader, so in kernel part we
> should get current PWM state to avoid reconfiguration if state
> configuration are same.

This is also important as some consumers might do something like:

	state = pwm_get_state(mypwm)
	if (something):
		state->duty = 0
	else:
		state->duty = state->period / 2
	pwm_set_state(mypwm, state)

and then period shouldn't get smaller in each step.
(This won't happen as of now because the PWM framework caches the last
state that was set and returns this for pwm_get_state. Still getting
this right would be good.)

> > > +     tmp = (prescale + 1) * 1000000000ULL * duty;
> > > +     state->duty_cycle = div64_u64(tmp, chn->clk_rate);
> > > +
> > > +     state->enabled = !!enabled;
> > > +
> > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > +     if (!enabled) {
> > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > +             chn->clk_enabled = false;
> > > +     }
> > > +}
> > > +
> > > +static const struct pwm_ops sprd_pwm_ops = {
> > > +     .config = sprd_pwm_config,
> > > +     .enable = sprd_pwm_enable,
> > > +     .disable = sprd_pwm_disable,
> > > +     .get_state = sprd_pwm_get_state,
> > > +     .owner = THIS_MODULE,
> > > +};
> > > +
> > > +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> > > +{
> > > +     struct clk *clk_parent, *clk_pwm;
> > > +     int ret, i, clk_index = 0;
> > > +
> > > +     clk_parent = devm_clk_get(spc->dev, "source");
> > > +     if (IS_ERR(clk_parent)) {
> > > +             dev_err(spc->dev, "failed to get source clock\n");
> > > +             return PTR_ERR(clk_parent);
> > > +     }
> > > +
> > > +     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;
> >
> > devm_clk_bulk_get_optional ? I'm sure you don't want to get all 8 clocks
> > 8 times.
> 
> This is not optional, each channel has 2 required clocks, and we have
> 4 channels. (SPRD_PWM_NUM_CLKS == 2)

I see. Currently it is not possible to use channel 2 if there are no
clocks for channel 0, right? There is no hardware related problem here,
just a shortcoming of the driver?

> > > +
> > > +                     dev_err(spc->dev, "failed to get channel clocks\n");
> > > +                     return ret;
> > > +             }
> > > +
> > > +             clk_pwm = chn->clks[1].clk;
> >
> > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > all? You're not using i in the loop at all, this doesn't look right.
> 
> Like I said above, each channel has 2 clocks: enable clock and pwm
> clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> which is used to set the source clock. I know this's not easy to read,
> so do you have any good suggestion?

Not sure this is easily possible to rework to make this clearer.
 
Do these clks have different uses? e.g. one to enable register access
and the other to enable the pwm output? If so just using
devm_clk_bulk_get isn't the right thing because you should be able know
if clks[0] or clks[1] is the one you need to enable the output (or
register access).

> > > +             if (!clk_set_parent(clk_pwm, clk_parent))
> > > +                     chn->clk_rate = clk_get_rate(clk_pwm);
> > > +             else
> > > +                     chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> >
> > I don't know all the clock framework details, but I think there are
> > better ways to ensure that a given clock is used as parent for another
> > given clock. Please read the chapter about "Assigned clock parents and
> > rates" in the clock bindings and check if this could be used for the
> > purpose here and so simplify the driver.
> 
> Actually there are many other drivers set the parent clock like this,
> and we want a default clock if failed to set the parent clock.

These might be older than the clk framework capabilities, or the
reviewers didn't pay attention to this detail; both shouldn't be a
reason to not make it better here.

> > > +     return 0;
> > > +}
> > > +
> > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > +{
> > > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > +     int i;
> > > +
> > > +     for (i = 0; i < spc->num_pwms; i++)
> > > +             pwm_disable(&spc->chip.pwms[i]);
> >
> > This is wrong. You must not call pwm_disable here. It's the consumer's
> > responsibility to disable the hardware.
> 
> Emm, I saw some drivers did like this, like pwm-spear.c.
> Could you elaborate on what's the problem if the driver issues pwm_disable?

This is a function to be called from code that called pwm_get before
(i.e. pwm consumers). This just doesn't explode because up to now the
PWM framework is only a thin wrapper around the lowlevel driver
callbacks.

The reasoning is similar to what I wrote in commit
f82d15e223403b05fffb33ba792b87a8620f6fee. I'd like to have a PWM_DEBUG
setting that catches such problems but the discussion with Thierry to
even document the expectations is stuck, see
https://patchwork.ozlabs.org/patch/1021566/.

Best regards
Uwe

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

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

[Baolin Wang]

Hi Uwe,

On Fri, 9 Aug 2019 at 22:41, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> > On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > > +{
> > > > +     struct sprd_pwm_chip *spc =
> > > > +             container_of(chip, struct sprd_pwm_chip, chip);
> > > > +     struct sprd_pwm_chn *chn = &spc->chn[pwm->hwpwm];
> > > > +     u64 div, tmp;
> > > > +     u32 prescale, duty;
> > > > +     int ret;
> > > > +
> > > > +     /*
> > > > +      * NOTE: the clocks to PWM channel has to be enabled first before
> > > > +      * writing to the registers.
> > > > +      */
> > > > +     if (!chn->clk_enabled) {
> > > > +             ret = clk_bulk_prepare_enable(SPRD_PWM_NUM_CLKS, chn->clks);
> > >
> > > Do you really need to enable all 8 clocks to configure a single channel?
> >
> > We have 4 channels, and each channel use 2 clocks, so here only enable
> > 2 clocks, see SPRD_PWM_NUM_CLKS.
>
> Ah, I thought SPRD_PWM_NUM_CLKS was 8.
>
> > > > +             if (ret) {
> > > > +                     dev_err(spc->dev, "failed to enable pwm%u clock\n",
> > > > +                             pwm->hwpwm);
> > > > +                     return ret;
> > > > +             }
> > > > +
> > > > +             chn->clk_enabled = true;
> > > > +     }
> > > > +
> > > > +     duty = duty_ns * SPRD_PWM_MOD_MAX / period_ns;
> > >
> > > @Baolin: until we're there that there are framework requirements how to
> > > round, please document at least how you do it here. Also describing the
> > > underlying concepts would be good for the driver reader.
> > >
> > > Something like:
> > >
> > > /*
> > >  * 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 = 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 = MOD_MAX and input clock).
> > >  */
> >
> > Yes, totally agree with you. I will add some documentation for our
> > controller's setting.
> >
> > >
> > > @Thierry: having a framework guideline here would be good. Or still
> > > better a guideline and a debug setting that notices drivers stepping out
> > > of line.
> > >
> > > I assume using MOD = 0 is forbidden?
> > >
> > > > +     /*
> > > > +      * According to the datasheet, the period_ns calculation formula
> > > > +      * should be:
> > > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > +      *
> > > > +      * Then we can get the prescale formula:
> > > > +      * prescale = (period_ns * clk_rate) / (10^9 * mod) -1
> > > > +      */
> > > > +     tmp = chn->clk_rate * period_ns;
> > > > +     div = 1000000000ULL * SPRD_PWM_MOD_MAX;
> > >
> > > Please use NSEC_PER_SEC instead of 1000000000ULL.
> >
> > Sure.
> >
> > >
> > > > +     prescale = div64_u64(tmp, div) - 1;
> > >
> > > If tmp is smaller than div you end up with prescale = 0xffffffff which
> > > should be catched. What if prescale > 0xffff?
> >
> > Usually we can not meet this case, but you are right, the prescale has
> > a limit according to the register definition. So will add a validation
> > here.
> >
> > >
> > > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_MOD, SPRD_PWM_MOD_MAX);
> > > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_DUTY, duty);
> > >
> > > You're losing precision here as you always use SPRD_PWM_MOD_MAX, right?
> > > (Just for my understanding, if this simpler approach is good enough
> > > here that's fine.)
> >
> > Yes, SPRD_PWM_MOD_MAX is good enough.
>
> ok.
>
> > >
> > > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_LOW, SPRD_PWM_REG_MSK);
> > > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PAT_HIGH, SPRD_PWM_REG_MSK);
> > >
> > > Please describe these two registers in a short comment.
> >
> > Sure.
> >
> > >
> > > > +     sprd_pwm_write(spc, pwm->hwpwm, SPRD_PWM_PRESCALE, prescale);
> > >
> > > Is the configuration here atomic in the sense that the write of DUTY
> > > above only gets effective when PRESCALE is written? If not, please add
> >
> > Yes.
> >
> > > a describing paragraph at the top of the driver similar to what is
> > > written in pwm-sifive.c. When the PWM is already running before, how
> > > does a configuration change effects the output? Is the currently running
> > > period completed?
> >
> > Anyway, I'll add some comments to explain how it works.
>
> I'd apreciate if you'd stick to the format in pwm-sifive.c to make it
> easier to grep for that.

OK.

>
> > > > +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 enabled, duty, prescale;
> > > > +     u64 tmp;
> > > > +     int ret;
> > > > +
> > > > +     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;
> > > > +     }
> > > > +
> > > > +     chn->clk_enabled = true;
> > > > +
> > > > +     duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > > +     prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > > +     enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > > +
> > > > +     /*
> > > > +      * According to the datasheet, the period_ns and duty_ns calculation
> > > > +      * formula should be:
> > > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > +      * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > > +      */
> > > > +     tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > > +     state->period = div64_u64(tmp, chn->clk_rate);
> > >
> > > This is not idempotent. If you apply the configuration that is returned
> > > here this shouldn't result in a reconfiguration.
> >
> > Since we may configure the  PWM in bootloader, so in kernel part we
> > should get current PWM state to avoid reconfiguration if state
> > configuration are same.
>
> This is also important as some consumers might do something like:
>
>         state = pwm_get_state(mypwm)
>         if (something):
>                 state->duty = 0
>         else:
>                 state->duty = state->period / 2
>         pwm_set_state(mypwm, state)
>
> and then period shouldn't get smaller in each step.
> (This won't happen as of now because the PWM framework caches the last
> state that was set and returns this for pwm_get_state. Still getting
> this right would be good.)

I understood your concern, but the period can be configured in
bootloader, we have no software things to save the accurate period.
Moreover I think I can change to use DIV_ROUND_CLOSET_ULL to keep the
accuracy.

>
> > > > +     tmp = (prescale + 1) * 1000000000ULL * duty;
> > > > +     state->duty_cycle = div64_u64(tmp, chn->clk_rate);
> > > > +
> > > > +     state->enabled = !!enabled;
> > > > +
> > > > +     /* Disable PWM clocks if the PWM channel is not in enable state. */
> > > > +     if (!enabled) {
> > > > +             clk_bulk_disable_unprepare(SPRD_PWM_NUM_CLKS, chn->clks);
> > > > +             chn->clk_enabled = false;
> > > > +     }
> > > > +}
> > > > +
> > > > +static const struct pwm_ops sprd_pwm_ops = {
> > > > +     .config = sprd_pwm_config,
> > > > +     .enable = sprd_pwm_enable,
> > > > +     .disable = sprd_pwm_disable,
> > > > +     .get_state = sprd_pwm_get_state,
> > > > +     .owner = THIS_MODULE,
> > > > +};
> > > > +
> > > > +static int sprd_pwm_clk_init(struct sprd_pwm_chip *spc)
> > > > +{
> > > > +     struct clk *clk_parent, *clk_pwm;
> > > > +     int ret, i, clk_index = 0;
> > > > +
> > > > +     clk_parent = devm_clk_get(spc->dev, "source");
> > > > +     if (IS_ERR(clk_parent)) {
> > > > +             dev_err(spc->dev, "failed to get source clock\n");
> > > > +             return PTR_ERR(clk_parent);
> > > > +     }
> > > > +
> > > > +     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;
> > >
> > > devm_clk_bulk_get_optional ? I'm sure you don't want to get all 8 clocks
> > > 8 times.
> >
> > This is not optional, each channel has 2 required clocks, and we have
> > 4 channels. (SPRD_PWM_NUM_CLKS == 2)
>
> I see. Currently it is not possible to use channel 2 if there are no
> clocks for channel 0, right? There is no hardware related problem here,

Yes.

> just a shortcoming of the driver?

Yes.

>
> > > > +
> > > > +                     dev_err(spc->dev, "failed to get channel clocks\n");
> > > > +                     return ret;
> > > > +             }
> > > > +
> > > > +             clk_pwm = chn->clks[1].clk;
> > >
> > > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > > all? You're not using i in the loop at all, this doesn't look right.
> >
> > Like I said above, each channel has 2 clocks: enable clock and pwm
> > clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> > which is used to set the source clock. I know this's not easy to read,
> > so do you have any good suggestion?
>
> Not sure this is easily possible to rework to make this clearer.
>
> Do these clks have different uses? e.g. one to enable register access
> and the other to enable the pwm output? If so just using

Yes.

> devm_clk_bulk_get isn't the right thing because you should be able know
> if clks[0] or clks[1] is the one you need to enable the output (or
> register access).

We've fixed the clock order in bulk clocks by the array
'sprd_pwm_clks', maybe I should define one readable macro instead of
magic number.

>
> > > > +             if (!clk_set_parent(clk_pwm, clk_parent))
> > > > +                     chn->clk_rate = clk_get_rate(clk_pwm);
> > > > +             else
> > > > +                     chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> > >
> > > I don't know all the clock framework details, but I think there are
> > > better ways to ensure that a given clock is used as parent for another
> > > given clock. Please read the chapter about "Assigned clock parents and
> > > rates" in the clock bindings and check if this could be used for the
> > > purpose here and so simplify the driver.
> >
> > Actually there are many other drivers set the parent clock like this,
> > and we want a default clock if failed to set the parent clock.
>
> These might be older than the clk framework capabilities, or the
> reviewers didn't pay attention to this detail; both shouldn't be a
> reason to not make it better here.

The clock framework supplies 'assigned-clocks' and
'assigned-clock-parents' properties to set parent, but for our case we
still want to set a default clock rate if failed to set parent when
met some abnormal things.

>
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int sprd_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > +     struct sprd_pwm_chip *spc = platform_get_drvdata(pdev);
> > > > +     int i;
> > > > +
> > > > +     for (i = 0; i < spc->num_pwms; i++)
> > > > +             pwm_disable(&spc->chip.pwms[i]);
> > >
> > > This is wrong. You must not call pwm_disable here. It's the consumer's
> > > responsibility to disable the hardware.
> >
> > Emm, I saw some drivers did like this, like pwm-spear.c.
> > Could you elaborate on what's the problem if the driver issues pwm_disable?
>
> This is a function to be called from code that called pwm_get before
> (i.e. pwm consumers). This just doesn't explode because up to now the
> PWM framework is only a thin wrapper around the lowlevel driver
> callbacks.
>
> The reasoning is similar to what I wrote in commit
> f82d15e223403b05fffb33ba792b87a8620f6fee. I'd like to have a PWM_DEBUG
> setting that catches such problems but the discussion with Thierry to
> even document the expectations is stuck, see
> https://patchwork.ozlabs.org/patch/1021566/.
>

Thanks for your explanation, and I'll remove pwm_disable from driver.

Thanks for your comments.

-- 
Baolin Wang
Best Regards

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

[Uwe Kleine-König]

Hello Baolin,

On Mon, Aug 12, 2019 at 03:29:07PM +0800, Baolin Wang wrote:
> Hi Uwe,
> 
> On Fri, 9 Aug 2019 at 22:41, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> > > On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > > > +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 enabled, duty, prescale;
> > > > > +     u64 tmp;
> > > > > +     int ret;
> > > > > +
> > > > > +     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;
> > > > > +     }
> > > > > +
> > > > > +     chn->clk_enabled = true;
> > > > > +
> > > > > +     duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > > > +     prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > > > +     enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > > > +
> > > > > +     /*
> > > > > +      * According to the datasheet, the period_ns and duty_ns calculation
> > > > > +      * formula should be:
> > > > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > > +      * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > > > +      */
> > > > > +     tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > > > +     state->period = div64_u64(tmp, chn->clk_rate);
> > > >
> > > > This is not idempotent. If you apply the configuration that is returned
> > > > here this shouldn't result in a reconfiguration.
> > >
> > > Since we may configure the  PWM in bootloader, so in kernel part we
> > > should get current PWM state to avoid reconfiguration if state
> > > configuration are same.
> >
> > This is also important as some consumers might do something like:
> >
> >         state = pwm_get_state(mypwm)
> >         if (something):
> >                 state->duty = 0
> >         else:
> >                 state->duty = state->period / 2
> >         pwm_set_state(mypwm, state)
> >
> > and then period shouldn't get smaller in each step.
> > (This won't happen as of now because the PWM framework caches the last
> > state that was set and returns this for pwm_get_state. Still getting
> > this right would be good.)
> 
> I understood your concern, but the period can be configured in
> bootloader, we have no software things to save the accurate period.

I don't understand what you're saying here. The bootloader configuring
the hardware is a usual use-case. That's why we have the .get_state
callback in the first place.

> Moreover I think I can change to use DIV_ROUND_CLOSET_ULL to keep the
> accuracy.

DIV_ROUND_CLOSEST_ULL still doesn't match what the apply callback uses.
With the lack of an official statement from the maintainer I'd prefer
.apply to round down and implement .get_state such that

	pwm_apply(pwm, pwm_get_state(pwm))

is a no-op.
 
> > > > > +
> > > > > +                     dev_err(spc->dev, "failed to get channel clocks\n");
> > > > > +                     return ret;
> > > > > +             }
> > > > > +
> > > > > +             clk_pwm = chn->clks[1].clk;
> > > >
> > > > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > > > all? You're not using i in the loop at all, this doesn't look right.
> > >
> > > Like I said above, each channel has 2 clocks: enable clock and pwm
> > > clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> > > which is used to set the source clock. I know this's not easy to read,
> > > so do you have any good suggestion?
> >
> > Not sure this is easily possible to rework to make this clearer.
> >
> > Do these clks have different uses? e.g. one to enable register access
> > and the other to enable the pwm output? If so just using
> 
> Yes.

So assuming one of the clocks is for operation of the output and the
other for accessing the registers, the latter can be disabled at the end
of each callback?

> > devm_clk_bulk_get isn't the right thing because you should be able know
> > if clks[0] or clks[1] is the one you need to enable the output (or
> > register access).
> 
> We've fixed the clock order in bulk clocks by the array
> 'sprd_pwm_clks', maybe I should define one readable macro instead of
> magic number.

ack.

> > > > > +             if (!clk_set_parent(clk_pwm, clk_parent))
> > > > > +                     chn->clk_rate = clk_get_rate(clk_pwm);
> > > > > +             else
> > > > > +                     chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> > > >
> > > > I don't know all the clock framework details, but I think there are
> > > > better ways to ensure that a given clock is used as parent for another
> > > > given clock. Please read the chapter about "Assigned clock parents and
> > > > rates" in the clock bindings and check if this could be used for the
> > > > purpose here and so simplify the driver.
> > >
> > > Actually there are many other drivers set the parent clock like this,
> > > and we want a default clock if failed to set the parent clock.
> >
> > These might be older than the clk framework capabilities, or the
> > reviewers didn't pay attention to this detail; both shouldn't be a
> > reason to not make it better here.
> 
> The clock framework supplies 'assigned-clocks' and
> 'assigned-clock-parents' properties to set parent, but for our case we
> still want to set a default clock rate if failed to set parent when
> met some abnormal things.

Without understanding the complete problem I'd say this is out of the
area the driver should care about.
 
Best regards
Uwe

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

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

[Baolin Wang]

Hi Uwe,

On Mon, 12 Aug 2019 at 16:36, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Mon, Aug 12, 2019 at 03:29:07PM +0800, Baolin Wang wrote:
> > Hi Uwe,
> >
> > On Fri, 9 Aug 2019 at 22:41, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Fri, Aug 09, 2019 at 06:06:21PM +0800, Baolin Wang wrote:
> > > > On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > > > > +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 enabled, duty, prescale;
> > > > > > +     u64 tmp;
> > > > > > +     int ret;
> > > > > > +
> > > > > > +     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;
> > > > > > +     }
> > > > > > +
> > > > > > +     chn->clk_enabled = true;
> > > > > > +
> > > > > > +     duty = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_DUTY) & SPRD_PWM_REG_MSK;
> > > > > > +     prescale = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_PRESCALE) & SPRD_PWM_REG_MSK;
> > > > > > +     enabled = sprd_pwm_read(spc, pwm->hwpwm, SPRD_PWM_ENABLE) & SPRD_PWM_ENABLE_BIT;
> > > > > > +
> > > > > > +     /*
> > > > > > +      * According to the datasheet, the period_ns and duty_ns calculation
> > > > > > +      * formula should be:
> > > > > > +      * period_ns = 10^9 * (prescale + 1) * mod / clk_rate
> > > > > > +      * duty_ns = 10^9 * (prescale + 1) * duty / clk_rate
> > > > > > +      */
> > > > > > +     tmp = (prescale + 1) * 1000000000ULL * SPRD_PWM_MOD_MAX;
> > > > > > +     state->period = div64_u64(tmp, chn->clk_rate);
> > > > >
> > > > > This is not idempotent. If you apply the configuration that is returned
> > > > > here this shouldn't result in a reconfiguration.
> > > >
> > > > Since we may configure the  PWM in bootloader, so in kernel part we
> > > > should get current PWM state to avoid reconfiguration if state
> > > > configuration are same.
> > >
> > > This is also important as some consumers might do something like:
> > >
> > >         state = pwm_get_state(mypwm)
> > >         if (something):
> > >                 state->duty = 0
> > >         else:
> > >                 state->duty = state->period / 2
> > >         pwm_set_state(mypwm, state)
> > >
> > > and then period shouldn't get smaller in each step.
> > > (This won't happen as of now because the PWM framework caches the last
> > > state that was set and returns this for pwm_get_state. Still getting
> > > this right would be good.)
> >
> > I understood your concern, but the period can be configured in
> > bootloader, we have no software things to save the accurate period.
>
> I don't understand what you're saying here. The bootloader configuring
> the hardware is a usual use-case. That's why we have the .get_state
> callback in the first place.

Ah, sorry for confusing. I think I get your point now with below explanation.

>
> > Moreover I think I can change to use DIV_ROUND_CLOSET_ULL to keep the
> > accuracy.
>
> DIV_ROUND_CLOSEST_ULL still doesn't match what the apply callback uses.
> With the lack of an official statement from the maintainer I'd prefer
> .apply to round down and implement .get_state such that
>
>         pwm_apply(pwm, pwm_get_state(pwm))
>
> is a no-op.

OK.

>
> > > > > > +
> > > > > > +                     dev_err(spc->dev, "failed to get channel clocks\n");
> > > > > > +                     return ret;
> > > > > > +             }
> > > > > > +
> > > > > > +             clk_pwm = chn->clks[1].clk;
> > > > >
> > > > > This 1 looks suspicious. Are you using all clocks provided in the dtb at
> > > > > all? You're not using i in the loop at all, this doesn't look right.
> > > >
> > > > Like I said above, each channel has 2 clocks: enable clock and pwm
> > > > clock, the 2nd clock of each channel's bulk clocks is the pwm clock,
> > > > which is used to set the source clock. I know this's not easy to read,
> > > > so do you have any good suggestion?
> > >
> > > Not sure this is easily possible to rework to make this clearer.
> > >
> > > Do these clks have different uses? e.g. one to enable register access
> > > and the other to enable the pwm output? If so just using
> >
> > Yes.
>
> So assuming one of the clocks is for operation of the output and the
> other for accessing the registers, the latter can be disabled at the end

Right.

> of each callback?

We can not disable the latter one when using the PWM channel, we must
enable the pwm-enable clock, then enable pwm-output clock to make PWM
work. When disabling PWM channel, we should disable the pwm-output
clock, then pwm-enable clock.

>
> > > devm_clk_bulk_get isn't the right thing because you should be able know
> > > if clks[0] or clks[1] is the one you need to enable the output (or
> > > register access).
> >
> > We've fixed the clock order in bulk clocks by the array
> > 'sprd_pwm_clks', maybe I should define one readable macro instead of
> > magic number.
>
> ack.
>
> > > > > > +             if (!clk_set_parent(clk_pwm, clk_parent))
> > > > > > +                     chn->clk_rate = clk_get_rate(clk_pwm);
> > > > > > +             else
> > > > > > +                     chn->clk_rate = SPRD_PWM_DEFAULT_CLK;
> > > > >
> > > > > I don't know all the clock framework details, but I think there are
> > > > > better ways to ensure that a given clock is used as parent for another
> > > > > given clock. Please read the chapter about "Assigned clock parents and
> > > > > rates" in the clock bindings and check if this could be used for the
> > > > > purpose here and so simplify the driver.
> > > >
> > > > Actually there are many other drivers set the parent clock like this,
> > > > and we want a default clock if failed to set the parent clock.
> > >
> > > These might be older than the clk framework capabilities, or the
> > > reviewers didn't pay attention to this detail; both shouldn't be a
> > > reason to not make it better here.
> >
> > The clock framework supplies 'assigned-clocks' and
> > 'assigned-clock-parents' properties to set parent, but for our case we
> > still want to set a default clock rate if failed to set parent when
> > met some abnormal things.
>
> Without understanding the complete problem I'd say this is out of the
> area the driver should care about.

Fair enough, I will try to use 'assigned-clocks' and
'assigned-clock-parents' to simplify the code.

Thanks.

-- 
Baolin Wang
Best Regards

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

[Uwe Kleine-König]

Hello Baolin,

On Mon, Aug 12, 2019 at 05:11:56PM +0800, Baolin Wang wrote:
> On Mon, 12 Aug 2019 at 16:36, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Mon, Aug 12, 2019 at 03:29:07PM +0800, Baolin Wang wrote:
> > > The clock framework supplies 'assigned-clocks' and
> > > 'assigned-clock-parents' properties to set parent, but for our case we
> > > still want to set a default clock rate if failed to set parent when
> > > met some abnormal things.
> >
> > Without understanding the complete problem I'd say this is out of the
> > area the driver should care about.
> 
> Fair enough, I will try to use 'assigned-clocks' and
> 'assigned-clock-parents' to simplify the code.

There is also assigned-clock-rates if you need that.

Best regards
Uwe

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

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

[Baolin Wang]

On Mon, 12 Aug 2019 at 17:46, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Baolin,
>
> On Mon, Aug 12, 2019 at 05:11:56PM +0800, Baolin Wang wrote:
> > On Mon, 12 Aug 2019 at 16:36, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Mon, Aug 12, 2019 at 03:29:07PM +0800, Baolin Wang wrote:
> > > > The clock framework supplies 'assigned-clocks' and
> > > > 'assigned-clock-parents' properties to set parent, but for our case we
> > > > still want to set a default clock rate if failed to set parent when
> > > > met some abnormal things.
> > >
> > > Without understanding the complete problem I'd say this is out of the
> > > area the driver should care about.
> >
> > Fair enough, I will try to use 'assigned-clocks' and
> > 'assigned-clock-parents' to simplify the code.
>
> There is also assigned-clock-rates if you need that.

Thanks for reminding, but we have no use case to set clock rate in PWM driver.

-- 
Baolin Wang
Best Regards

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

[Baolin Wang]

Hi Uwe,

On Fri, 9 Aug 2019 at 18:06, Baolin Wang <baolin.wang@linaro.org> wrote:
>
>  Hi Uwe,
>
> On Fri, 9 Aug 2019 at 17:10, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Thu, Aug 08, 2019 at 04:59:39PM +0800, Baolin Wang wrote:
> > > From: Neo Hou <neo.hou@unisoc.com>
> > >
> > > This patch adds the Spreadtrum PWM support, which provides maximum 4
> > > channels.
> > >
> > > Signed-off-by: Neo Hou <neo.hou@unisoc.com>
> > > Co-developed-by: Baolin Wang <baolin.wang@linaro.org>
> > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > > ---
> > >  drivers/pwm/Kconfig    |   10 ++
> > >  drivers/pwm/Makefile   |    1 +
> > >  drivers/pwm/pwm-sprd.c |  311 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 322 insertions(+)
> > >  create mode 100644 drivers/pwm/pwm-sprd.c
> > >
> > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > index a7e5751..4963b4d 100644
> > > --- a/drivers/pwm/Kconfig
> > > +++ b/drivers/pwm/Kconfig
> > > @@ -423,6 +423,16 @@ 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
> >
> > I think you need
> >
> >         depends on HAS_IOMEM
>
> OK.
>
> >
> > > +     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..f6fc793
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sprd.c
> > > @@ -0,0 +1,311 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright (C) 2019 Spreadtrum Communications Inc.
> >
> > If there is a publicly available reference manual available, please add
> > a link to it here.
>
> Sure.

Sorry, we have not supplied a publicly available reference manual now.
So no change for this comment in next version.

-- 
Baolin Wang
Best Regards