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

[Ryan Chen]

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Friday, March 3, 2023 4:20 PM
> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>; Wolfram Sang
> <wsa@xxxxxxxxxx>
> Cc: Joel Stanley <joel@xxxxxxxxx>; Brendan Higgins
> <brendan.higgins@xxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Andrew Jeffery <andrew@xxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>; Rob
> Herring <robh+dt@xxxxxxxxxx>; Benjamin Herrenschmidt
> <benh@xxxxxxxxxxxxxxxxxxx>; linux-aspeed@xxxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> openbmc@xxxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx
> 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 <krzysztof.kozlowski@xxxxxxxxxx>
> >> Sent: Monday, February 27, 2023 4:25 PM
> >> To: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx>; Andrew Jeffery
> >> <andrew@xxxxxxxx>; Brendan Higgins <brendan.higgins@xxxxxxxxx>;
> >> Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>; Joel Stanley
> >> <joel@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof
> >> Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Philipp Zabel
> >> <p.zabel@xxxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx;
> >> openbmc@xxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> >> linux-aspeed@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> >> 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 <ryan_chen@xxxxxxxxxxxxxx>
> >>> ---
> >>>  .../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