| Message ID | f298715ab197ae72ab9b33caee2a19cc3e8be3f5.1600827061.git.mallikarjunax.reddy@linux.intel.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Intel LGM soc DMA support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
Hello Reddy, I think "Intel" should always be written with a capital "I" (like in the Subject, but except in the binding below) > + compatible: > + oneOf: > + - const: intel,lgm-cdma > + - const: intel,lgm-dma2tx > + - const: intel,lgm-dma1rx > + - const: intel,lgm-dma1tx > + - const: intel,lgm-dma0tx > + - const: intel,lgm-dma3 > + - const: intel,lgm-toe-dma30 > + - const: intel,lgm-toe-dma31 Bindings are normally not per instance. What if next generation chip gets more DMA modules but has no other changes in the HW block? What is wrong with - const: intel,lgm-cdma - const: intel,lgm-hdma and extra attributes to define the rx/tx restriction (or what do it mean?)? From the driver code I saw that "toe" is also just of type "hdma" and no further differences in code are done. Best regards, Thomas
On Tue, 27 Oct 2020 16:03:06 +0800, Amireddy Mallikarjuna reddy wrote: > Add DT bindings YAML schema for DMA controller driver > of Lightning Mountain(LGM) SoC. > > Signed-off-by: Amireddy Mallikarjuna reddy <mallikarjunax.reddy@linux.intel.com> > --- > v1: > - Initial version. > > v2: > - Fix bot errors. > > v3: > - No change. > > v4: > - Address Thomas langer comments > - use node name pattern as dma-controller as in common binding. > - Remove "_" (underscore) in instance name. > - Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes. > > v5: > - Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties. > > v6: > - Add additionalProperties: false > - completely removed 'dma-ports' and 'dma-channels' child nodes. > - Moved channel dt properties to client side dmas. > - Use standard dma-channels and dma-channel-mask properties. > - Documented reset-names > - Add description for dma-cells > > v7: > - modified compatible to oneof > - Reduced number of dma-cells to 3 > - Fine tune the description of some properties. > --- > .../devicetree/bindings/dma/intel,ldma.yaml | 135 +++++++++++++++++++++ > 1 file changed, 135 insertions(+) > create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:17:2: [warning] wrong indentation: expected 2 but found 1 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:21:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:22:4: [warning] wrong indentation: expected 4 but found 3 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:32:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:35:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:37:6: [warning] wrong indentation: expected 4 but found 5 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:42:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:46:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:50:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:53:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:56:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:60:3: [warning] wrong indentation: expected 3 but found 2 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:76:5: [warning] wrong indentation: expected 3 but found 4 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:81:5: [warning] wrong indentation: expected 3 but found 4 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:83:8: [warning] wrong indentation: expected 6 but found 7 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:87:5: [warning] wrong indentation: expected 3 but found 4 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:89:8: [warning] wrong indentation: expected 6 but found 7 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:93:5: [warning] wrong indentation: expected 3 but found 4 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:95:8: [warning] wrong indentation: expected 6 but found 7 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:100:2: [warning] wrong indentation: expected 2 but found 1 (indentation) ./Documentation/devicetree/bindings/dma/intel,ldma.yaml:107:2: [warning] wrong indentation: expected 2 but found 1 (indentation) dtschema/dtc warnings/errors: See https://patchwork.ozlabs.org/patch/1388332 The base for the patch is generally the last rc1. Any dependencies should be noted. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
Hi Thomas, Thanks for the review, my comments inline. On 10/28/2020 3:24 AM, Thomas Langer wrote: > Hello Reddy, > > I think "Intel" should always be written with a capital "I" (like in the Subject, but except in the binding below) OK. > >> + compatible: >> + oneOf: >> + - const: intel,lgm-cdma >> + - const: intel,lgm-dma2tx >> + - const: intel,lgm-dma1rx >> + - const: intel,lgm-dma1tx >> + - const: intel,lgm-dma0tx >> + - const: intel,lgm-dma3 >> + - const: intel,lgm-toe-dma30 >> + - const: intel,lgm-toe-dma31 > Bindings are normally not per instance. > What if next generation chip gets more DMA modules but has no other changes in the HW block? > What is wrong with > - const: intel,lgm-cdma > - const: intel,lgm-hdma > and extra attributes to define the rx/tx restriction (or what do it mean?)? > From the driver code I saw that "toe" is also just of type "hdma" and no further differences in code are done. We had a discussion on the same in the previous patches and Rob Herring said Okay using Different compatibles. below the snippet. +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>> + compatible: >>> + anyOf: >>> + - const: intel,lgm-cdma >>> + - const: intel,lgm-dma2tx >>> + - const: intel,lgm-dma1rx >>> + - const: intel,lgm-dma1tx >>> + - const: intel,lgm-dma0tx >>> + - const: intel,lgm-dma3 >>> + - const: intel,lgm-toe-dma30 >>> + - const: intel,lgm-toe-dma31 >> Please explain why you need so many different compatible strings. > This hw dma has 7 DMA instances. > Some for datapath, some for memcpy and some for TOE. > Some for TX only, some for RX only, and some for TX/RX(memcpy and ToE). > > dma TX/RX type we considered as driver specific data of each instance and > used different compatible strings for each instance. > And also idea is in future if any driver specific data of any particular > instance we can handle. > > Here if dma name and type(tx or rx) will be accepted as devicetree > attributes then we can move .name = "toe_dma31", & .type = DMA_TYPE_MCPY > to devicetree. So that the compatible strings can be limited to two. > intel,lgm-cdma & intel,lgm-hdma . [Rob] Different compatibles are okay if the instances are different and we don't have properties to describe the differences. For some of what you have in this binding, I think it should be part of the consumer cells. ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > Best regards, > Thomas >
> -----Original Message----- > From: Reddy, MallikarjunaX <mallikarjunax.reddy@linux.intel.com> > Sent: Montag, 2. November 2020 15:42 > To: Thomas Langer <tlanger@maxlinear.com>; dmaengine@vger.kernel.org; > vkoul@kernel.org; devicetree@vger.kernel.org; robh+dt@kernel.org > Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy > <andriy.shevchenko@intel.com>; chuanhua.lei@linux.intel.com; Kim, > Cheol Yong <Cheol.Yong.Kim@intel.com>; Wu, Qiming <qi- > ming.wu@intel.com>; malliamireddy009@gmail.com; peter.ujfalusi@ti.com; > Langer, Thomas <thomas.langer@intel.com> > Subject: Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel > LGM SOC > > This email was sent from outside of MaxLinear. > > > Hi Thomas, > Thanks for the review, my comments inline. > > On 10/28/2020 3:24 AM, Thomas Langer wrote: > > Hello Reddy, > > > > I think "Intel" should always be written with a capital "I" (like in > the Subject, but except in the binding below) > OK. > > > >> + compatible: > >> + oneOf: > >> + - const: intel,lgm-cdma > >> + - const: intel,lgm-dma2tx > >> + - const: intel,lgm-dma1rx > >> + - const: intel,lgm-dma1tx > >> + - const: intel,lgm-dma0tx > >> + - const: intel,lgm-dma3 > >> + - const: intel,lgm-toe-dma30 > >> + - const: intel,lgm-toe-dma31 > > Bindings are normally not per instance. > > What if next generation chip gets more DMA modules but has no other > changes in the HW block? > > What is wrong with > > - const: intel,lgm-cdma > > - const: intel,lgm-hdma > > and extra attributes to define the rx/tx restriction (or what do it > mean?)? > > From the driver code I saw that "toe" is also just of type "hdma" > and no further differences in code are done. > We had a discussion on the same in the previous patches and Rob > Herring > said Okay using Different compatibles. > below the snippet. > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > + > >>> + compatible: > >>> + anyOf: > >>> + - const: intel,lgm-cdma > >>> + - const: intel,lgm-dma2tx > >>> + - const: intel,lgm-dma1rx > >>> + - const: intel,lgm-dma1tx > >>> + - const: intel,lgm-dma0tx > >>> + - const: intel,lgm-dma3 > >>> + - const: intel,lgm-toe-dma30 > >>> + - const: intel,lgm-toe-dma31 > >> Please explain why you need so many different compatible strings. > > This hw dma has 7 DMA instances. > > Some for datapath, some for memcpy and some for TOE. > > Some for TX only, some for RX only, and some for TX/RX(memcpy and > ToE). > > > > dma TX/RX type we considered as driver specific data of each > instance and > > used different compatible strings for each instance. > > And also idea is in future if any driver specific data of any > particular > > instance we can handle. > > > > Here if dma name and type(tx or rx) will be accepted as devicetree > > attributes then we can move .name = "toe_dma31", & .type = > DMA_TYPE_MCPY > > to devicetree. So that the compatible strings can be limited to > two. > > intel,lgm-cdma & intel,lgm-hdma . > > [Rob] > Different compatibles are okay if the instances are different and we > don't have properties to describe the differences. Okay, but then explain what the differences are, that cannot be described by other properties/attributes. In the driver code I cannot see anything, except the "name". But for printouts in driver, "drv_dbg" or similar will just use the node path for the instance. > > For some of what you have in this binding, I think it should be part > of > the consumer cells. > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > ++ > > > > Best regards, > > Thomas > >
Hi Thomas, On 11/11/2020 1:39 AM, Thomas Langer wrote: > >> -----Original Message----- >> From: Reddy, MallikarjunaX <mallikarjunax.reddy@linux.intel.com> >> Sent: Montag, 2. November 2020 15:42 >> To: Thomas Langer <tlanger@maxlinear.com>; dmaengine@vger.kernel.org; >> vkoul@kernel.org; devicetree@vger.kernel.org; robh+dt@kernel.org >> Cc: linux-kernel@vger.kernel.org; Shevchenko, Andriy >> <andriy.shevchenko@intel.com>; chuanhua.lei@linux.intel.com; Kim, >> Cheol Yong <Cheol.Yong.Kim@intel.com>; Wu, Qiming <qi- >> ming.wu@intel.com>; malliamireddy009@gmail.com; peter.ujfalusi@ti.com; >> Langer, Thomas <thomas.langer@intel.com> >> Subject: Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel >> LGM SOC >> >> This email was sent from outside of MaxLinear. >> >> >> Hi Thomas, >> Thanks for the review, my comments inline. >> >> On 10/28/2020 3:24 AM, Thomas Langer wrote: >>> Hello Reddy, >>> >>> I think "Intel" should always be written with a capital "I" (like in >> the Subject, but except in the binding below) >> OK. >>>> + compatible: >>>> + oneOf: >>>> + - const: intel,lgm-cdma >>>> + - const: intel,lgm-dma2tx >>>> + - const: intel,lgm-dma1rx >>>> + - const: intel,lgm-dma1tx >>>> + - const: intel,lgm-dma0tx >>>> + - const: intel,lgm-dma3 >>>> + - const: intel,lgm-toe-dma30 >>>> + - const: intel,lgm-toe-dma31 >>> Bindings are normally not per instance. >>> What if next generation chip gets more DMA modules but has no other >> changes in the HW block? >>> What is wrong with >>> - const: intel,lgm-cdma >>> - const: intel,lgm-hdma >>> and extra attributes to define the rx/tx restriction (or what do it >> mean?)? >>> From the driver code I saw that "toe" is also just of type "hdma" >> and no further differences in code are done. >> We had a discussion on the same in the previous patches and Rob >> Herring >> said Okay using Different compatibles. >> below the snippet. >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> + >> >>> + compatible: >> >>> + anyOf: >> >>> + - const: intel,lgm-cdma >> >>> + - const: intel,lgm-dma2tx >> >>> + - const: intel,lgm-dma1rx >> >>> + - const: intel,lgm-dma1tx >> >>> + - const: intel,lgm-dma0tx >> >>> + - const: intel,lgm-dma3 >> >>> + - const: intel,lgm-toe-dma30 >> >>> + - const: intel,lgm-toe-dma31 >> >> Please explain why you need so many different compatible strings. >> > This hw dma has 7 DMA instances. >> > Some for datapath, some for memcpy and some for TOE. >> > Some for TX only, some for RX only, and some for TX/RX(memcpy and >> ToE). >> > >> > dma TX/RX type we considered as driver specific data of each >> instance and >> > used different compatible strings for each instance. >> > And also idea is in future if any driver specific data of any >> particular >> > instance we can handle. >> > >> > Here if dma name and type(tx or rx) will be accepted as devicetree >> > attributes then we can move .name = "toe_dma31", & .type = >> DMA_TYPE_MCPY >> > to devicetree. So that the compatible strings can be limited to >> two. >> > intel,lgm-cdma & intel,lgm-hdma . >> >> [Rob] >> Different compatibles are okay if the instances are different and we >> don't have properties to describe the differences. > Okay, but then explain what the differences are, that cannot be described > by other properties/attributes. In the driver code I cannot see anything, > except the "name". But for printouts in driver, "drv_dbg" or similar will > just use the node path for the instance. On patch4 series we had the same discussion. i will brief it here again. This hw dma has 7 DMA instances, and each Some for TX only, some for RX only, and some for TX/RX. dma TX/RX type we considered as driver specific data and it cant be used as dt property as per the previous reviewers. So i moved it to driver specific data. If type(tx or rx) will be accepted as devicetree attributes then we can move it to devicetree. So as you said we can limit compatible strings can be limited to two. intel,lgm-cdma & intel,lgm-hdma . One more advantage i see with this model is in future if any driver specific data of any particular instance we can handle easily. > >> For some of what you have in this binding, I think it should be part >> of >> the consumer cells. >> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> ++ >>> Best regards, >>> Thomas >>>
diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml new file mode 100644 index 000000000000..d38143932b05 --- /dev/null +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml @@ -0,0 +1,135 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/dma/intel,ldma.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Lightning Mountain centralized low speed DMA and high speed DMA controllers. + +maintainers: + - chuanhua.lei@intel.com + - mallikarjunax.reddy@intel.com + +allOf: + - $ref: "dma-controller.yaml#" + +properties: + $nodename: + pattern: "^dma-controller(@.*)?$" + + compatible: + oneOf: + - const: intel,lgm-cdma + - const: intel,lgm-dma2tx + - const: intel,lgm-dma1rx + - const: intel,lgm-dma1tx + - const: intel,lgm-dma0tx + - const: intel,lgm-dma3 + - const: intel,lgm-toe-dma30 + - const: intel,lgm-toe-dma31 + + reg: + maxItems: 1 + + "#dma-cells": + const: 3 + description: + The first cell is the peripheral's DMA request line. + The second cell is the peripheral's (port) number corresponding to the channel. + The third cell is the burst length of the channel. + + dma-channels: + minimum: 1 + maximum: 16 + + dma-channel-mask: + items: + minItems: 1 + + clocks: + maxItems: 1 + + resets: + maxItems: 1 + + reset-names: + items: + - const: ctrl + + interrupts: + maxItems: 1 + + intel,dma-poll-cnt: + $ref: /schemas/types.yaml#definitions/uint32 + description: + DMA descriptor polling counter is used to control the poling mechanism + for the descriptor fetching for all channels. + + intel,dma-byte-en: + type: boolean + description: + DMA byte enable is only valid for DMA write(RX). + Byte enable(1) means DMA write will be based on the number of dwords + instead of the whole burst. + + intel,dma-drb: + type: boolean + description: + DMA descriptor read back to make sure data and desc synchronization. + + intel,dma-desc-in-sram: + type: boolean + description: + DMA descritpors in SRAM or not. Some old controllers descriptors + can be in DRAM or SRAM. The new ones are all in SRAM. + + intel,dma-orrc: + $ref: /schemas/types.yaml#definitions/uint32 + description: + DMA outstanding read counter value determine the number of + ORR-Outstanding Read Request. The maximum value is 16. + + intel,dma-dburst-wr: + type: boolean + description: + Enable RX dynamic burst write. When it is enabled, the DMA does RX dynamic burst; + if it is disabled, the DMA RX will still support programmable fixed burst size of 2,4,8,16. + It only applies to RX DMA and memcopy DMA. + +required: + - compatible + - reg + - '#dma-cells' + +additionalProperties: false + +examples: + - | + dma0: dma-controller@e0e00000 { + compatible = "intel,lgm-cdma"; + reg = <0xe0e00000 0x1000>; + #dma-cells = <3>; + dma-channels = <16>; + dma-channel-mask = <0xFFFF>; + interrupt-parent = <&ioapic1>; + interrupts = <82 1>; + resets = <&rcu0 0x30 0>; + reset-names = "ctrl"; + clocks = <&cgu0 80>; + intel,dma-poll-cnt = <4>; + intel,dma-byte-en; + intel,dma-drb; + }; + - | + dma3: dma-controller@ec800000 { + compatible = "intel,lgm-dma3"; + reg = <0xec800000 0x1000>; + clocks = <&cgu0 71>; + resets = <&rcu0 0x10 9>; + #dma-cells = <3>; + intel,dma-poll-cnt = <16>; + intel,dma-desc-in-sram; + intel,dma-orrc = <16>; + intel,dma-byte-en; + intel,dma-dburst-wr; + };
Add DT bindings YAML schema for DMA controller driver of Lightning Mountain(LGM) SoC. Signed-off-by: Amireddy Mallikarjuna reddy <mallikarjunax.reddy@linux.intel.com> --- v1: - Initial version. v2: - Fix bot errors. v3: - No change. v4: - Address Thomas langer comments - use node name pattern as dma-controller as in common binding. - Remove "_" (underscore) in instance name. - Remove "port-" and "chan-" in attribute name for both 'dma-ports' & 'dma-channels' child nodes. v5: - Moved some of the attributes in 'dma-ports' & 'dma-channels' child nodes to dma client/consumer side as cells in 'dmas' properties. v6: - Add additionalProperties: false - completely removed 'dma-ports' and 'dma-channels' child nodes. - Moved channel dt properties to client side dmas. - Use standard dma-channels and dma-channel-mask properties. - Documented reset-names - Add description for dma-cells v7: - modified compatible to oneof - Reduced number of dma-cells to 3 - Fine tune the description of some properties. --- .../devicetree/bindings/dma/intel,ldma.yaml | 135 +++++++++++++++++++++ 1 file changed, 135 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml