[v7,3/3] dt-bindings: ap806: document gpio marvell,pwm-offset property

Update the example as well. Add the '#pwm-cells' and 'clocks' properties
for a complete working example.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Baruch Siach <baruch@tkos.co.il>
---
 .../bindings/arm/marvell/ap80x-system-controller.txt      | 8 ++++++++
 1 file changed, 8 insertions(+)

Hi Baruch,

this caught my eye:

On Mon, Jan 11, 2021 at 12:47 PM Baruch Siach <baruch@tkos.co.il> wrote:

> Update the example as well. Add the '#pwm-cells' and 'clocks' properties
> for a complete working example.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Baruch Siach <baruch@tkos.co.il>

(...)
> +Optional properties:
> +
> +- marvell,pwm-offset: offset address of PWM duration control registers inside
> +  the syscon block
(...)
>  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>;

This seems to be one of those cases where we start to encode things related
to the hardware variant into the device tree.

Is this just documenting ABI that was introduced in the past and we can not
do anything about now? In that case it is OK I suppose.

For a new binding we would certainly require that the system controller
provide a specific tertiary compatible string for this, lest we disguise
the not-so-simple system controller as "simple-mfd" so:

compatible = "syscon", "simple-mfd", "my-silicon-id";

Then detect the PWM offset by using
if(of_device_is_compatibe(np, "my-silicon-id"))
in the code rather than parsing any marvell,pwm-offset property.

Yours,
Linus Walleij

On Tue, Jan 12, 2021 at 09:49:16AM +0100, Linus Walleij wrote:
> Hi Baruch,
> 
> this caught my eye:
> 
> On Mon, Jan 11, 2021 at 12:47 PM Baruch Siach <baruch@tkos.co.il> wrote:
> 
> > Update the example as well. Add the '#pwm-cells' and 'clocks' properties
> > for a complete working example.
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Baruch Siach <baruch@tkos.co.il>
> 
> (...)
> > +Optional properties:
> > +
> > +- marvell,pwm-offset: offset address of PWM duration control registers inside
> > +  the syscon block
> (...)
> >  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>;
> 
> This seems to be one of those cases where we start to encode things related
> to the hardware variant into the device tree.
> 
> Is this just documenting ABI that was introduced in the past and we can not
> do anything about now? In that case it is OK I suppose.
> 
> For a new binding we would certainly require that the system controller
> provide a specific tertiary compatible string for this, lest we disguise
> the not-so-simple system controller as "simple-mfd" so:
> 
> compatible = "syscon", "simple-mfd", "my-silicon-id";
> 
> Then detect the PWM offset by using
> if(of_device_is_compatibe(np, "my-silicon-id"))
> in the code rather than parsing any marvell,pwm-offset property.

I think it would be a good idea to describe the hardware more fully.
For the CP110 and AP80x dies on Armada 8040:

CP110	AP80x
Offset	Offset
00/40	5040	Data Out
04/44	5044	Data Out Enable
08/48	5048	Blink Enable
0c/4c	504c	Data In polarity
10/50	5050	Data In
14/54	5054	IRQ Cause
18/58	5058	IRQ Mask
1c/5c	505c	IRQ Level mask
20/60	5060	Blink Counter Select
28/68	5068	Control Set
2c/6c	506c	Control Clear
30/70	5070	Data Out Set
34/74	5074	Data Out Clear
f0	50c0	Blink Counter A ON duration
f4	50c4	Blink Counter A OFF duration
f8	50c8	Blink Counter B ON duration
fc	50cc	Blink Counter B OFF duration

We identify both of these using a compatible of "marvell,armada-8k-gpio"
which really only describes the first 64 bytes of the register set:

			ap_gpio: gpio@1040 {
				compatible = "marvell,armada-8k-gpio";
				offset = <0x1040>;
				...
			};

			CP11X_LABEL(gpio1): gpio@100 {
				compatible = "marvell,armada-8k-gpio";
				offset = <0x100>;
				...
			};

			CP11X_LABEL(gpio2): gpio@140 {
				compatible = "marvell,armada-8k-gpio";
				offset = <0x140>;
				...
			};

Note that on the CP11x dies, there are two GPIO controllers sharing the
same set of blink counter registers - one at offset 0 the other at
offset 0x40.

However, the pwm-offset is the offset in the regmap of the parent node.

It is possible to use a more specific compatible that would describe
the PWM offset for the CP11x and AP806 (which would need two different
ones) but that starts getting messy when you consider that we already
describe an offset in regmap for the first 64 registers, and encoding
the blink register offset in a compatible would partially end up
encoding the "offset" we already have.

In any case, these offsets are a function of how it was originally
chosen to describe the hardware in DT, rather than anything about the
hardware itself. The choice to use a syscon/regmap is purely an
implementation decision rather than something from the hardware, so
this DT description is already based around describing what is required
for the Linux implementation, rather than purely being a hardware
description.

On Tue, Jan 12, 2021 at 11:36 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:

> In any case, these offsets are a function of how it was originally
> chosen to describe the hardware in DT, rather than anything about the
> hardware itself. The choice to use a syscon/regmap is purely an
> implementation decision rather than something from the hardware, so
> this DT description is already based around describing what is required
> for the Linux implementation, rather than purely being a hardware
> description.

OK I will not complain about it then, this kind of thing happens
sometimes.

Thanks Russell!
Linus Walleij