[PATCH 1/2 v2] clk: Add bindings for the Gemini Clock Controller

[Rob Herring]

On Mon, May 8, 2017 at 4:41 PM, Linus Walleij <linus.walleij at linaro.org> wrote:
> On Mon, May 8, 2017 at 11:24 PM, Rob Herring <robh+dt at kernel.org> wrote:
>
>>> +Example:
>>> +
>>> +syscon: syscon at 40000000 {
>>> +       compatible = "cortina,gemini-syscon", "cortina,gemini-clock-controller",
>>> +                    "syscon", "simple-mfd";
>>
>> There are no child nodes, so you don't need simple-mfd.
>
> The example is taken from an actual device tree (look below),
> where there are child nodes, I can trim it down.
>
>>> +       reg = <0x40000000 0x1000>;
>>
>> Looks like you have 2 nodes pointing to the same address with your
>> reset binding? You shouldn't have overlapping resources. It's allowed
>> for historical reasons but breaks resource creation in Linux.
>
> No... they are all in the same node, just sharing the same
> resource by way of regmap (syscon).

Okay, then please document at least the parent syscon node in a single
doc. Splitting it is very confusing.

>
> In the end, as I think you requested, when you said:
>
>>> +     clock-controller {
>>> +             compatible = "cortina,gemini-clock-controller";
>>> +             #clock-cells = <1>;
> (...)
>> There's not really much reason to have a child node here. The parent can
>> be the clock provider.
> (...)
>> Same comment as clocks. The parent can be the provider.
>
> So as you say, no specific child is needed and syscon provides
> clocks and resets:
>
>                 syscon: syscon at 40000000 {
>                         compatible = "cortina,gemini-syscon",
>                                      "cortina,gemini-clock-controller",
>                                      "cortina,gemini-reset",

This mostly looks fine, but you shouldn't need 3 compatible strings
for the block.

>                                      "syscon", "simple-mfd";
>                         reg = <0x40000000 0x1000>;
>                         #clock-cells = <1>;
>                         #reset-cells = <1>;
>
>                         syscon-reboot {
>                                 compatible = "syscon-reboot";
>                                 regmap = <&syscon>;
>                                 /* GLOBAL_RESET register */
>                                 offset = <0x0c>;
>                                 /* RESET_GLOBAL | RESET_CPU1 */
>                                 mask = <0xC0000000>;
>                         };
>                 };
>