[PATCH 00/15] Ambarella S6LM SoC bring-up

[PATCH 00/15] Ambarella S6LM SoC bring-up

[Li Chen]

This series brings up initial support for the Ambarella S6LM
SoC.

The following features are supported in this initial port:

- UART with console support
- Pinctrl with GPIO controller
- Nand flash controller
- Devicetree

Li Chen (15):
  debugfs: allow to use regmap for print regs
  dt-bindings: vendor-prefixes: add Ambarella prefix
  dt-bindings: arm: ambarella: Add binding for Ambarella ARM platforms
  dt-bindings: arm: add support for Ambarella SoC
  arm64: Kconfig: Introduce CONFIG_ARCH_AMBARELLA
  soc: add Ambarella driver
  dt-bindings: clock: Add Ambarella clock bindings
  clk: add support for Ambarella clocks
  dt-bindings: serial: add support for Ambarella
  serial: ambarella: add support for Ambarella uart_port
  dt-bindings: mtd: Add binding for Ambarella
  mtd: nand: add Ambarella nand support
  dt-bindings: pinctrl: add support for Ambarella
  pinctrl: Add pinctrl/GPIO for Ambarella SoCs
  arm64: dts: ambarella: introduce Ambarella s6lm SoC

 .../devicetree/bindings/arm/ambarella.yaml    |   22 +
 .../arm/ambarella/ambarella,cpuid.yaml        |   24 +
 .../bindings/arm/ambarella/ambarella,rct.yaml |   24 +
 .../arm/ambarella/ambarella,scratchpad.yaml   |   24 +
 .../bindings/arm/ambarella/ambarella.yaml     |   22 +
 .../clock/ambarella,composite-clock.yaml      |   52 +
 .../bindings/clock/ambarella,pll-clock.yaml   |   59 +
 .../bindings/mtd/ambarella,nand.yaml          |   77 +
 .../bindings/pinctrl/ambarella,pinctrl.yaml   |  160 ++
 .../bindings/serial/ambarella_uart.yaml       |   57 +
 .../devicetree/bindings/vendor-prefixes.yaml  |    2 +
 Documentation/filesystems/debugfs.rst         |    2 +
 MAINTAINERS                                   |   29 +
 arch/arm64/Kconfig.platforms                  |    9 +
 .../boot/dts/ambarella/ambarella-s6lm.dtsi    |  332 ++++
 .../boot/dts/ambarella/s6lm_pineapple.dts     |   29 +
 drivers/clk/Makefile                          |    1 +
 drivers/clk/ambarella/Makefile                |    5 +
 drivers/clk/ambarella/clk-composite.c         |  293 +++
 drivers/clk/ambarella/clk-pll-common.c        |  308 ++++
 drivers/clk/ambarella/clk-pll-common.h        |   96 +
 drivers/clk/ambarella/clk-pll-normal.c        |  328 ++++
 drivers/mtd/nand/raw/Kconfig                  |    8 +
 drivers/mtd/nand/raw/Makefile                 |    1 +
 drivers/mtd/nand/raw/ambarella_combo_nand.c   | 1519 ++++++++++++++++
 drivers/mtd/nand/raw/ambarella_combo_nand.h   |  370 ++++
 drivers/mtd/nand/raw/nand_ids.c               |    4 +
 drivers/pinctrl/Kconfig                       |    6 +
 drivers/pinctrl/Makefile                      |    1 +
 drivers/pinctrl/pinctrl-ambarella.c           | 1357 ++++++++++++++
 drivers/soc/Makefile                          |    1 +
 drivers/soc/ambarella/Makefile                |    3 +
 drivers/soc/ambarella/soc.c                   |  136 ++
 drivers/tty/serial/Kconfig                    |   16 +
 drivers/tty/serial/Makefile                   |    1 +
 drivers/tty/serial/ambarella_uart.c           | 1581 +++++++++++++++++
 drivers/tty/serial/ambarella_uart.h           |  120 ++
 fs/debugfs/file.c                             |   43 +-
 include/linux/debugfs.h                       |   11 +
 include/soc/ambarella/misc.h                  |   17 +
 40 files changed, 7149 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/ambarella.yaml
 create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
 create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
 create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
 create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
 create mode 100644 Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
 create mode 100644 Documentation/devicetree/bindings/mtd/ambarella,nand.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/ambarella,pinctrl.yaml
 create mode 100644 Documentation/devicetree/bindings/serial/ambarella_uart.yaml
 create mode 100644 arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
 create mode 100644 arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts
 create mode 100644 drivers/clk/ambarella/Makefile
 create mode 100644 drivers/clk/ambarella/clk-composite.c
 create mode 100644 drivers/clk/ambarella/clk-pll-common.c
 create mode 100644 drivers/clk/ambarella/clk-pll-common.h
 create mode 100644 drivers/clk/ambarella/clk-pll-normal.c
 create mode 100644 drivers/mtd/nand/raw/ambarella_combo_nand.c
 create mode 100644 drivers/mtd/nand/raw/ambarella_combo_nand.h
 create mode 100644 drivers/pinctrl/pinctrl-ambarella.c
 create mode 100644 drivers/soc/ambarella/Makefile
 create mode 100644 drivers/soc/ambarella/soc.c
 create mode 100644 drivers/tty/serial/ambarella_uart.c
 create mode 100644 drivers/tty/serial/ambarella_uart.h
 create mode 100644 include/soc/ambarella/misc.h

-- 
2.34.1

[PATCH 02/15] dt-bindings: vendor-prefixes: add Ambarella prefix

[Li Chen]

Add vendor prefix for Ambarella, Inc.

Signed-off-by: Li Chen <lchen@ambarella.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 70ffb3780621..97bfd98fcae1 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -89,6 +89,8 @@ patternProperties:
     description: Amarula Solutions
   "^amazon,.*":
     description: Amazon.com, Inc.
+  "^ambarella,.*":
+    description: Ambarella, Inc.
   "^amcc,.*":
     description: Applied Micro Circuits Corporation (APM, formally AMCC)
   "^amd,.*":
-- 
2.34.1

Re: [PATCH 02/15] dt-bindings: vendor-prefixes: add Ambarella prefix

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> Add vendor prefix for Ambarella, Inc.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 03/15] dt-bindings: arm: ambarella: Add binding for Ambarella ARM platforms

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> This introduces binding for Ambarella s6lm SoC.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: Id95d91218625a1f0b3413aac101c2ca8831f0385

Please run scripts/checkpatch.pl and fix reported warnings.

> ---
>  .../devicetree/bindings/arm/ambarella.yaml    | 22 +++++++++++++++++++
>  MAINTAINERS                                   |  6 +++++
>  2 files changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/ambarella.yaml b/Documentation/devicetree/bindings/arm/ambarella.yaml
> new file mode 100644
> index 000000000000..2c8ff75b57b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/ambarella.yaml
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/ambarella.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella SoC Device Tree Bindings

Drop "Device Tree Bindings"

> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +
> +properties:
> +  $nodename:
> +    const: "/"
> +  compatible:
> +    oneOf:
> +      - description: Ambarella SoC based platforms
> +        items:
> +          - enum:
> +              - ambarella,s6lm


Best regards,
Krzysztof

Re: [PATCH 03/15] dt-bindings: arm: ambarella: Add binding for Ambarella ARM platforms

[Li Chen]

On Mon, 23 Jan 2023 16:03:27 +0800,
Krzysztof Kozlowski wrote:
> 
> On 23/01/2023 08:32, Li Chen wrote:
> > This introduces binding for Ambarella s6lm SoC.
> > 
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> > Change-Id: Id95d91218625a1f0b3413aac101c2ca8831f0385
> 
> Please run scripts/checkpatch.pl and fix reported warnings.

Sorry for it, I do run checkpatch & sparse and fix issues, but forget to remove the
Change-id finally, will remove it in v2.

> > ---
> >  .../devicetree/bindings/arm/ambarella.yaml    | 22 +++++++++++++++++++
> >  MAINTAINERS                                   |  6 +++++
> >  2 files changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella.yaml b/Documentation/devicetree/bindings/arm/ambarella.yaml
> > new file mode 100644
> > index 000000000000..2c8ff75b57b9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella.yaml
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: GPL-2.0-only or BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/ambarella.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella SoC Device Tree Bindings
> 
> Drop "Device Tree Bindings"

Well noted.

> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  $nodename:
> > +    const: "/"
> > +  compatible:
> > +    oneOf:
> > +      - description: Ambarella SoC based platforms
> > +        items:
> > +          - enum:
> > +              - ambarella,s6lm

Thanks for your review!

Regards,
Li

Re: [PATCH 04/15] dt-bindings: arm: add support for Ambarella SoC

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> Create a vendor directory for Ambarella, and add
> cpuid, rct, scratchpad documents.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: I2c29e45c08666489b0d9b588ac37d713f5b723d1

Please run scripts/checkpatch.pl and fix reported warnings.

Applies to all your patches. Also test them... I have doubts that you
tested if you actually ignored checkpatch :/

> ---
>  .../arm/ambarella/ambarella,cpuid.yaml        | 24 +++++++++++++++++++
>  .../bindings/arm/ambarella/ambarella,rct.yaml | 24 +++++++++++++++++++
>  .../arm/ambarella/ambarella,scratchpad.yaml   | 24 +++++++++++++++++++
>  .../bindings/arm/ambarella/ambarella.yaml     | 22 +++++++++++++++++
>  MAINTAINERS                                   |  4 ++++
>  5 files changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
> new file mode 100644
> index 000000000000..1f4d9cec8f92
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml

This goes to soc

> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ambarella,cpuid.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella SoC ID
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>

Missing description.

> +
> +properties:
> +  compatible:
> +    const: "ambarella,cpuid", "syscon"

Drop quotes (applies to all your patches)

Missing SoC specific compatible.

> +
> +  reg:
> +    maxItems: 1

Missing additionalProperties. sorry, start from scratch from some
existing recent bindings or better example-schema.

> +
> +examples:
> +  - |
> +    cpuid_syscon: cpuid@e0000000 {
> +        compatible = "ambarella,cpuid", "syscon";
> +        reg = <0xe0000000 0x1000>;
> +    };
> diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> new file mode 100644
> index 000000000000..7279bab17d9e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ambarella,rct.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella RCT module
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +
> +properties:
> +  compatible:
> +    const: "ambarella,rct", "syscon"

All the same problems.

> +
> +  reg:
> +    maxItems: 1
> +
> +examples:
> +  - |
> +		rct_syscon: rct_syscon@ed080000 {

Really? Just take a look and you will see wrong indentation. Also drop
underscores in node names and "rct". Node names should be generic.


> +        compatible = "ambarella,rct", "syscon";
> +        reg = <0xed080000 0x1000>;
> +    };
> diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> new file mode 100644
> index 000000000000..5d2bd243b5c9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ambarella,scratchpad.yaml#

That's not a clock controller!


> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella Scratchpad
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +
> +properties:
> +  compatible:
> +    const: "ambarella,scratchpad", "syscon"
> +
> +  reg:
> +    maxItems: 1
> +
> +examples:
> +  - |
> +    scratchpad_syscon: scratchpad_syscon@e0022000 {

All the same problems.

> +        compatible = "ambarella,scratchpad", "syscon";
> +        reg = <0xe0022000 0x100>;
> +    };
> diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> new file mode 100644
> index 000000000000..5991bd745c05
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> @@ -0,0 +1,22 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/ambarella.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella SoC Device Tree Bindings
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +
> +properties:
> +  $nodename:
> +    const: "/"
> +  compatible:
> +    oneOf:
> +      - description: Ambarella SoC based platforms
> +        items:
> +          - enum:
> +              - ambarella,s6lm

What is this? How do you expect it to apply? Can you try by yourself?


Best regards,
Krzysztof

Re: [PATCH 04/15] dt-bindings: arm: add support for Ambarella SoC

[Li Chen]

On Mon, 23 Jan 2023 16:07:32 +0800,
Krzysztof Kozlowski wrote:
>
> On 23/01/2023 08:32, Li Chen wrote:
> > Create a vendor directory for Ambarella, and add
> > cpuid, rct, scratchpad documents.
> >
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> > Change-Id: I2c29e45c08666489b0d9b588ac37d713f5b723d1
>
> Please run scripts/checkpatch.pl and fix reported warnings.
>
> Applies to all your patches. Also test them... I have doubts that you
> tested if you actually ignored checkpatch :/

Yeah, I checkpatch all patches, and have planned to fix Change-Id finally(manually),
but forget it before sending mails, my bad, sorry. I will remove it in v2.

> > ---
> >  .../arm/ambarella/ambarella,cpuid.yaml        | 24 +++++++++++++++++++
> >  .../bindings/arm/ambarella/ambarella,rct.yaml | 24 +++++++++++++++++++
> >  .../arm/ambarella/ambarella,scratchpad.yaml   | 24 +++++++++++++++++++
> >  .../bindings/arm/ambarella/ambarella.yaml     | 22 +++++++++++++++++
> >  MAINTAINERS                                   |  4 ++++
> >  5 files changed, 98 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> >  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
> > new file mode 100644
> > index 000000000000..1f4d9cec8f92
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
>
> This goes to soc

Thanks, I wasn't aware that there is a document dir named soc. I will move cpuid yaml
to bindings/soc/ambarella/, and leave other yaml still here.

> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,cpuid.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella SoC ID
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
>
> Missing description.

Sorry, description will be added in v2. BTW, does other YAMLs in this patch
also need descriptions?

> > +
> > +properties:
> > +  compatible:
> > +    const: "ambarella,cpuid", "syscon"
>
> Drop quotes (applies to all your patches)

OK, thanks!

> Missing SoC specific compatible.
>
> > +
> > +  reg:
> > +    maxItems: 1
>
> Missing additionalProperties. sorry, start from scratch from some
> existing recent bindings or better example-schema.

Good to know that there is example-schema, thanks!
 
> > +
> > +examples:
> > +  - |
> > +    cpuid_syscon: cpuid@e0000000 {
> > +        compatible = "ambarella,cpuid", "syscon";
> > +        reg = <0xe0000000 0x1000>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> > new file mode 100644
> > index 000000000000..7279bab17d9e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,rct.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella RCT module
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: "ambarella,rct", "syscon"
>
> All the same problems.

Well noted.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +examples:
> > +  - |
> > +		rct_syscon: rct_syscon@ed080000 {
>
> Really? Just take a look and you will see wrong indentation. Also drop
> underscores in node names and "rct". Node names should be generic.

Sorry for the wrong indentation, will fix it in v2.

Is it ok to contain underscores in lable? if so, I will change it into

rct_syscon: syscon@ed080000 {

in v2.

>
> > +        compatible = "ambarella,rct", "syscon";
> > +        reg = <0xed080000 0x1000>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> > new file mode 100644
> > index 000000000000..5d2bd243b5c9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
> > @@ -0,0 +1,24 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,scratchpad.yaml#
>
> That's not a clock controller!

Sorry, will fix it in v2.

> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella Scratchpad
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: "ambarella,scratchpad", "syscon"
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +examples:
> > +  - |
> > +    scratchpad_syscon: scratchpad_syscon@e0022000 {
>
> All the same problems.

Well noted.

> > +        compatible = "ambarella,scratchpad", "syscon";
> > +        reg = <0xe0022000 0x100>;
> > +    };
> > diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> > new file mode 100644
> > index 000000000000..5991bd745c05
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/ambarella.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella SoC Device Tree Bindings
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  $nodename:
> > +    const: "/"
> > +  compatible:
> > +    oneOf:
> > +      - description: Ambarella SoC based platforms
> > +        items:
> > +          - enum:
> > +              - ambarella,s6lm
>
> What is this? How do you expect it to apply? Can you try by yourself?

Sorry, I didn't find this file is duplicited with outside ambarella.yaml.
I will remove it in v2.

Thanks for your review!

Regards,
Li

Re: [PATCH 04/15] dt-bindings: arm: add support for Ambarella SoC

[Krzysztof Kozlowski]

On 23/01/2023 16:09, Li Chen wrote:
> On Mon, 23 Jan 2023 16:07:32 +0800,
> Krzysztof Kozlowski wrote:
>>
>> On 23/01/2023 08:32, Li Chen wrote:
>>> Create a vendor directory for Ambarella, and add
>>> cpuid, rct, scratchpad documents.
>>>
>>> Signed-off-by: Li Chen <lchen@ambarella.com>
>>> Change-Id: I2c29e45c08666489b0d9b588ac37d713f5b723d1
>>
>> Please run scripts/checkpatch.pl and fix reported warnings.
>>
>> Applies to all your patches. Also test them... I have doubts that you
>> tested if you actually ignored checkpatch :/
> 
> Yeah, I checkpatch all patches, and have planned to fix Change-Id finally(manually),
> but forget it before sending mails, my bad, sorry. I will remove it in v2.
> 
>>> ---
>>>  .../arm/ambarella/ambarella,cpuid.yaml        | 24 +++++++++++++++++++
>>>  .../bindings/arm/ambarella/ambarella,rct.yaml | 24 +++++++++++++++++++
>>>  .../arm/ambarella/ambarella,scratchpad.yaml   | 24 +++++++++++++++++++
>>>  .../bindings/arm/ambarella/ambarella.yaml     | 22 +++++++++++++++++
>>>  MAINTAINERS                                   |  4 ++++
>>>  5 files changed, 98 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella,scratchpad.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/arm/ambarella/ambarella.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
>>> new file mode 100644
>>> index 000000000000..1f4d9cec8f92
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
>>
>> This goes to soc
> 
> Thanks, I wasn't aware that there is a document dir named soc. I will move cpuid yaml
> to bindings/soc/ambarella/, and leave other yaml still here.

However if device has chip identification features (chipid), then the
location is "hwinfo".

> 
>>> @@ -0,0 +1,24 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/ambarella,cpuid.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Ambarella SoC ID
>>> +
>>> +maintainers:
>>> +  - Li Chen <lchen@ambarella.com>
>>
>> Missing description.
> 
> Sorry, description will be added in v2. BTW, does other YAMLs in this patch
> also need descriptions?

In general yes - we want descriptions which will bring additional
information. Description should not repeat title, but add more data. For
trivial cases - maybe actually this one SoC ID - you can skip it.



> 
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: "ambarella,cpuid", "syscon"
>>
>> Drop quotes (applies to all your patches)
> 
> OK, thanks!
> 
>> Missing SoC specific compatible.
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>
>> Missing additionalProperties. sorry, start from scratch from some
>> existing recent bindings or better example-schema.
> 
> Good to know that there is example-schema, thanks!
>  
>>> +
>>> +examples:
>>> +  - |
>>> +    cpuid_syscon: cpuid@e0000000 {
>>> +        compatible = "ambarella,cpuid", "syscon";
>>> +        reg = <0xe0000000 0x1000>;
>>> +    };
>>> diff --git a/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
>>> new file mode 100644
>>> index 000000000000..7279bab17d9e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
>>> @@ -0,0 +1,24 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/ambarella,rct.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Ambarella RCT module
>>> +
>>> +maintainers:
>>> +  - Li Chen <lchen@ambarella.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: "ambarella,rct", "syscon"
>>
>> All the same problems.
> 
> Well noted.
> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +examples:
>>> +  - |
>>> +		rct_syscon: rct_syscon@ed080000 {
>>
>> Really? Just take a look and you will see wrong indentation. Also drop
>> underscores in node names and "rct". Node names should be generic.
> 
> Sorry for the wrong indentation, will fix it in v2.
> 
> Is it ok to contain underscores in lable? if so, I will change it into
> 
> rct_syscon: syscon@ed080000 {

Yes, label can have it.

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> This patch introduce clock bindings for Ambarella.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: I29018a23ed3a5b79a1103e859a5c7ed7bb83a261

All the same problems plus new:

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

> ---
>  .../clock/ambarella,composite-clock.yaml      | 52 ++++++++++++++++
>  .../bindings/clock/ambarella,pll-clock.yaml   | 59 +++++++++++++++++++
>  MAINTAINERS                                   |  2 +
>  3 files changed, 113 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> new file mode 100644
> index 000000000000..fac1cb9379c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ambarella,composite-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella Composite Clock
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +

Missing description.

> +properties:
> +  compatible:
> +    items:

Drop items.

> +      - const: ambarella,composite-clock

Missing SoC specific compatible. This is anyway not really correct
compatible...

> +
> +  clocks: true

No, needs constraints.

> +  assigned-clocks: true
> +  assigned-clock-parents: true
> +  assigned-clock-rates: true

Drop these three.

> +  clock-output-names: true

Missing constraints.

> +  amb,mux-regmap: true

NAK.

It's enough. The patches have very, very poor quality.

Missing description, missing type/$ref, wrong prefix.

> +  amb,div-regmap: true
> +  amb,div-width: true
> +  amb,div-shift: true

These two are arguments to phandle.

> +
> +  '#clock-cells':
> +    const: 0
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - '#clock-cells'
> +
> +additionalProperties: false

So why you decided to add it here and not in other places?
> +
> +examples:
> +  - |
> +      gclk_uart0: gclk-uart0 {

Wrong indentation.

> +        #clock-cells = <0>;
> +        compatible = "ambarella,composite-clock";
> +        clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>;
> +        clock-output-names = "gclk_uart0";
> +        assigned-clocks = <&gclk_uart0>;
> +        assigned-clock-parents = <&osc>;
> +        assigned-clock-rates = <24000000>;
> +        amb,mux-regmap = <&rct_syscon 0x1c8>;
> +        amb,div-regmap = <&rct_syscon 0x038>;
> +        amb,div-width = <24>;
> +        amb,div-shift = <0>;
> +      };
> diff --git a/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> new file mode 100644
> index 000000000000..65c1feb60041
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> @@ -0,0 +1,59 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ambarella,pll-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella PLL Clock
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ambarella,pll-clock
> +      - ambarella,clkpll-v0
> +
> +if:

No, this does not work like that. It sits under "allOf", located after
"required:".

> +  properties:
> +    compatible:
> +      const: ambarella,pll-clock
> +
> +then:
> +  properties:
> +    clocks:
> +      maxItems: 1
> +
> +    clock-output-names: true
> +    amb,clk-regmap: true
> +    amb,frac-mode: true
> +    assigned-clocks: true
> +    assigned-clock-rates: true

Same problems.

> +    gclk_axi: gclk-axi {
> +        #clock-cells = <0>;
> +        compatible = "fixed-factor-clock";

What is this example about? Not related at all. Provide real example.



Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

Hi Krzysztof,

Sorry for my late reply.

On Mon, 23 Jan 2023 16:11:08 +0800,
Krzysztof Kozlowski wrote:
>
> On 23/01/2023 08:32, Li Chen wrote:
> > This patch introduce clock bindings for Ambarella.
> >
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> > Change-Id: I29018a23ed3a5b79a1103e859a5c7ed7bb83a261
>
> All the same problems plus new:
>
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.

Well noted.

> > ---
> >  .../clock/ambarella,composite-clock.yaml      | 52 ++++++++++++++++
> >  .../bindings/clock/ambarella,pll-clock.yaml   | 59 +++++++++++++++++++
> >  MAINTAINERS                                   |  2 +
> >  3 files changed, 113 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> >  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> > new file mode 100644
> > index 000000000000..fac1cb9379c4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,composite-clock.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella Composite Clock
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
>
> Missing description.

Thanks, description as below will be added in v2:

"Ambarella SoCs integrates some composite clocks, like uart0, which aggrate the functionality
of the basic clock types, like mux and div."

> > +properties:
> > +  compatible:
> > +    items:
>
> Drop items.

Ok.

> > +      - const: ambarella,composite-clock
>
> Missing SoC specific compatible. This is anyway not really correct
> compatible...

Most Ambarella's compatibles don't contain SoC name, because we prefer
to use syscon + offsets in dts to tell driver the correct register offsets, or
ues struct soc_device and SoC identity stores in a given physical address.

So compatibles like "ambarella,composite-clock" and "ambarella,pinctrl" are
used widely in Ambarella kernels. Feel free to correct me if you think this
is not a good idea.

> > +
> > +  clocks: true
>
> No, needs constraints.

Ok. I will list all clocks name

> > +  assigned-clocks: true
> > +  assigned-clock-parents: true
> > +  assigned-clock-rates: true
>
> Drop these three.

Ok

> > +  clock-output-names: true
>
> Missing constraints.

Ok, I will add "maxItems: 1"

> > +  amb,mux-regmap: true
>
> NAK.
>
> It's enough. The patches have very, very poor quality.
>
> Missing description, missing type/$ref, wrong prefix.

Sorry, I forget to run dt_binding_check, I will spend some
time learning the binding and check, sorry for it.

> > +  amb,div-regmap: true
> > +  amb,div-width: true
> > +  amb,div-shift: true
>
> These two are arguments to phandle.

I will add description and $ref to regmap and width/shift.

> > +
> > +  '#clock-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - '#clock-cells'
> > +
> > +additionalProperties: false
>
> So why you decided to add it here and not in other places?

I didn't understand it well. I will add it to other places in v2,
thanks for pointint out it.

> > +
> > +examples:
> > +  - |
> > +      gclk_uart0: gclk-uart0 {
>
> Wrong indentation.

Well noted.

> > +        #clock-cells = <0>;
> > +        compatible = "ambarella,composite-clock";
> > +        clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>;
> > +        clock-output-names = "gclk_uart0";
> > +        assigned-clocks = <&gclk_uart0>;
> > +        assigned-clock-parents = <&osc>;
> > +        assigned-clock-rates = <24000000>;
> > +        amb,mux-regmap = <&rct_syscon 0x1c8>;
> > +        amb,div-regmap = <&rct_syscon 0x038>;
> > +        amb,div-width = <24>;
> > +        amb,div-shift = <0>;
> > +      };
> > diff --git a/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> > new file mode 100644
> > index 000000000000..65c1feb60041
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> > @@ -0,0 +1,59 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/ambarella,pll-clock.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella PLL Clock
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - ambarella,pll-clock
> > +      - ambarella,clkpll-v0
> > +
> > +if:
>
> No, this does not work like that. It sits under "allOf", located after
> "required:".

Thanks, I will learn "allOf" and use it in v2. BTW, we use the two compatibles as below:
clocks {
                compatible = "ambarella,clkpll-v0";
                ...
                gclk_core: gclk-core {
                        #clock-cells = <0>;
                        compatible = "ambarella,pll-clock";
                        clocks = <&osc>;
                        clock-output-names = "gclk_core";
                        amb,clk-regmap = <&rct_syscon 0x000 0x004 0x100 0x104 0x000 0x000>;
                };
                ...
}

I'm not sure can I describe the two compatibles in this single yaml, can you give some advice? thanks!

> > +  properties:
> > +    compatible:
> > +      const: ambarella,pll-clock
> > +
> > +then:
> > +  properties:
> > +    clocks:
> > +      maxItems: 1
> > +
> > +    clock-output-names: true
> > +    amb,clk-regmap: true
> > +    amb,frac-mode: true
> > +    assigned-clocks: true
> > +    assigned-clock-rates: true
>
> Same problems.

Ok.

> > +    gclk_axi: gclk-axi {
> > +        #clock-cells = <0>;
> > +        compatible = "fixed-factor-clock";
>
> What is this example about? Not related at all. Provide real example.

Sorry, I paste the wrong example, I will replace it with our gclk_core pll
instead.

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 25/01/2023 10:28, Li Chen wrote:
> 
> Hi Krzysztof,
> 
> Sorry for my late reply.
> 
> On Mon, 23 Jan 2023 16:11:08 +0800,
> Krzysztof Kozlowski wrote:
>>
>> On 23/01/2023 08:32, Li Chen wrote:
>>> This patch introduce clock bindings for Ambarella.
>>>
>>> Signed-off-by: Li Chen <lchen@ambarella.com>
>>> Change-Id: I29018a23ed3a5b79a1103e859a5c7ed7bb83a261
>>
>> All the same problems plus new:
>>
>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>> prefix is already stating that these are bindings.
> 
> Well noted.
> 
>>> ---
>>>  .../clock/ambarella,composite-clock.yaml      | 52 ++++++++++++++++
>>>  .../bindings/clock/ambarella,pll-clock.yaml   | 59 +++++++++++++++++++
>>>  MAINTAINERS                                   |  2 +
>>>  3 files changed, 113 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
>>>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
>>> new file mode 100644
>>> index 000000000000..fac1cb9379c4
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
>>> @@ -0,0 +1,52 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/ambarella,composite-clock.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Ambarella Composite Clock
>>> +
>>> +maintainers:
>>> +  - Li Chen <lchen@ambarella.com>
>>> +
>>
>> Missing description.
> 
> Thanks, description as below will be added in v2:
> 
> "Ambarella SoCs integrates some composite clocks, like uart0, which aggrate the functionality
> of the basic clock types, like mux and div."
> 
>>> +properties:
>>> +  compatible:
>>> +    items:
>>
>> Drop items.
> 
> Ok.
> 
>>> +      - const: ambarella,composite-clock
>>
>> Missing SoC specific compatible. This is anyway not really correct
>> compatible...
> 
> Most Ambarella's compatibles don't contain SoC name, because we prefer
> to use syscon + offsets in dts to tell driver the correct register offsets, or
> ues struct soc_device and SoC identity stores in a given physical address.

That's not correct hardware description. Drop the syscon and offsets.

> 
> So compatibles like "ambarella,composite-clock" and "ambarella,pinctrl" are
> used widely in Ambarella kernels. 

What do you do downstream does not matter. You can invent any crazy idea
and it is not an argument that it should be done like that. Usually
downstream code is incorrect...

> Feel free to correct me if you think this
> is not a good idea.

This is bad idea. Compatibles should be specific. Devices should not use
syscons to poke other registers, unless strictly necessary, but have
strictly defined MMIO address space and use it.

> 
>>> +
>>> +  clocks: true
>>
>> No, needs constraints.
> 
> Ok. I will list all clocks name
> 
>>> +  assigned-clocks: true
>>> +  assigned-clock-parents: true
>>> +  assigned-clock-rates: true
>>
>> Drop these three.
> 
> Ok
> 
>>> +  clock-output-names: true
>>
>> Missing constraints.
> 
> Ok, I will add "maxItems: 1"
> 
>>> +  amb,mux-regmap: true
>>
>> NAK.
>>
>> It's enough. The patches have very, very poor quality.
>>
>> Missing description, missing type/$ref, wrong prefix.
> 
> Sorry, I forget to run dt_binding_check, I will spend some
> time learning the binding and check, sorry for it.
> 
>>> +  amb,div-regmap: true
>>> +  amb,div-width: true
>>> +  amb,div-shift: true
>>
>> These two are arguments to phandle.
> 
> I will add description and $ref to regmap and width/shift.

Drop all these syscon properties.

> 
>>> +
>>> +  '#clock-cells':
>>> +    const: 0
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - clocks
>>> +  - '#clock-cells'
>>> +
>>> +additionalProperties: false
>>
>> So why you decided to add it here and not in other places?
> 
> I didn't understand it well. I will add it to other places in v2,
> thanks for pointint out it.
> 
>>> +
>>> +examples:
>>> +  - |
>>> +      gclk_uart0: gclk-uart0 {
>>
>> Wrong indentation.
> 
> Well noted.
> 
>>> +        #clock-cells = <0>;
>>> +        compatible = "ambarella,composite-clock";
>>> +        clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>;
>>> +        clock-output-names = "gclk_uart0";
>>> +        assigned-clocks = <&gclk_uart0>;
>>> +        assigned-clock-parents = <&osc>;
>>> +        assigned-clock-rates = <24000000>;
>>> +        amb,mux-regmap = <&rct_syscon 0x1c8>;
>>> +        amb,div-regmap = <&rct_syscon 0x038>;
>>> +        amb,div-width = <24>;
>>> +        amb,div-shift = <0>;
>>> +      };
>>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
>>> new file mode 100644
>>> index 000000000000..65c1feb60041
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
>>> @@ -0,0 +1,59 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/clock/ambarella,pll-clock.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Ambarella PLL Clock
>>> +
>>> +maintainers:
>>> +  - Li Chen <lchen@ambarella.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ambarella,pll-clock
>>> +      - ambarella,clkpll-v0
>>> +
>>> +if:
>>
>> No, this does not work like that. It sits under "allOf", located after
>> "required:".
> 
> Thanks, I will learn "allOf" and use it in v2. BTW, we use the two compatibles as below:
> clocks {
>                 compatible = "ambarella,clkpll-v0";

Nope.

>                 ...
>                 gclk_core: gclk-core {
>                         #clock-cells = <0>;
>                         compatible = "ambarella,pll-clock";

Also nope.

>                         clocks = <&osc>;
>                         clock-output-names = "gclk_core";
>                         amb,clk-regmap = <&rct_syscon 0x000 0x004 0x100 0x104 0x000 0x000>;

Nope, nope, nope.

You need proper clock-controller with its own MMIO address space.

>                 };
>                 ...
> }
> 
> I'm not sure can I describe the two compatibles in this single yaml, can you give some advice? thanks!

There are plenty of examples, including example-schema.

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

On Wed, 25 Jan 2023 17:55:34 +0800,
Hi Krzysztof,

Krzysztof Kozlowski wrote:
>
> On 25/01/2023 10:28, Li Chen wrote:
> >
> > Hi Krzysztof,
> >
> > Sorry for my late reply.
> >
> > On Mon, 23 Jan 2023 16:11:08 +0800,
> > Krzysztof Kozlowski wrote:
> >>
> >> On 23/01/2023 08:32, Li Chen wrote:
> >>> This patch introduce clock bindings for Ambarella.
> >>>
> >>> Signed-off-by: Li Chen <lchen@ambarella.com>
> >>> Change-Id: I29018a23ed3a5b79a1103e859a5c7ed7bb83a261
> >>
> >> All the same problems plus new:
> >>
> >> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> >> prefix is already stating that these are bindings.
> >
> > Well noted.
> >
> >>> ---
> >>>  .../clock/ambarella,composite-clock.yaml      | 52 ++++++++++++++++
> >>>  .../bindings/clock/ambarella,pll-clock.yaml   | 59 +++++++++++++++++++
> >>>  MAINTAINERS                                   |  2 +
> >>>  3 files changed, 113 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> >>>  create mode 100644 Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> >>> new file mode 100644
> >>> index 000000000000..fac1cb9379c4
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml
> >>> @@ -0,0 +1,52 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/clock/ambarella,composite-clock.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Ambarella Composite Clock
> >>> +
> >>> +maintainers:
> >>> +  - Li Chen <lchen@ambarella.com>
> >>> +
> >>
> >> Missing description.
> >
> > Thanks, description as below will be added in v2:
> >
> > "Ambarella SoCs integrates some composite clocks, like uart0, which aggrate the functionality
> > of the basic clock types, like mux and div."
> >
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>
> >> Drop items.
> >
> > Ok.
> >
> >>> +      - const: ambarella,composite-clock
> >>
> >> Missing SoC specific compatible. This is anyway not really correct
> >> compatible...
> >
> > Most Ambarella's compatibles don't contain SoC name, because we prefer
> > to use syscon + offsets in dts to tell driver the correct register offsets, or
> > ues struct soc_device and SoC identity stores in a given physical address.
>
> That's not correct hardware description. Drop the syscon and offsets.

Ok.

> > 
> > So compatibles like "ambarella,composite-clock" and "ambarella,pinctrl" are
> > used widely in Ambarella kernels.
>
> What do you do downstream does not matter. You can invent any crazy idea
> and it is not an argument that it should be done like that. Usually
> downstream code is incorrect...

Yeah, I understand it.
I really hope to learn the standard/right ways and
I am very grateful for your careful reviews.

> > Feel free to correct me if you think this
> > is not a good idea.
>
> This is bad idea. Compatibles should be specific. Devices should not use
> syscons to poke other registers, unless strictly necessary, but have
> strictly defined MMIO address space and use it.

Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.

But I have three questions:

0. why syscon + offsets is a bad idea copared to specific compatibles?
1. when would it be a good idea to use syscon in device tree?
2. syscon VS reg, which is preferred in device tree?

Thanks in advanced.

> >
> >>> +
> >>> +  clocks: true
> >>
> >> No, needs constraints.
> >
> > Ok. I will list all clocks name
> >
> >>> +  assigned-clocks: true
> >>> +  assigned-clock-parents: true
> >>> +  assigned-clock-rates: true
> >>
> >> Drop these three.
> >
> > Ok
> >
> >>> +  clock-output-names: true
> >>
> >> Missing constraints.
> >
> > Ok, I will add "maxItems: 1"
> >
> >>> +  amb,mux-regmap: true
> >>
> >> NAK.
> >>
> >> It's enough. The patches have very, very poor quality.
> >>
> >> Missing description, missing type/$ref, wrong prefix.
> >
> > Sorry, I forget to run dt_binding_check, I will spend some
> > time learning the binding and check, sorry for it.
> >
> >>> +  amb,div-regmap: true
> >>> +  amb,div-width: true
> >>> +  amb,div-shift: true
> >>
> >> These two are arguments to phandle.
> >
> > I will add description and $ref to regmap and width/shift.
>
> Drop all these syscon properties.

Ok, so I should replace these regmaps with reg, right?

> >
> >>> +
> >>> +  '#clock-cells':
> >>> +    const: 0
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - clocks
> >>> +  - '#clock-cells'
> >>> +
> >>> +additionalProperties: false
> >>
> >> So why you decided to add it here and not in other places?
> >
> > I didn't understand it well. I will add it to other places in v2,
> > thanks for pointint out it.
> >
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +      gclk_uart0: gclk-uart0 {
> >>
> >> Wrong indentation.
> >
> > Well noted.
> >
> >>> +        #clock-cells = <0>;
> >>> +        compatible = "ambarella,composite-clock";
> >>> +        clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>;
> >>> +        clock-output-names = "gclk_uart0";
> >>> +        assigned-clocks = <&gclk_uart0>;
> >>> +        assigned-clock-parents = <&osc>;
> >>> +        assigned-clock-rates = <24000000>;
> >>> +        amb,mux-regmap = <&rct_syscon 0x1c8>;
> >>> +        amb,div-regmap = <&rct_syscon 0x038>;
> >>> +        amb,div-width = <24>;
> >>> +        amb,div-shift = <0>;
> >>> +      };
> >>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> >>> new file mode 100644
> >>> index 000000000000..65c1feb60041
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml
> >>> @@ -0,0 +1,59 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/clock/ambarella,pll-clock.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Ambarella PLL Clock
> >>> +
> >>> +maintainers:
> >>> +  - Li Chen <lchen@ambarella.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    enum:
> >>> +      - ambarella,pll-clock
> >>> +      - ambarella,clkpll-v0
> >>> +
> >>> +if:
> >>
> >> No, this does not work like that. It sits under "allOf", located after
> >> "required:".
> >
> > Thanks, I will learn "allOf" and use it in v2. BTW, we use the two compatibles as below:
> > clocks {
> >                 compatible = "ambarella,clkpll-v0";
>
> Nope.
>
> >                 ...
> >                 gclk_core: gclk-core {
> >                         #clock-cells = <0>;
> >                         compatible = "ambarella,pll-clock";
>
> Also nope.
>
> >                         clocks = <&osc>;
> >                         clock-output-names = "gclk_core";
> >                         amb,clk-regmap = <&rct_syscon 0x000 0x004 0x100 0x104 0x000 0x000>;
>
> Nope, nope, nope.
>
> You need proper clock-controller with its own MMIO address space.
>
> >                 };
> >                 ...
> > }
> >
> > I'm not sure can I describe the two compatibles in this single yaml, can you give some advice? thanks!
>
> There are plenty of examples, including example-schema.

Ok, I will learn more and fix it.

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 25/01/2023 13:06, Li Chen wrote:
>>> Feel free to correct me if you think this
>>> is not a good idea.
>>
>> This is bad idea. Compatibles should be specific. Devices should not use
>> syscons to poke other registers, unless strictly necessary, but have
>> strictly defined MMIO address space and use it.
> 
> Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.
> 
> But I have three questions:
> 
> 0. why syscon + offsets is a bad idea copared to specific compatibles?

Specific compatibles are a requirement. They are needed to match device
in exact way, not some generic and unspecific. The same with every other
interface, it must be specific to allow only correct usage.

It's of course different with generic fallbacks, but we do not talk
about them here...

> 1. when would it be a good idea to use syscon in device tree?

When your device needs to poke one or few registers from some
system-controller block.

> 2. syscon VS reg, which is preferred in device tree?

There is no such choice. Your DTS *must* describe the hardware. The
hardware description is for example clock controller which has its own
address space. If you now do not add clock controller's address space to
the clock controller, it is not a proper hardware description. The same
with every other property. If your device has interrupts, but you do not
add them, it is not correct description.

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

On Wed, 25 Jan 2023 20:14:16 +0800,

Hi Krzysztof,

Krzysztof Kozlowski wrote:
>
> On 25/01/2023 13:06, Li Chen wrote:
> >>> Feel free to correct me if you think this
> >>> is not a good idea.
> >>
> >> This is bad idea. Compatibles should be specific. Devices should not use
> >> syscons to poke other registers, unless strictly necessary, but have
> >> strictly defined MMIO address space and use it.
> >
> > Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.
> >
> > But I have three questions:
> >
> > 0. why syscon + offsets is a bad idea copared to specific compatibles?
>
> Specific compatibles are a requirement. They are needed to match device
> in exact way, not some generic and unspecific. The same with every other
> interface, it must be specific to allow only correct usage.
>
> It's of course different with generic fallbacks, but we do not talk
> about them here...
>
> > 1. when would it be a good idea to use syscon in device tree?
>
> When your device needs to poke one or few registers from some
> system-controller block.
>
> > 2. syscon VS reg, which is preferred in device tree?
>
> There is no such choice. Your DTS *must* describe the hardware. The
> hardware description is for example clock controller which has its own
> address space. If you now do not add clock controller's address space to
> the clock controller, it is not a proper hardware description. The same
> with every other property. If your device has interrupts, but you do not
> add them, it is not correct description.

Got it. But Ambarella hardware design is kind of strange. I want to add mroe
expalaination about why Ambarella's downstream kernel
use so much syscon in device trees:

For most SoCs from other vendors, they have seperate address space regions
for different peripherals, like
axi address space A: ENET
axi address space B: PCIe
axi address space B: USB
...

Ambarella is somewhat **different**, its SoCs have two system controllers regions:
RCT and scratchpad, take RCT for example:
"The S6LM system software
interacts with PLLs, PHYs and several other low-level hardware blocks using APB reset clock and test (RCT)
registers with a system-layer application programming interface (API).
This includes the setting of clock frequencies."

There are so many peripherals registers located inside RCT and scratchpad
(like usb/phy, gpio, sd, dac, enet, rng), and some peripherals even have no their
own modules for register definitions.

So most time(for a peripheral driver), the only differences between different
Ambarella SoCs are just the syscon(rct or scratchpad) offsets get changed.

I don't think such lazy hardware design is common in vendors other than ambarella.

If I switch to SoC-specific compatibles, and remove these syscon from device tree,
of_device_id->data may only contain system controller(rct or scratchpad) offset for many Ambarella drivers,
and ioremap/devm_ioremap carefully.

The question is: can upstream kernel accept such codes?

If yes, I will switch to SoC-specific compatibles and remove syscon without hesitation.

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 25/01/2023 14:40, Li Chen wrote:
> On Wed, 25 Jan 2023 20:14:16 +0800,
> 
> Hi Krzysztof,
> 
> Krzysztof Kozlowski wrote:
>>
>> On 25/01/2023 13:06, Li Chen wrote:
>>>>> Feel free to correct me if you think this
>>>>> is not a good idea.
>>>>
>>>> This is bad idea. Compatibles should be specific. Devices should not use
>>>> syscons to poke other registers, unless strictly necessary, but have
>>>> strictly defined MMIO address space and use it.
>>>
>>> Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.
>>>
>>> But I have three questions:
>>>
>>> 0. why syscon + offsets is a bad idea copared to specific compatibles?
>>
>> Specific compatibles are a requirement. They are needed to match device
>> in exact way, not some generic and unspecific. The same with every other
>> interface, it must be specific to allow only correct usage.
>>
>> It's of course different with generic fallbacks, but we do not talk
>> about them here...
>>
>>> 1. when would it be a good idea to use syscon in device tree?
>>
>> When your device needs to poke one or few registers from some
>> system-controller block.
>>
>>> 2. syscon VS reg, which is preferred in device tree?
>>
>> There is no such choice. Your DTS *must* describe the hardware. The
>> hardware description is for example clock controller which has its own
>> address space. If you now do not add clock controller's address space to
>> the clock controller, it is not a proper hardware description. The same
>> with every other property. If your device has interrupts, but you do not
>> add them, it is not correct description.
> 
> Got it. But Ambarella hardware design is kind of strange. I want to add mroe
> expalaination about why Ambarella's downstream kernel
> use so much syscon in device trees:
> 
> For most SoCs from other vendors, they have seperate address space regions
> for different peripherals, like
> axi address space A: ENET
> axi address space B: PCIe
> axi address space B: USB
> ...
> 
> Ambarella is somewhat **different**, its SoCs have two system controllers regions:
> RCT and scratchpad, take RCT for example:
> "The S6LM system software
> interacts with PLLs, PHYs and several other low-level hardware blocks using APB reset clock and test (RCT)
> registers with a system-layer application programming interface (API).
> This includes the setting of clock frequencies."
> 
> There are so many peripherals registers located inside RCT and scratchpad
> (like usb/phy, gpio, sd, dac, enet, rng), and some peripherals even have no their
> own modules for register definitions.

Then the syscon is the parent device of these peripherals and clocks.
You did not represent them as children but as siblings which does not
look correct.

> 
> So most time(for a peripheral driver), the only differences between different
> Ambarella SoCs are just the syscon(rct or scratchpad) offsets get changed.
> 
> I don't think such lazy hardware design is common in vendors other than ambarella.
> 
> If I switch to SoC-specific compatibles, 

This is independent topic. SoC-specific compatibles are a requirement
but it does not affect your device hierarchy.

> and remove these syscon from device tree,
> of_device_id->data may only contain system controller(rct or scratchpad) offset for many Ambarella drivers,
> and ioremap/devm_ioremap carefully.

I don't understand the problem. Neither the solution.

> 
> The question is: can upstream kernel accept such codes?
> 
> If yes, I will switch to SoC-specific compatibles and remove syscon without hesitation.

Sorry, none of your explanations here match your DTS. Your DTS clearly
models (for some reason there is no soc which makes even bigger confusion):

rct_syscon
clocks
 |-gclk-core
 |-gclk-ddr

but what you are saying is that there is no separate clock controller
device with its own IO address but these clocks are part of rct_syscon.
Then model it that way in DTS. The rct_syscon is then your clock
controller and all these fake gclk-core and gclk-ddr nodes should be gone.

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

Hi Krzysztof,
 ---- On Thu, 26 Jan 2023 19:29:05 +0800  Krzysztof Kozlowski  wrote --- 
 > On 25/01/2023 14:40, Li Chen wrote:
 > > On Wed, 25 Jan 2023 20:14:16 +0800,
 > > 
 > > Hi Krzysztof,
 > > 
 > > Krzysztof Kozlowski wrote:
 > >>
 > >> On 25/01/2023 13:06, Li Chen wrote:
 > >>>>> Feel free to correct me if you think this
 > >>>>> is not a good idea.
 > >>>>
 > >>>> This is bad idea. Compatibles should be specific. Devices should not use
 > >>>> syscons to poke other registers, unless strictly necessary, but have
 > >>>> strictly defined MMIO address space and use it.
 > >>>
 > >>> Ok, I will convert syscon-based regmaps to SoC-specific compatibles and of_device_id->data.
 > >>>
 > >>> But I have three questions:
 > >>>
 > >>> 0. why syscon + offsets is a bad idea copared to specific compatibles?
 > >>
 > >> Specific compatibles are a requirement. They are needed to match device
 > >> in exact way, not some generic and unspecific. The same with every other
 > >> interface, it must be specific to allow only correct usage.
 > >>
 > >> It's of course different with generic fallbacks, but we do not talk
 > >> about them here...
 > >>
 > >>> 1. when would it be a good idea to use syscon in device tree?
 > >>
 > >> When your device needs to poke one or few registers from some
 > >> system-controller block.
 > >>
 > >>> 2. syscon VS reg, which is preferred in device tree?
 > >>
 > >> There is no such choice. Your DTS *must* describe the hardware. The
 > >> hardware description is for example clock controller which has its own
 > >> address space. If you now do not add clock controller's address space to
 > >> the clock controller, it is not a proper hardware description. The same
 > >> with every other property. If your device has interrupts, but you do not
 > >> add them, it is not correct description.
 > > 
 > > Got it. But Ambarella hardware design is kind of strange. I want to add mroe
 > > expalaination about why Ambarella's downstream kernel
 > > use so much syscon in device trees:
 > > 
 > > For most SoCs from other vendors, they have seperate address space regions
 > > for different peripherals, like
 > > axi address space A: ENET
 > > axi address space B: PCIe
 > > axi address space B: USB
 > > ...
 > > 
 > > Ambarella is somewhat **different**, its SoCs have two system controllers regions:
 > > RCT and scratchpad, take RCT for example:
 > > "The S6LM system software
 > > interacts with PLLs, PHYs and several other low-level hardware blocks using APB reset clock and test (RCT)
 > > registers with a system-layer application programming interface (API).
 > > This includes the setting of clock frequencies."
 > > 
 > > There are so many peripherals registers located inside RCT and scratchpad
 > > (like usb/phy, gpio, sd, dac, enet, rng), and some peripherals even have no their
 > > own modules for register definitions.
 > 
 > Then the syscon is the parent device of these peripherals and clocks.
 > You did not represent them as children but as siblings which does not
 > look correct.
 
Ok, I will these syscon(RCT  and scratchpad) as the parent node of our clocks and related peripherals.

 > > 
 > > So most time(for a peripheral driver), the only differences between different
 > > Ambarella SoCs are just the syscon(rct or scratchpad) offsets get changed.
 > > 
 > > I don't think such lazy hardware design is common in vendors other than ambarella.
 > > 
 > > If I switch to SoC-specific compatibles, 
 > 
 > This is independent topic. SoC-specific compatibles are a requirement
 > but it does not affect your device hierarchy.
 
Thanks, "requirement" makes things much more clear. So I will always use SoC-specific compatibles even
if different Amarella SoCs may share the same reg offset and setting.

 > > and remove these syscon from device tree,
 > > of_device_id->data may only contain system controller(rct or scratchpad) offset for many Ambarella drivers,
 > > and ioremap/devm_ioremap carefully.
 > 
 > I don't understand the problem. Neither the solution.
 > 
 > > 
 > > The question is: can upstream kernel accept such codes?
 > > 
 > > If yes, I will switch to SoC-specific compatibles and remove syscon without hesitation.
 > 
 > Sorry, none of your explanations here match your DTS. Your DTS clearly
 > models (for some reason there is no soc which makes even bigger confusion):
 > 
 > rct_syscon
 > clocks
 >  |-gclk-core
 >  |-gclk-ddr
 > 
 > but what you are saying is that there is no separate clock controller
 > device with its own IO address but these clocks are part of rct_syscon.
 > Then model it that way in DTS. The rct_syscon is then your clock
 > controller and all these fake gclk-core and gclk-ddr nodes should be gone.

Ok, I will remove these fake nodes, and model the hardware as:

rct_syscon node
| clock node(pll, div, mux, composite  clocks live in the same driver)
| other periphal nodes

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 27/01/2023 15:48, Li Chen wrote:
>  > 
>  > but what you are saying is that there is no separate clock controller
>  > device with its own IO address but these clocks are part of rct_syscon.
>  > Then model it that way in DTS. The rct_syscon is then your clock
>  > controller and all these fake gclk-core and gclk-ddr nodes should be gone.
> 
> Ok, I will remove these fake nodes, and model the hardware as:
> 
> rct_syscon node
> | clock node(pll, div, mux, composite  clocks live in the same driver)
> | other periphal nodes

You need clock node if it takes any resources. If it doesn't, you do not
need it.

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

Hi Krzysztof,

 ---- On Fri, 27 Jan 2023 23:08:09 +0800  Krzysztof Kozlowski  wrote --- 
 > On 27/01/2023 15:48, Li Chen wrote:
 > >  > 
 > >  > but what you are saying is that there is no separate clock controller
 > >  > device with its own IO address but these clocks are part of rct_syscon.
 > >  > Then model it that way in DTS. The rct_syscon is then your clock
 > >  > controller and all these fake gclk-core and gclk-ddr nodes should be gone.
 > > 
 > > Ok, I will remove these fake nodes, and model the hardware as:
 > > 
 > > rct_syscon node
 > > | clock node(pll, div, mux, composite  clocks live in the same driver)
 > > | other periphal nodes
 > 
 > You need clock node if it takes any resources. If it doesn't, you do not
 > need it.

Got it, I will model it as:

rct_syscon(compatible include "ambarella, <SoC>-clock"...)
| peripheral A
| peripheral B
| ...


One more question, two driver models:
a. compatible = "ambarella, <SoC>-clock", handle all clocks(pll, div, mux, composite) in single driver.
b. compatible = "ambarella, <SoC>-pll-clock", "ambarella, <SoC>-composite-clock", "ambarella, <SoC>-div-clock"...... 
    and implement a driver for each of them.

Which driver model is preferred?

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 28/01/2023 10:42, Li Chen wrote:
> Got it, I will model it as:
> 
> rct_syscon(compatible include "ambarella, <SoC>-clock"...)
> | peripheral A
> | peripheral B
> | ...
> 
> 
> One more question, two driver models:
> a. compatible = "ambarella, <SoC>-clock", handle all clocks(pll, div, mux, composite) in single driver.
> b. compatible = "ambarella, <SoC>-pll-clock", "ambarella, <SoC>-composite-clock", "ambarella, <SoC>-div-clock"...... 
>     and implement a driver for each of them.
> 
> Which driver model is preferred?

We do not talk here at all about drivers. This is independent and not
really related.

Anyway, independent features mostly have separate drivers. Each separate
driver should be located in respective subsystem. But again - we do not
talk here about drivers at all, so please do not bring them into the
problem. It will make everything more complicated...

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

Hi Krzysztof,

 ---- On Sat, 28 Jan 2023 18:08:00 +0800  Krzysztof Kozlowski  wrote --- 
 > On 28/01/2023 10:42, Li Chen wrote:
 > > Got it, I will model it as:
 > > 
 > > rct_syscon(compatible include "ambarella, -clock"...)
 > > | peripheral A
 > > | peripheral B
 > > | ...
 > > 
 > > 
 > > One more question, two driver models:
 > > a. compatible = "ambarella, -clock", handle all clocks(pll, div, mux, composite) in single driver.
 > > b. compatible = "ambarella, -pll-clock", "ambarella, -composite-clock", "ambarella, -div-clock"...... 
 > >     and implement a driver for each of them.
 > > 
 > > Which driver model is preferred?
 > 
 > We do not talk here at all about drivers. This is independent and not
 > really related.
 > 
 > Anyway, independent features mostly have separate drivers. Each separate
 > driver should be located in respective subsystem. But again - we do not
 > talk here about drivers at all, so please do not bring them into the
 > problem. It will make everything more complicated...
 
Ok, that makes sense. Sorry about that.

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

Hi Krzysztof ,

 ---- On Fri, 27 Jan 2023 23:08:09 +0800  Krzysztof Kozlowski  wrote --- 
 > On 27/01/2023 15:48, Li Chen wrote:
 > >  > 
 > >  > but what you are saying is that there is no separate clock controller
 > >  > device with its own IO address but these clocks are part of rct_syscon.
 > >  > Then model it that way in DTS. The rct_syscon is then your clock
 > >  > controller and all these fake gclk-core and gclk-ddr nodes should be gone.
 > > 
 > > Ok, I will remove these fake nodes, and model the hardware as:
 > > 
 > > rct_syscon node
 > > | clock node(pll, div, mux, composite  clocks live in the same driver)
 > > | other periphal nodes
 > 
 > You need clock node if it takes any resources. If it doesn't, you do not
 > need it.

If the only hardware resource the clock node can take is its parent clock(clocks = <&osc>;),
then can I have this clock node?

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 06/02/2023 12:28, Li Chen wrote:
> Hi Krzysztof ,
> 
>  ---- On Fri, 27 Jan 2023 23:08:09 +0800  Krzysztof Kozlowski  wrote --- 
>  > On 27/01/2023 15:48, Li Chen wrote:
>  > >  > 
>  > >  > but what you are saying is that there is no separate clock controller
>  > >  > device with its own IO address but these clocks are part of rct_syscon.
>  > >  > Then model it that way in DTS. The rct_syscon is then your clock
>  > >  > controller and all these fake gclk-core and gclk-ddr nodes should be gone.
>  > > 
>  > > Ok, I will remove these fake nodes, and model the hardware as:
>  > > 
>  > > rct_syscon node
>  > > | clock node(pll, div, mux, composite  clocks live in the same driver)
>  > > | other periphal nodes
>  > 
>  > You need clock node if it takes any resources. If it doesn't, you do not
>  > need it.
> 
> If the only hardware resource the clock node can take is its parent clock(clocks = <&osc>;),
> then can I have this clock node?

I am not sure if I understand. osc does not look like parent device, so
this part of comment confuses me.

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

Hi Krzysztof,
 ---- On Mon, 06 Feb 2023 21:41:44 +0800  Krzysztof Kozlowski  wrote --- 
 > On 06/02/2023 12:28, Li Chen wrote:
 > > Hi Krzysztof ,
 > > 
 > >  ---- On Fri, 27 Jan 2023 23:08:09 +0800  Krzysztof Kozlowski  wrote --- 
 > >  > On 27/01/2023 15:48, Li Chen wrote:
 > >  > >  > 
 > >  > >  > but what you are saying is that there is no separate clock controller
 > >  > >  > device with its own IO address but these clocks are part of rct_syscon.
 > >  > >  > Then model it that way in DTS. The rct_syscon is then your clock
 > >  > >  > controller and all these fake gclk-core and gclk-ddr nodes should be gone.
 > >  > > 
 > >  > > Ok, I will remove these fake nodes, and model the hardware as:
 > >  > > 
 > >  > > rct_syscon node
 > >  > > | clock node(pll, div, mux, composite  clocks live in the same driver)
 > >  > > | other periphal nodes
 > >  > 
 > >  > You need clock node if it takes any resources. If it doesn't, you do not
 > >  > need it.
 > > 
 > > If the only hardware resource the clock node can take is its parent clock(clocks = &osc;),
 > > then can I have this clock node?
 > 
 > I am not sure if I understand. osc does not look like parent device, so
 > this part of comment confuses me.

Sorry for the confusion. I mean osc is the root of clock tree:

osc
  | pll A
  | pll B
  | ...

So if I have a clock node under rct_syscon node, I think it should take osc as the parent(node) clock:
rct_syscon {
    ......
    clock_controller {
          clocks = <&osc>;
          ......

You have said "You need clock node if it takes any resources. ", do you think osc here can be counted as a used resource?

Regards,
Li

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 06/02/2023 15:57, Li Chen wrote:
> Hi Krzysztof,
>  ---- On Mon, 06 Feb 2023 21:41:44 +0800  Krzysztof Kozlowski  wrote --- 
>  > On 06/02/2023 12:28, Li Chen wrote:
>  > > Hi Krzysztof ,
>  > > 
>  > >  ---- On Fri, 27 Jan 2023 23:08:09 +0800  Krzysztof Kozlowski  wrote --- 
>  > >  > On 27/01/2023 15:48, Li Chen wrote:
>  > >  > >  > 
>  > >  > >  > but what you are saying is that there is no separate clock controller
>  > >  > >  > device with its own IO address but these clocks are part of rct_syscon.
>  > >  > >  > Then model it that way in DTS. The rct_syscon is then your clock
>  > >  > >  > controller and all these fake gclk-core and gclk-ddr nodes should be gone.
>  > >  > > 
>  > >  > > Ok, I will remove these fake nodes, and model the hardware as:
>  > >  > > 
>  > >  > > rct_syscon node
>  > >  > > | clock node(pll, div, mux, composite  clocks live in the same driver)
>  > >  > > | other periphal nodes
>  > >  > 
>  > >  > You need clock node if it takes any resources. If it doesn't, you do not
>  > >  > need it.
>  > > 
>  > > If the only hardware resource the clock node can take is its parent clock(clocks = &osc;),
>  > > then can I have this clock node?
>  > 
>  > I am not sure if I understand. osc does not look like parent device, so
>  > this part of comment confuses me.
> 
> Sorry for the confusion. I mean osc is the root of clock tree:
> 
> osc
>   | pll A
>   | pll B
>   | ...
> 
> So if I have a clock node under rct_syscon node, I think it should take osc as the parent(node) clock:
> rct_syscon {
>     ......
>     clock_controller {
>           clocks = <&osc>;
>           ......
> 
> You have said "You need clock node if it takes any resources. ", do you think osc here can be counted as a used resource?

Yes, in that matter osc should be the input to this clock controller.

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

On 27/01/2023 15:48, Li Chen wrote:
>  > This is independent topic. SoC-specific compatibles are a requirement
>  > but it does not affect your device hierarchy.
>  
> Thanks, "requirement" makes things much more clear. So I will always use SoC-specific compatibles even
> if different Amarella SoCs may share the same reg offset and setting.

Just please read before sending any new versions:
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst

Best regards,
Krzysztof

Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Li Chen]

Hi Krzysztof,

 ---- On Fri, 27 Jan 2023 23:11:26 +0800  Krzysztof Kozlowski  wrote --- 
 > On 27/01/2023 15:48, Li Chen wrote:
 > >  > This is independent topic. SoC-specific compatibles are a requirement
 > >  > but it does not affect your device hierarchy.
 > >  
 > > Thanks, "requirement" makes things much more clear. So I will always use SoC-specific compatibles even
 > > if different Amarella SoCs may share the same reg offset and setting.
 > 
 > Just please read before sending any new versions:
 > https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst

Gotcha.

Regards,
Li

Re: [PATCH 09/15] dt-bindings: serial: add support for Ambarella

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> Add compatible for Ambarella.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: I32513d98f52af0311dfb55dd5c4739a58f6b9fc1
> ---
>  .../bindings/serial/ambarella_uart.yaml       | 57 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/ambarella_uart.yaml
> 
> diff --git a/Documentation/devicetree/bindings/serial/ambarella_uart.yaml b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
> new file mode 100644
> index 000000000000..238d68078270
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
> @@ -0,0 +1,57 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/ambarella_uart.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella S6LM SoC UART Controller
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +
> +properties:
> +  compatible:
> +    const: ambarella,uart
> +
> +  reg:
> +    maxItems: 1
> +
> +  amb,ignore-fe:
> +    description: |
> +      ignore frame error report for CV2/CV22/CV25/S6LM because it's
> +      checked too strict so that normal stop may be treated as frame error.

Missing type. I don't understand why this is property of DT.

Anyway several problems mentioned earlier, please fix.


Best regards,
Krzysztof

Re: [PATCH 09/15] dt-bindings: serial: add support for Ambarella

[Li Chen]

Hi Krzysztof Kozlowski,

Sorry for my late reply.

On Mon, 23 Jan 2023 16:11:52 +0800,
Krzysztof Kozlowski wrote:
>
> On 23/01/2023 08:32, Li Chen wrote:
> > Add compatible for Ambarella.
> >
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> > Change-Id: I32513d98f52af0311dfb55dd5c4739a58f6b9fc1
> > ---
> >  .../bindings/serial/ambarella_uart.yaml       | 57 +++++++++++++++++++
> >  MAINTAINERS                                   |  1 +
> >  2 files changed, 58 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/serial/ambarella_uart.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/serial/ambarella_uart.yaml b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
> > new file mode 100644
> > index 000000000000..238d68078270
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
> > @@ -0,0 +1,57 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/serial/ambarella_uart.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Ambarella S6LM SoC UART Controller
> > +
> > +maintainers:
> > +  - Li Chen <lchen@ambarella.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: ambarella,uart
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  amb,ignore-fe:
> > +    description: |
> > +      ignore frame error report for CV2/CV22/CV25/S6LM because it's
> > +      checked too strict so that normal stop may be treated as frame error.
>
> Missing type. I don't understand why this is property of DT.

Ok, I will add "type: boolean" to it.

> Anyway several problems mentioned earlier, please fix.

Well noted.

Regards,
Li

Re: [PATCH 09/15] dt-bindings: serial: add support for Ambarella

[Krzysztof Kozlowski]

On 25/01/2023 10:54, Li Chen wrote:
> 
> Hi Krzysztof Kozlowski,
> 
> Sorry for my late reply.
> 
> On Mon, 23 Jan 2023 16:11:52 +0800,
> Krzysztof Kozlowski wrote:
>>
>> On 23/01/2023 08:32, Li Chen wrote:
>>> Add compatible for Ambarella.
>>>
>>> Signed-off-by: Li Chen <lchen@ambarella.com>
>>> Change-Id: I32513d98f52af0311dfb55dd5c4739a58f6b9fc1
>>> ---
>>>  .../bindings/serial/ambarella_uart.yaml       | 57 +++++++++++++++++++
>>>  MAINTAINERS                                   |  1 +
>>>  2 files changed, 58 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/serial/ambarella_uart.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/ambarella_uart.yaml b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
>>> new file mode 100644
>>> index 000000000000..238d68078270
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
>>> @@ -0,0 +1,57 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/serial/ambarella_uart.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Ambarella S6LM SoC UART Controller
>>> +
>>> +maintainers:
>>> +  - Li Chen <lchen@ambarella.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: ambarella,uart
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  amb,ignore-fe:
>>> +    description: |
>>> +      ignore frame error report for CV2/CV22/CV25/S6LM because it's
>>> +      checked too strict so that normal stop may be treated as frame error.
>>
>> Missing type. I don't understand why this is property of DT.
> 
> Ok, I will add "type: boolean" to it.

I still do not understand why this is a property of DT. You need to
justify it.

Otherwise: No. drop it.

Best regards,
Krzysztof

Re: [PATCH 09/15] dt-bindings: serial: add support for Ambarella

[Li Chen]

Hi Krzysztof,

Sorry for my late reply.

 ---- On Wed, 25 Jan 2023 17:56:15 +0800  Krzysztof Kozlowski  wrote --- 
 > On 25/01/2023 10:54, Li Chen wrote:
 > > 
 > > Hi Krzysztof Kozlowski,
 > > 
 > > Sorry for my late reply.
 > > 
 > > On Mon, 23 Jan 2023 16:11:52 +0800,
 > > Krzysztof Kozlowski wrote:
 > >>
 > >> On 23/01/2023 08:32, Li Chen wrote:
 > >>> Add compatible for Ambarella.
 > >>>
 > >>> Signed-off-by: Li Chen lchen@ambarella.com>
 > >>> Change-Id: I32513d98f52af0311dfb55dd5c4739a58f6b9fc1
 > >>> ---
 > >>>  .../bindings/serial/ambarella_uart.yaml       | 57 +++++++++++++++++++
 > >>>  MAINTAINERS                                   |  1 +
 > >>>  2 files changed, 58 insertions(+)
 > >>>  create mode 100644 Documentation/devicetree/bindings/serial/ambarella_uart.yaml
 > >>>
 > >>> diff --git a/Documentation/devicetree/bindings/serial/ambarella_uart.yaml b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
 > >>> new file mode 100644
 > >>> index 000000000000..238d68078270
 > >>> --- /dev/null
 > >>> +++ b/Documentation/devicetree/bindings/serial/ambarella_uart.yaml
 > >>> @@ -0,0 +1,57 @@
 > >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 > >>> +%YAML 1.2
 > >>> +---
 > >>> +$id: http://devicetree.org/schemas/serial/ambarella_uart.yaml#
 > >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
 > >>> +
 > >>> +title: Ambarella S6LM SoC UART Controller
 > >>> +
 > >>> +maintainers:
 > >>> +  - Li Chen lchen@ambarella.com>
 > >>> +
 > >>> +properties:
 > >>> +  compatible:
 > >>> +    const: ambarella,uart
 > >>> +
 > >>> +  reg:
 > >>> +    maxItems: 1
 > >>> +
 > >>> +  amb,ignore-fe:
 > >>> +    description: |
 > >>> +      ignore frame error report for CV2/CV22/CV25/S6LM because it's
 > >>> +      checked too strict so that normal stop may be treated as frame error.
 > >>
 > >> Missing type. I don't understand why this is property of DT.
 > > 
 > > Ok, I will add "type: boolean" to it.
 > 
 > I still do not understand why this is a property of DT. You need to
 > justify it.
 > 
 > Otherwise: No. drop it.

Yes, this property is not describing hardware. I will drop it from dts and handle it via soc_device_attribute->data
or of_device_id->data instead.

Regards,
Li

Re: [PATCH 11/15] dt-bindings: mtd: Add binding for Ambarella

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> Ambarella SoC contains nand flash controller.
> Add compatible for it.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: I4108699a0678ba45e5d4347cbd860bc552dd91dd
> ---
>  .../bindings/mtd/ambarella,nand.yaml          | 77 +++++++++++++++++++
>  MAINTAINERS                                   |  1 +
>  2 files changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/ambarella,nand.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mtd/ambarella,nand.yaml b/Documentation/devicetree/bindings/mtd/ambarella,nand.yaml
> new file mode 100644
> index 000000000000..7c2e7c2ebc7b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/ambarella,nand.yaml
> @@ -0,0 +1,77 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mtd/ambarella,nand.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella NAND Controller
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.com>
> +
> +properties:
> +  compatible:
> +    - const: ambarella,nand
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 1

All your patches are weirder and weirder. Do you see such syntax in any
patches before?

Drop minItems.

> +
> +  interrupts:
> +    minItems: 1
> +    maxItems: 1

Drop both.

> +    items:
> +      - description: fio irq
> +
> +  clocks:
> +    maxItems: 1
> +    description: reference to the clock for the NAND controller
> +
> +  nand-on-flash-bbt:

OK, so this was for sure not tested.

Do not send untested patches.

Best regards,
Krzysztof

Re: [PATCH 13/15] dt-bindings: pinctrl: add support for Ambarella

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> Add a Ambarella compatible.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: I8bcab3b763bdc7e400a04cc46589f0f694028a66
> ---
>  .../bindings/pinctrl/ambarella,pinctrl.yaml   | 160 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 161 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/ambarella,pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/ambarella,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/ambarella,pinctrl.yaml
> new file mode 100644
> index 000000000000..51f5a9cc4714
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/ambarella,pinctrl.yaml
> @@ -0,0 +1,160 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/ambarella,pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Ambarella PIN controller
> +
> +maintainers:
> +  - Li Chen <lchen@ambarella.org>
> +
> +description: |
> +  The pins controlled by Ambarella SoC chip are organized in banks, each bank
> +  has 32 pins.  Each pin has at least 2 multiplexing functions, and generally,
> +  the first function is GPIO.
> +
> +  The PINCTRL node acts as a container for an arbitrary number of subnodes. And
> +  these subnodes will fall into two categories.
> +
> +  One is for GPIO, please see the "GPIO node" section for detail, and another one
> +  is to set up a group of pins for a function, both pin configurations and mux
> +  selection, and it's called group node in the binding document.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: ambarella,pinctrl
> +
> +  reg:
> +    minItems: 4
> +    maxItems: 4
> +

Same problems as with other patches. You need to fix all of my previous
comments.

Best regards,
Krzysztof

Re: [PATCH 13/15] dt-bindings: pinctrl: add support for Ambarella

[Linus Walleij]

Hi Li,

thanks for your patch!

It's nice to see Ambarella working with the kernel community.

On Mon, Jan 23, 2023 at 8:41 AM Li Chen <lchen@ambarella.com> wrote:

> +properties:
> +  compatible:
> +    items:
> +      - const: ambarella,pinctrl

I bet there will be more instances of pin controllers from Ambarella, so I would
use this only as a fallback, so the for should likely be:

compatible = "ambarella,<soc-name>-pinctrl", "ambarella,pinctrl";

we need to establish this already otherwise "ambarella,pinctrl" just becomes
the "weird name of the first ambarella SoC supported by standard DT bindings".

> +  amb,pull-regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      maxItems: 1
> +
> +  amb,ds-regmap:
> +    items:
> +      maxItems: 1

Interesting that these registers are elsewhere, but I bet there is an
engineering
explanation for this :)

> +    properties:
> +      amb,pinmux-ids:
> +        description: |
> +          an integer array. Each integer in the array specifies a pin
> +          with given mux function, with pin id and mux packed as:
> +          mux << 12 | pin id
> +          Here mux means function of this pin, and pin id is identical to gpio id. For
> +          the SoC supporting IOMUX, like S2L, the maximal value of mux is 5. However,
> +          for the SoC not supporting IOMUX, like A5S, S2, the third or fourth function
> +          is selected by other "virtual pins" setting. Here the "virtual pins" means
> +          there is no real hardware pins mapping to the corresponding register address.
> +          So the registers for the "virtual pins" can be used for the selection of 3rd
> +          or 4th function for other real pins.

I think you can use the standard bindings for this if you insist on
using the "magic
numbers" scheme.

(I prefer function names and group names as strings, but I gave up on trying
to convince the world to use this because people have so strong opions about
it.)

From Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:

  pinmux:
    description:
      The list of numeric pin ids and their mux settings that properties in the
      node apply to (either this, "pins" or "groups" have to be specified)
    $ref: /schemas/types.yaml#/definitions/uint32-array

> +      amb,pinconf-ids:
> +        description: |
> +          an integer array. Each integer in the array specifies a pin
> +          with given configuration, with pin id and config packed as:
> +            config << 16 | pin id
> +          Here config is used to configure pull up/down and drive strength of the pin,
> +          and it's orgnization is:
> +          bit1~0: 00: pull down, 01: pull up, 1x: clear pull up/down
> +          bit2:   reserved
> +          bit3:   0: leave pull up/down as default value, 1: config pull up/down
> +          bit5~4: drive strength value, 0: 2mA, 1: 4mA, 2: 8mA, 3: 12mA
> +          bit6:   reserved
> +          bit7:   0: leave drive strength as default value, 1: config drive strength

I would be very happy if I could convince you to use the standard (string)
bindings for this.
And from Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml

For each config node this means using strings such as
bias-high-impedance; etc in the device tree pin config node.

Following that scheme just makes life so much easier for maintainers and
reviewers: very few people reviewing or debugging the system will think
it is easy to read a magic number and then (in their head) mask out the
bits to see that "OK this is drive strength 8mA" and then have energy and
memory enough in their head to remember that "wait a minute, that is supposed
to be 12mA in this design", leading to long review and development
cycles.

By using:

drive-push-pull;
drive-strength = <8>;

you make the cognitive load on the people reading the device tree much
lower and easier to write, maintain and debug for humans.

The tendency to encode this info in terse bitfields appear to be done for either
of these reasons:

- Footprint / memory usage
- Adopt the users to the way the machine thinks instead of the other way around
- "We always did it this way"

Neither is a very good argument on a new 64bit platform.

Yours,
Linus Walleij

Re: [PATCH 13/15] dt-bindings: pinctrl: add support for Ambarella

[Li Chen]

Hi Linus,

Sorry for my late reply.

 ---- On Mon, 23 Jan 2023 20:32:28 +0800  Linus Walleij  wrote --- 
 > Hi Li,
 > 
 > thanks for your patch!
 > 
 > It's nice to see Ambarella working with the kernel community.
 > 
 > On Mon, Jan 23, 2023 at 8:41 AM Li Chen lchen@ambarella.com> wrote:
 > 
 > > +properties:
 > > +  compatible:
 > > +    items:
 > > +      - const: ambarella,pinctrl
 > 
 > I bet there will be more instances of pin controllers from Ambarella, so I would
 > use this only as a fallback, so the for should likely be:
 > 
 > compatible = "ambarella,-pinctrl", "ambarella,pinctrl";
 > 
 > we need to establish this already otherwise "ambarella,pinctrl" just becomes
 > the "weird name of the first ambarella SoC supported by standard DT bindings".
 
There is only single "ambarella,pinctrl" in Ambarella downstream kernels, and we use soc_device_attribute->data
and soc_device_attribute->soc_id/family to get correct SoC-specific information like reg offset and etc.

Krzysztof has taught me that this way is wrong and
SoC is required in compatible: https://www.spinics.net/lists/arm-kernel/msg1043145.html

So I will update this property to "ambarella,s6lm-pinctrl" in the new version.

 > > +  amb,pull-regmap:
 > > +    $ref: /schemas/types.yaml#/definitions/phandle-array
 > > +    items:
 > > +      maxItems: 1
 > > +
 > > +  amb,ds-regmap:
 > > +    items:
 > > +      maxItems: 1
 > 
 > Interesting that these registers are elsewhere, but I bet there is an
 > engineering
 > explanation for this :)
 > 
 > > +    properties:
 > > +      amb,pinmux-ids:
 > > +        description: |
 > > +          an integer array. Each integer in the array specifies a pin
 > > +          with given mux function, with pin id and mux packed as:
 > > +          mux << 12 | pin id
 > > +          Here mux means function of this pin, and pin id is identical to gpio id. For
 > > +          the SoC supporting IOMUX, like S2L, the maximal value of mux is 5. However,
 > > +          for the SoC not supporting IOMUX, like A5S, S2, the third or fourth function
 > > +          is selected by other "virtual pins" setting. Here the "virtual pins" means
 > > +          there is no real hardware pins mapping to the corresponding register address.
 > > +          So the registers for the "virtual pins" can be used for the selection of 3rd
 > > +          or 4th function for other real pins.
 > 
 > I think you can use the standard bindings for this if you insist on
 > using the "magic
 > numbers" scheme.
 > 
 > (I prefer function names and group names as strings, but I gave up on trying
 > to convince the world to use this because people have so strong opions about
 > it.)
 > 
 > From Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
 > 
 >   pinmux:
 >     description:
 >       The list of numeric pin ids and their mux settings that properties in the
 >       node apply to (either this, "pins" or "groups" have to be specified)
 >     $ref: /schemas/types.yaml#/definitions/uint32-array
 > 

Well noted, I will switch to pinmux in v2.

 > > +      amb,pinconf-ids:
 > > +        description: |
 > > +          an integer array. Each integer in the array specifies a pin
 > > +          with given configuration, with pin id and config packed as:
 > > +            config << 16 | pin id
 > > +          Here config is used to configure pull up/down and drive strength of the pin,
 > > +          and it's orgnization is:
 > > +          bit1~0: 00: pull down, 01: pull up, 1x: clear pull up/down
 > > +          bit2:   reserved
 > > +          bit3:   0: leave pull up/down as default value, 1: config pull up/down
 > > +          bit5~4: drive strength value, 0: 2mA, 1: 4mA, 2: 8mA, 3: 12mA
 > > +          bit6:   reserved
 > > +          bit7:   0: leave drive strength as default value, 1: config drive strength
 > 
 > I would be very happy if I could convince you to use the standard (string)
 > bindings for this.
 > And from Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
 > 
 > For each config node this means using strings such as
 > bias-high-impedance; etc in the device tree pin config node.
 > 
 > Following that scheme just makes life so much easier for maintainers and
 > reviewers: very few people reviewing or debugging the system will think
 > it is easy to read a magic number and then (in their head) mask out the
 > bits to see that "OK this is drive strength 8mA" and then have energy and
 > memory enough in their head to remember that "wait a minute, that is supposed
 > to be 12mA in this design", leading to long review and development
 > cycles.
 > 
 > By using:
 > 
 > drive-push-pull;
 > drive-strength = ;
 > 
 > you make the cognitive load on the people reading the device tree much
 > lower and easier to write, maintain and debug for humans.
 > 
 > The tendency to encode this info in terse bitfields appear to be done for either
 > of these reasons:
 > 
 > - Footprint / memory usage
 > - Adopt the users to the way the machine thinks instead of the other way around
 > - "We always did it this way"
 > 
 > Neither is a very good argument on a new 64bit platform.

Thanks for your detailed explanation. I totally agree with you and I also really hate
magic number haha.

I will convert it to standard binding after convincing my manager.

Regards,
Li

Re: [PATCH 15/15] arm64: dts: ambarella: introduce Ambarella s6lm SoC

[Krzysztof Kozlowski]

On 23/01/2023 08:32, Li Chen wrote:
> This patch adds s6lm's devicetree.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: Idc7a04cc64a53b3296bc9fcf9dd3cd648ebefeae
> ---
>  MAINTAINERS                                   |   2 +
>  .../boot/dts/ambarella/ambarella-s6lm.dtsi    | 332 ++++++++++++++++++
>  .../boot/dts/ambarella/s6lm_pineapple.dts     |  29 ++
>  3 files changed, 363 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
>  create mode 100644 arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1360fe2f4236..d9d1033b6452 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1894,6 +1894,8 @@ ARM/Ambarella SoC support
>  M:	Li Chen <me@linux.beauty>
>  L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:	Supported
> +F:	arch/arm64/boot/dts/ambarella/ambarella-s6lm.dts
> +F:	arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
>  F:	Documentation/devicetree/bindings/arm/ambarella.yaml
>  F:	Documentation/devicetree/bindings/arm/ambarella/ambarella,cpuid.yaml
>  F:	Documentation/devicetree/bindings/arm/ambarella/ambarella,rct.yaml
> diff --git a/arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi b/arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
> new file mode 100644
> index 000000000000..c3fa77b33e04
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ambarella/ambarella-s6lm.dtsi
> @@ -0,0 +1,332 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Ambarella,Inc. - http://www.ambarella.com/
> + * Author: Cao Rongrong <rrcao@ambarella.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Drop boilerplate.

> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +/ {
> +	compatible = "ambarella,s6lm";
> +	interrupt-parent = <&gic>;
> +	#address-cells = <1>;
> +	#size-cells = <1>;
> +
> +	aliases {
> +		serial0 = &uart0;
> +		nand = &nand0;
> +	};
> +
> +	chosen {
> +		stdout-path = &uart0;
> +	};
> +
> +	/*
> +	 * the memory node will be overwritten in Amboot,
> +	 * here is just the default value.
> +	 */
> +	memory {
> +		device_type = "memory";
> +		reg = <0x00200000 0x07e00000>; /* 126M */
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +	};
> +
> +	psci {
> +		compatible = "arm,psci-0.2";
> +		method = "smc";
> +	};
> +
> +	pmu {
> +		compatible = "arm,cortex-a53-pmu";
> +		interrupts = <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> +			     <0 5 0x4>,

??? Just compare these two lines above...

> +			     <0 6 0x4>,
> +			     <0 7 0x4>;
> +	};
> +
> +	cpus {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		cpu@0 {
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			device_type = "cpu";
> +			reg = <0x0>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu@1 {
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			device_type = "cpu";
> +			reg = <0x1>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu@2 {
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			device_type = "cpu";
> +			reg = <0x2>;
> +			enable-method = "psci";
> +		};
> +
> +		cpu@3 {
> +			compatible = "arm,cortex-a53", "arm,armv8";
> +			device_type = "cpu";
> +			reg = <0x3>;
> +			enable-method = "psci";
> +		};
> +	};
> +
> +	gic: interrupt-controller@f3000000 {
> +		compatible = "arm,gic-400";

reg should be second property.

> +		#interrupt-cells = <3>;
> +		#address-cells = <0>;
> +		interrupt-controller;
> +		reg = <0xf3001000 0x1000>,	/* GIC Dist */
> +		      <0xf3002000 0x2000>,	/* GIC CPU */
> +		      /* following are not used if no virtulization */
> +		      <0xf3004000 0x2000>,	/* GIC VCPU Control */
> +		      <0xf3006000 0x2000>;	/* GIC VCPU */
> +		interrupts = <GIC_PPI 9 0xf04>;	/* GIC Maintenence IRQ */
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupts = <GIC_PPI 13 0xf08>,	/* Secure Phys IRQ */
> +			     <1 14 0xf08>,	/* Non-secure Phys IRQ */

And you do not see here anything odd?

> +			     <1 11 0xf08>,	/* Virt IRQ */
> +			     <1 10 0xf08>;	/* Hyp IRQ */
> +	};
> +
> +	n_apb@e4000000 { /* Non-Secure APB, but configurable */

No underscores in node names. Generic node names, so just "bus"

> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0xe4000000 0x01000000>;
> +		ranges;
> +
> +		uart0: uart@e4000000 {

serial

> +			compatible = "ambarella,uart";
> +			reg = <0xe4000000 0x1000>;
> +			interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&uart0_pins>;
> +			clocks = <&gclk_uart0>;
> +			amb,ignore-fe;
> +			status = "ok";
> +		};
> +	};
> +
> +	s_apb@ec000000 { /* Secure APB, but configurable */

bus

> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0xec000000 0x01000000>;

reg is second property

> +		ranges;
> +
> +		pinctrl: pinctrl@0xec003000 {
> +			compatible = "ambarella,pinctrl";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0xec003000 0x1000>,
> +			      <0xec004000 0x1000>,
> +			      <0xec005000 0x1000>,
> +			      <0xec000000 0x1000>;

reg is second
> +			reg-names = "gpio0", "gpio1", "gpio2", "iomux";
> +			interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
> +			amb,pull-regmap = <&scratchpad_syscon 0x14 0x30>;
> +			amb,ds-regmap = <&rct_syscon>;
> +
> +			gpio: gpio@0 {
> +				reg = <0>;
> +				gpio-controller;
> +				#gpio-cells = <2>;
> +				gpio-ranges = <&pinctrl 0 0 86>;
> +				interrupt-controller;
> +				#interrupt-cells = <2>;
> +			};
> +
> +			uart0_pins: uart0@0 {
> +				reg = <0>;

Why do you have unit addresses here? What is this?

> +				amb,pinmux-ids = <0x100a 0x100b>;

This will need more explanation... your bindings are not ready for
review, so we will get to this later when you rewrite your bindings and
test them before sending.

> +			};
> +
> +			snand0_pins_a: snand0@0 {
> +				reg = <0>;
> +				amb,pinmux-ids = <0x2024 0x2025 0x202a 0x202b
> +						  0x202c 0x202d>;
> +			};
> +
> +			snand0_pins_b: snand0@1 {
> +				reg = <1>;
> +				amb,pinmux-ids = <0x4036 0x4037 0x4038 0x4039
> +						  0x403a 0x403b>;
> +			};
> +		};
> +	};
> +
> +	n_ahb@e0000000 { /* Non-Secure AHB, but configurable */

bus

> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0xe0000000 0x01000000>;

reg is a second property.

> +		ranges;
> +
> +		cpuid_syscon: cpuid@e0000000 {
> +			compatible = "ambarella,cpuid", "syscon";
> +			reg = <0xe0000000 0x1000>;
> +		};
> +
> +		nand0: nand@e0002000 {
> +			compatible = "ambarella,nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0xe0002000 0x1000>;

ditto

> +			interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;	/* fio_irq */
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&snand0_pins_a>;
> +			clocks = <&pll_out_enet>;
> +			nand-on-flash-bbt;
> +			/* amb,soft-ecc = <6>; */

Drop dead code

> +		};
> +
> +		scratchpad_syscon: scratchpad_syscon@e0022000 {
> +			compatible = "ambarella,scratchpad", "syscon";
> +			reg = <0xe0022000 0x100>;
> +		};
> +	};
> +
> +	rct@ed080000 {

bus

> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		reg = <0xed080000 0x1000>;
> +		ranges;
> +
> +		rct_syscon: rct_syscon@ed080000 {

syscon
(no underscores in node names, generic node names)

> +			compatible = "ambarella,rct", "syscon";
> +			reg = <0xed080000 0x1000>;
> +		};
> +	};
> +
> +	clocks {
> +		compatible = "ambarella,clkpll-v0";

Nope, nope. Register proper clock controller.


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

Why? You do not have any children with unit addresses.
> +
> +		/*
> +		 * This is a dummy clock, to be used as placeholder on other
> +		 * mux clocks when corresponding bits in register don't
> +		 * represent real parent clock.
> +		 */
> +		gclk_dummy: gclk_dummy {

NAK. Register proper clock controller.

> +			#clock-cells = <0>;
> +			compatible = "fixed-clock";
> +			clock-frequency = <0>;
> +		};
> +
> +		/*
> +		 * Fixed 24MHz oscillator inputs to SoC
> +		 */
> +		osc: oscillator {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <24000000>;

This is actually the only clock which makes sense, but it is not a
property of SoC. Put it with the board DTS. At least frequency goes
there to note it.

> +			clock-output-names = "osc";
> +		};
> +
> +		gclk_core: gclk-core {

NAK here and for all other weird nodes below

> +			#clock-cells = <0>;
> +			compatible = "ambarella,pll-clock";
> +			clocks = <&osc>;
> +			clock-output-names = "gclk_core";
> +			amb,clk-regmap = <&rct_syscon 0x000 0x004 0x100 0x104 0x000 0x000>;
> +		};
> +
> +		gclk_ddr: gclk-ddr {
> +			#clock-cells = <0>;
> +			compatible = "ambarella,pll-clock";
> +			clocks = <&osc>;
> +			clock-output-names = "gclk_ddr";
> +			amb,clk-regmap = <&rct_syscon 0x0dc 0x0e0 0x110 0x114 0x000 0x000>;
> +			amb,fix-divider = <2>;
> +		};
> +
> +		gclk_cortex: gclk-cortex {
> +			#clock-cells = <0>;
> +			compatible = "ambarella,pll-clock";
> +			clocks = <&osc>;
> +			clock-output-names = "gclk_cortex";
> +			amb,clk-regmap = <&rct_syscon 0x264 0x268 0x26c 0x270 0x000 0x000>;
> +		};
> +
> +		gclk_axi: gclk-axi {
> +			#clock-cells = <0>;
> +			compatible = "fixed-factor-clock";
> +			clocks = <&gclk_cortex>;
> +			clock-output-names = "gclk_axi";
> +			clock-mult = <1>;
> +			clock-div = <3>;
> +		};
> +
> +		gclk_so: gclk-so {
> +			#clock-cells = <0>;
> +			compatible = "ambarella,pll-clock";
> +			clocks = <&osc>;
> +			clock-output-names = "gclk_so";
> +			assigned-clocks = <&gclk_so>;
> +			assigned-clock-rates = <24000000>;
> +			amb,clk-regmap = <&rct_syscon 0x024 0x028 0x11c 0x120 0x04c 0x030>;
> +			amb,frac-mode;
> +		};
> +
> +		pll_out_vo2: pll-out-vo2 {
> +			#clock-cells = <0>;
> +			compatible = "ambarella,pll-clock";
> +			clocks = <&osc>;
> +			clock-output-names = "pll_out_vo2";
> +			assigned-clocks = <&pll_out_vo2>;
> +			assigned-clock-rates = <24000000>;
> +			amb,clk-regmap = <&rct_syscon 0x0c0 0x0c4 0x13c 0x140 0x0c8 0x000>;
> +			amb,frac-mode;
> +		};
> +
> +		pll_out_sd: pll-out-sd {
> +			#clock-cells = <0>;
> +			compatible = "ambarella,pll-clock";
> +			clocks = <&osc>;
> +			clock-output-names = "pll_out_sd";
> +			amb,clk-regmap = <&rct_syscon 0x4ac 0x4b0 0x4b4 0x4b8 0x000 0x000>;
> +		};
> +		pll_out_enet: pll-out-enet {
> +			#clock-cells = <0>;
> +			compatible = "ambarella,pll-clock";
> +			clocks = <&osc>;
> +			clock-output-names = "pll_out_enet";
> +			amb,clk-regmap = <&rct_syscon 0x520 0x524 0x528 0x52c 0x000 0x000>;
> +		};
> +		gclk_uart0: gclk-uart0 {
> +			#clock-cells = <0>;
> +			compatible = "ambarella,composite-clock";
> +			clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>;
> +			clock-output-names = "gclk_uart0";
> +			assigned-clocks = <&gclk_uart0>;
> +			assigned-clock-parents = <&osc>;
> +			assigned-clock-rates = <24000000>;
> +			amb,mux-regmap = <&rct_syscon 0x1c8>;
> +			amb,div-regmap = <&rct_syscon 0x038>;
> +			amb,div-width = <24>;
> +			amb,div-shift = <0>;
> +		};
> +	};
> +};
> diff --git a/arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts b/arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts
> new file mode 100644
> index 000000000000..2d5d6d18e738
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ambarella/s6lm_pineapple.dts
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright (C) 2023 Ambarella,Inc. - http://www.ambarella.com/
> + * Author: Cao Rongrong <rrcao@ambarella.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/dts-v1/;
> +
> +#include "ambarella-s6lm.dtsi"
> +
> +/ {
> +	model = "Ambarella S6LM Pineapple Board";
> +	compatible = "ambarella,s6lm";

Missing board compatible.

> +
> +	chosen {
> +		bootargs = "console=ttyS0 ubi.mtd=lnx root=ubi0:rootfs rw
> +			rootfstype=ubifs init=/linuxrc";

Drop bootargs. These should not be needed in DTS. Otherwise - why do you
need them? Why bootloader cannot provide these?

> +	};
> +
> +	n_apb@e4000000 {
> +		i2c1: i2c@e4009000 {

No, use override by phandle/label.

> +			status = "ok";
> +		};
> +	};
> +};

Best regards,
Krzysztof

Re: [PATCH 00/15] Ambarella S6LM SoC bring-up

[Arnd Bergmann]

On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
> This series brings up initial support for the Ambarella S6LM
> SoC.
>
> The following features are supported in this initial port:
>
> - UART with console support
> - Pinctrl with GPIO controller
> - Nand flash controller
> - Devicetree

I seem to only have part of the series, please add both me and
the linux-arm-kernel mailing list to each part of the initial
submission.

It's possible that some patches were already Cc'd to
linux-arm-kernel but did not make it through because the Cc list
was too long (it has to fit within 1024 characters for many lists).
I think you too the Cc list from get_maintainers.pl, but when
sending new drivers this does not work well because it picks
up everyone that recently touched the Makefile/Kconfig.

     Arnd

Re: [PATCH 00/15] Ambarella S6LM SoC bring-up

[Bagas Sanjaya]

On 1/23/23 15:39, Arnd Bergmann wrote:
> I seem to only have part of the series, please add both me and
> the linux-arm-kernel mailing list to each part of the initial
> submission.
> 
> It's possible that some patches were already Cc'd to
> linux-arm-kernel but did not make it through because the Cc list
> was too long (it has to fit within 1024 characters for many lists).
> I think you too the Cc list from get_maintainers.pl, but when
> sending new drivers this does not work well because it picks
> up everyone that recently touched the Makefile/Kconfig.

Hi Arnd,

It is possible (and common) that people who recently touched these
files, when given new drivers patches, aren't interested in reviewing
them for many reasons.

In that case, you may want to see Alison's trick posted on kernel
outreachy list [1]. In summary, pass `--no-gitfallback` (don't give
addresses of recent commit authors) and `--norolestats` (only name and
email are printed; MLs don't get open list:-generated names). Also,
another trick that I use is to condense the list by passing
`--separator , ` so that it can be easily copy-pasted to
git-send-email(1).

Thanks.

[1]: https://lore.kernel.org/outreachy/20211015171331.GA431883@alison-desk/

-- 
An old man doll... just what I always wanted! - Clara