diff mbox series

[1/2] dt-bindings: clk: Add binding for versal clocking wizard

Message ID 20220930080400.15619-2-shubhrajyoti.datta@amd.com
State Changes Requested, archived
Headers show
Series clocking-wizard: Add versal clocking wizard support | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Datta, Shubhrajyoti Sept. 30, 2022, 8:03 a.m. UTC
The Clocking Wizard for Versal adaptive compute acceleration platforms
generates multiple configurable number of clock outputs.
Add device tree binding for Versal clocking wizard support.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
---

 .../bindings/clock/xlnx,clk-wizard.yaml       | 66 +++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml

Comments

Krzysztof Kozlowski Sept. 30, 2022, 11:19 a.m. UTC | #1
On 30/09/2022 10:03, Shubhrajyoti Datta wrote:
> The Clocking Wizard for Versal adaptive compute acceleration platforms
> generates multiple configurable number of clock outputs.
> Add device tree binding for Versal clocking wizard support.

Drop second "binding" from subject. Redundant.

> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
> 
>  .../bindings/clock/xlnx,clk-wizard.yaml       | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
> new file mode 100644
> index 000000000000..41a6f4bcaccd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/xlnx,clk-wizard.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Versal clocking wizard
> +
> +maintainers:
> +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> +
> +description:
> +  The clocking wizard is a soft ip clocking block of Xilinx versal. The IP

Isn't versal a proper noun? Some product? If so, it starts with a
capital letter.

> +  uses the input clock frequencies and generates the requested
> +  clock output.
> +
> +properties:
> +  compatible:
> +    const: xlnx,clk-wizard-1.0
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    description: List of clock specifiers which are external input
> +      clocks to the given clock controller.

Drop "List of clock specifiers which are "

> +    items:
> +      - description: functional clock input
> +      - description: axi clock or the interface clock
> +
> +  clock-names:
> +    items:
> +      - const: clk_in1

Just "in1"

> +      - const: s_axi_aclk

Just "s_axi"

> +
> +  xlnx,nr-outputs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 8
> +    description:
> +      Number of outputs.

Nope, you just copied the name of property. That's like documenting a
function with the name of that function.

Not useful at all.

> +
> +required:
> +  - compatible
> +  - reg
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +  - xlnx,nr-outputs
> +
> +...

Best regards,
Krzysztof
Rob Herring Sept. 30, 2022, 12:25 p.m. UTC | #2
On Fri, Sep 30, 2022 at 3:04 AM Shubhrajyoti Datta
<shubhrajyoti.datta@amd.com> wrote:
>
> The Clocking Wizard for Versal adaptive compute acceleration platforms
> generates multiple configurable number of clock outputs.
> Add device tree binding for Versal clocking wizard support.

Really v1? I'm sure I heard of this wizard before.

What about this?:

drivers/staging/clocking-wizard/dt-binding.txt

That needs to be moved out of staging rather than adding a 2nd one.

>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> ---
>
>  .../bindings/clock/xlnx,clk-wizard.yaml       | 66 +++++++++++++++++++
>  1 file changed, 66 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
>
> diff --git a/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
> new file mode 100644
> index 000000000000..41a6f4bcaccd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/xlnx,clk-wizard.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Xilinx Versal clocking wizard
> +
> +maintainers:
> +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
> +
> +description:
> +  The clocking wizard is a soft ip clocking block of Xilinx versal. The IP
> +  uses the input clock frequencies and generates the requested
> +  clock output.
> +
> +properties:
> +  compatible:
> +    const: xlnx,clk-wizard-1.0

Where does 1.0 come from? A 1.0 always feels made up. This should be
based on some IP versioning that's documented somewhere.


> +
> +  reg:
> +    maxItems: 1
> +
> +  "#clock-cells":
> +    const: 1
> +
> +  clocks:
> +    description: List of clock specifiers which are external input
> +      clocks to the given clock controller.
> +    items:
> +      - description: functional clock input
> +      - description: axi clock or the interface clock
> +
> +  clock-names:
> +    items:
> +      - const: clk_in1
> +      - const: s_axi_aclk
> +
> +  xlnx,nr-outputs:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 8
> +    description:
> +      Number of outputs.
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#clock-cells"
> +  - clocks
> +  - clock-names
> +  - xlnx,nr-outputs
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    clock-generator@40040000 {
> +        compatible = "xlnx,clk-wizard-1.0";
> +        reg = <0x40040000 0x1000>;
> +        #clock-cells = <1>;
> +        clocks = <&clkc 15>, <&clkc 15>;
> +        clock-names = "clk_in1", "s_axi_aclk";
> +        xlnx,nr-outputs = <6>;
> +    };
> +...
> --
> 2.17.1
>
Michal Simek Sept. 30, 2022, 1 p.m. UTC | #3
Hi Rob,

On 9/30/22 14:25, Rob Herring wrote:
> On Fri, Sep 30, 2022 at 3:04 AM Shubhrajyoti Datta
> <shubhrajyoti.datta@amd.com> wrote:
>>
>> The Clocking Wizard for Versal adaptive compute acceleration platforms
>> generates multiple configurable number of clock outputs.
>> Add device tree binding for Versal clocking wizard support.
> 
> Really v1? I'm sure I heard of this wizard before.
> 
> What about this?:
> 
> drivers/staging/clocking-wizard/dt-binding.txt
> 
> That needs to be moved out of staging rather than adding a 2nd one.


Let me clarify this. This is IP which is already moved out of staging. 
Linux-next has these changes and waiting for MW to happen (already in clock tree).

And this is new IP. Not sure who has chosen similar name but this targets Xilinx 
Versal SOCs. Origin one was targeting previous families.


> 
>>
>> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
>> ---
>>
>>   .../bindings/clock/xlnx,clk-wizard.yaml       | 66 +++++++++++++++++++
>>   1 file changed, 66 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
>> new file mode 100644
>> index 000000000000..41a6f4bcaccd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
>> @@ -0,0 +1,66 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/xlnx,clk-wizard.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Xilinx Versal clocking wizard
>> +
>> +maintainers:
>> +  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
>> +
>> +description:
>> +  The clocking wizard is a soft ip clocking block of Xilinx versal. The IP
>> +  uses the input clock frequencies and generates the requested
>> +  clock output.
>> +
>> +properties:
>> +  compatible:
>> +    const: xlnx,clk-wizard-1.0
> 
> Where does 1.0 come from? A 1.0 always feels made up. This should be
> based on some IP versioning that's documented somewhere.

Soft IP catalog name with version:
xilinx.com:ip:clk_wizard:1.0

https://docs.xilinx.com/r/en-US/pg321-clocking-wizard/Introduction


Thanks,
Michal
Rob Herring Sept. 30, 2022, 9:39 p.m. UTC | #4
On Fri, Sep 30, 2022 at 03:00:28PM +0200, Michal Simek wrote:
> Hi Rob,
> 
> On 9/30/22 14:25, Rob Herring wrote:
> > On Fri, Sep 30, 2022 at 3:04 AM Shubhrajyoti Datta
> > <shubhrajyoti.datta@amd.com> wrote:
> > > 
> > > The Clocking Wizard for Versal adaptive compute acceleration platforms
> > > generates multiple configurable number of clock outputs.
> > > Add device tree binding for Versal clocking wizard support.
> > 
> > Really v1? I'm sure I heard of this wizard before.
> > 
> > What about this?:
> > 
> > drivers/staging/clocking-wizard/dt-binding.txt
> > 
> > That needs to be moved out of staging rather than adding a 2nd one.
> 
> 
> Let me clarify this. This is IP which is already moved out of staging.
> Linux-next has these changes and waiting for MW to happen (already in clock
> tree).

Where does this series explain that? If the dependency is not the latest 
rc1, then state that.

> And this is new IP. Not sure who has chosen similar name but this targets
> Xilinx Versal SOCs. Origin one was targeting previous families.

Do we need a whole new schema doc?

It is not ideal to define the same property, xlnx,nr-outputs, more than 
once. And it's only a new compatible string.

Rob
Michal Simek Oct. 3, 2022, 7:15 a.m. UTC | #5
On 9/30/22 23:39, Rob Herring wrote:
> On Fri, Sep 30, 2022 at 03:00:28PM +0200, Michal Simek wrote:
>> Hi Rob,
>>
>> On 9/30/22 14:25, Rob Herring wrote:
>>> On Fri, Sep 30, 2022 at 3:04 AM Shubhrajyoti Datta
>>> <shubhrajyoti.datta@amd.com> wrote:
>>>>
>>>> The Clocking Wizard for Versal adaptive compute acceleration platforms
>>>> generates multiple configurable number of clock outputs.
>>>> Add device tree binding for Versal clocking wizard support.
>>>
>>> Really v1? I'm sure I heard of this wizard before.
>>>
>>> What about this?:
>>>
>>> drivers/staging/clocking-wizard/dt-binding.txt
>>>
>>> That needs to be moved out of staging rather than adding a 2nd one.
>>
>>
>> Let me clarify this. This is IP which is already moved out of staging.
>> Linux-next has these changes and waiting for MW to happen (already in clock
>> tree).
> 
> Where does this series explain that? If the dependency is not the latest
> rc1, then state that.

Please take a look below.


>> And this is new IP. Not sure who has chosen similar name but this targets
>> Xilinx Versal SOCs. Origin one was targeting previous families.
> 
> Do we need a whole new schema doc?

It is completely new IP with different logic compare to origin one.

> 
> It is not ideal to define the same property, xlnx,nr-outputs, more than
> once. And it's only a new compatible string.

I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml

also for this IP if that's fine with you.
Only xlnx,speed-grade can be defined for previous IP which is easy to mark.

But as I said, driver is different and integrating to current driver is not a 
good idea. But if two separate drivers can use the same DT binding document with 
adding new compatible string (and small tweak around one dt property) then it 
shouldn't be a problem to do it in v2.

Thanks,
Michal
Krzysztof Kozlowski Oct. 3, 2022, 7:23 a.m. UTC | #6
On 03/10/2022 09:15, Michal Simek wrote:
>>> And this is new IP. Not sure who has chosen similar name but this targets
>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>
>> Do we need a whole new schema doc?
> 
> It is completely new IP with different logic compare to origin one.
> 
>>
>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>> once. And it's only a new compatible string.
> 
> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml

So we already have out of staging document:
devicetree/bindings/clock/xlnx,clocking-wizard.yaml

and author wants to add one more:
devicetree/bindings/clock/xlnx,clk-wizard.yaml

Shall we expect in two years, a third document like:
devicetree/bindings/clock/xlnx,clk-wzrd.yaml
?

> 
> also for this IP if that's fine with you.
> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.

That old binding also explained nr-outputs as "Number of outputs".
Perfect... :(

Best regards,
Krzysztof
Michal Simek Oct. 3, 2022, 7:58 a.m. UTC | #7
On 10/3/22 09:23, Krzysztof Kozlowski wrote:
> On 03/10/2022 09:15, Michal Simek wrote:
>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>
>>> Do we need a whole new schema doc?
>>
>> It is completely new IP with different logic compare to origin one.
>>
>>>
>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>> once. And it's only a new compatible string.
>>
>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> 
> So we already have out of staging document:
> devicetree/bindings/clock/xlnx,clocking-wizard.yaml

in 6.1 yes.

> 
> and author wants to add one more:
> devicetree/bindings/clock/xlnx,clk-wizard.yaml

as I said it is completely different IP which requires complete different driver 
but IP designers choose similar name which is out of developer control.

> 
> Shall we expect in two years, a third document like:
> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
> ?

Developer definitely doesn't know. If new SoC requires for the same purpose 
different IP with completely different driver is something out of developer 
control. As of today I am not aware about such a requirement and need and 
personally I can just hope that if they need to do such a change they will be 
able to keep current SW driver compatible with new HW IP.

>>
>> also for this IP if that's fine with you.
>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
> 
> That old binding also explained nr-outputs as "Number of outputs".
> Perfect... :(

Anyway if description should be improved let's just do it. I just want to get 
guidance if we should update current dt binding for similar IP or just create 
new one as this one is trying to do.

Thanks,
Michal
Krzysztof Kozlowski Oct. 3, 2022, 8:10 a.m. UTC | #8
On 03/10/2022 09:58, Michal Simek wrote:
> 
> 
> On 10/3/22 09:23, Krzysztof Kozlowski wrote:
>> On 03/10/2022 09:15, Michal Simek wrote:
>>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>>
>>>> Do we need a whole new schema doc?
>>>
>>> It is completely new IP with different logic compare to origin one.
>>>
>>>>
>>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>>> once. And it's only a new compatible string.
>>>
>>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>
>> So we already have out of staging document:
>> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> 
> in 6.1 yes.
> 
>>
>> and author wants to add one more:
>> devicetree/bindings/clock/xlnx,clk-wizard.yaml
> 
> as I said it is completely different IP which requires complete different driver 
> but IP designers choose similar name which is out of developer control.
> 
>>
>> Shall we expect in two years, a third document like:
>> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
>> ?
> 
> Developer definitely doesn't know. If new SoC requires for the same purpose 
> different IP with completely different driver is something out of developer 
> control. As of today I am not aware about such a requirement and need and 
> personally I can just hope that if they need to do such a change they will be 
> able to keep current SW driver compatible with new HW IP.

Then please start naming them reasonable, not two (and in future
x-times) the same names for entirely different blocks. And by name I
mean compatible, filename and device name.

>>> also for this IP if that's fine with you.
>>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
>>
>> That old binding also explained nr-outputs as "Number of outputs".
>> Perfect... :(
> 
> Anyway if description should be improved let's just do it. I just want to get 
> guidance if we should update current dt binding for similar IP or just create 
> new one as this one is trying to do.

IMHO, new binding is extremely confusing. We already have support for
devices named "xlnx,clocking-wizard" and now you add exactly the same
(clk=clocking) with almost the same properties, named
"xlnx,clk-wizard-1.0". For a different IP?

How anyone (even Xilinx' customer) can understand which block is for
what if they have exactly the same name and (almost) the same
properties, but as you said - these are entirely different IP?

Best regards,
Krzysztof
Greg KH Oct. 3, 2022, 8:16 a.m. UTC | #9
On Mon, Oct 03, 2022 at 10:10:38AM +0200, Krzysztof Kozlowski wrote:
> On 03/10/2022 09:58, Michal Simek wrote:
> > 
> > 
> > On 10/3/22 09:23, Krzysztof Kozlowski wrote:
> >> On 03/10/2022 09:15, Michal Simek wrote:
> >>>>> And this is new IP. Not sure who has chosen similar name but this targets
> >>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
> >>>>
> >>>> Do we need a whole new schema doc?
> >>>
> >>> It is completely new IP with different logic compare to origin one.
> >>>
> >>>>
> >>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
> >>>> once. And it's only a new compatible string.
> >>>
> >>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
> >>>
> >>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> >>
> >> So we already have out of staging document:
> >> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> > 
> > in 6.1 yes.
> > 
> >>
> >> and author wants to add one more:
> >> devicetree/bindings/clock/xlnx,clk-wizard.yaml
> > 
> > as I said it is completely different IP which requires complete different driver 
> > but IP designers choose similar name which is out of developer control.
> > 
> >>
> >> Shall we expect in two years, a third document like:
> >> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
> >> ?
> > 
> > Developer definitely doesn't know. If new SoC requires for the same purpose 
> > different IP with completely different driver is something out of developer 
> > control. As of today I am not aware about such a requirement and need and 
> > personally I can just hope that if they need to do such a change they will be 
> > able to keep current SW driver compatible with new HW IP.
> 
> Then please start naming them reasonable, not two (and in future
> x-times) the same names for entirely different blocks. And by name I
> mean compatible, filename and device name.
> 
> >>> also for this IP if that's fine with you.
> >>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
> >>
> >> That old binding also explained nr-outputs as "Number of outputs".
> >> Perfect... :(
> > 
> > Anyway if description should be improved let's just do it. I just want to get 
> > guidance if we should update current dt binding for similar IP or just create 
> > new one as this one is trying to do.
> 
> IMHO, new binding is extremely confusing. We already have support for
> devices named "xlnx,clocking-wizard" and now you add exactly the same
> (clk=clocking) with almost the same properties, named
> "xlnx,clk-wizard-1.0". For a different IP?
> 
> How anyone (even Xilinx' customer) can understand which block is for
> what if they have exactly the same name and (almost) the same
> properties, but as you said - these are entirely different IP?

Maybe we should just delete the staging one (and the staging driver),
and start over?  No one has taken the time to get the staging driver out
of there, so I have no objection to dropping it for 6.1.

thanks,

greg k-h
Michal Simek Oct. 3, 2022, 9:59 a.m. UTC | #10
Hi Greg,

On 10/3/22 10:16, Greg Kroah-Hartman wrote:
> On Mon, Oct 03, 2022 at 10:10:38AM +0200, Krzysztof Kozlowski wrote:
>> On 03/10/2022 09:58, Michal Simek wrote:
>>>
>>>
>>> On 10/3/22 09:23, Krzysztof Kozlowski wrote:
>>>> On 03/10/2022 09:15, Michal Simek wrote:
>>>>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>>>>
>>>>>> Do we need a whole new schema doc?
>>>>>
>>>>> It is completely new IP with different logic compare to origin one.
>>>>>
>>>>>>
>>>>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>>>>> once. And it's only a new compatible string.
>>>>>
>>>>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>>
>>>> So we already have out of staging document:
>>>> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>
>>> in 6.1 yes.
>>>
>>>>
>>>> and author wants to add one more:
>>>> devicetree/bindings/clock/xlnx,clk-wizard.yaml
>>>
>>> as I said it is completely different IP which requires complete different driver
>>> but IP designers choose similar name which is out of developer control.
>>>
>>>>
>>>> Shall we expect in two years, a third document like:
>>>> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
>>>> ?
>>>
>>> Developer definitely doesn't know. If new SoC requires for the same purpose
>>> different IP with completely different driver is something out of developer
>>> control. As of today I am not aware about such a requirement and need and
>>> personally I can just hope that if they need to do such a change they will be
>>> able to keep current SW driver compatible with new HW IP.
>>
>> Then please start naming them reasonable, not two (and in future
>> x-times) the same names for entirely different blocks. And by name I
>> mean compatible, filename and device name.
>>
>>>>> also for this IP if that's fine with you.
>>>>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
>>>>
>>>> That old binding also explained nr-outputs as "Number of outputs".
>>>> Perfect... :(
>>>
>>> Anyway if description should be improved let's just do it. I just want to get
>>> guidance if we should update current dt binding for similar IP or just create
>>> new one as this one is trying to do.
>>
>> IMHO, new binding is extremely confusing. We already have support for
>> devices named "xlnx,clocking-wizard" and now you add exactly the same
>> (clk=clocking) with almost the same properties, named
>> "xlnx,clk-wizard-1.0". For a different IP?
>>
>> How anyone (even Xilinx' customer) can understand which block is for
>> what if they have exactly the same name and (almost) the same
>> properties, but as you said - these are entirely different IP?
> 
> Maybe we should just delete the staging one (and the staging driver),
> and start over?  No one has taken the time to get the staging driver out
> of there, so I have no objection to dropping it for 6.1.

As I said it is be out of staging in linux-next. When CLK tree is merged in 
these 2 weeks we are done at least with this driver.

Thanks,
Michal
Michal Simek Oct. 3, 2022, 10:37 a.m. UTC | #11
On 10/3/22 10:10, Krzysztof Kozlowski wrote:
> On 03/10/2022 09:58, Michal Simek wrote:
>>
>>
>> On 10/3/22 09:23, Krzysztof Kozlowski wrote:
>>> On 03/10/2022 09:15, Michal Simek wrote:
>>>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>>>
>>>>> Do we need a whole new schema doc?
>>>>
>>>> It is completely new IP with different logic compare to origin one.
>>>>
>>>>>
>>>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>>>> once. And it's only a new compatible string.
>>>>
>>>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>
>>> So we already have out of staging document:
>>> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>
>> in 6.1 yes.
>>
>>>
>>> and author wants to add one more:
>>> devicetree/bindings/clock/xlnx,clk-wizard.yaml
>>
>> as I said it is completely different IP which requires complete different driver
>> but IP designers choose similar name which is out of developer control.
>>
>>>
>>> Shall we expect in two years, a third document like:
>>> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
>>> ?
>>
>> Developer definitely doesn't know. If new SoC requires for the same purpose
>> different IP with completely different driver is something out of developer
>> control. As of today I am not aware about such a requirement and need and
>> personally I can just hope that if they need to do such a change they will be
>> able to keep current SW driver compatible with new HW IP.
> 
> Then please start naming them reasonable, not two (and in future
> x-times) the same names for entirely different blocks. And by name I
> mean compatible, filename and device name.
> 
>>>> also for this IP if that's fine with you.
>>>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
>>>
>>> That old binding also explained nr-outputs as "Number of outputs".
>>> Perfect... :(
>>
>> Anyway if description should be improved let's just do it. I just want to get
>> guidance if we should update current dt binding for similar IP or just create
>> new one as this one is trying to do.
> 
> IMHO, new binding is extremely confusing. We already have support for
> devices named "xlnx,clocking-wizard" and now you add exactly the same
> (clk=clocking) with almost the same properties, named
> "xlnx,clk-wizard-1.0". For a different IP?
> 
> How anyone (even Xilinx' customer) can understand which block is for
> what if they have exactly the same name and (almost) the same
> properties, but as you said - these are entirely different IP?

Let me start from last one. Xilinx has IP catalog in design tools called Vivado. 
You choose device you have and then you will find clk wizard and you get an IP.
It means depends on SOC you have ZynqMP or Versal and based on that one or 
another is taken.
And because this is fpga world none is really describing programmable logic by 
hand because it would take a look a lot of time. That's why I created long time 
ago device-tree generator (DTG) which gets design data and based on it generate 
device tree description. Newest version is available for example here.
https://github.com/Xilinx/device-tree-xlnx
There is also newer version called system device tree generato
https://github.com/Xilinx/system-device-tree-xlnx

Because of this infrastructure user will all the time get proper compatible 
string which is aligned with IP catalog.

If you think that we should use the name which is more closer to ZynqMP clocking 
wizard driver we can try to take a look at it.
We can also update description in dt binding to explain this better.

I understand that it is confusing but we just got this name from IP catalog 
which is created by HW guys and we won't be able to change it. We can't also 
enforce that they will rename existing IP.
But we can use different compatible string and wire it up in DTG that we will 
start to generate.
We tend to use IP name and also IP version for compatible string generation 
because HW guys are IP owner and when any change happen they should bump IP 
version.

Thanks,
Michal
Michal Simek Oct. 3, 2022, 10:41 a.m. UTC | #12
Hi again,

On 10/3/22 11:59, Michal Simek wrote:
> Hi Greg,
> 
> On 10/3/22 10:16, Greg Kroah-Hartman wrote:
>> On Mon, Oct 03, 2022 at 10:10:38AM +0200, Krzysztof Kozlowski wrote:
>>> On 03/10/2022 09:58, Michal Simek wrote:
>>>>
>>>>
>>>> On 10/3/22 09:23, Krzysztof Kozlowski wrote:
>>>>> On 03/10/2022 09:15, Michal Simek wrote:
>>>>>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>>>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>>>>>
>>>>>>> Do we need a whole new schema doc?
>>>>>>
>>>>>> It is completely new IP with different logic compare to origin one.
>>>>>>
>>>>>>>
>>>>>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>>>>>> once. And it's only a new compatible string.
>>>>>>
>>>>>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml 
>>>>>>
>>>>>
>>>>> So we already have out of staging document:
>>>>> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>>
>>>> in 6.1 yes.
>>>>
>>>>>
>>>>> and author wants to add one more:
>>>>> devicetree/bindings/clock/xlnx,clk-wizard.yaml
>>>>
>>>> as I said it is completely different IP which requires complete different 
>>>> driver
>>>> but IP designers choose similar name which is out of developer control.
>>>>
>>>>>
>>>>> Shall we expect in two years, a third document like:
>>>>> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
>>>>> ?
>>>>
>>>> Developer definitely doesn't know. If new SoC requires for the same purpose
>>>> different IP with completely different driver is something out of developer
>>>> control. As of today I am not aware about such a requirement and need and
>>>> personally I can just hope that if they need to do such a change they will be
>>>> able to keep current SW driver compatible with new HW IP.
>>>
>>> Then please start naming them reasonable, not two (and in future
>>> x-times) the same names for entirely different blocks. And by name I
>>> mean compatible, filename and device name.
>>>
>>>>>> also for this IP if that's fine with you.
>>>>>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
>>>>>
>>>>> That old binding also explained nr-outputs as "Number of outputs".
>>>>> Perfect... :(
>>>>
>>>> Anyway if description should be improved let's just do it. I just want to get
>>>> guidance if we should update current dt binding for similar IP or just create
>>>> new one as this one is trying to do.
>>>
>>> IMHO, new binding is extremely confusing. We already have support for
>>> devices named "xlnx,clocking-wizard" and now you add exactly the same
>>> (clk=clocking) with almost the same properties, named
>>> "xlnx,clk-wizard-1.0". For a different IP?
>>>
>>> How anyone (even Xilinx' customer) can understand which block is for
>>> what if they have exactly the same name and (almost) the same
>>> properties, but as you said - these are entirely different IP?
>>
>> Maybe we should just delete the staging one (and the staging driver),
>> and start over?  No one has taken the time to get the staging driver out
>> of there, so I have no objection to dropping it for 6.1.
> 
> As I said it is be out of staging in linux-next. When CLK tree is merged in 
> these 2 weeks we are done at least with this driver.

FYI: Here is link where I asked you for your ACK to get the driver out of staging.
https://lore.kernel.org/all/Ys%2F%2FaPLkLGaooYYw@kroah.com/

Thanks,
Michal
Krzysztof Kozlowski Oct. 3, 2022, 10:50 a.m. UTC | #13
On 03/10/2022 12:37, Michal Simek wrote:
> 
> 
> On 10/3/22 10:10, Krzysztof Kozlowski wrote:
>> On 03/10/2022 09:58, Michal Simek wrote:
>>>
>>>
>>> On 10/3/22 09:23, Krzysztof Kozlowski wrote:
>>>> On 03/10/2022 09:15, Michal Simek wrote:
>>>>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>>>>
>>>>>> Do we need a whole new schema doc?
>>>>>
>>>>> It is completely new IP with different logic compare to origin one.
>>>>>
>>>>>>
>>>>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>>>>> once. And it's only a new compatible string.
>>>>>
>>>>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>>>>
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>>
>>>> So we already have out of staging document:
>>>> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>
>>> in 6.1 yes.
>>>
>>>>
>>>> and author wants to add one more:
>>>> devicetree/bindings/clock/xlnx,clk-wizard.yaml
>>>
>>> as I said it is completely different IP which requires complete different driver
>>> but IP designers choose similar name which is out of developer control.
>>>
>>>>
>>>> Shall we expect in two years, a third document like:
>>>> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
>>>> ?
>>>
>>> Developer definitely doesn't know. If new SoC requires for the same purpose
>>> different IP with completely different driver is something out of developer
>>> control. As of today I am not aware about such a requirement and need and
>>> personally I can just hope that if they need to do such a change they will be
>>> able to keep current SW driver compatible with new HW IP.
>>
>> Then please start naming them reasonable, not two (and in future
>> x-times) the same names for entirely different blocks. And by name I
>> mean compatible, filename and device name.
>>
>>>>> also for this IP if that's fine with you.
>>>>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
>>>>
>>>> That old binding also explained nr-outputs as "Number of outputs".
>>>> Perfect... :(
>>>
>>> Anyway if description should be improved let's just do it. I just want to get
>>> guidance if we should update current dt binding for similar IP or just create
>>> new one as this one is trying to do.
>>
>> IMHO, new binding is extremely confusing. We already have support for
>> devices named "xlnx,clocking-wizard" and now you add exactly the same
>> (clk=clocking) with almost the same properties, named
>> "xlnx,clk-wizard-1.0". For a different IP?
>>
>> How anyone (even Xilinx' customer) can understand which block is for
>> what if they have exactly the same name and (almost) the same
>> properties, but as you said - these are entirely different IP?
> 
> Let me start from last one. Xilinx has IP catalog in design tools called Vivado. 
> You choose device you have and then you will find clk wizard and you get an IP.

So you have a specific device? Why it is not part of name and compatible?

> It means depends on SOC you have ZynqMP or Versal and based on that one or 
> another is taken.

Exactly. The names xlnx,clocking-wizard and xlnx,clk-wizard-1.0 are
therefore not specific enough and mixing different devices.


> And because this is fpga world none is really describing programmable logic by 
> hand because it would take a look a lot of time. That's why I created long time 
> ago device-tree generator (DTG) which gets design data and based on it generate 
> device tree description. Newest version is available for example here.
> https://github.com/Xilinx/device-tree-xlnx
> There is also newer version called system device tree generato
> https://github.com/Xilinx/system-device-tree-xlnx
> 
> Because of this infrastructure user will all the time get proper compatible 
> string which is aligned with IP catalog.

I don't think so. Let's skip for now "clk" and "clocking" differences
and assume both are "clocking". You have then compatibles:

xlnx,clocking-wizard and xlnx,clocking-wizard-1.0

and you said these are entirely different blocks.

There is no way this creates readable DTS.

Best regards,
Krzysztof
Greg KH Oct. 3, 2022, 2:50 p.m. UTC | #14
On Mon, Oct 03, 2022 at 12:41:34PM +0200, Michal Simek wrote:
> Hi again,
> 
> On 10/3/22 11:59, Michal Simek wrote:
> > Hi Greg,
> > 
> > On 10/3/22 10:16, Greg Kroah-Hartman wrote:
> > > On Mon, Oct 03, 2022 at 10:10:38AM +0200, Krzysztof Kozlowski wrote:
> > > > On 03/10/2022 09:58, Michal Simek wrote:
> > > > > 
> > > > > 
> > > > > On 10/3/22 09:23, Krzysztof Kozlowski wrote:
> > > > > > On 03/10/2022 09:15, Michal Simek wrote:
> > > > > > > > > And this is new IP. Not sure who has chosen similar name but this targets
> > > > > > > > > Xilinx Versal SOCs. Origin one was targeting previous families.
> > > > > > > > 
> > > > > > > > Do we need a whole new schema doc?
> > > > > > > 
> > > > > > > It is completely new IP with different logic compare to origin one.
> > > > > > > 
> > > > > > > > 
> > > > > > > > It is not ideal to define the same property, xlnx,nr-outputs, more than
> > > > > > > > once. And it's only a new compatible string.
> > > > > > > 
> > > > > > > I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
> > > > > > > 
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> > > > > > > 
> > > > > > 
> > > > > > So we already have out of staging document:
> > > > > > devicetree/bindings/clock/xlnx,clocking-wizard.yaml
> > > > > 
> > > > > in 6.1 yes.
> > > > > 
> > > > > > 
> > > > > > and author wants to add one more:
> > > > > > devicetree/bindings/clock/xlnx,clk-wizard.yaml
> > > > > 
> > > > > as I said it is completely different IP which requires
> > > > > complete different driver
> > > > > but IP designers choose similar name which is out of developer control.
> > > > > 
> > > > > > 
> > > > > > Shall we expect in two years, a third document like:
> > > > > > devicetree/bindings/clock/xlnx,clk-wzrd.yaml
> > > > > > ?
> > > > > 
> > > > > Developer definitely doesn't know. If new SoC requires for the same purpose
> > > > > different IP with completely different driver is something out of developer
> > > > > control. As of today I am not aware about such a requirement and need and
> > > > > personally I can just hope that if they need to do such a change they will be
> > > > > able to keep current SW driver compatible with new HW IP.
> > > > 
> > > > Then please start naming them reasonable, not two (and in future
> > > > x-times) the same names for entirely different blocks. And by name I
> > > > mean compatible, filename and device name.
> > > > 
> > > > > > > also for this IP if that's fine with you.
> > > > > > > Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
> > > > > > 
> > > > > > That old binding also explained nr-outputs as "Number of outputs".
> > > > > > Perfect... :(
> > > > > 
> > > > > Anyway if description should be improved let's just do it. I just want to get
> > > > > guidance if we should update current dt binding for similar IP or just create
> > > > > new one as this one is trying to do.
> > > > 
> > > > IMHO, new binding is extremely confusing. We already have support for
> > > > devices named "xlnx,clocking-wizard" and now you add exactly the same
> > > > (clk=clocking) with almost the same properties, named
> > > > "xlnx,clk-wizard-1.0". For a different IP?
> > > > 
> > > > How anyone (even Xilinx' customer) can understand which block is for
> > > > what if they have exactly the same name and (almost) the same
> > > > properties, but as you said - these are entirely different IP?
> > > 
> > > Maybe we should just delete the staging one (and the staging driver),
> > > and start over?  No one has taken the time to get the staging driver out
> > > of there, so I have no objection to dropping it for 6.1.
> > 
> > As I said it is be out of staging in linux-next. When CLK tree is merged
> > in these 2 weeks we are done at least with this driver.
> 
> FYI: Here is link where I asked you for your ACK to get the driver out of staging.
> https://lore.kernel.org/all/Ys%2F%2FaPLkLGaooYYw@kroah.com/

Ah, good, I forgot about that, nevermind!

greg k-h
Michal Simek Oct. 3, 2022, 3:27 p.m. UTC | #15
On 10/3/22 12:50, Krzysztof Kozlowski wrote:
> On 03/10/2022 12:37, Michal Simek wrote:
>>
>>
>> On 10/3/22 10:10, Krzysztof Kozlowski wrote:
>>> On 03/10/2022 09:58, Michal Simek wrote:
>>>>
>>>>
>>>> On 10/3/22 09:23, Krzysztof Kozlowski wrote:
>>>>> On 03/10/2022 09:15, Michal Simek wrote:
>>>>>>>> And this is new IP. Not sure who has chosen similar name but this targets
>>>>>>>> Xilinx Versal SOCs. Origin one was targeting previous families.
>>>>>>>
>>>>>>> Do we need a whole new schema doc?
>>>>>>
>>>>>> It is completely new IP with different logic compare to origin one.
>>>>>>
>>>>>>>
>>>>>>> It is not ideal to define the same property, xlnx,nr-outputs, more than
>>>>>>> once. And it's only a new compatible string.
>>>>>>
>>>>>> I can't see any issue with using dt binding for xlnx,clocking-wizard.yaml
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>>>
>>>>> So we already have out of staging document:
>>>>> devicetree/bindings/clock/xlnx,clocking-wizard.yaml
>>>>
>>>> in 6.1 yes.
>>>>
>>>>>
>>>>> and author wants to add one more:
>>>>> devicetree/bindings/clock/xlnx,clk-wizard.yaml
>>>>
>>>> as I said it is completely different IP which requires complete different driver
>>>> but IP designers choose similar name which is out of developer control.
>>>>
>>>>>
>>>>> Shall we expect in two years, a third document like:
>>>>> devicetree/bindings/clock/xlnx,clk-wzrd.yaml
>>>>> ?
>>>>
>>>> Developer definitely doesn't know. If new SoC requires for the same purpose
>>>> different IP with completely different driver is something out of developer
>>>> control. As of today I am not aware about such a requirement and need and
>>>> personally I can just hope that if they need to do such a change they will be
>>>> able to keep current SW driver compatible with new HW IP.
>>>
>>> Then please start naming them reasonable, not two (and in future
>>> x-times) the same names for entirely different blocks. And by name I
>>> mean compatible, filename and device name.
>>>
>>>>>> also for this IP if that's fine with you.
>>>>>> Only xlnx,speed-grade can be defined for previous IP which is easy to mark.
>>>>>
>>>>> That old binding also explained nr-outputs as "Number of outputs".
>>>>> Perfect... :(
>>>>
>>>> Anyway if description should be improved let's just do it. I just want to get
>>>> guidance if we should update current dt binding for similar IP or just create
>>>> new one as this one is trying to do.
>>>
>>> IMHO, new binding is extremely confusing. We already have support for
>>> devices named "xlnx,clocking-wizard" and now you add exactly the same
>>> (clk=clocking) with almost the same properties, named
>>> "xlnx,clk-wizard-1.0". For a different IP?
>>>
>>> How anyone (even Xilinx' customer) can understand which block is for
>>> what if they have exactly the same name and (almost) the same
>>> properties, but as you said - these are entirely different IP?
>>
>> Let me start from last one. Xilinx has IP catalog in design tools called Vivado.
>> You choose device you have and then you will find clk wizard and you get an IP.
> 
> So you have a specific device? Why it is not part of name and compatible?
> 
>> It means depends on SOC you have ZynqMP or Versal and based on that one or
>> another is taken.
> 
> Exactly. The names xlnx,clocking-wizard and xlnx,clk-wizard-1.0 are
> therefore not specific enough and mixing different devices.

And just to be clear these IPs can be combined with systems where the main cpu 
can be Microblaze. I have also seen some vendors mixing RISC-V with Xilinx IPs.

Please look below.
> 
>> And because this is fpga world none is really describing programmable logic by
>> hand because it would take a look a lot of time. That's why I created long time
>> ago device-tree generator (DTG) which gets design data and based on it generate
>> device tree description. Newest version is available for example here.
>> https://github.com/Xilinx/device-tree-xlnx
>> There is also newer version called system device tree generato
>> https://github.com/Xilinx/system-device-tree-xlnx
>>
>> Because of this infrastructure user will all the time get proper compatible
>> string which is aligned with IP catalog.
> 
> I don't think so. Let's skip for now "clk" and "clocking" differences
> and assume both are "clocking". You have then compatibles:
> 
> xlnx,clocking-wizard and xlnx,clocking-wizard-1.0
> 
> and you said these are entirely different blocks.
> 
> There is no way this creates readable DTS.

And I really thank you for this discussion to do it properly and have proper 
compatible string and description for this block.

Shubhrajyoti: please correct me if I am wrong.

All Xilinx SOCs have programmable logic aligned with FPGAs. Zynq is based 28nm,
ZynqMP (Ultrascale MPSOC) is based on 16nm and Versal is based on 7nm.

I think these clocking IPs are using low level primitives available in PL logic.
Which means there is connection to fpga/pl technology instead of SOC family and 
main cpu.
It can be of course said that if this is ZynqMP SOC that IP A is used. The same 
for Versal SOC. But for soft cores this can't be said.

Would it be better to reflect PL technology in compatible string?

xlnx,clocking-wizard-vX.X (used now for ZynqMP and previous families)
xlnx,clocking-wizard-7nm-vX.X (or find better name names which reflect PL logic 
type)

Thanks,
Michal
Krzysztof Kozlowski Oct. 4, 2022, 11 a.m. UTC | #16
On 03/10/2022 17:27, Michal Simek wrote:
>>
>> Exactly. The names xlnx,clocking-wizard and xlnx,clk-wizard-1.0 are
>> therefore not specific enough and mixing different devices.
> 
> And just to be clear these IPs can be combined with systems where the main cpu 
> can be Microblaze. I have also seen some vendors mixing RISC-V with Xilinx IPs.
> 
> Please look below.
>>
>>> And because this is fpga world none is really describing programmable logic by
>>> hand because it would take a look a lot of time. That's why I created long time
>>> ago device-tree generator (DTG) which gets design data and based on it generate
>>> device tree description. Newest version is available for example here.
>>> https://github.com/Xilinx/device-tree-xlnx
>>> There is also newer version called system device tree generato
>>> https://github.com/Xilinx/system-device-tree-xlnx
>>>
>>> Because of this infrastructure user will all the time get proper compatible
>>> string which is aligned with IP catalog.
>>
>> I don't think so. Let's skip for now "clk" and "clocking" differences
>> and assume both are "clocking". You have then compatibles:
>>
>> xlnx,clocking-wizard and xlnx,clocking-wizard-1.0
>>
>> and you said these are entirely different blocks.
>>
>> There is no way this creates readable DTS.
> 
> And I really thank you for this discussion to do it properly and have proper 
> compatible string and description for this block.
> 
> Shubhrajyoti: please correct me if I am wrong.
> 
> All Xilinx SOCs have programmable logic aligned with FPGAs. Zynq is based 28nm,
> ZynqMP (Ultrascale MPSOC) is based on 16nm and Versal is based on 7nm.
> 
> I think these clocking IPs are using low level primitives available in PL logic.
> Which means there is connection to fpga/pl technology instead of SOC family and 
> main cpu.

Then maybe the compatibles (and device names) should have that fpga/pl
technology used to differentiate between them?

> It can be of course said that if this is ZynqMP SOC that IP A is used. The same 
> for Versal SOC. But for soft cores this can't be said.
> 
> Would it be better to reflect PL technology in compatible string?

Yes, although we might misunderstand what PL technology is. 28/16/7 nm
is the size of transistor or the process. Even two different processes
can use same type of technology, e.g. FinFET:
https://en.wikipedia.org/wiki/14_nm_process
https://en.wikipedia.org/wiki/10_nm_process

You could have very similar (or even the same) designs done in 28 nm and
16 nm. Does it mean these are entirely different devices? Not
necessarily... Maybe they are, maybe not, but is the size of process
differentiating? I actually don't know what's there in 28/16/7, I am
just saying that number alone might not mean different technology.
Programming API could be the same, inputs/outputs could be the same.
Just the size of transistor is different...

> 
> xlnx,clocking-wizard-vX.X (used now for ZynqMP and previous families)
> xlnx,clocking-wizard-7nm-vX.X (or find better name names which reflect PL logic 
> type)

Best regards,
Krzysztof
Michal Simek Oct. 4, 2022, 11:04 a.m. UTC | #17
On 10/4/22 13:00, Krzysztof Kozlowski wrote:
> On 03/10/2022 17:27, Michal Simek wrote:
>>>
>>> Exactly. The names xlnx,clocking-wizard and xlnx,clk-wizard-1.0 are
>>> therefore not specific enough and mixing different devices.
>>
>> And just to be clear these IPs can be combined with systems where the main cpu
>> can be Microblaze. I have also seen some vendors mixing RISC-V with Xilinx IPs.
>>
>> Please look below.
>>>
>>>> And because this is fpga world none is really describing programmable logic by
>>>> hand because it would take a look a lot of time. That's why I created long time
>>>> ago device-tree generator (DTG) which gets design data and based on it generate
>>>> device tree description. Newest version is available for example here.
>>>> https://github.com/Xilinx/device-tree-xlnx
>>>> There is also newer version called system device tree generato
>>>> https://github.com/Xilinx/system-device-tree-xlnx
>>>>
>>>> Because of this infrastructure user will all the time get proper compatible
>>>> string which is aligned with IP catalog.
>>>
>>> I don't think so. Let's skip for now "clk" and "clocking" differences
>>> and assume both are "clocking". You have then compatibles:
>>>
>>> xlnx,clocking-wizard and xlnx,clocking-wizard-1.0
>>>
>>> and you said these are entirely different blocks.
>>>
>>> There is no way this creates readable DTS.
>>
>> And I really thank you for this discussion to do it properly and have proper
>> compatible string and description for this block.
>>
>> Shubhrajyoti: please correct me if I am wrong.
>>
>> All Xilinx SOCs have programmable logic aligned with FPGAs. Zynq is based 28nm,
>> ZynqMP (Ultrascale MPSOC) is based on 16nm and Versal is based on 7nm.
>>
>> I think these clocking IPs are using low level primitives available in PL logic.
>> Which means there is connection to fpga/pl technology instead of SOC family and
>> main cpu.
> 
> Then maybe the compatibles (and device names) should have that fpga/pl
> technology used to differentiate between them?

I am already trying to find out better generic description without mentioning 
sizes.


>> It can be of course said that if this is ZynqMP SOC that IP A is used. The same
>> for Versal SOC. But for soft cores this can't be said.
>>
>> Would it be better to reflect PL technology in compatible string?
> 
> Yes, although we might misunderstand what PL technology is. 28/16/7 nm
> is the size of transistor or the process. Even two different processes
> can use same type of technology, e.g. FinFET:
> https://en.wikipedia.org/wiki/14_nm_process
> https://en.wikipedia.org/wiki/10_nm_process
> 
> You could have very similar (or even the same) designs done in 28 nm and
> 16 nm. Does it mean these are entirely different devices? Not
> necessarily... Maybe they are, maybe not, but is the size of process
> differentiating? I actually don't know what's there in 28/16/7, I am
> just saying that number alone might not mean different technology.
> Programming API could be the same, inputs/outputs could be the same.
> Just the size of transistor is different...

I agree. Will try to come up with better name without nm inside to uniquely 
identify PL logic type.

Thanks,
Michal
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
new file mode 100644
index 000000000000..41a6f4bcaccd
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml
@@ -0,0 +1,66 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/xlnx,clk-wizard.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx Versal clocking wizard
+
+maintainers:
+  - Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
+
+description:
+  The clocking wizard is a soft ip clocking block of Xilinx versal. The IP
+  uses the input clock frequencies and generates the requested
+  clock output.
+
+properties:
+  compatible:
+    const: xlnx,clk-wizard-1.0
+
+  reg:
+    maxItems: 1
+
+  "#clock-cells":
+    const: 1
+
+  clocks:
+    description: List of clock specifiers which are external input
+      clocks to the given clock controller.
+    items:
+      - description: functional clock input
+      - description: axi clock or the interface clock
+
+  clock-names:
+    items:
+      - const: clk_in1
+      - const: s_axi_aclk
+
+  xlnx,nr-outputs:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 8
+    description:
+      Number of outputs.
+
+required:
+  - compatible
+  - reg
+  - "#clock-cells"
+  - clocks
+  - clock-names
+  - xlnx,nr-outputs
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-generator@40040000 {
+        compatible = "xlnx,clk-wizard-1.0";
+        reg = <0x40040000 0x1000>;
+        #clock-cells = <1>;
+        clocks = <&clkc 15>, <&clkc 15>;
+        clock-names = "clk_in1", "s_axi_aclk";
+        xlnx,nr-outputs = <6>;
+    };
+...