| Message ID | 20230508131515.19403-1-bbhushan2@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2,v7] dt-bindings: watchdog: marvell GTI system watchdog driver | expand |
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, May 9, 2023 6:36 PM > To: Bharat Bhushan <bbhushan2@marvell.com>; wim@linux-watchdog.org; > linux@roeck-us.net; robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; > linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com> > Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system > watchdog driver > > On 09/05/2023 11:01, Bharat Bhushan wrote: > > > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> Sent: Tuesday, May 9, 2023 1:38 PM > >> To: Bharat Bhushan <bbhushan2@marvell.com>; wim@linux-watchdog.org; > >> linux@roeck-us.net; robh+dt@kernel.org; > >> krzysztof.kozlowski+dt@linaro.org; > >> linux-watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux- > >> kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com> > >> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell > >> GTI system watchdog driver > >> > >> On 09/05/2023 09:26, Bharat Bhushan wrote: > >> > >> > >>>>> +properties: > >>>>> + compatible: > >>>>> + oneOf: > >>>>> + - const: marvell,octeontx2-wdt > >>>> > >>>> Why is this alone? Judging by the enum below, octeontx2 is not specific. > >>>> > >>>>> + - items: > >>>>> + - enum: > >>>>> + - marvell,octeontx2-95xx-wdt > >>>>> + - marvell,octeontx2-96xx-wdt > >>>>> + - marvell,octeontx2-98xx-wdt > >>>> > >>>> We don't allow wildcards in general > >>> > >>> Marvell have octeontx2 series of processor which have watchdog timer. > >>> In 95xx,98xx,96xx are the processors in octeontx2 series of > >>> processor. So > >> octeontx2-95xx is on soc, octeontx2-96xx is another and so on. > >> > >> No, 95xx is not a processor. Otherwise please point me to exact > >> product datasheet. Hint: I checked it. > > > > Looks like 95xx data sheet is not public, will remove in that case. > > We can talk about 96xx. Can you point me to the SoC named exactly like this? > Hint: I checked it. To recap what Bharat mentioned before along with references to individual processors. OcteonTx2 is a family of processors https://www.marvell.com/products/data-processing-units.html Please check for "OCTEON TX2 DPUs" CN96xx and CN98xx are two silicon variants in this family. https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn96xx-cn98xx-product-brief-2020-02.pdf And CNF95xx is another silicon variant in the same family. https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-fusion-cnf95xx-product-brief.pdf Since the HW block is same in all the variants of silicons in this family, we would like to use a generic string instead of different compatible string for each one. ie - const: marvell,octeontx2-wdt Hope this is okay. Same with CN10K or Octeon10 family of silicons. https://www.marvell.com/products/data-processing-units.html Please check for "OCTEON 10" CN103xx and CN106xx are two silicons in this family. https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-octeon-10-dpu-platform-product-brief.pdf Same with CNF105xx https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-octeon-10-fusion-cnf105xx-product-brief.pdf For this family we would like to use - const: marvell,cn10k-wdt as the compatible string, as it represents all silicon variants in this family. Thanks, Sunil.
On 16/05/2023 12:06, Sunil Kovvuri Goutham wrote: >>>>> Marvell have octeontx2 series of processor which have watchdog timer. >>>>> In 95xx,98xx,96xx are the processors in octeontx2 series of >>>>> processor. So >>>> octeontx2-95xx is on soc, octeontx2-96xx is another and so on. >>>> >>>> No, 95xx is not a processor. Otherwise please point me to exact >>>> product datasheet. Hint: I checked it. >>> >>> Looks like 95xx data sheet is not public, will remove in that case. >> >> We can talk about 96xx. Can you point me to the SoC named exactly like this? >> Hint: I checked it. > > To recap what Bharat mentioned before along with references to individual processors. > OcteonTx2 is a family of processors > https://www.marvell.com/products/data-processing-units.html > Please check for "OCTEON TX2 DPUs" > CN96xx and CN98xx are two silicon variants in this family. > https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn96xx-cn98xx-product-brief-2020-02.pdf This is a product brief which further suggests CN96xx is a family (or sub-family). "xx" is pretty often used as family, not as product. Otherwise how one product CN92XX can come with 12-18 cores *in the same time*? https://www.marvell.com/company/newsroom/marvell-announces-octeon-tx2-family-of-multi-core-infrastructure-processors.html "Marvell’s CN91xx, CN92xx, CN96xx, and CN98xx processor families include:" https://www.marvell.com/products/data-processing-units.html > And CNF95xx is another silicon variant in the same family. > https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-infrastructure-processors-octeon-fusion-cnf95xx-product-brief.pdf Again, unspecific product brief. Your other briefs specify them clearer, e.g. CN9130, CN9131 > > Since the HW block is same in all the variants of silicons in this family, we would like to use a > generic string instead of different compatible string for each one. ie > - const: marvell,octeontx2-wdt > Hope this is okay. https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42 > > Same with CN10K or Octeon10 family of silicons. > https://www.marvell.com/products/data-processing-units.html > Please check for "OCTEON 10" > > CN103xx and CN106xx are two silicons in this family. Are they? "Up to 8" cores, so how this can be one specific silicon? One customer buys CN10300 with 8 cores, second buys exactly the same CN10300 and has 4 cores? You are mixing families and specific devices. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Tuesday, May 16, 2023 3:55 PM > To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Bharat Bhushan > <bbhushan2@marvell.com>; wim@linux-watchdog.org; linux@roeck-us.net; > robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux- > watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system > watchdog driver > > On 16/05/2023 12:06, Sunil Kovvuri Goutham wrote: > > > >>>>> Marvell have octeontx2 series of processor which have watchdog timer. > >>>>> In 95xx,98xx,96xx are the processors in octeontx2 series of > >>>>> processor. So > >>>> octeontx2-95xx is on soc, octeontx2-96xx is another and so on. > >>>> > >>>> No, 95xx is not a processor. Otherwise please point me to exact > >>>> product datasheet. Hint: I checked it. > >>> > >>> Looks like 95xx data sheet is not public, will remove in that case. > >> > >> We can talk about 96xx. Can you point me to the SoC named exactly like this? > >> Hint: I checked it. > > > > To recap what Bharat mentioned before along with references to individual > processors. > > OcteonTx2 is a family of processors > > https://www.marvell.com/products/data-processing-units.html > > Please check for "OCTEON TX2 DPUs" > > CN96xx and CN98xx are two silicon variants in this family. > > https://www.marvell.com/content/dam/marvell/en/public-collateral/embed > > ded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn9 > > 6xx-cn98xx-product-brief-2020-02.pdf > > This is a product brief which further suggests CN96xx is a family (or sub-family). > > "xx" is pretty often used as family, not as product. Otherwise how one product > CN92XX can come with 12-18 cores *in the same time*? "xx" here suggests skews, some 92xx may have 18 cores and some may have few cores fused out resulting in 12cores. > > https://www.marvell.com/company/newsroom/marvell-announces-octeon- > tx2-family-of-multi-core-infrastructure-processors.html > "Marvell’s CN91xx, CN92xx, CN96xx, and CN98xx processor families include:" > > https://www.marvell.com/products/data-processing-units.html > > > > And CNF95xx is another silicon variant in the same family. > > https://www.marvell.com/content/dam/marvell/en/public-collateral/embed > > ded-processors/marvell-infrastructure-processors-octeon-fusion-cnf95xx > > -product-brief.pdf > > Again, unspecific product brief. Your other briefs specify them clearer, e.g. > CN9130, CN9131 Not sure what you are looking for in the product brief, I pointed to the link just to show proof of 95xx being an official product. > > > > > Since the HW block is same in all the variants of silicons in this > > family, we would like to use a generic string instead of different > > compatible string for each one. ie > > - const: marvell,octeontx2-wdt > > Hope this is okay. > > https://urldefense.proofpoint.com/v2/url?u=https- > 3A__elixir.bootlin.com_linux_v6.1- > 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst- > 23L42&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTz > HuhwawxR1P9_tMCN2FODU4&m=GGfz5QI8ldHRqao5OsrfuHZQso5LLNBeBxCZr > Ai7Zow- > qoKl_S1Yw90OWnSZ3FFx&s=kM0VFY1b15BYvp2EUapQjZ6Eb96aZ_yAE76EKCmA > aEQ&e= > > > > > Same with CN10K or Octeon10 family of silicons. > > https://www.marvell.com/products/data-processing-units.html > > Please check for "OCTEON 10" > > > > CN103xx and CN106xx are two silicons in this family. > > Are they? "Up to 8" cores, so how this can be one specific silicon? One customer > buys CN10300 with 8 cores, second buys exactly the same CN10300 and has 4 > cores? > > You are mixing families and specific devices. I was making it simple to understand. OcteonTx2 and Octeon10 (CN10K) are two generations of Octeon processors from Marvell. Within each generation there are multiple silicon variants. Again for each variant there are multiple skews. Since the watchdog hardware block functionality is same across all of above generations / variants / families / skews, is it okay to use below compatible strings ? - const: marvell,octeontx2-wdt - const: marvell,cn10k-wdt Also this is the same naming we have been using in other drivers as well. drivers/net/ethernet/marvell/octeontx2 drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c drivers/perf/marvell_cn10k_ddr_pmu.c static const struct of_device_id cn10k_ddr_pmu_of_match[] = { { .compatible = "marvell,cn10k-ddr-pmu", }, { }, }; Thanks, Sunil.
On 16/05/2023 13:24, Sunil Kovvuri Goutham wrote: > > >> -----Original Message----- >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> Sent: Tuesday, May 16, 2023 3:55 PM >> To: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Bharat Bhushan >> <bbhushan2@marvell.com>; wim@linux-watchdog.org; linux@roeck-us.net; >> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux- >> watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux- >> kernel@vger.kernel.org >> Subject: Re: [EXT] Re: [PATCH 1/2 v7] dt-bindings: watchdog: marvell GTI system >> watchdog driver >> >> On 16/05/2023 12:06, Sunil Kovvuri Goutham wrote: >> >> >>>>>>> Marvell have octeontx2 series of processor which have watchdog timer. >>>>>>> In 95xx,98xx,96xx are the processors in octeontx2 series of >>>>>>> processor. So >>>>>> octeontx2-95xx is on soc, octeontx2-96xx is another and so on. >>>>>> >>>>>> No, 95xx is not a processor. Otherwise please point me to exact >>>>>> product datasheet. Hint: I checked it. >>>>> >>>>> Looks like 95xx data sheet is not public, will remove in that case. >>>> >>>> We can talk about 96xx. Can you point me to the SoC named exactly like this? >>>> Hint: I checked it. >>> >>> To recap what Bharat mentioned before along with references to individual >> processors. >>> OcteonTx2 is a family of processors >>> https://www.marvell.com/products/data-processing-units.html >>> Please check for "OCTEON TX2 DPUs" >>> CN96xx and CN98xx are two silicon variants in this family. >>> https://www.marvell.com/content/dam/marvell/en/public-collateral/embed >>> ded-processors/marvell-infrastructure-processors-octeon-tx2-cn92xx-cn9 >>> 6xx-cn98xx-product-brief-2020-02.pdf >> >> This is a product brief which further suggests CN96xx is a family (or sub-family). >> >> "xx" is pretty often used as family, not as product. Otherwise how one product >> CN92XX can come with 12-18 cores *in the same time*? > > "xx" here suggests skews, some 92xx may have 18 cores and some may have > few cores fused out resulting in 12cores. Is the DTSI for them the same? IOW, 12-core and 18-core SoCs have exactly the same DTSI with all properties being the same, valid and not customized by firmware per skew? If we talk about ARM architecture, DTS expects CPUs there, so I really wonder how do you write one DTS which has in the same time 12 and 18 enabled CPUs. Remember - the same time and not changed by firmware. > >> >>> >>> Since the HW block is same in all the variants of silicons in this >>> family, we would like to use a generic string instead of different >>> compatible string for each one. ie >>> - const: marvell,octeontx2-wdt >>> Hope this is okay. >> >> https://urldefense.proofpoint.com/v2/url?u=https- >> 3A__elixir.bootlin.com_linux_v6.1- >> 2Drc1_source_Documentation_devicetree_bindings_writing-2Dbindings.rst- >> 23L42&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=q3VKxXQKiboRw_F01ggTz >> HuhwawxR1P9_tMCN2FODU4&m=GGfz5QI8ldHRqao5OsrfuHZQso5LLNBeBxCZr >> Ai7Zow- >> qoKl_S1Yw90OWnSZ3FFx&s=kM0VFY1b15BYvp2EUapQjZ6Eb96aZ_yAE76EKCmA >> aEQ&e= >> >>> >>> Same with CN10K or Octeon10 family of silicons. >>> https://www.marvell.com/products/data-processing-units.html >>> Please check for "OCTEON 10" >>> >>> CN103xx and CN106xx are two silicons in this family. >> >> Are they? "Up to 8" cores, so how this can be one specific silicon? One customer >> buys CN10300 with 8 cores, second buys exactly the same CN10300 and has 4 >> cores? >> >> You are mixing families and specific devices. > > I was making it simple to understand. > > OcteonTx2 and Octeon10 (CN10K) are two generations of Octeon processors from Marvell. I know. I don't think we talk about the same thing... > Within each generation there are multiple silicon variants. > Again for each variant there are multiple skews. > > Since the watchdog hardware block functionality is same across all of above > generations / variants / families / skews, is it okay to use below compatible strings ? You got the link which explains it. We had this discussions for thousands times. Just a few references from bookmarks: https://lore.kernel.org/all/20220822181701.GA89665-robh@kernel.org/ https://lore.kernel.org/all/78651e07-6b3e-4243-8e1f-fcd1dfb3ffe1@www.fastmail.com/ https://lore.kernel.org/all/288f56ba9cfad46354203b7698babe91@walle.cc/ https://lore.kernel.org/all/106e443a-e765-51fe-b556-e4e7e2aa771c@linaro.org/ Explain me how is this different. Please do not repeat the same arguments as everywhere else, because we covered them. > > - const: marvell,octeontx2-wdt > - const: marvell,cn10k-wdt > > Also this is the same naming we have been using in other drivers as well. > drivers/net/ethernet/marvell/octeontx2 > drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c Ah, the argument "others did it" or "in the past we did". If the approach is buggy, does it mean you should duplicate the same buggy approach to new bindings? Best regards, Krzysztof
On 16/05/2023 13:24, Sunil Kovvuri Goutham wrote: > > Also this is the same naming we have been using in other drivers as well. > drivers/net/ethernet/marvell/octeontx2 > drivers/net/ethernet/marvell/octeontx2/af/rvu_cn10k.c > > drivers/perf/marvell_cn10k_ddr_pmu.c > static const struct of_device_id cn10k_ddr_pmu_of_match[] = { > { .compatible = "marvell,cn10k-ddr-pmu", }, > { }, BTW, I don't understand this part. We do not talk about fallback compatible, so what does it prove? Of course driver will look like that, but we established it some time ago, didn't we? Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml new file mode 100644 index 000000000000..72951b10f1f3 --- /dev/null +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/watchdog/marvell,octeontx2-wdt.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Marvell Global Timer (GTI) system watchdog + +allOf: + - $ref: watchdog.yaml# + +maintainers: + - Bharat Bhushan <bbhushan2@marvell.com> + +properties: + compatible: + oneOf: + - const: marvell,octeontx2-wdt + - items: + - enum: + - marvell,octeontx2-95xx-wdt + - marvell,octeontx2-96xx-wdt + - marvell,octeontx2-98xx-wdt + - const: marvell,octeontx2-wdt + - const: marvell,cn10k-wdt + - items: + - enum: + - marvell,cn10kx-wdt + - marvell,cnf10kx-wdt + - const: marvell,cn10k-wdt + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + minItems: 1 + + clock-names: + minItems: 1 + + marvell,wdt-timer-index: + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 63 + description: + An SoC have many timers (up to 64), firmware can reserve one or more timer + for some other use case and configures one of the global timer as watchdog + timer. Firmware will update this field with the timer number configured + as watchdog timer. + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + soc { + #address-cells = <2>; + #size-cells = <2>; + + watchdog@802000040000 { + compatible = "marvell,octeontx2-wdt"; + reg = <0x00008020 0x00040000 0x00000000 0x00020000>; + interrupts = <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>; + clocks = <&sclk>; + clock-names = "ref_clk"; + marvell,wdt-timer-index = <63>; + }; + }; + +...
Add binding documentation for the Marvell GTI system watchdog driver. Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com> --- v7: - Corrected compatible to have soc name - Converted marvell,wdt-timer-index to optional .../watchdog/marvell,octeontx2-wdt.yaml | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml