[RFC 0/2] Tegra Harmony: Device Tree Initial Steps

[RFC 0/2] Tegra Harmony: Device Tree Initial Steps

[John Bonesio]

These patches are the first steps towards getting the board specific devices on the Tegra
Harmony board to be discovered and initialized using only a device tree -
rather than platform specific code. In particular, this patch set is getting ready to
initialize i2c devices based solely on a device tree.

This patch series is presented at this time only for review and comments.

---

John Bonesio (2):
      ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized
      ARM: Tegra: Device Tree Support: Add i2c devices


 arch/arm/boot/dts/tegra250.dtsi |   33 +++++++++++
 arch/arm/mach-tegra/Makefile    |    1 
 arch/arm/mach-tegra/board-dt.c  |  120 ++++++++++++++++-----------------------
 drivers/i2c/busses/i2c-tegra.c  |    4 +
 4 files changed, 87 insertions(+), 71 deletions(-)

-- 
- John

[RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[John Bonesio]

This patch changes how sdhci devices are initialized so that a later patch can
easily add support for i2c devies.

Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 arch/arm/mach-tegra/Makefile   |    1 
 arch/arm/mach-tegra/board-dt.c |  106 ++++++++++------------------------------
 2 files changed, 27 insertions(+), 80 deletions(-)

diff --git a/arch/arm/mach-tegra/Makefile b/arch/arm/mach-tegra/Makefile
index 842ec1a..54d2215 100644
--- a/arch/arm/mach-tegra/Makefile
+++ b/arch/arm/mach-tegra/Makefile
@@ -31,6 +31,7 @@ obj-${CONFIG_MACH_SEABOARD}             += board-seaboard.o
 obj-${CONFIG_MACH_SEABOARD}             += board-seaboard-pinmux.o
 
 obj-${CONFIG_MACH_TEGRA_DT}             += board-dt.o
+obj-${CONFIG_MACH_TEGRA_DT}             += board-harmony-pinmux.o
 
 obj-${CONFIG_MACH_TRIMSLICE}            += board-trimslice.o
 obj-${CONFIG_MACH_TRIMSLICE}            += board-trimslice-pinmux.o
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index 9a35741..e61d563 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -36,90 +36,13 @@
 
 #include <mach/iomap.h>
 #include <mach/irqs.h>
+#include <mach/sdhci.h>
 
 #include "board.h"
 #include "board-harmony.h"
 #include "clock.h"
-
-static struct resource sdhci_resource1[] = {
-	[0] = {
-		.start  = INT_SDMMC1,
-		.end    = INT_SDMMC1,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC1_BASE,
-		.end	= TEGRA_SDMMC1_BASE + TEGRA_SDMMC1_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device1 = {
-	.name		= "sdhci-tegra",
-	.id		= 0,
-	.resource	= sdhci_resource1,
-	.num_resources	= ARRAY_SIZE(sdhci_resource1),
-};
-
-static struct resource sdhci_resource2[] = {
-	[0] = {
-		.start  = INT_SDMMC2,
-		.end    = INT_SDMMC2,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC2_BASE,
-		.end	= TEGRA_SDMMC2_BASE + TEGRA_SDMMC2_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device2 = {
-	.name		= "sdhci-tegra",
-	.id		= 1,
-	.resource	= sdhci_resource2,
-	.num_resources	= ARRAY_SIZE(sdhci_resource2),
-};
-
-static struct resource sdhci_resource3[] = {
-	[0] = {
-		.start  = INT_SDMMC3,
-		.end    = INT_SDMMC3,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC3_BASE,
-		.end	= TEGRA_SDMMC3_BASE + TEGRA_SDMMC3_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device3 = {
-	.name		= "sdhci-tegra",
-	.id		= 2,
-	.resource	= sdhci_resource3,
-	.num_resources	= ARRAY_SIZE(sdhci_resource3),
-};
-
-static struct resource sdhci_resource4[] = {
-	[0] = {
-		.start  = INT_SDMMC4,
-		.end    = INT_SDMMC4,
-		.flags  = IORESOURCE_IRQ,
-	},
-	[1] = {
-		.start	= TEGRA_SDMMC4_BASE,
-		.end	= TEGRA_SDMMC4_BASE + TEGRA_SDMMC4_SIZE-1,
-		.flags	= IORESOURCE_MEM,
-	},
-};
-
-static struct platform_device tegra_sdhci_device4 = {
-	.name		= "sdhci-tegra",
-	.id		= 3,
-	.resource	= sdhci_resource4,
-	.num_resources	= ARRAY_SIZE(sdhci_resource4),
-};
+#include "devices.h"
+#include "gpio-names.h"
 
 static struct platform_device *harmony_devices[] __initdata = {
 	&tegra_sdhci_device1,
@@ -134,6 +57,25 @@ static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
 	{ NULL,		NULL,		0,		0},
 };
 
+static struct tegra_sdhci_platform_data sdhci_pdata1 = {
+	.cd_gpio        = -1,
+	.wp_gpio        = -1,
+	.power_gpio     = -1,
+};
+
+static struct tegra_sdhci_platform_data sdhci_pdata2 = {
+	.cd_gpio        = TEGRA_GPIO_SD2_CD,
+	.wp_gpio        = TEGRA_GPIO_SD2_WP,
+	.power_gpio     = TEGRA_GPIO_SD2_POWER,
+};
+
+static struct tegra_sdhci_platform_data sdhci_pdata4 = {
+	.cd_gpio        = TEGRA_GPIO_SD4_CD,
+	.wp_gpio        = TEGRA_GPIO_SD4_WP,
+	.power_gpio     = TEGRA_GPIO_SD4_POWER,
+	.is_8bit        = 1,
+};
+
 static struct of_device_id tegra_dt_match_table[] __initdata = {
 	{ .compatible = "simple-bus", },
 	{}
@@ -164,6 +106,10 @@ static void __init tegra_dt_init(void)
 
 	harmony_pinmux_init();
 
+	tegra_sdhci_device1.dev.platform_data = &sdhci_pdata1;
+	tegra_sdhci_device2.dev.platform_data = &sdhci_pdata2;
+	tegra_sdhci_device4.dev.platform_data = &sdhci_pdata4;
+
 	platform_add_devices(harmony_devices, ARRAY_SIZE(harmony_devices));
 
 	/*

RE: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Stephen Warren]

John Bonesio wrote at Tuesday, May 10, 2011 2:14 PM:
> This patch changes how sdhci devices are initialized so that a later patch can
> easily add support for i2c devies.
> 
> Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

> @@ -134,6 +57,25 @@ static __initdata struct tegra_clk_init_table
> tegra_dt_clk_init_table[] = {
>  	{ NULL,		NULL,		0,		0},
>  };
> 
> +static struct tegra_sdhci_platform_data sdhci_pdata1 = {
> +	.cd_gpio        = -1,
> +	.wp_gpio        = -1,
> +	.power_gpio     = -1,
> +};
> +
> +static struct tegra_sdhci_platform_data sdhci_pdata2 = {
> +	.cd_gpio        = TEGRA_GPIO_SD2_CD,
> +	.wp_gpio        = TEGRA_GPIO_SD2_WP,
> +	.power_gpio     = TEGRA_GPIO_SD2_POWER,
> +};
> +
> +static struct tegra_sdhci_platform_data sdhci_pdata4 = {
> +	.cd_gpio        = TEGRA_GPIO_SD4_CD,
> +	.wp_gpio        = TEGRA_GPIO_SD4_WP,
> +	.power_gpio     = TEGRA_GPIO_SD4_POWER,
> +	.is_8bit        = 1,
> +};
> +

So I understand why board-dt is itself registering the SDHCI platform
devices itself; so that the pdev initialization path is identical
between fully hard-coded boards and board-dt.c.

However, I'm surprised that we would include the platform data for
those devices within that argument; if we do that, we'll end up needing
different platform data for every board, and devicetree won't have
bought us anything.

In other words, I understood that board-dt would register all the
platform devices within Tegra, but not set up any of the plataform
data, and the device drivers for those devices would be updated to
get their platform data from devicetree if available, then fall back
to old-style platform data. In other words, the way the SDHCI drivers
work today in the devicetree/test branch.

If my understanding is correct, shouldn't the chunk above and the
one below be removed?

>  static struct of_device_id tegra_dt_match_table[] __initdata = {
>  	{ .compatible = "simple-bus", },
>  	{}
> @@ -164,6 +106,10 @@ static void __init tegra_dt_init(void)
> 
>  	harmony_pinmux_init();
> 
> +	tegra_sdhci_device1.dev.platform_data = &sdhci_pdata1;
> +	tegra_sdhci_device2.dev.platform_data = &sdhci_pdata2;
> +	tegra_sdhci_device4.dev.platform_data = &sdhci_pdata4;
> +
>  	platform_add_devices(harmony_devices, ARRAY_SIZE(harmony_devices));
> 
>  	/*

-- 
nvpublic

Re: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Olof Johansson]

[-- Attachment #1.1: Type: text/plain, Size: 2381 bytes --]

On Tue, May 10, 2011 at 3:03 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> John Bonesio wrote at Tuesday, May 10, 2011 2:14 PM:
> > This patch changes how sdhci devices are initialized so that a later
> patch can
> > easily add support for i2c devies.
> >
> > Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>
> > @@ -134,6 +57,25 @@ static __initdata struct tegra_clk_init_table
> > tegra_dt_clk_init_table[] = {
> >       { NULL,         NULL,           0,              0},
> >  };
> >
> > +static struct tegra_sdhci_platform_data sdhci_pdata1 = {
> > +     .cd_gpio        = -1,
> > +     .wp_gpio        = -1,
> > +     .power_gpio     = -1,
> > +};
> > +
> > +static struct tegra_sdhci_platform_data sdhci_pdata2 = {
> > +     .cd_gpio        = TEGRA_GPIO_SD2_CD,
> > +     .wp_gpio        = TEGRA_GPIO_SD2_WP,
> > +     .power_gpio     = TEGRA_GPIO_SD2_POWER,
> > +};
> > +
> > +static struct tegra_sdhci_platform_data sdhci_pdata4 = {
> > +     .cd_gpio        = TEGRA_GPIO_SD4_CD,
> > +     .wp_gpio        = TEGRA_GPIO_SD4_WP,
> > +     .power_gpio     = TEGRA_GPIO_SD4_POWER,
> > +     .is_8bit        = 1,
> > +};
> > +
>
> So I understand why board-dt is itself registering the SDHCI platform
> devices itself; so that the pdev initialization path is identical
> between fully hard-coded boards and board-dt.c.
>
> However, I'm surprised that we would include the platform data for
> those devices within that argument; if we do that, we'll end up needing
> different platform data for every board, and devicetree won't have
> bought us anything.
>
> In other words, I understood that board-dt would register all the
> platform devices within Tegra, but not set up any of the plataform
> data, and the device drivers for those devices would be updated to
> get their platform data from devicetree if available, then fall back
> to old-style platform data. In other words, the way the SDHCI drivers
> work today in the devicetree/test branch.
>
> If my understanding is correct, shouldn't the chunk above and the
> one below be removed?
>

That's if we cut everything over, I think. until then, this code should fill
in the platform_device and register it. But the values should come from the
device tree, and it should only register the controllers that are in the
tree (and not marked as disabled).


-Olof

[-- Attachment #1.2: Type: text/html, Size: 3049 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

RE: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Stephen Warren]

Olof Johansson wrote at Tuesday, May 10, 2011 4:07 PM:
> > On Tue, May 10, 2011 at 3:03 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > John Bonesio wrote at Tuesday, May 10, 2011 2:14 PM:
> > > This patch changes how sdhci devices are initialized so that a later
> > > patch can easily add support for i2c devies.
> > >
> > > +static struct tegra_sdhci_platform_data sdhci_pdata1 = {
> > > +     .cd_gpio        = -1,
> > > +     .wp_gpio        = -1,
> > > +     .power_gpio     = -1,
> > > +};
> > > +
> > So I understand why board-dt is itself registering the SDHCI platform
> > devices itself; so that the pdev initialization path is identical
> > between fully hard-coded boards and board-dt.c.
> > 
> > However, I'm surprised that we would include the platform data for
> > those devices within that argument; if we do that, we'll end up needing
> > different platform data for every board, and devicetree won't have
> > bought us anything.
> > 
> > In other words, I understood that board-dt would register all the
> > platform devices within Tegra, but not set up any of the plataform
> > data, and the device drivers for those devices would be updated to
> > get their platform data from devicetree if available, then fall back
> > to old-style platform data. In other words, the way the SDHCI drivers
> > work today in the devicetree/test branch.
> > 
> > If my understanding is correct, shouldn't the chunk above and the
> > one below be removed?
>
> That's if we cut everything over, I think. until then, this code should
> fill in the platform_device and register it. But the values should come
> from the device tree, and it should only register the controllers that are
> in the tree (and not marked as disabled).

But doesn't that require board-dt.c to walk and parse the devicetree,
extract the platform data from the DT, and fill in the platform data
structures? I thought we'd explicitly talked about that and decided that
was a bad idea, since it'd duplicate each driver's DT parsing into
board-dt.c?

-- 
nvpublic

Re: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[John Bonesio]

On 05/10/2011 03:16 PM, Stephen Warren wrote:
> Olof Johansson wrote at Tuesday, May 10, 2011 4:07 PM:
>>> On Tue, May 10, 2011 at 3:03 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>> John Bonesio wrote at Tuesday, May 10, 2011 2:14 PM:
>>>> This patch changes how sdhci devices are initialized so that a later
>>>> patch can easily add support for i2c devies.
>>>>
>>>> +static struct tegra_sdhci_platform_data sdhci_pdata1 = {
>>>> +     .cd_gpio        = -1,
>>>> +     .wp_gpio        = -1,
>>>> +     .power_gpio     = -1,
>>>> +};
>>>> +
>>> So I understand why board-dt is itself registering the SDHCI platform
>>> devices itself; so that the pdev initialization path is identical
>>> between fully hard-coded boards and board-dt.c.
>>>
>>> However, I'm surprised that we would include the platform data for
>>> those devices within that argument; if we do that, we'll end up needing
>>> different platform data for every board, and devicetree won't have
>>> bought us anything.
>>>
>>> In other words, I understood that board-dt would register all the
>>> platform devices within Tegra, but not set up any of the plataform
>>> data, and the device drivers for those devices would be updated to
>>> get their platform data from devicetree if available, then fall back
>>> to old-style platform data. In other words, the way the SDHCI drivers
>>> work today in the devicetree/test branch.
>>>
>>> If my understanding is correct, shouldn't the chunk above and the
>>> one below be removed?
>>
>> That's if we cut everything over, I think. until then, this code should
>> fill in the platform_device and register it. But the values should come
>> from the device tree, and it should only register the controllers that are
>> in the tree (and not marked as disabled).
> 
> But doesn't that require board-dt.c to walk and parse the devicetree,
> extract the platform data from the DT, and fill in the platform data
> structures? I thought we'd explicitly talked about that and decided that
> was a bad idea, since it'd duplicate each driver's DT parsing into
> board-dt.c?
> 

The goal I'm working towards right now is to have the platform specific
code continue to register the devices that are inside the SoC. Then use
the device tree to register all other on board devices.

This patch doesn't accomplish the whole goal in one step, but gets the
code a little closer toward this goal.

To accomplish the goal, nodes need to be in the device tree for the
devices inside the SoC. This is then used to attach device tree nodes to
the existing devices - gives us a "hook" on which to "hang" the board
specific devices. There is only one device tree used, so everything
needs to be connected into the same device tree.

These patches just get the i2c controller and driver instantiated. This
includes having the platform code to register the i2c controller in the
"normal" fashion. This patch also gets a device tree node attached to
the i2c device.

A following patch, which I'll send later this week/early next week, will
instantiate the wm8903 audio codec device solely from the device tree.

Hopefully this makes the plan a little clearer. Let me know if you have
any questions.

- John

Re: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Mark Brown]

On Tue, May 10, 2011 at 03:33:29PM -0700, John Bonesio wrote:

> A following patch, which I'll send later this week/early next week, will
> instantiate the wm8903 audio codec device solely from the device tree.

Please CC me on this work - I've got an interest both from the chip and
subsystem point of view in how this stuff ends up looking and there's
some fun and games with audio in device tree.

RE: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Stephen Warren]

John Bonesio wrote at Tuesday, May 10, 2011 4:33 PM:
> The goal I'm working towards right now is to have the platform specific
> code continue to register the devices that are inside the SoC. Then use
> the device tree to register all other on board devices.

Sure, that sounds right.

Which branch are your patches aimed at? My discussions are all based
on my experience with Grant's devicetree/test branch.

> This patch doesn't accomplish the whole goal in one step, but gets the
> code a little closer toward this goal.

So what's confusing me is that I think the devicetree/test branch is
already there; board-dt.c registers all 4 SDHCI controllers,
tegra250.dtsi defines each controller's memory map etc., and disables
them all, then tegra-harmony.dts and tegra-seaboard.dts enable the
relevant subset of those controllers, and provides the required
platform data for them.

When adding the I2C controllers, it seems like they'd just work the
same way as the existing SDHCI code.

So, all this already works without putting the board-specific platform
data definitions into board-dt.c as an temporary measure.

-- 
nvpublic

Re: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[John Bonesio]

On 05/10/2011 04:22 PM, Stephen Warren wrote:
> John Bonesio wrote at Tuesday, May 10, 2011 4:33 PM:
>> The goal I'm working towards right now is to have the platform specific
>> code continue to register the devices that are inside the SoC. Then use
>> the device tree to register all other on board devices.
> 
> Sure, that sounds right.
> 
> Which branch are your patches aimed at? My discussions are all based
> on my experience with Grant's devicetree/test branch.
> 
>> This patch doesn't accomplish the whole goal in one step, but gets the
>> code a little closer toward this goal.
> 
> So what's confusing me is that I think the devicetree/test branch is
> already there; board-dt.c registers all 4 SDHCI controllers,
> tegra250.dtsi defines each controller's memory map etc., and disables
> them all, then tegra-harmony.dts and tegra-seaboard.dts enable the
> relevant subset of those controllers, and provides the required
> platform data for them.
> 
> When adding the I2C controllers, it seems like they'd just work the
> same way as the existing SDHCI code.

>From a philosophical point of view, they should be initialized the same way.

Are you thinking that the i2c controller should be in the
harmony_devices array and initialized with the other devices? If so i2c
is different in the kernel. An i2c controller isn't registered with
platform_device_register().

I may not be understanding your question.

> 
> So, all this already works without putting the board-specific platform
> data definitions into board-dt.c as an temporary measure.
> 

RE: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Stephen Warren]

John Bonesio wrote at Tuesday, May 10, 2011 5:33 PM:
> On 05/10/2011 04:22 PM, Stephen Warren wrote:
> > John Bonesio wrote at Tuesday, May 10, 2011 4:33 PM:
> >> The goal I'm working towards right now is to have the platform specific
> >> code continue to register the devices that are inside the SoC. Then use
> >> the device tree to register all other on board devices.
> >
> > Sure, that sounds right.
> >
> > Which branch are your patches aimed at? My discussions are all based
> > on my experience with Grant's devicetree/test branch.
> >
> >> This patch doesn't accomplish the whole goal in one step, but gets the
> >> code a little closer toward this goal.
> >
> > So what's confusing me is that I think the devicetree/test branch is
> > already there; board-dt.c registers all 4 SDHCI controllers,
> > tegra250.dtsi defines each controller's memory map etc., and disables
> > them all, then tegra-harmony.dts and tegra-seaboard.dts enable the
> > relevant subset of those controllers, and provides the required
> > platform data for them.
> >
> > When adding the I2C controllers, it seems like they'd just work the
> > same way as the existing SDHCI code.
> 
> From a philosophical point of view, they should be initialized the same
> way.
> 
> Are you thinking that the i2c controller should be in the
> harmony_devices array and initialized with the other devices? If so i2c
> is different in the kernel. An i2c controller isn't registered with
> platform_device_register().

Um. Your patch (and all existing code I'm familiar with) *does* use
platform_device_register for it:

+	platform_device_register(&tegra_i2c_device1);

But, I'm not talking about registering the platform devices, i.e. I'm
not talking about the line of code quoted above. I'm talking about the
platform data setup for those devices, i.e. the following from your
SDHCI patch:

+static struct tegra_sdhci_platform_data sdhci_pdata1 = {
+	.cd_gpio        = -1,
+	.wp_gpio        = -1,
+	.power_gpio     = -1,
+};
...
+	tegra_sdhci_device1.dev.platform_data = &sdhci_pdata1;

The SDHCI driver has already been updated to get this platform data
from the OF node in the platform device; it doesn't need hard-coded
platform data to be passed to it.

Similarly, the following from your I2C patch shouldn't be required:

+static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
+	.bus_clk_rate   = 400000,
+};
...
+	tegra_i2c_device1.dev.platform_data = &harmony_i2c1_platform_data;

Instead, the I2C driver should be modified to get its data from the
OF node in the platform device just like the SDHCI driver does. This
should be a pretty trivial amount of code to add to the I2C patch.

> I may not be understanding your question.
> 
> >
> > So, all this already works without putting the board-specific platform
> > data definitions into board-dt.c as an temporary measure.
> >

-- 
nvpublic

Re: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[John Bonesio]

On 05/10/2011 10:01 PM, Stephen Warren wrote:
> John Bonesio wrote at Tuesday, May 10, 2011 5:33 PM:
>> On 05/10/2011 04:22 PM, Stephen Warren wrote:
>>> John Bonesio wrote at Tuesday, May 10, 2011 4:33 PM:
>>>> The goal I'm working towards right now is to have the platform specific
>>>> code continue to register the devices that are inside the SoC. Then use
>>>> the device tree to register all other on board devices.
>>>
>>> Sure, that sounds right.
>>>
>>> Which branch are your patches aimed at? My discussions are all based
>>> on my experience with Grant's devicetree/test branch.
>>>
>>>> This patch doesn't accomplish the whole goal in one step, but gets the
>>>> code a little closer toward this goal.
>>>
>>> So what's confusing me is that I think the devicetree/test branch is
>>> already there; board-dt.c registers all 4 SDHCI controllers,
>>> tegra250.dtsi defines each controller's memory map etc., and disables
>>> them all, then tegra-harmony.dts and tegra-seaboard.dts enable the
>>> relevant subset of those controllers, and provides the required
>>> platform data for them.
>>>
>>> When adding the I2C controllers, it seems like they'd just work the
>>> same way as the existing SDHCI code.
>>
>> From a philosophical point of view, they should be initialized the same
>> way.
>>
>> Are you thinking that the i2c controller should be in the
>> harmony_devices array and initialized with the other devices? If so i2c
>> is different in the kernel. An i2c controller isn't registered with
>> platform_device_register().
> 
> Um. Your patch (and all existing code I'm familiar with) *does* use
> platform_device_register for it:

oops. Right. I was confusing this with the way i2c devices are
registered. I copied the way i2c was initialized in board-harmony.c

> 
> +	platform_device_register(&tegra_i2c_device1);
> 
> But, I'm not talking about registering the platform devices, i.e. I'm
> not talking about the line of code quoted above. I'm talking about the
> platform data setup for those devices, i.e. the following from your
> SDHCI patch:
> 
> +static struct tegra_sdhci_platform_data sdhci_pdata1 = {
> +	.cd_gpio        = -1,
> +	.wp_gpio        = -1,
> +	.power_gpio     = -1,
> +};
> ...
> +	tegra_sdhci_device1.dev.platform_data = &sdhci_pdata1;
> 
> The SDHCI driver has already been updated to get this platform data
> from the OF node in the platform device; it doesn't need hard-coded
> platform data to be passed to it.
> 
> Similarly, the following from your I2C patch shouldn't be required:
> 
> +static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
> +	.bus_clk_rate   = 400000,
> +};
> ...
> +	tegra_i2c_device1.dev.platform_data = &harmony_i2c1_platform_data;
> 
> Instead, the I2C driver should be modified to get its data from the
> OF node in the platform device just like the SDHCI driver does. This
> should be a pretty trivial amount of code to add to the I2C patch.

Grant and I have chatted about this. Our plan was to get to a
intermediate step where all of the devices inside the SoC including the
i2c controller (I believe the i2c controller itself is inside the SoC)
continue to be initialized using the current static method
(platform_device_register).

The implementation works like this ...

In tegra_dt_init(), of_platform_prepare() is called before devices are
registered. of_platform_prepare() makes note of all the device tree
nodes without registering anything.

When platform devices are registered (platform_device_register()),
of_platform is notified and it attaches the device tree node to the
device struct.

Then when all the SoC internal devices are done being registered using
the static method, there remains a set of device tree nodes that haven't
been registered as devices in the system. These device tree nodes should
be primarily for devices on the board and not in the SoC.

At this point, of_platform_populate() is called, this registers the
remaining devices in the device tree node so that drivers can be loaded
for them using device tree methods.

The reason for this approach was that this step would make for minimal
impact to existing systems. Yet it would still provide more flexibility
for board specific devices.

The thought was that once this is in place we can collectively
re-evaluate if we want to take the next step and initialize everything
from the device tree.

> 
>> I may not be understanding your question.
>>
>>>
>>> So, all this already works without putting the board-specific platform
>>> data definitions into board-dt.c as an temporary measure.
>>>
> 

Re: [RFC 1/2] ARM: Tegra: Device Tree Support: Update how sdhci devices are initialized

[Grant Likely]

On Wed, May 11, 2011 at 7:01 AM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> John Bonesio wrote at Tuesday, May 10, 2011 5:33 PM:
> +static struct tegra_sdhci_platform_data sdhci_pdata1 = {
> +       .cd_gpio        = -1,
> +       .wp_gpio        = -1,
> +       .power_gpio     = -1,
> +};
> ...
> +       tegra_sdhci_device1.dev.platform_data = &sdhci_pdata1;
>
> The SDHCI driver has already been updated to get this platform data
> from the OF node in the platform device; it doesn't need hard-coded
> platform data to be passed to it.
>
> Similarly, the following from your I2C patch shouldn't be required:
>
> +static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
> +       .bus_clk_rate   = 400000,
> +};
> ...
> +       tegra_i2c_device1.dev.platform_data = &harmony_i2c1_platform_data;

Correct.  John, you /should/ be able to simply drop the hunk in this
patch that adds platform_data bits.  All you're doing with this patch
is dropping the local declaration of the platform_devices and using
the ones defined in arch/arm/mach-tegra/devices.c.  Setting the
platform_data pointer is not required.

g.

[RFC 2/2] ARM: Tegra: Device Tree Support: Add i2c devices

[John Bonesio]

This patch initializes i2c controller devices in board-dt.c. The i2c controller
is added to tegra250.dtsi so later on-board i2c devices can be found and
initialized based on the device tree information.

Signed-off-by: John Bonesio <bones-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---

 arch/arm/boot/dts/tegra250.dtsi |   33 +++++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/board-dt.c  |   32 ++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-tegra.c  |    4 ++++
 3 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/tegra250.dtsi b/arch/arm/boot/dts/tegra250.dtsi
index f1801b8..7d8fc51 100644
--- a/arch/arm/boot/dts/tegra250.dtsi
+++ b/arch/arm/boot/dts/tegra250.dtsi
@@ -20,6 +20,39 @@
 		};
 	};
 
+
+	i2c@7000C000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000C000 0x100>;
+		interrupts = < 70 >;
+	};
+
+	i2c@7000C400 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000C400 0x100>;
+		interrupts = < 116 >;
+	};
+
+	i2c@7000C500 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000C500 0x100>;
+		interrupts = < 124 >;
+	};
+
+	i2c@7000D000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra250-i2c";
+		reg = <0x7000D000 0x200>;
+		interrupts = < 85 >;
+	};
+
 	gpio: gpio@6000d000 {
 		compatible = "nvidia,tegra250-gpio";
 		reg = < 0x6000d000 0x1000 >;
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index e61d563..cc7903b 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -28,6 +28,8 @@
 #include <linux/of_platform.h>
 #include <linux/pda_power.h>
 #include <linux/io.h>
+#include <linux/i2c.h>
+#include <linux/i2c-tegra.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -44,6 +46,35 @@
 #include "devices.h"
 #include "gpio-names.h"
 
+static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
+	.bus_clk_rate   = 400000,
+};
+
+static struct tegra_i2c_platform_data harmony_i2c2_platform_data = {
+	.bus_clk_rate   = 400000,
+};
+
+static struct tegra_i2c_platform_data harmony_i2c3_platform_data = {
+	.bus_clk_rate   = 400000,
+};
+
+static struct tegra_i2c_platform_data harmony_dvc_platform_data = {
+	.bus_clk_rate   = 400000,
+};
+
+static void __init harmony_i2c_init(void)
+{
+	tegra_i2c_device1.dev.platform_data = &harmony_i2c1_platform_data;
+	tegra_i2c_device2.dev.platform_data = &harmony_i2c2_platform_data;
+	tegra_i2c_device3.dev.platform_data = &harmony_i2c3_platform_data;
+	tegra_i2c_device4.dev.platform_data = &harmony_dvc_platform_data;
+
+	platform_device_register(&tegra_i2c_device1);
+	platform_device_register(&tegra_i2c_device2);
+	platform_device_register(&tegra_i2c_device3);
+	platform_device_register(&tegra_i2c_device4);
+}
+
 static struct platform_device *harmony_devices[] __initdata = {
 	&tegra_sdhci_device1,
 	&tegra_sdhci_device2,
@@ -111,6 +142,7 @@ static void __init tegra_dt_init(void)
 	tegra_sdhci_device4.dev.platform_data = &sdhci_pdata4;
 
 	platform_add_devices(harmony_devices, ARRAY_SIZE(harmony_devices));
+	harmony_i2c_init();
 
 	/*
 	 * Finished with the static registrations now; fill in the missing
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index b4ab39b..a2f3ec6 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -26,6 +26,7 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/i2c-tegra.h>
+#include <linux/of_i2c.h>
 
 #include <asm/unaligned.h>
 
@@ -598,6 +599,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->adapter.dev.parent = &pdev->dev;
 	i2c_dev->adapter.nr = pdev->id;
+	i2c_dev->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
 
 	ret = i2c_add_numbered_adapter(&i2c_dev->adapter);
 	if (ret) {
@@ -605,6 +607,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		goto err_free_irq;
 	}
 
+	of_i2c_register_devices(&i2c_dev->adapter);
+
 	return 0;
 err_free_irq:
 	free_irq(i2c_dev->irq, i2c_dev);

RE: [RFC 2/2] ARM: Tegra: Device Tree Support: Add i2c devices

[Stephen Warren]

John Bonesio wrote at Tuesday, May 10, 2011 2:15 PM:
> This patch initializes i2c controller devices in board-dt.c. The i2c
> controller
> is added to tegra250.dtsi so later on-board i2c devices can be found and
> initialized based on the device tree information.

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

> +
> +	i2c@7000C000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "nvidia,tegra250-i2c";
> +		reg = <0x7000C000 0x100>;
> +		interrupts = < 70 >;
> +	};

Could we add 'status = "disabled"' for all of these in the core Tegra SoC
file. Then, the board-specific files (e.g. tegra-harmony.dts) will set
'status = "ok"' for just the particular controllers that are actually used
by the board. The SDHCI controllers are setup this way now per my recent
patches.

> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c

> +static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
> +	.bus_clk_rate   = 400000,
> +};

Similar to my comments on the SDHCI patch, shouldn't the platform data
come only from devicetree?

> +static void __init harmony_i2c_init(void)

It'd be nice not to name stuff in board-dt.c after "harmony", since the
idea is for it to work on arbitrary boards given the appropriate
devicetree.

> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c

Since the driver and board files go to separate subsystems, shouldn't
the changes to this file be a separate patch, to aid eventual
upstreaming?

BTW, thanks for working on Tegra!

-- 
nvpublic

Re: [RFC 2/2] ARM: Tegra: Device Tree Support: Add i2c devices

[John Bonesio]

My comments are inline below.

- John

On 05/10/2011 03:10 PM, Stephen Warren wrote:
> John Bonesio wrote at Tuesday, May 10, 2011 2:15 PM:
>> This patch initializes i2c controller devices in board-dt.c. The i2c
>> controller
>> is added to tegra250.dtsi so later on-board i2c devices can be found and
>> initialized based on the device tree information.
> 
>> diff --git a/arch/arm/boot/dts/tegra250.dtsi b/arch/arm/boot/dts/tegra250.dtsi
> 
>> +
>> +	i2c@7000C000 {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		compatible = "nvidia,tegra250-i2c";
>> +		reg = <0x7000C000 0x100>;
>> +		interrupts = < 70 >;
>> +	};
> 
> Could we add 'status = "disabled"' for all of these in the core Tegra SoC
> file. Then, the board-specific files (e.g. tegra-harmony.dts) will set
> 'status = "ok"' for just the particular controllers that are actually used
> by the board. The SDHCI controllers are setup this way now per my recent
> patches.

Good idea.

> 
>> diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> 
>> +static struct tegra_i2c_platform_data harmony_i2c1_platform_data = {
>> +	.bus_clk_rate   = 400000,
>> +};
> 
> Similar to my comments on the SDHCI patch, shouldn't the platform data
> come only from devicetree?
> 
>> +static void __init harmony_i2c_init(void)
> 
> It'd be nice not to name stuff in board-dt.c after "harmony", since the
> idea is for it to work on arbitrary boards given the appropriate
> devicetree.

Yep. I'll fix this in the next revision.

> 
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> 
> Since the driver and board files go to separate subsystems, shouldn't
> the changes to this file be a separate patch, to aid eventual
> upstreaming?

Maybe. Let's see what others think.

> 
> BTW, thanks for working on Tegra!
> 

:-)