[PATCH v3 0/9] Add RZ/G3E xSPI support

[PATCH v3 0/9] Add RZ/G3E xSPI support

[Biju Das]

The xSPI IP found on RZ/G3E SoC similar to RPC-IF interface, but it
can support writes on memory-mapped area. Even though the registers are
different, the rpcif driver code can be reused for xSPI by adding wrapper
function to it.

This patch series tested on RZ/G2L and RZ/G3E by overwriting boot
partitions.

v2->v3:
 * Fixed RPCIF_DRENR_CDB macro error.
v1->v2:
 * As rz-xspi is too generic, replaced file name rz-xspi->rzg3e-xspi
   and dropped generic compatible rz-xspi.
 * Dropped prefix spi from interrupt names.
 * Updated the example with above changes.
 * Retained Rb tag from Rob as these changes are trivial.
 * Fixed the build error reported by bot by dropping 
   EXPORT_SYMBOL(xspi_dirmap_read) and restoring
   EXPORT_SYMBOL(rpcif_dirmap_read).
 * Replaced enum XSPI_RZ->XSPI_RZ_G3E.
 * Replaced compatible rz-xspi->r9a09g047-xspi and device data
   xspi_info_rz->xspi_info_r9a09g047.

Biju Das (9):
  dt-bindings: memory: Document RZ/G3E support
  memory: renesas-rpc-if: Fix RPCIF_DRENR_CDB macro error
  memory: renesas-rpc-if: Move rpc-if reg definitions
  memory: renesas-rpc-if: Use devm_reset_control_array_get_exclusive()
  memory: renesas-rpc-if: Move rpcif_info definitions near to the user
  memory: renesas-rpc-if: Add regmap to struct rpcif_info
  memory: renesas-rpc-if: Add wrapper functions
  memory: renesas-rpc-if: Add RZ/G3E xSPI support
  spi: rpc-if: Add write support for memory-mapped area

 .../renesas,rzg3e-xspi.yaml                   | 135 ++++
 drivers/memory/renesas-rpc-if-regs.h          | 147 ++++
 drivers/memory/renesas-rpc-if.c               | 665 +++++++++++++-----
 drivers/memory/renesas-xspi-if-regs.h         | 105 +++
 drivers/spi/spi-rpc-if.c                      |  16 +-
 include/memory/renesas-rpc-if.h               |   4 +
 6 files changed, 881 insertions(+), 191 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml
 create mode 100644 drivers/memory/renesas-rpc-if-regs.h
 create mode 100644 drivers/memory/renesas-xspi-if-regs.h

-- 
2.43.0

[PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Document support for the Expanded Serial Peripheral Interface (xSPI)
Controller in the Renesas RZ/G3E (R9A09G047) SoC.

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v2->v3:
 * No change.
v1->v2:
 * As rz-xspi is too generic, replaced file name rz-xspi->rzg3e-xspi
   and dropped generic compatible rz-xspi.
 * Dropped prefix spi from interrupt names.
 * Updated the example with above changes.
 * Retained Rb tag from Rob as these changes are trivial.
---
 .../renesas,rzg3e-xspi.yaml                   | 135 ++++++++++++++++++
 1 file changed, 135 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml

diff --git a/Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml b/Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml
new file mode 100644
index 000000000000..4d5aa5812d74
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml
@@ -0,0 +1,135 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/renesas,rzg3e-xspi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas Expanded Serial Peripheral Interface (xSPI)
+
+maintainers:
+  - Biju Das <biju.das.jz@bp.renesas.com>
+
+description: |
+  Renesas xSPI allows a SPI flash connected to the SoC to be accessed via
+  the memory-mapping or the manual command mode.
+
+  The flash chip itself should be represented by a subnode of the XSPI node.
+  The flash interface is selected based on the "compatible" property of this
+  subnode:
+  -  "jedec,spi-nor";
+
+allOf:
+  - $ref: /schemas/spi/spi-controller.yaml#
+
+properties:
+  compatible:
+    const: renesas,r9a09g047-xspi  # RZ/G3E
+
+  reg:
+    items:
+      - description: xSPI registers
+      - description: direct mapping area
+
+  reg-names:
+    items:
+      - const: regs
+      - const: dirmap
+
+  interrupts:
+    items:
+      - description: Interrupt pulse signal by factors excluding errors
+      - description: Interrupt pulse signal by error factors
+
+  interrupt-names:
+    items:
+      - const: pulse
+      - const: err_pulse
+
+  clocks:
+    items:
+      - description: AHB clock
+      - description: AXI clock
+      - description: SPI clock
+      - description: Double speed SPI clock
+
+  clock-names:
+    items:
+      - const: ahb
+      - const: axi
+      - const: spi
+      - const: spix2
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    items:
+      - description: Hardware reset
+      - description: AXI reset
+
+  reset-names:
+    items:
+      - const: hresetn
+      - const: aresetn
+
+  renesas,xspi-cs-addr-sys:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: |
+      Phandle to the system controller (sys) that allows to configure
+      xSPI CS0 and CS1 addresses.
+
+patternProperties:
+  "flash@[0-9a-f]+$":
+    type: object
+    additionalProperties: true
+
+    properties:
+      compatible:
+        contains:
+          const: jedec,spi-nor
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - power-domains
+  - resets
+  - reset-names
+  - '#address-cells'
+  - '#size-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/renesas-cpg-mssr.h>
+
+    spi@11030000 {
+        compatible = "renesas,r9a09g047-xspi";
+        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
+        reg-names = "regs", "dirmap";
+        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
+                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "pulse", "err_pulse";
+        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
+                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
+        clock-names = "ahb", "axi", "spi", "spix2";
+        power-domains = <&cpg>;
+        resets = <&cpg 0xa3>, <&cpg 0xa4>;
+        reset-names = "hresetn", "aresetn";
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        flash@0 {
+          compatible = "jedec,spi-nor";
+          reg = <0>;
+          spi-max-frequency = <40000000>;
+          spi-tx-bus-width = <1>;
+          spi-rx-bus-width = <1>;
+        };
+    };
-- 
2.43.0

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi All,

> -----Original Message-----
> From: Biju Das <biju.das.jz@bp.renesas.com>
> Sent: 11 March 2025 11:36
> Subject: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Document support for the Expanded Serial Peripheral Interface (xSPI) Controller in the Renesas RZ/G3E
> (R9A09G047) SoC.
> 
> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> v2->v3:
>  * No change.
> v1->v2:
>  * As rz-xspi is too generic, replaced file name rz-xspi->rzg3e-xspi
>    and dropped generic compatible rz-xspi.
>  * Dropped prefix spi from interrupt names.
>  * Updated the example with above changes.
>  * Retained Rb tag from Rob as these changes are trivial.
> ---
>  .../renesas,rzg3e-xspi.yaml                   | 135 ++++++++++++++++++
>  1 file changed, 135 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml
> b/Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e-xspi.yaml
> new file mode 100644
> index 000000000000..4d5aa5812d74
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e
> +++ -xspi.yaml
> @@ -0,0 +1,135 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> +---
> +$id:
> +http://devicetree.org/schemas/memory-controllers/renesas,rzg3e-xspi.yam
> +l#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas Expanded Serial Peripheral Interface (xSPI)
> +
> +maintainers:
> +  - Biju Das <biju.das.jz@bp.renesas.com>
> +
> +description: |
> +  Renesas xSPI allows a SPI flash connected to the SoC to be accessed
> +via
> +  the memory-mapping or the manual command mode.
> +
> +  The flash chip itself should be represented by a subnode of the XSPI node.
> +  The flash interface is selected based on the "compatible" property of
> + this
> +  subnode:
> +  -  "jedec,spi-nor";
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: renesas,r9a09g047-xspi  # RZ/G3E
> +
> +  reg:
> +    items:
> +      - description: xSPI registers
> +      - description: direct mapping area
> +
> +  reg-names:
> +    items:
> +      - const: regs
> +      - const: dirmap
> +
> +  interrupts:
> +    items:
> +      - description: Interrupt pulse signal by factors excluding errors
> +      - description: Interrupt pulse signal by error factors
> +
> +  interrupt-names:
> +    items:
> +      - const: pulse
> +      - const: err_pulse
> +
> +  clocks:
> +    items:
> +      - description: AHB clock
> +      - description: AXI clock
> +      - description: SPI clock
> +      - description: Double speed SPI clock
> +
> +  clock-names:
> +    items:
> +      - const: ahb
> +      - const: axi
> +      - const: spi
> +      - const: spix2
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    items:
> +      - description: Hardware reset
> +      - description: AXI reset
> +
> +  reset-names:
> +    items:
> +      - const: hresetn
> +      - const: aresetn
> +
> +  renesas,xspi-cs-addr-sys:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: |
> +      Phandle to the system controller (sys) that allows to configure
> +      xSPI CS0 and CS1 addresses.
> +
> +patternProperties:
> +  "flash@[0-9a-f]+$":
> +    type: object
> +    additionalProperties: true
> +
> +    properties:
> +      compatible:
> +        contains:
> +          const: jedec,spi-nor
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - resets
> +  - reset-names
> +  - '#address-cells'
> +  - '#size-cells'
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/renesas-cpg-mssr.h>
> +
> +    spi@11030000 {
> +        compatible = "renesas,r9a09g047-xspi";
> +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> +        reg-names = "regs", "dirmap";
> +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> +        interrupt-names = "pulse", "err_pulse";
> +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;

On the next version I am going to update spix2 clk as
<&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>

Based on [1], the clk specifier cannot distinguish between
spi and spix2 clk, as entries are same(gating bits). So, treating
spix2 as core clock to distinguish them.

Please let me know if there are any issues in this approach?

[1] https://lore.kernel.org/all/TY3PR01MB11346B3B6CFF1359411B475A386A62@TY3PR01MB11346.jpnprd01.prod.outlook.com/

Cheers,
Biju

> +        clock-names = "ahb", "axi", "spi", "spix2";
> +        power-domains = <&cpg>;
> +        resets = <&cpg 0xa3>, <&cpg 0xa4>;
> +        reset-names = "hresetn", "aresetn";
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        flash@0 {
> +          compatible = "jedec,spi-nor";
> +          reg = <0>;
> +          spi-max-frequency = <40000000>;
> +          spi-tx-bus-width = <1>;
> +          spi-rx-bus-width = <1>;
> +        };
> +    };
> --
> 2.43.0

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Biju,

On Mon, 31 Mar 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Biju Das <biju.das.jz@bp.renesas.com>
> > Document support for the Expanded Serial Peripheral Interface (xSPI) Controller in the Renesas RZ/G3E
> > (R9A09G047) SoC.
> >
> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/renesas,rzg3e
> > +++ -xspi.yaml

> > +    spi@11030000 {
> > +        compatible = "renesas,r9a09g047-xspi";
> > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > +        reg-names = "regs", "dirmap";
> > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > +        interrupt-names = "pulse", "err_pulse";
> > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
>
> On the next version I am going to update spix2 clk as
> <&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>

What's spix2 clk? Ah, re-adding dropped line:

> > +        clock-names = "ahb", "axi", "spi", "spix2";

> Based on [1], the clk specifier cannot distinguish between
> spi and spix2 clk, as entries are same(gating bits). So, treating
> spix2 as core clock to distinguish them.
>
> Please let me know if there are any issues in this approach?

As you wrote in [2], you have to check the two monitor register bits
together. How do you plan to handle that requirement?

> [1] https://lore.kernel.org/all/TY3PR01MB11346B3B6CFF1359411B475A386A62@TY3PR01MB11346.jpnprd01.prod.outlook.com/

[2] https://lore.kernel.org/all/TY3PR01MB11346D2881A8CC9C3019C978386D22@TY3PR01MB11346.jpnprd01.prod.outlook.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 31 March 2025 15:12
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Hi Biju,
> 
> On Mon, 31 Mar 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for the
> > > Expanded Serial Peripheral Interface (xSPI) Controller in the
> > > Renesas RZ/G3E
> > > (R9A09G047) SoC.
> > >
> > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> 
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/memory-controllers/renesas,r
> > > +++ zg3e
> > > +++ -xspi.yaml
> 
> > > +    spi@11030000 {
> > > +        compatible = "renesas,r9a09g047-xspi";
> > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > +        reg-names = "regs", "dirmap";
> > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > +        interrupt-names = "pulse", "err_pulse";
> > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> >
> > On the next version I am going to update spix2 clk as <&cpg CPG_CORE
> > R9A09G047_SPI_CLK_SPIX2>
> 
> What's spix2 clk? Ah, re-adding dropped line:
> 
> > > +        clock-names = "ahb", "axi", "spi", "spix2";
> 
> > Based on [1], the clk specifier cannot distinguish between spi and
> > spix2 clk, as entries are same(gating bits). So, treating
> > spix2 as core clock to distinguish them.
> >
> > Please let me know if there are any issues in this approach?
> 
> As you wrote in [2], you have to check the two monitor register bits together. How do you plan to
> handle that requirement?

As per hardware team, spix2 clock is twice the frequency of the spi clock, and the clock ON/OFF period
displayed for each bit in the monitor register varies slightly due to the difference in frequency. 

So, if I monitor the bit of slowest clock(spi) that will confirm both right?

Cheers,
Biju

> 
> > [1]
> > https://lore.kernel.org/all/TY3PR01MB11346B3B6CFF1359411B475A386A62@TY
> > 3PR01MB11346.jpnprd01.prod.outlook.com/
> 
> [2]
> https://lore.kernel.org/all/TY3PR01MB11346D2881A8CC9C3019C978386D22@TY3PR01MB11346.jpnprd01.prod.outlo
> ok.com
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But when I'm talking to
> journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Biju,

On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > On Mon, 31 Mar 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for the
> > > > Expanded Serial Peripheral Interface (xSPI) Controller in the
> > > > Renesas RZ/G3E
> > > > (R9A09G047) SoC.
> > > >
> > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renesas,r
> > > > +++ zg3e
> > > > +++ -xspi.yaml
> >
> > > > +    spi@11030000 {
> > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > +        reg-names = "regs", "dirmap";
> > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > +        interrupt-names = "pulse", "err_pulse";
> > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > >
> > > On the next version I am going to update spix2 clk as <&cpg CPG_CORE
> > > R9A09G047_SPI_CLK_SPIX2>
> >
> > What's spix2 clk? Ah, re-adding dropped line:
> >
> > > > +        clock-names = "ahb", "axi", "spi", "spix2";
> >
> > > Based on [1], the clk specifier cannot distinguish between spi and
> > > spix2 clk, as entries are same(gating bits). So, treating
> > > spix2 as core clock to distinguish them.
> > >
> > > Please let me know if there are any issues in this approach?
> >
> > As you wrote in [2], you have to check the two monitor register bits together. How do you plan to
> > handle that requirement?
>
> As per hardware team, spix2 clock is twice the frequency of the spi clock, and the clock ON/OFF period
> displayed for each bit in the monitor register varies slightly due to the difference in frequency.
>
> So, if I monitor the bit of slowest clock(spi) that will confirm both right?

(perhaps naively) I would assume so, too.

Bute then why did you (or the hardware team) write:

   "So to check the status after changing the clock ON/OFF register setting,
    please check the two monitor register bits together".

???

> > > [1]
> > > https://lore.kernel.org/all/TY3PR01MB11346B3B6CFF1359411B475A386A62@TY3PR01MB11346.jpnprd01.prod.outlook.com/
> >
> > [2]
> > https://lore.kernel.org/all/TY3PR01MB11346D2881A8CC9C3019C978386D22@TY3PR01MB11346.jpnprd01.prod.outlook.com

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 31 March 2025 16:27
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Hi Biju,
> 
> On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > the Renesas RZ/G3E
> > > > > (R9A09G047) SoC.
> > > > >
> > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > >
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > +++ as,r
> > > > > +++ zg3e
> > > > > +++ -xspi.yaml
> > >
> > > > > +    spi@11030000 {
> > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > +        reg-names = "regs", "dirmap";
> > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > >
> > > > On the next version I am going to update spix2 clk as <&cpg
> > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > >
> > > What's spix2 clk? Ah, re-adding dropped line:
> > >
> > > > > +        clock-names = "ahb", "axi", "spi", "spix2";
> > >
> > > > Based on [1], the clk specifier cannot distinguish between spi and
> > > > spix2 clk, as entries are same(gating bits). So, treating
> > > > spix2 as core clock to distinguish them.
> > > >
> > > > Please let me know if there are any issues in this approach?
> > >
> > > As you wrote in [2], you have to check the two monitor register bits
> > > together. How do you plan to handle that requirement?
> >
> > As per hardware team, spix2 clock is twice the frequency of the spi
> > clock, and the clock ON/OFF period displayed for each bit in the monitor register varies slightly
> due to the difference in frequency.
> >
> > So, if I monitor the bit of slowest clock(spi) that will confirm both right?
> 
> (perhaps naively) I would assume so, too.
> 
> Bute then why did you (or the hardware team) write:
> 
>    "So to check the status after changing the clock ON/OFF register setting,
>     please check the two monitor register bits together".

Basically, It is feedback from hardware team.

<snippet>
There is no use case in which each bit in the monitor register is used independently, 
so as you pointed out, I think it would have been better to bundle them into one bit,
like the clock ON/OFF register. Note that the spix2 clock is twice the frequency of the spi clock,
and the clock ON/OFF period displayed for each bit in the monitor register varies slightly due to the difference in frequency. 
To check the status after changing the clock ON/OFF register setting, please check the two monitor register bits together.
</snippet>


Cheers,
Biju

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Biju,

On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > the Renesas RZ/G3E
> > > > > > (R9A09G047) SoC.
> > > > > >
> > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > >
> > > > > > --- /dev/null
> > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > +++ as,r
> > > > > > +++ zg3e
> > > > > > +++ -xspi.yaml
> > > >
> > > > > > +    spi@11030000 {
> > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > +        reg-names = "regs", "dirmap";
> > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > >
> > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>

According to the RZ/G3E clock system diagram, (the parent of) clk_spi
is derived from (the parent of) clk_spix2, not the other way around?
So you can model clk_spi as a fixed divider clock with parent clk_spix2
and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
instead of your proposed R9A09G047_SPI_CLK_SPIX2?

> > > > What's spix2 clk? Ah, re-adding dropped line:
> > > >
> > > > > > +        clock-names = "ahb", "axi", "spi", "spix2";
> > > >
> > > > > Based on [1], the clk specifier cannot distinguish between spi and
> > > > > spix2 clk, as entries are same(gating bits). So, treating
> > > > > spix2 as core clock to distinguish them.
> > > > >
> > > > > Please let me know if there are any issues in this approach?
> > > >
> > > > As you wrote in [2], you have to check the two monitor register bits
> > > > together. How do you plan to handle that requirement?
> > >
> > > As per hardware team, spix2 clock is twice the frequency of the spi
> > > clock, and the clock ON/OFF period displayed for each bit in the monitor register varies slightly
> > due to the difference in frequency.
> > >
> > > So, if I monitor the bit of slowest clock(spi) that will confirm both right?
> >
> > (perhaps naively) I would assume so, too.
> >
> > Bute then why did you (or the hardware team) write:
> >
> >    "So to check the status after changing the clock ON/OFF register setting,
> >     please check the two monitor register bits together".
>
> Basically, It is feedback from hardware team.
>
> <snippet>
> There is no use case in which each bit in the monitor register is used independently,
> so as you pointed out, I think it would have been better to bundle them into one bit,
> like the clock ON/OFF register. Note that the spix2 clock is twice the frequency of the spi clock,

OK, so one bit would have been fine...

> and the clock ON/OFF period displayed for each bit in the monitor register varies slightly due to the difference in frequency.
> To check the status after changing the clock ON/OFF register setting, please check the two monitor register bits together.

... except that this part says to check both.

> </snippet>

Oh well...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 31 March 2025 19:24
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Hi Biju,
> 
> On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar
> > > > > 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support
> > > > > > > for the Expanded Serial Peripheral Interface (xSPI)
> > > > > > > Controller in the Renesas RZ/G3E
> > > > > > > (R9A09G047) SoC.
> > > > > > >
> > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > >
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/r
> > > > > > > +++ enes
> > > > > > > +++ as,r
> > > > > > > +++ zg3e
> > > > > > > +++ -xspi.yaml
> > > > >
> > > > > > > +    spi@11030000 {
> > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > >
> > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> 
> According to the RZ/G3E clock system diagram, (the parent of) clk_spi is derived from (the parent of)
> clk_spix2, not the other way around?
> So you can model clk_spi as a fixed divider clock with parent clk_spix2 and factor two.  I.e. provide
> a new core clock R9A09G047_SPI_CLK_SPI instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> 
> > > > > What's spix2 clk? Ah, re-adding dropped line:
> > > > >
> > > > > > > +        clock-names = "ahb", "axi", "spi", "spix2";
> > > > >
> > > > > > Based on [1], the clk specifier cannot distinguish between spi
> > > > > > and
> > > > > > spix2 clk, as entries are same(gating bits). So, treating
> > > > > > spix2 as core clock to distinguish them.
> > > > > >
> > > > > > Please let me know if there are any issues in this approach?
> > > > >
> > > > > As you wrote in [2], you have to check the two monitor register
> > > > > bits together. How do you plan to handle that requirement?
> > > >
> > > > As per hardware team, spix2 clock is twice the frequency of the
> > > > spi clock, and the clock ON/OFF period displayed for each bit in
> > > > the monitor register varies slightly
> > > due to the difference in frequency.
> > > >
> > > > So, if I monitor the bit of slowest clock(spi) that will confirm both right?
> > >
> > > (perhaps naively) I would assume so, too.
> > >
> > > Bute then why did you (or the hardware team) write:
> > >
> > >    "So to check the status after changing the clock ON/OFF register setting,
> > >     please check the two monitor register bits together".
> >
> > Basically, It is feedback from hardware team.
> >
> > <snippet>
> > There is no use case in which each bit in the monitor register is used
> > independently, so as you pointed out, I think it would have been
> > better to bundle them into one bit, like the clock ON/OFF register.
> > Note that the spix2 clock is twice the frequency of the spi clock,
> 
> OK, so one bit would have been fine...
> 
> > and the clock ON/OFF period displayed for each bit in the monitor register varies slightly due to
> the difference in frequency.
> > To check the status after changing the clock ON/OFF register setting, please check the two monitor
> register bits together.
> 
> ... except that this part says to check both.

Can you please share your thoughts to handle this?

1) Gate only spi clk
2) For monitoring use both clock
3) Clock specifier needs two distinct entries. So that consumer will get
   proper rates for both clocks.

Cheers,
Biju

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Biju,

On Mon, 31 Mar 2025 at 20:29, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar
> > > > > > 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support
> > > > > > > > for the Expanded Serial Peripheral Interface (xSPI)
> > > > > > > > Controller in the Renesas RZ/G3E
> > > > > > > > (R9A09G047) SoC.
> > > > > > > >
> > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > >
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/r
> > > > > > > > +++ enes
> > > > > > > > +++ as,r
> > > > > > > > +++ zg3e
> > > > > > > > +++ -xspi.yaml
> > > > > >
> > > > > > > > +    spi@11030000 {
> > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > >
> > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> >
> > According to the RZ/G3E clock system diagram, (the parent of) clk_spi is derived from (the parent of)
> > clk_spix2, not the other way around?
> > So you can model clk_spi as a fixed divider clock with parent clk_spix2 and factor two.  I.e. provide
> > a new core clock R9A09G047_SPI_CLK_SPI instead of your proposed R9A09G047_SPI_CLK_SPIX2?
    ^^^^

> > > > > > What's spix2 clk? Ah, re-adding dropped line:
> > > > > >
> > > > > > > > +        clock-names = "ahb", "axi", "spi", "spix2";

> Can you please share your thoughts to handle this?

See above ^^^^ ;-)

> 1) Gate only spi clk

Gate only clk_spix2, which is the parent of clk_spi.
So enabling any of them will (propagate to) enable clk_spix2,
which uses the hardware gate.

> 2) For monitoring use both clock

Check only clk_spix2 for monitoring.

> 3) Clock specifier needs two distinct entries. So that consumer will get
>    proper rates for both clocks.

clk_spi would be a separate fixed-divider clock.

Does that make sense?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 31 March 2025 20:05
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Hi Biju,
> 
> On Mon, 31 Mar 2025 at 20:29, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar
> > > > > 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31
> > > > > > > Mar
> > > > > > > 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document
> > > > > > > > > support for the Expanded Serial Peripheral Interface
> > > > > > > > > (xSPI) Controller in the Renesas RZ/G3E
> > > > > > > > > (R9A09G047) SoC.
> > > > > > > > >
> > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > >
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controlle
> > > > > > > > > +++ rs/r
> > > > > > > > > +++ enes
> > > > > > > > > +++ as,r
> > > > > > > > > +++ zg3e
> > > > > > > > > +++ -xspi.yaml
> > > > > > >
> > > > > > > > > +    spi@11030000 {
> > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD
> > > > > > > > > + 0xa1>;
> > > > > > > >
> > > > > > > > On the next version I am going to update spix2 clk as
> > > > > > > > <&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > >
> > > According to the RZ/G3E clock system diagram, (the parent of)
> > > clk_spi is derived from (the parent of) clk_spix2, not the other way around?
> > > So you can model clk_spi as a fixed divider clock with parent
> > > clk_spix2 and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI instead of your
> proposed R9A09G047_SPI_CLK_SPIX2?
>     ^^^^

OK.

> 
> > > > > > > What's spix2 clk? Ah, re-adding dropped line:
> > > > > > >
> > > > > > > > > +        clock-names = "ahb", "axi", "spi", "spix2";
> 
> > Can you please share your thoughts to handle this?
> 
> See above ^^^^ ;-)
> 
> > 1) Gate only spi clk
> 
> Gate only clk_spix2, which is the parent of clk_spi.
> So enabling any of them will (propagate to) enable clk_spix2, which uses the hardware gate.

Agreed.

> 
> > 2) For monitoring use both clock
> 
> Check only clk_spix2 for monitoring.

Ok.

> 
> > 3) Clock specifier needs two distinct entries. So that consumer will get
> >    proper rates for both clocks.
> 
> clk_spi would be a separate fixed-divider clock.

Agreed.

Cheers,
Biju

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Lad, Prabhakar]

Hi Geert,

On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Biju,
>
> On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > the Renesas RZ/G3E
> > > > > > > (R9A09G047) SoC.
> > > > > > >
> > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > >
> > > > > > > --- /dev/null
> > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > +++ as,r
> > > > > > > +++ zg3e
> > > > > > > +++ -xspi.yaml
> > > > >
> > > > > > > +    spi@11030000 {
> > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > >
> > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
>
> According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> is derived from (the parent of) clk_spix2, not the other way around?
> So you can model clk_spi as a fixed divider clock with parent clk_spix2
> and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> instead of your proposed R9A09G047_SPI_CLK_SPIX2?
>
With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
and XSPI node is disabled the clk_summary reports the core clock is ON
(while it's actually OFF).

root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
          .smux2_xspi_clk1           0       0        0
320000000   0          0     50000      Y            deviceless
              no_connection_id
             .pllcm33_xspi           0       0        0
40000000    0          0     50000      Y               deviceless
                 no_connection_id
                spi_clk_spix2        0       0        0
40000000    0          0     50000      N                  deviceless
                    no_connection_id
                spi_clk_spi          0       0        0
20000000    0          0     50000      Y                  deviceless
                    no_connection_id
             spi_aclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
             spi_hclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
          .smux2_xspi_clk0           0       0        0
533333333   0          0     50000      Y            deviceless
              no_connection_id

Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
0x190) and represent this is a module clock for example for the
spi_clk_spix2 clock and use this in the DT and let the CPG core code
handle such turning ON/OF the module clocks based on the enable count
which will be handled internally in the driver?

I have some POC code for the paired clocks which will handle the
enable count of the paired module clocks, below is the diff. Please
share your thoughts.

diff --git a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
index 3c5984ee27ca..8a8f59ffdb4c 100644
--- a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
@@ -290,10 +290,10 @@ xspi: spi@11030000 {
                        interrupt-names = "pulse", "err_pulse";
                        clocks = <&cpg CPG_MOD 0x9f>,
                                 <&cpg CPG_MOD 0xa0>,
-                                <&cpg CPG_CORE R9A09G057_SPI_CLK_SPI>,
-                                <&cpg CPG_MOD 0xa1>;
+                                <&cpg CPG_MOD 0xa1>,
+                                <&cpg CPG_MOD 0x190>;
                        clock-names = "ahb", "axi", "spi", "spix2";
-                       assigned-clocks = <&cpg CPG_CORE R9A09G057_SPI_CLK_SPI>;
+                       assigned-clocks = <&cpg CPG_MOD 0xa1>;
                        assigned-clock-rates = <133333334>;
                        resets = <&cpg 0xa3>, <&cpg 0xa4>;
                        reset-names = "hresetn", "aresetn";
diff --git a/drivers/clk/renesas/r9a09g057-cpg.c
b/drivers/clk/renesas/r9a09g057-cpg.c
index 9952474bcf48..d5eef17ad5fc 100644
--- a/drivers/clk/renesas/r9a09g057-cpg.c
+++ b/drivers/clk/renesas/r9a09g057-cpg.c
@@ -43,6 +43,7 @@ enum clk_ids {
        CLK_SMUX2_XSPI_CLK0,
        CLK_SMUX2_XSPI_CLK1,
        CLK_PLLCM33_XSPI,
+       CLK_PLLCM33_XSPIX2,
        CLK_PLLCLN_DIV2,
        CLK_PLLCLN_DIV8,
        CLK_PLLCLN_DIV16,
@@ -176,6 +177,7 @@ static const struct cpg_core_clk
r9a09g057_core_clks[] __initconst = {
        DEF_SMUX(".smux2_xspi_clk1", CLK_SMUX2_XSPI_CLK1,
SSEL1_SELCTL3, smux2_xspi_clk1),
        DEF_CSDIV(".pllcm33_xspi", CLK_PLLCM33_XSPI,
CLK_SMUX2_XSPI_CLK1, CSDIV0_DIVCTL3,
                  dtable_2_16),
+       DEF_FIXED(".pllcm33_xspix2", CLK_PLLCM33_XSPIX2,
CLK_PLLCM33_XSPI, 2, 1),

        DEF_FIXED(".pllcln_div2", CLK_PLLCLN_DIV2, CLK_PLLCLN, 1, 2),
        DEF_FIXED(".pllcln_div8", CLK_PLLCLN_DIV8, CLK_PLLCLN, 1, 8),
@@ -231,7 +233,6 @@ static const struct cpg_core_clk
r9a09g057_core_clks[] __initconst = {
                  CLK_PLLETH_DIV_125_FIX, 1, 1),
        DEF_FIXED("gbeth_1_clk_ptp_ref_i", R9A09G057_GBETH_1_CLK_PTP_REF_I,
                  CLK_PLLETH_DIV_125_FIX, 1, 1),
-       DEF_FIXED("spi_clk_spi", R9A09G057_SPI_CLK_SPI, CLK_PLLCM33_XSPI, 1, 2),
 };

 static const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst = {
@@ -311,8 +312,10 @@ static const struct rzv2h_mod_clk
r9a09g057_mod_clks[] __initconst = {
                                                BUS_MSTOP(4, BIT(5))),
        DEF_MOD("spi_aclk",                     CLK_PLLCM33_GEAR, 10, 0, 5, 0,
                                                BUS_MSTOP(4, BIT(5))),
-       DEF_MOD_NO_PM("spi_clk_spix2",          CLK_PLLCM33_XSPI, 10, 1, 5, 2,
+       DEF_MOD_PAIRED("spi_clk_spi",           CLK_PLLCM33_XSPI, 10, 1, 5, 1,
                                                BUS_MSTOP(4, BIT(5))),
+       DEF_MOD_PAIRED_ALIAS("spi_clk_spix2",   CLK_PLLCM33_XSPIX2, 25, 0, 5, 2,
+                                               BUS_MSTOP(4, BIT(5)), 10, 1),
        DEF_MOD("sdhi_0_imclk",                 CLK_PLLCLN_DIV8, 10, 3, 5, 3,
                                                BUS_MSTOP(8, BIT(2))),
        DEF_MOD("sdhi_0_imclk2",                CLK_PLLCLN_DIV8, 10, 4, 5, 4,
@@ -508,7 +511,7 @@ const struct rzv2h_cpg_info r9a09g057_cpg_info
__initconst = {
        /* Module Clocks */
        .mod_clks = r9a09g057_mod_clks,
        .num_mod_clks = ARRAY_SIZE(r9a09g057_mod_clks),
-       .num_hw_mod_clks = 25 * 16,
+       .num_hw_mod_clks = 26 * 16,

        /* Resets */
        .resets = r9a09g057_resets,
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index 97bcd252fcbf..847ea71df865 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -71,6 +71,12 @@

 #define CPG_CLKSTATUS0         (0x700)

+struct rzv2h_paired_clock {
+       u8 on_index;
+       u8 on_bit;
+       unsigned int enabled_count;
+};
+
 /**
  * struct rzv2h_cpg_priv - Clock Pulse Generator Private Data
  *
@@ -87,6 +93,8 @@
  * @rcdev: Reset controller entity
  * @dsi_limits: PLL DSI parameters limits
  * @plldsi_div_parameters: PLL DSI and divider parameters configuration
+ * @pair_clks: Array of paired clocks
+ * @num_pair_clks: Number of paired clocks in pair_clks[]
  */
 struct rzv2h_cpg_priv {
        struct device *dev;
@@ -102,6 +110,9 @@ struct rzv2h_cpg_priv {

        atomic_t *mstop_count;

+       struct rzv2h_paired_clock *pair_clks;
+       unsigned int num_pair_clks;
+
        struct reset_controller_dev rcdev;

        const struct rzv2h_pll_div_limits *dsi_limits;
@@ -131,6 +142,11 @@ struct pll_clk {
  * @mon_index: monitor register offset
  * @mon_bit: monitor bit
  * @ext_clk_mux_index: mux index for external clock source, or -1 if internal
+ * @paired: indicates if this clock is paired with another clock ie it shares
+ *          the same ON bit and has a separate MON bit.
+ * @alias: indicates if this clock is paired and has been given dummy ON bit
+ * @pair_clk: pointer to paired clock structure if this clock is paired,
+ *           otherwise NULL.
  */
 struct mod_clock {
        struct rzv2h_cpg_priv *priv;
@@ -142,6 +158,9 @@ struct mod_clock {
        s8 mon_index;
        u8 mon_bit;
        s8 ext_clk_mux_index;
+       bool paired;
+       bool alias;
+       struct rzv2h_paired_clock *pair_clk;
 };

 #define to_mod_clock(_hw) container_of(_hw, struct mod_clock, hw)
@@ -853,8 +872,13 @@ static int rzv2h_mod_clock_is_enabled(struct clk_hw *hw)
                        return 0;
        }

-       offset = GET_CLK_ON_OFFSET(clock->on_index);
-       bitmask = BIT(clock->on_bit);
+       if (clock->alias) {
+               offset = GET_CLK_ON_OFFSET(clock->pair_clk->on_index);
+               bitmask = BIT(clock->pair_clk->on_bit);
+       } else {
+               offset = GET_CLK_ON_OFFSET(clock->on_index);
+               bitmask = BIT(clock->on_bit);
+       }

        return readl(priv->base + offset) & bitmask;
 }
@@ -870,24 +894,57 @@ static int rzv2h_mod_clock_endisable(struct
clk_hw *hw, bool enable)
        u32 value;
        int error;

-       dev_dbg(dev, "CLK_ON 0x%x/%pC %s\n", reg, hw->clk,
-               str_on_off(enable));
+       if (clock->alias) {
+               reg = GET_CLK_ON_OFFSET(clock->pair_clk->on_index);
+               bitmask = BIT(clock->pair_clk->on_bit);
+       }

-       if (enabled == enable)
+       if (clock->paired)
+               dev_err(dev, "CLK_ON 0x%x/%pC %s\n", reg, hw->clk,
+                       str_on_off(enable));
+
+       if (enabled == enable) {
+               if (clock->paired && enable && clock->pair_clk->enabled_count) {
+                       clock->pair_clk->enabled_count++;
+                       dev_err(dev, "1: CLK_ON 0x%x/%pC %s cnt:%u\n",
+                               reg, hw->clk, str_on_off(enable),
+                               clock->pair_clk->enabled_count);
+               }
                return 0;
+       }

        value = bitmask << 16;
        if (enable) {
+               if (clock->paired && clock->pair_clk->enabled_count) {
+                       clock->pair_clk->enabled_count++;
+                       dev_err(dev, "2: CLK_ON 0x%x/%pC %s cnt:%u\n",
+                               reg, hw->clk, str_on_off(enable),
+                               clock->pair_clk->enabled_count);
+                       goto check_mon;
+               }
                value |= bitmask;
                writel(value, priv->base + reg);
                if (clock->mstop_data != BUS_MSTOP_NONE)
                        rzv2h_mod_clock_mstop_enable(priv, clock->mstop_data);
+               if (clock->paired)
+                       clock->pair_clk->enabled_count++;
        } else {
+               if (clock->paired) {
+                       if (clock->pair_clk->enabled_count)
+                               clock->pair_clk->enabled_count--;
+                       dev_err(dev, "3: CLK_ON 0x%x/%pC %s cnt:%u\n",
+                               reg, hw->clk, str_on_off(enable),
+                               clock->pair_clk->enabled_count);
+                       if (clock->pair_clk->enabled_count > 0)
+                               goto check_mon;
+               }
+
                if (clock->mstop_data != BUS_MSTOP_NONE)
                        rzv2h_mod_clock_mstop_disable(priv, clock->mstop_data);
                writel(value, priv->base + reg);
        }

+check_mon:
        if (!enable || clock->mon_index < 0)
                return 0;

@@ -918,6 +975,55 @@ static const struct clk_ops rzv2h_mod_clock_ops = {
        .is_enabled = rzv2h_mod_clock_is_enabled,
 };

+static int rzv2h_mod_clk_get_paired_cfg(struct rzv2h_cpg_priv *priv,
+                                       struct mod_clock *clock,
+                                       const struct rzv2h_mod_clk *mod)
+{
+       unsigned int i;
+       u8 on_index;
+       u8 on_bit;
+
+
+       if (!clock->paired)
+               return 0;
+
+       /* Get the paired clock configuration */
+       if (clock->alias) {
+               on_index = mod->alias_on_index;
+               on_bit = mod->alias_on_bit;
+       } else {
+               on_index = clock->on_index;
+               on_bit = clock->on_bit;
+       }
+
+       for (i = 0; i < priv->num_pair_clks; i++) {
+               if (priv->pair_clks[i].on_index == on_index &&
+                   priv->pair_clks[i].on_bit == on_bit) {
+                       clock->pair_clk = &priv->pair_clks[i];
+                       pr_err("cpg: 2: register paired clock %s
on_index %u on_bit %u num:%u\n",
+                       clock->hw.init->name, on_index, on_bit,
+                       priv->num_pair_clks);
+                       return 0;
+               }
+       }
+
+       priv->num_pair_clks++;
+       priv->pair_clks = devm_krealloc(priv->dev, priv->pair_clks,
+                                       sizeof(*priv->pair_clks) *
priv->num_pair_clks, GFP_KERNEL);
+       if (!priv->pair_clks)
+               return -ENOMEM;
+
+       pr_err("cpg: register paired clock %s on_index %u on_bit %u num:%u\n",
+              clock->hw.init->name, on_index, on_bit,
+              priv->num_pair_clks);
+       priv->pair_clks[priv->num_pair_clks - 1].on_index = on_index;
+       priv->pair_clks[priv->num_pair_clks - 1].on_bit = on_bit;
+       priv->pair_clks[priv->num_pair_clks - 1].enabled_count = 0;
+       clock->pair_clk = &priv->pair_clks[priv->num_pair_clks - 1];
+
+       return 0;
+}
+
 static void __init
 rzv2h_cpg_register_mod_clk(const struct rzv2h_mod_clk *mod,
                           struct rzv2h_cpg_priv *priv)
@@ -966,7 +1072,13 @@ rzv2h_cpg_register_mod_clk(const struct
rzv2h_mod_clk *mod,
        clock->priv = priv;
        clock->hw.init = &init;
        clock->mstop_data = mod->mstop_data;
-
+       clock->paired = mod->paired;
+       clock->alias = mod->alias;
+       ret = rzv2h_mod_clk_get_paired_cfg(priv, clock, mod);
+       if (ret) {
+               clk = ERR_PTR(ret);
+               goto fail;
+       }
        ret = devm_clk_hw_register(dev, &clock->hw);
        if (ret) {
                clk = ERR_PTR(ret);
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index bce131bec80b..87b62fc908ba 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -223,6 +223,11 @@ enum clk_types {
  * @mon_index: monitor register index
  * @mon_bit: monitor bit
  * @ext_clk_mux_index: mux index for external clock source, or -1 if internal
+ * @paired: indicates if this clock is paired with another clock,
i.e., it shares
+ *          the same ON bit and has a separate MON bit.
+ * @alias: indicates if this clock is paired and has been given dummy ON bit
+ * @alias_on_index: For paired clocks, the index of the alias ON bit
+ * @alias_on_bit: For paired clocks, the index of the alias ON bit
  */
 struct rzv2h_mod_clk {
        const char *name;
@@ -235,10 +240,15 @@ struct rzv2h_mod_clk {
        s8 mon_index;
        u8 mon_bit;
        s8 ext_clk_mux_index;
+       bool paired;
+       bool alias;
+       u8 alias_on_index;
+       u8 alias_on_bit;
 };

 #define DEF_MOD_BASE(_name, _mstop, _parent, _critical, _no_pm, _onindex, \
-                    _onbit, _monindex, _monbit, _ext_clk_mux_index) \
+                    _onbit, _monindex, _monbit, _ext_clk_mux_index, _paired, \
+                    _alias, _alias_on_index, _alias_on_bit) \
        { \
                .name = (_name), \
                .mstop_data = (_mstop), \
@@ -250,23 +260,40 @@ struct rzv2h_mod_clk {
                .mon_index = (_monindex), \
                .mon_bit = (_monbit), \
                .ext_clk_mux_index = (_ext_clk_mux_index), \
+               .paired = (_paired), \
+               .alias = (_alias), \
+               .alias_on_index = (_alias_on_index), \
+               .alias_on_bit = (_alias_on_bit), \
        }

 #define DEF_MOD(_name, _parent, _onindex, _onbit, _monindex, _monbit, _mstop) \
-       DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex,
_onbit, _monindex, _monbit, -1)
+       DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex, _onbit, \
+                    _monindex, _monbit, -1, false, false, 0, 0)

 #define DEF_MOD_CRITICAL(_name, _parent, _onindex, _onbit, _monindex,
_monbit, _mstop) \
-       DEF_MOD_BASE(_name, _mstop, _parent, true, false, _onindex,
_onbit, _monindex, _monbit, -1)
+       DEF_MOD_BASE(_name, _mstop, _parent, true, false, _onindex,
_onbit, _monindex, \
+                    _monbit, -1, false, false, 0, 0)

 #define DEF_MOD_NO_PM(_name, _parent, _onindex, _onbit, _monindex,
_monbit, _mstop) \
-       DEF_MOD_BASE(_name, _mstop, _parent, false, true, _onindex,
_onbit, _monindex, _monbit, -1)
+       DEF_MOD_BASE(_name, _mstop, _parent, false, true, _onindex,
_onbit, _monindex, \
+                    _monbit, -1, false, false, 0, 0)

 #define DEF_MOD_MUX_EXTERNAL(_name, _parent, _onindex, _onbit,
_monindex, _monbit, _mstop, \
                             _ext_clk_mux_index) \
        DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex,
_onbit, _monindex, _monbit, \
-                    _ext_clk_mux_index)
+                    _ext_clk_mux_index, false, false, 0, 0)

-/**
+#define DEF_MOD_PAIRED(_name, _parent, _onindex, _onbit, _monindex,
_monbit, _mstop) \
+       DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex,
_onbit, _monindex, \
+                    _monbit, -1, true, false, 0, 0)
+
+#define DEF_MOD_PAIRED_ALIAS(_name, _parent, _onindex, _onbit,
_monindex, _monbit, _mstop, \
+                            _alias_on_index, _alias_on_bit) \
+       DEF_MOD_BASE(_name, _mstop, _parent, false, false, _onindex,
_onbit, _monindex, _monbit, -1, \
+                    true, true, _alias_on_index, _alias_on_bit)
+
+
+ /**
  * struct rzv2h_reset - Reset definitions
  *
  * @reset_index: reset register index



# Output when module is loaded
root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
             spi_aclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
             spi_hclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
          .smux2_xspi_clk0           1       1        0
533333333   0          0     50000      Y            deviceless
              no_connection_id
             .smux2_xspi_clk1        1       1        0
533333333   0          0     50000      Y               deviceless
                 no_connection_id
                .pllcm33_xspi        2       2        0
133333334   0          0     50000      Y                  deviceless
                    no_connection_id
                   spi_clk_spi       1       2        0
133333334   0          0     50000      Y
11030000.spi                    spi
                   .pllcm33_xspix2   1       1        0
266666668   0          0     50000      Y
deviceless                      no_connection_id
                      spi_clk_spix2  1       2        0
266666668   0          0     50000      Y
11030000.spi                    spix2


# Output when module is unloaded
root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
             spi_aclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
             spi_hclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
          .smux2_xspi_clk0           0       0        0
533333333   0          0     50000      Y            deviceless
              no_connection_id
             .smux2_xspi_clk1        0       0        0
533333333   0          0     50000      Y               deviceless
                 no_connection_id
                .pllcm33_xspi        0       0        0
133333334   0          0     50000      Y                  deviceless
                    no_connection_id
                   spi_clk_spi       0       0        0
133333334   0          0     50000      N
deviceless                      no_connection_id
                   .pllcm33_xspix2   0       0        0
266666668   0          0     50000      Y
deviceless                      no_connection_id
                      spi_clk_spix2  0       0        0
266666668   0          0     50000      N
deviceless                      no_connection_id
root@rzv2h-evk:~#
root@rzv2h-evk:~#


Cheers,
Prabhakar

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi Prabhakar,

> -----Original Message-----
> From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> Sent: 17 June 2025 22:05
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Hi Geert,
> 
> On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> >
> > Hi Biju,
> >
> > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar
> > > > 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar
> > > > > > 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document
> > > > > > > > support for the Expanded Serial Peripheral Interface
> > > > > > > > (xSPI) Controller in the Renesas RZ/G3E
> > > > > > > > (R9A09G047) SoC.
> > > > > > > >
> > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > >
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers
> > > > > > > > +++ /renes
> > > > > > > > +++ as,r
> > > > > > > > +++ zg3e
> > > > > > > > +++ -xspi.yaml
> > > > > >
> > > > > > > > +    spi@11030000 {
> > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD
> > > > > > > > + 0xa1>;
> > > > > > >
> > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> >
> > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > is derived from (the parent of) clk_spix2, not the other way around?
> > So you can model clk_spi as a fixed divider clock with parent
> > clk_spix2 and factor two.  I.e. provide a new core clock
> > R9A09G047_SPI_CLK_SPI instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> >
> With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock and XSPI node is disabled the
> clk_summary reports the core clock is ON (while it's actually OFF).
> 
> root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
>           .smux2_xspi_clk1           0       0        0
> 320000000   0          0     50000      Y            deviceless
>               no_connection_id
>              .pllcm33_xspi           0       0        0
> 40000000    0          0     50000      Y               deviceless
>                  no_connection_id
>                 spi_clk_spix2        0       0        0
> 40000000    0          0     50000      N                  deviceless
>                     no_connection_id
>                 spi_clk_spi          0       0        0
> 20000000    0          0     50000      Y                  deviceless
>                     no_connection_id
>              spi_aclk                0       0        0
> 200000000   0          0     50000      N               deviceless
>                  no_connection_id
>              spi_hclk                0       0        0
> 200000000   0          0     50000      N               deviceless
>                  no_connection_id
>           .smux2_xspi_clk0           0       0        0
> 533333333   0          0     50000      Y            deviceless
>               no_connection_id
> 
> Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> 0x190) and represent this is a module clock for example for the
> spi_clk_spix2 clock and use this in the DT and let the CPG core code handle such turning ON/OF the
> module clocks based on the enable count which will be handled internally in the driver?
> 
> I have some POC code for the paired clocks which will handle the enable count of the paired module
> clocks, below is the diff. Please share your thoughts.
> 
> diff --git a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> index 3c5984ee27ca..8a8f59ffdb4c 100644
> --- a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> @@ -290,10 +290,10 @@ xspi: spi@11030000 {
>                         interrupt-names = "pulse", "err_pulse";
>                         clocks = <&cpg CPG_MOD 0x9f>,
>                                  <&cpg CPG_MOD 0xa0>,
> -                                <&cpg CPG_CORE R9A09G057_SPI_CLK_SPI>,
> -                                <&cpg CPG_MOD 0xa1>;
> +                                <&cpg CPG_MOD 0xa1>,
> +                                <&cpg CPG_MOD 0x190>;

Does this dummy index need to be documented to be in bindings?

>                         clock-names = "ahb", "axi", "spi", "spix2";
> -                       assigned-clocks = <&cpg CPG_CORE R9A09G057_SPI_CLK_SPI>;
> +                       assigned-clocks = <&cpg CPG_MOD 0xa1>;
>                         assigned-clock-rates = <133333334>;
>                         resets = <&cpg 0xa3>, <&cpg 0xa4>;
>                         reset-names = "hresetn", "aresetn"; diff --git
> a/drivers/clk/renesas/r9a09g057-cpg.c
> b/drivers/clk/renesas/r9a09g057-cpg.c
> index 9952474bcf48..d5eef17ad5fc 100644
> --- a/drivers/clk/renesas/r9a09g057-cpg.c
> +++ b/drivers/clk/renesas/r9a09g057-cpg.c
> @@ -43,6 +43,7 @@ enum clk_ids {
>         CLK_SMUX2_XSPI_CLK0,
>         CLK_SMUX2_XSPI_CLK1,
>         CLK_PLLCM33_XSPI,
> +       CLK_PLLCM33_XSPIX2,
>         CLK_PLLCLN_DIV2,
>         CLK_PLLCLN_DIV8,
>         CLK_PLLCLN_DIV16,
> @@ -176,6 +177,7 @@ static const struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
>         DEF_SMUX(".smux2_xspi_clk1", CLK_SMUX2_XSPI_CLK1, SSEL1_SELCTL3, smux2_xspi_clk1),
>         DEF_CSDIV(".pllcm33_xspi", CLK_PLLCM33_XSPI, CLK_SMUX2_XSPI_CLK1, CSDIV0_DIVCTL3,
>                   dtable_2_16),
> +       DEF_FIXED(".pllcm33_xspix2", CLK_PLLCM33_XSPIX2,
> CLK_PLLCM33_XSPI, 2, 1),
	
Will this internal core clk shows same status like core clk? For eg: XSPI node is disabled the
clk_summary reports this internal core clock is ON (while it's actually OFF)?


> 
>         DEF_FIXED(".pllcln_div2", CLK_PLLCLN_DIV2, CLK_PLLCLN, 1, 2),
>         DEF_FIXED(".pllcln_div8", CLK_PLLCLN_DIV8, CLK_PLLCLN, 1, 8), @@ -231,7 +233,6 @@ static const
> struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
>                   CLK_PLLETH_DIV_125_FIX, 1, 1),
>         DEF_FIXED("gbeth_1_clk_ptp_ref_i", R9A09G057_GBETH_1_CLK_PTP_REF_I,
>                   CLK_PLLETH_DIV_125_FIX, 1, 1),
> -       DEF_FIXED("spi_clk_spi", R9A09G057_SPI_CLK_SPI, CLK_PLLCM33_XSPI, 1, 2),
>  };
> 
>  static const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst = { @@ -311,8 +312,10 @@ static
> const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst = {
>                                                 BUS_MSTOP(4, BIT(5))),
>         DEF_MOD("spi_aclk",                     CLK_PLLCM33_GEAR, 10, 0, 5, 0,
>                                                 BUS_MSTOP(4, BIT(5))),
> -       DEF_MOD_NO_PM("spi_clk_spix2",          CLK_PLLCM33_XSPI, 10, 1, 5, 2,
> +       DEF_MOD_PAIRED("spi_clk_spi",           CLK_PLLCM33_XSPI, 10, 1, 5, 1,
>                                                 BUS_MSTOP(4, BIT(5))),
> +       DEF_MOD_PAIRED_ALIAS("spi_clk_spix2",   CLK_PLLCM33_XSPIX2, 25, 0, 5, 2,
> +                                               BUS_MSTOP(4, BIT(5)),
> + 10, 1),
>         DEF_MOD("sdhi_0_imclk",                 CLK_PLLCLN_DIV8, 10, 3, 5, 3,
>                                                 BUS_MSTOP(8, BIT(2))),
>         DEF_MOD("sdhi_0_imclk2",                CLK_PLLCLN_DIV8, 10, 4, 5, 4,
> @@ -508,7 +511,7 @@ const struct rzv2h_cpg_info r9a09g057_cpg_info __initconst = {
>         /* Module Clocks */
>         .mod_clks = r9a09g057_mod_clks,
>         .num_mod_clks = ARRAY_SIZE(r9a09g057_mod_clks),
> -       .num_hw_mod_clks = 25 * 16,
> +       .num_hw_mod_clks = 26 * 16,

	   .num_hw_mod_clks = 25 * 16 + 1,??

> 
>         /* Resets */
>         .resets = r9a09g057_resets,
> diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c index
> 97bcd252fcbf..847ea71df865 100644
> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -71,6 +71,12 @@
> 
>  #define CPG_CLKSTATUS0         (0x700)
> 
> +struct rzv2h_paired_clock {
> +       u8 on_index;
> +       u8 on_bit;
> +       unsigned int enabled_count;

refcount_t ??

Cheers,
Biju

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Prabhakar,

On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > the Renesas RZ/G3E
> > > > > > > > (R9A09G047) SoC.
> > > > > > > >
> > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > >
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > +++ as,r
> > > > > > > > +++ zg3e
> > > > > > > > +++ -xspi.yaml
> > > > > >
> > > > > > > > +    spi@11030000 {
> > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > >
> > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> >
> > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > is derived from (the parent of) clk_spix2, not the other way around?
> > So you can model clk_spi as a fixed divider clock with parent clk_spix2
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> >
> With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> and XSPI node is disabled the clk_summary reports the core clock is ON
> (while it's actually OFF).

Is that a real problem, or is it purely cosmetic?

> Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> 0x190) and represent this is a module clock for example for the
> spi_clk_spix2 clock and use this in the DT and let the CPG core code
> handle such turning ON/OF the module clocks based on the enable count
> which will be handled internally in the driver?

Please do not use "unused" module clock bits.  These do not describe
the hardware, and may actually exist in the hardware (try disabling
all undocumented module clocks, and observe what fails...).

If spi_clk_spi really must show being disabled, you can change it
from a fixed divider clock (which does not implement .{en,dis}able())
to a custom fixed divider clock that does implement .{en,dis}able()
and keeps track internally of the fake state, or even looks at the
state of spi_clk_spix2?

However, upon second look, spi_clk_spi is not implemented as a fixed
divider clock with parent clk_spix2, as described above:

      .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
         .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
            spi_clk_spix2  0  0  0 40000000   0  0  50000  N
            spi_clk_spi    0  0  0 20000000   0  0  50000  Y
         spi_aclk          0  0  0 200000000  0  0  50000  N
         spi_hclk          0  0  0 200000000  0  0  50000  N
      .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y

Instead, they both use pllcm33_xspi as the parent clock.
Apparently I missed that in the review of RZ/G3E XSPI clock support.
The changelog for that patch does describe the correct topology?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Lad, Prabhakar]

Hi Biju,

On Wed, Jun 18, 2025 at 7:21 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
>
> Hi Prabhakar,
>
> > -----Original Message-----
> > From: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> > Sent: 17 June 2025 22:05
> > Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> >
> > Hi Geert,
> >
> > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > >
> > > Hi Biju,
> > >
> > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar
> > > > > 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar
> > > > > > > 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document
> > > > > > > > > support for the Expanded Serial Peripheral Interface
> > > > > > > > > (xSPI) Controller in the Renesas RZ/G3E
> > > > > > > > > (R9A09G047) SoC.
> > > > > > > > >
> > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > >
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers
> > > > > > > > > +++ /renes
> > > > > > > > > +++ as,r
> > > > > > > > > +++ zg3e
> > > > > > > > > +++ -xspi.yaml
<snip>
> > Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> > 0x190) and represent this is a module clock for example for the
> > spi_clk_spix2 clock and use this in the DT and let the CPG core code handle such turning ON/OF the
> > module clocks based on the enable count which will be handled internally in the driver?
> >
> > I have some POC code for the paired clocks which will handle the enable count of the paired module
> > clocks, below is the diff. Please share your thoughts.
> >
> > diff --git a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> > b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> > index 3c5984ee27ca..8a8f59ffdb4c 100644
> > --- a/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r9a09g057.dtsi
> > @@ -290,10 +290,10 @@ xspi: spi@11030000 {
> >                         interrupt-names = "pulse", "err_pulse";
> >                         clocks = <&cpg CPG_MOD 0x9f>,
> >                                  <&cpg CPG_MOD 0xa0>,
> > -                                <&cpg CPG_CORE R9A09G057_SPI_CLK_SPI>,
> > -                                <&cpg CPG_MOD 0xa1>;
> > +                                <&cpg CPG_MOD 0xa1>,
> > +                                <&cpg CPG_MOD 0x190>;
>
> Does this dummy index need to be documented to be in bindings?
>
Yes, indeed we will have to.

> >                         clock-names = "ahb", "axi", "spi", "spix2";
> > -                       assigned-clocks = <&cpg CPG_CORE R9A09G057_SPI_CLK_SPI>;
> > +                       assigned-clocks = <&cpg CPG_MOD 0xa1>;
> >                         assigned-clock-rates = <133333334>;
> >                         resets = <&cpg 0xa3>, <&cpg 0xa4>;
> >                         reset-names = "hresetn", "aresetn"; diff --git
> > a/drivers/clk/renesas/r9a09g057-cpg.c
> > b/drivers/clk/renesas/r9a09g057-cpg.c
> > index 9952474bcf48..d5eef17ad5fc 100644
> > --- a/drivers/clk/renesas/r9a09g057-cpg.c
> > +++ b/drivers/clk/renesas/r9a09g057-cpg.c
> > @@ -43,6 +43,7 @@ enum clk_ids {
> >         CLK_SMUX2_XSPI_CLK0,
> >         CLK_SMUX2_XSPI_CLK1,
> >         CLK_PLLCM33_XSPI,
> > +       CLK_PLLCM33_XSPIX2,
> >         CLK_PLLCLN_DIV2,
> >         CLK_PLLCLN_DIV8,
> >         CLK_PLLCLN_DIV16,
> > @@ -176,6 +177,7 @@ static const struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
> >         DEF_SMUX(".smux2_xspi_clk1", CLK_SMUX2_XSPI_CLK1, SSEL1_SELCTL3, smux2_xspi_clk1),
> >         DEF_CSDIV(".pllcm33_xspi", CLK_PLLCM33_XSPI, CLK_SMUX2_XSPI_CLK1, CSDIV0_DIVCTL3,
> >                   dtable_2_16),
> > +       DEF_FIXED(".pllcm33_xspix2", CLK_PLLCM33_XSPIX2,
> > CLK_PLLCM33_XSPI, 2, 1),
>
> Will this internal core clk shows same status like core clk? For eg: XSPI node is disabled the
> clk_summary reports this internal core clock is ON (while it's actually OFF)?
>
The internal core clock state will show as always ON.

>
> >
> >         DEF_FIXED(".pllcln_div2", CLK_PLLCLN_DIV2, CLK_PLLCLN, 1, 2),
> >         DEF_FIXED(".pllcln_div8", CLK_PLLCLN_DIV8, CLK_PLLCLN, 1, 8), @@ -231,7 +233,6 @@ static const
> > struct cpg_core_clk r9a09g057_core_clks[] __initconst = {
> >                   CLK_PLLETH_DIV_125_FIX, 1, 1),
> >         DEF_FIXED("gbeth_1_clk_ptp_ref_i", R9A09G057_GBETH_1_CLK_PTP_REF_I,
> >                   CLK_PLLETH_DIV_125_FIX, 1, 1),
> > -       DEF_FIXED("spi_clk_spi", R9A09G057_SPI_CLK_SPI, CLK_PLLCM33_XSPI, 1, 2),
> >  };
> >
> >  static const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst = { @@ -311,8 +312,10 @@ static
> > const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst = {
> >                                                 BUS_MSTOP(4, BIT(5))),
> >         DEF_MOD("spi_aclk",                     CLK_PLLCM33_GEAR, 10, 0, 5, 0,
> >                                                 BUS_MSTOP(4, BIT(5))),
> > -       DEF_MOD_NO_PM("spi_clk_spix2",          CLK_PLLCM33_XSPI, 10, 1, 5, 2,
> > +       DEF_MOD_PAIRED("spi_clk_spi",           CLK_PLLCM33_XSPI, 10, 1, 5, 1,
> >                                                 BUS_MSTOP(4, BIT(5))),
> > +       DEF_MOD_PAIRED_ALIAS("spi_clk_spix2",   CLK_PLLCM33_XSPIX2, 25, 0, 5, 2,
> > +                                               BUS_MSTOP(4, BIT(5)),
> > + 10, 1),
> >         DEF_MOD("sdhi_0_imclk",                 CLK_PLLCLN_DIV8, 10, 3, 5, 3,
> >                                                 BUS_MSTOP(8, BIT(2))),
> >         DEF_MOD("sdhi_0_imclk2",                CLK_PLLCLN_DIV8, 10, 4, 5, 4,
> > @@ -508,7 +511,7 @@ const struct rzv2h_cpg_info r9a09g057_cpg_info __initconst = {
> >         /* Module Clocks */
> >         .mod_clks = r9a09g057_mod_clks,
> >         .num_mod_clks = ARRAY_SIZE(r9a09g057_mod_clks),
> > -       .num_hw_mod_clks = 25 * 16,
> > +       .num_hw_mod_clks = 26 * 16,
>
>            .num_hw_mod_clks = 25 * 16 + 1,??
Agreed.

>
> >
> >         /* Resets */
> >         .resets = r9a09g057_resets,
> > diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c index
> > 97bcd252fcbf..847ea71df865 100644
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -71,6 +71,12 @@
> >
> >  #define CPG_CLKSTATUS0         (0x700)
> >
> > +struct rzv2h_paired_clock {
> > +       u8 on_index;
> > +       u8 on_bit;
> > +       unsigned int enabled_count;
>
> refcount_t ??
>
Agreed.

Cheers,
Prabhakar

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Lad, Prabhakar]

Hi Geert,

On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > (R9A09G047) SoC.
> > > > > > > > >
> > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > >
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > > +++ as,r
> > > > > > > > > +++ zg3e
> > > > > > > > > +++ -xspi.yaml
> > > > > > >
> > > > > > > > > +    spi@11030000 {
> > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > > >
> > > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > >
> > > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > > is derived from (the parent of) clk_spix2, not the other way around?
> > > So you can model clk_spi as a fixed divider clock with parent clk_spix2
>                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > >
> > With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> > and XSPI node is disabled the clk_summary reports the core clock is ON
> > (while it's actually OFF).
>
> Is that a real problem, or is it purely cosmetic?
Just cosmetic tbh as despite being a MOD clock we have to define it as
a core clock in the DT.

>
> > Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> > 0x190) and represent this is a module clock for example for the
> > spi_clk_spix2 clock and use this in the DT and let the CPG core code
> > handle such turning ON/OF the module clocks based on the enable count
> > which will be handled internally in the driver?
>
> Please do not use "unused" module clock bits.  These do not describe
> the hardware, and may actually exist in the hardware (try disabling
> all undocumented module clocks, and observe what fails...).
>
Agreed, "unused" module clock bits were only used as a dummy. The
read/write operations were only performed on the actual bits which are
documented in the HW manual.

> If spi_clk_spi really must show being disabled, you can change it
> from a fixed divider clock (which does not implement .{en,dis}able())
> to a custom fixed divider clock that does implement .{en,dis}able()
> and keeps track internally of the fake state, or even looks at the
> state of spi_clk_spix2?
>
Good point. Maybe instead of implementing the dummy .{en,dis}able() I
will implement the is_enabled() + (clk_fixed_factor_ops). The
is_enabled() will take care of reading from the MON bits and report
the actual state of the clock.

> However, upon second look, spi_clk_spi is not implemented as a fixed
> divider clock with parent clk_spix2, as described above:
>
>       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
>          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
>             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
>             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
>          spi_aclk          0  0  0 200000000  0  0  50000  N
>          spi_hclk          0  0  0 200000000  0  0  50000  N
>       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
>
> Instead, they both use pllcm33_xspi as the parent clock.
> Apparently I missed that in the review of RZ/G3E XSPI clock support.
> The changelog for that patch does describe the correct topology?
>
The topology is correct for RZ/G3E, spi/spix2 are sourced from
pllcm33_xspi divider and there is a divider (/2) for spi.

So to conclude I will implement spi as a core clock (as done on
RZ/G3E) and implement a fixed factor clock which can report the MON
bit for spi clock. Is this approach OK with you?

Cheers,
Prabhakar

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Prabhakar,

On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > >
> > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > >
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > > > +++ as,r
> > > > > > > > > > +++ zg3e
> > > > > > > > > > +++ -xspi.yaml
> > > > > > > >
> > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > > > >
> > > > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > >
> > > > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > > > is derived from (the parent of) clk_spix2, not the other way around?
> > > > So you can model clk_spi as a fixed divider clock with parent clk_spix2
> >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[A]

> > > > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > > > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > >
> > > With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> > > and XSPI node is disabled the clk_summary reports the core clock is ON
> > > (while it's actually OFF).
> >
> > Is that a real problem, or is it purely cosmetic?
> Just cosmetic tbh as despite being a MOD clock we have to define it as
> a core clock in the DT.
>
> > > Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> > > 0x190) and represent this is a module clock for example for the
> > > spi_clk_spix2 clock and use this in the DT and let the CPG core code
> > > handle such turning ON/OF the module clocks based on the enable count
> > > which will be handled internally in the driver?
> >
> > Please do not use "unused" module clock bits.  These do not describe
> > the hardware, and may actually exist in the hardware (try disabling
> > all undocumented module clocks, and observe what fails...).
> >
> Agreed, "unused" module clock bits were only used as a dummy. The
> read/write operations were only performed on the actual bits which are
> documented in the HW manual.
>
> > If spi_clk_spi really must show being disabled, you can change it
> > from a fixed divider clock (which does not implement .{en,dis}able())
> > to a custom fixed divider clock that does implement .{en,dis}able()
> > and keeps track internally of the fake state, or even looks at the
> > state of spi_clk_spix2?
> >
> Good point. Maybe instead of implementing the dummy .{en,dis}able() I
> will implement the is_enabled() + (clk_fixed_factor_ops). The
> is_enabled() will take care of reading from the MON bits and report
> the actual state of the clock.
>
> > However, upon second look, spi_clk_spi is not implemented as a fixed
> > divider clock with parent clk_spix2, as described above:
> >
> >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> >          spi_aclk          0  0  0 200000000  0  0  50000  N
> >          spi_hclk          0  0  0 200000000  0  0  50000  N
> >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> >
> > Instead, they both use pllcm33_xspi as the parent clock.
> > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > The changelog for that patch does describe the correct topology?
> >
> The topology is correct for RZ/G3E, spi/spix2 are sourced from
> pllcm33_xspi divider and there is a divider (/2) for spi.

Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
immediate parent.

[A] describes something different:

    .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
        spi_clk_spix2  0  0  0 40000000   0  0  50000  N
            spi_clk_spi    0  0  0 20000000   0  0  50000  Y

I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi Geert,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 18 June 2025 13:59
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Hi Prabhakar,
> 
> On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31
> > > > > > > Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon,
> > > > > > > > > 31 Mar 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document
> > > > > > > > > > > support for the Expanded Serial Peripheral Interface
> > > > > > > > > > > (xSPI) Controller in the Renesas RZ/G3E
> > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > >
> > > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > >
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-contr
> > > > > > > > > > > +++ ollers/renes
> > > > > > > > > > > +++ as,r
> > > > > > > > > > > +++ zg3e
> > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > >
> > > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD
> > > > > > > > > > > + 0xa1>;
> > > > > > > > > >
> > > > > > > > > > On the next version I am going to update spix2 clk as
> > > > > > > > > > <&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > >
> > > > > According to the RZ/G3E clock system diagram, (the parent of)
> > > > > clk_spi is derived from (the parent of) clk_spix2, not the other way around?
> > > > > So you can model clk_spi as a fixed divider clock with parent
> > > > > clk_spix2
> > >
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [A]
> 
> > > > > and factor two.  I.e. provide a new core clock
> > > > > R9A09G047_SPI_CLK_SPI instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > >
> > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a core
> > > > clock and XSPI node is disabled the clk_summary reports the core
> > > > clock is ON (while it's actually OFF).
> > >
> > > Is that a real problem, or is it purely cosmetic?
> > Just cosmetic tbh as despite being a MOD clock we have to define it as
> > a core clock in the DT.
> >
> > > > Can we maybe use a unused ON index and ON bit for example 25, 0
> > > > (ie
> > > > 0x190) and represent this is a module clock for example for the
> > > > spi_clk_spix2 clock and use this in the DT and let the CPG core
> > > > code handle such turning ON/OF the module clocks based on the
> > > > enable count which will be handled internally in the driver?
> > >
> > > Please do not use "unused" module clock bits.  These do not describe
> > > the hardware, and may actually exist in the hardware (try disabling
> > > all undocumented module clocks, and observe what fails...).
> > >
> > Agreed, "unused" module clock bits were only used as a dummy. The
> > read/write operations were only performed on the actual bits which are
> > documented in the HW manual.
> >
> > > If spi_clk_spi really must show being disabled, you can change it
> > > from a fixed divider clock (which does not implement
> > > .{en,dis}able()) to a custom fixed divider clock that does implement
> > > .{en,dis}able() and keeps track internally of the fake state, or
> > > even looks at the state of spi_clk_spix2?
> > >
> > Good point. Maybe instead of implementing the dummy .{en,dis}able() I
> > will implement the is_enabled() + (clk_fixed_factor_ops). The
> > is_enabled() will take care of reading from the MON bits and report
> > the actual state of the clock.
> >
> > > However, upon second look, spi_clk_spi is not implemented as a fixed
> > > divider clock with parent clk_spix2, as described above:
> > >
> > >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> > >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > >          spi_aclk          0  0  0 200000000  0  0  50000  N
> > >          spi_hclk          0  0  0 200000000  0  0  50000  N
> > >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> > >
> > > Instead, they both use pllcm33_xspi as the parent clock.
> > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > The changelog for that patch does describe the correct topology?
> > >
> > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > pllcm33_xspi divider and there is a divider (/2) for spi.
> 
> Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as immediate parent.
> 
> [A] describes something different:
> 
>     .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
>         spi_clk_spix2  0  0  0 40000000   0  0  50000  N
>             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> 
> I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.

Looks like I missed this part. Sorry for the mistake.

Cheers,
Biju

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Lad, Prabhakar]

Hi Geert,

On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > >
> > > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > >
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > > > > +++ as,r
> > > > > > > > > > > +++ zg3e
> > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > >
> > > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > > > > >
> > > > > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > >
> > > > > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > > > > is derived from (the parent of) clk_spix2, not the other way around?
> > > > > So you can model clk_spi as a fixed divider clock with parent clk_spix2
> > >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> [A]
>
> > > > > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > > > > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > >
> > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> > > > and XSPI node is disabled the clk_summary reports the core clock is ON
> > > > (while it's actually OFF).
> > >
> > > Is that a real problem, or is it purely cosmetic?
> > Just cosmetic tbh as despite being a MOD clock we have to define it as
> > a core clock in the DT.
> >
> > > > Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> > > > 0x190) and represent this is a module clock for example for the
> > > > spi_clk_spix2 clock and use this in the DT and let the CPG core code
> > > > handle such turning ON/OF the module clocks based on the enable count
> > > > which will be handled internally in the driver?
> > >
> > > Please do not use "unused" module clock bits.  These do not describe
> > > the hardware, and may actually exist in the hardware (try disabling
> > > all undocumented module clocks, and observe what fails...).
> > >
> > Agreed, "unused" module clock bits were only used as a dummy. The
> > read/write operations were only performed on the actual bits which are
> > documented in the HW manual.
> >
> > > If spi_clk_spi really must show being disabled, you can change it
> > > from a fixed divider clock (which does not implement .{en,dis}able())
> > > to a custom fixed divider clock that does implement .{en,dis}able()
> > > and keeps track internally of the fake state, or even looks at the
> > > state of spi_clk_spix2?
> > >
> > Good point. Maybe instead of implementing the dummy .{en,dis}able() I
> > will implement the is_enabled() + (clk_fixed_factor_ops). The
> > is_enabled() will take care of reading from the MON bits and report
> > the actual state of the clock.
> >
> > > However, upon second look, spi_clk_spi is not implemented as a fixed
> > > divider clock with parent clk_spix2, as described above:
> > >
> > >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> > >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > >          spi_aclk          0  0  0 200000000  0  0  50000  N
> > >          spi_hclk          0  0  0 200000000  0  0  50000  N
> > >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> > >
> > > Instead, they both use pllcm33_xspi as the parent clock.
> > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > The changelog for that patch does describe the correct topology?
> > >
> > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > pllcm33_xspi divider and there is a divider (/2) for spi.
>
> Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
> immediate parent.
>
> [A] describes something different:
>
>     .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
>         spi_clk_spix2  0  0  0 40000000   0  0  50000  N
>             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
>
> I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.
>
Okay, thanks - got it.

To clarify, to implement spi_clk_spi core clock as a parent of
spi_clk_spix2 I will need to implement some sort of mechanism which
registers (late) core clks after core clks and module clks are
registered as spi_clk_spix2 is a module clock.

Cheers,
Prabhakar

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Prabhakar,

On Wed, 18 Jun 2025 at 15:41, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > > >
> > > > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > > >
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > > > > > +++ as,r
> > > > > > > > > > > > +++ zg3e
> > > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > > >
> > > > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > > > > > >
> > > > > > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > > >
> > > > > > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > > > > > is derived from (the parent of) clk_spix2, not the other way around?
> > > > > > So you can model clk_spi as a fixed divider clock with parent clk_spix2
> > > >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > [A]
> >
> > > > > > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > > > > > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > > >
> > > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> > > > > and XSPI node is disabled the clk_summary reports the core clock is ON
> > > > > (while it's actually OFF).
> > > >
> > > > Is that a real problem, or is it purely cosmetic?
> > > Just cosmetic tbh as despite being a MOD clock we have to define it as
> > > a core clock in the DT.
> > >
> > > > > Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> > > > > 0x190) and represent this is a module clock for example for the
> > > > > spi_clk_spix2 clock and use this in the DT and let the CPG core code
> > > > > handle such turning ON/OF the module clocks based on the enable count
> > > > > which will be handled internally in the driver?
> > > >
> > > > Please do not use "unused" module clock bits.  These do not describe
> > > > the hardware, and may actually exist in the hardware (try disabling
> > > > all undocumented module clocks, and observe what fails...).
> > > >
> > > Agreed, "unused" module clock bits were only used as a dummy. The
> > > read/write operations were only performed on the actual bits which are
> > > documented in the HW manual.
> > >
> > > > If spi_clk_spi really must show being disabled, you can change it
> > > > from a fixed divider clock (which does not implement .{en,dis}able())
> > > > to a custom fixed divider clock that does implement .{en,dis}able()
> > > > and keeps track internally of the fake state, or even looks at the
> > > > state of spi_clk_spix2?
> > > >
> > > Good point. Maybe instead of implementing the dummy .{en,dis}able() I
> > > will implement the is_enabled() + (clk_fixed_factor_ops). The
> > > is_enabled() will take care of reading from the MON bits and report
> > > the actual state of the clock.
> > >
> > > > However, upon second look, spi_clk_spi is not implemented as a fixed
> > > > divider clock with parent clk_spix2, as described above:
> > > >
> > > >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> > > >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > >          spi_aclk          0  0  0 200000000  0  0  50000  N
> > > >          spi_hclk          0  0  0 200000000  0  0  50000  N
> > > >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> > > >
> > > > Instead, they both use pllcm33_xspi as the parent clock.
> > > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > > The changelog for that patch does describe the correct topology?
> > > >
> > > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > > pllcm33_xspi divider and there is a divider (/2) for spi.
> >
> > Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
> > immediate parent.
> >
> > [A] describes something different:
> >
> >     .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> >         spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> >
> > I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.
> >
> Okay, thanks - got it.
>
> To clarify, to implement spi_clk_spi core clock as a parent of
> spi_clk_spix2 I will need to implement some sort of mechanism which
> registers (late) core clks after core clks and module clks are
> registered as spi_clk_spix2 is a module clock.

Yes, I wondered about that as well, but wasn't too worried as you
already added the smux with e.g. "et0_rxclk" as parent, which also
doesn't exist at registration time ;-)

But indeed, the smux uses clock names to find the parents, while
fixed-factor clocks in zv2h_cpg_register_core_clk() expect clock IDs
(which are converted to names), and don't handle non-core clocks yet.
So either add support for late core clocks, or modify CLK_TYPE_FF
to use cpg_core_clock.parent_names[] in case of a non-core parent?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Lad, Prabhakar]

Hi Geert,

On Wed, Jun 18, 2025 at 3:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, 18 Jun 2025 at 15:41, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > > > >
> > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > > > > > > +++ as,r
> > > > > > > > > > > > > +++ zg3e
> > > > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > > > >
> > > > > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > > > > > > >
> > > > > > > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > > > >
> > > > > > > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > > > > > > is derived from (the parent of) clk_spix2, not the other way around?
> > > > > > > So you can model clk_spi as a fixed divider clock with parent clk_spix2
> > > > >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > [A]
> > >
> > > > > > > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > > > > > > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > > > >
> > > > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> > > > > > and XSPI node is disabled the clk_summary reports the core clock is ON
> > > > > > (while it's actually OFF).
> > > > >
> > > > > Is that a real problem, or is it purely cosmetic?
> > > > Just cosmetic tbh as despite being a MOD clock we have to define it as
> > > > a core clock in the DT.
> > > >
> > > > > > Can we maybe use a unused ON index and ON bit for example 25, 0 (ie
> > > > > > 0x190) and represent this is a module clock for example for the
> > > > > > spi_clk_spix2 clock and use this in the DT and let the CPG core code
> > > > > > handle such turning ON/OF the module clocks based on the enable count
> > > > > > which will be handled internally in the driver?
> > > > >
> > > > > Please do not use "unused" module clock bits.  These do not describe
> > > > > the hardware, and may actually exist in the hardware (try disabling
> > > > > all undocumented module clocks, and observe what fails...).
> > > > >
> > > > Agreed, "unused" module clock bits were only used as a dummy. The
> > > > read/write operations were only performed on the actual bits which are
> > > > documented in the HW manual.
> > > >
> > > > > If spi_clk_spi really must show being disabled, you can change it
> > > > > from a fixed divider clock (which does not implement .{en,dis}able())
> > > > > to a custom fixed divider clock that does implement .{en,dis}able()
> > > > > and keeps track internally of the fake state, or even looks at the
> > > > > state of spi_clk_spix2?
> > > > >
> > > > Good point. Maybe instead of implementing the dummy .{en,dis}able() I
> > > > will implement the is_enabled() + (clk_fixed_factor_ops). The
> > > > is_enabled() will take care of reading from the MON bits and report
> > > > the actual state of the clock.
> > > >
> > > > > However, upon second look, spi_clk_spi is not implemented as a fixed
> > > > > divider clock with parent clk_spix2, as described above:
> > > > >
> > > > >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> > > > >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > > >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > > >          spi_aclk          0  0  0 200000000  0  0  50000  N
> > > > >          spi_hclk          0  0  0 200000000  0  0  50000  N
> > > > >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> > > > >
> > > > > Instead, they both use pllcm33_xspi as the parent clock.
> > > > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > > > The changelog for that patch does describe the correct topology?
> > > > >
> > > > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > > > pllcm33_xspi divider and there is a divider (/2) for spi.
> > >
> > > Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
> > > immediate parent.
> > >
> > > [A] describes something different:
> > >
> > >     .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > >         spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > >
> > > I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.
> > >
> > Okay, thanks - got it.
> >
> > To clarify, to implement spi_clk_spi core clock as a parent of
> > spi_clk_spix2 I will need to implement some sort of mechanism which
> > registers (late) core clks after core clks and module clks are
> > registered as spi_clk_spix2 is a module clock.
>
> Yes, I wondered about that as well, but wasn't too worried as you
> already added the smux with e.g. "et0_rxclk" as parent, which also
> doesn't exist at registration time ;-)
>
Good point.

> But indeed, the smux uses clock names to find the parents, while
> fixed-factor clocks in zv2h_cpg_register_core_clk() expect clock IDs
> (which are converted to names), and don't handle non-core clocks yet.
> So either add support for late core clocks, or modify CLK_TYPE_FF
> to use cpg_core_clock.parent_names[] in case of a non-core parent?
>
I choose the late core registration of the clocks and with this the
core clk_spi still reports `Y` in the summary while the parent is OFF
(since its a FF clock).

Logs for option #1
------------------------
root@rzv2h-evk:~# modprobe spi_rpc_if
[  217.783625] 3 fixed-partitions partitions found on MTD device spi0.0
[  217.790029] Creating 3 MTD partitions on "spi0.0":
[  217.794958] 0x000000000000-0x000000060000 : "bl2"
[  217.800464] 0x000000060000-0x000002000000 : "fip"
[  217.807788] 0x000002000000-0x000004000000 : "user"
root@rzv2h-evk:~#
root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
             spi_aclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
             spi_hclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
          .smux2_xspi_clk0           1       1        0
533333333   0          0     50000      Y            deviceless
              no_connection_id
             .smux2_xspi_clk1        1       1        0
533333333   0          0     50000      Y               deviceless
                 no_connection_id
                .pllcm33_xspi        1       1        0
266666667   0          0     50000      Y                  deviceless
                    no_connection_id
                   spi_clk_spix2     2       2        0
266666667   0          0     50000      Y
11030000.spi                    spix2
                      spi_clk_spi    1       1        0
133333333   0          0     50000      Y
11030000.spi                    spi
root@rzv2h-evk:~#
root@rzv2h-evk:~# modprobe -r  spi_rpc_if
[  225.376563] Deleting MTD partitions on "spi0.0":
[  225.381218] Deleting bl2 MTD partition
[  225.385504] Deleting fip MTD partition
[  225.617827] Deleting user MTD partition
root@rzv2h-evk:~#
root@rzv2h-evk:~#
root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
             spi_aclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
             spi_hclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
          .smux2_xspi_clk0           0       0        0
533333333   0          0     50000      Y            deviceless
              no_connection_id
             .smux2_xspi_clk1        0       0        0
533333333   0          0     50000      Y               deviceless
                 no_connection_id
                .pllcm33_xspi        0       0        0
266666667   0          0     50000      Y                  deviceless
                    no_connection_id
                   spi_clk_spix2     0       0        0
266666667   0          0     50000      N
deviceless                      no_connection_id
                      spi_clk_spi    0       0        0
133333333   0          0     50000      Y
deviceless                      no_connection_id
root@rzv2h-evk:~#
root@rzv2h-evk:~#




Code implementation for option#1
------------------------------------------------
diff --git a/drivers/clk/renesas/r9a09g057-cpg.c
b/drivers/clk/renesas/r9a09g057-cpg.c
index 9952474bcf48..ab9e9a3e8cd1 100644
--- a/drivers/clk/renesas/r9a09g057-cpg.c
+++ b/drivers/clk/renesas/r9a09g057-cpg.c
@@ -231,7 +231,10 @@ static const struct cpg_core_clk
r9a09g057_core_clks[] __initconst = {
           CLK_PLLETH_DIV_125_FIX, 1, 1),
     DEF_FIXED("gbeth_1_clk_ptp_ref_i", R9A09G057_GBETH_1_CLK_PTP_REF_I,
           CLK_PLLETH_DIV_125_FIX, 1, 1),
-    DEF_FIXED("spi_clk_spi", R9A09G057_SPI_CLK_SPI, CLK_PLLCM33_XSPI, 1, 2),
+};
+
+static const struct cpg_core_clk r9a09g057_late_core_clks[] __initconst = {
+    DEF_FIXED("spi_clk_spi", R9A09G057_SPI_CLK_SPI, 0xa1, 1, 2),
 };

 static const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst = {
@@ -502,6 +505,8 @@ const struct rzv2h_cpg_info r9a09g057_cpg_info
__initconst = {
     /* Core Clocks */
     .core_clks = r9a09g057_core_clks,
     .num_core_clks = ARRAY_SIZE(r9a09g057_core_clks),
+    .late_core_clks = r9a09g057_late_core_clks,
+    .num_late_core_clks = ARRAY_SIZE(r9a09g057_late_core_clks),
     .last_dt_core_clk = LAST_DT_CORE_CLK,
     .num_total_core_clks = MOD_CLK_BASE,

diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index 97bcd252fcbf..0fdac1578f8b 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -710,7 +710,7 @@ static struct clk

 static void __init
 rzv2h_cpg_register_core_clk(const struct cpg_core_clk *core,
-                struct rzv2h_cpg_priv *priv)
+                struct rzv2h_cpg_priv *priv, bool late)
 {
     struct clk *clk = ERR_PTR(-EOPNOTSUPP), *parent;
     unsigned int id = core->id, div = core->div;
@@ -727,8 +727,12 @@ rzv2h_cpg_register_core_clk(const struct
cpg_core_clk *core,
         break;
     case CLK_TYPE_FF:
         WARN_DEBUG(core->parent >= priv->num_core_clks);
-        parent = priv->clks[core->parent];
+        if (late)
+            parent = priv->clks[priv->num_core_clks + core->parent];
+        else
+            parent = priv->clks[core->parent];
         if (IS_ERR(parent)) {
+            pr_err("parent clk is NULL for %s parent:%d\n",
core->name, core->parent);
             clk = parent;
             goto fail;
         }
@@ -1298,11 +1302,14 @@ static int __init rzv2h_cpg_probe(struct
platform_device *pdev)
         clks[i] = ERR_PTR(-ENOENT);

     for (i = 0; i < info->num_core_clks; i++)
-        rzv2h_cpg_register_core_clk(&info->core_clks[i], priv);
+        rzv2h_cpg_register_core_clk(&info->core_clks[i], priv, false);

     for (i = 0; i < info->num_mod_clks; i++)
         rzv2h_cpg_register_mod_clk(&info->mod_clks[i], priv);

+    for (i = 0; i < info->num_late_core_clks; i++)
+        rzv2h_cpg_register_core_clk(&info->late_core_clks[i], priv, true);
+
     error = of_clk_add_provider(np, rzv2h_cpg_clk_src_twocell_get, priv);
     if (error)
         return error;
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index bce131bec80b..442289b9cafb 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -297,6 +297,8 @@ struct rzv2h_reset {
  *
  * @core_clks: Array of Core Clock definitions
  * @num_core_clks: Number of entries in core_clks[]
+ * @late_core_clks: Array of Core Clocks that are late initialized
+ * @num_late_core_clks: Number of entries in late_core_clks[]
  * @last_dt_core_clk: ID of the last Core Clock exported to DT
  * @num_total_core_clks: Total number of Core Clocks (exported + internal)
  *
@@ -315,6 +317,8 @@ struct rzv2h_cpg_info {
     /* Core Clocks */
     const struct cpg_core_clk *core_clks;
     unsigned int num_core_clks;
+    const struct cpg_core_clk *late_core_clks;
+    unsigned int num_late_core_clks;
     unsigned int last_dt_core_clk;
     unsigned int num_total_core_clks;


# Option#2
As mentioned in the previous thread I implemented FF clock with
is_enabled() with this I can see the status of core clk_spi reports
correct status.

Logs for option #2
---------------------
root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
             spi_aclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
             spi_hclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
          .smux2_xspi_clk0           1       1        0
533333333   0          0     50000      Y            deviceless
              no_connection_id
             .smux2_xspi_clk1        1       1        0
533333333   0          0     50000      Y               deviceless
                 no_connection_id
                .pllcm33_xspi        2       2        0
266666667   0          0     50000      Y                  deviceless
                    no_connection_id
                   spi_clk_spix2     1       1        0
266666667   0          0     50000      Y
11030000.spi                    spix2
                   spi_clk_spi       1       1        0
133333333   0          0     50000      Y
11030000.spi                    spi
root@rzv2h-evk:~#
root@rzv2h-evk:~# modprobe -r spi_rpc_if
[   58.860437] Deleting MTD partitions on "spi0.0":
[   58.865078] Deleting bl2 MTD partition
[   58.869355] Deleting fip MTD partition
[   58.907329] Deleting user MTD partition
root@rzv2h-evk:~#
root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
             spi_aclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
             spi_hclk                0       0        0
200000000   0          0     50000      N               deviceless
                 no_connection_id
          .smux2_xspi_clk0           0       0        0
533333333   0          0     50000      Y            deviceless
              no_connection_id
             .smux2_xspi_clk1        0       0        0
533333333   0          0     50000      Y               deviceless
                 no_connection_id
                .pllcm33_xspi        0       0        0
266666667   0          0     50000      Y                  deviceless
                    no_connection_id
                   spi_clk_spix2     0       0        0
266666667   0          0     50000      N
deviceless                      no_connection_id
                   spi_clk_spi       0       0        0
133333333   0          0     50000      N
deviceless                      no_connection_id
root@rzv2h-evk:~#
root@rzv2h-evk:~# modprobe spi_rpc_if
[   65.423581] 3 fixed-partitions partitions found on MTD device spi0.0
[   65.429971] Creating 3 MTD partitions on "spi0.0":
[   65.434778] 0x000000000000-0x000000060000 : "bl2"
[   65.440203] 0x000000060000-0x000002000000 : "fip"
[   65.446337] 0x000002000000-0x000004000000 : "user"
root@rzv2h-evk:~#
root@rzv2h-evk:~#
root@rzv2h-evk:~# cat /sys/kernel/debug/clk/clk_summary | grep spi
             spi_aclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
             spi_hclk                0       1        0
200000000   0          0     50000      N               deviceless
                 of_clk_get_from_provider
          .smux2_xspi_clk0           1       1        0
533333333   0          0     50000      Y            deviceless
              no_connection_id
             .smux2_xspi_clk1        1       1        0
533333333   0          0     50000      Y               deviceless
                 no_connection_id
                .pllcm33_xspi        2       2        0
266666667   0          0     50000      Y                  deviceless
                    no_connection_id
                   spi_clk_spix2     1       1        0
266666667   0          0     50000      Y
11030000.spi                    spix2
                   spi_clk_spi       1       1        0
133333333   0          0     50000      Y
11030000.spi                    spi
root@rzv2h-evk:~#

Code implementation for option#2
------------------------------------------------

diff --git a/drivers/clk/renesas/r9a09g057-cpg.c
b/drivers/clk/renesas/r9a09g057-cpg.c
index 9952474bcf48..c56e43492a02 100644
--- a/drivers/clk/renesas/r9a09g057-cpg.c
+++ b/drivers/clk/renesas/r9a09g057-cpg.c
@@ -231,7 +231,8 @@ static const struct cpg_core_clk
r9a09g057_core_clks[] __initconst = {
           CLK_PLLETH_DIV_125_FIX, 1, 1),
     DEF_FIXED("gbeth_1_clk_ptp_ref_i", R9A09G057_GBETH_1_CLK_PTP_REF_I,
           CLK_PLLETH_DIV_125_FIX, 1, 1),
-    DEF_FIXED("spi_clk_spi", R9A09G057_SPI_CLK_SPI, CLK_PLLCM33_XSPI, 1, 2),
+    DEF_FIXED_MOD_STATUS("spi_clk_spi", R9A09G057_SPI_CLK_SPI,
CLK_PLLCM33_XSPI, 1, 2,
+                 FIXED_MOD_CONF_XSPI),
 };

 static const struct rzv2h_mod_clk r9a09g057_mod_clks[] __initconst = {
diff --git a/drivers/clk/renesas/rzv2h-cpg.c b/drivers/clk/renesas/rzv2h-cpg.c
index 97bcd252fcbf..96db6ce4460f 100644
--- a/drivers/clk/renesas/rzv2h-cpg.c
+++ b/drivers/clk/renesas/rzv2h-cpg.c
@@ -179,6 +179,28 @@ struct rzv2h_plldsi_div_clk {
 #define to_plldsi_div_clk(_hw) \
     container_of(_hw, struct rzv2h_plldsi_div_clk, hw)

+/**
+ * struct rzv2h_ff_mod_status_clk - Fixed Factor Module Status Clock
+ *
+ * @priv: CPG private data
+ * @conf: fixed mod configuration
+ * @hw: Fixed Factor Status Clock handle
+ * @mult: multiplier value
+ * @div: divider value
+ * @flags: flags for the clock
+ */
+ struct rzv2h_ff_mod_status_clk {
+    struct rzv2h_cpg_priv *priv;
+    struct fixed_mod_conf conf;
+    struct clk_hw hw;
+    unsigned int mult;
+    unsigned int div;
+    unsigned int flags;
+};
+
+#define to_rzv2h_ff_mod_status_clk(_hw) \
+    container_of(_hw, struct rzv2h_ff_mod_status_clk, hw)
+
 static int rzv2h_cpg_pll_clk_is_enabled(struct clk_hw *hw)
 {
     struct pll_clk *pll_clk = to_pll(hw);
@@ -664,6 +686,114 @@ rzv2h_cpg_mux_clk_register(const struct
cpg_core_clk *core,
     return clk_hw->clk;
 }

+static unsigned long
+rzv2h_clk_ff_mod_status_recalc_rate(struct clk_hw *hw,
+                    unsigned long parent_rate)
+{
+    struct rzv2h_ff_mod_status_clk *fix = to_rzv2h_ff_mod_status_clk(hw);
+    unsigned long long int rate;
+
+    rate = (unsigned long long int)parent_rate * fix->mult;
+    do_div(rate, fix->div);
+    return (unsigned long)rate;
+}
+
+static long
+rzv2h_clk_ff_mod_status_round_rate(struct clk_hw *hw, unsigned long rate,
+                   unsigned long *prate)
+{
+    struct rzv2h_ff_mod_status_clk *fix = to_rzv2h_ff_mod_status_clk(hw);
+
+    if (clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) {
+        unsigned long best_parent;
+
+        best_parent = (rate / fix->mult) * fix->div;
+        *prate = clk_hw_round_rate(clk_hw_get_parent(hw), best_parent);
+    }
+
+    return (*prate / fix->div) * fix->mult;
+}
+
+static int rzv2h_clk_ff_mod_status_set_rate(struct clk_hw *hw,
unsigned long rate,
+                        unsigned long parent_rate)
+{
+    return 0;
+}
+
+static unsigned long
+rzv2h_clk_ff_mod_status_recalc_accuracy(struct clk_hw *hw,
+                    unsigned long parent_accuracy)
+{
+    return parent_accuracy;
+}
+
+static int
+rzv2h_clk_ff_mod_status_is_enabled(struct clk_hw *hw)
+{
+    struct rzv2h_ff_mod_status_clk *fix = to_rzv2h_ff_mod_status_clk(hw);
+    struct rzv2h_cpg_priv *priv = fix->priv;
+    u32 offset = GET_CLK_MON_OFFSET(fix->conf.mon_index);
+    u32 bitmask = BIT(fix->conf.mon_bit);
+    u32 val;
+
+    val = readl(priv->base + offset);
+    return !!(val & bitmask);
+}
+
+static const struct clk_ops rzv2h_clk_ff_mod_status_ops = {
+    .round_rate = rzv2h_clk_ff_mod_status_round_rate,
+    .set_rate = rzv2h_clk_ff_mod_status_set_rate,
+    .recalc_rate = rzv2h_clk_ff_mod_status_recalc_rate,
+    .recalc_accuracy = rzv2h_clk_ff_mod_status_recalc_accuracy,
+    .is_enabled = rzv2h_clk_ff_mod_status_is_enabled,
+};
+
+static struct clk * __init
+rzv2h_cpg_fixed_mod_status_clk_register(const struct cpg_core_clk *core,
+                    struct rzv2h_cpg_priv *priv)
+{
+    struct rzv2h_ff_mod_status_clk *clk_hw_data;
+    struct clk_init_data init = { };
+    const struct clk *parent;
+    const char *parent_name;
+    struct clk_hw *hw;
+    int ret;
+
+    WARN_DEBUG(core->parent >= priv->num_core_clks);
+    parent = priv->clks[core->parent];
+    if (IS_ERR(parent))
+        return ERR_CAST(parent);
+
+    parent_name = __clk_get_name(parent);
+    parent = priv->clks[core->parent];
+    if (IS_ERR(parent))
+        return ERR_CAST(parent);
+
+    clk_hw_data = devm_kzalloc(priv->dev, sizeof(*clk_hw_data), GFP_KERNEL);
+    if (!clk_hw_data)
+        return ERR_PTR(-ENOMEM);
+
+    clk_hw_data->priv = priv;
+    clk_hw_data->conf = core->cfg.fixed_mod;
+    clk_hw_data->mult = core->mult;
+    clk_hw_data->div = core->div;
+
+    init.name = core->name;
+    init.ops = &rzv2h_clk_ff_mod_status_ops;
+    init.flags = CLK_SET_RATE_PARENT;
+    init.parent_names = &parent_name;
+    init.num_parents = 1;
+
+    hw = &clk_hw_data->hw;
+    hw->init = &init;
+
+    ret = devm_clk_hw_register(priv->dev, hw);
+    if (ret)
+        return ERR_PTR(ret);
+
+    return hw->clk;
+}
+
 static struct clk
 *rzv2h_cpg_clk_src_twocell_get(struct of_phandle_args *clkspec,
                    void *data)
@@ -742,6 +872,9 @@ rzv2h_cpg_register_core_clk(const struct cpg_core_clk *core,
         else
             clk = clk_hw->clk;
         break;
+    case CLK_TYPE_FF_MOD_STATUS:
+        clk = rzv2h_cpg_fixed_mod_status_clk_register(core, priv);
+        break;
     case CLK_TYPE_PLL:
         clk = rzv2h_cpg_pll_clk_register(core, priv,
&rzv2h_cpg_pll_ops, false);
         break;
diff --git a/drivers/clk/renesas/rzv2h-cpg.h b/drivers/clk/renesas/rzv2h-cpg.h
index bce131bec80b..29e1dc841b46 100644
--- a/drivers/clk/renesas/rzv2h-cpg.h
+++ b/drivers/clk/renesas/rzv2h-cpg.h
@@ -94,6 +94,23 @@ struct smuxed {
         .width = (_width), \
     })

+/**
+ * struct fixed_mod_conf - Structure for fixed module configuration
+ *
+ * @mon_index: monitor index
+ * @mon_bit: monitor bit
+ */
+struct fixed_mod_conf {
+    u8 mon_index;
+    u8 mon_bit;
+};
+
+#define FIXED_MOD_CONF_PACK(_index, _bit) \
+    ((struct fixed_mod_conf){ \
+        .mon_index = (_index), \
+        .mon_bit = (_bit), \
+    })
+
 #define CPG_SSEL0        (0x300)
 #define CPG_SSEL1        (0x304)
 #define CPG_CDDIV0        (0x400)
@@ -137,6 +154,8 @@ struct smuxed {
                  FIELD_PREP_CONST(BUS_MSTOP_BITS_MASK, (mask)))
 #define BUS_MSTOP_NONE        GENMASK(31, 0)

+#define FIXED_MOD_CONF_XSPI    FIXED_MOD_CONF_PACK(5, 1)
+
 /**
  * Definitions of CPG Core Clocks
  *
@@ -157,6 +176,7 @@ struct cpg_core_clk {
         struct ddiv ddiv;
         struct pll pll;
         struct smuxed smux;
+        struct fixed_mod_conf fixed_mod;
     } cfg;
     const struct clk_div_table *dtable;
     const char * const *parent_names;
@@ -169,6 +189,7 @@ enum clk_types {
     /* Generic */
     CLK_TYPE_IN,        /* External Clock Input */
     CLK_TYPE_FF,        /* Fixed Factor Clock */
+    CLK_TYPE_FF_MOD_STATUS,    /* Fixed Factor Clock which can report
the status of module clock */
     CLK_TYPE_PLL,
     CLK_TYPE_DDIV,        /* Dynamic Switching Divider */
     CLK_TYPE_SMUX,        /* Static Mux */
@@ -186,6 +207,9 @@ enum clk_types {
     DEF_TYPE(_name, _id, CLK_TYPE_IN)
 #define DEF_FIXED(_name, _id, _parent, _mult, _div) \
     DEF_BASE(_name, _id, CLK_TYPE_FF, _parent, .div = _div, .mult = _mult)
+#define DEF_FIXED_MOD_STATUS(_name, _id, _parent, _mult, _div, _gate) \
+    DEF_BASE(_name, _id, CLK_TYPE_FF_MOD_STATUS, _parent, .div = _div, \
+         .mult = _mult, .cfg.fixed_mod = _gate)
 #define DEF_DDIV(_name, _id, _parent, _ddiv_packed, _dtable) \
     DEF_TYPE(_name, _id, CLK_TYPE_DDIV, \
         .cfg.ddiv = _ddiv_packed, \

Please share your thoughts on this.

Cheers,
Prabhakar

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Geert Uytterhoeven]

Hi Prabhakar,

On Wed, 18 Jun 2025 at 17:24, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> On Wed, Jun 18, 2025 at 3:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, 18 Jun 2025 at 15:41, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > > > > >
> > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > > > > > > > +++ as,r
> > > > > > > > > > > > > > +++ zg3e
> > > > > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > > > > >
> > > > > > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > > > > > > > >
> > > > > > > > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > > > > >
> > > > > > > > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > > > > > > > is derived from (the parent of) clk_spix2, not the other way around?
> > > > > > > > So you can model clk_spi as a fixed divider clock with parent clk_spix2
> > > > > >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > [A]
> > > >
> > > > > > > > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > > > > > > > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > > > > >
> > > > > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> > > > > > > and XSPI node is disabled the clk_summary reports the core clock is ON
> > > > > > > (while it's actually OFF).
> > > > > >
> > > > > > Is that a real problem, or is it purely cosmetic?
> > > > > Just cosmetic tbh as despite being a MOD clock we have to define it as
> > > > > a core clock in the DT.

> > > > > > If spi_clk_spi really must show being disabled, you can change it
> > > > > > from a fixed divider clock (which does not implement .{en,dis}able())
> > > > > > to a custom fixed divider clock that does implement .{en,dis}able()
> > > > > > and keeps track internally of the fake state, or even looks at the
> > > > > > state of spi_clk_spix2?
> > > > > >
> > > > > Good point. Maybe instead of implementing the dummy .{en,dis}able() I
> > > > > will implement the is_enabled() + (clk_fixed_factor_ops). The
> > > > > is_enabled() will take care of reading from the MON bits and report
> > > > > the actual state of the clock.
> > > > >
> > > > > > However, upon second look, spi_clk_spi is not implemented as a fixed
> > > > > > divider clock with parent clk_spix2, as described above:
> > > > > >
> > > > > >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> > > > > >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > > > >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > > > >          spi_aclk          0  0  0 200000000  0  0  50000  N
> > > > > >          spi_hclk          0  0  0 200000000  0  0  50000  N
> > > > > >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> > > > > >
> > > > > > Instead, they both use pllcm33_xspi as the parent clock.
> > > > > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > > > > The changelog for that patch does describe the correct topology?
> > > > > >
> > > > > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > > > > pllcm33_xspi divider and there is a divider (/2) for spi.
> > > >
> > > > Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
> > > > immediate parent.
> > > >
> > > > [A] describes something different:
> > > >
> > > >     .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > >         spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > >
> > > > I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.
> > > >
> > > Okay, thanks - got it.
> > >
> > > To clarify, to implement spi_clk_spi core clock as a parent of
> > > spi_clk_spix2 I will need to implement some sort of mechanism which
> > > registers (late) core clks after core clks and module clks are
> > > registered as spi_clk_spix2 is a module clock.
> >
> > Yes, I wondered about that as well, but wasn't too worried as you
> > already added the smux with e.g. "et0_rxclk" as parent, which also
> > doesn't exist at registration time ;-)
> >
> Good point.
>
> > But indeed, the smux uses clock names to find the parents, while
> > fixed-factor clocks in zv2h_cpg_register_core_clk() expect clock IDs
> > (which are converted to names), and don't handle non-core clocks yet.
> > So either add support for late core clocks, or modify CLK_TYPE_FF
> > to use cpg_core_clock.parent_names[] in case of a non-core parent?
> >
> I choose the late core registration of the clocks and with this the
> core clk_spi still reports `Y` in the summary while the parent is OFF
> (since its a FF clock).

That leads to an interesting philosophical question: if a clock does
not have an .is_enabled() callback, or cannot be disabled, should its
enabled state match the enabled state of its parent?
However, the same question can be asked for a clock that does have an
.is_enabled() callback, and is currently enabled.  What if its parent
is disabled?

> Code implementation for option#1
> ------------------------------------------------


> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c

> @@ -727,8 +727,12 @@ rzv2h_cpg_register_core_clk(const struct
> cpg_core_clk *core,
>          break;
>      case CLK_TYPE_FF:
>          WARN_DEBUG(core->parent >= priv->num_core_clks);
> -        parent = priv->clks[core->parent];
> +        if (late)
> +            parent = priv->clks[priv->num_core_clks + core->parent];
> +        else
> +            parent = priv->clks[core->parent];

I'd rather keep the meaning of the parent ID the same in both cases,
to avoid confusion.

Perhaps keep the (modified) WARN_DEBUG():

    WARN_DEBUG(core->parent >=
               priv->num_core_clks + (late ? priv->num_mod_clks : 0));

>          if (IS_ERR(parent)) {
> +            pr_err("parent clk is NULL for %s parent:%d\n",
> core->name, core->parent);
>              clk = parent;
>              goto fail;
>          }

Thanks , this the simplest option of the two, but still shows the
clock as enabled when it is not.

> # Option#2
> As mentioned in the previous thread I implemented FF clock with
> is_enabled() with this I can see the status of core clk_spi reports
> correct status.

> Code implementation for option#2
> ------------------------------------------------

> --- a/drivers/clk/renesas/rzv2h-cpg.c
> +++ b/drivers/clk/renesas/rzv2h-cpg.c
> @@ -179,6 +179,28 @@ struct rzv2h_plldsi_div_clk {
>  #define to_plldsi_div_clk(_hw) \
>      container_of(_hw, struct rzv2h_plldsi_div_clk, hw)
>
> +/**
> + * struct rzv2h_ff_mod_status_clk - Fixed Factor Module Status Clock
> + *
> + * @priv: CPG private data
> + * @conf: fixed mod configuration
> + * @hw: Fixed Factor Status Clock handle
> + * @mult: multiplier value
> + * @div: divider value
> + * @flags: flags for the clock
> + */
> + struct rzv2h_ff_mod_status_clk {
> +    struct rzv2h_cpg_priv *priv;
> +    struct fixed_mod_conf conf;
> +    struct clk_hw hw;
> +    unsigned int mult;
> +    unsigned int div;
> +    unsigned int flags;

You can replace the last four by embedding struct clk_fixed_factor
(at the top of the struct!).

> +};
> +
> +#define to_rzv2h_ff_mod_status_clk(_hw) \
> +    container_of(_hw, struct rzv2h_ff_mod_status_clk, hw)
> +
>  static int rzv2h_cpg_pll_clk_is_enabled(struct clk_hw *hw)
>  {
>      struct pll_clk *pll_clk = to_pll(hw);
> @@ -664,6 +686,114 @@ rzv2h_cpg_mux_clk_register(const struct
> cpg_core_clk *core,
>      return clk_hw->clk;
>  }
>
> +static unsigned long
> +rzv2h_clk_ff_mod_status_recalc_rate(struct clk_hw *hw,
> +                    unsigned long parent_rate)

[...]

> +static const struct clk_ops rzv2h_clk_ff_mod_status_ops = {
> +    .round_rate = rzv2h_clk_ff_mod_status_round_rate,
> +    .set_rate = rzv2h_clk_ff_mod_status_set_rate,
> +    .recalc_rate = rzv2h_clk_ff_mod_status_recalc_rate,
> +    .recalc_accuracy = rzv2h_clk_ff_mod_status_recalc_accuracy,
> +    .is_enabled = rzv2h_clk_ff_mod_status_is_enabled,
> +};

Lots of code copied from drivers/clk/clk-fixed-factor.c.  Fortunately
clk_fixed_factor_ops is public and exported, so you can just copy its
callback pointers. instead of duplicating the code.

> Please share your thoughts on this.

Thanks , this is (slightly) more complex, and shows the correct
clock state.

Initially, I favored the first solution, due to its simplicity. But
that was before I realized the second option could avoid duplicating
code by reusing the callbacks from clk_fixed_factor_ops...
What do other people think?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Biju Das]

Hi Geert, Prabhakar,

> -----Original Message-----
> From: Geert Uytterhoeven <geert@linux-m68k.org>
> Sent: 19 June 2025 09:17
> Subject: Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support
> 
> Hi Prabhakar,
> 
> On Wed, 18 Jun 2025 at 17:24, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Jun 18, 2025 at 3:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, 18 Jun 2025 at 15:41, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On
> > > > > > > > > > > Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > > > > > > > On Mon, 31 Mar 2025 at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > > > > > > > > Document support for the Expanded Serial
> > > > > > > > > > > > > > > Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Reviewed-by: Rob Herring (Arm)
> > > > > > > > > > > > > > > <robh@kernel.org>
> > > > > > > > > > > > > > > Signed-off-by: Biju Das
> > > > > > > > > > > > > > > <biju.das.jz@bp.renesas.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memo
> > > > > > > > > > > > > > > +++ ry-controllers/renes
> > > > > > > > > > > > > > > +++ as,r
> > > > > > > > > > > > > > > +++ zg3e
> > > > > > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg
> > > > > > > > > > > > > > > + CPG_MOD 0xa1>;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On the next version I am going to update spix2
> > > > > > > > > > > > > > clk as <&cpg CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > > > > > >
> > > > > > > > > According to the RZ/G3E clock system diagram, (the
> > > > > > > > > parent of) clk_spi is derived from (the parent of) clk_spix2, not the other way
> around?
> > > > > > > > > So you can model clk_spi as a fixed divider clock with
> > > > > > > > > parent clk_spix2
> > > > > > >
> > > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > [A]
> > > > >
> > > > > > > > > and factor two.  I.e. provide a new core clock
> > > > > > > > > R9A09G047_SPI_CLK_SPI instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > > > > > >
> > > > > > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a
> > > > > > > > core clock and XSPI node is disabled the clk_summary
> > > > > > > > reports the core clock is ON (while it's actually OFF).
> > > > > > >
> > > > > > > Is that a real problem, or is it purely cosmetic?
> > > > > > Just cosmetic tbh as despite being a MOD clock we have to
> > > > > > define it as a core clock in the DT.
> 
> > > > > > > If spi_clk_spi really must show being disabled, you can
> > > > > > > change it from a fixed divider clock (which does not
> > > > > > > implement .{en,dis}able()) to a custom fixed divider clock
> > > > > > > that does implement .{en,dis}able() and keeps track
> > > > > > > internally of the fake state, or even looks at the state of spi_clk_spix2?
> > > > > > >
> > > > > > Good point. Maybe instead of implementing the dummy
> > > > > > .{en,dis}able() I will implement the is_enabled() +
> > > > > > (clk_fixed_factor_ops). The
> > > > > > is_enabled() will take care of reading from the MON bits and
> > > > > > report the actual state of the clock.
> > > > > >
> > > > > > > However, upon second look, spi_clk_spi is not implemented as
> > > > > > > a fixed divider clock with parent clk_spix2, as described above:
> > > > > > >
> > > > > > >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> > > > > > >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > > > > >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > > > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > > > > >          spi_aclk          0  0  0 200000000  0  0  50000  N
> > > > > > >          spi_hclk          0  0  0 200000000  0  0  50000  N
> > > > > > >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> > > > > > >
> > > > > > > Instead, they both use pllcm33_xspi as the parent clock.
> > > > > > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > > > > > The changelog for that patch does describe the correct topology?
> > > > > > >
> > > > > > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > > > > > pllcm33_xspi divider and there is a divider (/2) for spi.
> > > > >
> > > > > Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
> > > > > immediate parent.
> > > > >
> > > > > [A] describes something different:
> > > > >
> > > > >     .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > > >         spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > > >
> > > > > I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.
> > > > >
> > > > Okay, thanks - got it.
> > > >
> > > > To clarify, to implement spi_clk_spi core clock as a parent of
> > > > spi_clk_spix2 I will need to implement some sort of mechanism
> > > > which registers (late) core clks after core clks and module clks
> > > > are registered as spi_clk_spix2 is a module clock.
> > >
> > > Yes, I wondered about that as well, but wasn't too worried as you
> > > already added the smux with e.g. "et0_rxclk" as parent, which also
> > > doesn't exist at registration time ;-)
> > >
> > Good point.
> >
> > > But indeed, the smux uses clock names to find the parents, while
> > > fixed-factor clocks in zv2h_cpg_register_core_clk() expect clock IDs
> > > (which are converted to names), and don't handle non-core clocks yet.
> > > So either add support for late core clocks, or modify CLK_TYPE_FF to
> > > use cpg_core_clock.parent_names[] in case of a non-core parent?
> > >
> > I choose the late core registration of the clocks and with this the
> > core clk_spi still reports `Y` in the summary while the parent is OFF
> > (since its a FF clock).
> 
> That leads to an interesting philosophical question: if a clock does not have an .is_enabled()
> callback, or cannot be disabled, should its enabled state match the enabled state of its parent?
> However, the same question can be asked for a clock that does have an
> .is_enabled() callback, and is currently enabled.  What if its parent is disabled?
> 
> > Code implementation for option#1
> > ------------------------------------------------
> 
> 
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> 
> > @@ -727,8 +727,12 @@ rzv2h_cpg_register_core_clk(const struct
> > cpg_core_clk *core,
> >          break;
> >      case CLK_TYPE_FF:
> >          WARN_DEBUG(core->parent >= priv->num_core_clks);
> > -        parent = priv->clks[core->parent];
> > +        if (late)
> > +            parent = priv->clks[priv->num_core_clks + core->parent];
> > +        else
> > +            parent = priv->clks[core->parent];
> 
> I'd rather keep the meaning of the parent ID the same in both cases, to avoid confusion.
> 
> Perhaps keep the (modified) WARN_DEBUG():
> 
>     WARN_DEBUG(core->parent >=
>                priv->num_core_clks + (late ? priv->num_mod_clks : 0));
> 
> >          if (IS_ERR(parent)) {
> > +            pr_err("parent clk is NULL for %s parent:%d\n",
> > core->name, core->parent);
> >              clk = parent;
> >              goto fail;
> >          }
> 
> Thanks , this the simplest option of the two, but still shows the clock as enabled when it is not.
> 
> > # Option#2
> > As mentioned in the previous thread I implemented FF clock with
> > is_enabled() with this I can see the status of core clk_spi reports
> > correct status.
> 
> > Code implementation for option#2
> > ------------------------------------------------
> 
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -179,6 +179,28 @@ struct rzv2h_plldsi_div_clk {  #define
> > to_plldsi_div_clk(_hw) \
> >      container_of(_hw, struct rzv2h_plldsi_div_clk, hw)
> >
> > +/**
> > + * struct rzv2h_ff_mod_status_clk - Fixed Factor Module Status Clock
> > + *
> > + * @priv: CPG private data
> > + * @conf: fixed mod configuration
> > + * @hw: Fixed Factor Status Clock handle
> > + * @mult: multiplier value
> > + * @div: divider value
> > + * @flags: flags for the clock
> > + */
> > + struct rzv2h_ff_mod_status_clk {
> > +    struct rzv2h_cpg_priv *priv;
> > +    struct fixed_mod_conf conf;
> > +    struct clk_hw hw;
> > +    unsigned int mult;
> > +    unsigned int div;
> > +    unsigned int flags;
> 
> You can replace the last four by embedding struct clk_fixed_factor (at the top of the struct!).
> 
> > +};
> > +
> > +#define to_rzv2h_ff_mod_status_clk(_hw) \
> > +    container_of(_hw, struct rzv2h_ff_mod_status_clk, hw)
> > +
> >  static int rzv2h_cpg_pll_clk_is_enabled(struct clk_hw *hw)  {
> >      struct pll_clk *pll_clk = to_pll(hw); @@ -664,6 +686,114 @@
> > rzv2h_cpg_mux_clk_register(const struct cpg_core_clk *core,
> >      return clk_hw->clk;
> >  }
> >
> > +static unsigned long
> > +rzv2h_clk_ff_mod_status_recalc_rate(struct clk_hw *hw,
> > +                    unsigned long parent_rate)
> 
> [...]
> 
> > +static const struct clk_ops rzv2h_clk_ff_mod_status_ops = {
> > +    .round_rate = rzv2h_clk_ff_mod_status_round_rate,
> > +    .set_rate = rzv2h_clk_ff_mod_status_set_rate,
> > +    .recalc_rate = rzv2h_clk_ff_mod_status_recalc_rate,
> > +    .recalc_accuracy = rzv2h_clk_ff_mod_status_recalc_accuracy,
> > +    .is_enabled = rzv2h_clk_ff_mod_status_is_enabled,
> > +};
> 
> Lots of code copied from drivers/clk/clk-fixed-factor.c.  Fortunately clk_fixed_factor_ops is public
> and exported, so you can just copy its callback pointers. instead of duplicating the code.
> 
> > Please share your thoughts on this.
> 
> Thanks , this is (slightly) more complex, and shows the correct clock state.
> 
> Initially, I favored the first solution, due to its simplicity. But that was before I realized the
> second option could avoid duplicating code by reusing the callbacks from clk_fixed_factor_ops...
> What do other people think?

+1 for second solution.

Cheers,
Biju

Re: [PATCH v3 1/9] dt-bindings: memory: Document RZ/G3E support

[Lad, Prabhakar]

Hi Geert,

On Thu, Jun 19, 2025 at 9:17 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Wed, 18 Jun 2025 at 17:24, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > On Wed, Jun 18, 2025 at 3:03 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, 18 Jun 2025 at 15:41, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > On Wed, Jun 18, 2025 at 1:59 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > On Wed, 18 Jun 2025 at 14:06, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > On Wed, Jun 18, 2025 at 8:03 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > On Tue, 17 Jun 2025 at 23:05, Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > > > > > > > On Mon, Mar 31, 2025 at 7:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > > > > > > On Mon, 31 Mar 2025 at 17:33, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > > > > > > > On Mon, 31 Mar 2025 at 16:34, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> On Mon, 31 Mar 2025
> > > > > > > > > > > > > at 15:54, Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > > > > > > > > > > > > > > From: Biju Das <biju.das.jz@bp.renesas.com> Document support for
> > > > > > > > > > > > > > > the Expanded Serial Peripheral Interface (xSPI) Controller in
> > > > > > > > > > > > > > > the Renesas RZ/G3E
> > > > > > > > > > > > > > > (R9A09G047) SoC.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > > > > > > > > > > > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > > > > +++ b/Documentation/devicetree/bindings/memory-controllers/renes
> > > > > > > > > > > > > > > +++ as,r
> > > > > > > > > > > > > > > +++ zg3e
> > > > > > > > > > > > > > > +++ -xspi.yaml
> > > > > > > > > > > > >
> > > > > > > > > > > > > > > +    spi@11030000 {
> > > > > > > > > > > > > > > +        compatible = "renesas,r9a09g047-xspi";
> > > > > > > > > > > > > > > +        reg = <0x11030000 0x10000>, <0x20000000 0x10000000>;
> > > > > > > > > > > > > > > +        reg-names = "regs", "dirmap";
> > > > > > > > > > > > > > > +        interrupts = <GIC_SPI 228 IRQ_TYPE_EDGE_RISING>,
> > > > > > > > > > > > > > > +                     <GIC_SPI 229 IRQ_TYPE_EDGE_RISING>;
> > > > > > > > > > > > > > > +        interrupt-names = "pulse", "err_pulse";
> > > > > > > > > > > > > > > +        clocks = <&cpg CPG_MOD 0x9f>, <&cpg CPG_MOD 0xa0>,
> > > > > > > > > > > > > > > +                 <&cpg CPG_MOD 0xa1>, <&cpg CPG_MOD 0xa1>;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On the next version I am going to update spix2 clk as <&cpg
> > > > > > > > > > > > > > CPG_CORE R9A09G047_SPI_CLK_SPIX2>
> > > > > > > > >
> > > > > > > > > According to the RZ/G3E clock system diagram, (the parent of) clk_spi
> > > > > > > > > is derived from (the parent of) clk_spix2, not the other way around?
> > > > > > > > > So you can model clk_spi as a fixed divider clock with parent clk_spix2
> > > > > > >                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > > [A]
> > > > >
> > > > > > > > > and factor two.  I.e. provide a new core clock R9A09G047_SPI_CLK_SPI
> > > > > > > > > instead of your proposed R9A09G047_SPI_CLK_SPIX2?
> > > > > > > > >
> > > > > > > > With this approach when R9A09G047_SPI_CLK_SPI is used as a core clock
> > > > > > > > and XSPI node is disabled the clk_summary reports the core clock is ON
> > > > > > > > (while it's actually OFF).
> > > > > > >
> > > > > > > Is that a real problem, or is it purely cosmetic?
> > > > > > Just cosmetic tbh as despite being a MOD clock we have to define it as
> > > > > > a core clock in the DT.
>
> > > > > > > If spi_clk_spi really must show being disabled, you can change it
> > > > > > > from a fixed divider clock (which does not implement .{en,dis}able())
> > > > > > > to a custom fixed divider clock that does implement .{en,dis}able()
> > > > > > > and keeps track internally of the fake state, or even looks at the
> > > > > > > state of spi_clk_spix2?
> > > > > > >
> > > > > > Good point. Maybe instead of implementing the dummy .{en,dis}able() I
> > > > > > will implement the is_enabled() + (clk_fixed_factor_ops). The
> > > > > > is_enabled() will take care of reading from the MON bits and report
> > > > > > the actual state of the clock.
> > > > > >
> > > > > > > However, upon second look, spi_clk_spi is not implemented as a fixed
> > > > > > > divider clock with parent clk_spix2, as described above:
> > > > > > >
> > > > > > >       .smux2_xspi_clk1     0  0  0 320000000  0  0  50000  Y
> > > > > > >          .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > > > > >             spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > > > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > > > > >          spi_aclk          0  0  0 200000000  0  0  50000  N
> > > > > > >          spi_hclk          0  0  0 200000000  0  0  50000  N
> > > > > > >       .smux2_xspi_clk0     0  0  0 533333333  0  0  50000  Y
> > > > > > >
> > > > > > > Instead, they both use pllcm33_xspi as the parent clock.
> > > > > > > Apparently I missed that in the review of RZ/G3E XSPI clock support.
> > > > > > > The changelog for that patch does describe the correct topology?
> > > > > > >
> > > > > > The topology is correct for RZ/G3E, spi/spix2 are sourced from
> > > > > > pllcm33_xspi divider and there is a divider (/2) for spi.
> > > > >
> > > > > Both spi_clk_spix2 and spi_clk_spix have .pllcm33_xspi as
> > > > > immediate parent.
> > > > >
> > > > > [A] describes something different:
> > > > >
> > > > >     .pllcm33_xspi     0  0  0 40000000   0  0  50000  Y
> > > > >         spi_clk_spix2  0  0  0 40000000   0  0  50000  N
> > > > >             spi_clk_spi    0  0  0 20000000   0  0  50000  Y
> > > > >
> > > > > I.e. if spi_clk_spix2() is disabled, spi_clk_spi() is disabled, too.
> > > > >
> > > > Okay, thanks - got it.
> > > >
> > > > To clarify, to implement spi_clk_spi core clock as a parent of
> > > > spi_clk_spix2 I will need to implement some sort of mechanism which
> > > > registers (late) core clks after core clks and module clks are
> > > > registered as spi_clk_spix2 is a module clock.
> > >
> > > Yes, I wondered about that as well, but wasn't too worried as you
> > > already added the smux with e.g. "et0_rxclk" as parent, which also
> > > doesn't exist at registration time ;-)
> > >
> > Good point.
> >
> > > But indeed, the smux uses clock names to find the parents, while
> > > fixed-factor clocks in zv2h_cpg_register_core_clk() expect clock IDs
> > > (which are converted to names), and don't handle non-core clocks yet.
> > > So either add support for late core clocks, or modify CLK_TYPE_FF
> > > to use cpg_core_clock.parent_names[] in case of a non-core parent?
> > >
> > I choose the late core registration of the clocks and with this the
> > core clk_spi still reports `Y` in the summary while the parent is OFF
> > (since its a FF clock).
>
> That leads to an interesting philosophical question: if a clock does
> not have an .is_enabled() callback, or cannot be disabled, should its
> enabled state match the enabled state of its parent?
> However, the same question can be asked for a clock that does have an
> .is_enabled() callback, and is currently enabled.  What if its parent
> is disabled?
>
That's indeed a subtle and thought-provoking question ;)

> > Code implementation for option#1
> > ------------------------------------------------
>
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
>
> > @@ -727,8 +727,12 @@ rzv2h_cpg_register_core_clk(const struct
> > cpg_core_clk *core,
> >          break;
> >      case CLK_TYPE_FF:
> >          WARN_DEBUG(core->parent >= priv->num_core_clks);
> > -        parent = priv->clks[core->parent];
> > +        if (late)
> > +            parent = priv->clks[priv->num_core_clks + core->parent];
> > +        else
> > +            parent = priv->clks[core->parent];
>
> I'd rather keep the meaning of the parent ID the same in both cases,
> to avoid confusion.
>
> Perhaps keep the (modified) WARN_DEBUG():
>
>     WARN_DEBUG(core->parent >=
>                priv->num_core_clks + (late ? priv->num_mod_clks : 0));
>
Agreed.

> >          if (IS_ERR(parent)) {
> > +            pr_err("parent clk is NULL for %s parent:%d\n",
> > core->name, core->parent);
> >              clk = parent;
> >              goto fail;
> >          }
>
> Thanks , this the simplest option of the two, but still shows the
> clock as enabled when it is not.
>
Yea.

> > # Option#2
> > As mentioned in the previous thread I implemented FF clock with
> > is_enabled() with this I can see the status of core clk_spi reports
> > correct status.
>
> > Code implementation for option#2
> > ------------------------------------------------
>
> > --- a/drivers/clk/renesas/rzv2h-cpg.c
> > +++ b/drivers/clk/renesas/rzv2h-cpg.c
> > @@ -179,6 +179,28 @@ struct rzv2h_plldsi_div_clk {
> >  #define to_plldsi_div_clk(_hw) \
> >      container_of(_hw, struct rzv2h_plldsi_div_clk, hw)
> >
> > +/**
> > + * struct rzv2h_ff_mod_status_clk - Fixed Factor Module Status Clock
> > + *
> > + * @priv: CPG private data
> > + * @conf: fixed mod configuration
> > + * @hw: Fixed Factor Status Clock handle
> > + * @mult: multiplier value
> > + * @div: divider value
> > + * @flags: flags for the clock
> > + */
> > + struct rzv2h_ff_mod_status_clk {
> > +    struct rzv2h_cpg_priv *priv;
> > +    struct fixed_mod_conf conf;
> > +    struct clk_hw hw;
> > +    unsigned int mult;
> > +    unsigned int div;
> > +    unsigned int flags;
>
> You can replace the last four by embedding struct clk_fixed_factor
> (at the top of the struct!).
>
Agreed.

> > +};
> > +
> > +#define to_rzv2h_ff_mod_status_clk(_hw) \
> > +    container_of(_hw, struct rzv2h_ff_mod_status_clk, hw)
> > +
> >  static int rzv2h_cpg_pll_clk_is_enabled(struct clk_hw *hw)
> >  {
> >      struct pll_clk *pll_clk = to_pll(hw);
> > @@ -664,6 +686,114 @@ rzv2h_cpg_mux_clk_register(const struct
> > cpg_core_clk *core,
> >      return clk_hw->clk;
> >  }
> >
> > +static unsigned long
> > +rzv2h_clk_ff_mod_status_recalc_rate(struct clk_hw *hw,
> > +                    unsigned long parent_rate)
>
> [...]
>
> > +static const struct clk_ops rzv2h_clk_ff_mod_status_ops = {
> > +    .round_rate = rzv2h_clk_ff_mod_status_round_rate,
> > +    .set_rate = rzv2h_clk_ff_mod_status_set_rate,
> > +    .recalc_rate = rzv2h_clk_ff_mod_status_recalc_rate,
> > +    .recalc_accuracy = rzv2h_clk_ff_mod_status_recalc_accuracy,
> > +    .is_enabled = rzv2h_clk_ff_mod_status_is_enabled,
> > +};
>
> Lots of code copied from drivers/clk/clk-fixed-factor.c.  Fortunately
> clk_fixed_factor_ops is public and exported, so you can just copy its
> callback pointers. instead of duplicating the code.
>
Agreed.

> > Please share your thoughts on this.
>
> Thanks , this is (slightly) more complex, and shows the correct
> clock state.
>
> Initially, I favored the first solution, due to its simplicity. But
> that was before I realized the second option could avoid duplicating
> code by reusing the callbacks from clk_fixed_factor_ops...
> What do other people think?
>
Given that, Biju is OK with this solution I will go ahead with this approach.

Cheers,
Prabhakar