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: Tuesday, March 7, 2023 4:12 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 06/03/2023 01:48, Ryan Chen wrote:
> > Hello Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >> Sent: Sunday, March 5, 2023 5:49 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 04/03/2023 02:33, Ryan Chen wrote:
> >>> Hello Krzysztof,
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> >>>> Sent: Friday, March 3, 2023 6:41 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 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