[PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
and description for ast2600-i2cv2.

Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
index 5b9bd2feda3b..356033d18f90 100644
--- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
@@ -44,12 +44,60 @@ properties:
     description: frequency of the bus clock in Hz defaults to 100 kHz when not
       specified
 
+  multi-master:
+    type: boolean
+    description:
+      states that there is another master active on this bus
+
+  aspeed,enable-dma:
+    type: boolean
+    description: |
+      I2C bus enable dma mode transfer.
+
+      ASPEED ast2600 platform equipped with 16 I2C controllers that share a
+      single DMA engine. DTS files can specify the data transfer mode to/from
+      the device, either DMA or programmed I/O. However, hardware limitations
+      may require a DTS to manually allocate which controller can use DMA mode.
+      The "aspeed,enable-dma" property allows control of this.
+
+      In cases where one the hardware design results in a specific
+      controller handling a larger amount of data, a DTS would likely
+      enable DMA mode for that one controller.
+
+  aspeed,enable-byte:
+    type: boolean
+    description: |
+      I2C bus enable byte mode transfer.
+
+  aspeed,global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of i2c global register node.
+
 required:
   - reg
   - compatible
   - clocks
   - resets
 
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: aspeed,ast2600-i2cv2
+
+    then:
+      properties:
+        reg:
+          minItems: 2
+      required:
+        - aspeed,global-regs
+    else:
+      properties:
+        aspeed,global-regs: false
+        aspeed,enable-dma: false
+
 unevaluatedProperties: false
 
 examples:
@@ -66,3 +114,13 @@ examples:
       interrupts = <0>;
       interrupt-parent = <&i2c_ic>;
     };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    i2c1: i2c@80 {
+      compatible = "aspeed,ast2600-i2cv2";
+      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>;
+    };
-- 
2.34.1

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Rob Herring (Arm)]

On Mon, 24 Feb 2025 13:59:34 +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> and description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: [error] duplication of key "allOf" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml: ignoring, error parsing file
./Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[2]: *** Deleting file 'Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dts'
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1511: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250224055936.1804279-2-ryan_chen@aspeedtech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Rob Herring (Arm)]

On Mon, 24 Feb 2025 13:59:34 +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> and description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: [error] duplication of key "allOf" in mapping (key-duplicates)

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml: ignoring, error parsing file
./Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[2]: *** Deleting file 'Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dts'
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml:82:1: found duplicate key "allOf" with value "[]" (original value: "[]")
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/i2c/aspeed,i2c.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1511: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250224055936.1804279-2-ryan_chen@aspeedtech.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> and description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index 5b9bd2feda3b..356033d18f90 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -44,12 +44,60 @@ properties:
>      description: frequency of the bus clock in Hz defaults to 100 kHz when not
>        specified
>  
> +  multi-master:
> +    type: boolean
> +    description:
> +      states that there is another master active on this bus

Except that this wasn't ever tested...

Don't duplicate properties. i2c-controller schema has it already.

> +
> +  aspeed,enable-dma:
> +    type: boolean
> +    description: |
> +      I2C bus enable dma mode transfer.
> +
> +      ASPEED ast2600 platform equipped with 16 I2C controllers that share a
> +      single DMA engine. DTS files can specify the data transfer mode to/from
> +      the device, either DMA or programmed I/O. However, hardware limitations

so what is byte mode?

> +      may require a DTS to manually allocate which controller can use DMA mode.
> +      The "aspeed,enable-dma" property allows control of this.
> +
> +      In cases where one the hardware design results in a specific
> +      controller handling a larger amount of data, a DTS would likely
> +      enable DMA mode for that one controller.
> +
> +  aspeed,enable-byte:
> +    type: boolean
> +    description: |
> +      I2C bus enable byte mode transfer.

No, either this is expressed as lack of dma mode property or if you have
three modes, then rather some enum (aspeed,transfer-mode ?)



> +
> +  aspeed,global-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of i2c global register node.

For what? Same question as usual: do not repeat property name, but say
what is this used for and what exactly it points to.

s/i2c/I2C/ but then what is "I2C global register node"? This is how you
call your device in datasheet?


> +
>  required:
>    - reg
>    - compatible
>    - clocks
>    - resets
>  
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-i2cv2

NAK, undocumented compatible.

> +
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2


Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> and description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> index 5b9bd2feda3b..356033d18f90 100644
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -44,12 +44,60 @@ properties:
>      description: frequency of the bus clock in Hz defaults to 100 kHz when not
>        specified
>  
> +  multi-master:
> +    type: boolean
> +    description:
> +      states that there is another master active on this bus

Except that this wasn't ever tested...

Don't duplicate properties. i2c-controller schema has it already.

> +
> +  aspeed,enable-dma:
> +    type: boolean
> +    description: |
> +      I2C bus enable dma mode transfer.
> +
> +      ASPEED ast2600 platform equipped with 16 I2C controllers that share a
> +      single DMA engine. DTS files can specify the data transfer mode to/from
> +      the device, either DMA or programmed I/O. However, hardware limitations

so what is byte mode?

> +      may require a DTS to manually allocate which controller can use DMA mode.
> +      The "aspeed,enable-dma" property allows control of this.
> +
> +      In cases where one the hardware design results in a specific
> +      controller handling a larger amount of data, a DTS would likely
> +      enable DMA mode for that one controller.
> +
> +  aspeed,enable-byte:
> +    type: boolean
> +    description: |
> +      I2C bus enable byte mode transfer.

No, either this is expressed as lack of dma mode property or if you have
three modes, then rather some enum (aspeed,transfer-mode ?)



> +
> +  aspeed,global-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of i2c global register node.

For what? Same question as usual: do not repeat property name, but say
what is this used for and what exactly it points to.

s/i2c/I2C/ but then what is "I2C global register node"? This is how you
call your device in datasheet?


> +
>  required:
>    - reg
>    - compatible
>    - clocks
>    - resets
>  
> +allOf:
> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: aspeed,ast2600-i2cv2

NAK, undocumented compatible.

> +
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2


Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/02/2025 10:11, Krzysztof Kozlowski wrote:
> 
> 
>> +
>>  required:
>>    - reg
>>    - compatible
>>    - clocks
>>    - resets
>>  
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: aspeed,ast2600-i2cv2
> 
> NAK, undocumented compatible.

Heh, and this was in previous versions as well.

You have never tested your DTS. At v16 still no testing. This is not
acceptable.

Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/02/2025 10:11, Krzysztof Kozlowski wrote:
> 
> 
>> +
>>  required:
>>    - reg
>>    - compatible
>>    - clocks
>>    - resets
>>  
>> +allOf:
>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: aspeed,ast2600-i2cv2
> 
> NAK, undocumented compatible.

Heh, and this was in previous versions as well.

You have never tested your DTS. At v16 still no testing. This is not
acceptable.

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> > Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> > and description for ast2600-i2cv2.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > index 5b9bd2feda3b..356033d18f90 100644
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -44,12 +44,60 @@ properties:
> >      description: frequency of the bus clock in Hz defaults to 100 kHz when
> not
> >        specified
> >
> > +  multi-master:
> > +    type: boolean
> > +    description:
> > +      states that there is another master active on this bus
> 
> Except that this wasn't ever tested...
> 
> Don't duplicate properties. i2c-controller schema has it already.

I will remove it to avoid duplication.
> 
> > +
> > +  aspeed,enable-dma:
> > +    type: boolean
> > +    description: |
> > +      I2C bus enable dma mode transfer.
> > +
> > +      ASPEED ast2600 platform equipped with 16 I2C controllers that
> share a
> > +      single DMA engine. DTS files can specify the data transfer mode
> to/from
> > +      the device, either DMA or programmed I/O. However, hardware
> > + limitations
> 
> so what is byte mode?
I explain in cover, I will add here also. 
aspeed,enable-byte:
Force i2c controller use byte mode transfer. the byte mode transfer
will send i2c data each byte by byte, inlcude address read/write.

> 
> > +      may require a DTS to manually allocate which controller can use
> DMA mode.
> > +      The "aspeed,enable-dma" property allows control of this.
> > +
> > +      In cases where one the hardware design results in a specific
> > +      controller handling a larger amount of data, a DTS would likely
> > +      enable DMA mode for that one controller.
> > +
> > +  aspeed,enable-byte:
> > +    type: boolean
> > +    description: |
> > +      I2C bus enable byte mode transfer.
> 
> No, either this is expressed as lack of dma mode property or if you have three
> modes, then rather some enum (aspeed,transfer-mode ?)

Thanks suggestion, I will using an enum property like aspeed,transfer-mode instead.
> 
> 
> 
> > +
> > +  aspeed,global-regs:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of i2c global register node.
> 
> For what? Same question as usual: do not repeat property name, but say what
> is this used for and what exactly it points to.
> 
> s/i2c/I2C/ but then what is "I2C global register node"? This is how you call your
> device in datasheet?
> 
I do descript in cover, should add into the yaml file ?

aspeed,global-regs: 
This global register is needed, global register is setting for
new clock divide control, and new register set control.

> 
> > +
> >  required:
> >    - reg
> >    - compatible
> >    - clocks
> >    - resets
> >
> > +allOf:
> > +  - $ref: /schemas/i2c/i2c-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: aspeed,ast2600-i2cv2
> 
> NAK, undocumented compatible.

Sorry, I should add what kind of document about this compatible?

The cover descripts it, should I add into yaml?


This series add AST2600 i2cv2 new register set driver. The i2cv2 driver
is new register set that have new clock divider option for more flexiable
generation. And also have separate i2c controller and target register
set for control, patch #2 is i2c controller driver only, patch #3 is add
i2c target mode driver.

The legacy register layout is mix controller/target register control
together. The following is add more detail description about new register
layout. And new feature set add for register.
> 
> > +
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 2
> 
> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> > Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
> > and description for ast2600-i2cv2.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
> >  1 file changed, 58 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > index 5b9bd2feda3b..356033d18f90 100644
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -44,12 +44,60 @@ properties:
> >      description: frequency of the bus clock in Hz defaults to 100 kHz when
> not
> >        specified
> >
> > +  multi-master:
> > +    type: boolean
> > +    description:
> > +      states that there is another master active on this bus
> 
> Except that this wasn't ever tested...
> 
> Don't duplicate properties. i2c-controller schema has it already.

I will remove it to avoid duplication.
> 
> > +
> > +  aspeed,enable-dma:
> > +    type: boolean
> > +    description: |
> > +      I2C bus enable dma mode transfer.
> > +
> > +      ASPEED ast2600 platform equipped with 16 I2C controllers that
> share a
> > +      single DMA engine. DTS files can specify the data transfer mode
> to/from
> > +      the device, either DMA or programmed I/O. However, hardware
> > + limitations
> 
> so what is byte mode?
I explain in cover, I will add here also. 
aspeed,enable-byte:
Force i2c controller use byte mode transfer. the byte mode transfer
will send i2c data each byte by byte, inlcude address read/write.

> 
> > +      may require a DTS to manually allocate which controller can use
> DMA mode.
> > +      The "aspeed,enable-dma" property allows control of this.
> > +
> > +      In cases where one the hardware design results in a specific
> > +      controller handling a larger amount of data, a DTS would likely
> > +      enable DMA mode for that one controller.
> > +
> > +  aspeed,enable-byte:
> > +    type: boolean
> > +    description: |
> > +      I2C bus enable byte mode transfer.
> 
> No, either this is expressed as lack of dma mode property or if you have three
> modes, then rather some enum (aspeed,transfer-mode ?)

Thanks suggestion, I will using an enum property like aspeed,transfer-mode instead.
> 
> 
> 
> > +
> > +  aspeed,global-regs:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of i2c global register node.
> 
> For what? Same question as usual: do not repeat property name, but say what
> is this used for and what exactly it points to.
> 
> s/i2c/I2C/ but then what is "I2C global register node"? This is how you call your
> device in datasheet?
> 
I do descript in cover, should add into the yaml file ?

aspeed,global-regs: 
This global register is needed, global register is setting for
new clock divide control, and new register set control.

> 
> > +
> >  required:
> >    - reg
> >    - compatible
> >    - clocks
> >    - resets
> >
> > +allOf:
> > +  - $ref: /schemas/i2c/i2c-controller.yaml#
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: aspeed,ast2600-i2cv2
> 
> NAK, undocumented compatible.

Sorry, I should add what kind of document about this compatible?

The cover descripts it, should I add into yaml?


This series add AST2600 i2cv2 new register set driver. The i2cv2 driver
is new register set that have new clock divider option for more flexiable
generation. And also have separate i2c controller and target register
set for control, patch #2 is i2c controller driver only, patch #3 is add
i2c target mode driver.

The legacy register layout is mix controller/target register control
together. The following is add more detail description about new register
layout. And new feature set add for register.
> 
> > +
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 2
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 26/02/2025 10:28, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
>>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
>>> and description for ast2600-i2cv2.
>>>
>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
>>> ---
>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>>>  1 file changed, 58 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> index 5b9bd2feda3b..356033d18f90 100644
>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> @@ -44,12 +44,60 @@ properties:
>>>      description: frequency of the bus clock in Hz defaults to 100 kHz when
>> not
>>>        specified
>>>
>>> +  multi-master:
>>> +    type: boolean
>>> +    description:
>>> +      states that there is another master active on this bus
>>
>> Except that this wasn't ever tested...
>>
>> Don't duplicate properties. i2c-controller schema has it already.
> 
> I will remove it to avoid duplication.
>>
>>> +
>>> +  aspeed,enable-dma:
>>> +    type: boolean
>>> +    description: |
>>> +      I2C bus enable dma mode transfer.
>>> +
>>> +      ASPEED ast2600 platform equipped with 16 I2C controllers that
>> share a
>>> +      single DMA engine. DTS files can specify the data transfer mode
>> to/from
>>> +      the device, either DMA or programmed I/O. However, hardware
>>> + limitations
>>
>> so what is byte mode?
> I explain in cover, I will add here also. 

Cover letters do not get merged and we do not read them, except when
looking for dependencies and explanations of process (like RFC). Your
hardware description must be fully contained in commits, not cover letter.


> aspeed,enable-byte:
> Force i2c controller use byte mode transfer. the byte mode transfer
> will send i2c data each byte by byte, inlcude address read/write.

Isn't this standard FIFO mode?

Why anyone would need to enable byte mode for given board?

> 
>>
>>> +      may require a DTS to manually allocate which controller can use
>> DMA mode.
>>> +      The "aspeed,enable-dma" property allows control of this.
>>> +
>>> +      In cases where one the hardware design results in a specific
>>> +      controller handling a larger amount of data, a DTS would likely
>>> +      enable DMA mode for that one controller.
>>> +
>>> +  aspeed,enable-byte:
>>> +    type: boolean
>>> +    description: |
>>> +      I2C bus enable byte mode transfer.
>>
>> No, either this is expressed as lack of dma mode property or if you have three
>> modes, then rather some enum (aspeed,transfer-mode ?)
> 
> Thanks suggestion, I will using an enum property like aspeed,transfer-mode instead.
>>
>>
>>
>>> +
>>> +  aspeed,global-regs:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: The phandle of i2c global register node.
>>
>> For what? Same question as usual: do not repeat property name, but say what
>> is this used for and what exactly it points to.
>>
>> s/i2c/I2C/ but then what is "I2C global register node"? This is how you call your
>> device in datasheet?
>>
> I do descript in cover, should add into the yaml file ?


Again, cover letter does not matter. Your hardware must be explained here.

> 
> aspeed,global-regs: 
> This global register is needed, global register is setting for
> new clock divide control, and new register set control.

So this means you implement clock controller via syscon?

> 
>>
>>> +
>>>  required:
>>>    - reg
>>>    - compatible
>>>    - clocks
>>>    - resets
>>>
>>> +allOf:
>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: aspeed,ast2600-i2cv2
>>
>> NAK, undocumented compatible.
> 
> Sorry, I should add what kind of document about this compatible?

You cannot add new compatibles without documenting them. Documentation
is in the form of DT schema and each compatible must be listed (in some
way) in compatible property description.


Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 26/02/2025 10:28, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
>>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,enable-dma
>>> and description for ast2600-i2cv2.
>>>
>>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
>>> ---
>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58 +++++++++++++++++++
>>>  1 file changed, 58 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> index 5b9bd2feda3b..356033d18f90 100644
>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> @@ -44,12 +44,60 @@ properties:
>>>      description: frequency of the bus clock in Hz defaults to 100 kHz when
>> not
>>>        specified
>>>
>>> +  multi-master:
>>> +    type: boolean
>>> +    description:
>>> +      states that there is another master active on this bus
>>
>> Except that this wasn't ever tested...
>>
>> Don't duplicate properties. i2c-controller schema has it already.
> 
> I will remove it to avoid duplication.
>>
>>> +
>>> +  aspeed,enable-dma:
>>> +    type: boolean
>>> +    description: |
>>> +      I2C bus enable dma mode transfer.
>>> +
>>> +      ASPEED ast2600 platform equipped with 16 I2C controllers that
>> share a
>>> +      single DMA engine. DTS files can specify the data transfer mode
>> to/from
>>> +      the device, either DMA or programmed I/O. However, hardware
>>> + limitations
>>
>> so what is byte mode?
> I explain in cover, I will add here also. 

Cover letters do not get merged and we do not read them, except when
looking for dependencies and explanations of process (like RFC). Your
hardware description must be fully contained in commits, not cover letter.


> aspeed,enable-byte:
> Force i2c controller use byte mode transfer. the byte mode transfer
> will send i2c data each byte by byte, inlcude address read/write.

Isn't this standard FIFO mode?

Why anyone would need to enable byte mode for given board?

> 
>>
>>> +      may require a DTS to manually allocate which controller can use
>> DMA mode.
>>> +      The "aspeed,enable-dma" property allows control of this.
>>> +
>>> +      In cases where one the hardware design results in a specific
>>> +      controller handling a larger amount of data, a DTS would likely
>>> +      enable DMA mode for that one controller.
>>> +
>>> +  aspeed,enable-byte:
>>> +    type: boolean
>>> +    description: |
>>> +      I2C bus enable byte mode transfer.
>>
>> No, either this is expressed as lack of dma mode property or if you have three
>> modes, then rather some enum (aspeed,transfer-mode ?)
> 
> Thanks suggestion, I will using an enum property like aspeed,transfer-mode instead.
>>
>>
>>
>>> +
>>> +  aspeed,global-regs:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: The phandle of i2c global register node.
>>
>> For what? Same question as usual: do not repeat property name, but say what
>> is this used for and what exactly it points to.
>>
>> s/i2c/I2C/ but then what is "I2C global register node"? This is how you call your
>> device in datasheet?
>>
> I do descript in cover, should add into the yaml file ?


Again, cover letter does not matter. Your hardware must be explained here.

> 
> aspeed,global-regs: 
> This global register is needed, global register is setting for
> new clock divide control, and new register set control.

So this means you implement clock controller via syscon?

> 
>>
>>> +
>>>  required:
>>>    - reg
>>>    - compatible
>>>    - clocks
>>>    - resets
>>>
>>> +allOf:
>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            const: aspeed,ast2600-i2cv2
>>
>> NAK, undocumented compatible.
> 
> Sorry, I should add what kind of document about this compatible?

You cannot add new compatibles without documenting them. Documentation
is in the form of DT schema and each compatible must be listed (in some
way) in compatible property description.


Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 26/02/2025 10:28, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> >>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>> aspeed,enable-dma and description for ast2600-i2cv2.
> >>>
> >>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >>> ---
> >>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58
> +++++++++++++++++++
> >>>  1 file changed, 58 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> index 5b9bd2feda3b..356033d18f90 100644
> >>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> @@ -44,12 +44,60 @@ properties:
> >>>      description: frequency of the bus clock in Hz defaults to 100
> >>> kHz when
> >> not
> >>>        specified
> >>>
> >>> +  multi-master:
> >>> +    type: boolean
> >>> +    description:
> >>> +      states that there is another master active on this bus
> >>
> >> Except that this wasn't ever tested...
> >>
> >> Don't duplicate properties. i2c-controller schema has it already.
> >
> > I will remove it to avoid duplication.
> >>
> >>> +
> >>> +  aspeed,enable-dma:
> >>> +    type: boolean
> >>> +    description: |
> >>> +      I2C bus enable dma mode transfer.
> >>> +
> >>> +      ASPEED ast2600 platform equipped with 16 I2C controllers that
> >> share a
> >>> +      single DMA engine. DTS files can specify the data transfer
> >>> + mode
> >> to/from
> >>> +      the device, either DMA or programmed I/O. However, hardware
> >>> + limitations
> >>
> >> so what is byte mode?
> > I explain in cover, I will add here also.
> 
> Cover letters do not get merged and we do not read them, except when looking
> for dependencies and explanations of process (like RFC). Your hardware
> description must be fully contained in commits, not cover letter.

Got it, I will paste it in commits. 
> 
> 
> > aspeed,enable-byte:
> > Force i2c controller use byte mode transfer. the byte mode transfer
> > will send i2c data each byte by byte, inlcude address read/write.
> 
> Isn't this standard FIFO mode?
Yes, it is.
> 
> Why anyone would need to enable byte mode for given board?
By default, it is buffer-mode, for performance, I don't want user enable byte-mode, it will increase CPU utilize.
But someone want to be force enable byte-mode, so I add properties. 
https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235.2254138-3-ryan_chen@aspeedtech.com/
> 
> >
> >>
> >>> +      may require a DTS to manually allocate which controller can
> >>> + use
> >> DMA mode.
> >>> +      The "aspeed,enable-dma" property allows control of this.
> >>> +
> >>> +      In cases where one the hardware design results in a specific
> >>> +      controller handling a larger amount of data, a DTS would likely
> >>> +      enable DMA mode for that one controller.
> >>> +
> >>> +  aspeed,enable-byte:
> >>> +    type: boolean
> >>> +    description: |
> >>> +      I2C bus enable byte mode transfer.
> >>
> >> No, either this is expressed as lack of dma mode property or if you
> >> have three modes, then rather some enum (aspeed,transfer-mode ?)
> >
> > Thanks suggestion, I will using an enum property like aspeed,transfer-mode
> instead.
> >>
> >>
> >>
> >>> +
> >>> +  aspeed,global-regs:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: The phandle of i2c global register node.
> >>
> >> For what? Same question as usual: do not repeat property name, but
> >> say what is this used for and what exactly it points to.
> >>
> >> s/i2c/I2C/ but then what is "I2C global register node"? This is how
> >> you call your device in datasheet?
> >>
> > I do descript in cover, should add into the yaml file ?
> 
> 
> Again, cover letter does not matter. Your hardware must be explained here.
Will add into commit. 
> 
> >
> > aspeed,global-regs:
> > This global register is needed, global register is setting for new
> > clock divide control, and new register set control.
> 
> So this means you implement clock controller via syscon?
No, it is just mode switch. It also explain in cover. I will add it in commit. 
The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
> 
> >
> >>
> >>> +
> >>>  required:
> >>>    - reg
> >>>    - compatible
> >>>    - clocks
> >>>    - resets
> >>>
> >>> +allOf:
> >>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: aspeed,ast2600-i2cv2
> >>
> >> NAK, undocumented compatible.
> >
> > Sorry, I should add what kind of document about this compatible?
> 
> You cannot add new compatibles without documenting them. Documentation
> is in the form of DT schema and each compatible must be listed (in some
> way) in compatible property description.

Sorry, do you mean, I add following in yaml or commit message?

This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new register set that have new clock divider option for more flexiable generation. And also have separate i2c controller and target register set for control, patch #2 is i2c controller driver only, patch #3 is add i2c target mode driver.

The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.

-Add new clock divider option for more flexible and accurate clock rate generation -Add tCKHighMin timing to guarantee SCL high pulse width.
-Add support dual pool buffer mode, split 32 bytes pool buffer of each device into 2 x 16 bytes for Tx and Rx individually.
-Increase DMA buffer size to 4096 bytes and support byte alignment.
-Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
-Re-define registers for separating controller and target mode control.
-Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.

> 
> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 26/02/2025 10:28, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On Mon, Feb 24, 2025 at 01:59:34PM +0800, Ryan Chen wrote:
> >>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>> aspeed,enable-dma and description for ast2600-i2cv2.
> >>>
> >>> Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> >>> ---
> >>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 58
> +++++++++++++++++++
> >>>  1 file changed, 58 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> index 5b9bd2feda3b..356033d18f90 100644
> >>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> @@ -44,12 +44,60 @@ properties:
> >>>      description: frequency of the bus clock in Hz defaults to 100
> >>> kHz when
> >> not
> >>>        specified
> >>>
> >>> +  multi-master:
> >>> +    type: boolean
> >>> +    description:
> >>> +      states that there is another master active on this bus
> >>
> >> Except that this wasn't ever tested...
> >>
> >> Don't duplicate properties. i2c-controller schema has it already.
> >
> > I will remove it to avoid duplication.
> >>
> >>> +
> >>> +  aspeed,enable-dma:
> >>> +    type: boolean
> >>> +    description: |
> >>> +      I2C bus enable dma mode transfer.
> >>> +
> >>> +      ASPEED ast2600 platform equipped with 16 I2C controllers that
> >> share a
> >>> +      single DMA engine. DTS files can specify the data transfer
> >>> + mode
> >> to/from
> >>> +      the device, either DMA or programmed I/O. However, hardware
> >>> + limitations
> >>
> >> so what is byte mode?
> > I explain in cover, I will add here also.
> 
> Cover letters do not get merged and we do not read them, except when looking
> for dependencies and explanations of process (like RFC). Your hardware
> description must be fully contained in commits, not cover letter.

Got it, I will paste it in commits. 
> 
> 
> > aspeed,enable-byte:
> > Force i2c controller use byte mode transfer. the byte mode transfer
> > will send i2c data each byte by byte, inlcude address read/write.
> 
> Isn't this standard FIFO mode?
Yes, it is.
> 
> Why anyone would need to enable byte mode for given board?
By default, it is buffer-mode, for performance, I don't want user enable byte-mode, it will increase CPU utilize.
But someone want to be force enable byte-mode, so I add properties. 
https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235.2254138-3-ryan_chen@aspeedtech.com/
> 
> >
> >>
> >>> +      may require a DTS to manually allocate which controller can
> >>> + use
> >> DMA mode.
> >>> +      The "aspeed,enable-dma" property allows control of this.
> >>> +
> >>> +      In cases where one the hardware design results in a specific
> >>> +      controller handling a larger amount of data, a DTS would likely
> >>> +      enable DMA mode for that one controller.
> >>> +
> >>> +  aspeed,enable-byte:
> >>> +    type: boolean
> >>> +    description: |
> >>> +      I2C bus enable byte mode transfer.
> >>
> >> No, either this is expressed as lack of dma mode property or if you
> >> have three modes, then rather some enum (aspeed,transfer-mode ?)
> >
> > Thanks suggestion, I will using an enum property like aspeed,transfer-mode
> instead.
> >>
> >>
> >>
> >>> +
> >>> +  aspeed,global-regs:
> >>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>> +    description: The phandle of i2c global register node.
> >>
> >> For what? Same question as usual: do not repeat property name, but
> >> say what is this used for and what exactly it points to.
> >>
> >> s/i2c/I2C/ but then what is "I2C global register node"? This is how
> >> you call your device in datasheet?
> >>
> > I do descript in cover, should add into the yaml file ?
> 
> 
> Again, cover letter does not matter. Your hardware must be explained here.
Will add into commit. 
> 
> >
> > aspeed,global-regs:
> > This global register is needed, global register is setting for new
> > clock divide control, and new register set control.
> 
> So this means you implement clock controller via syscon?
No, it is just mode switch. It also explain in cover. I will add it in commit. 
The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
> 
> >
> >>
> >>> +
> >>>  required:
> >>>    - reg
> >>>    - compatible
> >>>    - clocks
> >>>    - resets
> >>>
> >>> +allOf:
> >>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            const: aspeed,ast2600-i2cv2
> >>
> >> NAK, undocumented compatible.
> >
> > Sorry, I should add what kind of document about this compatible?
> 
> You cannot add new compatibles without documenting them. Documentation
> is in the form of DT schema and each compatible must be listed (in some
> way) in compatible property description.

Sorry, do you mean, I add following in yaml or commit message?

This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new register set that have new clock divider option for more flexiable generation. And also have separate i2c controller and target register set for control, patch #2 is i2c controller driver only, patch #3 is add i2c target mode driver.

The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.

-Add new clock divider option for more flexible and accurate clock rate generation -Add tCKHighMin timing to guarantee SCL high pulse width.
-Add support dual pool buffer mode, split 32 bytes pool buffer of each device into 2 x 16 bytes for Tx and Rx individually.
-Increase DMA buffer size to 4096 bytes and support byte alignment.
-Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
-Re-define registers for separating controller and target mode control.
-Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[kernel test robot]

:::::: 
:::::: Manual check reason: "dtcheck: binding changes may go via different trees"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250224055936.1804279-2-ryan_chen@aspeedtech.com>
References: <20250224055936.1804279-2-ryan_chen@aspeedtech.com>
TO: Ryan Chen <ryan_chen@aspeedtech.com>
TO: ryan_chen@aspeedtech.com
TO: benh@kernel.crashing.org
TO: joel@jms.id.au
TO: andi.shyti@kernel.org
TO: robh@kernel.org
TO: krzk+dt@kernel.org
TO: conor+dt@kernel.org
TO: andrew@codeconstruct.com.au
TO: p.zabel@pengutronix.de
TO: andriy.shevchenko@linux.intel.com
TO: linux-i2c@vger.kernel.org
TO: openbmc@lists.ozlabs.org
TO: devicetree@vger.kernel.org
TO: linux-arm-kernel@lists.infradead.org
TO: linux-aspeed@lists.ozlabs.org
TO: linux-kernel@vger.kernel.org

Hi Ryan,

kernel test robot noticed the following build warnings:

[auto build test WARNING on andi-shyti/i2c/i2c-host]
[also build test WARNING on linus/master v6.14-rc4 next-20250227]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ryan-Chen/dt-bindings-i2c-aspeed-support-for-AST2600-i2cv2/20250224-140221
base:   https://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20250224055936.1804279-2-ryan_chen%40aspeedtech.com
patch subject: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: csky-randconfig-051-20250227 (https://download.01.org/0day-ci/archive/20250227/202502272019.a37Bi8kD-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.2.0
dtschema version: 2025.3.dev3+gabf9328
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250227/202502272019.a37Bi8kD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202502272019.a37Bi8kD-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
>> Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml: ignoring, error parsing file
   Documentation/devicetree/bindings/net/snps,dwmac.yaml: mac-mode: missing type definition

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 27/02/2025 09:19, Ryan Chen wrote:
>>
>>
>>> aspeed,enable-byte:
>>> Force i2c controller use byte mode transfer. the byte mode transfer
>>> will send i2c data each byte by byte, inlcude address read/write.
>>
>> Isn't this standard FIFO mode?
> Yes, it is.
>>
>> Why anyone would need to enable byte mode for given board?
> By default, it is buffer-mode, for performance, I don't want user enable byte-mode, it will increase CPU utilize.
> But someone want to be force enable byte-mode, so I add properties. 
> https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235.2254138-3-ryan_chen@aspeedtech.com/


I don't see the reason why this would be a board property.

I understood need for DMA because it is shared and only some of the
controllers can use it. But why choice between buffer and FIFO depending
on hardware?


>>
>>>
>>>>
>>>>> +      may require a DTS to manually allocate which controller can
>>>>> + use
>>>> DMA mode.
>>>>> +      The "aspeed,enable-dma" property allows control of this.
>>>>> +
>>>>> +      In cases where one the hardware design results in a specific
>>>>> +      controller handling a larger amount of data, a DTS would likely
>>>>> +      enable DMA mode for that one controller.
>>>>> +
>>>>> +  aspeed,enable-byte:
>>>>> +    type: boolean
>>>>> +    description: |
>>>>> +      I2C bus enable byte mode transfer.
>>>>
>>>> No, either this is expressed as lack of dma mode property or if you
>>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
>>>
>>> Thanks suggestion, I will using an enum property like aspeed,transfer-mode
>> instead.
>>>>
>>>>
>>>>
>>>>> +
>>>>> +  aspeed,global-regs:
>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>> +    description: The phandle of i2c global register node.
>>>>
>>>> For what? Same question as usual: do not repeat property name, but
>>>> say what is this used for and what exactly it points to.
>>>>
>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
>>>> you call your device in datasheet?
>>>>
>>> I do descript in cover, should add into the yaml file ?
>>
>>
>> Again, cover letter does not matter. Your hardware must be explained here.
> Will add into commit. 
>>
>>>
>>> aspeed,global-regs:
>>> This global register is needed, global register is setting for new
>>> clock divide control, and new register set control.
>>
>> So this means you implement clock controller via syscon?
> No, it is just mode switch. It also explain in cover. I will add it in commit. 
> The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
>>
>>>
>>>>
>>>>> +
>>>>>  required:
>>>>>    - reg
>>>>>    - compatible
>>>>>    - clocks
>>>>>    - resets
>>>>>
>>>>> +allOf:
>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: aspeed,ast2600-i2cv2
>>>>
>>>> NAK, undocumented compatible.
>>>
>>> Sorry, I should add what kind of document about this compatible?
>>
>> You cannot add new compatibles without documenting them. Documentation
>> is in the form of DT schema and each compatible must be listed (in some
>> way) in compatible property description.
> 
> Sorry, do you mean, I add following in yaml or commit message?

You need to list this in compatibles first.

> 
> This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new register set that have new clock divider option for more flexiable generation. And also have separate i2c controller and target register set for control, patch #2 is i2c controller driver only, patch #3 is add i2c target mode driver.

All this describes driver, not hardware.

> 
> The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
> 
> -Add new clock divider option for more flexible and accurate clock rate generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> -Add support dual pool buffer mode, split 32 bytes pool buffer of each device into 2 x 16 bytes for Tx and Rx individually.
> -Increase DMA buffer size to 4096 bytes and support byte alignment.
> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> -Re-define registers for separating controller and target mode control.
> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.

Does it mean hardware changed on AST2600? Or these are different devices
than aspeed,ast2600-i2c-bus? If this is not a different device, how one
SoC can have two different flavors of same device in the same instance?




Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 27/02/2025 09:19, Ryan Chen wrote:
>>
>>
>>> aspeed,enable-byte:
>>> Force i2c controller use byte mode transfer. the byte mode transfer
>>> will send i2c data each byte by byte, inlcude address read/write.
>>
>> Isn't this standard FIFO mode?
> Yes, it is.
>>
>> Why anyone would need to enable byte mode for given board?
> By default, it is buffer-mode, for performance, I don't want user enable byte-mode, it will increase CPU utilize.
> But someone want to be force enable byte-mode, so I add properties. 
> https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235.2254138-3-ryan_chen@aspeedtech.com/


I don't see the reason why this would be a board property.

I understood need for DMA because it is shared and only some of the
controllers can use it. But why choice between buffer and FIFO depending
on hardware?


>>
>>>
>>>>
>>>>> +      may require a DTS to manually allocate which controller can
>>>>> + use
>>>> DMA mode.
>>>>> +      The "aspeed,enable-dma" property allows control of this.
>>>>> +
>>>>> +      In cases where one the hardware design results in a specific
>>>>> +      controller handling a larger amount of data, a DTS would likely
>>>>> +      enable DMA mode for that one controller.
>>>>> +
>>>>> +  aspeed,enable-byte:
>>>>> +    type: boolean
>>>>> +    description: |
>>>>> +      I2C bus enable byte mode transfer.
>>>>
>>>> No, either this is expressed as lack of dma mode property or if you
>>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
>>>
>>> Thanks suggestion, I will using an enum property like aspeed,transfer-mode
>> instead.
>>>>
>>>>
>>>>
>>>>> +
>>>>> +  aspeed,global-regs:
>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>> +    description: The phandle of i2c global register node.
>>>>
>>>> For what? Same question as usual: do not repeat property name, but
>>>> say what is this used for and what exactly it points to.
>>>>
>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
>>>> you call your device in datasheet?
>>>>
>>> I do descript in cover, should add into the yaml file ?
>>
>>
>> Again, cover letter does not matter. Your hardware must be explained here.
> Will add into commit. 
>>
>>>
>>> aspeed,global-regs:
>>> This global register is needed, global register is setting for new
>>> clock divide control, and new register set control.
>>
>> So this means you implement clock controller via syscon?
> No, it is just mode switch. It also explain in cover. I will add it in commit. 
> The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
>>
>>>
>>>>
>>>>> +
>>>>>  required:
>>>>>    - reg
>>>>>    - compatible
>>>>>    - clocks
>>>>>    - resets
>>>>>
>>>>> +allOf:
>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            const: aspeed,ast2600-i2cv2
>>>>
>>>> NAK, undocumented compatible.
>>>
>>> Sorry, I should add what kind of document about this compatible?
>>
>> You cannot add new compatibles without documenting them. Documentation
>> is in the form of DT schema and each compatible must be listed (in some
>> way) in compatible property description.
> 
> Sorry, do you mean, I add following in yaml or commit message?

You need to list this in compatibles first.

> 
> This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new register set that have new clock divider option for more flexiable generation. And also have separate i2c controller and target register set for control, patch #2 is i2c controller driver only, patch #3 is add i2c target mode driver.

All this describes driver, not hardware.

> 
> The legacy register layout is mix controller/target register control together. The following is add more detail description about new register layout. And new feature set add for register.
> 
> -Add new clock divider option for more flexible and accurate clock rate generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> -Add support dual pool buffer mode, split 32 bytes pool buffer of each device into 2 x 16 bytes for Tx and Rx individually.
> -Increase DMA buffer size to 4096 bytes and support byte alignment.
> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> -Re-define registers for separating controller and target mode control.
> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.

Does it mean hardware changed on AST2600? Or these are different devices
than aspeed,ast2600-i2c-bus? If this is not a different device, how one
SoC can have two different flavors of same device in the same instance?




Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 27/02/2025 09:19, Ryan Chen wrote:
> >>
> >>
> >>> aspeed,enable-byte:
> >>> Force i2c controller use byte mode transfer. the byte mode transfer
> >>> will send i2c data each byte by byte, inlcude address read/write.
> >>
> >> Isn't this standard FIFO mode?
> > Yes, it is.
> >>
> >> Why anyone would need to enable byte mode for given board?
> > By default, it is buffer-mode, for performance, I don't want user enable
> byte-mode, it will increase CPU utilize.
> > But someone want to be force enable byte-mode, so I add properties.
> > https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235
> > .2254138-3-ryan_chen@aspeedtech.com/
> 
> 
> I don't see the reason why this would be a board property.
> 
> I understood need for DMA because it is shared and only some of the
> controllers can use it. But why choice between buffer and FIFO depending on
> hardware?

By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes buffer to transmit data.
Enabling byte mode will increases CPU utilization, so it is not recommended. 
However, some user might require forcing byte mode, so I added this property to allow that.

> 
> 
> >>
> >>>
> >>>>
> >>>>> +      may require a DTS to manually allocate which controller can
> >>>>> + use
> >>>> DMA mode.
> >>>>> +      The "aspeed,enable-dma" property allows control of this.
> >>>>> +
> >>>>> +      In cases where one the hardware design results in a specific
> >>>>> +      controller handling a larger amount of data, a DTS would likely
> >>>>> +      enable DMA mode for that one controller.
> >>>>> +
> >>>>> +  aspeed,enable-byte:
> >>>>> +    type: boolean
> >>>>> +    description: |
> >>>>> +      I2C bus enable byte mode transfer.
> >>>>
> >>>> No, either this is expressed as lack of dma mode property or if you
> >>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
> >>>
> >>> Thanks suggestion, I will using an enum property like
> >>> aspeed,transfer-mode
> >> instead.
> >>>>
> >>>>
> >>>>
> >>>>> +
> >>>>> +  aspeed,global-regs:
> >>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>> +    description: The phandle of i2c global register node.
> >>>>
> >>>> For what? Same question as usual: do not repeat property name, but
> >>>> say what is this used for and what exactly it points to.
> >>>>
> >>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
> >>>> you call your device in datasheet?
> >>>>
> >>> I do descript in cover, should add into the yaml file ?
> >>
> >>
> >> Again, cover letter does not matter. Your hardware must be explained here.
> > Will add into commit.
> >>
> >>>
> >>> aspeed,global-regs:
> >>> This global register is needed, global register is setting for new
> >>> clock divide control, and new register set control.
> >>
> >> So this means you implement clock controller via syscon?
> > No, it is just mode switch. It also explain in cover. I will add it in commit.
> > The legacy register layout is mix controller/target register control together.
> The following is add more detail description about new register layout. And
> new feature set add for register.
> >>
> >>>
> >>>>
> >>>>> +
> >>>>>  required:
> >>>>>    - reg
> >>>>>    - compatible
> >>>>>    - clocks
> >>>>>    - resets
> >>>>>
> >>>>> +allOf:
> >>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>>>> +  - if:
> >>>>> +      properties:
> >>>>> +        compatible:
> >>>>> +          contains:
> >>>>> +            const: aspeed,ast2600-i2cv2
> >>>>
> >>>> NAK, undocumented compatible.
> >>>
> >>> Sorry, I should add what kind of document about this compatible?
> >>
> >> You cannot add new compatibles without documenting them.
> >> Documentation is in the form of DT schema and each compatible must be
> >> listed (in some
> >> way) in compatible property description.
> >
> > Sorry, do you mean, I add following in yaml or commit message?
> 
> You need to list this in compatibles first.

I will add it in compatible in next submit.

	enum:
		-aspeed,ast2600-i2cv2
> 
> >
> > This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new
> register set that have new clock divider option for more flexiable generation.
> And also have separate i2c controller and target register set for control, patch
> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
> 
> All this describes driver, not hardware.

Sorry, the cover letter description the register. I will add int in commit message.

-Add new clock divider option for more flexible and accurate clock rate
generation -Add tCKHighMin timing to guarantee SCL high pulse width.
-Add support dual pool buffer mode, split 32 bytes pool buffer of each
device into 2 x 16 bytes for Tx and Rx individually.
-Increase DMA buffer size to 4096 bytes and support byte alignment.
-Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
-Re-define registers for separating controller and target mode control.
-Support 4 individual DMA buffers for controller Tx and Rx,
target Tx and Rx.

And following is new register set for package transfer sequence.
-New Master operation mode:
 S -> Aw -> P
 S -> Aw -> TxD -> P
 S -> Ar -> RxD -> P
 S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
-Bus SDA lock auto-release capability for new controller DMA command mode.
-Bus auto timeout for new controller/target DMA mode.

The following is two versus register layout.
Old:
{I2CD00}: Function Control Register
{I2CD04}: Clock and AC Timing Control Register
{I2CD08}: Clock and AC Timing Control Register
{I2CD0C}: Interrupt Control Register
{I2CD10}: Interrupt Status Register
{I2CD14}: Command/Status Register
{I2CD18}: Slave Device Address Register
{I2CD1C}: Pool Buffer Control Register
{I2CD20}: Transmit/Receive Byte Buffer Register
{I2CD24}: DMA Mode Buffer Address Register
{I2CD28}: DMA Transfer Length Register
{I2CD2C}: Original DMA Mode Buffer Address Setting
{I2CD30}: Original DMA Transfer Length Setting and Final Status

New Register mode
{I2CC00}: Master/Slave Function Control Register
{I2CC04}: Master/Slave Clock and AC Timing Control Register
{I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
{I2CC0C}: Master/Slave Pool Buffer Control Register
{I2CM10}: Master Interrupt Control Register
{I2CM14}: Master Interrupt Status Register
{I2CM18}: Master Command/Status Register
{I2CM1C}: Master DMA Buffer Length Register
{I2CS20}: Slave~ Interrupt Control Register
{I2CS24}: Slave~ Interrupt Status Register
{I2CS28}: Slave~ Command/Status Register
{I2CS2C}: Slave~ DMA Buffer Length Register
{I2CM30}: Master DMA Mode Tx Buffer Base Address
{I2CM34}: Master DMA Mode Rx Buffer Base Address
{I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
{I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
{I2CS40}: Slave Device Address Register
{I2CM48}: Master DMA Length Status Register
{I2CS4C}: Slave  DMA Length Status Register
{I2CC50}: Current DMA Operating Address Status
{I2CC54}: Current DMA Operating Length  Status

> 
> >
> > The legacy register layout is mix controller/target register control together.
> The following is add more detail description about new register layout. And
> new feature set add for register.
> >
> > -Add new clock divider option for more flexible and accurate clock rate
> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> > -Add support dual pool buffer mode, split 32 bytes pool buffer of each device
> into 2 x 16 bytes for Tx and Rx individually.
> > -Increase DMA buffer size to 4096 bytes and support byte alignment.
> > -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> > -Re-define registers for separating controller and target mode control.
> > -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.
> 
> Does it mean hardware changed on AST2600? 
No Hw change, it is different mode setting will have another mode register setting.
Mode setting is in global register, I will add in next commit message

I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
I2CG00: Device Master/Slave Mode Interrupt Status Register (I2CG0C[3]=0)
I2CG04: Device Slave Mode Interrupt Status Register
I2CG0C: Global Control Register
I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)

> Or these are different devices
> than aspeed,ast2600-i2c-bus? If this is not a different device, how one SoC can
> have two different flavors of same device in the same instance?

When global setting for new, will new register mapping, no setting will keep old register mapping.
> 
> 
> 
> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 27/02/2025 09:19, Ryan Chen wrote:
> >>
> >>
> >>> aspeed,enable-byte:
> >>> Force i2c controller use byte mode transfer. the byte mode transfer
> >>> will send i2c data each byte by byte, inlcude address read/write.
> >>
> >> Isn't this standard FIFO mode?
> > Yes, it is.
> >>
> >> Why anyone would need to enable byte mode for given board?
> > By default, it is buffer-mode, for performance, I don't want user enable
> byte-mode, it will increase CPU utilize.
> > But someone want to be force enable byte-mode, so I add properties.
> > https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235
> > .2254138-3-ryan_chen@aspeedtech.com/
> 
> 
> I don't see the reason why this would be a board property.
> 
> I understood need for DMA because it is shared and only some of the
> controllers can use it. But why choice between buffer and FIFO depending on
> hardware?

By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes buffer to transmit data.
Enabling byte mode will increases CPU utilization, so it is not recommended. 
However, some user might require forcing byte mode, so I added this property to allow that.

> 
> 
> >>
> >>>
> >>>>
> >>>>> +      may require a DTS to manually allocate which controller can
> >>>>> + use
> >>>> DMA mode.
> >>>>> +      The "aspeed,enable-dma" property allows control of this.
> >>>>> +
> >>>>> +      In cases where one the hardware design results in a specific
> >>>>> +      controller handling a larger amount of data, a DTS would likely
> >>>>> +      enable DMA mode for that one controller.
> >>>>> +
> >>>>> +  aspeed,enable-byte:
> >>>>> +    type: boolean
> >>>>> +    description: |
> >>>>> +      I2C bus enable byte mode transfer.
> >>>>
> >>>> No, either this is expressed as lack of dma mode property or if you
> >>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
> >>>
> >>> Thanks suggestion, I will using an enum property like
> >>> aspeed,transfer-mode
> >> instead.
> >>>>
> >>>>
> >>>>
> >>>>> +
> >>>>> +  aspeed,global-regs:
> >>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>> +    description: The phandle of i2c global register node.
> >>>>
> >>>> For what? Same question as usual: do not repeat property name, but
> >>>> say what is this used for and what exactly it points to.
> >>>>
> >>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
> >>>> you call your device in datasheet?
> >>>>
> >>> I do descript in cover, should add into the yaml file ?
> >>
> >>
> >> Again, cover letter does not matter. Your hardware must be explained here.
> > Will add into commit.
> >>
> >>>
> >>> aspeed,global-regs:
> >>> This global register is needed, global register is setting for new
> >>> clock divide control, and new register set control.
> >>
> >> So this means you implement clock controller via syscon?
> > No, it is just mode switch. It also explain in cover. I will add it in commit.
> > The legacy register layout is mix controller/target register control together.
> The following is add more detail description about new register layout. And
> new feature set add for register.
> >>
> >>>
> >>>>
> >>>>> +
> >>>>>  required:
> >>>>>    - reg
> >>>>>    - compatible
> >>>>>    - clocks
> >>>>>    - resets
> >>>>>
> >>>>> +allOf:
> >>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>>>> +  - if:
> >>>>> +      properties:
> >>>>> +        compatible:
> >>>>> +          contains:
> >>>>> +            const: aspeed,ast2600-i2cv2
> >>>>
> >>>> NAK, undocumented compatible.
> >>>
> >>> Sorry, I should add what kind of document about this compatible?
> >>
> >> You cannot add new compatibles without documenting them.
> >> Documentation is in the form of DT schema and each compatible must be
> >> listed (in some
> >> way) in compatible property description.
> >
> > Sorry, do you mean, I add following in yaml or commit message?
> 
> You need to list this in compatibles first.

I will add it in compatible in next submit.

	enum:
		-aspeed,ast2600-i2cv2
> 
> >
> > This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new
> register set that have new clock divider option for more flexiable generation.
> And also have separate i2c controller and target register set for control, patch
> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
> 
> All this describes driver, not hardware.

Sorry, the cover letter description the register. I will add int in commit message.

-Add new clock divider option for more flexible and accurate clock rate
generation -Add tCKHighMin timing to guarantee SCL high pulse width.
-Add support dual pool buffer mode, split 32 bytes pool buffer of each
device into 2 x 16 bytes for Tx and Rx individually.
-Increase DMA buffer size to 4096 bytes and support byte alignment.
-Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
-Re-define registers for separating controller and target mode control.
-Support 4 individual DMA buffers for controller Tx and Rx,
target Tx and Rx.

And following is new register set for package transfer sequence.
-New Master operation mode:
 S -> Aw -> P
 S -> Aw -> TxD -> P
 S -> Ar -> RxD -> P
 S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
-Bus SDA lock auto-release capability for new controller DMA command mode.
-Bus auto timeout for new controller/target DMA mode.

The following is two versus register layout.
Old:
{I2CD00}: Function Control Register
{I2CD04}: Clock and AC Timing Control Register
{I2CD08}: Clock and AC Timing Control Register
{I2CD0C}: Interrupt Control Register
{I2CD10}: Interrupt Status Register
{I2CD14}: Command/Status Register
{I2CD18}: Slave Device Address Register
{I2CD1C}: Pool Buffer Control Register
{I2CD20}: Transmit/Receive Byte Buffer Register
{I2CD24}: DMA Mode Buffer Address Register
{I2CD28}: DMA Transfer Length Register
{I2CD2C}: Original DMA Mode Buffer Address Setting
{I2CD30}: Original DMA Transfer Length Setting and Final Status

New Register mode
{I2CC00}: Master/Slave Function Control Register
{I2CC04}: Master/Slave Clock and AC Timing Control Register
{I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
{I2CC0C}: Master/Slave Pool Buffer Control Register
{I2CM10}: Master Interrupt Control Register
{I2CM14}: Master Interrupt Status Register
{I2CM18}: Master Command/Status Register
{I2CM1C}: Master DMA Buffer Length Register
{I2CS20}: Slave~ Interrupt Control Register
{I2CS24}: Slave~ Interrupt Status Register
{I2CS28}: Slave~ Command/Status Register
{I2CS2C}: Slave~ DMA Buffer Length Register
{I2CM30}: Master DMA Mode Tx Buffer Base Address
{I2CM34}: Master DMA Mode Rx Buffer Base Address
{I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
{I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
{I2CS40}: Slave Device Address Register
{I2CM48}: Master DMA Length Status Register
{I2CS4C}: Slave  DMA Length Status Register
{I2CC50}: Current DMA Operating Address Status
{I2CC54}: Current DMA Operating Length  Status

> 
> >
> > The legacy register layout is mix controller/target register control together.
> The following is add more detail description about new register layout. And
> new feature set add for register.
> >
> > -Add new clock divider option for more flexible and accurate clock rate
> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> > -Add support dual pool buffer mode, split 32 bytes pool buffer of each device
> into 2 x 16 bytes for Tx and Rx individually.
> > -Increase DMA buffer size to 4096 bytes and support byte alignment.
> > -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> > -Re-define registers for separating controller and target mode control.
> > -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.
> 
> Does it mean hardware changed on AST2600? 
No Hw change, it is different mode setting will have another mode register setting.
Mode setting is in global register, I will add in next commit message

I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
I2CG00: Device Master/Slave Mode Interrupt Status Register (I2CG0C[3]=0)
I2CG04: Device Slave Mode Interrupt Status Register
I2CG0C: Global Control Register
I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)

> Or these are different devices
> than aspeed,ast2600-i2c-bus? If this is not a different device, how one SoC can
> have two different flavors of same device in the same instance?

When global setting for new, will new register mapping, no setting will keep old register mapping.
> 
> 
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 05/03/2025 10:35, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 27/02/2025 09:19, Ryan Chen wrote:
>>>>
>>>>
>>>>> aspeed,enable-byte:
>>>>> Force i2c controller use byte mode transfer. the byte mode transfer
>>>>> will send i2c data each byte by byte, inlcude address read/write.
>>>>
>>>> Isn't this standard FIFO mode?
>>> Yes, it is.
>>>>
>>>> Why anyone would need to enable byte mode for given board?
>>> By default, it is buffer-mode, for performance, I don't want user enable
>> byte-mode, it will increase CPU utilize.
>>> But someone want to be force enable byte-mode, so I add properties.
>>> https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235
>>> .2254138-3-ryan_chen@aspeedtech.com/
>>
>>
>> I don't see the reason why this would be a board property.
>>
>> I understood need for DMA because it is shared and only some of the
>> controllers can use it. But why choice between buffer and FIFO depending on
>> hardware?
> 
> By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes buffer to transmit data.
> Enabling byte mode will increases CPU utilization, so it is not recommended. 
> However, some user might require forcing byte mode, so I added this property to allow that.


You are not answering the question. I asked why the choice depends on
hardware and you answer "user might required".

Do you understand that this is about hardware, not user choices?


>>
>>
>>>>
>>>>>
>>>>>>
>>>>>>> +      may require a DTS to manually allocate which controller can
>>>>>>> + use
>>>>>> DMA mode.
>>>>>>> +      The "aspeed,enable-dma" property allows control of this.
>>>>>>> +
>>>>>>> +      In cases where one the hardware design results in a specific
>>>>>>> +      controller handling a larger amount of data, a DTS would likely
>>>>>>> +      enable DMA mode for that one controller.
>>>>>>> +
>>>>>>> +  aspeed,enable-byte:
>>>>>>> +    type: boolean
>>>>>>> +    description: |
>>>>>>> +      I2C bus enable byte mode transfer.
>>>>>>
>>>>>> No, either this is expressed as lack of dma mode property or if you
>>>>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
>>>>>
>>>>> Thanks suggestion, I will using an enum property like
>>>>> aspeed,transfer-mode
>>>> instead.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +  aspeed,global-regs:
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>>>> +    description: The phandle of i2c global register node.
>>>>>>
>>>>>> For what? Same question as usual: do not repeat property name, but
>>>>>> say what is this used for and what exactly it points to.
>>>>>>
>>>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
>>>>>> you call your device in datasheet?
>>>>>>
>>>>> I do descript in cover, should add into the yaml file ?
>>>>
>>>>
>>>> Again, cover letter does not matter. Your hardware must be explained here.
>>> Will add into commit.
>>>>
>>>>>
>>>>> aspeed,global-regs:
>>>>> This global register is needed, global register is setting for new
>>>>> clock divide control, and new register set control.
>>>>
>>>> So this means you implement clock controller via syscon?
>>> No, it is just mode switch. It also explain in cover. I will add it in commit.
>>> The legacy register layout is mix controller/target register control together.
>> The following is add more detail description about new register layout. And
>> new feature set add for register.
>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>>  required:
>>>>>>>    - reg
>>>>>>>    - compatible
>>>>>>>    - clocks
>>>>>>>    - resets
>>>>>>>
>>>>>>> +allOf:
>>>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>>>>>> +  - if:
>>>>>>> +      properties:
>>>>>>> +        compatible:
>>>>>>> +          contains:
>>>>>>> +            const: aspeed,ast2600-i2cv2
>>>>>>
>>>>>> NAK, undocumented compatible.
>>>>>
>>>>> Sorry, I should add what kind of document about this compatible?
>>>>
>>>> You cannot add new compatibles without documenting them.
>>>> Documentation is in the form of DT schema and each compatible must be
>>>> listed (in some
>>>> way) in compatible property description.
>>>
>>> Sorry, do you mean, I add following in yaml or commit message?
>>
>> You need to list this in compatibles first.
> 
> I will add it in compatible in next submit.
> 
> 	enum:
> 		-aspeed,ast2600-i2cv2
>>
>>>
>>> This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new
>> register set that have new clock divider option for more flexiable generation.
>> And also have separate i2c controller and target register set for control, patch
>> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
>>
>> All this describes driver, not hardware.
> 
> Sorry, the cover letter description the register. I will add int in commit message.
> 
> -Add new clock divider option for more flexible and accurate clock rate
> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> -Add support dual pool buffer mode, split 32 bytes pool buffer of each
> device into 2 x 16 bytes for Tx and Rx individually.
> -Increase DMA buffer size to 4096 bytes and support byte alignment.
> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> -Re-define registers for separating controller and target mode control.
> -Support 4 individual DMA buffers for controller Tx and Rx,
> target Tx and Rx.
> 
> And following is new register set for package transfer sequence.
> -New Master operation mode:
>  S -> Aw -> P
>  S -> Aw -> TxD -> P
>  S -> Ar -> RxD -> P
>  S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
> -Bus SDA lock auto-release capability for new controller DMA command mode.
> -Bus auto timeout for new controller/target DMA mode.
> 
> The following is two versus register layout.
> Old:
> {I2CD00}: Function Control Register
> {I2CD04}: Clock and AC Timing Control Register
> {I2CD08}: Clock and AC Timing Control Register
> {I2CD0C}: Interrupt Control Register
> {I2CD10}: Interrupt Status Register
> {I2CD14}: Command/Status Register
> {I2CD18}: Slave Device Address Register
> {I2CD1C}: Pool Buffer Control Register
> {I2CD20}: Transmit/Receive Byte Buffer Register
> {I2CD24}: DMA Mode Buffer Address Register
> {I2CD28}: DMA Transfer Length Register
> {I2CD2C}: Original DMA Mode Buffer Address Setting
> {I2CD30}: Original DMA Transfer Length Setting and Final Status
> 
> New Register mode
> {I2CC00}: Master/Slave Function Control Register
> {I2CC04}: Master/Slave Clock and AC Timing Control Register
> {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
> {I2CC0C}: Master/Slave Pool Buffer Control Register
> {I2CM10}: Master Interrupt Control Register
> {I2CM14}: Master Interrupt Status Register
> {I2CM18}: Master Command/Status Register
> {I2CM1C}: Master DMA Buffer Length Register
> {I2CS20}: Slave~ Interrupt Control Register
> {I2CS24}: Slave~ Interrupt Status Register
> {I2CS28}: Slave~ Command/Status Register
> {I2CS2C}: Slave~ DMA Buffer Length Register
> {I2CM30}: Master DMA Mode Tx Buffer Base Address
> {I2CM34}: Master DMA Mode Rx Buffer Base Address
> {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
> {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
> {I2CS40}: Slave Device Address Register
> {I2CM48}: Master DMA Length Status Register
> {I2CS4C}: Slave  DMA Length Status Register
> {I2CC50}: Current DMA Operating Address Status
> {I2CC54}: Current DMA Operating Length  Status

I don't understand anything from that and how is this relevant to our
discussion.

> 
>>
>>>
>>> The legacy register layout is mix controller/target register control together.
>> The following is add more detail description about new register layout. And
>> new feature set add for register.
>>>
>>> -Add new clock divider option for more flexible and accurate clock rate
>> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
>>> -Add support dual pool buffer mode, split 32 bytes pool buffer of each device
>> into 2 x 16 bytes for Tx and Rx individually.
>>> -Increase DMA buffer size to 4096 bytes and support byte alignment.
>>> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
>>> -Re-define registers for separating controller and target mode control.
>>> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.
>>
>> Does it mean hardware changed on AST2600? 
> No Hw change, it is different mode setting will have another mode register setting.
> Mode setting is in global register, I will add in next commit message

Neither this.

So it seems you describe already existing and documented I2C, but for
some reason you want second compatible. The problem is that you do not
provide reason from the point of view of bindings.

To summarize: what your users want - don't care. Start properly
describing hardware and your SoC.


> 
> I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
> I2CG00: Device Master/Slave Mode Interrupt Status Register (I2CG0C[3]=0)
> I2CG04: Device Slave Mode Interrupt Status Register
> I2CG0C: Global Control Register
> I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)
> 
>> Or these are different devices
>> than aspeed,ast2600-i2c-bus? If this is not a different device, how one SoC can
>> have two different flavors of same device in the same instance?
> 
> When global setting for new, will new register mapping, no setting will keep old register mapping.


I cannot parse this.


Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 05/03/2025 10:35, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 27/02/2025 09:19, Ryan Chen wrote:
>>>>
>>>>
>>>>> aspeed,enable-byte:
>>>>> Force i2c controller use byte mode transfer. the byte mode transfer
>>>>> will send i2c data each byte by byte, inlcude address read/write.
>>>>
>>>> Isn't this standard FIFO mode?
>>> Yes, it is.
>>>>
>>>> Why anyone would need to enable byte mode for given board?
>>> By default, it is buffer-mode, for performance, I don't want user enable
>> byte-mode, it will increase CPU utilize.
>>> But someone want to be force enable byte-mode, so I add properties.
>>> https://patchwork.ozlabs.org/project/linux-aspeed/patch/20241007035235
>>> .2254138-3-ryan_chen@aspeedtech.com/
>>
>>
>> I don't see the reason why this would be a board property.
>>
>> I understood need for DMA because it is shared and only some of the
>> controllers can use it. But why choice between buffer and FIFO depending on
>> hardware?
> 
> By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes buffer to transmit data.
> Enabling byte mode will increases CPU utilization, so it is not recommended. 
> However, some user might require forcing byte mode, so I added this property to allow that.


You are not answering the question. I asked why the choice depends on
hardware and you answer "user might required".

Do you understand that this is about hardware, not user choices?


>>
>>
>>>>
>>>>>
>>>>>>
>>>>>>> +      may require a DTS to manually allocate which controller can
>>>>>>> + use
>>>>>> DMA mode.
>>>>>>> +      The "aspeed,enable-dma" property allows control of this.
>>>>>>> +
>>>>>>> +      In cases where one the hardware design results in a specific
>>>>>>> +      controller handling a larger amount of data, a DTS would likely
>>>>>>> +      enable DMA mode for that one controller.
>>>>>>> +
>>>>>>> +  aspeed,enable-byte:
>>>>>>> +    type: boolean
>>>>>>> +    description: |
>>>>>>> +      I2C bus enable byte mode transfer.
>>>>>>
>>>>>> No, either this is expressed as lack of dma mode property or if you
>>>>>> have three modes, then rather some enum (aspeed,transfer-mode ?)
>>>>>
>>>>> Thanks suggestion, I will using an enum property like
>>>>> aspeed,transfer-mode
>>>> instead.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +  aspeed,global-regs:
>>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>>>>>> +    description: The phandle of i2c global register node.
>>>>>>
>>>>>> For what? Same question as usual: do not repeat property name, but
>>>>>> say what is this used for and what exactly it points to.
>>>>>>
>>>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is how
>>>>>> you call your device in datasheet?
>>>>>>
>>>>> I do descript in cover, should add into the yaml file ?
>>>>
>>>>
>>>> Again, cover letter does not matter. Your hardware must be explained here.
>>> Will add into commit.
>>>>
>>>>>
>>>>> aspeed,global-regs:
>>>>> This global register is needed, global register is setting for new
>>>>> clock divide control, and new register set control.
>>>>
>>>> So this means you implement clock controller via syscon?
>>> No, it is just mode switch. It also explain in cover. I will add it in commit.
>>> The legacy register layout is mix controller/target register control together.
>> The following is add more detail description about new register layout. And
>> new feature set add for register.
>>>>
>>>>>
>>>>>>
>>>>>>> +
>>>>>>>  required:
>>>>>>>    - reg
>>>>>>>    - compatible
>>>>>>>    - clocks
>>>>>>>    - resets
>>>>>>>
>>>>>>> +allOf:
>>>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
>>>>>>> +  - if:
>>>>>>> +      properties:
>>>>>>> +        compatible:
>>>>>>> +          contains:
>>>>>>> +            const: aspeed,ast2600-i2cv2
>>>>>>
>>>>>> NAK, undocumented compatible.
>>>>>
>>>>> Sorry, I should add what kind of document about this compatible?
>>>>
>>>> You cannot add new compatibles without documenting them.
>>>> Documentation is in the form of DT schema and each compatible must be
>>>> listed (in some
>>>> way) in compatible property description.
>>>
>>> Sorry, do you mean, I add following in yaml or commit message?
>>
>> You need to list this in compatibles first.
> 
> I will add it in compatible in next submit.
> 
> 	enum:
> 		-aspeed,ast2600-i2cv2
>>
>>>
>>> This series add AST2600 i2cv2 new register set driver. The i2cv2 driver is new
>> register set that have new clock divider option for more flexiable generation.
>> And also have separate i2c controller and target register set for control, patch
>> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
>>
>> All this describes driver, not hardware.
> 
> Sorry, the cover letter description the register. I will add int in commit message.
> 
> -Add new clock divider option for more flexible and accurate clock rate
> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> -Add support dual pool buffer mode, split 32 bytes pool buffer of each
> device into 2 x 16 bytes for Tx and Rx individually.
> -Increase DMA buffer size to 4096 bytes and support byte alignment.
> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> -Re-define registers for separating controller and target mode control.
> -Support 4 individual DMA buffers for controller Tx and Rx,
> target Tx and Rx.
> 
> And following is new register set for package transfer sequence.
> -New Master operation mode:
>  S -> Aw -> P
>  S -> Aw -> TxD -> P
>  S -> Ar -> RxD -> P
>  S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
> -Bus SDA lock auto-release capability for new controller DMA command mode.
> -Bus auto timeout for new controller/target DMA mode.
> 
> The following is two versus register layout.
> Old:
> {I2CD00}: Function Control Register
> {I2CD04}: Clock and AC Timing Control Register
> {I2CD08}: Clock and AC Timing Control Register
> {I2CD0C}: Interrupt Control Register
> {I2CD10}: Interrupt Status Register
> {I2CD14}: Command/Status Register
> {I2CD18}: Slave Device Address Register
> {I2CD1C}: Pool Buffer Control Register
> {I2CD20}: Transmit/Receive Byte Buffer Register
> {I2CD24}: DMA Mode Buffer Address Register
> {I2CD28}: DMA Transfer Length Register
> {I2CD2C}: Original DMA Mode Buffer Address Setting
> {I2CD30}: Original DMA Transfer Length Setting and Final Status
> 
> New Register mode
> {I2CC00}: Master/Slave Function Control Register
> {I2CC04}: Master/Slave Clock and AC Timing Control Register
> {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
> {I2CC0C}: Master/Slave Pool Buffer Control Register
> {I2CM10}: Master Interrupt Control Register
> {I2CM14}: Master Interrupt Status Register
> {I2CM18}: Master Command/Status Register
> {I2CM1C}: Master DMA Buffer Length Register
> {I2CS20}: Slave~ Interrupt Control Register
> {I2CS24}: Slave~ Interrupt Status Register
> {I2CS28}: Slave~ Command/Status Register
> {I2CS2C}: Slave~ DMA Buffer Length Register
> {I2CM30}: Master DMA Mode Tx Buffer Base Address
> {I2CM34}: Master DMA Mode Rx Buffer Base Address
> {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
> {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
> {I2CS40}: Slave Device Address Register
> {I2CM48}: Master DMA Length Status Register
> {I2CS4C}: Slave  DMA Length Status Register
> {I2CC50}: Current DMA Operating Address Status
> {I2CC54}: Current DMA Operating Length  Status

I don't understand anything from that and how is this relevant to our
discussion.

> 
>>
>>>
>>> The legacy register layout is mix controller/target register control together.
>> The following is add more detail description about new register layout. And
>> new feature set add for register.
>>>
>>> -Add new clock divider option for more flexible and accurate clock rate
>> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
>>> -Add support dual pool buffer mode, split 32 bytes pool buffer of each device
>> into 2 x 16 bytes for Tx and Rx individually.
>>> -Increase DMA buffer size to 4096 bytes and support byte alignment.
>>> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
>>> -Re-define registers for separating controller and target mode control.
>>> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and Rx.
>>
>> Does it mean hardware changed on AST2600? 
> No Hw change, it is different mode setting will have another mode register setting.
> Mode setting is in global register, I will add in next commit message

Neither this.

So it seems you describe already existing and documented I2C, but for
some reason you want second compatible. The problem is that you do not
provide reason from the point of view of bindings.

To summarize: what your users want - don't care. Start properly
describing hardware and your SoC.


> 
> I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
> I2CG00: Device Master/Slave Mode Interrupt Status Register (I2CG0C[3]=0)
> I2CG04: Device Slave Mode Interrupt Status Register
> I2CG0C: Global Control Register
> I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)
> 
>> Or these are different devices
>> than aspeed,ast2600-i2c-bus? If this is not a different device, how one SoC can
>> have two different flavors of same device in the same instance?
> 
> When global setting for new, will new register mapping, no setting will keep old register mapping.


I cannot parse this.


Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 05/03/2025 10:35, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 27/02/2025 09:19, Ryan Chen wrote:
> >>>>
> >>>>
> >>>>> aspeed,enable-byte:
> >>>>> Force i2c controller use byte mode transfer. the byte mode
> >>>>> transfer will send i2c data each byte by byte, inlcude address
> read/write.
> >>>>
> >>>> Isn't this standard FIFO mode?
> >>> Yes, it is.
> >>>>
> >>>> Why anyone would need to enable byte mode for given board?
> >>> By default, it is buffer-mode, for performance, I don't want user
> >>> enable
> >> byte-mode, it will increase CPU utilize.
> >>> But someone want to be force enable byte-mode, so I add properties.
> >>> https://patchwork.ozlabs.org/project/linux-aspeed/patch/202410070352
> >>> 35 .2254138-3-ryan_chen@aspeedtech.com/
> >>
> >>
> >> I don't see the reason why this would be a board property.
> >>
> >> I understood need for DMA because it is shared and only some of the
> >> controllers can use it. But why choice between buffer and FIFO
> >> depending on hardware?
> >
> > By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes
> buffer to transmit data.
> > Enabling byte mode will increases CPU utilization, so it is not recommended.
> > However, some user might require forcing byte mode, so I added this
> property to allow that.
> 
> 
> You are not answering the question. I asked why the choice depends on
> hardware and you answer "user might required".
> 
> Do you understand that this is about hardware, not user choices?

The AST2600 I2C controller support 3 modes. 1. Byte mode, 2.buffer mode. 3.dma mode.
Which Buffer vs byte mode. is buffer mode have 16 bytes buffer for each i2c transition.
Byte mode is 1 by 1 transfer.
So, my original submit patch is only for buffer/dma mode selection in property.
But someone in review feedback want to use byte mode, so I add byte mode property now.
If not acceptable, I can keep buffer/dma two mode selection in property.

> 
> 
> >>
> >>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> +      may require a DTS to manually allocate which controller
> >>>>>>> + can use
> >>>>>> DMA mode.
> >>>>>>> +      The "aspeed,enable-dma" property allows control of this.
> >>>>>>> +
> >>>>>>> +      In cases where one the hardware design results in a specific
> >>>>>>> +      controller handling a larger amount of data, a DTS would
> likely
> >>>>>>> +      enable DMA mode for that one controller.
> >>>>>>> +
> >>>>>>> +  aspeed,enable-byte:
> >>>>>>> +    type: boolean
> >>>>>>> +    description: |
> >>>>>>> +      I2C bus enable byte mode transfer.
> >>>>>>
> >>>>>> No, either this is expressed as lack of dma mode property or if
> >>>>>> you have three modes, then rather some enum (aspeed,transfer-mode
> >>>>>> ?)
> >>>>>
> >>>>> Thanks suggestion, I will using an enum property like
> >>>>> aspeed,transfer-mode
> >>>> instead.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> +
> >>>>>>> +  aspeed,global-regs:
> >>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>>> +    description: The phandle of i2c global register node.
> >>>>>>
> >>>>>> For what? Same question as usual: do not repeat property name,
> >>>>>> but say what is this used for and what exactly it points to.
> >>>>>>
> >>>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is
> >>>>>> how you call your device in datasheet?
> >>>>>>
> >>>>> I do descript in cover, should add into the yaml file ?
> >>>>
> >>>>
> >>>> Again, cover letter does not matter. Your hardware must be explained
> here.
> >>> Will add into commit.
> >>>>
> >>>>>
> >>>>> aspeed,global-regs:
> >>>>> This global register is needed, global register is setting for new
> >>>>> clock divide control, and new register set control.
> >>>>
> >>>> So this means you implement clock controller via syscon?
> >>> No, it is just mode switch. It also explain in cover. I will add it in commit.
> >>> The legacy register layout is mix controller/target register control together.
> >> The following is add more detail description about new register
> >> layout. And new feature set add for register.
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> +
> >>>>>>>  required:
> >>>>>>>    - reg
> >>>>>>>    - compatible
> >>>>>>>    - clocks
> >>>>>>>    - resets
> >>>>>>>
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>>>>>> +  - if:
> >>>>>>> +      properties:
> >>>>>>> +        compatible:
> >>>>>>> +          contains:
> >>>>>>> +            const: aspeed,ast2600-i2cv2
> >>>>>>
> >>>>>> NAK, undocumented compatible.
> >>>>>
> >>>>> Sorry, I should add what kind of document about this compatible?
> >>>>
> >>>> You cannot add new compatibles without documenting them.
> >>>> Documentation is in the form of DT schema and each compatible must
> >>>> be listed (in some
> >>>> way) in compatible property description.
> >>>
> >>> Sorry, do you mean, I add following in yaml or commit message?
> >>
> >> You need to list this in compatibles first.
> >
> > I will add it in compatible in next submit.
> >
> > 	enum:
> > 		-aspeed,ast2600-i2cv2
> >>
> >>>
> >>> This series add AST2600 i2cv2 new register set driver. The i2cv2
> >>> driver is new
> >> register set that have new clock divider option for more flexiable
> generation.
> >> And also have separate i2c controller and target register set for
> >> control, patch
> >> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
> >>
> >> All this describes driver, not hardware.
> >
> > Sorry, the cover letter description the register. I will add int in commit
> message.
> >
> > -Add new clock divider option for more flexible and accurate clock
> > rate generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> > -Add support dual pool buffer mode, split 32 bytes pool buffer of each
> > device into 2 x 16 bytes for Tx and Rx individually.
> > -Increase DMA buffer size to 4096 bytes and support byte alignment.
> > -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> > -Re-define registers for separating controller and target mode control.
> > -Support 4 individual DMA buffers for controller Tx and Rx, target Tx
> > and Rx.
> >
> > And following is new register set for package transfer sequence.
> > -New Master operation mode:
> >  S -> Aw -> P
> >  S -> Aw -> TxD -> P
> >  S -> Ar -> RxD -> P
> >  S -> Aw -> RxD -> Sr -> Ar -> TxD -> P -Bus SDA lock auto-release
> > capability for new controller DMA command mode.
> > -Bus auto timeout for new controller/target DMA mode.
> >
> > The following is two versus register layout.
> > Old:
> > {I2CD00}: Function Control Register
> > {I2CD04}: Clock and AC Timing Control Register
> > {I2CD08}: Clock and AC Timing Control Register
> > {I2CD0C}: Interrupt Control Register
> > {I2CD10}: Interrupt Status Register
> > {I2CD14}: Command/Status Register
> > {I2CD18}: Slave Device Address Register
> > {I2CD1C}: Pool Buffer Control Register
> > {I2CD20}: Transmit/Receive Byte Buffer Register
> > {I2CD24}: DMA Mode Buffer Address Register
> > {I2CD28}: DMA Transfer Length Register
> > {I2CD2C}: Original DMA Mode Buffer Address Setting
> > {I2CD30}: Original DMA Transfer Length Setting and Final Status
> >
> > New Register mode
> > {I2CC00}: Master/Slave Function Control Register
> > {I2CC04}: Master/Slave Clock and AC Timing Control Register
> > {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
> > {I2CC0C}: Master/Slave Pool Buffer Control Register
> > {I2CM10}: Master Interrupt Control Register
> > {I2CM14}: Master Interrupt Status Register
> > {I2CM18}: Master Command/Status Register
> > {I2CM1C}: Master DMA Buffer Length Register
> > {I2CS20}: Slave~ Interrupt Control Register
> > {I2CS24}: Slave~ Interrupt Status Register
> > {I2CS28}: Slave~ Command/Status Register
> > {I2CS2C}: Slave~ DMA Buffer Length Register
> > {I2CM30}: Master DMA Mode Tx Buffer Base Address
> > {I2CM34}: Master DMA Mode Rx Buffer Base Address
> > {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
> > {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
> > {I2CS40}: Slave Device Address Register
> > {I2CM48}: Master DMA Length Status Register
> > {I2CS4C}: Slave  DMA Length Status Register
> > {I2CC50}: Current DMA Operating Address Status
> > {I2CC54}: Current DMA Operating Length  Status
> 
> I don't understand anything from that and how is this relevant to our
> discussion.

Sorry, I don't catch your wanted. 
I assume you want to know new mode register set compare original register set.
> 
> >
> >>
> >>>
> >>> The legacy register layout is mix controller/target register control together.
> >> The following is add more detail description about new register
> >> layout. And new feature set add for register.
> >>>
> >>> -Add new clock divider option for more flexible and accurate clock
> >>> rate
> >> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> >>> -Add support dual pool buffer mode, split 32 bytes pool buffer of
> >>> each device
> >> into 2 x 16 bytes for Tx and Rx individually.
> >>> -Increase DMA buffer size to 4096 bytes and support byte alignment.
> >>> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> >>> -Re-define registers for separating controller and target mode control.
> >>> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and
> Rx.
> >>
> >> Does it mean hardware changed on AST2600?
> > No Hw change, it is different mode setting will have another mode register
> setting.
> > Mode setting is in global register, I will add in next commit message
> 
> Neither this.
> 
> So it seems you describe already existing and documented I2C, but for some
> reason you want second compatible. The problem is that you do not provide
> reason from the point of view of bindings.
> 
> To summarize: what your users want - don't care. Start properly describing
> hardware and your SoC.

OK, for ast2600 i2c controller have two register mode setting.
One, I call it is old register setting, that is right now i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus",
And there have a global register that can set i2c controller as new mode register set. 
That I am going to drive. That I post is all register in new an old register list.

For example, 
Global register [2] = 0 => i2c present as old register set
Global register [2] = 1 => i2c present as new register set
> 
> 
> >
> > I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
> > I2CG00: Device Master/Slave Mode Interrupt Status Register
> > (I2CG0C[3]=0)
> > I2CG04: Device Slave Mode Interrupt Status Register
> > I2CG0C: Global Control Register
> > I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)
> >
> >> Or these are different devices
> >> than aspeed,ast2600-i2c-bus? If this is not a different device, how
> >> one SoC can have two different flavors of same device in the same instance?
> >
> > When global setting for new, will new register mapping, no setting will keep
> old register mapping.
> 
> 
> I cannot parse this.
> 
> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 05/03/2025 10:35, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 27/02/2025 09:19, Ryan Chen wrote:
> >>>>
> >>>>
> >>>>> aspeed,enable-byte:
> >>>>> Force i2c controller use byte mode transfer. the byte mode
> >>>>> transfer will send i2c data each byte by byte, inlcude address
> read/write.
> >>>>
> >>>> Isn't this standard FIFO mode?
> >>> Yes, it is.
> >>>>
> >>>> Why anyone would need to enable byte mode for given board?
> >>> By default, it is buffer-mode, for performance, I don't want user
> >>> enable
> >> byte-mode, it will increase CPU utilize.
> >>> But someone want to be force enable byte-mode, so I add properties.
> >>> https://patchwork.ozlabs.org/project/linux-aspeed/patch/202410070352
> >>> 35 .2254138-3-ryan_chen@aspeedtech.com/
> >>
> >>
> >> I don't see the reason why this would be a board property.
> >>
> >> I understood need for DMA because it is shared and only some of the
> >> controllers can use it. But why choice between buffer and FIFO
> >> depending on hardware?
> >
> > By default, the I2C controller runs in buffer mode. Each i2c bus have 16bytes
> buffer to transmit data.
> > Enabling byte mode will increases CPU utilization, so it is not recommended.
> > However, some user might require forcing byte mode, so I added this
> property to allow that.
> 
> 
> You are not answering the question. I asked why the choice depends on
> hardware and you answer "user might required".
> 
> Do you understand that this is about hardware, not user choices?

The AST2600 I2C controller support 3 modes. 1. Byte mode, 2.buffer mode. 3.dma mode.
Which Buffer vs byte mode. is buffer mode have 16 bytes buffer for each i2c transition.
Byte mode is 1 by 1 transfer.
So, my original submit patch is only for buffer/dma mode selection in property.
But someone in review feedback want to use byte mode, so I add byte mode property now.
If not acceptable, I can keep buffer/dma two mode selection in property.

> 
> 
> >>
> >>
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> +      may require a DTS to manually allocate which controller
> >>>>>>> + can use
> >>>>>> DMA mode.
> >>>>>>> +      The "aspeed,enable-dma" property allows control of this.
> >>>>>>> +
> >>>>>>> +      In cases where one the hardware design results in a specific
> >>>>>>> +      controller handling a larger amount of data, a DTS would
> likely
> >>>>>>> +      enable DMA mode for that one controller.
> >>>>>>> +
> >>>>>>> +  aspeed,enable-byte:
> >>>>>>> +    type: boolean
> >>>>>>> +    description: |
> >>>>>>> +      I2C bus enable byte mode transfer.
> >>>>>>
> >>>>>> No, either this is expressed as lack of dma mode property or if
> >>>>>> you have three modes, then rather some enum (aspeed,transfer-mode
> >>>>>> ?)
> >>>>>
> >>>>> Thanks suggestion, I will using an enum property like
> >>>>> aspeed,transfer-mode
> >>>> instead.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> +
> >>>>>>> +  aspeed,global-regs:
> >>>>>>> +    $ref: /schemas/types.yaml#/definitions/phandle
> >>>>>>> +    description: The phandle of i2c global register node.
> >>>>>>
> >>>>>> For what? Same question as usual: do not repeat property name,
> >>>>>> but say what is this used for and what exactly it points to.
> >>>>>>
> >>>>>> s/i2c/I2C/ but then what is "I2C global register node"? This is
> >>>>>> how you call your device in datasheet?
> >>>>>>
> >>>>> I do descript in cover, should add into the yaml file ?
> >>>>
> >>>>
> >>>> Again, cover letter does not matter. Your hardware must be explained
> here.
> >>> Will add into commit.
> >>>>
> >>>>>
> >>>>> aspeed,global-regs:
> >>>>> This global register is needed, global register is setting for new
> >>>>> clock divide control, and new register set control.
> >>>>
> >>>> So this means you implement clock controller via syscon?
> >>> No, it is just mode switch. It also explain in cover. I will add it in commit.
> >>> The legacy register layout is mix controller/target register control together.
> >> The following is add more detail description about new register
> >> layout. And new feature set add for register.
> >>>>
> >>>>>
> >>>>>>
> >>>>>>> +
> >>>>>>>  required:
> >>>>>>>    - reg
> >>>>>>>    - compatible
> >>>>>>>    - clocks
> >>>>>>>    - resets
> >>>>>>>
> >>>>>>> +allOf:
> >>>>>>> +  - $ref: /schemas/i2c/i2c-controller.yaml#
> >>>>>>> +  - if:
> >>>>>>> +      properties:
> >>>>>>> +        compatible:
> >>>>>>> +          contains:
> >>>>>>> +            const: aspeed,ast2600-i2cv2
> >>>>>>
> >>>>>> NAK, undocumented compatible.
> >>>>>
> >>>>> Sorry, I should add what kind of document about this compatible?
> >>>>
> >>>> You cannot add new compatibles without documenting them.
> >>>> Documentation is in the form of DT schema and each compatible must
> >>>> be listed (in some
> >>>> way) in compatible property description.
> >>>
> >>> Sorry, do you mean, I add following in yaml or commit message?
> >>
> >> You need to list this in compatibles first.
> >
> > I will add it in compatible in next submit.
> >
> > 	enum:
> > 		-aspeed,ast2600-i2cv2
> >>
> >>>
> >>> This series add AST2600 i2cv2 new register set driver. The i2cv2
> >>> driver is new
> >> register set that have new clock divider option for more flexiable
> generation.
> >> And also have separate i2c controller and target register set for
> >> control, patch
> >> #2 is i2c controller driver only, patch #3 is add i2c target mode driver.
> >>
> >> All this describes driver, not hardware.
> >
> > Sorry, the cover letter description the register. I will add int in commit
> message.
> >
> > -Add new clock divider option for more flexible and accurate clock
> > rate generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> > -Add support dual pool buffer mode, split 32 bytes pool buffer of each
> > device into 2 x 16 bytes for Tx and Rx individually.
> > -Increase DMA buffer size to 4096 bytes and support byte alignment.
> > -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> > -Re-define registers for separating controller and target mode control.
> > -Support 4 individual DMA buffers for controller Tx and Rx, target Tx
> > and Rx.
> >
> > And following is new register set for package transfer sequence.
> > -New Master operation mode:
> >  S -> Aw -> P
> >  S -> Aw -> TxD -> P
> >  S -> Ar -> RxD -> P
> >  S -> Aw -> RxD -> Sr -> Ar -> TxD -> P -Bus SDA lock auto-release
> > capability for new controller DMA command mode.
> > -Bus auto timeout for new controller/target DMA mode.
> >
> > The following is two versus register layout.
> > Old:
> > {I2CD00}: Function Control Register
> > {I2CD04}: Clock and AC Timing Control Register
> > {I2CD08}: Clock and AC Timing Control Register
> > {I2CD0C}: Interrupt Control Register
> > {I2CD10}: Interrupt Status Register
> > {I2CD14}: Command/Status Register
> > {I2CD18}: Slave Device Address Register
> > {I2CD1C}: Pool Buffer Control Register
> > {I2CD20}: Transmit/Receive Byte Buffer Register
> > {I2CD24}: DMA Mode Buffer Address Register
> > {I2CD28}: DMA Transfer Length Register
> > {I2CD2C}: Original DMA Mode Buffer Address Setting
> > {I2CD30}: Original DMA Transfer Length Setting and Final Status
> >
> > New Register mode
> > {I2CC00}: Master/Slave Function Control Register
> > {I2CC04}: Master/Slave Clock and AC Timing Control Register
> > {I2CC08}: Master/Slave Transmit/Receive Byte Buffer Register
> > {I2CC0C}: Master/Slave Pool Buffer Control Register
> > {I2CM10}: Master Interrupt Control Register
> > {I2CM14}: Master Interrupt Status Register
> > {I2CM18}: Master Command/Status Register
> > {I2CM1C}: Master DMA Buffer Length Register
> > {I2CS20}: Slave~ Interrupt Control Register
> > {I2CS24}: Slave~ Interrupt Status Register
> > {I2CS28}: Slave~ Command/Status Register
> > {I2CS2C}: Slave~ DMA Buffer Length Register
> > {I2CM30}: Master DMA Mode Tx Buffer Base Address
> > {I2CM34}: Master DMA Mode Rx Buffer Base Address
> > {I2CS38}: Slave~ DMA Mode Tx Buffer Base Address
> > {I2CS3C}: Slave~ DMA Mode Rx Buffer Base Address
> > {I2CS40}: Slave Device Address Register
> > {I2CM48}: Master DMA Length Status Register
> > {I2CS4C}: Slave  DMA Length Status Register
> > {I2CC50}: Current DMA Operating Address Status
> > {I2CC54}: Current DMA Operating Length  Status
> 
> I don't understand anything from that and how is this relevant to our
> discussion.

Sorry, I don't catch your wanted. 
I assume you want to know new mode register set compare original register set.
> 
> >
> >>
> >>>
> >>> The legacy register layout is mix controller/target register control together.
> >> The following is add more detail description about new register
> >> layout. And new feature set add for register.
> >>>
> >>> -Add new clock divider option for more flexible and accurate clock
> >>> rate
> >> generation -Add tCKHighMin timing to guarantee SCL high pulse width.
> >>> -Add support dual pool buffer mode, split 32 bytes pool buffer of
> >>> each device
> >> into 2 x 16 bytes for Tx and Rx individually.
> >>> -Increase DMA buffer size to 4096 bytes and support byte alignment.
> >>> -Re-define the base address of BUS1 ~ BUS16 and Pool buffer.
> >>> -Re-define registers for separating controller and target mode control.
> >>> -Support 4 individual DMA buffers for controller Tx and Rx, target Tx and
> Rx.
> >>
> >> Does it mean hardware changed on AST2600?
> > No Hw change, it is different mode setting will have another mode register
> setting.
> > Mode setting is in global register, I will add in next commit message
> 
> Neither this.
> 
> So it seems you describe already existing and documented I2C, but for some
> reason you want second compatible. The problem is that you do not provide
> reason from the point of view of bindings.
> 
> To summarize: what your users want - don't care. Start properly describing
> hardware and your SoC.

OK, for ast2600 i2c controller have two register mode setting.
One, I call it is old register setting, that is right now i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus",
And there have a global register that can set i2c controller as new mode register set. 
That I am going to drive. That I post is all register in new an old register list.

For example, 
Global register [2] = 0 => i2c present as old register set
Global register [2] = 1 => i2c present as new register set
> 
> 
> >
> > I2CG00: Device Master Mode Interrupt Status Register (I2CG0C[3]=1)
> > I2CG00: Device Master/Slave Mode Interrupt Status Register
> > (I2CG0C[3]=0)
> > I2CG04: Device Slave Mode Interrupt Status Register
> > I2CG0C: Global Control Register
> > I2CG10: New Clock Divider Control Register (I2CG0C[1] = 1)
> >
> >> Or these are different devices
> >> than aspeed,ast2600-i2c-bus? If this is not a different device, how
> >> one SoC can have two different flavors of same device in the same instance?
> >
> > When global setting for new, will new register mapping, no setting will keep
> old register mapping.
> 
> 
> I cannot parse this.
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 17/03/2025 10:21, Ryan Chen wrote:
>> Neither this.
>>
>> So it seems you describe already existing and documented I2C, but for some
>> reason you want second compatible. The problem is that you do not provide
>> reason from the point of view of bindings.
>>
>> To summarize: what your users want - don't care. Start properly describing
>> hardware and your SoC.
> 
> OK, for ast2600 i2c controller have two register mode setting.
> One, I call it is old register setting, that is right now i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus",
> And there have a global register that can set i2c controller as new mode register set. 
> That I am going to drive. That I post is all register in new an old register list.
> 
> For example, 
> Global register [2] = 0 => i2c present as old register set
> Global register [2] = 1 => i2c present as new register set
It's the same device though, so the same compatible.

Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 17/03/2025 10:21, Ryan Chen wrote:
>> Neither this.
>>
>> So it seems you describe already existing and documented I2C, but for some
>> reason you want second compatible. The problem is that you do not provide
>> reason from the point of view of bindings.
>>
>> To summarize: what your users want - don't care. Start properly describing
>> hardware and your SoC.
> 
> OK, for ast2600 i2c controller have two register mode setting.
> One, I call it is old register setting, that is right now i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus",
> And there have a global register that can set i2c controller as new mode register set. 
> That I am going to drive. That I post is all register in new an old register list.
> 
> For example, 
> Global register [2] = 0 => i2c present as old register set
> Global register [2] = 1 => i2c present as new register set
It's the same device though, so the same compatible.

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 17/03/2025 10:21, Ryan Chen wrote:
> >> Neither this.
> >>
> >> So it seems you describe already existing and documented I2C, but for
> >> some reason you want second compatible. The problem is that you do
> >> not provide reason from the point of view of bindings.
> >>
> >> To summarize: what your users want - don't care. Start properly
> >> describing hardware and your SoC.
> >
> > OK, for ast2600 i2c controller have two register mode setting.
> > One, I call it is old register setting, that is right now i2c-aspeed.c
> > .compatible = "aspeed,ast2600-i2c-bus", And there have a global register
> that can set i2c controller as new mode register set.
> > That I am going to drive. That I post is all register in new an old register list.
> >
> > For example,
> > Global register [2] = 0 => i2c present as old register set Global
> > register [2] = 1 => i2c present as new register set
> It's the same device though, so the same compatible.

Sorry, it is different design, and it share the same register space.
So that the reason add new compatible "aspeed,ast2600-i2cv2" for this driver.
It is different register layout.


> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 17/03/2025 10:21, Ryan Chen wrote:
> >> Neither this.
> >>
> >> So it seems you describe already existing and documented I2C, but for
> >> some reason you want second compatible. The problem is that you do
> >> not provide reason from the point of view of bindings.
> >>
> >> To summarize: what your users want - don't care. Start properly
> >> describing hardware and your SoC.
> >
> > OK, for ast2600 i2c controller have two register mode setting.
> > One, I call it is old register setting, that is right now i2c-aspeed.c
> > .compatible = "aspeed,ast2600-i2c-bus", And there have a global register
> that can set i2c controller as new mode register set.
> > That I am going to drive. That I post is all register in new an old register list.
> >
> > For example,
> > Global register [2] = 0 => i2c present as old register set Global
> > register [2] = 1 => i2c present as new register set
> It's the same device though, so the same compatible.

Sorry, it is different design, and it share the same register space.
So that the reason add new compatible "aspeed,ast2600-i2cv2" for this driver.
It is different register layout.


> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 19/03/2025 12:12, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>> Neither this.
>>>>
>>>> So it seems you describe already existing and documented I2C, but for
>>>> some reason you want second compatible. The problem is that you do
>>>> not provide reason from the point of view of bindings.
>>>>
>>>> To summarize: what your users want - don't care. Start properly
>>>> describing hardware and your SoC.
>>>
>>> OK, for ast2600 i2c controller have two register mode setting.
>>> One, I call it is old register setting, that is right now i2c-aspeed.c
>>> .compatible = "aspeed,ast2600-i2c-bus", And there have a global register
>> that can set i2c controller as new mode register set.
>>> That I am going to drive. That I post is all register in new an old register list.
>>>
>>> For example,
>>> Global register [2] = 0 => i2c present as old register set Global
>>> register [2] = 1 => i2c present as new register set
>> It's the same device though, so the same compatible.
> 
> Sorry, it is different design, and it share the same register space.
> So that the reason add new compatible "aspeed,ast2600-i2cv2" for this driver.
> It is different register layout.

Which device is described by the existing "aspeed,ast2600-i2c-bus"
compatible? And which device is described by new compatible?



Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 19/03/2025 12:12, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>> Neither this.
>>>>
>>>> So it seems you describe already existing and documented I2C, but for
>>>> some reason you want second compatible. The problem is that you do
>>>> not provide reason from the point of view of bindings.
>>>>
>>>> To summarize: what your users want - don't care. Start properly
>>>> describing hardware and your SoC.
>>>
>>> OK, for ast2600 i2c controller have two register mode setting.
>>> One, I call it is old register setting, that is right now i2c-aspeed.c
>>> .compatible = "aspeed,ast2600-i2c-bus", And there have a global register
>> that can set i2c controller as new mode register set.
>>> That I am going to drive. That I post is all register in new an old register list.
>>>
>>> For example,
>>> Global register [2] = 0 => i2c present as old register set Global
>>> register [2] = 1 => i2c present as new register set
>> It's the same device though, so the same compatible.
> 
> Sorry, it is different design, and it share the same register space.
> So that the reason add new compatible "aspeed,ast2600-i2cv2" for this driver.
> It is different register layout.

Which device is described by the existing "aspeed,ast2600-i2c-bus"
compatible? And which device is described by new compatible?



Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 19/03/2025 12:12, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>> Neither this.
> >>>>
> >>>> So it seems you describe already existing and documented I2C, but
> >>>> for some reason you want second compatible. The problem is that you
> >>>> do not provide reason from the point of view of bindings.
> >>>>
> >>>> To summarize: what your users want - don't care. Start properly
> >>>> describing hardware and your SoC.
> >>>
> >>> OK, for ast2600 i2c controller have two register mode setting.
> >>> One, I call it is old register setting, that is right now
> >>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there have
> >>> a global register
> >> that can set i2c controller as new mode register set.
> >>> That I am going to drive. That I post is all register in new an old register
> list.
> >>>
> >>> For example,
> >>> Global register [2] = 0 => i2c present as old register set Global
> >>> register [2] = 1 => i2c present as new register set
> >> It's the same device though, so the same compatible.
> >
> > Sorry, it is different design, and it share the same register space.
> > So that the reason add new compatible "aspeed,ast2600-i2cv2" for this
> driver.
> > It is different register layout.
> 
> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> compatible? And which device is described by new compatible?
> 
On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~ I2C16).
Each of these controllers is hardwired at the SoC level to use either the legacy register layout or the new v2 register layout.
The mode is selected by a bit in the global register, these represent two different hardware blocks:
"aspeed,ast2600-i2c-bus" describes controllers using the legacy register layout.
"aspeed,ast2600-i2cv2" describes controllers using the new register layout
> 
> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 19/03/2025 12:12, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>> Neither this.
> >>>>
> >>>> So it seems you describe already existing and documented I2C, but
> >>>> for some reason you want second compatible. The problem is that you
> >>>> do not provide reason from the point of view of bindings.
> >>>>
> >>>> To summarize: what your users want - don't care. Start properly
> >>>> describing hardware and your SoC.
> >>>
> >>> OK, for ast2600 i2c controller have two register mode setting.
> >>> One, I call it is old register setting, that is right now
> >>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there have
> >>> a global register
> >> that can set i2c controller as new mode register set.
> >>> That I am going to drive. That I post is all register in new an old register
> list.
> >>>
> >>> For example,
> >>> Global register [2] = 0 => i2c present as old register set Global
> >>> register [2] = 1 => i2c present as new register set
> >> It's the same device though, so the same compatible.
> >
> > Sorry, it is different design, and it share the same register space.
> > So that the reason add new compatible "aspeed,ast2600-i2cv2" for this
> driver.
> > It is different register layout.
> 
> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> compatible? And which device is described by new compatible?
> 
On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~ I2C16).
Each of these controllers is hardwired at the SoC level to use either the legacy register layout or the new v2 register layout.
The mode is selected by a bit in the global register, these represent two different hardware blocks:
"aspeed,ast2600-i2c-bus" describes controllers using the legacy register layout.
"aspeed,ast2600-i2cv2" describes controllers using the new register layout
> 
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/03/2025 09:30, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 19/03/2025 12:12, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>>>> Neither this.
>>>>>>
>>>>>> So it seems you describe already existing and documented I2C, but
>>>>>> for some reason you want second compatible. The problem is that you
>>>>>> do not provide reason from the point of view of bindings.
>>>>>>
>>>>>> To summarize: what your users want - don't care. Start properly
>>>>>> describing hardware and your SoC.
>>>>>
>>>>> OK, for ast2600 i2c controller have two register mode setting.
>>>>> One, I call it is old register setting, that is right now
>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there have
>>>>> a global register
>>>> that can set i2c controller as new mode register set.
>>>>> That I am going to drive. That I post is all register in new an old register
>> list.
>>>>>
>>>>> For example,
>>>>> Global register [2] = 0 => i2c present as old register set Global
>>>>> register [2] = 1 => i2c present as new register set
>>>> It's the same device though, so the same compatible.
>>>
>>> Sorry, it is different design, and it share the same register space.
>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for this
>> driver.
>>> It is different register layout.
>>
>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
>> compatible? And which device is described by new compatible?
>>
> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~ I2C16).

So you have 16 same devices.

> Each of these controllers is hardwired at the SoC level to use either the legacy register layout or the new v2 register layout.
> The mode is selected by a bit in the global register, these represent two different hardware blocks:
> "aspeed,ast2600-i2c-bus" describes controllers using the legacy register layout.
> "aspeed,ast2600-i2cv2" describes controllers using the new register layout

Which part of "same device" is not clear? You have one device, one
compatible. Whatever you do with register layout, is already defined by
that compatible. It does not matter that you forgot to implement it in
the Linux kernel.

Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/03/2025 09:30, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 19/03/2025 12:12, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>>>> Neither this.
>>>>>>
>>>>>> So it seems you describe already existing and documented I2C, but
>>>>>> for some reason you want second compatible. The problem is that you
>>>>>> do not provide reason from the point of view of bindings.
>>>>>>
>>>>>> To summarize: what your users want - don't care. Start properly
>>>>>> describing hardware and your SoC.
>>>>>
>>>>> OK, for ast2600 i2c controller have two register mode setting.
>>>>> One, I call it is old register setting, that is right now
>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there have
>>>>> a global register
>>>> that can set i2c controller as new mode register set.
>>>>> That I am going to drive. That I post is all register in new an old register
>> list.
>>>>>
>>>>> For example,
>>>>> Global register [2] = 0 => i2c present as old register set Global
>>>>> register [2] = 1 => i2c present as new register set
>>>> It's the same device though, so the same compatible.
>>>
>>> Sorry, it is different design, and it share the same register space.
>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for this
>> driver.
>>> It is different register layout.
>>
>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
>> compatible? And which device is described by new compatible?
>>
> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~ I2C16).

So you have 16 same devices.

> Each of these controllers is hardwired at the SoC level to use either the legacy register layout or the new v2 register layout.
> The mode is selected by a bit in the global register, these represent two different hardware blocks:
> "aspeed,ast2600-i2c-bus" describes controllers using the legacy register layout.
> "aspeed,ast2600-i2cv2" describes controllers using the new register layout

Which part of "same device" is not clear? You have one device, one
compatible. Whatever you do with register layout, is already defined by
that compatible. It does not matter that you forgot to implement it in
the Linux kernel.

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 24/03/2025 09:30, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>> Neither this.
> >>>>>>
> >>>>>> So it seems you describe already existing and documented I2C, but
> >>>>>> for some reason you want second compatible. The problem is that
> >>>>>> you do not provide reason from the point of view of bindings.
> >>>>>>
> >>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>> describing hardware and your SoC.
> >>>>>
> >>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>> One, I call it is old register setting, that is right now
> >>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>> have a global register
> >>>> that can set i2c controller as new mode register set.
> >>>>> That I am going to drive. That I post is all register in new an
> >>>>> old register
> >> list.
> >>>>>
> >>>>> For example,
> >>>>> Global register [2] = 0 => i2c present as old register set Global
> >>>>> register [2] = 1 => i2c present as new register set
> >>>> It's the same device though, so the same compatible.
> >>>
> >>> Sorry, it is different design, and it share the same register space.
> >>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
> >>> this
> >> driver.
> >>> It is different register layout.
> >>
> >> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >> compatible? And which device is described by new compatible?
> >>
> > On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~
> I2C16).
> 
> So you have 16 same devices.
Yes, one driver instance for 16 i2c device. 
> 
> > Each of these controllers is hardwired at the SoC level to use either the
> legacy register layout or the new v2 register layout.
> > The mode is selected by a bit in the global register, these represent two
> different hardware blocks:
> > "aspeed,ast2600-i2c-bus" describes controllers using the legacy register
> layout.
> > "aspeed,ast2600-i2cv2" describes controllers using the new register
> > layout
> 
> Which part of "same device" is not clear? You have one device, one compatible.
> Whatever you do with register layout, is already defined by that compatible. It
> does not matter that you forgot to implement it in the Linux kernel.

Sorry, I still can't catch your point.
I separated the support into two drivers:
i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
i2c-ast2600.c for the new register set, compatible compatible "aspeed,ast2600-i2cv2"
Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be separate two driver?
> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 24/03/2025 09:30, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>> Neither this.
> >>>>>>
> >>>>>> So it seems you describe already existing and documented I2C, but
> >>>>>> for some reason you want second compatible. The problem is that
> >>>>>> you do not provide reason from the point of view of bindings.
> >>>>>>
> >>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>> describing hardware and your SoC.
> >>>>>
> >>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>> One, I call it is old register setting, that is right now
> >>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>> have a global register
> >>>> that can set i2c controller as new mode register set.
> >>>>> That I am going to drive. That I post is all register in new an
> >>>>> old register
> >> list.
> >>>>>
> >>>>> For example,
> >>>>> Global register [2] = 0 => i2c present as old register set Global
> >>>>> register [2] = 1 => i2c present as new register set
> >>>> It's the same device though, so the same compatible.
> >>>
> >>> Sorry, it is different design, and it share the same register space.
> >>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
> >>> this
> >> driver.
> >>> It is different register layout.
> >>
> >> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >> compatible? And which device is described by new compatible?
> >>
> > On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~
> I2C16).
> 
> So you have 16 same devices.
Yes, one driver instance for 16 i2c device. 
> 
> > Each of these controllers is hardwired at the SoC level to use either the
> legacy register layout or the new v2 register layout.
> > The mode is selected by a bit in the global register, these represent two
> different hardware blocks:
> > "aspeed,ast2600-i2c-bus" describes controllers using the legacy register
> layout.
> > "aspeed,ast2600-i2cv2" describes controllers using the new register
> > layout
> 
> Which part of "same device" is not clear? You have one device, one compatible.
> Whatever you do with register layout, is already defined by that compatible. It
> does not matter that you forgot to implement it in the Linux kernel.

Sorry, I still can't catch your point.
I separated the support into two drivers:
i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
i2c-ast2600.c for the new register set, compatible compatible "aspeed,ast2600-i2cv2"
Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be separate two driver?
> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/03/2025 11:01, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 24/03/2025 09:30, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 19/03/2025 12:12, Ryan Chen wrote:
>>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>>>> AST2600-i2cv2
>>>>>>
>>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>>>>>> Neither this.
>>>>>>>>
>>>>>>>> So it seems you describe already existing and documented I2C, but
>>>>>>>> for some reason you want second compatible. The problem is that
>>>>>>>> you do not provide reason from the point of view of bindings.
>>>>>>>>
>>>>>>>> To summarize: what your users want - don't care. Start properly
>>>>>>>> describing hardware and your SoC.
>>>>>>>
>>>>>>> OK, for ast2600 i2c controller have two register mode setting.
>>>>>>> One, I call it is old register setting, that is right now
>>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
>>>>>>> have a global register
>>>>>> that can set i2c controller as new mode register set.
>>>>>>> That I am going to drive. That I post is all register in new an
>>>>>>> old register
>>>> list.
>>>>>>>
>>>>>>> For example,
>>>>>>> Global register [2] = 0 => i2c present as old register set Global
>>>>>>> register [2] = 1 => i2c present as new register set
>>>>>> It's the same device though, so the same compatible.
>>>>>
>>>>> Sorry, it is different design, and it share the same register space.
>>>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
>>>>> this
>>>> driver.
>>>>> It is different register layout.
>>>>
>>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
>>>> compatible? And which device is described by new compatible?
>>>>
>>> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~
>> I2C16).
>>
>> So you have 16 same devices.
> Yes, one driver instance for 16 i2c device. 
>>
>>> Each of these controllers is hardwired at the SoC level to use either the
>> legacy register layout or the new v2 register layout.
>>> The mode is selected by a bit in the global register, these represent two
>> different hardware blocks:
>>> "aspeed,ast2600-i2c-bus" describes controllers using the legacy register
>> layout.
>>> "aspeed,ast2600-i2cv2" describes controllers using the new register
>>> layout
>>
>> Which part of "same device" is not clear? You have one device, one compatible.
>> Whatever you do with register layout, is already defined by that compatible. It
>> does not matter that you forgot to implement it in the Linux kernel.
> 
> Sorry, I still can't catch your point.


I repeated twice "You have one device, one compatible.", provided few
more sentences and if this is still unclear, I don't know what to say more.

> I separated the support into two drivers:

I don't care about your drivers, why are you bringing them here?

> i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
> i2c-ast2600.c for the new register set, compatible compatible "aspeed,ast2600-i2cv2"
> Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be separate two driver?

What is this patch about? Bindings. Not drivers. Look at email month ago:

"All this describes driver, not hardware."

Why are you keep bringing here drivers since a month?

Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 24/03/2025 11:01, Ryan Chen wrote:
>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>> AST2600-i2cv2
>>
>> On 24/03/2025 09:30, Ryan Chen wrote:
>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 19/03/2025 12:12, Ryan Chen wrote:
>>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>>>> AST2600-i2cv2
>>>>>>
>>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>>>>>> Neither this.
>>>>>>>>
>>>>>>>> So it seems you describe already existing and documented I2C, but
>>>>>>>> for some reason you want second compatible. The problem is that
>>>>>>>> you do not provide reason from the point of view of bindings.
>>>>>>>>
>>>>>>>> To summarize: what your users want - don't care. Start properly
>>>>>>>> describing hardware and your SoC.
>>>>>>>
>>>>>>> OK, for ast2600 i2c controller have two register mode setting.
>>>>>>> One, I call it is old register setting, that is right now
>>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
>>>>>>> have a global register
>>>>>> that can set i2c controller as new mode register set.
>>>>>>> That I am going to drive. That I post is all register in new an
>>>>>>> old register
>>>> list.
>>>>>>>
>>>>>>> For example,
>>>>>>> Global register [2] = 0 => i2c present as old register set Global
>>>>>>> register [2] = 1 => i2c present as new register set
>>>>>> It's the same device though, so the same compatible.
>>>>>
>>>>> Sorry, it is different design, and it share the same register space.
>>>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
>>>>> this
>>>> driver.
>>>>> It is different register layout.
>>>>
>>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
>>>> compatible? And which device is described by new compatible?
>>>>
>>> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~
>> I2C16).
>>
>> So you have 16 same devices.
> Yes, one driver instance for 16 i2c device. 
>>
>>> Each of these controllers is hardwired at the SoC level to use either the
>> legacy register layout or the new v2 register layout.
>>> The mode is selected by a bit in the global register, these represent two
>> different hardware blocks:
>>> "aspeed,ast2600-i2c-bus" describes controllers using the legacy register
>> layout.
>>> "aspeed,ast2600-i2cv2" describes controllers using the new register
>>> layout
>>
>> Which part of "same device" is not clear? You have one device, one compatible.
>> Whatever you do with register layout, is already defined by that compatible. It
>> does not matter that you forgot to implement it in the Linux kernel.
> 
> Sorry, I still can't catch your point.


I repeated twice "You have one device, one compatible.", provided few
more sentences and if this is still unclear, I don't know what to say more.

> I separated the support into two drivers:

I don't care about your drivers, why are you bringing them here?

> i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
> i2c-ast2600.c for the new register set, compatible compatible "aspeed,ast2600-i2cv2"
> Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be separate two driver?

What is this patch about? Bindings. Not drivers. Look at email month ago:

"All this describes driver, not hardware."

Why are you keep bringing here drivers since a month?

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 24/03/2025 11:01, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 24/03/2025 09:30, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support
> >>>>>> for
> >>>>>> AST2600-i2cv2
> >>>>>>
> >>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>>>> Neither this.
> >>>>>>>>
> >>>>>>>> So it seems you describe already existing and documented I2C,
> >>>>>>>> but for some reason you want second compatible. The problem is
> >>>>>>>> that you do not provide reason from the point of view of bindings.
> >>>>>>>>
> >>>>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>>>> describing hardware and your SoC.
> >>>>>>>
> >>>>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>>>> One, I call it is old register setting, that is right now
> >>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>>>> have a global register
> >>>>>> that can set i2c controller as new mode register set.
> >>>>>>> That I am going to drive. That I post is all register in new an
> >>>>>>> old register
> >>>> list.
> >>>>>>>
> >>>>>>> For example,
> >>>>>>> Global register [2] = 0 => i2c present as old register set
> >>>>>>> Global register [2] = 1 => i2c present as new register set
> >>>>>> It's the same device though, so the same compatible.
> >>>>>
> >>>>> Sorry, it is different design, and it share the same register space.
> >>>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
> >>>>> this
> >>>> driver.
> >>>>> It is different register layout.
> >>>>
> >>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >>>> compatible? And which device is described by new compatible?
> >>>>
> >>> On the AST2600 SoC, there are up to 16 I2C controller instances
> >>> (I2C1 ~
> >> I2C16).
> >>
> >> So you have 16 same devices.
> > Yes, one driver instance for 16 i2c device.
> >>
> >>> Each of these controllers is hardwired at the SoC level to use
> >>> either the
> >> legacy register layout or the new v2 register layout.
> >>> The mode is selected by a bit in the global register, these
> >>> represent two
> >> different hardware blocks:
> >>> "aspeed,ast2600-i2c-bus" describes controllers using the legacy
> >>> register
> >> layout.
> >>> "aspeed,ast2600-i2cv2" describes controllers using the new register
> >>> layout
> >>
> >> Which part of "same device" is not clear? You have one device, one
> compatible.
> >> Whatever you do with register layout, is already defined by that
> >> compatible. It does not matter that you forgot to implement it in the Linux
> kernel.
> >
> > Sorry, I still can't catch your point.
> 
> 
> I repeated twice "You have one device, one compatible.", provided few more
> sentences and if this is still unclear, I don't know what to say more.
> 
> > I separated the support into two drivers:
> 
> I don't care about your drivers, why are you bringing them here?
> 
> > i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
> > i2c-ast2600.c for the new register set, compatible compatible
> "aspeed,ast2600-i2cv2"
> > Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be
> separate two driver?
> 
> What is this patch about? Bindings. Not drivers. Look at email month ago:
> 
> "All this describes driver, not hardware."
> 
> Why are you keep bringing here drivers since a month?
Let me try to rephrase the reasoning from a hardware point of view:

On AST2600, each I2C controller instance is functionally the same type of device (I2C controller), 
but it can present two different and incompatible register layouts, 
depending on a hardware-controlled configuration (via global register bits that are fixed in production SoCs).
The layouts are not backward-compatible:
Registers are at different offsets. Bit definitions differ.
The programming sequence is not the same.
Because of this incompatibility at the register level, software cannot handle both layouts in a generic way without explicit knowledge of which version is present.
That’s why I proposed a new compatible string — to represent a hardware-visible difference in register interface, even though the logical function (I2C controller) is the same.


> 
> Best regards,
> Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 24/03/2025 11:01, Ryan Chen wrote:
> >> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 24/03/2025 09:30, Ryan Chen wrote:
> >>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support
> >>>>>> for
> >>>>>> AST2600-i2cv2
> >>>>>>
> >>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>>>> Neither this.
> >>>>>>>>
> >>>>>>>> So it seems you describe already existing and documented I2C,
> >>>>>>>> but for some reason you want second compatible. The problem is
> >>>>>>>> that you do not provide reason from the point of view of bindings.
> >>>>>>>>
> >>>>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>>>> describing hardware and your SoC.
> >>>>>>>
> >>>>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>>>> One, I call it is old register setting, that is right now
> >>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>>>> have a global register
> >>>>>> that can set i2c controller as new mode register set.
> >>>>>>> That I am going to drive. That I post is all register in new an
> >>>>>>> old register
> >>>> list.
> >>>>>>>
> >>>>>>> For example,
> >>>>>>> Global register [2] = 0 => i2c present as old register set
> >>>>>>> Global register [2] = 1 => i2c present as new register set
> >>>>>> It's the same device though, so the same compatible.
> >>>>>
> >>>>> Sorry, it is different design, and it share the same register space.
> >>>>> So that the reason add new compatible "aspeed,ast2600-i2cv2" for
> >>>>> this
> >>>> driver.
> >>>>> It is different register layout.
> >>>>
> >>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >>>> compatible? And which device is described by new compatible?
> >>>>
> >>> On the AST2600 SoC, there are up to 16 I2C controller instances
> >>> (I2C1 ~
> >> I2C16).
> >>
> >> So you have 16 same devices.
> > Yes, one driver instance for 16 i2c device.
> >>
> >>> Each of these controllers is hardwired at the SoC level to use
> >>> either the
> >> legacy register layout or the new v2 register layout.
> >>> The mode is selected by a bit in the global register, these
> >>> represent two
> >> different hardware blocks:
> >>> "aspeed,ast2600-i2c-bus" describes controllers using the legacy
> >>> register
> >> layout.
> >>> "aspeed,ast2600-i2cv2" describes controllers using the new register
> >>> layout
> >>
> >> Which part of "same device" is not clear? You have one device, one
> compatible.
> >> Whatever you do with register layout, is already defined by that
> >> compatible. It does not matter that you forgot to implement it in the Linux
> kernel.
> >
> > Sorry, I still can't catch your point.
> 
> 
> I repeated twice "You have one device, one compatible.", provided few more
> sentences and if this is still unclear, I don't know what to say more.
> 
> > I separated the support into two drivers:
> 
> I don't care about your drivers, why are you bringing them here?
> 
> > i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
> > i2c-ast2600.c for the new register set, compatible compatible
> "aspeed,ast2600-i2cv2"
> > Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be
> separate two driver?
> 
> What is this patch about? Bindings. Not drivers. Look at email month ago:
> 
> "All this describes driver, not hardware."
> 
> Why are you keep bringing here drivers since a month?
Let me try to rephrase the reasoning from a hardware point of view:

On AST2600, each I2C controller instance is functionally the same type of device (I2C controller), 
but it can present two different and incompatible register layouts, 
depending on a hardware-controlled configuration (via global register bits that are fixed in production SoCs).
The layouts are not backward-compatible:
Registers are at different offsets. Bit definitions differ.
The programming sequence is not the same.
Because of this incompatibility at the register level, software cannot handle both layouts in a generic way without explicit knowledge of which version is present.
That’s why I proposed a new compatible string — to represent a hardware-visible difference in register interface, even though the logical function (I2C controller) is the same.


> 
> Best regards,
> Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 25/03/2025 10:52, Ryan Chen wrote:
>>
>>> i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
>>> i2c-ast2600.c for the new register set, compatible compatible
>> "aspeed,ast2600-i2cv2"
>>> Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be
>> separate two driver?
>>
>> What is this patch about? Bindings. Not drivers. Look at email month ago:
>>
>> "All this describes driver, not hardware."
>>
>> Why are you keep bringing here drivers since a month?
> Let me try to rephrase the reasoning from a hardware point of view:
> 
> On AST2600, each I2C controller instance is functionally the same type of device (I2C controller), 
> but it can present two different and incompatible register layouts, 
> depending on a hardware-controlled configuration (via global register bits that are fixed in production SoCs).
> The layouts are not backward-compatible:
> Registers are at different offsets. Bit definitions differ.
> The programming sequence is not the same.
> Because of this incompatibility at the register level, software cannot handle both layouts in a generic way without explicit knowledge of which version is present.
> That’s why I proposed a new compatible string — to represent a hardware-visible difference in register interface, even though the logical function (I2C controller) is the same.

You implied software is set in stone and that's not true. You have a
compatible which says to the software - this device has two programming
interfaces, choose whatever you wish and work with that.

Different compatible would mean the device is different, it changes or
you have compatibility issues.

NONE of these cases are here.

I am done with this topic, because explaining this and other
trivialities to Aspeed takes way too much time, so last opinion:

Your compatible already expressed that there are two interfaces, so your
drivers can just choose whichever they want. If you need to toggle a bit
in system controller, it is fine. If you need different compatible, then
that's a NAK.

Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 25/03/2025 10:52, Ryan Chen wrote:
>>
>>> i2c-aspeed.c for legacy layout, compatible "aspeed,ast2600-i2c-bus"
>>> i2c-ast2600.c for the new register set, compatible compatible
>> "aspeed,ast2600-i2cv2"
>>> Do you mean I have integrate into 1 driver at i2c-aspeed.c ? can't be
>> separate two driver?
>>
>> What is this patch about? Bindings. Not drivers. Look at email month ago:
>>
>> "All this describes driver, not hardware."
>>
>> Why are you keep bringing here drivers since a month?
> Let me try to rephrase the reasoning from a hardware point of view:
> 
> On AST2600, each I2C controller instance is functionally the same type of device (I2C controller), 
> but it can present two different and incompatible register layouts, 
> depending on a hardware-controlled configuration (via global register bits that are fixed in production SoCs).
> The layouts are not backward-compatible:
> Registers are at different offsets. Bit definitions differ.
> The programming sequence is not the same.
> Because of this incompatibility at the register level, software cannot handle both layouts in a generic way without explicit knowledge of which version is present.
> That’s why I proposed a new compatible string — to represent a hardware-visible difference in register interface, even though the logical function (I2C controller) is the same.

You implied software is set in stone and that's not true. You have a
compatible which says to the software - this device has two programming
interfaces, choose whatever you wish and work with that.

Different compatible would mean the device is different, it changes or
you have compatibility issues.

NONE of these cases are here.

I am done with this topic, because explaining this and other
trivialities to Aspeed takes way too much time, so last opinion:

Your compatible already expressed that there are two interfaces, so your
drivers can just choose whichever they want. If you need to toggle a bit
in system controller, it is fine. If you need different compatible, then
that's a NAK.

Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Jeremy Kerr]

Hi Ryan & Krzysztof,

[my response is intended to make progress on the newer v18 submission,
but we still have this item from v16 to resolve, hence picking up this
thread]

> Your compatible already expressed that there are two interfaces, so
> your drivers can just choose whichever they want. If you need to toggle a
> bit in system controller, it is fine. If you need different compatible,
> then that's a NAK.

I think the mention of "two register interfaces" is a bit misleading
here; it implies that it's just two interfaces to the same hardware.

From reading between the lines on the datasheet, it seems that this is
two completely separate IP cores, that:

 * are mapped to the same MMIO space; but
 * both happen to be I2C controllers.

- where the single "global register" (which you mention above) provides
the facility to mux the MMIO mapping between the two. Some versions of
the overall SoC have only the old core, some have only the new, and some
have both, selectable via this register.

Ryan, can you confirm whether this is the case?

Given there are actual behavioural differences between the two
peripherals - beyond just the register set - that would seem to indicate
separate binding types (+ a syscon mux control) to me, but I'm keen to
hear any other options.

Krzysztof, if that is the case, any thoughts on the representation of
separate bindings?

Cheers,


Jeremy

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 10/09/2025 09:25, Jeremy Kerr wrote:
> Hi Ryan & Krzysztof,
> 
> [my response is intended to make progress on the newer v18 submission,
> but we still have this item from v16 to resolve, hence picking up this
> thread]
> 
>> Your compatible already expressed that there are two interfaces, so
>> your drivers can just choose whichever they want. If you need to toggle a
>> bit in system controller, it is fine. If you need different compatible,
>> then that's a NAK.

You trimmed response and brought some very old thread which does not
exist in my inbox.

I have absolutely no clue what this refers to.

> 
> I think the mention of "two register interfaces" is a bit misleading
> here; it implies that it's just two interfaces to the same hardware.
> 
> From reading between the lines on the datasheet, it seems that this is
> two completely separate IP cores, that:
> 
>  * are mapped to the same MMIO space; but
>  * both happen to be I2C controllers.
> 
> - where the single "global register" (which you mention above) provides
> the facility to mux the MMIO mapping between the two. Some versions of
> the overall SoC have only the old core, some have only the new, and some
> have both, selectable via this register.
> 
> Ryan, can you confirm whether this is the case?
> 
> Given there are actual behavioural differences between the two
> peripherals - beyond just the register set - that would seem to indicate
> separate binding types (+ a syscon mux control) to me, but I'm keen to
> hear any other options.
> 
> Krzysztof, if that is the case, any thoughts on the representation of
> separate bindings?


I have no clue what is this about.


Best regards,
Krzysztof

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Jeremy Kerr]

Hi Krzysztof,

> You trimmed response and brought some very old thread which does not
> exist in my inbox.
> 
> I have absolutely no clue what this refers to.

OK, reconstructing the relevant parts of that thread - Ryan providing
background on the old/new register interfaces (trimmed a little for
brevity; original context at [1] if you need):

>On 24/03/2025 09:30, Ryan Chen wrote:
>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>> AST2600-i2cv2
>>>
>>> On 19/03/2025 12:12, Ryan Chen wrote:
>>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
>>>>> AST2600-i2cv2
>>>>>
>>>>> On 17/03/2025 10:21, Ryan Chen wrote:
>>>>>>> Neither this.
>>>>>>>
>>>>>>> So it seems you describe already existing and documented I2C, but
>>>>>>> for some reason you want second compatible. The problem is that you
>>>>>>> do not provide reason from the point of view of bindings.
>>>>>>>
>>>>>>> To summarize: what your users want - don't care. Start properly
>>>>>>> describing hardware and your SoC.
>>>>>>
>>>>>> OK, for ast2600 i2c controller have two register mode setting.
>>>>>> One, I call it is old register setting, that is right now
>>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
>>>>>> have a global register that can set i2c controller as new mode
>>>>>> register set. That I am going to drive. That I post is all
>>>>>> register in new an old register list.
>>>>>>
>>>>>> For example,
>>>>>> Global register [2] = 0 => i2c present as old register set Global
>>>>>> register [2] = 1 => i2c present as new register set
>>>>> It's the same device though, so the same compatible.
>>>>
>>>> Sorry, it is different design, and it share the same register
>>>> space. So that the reason add new compatible "aspeed,ast2600-i2cv2"
>>>> for this driver. It is different register layout.
>>>
>>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
>>> compatible? And which device is described by new compatible?
>>>
>> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~ I2C16).
>
> So you have 16 same devices.
>
>> Each of these controllers is hardwired at the SoC level to use either
>> the legacy register layout or the new v2 register layout. The mode is
>> selected by a bit in the global register, these represent two
>> different hardware blocks: "aspeed,ast2600-i2c-bus" describes
>> controllers using the legacy register layout. "aspeed,ast2600-i2cv2"
>> describes controllers using the new register layout
> 
> Which part of "same device" is not clear? You have one device, one
> compatible. Whatever you do with register layout, is already defined by
> that compatible. It does not matter that you forgot to implement it in
> the Linux kernel.

So, I'm trying to pick up (from Ryan) on whether we're actually dealing
with separate devices here; that was ambiguous in his responses.

To me, it seems like we do have separate IP cores, just multiplexed to
the same MMIO space. And if so, what the preference on binding
implementation is, particularly with different SoCs having either only
the "old", only the "new", or a switchable set of both.

Hence my query:

> Given there are actual behavioural differences between the two
> peripherals - beyond just the register set - that would seem to indicate
> separate binding types (+ a syscon mux control) to me, but I'm keen to
> hear any other options.
>
> Krzysztof, if that is the case, any thoughts on the representation of
> separate bindings?

- given we may not be dealing with "the same device" in actual hardware,
in reference to Ryan's proposed compatible split between the two.

Cheers,


Jeremy

[1]: https://lore.kernel.org/all/20250224055936.1804279-2-ryan_chen@aspeedtech.com/t/#u

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> Hi Krzysztof,
> 
> > You trimmed response and brought some very old thread which does not
> > exist in my inbox.
> >
> > I have absolutely no clue what this refers to.
> 
> OK, reconstructing the relevant parts of that thread - Ryan providing
> background on the old/new register interfaces (trimmed a little for brevity;
> original context at [1] if you need):
> 
> >On 24/03/2025 09:30, Ryan Chen wrote:
> >>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>> AST2600-i2cv2
> >>>
> >>> On 19/03/2025 12:12, Ryan Chen wrote:
> >>>>> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> >>>>> AST2600-i2cv2
> >>>>>
> >>>>> On 17/03/2025 10:21, Ryan Chen wrote:
> >>>>>>> Neither this.
> >>>>>>>
> >>>>>>> So it seems you describe already existing and documented I2C,
> >>>>>>> but for some reason you want second compatible. The problem is
> >>>>>>> that you do not provide reason from the point of view of bindings.
> >>>>>>>
> >>>>>>> To summarize: what your users want - don't care. Start properly
> >>>>>>> describing hardware and your SoC.
> >>>>>>
> >>>>>> OK, for ast2600 i2c controller have two register mode setting.
> >>>>>> One, I call it is old register setting, that is right now
> >>>>>> i2c-aspeed.c .compatible = "aspeed,ast2600-i2c-bus", And there
> >>>>>> have a global register that can set i2c controller as new mode
> >>>>>> register set. That I am going to drive. That I post is all
> >>>>>> register in new an old register list.
> >>>>>>
> >>>>>> For example,
> >>>>>> Global register [2] = 0 => i2c present as old register set Global
> >>>>>> register [2] = 1 => i2c present as new register set
> >>>>> It's the same device though, so the same compatible.
> >>>>
> >>>> Sorry, it is different design, and it share the same register
> >>>> space. So that the reason add new compatible "aspeed,ast2600-i2cv2"
> >>>> for this driver. It is different register layout.
> >>>
> >>> Which device is described by the existing "aspeed,ast2600-i2c-bus"
> >>> compatible? And which device is described by new compatible?
> >>>
> >> On the AST2600 SoC, there are up to 16 I2C controller instances (I2C1 ~
> I2C16).
> >
> > So you have 16 same devices.
> >
> >> Each of these controllers is hardwired at the SoC level to use either
> >> the legacy register layout or the new v2 register layout. The mode is
> >> selected by a bit in the global register, these represent two
> >> different hardware blocks: "aspeed,ast2600-i2c-bus" describes
> >> controllers using the legacy register layout. "aspeed,ast2600-i2cv2"
> >> describes controllers using the new register layout
> >
> > Which part of "same device" is not clear? You have one device, one
> > compatible. Whatever you do with register layout, is already defined
> > by that compatible. It does not matter that you forgot to implement it
> > in the Linux kernel.
> 
> So, I'm trying to pick up (from Ryan) on whether we're actually dealing with
> separate devices here; that was ambiguous in his responses.

Hello Jeremy, Krzysztof

Sorry, for ambiguous.
The global register like a mux selection for new/old register layout.
Like following example.

						=======================
						Driver : compatible = "aspeed,ast2600-i2c-bus"
						Old register layout : i2c0 ~ 15
						=======================
					0:/
==================
aspeed,global-regs
Global MUX (new/old)
==================
					1:\
						=======================
						Driver: compatible = "aspeed,ast2600-i2cv2-bus "
						New register layout : i2c0 ~ 15
						=======================

> 
> To me, it seems like we do have separate IP cores, just multiplexed to the same
> MMIO space. And if so, what the preference on binding implementation is,
> particularly with different SoCs having either only the "old", only the "new", or
> a switchable set of both.
> 
> Hence my query:
> 
> > Given there are actual behavioural differences between the two
> > peripherals - beyond just the register set - that would seem to
> > indicate separate binding types (+ a syscon mux control) to me, but
> > I'm keen to hear any other options.
> >
> > Krzysztof, if that is the case, any thoughts on the representation of
> > separate bindings?
> 
> - given we may not be dealing with "the same device" in actual hardware, in
> reference to Ryan's proposed compatible split between the two.
> 
> Cheers,
> 
> 
> Jeremy
> 
> [1]:
> https://lore.kernel.org/all/20250224055936.1804279-2-ryan_chen@aspeedtec
> h.com/t/#u

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Jeremy Kerr]

Hi Ryan,

> Sorry, for ambiguous.
> The global register like a mux selection for new/old register layout.
> Like following example.

That wasn't the ambiguous part - I think we are clear that there is a
multiplexer that controls what registers appear at the node's bus
address.

The question was more: it sounds like you're switching between
*fundamentally different* hardware units with the mux switch - not just
a different register interface for the same peripheral hardware. Is that
the case?

This is an important distinction in that some SoCs have the old
peripheral, some have the new, and some have the mux-switching between
both.

Behaviour of those two peripheral options differs beyond just a
"register interface", and calling it just a change in register layout is
misguiding the conversation somewhat.

If that's the case, then the separate compatible values may make more
sense.

Cheers,


Jeremy

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Jeremy Kerr]

Hi all,

After a bit of a chat with Ryan, some updates on this:

> The question was more: it sounds like you're switching between
> *fundamentally different* hardware units with the mux switch - not just
> a different register interface for the same peripheral hardware. Is that
> the case?

Turns out: no. The controller core is the same, but what gets muxed
in/out is more of a compatibility interface. This provides an
ast2500-like register set to the ast2600 i2c peripheral.

So, the plan to use the same aspeed,ast2600-i2c-bus binding for the
controllers (ie, as in v18) seems mostly sensible to me.

The newly-introduced driver can make use of the non-compat interface to
the peripheral (using the existing compatible value), but needs the
introduction of support for the global register set to do so. Ideally,
this would have been included on the original spec for the
aspeed,ast2600-i2c-bus binding (since that's how the hardware is laid
out), but that wasn't the case.

We may want to split the aspeed,ast2600-i2c-bus binding out from the
existing spec, as we currently have:

  compatible:
    enum:
      - aspeed,ast2400-i2c-bus
      - aspeed,ast2500-i2c-bus
      - aspeed,ast2600-i2c-bus

- but the former two do not have a global register-set.

We may have better syscon-like options for handling that global register
set, but that's more of a conversation for the v18 thread.

Cheers,


Jeremy

Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Krzysztof Kozlowski]

On 11/09/2025 11:03, Jeremy Kerr wrote:
> Hi all,
> 
> After a bit of a chat with Ryan, some updates on this:
> 
>> The question was more: it sounds like you're switching between
>> *fundamentally different* hardware units with the mux switch - not just
>> a different register interface for the same peripheral hardware. Is that
>> the case?
> 
> Turns out: no. The controller core is the same, but what gets muxed
> in/out is more of a compatibility interface. This provides an
> ast2500-like register set to the ast2600 i2c peripheral.


If you had two separate bindings, how would you represent it in DTS? Two
device nodes, right? That's confusing, because there is only one device.

If the device can present or change its programming interface, it is
still that device, so still one binding for it. And that device driver
will handle both (or one) programming models.

I remember now the problem we talk about, but I don't get what exactly
you want to solve/discuss. Anyway any discussion should be about newest
patch, not something from February, so if you still have concerns please
raise them at v18 (or whichever version is now).

Best regards,
Krzysztof

RE: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

[Ryan Chen]

> Subject: Re: [PATCH v16 1/3] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 11/09/2025 11:03, Jeremy Kerr wrote:
> > Hi all,
> >
> > After a bit of a chat with Ryan, some updates on this:
> >
> >> The question was more: it sounds like you're switching between
> >> *fundamentally different* hardware units with the mux switch - not
> >> just a different register interface for the same peripheral hardware.
> >> Is that the case?
> >
> > Turns out: no. The controller core is the same, but what gets muxed
> > in/out is more of a compatibility interface. This provides an
> > ast2500-like register set to the ast2600 i2c peripheral.
> 
> 
> If you had two separate bindings, how would you represent it in DTS? Two
> device nodes, right? That's confusing, because there is only one device.
> 
> If the device can present or change its programming interface, it is still that
> device, so still one binding for it. And that device driver will handle both (or
> one) programming models.
> 
> I remember now the problem we talk about, but I don't get what exactly you
> want to solve/discuss. Anyway any discussion should be about newest patch,
> not something from February, so if you still have concerns please raise them at
> v18 (or whichever version is now).
> 
> Best regards,
> Krzysztof

Hello Krzysztof, 
	Thanks a lot.
	That I submitted with v18. (that add reason in commit why I add in yaml.
	So, let's back to v18 give me more feedback, that I can fulfill.
	Appreciate your help.
v18 https://lore.kernel.org/all/20250820051832.3605405-2-ryan_chen@aspeedtech.com/

Ryan