Patchwork [U-Boot,1/2] Tegra: fdt: Add/enhance sdhci (mmc) nodes for all T20 DT files

login
register
mail settings
Submitter Tom Warren
Date Feb. 4, 2013, 11:48 p.m.
Message ID <1360021735-13286-2-git-send-email-twarren@nvidia.com>
Download mbox | patch
Permalink /patch/218119/
State Superseded
Delegated to: Tom Warren
Headers show

Comments

Tom Warren - Feb. 4, 2013, 11:48 p.m.
Linux dts files were used for those boards that didn't already
have sdhci info populated. If a cd-gpio was present, I set
"removable = <1>", else removable = <0>.

Signed-off-by: Tom Warren <twarren@nvidia.com>
---
 arch/arm/dts/tegra20.dtsi                        |   12 +++++++++++
 board/avionic-design/dts/tegra20-medcom-wide.dts |    9 ++++++++
 board/avionic-design/dts/tegra20-plutux.dts      |    9 ++++++++
 board/avionic-design/dts/tegra20-tec.dts         |    9 ++++++++
 board/compal/dts/tegra20-paz00.dts               |   22 +++++++++++++++++++-
 board/compulab/dts/tegra20-trimslice.dts         |   17 ++++++++++++++++
 board/nvidia/dts/tegra20-harmony.dts             |   21 ++++++++++++++++++++
 board/nvidia/dts/tegra20-seaboard.dts            |   19 ++++++++++++-----
 board/nvidia/dts/tegra20-ventana.dts             |   23 ++++++++++++++++++++++
 board/nvidia/dts/tegra20-whistler.dts            |   16 +++++++++++++++
 board/toradex/dts/tegra20-colibri_t20_iris.dts   |    8 +++++++
 11 files changed, 157 insertions(+), 8 deletions(-)
Stephen Warren - Feb. 5, 2013, 7:54 p.m.
On 02/04/2013 04:48 PM, Tom Warren wrote:
> Linux dts files were used for those boards that didn't already
> have sdhci info populated. If a cd-gpio was present, I set
> "removable = <1>", else removable = <0>.

> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi

>  	sdhci@c8000200 {
>  		compatible = "nvidia,tegra20-sdhci";
>  		reg = <0xc8000200 0x200>;
>  		interrupts = < 47 >;
> +		status = "disabled";
> +		/* PERIPH_ID_SDMMC2, PLLP_OUT? */
> +		clocks = <&tegra_car 9>;
>  	};

What does "PLLP_OUT?" mean?

I'm not entirely convinced it's a good idea to add this comment, since
it creates a diff between the .dts files in the kernel and U-Boot.

Similarly, the status and clocks properties are in the other order in
the kernel .dts files. It'd be good to be consistent to allow minimal
diffs between them.

> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts

I suppose that there are no aliases in this file because only one SDHCI
controller is enabled. I wonder if we should add aliases to all .dts
files just to be explicit? Perhaps it's not necessary because this board
really will never ever get another SDHCI controller added (I assume that
any SDHCI controllers the board has are already enabled, although I
wonder about SDIO...), so there doesn't need to be a "hint" that there
should be an alias added too?

> +	sdhci@c8000600 {

In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
its parameters are defined by the carrier board. I think U-Boot .dts
files should match.


The following 3 comments apply to all the .dts files (or at least the
1st and 3rd; not sure about the 2nd):

> +		status = "okay";
> +		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */

I don't think that comment is particularly useful. While it's true,
duplicating it every single place a GPIO is used seems wasteful. And the
comment is more re: the GPIO binding that anything to do with SDHCI.
Plus, it makes another diff relative to the kernel.

> +		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
> +		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */

The kernel appears to have a space before the comment not a TAB, so this
makes another diff..

> +		bus-width = <4>;
> +		removable = <1>;

What is "removable"? That's not in the binding documentation. There is a
"non-removable" property defined in the kernel's
Documentation/devicetree/bindings/mmc/mmc.txt though...

> +	};

> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts

This file doesn't have any aliases added.

> +	sdhci@c8000000 {
> +		status = "okay";
> +		power-gpios = <&gpio 86 0>;	/* gpio PK6 */
> +		bus-width = <4>;
> +		removable = <0>;
> +	};

I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.
Tom Warren - Feb. 5, 2013, 8:29 p.m.
Stephen,

On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/04/2013 04:48 PM, Tom Warren wrote:
>> Linux dts files were used for those boards that didn't already
>> have sdhci info populated. If a cd-gpio was present, I set
>> "removable = <1>", else removable = <0>.
>
>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>
>>       sdhci@c8000200 {
>>               compatible = "nvidia,tegra20-sdhci";
>>               reg = <0xc8000200 0x200>;
>>               interrupts = < 47 >;
>> +             status = "disabled";
>> +             /* PERIPH_ID_SDMMC2, PLLP_OUT? */
>> +             clocks = <&tegra_car 9>;
>>       };
>
> What does "PLLP_OUT?" mean?

The clock source used for this periph. I removed it in the I2C DT
files - I'll remove it here, too, because it's up to the driver to
choose that based on the freq.

>
> I'm not entirely convinced it's a good idea to add this comment, since
> it creates a diff between the .dts files in the kernel and U-Boot.
>
> Similarly, the status and clocks properties are in the other order in
> the kernel .dts files. It'd be good to be consistent to allow minimal
> diffs between them.

I used the kernel DTS files you pointed to in our internal 'Initialize
SDHCI from device tree' bug:
repo git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git
branch for-next

The order matches arch/arm/boot/dts/tegra20.dtsi. I don't see a clocks
property in that kernel file, either. I do see that the interrupts
(that were already in tegra20.dtsi before I edited it) don't match the
kernel dtsi for most peripherals (i2c, i2s, sdhci, gpio). I'll fix the
SDHCI ones in the next patchset, though.

>
>> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
>
> I suppose that there are no aliases in this file because only one SDHCI
> controller is enabled. I wonder if we should add aliases to all .dts
> files just to be explicit? Perhaps it's not necessary because this board
> really will never ever get another SDHCI controller added (I assume that
> any SDHCI controllers the board has are already enabled, although I
> wonder about SDIO...), so there doesn't need to be a "hint" that there
> should be an alias added too?

If there was already an alias property in the DT file, then I tried to
be consistent and add one for mmc. But adding aliases to
other-than-NVIDIA boards should be up to the board
maintainer/manufacturer, IMO.

>
>> +     sdhci@c8000600 {
>
> In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
> its parameters are defined by the carrier board. I think U-Boot .dts
> files should match.

Saw that, but I didn't replicate it here because, well, U-Boot's Not
Linux, and IMO each board file (dts) should have its periph nodes
called out explicitly. If they all happen to be exactly the same for
each board, then I think the manufacturer/board maintainer can do that
'optimization' if they wish - they know better than I if they're
coming out with a new board that may differ in its SDHCI properties
(GPIOs, for instance).

>
>
> The following 3 comments apply to all the .dts files (or at least the
> 1st and 3rd; not sure about the 2nd):
>
>> +             status = "okay";
>> +             /* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
>
> I don't think that comment is particularly useful. While it's true,
> duplicating it every single place a GPIO is used seems wasteful. And the
> comment is more re: the GPIO binding that anything to do with SDHCI.
> Plus, it makes another diff relative to the kernel.

Diffing the DTS files against the kernel isn't really that big a deal
with a decent diff program (kompare, etc.). That GPIO comment is
useful if you need to understand the 3rd param for the pwr-gpios, for
instance (the cd and wp gpios almost always are input/low). And it
only appears once in each DTS file, not "in every single place a GPIO
is used".
>
>> +             cd-gpios = <&gpio 58 0>;        /* gpio PH2 */
>> +             wp-gpios = <&gpio 59 0>;        /* gpio PH3 */
>
> The kernel appears to have a space before the comment not a TAB, so this
> makes another diff..
Really? That seems a little nit-picky. :/
My whitespace is consistent through-out the DTS file, and with how I
always space comments on the end of a line of 'code'.

>
>> +             bus-width = <4>;
>> +             removable = <1>;
>
> What is "removable"? That's not in the binding documentation. There is a
> "non-removable" property defined in the kernel's
> Documentation/devicetree/bindings/mmc/mmc.txt though...

These are left-over from Seaboard's original DTS file (with which I
did my original development). It isn't used anywhere in U-Boot that I
can see. I can remove it.

>
>> +     };
>
>> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
>
> This file doesn't have any aliases added.
>
>> +     sdhci@c8000000 {
>> +             status = "okay";
>> +             power-gpios = <&gpio 86 0>;     /* gpio PK6 */
>> +             bus-width = <4>;
>> +             removable = <0>;
>> +     };
>
> I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.
It's in the kernel DTS for Ventana. Won't that screw up your diff? ;)
I'll remove it.

Tom
Stephen Warren - Feb. 5, 2013, 8:49 p.m.
On 02/05/2013 01:29 PM, Tom Warren wrote:
> Stephen,
> 
> On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/04/2013 04:48 PM, Tom Warren wrote:
>>> Linux dts files were used for those boards that didn't already
>>> have sdhci info populated. If a cd-gpio was present, I set
>>> "removable = <1>", else removable = <0>.
>>
>>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>>
>>>       sdhci@c8000200 {
>>>               compatible = "nvidia,tegra20-sdhci";
>>>               reg = <0xc8000200 0x200>;
>>>               interrupts = < 47 >;
>>> +             status = "disabled";
>>> +             /* PERIPH_ID_SDMMC2, PLLP_OUT? */
>>> +             clocks = <&tegra_car 9>;
>>>       };
>>
>> What does "PLLP_OUT?" mean?
> 
> The clock source used for this periph. I removed it in the I2C DT
> files - I'll remove it here, too, because it's up to the driver to
> choose that based on the freq.
> 
>>
>> I'm not entirely convinced it's a good idea to add this comment, since
>> it creates a diff between the .dts files in the kernel and U-Boot.
>>
>> Similarly, the status and clocks properties are in the other order in
>> the kernel .dts files. It'd be good to be consistent to allow minimal
>> diffs between them.
> 
> I used the kernel DTS files you pointed to in our internal 'Initialize
> SDHCI from device tree' bug:
> repo git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git
> branch for-next

linux-next or the Tegra git tree have the latest additions. arm-soc
hopefully will have them merged in the next day or two.

git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
(whatever the latest tag is there)

git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git
(for-next)

>>> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
>>
>> I suppose that there are no aliases in this file because only one SDHCI
>> controller is enabled. I wonder if we should add aliases to all .dts
>> files just to be explicit? Perhaps it's not necessary because this board
>> really will never ever get another SDHCI controller added (I assume that
>> any SDHCI controllers the board has are already enabled, although I
>> wonder about SDIO...), so there doesn't need to be a "hint" that there
>> should be an alias added too?
> 
> If there was already an alias property in the DT file, then I tried to
> be consistent and add one for mmc. But adding aliases to
> other-than-NVIDIA boards should be up to the board
> maintainer/manufacturer, IMO.

I don't think so; at least with the driver as-is, the code appears not
to work without aliases, so not adding them causes a regression. Even
ignoring that, I don't see why the code->DT conversion patch shouldn't
do this for all boards.

>>> +     sdhci@c8000600 {
>>
>> In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
>> its parameters are defined by the carrier board. I think U-Boot .dts
>> files should match.
> 
> Saw that, but I didn't replicate it here because, well, U-Boot's Not
> Linux, and IMO each board file (dts) should have its periph nodes
> called out explicitly. If they all happen to be exactly the same for
> each board, then I think the manufacturer/board maintainer can do that
> 'optimization' if they wish - they know better than I if they're
> coming out with a new board that may differ in its SDHCI properties
> (GPIOs, for instance).

No, this has nothing to do with U-Boot vs. Linux. The device tree files
are (should eventually be) standard between the two, and indeed hosted
outside U-Boot. Unrelated, common code is common and should be
represented at a common location. In this case, the vendor for this
particular file has already correctly chosen to put the SDHCI nodes in
the common file, so this needs to be maintained.

>> The following 3 comments apply to all the .dts files (or at least the
>> 1st and 3rd; not sure about the 2nd):
>>
>>> +             status = "okay";
>>> +             /* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
>>
>> I don't think that comment is particularly useful. While it's true,
>> duplicating it every single place a GPIO is used seems wasteful. And the
>> comment is more re: the GPIO binding that anything to do with SDHCI.
>> Plus, it makes another diff relative to the kernel.
> 
> Diffing the DTS files against the kernel isn't really that big a deal
> with a decent diff program (kompare, etc.). That GPIO comment is
> useful if you need to understand the 3rd param for the pwr-gpios, for
> instance (the cd and wp gpios almost always are input/low). And it
> only appears once in each DTS file, not "in every single place a GPIO
> is used".

If there is no diff at all, it's even easier.

The third parameter is already documented in the binding documentation.

>>> +             cd-gpios = <&gpio 58 0>;        /* gpio PH2 */
>>> +             wp-gpios = <&gpio 59 0>;        /* gpio PH3 */
>>
>> The kernel appears to have a space before the comment not a TAB, so this
>> makes another diff..
>
> Really? That seems a little nit-picky. :/
> My whitespace is consistent through-out the DTS file, and with how I
> always space comments on the end of a line of 'code'.

Yes, really. Why would I bother to make this review comment otherwise.
As I have repeatedly specified, the idea is to reduce the diff between
the kernel and U-Boot .dts files so the diff for a node is zero. There's
a big difference between zero (no manual checking required at all) vs.
even something trivial (always requires manual checking every time a
comparison is made).

Note that at some point, all these .dts files are probably going to be
removed from the kernel and U-Boot anyway, and hosted separately, so
unifying them can only help there.

Sometimes I wonder why I even both reviewing things.

>>> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
>>
>> This file doesn't have any aliases added.
>>
>>> +     sdhci@c8000000 {
>>> +             status = "okay";
>>> +             power-gpios = <&gpio 86 0>;     /* gpio PK6 */
>>> +             bus-width = <4>;
>>> +             removable = <0>;
>>> +     };
>>
>> I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.
>
> It's in the kernel DTS for Ventana. Won't that screw up your diff? ;)
> I'll remove it.

Perhaps, but as I said before, whole nodes present/missing is a lot
easier to deal with than diffs within nodes.

Right now, I believe your/Simon's policy on DT is to only include in the
U-Boot .dts files what's actually needed for U-Boot. I've asked that
this be done on a per-node basis rather than per-property basis in order
to reduce diffs. If you want to change that, and include nodes that
U-Boot doesn't need, that'd be great and assist unification, but then
I'd recommend simply importing the current kernel .dts files as-is
without any changes, rather than adding things piece-meal.
Simon Glass - Feb. 6, 2013, 4:56 a.m.
Hi,

On Tue, Feb 5, 2013 at 12:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/05/2013 01:29 PM, Tom Warren wrote:
>> Stephen,
>>
>> On Tue, Feb 5, 2013 at 12:54 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 02/04/2013 04:48 PM, Tom Warren wrote:
>>>> Linux dts files were used for those boards that didn't already
>>>> have sdhci info populated. If a cd-gpio was present, I set
>>>> "removable = <1>", else removable = <0>.
>>>
>>>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>>>
>>>>       sdhci@c8000200 {
>>>>               compatible = "nvidia,tegra20-sdhci";
>>>>               reg = <0xc8000200 0x200>;
>>>>               interrupts = < 47 >;
>>>> +             status = "disabled";
>>>> +             /* PERIPH_ID_SDMMC2, PLLP_OUT? */
>>>> +             clocks = <&tegra_car 9>;
>>>>       };
>>>
>>> What does "PLLP_OUT?" mean?
>>
>> The clock source used for this periph. I removed it in the I2C DT
>> files - I'll remove it here, too, because it's up to the driver to
>> choose that based on the freq.
>>
>>>
>>> I'm not entirely convinced it's a good idea to add this comment, since
>>> it creates a diff between the .dts files in the kernel and U-Boot.
>>>
>>> Similarly, the status and clocks properties are in the other order in
>>> the kernel .dts files. It'd be good to be consistent to allow minimal
>>> diffs between them.
>>
>> I used the kernel DTS files you pointed to in our internal 'Initialize
>> SDHCI from device tree' bug:
>> repo git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git
>> branch for-next
>
> linux-next or the Tegra git tree have the latest additions. arm-soc
> hopefully will have them merged in the next day or two.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> (whatever the latest tag is there)
>
> git://git.kernel.org/pub/scm/linux/kernel/git/swarren/linux-tegra.git
> (for-next)
>
>>>> diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
>>>
>>> I suppose that there are no aliases in this file because only one SDHCI
>>> controller is enabled. I wonder if we should add aliases to all .dts
>>> files just to be explicit? Perhaps it's not necessary because this board
>>> really will never ever get another SDHCI controller added (I assume that
>>> any SDHCI controllers the board has are already enabled, although I
>>> wonder about SDIO...), so there doesn't need to be a "hint" that there
>>> should be an alias added too?
>>
>> If there was already an alias property in the DT file, then I tried to
>> be consistent and add one for mmc. But adding aliases to
>> other-than-NVIDIA boards should be up to the board
>> maintainer/manufacturer, IMO.
>
> I don't think so; at least with the driver as-is, the code appears not
> to work without aliases, so not adding them causes a regression. Even
> ignoring that, I don't see why the code->DT conversion patch shouldn't
> do this for all boards.
>
>>>> +     sdhci@c8000600 {
>>>
>>> In the kernel, this SDHCI node is part of tegra20-tamonten.dtsi, since
>>> its parameters are defined by the carrier board. I think U-Boot .dts
>>> files should match.
>>
>> Saw that, but I didn't replicate it here because, well, U-Boot's Not
>> Linux, and IMO each board file (dts) should have its periph nodes
>> called out explicitly. If they all happen to be exactly the same for
>> each board, then I think the manufacturer/board maintainer can do that
>> 'optimization' if they wish - they know better than I if they're
>> coming out with a new board that may differ in its SDHCI properties
>> (GPIOs, for instance).
>
> No, this has nothing to do with U-Boot vs. Linux. The device tree files
> are (should eventually be) standard between the two, and indeed hosted
> outside U-Boot. Unrelated, common code is common and should be
> represented at a common location. In this case, the vendor for this
> particular file has already correctly chosen to put the SDHCI nodes in
> the common file, so this needs to be maintained.
>
>>> The following 3 comments apply to all the .dts files (or at least the
>>> 1st and 3rd; not sure about the 2nd):
>>>
>>>> +             status = "okay";
>>>> +             /* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
>>>
>>> I don't think that comment is particularly useful. While it's true,
>>> duplicating it every single place a GPIO is used seems wasteful. And the
>>> comment is more re: the GPIO binding that anything to do with SDHCI.
>>> Plus, it makes another diff relative to the kernel.
>>
>> Diffing the DTS files against the kernel isn't really that big a deal
>> with a decent diff program (kompare, etc.). That GPIO comment is
>> useful if you need to understand the 3rd param for the pwr-gpios, for
>> instance (the cd and wp gpios almost always are input/low). And it
>> only appears once in each DTS file, not "in every single place a GPIO
>> is used".
>
> If there is no diff at all, it's even easier.
>
> The third parameter is already documented in the binding documentation.
>
>>>> +             cd-gpios = <&gpio 58 0>;        /* gpio PH2 */
>>>> +             wp-gpios = <&gpio 59 0>;        /* gpio PH3 */
>>>
>>> The kernel appears to have a space before the comment not a TAB, so this
>>> makes another diff..
>>
>> Really? That seems a little nit-picky. :/
>> My whitespace is consistent through-out the DTS file, and with how I
>> always space comments on the end of a line of 'code'.
>
> Yes, really. Why would I bother to make this review comment otherwise.
> As I have repeatedly specified, the idea is to reduce the diff between
> the kernel and U-Boot .dts files so the diff for a node is zero. There's
> a big difference between zero (no manual checking required at all) vs.
> even something trivial (always requires manual checking every time a
> comparison is made).
>
> Note that at some point, all these .dts files are probably going to be
> removed from the kernel and U-Boot anyway, and hosted separately, so
> unifying them can only help there.
>
> Sometimes I wonder why I even both reviewing things.
>
>>>> diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
>>>
>>> This file doesn't have any aliases added.
>>>
>>>> +     sdhci@c8000000 {
>>>> +             status = "okay";
>>>> +             power-gpios = <&gpio 86 0>;     /* gpio PK6 */
>>>> +             bus-width = <4>;
>>>> +             removable = <0>;
>>>> +     };
>>>
>>> I think that's the WiFi SDIO port, so you may not need to add it to U-Boot.
>>
>> It's in the kernel DTS for Ventana. Won't that screw up your diff? ;)
>> I'll remove it.
>
> Perhaps, but as I said before, whole nodes present/missing is a lot
> easier to deal with than diffs within nodes.
>
> Right now, I believe your/Simon's policy on DT is to only include in the
> U-Boot .dts files what's actually needed for U-Boot. I've asked that
> this be done on a per-node basis rather than per-property basis in order
> to reduce diffs. If you want to change that, and include nodes that
> U-Boot doesn't need, that'd be great and assist unification, but then
> I'd recommend simply importing the current kernel .dts files as-is
> without any changes, rather than adding things piece-meal.

I have to say that within reason I like the idea of bring in the DT
from the kernel as is, limited perhaps to the nodes that U-Boot
actually uses.

A separate repo for the DT files seems like something that should
happen, but I have seen little progress on that front. Still, when it
happens, it would be nice it we could drop U-Boot's files and just use
the kernel's. That will be a lot easier if we head in that direction
now.

Regards,
Simon

> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
Scott Wood - Feb. 11, 2013, 7:48 p.m.
On 02/05/2013 10:56:59 PM, Simon Glass wrote:
> Hi,
> 
> On Tue, Feb 5, 2013 at 12:49 PM, Stephen Warren  
> <swarren@wwwdotorg.org> wrote:
> > Right now, I believe your/Simon's policy on DT is to only include  
> in the
> > U-Boot .dts files what's actually needed for U-Boot. I've asked that
> > this be done on a per-node basis rather than per-property basis in  
> order
> > to reduce diffs. If you want to change that, and include nodes that
> > U-Boot doesn't need, that'd be great and assist unification, but  
> then
> > I'd recommend simply importing the current kernel .dts files as-is
> > without any changes, rather than adding things piece-meal.
> 
> I have to say that within reason I like the idea of bring in the DT
> from the kernel as is, limited perhaps to the nodes that U-Boot
> actually uses.
> 
> A separate repo for the DT files seems like something that should
> happen, but I have seen little progress on that front. Still, when it
> happens, it would be nice it we could drop U-Boot's files and just use
> the kernel's. That will be a lot easier if we head in that direction
> now.

I think any device tree that makes assumptions about what U-Boot will  
be fixing up, or even what addresses U-Boot will configure devices at,  
belongs in the U-Boot tree.  Keeping such trees in Linux has been  
awkward so far, especially when a change gets made to such an  
assumption, or when U-Boot isn't the only supported firmware.

-Scott

Patch

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index 12049fd..2603277 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -307,23 +307,35 @@ 
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000000 0x200>;
 		interrupts = < 46 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC1, PLLP_OUT? */
+		clocks = <&tegra_car 14>;
 	};
 
 	sdhci@c8000200 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000200 0x200>;
 		interrupts = < 47 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC2, PLLP_OUT? */
+		clocks = <&tegra_car 9>;
 	};
 
 	sdhci@c8000400 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000400 0x200>;
 		interrupts = < 51 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC3, PLLP_OUT? */
+		clocks = <&tegra_car 69>;
 	};
 
 	sdhci@c8000600 {
 		compatible = "nvidia,tegra20-sdhci";
 		reg = <0xc8000600 0x200>;
 		interrupts = < 63 >;
+		status = "disabled";
+		/* PERIPH_ID_SDMMC4, PLLP_OUT? */
+		clocks = <&tegra_car 15>;
 	};
 };
diff --git a/board/avionic-design/dts/tegra20-medcom-wide.dts b/board/avionic-design/dts/tegra20-medcom-wide.dts
index e46afbe..33d174d 100644
--- a/board/avionic-design/dts/tegra20-medcom-wide.dts
+++ b/board/avionic-design/dts/tegra20-medcom-wide.dts
@@ -55,6 +55,15 @@ 
 		status = "disabled";
 	};
 
+	sdhci@c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
 	lcd_panel: panel {
 		clock = <61715000>;
 		xres = <1366>;
diff --git a/board/avionic-design/dts/tegra20-plutux.dts b/board/avionic-design/dts/tegra20-plutux.dts
index 3e6cce0..3a0147f 100644
--- a/board/avionic-design/dts/tegra20-plutux.dts
+++ b/board/avionic-design/dts/tegra20-plutux.dts
@@ -41,4 +41,13 @@ 
 	usb@c5004000 {
 		status = "disabled";
 	};
+
+	sdhci@c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		bus-width = <4>;
+		removable = <1>;
+	};
 };
diff --git a/board/avionic-design/dts/tegra20-tec.dts b/board/avionic-design/dts/tegra20-tec.dts
index bf3ff1d..521a255 100644
--- a/board/avionic-design/dts/tegra20-tec.dts
+++ b/board/avionic-design/dts/tegra20-tec.dts
@@ -66,6 +66,15 @@ 
 		status = "disabled";
 	};
 
+	sdhci@c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
 	lcd_panel: panel {
 		clock = <33260000>;
 		xres = <800>;
diff --git a/board/compal/dts/tegra20-paz00.dts b/board/compal/dts/tegra20-paz00.dts
index 31b064d..ed709c8 100644
--- a/board/compal/dts/tegra20-paz00.dts
+++ b/board/compal/dts/tegra20-paz00.dts
@@ -3,11 +3,13 @@ 
 /include/ ARCH_CPU_DTS
 
 / {
-        model = "Toshiba AC100 / Dynabook AZ";
-        compatible = "compal,paz00", "nvidia,tegra20";
+	model = "Toshiba AC100 / Dynabook AZ";
+	compatible = "compal,paz00", "nvidia,tegra20";
 
 	aliases {
 		usb0 = "/usb@c5008000";
+		sdmmc0 = "/sdhci@c8000600";
+		sdmmc1 = "/sdhci@c8000000";
 	};
 
 	memory {
@@ -53,6 +55,22 @@ 
 		status = "disabled";
 	};
 
+	sdhci@c8000000 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 173 0>;	/* gpio PV5 */
+		wp-gpios = <&gpio 57 0>;	/* gpio PH1 */
+		power-gpios = <&gpio 169 0>;	/* gpio PV1 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
+	sdhci@c8000600 {
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
+	};
+
 	lcd_panel: panel {
 		/* PAZ00 has 1024x600 */
 		clock = <54030000>;
diff --git a/board/compulab/dts/tegra20-trimslice.dts b/board/compulab/dts/tegra20-trimslice.dts
index c8a4dd4..0ab1c1d 100644
--- a/board/compulab/dts/tegra20-trimslice.dts
+++ b/board/compulab/dts/tegra20-trimslice.dts
@@ -9,6 +9,8 @@ 
 	aliases {
 		usb0 = "/usb@c5008000";
 		usb1 = "/usb@c5000000";
+		sdmmc0 = "/sdhci@c8000600";
+		sdmmc1 = "/sdhci@c8000000";
 	};
 
 	memory {
@@ -42,4 +44,19 @@ 
 	usb@c5004000 {
 		status = "disabled";
 	};
+
+	sdhci@c8000000 {
+		status = "okay";
+		bus-width = <4>;
+		removable = <0>;
+	};
+
+	sdhci@c8000600 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 121 0>;	/* gpio PP1 */
+		wp-gpios = <&gpio 122 0>;	/* gpio PP2 */
+		bus-width = <4>;
+		removable = <1>;
+	};
 };
diff --git a/board/nvidia/dts/tegra20-harmony.dts b/board/nvidia/dts/tegra20-harmony.dts
index aeda3a1..de7ebdc 100644
--- a/board/nvidia/dts/tegra20-harmony.dts
+++ b/board/nvidia/dts/tegra20-harmony.dts
@@ -9,6 +9,8 @@ 
 	aliases {
 		usb0 = "/usb@c5008000";
 		usb1 = "/usb@c5004000";
+		sdmmc0 = "/sdhci@c8000600";
+		sdmmc1 = "/sdhci@c8000200";
 	};
 
 	memory {
@@ -52,4 +54,23 @@ 
 	usb@c5004000 {
 		nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
 	};
+
+	sdhci@c8000200 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 69 0>;	/* gpio PI5 */
+		wp-gpios = <&gpio 57 0>;	/* gpio PH1 */
+		power-gpios = <&gpio 155 0>;	/* gpio PT3 */
+		bus-width = <4>;
+		removeable = <1>;
+	};
+
+	sdhci@c8000600 {
+		status = "okay";
+		cd-gpios = <&gpio 58 0>;	/* gpio PH2 */
+		wp-gpios = <&gpio 59 0>;	/* gpio PH3 */
+		power-gpios = <&gpio 70 0>;	/* gpio PI6 */
+		bus-width = <8>;
+		removeable = <1>;
+	};
 };
diff --git a/board/nvidia/dts/tegra20-seaboard.dts b/board/nvidia/dts/tegra20-seaboard.dts
index 527a296..1b1f563 100644
--- a/board/nvidia/dts/tegra20-seaboard.dts
+++ b/board/nvidia/dts/tegra20-seaboard.dts
@@ -12,14 +12,15 @@ 
 	};
 
 	aliases {
-		/* This defines the order of our USB ports */
+		/* This defines the order of our ports */
 		usb0 = "/usb@c5008000";
 		usb1 = "/usb@c5000000";
-
 		i2c0 = "/i2c@7000d000";
 		i2c1 = "/i2c@7000c000";
 		i2c2 = "/i2c@7000c400";
 		i2c3 = "/i2c@7000c500";
+		sdmmc0 = "/sdhci@c8000600";
+		sdmmc1 = "/sdhci@c8000400";
 	};
 
 	memory {
@@ -156,13 +157,19 @@ 
 	};
 
 	sdhci@c8000400 {
-		cd-gpios = <&gpio 69 0>; /* gpio PI5 */
-		wp-gpios = <&gpio 57 0>; /* gpio PH1 */
-		power-gpios = <&gpio 70 0>; /* gpio PI6 */
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 69 0>;	/* card detect, gpio PI5 */
+		wp-gpios = <&gpio 57 0>;	/* write protect, gpio PH1 */
+		power-gpios = <&gpio 70 3>;	/* power enable, gpio PI6 */
+		bus-width = <4>;
+		removable = <1>;
 	};
 
 	sdhci@c8000600 {
-		support-8bit;
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
 	};
 
 	lcd_panel: panel {
diff --git a/board/nvidia/dts/tegra20-ventana.dts b/board/nvidia/dts/tegra20-ventana.dts
index 3e5e39d..fed27ab 100644
--- a/board/nvidia/dts/tegra20-ventana.dts
+++ b/board/nvidia/dts/tegra20-ventana.dts
@@ -41,4 +41,27 @@ 
 	usb@c5004000 {
 		status = "disabled";
 	};
+
+	sdhci@c8000000 {
+		status = "okay";
+		power-gpios = <&gpio 86 0>;	/* gpio PK6 */
+		bus-width = <4>;
+		removable = <0>;
+	};
+
+	sdhci@c8000400 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cd-gpios = <&gpio 69 0>;	/* gpio PI5 */
+		wp-gpios = <&gpio 57 0>;	/* gpio PH1 */
+		power-gpios = <&gpio 70 0>;	/* gpio PI6 */
+		bus-width = <4>;
+		removable = <1>;
+	};
+
+	sdhci@c8000600 {
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
+	};
 };
diff --git a/board/nvidia/dts/tegra20-whistler.dts b/board/nvidia/dts/tegra20-whistler.dts
index 4579557..ae198bd 100644
--- a/board/nvidia/dts/tegra20-whistler.dts
+++ b/board/nvidia/dts/tegra20-whistler.dts
@@ -9,6 +9,8 @@ 
 	aliases {
 		i2c0 = "/i2c@7000d000";
 		usb0 = "/usb@c5008000";
+		sdmmc0 = "/sdhci@c8000600";
+		sdmmc1 = "/sdhci@c8000400";
 	};
 
 	memory {
@@ -57,4 +59,18 @@ 
 	usb@c5004000 {
 		status = "disabled";
 	};
+
+	sdhci@c8000400 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		wp-gpios = <&gpio 173 0>;	/* gpio PV5 */
+		bus-width = <8>;
+		removable = <1>;
+	};
+
+	sdhci@c8000600 {
+		status = "okay";
+		bus-width = <8>;
+		removable = <0>;
+	};
 };
diff --git a/board/toradex/dts/tegra20-colibri_t20_iris.dts b/board/toradex/dts/tegra20-colibri_t20_iris.dts
index c29b43a..1689e1a 100644
--- a/board/toradex/dts/tegra20-colibri_t20_iris.dts
+++ b/board/toradex/dts/tegra20-colibri_t20_iris.dts
@@ -35,4 +35,12 @@ 
 			compatible = "nand-flash";
 		};
 	};
+
+	sdhci@c8000400 {
+		status = "okay";
+		/* Parameter 3 bit 0:1=output, 0=input; bit 1:1=high, 0=low */
+		cp-gpios = <&gpio 39 0>;	/* gpio PC7 */
+		bus-width = <4>;
+		removable = <1>;
+	};
 };