linux-kernel - RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for AST2600-i2cv2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <SEZPR06MB5269CA376D572B6241FCAD4DF2B79@SEZPR06MB5269.apcprd06.prod.outlook.com>
Date:   Tue, 7 Mar 2023 10:09:51 +0000
From:   Ryan Chen <ryan_chen@...eedtech.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Wolfram Sang <wsa@...nel.org>
CC:     Joel Stanley <joel@....id.au>,
        Brendan Higgins <brendan.higgins@...ux.dev>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andrew Jeffery <andrew@...id.au>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Rob Herring <robh+dt@...nel.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        "linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "openbmc@...ts.ozlabs.org" <openbmc@...ts.ozlabs.org>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>
Subject: RE: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
 AST2600-i2cv2

Hello Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Tuesday, March 7, 2023 4:12 PM
> To: Ryan Chen <ryan_chen@...eedtech.com>; Wolfram Sang
> <wsa@...nel.org>
> Cc: Joel Stanley <joel@....id.au>; Brendan Higgins
> <brendan.higgins@...ux.dev>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@...aro.org>; Andrew Jeffery <andrew@...id.au>;
> devicetree@...r.kernel.org; Philipp Zabel <p.zabel@...gutronix.de>; Rob
> Herring <robh+dt@...nel.org>; Benjamin Herrenschmidt
> <benh@...nel.crashing.org>; linux-aspeed@...ts.ozlabs.org;
> linux-arm-kernel@...ts.infradead.org; linux-kernel@...r.kernel.org;
> openbmc@...ts.ozlabs.org; linux-i2c@...r.kernel.org
> 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@...aro.org>
> >> Sent: Sunday, March 5, 2023 5:49 PM
> >> To: Ryan Chen <ryan_chen@...eedtech.com>; Wolfram Sang
> >> <wsa@...nel.org>
> >> Cc: Joel Stanley <joel@....id.au>; Brendan Higgins
> >> <brendan.higgins@...ux.dev>; Krzysztof Kozlowski
> >> <krzysztof.kozlowski+dt@...aro.org>; Andrew Jeffery
> >> <andrew@...id.au>; devicetree@...r.kernel.org; Philipp Zabel
> >> <p.zabel@...gutronix.de>; Rob Herring <robh+dt@...nel.org>; Benjamin
> >> Herrenschmidt <benh@...nel.crashing.org>;
> >> linux-aspeed@...ts.ozlabs.org; linux-arm-kernel@...ts.infradead.org;
> >> linux-kernel@...r.kernel.org; openbmc@...ts.ozlabs.org;
> >> linux-i2c@...r.kernel.org
> >> 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@...aro.org>
> >>>> Sent: Friday, March 3, 2023 6:41 PM
> >>>> To: Ryan Chen <ryan_chen@...eedtech.com>; Wolfram Sang
> >>>> <wsa@...nel.org>
> >>>> Cc: Joel Stanley <joel@....id.au>; Brendan Higgins
> >>>> <brendan.higgins@...ux.dev>; Krzysztof Kozlowski
> >>>> <krzysztof.kozlowski+dt@...aro.org>; Andrew Jeffery
> >>>> <andrew@...id.au>; devicetree@...r.kernel.org; Philipp Zabel
> >>>> <p.zabel@...gutronix.de>; Rob Herring <robh+dt@...nel.org>;
> >>>> Benjamin Herrenschmidt <benh@...nel.crashing.org>;
> >>>> linux-aspeed@...ts.ozlabs.org;
> >>>> linux-arm-kernel@...ts.infradead.org;
> >>>> linux-kernel@...r.kernel.org; openbmc@...ts.ozlabs.org;
> >>>> linux-i2c@...r.kernel.org
> >>>> 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