Message-ID:
 <OS8PR06MB75410910BC7AD0BAF9866E3FF234A@OS8PR06MB7541.apcprd06.prod.outlook.com>
Date: Fri, 15 Aug 2025 07:06:11 +0000
From: Ryan Chen <ryan_chen@...eedtech.com>
To: Rob Herring <robh@...nel.org>
CC: "benh@...nel.crashing.org" <benh@...nel.crashing.org>, "joel@....id.au"
	<joel@....id.au>, "andi.shyti@...nel.org" <andi.shyti@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "andrew@...econstruct.com.au"
	<andrew@...econstruct.com.au>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, "andriy.shevchenko@...ux.intel.com"
	<andriy.shevchenko@...ux.intel.com>, "naresh.solanki@...ements.com"
	<naresh.solanki@...ements.com>, "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 v17 1/3] dt-bindings: i2c: aspeed,i2c.yaml: add
 transfer-mode and global-regs properties and update example

> Subject: Re: [PATCH v17 1/3] dt-bindings: i2c: aspeed,i2c.yaml: add
> transfer-mode and global-regs properties and update example
> 
> On Thu, Aug 14, 2025 at 04:41:54PM +0800, Ryan Chen wrote:
> > - Add property "aspeed,global-regs" to get phandle set global
> > register, for register mode selection and clock divider control.
> > - Add an optional property "aspeed,transfer-mode" to allow device tree
> > to specify the desired transfer method used by each I2C controller
> > instance.
> > - Update example to demonstrate usage of 'aspeed,global-regs' and
> > 'aspeed,transfer-mode' for AST2600 I2C controller.
> 
> All of this is evident reading the patch. Explain here why you need this change.

The AST2600 I2C controller supports three transfer modes: byte, buffer, and DMA.
To allow board designers and firmware to explicitly select the preferred transfer mode
for each controller instance, this patch introduces the aspeed,transfer-mode property
in the device tree binding.
On AST2600, each I2C controller can operate in either the legacy or the new
register mode and clock divider control, selectable by a global register setting. 

> 
> >
> > Signed-off-by: Ryan Chen <ryan_chen@...eedtech.com>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 39 +++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > index 5b9bd2feda3b..2a9f7d1d2ea1 100644
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -44,6 +44,34 @@ properties:
> >      description: frequency of the bus clock in Hz defaults to 100 kHz when
> not
> >        specified
> >
> > +  aspeed,transfer-mode:
> > +    description: |
> > +      ASPEED ast2600 platform equipped with 16 I2C controllers each i2c
> controller
> > +      have 1 byte transfer buffer(byte mode), 32 bytes buffer(buffer mode),
> and
> > +      share a DMA engine.
> > +      Select I2C transfer mode for this controller. Supported values are:
> > +        - "byte": Use 1 byte for i2c transmit (1-byte buffer).
> > +        - "buffer": Use buffer (32-byte buffer) for i2c transmit. (default)
> > +                    Better performance then byte mode.
> 
> If no 'aspeed,transfer-mode' property defaults to buffer mode, you never need
> 'aspeed,transfer-mode = "buffer"'.
> 
> When would you ever use "byte" mode? Sometimes you want worse
> performance? That makes no sense.
> 
> I feel like we already discussed this, but I'm not going to dig thru 17 versions to
> see.

Yes, my original through is dma and buffer property selection at v15, v16.
The reason for keeping "byte" mode is to support use cases reported by 
other users (for example, as raised by Yikai in the above thread),
where certain I2C devices or board configurations only function
correctly in byte mode. This is not a common case, but providing
this flexibility avoids the need for downstream patches or hacks.

https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235.2254138-3-ryan_chen@aspeedtech.com/#3412650
Yikai yikai.tsai.wiwynn@...il.com

> 
> > +        - "dma": Each controller DMA mode is shared DMA engine. The
> AST2600 SoC
> > +                 provides a single DMA engine shared for 16 I2C
> controllers,
> > +                 so only a limited number of controllers can use DMA
> simultaneously.
> > +                 Therefore, the DTS must explicitly assign which
> controllers are
> > +                 configured to use DMA.
> > +      Only one mode can be selected per controller.
> 
> The only thing that really makes sense is dma, and you need 1 boolean
> property for that. IOW, what you had on v10 which we gave reviewed-by 2+
> years ago.
> 
> Do you have any actual test results or usecase that show DMA is useful here?
> Typical I2C xfers are not streaming large amounts of data to justify the setup
> costs of DMA. It wouldn't surprise me if DMA was actually slower.
> 
For AST2600 used in server platforms, MCTP over I2C often requires transferring large
data blocks (e.g., 512 bytes per message). In buffer mode, this results in 32 interrupts
per transaction, while DMA mode reduces this to a single interrupt, significantly
lowering CPU load. We have observed measurable CPU utilization improvements
when using DMA mode for such use cases.

> > +      On AST2600, each controller supports all three modes.
> > +      If not specified, buffer mode is used by default.
> > +    enum:
> > +      - byte
> > +      - buffer
> > +      - dma
> > +
> > +  aspeed,global-regs:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: |
> 
> Don't need '|'. Read the documentation about when it is needed or not
> needed.

Thanks, will remove it.

> 
> > +      The phandle of i2c global register node, For control the i2c register
> > +      define selection, clock divider mode selection and clock divider
> control.
> > +
> >  required:
> >    - reg
> >    - compatible
> > @@ -66,3 +94,14 @@ examples:
> >        interrupts = <0>;
> >        interrupt-parent = <&i2c_ic>;
> >      };
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    i2c1: i2c@80 {
> > +      compatible = "aspeed,ast2600-i2c-bus";
> > +      reg = <0x80 0x80>, <0xc00 0x20>;
> > +      aspeed,global-regs = <&i2c_global>;
> > +      clocks = <&syscon ASPEED_CLK_APB>;
> > +      resets = <&syscon ASPEED_RESET_I2C>;
> > +      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
> > +      aspeed,transfer-mode = "buffer";
> > +    };
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists