[0/3] Ensure Low period of SCL is correct

Message

Lucas Tanure March 26, 2022, 10:22 a.m. UTC

The default duty cycle of 33% is less than the required
by the I2C specs for the LOW period of the SCL clock.

So, for 100Khz or less, use 50%H/50%L duty cycle, and
for the clock above 100Khz, use 40%H/60%L duty cycle.
That ensures the low period of SCL is always more than
the minimum required by the specs at any given frequency.

Lucas Tanure (3):
  i2c: meson: Use _SHIFT and _MASK for register definitions
  i2c: meson: Use 50% duty cycle for I2C clock
  i2c: meson: Remove meson_i2c_data

 drivers/i2c/busses/i2c-meson.c | 104 ++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 48 deletions(-)

Comments

Kevin Hilman March 28, 2022, 8:37 p.m. UTC | #1

Hi Lucas,

Lucas Tanure <[e-mail]> writes:

> The default duty cycle of 33% is less than the required
> by the I2C specs for the LOW period of the SCL clock.
>
> So, for 100Khz or less, use 50%H/50%L duty cycle, and
> for the clock above 100Khz, use 40%H/60%L duty cycle.
> That ensures the low period of SCL is always more than
> the minimum required by the specs at any given frequency.

Thanks for the fixes!

This is going to affect all SoCs, so ould you please summarize how your
changes were tested, and on which SoCs & boards?

Thanks,

Kevin

Lucas Tanure March 28, 2022, 10:31 p.m. UTC | #2

On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <[e-mail]> wrote:
>
> Hi Lucas,
>
> Lucas Tanure <[e-mail]> writes:
>
> > The default duty cycle of 33% is less than the required
> > by the I2C specs for the LOW period of the SCL clock.
> >
> > So, for 100Khz or less, use 50%H/50%L duty cycle, and
> > for the clock above 100Khz, use 40%H/60%L duty cycle.
> > That ensures the low period of SCL is always more than
> > the minimum required by the specs at any given frequency.
>
> Thanks for the fixes!
>
> This is going to affect all SoCs, so ould you please summarize how your
> changes were tested, and on which SoCs & boards?
>
> Thanks,
>
> Kevin

Hi,

I only tested against the vim3 board, measured the bus with a Saleae
logic pro 16.
The measurements were with 100k, 400k, and a few in-between frequencies.

Is that enough?

Thanks
Lucas

Neil Armstrong April 4, 2022, 8:01 a.m. UTC | #3

Hi,

On 29/03/2022 00:31, Lucas Tanure wrote:
> On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <[e-mail]> wrote:
>>
>> Hi Lucas,
>>
>> Lucas Tanure <[e-mail]> writes:
>>
>>> The default duty cycle of 33% is less than the required
>>> by the I2C specs for the LOW period of the SCL clock.
>>>
>>> So, for 100Khz or less, use 50%H/50%L duty cycle, and
>>> for the clock above 100Khz, use 40%H/60%L duty cycle.
>>> That ensures the low period of SCL is always more than
>>> the minimum required by the specs at any given frequency.
>>
>> Thanks for the fixes!
>>
>> This is going to affect all SoCs, so ould you please summarize how your
>> changes were tested, and on which SoCs & boards?
>>
>> Thanks,
>>
>> Kevin
> 
> Hi,
> 
> I only tested against the vim3 board, measured the bus with a Saleae
> logic pro 16.
> The measurements were with 100k, 400k, and a few in-between frequencies.

Thanks, it's a great addition to have !

> 
> Is that enough?

A test on GXL/GXM (VIM1 or VIM2) & GXBB (Odroid-C2) devices is lacking before we
can merge this.

If I find some time, I'll have a try, but everyone is welcome testing this serie
and report if it still works fine for them.

Vyacheslav, do you think you can test on your JetHub devices ? it would validate GXL & AXG.

Neil

> 
> Thanks
> Lucas

Viacheslav April 4, 2022, 6 p.m. UTC | #4

04.04.2022 11:01, Neil Armstrong wrote:
> Hi,
> 
> On 29/03/2022 00:31, Lucas Tanure wrote:
>> On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <[e-mail]> wrote:
>>>
>>> Hi Lucas,
>>>
>>> Lucas Tanure <[e-mail]> writes:
>>>
>>>> The default duty cycle of 33% is less than the required
>>>> by the I2C specs for the LOW period of the SCL clock.
>>>>
>>>> So, for 100Khz or less, use 50%H/50%L duty cycle, and
>>>> for the clock above 100Khz, use 40%H/60%L duty cycle.
>>>> That ensures the low period of SCL is always more than
>>>> the minimum required by the specs at any given frequency.
>>>
>>> Thanks for the fixes!
>>>
>>> This is going to affect all SoCs, so ould you please summarize how your
>>> changes were tested, and on which SoCs & boards?
>>>
>>> Thanks,
>>>
>>> Kevin
>>
>> Hi,
>>
>> I only tested against the vim3 board, measured the bus with a Saleae
>> logic pro 16.
>> The measurements were with 100k, 400k, and a few in-between frequencies.
> 
> Thanks, it's a great addition to have !
> 
>>
>> Is that enough?
> 
> A test on GXL/GXM (VIM1 or VIM2) & GXBB (Odroid-C2) devices is lacking 
> before we
> can merge this.
> 
> If I find some time, I'll have a try, but everyone is welcome testing 
> this serie
> and report if it still works fine for them.
> 
> Vyacheslav, do you think you can test on your JetHub devices ? it would 
> validate GXL & AXG.

It builds ok on 5.17. JetHub H1/D1 has only rtc clock (pcf8563) and 
1-wire controller (ds2482) on i2c bus. I did't see any difference with 
or without patches. all works at first look.

Vyacheslav

Neil Armstrong April 5, 2022, 3:11 p.m. UTC | #5

Hi,

On 28/03/2022 23:51, Lucas Tanure wrote:
> 
> 
> On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <[e-mail] <mailto:[e-mail]>> wrote:
> 
>     Hi Lucas,
> 
>     Lucas Tanure <[e-mail] <mailto:[e-mail]>> writes:
> 
>      > The default duty cycle of 33% is less than the required
>      > by the I2C specs for the LOW period of the SCL clock.
>      >
>      > So, for 100Khz or less, use 50%H/50%L duty cycle, and
>      > for the clock above 100Khz, use 40%H/60%L duty cycle.
>      > That ensures the low period of SCL is always more than
>      > the minimum required by the specs at any given frequency.
> 
>     Thanks for the fixes!
> 
>     This is going to affect all SoCs, so ould you please summarize how your
>     changes were tested, and on which SoCs & boards?
> 
>     Thanks,
> 
>     Kevin
> 
> 
> Hi,
> 
> I only tested against vim3 board, measured the bus with an saleae logic pro 16.
> The measurements were with 100k, 400k and a few in between frequencies.
> 
> Is that enough?

I did a few measures on the Libre Computer Le Potato S905X board:

i2c_AO:

Before the patchset, I got:
- 100KHz: 1.66uS HIGH, 6.75uS LOW, 20%/80%, Freq 118KHz /!\
- 400KHz: Unable to decode, clock line is invalid, Data line is also invalid

With the patchset
- 100KHz: 4.25uS HIGH, 6.58uS LOW, 40%/60%, Freq 92KHz
- 400KHz: 0.33uS HIGH, 3.00uS LOW, 10%/90%, Freq 300KHz

i2c_B:

Before the patchset, I got:
- 100KHz: 2.25uS HIGH, 5.41uS LOW, 29%/71%, Freq 130KHz /!\
- 400KHz: 0.42uS HIGH, 1.66uS LOW, 20%/80%, Freq 480KHz /!\

With the patchset
- 100KHz: 4.75uS HIGH, 5.42uS LOW, 46%/54%, Freq 98KHz
- 400KHz: 0.66uS HIGH, 2.00uS LOW, 24%/75%, Freq 375KHz


So this fixes the frequency, before they were invalid.
And it fixes 400KHz on i2c_AO...

I do not understand why behavior is different between i2c_AO & i2c_B, they
are feed with the same clock so it should be the same.

Did you check on both i2c interfaces ? can you share your results ?

Neil

> 
> Thanks
> Lucas
> 
> 
>

Lucas Tanure April 8, 2022, 7:19 a.m. UTC | #6

On Tue, Apr 5, 2022 at 4:11 PM Neil Armstrong <[e-mail]> wrote:
>
> Hi,
>
> On 28/03/2022 23:51, Lucas Tanure wrote:
> >
> >
> > On Mon, 28 Mar 2022, 21:37 Kevin Hilman, <[e-mail] <mailto:[e-mail]>> wrote:
> >
> >     Hi Lucas,
> >
> >     Lucas Tanure <[e-mail] <mailto:[e-mail]>> writes:
> >
> >      > The default duty cycle of 33% is less than the required
> >      > by the I2C specs for the LOW period of the SCL clock.
> >      >
> >      > So, for 100Khz or less, use 50%H/50%L duty cycle, and
> >      > for the clock above 100Khz, use 40%H/60%L duty cycle.
> >      > That ensures the low period of SCL is always more than
> >      > the minimum required by the specs at any given frequency.
> >
> >     Thanks for the fixes!
> >
> >     This is going to affect all SoCs, so ould you please summarize how your
> >     changes were tested, and on which SoCs & boards?
> >
> >     Thanks,
> >
> >     Kevin
> >
> >
> > Hi,
> >
> > I only tested against vim3 board, measured the bus with an saleae logic pro 16.
> > The measurements were with 100k, 400k and a few in between frequencies.
> >
> > Is that enough?
>
> I did a few measures on the Libre Computer Le Potato S905X board:
>
> i2c_AO:
>
> Before the patchset, I got:
> - 100KHz: 1.66uS HIGH, 6.75uS LOW, 20%/80%, Freq 118KHz /!\
> - 400KHz: Unable to decode, clock line is invalid, Data line is also invalid
>
> With the patchset
> - 100KHz: 4.25uS HIGH, 6.58uS LOW, 40%/60%, Freq 92KHz
> - 400KHz: 0.33uS HIGH, 3.00uS LOW, 10%/90%, Freq 300KHz
>
> i2c_B:
>
> Before the patchset, I got:
> - 100KHz: 2.25uS HIGH, 5.41uS LOW, 29%/71%, Freq 130KHz /!\
> - 400KHz: 0.42uS HIGH, 1.66uS LOW, 20%/80%, Freq 480KHz /!\
>
> With the patchset
> - 100KHz: 4.75uS HIGH, 5.42uS LOW, 46%/54%, Freq 98KHz
> - 400KHz: 0.66uS HIGH, 2.00uS LOW, 24%/75%, Freq 375KHz
>
>
> So this fixes the frequency, before they were invalid.
> And it fixes 400KHz on i2c_AO...
>
> I do not understand why behavior is different between i2c_AO & i2c_B, they
> are feed with the same clock so it should be the same.
>
> Did you check on both i2c interfaces ? can you share your results ?

I only checked I2C interfaces i2c3 and i2c_ao.
I will submit a new patch chain with more results.

>
> Neil
>
> >
> > Thanks
> > Lucas
> >
> >
> >
>