On 23/01/2023 08:32, Li Chen wrote:
> This patch introduce clock bindings for Ambarella.
>
> Signed-off-by: Li Chen <[email protected]>
> 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 <[email protected]>
> +
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 <[email protected]>
> +
> +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
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 <[email protected]>
> > 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 <[email protected]>
> > +
>
> 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 <[email protected]>
> > +
> > +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
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 <[email protected]>
>>> 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 <[email protected]>
>>> +
>>
>> 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 <[email protected]>
>>> +
>>> +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
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 <[email protected]>
> >>> 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 <[email protected]>
> >>> +
> >>
> >> 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 <[email protected]>
> >>> +
> >>> +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
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
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
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
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
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
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
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
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
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
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
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
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
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
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