[PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Stephen Warren]

tegra250.dts describes everything within the Tegra SoC, but leaves all
optional devices disabled.

tegra-harmony.dts is now the configuration of the SoC for the board,
i.e. it enables the optional devices that are required for the board,
and provides any required platform data for those devics.

The prevents every Tegra board having to repeat all the basic internal
SoC wiring.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra-harmony.dts |   90 ++------------------------------
 arch/arm/boot/dts/tegra250.dts      |   99 +++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 85 deletions(-)
 create mode 100644 arch/arm/boot/dts/tegra250.dts

diff --git a/arch/arm/boot/dts/tegra-harmony.dts b/arch/arm/boot/dts/tegra-harmony.dts
index a22c1bb..63f3ca0 100644
--- a/arch/arm/boot/dts/tegra-harmony.dts
+++ b/arch/arm/boot/dts/tegra-harmony.dts
@@ -1,12 +1,9 @@
-
 /dts-v1/;
+/include/ "tegra250.dts"
 
 / {
-	model = "nVidia Harmony";
+	model = "NVIDIA Harmony";
 	compatible = "nvidia,harmony", "nvidia,tegra250";
-	#address-cells = <1>;
-	#size-cells = <1>;
-	interrupt-parent = <&intc>;
 
 	chosen {
 		bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk0p2 rw rootdelay=2";
@@ -18,97 +15,20 @@
 		        0x20000000 0x20000000 >;
 	};
 
-	amba {
-		compatible = "simple-bus";
-		#address-cells = <1>;
-		#size-cells = <1>;
-		ranges;
-
-		intc: intc {
-			compatible = "nvidia,tegra250-gic", "arm,gic";
-			interrupt-controller;
-			#interrupt-cells = <1>;
-			reg = < 0x50041000 0x1000
-			        0x50040100 0x100 >;
-		};
-	};
-
-	gpio: gpio@6000d000 {
-		compatible = "nvidia,tegra250-gpio";
-		reg = < 0x6000d000 0x1000 >;
-		interrupts = < 64 65 66 67 87 119 121 >;
-		#gpio-cells = <2>;
-		gpio-controller;
-	};
-
-	serial@70006000 {
-		compatible = "nvidia,tegra250-uart";
-		reg = <0x70006000 0x40>;
-		reg-shift = <2>;
-		interrupts = < 68 >;
-		status = "disabled";
-	};
-
-	serial@70006040 {
-		compatible = "nvidia,tegra250-uart";
-		reg = <0x70006040 0x40>;
-		reg-shift = <2>;
-		interrupts = < 69 >;
-		status = "disabled";
-	};
-
-	serial@70006200 {
-		compatible = "nvidia,tegra250-uart";
-		reg = <0x70006200 0x100>;
-		reg-shift = <2>;
-		interrupts = < 78 >;
-		status = "disabled";
-	};
-
 	serial@70006300 {
-		compatible = "nvidia,tegra250-uart";
-		reg = <0x70006300 0x100>;
-		reg-shift = <2>;
-		interrupts = < 122 >;
-
+		status = "ok";
 		clock-frequency = < 216000000 >;
 	};
 
-	serial@70006400 {
-		compatible = "nvidia,tegra250-uart";
-		reg = <0x70006400 0x100>;
-		reg-shift = <2>;
-		interrupts = < 123 >;
-		status = "disabled";
-	};
-
-	sdhci@c8000000 {
-		compatible = "nvidia,tegra250-sdhci";
-		reg = <0xc8000000 0x200>;
-		interrupts = < 46 >;
-		status = "disabled";
-	};
-
 	sdhci@c8000200 {
-		compatible = "nvidia,tegra250-sdhci";
-		reg = <0xc8000200 0x200>;
-		interrupts = < 47 >;
+		status = "ok";
 		gpios = <&gpio 69 0>, /* cd, gpio PI5 */
 			<&gpio 57 0>, /* wp, gpio PH1 */
 			<&gpio 155 0>; /* power, gpio PT3 */
 	};
 
-	sdhci@c8000400 {
-		compatible = "nvidia,tegra250-sdhci";
-		reg = <0xc8000400 0x200>;
-		interrupts = < 51 >;
-		status = "disabled";
-	};
-
 	sdhci@c8000600 {
-		compatible = "nvidia,tegra250-sdhci";
-		reg = <0xc8000600 0x200>;
-		interrupts = < 63 >;
+		status = "ok";
 		gpios = <&gpio 58 0>, /* cd, gpio PH2 */
 			<&gpio 59 0>, /* wp, gpio PH3 */
 			<&gpio 70 0>; /* power, gpio PI6 */
diff --git a/arch/arm/boot/dts/tegra250.dts b/arch/arm/boot/dts/tegra250.dts
new file mode 100644
index 0000000..b60d18d
--- /dev/null
+++ b/arch/arm/boot/dts/tegra250.dts
@@ -0,0 +1,99 @@
+/ {
+	model = "NVIDIA Tegra 250";
+	compatible = "nvidia,tegra250";
+	#address-cells = <1>;
+	#size-cells = <1>;
+	interrupt-parent = <&intc>;
+
+	amba {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+
+		intc: intc {
+			compatible = "nvidia,tegra250-gic", "arm,gic";
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			reg = < 0x50041000 0x1000
+			        0x50040100 0x100 >;
+		};
+	};
+
+	gpio: gpio@6000d000 {
+		compatible = "nvidia,tegra250-gpio";
+		reg = < 0x6000d000 0x1000 >;
+		interrupts = < 64 65 66 67 87 119 121 >;
+		#gpio-cells = <2>;
+		gpio-controller;
+	};
+
+	serial@70006000 {
+		compatible = "nvidia,tegra250-uart";
+		reg = <0x70006000 0x40>;
+		reg-shift = <2>;
+		interrupts = < 68 >;
+		status = "disabled";
+	};
+
+	serial@70006040 {
+		compatible = "nvidia,tegra250-uart";
+		reg = <0x70006040 0x40>;
+		reg-shift = <2>;
+		interrupts = < 69 >;
+		status = "disabled";
+	};
+
+	serial@70006200 {
+		compatible = "nvidia,tegra250-uart";
+		reg = <0x70006200 0x100>;
+		reg-shift = <2>;
+		interrupts = < 78 >;
+		status = "disabled";
+	};
+
+	serial@70006300 {
+		compatible = "nvidia,tegra250-uart";
+		reg = <0x70006300 0x100>;
+		reg-shift = <2>;
+		interrupts = < 122 >;
+		status = "disabled";
+	};
+
+	serial@70006400 {
+		compatible = "nvidia,tegra250-uart";
+		reg = <0x70006400 0x100>;
+		reg-shift = <2>;
+		interrupts = < 123 >;
+		status = "disabled";
+	};
+
+	sdhci@c8000000 {
+		compatible = "nvidia,tegra250-sdhci";
+		reg = <0xc8000000 0x200>;
+		interrupts = < 46 >;
+		status = "disabled";
+	};
+
+	sdhci@c8000200 {
+		compatible = "nvidia,tegra250-sdhci";
+		reg = <0xc8000200 0x200>;
+		interrupts = < 47 >;
+		status = "disabled";
+	};
+
+	sdhci@c8000400 {
+		compatible = "nvidia,tegra250-sdhci";
+		reg = <0xc8000400 0x200>;
+		interrupts = < 51 >;
+		status = "disabled";
+	};
+
+	sdhci@c8000600 {
+		compatible = "nvidia,tegra250-sdhci";
+		reg = <0xc8000600 0x200>;
+		interrupts = < 63 >;
+		status = "disabled";
+	};
+};
+
-- 
1.7.1

[PATCH 2/2] ARM: Tegra: Seaboard: Add devicetree support

[Stephen Warren]

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra-seaboard.dts |   33 +++++++++++++++++++++++++++++++++
 arch/arm/mach-tegra/Makefile.boot    |    1 +
 2 files changed, 34 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/boot/dts/tegra-seaboard.dts

diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
new file mode 100644
index 0000000..d5c9ad7
--- /dev/null
+++ b/arch/arm/boot/dts/tegra-seaboard.dts
@@ -0,0 +1,33 @@
+/dts-v1/;
+/include/ "tegra250.dts"
+
+/ {
+	model = "NVIDIA Harmony";
+	compatible = "nvidia,seaboard", "nvidia,tegra250";
+
+	chosen {
+		bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk1p3 rw rootdelay=2";
+	};
+
+	memory {
+		device_type = "memory";
+		reg = < 0x00000000 0x1C000000
+		        0x20000000 0x20000000 >;
+	};
+
+	serial@70006300 {
+		status = "ok";
+		clock-frequency = < 216000000 >;
+	};
+
+	sdhci@c8000400 {
+		status = "ok";
+		gpios = <&gpio 69 0>, /* cd, gpio PI5 */
+			<&gpio 57 0>, /* wp, gpio PH1 */
+			<&gpio 70 0>; /* power, gpio PI6 */
+	};
+
+	sdhci@c8000600 {
+		status = "ok";
+	};
+};
diff --git a/arch/arm/mach-tegra/Makefile.boot b/arch/arm/mach-tegra/Makefile.boot
index c98addf..428ad12 100644
--- a/arch/arm/mach-tegra/Makefile.boot
+++ b/arch/arm/mach-tegra/Makefile.boot
@@ -3,3 +3,4 @@ params_phys-$(CONFIG_ARCH_TEGRA_2x_SOC)	:= 0x00000100
 initrd_phys-$(CONFIG_ARCH_TEGRA_2x_SOC)	:= 0x00800000
 
 dtb-$(CONFIG_MACH_HARMONY) += tegra-harmony.dtb
+dtb-$(CONFIG_MACH_SEABOARD) += tegra-seaboard.dtb
-- 
1.7.1

Re: [PATCH 2/2] ARM: Tegra: Seaboard: Add devicetree support

[Olof Johansson]

Hi,

On Fri, Apr 29, 2011 at 10:12:31PM -0600, Stephen Warren wrote:
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/tegra-seaboard.dts |   33 +++++++++++++++++++++++++++++++++
>  arch/arm/mach-tegra/Makefile.boot    |    1 +
>  2 files changed, 34 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/boot/dts/tegra-seaboard.dts
> 
> diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
> new file mode 100644
> index 0000000..d5c9ad7
> --- /dev/null
> +++ b/arch/arm/boot/dts/tegra-seaboard.dts
> @@ -0,0 +1,33 @@
> +/dts-v1/;
> +/include/ "tegra250.dts"
> +
> +/ {
> +	model = "NVIDIA Harmony";

NVIDIA Seaboard?

> +	compatible = "nvidia,seaboard", "nvidia,tegra250";
> +
> +	chosen {
> +		bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk1p3 rw rootdelay=2";

I'm partial to rootwait instead of rootdelay, a little less fragile for slow
devices.

> +	memory {
> +		device_type = "memory";
> +		reg = < 0x00000000 0x1C000000
> +		        0x20000000 0x20000000 >;
> +	};

I guess a node for the graphics memory could be warranted

> +
> +	serial@70006300 {
> +		status = "ok";
> +		clock-frequency = < 216000000 >;
> +	};
> +
> +	sdhci@c8000400 {
> +		status = "ok";
> +		gpios = <&gpio 69 0>, /* cd, gpio PI5 */
> +			<&gpio 57 0>, /* wp, gpio PH1 */
> +			<&gpio 70 0>; /* power, gpio PI6 */

How would you represent just one missing gpio here? It might be better
to have separate properties for each.

Also, it'd be nice to document these assumptions where other device tree
bindings are (Documentation/devicetree/bindings).


-Olof

Re: [PATCH 2/2] ARM: Tegra: Seaboard: Add devicetree support

[Grant Likely]

On Sun, May 1, 2011 at 8:38 AM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> Hi,
>
> On Fri, Apr 29, 2011 at 10:12:31PM -0600, Stephen Warren wrote:
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  arch/arm/boot/dts/tegra-seaboard.dts |   33 +++++++++++++++++++++++++++++++++
>>  arch/arm/mach-tegra/Makefile.boot    |    1 +
>>  2 files changed, 34 insertions(+), 0 deletions(-)
>>  create mode 100644 arch/arm/boot/dts/tegra-seaboard.dts
>>
>> diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
>> new file mode 100644
>> index 0000000..d5c9ad7
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/tegra-seaboard.dts
>> @@ -0,0 +1,33 @@
>> +/dts-v1/;
>> +/include/ "tegra250.dts"
>> +
>> +/ {
>> +     model = "NVIDIA Harmony";
>
> NVIDIA Seaboard?
>
>> +     compatible = "nvidia,seaboard", "nvidia,tegra250";
>> +
>> +     chosen {
>> +             bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk1p3 rw rootdelay=2";
>
> I'm partial to rootwait instead of rootdelay, a little less fragile for slow
> devices.
>
>> +     memory {
>> +             device_type = "memory";
>> +             reg = < 0x00000000 0x1C000000
>> +                     0x20000000 0x20000000 >;
>> +     };
>
> I guess a node for the graphics memory could be warranted

I've modified this in devicetree test to specify the entire memory
range, and carve out 0x1c000000..0x1fffffff with a memreserve section.

>
>> +
>> +     serial@70006300 {
>> +             status = "ok";
>> +             clock-frequency = < 216000000 >;
>> +     };
>> +
>> +     sdhci@c8000400 {
>> +             status = "ok";
>> +             gpios = <&gpio 69 0>, /* cd, gpio PI5 */
>> +                     <&gpio 57 0>, /* wp, gpio PH1 */
>> +                     <&gpio 70 0>; /* power, gpio PI6 */
>
> How would you represent just one missing gpio here? It might be better
> to have separate properties for each.

The gpio binding allows for empty entries by using '0' for the phandle.

>
> Also, it'd be nice to document these assumptions where other device tree
> bindings are (Documentation/devicetree/bindings).
>
>
> -Olof
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 2/2] ARM: Tegra: Seaboard: Add devicetree support

[Olof Johansson]

On Sun, May 01, 2011 at 08:46:04AM -0600, Grant Likely wrote:
> On Sun, May 1, 2011 at 8:38 AM, Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org> wrote:
> > Hi,
> >
> > On Fri, Apr 29, 2011 at 10:12:31PM -0600, Stephen Warren wrote:
> >> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  arch/arm/boot/dts/tegra-seaboard.dts |   33 +++++++++++++++++++++++++++++++++
> >>  arch/arm/mach-tegra/Makefile.boot    |    1 +
> >>  2 files changed, 34 insertions(+), 0 deletions(-)
> >>  create mode 100644 arch/arm/boot/dts/tegra-seaboard.dts
> >>
> >> diff --git a/arch/arm/boot/dts/tegra-seaboard.dts b/arch/arm/boot/dts/tegra-seaboard.dts
> >> new file mode 100644
> >> index 0000000..d5c9ad7
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/tegra-seaboard.dts
> >> @@ -0,0 +1,33 @@
> >> +/dts-v1/;
> >> +/include/ "tegra250.dts"
> >> +
> >> +/ {
> >> +     model = "NVIDIA Harmony";
> >
> > NVIDIA Seaboard?
> >
> >> +     compatible = "nvidia,seaboard", "nvidia,tegra250";
> >> +
> >> +     chosen {
> >> +             bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8 root=/dev/mmcblk1p3 rw rootdelay=2";
> >
> > I'm partial to rootwait instead of rootdelay, a little less fragile for slow
> > devices.
> >
> >> +     memory {
> >> +             device_type = "memory";
> >> +             reg = < 0x00000000 0x1C000000
> >> +                     0x20000000 0x20000000 >;
> >> +     };
> >
> > I guess a node for the graphics memory could be warranted
> 
> I've modified this in devicetree test to specify the entire memory
> range, and carve out 0x1c000000..0x1fffffff with a memreserve section.
> 
> >
> >> +
> >> +     serial@70006300 {
> >> +             status = "ok";
> >> +             clock-frequency = < 216000000 >;
> >> +     };
> >> +
> >> +     sdhci@c8000400 {
> >> +             status = "ok";
> >> +             gpios = <&gpio 69 0>, /* cd, gpio PI5 */
> >> +                     <&gpio 57 0>, /* wp, gpio PH1 */
> >> +                     <&gpio 70 0>; /* power, gpio PI6 */
> >
> > How would you represent just one missing gpio here? It might be better
> > to have separate properties for each.
> 
> The gpio binding allows for empty entries by using '0' for the phandle.

Hm, I'm still not much a fan of an opaque 3-entry array instead of named
entries but I'm ok with either I suppose. How do other bindings do similar things?


-Olof

Re: [PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Olof Johansson]

On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
> tegra250.dts describes everything within the Tegra SoC, but leaves all
> optional devices disabled.
> 
> tegra-harmony.dts is now the configuration of the SoC for the board,
> i.e. it enables the optional devices that are required for the board,
> and provides any required platform data for those devics.
> 
> The prevents every Tegra board having to repeat all the basic internal
> SoC wiring.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Acked-by: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>

A mostly unrelated question below.

[...]

> +	serial@70006000 {
> +		compatible = "nvidia,tegra250-uart";

I know this is how Grant specified it, but shouldn't these also have
a compat for ns16550?

> +		reg = <0x70006000 0x40>;
> +		reg-shift = <2>;
> +		interrupts = < 68 >;
> +		status = "disabled";
> +	};


-Olof

RE: [PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Stephen Warren]

Olof Johansson wrote at Sunday, May 01, 2011 8:56 AM:
> On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
> > tegra250.dts describes everything within the Tegra SoC, but leaves all
> > optional devices disabled.
> >
> > tegra-harmony.dts is now the configuration of the SoC for the board,
> > i.e. it enables the optional devices that are required for the board,
> > and provides any required platform data for those devics.
> >
> > The prevents every Tegra board having to repeat all the basic internal
> > SoC wiring.
> >
> > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Acked-by: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
> 
> A mostly unrelated question below.
> 
> [...]
> 
> > +	serial@70006000 {
> > +		compatible = "nvidia,tegra250-uart";
> 
> I know this is how Grant specified it, but shouldn't these also have
> a compat for ns16550?

At present, I'm not sure it is technically compatible. If you look at
drivers/tty/serial/of_serial.c, you'll see:

static struct of_device_id __devinitdata of_platform_serial_table[] = {
...
        { .compatible = "ns16550",  .data = (void *)PORT_16550, },
...
        { .compatible = "nvidia,tegra250-uart",  .data = (void *)PORT_XSCALE, },

That PORT_XSCALE is different to the ns16550 entry. Arguably, that
field should be something that comes from the device tree, just
like e.g. reg-shift, but it certainly doesn't right now.

Grant, what are your thoughts on this?

Thanks.

-- 
nvpublic

Re: [PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Grant Likely]

On Mon, May 2, 2011 at 12:00 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Olof Johansson wrote at Sunday, May 01, 2011 8:56 AM:
>> On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
>> > tegra250.dts describes everything within the Tegra SoC, but leaves all
>> > optional devices disabled.
>> >
>> > tegra-harmony.dts is now the configuration of the SoC for the board,
>> > i.e. it enables the optional devices that are required for the board,
>> > and provides any required platform data for those devics.
>> >
>> > The prevents every Tegra board having to repeat all the basic internal
>> > SoC wiring.
>> >
>> > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> Acked-by: Olof Johansson <olof-nZhT3qVonbNeoWH0uzbU5w@public.gmane.org>
>>
>> A mostly unrelated question below.
>>
>> [...]
>>
>> > +   serial@70006000 {
>> > +           compatible = "nvidia,tegra250-uart";
>>
>> I know this is how Grant specified it, but shouldn't these also have
>> a compat for ns16550?
>
> At present, I'm not sure it is technically compatible. If you look at
> drivers/tty/serial/of_serial.c, you'll see:
>
> static struct of_device_id __devinitdata of_platform_serial_table[] = {
> ...
>        { .compatible = "ns16550",  .data = (void *)PORT_16550, },
> ...
>        { .compatible = "nvidia,tegra250-uart",  .data = (void *)PORT_XSCALE, },
>
> That PORT_XSCALE is different to the ns16550 entry. Arguably, that
> field should be something that comes from the device tree, just
> like e.g. reg-shift, but it certainly doesn't right now.
>
> Grant, what are your thoughts on this?

Heh, when I added that line I just mirrored the 'type' of 16550 as
detected by the 8250.c driver when it probes and made a mental note
that it should be revisited.  I don't know the hardware very well, so
I cannot easily say what the right thing to do is, but making it
ns16550 compatible does seem to make sense.

g.

RE: [PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Stephen Warren]

Grant Likely wrote at Monday, May 02, 2011 2:16 PM:
> On Mon, May 2, 2011 at 12:00 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Olof Johansson wrote at Sunday, May 01, 2011 8:56 AM:
> >> On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
> >> A mostly unrelated question below.
> >>
> >> [...]
> >>
> >> > +   serial@70006000 {
> >> > +           compatible = "nvidia,tegra250-uart";
> >>
> >> I know this is how Grant specified it, but shouldn't these also have
> >> a compat for ns16550?
> >
> > At present, I'm not sure it is technically compatible. If you look at
> > drivers/tty/serial/of_serial.c, you'll see:
> >
> > static struct of_device_id __devinitdata of_platform_serial_table[] = {
> > ...
> >        { .compatible = "ns16550",  .data = (void *)PORT_16550, },
> > ...
> >        { .compatible = "nvidia,tegra250-uart",  .data = (void *)PORT_XSCALE, },
> >
> > That PORT_XSCALE is different to the ns16550 entry. Arguably, that
> > field should be something that comes from the device tree, just
> > like e.g. reg-shift, but it certainly doesn't right now.
> >
> > Grant, what are your thoughts on this?
> 
> Heh, when I added that line I just mirrored the 'type' of 16550 as
> detected by the 8250.c driver when it probes and made a mental note
> that it should be revisited.  I don't know the hardware very well, so
> I cannot easily say what the right thing to do is, but making it
> ns16550 compatible does seem to make sense.

I looked into this a bit more and found the following:

In all the existing board files for Tegra, none of the
plat_serial8250_port initializations specify any type value, and equally
the flags do not include UPF_FIXED_TYPE. This causes the UART driver to
probe (run tests on) the hardware to determine exactly what type of UART
is present.

However, of_serial.c does set UPF_FIXED_TYPE, and hence port->type must
have a valid value. I suspect it'd be a bad idea to change this since
it'd affect a ton of extant PPC platforms.

Equally, PORT_XSCALE does imply different HW behavior to any of the
other types, so tegra250.dtsi shouldn't just specify that it's compatible
with "ns16550".

However, I believe we should make the following change: Instead of adding
a specific Tegra "nvidia,tegra250-uart" to of_serial.c's device table, we
should instead add a generic "xscale" entry along the same lines as all
the existing generic entries. This is so if we end up with 10 different
vendors with "XSCALE" serial ports, we don't have to add an entry into
of_serial.c for each one.

Does that seem reasonable?

I'll send a patch series to implement that change in just a second.

-- 
nvpublic

Re: [PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Grant Likely]

On Thu, May 12, 2011 at 6:40 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Grant Likely wrote at Monday, May 02, 2011 2:16 PM:
>> On Mon, May 2, 2011 at 12:00 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > Olof Johansson wrote at Sunday, May 01, 2011 8:56 AM:
>> >> On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
>> >> A mostly unrelated question below.
>> >>
>> >> [...]
>> >>
>> >> > +   serial@70006000 {
>> >> > +           compatible = "nvidia,tegra250-uart";
>> >>
>> >> I know this is how Grant specified it, but shouldn't these also have
>> >> a compat for ns16550?
>> >
>> > At present, I'm not sure it is technically compatible. If you look at
>> > drivers/tty/serial/of_serial.c, you'll see:
>> >
>> > static struct of_device_id __devinitdata of_platform_serial_table[] = {
>> > ...
>> >        { .compatible = "ns16550",  .data = (void *)PORT_16550, },
>> > ...
>> >        { .compatible = "nvidia,tegra250-uart",  .data = (void *)PORT_XSCALE, },
>> >
>> > That PORT_XSCALE is different to the ns16550 entry. Arguably, that
>> > field should be something that comes from the device tree, just
>> > like e.g. reg-shift, but it certainly doesn't right now.
>> >
>> > Grant, what are your thoughts on this?
>>
>> Heh, when I added that line I just mirrored the 'type' of 16550 as
>> detected by the 8250.c driver when it probes and made a mental note
>> that it should be revisited.  I don't know the hardware very well, so
>> I cannot easily say what the right thing to do is, but making it
>> ns16550 compatible does seem to make sense.
>
> I looked into this a bit more and found the following:
>
> In all the existing board files for Tegra, none of the
> plat_serial8250_port initializations specify any type value, and equally
> the flags do not include UPF_FIXED_TYPE. This causes the UART driver to
> probe (run tests on) the hardware to determine exactly what type of UART
> is present.
>
> However, of_serial.c does set UPF_FIXED_TYPE, and hence port->type must
> have a valid value. I suspect it'd be a bad idea to change this since
> it'd affect a ton of extant PPC platforms.
>
> Equally, PORT_XSCALE does imply different HW behavior to any of the
> other types, so tegra250.dtsi shouldn't just specify that it's compatible
> with "ns16550".

I investigated and found the same thing, and that is why I set up the
current code the way I did; to preserve the current behaviour.

However, is the current behaviour even right?  My impression from the
code is that probing for the type of ns16550 is little more than an
accident.  Is xscale really the best behaviour to select fro this
device?

g.

RE: [PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Stephen Warren]

Grant Likely wrote at Thursday, May 12, 2011 5:02 PM:
> On Thu, May 12, 2011 at 6:40 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Grant Likely wrote at Monday, May 02, 2011 2:16 PM:
> >> On Mon, May 2, 2011 at 12:00 PM, Stephen Warren <swarren-DDmLM1+adcphl2p70BpVqQ@public.gmane.orgm> wrote:
> >> > Olof Johansson wrote at Sunday, May 01, 2011 8:56 AM:
> >> >> On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
> >> >> A mostly unrelated question below.
> >> >>
> >> >> [...]
> >> >>
> >> >> > +   serial@70006000 {
> >> >> > +           compatible = "nvidia,tegra250-uart";
> >> >>
> >> >> I know this is how Grant specified it, but shouldn't these also have
> >> >> a compat for ns16550?
> >> >
> >> > At present, I'm not sure it is technically compatible. If you look at
> >> > drivers/tty/serial/of_serial.c, you'll see:
> >> >
> >> > static struct of_device_id __devinitdata of_platform_serial_table[] =
> {
> >> > ...
> >> >        { .compatible = "ns16550",  .data = (void *)PORT_16550, },
> >> > ...
> >> >        { .compatible = "nvidia,tegra250-uart",  .data = (void *)PORT_XSCALE, },
> >> >
> >> > That PORT_XSCALE is different to the ns16550 entry. Arguably, that
> >> > field should be something that comes from the device tree, just
> >> > like e.g. reg-shift, but it certainly doesn't right now.
> >> >
> >> > Grant, what are your thoughts on this?
> >>
> >> Heh, when I added that line I just mirrored the 'type' of 16550 as
> >> detected by the 8250.c driver when it probes and made a mental note
> >> that it should be revisited.  I don't know the hardware very well, so
> >> I cannot easily say what the right thing to do is, but making it
> >> ns16550 compatible does seem to make sense.
> >
> > I looked into this a bit more and found the following:
> >
> > In all the existing board files for Tegra, none of the
> > plat_serial8250_port initializations specify any type value, and equally
> > the flags do not include UPF_FIXED_TYPE. This causes the UART driver to
> > probe (run tests on) the hardware to determine exactly what type of UART
> > is present.
> >
> > However, of_serial.c does set UPF_FIXED_TYPE, and hence port->type must
> > have a valid value. I suspect it'd be a bad idea to change this since
> > it'd affect a ton of extant PPC platforms.
> >
> > Equally, PORT_XSCALE does imply different HW behavior to any of the
> > other types, so tegra250.dtsi shouldn't just specify that it's compatible
> > with "ns16550".
> 
> I investigated and found the same thing, and that is why I set up the
> current code the way I did; to preserve the current behaviour.
> 
> However, is the current behaviour even right?  My impression from the
> code is that probing for the type of ns16550 is little more than an
> accident.  Is xscale really the best behaviour to select fro this
> device?

The detection for XSCALE port type is solely based on the writability
of a single bit in the IER. The Tegra documentation says this bit is
undefined, so I agree it's probably an accident that Tegra's UART is
being detected as XSCALE. If I disable that part of the auto-detection,
Tegra's UART is detected as a 16550, as expected.

However, that setting doesn't work correctly. Specifically, on Tegra,
there's a bit in the Interrupt Enable Register (IER) that needs to be
set to enable interrupt on RX timeout. Without this, one needs to fill
the RX FIFO up to its attention level before the HW interrupts and
before SW sees any of those characters.

That's one of the two things that the XSCALE port type causes to happen.
The other thing is programming of the IER UUE bit, which is the bit used
in the auto-detection that Tegra apparently doesn't actually implement.

While researching this, I found a patch to 8250.c to add a specific
Tegra port type:

http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff_plain;h=ff4d0279c458b724d273efd88ee87ff7caf48991

I think the overall answer therefore is to:

a) Upstream that patch, although I'll want to separate out the new port
type definition from the break handling, since I haven't researched the
break handling part of that patch. Plus there are a couple of things I
need to follow up on internally before upstreaming.

b) Update of_serial.c to point the new Tegra entry at PORT_TEGRA instead
of PORT_XSCALE.

c) Update the hard-coded board files to specify port_type = PORT_TEGRA
for the UARTs rather than allowing probing.

Does that seem reasonable to everyone?

Thanks for reading!

-- 
nvpublic

Re: [PATCH 1/2] ARM: Tegra: dt: Split out separate Tegra SoC DT

[Grant Likely]

On Fri, May 13, 2011 at 9:28 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Grant Likely wrote at Thursday, May 12, 2011 5:02 PM:
>> On Thu, May 12, 2011 at 6:40 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> > Grant Likely wrote at Monday, May 02, 2011 2:16 PM:
>> >> On Mon, May 2, 2011 at 12:00 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> >> > Olof Johansson wrote at Sunday, May 01, 2011 8:56 AM:
>> >> >> On Fri, Apr 29, 2011 at 10:12:30PM -0600, Stephen Warren wrote:
>> >> >> A mostly unrelated question below.
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> > +   serial@70006000 {
>> >> >> > +           compatible = "nvidia,tegra250-uart";
>> >> >>
>> >> >> I know this is how Grant specified it, but shouldn't these also have
>> >> >> a compat for ns16550?
>> >> >
>> >> > At present, I'm not sure it is technically compatible. If you look at
>> >> > drivers/tty/serial/of_serial.c, you'll see:
>> >> >
>> >> > static struct of_device_id __devinitdata of_platform_serial_table[] =
>> {
>> >> > ...
>> >> >        { .compatible = "ns16550",  .data = (void *)PORT_16550, },
>> >> > ...
>> >> >        { .compatible = "nvidia,tegra250-uart",  .data = (void *)PORT_XSCALE, },
>> >> >
>> >> > That PORT_XSCALE is different to the ns16550 entry. Arguably, that
>> >> > field should be something that comes from the device tree, just
>> >> > like e.g. reg-shift, but it certainly doesn't right now.
>> >> >
>> >> > Grant, what are your thoughts on this?
>> >>
>> >> Heh, when I added that line I just mirrored the 'type' of 16550 as
>> >> detected by the 8250.c driver when it probes and made a mental note
>> >> that it should be revisited.  I don't know the hardware very well, so
>> >> I cannot easily say what the right thing to do is, but making it
>> >> ns16550 compatible does seem to make sense.
>> >
>> > I looked into this a bit more and found the following:
>> >
>> > In all the existing board files for Tegra, none of the
>> > plat_serial8250_port initializations specify any type value, and equally
>> > the flags do not include UPF_FIXED_TYPE. This causes the UART driver to
>> > probe (run tests on) the hardware to determine exactly what type of UART
>> > is present.
>> >
>> > However, of_serial.c does set UPF_FIXED_TYPE, and hence port->type must
>> > have a valid value. I suspect it'd be a bad idea to change this since
>> > it'd affect a ton of extant PPC platforms.
>> >
>> > Equally, PORT_XSCALE does imply different HW behavior to any of the
>> > other types, so tegra250.dtsi shouldn't just specify that it's compatible
>> > with "ns16550".
>>
>> I investigated and found the same thing, and that is why I set up the
>> current code the way I did; to preserve the current behaviour.
>>
>> However, is the current behaviour even right?  My impression from the
>> code is that probing for the type of ns16550 is little more than an
>> accident.  Is xscale really the best behaviour to select fro this
>> device?
>
> The detection for XSCALE port type is solely based on the writability
> of a single bit in the IER. The Tegra documentation says this bit is
> undefined, so I agree it's probably an accident that Tegra's UART is
> being detected as XSCALE. If I disable that part of the auto-detection,
> Tegra's UART is detected as a 16550, as expected.
>
> However, that setting doesn't work correctly. Specifically, on Tegra,
> there's a bit in the Interrupt Enable Register (IER) that needs to be
> set to enable interrupt on RX timeout. Without this, one needs to fill
> the RX FIFO up to its attention level before the HW interrupts and
> before SW sees any of those characters.
>
> That's one of the two things that the XSCALE port type causes to happen.
> The other thing is programming of the IER UUE bit, which is the bit used
> in the auto-detection that Tegra apparently doesn't actually implement.
>
> While researching this, I found a patch to 8250.c to add a specific
> Tegra port type:
>
> http://nv-tegra.nvidia.com/gitweb/?p=linux-2.6.git;a=commitdiff_plain;h=ff4d0279c458b724d273efd88ee87ff7caf48991
>
> I think the overall answer therefore is to:
>
> a) Upstream that patch, although I'll want to separate out the new port
> type definition from the break handling, since I haven't researched the
> break handling part of that patch. Plus there are a couple of things I
> need to follow up on internally before upstreaming.
>
> b) Update of_serial.c to point the new Tegra entry at PORT_TEGRA instead
> of PORT_XSCALE.
>
> c) Update the hard-coded board files to specify port_type = PORT_TEGRA
> for the UARTs rather than allowing probing.
>
> Does that seem reasonable to everyone?

Sounds right to me.

g.