gpio: mvebu: Armada 8K/7K PWM support
Subsystems (4)
v6 cover gpio: mvebu: Armada 8K/7K PWM support
Changes in v6:
* Reduce rounding error in the get_state fix (RMK)
Changes in v5:
* Add a fix for get_state
* Fix typo in patch #4 subject line
* Add Rob's review tag on the binding documentation patch
Changes in v4:
* Remove patches that are in LinusW linux-gpio for-next and fixes
* Rename the 'pwm-offset' property to 'marvell,pwm-offset' as suggested by Rob Herring
The original cover letter follows (with DT property name updated).
The gpio-mvebu driver supports the PWM functionality of the GPIO block for earlier Armada variants like XP, 370 and 38x. This series extends support to newer Armada variants that use CP11x and AP80x, like Armada 8K and 7K.
This series adds adds the 'marvell,pwm-offset' property to DT binding. 'marvell,pwm-offset' points to the base of A/B counter registers that determine the PWM period and duty cycle.
The existing PWM DT binding reflects an arbitrary decision to allocate the A counter to the first GPIO block, and B counter to the other one. In attempt to provide better future flexibility, the new 'marvell,pwm-offset' property always points to the base address of both A/B counters. The driver code still allocates the counters in the same way, but this might change in the future with no change to the DT.
Tested AP806 and CP110 (both) on Armada 8040 based system.
Baruch Siach (4):
gpio: mvebu: fix pwm get_state period calculation
gpio: mvebu: add pwm support for Armada 8K/7K
arm64: dts: armada: add pwm offsets for ap/cp gpios
dt-bindings: ap806: document gpio marvell,pwm-offset property
.../arm/marvell/ap80x-system-controller.txt | 8 ++ arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 3 + arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 10 ++ drivers/gpio/gpio-mvebu.c | 120 +++++++++++------- 4 files changed, 97 insertions(+), 44 deletions(-)
Signature · 1 line
2.29.2
PATCH v6 1/4 gpio: mvebu: fix pwm get_state period calculation
The period is the sum of on and off values.
---
v6: divide (on + off) sum to reduce rounding error (RMK)
--- drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
drivers/gpio/gpio-mvebu.c +8 -11
@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,else state->duty_cycle =1 ; + val = (unsigned long long ) u;/* on duration */ regmap_read (mvpwm->regs,mvebu_pwmreg_blink_off_duration (mvpwm), &u); - val = (unsigned long long ) u *NSEC_PER_SEC ; + val += (unsigned long long ) u;/* period = on + off duration */ + val *=NSEC_PER_SEC ;do_div (val, mvpwm->clk_rate); -if (val < state->duty_cycle) { +if (val >UINT_MAX ) + state->period =UINT_MAX ; +else if (val) + state->period = val; +else state->period =1 ; - }else { - val -= state->duty_cycle; -if (val >UINT_MAX ) - state->period =UINT_MAX ; -else if (val) - state->period = val; -else - state->period =1 ; - }regmap_read (mvchip->regs,GPIO_BLINK_EN_OFF + mvchip->offset, &u);if (u)
Signature · 1 line
2.29.2
Re: [PATCH v6 1/4] gpio: mvebu: fix pwm get_state period calculation
Quoted 18 lines · Baruch Siach
The period is the sum of on and off values.
Reported-by Russell King <[email protected]>Fixes 757642f9a584e ("gpio: mvebu: Add limited PWM support")Signed-off-by Baruch Siach <[email protected]>---
v6: divide (on + off) sum to reduce rounding error (RMK)--- drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)drivers/gpio/gpio-mvebu.c +0
-0@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,else state->duty_cycle =1 ;
drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 685
Quoted 10 lines
drivers/gpio/gpio-mvebu.c +5
-2+ val = (unsigned long long ) u;/* on duration */ regmap_read (mvpwm->regs,mvebu_pwmreg_blink_off_duration (mvpwm), &u); - val = (unsigned long long ) u *NSEC_PER_SEC ; + val += (unsigned long long ) u;/* period = on + off duration */ + val *=NSEC_PER_SEC ;do_div (val, mvpwm->clk_rate); -if (val < state->duty_cycle) { +if (val >UINT_MAX ) + state->period =UINT_MAX ;
state->period is an u64, so there is no reason to not use values greater than UINT_MAX.
drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 689
Quoted 4 lines
drivers/gpio/gpio-mvebu.c +3
-0+else if (val) + state->period = val; +else state->period =1 ;
This case assigning 1 looks strange. An explanation in a comment would be great. I wonder if this is a hardware property or if it is only used to not report 0 in case that mvpwm->clk_rate is high.
I found a few further shortcommings in the mvebu_pwm implementation while looking through it:
a) The rounding problem that RMK found is also present in .apply
There we have:
val = clk_rate * (period - duty_cycle) / NSEC_PER_SEC
while
val = clk_rate * period / NSEC_PER_SEC - on
would be more exact.
b) To make
pwm_get_state(pwm, &state);
pwm_apply_state(pwm, &state);
idempotent .get_state should round up the division results.c) .apply also has a check for val being zero and configures at least 1 cycle for the on and off intervals. Is this a hardware imposed limitation?
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Re: [PATCH v6 1/4] gpio: mvebu: fix pwm get_state period calculation
Hi Uwe,
Thanks for your review comments.
Quoted 20 lines · Uwe Kleine-König
Quoted 19 lines · Baruch Siach
The period is the sum of on and off values.
Reported-by Russell King <[email protected]>Fixes 757642f9a584e ("gpio: mvebu: Add limited PWM support")Signed-off-by Baruch Siach <[email protected]>---
v6: divide (on + off) sum to reduce rounding error (RMK)--- drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)drivers/gpio/gpio-mvebu.c +0
-0@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,else state->duty_cycle =1 ;
Quoted 12 lines
drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 685
Quoted 9 lines
drivers/gpio/gpio-mvebu.c +5
-2+ val = (unsigned long long ) u;/* on duration */ regmap_read (mvpwm->regs,mvebu_pwmreg_blink_off_duration (mvpwm), &u); - val = (unsigned long long ) u *NSEC_PER_SEC ; + val += (unsigned long long ) u;/* period = on + off duration */ + val *=NSEC_PER_SEC ;do_div (val, mvpwm->clk_rate); -if (val < state->duty_cycle) { +if (val >UINT_MAX ) + state->period =UINT_MAX ;
state->periodis an u64, so there is no reason to not use values greater thanUINT_MAX.
I'll post a patch for that.
Quoted 8 lines
drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 689
Quoted 4 lines
drivers/gpio/gpio-mvebu.c +3
-0+else if (val) + state->period = val; +else state->period =1 ;This case assigning 1 looks strange. An explanation in a comment would be great. I wonder if this is a hardware property or if it is only used to not report 0 in case that
mvpwm->clk_rateis high.
I guess that this is because 0 period does not make sense for pwm. It is like a zero divisor. What is the common behavior?
Quoted 7 lines
I found a few further shortcommings in the
mvebu_pwmimplementation while looking through it:a) The rounding problem that RMK found is also present in .apply
There we have:
Quoted 7 lines
val =
clk_rate* (period -duty_cycle) /NSEC_PER_SECwhile
val =
clk_rate* period /NSEC_PER_SEC- onwould be more exact.
I'll post a patch for that.
Quoted 6 lines
b) To make
pwm_get_state(pwm, &state); pwm_apply_state(pwm, &state); idempotent .get_state should round up the division results.
I'll post a patch for that as well.
Quoted 3 lines
c) .apply also has a check for val being zero and configures at least 1 cycle for the on and off intervals. Is this a hardware imposed limitation?
Not sure what was the original intention. Maybe Andrew can explain.
On my hardware (Armada 8040), zero 'on' value does not work as expected. There is a blink once in a long while. Maybe this is the reason?
As I understand, all these issues should not block this patch, right?
BTW, the key you used to sign your message is expired since 2020-01-10 on the key server I use (keys.gnupg.net). Where can I find your updated key?
Thanks,
baruch
Signature · 3 lines
~. .~ Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
- [email protected] - tel: +972.52.368.4656, http://www.tkos.co.il -Re: [PATCH v6 1/4] gpio: mvebu: fix pwm get_state period calculation
Quoted 49 lines · Baruch Siach
Hi Uwe,
Thanks for your review comments.
Quoted 20 lines · Uwe Kleine-König
Quoted 19 lines · Baruch Siach
The period is the sum of on and off values.
Reported-by Russell King <[email protected]>Fixes 757642f9a584e ("gpio: mvebu: Add limited PWM support")Signed-off-by Baruch Siach <[email protected]>---
v6: divide (on + off) sum to reduce rounding error (RMK)--- drivers/gpio/gpio-mvebu.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)drivers/gpio/gpio-mvebu.c +0
-0@@ -676,20 +676,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,else state->duty_cycle =1 ;Quoted 12 lines
drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 685
Quoted 9 lines
drivers/gpio/gpio-mvebu.c +5
-2+ val = (unsigned long long ) u;/* on duration */ regmap_read (mvpwm->regs,mvebu_pwmreg_blink_off_duration (mvpwm), &u); - val = (unsigned long long ) u *NSEC_PER_SEC ; + val += (unsigned long long ) u;/* period = on + off duration */ + val *=NSEC_PER_SEC ;do_div (val, mvpwm->clk_rate); -if (val < state->duty_cycle) { +if (val >UINT_MAX ) + state->period =UINT_MAX ;
state->periodis an u64, so there is no reason to not use values greater thanUINT_MAX.I'll post a patch for that.
Quoted 8 lines
drivers/gpio/gpio-mvebu.c static void mvebu_pwm_get_state(struct pwm_chip *chip, · line 689
Quoted 4 lines
drivers/gpio/gpio-mvebu.c +3
-0+else if (val) + state->period = val; +else state->period =1 ;This case assigning 1 looks strange. An explanation in a comment would be great. I wonder if this is a hardware property or if it is only used to not report 0 in case that
mvpwm->clk_rateis high.
Quoted 2 lines
I guess that this is because 0 period does not make sense for pwm. It is like a zero divisor. What is the common behavior?
It depends on how the hardware behaves in this case. One measure is to use round-up (in .get_state) for the divisions -- as I already pointed out. Then the problem only triggers if both on and off registers are zero.
Quoted 30 lines
Quoted 7 lines
I found a few further shortcommings in the
mvebu_pwmimplementation while looking through it:a) The rounding problem that RMK found is also present in .apply
There we have:
Quoted 7 lines
val =
clk_rate* (period -duty_cycle) /NSEC_PER_SECwhile
val =
clk_rate* period /NSEC_PER_SEC- onwould be more exact.
I'll post a patch for that.
Quoted 6 lines
b) To make
pwm_get_state(pwm, &state); pwm_apply_state(pwm, &state); idempotent .get_state should round up the division results.I'll post a patch for that as well.
Quoted 3 lines
c) .apply also has a check for val being zero and configures at least 1 cycle for the on and off intervals. Is this a hardware imposed limitation?
Quoted 5 lines
Not sure what was the original intention. Maybe Andrew can explain.
On my hardware (Armada 8040), zero 'on' value does not work as expected. There is a blink once in a long while. Maybe this is the reason?
It would be good to understand and document this. Hardware PWMs not supporting a zero duty cycle are always a surprise for me and probably also others.
Quoted 1 line
As I understand, all these issues should not block this patch, right?
Yes, having this patch in a series fixing all the stuff I pointed out would still be very welcome :-)
Quoted 3 lines
BTW, the key you used to sign your message is expired since 2020-01-10 on the key server I use (keys.gnupg.net). Where can I find your updated key?
Hmm, I thought keyservers are out of fashion. Anyhow, I updated my key there. There is no WKD for @pengutronix.de addresses, but
gpg --locate-external-keys [email protected]
should work just fine, too.
Best regards
Uwe
Signature · 2 lines
Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
PATCH v6 2/4 gpio: mvebu: add pwm support for Armada 8K/7K
In reply to Baruch SiachUse the marvell,pwm-offset DT property to store the location of PWM signal duration registers.
Since we have more than two GPIO chips per system, we can't use the alias id to differentiate between them. Use the offset value for that.
---
v3: Split mvebu_pwm_probe() move out of this patch into a separate fix
(Andrew Lunn)
--- drivers/gpio/gpio-mvebu.c | 101 +++++++++++++++++++++++++------------- 1 file changed, 68 insertions(+), 33 deletions(-)
drivers/gpio/gpio-mvebu.c +68 -33
@@ -70,7 +70,12 @@*/ #define PWM_BLINK_ON_DURATION_OFF 0x0#define PWM_BLINK_OFF_DURATION_OFF 0x4 +#define PWM_BLINK_COUNTER_B_OFF 0x8 +/* Armada 8k variant gpios register offsets */ +#define AP80X_GPIO0_OFF_A8K 0x1040 +#define CP11X_GPIO0_OFF_A8K 0x100 +#define CP11X_GPIO1_OFF_A8K 0x140/* The MV78200 has per-CPU registers for edge mask and level mask */ #define GPIO_EDGE_MASK_MV78200_OFF (cpu) ((cpu) ? 0x30 : 0x18)
@@ -93,6 +98,7 @@struct mvebu_pwm {struct regmap *regs; +u32 offset;unsigned long clk_rate;struct gpio_desc *gpiod;struct pwm_chip chip;
@@ -283,12 +289,12 @@ mvebu_gpio_write_level_mask(struct mvebu_gpio_chip *mvchip, u32 val)*/ static unsigned int mvebu_pwmreg_blink_on_duration (struct mvebu_pwm *mvpwm) { -return PWM_BLINK_ON_DURATION_OFF ; +return mvpwm->offset +PWM_BLINK_ON_DURATION_OFF ; }static unsigned int mvebu_pwmreg_blink_off_duration (struct mvebu_pwm *mvpwm) { -return PWM_BLINK_OFF_DURATION_OFF ; +return mvpwm->offset +PWM_BLINK_OFF_DURATION_OFF ; }/*
@@ -778,51 +784,80 @@ static int mvebu_pwm_probe(struct platform_device *pdev,struct device *dev = &pdev->dev;struct mvebu_pwm *mvpwm;void __iomem *base; +u32 offset;u32 set; -if (!of_device_is_compatible (mvchip->chip.of_node, -"marvell,armada-370-gpio" )) -return 0 ; - -/* -* There are only two sets of PWM configuration registers for -* all the GPIO lines on those SoCs which this driver reserves -* for the first two GPIO chips. So if the resource is missing -* we can't treat it as an error. -*/ -if (!platform_get_resource_byname (pdev,IORESOURCE_MEM ,"pwm" )) +if (of_device_is_compatible (mvchip->chip.of_node, +"marvell,armada-370-gpio" )) { +/* +* There are only two sets of PWM configuration registers for +* all the GPIO lines on those SoCs which this driver reserves +* for the first two GPIO chips. So if the resource is missing +* we can't treat it as an error. +*/ +if (!platform_get_resource_byname (pdev,IORESOURCE_MEM ,"pwm" )) +return 0 ; + offset =0 ; + }else if (mvchip->soc_variant ==MVEBU_GPIO_SOC_VARIANT_A8K ) { +int ret =of_property_read_u32 (dev->of_node, +"marvell,pwm-offset" , &offset); +if (ret <0 ) +return 0 ; + }else {return 0 ; + }if (IS_ERR (mvchip->clk))return PTR_ERR (mvchip->clk); -/* -* Use set A for lines of GPIO chip with id 0, B for GPIO chip -* with id 1. Don't allow further GPIO chips to be used for PWM. -*/ -if (id ==0 ) - set =0 ; -else if (id ==1 ) - set =U32_MAX ; -else -return -EINVAL ; -regmap_write (mvchip->regs, -GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set); - mvpwm =devm_kzalloc (dev,sizeof (struct mvebu_pwm ),GFP_KERNEL );if (!mvpwm)return -ENOMEM ; mvchip->mvpwm = mvpwm; mvpwm->mvchip = mvchip; + mvpwm->offset = offset; + +if (mvchip->soc_variant ==MVEBU_GPIO_SOC_VARIANT_A8K ) { + mvpwm->regs = mvchip->regs; + +switch (mvchip->offset) { +case AP80X_GPIO0_OFF_A8K : +case CP11X_GPIO0_OFF_A8K : +/* Blink counter A */ + set =0 ; +break ; +case CP11X_GPIO1_OFF_A8K : +/* Blink counter B */ + set =U32_MAX ; + mvpwm->offset +=PWM_BLINK_COUNTER_B_OFF ; +break ; +default : +return -EINVAL ; + } + }else { + base =devm_platform_ioremap_resource_byname (pdev,"pwm" ); +if (IS_ERR (base)) +return PTR_ERR (base); - base =devm_platform_ioremap_resource_byname (pdev,"pwm" ); -if (IS_ERR (base)) -return PTR_ERR (base); + mvpwm->regs =devm_regmap_init_mmio (&pdev->dev, base, + &mvebu_gpio_regmap_config); +if (IS_ERR (mvpwm->regs)) +return PTR_ERR (mvpwm->regs); - mvpwm->regs =devm_regmap_init_mmio (&pdev->dev, base, - &mvebu_gpio_regmap_config); -if (IS_ERR (mvpwm->regs)) -return PTR_ERR (mvpwm->regs); +/* +* Use set A for lines of GPIO chip with id 0, B for GPIO chip +* with id 1. Don't allow further GPIO chips to be used for PWM. +*/ +if (id ==0 ) + set =0 ; +else if (id ==1 ) + set =U32_MAX ; +else +return -EINVAL ; + } + +regmap_write (mvchip->regs, +GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set); mvpwm->clk_rate =clk_get_rate (mvchip->clk);if (!mvpwm->clk_rate) {
Signature · 1 line
2.29.2
PATCH v6 3/4 arm64: dts: armada: add pwm offsets for ap/cp gpios
In reply to Baruch SiachThe 'marvell,pwm-offset' property of both GPIO blocks (per CP component) point to the same counter registers offset. The driver will decide how to use counters A/B.
This is different from the convention of pwm on earlier Armada series (370/38x). On those systems the assignment of A/B counters to GPIO blocks is coded in both DT and the driver. The actual behaviour of the current driver on Armada 8K/7K is the same as earlier systems.
Add also clock properties for base pwm frequency reference.
--- arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 3 +++ arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 10 ++++++++++ 2 files changed, 13 insertions(+)
arch/arm64/boot/dts/marvell/armada-ap80x.dtsi +3 -0
@@ -281,6 +281,9 @@ ap_gpio: gpio@1040 { gpio-controller; #gpio-cells = <2>; gpio-ranges = <&ap_pinctrl 0 0 20>; + marvell,pwm-offset = <0x10c0>; + #pwm-cells = <2>; + clocks = <&ap_clk 3>; }; };
arch/arm64/boot/dts/marvell/armada-cp11x.dtsi +10 -0
@@ -234,12 +234,17 @@ CP11X_LABEL(gpio1): gpio@100 { gpio-controller; #gpio-cells = <2>; gpio-ranges = <&CP11X_LABEL(pinctrl) 0 0 32>; + marvell,pwm-offset = <0x1f0>; + #pwm-cells = <2>; interrupt-controller; interrupts = <86 IRQ_TYPE_LEVEL_HIGH>, <85 IRQ_TYPE_LEVEL_HIGH>, <84 IRQ_TYPE_LEVEL_HIGH>, <83 IRQ_TYPE_LEVEL_HIGH>; #interrupt-cells = <2>; + clock-names = "core", "axi"; + clocks = <&CP11X_LABEL(clk) 1 21>, + <&CP11X_LABEL(clk) 1 17>; status = "disabled"; };
@@ -250,12 +255,17 @@ CP11X_LABEL(gpio2): gpio@140 { gpio-controller; #gpio-cells = <2>; gpio-ranges = <&CP11X_LABEL(pinctrl) 0 32 31>; + marvell,pwm-offset = <0x1f0>; + #pwm-cells = <2>; interrupt-controller; interrupts = <82 IRQ_TYPE_LEVEL_HIGH>, <81 IRQ_TYPE_LEVEL_HIGH>, <80 IRQ_TYPE_LEVEL_HIGH>, <79 IRQ_TYPE_LEVEL_HIGH>; #interrupt-cells = <2>; + clock-names = "core", "axi"; + clocks = <&CP11X_LABEL(clk) 1 21>, + <&CP11X_LABEL(clk) 1 17>; status = "disabled"; }; };
Signature · 1 line
2.29.2
PATCH v6 4/4 dt-bindings: ap806: document gpio marvell, pwm-offset property
In reply to Baruch SiachUpdate the example as well. Add the '#pwm-cells' and 'clocks' properties for a complete working example.
--- .../bindings/arm/marvell/ap80x-system-controller.txt | 8 ++++++++ 1 file changed, 8 insertions(+)
Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt +8 -0
@@ -80,6 +80,11 @@ Required properties: - offset: offset address inside the syscon block +Optional properties: + +- marvell,pwm-offset: offset address of PWM duration control registers inside + the syscon block + Example: ap_syscon: system-controller@6f4000 { compatible = "syscon", "simple-mfd";
@@ -101,6 +106,9 @@ ap_syscon: system-controller@6f4000 { gpio-controller; #gpio-cells = <2>; gpio-ranges = <&ap_pinctrl 0 0 19>; + marvell,pwm-offset = <0x10c0>; + #pwm-cells = <2>; + clocks = <&ap_clk 3>; }; };
Signature · 1 line
2.29.2