[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

[PATCH 05/15] arm64: Kconfig: Introduce CONFIG_ARCH_AMBARELLA

[Li Chen]

This adds a Kconfig option to toggle support
for Ambarella ARM SoCs.

Signed-off-by: Li Chen <lchen@ambarella.com>
Change-Id: I41345f5052b08023d399cb9db3faa228ca54d265
---
 arch/arm64/Kconfig.platforms | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index d1970adf80ab..8def8aaca0c4 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -8,6 +8,15 @@ config ARCH_ACTIONS
 	help
 	  This enables support for the Actions Semiconductor S900 SoC family.
 
+config ARCH_AMBARELLA
+	bool "Ambarella ARMv8 SoC Family"
+	select PINCTRL
+	select PINCTRL_AMB
+	select RATIONAL
+	help
+	  This enables support for Ambarella ARMv8 SoC Family starting
+          from s6lm.
+
 config ARCH_SUNXI
 	bool "Allwinner sunxi 64-bit SoC Family"
 	select ARCH_HAS_RESET_CONTROLLER
-- 
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 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 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 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 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 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 06/15] soc: add Ambarella driver

[Arnd Bergmann]

On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
> This driver add soc_id support for Ambarella,
> which is stored inside "cpuid" AXI address mapping.
>
> Also provide sys_config(POC, aka power on configuration)
> for other drivers.
>
> Signed-off-by: Li Chen <lchen@ambarella.com>

The soc_id support looks ok

> Change-Id: I4869a3497366ac7779e792835f8e0309239036a8

Please drop these lines in the submission, the IDs are
not reachable outside of your own git, so we don't want
these to show up in the public history.

> +static struct ambarella_soc_id {
> +	unsigned int id;
> +	const char *name;
> +	const char *family;
> +} soc_ids[] = {
> +	{ 0x00483245, "s6lm",  "10nm", },
> +};

I would suggest something more descriptive in the "family"
field to let users know they are on an Ambarella SoC.

Maybe just "Ambarella 10nm".

> +static int __init ambarella_socinfo_init(void)
> +{
> +	struct soc_device_attribute *soc_dev_attr;
> +	struct soc_device *soc_dev;
> +	struct device_node *np;
> +	struct regmap *cpuid_regmap;
> +	unsigned int soc_id;
> +
> +	cpuid_regmap = syscon_regmap_lookup_by_compatible("ambarella,cpuid");
> +	if (IS_ERR(cpuid_regmap))
> +		return PTR_ERR(cpuid_regmap);

Is there anything else in this syscon node? If the block
of registers only contains the identification bits, you
could just make this file a platform_driver that binds to
the node instead of using a syscon.

If there are other unrelated registers in there, the compatible
string should probably be changed to better describe the
entire area based on the name in the datasheet.

> +static unsigned int ambsys_config;
> +
> +unsigned int ambarella_sys_config(void)
> +{
> +	return ambsys_config;
> +}
> +EXPORT_SYMBOL(ambarella_sys_config);

Which drivers use this bit? Can they be changed to 
use soc_device_match() instead to avoid the export?

> +static int __init ambarella_soc_init(void)
> +{
> +	struct regmap *rct_regmap;
> +	int ret;
> +
> +	rct_regmap = syscon_regmap_lookup_by_compatible("ambarella,rct");
> +	if (IS_ERR(rct_regmap)) {
> +		pr_err("failed to get ambarella rct regmap\n");
> +		return PTR_ERR(rct_regmap);
> +	}
...
> +arch_initcall(ambarella_soc_init);

It is not an error to use a chip from another manufacturer,
please drop the pr_err() and return success here.

> +#ifndef __SOC_AMBARELLA_MISC_H__
> +#define __SOC_AMBARELLA_MISC_H__
> +
> +extern unsigned int ambarella_sys_config(void);
> +extern struct proc_dir_entry *ambarella_proc_dir(void);
> +

The ambarella_proc_dir looks like a stale entry that should be
removed. Ideally you should not need a private header at all.

    Arnd

Re: [PATCH 12/15] mtd: nand: add Ambarella nand support

[Miquel Raynal]

Hi Li,

I'm sorry, this is not going to work at all.

> +	ambarella_nand_init(host);
> +
> +	mtd = nand_to_mtd(&host->chip);
> +	mtd->name = "amba_nand";
> +
> +	nand_controller_init(&host->controller);
> +	nand_set_controller_data(&host->chip, host);
> +	nand_set_flash_node(&host->chip, dev->of_node);
> +
> +	host->chip.controller = &host->controller;
> +	host->chip.controller->ops = &ambarella_controller_ops;
> +	host->chip.legacy.chip_delay = 0;
> +	host->chip.legacy.read_byte = ambarella_nand_read_byte;
> +	host->chip.legacy.write_buf = ambarella_nand_write_buf;
> +	host->chip.legacy.read_buf = ambarella_nand_read_buf;
> +	host->chip.legacy.select_chip = ambarella_nand_select_chip;
> +	host->chip.legacy.cmd_ctrl = ambarella_nand_cmd_ctrl;
> +	host->chip.legacy.dev_ready = ambarella_nand_dev_ready;
> +	host->chip.legacy.waitfunc = ambarella_nand_waitfunc;
> +	host->chip.legacy.cmdfunc = ambarella_nand_cmdfunc;
> +	host->chip.legacy.set_features = nand_get_set_features_notsupp;
> +	host->chip.legacy.get_features = nand_get_set_features_notsupp;

Please be aware that we no longer accept legacy introductions upstream.

You can look for ->exec_op() conversions using git-log.

> +	host->chip.options |= NAND_NO_SUBPAGE_WRITE | NAND_USES_DMA;
> +
> +	rval = nand_scan(&host->chip, 1);
> +	if (rval < 0)
> +		return rval;
> +
> +	rval = mtd_device_register(mtd, NULL, 0);
> +	if (rval < 0)
> +		nand_cleanup(&host->chip);
> +
> +	return rval;
> +}

[...]

> diff --git a/drivers/mtd/nand/raw/nand_ids.c
b/drivers/mtd/nand/raw/nand_ids.c
> index dacc5529b3df..9f264e2a6484 100644
> --- a/drivers/mtd/nand/raw/nand_ids.c
> +++ b/drivers/mtd/nand/raw/nand_ids.c
> @@ -62,6 +62,10 @@ struct nand_flash_dev nand_flash_ids[] = {
>  		{ .id = {0x98, 0xd3, 0x91, 0x26, 0x76} },
>  		  SZ_4K, SZ_1K, SZ_256K, 0, 5, 256, NAND_ECC_INFO(8, SZ_512)},
>  
> +	{"MT29F2G01ABAGD SPINAND 2G 3.3V 8-bit",
> +		{ .id = {0x2c, 0x24, 0x00, 0x00, 0x00} },
> +		  SZ_2K, SZ_256, SZ_128K, 0, 2, 128},
> +

Raw NAND != SPI-NAND. I don't get what you're doing here but either you
want to drive SPI-NANDs and this is a SPI controller driver that
implements spi-mem ops and should be located under drivers/spi/, or
this is a plain raw NAND controller which is wired to a parallel NAND
and this should be under drivers/mtd/nand/raw/.

ECC controllers can be shared with the ECC engine abstraction though.

>  	LEGACY_ID_NAND("NAND 4MiB 5V 8-bit",   0x6B, 4, SZ_8K, SP_OPTIONS),
>  	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE3, 4, SZ_8K, SP_OPTIONS),
>  	LEGACY_ID_NAND("NAND 4MiB 3,3V 8-bit", 0xE5, 4, SZ_8K, SP_OPTIONS),


Thanks,
Miquèl

Re: [PATCH 05/15] arm64: Kconfig: Introduce CONFIG_ARCH_AMBARELLA

[Arnd Bergmann]

On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
> +config ARCH_AMBARELLA
> +	bool "Ambarella ARMv8 SoC Family"
> +	select PINCTRL
> +	select PINCTRL_AMB
> +	select RATIONAL
> +	help
> +	  This enables support for Ambarella ARMv8 SoC Family starting
> +          from s6lm.

The patch is ok, but drop the "ARMv8" from the "bool" prompt,
we are removing this from all the other entries as well.

     Arnd

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 10/15] serial: ambarella: add support for Ambarella uart_port

[Greg Kroah-Hartman]

On Mon, Jan 23, 2023 at 03:32:25PM +0800, Li Chen wrote:
> This driver add support for Ambarella's uart, which
> can be used for console and etc.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: Ie68af7ad2187e21853e58d52cd97fd7145303730

Please always use scripts/checkpatch.pl so you don't get grumpy
maintainers asking you why you didn't run scripts/checkpatch.pl :(

Re: [PATCH 10/15] serial: ambarella: add support for Ambarella uart_port

[Greg Kroah-Hartman]

On Mon, Jan 23, 2023 at 03:32:25PM +0800, Li Chen wrote:
> This driver add support for Ambarella's uart, which
> can be used for console and etc.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: Ie68af7ad2187e21853e58d52cd97fd7145303730
> ---
>  MAINTAINERS                         |    1 +
>  drivers/tty/serial/Kconfig          |   16 +
>  drivers/tty/serial/Makefile         |    1 +
>  drivers/tty/serial/ambarella_uart.c | 1581 +++++++++++++++++++++++++++
>  drivers/tty/serial/ambarella_uart.h |  120 ++

Why do you need a .h file for a single .c file?  They should all be in
one file please.

Also, no change-id, you know this...

thanks,

greg k-h

Re: [PATCH 06/15] soc: add Ambarella driver

[Conor.Dooley]

Hey Li Chen,

Since you've already got a bunch of other comments, I have
two minor ones for you.

On 23/01/2023 07:32, Li Chen wrote:
> 
> This driver add soc_id support for Ambarella,
> which is stored inside "cpuid" AXI address mapping.
> 
> Also provide sys_config(POC, aka power on configuration)
> for other drivers.
> 
> Signed-off-by: Li Chen <lchen@ambarella.com>
> Change-Id: I4869a3497366ac7779e792835f8e0309239036a8
> ---

> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index fff513bd522d..e3e270aa32a4 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -8,6 +8,7 @@ obj-y                           += apple/
>   obj-y                          += aspeed/
>   obj-$(CONFIG_ARCH_AT91)                += atmel/
>   obj-y                          += bcm/
> +obj-$(CONFIG_ARCH_AMBARELLA)   += ambarella/
>   obj-$(CONFIG_SOC_CANAAN)       += canaan/

If you could follow the existing (by directory) alphabetical
order that'd be great.

>   obj-$(CONFIG_ARCH_DOVE)                += dove/
>   obj-$(CONFIG_MACH_DOVE)                += dove/
> diff --git a/drivers/soc/ambarella/Makefile b/drivers/soc/ambarella/Makefile
> new file mode 100644
> index 000000000000..384276c046ca
> --- /dev/null
> +++ b/drivers/soc/ambarella/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o

The subdirectory is already gated by this symbol, so is there much point
gating it on the same one again?

Re: [PATCH 01/15] debugfs: allow to use regmap for print regs

[Greg Kroah-Hartman]

On Mon, Jan 23, 2023 at 03:32:16PM +0800, Li Chen wrote:
> Currently, debugfs_regset32 only contains void __iomem *base,
> and it is not friendly to regmap user.
> 
> Let's add regmap to debugfs_regset32, and add regmap
> support to debugfs_print_reg32.
> 
> Signed-off-by: Li Chen <me@linux.beauty>
> Change-Id: I8ef015ed0906a4ad85b7592f771dcf64c23f7832

No change-id please.

> ---
>  Documentation/filesystems/debugfs.rst |  2 ++
>  fs/debugfs/file.c                     | 43 ++++++++++++++++++++++++++-
>  include/linux/debugfs.h               | 11 +++++++
>  3 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
> index dc35da8b8792..b2c76ac3a333 100644
> --- a/Documentation/filesystems/debugfs.rst
> +++ b/Documentation/filesystems/debugfs.rst
> @@ -178,6 +178,8 @@ file::
>  
>      void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
>  			 int nregs, void __iomem *base, char *prefix);
> +    void debugfs_print_regmap_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> +			 int nregs, struct regmap *regmap*, char *prefix);

One too many "*" characters on that last line, right?

>  
>  The "base" argument may be 0, but you may want to build the reg32 array
>  using __stringify, and a number of register names (macros) are actually
> diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> index b54f470e0d03..2fb792843b30 100644
> --- a/fs/debugfs/file.c
> +++ b/fs/debugfs/file.c
> @@ -1137,14 +1137,55 @@ void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
>  }
>  EXPORT_SYMBOL_GPL(debugfs_print_regs32);
>  
> +/**
> + * debugfs_print_regmap_regs32 - use seq_print to describe a set of registers
> + * @s: the seq_file structure being used to generate output
> + * @regs: an array if struct debugfs_reg32 structures
> + * @nregs: the length of the above array
> + * @regmap: regmap to be used in reading the registers
> + * @prefix: a string to be prefixed to every output line
> + *
> + * This function outputs a text block describing the current values of
> + * some 32-bit hardware registers. It is meant to be used within debugfs
> + * files based on seq_file that need to show registers, intermixed with other
> + * information. The prefix argument may be used to specify a leading string,
> + * because some peripherals have several blocks of identical registers,
> + * for example configuration of dma channels
> + */
> +void debugfs_print_regmap_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> +			  int nregs, struct regmap *regmap, char *prefix)
> +{
> +	int i;
> +	u32 val;
> +
> +	for (i = 0; i < nregs; i++, regs++) {
> +		if (prefix)
> +			seq_printf(s, "%s", prefix);
> +		regmap_read(regmap, regs->offset, &val);
> +		seq_printf(s, "%s = 0x%08x\n", regs->name, val);
> +		if (seq_has_overflowed(s))
> +			break;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(debugfs_print_regmap_regs32);
> +
>  static int debugfs_regset32_show(struct seq_file *s, void *data)
>  {
>  	struct debugfs_regset32 *regset = s->private;
>  
> +	void __iomem *base = regset->base;
> +	struct regmap *regmap = regset->regmap;

Why the extra blank line?  Did you run checkpatch?

And it's generally not considered a good idea to dereference a pointer
_before_ it is checked.  It will not crash, but static checkers will
have a field day with it.


> +
> +	if ((regmap && base) || (!regmap && !base))
> +		return -EINVAL;
> +
>  	if (regset->dev)
>  		pm_runtime_get_sync(regset->dev);
>  
> -	debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
> +	if (base)
> +		debugfs_print_regs32(s, regset->regs, regset->nregs, base, "");
> +	else
> +		debugfs_print_regmap_regs32(s, regset->regs, regset->nregs, regmap, "");
>  
>  	if (regset->dev)
>  		pm_runtime_put(regset->dev);
> diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> index ea2d919fd9c7..87dfea6a25a0 100644
> --- a/include/linux/debugfs.h
> +++ b/include/linux/debugfs.h
> @@ -17,6 +17,7 @@
>  
>  #include <linux/types.h>
>  #include <linux/compiler.h>
> +#include <linux/regmap.h>

No need to include this here, just provide a prototype for "struct
regmap" and all will be fine.

thanks,

greg k-h

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 01/15] debugfs: allow to use regmap for print regs

[Li Chen]

Hi Greg,

On Mon, 23 Jan 2023 19:52:45 +0800,
Greg Kroah-Hartman wrote:
>
> On Mon, Jan 23, 2023 at 03:32:16PM +0800, Li Chen wrote:
> > Currently, debugfs_regset32 only contains void __iomem *base,
> > and it is not friendly to regmap user.
> >
> > Let's add regmap to debugfs_regset32, and add regmap
> > support to debugfs_print_reg32.
> >
> > Signed-off-by: Li Chen <me@linux.beauty>
> > Change-Id: I8ef015ed0906a4ad85b7592f771dcf64c23f7832
>
> No change-id please.

Sorry, my bad, will remove it in v2.

> > ---
> >  Documentation/filesystems/debugfs.rst |  2 ++
> >  fs/debugfs/file.c                     | 43 ++++++++++++++++++++++++++-
> >  include/linux/debugfs.h               | 11 +++++++
> >  3 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/filesystems/debugfs.rst b/Documentation/filesystems/debugfs.rst
> > index dc35da8b8792..b2c76ac3a333 100644
> > --- a/Documentation/filesystems/debugfs.rst
> > +++ b/Documentation/filesystems/debugfs.rst
> > @@ -178,6 +178,8 @@ file::
> >
> >      void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> >  			 int nregs, void __iomem *base, char *prefix);
> > +    void debugfs_print_regmap_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> > +			 int nregs, struct regmap *regmap*, char *prefix);
>
> One too many "*" characters on that last line, right?

Good catch! I will remove it in V2.

> >
> >  The "base" argument may be 0, but you may want to build the reg32 array
> >  using __stringify, and a number of register names (macros) are actually
> > diff --git a/fs/debugfs/file.c b/fs/debugfs/file.c
> > index b54f470e0d03..2fb792843b30 100644
> > --- a/fs/debugfs/file.c
> > +++ b/fs/debugfs/file.c
> > @@ -1137,14 +1137,55 @@ void debugfs_print_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> >  }
> >  EXPORT_SYMBOL_GPL(debugfs_print_regs32);
> >
> > +/**
> > + * debugfs_print_regmap_regs32 - use seq_print to describe a set of registers
> > + * @s: the seq_file structure being used to generate output
> > + * @regs: an array if struct debugfs_reg32 structures
> > + * @nregs: the length of the above array
> > + * @regmap: regmap to be used in reading the registers
> > + * @prefix: a string to be prefixed to every output line
> > + *
> > + * This function outputs a text block describing the current values of
> > + * some 32-bit hardware registers. It is meant to be used within debugfs
> > + * files based on seq_file that need to show registers, intermixed with other
> > + * information. The prefix argument may be used to specify a leading string,
> > + * because some peripherals have several blocks of identical registers,
> > + * for example configuration of dma channels
> > + */
> > +void debugfs_print_regmap_regs32(struct seq_file *s, const struct debugfs_reg32 *regs,
> > +			  int nregs, struct regmap *regmap, char *prefix)
> > +{
> > +	int i;
> > +	u32 val;
> > +
> > +	for (i = 0; i < nregs; i++, regs++) {
> > +		if (prefix)
> > +			seq_printf(s, "%s", prefix);
> > +		regmap_read(regmap, regs->offset, &val);
> > +		seq_printf(s, "%s = 0x%08x\n", regs->name, val);
> > +		if (seq_has_overflowed(s))
> > +			break;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(debugfs_print_regmap_regs32);
> > +
> >  static int debugfs_regset32_show(struct seq_file *s, void *data)
> >  {
> >  	struct debugfs_regset32 *regset = s->private;
> >
> > +	void __iomem *base = regset->base;
> > +	struct regmap *regmap = regset->regmap;
>
> Why the extra blank line?  Did you run checkpatch?

Yeah, I do checkpatch & sparse(coccinelle crash somehow)
for all my patches(but forget to remove gerrit's Change-ID finally, sorry).

checkpatch didn't find this extra blank line.

I will remove it in v2, thanks!

> And it's generally not considered a good idea to dereference a pointer
> _before_ it is checked.  It will not crash, but static checkers will
> have a field day with it.

Ok, will check regset before access in v2.

> > +
> > +	if ((regmap && base) || (!regmap && !base))
> > +		return -EINVAL;
> > +
> >  	if (regset->dev)
> >  		pm_runtime_get_sync(regset->dev);
> >
> > -	debugfs_print_regs32(s, regset->regs, regset->nregs, regset->base, "");
> > +	if (base)
> > +		debugfs_print_regs32(s, regset->regs, regset->nregs, base, "");
> > +	else
> > +		debugfs_print_regmap_regs32(s, regset->regs, regset->nregs, regmap, "");
> >
> >  	if (regset->dev)
> >  		pm_runtime_put(regset->dev);
> > diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h
> > index ea2d919fd9c7..87dfea6a25a0 100644
> > --- a/include/linux/debugfs.h
> > +++ b/include/linux/debugfs.h
> > @@ -17,6 +17,7 @@
> >
> >  #include <linux/types.h>
> >  #include <linux/compiler.h>
> > +#include <linux/regmap.h>
>
> No need to include this here, just provide a prototype for "struct
> regmap" and all will be fine.

Well noted, I will forward declare "struct regmap" in debugfs.h,
and move regmap.h to debugfs.c(regmap_read is used here).

Thanks for your review.

Regards,
Li

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

[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 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

Re: [PATCH 06/15] soc: add Ambarella driver

[Li Chen]

Hi Arnd,

On Mon, 23 Jan 2023 16:29:06 +0800,
Arnd Bergmann wrote:
>
> On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
> > This driver add soc_id support for Ambarella,
> > which is stored inside "cpuid" AXI address mapping.
> >
> > Also provide sys_config(POC, aka power on configuration)
> > for other drivers.
> >
> > Signed-off-by: Li Chen <lchen@ambarella.com>
>
> The soc_id support looks ok
>
> > Change-Id: I4869a3497366ac7779e792835f8e0309239036a8
>
> Please drop these lines in the submission, the IDs are
> not reachable outside of your own git, so we don't want
> these to show up in the public history.

Sorry, I forgot to remove this.

> > +static struct ambarella_soc_id {
> > +	unsigned int id;
> > +	const char *name;
> > +	const char *family;
> > +} soc_ids[] = {
> > +	{ 0x00483245, "s6lm",  "10nm", },
> > +};
>
> I would suggest something more descriptive in the "family"
> field to let users know they are on an Ambarella SoC.
>
> Maybe just "Ambarella 10nm".

There is a "pr_info("Ambarella SoC %s detected\n", soc_dev_attr->soc_id);" in this file,
I think this should be enough, right?

> > +static int __init ambarella_socinfo_init(void)
> > +{
> > +	struct soc_device_attribute *soc_dev_attr;
> > +	struct soc_device *soc_dev;
> > +	struct device_node *np;
> > +	struct regmap *cpuid_regmap;
> > +	unsigned int soc_id;
> > +
> > +	cpuid_regmap = syscon_regmap_lookup_by_compatible("ambarella,cpuid");
> > +	if (IS_ERR(cpuid_regmap))
> > +		return PTR_ERR(cpuid_regmap);
>
> Is there anything else in this syscon node? If the block
> of registers only contains the identification bits, you
> could just make this file a platform_driver that binds to
> the node instead of using a syscon.
>
> If there are other unrelated registers in there, the compatible
> string should probably be changed to better describe the
> entire area based on the name in the datasheet.

Yeah, this block is only used for identification bits. In datasheet,
it is also named "CPU ID".

Other than cpuid_regmap, this driver also looks for "model" name as soc machine name:
of_property_read_string(np, "model", &soc_dev_attr->machine);

So I think it is not a good idea to conver it to into a platform driver.

As for "syscon", I think it is still very helpful to get regmap easily. Generally speaking,
I prefer regmap over void*, because it has debugfs support, so I can get its value more easily.

> > +static unsigned int ambsys_config;
> > +
> > +unsigned int ambarella_sys_config(void)
> > +{
> > +	return ambsys_config;
> > +}
> > +EXPORT_SYMBOL(ambarella_sys_config);
>
> Which drivers use this bit? Can they be changed to
> use soc_device_match() instead to avoid the export?

sys_config is used by our nand and sd drivers. I also don't want to export,
but struct soc_device_attribute/soc_device don't have private data to store it,
I think there is no better way.

> > +static int __init ambarella_soc_init(void)
> > +{
> > +	struct regmap *rct_regmap;
> > +	int ret;
> > +
> > +	rct_regmap = syscon_regmap_lookup_by_compatible("ambarella,rct");
> > +	if (IS_ERR(rct_regmap)) {
> > +		pr_err("failed to get ambarella rct regmap\n");
> > +		return PTR_ERR(rct_regmap);
> > +	}
> ...
> > +arch_initcall(ambarella_soc_init);
>
> It is not an error to use a chip from another manufacturer,
> please drop the pr_err() and return success here.

Ok, good to know, thanks. But we don't have other manufacturers at least for now,
and rct_regmap is need to be updated here, like sys_config and soft reboot. So I think
this rct regmap is still needed.

> > +#ifndef __SOC_AMBARELLA_MISC_H__
> > +#define __SOC_AMBARELLA_MISC_H__
> > +
> > +extern unsigned int ambarella_sys_config(void);
> > +extern struct proc_dir_entry *ambarella_proc_dir(void);
> > +
>
> The ambarella_proc_dir looks like a stale entry that should be
> removed. Ideally you should not need a private header at all.

Oops, my bad. I will remove proc dir in v2.

Regards,
Li

Re: [PATCH 06/15] soc: add Ambarella driver

[Li Chen]

On Mon, 23 Jan 2023 19:48:42 +0800,

Hi Conor.Dooley

<Conor.Dooley@microchip.com> wrote:
>
> Hey Li Chen,
>
> Since you've already got a bunch of other comments, I have
> two minor ones for you.
>
> On 23/01/2023 07:32, Li Chen wrote:
> >
> > This driver add soc_id support for Ambarella,
> > which is stored inside "cpuid" AXI address mapping.
> >
> > Also provide sys_config(POC, aka power on configuration)
> > for other drivers.
> >
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> > Change-Id: I4869a3497366ac7779e792835f8e0309239036a8
> > ---
>
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index fff513bd522d..e3e270aa32a4 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -8,6 +8,7 @@ obj-y                           += apple/
> >   obj-y                          += aspeed/
> >   obj-$(CONFIG_ARCH_AT91)                += atmel/
> >   obj-y                          += bcm/
> > +obj-$(CONFIG_ARCH_AMBARELLA)   += ambarella/
> >   obj-$(CONFIG_SOC_CANAAN)       += canaan/
>
> If you could follow the existing (by directory) alphabetical
> order that'd be great.

Well noted. I will fix it in v2.

> >   obj-$(CONFIG_ARCH_DOVE)                += dove/
> >   obj-$(CONFIG_MACH_DOVE)                += dove/
> > diff --git a/drivers/soc/ambarella/Makefile b/drivers/soc/ambarella/Makefile
> > new file mode 100644
> > index 000000000000..384276c046ca
> > --- /dev/null
> > +++ b/drivers/soc/ambarella/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o
>
> The subdirectory is already gated by this symbol, so is there much point
> gating it on the same one again?

Yeah, it lookgs kind of redundant now, but I will upstream other drivers after
this series get merged, which are not gated by this symbol.

Regards,
Li

Re: [PATCH 06/15] soc: add Ambarella driver

[Conor.Dooley]

Hey,

>>> +++ b/drivers/soc/ambarella/Makefile
>>> @@ -0,0 +1,3 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o
>>
>> The subdirectory is already gated by this symbol, so is there much point
>> gating it on the same one again?
> 
> Yeah, it lookgs kind of redundant now, but I will upstream other drivers after
> this series get merged, which are not gated by this symbol.

You could make the directory by obj-y, and therefore always included,
and have various drivers controlled by their own Kconfig symbols.

Or else, you could leave the directory controlled by ARCH_AMBARELLA
and make the above `obj-y += soc.o` instead, since it's always
going to be built if the directory is included.

Thanks,
Conor.

Re: [PATCH 06/15] soc: add Ambarella driver

[Li Chen]

Hi Conor.Dooley,

On Tue, 24 Jan 2023 16:46:41 +0800,
<Conor.Dooley@microchip.com> wrote:
> 
> Hey,
> 
> >>> +++ b/drivers/soc/ambarella/Makefile
> >>> @@ -0,0 +1,3 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only
> >>> +
> >>> +obj-$(CONFIG_ARCH_AMBARELLA) += soc.o
> >>
> >> The subdirectory is already gated by this symbol, so is there much point
> >> gating it on the same one again?
> > 
> > Yeah, it lookgs kind of redundant now, but I will upstream other drivers after
> > this series get merged, which are not gated by this symbol.
> 
> You could make the directory by obj-y, and therefore always included,
> and have various drivers controlled by their own Kconfig symbols.
> 
> Or else, you could leave the directory controlled by ARCH_AMBARELLA
> and make the above `obj-y += soc.o` instead, since it's always
> going to be built if the directory is included.

Gotcha, I will fix it in v2. Thanks for your kindness!

Regards,
Li

Re: [PATCH 06/15] soc: add Ambarella driver

[Arnd Bergmann]

On Tue, Jan 24, 2023, at 08:58, Li Chen wrote:
> On Mon, 23 Jan 2023 16:29:06 +0800,
> Arnd Bergmann wrote:
>> On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
>> > +static struct ambarella_soc_id {
>> > +	unsigned int id;
>> > +	const char *name;
>> > +	const char *family;
>> > +} soc_ids[] = {
>> > +	{ 0x00483245, "s6lm",  "10nm", },
>> > +};
>>
>> I would suggest something more descriptive in the "family"
>> field to let users know they are on an Ambarella SoC.
>>
>> Maybe just "Ambarella 10nm".
>
> There is a "pr_info("Ambarella SoC %s detected\n", 
> soc_dev_attr->soc_id);" in this file,
> I think this should be enough, right?

The pr_info() can probably be removed here, or reworded
based on the changed contents, those are just meant for
humans reading through the log rather than parsed by
software.

The soc_id fields on the other hand need to be parsable
by scripts looking at the sysfs files, in a way that lets
them identify the system. Usually the script would look
at the "family" as the primary key before looking up the
"name", so you have to make sure that the family uniquely
identifies this as one of yours rather than a 10nm chip
from some other company.

>> If there are other unrelated registers in there, the compatible
>> string should probably be changed to better describe the
>> entire area based on the name in the datasheet.
>
> Yeah, this block is only used for identification bits. In datasheet,
> it is also named "CPU ID".

ok.

> Other than cpuid_regmap, this driver also looks for "model" name as soc 
> machine name:
> of_property_read_string(np, "model", &soc_dev_attr->machine);
>
> So I think it is not a good idea to conver it to into a platform driver.

I don't understand what you mean. A lot of soc_id drivers put
the model string into soc_dev_attr->machine, this makes no
difference here.

> As for "syscon", I think it is still very helpful to get regmap easily. 
> Generally speaking,
> I prefer regmap over void*, because it has debugfs support, so I can 
> get its value more easily.

What value would you get through debugfs that is not already in
the soc_device?

>> > +static unsigned int ambsys_config;
>> > +
>> > +unsigned int ambarella_sys_config(void)
>> > +{
>> > +	return ambsys_config;
>> > +}
>> > +EXPORT_SYMBOL(ambarella_sys_config);
>>
>> Which drivers use this bit? Can they be changed to
>> use soc_device_match() instead to avoid the export?
>
> sys_config is used by our nand and sd drivers. I also don't want to export,
> but struct soc_device_attribute/soc_device don't have private data to store it,
> I think there is no better way.

The nand and sd drivers should not rely on any private data
from another driver. What information do they actually
need here that is not already in their own DT nodes or
in the soc_device_attributes?

>> > +static int __init ambarella_soc_init(void)
>> > +{
>> > +	struct regmap *rct_regmap;
>> > +	int ret;
>> > +
>> > +	rct_regmap = syscon_regmap_lookup_by_compatible("ambarella,rct");
>> > +	if (IS_ERR(rct_regmap)) {
>> > +		pr_err("failed to get ambarella rct regmap\n");
>> > +		return PTR_ERR(rct_regmap);
>> > +	}
>> ...
>> > +arch_initcall(ambarella_soc_init);
>>
>> It is not an error to use a chip from another manufacturer,
>> please drop the pr_err() and return success here.
>
> Ok, good to know, thanks. But we don't have other manufacturers at 
> least for now,

I care a lot about supporting multiple SoC vendors, it would seem
very rude to assume that we stop supporting everything else after
merging Ambarella support.

> and rct_regmap is need to be updated here, like sys_config and soft 
> reboot. So I think this rct regmap is still needed.

It is certainly only needed on Ambarella SoCs, no other one
has this device at the moment.

      Arnd

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 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 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 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 10/15] serial: ambarella: add support for Ambarella uart_port

[Li Chen]

On Mon, 23 Jan 2023 17:51:25 +0800,

Hi Greg,

Sorry for my late reply.

Greg Kroah-Hartman wrote:
> 
> On Mon, Jan 23, 2023 at 03:32:25PM +0800, Li Chen wrote:
> > This driver add support for Ambarella's uart, which
> > can be used for console and etc.
> > 
> > Signed-off-by: Li Chen <lchen@ambarella.com>
> > Change-Id: Ie68af7ad2187e21853e58d52cd97fd7145303730
> > ---
> >  MAINTAINERS                         |    1 +
> >  drivers/tty/serial/Kconfig          |   16 +
> >  drivers/tty/serial/Makefile         |    1 +
> >  drivers/tty/serial/ambarella_uart.c | 1581 +++++++++++++++++++++++++++
> >  drivers/tty/serial/ambarella_uart.h |  120 ++
> 
> Why do you need a .h file for a single .c file?  They should all be in
> one file please.

Ok, I will combine them into single source file.

> Also, no change-id, you know this...
> 
> thanks,
> 
> greg k-h

Regards,
Li

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

[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 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 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

[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 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 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 06/15] soc: add Ambarella driver

[Li Chen]

Hi Arnd,

Sorry for late reply.

On Tue, 24 Jan 2023 23:46:06 +0800,
Arnd Bergmann wrote:
>
> On Tue, Jan 24, 2023, at 08:58, Li Chen wrote:
> > On Mon, 23 Jan 2023 16:29:06 +0800,
> > Arnd Bergmann wrote:
> >> On Mon, Jan 23, 2023, at 08:32, Li Chen wrote:
> >> > +static struct ambarella_soc_id {
> >> > +	unsigned int id;
> >> > +	const char *name;
> >> > +	const char *family;
> >> > +} soc_ids[] = {
> >> > +	{ 0x00483245, "s6lm",  "10nm", },
> >> > +};
> >>
> >> I would suggest something more descriptive in the "family"
> >> field to let users know they are on an Ambarella SoC.
> >>
> >> Maybe just "Ambarella 10nm".
> >
> > There is a "pr_info("Ambarella SoC %s detected\n",
> > soc_dev_attr->soc_id);" in this file,
> > I think this should be enough, right?
>
> The pr_info() can probably be removed here, or reworded
> based on the changed contents, those are just meant for
> humans reading through the log rather than parsed by
> software.
>
> The soc_id fields on the other hand need to be parsable
> by scripts looking at the sysfs files, in a way that lets
> them identify the system. Usually the script would look
> at the "family" as the primary key before looking up the
> "name", so you have to make sure that the family uniquely
> identifies this as one of yours rather than a 10nm chip
> from some other company.

Ok, I will add "Ambarella" prefix to ->family.

> >> If there are other unrelated registers in there, the compatible
> >> string should probably be changed to better describe the
> >> entire area based on the name in the datasheet.
> >
> > Yeah, this block is only used for identification bits. In datasheet,
> > it is also named "CPU ID".
>
> ok.
>
> > Other than cpuid_regmap, this driver also looks for "model" name as soc
> > machine name:
> > of_property_read_string(np, "model", &soc_dev_attr->machine);
> >
> > So I think it is not a good idea to conver it to into a platform driver.
> I don't understand what you mean. A lot of soc_id drivers put
> the model string into soc_dev_attr->machine, this makes no
> difference here.

Ok, I will switch to builtin platform driver. Which compatible do you prefer?

compatible = "ambarella,cpuid"
or
compatible = "ambarella,<SoC>-cpuid"

> > As for "syscon", I think it is still very helpful to get regmap easily.
> > Generally speaking,
> > I prefer regmap over void*, because it has debugfs support, so I can
> > get its value more easily.
>
> What value would you get through debugfs that is not already in
> the soc_device?

Agree with you.

> >> > +static unsigned int ambsys_config;
> >> > +
> >> > +unsigned int ambarella_sys_config(void)
> >> > +{
> >> > +	return ambsys_config;
> >> > +}
> >> > +EXPORT_SYMBOL(ambarella_sys_config);
> >>
> >> Which drivers use this bit? Can they be changed to
> >> use soc_device_match() instead to avoid the export?
> >
> > sys_config is used by our nand and sd drivers. I also don't want to export,
> > but struct soc_device_attribute/soc_device don't have private data to store it,
> > I think there is no better way.
>
> The nand and sd drivers should not rely on any private data
> from another driver.

Agree.

> What information do they actually
> need here that is not already in their own DT nodes or
> in the soc_device_attributes?

sys_config from rct_regmap is not available for sd/nand node neither soc_device_attribute.

But given that I will switch dts model to(see https://www.spinics.net/lists/arm-kernel/msg1043684.html):

rct
| nand
| sd
| ...

Then I can easily and naturally get sys_config via syscon_node_to_regmap(of_get_parent(nand_np/sd_np)).

So this is not a problem anymore and I will switch to builtin platform driver in v2.

> >> > +static int __init ambarella_soc_init(void)
> >> > +{
> >> > +	struct regmap *rct_regmap;
> >> > +	int ret;
> >> > +
> >> > +	rct_regmap = syscon_regmap_lookup_by_compatible("ambarella,rct");
> >> > +	if (IS_ERR(rct_regmap)) {
> >> > +		pr_err("failed to get ambarella rct regmap\n");
> >> > +		return PTR_ERR(rct_regmap);
> >> > +	}
> >> ...
> >> > +arch_initcall(ambarella_soc_init);
> >>
> >> It is not an error to use a chip from another manufacturer,
> >> please drop the pr_err() and return success here.
> >
> > Ok, good to know, thanks. But we don't have other manufacturers at
> > least for now,
>
> I care a lot about supporting multiple SoC vendors, it would seem
> very rude to assume that we stop supporting everything else after
> merging Ambarella support.
>
> > and rct_regmap is need to be updated here, like sys_config and soft
> > reboot. So I think this rct regmap is still needed.
>
> It is certainly only needed on Ambarella SoCs, no other one
> has this device at the moment.

I'm really sorry that I forgot that this builtin arch_initcall code would run on SoCs
other than Ambarella.

I will remove the err output in v2.

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