Message-ID: <20201124172309.GU8403@vkoul-mobl>
Date:   Tue, 24 Nov 2020 22:53:09 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     "Reddy, MallikarjunaX" <mallikarjunax.reddy@...ux.intel.com>
Cc:     dmaengine@...r.kernel.org, devicetree@...r.kernel.org,
        robh+dt@...nel.org, linux-kernel@...r.kernel.org,
        andriy.shevchenko@...el.com, chuanhua.lei@...ux.intel.com,
        cheol.yong.kim@...el.com, qi-ming.wu@...el.com,
        malliamireddy009@...il.com, peter.ujfalusi@...com
Subject: Re: [PATCH v9 1/2] dt-bindings: dma: Add bindings for Intel LGM SoC

On 24-11-20, 00:30, Reddy, MallikarjunaX wrote:
> Hi Vinod,
> 
> Thanks for your valuable review. My comments inline.
> 
> On 11/21/2020 8:19 PM, Vinod Koul wrote:
> > On 20-11-20, 19:30, Reddy, MallikarjunaX wrote:
> > > Hi Vinod,
> > > Thanks for the review. My comments inline.
> > > 
> > > On 11/18/2020 11:55 PM, Vinod Koul wrote:
> > > > On 12-11-20, 13:38, 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@...ux.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.
> > > > > 
> > > > > v7-resend:
> > > > > - rebase to 5.10-rc1
> > > > > 
> > > > > v8:
> > > > > - rebased to 5.10-rc3
> > > > > - Fixing the bot issues (wrong indentation)
> > > > > 
> > > > > v9:
> > > > > - rebased to 5.10-rc3
> > > > > - Use 'enum' instead of oneOf+const
> > > > > - Drop '#dma-cells' in required:, already covered in dma-common.yaml
> > > > > - Drop nodename Already covered by dma-controller.yaml
> > > > > ---
> > > > >    .../devicetree/bindings/dma/intel,ldma.yaml        | 130 +++++++++++++++++++++
> > > > >    1 file changed, 130 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..c06281a10178
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/dma/intel,ldma.yaml
> > > > > @@ -0,0 +1,130 @@
> > > > > +# 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@...el.com
> > > > > +  - mallikarjunax.reddy@...el.com
> > > > > +
> > > > > +allOf:
> > > > > +  - $ref: "dma-controller.yaml#"
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - intel,lgm-cdma
> > > > > +      - intel,lgm-dma2tx
> > > > > +      - intel,lgm-dma1rx
> > > > > +      - intel,lgm-dma1tx
> > > > > +      - intel,lgm-dma0tx
> > > > > +      - intel,lgm-dma3
> > > > > +      - intel,lgm-toe-dma30
> > > > > +      - 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:
> > > > > +    maxItems: 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
> > > > s/poling/polling
> > > Ok, Thanks.
> > > > > +      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.
> > > > Can you explain this, also sounds you could use _maxburst values..?
> > > when dma-byte-en = 0 (disabled) DMA write will be in terms of burst length,
> > > dma-byte-en = 1 (enabled) write will be in terms of Dwords.
> > > 
> > > Byte enable = 0 (Disabled) means that DMA write will be based on the burst
> > > length, even if it only transmits one byte.
> > > Byte enable = 1(enabled) means that DMA write will be based on the number of
> > > Dwords, instead of the whole burst.
> > Sounds like a hw property or is this configurable to engine..?
> Yes its hw property. Not configurable to engine.
> > > > > +
> > > > > +  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.
> > > > should that not be decided by driver..? Or is this a hw property?
> > > This is DMA controller capability. It can be decided from driver also. i
> > > will change accordingly.
> > > > > +
> > > > > +  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.
> > > > How would this be used by folks..?
> > > A register bit will be used to enable/disable the ORR feature.
> > > 
> > > Outstanding Read Capability introduce CMD FIFO to support up to 16
> > > outstanding reads for different packet in same channel.
> > > 
> > > For large packets up to 16 OR can be issued, the number of OR is
> > > configurable.
> > How will configure this and when..?
> 
> This is DMA (ver > DMA_VER22) hw capability and is configured from device
> tree.
> 
> If this property is not present or count is zero means orrc capability is
> disabled.
> If orrc count is 4 <= orr_cnt < 16 then write the enable bit and value to
> corresponding register.
> 
> Ex:
>         if (d->orrc > 0 && d->orrc <= DMA_ORRC_MAX_CNT)
>                 val = DMA_ORRC_EN | FIELD_PREP(DMA_ORRC_ORRCNT, d->orrc);
> 
>         ldma_update_bits(d, mask, val, DMA_ORRC);
> 
> This hw capability supports dma instances ver > DMA_VER22.

Sounds like this can be coded in driver and used based on compatible?

-- 
~Vinod

Powered by blists - more mailing lists