Re: [PATCH v2 0/3] Add PWM clock support for bcm2835
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] Add PWM clock support for bcm2835
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] Add PWM clock support for bcm2835
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] Add PWM clock support for bcm2835
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] Add PWM clock support for bcm2835
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 @@ }; { - pinctrl-0 = < >; + pinctrl-0 = < >; + + gpiopwm: pwm { + brcm,pins = <18>; + brcm,function = ; + }; }; 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>; }; + + { + 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 = < BCM2835_CLOCK_PWM>; + assigned-clocks = < BCM2835_CLOCK_PWM>; + assigned-clock-rates = <960>; + status = "disabled"; + }; + sdhci: sdhci@7e30 { compatible = "brcm,bcm2835-sdhci"; reg = <0x7e30 0x100>; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 = >dev; pc->chip.ops = _pwm_ops; pc->chip.npwm = 2; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 = >dev; pc->chip.ops = _pwm_ops; pc->chip.npwm = 2; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] Add PWM clock support for bcm2835
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 @@ }; { - pinctrl-0 = < >; + pinctrl-0 = < >; + + gpiopwm: pwm { + brcm,pins = <18>; + brcm,function = ; + }; }; 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>; }; + + { + 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 = < BCM2835_CLOCK_PWM>; + assigned-clocks = < BCM2835_CLOCK_PWM>; + assigned-clock-rates = <960>; + status = "disabled"; + }; + sdhci: sdhci@7e30 { compatible = "brcm,bcm2835-sdhci"; reg = <0x7e30 0x100>; -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/3] Add PWM clock support for bcm2835
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 Remi Pommarel (3): clk: bcm2835: Always round up clock divisor clk: bcm2835: Support for clock parent selection clk: bcm2835: Add PWM clock support drivers/clk/bcm/clk-bcm2835.c | 151 +++- include/dt-bindings/clock/bcm2835.h | 3 +- 2 files changed, 101 insertions(+), 53 deletions(-) -- 2.0.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 0/3] Add PWM clock support for bcm2835
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 Remi Pommarel (3): clk: bcm2835: Always round up clock divisor clk: bcm2835: Support for clock parent selection clk: bcm2835: Add PWM clock support drivers/clk/bcm/clk-bcm2835.c | 151 +++- include/dt-bindings/clock/bcm2835.h | 3 +- 2 files changed, 101 insertions(+), 53 deletions(-) -- 2.0.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/