mbox series

[00/21] Adding support of ADI ARMv8 ADSP-SC598 SoC.

Message ID 20240912-test-v1-0-458fa57c8ccf@analog.com
Headers show
Series Adding support of ADI ARMv8 ADSP-SC598 SoC. | expand

Message

Arturs Artamonovs via B4 Relay Sept. 12, 2024, 6:24 p.m. UTC
This set of patches based on ADI fork of Linux Kerenl that support family of ADSP-SC5xx
SoC's and used by customers for some time . Patch series contains minimal set
of changes to add ADSP-SC598 support to upstream kernel. This series include
UART,I2C,IRQCHIP,RCU drivers and device-tree to be able boot on EV-SC598-SOM
board into serial shell and able to reset the board. Current SOM board
requires I2C expander to enable UART output.

UART,I2C and PINCTRL drivers are based on old Blackfin drivers with
ADSP-SC5xx related bug fixes and improvments.

Signed-off-by: Arturs Artamonovs <arturs.artamonovs@analog.com>
---
Arturs Artamonovs (21):
      arm64: Add ADI ADSP-SC598 SoC
      reset: Add driver for ADI ADSP-SC5xx reset controller
      dt-bindigs: arm64: adi,sc598 bindings
      dt-bindings: arm64: adi,sc598: Add ADSP-SC598 SoC bindings
      clock:Add driver for ADI ADSP-SC5xx PLL
      include: dt-binding: clock: add adi clock header file
      clock: Add driver for ADI ADSP-SC5xx clock
      dt-bindings: clock: adi,sc5xx-clocks: add bindings
      gpio: add driver for ADI ADSP-SC5xx platform
      dt-bindings: gpio: adi,adsp-port-gpio: add bindings
      irqchip: Add irqchip for ADI ADSP-SC5xx platform
      dt-bindings: irqchip: adi,adsp-pint: add binding
      pinctrl: Add drivers for ADI ADSP-SC5xx platform
      dt-bindings: pinctrl: adi,adsp-pinctrl: add bindings
      i2c: Add driver for ADI ADSP-SC5xx platforms
      dt-bindings: i2c: add i2c/twi driver documentation
      serial: adi,uart: Add driver for ADI ADSP-SC5xx
      dt-bindings: serial: adi,uart4: add adi,uart4 driver documentation
      arm64: dts: adi: sc598: add device tree
      arm64: defconfig: sc598 add minimal changes
      MAINTAINERS: add adi sc5xx maintainers

 .../devicetree/bindings/arm/analog/adi,sc5xx.yaml  |   24 +
 .../bindings/clock/adi,sc5xx-clocks.yaml           |   65 ++
 .../bindings/gpio/adi,adsp-port-gpio.yaml          |   69 ++
 Documentation/devicetree/bindings/i2c/adi,twi.yaml |   71 ++
 .../interrupt-controller/adi,adsp-pint.yaml        |   51 +
 .../bindings/pinctrl/adi,adsp-pinctrl.yaml         |   83 ++
 .../devicetree/bindings/serial/adi,uart.yaml       |   85 ++
 .../bindings/soc/adi/adi,reset-controller.yaml     |   38 +
 MAINTAINERS                                        |   22 +
 arch/arm64/Kconfig.platforms                       |   13 +
 arch/arm64/boot/dts/Makefile                       |    1 +
 arch/arm64/boot/dts/adi/Makefile                   |    2 +
 arch/arm64/boot/dts/adi/sc598-som-ezkit.dts        |   14 +
 arch/arm64/boot/dts/adi/sc598-som.dtsi             |   58 ++
 arch/arm64/boot/dts/adi/sc59x-64.dtsi              |  367 +++++++
 arch/arm64/configs/defconfig                       |    6 +
 drivers/clk/Kconfig                                |    9 +
 drivers/clk/Makefile                               |    1 +
 drivers/clk/adi/Makefile                           |    4 +
 drivers/clk/adi/clk-adi-pll.c                      |  151 +++
 drivers/clk/adi/clk-adi-sc598.c                    |  329 ++++++
 drivers/clk/adi/clk.h                              |   99 ++
 drivers/gpio/Kconfig                               |    8 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-adi-adsp-port.c                  |  145 +++
 drivers/i2c/busses/Kconfig                         |   17 +
 drivers/i2c/busses/Makefile                        |    1 +
 drivers/i2c/busses/i2c-adi-twi.c                   |  940 ++++++++++++++++++
 drivers/irqchip/Kconfig                            |    9 +
 drivers/irqchip/Makefile                           |    2 +
 drivers/irqchip/irq-adi-adsp.c                     |  310 ++++++
 drivers/pinctrl/Kconfig                            |   12 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/pinctrl-adsp.c                     |  919 +++++++++++++++++
 drivers/reset/Makefile                             |    1 +
 drivers/soc/Makefile                               |    1 +
 drivers/soc/adi/Makefile                           |    5 +
 drivers/soc/adi/system.c                           |  257 +++++
 drivers/tty/serial/Kconfig                         |   19 +-
 drivers/tty/serial/Makefile                        |    1 +
 drivers/tty/serial/adi_uart.c                      | 1045 ++++++++++++++++++++
 include/dt-bindings/clock/adi-sc5xx-clock.h        |   93 ++
 include/dt-bindings/pinctrl/adi-adsp.h             |   19 +
 include/linux/soc/adi/adsp-gpio-port.h             |   85 ++
 include/linux/soc/adi/cpu.h                        |  107 ++
 include/linux/soc/adi/rcu.h                        |   55 ++
 include/linux/soc/adi/sc59x.h                      |  147 +++
 include/linux/soc/adi/system_config.h              |   65 ++
 include/uapi/linux/serial_core.h                   |    3 +
 49 files changed, 5829 insertions(+), 1 deletion(-)
---
base-commit: da3ea35007d0af457a0afc87e84fddaebc4e0b63
change-id: 20240909-test-8ec5f76fe6d2

Best regards,

Comments

Rob Herring (Arm) Sept. 12, 2024, 8:02 p.m. UTC | #1
On Thu, 12 Sep 2024 19:25:03 +0100, Arturs Artamonovs wrote:
> Add serial driver bindings.
> 
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
> Signed-off-by: Utsav Agarwal <Utsav.Agarwal@analog.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
> ---
>  .../devicetree/bindings/serial/adi,uart.yaml       | 85 ++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/adi,uart.yaml: $id: Cannot determine base path from $id, relative path/filename doesn't match actual path or filename
 	 $id: http://devicetree.org/schemas/serial/adi,uart4.yaml
 	file: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/serial/adi,uart.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240912-test-v1-18-458fa57c8ccf@analog.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring (Arm) Sept. 12, 2024, 9:04 p.m. UTC | #2
On Thu, 12 Sep 2024 19:24:45 +0100, Arturs Artamonovs wrote:
> This set of patches based on ADI fork of Linux Kerenl that support family of ADSP-SC5xx
> SoC's and used by customers for some time . Patch series contains minimal set
> of changes to add ADSP-SC598 support to upstream kernel. This series include
> UART,I2C,IRQCHIP,RCU drivers and device-tree to be able boot on EV-SC598-SOM
> board into serial shell and able to reset the board. Current SOM board
> requires I2C expander to enable UART output.
> 
> UART,I2C and PINCTRL drivers are based on old Blackfin drivers with
> ADSP-SC5xx related bug fixes and improvments.
> 
> Signed-off-by: Arturs Artamonovs <arturs.artamonovs@analog.com>
> ---
> Arturs Artamonovs (21):
>       arm64: Add ADI ADSP-SC598 SoC
>       reset: Add driver for ADI ADSP-SC5xx reset controller
>       dt-bindigs: arm64: adi,sc598 bindings
>       dt-bindings: arm64: adi,sc598: Add ADSP-SC598 SoC bindings
>       clock:Add driver for ADI ADSP-SC5xx PLL
>       include: dt-binding: clock: add adi clock header file
>       clock: Add driver for ADI ADSP-SC5xx clock
>       dt-bindings: clock: adi,sc5xx-clocks: add bindings
>       gpio: add driver for ADI ADSP-SC5xx platform
>       dt-bindings: gpio: adi,adsp-port-gpio: add bindings
>       irqchip: Add irqchip for ADI ADSP-SC5xx platform
>       dt-bindings: irqchip: adi,adsp-pint: add binding
>       pinctrl: Add drivers for ADI ADSP-SC5xx platform
>       dt-bindings: pinctrl: adi,adsp-pinctrl: add bindings
>       i2c: Add driver for ADI ADSP-SC5xx platforms
>       dt-bindings: i2c: add i2c/twi driver documentation
>       serial: adi,uart: Add driver for ADI ADSP-SC5xx
>       dt-bindings: serial: adi,uart4: add adi,uart4 driver documentation
>       arm64: dts: adi: sc598: add device tree
>       arm64: defconfig: sc598 add minimal changes
>       MAINTAINERS: add adi sc5xx maintainers
> 
>  .../devicetree/bindings/arm/analog/adi,sc5xx.yaml  |   24 +
>  .../bindings/clock/adi,sc5xx-clocks.yaml           |   65 ++
>  .../bindings/gpio/adi,adsp-port-gpio.yaml          |   69 ++
>  Documentation/devicetree/bindings/i2c/adi,twi.yaml |   71 ++
>  .../interrupt-controller/adi,adsp-pint.yaml        |   51 +
>  .../bindings/pinctrl/adi,adsp-pinctrl.yaml         |   83 ++
>  .../devicetree/bindings/serial/adi,uart.yaml       |   85 ++
>  .../bindings/soc/adi/adi,reset-controller.yaml     |   38 +
>  MAINTAINERS                                        |   22 +
>  arch/arm64/Kconfig.platforms                       |   13 +
>  arch/arm64/boot/dts/Makefile                       |    1 +
>  arch/arm64/boot/dts/adi/Makefile                   |    2 +
>  arch/arm64/boot/dts/adi/sc598-som-ezkit.dts        |   14 +
>  arch/arm64/boot/dts/adi/sc598-som.dtsi             |   58 ++
>  arch/arm64/boot/dts/adi/sc59x-64.dtsi              |  367 +++++++
>  arch/arm64/configs/defconfig                       |    6 +
>  drivers/clk/Kconfig                                |    9 +
>  drivers/clk/Makefile                               |    1 +
>  drivers/clk/adi/Makefile                           |    4 +
>  drivers/clk/adi/clk-adi-pll.c                      |  151 +++
>  drivers/clk/adi/clk-adi-sc598.c                    |  329 ++++++
>  drivers/clk/adi/clk.h                              |   99 ++
>  drivers/gpio/Kconfig                               |    8 +
>  drivers/gpio/Makefile                              |    1 +
>  drivers/gpio/gpio-adi-adsp-port.c                  |  145 +++
>  drivers/i2c/busses/Kconfig                         |   17 +
>  drivers/i2c/busses/Makefile                        |    1 +
>  drivers/i2c/busses/i2c-adi-twi.c                   |  940 ++++++++++++++++++
>  drivers/irqchip/Kconfig                            |    9 +
>  drivers/irqchip/Makefile                           |    2 +
>  drivers/irqchip/irq-adi-adsp.c                     |  310 ++++++
>  drivers/pinctrl/Kconfig                            |   12 +
>  drivers/pinctrl/Makefile                           |    1 +
>  drivers/pinctrl/pinctrl-adsp.c                     |  919 +++++++++++++++++
>  drivers/reset/Makefile                             |    1 +
>  drivers/soc/Makefile                               |    1 +
>  drivers/soc/adi/Makefile                           |    5 +
>  drivers/soc/adi/system.c                           |  257 +++++
>  drivers/tty/serial/Kconfig                         |   19 +-
>  drivers/tty/serial/Makefile                        |    1 +
>  drivers/tty/serial/adi_uart.c                      | 1045 ++++++++++++++++++++
>  include/dt-bindings/clock/adi-sc5xx-clock.h        |   93 ++
>  include/dt-bindings/pinctrl/adi-adsp.h             |   19 +
>  include/linux/soc/adi/adsp-gpio-port.h             |   85 ++
>  include/linux/soc/adi/cpu.h                        |  107 ++
>  include/linux/soc/adi/rcu.h                        |   55 ++
>  include/linux/soc/adi/sc59x.h                      |  147 +++
>  include/linux/soc/adi/system_config.h              |   65 ++
>  include/uapi/linux/serial_core.h                   |    3 +
>  49 files changed, 5829 insertions(+), 1 deletion(-)
> ---
> base-commit: da3ea35007d0af457a0afc87e84fddaebc4e0b63
> change-id: 20240909-test-8ec5f76fe6d2
> 
> Best regards,
> --
> Arturs Artamonovs <arturs.artamonovs@analog.com>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y adi/sc598-som-ezkit.dtb' for 20240912-test-v1-0-458fa57c8ccf@analog.com:

arch/arm64/boot/dts/adi/sc598-som-ezkit.dtb: /scb-bus/sec@31089000: failed to match any schema with compatible: ['adi,system-event-controller']
Arnd Bergmann Sept. 13, 2024, 7:22 a.m. UTC | #3
On Thu, Sep 12, 2024, at 18:24, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@analog.com>
>
> Adding support for ADI ADSP reset controller. This driver allows
> trigger a software reset.
>
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
> Co-developed-by: Utsav Agarwal <Utsav.Agarwal@analog.com>
> Signed-off-by: Utsav Agarwal <Utsav.Agarwal@analog.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
> ---
>  drivers/reset/Makefile | 1 +
>  1 file changed, 1 insertion(+)

It looks like you accidentally dropped the actual driver during
a rebase, this is only the Makefile change.

       Arnd
Arnd Bergmann Sept. 13, 2024, 7:24 a.m. UTC | #4
On Thu, Sep 12, 2024, at 18:25, Arturs Artamonovs via B4 Relay wrote:
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,twi
> +

The "adi,twi" string is not specific enough to identify a particular
implementation, the name should either include the version of the
IP block that was used in each chip, or (if that is not public
knowledge) the identifier of the chip it was integrated in.

      Arnd
Arnd Bergmann Sept. 13, 2024, 7:35 a.m. UTC | #5
On Thu, Sep 12, 2024, at 18:24, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@analog.com>
>
> Add adi clock driver header file

Are you sure this is necessary? If the clk controller follows
a logical structure, it's usually easier to identify individual
clks by the way the hardware is laid out.

> +#ifndef DT_BINDINGS_CLOCK_ADI_SC5XX_CLOCK_H
> +#define DT_BINDINGS_CLOCK_ADI_SC5XX_CLOCK_H
> +
> +#define ADSP_SC598_CLK_DUMMY 0
> +#define ADSP_SC598_CLK_SYS_CLKIN0 1
> +#define ADSP_SC598_CLK_SYS_CLKIN1 2
> +#define ADSP_SC598_CLK_CGU0_PLL_IN 3
> +#define ADSP_SC598_CLK_CGU0_VCO_OUT 4

Unlike the DT compatible strings, these #defines don't have
to be specific to a particular SoC, you could just reuse them
for a family of chips even if they each use a slightly different
subset. Maybe name them "ADSP_CLK_*" or "ADSP_SC5XX_CLK_*"?

> +#define ADSP_SC598_CLK_END 80

This should not be part of the binding, in particular you
probably want to be able to extend this in order to support
additional chips.

      Arnd
Arnd Bergmann Sept. 13, 2024, 7:44 a.m. UTC | #6
On Thu, Sep 12, 2024, at 18:25, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@analog.com>
>
> Add ADSP-SC598 defconfig
>
> @@ -513,6 +515,7 @@ CONFIG_TCG_TIS_I2C_CR50=m
>  CONFIG_TCG_TIS_I2C_INFINEON=y
>  CONFIG_I2C_CHARDEV=y
>  CONFIG_I2C_MUX=y
> +CONFIG_I2C_ADI_TWI=y
>  CONFIG_I2C_MUX_PCA954x=y
>  CONFIG_I2C_BCM2835=m
>  CONFIG_I2C_CADENCE=m
> @@ -539,6 +542,7 @@ CONFIG_I2C_UNIPHIER_F=y
>  CONFIG_I2C_RCAR=y
>  CONFIG_I2C_CROS_EC_TUNNEL=y
>  CONFIG_SPI=y
> +CONFIG_SPI_ADI=y
>  CONFIG_SPI_ARMADA_3700=y
>  CONFIG_SPI_BCM2835=m
>  CONFIG_SPI_BCM2835AUX=m

These are usually not required for booting and should be
made loadable modules if possible.

      Arnd
Arnd Bergmann Sept. 13, 2024, 8:16 a.m. UTC | #7
On Thu, Sep 12, 2024, at 18:24, Arturs Artamonovs via B4 Relay wrote:
> From: Arturs Artamonovs <arturs.artamonovs@analog.com>
>
> Add ADSP-SC598 platform.
>

> --- a/arch/arm64/Kconfig.platforms
> +++ b/arch/arm64/Kconfig.platforms
> @@ -292,6 +292,19 @@ config ARCH_ROCKCHIP
>  	  This enables support for the ARMv8 based Rockchip chipsets,
>  	  like the RK3368.
> 
> +config ARCH_SC59X_64
> +	bool "ADI 64-bit SC59X Platforms"
> +	select TIMER_OF
> +	select GPIOLIB
> +	select PINCTRL
> +	select COMMON_CLK_ADI_SC598
> +	select PINCTRL_ADSP
> +	select ADI_ADSP_IRQ
> +	select COUNTER

You can remove the 'select' statements above and just
make your drivers 'default ARCH_SC59X_64'.

It may also help to pick a more generic name for the platform
in case someone wants to add support for SC57x/SC58x later,
assuming these use some of the same drivers,.

The Kconfig change can normally go into the same patch
as the MAINTAINERS file update, but should be separate
from any of the drivers.

> --- /dev/null
> +++ b/drivers/soc/adi/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# todo modularize; already depends on CONFIG_ARCH_SC59X_64 though
> +
> +obj-y += system.o
> diff --git a/drivers/soc/adi/system.c b/drivers/soc/adi/system.c

I'm confused about the purpose of this driver. Please
split this out into a separate patch and add a detailed
description of how it is actually being used, since it
does not interact with any of the normal subsystems.


> diff --git a/include/linux/soc/adi/adsp-gpio-port.h 
> b/include/linux/soc/adi/adsp-gpio-port.h

> --- /dev/null
> +++ b/include/linux/soc/adi/cpu.h

> --- /dev/null
> +++ b/include/linux/soc/adi/rcu.h
> @@ -0,0 +1,55 @@

> diff --git a/include/linux/soc/adi/sc59x.h 
> b/include/linux/soc/adi/sc59x.h

> --- /dev/null
> +++ b/include/linux/soc/adi/sc59x.h

I don't see these files being included in the driver you add
here, maybe they got added by accident here?

       Arnd
Arnd Bergmann Sept. 13, 2024, 8:20 a.m. UTC | #8
On Thu, Sep 12, 2024, at 18:24, Arturs Artamonovs via B4 Relay wrote:
> This set of patches based on ADI fork of Linux Kerenl that support 
> family of ADSP-SC5xx
> SoC's and used by customers for some time . Patch series contains 
> minimal set
> of changes to add ADSP-SC598 support to upstream kernel. This series 
> include
> UART,I2C,IRQCHIP,RCU drivers and device-tree to be able boot on 
> EV-SC598-SOM
> board into serial shell and able to reset the board. Current SOM board
> requires I2C expander to enable UART output.
>
> UART,I2C and PINCTRL drivers are based on old Blackfin drivers with
> ADSP-SC5xx related bug fixes and improvments.
>
> Signed-off-by: Arturs Artamonovs <arturs.artamonovs@analog.com>

Hi Arturs,

Thanks for your submission. I've done a first pass of a review
now, but the drivers will all need a more detailed review from
the subsystem maintainers as well.

For the drivers/soc and include/linux/soc portions, I need
to do second review round when you have added a description
about what these are used for, ideally I would hope that most
of those can disappear from the final series when the required
bits are moved into other drivers.

I commented on one of the bindings about the compatible
string, but later saw that the same issue is present in all
of the bindings, which each need a more specific identifier
for a particular piece of hardware they are compatible with.

        Arnd
Artamonovs, Arturs Sept. 13, 2024, 9:54 a.m. UTC | #9
> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Friday, September 13, 2024 9:16 AM
> To: Artamonovs, Arturs <Arturs.Artamonovs@analog.com>; Catalin Marinas
> <catalin.marinas@arm.com>; Will Deacon <will@kernel.org>; Greg Malysa
> <greg.malysa@timesys.com>; Philipp Zabel <p.zabel@pengutronix.de>; Rob
> Herring <robh@kernel.org>; Krzysztof Kozlowski <krzk+dt@kernel.org>; Conor
> Dooley <conor+dt@kernel.org>; Agarwal, Utsav <Utsav.Agarwal@analog.com>;
> Michael Turquette <mturquette@baylibre.com>; Stephen Boyd
> <sboyd@kernel.org>; Linus Walleij <linus.walleij@linaro.org>; Bartosz
> Golaszewski <brgl@bgdev.pl>; Thomas Gleixner <tglx@linutronix.de>; Andi Shyti
> <andi.shyti@kernel.org>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>;
> Jiri Slaby <jirislaby@kernel.org>; Olof Johansson <olof@lixom.net>;
> soc@kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicetree@vger.kernel.org; linux-clk@vger.kernel.org; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-i2c@vger.kernel.org; linux-
> serial@vger.kernel.org; Linux Factory <adsp-linux@analog.com>; Nathan Barrett-
> Morrison <nathan.morrison@timesys.com>
> Subject: Re: [PATCH 01/21] arm64: Add ADI ADSP-SC598 SoC
> 
> [External]
> 
> On Thu, Sep 12, 2024, at 18:24, Arturs Artamonovs via B4 Relay wrote:
> > From: Arturs Artamonovs <arturs.artamonovs@analog.com>
> >
> > Add ADSP-SC598 platform.
> >
> 
> > --- a/arch/arm64/Kconfig.platforms
> > +++ b/arch/arm64/Kconfig.platforms
> > @@ -292,6 +292,19 @@ config ARCH_ROCKCHIP
> >  	  This enables support for the ARMv8 based Rockchip chipsets,
> >  	  like the RK3368.
> >
> > +config ARCH_SC59X_64
> > +	bool "ADI 64-bit SC59X Platforms"
> > +	select TIMER_OF
> > +	select GPIOLIB
> > +	select PINCTRL
> > +	select COMMON_CLK_ADI_SC598
> > +	select PINCTRL_ADSP
> > +	select ADI_ADSP_IRQ
> > +	select COUNTER
> 
> You can remove the 'select' statements above and just
> make your drivers 'default ARCH_SC59X_64'.
> 
> It may also help to pick a more generic name for the platform
> in case someone wants to add support for SC57x/SC58x later,
> assuming these use some of the same drivers,.
> 
> The Kconfig change can normally go into the same patch
> as the MAINTAINERS file update, but should be separate
> from any of the drivers.
> 

Hi, yes future plan is too add other platforms  like
SC57x/SC58x and SC594. Drivers are compatible. 

> > --- /dev/null
> > +++ b/drivers/soc/adi/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +# todo modularize; already depends on CONFIG_ARCH_SC59X_64 though
> > +
> > +obj-y += system.o
> > diff --git a/drivers/soc/adi/system.c b/drivers/soc/adi/system.c
> 
> I'm confused about the purpose of this driver. Please
> split this out into a separate patch and add a detailed
> description of how it is actually being used, since it
> does not interact with any of the normal subsystems.
> 

Hi, yes we cleaned this driver as much as possible, will
make effort to remove it. 

> > diff --git a/include/linux/soc/adi/adsp-gpio-port.h
> > b/include/linux/soc/adi/adsp-gpio-port.h
> 
> > --- /dev/null
> > +++ b/include/linux/soc/adi/cpu.h
> 
> > --- /dev/null
> > +++ b/include/linux/soc/adi/rcu.h
> > @@ -0,0 +1,55 @@
> 
> > diff --git a/include/linux/soc/adi/sc59x.h
> > b/include/linux/soc/adi/sc59x.h
> 
> > --- /dev/null
> > +++ b/include/linux/soc/adi/sc59x.h
> 
> I don't see these files being included in the driver you add
> here, maybe they got added by accident here?
> 

Should be used in reset driver its removed during rebase, will fix that 
In next series. 

>        Arnd
Rob Herring (Arm) Sept. 13, 2024, 2:06 p.m. UTC | #10
On Thu, Sep 12, 2024 at 07:25:03PM +0100, Arturs Artamonovs wrote:
> Add serial driver bindings.

Don''t need 'documentation' in the the subject. That's redundant with 
'dt-bindings'.


> 
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>

Your S-o-b goes last since you are sending the patch.

> Signed-off-by: Utsav Agarwal <Utsav.Agarwal@analog.com>

Not clear what Utsav's role was. Needs Co-developed-by?

> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
> ---
>  .../devicetree/bindings/serial/adi,uart.yaml       | 85 ++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/serial/adi,uart.yaml b/Documentation/devicetree/bindings/serial/adi,uart.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..de58059efa7e21acaa5b7f4984ffadca18f7f53a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/adi,uart.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/adi,uart4.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices UART Driver for SC5XX-family processors

Bindings aren't a driver.

> +
> +maintainers:
> +  - Arturs Artamonovs <arturs.artamonovs@analog.com>
> +  - Utsav Agarwal <Utsav.Agarwal@analog.com>
> +
> +description: |

Don't need '|'.

> +  Analog Devices UART Driver for SC59X-family processors
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,uart

Only 1 UART implementation for all of Analog Devices ever.

compatibles should be specific to SoC.

> +
> +  reg:
> +    maxItems: 1
> +
> +  dmas:
> +    maxItems: 2
> +    minItems: 2
> +    description: TX and RX DMA cluster numbers
> +
> +  dma-names:
> +    maxItems: 2
> +    minItems: 2
> +    description: DMA channel names (tx and rx)

Names need to be constraints, not freeform text. Plenty of examples to 
look at...

> +
> +  clocks:
> +    maxItems: 1
> +    description: Clock being used for UART

That's obvious. Drop description or say something unique to this device.

> +
> +  clock-names:
> +    maxItems: 1
> +    description: Clock name (sclk0)
> +
> +  interrupt-names:
> +    minItems: 3
> +    maxItems: 3
> +    description: Interrupt names (tx + rx + status)
> +
> +  interrupts:
> +    minItems: 3
> +    maxItems: 3
> +    description: GIC interrupt numbers
> +
> +  adi,use-edbo:
> +    type: boolean
> +    description: Enable divide by one in divisor

Versus divide by ???

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupt-names
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/clock/adi-sc5xx-clock.h>
> +
> +    uart0: uart@31003000 {

serial@...

Drop unused labels.

> +      compatible = "adi,uart";
> +      reg = <0x31003000 0x40>;
> +      clocks = <&clk ADSP_SC598_CLK_CGU0_SCLK0>;
> +      clock-names = "sclk0";
> +      interrupt-parent = <&gic>;
> +      interrupt-names = "tx", "rx", "status";
> +      interrupts = <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>,
> +                   <GIC_SPI 139 IRQ_TYPE_LEVEL_HIGH>,
> +                   <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
> +      adi,use-edbo;
> +      status = "disabled";

Examples should be enabled. Drop.

> +    };
> +
> 
> -- 
> 2.25.1
>
Rob Herring (Arm) Sept. 13, 2024, 10:05 p.m. UTC | #11
On Thu, Sep 12, 2024 at 07:24:48PM +0100, Arturs Artamonovs wrote:
> Bindigs for ADI ADSP-SC5xx reset controller

Typo. Here and the subject. Please write complete sentences.

> 
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
> Co-developed-by: Utsav Agarwal <Utsav.Agarwal@analog.com>
> Signed-off-by: Utsav Agarwal <Utsav.Agarwal@analog.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
> ---
>  .../bindings/soc/adi/adi,reset-controller.yaml     | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/soc/adi/adi,reset-controller.yaml b/Documentation/devicetree/bindings/soc/adi/adi,reset-controller.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..7a6df1cfb709d818d5e3dbcd202938d6aaaaaa9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/adi/adi,reset-controller.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/adi/adi,reset-controller.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Reset Controller for SC5XX processor family
> +
> +maintainers:
> +  - Arturs Artamonovs <arturs.artamonovs@analog.com>
> +  - Utsav Agarwal <Utsav.Agarwal@analog.com>
> +
> +description: |

Don't need '|'

> +  SHARC and ARM core reset control unit for starting/stopping/resetting
> +  processors

Complete sentences for top-level description.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,reset-controller

Too generic.

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    rcu: rcu@3108c000 {
> +      compatible = "adi,reset-controller";
> +      reg = <0x3108c000 0x1000>;
> +      status = "okay";

Don't need status in examples.

Shouldn't a reset controller use the reset binding (i.e. #reset-cells)?

All you patches seem to have similar issues, so I'm not going to comment 
on all of them. Please read the documentation in 
Documentation/devicetree/bindings/. It doesn't seem like you have.

Rob
Rob Herring (Arm) Sept. 13, 2024, 10:09 p.m. UTC | #12
On Thu, Sep 12, 2024 at 07:24:59PM +0100, Arturs Artamonovs wrote:
> Add PINCTRL driver bindings.
> 
> Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
> Co-developed-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Signed-off-by: Nathan Barrett-Morrison <nathan.morrison@timesys.com>
> Co-developed-by: Greg Malysa <greg.malysa@timesys.com>
> Signed-off-by: Greg Malysa <greg.malysa@timesys.com>
> ---
>  .../bindings/pinctrl/adi,adsp-pinctrl.yaml         | 83 ++++++++++++++++++++++
>  include/dt-bindings/pinctrl/adi-adsp.h             | 19 +++++
>  2 files changed, 102 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/adi,adsp-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/adi,adsp-pinctrl.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..073442b4f680bf536f631b4c17a1d3195c2233d6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/adi,adsp-pinctrl.yaml
> @@ -0,0 +1,83 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/adi,adsp-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices Pinmuxing Control for SC5XX Processor Family
> +
> +maintainers:
> +  - Arturs Artamonovs <arturs.artamonovs@analog.com>
> +  - Utsav Agarwal <Utsav.Agarwal@analog.com>
> +
> +description: |
> +  Pinmuxing Control Driver for Configuring Processor Pins/Pads
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adsp-pinctrl
> +
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +  reg:
> +    maxItems: 1
> +
> +  "adi,port-sizes":

Don't need quotes.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    maxItems: 9
> +    description: Space delimited integer list denoting number of pins per port
> +      Ports A-I exist, so this is up to 9 items long

No constraints on the entries?

> +
> +  "adi,no-drive-strength":
> +    type: boolean
> +    description: Indicate missing drive strength registers
> +
> +  "adi,no-pull-up-down":
> +    type: boolean
> +    description: Indicate missing pull up/down enable registers
> +
> +patternProperties:
> +  '-pins$':
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      pins:
> +        type: object
> +        description: |
> +          A pinctrl node should contain a pin property, specifying the actual
> +          pins to use.
> +
> +        properties:
> +          pinmux:
> +            $ref: /schemas/types.yaml#/definitions/uint32-array
> +            description: |
> +              pinmux is used to specify which of the available functionalities
> +              for a given pin are actually used.
> +
> +        additionalProperties: false
> +
> +required:
> +  - compatible
> +  - "#address-cells"
> +  - "#size-cells"
> +  - reg
> +  - "adi,port-sizes"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pinctrl0: pinctrl@31004600 {
> +      compatible = "adi,adsp-pinctrl";
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      reg = <0x31004600 0x400>;
> +      adi,port-sizes = <16 16 16 16 16 16 16 16 7>;
> +    };
> +
> diff --git a/include/dt-bindings/pinctrl/adi-adsp.h b/include/dt-bindings/pinctrl/adi-adsp.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..dc5b86a0d9190acdd242a6ba4972c3aac7a61821
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/adi-adsp.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0*/

Missing space                         ^

New bindings should be dual licensed.


> +/*
> + * Macros for populating pinmux properties on the pincontroller
> + *
> + * Copyright 2022-2024 - Analog Devices Inc.
> + */
> +
> +#ifndef DT_BINDINGS_PINCTRL_ADI_ADSP_H
> +#define DT_BINDINGS_PINCTRL_ADI_ADSP_H
> +
> +#define ADI_ADSP_PINFUNC_GPIO     0
> +#define ADI_ADSP_PINFUNC_ALT0     1
> +#define ADI_ADSP_PINFUNC_ALT1     2
> +#define ADI_ADSP_PINFUNC_ALT2     3
> +#define ADI_ADSP_PINFUNC_ALT3     4
> +
> +#define ADI_ADSP_PINMUX(port, pin, func) ((((port - 'A')*16 + pin) << 8) + func)
> +
> +#endif
> 
> -- 
> 2.25.1
>
Krzysztof Kozlowski Sept. 16, 2024, 6:57 a.m. UTC | #13
On 12/09/2024 23:04, Rob Herring (Arm) wrote:
> 
> On Thu, 12 Sep 2024 19:24:45 +0100, Arturs Artamonovs wrote:
>> This set of patches based on ADI fork of Linux Kerenl that support family of ADSP-SC5xx
>> SoC's and used by customers for some time . Patch series contains minimal set
>> of changes to add ADSP-SC598 support to upstream kernel. This series include
>> UART,I2C,IRQCHIP,RCU drivers and device-tree to be able boot on EV-SC598-SOM
>> board into serial shell and able to reset the board. Current SOM board
>> requires I2C expander to enable UART output.
>>
>> UART,I2C and PINCTRL drivers are based on old Blackfin drivers with
>> ADSP-SC5xx related bug fixes and improvments.
>>
>> Signed-off-by: Arturs Artamonovs <arturs.artamonovs@analog.com>
>> ---
>> Arturs Artamonovs (21):
>>       arm64: Add ADI ADSP-SC598 SoC
>>       reset: Add driver for ADI ADSP-SC5xx reset controller
>>       dt-bindigs: arm64: adi,sc598 bindings
>>       dt-bindings: arm64: adi,sc598: Add ADSP-SC598 SoC bindings
>>       clock:Add driver for ADI ADSP-SC5xx PLL
>>       include: dt-binding: clock: add adi clock header file
>>       clock: Add driver for ADI ADSP-SC5xx clock
>>       dt-bindings: clock: adi,sc5xx-clocks: add bindings
>>       gpio: add driver for ADI ADSP-SC5xx platform
>>       dt-bindings: gpio: adi,adsp-port-gpio: add bindings
>>       irqchip: Add irqchip for ADI ADSP-SC5xx platform
>>       dt-bindings: irqchip: adi,adsp-pint: add binding
>>       pinctrl: Add drivers for ADI ADSP-SC5xx platform
>>       dt-bindings: pinctrl: adi,adsp-pinctrl: add bindings
>>       i2c: Add driver for ADI ADSP-SC5xx platforms
>>       dt-bindings: i2c: add i2c/twi driver documentation
>>       serial: adi,uart: Add driver for ADI ADSP-SC5xx
>>       dt-bindings: serial: adi,uart4: add adi,uart4 driver documentation
>>       arm64: dts: adi: sc598: add device tree
>>       arm64: defconfig: sc598 add minimal changes
>>       MAINTAINERS: add adi sc5xx maintainers
>>
>>  .../devicetree/bindings/arm/analog/adi,sc5xx.yaml  |   24 +
>>  .../bindings/clock/adi,sc5xx-clocks.yaml           |   65 ++
>>  .../bindings/gpio/adi,adsp-port-gpio.yaml          |   69 ++
>>  Documentation/devicetree/bindings/i2c/adi,twi.yaml |   71 ++
>>  .../interrupt-controller/adi,adsp-pint.yaml        |   51 +
>>  .../bindings/pinctrl/adi,adsp-pinctrl.yaml         |   83 ++
>>  .../devicetree/bindings/serial/adi,uart.yaml       |   85 ++
>>  .../bindings/soc/adi/adi,reset-controller.yaml     |   38 +
>>  MAINTAINERS                                        |   22 +
>>  arch/arm64/Kconfig.platforms                       |   13 +
>>  arch/arm64/boot/dts/Makefile                       |    1 +
>>  arch/arm64/boot/dts/adi/Makefile                   |    2 +
>>  arch/arm64/boot/dts/adi/sc598-som-ezkit.dts        |   14 +
>>  arch/arm64/boot/dts/adi/sc598-som.dtsi             |   58 ++
>>  arch/arm64/boot/dts/adi/sc59x-64.dtsi              |  367 +++++++
>>  arch/arm64/configs/defconfig                       |    6 +
>>  drivers/clk/Kconfig                                |    9 +
>>  drivers/clk/Makefile                               |    1 +
>>  drivers/clk/adi/Makefile                           |    4 +
>>  drivers/clk/adi/clk-adi-pll.c                      |  151 +++
>>  drivers/clk/adi/clk-adi-sc598.c                    |  329 ++++++
>>  drivers/clk/adi/clk.h                              |   99 ++
>>  drivers/gpio/Kconfig                               |    8 +
>>  drivers/gpio/Makefile                              |    1 +
>>  drivers/gpio/gpio-adi-adsp-port.c                  |  145 +++
>>  drivers/i2c/busses/Kconfig                         |   17 +
>>  drivers/i2c/busses/Makefile                        |    1 +
>>  drivers/i2c/busses/i2c-adi-twi.c                   |  940 ++++++++++++++++++
>>  drivers/irqchip/Kconfig                            |    9 +
>>  drivers/irqchip/Makefile                           |    2 +
>>  drivers/irqchip/irq-adi-adsp.c                     |  310 ++++++
>>  drivers/pinctrl/Kconfig                            |   12 +
>>  drivers/pinctrl/Makefile                           |    1 +
>>  drivers/pinctrl/pinctrl-adsp.c                     |  919 +++++++++++++++++
>>  drivers/reset/Makefile                             |    1 +
>>  drivers/soc/Makefile                               |    1 +
>>  drivers/soc/adi/Makefile                           |    5 +
>>  drivers/soc/adi/system.c                           |  257 +++++
>>  drivers/tty/serial/Kconfig                         |   19 +-
>>  drivers/tty/serial/Makefile                        |    1 +
>>  drivers/tty/serial/adi_uart.c                      | 1045 ++++++++++++++++++++
>>  include/dt-bindings/clock/adi-sc5xx-clock.h        |   93 ++
>>  include/dt-bindings/pinctrl/adi-adsp.h             |   19 +
>>  include/linux/soc/adi/adsp-gpio-port.h             |   85 ++
>>  include/linux/soc/adi/cpu.h                        |  107 ++
>>  include/linux/soc/adi/rcu.h                        |   55 ++
>>  include/linux/soc/adi/sc59x.h                      |  147 +++
>>  include/linux/soc/adi/system_config.h              |   65 ++
>>  include/uapi/linux/serial_core.h                   |    3 +
>>  49 files changed, 5829 insertions(+), 1 deletion(-)
>> ---
>> base-commit: da3ea35007d0af457a0afc87e84fddaebc4e0b63
>> change-id: 20240909-test-8ec5f76fe6d2
>>
>> Best regards,
>> --
>> Arturs Artamonovs <arturs.artamonovs@analog.com>
>>
>>
>>
> 
> 
> My bot found new DTB warnings on the .dts files added or changed in this
> series.
> 
> Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
> are fixed by another series. Ultimately, it is up to the platform
> maintainer whether these warnings are acceptable or not. No need to reply
> unless the platform maintainer has comments.
> 
> If you already ran DT checks and didn't see these error(s), then
> make sure dt-schema is up to date:
> 
>   pip3 install dtschema --upgrade
> 
> 
> New warnings running 'make CHECK_DTBS=y adi/sc598-som-ezkit.dtb' for 20240912-test-v1-0-458fa57c8ccf@analog.com:
> 
> arch/arm64/boot/dts/adi/sc598-som-ezkit.dtb: /scb-bus/sec@31089000: failed to match any schema with compatible: ['adi,system-event-controller']

This must be addressed and fixed.

Best regards,
Krzysztof
Krzysztof Kozlowski Sept. 16, 2024, 9:05 a.m. UTC | #14
On 12/09/2024 20:24, Arturs Artamonovs via B4 Relay wrote:
> This set of patches based on ADI fork of Linux Kerenl that support family of ADSP-SC5xx
> SoC's and used by customers for some time . Patch series contains minimal set
> of changes to add ADSP-SC598 support to upstream kernel. This series include
> UART,I2C,IRQCHIP,RCU drivers and device-tree to be able boot on EV-SC598-SOM
> board into serial shell and able to reset the board. Current SOM board
> requires I2C expander to enable UART output.
> 
> UART,I2C and PINCTRL drivers are based on old Blackfin drivers with
> ADSP-SC5xx related bug fixes and improvments.
> 
> Signed-off-by: Arturs Artamonovs <arturs.artamonovs@analog.com>
> ---

For new platform, be sure you have 0 warnings:
1. Please run standard kernel tools for static analysis, like
coccinelle, smatch and sparse, and fix reported warnings.

2. Also check for warnings when building with W=1. Most of these
commands (checks or W=1 build) can build specific targets, like some
directory, to narrow the scope to only your code. The code here looks
like it needs a fix. Feel free to get in touch if the warning is not clear.

3. Fix all compile test warning reported by LKP and check for common
configs, regardless of reports.

4. Please run `make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

5. Please run scripts/checkpatch.pl and fix reported warnings. Then
please run `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run.


Best regards,
Krzysztof