devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm,davinci: configure davinci aemif chipselects through OF
@ 2011-12-04  9:41 Heiko Schocher
       [not found] ` <1322991679-20947-1-git-send-email-hs-ynQEQJNshbs@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Heiko Schocher @ 2011-12-04  9:41 UTC (permalink / raw)
  Cc: Kevin Hilman, davinci-linux-open-source, Wolfgang Denk,
	devicetree-discuss, Sekhar Nori, grant.likely, Heiko Schocher,
	linux-arm-kernel

Signed-off-by: Heiko Schocher <hs@denx.de>
Cc: davinci-linux-open-source@linux.davincidsp.com
Cc: devicetree-discuss@lists.ozlabs.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: grant.likely@secretlab.ca
Cc: Sekhar Nori <nsekhar@ti.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Wolfgang Denk <wd@denx.de>
---
 .../devicetree/bindings/arm/davinci/aemif.txt      |   85 ++++++++++++++++
 arch/arm/mach-davinci/aemif.c                      |  105 +++++++++++++++++++-
 arch/arm/mach-davinci/include/mach/aemif.h         |    1 +
 3 files changed, 190 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/davinci/aemif.txt

diff --git a/Documentation/devicetree/bindings/arm/davinci/aemif.txt b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
new file mode 100644
index 0000000..c9ed551
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
@@ -0,0 +1,85 @@
+* Texas Instruments Davinci AEMIF
+
+This file provides information, what the device node for the
+davinci aemifa interface contain.
+
+Required properties:
+- compatible: "ti,davinci-emifa";
+- #address-cells : Should be either two or three.  The first cell is the
+                   chipselect number, and the remaining cells are the
+                   offset into the chipselect.
+- #size-cells : Either one or two, depending on how large each chipselect
+                can be.
+- ranges : Each range corresponds to a single chipselect, and cover
+           the entire access window as configured.
+
+Optional properties:
+- none
+
+Optional subnodes:
+- Chipselect setup:
+  - Required properties:
+	- compatible: "ti,davinci-cs";
+	- #address-cells = <1>;
+	- #size-cells = <1>;
+
+    Timing setup, all timings in nanoseconds
+	- cs:		chipselect (value 2,3,4 or 5)
+	- asize:	Asynchronous Data Bus Width.
+			value:
+			0: 8 bit
+			1: 16 bit
+	- ta:		Minimum Turn-Around time.
+	- rhold:	Read hold width
+	- rstrobe:	Read strobe width
+	- rsetup:	Read setup width
+	- whold:	Write hold width
+	- wstrobe:	Write strobe width
+	- wsetup:	Write setup width
+	- ew:		Extend Wait bit
+			value:
+			0: Extended wait cycles disabled.
+			1: Extended wait cycles enabled.
+	-ss:		Select Strobe bit.
+			value:
+			0: Normal Mode enabled.
+			1: Select Strobe Mode enabled.
+- CFI driver:
+  see: Documentation/devicetree/bindings/mtd/mtd-physmap.txt
+
+Example (enbw_cmc board):
+	aemif@60000000 {
+		compatible = "ti,davinci-emifa";
+		#address-cells = <2>;
+		#size-cells = <1>;
+		reg = <0x68000000 0x80000>;
+		ranges = <2 0 0x60000000 0x02000000
+			  3 0 0x62000000 0x02000000
+			  4 0 0x64000000 0x02000000
+			  5 0 0x66000000 0x02000000>;
+		cs2@0x60000000 {
+			compatible = "ti,davinci-cs";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			/* all timings in nanoseconds */
+			cs = <2>;
+			asize = <1>;
+			ta = <0>;
+			rhold = <7>;
+			rstrobe = <42>;
+			rsetup = <14>;
+			whold = <7>;
+			wstrobe = <42>;
+			wsetup = <14>;
+			ew = <0>;
+			ss = <0>;
+		};
+		flash@2,0 {
+			compatible = "cfi-flash";
+			reg = <2 0x0 0x400000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			bank-width = <2>;
+			device-width = <2>;
+		};
+	};
diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
index 1ce70a9..12c559f 100644
--- a/arch/arm/mach-davinci/aemif.c
+++ b/arch/arm/mach-davinci/aemif.c
@@ -13,12 +13,14 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
 #include <linux/time.h>
 
 #include <mach/aemif.h>
 
 /* Timing value configuration */
-
+#define ASIZE(x)	(x)
 #define TA(x)		((x) << 2)
 #define RHOLD(x)	((x) << 4)
 #define RSTROBE(x)	((x) << 7)
@@ -26,7 +28,10 @@
 #define WHOLD(x)	((x) << 17)
 #define WSTROBE(x)	((x) << 20)
 #define WSETUP(x)	((x) << 26)
+#define EW(x)		((x) << 30)
+#define SS(x)		((x) << 31)
 
+#define ASIZE_MAX	0x1
 #define TA_MAX		0x3
 #define RHOLD_MAX	0x7
 #define RSTROBE_MAX	0x3f
@@ -34,6 +39,8 @@
 #define WHOLD_MAX	0x7
 #define WSTROBE_MAX	0x3f
 #define WSETUP_MAX	0xf
+#define EW_MAX		0x1
+#define SS_MAX		0x1
 
 #define TIMING_MASK	(TA(TA_MAX) | \
 				RHOLD(RHOLD_MAX) | \
@@ -131,3 +138,99 @@ int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
 	return 0;
 }
 EXPORT_SYMBOL(davinci_aemif_setup_timing);
+
+#if defined(CONFIG_OF)
+static int dv_get_value(struct device_node *np, const char *name)
+{
+	const u32 *data;
+	int len;
+
+	data = of_get_property(np, name, &len);
+	if (data)
+		return be32_to_cpu(readl(data));
+
+	return -EINVAL;
+}
+
+static int davinci_aemif_setup_timing_of_one(struct device_node *np,
+		void __iomem *base)
+{
+	unsigned val;
+	int asize, ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
+	int ew, ss;
+	int cs;
+	unsigned offset;
+	struct clk *aemif_clk;
+	unsigned long clkrate;
+
+	aemif_clk = clk_get(NULL, "aemif");
+	if (IS_ERR(aemif_clk))
+		return PTR_ERR(aemif_clk);
+
+	clkrate = clk_get_rate(aemif_clk);
+
+	clkrate /= 1000;	/* turn clock into kHz for ease of use */
+
+	cs = dv_get_value(np, "cs");
+	if (cs < 2)
+		return -EINVAL;
+
+	offset = A1CR_OFFSET + (cs - 2) * 4;
+	asize	= dv_get_value(np, "asize");
+	ta	= aemif_calc_rate(dv_get_value(np, "ta"), clkrate, TA_MAX);
+	rhold	= aemif_calc_rate(dv_get_value(np, "rhold"), clkrate,
+			RHOLD_MAX);
+	rstrobe	= aemif_calc_rate(dv_get_value(np, "rstrobe"), clkrate,
+			RSTROBE_MAX);
+	rsetup	= aemif_calc_rate(dv_get_value(np, "rsetup"), clkrate,
+			RSETUP_MAX);
+	whold	= aemif_calc_rate(dv_get_value(np, "whold"), clkrate,
+			WHOLD_MAX);
+	wstrobe	= aemif_calc_rate(dv_get_value(np, "wstrobe"), clkrate,
+			WSTROBE_MAX);
+	wsetup	= aemif_calc_rate(dv_get_value(np, "wsetup"), clkrate,
+			WSETUP_MAX);
+	ew	= dv_get_value(np, "ew");
+	ss	= dv_get_value(np, "ss");
+
+	if (asize < 0 || ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
+			whold < 0 || wstrobe < 0 || wsetup < 0 || ew < 0 ||
+			ss < 0) {
+		pr_err("%s: cannot get suitable timings\n", __func__);
+		return -EINVAL;
+	}
+
+	val = ASIZE(asize) | TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) |
+		RSETUP(rsetup) | WHOLD(whold) | WSTROBE(wstrobe) |
+		WSETUP(wsetup) | EW(ew) | SS(ss);
+
+	__raw_writel(val, base + offset);
+
+	return 0;
+}
+
+int davinci_aemif_setup_timing_of(void)
+{
+	struct device_node *np = NULL;
+	void __iomem *base;
+
+	np =  of_find_compatible_node(NULL, NULL, "ti,davinci-emifa");
+	if (!np) {
+		pr_warning("%s: ti,davinci-emifa not found\n", __func__);
+		return 0;
+	}
+
+	base = of_iomap(np, 0);
+	for_each_compatible_node(np, NULL, "ti,davinci-cs")
+		davinci_aemif_setup_timing_of_one(np, base);
+
+	iounmap(base);
+	return 0;
+}
+#else
+int davinci_aemif_setup_timing_of(void)
+{
+	return 0;
+}
+#endif
+EXPORT_SYMBOL(davinci_aemif_setup_timing_of);
diff --git a/arch/arm/mach-davinci/include/mach/aemif.h b/arch/arm/mach-davinci/include/mach/aemif.h
index 05b2934..1538565 100644
--- a/arch/arm/mach-davinci/include/mach/aemif.h
+++ b/arch/arm/mach-davinci/include/mach/aemif.h
@@ -33,4 +33,5 @@ struct davinci_aemif_timing {
 
 int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
 					void __iomem *base, unsigned cs);
+int davinci_aemif_setup_timing_of(void);
 #endif
-- 
1.7.6.4

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
       [not found] ` <1322991679-20947-1-git-send-email-hs-ynQEQJNshbs@public.gmane.org>
@ 2011-12-04 12:25   ` Sergei Shtylyov
  2011-12-05 10:50     ` Heiko Schocher
  2011-12-04 12:33   ` Sergei Shtylyov
  2011-12-07 10:44   ` [PATCH] arm,davinci: " Nori, Sekhar
  2 siblings, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2011-12-04 12:25 UTC (permalink / raw)
  To: Heiko Schocher
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Wolfgang Denk, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

Hello.

On 04-12-2011 13:41, Heiko Schocher wrote:

> Signed-off-by: Heiko Schocher<hs-ynQEQJNshbs@public.gmane.org>
> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org
> Cc: Sekhar Nori<nsekhar-l0cyMroinI0@public.gmane.org>
> Cc: Kevin Hilman<khilman-l0cyMroinI0@public.gmane.org>
> Cc: Wolfgang Denk<wd-ynQEQJNshbs@public.gmane.org>

> diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
> index 1ce70a9..12c559f 100644
> --- a/arch/arm/mach-davinci/aemif.c
> +++ b/arch/arm/mach-davinci/aemif.c
[...]
> @@ -131,3 +138,99 @@ int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
>   	return 0;
>   }
>   EXPORT_SYMBOL(davinci_aemif_setup_timing);
> +
> +#if defined(CONFIG_OF)
> +static int dv_get_value(struct device_node *np, const char *name)
> +{
> +	const u32 *data;
> +	int len;
> +
> +	data = of_get_property(np, name,&len);
> +	if (data)
> +		return be32_to_cpu(readl(data));

    Why readl() here?! Device tree is not a peripheral device...

> +
> +	return -EINVAL;
> +}

    Isn't there already a standard helper for that, of_property_read_u32()?

WBR, Sergei

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
       [not found] ` <1322991679-20947-1-git-send-email-hs-ynQEQJNshbs@public.gmane.org>
  2011-12-04 12:25   ` [PATCH] arm, davinci: " Sergei Shtylyov
@ 2011-12-04 12:33   ` Sergei Shtylyov
       [not found]     ` <4EDB68B7.8080609-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
  2011-12-07 10:44   ` [PATCH] arm,davinci: " Nori, Sekhar
  2 siblings, 1 reply; 14+ messages in thread
From: Sergei Shtylyov @ 2011-12-04 12:33 UTC (permalink / raw)
  To: Heiko Schocher
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Wolfgang Denk, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

Hello.

On 04-12-2011 13:41, Heiko Schocher wrote:

> Signed-off-by: Heiko Schocher <hs-ynQEQJNshbs@public.gmane.org>
> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org
> Cc: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> Cc: Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org>
> Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
[...]

> diff --git a/Documentation/devicetree/bindings/arm/davinci/aemif.txt b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
> new file mode 100644
> index 0000000..c9ed551
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
> @@ -0,0 +1,85 @@
> +* Texas Instruments Davinci AEMIF
> +
> +This file provides information, what the device node for the
> +davinci aemifa interface contain.
[...]
> +Example (enbw_cmc board):
> +	aemif@60000000 {
> +		compatible = "ti,davinci-emifa";
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		reg = <0x68000000 0x80000>;
> +		ranges = <2 0 0x60000000 0x02000000
> +			  3 0 0x62000000 0x02000000
> +			  4 0 0x64000000 0x02000000
> +			  5 0 0x66000000 0x02000000>;
> +		cs2@0x60000000 {

    0x shouldn't be included.

> +			compatible = "ti,davinci-cs";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			/* all timings in nanoseconds */
> +			cs = <2>;
> +			asize = <1>;
> +			ta = <0>;
> +			rhold = <7>;
> +			rstrobe = <42>;
> +			rsetup = <14>;
> +			whold = <7>;
> +			wstrobe = <42>;
> +			wsetup = <14>;
> +			ew = <0>;
> +			ss = <0>;
> +		};
> +		flash@2,0 {

   Why you have one kind of address for cs2@60000000 node and other kind for 
this node?

WBR, Sergei

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
  2011-12-04 12:25   ` [PATCH] arm, davinci: " Sergei Shtylyov
@ 2011-12-05 10:50     ` Heiko Schocher
  0 siblings, 0 replies; 14+ messages in thread
From: Heiko Schocher @ 2011-12-05 10:50 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: devicetree-discuss, grant.likely, davinci-linux-open-source,
	Wolfgang Denk, linux-arm-kernel

Hello Sergei,

Sergei Shtylyov wrote:
> Hello.
> 
> On 04-12-2011 13:41, Heiko Schocher wrote:
> 
>> Signed-off-by: Heiko Schocher<hs@denx.de>
>> Cc: davinci-linux-open-source@linux.davincidsp.com
>> Cc: devicetree-discuss@lists.ozlabs.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: grant.likely@secretlab.ca
>> Cc: Sekhar Nori<nsekhar@ti.com>
>> Cc: Kevin Hilman<khilman@ti.com>
>> Cc: Wolfgang Denk<wd@denx.de>
> 
>> diff --git a/arch/arm/mach-davinci/aemif.c
>> b/arch/arm/mach-davinci/aemif.c
>> index 1ce70a9..12c559f 100644
>> --- a/arch/arm/mach-davinci/aemif.c
>> +++ b/arch/arm/mach-davinci/aemif.c
> [...]
>> @@ -131,3 +138,99 @@ int davinci_aemif_setup_timing(struct
>> davinci_aemif_timing *t,
>>       return 0;
>>   }
>>   EXPORT_SYMBOL(davinci_aemif_setup_timing);
>> +
>> +#if defined(CONFIG_OF)
>> +static int dv_get_value(struct device_node *np, const char *name)
>> +{
>> +    const u32 *data;
>> +    int len;
>> +
>> +    data = of_get_property(np, name,&len);
>> +    if (data)
>> +        return be32_to_cpu(readl(data));
> 
>    Why readl() here?! Device tree is not a peripheral device...
> 
>> +
>> +    return -EINVAL;
>> +}
> 
>    Isn't there already a standard helper for that, of_property_read_u32()?

Yep, fixed that.

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
       [not found]     ` <4EDB68B7.8080609-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
@ 2011-12-05 11:49       ` Heiko Schocher
  0 siblings, 0 replies; 14+ messages in thread
From: Heiko Schocher @ 2011-12-05 11:49 UTC (permalink / raw)
  To: Sergei Shtylyov
  Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/,
	Wolfgang Denk, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r

Hello Sergei,

Sergei Shtylyov wrote:
> Hello.
> 
> On 04-12-2011 13:41, Heiko Schocher wrote:
> 
>> Signed-off-by: Heiko Schocher <hs-ynQEQJNshbs@public.gmane.org>
>> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
>> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> Cc: grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org
>> Cc: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>> Cc: Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org>
>> Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
> [...]
> 
>> diff --git a/Documentation/devicetree/bindings/arm/davinci/aemif.txt
>> b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
>> new file mode 100644
>> index 0000000..c9ed551
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
>> @@ -0,0 +1,85 @@
>> +* Texas Instruments Davinci AEMIF
>> +
>> +This file provides information, what the device node for the
>> +davinci aemifa interface contain.
> [...]
>> +Example (enbw_cmc board):
>> +    aemif@60000000 {
>> +        compatible = "ti,davinci-emifa";
>> +        #address-cells = <2>;
>> +        #size-cells = <1>;
>> +        reg = <0x68000000 0x80000>;
>> +        ranges = <2 0 0x60000000 0x02000000
>> +              3 0 0x62000000 0x02000000
>> +              4 0 0x64000000 0x02000000
>> +              5 0 0x66000000 0x02000000>;
>> +        cs2@0x60000000 {
> 
>    0x shouldn't be included.

fixed.

>> +            compatible = "ti,davinci-cs";
>> +            #address-cells = <1>;
>> +            #size-cells = <1>;
>> +            /* all timings in nanoseconds */
>> +            cs = <2>;
>> +            asize = <1>;
>> +            ta = <0>;
>> +            rhold = <7>;
>> +            rstrobe = <42>;
>> +            rsetup = <14>;
>> +            whold = <7>;
>> +            wstrobe = <42>;
>> +            wsetup = <14>;
>> +            ew = <0>;
>> +            ss = <0>;
>> +        };
>> +        flash@2,0 {
> 
>   Why you have one kind of address for cs2@60000000 node and other kind
> for this node?

Typo it must be

cs2@68000010 {
[...]
}

As the chipselect registers start @0x68000000 maybe
I should add in ranges a "x 0 0x68000000 0x00080000" line? But
which value has x, as this range has no chipselect? So I could add
subnodes "cs2@680000yy" with a reg entry "reg = <x yy 4>;" with
yy = offset from 0x68000000 and drop the "cs" property?

Or are the "ti,davinci-cs" nodes no subnodes from "ti,davinci-emifa",
so I easy can use "reg = <0x68000010 4>;"?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

* RE: [PATCH] arm,davinci: configure davinci aemif chipselects through OF
       [not found] ` <1322991679-20947-1-git-send-email-hs-ynQEQJNshbs@public.gmane.org>
  2011-12-04 12:25   ` [PATCH] arm, davinci: " Sergei Shtylyov
  2011-12-04 12:33   ` Sergei Shtylyov
@ 2011-12-07 10:44   ` Nori, Sekhar
       [not found]     ` <DF0F476B391FA8409C78302C7BA518B603EFB2-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
  2 siblings, 1 reply; 14+ messages in thread
From: Nori, Sekhar @ 2011-12-07 10:44 UTC (permalink / raw)
  To: Heiko Schocher
  Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Hi Heiko,

On Sun, Dec 04, 2011 at 15:11:19, Heiko Schocher wrote:

Please provide a patch description. Nice to see device tree
support being added for DaVinci devices.

> Signed-off-by: Heiko Schocher <hs-ynQEQJNshbs@public.gmane.org>
> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> Cc: grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org
> Cc: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
> Cc: Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org>
> Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
> ---
>  .../devicetree/bindings/arm/davinci/aemif.txt      |   85 ++++++++++++++++
>  arch/arm/mach-davinci/aemif.c                      |  105 +++++++++++++++++++-
>  arch/arm/mach-davinci/include/mach/aemif.h         |    1 +
>  3 files changed, 190 insertions(+), 1 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/davinci/aemif.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/davinci/aemif.txt b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
> new file mode 100644
> index 0000000..c9ed551
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
> @@ -0,0 +1,85 @@
> +* Texas Instruments Davinci AEMIF
> +
> +This file provides information, what the device node for the
> +davinci aemifa interface contain.
           ^^^^^^
           aemif
> +
> +Required properties:
> +- compatible: "ti,davinci-emifa";
> +- #address-cells : Should be either two or three.  The first cell is the
> +                   chipselect number, and the remaining cells are the
> +                   offset into the chipselect.
> +- #size-cells : Either one or two, depending on how large each chipselect
> +                can be.
> +- ranges : Each range corresponds to a single chipselect, and cover
> +           the entire access window as configured.
> +
> +Optional properties:
> +- none
> +
> +Optional subnodes:
> +- Chipselect setup:
> +  - Required properties:
> +	- compatible: "ti,davinci-cs";
> +	- #address-cells = <1>;
> +	- #size-cells = <1>;
> +
> +    Timing setup, all timings in nanoseconds
> +	- cs:		chipselect (value 2,3,4 or 5)
> +	- asize:	Asynchronous Data Bus Width.
> +			value:
> +			0: 8 bit
> +			1: 16 bit
> +	- ta:		Minimum Turn-Around time.
> +	- rhold:	Read hold width
> +	- rstrobe:	Read strobe width
> +	- rsetup:	Read setup width
> +	- whold:	Write hold width
> +	- wstrobe:	Write strobe width
> +	- wsetup:	Write setup width
> +	- ew:		Extend Wait bit
> +			value:
> +			0: Extended wait cycles disabled.
> +			1: Extended wait cycles enabled.
> +	-ss:		Select Strobe bit.
> +			value:
> +			0: Normal Mode enabled.
> +			1: Select Strobe Mode enabled.
> +- CFI driver:
> +  see: Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> +
> +Example (enbw_cmc board):
> +	aemif@60000000 {
> +		compatible = "ti,davinci-emifa";
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		reg = <0x68000000 0x80000>;
> +		ranges = <2 0 0x60000000 0x02000000
> +			  3 0 0x62000000 0x02000000
> +			  4 0 0x64000000 0x02000000
> +			  5 0 0x66000000 0x02000000>;
> +		cs2@0x60000000 {
> +			compatible = "ti,davinci-cs";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			/* all timings in nanoseconds */
> +			cs = <2>;
> +			asize = <1>;
> +			ta = <0>;
> +			rhold = <7>;
> +			rstrobe = <42>;
> +			rsetup = <14>;
> +			whold = <7>;
> +			wstrobe = <42>;
> +			wsetup = <14>;
> +			ew = <0>;
> +			ss = <0>;
> +		};
> +		flash@2,0 {
> +			compatible = "cfi-flash";
> +			reg = <2 0x0 0x400000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			bank-width = <2>;
> +			device-width = <2>;
> +		};
> +	};
> diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
> index 1ce70a9..12c559f 100644
> --- a/arch/arm/mach-davinci/aemif.c
> +++ b/arch/arm/mach-davinci/aemif.c
> @@ -13,12 +13,14 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/time.h>
>  
>  #include <mach/aemif.h>
>  
>  /* Timing value configuration */
> -
> +#define ASIZE(x)	(x)
>  #define TA(x)		((x) << 2)
>  #define RHOLD(x)	((x) << 4)
>  #define RSTROBE(x)	((x) << 7)
> @@ -26,7 +28,10 @@
>  #define WHOLD(x)	((x) << 17)
>  #define WSTROBE(x)	((x) << 20)
>  #define WSETUP(x)	((x) << 26)
> +#define EW(x)		((x) << 30)
> +#define SS(x)		((x) << 31)

You are adding support for additional configuration
parameters which should be done in a separate patch.

>  
> +#define ASIZE_MAX	0x1
>  #define TA_MAX		0x3
>  #define RHOLD_MAX	0x7
>  #define RSTROBE_MAX	0x3f
> @@ -34,6 +39,8 @@
>  #define WHOLD_MAX	0x7
>  #define WSTROBE_MAX	0x3f
>  #define WSETUP_MAX	0xf
> +#define EW_MAX		0x1
> +#define SS_MAX		0x1
>  
>  #define TIMING_MASK	(TA(TA_MAX) | \
>  				RHOLD(RHOLD_MAX) | \
> @@ -131,3 +138,99 @@ int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
>  	return 0;
>  }
>  EXPORT_SYMBOL(davinci_aemif_setup_timing);
> +
> +#if defined(CONFIG_OF)
> +static int dv_get_value(struct device_node *np, const char *name)
> +{
> +	const u32 *data;
> +	int len;
> +
> +	data = of_get_property(np, name, &len);
> +	if (data)
> +		return be32_to_cpu(readl(data));
> +
> +	return -EINVAL;
> +}
> +
> +static int davinci_aemif_setup_timing_of_one(struct device_node *np,
> +		void __iomem *base)
> +{
> +	unsigned val;
> +	int asize, ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
> +	int ew, ss;
> +	int cs;
> +	unsigned offset;
> +	struct clk *aemif_clk;
> +	unsigned long clkrate;
> +
> +	aemif_clk = clk_get(NULL, "aemif");
> +	if (IS_ERR(aemif_clk))
> +		return PTR_ERR(aemif_clk);
> +
> +	clkrate = clk_get_rate(aemif_clk);
> +
> +	clkrate /= 1000;	/* turn clock into kHz for ease of use */
> +
> +	cs = dv_get_value(np, "cs");
> +	if (cs < 2)
> +		return -EINVAL;
> +
> +	offset = A1CR_OFFSET + (cs - 2) * 4;
> +	asize	= dv_get_value(np, "asize");
> +	ta	= aemif_calc_rate(dv_get_value(np, "ta"), clkrate, TA_MAX);
> +	rhold	= aemif_calc_rate(dv_get_value(np, "rhold"), clkrate,
> +			RHOLD_MAX);
> +	rstrobe	= aemif_calc_rate(dv_get_value(np, "rstrobe"), clkrate,
> +			RSTROBE_MAX);
> +	rsetup	= aemif_calc_rate(dv_get_value(np, "rsetup"), clkrate,
> +			RSETUP_MAX);
> +	whold	= aemif_calc_rate(dv_get_value(np, "whold"), clkrate,
> +			WHOLD_MAX);
> +	wstrobe	= aemif_calc_rate(dv_get_value(np, "wstrobe"), clkrate,
> +			WSTROBE_MAX);
> +	wsetup	= aemif_calc_rate(dv_get_value(np, "wsetup"), clkrate,
> +			WSETUP_MAX);
> +	ew	= dv_get_value(np, "ew");
> +	ss	= dv_get_value(np, "ss");
> +
> +	if (asize < 0 || ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
> +			whold < 0 || wstrobe < 0 || wsetup < 0 || ew < 0 ||
> +			ss < 0) {
> +		pr_err("%s: cannot get suitable timings\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	val = ASIZE(asize) | TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) |
> +		RSETUP(rsetup) | WHOLD(whold) | WSTROBE(wstrobe) |
> +		WSETUP(wsetup) | EW(ew) | SS(ss);
> +
> +	__raw_writel(val, base + offset);
> +
> +	return 0;
> +}

This shares a large amount of code with davinci_aemif_setup_timing().
Can you try writing this as a OF wrapper to the existing function?

Thanks,
Sekhar

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
       [not found]     ` <DF0F476B391FA8409C78302C7BA518B603EFB2-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
@ 2011-12-08  7:47       ` Heiko Schocher
       [not found]         ` <4EE06B79.7070804-ynQEQJNshbs@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Heiko Schocher @ 2011-12-08  7:47 UTC (permalink / raw)
  To: Nori, Sekhar
  Cc: Hilman, Kevin,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Hello Nori,

Nori, Sekhar wrote:
> Hi Heiko,
> 
> On Sun, Dec 04, 2011 at 15:11:19, Heiko Schocher wrote:
> 
> Please provide a patch description. Nice to see device tree
> support being added for DaVinci devices.

fixed.

>> Signed-off-by: Heiko Schocher <hs-ynQEQJNshbs@public.gmane.org>
>> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
>> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> Cc: grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org
>> Cc: Sekhar Nori <nsekhar-l0cyMroinI0@public.gmane.org>
>> Cc: Kevin Hilman <khilman-l0cyMroinI0@public.gmane.org>
>> Cc: Wolfgang Denk <wd-ynQEQJNshbs@public.gmane.org>
>> ---
>>  .../devicetree/bindings/arm/davinci/aemif.txt      |   85 ++++++++++++++++
>>  arch/arm/mach-davinci/aemif.c                      |  105 +++++++++++++++++++-
>>  arch/arm/mach-davinci/include/mach/aemif.h         |    1 +
>>  3 files changed, 190 insertions(+), 1 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/davinci/aemif.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/davinci/aemif.txt b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
>> new file mode 100644
>> index 0000000..c9ed551
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
>> @@ -0,0 +1,85 @@
>> +* Texas Instruments Davinci AEMIF
>> +
>> +This file provides information, what the device node for the
>> +davinci aemifa interface contain.
>            ^^^^^^
>            aemif

fixed, thanks.

>> +
>> +Required properties:
>> +- compatible: "ti,davinci-emifa";

Shouldn't this also be "ti,davinci-aemif" ?

[...]
>> diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
>> index 1ce70a9..12c559f 100644
>> --- a/arch/arm/mach-davinci/aemif.c
>> +++ b/arch/arm/mach-davinci/aemif.c
>> @@ -13,12 +13,14 @@
>>  #include <linux/err.h>
>>  #include <linux/clk.h>
>>  #include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>>  #include <linux/time.h>
>>  
>>  #include <mach/aemif.h>
>>  
>>  /* Timing value configuration */
>> -
>> +#define ASIZE(x)	(x)
>>  #define TA(x)		((x) << 2)
>>  #define RHOLD(x)	((x) << 4)
>>  #define RSTROBE(x)	((x) << 7)
>> @@ -26,7 +28,10 @@
>>  #define WHOLD(x)	((x) << 17)
>>  #define WSTROBE(x)	((x) << 20)
>>  #define WSETUP(x)	((x) << 26)
>> +#define EW(x)		((x) << 30)
>> +#define SS(x)		((x) << 31)
> 
> You are adding support for additional configuration
> parameters which should be done in a separate patch.

Hmm.. they are only used in the OF case ... is this split
really needed?

[...]
>> +static int davinci_aemif_setup_timing_of_one(struct device_node *np,
>> +		void __iomem *base)
>> +{
[...]
>> +	val = ASIZE(asize) | TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) |
>> +		RSETUP(rsetup) | WHOLD(whold) | WSTROBE(wstrobe) |
>> +		WSETUP(wsetup) | EW(ew) | SS(ss);
>> +
>> +	__raw_writel(val, base + offset);
>> +
>> +	return 0;
>> +}
> 
> This shares a large amount of code with davinci_aemif_setup_timing().
> Can you try writing this as a OF wrapper to the existing function?

Done.

Waiting to your response to my 2 questions above, after that sending
the v2.

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

* RE: [PATCH] arm,davinci: configure davinci aemif chipselects through OF
       [not found]         ` <4EE06B79.7070804-ynQEQJNshbs@public.gmane.org>
@ 2011-12-08  8:19           ` Nori, Sekhar
       [not found]             ` <DF0F476B391FA8409C78302C7BA518B6040A03-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Nori, Sekhar @ 2011-12-08  8:19 UTC (permalink / raw)
  To: hs-ynQEQJNshbs@public.gmane.org
  Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

On Thu, Dec 08, 2011 at 13:17:05, Heiko Schocher wrote:

> >> diff --git a/Documentation/devicetree/bindings/arm/davinci/aemif.txt b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
> >> new file mode 100644
> >> index 0000000..c9ed551
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
> >> @@ -0,0 +1,85 @@
> >> +* Texas Instruments Davinci AEMIF
> >> +
> >> +This file provides information, what the device node for the
> >> +davinci aemifa interface contain.
> >            ^^^^^^
> >            aemif
> 
> fixed, thanks.
> 
> >> +
> >> +Required properties:
> >> +- compatible: "ti,davinci-emifa";
> 
> Shouldn't this also be "ti,davinci-aemif" ?

Yes, makes sense.

> 
> [...]
> >> diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
> >> index 1ce70a9..12c559f 100644
> >> --- a/arch/arm/mach-davinci/aemif.c
> >> +++ b/arch/arm/mach-davinci/aemif.c
> >> @@ -13,12 +13,14 @@
> >>  #include <linux/err.h>
> >>  #include <linux/clk.h>
> >>  #include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_address.h>
> >>  #include <linux/time.h>
> >>  
> >>  #include <mach/aemif.h>
> >>  
> >>  /* Timing value configuration */
> >> -
> >> +#define ASIZE(x)	(x)
> >>  #define TA(x)		((x) << 2)
> >>  #define RHOLD(x)	((x) << 4)
> >>  #define RSTROBE(x)	((x) << 7)
> >> @@ -26,7 +28,10 @@
> >>  #define WHOLD(x)	((x) << 17)
> >>  #define WSTROBE(x)	((x) << 20)
> >>  #define WSETUP(x)	((x) << 26)
> >> +#define EW(x)		((x) << 30)
> >> +#define SS(x)		((x) << 31)
> > 
> > You are adding support for additional configuration
> > parameters which should be done in a separate patch.
> 
> Hmm.. they are only used in the OF case ... is this split
> really needed?

But they should also be useful in the non-OF case, no?
Why restrict their usage to the OF case?

Thanks,
Sekhar

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
       [not found]             ` <DF0F476B391FA8409C78302C7BA518B6040A03-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
@ 2011-12-08  9:06               ` Heiko Schocher
       [not found]                 ` <4EE07E27.8090602-ynQEQJNshbs@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Heiko Schocher @ 2011-12-08  9:06 UTC (permalink / raw)
  To: Nori, Sekhar
  Cc: Hilman, Kevin,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Hello Nori,

Nori, Sekhar wrote:
> On Thu, Dec 08, 2011 at 13:17:05, Heiko Schocher wrote:
> 
>>>> diff --git a/Documentation/devicetree/bindings/arm/davinci/aemif.txt b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
>>>> new file mode 100644
>>>> index 0000000..c9ed551
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/davinci/aemif.txt
>>>> @@ -0,0 +1,85 @@
>>>> +* Texas Instruments Davinci AEMIF
>>>> +
>>>> +This file provides information, what the device node for the
>>>> +davinci aemifa interface contain.
>>>            ^^^^^^
>>>            aemif
>> fixed, thanks.
>>
>>>> +
>>>> +Required properties:
>>>> +- compatible: "ti,davinci-emifa";
>> Shouldn't this also be "ti,davinci-aemif" ?
> 
> Yes, makes sense.

Ok, change this.

>> [...]
>>>> diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
>>>> index 1ce70a9..12c559f 100644
>>>> --- a/arch/arm/mach-davinci/aemif.c
>>>> +++ b/arch/arm/mach-davinci/aemif.c
>>>> @@ -13,12 +13,14 @@
>>>>  #include <linux/err.h>
>>>>  #include <linux/clk.h>
>>>>  #include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_address.h>
>>>>  #include <linux/time.h>
>>>>  
>>>>  #include <mach/aemif.h>
>>>>  
>>>>  /* Timing value configuration */
>>>> -
>>>> +#define ASIZE(x)	(x)
>>>>  #define TA(x)		((x) << 2)
>>>>  #define RHOLD(x)	((x) << 4)
>>>>  #define RSTROBE(x)	((x) << 7)
>>>> @@ -26,7 +28,10 @@
>>>>  #define WHOLD(x)	((x) << 17)
>>>>  #define WSTROBE(x)	((x) << 20)
>>>>  #define WSETUP(x)	((x) << 26)
>>>> +#define EW(x)		((x) << 30)
>>>> +#define SS(x)		((x) << 31)
>>> You are adding support for additional configuration
>>> parameters which should be done in a separate patch.
>> Hmm.. they are only used in the OF case ... is this split
>> really needed?
> 
> But they should also be useful in the non-OF case, no?
> Why restrict their usage to the OF case?

I thought there is no need, because nobody needed this yet ...
Ok, I change this too, so I have to fix this files:

$ grep -lr davinci_aemif_timing .
./arch/arm/mach-davinci/board-dm644x-evm.c
./arch/arm/mach-davinci/board-dm646x-evm.c
./arch/arm/mach-davinci/board-da850-evm.c
./arch/arm/mach-davinci/include/mach/aemif.h
./arch/arm/mach-davinci/include/mach/nand.h
./arch/arm/mach-davinci/board-da830-evm.c
./drivers/mtd/nand/davinci_nand.c

I do not know the values for this boards for the new parameters.
How to proceed? Do you know this values? Or is it Ok, if I add in
arch/arm/mach-davinci/include/mach/aemif.h
a "#define AEMIF_VALUE_NOT USED 0xff" and if the new value is set to
this I don't change this setting in the register?

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

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

* RE: [PATCH] arm,davinci: configure davinci aemif chipselects through OF
       [not found]                 ` <4EE07E27.8090602-ynQEQJNshbs@public.gmane.org>
@ 2011-12-08 10:29                   ` Nori, Sekhar
       [not found]                     ` <DF0F476B391FA8409C78302C7BA518B6040CBB-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Nori, Sekhar @ 2011-12-08 10:29 UTC (permalink / raw)
  To: hs-ynQEQJNshbs@public.gmane.org,
	gregkh-l3A5Bk7waGM@public.gmane.org, Arnd Bergmann
  Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Hi Heiko,

On Thu, Dec 08, 2011 at 14:36:47, Heiko Schocher wrote:

> >>>> diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
> >>>> index 1ce70a9..12c559f 100644
> >>>> --- a/arch/arm/mach-davinci/aemif.c
> >>>> +++ b/arch/arm/mach-davinci/aemif.c
> >>>> @@ -13,12 +13,14 @@
> >>>>  #include <linux/err.h>
> >>>>  #include <linux/clk.h>
> >>>>  #include <linux/module.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_address.h>
> >>>>  #include <linux/time.h>
> >>>>  
> >>>>  #include <mach/aemif.h>
> >>>>  
> >>>>  /* Timing value configuration */
> >>>> -
> >>>> +#define ASIZE(x)	(x)
> >>>>  #define TA(x)		((x) << 2)
> >>>>  #define RHOLD(x)	((x) << 4)
> >>>>  #define RSTROBE(x)	((x) << 7)
> >>>> @@ -26,7 +28,10 @@
> >>>>  #define WHOLD(x)	((x) << 17)
> >>>>  #define WSTROBE(x)	((x) << 20)
> >>>>  #define WSETUP(x)	((x) << 26)
> >>>> +#define EW(x)		((x) << 30)
> >>>> +#define SS(x)		((x) << 31)
> >>> You are adding support for additional configuration
> >>> parameters which should be done in a separate patch.
> >> Hmm.. they are only used in the OF case ... is this split
> >> really needed?
> > 
> > But they should also be useful in the non-OF case, no?
> > Why restrict their usage to the OF case?
> 
> I thought there is no need, because nobody needed this yet ...
> Ok, I change this too, so I have to fix this files:

True, no one configured these so far (relied on the defaults).

Also, they probably did not get added into the timing API since
they are not really timing parameters. Its better to add a separate 
configuration API for these.

It will be better to move the driver outside of arch/arm/
before adding new features. I don't know if there is a place
defined yet for "memory interface" drivers, so, drivers/misc/
may be the place.

I have added Greg and Arnd for their opinion in this.

Greg, Arnd,

Brief background:

DaVinci AEMIF is an async memory interface peripheral implemented
in arch/arm/mach-davinci/aemif.c. It helps interface to NAND, NOR
and other asynchronous memories. Currently it just provides an API
for timing value configuration. The API is invoked by the MTD NAND
driver.

Specification here: http://www.ti.com/lit/ug/sprue20c/sprue20c.pdf

We are looking at a place for this outside of architecture code.

> 
> $ grep -lr davinci_aemif_timing .
> ./arch/arm/mach-davinci/board-dm644x-evm.c
> ./arch/arm/mach-davinci/board-dm646x-evm.c
> ./arch/arm/mach-davinci/board-da850-evm.c
> ./arch/arm/mach-davinci/include/mach/aemif.h
> ./arch/arm/mach-davinci/include/mach/nand.h
> ./arch/arm/mach-davinci/board-da830-evm.c
> ./drivers/mtd/nand/davinci_nand.c
> 
> I do not know the values for this boards for the new parameters.
> How to proceed? Do you know this values? Or is it Ok, if I add in

If we add a new API, there won't be a need to touch existing
board code.

> arch/arm/mach-davinci/include/mach/aemif.h
> a "#define AEMIF_VALUE_NOT USED 0xff" and if the new value is set to
> this I don't change this setting in the register?
> 
> bye,
> Heiko

Thanks,
Sekhar

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
       [not found]                     ` <DF0F476B391FA8409C78302C7BA518B6040CBB-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
@ 2011-12-08 15:48                       ` Arnd Bergmann
       [not found]                         ` <201112081548.08548.arnd-r2nGTMty4D4@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2011-12-08 15:48 UTC (permalink / raw)
  To: Nori, Sekhar
  Cc: Hilman, Kevin,
	davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	gregkh-l3A5Bk7waGM@public.gmane.org,
	hs-ynQEQJNshbs@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

On Thursday 08 December 2011, Nori, Sekhar wrote:
> DaVinci AEMIF is an async memory interface peripheral implemented
> in arch/arm/mach-davinci/aemif.c. It helps interface to NAND, NOR
> and other asynchronous memories. Currently it just provides an API
> for timing value configuration. The API is invoked by the MTD NAND
> driver.
> 
> Specification here: http://www.ti.com/lit/ug/sprue20c/sprue20c.pdf
> 
> We are looking at a place for this outside of architecture code.

I think the best place depends on how you plan to use the sram
interface. If all memory behind the aemif is handled by mtd, I would
put the entire driver somewhere in the mtd infrastructure.

If you want it to provide endpoint devices that are handled by
distinct subsystems in Linux, I would make it an mfd multifunction
device and make the common code a driver that scans the connected
memories in order to register its child devices for each of the
subsystems.

	Arnd

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

* RE: [PATCH] arm,davinci: configure davinci aemif chipselects through OF
       [not found]                         ` <201112081548.08548.arnd-r2nGTMty4D4@public.gmane.org>
@ 2011-12-13 18:34                           ` Nori, Sekhar
  2011-12-14 14:35                             ` [PATCH] arm, davinci: " Ben Gardiner
  0 siblings, 1 reply; 14+ messages in thread
From: Nori, Sekhar @ 2011-12-13 18:34 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	gregkh-l3A5Bk7waGM@public.gmane.org,
	grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org,
	hs-ynQEQJNshbs@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Hi Arnd,

On Thu, Dec 08, 2011 at 21:18:08, Arnd Bergmann wrote:
> On Thursday 08 December 2011, Nori, Sekhar wrote:
> > DaVinci AEMIF is an async memory interface peripheral implemented
> > in arch/arm/mach-davinci/aemif.c. It helps interface to NAND, NOR
> > and other asynchronous memories. Currently it just provides an API
> > for timing value configuration. The API is invoked by the MTD NAND
> > driver.
> > 
> > Specification here: http://www.ti.com/lit/ug/sprue20c/sprue20c.pdf
> > 
> > We are looking at a place for this outside of architecture code.
> 
> I think the best place depends on how you plan to use the sram
> interface. If all memory behind the aemif is handled by mtd, I would
> put the entire driver somewhere in the mtd infrastructure.

All the boards and use cases I have come across for AEMIF so far
(5-6 years) have either used NAND or NOR memory.

> If you want it to provide endpoint devices that are handled by
> distinct subsystems in Linux, I would make it an mfd multifunction
> device and make the common code a driver that scans the connected
> memories in order to register its child devices for each of the
> subsystems.

Okay. Thanks for the explanation. Since the users of AEMIF at this
point are mtd devices, I propose moving it to drivers/mtd/davinci-aemif.c
(of course, mtd folks need to approve).

Thanks,
Sekhar

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

* Re: [PATCH] arm, davinci: configure davinci aemif chipselects through OF
  2011-12-13 18:34                           ` [PATCH] arm,davinci: " Nori, Sekhar
@ 2011-12-14 14:35                             ` Ben Gardiner
       [not found]                               ` <CAPcSpELkWJpP9-3UoNHF_aWSdd_b0FHNooNm+zF7UTjHEBC_vg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Ben Gardiner @ 2011-12-14 14:35 UTC (permalink / raw)
  To: Nori, Sekhar, Arnd Bergmann
  Cc: davinci-linux-open-source@linux.davincidsp.com,
	Sudhakar Rajashekhara, Wolfgang Denk,
	devicetree-discuss@lists.ozlabs.org, gregkh@suse.de,
	grant.likely@secretlab.ca, hs@denx.de,
	linux-arm-kernel@lists.infradead.org

Hello Arnd and Sekhar,

On Tue, Dec 13, 2011 at 1:34 PM, Nori, Sekhar <nsekhar@ti.com> wrote:
> [...]
> On Thu, Dec 08, 2011 at 21:18:08, Arnd Bergmann wrote:
>> [...]
>> If you want it to provide endpoint devices that are handled by
>> distinct subsystems in Linux, I would make it an mfd multifunction
>> device and make the common code a driver that scans the connected
>> memories in order to register its child devices for each of the
>> subsystems.
>
> Okay. Thanks for the explanation. Since the users of AEMIF at this
> point are mtd devices, I propose moving it to drivers/mtd/davinci-aemif.c
> (of course, mtd folks need to approve).

We have a vested interest in the davinci AEMIF setup facilities
in-kernel; I'm electing to pipe-up now rather than later. Sadly our
board is not yet in mainline so my opinions may be redirected to the
bit-bucket as you see fit. We are planning to post a patch series for
our complete board support -- but we can't do it right now.

The AEMIF is useful for interfacing to other asynchronous devices too;
our newest board uses it for accessing memory mapped FPGA functional
blocks via UIO and for permanent storage to a compact flash using
pata_platform. In both cases the timings and mode for the chip-select
are manually configured before the devices are registered. In both
cases the performance of the endpoints could be better preserved
across CPU freq transitions if the hooks for cpufreq transitions
recently proposed by Sudhakar [1] were part of an mfd device and hence
applicable to devices other than mtd.

Again, I apologize for requesting features for boards that are not yet
in mainline.

Best Regards,
Ben Gardiner

[1] http://article.gmane.org/gmane.linux.drivers.mtd/36876/

---
Nanometrics Inc.
http://www.nanometrics.ca

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

* RE: [PATCH] arm,davinci: configure davinci aemif chipselects through OF
       [not found]                               ` <CAPcSpELkWJpP9-3UoNHF_aWSdd_b0FHNooNm+zF7UTjHEBC_vg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-12-15 17:10                                 ` Nori, Sekhar
  0 siblings, 0 replies; 14+ messages in thread
From: Nori, Sekhar @ 2011-12-15 17:10 UTC (permalink / raw)
  To: Ben Gardiner, Arnd Bergmann
  Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org,
	Wolfgang Denk,
	devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
	gregkh-l3A5Bk7waGM@public.gmane.org,
	grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org,
	hs-ynQEQJNshbs@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org

Hi Ben,

On Wed, Dec 14, 2011 at 20:05:25, Ben Gardiner wrote:
> Hello Arnd and Sekhar,
> 
> On Tue, Dec 13, 2011 at 1:34 PM, Nori, Sekhar <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> > [...]
> > On Thu, Dec 08, 2011 at 21:18:08, Arnd Bergmann wrote:
> >> [...]
> >> If you want it to provide endpoint devices that are handled by
> >> distinct subsystems in Linux, I would make it an mfd multifunction
> >> device and make the common code a driver that scans the connected
> >> memories in order to register its child devices for each of the
> >> subsystems.
> >
> > Okay. Thanks for the explanation. Since the users of AEMIF at this
> > point are mtd devices, I propose moving it to drivers/mtd/davinci-aemif.c
> > (of course, mtd folks need to approve).
> 
> We have a vested interest in the davinci AEMIF setup facilities
> in-kernel; I'm electing to pipe-up now rather than later. Sadly our
> board is not yet in mainline so my opinions may be redirected to the
> bit-bucket as you see fit. We are planning to post a patch series for
> our complete board support -- but we can't do it right now.
> 
> The AEMIF is useful for interfacing to other asynchronous devices too;
> our newest board uses it for accessing memory mapped FPGA functional
> blocks via UIO and for permanent storage to a compact flash using
> pata_platform. In both cases the timings and mode for the chip-select
> are manually configured before the devices are registered. In both
> cases the performance of the endpoints could be better preserved
> across CPU freq transitions if the hooks for cpufreq transitions
> recently proposed by Sudhakar [1] were part of an mfd device and hence
> applicable to devices other than mtd.
> 
> Again, I apologize for requesting features for boards that are not yet
> in mainline.

No problem. Thanks for showing a use case I didn't know existed.
Now we just need to find someone to work on moving aemif to mfd
multifunction device :)

Thanks,
Sekhar

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

end of thread, other threads:[~2011-12-15 17:10 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-04  9:41 [PATCH] arm,davinci: configure davinci aemif chipselects through OF Heiko Schocher
     [not found] ` <1322991679-20947-1-git-send-email-hs-ynQEQJNshbs@public.gmane.org>
2011-12-04 12:25   ` [PATCH] arm, davinci: " Sergei Shtylyov
2011-12-05 10:50     ` Heiko Schocher
2011-12-04 12:33   ` Sergei Shtylyov
     [not found]     ` <4EDB68B7.8080609-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
2011-12-05 11:49       ` Heiko Schocher
2011-12-07 10:44   ` [PATCH] arm,davinci: " Nori, Sekhar
     [not found]     ` <DF0F476B391FA8409C78302C7BA518B603EFB2-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
2011-12-08  7:47       ` [PATCH] arm, davinci: " Heiko Schocher
     [not found]         ` <4EE06B79.7070804-ynQEQJNshbs@public.gmane.org>
2011-12-08  8:19           ` [PATCH] arm,davinci: " Nori, Sekhar
     [not found]             ` <DF0F476B391FA8409C78302C7BA518B6040A03-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
2011-12-08  9:06               ` [PATCH] arm, davinci: " Heiko Schocher
     [not found]                 ` <4EE07E27.8090602-ynQEQJNshbs@public.gmane.org>
2011-12-08 10:29                   ` [PATCH] arm,davinci: " Nori, Sekhar
     [not found]                     ` <DF0F476B391FA8409C78302C7BA518B6040CBB-Er742YJ7I/eIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
2011-12-08 15:48                       ` [PATCH] arm, davinci: " Arnd Bergmann
     [not found]                         ` <201112081548.08548.arnd-r2nGTMty4D4@public.gmane.org>
2011-12-13 18:34                           ` [PATCH] arm,davinci: " Nori, Sekhar
2011-12-14 14:35                             ` [PATCH] arm, davinci: " Ben Gardiner
     [not found]                               ` <CAPcSpELkWJpP9-3UoNHF_aWSdd_b0FHNooNm+zF7UTjHEBC_vg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-12-15 17:10                                 ` [PATCH] arm,davinci: " Nori, Sekhar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).