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
