Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1683523 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support David Wu <david.wu@rock-chips.com> - 2017-07-08 06:10 +0200
    Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support Doug Anderson <dianders@chromium.org> - 2017-07-11 19:10 +0200
      Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks  support "David.Wu" <david.wu@rock-chips.com> - 2017-07-12 10:40 +0200
        Re: [PATCH v2 1/7] pwm: rockchip: Add APB and function both clocks support Heiko Stübner <heiko@sntech.de> - 2017-07-12 21:10 +0200

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

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

[toc] | [next] | [standalone]

#1685211

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

[toc] | [prev] | [next] | [standalone]

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

Hi Doug,

在 2017/7/12 1:03, 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.
> 

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
> 
> 
> 

[toc] | [prev] | [next] | [standalone]

#1685982

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@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.
> 
> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web