Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
Sorry for late review. On Thu, 2020-05-14 at 21:09 +0800, Qii Wang wrote: > This patch adds a algorithm to calculate some ac-timing parameters > which can fully meet I2C Spec. > > Signed-off-by: Qii Wang > --- > drivers/i2c/busses/i2c-mt65xx.c | 328 > +--- > 1 file changed, 277 insertions(+), 51 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c > index 0ca6c38a..7020618 100644 > --- a/drivers/i2c/busses/i2c-mt65xx.c > +++ b/drivers/i2c/busses/i2c-mt65xx.c <...> > @@ -948,9 +1177,6 @@ static int mtk_i2c_probe(struct platform_device *pdev) > if (ret) > return -EINVAL; > > - if (i2c->dev_comp->timing_adjust) > - i2c->clk_src_div *= I2C_DEFAULT_CLK_DIV; > - After this patch, the 'clock-div' property in device tree is no longer used for platform with timing_adjust ability. Please change the binding, so we don't need to provide 'clock-div' for these platform. Joe.C > if (i2c->have_pmic && !i2c->dev_comp->pmic_i2c) > return -EINVAL; >
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
Hi Geert,
On Wed, 2020-05-20 at 10:58 +0200, Geert Uytterhoeven wrote:
> Hi Qii,
>
> On Wed, May 20, 2020 at 10:44 AM Qii Wang wrote:
> > On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> > > On Tue, May 19, 2020 at 4:59 AM Qii Wang wrote:
> > > > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > > > On Thu, May 14, 2020 at 3:13 PM Qii Wang
> > > > > wrote:
> > > > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > > > which can fully meet I2C Spec.
> > > > > >
> > > > > > Signed-off-by: Qii Wang
> > > > > > ---
> > > > > > drivers/i2c/busses/i2c-mt65xx.c | 328
> > > > > > +---
> > > > > > 1 file changed, 277 insertions(+), 51 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c
> > > > > > b/drivers/i2c/busses/i2c-mt65xx.c
> > > > > > index 0ca6c38a..7020618 100644
> > > > > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > > > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > > > >
> > > > > > +/*
> > > > > > + * Check and Calculate i2c ac-timing
> > > > > > + *
> > > > > > + * Hardware design:
> > > > > > + * sample_ns = (10 * (sample_cnt + 1)) / clk_src
> > > > > > + * xxx_cnt_div = spec->min_xxx_ns / sample_ns
> > > > > > + *
> > > > > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > > > > + * than the smallest spec.
> > > > > > + * The sda_timing is chosen as the middle value between
> > > > > > + * the largest and smallest.
> > > > > > + */
> > > > > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > > > > + unsigned int clk_src,
> > > > > > + unsigned int check_speed,
> > > > > > + unsigned int step_cnt,
> > > > > > + unsigned int sample_cnt)
> > > > > > +{
> > > > > > + const struct i2c_spec_values *spec;
> > > > > > + unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > > > > + unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > > > > + long long sample_ns = (10 * (sample_cnt + 1)) /
> > > > > > clk_src;
> > > > >
> > > > > So sample_ns is a 64-bit value. Is that really needed?
> > > > >
> > > >
> > > > (10 * (sample_cnt + 1)) / clk_src value is a 32-bit, (10
> > > > * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
> > >
> > > The intermediate value will indeed not fit in 32-bit.
> > > But that doesn't mean the end result won't fit in 32-bit.
> > > As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
> > > are small numbers) by sample_ns below, sample_ns cannot be very large,
> > > or the quotient will be zero anyway.
> > > So just doing the multiplication in 64-bit, followed by a 64-by-32
> > > division is probably fine:
> > >
> > > unsigned int sample_ns = div_u64(10ULL * (sample_cnt + 1),
> > > clk_src);
> > >
> > > You may want to take precautions for the case where the passed value of
> > > clk_src is a small number (can that happen?).
> > >
> > > BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
> > > takes an "unsigned int" parent_clk, which may cause future issues.
> > > You may want to change that to "unsigned long", along the whole
> > > propagation path, and use div64_ul() instead of div_u64() above.
> > >
> >
> > The return type of div_u64 is u64(unsigned long long), there is a
> > compulsory type conversion operator. Do you think it is needed?
>
> The result of a 64-by-32 bit division may indeed not fit in 32-bit, so that's
> why it returns u64.
> If you know the quotient will always fit, it's fine.
>
> > BTW, we just need to change the type of sample_ns to unsigned int, no
> > matter which method is used, what is your opinion?
>
> Indeed.
>
> BTW, I just realize
>
> long long sample_ns = (10 * (sample_cnt + 1)) / clk_src;
>
> wasn't doing what you wanted anyway, as 10 is (implicit) int,
> and sample_cnt is unsigned int, so the multiplication was done in 32-bit,
> possible leading to a truncation. Hence that division was done in 32-bit,
> too,
> that's why I didn't notice a call to __udivdi3() in the assembler output here.
>
> So you have to force the multiplication to be done in 64-bit, e.g.
> by changing the constant to 10ULL, and use div_u64() for
> the division.
>
ok, I will give a patch with your way, thanks for your opinion.
> >
> > > > I think 10 and clk_src is too big, maybe I can reduce then with
> > > > be divided all by 1000.
> > > > example:
> > > >
> > > > unsigned int sample_ns;
> > > > unsigned int clk_src_khz = clk_src / 1000;
> > >
> > > That may cause too much loss of precision.
> > >
> >
> > clk_src is more than MHz and less than GHZ for MTK i2c controller, so it
> > wouldn't cause too much loss of precision.
>
> OK, so that would work, too.
>
> > > >
> > > > if(clk_src_khz)
> > > >
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
Hi Qii,
On Wed, May 20, 2020 at 10:44 AM Qii Wang wrote:
> On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> > On Tue, May 19, 2020 at 4:59 AM Qii Wang wrote:
> > > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > > On Thu, May 14, 2020 at 3:13 PM Qii Wang wrote:
> > > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > > which can fully meet I2C Spec.
> > > > >
> > > > > Signed-off-by: Qii Wang
> > > > > ---
> > > > > drivers/i2c/busses/i2c-mt65xx.c | 328
> > > > > +---
> > > > > 1 file changed, 277 insertions(+), 51 deletions(-)
> > > > >
> > > > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c
> > > > > b/drivers/i2c/busses/i2c-mt65xx.c
> > > > > index 0ca6c38a..7020618 100644
> > > > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > > >
> > > > > +/*
> > > > > + * Check and Calculate i2c ac-timing
> > > > > + *
> > > > > + * Hardware design:
> > > > > + * sample_ns = (10 * (sample_cnt + 1)) / clk_src
> > > > > + * xxx_cnt_div = spec->min_xxx_ns / sample_ns
> > > > > + *
> > > > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > > > + * than the smallest spec.
> > > > > + * The sda_timing is chosen as the middle value between
> > > > > + * the largest and smallest.
> > > > > + */
> > > > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > > > + unsigned int clk_src,
> > > > > + unsigned int check_speed,
> > > > > + unsigned int step_cnt,
> > > > > + unsigned int sample_cnt)
> > > > > +{
> > > > > + const struct i2c_spec_values *spec;
> > > > > + unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > > > + unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > > > + long long sample_ns = (10 * (sample_cnt + 1)) /
> > > > > clk_src;
> > > >
> > > > So sample_ns is a 64-bit value. Is that really needed?
> > > >
> > >
> > > (10 * (sample_cnt + 1)) / clk_src value is a 32-bit, (10
> > > * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
> >
> > The intermediate value will indeed not fit in 32-bit.
> > But that doesn't mean the end result won't fit in 32-bit.
> > As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
> > are small numbers) by sample_ns below, sample_ns cannot be very large,
> > or the quotient will be zero anyway.
> > So just doing the multiplication in 64-bit, followed by a 64-by-32
> > division is probably fine:
> >
> > unsigned int sample_ns = div_u64(10ULL * (sample_cnt + 1),
> > clk_src);
> >
> > You may want to take precautions for the case where the passed value of
> > clk_src is a small number (can that happen?).
> >
> > BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
> > takes an "unsigned int" parent_clk, which may cause future issues.
> > You may want to change that to "unsigned long", along the whole
> > propagation path, and use div64_ul() instead of div_u64() above.
> >
>
> The return type of div_u64 is u64(unsigned long long), there is a
> compulsory type conversion operator. Do you think it is needed?
The result of a 64-by-32 bit division may indeed not fit in 32-bit, so that's
why it returns u64.
If you know the quotient will always fit, it's fine.
> BTW, we just need to change the type of sample_ns to unsigned int, no
> matter which method is used, what is your opinion?
Indeed.
BTW, I just realize
long long sample_ns = (10 * (sample_cnt + 1)) / clk_src;
wasn't doing what you wanted anyway, as 10 is (implicit) int,
and sample_cnt is unsigned int, so the multiplication was done in 32-bit,
possible leading to a truncation. Hence that division was done in 32-bit, too,
that's why I didn't notice a call to __udivdi3() in the assembler output here.
So you have to force the multiplication to be done in 64-bit, e.g.
by changing the constant to 10ULL, and use div_u64() for
the division.
>
> > > I think 10 and clk_src is too big, maybe I can reduce then with
> > > be divided all by 1000.
> > > example:
> > >
> > > unsigned int sample_ns;
> > > unsigned int clk_src_khz = clk_src / 1000;
> >
> > That may cause too much loss of precision.
> >
>
> clk_src is more than MHz and less than GHZ for MTK i2c controller, so it
> wouldn't cause too much loss of precision.
OK, so that would work, too.
> > >
> > > if(clk_src_khz)
> > > sample_ns = (100 * (sample_cnt + 1)) / clk_src_khz;
> > > else
> > > return -EINVAL;
> > >
> > > > > + if (!i2c->dev_comp->timing_adjust)
> > > > > + return 0;
> > > > > +
> > > > > + if (i2c->dev_comp->ltiming_adjust)
> > > > > + max_sta_cnt = 0x100;
> > > > > +
> > > > > + spec = mtk_i2c_get_spec(check_speed);
> > > > > +
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
Hi Geert,
On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> Hi Qii,
>
> On Tue, May 19, 2020 at 4:59 AM Qii Wang wrote:
> > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > On Thu, May 14, 2020 at 3:13 PM Qii Wang wrote:
> > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > which can fully meet I2C Spec.
> > > >
> > > > Signed-off-by: Qii Wang
> > > > ---
> > > > drivers/i2c/busses/i2c-mt65xx.c | 328
> > > > +---
> > > > 1 file changed, 277 insertions(+), 51 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c
> > > > b/drivers/i2c/busses/i2c-mt65xx.c
> > > > index 0ca6c38a..7020618 100644
> > > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > >
> > > > +/*
> > > > + * Check and Calculate i2c ac-timing
> > > > + *
> > > > + * Hardware design:
> > > > + * sample_ns = (10 * (sample_cnt + 1)) / clk_src
> > > > + * xxx_cnt_div = spec->min_xxx_ns / sample_ns
> > > > + *
> > > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > > + * than the smallest spec.
> > > > + * The sda_timing is chosen as the middle value between
> > > > + * the largest and smallest.
> > > > + */
> > > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > > + unsigned int clk_src,
> > > > + unsigned int check_speed,
> > > > + unsigned int step_cnt,
> > > > + unsigned int sample_cnt)
> > > > +{
> > > > + const struct i2c_spec_values *spec;
> > > > + unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > > + unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > > + long long sample_ns = (10 * (sample_cnt + 1)) / clk_src;
> > >
> > > So sample_ns is a 64-bit value. Is that really needed?
> > >
> >
> > (10 * (sample_cnt + 1)) / clk_src value is a 32-bit, (10
> > * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
>
> The intermediate value will indeed not fit in 32-bit.
> But that doesn't mean the end result won't fit in 32-bit.
> As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
> are small numbers) by sample_ns below, sample_ns cannot be very large,
> or the quotient will be zero anyway.
> So just doing the multiplication in 64-bit, followed by a 64-by-32
> division is probably fine:
>
> unsigned int sample_ns = div_u64(10ULL * (sample_cnt + 1),
> clk_src);
>
> You may want to take precautions for the case where the passed value of
> clk_src is a small number (can that happen?).
>
> BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
> takes an "unsigned int" parent_clk, which may cause future issues.
> You may want to change that to "unsigned long", along the whole
> propagation path, and use div64_ul() instead of div_u64() above.
>
The return type of div_u64 is u64(unsigned long long), there is a
compulsory type conversion operator. Do you think it is needed?
BTW, we just need to change the type of sample_ns to unsigned int, no
matter which method is used, what is your opinion?
> > I think 10 and clk_src is too big, maybe I can reduce then with
> > be divided all by 1000.
> > example:
> >
> > unsigned int sample_ns;
> > unsigned int clk_src_khz = clk_src / 1000;
>
> That may cause too much loss of precision.
>
clk_src is more than MHz and less than GHZ for MTK i2c controller, so it
wouldn't cause too much loss of precision.
> >
> > if(clk_src_khz)
> > sample_ns = (100 * (sample_cnt + 1)) / clk_src_khz;
> > else
> > return -EINVAL;
> >
> > > > + if (!i2c->dev_comp->timing_adjust)
> > > > + return 0;
> > > > +
> > > > + if (i2c->dev_comp->ltiming_adjust)
> > > > + max_sta_cnt = 0x100;
> > > > +
> > > > + spec = mtk_i2c_get_spec(check_speed);
> > > > +
> > > > + if (i2c->dev_comp->ltiming_adjust)
> > > > + clk_ns = 10 / clk_src;
> > > > + else
> > > > + clk_ns = sample_ns / 2;
> > > > +
> > > > + su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > > > + if (su_sta_cnt > max_sta_cnt)
> > > > + return -1;
> > > > +
> > > > + low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
> > >
> > > So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)
>
> Gr{oetje,eeting}s,
>
> Geert
>
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
On Tue, 19 May 2020 10:57:53 +0800, Qii Wang said: > (10 * (sample_cnt + 1)) / clk_src value is a 32-bit, (10 > * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7. > > I think 10 and clk_src is too big, maybe I can reduce then with > be divided all by 1000. Yes, it's definitely too big, the 3 DIV_ROUND_UP calls in mtk_i2c_check_ac_timing() end up causing a build issue during modpost on a 32-bit RPi4: ERROR: modpost: "__aeabi_uldivmod" [drivers/i2c/busses/i2c-mt65xx.ko] undefined! ERROR: modpost: "__aeabi_ldivmod" [drivers/i2c/busses/i2c-mt65xx.ko] undefined! pgpBCSBQ46GbV.pgp Description: PGP signature
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
Hi Qii,
On Tue, May 19, 2020 at 4:59 AM Qii Wang wrote:
> On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > On Thu, May 14, 2020 at 3:13 PM Qii Wang wrote:
> > > This patch adds a algorithm to calculate some ac-timing parameters
> > > which can fully meet I2C Spec.
> > >
> > > Signed-off-by: Qii Wang
> > > ---
> > > drivers/i2c/busses/i2c-mt65xx.c | 328
> > > +---
> > > 1 file changed, 277 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c
> > > b/drivers/i2c/busses/i2c-mt65xx.c
> > > index 0ca6c38a..7020618 100644
> > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> >
> > > +/*
> > > + * Check and Calculate i2c ac-timing
> > > + *
> > > + * Hardware design:
> > > + * sample_ns = (10 * (sample_cnt + 1)) / clk_src
> > > + * xxx_cnt_div = spec->min_xxx_ns / sample_ns
> > > + *
> > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > + * than the smallest spec.
> > > + * The sda_timing is chosen as the middle value between
> > > + * the largest and smallest.
> > > + */
> > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > + unsigned int clk_src,
> > > + unsigned int check_speed,
> > > + unsigned int step_cnt,
> > > + unsigned int sample_cnt)
> > > +{
> > > + const struct i2c_spec_values *spec;
> > > + unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > + unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > + long long sample_ns = (10 * (sample_cnt + 1)) / clk_src;
> >
> > So sample_ns is a 64-bit value. Is that really needed?
> >
>
> (10 * (sample_cnt + 1)) / clk_src value is a 32-bit, (10
> * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
The intermediate value will indeed not fit in 32-bit.
But that doesn't mean the end result won't fit in 32-bit.
As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
are small numbers) by sample_ns below, sample_ns cannot be very large,
or the quotient will be zero anyway.
So just doing the multiplication in 64-bit, followed by a 64-by-32
division is probably fine:
unsigned int sample_ns = div_u64(10ULL * (sample_cnt + 1), clk_src);
You may want to take precautions for the case where the passed value of
clk_src is a small number (can that happen?).
BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
takes an "unsigned int" parent_clk, which may cause future issues.
You may want to change that to "unsigned long", along the whole
propagation path, and use div64_ul() instead of div_u64() above.
> I think 10 and clk_src is too big, maybe I can reduce then with
> be divided all by 1000.
> example:
>
> unsigned int sample_ns;
> unsigned int clk_src_khz = clk_src / 1000;
That may cause too much loss of precision.
>
> if(clk_src_khz)
> sample_ns = (100 * (sample_cnt + 1)) / clk_src_khz;
> else
> return -EINVAL;
>
> > > + if (!i2c->dev_comp->timing_adjust)
> > > + return 0;
> > > +
> > > + if (i2c->dev_comp->ltiming_adjust)
> > > + max_sta_cnt = 0x100;
> > > +
> > > + spec = mtk_i2c_get_spec(check_speed);
> > > +
> > > + if (i2c->dev_comp->ltiming_adjust)
> > > + clk_ns = 10 / clk_src;
> > > + else
> > > + clk_ns = sample_ns / 2;
> > > +
> > > + su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > > + if (su_sta_cnt > max_sta_cnt)
> > > + return -1;
> > > +
> > > + low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
> >
> > So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> On Thu, May 14, 2020 at 3:13 PM Qii Wang wrote:
> > This patch adds a algorithm to calculate some ac-timing parameters
> > which can fully meet I2C Spec.
> >
> > Signed-off-by: Qii Wang
> > ---
> > drivers/i2c/busses/i2c-mt65xx.c | 328
> > +---
> > 1 file changed, 277 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c
> > b/drivers/i2c/busses/i2c-mt65xx.c
> > index 0ca6c38a..7020618 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
>
> > +/*
> > + * Check and Calculate i2c ac-timing
> > + *
> > + * Hardware design:
> > + * sample_ns = (10 * (sample_cnt + 1)) / clk_src
> > + * xxx_cnt_div = spec->min_xxx_ns / sample_ns
> > + *
> > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > + * than the smallest spec.
> > + * The sda_timing is chosen as the middle value between
> > + * the largest and smallest.
> > + */
> > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > + unsigned int clk_src,
> > + unsigned int check_speed,
> > + unsigned int step_cnt,
> > + unsigned int sample_cnt)
> > +{
> > + const struct i2c_spec_values *spec;
> > + unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > + unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > + long long sample_ns = (10 * (sample_cnt + 1)) / clk_src;
>
> So sample_ns is a 64-bit value. Is that really needed?
>
(10 * (sample_cnt + 1)) / clk_src value is a 32-bit, (10
* (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
I think 10 and clk_src is too big, maybe I can reduce then with
be divided all by 1000.
example:
unsigned int sample_ns;
unsigned int clk_src_khz = clk_src / 1000;
if(clk_src_khz)
sample_ns = (100 * (sample_cnt + 1)) / clk_src_khz;
else
return -EINVAL;
> > + if (!i2c->dev_comp->timing_adjust)
> > + return 0;
> > +
> > + if (i2c->dev_comp->ltiming_adjust)
> > + max_sta_cnt = 0x100;
> > +
> > + spec = mtk_i2c_get_spec(check_speed);
> > +
> > + if (i2c->dev_comp->ltiming_adjust)
> > + clk_ns = 10 / clk_src;
> > + else
> > + clk_ns = sample_ns / 2;
> > +
> > + su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > + if (su_sta_cnt > max_sta_cnt)
> > + return -1;
> > +
> > + low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
>
> So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)
>
> nore...@ellerman.id.au reports:
>
> ERROR: modpost: "__udivdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
> ERROR: modpost: "__divdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
>
> for 32-bit builds.
>
> > + max_step_cnt = mtk_i2c_max_step_cnt(check_speed);
> > + if ((2 * step_cnt) > low_cnt && low_cnt < max_step_cnt) {
> > + if (low_cnt > step_cnt) {
> > + high_cnt = 2 * step_cnt - low_cnt;
> > + } else {
> > + high_cnt = step_cnt;
> > + low_cnt = step_cnt;
> > + }
> > + } else {
> > + return -2;
> > + }
> > +
> > + sda_max = spec->max_hd_dat_ns / sample_ns;
> > + if (sda_max > low_cnt)
> > + sda_max = 0;
> > +
> > + sda_min = DIV_ROUND_UP(spec->min_su_dat_ns, sample_ns);
>
> One more.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> ge...@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like
> that.
> -- Linus Torvalds
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
On Thu, May 14, 2020 at 3:13 PM Qii Wang wrote:
> This patch adds a algorithm to calculate some ac-timing parameters
> which can fully meet I2C Spec.
>
> Signed-off-by: Qii Wang
> ---
> drivers/i2c/busses/i2c-mt65xx.c | 328
> +---
> 1 file changed, 277 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 0ca6c38a..7020618 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c
> +/*
> + * Check and Calculate i2c ac-timing
> + *
> + * Hardware design:
> + * sample_ns = (10 * (sample_cnt + 1)) / clk_src
> + * xxx_cnt_div = spec->min_xxx_ns / sample_ns
> + *
> + * Sample_ns is rounded down for xxx_cnt_div would be greater
> + * than the smallest spec.
> + * The sda_timing is chosen as the middle value between
> + * the largest and smallest.
> + */
> +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> + unsigned int clk_src,
> + unsigned int check_speed,
> + unsigned int step_cnt,
> + unsigned int sample_cnt)
> +{
> + const struct i2c_spec_values *spec;
> + unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> + unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> + long long sample_ns = (10 * (sample_cnt + 1)) / clk_src;
So sample_ns is a 64-bit value. Is that really needed?
> + if (!i2c->dev_comp->timing_adjust)
> + return 0;
> +
> + if (i2c->dev_comp->ltiming_adjust)
> + max_sta_cnt = 0x100;
> +
> + spec = mtk_i2c_get_spec(check_speed);
> +
> + if (i2c->dev_comp->ltiming_adjust)
> + clk_ns = 10 / clk_src;
> + else
> + clk_ns = sample_ns / 2;
> +
> + su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> + if (su_sta_cnt > max_sta_cnt)
> + return -1;
> +
> + low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)
nore...@ellerman.id.au reports:
ERROR: modpost: "__udivdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
ERROR: modpost: "__divdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
for 32-bit builds.
> + max_step_cnt = mtk_i2c_max_step_cnt(check_speed);
> + if ((2 * step_cnt) > low_cnt && low_cnt < max_step_cnt) {
> + if (low_cnt > step_cnt) {
> + high_cnt = 2 * step_cnt - low_cnt;
> + } else {
> + high_cnt = step_cnt;
> + low_cnt = step_cnt;
> + }
> + } else {
> + return -2;
> + }
> +
> + sda_max = spec->max_hd_dat_ns / sample_ns;
> + if (sda_max > low_cnt)
> + sda_max = 0;
> +
> + sda_min = DIV_ROUND_UP(spec->min_su_dat_ns, sample_ns);
One more.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support
On Thu, May 14, 2020 at 09:09:05PM +0800, Qii Wang wrote: > This patch adds a algorithm to calculate some ac-timing parameters > which can fully meet I2C Spec. > > Signed-off-by: Qii Wang Applied to for-next, thanks! signature.asc Description: PGP signature
