diff mbox

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

Message ID 20170508201159.31634-1-linus.walleij@linaro.org (mailing list archive)
State Superseded
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

Rob Herring May 8, 2017, 9:24 p.m. UTC | #1 
On Mon, May 8, 2017 at 3:11 PM, 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";

There are no child nodes, so you don't need simple-mfd.

> +       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.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring May 9, 2017, 2:23 p.m. UTC | #3 
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.

>
> 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",

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>;
>                         };
>                 };
>
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij May 10, 2017, 3:47 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
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 */