Re: [U-Boot] [PATCH v3 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
Stephen/Laxman,
On Tue, Feb 12, 2013 at 12:07 PM, Tom Warren wrote:
> Stephen,
>
> On Tue, Feb 12, 2013 at 11:10 AM, Stephen Warren
> wrote:
>> On 02/12/2013 10:40 AM, Tom Warren wrote:
>>> Laxman,
>>>
>>> On Tue, Feb 12, 2013 at 5:02 AM, Laxman Dewangan
>>> 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
Re: [U-Boot] [PATCH v3 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
Stephen,
On Tue, Feb 12, 2013 at 11:10 AM, Stephen Warren wrote:
> On 02/12/2013 10:40 AM, Tom Warren wrote:
>> Laxman,
>>
>> On Tue, Feb 12, 2013 at 5:02 AM, Laxman Dewangan
>> 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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
On 02/12/2013 10:40 AM, Tom Warren wrote:
> Laxman,
>
> On Tue, Feb 12, 2013 at 5:02 AM, Laxman Dewangan 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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
Laxman,
On Tue, Feb 12, 2013 at 5:02 AM, Laxman Dewangan 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.
> ---
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
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.
---
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v3 2/3] Tegra114: fdt: Update DT files with I2C info for T114/Dalmore
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.
Signed-off-by: Tom Warren
Acked-by: Laxman Dewangan
---
v2: Match dts files with kernel files, remove unused apdma node
v3:
- moved aliases to dtsi file as per StephenW
- removed tegra30-car & tegra20-i2c compatible strings
- changed I2C5 (PWR_I2C) clock to 400MHz
arch/arm/dts/tegra114.dtsi| 64 +
board/nvidia/dts/tegra114-dalmore.dts | 28 ++
2 files changed, 92 insertions(+), 0 deletions(-)
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 = <0x8000 0x8000>;
};
+
+ i2c@7000c000 {
+ status = "okay";
+ clock-frequency = <10>;
+ };
+
+ i2c@7000c400 {
+ status = "okay";
+ clock-frequency = <10>;
+ };
+
+ i2c@7000c500 {
+ status = "okay";
+ clock-frequency = <10>;
+ };
+
+ i2c@7000c700 {
+ status = "okay";
+ clock-frequency = <10>;
+ };
+
+ i2c@7000d000 {
+ status = "okay";
+ clock-frequency = <40>;
+ };
};
--
1.7.0.4
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
