All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-09-18  5:30 ` Philip, Avinash
  0 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-09-18  5:30 UTC (permalink / raw)
  To: linux, tony
  Cc: linux-arm-kernel, linux-kernel, linux-omap, nsekhar,
	gururaja.hebbar, Philip, Avinash

Add McSPI data node to AM33XX device tree file. The McSPI module (and so
as the driver) is reused from OMAP4.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Tested-by: Matt Porter <mporter@ti.com>
---
Changes since v1:
        - Corrected reg offset in reg DT entry.

:100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
 arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index ff3badb..065fd54 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -219,5 +219,30 @@
 			interrupt-parent = <&intc>;
 			interrupts = <91>;
 		};
+
+		spi0: spi@48030000 {
+			compatible = "ti,omap4-mcspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x48030000 0x400>;
+			interrupt-parent = <&intc>;
+			interrupt = <65>;
+			ti,spi-num-cs = <2>;
+			ti,hwmods = "spi0";
+			status = "disabled";
+
+		};
+
+		spi1: spi@481a0000 {
+			compatible = "ti,omap4-mcspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x481a0000 0x400>;
+			interrupt-parent = <&intc>;
+			interrupt = <125>;
+			ti,spi-num-cs = <2>;
+			ti,hwmods = "spi1";
+			status = "disabled";
+		};
 	};
 };
-- 
1.7.4.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-09-18  5:30 ` Philip, Avinash
  0 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-09-18  5:30 UTC (permalink / raw)
  To: linux, tony
  Cc: linux-arm-kernel, linux-kernel, linux-omap, nsekhar,
	gururaja.hebbar, Philip, Avinash

Add McSPI data node to AM33XX device tree file. The McSPI module (and so
as the driver) is reused from OMAP4.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Tested-by: Matt Porter <mporter@ti.com>
---
Changes since v1:
        - Corrected reg offset in reg DT entry.

:100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
 arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index ff3badb..065fd54 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -219,5 +219,30 @@
 			interrupt-parent = <&intc>;
 			interrupts = <91>;
 		};
+
+		spi0: spi@48030000 {
+			compatible = "ti,omap4-mcspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x48030000 0x400>;
+			interrupt-parent = <&intc>;
+			interrupt = <65>;
+			ti,spi-num-cs = <2>;
+			ti,hwmods = "spi0";
+			status = "disabled";
+
+		};
+
+		spi1: spi@481a0000 {
+			compatible = "ti,omap4-mcspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x481a0000 0x400>;
+			interrupt-parent = <&intc>;
+			interrupt = <125>;
+			ti,spi-num-cs = <2>;
+			ti,hwmods = "spi1";
+			status = "disabled";
+		};
 	};
 };
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-09-18  5:30 ` Philip, Avinash
  0 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-09-18  5:30 UTC (permalink / raw)
  To: linux-arm-kernel

Add McSPI data node to AM33XX device tree file. The McSPI module (and so
as the driver) is reused from OMAP4.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
Tested-by: Matt Porter <mporter@ti.com>
---
Changes since v1:
        - Corrected reg offset in reg DT entry.

:100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
 arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index ff3badb..065fd54 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -219,5 +219,30 @@
 			interrupt-parent = <&intc>;
 			interrupts = <91>;
 		};
+
+		spi0: spi at 48030000 {
+			compatible = "ti,omap4-mcspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x48030000 0x400>;
+			interrupt-parent = <&intc>;
+			interrupt = <65>;
+			ti,spi-num-cs = <2>;
+			ti,hwmods = "spi0";
+			status = "disabled";
+
+		};
+
+		spi1: spi at 481a0000 {
+			compatible = "ti,omap4-mcspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x481a0000 0x400>;
+			interrupt-parent = <&intc>;
+			interrupt = <125>;
+			ti,spi-num-cs = <2>;
+			ti,hwmods = "spi1";
+			status = "disabled";
+		};
 	};
 };
-- 
1.7.4.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* RE: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
  2012-09-18  5:30 ` Philip, Avinash
  (?)
@ 2012-09-26  9:47   ` Philip, Avinash
  -1 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-09-26  9:47 UTC (permalink / raw)
  To: Cousson, Benoit, linux, tony
  Cc: linux-arm-kernel, linux-kernel, linux-omap, Nori, Sekhar, Hebbar,
	Gururaja

Hi Benoit,

On Tue, Sep 18, 2012 at 11:00:28, Philip, Avinash wrote:
> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> as the driver) is reused from OMAP4.
> 	
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Tested-by: Matt Porter <mporter@ti.com>


I haven't received any comments till now for this patch. 
Do you have any comments for this patch? 

With this patch SPI is working on AM335x SoC in PIO mode.

If no comments, can you pull this patch for 3.7?

Thanks
Avinash

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-09-26  9:47   ` Philip, Avinash
  0 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-09-26  9:47 UTC (permalink / raw)
  To: Cousson, Benoit, linux, tony
  Cc: linux-arm-kernel, linux-kernel, linux-omap, Nori, Sekhar, Hebbar,
	Gururaja

Hi Benoit,

On Tue, Sep 18, 2012 at 11:00:28, Philip, Avinash wrote:
> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> as the driver) is reused from OMAP4.
> 	
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Tested-by: Matt Porter <mporter@ti.com>


I haven't received any comments till now for this patch. 
Do you have any comments for this patch? 

With this patch SPI is working on AM335x SoC in PIO mode.

If no comments, can you pull this patch for 3.7?

Thanks
Avinash

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-09-26  9:47   ` Philip, Avinash
  0 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-09-26  9:47 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Benoit,

On Tue, Sep 18, 2012 at 11:00:28, Philip, Avinash wrote:
> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> as the driver) is reused from OMAP4.
> 	
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Tested-by: Matt Porter <mporter@ti.com>


I haven't received any comments till now for this patch. 
Do you have any comments for this patch? 

With this patch SPI is working on AM335x SoC in PIO mode.

If no comments, can you pull this patch for 3.7?

Thanks
Avinash

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
  2012-09-18  5:30 ` Philip, Avinash
  (?)
@ 2012-10-19  8:24   ` Benoit Cousson
  -1 siblings, 0 replies; 19+ messages in thread
From: Benoit Cousson @ 2012-10-19  8:24 UTC (permalink / raw)
  To: Philip, Avinash
  Cc: linux, tony, linux-arm-kernel, linux-kernel, linux-omap, nsekhar,
	gururaja.hebbar

Hi Avinash,

This look good to me except the: status = "disabled".

The "disabled" should be reserved for variant that does not contain the IP.
Is it the case here?

Regards,
Benoit

On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> as the driver) is reused from OMAP4.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Tested-by: Matt Porter <mporter@ti.com>
> ---
> Changes since v1:
>         - Corrected reg offset in reg DT entry.
> 
> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index ff3badb..065fd54 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -219,5 +219,30 @@
>  			interrupt-parent = <&intc>;
>  			interrupts = <91>;
>  		};
> +
> +		spi0: spi@48030000 {
> +			compatible = "ti,omap4-mcspi";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x48030000 0x400>;
> +			interrupt-parent = <&intc>;
> +			interrupt = <65>;
> +			ti,spi-num-cs = <2>;
> +			ti,hwmods = "spi0";
> +			status = "disabled";
> +
> +		};
> +
> +		spi1: spi@481a0000 {
> +			compatible = "ti,omap4-mcspi";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x481a0000 0x400>;
> +			interrupt-parent = <&intc>;
> +			interrupt = <125>;
> +			ti,spi-num-cs = <2>;
> +			ti,hwmods = "spi1";
> +			status = "disabled";
> +		};
>  	};
>  };
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19  8:24   ` Benoit Cousson
  0 siblings, 0 replies; 19+ messages in thread
From: Benoit Cousson @ 2012-10-19  8:24 UTC (permalink / raw)
  To: Philip, Avinash
  Cc: linux, tony, linux-arm-kernel, linux-kernel, linux-omap, nsekhar,
	gururaja.hebbar

Hi Avinash,

This look good to me except the: status = "disabled".

The "disabled" should be reserved for variant that does not contain the IP.
Is it the case here?

Regards,
Benoit

On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> as the driver) is reused from OMAP4.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Tested-by: Matt Porter <mporter@ti.com>
> ---
> Changes since v1:
>         - Corrected reg offset in reg DT entry.
> 
> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index ff3badb..065fd54 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -219,5 +219,30 @@
>  			interrupt-parent = <&intc>;
>  			interrupts = <91>;
>  		};
> +
> +		spi0: spi@48030000 {
> +			compatible = "ti,omap4-mcspi";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x48030000 0x400>;
> +			interrupt-parent = <&intc>;
> +			interrupt = <65>;
> +			ti,spi-num-cs = <2>;
> +			ti,hwmods = "spi0";
> +			status = "disabled";
> +
> +		};
> +
> +		spi1: spi@481a0000 {
> +			compatible = "ti,omap4-mcspi";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x481a0000 0x400>;
> +			interrupt-parent = <&intc>;
> +			interrupt = <125>;
> +			ti,spi-num-cs = <2>;
> +			ti,hwmods = "spi1";
> +			status = "disabled";
> +		};
>  	};
>  };
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19  8:24   ` Benoit Cousson
  0 siblings, 0 replies; 19+ messages in thread
From: Benoit Cousson @ 2012-10-19  8:24 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Avinash,

This look good to me except the: status = "disabled".

The "disabled" should be reserved for variant that does not contain the IP.
Is it the case here?

Regards,
Benoit

On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> as the driver) is reused from OMAP4.
> 
> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> Tested-by: Matt Porter <mporter@ti.com>
> ---
> Changes since v1:
>         - Corrected reg offset in reg DT entry.
> 
> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index ff3badb..065fd54 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -219,5 +219,30 @@
>  			interrupt-parent = <&intc>;
>  			interrupts = <91>;
>  		};
> +
> +		spi0: spi at 48030000 {
> +			compatible = "ti,omap4-mcspi";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x48030000 0x400>;
> +			interrupt-parent = <&intc>;
> +			interrupt = <65>;
> +			ti,spi-num-cs = <2>;
> +			ti,hwmods = "spi0";
> +			status = "disabled";
> +
> +		};
> +
> +		spi1: spi at 481a0000 {
> +			compatible = "ti,omap4-mcspi";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x481a0000 0x400>;
> +			interrupt-parent = <&intc>;
> +			interrupt = <125>;
> +			ti,spi-num-cs = <2>;
> +			ti,hwmods = "spi1";
> +			status = "disabled";
> +		};
>  	};
>  };
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
  2012-10-19  8:24   ` Benoit Cousson
  (?)
@ 2012-10-19 10:01     ` Philip, Avinash
  -1 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-10-19 10:01 UTC (permalink / raw)
  To: Cousson, Benoit
  Cc: linux, tony, linux-arm-kernel, linux-kernel, linux-omap, Nori,
	Sekhar, Hebbar, Gururaja

On Fri, Oct 19, 2012 at 13:54:15, Cousson, Benoit wrote:
> Hi Avinash,
> 
> This look good to me except the: status = "disabled".

status = "disabled" in soc .dtsi  file to make sure that IP driver
won't loaded unless if IP used. 
So from board .dts file status = "okay" should be set if IP being used.

Follows the uart1 case in AM33xx dtsi.

uart1 also sets status = "disabled"; in am33xx.dtsi & set status = "okay";
in am335x-evm.dts.

Thanks
Avinash

> 
> The "disabled" should be reserved for variant that does not contain the IP.
> Is it the case here?
> 
> Regards,
> Benoit
> 
> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> > Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> > as the driver) is reused from OMAP4.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Tested-by: Matt Porter <mporter@ti.com>
> > ---
> > Changes since v1:
> >         - Corrected reg offset in reg DT entry.
> > 
> > :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
> >  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
> >  1 files changed, 25 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > index ff3badb..065fd54 100644
> > --- a/arch/arm/boot/dts/am33xx.dtsi
> > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > @@ -219,5 +219,30 @@
> >  			interrupt-parent = <&intc>;
> >  			interrupts = <91>;
> >  		};
> > +
> > +		spi0: spi@48030000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x48030000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <65>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi0";
> > +			status = "disabled";
> > +
> > +		};
> > +
> > +		spi1: spi@481a0000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x481a0000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <125>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi1";
> > +			status = "disabled";
> > +		};
> >  	};
> >  };
> > 
> 
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19 10:01     ` Philip, Avinash
  0 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-10-19 10:01 UTC (permalink / raw)
  To: Cousson, Benoit
  Cc: linux, tony, linux-arm-kernel, linux-kernel, linux-omap, Nori,
	Sekhar, Hebbar, Gururaja

On Fri, Oct 19, 2012 at 13:54:15, Cousson, Benoit wrote:
> Hi Avinash,
> 
> This look good to me except the: status = "disabled".

status = "disabled" in soc .dtsi  file to make sure that IP driver
won't loaded unless if IP used. 
So from board .dts file status = "okay" should be set if IP being used.

Follows the uart1 case in AM33xx dtsi.

uart1 also sets status = "disabled"; in am33xx.dtsi & set status = "okay";
in am335x-evm.dts.

Thanks
Avinash

> 
> The "disabled" should be reserved for variant that does not contain the IP.
> Is it the case here?
> 
> Regards,
> Benoit
> 
> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> > Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> > as the driver) is reused from OMAP4.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Tested-by: Matt Porter <mporter@ti.com>
> > ---
> > Changes since v1:
> >         - Corrected reg offset in reg DT entry.
> > 
> > :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
> >  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
> >  1 files changed, 25 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > index ff3badb..065fd54 100644
> > --- a/arch/arm/boot/dts/am33xx.dtsi
> > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > @@ -219,5 +219,30 @@
> >  			interrupt-parent = <&intc>;
> >  			interrupts = <91>;
> >  		};
> > +
> > +		spi0: spi@48030000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x48030000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <65>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi0";
> > +			status = "disabled";
> > +
> > +		};
> > +
> > +		spi1: spi@481a0000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x481a0000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <125>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi1";
> > +			status = "disabled";
> > +		};
> >  	};
> >  };
> > 
> 
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19 10:01     ` Philip, Avinash
  0 siblings, 0 replies; 19+ messages in thread
From: Philip, Avinash @ 2012-10-19 10:01 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Oct 19, 2012 at 13:54:15, Cousson, Benoit wrote:
> Hi Avinash,
> 
> This look good to me except the: status = "disabled".

status = "disabled" in soc .dtsi  file to make sure that IP driver
won't loaded unless if IP used. 
So from board .dts file status = "okay" should be set if IP being used.

Follows the uart1 case in AM33xx dtsi.

uart1 also sets status = "disabled"; in am33xx.dtsi & set status = "okay";
in am335x-evm.dts.

Thanks
Avinash

> 
> The "disabled" should be reserved for variant that does not contain the IP.
> Is it the case here?
> 
> Regards,
> Benoit
> 
> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> > Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> > as the driver) is reused from OMAP4.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Tested-by: Matt Porter <mporter@ti.com>
> > ---
> > Changes since v1:
> >         - Corrected reg offset in reg DT entry.
> > 
> > :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
> >  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
> >  1 files changed, 25 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > index ff3badb..065fd54 100644
> > --- a/arch/arm/boot/dts/am33xx.dtsi
> > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > @@ -219,5 +219,30 @@
> >  			interrupt-parent = <&intc>;
> >  			interrupts = <91>;
> >  		};
> > +
> > +		spi0: spi at 48030000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x48030000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <65>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi0";
> > +			status = "disabled";
> > +
> > +		};
> > +
> > +		spi1: spi at 481a0000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x481a0000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <125>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi1";
> > +			status = "disabled";
> > +		};
> >  	};
> >  };
> > 
> 
> 

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
  2012-10-19  8:24   ` Benoit Cousson
@ 2012-10-19 11:30     ` Matt Porter
  -1 siblings, 0 replies; 19+ messages in thread
From: Matt Porter @ 2012-10-19 11:30 UTC (permalink / raw)
  To: Benoit Cousson
  Cc: Philip, Avinash, linux, tony, linux-arm-kernel, linux-kernel,
	linux-omap, nsekhar, gururaja.hebbar

On Fri, Oct 19, 2012 at 10:24:15AM +0200, Benoit Cousson wrote:
> Hi Avinash,
> 
> This look good to me except the: status = "disabled".
> 
> The "disabled" should be reserved for variant that does not contain the IP.
> Is it the case here?

http://comments.gmane.org/gmane.linux.drivers.devicetree/18968 is what
I've been going by with the DTS support in the EDMA dmaengine series. It
does make the most sense to only enable what you need in the
<board>.dts.

-Matt

> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> > Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> > as the driver) is reused from OMAP4.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Tested-by: Matt Porter <mporter@ti.com>
> > ---
> > Changes since v1:
> >         - Corrected reg offset in reg DT entry.
> > 
> > :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
> >  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
> >  1 files changed, 25 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > index ff3badb..065fd54 100644
> > --- a/arch/arm/boot/dts/am33xx.dtsi
> > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > @@ -219,5 +219,30 @@
> >  			interrupt-parent = <&intc>;
> >  			interrupts = <91>;
> >  		};
> > +
> > +		spi0: spi@48030000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x48030000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <65>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi0";
> > +			status = "disabled";
> > +
> > +		};
> > +
> > +		spi1: spi@481a0000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x481a0000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <125>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi1";
> > +			status = "disabled";
> > +		};
> >  	};
> >  };
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19 11:30     ` Matt Porter
  0 siblings, 0 replies; 19+ messages in thread
From: Matt Porter @ 2012-10-19 11:30 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Oct 19, 2012 at 10:24:15AM +0200, Benoit Cousson wrote:
> Hi Avinash,
> 
> This look good to me except the: status = "disabled".
> 
> The "disabled" should be reserved for variant that does not contain the IP.
> Is it the case here?

http://comments.gmane.org/gmane.linux.drivers.devicetree/18968 is what
I've been going by with the DTS support in the EDMA dmaengine series. It
does make the most sense to only enable what you need in the
<board>.dts.

-Matt

> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> > Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> > as the driver) is reused from OMAP4.
> > 
> > Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> > Tested-by: Matt Porter <mporter@ti.com>
> > ---
> > Changes since v1:
> >         - Corrected reg offset in reg DT entry.
> > 
> > :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
> >  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
> >  1 files changed, 25 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> > index ff3badb..065fd54 100644
> > --- a/arch/arm/boot/dts/am33xx.dtsi
> > +++ b/arch/arm/boot/dts/am33xx.dtsi
> > @@ -219,5 +219,30 @@
> >  			interrupt-parent = <&intc>;
> >  			interrupts = <91>;
> >  		};
> > +
> > +		spi0: spi at 48030000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x48030000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <65>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi0";
> > +			status = "disabled";
> > +
> > +		};
> > +
> > +		spi1: spi at 481a0000 {
> > +			compatible = "ti,omap4-mcspi";
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +			reg = <0x481a0000 0x400>;
> > +			interrupt-parent = <&intc>;
> > +			interrupt = <125>;
> > +			ti,spi-num-cs = <2>;
> > +			ti,hwmods = "spi1";
> > +			status = "disabled";
> > +		};
> >  	};
> >  };
> > 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
  2012-10-19 11:30     ` Matt Porter
  (?)
@ 2012-10-19 12:40       ` Benoit Cousson
  -1 siblings, 0 replies; 19+ messages in thread
From: Benoit Cousson @ 2012-10-19 12:40 UTC (permalink / raw)
  To: Matt Porter
  Cc: Philip, Avinash, linux, tony, linux-arm-kernel, linux-kernel,
	linux-omap, nsekhar, gururaja.hebbar

Hi Matt,

On 10/19/2012 01:30 PM, Matt Porter wrote:
> On Fri, Oct 19, 2012 at 10:24:15AM +0200, Benoit Cousson wrote:
>> Hi Avinash,
>>
>> This look good to me except the: status = "disabled".
>>
>> The "disabled" should be reserved for variant that does not contain the IP.
>> Is it the case here?
> 
> http://comments.gmane.org/gmane.linux.drivers.devicetree/18968 is what
> I've been going by with the DTS support in the EDMA dmaengine series. It
> does make the most sense to only enable what you need in the
> <board>.dts.

Thanks, I missed that thread. That being said, there is no real rational
:-)
It seems to be a preference more than anything else.
I'm curious now, why powerpc was not really using that approach?

I'd rather explicitly disable an IP than assuming than it is disabled by
default and then enabling it in the board file. But again it is just a
different view point, since at the end it will have the same effect.

If we really want the disabled state to be the default state, why is it
not disabled in the DT fmwk by default?

Regards,
Benoit

> 
> -Matt
> 
>> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
>>> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
>>> as the driver) is reused from OMAP4.
>>>
>>> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
>>> Tested-by: Matt Porter <mporter@ti.com>
>>> ---
>>> Changes since v1:
>>>         - Corrected reg offset in reg DT entry.
>>>
>>> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
>>>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
>>>  1 files changed, 25 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
>>> index ff3badb..065fd54 100644
>>> --- a/arch/arm/boot/dts/am33xx.dtsi
>>> +++ b/arch/arm/boot/dts/am33xx.dtsi
>>> @@ -219,5 +219,30 @@
>>>  			interrupt-parent = <&intc>;
>>>  			interrupts = <91>;
>>>  		};
>>> +
>>> +		spi0: spi@48030000 {
>>> +			compatible = "ti,omap4-mcspi";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			reg = <0x48030000 0x400>;
>>> +			interrupt-parent = <&intc>;
>>> +			interrupt = <65>;
>>> +			ti,spi-num-cs = <2>;
>>> +			ti,hwmods = "spi0";
>>> +			status = "disabled";
>>> +
>>> +		};
>>> +
>>> +		spi1: spi@481a0000 {
>>> +			compatible = "ti,omap4-mcspi";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			reg = <0x481a0000 0x400>;
>>> +			interrupt-parent = <&intc>;
>>> +			interrupt = <125>;
>>> +			ti,spi-num-cs = <2>;
>>> +			ti,hwmods = "spi1";
>>> +			status = "disabled";
>>> +		};
>>>  	};
>>>  };
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19 12:40       ` Benoit Cousson
  0 siblings, 0 replies; 19+ messages in thread
From: Benoit Cousson @ 2012-10-19 12:40 UTC (permalink / raw)
  To: Matt Porter
  Cc: Philip, Avinash, linux, tony, linux-arm-kernel, linux-kernel,
	linux-omap, nsekhar, gururaja.hebbar

Hi Matt,

On 10/19/2012 01:30 PM, Matt Porter wrote:
> On Fri, Oct 19, 2012 at 10:24:15AM +0200, Benoit Cousson wrote:
>> Hi Avinash,
>>
>> This look good to me except the: status = "disabled".
>>
>> The "disabled" should be reserved for variant that does not contain the IP.
>> Is it the case here?
> 
> http://comments.gmane.org/gmane.linux.drivers.devicetree/18968 is what
> I've been going by with the DTS support in the EDMA dmaengine series. It
> does make the most sense to only enable what you need in the
> <board>.dts.

Thanks, I missed that thread. That being said, there is no real rational
:-)
It seems to be a preference more than anything else.
I'm curious now, why powerpc was not really using that approach?

I'd rather explicitly disable an IP than assuming than it is disabled by
default and then enabling it in the board file. But again it is just a
different view point, since at the end it will have the same effect.

If we really want the disabled state to be the default state, why is it
not disabled in the DT fmwk by default?

Regards,
Benoit

> 
> -Matt
> 
>> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
>>> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
>>> as the driver) is reused from OMAP4.
>>>
>>> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
>>> Tested-by: Matt Porter <mporter@ti.com>
>>> ---
>>> Changes since v1:
>>>         - Corrected reg offset in reg DT entry.
>>>
>>> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
>>>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
>>>  1 files changed, 25 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
>>> index ff3badb..065fd54 100644
>>> --- a/arch/arm/boot/dts/am33xx.dtsi
>>> +++ b/arch/arm/boot/dts/am33xx.dtsi
>>> @@ -219,5 +219,30 @@
>>>  			interrupt-parent = <&intc>;
>>>  			interrupts = <91>;
>>>  		};
>>> +
>>> +		spi0: spi@48030000 {
>>> +			compatible = "ti,omap4-mcspi";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			reg = <0x48030000 0x400>;
>>> +			interrupt-parent = <&intc>;
>>> +			interrupt = <65>;
>>> +			ti,spi-num-cs = <2>;
>>> +			ti,hwmods = "spi0";
>>> +			status = "disabled";
>>> +
>>> +		};
>>> +
>>> +		spi1: spi@481a0000 {
>>> +			compatible = "ti,omap4-mcspi";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			reg = <0x481a0000 0x400>;
>>> +			interrupt-parent = <&intc>;
>>> +			interrupt = <125>;
>>> +			ti,spi-num-cs = <2>;
>>> +			ti,hwmods = "spi1";
>>> +			status = "disabled";
>>> +		};
>>>  	};
>>>  };
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19 12:40       ` Benoit Cousson
  0 siblings, 0 replies; 19+ messages in thread
From: Benoit Cousson @ 2012-10-19 12:40 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Matt,

On 10/19/2012 01:30 PM, Matt Porter wrote:
> On Fri, Oct 19, 2012 at 10:24:15AM +0200, Benoit Cousson wrote:
>> Hi Avinash,
>>
>> This look good to me except the: status = "disabled".
>>
>> The "disabled" should be reserved for variant that does not contain the IP.
>> Is it the case here?
> 
> http://comments.gmane.org/gmane.linux.drivers.devicetree/18968 is what
> I've been going by with the DTS support in the EDMA dmaengine series. It
> does make the most sense to only enable what you need in the
> <board>.dts.

Thanks, I missed that thread. That being said, there is no real rational
:-)
It seems to be a preference more than anything else.
I'm curious now, why powerpc was not really using that approach?

I'd rather explicitly disable an IP than assuming than it is disabled by
default and then enabling it in the board file. But again it is just a
different view point, since at the end it will have the same effect.

If we really want the disabled state to be the default state, why is it
not disabled in the DT fmwk by default?

Regards,
Benoit

> 
> -Matt
> 
>> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
>>> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
>>> as the driver) is reused from OMAP4.
>>>
>>> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
>>> Tested-by: Matt Porter <mporter@ti.com>
>>> ---
>>> Changes since v1:
>>>         - Corrected reg offset in reg DT entry.
>>>
>>> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
>>>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
>>>  1 files changed, 25 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
>>> index ff3badb..065fd54 100644
>>> --- a/arch/arm/boot/dts/am33xx.dtsi
>>> +++ b/arch/arm/boot/dts/am33xx.dtsi
>>> @@ -219,5 +219,30 @@
>>>  			interrupt-parent = <&intc>;
>>>  			interrupts = <91>;
>>>  		};
>>> +
>>> +		spi0: spi at 48030000 {
>>> +			compatible = "ti,omap4-mcspi";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			reg = <0x48030000 0x400>;
>>> +			interrupt-parent = <&intc>;
>>> +			interrupt = <65>;
>>> +			ti,spi-num-cs = <2>;
>>> +			ti,hwmods = "spi0";
>>> +			status = "disabled";
>>> +
>>> +		};
>>> +
>>> +		spi1: spi at 481a0000 {
>>> +			compatible = "ti,omap4-mcspi";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +			reg = <0x481a0000 0x400>;
>>> +			interrupt-parent = <&intc>;
>>> +			interrupt = <125>;
>>> +			ti,spi-num-cs = <2>;
>>> +			ti,hwmods = "spi1";
>>> +			status = "disabled";
>>> +		};
>>>  	};
>>>  };
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
  2012-10-19 12:40       ` Benoit Cousson
@ 2012-10-19 13:51         ` Matt Porter
  -1 siblings, 0 replies; 19+ messages in thread
From: Matt Porter @ 2012-10-19 13:51 UTC (permalink / raw)
  To: Benoit Cousson
  Cc: Philip, Avinash, linux, tony, linux-arm-kernel, linux-kernel,
	linux-omap, nsekhar, gururaja.hebbar

On Fri, Oct 19, 2012 at 02:40:58PM +0200, Benoit Cousson wrote:
> Hi Matt,
> 
> On 10/19/2012 01:30 PM, Matt Porter wrote:
> > On Fri, Oct 19, 2012 at 10:24:15AM +0200, Benoit Cousson wrote:
> >> Hi Avinash,
> >>
> >> This look good to me except the: status = "disabled".
> >>
> >> The "disabled" should be reserved for variant that does not contain the IP.
> >> Is it the case here?
> > 
> > http://comments.gmane.org/gmane.linux.drivers.devicetree/18968 is what
> > I've been going by with the DTS support in the EDMA dmaengine series. It
> > does make the most sense to only enable what you need in the
> > <board>.dts.
> 
> Thanks, I missed that thread. That being said, there is no real rational
> :-)
> It seems to be a preference more than anything else.

Ok, agreed. Here's an example to attempt to illustrate why the one true
preference is to default to disabled. :) Let's say I'm a system developer
with a custom board in the default enabled model. My board makes use of,
say, mmc1 and uart1 on am33xx (oh yes, we must fix the numbering that
doesn't match the TRM) only. In that case, I develop a board.dts
with:

	uart2: serial@foo {
		status = "disabled";
	};
	.
	.
	.
	uart6: serial@bar {
		status = "disabled";
	};

	&mmc1 {
        	vmmc-supply = <&ldo3_reg>;
	};

	&mmc2 {
		status = "disabled";
	};

	&mmc3 {
		status = "disabled";
	};

In the "positive logic" case I would have a board.dts with:

	uart1: serial@foo {
		status = "okay";
	};

	&mmc1 {
		status = "okay";
        	vmmc-supply = <&ldo3_reg>;
	};
	
In the first case, the fact that there's a board specific property
present for mmc1 can almost get lost in the sea of disabled peripherals.
Extend that out to all the other devices that would be disabled in a
full board.dts and it becomes clear (at least to me) as to how ugly the
board.dts will become. My concern is that this hardware description will
quickly become unreadable to a human.

You might say it's unnecessary to disable everything, but in a
production system the developer is not going to want all these
unused devices to be instantiated. They may have a common family of
boards with a common kernel image, all with a separate dtb so they
can precisely control which items are enabled such that unused drivers
are not loaded.

So then look at the second case. At least for me, I can look at that in
the board.dts and I know that this board uses uart1 and mmc1...and
nothing else. That's a clear and concise hardware description at a
board level, but I understand that can be just considered a
preference.

> I'm curious now, why powerpc was not really using that approach?

Good question.

> I'd rather explicitly disable an IP than assuming than it is disabled by
> default and then enabling it in the board file. But again it is just a
> different view point, since at the end it will have the same effect.

Well, there's functionality then there's production usability. I'm
pointing out that there are many cases where the readability (and
therefore) usability for someone creating a new board is reduced when
devices default to enabled.

> If we really want the disabled state to be the default state, why is it
> not disabled in the DT fmwk by default?

I think it's simply answered by the fact that powerpc always took the
negative logic approach.

-Matt

> >> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> >>> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> >>> as the driver) is reused from OMAP4.
> >>>
> >>> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> >>> Tested-by: Matt Porter <mporter@ti.com>
> >>> ---
> >>> Changes since v1:
> >>>         - Corrected reg offset in reg DT entry.
> >>>
> >>> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
> >>>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
> >>>  1 files changed, 25 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> >>> index ff3badb..065fd54 100644
> >>> --- a/arch/arm/boot/dts/am33xx.dtsi
> >>> +++ b/arch/arm/boot/dts/am33xx.dtsi
> >>> @@ -219,5 +219,30 @@
> >>>  			interrupt-parent = <&intc>;
> >>>  			interrupts = <91>;
> >>>  		};
> >>> +
> >>> +		spi0: spi@48030000 {
> >>> +			compatible = "ti,omap4-mcspi";
> >>> +			#address-cells = <1>;
> >>> +			#size-cells = <0>;
> >>> +			reg = <0x48030000 0x400>;
> >>> +			interrupt-parent = <&intc>;
> >>> +			interrupt = <65>;
> >>> +			ti,spi-num-cs = <2>;
> >>> +			ti,hwmods = "spi0";
> >>> +			status = "disabled";
> >>> +
> >>> +		};
> >>> +
> >>> +		spi1: spi@481a0000 {
> >>> +			compatible = "ti,omap4-mcspi";
> >>> +			#address-cells = <1>;
> >>> +			#size-cells = <0>;
> >>> +			reg = <0x481a0000 0x400>;
> >>> +			interrupt-parent = <&intc>;
> >>> +			interrupt = <125>;
> >>> +			ti,spi-num-cs = <2>;
> >>> +			ti,hwmods = "spi1";
> >>> +			status = "disabled";
> >>> +		};
> >>>  	};
> >>>  };
> >>>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v2] arm/dts: AM33XX: Add SPI device tree data
@ 2012-10-19 13:51         ` Matt Porter
  0 siblings, 0 replies; 19+ messages in thread
From: Matt Porter @ 2012-10-19 13:51 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Oct 19, 2012 at 02:40:58PM +0200, Benoit Cousson wrote:
> Hi Matt,
> 
> On 10/19/2012 01:30 PM, Matt Porter wrote:
> > On Fri, Oct 19, 2012 at 10:24:15AM +0200, Benoit Cousson wrote:
> >> Hi Avinash,
> >>
> >> This look good to me except the: status = "disabled".
> >>
> >> The "disabled" should be reserved for variant that does not contain the IP.
> >> Is it the case here?
> > 
> > http://comments.gmane.org/gmane.linux.drivers.devicetree/18968 is what
> > I've been going by with the DTS support in the EDMA dmaengine series. It
> > does make the most sense to only enable what you need in the
> > <board>.dts.
> 
> Thanks, I missed that thread. That being said, there is no real rational
> :-)
> It seems to be a preference more than anything else.

Ok, agreed. Here's an example to attempt to illustrate why the one true
preference is to default to disabled. :) Let's say I'm a system developer
with a custom board in the default enabled model. My board makes use of,
say, mmc1 and uart1 on am33xx (oh yes, we must fix the numbering that
doesn't match the TRM) only. In that case, I develop a board.dts
with:

	uart2: serial at foo {
		status = "disabled";
	};
	.
	.
	.
	uart6: serial at bar {
		status = "disabled";
	};

	&mmc1 {
        	vmmc-supply = <&ldo3_reg>;
	};

	&mmc2 {
		status = "disabled";
	};

	&mmc3 {
		status = "disabled";
	};

In the "positive logic" case I would have a board.dts with:

	uart1: serial at foo {
		status = "okay";
	};

	&mmc1 {
		status = "okay";
        	vmmc-supply = <&ldo3_reg>;
	};
	
In the first case, the fact that there's a board specific property
present for mmc1 can almost get lost in the sea of disabled peripherals.
Extend that out to all the other devices that would be disabled in a
full board.dts and it becomes clear (at least to me) as to how ugly the
board.dts will become. My concern is that this hardware description will
quickly become unreadable to a human.

You might say it's unnecessary to disable everything, but in a
production system the developer is not going to want all these
unused devices to be instantiated. They may have a common family of
boards with a common kernel image, all with a separate dtb so they
can precisely control which items are enabled such that unused drivers
are not loaded.

So then look at the second case. At least for me, I can look at that in
the board.dts and I know that this board uses uart1 and mmc1...and
nothing else. That's a clear and concise hardware description at a
board level, but I understand that can be just considered a
preference.

> I'm curious now, why powerpc was not really using that approach?

Good question.

> I'd rather explicitly disable an IP than assuming than it is disabled by
> default and then enabling it in the board file. But again it is just a
> different view point, since at the end it will have the same effect.

Well, there's functionality then there's production usability. I'm
pointing out that there are many cases where the readability (and
therefore) usability for someone creating a new board is reduced when
devices default to enabled.

> If we really want the disabled state to be the default state, why is it
> not disabled in the DT fmwk by default?

I think it's simply answered by the fact that powerpc always took the
negative logic approach.

-Matt

> >> On 09/18/2012 07:30 AM, Philip, Avinash wrote:
> >>> Add McSPI data node to AM33XX device tree file. The McSPI module (and so
> >>> as the driver) is reused from OMAP4.
> >>>
> >>> Signed-off-by: Philip, Avinash <avinashphilip@ti.com>
> >>> Tested-by: Matt Porter <mporter@ti.com>
> >>> ---
> >>> Changes since v1:
> >>>         - Corrected reg offset in reg DT entry.
> >>>
> >>> :100644 100644 ff3badb... 065fd54... M	arch/arm/boot/dts/am33xx.dtsi
> >>>  arch/arm/boot/dts/am33xx.dtsi |   25 +++++++++++++++++++++++++
> >>>  1 files changed, 25 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> >>> index ff3badb..065fd54 100644
> >>> --- a/arch/arm/boot/dts/am33xx.dtsi
> >>> +++ b/arch/arm/boot/dts/am33xx.dtsi
> >>> @@ -219,5 +219,30 @@
> >>>  			interrupt-parent = <&intc>;
> >>>  			interrupts = <91>;
> >>>  		};
> >>> +
> >>> +		spi0: spi at 48030000 {
> >>> +			compatible = "ti,omap4-mcspi";
> >>> +			#address-cells = <1>;
> >>> +			#size-cells = <0>;
> >>> +			reg = <0x48030000 0x400>;
> >>> +			interrupt-parent = <&intc>;
> >>> +			interrupt = <65>;
> >>> +			ti,spi-num-cs = <2>;
> >>> +			ti,hwmods = "spi0";
> >>> +			status = "disabled";
> >>> +
> >>> +		};
> >>> +
> >>> +		spi1: spi at 481a0000 {
> >>> +			compatible = "ti,omap4-mcspi";
> >>> +			#address-cells = <1>;
> >>> +			#size-cells = <0>;
> >>> +			reg = <0x481a0000 0x400>;
> >>> +			interrupt-parent = <&intc>;
> >>> +			interrupt = <125>;
> >>> +			ti,spi-num-cs = <2>;
> >>> +			ti,hwmods = "spi1";
> >>> +			status = "disabled";
> >>> +		};
> >>>  	};
> >>>  };
> >>>
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> the body of a message to majordomo at vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2012-10-19 13:51 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-18  5:30 [PATCH v2] arm/dts: AM33XX: Add SPI device tree data Philip, Avinash
2012-09-18  5:30 ` Philip, Avinash
2012-09-18  5:30 ` Philip, Avinash
2012-09-26  9:47 ` Philip, Avinash
2012-09-26  9:47   ` Philip, Avinash
2012-09-26  9:47   ` Philip, Avinash
2012-10-19  8:24 ` Benoit Cousson
2012-10-19  8:24   ` Benoit Cousson
2012-10-19  8:24   ` Benoit Cousson
2012-10-19 10:01   ` Philip, Avinash
2012-10-19 10:01     ` Philip, Avinash
2012-10-19 10:01     ` Philip, Avinash
2012-10-19 11:30   ` Matt Porter
2012-10-19 11:30     ` Matt Porter
2012-10-19 12:40     ` Benoit Cousson
2012-10-19 12:40       ` Benoit Cousson
2012-10-19 12:40       ` Benoit Cousson
2012-10-19 13:51       ` Matt Porter
2012-10-19 13:51         ` Matt Porter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.