Re: [PATCH 2/3] pinctrl: zx: Add ZTE pinctrl dts document

Re: [PATCH 2/3] pinctrl: zx: Add ZTE pinctrl dts document

[Linus Walleij]

Please always include devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org on subsequent
postings of the bindings.

Also include Heiko Stübner and Doug Andersson who are also
interested in hierarchical pin controllers on subsequent postings.

On Fri, Aug 26, 2016 at 2:19 PM, Jun Nie <jun.nie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> +The pins controlled by ZX pin controller are organized in banks,
> +number of pins in each bank may vary.  Each pin has different multiplexing
> +functions. There are two type of pins, normal ones and AON ones.

I understand so far. Spell out what the acronym AON means. Does
it mean "always-on?"

> + AON
> +pins control high level multiplex and normal pins may require multiplex
> +configuration of parent AON pins.

I don't understand this at all.

What is "high level multiplex"?

Since you mention parents I suspect that what you are referring to is
that the pins are hierarchical, and that they are controlled by two
levels of pin controllers, such that a physical pin is connected to pin
controller A which can multiplex pins to N functions, and then the line
from A is connected to another pin controller B that can again multiplex
to M other functions?

Please describe this hardware set-up with *lots* of detail so we have
a chance of understanding it fully. All necessary information shall go
into this DT binding document so we know where to look it up.

> + As the AON pins number is not as much as
> +normal pins, some normal pins are not routed through AON pin side and are
> +under direct control by itself.

I don't understand this. I suspect the English language may be a
problem here, I think "as much as" should be "as many as", also
subject and object is very hard to understand when the pin controller
is referred to as "itself". It's just very hard to parse, sorry. Please
elaborate and I will try to understand and help as much as I can.

> +Required properties:
> +- compatible:
> +  "zte,zx296718-pinctrl"
> +  "zte,zx296718-aonpmx"
> +
> +- reg: Should contain the register physical address and length for the
> +  pin controller.

OK

> +IO pull up/down etc configuration is supported with unified management of
> +normal pins and AON pins. The configuration registers area is just after
> +AON pinmux reg area, while normal pins regs in different area. So two dts
> +nodes are needed to provides the two reg regions.

Again hard to understand, but why do you need two nodes
and not just two register areas regs = <a b>, <c d>;?

I guess because one pin controller is using another one but
I'm not sure.

> +Below configuration are supported. Please refer to pinctrl-bindings.txt
> +in this directory for more details of the common pinctrl bindings used
> +by client devices.
> +
> +bias-pull-up            - pull up the pin
> +bias-pull-down          - pull down the pin
> +drive-strength          - sink or source at most 7 mA
> +input-enable            - enable input on pin (no effect on output)
> +power-source            - select power supplies. 1: 1.8V, 0: 3.3V
> +slew-rate               - set the slew rate

OK

> +Pin names are defined by bank sequence and pins number in the bank. For
> +example, B2 is the 3rd pin in the second bank. The AON pin has prefix
> +AON, like AONC2.

OK

> +Example dts nodes:
> +
> +pinctop: pinctrl@01462000 {
> +       compatible = "zte,zx296718-pinctrl";
> +       reg = <0x01462000 0x1000>;
> +
> +       i2c5_pins: i2c5pins {
> +               pins = "G6", "G7";
> +               function = "I2C5";
> +       }
> +};
> +
> +pmx_aon: pinctrl@00119000 {
> +       compatible = "zte,zx296718-aonpmx";
> +       reg = <0x00119000 0x1000>;
> +};

The problem with this devicetree is that it does not express the hierarchical
nature that you describe above. And I have no idea how the driver
handles that relation. These look like two independent pin controller
but AFAICT they are not.

These are arrangements that make more sense:

pinctrla {
    compatible = "parent-device-compat";
    (...)
    pinctrlb {
        compatible = "child-device-compat";
        (...)
    };
};

This makes the child pin controller a proper child of the parent and
make the child probe instantiate from the parent (in
Linux of_platform_default_populate() inside the parent driver)
and thus it knows the parent is always available before the child.

Another arrangement is:

pa: pinctrla {
    compatible = "parent-device-compat";
    (...)
};

pb: pinctrlb {
    compatible = "child-device-compat";
    pinctrl-parent = &pinctrla;
    (...)
};

This is in case we need to have hierarchies defined this way.

I don't know the best way to define hierarchical pin controllers yet
so we definately need some discussion about it.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] pinctrl: zx: Add ZTE pinctrl dts document

[Linus Walleij]

On Tue, Sep 6, 2016 at 4:15 PM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> pa: pinctrla {
>     compatible = "parent-device-compat";
>     (...)
> };
>
> pb: pinctrlb {
>     compatible = "child-device-compat";
>     pinctrl-parent = &pinctrla;
>     (...)
> };

After thinking some more I kind of fancy the GPIO ranges
approach to be reused:

pa: pinctrla {
    compatible = "parent-device-compat";
    (...)
};

pb: pinctrlb {
    compatible = "child-device-compat";
    pinctrl-ranges = <&pa 0 31 0>;
    (...)
};

The definition and use would be similar to gpio-ranges, see:
Documentation/devicetree/bindings/gpio/gpio.txt, but these
bindings are specified in BNF which IMO is just confusing.

Anyways its <phandle local-offset count remote-offset>

Also group names are supported for GPIO ranges.

In the Linux implementation this facilitates reuse of the
existing GPIO ranges parsing and resolution.

To make cross-calls we need an abstract call akin to the
existing pinctrl_request_gpio() and pinctrl_free_gpio()
just named pinctrl_request_hierarchical()
pinctrl_free_hierarchical() or so so it is chrystal clear
what is going on when reading the code.

As this passes through the pinctrl driver core, new callbacks
are needed in struct pinmux_ops such as
.hierarchical_request(), .hierarchical_free().

This is going to be complex, but a way more maintainable
solution than doing the hack in patch 1/3 where we totally
loose control and definition of what happens.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] pinctrl: zx: Add ZTE pinctrl dts document

[Jun Nie]

2016-09-06 22:43 GMT+08:00 Linus Walleij <linus.walleij@linaro.org>:
> On Tue, Sep 6, 2016 at 4:15 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> pa: pinctrla {
>>     compatible = "parent-device-compat";
>>     (...)
>> };
>>
>> pb: pinctrlb {
>>     compatible = "child-device-compat";
>>     pinctrl-parent = &pinctrla;
>>     (...)
>> };
>
> After thinking some more I kind of fancy the GPIO ranges
> approach to be reused:
>
> pa: pinctrla {
>     compatible = "parent-device-compat";
>     (...)
> };
>
> pb: pinctrlb {
>     compatible = "child-device-compat";
>     pinctrl-ranges = <&pa 0 31 0>;
>     (...)
> };
>
> The definition and use would be similar to gpio-ranges, see:
> Documentation/devicetree/bindings/gpio/gpio.txt, but these
> bindings are specified in BNF which IMO is just confusing.
>
> Anyways its <phandle local-offset count remote-offset>
>
> Also group names are supported for GPIO ranges.
>
> In the Linux implementation this facilitates reuse of the
> existing GPIO ranges parsing and resolution.
>
> To make cross-calls we need an abstract call akin to the
> existing pinctrl_request_gpio() and pinctrl_free_gpio()
> just named pinctrl_request_hierarchical()
> pinctrl_free_hierarchical() or so so it is chrystal clear
> what is going on when reading the code.
>
> As this passes through the pinctrl driver core, new callbacks
> are needed in struct pinmux_ops such as
> .hierarchical_request(), .hierarchical_free().

The GPIO range method does provide clear hierarchical relation
and ease maintenance. But pin configuration of normal pins
and AON/normal shared pins are mixed together with one register
controlling 3 pin config. So I need find a way to handle the pin
configuration sharing.

Maybe splitting pin configuration to another driver is a solution with
pin config range linked the two pinmux devices like GPIO range.
But this lead to more complexity.

>
> This is going to be complex, but a way more maintainable
> solution than doing the hack in patch 1/3 where we totally
> loose control and definition of what happens.
>
> Yours,
> Linus Walleij

Re: [PATCH 2/3] pinctrl: zx: Add ZTE pinctrl dts document

[Jun Nie]

2016-09-06 22:15 GMT+08:00 Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>:
> Please always include devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org on subsequent
> postings of the bindings.
>
> Also include Heiko Stübner and Doug Andersson who are also
> interested in hierarchical pin controllers on subsequent postings.
>
> On Fri, Aug 26, 2016 at 2:19 PM, Jun Nie <jun.nie-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
>> +The pins controlled by ZX pin controller are organized in banks,
>> +number of pins in each bank may vary.  Each pin has different multiplexing
>> +functions. There are two type of pins, normal ones and AON ones.
>
> I understand so far. Spell out what the acronym AON means. Does
> it mean "always-on?"
>
>> + AON
>> +pins control high level multiplex and normal pins may require multiplex
>> +configuration of parent AON pins.
>
> I don't understand this at all.
>
> What is "high level multiplex"?
>
> Since you mention parents I suspect that what you are referring to is
> that the pins are hierarchical, and that they are controlled by two
> levels of pin controllers, such that a physical pin is connected to pin
> controller A which can multiplex pins to N functions, and then the line
> from A is connected to another pin controller B that can again multiplex
> to M other functions?
>
> Please describe this hardware set-up with *lots* of detail so we have
> a chance of understanding it fully. All necessary information shall go
> into this DT binding document so we know where to look it up.
>
Here is more hardware detail. Will add it to document later.

Hardware has two register regions to control pinmux, one is for normal
function configuration and the other one is for always on subsystem(AON).
Some pins are controlled by both normal register and AON register, while
some pins are controlled only by normal register. That means some pins
are hierarchical, AON register is the 1st level control for pinmux. If
we need any function in normal register pin multiplexing, we need configure
related AON pinmux register to non-AON function first. Then we configure
normal pinmux register to get the pin function we want. For those pins
that are not controlled by AON registers, we just need to configure normal
pinmux register for function multiplexing.

All pin configuration, such as pull up/down, is controlled by registers
in AON register region, which is layouted just after AON pinmux registers.
So we need access AON register region even we are configuring normal
pins which has no AON pin multiplex functions.

>> + As the AON pins number is not as much as
>> +normal pins, some normal pins are not routed through AON pin side and are
>> +under direct control by itself.
>
> I don't understand this. I suspect the English language may be a
> problem here, I think "as much as" should be "as many as", also
> subject and object is very hard to understand when the pin controller
> is referred to as "itself". It's just very hard to parse, sorry. Please
> elaborate and I will try to understand and help as much as I can.
>
>> +Required properties:
>> +- compatible:
>> +  "zte,zx296718-pinctrl"
>> +  "zte,zx296718-aonpmx"
>> +
>> +- reg: Should contain the register physical address and length for the
>> +  pin controller.
>
> OK
>
>> +IO pull up/down etc configuration is supported with unified management of
>> +normal pins and AON pins. The configuration registers area is just after
>> +AON pinmux reg area, while normal pins regs in different area. So two dts
>> +nodes are needed to provides the two reg regions.
>
> Again hard to understand, but why do you need two nodes
> and not just two register areas regs = <a b>, <c d>;?
>
> I guess because one pin controller is using another one but
> I'm not sure.

Thanks for pointing the usage of  regs = <a b>, <c d>;
For normal pinmux function, we still need AON registers for pin
configuration, maybe
this is what you care.

>
>> +Below configuration are supported. Please refer to pinctrl-bindings.txt
>> +in this directory for more details of the common pinctrl bindings used
>> +by client devices.
>> +
>> +bias-pull-up            - pull up the pin
>> +bias-pull-down          - pull down the pin
>> +drive-strength          - sink or source at most 7 mA
>> +input-enable            - enable input on pin (no effect on output)
>> +power-source            - select power supplies. 1: 1.8V, 0: 3.3V
>> +slew-rate               - set the slew rate
>
> OK
>
>> +Pin names are defined by bank sequence and pins number in the bank. For
>> +example, B2 is the 3rd pin in the second bank. The AON pin has prefix
>> +AON, like AONC2.
>
> OK
>
>> +Example dts nodes:
>> +
>> +pinctop: pinctrl@01462000 {
>> +       compatible = "zte,zx296718-pinctrl";
>> +       reg = <0x01462000 0x1000>;
>> +
>> +       i2c5_pins: i2c5pins {
>> +               pins = "G6", "G7";
>> +               function = "I2C5";
>> +       }
>> +};
>> +
>> +pmx_aon: pinctrl@00119000 {
>> +       compatible = "zte,zx296718-aonpmx";
>> +       reg = <0x00119000 0x1000>;
>> +};
>
> The problem with this devicetree is that it does not express the hierarchical
> nature that you describe above. And I have no idea how the driver
> handles that relation. These look like two independent pin controller
> but AFAICT they are not.
>
> These are arrangements that make more sense:
>
> pinctrla {
>     compatible = "parent-device-compat";
>     (...)
>     pinctrlb {
>         compatible = "child-device-compat";
>         (...)
>     };
> };
>
> This makes the child pin controller a proper child of the parent and
> make the child probe instantiate from the parent (in
> Linux of_platform_default_populate() inside the parent driver)
> and thus it knows the parent is always available before the child.
>
> Another arrangement is:
>
> pa: pinctrla {
>     compatible = "parent-device-compat";
>     (...)
> };
>
> pb: pinctrlb {
>     compatible = "child-device-compat";
>     pinctrl-parent = &pinctrla;
>     (...)
> };
>
> This is in case we need to have hierarchies defined this way.

Yes, the two arrangements are clearer to show the parent relation.

>
> I don't know the best way to define hierarchical pin controllers yet
> so we definately need some discussion about it.
>
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html