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

On Tue, Feb 12, 2013 at 07:51:55AM +0100, Thierry Reding wrote:
> On Mon, Feb 11, 2013 at 12:21:59PM -0700, Tom Warren wrote:
> > Thierry/Lucas,
> > 
> > On Mon, Feb 11, 2013 at 12:11 PM, Thierry Reding
> > <thierry.reding@avionic-design.de> wrote:
> > > On Mon, Feb 11, 2013 at 10:56:33AM -0700, Tom Warren wrote:
> > >> Lucas,
> > >>
> > >> On Mon, Feb 11, 2013 at 10:28 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > >> > Hi Tom,
> > >> >
> > >> > Am Montag, den 11.02.2013, 10:17 -0700 schrieb Tom Warren:
> > >> >> Linux dts files were used for those boards that didn't already
> > >> >> have sdhci info populated. Tamonten has their own dtsi file with
> > >> >> common sdhci nodes (sourced from Linux).
> > >> >>
> > >> >> Signed-off-by: Tom Warren <twarren@nvidia.com>
> > >> >> ---
> > >> >> v2:
> > >> >> - cleanup comments in dts files/match w/kernel files
> > >> >> - add sdhci aliases in all dts files
> > >> >> - use tegra20-tamonten.dtsi from the kernel for AD boards
> > >> >>
> > >> >>  arch/arm/dts/tegra20-tamonten.dtsi               |  489 ++++++++++++++++++++++
> > >> >
> > >> > I'm not sure if pushing the whole file in this patch is the right thing
> > >> > to do.
> > >>
> > >> I didn't want to edit it since we seem to be moving towards using the
> > >> Linux DTS files in toto in U-Boot (as per Stephen & Simon). Does it do
> > >> any harm to have the whole thing here? Saves some work later. Thierry
> > >> - what do you think?
> > >
> > > Given that it isn't used anywhere I don't think we really need it right
> > > now. We can always add it later when we can make better use of it.
> > 
> > It actually is used (for SDMMC/sdhci) now, Thierry. That's why it's in
> > this patchset.
> 
> Right, I hadn't looked at that patch yet.
> 
> > I had originally put the sdhci node for Avionic Design
> > boards in their respective .dts files, but Stephen pointed out that
> > the kernel had a tegra20-tamonten.dtsi file with common info, which
> > included the sdhci node, and asked that I use it, instead, so we echo
> > the kernel layout. So I pulled that file into my MMC DT patchset, and
> > used it in all AD board builds (medcom/tec/plutux) - it's pulled in
> > via an override of CONFIG_ARCH_DEVICE_TREE in the config files.
> > 
> > So the options seem to be:
> > 
> > a) Don't use the tamonton dtsi file, and put the sdhci nodes in the AD
> > dts files, just like all other boards (this was my V1 approach).
> > Vetoed by Stephen.
> > b) Use tegra20-tamonten.dtsi as is, identical to the kernel file. If
> > necessary, I can move it's inclusion to a separate patch, independent
> > of the MMC DT patchset, as suggested by Lucas.
> > c) Use tegra20-tamonten.dtsi, but just with the sdhci node (is this
> > what you're suggesting, Thierry?). I'd still pull it in via a
> > CONFIG_ARCH_DEVICE_TREE #define in the AD config files.
> > 
> > Let me know ASAP - I'd like to get V3 upstreamed soon so I can move on
> > to work on the T30/T114 MMC patches.
> 
> I think option b) sounds fine given that we want to move to the same DTS
> as the kernel eventually anyway. So for the Tamonten (and AD board)
> pieces, consider this:
> 
> 	Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> 
> I can't give you a Tested-by because I have a bunch of other things to
> take care of and I probably won't get to testing this for a few days.

So it turned out that I need to touch U-Boot anyway, so I decided to
give this a spin. I noticed that overriding CONFIG_ARCH_DEVICE_TREE from
the board configuration file doesn't work currently. What happens is
that the autoconf.mk (which is derived from the board configuration) is
included before the CPU config.mk which sets CONFIG_ARCH_DEVICE_TREE to
tegra20 (or tegra30, tegra114). I came up with the attached patch to set
the variable if not set previously (by the board configuration file).

Feel free to squash that in your patch series if you deem it a proper
solution. I can also provide a proper separate patch if you prefer.

Thierry

On Tue, Feb 12, 2013 at 11:41:09AM +0100, Thierry Reding wrote:
> On Tue, Feb 12, 2013 at 07:51:55AM +0100, Thierry Reding wrote:
> > On Mon, Feb 11, 2013 at 12:21:59PM -0700, Tom Warren wrote:
> > > Thierry/Lucas,
> > > 
> > > On Mon, Feb 11, 2013 at 12:11 PM, Thierry Reding
> > > <thierry.reding@avionic-design.de> wrote:
> > > > On Mon, Feb 11, 2013 at 10:56:33AM -0700, Tom Warren wrote:
> > > >> Lucas,
> > > >>
> > > >> On Mon, Feb 11, 2013 at 10:28 AM, Lucas Stach <dev@lynxeye.de> wrote:
> > > >> > Hi Tom,
> > > >> >
> > > >> > Am Montag, den 11.02.2013, 10:17 -0700 schrieb Tom Warren:
> > > >> >> Linux dts files were used for those boards that didn't already
> > > >> >> have sdhci info populated. Tamonten has their own dtsi file with
> > > >> >> common sdhci nodes (sourced from Linux).
> > > >> >>
> > > >> >> Signed-off-by: Tom Warren <twarren@nvidia.com>
> > > >> >> ---
> > > >> >> v2:
> > > >> >> - cleanup comments in dts files/match w/kernel files
> > > >> >> - add sdhci aliases in all dts files
> > > >> >> - use tegra20-tamonten.dtsi from the kernel for AD boards
> > > >> >>
> > > >> >>  arch/arm/dts/tegra20-tamonten.dtsi               |  489 ++++++++++++++++++++++
> > > >> >
> > > >> > I'm not sure if pushing the whole file in this patch is the right thing
> > > >> > to do.
> > > >>
> > > >> I didn't want to edit it since we seem to be moving towards using the
> > > >> Linux DTS files in toto in U-Boot (as per Stephen & Simon). Does it do
> > > >> any harm to have the whole thing here? Saves some work later. Thierry
> > > >> - what do you think?
> > > >
> > > > Given that it isn't used anywhere I don't think we really need it right
> > > > now. We can always add it later when we can make better use of it.
> > > 
> > > It actually is used (for SDMMC/sdhci) now, Thierry. That's why it's in
> > > this patchset.
> > 
> > Right, I hadn't looked at that patch yet.
> > 
> > > I had originally put the sdhci node for Avionic Design
> > > boards in their respective .dts files, but Stephen pointed out that
> > > the kernel had a tegra20-tamonten.dtsi file with common info, which
> > > included the sdhci node, and asked that I use it, instead, so we echo
> > > the kernel layout. So I pulled that file into my MMC DT patchset, and
> > > used it in all AD board builds (medcom/tec/plutux) - it's pulled in
> > > via an override of CONFIG_ARCH_DEVICE_TREE in the config files.
> > > 
> > > So the options seem to be:
> > > 
> > > a) Don't use the tamonton dtsi file, and put the sdhci nodes in the AD
> > > dts files, just like all other boards (this was my V1 approach).
> > > Vetoed by Stephen.
> > > b) Use tegra20-tamonten.dtsi as is, identical to the kernel file. If
> > > necessary, I can move it's inclusion to a separate patch, independent
> > > of the MMC DT patchset, as suggested by Lucas.
> > > c) Use tegra20-tamonten.dtsi, but just with the sdhci node (is this
> > > what you're suggesting, Thierry?). I'd still pull it in via a
> > > CONFIG_ARCH_DEVICE_TREE #define in the AD config files.
> > > 
> > > Let me know ASAP - I'd like to get V3 upstreamed soon so I can move on
> > > to work on the T30/T114 MMC patches.
> > 
> > I think option b) sounds fine given that we want to move to the same DTS
> > as the kernel eventually anyway. So for the Tamonten (and AD board)
> > pieces, consider this:
> > 
> > 	Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> > 
> > I can't give you a Tested-by because I have a bunch of other things to
> > take care of and I probably won't get to testing this for a few days.
> 
> So it turned out that I need to touch U-Boot anyway, so I decided to
> give this a spin. I noticed that overriding CONFIG_ARCH_DEVICE_TREE from
> the board configuration file doesn't work currently. What happens is
> that the autoconf.mk (which is derived from the board configuration) is
> included before the CPU config.mk which sets CONFIG_ARCH_DEVICE_TREE to
> tegra20 (or tegra30, tegra114). I came up with the attached patch to set
> the variable if not set previously (by the board configuration file).
> 
> Feel free to squash that in your patch series if you deem it a proper
> solution. I can also provide a proper separate patch if you prefer.
> 
> Thierry

> diff --git a/arch/arm/cpu/armv7/tegra114/config.mk b/arch/arm/cpu/armv7/tegra114/config.mk
> index cb1a19d..e7c22c0 100644
> --- a/arch/arm/cpu/armv7/tegra114/config.mk
> +++ b/arch/arm/cpu/armv7/tegra114/config.mk
> @@ -16,4 +16,4 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  #
> -CONFIG_ARCH_DEVICE_TREE := tegra114
> +CONFIG_ARCH_DEVICE_TREE ?= tegra114
> diff --git a/arch/arm/cpu/armv7/tegra20/config.mk b/arch/arm/cpu/armv7/tegra20/config.mk
> index 6432e75..9042664 100644
> --- a/arch/arm/cpu/armv7/tegra20/config.mk
> +++ b/arch/arm/cpu/armv7/tegra20/config.mk
> @@ -23,4 +23,4 @@
>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>  # MA 02111-1307 USA
>  #
> -CONFIG_ARCH_DEVICE_TREE := tegra20
> +CONFIG_ARCH_DEVICE_TREE ?= tegra20
> diff --git a/arch/arm/cpu/armv7/tegra30/config.mk b/arch/arm/cpu/armv7/tegra30/config.mk
> index 719ca81..0035bc5 100644
> --- a/arch/arm/cpu/armv7/tegra30/config.mk
> +++ b/arch/arm/cpu/armv7/tegra30/config.mk
> @@ -16,4 +16,4 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  #
> -CONFIG_ARCH_DEVICE_TREE := tegra30
> +CONFIG_ARCH_DEVICE_TREE ?= tegra30

I forgot: with that patch applied:

	Tested-by: Thierry Reding <thierry.reding@avionic-design.de>

Thierry

Thierry,

On Tue, Feb 12, 2013 at 3:53 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Tue, Feb 12, 2013 at 11:41:09AM +0100, Thierry Reding wrote:
>> On Tue, Feb 12, 2013 at 07:51:55AM +0100, Thierry Reding wrote:
>> > On Mon, Feb 11, 2013 at 12:21:59PM -0700, Tom Warren wrote:
>> > > Thierry/Lucas,
>> > >
>> > > On Mon, Feb 11, 2013 at 12:11 PM, Thierry Reding
>> > > <thierry.reding@avionic-design.de> wrote:
>> > > > On Mon, Feb 11, 2013 at 10:56:33AM -0700, Tom Warren wrote:
>> > > >> Lucas,
>> > > >>
>> > > >> On Mon, Feb 11, 2013 at 10:28 AM, Lucas Stach <dev@lynxeye.de> wrote:
>> > > >> > Hi Tom,
>> > > >> >
>> > > >> > Am Montag, den 11.02.2013, 10:17 -0700 schrieb Tom Warren:
>> > > >> >> Linux dts files were used for those boards that didn't already
>> > > >> >> have sdhci info populated. Tamonten has their own dtsi file with
>> > > >> >> common sdhci nodes (sourced from Linux).
>> > > >> >>
>> > > >> >> Signed-off-by: Tom Warren <twarren@nvidia.com>
>> > > >> >> ---
>> > > >> >> v2:
>> > > >> >> - cleanup comments in dts files/match w/kernel files
>> > > >> >> - add sdhci aliases in all dts files
>> > > >> >> - use tegra20-tamonten.dtsi from the kernel for AD boards
>> > > >> >>
>> > > >> >>  arch/arm/dts/tegra20-tamonten.dtsi               |  489 ++++++++++++++++++++++
>> > > >> >
>> > > >> > I'm not sure if pushing the whole file in this patch is the right thing
>> > > >> > to do.
>> > > >>
>> > > >> I didn't want to edit it since we seem to be moving towards using the
>> > > >> Linux DTS files in toto in U-Boot (as per Stephen & Simon). Does it do
>> > > >> any harm to have the whole thing here? Saves some work later. Thierry
>> > > >> - what do you think?
>> > > >
>> > > > Given that it isn't used anywhere I don't think we really need it right
>> > > > now. We can always add it later when we can make better use of it.
>> > >
>> > > It actually is used (for SDMMC/sdhci) now, Thierry. That's why it's in
>> > > this patchset.
>> >
>> > Right, I hadn't looked at that patch yet.
>> >
>> > > I had originally put the sdhci node for Avionic Design
>> > > boards in their respective .dts files, but Stephen pointed out that
>> > > the kernel had a tegra20-tamonten.dtsi file with common info, which
>> > > included the sdhci node, and asked that I use it, instead, so we echo
>> > > the kernel layout. So I pulled that file into my MMC DT patchset, and
>> > > used it in all AD board builds (medcom/tec/plutux) - it's pulled in
>> > > via an override of CONFIG_ARCH_DEVICE_TREE in the config files.
>> > >
>> > > So the options seem to be:
>> > >
>> > > a) Don't use the tamonton dtsi file, and put the sdhci nodes in the AD
>> > > dts files, just like all other boards (this was my V1 approach).
>> > > Vetoed by Stephen.
>> > > b) Use tegra20-tamonten.dtsi as is, identical to the kernel file. If
>> > > necessary, I can move it's inclusion to a separate patch, independent
>> > > of the MMC DT patchset, as suggested by Lucas.
>> > > c) Use tegra20-tamonten.dtsi, but just with the sdhci node (is this
>> > > what you're suggesting, Thierry?). I'd still pull it in via a
>> > > CONFIG_ARCH_DEVICE_TREE #define in the AD config files.
>> > >
>> > > Let me know ASAP - I'd like to get V3 upstreamed soon so I can move on
>> > > to work on the T30/T114 MMC patches.
>> >
>> > I think option b) sounds fine given that we want to move to the same DTS
>> > as the kernel eventually anyway. So for the Tamonten (and AD board)
>> > pieces, consider this:
>> >
>> >     Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
>> >
>> > I can't give you a Tested-by because I have a bunch of other things to
>> > take care of and I probably won't get to testing this for a few days.
>>
>> So it turned out that I need to touch U-Boot anyway, so I decided to
>> give this a spin. I noticed that overriding CONFIG_ARCH_DEVICE_TREE from
>> the board configuration file doesn't work currently. What happens is
>> that the autoconf.mk (which is derived from the board configuration) is
>> included before the CPU config.mk which sets CONFIG_ARCH_DEVICE_TREE to
>> tegra20 (or tegra30, tegra114). I came up with the attached patch to set
>> the variable if not set previously (by the board configuration file).
>>
>> Feel free to squash that in your patch series if you deem it a proper
>> solution. I can also provide a proper separate patch if you prefer.
>>
>> Thierry
>
>> diff --git a/arch/arm/cpu/armv7/tegra114/config.mk b/arch/arm/cpu/armv7/tegra114/config.mk
>> index cb1a19d..e7c22c0 100644
>> --- a/arch/arm/cpu/armv7/tegra114/config.mk
>> +++ b/arch/arm/cpu/armv7/tegra114/config.mk
>> @@ -16,4 +16,4 @@
>>  # You should have received a copy of the GNU General Public License
>>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>  #
>> -CONFIG_ARCH_DEVICE_TREE := tegra114
>> +CONFIG_ARCH_DEVICE_TREE ?= tegra114
>> diff --git a/arch/arm/cpu/armv7/tegra20/config.mk b/arch/arm/cpu/armv7/tegra20/config.mk
>> index 6432e75..9042664 100644
>> --- a/arch/arm/cpu/armv7/tegra20/config.mk
>> +++ b/arch/arm/cpu/armv7/tegra20/config.mk
>> @@ -23,4 +23,4 @@
>>  # Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>  # MA 02111-1307 USA
>>  #
>> -CONFIG_ARCH_DEVICE_TREE := tegra20
>> +CONFIG_ARCH_DEVICE_TREE ?= tegra20
>> diff --git a/arch/arm/cpu/armv7/tegra30/config.mk b/arch/arm/cpu/armv7/tegra30/config.mk
>> index 719ca81..0035bc5 100644
>> --- a/arch/arm/cpu/armv7/tegra30/config.mk
>> +++ b/arch/arm/cpu/armv7/tegra30/config.mk
>> @@ -16,4 +16,4 @@
>>  # You should have received a copy of the GNU General Public License
>>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>  #
>> -CONFIG_ARCH_DEVICE_TREE := tegra30
>> +CONFIG_ARCH_DEVICE_TREE ?= tegra30
>
> I forgot: with that patch applied:
>
>         Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
>
> Thierry
Thanks for the patch - I never would have seen that, since it all
built OK w/MAKEALL.

Tom

On 02/12/2013 03:41 AM, Thierry Reding wrote:
...
> So it turned out that I need to touch U-Boot anyway, so I decided
> to give this a spin. I noticed that overriding
> CONFIG_ARCH_DEVICE_TREE from the board configuration file doesn't
> work currently. What happens is that the autoconf.mk (which is
> derived from the board configuration) is included before the CPU
> config.mk which sets CONFIG_ARCH_DEVICE_TREE to tegra20 (or
> tegra30, tegra114). I came up with the attached patch to set the
> variable if not set previously (by the board configuration file).
> 
> Feel free to squash that in your patch series if you deem it a
> proper solution. I can also provide a proper separate patch if you
> prefer.

> diff --git a/arch/arm/cpu/armv7/tegra114/config.mk
> b/arch/arm/cpu/armv7/tegra114/config.mk

> -CONFIG_ARCH_DEVICE_TREE := tegra114 +CONFIG_ARCH_DEVICE_TREE ?=
> tegra114

That looks very odd. What value is CONFIG_ARCH_DEVICE_TREE before that
assignment, and why exactly is it wrong?

On Tue, Feb 12, 2013 at 01:19:18PM -0700, Stephen Warren wrote:
> On 02/12/2013 03:41 AM, Thierry Reding wrote:
> ...
> > So it turned out that I need to touch U-Boot anyway, so I decided
> > to give this a spin. I noticed that overriding
> > CONFIG_ARCH_DEVICE_TREE from the board configuration file doesn't
> > work currently. What happens is that the autoconf.mk (which is
> > derived from the board configuration) is included before the CPU
> > config.mk which sets CONFIG_ARCH_DEVICE_TREE to tegra20 (or
> > tegra30, tegra114). I came up with the attached patch to set the
> > variable if not set previously (by the board configuration file).
> > 
> > Feel free to squash that in your patch series if you deem it a
> > proper solution. I can also provide a proper separate patch if you
> > prefer.
> 
> > diff --git a/arch/arm/cpu/armv7/tegra114/config.mk
> > b/arch/arm/cpu/armv7/tegra114/config.mk
> 
> > -CONFIG_ARCH_DEVICE_TREE := tegra114 +CONFIG_ARCH_DEVICE_TREE ?=
> > tegra114
> 
> That looks very odd. What value is CONFIG_ARCH_DEVICE_TREE before that
> assignment, and why exactly is it wrong?

Tom's patches add a

	#define CONFIG_ARCH_DEVICE_TREE tegra20-tamonten

to the configuration files of Tamonten-derived boards so that the proper
DTSI is picked up. However, that line causes the make variable to be
defined in autoconf.mk, which is included before the CPU config.mk, so
if the config.mk has

	CONFIG_ARCH_DEVICE_TREE := tegra20

it will overwrite the value set by autoconf.mk.

Thierry