diff mbox

[OpenWrt-Devel,1/2,v2] clk: Add bindings for the Gemini Clock Controller

Message ID 20170508201159.31634-1-linus.walleij@linaro.org
State Not Applicable
Delegated to: John Crispin
Headers show

Commit Message

Linus Walleij May 8, 2017, 8:11 p.m. UTC 
This adds device tree bindings and a header for the Gemini SoC
Clock Controller.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Move the clock controller to be directly in the parent syscon
  node.
---
 .../clock/cortina,gemini-clock-controller.txt      | 22 ++++++++++++++++
 include/dt-bindings/clock/cortina,gemini-clock.h   | 29 ++++++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/cortina,gemini-clock-controller.txt
 create mode 100644 include/dt-bindings/clock/cortina,gemini-clock.h

Comments

Tom Psyborg May 8, 2017, 9:17 p.m. UTC | #1 
Is it ever going to be added so this endless spam can end?


On 8 May 2017 at 22:11, Linus Walleij <linus.walleij@linaro.org> wrote:

> This adds device tree bindings and a header for the Gemini SoC
> Clock Controller.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Move the clock controller to be directly in the parent syscon
>   node.
> ---
>  .../clock/cortina,gemini-clock-controller.txt      | 22 ++++++++++++++++
>  include/dt-bindings/clock/cortina,gemini-clock.h   | 29
> ++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/
> bindings/clock/cortina,gemini-clock-controller.txt
>  create mode 100644 include/dt-bindings/clock/cortina,gemini-clock.h
>
> diff --git a/Documentation/devicetree/bindings/clock/cortina,gemini-clock-controller.txt
> b/Documentation/devicetree/bindings/clock/cortina,gemini-
> clock-controller.txt
> new file mode 100644
> index 000000000000..ae0046bccba0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/cortina,gemini-
> clock-controller.txt
> @@ -0,0 +1,22 @@
> +Clock bindings for the Cortina Systems Gemini SoC Clock Controller
> +
> +Required properties :
> +- compatible : shall contain the following:
> +  "cortina,gemini-clock-controller"
> +- #clock-cells should be <1>
> +
> +The Gemini clock controller needs to be identical to the system controller
> +node.
> +
> +All available clocks are defined as preprocessor macros in
> +dt-bindings/clock/cortina,gemini-clock.h header and can be used in device
> +tree sources.
> +
> +Example:
> +
> +syscon: syscon@40000000 {
> +       compatible = "cortina,gemini-syscon", "cortina,gemini-clock-
> controller",
> +                    "syscon", "simple-mfd";
> +       reg = <0x40000000 0x1000>;
> +       #clock-cells = <1>;
> +};
> diff --git a/include/dt-bindings/clock/cortina,gemini-clock.h
> b/include/dt-bindings/clock/cortina,gemini-clock.h
> new file mode 100644
> index 000000000000..acf5cd550b0c
> --- /dev/null
> +++ b/include/dt-bindings/clock/cortina,gemini-clock.h
> @@ -0,0 +1,29 @@
> +#ifndef DT_BINDINGS_CORTINA_GEMINI_CLOCK_H
> +#define DT_BINDINGS_CORTINA_GEMINI_CLOCK_H
> +
> +/* RTC, AHB, APB, CPU, PCI, TVC, UART clocks and 13 gates */
> +#define GEMINI_NUM_CLKS 20
> +
> +#define GEMINI_CLK_RTC 0
> +#define GEMINI_CLK_AHB 1
> +#define GEMINI_CLK_APB 2
> +#define GEMINI_CLK_CPU 3
> +#define GEMINI_CLK_PCI 4
> +#define GEMINI_CLK_TVC 5
> +#define GEMINI_CLK_UART 6
> +#define GEMINI_CLK_GATES 7
> +#define GEMINI_CLK_GATE_SECURITY 7
> +#define GEMINI_CLK_GATE_GMAC0 8
> +#define GEMINI_CLK_GATE_GMAC1 9
> +#define GEMINI_CLK_GATE_SATA0 10
> +#define GEMINI_CLK_GATE_SATA1 11
> +#define GEMINI_CLK_GATE_USB0 12
> +#define GEMINI_CLK_GATE_USB1 13
> +#define GEMINI_CLK_GATE_IDE 14
> +#define GEMINI_CLK_GATE_PCI 15
> +#define GEMINI_CLK_GATE_DDR 16
> +#define GEMINI_CLK_GATE_FLASH 17
> +#define GEMINI_CLK_GATE_TVC 18
> +#define GEMINI_CLK_GATE_BOOT 19
> +
> +#endif /* DT_BINDINGS_CORTINA_GEMINI_CLOCK_H */
> --
> 2.9.3
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
>
Linus Walleij May 8, 2017, 9:41 p.m. UTC | #2 
On Mon, May 8, 2017 at 11:24 PM, Rob Herring <robh+dt@kernel.org> wrote:

>> +Example:
>> +
>> +syscon: syscon@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).

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@40000000 {
                        compatible = "cortina,gemini-syscon",
                                     "cortina,gemini-clock-controller",
                                     "cortina,gemini-reset",
                                     "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>;
                        };
                };

Before your (as I percieved it) requested changes to avoid
surplus children in the syscon it looked like this:

                syscon: syscon@40000000 {
                        compatible = "cortina,gemini-syscon",
"syscon", "simple-mfd";
                        reg = <0x40000000 0x1000>;

                        syscon-reboot {
                                compatible = "syscon-reboot";
                                regmap = <&syscon>;
                                /* GLOBAL_RESET register */
                                offset = <0x0c>;
                                /* RESET_GLOBAL | RESET_CPU1 */
                                mask = <0xC0000000>;
                        };

                        rcon: reset-controller {
                                compatible = "cortina,gemini-reset";
                                #reset-cells = <1>;
                        };

                        gcc: clock-controller {
                                compatible = "cortina,gemini-clock-controller";
                                #clock-cells = <1>;
                        };
                };


Yours,
Linus Walleij
Linus Walleij May 10, 2017, 3:47 p.m. UTC | #3 
On Tue, May 9, 2017 at 4:23 PM, Rob Herring <robh+dt@kernel.org> wrote:
> On Mon, May 8, 2017 at 4:41 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, May 8, 2017 at 11:24 PM, Rob Herring <robh+dt@kernel.org> wrote:
>>
>>>> +Example:
>>>> +
>>>> +syscon: syscon@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.

I'm sorry. :(

I'll patch the document in arm/gemini.txt
where the syscon node is documented, with a single patch adding
both clock and reset bindings.

>>                 syscon: syscon@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.

OK I'll see if I can make it work with just "cortina,gemini-syscon"
and skip the two others.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/clock/cortina,gemini-clock-controller.txt b/Documentation/devicetree/bindings/clock/cortina,gemini-clock-controller.txt
new file mode 100644
index 000000000000..ae0046bccba0
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/cortina,gemini-clock-controller.txt
@@ -0,0 +1,22 @@ 
+Clock bindings for the Cortina Systems Gemini SoC Clock Controller
+
+Required properties :
+- compatible : shall contain the following:
+  "cortina,gemini-clock-controller"
+- #clock-cells should be <1>
+
+The Gemini clock controller needs to be identical to the system controller
+node.
+
+All available clocks are defined as preprocessor macros in
+dt-bindings/clock/cortina,gemini-clock.h header and can be used in device
+tree sources.
+
+Example:
+
+syscon: syscon@40000000 {
+	compatible = "cortina,gemini-syscon", "cortina,gemini-clock-controller",
+		     "syscon", "simple-mfd";
+	reg = <0x40000000 0x1000>;
+	#clock-cells = <1>;
+};
diff --git a/include/dt-bindings/clock/cortina,gemini-clock.h b/include/dt-bindings/clock/cortina,gemini-clock.h
new file mode 100644
index 000000000000..acf5cd550b0c
--- /dev/null
+++ b/include/dt-bindings/clock/cortina,gemini-clock.h
@@ -0,0 +1,29 @@ 
+#ifndef DT_BINDINGS_CORTINA_GEMINI_CLOCK_H
+#define DT_BINDINGS_CORTINA_GEMINI_CLOCK_H
+
+/* RTC, AHB, APB, CPU, PCI, TVC, UART clocks and 13 gates */
+#define GEMINI_NUM_CLKS 20
+
+#define GEMINI_CLK_RTC 0
+#define GEMINI_CLK_AHB 1
+#define GEMINI_CLK_APB 2
+#define GEMINI_CLK_CPU 3
+#define GEMINI_CLK_PCI 4
+#define GEMINI_CLK_TVC 5
+#define GEMINI_CLK_UART 6
+#define GEMINI_CLK_GATES 7
+#define GEMINI_CLK_GATE_SECURITY 7
+#define GEMINI_CLK_GATE_GMAC0 8
+#define GEMINI_CLK_GATE_GMAC1 9
+#define GEMINI_CLK_GATE_SATA0 10
+#define GEMINI_CLK_GATE_SATA1 11
+#define GEMINI_CLK_GATE_USB0 12
+#define GEMINI_CLK_GATE_USB1 13
+#define GEMINI_CLK_GATE_IDE 14
+#define GEMINI_CLK_GATE_PCI 15
+#define GEMINI_CLK_GATE_DDR 16
+#define GEMINI_CLK_GATE_FLASH 17
+#define GEMINI_CLK_GATE_TVC 18
+#define GEMINI_CLK_GATE_BOOT 19
+
+#endif /* DT_BINDINGS_CORTINA_GEMINI_CLOCK_H */