| Message ID | 20241001082937.680372-1-michael.wu@kneron.us |
|---|---|
| Headers | show |
| Series | Compute HS HCNT and LCNT based on HW parameters | expand |
> 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@kneron.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. 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@c5000000 { > > + compatible = "snps,designware-i2c"; > > Extend EXISTING example. Not add new example. Should I insert these two properties into one or all existing examples? Thanks & Regards Michael
> 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@kneron.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. 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@c5000000 { > > + compatible = "snps,designware-i2c"; > > Extend EXISTING example. Not add new example. Should I insert these two properties into one or all existing examples? Thanks & Regards Michael
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@kneron.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@c5000000 { >>> + 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
In commit 35eba185fd1a ("i2c: designware: Calculate SCL timing parameters for High Speed Mode") hs_hcnt and hs_lcnt are computed based on fixed tHIGH = 160 and tLOW = 320. However, the set of these fixed values only applies to the combination of hardware parameters "IC_CAP_LOADING = 400pF" and "IC_FREQ_OPTIMIZATION = 1". Outside of this combination, SCL frequency may not reach 3.4 MHz if hs_hcnt and hs_lcnt are both computed using these two fixed values. Since there are no any registers controlling these two hardware parameters, their values can only be declared through the device tree. v3: - add vendor prefix on new property name - read new properties in i2c_dw_fw_parse_and_configure() directly - in i2c_dw_set_timings_master() check dev->bus_capacitance_pf and then decide t_high and t_low v2: - provide more hardware information in dt-bindings - rename "bus-loading" to "bus-capacitance-pf" - call new i2c_dw_fw_parse_hw_params() in i2c_dw_fw_parse_and_configure() to parse hardware parameters from the device tree. Michael Wu (2): dt-bindings: i2c: snps,designware-i2c: declare bus capacitance and clk freq optimized i2c: dwsignware: determine HS tHIGH and tLOW based on HW parameters .../bindings/i2c/snps,designware-i2c.yaml | 24 +++++++++++++++++++ drivers/i2c/busses/i2c-designware-common.c | 5 ++++ drivers/i2c/busses/i2c-designware-core.h | 5 ++++ drivers/i2c/busses/i2c-designware-master.c | 23 ++++++++++++++++-- 4 files changed, 55 insertions(+), 2 deletions(-)