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

Commit Message

Ryan Chen Feb. 26, 2023, 3:13 a.m. UTC

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

Signed-off-by: Ryan Chen <[e-mail]>
---
 .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Jeremy Kerr Feb. 26, 2023, 7:04 a.m. UTC | #1

Hi Ryan,

> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -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

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 Feb. 27, 2023, 4:12 a.m. UTC | #2

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <[e-mail]>
> Sent: Sunday, February 26, 2023 3:04 PM
> To: Ryan Chen <[e-mail]>; Andrew Jeffery
> <[e-mail]>; Brendan Higgins <[e-mail]>; Benjamin
> Herrenschmidt <[e-mail]>; Joel Stanley <[e-mail]>;
> Rob Herring <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Philipp Zabel <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -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
> 
> 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?

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.

> 
> 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 Feb. 27, 2023, 5:40 a.m. UTC | #3

Hi Ryan,

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

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

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

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

Krzysztof Kozlowski Feb. 27, 2023, 8:25 a.m. UTC | #4

On 26/02/2023 04:13, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> aspeed,xfer-mode description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <[e-mail]>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>  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
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -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.

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

> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array

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

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

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

> +
>  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@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 March 1, 2023, 3:02 a.m. UTC | #5

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <[e-mail]>
> Sent: Monday, February 27, 2023 1:40 PM
> To: Ryan Chen <[e-mail]>; Andrew Jeffery
> <[e-mail]>; Brendan Higgins <[e-mail]>; Benjamin
> Herrenschmidt <[e-mail]>; Joel Stanley <[e-mail]>;
> Rob Herring <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Philipp Zabel <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > 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)
      
> 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.
> 
> > 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.
> 
> 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".

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

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

Best Regards.
Ryan

Jeremy Kerr March 1, 2023, 3:23 a.m. UTC | #6

Hi Ryan,

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

> > 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?
> 
> 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 March 1, 2023, 3:40 a.m. UTC | #7

Hello Jeremy,

> -----Original Message-----
> From: Jeremy Kerr <[e-mail]>
> Sent: Wednesday, March 1, 2023 11:24 AM
> To: Ryan Chen <[e-mail]>; Andrew Jeffery
> <[e-mail]>; Brendan Higgins <[e-mail]>; Benjamin
> Herrenschmidt <[e-mail]>; Joel Stanley <[e-mail]>;
> Rob Herring <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Philipp Zabel <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> Hi Ryan,
> 
> > 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.
> 
It is more clear now, I will add in next patch.

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

> 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 March 1, 2023, 5:57 a.m. UTC | #8

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Monday, February 27, 2023 4:25 PM
> To: Ryan Chen <[e-mail]>; Andrew Jeffery
> <[e-mail]>; Brendan Higgins <[e-mail]>; Benjamin
> Herrenschmidt <[e-mail]>; Joel Stanley <[e-mail]>;
> Rob Herring <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Philipp Zabel <[e-mail]>;
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 26/02/2023 04:13, Ryan Chen wrote:
> > Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> > aspeed,xfer-mode description for ast2600-i2cv2.
> >
> > Signed-off-by: Ryan Chen <[e-mail]>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
> >  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
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -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.

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

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?

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

> > +
> >  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@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,
Ryan Chen.

Krzysztof Kozlowski March 3, 2023, 8:20 a.m. UTC | #9

On 01/03/2023 06:57, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Monday, February 27, 2023 4:25 PM
>> To: Ryan Chen <[e-mail]>; Andrew Jeffery
>> <[e-mail]>; Brendan Higgins <[e-mail]>; Benjamin
>> Herrenschmidt <[e-mail]>; Joel Stanley <[e-mail]>;
>> Rob Herring <[e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>; Philipp Zabel <[e-mail]>;
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 26/02/2023 04:13, Ryan Chen wrote:
>>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
>>> aspeed,xfer-mode description for ast2600-i2cv2.
>>>
>>> Signed-off-by: Ryan Chen <[e-mail]>
>>> ---
>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>>>  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
>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>> @@ -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.
> 
> 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 |          |                       |
> -------------------------                       ------------------------
> 
> 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?

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

> Could you point me what ref I can refer to?
> That I can check into Linux example. Thanks a lot.
>>
>>> +
>>> +  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)
>>
>>> +  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

yes



Best regards,
Krzysztof

Ryan Chen March 3, 2023, 8:28 a.m. UTC | #10

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Friday, March 3, 2023 4:20 PM
> To: Ryan Chen <[e-mail]>; Wolfram Sang
> <[e-mail]>
> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> [e-mail]; Philipp Zabel <[e-mail]>; Rob
> Herring <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 01/03/2023 06:57, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Monday, February 27, 2023 4:25 PM
> >> To: Ryan Chen <[e-mail]>; Andrew Jeffery
> >> <[e-mail]>; Brendan Higgins <[e-mail]>;
> >> Benjamin Herrenschmidt <[e-mail]>; Joel Stanley
> >> <[e-mail]>; Rob Herring <[e-mail]>; Krzysztof
> >> Kozlowski <[e-mail]>; Philipp Zabel
> >> <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 26/02/2023 04:13, Ryan Chen wrote:
> >>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> >>> aspeed,xfer-mode description for ast2600-i2cv2.
> >>>
> >>> Signed-off-by: Ryan Chen <[e-mail]>
> >>> ---
> >>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> +++++++++++++++++++
> >>>  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
> >>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>> @@ -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.
> >
> > 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 |          |
> |
> > -------------------------                       ------------------------
> >
> > 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?
> 
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.

> >>
> >>> +
> >>> +  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.
> >>
> >>> +    $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.
> 
> > Could you point me what ref I can refer to?
> > That I can check into Linux example. Thanks a lot.
> >>
> >>> +
> >>> +  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)
> >>
> >>> +  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
> 
> yes
> 
> 
> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski March 3, 2023, 8:50 a.m. UTC | #11

On 03/03/2023 09:28, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Friday, March 3, 2023 4:20 PM
>> To: Ryan Chen <[e-mail]>; Wolfram Sang
>> <[e-mail]>
>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
>> <[e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>; Andrew Jeffery <[e-mail]>;
>> [e-mail]; Philipp Zabel <[e-mail]>; Rob
>> Herring <[e-mail]>; Benjamin Herrenschmidt
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 01/03/2023 06:57, Ryan Chen wrote:
>>> Hello Krzysztof,
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: Monday, February 27, 2023 4:25 PM
>>>> To: Ryan Chen <[e-mail]>; Andrew Jeffery
>>>> <[e-mail]>; Brendan Higgins <[e-mail]>;
>>>> Benjamin Herrenschmidt <[e-mail]>; Joel Stanley
>>>> <[e-mail]>; Rob Herring <[e-mail]>; Krzysztof
>>>> Kozlowski <[e-mail]>; Philipp Zabel
>>>> <[e-mail]>; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail];
>>>> [e-mail]; [e-mail]
>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 26/02/2023 04:13, Ryan Chen wrote:
>>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
>>>>> aspeed,xfer-mode description for ast2600-i2cv2.
>>>>>
>>>>> Signed-off-by: Ryan Chen <[e-mail]>
>>>>> ---
>>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
>> +++++++++++++++++++
>>>>>  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
>>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>> @@ -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.
>>>
>>> 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 |          |
>> |
>>> -------------------------                       ------------------------
>>>
>>> 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?
>>
> 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 March 3, 2023, 8:55 a.m. UTC | #12

Hello Krzysztof,
	

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Friday, March 3, 2023 4:51 PM
> To: Ryan Chen <[e-mail]>; Wolfram Sang
> <[e-mail]>
> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> [e-mail]; Philipp Zabel <[e-mail]>; Rob
> Herring <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 03/03/2023 09:28, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Friday, March 3, 2023 4:20 PM
> >> To: Ryan Chen <[e-mail]>; Wolfram Sang
> >> <[e-mail]>
> >> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> >> <[e-mail]>; Krzysztof Kozlowski
> >> <[e-mail]>; Andrew Jeffery
> >> <[e-mail]>; [e-mail]; Philipp Zabel
> >> <[e-mail]>; Rob Herring <[e-mail]>; Benjamin
> >> Herrenschmidt <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 01/03/2023 06:57, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>> Sent: Monday, February 27, 2023 4:25 PM
> >>>> To: Ryan Chen <[e-mail]>; Andrew Jeffery
> >>>> <[e-mail]>; Brendan Higgins <[e-mail]>;
> >>>> Benjamin Herrenschmidt <[e-mail]>; Joel Stanley
> >>>> <[e-mail]>; Rob Herring <[e-mail]>; Krzysztof
> >>>> Kozlowski <[e-mail]>; Philipp Zabel
> >>>> <[e-mail]>; [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail];
> >>>> [e-mail]; [e-mail]
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 26/02/2023 04:13, Ryan Chen wrote:
> >>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>>>> aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
> >>>>>
> >>>>> Signed-off-by: Ryan Chen <[e-mail]>
> >>>>> ---
> >>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> >> +++++++++++++++++++
> >>>>>  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
> >>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>> @@ -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.
> >>>
> >>> 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 |          |
> >> |
> >>> -------------------------                       ------------------------
> >>>
> >>> 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?
> >>
> > 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 March 3, 2023, 9:26 a.m. UTC | #13

On 03/03/2023 09:55, Ryan Chen wrote:
> Hello Krzysztof,
> 	
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Friday, March 3, 2023 4:51 PM
>> To: Ryan Chen <[e-mail]>; Wolfram Sang
>> <[e-mail]>
>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
>> <[e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>; Andrew Jeffery <[e-mail]>;
>> [e-mail]; Philipp Zabel <[e-mail]>; Rob
>> Herring <[e-mail]>; Benjamin Herrenschmidt
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 03/03/2023 09:28, Ryan Chen wrote:
>>> Hello Krzysztof,
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: Friday, March 3, 2023 4:20 PM
>>>> To: Ryan Chen <[e-mail]>; Wolfram Sang
>>>> <[e-mail]>
>>>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
>>>> <[e-mail]>; Krzysztof Kozlowski
>>>> <[e-mail]>; Andrew Jeffery
>>>> <[e-mail]>; [e-mail]; Philipp Zabel
>>>> <[e-mail]>; Rob Herring <[e-mail]>; Benjamin
>>>> Herrenschmidt <[e-mail]>;
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]
>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 01/03/2023 06:57, Ryan Chen wrote:
>>>>> Hello Krzysztof,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>>>> Sent: Monday, February 27, 2023 4:25 PM
>>>>>> To: Ryan Chen <[e-mail]>; Andrew Jeffery
>>>>>> <[e-mail]>; Brendan Higgins <[e-mail]>;
>>>>>> Benjamin Herrenschmidt <[e-mail]>; Joel Stanley
>>>>>> <[e-mail]>; Rob Herring <[e-mail]>; Krzysztof
>>>>>> Kozlowski <[e-mail]>; Philipp Zabel
>>>>>> <[e-mail]>; [e-mail];
>>>>>> [e-mail]; [e-mail];
>>>>>> [e-mail];
>>>>>> [e-mail]; [e-mail]
>>>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>>>> AST2600-i2cv2
>>>>>>
>>>>>> On 26/02/2023 04:13, Ryan Chen wrote:
>>>>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
>>>>>>> aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
>>>>>>>
>>>>>>> Signed-off-by: Ryan Chen <[e-mail]>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
>>>> +++++++++++++++++++
>>>>>>>  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
>>>>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
>>>>>>> @@ -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.
>>>>>
>>>>> 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 |          |
>>>> |
>>>>> -------------------------                       ------------------------
>>>>>
>>>>> 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?
>>>>
>>> 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.

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

Best regards,
Krzysztof

Ryan Chen March 3, 2023, 10:16 a.m. UTC | #14

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Friday, March 3, 2023 5:26 PM
> To: Ryan Chen <[e-mail]>; Wolfram Sang
> <[e-mail]>
> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> [e-mail]; Philipp Zabel <[e-mail]>; Rob
> Herring <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> On 03/03/2023 09:55, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Friday, March 3, 2023 4:51 PM
> >> To: Ryan Chen <[e-mail]>; Wolfram Sang
> >> <[e-mail]>
> >> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> >> <[e-mail]>; Krzysztof Kozlowski
> >> <[e-mail]>; Andrew Jeffery
> >> <[e-mail]>; [e-mail]; Philipp Zabel
> >> <[e-mail]>; Rob Herring <[e-mail]>;
> Benjamin
> >> Herrenschmidt <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 03/03/2023 09:28, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>> Sent: Friday, March 3, 2023 4:20 PM
> >>>> To: Ryan Chen <[e-mail]>; Wolfram Sang
> >>>> <[e-mail]>
> >>>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> >>>> <[e-mail]>; Krzysztof Kozlowski
> >>>> <[e-mail]>; Andrew Jeffery
> >>>> <[e-mail]>; [e-mail]; Philipp Zabel
> >>>> <[e-mail]>; Rob Herring <[e-mail]>;
> >>>> Benjamin Herrenschmidt <[e-mail]>;
> >>>> [e-mail];
> >>>> [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 01/03/2023 06:57, Ryan Chen wrote:
> >>>>> Hello Krzysztof,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>>>> Sent: Monday, February 27, 2023 4:25 PM
> >>>>>> To: Ryan Chen <[e-mail]>; Andrew Jeffery
> >>>>>> <[e-mail]>; Brendan Higgins <[e-mail]>;
> >>>>>> Benjamin Herrenschmidt <[e-mail]>; Joel Stanley
> >>>>>> <[e-mail]>; Rob Herring <[e-mail]>; Krzysztof
> >>>>>> Kozlowski <[e-mail]>; Philipp Zabel
> >>>>>> <[e-mail]>; [e-mail];
> >>>>>> [e-mail]; [e-mail];
> >>>>>> [e-mail];
> >>>>>> [e-mail]; [e-mail]
> >>>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>>>> AST2600-i2cv2
> >>>>>>
> >>>>>> On 26/02/2023 04:13, Ryan Chen wrote:
> >>>>>>> Add ast2600-i2cv2 compatible and aspeed,global-regs,
> >>>>>>> aspeed,timeout aspeed,xfer-mode description for ast2600-i2cv2.
> >>>>>>>
> >>>>>>> Signed-off-by: Ryan Chen <[e-mail]>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> >>>> +++++++++++++++++++
> >>>>>>>  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
> >>>>>>> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> >>>>>>> @@ -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.
> >>>>>
> >>>>> 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 |          |
> >>>> |
> >>>>> -------------------------                       ------------------------
> >>>>>
> >>>>> 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?
> >>>>
> >>> 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.
> 
> 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 March 3, 2023, 10:41 a.m. UTC | #15

On 03/03/2023 11:16, Ryan Chen wrote:
>>>>>>> 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?
>>>>>>
>>>>> 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.
>>
>> 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 March 4, 2023, 1:33 a.m. UTC | #16

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Friday, March 3, 2023 6:41 PM
> To: Ryan Chen <[e-mail]>; Wolfram Sang
> <[e-mail]>
> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> [e-mail]; Philipp Zabel <[e-mail]>; Rob
> Herring <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 03/03/2023 11:16, Ryan Chen wrote:
> >>>>>>> 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?
> >>>>>>
> >>>>> 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.
> >>
> >> 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 March 5, 2023, 9:49 a.m. UTC | #17

On 04/03/2023 02:33, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Friday, March 3, 2023 6:41 PM
>> To: Ryan Chen <[e-mail]>; Wolfram Sang
>> <[e-mail]>
>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
>> <[e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>; Andrew Jeffery <[e-mail]>;
>> [e-mail]; Philipp Zabel <[e-mail]>; Rob
>> Herring <[e-mail]>; Benjamin Herrenschmidt
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 03/03/2023 11:16, Ryan Chen wrote:
>>>>>>>>> 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?
>>>>>>>>
>>>>>>> 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.
>>>>
>>>> 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? 

So, let's repeat my last questions:

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

You wrote:
> 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 March 6, 2023, 12:48 a.m. UTC | #18

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Sunday, March 5, 2023 5:49 PM
> To: Ryan Chen <[e-mail]>; Wolfram Sang
> <[e-mail]>
> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> [e-mail]; Philipp Zabel <[e-mail]>; Rob
> Herring <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 04/03/2023 02:33, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Friday, March 3, 2023 6:41 PM
> >> To: Ryan Chen <[e-mail]>; Wolfram Sang
> >> <[e-mail]>
> >> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> >> <[e-mail]>; Krzysztof Kozlowski
> >> <[e-mail]>; Andrew Jeffery
> >> <[e-mail]>; [e-mail]; Philipp Zabel
> >> <[e-mail]>; Rob Herring <[e-mail]>; Benjamin
> >> Herrenschmidt <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 03/03/2023 11:16, Ryan Chen wrote:
> >>>>>>>>> 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?
> >>>>>>>>
> >>>>>>> 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.
> >>>>
> >>>> 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?
> 
> So, let's repeat my last questions:
> 
> 1. Why you wouldn't enable timeout always...
> 
> You wrote:
> > 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. 
 
> 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 March 7, 2023, 8:11 a.m. UTC | #19

On 06/03/2023 01:48, Ryan Chen wrote:
> Hello Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[e-mail]>
>> Sent: Sunday, March 5, 2023 5:49 PM
>> To: Ryan Chen <[e-mail]>; Wolfram Sang
>> <[e-mail]>
>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
>> <[e-mail]>; Krzysztof Kozlowski
>> <[e-mail]>; Andrew Jeffery <[e-mail]>;
>> [e-mail]; Philipp Zabel <[e-mail]>; Rob
>> Herring <[e-mail]>; Benjamin Herrenschmidt
>> <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
>>
>> On 04/03/2023 02:33, Ryan Chen wrote:
>>> Hello Krzysztof,
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <[e-mail]>
>>>> Sent: Friday, March 3, 2023 6:41 PM
>>>> To: Ryan Chen <[e-mail]>; Wolfram Sang
>>>> <[e-mail]>
>>>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
>>>> <[e-mail]>; Krzysztof Kozlowski
>>>> <[e-mail]>; Andrew Jeffery
>>>> <[e-mail]>; [e-mail]; Philipp Zabel
>>>> <[e-mail]>; Rob Herring <[e-mail]>; Benjamin
>>>> Herrenschmidt <[e-mail]>;
>>>> [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail];
>>>> [e-mail]
>>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
>>>> AST2600-i2cv2
>>>>
>>>> On 03/03/2023 11:16, Ryan Chen wrote:
>>>>>>>>>>> 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?
>>>>>>>>>>
>>>>>>>>> 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.
>>>>>>
>>>>>> 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?
>>
>> So, let's repeat my last questions:
>>
>> 1. Why you wouldn't enable timeout always...
>>
>> You wrote:
>>> 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?

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

>  
>> 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 March 7, 2023, 10:09 a.m. UTC | #20

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Tuesday, March 7, 2023 4:12 PM
> To: Ryan Chen <[e-mail]>; Wolfram Sang
> <[e-mail]>
> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> [e-mail]; Philipp Zabel <[e-mail]>; Rob
> Herring <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 06/03/2023 01:48, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <[e-mail]>
> >> Sent: Sunday, March 5, 2023 5:49 PM
> >> To: Ryan Chen <[e-mail]>; Wolfram Sang
> >> <[e-mail]>
> >> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> >> <[e-mail]>; Krzysztof Kozlowski
> >> <[e-mail]>; Andrew Jeffery
> >> <[e-mail]>; [e-mail]; Philipp Zabel
> >> <[e-mail]>; Rob Herring <[e-mail]>; Benjamin
> >> Herrenschmidt <[e-mail]>;
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]
> >> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >> AST2600-i2cv2
> >>
> >> On 04/03/2023 02:33, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <[e-mail]>
> >>>> Sent: Friday, March 3, 2023 6:41 PM
> >>>> To: Ryan Chen <[e-mail]>; Wolfram Sang
> >>>> <[e-mail]>
> >>>> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> >>>> <[e-mail]>; Krzysztof Kozlowski
> >>>> <[e-mail]>; Andrew Jeffery
> >>>> <[e-mail]>; [e-mail]; Philipp Zabel
> >>>> <[e-mail]>; Rob Herring <[e-mail]>;
> >>>> Benjamin Herrenschmidt <[e-mail]>;
> >>>> [e-mail];
> >>>> [e-mail];
> >>>> [e-mail]; [e-mail];
> >>>> [e-mail]
> >>>> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> >>>> AST2600-i2cv2
> >>>>
> >>>> On 03/03/2023 11:16, Ryan Chen wrote:
> >>>>>>>>>>> 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?
> >>>>>>>>>>
> >>>>>>>>> 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.
> >>>>>>
> >>>>>> 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
> >>>
> >>>   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...
> >>
> >> You wrote:
> >>> 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?

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

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

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.

> >
> >> 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 March 9, 2023, 8:51 a.m. UTC | #21

On 07/03/2023 11:09, Ryan Chen wrote:
>>>> 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.

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 March 9, 2023, 9:15 a.m. UTC | #22

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <[e-mail]>
> Sent: Thursday, March 9, 2023 4:52 PM
> To: Ryan Chen <[e-mail]>; Wolfram Sang
> <[e-mail]>
> Cc: Joel Stanley <[e-mail]>; Brendan Higgins
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Andrew Jeffery <[e-mail]>;
> [e-mail]; Philipp Zabel <[e-mail]>; Rob
> Herring <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2
> 
> On 07/03/2023 11:09, Ryan Chen wrote:
> >>>> 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.
> 
> 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 March 12, 2023, 12:33 p.m. UTC | #23

Hi Krzysztof and Ryan,

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

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 March 18, 2023, 9:09 a.m. UTC | #24

Hi Ryan,

On Sun, Feb 26, 2023 at 11:13:20AM +0800, Ryan Chen wrote:
> Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> aspeed,xfer-mode description for ast2600-i2cv2.
> 
> Signed-off-by: Ryan Chen <[e-mail]>
> ---
>  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44 +++++++++++++++++++
>  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
> --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> @@ -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 March 19, 2023, 2:05 a.m. UTC | #25

Hello,

> -----Original Message-----
> From: Andi Shyti <[e-mail]>
> Sent: Saturday, March 18, 2023 5:10 PM
> To: Ryan Chen <[e-mail]>
> Cc: Andrew Jeffery <[e-mail]>; Brendan Higgins
> <[e-mail]>; Benjamin Herrenschmidt
> <[e-mail]>; Joel Stanley <[e-mail]>; Rob Herring
> <[e-mail]>; Krzysztof Kozlowski
> <[e-mail]>; Philipp Zabel
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
> AST2600-i2cv2
> 
> Hi Ryan,
> 
> On Sun, Feb 26, 2023 at 11:13:20AM +0800, Ryan Chen wrote:
> > Add ast2600-i2cv2 compatible and aspeed,global-regs, aspeed,timeout
> > aspeed,xfer-mode description for ast2600-i2cv2.
> >
> > Signed-off-by: Ryan Chen <[e-mail]>
> > ---
> >  .../devicetree/bindings/i2c/aspeed,i2c.yaml   | 44
> +++++++++++++++++++
> >  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
> > --- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
> > @@ -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.

Thanks a lot, I will start progress this. 

Best Regards
Ryan

Patch

diff --git a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
index f597f73ccd87..75de3ce41cf5 100644
--- a/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/aspeed,i2c.yaml
@@ -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@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>;
+    };