* [PATCH 0/2] clocking-wizard: Add versal clocking wizard support @ 2022-09-30 8:03 Shubhrajyoti Datta 2022-09-30 8:03 ` [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard Shubhrajyoti Datta 2022-09-30 8:04 ` [PATCH 2/2] clocking-wizard: Add versal clocking wizard support Shubhrajyoti Datta 0 siblings, 2 replies; 20+ messages in thread From: Shubhrajyoti Datta @ 2022-09-30 8:03 UTC (permalink / raw) To: linux-clk Cc: git, devicetree, krzysztof.kozlowski+dt, robh+dt, sboyd, mturquette Add Versal clocking wizard IP driver support The Versal clocking wizard is clock circuits customized to cater to clocking requirements. It provides configurable number of outputs. Datasheet: https://docs.xilinx.com/r/en-US/pg321-clocking-wizard Shubhrajyoti Datta (2): dt-bindings: clk: Add binding for versal clocking wizard clocking-wizard: Add versal clocking wizard support .../bindings/clock/xlnx,clk-wizard.yaml | 66 ++ drivers/clk/xilinx/Kconfig | 13 + drivers/clk/xilinx/Makefile | 1 + drivers/clk/xilinx/clk-xlnx-clock-wizard-v.c | 740 ++++++++++++++++++ 4 files changed, 820 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/xlnx,clk-wizard.yaml create mode 100644 drivers/clk/xilinx/clk-xlnx-clock-wizard-v.c -- 2.17.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-09-30 8:03 [PATCH 0/2] clocking-wizard: Add versal clocking wizard support Shubhrajyoti Datta @ 2022-09-30 8:03 ` Shubhrajyoti Datta 2022-09-30 11:19 ` Krzysztof Kozlowski 2022-09-30 12:25 ` Rob Herring 2022-09-30 8:04 ` [PATCH 2/2] clocking-wizard: Add versal clocking wizard support Shubhrajyoti Datta 1 sibling, 2 replies; 20+ messages in thread From: Shubhrajyoti Datta @ 2022-09-30 8:03 UTC (permalink / raw) To: linux-clk Cc: git, devicetree, krzysztof.kozlowski+dt, robh+dt, sboyd, mturquette 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 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>; + }; +... -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-09-30 8:03 ` [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard Shubhrajyoti Datta @ 2022-09-30 11:19 ` Krzysztof Kozlowski 2022-09-30 12:25 ` Rob Herring 1 sibling, 0 replies; 20+ messages in thread From: Krzysztof Kozlowski @ 2022-09-30 11:19 UTC (permalink / raw) To: Shubhrajyoti Datta, linux-clk Cc: git, devicetree, krzysztof.kozlowski+dt, robh+dt, sboyd, mturquette 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-09-30 8:03 ` [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard Shubhrajyoti Datta 2022-09-30 11:19 ` Krzysztof Kozlowski @ 2022-09-30 12:25 ` Rob Herring 2022-09-30 13:00 ` Michal Simek 1 sibling, 1 reply; 20+ messages in thread From: Rob Herring @ 2022-09-30 12:25 UTC (permalink / raw) To: Shubhrajyoti Datta Cc: linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-09-30 12:25 ` Rob Herring @ 2022-09-30 13:00 ` Michal Simek 2022-09-30 21:39 ` Rob Herring 0 siblings, 1 reply; 20+ messages in thread From: Michal Simek @ 2022-09-30 13:00 UTC (permalink / raw) To: Rob Herring, Shubhrajyoti Datta Cc: linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-09-30 13:00 ` Michal Simek @ 2022-09-30 21:39 ` Rob Herring 2022-10-03 7:15 ` Michal Simek 0 siblings, 1 reply; 20+ messages in thread From: Rob Herring @ 2022-09-30 21:39 UTC (permalink / raw) To: Michal Simek Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-09-30 21:39 ` Rob Herring @ 2022-10-03 7:15 ` Michal Simek 2022-10-03 7:23 ` Krzysztof Kozlowski 0 siblings, 1 reply; 20+ messages in thread From: Michal Simek @ 2022-10-03 7:15 UTC (permalink / raw) To: Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 7:15 ` Michal Simek @ 2022-10-03 7:23 ` Krzysztof Kozlowski 2022-10-03 7:58 ` Michal Simek 0 siblings, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2022-10-03 7:23 UTC (permalink / raw) To: Michal Simek, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 7:23 ` Krzysztof Kozlowski @ 2022-10-03 7:58 ` Michal Simek 2022-10-03 8:10 ` Krzysztof Kozlowski 0 siblings, 1 reply; 20+ messages in thread From: Michal Simek @ 2022-10-03 7:58 UTC (permalink / raw) To: Krzysztof Kozlowski, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 7:58 ` Michal Simek @ 2022-10-03 8:10 ` Krzysztof Kozlowski 2022-10-03 8:16 ` Greg Kroah-Hartman 2022-10-03 10:37 ` Michal Simek 0 siblings, 2 replies; 20+ messages in thread From: Krzysztof Kozlowski @ 2022-10-03 8:10 UTC (permalink / raw) To: Michal Simek, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 8:10 ` Krzysztof Kozlowski @ 2022-10-03 8:16 ` Greg Kroah-Hartman 2022-10-03 9:59 ` Michal Simek 2022-10-03 10:37 ` Michal Simek 1 sibling, 1 reply; 20+ messages in thread From: Greg Kroah-Hartman @ 2022-10-03 8:16 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Michal Simek, Rob Herring, Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 8:16 ` Greg Kroah-Hartman @ 2022-10-03 9:59 ` Michal Simek 2022-10-03 10:41 ` Michal Simek 0 siblings, 1 reply; 20+ messages in thread From: Michal Simek @ 2022-10-03 9:59 UTC (permalink / raw) To: Greg Kroah-Hartman, Krzysztof Kozlowski Cc: Rob Herring, Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 9:59 ` Michal Simek @ 2022-10-03 10:41 ` Michal Simek 2022-10-03 14:50 ` Greg Kroah-Hartman 0 siblings, 1 reply; 20+ messages in thread From: Michal Simek @ 2022-10-03 10:41 UTC (permalink / raw) To: Greg Kroah-Hartman, Krzysztof Kozlowski Cc: Rob Herring, Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 10:41 ` Michal Simek @ 2022-10-03 14:50 ` Greg Kroah-Hartman 0 siblings, 0 replies; 20+ messages in thread From: Greg Kroah-Hartman @ 2022-10-03 14:50 UTC (permalink / raw) To: Michal Simek Cc: Krzysztof Kozlowski, Rob Herring, Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 8:10 ` Krzysztof Kozlowski 2022-10-03 8:16 ` Greg Kroah-Hartman @ 2022-10-03 10:37 ` Michal Simek 2022-10-03 10:50 ` Krzysztof Kozlowski 1 sibling, 1 reply; 20+ messages in thread From: Michal Simek @ 2022-10-03 10:37 UTC (permalink / raw) To: Krzysztof Kozlowski, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 10:37 ` Michal Simek @ 2022-10-03 10:50 ` Krzysztof Kozlowski 2022-10-03 15:27 ` Michal Simek 0 siblings, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2022-10-03 10:50 UTC (permalink / raw) To: Michal Simek, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 10:50 ` Krzysztof Kozlowski @ 2022-10-03 15:27 ` Michal Simek 2022-10-04 11:00 ` Krzysztof Kozlowski 0 siblings, 1 reply; 20+ messages in thread From: Michal Simek @ 2022-10-03 15:27 UTC (permalink / raw) To: Krzysztof Kozlowski, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-03 15:27 ` Michal Simek @ 2022-10-04 11:00 ` Krzysztof Kozlowski 2022-10-04 11:04 ` Michal Simek 0 siblings, 1 reply; 20+ messages in thread From: Krzysztof Kozlowski @ 2022-10-04 11:00 UTC (permalink / raw) To: Michal Simek, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard 2022-10-04 11:00 ` Krzysztof Kozlowski @ 2022-10-04 11:04 ` Michal Simek 0 siblings, 0 replies; 20+ messages in thread From: Michal Simek @ 2022-10-04 11:04 UTC (permalink / raw) To: Krzysztof Kozlowski, Rob Herring Cc: Shubhrajyoti Datta, linux-clk, git, devicetree, krzysztof.kozlowski+dt, sboyd, mturquette, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH 2/2] clocking-wizard: Add versal clocking wizard support 2022-09-30 8:03 [PATCH 0/2] clocking-wizard: Add versal clocking wizard support Shubhrajyoti Datta 2022-09-30 8:03 ` [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard Shubhrajyoti Datta @ 2022-09-30 8:04 ` Shubhrajyoti Datta 1 sibling, 0 replies; 20+ messages in thread From: Shubhrajyoti Datta @ 2022-09-30 8:04 UTC (permalink / raw) To: linux-clk Cc: git, devicetree, krzysztof.kozlowski+dt, robh+dt, sboyd, mturquette The Clocking Wizard for Versal adaptive compute acceleration platforms. Add Versal clocking wizard support. The driver supports configurable number of outputs. Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> --- drivers/clk/xilinx/Kconfig | 13 + drivers/clk/xilinx/Makefile | 1 + drivers/clk/xilinx/clk-xlnx-clock-wizard-v.c | 740 +++++++++++++++++++ 3 files changed, 754 insertions(+) create mode 100644 drivers/clk/xilinx/clk-xlnx-clock-wizard-v.c diff --git a/drivers/clk/xilinx/Kconfig b/drivers/clk/xilinx/Kconfig index 5b99ecfd2f06..f2a4bfa963c0 100644 --- a/drivers/clk/xilinx/Kconfig +++ b/drivers/clk/xilinx/Kconfig @@ -28,3 +28,16 @@ config COMMON_CLK_XLNX_CLKWZRD If unsure, say N. +config COMMON_CLK_XLNX_CLKWZRD_V + tristate "Xilinx Versal Clocking Wizard" + depends on COMMON_CLK && OF + help + Support for the Versal Xilinx Clocking Wizard IP core clock generator. + This driver supports the Versal Xilinx clocking wizard programmable clock + synthesizer. The number of output is configurable in the design. + + If unsure, say N. + + To compile this driver as a module, choose M here: the + module will be called clk-xlnx-clock-wizard-v. + diff --git a/drivers/clk/xilinx/Makefile b/drivers/clk/xilinx/Makefile index 7ac1789c6b1b..9e7e1c1c026c 100644 --- a/drivers/clk/xilinx/Makefile +++ b/drivers/clk/xilinx/Makefile @@ -1,3 +1,4 @@ # SPDX-License-Identifier: GPL-2.0 obj-$(CONFIG_XILINX_VCU) += xlnx_vcu.o +obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD_V) += clk-xlnx-clock-wizard-v.o obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD) += clk-xlnx-clock-wizard.o diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard-v.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard-v.c new file mode 100644 index 000000000000..7eff5c2a5755 --- /dev/null +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard-v.c @@ -0,0 +1,740 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Xilinx Versal Clocking Wizard driver + * + * Copyright (C) 2020 - 2022, Xilinx, Inc. + * Copyright (C) 2022, Advanced Micro Devices, Inc. + * + * Shubhrajyoti Datta <shubhrajyoti.datta@amd.com> + */ + +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/iopoll.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#define WZRD_NUM_OUTPUTS 7 +#define WZRD_ACLK_MAX_FREQ 250000000UL + +#define WZRD_CLK_CFG_REG(n) (0x330 + 4 * (n)) + +#define WZRD_CLKFBOUT_1 0 +#define WZRD_CLKFBOUT_2 1 +#define WZRD_CLKOUT0_1 2 +#define WZRD_CLKOUT0_2 3 +#define WZRD_DESKEW_2 20 +#define WZRD_DIVCLK 21 +#define WZRD_CLKFBOUT_4 51 +#define WZRD_CLKFBOUT_3 48 +#define WZRD_DUTY_CYCLE 2 +#define WZRD_O_DIV 4 + +#define WZRD_CLKFBOUT_FRAC_EN BIT(1) +#define WZRD_CLKFBOUT_PREDIV2 (BIT(11) | BIT(12) | BIT(9)) +#define WZRD_MULT_PREDIV2 (BIT(10) | BIT(9) | BIT(12)) +#define WZRD_CLKFBOUT_EDGE BIT(8) +#define WZRD_EDGE_SHIFT 8 +#define WZRD_P5EN BIT(13) +#define WZRD_P5EN_SHIFT 13 +#define WZRD_P5FEDGE BIT(15) +#define WZRD_P5FEDGE_SHIFT 15 +#define WZRD_DIVCLK_EDGE BIT(10) +#define WZRD_CLKOUT0_PREDIV2 BIT(11) + +#define WZRD_CLKFBOUT_L_SHIFT 0 +#define WZRD_CLKFBOUT_H_SHIFT 8 +#define WZRD_CLKFBOUT_L_MASK GENMASK(7, 0) +#define WZRD_CLKFBOUT_H_MASK GENMASK(15, 8) +#define WZRD_CLKFBOUT_FRAC_SHIFT 16 +#define WZRD_CLKFBOUT_FRAC_MASK GENMASK(5, 0) +#define WZRD_DIVCLK_DIVIDE_MASK GENMASK(7, 0) +#define WZRD_CLKOUT_DIVIDE_SHIFT 0 +#define WZRD_CLKOUT_DIVIDE_WIDTH 8 +#define WZRD_CLKOUT_DIVIDE_MASK GENMASK(7, 0) +#define WZRD_CLKOUT_FRAC_SHIFT 8 +#define WZRD_CLKOUT_FRAC_MASK 0x3ff + +#define WZRD_DR_MAX_INT_DIV_VALUE 32767 +#define WZRD_DR_STATUS_REG_OFFSET 0x04 +#define WZRD_DR_LOCK_BIT_MASK BIT(0) +#define WZRD_DR_INIT_REG_OFFSET 0x14 +#define WZRD_DR_DIV_TO_PHASE_OFFSET 4 +#define WZRD_DR_BEGIN_DYNA_RECONF 0x03 +#define WZRD_MIN_ERR 500000 +#define WZRD_USEC_POLL 10 +#define WZRD_TIMEOUT_POLL 1000 +#define WZRD_FRAC_GRADIENT 64 +#define PREDIV2_MULT 2 + +#define DIV_O 1 +#define DIV_ALL 3 + +#define WZRD_M_MIN 4 +#define WZRD_M_MAX 432 +#define WZRD_D_MIN 1 +#define WZRD_D_MAX 123 +#define WZRD_VCO_MIN 2160000000 +#define WZRD_VCO_MAX 4320000000 +#define WZRD_O_MIN 2 +#define WZRD_O_MAX 511 + +/* Extract divider instance from clock hardware instance */ +#define to_clk_wzrd_divider(_hw) container_of(_hw, struct clk_wzrd_divider, hw) + +enum clk_wzrd_int_clks { + wzrd_clk_mul, + wzrd_clk_mul_div, + wzrd_clk_int_max +}; + +/** + * struct clk_wzrd - Clock wizard private data structure + * + * @clk_data: Clock data + * @nb: Notifier block + * @base: Memory base + * @clk_in1: Handle to input clock 'clk_in1' + * @axi_clk: Handle to input clock 's_axi_aclk' + * @clks_internal: Internal clocks + * @clkout: Output clocks + */ +struct clk_wzrd { + struct clk_onecell_data clk_data; + struct notifier_block nb; + void __iomem *base; + struct clk *clk_in1; + struct clk *axi_clk; + struct clk *clks_internal[wzrd_clk_int_max]; + struct clk *clkout[WZRD_NUM_OUTPUTS]; +}; + +/** + * struct clk_wzrd_divider - clock divider specific to clk_wzrd + * + * @hw: handle between common and hardware-specific interfaces + * @base: base address of register containing the divider + * @offset: offset address of register containing the divider + * @shift: shift to the divider bit field + * @width: width of the divider bit field + * @flags: clk_wzrd divider flags + * @valuem: value of the multiplier + * @valued: value of the common divider + * @valueo: value of the leaf divider + * @table: array of value/divider pairs, last entry should have div = 0 + * @lock: register lock + */ +struct clk_wzrd_divider { + struct clk_hw hw; + void __iomem *base; + u16 offset; + u8 shift; + u8 width; + u8 flags; + u32 valuem; + u32 valued; + u32 valueo; + const struct clk_div_table *table; + spinlock_t *lock; /* divider lock */ +}; + +#define to_clk_wzrd(_nb) container_of(_nb, struct clk_wzrd, nb) + +/* spin lock variable for clk_wzrd */ +static DEFINE_SPINLOCK(clkwzrd_lock); + +static unsigned long clk_wzrd_recalc_rate_all(struct clk_hw *hw, + unsigned long parent_rate) +{ + u32 edged, div, div2, p5en, edge, prediv2, all, regl, regh, mult, reg; + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); + + edge = !!(readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_1)) & WZRD_CLKFBOUT_EDGE); + + reg = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_2)); + regl = FIELD_GET(WZRD_CLKFBOUT_L_MASK, reg); + regh = FIELD_GET(WZRD_CLKFBOUT_H_MASK, reg); + + mult = regl + regh + edge; + if (!mult) + mult = 1; + + regl = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_4)) & + WZRD_CLKFBOUT_FRAC_EN; + if (regl) { + regl = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_3)) & + WZRD_CLKFBOUT_FRAC_MASK; + mult = mult * WZRD_FRAC_GRADIENT + regl; + parent_rate = DIV_ROUND_CLOSEST((parent_rate * mult), WZRD_FRAC_GRADIENT); + } else { + parent_rate = parent_rate * mult; + } + + /* O Calculation */ + reg = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKOUT0_1)); + edged = FIELD_GET(WZRD_CLKFBOUT_EDGE, reg); + p5en = FIELD_GET(WZRD_P5EN, reg); + prediv2 = FIELD_GET(WZRD_CLKOUT0_PREDIV2, reg); + + reg = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKOUT0_2)); + /* Low time */ + regl = FIELD_GET(WZRD_CLKFBOUT_L_MASK, reg); + /* High time */ + regh = FIELD_GET(WZRD_CLKFBOUT_H_MASK, reg); + all = regh + regl + edged; + if (!all) + all = 1; + + if (prediv2) + div2 = PREDIV2_MULT * all + p5en; + else + div2 = all; + + /* D calculation */ + edged = !!(readl(divider->base + WZRD_CLK_CFG_REG(WZRD_DESKEW_2)) & + WZRD_DIVCLK_EDGE); + reg = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_DIVCLK)); + /* Low time */ + regl = FIELD_GET(WZRD_CLKFBOUT_L_MASK, reg); + /* High time */ + regh = FIELD_GET(WZRD_CLKFBOUT_H_MASK, reg); + div = regl + regh + edged; + if (!div) + div = 1; + + div = div * div2; + return divider_recalc_rate(hw, parent_rate, div, divider->table, + divider->flags, divider->width); +} + +static int clk_wzrd_get_divisors(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); + u64 vco_freq, freq, diff; + u32 m, d, o; + + for (m = WZRD_M_MIN; m <= WZRD_M_MAX; m++) { + for (d = WZRD_D_MIN; d <= WZRD_D_MAX; d++) { + vco_freq = DIV_ROUND_CLOSEST((parent_rate * m), d); + if (vco_freq >= WZRD_VCO_MIN && vco_freq <= WZRD_VCO_MAX) { + for (o = WZRD_O_MIN; o <= WZRD_O_MAX; o++) { + freq = DIV_ROUND_CLOSEST(vco_freq, o); + diff = abs(freq - rate); + + if (diff < WZRD_MIN_ERR) { + divider->valuem = m; + divider->valued = d; + divider->valueo = o; + return 0; + } + } + } + } + } + return -EBUSY; +} + +static int clk_wzrd_dynamic_all_nolock(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + u32 value, regh, edged, p5en, p5fedge, value2, m, regval, regval1; + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); + int err; + + err = clk_wzrd_get_divisors(hw, rate, parent_rate); + if (err) + return err; + + writel(0, divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_4)); + + m = divider->valuem; + edged = m % WZRD_DUTY_CYCLE; + regh = m / WZRD_DUTY_CYCLE; + regval1 = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_1)); + regval1 |= WZRD_MULT_PREDIV2; + if (edged) + regval1 = regval1 | WZRD_CLKFBOUT_EDGE; + else + regval1 = regval1 & ~WZRD_CLKFBOUT_EDGE; + + writel(regval1, divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_1)); + regval1 = regh | regh << WZRD_CLKFBOUT_H_SHIFT; + writel(regval1, divider->base + WZRD_CLK_CFG_REG(WZRD_CLKFBOUT_2)); + + value2 = divider->valued; + edged = value2 % WZRD_DUTY_CYCLE; + regh = (value2 / WZRD_DUTY_CYCLE); + regval1 = FIELD_PREP(WZRD_DIVCLK_EDGE, edged); + writel(regval1, divider->base + WZRD_CLK_CFG_REG(WZRD_DESKEW_2)); + regval1 = regh | regh << WZRD_CLKFBOUT_H_SHIFT; + writel(regval1, divider->base + WZRD_CLK_CFG_REG(WZRD_DIVCLK)); + + value = divider->valueo; + regh = value / WZRD_O_DIV; + regval1 = readl(divider->base + WZRD_CLK_CFG_REG(WZRD_CLKOUT0_1)); + regval1 |= WZRD_CLKFBOUT_PREDIV2; + regval1 = regval1 & ~(WZRD_CLKFBOUT_EDGE | WZRD_P5EN | WZRD_P5FEDGE); + + if (value % WZRD_O_DIV > 1) { + edged = 1; + regval1 |= edged << WZRD_CLKFBOUT_H_SHIFT; + } + + p5fedge = value % WZRD_DUTY_CYCLE; + p5en = value % WZRD_DUTY_CYCLE; + + regval1 = regval1 | FIELD_PREP(WZRD_P5EN, p5en) | FIELD_PREP(WZRD_P5FEDGE, p5fedge); + writel(regval1, divider->base + WZRD_CLK_CFG_REG(WZRD_CLKOUT0_1)); + regval = regh | regh << WZRD_CLKFBOUT_H_SHIFT; + writel(regval, divider->base + WZRD_CLK_CFG_REG(WZRD_CLKOUT0_2)); + + /* Check status register */ + err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, + value, value & WZRD_DR_LOCK_BIT_MASK, + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL); + if (err) + return err; + + /* Initiate reconfiguration */ + writel(WZRD_DR_BEGIN_DYNA_RECONF, + divider->base + WZRD_DR_INIT_REG_OFFSET); + + /* Check status register */ + return readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, + value, value & WZRD_DR_LOCK_BIT_MASK, + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL); +} + +static int clk_wzrd_dynamic_all(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); + unsigned long flags = 0; + int ret; + + if (divider->lock) + spin_lock_irqsave(divider->lock, flags); + else + __acquire(divider->lock); + + ret = clk_wzrd_dynamic_all_nolock(hw, rate, parent_rate); + + if (divider->lock) + spin_unlock_irqrestore(divider->lock, flags); + else + __release(divider->lock); + + return ret; +} + +static unsigned long clk_wzrd_recalc_rate(struct clk_hw *hw, + unsigned long parent_rate) +{ + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); + void __iomem *div_addr = divider->base + divider->offset; + u32 div, p5en, edge, prediv2, all; + unsigned int vall, valh; + + edge = !!(readl(div_addr) & WZRD_CLKFBOUT_EDGE); + p5en = !!(readl(div_addr) & WZRD_P5EN); + prediv2 = !!(readl(div_addr) & WZRD_CLKOUT0_PREDIV2); + vall = readl(div_addr + 4) & WZRD_CLKFBOUT_L_MASK; + valh = readl(div_addr + 4) >> WZRD_CLKFBOUT_H_SHIFT; + all = valh + vall + edge; + if (!all) + all = 1; + + if (prediv2) + div = 2 * all + prediv2 * p5en; + else + div = all; + + return DIV_ROUND_UP_ULL((u64)parent_rate, div); +} + +static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw); + void __iomem *div_addr = divider->base + divider->offset; + u32 value, regh, edged, p5en, p5fedge, regval, regval1; + unsigned long flags = 0; + int err; + + if (divider->lock) + spin_lock_irqsave(divider->lock, flags); + else + __acquire(divider->lock); + + value = DIV_ROUND_CLOSEST(parent_rate, rate); + regh = (value / 4); + regval1 = readl(div_addr); + regval1 |= WZRD_CLKFBOUT_PREDIV2; + regval1 = regval1 & ~(WZRD_CLKFBOUT_EDGE | WZRD_P5EN | WZRD_P5FEDGE); + if (value % 4 > 1) { + edged = 1; + regval1 |= (edged << WZRD_EDGE_SHIFT); + } + p5fedge = value % 2; + p5en = value % 2; + regval1 = regval1 | p5en << WZRD_P5EN_SHIFT | p5fedge << WZRD_P5FEDGE_SHIFT; + writel(regval1, div_addr); + + regval = regh | regh << WZRD_CLKFBOUT_H_SHIFT; + writel(regval, div_addr + 4); + /* Check status register */ + err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, + value, value & WZRD_DR_LOCK_BIT_MASK, + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL); + if (err) + goto err_reconfig; + + /* Initiate reconfiguration */ + writel(WZRD_DR_BEGIN_DYNA_RECONF, + divider->base + WZRD_DR_INIT_REG_OFFSET); + + /* Check status register */ + err = readl_poll_timeout(divider->base + WZRD_DR_STATUS_REG_OFFSET, + value, value & WZRD_DR_LOCK_BIT_MASK, + WZRD_USEC_POLL, WZRD_TIMEOUT_POLL); + +err_reconfig: + if (divider->lock) + spin_unlock_irqrestore(divider->lock, flags); + else + __release(divider->lock); + + return err; +} + +static long clk_wzrd_round_rate_all(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + return rate; +} + +static long clk_wzrd_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *prate) +{ + u8 div; + + div = DIV_ROUND_CLOSEST(*prate, rate); + + return *prate / div; +} + +static const struct clk_ops clk_wzrd_clk_divider_ops = { + .round_rate = clk_wzrd_round_rate, + .set_rate = clk_wzrd_dynamic_reconfig, + .recalc_rate = clk_wzrd_recalc_rate, +}; + +static const struct clk_ops clk_wzrd_clk_div_all_ops = { + .round_rate = clk_wzrd_round_rate_all, + .set_rate = clk_wzrd_dynamic_all, + .recalc_rate = clk_wzrd_recalc_rate_all, +}; + +static struct clk *clk_wzrd_register_divider(struct device *dev, + const char *name, + const char *parent_name, + unsigned long flags, + void __iomem *base, u16 offset, + u8 shift, u8 width, + u8 clk_divider_flags, + u32 div_type, + spinlock_t *lock) +{ + struct clk_wzrd_divider *div; + struct clk_init_data init; + struct clk_hw *hw; + int ret; + + if (clk_divider_flags & CLK_DIVIDER_HIWORD_MASK) { + if (width + shift > 16) { + dev_warn(dev, "divider value exceeds LOWORD field\n"); + return ERR_PTR(-EINVAL); + } + } + + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); + if (!div) + return ERR_PTR(-ENOMEM); + + init.name = name; + if (clk_divider_flags & CLK_DIVIDER_READ_ONLY) + init.ops = &clk_divider_ro_ops; + else if (div_type == DIV_O) + init.ops = &clk_wzrd_clk_divider_ops; + else + init.ops = &clk_wzrd_clk_div_all_ops; + + init.flags = flags; + init.parent_names = (parent_name ? &parent_name : NULL); + init.num_parents = (parent_name ? 1 : 0); + + div->base = base; + div->offset = offset; + div->shift = shift; + div->width = width; + div->flags = clk_divider_flags; + div->lock = lock; + div->hw.init = &init; + div->table = NULL; + + hw = &div->hw; + ret = devm_clk_hw_register(dev, hw); + if (ret) + return ERR_PTR(ret); + + return hw->clk; +} + +static int __maybe_unused clk_wzrd_suspend(struct device *dev) +{ + struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev); + + clk_disable_unprepare(clk_wzrd->axi_clk); + + return 0; +} + +static int __maybe_unused clk_wzrd_resume(struct device *dev) +{ + int ret; + struct clk_wzrd *clk_wzrd = dev_get_drvdata(dev); + + ret = clk_prepare_enable(clk_wzrd->axi_clk); + if (ret) { + dev_err(dev, "unable to enable s_axi_aclk\n"); + return ret; + } + + return 0; +} + +static SIMPLE_DEV_PM_OPS(clk_wzrd_dev_pm_ops, clk_wzrd_suspend, + clk_wzrd_resume); + +static int clk_wzrd_probe(struct platform_device *pdev) +{ + u32 regl, regh, edge, mult, regld, reghd, edged, div; + const char *clkout_name, *clk_name, *clk_mul_name; + struct device_node *np = pdev->dev.of_node; + struct clk_wzrd *clk_wzrd; + unsigned long rate; + int nr_outputs; + int i, j, ret; + + clk_wzrd = devm_kzalloc(&pdev->dev, sizeof(*clk_wzrd), GFP_KERNEL); + if (!clk_wzrd) + return -ENOMEM; + + platform_set_drvdata(pdev, clk_wzrd); + + clk_wzrd->base = devm_platform_ioremap_resource(pdev, 0); + if (IS_ERR(clk_wzrd->base)) + return PTR_ERR(clk_wzrd->base); + + ret = of_property_read_u32(np, "xlnx,nr-outputs", &nr_outputs); + if (ret || nr_outputs > WZRD_NUM_OUTPUTS) + return -EINVAL; + + clk_wzrd->clk_in1 = devm_clk_get(&pdev->dev, "clk_in1"); + if (IS_ERR(clk_wzrd->clk_in1)) + return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->clk_in1), + "clk_in1 not found\n"); + + clk_wzrd->axi_clk = devm_clk_get(&pdev->dev, "s_axi_aclk"); + if (IS_ERR(clk_wzrd->axi_clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(clk_wzrd->axi_clk), + "s_axi_aclk not found\n"); + + ret = clk_prepare_enable(clk_wzrd->axi_clk); + if (ret) { + dev_err(&pdev->dev, "enabling s_axi_aclk failed\n"); + return ret; + } + + rate = clk_get_rate(clk_wzrd->axi_clk); + if (rate > WZRD_ACLK_MAX_FREQ) { + dev_err(&pdev->dev, "s_axi_aclk frequency (%lu) too high\n", + rate); + ret = -EINVAL; + goto err_disable_clk; + } + + if (nr_outputs == 1) { + clkout_name = kasprintf(GFP_KERNEL, "%s_out0", dev_name(&pdev->dev)); + if (!clkout_name) { + ret = -ENOMEM; + goto err_disable_clk; + } + + clk_wzrd->clkout[0] = clk_wzrd_register_divider + (&pdev->dev, clkout_name, + __clk_get_name(clk_wzrd->clk_in1), 0, + clk_wzrd->base, WZRD_CLK_CFG_REG(3), + WZRD_CLKOUT_DIVIDE_SHIFT, + WZRD_CLKOUT_DIVIDE_WIDTH, + CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO, + DIV_ALL, &clkwzrd_lock); + + goto out; + } + + /* register multiplier */ + edge = !!(readl(clk_wzrd->base + WZRD_CLK_CFG_REG(0)) & BIT(8)); + regl = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(1)) & + WZRD_CLKFBOUT_L_MASK) >> WZRD_CLKFBOUT_L_SHIFT; + regh = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(1)) & + WZRD_CLKFBOUT_H_MASK) >> WZRD_CLKFBOUT_H_SHIFT; + mult = regl + regh + edge; + if (!mult) + mult = 1; + mult = mult * WZRD_FRAC_GRADIENT; + + regl = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(51)) & + WZRD_CLKFBOUT_FRAC_EN; + if (regl) { + regl = readl(clk_wzrd->base + WZRD_CLK_CFG_REG(48)) & + WZRD_CLKFBOUT_FRAC_MASK; + mult = mult + regl; + } + + clk_name = kasprintf(GFP_KERNEL, "%s_mul", dev_name(&pdev->dev)); + if (!clk_name) { + ret = -ENOMEM; + goto err_disable_clk; + } + clk_wzrd->clks_internal[wzrd_clk_mul] = clk_register_fixed_factor + (&pdev->dev, clk_name, + __clk_get_name(clk_wzrd->clk_in1), + 0, mult, WZRD_FRAC_GRADIENT); + if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul])) { + dev_err(&pdev->dev, "unable to register fixed-factor clock\n"); + ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul]); + goto err_disable_clk; + } + + /* register div */ + edged = !!(readl(clk_wzrd->base + WZRD_CLK_CFG_REG(20)) & + BIT(10)); + regld = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(21)) & + WZRD_CLKFBOUT_L_MASK) >> WZRD_CLKFBOUT_L_SHIFT; + reghd = (readl(clk_wzrd->base + WZRD_CLK_CFG_REG(21)) & + WZRD_CLKFBOUT_H_MASK) >> WZRD_CLKFBOUT_H_SHIFT; + div = (regld + reghd + edged); + if (!div) + div = 1; + + clk_name = kasprintf(GFP_KERNEL, "%s_mul_div", dev_name(&pdev->dev)); + if (!clk_name) { + ret = -ENOMEM; + goto err_rm_int_clk; + } + + clk_mul_name = __clk_get_name(clk_wzrd->clks_internal[wzrd_clk_mul]); + clk_wzrd->clks_internal[wzrd_clk_mul_div] = + clk_register_fixed_factor(&pdev->dev, clk_name, + clk_mul_name, 0, 1, div); + if (IS_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div])) { + dev_err(&pdev->dev, "unable to register divider clock\n"); + ret = PTR_ERR(clk_wzrd->clks_internal[wzrd_clk_mul_div]); + goto err_rm_int_clk; + } + + /* register div per output */ + for (i = nr_outputs - 1; i >= 0 ; i--) { + clkout_name = kasprintf(GFP_KERNEL, "%s_out%d", dev_name(&pdev->dev), i); + if (!clkout_name) { + ret = -ENOMEM; + goto err_rm_int_clks; + } + + clk_wzrd->clkout[i] = + clk_wzrd_register_divider(&pdev->dev, + clkout_name, clk_name, 0, + clk_wzrd->base, + (WZRD_CLK_CFG_REG(3) + i * 8), + WZRD_CLKOUT_DIVIDE_SHIFT, + WZRD_CLKOUT_DIVIDE_WIDTH, + CLK_DIVIDER_ONE_BASED | + CLK_DIVIDER_ALLOW_ZERO, + DIV_O, &clkwzrd_lock); + + if (IS_ERR(clk_wzrd->clkout[i])) { + for (j = i + 1; j < nr_outputs; j++) + clk_unregister(clk_wzrd->clkout[j]); + dev_err(&pdev->dev, + "unable to register divider clock\n"); + ret = PTR_ERR(clk_wzrd->clkout[i]); + goto err_rm_int_clks; + } + } + + kfree(clk_name); + kfree(clkout_name); + +out: + clk_wzrd->clk_data.clks = clk_wzrd->clkout; + clk_wzrd->clk_data.clk_num = ARRAY_SIZE(clk_wzrd->clkout); + of_clk_add_provider(np, of_clk_src_onecell_get, &clk_wzrd->clk_data); + + return 0; + +err_rm_int_clks: + kfree(clkout_name); + clk_unregister(clk_wzrd->clks_internal[1]); +err_rm_int_clk: + kfree(clk_name); + clk_unregister(clk_wzrd->clks_internal[0]); +err_disable_clk: + clk_disable_unprepare(clk_wzrd->axi_clk); + + return ret; +} + +static int clk_wzrd_remove(struct platform_device *pdev) +{ + int i; + struct clk_wzrd *clk_wzrd = platform_get_drvdata(pdev); + + of_clk_del_provider(pdev->dev.of_node); + + for (i = 0; i < WZRD_NUM_OUTPUTS; i++) + clk_unregister(clk_wzrd->clkout[i]); + for (i = 0; i < wzrd_clk_int_max; i++) + clk_unregister(clk_wzrd->clks_internal[i]); + + clk_disable_unprepare(clk_wzrd->axi_clk); + + return 0; +} + +static const struct of_device_id clk_wzrd_ids[] = { + { .compatible = "xlnx,clk-wizard-1.0" }, + { }, +}; +MODULE_DEVICE_TABLE(of, clk_wzrd_ids); + +static struct platform_driver clk_wzrd_driver = { + .driver = { + .name = "clk-wizard", + .of_match_table = clk_wzrd_ids, + .pm = &clk_wzrd_dev_pm_ops, + }, + .probe = clk_wzrd_probe, + .remove = clk_wzrd_remove, +}; +module_platform_driver(clk_wzrd_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>"); +MODULE_DESCRIPTION("Driver for the Versal Clocking Wizard IP core"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-10-04 11:05 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-30 8:03 [PATCH 0/2] clocking-wizard: Add versal clocking wizard support Shubhrajyoti Datta 2022-09-30 8:03 ` [PATCH 1/2] dt-bindings: clk: Add binding for versal clocking wizard Shubhrajyoti Datta 2022-09-30 11:19 ` Krzysztof Kozlowski 2022-09-30 12:25 ` Rob Herring 2022-09-30 13:00 ` Michal Simek 2022-09-30 21:39 ` Rob Herring 2022-10-03 7:15 ` Michal Simek 2022-10-03 7:23 ` Krzysztof Kozlowski 2022-10-03 7:58 ` Michal Simek 2022-10-03 8:10 ` Krzysztof Kozlowski 2022-10-03 8:16 ` Greg Kroah-Hartman 2022-10-03 9:59 ` Michal Simek 2022-10-03 10:41 ` Michal Simek 2022-10-03 14:50 ` Greg Kroah-Hartman 2022-10-03 10:37 ` Michal Simek 2022-10-03 10:50 ` Krzysztof Kozlowski 2022-10-03 15:27 ` Michal Simek 2022-10-04 11:00 ` Krzysztof Kozlowski 2022-10-04 11:04 ` Michal Simek 2022-09-30 8:04 ` [PATCH 2/2] clocking-wizard: Add versal clocking wizard support Shubhrajyoti Datta
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).