| Message ID | 1443365828-8956-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey Hans, On 27-09-15 16:57, Hans de Goede wrote: > According to the datasheets to n factor for dividing the tclk is > 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is > on other mv64xxx implementations. Ah! > > I've contacted Allwinner about this and they have confirmed that the > datasheet is correct. > > This commit fixes the clk-divider calculations for Allwinner SoCs > accordingly. So this explains why all my i2c frequenties are double of what I setup. Thanks for taking the time of figuring it out! I'll give it a test hopefully soon. Olliver > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++++++++++--------- > 1 file changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > index 30059c1..e75cf6d 100644 > --- a/drivers/i2c/busses/i2c-mv64xxx.c > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > @@ -146,6 +146,8 @@ struct mv64xxx_i2c_data { > bool errata_delay; > struct reset_control *rstc; > bool irq_clear_inverted; > + /* Clk div is 2 to the power n, not 2 to the power n + 1 */ > + bool clk_n_base_0; > }; > > static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { > @@ -759,25 +761,29 @@ MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table); > #ifdef CONFIG_OF > #ifdef CONFIG_HAVE_CLK > static int > -mv64xxx_calc_freq(const int tclk, const int n, const int m) > +mv64xxx_calc_freq(struct mv64xxx_i2c_data *drv_data, > + const int tclk, const int n, const int m) > { > - return tclk / (10 * (m + 1) * (2 << n)); > + if (drv_data->clk_n_base_0) > + return tclk / (10 * (m + 1) * (1 << n)); > + else > + return tclk / (10 * (m + 1) * (2 << n)); > } > > static bool > -mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n, > - u32 *best_m) > +mv64xxx_find_baud_factors(struct mv64xxx_i2c_data *drv_data, > + const u32 req_freq, const u32 tclk) > { > int freq, delta, best_delta = INT_MAX; > int m, n; > > for (n = 0; n <= 7; n++) > for (m = 0; m <= 15; m++) { > - freq = mv64xxx_calc_freq(tclk, n, m); > + freq = mv64xxx_calc_freq(drv_data, tclk, n, m); > delta = req_freq - freq; > if (delta >= 0 && delta < best_delta) { > - *best_m = m; > - *best_n = n; > + drv_data->freq_m = m; > + drv_data->freq_n = n; > best_delta = delta; > } > if (best_delta == 0) > @@ -815,8 +821,11 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, > if (of_property_read_u32(np, "clock-frequency", &bus_freq)) > bus_freq = 100000; /* 100kHz by default */ > > - if (!mv64xxx_find_baud_factors(bus_freq, tclk, > - &drv_data->freq_n, &drv_data->freq_m)) { > + if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") || > + of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) > + drv_data->clk_n_base_0 = true; > + > + if (!mv64xxx_find_baud_factors(drv_data, bus_freq, tclk)) { > rc = -EINVAL; > goto out; > }
On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote: > Hey Hans, > > On 27-09-15 16:57, Hans de Goede wrote: > >According to the datasheets to n factor for dividing the tclk is > >2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is > >on other mv64xxx implementations. > Ah! Just to be sure, i checked Kirkwood, Armada XP and Armada 370 datasheets. They all say n+1. > >+ if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") || > >+ of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) Rather than have to extend this list every so often, how about adding a helper of_device_is_compatible_vendor(), so you can just have: > >+ if (of_device_is_compatible_vendor(np, "allwinner") Andrew
>>>>> "Hans" == Hans de Goede <hdegoede@redhat.com> writes: > According to the datasheets to n factor for dividing the tclk is s/to/the/
Hi, On 27-09-15 18:53, Andrew Lunn wrote: > On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote: >> Hey Hans, >> >> On 27-09-15 16:57, Hans de Goede wrote: >>> According to the datasheets to n factor for dividing the tclk is >>> 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is >>> on other mv64xxx implementations. >> Ah! > > Just to be sure, i checked Kirkwood, Armada XP and Armada 370 > datasheets. They all say n+1. Thanks. >>> + if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") || >>> + of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) > > Rather than have to extend this list every so often, how about adding > a helper of_device_is_compatible_vendor(), so you can just have: > >>> + if (of_device_is_compatible_vendor(np, "allwinner") I agree that if such a helper would already exist it would be a good idea to use it, but it seems overkill to just at it for this. Regards, Hans
On Sun, Sep 27, 2015 at 04:57:08PM +0200, Hans de Goede wrote: > According to the datasheets to n factor for dividing the tclk is > 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is > on other mv64xxx implementations. > > I've contacted Allwinner about this and they have confirmed that the > datasheet is correct. > > This commit fixes the clk-divider calculations for Allwinner SoCs > accordingly. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Thanks! Maxime
On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote: > Hey Hans, > > On 27-09-15 16:57, Hans de Goede wrote: > >According to the datasheets to n factor for dividing the tclk is > >2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is > >on other mv64xxx implementations. > Ah! > > > >I've contacted Allwinner about this and they have confirmed that the > >datasheet is correct. > > > >This commit fixes the clk-divider calculations for Allwinner SoCs > >accordingly. > So this explains why all my i2c frequenties are double of what I setup. > Thanks for taking the time of figuring it out! I'll give it a test hopefully > soon. It would have been great to let us know... Maxime
On Sun, Sep 27, 2015 at 06:53:03PM +0200, Andrew Lunn wrote: > > >+ if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") || > > >+ of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) > > Rather than have to extend this list every so often, how about adding > a helper of_device_is_compatible_vendor(), so you can just have: I don't know, I kind of like the fact that it's explicit. If we ever have another SoC coming in with a different behaviour, we won't have to expand it back. Maxime
Hey Maxime, On 29-09-15 14:09, Maxime Ripard wrote: > On Sun, Sep 27, 2015 at 06:05:35PM +0200, Olliver Schinagl wrote: >> Hey Hans, >> >> On 27-09-15 16:57, Hans de Goede wrote: >>> According to the datasheets to n factor for dividing the tclk is >>> 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is >>> on other mv64xxx implementations. >> Ah! >>> I've contacted Allwinner about this and they have confirmed that the >>> datasheet is correct. >>> >>> This commit fixes the clk-divider calculations for Allwinner SoCs >>> accordingly. >> So this explains why all my i2c frequenties are double of what I setup. >> Thanks for taking the time of figuring it out! I'll give it a test hopefully >> soon. > It would have been great to let us know... If your talking about past tence, I actually did ;) [0][1] and I actually had a tree made ready just 3 weeks ago with 3.15 to start my bisection with. It seemed logical to see if it worked there as that was the first iteration (based on the sunxi/allwinner based tree before the mv migration. Anyway, I even built tried to build the kernel! but my gcc failed to build it so i put it on the back-burner for a while. WIth Hans having figured it out and fixing it, I'll absolutly will take a nother look and check with a scope if it all works out now. Olliver [0] http://irclog.whitequark.org/linux-sunxi/2015-01-16#11522114; [1] http://s24.postimg.org/yiykh6kkl/DS1_Z_Quick_Print2.png > Maxime >
> WIth Hans having figured it out and fixing it, I'll absolutly will take a > nother look and check with a scope if it all works out now. Have you done this already? /me is always looking for Tested-by: tags :)
I shamefully admit I have not. My plate is very full at the moment, but I will make room for this upcoming weekend as I have a few other patches to test aswell. Sorry for the delay! On 20-10-15 18:58, Wolfram Sang wrote: >> WIth Hans having figured it out and fixing it, I'll absolutly will take a >> nother look and check with a scope if it all works out now. > Have you done this already? /me is always looking for Tested-by: tags :) >
Hey Wolfram, On 20-10-15 17:58, Wolfram Sang wrote: >> WIth Hans having figured it out and fixing it, I'll absolutly will take a >> nother look and check with a scope if it all works out now. > Have you done this already? /me is always looking for Tested-by: tags :) And here you have it. Tested-by: Olliver Schinagl <oliver@schinagl.nl> I attached the scope traces for a 200.000 Hz i2c0 and a 400.000 Hz i2c1 (without anything connected to the bus) on a cubieboard2. Olliver
On Sun, Sep 27, 2015 at 04:57:08PM +0200, Hans de Goede wrote: > According to the datasheets to n factor for dividing the tclk is > 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is > on other mv64xxx implementations. > > I've contacted Allwinner about this and they have confirmed that the > datasheet is correct. > > This commit fixes the clk-divider calculations for Allwinner SoCs > accordingly. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Applied to for-current, thanks!
On Mon, Nov 30, 2015 at 02:51:41PM +0100, Wolfram Sang wrote: > On Sun, Sep 27, 2015 at 04:57:08PM +0200, Hans de Goede wrote: > > According to the datasheets to n factor for dividing the tclk is > > 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is > > on other mv64xxx implementations. > > > > I've contacted Allwinner about this and they have confirmed that the > > datasheet is correct. > > > > This commit fixes the clk-divider calculations for Allwinner SoCs > > accordingly. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Applied to for-current, thanks! And added stable...
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index 30059c1..e75cf6d 100644 --- a/drivers/i2c/busses/i2c-mv64xxx.c +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -146,6 +146,8 @@ struct mv64xxx_i2c_data { bool errata_delay; struct reset_control *rstc; bool irq_clear_inverted; + /* Clk div is 2 to the power n, not 2 to the power n + 1 */ + bool clk_n_base_0; }; static struct mv64xxx_i2c_regs mv64xxx_i2c_regs_mv64xxx = { @@ -759,25 +761,29 @@ MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table); #ifdef CONFIG_OF #ifdef CONFIG_HAVE_CLK static int -mv64xxx_calc_freq(const int tclk, const int n, const int m) +mv64xxx_calc_freq(struct mv64xxx_i2c_data *drv_data, + const int tclk, const int n, const int m) { - return tclk / (10 * (m + 1) * (2 << n)); + if (drv_data->clk_n_base_0) + return tclk / (10 * (m + 1) * (1 << n)); + else + return tclk / (10 * (m + 1) * (2 << n)); } static bool -mv64xxx_find_baud_factors(const u32 req_freq, const u32 tclk, u32 *best_n, - u32 *best_m) +mv64xxx_find_baud_factors(struct mv64xxx_i2c_data *drv_data, + const u32 req_freq, const u32 tclk) { int freq, delta, best_delta = INT_MAX; int m, n; for (n = 0; n <= 7; n++) for (m = 0; m <= 15; m++) { - freq = mv64xxx_calc_freq(tclk, n, m); + freq = mv64xxx_calc_freq(drv_data, tclk, n, m); delta = req_freq - freq; if (delta >= 0 && delta < best_delta) { - *best_m = m; - *best_n = n; + drv_data->freq_m = m; + drv_data->freq_n = n; best_delta = delta; } if (best_delta == 0) @@ -815,8 +821,11 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, if (of_property_read_u32(np, "clock-frequency", &bus_freq)) bus_freq = 100000; /* 100kHz by default */ - if (!mv64xxx_find_baud_factors(bus_freq, tclk, - &drv_data->freq_n, &drv_data->freq_m)) { + if (of_device_is_compatible(np, "allwinner,sun4i-a10-i2c") || + of_device_is_compatible(np, "allwinner,sun6i-a31-i2c")) + drv_data->clk_n_base_0 = true; + + if (!mv64xxx_find_baud_factors(drv_data, bus_freq, tclk)) { rc = -EINVAL; goto out; }
According to the datasheets to n factor for dividing the tclk is 2 to the power n on Allwinner SoCs, not 2 to the power n + 1 as it is on other mv64xxx implementations. I've contacted Allwinner about this and they have confirmed that the datasheet is correct. This commit fixes the clk-divider calculations for Allwinner SoCs accordingly. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-mv64xxx.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)