[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 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 00/15] Ambarella S6LM SoC bring-up

[Arnd Bergmann]

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

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

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

     Arnd

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

[Bagas Sanjaya]

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

Hi Arnd,

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

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

Thanks.

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

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

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

[Li Chen]

Hi Krzysztof,

Sorry for my late reply.

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

Well noted.

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

Thanks, description as below will be added in v2:

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

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

Ok.

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

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

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

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

Ok. I will list all clocks name

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

Ok

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

Ok, I will add "maxItems: 1"

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

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

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

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

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

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

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

Well noted.

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

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

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

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

Ok.

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

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

Regards,
Li

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

[Krzysztof Kozlowski]

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

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

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

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

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

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

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

Drop all these syscon properties.

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

Nope.

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

Also nope.

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

Nope, nope, nope.

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

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

There are plenty of examples, including example-schema.

Best regards,
Krzysztof

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

[Li Chen]

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

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

Ok.

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

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

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

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

But I have three questions:

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

Thanks in advanced.

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

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

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

Ok, I will learn more and fix it.

Regards,
Li

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

[Krzysztof Kozlowski]

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

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

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

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

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

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

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

Best regards,
Krzysztof

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

[Li Chen]

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

Hi Krzysztof,

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

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

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

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

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

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

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

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

The question is: can upstream kernel accept such codes?

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

Regards,
Li

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

[Krzysztof Kozlowski]

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

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

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

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

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

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

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

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

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

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

Best regards,
Krzysztof

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

[Li Chen]

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

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

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

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

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

Regards,
Li

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

[Krzysztof Kozlowski]

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

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

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

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

Best regards,
Krzysztof

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

[Li Chen]

Hi Krzysztof,

 ---- On Fri, 27 Jan 2023 23: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 07/15] dt-bindings: clock: Add Ambarella clock bindings

[Krzysztof Kozlowski]

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

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

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

Best regards,
Krzysztof

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

[Li Chen]

Hi Krzysztof,

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

Regards,
Li

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

[Li Chen]

Hi Krzysztof ,

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

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

Regards,
Li

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

[Krzysztof Kozlowski]

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

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

Best regards,
Krzysztof

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

[Li Chen]

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

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

osc
  | pll A
  | pll B
  | ...

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

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

Regards,
Li

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

[Krzysztof Kozlowski]

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

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

Best regards,
Krzysztof