Message-ID: <c5cf6e0a-05dc-dff7-6218-df0984d1ba47@linaro.org>
Date:   Fri, 3 Mar 2023 11:41:10 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v6 1/2] dt-bindings: i2c: aspeed: support for
 AST2600-i2cv2
Content-Language: en-US
To:     Ryan Chen <ryan_chen@aspeedtech.com>, Wolfram Sang <wsa@kernel.org>
Cc:     Joel Stanley <joel@jms.id.au>,
        Brendan Higgins <brendan.higgins@linux.dev>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Andrew Jeffery <andrew@aj.id.au>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Rob Herring <robh+dt@kernel.org>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
        "linux-i2c@vger.kernel.org" <linux-i2c@vger.kernel.org>
References: <20230226031321.3126756-1-ryan_chen@aspeedtech.com>
 <20230226031321.3126756-2-ryan_chen@aspeedtech.com>
 <53090449-58c9-bc03-56df-aa8ae93c0c26@linaro.org>
 <SEZPR06MB52699DEB2255EB54F35C2A59F2AD9@SEZPR06MB5269.apcprd06.prod.outlook.com>
 <fc20a2d1-e2f9-c22b-dcdf-153cb527eea8@linaro.org>
 <SEZPR06MB526902637624A97D7DFB8523F2B39@SEZPR06MB5269.apcprd06.prod.outlook.com>
 <c41ee6b5-ddb4-1253-de54-a295b3bab2cc@linaro.org>
 <SEZPR06MB5269E7B8785B3CB56A63A916F2B39@SEZPR06MB5269.apcprd06.prod.outlook.com>
 <a3615fe7-aa2a-53e9-2732-ba4512b9369d@linaro.org>
 <SEZPR06MB5269E7B070B239F8E349C427F2B39@SEZPR06MB5269.apcprd06.prod.outlook.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <SEZPR06MB5269E7B070B239F8E349C427F2B39@SEZPR06MB5269.apcprd06.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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/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. 

No, because none of my concerns above are addressed.

Best regards,
Krzysztof