linux-kernel - RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SEZPR06MB52695281E21B27DB57A7B2FFF2AD9@SEZPR06MB5269.apcprd06.prod.outlook.com>
Date:   Wed, 1 Mar 2023 03:02:11 +0000
From:   Ryan Chen <ryan_chen@...eedtech.com>
To:     Jeremy Kerr <jk@...econstruct.com.au>,
        Andrew Jeffery <andrew@...id.au>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Joel Stanley <joel@....id.au>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
 AST2600-i2cv2

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <jk@...econstruct.com.au>
> Sent: Monday, February 27, 2023 1:40 PM
> To: Ryan Chen <ryan_chen@...eedtech.com>; Andrew Jeffery
> <andrew@...id.au>; Brendan Higgins <brendan.higgins@...ux.dev>; Benjamin
> Herrenschmidt <benh@...nel.crashing.org>; Joel Stanley <joel@....id.au>;
> Rob Herring <robh+dt@...nel.org>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Philipp Zabel <p.zabel@...gutronix.de>;
> linux-i2c@...r.kernel.org; openbmc@...ts.ozlabs.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org;
> linux-aspeed@...ts.ozlabs.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > Yes, I2C controller share the same dma engine. The original thought
> > can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA
> > fails when DRAM bus is busy and it can not take DMA write data
> > immediately", So it means only 1 i2c bus can be enable for DMA mode.
> 
> OK, this is a pretty important detail! I'd suggest putting it in the binding
> document.

Sorry, Do you mean add in description like following??
  aspeed,xfer-mode:
    description: |
      I2C bus transfer mode selection.
	  ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data
Immediately", only 1 i2c bus can be enable for DMA mode.
      - "byte": I2C bus byte transfer mode.
      - "buffered": I2C bus buffer register transfer mode.
      - "dma": I2C bus dma transfer mode (default)
      
> Anything in the cover letter will get lost after review. If there is documentation
> that would be useful for a DTS author, I'd suggest putting it in the binding.
> 
> > It means only 1 bus channel can be enable DMA for use case.
> > That following example for board-specific selection.
> > It is description in cover-letter.
> > The following is board-specific design example.
> > Board A                                         Board
> B
> > -------------------------
> > ------------------------
> > > i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x
> > > (master/slave)| i2c bus#2(master)-> tmp i2c device
> |        |
> > > | i2c bus#3(master)-> adc i2c device
> |        |                   |
> > -------------------------
> > ------------------------
> >
> > - in bus#1 situation, you should use DMA mode.
> > Because bus#1 have trunk data needed for transfer, it can enable bus
> > dma mode to reduce cpu utilized.
> 
> What is "trunk data" in this context? Is this just a statement about the amount
> of expected transfers?
Sorry, I can't catch your point, for example for most server application usage.
The i2c not only connect with small device (like temperature sensor/ adc).
It also connect with other mcu base device support i2c slave.
Most case is transfer MCTP package. (basic 64kbytes). So I say "trunk data".

> 
> > - in bus#2/3 situation, you should use buffer/byte mode
> > bus#2/3 is small package transmit, it can enable buffer mode or byte
> > mode to reduce memory cache flush overhead.
> > Buffer mode is better, because byte mode have interrupt
> > overhead(interrupt per byte data transmit),
> >
> > -But if you more bus#4 that still have trunk data needed for transfer
> > (master/slave), it also use buffer mode to transmit. Because bus#1
> > have been use for DMA mode.
> 
> So, it sounds like:
> 
>  - there's no point in using byte mode, as buffer mode provides
>    equivalent functionality with fewer drawbacks (ie, less interrupt
>    load)
> 
>  - this just leaves the dma and buffer modes
> 
>  - only one controller can use dma mode
> 
> So: how about just a single boolean property to indicate "use DMA on this
> controller"? Something like aspeed,enable-dma? Or if DT binding experts can
> suggest something common that might be more suitable?

If so, just leave enable-dma and only support for buffer mode and dma mode, am I right?

Best Regards.
Ryan