mbox series

[00/16] Add new Renesas RZ/G2L SoC and Renesas RZ/G2L SMARC EVK support

Message ID 20210514192218.13022-1-prabhakar.mahadev-lad.rj@bp.renesas.com
Headers show
Series Add new Renesas RZ/G2L SoC and Renesas RZ/G2L SMARC EVK support | expand

Message

Prabhakar Mahadev Lad May 14, 2021, 7:22 p.m. UTC
Hi All,

This patch series adds initial support for Renesas RZ/G2L SoC and
Renesas RZ/G2L SMARC EVK.

The RZ/G2L SoC includes a single/dual Cortex-A55 CPU including
below list of IP's:
* Cortex-M33
* 3D Graphics engine (Arm Mali-G31)
* Video Codec (H.264)
* Camera interface (MIPI-CSI or Parallel-IF)
* Display interface (MIPI-DSI or Parallel-IF)
* USB2.0 interface 2ch, SD interface 2ch
* CAN interface (CAN-FD)
* Giga bit Ethernet 2ch

Initial patches enables minimal peripherals on Renesas RZ/G2L
SMARC EVK and booted via initramfs.
* Documentation for RZ/G2{L,LC,UL} SoC variants
* SoC identification support
* CPG core support
* Minimal SoC DTSi
* Minimal DTS for SMARC EVK

Patches are based on top of [1] master branch.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-drivers.git/

Links for SoC and EVK:
[*] https://www.renesas.com/us/en/products/microcontrollers-microprocessors/
rz-arm-based-high-end-32-64-bit-mpus/rzg2l-general-purpose-microprocessors-
dual-core-arm-cortex-a55-12-ghz-cpus-3d-graphics-and-video-codec
[*] https://www.renesas.com/us/en/products/microcontrollers-microprocessors/
rz-arm-based-high-end-32-64-bit-mpus/rzg2lc-general-purpose-microprocessors-
dual-core-arm-cortex-a55-12-ghz-cpus-3d-graphics
[*] https://renesas.info/wiki/RZ-G/RZ-G2L_SMARC

Cheers,
Prabhakar

Biju Das (1):
  serial: sh-sci: Add support for RZ/G2L SoC

Lad Prabhakar (15):
  dt-bindings: arm: renesas: Document Renesas RZ/G2UL SoC
  dt-bindings: arm: renesas: Document Renesas RZ/G2{L,LC} SoC variants
  dt-bindings: arm: renesas: Document SMARC EVK
  soc: renesas: Add ARCH_R9A07G044{L,LC} for the new RZ/G2{L,LC} SoC's
  arm64: defconfig: Enable ARCH_R9A07G044{L,LC}
  dt-bindings: arm: renesas,prr: Add new compatible string for
    RZ/G{L,LC,UL}
  soc: renesas: Add support to read LSI DEVID register
  soc: renesas: Add support to identify RZ/G2{L,LC} SoC's
  dt-bindings: serial: renesas,scif: Document r9a07g044 bindings
  dt-bindings: clock: renesas: Document RZ/G2L SoC CPG driver
  clk: renesas: Define RZ/G2L CPG Clock Definitions
  clk: renesas: Add CPG core wrapper for RZ/G2L SoC
  clk: renesas: Add support for R9A07G044L SoC
  arm64: dts: renesas: Add initial DTSI for RZ/G2{L,LC} SoC's
  arm64: dts: renesas: Add initial device tree for RZ/G2L SMARC EVK

 .../devicetree/bindings/arm/renesas,prr.yaml  |   6 +-
 .../devicetree/bindings/arm/renesas.yaml      |  18 +
 .../bindings/clock/renesas,rzg2l-cpg.yaml     |  80 ++
 .../bindings/serial/renesas,scif.yaml         |   4 +
 arch/arm64/boot/dts/renesas/Makefile          |   2 +
 arch/arm64/boot/dts/renesas/g2l-smarc.dtsi    |  27 +
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi    |  70 ++
 arch/arm64/boot/dts/renesas/r9a07g044l.dtsi   |  21 +
 arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi  |  43 +
 .../boot/dts/renesas/r9a07g044l2-smarc.dts    |  21 +
 arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi  |  62 ++
 arch/arm64/configs/defconfig                  |   2 +
 drivers/clk/renesas/Kconfig                   |  12 +
 drivers/clk/renesas/Makefile                  |   2 +
 drivers/clk/renesas/r9a07g044l-cpg.c          | 372 +++++++
 drivers/clk/renesas/renesas-rzg2l-cpg.c       | 964 ++++++++++++++++++
 drivers/clk/renesas/renesas-rzg2l-cpg.h       | 223 ++++
 drivers/soc/renesas/Kconfig                   |  10 +
 drivers/soc/renesas/renesas-soc.c             |  33 +-
 drivers/tty/serial/sh-sci.c                   |  11 +
 drivers/tty/serial/sh-sci.h                   |   1 +
 include/dt-bindings/clock/r9a07g044l-cpg.h    |  89 ++
 22 files changed, 2070 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
 create mode 100644 arch/arm64/boot/dts/renesas/g2l-smarc.dtsi
 create mode 100644 arch/arm64/boot/dts/renesas/r9a07g044.dtsi
 create mode 100644 arch/arm64/boot/dts/renesas/r9a07g044l.dtsi
 create mode 100644 arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi
 create mode 100644 arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dts
 create mode 100644 arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi
 create mode 100644 drivers/clk/renesas/r9a07g044l-cpg.c
 create mode 100644 drivers/clk/renesas/renesas-rzg2l-cpg.c
 create mode 100644 drivers/clk/renesas/renesas-rzg2l-cpg.h
 create mode 100644 include/dt-bindings/clock/r9a07g044l-cpg.h

Comments

Rob Herring May 18, 2021, 1:31 a.m. UTC | #1
On Fri, 14 May 2021 20:22:03 +0100, Lad Prabhakar wrote:
> Add device tree bindings documentation for Renesas RZ/G2UL SoC.

> 

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>

> ---

>  Documentation/devicetree/bindings/arm/renesas.yaml | 6 ++++++

>  1 file changed, 6 insertions(+)

> 


Acked-by: Rob Herring <robh@kernel.org>
Rob Herring May 18, 2021, 1:32 a.m. UTC | #2
On Fri, 14 May 2021 20:22:05 +0100, Lad Prabhakar wrote:
> Document Renesas SMARC EVK boards which are based on RZ/G2L (R9A07G044L)

> SoC. The SMARC EVK consists of RZ/G2L SoM module and SMARC carrier board,

> the SoM module sits on top of carrier board.

> 

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>

> ---

>  Documentation/devicetree/bindings/arm/renesas.yaml | 3 +++

>  1 file changed, 3 insertions(+)

> 


Acked-by: Rob Herring <robh@kernel.org>
Rob Herring May 18, 2021, 1:35 a.m. UTC | #3
On Fri, 14 May 2021 20:22:13 +0100, Lad Prabhakar wrote:
> Document the device tree bindings of the Renesas RZ/G2L SoC clock
> driver in Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  .../bindings/clock/renesas,rzg2l-cpg.yaml     | 80 +++++++++++++++++++
>  1 file changed, 80 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Geert Uytterhoeven May 21, 2021, 1:22 p.m. UTC | #4
Hi Prabhakar,

On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add device tree bindings documentation for Renesas RZ/G2UL SoC.

>

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>


Thanks for your patch!

> --- a/Documentation/devicetree/bindings/arm/renesas.yaml

> +++ b/Documentation/devicetree/bindings/arm/renesas.yaml

> @@ -302,6 +302,12 @@ properties:

>                - renesas,rzn1d400-db # RZN1D-DB (RZ/N1D Demo Board for the RZ/N1D 400 pins package)

>            - const: renesas,r9a06g032

>

> +      - description: RZ/G2UL (R9A07G043)

> +        items:

> +          - enum:

> +              - renesas,r9a07g043u11 # Single Cortex-A55 RZ/G2UL


Is there any specific reason you're including the final "1", unlike the
RZ/G2{L,LC} binding?
As RZ/G2UL is always single-core, perhaps this compatible value can be
dropped?

> +          - const: renesas,r9a07g043

> +

>  additionalProperties: true


For now, there are no users of this binding?
I assume you're posting it already, as RZ/G2UL is pin-compatible with RZ/G2LC,
and thus can be used interchangeably on the G2L SOM?
However, the DTS board part in this series is for RZ/G2L, not RZ/GLC?

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

i.e. will queue in renesas-devel for v5.14, after the above have been
resolved.

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
Geert Uytterhoeven May 21, 2021, 1:24 p.m. UTC | #5
Hi Prabhakar,

On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Document Renesas SMARC EVK boards which are based on RZ/G2L (R9A07G044L)

> SoC. The SMARC EVK consists of RZ/G2L SoM module and SMARC carrier board,

> the SoM module sits on top of carrier board.

>

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>


> --- a/Documentation/devicetree/bindings/arm/renesas.yaml

> +++ b/Documentation/devicetree/bindings/arm/renesas.yaml

> @@ -310,6 +310,9 @@ properties:

>

>        - description: RZ/G2{L,LC} (R9A07G044)

>          items:

> +          - enum:

> +              - renesas,smarc-r9a07g044l1 # SMARC EVK with single Cortex-A55

> +              - renesas,smarc-r9a07g044l2 # SMARC EVK with dual Cortex-A55

>            - enum:

>                - renesas,r9a07g044c1 # Single Cortex-A55 RZ/G2LC

>                - renesas,r9a07g044c2 # Dual Cortex-A55 RZ/G2LC


Likewise, do we care (at the top level) if this is a board with an SoC
die that has one or two Cortex-A55 cores enabled?

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
Geert Uytterhoeven May 21, 2021, 1:25 p.m. UTC | #6
Hi Prabhakar,

On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add ARCH_R9A07G044{L,LC} as a configuration symbol for the new Renesas

> RZ/G2{L,LC} SoC's.

>

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>


Thanks for your patch!

> --- a/drivers/soc/renesas/Kconfig

> +++ b/drivers/soc/renesas/Kconfig

> @@ -279,6 +279,16 @@ config ARCH_R8A774B1

>         help

>           This enables support for the Renesas RZ/G2N SoC.

>

> +config ARCH_R9A07G044L

> +       bool "ARM64 Platform support for RZ/G2L SoC"


Please drop the "SoC", for consistency with other entries.

> +       help

> +         This enables support for the Renesas RZ/G2L SoC.

> +

> +config ARCH_R9A07G044LC

> +       bool "ARM64 Platform support for RZ/G2LC SoC"


Likewise.

> +       help

> +         This enables support for the Renesas RZ/G2LC SoC.

> +

>  endif # ARM64


Given LSI DEVID is the same, do we need both, or can we do with a
single ARCH_R9A07G044?

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
Geert Uytterhoeven May 21, 2021, 1:26 p.m. UTC | #7
Hi Prabhakar,

On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> From: Biju Das <biju.das.jz@bp.renesas.com>
>
> Add serial support for RZ/G2L SoC with earlycon and
> extended mode register support.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -306,6 +306,7 @@ static const struct sci_port_params sci_port_params[SCIx_NR_REGTYPES] = {
>                         [SCFDR]         = { 0x0E, 16 },
>                         [SCSPTR]        = { 0x10, 16 },
>                         [SCLSR]         = { 0x12, 16 },
> +                       [SEMR]          = { 0x14, 8 },

This is the parameter section for RZ/T and RZ/A2.  Please update the
comments above, to say this also applies to RZ/G2L.
I can confirm the documentation for RZ/T1 and RZ/A2 agrees about the
existence and behavior of SEMR.

>                 },
>                 .fifosize = 16,
>                 .overrun_reg = SCLSR,
> @@ -2527,6 +2528,8 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
>                         case 27: smr_val |= SCSMR_SRC_27; break;
>                         }
>                 smr_val |= cks;
> +               if (sci_getreg(port, SEMR)->size)
> +                       serial_port_out(port, SEMR, 0);

As this is done in both branches of the if() statement, I think it
should be moved up.

>                 serial_port_out(port, SCSCR, scr_val | s->hscif_tot);
>                 serial_port_out(port, SCSMR, smr_val);
>                 serial_port_out(port, SCBRR, brr);
> @@ -2561,6 +2564,8 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
>                 scr_val = s->cfg->scscr & (SCSCR_CKE1 | SCSCR_CKE0);
>                 smr_val |= serial_port_in(port, SCSMR) &
>                            (SCSMR_CKEDG | SCSMR_SRC_MASK | SCSMR_CKS);
> +               if (sci_getreg(port, SEMR)->size)
> +                       serial_port_out(port, SEMR, 0);

(else branch)

>                 serial_port_out(port, SCSCR, scr_val | s->hscif_tot);
>                 serial_port_out(port, SCSMR, smr_val);
>         }
> @@ -3170,6 +3175,10 @@ static const struct of_device_id of_sci_match[] = {
>                 .compatible = "renesas,scif-r7s9210",
>                 .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
>         },
> +       {
> +               .compatible = "renesas,scif-r9a07g044",
> +               .data = SCI_OF_DATA(PORT_SCIF, SCIx_RZ_SCIFA_REGTYPE),
> +       },
>         /* Family-specific types */
>         {
>                 .compatible = "renesas,rcar-gen1-scif",

The rest looks good to me.

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
Geert Uytterhoeven May 21, 2021, 3:02 p.m. UTC | #8
Hi Prabhakar,

On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add CPG core wrapper for RZ/G2L family.

>

> Based on a patch in the BSP by Binh Nguyen

> <binh.nguyen.jz@renesas.com>.

>

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>


Thanks for your patch!

> --- a/drivers/clk/renesas/Kconfig

> +++ b/drivers/clk/renesas/Kconfig

> @@ -34,6 +34,10 @@ config CLK_RENESAS

>         select CLK_R9A06G032 if ARCH_R9A06G032

>         select CLK_SH73A0 if ARCH_SH73A0

>

> +config CLK_RENESAS_RZG2L

> +       bool "Renesas RZ/G2L SoC clock support" if COMPILE_TEST && !ARCH_RENESAS

> +       default y if ARCH_RENESAS


Why do you need this symbol here, and why does it default to y?

I guess the plan is to share this by the RZ/G2L(C) and RZ/G2UL clock
drivers, so probably you want to move it to the family-specific
section below, like CLK_RCAR_GEN3_CPG.

> +

>  if CLK_RENESAS

>

>  # SoC

> diff --git a/drivers/clk/renesas/Makefile b/drivers/clk/renesas/Makefile

> index ef0d2bba92bf..e4f3d7fab67c 100644

> --- a/drivers/clk/renesas/Makefile

> +++ b/drivers/clk/renesas/Makefile

> @@ -36,6 +36,7 @@ obj-$(CONFIG_CLK_RCAR_CPG_LIB)                += rcar-cpg-lib.o

>  obj-$(CONFIG_CLK_RCAR_GEN2_CPG)                += rcar-gen2-cpg.o

>  obj-$(CONFIG_CLK_RCAR_GEN3_CPG)                += rcar-gen3-cpg.o

>  obj-$(CONFIG_CLK_RCAR_USB2_CLOCK_SEL)  += rcar-usb2-clock-sel.o

> +obj-$(CONFIG_CLK_RENESAS_RZG2L)                += renesas-rzg2l-cpg.o


As CLK_RENESAS_RZG2L defaults to y, this is compiled by
default on ARCH_RENESAS.

>

>  # Generic

>  obj-$(CONFIG_CLK_RENESAS_CPG_MSSR)     += renesas-cpg-mssr.o

> diff --git a/drivers/clk/renesas/renesas-rzg2l-cpg.c b/drivers/clk/renesas/renesas-rzg2l-cpg.c

> new file mode 100644

> index 000000000000..dc54ffc6114c

> --- /dev/null

> +++ b/drivers/clk/renesas/renesas-rzg2l-cpg.c

> @@ -0,0 +1,958 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/*

> + * RZ/G2L Clock Pulse Generator

> + *

> + * Copyright (C) 2021 Renesas Electronics Corp.

> + *

> + * Based on renesas-cpg-mssr.c

> + *

> + * Copyright (C) 2015 Glider bvba

> + * Copyright (C) 2013 Ideas On Board SPRL

> + * Copyright (C) 2015 Renesas Electronics Corp.

> + */

> +

> +#include <linux/clk.h>

> +#include <linux/clk-provider.h>

> +#include <linux/clk/renesas.h>

> +#include <linux/delay.h>

> +#include <linux/device.h>

> +#include <linux/init.h>

> +#include <linux/mod_devicetable.h>

> +#include <linux/module.h>

> +#include <linux/of_address.h>

> +#include <linux/of_device.h>

> +#include <linux/platform_device.h>

> +#include <linux/pm_clock.h>

> +#include <linux/pm_domain.h>

> +#include <linux/psci.h>


Unused

> +#include <linux/reset-controller.h>

> +#include <linux/slab.h>

> +

> +#include <dt-bindings/clock/renesas-cpg-mssr.h>

> +

> +#include "renesas-rzg2l-cpg.h"

> +

> +#ifdef DEBUG

> +#define WARN_DEBUG(x)  WARN_ON(x)

> +#else

> +#define WARN_DEBUG(x)  do { } while (0)

> +#endif

> +

> +#define DIV_RSMASK(v, s, m)    ((v >> s) & m)

> +#define GET_REG(val)           ((val >> 20) & 0xfff)

> +#define GET_SHIFT(val)         ((val >> 12) & 0xff)

> +#define GET_WIDTH(val)         ((val >> 8) & 0xf)

> +

> +#define KDIV(val)              DIV_RSMASK(val, 16, 0xffff)

> +#define MDIV(val)              DIV_RSMASK(val, 6, 0x3ff)

> +#define PDIV(val)              DIV_RSMASK(val, 0, 0x3f)

> +#define SDIV(val)              DIV_RSMASK(val, 0, 0x7)

> +#define REFDIV(val)            DIV_RSMASK(val, 8, 0x3f)

> +#define POSTDIV1(val)          DIV_RSMASK(val, 0, 0x7)

> +#define POSTDIV2(val)          DIV_RSMASK(val, 4, 0x7)

> +#define FRACIN(val)            DIV_RSMASK(val, 8, 0xffffff)

> +#define INITIN(val)            DIV_RSMASK(val, 16, 0xfff)

> +

> +/**

> + * Clock Pulse Generator Private Data


struct cpg_mssr_priv - Clock Pulse Generator Private Data

> + *

> + * @rcdev: Optional reset controller entity

> + * @dev: CPG/MSSR device


CPG?

> + * @base: CPG/MSSR register block base address


CPG?

> + * @rmw_lock: protects RMW register accesses

> + * @clks: Array containing all Core and Module Clocks

> + * @num_core_clks: Number of Core Clocks in clks[]

> + * @num_mod_clks: Number of Module Clocks in clks[]

> + * @last_dt_core_clk: ID of the last Core Clock exported to DT

> + * @notifiers: Notifier chain to save/restore clock state for system resume

> + * @info: Pointer to platform data

> + */

> +struct cpg_mssr_priv {


rzg2l_cpg_priv?

> +#ifdef CONFIG_RESET_CONTROLLER

> +       struct reset_controller_dev rcdev;

> +#endif

> +       struct device *dev;

> +       void __iomem *base;

> +       spinlock_t rmw_lock;

> +

> +       struct clk **clks;

> +       unsigned int num_core_clks;

> +       unsigned int num_mod_clks;

> +       unsigned int last_dt_core_clk;

> +

> +       struct raw_notifier_head notifiers;

> +       const struct cpg_mssr_info *info;

> +};



> +struct div2_clk {

> +       struct clk_hw hw;

> +       unsigned int conf;

> +       const struct clk_div_table *dtable;

> +       unsigned int confs;

> +       const struct clk_div_table *dtables;


Please don't interleave pointers and integers on a 64-bit platform,
as it requires padding to keep the pointers naturally aligned.
Same for struct pll_clk below.

> +       void __iomem *base;

> +       struct cpg_mssr_priv *priv;

> +};

> +

> +#define to_d2clk(_hw)  container_of(_hw, struct div2_clk, hw)

> +

> +static unsigned int div2_clock_get_div(unsigned int val,

> +                                      const struct clk_div_table *t,

> +                                      int length)


unsigned int

> +{

> +       int i;


unsigned int (for all positive loop counters)

> +

> +       for (i = 0; i <= length; i++)

> +               if (val == t[i].val)

> +                       return t[i].div;

> +

> +       /* return div=1 if failed */

> +       return 1;

> +}


> +static long rzg2l_cpg_div2_clk_round_rate(struct clk_hw *hw,

> +                                         unsigned long rate,

> +                                         unsigned long *parent_rate)


Please implement .determine_rate() instead of .round_rate() in new
drivers.

> +{

> +       unsigned long best_diff = (unsigned long)-1;

> +       unsigned int best_div, best_divs, div, divs;

> +       struct div2_clk *d2clk = to_d2clk(hw);

> +       unsigned long diff;

> +       int i, j, n, ns;

> +

> +       n = BIT(GET_WIDTH(d2clk->conf)) - 1;

> +       ns = BIT(GET_WIDTH(d2clk->confs)) - 1;

> +       for (i = 0; i <= n; i++) {

> +               for (j = 0; j <= ns; j++) {

> +                       div = div2_clock_get_div(i, d2clk->dtable, n);

> +                       divs = div2_clock_get_div(j, d2clk->dtables, ns);

> +                       diff = abs(*parent_rate - (rate * div * divs));

> +                       if (best_diff > diff) {

> +                               best_diff = diff;

> +                               best_div = div;

> +                               best_divs = divs;

> +                       }

> +               }

> +       }

> +

> +       return DIV_ROUND_CLOSEST_ULL((u64)*parent_rate, best_div * best_divs);

> +}


> +/**

> + * struct mstp_clock - MSTP gating clock

> + * @hw: handle between common and hardware-specific interfaces

> + * @bit: 16bits register offset, 8bits ON/MON, 8bits RESET

> + * @priv: CPG/MSSR private data

> + */

> +struct mstp_clock {

> +       struct clk_hw hw;

> +       u32 bit;


I think the code accessing this field can be simplified by not packing
everything into a 32-bit value:

    u16 off;
    u8 onoff;
    u8 reset;

> +       struct cpg_mssr_priv *priv;

> +};


> diff --git a/drivers/clk/renesas/renesas-rzg2l-cpg.h b/drivers/clk/renesas/renesas-rzg2l-cpg.h

> new file mode 100644

> index 000000000000..8dce6b5546b1

> --- /dev/null

> +++ b/drivers/clk/renesas/renesas-rzg2l-cpg.h

> @@ -0,0 +1,221 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +/*

> + * RZ/G2L Clock Pulse Generator

> + *

> + * Copyright (C) 2021 Renesas Electronics Corp.

> + *

> + */

> +

> +#ifndef __RENESAS_RZG2L_CPG_H__

> +#define __RENESAS_RZG2L_CPG_H__

> +

> +/* Register offset */

> +/* n : 0, 1, 2 : PLL1, PLL4, PLL6 */

> +#define PLL146_CLK1_R(n)       (0x04 + (16 * n))

> +#define PLL146_CLK2_R(n)       (0x08 + (16 * n))

> +#define PLL146_MON_R(n)                (0x0C + (16 * n))

> +#define PLL146_STBY_R(n)       (0x00 + (16 * n))

> +

> +/* n : 0, 1, 2 : PLL2, PLL3, PLL5 */

> +#define PLL235_CLK1_R(n)       (0x104 + (32 * n))

> +#define PLL235_CLK3_R(n)       (0x10c + (32 * n))

> +#define PLL235_CLK4_R(n)       (0x110 + (32 * n))

> +#define PLL235_MON_R(n)                (0x11C + (32 * n))

> +#define PLL235_STBY_R(n)       (0x100 + (32 * n))

> +

> +#define PLL1_DIV_R             (0x200)

> +#define PLL2_DIV_R             (0x204)

> +#define PLL3A_DIV_R            (0x208)

> +#define PLL3B_DIV_R            (0x20c)

> +#define PLL6_DIV_R             (0x210)

> +#define PL2SDHI_SEL_R          (0x218)

> +#define CLK_STATUS_R           (0x280)

> +#define CA55_SSEL_R            (0x400)

> +#define PL2_SSEL_R             (0x404)

> +#define PL3_SSEL_R             (0x408)

> +#define PL4_DSEL_R             (0x21C)

> +#define PL5_SSEL_R             (0x410)

> +#define PL6_SSEL_R             (0x414)

> +#define PL6_ETH_SSEL_R         (0x418)

> +#define PL5_SDIV_R             (0x420)

> +#define OTHERFUNC1_R           (0xBE8)

> +#define CLK_ON_R(reg)          (0x500 + reg)

> +#define CLK_MON_R(reg)         (0x680 + reg)

> +#define CLK_RST_R(reg)         (0x800 + reg)

> +#define CLK_MRST_R(reg)                (0x980 + reg)


The register offsets are only used by renesas-rzg2l-cpg.c, so they
can be moved there.

> +

> +#define SEL_PLL1       (CA55_SSEL_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_PLL2_1     (PL2_SSEL_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_PLL2_2     (PL2_SSEL_R << 20 | 4 << 12 | 1 << 8)

> +#define SEL_PLL3_1     (PL3_SSEL_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_PLL3_2     (PL3_SSEL_R << 20 | 4 << 12 | 1 << 8)

> +#define SEL_PLL3_3     (PL3_SSEL_R << 20 | 8 << 12 | 1 << 8)

> +#define SEL_PLL4       (PL4_DSEL_R << 20 | 8 << 12 | 1 << 8)

> +#define SEL_PLL5_1     (PL5_SSEL_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_PLL5_2     (PL5_SSEL_R << 20 | 4 << 12 | 1 << 8)

> +#define SEL_PLL5_3     (PL5_SSEL_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_PLL5_4     (OTHERFUNC1_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_PLL6_1     (PL6_SSEL_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_PLL6_2     (PL6_ETH_SSEL_R << 20 | 0 << 12 | 1 << 8)

> +#define SEL_ETH                (PL6_ETH_SSEL_R << 20 | 4 << 12 | 1 << 8)

> +#define SEL_G1_1       (PL6_SSEL_R << 20 | 4 << 12 | 1 << 8)

> +#define SEL_G1_2       (PL6_SSEL_R << 20 | 8 << 12 | 1 << 8)

> +#define SEL_G2         (PL6_SSEL_R << 20 | 12 << 12 | 1 << 8)

> +#define SEL_SDHI0      (PL2SDHI_SEL_R << 20 | 0 << 12 | 2 << 8)

> +#define SEL_SDHI1      (PL2SDHI_SEL_R << 20 | 4 << 12 | 2 << 8)

> +#define DIVPL1         (PLL1_DIV_R << 20 | 0 << 12 | 2 << 8)

> +#define DIVPL2A                (PLL2_DIV_R << 20 | 0 << 12 | 3 << 8)

> +#define DIVPL2B                (PLL2_DIV_R << 20 | 4 << 12 | 3 << 8)

> +#define DIVPL2C                (PLL2_DIV_R << 20 | 8 << 12 | 3 << 8)

> +#define DIVDSILPCL     (PLL2_DIV_R << 20 | 12 << 12 | 2 << 8)

> +#define DIVPL3A                (PLL3A_DIV_R << 20 | 0 << 12 | 3 << 8)

> +#define DIVPL3B                (PLL3A_DIV_R << 20 | 4 << 12 | 3 << 8)

> +#define DIVPL3C                (PLL3A_DIV_R << 20 | 8 << 12 | 3 << 8)

> +#define DIVPL3CLK200FIX        (PLL3B_DIV_R << 20 | 0 << 12 | 3 << 8)

> +#define DIVGPU         (PLL6_DIV_R << 20 | 0 << 12 | 2 << 8)

> +#define DIVDSIB                (PL5_SDIV_R << 20 | 8 << 12 | 4 << 8)

> +#define DIVDSIA                (PL5_SDIV_R << 20 | 0 << 12 | 2 << 8)

> +

> +#define PLL146_STBY(n) (PLL146_STBY_R(n) << 20 | 2 << 16 | 0 << 12)

> +#define PLL146_MON(n)  (PLL146_MON_R(n) << 20 | 4 << 16 | 0 << 12)

> +#define PLL235_STBY(n) (PLL235_STBY_R(n) << 20 | 2 << 16 | 0 << 12)

> +#define PLL235_MON(n)  (PLL235_MON_R(n) << 20 | 4 << 16 | 0 << 12)

> +

> +#define PLL146_CONF(n) (PLL146_CLK1_R(n) << 22 | PLL146_CLK2_R(n) << 12 | 0)

> +#define PLL235_CONF(n) (PLL235_CLK1_R(n) << 22 | PLL235_CLK3_R(n) << 12 | PLL235_CLK4_R(n))

> +

> +#define GET_REG1(val)  ((val >> 22) & 0x3ff)

> +#define GET_REG2(val)  ((val >> 12) & 0x3ff)

> +#define GET_REG3(val)  (val & 0x3ff)


The GET_REG() macros are only used by renesas-rzg2l-cpg.c.

> +

> +#define MSSR(off, on, res)     ((off & 0xffff) << 16 | \

> +                                (on & 0xff) << 8 | (res & 0xff))

> +#define MSSR_OFF(val)  ((val >> 16) & 0xffff)

> +#define MSSR_ON(val)   ((val >> 8) & 0xff)

> +#define MSSR_RES(val)  (val & 0xff)


Can be removed after mstp_clock.bit split.

> +/*

> + * Definitions of Module Clocks

> + * @name: handle between common and hardware-specific interfaces

> + * @id: clock index in array containing all Core and Module Clocks

> + * @parent: id of parent clock

> + * @bit: 16bits register offset, 8bits ON/MON, 8bits RESET

> + */

> +struct mssr_mod_clk {


rzg2l_mod_clk?

> +       const char *name;

> +       unsigned int id;

> +       unsigned int parent;

> +       unsigned int bit;


Same comment as for mstp_clock.bit.

> +};

> +

> +#define DEF_MOD(_name, _id, _parent, _bit)     \

> +       { .name = _name, .id = MOD_CLK_BASE + _id, .parent = _parent,\

> +       .bit = _bit }

> +

> +/**

> + * SoC-specific CPG/MSSR Description

> + *

> + * @core_clks: Array of Core Clock definitions

> + * @num_core_clks: Number of entries in 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)

> + *

> + * @pll_info: array of PLL register info

> + * @pll_info_size: Total number of PLL registers info

> + *

> + * @mod_clks: Array of Module Clock definitions

> + * @num_mod_clks: Number of entries in mod_clks[]

> + * @num_hw_mod_clks: Number of Module Clocks supported by the hardware

> + *

> + * @crit_mod_clks: Array with Module Clock IDs of critical clocks that

> + *                 should not be disabled without a knowledgeable driver

> + * @num_crit_mod_clks: Number of entries in crit_mod_clks[]

> + */

> +struct cpg_mssr_info {


rzg2l_cpg_info?

> +       /* Core Clocks */

> +       const struct cpg_core_clk *core_clks;

> +       unsigned int num_core_clks;

> +       unsigned int last_dt_core_clk;

> +       unsigned int num_total_core_clks;

> +

> +       /* Module Clocks */

> +       const struct mssr_mod_clk *mod_clks;

> +       unsigned int num_mod_clks;

> +       unsigned int num_hw_mod_clks;

> +

> +       /* Critical Module Clocks that should not be disabled */

> +       const unsigned int *crit_mod_clks;

> +       unsigned int num_crit_mod_clks;

> +};

> +

> +#endif


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
Geert Uytterhoeven May 21, 2021, 3:04 p.m. UTC | #9
Hi Prabhakar,

On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Document the device tree bindings of the Renesas RZ/G2L SoC clock
> driver in Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
> @@ -0,0 +1,80 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/clock/renesas,rzg2l-cpg.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Renesas RZ/G2L Clock Pulse Generator / Module Stop and Software Reset

(Module Standby Mode
> +
> +maintainers:
> +  - Geert Uytterhoeven <geert+renesas@glider.be>
> +
> +description: |
> +  On Renesas RZ/G2L SoC, the CPG (Clock Pulse Generator) and MSTP
> +  (Module Stop and Software Reset) share the same register block.
> +
> +  They provide the following functionalities:
> +    - The CPG block generates various core clocks,
> +    - The MSTP block provides two functions:
> +        1. Module Stop, providing a Clock Domain to control the clock supply
> +           to individual SoC devices,
> +        2. Reset Control, to perform a software reset of individual SoC devices.
> +
> +properties:
> +  compatible:
> +    const: renesas,r9a07g044l-cpg  # RZ/G2L

renesas,r9a07g044-cpg?

I believe it's the same block on RZ/G2L ('044l) and RZ/G2LC ('044c).

> +  '#clock-cells':
> +    description: |
> +      - For CPG core clocks, the two clock specifier cells must be "CPG_CORE"
> +        and a core clock reference, as defined in
> +        <dt-bindings/clock/*-cpg-mssr.h>

<dt-bindings/clock/r9a07g044l-cpg.h>

> +      - For module clocks, the two clock specifier cells must be "CPG_MOD" and
> +        a module number, as defined in the datasheet.

Also in <dt-bindings/clock/r9a07g044l-cpg.h>?

> +    const: 2
> +
> +  '#power-domain-cells':
> +    description:
> +      SoC devices that are part of the CPG/MSTP Clock Domain and can be
> +      power-managed through Module Stop should refer to the CPG device node
> +      in their "power-domains" property, as documented by the generic PM Domain
> +      bindings in Documentation/devicetree/bindings/power/power-domain.yaml.
> +    const: 0
> +
> +  '#reset-cells':
> +    description:
> +      The single reset specifier cell must be the module number, as defined in
> +      the datasheet.

Also in <dt-bindings/clock/r9a07g044l-cpg.h>?

> +    const: 1

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
Geert Uytterhoeven May 21, 2021, 3:35 p.m. UTC | #10
Hi Prabhakar,

On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add initial DTSI for RZ/G2{L,LC} SoC's.
>
> File structure:
> r9a07g044.dtsi  => RZ/G2L family SoC common parts
> r9a07g044l.dtsi => Specific to RZ/G2L (R9A07G044L) SoC
> r9a07g044l1.dtsi => Specific to RZ/G2L (R9A07G044L single cortex A55) SoC
> r9a07g044l2.dtsi => Specific to RZ/G2L (R9A07G044L dual cortex A55) SoC
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> index 000000000000..c625d302f889
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0

Do we want to use

    SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

for new DTS files?
This actually also applies to <dt-bindings/...> files.

> +/*
> + * Device Tree Source for the RZ/G2L and RZ/G2LC common SoC parts
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/r9a07g044l-cpg.h>
> +
> +/ {
> +       compatible = "renesas,r9a07g044";
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +
> +       extal_clk: extal {
> +               compatible = "fixed-clock";
> +               #clock-cells = <0>;
> +               /* This value must be overridden by the board */
> +               clock-frequency = <0>;
> +       };
> +
> +       psci {
> +               compatible = "arm,psci-1.0", "arm,psci-0.2";
> +               method = "smc";
> +       };
> +
> +       soc: soc {
> +               compatible = "simple-bus";
> +               interrupt-parent = <&gic>;
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +
> +               scif0: serial@1004b800 {
> +                       compatible = "renesas,scif-r9a07g044";
> +                       reg = <0 0x1004b800 0 0x400>;
> +                       interrupts =
> +                               <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>,
> +                               <GIC_SPI 382 IRQ_TYPE_LEVEL_HIGH>,
> +                               <GIC_SPI 383 IRQ_TYPE_LEVEL_HIGH>,
> +                               <GIC_SPI 381 IRQ_TYPE_LEVEL_HIGH>,
> +                               <GIC_SPI 384 IRQ_TYPE_LEVEL_HIGH>;

"make dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/serial/renesas,scif.yaml":

    arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dt.yaml:
serial@1004b800: interrupts: 'oneOf' conditional failed, one must be
fixed:
    [[0, 380, 4], [0, 382, 4], [0, 383, 4], [0, 381, 4], [0, 384, 4]]
is too long
    Additional items are not allowed ([0, 382, 4], [0, 383, 4], [0,
381, 4], [0, 384, 4] were unexpected)
    Additional items are not allowed ([0, 384, 4] was unexpected)
    [[0, 380, 4], [0, 382, 4], [0, 383, 4], [0, 381, 4], [0, 384, 4]]
is too short

One interrupt is missing.  According to the documentation, "tei" and
"dri" share an interrupt, so they should map to the same interrupt number.
Please add interrupt-names.

> +                       clocks = <&cpg CPG_MOD R9A07G044_CLK_SCIF0>;
> +                       clock-names = "fck";
> +                       power-domains = <&cpg>;
> +                       resets = <&cpg R9A07G044_CLK_SCIF0>;
> +                       status = "disabled";
> +               };
> +
> +               devid: chipid@11020a04 {
> +                       compatible = "renesas,devid";
> +                       reg = <0 0x11020a04 0 4>;
> +               };
> +
> +               gic: interrupt-controller@11900000 {
> +                       compatible = "arm,gic-v3";
> +                       #interrupt-cells = <3>;
> +                       #address-cells = <0>;
> +                       interrupt-controller;
> +                       reg = <0x0 0x11900000 0 0x40000>,
> +                             <0x0 0x11940000 0 0x60000>;
> +                       interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
> +                       clocks = <&cpg CPG_MOD R9A07G044_CLK_GIC600>;
> +                       clock-names = "gic6000";

This looks like a weird name ;-)
In addition, it should be the consumer clock name, not the provider
clock name.

> +                       power-domains = <&cpg>;
> +                       resets = <&cpg R9A07G044_CLK_GIC600>;

"make dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml":

    arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dt.yaml:
interrupt-controller@11900000: 'clock-names', 'clocks',
'power-domains', 'resets' do not match any of the regexes:
'^(msi-controller|gic-its|interrupt-controller)@[0-9a-f]+$',
'^gic-its@', '^interrupt-controller@[0-9a-f]+$', 'pinctrl-[0-9]+'

These properties should be added to the GIC-v3 bindings, cfr. the normal
GIC bindings.


> +               };
> +       };
> +};
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g044l.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044l.dtsi
> new file mode 100644
> index 000000000000..8d396b9100c1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044l.dtsi
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the RZ/G2L common SoC parts
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +/dts-v1/;
> +#include "r9a07g044.dtsi"
> +
> +&soc {
> +       cpg: clock-controller@11010000 {
> +               compatible = "renesas,r9a07g044l-cpg";
> +               reg = <0 0x11010000 0 0x10000>;
> +               clocks = <&extal_clk>;
> +               clock-names = "extal";
> +               #clock-cells = <2>;
> +               #reset-cells = <1>;
> +               #power-domain-cells = <0>;
> +       };

As I think this is shared by RZ/G2L and RZ/G2LC, it belongs to
r9a07g044.dtsi.

> +};
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi
> new file mode 100644
> index 000000000000..44d4504e44c3
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the RZ/G2L R9A07G044L1 common parts
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +/dts-v1/;
> +#include "r9a07g044l.dtsi"
> +
> +/ {
> +       compatible = "renesas,r9a07g044l1";
> +       #address-cells = <2>;
> +       #size-cells = <2>;

#{address,size}-cells already defined in r9a07g044.dtsi.

> +
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               a55_0: cpu@0 {
> +                       compatible = "arm,cortex-a55";
> +                       reg = <0>;
> +                       device_type = "cpu";
> +                       next-level-cache = <&L3_CA55>;
> +                       enable-method = "psci";
> +               };
> +
> +               L3_CA55: cache-controller-0 {
> +                       compatible = "cache";
> +                       cache-unified;
> +                       cache-size = <0x40000>;
> +               };

I think the first CPU core should be in the base r9a07g044.dtsi file.

> +       };
> +
> +       timer {
> +               compatible = "arm,armv8-timer";
> +               interrupts-extended =
> +                       <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>,
> +                       <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>,
> +                       <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>,
> +                       <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>;

Also in the base file, with interrupts-extended overridden where
needed?

> +       };
> +};
> diff --git a/arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi
> new file mode 100644
> index 000000000000..33bb35e1c369
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the RZ/G2L R9A07G044L2 common parts
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +/dts-v1/;
> +#include "r9a07g044l.dtsi"
> +
> +/ {
> +       compatible = "renesas,r9a07g044l2";
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +
> +       cpus {
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +               cpu-map {
> +                       cluster0 {
> +                               core0 {
> +                                       cpu = <&a55_0>;
> +                               };
> +                               core1 {
> +                                       cpu = <&a55_1>;
> +                               };
> +                       };
> +               };
> +
> +               a55_0: cpu@0 {
> +                       compatible = "arm,cortex-a55";
> +                       reg = <0>;
> +                       device_type = "cpu";
> +                       next-level-cache = <&L3_CA55>;
> +                       enable-method = "psci";
> +               };
> +
> +               a55_1: cpu@1 {
> +                       compatible = "arm,cortex-a55";
> +                       reg = <0x100>;
> +                       device_type = "cpu";
> +                       next-level-cache = <&L3_CA55>;
> +                       enable-method = "psci";
> +               };
> +
> +               L3_CA55: cache-controller-0 {
> +                       compatible = "cache";
> +                       cache-unified;
> +                       cache-size = <0x40000>;
> +               };

I think (at least) the first CPU core should be in the base
r9a07g044.dtsi file.
Probably the second CPU core should be in the base file, too, and
removed by /delete-node/ in r9a07g044l1.dtsi?

> +       };
> +
> +       timer {
> +               compatible = "arm,armv8-timer";
> +               interrupts-extended =
> +                       <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
> +                       <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
> +                       <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,
> +                       <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>;
> +       };

Also in the base file, with interrupts-extended overridden where
needed?

> +};
> --
> 2.17.1
>


--
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
Geert Uytterhoeven May 21, 2021, 3:40 p.m. UTC | #11
Hi Prabhakar,

On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add basic support for RZ/G2L SMARC EVK (based on R9A07G044L2):
> - memory
> - External input clock
> - SCIF
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

> --- a/arch/arm64/boot/dts/renesas/Makefile
> +++ b/arch/arm64/boot/dts/renesas/Makefile
> @@ -62,3 +62,5 @@ dtb-$(CONFIG_ARCH_R8A77990) += r8a77990-ebisu.dtb
>  dtb-$(CONFIG_ARCH_R8A77995) += r8a77995-draak.dtb
>
>  dtb-$(CONFIG_ARCH_R8A779A0) += r8a779a0-falcon.dtb
> +
> +dtb-$(CONFIG_ARCH_R9A07G044L) += r9a07g044l2-smarc.dtb
> diff --git a/arch/arm64/boot/dts/renesas/g2l-smarc.dtsi b/arch/arm64/boot/dts/renesas/g2l-smarc.dtsi
> new file mode 100644
> index 000000000000..9b95d73fb798
> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/g2l-smarc.dtsi

rzg2l-smarc?

The rest looks good to me (taking into account compatible value
discussions).

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
Lad, Prabhakar May 21, 2021, 4:54 p.m. UTC | #12
Hi Geert,

Thank you for the review.

On Fri, May 21, 2021 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Prabhakar,

>

> On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar

> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> > Add device tree bindings documentation for Renesas RZ/G2UL SoC.

> >

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>

>

> Thanks for your patch!

>

> > --- a/Documentation/devicetree/bindings/arm/renesas.yaml

> > +++ b/Documentation/devicetree/bindings/arm/renesas.yaml

> > @@ -302,6 +302,12 @@ properties:

> >                - renesas,rzn1d400-db # RZN1D-DB (RZ/N1D Demo Board for the RZ/N1D 400 pins package)

> >            - const: renesas,r9a06g032

> >

> > +      - description: RZ/G2UL (R9A07G043)

> > +        items:

> > +          - enum:

> > +              - renesas,r9a07g043u11 # Single Cortex-A55 RZ/G2UL

>

> Is there any specific reason you're including the final "1", unlike the

> RZ/G2{L,LC} binding?

>

To be consistent with the RZ/G2L family of SoC's "1" is appended to
the compatible string.

> As RZ/G2UL is always single-core, perhaps this compatible value can be

> dropped?

>

Do agree with you.

> > +          - const: renesas,r9a07g043

> > +

> >  additionalProperties: true

>

> For now, there are no users of this binding?

> I assume you're posting it already, as RZ/G2UL is pin-compatible with RZ/G2LC,

> and thus can be used interchangeably on the G2L SOM?

> However, the DTS board part in this series is for RZ/G2L, not RZ/GLC?

>

Intention here is to start with RZ/G2L SoC first  so that the core
changes (pinctrl/CPG) hit upstream and for the rest of the SoC's it
will be followed up.

Cheers,
Prabhakar

> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> i.e. will queue in renesas-devel for v5.14, after the above have been

> resolved.

>

> 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
Lad, Prabhakar May 21, 2021, 5:21 p.m. UTC | #13
Hi Geert,

Thank you for the review.

On Fri, May 21, 2021 at 2:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Add ARCH_R9A07G044{L,LC} as a configuration symbol for the new Renesas
> > RZ/G2{L,LC} SoC's.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/soc/renesas/Kconfig
> > +++ b/drivers/soc/renesas/Kconfig
> > @@ -279,6 +279,16 @@ config ARCH_R8A774B1
> >         help
> >           This enables support for the Renesas RZ/G2N SoC.
> >
> > +config ARCH_R9A07G044L
> > +       bool "ARM64 Platform support for RZ/G2L SoC"
>
> Please drop the "SoC", for consistency with other entries.
>
Oops will do that.

> > +       help
> > +         This enables support for the Renesas RZ/G2L SoC.
> > +
> > +config ARCH_R9A07G044LC
> > +       bool "ARM64 Platform support for RZ/G2LC SoC"
>
> Likewise.
>
will do.

> > +       help
> > +         This enables support for the Renesas RZ/G2LC SoC.
> > +
> >  endif # ARM64
>
> Given LSI DEVID is the same, do we need both, or can we do with a
> single ARCH_R9A07G044?
>
The reason behind adding separate configs was in case if we wanted to
just build an image for RZ/G2L and not RZ/G2LC this would increase
image size and also build unneeded dtb's.

Cheers,
Prabhakar

> 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
Lad, Prabhakar May 21, 2021, 6:21 p.m. UTC | #14
Hi Geert,

Thank you for the review.

On Fri, May 21, 2021 at 4:41 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Prabhakar,

>

> On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar

> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> > Add basic support for RZ/G2L SMARC EVK (based on R9A07G044L2):

> > - memory

> > - External input clock

> > - SCIF

> >

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

>

> Thanks for your patch!

>

> > --- a/arch/arm64/boot/dts/renesas/Makefile

> > +++ b/arch/arm64/boot/dts/renesas/Makefile

> > @@ -62,3 +62,5 @@ dtb-$(CONFIG_ARCH_R8A77990) += r8a77990-ebisu.dtb

> >  dtb-$(CONFIG_ARCH_R8A77995) += r8a77995-draak.dtb

> >

> >  dtb-$(CONFIG_ARCH_R8A779A0) += r8a779a0-falcon.dtb

> > +

> > +dtb-$(CONFIG_ARCH_R9A07G044L) += r9a07g044l2-smarc.dtb

> > diff --git a/arch/arm64/boot/dts/renesas/g2l-smarc.dtsi b/arch/arm64/boot/dts/renesas/g2l-smarc.dtsi

> > new file mode 100644

> > index 000000000000..9b95d73fb798

> > --- /dev/null

> > +++ b/arch/arm64/boot/dts/renesas/g2l-smarc.dtsi

>

> rzg2l-smarc?

>

Agreed will do.

Cheers,
Prabhakar

> The rest looks good to me (taking into account compatible value

> discussions).

>

> 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
Lad, Prabhakar May 21, 2021, 6:36 p.m. UTC | #15
Hi Geert,

Thank you for the review.

On Fri, May 21, 2021 at 4:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>

> Hi Prabhakar,

>

> On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar

> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> > Add initial DTSI for RZ/G2{L,LC} SoC's.

> >

> > File structure:

> > r9a07g044.dtsi  => RZ/G2L family SoC common parts

> > r9a07g044l.dtsi => Specific to RZ/G2L (R9A07G044L) SoC

> > r9a07g044l1.dtsi => Specific to RZ/G2L (R9A07G044L single cortex A55) SoC

> > r9a07g044l2.dtsi => Specific to RZ/G2L (R9A07G044L dual cortex A55) SoC

> >

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

>

> Thanks for your patch!

>

> > index 000000000000..c625d302f889

> > --- /dev/null

> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi

> > @@ -0,0 +1,70 @@

> > +// SPDX-License-Identifier: GPL-2.0

>

> Do we want to use

>

>     SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

>

> for new DTS files?

> This actually also applies to <dt-bindings/...> files.

>

> > +/*

> > + * Device Tree Source for the RZ/G2L and RZ/G2LC common SoC parts

> > + *

> > + * Copyright (C) 2021 Renesas Electronics Corp.

> > + */

> > +

> > +#include <dt-bindings/interrupt-controller/arm-gic.h>

> > +#include <dt-bindings/clock/r9a07g044l-cpg.h>

> > +

> > +/ {

> > +       compatible = "renesas,r9a07g044";

> > +       #address-cells = <2>;

> > +       #size-cells = <2>;

> > +

> > +       extal_clk: extal {

> > +               compatible = "fixed-clock";

> > +               #clock-cells = <0>;

> > +               /* This value must be overridden by the board */

> > +               clock-frequency = <0>;

> > +       };

> > +

> > +       psci {

> > +               compatible = "arm,psci-1.0", "arm,psci-0.2";

> > +               method = "smc";

> > +       };

> > +

> > +       soc: soc {

> > +               compatible = "simple-bus";

> > +               interrupt-parent = <&gic>;

> > +               #address-cells = <2>;

> > +               #size-cells = <2>;

> > +               ranges;

> > +

> > +               scif0: serial@1004b800 {

> > +                       compatible = "renesas,scif-r9a07g044";

> > +                       reg = <0 0x1004b800 0 0x400>;

> > +                       interrupts =

> > +                               <GIC_SPI 380 IRQ_TYPE_LEVEL_HIGH>,

> > +                               <GIC_SPI 382 IRQ_TYPE_LEVEL_HIGH>,

> > +                               <GIC_SPI 383 IRQ_TYPE_LEVEL_HIGH>,

> > +                               <GIC_SPI 381 IRQ_TYPE_LEVEL_HIGH>,

> > +                               <GIC_SPI 384 IRQ_TYPE_LEVEL_HIGH>;

>

> "make dtbs_check

> DT_SCHEMA_FILES=Documentation/devicetree/bindings/serial/renesas,scif.yaml":

>

>     arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dt.yaml:

> serial@1004b800: interrupts: 'oneOf' conditional failed, one must be

> fixed:

>     [[0, 380, 4], [0, 382, 4], [0, 383, 4], [0, 381, 4], [0, 384, 4]]

> is too long

>     Additional items are not allowed ([0, 382, 4], [0, 383, 4], [0,

> 381, 4], [0, 384, 4] were unexpected)

>     Additional items are not allowed ([0, 384, 4] was unexpected)

>     [[0, 380, 4], [0, 382, 4], [0, 383, 4], [0, 381, 4], [0, 384, 4]]

> is too short

>

> One interrupt is missing.  According to the documentation, "tei" and

> "dri" share an interrupt, so they should map to the same interrupt number.

> Please add interrupt-names.

>

Agreed will do.

> > +                       clocks = <&cpg CPG_MOD R9A07G044_CLK_SCIF0>;

> > +                       clock-names = "fck";

> > +                       power-domains = <&cpg>;

> > +                       resets = <&cpg R9A07G044_CLK_SCIF0>;

> > +                       status = "disabled";

> > +               };

> > +

> > +               devid: chipid@11020a04 {

> > +                       compatible = "renesas,devid";

> > +                       reg = <0 0x11020a04 0 4>;

> > +               };

> > +

> > +               gic: interrupt-controller@11900000 {

> > +                       compatible = "arm,gic-v3";

> > +                       #interrupt-cells = <3>;

> > +                       #address-cells = <0>;

> > +                       interrupt-controller;

> > +                       reg = <0x0 0x11900000 0 0x40000>,

> > +                             <0x0 0x11940000 0 0x60000>;

> > +                       interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;

> > +                       clocks = <&cpg CPG_MOD R9A07G044_CLK_GIC600>;

> > +                       clock-names = "gic6000";

>

> This looks like a weird name ;-)

> In addition, it should be the consumer clock name, not the provider

> clock name.

>

Agreed will rename that.

> > +                       power-domains = <&cpg>;

> > +                       resets = <&cpg R9A07G044_CLK_GIC600>;

>

> "make dtbs_check

> DT_SCHEMA_FILES=Documentation/devicetree/bindings/interrupt-controller/arm,gic-v3.yaml":

>

>     arch/arm64/boot/dts/renesas/r9a07g044l2-smarc.dt.yaml:

> interrupt-controller@11900000: 'clock-names', 'clocks',

> 'power-domains', 'resets' do not match any of the regexes:

> '^(msi-controller|gic-its|interrupt-controller)@[0-9a-f]+$',

> '^gic-its@', '^interrupt-controller@[0-9a-f]+$', 'pinctrl-[0-9]+'

>

> These properties should be added to the GIC-v3 bindings, cfr. the normal

> GIC bindings.

>

Agreed will do that.

>

> > +               };

> > +       };

> > +};

> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044l.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044l.dtsi

> > new file mode 100644

> > index 000000000000..8d396b9100c1

> > --- /dev/null

> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044l.dtsi

> > @@ -0,0 +1,21 @@

> > +// SPDX-License-Identifier: GPL-2.0

> > +/*

> > + * Device Tree Source for the RZ/G2L common SoC parts

> > + *

> > + * Copyright (C) 2021 Renesas Electronics Corp.

> > + */

> > +

> > +/dts-v1/;

> > +#include "r9a07g044.dtsi"

> > +

> > +&soc {

> > +       cpg: clock-controller@11010000 {

> > +               compatible = "renesas,r9a07g044l-cpg";

> > +               reg = <0 0x11010000 0 0x10000>;

> > +               clocks = <&extal_clk>;

> > +               clock-names = "extal";

> > +               #clock-cells = <2>;

> > +               #reset-cells = <1>;

> > +               #power-domain-cells = <0>;

> > +       };

>

> As I think this is shared by RZ/G2L and RZ/G2LC, it belongs to

> r9a07g044.dtsi.

>

As some of the IP blocks present on RZ/G2L aren't present in RZ/G2LC
the clocks for the IP will be missing hence this is added to SoC
specific dtsi.

> > +};

> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi

> > new file mode 100644

> > index 000000000000..44d4504e44c3

> > --- /dev/null

> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi

> > @@ -0,0 +1,43 @@

> > +// SPDX-License-Identifier: GPL-2.0

> > +/*

> > + * Device Tree Source for the RZ/G2L R9A07G044L1 common parts

> > + *

> > + * Copyright (C) 2021 Renesas Electronics Corp.

> > + */

> > +

> > +/dts-v1/;

> > +#include "r9a07g044l.dtsi"

> > +

> > +/ {

> > +       compatible = "renesas,r9a07g044l1";

> > +       #address-cells = <2>;

> > +       #size-cells = <2>;

>

> #{address,size}-cells already defined in r9a07g044.dtsi.

>

Will drop that.

> > +

> > +       cpus {

> > +               #address-cells = <1>;

> > +               #size-cells = <0>;

> > +

> > +               a55_0: cpu@0 {

> > +                       compatible = "arm,cortex-a55";

> > +                       reg = <0>;

> > +                       device_type = "cpu";

> > +                       next-level-cache = <&L3_CA55>;

> > +                       enable-method = "psci";

> > +               };

> > +

> > +               L3_CA55: cache-controller-0 {

> > +                       compatible = "cache";

> > +                       cache-unified;

> > +                       cache-size = <0x40000>;

> > +               };

>

> I think the first CPU core should be in the base r9a07g044.dtsi file.

>

OK will move that.

> > +       };

> > +

> > +       timer {

> > +               compatible = "arm,armv8-timer";

> > +               interrupts-extended =

> > +                       <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>,

> > +                       <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>,

> > +                       <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>,

> > +                       <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(1) | IRQ_TYPE_LEVEL_LOW)>;

>

> Also in the base file, with interrupts-extended overridden where

> needed?

>

OK will  override here with the timer node moved from r9a07g044l2.dtsi
to common SoC file.

> > +       };

> > +};

> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi

> > new file mode 100644

> > index 000000000000..33bb35e1c369

> > --- /dev/null

> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044l2.dtsi

> > @@ -0,0 +1,62 @@

> > +// SPDX-License-Identifier: GPL-2.0

> > +/*

> > + * Device Tree Source for the RZ/G2L R9A07G044L2 common parts

> > + *

> > + * Copyright (C) 2021 Renesas Electronics Corp.

> > + */

> > +

> > +/dts-v1/;

> > +#include "r9a07g044l.dtsi"

> > +

> > +/ {

> > +       compatible = "renesas,r9a07g044l2";

> > +       #address-cells = <2>;

> > +       #size-cells = <2>;

> > +

> > +       cpus {

> > +               #address-cells = <1>;

> > +               #size-cells = <0>;

> > +

> > +               cpu-map {

> > +                       cluster0 {

> > +                               core0 {

> > +                                       cpu = <&a55_0>;

> > +                               };

> > +                               core1 {

> > +                                       cpu = <&a55_1>;

> > +                               };

> > +                       };

> > +               };

> > +

> > +               a55_0: cpu@0 {

> > +                       compatible = "arm,cortex-a55";

> > +                       reg = <0>;

> > +                       device_type = "cpu";

> > +                       next-level-cache = <&L3_CA55>;

> > +                       enable-method = "psci";

> > +               };

> > +

> > +               a55_1: cpu@1 {

> > +                       compatible = "arm,cortex-a55";

> > +                       reg = <0x100>;

> > +                       device_type = "cpu";

> > +                       next-level-cache = <&L3_CA55>;

> > +                       enable-method = "psci";

> > +               };

> > +

> > +               L3_CA55: cache-controller-0 {

> > +                       compatible = "cache";

> > +                       cache-unified;

> > +                       cache-size = <0x40000>;

> > +               };

>

> I think (at least) the first CPU core should be in the base

> r9a07g044.dtsi file.

> Probably the second CPU core should be in the base file, too, and

> removed by /delete-node/ in r9a07g044l1.dtsi?

>

Agreed will do that.

> > +       };

> > +

> > +       timer {

> > +               compatible = "arm,armv8-timer";

> > +               interrupts-extended =

> > +                       <&gic GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,

> > +                       <&gic GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,

> > +                       <&gic GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>,

> > +                       <&gic GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(2) | IRQ_TYPE_LEVEL_LOW)>;

> > +       };

>

> Also in the base file, with interrupts-extended overridden where

> needed?

>

Will move this node to common SoC r9a07g044.dtsi file and override in
r9a07g044l1.dtsi with CPU maks to 1.

Cheers,
Prabhakar

> > +};

> > --

> > 2.17.1

> >

>

>

> --

> 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
Lad, Prabhakar May 21, 2021, 6:42 p.m. UTC | #16
Hi Geert,

Thank you for the review.

On Fri, May 21, 2021 at 4:04 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Document the device tree bindings of the Renesas RZ/G2L SoC clock
> > driver in Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
> > @@ -0,0 +1,80 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/clock/renesas,rzg2l-cpg.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: Renesas RZ/G2L Clock Pulse Generator / Module Stop and Software Reset
>
> (Module Standby Mode
> > +
> > +maintainers:
> > +  - Geert Uytterhoeven <geert+renesas@glider.be>
> > +
> > +description: |
> > +  On Renesas RZ/G2L SoC, the CPG (Clock Pulse Generator) and MSTP
> > +  (Module Stop and Software Reset) share the same register block.
> > +
> > +  They provide the following functionalities:
> > +    - The CPG block generates various core clocks,
> > +    - The MSTP block provides two functions:
> > +        1. Module Stop, providing a Clock Domain to control the clock supply
> > +           to individual SoC devices,
> > +        2. Reset Control, to perform a software reset of individual SoC devices.
> > +
> > +properties:
> > +  compatible:
> > +    const: renesas,r9a07g044l-cpg  # RZ/G2L
>
> renesas,r9a07g044-cpg?
>
As some IP blocks present in RZ/G2L aren't present in RZ/G2LC clock
handling will differ so as a result SoC specific compatible string is
added.

> I believe it's the same block on RZ/G2L ('044l) and RZ/G2LC ('044c).
>
> > +  '#clock-cells':
> > +    description: |
> > +      - For CPG core clocks, the two clock specifier cells must be "CPG_CORE"
> > +        and a core clock reference, as defined in
> > +        <dt-bindings/clock/*-cpg-mssr.h>
>
> <dt-bindings/clock/r9a07g044l-cpg.h>
>
Indeed

> > +      - For module clocks, the two clock specifier cells must be "CPG_MOD" and
> > +        a module number, as defined in the datasheet.
>
> Also in <dt-bindings/clock/r9a07g044l-cpg.h>?
>
Agreed.

> > +    const: 2
> > +
> > +  '#power-domain-cells':
> > +    description:
> > +      SoC devices that are part of the CPG/MSTP Clock Domain and can be
> > +      power-managed through Module Stop should refer to the CPG device node
> > +      in their "power-domains" property, as documented by the generic PM Domain
> > +      bindings in Documentation/devicetree/bindings/power/power-domain.yaml.
> > +    const: 0
> > +
> > +  '#reset-cells':
> > +    description:
> > +      The single reset specifier cell must be the module number, as defined in
> > +      the datasheet.
>
> Also in <dt-bindings/clock/r9a07g044l-cpg.h>?
>
Agreed.

Cheers,
Prabhakar

> > +    const: 1
>
> 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
Geert Uytterhoeven May 27, 2021, 11:17 a.m. UTC | #17
Hi Prabhakar,

On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add initial DTSI for RZ/G2{L,LC} SoC's.
>
> File structure:
> r9a07g044.dtsi  => RZ/G2L family SoC common parts
> r9a07g044l.dtsi => Specific to RZ/G2L (R9A07G044L) SoC
> r9a07g044l1.dtsi => Specific to RZ/G2L (R9A07G044L single cortex A55) SoC
> r9a07g044l2.dtsi => Specific to RZ/G2L (R9A07G044L dual cortex A55) SoC
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> @@ -0,0 +1,70 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the RZ/G2L and RZ/G2LC common SoC parts
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/clock/r9a07g044l-cpg.h>
> +
> +/ {
> +       compatible = "renesas,r9a07g044";

> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device Tree Source for the RZ/G2L R9A07G044L1 common parts
> + *
> + * Copyright (C) 2021 Renesas Electronics Corp.
> + */
> +
> +/dts-v1/;
> +#include "r9a07g044l.dtsi"
> +
> +/ {
> +       compatible = "renesas,r9a07g044l1";

This overwrites the main compatible value set by r9a07g044.dtsi before.
As per your bindings, you want both:

    compatible = "renesas,r9a07g044l1", "renesas,r9a07g044".

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
Geert Uytterhoeven May 27, 2021, 11:29 a.m. UTC | #18
Hi Prabhakar,

On Fri, May 21, 2021 at 6:54 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, May 21, 2021 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar

> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> > > Add device tree bindings documentation for Renesas RZ/G2UL SoC.

> > >

> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>

> >

> > Thanks for your patch!

> >

> > > --- a/Documentation/devicetree/bindings/arm/renesas.yaml

> > > +++ b/Documentation/devicetree/bindings/arm/renesas.yaml

> > > @@ -302,6 +302,12 @@ properties:

> > >                - renesas,rzn1d400-db # RZN1D-DB (RZ/N1D Demo Board for the RZ/N1D 400 pins package)

> > >            - const: renesas,r9a06g032

> > >

> > > +      - description: RZ/G2UL (R9A07G043)

> > > +        items:

> > > +          - enum:

> > > +              - renesas,r9a07g043u11 # Single Cortex-A55 RZ/G2UL

> >

> > Is there any specific reason you're including the final "1", unlike the

> > RZ/G2{L,LC} binding?

> >

> To be consistent with the RZ/G2L family of SoC's "1" is appended to

> the compatible string.


No, for RZ/G2L you have:

    renesas,r9a07g044c1 for r9a07g044c12
    renesas,r9a07g044c2 for r9a07g044c22
    renesas,r9a07g044l1 for r9a07g044l13 and r9a07g044l14
    renesas,r9a07g044l2 for r9a07g044l23 and r9a07g044l24

i.e. the compatible value lacks the final digit.

For RZ/G2UL, I do not know if we have to distinguish between
r9a07g043u11 and r9a07g043u12.

> > As RZ/G2UL is always single-core, perhaps this compatible value can be

> > dropped?

> >

> Do agree with you.


In light of the continued discussion for [PATCH 02/16], perhaps it's
good to keep it anyway?

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
Lad, Prabhakar May 27, 2021, 11:47 a.m. UTC | #19
Hi Geert,

On Thu, May 27, 2021 at 12:29 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>

> Hi Prabhakar,

>

> On Fri, May 21, 2021 at 6:54 PM Lad, Prabhakar

> <prabhakar.csengg@gmail.com> wrote:

> > On Fri, May 21, 2021 at 2:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > > On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar

> > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> > > > Add device tree bindings documentation for Renesas RZ/G2UL SoC.

> > > >

> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> > > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> > > > Reviewed-by: Chris Paterson <Chris.Paterson2@renesas.com>

> > >

> > > Thanks for your patch!

> > >

> > > > --- a/Documentation/devicetree/bindings/arm/renesas.yaml

> > > > +++ b/Documentation/devicetree/bindings/arm/renesas.yaml

> > > > @@ -302,6 +302,12 @@ properties:

> > > >                - renesas,rzn1d400-db # RZN1D-DB (RZ/N1D Demo Board for the RZ/N1D 400 pins package)

> > > >            - const: renesas,r9a06g032

> > > >

> > > > +      - description: RZ/G2UL (R9A07G043)

> > > > +        items:

> > > > +          - enum:

> > > > +              - renesas,r9a07g043u11 # Single Cortex-A55 RZ/G2UL

> > >

> > > Is there any specific reason you're including the final "1", unlike the

> > > RZ/G2{L,LC} binding?

> > >

> > To be consistent with the RZ/G2L family of SoC's "1" is appended to

> > the compatible string.

>

My bad, the reason for adding 1 in the end was there are two variants
of RZ/G2UL [1].  For the next respin I'll include renesas,r9a07g043u12
too.

> No, for RZ/G2L you have:

>

>     renesas,r9a07g044c1 for r9a07g044c12

>     renesas,r9a07g044c2 for r9a07g044c22

>     renesas,r9a07g044l1 for r9a07g044l13 and r9a07g044l14

>     renesas,r9a07g044l2 for r9a07g044l23 and r9a07g044l24

>

> i.e. the compatible value lacks the final digit.

>

> For RZ/G2UL, I do not know if we have to distinguish between

> r9a07g043u11 and r9a07g043u12.

>

Some IP blocks are missing in type2 compared to type1. And at the
higher level we might want to know the exact SoC type the board is
built ?

> > > As RZ/G2UL is always single-core, perhaps this compatible value can be

> > > dropped?

> > >

> > Do agree with you.

>

> In light of the continued discussion for [PATCH 02/16], perhaps it's

> good to keep it anyway?

>

Yes will keep the compatible string.

[1] https://www.renesas.com/us/en/products/microcontrollers-microprocessors/rz-arm-based-high-end-32-64-bit-mpus/rzg2ul-general-purpose-microprocessors-single-core-arm-cortex-a55-10-ghz-cpu-2ch-giga-bit-ethernet

Cheers,
Prabhakar
Geert Uytterhoeven May 27, 2021, 11:47 a.m. UTC | #20
Hi Prabhakar,

On Fri, May 21, 2021 at 7:21 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, May 21, 2021 at 2:25 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > Add ARCH_R9A07G044{L,LC} as a configuration symbol for the new Renesas
> > > RZ/G2{L,LC} SoC's.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/soc/renesas/Kconfig
> > > +++ b/drivers/soc/renesas/Kconfig
> > > @@ -279,6 +279,16 @@ config ARCH_R8A774B1
> > >         help
> > >           This enables support for the Renesas RZ/G2N SoC.
> > >
> > > +config ARCH_R9A07G044L
> > > +       bool "ARM64 Platform support for RZ/G2L SoC"
> >
> > Please drop the "SoC", for consistency with other entries.
> >
> Oops will do that.
>
> > > +       help
> > > +         This enables support for the Renesas RZ/G2L SoC.
> > > +
> > > +config ARCH_R9A07G044LC
> > > +       bool "ARM64 Platform support for RZ/G2LC SoC"
> >
> > Likewise.
> >
> will do.
>
> > > +       help
> > > +         This enables support for the Renesas RZ/G2LC SoC.
> > > +
> > >  endif # ARM64
> >
> > Given LSI DEVID is the same, do we need both, or can we do with a
> > single ARCH_R9A07G044?
> >
> The reason behind adding separate configs was in case if we wanted to
> just build an image for RZ/G2L and not RZ/G2LC this would increase
> image size and also build unneeded dtb's.

How would it increase image size? I understand clock and pin control
are the same blocks.

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven May 27, 2021, 11:51 a.m. UTC | #21
Hi Prabhakar,

On Fri, May 21, 2021 at 8:43 PM Lad, Prabhakar
<prabhakar.csengg@gmail.com> wrote:
> On Fri, May 21, 2021 at 4:04 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, May 14, 2021 at 9:23 PM Lad Prabhakar
> > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > > Document the device tree bindings of the Renesas RZ/G2L SoC clock
> > > driver in Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/renesas,rzg2l-cpg.yaml
> > > @@ -0,0 +1,80 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/clock/renesas,rzg2l-cpg.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: Renesas RZ/G2L Clock Pulse Generator / Module Stop and Software Reset
> >
> > (Module Standby Mode
> > > +
> > > +maintainers:
> > > +  - Geert Uytterhoeven <geert+renesas@glider.be>
> > > +
> > > +description: |
> > > +  On Renesas RZ/G2L SoC, the CPG (Clock Pulse Generator) and MSTP
> > > +  (Module Stop and Software Reset) share the same register block.
> > > +
> > > +  They provide the following functionalities:
> > > +    - The CPG block generates various core clocks,
> > > +    - The MSTP block provides two functions:
> > > +        1. Module Stop, providing a Clock Domain to control the clock supply
> > > +           to individual SoC devices,
> > > +        2. Reset Control, to perform a software reset of individual SoC devices.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: renesas,r9a07g044l-cpg  # RZ/G2L
> >
> > renesas,r9a07g044-cpg?
> >
> As some IP blocks present in RZ/G2L aren't present in RZ/G2LC clock
> handling will differ so as a result SoC specific compatible string is
> added.

The RZ/G2L Hardware User's Manual Rev. 0.41 doesn't mention any
differences between the CPG on RZ/G2L and RZ/G2LC.  So I think it's
safe to have a single driver for both members.

Gr{oetje,eeting}s,

                        Geert
Lad, Prabhakar May 27, 2021, 11:51 a.m. UTC | #22
Hi Geert,

On Thu, May 27, 2021 at 12:17 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Add initial DTSI for RZ/G2{L,LC} SoC's.
> >
> > File structure:
> > r9a07g044.dtsi  => RZ/G2L family SoC common parts
> > r9a07g044l.dtsi => Specific to RZ/G2L (R9A07G044L) SoC
> > r9a07g044l1.dtsi => Specific to RZ/G2L (R9A07G044L single cortex A55) SoC
> > r9a07g044l2.dtsi => Specific to RZ/G2L (R9A07G044L dual cortex A55) SoC
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
> > @@ -0,0 +1,70 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree Source for the RZ/G2L and RZ/G2LC common SoC parts
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corp.
> > + */
> > +
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +#include <dt-bindings/clock/r9a07g044l-cpg.h>
> > +
> > +/ {
> > +       compatible = "renesas,r9a07g044";
>
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044l1.dtsi
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device Tree Source for the RZ/G2L R9A07G044L1 common parts
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corp.
> > + */
> > +
> > +/dts-v1/;
> > +#include "r9a07g044l.dtsi"
> > +
> > +/ {
> > +       compatible = "renesas,r9a07g044l1";
>
> This overwrites the main compatible value set by r9a07g044.dtsi before.
> As per your bindings, you want both:
>
>     compatible = "renesas,r9a07g044l1", "renesas,r9a07g044".
>
Agreed will fix that in next respin.

Cheers,
Prabhakar
Geert Uytterhoeven May 27, 2021, 12:04 p.m. UTC | #23
Hi Prabhakar,

On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> Add CPG core wrapper for RZ/G2L family.
>
> Based on a patch in the BSP by Binh Nguyen
> <binh.nguyen.jz@renesas.com>.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

> --- /dev/null
> +++ b/drivers/clk/renesas/renesas-rzg2l-cpg.c

> +static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
> +{
> +       struct mstp_clock *clock = to_mod_clock(hw);
> +       struct cpg_mssr_priv *priv = clock->priv;
> +       unsigned int reg = MSSR_OFF(clock->bit) * 4;

The "* 4" here makes it difficult to review the module clock tables.

E.g.

       DEF_MOD("gic",          R9A07G044_CLK_GIC600,
                               R9A07G044_CLK_P1,
                               MSSR(5, BIT(0), (BIT(0) | BIT(1)))),

The "5" means the CLK_ON_GIC600 register is at offset CLK_ON_R(5 * 4)
 = 0x514.  Removing the "* 4" means you could use
"MSSR(0x14, BIT(0), (BIT(0) | BIT(1))" instead.

Unless it has unpleasant side effects, I'd even consider putting
the full CLK_ON offset there, i.e.
"MSSR(0x514, BIT(0), (BIT(0) | BIT(1))" and change the macros like:

    #define CLK_ON_R(reg)          (reg)
    #define CLK_MON_R(reg)         (0x680 - 0x500 + (reg))

> --- /dev/null
> +++ b/drivers/clk/renesas/renesas-rzg2l-cpg.h

> +#define CLK_ON_R(reg)          (0x500 + reg)
> +#define CLK_MON_R(reg)         (0x680 + reg)
> +#define CLK_RST_R(reg)         (0x800 + reg)
> +#define CLK_MRST_R(reg)                (0x980 + reg)

The last three don't seem to be documented?


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
Lad, Prabhakar May 28, 2021, 7:51 a.m. UTC | #24
Hi Geert,

On Thu, May 27, 2021 at 1:04 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Fri, May 14, 2021 at 9:24 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Add CPG core wrapper for RZ/G2L family.
> >
> > Based on a patch in the BSP by Binh Nguyen
> > <binh.nguyen.jz@renesas.com>.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
>
> > --- /dev/null
> > +++ b/drivers/clk/renesas/renesas-rzg2l-cpg.c
>
> > +static int rzg2l_mod_clock_endisable(struct clk_hw *hw, bool enable)
> > +{
> > +       struct mstp_clock *clock = to_mod_clock(hw);
> > +       struct cpg_mssr_priv *priv = clock->priv;
> > +       unsigned int reg = MSSR_OFF(clock->bit) * 4;
>
> The "* 4" here makes it difficult to review the module clock tables.
>
> E.g.
>
>        DEF_MOD("gic",          R9A07G044_CLK_GIC600,
>                                R9A07G044_CLK_P1,
>                                MSSR(5, BIT(0), (BIT(0) | BIT(1)))),
>
> The "5" means the CLK_ON_GIC600 register is at offset CLK_ON_R(5 * 4)
>  = 0x514.  Removing the "* 4" means you could use
> "MSSR(0x14, BIT(0), (BIT(0) | BIT(1))" instead.
>
> Unless it has unpleasant side effects, I'd even consider putting
> the full CLK_ON offset there, i.e.
> "MSSR(0x514, BIT(0), (BIT(0) | BIT(1))" and change the macros like:
>
>     #define CLK_ON_R(reg)          (reg)
>     #define CLK_MON_R(reg)         (0x680 - 0x500 + (reg))
>
OK will do that.

> > --- /dev/null
> > +++ b/drivers/clk/renesas/renesas-rzg2l-cpg.h
>
> > +#define CLK_ON_R(reg)          (0x500 + reg)
> > +#define CLK_MON_R(reg)         (0x680 + reg)
> > +#define CLK_RST_R(reg)         (0x800 + reg)
> > +#define CLK_MRST_R(reg)                (0x980 + reg)
>
> The last three don't seem to be documented?
>
I have asked Chris to send the document across.

Cheers,
Prabhakar