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

Groups > linux.kernel > #1279222 > unrolled thread

Re: [PATCH v2 0/3] Add PWM clock support for bcm2835

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

  Re: [PATCH v2 0/3] Add PWM clock support for bcm2835 Stefan Wahren <info@lategoodbye.de> - 2015-11-28 22:00 +0100
    Re: [PATCH v2 0/3] Add PWM clock support for bcm2835 Remi Pommarel <repk@triplefau.lt> - 2015-11-29 01:40 +0100
      Re: [PATCH v2 0/3] Add PWM clock support for bcm2835 Stefan Wahren <info@lategoodbye.de> - 2015-11-29 22:30 +0100
        Re: [PATCH v2 0/3] Add PWM clock support for bcm2835 Remi Pommarel <repk@triplefau.lt> - 2015-11-29 23:30 +0100

#1279222 — Re: [PATCH v2 0/3] Add PWM clock support for bcm2835

Hi Remi,
hi Eric,

Am 11.11.2015 um 15:22 schrieb Remi Pommarel:
> Hi,
>
> This patchset adds support for pwm clock. At boot, this clock does not have a
> default parent nor a default rate set. Thus we should be able to change its
> parent to get this clock working. The current clock implementation is using a
> mux to select the parent, but these clocks need to add a password (0x5a) in
> higher register bits when changing parent. So a generic mux cannot be used
> here.
>
> The two first patches fix the clock parent selection, while the second one is
> actually adding the pwm clock registration.
>
> Changes since v1:
> 	- determine_rate now based its parent selection upon divided rate
> 	  instead of the parent one
> 	- bcm2835_clock_choose_div has been modified to produce an avarage rate
> 	  lower or equal to the requested one
> 	- devicetree modifications have removed to be send in another patch
>

i applied the series including the devicetree modification, but it 
doesn't work for me.

First of all i get an ugly division by zero warning from the pwm driver. 
The pwm driver still assume a fixed clock and doesn't handle the error 
cases of clk_get_rate(). I attached a patch at the end.

The reason in my case why clk_get_rate() returns zero is that the pwm 
clock is orphan ( pwm is listed under 
/sys/kernel/debug/clk_orphan_summary ).

My suspicion is it has something to do with the clock manager driver.
The bcm2835_clock_per_parents contains only 8 entries. But according to
BCM2835-ARM-Peripherals.pdf [1] CM_GP0CTL SRC page 107 has 16 entries. 
The upper 8 entries are all mapped to GND. It looks to me that the 
driver doesn't take care of this and so the pwm clock isn't able to 
determine it's parent.

Best regards
Stefan

[1] - 
https://www.raspberrypi.org/wp-content/uploads/2012/02/BCM2835-ARM-Peripherals.pdf

---------------------------------->8------------------------------
diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index b4c7f95..49e28df 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -29,7 +29,6 @@
  struct bcm2835_pwm {
  	struct pwm_chip chip;
  	struct device *dev;
-	unsigned long scaler;
  	void __iomem *base;
  	struct clk *clk;
  };
@@ -66,6 +65,15 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, 
struct pwm_device *pwm,
  			      int duty_ns, int period_ns)
  {
  	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
+	unsigned long rate = clk_get_rate(pc->clk);
+	unsigned long scaler;
+
+	if (rate <= 0) {
+		dev_err(pc->dev, "Invalid clock rate: %ld\n", rate);
+		return -EINVAL;
+	}
+
+	scaler = NSEC_PER_SEC / rate;

  	if (period_ns <= MIN_PERIOD) {
  		dev_err(pc->dev, "period %d not supported, minimum %d\n",
@@ -73,8 +81,8 @@ static int bcm2835_pwm_config(struct pwm_chip *chip, 
struct pwm_device *pwm,
  		return -EINVAL;
  	}

-	writel(duty_ns / pc->scaler, pc->base + DUTY(pwm->hwpwm));
-	writel(period_ns / pc->scaler, pc->base + PERIOD(pwm->hwpwm));
+	writel(duty_ns / scaler, pc->base + DUTY(pwm->hwpwm));
+	writel(period_ns / scaler, pc->base + PERIOD(pwm->hwpwm));

  	return 0;
  }
@@ -156,8 +164,6 @@ static int bcm2835_pwm_probe(struct platform_device 
*pdev)
  	if (ret)
  		return ret;

-	pc->scaler = NSEC_PER_SEC / clk_get_rate(pc->clk);
-
  	pc->chip.dev = &pdev->dev;
  	pc->chip.ops = &bcm2835_pwm_ops;
  	pc->chip.npwm = 2;



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1279252

Hi Stefan,

On Sat, Nov 28, 2015 at 09:52:07PM +0100, Stefan Wahren wrote:
> i applied the series including the devicetree modification, but it
> doesn't work for me.
> 
> First of all i get an ugly division by zero warning from the pwm
> driver. The pwm driver still assume a fixed clock and doesn't handle
> the error cases of clk_get_rate(). I attached a patch at the end.

Yes the devicetree patch from patchset version one does not work with
this version. I haven't sent the modified devicetree because Eric said
it is better to send it in a separate patchset. If you want to test it I
attached the working devicetree patch at the end.

But, yes, that would be nice if pwm driver was protected from this
division by zero.

> 
> The reason in my case why clk_get_rate() returns zero is that the
> pwm clock is orphan ( pwm is listed under
> /sys/kernel/debug/clk_orphan_summary ).
> 
> My suspicion is it has something to do with the clock manager driver.
> The bcm2835_clock_per_parents contains only 8 entries. But according to
> BCM2835-ARM-Peripherals.pdf [1] CM_GP0CTL SRC page 107 has 16
> entries. The upper 8 entries are all mapped to GND. It looks to me
> that the driver doesn't take care of this and so the pwm clock isn't
> able to determine it's parent.
> 

In fact, default parent for pwm after boot up is GND (CM_GP0CTL SRC ==
0). Which means that the default pwm clock rate is 0. The clock appears
to be orphan because in the bcm2835 clock driver, the GND clock is not
registered.

So, IMHO, we have to set the default pwm rate from the devicetree, using
assigned-clock-rates. That what does the following dts patch.

This patch also set the gpio pin 18 to proper alternate function in order
to be able to get pwm output from this gpio.

Thanks

-- 
Remi

---------------------------------->8------------------------------
diff --git a/arch/arm/boot/dts/bcm2835-rpi-b.dts b/arch/arm/boot/dts/bcm2835-rpi-b.dts
index ff6b2d1..478aa79 100644
--- a/arch/arm/boot/dts/bcm2835-rpi-b.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b.dts
@@ -13,5 +13,10 @@
 };
 
 &gpio {
-       pinctrl-0 = <&gpioout &alt0 &alt3>;
+       pinctrl-0 = <&gpioout &alt0 &alt3 &gpiopwm>;
+
+       gpiopwm: pwm {
+               brcm,pins = <18>;
+               brcm,function = <BCM2835_FSEL_ALT5>;
+       };
 };
diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index 3572f03..55801e0 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -60,3 +60,7 @@
        status = "okay";
        bus-width = <4>;
 };
+
+&pwm {
+       status = "okay";
+};
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index aef64de..567bd35 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -149,6 +149,15 @@
                        status = "disabled";
                };
 
+               pwm: pwm@7e20c000 {
+                       compatible = "brcm,bcm2835-pwm";
+                       reg = <0x7e20c000 0x28>;
+                       clocks = <&clocks BCM2835_CLOCK_PWM>;
+                       assigned-clocks = <&clocks BCM2835_CLOCK_PWM>;
+                       assigned-clock-rates = <9600000>;
+                       status = "disabled";
+               };
+
                sdhci: sdhci@7e300000 {
                        compatible = "brcm,bcm2835-sdhci";
                        reg = <0x7e300000 0x100>;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1279421

Hi Remi,

Am 29.11.2015 um 01:31 schrieb Remi Pommarel:
> Hi Stefan,
>
> On Sat, Nov 28, 2015 at 09:52:07PM +0100, Stefan Wahren wrote:
>> i applied the series including the devicetree modification, but it
>> doesn't work for me.
>>
>> First of all i get an ugly division by zero warning from the pwm
>> driver. The pwm driver still assume a fixed clock and doesn't handle
>> the error cases of clk_get_rate(). I attached a patch at the end.
>
> Yes the devicetree patch from patchset version one does not work with
> this version.

thanks. I successfully tested the pwm with the led pwm driver.

> I haven't sent the modified devicetree because Eric said
> it is better to send it in a separate patchset. If you want to test it I
> attached the working devicetree patch at the end.

I don't think that he said that. He wanted you to send the devicetree 
changes as a separate patch. So it should be okay if it's part of the 
same patchset.

>
> But, yes, that would be nice if pwm driver was protected from this
> division by zero.

I will create a proper patch.

>
>>
>> The reason in my case why clk_get_rate() returns zero is that the
>> pwm clock is orphan ( pwm is listed under
>> /sys/kernel/debug/clk_orphan_summary ).
>>
>> My suspicion is it has something to do with the clock manager driver.
>> The bcm2835_clock_per_parents contains only 8 entries. But according to
>> BCM2835-ARM-Peripherals.pdf [1] CM_GP0CTL SRC page 107 has 16
>> entries. The upper 8 entries are all mapped to GND. It looks to me
>> that the driver doesn't take care of this and so the pwm clock isn't
>> able to determine it's parent.
>>
>
> In fact, default parent for pwm after boot up is GND (CM_GP0CTL SRC ==
> 0). Which means that the default pwm clock rate is 0. The clock appears
> to be orphan because in the bcm2835 clock driver, the GND clock is not
> registered.

Thanks for the explanation.

Best regards
Stefan

>
> So, IMHO, we have to set the default pwm rate from the devicetree, using
> assigned-clock-rates. That what does the following dts patch.
>
> This patch also set the gpio pin 18 to proper alternate function in order
> to be able to get pwm output from this gpio.
>
> Thanks
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1279471

On Sun, Nov 29, 2015 at 10:22:40PM +0100, Stefan Wahren wrote:
> Hi Remi,
> 
> Am 29.11.2015 um 01:31 schrieb Remi Pommarel:
> >Hi Stefan,
> >
> >On Sat, Nov 28, 2015 at 09:52:07PM +0100, Stefan Wahren wrote:
> >>i applied the series including the devicetree modification, but it
> >>doesn't work for me.
> >>
> >>First of all i get an ugly division by zero warning from the pwm
> >>driver. The pwm driver still assume a fixed clock and doesn't handle
> >>the error cases of clk_get_rate(). I attached a patch at the end.
> >
> >Yes the devicetree patch from patchset version one does not work with
> >this version.
> 
> thanks. I successfully tested the pwm with the led pwm driver.
> 

Good news, thank you.

> >I haven't sent the modified devicetree because Eric said
> >it is better to send it in a separate patchset. If you want to test it I
> >attached the working devicetree patch at the end.
> 
> I don't think that he said that. He wanted you to send the
> devicetree changes as a separate patch. So it should be okay if it's
> part of the same patchset.
> 

I could have misunderstood him, sorry about that. I will send a
devicetree separated patch with the next version if Eric agrees with the
GENMASK logic used in my first patch.

Best Regards,

-- 
Remi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web