[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

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