[PATCH v2 0/7] Add rk3328 pwm support

[PATCH v2 0/7] Add rk3328 pwm support

[David Wu]

There are two features of rk3328 pwm module.
 - PWM APB and function clocks are different.
 - Add pwm atomic hardware update

David Wu (7):
  pwm: rockchip: Add APB and function both clocks support
  pwm: rockchip: Remove the judge from return value of pwm_config
  pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops
  pwm: rockchip: Use pwm_apply instead of the pwm_enable
  pwm: rockchip: Move the configuration of polarity from
    rockchip_pwm_set_enable() to rockchip_pwm_config()
  pwm: rockchip: Add rk3328 pwm support
  arm64: dts: rockchip: Add pwm nodes for rk3328

 .../devicetree/bindings/pwm/pwm-rockchip.txt       |   9 +-
 arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  45 ++++
 drivers/pwm/pwm-rockchip.c                         | 275 ++++++++++++++-------
 3 files changed, 236 insertions(+), 93 deletions(-)

-- 
1.9.1

[PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support

[David Wu]

New PWM module provides two individual clocks for APB clock
and function clock.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/pwm/pwm-rockchip.txt       |  8 +++-
 drivers/pwm/pwm-rockchip.c                         | 53 ++++++++++++++++++----
 2 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
index b8be3d0..2350ef9 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
@@ -6,7 +6,13 @@ Required properties:
    "rockchip,rk3288-pwm": found on RK3288 SoC
    "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
  - reg: physical base address and length of the controller's registers
- - clocks: phandle and clock specifier of the PWM reference clock
+ - clocks: See ../clock/clock-bindings.txt
+   - For older hardware (rk2928, rk3066, rk3188, rk3228, rk3288, rk3399):
+     - There is one clock that's used both to derive the functional clock
+       for the device and as the bus clock.
+   - For newer hardware (rk3328 and future socs): specified by name
+     - "pwm": This is used to derive the functional clock.
+     - "pclk": This is the APB bus clock.
  - #pwm-cells: must be 2 (rk2928) or 3 (rk3288). See pwm.txt in this directory
    for a description of the cell format.
 
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 744d561..617824c 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -33,6 +33,7 @@
 struct rockchip_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
+	struct clk *pclk;
 	const struct rockchip_pwm_data *data;
 	void __iomem *base;
 };
@@ -145,7 +146,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 	u64 tmp;
 	int ret;
 
-	ret = clk_enable(pc->clk);
+	ret = clk_enable(pc->pclk);
 	if (ret)
 		return;
 
@@ -161,7 +162,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 
 	pc->data->get_state(chip, pwm, state);
 
-	clk_disable(pc->clk);
+	clk_disable(pc->pclk);
 }
 
 static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -224,7 +225,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	pwm_get_state(pwm, &curstate);
 	enabled = curstate.enabled;
 
-	ret = clk_enable(pc->clk);
+	ret = clk_enable(pc->pclk);
 	if (ret)
 		return ret;
 
@@ -257,7 +258,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	rockchip_pwm_get_state(chip, pwm, state);
 
 out:
-	clk_disable(pc->clk);
+	clk_disable(pc->pclk);
 
 	return ret;
 }
@@ -328,7 +329,7 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	const struct of_device_id *id;
 	struct rockchip_pwm_chip *pc;
 	struct resource *r;
-	int ret;
+	int ret, count;
 
 	id = of_match_device(rockchip_pwm_dt_ids, &pdev->dev);
 	if (!id)
@@ -343,13 +344,38 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pc->base))
 		return PTR_ERR(pc->base);
 
-	pc->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(pc->clk))
-		return PTR_ERR(pc->clk);
+	pc->clk = devm_clk_get(&pdev->dev, "pwm");
+	count = of_property_count_strings(pdev->dev.of_node, "clock-names");
+	if (count == 2)
+		pc->pclk = devm_clk_get(&pdev->dev, "pclk");
+	else
+		pc->pclk = pc->clk;
+
+	if (IS_ERR(pc->clk)) {
+		ret = PTR_ERR(pc->clk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
+		return ret;
+	}
+
+	if (IS_ERR(pc->pclk)) {
+		ret = PTR_ERR(pc->pclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret);
+		return ret;
+	}
 
 	ret = clk_prepare_enable(pc->clk);
-	if (ret)
+	if (ret) {
+		dev_err(&pdev->dev, "Can't prepare enable bus clk: %d\n", ret);
 		return ret;
+	}
+
+	ret = clk_prepare(pc->pclk);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't prepare APB clk: %d\n", ret);
+		goto err_clk;
+	}
 
 	platform_set_drvdata(pdev, pc);
 
@@ -368,12 +394,20 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
 	if (ret < 0) {
 		clk_unprepare(pc->clk);
 		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
+		goto err_pclk;
 	}
 
 	/* Keep the PWM clk enabled if the PWM appears to be up and running. */
 	if (!pwm_is_enabled(pc->chip.pwms))
 		clk_disable(pc->clk);
 
+	return 0;
+
+err_pclk:
+	clk_unprepare(pc->pclk);
+err_clk:
+	clk_disable_unprepare(pc->clk);
+
 	return ret;
 }
 
@@ -395,6 +429,7 @@ static int rockchip_pwm_remove(struct platform_device *pdev)
 	if (pwm_is_enabled(pc->chip.pwms))
 		clk_disable(pc->clk);
 
+	clk_unprepare(pc->pclk);
 	clk_unprepare(pc->clk);
 
 	return pwmchip_remove(&pc->chip);
-- 
1.9.1

Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support

[Doug Anderson]

Hi,

On Fri, Jul 7, 2017 at 9:03 PM, David Wu <david.wu@rock-chips.com> wrote:

> @@ -6,7 +6,13 @@ Required properties:
>     "rockchip,rk3288-pwm": found on RK3288 SoC
>     "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
>   - reg: physical base address and length of the controller's registers
> - - clocks: phandle and clock specifier of the PWM reference clock
> + - clocks: See ../clock/clock-bindings.txt
> +   - For older hardware (rk2928, rk3066, rk3188, rk3228, rk3288, rk3399):
> +     - There is one clock that's used both to derive the functional clock
> +       for the device and as the bus clock.
> +   - For newer hardware (rk3328 and future socs): specified by name
> +     - "pwm": This is used to derive the functional clock.
> +     - "pclk": This is the APB bus clock.

I'm pretty sure that that the above description doesn't quite match the code.

* The above description says that for old hardware there is one clock
and 'clock-names' was not necessary (though as I understand it it's OK
if it's there).

* The old code matched the old description.  AKA: if there was no
"clock-names" then everything was OK.

* The new code will not work if there was no "clock-names".

Many of the old devices had a clock-names present (and it was "pwm"),
but not all.  Specifically it looks like
"arch/arm/boot/dts/rk3xxx.dtsi" doesn't specify a clock-names.

> @@ -343,13 +344,38 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>         if (IS_ERR(pc->base))
>                 return PTR_ERR(pc->base);
>
> -       pc->clk = devm_clk_get(&pdev->dev, NULL);
> -       if (IS_ERR(pc->clk))
> -               return PTR_ERR(pc->clk);
> +       pc->clk = devm_clk_get(&pdev->dev, "pwm");
> +       count = of_property_count_strings(pdev->dev.of_node, "clock-names");
> +       if (count == 2)
> +               pc->pclk = devm_clk_get(&pdev->dev, "pclk");
> +       else
> +               pc->pclk = pc->clk;
> +
> +       if (IS_ERR(pc->clk)) {
> +               ret = PTR_ERR(pc->clk);
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
> +               return ret;
> +       }
> +
> +       if (IS_ERR(pc->pclk)) {
> +               ret = PTR_ERR(pc->pclk);
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret);
> +               return ret;
> +       }

In the above code you need to check the count _before_ trying to get
the clock by name.


-Doug

Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support

[David.Wu]

Hi Doug,

在 2017/7/12 1:03, Doug Anderson 写道:
> Hi,
> 
> On Fri, Jul 7, 2017 at 9:03 PM, David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> 
>> @@ -6,7 +6,13 @@ Required properties:
>>      "rockchip,rk3288-pwm": found on RK3288 SoC
>>      "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
>>    - reg: physical base address and length of the controller's registers
>> - - clocks: phandle and clock specifier of the PWM reference clock
>> + - clocks: See ../clock/clock-bindings.txt
>> +   - For older hardware (rk2928, rk3066, rk3188, rk3228, rk3288, rk3399):
>> +     - There is one clock that's used both to derive the functional clock
>> +       for the device and as the bus clock.
>> +   - For newer hardware (rk3328 and future socs): specified by name
>> +     - "pwm": This is used to derive the functional clock.
>> +     - "pclk": This is the APB bus clock.
> 
> I'm pretty sure that that the above description doesn't quite match the code.
> 
> * The above description says that for old hardware there is one clock
> and 'clock-names' was not necessary (though as I understand it it's OK
> if it's there).
> 
> * The old code matched the old description.  AKA: if there was no
> "clock-names" then everything was OK.
> 
> * The new code will not work if there was no "clock-names".
> 
> Many of the old devices had a clock-names present (and it was "pwm"),
> but not all.  Specifically it looks like
> "arch/arm/boot/dts/rk3xxx.dtsi" doesn't specify a clock-names.
> 

So we can keep code: the pc->clk = devm_clk_get(&pdev->dev, NULL);
If the name is NULL, we can get the first clk defined at DTB.

>> @@ -343,13 +344,38 @@ static int rockchip_pwm_probe(struct platform_device *pdev)
>>          if (IS_ERR(pc->base))
>>                  return PTR_ERR(pc->base);
>>
>> -       pc->clk = devm_clk_get(&pdev->dev, NULL);
>> -       if (IS_ERR(pc->clk))
>> -               return PTR_ERR(pc->clk);
>> +       pc->clk = devm_clk_get(&pdev->dev, "pwm");
>> +       count = of_property_count_strings(pdev->dev.of_node, "clock-names");
>> +       if (count == 2)
>> +               pc->pclk = devm_clk_get(&pdev->dev, "pclk");
>> +       else
>> +               pc->pclk = pc->clk;
>> +
>> +       if (IS_ERR(pc->clk)) {
>> +               ret = PTR_ERR(pc->clk);
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(&pdev->dev, "Can't get bus clk: %d\n", ret);
>> +               return ret;
>> +       }
>> +
>> +       if (IS_ERR(pc->pclk)) {
>> +               ret = PTR_ERR(pc->pclk);
>> +               if (ret != -EPROBE_DEFER)
>> +                       dev_err(&pdev->dev, "Can't get APB clk: %d\n", ret);
>> +               return ret;
>> +       }
> 
> In the above code you need to check the count _before_ trying to get
> the clock by name.

Okay, I will fix it.

> 
> 
> -Doug
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support

[Heiko Stübner]

Hi David,

Am Mittwoch, 12. Juli 2017, 16:38:09 CEST schrieb David.Wu:
> Hi Doug,
> 
> 在 2017/7/12 1:03, Doug Anderson 写道:
> > Hi,
> > 
> > On Fri, Jul 7, 2017 at 9:03 PM, David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> >> @@ -6,7 +6,13 @@ Required properties:
> >>      "rockchip,rk3288-pwm": found on RK3288 SoC
> >>      "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
> >>    
> >>    - reg: physical base address and length of the controller's registers
> >> 
> >> - - clocks: phandle and clock specifier of the PWM reference clock
> >> + - clocks: See ../clock/clock-bindings.txt
> >> +   - For older hardware (rk2928, rk3066, rk3188, rk3228, rk3288,
> >> rk3399):
> >> +     - There is one clock that's used both to derive the functional
> >> clock
> >> +       for the device and as the bus clock.
> >> +   - For newer hardware (rk3328 and future socs): specified by name
> >> +     - "pwm": This is used to derive the functional clock.
> >> +     - "pclk": This is the APB bus clock.
> > 
> > I'm pretty sure that that the above description doesn't quite match the
> > code.
> > 
> > * The above description says that for old hardware there is one clock
> > and 'clock-names' was not necessary (though as I understand it it's OK
> > if it's there).
> > 
> > * The old code matched the old description.  AKA: if there was no
> > "clock-names" then everything was OK.
> > 
> > * The new code will not work if there was no "clock-names".
> > 
> > Many of the old devices had a clock-names present (and it was "pwm"),
> > but not all.  Specifically it looks like
> > "arch/arm/boot/dts/rk3xxx.dtsi" doesn't specify a clock-names.
> 
> So we can keep code: the pc->clk = devm_clk_get(&pdev->dev, NULL);
> If the name is NULL, we can get the first clk defined at DTB.

I don't think it will work that way.

clk_get with NULL argument will likely grab the first clock of the list
independent of clock-names being present.

It might be better to do something like [pseudo-code]:

pwm->pclk = clk_get( ..., "pclk");
if (IS_ERR(pwm->pclk))
	pwm->pclk = NULL;

pwm->pwm_clk = clk_get(..., "pwm");
if (IS_ERR(pwm->pwm_clk))
	pwm->pwm_clk = clk_get(..., NULL);
if (IS_ERR(pwm->pwm_clk))
	return PTR_ERR(pwm->pwm_clk);

That way, you make your way backwards through the pwm-binding-history.
Ending at the single-clock without clock-names property as the last fallback.


Heiko
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/7] pwm: rockchip: Remove the judge from return value of pwm_config

[David Wu]

It seems the rockchip_pwm_config always returns the result 0,
so remove the judge.

Signed-off-by: David Wu <david.wu@rock-chips.com>
Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/pwm/pwm-rockchip.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 617824c..cd45f17 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -165,7 +165,7 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 	clk_disable(pc->pclk);
 }
 
-static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			       int duty_ns, int period_ns)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
@@ -188,8 +188,6 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
-
-	return 0;
 }
 
 static int rockchip_pwm_enable(struct pwm_chip *chip,
@@ -236,13 +234,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		enabled = false;
 	}
 
-	ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
-	if (ret) {
-		if (enabled != curstate.enabled)
-			rockchip_pwm_enable(chip, pwm, !enabled,
-				      state->polarity);
-		goto out;
-	}
+	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
 
 	if (state->enabled != enabled) {
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-- 
1.9.1

[PATCH v2 4/7] pwm: rockchip: Use pwm_apply instead of the pwm_enable

[David Wu]

Drop the custom hook of pwm_enable and implement
pwm_apply_v1 and pwm_apply_v2 instead.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 drivers/pwm/pwm-rockchip.c | 141 +++++++++++++++++++++++++--------------------
 1 file changed, 77 insertions(+), 64 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 85f9515..12e7b4a8 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -51,11 +51,10 @@ struct rockchip_pwm_data {
 	bool supports_polarity;
 	const struct pwm_ops *ops;
 
-	void (*set_enable)(struct pwm_chip *chip,
-			   struct pwm_device *pwm, bool enable,
-			   enum pwm_polarity polarity);
 	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state);
+	int (*pwm_apply)(struct pwm_chip *chip, struct pwm_device *pwm,
+			 struct pwm_state *state);
 };
 
 static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
@@ -63,24 +62,6 @@ static inline struct rockchip_pwm_chip *to_rockchip_pwm_chip(struct pwm_chip *c)
 	return container_of(c, struct rockchip_pwm_chip, chip);
 }
 
-static void rockchip_pwm_set_enable_v1(struct pwm_chip *chip,
-				       struct pwm_device *pwm, bool enable,
-				       enum pwm_polarity polarity)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
-	u32 val;
-
-	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
-
-	if (enable)
-		val |= enable_conf;
-	else
-		val &= ~enable_conf;
-
-	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
-}
-
 static void rockchip_pwm_get_state_v1(struct pwm_chip *chip,
 				      struct pwm_device *pwm,
 				      struct pwm_state *state)
@@ -94,30 +75,6 @@ static void rockchip_pwm_get_state_v1(struct pwm_chip *chip,
 		state->enabled = true;
 }
 
-static void rockchip_pwm_set_enable_v2(struct pwm_chip *chip,
-				       struct pwm_device *pwm, bool enable,
-				       enum pwm_polarity polarity)
-{
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
-	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
-			  PWM_CONTINUOUS;
-	u32 val;
-
-	if (polarity == PWM_POLARITY_INVERSED)
-		enable_conf |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
-	else
-		enable_conf |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
-
-	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
-
-	if (enable)
-		val |= enable_conf;
-	else
-		val &= ~enable_conf;
-
-	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
-}
-
 static void rockchip_pwm_get_state_v2(struct pwm_chip *chip,
 				      struct pwm_device *pwm,
 				      struct pwm_state *state)
@@ -193,10 +150,12 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int rockchip_pwm_enable(struct pwm_chip *chip,
 			 struct pwm_device *pwm,
 			 bool enable,
-			 enum pwm_polarity polarity)
+			 enum pwm_polarity polarity,
+			 u32 enable_conf)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	int ret;
+	u32 val;
 
 	if (enable) {
 		ret = clk_enable(pc->clk);
@@ -204,7 +163,23 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 			return ret;
 	}
 
-	pc->data->set_enable(chip, pwm, enable, polarity);
+	if (pc->data->supports_polarity) {
+		if (polarity == PWM_POLARITY_INVERSED)
+			enable_conf |= PWM_DUTY_NEGATIVE |
+				       PWM_INACTIVE_POSITIVE;
+		else
+			enable_conf |= PWM_DUTY_POSITIVE |
+				       PWM_INACTIVE_NEGATIVE;
+	}
+
+	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
+
+	if (enable)
+		val |= enable_conf;
+	else
+		val &= ~enable_conf;
+
+	writel_relaxed(val, pc->base + pc->data->regs.ctrl);
 
 	if (!enable)
 		clk_disable(pc->clk);
@@ -212,37 +187,75 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 	return 0;
 }
 
-static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			      struct pwm_state *state)
+static int rockchip_pwm_apply_v1(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
 {
-	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	u32 enable_conf = PWM_CTRL_OUTPUT_EN | PWM_CTRL_TIMER_EN;
 	struct pwm_state curstate;
 	bool enabled;
-	int ret;
+	int ret = 0;
 
 	pwm_get_state(pwm, &curstate);
 	enabled = curstate.enabled;
 
-	ret = clk_enable(pc->pclk);
-	if (ret)
-		return ret;
-
 	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity);
+		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
+					  enable_conf);
 		if (ret)
-			goto out;
+			return ret;
 		enabled = false;
 	}
 
 	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
-
-	if (state->enabled != enabled) {
+	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-				    state->polarity);
+				    state->polarity, enable_conf);
+
+	return ret;
+}
+
+static int rockchip_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+		  PWM_CONTINUOUS;
+	struct pwm_state curstate;
+	bool enabled;
+	int ret = 0;
+
+	pwm_get_state(pwm, &curstate);
+	enabled = curstate.enabled;
+
+	if (state->polarity != curstate.polarity && enabled) {
+		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
+					  enable_conf);
 		if (ret)
-			goto out;
+			return ret;
+		enabled = false;
 	}
 
+	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	if (state->enabled != enabled)
+		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
+					  state->polarity, enable_conf);
+
+	return ret;
+}
+
+static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      struct pwm_state *state)
+{
+	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+	int ret;
+
+	ret = clk_enable(pc->pclk);
+	if (ret)
+		return ret;
+
+	ret = pc->data->pwm_apply(chip, pwm, state);
+	if (ret)
+		goto out;
+
 	/*
 	 * Update the state with the real hardware, which can differ a bit
 	 * because of period/duty_cycle approximation.
@@ -270,8 +283,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 2,
 	.ops = &rockchip_pwm_ops,
-	.set_enable = rockchip_pwm_set_enable_v1,
 	.get_state = rockchip_pwm_get_state_v1,
+	.pwm_apply = rockchip_pwm_apply_v1,
 };
 
 static const struct rockchip_pwm_data pwm_data_v2 = {
@@ -284,8 +297,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	.prescaler = 1,
 	.supports_polarity = true,
 	.ops = &rockchip_pwm_ops,
-	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
+	.pwm_apply = rockchip_pwm_apply_v2,
 };
 
 static const struct rockchip_pwm_data pwm_data_vop = {
@@ -298,8 +311,8 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	.prescaler = 1,
 	.supports_polarity = true,
 	.ops = &rockchip_pwm_ops,
-	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
+	.pwm_apply = rockchip_pwm_apply_v2,
 };
 
 static const struct of_device_id rockchip_pwm_dt_ids[] = {
-- 
1.9.1

[PATCH v2 6/7] pwm: rockchip: Add rk3328 pwm support

[David Wu]

The rk3328 soc supports atomic update, we could lock the configuration
of period and duty at first, after unlock is configured, the period and
duty are effective at the same time.

If the polarity, period and duty need to be configured together,
the way for atomic update is "configure lock and old polarity" ->
"configure period and duty" -> "configure unlock and new polarity".

Signed-off-by: David Wu <david.wu@rock-chips.com>
Acked-by: Rob Herring <robh@kernel.org>
---
change in v2:
 - 3 different pwm_ops apply(),one for each IP revision.
 .../devicetree/bindings/pwm/pwm-rockchip.txt       |  1 +
 drivers/pwm/pwm-rockchip.c                         | 63 ++++++++++++++++++++--
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
index 2350ef9..152c736 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
@@ -4,6 +4,7 @@ Required properties:
  - compatible: should be "rockchip,<name>-pwm"
    "rockchip,rk2928-pwm": found on RK29XX,RK3066 and RK3188 SoCs
    "rockchip,rk3288-pwm": found on RK3288 SoC
+   "rockchip,rk3328-pwm": found on RK3328 SoC
    "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
  - reg: physical base address and length of the controller's registers
  - clocks: See ../clock/clock-bindings.txt
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 83703e1..838e8fc 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -29,6 +29,7 @@
 #define PWM_INACTIVE_POSITIVE	(1 << 4)
 #define PWM_POLARITY_MASK	(PWM_DUTY_POSITIVE | PWM_INACTIVE_POSITIVE)
 #define PWM_OUTPUT_LEFT		(0 << 5)
+#define PWM_LOCK_EN		(1 << 6)
 #define PWM_LP_DISABLE		(0 << 8)
 
 struct rockchip_pwm_chip {
@@ -124,13 +125,24 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 }
 
 static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			       struct pwm_state *state, bool polarity)
+			       struct pwm_state *state, bool polarity,
+			       bool lock)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	unsigned long period, duty;
 	u64 clk_rate, div;
 	u32 ctrl;
 
+	/*
+	 * Lock the period and duty of previous configuration, then
+	 * change the duty and period, that would not be effective.
+	 */
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	if (lock) {
+		ctrl |= PWM_LOCK_EN;
+		writel_relaxed(ctrl, pc->base + pc->data->regs.ctrl);
+	}
+
 	clk_rate = clk_get_rate(pc->clk);
 
 	/*
@@ -148,7 +160,6 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
 
-	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
 	if (polarity) {
 		ctrl &= ~PWM_POLARITY_MASK;
 		if (state->polarity == PWM_POLARITY_INVERSED)
@@ -156,6 +167,15 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		else
 			ctrl |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
 	}
+
+	/*
+	 * Unlock and set polarity at the same time,
+	 * the configuration of duty, period and polarity
+	 * would be effective together at next period.
+	 */
+	if (lock)
+		ctrl &= ~PWM_LOCK_EN;
+
 	writel(ctrl, pc->base + pc->data->regs.ctrl);
 
 	return 0;
@@ -209,7 +229,7 @@ static int rockchip_pwm_apply_v1(struct pwm_chip *chip, struct pwm_device *pwm,
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state, false);
+	rockchip_pwm_config(chip, pwm, state, false, false);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
 				    enable_conf);
@@ -236,7 +256,27 @@ static int rockchip_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state, true);
+	rockchip_pwm_config(chip, pwm, state, true, false);
+	if (state->enabled != enabled)
+		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
+					  enable_conf);
+
+	return ret;
+}
+
+static int rockchip_pwm_apply_v3(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+		  PWM_CONTINUOUS;
+	struct pwm_state curstate;
+	bool enabled;
+	int ret = 0;
+
+	pwm_get_state(pwm, &curstate);
+	enabled = curstate.enabled;
+
+	rockchip_pwm_config(chip, pwm, state, true, true);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
 					  enable_conf);
@@ -317,9 +357,24 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	.pwm_apply = rockchip_pwm_apply_v2,
 };
 
+static const struct rockchip_pwm_data pwm_data_v3 = {
+	.regs = {
+		.duty = 0x08,
+		.period = 0x04,
+		.cntr = 0x00,
+		.ctrl = 0x0c,
+	},
+	.prescaler = 1,
+	.supports_polarity = true,
+	.ops = &rockchip_pwm_ops,
+	.get_state = rockchip_pwm_get_state_v2,
+	.pwm_apply = rockchip_pwm_apply_v3,
+};
+
 static const struct of_device_id rockchip_pwm_dt_ids[] = {
 	{ .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1},
 	{ .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2},
+	{ .compatible = "rockchip,rk3328-pwm", .data = &pwm_data_v2},
 	{ .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop},
 	{ /* sentinel */ }
 };
-- 
1.9.1

[PATCH v2 7/7] arm64: dts: rockchip: Add pwm nodes for rk3328

[David Wu]

There are 4 pwm channels built in rk3328 soc, need to configure
the both APB clock and bus clock.

Signed-off-by: David Wu <david.wu@rock-chips.com>
---
 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 45 ++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index 29b3800..46f0847 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -310,6 +310,51 @@
 		interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
 	};
 
+	pwm0: pwm@ff1b0000 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0000 0x0 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm0_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
+	pwm1: pwm@ff1b0010 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0010 0x0 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm1_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
+	pwm2: pwm@ff1b0020 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0020 0x0 0x10>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwm2_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
+	pwm3: pwm@ff1b0030 {
+		compatible = "rockchip,rk3328-pwm";
+		reg = <0x0 0xff1b0030 0x0 0x10>;
+		interrupts = <GIC_SPI 50 IRQ_TYPE_LEVEL_HIGH>;
+		#pwm-cells = <3>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwmir_pin>;
+		clocks = <&cru SCLK_PWM>, <&cru PCLK_PWM>;
+		clock-names = "pwm", "pclk";
+		status = "disabled";
+	};
+
 	saradc: adc@ff280000 {
 		compatible = "rockchip,rk3328-saradc", "rockchip,rk3399-saradc";
 		reg = <0x0 0xff280000 0x0 0x100>;
-- 
1.9.1

[PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

[David Wu]

The rockchip_pwm_ops_v1 and rockchip_pwm_ops_v2 ops are the same
struct members, remove one of them.

Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 drivers/pwm/pwm-rockchip.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index cd45f17..85f9515 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -255,13 +255,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
-static const struct pwm_ops rockchip_pwm_ops_v1 = {
-	.get_state = rockchip_pwm_get_state,
-	.apply = rockchip_pwm_apply,
-	.owner = THIS_MODULE,
-};
-
-static const struct pwm_ops rockchip_pwm_ops_v2 = {
+static const struct pwm_ops rockchip_pwm_ops = {
 	.get_state = rockchip_pwm_get_state,
 	.apply = rockchip_pwm_apply,
 	.owner = THIS_MODULE,
@@ -275,7 +269,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		.ctrl = 0x0c,
 	},
 	.prescaler = 2,
-	.ops = &rockchip_pwm_ops_v1,
+	.ops = &rockchip_pwm_ops,
 	.set_enable = rockchip_pwm_set_enable_v1,
 	.get_state = rockchip_pwm_get_state_v1,
 };
@@ -289,7 +283,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 1,
 	.supports_polarity = true,
-	.ops = &rockchip_pwm_ops_v2,
+	.ops = &rockchip_pwm_ops,
 	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
 };
@@ -303,7 +297,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	},
 	.prescaler = 1,
 	.supports_polarity = true,
-	.ops = &rockchip_pwm_ops_v2,
+	.ops = &rockchip_pwm_ops,
 	.set_enable = rockchip_pwm_set_enable_v2,
 	.get_state = rockchip_pwm_get_state_v2,
 };
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

[Boris Brezillon]

On Sat,  8 Jul 2017 12:03:45 +0800
David Wu <david.wu@rock-chips.com> wrote:

> The rockchip_pwm_ops_v1 and rockchip_pwm_ops_v2 ops are the same
> struct members, remove one of them.
> 
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
>  drivers/pwm/pwm-rockchip.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index cd45f17..85f9515 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -255,13 +255,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return ret;
>  }
>  
> -static const struct pwm_ops rockchip_pwm_ops_v1 = {
> -	.get_state = rockchip_pwm_get_state,
> -	.apply = rockchip_pwm_apply,
> -	.owner = THIS_MODULE,
> -};
> -
> -static const struct pwm_ops rockchip_pwm_ops_v2 = {
> +static const struct pwm_ops rockchip_pwm_ops = {
>  	.get_state = rockchip_pwm_get_state,
>  	.apply = rockchip_pwm_apply,
>  	.owner = THIS_MODULE,
> @@ -275,7 +269,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		.ctrl = 0x0c,
>  	},
>  	.prescaler = 2,
> -	.ops = &rockchip_pwm_ops_v1,
> +	.ops = &rockchip_pwm_ops,
>  	.set_enable = rockchip_pwm_set_enable_v1,
>  	.get_state = rockchip_pwm_get_state_v1,
>  };
> @@ -289,7 +283,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	},
>  	.prescaler = 1,
>  	.supports_polarity = true,
> -	.ops = &rockchip_pwm_ops_v2,
> +	.ops = &rockchip_pwm_ops,
>  	.set_enable = rockchip_pwm_set_enable_v2,
>  	.get_state = rockchip_pwm_get_state_v2,
>  };
> @@ -303,7 +297,7 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	},
>  	.prescaler = 1,
>  	.supports_polarity = true,
> -	.ops = &rockchip_pwm_ops_v2,
> +	.ops = &rockchip_pwm_ops,
>  	.set_enable = rockchip_pwm_set_enable_v2,
>  	.get_state = rockchip_pwm_get_state_v2,
>  };

Actually, when I suggested to just implement ->apply_state() and be
done with all other fields I was thinking that you could get rid of
this rockchip_pwm_data struct entirely and just have 3 different
pwm_ops. You seem to take the other direction here: you're removing
rockchip_pwm_ops_v1 and renaming rockchip_pwm_ops_v2 into
rockchip_pwm_ops.

Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

[David.Wu]

Hi Boris,

在 2017/8/2 16:59, Boris Brezillon 写道:
> Actually, when I suggested to just implement ->apply_state() and be
> done with all other fields I was thinking that you could get rid of
> this rockchip_pwm_data struct entirely and just have 3 different
> pwm_ops. You seem to take the other direction here: you're removing
> rockchip_pwm_ops_v1 and renaming rockchip_pwm_ops_v2 into
> rockchip_pwm_ops.

Yes, i really didn't understand exactly what you mean.
Your mean is that remove the set_enable, get_state and other hooks,
then use the pwm_ops instead of them, which has 3 different version, and 
implement the pwm_ops's functions like apply(), enable(), get_state() 
and others...?

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

[Boris Brezillon]

On Wed, 2 Aug 2017 19:31:57 +0800
"David.Wu" <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:

> Hi Boris,
> 
> 在 2017/8/2 16:59, Boris Brezillon 写道:
> > Actually, when I suggested to just implement ->apply_state() and be
> > done with all other fields I was thinking that you could get rid of
> > this rockchip_pwm_data struct entirely and just have 3 different
> > pwm_ops. You seem to take the other direction here: you're removing
> > rockchip_pwm_ops_v1 and renaming rockchip_pwm_ops_v2 into
> > rockchip_pwm_ops.  
> 
> Yes, i really didn't understand exactly what you mean.
> Your mean is that remove the set_enable, get_state and other hooks,
> then use the pwm_ops instead of them, which has 3 different version, and 
> implement the pwm_ops's functions like apply(), enable(), get_state() 
> and others...?
> 

Yep, just define 3 different pwm_ops (one for each IP), each of them
implementing ->apply() and ->get_state() and that's all.

Something like:

static const struct pwm_ops rockchip_pwm_ops_v1 = {
	.get_state = rockchip_pwm_v1_get_state,
	.apply = rockchip_pwm_v1_apply,
	.owner = THIS_MODULE,
};

static const struct pwm_ops rockchip_pwm_ops_v2 = {
	.get_state = rockchip_pwm_v2_get_state,
	.apply = rockchip_pwm_v2_apply,
	.owner = THIS_MODULE,
};

static const struct pwm_ops rockchip_pwm_ops_vop = {
	.get_state = rockchip_pwm_vop_get_state,
	.apply = rockchip_pwm_vop_apply,
	.owner = THIS_MODULE,
};

static const struct of_device_id rockchip_pwm_dt_ids[] = {
	{ .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 },
	{ .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 },
	{ .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop },
	{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

[David.Wu]

Hi Boris，

在 2017/8/2 19:40, Boris Brezillon 写道:
> Yep, just define 3 different pwm_ops (one for each IP), each of them
> implementing ->apply() and ->get_state() and that's all.
> 
> Something like:
> 
> static const struct pwm_ops rockchip_pwm_ops_v1 = {
> 	.get_state = rockchip_pwm_v1_get_state,
> 	.apply = rockchip_pwm_v1_apply,
> 	.owner = THIS_MODULE,
> };
> 
> static const struct pwm_ops rockchip_pwm_ops_v2 = {
> 	.get_state = rockchip_pwm_v2_get_state,
> 	.apply = rockchip_pwm_v2_apply,
> 	.owner = THIS_MODULE,
> };
> 
> static const struct pwm_ops rockchip_pwm_ops_vop = {
> 	.get_state = rockchip_pwm_vop_get_state,
> 	.apply = rockchip_pwm_vop_apply,
> 	.owner = THIS_MODULE,
> };
> 
> static const struct of_device_id rockchip_pwm_dt_ids[] = {
> 	{ .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 },
> 	{ .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 },
> 	{ .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop },
> 	{ /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);

I think we should keep the data members in the rockchip_pwm_data，like 
supports_polarity and regs...

The supports_polarity is needed for of_pwm_n_cells when pwm registered.
And the other data members is helpful for us to use common code.

It's okay for just define 3 different pwm_ops (one for each IP), but 
they are with other data members in the struct of rockchip_pwm_data.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

[Boris Brezillon]

On Fri, 4 Aug 2017 10:38:26 +0800
"David.Wu" <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:

> Hi Boris，
> 
> 在 2017/8/2 19:40, Boris Brezillon 写道:
> > Yep, just define 3 different pwm_ops (one for each IP), each of them
> > implementing ->apply() and ->get_state() and that's all.
> > 
> > Something like:
> > 
> > static const struct pwm_ops rockchip_pwm_ops_v1 = {
> > 	.get_state = rockchip_pwm_v1_get_state,
> > 	.apply = rockchip_pwm_v1_apply,
> > 	.owner = THIS_MODULE,
> > };
> > 
> > static const struct pwm_ops rockchip_pwm_ops_v2 = {
> > 	.get_state = rockchip_pwm_v2_get_state,
> > 	.apply = rockchip_pwm_v2_apply,
> > 	.owner = THIS_MODULE,
> > };
> > 
> > static const struct pwm_ops rockchip_pwm_ops_vop = {
> > 	.get_state = rockchip_pwm_vop_get_state,
> > 	.apply = rockchip_pwm_vop_apply,
> > 	.owner = THIS_MODULE,
> > };
> > 
> > static const struct of_device_id rockchip_pwm_dt_ids[] = {
> > 	{ .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 },
> > 	{ .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 },
> > 	{ .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop },
> > 	{ /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);  
> 
> I think we should keep the data members in the rockchip_pwm_data，like 
> supports_polarity and regs...
> 
> The supports_polarity is needed for of_pwm_n_cells when pwm registered.
> And the other data members is helpful for us to use common code.
> 
> It's okay for just define 3 different pwm_ops (one for each IP), but 
> they are with other data members in the struct of rockchip_pwm_data.
> 

I think we could even get rid of the other fields in rockchip_pwm_data,
but ok, let's do that.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/7] pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops

[David.Wu]

Hi Boris,

在 2017/8/4 15:09, Boris Brezillon 写道:
> On Fri, 4 Aug 2017 10:38:26 +0800
> "David.Wu" <david.wu@rock-chips.com> wrote:
> 
>> Hi Boris，
>>
>> 在 2017/8/2 19:40, Boris Brezillon 写道:
>>> Yep, just define 3 different pwm_ops (one for each IP), each of them
>>> implementing ->apply() and ->get_state() and that's all.
>>>
>>> Something like:
>>>
>>> static const struct pwm_ops rockchip_pwm_ops_v1 = {
>>> 	.get_state = rockchip_pwm_v1_get_state,
>>> 	.apply = rockchip_pwm_v1_apply,
>>> 	.owner = THIS_MODULE,
>>> };
>>>
>>> static const struct pwm_ops rockchip_pwm_ops_v2 = {
>>> 	.get_state = rockchip_pwm_v2_get_state,
>>> 	.apply = rockchip_pwm_v2_apply,
>>> 	.owner = THIS_MODULE,
>>> };
>>>
>>> static const struct pwm_ops rockchip_pwm_ops_vop = {
>>> 	.get_state = rockchip_pwm_vop_get_state,
>>> 	.apply = rockchip_pwm_vop_apply,
>>> 	.owner = THIS_MODULE,
>>> };
>>>
>>> static const struct of_device_id rockchip_pwm_dt_ids[] = {
>>> 	{ .compatible = "rockchip,rk2928-pwm", .data = &rockchip_pwm_ops_v1 },
>>> 	{ .compatible = "rockchip,rk3288-pwm", .data = &rockchip_pwm_ops_v2 },
>>> 	{ .compatible = "rockchip,vop-pwm", .data = &rockchip_pwm_ops_vop },
>>> 	{ /* sentinel */ }
>>> };
>>> MODULE_DEVICE_TABLE(of, rockchip_pwm_dt_ids);
>>
>> I think we should keep the data members in the rockchip_pwm_data，like
>> supports_polarity and regs...
>>
>> The supports_polarity is needed for of_pwm_n_cells when pwm registered.
>> And the other data members is helpful for us to use common code.
>>
>> It's okay for just define 3 different pwm_ops (one for each IP), but
>> they are with other data members in the struct of rockchip_pwm_data.
>>
> 
> I think we could even get rid of the other fields in rockchip_pwm_data,
> but ok, let's do that.

I use the same pwm ops for each IP at V3's patch, but defined 3 
different rockchip_pwm_data for use. I think this might look more clean.

> 
> 
> 

[PATCH v2 5/7] pwm: rockchip: Move the configuration of polarity from rockchip_pwm_set_enable() to rockchip_pwm_config()

[David Wu]

It is usually possible to configure the polarity, cycle and duty all at once,
so that the polarity and cycle and duty should be binding together. Move it
into rockchip_pwm_config(), as well as prepared for the next atomic update
commit.

Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 drivers/pwm/pwm-rockchip.c | 46 ++++++++++++++++++++++++----------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 12e7b4a8..83703e1 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -27,6 +27,7 @@
 #define PWM_DUTY_NEGATIVE	(0 << 3)
 #define PWM_INACTIVE_NEGATIVE	(0 << 4)
 #define PWM_INACTIVE_POSITIVE	(1 << 4)
+#define PWM_POLARITY_MASK	(PWM_DUTY_POSITIVE | PWM_INACTIVE_POSITIVE)
 #define PWM_OUTPUT_LEFT		(0 << 5)
 #define PWM_LP_DISABLE		(0 << 8)
 
@@ -122,12 +123,13 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 	clk_disable(pc->pclk);
 }
 
-static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			       int duty_ns, int period_ns)
+static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state, bool polarity)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	unsigned long period, duty;
 	u64 clk_rate, div;
+	u32 ctrl;
 
 	clk_rate = clk_get_rate(pc->clk);
 
@@ -136,21 +138,32 @@ static void rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * bits, every possible input period can be obtained using the
 	 * default prescaler value for all practical clock rate values.
 	 */
-	div = clk_rate * period_ns;
+	div = clk_rate * state->period;
 	period = DIV_ROUND_CLOSEST_ULL(div,
 				       pc->data->prescaler * NSEC_PER_SEC);
 
-	div = clk_rate * duty_ns;
+	div = clk_rate * state->duty_cycle;
 	duty = DIV_ROUND_CLOSEST_ULL(div, pc->data->prescaler * NSEC_PER_SEC);
 
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
+
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	if (polarity) {
+		ctrl &= ~PWM_POLARITY_MASK;
+		if (state->polarity == PWM_POLARITY_INVERSED)
+			ctrl |= PWM_DUTY_NEGATIVE | PWM_INACTIVE_POSITIVE;
+		else
+			ctrl |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
+	}
+	writel(ctrl, pc->base + pc->data->regs.ctrl);
+
+	return 0;
 }
 
 static int rockchip_pwm_enable(struct pwm_chip *chip,
 			 struct pwm_device *pwm,
 			 bool enable,
-			 enum pwm_polarity polarity,
 			 u32 enable_conf)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
@@ -163,15 +176,6 @@ static int rockchip_pwm_enable(struct pwm_chip *chip,
 			return ret;
 	}
 
-	if (pc->data->supports_polarity) {
-		if (polarity == PWM_POLARITY_INVERSED)
-			enable_conf |= PWM_DUTY_NEGATIVE |
-				       PWM_INACTIVE_POSITIVE;
-		else
-			enable_conf |= PWM_DUTY_POSITIVE |
-				       PWM_INACTIVE_NEGATIVE;
-	}
-
 	val = readl_relaxed(pc->base + pc->data->regs.ctrl);
 
 	if (enable)
@@ -199,17 +203,16 @@ static int rockchip_pwm_apply_v1(struct pwm_chip *chip, struct pwm_device *pwm,
 	enabled = curstate.enabled;
 
 	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
-					  enable_conf);
+		ret = rockchip_pwm_enable(chip, pwm, false, enable_conf);
 		if (ret)
 			return ret;
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	rockchip_pwm_config(chip, pwm, state, false);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-				    state->polarity, enable_conf);
+				    enable_conf);
 
 	return ret;
 }
@@ -227,17 +230,16 @@ static int rockchip_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 	enabled = curstate.enabled;
 
 	if (state->polarity != curstate.polarity && enabled) {
-		ret = rockchip_pwm_enable(chip, pwm, false, state->polarity,
-					  enable_conf);
+		ret = rockchip_pwm_enable(chip, pwm, false, enable_conf);
 		if (ret)
 			return ret;
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
+	rockchip_pwm_config(chip, pwm, state, true);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
-					  state->polarity, enable_conf);
+					  enable_conf);
 
 	return ret;
 }
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RESEND PATCH v2 6/7] pwm: rockchip: Add rk3328 pwm support

[David Wu]

The rk3328 soc supports atomic update, we could lock the configuration
of period and duty at first, after unlock is configured, the period and
duty are effective at the same time.

If the polarity, period and duty need to be configured together,
the way for atomic update is "configure lock and old polarity" ->
"configure period and duty" -> "configure unlock and new polarity".

Signed-off-by: David Wu <david.wu-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
change in v2:
 - 3 different pwm_ops apply(),one for each IP revision.
 .../devicetree/bindings/pwm/pwm-rockchip.txt       |  1 +
 drivers/pwm/pwm-rockchip.c                         | 63 ++++++++++++++++++++--
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
index 2350ef9..152c736 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-rockchip.txt
@@ -4,6 +4,7 @@ Required properties:
  - compatible: should be "rockchip,<name>-pwm"
    "rockchip,rk2928-pwm": found on RK29XX,RK3066 and RK3188 SoCs
    "rockchip,rk3288-pwm": found on RK3288 SoC
+   "rockchip,rk3328-pwm": found on RK3328 SoC
    "rockchip,vop-pwm": found integrated in VOP on RK3288 SoC
  - reg: physical base address and length of the controller's registers
  - clocks: See ../clock/clock-bindings.txt
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 83703e1..838e8fc 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -29,6 +29,7 @@
 #define PWM_INACTIVE_POSITIVE	(1 << 4)
 #define PWM_POLARITY_MASK	(PWM_DUTY_POSITIVE | PWM_INACTIVE_POSITIVE)
 #define PWM_OUTPUT_LEFT		(0 << 5)
+#define PWM_LOCK_EN		(1 << 6)
 #define PWM_LP_DISABLE		(0 << 8)
 
 struct rockchip_pwm_chip {
@@ -124,13 +125,24 @@ static void rockchip_pwm_get_state(struct pwm_chip *chip,
 }
 
 static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			       struct pwm_state *state, bool polarity)
+			       struct pwm_state *state, bool polarity,
+			       bool lock)
 {
 	struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
 	unsigned long period, duty;
 	u64 clk_rate, div;
 	u32 ctrl;
 
+	/*
+	 * Lock the period and duty of previous configuration, then
+	 * change the duty and period, that would not be effective.
+	 */
+	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
+	if (lock) {
+		ctrl |= PWM_LOCK_EN;
+		writel_relaxed(ctrl, pc->base + pc->data->regs.ctrl);
+	}
+
 	clk_rate = clk_get_rate(pc->clk);
 
 	/*
@@ -148,7 +160,6 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	writel(period, pc->base + pc->data->regs.period);
 	writel(duty, pc->base + pc->data->regs.duty);
 
-	ctrl = readl_relaxed(pc->base + pc->data->regs.ctrl);
 	if (polarity) {
 		ctrl &= ~PWM_POLARITY_MASK;
 		if (state->polarity == PWM_POLARITY_INVERSED)
@@ -156,6 +167,15 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		else
 			ctrl |= PWM_DUTY_POSITIVE | PWM_INACTIVE_NEGATIVE;
 	}
+
+	/*
+	 * Unlock and set polarity at the same time,
+	 * the configuration of duty, period and polarity
+	 * would be effective together at next period.
+	 */
+	if (lock)
+		ctrl &= ~PWM_LOCK_EN;
+
 	writel(ctrl, pc->base + pc->data->regs.ctrl);
 
 	return 0;
@@ -209,7 +229,7 @@ static int rockchip_pwm_apply_v1(struct pwm_chip *chip, struct pwm_device *pwm,
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state, false);
+	rockchip_pwm_config(chip, pwm, state, false, false);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
 				    enable_conf);
@@ -236,7 +256,27 @@ static int rockchip_pwm_apply_v2(struct pwm_chip *chip, struct pwm_device *pwm,
 		enabled = false;
 	}
 
-	rockchip_pwm_config(chip, pwm, state, true);
+	rockchip_pwm_config(chip, pwm, state, true, false);
+	if (state->enabled != enabled)
+		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
+					  enable_conf);
+
+	return ret;
+}
+
+static int rockchip_pwm_apply_v3(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	u32 enable_conf = PWM_OUTPUT_LEFT | PWM_LP_DISABLE | PWM_ENABLE |
+		  PWM_CONTINUOUS;
+	struct pwm_state curstate;
+	bool enabled;
+	int ret = 0;
+
+	pwm_get_state(pwm, &curstate);
+	enabled = curstate.enabled;
+
+	rockchip_pwm_config(chip, pwm, state, true, true);
 	if (state->enabled != enabled)
 		ret = rockchip_pwm_enable(chip, pwm, state->enabled,
 					  enable_conf);
@@ -317,9 +357,24 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	.pwm_apply = rockchip_pwm_apply_v2,
 };
 
+static const struct rockchip_pwm_data pwm_data_v3 = {
+	.regs = {
+		.duty = 0x08,
+		.period = 0x04,
+		.cntr = 0x00,
+		.ctrl = 0x0c,
+	},
+	.prescaler = 1,
+	.supports_polarity = true,
+	.ops = &rockchip_pwm_ops,
+	.get_state = rockchip_pwm_get_state_v2,
+	.pwm_apply = rockchip_pwm_apply_v3,
+};
+
 static const struct of_device_id rockchip_pwm_dt_ids[] = {
 	{ .compatible = "rockchip,rk2928-pwm", .data = &pwm_data_v1},
 	{ .compatible = "rockchip,rk3288-pwm", .data = &pwm_data_v2},
+	{ .compatible = "rockchip,rk3328-pwm", .data = &pwm_data_v3},
 	{ .compatible = "rockchip,vop-pwm", .data = &pwm_data_vop},
 	{ /* sentinel */ }
 };
-- 
1.9.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] Add rk3328 pwm support

[David.Wu]

Hi thierry & boris,

Do you have any other suggestion for the patches?
We hope to make them arrive in Linux-v4.14.

在 2017/7/8 12:03, David Wu 写道:
> There are two features of rk3328 pwm module.
>   - PWM APB and function clocks are different.
>   - Add pwm atomic hardware update
> 
> David Wu (7):
>    pwm: rockchip: Add APB and function both clocks support
>    pwm: rockchip: Remove the judge from return value of pwm_config
>    pwm: rockchip: Remove the dumplicate rockchip_pwm_ops ops
>    pwm: rockchip: Use pwm_apply instead of the pwm_enable
>    pwm: rockchip: Move the configuration of polarity from
>      rockchip_pwm_set_enable() to rockchip_pwm_config()
>    pwm: rockchip: Add rk3328 pwm support
>    arm64: dts: rockchip: Add pwm nodes for rk3328
> 
>   .../devicetree/bindings/pwm/pwm-rockchip.txt       |   9 +-
>   arch/arm64/boot/dts/rockchip/rk3328.dtsi           |  45 ++++
>   drivers/pwm/pwm-rockchip.c                         | 275 ++++++++++++++-------
>   3 files changed, 236 insertions(+), 93 deletions(-)
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html