diff mbox series

[3/8] dt-bindings: clock: Add rockchip,rk3528-cru

Message ID 20241001042401.31903-5-ziyao@disroot.org (mailing list archive)
State New
Headers show
Series Support clock and reset unit of Rockchip RK3528 | expand

Commit Message

Yao Zi Oct. 1, 2024, 4:23 a.m. UTC 
Document Rockchip RK3528 clock and reset unit.

Signed-off-by: Yao Zi <ziyao@disroot.org>
---
 .../bindings/clock/rockchip,rk3528-cru.yaml   | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml

Comments

Conor Dooley Oct. 1, 2024, 4:29 p.m. UTC | #1 
On Tue, Oct 01, 2024 at 04:23:57AM +0000, Yao Zi wrote:
> Document Rockchip RK3528 clock and reset unit.
> 
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
>  .../bindings/clock/rockchip,rk3528-cru.yaml   | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> new file mode 100644
> index 000000000000..ae51dfde5bb9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> @@ -0,0 +1,63 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/rockchip,rk3528-cru.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip RK3528 Clock and Reset Controller
> +
> +maintainers:
> +  - Yao Zi <ziyao@disroot.org>
> +
> +description: |
> +  The RK3528 clock controller generates the clock and also implements a reset
> +  controller for SoC peripherals. For example, it provides SCLK_UART0 and
> +  PCLK_UART0 as well as SRST_P_UART0 and SRST_S_UART0 for the first UART
> +  module.
> +  Each clock is assigned an identifier, consumer nodes can use it to specify
> +  the clock. All available clock and reset IDs are defined in dt-binding
> +  headers.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3528-cru

nit: This can probably be a const, rather than an enum.

> +
> +  reg:
> +    maxItems: 1
> +
> +  assigned-clocks: true
> +
> +  assigned-clock-rates: true
> +
> +  clocks:
> +    minItems: 2
> +    maxItems: 2
> +
> +  clock-names:
> +    items:
> +      - const: xin24m
> +      - const: phy_50m_out

Why is this input clock named "out"? clocks should be named after how
they're used in the IP in question, not the name of the source of that
clock in the SoC.
Without descriptions provided in the clocks property, it is hard to
understand what this second clock is for.

> +
> +  "#clock-cells":
> +    const: 1
> +
> +  "#reset-cells":
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg

Why would the input clocks be optional?

> +  - "#clock-cells"
> +  - "#reset-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    cru: clock-controller@ff4a0000 {

nit: the cru label is not used and can be dropped.

Cheers,
Conor.

> +        compatible = "rockchip,rk3528-cru";
> +        reg = <0xff4a0000 0x30000>;
> +        #clock-cells = <1>;
> +        #reset-cells = <1>;
> +    };
> -- 
> 2.46.0
>
Yao Zi Oct. 1, 2024, 9:18 p.m. UTC | #2 
On Tue, Oct 01, 2024 at 05:29:15PM +0100, Conor Dooley wrote:
> On Tue, Oct 01, 2024 at 04:23:57AM +0000, Yao Zi wrote:
> > Document Rockchip RK3528 clock and reset unit.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > ---
> >  .../bindings/clock/rockchip,rk3528-cru.yaml   | 63 +++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> > new file mode 100644
> > index 000000000000..ae51dfde5bb9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> > @@ -0,0 +1,63 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/rockchip,rk3528-cru.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip RK3528 Clock and Reset Controller
> > +
> > +maintainers:
> > +  - Yao Zi <ziyao@disroot.org>
> > +
> > +description: |
> > +  The RK3528 clock controller generates the clock and also implements a reset
> > +  controller for SoC peripherals. For example, it provides SCLK_UART0 and
> > +  PCLK_UART0 as well as SRST_P_UART0 and SRST_S_UART0 for the first UART
> > +  module.
> > +  Each clock is assigned an identifier, consumer nodes can use it to specify
> > +  the clock. All available clock and reset IDs are defined in dt-binding
> > +  headers.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - rockchip,rk3528-cru
> 
> nit: This can probably be a const, rather than an enum.
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  assigned-clocks: true
> > +
> > +  assigned-clock-rates: true
> > +
> > +  clocks:
> > +    minItems: 2
> > +    maxItems: 2
> > +
> > +  clock-names:
> > +    items:
> > +      - const: xin24m
> > +      - const: phy_50m_out
> 
> Why is this input clock named "out"? clocks should be named after how
> they're used in the IP in question, not the name of the source of that
> clock in the SoC.
> Without descriptions provided in the clocks property, it is hard to
> understand what this second clock is for.

Thanks for explaination, it should something like "clk_gmac0".

> > +
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  "#reset-cells":
> > +    const: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> 
> Why would the input clocks be optional?

This follows other Rockchip SoCs, which often omit input clocks in
devicetree and depend on clock names registered in common clock
framework to work.

For completeness, they really shouldn't be optional.

> > +  - "#clock-cells"
> > +  - "#reset-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    cru: clock-controller@ff4a0000 {
> 
> nit: the cru label is not used and can be dropped.
> 
> Cheers,
> Conor.

All comments will be adapted in next revision. Thanks.

Best regards,
Yao Zi
Conor Dooley Oct. 2, 2024, 8:49 a.m. UTC | #3 
On Tue, Oct 01, 2024 at 09:18:03PM +0000, Yao Zi wrote:
> On Tue, Oct 01, 2024 at 05:29:15PM +0100, Conor Dooley wrote:
> > On Tue, Oct 01, 2024 at 04:23:57AM +0000, Yao Zi wrote:
> > > Document Rockchip RK3528 clock and reset unit.
> > > 
> > > Signed-off-by: Yao Zi <ziyao@disroot.org>
> > > ---
> > >  .../bindings/clock/rockchip,rk3528-cru.yaml   | 63 +++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> > > new file mode 100644
> > > index 000000000000..ae51dfde5bb9
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
> > > @@ -0,0 +1,63 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/rockchip,rk3528-cru.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rockchip RK3528 Clock and Reset Controller
> > > +
> > > +maintainers:
> > > +  - Yao Zi <ziyao@disroot.org>
> > > +
> > > +description: |
> > > +  The RK3528 clock controller generates the clock and also implements a reset
> > > +  controller for SoC peripherals. For example, it provides SCLK_UART0 and
> > > +  PCLK_UART0 as well as SRST_P_UART0 and SRST_S_UART0 for the first UART
> > > +  module.
> > > +  Each clock is assigned an identifier, consumer nodes can use it to specify
> > > +  the clock. All available clock and reset IDs are defined in dt-binding
> > > +  headers.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - rockchip,rk3528-cru
> > 
> > nit: This can probably be a const, rather than an enum.
> > 
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  assigned-clocks: true
> > > +
> > > +  assigned-clock-rates: true
> > > +
> > > +  clocks:
> > > +    minItems: 2
> > > +    maxItems: 2
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: xin24m
> > > +      - const: phy_50m_out
> > 
> > Why is this input clock named "out"? clocks should be named after how
> > they're used in the IP in question, not the name of the source of that
> > clock in the SoC.
> > Without descriptions provided in the clocks property, it is hard to
> > understand what this second clock is for.
> 
> Thanks for explaination, it should something like "clk_gmac0".

So it is actually an input clock to the cru? I'd like to see an items
list in the clocks property please, describing what these clocks are.
Also, "clk" is redundant, since these are all clocks, so drop that from
the name.

> 
> > > +
> > > +  "#clock-cells":
> > > +    const: 1
> > > +
> > > +  "#reset-cells":
> > > +    const: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > 
> > Why would the input clocks be optional?
> 
> This follows other Rockchip SoCs, which often omit input clocks in
> devicetree and depend on clock names registered in common clock
> framework to work.
> 
> For completeness, they really shouldn't be optional.

Then please make it required. If the input clocks are required to make
the clock controller function, they should be marked as required.

> 
> > > +  - "#clock-cells"
> > > +  - "#reset-cells"
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    cru: clock-controller@ff4a0000 {
> > 
> > nit: the cru label is not used and can be dropped.
> 
> All comments will be adapted in next revision. Thanks.

Cool.

Thanks,
Conor.
Yao Zi Oct. 2, 2024, 10:02 a.m. UTC | #4 
On Wed, Oct 02, 2024 at 09:49:21AM +0100, Conor Dooley wrote:
> On Tue, Oct 01, 2024 at 09:18:03PM +0000, Yao Zi wrote:
> > On Tue, Oct 01, 2024 at 05:29:15PM +0100, Conor Dooley wrote:
> > > On Tue, Oct 01, 2024 at 04:23:57AM +0000, Yao Zi wrote:
> > > > +  clock-names:
> > > > +    items:
> > > > +      - const: xin24m
> > > > +      - const: phy_50m_out
> > > 
> > > Why is this input clock named "out"? clocks should be named after how
> > > they're used in the IP in question, not the name of the source of that
> > > clock in the SoC.
> > > Without descriptions provided in the clocks property, it is hard to
> > > understand what this second clock is for.
> > 
> > Thanks for explaination, it should something like "clk_gmac0".
> 
> So it is actually an input clock to the cru?

Yes, phy module generates it, being parent of several CRU clocks.

> I'd like to see an items list in the clocks property please,
> describing what these clocks are.
>
> Also, "clk" is redundant, since these are all clocks, so drop that
> from the name.

Okay.

Cheers,
Yao Zi
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
new file mode 100644
index 000000000000..ae51dfde5bb9
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/rockchip,rk3528-cru.yaml
@@ -0,0 +1,63 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/rockchip,rk3528-cru.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip RK3528 Clock and Reset Controller
+
+maintainers:
+  - Yao Zi <ziyao@disroot.org>
+
+description: |
+  The RK3528 clock controller generates the clock and also implements a reset
+  controller for SoC peripherals. For example, it provides SCLK_UART0 and
+  PCLK_UART0 as well as SRST_P_UART0 and SRST_S_UART0 for the first UART
+  module.
+  Each clock is assigned an identifier, consumer nodes can use it to specify
+  the clock. All available clock and reset IDs are defined in dt-binding
+  headers.
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3528-cru
+
+  reg:
+    maxItems: 1
+
+  assigned-clocks: true
+
+  assigned-clock-rates: true
+
+  clocks:
+    minItems: 2
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: xin24m
+      - const: phy_50m_out
+
+  "#clock-cells":
+    const: 1
+
+  "#reset-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - "#clock-cells"
+  - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    cru: clock-controller@ff4a0000 {
+        compatible = "rockchip,rk3528-cru";
+        reg = <0xff4a0000 0x30000>;
+        #clock-cells = <1>;
+        #reset-cells = <1>;
+    };