| Message ID | 1360193208-16055-3-git-send-email-twarren@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
On 02/06/2013 04:26 PM, Tom Warren wrote: > Note that T114 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). > diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi > + tegra_car: clock@60006000 { > + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; Only the Tegra114 value should be listed here; the presence of the Tegra30 value in the upstream kernel is a mistake that will be fixed as part of the Tegra114 clock driver patches. > + i2c@7000c000 { > + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c"; Same here; only include the Tegra114 value since the HW isn't 100% compatible with the Tegra20 HW. > diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts > + i2c@7000d000 { > + status = "okay"; > + clock-frequency = <100000>; > + }; According to our downstream Linux kernel, that I2C controller can run up to 400KHz on this board.
On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: > Note that T114 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). > > Signed-off-by: Tom Warren <twarren@nvidia.com> > --- > diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts > index 7315577..13b07f3 100644 > --- a/board/nvidia/dts/tegra114-dalmore.dts > +++ b/board/nvidia/dts/tegra114-dalmore.dts > @@ -6,8 +6,41 @@ > model =NVIDIA Dalmore"; > compatible =nvidia,dalmore", "nvidia,tegra114"; > > + aliases { > + i2c0 =/i2c@7000d000"; > + i2c1 =/i2c@7000c000"; > + i2c2 =/i2c@7000c400"; > + i2c3 =/i2c@7000c500"; > + i2c4 =/i2c@7000c700"; > + }; Can we move this to tegar114.dtsi file. otherwise it looks good. Acked-by: Laxman Dewangan <ldewangan@nvidia.com> ----------------------------------------------------------------------------------- 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. -----------------------------------------------------------------------------------
Stephen, On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/06/2013 04:26 PM, Tom Warren wrote: >> Note that T114 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). > >> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi > >> + tegra_car: clock@60006000 { >> + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; > > Only the Tegra114 value should be listed here; the presence of the > Tegra30 value in the upstream kernel is a mistake that will be fixed as > part of the Tegra114 clock driver patches. OK. But this is why I think hewing to the Linux DT files, while laudable, is a time sink. Though since T114 is so new & still settling in, I guess some churn is expected. > >> + i2c@7000c000 { >> + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c"; > > Same here; only include the Tegra114 value since the HW isn't 100% > compatible with the Tegra20 HW. What does the 'compatible' designater mean, exactly? Does it require 100% identical HW? Compatible, in a SW/HW sense, to me means that newer SW will work with older/similar (but not exactly the same) HW, etc. Tegra U-Boot uses the "tegra20-i2c" name to find and load the I2C driver. I could add a new entry in the compat-names table for tegra114-i2c, but I still don't think there's enough difference between T20/T30 and T114 I2C to justify a whole new I2C driver - so far, it's just one register with a new clk divider that has to be taken in consideration when programming the clock source divider. I could do as the driver does for T20, where there is a separate DVC controller, plus 3 normal I2C controllers. I could 'find_aliases' for the tegrat114-i2c controller first, process its nodes, and return. If no tegrat114-i2c exists, the driver would continue on to look for tegra20-i2c/tegra20-dvc controller info as it does now. It'd still be one tegra_i2c.c driver, with a flag in the i2c_bus struct telling me if I found T114 I2C HW (i2c_bus->single_clk, etc.) and I could then do the new clock divider dance based on that flag. How does that sound? > >> diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts > >> + i2c@7000d000 { >> + status = "okay"; >> + clock-frequency = <100000>; >> + }; > > According to our downstream Linux kernel, that I2C controller can run up > to 400KHz on this board. > But it also runs @ 100KHz just fine, too. Do we need to run at the fastest clock? That's the DVC (PWR_I2C) controller, which U-Boot does little to nothing with right now. I can set it to 400KHz, but what are the advantages/justifications? Is anything wrong with leaving it at 100KHz? Tom
Laxman, On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: >> >> Note that T114 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). >> >> Signed-off-by: Tom Warren <twarren@nvidia.com> >> --- > > > >> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >> b/board/nvidia/dts/tegra114-dalmore.dts >> index 7315577..13b07f3 100644 >> --- a/board/nvidia/dts/tegra114-dalmore.dts >> +++ b/board/nvidia/dts/tegra114-dalmore.dts >> @@ -6,8 +6,41 @@ >> model =NVIDIA Dalmore"; >> compatible =nvidia,dalmore", "nvidia,tegra114"; >> >> + aliases { >> + i2c0 =/i2c@7000d000"; >> + i2c1 =/i2c@7000c000"; >> + i2c2 =/i2c@7000c400"; >> + i2c3 =/i2c@7000c500"; >> + i2c4 =/i2c@7000c700"; >> + }; > > > Can we move this to tegar114.dtsi file. I could, but why? Most, if not all, of the U-Boot boards that use DT are putting their aliases in the .dts file in the board directory. > > otherwise it looks good. > Acked-by: Laxman Dewangan <ldewangan@nvidia.com> Thanks, Laxman. Appreciate your taking a look. 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/07/2013 09:17 AM, Tom Warren wrote: > Laxman, > > On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >> On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: >>> >>> Note that T114 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). >>> >>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>> --- >> >> >> >>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>> b/board/nvidia/dts/tegra114-dalmore.dts >>> index 7315577..13b07f3 100644 >>> --- a/board/nvidia/dts/tegra114-dalmore.dts >>> +++ b/board/nvidia/dts/tegra114-dalmore.dts >>> @@ -6,8 +6,41 @@ >>> model =NVIDIA Dalmore"; >>> compatible =nvidia,dalmore", "nvidia,tegra114"; >>> >>> + aliases { >>> + i2c0 =/i2c@7000d000"; >>> + i2c1 =/i2c@7000c000"; >>> + i2c2 =/i2c@7000c400"; >>> + i2c3 =/i2c@7000c500"; >>> + i2c4 =/i2c@7000c700"; >>> + }; >> >> >> Can we move this to tegar114.dtsi file. > > I could, but why? Most, if not all, of the U-Boot boards that use DT > are putting their aliases in the .dts file in the board directory. Laxman, the issue here is that right now in U-Boot, I believe, if a particular board only uses I2C adapters 0, 2, and 4, then only U-Boot device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's why the aliases are in the per-board file for now, because the actual set of I2C adapters enabled is board-specific. Tom, as background for Laxman's request, for other devices (e.g. serial ports), customer engineers have pushed back on that naming scheme, and always want a specific HW device to end up with a static name, irrespective of which other devices of the same type are used, if any. For that reason, in the kernel, we have aliases for the serial ports in tegra*.dtsi rather than in per-board files, and the names are static. So I wonder if in U-Boot we really have to have IDs 0..n rather than e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we could just put the aliases in tegra*.dtsi, which makes life simpler when creating board .dts files...
Stephen, On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/07/2013 09:17 AM, Tom Warren wrote: >> Laxman, >> >> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >>> On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: >>>> >>>> Note that T114 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). >>>> >>>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>>> --- >>> >>> >>> >>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>>> b/board/nvidia/dts/tegra114-dalmore.dts >>>> index 7315577..13b07f3 100644 >>>> --- a/board/nvidia/dts/tegra114-dalmore.dts >>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts >>>> @@ -6,8 +6,41 @@ >>>> model =NVIDIA Dalmore"; >>>> compatible =nvidia,dalmore", "nvidia,tegra114"; >>>> >>>> + aliases { >>>> + i2c0 =/i2c@7000d000"; >>>> + i2c1 =/i2c@7000c000"; >>>> + i2c2 =/i2c@7000c400"; >>>> + i2c3 =/i2c@7000c500"; >>>> + i2c4 =/i2c@7000c700"; >>>> + }; >>> >>> >>> Can we move this to tegar114.dtsi file. >> >> I could, but why? Most, if not all, of the U-Boot boards that use DT >> are putting their aliases in the .dts file in the board directory. > > Laxman, the issue here is that right now in U-Boot, I believe, if a > particular board only uses I2C adapters 0, 2, and 4, then only U-Boot > device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's > why the aliases are in the per-board file for now, because the actual > set of I2C adapters enabled is board-specific. > > Tom, as background for Laxman's request, for other devices (e.g. serial > ports), customer engineers have pushed back on that naming scheme, and > always want a specific HW device to end up with a static name, > irrespective of which other devices of the same type are used, if any. > For that reason, in the kernel, we have aliases for the serial ports in > tegra*.dtsi rather than in per-board files, and the names are static. > > So I wonder if in U-Boot we really have to have IDs 0..n rather than > e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we > could just put the aliases in tegra*.dtsi, which makes life simpler when > creating board .dts files... Thanks for the background info. I'm focusing on getting T114 I2C in right now, so I can move on to MMC and USB, and what I have seems reasonable/conforms to what's already done in U-Boot. I don't want to get into reworking everyone's dts/dtsi files right now to move aliases around, or find out which boards really have unused I2C ports (Dalmore uses 'em all, BTW, according to our I2C spreadsheet). If you want to send a set of cleanup/re-org patches for everybody's DTS files, or even just the Tegra boards, feel free. I'll be glad to review it/apply it to u-boot-tegra later. Thanks, Tom
On 02/07/2013 09:14 AM, Tom Warren wrote: > Stephen, > > On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 02/06/2013 04:26 PM, Tom Warren wrote: >>> Note that T114 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). >> >>> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi >> >>> + tegra_car: clock@60006000 { >>> + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; >> >> Only the Tegra114 value should be listed here; the presence of the >> Tegra30 value in the upstream kernel is a mistake that will be fixed as >> part of the Tegra114 clock driver patches. > > OK. But this is why I think hewing to the Linux DT files, while > laudable, is a time sink. Though since T114 is so new & still settling > in, I guess some churn is expected. The issue here is not about making U-Boot fall in line with the kernel. The issue is making the device tree in U-Boot be correct. Right now, the kernel happens to have the most correct device tree, so it's a good reference for U-Boot. >>> + i2c@7000c000 { >>> + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c"; >> >> Same here; only include the Tegra114 value since the HW isn't 100% >> compatible with the Tegra20 HW. > > What does the 'compatible' designater mean, exactly? Does it require > 100% identical HW? Compatible, in a SW/HW sense, to me means that > newer SW will work with older/similar (but not exactly the same) HW, > etc. The idea here is that the first entry in compatible always defines the most specific implementation identification possible. So, compatible will at least contain: Tegra20: nvidia,tegra20-i2c Tegra30: nvidia,tegra30-i2c Tegra114: nvidia,tegra114-i2c Now, if a piece of newer hardware can be operated 100% correctly (ignoring issues due to new features not being exposed, or operating at degraded performance) by a driver that only knows about the older hardware, then the compatible property can additionally contain other values indicating what HW it is compatible with. So, we actually end up with: Tegra20: nvidia,tegra20-i2c Tegra30: nvidia,tegra30-i2c nvidia,tegra20-i2c Tegra114: nvidia,tegra114-i2c Tegra114 I2C HW isn't marked as compatible with either Tegra20 or Tegra30, because the clock divider programming must be different. > Tegra U-Boot uses the "tegra20-i2c" name to find and load the I2C > driver. I could add a new entry in the compat-names table for > tegra114-i2c, Yes, that is the way to go. The driver should include a list of all the different compatible values that it supports. > but I still don't think there's enough difference > between T20/T30 and T114 I2C to justify a whole new I2C driver - so > far, it's just one register with a new clk divider that has to be > taken in consideration when programming the clock source divider. But that's required to operate the hardware correctly. It doesn't matter how trivial the difference is; it could just be a single bit that needs to be set/cleared on new HW - there would still be a difference that would otherwise make the HW operate incorrectly. > I could do as the driver does for T20, where there is a separate DVC > controller, plus 3 normal I2C controllers. I could 'find_aliases' for > the tegrat114-i2c controller first, process its nodes, and return. If > no tegrat114-i2c exists, the driver would continue on to look for > tegra20-i2c/tegra20-dvc controller info as it does now. It'd still be > one tegra_i2c.c driver, with a flag in the i2c_bus struct telling me > if I found T114 I2C HW (i2c_bus->single_clk, etc.) and I could then do > the new clock divider dance based on that flag. How does that sound? The U-Boot function that returns the list of devices supported by the driver should be enhanced so that it can search for nodes compatible with any 1 (or more) of n compatible values at a time, rather than just a single compatible value. Then, all you'd have to do with this change is add a new entry into the table, not add extra code that calls that function separately for each compatible value. >>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts >> >>> + i2c@7000d000 { >>> + status = "okay"; >>> + clock-frequency = <100000>; >>> + }; >> >> According to our downstream Linux kernel, that I2C controller can run up >> to 400KHz on this board. > > But it also runs @ 100KHz just fine, too. Do we need to run at the > fastest clock? That's the DVC (PWR_I2C) controller, which U-Boot does > little to nothing with right now. > > I can set it to 400KHz, but what are the advantages/justifications? Is > anything wrong with leaving it at 100KHz? The device tree is about describing the hardware. The hardware can run at 400KHz, so the device tree should accurately describe this. It's simply a matter of correctness, rather than randomly filling in something that happens to work. One practical advantage here is increased boot speed due to I2C accesses taking less time. The advantage might be small here since we probably don't configure too many regulators in U-Boot, but I bet Simon Glass is counting every uS. And again, if/when we can ever use the same DT for U-Boot and the kernel, this needs to be corrected so the kernel isn't forced to run at a reduced speed for some reason. The kernel sends many many more commands to the PMIC, and many are time-sensitive (e.g. CPU voltage bus adjustments for DVFS).
On 02/07/2013 11:14 AM, Tom Warren wrote: > Stephen, > > On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 02/07/2013 09:17 AM, Tom Warren wrote: >>> Laxman, >>> >>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >>>> On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: >>>>> >>>>> Note that T114 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). >>>>> >>>>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>>>> --- >>>> >>>> >>>> >>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>>>> b/board/nvidia/dts/tegra114-dalmore.dts >>>>> index 7315577..13b07f3 100644 >>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts >>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts >>>>> @@ -6,8 +6,41 @@ >>>>> model =NVIDIA Dalmore"; >>>>> compatible =nvidia,dalmore", "nvidia,tegra114"; >>>>> >>>>> + aliases { >>>>> + i2c0 =/i2c@7000d000"; >>>>> + i2c1 =/i2c@7000c000"; >>>>> + i2c2 =/i2c@7000c400"; >>>>> + i2c3 =/i2c@7000c500"; >>>>> + i2c4 =/i2c@7000c700"; >>>>> + }; >>>> >>>> >>>> Can we move this to tegar114.dtsi file. >>> >>> I could, but why? Most, if not all, of the U-Boot boards that use DT >>> are putting their aliases in the .dts file in the board directory. >> >> Laxman, the issue here is that right now in U-Boot, I believe, if a >> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot >> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's >> why the aliases are in the per-board file for now, because the actual >> set of I2C adapters enabled is board-specific. >> >> Tom, as background for Laxman's request, for other devices (e.g. serial >> ports), customer engineers have pushed back on that naming scheme, and >> always want a specific HW device to end up with a static name, >> irrespective of which other devices of the same type are used, if any. >> For that reason, in the kernel, we have aliases for the serial ports in >> tegra*.dtsi rather than in per-board files, and the names are static. >> >> So I wonder if in U-Boot we really have to have IDs 0..n rather than >> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we >> could just put the aliases in tegra*.dtsi, which makes life simpler when >> creating board .dts files... > > Thanks for the background info. I'm focusing on getting T114 I2C in > right now, so I can move on to MMC and USB, and what I have seems > reasonable/conforms to what's already done in U-Boot. That doesn't necessarily make it correct. > I don't want to get into reworking everyone's dts/dtsi files right now > to move aliases around, or find out which boards really have unused > I2C ports (Dalmore uses 'em all, BTW, according to our I2C > spreadsheet). If you want to send a set of cleanup/re-org patches for > everybody's DTS files, or even just the Tegra boards, feel free. I'll > be glad to review it/apply it to u-boot-tegra later. Sorry, but that's simply part of writing the .dts file for a board; there is no way around that.
Stephen, On Thu, Feb 7, 2013 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/07/2013 11:14 AM, Tom Warren wrote: >> Stephen, >> >> On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 02/07/2013 09:17 AM, Tom Warren wrote: >>>> Laxman, >>>> >>>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >>>>> On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: >>>>>> >>>>>> Note that T114 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). >>>>>> >>>>>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>>>>> --- >>>>> >>>>> >>>>> >>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>>>>> b/board/nvidia/dts/tegra114-dalmore.dts >>>>>> index 7315577..13b07f3 100644 >>>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts >>>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts >>>>>> @@ -6,8 +6,41 @@ >>>>>> model =NVIDIA Dalmore"; >>>>>> compatible =nvidia,dalmore", "nvidia,tegra114"; >>>>>> >>>>>> + aliases { >>>>>> + i2c0 =/i2c@7000d000"; >>>>>> + i2c1 =/i2c@7000c000"; >>>>>> + i2c2 =/i2c@7000c400"; >>>>>> + i2c3 =/i2c@7000c500"; >>>>>> + i2c4 =/i2c@7000c700"; >>>>>> + }; >>>>> >>>>> >>>>> Can we move this to tegar114.dtsi file. >>>> >>>> I could, but why? Most, if not all, of the U-Boot boards that use DT >>>> are putting their aliases in the .dts file in the board directory. >>> >>> Laxman, the issue here is that right now in U-Boot, I believe, if a >>> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot >>> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's >>> why the aliases are in the per-board file for now, because the actual >>> set of I2C adapters enabled is board-specific. >>> >>> Tom, as background for Laxman's request, for other devices (e.g. serial >>> ports), customer engineers have pushed back on that naming scheme, and >>> always want a specific HW device to end up with a static name, >>> irrespective of which other devices of the same type are used, if any. >>> For that reason, in the kernel, we have aliases for the serial ports in >>> tegra*.dtsi rather than in per-board files, and the names are static. >>> >>> So I wonder if in U-Boot we really have to have IDs 0..n rather than >>> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we >>> could just put the aliases in tegra*.dtsi, which makes life simpler when >>> creating board .dts files... >> >> Thanks for the background info. I'm focusing on getting T114 I2C in >> right now, so I can move on to MMC and USB, and what I have seems >> reasonable/conforms to what's already done in U-Boot. > > That doesn't necessarily make it correct. > >> I don't want to get into reworking everyone's dts/dtsi files right now >> to move aliases around, or find out which boards really have unused >> I2C ports (Dalmore uses 'em all, BTW, according to our I2C >> spreadsheet). If you want to send a set of cleanup/re-org patches for >> everybody's DTS files, or even just the Tegra boards, feel free. I'll >> be glad to review it/apply it to u-boot-tegra later. > > Sorry, but that's simply part of writing the .dts file for a board; > there is no way around that. OK, how about this. For T114, I'll move the aliases to tegra114.dtsi, and use 400KHz for all I2C nodes except I2C1 (saw some internal code saying that it may not run well at that speed on the E1611 board). Since all 5 controllers have devices behind them, I think it's OK to leave all 5 in the aliases. What do you think of that approach? Adding Yen Lin to CC Tom
On 02/07/2013 12:05 PM, Tom Warren wrote: > Stephen, > > On Thu, Feb 7, 2013 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 02/07/2013 11:14 AM, Tom Warren wrote: >>> Stephen, >>> >>> On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 02/07/2013 09:17 AM, Tom Warren wrote: >>>>> Laxman, >>>>> >>>>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >>>>>> On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: >>>>>>> >>>>>>> Note that T114 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). >>>>>>> >>>>>>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>>>>>> --- >>>>>> >>>>>> >>>>>> >>>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>>>>>> b/board/nvidia/dts/tegra114-dalmore.dts >>>>>>> index 7315577..13b07f3 100644 >>>>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts >>>>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts >>>>>>> @@ -6,8 +6,41 @@ >>>>>>> model =NVIDIA Dalmore"; >>>>>>> compatible =nvidia,dalmore", "nvidia,tegra114"; >>>>>>> >>>>>>> + aliases { >>>>>>> + i2c0 =/i2c@7000d000"; >>>>>>> + i2c1 =/i2c@7000c000"; >>>>>>> + i2c2 =/i2c@7000c400"; >>>>>>> + i2c3 =/i2c@7000c500"; >>>>>>> + i2c4 =/i2c@7000c700"; >>>>>>> + }; >>>>>> >>>>>> >>>>>> Can we move this to tegar114.dtsi file. >>>>> >>>>> I could, but why? Most, if not all, of the U-Boot boards that use DT >>>>> are putting their aliases in the .dts file in the board directory. >>>> >>>> Laxman, the issue here is that right now in U-Boot, I believe, if a >>>> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot >>>> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's >>>> why the aliases are in the per-board file for now, because the actual >>>> set of I2C adapters enabled is board-specific. >>>> >>>> Tom, as background for Laxman's request, for other devices (e.g. serial >>>> ports), customer engineers have pushed back on that naming scheme, and >>>> always want a specific HW device to end up with a static name, >>>> irrespective of which other devices of the same type are used, if any. >>>> For that reason, in the kernel, we have aliases for the serial ports in >>>> tegra*.dtsi rather than in per-board files, and the names are static. >>>> >>>> So I wonder if in U-Boot we really have to have IDs 0..n rather than >>>> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we >>>> could just put the aliases in tegra*.dtsi, which makes life simpler when >>>> creating board .dts files... >>> >>> Thanks for the background info. I'm focusing on getting T114 I2C in >>> right now, so I can move on to MMC and USB, and what I have seems >>> reasonable/conforms to what's already done in U-Boot. >> >> That doesn't necessarily make it correct. >> >>> I don't want to get into reworking everyone's dts/dtsi files right now >>> to move aliases around, or find out which boards really have unused >>> I2C ports (Dalmore uses 'em all, BTW, according to our I2C >>> spreadsheet). If you want to send a set of cleanup/re-org patches for >>> everybody's DTS files, or even just the Tegra boards, feel free. I'll >>> be glad to review it/apply it to u-boot-tegra later. >> >> Sorry, but that's simply part of writing the .dts file for a board; >> there is no way around that. > > OK, how about this. For T114, I'll move the aliases to tegra114.dtsi, I believe that makes sense. But please do check that if you do disable one of the controllers, that the other 4 I2C ports do all get registered and still work OK, so we've tested that code-path. I'm not sure if we have tested the case where the I2C channels don't have contiguous IDs yet, so I have no idea if it works. > and use 400KHz for all I2C nodes except I2C1 (saw some internal code > saying that it may not run well at that speed on the E1611 board). > Since all 5 controllers have devices behind them, I think it's OK to > leave all 5 in the aliases. The downstream kernel only has I2C5 (IIRC) set to 400KHz - the other 4 are limited to 100KHz.
Stephen, On Thu, Feb 7, 2013 at 12:08 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/07/2013 12:05 PM, Tom Warren wrote: >> Stephen, >> >> On Thu, Feb 7, 2013 at 11:20 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 02/07/2013 11:14 AM, Tom Warren wrote: >>>> Stephen, >>>> >>>> On Thu, Feb 7, 2013 at 11:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> On 02/07/2013 09:17 AM, Tom Warren wrote: >>>>>> Laxman, >>>>>> >>>>>> On Thu, Feb 7, 2013 at 7:57 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: >>>>>>> On Thursday 07 February 2013 04:56 AM, Tom Warren wrote: >>>>>>>> >>>>>>>> Note that T114 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). >>>>>>>> >>>>>>>> Signed-off-by: Tom Warren <twarren@nvidia.com> >>>>>>>> --- >>>>>>> >>>>>>> >>>>>>> >>>>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts >>>>>>>> b/board/nvidia/dts/tegra114-dalmore.dts >>>>>>>> index 7315577..13b07f3 100644 >>>>>>>> --- a/board/nvidia/dts/tegra114-dalmore.dts >>>>>>>> +++ b/board/nvidia/dts/tegra114-dalmore.dts >>>>>>>> @@ -6,8 +6,41 @@ >>>>>>>> model =NVIDIA Dalmore"; >>>>>>>> compatible =nvidia,dalmore", "nvidia,tegra114"; >>>>>>>> >>>>>>>> + aliases { >>>>>>>> + i2c0 =/i2c@7000d000"; >>>>>>>> + i2c1 =/i2c@7000c000"; >>>>>>>> + i2c2 =/i2c@7000c400"; >>>>>>>> + i2c3 =/i2c@7000c500"; >>>>>>>> + i2c4 =/i2c@7000c700"; >>>>>>>> + }; >>>>>>> >>>>>>> >>>>>>> Can we move this to tegar114.dtsi file. >>>>>> >>>>>> I could, but why? Most, if not all, of the U-Boot boards that use DT >>>>>> are putting their aliases in the .dts file in the board directory. >>>>> >>>>> Laxman, the issue here is that right now in U-Boot, I believe, if a >>>>> particular board only uses I2C adapters 0, 2, and 4, then only U-Boot >>>>> device IDs 0, 1, and 2 are defined, rather than IDs 0, 2, and 4. That's >>>>> why the aliases are in the per-board file for now, because the actual >>>>> set of I2C adapters enabled is board-specific. >>>>> >>>>> Tom, as background for Laxman's request, for other devices (e.g. serial >>>>> ports), customer engineers have pushed back on that naming scheme, and >>>>> always want a specific HW device to end up with a static name, >>>>> irrespective of which other devices of the same type are used, if any. >>>>> For that reason, in the kernel, we have aliases for the serial ports in >>>>> tegra*.dtsi rather than in per-board files, and the names are static. >>>>> >>>>> So I wonder if in U-Boot we really have to have IDs 0..n rather than >>>>> e.g. IDs 0, 2, 4 for the I2C ports (when some aren't used). Then, we >>>>> could just put the aliases in tegra*.dtsi, which makes life simpler when >>>>> creating board .dts files... >>>> >>>> Thanks for the background info. I'm focusing on getting T114 I2C in >>>> right now, so I can move on to MMC and USB, and what I have seems >>>> reasonable/conforms to what's already done in U-Boot. >>> >>> That doesn't necessarily make it correct. >>> >>>> I don't want to get into reworking everyone's dts/dtsi files right now >>>> to move aliases around, or find out which boards really have unused >>>> I2C ports (Dalmore uses 'em all, BTW, according to our I2C >>>> spreadsheet). If you want to send a set of cleanup/re-org patches for >>>> everybody's DTS files, or even just the Tegra boards, feel free. I'll >>>> be glad to review it/apply it to u-boot-tegra later. >>> >>> Sorry, but that's simply part of writing the .dts file for a board; >>> there is no way around that. >> >> OK, how about this. For T114, I'll move the aliases to tegra114.dtsi, > > I believe that makes sense. > > But please do check that if you do disable one of the controllers, that > the other 4 I2C ports do all get registered and still work OK, so we've > tested that code-path. I'm not sure if we have tested the case where the > I2C channels don't have contiguous IDs yet, so I have no idea if it works. Yep, if I remove the 'status = okay' from one of the ports (meaning that the .dtsi 'status=disabled' takes over), it can't be selected in the 'i2c' command @ the cmd prompt. Even if I take out dev 2, for instance, I can still select/probe devs 0, 1, 3, and 4. > >> and use 400KHz for all I2C nodes except I2C1 (saw some internal code >> saying that it may not run well at that speed on the E1611 board). >> Since all 5 controllers have devices behind them, I think it's OK to >> leave all 5 in the aliases. > > The downstream kernel only has I2C5 (IIRC) set to 400KHz - the other 4 > are limited to 100KHz. OK, I'll update just I2C5/dev 0/PWR_I2C to 400KHz. Thanks
Hi Stephen, On Thu, Feb 7, 2013 at 10:17 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/07/2013 09:14 AM, Tom Warren wrote: >> Stephen, >> >> On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 02/06/2013 04:26 PM, Tom Warren wrote: >>>> Note that T114 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). >>> >>>> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi >>> >>>> + tegra_car: clock@60006000 { >>>> + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; >>> >>> Only the Tegra114 value should be listed here; the presence of the >>> Tegra30 value in the upstream kernel is a mistake that will be fixed as >>> part of the Tegra114 clock driver patches. >> >> OK. But this is why I think hewing to the Linux DT files, while >> laudable, is a time sink. Though since T114 is so new & still settling >> in, I guess some churn is expected. > > The issue here is not about making U-Boot fall in line with the kernel. > The issue is making the device tree in U-Boot be correct. Right now, the > kernel happens to have the most correct device tree, so it's a good > reference for U-Boot. > >>>> + i2c@7000c000 { >>>> + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c"; >>> >>> Same here; only include the Tegra114 value since the HW isn't 100% >>> compatible with the Tegra20 HW. >> >> What does the 'compatible' designater mean, exactly? Does it require >> 100% identical HW? Compatible, in a SW/HW sense, to me means that >> newer SW will work with older/similar (but not exactly the same) HW, >> etc. > > The idea here is that the first entry in compatible always defines the > most specific implementation identification possible. So, compatible > will at least contain: > > Tegra20: nvidia,tegra20-i2c > Tegra30: nvidia,tegra30-i2c > Tegra114: nvidia,tegra114-i2c > > Now, if a piece of newer hardware can be operated 100% correctly > (ignoring issues due to new features not being exposed, or operating at > degraded performance) by a driver that only knows about the older > hardware, then the compatible property can additionally contain other > values indicating what HW it is compatible with. So, we actually end up > with: > > Tegra20: nvidia,tegra20-i2c > Tegra30: nvidia,tegra30-i2c nvidia,tegra20-i2c > Tegra114: nvidia,tegra114-i2c > > Tegra114 I2C HW isn't marked as compatible with either Tegra20 or > Tegra30, because the clock divider programming must be different. > >> Tegra U-Boot uses the "tegra20-i2c" name to find and load the I2C >> driver. I could add a new entry in the compat-names table for >> tegra114-i2c, > > Yes, that is the way to go. The driver should include a list of all the > different compatible values that it supports. > >> but I still don't think there's enough difference >> between T20/T30 and T114 I2C to justify a whole new I2C driver - so >> far, it's just one register with a new clk divider that has to be >> taken in consideration when programming the clock source divider. > > But that's required to operate the hardware correctly. It doesn't matter > how trivial the difference is; it could just be a single bit that needs > to be set/cleared on new HW - there would still be a difference that > would otherwise make the HW operate incorrectly. > >> I could do as the driver does for T20, where there is a separate DVC >> controller, plus 3 normal I2C controllers. I could 'find_aliases' for >> the tegrat114-i2c controller first, process its nodes, and return. If >> no tegrat114-i2c exists, the driver would continue on to look for >> tegra20-i2c/tegra20-dvc controller info as it does now. It'd still be >> one tegra_i2c.c driver, with a flag in the i2c_bus struct telling me >> if I found T114 I2C HW (i2c_bus->single_clk, etc.) and I could then do >> the new clock divider dance based on that flag. How does that sound? > > The U-Boot function that returns the list of devices supported by the > driver should be enhanced so that it can search for nodes compatible > with any 1 (or more) of n compatible values at a time, rather than just > a single compatible value. Then, all you'd have to do with this change > is add a new entry into the table, not add extra code that calls that > function separately for each compatible value. Yes, that needs to be done, and while we are at it I think the function should return a list containing struct {int offset; enum fdt_compat_id; }; I could probably do this by end of week if no one else can do it faster. Please let me know. Tests need to updated also. > >>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts >>> >>>> + i2c@7000d000 { >>>> + status = "okay"; >>>> + clock-frequency = <100000>; >>>> + }; >>> >>> According to our downstream Linux kernel, that I2C controller can run up >>> to 400KHz on this board. >> >> But it also runs @ 100KHz just fine, too. Do we need to run at the >> fastest clock? That's the DVC (PWR_I2C) controller, which U-Boot does >> little to nothing with right now. >> >> I can set it to 400KHz, but what are the advantages/justifications? Is >> anything wrong with leaving it at 100KHz? > > The device tree is about describing the hardware. The hardware can run > at 400KHz, so the device tree should accurately describe this. It's > simply a matter of correctness, rather than randomly filling in > something that happens to work. > > One practical advantage here is increased boot speed due to I2C accesses > taking less time. The advantage might be small here since we probably > don't configure too many regulators in U-Boot, but I bet Simon Glass is > counting every uS. Well we count uS, but only refactor code for mS :-) > > And again, if/when we can ever use the same DT for U-Boot and the > kernel, this needs to be corrected so the kernel isn't forced to run at > a reduced speed for some reason. The kernel sends many many more > commands to the PMIC, and many are time-sensitive (e.g. CPU voltage bus > adjustments for DVFS). Yes we should use the kernel FDT where possible. > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot Regards, Simon
Simon, On Tue, Feb 12, 2013 at 11:13 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Stephen, > > On Thu, Feb 7, 2013 at 10:17 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 02/07/2013 09:14 AM, Tom Warren wrote: >>> Stephen, >>> >>> On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 02/06/2013 04:26 PM, Tom Warren wrote: >>>>> Note that T114 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). >>>> >>>>> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi >>>> >>>>> + tegra_car: clock@60006000 { >>>>> + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; >>>> >>>> Only the Tegra114 value should be listed here; the presence of the >>>> Tegra30 value in the upstream kernel is a mistake that will be fixed as >>>> part of the Tegra114 clock driver patches. >>> >>> OK. But this is why I think hewing to the Linux DT files, while >>> laudable, is a time sink. Though since T114 is so new & still settling >>> in, I guess some churn is expected. >> >> The issue here is not about making U-Boot fall in line with the kernel. >> The issue is making the device tree in U-Boot be correct. Right now, the >> kernel happens to have the most correct device tree, so it's a good >> reference for U-Boot. >> >>>>> + i2c@7000c000 { >>>>> + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c"; >>>> >>>> Same here; only include the Tegra114 value since the HW isn't 100% >>>> compatible with the Tegra20 HW. >>> >>> What does the 'compatible' designater mean, exactly? Does it require >>> 100% identical HW? Compatible, in a SW/HW sense, to me means that >>> newer SW will work with older/similar (but not exactly the same) HW, >>> etc. >> >> The idea here is that the first entry in compatible always defines the >> most specific implementation identification possible. So, compatible >> will at least contain: >> >> Tegra20: nvidia,tegra20-i2c >> Tegra30: nvidia,tegra30-i2c >> Tegra114: nvidia,tegra114-i2c >> >> Now, if a piece of newer hardware can be operated 100% correctly >> (ignoring issues due to new features not being exposed, or operating at >> degraded performance) by a driver that only knows about the older >> hardware, then the compatible property can additionally contain other >> values indicating what HW it is compatible with. So, we actually end up >> with: >> >> Tegra20: nvidia,tegra20-i2c >> Tegra30: nvidia,tegra30-i2c nvidia,tegra20-i2c >> Tegra114: nvidia,tegra114-i2c >> >> Tegra114 I2C HW isn't marked as compatible with either Tegra20 or >> Tegra30, because the clock divider programming must be different. >> >>> Tegra U-Boot uses the "tegra20-i2c" name to find and load the I2C >>> driver. I could add a new entry in the compat-names table for >>> tegra114-i2c, >> >> Yes, that is the way to go. The driver should include a list of all the >> different compatible values that it supports. >> >>> but I still don't think there's enough difference >>> between T20/T30 and T114 I2C to justify a whole new I2C driver - so >>> far, it's just one register with a new clk divider that has to be >>> taken in consideration when programming the clock source divider. >> >> But that's required to operate the hardware correctly. It doesn't matter >> how trivial the difference is; it could just be a single bit that needs >> to be set/cleared on new HW - there would still be a difference that >> would otherwise make the HW operate incorrectly. >> >>> I could do as the driver does for T20, where there is a separate DVC >>> controller, plus 3 normal I2C controllers. I could 'find_aliases' for >>> the tegrat114-i2c controller first, process its nodes, and return. If >>> no tegrat114-i2c exists, the driver would continue on to look for >>> tegra20-i2c/tegra20-dvc controller info as it does now. It'd still be >>> one tegra_i2c.c driver, with a flag in the i2c_bus struct telling me >>> if I found T114 I2C HW (i2c_bus->single_clk, etc.) and I could then do >>> the new clock divider dance based on that flag. How does that sound? >> >> The U-Boot function that returns the list of devices supported by the >> driver should be enhanced so that it can search for nodes compatible >> with any 1 (or more) of n compatible values at a time, rather than just >> a single compatible value. Then, all you'd have to do with this change >> is add a new entry into the table, not add extra code that calls that >> function separately for each compatible value. > > Yes, that needs to be done, and while we are at it I think the > function should return a list containing struct {int offset; enum > fdt_compat_id; }; > > I could probably do this by end of week if no one else can do it > faster. Please let me know. Tests need to updated also. I won't have time for this this week (on vacation Friday thru Tuesday). If you want to tackle this, go ahead. The current T114 I2C patchset is good-to-go AFAICT, with the exception of the clock divisor fix Stephen just pointed out in another thread. So you can use that as your basis for rewrite, if you wish. > >> >>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts >>>> >>>>> + i2c@7000d000 { >>>>> + status = "okay"; >>>>> + clock-frequency = <100000>; >>>>> + }; >>>> >>>> According to our downstream Linux kernel, that I2C controller can run up >>>> to 400KHz on this board. >>> >>> But it also runs @ 100KHz just fine, too. Do we need to run at the >>> fastest clock? That's the DVC (PWR_I2C) controller, which U-Boot does >>> little to nothing with right now. >>> >>> I can set it to 400KHz, but what are the advantages/justifications? Is >>> anything wrong with leaving it at 100KHz? >> >> The device tree is about describing the hardware. The hardware can run >> at 400KHz, so the device tree should accurately describe this. It's >> simply a matter of correctness, rather than randomly filling in >> something that happens to work. >> >> One practical advantage here is increased boot speed due to I2C accesses >> taking less time. The advantage might be small here since we probably >> don't configure too many regulators in U-Boot, but I bet Simon Glass is >> counting every uS. > > Well we count uS, but only refactor code for mS :-) > >> >> And again, if/when we can ever use the same DT for U-Boot and the >> kernel, this needs to be corrected so the kernel isn't forced to run at >> a reduced speed for some reason. The kernel sends many many more >> commands to the PMIC, and many are time-sensitive (e.g. CPU voltage bus >> adjustments for DVFS). > > Yes we should use the kernel FDT where possible. That's the current POR for Tegra DT use in upstream U-Boot, assuming I can find an up-to-date kernel with the latest DTS files (I'll use Stephen's swarren/linux-tegra.git/for-next until told otherwise). Tom > >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot > > Regards, > Simon
Hi Tom, On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote: > Simon, > > On Tue, Feb 12, 2013 at 11:13 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Stephen, >> >> On Thu, Feb 7, 2013 at 10:17 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 02/07/2013 09:14 AM, Tom Warren wrote: >>>> Stephen, >>>> >>>> On Wed, Feb 6, 2013 at 5:00 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> On 02/06/2013 04:26 PM, Tom Warren wrote: >>>>>> Note that T114 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). >>>>> >>>>>> diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi >>>>> >>>>>> + tegra_car: clock@60006000 { >>>>>> + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; >>>>> >>>>> Only the Tegra114 value should be listed here; the presence of the >>>>> Tegra30 value in the upstream kernel is a mistake that will be fixed as >>>>> part of the Tegra114 clock driver patches. >>>> >>>> OK. But this is why I think hewing to the Linux DT files, while >>>> laudable, is a time sink. Though since T114 is so new & still settling >>>> in, I guess some churn is expected. >>> >>> The issue here is not about making U-Boot fall in line with the kernel. >>> The issue is making the device tree in U-Boot be correct. Right now, the >>> kernel happens to have the most correct device tree, so it's a good >>> reference for U-Boot. >>> >>>>>> + i2c@7000c000 { >>>>>> + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-i2c"; >>>>> >>>>> Same here; only include the Tegra114 value since the HW isn't 100% >>>>> compatible with the Tegra20 HW. >>>> >>>> What does the 'compatible' designater mean, exactly? Does it require >>>> 100% identical HW? Compatible, in a SW/HW sense, to me means that >>>> newer SW will work with older/similar (but not exactly the same) HW, >>>> etc. >>> >>> The idea here is that the first entry in compatible always defines the >>> most specific implementation identification possible. So, compatible >>> will at least contain: >>> >>> Tegra20: nvidia,tegra20-i2c >>> Tegra30: nvidia,tegra30-i2c >>> Tegra114: nvidia,tegra114-i2c >>> >>> Now, if a piece of newer hardware can be operated 100% correctly >>> (ignoring issues due to new features not being exposed, or operating at >>> degraded performance) by a driver that only knows about the older >>> hardware, then the compatible property can additionally contain other >>> values indicating what HW it is compatible with. So, we actually end up >>> with: >>> >>> Tegra20: nvidia,tegra20-i2c >>> Tegra30: nvidia,tegra30-i2c nvidia,tegra20-i2c >>> Tegra114: nvidia,tegra114-i2c >>> >>> Tegra114 I2C HW isn't marked as compatible with either Tegra20 or >>> Tegra30, because the clock divider programming must be different. >>> >>>> Tegra U-Boot uses the "tegra20-i2c" name to find and load the I2C >>>> driver. I could add a new entry in the compat-names table for >>>> tegra114-i2c, >>> >>> Yes, that is the way to go. The driver should include a list of all the >>> different compatible values that it supports. >>> >>>> but I still don't think there's enough difference >>>> between T20/T30 and T114 I2C to justify a whole new I2C driver - so >>>> far, it's just one register with a new clk divider that has to be >>>> taken in consideration when programming the clock source divider. >>> >>> But that's required to operate the hardware correctly. It doesn't matter >>> how trivial the difference is; it could just be a single bit that needs >>> to be set/cleared on new HW - there would still be a difference that >>> would otherwise make the HW operate incorrectly. >>> >>>> I could do as the driver does for T20, where there is a separate DVC >>>> controller, plus 3 normal I2C controllers. I could 'find_aliases' for >>>> the tegrat114-i2c controller first, process its nodes, and return. If >>>> no tegrat114-i2c exists, the driver would continue on to look for >>>> tegra20-i2c/tegra20-dvc controller info as it does now. It'd still be >>>> one tegra_i2c.c driver, with a flag in the i2c_bus struct telling me >>>> if I found T114 I2C HW (i2c_bus->single_clk, etc.) and I could then do >>>> the new clock divider dance based on that flag. How does that sound? >>> >>> The U-Boot function that returns the list of devices supported by the >>> driver should be enhanced so that it can search for nodes compatible >>> with any 1 (or more) of n compatible values at a time, rather than just >>> a single compatible value. Then, all you'd have to do with this change >>> is add a new entry into the table, not add extra code that calls that >>> function separately for each compatible value. >> >> Yes, that needs to be done, and while we are at it I think the >> function should return a list containing struct {int offset; enum >> fdt_compat_id; }; >> >> I could probably do this by end of week if no one else can do it >> faster. Please let me know. Tests need to updated also. > > I won't have time for this this week (on vacation Friday thru > Tuesday). If you want to tackle this, go ahead. The current T114 I2C > patchset is good-to-go AFAICT, with the exception of the clock divisor > fix Stephen just pointed out in another thread. So you can use that as > your basis for rewrite, if you wish. OK, well it is independent of these patches, but seem find as they are anyway. It's something we should do but doesn' t need to hold up proceedings. > >> >>> >>>>>> diff --git a/board/nvidia/dts/tegra114-dalmore.dts b/board/nvidia/dts/tegra114-dalmore.dts >>>>> >>>>>> + i2c@7000d000 { >>>>>> + status = "okay"; >>>>>> + clock-frequency = <100000>; >>>>>> + }; >>>>> >>>>> According to our downstream Linux kernel, that I2C controller can run up >>>>> to 400KHz on this board. >>>> >>>> But it also runs @ 100KHz just fine, too. Do we need to run at the >>>> fastest clock? That's the DVC (PWR_I2C) controller, which U-Boot does >>>> little to nothing with right now. >>>> >>>> I can set it to 400KHz, but what are the advantages/justifications? Is >>>> anything wrong with leaving it at 100KHz? >>> >>> The device tree is about describing the hardware. The hardware can run >>> at 400KHz, so the device tree should accurately describe this. It's >>> simply a matter of correctness, rather than randomly filling in >>> something that happens to work. >>> >>> One practical advantage here is increased boot speed due to I2C accesses >>> taking less time. The advantage might be small here since we probably >>> don't configure too many regulators in U-Boot, but I bet Simon Glass is >>> counting every uS. >> >> Well we count uS, but only refactor code for mS :-) >> >>> >>> And again, if/when we can ever use the same DT for U-Boot and the >>> kernel, this needs to be corrected so the kernel isn't forced to run at >>> a reduced speed for some reason. The kernel sends many many more >>> commands to the PMIC, and many are time-sensitive (e.g. CPU voltage bus >>> adjustments for DVFS). >> >> Yes we should use the kernel FDT where possible. > That's the current POR for Tegra DT use in upstream U-Boot, assuming I > can find an up-to-date kernel with the latest DTS files (I'll use > Stephen's swarren/linux-tegra.git/for-next until told otherwise). Yes that was always my problem - finding what the kernel actually used, might use, etc. Regards, Simon > > Tom >> >>> _______________________________________________ >>> U-Boot mailing list >>> U-Boot@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot >> >> Regards, >> Simon
On 02/12/2013 12:17 PM, Simon Glass wrote: > Hi Tom, > > On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote: ... >> That's the current POR for Tegra DT use in upstream U-Boot, assuming I >> can find an up-to-date kernel with the latest DTS files (I'll use >> Stephen's swarren/linux-tegra.git/for-next until told otherwise). > > Yes that was always my problem - finding what the kernel actually > used, might use, etc. I must say I find this quite puzzling; the kernel repos aren't exactly hidden. Tegra's kernel for-next is likely the best place right now as any DT changes typically go through that tree. However, on the off-chance any other maintainer picks up any changes, linux-next.git would have the latest bindings. That's all been true for a year or more. That said, there is a move to either move the binding definitions and .dts files out of the kernel tree and/or stop taking changes to them through individual SoC trees, but perhaps through the device tree repo. If that does happen, I'll let you know.
Hi Stephen, On Tue, Feb 12, 2013 at 11:26 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/12/2013 12:17 PM, Simon Glass wrote: >> Hi Tom, >> >> On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote: > ... >>> That's the current POR for Tegra DT use in upstream U-Boot, assuming I >>> can find an up-to-date kernel with the latest DTS files (I'll use >>> Stephen's swarren/linux-tegra.git/for-next until told otherwise). >> >> Yes that was always my problem - finding what the kernel actually >> used, might use, etc. > > I must say I find this quite puzzling; the kernel repos aren't exactly > hidden. Well if a change has made it to a repo then things are easier, particularly if it is next/. It was chasing down patches on mailing lists that I found hard. > > Tegra's kernel for-next is likely the best place right now as any DT > changes typically go through that tree. However, on the off-chance any > other maintainer picks up any changes, linux-next.git would have the > latest bindings. That's all been true for a year or more. > > That said, there is a move to either move the binding definitions and > .dts files out of the kernel tree and/or stop taking changes to them > through individual SoC trees, but perhaps through the device tree repo. > If that does happen, I'll let you know. Sounds good. Regards, Simon
On Tue, Feb 12, 2013 at 12:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 02/12/2013 12:17 PM, Simon Glass wrote: >> Hi Tom, >> >> On Tue, Feb 12, 2013 at 11:13 AM, Tom Warren <twarren.nvidia@gmail.com> wrote: > ... >>> That's the current POR for Tegra DT use in upstream U-Boot, assuming I >>> can find an up-to-date kernel with the latest DTS files (I'll use >>> Stephen's swarren/linux-tegra.git/for-next until told otherwise). >> >> Yes that was always my problem - finding what the kernel actually >> used, might use, etc. > > I must say I find this quite puzzling; the kernel repos aren't exactly > hidden. Not hidden, but numerous. Perhaps I should have said 'can find _the correct_ kernel ...'. I've been pointed to 2 or 3 different repos over the course of these DT patches. I'm not a kernel guru, and I don't have the bandwidth to monitor kernel reflectors, etc. When I need something kernel-related, I resort to Google or asking you. > > Tegra's kernel for-next is likely the best place right now as any DT > changes typically go through that tree. However, on the off-chance any > other maintainer picks up any changes, linux-next.git would have the > latest bindings. That's all been true for a year or more. > > That said, there is a move to either move the binding definitions and > .dts files out of the kernel tree and/or stop taking changes to them > through individual SoC trees, but perhaps through the device tree repo. > If that does happen, I'll let you know.
diff --git a/arch/arm/dts/tegra114.dtsi b/arch/arm/dts/tegra114.dtsi index d06cd12..7b0c835 100644 --- a/arch/arm/dts/tegra114.dtsi +++ b/arch/arm/dts/tegra114.dtsi @@ -2,4 +2,60 @@ / { compatible = "nvidia,tegra114"; + + tegra_car: clock@60006000 { + compatible = "nvidia,tegra114-car, nvidia,tegra30-car"; + reg = <0x60006000 0x1000>; + #clock-cells = <1>; + }; + + i2c@7000c000 { + compatible = "nvidia,tegra114-i2c", "nvidia,tegra20-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", "nvidia,tegra20-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", "nvidia,tegra20-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", "nvidia,tegra20-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", "nvidia,tegra20-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..13b07f3 100644 --- a/board/nvidia/dts/tegra114-dalmore.dts +++ b/board/nvidia/dts/tegra114-dalmore.dts @@ -6,8 +6,41 @@ model = "NVIDIA Dalmore"; compatible = "nvidia,dalmore", "nvidia,tegra114"; + aliases { + i2c0 = "/i2c@7000d000"; + i2c1 = "/i2c@7000c000"; + i2c2 = "/i2c@7000c400"; + i2c3 = "/i2c@7000c500"; + i2c4 = "/i2c@7000c700"; + }; + 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 = <100000>; + }; };
Note that T114 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). Signed-off-by: Tom Warren <twarren@nvidia.com> --- v2: Match dts files with kernel files, remove unused apdma node arch/arm/dts/tegra114.dtsi | 56 +++++++++++++++++++++++++++++++++ board/nvidia/dts/tegra114-dalmore.dts | 33 +++++++++++++++++++ 2 files changed, 89 insertions(+), 0 deletions(-)