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. > >> > >>> + > >>> + aspeed,xfer-mode: > >>> + description: | > >>> + I2C bus transfer mode selection. > >>> + - "byte": I2C bus byte transfer mode. > >>> + - "buffered": I2C bus buffer register transfer mode. > >>> + - "dma": I2C bus dma transfer mode (default) > >>> + items: > >>> + enum: [byte, buffered, dma] > >>> + maxItems: 1 > >> > >> Drop, not an array. > >> > >>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > >> > >> Wrong ref. This is not an array, but one string. > > > > Sorry, I can't catch your "one string" point. > > How many strings you are going to have in this property? If one > (maxItems: 1), then this is not an array. > > > Could you point me what ref I can refer to? > > That I can check into Linux example. Thanks a lot. > >> > >>> + > >>> + aspeed,global-regs: > >>> + $ref: /schemas/types.yaml#/definitions/phandle > >>> + description: The phandle of i2c global register node. > >>> + > >>> required: > >>> - reg > >>> - compatible > >>> @@ -57,6 +76,19 @@ required: > >>> > >>> unevaluatedProperties: false > >>> > >>> +if: > >> > >> This should be under allOf (in this location) > >> > >>> + properties: > >>> + compatible: > >>> + contains: > >>> + const: aspeed,ast2600-i2cv2 > >>> + > >>> +then: > >>> + properties: > >>> + reg: > >>> + minItems: 2 > >>> + required: > >>> + - aspeed,global-regs > >> > >> else: > >> aspeed,global-regs: false > >> and the same for other v2 properties > >> > > > > Does modify by following? > > > > allOf: > > -if: > > properties: > > compatible: > > contains: > > const: aspeed,ast2600-i2cv2 > > > > then: > > properties: > > reg: > > minItems: 2 > > required: > > - aspeed,global-regs > > else: > > - aspeed,global-regs: false > > -aspeed,timeout: false > > - aspeed,xfer-mode: false > > yes > > > > Best regards, > Krzysztof
