Stephen/Laxman, On Tue, Feb 12, 2013 at 12:07 PM, Tom Warren <twarren.nvi...@gmail.com> wrote: > Stephen, > > On Tue, Feb 12, 2013 at 11:10 AM, Stephen Warren <swar...@wwwdotorg.org> > wrote: >> On 02/12/2013 10:40 AM, Tom Warren wrote: >>> Laxman, >>> >>> On Tue, Feb 12, 2013 at 5:02 AM, Laxman Dewangan <ldewan...@nvidia.com> >>> wrote: >>>> On Friday 08 February 2013 11:34 PM, Stephen Warren wrote: >>>>> >>>>> On 02/08/2013 10:25 AM, Tom Warren wrote: >>>>>> >>>>>> T114, like T30, does not have a separate/different DVC (power I2C) >>>>>> controller like T20 - all 5 I2C controllers are identical, but >>>>>> I2C5 is used to designate the controller intended for power >>>>>> control (PWR_I2C in the schematics). PWR_I2C is set to 400KHz. >>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>>>>> b/board/nvidia/dts/tegra114-dalmore.dts >>>>>> + aliases { >>>>>> + }; >>>>>> + >>>>> >>>>> There's no point adding an empty aliases node here. Feel free to fix >>>>> that up when you apply it rather than reposting if you want. >>>>> >>>>> I'd like too see Laxman sign-off on the "*2" question he had earlier >>>>> before actually checking this in. >>>>> >>>> We do not require *2 as the i2c clock divider is DIVU16 type. There was bug >>>> in early code on kernel also which we fixed in dowstream long back. >>>> Possibly >>>> uboot have not fixed this yet. >>> >>> Yeah, the Tegra I2C driver in U-Boot has always doubled the rate >>> before calling the clock set routine. >> >> Perhaps it's related to some dividers being U7.1 (i.e. the LSB of the >> divider represents 1/2 not 1)? However, as Laxman points out, this isn't >> the case for I2C clocks; they have a U16 divider, so the LSB represents >> 1 not 1/2. >> >>> But the important thing is that >>> the actual I2C clock is 100KHz (or 400KHz for I2C5) as measured at the >>> test points on the board. >>> >>> I'll look into the Tegra U-Boot clock routine idiosyncrasies later >>> when I get some more bandwidth. >> >> Just a few minutes of investigation points at clk_get_divider() being buggy. >> >> It assumes that all dividers have a built-in *2 clock multiplier on the >> front of them: >> >> u64 divider = parent_rate * 2; >> >> (the name for that variable is wrong; it should be something more like >> parent_rate or divider_input_rate) >> >> This (presence of a *2 pre-multiplier) is true for U7.1 dividers, since >> this is required for the LSB of the divider to represent 1/2 not 1. >> Hence, I assume that e.g. the SPI driver doesn't do this "* 2" on the >> clock rate; it actually has a U7.1 divider. >> >> However, this is not true for U16 dividers, since the HW doesn't need to >> multiply up the rate before dividing, since the LSB is 1 not 1/2. >> >> The solution here is to fix clk_get_divider() to return both a field >> width and a flag (or width) indicating whether a fractional part of the >> divider is present, and then pass that on to adjust_periph_pll() so that >> it can only optionally perform this initial "* 2". >> >> So at least that explains it. > Yeah, I just started looking at it before lunch, and saw the same thing. > > I'll see about fixing it now so I can put the T114 I2C patches to bed. > > Thanks >> >> I would strongly recommend just fixing this now. However, if you don't >> please do file a bug so that we don't forget about this.
I tried a quick fix for the 16-bit clock divider (only I2C HW on current Tegra HW uses it), but it didn't pass internal review. I don' t have time to rewrite this before I leave for a few days, so I'll look at it when I get back, if someone else hasn't already fixed it by then. But the current T114 I2C patches, which use a WAR of asking for a 2X rate, work OK for now (I2C1 clock, 100KHz expected rate, measured as 97-103KHz, so occasionally out-of-spec. Note that T20 has 'always' had this rate doubling request. As an aside, the new/extra clock divisor in T114 I2C HW (CLK_DIVISOR_STD_FAST_MODE) needs to change dynamically to come up with a better/closer actual rate - it's fixed at 0x19 (25) right now, and makes getting an exact clock_source divisor difficult. It'd be better (for 100KHz) if it were 0x32 (50), which would give a divisor of 9, and 408MHz/8/50+1/9+1 = 100KHz exactly. I'll look into changing it for each target I2C bus freq, too, when I get back. I'll file an internal bug so we can track the 16-bit divisor problem w/Tegra U-Boot. Tom _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot