linux-kernel - Re: [PATCH v3 1/2] dt-bindings: i2c: snps,designware-i2c: declare bus capacitance and clk freq optimized

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: <f67bcf92-ded6-42cb-827b-8a3d79f8f7d0@kernel.org>
Date: Wed, 2 Oct 2024 11:50:28 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Michael Wu <Michael.Wu@...ron.us>
Cc: Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Jarkko Nikula <jarkko.nikula@...ux.intel.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 Mika Westerberg <mika.westerberg@...ux.intel.com>,
 Jan Dabros <jsd@...ihalf.com>,
 "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 morgan chang <morgan.chang@...ron.us>,
 "mvp.kutali@...il.com" <mvp.kutali@...il.com>
Subject: Re: [PATCH v3 1/2] dt-bindings: i2c: snps,designware-i2c: declare bus
 capacitance and clk freq optimized

On 02/10/2024 11:20, Michael Wu wrote:
>> On Tue, Oct 01, 2024 at 04:29:33PM +0800, Michael Wu wrote:
>>> Since there are no registers controlling the hardware parameters
>>> IC_CAP_LOADING and IC_CLK_FREQ_OPTIMIZATION, their values can only be
>>> declared in the device tree.
>>>
>>> snps,bus-capacitance-pf indicates the bus capacitance in picofarads (pF).
>>> It affects the high and low pulse width of SCL line in high speed mode.
>>> The legal values for this property are 100 and 400 only, and default
>>> value is 100. This property corresponds to IC_CAP_LOADING.
>>>
>>> snps,clk-freq-optimized indicates whether the hardware input clock
>>> frequency is reduced by reducing the internal latency. This property
>>> corresponds to IC_CLK_FREQ_OPTIMIZATION.
>>>
>>> The driver can calculate hs_hcnt and hs_lcnt appropriate for the hardware
>>> based on these two properties.
>>>
>>> Signed-off-by: Michael Wu <michael.wu@...ron.us>
>>> ---
>>>  .../bindings/i2c/snps,designware-i2c.yaml     | 24
>> +++++++++++++++++++
>>>  1 file changed, 24 insertions(+)
>>>
>>> diff --git
>> a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
>> b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
>>> index 60035a787e5c..c373f3acd34b 100644
>>> --- a/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
>>> +++ b/Documentation/devicetree/bindings/i2c/snps,designware-i2c.yaml
> ...
>>> +      This property indicates the bus capacitance in picofarads (pF).
>>> +      This value is used to compute the tHIGH and tLOW periods for high
>> speed
>>> +      mode.
>>> +    default: 100
>>
>> I asked for some constraints here. min/maximum. I think you never
>> replied to this.
>>
> 
> In I2C DesignWare Databook v2.03a the mandatory option is provided to
> select whether the bus capacitance is 400pF or 100pF. It presents the
> description like that:
> 
>   Description:
>     For high speed mode, the bus loading (pF) affects the high and low
>     pulse width of SCL.
>   Values: 100, 400
>   Default Value: 100
>   Parameter Name: IC_CAP_LOADING
> 
> There is no further information describing this option except to the
> declaration of legal values above, let alone minimum and maximum limits.

So only two values are valid? Then not min/max but enum.


> As a user I don't think I have the right to define a value range for the
> vendor.
> 
> From the information provided in the data sheet, I prefer to list the
> legal values like the following:
> 
>   enum: [100, 400]
>   default: 100
> 
> instead of declaring its range. What do you think?
> 
> In patches v2 I used if (dev->bus_capacitance_pf == 400) {... } else {...}
> and other statements in the driver code to indicate that the capacitance
> can only be 400pf or not. Maybe this is a metaphor. I'm sorry that I
> wasn't more explicit about the constraints.
> 
>>> +
>>> +  snps,clk-freq-optimized:
>>> +    description: >
>>> +      This property indicates whether the hardware input clock frequency
>> is
>>> +      reduced by reducing the internal latency. This value is used to
>> compute
>>> +      the tHIGH and tLOW periods for high speed mode.
>>> +    type: boolean
>>> +
>>>  unevaluatedProperties: false
>>>
>>>  required:
>>> @@ -146,4 +161,13 @@ examples:
>>>        interrupts = <8>;
>>>        clocks = <&ahb_clk>;
>>>      };
>>> +  - |
>>> +    i2c@...00000 {
>>> +      compatible = "snps,designware-i2c";
>>
>> Extend EXISTING example. Not add new example.
> 
> Should I insert these two properties into one or all existing examples?

Into any example, where it looks reasonable.

Best regards,
Krzysztof