[0/5] gpio: mvebu: Armada 8K/7K PWM support

Message

Baruch Siach Nov. 18, 2020, 10:30 a.m. UTC

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 'pwm-offset' property to DT binding. '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 '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 (5):
  gpio: mvebu: update Armada XP per-CPU comment
  gpio: mvebu: switch pwm duration registers to regmap
  gpio: mvebu: add pwm support for Armada 8K/7K
  arm64: dts: armada: add pwm offsets for ap/cp gpios
  dt-bindings: ap806: document gpio 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                     | 165 +++++++++++-------
 4 files changed, 124 insertions(+), 62 deletions(-)

Comments

Andrew Lunn Nov. 18, 2020, 10:46 p.m. UTC | #1

On Wed, Nov 18, 2020 at 12:30:41PM +0200, Baruch Siach wrote:
> 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 'pwm-offset' property to DT binding. 'pwm-offset' 

One adds is enough.

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

It was not arbitrary. I decided on KISS. The few devices i've seen
using this have been for a single GPIO/PWN controlled fan. KISS was
sufficient for that, so why make it more complex?

	   Andrew

Baruch Siach Nov. 19, 2020, 5:49 a.m. UTC | #2

Hi Andrew,

Thanks for your review comments.

On Thu, Nov 19 2020, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 12:30:41PM +0200, Baruch Siach wrote:
>> 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 'pwm-offset' property to DT binding. 'pwm-offset' 
>
> One adds is enough.
>
>> 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.
>
> It was not arbitrary. I decided on KISS. The few devices i've seen
> using this have been for a single GPIO/PWN controlled fan. KISS was
> sufficient for that, so why make it more complex?

In saying "arbitrary" I don't mean to say it's not a good choice in the
context of the Linux PWM and GPIO subsystems. But this choice is still
arbitrary from hardware point of view. DT is meant to describe the
hardware. I think that coding the A/B counters allocation choice in DT
is not optimal in terms for hardware description. Both counters are
usable for both GPIO blocks.

I also don't see how this makes anything more complex. The driver code
is already aware of this A/B allocation (GPIO_BLINK_CNT_SELECT_OFF). My
suggestion is to keep this decision in the driver, and leave DT to
describe the hardware. This costs us a single code line (patch #3):

  mvpwm->offset += PWM_BLINK_COUNTER_B_OFF;

Does that make sense?

baruch