On Wed, Jun 19, 2019 at 2:00 PM Tao Ren <[email protected]> wrote:
>
> Some intermittent I2C transaction failures are observed on Facebook CMM and
> Minipack (ast2500) BMC platforms, because slave devices (such as CPLD, BIC
> and etc.) NACK the address byte sometimes. The issue can be resolved by
> increasing base clock divisor which affects ASPEED I2C Controller's base
> clock and other AC timing parameters.
>
> This patch allows to customize ASPEED I2C Controller's base clock divisor
> in device tree.
First off, are you sure you actually need this?
You should be able to achieve an effectively equivalent result by just
lowering the `bus-frequency` property specified in the DT. The
`bus-frequency` property ultimately determines all the register
values, and you should be able to set it to whatever you want by
refering to the Aspeed documentation.
Nevertheless, the code that determines the correct dividers from the
frequency is based on the tables in the Aspeed documentation. I don't
think the equation makes sense when the base_clk_divisor is fixed; I
mean it will probably just set the other divisor to max or min
depending on the values chosen. I think if someone really wants to
program this parameter manually, they probably want to set the other
parameters manually too.
[snip]
On 6/19/19 2:25 PM, Brendan Higgins wrote:
> On Wed, Jun 19, 2019 at 2:00 PM Tao Ren <[email protected]> wrote:
>>
>> Some intermittent I2C transaction failures are observed on Facebook CMM and
>> Minipack (ast2500) BMC platforms, because slave devices (such as CPLD, BIC
>> and etc.) NACK the address byte sometimes. The issue can be resolved by
>> increasing base clock divisor which affects ASPEED I2C Controller's base
>> clock and other AC timing parameters.
>>
>> This patch allows to customize ASPEED I2C Controller's base clock divisor
>> in device tree.
>
> First off, are you sure you actually need this?
>
> You should be able to achieve an effectively equivalent result by just
> lowering the `bus-frequency` property specified in the DT. The
> `bus-frequency` property ultimately determines all the register
> values, and you should be able to set it to whatever you want by
> refering to the Aspeed documentation.
>
> Nevertheless, the code that determines the correct dividers from the
> frequency is based on the tables in the Aspeed documentation. I don't
> think the equation makes sense when the base_clk_divisor is fixed; I
> mean it will probably just set the other divisor to max or min
> depending on the values chosen. I think if someone really wants to
> program this parameter manually, they probably want to set the other
> parameters manually too.
Thank you for the quick response, Brendan.
Aspeed I2C bus frequency is defined by 3 parameters (base_clk_divisor, clk_high_width, clk_low_width), and I choose base_clk_divisor because it controls all the Aspeed I2C timings (such as setup time and hold time). Once base_clk_divisor is decided (either by the current logic in i2c-aspeed driver or manually set in device tree), clk_high_width and clk_low_width will be calculated by i2c-aspeed driver to meet the specified I2C bus speed.
For example, by setting I2C bus frequency to 100KHz on AST2500 platform, (base_clock_divisor, clk_high_width, clk_low_width) is set to (3, 15, 14) by our driver. But some slave devices (on CMM i2c-8 and Minipack i2c-0) NACK byte transactions with the default timing setting: the issue can be resolved by setting base_clk_divisor to 4, and (clk_high_width, clk_low_width) will be set to (7, 7) by our i2c-aspeed driver to achieve similar I2C bus speed.
Not sure if my answer helps to address your concerns, but kindly let me know if you have further questions/suggestions.
Thanks,
Tao
On Wed, 2019-06-19 at 22:32 +0000, Tao Ren wrote:
> Thank you for the quick response, Brendan.
>
> Aspeed I2C bus frequency is defined by 3 parameters
> (base_clk_divisor, clk_high_width, clk_low_width), and I choose
> base_clk_divisor because it controls all the Aspeed I2C timings (such
> as setup time and hold time). Once base_clk_divisor is decided
> (either by the current logic in i2c-aspeed driver or manually set in
> device tree), clk_high_width and clk_low_width will be calculated by
> i2c-aspeed driver to meet the specified I2C bus speed.
>
> For example, by setting I2C bus frequency to 100KHz on AST2500
> platform, (base_clock_divisor, clk_high_width, clk_low_width) is set
> to (3, 15, 14) by our driver. But some slave devices (on CMM i2c-8
> and Minipack i2c-0) NACK byte transactions with the default timing
> setting: the issue can be resolved by setting base_clk_divisor to 4,
> and (clk_high_width, clk_low_width) will be set to (7, 7) by our i2c-
> aspeed driver to achieve similar I2C bus speed.
>
> Not sure if my answer helps to address your concerns, but kindly let
> me know if you have further questions/suggestions.
Did you look at the resulting output on a scope ? I'm curious what
might be wrong....
CCing Ryan from Aspeed, he might have some idea.
Could it be that with some specific dividers you have more jitter ?
Still, i2c devices tend to be rather robust vs crappy clocks unless you
are massively out of bounds, which makes me wonder whether something
else might be wrong in your setup.
Cheers,
Ben.
On 6/19/19 4:02 PM, Benjamin Herrenschmidt wrote:
> On Wed, 2019-06-19 at 22:32 +0000, Tao Ren wrote:
>> Thank you for the quick response, Brendan.
>>
>> Aspeed I2C bus frequency is defined by 3 parameters
>> (base_clk_divisor, clk_high_width, clk_low_width), and I choose
>> base_clk_divisor because it controls all the Aspeed I2C timings (such
>> as setup time and hold time). Once base_clk_divisor is decided
>> (either by the current logic in i2c-aspeed driver or manually set in
>> device tree), clk_high_width and clk_low_width will be calculated by
>> i2c-aspeed driver to meet the specified I2C bus speed.
>>
>> For example, by setting I2C bus frequency to 100KHz on AST2500
>> platform, (base_clock_divisor, clk_high_width, clk_low_width) is set
>> to (3, 15, 14) by our driver. But some slave devices (on CMM i2c-8
>> and Minipack i2c-0) NACK byte transactions with the default timing
>> setting: the issue can be resolved by setting base_clk_divisor to 4,
>> and (clk_high_width, clk_low_width) will be set to (7, 7) by our i2c-
>> aspeed driver to achieve similar I2C bus speed.
>>
>> Not sure if my answer helps to address your concerns, but kindly let
>> me know if you have further questions/suggestions.
>
> Did you look at the resulting output on a scope ? I'm curious what
> might be wrong....
>
> CCing Ryan from Aspeed, he might have some idea.
>
> Could it be that with some specific dividers you have more jitter ?
> Still, i2c devices tend to be rather robust vs crappy clocks unless you
> are massively out of bounds, which makes me wonder whether something
> else might be wrong in your setup.
>
> Cheers,
> Ben.
I've reached out to hardware team to see if they can provide more inputs (such as protocol decoder output) but so far I don't have such data. I'm suspecting it's caused by I2C timing mainly because:
1) the intermittent i2c transaction failures always happen to slave devices which are furthest away from ASPEED chip.
2) As the i2c-aspeed driver in my kernel 4.1 tree (derived from ASPEED SDK) works properly, and I copied I2CD04 (Clock and AC Timing Control) register value from kernel 4.1 and applied to the latest upstream driver: the transaction failure is fixed :)
Thank you Ben for looking into the issue and involving more experts (Ryan) for discussion. I have been suffering from the problem for several months and I'm looking forward for proper/right solutions.
Cheers,
Tao
Hello Tao,
Our recommend about clk divider setting is follow the datasheet clock setting table for clock divisor.
Ryan
-----Original Message-----
From: Linux-aspeed [mailto:[email protected]] On Behalf Of Tao Ren
Sent: Thursday, June 20, 2019 6:33 AM
To: Brendan Higgins <[email protected]>
Cc: Mark Rutland <[email protected]>; devicetree <[email protected]>; [email protected]; OpenBMC Maillist <[email protected]>; Linux Kernel Mailing List <[email protected]>; Rob Herring <[email protected]>; Linux ARM <[email protected]>; [email protected]
Subject: Re: [PATCH 1/2] i2c: aspeed: allow to customize base clock divisor
On 6/19/19 2:25 PM, Brendan Higgins wrote:
> On Wed, Jun 19, 2019 at 2:00 PM Tao Ren <[email protected]> wrote:
>>
>> Some intermittent I2C transaction failures are observed on Facebook
>> CMM and Minipack (ast2500) BMC platforms, because slave devices (such
>> as CPLD, BIC and etc.) NACK the address byte sometimes. The issue can
>> be resolved by increasing base clock divisor which affects ASPEED I2C
>> Controller's base clock and other AC timing parameters.
>>
>> This patch allows to customize ASPEED I2C Controller's base clock
>> divisor in device tree.
>
> First off, are you sure you actually need this?
>
> You should be able to achieve an effectively equivalent result by just
> lowering the `bus-frequency` property specified in the DT. The
> `bus-frequency` property ultimately determines all the register
> values, and you should be able to set it to whatever you want by
> refering to the Aspeed documentation.
>
> Nevertheless, the code that determines the correct dividers from the
> frequency is based on the tables in the Aspeed documentation. I don't
> think the equation makes sense when the base_clk_divisor is fixed; I
> mean it will probably just set the other divisor to max or min
> depending on the values chosen. I think if someone really wants to
> program this parameter manually, they probably want to set the other
> parameters manually too.
Thank you for the quick response, Brendan.
Aspeed I2C bus frequency is defined by 3 parameters (base_clk_divisor, clk_high_width, clk_low_width), and I choose base_clk_divisor because it controls all the Aspeed I2C timings (such as setup time and hold time). Once base_clk_divisor is decided (either by the current logic in i2c-aspeed driver or manually set in device tree), clk_high_width and clk_low_width will be calculated by i2c-aspeed driver to meet the specified I2C bus speed.
For example, by setting I2C bus frequency to 100KHz on AST2500 platform, (base_clock_divisor, clk_high_width, clk_low_width) is set to (3, 15, 14) by our driver. But some slave devices (on CMM i2c-8 and Minipack i2c-0) NACK byte transactions with the default timing setting: the issue can be resolved by setting base_clk_divisor to 4, and (clk_high_width, clk_low_width) will be set to (7, 7) by our i2c-aspeed driver to achieve similar I2C bus speed.
Not sure if my answer helps to address your concerns, but kindly let me know if you have further questions/suggestions.
Thanks,
Tao