From: "Reddy, MallikarjunaX" <mallikarjunax.reddy@linux.intel.com>
To: Rob Herring <robh@kernel.org>
Cc: dmaengine@vger.kernel.org, vkoul@kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
andriy.shevchenko@intel.com, cheol.yong.kim@intel.com,
qi-ming.wu@intel.com, chuanhua.lei@linux.intel.com,
malliamireddy009@gmail.com
Subject: Re: [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC
Date: Tue, 18 Aug 2020 15:00:44 +0800 [thread overview]
Message-ID: <7cdc0587-8b4f-4360-a303-1541c9ad57b2@linux.intel.com> (raw)
In-Reply-To: <20200814203222.GA2674896@bogus>
Hi Rob,
Thanks for your valuable comments. Please see my comments inline..
On 8/15/2020 4:32 AM, Rob Herring wrote:
> On Fri, Aug 14, 2020 at 01:26:09PM +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.
>> ---
>> .../devicetree/bindings/dma/intel,ldma.yaml | 319 +++++++++++++++++++++
>> 1 file changed, 319 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/dma/intel,ldma.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/dma/intel,ldma.yaml b/Documentation/devicetree/bindings/dma/intel,ldma.yaml
>> new file mode 100644
>> index 000000000000..9beaf191a6de
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml
>> @@ -0,0 +1,319 @@
>> +# 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(@.*)?$"
>> +
>> + "#dma-cells":
>> + const: 1
> Example says 3.
OK, i will fix it.
>
>> +
>> + 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
> 'anyOf' doesn't make sense here. This should be a single 'enum'.
ok. I will update it.
>
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + maxItems: 1
>> +
>> + resets:
>> + maxItems: 1
>> +
>> + interrupts:
>> + maxItems: 1
>> +
>> + intel,dma-poll-cnt:
>> + $ref: /schemas/types.yaml#definitions/uint32
>> + description:
>> + DMA descriptor polling counter. It may need fine tune according
>> + to the system application scenario.
>> +
>> + 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-burst:
>> + $ref: /schemas/types.yaml#definitions/uint32
>> + description:
>> + Specifiy the DMA burst size(in dwords), the valid value will be 8, 16, 32.
>> + Default is 16 for data path dma, 32 is for memcopy DMA.
>> +
>> + intel,dma-polling-cnt:
>> + $ref: /schemas/types.yaml#definitions/uint32
>> + description:
>> + DMA descriptor polling counter. It may need fine tune according to
>> + the system application scenario.
>> +
>> + 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. The maximum value is 16, and it may
>> + need fine tune according to the system application scenarios.
>> +
>> + intel,dma-dburst-wr:
>> + type: boolean
>> + description:
>> + Enable RX dynamic burst write. It only applies to RX DMA and memcopy DMA.
>> +
>> +
>> + dma-ports:
>> + type: object
>> + description:
>> + This sub-node must contain a sub-node for each DMA port.
>> + properties:
>> + '#address-cells':
>> + const: 1
>> + '#size-cells':
>> + const: 0
>> +
>> + patternProperties:
>> + "^dma-ports@[0-9]+$":
>> + type: object
>> +
>> + properties:
>> + reg:
>> + items:
>> + - enum: [0, 1, 2, 3, 4, 5]
>> + description:
>> + Which port this node refers to.
>> +
>> + intel,name:
>> + $ref: /schemas/types.yaml#definitions/string-array
>> + description:
>> + Port name of each DMA port.
> No other DMA controller needs this, why do you?
Answered below. (*ABC)
>
>> +
>> + intel,chans:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description:
>> + The channels included on this port. Format is channel start
>> + number and how many channels on this port.
> Why does this need to be in DT? This all seems like it can be in the dma
> cells for each client.
(*ABC)
Yes. We need this.
for dma0(lgm-cdma) old SOC supports 16 channels and the new SOC supports
22 channels. and the logical channel mapping for the peripherals also
differ b/w old and new SOCs.
Because of this hardware limitation we are trying to configure the total
channels and port-channel mapping dynamically from device tree.
based on port name we are trying to configure the default values for
different peripherals(ports).
Example: burst length is not same for all ports, so using port name to
do default configurations.
>
>> +
>> + required:
>> + - reg
>> + - intel,name
>> + - intel,chans
>> +
>> +
>> + ldma-channels:
>> + type: object
>> + description:
>> + This sub-node must contain a sub-node for each DMA channel.
>> + properties:
>> + '#address-cells':
>> + const: 1
>> + '#size-cells':
>> + const: 0
>> +
>> + patternProperties:
>> + "^ldma-channels@[0-15]+$":
>> + type: object
>> +
>> + properties:
>> + reg:
>> + items:
>> + - enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]
>> + description:
>> + Which channel this node refers to.
>> +
>> + intel,desc_num:
>> + $ref: /schemas/types.yaml#/definitions/uint32
>> + description:
>> + Per channel maximum descriptor number. The max value is 255.
>> +
>> + intel,hdr-mode:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description:
>> + The first parameter is header mode size, the second
>> + parameter is checksum enable or disable. If enabled,
>> + header mode size is ignored. If disabled, header mode
>> + size must be provided.
>> +
>> + intel,hw-desc:
>> + $ref: /schemas/types.yaml#/definitions/uint32-array
>> + description:
>> + Per channel dma hardware descriptor configuration.
>> + The first parameter is descriptor physical address and the
>> + second parameter hardware descriptor number.
> Again, this all seems like per client information for dma cells.
Ok, if we move all these attributes to 'dmas' then 'dma-channels'
child node is not needed in dtsi.
#dma-cells number i am already using 7. If we move all these attributes
to 'dmas' then integer cells will increase.
Is there any limitation in using a number of integer cells & as
determined by the #dma-cells property?
>
>> +
>> + required:
>> + - reg
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - '#dma-cells'
> Add:
>
> additionalProperties: false
Sure i will update it in the next patch..
>
>> +
>> +examples:
>> + - |
>> + dma0: dma-controller@e0e00000 {
>> + compatible = "intel,lgm-cdma";
>> + reg = <0xe0e00000 0x1000>;
>> + #dma-cells = <3>;
>> + interrupt-parent = <&ioapic1>;
>> + interrupts = <82 1>;
>> + resets = <&rcu0 0x30 0>;
>> + reset-names = "ctrl";
> Not documented.
ok. will Document in the next patch.
>
>> + clocks = <&cgu0 80>;
>> + intel,dma-poll-cnt = <4>;
>> + intel,dma-byte-en;
>> + intel,dma-drb;
>> + dma-ports {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + dma-ports@0 {
>> + reg = <0>;
>> + intel,name = "SPI0";
>> + intel,chans = <0 2>;
>> + };
>> + dma-ports@1 {
>> + reg = <1>;
>> + intel,name = "SPI1";
>> + intel,chans = <2 2>;
>> + };
>> + dma-ports@2 {
>> + reg = <2>;
>> + intel,name = "SPI2";
>> + intel,chans = <4 2>;
>> + };
>> + dma-ports@3 {
>> + reg = <3>;
>> + intel,name = "SPI3";
>> + intel,chans = <6 2>;
>> + };
>> + dma-ports@4 {
>> + reg = <4>;
>> + intel,name = "HSNAND";
>> + intel,chans = <8 2>;
>> + };
>> + dma-ports@5 {
>> + reg = <5>;
>> + intel,name = "PCM";
>> + intel,chans = <10 6>;
>> + };
>> + };
>> + ldma-channels {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + ldma-channels@0 {
>> + reg = <0>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@1 {
>> + reg = <1>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@2 {
>> + reg = <2>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@3 {
>> + reg = <3>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@4 {
>> + reg = <4>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@5 {
>> + reg = <5>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@6 {
>> + reg = <6>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@7 {
>> + reg = <7>;
>> + intel,desc_num = <1>;
>> + };
>> + ldma-channels@8 {
>> + reg = <8>;
>> + };
>> + ldma-channels@9 {
>> + reg = <9>;
>> + };
>> + ldma-channels@10 {
>> + reg = <10>;
>> + };
>> + ldma-channels@11 {
>> + reg = <11>;
>> + };
>> + ldma-channels@12 {
>> + reg = <12>;
>> + };
>> + ldma-channels@13 {
>> + reg = <13>;
>> + };
>> + ldma-channels@14 {
>> + reg = <14>;
>> + };
>> + ldma-channels@15 {
>> + reg = <15>;
>> + };
>> + };
>> + };
>> + - |
>> + dma3: dma-controller@ec800000 {
>> + compatible = "intel,lgm-dma3";
>> + reg = <0xec800000 0x1000>;
>> + clocks = <&cgu0 71>;
>> + resets = <&rcu0 0x10 9>;
>> + #dma-cells = <7>;
>> + intel,dma-burst = <32>;
>> + intel,dma-polling-cnt = <16>;
>> + intel,dma-desc-in-sram;
>> + intel,dma-orrc = <16>;
>> + intel,dma-byte-en;
>> + intel,dma-dburst-wr;
>> + ldma-channels {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + ldma-channels@12 {
>> + reg = <12>;
>> + intel,hdr-mode = <128 0>;
>> + intel,hw-desc = <0x20000000 8>;
>> + };
>> + ldma-channels@13 {
>> + reg = <13>;
>> + intel,hdr-mode = <128 0>;
>> + };
>> + };
>> + };
>> --
>> 2.11.0
>>
next prev parent reply other threads:[~2020-08-18 7:01 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-14 5:26 [PATCH v5 0/2] Add Intel LGM soc DMA support Amireddy Mallikarjuna reddy
2020-08-14 5:26 ` [PATCH v5 1/2] dt-bindings: dma: Add bindings for intel LGM SOC Amireddy Mallikarjuna reddy
2020-08-14 20:32 ` Rob Herring
2020-08-18 7:00 ` Reddy, MallikarjunaX [this message]
2020-08-25 11:21 ` Vinod Koul
2020-08-27 9:54 ` Reddy, MallikarjunaX
2020-08-28 10:45 ` Vinod Koul
2020-08-31 8:06 ` Reddy, MallikarjunaX
2020-08-31 11:00 ` Vinod Koul
2020-09-01 15:03 ` Reddy, MallikarjunaX
2020-09-04 6:31 ` Peter Ujfalusi
2020-09-07 7:25 ` Reddy, MallikarjunaX
2020-08-14 5:26 ` [PATCH v5 2/2] Add Intel LGM soc DMA support Amireddy Mallikarjuna reddy
2020-08-14 8:47 ` kernel test robot
2020-08-18 10:16 ` Peter Ujfalusi
2020-08-18 10:29 ` Peter Ujfalusi
2020-08-24 2:30 ` Reddy, MallikarjunaX
2020-08-24 11:24 ` Peter Ujfalusi
2020-08-27 14:41 ` Reddy, MallikarjunaX
2020-08-28 11:17 ` Peter Ujfalusi
2020-08-31 8:07 ` Reddy, MallikarjunaX
2020-09-04 6:31 ` Peter Ujfalusi
2020-09-07 7:32 ` Reddy, MallikarjunaX
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7cdc0587-8b4f-4360-a303-1541c9ad57b2@linux.intel.com \
--to=mallikarjunax.reddy@linux.intel.com \
--cc=andriy.shevchenko@intel.com \
--cc=cheol.yong.kim@intel.com \
--cc=chuanhua.lei@linux.intel.com \
--cc=devicetree@vger.kernel.org \
--cc=dmaengine@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=malliamireddy009@gmail.com \
--cc=qi-ming.wu@intel.com \
--cc=robh@kernel.org \
--cc=vkoul@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).