Add ASPEED AST2600 I2Cv2 controller driver

PATCH Superseded Ryan Chen 4 participants 28 messages Feb 26, 2023 – Mar 19, 2023

Subsystems (4)

Device tree bindings I2C subsystem host drivers Arm/Aspeed machine support Arm/Aspeed I2C driver

lore

Share & tools

lore

Ryan Chen

author maintainer

2023-02-26 03:13 UTC #0 lore raw

v6 cover Add ASPEED AST2600 I2Cv2 controller driver

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

The legacy register layout is mix master/slave 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 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 -> Aw -> TxD -> P
S -> Ar -> RxD -> P
S -> Aw -> RxD -> Sr -> Ar -> TxD -> P
-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 {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

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

ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug. The following is board-specific design example.
Board A Board B
------------------------- ------------------------

|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected. Slave state machine will keep waiting for master clock in for rx/tx transmit. So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.

aspeed,xfer-mode:
For example The bus#1 have trunk data needed for tranfer, it can enable bus dma mode transfer, it can reduce cpu utilized. bus#2 is small transmit, it can enable buffer mode or byte mode to reduce memory cache flush overhead.

v6:
-remove aspeed,i2cv2.yaml, merge to aspeed,i2c.yaml
-add support for i2cv2 properites.
-i2c-ast2600.c
-fix ast2600_i2c_remove ordering.
-remove ast2600_i2c_probe goto labels, and add dev_err_probe -remove redundant deb_dbg debug message.
-rename gr_regmap -> global_regs

v5:
-remove ast2600-i2c-global.yaml, i2c-ast2600-global.c. -i2c-ast2600.c
-remove legacy clock divide, all go for new clock divide. -remove duplicated read isr.
-remove no used driver match
-fix probe return for each labels return.
-global use mfd driver, driver use phandle to regmap read/write.
-rename aspeed,i2c-ast2600.yaml to aspeed,i2cv2.yaml -remove bus-frequency.
-add required aspeed,gr
-add timeout, byte-mode, buff-mode properites.

v4:
-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

v3:
-fix i2c global clock divide default value.
-remove i2c slave no used dev_dbg info.

v2:
-add i2c global ymal file commit.
-rename file name from new to ast2600.
aspeed-i2c-new-global.c -> i2c-ast2600-global.c
aspeed-i2c-new-global.h -> i2c-ast2600-global.h
i2c-new-aspeed.c -> i2c-ast2600.c
-rename all driver function name to ast2600.

Ryan Chen (2):
dt-bindings: i2c: aspeed: support for AST2600-i2cv2
i2c: aspeed: support ast2600 i2cv new register mode driver

 .../devicetree/bindings/i2c/aspeed,i2c.yaml   |   44 +
 MAINTAINERS                                   |    9 +
 drivers/i2c/busses/Kconfig                    |   11 +
 drivers/i2c/busses/Makefile                   |    1 +
 drivers/i2c/busses/i2c-ast2600.c              | 1630 +++++++++++++++++
 5 files changed, 1695 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

Signature · 1 line

2.34.1

Ryan Chen

author maintainer

2023-02-26 03:13 UTC #1 lore raw

PATCH v6 1/2 dt-bindings: i2c: aspeed: support for AST2600-i2cv2

Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.

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

---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +44 -0

index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
+
+  aspeed,xfer-mode:
+    description: |
+      I2C bus transfer mode selection.
+      - "byte": I2C bus byte transfer mode.
+      - "buffered": I2C bus buffer register transfer mode.
+      - "dma": I2C bus dma transfer mode (default)
+    items:
+      enum: [byte, buffered, dma]
+    maxItems: 1
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+
+  aspeed,global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of i2c global register node.
+
 required:
   - reg
   - compatible

@@ -57,6 +76,19 @@ required:
 
 unevaluatedProperties: false
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: aspeed,ast2600-i2cv2
+
+then:
+  properties:
+    reg:
+      minItems: 2
+  required:
+    - aspeed,global-regs
+
 examples:
   - |
     #include <dt-bindings/clock/aspeed-clock.h>

@@ -71,3 +103,15 @@ examples:
       interrupts = <0>;
       interrupt-parent = <&i2c_ic>;
     };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    i2c1: i2c at 80 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "aspeed,ast2600-i2cv2";
+      reg = <0x80 0x80>, <0xc00 0x20>;
+      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+      aspeed,global-regs = <&i2c_global>;
+      clocks = <&syscon ASPEED_CLK_APB>;
+      resets = <&syscon ASPEED_RESET_I2C>;
+    };

Signature · 1 line

2.34.1

Jeremy Kerr

2023-02-26 07:04 UTC #2 lore raw

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

Hi Ryan,

Quoted 12 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
@@ -49,6 +49,25 @@ properties:
???? description:
?????? states that there is another master active on this bus ?
+? aspeed,timeout:
+??? type: boolean
+??? description: I2C bus timeout enable for master/slave mode +
+? aspeed,xfer-mode:
+??? description: |

Quoted 8 lines

+????? I2C bus transfer mode selection.
+????? - "byte": I2C bus byte transfer mode.
+????? - "buffered": I2C bus buffer register transfer mode. +????? - "dma": I2C bus dma transfer mode (default)
+??? items:
+????? enum: [byte, buffered, dma]
+??? maxItems: 1
+??? $ref: /schemas/types.yaml#/definitions/non-unique-string-array

There are still unresolved questions about this xfer-mode property from previous submissions of this binding. We don't yet have a justification on why the mode configuration is needed in the device tree rather than something that is specified in a driver implementation.

By now, I think we well understand what the modes are, and how a driver implementation might configure them, but none of that has (so far) provided sufficient rationale on why this belongs in the device tree.

The previous threads had a couple of pending discussions, following up on those here:

A) You mentioned in [1] that the DMA controller is shared between all i3c devices, does that have any consequence on which modes individual devices might want to choose?

B) You implied in [2] that the different transfer modes might be related to whether there are other masters present on the bus, but the logic behind that is not clear.

C) In [3] you mentioned that there might be some DRAM savings by using a particular mode.

and, most importantly:

D) unanswered from [4] and [5]: what are the hardware-specified reasons why a DT author would chose one mode over another?

If you can write this out in some format like:

- in hardware situation X, you should use DMA mode
- in hardware situation Y, you should use byte mode
- [...]

that might help us to understand where this configuration belongs, or what a reasonable DT representation should look like, or even if existing DT schema can already provide the information required to decide.

Cheers,

Jeremy

[1]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009876.html
[2]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009892.html
[3]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009880.html
[4]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009871.html
[5]: https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009884.html

Ryan Chen

author maintainer

2023-02-27 04:12 UTC #3 lore raw

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

Hello Jeremy,

Quoted 45 lines

-----Original Message-----
From: Jeremy Kerr <[email protected]>
Sent: Sunday, February 26, 2023 3:04 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Hi Ryan,
Quoted 6 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
@@ -49,6 +49,25 @@ properties:
???? description:
?????? states that there is another master active on this bus
Quoted 14 lines
+? aspeed,timeout:
+??? type: boolean
+??? description: I2C bus timeout enable for master/slave mode +
+? aspeed,xfer-mode:
+??? description: |
+????? I2C bus transfer mode selection.
+????? - "byte": I2C bus byte transfer mode.
+????? - "buffered": I2C bus buffer register transfer mode. +????? - "dma": I2C bus dma transfer mode (default)
+??? items:
+????? enum: [byte, buffered, dma]
+??? maxItems: 1
+??? $ref: /schemas/types.yaml#/definitions/non-unique-string-array
There are still unresolved questions about this xfer-mode property from previous submissions of this binding. We don't yet have a justification on why the mode configuration is needed in the device tree rather than something that is specified in a driver implementation.
By now, I think we well understand what the modes are, and how a driver implementation might configure them, but none of that has (so far) provided sufficient rationale on why this belongs in the device tree.

Quoted 6 lines

The previous threads had a couple of pending discussions, following up on those here:
A) You mentioned in [1] that the DMA controller is shared between all i3c devices, does that have any consequence on which modes individual devices might want to choose?

Yes, I2C controller share the same dma engine. The original thought can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data immediately", So it means only 1 i2c bus can be enable for DMA mode.
It means only 1 bus channel can be enable DMA for use case.
That following example for board-specific selection.
It is description in cover-letter.

The following is board-specific design example.
Board A Board B
------------------------- ------------------------

|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |        |                   |
|i2c bus#3(master)-> adc i2c device |        |                   |
-------------------------                       ------------------------

- in bus#1 situation, you should use DMA mode.
Because bus#1 have trunk data needed for transfer, it can enable bus dma mode to reduce cpu utilized.
- in bus#2/3 situation, you should use buffer/byte mode
bus#2/3 is small package transmit, it can enable buffer mode or byte mode to reduce memory cache flush overhead. Buffer mode is better, because byte mode have interrupt overhead(interrupt per byte data transmit),

-But if you more bus#4 that still have trunk data needed for transfer (master/slave), it also use buffer mode to transmit. Because bus#1 have been use for DMA mode.

Best Regards.
Ryan Chen.

Quoted 38 lines

B) You implied in [2] that the different transfer modes might be related to whether there are other masters present on the bus, but the logic behind that is not clear.
C) In [3] you mentioned that there might be some DRAM savings by using a particular mode.
and, most importantly:
D) unanswered from [4] and [5]: what are the hardware-specified reasons why a DT author would chose one mode over another?
If you can write this out in some format like:
- in hardware situation X, you should use DMA mode
- in hardware situation Y, you should use byte mode
- [...]
that might help us to understand where this configuration belongs, or what a reasonable DT representation should look like, or even if existing DT schema can already provide the information required to decide.
Cheers,
Jeremy
[1]:
https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009876.html
[2]:
https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009892.html
[3]:
https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009880.html
[4]:
https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009871.html
[5]:
https://lists.ozlabs.org/pipermail/linux-aspeed/2023-February/009884.html

Jeremy Kerr

2023-02-27 05:40 UTC #4 lore raw

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

Hi Ryan,

Quoted 4 lines

Yes, I2C controller share the same dma engine. The original thought can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data immediately", So it means only 1 i2c bus can be enable for DMA mode.

OK, this is a pretty important detail! I'd suggest putting it in the binding document.

Anything in the cover letter will get lost after review. If there is documentation that would be useful for a DTS author, I'd suggest putting it in the binding.

Quoted 6 lines

It means only 1 bus channel can be enable DMA for use case. That following example for board-specific selection.
It is description in cover-letter.
The following is board-specific design example.
Board A???????????????????????????????????????? Board B -------------------------?????????????????????? ------------------------

Quoted 8 lines

Quoted 3 lines
i2c bus#1(master/slave)? <===fingerprint ===> i2c bus#x (master/slave)|
i2c bus#2(master)-> tmp i2c device |??????? |?????????????????? |
i2c bus#3(master)-> adc i2c device |??????? |?????????????????? |
-------------------------?????????????????????? ------------------------
- in bus#1 situation, you should use DMA mode.
Because bus#1 have trunk data needed for transfer, it can enable bus dma mode to reduce cpu utilized.

What is "trunk data" in this context? Is this just a statement about the amount of expected transfers?

Quoted 10 lines

- in bus#2/3 situation, you should use buffer/byte mode bus#2/3 is small package transmit, it can enable buffer mode or byte mode to reduce memory cache flush overhead.
Buffer mode is better, because byte mode have interrupt overhead(interrupt per byte data transmit),
-But if you more bus#4 that still have trunk data needed for transfer (master/slave),
it also use buffer mode to transmit. Because bus#1 have been use for DMA mode.

So, it sounds like:

- there's no point in using byte mode, as buffer mode provides equivalent functionality with fewer drawbacks (ie, less interrupt load)

- this just leaves the dma and buffer modes

- only one controller can use dma mode

So: how about just a single boolean property to indicate "use DMA on this controller"? Something like aspeed,enable-dma? Or if DT binding experts can suggest something common that might be more suitable?

Cheers,

Jeremy

Ryan Chen

author maintainer

2023-03-01 03:02 UTC #5 lore raw

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

Hello Jeremy,

Quoted 15 lines

-----Original Message-----
From: Jeremy Kerr <[email protected]>
Sent: Monday, February 27, 2023 1:40 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Hi Ryan,

Quoted 7 lines

Quoted 4 lines
Yes, I2C controller share the same dma engine. The original thought can be enable in all i2c channel. But in AST2600 have ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data immediately", So it means only 1 i2c bus can be enable for DMA mode.
OK, this is a pretty important detail! I'd suggest putting it in the binding document.

Sorry, Do you mean add in description like following??
aspeed,xfer-mode:
description: |
I2C bus transfer mode selection.
ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data
Immediately", only 1 i2c bus can be enable for DMA mode.
- "byte": I2C bus byte transfer mode.
- "buffered": I2C bus buffer register transfer mode.
- "dma": I2C bus dma transfer mode (default)

Quoted 19 lines

Anything in the cover letter will get lost after review. If there is documentation that would be useful for a DTS author, I'd suggest putting it in the binding.
Quoted 5 lines
It means only 1 bus channel can be enable DMA for use case. That following example for board-specific selection. It is description in cover-letter.
The following is board-specific design example. Board A???????????????????????????????????????? Board
B
Quoted 4 lines
-------------------------
------------------------
Quoted 2 lines
i2c bus#1(master/slave)? <===fingerprint ===> i2c bus#x (master/slave)| i2c bus#2(master)-> tmp i2c device
|??????? |
> > | i2c bus#3(master)-> adc i2c device
|??????? |?????????????????? |
Quoted 3 lines
-------------------------
------------------------

Quoted 6 lines

Quoted 3 lines
- in bus#1 situation, you should use DMA mode.
Because bus#1 have trunk data needed for transfer, it can enable bus dma mode to reduce cpu utilized.
What is "trunk data" in this context? Is this just a statement about the amount of expected transfers?

Sorry, I can't catch your point, for example for most server application usage. The i2c not only connect with small device (like temperature sensor/ adc). It also connect with other mcu base device support i2c slave.
Most case is transfer MCTP package. (basic 64kbytes). So I say "trunk data".

Quoted 17 lines

Quoted 10 lines
- in bus#2/3 situation, you should use buffer/byte mode bus#2/3 is small package transmit, it can enable buffer mode or byte mode to reduce memory cache flush overhead.
Buffer mode is better, because byte mode have interrupt overhead(interrupt per byte data transmit),
-But if you more bus#4 that still have trunk data needed for transfer (master/slave), it also use buffer mode to transmit. Because bus#1 have been use for DMA mode.
So, it sounds like:
- there's no point in using byte mode, as buffer mode provides equivalent functionality with fewer drawbacks (ie, less interrupt load)

Quoted 7 lines

- this just leaves the dma and buffer modes
- only one controller can use dma mode
So: how about just a single boolean property to indicate "use DMA on this controller"? Something like aspeed,enable-dma? Or if DT binding experts can suggest something common that might be more suitable?

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

Best Regards.
Ryan

Jeremy Kerr

2023-03-01 03:23 UTC #6 lore raw

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

Hi Ryan,

Quoted 10 lines

Sorry, Do you mean add in description like following??
? aspeed,xfer-mode:
??? description: |
????? I2C bus transfer mode selection.
????????? ERRATA "I2C DMA fails when DRAM bus is busy and it can not take DMA write data
Immediately", only 1 i2c bus can be enable for DMA mode. ????? - "byte": I2C bus byte transfer mode.
????? - "buffered": I2C bus buffer register transfer mode. ????? - "dma": I2C bus dma transfer mode (default)

I would suggest putting some background about the transfer mode as a top-level description in the binding.

There has been a lot of discussion here on why the binding specifies the transfer mode; it would be useful (for future readers) to have a bit of context on what modes they should use.

Perhaps something like:

description: |
[general binding description]

ASPEED ast2600 platforms have a number of i2c controllers, and share a single DMA engine between the set. DTSes can specify the mode of data transfer to/from the device - either DMA or programmed I/O - but hardware limitations may require a DTS to manually allocate which controller can use DMA mode; the 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 allocate DMA mode for that one controller.

- adjusted for whatever property interface we settle on here, of course.

Quoted 8 lines

Quoted 8 lines
So, it sounds like:
?- there's no point in using byte mode, as buffer mode provides ?? equivalent functionality with fewer drawbacks (ie, less interrupt ?? load)
?- this just leaves the dma and buffer modes

Quoted 8 lines

Quoted 5 lines
?- only one controller can use dma mode
So: how about just a single boolean property to indicate "use DMA on this controller"? Something like aspeed,enable-dma? Or if DT binding experts can suggest something common that might be more suitable?
If so, just leave enable-dma and only support for buffer mode and dma mode, am I right?

Yes, from what you have said so far, I think just a single switch between DMA / not-DMA is all you need here (unless there is any time that byte mode is preferable?)

If there is already an existing DT convention for indicating/enabling DMA capability, I would suggest using that. Otherwise, just a boolean flag with a sensible name would seem to work fine. The DT experts probably have a good idea of what works best here :)

Cheers,

Jeremy

Ryan Chen

author maintainer

2023-03-01 03:40 UTC #7 lore raw

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

Hello Jeremy,

Quoted 49 lines

-----Original Message-----
From: Jeremy Kerr <[email protected]>
Sent: Wednesday, March 1, 2023 11:24 AM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Hi Ryan,
Quoted 5 lines
Sorry, Do you mean add in description like following?? ? aspeed,xfer-mode:
??? description: |
????? I2C bus transfer mode selection.
????????? ERRATA "I2C DMA fails when DRAM bus is busy and it can
not
Quoted 5 lines
take DMA write data Immediately", only 1 i2c bus can be enable for DMA mode.
????? - "byte": I2C bus byte transfer mode.
????? - "buffered": I2C bus buffer register transfer mode. ????? - "dma": I2C bus dma transfer mode (default)
I would suggest putting some background about the transfer mode as a top-level description in the binding.
There has been a lot of discussion here on why the binding specifies the transfer mode; it would be useful (for future readers) to have a bit of context on what modes they should use.
Perhaps something like:
description: |
[general binding description]
ASPEED ast2600 platforms have a number of i2c controllers, and share
a
single DMA engine between the set. DTSes can specify the mode of
data
transfer to/from the device - either DMA or programmed I/O - but hardware limitations may require a DTS to manually allocate which controller can use DMA mode; the enable-dma property allows control
of
this.

Quoted 6 lines

In cases where one the hardware design results in a specific controller handling a larger amount of data, a DTS would likely allocate DMA mode for that one controller.
- adjusted for whatever property interface we settle on here, of course.

It is more clear now, I will add in next patch.

Quoted 14 lines

Quoted 14 lines
Quoted 13 lines
So, it sounds like:
?- there's no point in using byte mode, as buffer mode provides ?? equivalent functionality with fewer drawbacks (ie, less interrupt ?? load)
?- this just leaves the dma and buffer modes
?- only one controller can use dma mode
So: how about just a single boolean property to indicate "use DMA on this controller"? Something like aspeed,enable-dma? Or if DT binding experts can suggest something common that might be more suitable?

Quoted 6 lines

Quoted 2 lines
If so, just leave enable-dma and only support for buffer mode and dma mode, am I right?
Yes, from what you have said so far, I think just a single switch between DMA / not-DMA is all you need here (unless there is any time that byte mode is preferable?)

Yes, I also think so, but if I only for dma to be single Boolean property. Should I remove all byte mode capability in driver?

Best Regards.
Ryan

Quoted 9 lines

If there is already an existing DT convention for indicating/enabling DMA capability, I would suggest using that. Otherwise, just a boolean flag with a sensible name would seem to work fine. The DT experts probably have a good idea of what works best here :)
Cheers,
Jeremy

Krzysztof Kozlowski

2023-02-27 08:25 UTC #8 lore raw

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

In reply to Ryan Chen

Quoted 13 lines · Ryan Chen

Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54

Quoted 6 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0

@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode

Nothing improved here in regards to my last comment.

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 64

Quoted 10 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +10 -0

+
+  aspeed,xfer-mode:
+    description: |
+      I2C bus transfer mode selection.
+      - "byte": I2C bus byte transfer mode.
+      - "buffered": I2C bus buffer register transfer mode.
+      - "dma": I2C bus dma transfer mode (default)
+    items:
+      enum: [byte, buffered, dma]
+    maxItems: 1

Drop, not an array.

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 65

Quoted 1 line

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +1 -0
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array

Wrong ref. This is not an array, but one string.

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 79

Quoted 13 lines

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

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

@@ -57,6 +76,19 @@ required:
 
 unevaluatedProperties: false
 
+if:

This should be under allOf (in this location)

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 90

Quoted 11 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +11 -0

+  properties:
+    compatible:
+      contains:
+        const: aspeed,ast2600-i2cv2
+
+then:
+  properties:
+    reg:
+      minItems: 2
+  required:
+    - aspeed,global-regs

else:
aspeed,global-regs: false
and the same for other v2 properties

Quoted 10 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0

+
 examples:
   - |
     #include <dt-bindings/clock/aspeed-clock.h>

@@ -71,3 +103,15 @@ examples:
       interrupts = <0>;
       interrupt-parent = <&i2c_ic>;
     };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml examples: · line 117

Quoted 10 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +10 -0

+    i2c1: i2c at 80 {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      compatible = "aspeed,ast2600-i2cv2";
+      reg = <0x80 0x80>, <0xc00 0x20>;
+      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+      aspeed,global-regs = <&i2c_global>;
+      clocks = <&syscon ASPEED_CLK_APB>;
+      resets = <&syscon ASPEED_RESET_I2C>;
+    };

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-01 05:57 UTC #9 lore raw

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

Hello Krzysztof,

Quoted 31 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 27, 2023 4:25 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 17 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 

Quoted 5 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.

Yes, as I know your require is about " DT binding to represent hardware setup" So I add more description about aspeed,timeout as blow.

|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected.
Slave state machine will keep waiting for master clock in for rx/tx transmit.
So it need timeout setting to enable timeout unlock controller state.
And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.

Do you mean add those description into ore aspeed,timout properites description?

Quoted 12 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 64
Quoted 11 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +10 -0
+
+  aspeed,xfer-mode:
+    description: |
+      I2C bus transfer mode selection.
+      - "byte": I2C bus byte transfer mode.
+      - "buffered": I2C bus buffer register transfer mode.
+      - "dma": I2C bus dma transfer mode (default)
+    items:
+      enum: [byte, buffered, dma]
+    maxItems: 1

Quoted 5 lines

Drop, not an array.
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 65
Quoted 1 line
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +1 -0
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
Wrong ref. This is not an array, but one string.

Sorry, I can't catch your "one string" point. Could you point me what ref I can refer to? That I can check into Linux example. Thanks a lot.

Quoted 17 lines

Quoted 10 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +5 -0
+
+  aspeed,global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of i2c global register node.
+
 required:
   - reg
   - compatible
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 79
Quoted 4 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +1 -0
@@ -57,6 +76,19 @@ required:
 
 unevaluatedProperties: false
 
+if:
This should be under allOf (in this location)

Quoted 16 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 90
Quoted 11 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +11 -0
+  properties:
+    compatible:
+      contains:
+        const: aspeed,ast2600-i2cv2
+
+then:
+  properties:
+    reg:
+      minItems: 2
+  required:
+    - aspeed,global-regs
else:
aspeed,global-regs: false
and the same for other v2 properties

Does modify by following?

allOf:
-if:
properties:

    compatible:
      contains:
        const: aspeed,ast2600-i2cv2

then:
properties:
reg:
minItems: 2
required:
- aspeed,global-regs
else:
- aspeed,global-regs: false
-aspeed,timeout: false
- aspeed,xfer-mode: false

Quoted 12 lines

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

+
 examples:
   - |
     #include <dt-bindings/clock/aspeed-clock.h>

@@ -71,3 +103,15 @@ examples:
       interrupts = <0>;
       interrupt-parent = <&i2c_ic>;
     };
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    i2c1: i2c at 80 {
+      #address-cells = <1>;

Quoted 9 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml examples: · line 117
Quoted 8 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +8 -0
+      #size-cells = <0>;
+      compatible = "aspeed,ast2600-i2cv2";
+      reg = <0x80 0x80>, <0xc00 0x20>;
+      interrupts = <GIC_SPI 110 IRQ_TYPE_LEVEL_HIGH>;
+      aspeed,global-regs = <&i2c_global>;
+      clocks = <&syscon ASPEED_CLK_APB>;
+      resets = <&syscon ASPEED_RESET_I2C>;
+    };

Best regards,
Ryan Chen.

Krzysztof Kozlowski

2023-03-03 08:20 UTC #10 lore raw

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

Quoted 51 lines · Ryan Chen

Hello Krzysztof,
Quoted 31 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 27, 2023 4:25 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 17 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
Quoted 5 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.
Yes, as I know your require is about " DT binding to represent hardware setup" So I add more description about aspeed,timeout as blow.
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug. The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

Quoted 7 lines

aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected.
Slave state machine will keep waiting for master clock in for rx/tx transmit.
So it need timeout setting to enable timeout unlock controller state.
And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.
Do you mean add those description into ore aspeed,timout properites description?

You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to configure/use. What I was looking for is to describe the actual configuration you have (e.g. multi-master) which leads to enable or disable such feature in your hardware. Especially that bool value does not scale later to actual timeout values in time (ms)...

I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has it...).

Also I wonder, why you wouldn't enable timeout always...

+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?

Quoted 13 lines

Quoted 12 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +10 -0

+
+  aspeed,xfer-mode:
+    description: |
+      I2C bus transfer mode selection.
+      - "byte": I2C bus byte transfer mode.
+      - "buffered": I2C bus buffer register transfer mode.
+      - "dma": I2C bus dma transfer mode (default)
+    items:
+      enum: [byte, buffered, dma]
+    maxItems: 1

Quoted 7 lines

Quoted 5 lines
Drop, not an array.
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 65
Quoted 1 line
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +1 -0
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
Wrong ref. This is not an array, but one string.
Sorry, I can't catch your "one string" point.

How many strings you are going to have in this property? If one (maxItems: 1), then this is not an array.

Quoted 45 lines

Could you point me what ref I can refer to? That I can check into Linux example. Thanks a lot.
Quoted 17 lines
Quoted 10 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +5 -0
+
+  aspeed,global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of i2c global register node.
+
 required:
   - reg
   - compatible
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 79
Quoted 4 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +1 -0
@@ -57,6 +76,19 @@ required:
 
 unevaluatedProperties: false
 
+if:
This should be under allOf (in this location)
Quoted 16 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 90
Quoted 11 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +11 -0
+  properties:
+    compatible:
+      contains:
+        const: aspeed,ast2600-i2cv2
+
+then:
+  properties:
+    reg:
+      minItems: 2
+  required:
+    - aspeed,global-regs
else:
aspeed,global-regs: false
and the same for other v2 properties
Does modify by following?
allOf:
-if:
   properties:
    compatible:
      contains:
        const: aspeed,ast2600-i2cv2

Quoted 10 lines

then:
properties:
reg:
minItems: 2
required:
- aspeed,global-regs
else:
- aspeed,global-regs: false
-aspeed,timeout: false
- aspeed,xfer-mode: false

yes

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-03 08:28 UTC #11 lore raw

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

Hello Krzysztof,

Quoted 97 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:20 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 24 lines · Ryan Chen
Hello Krzysztof,
Quoted 22 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 27, 2023 4:25 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 6 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
+++++++++++++++++++
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
Quoted 7 lines
Quoted 5 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.
Yes, as I know your require is about " DT binding to represent hardware
setup"
Quoted 3 lines
So I add more description about aspeed,timeout as blow.
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to
another board. And also support hotplug.
Quoted 5 lines
The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |
|
Quoted 1 line
|i2c bus#3(master)-> adc i2c device | |
|
Quoted 7 lines
------------------------- ------------------------
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected. Slave state machine will keep waiting for master clock in for rx/tx transmit. So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 2 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to configure/use.
What I was looking for is to describe the actual configuration you have (e.g. multi-master) which leads to enable or disable such feature in your hardware. Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has it...).

Quoted 6 lines

Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?

Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.

It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.

Quoted 85 lines

Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +10 -0
+
+  aspeed,xfer-mode:
+    description: |
+      I2C bus transfer mode selection.
+      - "byte": I2C bus byte transfer mode.
+      - "buffered": I2C bus buffer register transfer mode.
+      - "dma": I2C bus dma transfer mode (default)
+    items:
+      enum: [byte, buffered, dma]
+    maxItems: 1
Quoted 7 lines
Quoted 5 lines
Drop, not an array.
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 65
Quoted 1 line
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +1 -0
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
Wrong ref. This is not an array, but one string.
Sorry, I can't catch your "one string" point.
How many strings you are going to have in this property? If one (maxItems: 1), then this is not an array.
Quoted 45 lines
Could you point me what ref I can refer to? That I can check into Linux example. Thanks a lot.
Quoted 17 lines
Quoted 10 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +5 -0
+
+  aspeed,global-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of i2c global register node.
+
 required:
   - reg
   - compatible
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 79
Quoted 4 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +1 -0
@@ -57,6 +76,19 @@ required:
 
 unevaluatedProperties: false
 
+if:
This should be under allOf (in this location)
Quoted 16 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml required: · line 90
Quoted 11 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +11 -0
+  properties:
+    compatible:
+      contains:
+        const: aspeed,ast2600-i2cv2
+
+then:
+  properties:
+    reg:
+      minItems: 2
+  required:
+    - aspeed,global-regs
else:
aspeed,global-regs: false
and the same for other v2 properties
Does modify by following?
allOf:
-if:
   properties:
    compatible:
      contains:
        const: aspeed,ast2600-i2cv2
Quoted 10 lines
then:
properties:
reg:
minItems: 2
required:
- aspeed,global-regs
else:
- aspeed,global-regs: false
-aspeed,timeout: false
- aspeed,xfer-mode: false
yes
Best regards,
Krzysztof

Krzysztof Kozlowski

2023-03-03 08:50 UTC #12 lore raw

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

Quoted 105 lines · Ryan Chen

Hello Krzysztof,
Quoted 97 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:20 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 24 lines · Ryan Chen
Hello Krzysztof,
Quoted 22 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 27, 2023 4:25 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 6 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
+++++++++++++++++++
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
Quoted 7 lines
Quoted 5 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.
Yes, as I know your require is about " DT binding to represent hardware
setup"
Quoted 3 lines
So I add more description about aspeed,timeout as blow.
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to
another board. And also support hotplug.
Quoted 5 lines
The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |
|
Quoted 1 line
|i2c bus#3(master)-> adc i2c device | |
|
Quoted 7 lines
------------------------- ------------------------
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected. Slave state machine will keep waiting for master clock in for rx/tx transmit. So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 2 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to configure/use.
What I was looking for is to describe the actual configuration you have (e.g. multi-master) which leads to enable or disable such feature in your hardware. Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has it...).
Quoted 6 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?

Quoted 6 lines

Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.

Then you have already property for this - "smbus"?

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-03 08:55 UTC #13 lore raw

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

Hello Krzysztof,

Quoted 128 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:51 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 83 lines · Ryan Chen
Hello Krzysztof,
Quoted 81 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:20 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>;
linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 24 lines · Ryan Chen
Hello Krzysztof,
Quoted 22 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 27, 2023 4:25 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 6 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
+++++++++++++++++++
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
Quoted 8 lines
Quoted 5 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.
Yes, as I know your require is about " DT binding to represent hardware
setup"
Quoted 4 lines
So I add more description about aspeed,timeout as blow.
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to
another board. And also support hotplug.
Quoted 5 lines
The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |
|
Quoted 1 line
|i2c bus#3(master)-> adc i2c device | |
|
Quoted 5 lines
------------------------- ------------------------
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected. Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 2 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to configure/use. What I was looking for is to describe the actual configuration you have (e.g. multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 8 lines
Quoted 8 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?

Quoted 8 lines

Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?

To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.

Best regards,
Ryan Chen

Krzysztof Kozlowski

2023-03-03 09:26 UTC #14 lore raw

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

Quoted 138 lines · Ryan Chen

Hello Krzysztof,
Quoted 135 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:51 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 83 lines · Ryan Chen
Hello Krzysztof,
Quoted 81 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:20 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>;
linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 24 lines · Ryan Chen
Hello Krzysztof,
Quoted 22 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 27, 2023 4:25 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 6 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
+++++++++++++++++++
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
Quoted 8 lines
Quoted 5 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.
Yes, as I know your require is about " DT binding to represent hardware
setup"
Quoted 4 lines
So I add more description about aspeed,timeout as blow.
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to
another board. And also support hotplug.
Quoted 5 lines
The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |
|
Quoted 1 line
|i2c bus#3(master)-> adc i2c device | |
|
Quoted 5 lines
------------------------- ------------------------
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected. Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 2 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to configure/use. What I was looking for is to describe the actual configuration you have (e.g. multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 8 lines
Quoted 8 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 13 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.

Quoted 5 lines

Quoted 1 line
Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.

Yeah and this is the only place. It also differs because it allows actual timeout values.

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-03 10:16 UTC #15 lore raw

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

Hello Krzysztof,

Quoted 168 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 5:26 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 13 lines · Ryan Chen
Hello Krzysztof,
Quoted 10 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:51 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>;
Benjamin
Quoted 77 lines
Quoted 77 lines
Herrenschmidt <[email protected]>;
linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 69 lines · Ryan Chen
Hello Krzysztof,
Quoted 67 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 4:20 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org;
linux-arm-kernel at lists.infradead.org;
linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 24 lines · Ryan Chen
Hello Krzysztof,
Quoted 22 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Monday, February 27, 2023 4:25 PM
To: Ryan Chen <[email protected]>; Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 6 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
+++++++++++++++++++
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
 1 file changed, 44 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml index f597f73ccd87..75de3ce41cf5 100644
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
Quoted 8 lines
Quoted 5 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.
Yes, as I know your require is about " DT binding to represent hardware
setup"
Quoted 2 lines
So I add more description about aspeed,timeout as blow.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 2 lines
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to
another board. And also support hotplug.
Quoted 4 lines
The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave) <===fingerprint ===> i2c bus#x
(master/slave)|
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
|i2c bus#2(master)-> tmp i2c device | |
|
Quoted 1 line
|i2c bus#3(master)-> adc i2c device | |
|
Quoted 4 lines
------------------------- ------------------------
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly
disconnected.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 3 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to
configure/use.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
What I was looking for is to describe the actual configuration you have
(e.g.
Quoted 27 lines
Quoted 27 lines
Quoted 1 line
Quoted 1 line
multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 9 lines
Quoted 9 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?

Quoted 8 lines

Quoted 5 lines
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree
/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.
Yeah and this is the only place. It also differs because it allows actual timeout values.

Thanks, So can I still keep the property "aspeed,timeout" here? It is the only place.

Best regards,
Ryan Chen

Krzysztof Kozlowski

2023-03-03 10:41 UTC #16 lore raw

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

Quoted 52 lines · Ryan Chen

Quoted 52 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly
disconnected.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 3 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to
configure/use.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
What I was looking for is to describe the actual configuration you have
(e.g.
Quoted 32 lines
Quoted 19 lines
Quoted 1 line
Quoted 1 line
multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 9 lines
Quoted 9 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Quoted 8 lines
Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetree
/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.

Quoted 4 lines

Quoted 2 lines
Yeah and this is the only place. It also differs because it allows actual timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here? It is the only place.

No, because none of my concerns above are addressed.

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-04 01:33 UTC #17 lore raw

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

Hello Krzysztof,

Quoted 72 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 6:41 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 56 lines · Ryan Chen
Quoted 56 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly
disconnected.
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 3 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to
configure/use.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
What I was looking for is to describe the actual configuration you have
(e.g.
Quoted 34 lines
Quoted 20 lines
Quoted 2 lines
Quoted 2 lines
multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 9 lines
Quoted 9 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Quoted 8 lines
Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetr
ee
/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
Yeah and this is the only place. It also differs because it allows actual timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here? It is the only place.
No, because none of my concerns above are addressed.

Thanks, I realize your concerns.

So, I modify it like i2c-mpc.yaml
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47

aspeed,timeout:
$ref: /schemas/types.yaml#/definitions/uint32
description: |
I2C bus timeout in microseconds
Is this way acceptable?

Best regards,
Ryan Chen

Krzysztof Kozlowski

2023-03-05 09:49 UTC #18 lore raw

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

Quoted 83 lines · Ryan Chen

Hello Krzysztof,
Quoted 72 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 6:41 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 56 lines · Ryan Chen
Quoted 56 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly
disconnected.
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
So it need timeout setting to enable timeout unlock controller state. And in another side. In Master side also need avoid suddenly slave
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 3 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to
configure/use.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
What I was looking for is to describe the actual configuration you have
(e.g.
Quoted 34 lines
Quoted 20 lines
Quoted 2 lines
Quoted 2 lines
multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 9 lines
Quoted 9 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Quoted 8 lines
Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devicetr
ee
/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Yeah and this is the only place. It also differs because it allows actual timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here? It is the only place.
No, because none of my concerns above are addressed.
Thanks, I realize your concerns.

Quoted 8 lines

So, I modify it like i2c-mpc.yaml
https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-mpc.yaml#L43-L47
aspeed,timeout:
$ref: /schemas/types.yaml#/definitions/uint32
description: |
I2C bus timeout in microseconds
Is this way acceptable?

So, let's repeat my last questions:

1. Why you wouldn't enable timeout always...

Quoted 2 lines · You

http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.

which indicates you should always use timeout, doesn't it?

2. Why we do not have it for all controllers with SMBus v3? Why this one is special?

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-06 00:48 UTC #19 lore raw

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

Hello Krzysztof,

Quoted 116 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Sunday, March 5, 2023 5:49 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 27 lines · Ryan Chen
Hello Krzysztof,
Quoted 25 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 6:41 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>;
linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 7 lines · Ryan Chen
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly
disconnected.
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 1 line
Quoted 1 line
Quoted 1 line
So it need timeout setting to enable timeout unlock controller
state.
Quoted 64 lines
Quoted 51 lines
Quoted 51 lines
Quoted 51 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
And in another side. In Master side also need avoid suddenly slave
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 3 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to
configure/use.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
What I was looking for is to describe the actual configuration you have
(e.g.
Quoted 35 lines
Quoted 20 lines
Quoted 2 lines
Quoted 2 lines
multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 9 lines
Quoted 9 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Quoted 8 lines
Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/device
tr
ee
/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Yeah and this is the only place. It also differs because it allows actual timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here? It is the only place.
No, because none of my concerns above are addressed.
Thanks, I realize your concerns.
So, I modify it like i2c-mpc.yaml
https://github.com/torvalds/linux/blob/master/Documentation/devicetree
/bindings/i2c/i2c-mpc.yaml#L43-L47
Quoted 5 lines
aspeed,timeout:
$ref: /schemas/types.yaml#/definitions/uint32
description: |
I2C bus timeout in microseconds
Is this way acceptable?
So, let's repeat my last questions:

Quoted 7 lines

1. Why you wouldn't enable timeout always...
Quoted 2 lines · You
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
which indicates you should always use timeout, doesn't it?

Yes, if board design the bus is connected with SMBUS device, it should enable.
But in my previous statement, the board design is two multi-master devices connected each other. And both device is transfer with MCTP protocol.
That will not SMBUS protocol.
They need have timeout that prevent unexpected un-plug.
I do the study with smbus in Linux, that will different slave call back. Compare with smbus slave and mctp slave. So in this scenario, that is only enable for timeout.

Quoted 2 lines

2. Why we do not have it for all controllers with SMBus v3? Why this one is special?

Not all bus is connected with smbus. Most are i2c device connected in board. That will be specific statement for each bus.

Best regards,
Ryan Chen

Krzysztof Kozlowski

2023-03-07 08:11 UTC #20 lore raw

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

Quoted 120 lines · Ryan Chen

Hello Krzysztof,
Quoted 118 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Sunday, March 5, 2023 5:49 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 27 lines · Ryan Chen
Hello Krzysztof,
Quoted 25 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 6:41 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>;
linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 7 lines · Ryan Chen
Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly
disconnected.
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 1 line
Quoted 1 line
Quoted 1 line
So it need timeout setting to enable timeout unlock controller
state.
Quoted 64 lines
Quoted 51 lines
Quoted 51 lines
Quoted 51 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
And in another side. In Master side also need avoid suddenly slave
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 3 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to
configure/use.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
What I was looking for is to describe the actual configuration you have
(e.g.
Quoted 35 lines
Quoted 20 lines
Quoted 2 lines
Quoted 2 lines
multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 9 lines
Quoted 9 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Quoted 8 lines
Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/device
tr
ee
/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Yeah and this is the only place. It also differs because it allows actual timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here? It is the only place.
No, because none of my concerns above are addressed.
Thanks, I realize your concerns.
So, I modify it like i2c-mpc.yaml
https://github.com/torvalds/linux/blob/master/Documentation/devicetree
/bindings/i2c/i2c-mpc.yaml#L43-L47
Quoted 5 lines
aspeed,timeout:
$ref: /schemas/types.yaml#/definitions/uint32
description: |
I2C bus timeout in microseconds
Is this way acceptable?
So, let's repeat my last questions:
1. Why you wouldn't enable timeout always...

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines · You
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
which indicates you should always use timeout, doesn't it?
Yes, if board design the bus is connected with SMBUS device, it should enable. But in my previous statement, the board design is two multi-master devices connected each other.

For which you have the property, thus case is solved, isn't it? You want timeout always except for multi-master?

Quoted 5 lines

And both device is transfer with MCTP protocol.
That will not SMBUS protocol.
They need have timeout that prevent unexpected un-plug.
I do the study with smbus in Linux, that will different slave call back. Compare with smbus slave and mctp slave. So in this scenario, that is only enable for timeout.

And the driver knows which protocol it is going to talk and such choice should not be in DT.

Quoted 6 lines

Quoted 3 lines
2. Why we do not have it for all controllers with SMBus v3? Why this one is special?
Not all bus is connected with smbus. Most are i2c device connected in board. That will be specific statement for each bus.

That's not the answer to my question. Why other controllers which can be connected to I2C or SMBus devices do not need this property?

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-07 10:09 UTC #21 lore raw

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

Hello Krzysztof,

Quoted 147 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Tuesday, March 7, 2023 4:12 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 131 lines · Ryan Chen
Hello Krzysztof,
Quoted 128 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Sunday, March 5, 2023 5:49 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>;
linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 29 lines · Ryan Chen
Hello Krzysztof,
Quoted 27 lines
-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Friday, March 3, 2023 6:41 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org;
linux-arm-kernel at lists.infradead.org;
linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 8 lines · Ryan Chen
Quoted 8 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly
disconnected.
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Slave state machine will keep waiting for master clock in for rx/tx
transmit.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
So it need timeout setting to enable timeout unlock controller
state.
Quoted 66 lines
Quoted 52 lines
Quoted 52 lines
Quoted 52 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 2 lines
And in another side. In Master side also need avoid suddenly slave
miss(un-plug), Master will timeout and release the SDA/SCL.
Quoted 3 lines
Do you mean add those description into ore aspeed,timout properites
description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to
configure/use.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
What I was looking for is to describe the actual
configuration you have
(e.g.
Quoted 36 lines
Quoted 20 lines
Quoted 2 lines
Quoted 2 lines
multi-master) which leads to enable or disable such feature in your
hardware.
Quoted 9 lines
Quoted 9 lines
Especially that bool value does not scale later to actual timeout values in time (ms)...
I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has
it...).
Quoted 7 lines
Quoted 7 lines
Also I wonder, why you wouldn't enable timeout always...
+Cc Wolfram,
Maybe you know whether bool "timeout" property for one controller makes sense? Why we do not have it for all controllers?
Quoted 8 lines
Quoted 6 lines
Because, i2c bus didn?t specific timeout.
But SMBus defines a clock low time-out, TIMEOUT of 35 ms.
It have definition in SMBus specification.
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
Then you have already property for this - "smbus"?
To be a property "smbus", that would be a big topic, I saw fsl i2c also have this.
https://github.com/torvalds/linux/blob/master/Documentation/devi
ce
tr
ee
/bindings/i2c/i2c-mpc.yaml#L43-L47
So, I just think the "timeout" property.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Yeah and this is the only place. It also differs because it allows actual timeout values.
Thanks, So can I still keep the property "aspeed,timeout" here? It is the only place.
No, because none of my concerns above are addressed.
Thanks, I realize your concerns.
So, I modify it like i2c-mpc.yaml
https://github.com/torvalds/linux/blob/master/Documentation/devicetr
ee
/bindings/i2c/i2c-mpc.yaml#L43-L47
Quoted 4 lines
aspeed,timeout:
$ref: /schemas/types.yaml#/definitions/uint32
description: |
I2C bus timeout in microseconds Is this way acceptable?
So, let's repeat my last questions:
1. Why you wouldn't enable timeout always...
Quoted 2 lines · You
http://smbus.org/specs/SMBus_3_1_20180319.pdf
You can check Page 18, Note3 that have timeout description.
which indicates you should always use timeout, doesn't it?

Quoted 7 lines

Quoted 1 line
Yes, if board design the bus is connected with SMBUS device, it should
enable.
Quoted 1 line
But in my previous statement, the board design is two multi-master devices
connected each other.
For which you have the property, thus case is solved, isn't it? You want timeout always except for multi-master?

But even if in multi-master board design, no hot-plug requirement. And it will not need enable the timeout.
That the reason I separate the timeout and multi-master property

Quoted 10 lines

Quoted 5 lines
And both device is transfer with MCTP protocol.
That will not SMBUS protocol.
They need have timeout that prevent unexpected un-plug.
I do the study with smbus in Linux, that will different slave call back.
Compare with smbus slave and mctp slave.
Quoted 1 line
So in this scenario, that is only enable for timeout.
And the driver knows which protocol it is going to talk and such choice should not be in DT.

Sorry, as I understand the i2c driver don't know which protocol is. Due to in i2c driver implement, it just a slave call back function. i2c_slave_event -> client->slave_cb : up layer to do protocol operate.

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
2. Why we do not have it for all controllers with SMBus v3? Why this one is special?
Not all bus is connected with smbus. Most are i2c device connected in board. That will be specific statement for each bus.
That's not the answer to my question. Why other controllers which can be connected to I2C or SMBus devices do not need this property?

For example following is the board specific connection.

Board A Board B
------------------------- ------------------------

|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

Bus#1 is mctp transfer for each BoardA/B. (Not smbus connected) Bus#2 is i2c device connected.
Bus#3 is i2c device connected.

Best regards,
Ryan Chen

Krzysztof Kozlowski

2023-03-09 08:51 UTC #22 lore raw

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

Quoted 13 lines · Ryan Chen

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
2. Why we do not have it for all controllers with SMBus v3? Why this one is special?
Not all bus is connected with smbus. Most are i2c device connected in board. That will be specific statement for each bus.
That's not the answer to my question. Why other controllers which can be connected to I2C or SMBus devices do not need this property?
For example following is the board specific connection.
Board A Board B
------------------------- ------------------------

Quoted 8 lines

|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------

Bus#1 is mctp transfer for each BoardA/B. (Not smbus connected) Bus#2 is i2c device connected.
Bus#3 is i2c device connected.

You are repeating the same stuff for my question. Where do you see on this diagram here other I2C controller? How does it answer my question? You keep repeating same and same, so it won't work.

Best regards,
Krzysztof

Ryan Chen

author maintainer

2023-03-09 09:15 UTC #23 lore raw

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

Hello Krzysztof,

Quoted 37 lines

-----Original Message-----
From: Krzysztof Kozlowski <[email protected]>
Sent: Thursday, March 9, 2023 4:52 PM
To: Ryan Chen <[email protected]>; Wolfram Sang <[email protected]>
Cc: Joel Stanley <[email protected]>; Brendan Higgins <[email protected]>; Krzysztof Kozlowski <[email protected]>; Andrew Jeffery <[email protected]>; devicetree at vger.kernel.org; Philipp Zabel <[email protected]>; Rob Herring <[email protected]>; Benjamin Herrenschmidt <[email protected]>; linux-aspeed at lists.ozlabs.org; linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; openbmc at lists.ozlabs.org; linux-i2c at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Quoted 4 lines · Ryan Chen
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
2. Why we do not have it for all controllers with SMBus v3? Why this one is special?
Not all bus is connected with smbus. Most are i2c device connected in
board.
Quoted 11 lines
Quoted 4 lines
Quoted 1 line
That will be specific statement for each bus.
That's not the answer to my question. Why other controllers which can be connected to I2C or SMBus devices do not need this property?
For example following is the board specific connection.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |
|
Quoted 1 line
|i2c bus#3(master)-> adc i2c device | |
|
Quoted 2 lines
------------------------- ------------------------

Quoted 8 lines

Quoted 3 lines
Bus#1 is mctp transfer for each BoardA/B. (Not smbus connected) Bus#2 is i2c device connected.
Bus#3 is i2c device connected.
You are repeating the same stuff for my question. Where do you see on this diagram here other I2C controller? How does it answer my question? You keep repeating same and same, so it won't work.

Sorry, my mis-understand.
I don't see many controllers support for timeout feature.
I do study with other controllers that is implement it by sw impelement.
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-mlxbf.c#L302-L307
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-img-scb.c#L52-L55
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-omap.c#L588-L596

So I asking for aspeed,timeout property, If it not acceptable, I will default enable timeout in driver. Thanks your guidance.

Best regards,
Ryan Chen

Andi Shyti

maintainer

2023-03-12 12:33 UTC #24 lore raw

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

In reply to Krzysztof Kozlowski

Hi Krzysztof and Ryan,

Quoted 33 lines

Quoted 18 lines
Quoted 5 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Nothing improved here in regards to my last comment.
Yes, as I know your require is about " DT binding to represent hardware setup" So I add more description about aspeed,timeout as blow.
ASPEED SOC chip is server product, i2c bus may have fingerprint connect to another board. And also support hotplug. The following is board-specific design example.
Board A Board B
------------------------- ------------------------
|i2c bus#1(master/slave)  <===fingerprint ===> i2c bus#x (master/slave)|
|i2c bus#2(master)-> tmp i2c device |          |                       |
|i2c bus#3(master)-> adc i2c device |          |                       |
-------------------------                       ------------------------
Quoted 7 lines
aspeed,timout properites:
For example I2C controller as slave mode, and suddenly disconnected.
Slave state machine will keep waiting for master clock in for rx/tx transmit.
So it need timeout setting to enable timeout unlock controller state.
And in another side. In Master side also need avoid suddenly slave miss(un-plug), Master will timeout and release the SDA/SCL.
Do you mean add those description into ore aspeed,timout properites description?
You are describing here one particular feature you want to enable in the driver which looks non-scalable and more difficult to configure/use. What I was looking for is to describe the actual configuration you have (e.g. multi-master) which leads to enable or disable such feature in your hardware. Especially that bool value does not scale later to actual timeout values in time (ms)...

Quoted 5 lines

I don't know I2C that much, but I wonder - why this should be specific to Aspeed I2C and no other I2C controllers implement it? IOW, this looks quite generic and every I2C controller should have it. Adding it specific to Aspeed suggests that either we miss a generic property or this should not be in DT at all (because no one else has it...).

this property is missing in the i2c devicetree property and because this is the second driver needing it, I think it should be added.

To be clear, this timeout means that the SCL is kept low for some number of milliseconds in order to force the slave to enter a wait state. This is done when the master has some particular needs as Ryan is describing.

It's defined in the i2c specification, while smbus defines it in a range from 25 to 35 ms.

In any case it's not a boolean value unless the controller has it defined internally by the firmware.

So... nack! Please, hold a bit, I'm sending a patch.

Andi

Andi Shyti

maintainer

2023-03-18 09:09 UTC #25 lore raw

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

In reply to Ryan Chen

Hi Ryan,

Quoted 13 lines · Ryan Chen

Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54

Quoted 6 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0

@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode

Finally you can proceed with this. Please remove "aspeed,timeout" and use "i2c-scl-has-clk-low-timeout" instead.

Andi

Ryan Chen

author maintainer

2023-03-19 02:05 UTC #26 lore raw

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

Hello,

Quoted 37 lines

-----Original Message-----
From: Andi Shyti <[email protected]>
Sent: Saturday, March 18, 2023 5:10 PM
To: Ryan Chen <[email protected]>
Cc: Andrew Jeffery <[email protected]>; Brendan Higgins <[email protected]>; Benjamin Herrenschmidt <[email protected]>; Joel Stanley <[email protected]>; Rob Herring <[email protected]>; Krzysztof Kozlowski <[email protected]>; Philipp Zabel <[email protected]>; linux-i2c at vger.kernel.org; openbmc at lists.ozlabs.org; devicetree at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-aspeed at lists.ozlabs.org; linux-kernel at vger.kernel.org
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
Hi Ryan,
Quoted 6 lines · Ryan Chen
Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
Signed-off-by Ryan Chen <[email protected]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
+++++++++++++++++++
Quoted 11 lines
 1 file changed, 44 insertions(+)
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +0 -0
index f597f73ccd87..75de3ce41cf5 100644
@@ -49,6 +49,25 @@ properties:
     description:
       states that there is another master active on this bus
 

Quoted 6 lines

Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml properties: · line 54
Quoted 3 lines
Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml +3 -0
+  aspeed,timeout:
+    type: boolean
+    description: I2C bus timeout enable for master/slave mode
Finally you can proceed with this. Please remove "aspeed,timeout" and use "i2c-scl-has-clk-low-timeout" instead.

Thanks a lot, I will start progress this.

Best Regards
Ryan

Ryan Chen

author maintainer

2023-02-26 03:13 UTC #27 lore raw

PATCH v6 2/2 i2c: aspeed: support ast2600 i2cv new register mode driver

In reply to Ryan Chen

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                      |    9 +
 drivers/i2c/busses/Kconfig       |   11 +
 drivers/i2c/busses/Makefile      |    1 +
 drivers/i2c/busses/i2c-ast2600.c | 1630 ++++++++++++++++++++++++++++++
 4 files changed, 1651 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ast2600.c

MAINTAINERS +9 -0

index 749710e22b4d..433ec2309441 100644
@@ -1924,6 +1924,15 @@ F:	Documentation/devicetree/bindings/interrupt-controller/aspeed,ast2400-i2c-ic.
 F:	drivers/i2c/busses/i2c-aspeed.c
 F:	drivers/irqchip/irq-aspeed-i2c-ic.c
 
+ARM/ASPEED I2CV2 DRIVER
+M:	Ryan Chen <[email protected]>
+R:	Andrew Jeffery <[email protected]>
+L:	linux-i2c at vger.kernel.org
+L:	openbmc at lists.ozlabs.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+F:	drivers/i2c/busses/i2c-ast2600.c
+
 ARM/ASPEED MACHINE SUPPORT
 M:	Joel Stanley <[email protected]>
 R:	Andrew Jeffery <[email protected]>

drivers/i2c/busses/Kconfig +11 -0

index 7284206b278b..4f6935866257 100644
@@ -389,6 +389,17 @@ config I2C_ALTERA
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-altera.
 
+config I2C_AST2600
+	tristate "Aspeed I2C v2 Controller"
+	depends on ARCH_ASPEED || COMPILE_TEST
+	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-ast2600.
+
 config I2C_ASPEED
 	tristate "Aspeed I2C Controller"
 	depends on ARCH_ASPEED || COMPILE_TEST

drivers/i2c/busses/Makefile +1 -0

index c5cac15f075c..d0ab4d45781c 100644
@@ -38,6 +38,7 @@ obj-$(CONFIG_I2C_POWERMAC)	+= i2c-powermac.o
 obj-$(CONFIG_I2C_ALTERA)	+= i2c-altera.o
 obj-$(CONFIG_I2C_AMD_MP2)	+= i2c-amd-mp2-pci.o i2c-amd-mp2-plat.o
 obj-$(CONFIG_I2C_ASPEED)	+= i2c-aspeed.o
+obj-$(CONFIG_I2C_AST2600)	+= i2c-ast2600.o
 obj-$(CONFIG_I2C_AT91)		+= i2c-at91.o
 i2c-at91-objs			:= i2c-at91-core.o i2c-at91-master.o
 ifeq ($(CONFIG_I2C_AT91_SLAVE_EXPERIMENTAL),y)

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

1,630 lines not on this page · raw message

Signature · 1 line

2.34.1