linux-kernel - Re: [PATCH v17 1/3] dt-bindings: i2c: aspeed,i2c.yaml: add transfer-mode and global-regs properties and update example

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] [day] [month] [year] [list]

Message-ID: <20250814223052.GA4004307-robh@kernel.org>
Date: Thu, 14 Aug 2025 17:30:52 -0500
From: Rob Herring <robh@...nel.org>
To: Ryan Chen <ryan_chen@...eedtech.com>
Cc: benh@...nel.crashing.org, joel@....id.au, andi.shyti@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org,
	andrew@...econstruct.com.au, p.zabel@...gutronix.de,
	andriy.shevchenko@...ux.intel.com, naresh.solanki@...ements.com,
	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 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.

> 
> 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.

> +        - "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.

> +      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.

> +      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
>