mbox series

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

Message ID 20231201160925.3136868-1-peter.griffin@linaro.org (mailing list archive)
Headers show
Series Add minimal Tensor/GS101 SoC support and Oriole/Pixel6 board | expand

Message

Peter Griffin Dec. 1, 2023, 4:09 p.m. UTC
Hi folks,

Thanks to everyone who reviewed the previous rounds. V5 incorporates
the review feedback received so far, and is rebased onto linux-next as
per Krzysztof request to incorporate all his dt-binding changes for
exynos.

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 can be re-used.

The support added in this series consists of:
* cpus
* pinctrl
* CCF implementation of cmu_top, cmu_misc & cmu_apm
* watchdog
* USI 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: Booting without a dtbo works with some versions of the bootloader
but crashes on others. Later versions aren't necessarily better. You can
get the bootloader version with `fastboot getvar version-bootloader`
Known good bootloader versions are: -
- slider-1.3-11000330
- slider-1.2-9152140
Known to crash without dtbo
- slider-1.3-10780582

kind regards,

Peter.

lore v4: https://lore.kernel.org/linux-arm-kernel/20231120212037.911774-1-peter.griffin@linaro.org/T/
pw   v3: https://patchwork.kernel.org/project/linux-samsung-soc/cover/20231011184823.443959-1-peter.griffin@linaro.org/
lore v2: https://lore.kernel.org/all/20231010224928.2296997-1-peter.griffin@linaro.org/
lore v1: https://lore.kernel.org/linux-arm-kernel/20231005155618.700312-1-peter.griffin@linaro.org/

Changes since v4:
 - clk-gs101: order cmu_top by register address, fix incorrect mux widths,
   add missing mux/div/gates (Andre)
 - google,gs101.h: add missing space in comment (Andre)
 - ckl-gs101:google,gs101.h add all remaining gates for cmu_misc and cmu_apm
 - update pmu dt labels (Krzysztof)
 - Remove uart16 rts/tx gpio definitions (Krzysztof)
 - Fixup various dts cosmetic nits (using consts, alignments,
   names) (Sam/Krzysztof)
 - Add more specific compatibles for arm cpu's and pmu (Sam)
 - Use address-cells 1 and ranges property for SoC addresses (Sam)
 - Encapsulate uart node in USI node (Sam)
 - Remove earlycon from bootargs
 - s3c2410_wdt: Reword QUIRK_HAS_DBGACK_BIT docs and add comment (Guenter)
 - s3c2410_wdt: Reorder DBGACK_MASK functionality first, gs101
   SoC second (Sam/Krzysztof)
 
Changes since v3:
 - Add reviewed-by and tested-by tags
 - google,gs101-clock.yaml: move Required to before Allof,
   enum for cmu*top/misc (Krzysztof)
 - samsung-wdt.yaml: stick to 80chars (Sam)
 - google.yaml - remove new line
 - samsung,pinctrl-wakeup-interrupt: sort alphabetically (RobH)
 - gs101-oriole.dts: update gpio-keys pinctrl-0 phandle for keys (Stephen)
 - samsung,exynos-sysreg.yaml - Alphabetical order (RobH)
 - pinctrl-exynos: update/move comments, slight cosmetic changes (Sam)
 - samsung_tty.c: update to generic drv_data name/macro (Arnd)
 - samsung_uart.yaml: make samsung,uart-fifosize required for gs101-uart (Arnd)
 - pinctrl-exynos: Remove eint irqs from alive pin controller node (Peter/Rob)
 - Fixup kernel test robot unused const variable warnings
 - clk-gs101: Update to mout_cmu_eh_bus to CLK_CON_MUX_MUX_CLKCMU_EH_BUS
   (Chanwoo)
 - clk-gs101: Update g3aa gout/dout/mout names to g3aa_g3aa for
   consistency (Chanwoo)
 - Remove .eint_gpio_init() cb on alive, alive_far, gsacore & gsactrl
   banks (Sam)
 - s3c2410_wdt: Drop windowed watchdog mode for now (Peter)
 - s3c2410_wdt: Separate gs101 SoC support from dbgack feature (Sam)
 - Move dts to arch/arm64/boot/dts/exynos/google directory (Krzysztof)

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 is 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 registered 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 (19):
  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 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
  dt-bindings: serial: samsung: Make samsung,uart-fifosize required
    property
  clk: samsung: clk-pll: Add support for pll_{0516,0517,518}
  clk: samsung: clk-gs101: Add cmu_top, cmu_misc and cmu_apm support
  pinctrl: samsung: Add filter selection support for alive banks
  pinctrl: samsung: Add gs101 SoC pinctrl configuration
  watchdog: s3c2410_wdt: Add support for WTCON register DBGACK_MASK bit
  watchdog: s3c2410_wdt: Add support for Google gs101 SoC
  tty: serial: samsung: Add gs101 compatible and common
    fifoszdt_serial_drv_data
  arm64: dts: exynos: google: Add initial Google gs101 SoC support
  arm64: dts: exynos: google: Add initial Oriole/pixel 6 board support
  MAINTAINERS: add entry for Google Tensor SoC

Tudor Ambarus (1):
  dt-bindings: soc: samsung: usi: add google,gs101-usi compatible

 .../devicetree/bindings/arm/google.yaml       |   53 +
 .../bindings/clock/google,gs101-clock.yaml    |  110 +
 .../samsung,pinctrl-wakeup-interrupt.yaml     |    2 +
 .../bindings/pinctrl/samsung,pinctrl.yaml     |    1 +
 .../bindings/serial/samsung_uart.yaml         |   11 +
 .../bindings/soc/samsung/exynos-pmu.yaml      |    2 +
 .../bindings/soc/samsung/exynos-usi.yaml      |    3 +
 .../soc/samsung/samsung,exynos-sysreg.yaml    |    6 +
 .../bindings/watchdog/samsung-wdt.yaml        |    8 +-
 MAINTAINERS                                   |   10 +
 arch/arm64/boot/dts/exynos/Makefile           |    2 +
 arch/arm64/boot/dts/exynos/google/Makefile    |    4 +
 .../boot/dts/exynos/google/gs101-oriole.dts   |  105 +
 .../boot/dts/exynos/google/gs101-pinctrl.dtsi | 1250 +++++++++
 .../boot/dts/exynos/google/gs101-pinctrl.h    |   33 +
 arch/arm64/boot/dts/exynos/google/gs101.dtsi  |  476 ++++
 drivers/clk/samsung/Makefile                  |    1 +
 drivers/clk/samsung/clk-gs101.c               | 2495 +++++++++++++++++
 drivers/clk/samsung/clk-pll.c                 |    6 +
 drivers/clk/samsung/clk-pll.h                 |    3 +
 .../pinctrl/samsung/pinctrl-exynos-arm64.c    |  159 ++
 drivers/pinctrl/samsung/pinctrl-exynos.c      |   91 +-
 drivers/pinctrl/samsung/pinctrl-exynos.h      |   41 +
 drivers/pinctrl/samsung/pinctrl-samsung.c     |    4 +
 drivers/pinctrl/samsung/pinctrl-samsung.h     |   23 +
 drivers/tty/serial/samsung_tty.c              |   16 +
 drivers/watchdog/s3c2410_wdt.c                |   74 +-
 include/dt-bindings/clock/google,gs101.h      |  392 +++
 28 files changed, 5370 insertions(+), 11 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/exynos/google/Makefile
 create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
 create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-pinctrl.dtsi
 create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-pinctrl.h
 create mode 100644 arch/arm64/boot/dts/exynos/google/gs101.dtsi
 create mode 100644 drivers/clk/samsung/clk-gs101.c
 create mode 100644 include/dt-bindings/clock/google,gs101.h

Comments

William McVicker Dec. 1, 2023, 10:40 p.m. UTC | #1
On 12/01/2023, Peter Griffin wrote:
> Hi folks,
> 
> Thanks to everyone who reviewed the previous rounds. V5 incorporates
> the review feedback received so far, and is rebased onto linux-next as
> per Krzysztof request to incorporate all his dt-binding changes for
> exynos.
> 
> 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 can be re-used.
> 
> The support added in this series consists of:
> * cpus
> * pinctrl
> * CCF implementation of cmu_top, cmu_misc & cmu_apm
> * watchdog
> * USI 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: Booting without a dtbo works with some versions of the bootloader
> but crashes on others. Later versions aren't necessarily better. You can
> get the bootloader version with `fastboot getvar version-bootloader`
> Known good bootloader versions are: -
> - slider-1.3-11000330
> - slider-1.2-9152140
> Known to crash without dtbo
> - slider-1.3-10780582
> 
> kind regards,
> 
> Peter.
> 
> lore v4: https://lore.kernel.org/linux-arm-kernel/20231120212037.911774-1-peter.griffin@linaro.org/T/
> pw   v3: https://patchwork.kernel.org/project/linux-samsung-soc/cover/20231011184823.443959-1-peter.griffin@linaro.org/
> lore v2: https://lore.kernel.org/all/20231010224928.2296997-1-peter.griffin@linaro.org/
> lore v1: https://lore.kernel.org/linux-arm-kernel/20231005155618.700312-1-peter.griffin@linaro.org/
> 
> Changes since v4:
>  - clk-gs101: order cmu_top by register address, fix incorrect mux widths,
>    add missing mux/div/gates (Andre)
>  - google,gs101.h: add missing space in comment (Andre)
>  - ckl-gs101:google,gs101.h add all remaining gates for cmu_misc and cmu_apm
>  - update pmu dt labels (Krzysztof)
>  - Remove uart16 rts/tx gpio definitions (Krzysztof)
>  - Fixup various dts cosmetic nits (using consts, alignments,
>    names) (Sam/Krzysztof)
>  - Add more specific compatibles for arm cpu's and pmu (Sam)
>  - Use address-cells 1 and ranges property for SoC addresses (Sam)
>  - Encapsulate uart node in USI node (Sam)
>  - Remove earlycon from bootargs
>  - s3c2410_wdt: Reword QUIRK_HAS_DBGACK_BIT docs and add comment (Guenter)
>  - s3c2410_wdt: Reorder DBGACK_MASK functionality first, gs101
>    SoC second (Sam/Krzysztof)
>  
> Changes since v3:
>  - Add reviewed-by and tested-by tags
>  - google,gs101-clock.yaml: move Required to before Allof,
>    enum for cmu*top/misc (Krzysztof)
>  - samsung-wdt.yaml: stick to 80chars (Sam)
>  - google.yaml - remove new line
>  - samsung,pinctrl-wakeup-interrupt: sort alphabetically (RobH)
>  - gs101-oriole.dts: update gpio-keys pinctrl-0 phandle for keys (Stephen)
>  - samsung,exynos-sysreg.yaml - Alphabetical order (RobH)
>  - pinctrl-exynos: update/move comments, slight cosmetic changes (Sam)
>  - samsung_tty.c: update to generic drv_data name/macro (Arnd)
>  - samsung_uart.yaml: make samsung,uart-fifosize required for gs101-uart (Arnd)
>  - pinctrl-exynos: Remove eint irqs from alive pin controller node (Peter/Rob)
>  - Fixup kernel test robot unused const variable warnings
>  - clk-gs101: Update to mout_cmu_eh_bus to CLK_CON_MUX_MUX_CLKCMU_EH_BUS
>    (Chanwoo)
>  - clk-gs101: Update g3aa gout/dout/mout names to g3aa_g3aa for
>    consistency (Chanwoo)
>  - Remove .eint_gpio_init() cb on alive, alive_far, gsacore & gsactrl
>    banks (Sam)
>  - s3c2410_wdt: Drop windowed watchdog mode for now (Peter)
>  - s3c2410_wdt: Separate gs101 SoC support from dbgack feature (Sam)
>  - Move dts to arch/arm64/boot/dts/exynos/google directory (Krzysztof)
> 
> 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 is 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 registered 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 (19):
>   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 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
>   dt-bindings: serial: samsung: Make samsung,uart-fifosize required
>     property
>   clk: samsung: clk-pll: Add support for pll_{0516,0517,518}
>   clk: samsung: clk-gs101: Add cmu_top, cmu_misc and cmu_apm support
>   pinctrl: samsung: Add filter selection support for alive banks
>   pinctrl: samsung: Add gs101 SoC pinctrl configuration
>   watchdog: s3c2410_wdt: Add support for WTCON register DBGACK_MASK bit
>   watchdog: s3c2410_wdt: Add support for Google gs101 SoC
>   tty: serial: samsung: Add gs101 compatible and common
>     fifoszdt_serial_drv_data
>   arm64: dts: exynos: google: Add initial Google gs101 SoC support
>   arm64: dts: exynos: google: Add initial Oriole/pixel 6 board support
>   MAINTAINERS: add entry for Google Tensor SoC
> 
> Tudor Ambarus (1):
>   dt-bindings: soc: samsung: usi: add google,gs101-usi compatible
> 
>  .../devicetree/bindings/arm/google.yaml       |   53 +
>  .../bindings/clock/google,gs101-clock.yaml    |  110 +
>  .../samsung,pinctrl-wakeup-interrupt.yaml     |    2 +
>  .../bindings/pinctrl/samsung,pinctrl.yaml     |    1 +
>  .../bindings/serial/samsung_uart.yaml         |   11 +
>  .../bindings/soc/samsung/exynos-pmu.yaml      |    2 +
>  .../bindings/soc/samsung/exynos-usi.yaml      |    3 +
>  .../soc/samsung/samsung,exynos-sysreg.yaml    |    6 +
>  .../bindings/watchdog/samsung-wdt.yaml        |    8 +-
>  MAINTAINERS                                   |   10 +
>  arch/arm64/boot/dts/exynos/Makefile           |    2 +
>  arch/arm64/boot/dts/exynos/google/Makefile    |    4 +
>  .../boot/dts/exynos/google/gs101-oriole.dts   |  105 +
>  .../boot/dts/exynos/google/gs101-pinctrl.dtsi | 1250 +++++++++
>  .../boot/dts/exynos/google/gs101-pinctrl.h    |   33 +
>  arch/arm64/boot/dts/exynos/google/gs101.dtsi  |  476 ++++
>  drivers/clk/samsung/Makefile                  |    1 +
>  drivers/clk/samsung/clk-gs101.c               | 2495 +++++++++++++++++
>  drivers/clk/samsung/clk-pll.c                 |    6 +
>  drivers/clk/samsung/clk-pll.h                 |    3 +
>  .../pinctrl/samsung/pinctrl-exynos-arm64.c    |  159 ++
>  drivers/pinctrl/samsung/pinctrl-exynos.c      |   91 +-
>  drivers/pinctrl/samsung/pinctrl-exynos.h      |   41 +
>  drivers/pinctrl/samsung/pinctrl-samsung.c     |    4 +
>  drivers/pinctrl/samsung/pinctrl-samsung.h     |   23 +
>  drivers/tty/serial/samsung_tty.c              |   16 +
>  drivers/watchdog/s3c2410_wdt.c                |   74 +-
>  include/dt-bindings/clock/google,gs101.h      |  392 +++
>  28 files changed, 5370 insertions(+), 11 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/exynos/google/Makefile
>  create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
>  create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-pinctrl.dtsi
>  create mode 100644 arch/arm64/boot/dts/exynos/google/gs101-pinctrl.h
>  create mode 100644 arch/arm64/boot/dts/exynos/google/gs101.dtsi
>  create mode 100644 drivers/clk/samsung/clk-gs101.c
>  create mode 100644 include/dt-bindings/clock/google,gs101.h
> 
> -- 
> 2.43.0.rc2.451.g8631bc7472-goog
> 

Thanks Peter for the updated patch series! I've gone through and reviewed the
changes and tested them on my Oriole device. I was able to boot to the busybox
console and verify the appropriate drivers probed.

Great work!

Regards,
Will
André Draszik Dec. 4, 2023, 5:51 p.m. UTC | #2
On Fri, 2023-12-01 at 16:09 +0000, Peter Griffin wrote:
> cmu_top is the top level clock management unit which contains PLLs, muxes,
> dividers and gates that feed the other clock management units.
> 
> cmu_misc clocks IPs such as Watchdog and cmu_apm clocks ips part of the
> APM module.
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Will McVicker <willmcvicker@google.com>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/clk/samsung/Makefile    |    1 +
>  drivers/clk/samsung/clk-gs101.c | 2495 +++++++++++++++++++++++++++++++
>  2 files changed, 2496 insertions(+)
>  create mode 100644 drivers/clk/samsung/clk-gs101.c
> 
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index ebbeacabe88f..3056944a5a54 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7885.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynosautov9.o
> +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-gs101.o
>  obj-$(CONFIG_S3C64XX_COMMON_CLK)	+= clk-s3c64xx.o
>  obj-$(CONFIG_S5PV210_COMMON_CLK)	+= clk-s5pv210.o clk-s5pv210-audss.o
>  obj-$(CONFIG_TESLA_FSD_COMMON_CLK)	+= clk-fsd.o
> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> new file mode 100644
> index 000000000000..6bd233a7ab63
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-gs101.c
> @@ -0,0 +1,2495 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Linaro Ltd.
> + * Author: Peter Griffin <peter.griffin@linaro.org>
> + *
> + * Common Clock Framework support for GS101.
> + */
> [...]
> +
> +/* List of parent clocks for Muxes in CMU_TOP: for CMU_HSI0 */
> +PNAME(mout_cmu_hsi0_usb31drd_p)	= { "oscclk", "dout_shared2_div2" };
> +
> +PNAME(mout_cmu_hsi0_bus_p)	= { "dout_shared0_div4", "dout_shared1_div4",
> +				    "dout_shared2_div2", "dout_shared3_div2",
> +				    "fout_spare_pll" };

This should also be updated....
 
> [...]
> +	MUX(CLK_MOUT_HSI0_BUS, "mout_cmu_hsi0_bus", mout_cmu_hsi0_bus_p,
> +	    CLK_CON_MUX_MUX_CLKCMU_HSI0_BUS, 0, 3),

...because we have 8 possibilities now.

(I didn't check the other parents, but you mentioned you updated field widths
in other registers, too, so maybe need to double check the parent strings as well)


Cheers,
Andre'
André Draszik Dec. 5, 2023, 7:52 a.m. UTC | #3
Hi Pete,

On Fri, 2023-12-01 at 16:09 +0000, Peter Griffin wrote:
> cmu_top is the top level clock management unit which contains PLLs, muxes,
> dividers and gates that feed the other clock management units.
> 
> cmu_misc clocks IPs such as Watchdog and cmu_apm clocks ips part of the
> APM module.
> 
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> Tested-by: Will McVicker <willmcvicker@google.com>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  drivers/clk/samsung/Makefile    |    1 +
>  drivers/clk/samsung/clk-gs101.c | 2495 +++++++++++++++++++++++++++++++
>  2 files changed, 2496 insertions(+)
>  create mode 100644 drivers/clk/samsung/clk-gs101.c
> 
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index ebbeacabe88f..3056944a5a54 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos7885.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynos850.o
>  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-exynosautov9.o
> +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)	+= clk-gs101.o
>  obj-$(CONFIG_S3C64XX_COMMON_CLK)	+= clk-s3c64xx.o
>  obj-$(CONFIG_S5PV210_COMMON_CLK)	+= clk-s5pv210.o clk-s5pv210-audss.o
>  obj-$(CONFIG_TESLA_FSD_COMMON_CLK)	+= clk-fsd.o
> diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> new file mode 100644
> index 000000000000..6bd233a7ab63
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-gs101.c
> @@ -0,0 +1,2495 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Linaro Ltd.
> + * Author: Peter Griffin <peter.griffin@linaro.org>
> + *
> + * Common Clock Framework support for GS101.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/clock/google,gs101.h>
> +
> +#include "clk.h"
> +#include "clk-exynos-arm64.h"
> +
> +/* NOTE: Must be equal to the last clock ID increased by one */
> +#define TOP_NR_CLK	(CLK_GOUT_TPU_UART + 1)
> +#define APM_NR_CLK	(CLK_APM_PLL_DIV16_APM + 1)
> +#define MISC_NR_CLK	(CLK_GOUT_MISC_XIU_D_MISC_IPCLKPORT_ACLK + 1)
> +
> +/* ---- CMU_TOP ------------------------------------------------------------- */
> +
> [...]
> +
> +/* ---- CMU_APM ------------------------------------------------------------- */
> [..]
> +
> +/* ---- CMU_MISC ------------------------------------------------------------- */

nit - the CMU_MISC comment here is an outlier.

> [..]
> +
> +/* ---- platform_driver ----------------------------------------------------- */
> +
> [...]

Cheers,
Andre'
André Draszik Dec. 5, 2023, 8:09 a.m. UTC | #4
Hi Sam,

On Fri, 2023-12-01 at 16:40 -0600, Sam Protsenko wrote:
> On Fri, Dec 1, 2023 at 10:11 AM Peter Griffin <peter.griffin@linaro.org> wrote:
> > 
> > [...]
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_G_DBGCORE_IPCLKPORT_ACLK                     0x20b8
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_G_DBGCORE_IPCLKPORT_PCLK                     0x20bc
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SS_DBGCORE_IPCLKPORT_SS_DBGCORE_IPCLKPORT_HCLK    0x20c0
> 
> As I understand, all those parts like IPCLKPORT, BLK, UID (RSTNSYNC
> probably too) -- they don't really mean anything in the context of the
> driver, just noise. And if you remove those -- there won't be any
> conflicts with other names, because those bits are not the unique
> parts. Following the TRM letter for letter in this case just makes
> things extremely long without adding any value IMHO. For example, that
> name above might be:
> 
>     CLK_CON_GAT_GOUT_APM_SS_DBGCORE_SS_DBGCORE_HCLK
> 
> or even
> 
>     CLK_CON_GAT_GOUT_APM_SS_DBGCORE_HCLK
> 
> would be fine.
> 
> In clk-exynos850 driver I removed all those parts, because they make
> it pretty much impossible to read both the driver and dts. And yeah,
> because those names consequenty lead to very long string names, dts
> will be hard to read too, which is even worse. But again, that's only
> my IMHO.

The advantage of keeping the same name as in the data sheet is that then it's easy to
correlate between driver and hardware and future / other hardware.
By arbitrarily diverging from the TRM you make life much harder for yourself when you
have to look at this again in 12 months time after you've forgotten your name mangling
approach; you also make it harder for everybody to review the driver or to use the
driver as a reference in the future for other hardware, as nobody except you knows
what kind of name-mangling was applied and which register (or clock in this case) is
really meant.


Cheers,
Andre'
Peter Griffin Dec. 8, 2023, 11:35 a.m. UTC | #5
Hi Sam,

Thanks for your review. I've trimmed the non relevant parts of the
email a bit at Krzysztofs request.

On Fri, 1 Dec 2023 at 22:40, Sam Protsenko <semen.protsenko@linaro.org> wrote:
>
> On Fri, Dec 1, 2023 at 10:11 AM Peter Griffin <peter.griffin@linaro.org> wrote:
> >
> > cmu_top is the top level clock management unit which contains PLLs, muxes,
> > dividers and gates that feed the other clock management units.
> >
> > cmu_misc clocks IPs such as Watchdog and cmu_apm clocks ips part of the
> > APM module.
> >
> > Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Tested-by: Will McVicker <willmcvicker@google.com>
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  drivers/clk/samsung/Makefile    |    1 +
> >  drivers/clk/samsung/clk-gs101.c | 2495 +++++++++++++++++++++++++++++++
> >  2 files changed, 2496 insertions(+)
> >  create mode 100644 drivers/clk/samsung/clk-gs101.c
> >
> > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> > index ebbeacabe88f..3056944a5a54 100644
> > --- a/drivers/clk/samsung/Makefile
> > +++ b/drivers/clk/samsung/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK) += clk-exynos7.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)  += clk-exynos7885.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)  += clk-exynos850.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)  += clk-exynosautov9.o
> > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)  += clk-gs101.o
> >  obj-$(CONFIG_S3C64XX_COMMON_CLK)       += clk-s3c64xx.o
> >  obj-$(CONFIG_S5PV210_COMMON_CLK)       += clk-s5pv210.o clk-s5pv210-audss.o
> >  obj-$(CONFIG_TESLA_FSD_COMMON_CLK)     += clk-fsd.o
> > diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> > new file mode 100644
> > index 000000000000..6bd233a7ab63
> > --- /dev/null
> > +++ b/drivers/clk/samsung/clk-gs101.c
> > @@ -0,0 +1,2495 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2023 Linaro Ltd.
> > + * Author: Peter Griffin <peter.griffin@linaro.org>
> > + *
> > + * Common Clock Framework support for GS101.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <dt-bindings/clock/google,gs101.h>
> > +
> > +#include "clk.h"
> > +#include "clk-exynos-arm64.h"
> > +
> > +/* NOTE: Must be equal to the last clock ID increased by one */
> > +#define TOP_NR_CLK     (CLK_GOUT_TPU_UART + 1)
> > +#define APM_NR_CLK     (CLK_APM_PLL_DIV16_APM + 1)
> > +#define MISC_NR_CLK    (CLK_GOUT_MISC_XIU_D_MISC_IPCLKPORT_ACLK + 1)
> > +
>
> Suggest using CLKS_NR_* naming, to follow other drivers for consistency.

Will fix

>
> > +/* ---- CMU_TOP ------------------------------------------------------------- */
> > +
> > +/* Register Offset definitions for CMU_TOP (0x1e080000) */
> > +
> > +#define PLL_LOCKTIME_PLL_SHARED0                       0x0000
> > +#define PLL_LOCKTIME_PLL_SHARED1                       0x0004
> > +#define PLL_LOCKTIME_PLL_SHARED2                       0x0008
> > +#define PLL_LOCKTIME_PLL_SHARED3                       0x000c
<cut>
> > +       DIV(CLK_DOUT_MFC_MFC, "dout_cmu_mfc_mfc", "gout_cmu_mfc_mfc",
> > +           CLK_CON_DIV_CLKCMU_MFC_MFC, 0, 4),
> > +       DIV(CLK_DOUT_MIF_BUSP, "dout_cmu_mif_busp", "gout_cmu_mif_busp",
> > +           CLK_CON_DIV_CLKCMU_MIF_BUSP, 0, 4),
> > +       DIV(CLK_DOUT_MISC_BUS, "dout_cmu_misc_bus", "gout_cmu_misc_bus",
> > +           CLK_CON_DIV_CLKCMU_MISC_BUS, 0, 4),
> > +       DIV(CLK_DOUT_MISC_SSS, "dout_cmu_misc_sss", "gout_cmu_misc_sss",
> > +           CLK_CON_DIV_CLKCMU_MISC_SSS, 0, 4),
> > +       /* CLK_CON_DIV_CLKCMU_OTP lower bits reserved*/
>
> Why the above clock where bits are reserved is declared, but this one
> is not? Also, above comment is marked as TODO, but this one is not.
> And there is no space before */.

Good spot! These two dividers with reserved lower bits should be fixed
factor clocks. I've fixed in v6.

>
> > +       DIV(CLK_DOUT_PDP_BUS, "dout_cmu_pdp_bus", "gout_cmu_pdp_bus",
> > +           CLK_CON_DIV_CLKCMU_PDP_BUS, 0, 4),
> > +       DIV(CLK_DOUT_PDP_VRA, "dout_cmu_pdp_vra", "gout_cmu_pdp_vra",
> > +           CLK_CON_DIV_CLKCMU_PDP_VRA, 0, 4),
> > +       DIV(CLK_DOUT_PERIC0_BUS, "dout_cmu_peric0_bus", "gout_cmu_peric0_bus",
> > +           CLK_CON_DIV_CLKCMU_PERIC0_BUS, 0, 4),
<cut>
> > +       DIV(CLK_DOUT_SHARED1_DIV2, "dout_shared1_div2", "mout_shared1_pll",
> > +           CLK_CON_DIV_PLL_SHARED1_DIV2, 0, 1),
> > +       DIV(CLK_DOUT_SHARED1_DIV3, "dout_shared1_div3", "mout_shared1_pll",
> > +           CLK_CON_DIV_PLL_SHARED1_DIV3, 0, 2),
> > +       DIV(CLK_DOUT_SHARED1_DIV4, "dout_shared1_div4", "mout_shared1_pll",
> > +           CLK_CON_DIV_PLL_SHARED1_DIV4, 0, 1),
> > +       DIV(CLK_DOUT_SHARED2_DIV2, "dout_shared2_div2", "mout_shared2_pll",
> > +           CLK_CON_DIV_PLL_SHARED2_DIV2, 0, 1),
> > +       DIV(CLK_DOUT_SHARED3_DIV2, "dout_shared3_div2", "mout_shared3_pll",
> > +           CLK_CON_DIV_PLL_SHARED3_DIV2, 0, 1),
> > +};
> > +
> > +static const struct samsung_gate_clock cmu_top_gate_clks[] __initconst = {
> > +       GATE(CLK_GOUT_BUS0_BOOST, "gout_cmu_bus0_boost", "mout_cmu_boost_option1",
>
> Please stick to 80 characters length, here and below.

Will fix.

>
> > +            CLK_CON_GAT_CLKCMU_BUS0_BOOST, 21, 0, 0),
> > +       GATE(CLK_GOUT_BUS1_BOOST, "gout_cmu_bus1_boost", "mout_cmu_boost_option1",
> > +            CLK_CON_GAT_CLKCMU_BUS1_BOOST, 21, 0, 0),
> > +       GATE(CLK_GOUT_BUS2_BOOST, "gout_cmu_bus2_boost", "mout_cmu_boost_option1",
> > +            CLK_CON_GAT_CLKCMU_BUS2_BOOST, 21, 0, 0),
> > +       GATE(CLK_GOUT_CORE_BOOST, "gout_cmu_core_boost", "mout_cmu_boost_option1",
> > +            CLK_CON_GAT_CLKCMU_CORE_BOOST, 21, 0, 0),

> > +static void __init gs101_cmu_top_init(struct device_node *np)
> > +{
> > +       exynos_arm64_register_cmu(NULL, np, &top_cmu_info);
> > +}
> > +
> > +/* Register CMU_TOP early, as it's a dependency for other early domains */
> > +CLK_OF_DECLARE(gs101_cmu_top, "google,gs101-cmu-top",
> > +              gs101_cmu_top_init);
> > +
> > +/* ---- CMU_APM ------------------------------------------------------------- */
>
> Suggest adding an empty line here.

Will fix

>
> > +/* Register Offset definitions for CMU_APM (0x17400000) */
> > +#define APM_CMU_APM_CONTROLLER_OPTION                                                  0x0800
> > +#define CLKOUT_CON_BLK_APM_CMU_APM_CLKOUT0                                             0x0810
> > +#define CLK_CON_MUX_MUX_CLKCMU_APM_FUNC                                                        0x1000
> > +#define CLK_CON_MUX_MUX_CLKCMU_APM_FUNCSRC                                             0x1004
> > +#define CLK_CON_DIV_DIV_CLK_APM_BOOST                                                  0x1800
> > +#define CLK_CON_DIV_DIV_CLK_APM_USI0_UART                                              0x1804
> > +#define CLK_CON_DIV_DIV_CLK_APM_USI0_USI                                               0x1808
> > +#define CLK_CON_DIV_DIV_CLK_APM_USI1_UART                                              0x180c
> > +#define CLK_CON_GAT_CLK_BLK_APM_UID_APM_CMU_APM_IPCLKPORT_PCLK                         0x2000
> > +#define CLK_CON_GAT_CLK_BUS0_BOOST_OPTION1                                             0x2004
> > +#define CLK_CON_GAT_CLK_CMU_BOOST_OPTION1                                              0x2008
> > +#define CLK_CON_GAT_CLK_CORE_BOOST_OPTION1                                             0x200c
<cut>
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SPEEDY_APM_IPCLKPORT_PCLK                         0x20a8
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SPEEDY_SUB_APM_IPCLKPORT_PCLK                     0x20ac
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_D_APM_IPCLKPORT_ACLK                         0x20b0
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_D_APM_IPCLKPORT_PCLK                         0x20b4
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_G_DBGCORE_IPCLKPORT_ACLK                     0x20b8
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SSMT_G_DBGCORE_IPCLKPORT_PCLK                     0x20bc
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SS_DBGCORE_IPCLKPORT_SS_DBGCORE_IPCLKPORT_HCLK    0x20c0
>
> As I understand, all those parts like IPCLKPORT, BLK, UID (RSTNSYNC
> probably too) -- they don't really mean anything in the context of the
> driver, just noise. And if you remove those -- there won't be any
> conflicts with other names, because those bits are not the unique
> parts. Following the TRM letter for letter in this case just makes
> things extremely long without adding any value IMHO. For example, that
> name above might be:
>
>     CLK_CON_GAT_GOUT_APM_SS_DBGCORE_SS_DBGCORE_HCLK
>
> or even
>
>     CLK_CON_GAT_GOUT_APM_SS_DBGCORE_HCLK
>
> would be fine.
>
> In clk-exynos850 driver I removed all those parts, because they make
> it pretty much impossible to read both the driver and dts. And yeah,
> because those names consequenty lead to very long string names, dts
> will be hard to read too, which is even worse. But again, that's only
> my IMHO.

I would like to keep the register names unmodified, as I mentioned
previously mangling them makes checking with the datasheet much
harder. As the name in the header and string are already mangled a bit
I'm OK with mangling that a bit more.

With that in mind I've done the following mangling in v6: -

Register name: CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_GPIO_ALIVE_IPCLKPORT_PCLK

Replace `CLK_CON_GAT_GOUT_BLK_<blockname>_UID_` with `CLK_GOUT_<blockname>_`
Replace ` _IPCLKPORT` with nothing
Replace `_RSTNSYNC` with nothing

So you end up with:

GATE(CLK_GOUT_APM_APBIF_GPIO_ALIVE_PCLK,
            "gout_apm_apbif_gpio_alive_pclk", "gout_apm_func",
             CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_GPIO_ALIVE_IPCLKPORT_PCLK,
             21, 0, 0),

That still enables cross referencing with the datasheet via the
unmodified register name, but the dt binding define and string are
slightly shorter like you wanted. The mangling described above the
vast majority of clock names look fine. There are a couple of
anomalies though like  `CLK_GOUT_MISC_MISC_CMU_MISC_PCLK` and
`CLK_GOUT_APM_APM_CMU_APM_PCLK`. I think it is a *really* bad idea to
do custom mangling on these specific names though.

>
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SYSMMU_D_APM_IPCLKPORT_CLK_S2                     0x20c4
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_SYSREG_APM_IPCLKPORT_PCLK                         0x20cc
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_APM_IPCLKPORT_ACLK                           0x20d0
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_APM_IPCLKPORT_PCLK                           0x20d4
> > +#define CLK_CON_GAT_GOUT_BLK_APM_UID_UASC_DBGCORE_IPCLKPORT_ACLK                       0x20d8

<cut>

> > +       GATE(CLK_GOUT_APM_APBIF_RTC_IPCLKPORT_PCLK,
> > +            "gout_apm_apbif_rtc_ipclkport_pclk", "gout_apm_func",
> > +            CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_RTC_IPCLKPORT_PCLK, 21, 0, 0),
> > +       GATE(CLK_GOUT_APM_APBIF_TRTC_IPCLKPORT_PCLK,
> > +            "gout_apm_apbif_trtc_ipclkport_pclk", "gout_apm_func",
> > +            CLK_CON_GAT_GOUT_BLK_APM_UID_APBIF_TRTC_IPCLKPORT_PCLK, 21, 0, 0),
> > +       GATE(CLK_GOUT_APM_APM_USI0_UART_IPCLKPORT_PCLK,
> > +            "gout_apm_apm_usi0_uart_ipclkport_pclk", "gout_apm_func",
> > +            CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_UART_IPCLKPORT_PCLK,
> > +            21, 0, 0),
> > +       GATE(CLK_GOUT_APM_APM_USI0_USI_IPCLKPORT_IPCLK,
> > +            "gout_apm_apm_usi0_usi_ipclkport_ipclk", "dout_apm_usi0_uart",
> > +            CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_USI_IPCLKPORT_PCLK,
>
> PCLK vs IPCLK?

Will fix. The order here should match the register offsets

CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_UART_IPCLKPORT_IPCLK,
CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_UART_IPCLKPORT_PCLK,
CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_USI_IPCLKPORT_IPCLK,
CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI0_USI_IPCLKPORT_PCLK,
CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_IPCLK,
CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_PCLK,

>
> > +            21, 0, 0),
> > +       GATE(CLK_GOUT_APM_APM_USI0_USI_IPCLKPORT_PCLK,
> > +            "gout_apm_apm_usi0_usi_ipclkport_pclk", "gout_apm_func",
> > +            CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_PCLK, 21, 0, 0),
>
> Should it be USI0 instead of USI1? It's the same as the definition
> below. Also, please stick to 80 characters per line, here and below.

I've revisited the 6 clock gates described above to ensure they are in
register offset order.

>
> > +       GATE(CLK_GOUT_APM_APM_USI1_UART_IPCLKPORT_IPCLK,
> > +            "gout_apm_apm_usi1_uart_ipclkport_ipclk", "dout_apm_usi1_uart",
> > +            CLK_CON_GAT_GOUT_BLK_APM_UID_APM_USI1_UART_IPCLKPORT_IPCLK, 21, 0, 0),
> > +       GATE(CLK_GOUT_APM_APM_USI1_UART_IPCLKPORT_PCLK,
> > +            "gout_apm_apm_usi1_uart_ipclkport_pclk", "gout_apm_func",

<cut>

> > +static const struct samsung_cmu_info apm_cmu_info __initconst = {
> > +       .mux_clks               = apm_mux_clks,
> > +       .nr_mux_clks            = ARRAY_SIZE(apm_mux_clks),
> > +       .div_clks               = apm_div_clks,
> > +       .nr_div_clks            = ARRAY_SIZE(apm_div_clks),
> > +       .gate_clks              = apm_gate_clks,
> > +       .nr_gate_clks           = ARRAY_SIZE(apm_gate_clks),
> > +       .fixed_clks             = apm_fixed_clks,
> > +       .nr_fixed_clks          = ARRAY_SIZE(apm_fixed_clks),
> > +       .nr_clk_ids             = APM_NR_CLK,
> > +       .clk_regs               = apm_clk_regs,
> > +       .nr_clk_regs            = ARRAY_SIZE(apm_clk_regs),
> > +};
> > +
> > +/* ---- CMU_MISC ------------------------------------------------------------- */
>
> Suggest adding an empty line here.

Will fix
>
> > +/* Register Offset definitions for CMU_MISC (0x10010000) */
> > +#define PLL_CON0_MUX_CLKCMU_MISC_BUS_USER      0x0600
> > +#define PLL_CON1_MUX_CLKCMU_MISC_BUS_USER      0x0604
> > +#define PLL_CON0_MUX_CLKCMU_MISC_SSS_USER      0x0610
> > +#define PLL_CON1_MUX_CLKCMU_MISC_SSS_USER      0x0614
> > +#define MISC_CMU_MISC_CONTROLLER_OPTION                0x0800
> > +#define CLKOUT_CON_BLK_MISC_CMU_MISC_CLKOUT0   0x0810
> > +#define CLK_CON_MUX_MUX_CLK_MISC_GIC           0x1000
> > +#define CLK_CON_DIV_DIV_CLK_MISC_BUSP          0x1800
> > +#define CLK_CON_DIV_DIV_CLK_MISC_GIC           0x1804
>
> Please align all values for this group at the same indentation level.

Will fix

>
> > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_MISC_CMU_MISC_IPCLKPORT_PCLK              0x2000
> > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BIRA_IPCLKPORT_I_OSCCLK           0x2004
> > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BISR_IPCLKPORT_I_OSCCLK           0x2008
> > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_TOP_IPCLKPORT_I_OSCCLK            0x200c
> > +#define CLK_CON_GAT_CLK_BLK_MISC_UID_RSTNSYNC_CLK_MISC_OSCCLK_IPCLKPORT_CLK    0x2010
>
> Just want to give you one more example for my rant above. Look how
> much easier it is to understand this name (than above one):
>
>     CLK_CON_GAT_MISC_OSCCLK_CLK
>
> Which also turns this (public API!):
>
>     "gout_misc_rstnsync_clk_misc_oscclk_ipclkport_clk"
>
> into this:
>
>     "gout_misc_oscclk_clk"

In v6 this becomes

GATE(CLK_GOUT_MISC_CLK_MISC_OSCCLK_CLK,
     "gout_misc_clk_misc_oscclk_clk", "dout_misc_busp",
     CLK_CON_GAT_CLK_BLK_MISC_UID_RSTNSYNC_CLK_MISC_OSCCLK_IPCLKPORT_CLK,
     21, 0, 0),

<cut>
>
> > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_ADM_AHB_SSS_IPCLKPORT_HCLKM              0x2014
> > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_AD_APB_DIT_IPCLKPORT_PCLKM               0x2018
> > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_AD_APB_PUF_IPCLKPORT_PCLKM               0x201c
> > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_DIT_IPCLKPORT_ICLKL2A                    0x2020
> > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_D_TZPC_MISC_IPCLKPORT_PCLK               0x2024
> > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_GIC_IPCLKPORT_GICCLK                     0x2028
> > +#define CLK_CON_GAT_GOUT_BLK_MISC_UID_GPC_MISC_IPCLKPORT_PCLK                  0x202c

<cut>

> > +static const struct samsung_div_clock misc_div_clks[] __initconst = {
> > +       DIV(CLK_DOUT_MISC_BUSP, "dout_misc_busp", "mout_misc_bus_user",
> > +           CLK_CON_DIV_DIV_CLK_MISC_BUSP, 0, 3),
> > +       DIV(CLK_DOUT_MISC_GIC, "dout_misc_gic", "mout_misc_bus_user",
> > +           CLK_CON_DIV_DIV_CLK_MISC_GIC, 0, 3),
> > +};
> > +
> > +static const struct samsung_gate_clock misc_gate_clks[] __initconst = {
> > +       GATE(CLK_GOUT_MISC_MISC_CMU_MISC_IPCLKPORT_PCLK,
> > +            "gout_misc_ipclkport_pclk", "dout_misc_busp",
>
> So if you want to be consistent, it should be
> "gout_misc_misc_misc_....". My point is -- Samsung's naming in TRM is
> insane. They wanted to stick to some very detailed and unified naming
> schema (or just generated those names from some internal clock tree
> data), granted. But I'm just not sure if it's the best idea to follow
> it in driver's code.

With the naming described above it becomes
"gout_misc_misc_cmu_misc_pclk". Not ideal, and this is one of the
clocks I mentioned above. The overwhelming majority of the clock names
don't have such repetition in their name though. If we start doing
custom mangling on specific clocks it is going to become a nightmare
to track. As I mentioned previously there are thousands of clocks in
the SoC. Most upstream platforms only seem to implement a small subset
of clocks in each CMU which maybe makes it easier to track. Our goal
though is to have full functionality upstream.

>
> > +            CLK_CON_GAT_CLK_BLK_MISC_UID_MISC_CMU_MISC_IPCLKPORT_PCLK,
> > +            21, 0, 0),
> > +       GATE(CLK_GOUT_MISC_OTP_CON_BIRA_IPCLKPORT_I_OSCCLK,
> > +            "gout_misc_otp_con_bira_ipclkport_i_oscclk", "dout_misc_busp",
> > +            CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BIRA_IPCLKPORT_I_OSCCLK,
> > +            21, 0, 0),
> > +       GATE(CLK_GOUT_MISC_OTP_CON_BISR_IPCLKPORT_I_OSCCLK,
> > +            "gout_misc_otp_con_bisr_ipclkport_i_oscclk", "dout_misc_busp",
> > +            CLK_CON_GAT_CLK_BLK_MISC_UID_OTP_CON_BISR_IPCLKPORT_I_OSCCLK,
> > +            21, 0, 0),
<snip>
> > +       GATE(CLK_GOUT_MISC_RTIC_IPCLKPORT_I_ACLK,
> > +            "gout_misc_rtic_ipclkport_i_aclk", "dout_misc_busp",
> > +            CLK_CON_GAT_GOUT_BLK_MISC_UID_RTIC_IPCLKPORT_I_ACLK,
> > +            21, 0, 0),
> > +       GATE(CLK_GOUT_MISC_RTIC_IPCLKPORT_I_PCLK, "gout_misc_rtic_ipclkport_i_pclk",
>
> 80 characters per line please.

Will fix

regards,

Peter
Peter Griffin Dec. 8, 2023, 2:27 p.m. UTC | #6
Hi André,

Thanks for the review

On Mon, 4 Dec 2023 at 17:51, André Draszik <andre.draszik@linaro.org> wrote:
>
> On Fri, 2023-12-01 at 16:09 +0000, Peter Griffin wrote:
> > cmu_top is the top level clock management unit which contains PLLs, muxes,
> > dividers and gates that feed the other clock management units.
> >
> > cmu_misc clocks IPs such as Watchdog and cmu_apm clocks ips part of the
> > APM module.
> >
> > Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Tested-by: Will McVicker <willmcvicker@google.com>
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  drivers/clk/samsung/Makefile    |    1 +
> >  drivers/clk/samsung/clk-gs101.c | 2495 +++++++++++++++++++++++++++++++
> >  2 files changed, 2496 insertions(+)
> >  create mode 100644 drivers/clk/samsung/clk-gs101.c
> >
> > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> > index ebbeacabe88f..3056944a5a54 100644
> > --- a/drivers/clk/samsung/Makefile
> > +++ b/drivers/clk/samsung/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)       += clk-exynos7.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos7885.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos850.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynosautov9.o
> > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-gs101.o
> >  obj-$(CONFIG_S3C64XX_COMMON_CLK)     += clk-s3c64xx.o
> >  obj-$(CONFIG_S5PV210_COMMON_CLK)     += clk-s5pv210.o clk-s5pv210-audss.o
> >  obj-$(CONFIG_TESLA_FSD_COMMON_CLK)   += clk-fsd.o
> > diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> > new file mode 100644
> > index 000000000000..6bd233a7ab63
> > --- /dev/null
> > +++ b/drivers/clk/samsung/clk-gs101.c
> > @@ -0,0 +1,2495 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2023 Linaro Ltd.
> > + * Author: Peter Griffin <peter.griffin@linaro.org>
> > + *
> > + * Common Clock Framework support for GS101.
> > + */
> > [...]
> > +
> > +/* List of parent clocks for Muxes in CMU_TOP: for CMU_HSI0 */
> > +PNAME(mout_cmu_hsi0_usb31drd_p)      = { "oscclk", "dout_shared2_div2" };
> > +
> > +PNAME(mout_cmu_hsi0_bus_p)   = { "dout_shared0_div4", "dout_shared1_div4",
> > +                                 "dout_shared2_div2", "dout_shared3_div2",
> > +                                 "fout_spare_pll" };
>
> This should also be updated....
>
> > [...]
> > +     MUX(CLK_MOUT_HSI0_BUS, "mout_cmu_hsi0_bus", mout_cmu_hsi0_bus_p,
> > +         CLK_CON_MUX_MUX_CLKCMU_HSI0_BUS, 0, 3),
>
> ...because we have 8 possibilities now.

Interesting, unfortunately there is some discrepancy between the
documentation again :( All the cmu_top clock parents were authored
using the cmu_diagrams which only shows the 5 parents listed above.
Checking the mux register definition it lists 5-7 as being oscclk
5=osclk
6=osclk
7=oscclk

Downstream clock implementation lists these oscclk 5-7 as well, so I
guess we should add them...sigh

> (I didn't check the other parents, but you mentioned you updated field widths
> in other registers, too, so maybe need to double check the parent strings as well)

Yes I will go through and re-check these parent names again.

Peter
Peter Griffin Dec. 8, 2023, 9:14 p.m. UTC | #7
Hi André

On Tue, 5 Dec 2023 at 07:52, André Draszik <andre.draszik@linaro.org> wrote:
>
> Hi Pete,
>
> On Fri, 2023-12-01 at 16:09 +0000, Peter Griffin wrote:
> > cmu_top is the top level clock management unit which contains PLLs, muxes,
> > dividers and gates that feed the other clock management units.
> >
> > cmu_misc clocks IPs such as Watchdog and cmu_apm clocks ips part of the
> > APM module.
> >
> > Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> > Tested-by: Will McVicker <willmcvicker@google.com>
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> >  drivers/clk/samsung/Makefile    |    1 +
> >  drivers/clk/samsung/clk-gs101.c | 2495 +++++++++++++++++++++++++++++++
> >  2 files changed, 2496 insertions(+)
> >  create mode 100644 drivers/clk/samsung/clk-gs101.c
> >
> > diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> > index ebbeacabe88f..3056944a5a54 100644
> > --- a/drivers/clk/samsung/Makefile
> > +++ b/drivers/clk/samsung/Makefile
> > @@ -21,6 +21,7 @@ obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)       += clk-exynos7.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos7885.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynos850.o
> >  obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-exynosautov9.o
> > +obj-$(CONFIG_EXYNOS_ARM64_COMMON_CLK)        += clk-gs101.o
> >  obj-$(CONFIG_S3C64XX_COMMON_CLK)     += clk-s3c64xx.o
> >  obj-$(CONFIG_S5PV210_COMMON_CLK)     += clk-s5pv210.o clk-s5pv210-audss.o
> >  obj-$(CONFIG_TESLA_FSD_COMMON_CLK)   += clk-fsd.o
> > diff --git a/drivers/clk/samsung/clk-gs101.c b/drivers/clk/samsung/clk-gs101.c
> > new file mode 100644
> > index 000000000000..6bd233a7ab63
> > --- /dev/null
> > +++ b/drivers/clk/samsung/clk-gs101.c
> > @@ -0,0 +1,2495 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2023 Linaro Ltd.
> > + * Author: Peter Griffin <peter.griffin@linaro.org>
> > + *
> > + * Common Clock Framework support for GS101.
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <dt-bindings/clock/google,gs101.h>
> > +
> > +#include "clk.h"
> > +#include "clk-exynos-arm64.h"
> > +
> > +/* NOTE: Must be equal to the last clock ID increased by one */
> > +#define TOP_NR_CLK   (CLK_GOUT_TPU_UART + 1)
> > +#define APM_NR_CLK   (CLK_APM_PLL_DIV16_APM + 1)
> > +#define MISC_NR_CLK  (CLK_GOUT_MISC_XIU_D_MISC_IPCLKPORT_ACLK + 1)
> > +
> > +/* ---- CMU_TOP ------------------------------------------------------------- */
> > +
> > [...]
> > +
> > +/* ---- CMU_APM ------------------------------------------------------------- */
> > [..]
> > +
> > +/* ---- CMU_MISC ------------------------------------------------------------- */
>
> nit - the CMU_MISC comment here is an outlier.

Will fix.

Peter.