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