Add ASPEED AST2600 I2C new controller driver

AST2600 I2C global register controller, add bindings document to support driver of i2c global register set.

+allOf:
+? - $ref: /schemas/i2c/i2c-controller.yaml#

Quoted 3 lines

AST2600 I2C global register controller, add bindings document to support driver of i2c global register set.

[...]

Quoted 2 lines

+allOf:
+? - $ref: /schemas/i2c/i2c-controller.yaml#

Are you sure about this? This particular binding doesn't describe the i2c controller.

Review

Quoted 12 lines

(quoted diff) +11 -0

+
+properties:
+? compatible:
+??? enum:
+????? - aspeed,ast2600-i2c-global
+
+? reg:
+??? minItems: 1
+??? items:
+????? - description: address offset and range of bus
+????? - description: address offset and range of bus buffer

Seems to be a duplicate here.

Jeremy

AST2600 I2C global register controller, add bindings document to support driver of i2c global register set.

Signed-off-by Ryan Chen <[email protected]>

---
 .../i2c/aspeed,i2c-ast2600-global.yaml        | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600-global.yaml

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600-global.yaml new file +0 -0

new file mode 100644
index 000000000000..d92a0878d03b

AST2600 support new register set for I2C controller, add bindings document to support driver of i2c new register mode controller.

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +4 -0

+
+maintainers:
+  - Ryan Chen <[email protected]>
+

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +13 -0

+
+  multi-master:
+    type: boolean
+    description:
+      states that there is another master active on this bus
+
+required:
+  - reg
+  - compatible
+  - clocks
+  - resets
+
+unevaluatedProperties: false

AST2600 support new register set for I2C controller, add bindings document to support driver of i2c new register mode controller.

Signed-off-by Ryan Chen <[email protected]>

---
 .../bindings/i2c/aspeed,i2c-ast2600.yaml      | 78 +++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +8 -0

new file mode 100644
index 000000000000..b7d7bc303e77
@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AST2600 I2C Controller on the AST26XX SoCs Device Tree Bindings
+

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 2, 2023 4:58 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 6 lines · Ryan Chen

AST2600 support new register set for I2C controller, add bindings document to support driver of i2c new register mode controller.

Signed-off-by Ryan Chen <[email protected]>

---
 .../bindings/i2c/aspeed,i2c-ast2600.yaml      | 78

+++++++++++++++++++

Quoted 25 lines

 1 file changed, 78 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml

diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file mode 100644
index 000000000000..b7d7bc303e77

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +13 -0

@@ -0,0 +1,78 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/aspeed,i2c-ast2600.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AST2600 I2C Controller on the AST26XX SoCs Device Tree
+Bindings
+
+maintainers:
+  - Ryan Chen <[email protected]>
+
+allOf:
+  - $ref: /schemas/i2c/i2c-controller.yaml#

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml line 18

Quoted 5 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +5 -0

+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

Quoted 9 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml line 18

Quoted 6 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

It is not duplicated, as my description in cover " This series add AST2600 i2c new register set driver" So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus".

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 11 lines · Ryan Chen

Quoted 9 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml line 18

Quoted 6 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

It is not duplicated, as my description in cover " This series add AST2600 i2c

new register set driver"

Quoted 6 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus".

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a differentiating name.

Hello Krzysztof

Quoted 32 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 11 lines · Ryan Chen

Quoted 9 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml line 18

Quoted 6 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

It is not duplicated, as my description in cover " This series add AST2600 i2c

new register set driver"

Quoted 6 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus".

Quoted 4 lines

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a differentiating name.

For misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2, will it more clear way ?

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 5:22 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 35 lines · Ryan Chen

Hello Krzysztof

Quoted 33 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 10 lines · Ryan Chen

Quoted 9 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml line 18

Quoted 6 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml new file +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

Quoted 2 lines

It is not duplicated, as my description in cover " This series add AST2600 i2c

new register set driver"

Quoted 6 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove

"bus".

Quoted 5 lines

Quoted 5 lines

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a

differentiating name.

Quoted 2 lines

For misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2, will it more clear way ?

I don't know. I still did not get answers. I asked here several questions.

Hello Krzysztof

Quoted 54 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 5:22 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 35 lines · Ryan Chen

Hello Krzysztof

Quoted 33 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 10 lines · Ryan Chen

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

Quoted 2 lines

It is not duplicated, as my description in cover " This series add AST2600 i2c

new register set driver"

Quoted 6 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove

"bus".

Quoted 5 lines

Quoted 5 lines

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a

Quoted 5 lines

differentiating name.

Quoted 2 lines

For misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2, will it more clear way ?

I don't know. I still did not get answers. I asked here several questions.

Those are different i2c controller, as I description in cover letter.

The i2c new register mode, there have two separate slave/master register. And different register with old register.

So now, avoid misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2.

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, February 17, 2023 4:37 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 65 lines · Ryan Chen

Hello Krzysztof

Quoted 63 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 5:22 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 37 lines · Ryan Chen

Hello Krzysztof

Quoted 35 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for
AST2600 i2C driver

Quoted 10 lines · Ryan Chen

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

Quoted 2 lines

It is not duplicated, as my description in cover " This series add AST2600 i2c

new register set driver"

Quoted 7 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove

"bus".

Quoted 6 lines

Quoted 6 lines

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a

differentiating name.

Quoted 2 lines

For misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2, will it more clear way ?

I don't know. I still did not get answers. I asked here several questions.

Quoted 1 line

Those are different i2c controller, as I description in cover letter.

The cover letter does not explain here anything. It barely mentions "new register set" and "separate register set". This is really short, so without proper explanations you will get all these questions. Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them? So many questions but cover letter is basically two sentences and here:

Quoted 2 lines

The i2c new register mode, there have two separate slave/master register. And different register with old register.

you repeat the same.

Quoted 2 lines

So now, avoid misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2.

Best regards,
Krzysztof

Hello Krzysztof,

Quoted 78 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, February 17, 2023 4:37 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 65 lines · Ryan Chen

Hello Krzysztof

Quoted 63 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 5:22 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 37 lines · Ryan Chen

Hello Krzysztof

Quoted 35 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for
AST2600 i2C driver

Quoted 10 lines · Ryan Chen

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

Quoted 2 lines

It is not duplicated, as my description in cover " This series add AST2600 i2c

new register set driver"

Quoted 7 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove

"bus".

Quoted 6 lines

Quoted 6 lines

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a

differentiating name.

Quoted 2 lines

For misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2, will it more clear way ?

I don't know. I still did not get answers. I asked here several questions.

Quoted 8 lines

Quoted 1 line

Those are different i2c controller, as I description in cover letter.

The cover letter does not explain here anything. It barely mentions "new register set" and "separate register set". This is really short, so without proper explanations you will get all these questions. Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them? So many questions but cover letter is basically two sentences and here:

Sorry my misunderstanding.
The legacy register layout is mix master/slave register control together. So will let confuse about register.
The following is add more detail description about new register layout. And new feature set add for register.

(quoted diff) +0 -7

-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 master and slave mode control.
-Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx.

And following is new register set for package transfer sequence.
New Master operation mode: S -> Aw -> P {S: Start, Sr: Repeat Start, Aw/r: Address for write/read, P: Stop}. New Master operation mode: S -> Aw -> TxD -> P
New Master operation mode: S -> Ar -> RxD -> P
New Slave operation mode: S -> Aw -> RxD -> Sr -> Ar -> TxD -> P.

(quoted diff) +0 -2

-Bus SDA lock auto-release capability for new master DMA command mode.
-Bus auto timeout for new master/slave DMA mode.

The following is two versus register layout.
Old:
{I2CD00}: Function Control Register
{I2CD04}: Clock and AC Timing Control Register \#1 {I2CD08}: Clock and AC Timing Control Register \#2 {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

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 20, 2023 3:59 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 100 lines · Ryan Chen

Hello Krzysztof,

Quoted 83 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, February 17, 2023 4:37 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 62 lines · Ryan Chen

Hello Krzysztof

Quoted 60 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 5:22 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for
AST2600 i2C driver

Quoted 38 lines · Ryan Chen

Hello Krzysztof

Quoted 36 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for
AST2600 i2C driver

Quoted 10 lines · Ryan Chen

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

Quoted 3 lines

It is not duplicated, as my description in cover " This series add
AST2600 i2c

new register set driver"

Quoted 7 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove

"bus".

Quoted 6 lines

Quoted 6 lines

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a

Quoted 6 lines

Quoted 5 lines

differentiating name.

Quoted 2 lines

For misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2, will it more clear way ?

I don't know. I still did not get answers. I asked here several questions.

Those are different i2c controller, as I description in cover letter.

Quoted 7 lines

The cover letter does not explain here anything. It barely mentions "new register set" and "separate register set". This is really short, so without proper explanations you will get all these questions. Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them? So many questions but cover letter is basically two sentences and
here:

Sorry my misunderstanding.
The legacy register layout is mix master/slave register control together. So will let confuse about register.
The following is add more detail description about new register layout. And new feature set add for register.

Quoted 3 lines

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

Quoted 7 lines

(quoted diff) +0 -4

-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 master and slave mode control.
-Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx.

And following is new register set for package transfer sequence. New Master operation mode: S -> Aw -> P {S: Start, Sr: Repeat Start, Aw/r:

Address for write/read, P: Stop}.

Quoted 36 lines

New Master operation mode: S -> Aw -> TxD -> P New Master operation
mode: S -> Ar -> RxD -> P New Slave operation mode: S -> Aw -> RxD -> Sr -> Ar -> TxD -> P.

(quoted diff) +0 -2

-Bus SDA lock auto-release capability for new master DMA command mode.
-Bus auto timeout for new master/slave DMA mode.

The following is two versus register layout.
Old:
{I2CD00}: Function Control Register
{I2CD04}: Clock and AC Timing Control Register \#1 {I2CD08}: Clock and AC Timing Control Register \#2 {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

Quoted 8 lines

{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

Thanks for explanation, yet still I don't get whether these are separate devices or not. So again, you got several questions and you should answer them, not only parts of them.

Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them?

I don't understand why we struggle so much to get basic info justifying this patchset.

Best regards,
Krzysztof

Hello Krzysztof,

Quoted 174 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 20, 2023 3:59 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 100 lines · Ryan Chen

Hello Krzysztof,

Quoted 83 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, February 17, 2023 4:37 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Quoted 62 lines · Ryan Chen

Hello Krzysztof

Quoted 60 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 5:22 PM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for
AST2600 i2C driver

Quoted 38 lines · Ryan Chen

Hello Krzysztof

Quoted 36 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, February 16, 2023 4:18 AM
To: Ryan Chen <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for
AST2600 i2C driver

Quoted 10 lines · Ryan Chen

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+  - $ref: /schemas/i2c/i2c-controller.yaml#
+
+properties:
+  compatible:
+    enum:
+      - aspeed,ast2600-i2c

NAK. It's already there. Please do not waste our time in submitting duplicated drivers.

Quoted 3 lines

It is not duplicated, as my description in cover " This series add
AST2600 i2c

new register set driver"

Quoted 7 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove

"bus".

Quoted 6 lines

Quoted 6 lines

Bindings are documenting hardware, so I claim - we already have this hardware described and this is duplicated. Otherwise - what are these two I2C controllers and what are the differences? Why they do not have really different name? Bus looks more like a mistake than a

Quoted 6 lines

Quoted 5 lines

differentiating name.

Quoted 2 lines

For misunderstanding, or mistaken.
I purpose to be aspeed,ast2600-i2cv2, will it more clear way ?

I don't know. I still did not get answers. I asked here several questions.

Those are different i2c controller, as I description in cover letter.

Quoted 7 lines

The cover letter does not explain here anything. It barely mentions "new register set" and "separate register set". This is really short, so without proper explanations you will get all these questions. Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them? So many questions but cover letter is basically two sentences and
here:

Sorry my misunderstanding.
The legacy register layout is mix master/slave register control together. So will let confuse about register.
The following is add more detail description about new register layout. And new feature set add for register.

Quoted 3 lines

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

Quoted 7 lines

(quoted diff) +0 -4

-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 master and slave mode control.
-Support 4 individual DMA buffers for master Tx and Rx, slave Tx and Rx.

And following is new register set for package transfer sequence. New Master operation mode: S -> Aw -> P {S: Start, Sr: Repeat Start, Aw/r:

Address for write/read, P: Stop}.

Quoted 36 lines

New Master operation mode: S -> Aw -> TxD -> P New Master operation
mode: S -> Ar -> RxD -> P New Slave operation mode: S -> Aw -> RxD -> Sr -> Ar -> TxD -> P.

(quoted diff) +0 -2

-Bus SDA lock auto-release capability for new master DMA command mode.
-Bus auto timeout for new master/slave DMA mode.

The following is two versus register layout.
Old:
{I2CD00}: Function Control Register
{I2CD04}: Clock and AC Timing Control Register \#1 {I2CD08}: Clock and AC Timing Control Register \#2 {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

Quoted 8 lines

{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

Thanks for explanation, yet still I don't get whether these are separate devices or not. So again, you got several questions and you should answer them, not only parts of them.

Quoted 2 lines

Are they compatible? Do they overlap? Are they completely different? If so, why datasheet uses same name for them?

They are not compatible. The register offset is overlap.
Old register is from 0x00 ~ 0x30
New register is from 0x00 ~ 0x54
The new design has another register, call global register that do 1 bit to set switch register decode to be new or old register layout.
For example, old register AC timing have two setting I2CD04/08 but new is I2CC04. And now register setting for AC timing.
And also master/slave register separate control. Not mix at I2CD14. About naming in datasheet, due to you can see the new design concept is the same.

So most use the same name to be offset register name.
From mix to separate master/slave control.
The following an example.
IER 1 to 2
{I2CD0C}: Interrupt Control Register to 2 set {I2CM10}: Master Interrupt Control Register, {I2CS20}: Slave~ Interrupt Control Register ISR, 1 to 2
{I2CD0C}: Interrupt Control Register -> {I2CM14}: Master Interrupt Status Register, {I2CS24}: Slave~ Interrupt Status Register And so on...

It is not duplicated, as my description in cover " This series add AST2600 i2c new register set driver" So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus".

Hi Ryan,

Quoted 7 lines · Ryan Chen

It is not duplicated, as my description in cover " This series add AST2600 i2c new register set driver" So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus".

Is it possible to keep existing driver drivers/i2c/busses/i2c-aspeed.c for ast2400/ast2500, while move ast2600 support to new driver altogether, say i2c-ast2600.c along with new register mode? By default new driver can support legacy mode with same compatible "aspeed,ast2600- i2c-bus", additionally driven by dt props, switch to new mode.

-----Original Message-----
From: Dhananjay Phadke <[email protected]>
Sent: Thursday, February 16, 2023 12:17 PM
To: Ryan Chen <[email protected]>; Krzysztof Kozlowski <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Joel Stanley <[email protected]>; Andrew Jeffery <[email protected]>; Philipp Zabel <[email protected]>; openbmc at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v4 2/3] dt-bindings: i2c-ast2600: Add support for AST2600 i2C driver

Hi Ryan,

Quoted 1 line · Ryan Chen

It is not duplicated, as my description in cover " This series add AST2600 i2c

new register set driver"

Quoted 6 lines

So, this will be different driver compatible.
The original compatible is
- aspeed,ast2400-i2c-bus
- aspeed,ast2500-i2c-bus
- aspeed,ast2600-i2c-bus
So the new register set compatible is "- aspeed,ast2600-i2c", remove "bus".

Is it possible to keep existing driver drivers/i2c/busses/i2c-aspeed.c for ast2400/ast2500, while move ast2600 support to new driver altogether, say i2c-ast2600.c along with new register mode? By default new driver can support legacy mode with same compatible "aspeed,ast2600- i2c-bus", additionally driven by dt props, switch to new mode.

Add i2c new register mode driver to support AST2600 i2c new register mode.

(quoted diff) +16 -0

+static int ast2600_i2c_global_probe(struct platform_device *pdev)
+{
+???????struct ast2600_i2c_global *i2c_global;
+???????struct resource *res;
+
+???????i2c_global = devm_kzalloc(&pdev->dev, sizeof(*i2c_global), GFP_KERNEL);
+???????if (IS_ERR(i2c_global))
+???????????????return PTR_ERR(i2c_global);
+
+???????res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+???????i2c_global->base = devm_ioremap_resource(&pdev->dev, res);
+???????if (IS_ERR(i2c_global->base))
+???????????????return PTR_ERR(i2c_global->base);
+
+???????i2c_global->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
+???????if (IS_ERR(i2c_global->rst))

drivers/i2c/busses/i2c-ast2600-global.c new file +4 -0

+
+static struct platform_driver ast2600_i2c_global_driver = {
+???????.probe? = ast2600_i2c_global_probe,
+???????.driver = {

(quoted diff) +4 -0

+enum xfer_mode {
+???????BYTE_MODE = 0,
+???????BUFF_MODE,
+???????DMA_MODE,

+???????/* 0: old mode, 1: new mode */ +???????int?????????????????????????????????????clk_div_mode;

(quoted diff) +57 -0

+static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
+{
+???????unsigned long base_clk1;
+???????unsigned long base_clk2;
+???????unsigned long base_clk3;
+???????unsigned long base_clk4;
+???????int baseclk_idx;
+???????u32 clk_div_reg;
+???????u32 scl_low;
+???????u32 scl_high;
+???????int divisor;
+???????int inc = 0;
+???????u32 data;
+???????int i;
+
+???????if (i2c_bus->clk_div_mode) {
+???????????????regmap_read(i2c_bus->global_reg, AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg);
+???????????????base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg & 0xff) + 2) * 10) / 2);
+???????????????base_clk2 = (i2c_bus->apb_clk * 10) /
+???????????????????????????????(((((clk_div_reg >> 8) & 0xff) + 2) * 10) / 2);
+???????????????base_clk3 = (i2c_bus->apb_clk * 10) /
+???????????????????????????????(((((clk_div_reg >> 16) & 0xff) + 2) * 10) / 2);
+???????????????base_clk4 = (i2c_bus->apb_clk * 10) /
+???????????????????????????????(((((clk_div_reg >> 24) & 0xff) + 2) * 10) / 2);
+
+???????????????if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <= 32) {
+???????????????????????baseclk_idx = 0;
+???????????????????????divisor = DIV_ROUND_UP(i2c_bus->apb_clk, i2c_bus->bus_frequency);
+???????????????} else if ((base_clk1 / i2c_bus->bus_frequency) <= 32) {
+???????????????????????baseclk_idx = 1;
+???????????????????????divisor = DIV_ROUND_UP(base_clk1, i2c_bus->bus_frequency);
+???????????????} else if ((base_clk2 / i2c_bus->bus_frequency) <= 32) {
+???????????????????????baseclk_idx = 2;
+???????????????????????divisor = DIV_ROUND_UP(base_clk2, i2c_bus->bus_frequency);
+???????????????} else if ((base_clk3 / i2c_bus->bus_frequency) <= 32) {
+???????????????????????baseclk_idx = 3;
+???????????????????????divisor = DIV_ROUND_UP(base_clk3, i2c_bus->bus_frequency);
+???????????????} else {
+???????????????????????baseclk_idx = 4;
+???????????????????????divisor = DIV_ROUND_UP(base_clk4, i2c_bus->bus_frequency);
+???????????????????????inc = 0;
+???????????????????????while ((divisor + inc) > 32) {
+???????????????????????????????inc |= divisor & 0x1;
+???????????????????????????????divisor >>= 1;
+???????????????????????????????baseclk_idx++;
+???????????????????????}
+???????????????????????divisor += inc;
+???????????????}
+???????????????divisor = min_t(int, divisor, 32);
+???????????????baseclk_idx &= 0xf;
+???????????????scl_low = ((divisor * 9) / 16) - 1;
+???????????????scl_low = min_t(u32, scl_low, 0xf);
+???????????????scl_high = (divisor - scl_low - 2) & 0xf;
+???????????????/* Divisor : Base Clock : tCKHighMin : tCK High : tCK Low? */
+???????????????data = ((scl_high - 1) << 20) | (scl_high << 16) | (scl_low << 12) | (baseclk_idx);
+???????????????if (i2c_bus->timeout_enable) {
+???????????????????????data |= AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK);

(quoted diff) +10 -0

+static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus)
+{
+???????struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+???????int xfer_len = 0;
+???????int i = 0;
+???????u32 cmd;
+
+???????cmd = AST2600_I2CM_PKT_EN | AST2600_I2CM_PKT_ADDR(msg->addr) | AST2600_I2CM_START_CMD;
+
+???????dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",

+???????????????} else if (i2c_bus->mode == BUFF_MODE) {

+???????????????} else {

+???????????????}
+???????} else {
+???????????????if (i2c_bus->mode == DMA_MODE) {

+???????????????} else if (i2c_bus->mode == BUFF_MODE) {

+???????????????} else {

+???????????????}
+???????}
+???????dev_dbg(i2c_bus->dev, "len %d , cmd %x\n", xfer_len, cmd); +???????writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); +???????return 0;
+}

drivers/i2c/busses/i2c-ast2600.c new file +4 -0

+
+static int ast2600_i2c_is_irq_error(u32 irq_status)
+{
+???????if (irq_status & AST2600_I2CM_ARBIT_LOSS)

(quoted diff) +49 -0

+static int ast2600_i2c_master_irq(struct ast2600_i2c_bus *i2c_bus)
+{
+???????u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????u32 ier = readl(i2c_bus->reg_base + AST2600_I2CM_IER);
+???????u32 ctrl = 0;
+
+???????dev_dbg(i2c_bus->dev, "M sts %x\n", sts);
+???????if (!i2c_bus->alert_enable)
+???????????????sts &= ~AST2600_I2CM_SMBUS_ALT;
+
+???????if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
+???????????????dev_dbg(i2c_bus->dev, "M clear isr: AST2600_I2CM_BUS_RECOVER_FAIL= %x\n", sts);
+???????????????writel(AST2600_I2CM_BUS_RECOVER_FAIL, i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????????????ctrl = readl(i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+???????????????writel(0, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+???????????????writel(ctrl, i2c_bus->reg_base + AST2600_I2CC_FUN_CTRL);
+???????????????i2c_bus->cmd_err = -EPROTO;
+???????????????complete(&i2c_bus->cmd_complete);
+???????????????return 1;
+???????}
+
+???????if (AST2600_I2CM_BUS_RECOVER & sts) {
+???????????????dev_dbg(i2c_bus->dev, "M clear isr: AST2600_I2CM_BUS_RECOVER= %x\n", sts);
+???????????????writel(AST2600_I2CM_BUS_RECOVER, i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????????????i2c_bus->cmd_err = 0;
+???????????????complete(&i2c_bus->cmd_complete);
+???????????????return 1;
+???????}
+
+???????if (AST2600_I2CM_SMBUS_ALT & sts) {
+???????????????if (ier & AST2600_I2CM_SMBUS_ALT) {
+???????????????????????dev_dbg(i2c_bus->dev, "M clear isr: AST2600_I2CM_SMBUS_ALT= %x\n", sts);
+???????????????????????/* Disable ALT INT */
+???????????????????????writel(ier & ~AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_IER);
+???????????????????????i2c_handle_smbus_alert(i2c_bus->ara);
+???????????????????????writel(AST2600_I2CM_SMBUS_ALT, i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????????????????????dev_err(i2c_bus->dev,
+???????????????????????????????"ast2600_master_alert_recv bus id %d, Disable Alt, Please Imple\n",
+???????????????????????????????i2c_bus->adap.nr);
+???????????????????????return 1;
+???????????????}
+???????}
+
+???????i2c_bus->cmd_err = ast2600_i2c_is_irq_error(sts);
+???????if (i2c_bus->cmd_err) {
+???????????????dev_dbg(i2c_bus->dev, "received error interrupt: 0x%02x\n", sts);
+???????????????writel(AST2600_I2CM_PKT_DONE, i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????????????complete(&i2c_bus->cmd_complete);
+???????????????return 1;

+???????????????writel(readl(i2c_bus->reg_base + AST2600_I2CM_ISR), +???????????????????????????????i2c_bus->reg_base + AST2600_I2CM_ISR);

(quoted diff) +36 -0

+static int ast2600_i2c_probe(struct platform_device *pdev)
+{
+???????struct ast2600_i2c_bus *i2c_bus;
+???????const struct of_device_id *match;
+???????struct resource *res;
+???????u32 global_ctrl;
+???????int ret = 0;
+
+???????i2c_bus = devm_kzalloc(&pdev->dev, sizeof(*i2c_bus), GFP_KERNEL);
+???????if (!i2c_bus)
+???????????????return -ENOMEM;
+
+???????i2c_bus->global_reg = syscon_regmap_lookup_by_compatible("aspeed,ast2600-i2c-global");
+???????if (IS_ERR(i2c_bus->global_reg)) {
+???????????????dev_err(&pdev->dev, "failed to find ast2600 i2c global regmap\n");
+???????????????ret = -ENOMEM;
+???????????????goto free_mem;
+???????}
+
+???????regmap_read(i2c_bus->global_reg, AST2600_I2CG_CTRL, &global_ctrl);
+
+???????if (global_ctrl & AST2600_I2CG_CTRL_NEW_CLK_DIV)
+???????????????i2c_bus->clk_div_mode = 1;
+
+???????if (!(global_ctrl & AST2600_I2CG_CTRL_NEW_REG)) {
+???????????????ret = -ENOENT;
+???????????????dev_err(&pdev->dev, "Expect I2CG0C[2] = 1 (new reg mode)\n");
+???????????????goto free_mem;
+???????}
+
+???????i2c_bus->mode = DMA_MODE;
+???????i2c_bus->slave_operate = 0;
+
+???????if (of_property_read_bool(pdev->dev.of_node, "byte-mode"))
+???????????????i2c_bus->mode = BYTE_MODE;
+

+
+???????if (of_property_read_bool(pdev->dev.of_node, "timeout")) +???????????????i2c_bus->timeout_enable = 1;

(quoted diff) +12 -0

+
+???????i2c_bus->dev = &pdev->dev;
+???????init_completion(&i2c_bus->cmd_complete);
+
+???????i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
+???????if (IS_ERR(i2c_bus->reg_base)) {
+???????????????ret = PTR_ERR(i2c_bus->reg_base);
+???????????????goto free_mem;
+???????}
+
+???????i2c_bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+???????if (i2c_bus->irq < 0) {

(quoted diff) +54 -0

+
+???????platform_set_drvdata(pdev, i2c_bus);
+
+???????i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
+???????if (IS_ERR(i2c_bus->clk)) {
+???????????????dev_err(i2c_bus->dev, "no clock defined\n");
+???????????????ret = -ENODEV;
+???????????????goto free_irq;
+???????}
+???????i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
+???????dev_dbg(i2c_bus->dev, "i2c_bus->apb_clk %d\n", i2c_bus->apb_clk);
+
+???????ret = of_property_read_u32(pdev->dev.of_node, "bus-frequency", &i2c_bus->bus_frequency);
+???????if (ret < 0) {
+???????????????dev_err(&pdev->dev, "Could not read bus-frequency property\n");
+???????????????i2c_bus->bus_frequency = 100000;
+???????}
+
+???????/* Initialize the I2C adapter */
+???????i2c_bus->adap.owner = THIS_MODULE;
+???????i2c_bus->adap.algo = &i2c_ast2600_algorithm;
+???????i2c_bus->adap.retries = 0;
+???????i2c_bus->adap.dev.parent = i2c_bus->dev;
+???????i2c_bus->adap.dev.of_node = pdev->dev.of_node;
+???????i2c_bus->adap.algo_data = i2c_bus;
+???????strscpy(i2c_bus->adap.name, pdev->name, sizeof(i2c_bus->adap.name));
+???????i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
+
+???????ast2600_i2c_init(i2c_bus);
+
+???????ret = devm_request_irq(&pdev->dev, i2c_bus->irq, ast2600_i2c_bus_irq, 0,
+????????????????????????????? dev_name(&pdev->dev), i2c_bus);
+???????if (ret < 0)
+???????????????goto unmap;
+
+???????if (of_property_read_bool(pdev->dev.of_node, "smbus-alert")) {
+???????????????i2c_bus->alert_enable = 1;
+???????????????i2c_bus->ara = i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data);
+???????????????if (!i2c_bus->ara)
+???????????????????????dev_warn(i2c_bus->dev, "Failed to register ARA client\n");
+
+???????????????writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT,
+????????????????????? i2c_bus->reg_base + AST2600_I2CM_IER);
+???????} else {
+???????????????i2c_bus->alert_enable = 0;
+???????????????/* Set interrupt generation of I2C master controller */
+???????????????writel(AST2600_I2CM_PKT_DONE | AST2600_I2CM_BUS_RECOVER,
+???????????????????????????????i2c_bus->reg_base + AST2600_I2CM_IER);
+???????}
+
+???????ret = i2c_add_adapter(&i2c_bus->adap);
+???????if (ret < 0)
+???????????????goto unmap;
+

+free_mem:
+???????kfree(i2c_bus);

Quoted 3 lines

Add i2c new register mode driver to support AST2600 i2c new register mode.

Nice!

A bit of review below - I assume your state machine & event handling is correct, but this is more on the driver API side.

Quoted 23 lines

(quoted diff) +23 -0

+static int ast2600_i2c_global_probe(struct platform_device *pdev) {
+???????struct ast2600_i2c_global *i2c_global;
+???????struct resource *res;
+
+???????i2c_global = devm_kzalloc(&pdev->dev, sizeof(*i2c_global),
+GFP_KERNEL);
+???????if (IS_ERR(i2c_global))
+???????????????return PTR_ERR(i2c_global);
+
+???????res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+???????i2c_global->base = devm_ioremap_resource(&pdev->dev, res);
+???????if (IS_ERR(i2c_global->base))
+???????????????return PTR_ERR(i2c_global->base);
+
+???????i2c_global->rst = devm_reset_control_get_exclusive(&pdev->dev,
+NULL);
+???????if (IS_ERR(i2c_global->rst))
+???????????????return IS_ERR(i2c_global->rst);
+
+???????reset_control_assert(i2c_global->rst);
+???????udelay(3);
+???????reset_control_deassert(i2c_global->rst);
+

Review

Quoted 6 lines

+???????writel(AST2600_GLOBAL_INIT, i2c_global->base +

(quoted diff) +5 -0

+AST2600_I2CG_CTRL);
+???????writel(I2CCG_DIV_CTRL, i2c_global->base +
+AST2600_I2CG_CLK_DIV_CTRL);
+
+???????pr_info("i2c global registered\n");

I'm not the printk police, but I don't think this message is very useful.

drivers/i2c/busses/i2c-ast2600-global.c line 76

Quoted 4 lines

drivers/i2c/busses/i2c-ast2600-global.c new file +3 -0

+
+???????return 0;
+}

... so this driver is doing some common reset bits then just the two register writes - is it possible to just use a common syscon/mfd device instead? The core i2c driver could handle the reg writes through the syscon regmap instead.

Quoted 7 lines

(quoted diff) +7 -0

+
+static struct platform_driver ast2600_i2c_global_driver = {
+???????.probe? = ast2600_i2c_global_probe,
+???????.driver = {
+???????????????.name = KBUILD_MODNAME,
+???????????????.of_match_table = ast2600_i2c_global_of_match,
+???????},

drivers/i2c/busses/i2c-ast2600-global.c line 90

Quoted 6 lines

drivers/i2c/busses/i2c-ast2600-global.c new file +6 -0

+};
+
+static int __init ast2600_i2c_global_init(void) {
+???????return platform_driver_register(&ast2600_i2c_global_driver);
+}
+device_initcall(ast2600_i2c_global_init);

Maybe module_platform_driver() instead?

Quoted 5 lines

(quoted diff) +4 -0

+enum xfer_mode {
+???????BYTE_MODE = 0,
+???????BUFF_MODE,
+???????DMA_MODE,

Review

Quoted 10 lines

(quoted diff) +10 -0

+};
+
+struct ast2600_i2c_bus {
+???????struct i2c_adapter??????adap;
+???????struct device???????????*dev;
+???????void __iomem????????????*reg_base;
+???????struct regmap???????????*global_reg;
+???????int?????????????????????????????????????irq;
+???????/* 0: dma, 1: pool, 2:byte */
+???????enum xfer_mode??????????mode;

The comment doesn't match the enum.

Quoted 3 lines

+???????/* 0: old mode, 1: new mode */

+???????int?????????????????????????????????????clk_div _mode;

... and if you're using an enum for xfer_mode, may as well do the same for clk_div_mode, which will make some of the conditionals below more obvious.

Quoted 12 lines

(quoted diff) +12 -0

+static u32 ast2600_select_i2c_clock(struct ast2600_i2c_bus *i2c_bus)
+{
+???????unsigned long base_clk1;
+???????unsigned long base_clk2;
+???????unsigned long base_clk3;
+???????unsigned long base_clk4;
+???????int baseclk_idx;
+???????u32 clk_div_reg;
+???????u32 scl_low;
+???????u32 scl_high;
+???????int divisor;
+???????int inc = 0;

Review

Quoted 10 lines

(quoted diff) +10 -0

+???????u32 data;
+???????int i;
+
+???????if (i2c_bus->clk_div_mode) {
+???????????????regmap_read(i2c_bus->global_reg,
+AST2600_I2CG_CLK_DIV_CTRL, &clk_div_reg);
+???????????????base_clk1 = (i2c_bus->apb_clk * 10) / ((((clk_div_reg
+& 0xff) + 2) * 10) / 2);
+???????????????base_clk2 = (i2c_bus->apb_clk * 10) /
+???????????????????????????????(((((clk_div_reg >> 8) & 0xff)

+ 2) *

Quoted 3 lines

+10) / 2);
+???????????????base_clk3 = (i2c_bus->apb_clk * 10) / +???????????????????????????????(((((clk_div_reg >> 16) &

0xff) + 2) *

Quoted 3 lines

+10) / 2);
+???????????????base_clk4 = (i2c_bus->apb_clk * 10) / +???????????????????????????????(((((clk_div_reg >> 24) &

0xff) + 2) *

Quoted 3 lines

+10) / 2);
+
+???????????????if ((i2c_bus->apb_clk / i2c_bus->bus_frequency) <=

32)

Quoted 3 lines

+{
+???????????????????????baseclk_idx = 0;
+???????????????????????divisor =

DIV_ROUND_UP(i2c_bus->apb_clk,

Quoted 2 lines

+i2c_bus->bus_frequency);
+???????????????} else if ((base_clk1 / i2c_bus->bus_frequency) <=

32)

Review

Quoted 5 lines

(quoted diff) +5 -0

+{
+???????????????????????baseclk_idx = 1;
+???????????????????????divisor = DIV_ROUND_UP(base_clk1,
+i2c_bus->bus_frequency);
+???????????????} else if ((base_clk2 / i2c_bus->bus_frequency) <=

32)

Review

Quoted 5 lines

(quoted diff) +5 -0

+{
+???????????????????????baseclk_idx = 2;
+???????????????????????divisor = DIV_ROUND_UP(base_clk2,
+i2c_bus->bus_frequency);
+???????????????} else if ((base_clk3 / i2c_bus->bus_frequency) <=

32)

Quoted 35 lines

(quoted diff) +35 -0

+{
+???????????????????????baseclk_idx = 3;
+???????????????????????divisor = DIV_ROUND_UP(base_clk3,
+i2c_bus->bus_frequency);
+???????????????} else {
+???????????????????????baseclk_idx = 4;
+???????????????????????divisor = DIV_ROUND_UP(base_clk4,
+i2c_bus->bus_frequency);
+???????????????????????inc = 0;
+???????????????????????while ((divisor + inc) > 32) {
+???????????????????????????????inc |= divisor & 0x1;
+???????????????????????????????divisor >>= 1;
+???????????????????????????????baseclk_idx++;
+???????????????????????}
+???????????????????????divisor += inc;
+???????????????}
+???????????????divisor = min_t(int, divisor, 32);
+???????????????baseclk_idx &= 0xf;
+???????????????scl_low = ((divisor * 9) / 16) - 1;
+???????????????scl_low = min_t(u32, scl_low, 0xf);
+???????????????scl_high = (divisor - scl_low - 2) & 0xf;
+???????????????/* Divisor : Base Clock : tCKHighMin : tCK High : tCK
+Low? */
+???????????????data = ((scl_high - 1) << 20) | (scl_high << 16) |
+(scl_low << 12) | (baseclk_idx);
+???????????????if (i2c_bus->timeout_enable) {
+???????????????????????data |=
+AST2600_I2CC_TOUTBASECLK(AST_I2C_TIMEOUT_CLK);
+???????????????????????data |=
+AST2600_I2CC_TTIMEOUT(AST_I2C_TIMEOUT_COUNT);
+???????????????}
+???????} else {
+???????????????for (i = 0; i < ARRAY_SIZE(i2c_legacy_timing_table);
+i++) {
+???????????????????????if ((i2c_bus->apb_clk /

Quoted 6 lines

+i2c_legacy_timing_table[i].divisor) < +?????????????????????????? i2c_bus->bus_frequency) { +???????????????????????????????break; +???????????????????????}
+???????????????}
+???????????????data = i2c_legacy_timing_table[i].timing;

If you don't match a timing, this will overflow i2c_legacy_timing_table.

Review

Quoted 8 lines

(quoted diff) +7 -0

+static int ast2600_i2c_do_start(struct ast2600_i2c_bus *i2c_bus) {
+???????struct i2c_msg *msg = &i2c_bus->msgs[i2c_bus->msgs_index];
+???????int xfer_len = 0;
+???????int i = 0;
+???????u32 cmd;
+
+???????cmd = AST2600_I2CM_PKT_EN |

AST2600_I2CM_PKT_ADDR(msg->addr) |

Review

Quoted 4 lines

(quoted diff) +4 -0

+AST2600_I2CM_START_CMD;
+
+???????dev_dbg(i2c_bus->dev, "[%d] %sing %d byte%s %s 0x%02x\n",
+???????????????i2c_bus->msgs_index, msg->flags & I2C_M_RD ?

"read" :

Review

Quoted 10 lines

+"write",
+???????????????msg->len, msg->len > 1 ? "s" : "", +???????????????msg->flags & I2C_M_RD ? "from" : "to", msg->addr);

(quoted diff) +7 -0

+
+???????i2c_bus->master_xfer_cnt = 0;
+???????i2c_bus->buf_index = 0;
+
+???????if (msg->flags & I2C_M_RD) {
+???????????????cmd |= AST2600_I2CM_RX_CMD;
+???????????????if (i2c_bus->mode == DMA_MODE) {

                        [...]

Quoted 1 line

+???????????????} else if (i2c_bus->mode == BUFF_MODE) {

                        [...]

Quoted 1 line

+???????????????} else {

                        [...]

Quoted 3 lines

+???????????????}
+???????} else {
+???????????????if (i2c_bus->mode == DMA_MODE) {

                        [...]

Quoted 1 line

+???????????????} else if (i2c_bus->mode == BUFF_MODE) {

                        [...]

Quoted 1 line

+???????????????} else {

                        [...]

Quoted 6 lines

+???????????????}
+???????}
+???????dev_dbg(i2c_bus->dev, "len %d , cmd %x\n", xfer_len, cmd); +???????writel(cmd, i2c_bus->reg_base + AST2600_I2CM_CMD_STS); +???????return 0;
+}

The core parts of this function are essentially split into three, depending on the bus mode. Maybe it would be better to refactor those into separate functions for buffer handling?

Quoted 6 lines

(quoted diff) +5 -0

+
+static int ast2600_i2c_is_irq_error(u32 irq_status) {
+???????if (irq_status & AST2600_I2CM_ARBIT_LOSS)
+???????????????return -EAGAIN;
+???????if (irq_status & (AST2600_I2CM_SDA_DL_TO |

Review

Quoted 10 lines

(quoted diff) +10 -0

+AST2600_I2CM_SCL_LOW_TO))
+???????????????return -EBUSY;
+???????if (irq_status & (AST2600_I2CM_ABNORMAL))
+???????????????return -EPROTO;
+
+???????return 0;
+}
+
+static void ast2600_i2c_master_package_irq(struct ast2600_i2c_bus
+*i2c_bus, u32 sts)

What does 'package' refer to here?

Same as above with this one - there's a lot of inner conditionals on the bus modes.

Quoted 8 lines

(quoted diff) +7 -0

+static int ast2600_i2c_master_irq(struct ast2600_i2c_bus *i2c_bus) {
+???????u32 sts = readl(i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????u32 ier = readl(i2c_bus->reg_base + AST2600_I2CM_IER);
+???????u32 ctrl = 0;
+
+???????dev_dbg(i2c_bus->dev, "M sts %x\n", sts);
+???????if (!i2c_bus->alert_enable)

Review

Quoted 10 lines

(quoted diff) +10 -0

+???????????????sts &= ~AST2600_I2CM_SMBUS_ALT;
+
+???????if (AST2600_I2CM_BUS_RECOVER_FAIL & sts) {
+???????????????dev_dbg(i2c_bus->dev, "M clear isr:
+AST2600_I2CM_BUS_RECOVER_FAIL= %x\n", sts);
+???????????????writel(AST2600_I2CM_BUS_RECOVER_FAIL,
+i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????????????ctrl = readl(i2c_bus->reg_base +
+AST2600_I2CC_FUN_CTRL);
+???????????????writel(0, i2c_bus->reg_base +

AST2600_I2CC_FUN_CTRL);

Review

Quoted 11 lines

+???????????????writel(ctrl, i2c_bus->reg_base +

(quoted diff) +10 -0

+AST2600_I2CC_FUN_CTRL);
+???????????????i2c_bus->cmd_err = -EPROTO;
+???????????????complete(&i2c_bus->cmd_complete);
+???????????????return 1;
+???????}
+
+???????if (AST2600_I2CM_BUS_RECOVER & sts) {
+???????????????dev_dbg(i2c_bus->dev, "M clear isr:
+AST2600_I2CM_BUS_RECOVER= %x\n", sts);
+???????????????writel(AST2600_I2CM_BUS_RECOVER,

i2c_bus->reg_base +

Review

Quoted 12 lines

(quoted diff) +12 -0

+AST2600_I2CM_ISR);
+???????????????i2c_bus->cmd_err = 0;
+???????????????complete(&i2c_bus->cmd_complete);
+???????????????return 1;
+???????}
+
+???????if (AST2600_I2CM_SMBUS_ALT & sts) {
+???????????????if (ier & AST2600_I2CM_SMBUS_ALT) {
+???????????????????????dev_dbg(i2c_bus->dev, "M clear isr:
+AST2600_I2CM_SMBUS_ALT= %x\n", sts);
+???????????????????????/* Disable ALT INT */
+???????????????????????writel(ier &

~AST2600_I2CM_SMBUS_ALT,

Review

Quoted 6 lines

(quoted diff) +5 -0

+i2c_bus->reg_base + AST2600_I2CM_IER);
+???????????????????????i2c_handle_smbus_alert(i2c_bus->ara);
+???????????????????????writel(AST2600_I2CM_SMBUS_ALT,
+i2c_bus->reg_base + AST2600_I2CM_ISR);
+???????????????????????dev_err(i2c_bus->dev,

+???????????????????????????????"ast2600_master_alert_recv bus id %d,

Review

Quoted 11 lines

(quoted diff) +11 -0

+Disable Alt, Please Imple\n",
+???????????????????????????????i2c_bus->adap.nr);
+???????????????????????return 1;
+???????????????}
+???????}
+
+???????i2c_bus->cmd_err = ast2600_i2c_is_irq_error(sts);
+???????if (i2c_bus->cmd_err) {
+???????????????dev_dbg(i2c_bus->dev, "received error interrupt:
+0x%02x\n", sts);
+???????????????writel(AST2600_I2CM_PKT_DONE,

i2c_bus->reg_base +

Review

Quoted 12 lines

(quoted diff) +12 -0

+AST2600_I2CM_ISR);
+???????????????complete(&i2c_bus->cmd_complete);
+???????????????return 1;
+???????}
+
+???????if (AST2600_I2CM_PKT_DONE & sts) {
+???????????????ast2600_i2c_master_package_irq(i2c_bus, sts);
+???????????????return 1;
+???????}
+
+???????if (readl(i2c_bus->reg_base + AST2600_I2CM_ISR)) {
+???????????????dev_dbg(i2c_bus->dev, "master TODO care sts

%x\n",

Quoted 1 line

+???????????????????????readl(i2c_bus->reg_base +

AST2600_I2CM_ISR));

You're reading the same register twice here, is that intentional?

Quoted 2 lines

+???????????????writel(readl(i2c_bus->reg_base +

AST2600_I2CM_ISR),

Quoted 1 line

+???????????????????????????????i2c_bus->reg_base +

AST2600_I2CM_ISR);

Quoted 1 line

make that three times :)

Quoted 6 lines

(quoted diff) +6 -0

+static int ast2600_i2c_probe(struct platform_device *pdev) {
+???????struct ast2600_i2c_bus *i2c_bus;
+???????const struct of_device_id *match;
+???????struct resource *res;
+???????u32 global_ctrl;
+???????int ret = 0;

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+???????i2c_bus = devm_kzalloc(&pdev->dev, sizeof(*i2c_bus),
+GFP_KERNEL);
+???????if (!i2c_bus)
+???????????????return -ENOMEM;
+
+???????i2c_bus->global_reg =
+syscon_regmap_lookup_by_compatible("aspeed,ast2600-i2c-global");
+???????if (IS_ERR(i2c_bus->global_reg)) {
+???????????????dev_err(&pdev->dev, "failed to find ast2600 i2c

global

Quoted 4 lines

(quoted diff) +4 -0

+regmap\n");
+???????????????ret = -ENOMEM;
+???????????????goto free_mem;
+???????}

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+???????regmap_read(i2c_bus->global_reg, AST2600_I2CG_CTRL,
+&global_ctrl);
+
+???????if (global_ctrl & AST2600_I2CG_CTRL_NEW_CLK_DIV)
+???????????????i2c_bus->clk_div_mode = 1;
+
+???????if (!(global_ctrl & AST2600_I2CG_CTRL_NEW_REG)) {
+???????????????ret = -ENOENT;
+???????????????dev_err(&pdev->dev, "Expect I2CG0C[2] = 1 (new

reg

Review

Quoted 12 lines

(quoted diff) +12 -0

+mode)\n");
+???????????????goto free_mem;
+???????}
+
+???????i2c_bus->mode = DMA_MODE;
+???????i2c_bus->slave_operate = 0;
+
+???????if (of_property_read_bool(pdev->dev.of_node, "byte-mode"))
+???????????????i2c_bus->mode = BYTE_MODE;
+
+???????if (of_property_read_bool(pdev->dev.of_node, "buff-mode")) {
+???????????????res = platform_get_resource(pdev,

IORESOURCE_MEM, 1);

Review

Quoted 6 lines

+???????????????if (res && resource_size(res) >= 2) +???????????????????????i2c_bus->buf_base =

(quoted diff) +4 -0

+devm_ioremap_resource(&pdev->dev, res);
+
+???????????????if (!IS_ERR_OR_NULL(i2c_bus->buf_base))
+???????????????????????i2c_bus->buf_size = resource_size(res) /

2;

Quoted 3 lines

+
+???????????????i2c_bus->mode = BUFF_MODE;
+???????}

The buff-mode and byte-mode properties do not appear in your OF binding document. Using booleans to indicate mutually-exclusive modes seems a bit odd.

Quoted 3 lines

+
+???????if (of_property_read_bool(pdev->dev.of_node, "timeout")) +???????????????i2c_bus->timeout_enable = 1;

'timeout' is also not described in the binding.

Quoted 18 lines

(quoted diff) +17 -0

+
+???????i2c_bus->dev = &pdev->dev;
+???????init_completion(&i2c_bus->cmd_complete);
+
+???????i2c_bus->reg_base = devm_platform_ioremap_resource(pdev, 0);
+???????if (IS_ERR(i2c_bus->reg_base)) {
+???????????????ret = PTR_ERR(i2c_bus->reg_base);
+???????????????goto free_mem;
+???????}
+
+???????i2c_bus->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
+???????if (i2c_bus->irq < 0) {
+???????????????dev_err(&pdev->dev, "no irq specified\n");
+???????????????ret = -i2c_bus->irq;
+???????????????goto free_irq;
+???????}
+

Quoted 6 lines

+???????match = of_match_node(ast2600_i2c_bus_of_table, +pdev->dev.of_node);
+???????if (!match) {
+???????????????ret = -ENOENT; +???????????????goto free_irq;
+???????}

You don't use 'match' after this.

Quoted 28 lines

(quoted diff) +27 -0

+
+???????platform_set_drvdata(pdev, i2c_bus);
+
+???????i2c_bus->clk = devm_clk_get(i2c_bus->dev, NULL);
+???????if (IS_ERR(i2c_bus->clk)) {
+???????????????dev_err(i2c_bus->dev, "no clock defined\n");
+???????????????ret = -ENODEV;
+???????????????goto free_irq;
+???????}
+???????i2c_bus->apb_clk = clk_get_rate(i2c_bus->clk);
+???????dev_dbg(i2c_bus->dev, "i2c_bus->apb_clk %d\n",
+i2c_bus->apb_clk);
+
+???????ret = of_property_read_u32(pdev->dev.of_node, "bus-frequency",
+&i2c_bus->bus_frequency);
+???????if (ret < 0) {
+???????????????dev_err(&pdev->dev, "Could not read bus-frequency
+property\n");
+???????????????i2c_bus->bus_frequency = 100000;
+???????}
+
+???????/* Initialize the I2C adapter */
+???????i2c_bus->adap.owner = THIS_MODULE;
+???????i2c_bus->adap.algo = &i2c_ast2600_algorithm;
+???????i2c_bus->adap.retries = 0;
+???????i2c_bus->adap.dev.parent = i2c_bus->dev;
+???????i2c_bus->adap.dev.of_node = pdev->dev.of_node;

Review

Quoted 10 lines

(quoted diff) +10 -0

+???????i2c_bus->adap.algo_data = i2c_bus;
+???????strscpy(i2c_bus->adap.name, pdev->name,
+sizeof(i2c_bus->adap.name));
+???????i2c_set_adapdata(&i2c_bus->adap, i2c_bus);
+
+???????ast2600_i2c_init(i2c_bus);
+
+???????ret = devm_request_irq(&pdev->dev, i2c_bus->irq,
+ast2600_i2c_bus_irq, 0,
+????????????????????????????? dev_name(&pdev->dev),

i2c_bus);

Review

Quoted 9 lines

+???????if (ret < 0)
+???????????????goto unmap;

(quoted diff) +7 -0

+
+???????if (of_property_read_bool(pdev->dev.of_node, "smbus-alert")) {
+???????????????i2c_bus->alert_enable = 1;
+???????????????i2c_bus->ara =
+i2c_new_smbus_alert_device(&i2c_bus->adap, &i2c_bus->alert_data);
+???????????????if (!i2c_bus->ara)
+???????????????????????dev_warn(i2c_bus->dev, "Failed to

register ARA

Review

Quoted 12 lines

(quoted diff) +12 -0

+client\n");
+
+???????????????writel(AST2600_I2CM_PKT_DONE |
+AST2600_I2CM_BUS_RECOVER | AST2600_I2CM_SMBUS_ALT,
+????????????????????? i2c_bus->reg_base + AST2600_I2CM_IER);
+???????} else {
+???????????????i2c_bus->alert_enable = 0;
+???????????????/* Set interrupt generation of I2C master controller
+*/
+???????????????writel(AST2600_I2CM_PKT_DONE |
+AST2600_I2CM_BUS_RECOVER,
+???????????????????????????????i2c_bus->reg_base +

AST2600_I2CM_IER);

Quoted 8 lines

+???????}

(quoted diff) +7 -0

+
+???????ret = i2c_add_adapter(&i2c_bus->adap);
+???????if (ret < 0)
+???????????????goto unmap;
+
+???????dev_info(i2c_bus->dev, "%s [%d]: adapter [%d khz] mode
+[%d]\n",

Review

Quoted 10 lines

(quoted diff) +10 -0

+??????????????? pdev->dev.of_node->name, i2c_bus->adap.nr,
+i2c_bus->bus_frequency / 1000,
+??????????????? i2c_bus->mode);
+
+???????return 0;
+
+unmap:
+???????free_irq(i2c_bus->irq, i2c_bus);
+free_irq:
+???????devm_iounmap(&pdev->dev, i2c_bus->reg_base);

Something looks off in these goto labels.

Quoted 2 lines

+free_mem:
+???????kfree(i2c_bus);

you devm_kzalloc()ed above, but are kfree()ing here (as well as in the _remove function). Same with devm_request_irq(). You could just drop these?

Quoted 14 lines

drivers/i2c/busses/i2c-ast2600-global.c line 90

Quoted 12 lines

Quoted 1 line

+static struct platform_driver ast2600_i2c_global_driver = {

+???????.probe? = ast2600_i2c_global_probe, +???????.driver = {
+???????????????.name = KBUILD_MODNAME, +???????????????.of_match_table = ast2600_i2c_global_of_match, +???????},

drivers/i2c/busses/i2c-ast2600-global.c new file +6 -0

+};
+
+static int __init ast2600_i2c_global_init(void) {
+???????return platform_driver_register(&ast2600_i2c_global_driver);
+}
+device_initcall(ast2600_i2c_global_init);

Maybe module_platform_driver() instead?

Due to i2c global is top of all i2c bus like the scu, it make sure the driver is before the i2c bus driver probe. It is needed use device_initcal function.

Quoted 15 lines

Quoted 14 lines

drivers/i2c/busses/i2c-ast2600-global.c line 90

Quoted 12 lines

Quoted 1 line

+static struct platform_driver ast2600_i2c_global_driver = {

+???????.probe? = ast2600_i2c_global_probe, +???????.driver = {
+???????????????.name = KBUILD_MODNAME, +???????????????.of_match_table = ast2600_i2c_global_of_match, +???????},

drivers/i2c/busses/i2c-ast2600-global.c new file +6 -0

+};
+
+static int __init ast2600_i2c_global_init(void) {
+???????return platform_driver_register(&ast2600_i2c_global_driver);
+}
+device_initcall(ast2600_i2c_global_init);

Maybe module_platform_driver() instead?

Quoted 1 line

Due to i2c global is top of all i2c bus like the scu, it make sure the driver is

before the i2c bus driver probe.

Quoted 1 line

It is needed use device_initcal function.

No, we do not do such ordering. device links and proper resource sharing handle the order. This must be module_platform_driver() (assuming we do not drop entire submission).

Best regards,
Krzysztof

Quoted 7 lines · Ryan Chen

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

+static struct platform_driver ast2600_i2c_global_driver = {

+???????.probe? = ast2600_i2c_global_probe,
+???????.driver = {
+???????????????.name = KBUILD_MODNAME,
+???????????????.of_match_table =

ast2600_i2c_global_of_match,

Quoted 16 lines

Quoted 11 lines

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600-global.c line 90

Quoted 8 lines

+???????},

drivers/i2c/busses/i2c-ast2600-global.c new file +7 -0

+};
+
+static int __init ast2600_i2c_global_init(void) {
+???????return
+platform_driver_register(&ast2600_i2c_global_driver);
+}
+device_initcall(ast2600_i2c_global_init);

Maybe module_platform_driver() instead?

Quoted 2 lines

Due to i2c global is top of all i2c bus like the scu, it make sure the driver is

before the i2c bus driver probe.

Quoted 1 line

It is needed use device_initcal function.

Quoted 3 lines

No, we do not do such ordering. device links and proper resource sharing handle the order. This must be module_platform_driver() (assuming we do not drop entire submission).

If global driver use module_platform_driver, does i2c driver will latter the global driver probe?
If yes, I think it will workable.

Quoted 1 line

Hello Krzysztof,

Quoted 29 lines

Quoted 7 lines · Ryan Chen

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

+static struct platform_driver ast2600_i2c_global_driver = {

+???????.probe? = ast2600_i2c_global_probe,
+???????.driver = {
+???????????????.name = KBUILD_MODNAME,
+???????????????.of_match_table =

ast2600_i2c_global_of_match,

Quoted 19 lines

Quoted 11 lines

Quoted 10 lines

drivers/i2c/busses/i2c-ast2600-global.c line 90

Quoted 8 lines

+???????},

drivers/i2c/busses/i2c-ast2600-global.c new file +7 -0

+};
+
+static int __init ast2600_i2c_global_init(void) {
+???????return
+platform_driver_register(&ast2600_i2c_global_driver);
+}
+device_initcall(ast2600_i2c_global_init);

Maybe module_platform_driver() instead?

Quoted 2 lines

Due to i2c global is top of all i2c bus like the scu, it make sure the driver is

before the i2c bus driver probe.

Quoted 1 line

It is needed use device_initcal function.

No, we do not do such ordering. device links and proper resource sharing handle the order. This must be module_platform_driver() (assuming we do not drop entire submission).

Quoted 4 lines

If global driver use module_platform_driver, does i2c driver will latter the global driver probe?
If yes, I think it will workable.

Quoted 1 line

I find a good way instead. subsys_initcall(ast2600_i2c_global_init); is better ?

Quoted 30 lines

Hello Krzysztof,

Quoted 29 lines

Quoted 7 lines · Ryan Chen

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

+static struct platform_driver ast2600_i2c_global_driver = {

+???????.probe? = ast2600_i2c_global_probe,
+???????.driver = {
+???????????????.name = KBUILD_MODNAME,
+???????????????.of_match_table =

ast2600_i2c_global_of_match,

Quoted 19 lines

Quoted 11 lines

Quoted 10 lines

Review

Quoted 8 lines

+???????},

(quoted diff) +7 -0

+};
+
+static int __init ast2600_i2c_global_init(void) {
+???????return
+platform_driver_register(&ast2600_i2c_global_driver);
+}
+device_initcall(ast2600_i2c_global_init);

Maybe module_platform_driver() instead?

Quoted 2 lines

Due to i2c global is top of all i2c bus like the scu, it make sure the driver is

before the i2c bus driver probe.

Quoted 1 line

It is needed use device_initcal function.

No, we do not do such ordering. device links and proper resource sharing handle the order. This must be module_platform_driver() (assuming we do not drop entire submission).

Quoted 6 lines

Quoted 4 lines

If global driver use module_platform_driver, does i2c driver will latter the global driver probe?
If yes, I think it will workable.

Quoted 1 line

I find a good way instead. subsys_initcall(ast2600_i2c_global_init); is better ?

Not really, this is not a subsystem. This is module platform driver so use it.

Hello Krzysztof,

Quoted 36 lines · Ryan Chen

Quoted 34 lines

Hello Krzysztof,

Quoted 33 lines

Quoted 7 lines · Ryan Chen

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

+static struct platform_driver ast2600_i2c_global_driver = {

+???????.probe? = ast2600_i2c_global_probe,
+???????.driver = {
+???????????????.name = KBUILD_MODNAME,
+???????????????.of_match_table =

ast2600_i2c_global_of_match,

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Review

Quoted 8 lines

+???????},

(quoted diff) +7 -0

+};
+
+static int __init ast2600_i2c_global_init(void) {
+???????return
+platform_driver_register(&ast2600_i2c_global_driver);
+}
+device_initcall(ast2600_i2c_global_init);

Maybe module_platform_driver() instead?

Quoted 8 lines

Quoted 2 lines

Due to i2c global is top of all i2c bus like the scu, it make sure the driver is

before the i2c bus driver probe.

Quoted 1 line

It is needed use device_initcal function.

No, we do not do such ordering. device links and proper resource sharing handle the order. This must be module_platform_driver() (assuming we do not drop entire submission).

If global driver use module_platform_driver, does i2c driver will latter the global driver probe?
If yes, I think it will workable.

Quoted 1 line

Quoted 5 lines

Quoted 2 lines

I find a good way instead. subsys_initcall(ast2600_i2c_global_init); is better ?

Not really, this is not a subsystem. This is module platform driver so use it.

But, I need i2c-global to be early then i2c-ast2600 driver. The subsys_initcall is needed. I2c-global driver to initial full i2c clock and reset, after this, i2c bus driver(i2c-ast2600.c) can work.

So I suggest to use subsys_initcall in i2c global driver. Is it ok?

Add i2c new register mode driver to support AST2600 i2c new register mode. AST2600 i2c controller have legacy and new register mode. The new register mode have global register support 4 base clock for scl clock selection, and new clock divider mode. The i2c new register mode have separate register set to control i2c master and slave.

Signed-off-by Ryan Chen <[email protected]>

---
 MAINTAINERS                             |   10 +
 drivers/i2c/busses/Kconfig              |   11 +
 drivers/i2c/busses/Makefile             |    1 +
 drivers/i2c/busses/i2c-ast2600-global.c |   94 ++
 drivers/i2c/busses/i2c-ast2600-global.h |   19 +
 drivers/i2c/busses/i2c-ast2600.c        | 1811 +++++++++++++++++++++++
 6 files changed, 1946 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ast2600-global.c
 create mode 100644 drivers/i2c/busses/i2c-ast2600-global.h
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

MAINTAINERS +10 -0

index 749710e22b4d..67d338d834b4 100644
@@ -3209,6 +3209,16 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/usb/aspeed,ast2600-udc.yaml
 F:	drivers/usb/gadget/udc/aspeed_udc.c
 
+ASPEED AST2600 I2C DRIVER
+M:	Ryan Chen <[email protected]>
+L:	linux-aspeed at lists.ozlabs.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600-global.yaml
+F:	Documentation/devicetree/bindings/i2c/aspeed,i2c-ast2600.yaml
+F:	drivers/i2c/busses/i2c-ast2600-global.c
+F:	drivers/i2c/busses/i2c-ast2600-global.h
+F:	drivers/i2c/busses/i2c-ast2600.c
+
 ASPEED XDMA ENGINE DRIVER
 M:	Eddie James <[email protected]>
 L:	linux-aspeed at lists.ozlabs.org (moderated for non-subscribers)

drivers/i2c/busses/Kconfig +0 -0

index 7284206b278b..5ef3ee6aa70f 100644

drivers/i2c/busses/Kconfig +8 -0

+	select I2C_SMBUS
+	help
+	  If you say yes to this option, support will be included for the
+	  Aspeed I2C controller with new register set.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-new-aspeed.
+
 config I2C_ASPEED
 	tristate "Aspeed I2C Controller"
 	depends on ARCH_ASPEED || COMPILE_TEST

drivers/i2c/busses/Makefile +0 -0

index c5cac15f075c..ce83dfdc2094 100644

Review

Quoted 11 lines · Ryan Chen

This series add AST2600 i2c new register set driver. The i2c new register set have new clock divider option for more flexiable generation. And also have separate i2c master and slave register set for control.

v4:

(quoted diff) +0 -4

-fix i2c-ast2600.c driver buffer mode use single buffer conflit in
 master slave mode both enable.
-fix kmemleak issue when use dma mode.
-fix typo aspeed,i2c-ast2600.yaml compatible is "aspeed,ast2600-i2c"
-fix typo aspeed,i2c-ast2600.ymal to aspeed,i2c-ast2600.yaml

How this can be a v4? Where are all others?

https://lore.kernel.org/all/?q=dfn%3Aaspeed%2Ci2c-ast2600-global.yaml
https://lore.kernel.org/all/?q=dfn%3Aaspeed%2Ci2c-ast2600.yaml

I see it for the first time.