Hello Krzysztof, > -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: Friday, March 3, 2023 5:26 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 09:55, Ryan Chen wrote: > > Hello Krzysztof, > > > > > >> -----Original Message----- > >> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > >> Sent: Friday, March 3, 2023 4:51 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 09:28, Ryan Chen wrote: > >>> 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. > >> > >> 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
