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: <SEZPR06MB5269CB53B51B89C3CA039442F2B09@SEZPR06MB5269.apcprd06.prod.outlook.com>
Date:   Sat, 4 Mar 2023 01:33:42 +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: 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/devicetr
> >>> 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/devicetree/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? 

Best regards,
Ryan Chen