| Message ID | 1360344332-14664-3-git-send-email-twarren@nvidia.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Warren |
| Headers | show |
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. Aside from that, the series, Reviewed-by: Stephen Warren <swarren@nvidia.com>
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. ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
Laxman, On Tue, Feb 12, 2013 at 5:02 AM, Laxman Dewangan <ldewangan@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. 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. Thanks, Tom > > ----------------------------------------------------------------------------------- > This email message is for the sole use of the intended recipient(s) and may > contain > confidential information. Any unauthorized review, use, disclosure or > distribution > is prohibited. If you are not the intended recipient, please contact the > sender by > reply email and destroy all copies of the original message. > -----------------------------------------------------------------------------------
On 02/12/2013 10:40 AM, Tom Warren wrote: > Laxman, > > On Tue, Feb 12, 2013 at 5:02 AM, Laxman Dewangan <ldewangan@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. 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.
Stephen, On Tue, Feb 12, 2013 at 11:10 AM, Stephen Warren <swarren@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 <ldewangan@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.
Stephen/Laxman, On Tue, Feb 12, 2013 at 12:07 PM, Tom Warren <twarren.nvidia@gmail.com> wrote: > Stephen, > > On Tue, Feb 12, 2013 at 11:10 AM, Stephen Warren <swarren@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 <ldewangan@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
diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi index d06cd12..0655b4d 100644 --- a/arch/arm/dts/tegra114.dtsi +++ b/arch/arm/dts/tegra114.dtsi @@ -2,4 +2,68 @@ / { compatible = "nvidia,tegra114"; + + aliases { + i2c0 = "/i2c@7000d000"; + i2c1 = "/i2c@7000c000"; + i2c2 = "/i2c@7000c400"; + i2c3 = "/i2c@7000c500"; + i2c4 = "/i2c@7000c700"; + }; + + tegra_car: clock@60006000 { + compatible = "nvidia,tegra114-car"; + reg = <0x60006000 0x1000>; + #clock-cells = <1>; + }; + + i2c@7000c000 { + compatible = "nvidia,tegra114-i2c"; + reg = <0x7000c000 0x100>; + interrupts = <0 38 0x04>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&tegra_car 12>; + status = "disabled"; + }; + + i2c@7000c400 { + compatible = "nvidia,tegra114-i2c"; + reg = <0x7000c400 0x100>; + interrupts = <0 84 0x04>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&tegra_car 54>; + status = "disabled"; + }; + + i2c@7000c500 { + compatible = "nvidia,tegra114-i2c"; + reg = <0x7000c500 0x100>; + interrupts = <0 92 0x04>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&tegra_car 67>; + status = "disabled"; + }; + + i2c@7000c700 { + compatible = "nvidia,tegra114-i2c"; + reg = <0x7000c700 0x100>; + interrupts = <0 120 0x04>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&tegra_car 103>; + status = "disabled"; + }; + + i2c@7000d000 { + compatible = "nvidia,tegra114-i2c"; + reg = <0x7000d000 0x100>; + interrupts = <0 53 0x04>; + #address-cells = <1>; + #size-cells = <0>; + clocks = <&tegra_car 47>; + status = "disabled"; + }; }; diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts index 7315577..3f3cd81 100644 --- a/board/nvidia/dts/tegra114-dalmore.dts +++ b/board/nvidia/dts/tegra114-dalmore.dts @@ -6,8 +6,36 @@ model = "NVIDIA Dalmore"; compatible = "nvidia,dalmore", "nvidia,tegra114"; + aliases { + }; + memory { device_type = "memory"; reg = <0x80000000 0x80000000>; }; + + i2c@7000c000 { + status = "okay"; + clock-frequency = <100000>; + }; + + i2c@7000c400 { + status = "okay"; + clock-frequency = <100000>; + }; + + i2c@7000c500 { + status = "okay"; + clock-frequency = <100000>; + }; + + i2c@7000c700 { + status = "okay"; + clock-frequency = <100000>; + }; + + i2c@7000d000 { + status = "okay"; + clock-frequency = <400000>; + }; };