Re: [PATCH v2 2/5] gpio: syscon: Add gpio-syscon for rockchip

[Rob Herring]

On Thu, May 24, 2018 at 7:07 AM, Heiko Stuebner <heiko@xxxxxxxxx> wrote:
> Hi Rob,
>
> Am Mittwoch, 23. Mai 2018, 21:53:53 CEST schrieb Rob Herring:
>> On Wed, May 23, 2018 at 10:12 AM, Heiko StÃbner <heiko@xxxxxxxxx> wrote:
>> > Am Mittwoch, 23. Mai 2018, 16:43:07 CEST schrieb Rob Herring:
>> >> On Tue, May 22, 2018 at 9:02 PM, Levin Du <djw@xxxxxxxxxxxxx> wrote:
>> >> > On 2018-05-23 2:02 AM, Rob Herring wrote:
>> >> >> On Fri, May 18, 2018 at 11:52:05AM +0800, djw@xxxxxxxxxxxxx wrote:
>> >> >>> From: Levin Du <djw@xxxxxxxxxxxxx>
>> >> >>>
>> >> >>> Some GPIOs sit in the GRF_SOC_CON registers of Rockchip SoCs,
>> >> >>> which do not belong to the general pinctrl.
>> >> >>>
>> >> >>> Adding gpio-syscon support makes controlling regulator or
>> >> >>> LED using these special pins very easy by reusing existing
>> >> >>> drivers, such as gpio-regulator and led-gpio.
>> >> >>>
>> >> >>> Signed-off-by: Levin Du <djw@xxxxxxxxxxxxx>
>> >> >>>
>> >> >>> ---
>> >> >>>
>> >> >>> Changes in v2:
>> >> >>> - Rename gpio_syscon10 to gpio_mute in doc
>> >> >>>
>> >> >>> Changes in v1:
>> >> >>> - Refactured for general gpio-syscon usage for Rockchip SoCs.
>> >> >>> - Add doc rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>>   .../bindings/gpio/rockchip,gpio-syscon.txt         | 41
>> >> >>>
>> >> >>> ++++++++++++++++++++++
>> >> >>>
>> >> >>>   drivers/gpio/gpio-syscon.c                         | 30
>> >> >>>
>> >> >>> ++++++++++++++++
>> >> >>>
>> >> >>>   2 files changed, 71 insertions(+)
>> >> >>>   create mode 100644
>> >> >>>
>> >> >>> Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>>
>> >> >>> diff --git
>> >> >>> a/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> new file mode 100644
>> >> >>> index 0000000..b1b2a67
>> >> >>> --- /dev/null
>> >> >>> +++ b/Documentation/devicetree/bindings/gpio/rockchip,gpio-syscon.txt
>> >> >>> @@ -0,0 +1,41 @@
>> >> >>> +* Rockchip GPIO support for GRF_SOC_CON registers
>> >> >>> +
>> >> >>> +Required properties:
>> >> >>> +- compatible: Should contain "rockchip,gpio-syscon".
>> >> >>> +- gpio-controller: Marks the device node as a gpio controller.
>> >> >>> +- #gpio-cells: Should be two. The first cell is the pin number and
>> >> >>> +  the second cell is used to specify the gpio polarity:
>> >> >>> +    0 = Active high,
>> >> >>> +    1 = Active low.
>> >> >>
>> >> >> There's no need for this child node. Just make the parent node a gpio
>> >> >> controller.
>> >> >>
>> >> >> Rob
>> >> >
>> >> > Hi Rob, it is not clear to me. Do you suggest that the grf node should be
>> >> > a
>> >> > gpio controller,
>> >> > like below?
>> >> >
>> >> > +    grf: syscon at ff100000 {
>> >> > +        compatible = "rockchip,gpio-syscon", "rockchip,rk3328-grf",
>> >> > "syscon", "simple-mfd";
>> >>
>> >> Yes, but drop "rockchip,gpio-syscon" and "simple-mfd".
>> >
>> > I would disagree quite a bit here. The grf are the "general register files",
>> > a bunch of registers used for quite a lot of things, and so it seems
>> > among other users, also a gpio-controller for some more random pins
>> > not controlled through the regular gpio controllers.
>> >
>> > For a more fully stocked grf, please see
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288.dtsi#n855
>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399.dtsi#n1338
>> >
>> > So the gpio controller should definitly also be a subnode.
>>
>> Sigh, yes, if there are a bunch of functions needing subnodes like the
>> above, then yes that makes sense. But that's not what has been
>> presented. Please make some attempt at defining *all* the functions.
>> An actual binding would be nice, but I'll settle for just a list of
>> things. The list should have functions that have DT dependencies (like
>> clocks for phys in the above) because until you do, you don't need
>> child nodes.
>
> That's the problem with the Rockchip-GRF, you only realize its content
> when implementing specific features.
>
> Like on the rk3399 the table of the register-list of the GRF alone is 11
> pages long with the register details tables taking up another 230 pages.
> And functional description is often somewhat thin.

But surely one can scan thru it and have some clue what functions
there are. For example, does this chip have phy registers in GRF?

> So I'm not sure I fully understand what you're asking, but in general
> we define the bindings for sub-devices when tackling these individual
> components, see for example
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=72580a49a837c2c7da83f698c00592eac41537d8

Yes, and in that case it makes sense. The individual functions
themselves have resources defined in DT like clocks. What I don't want
to see are child nodes defining *only* a compatible and any provider
properties (e.g. #gpio-cells). The only reason to do that is to make
Linux bind a driver, but DT is not a list of drivers to bind.

This is what I don't want to see:

syscon {
  compatible = "foo,soc-sysctrl", "syscon", "simple-mfd";
  reg = <...>;
  clock-controller {
    compatible = "foo,soc-sysctrl-clocks";
    #clock-cells = <1>;
  };
  reset-controller {
    compatible = "foo,soc-sysctrl-resets";
    #reset-cells = <1>;
  };
  gpio {
    compatible = "foo,soc-sysctrl-gpios";
    #gpio-cells = <2>;
    gpio-controller;
  };
};

But rather:

syscon {
  compatible = "foo,soc-sysctrl";
  reg = <...>;
  #clock-cells = <1>;
  #reset-cells = <1>;
  #gpio-cells = <2>;
  gpio-controller;
};

> which also has a real phy-driver behind it and binding against that
> subnode of the GRF simple-mfd.
>
> These are real IP blocks somewhere on the socs, with regular supplies
> like resets, clocks etc in most cases. Only their controlling registers
> got dumped into the GRF for some reason.

I can tell that from your examples, but I can't tell that with this
binding. For this binding, it looks like you are adding a sub-node for
1 register bit. That wouldn't scale if you have 11 page register list.

> And in retrospect it really looks like we're doing something right,
> because it seems these bindings seem quite stable over time.
>
>
>> > The gpio in question is called "mute", so I'd think the gpio-syscon driver
>> > should just define a "rockchip,rk3328-gpio-mute" compatible and contain
>> > all the register voodoo in the driver itself and not define it in the dt.
>>
>> Is there really just one GPIO? If it has a defined function, then is
>> it really GP? Can you control direction? I know Linus W doesn't like
>> that kind of abuse of GPIO.
>
> looks like I convinced Linus that we're not abusing anything with this :-) .

Okay, but still my question remains: is it really only 1 GPIO?
Dropping "-mute" would be more future proof if not.

Rob