linux-kernel - Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

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] [day] [month] [year] [list]

Message-ID: <CACD3sJYU2gDbeGyhtqnu2Rjbfny9zE3Cfbhii34uKB=iG=EwuQ@mail.gmail.com>
Date: Tue, 5 Nov 2024 09:53:24 +0800
From: Tyrone Ting <warp5tw@...il.com>
To: Andi Shyti <andi.shyti@...nel.org>
Cc: avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com, 
	venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com, 
	andriy.shevchenko@...ux.intel.com, wsa@...nel.org, rand.sec96@...il.com, 
	wsa+renesas@...g-engineering.com, tali.perry@...oton.com, 
	Avi.Fishman@...oton.com, tomer.maimon@...oton.com, KWLIU@...oton.com, 
	JJLIU0@...oton.com, kfting@...oton.com, openbmc@...ts.ozlabs.org, 
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/4] i2c: npcm: use i2c frequency table

Hi Andi:

May I have your comments about my feedback on these patches?

Tyrone Ting <warp5tw@...il.com> 於 2024年10月25日 週五 上午9:46寫道：
>
> Hi Andi:
>
> Thank you for your comments.
>
> Andi Shyti <andi.shyti@...nel.org> 於 2024年10月24日 週四 下午6:20寫道：
> >
> > Hi Tyrone,
> >
> > ...
> >
> > > -     /* 100KHz and below: */
> > > -     if (bus_freq_hz <= I2C_MAX_STANDARD_MODE_FREQ) {
> > > -             sclfrq = src_clk_khz / (bus_freq_khz * 4);
> > > -
> > > -             if (sclfrq < SCLFRQ_MIN || sclfrq > SCLFRQ_MAX)
> > > -                     return -EDOM;
> > > -
> > > -             if (src_clk_khz >= 40000)
> > > -                     hldt = 17;
> > > -             else if (src_clk_khz >= 12500)
> > > -                     hldt = 15;
> > > -             else
> > > -                     hldt = 7;
> > > -     }
> > > -
> > > -     /* 400KHz: */
> > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_FREQ) {
> > > -             sclfrq = 0;
> > > +     switch (bus_freq_hz) {
> > > +     case I2C_MAX_STANDARD_MODE_FREQ:
> > > +             smb_timing = smb_timing_100khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_100khz);
> > > +             break;
> > > +     case I2C_MAX_FAST_MODE_FREQ:
> > > +             smb_timing = smb_timing_400khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_400khz);
> > >               fast_mode = I2CCTL3_400K_MODE;
> > > -
> > > -             if (src_clk_khz < 7500)
> > > -                     /* 400KHZ cannot be supported for core clock < 7.5MHz */
> > > -                     return -EDOM;
> > > -
> > > -             else if (src_clk_khz >= 50000) {
> > > -                     k1 = 80;
> > > -                     k2 = 48;
> > > -                     hldt = 12;
> > > -                     dbnct = 7;
> > > -             }
> > > -
> > > -             /* Master or Slave with frequency > 25MHz */
> > > -             else if (src_clk_khz > 25000) {
> > > -                     hldt = clk_coef(src_clk_khz, 300) + 7;
> > > -                     k1 = clk_coef(src_clk_khz, 1600);
> > > -                     k2 = clk_coef(src_clk_khz, 900);
> > > -             }
> > > -     }
> > > -
> > > -     /* 1MHz: */
> > > -     else if (bus_freq_hz <= I2C_MAX_FAST_MODE_PLUS_FREQ) {
> > > -             sclfrq = 0;
> > > +             break;
> > > +     case I2C_MAX_FAST_MODE_PLUS_FREQ:
> > > +             smb_timing = smb_timing_1000khz;
> > > +             table_size = ARRAY_SIZE(smb_timing_1000khz);
> > >               fast_mode = I2CCTL3_400K_MODE;
> > > -
> > > -             /* 1MHZ cannot be supported for core clock < 24 MHz */
> > > -             if (src_clk_khz < 24000)
> > > -                     return -EDOM;
> > > -
> > > -             k1 = clk_coef(src_clk_khz, 620);
> > > -             k2 = clk_coef(src_clk_khz, 380);
> > > -
> > > -             /* Core clk > 40 MHz */
> > > -             if (src_clk_khz > 40000) {
> > > -                     /*
> > > -                      * Set HLDT:
> > > -                      * SDA hold time:  (HLDT-7) * T(CLK) >= 120
> > > -                      * HLDT = 120/T(CLK) + 7 = 120 * FREQ(CLK) + 7
> > > -                      */
> > > -                     hldt = clk_coef(src_clk_khz, 120) + 7;
> > > -             } else {
> > > -                     hldt = 7;
> > > -                     dbnct = 2;
> > > -             }
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> >
> > There is here a slight change of behaiour which is not mentioned
> > in the commit log. Before the user could set a bus_freq_hz which
> > had to be <= I2C_MAX_..._MODE_FREQ, while now it has to be
> > precisely that.
> >
> > Do we want to check what the user has set in the DTS?
>
> The driver checks the bus frequency the user sets in the DTS.
>
> Please refer to the links:
> 1. https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L1995
> 2. https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/i2c-npcm7xx.c#L2002
>
> >
> > (Or am I missing something?)
> >
> > Thanks,
> > Andi
>
> Thank you.
>
> Regards,
> Tyrone

Thank you.

Regards,
Tyrone