mbox series

[v3,00/20] Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board

Message ID 20231011184823.443959-1-peter.griffin@linaro.org
Headers show
Series Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand

Message

Peter Griffin Oct. 11, 2023, 6:48 p.m. UTC
Hi folks,

Firstly, thanks to everyone who reviewed the v2/V1 series! V3 incorporates
all the review feedback received so far.

As this series spans multiple subsytems the expectation is that Krzysztof
will apply the whole series through the Samsung SoC tree. If the relevant
subsystem maintainers can give a acked-by or reviewed-by on the relevant
patches that would be most appreciated!

This series adds initial SoC support for the GS101 SoC and also initial board
support for Pixel 6 phone (Oriole).

The gs101 / Tensor SoC is also used in Pixel6a (bluejay) and Pixel 6 Pro
(raven) phones. Currently DT is added for the gs101 SoC and Oriole.
As you can see from the patches the SoC is based on a Samsung Exynos SoC,
and therefore lots of the low level Exynos drivers and bindings can be
re-used.

The support added in this series consists of:
* cpus
* pinctrl
* some CCF implementation
* watchdog
* uart
* gpio

This is enough to boot through to a busybox initramfs and shell using an
upstream kernel though :) More platform support will be added over the
following weeks and months.

For further information on how to build and flash the upstream kernel on your
Pixel 6, with a prebuilt busybox initramfs please refer to the script and
README.md here:

https://git.codelinaro.org/linaro/googlelt/pixelscripts

Note 1: I've removed the dtbo overlay from v2 and later submissions and
will re-submit once I have appropriate documentation for it.

Note 2: I've left the bootargs in dts with earlycon for now, for two reasons.
1) The bootloader hangs if bootargs isn't present in the dtb as it tries to
re-write this with additional bootargs.
2) there is a issue whereby the full serial console doesn't come up properly
if earlycon isn't also specified. This issue needs further investigation.

kind regards,

Peter.

Changes since v2:
 - Fixup pinctrl@174d0000: interrupts: [..] is too long DTC warning (Tudor)
 - Add missing windowed watchdog code (Guenter)
 - Fixup UART YAML bindings error (Krzysztof)
 - gs101.dtsi add missing serial_0 alias (me)
 - samsung_tty.c: fixup gs101_serial_drv_data so fifosize os obtained from DT
 
Changes since v1:
 - Remove irq/gs101.h and replace macros with irq numbers globally
 - exynos-pmu - keep alphabetical order
 - add cmu_apm to clock bindings documentation
 - sysreg bindings - remove superfluous `google,gs101-sysreg`
 - watchdog bindings - Alphanumerical order, update gs201 comment
 - samsung,pinctrl.yaml - add new "if:then:else:" to narrow for google SoC
 - samsung,pinctrl-wakeup-interrupt.yaml - Alphanumerical order
 - samsung,pinctrl- add google,gs101-wakeup-eint compatible
 - clk-pll: fixup typos
 - clk-gs101: fix kernel test robot warnings (add 2 new clocks,dividers,gate)
 - clk-gs101: fix alphabetical order
 - clk-gs101: cmu_apm: fixup typo and missing empty entry
 - clk-gs101: cmu_misc: remove clocks that were being registerred twice
 - pinctrl: filter sel: rename/reorder variables, add comment for FLTCON bitfield
 - pinctrl: filter sel: avoid setting reserved bits by loop over FLTCON1 pins as well
 - pinctrl: gs101: rename bank_type_6/7 structs to be more specific, split from filter
 - watchdog: s3c2410_wdt: remove dev_info prints
 - gs101.dtsi/oriole.dts: order by unit node, remove underscores from node name, blank lines
   add SoC node, split dts and dtsi into separate patches, remove 'DVT' suffix
 - gs101-oriole.dtso: Remove overlay until board_id is documented properly
 - Add GS101_PIN_* macros to gs101-pinctrl.h instead of using Exynos ones
 - gpio-keys: update linux,code to use input-event-code macros
 - add dedicated gs101-uart compatible

Peter Griffin (20):
  dt-bindings: soc: samsung: exynos-pmu: Add gs101 compatible
  dt-bindings: clock: Add Google gs101 clock management unit bindings
  dt-bindings: soc: google: exynos-sysreg: add dedicated SYSREG
    compatibles to GS101
  dt-bindings: watchdog: Document Google gs101 & gs201 watchdog bindings
  dt-bindings: arm: google: Add bindings for Google ARM platforms
  dt-bindings: pinctrl: samsung: add google,gs101-pinctrl compatible
  dt-bindings: pinctrl: samsung: add gs101-wakeup-eint compatible
  dt-bindings: serial: samsung: Add google-gs101-uart compatible
  clk: samsung: clk-pll: Add support for pll_{0516,0517,518}
  clk: samsung: clk-gs101: Add cmu_top registers, plls, mux and gates
  clk: samsung: clk-gs101: add CMU_APM support
  clk: samsung: clk-gs101: Add support for CMU_MISC clock unit
  pinctrl: samsung: Add filter selection support for alive banks
  pinctrl: samsung: Add gs101 SoC pinctrl configuration
  watchdog: s3c2410_wdt: Add support for Google tensor SoCs
  tty: serial: samsung: Add gs101 compatible and SoC data
  arm64: dts: google: Add initial Google gs101 SoC support
  arm64: dts: google: Add initial Oriole/pixel 6 board support
  arm64: defconfig: Enable Google Tensor SoC
  MAINTAINERS: add entry for Google Tensor SoC

 .../devicetree/bindings/arm/google.yaml       |   46 +
 .../bindings/clock/google,gs101-clock.yaml    |  125 +
 .../samsung,pinctrl-wakeup-interrupt.yaml     |    2 +
 .../bindings/pinctrl/samsung,pinctrl.yaml     |   22 +-
 .../bindings/serial/samsung_uart.yaml         |    1 +
 .../bindings/soc/samsung/exynos-pmu.yaml      |    2 +
 .../soc/samsung/samsung,exynos-sysreg.yaml    |    6 +
 .../bindings/watchdog/samsung-wdt.yaml        |   10 +-
 MAINTAINERS                                   |   10 +
 arch/arm64/Kconfig.platforms                  |    6 +
 arch/arm64/boot/dts/Makefile                  |    1 +
 arch/arm64/boot/dts/google/Makefile           |    4 +
 arch/arm64/boot/dts/google/gs101-oriole.dts   |   79 +
 arch/arm64/boot/dts/google/gs101-pinctrl.dtsi | 1275 ++++++++++
 arch/arm64/boot/dts/google/gs101-pinctrl.h    |   32 +
 arch/arm64/boot/dts/google/gs101.dtsi         |  504 ++++
 arch/arm64/configs/defconfig                  |    1 +
 drivers/clk/samsung/Kconfig                   |    9 +
 drivers/clk/samsung/Makefile                  |    2 +
 drivers/clk/samsung/clk-gs101.c               | 2164 +++++++++++++++++
 drivers/clk/samsung/clk-pll.c                 |    9 +-
 drivers/clk/samsung/clk-pll.h                 |    3 +
 .../pinctrl/samsung/pinctrl-exynos-arm64.c    |  163 ++
 drivers/pinctrl/samsung/pinctrl-exynos.c      |   84 +-
 drivers/pinctrl/samsung/pinctrl-exynos.h      |   41 +
 drivers/pinctrl/samsung/pinctrl-samsung.c     |    4 +
 drivers/pinctrl/samsung/pinctrl-samsung.h     |   24 +
 drivers/tty/serial/samsung_tty.c              |   13 +
 drivers/watchdog/s3c2410_wdt.c                |  127 +-
 include/dt-bindings/clock/google,gs101.h      |  232 ++
 30 files changed, 4985 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/google.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/google,gs101-clock.yaml
 create mode 100644 arch/arm64/boot/dts/google/Makefile
 create mode 100644 arch/arm64/boot/dts/google/gs101-oriole.dts
 create mode 100644 arch/arm64/boot/dts/google/gs101-pinctrl.dtsi
 create mode 100644 arch/arm64/boot/dts/google/gs101-pinctrl.h
 create mode 100644 arch/arm64/boot/dts/google/gs101.dtsi
 create mode 100644 drivers/clk/samsung/clk-gs101.c
 create mode 100644 include/dt-bindings/clock/google,gs101.h

Comments

Rob Herring Oct. 16, 2023, 1:41 p.m. UTC | #1
On Wed, Oct 11, 2023 at 07:48:09PM +0100, Peter Griffin wrote:
> Add the "google,gs101-pinctrl" compatible to the dt-schema bindings
> documentation.
> 
> Add maxItems of 50 for the interrupts property as gs101 can have
> multiple irqs.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  .../bindings/pinctrl/samsung,pinctrl.yaml     | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml
> index 26614621774a..6dc648490668 100644
> --- a/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml
> @@ -35,6 +35,7 @@ properties:
>  
>    compatible:
>      enum:
> +      - google,gs101-pinctrl
>        - samsung,s3c2412-pinctrl
>        - samsung,s3c2416-pinctrl
>        - samsung,s3c2440-pinctrl
> @@ -58,7 +59,8 @@ properties:
>    interrupts:
>      description:
>        Required for GPIO banks supporting external GPIO interrupts.
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 50
>  
>    power-domains:
>      maxItems: 1
> @@ -134,6 +136,24 @@ allOf:
>            minItems: 1
>            maxItems: 1
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: google,gs101-pinctrl
> +    then:
> +      properties:
> +        interrupts:
> +          description:
> +            Required for external wakeup interrupts. List all external

Is it external GPIO interrupts or wakeup interrupts?

> +            wakeup interrupts supported by this bank.
> +          minItems: 1
> +          maxItems: 50

For a given SoC, I don't see how this is variable? If it is variable, 
how do you know which entry is what?


> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 1
> +
>  additionalProperties: false
>  
>  examples:
> -- 
> 2.42.0.655.g421f12c284-goog
>
Peter Griffin Oct. 17, 2023, 9:39 p.m. UTC | #2
Hi Sam,

Thanks for your review.

On Thu, 12 Oct 2023 at 03:32, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Wed, Oct 11, 2023 at 1:49 PM Peter Griffin <peter.griffin@linaro.org> wrote:
> >
> > This patch adds the compatibles and drvdata for the Google
> > gs101 & gs201 SoCs found in Pixel 6 and Pixel 7 phones. Similar
> > to Exynos850 it has two watchdog instances, one for each cluster
> > and has some control bits in PMU registers.
> >
> > The watchdog IP found in gs101 SoCs also supports a few
> > additional bits/features in the WTCON register which we add
> > support for and an additional register detailed below.
> >
> > dbgack-mask - Enables masking WDT interrupt and reset request
> > according to asserted DBGACK input
> >
> > windowed-mode - Enabled Windowed watchdog mode
> >
> > Windowed watchdog mode also has an additional register WTMINCNT.
> > If windowed watchdog is enabled and you reload WTCNT when the
> > value is greater than WTMINCNT, it prompts interrupt or reset
> > request as if the watchdog time has expired.
> >
>
> A couple of thoughts in addition to what Guenter said.
>
> From the description it looks like this patch should be split into 3 patches:
>   1. Add "dbgack" feature
>   2. Add "windowed mode" feature
>   3. Enable gsX01 support

Sure I can split it up like that in v4 if that is preferable. The most important
part atm is SoC support, as the watchdog is left enabled by the bootloader
so without this, the system resets after ~ 1 minute.

> Also, it's not clear if those features are mandatory for gsX01 wdt to
> function properly, or optional?

The features aren't mandatory, the watchdog works fine with windowed
mode disabled and the "dback" feature just affects the watchdog reset
behaviour when an external debug agent is used.

> From the code it looks like both
> dbgack and windowed mode will only affect gsX01 variants (because of
> quirk flags), but maybe the commit message should be more clear about
> that.

Sure I will be more verbose in the commit messages when I split it out into
separate patches

>
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  drivers/watchdog/s3c2410_wdt.c | 127 ++++++++++++++++++++++++++++++---
> >  1 file changed, 116 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 0b4bd883ff28..36c170047180 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -31,12 +31,14 @@
> >  #define S3C2410_WTDAT          0x04
> >  #define S3C2410_WTCNT          0x08
> >  #define S3C2410_WTCLRINT       0x0c
> > -
> > +#define S3C2410_WTMINCNT       0x10
> >  #define S3C2410_WTCNT_MAXCNT   0xffff
> >
> > -#define S3C2410_WTCON_RSTEN    (1 << 0)
> > -#define S3C2410_WTCON_INTEN    (1 << 2)
> > -#define S3C2410_WTCON_ENABLE   (1 << 5)
> > +#define S3C2410_WTCON_RSTEN            (1 << 0)
> > +#define S3C2410_WTCON_INTEN            (1 << 2)
> > +#define S3C2410_WTCON_ENABLE           (1 << 5)
> > +#define S3C2410_WTCON_DBGACK_MASK      (1 << 16)
> > +#define S3C2410_WTCON_WINDOWED_WD      (1 << 20)
>
> Maybe use BIT() macro here?

I didn't use the BIT macro for my changes, as the rest of the driver
isn't currently using the BIT macro.

>
> >
> >  #define S3C2410_WTCON_DIV16    (0 << 3)
> >  #define S3C2410_WTCON_DIV32    (1 << 3)
> > @@ -51,6 +53,7 @@
> >
> >  #define S3C2410_WATCHDOG_ATBOOT                (0)
> >  #define S3C2410_WATCHDOG_DEFAULT_TIME  (15)
> > +#define S3C2410_WINDOW_MULTIPLIER      2
> >
> >  #define EXYNOS5_RST_STAT_REG_OFFSET            0x0404
> >  #define EXYNOS5_WDT_DISABLE_REG_OFFSET         0x0408
> > @@ -67,6 +70,13 @@
> >  #define EXYNOSAUTOV9_CLUSTER0_WDTRESET_BIT     25
> >  #define EXYNOSAUTOV9_CLUSTER1_WDTRESET_BIT     24
> >
> > +#define GS_CLUSTER0_NONCPU_OUT                 0x1220
> > +#define GS_CLUSTER1_NONCPU_OUT                 0x1420
> > +#define GS_CLUSTER0_NONCPU_INT_EN              0x1244
> > +#define GS_CLUSTER1_NONCPU_INT_EN              0x1444
> > +#define GS_CLUSTER2_NONCPU_INT_EN              0x1644
> > +#define GS_RST_STAT_REG_OFFSET                 0x3B44
>
> Please move those to the section above, where similar registers are
> described for other SoCs.

Will fix.

>
> > +
> >  /**
> >   * DOC: Quirk flags for different Samsung watchdog IP-cores
> >   *
> > @@ -106,6 +116,8 @@
> >  #define QUIRK_HAS_PMU_RST_STAT                 (1 << 2)
> >  #define QUIRK_HAS_PMU_AUTO_DISABLE             (1 << 3)
> >  #define QUIRK_HAS_PMU_CNT_EN                   (1 << 4)
> > +#define QUIRK_HAS_DBGACK_BIT                   (1 << 5)
> > +#define QUIRK_HAS_WTMINCNT_REG                 (1 << 6)
>
> Please also document those two quirks in the kernel-doc comment above.
> Btw, the comment correctness can be checked like this:
>
>     $ scripts/kernel-doc -v -none drivers/watchdog/s3c2410_wdt.c
>
> or without "-none" option to see how the comment is parsed by kernel-doc.

Will do, and thanks for the kernel-doc hint!

>
> >
> >  /* These quirks require that we have a PMU register map */
> >  #define QUIRKS_HAVE_PMUREG \
> > @@ -263,6 +275,54 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl1 = {
> >                   QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> >  };
> >
> > +static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = {
> > +       .mask_reset_reg = GS_CLUSTER0_NONCPU_INT_EN,
> > +       .mask_bit = 2,
> > +       .mask_reset_inv = true,
> > +       .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > +       .rst_stat_bit = 0,
> > +       .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT,
> > +       .cnt_en_bit = 8,
> > +       .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
>
> Here and further: please stick to 80 characters per line when possible.

Will fix

>
> > +                 QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> > +static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = {
> > +       .mask_reset_reg = GS_CLUSTER1_NONCPU_INT_EN,
> > +       .mask_bit = 2,
> > +       .mask_reset_inv = true,
> > +       .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > +       .rst_stat_bit = 1,
> > +       .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT,
> > +       .cnt_en_bit = 7,
> > +       .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> > +                 QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> > +static const struct s3c2410_wdt_variant drv_data_gs201_cl0 = {
> > +       .mask_reset_reg = GS_CLUSTER0_NONCPU_INT_EN,
> > +       .mask_bit = 2,
> > +       .mask_reset_inv = true,
> > +       .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > +       .rst_stat_bit = 0,
> > +       .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT,
> > +       .cnt_en_bit = 8,
> > +       .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> > +                 QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> > +static const struct s3c2410_wdt_variant drv_data_gs201_cl1 = {
> > +       .mask_reset_reg = GS_CLUSTER1_NONCPU_INT_EN,
> > +       .mask_bit = 2,
> > +       .mask_reset_inv = true,
> > +       .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > +       .rst_stat_bit = 1,
> > +       .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT,
> > +       .cnt_en_bit = 7,
> > +       .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> > +                 QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> >  static const struct of_device_id s3c2410_wdt_match[] = {
> >         { .compatible = "samsung,s3c2410-wdt",
> >           .data = &drv_data_s3c2410 },
> > @@ -278,6 +338,10 @@ static const struct of_device_id s3c2410_wdt_match[] = {
> >           .data = &drv_data_exynos850_cl0 },
> >         { .compatible = "samsung,exynosautov9-wdt",
> >           .data = &drv_data_exynosautov9_cl0 },
> > +       { .compatible = "google,gs101-wdt",
> > +         .data = &drv_data_gs101_cl0 },
> > +       { .compatible = "google,gs201-wdt",
> > +         .data = &drv_data_gs201_cl0 },
> >         {},
> >  };
> >  MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> > @@ -375,6 +439,21 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> >         return 0;
> >  }
> >
> > +static void s3c2410wdt_mask_dbgack(struct s3c2410_wdt *wdt, bool mask)
> > +{
> > +       unsigned long wtcon;
> > +
> > +       if (!(wdt->drv_data->quirks & QUIRK_HAS_DBGACK_BIT))
> > +               return;
> > +
> > +       wtcon = readl(wdt->reg_base + S3C2410_WTCON);
> > +       if (mask)
> > +               wtcon |= S3C2410_WTCON_DBGACK_MASK;
> > +       else
> > +               wtcon &= ~S3C2410_WTCON_DBGACK_MASK;
> > +       writel(wtcon, wdt->reg_base + S3C2410_WTCON);
> > +}
> > +
> >  static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
> >  {
> >         struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> > @@ -410,7 +489,7 @@ static int s3c2410wdt_stop(struct watchdog_device *wdd)
> >
> >  static int s3c2410wdt_start(struct watchdog_device *wdd)
> >  {
> > -       unsigned long wtcon;
> > +       unsigned long wtcon, wtmincnt;
> >         struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> >         unsigned long flags;
> >
> > @@ -432,6 +511,12 @@ static int s3c2410wdt_start(struct watchdog_device *wdd)
> >         dev_dbg(wdt->dev, "Starting watchdog: count=0x%08x, wtcon=%08lx\n",
> >                 wdt->count, wtcon);
> >
> > +       if (wdt->drv_data->quirks & QUIRK_HAS_WTMINCNT_REG) {
> > +               wtcon |= S3C2410_WTCON_WINDOWED_WD;
> > +               wtmincnt = wdt->count * S3C2410_WINDOW_MULTIPLIER;
> > +               writel(wtmincnt, wdt->reg_base + S3C2410_WTMINCNT);
> > +       }
> > +
> >         writel(wdt->count, wdt->reg_base + S3C2410_WTDAT);
> >         writel(wdt->count, wdt->reg_base + S3C2410_WTCNT);
> >         writel(wtcon, wdt->reg_base + S3C2410_WTCON);
> > @@ -447,7 +532,7 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> >         unsigned long freq = s3c2410wdt_get_freq(wdt);
> >         unsigned int count;
> >         unsigned int divisor = 1;
> > -       unsigned long wtcon;
> > +       unsigned long wtcon, wtmincnt;
> >
> >         if (timeout < 1)
> >                 return -EINVAL;
> > @@ -478,6 +563,11 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd,
> >         count = DIV_ROUND_UP(count, divisor);
> >         wdt->count = count;
> >
> > +       if (wdt->drv_data->quirks & QUIRK_HAS_WTMINCNT_REG) {
> > +               wtmincnt = count * S3C2410_WINDOW_MULTIPLIER;
> > +               writel(wtmincnt, wdt->reg_base + S3C2410_WTMINCNT);
> > +       }
> > +
> >         /* update the pre-scaler */
> >         wtcon = readl(wdt->reg_base + S3C2410_WTCON);
> >         wtcon &= ~S3C2410_WTCON_PRESCALE_MASK;
> > @@ -496,14 +586,20 @@ static int s3c2410wdt_restart(struct watchdog_device *wdd, unsigned long action,
> >  {
> >         struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> >         void __iomem *wdt_base = wdt->reg_base;
> > +       unsigned long wtcon;
> >
> >         /* disable watchdog, to be safe  */
> >         writel(0, wdt_base + S3C2410_WTCON);
> >
> >         /* put initial values into count and data */
> > +       if (wdt->drv_data->quirks & QUIRK_HAS_WTMINCNT_REG)
> > +               writel(0x100, wdt_base + S3C2410_WTMINCNT);
> >         writel(0x80, wdt_base + S3C2410_WTCNT);
> >         writel(0x80, wdt_base + S3C2410_WTDAT);
> >
> > +       if (wdt->drv_data->quirks & QUIRK_HAS_WTMINCNT_REG)
> > +               wtcon |= S3C2410_WTCON_WINDOWED_WD;
> > +
> >         /* set the watchdog to go and reset... */
> >         writel(S3C2410_WTCON_ENABLE | S3C2410_WTCON_DIV16 |
> >                 S3C2410_WTCON_RSTEN | S3C2410_WTCON_PRESCALE(0x20),
> > @@ -585,9 +681,11 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt)
> >         }
> >
> >  #ifdef CONFIG_OF
> > -       /* Choose Exynos850/ExynosAutov9 driver data w.r.t. cluster index */
> > +       /* Choose Exynos850/ExynosAutov9/gsx01 driver data w.r.t. cluster index */
>
> Please keep 80 characters per line.

Will fix

regards,

Peter
>
> >         if (variant == &drv_data_exynos850_cl0 ||
> > -           variant == &drv_data_exynosautov9_cl0) {
> > +           variant == &drv_data_exynosautov9_cl0 ||
> > +           variant == &drv_data_gs101_cl0 ||
> > +           variant == &drv_data_gs201_cl0) {
> >                 u32 index;
> >                 int err;
> >
> > @@ -600,9 +698,14 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt)
> >                 case 0:
> >                         break;
> >                 case 1:
> > -                       variant = (variant == &drv_data_exynos850_cl0) ?
> > -                               &drv_data_exynos850_cl1 :
> > -                               &drv_data_exynosautov9_cl1;
> > +                       if (variant == &drv_data_exynos850_cl0)
> > +                               variant = &drv_data_exynos850_cl1;
> > +                       else if (variant == &drv_data_exynosautov9_cl0)
> > +                               variant = &drv_data_exynosautov9_cl1;
> > +                       else if (variant == &drv_data_gs101_cl0)
> > +                               variant = &drv_data_gs101_cl1;
> > +                       else if (variant == &drv_data_gs201_cl0)
> > +                               variant = &drv_data_gs201_cl1;
> >                         break;
> >                 default:
> >                         return dev_err_probe(dev, -EINVAL, "wrong cluster index: %u\n", index);
> > @@ -700,6 +803,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> >         wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
> >         wdt->wdt_device.parent = dev;
> >
> > +       s3c2410wdt_mask_dbgack(wdt, true);
> > +
> >         /*
> >          * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> >          * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> > --
> > 2.42.0.655.g421f12c284-goog
> >
Chanwoo Choi Oct. 18, 2023, 4:51 p.m. UTC | #3
Hi Peter,

On 23. 10. 12. 03:48, Peter Griffin wrote:
> CMU_TOP is the top level clock management unit which contains PLLs, muxes
> and gates that feed the other clock management units.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/clk/samsung/Kconfig     |    9 +
>  drivers/clk/samsung/Makefile    |    2 +
>  drivers/clk/samsung/clk-gs101.c | 1551 +++++++++++++++++++++++++++++++
>  3 files changed, 1562 insertions(+)
>  create mode 100644 drivers/clk/samsung/clk-gs101.c
> 
> diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
> index 76a494e95027..14362ec9c543 100644
> --- a/drivers/clk/samsung/Kconfig
> +++ b/drivers/clk/samsung/Kconfig
> @@ -12,6 +12,7 @@ config COMMON_CLK_SAMSUNG
>  	select EXYNOS_5410_COMMON_CLK if ARM && SOC_EXYNOS5410
>  	select EXYNOS_5420_COMMON_CLK if ARM && SOC_EXYNOS5420
>  	select EXYNOS_ARM64_COMMON_CLK if ARM64 && ARCH_EXYNOS
> +	select GOOGLE_GS101_COMMON_CLK if ARM64 && ARCH_GOOGLE_TENSOR
>  	select TESLA_FSD_COMMON_CLK if ARM64 && ARCH_TESLA_FSD
>  
>  config S3C64XX_COMMON_CLK
> @@ -95,6 +96,14 @@ config EXYNOS_CLKOUT
>  	  status of the certains clocks from SoC, but it could also be tied to
>  	  other devices as an input clock.
>  
> +config GOOGLE_GS101_COMMON_CLK
> +	bool "Google gs101 clock controller support" if COMPILE_TEST
> +	depends on COMMON_CLK_SAMSUNG
> +	depends on EXYNOS_ARM64_COMMON_CLK
> +	help
> +	  Support for the clock controller present on the Google gs101 SoC.
> +	  Choose Y here only if you build for this SoC.
> +

(snip)

> +
> +/* gs101 */
> +static const struct samsung_mux_clock cmu_top_mux_clks[] __initconst = {
> +	/* CMU_TOP_PURECLKCOMP */
> +	MUX(CLK_MOUT_SHARED0_PLL, "mout_shared0_pll", mout_shared0_pll_p,
> +	    PLL_CON0_PLL_SHARED0, 4, 1),
> +	MUX(CLK_MOUT_SHARED1_PLL, "mout_shared1_pll", mout_shared1_pll_p,
> +	    PLL_CON0_PLL_SHARED1, 4, 1),
> +	MUX(CLK_MOUT_SHARED2_PLL, "mout_shared2_pll", mout_shared2_pll_p,
> +	    PLL_CON0_PLL_SHARED2, 4, 1),
> +	MUX(CLK_MOUT_SHARED3_PLL, "mout_shared3_pll", mout_shared3_pll_p,
> +	    PLL_CON0_PLL_SHARED3, 4, 1),
> +	MUX(CLK_MOUT_SPARE_PLL, "mout_spare_pll", mout_spare_pll_p,
> +	    PLL_CON0_PLL_SPARE, 4, 1),
> +
> +	/* BUS0 */
> +	MUX(CLK_MOUT_BUS0_BUS, "mout_cmu_bus0_bus", mout_cmu_bus0_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_BUS0_BUS, 0, 2),
> +	MUX(CLK_MOUT_CMU_BOOST, "mout_cmu_boost", mout_cmu_cmu_boost_p,

In order to keep the consistent naming style,
I think that need to change from 'mout_cmu_boost' to 'mout_cmu_cmu_boost'.

> +	    CLK_CON_MUX_MUX_CLKCMU_CMU_BOOST, 0, 2),
> +
> +	/* BUS1 */
> +	MUX(CLK_MOUT_BUS1_BUS, "mout_cmu_bus1_bus", mout_cmu_bus1_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_BUS1_BUS, 0, 2),
> +
> +	/* BUS2 */
> +	MUX(CLK_MOUT_BUS2_BUS, "mout_cmu_bus2_bus", mout_cmu_bus2_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_BUS2_BUS, 0, 2),
> +
> +	/* CORE */
> +	MUX(CLK_MOUT_CORE_BUS, "mout_cmu_core_bus", mout_cmu_core_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_CORE_BUS, 0, 2),
> +
> +	/* EH */
> +	MUX(CLK_MOUT_EH_BUS, "mout_cmu_eh_bus", mout_cmu_eh_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_CORE_BUS, 0, 2),

'mout_cmu_core_bus' and 'mout_cmu_eh_bus' uses the same register/shift/width information. 
I think it should be modified by changing the regiter or changing the shift/width information.

> +
> +	/* CPUCL{0,1,2,} */
> +	MUX(CLK_MOUT_CPUCL2_SWITCH, "mout_cmu_cpucl2_switch", mout_cmu_cpucl2_switch_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_CPUCL2_SWITCH, 0, 2),
> +
> +	MUX(CLK_MOUT_CPUCL1_SWITCH, "mout_cmu_cpucl1_switch", mout_cmu_cpucl1_switch_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_CPUCL1_SWITCH, 0, 2),
> +
> +	MUX(CLK_MOUT_CPUCL0_SWITCH, "mout_cmu_cpucl0_switch", mout_cmu_cpucl0_switch_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_CPUCL0_SWITCH, 0, 2),
> +
> +	MUX(CLK_MOUT_CPUCL0_DBG, "mout_cmu_cpucl0_dbg", mout_cmu_cpucl0_dbg_p,
> +	    CLK_CON_DIV_CLKCMU_CPUCL0_DBG, 0, 2),
> +
> +	MUX(CLK_MOUT_CMU_HPM, "mout_cmu_hpm", mout_cmu_hpm_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_HPM, 0, 2),
>

(snip)

> +	/* PDP */
> +	MUX(CLK_MOUT_PDP_BUS, "mout_cmu_pdp_bus", mout_cmu_pdp_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_PDP_BUS, 0, 2),
> +
> +	/* PDP */
> +	MUX(CLK_MOUT_PDP_VRA, "mout_cmu_pdp_vra", mout_cmu_pdp_vra_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_PDP_VRA, 0, 2),
> +
> +	/* IPP */
> +	MUX(CLK_MOUT_IPP_BUS, "mout_cmu_ipp_bus", mout_cmu_ipp_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_IPP_BUS, 0, 2),
> +
> +	/* G3AA */
> +	MUX(CLK_MOUT_G3AA, "mout_cmu_g3aa", mout_cmu_g3aa_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_G3AA_G3AA, 0, 2),

I think that need to change the mux name and mux parent name
because other mux name use the twice word according to the register name
even if use the same work such as 'mout_cmu_g2d_g2d', 'mout_cmu_mcsc_mcsc' and 'mout_cmu_mfc_mfc'.
- mout_cmu_g3aa -> mout_cmu_g3aa_g3aa
- mout_cmu_g3aa_p -> mount_cmu_g3aa_g3aa_p

(snip)

> +	/* CSIS */
> +	GATE(CLK_GOUT_CSIS, "gout_cmu_csis_bus", "mout_cmu_csis_bus",
> +	     CLK_CON_GAT_GATE_CLKCMU_CSIS_BUS, 21, 0, 0),
> +	/* PDP */
> +	GATE(CLK_GOUT_PDP_BUS, "gout_cmu_pdp_bus", "mout_cmu_pdp_bus",
> +	     CLK_CON_GAT_GATE_CLKCMU_PDP_BUS, 21, 0, 0),
> +
> +	GATE(CLK_GOUT_PDP_VRA, "gout_cmu_pdp_vra", "mout_cmu_pdp_vra",
> +	     CLK_CON_GAT_GATE_CLKCMU_PDP_BUS, 21, 0, 0),
> +
> +	/* IPP */
> +	GATE(CLK_GOUT_IPP_BUS, "gout_cmu_ipp_bus", "mout_cmu_ipp_bus",
> +	     CLK_CON_GAT_GATE_CLKCMU_IPP_BUS, 21, 0, 0),
> +	/* G3AA */
> +	GATE(CLK_GOUT_G3AA, "gout_cmu_g3aa", "mout_cmu_g3aa",
> +	     CLK_CON_MUX_MUX_CLKCMU_G3AA_G3AA, 21, 0, 0),

ditto.
gout_cmu_g3aa -> gout_cmu_g3aa_g3aa
mout_cmu_g3aa -> mout_cmu_g3aa_g3aa
Chanwoo Choi Oct. 18, 2023, 5 p.m. UTC | #4
Hi,

On 23. 10. 12. 03:48, Peter Griffin wrote:
> This patch adds all the registers for the APM clock controller unit.
> 
> We register all the muxes and dividers, but only a few of the
> gates currently for PMU and GPIO.
> 
> One clock is marked CLK_IS_CRITICAL because the system
> hangs if this clock is disabled.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/clk/samsung/clk-gs101.c | 301 ++++++++++++++++++++++++++++++++
>  1 file changed, 301 insertions(+)
> 
> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> index e2c62754b1eb..525f95e60665 100644
> --- a/drivers/clk/samsung/clk-gs101.c
> +++ b/drivers/clk/samsung/clk-gs101.c
> @@ -19,6 +19,7 @@
>  

Looks good to me.

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
Peter Griffin Nov. 7, 2023, 12:18 p.m. UTC | #5
Hi Rob,

Thanks for your review.

On Mon, 16 Oct 2023 at 14:41, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Oct 11, 2023 at 07:48:09PM +0100, Peter Griffin wrote:
> > Add the "google,gs101-pinctrl" compatible to the dt-schema bindings
> > documentation.
> >
> > Add maxItems of 50 for the interrupts property as gs101 can have
> > multiple irqs.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  .../bindings/pinctrl/samsung,pinctrl.yaml     | 22 ++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml
> > index 26614621774a..6dc648490668 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml
> > +++ b/Documentation/devicetree/bindings/pinctrl/samsung,pinctrl.yaml
> > @@ -35,6 +35,7 @@ properties:
> >
> >    compatible:
> >      enum:
> > +      - google,gs101-pinctrl
> >        - samsung,s3c2412-pinctrl
> >        - samsung,s3c2416-pinctrl
> >        - samsung,s3c2440-pinctrl
> > @@ -58,7 +59,8 @@ properties:
> >    interrupts:
> >      description:
> >        Required for GPIO banks supporting external GPIO interrupts.
> > -    maxItems: 1
> > +    minItems: 1
> > +    maxItems: 50
> >
> >    power-domains:
> >      maxItems: 1
> > @@ -134,6 +136,24 @@ allOf:
> >            minItems: 1
> >            maxItems: 1
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: google,gs101-pinctrl
> > +    then:
> > +      properties:
> > +        interrupts:
> > +          description:
> > +            Required for external wakeup interrupts. List all external
>
> Is it external GPIO interrupts or wakeup interrupts?

These are external wakeup interrupts.

Looking again I believe this can be dropped entirely as re-reading
samsung,pinctrl-gpio-bank.yaml we are already defining the
external wake-up interrupts on each bank in gs101-pinctrl.dtsi.

>
> > +            wakeup interrupts supported by this bank.
> > +          minItems: 1
> > +          maxItems: 50
>
> For a given SoC, I don't see how this is variable? If it is variable,
> how do you know which entry is what?

It isn't variable.

Peter.
Peter Griffin Nov. 8, 2023, 1:43 p.m. UTC | #6
Hi Sam,

On Thu, 12 Oct 2023 at 07:00, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Wed, Oct 11, 2023 at 1:49 PM Peter Griffin <peter.griffin@linaro.org> wrote:
> >
> > Add support for the pin-controller found on the gs101 SoC used in
> > Pixel 6 phones.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  .../pinctrl/samsung/pinctrl-exynos-arm64.c    | 163 ++++++++++++++++++
> >  drivers/pinctrl/samsung/pinctrl-exynos.c      |   2 +
> >  drivers/pinctrl/samsung/pinctrl-exynos.h      |  34 ++++
> >  drivers/pinctrl/samsung/pinctrl-samsung.c     |   2 +
> >  drivers/pinctrl/samsung/pinctrl-samsung.h     |   1 +
> >  5 files changed, 202 insertions(+)
> >
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > index cb965cf93705..db47001d1b35 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> > @@ -796,3 +796,166 @@ const struct samsung_pinctrl_of_match_data fsd_of_data __initconst = {
> >         .ctrl           = fsd_pin_ctrl,
> >         .num_ctrl       = ARRAY_SIZE(fsd_pin_ctrl),
> >  };
> > +
> > +/*
> > + * bank type for non-alive type
> > + * (CON bit field: 4, DAT bit field: 1, PUD bit field: 4, DRV bit field: 4)
> > + * (CONPDN bit field: 2, PUDPDN bit field: 4)
> > + */
> > +static struct samsung_pin_bank_type gs101_bank_type_off  = {
> > +       .fld_width = { 4, 1, 4, 4, 2, 4, },
> > +       .reg_offset = { 0x00, 0x04, 0x08, 0x0c, 0x10, 0x14, },
> > +};
> > +
> > +/*
> > + * bank type for alive type
> > + * (CON bit field: 4, DAT bit field: 1, PUD bit field: 4, DRV bit field: 4)
> > + */
> > +static const struct samsung_pin_bank_type gs101_bank_type_alive = {
> > +       .fld_width = { 4, 1, 4, 4, },
> > +       .reg_offset = { 0x00, 0x04, 0x08, 0x0c, },
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (ALIVE) */
> > +static const struct samsung_pin_bank_data gs101_pin_alive[] = {
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0x0, "gpa0", 0x00, 0x00, FLT_SELECTABLE),
>
> Here and further: please keep 80 characters per line when possible.

Will fix.

>
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 7, 0x20, "gpa1", 0x04, 0x08, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 5, 0x40, "gpa2", 0x08, 0x10, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 4, 0x60, "gpa3", 0x0c, 0x18, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 4, 0x80, "gpa4", 0x10, 0x1c, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 7, 0xa0, "gpa5", 0x14, 0x20, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0xc0, "gpa9", 0x18, 0x28, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 2, 0xe0, "gpa10", 0x1c, 0x30, FLT_SELECTABLE),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (FAR_ALIVE) */
> > +static const struct samsung_pin_bank_data gs101_pin_far_alive[] = {
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0x0, "gpa6", 0x00, 0x00, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 4, 0x20, "gpa7", 0x04, 0x08, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 8, 0x40, "gpa8", 0x08, 0x0c, FLT_SELECTABLE),
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 2, 0x60, "gpa11", 0x0c, 0x14, FLT_SELECTABLE),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (GSACORE) */
> > +static const struct samsung_pin_bank_data gs101_pin_gsacore[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x0, "gps0", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 8, 0x20, "gps1", 0x04, 0x04, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 3, 0x40, "gps2", 0x08, 0x0c, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (GSACTRL) */
> > +static const struct samsung_pin_bank_data gs101_pin_gsactrl[] = {
> > +       EXYNOS9_PIN_BANK_EINTW(gs101_bank_type_alive, 6, 0x0, "gps3", 0x00, 0x00, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (PERIC0) */
> > +static const struct samsung_pin_bank_data gs101_pin_peric0[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 5, 0x0, "gpp0", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x20, "gpp1", 0x04, 0x08, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x40, "gpp2", 0x08, 0x0c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x60, "gpp3", 0x0c, 0x10, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x80, "gpp4", 0x10, 0x14, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0xa0, "gpp5", 0x14, 0x18, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0xc0, "gpp6", 0x18, 0x1c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0xe0, "gpp7", 0x1c, 0x20, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x100, "gpp8", 0x20, 0x24, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x120, "gpp9", 0x24, 0x28, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x140, "gpp10", 0x28, 0x2c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x160, "gpp11", 0x2c, 0x30, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x180, "gpp12", 0x30, 0x34, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x1a0, "gpp13", 0x34, 0x38, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x1c0, "gpp14", 0x38, 0x3c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x1e0, "gpp15", 0x3c, 0x40, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x200, "gpp16", 0x40, 0x44, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x220, "gpp17", 0x44, 0x48, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x240, "gpp18", 0x48, 0x4c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x260, "gpp19", 0x4c, 0x50, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (PERIC1) */
> > +static const struct samsung_pin_bank_data gs101_pin_peric1[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 8, 0x0, "gpp20", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x20, "gpp21", 0x04, 0x08, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x40, "gpp22", 0x08, 0x0c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 8, 0x60, "gpp23", 0x0c, 0x10, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0x80, "gpp24", 0x10, 0x18, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0xa0, "gpp25", 0x14, 0x1c, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 5, 0xc0, "gpp26", 0x18, 0x20, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 4, 0xe0, "gpp27", 0x1c, 0x28, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (HSI1) */
> > +static const struct samsung_pin_bank_data gs101_pin_hsi1[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 6, 0x0, "gph0", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 7, 0x20, "gph1", 0x04, 0x08, FLT_DEFAULT),
> > +};
> > +
> > +/* pin banks of gs101 pin-controller (HSI2) */
> > +static const struct samsung_pin_bank_data gs101_pin_hsi2[] = {
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 6, 0x0, "gph2", 0x00, 0x00, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 2, 0x20, "gph3", 0x04, 0x08, FLT_DEFAULT),
> > +       EXYNOS9_PIN_BANK_EINTG(gs101_bank_type_off, 6, 0x40, "gph4", 0x08, 0x0c, FLT_DEFAULT),
> > +};
> > +
> > +static const struct samsung_pin_ctrl gs101_pin_ctrl[] __initconst = {
> > +       {
> > +               /* pin banks of gs101 pin-controller (ALIVE) */
> > +               .pin_banks      = gs101_pin_alive,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_alive),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .eint_wkup_init = exynos_eint_wkup_init,
>
> Is it ok to have both .eint_gpio_init and .eint_wkup_init set here and
> further? I remember doing something like that for Exynos850 before,
> only to realize further if was a mistake. Please check commit
> 96f79935015c ("pinctrl: samsung: Remove EINT handler for Exynos850
> ALIVE and CMGP gpios"). Maybe it's ok in your case.

Thanks for the hint :) You're correct eint_gpio_init is not required on alive,
far_alive, gsacore and gsactrl banks. Will update in v4.

>
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
>
> Did you manage to actually test those suspend/resume callbacks
> somehow? If so, can you please share the procedure? I guess I had some
> Power Domains and clock related problems on Exynos850 when I tried
> that before, so just curious.

You can test the callbacks using
echo mem > /sys/power/state

Thanks,

Peter.




>
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (FAR_ALIVE) */
> > +               .pin_banks      = gs101_pin_far_alive,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_far_alive),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .eint_wkup_init = exynos_eint_wkup_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (GSACORE) */
> > +               .pin_banks      = gs101_pin_gsacore,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_gsacore),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (GSACTRL) */
> > +               .pin_banks      = gs101_pin_gsactrl,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_gsactrl),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (PERIC0) */
> > +               .pin_banks      = gs101_pin_peric0,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_peric0),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (PERIC1) */
> > +               .pin_banks      = gs101_pin_peric1,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_peric1),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (HSI1) */
> > +               .pin_banks      = gs101_pin_hsi1,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_hsi1),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       }, {
> > +               /* pin banks of gs101 pin-controller (HSI2) */
> > +               .pin_banks      = gs101_pin_hsi2,
> > +               .nr_banks       = ARRAY_SIZE(gs101_pin_hsi2),
> > +               .eint_gpio_init = exynos_eint_gpio_init,
> > +               .suspend        = exynos_pinctrl_suspend,
> > +               .resume         = exynos_pinctrl_resume,
> > +       },
> > +};
> > +
> > +const struct samsung_pinctrl_of_match_data gs101_of_data __initconst = {
> > +       .ctrl           = gs101_pin_ctrl,
> > +       .num_ctrl       = ARRAY_SIZE(gs101_pin_ctrl),
> > +};
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c b/drivers/pinctrl/samsung/pinctrl-exynos.c
> > index 800831aa8357..014f0c37f97f 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> > @@ -533,6 +533,8 @@ static const struct of_device_id exynos_wkup_irq_ids[] = {
> >                         .data = &exynos7_wkup_irq_chip },
> >         { .compatible = "samsung,exynosautov9-wakeup-eint",
> >                         .data = &exynos7_wkup_irq_chip },
> > +       { .compatible = "google,gs101-wakeup-eint",
> > +                       .data = &exynos7_wkup_irq_chip },
> >         { }
> >  };
> >
> > diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.h b/drivers/pinctrl/samsung/pinctrl-exynos.h
> > index 63b2426ad5d6..0dd013654bd2 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-exynos.h
> > +++ b/drivers/pinctrl/samsung/pinctrl-exynos.h
> > @@ -147,6 +147,40 @@
> >                 .name           = id                            \
> >         }
> >
> > +#define EXYNOS9_PIN_BANK_EINTN(types, pins, reg, id)   \
> > +       {                                               \
> > +               .type           = &types,               \
> > +               .pctl_offset    = reg,                  \
> > +               .nr_pins        = pins,                 \
> > +               .eint_type      = EINT_TYPE_NONE,       \
> > +               .fltcon_type    = FLT_DEFAULT           \
> > +               .name           = id                    \
> > +       }
> > +
> > +#define EXYNOS9_PIN_BANK_EINTG(types, pins, reg, id, offs, fltcon_offs, fltcontype) \
> > +       {                                               \
> > +               .type           = &types,               \
> > +               .pctl_offset    = reg,                  \
> > +               .nr_pins        = pins,                 \
> > +               .eint_type      = EINT_TYPE_GPIO,       \
> > +               .eint_offset    = offs,                 \
> > +               .fltcon_type    = fltcontype,           \
> > +               .fltcon_offset  = fltcon_offs,          \
> > +               .name           = id                    \
> > +       }
> > +
> > +#define EXYNOS9_PIN_BANK_EINTW(types, pins, reg, id, offs, fltcon_offs, fltcontype) \
> > +       {                                               \
> > +               .type           = &types,               \
> > +               .pctl_offset    = reg,                  \
> > +               .nr_pins        = pins,                 \
> > +               .eint_type      = EINT_TYPE_WKUP,       \
> > +               .eint_offset    = offs,                 \
> > +               .fltcon_type    = fltcontype,           \
> > +               .fltcon_offset  = fltcon_offs,          \
> > +               .name           = id                    \
> > +       }
> > +
>
> Looks to me that instead of adding new macros the already existing
> EXYNOS850_PIN_BANK_* should be extended and re-used. Because those
> pinctrl IP-cores on all modern Exynos chips look very similar, even if
> you compare the downstream code. If EXYNOS850 prefix looks confusing,
> maybe it can be renamed to EXYNOS9 or something like that. Those
> filter parameters are also present in Exynos850 downstream kernel
> code. So I just feel like the proper way to add that feature would be
> to add that also for all modern ARM64 Exynos variants while at it.
>
> >  /**
> >   * struct exynos_weint_data: irq specific data for all the wakeup interrupts
> >   * generated by the external wakeup interrupt controller.
> > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > index 449f8109d8b5..12176f98440d 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> > @@ -1321,6 +1321,8 @@ static const struct of_device_id samsung_pinctrl_dt_match[] = {
> >                 .data = &exynosautov9_of_data },
> >         { .compatible = "tesla,fsd-pinctrl",
> >                 .data = &fsd_of_data },
> > +       { .compatible = "google,gs101-pinctrl",
> > +               .data = &gs101_of_data },
> >  #endif
> >  #ifdef CONFIG_PINCTRL_S3C64XX
> >         { .compatible = "samsung,s3c64xx-pinctrl",
> > diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.h b/drivers/pinctrl/samsung/pinctrl-samsung.h
> > index de2ca8e8b378..e62e909fb10d 100644
> > --- a/drivers/pinctrl/samsung/pinctrl-samsung.h
> > +++ b/drivers/pinctrl/samsung/pinctrl-samsung.h
> > @@ -374,6 +374,7 @@ extern const struct samsung_pinctrl_of_match_data exynos7885_of_data;
> >  extern const struct samsung_pinctrl_of_match_data exynos850_of_data;
> >  extern const struct samsung_pinctrl_of_match_data exynosautov9_of_data;
> >  extern const struct samsung_pinctrl_of_match_data fsd_of_data;
> > +extern const struct samsung_pinctrl_of_match_data gs101_of_data;
> >  extern const struct samsung_pinctrl_of_match_data s3c64xx_of_data;
> >  extern const struct samsung_pinctrl_of_match_data s3c2412_of_data;
> >  extern const struct samsung_pinctrl_of_match_data s3c2416_of_data;
> > --
> > 2.42.0.655.g421f12c284-goog
> >
Sam Protsenko Nov. 8, 2023, 5:33 p.m. UTC | #7
On Tue, Nov 7, 2023 at 7:57 AM Peter Griffin <peter.griffin@linaro.org> wrote:
>
> Hi Chanwoo,
>
> Thanks for your review!
>
> On Wed, 18 Oct 2023 at 17:51, Chanwoo Choi <chanwoo@kernel.org> wrote:
> >
> > Hi Peter,
> >
> > On 23. 10. 12. 03:48, Peter Griffin wrote:
> > > CMU_TOP is the top level clock management unit which contains PLLs, muxes
> > > and gates that feed the other clock management units.
> > >
> > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > > ---
> > >  drivers/clk/samsung/Kconfig     |    9 +
> > >  drivers/clk/samsung/Makefile    |    2 +
> > >  drivers/clk/samsung/clk-gs101.c | 1551 +++++++++++++++++++++++++++++++
> > >  3 files changed, 1562 insertions(+)
> > >  create mode 100644 drivers/clk/samsung/clk-gs101.c
> > >
> > > diff --git a/drivers/clk/samsung/Kconfig b/drivers/clk/samsung/Kconfig
> > > index 76a494e95027..14362ec9c543 100644
> > > --- a/drivers/clk/samsung/Kconfig
> > > +++ b/drivers/clk/samsung/Kconfig
> > > @@ -12,6 +12,7 @@ config COMMON_CLK_SAMSUNG
> > >       select EXYNOS_5410_COMMON_CLK if ARM && SOC_EXYNOS5410
> > >       select EXYNOS_5420_COMMON_CLK if ARM && SOC_EXYNOS5420
> > >       select EXYNOS_ARM64_COMMON_CLK if ARM64 && ARCH_EXYNOS
> > > +     select GOOGLE_GS101_COMMON_CLK if ARM64 && ARCH_GOOGLE_TENSOR
> > >       select TESLA_FSD_COMMON_CLK if ARM64 && ARCH_TESLA_FSD
> > >
> > >  config S3C64XX_COMMON_CLK
> > > @@ -95,6 +96,14 @@ config EXYNOS_CLKOUT
> > >         status of the certains clocks from SoC, but it could also be tied to
> > >         other devices as an input clock.
> > >
> > > +config GOOGLE_GS101_COMMON_CLK
> > > +     bool "Google gs101 clock controller support" if COMPILE_TEST
> > > +     depends on COMMON_CLK_SAMSUNG
> > > +     depends on EXYNOS_ARM64_COMMON_CLK
> > > +     help
> > > +       Support for the clock controller present on the Google gs101 SoC.
> > > +       Choose Y here only if you build for this SoC.
> > > +
> >
> > (snip)
> >
> > > +
> > > +/* gs101 */
> > > +static const struct samsung_mux_clock cmu_top_mux_clks[] __initconst = {
> > > +     /* CMU_TOP_PURECLKCOMP */
> > > +     MUX(CLK_MOUT_SHARED0_PLL, "mout_shared0_pll", mout_shared0_pll_p,
> > > +         PLL_CON0_PLL_SHARED0, 4, 1),
> > > +     MUX(CLK_MOUT_SHARED1_PLL, "mout_shared1_pll", mout_shared1_pll_p,
> > > +         PLL_CON0_PLL_SHARED1, 4, 1),
> > > +     MUX(CLK_MOUT_SHARED2_PLL, "mout_shared2_pll", mout_shared2_pll_p,
> > > +         PLL_CON0_PLL_SHARED2, 4, 1),
> > > +     MUX(CLK_MOUT_SHARED3_PLL, "mout_shared3_pll", mout_shared3_pll_p,
> > > +         PLL_CON0_PLL_SHARED3, 4, 1),
> > > +     MUX(CLK_MOUT_SPARE_PLL, "mout_spare_pll", mout_spare_pll_p,
> > > +         PLL_CON0_PLL_SPARE, 4, 1),
> > > +
> > > +     /* BUS0 */
> > > +     MUX(CLK_MOUT_BUS0_BUS, "mout_cmu_bus0_bus", mout_cmu_bus0_bus_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_BUS0_BUS, 0, 2),
> > > +     MUX(CLK_MOUT_CMU_BOOST, "mout_cmu_boost", mout_cmu_cmu_boost_p,
> >
> > In order to keep the consistent naming style,
> > I think that need to change from 'mout_cmu_boost' to 'mout_cmu_cmu_boost'.
>
> Yes, that's a good point, and a good spot! Will fix it in v4.
>

Why do we need cmu_cmu part at all? From the look of it, renaming all
*_cmu_cmu_* clocks to just cmu wouldn't cause any naming conflicts. So
I don't see any benefit of double cmu prefix really.

> >
> > > +         CLK_CON_MUX_MUX_CLKCMU_CMU_BOOST, 0, 2),
> > > +
> > > +     /* BUS1 */
> > > +     MUX(CLK_MOUT_BUS1_BUS, "mout_cmu_bus1_bus", mout_cmu_bus1_bus_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_BUS1_BUS, 0, 2),
> > > +
> > > +     /* BUS2 */
> > > +     MUX(CLK_MOUT_BUS2_BUS, "mout_cmu_bus2_bus", mout_cmu_bus2_bus_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_BUS2_BUS, 0, 2),
> > > +
> > > +     /* CORE */
> > > +     MUX(CLK_MOUT_CORE_BUS, "mout_cmu_core_bus", mout_cmu_core_bus_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_CORE_BUS, 0, 2),
> > > +
> > > +     /* EH */
> > > +     MUX(CLK_MOUT_EH_BUS, "mout_cmu_eh_bus", mout_cmu_eh_bus_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_CORE_BUS, 0, 2),
> >
> > 'mout_cmu_core_bus' and 'mout_cmu_eh_bus' uses the same register/shift/width information.
> > I think it should be modified by changing the regiter or changing the shift/width information.
>
> It should be using the CLK_CON_MUX_MUX_CLKCMU_EH_BUS register.
> Will fix it in v4.
>
> >
> > > +
> > > +     /* CPUCL{0,1,2,} */
> > > +     MUX(CLK_MOUT_CPUCL2_SWITCH, "mout_cmu_cpucl2_switch", mout_cmu_cpucl2_switch_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_CPUCL2_SWITCH, 0, 2),
> > > +
> > > +     MUX(CLK_MOUT_CPUCL1_SWITCH, "mout_cmu_cpucl1_switch", mout_cmu_cpucl1_switch_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_CPUCL1_SWITCH, 0, 2),
> > > +
> > > +     MUX(CLK_MOUT_CPUCL0_SWITCH, "mout_cmu_cpucl0_switch", mout_cmu_cpucl0_switch_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_CPUCL0_SWITCH, 0, 2),
> > > +
> > > +     MUX(CLK_MOUT_CPUCL0_DBG, "mout_cmu_cpucl0_dbg", mout_cmu_cpucl0_dbg_p,
> > > +         CLK_CON_DIV_CLKCMU_CPUCL0_DBG, 0, 2),
> > > +
> > > +     MUX(CLK_MOUT_CMU_HPM, "mout_cmu_hpm", mout_cmu_hpm_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_HPM, 0, 2),
> > >
> >
> > (snip)
> >
> > > +     /* PDP */
> > > +     MUX(CLK_MOUT_PDP_BUS, "mout_cmu_pdp_bus", mout_cmu_pdp_bus_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_PDP_BUS, 0, 2),
> > > +
> > > +     /* PDP */
> > > +     MUX(CLK_MOUT_PDP_VRA, "mout_cmu_pdp_vra", mout_cmu_pdp_vra_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_PDP_VRA, 0, 2),
> > > +
> > > +     /* IPP */
> > > +     MUX(CLK_MOUT_IPP_BUS, "mout_cmu_ipp_bus", mout_cmu_ipp_bus_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_IPP_BUS, 0, 2),
> > > +
> > > +     /* G3AA */
> > > +     MUX(CLK_MOUT_G3AA, "mout_cmu_g3aa", mout_cmu_g3aa_p,
> > > +         CLK_CON_MUX_MUX_CLKCMU_G3AA_G3AA, 0, 2),
> >
> > I think that need to change the mux name and mux parent name
> > because other mux name use the twice word according to the register name
> > even if use the same work such as 'mout_cmu_g2d_g2d', 'mout_cmu_mcsc_mcsc' and 'mout_cmu_mfc_mfc'.
> > - mout_cmu_g3aa -> mout_cmu_g3aa_g3aa
> > - mout_cmu_g3aa_p -> mount_cmu_g3aa_g3aa_p
>
> Will fix in v4
>

That consistent name duplication, while not causing any conflicts when
being removed, looks suspicious to me. That's probably some internal
scheme which doesn't make much sense for us and doesn't bring any
value, in terms of clock drivers. Maybe it'll be better to instead get
rid of such duplication throughout the driver, at least for clock name
strings? I mention this, because that's what I did in clk-exynos850.
With the only exception being the main domain clocks, which basically
enables/disables the whole unit internally, e.g.

    GATE(CLK_GOUT_G3D_CMU_G3D_PCLK, "gout_g3d_cmu_g3d_pclk", ...

which "G3D domain gate clock that enables/disables G3D", or something
like that. But clk-exynos850 doesn't have any duplicating bits like
"cmu_cmu" or "g3d_g3d". And the reason why I did that is I wanted
those clock names appear short and nice in device tree, as there were
no benefits in those duplicating bits.

> >
> > (snip)
> >
> > > +     /* CSIS */
> > > +     GATE(CLK_GOUT_CSIS, "gout_cmu_csis_bus", "mout_cmu_csis_bus",
> > > +          CLK_CON_GAT_GATE_CLKCMU_CSIS_BUS, 21, 0, 0),
> > > +     /* PDP */
> > > +     GATE(CLK_GOUT_PDP_BUS, "gout_cmu_pdp_bus", "mout_cmu_pdp_bus",
> > > +          CLK_CON_GAT_GATE_CLKCMU_PDP_BUS, 21, 0, 0),
> > > +
> > > +     GATE(CLK_GOUT_PDP_VRA, "gout_cmu_pdp_vra", "mout_cmu_pdp_vra",
> > > +          CLK_CON_GAT_GATE_CLKCMU_PDP_BUS, 21, 0, 0),
> > > +
> > > +     /* IPP */
> > > +     GATE(CLK_GOUT_IPP_BUS, "gout_cmu_ipp_bus", "mout_cmu_ipp_bus",
> > > +          CLK_CON_GAT_GATE_CLKCMU_IPP_BUS, 21, 0, 0),
> > > +     /* G3AA */
> > > +     GATE(CLK_GOUT_G3AA, "gout_cmu_g3aa", "mout_cmu_g3aa",
> > > +          CLK_CON_MUX_MUX_CLKCMU_G3AA_G3AA, 21, 0, 0),
> >
> > ditto.
> > gout_cmu_g3aa -> gout_cmu_g3aa_g3aa
> > mout_cmu_g3aa -> mout_cmu_g3aa_g3aa
>

Ditto.

> Will fix in V4
>
> regards,
>
> Peter.
Peter Griffin Nov. 24, 2023, 11:22 p.m. UTC | #8
Hi Krzysztof,

On Thu, 12 Oct 2023 at 07:40, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/10/2023 20:48, Peter Griffin wrote:
>
> ...
>
> > diff --git a/arch/arm64/boot/dts/google/gs101.dtsi b/arch/arm64/boot/dts/google/gs101.dtsi
> > new file mode 100644
> > index 000000000000..37fb0a4dc8d3
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/google/gs101.dtsi
> > @@ -0,0 +1,504 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * GS101 SoC
> > + *
> > + * Copyright 2019-2023 Google LLC
> > + *
> > + */
> > +
> > +#include <dt-bindings/clock/google,gs101.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +/ {
> > +     compatible = "google,gs101";
> > +     #address-cells = <2>;
> > +     #size-cells = <1>;
> > +
> > +     interrupt-parent = <&gic>;
> > +
> > +     aliases {
> > +             pinctrl0 = &pinctrl_0;
> > +             pinctrl1 = &pinctrl_1;
> > +             pinctrl2 = &pinctrl_2;
> > +             pinctrl3 = &pinctrl_3;
> > +             pinctrl4 = &pinctrl_4;
> > +             pinctrl5 = &pinctrl_5;
> > +             pinctrl6 = &pinctrl_6;
> > +             pinctrl7 = &pinctrl_7;
> > +             serial0 = &serial_0;
> > +     };
> > +
> > +     arm-pmu {
>
> pmu-0

will fix
>
> > +             compatible = "arm,armv8-pmuv3";
> > +             interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
> > +     };
> > +
> > +     dsu-pmu-0 {
>
> pmu-1

will fix

>
>
> > +             compatible = "arm,dsu-pmu";
> > +             interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH>;
> > +             cpus = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>,
> > +                    <&cpu4>, <&cpu5>, <&cpu6>, <&cpu7>;
> > +     };
> > +
> > +     /* TODO replace with CCF clock */
> > +     dummy_clk: oscillator {
> > +             compatible = "fixed-clock";
> > +             #clock-cells = <0>;
> > +             clock-frequency = <12345>;
> > +             clock-output-names = "pclk";
> > +     };
> > +
> > +     cpus {
> > +             #address-cells = <2>;
> > +             #size-cells = <0>;
> > +
> > +             cpu-map {
> > +                     cluster0 {
> > +                             core0 {
> > +                                     cpu = <&cpu0>;
> > +                             };
> > +                             core1 {
> > +                                     cpu = <&cpu1>;
> > +                             };
> > +                             core2 {
> > +                                     cpu = <&cpu2>;
> > +                             };
> > +                             core3 {
> > +                                     cpu = <&cpu3>;
> > +                             };
> > +                     };
> > +
> > +                     cluster1 {
> > +                             core0 {
> > +                                     cpu = <&cpu4>;
> > +                             };
> > +                             core1 {
> > +                                     cpu = <&cpu5>;
> > +                             };
> > +                     };
> > +
> > +                     cluster2 {
> > +                             core0 {
> > +                                     cpu = <&cpu6>;
> > +                             };
> > +                             core1 {
> > +                                     cpu = <&cpu7>;
> > +                             };
> > +                     };
> > +             };
> > +
> > +             cpu0: cpu@0 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0000>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&ANANKE_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <250>;
> > +                     dynamic-power-coefficient = <70>;
> > +             };
> > +
> > +             cpu1: cpu@100 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0100>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&ANANKE_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <250>;
> > +                     dynamic-power-coefficient = <70>;
> > +             };
> > +
> > +             cpu2: cpu@200 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0200>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&ANANKE_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <250>;
> > +                     dynamic-power-coefficient = <70>;
> > +             };
> > +
> > +             cpu3: cpu@300 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0300>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&ANANKE_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <250>;
> > +                     dynamic-power-coefficient = <70>;
> > +             };
> > +
> > +             cpu4: cpu@400 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0400>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&ENYO_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <620>;
> > +                     dynamic-power-coefficient = <284>;
> > +             };
> > +
> > +             cpu5: cpu@500 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0500>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&ENYO_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <620>;
> > +                     dynamic-power-coefficient = <284>;
> > +             };
> > +
> > +             cpu6: cpu@600 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0600>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&HERA_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <1024>;
> > +                     dynamic-power-coefficient = <650>;
> > +             };
> > +
> > +             cpu7: cpu@700 {
> > +                     device_type = "cpu";
> > +                     compatible = "arm,armv8";
> > +                     reg = <0x0 0x0700>;
> > +                     enable-method = "psci";
> > +                     cpu-idle-states =  <&HERA_CPU_SLEEP>;
> > +                     capacity-dmips-mhz = <1024>;
> > +                     dynamic-power-coefficient = <650>;
> > +             };
> > +
> > +             idle-states {
> > +                     entry-method = "psci";
> > +
> > +                     ANANKE_CPU_SLEEP: cpu-ananke-sleep {
> > +                             idle-state-name = "c2";
> > +                             compatible = "arm,idle-state";
> > +                             arm,psci-suspend-param = <0x0010000>;
> > +                             entry-latency-us = <70>;
> > +                             exit-latency-us = <160>;
> > +                             min-residency-us = <2000>;
> > +                     };
> > +
> > +                     ENYO_CPU_SLEEP: cpu-enyo-sleep {
> > +                             idle-state-name = "c2";
> > +                             compatible = "arm,idle-state";
> > +                             arm,psci-suspend-param = <0x0010000>;
> > +                             entry-latency-us = <150>;
> > +                             exit-latency-us = <190>;
> > +                             min-residency-us = <2500>;
> > +                     };
> > +
> > +                     HERA_CPU_SLEEP: cpu-hera-sleep {
> > +                             idle-state-name = "c2";
> > +                             compatible = "arm,idle-state";
> > +                             arm,psci-suspend-param = <0x0010000>;
> > +                             entry-latency-us = <235>;
> > +                             exit-latency-us = <220>;
> > +                             min-residency-us = <3500>;
> > +                     };
> > +             };
> > +     };
> > +
> > +     /* bootloader requires ect node */
> > +     ect {
>
> This needs bindings.

I experimented a bit more and the minimum I need is an empty dt node
called ect, otherwise the bootloader will boot loop and we can't boot
the kernel
[   2.977870] [E] [BOOT] fdt /ect path not found -1

Apart from a comment indicating that the bootloader requires this
empty ect dt node, what other bindings documentation would you like to
see? Something in google.yaml?

>
> > +             parameter_address = <0x90000000>;
>
> No underscores in property names. Use hyphen.
>
> > +             parameter_size = <0x53000>;
>
> No underscores.

Fortunately I can remove parameter_address and parameter_size and
still boot, so I will remove these in the next version.

Thanks,

Peter.
Peter Griffin Nov. 24, 2023, 11:53 p.m. UTC | #9
Hi Krzysztof,


On Thu, 12 Oct 2023 at 07:44, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/10/2023 20:48, Peter Griffin wrote:
> > Google gs101 SoC is ARMv8 mobile SoC found in the Pixel 6,
> > (oriole) Pixel 6a (bluejay) and Pixel 6 pro (raven) mobile
> > phones. It features:
> > * 4xA55 little cluster
> > * 2xA76 Mid cluster
> > * 2xX1 Big cluster
> >
>
> ...
>
> > +     gpa10: gpa10-gpio-bank  {
> > +             gpio-controller;
> > +             #gpio-cells = <2>;
> > +             interrupt-controller;
> > +             #interrupt-cells = <2>;
> > +             interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
> > +                        <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> > +     };
> > +
> > +     uart15_bus: uart15-bus-pins {
> > +            samsung,pins = "gpa2-3", "gpa2-4";
> > +            samsung,pin-function = <2>;
>
> GS101_PIN_FUNC_2

will fix
>
> > +            samsung,pin-pud = <0>;
>
> GS101_PIN_PULL_NONE

will fix

>
> > +     };
> > +
> > +     uart16_bus: uart16-bus-pins {
> > +            samsung,pins = "gpa3-0", "gpa3-1", "gpa3-2", "gpa3-3";
> > +            samsung,pin-function = <GS101_PIN_FUNC_2>;
> > +            samsung,pin-pud = <GS101_PIN_PULL_NONE>;
>
> But here it is correct...
>
> > +     };
> > +
> > +     uart16_bus_rts: uart1-bus-rts-pins {
> > +             samsung,pins = "gpa3-2";
> > +             samsung,pin-function = <GS101_PIN_FUNC_OUTPUT>;
> > +             samsung,pin-pud = <GS101_PIN_PULL_NONE>;
> > +             samsung,pin-val = <1>;
>
> Why do you set UART RTS pin value?

It's a remnant from the downstream drivers that support rts-gpio control.
I will remove the samsung,pin-val = <1> in the next version.

Thanks,

Peter