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. Best regards, Krzysztof
