Skip to contentSkip to conversation

dt: device tree support for TI EMIF driver

PATCH Superseded Aneesh V 2 participants 8 messages Mar 8, 2012 – Mar 18, 2012

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Aneesh V

author
#0 lore raw

cover dt: device tree support for TI EMIF driver

This series adds device tree support for TI EMIF SDRAM controller driver. For this, a binding has been added for representing AC timing parameters and other details of LPDDR2 memories.

This series depends my series for adding EMIF driver [1]

[1] http://marc.info/?l=linux-omap&m=133122243430942&w=2

Aneesh V (4):
dt: device tree bindings for LPDDR2 memories
dt: emif: device tree bindings for TI's EMIF sdram controller
arm: dts: EMIF and LPDDR2 device tree data for OMAP4 boards
misc: emif: add device tree support to emif driver

 .../devicetree/bindings/lpddr2/lpddr2-timings.txt  |   52 ++++
 .../devicetree/bindings/lpddr2/lpddr2.txt          |  102 +++++++
 .../bindings/memory-controllers/ti/emif.txt        |   55 ++++
 arch/arm/boot/dts/elpida_ecb240abacn.dtsi          |   67 +++++
 arch/arm/boot/dts/omap4-panda.dts                  |   13 +
 arch/arm/boot/dts/omap4-sdp.dts                    |   13 +
 arch/arm/boot/dts/omap4.dtsi                       |   18 ++
 drivers/misc/emif.c                                |  289 +++++++++++++++++++-
 8 files changed, 608 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/lpddr2/lpddr2-timings.txt
 create mode 100644 Documentation/devicetree/bindings/lpddr2/lpddr2.txt
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
 create mode 100644 arch/arm/boot/dts/elpida_ecb240abacn.dtsi
author
#1 lore raw

PATCH 1/4 dt: device tree bindings for LPDDR2 memories

device tree bindings for LPDDR2 SDRAM memories compliant to JESD209-2 standard.

The 'lpddr2' binding in-turn uses another binding 'lpddr2-timings' for specifying the AC timing parameters of the memory device at different speed-bins.

Cc Rajendra Nayak <[email protected]>
Cc Benoit Cousson <[email protected]>

---
Changes since RFC v4:
- Removed two DDR3 only timing parameters that were inadvertently added in the binding

---
 .../devicetree/bindings/lpddr2/lpddr2-timings.txt  |   52 ++++++++++
 .../devicetree/bindings/lpddr2/lpddr2.txt          |  102 ++++++++++++++++++++
 2 files changed, 154 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/lpddr2/lpddr2-timings.txt
 create mode 100644 Documentation/devicetree/bindings/lpddr2/lpddr2.txt
Documentation/devicetree/bindings/lpddr2/lpddr2-timings.txt new file +52 -0
new file mode 100644
index 0000000..a48f698
@@ -0,0 +1,52 @@
+* AC timing parameters of LPDDR2(JESD209-2) memories for a given speed-bin
+
+Required properties:
+- compatible : Should be "jedec,lpddr2-timings"
+- min-freq : minimum DDR clock frequency for the speed-bin. Type is <u32>
+- max-freq : maximum DDR clock frequency for the speed-bin. Type is <u32>
+
+Optional properties:
+
+The following properties represent AC timing parameters from the memory
+data-sheet of the device for a given speed-bin. All these properties are
+of type <u32> and the default unit is ps (pico seconds). Parameters with
+a different unit have a suffix indicating the unit such as 'tRAS-max-ns'
+- tRCD
+- tWR
+- tRAS-min
+- tRRD
+- tWTR
+- tXP
+- tRTP
+- tDQSCK-max
+- tFAW
+- tZQCS
+- tZQinit
+- tRPab
+- tZQCL
+- tCKESR
+- tRAS-max-ns
+- tDQSCK-max-derated
+
+Example:
+
+timings_elpida_ECB240ABACN_400mhz: lpddr2-timings at 0 {
+	compatible 	= "jedec,lpddr2-timings";
+	min-freq	= <10000000>;
+	max-freq	= <400000000>;
+	tRPab		= <21000>;
+	tRCD		= <18000>;
+	tWR		= <15000>;
+	tRAS-min	= <42000>;
+	tRRD		= <10000>;
+	tWTR		= <7500>;
+	tXP		= <7500>;
+	tRTP		= <7500>;
+	tCKESR		= <15000>;
+	tDQSCK-max 	= <5500>;
+	tFAW		= <50000>;
+	tZQCS		= <90000>;
+	tZQCL		= <360000>;
+	tZQinit		= <1000000>;
+	tRAS-max-ns	= <70000>;
+};
Documentation/devicetree/bindings/lpddr2/lpddr2.txt new file +102 -0

102 lines not on this page · raw message

Signature · 1 line
1.7.1
author
#2 lore raw

PATCH 2/4 dt: emif: device tree bindings for TI's EMIF sdram controller

In reply to Aneesh V

EMIF - External Memory Interface - is an SDRAM controller used in TI SoCs. EMIF supports, based on the IP revision, one or more of DDR2/DDR3/LPDDR2 protocols. This binding describes a given instance of the EMIF IP and memory parts attached to it.

Cc Rajendra Nayak <[email protected]>
Cc Benoit Cousson <[email protected]>

---
Changes sice RFC v4:
- None

---
 .../bindings/memory-controllers/ti/emif.txt        |   55 ++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti/emif.txt
Documentation/devicetree/bindings/memory-controllers/ti/emif.txt new file +55 -0
new file mode 100644
index 0000000..938f8e1
@@ -0,0 +1,55 @@
+* EMIF family of TI SDRAM controllers
+
+EMIF - External Memory Interface - is an SDRAM controller used in
+TI SoCs. EMIF supports, based on the IP revision, one or more of
+DDR2/DDR3/LPDDR2 protocols. This binding describes a given instance
+of the EMIF IP and memory parts attached to it.
+
+Required properties:
+- compatible	: Should be of the form "ti,emif-<ip-rev>" where <ip-rev>
+  is the IP revision of the specific EMIF instance.
+
+- phy-type	: <u32> indicating the DDR phy type. Following are the
+  allowed values
+  <1>	: Attila PHY
+  <2>	: Intelli PHY
+
+- device-handle	: phandle to a "lpddr2" node representing the memory part
+
+- ti,hwmods	: For TI hwmods processing and omap device creation
+  the value shall be "emif<n>" where <n> is the number of the EMIF
+  instance with base 1.
+
+Optional properties:
+- cs1-used		: Have this property if CS1 of this EMIF
+  instance has a memory part attached to it. If there is a memory
+  part attached to CS1, it should be the same type as the one on CS0,
+  so there is no need to give the details of this memory part.
+
+- cal-resistor-per-cs	: Have this property if the board has one
+  calibration resistor per chip-select.
+
+- hw-caps-read-idle-ctrl: Have this property if the controller
+  supports read idle window programming
+
+- hw-caps-dll-calib-ctrl: Have this property if the controller
+  supports dll calibration control
+
+- hw-caps-ll-interface	: Have this property if the controller
+  has a low latency interface and corresponding interrupt events
+
+- hw-caps-temp-alert	: Have this property if the controller
+  has capability for generating SDRAM temperature alerts
+
+Example:
+
+emif1: emif at 0x4c000000 {
+	compatible	= "ti,emif-4d";
+	ti,hwmods	= "emif2";
+	phy-type	= <1>;
+	device-handle	= <&elpida_ECB240ABACN>;
+	cs1-used;
+	hw-caps-read-idle-ctrl;
+	hw-caps-ll-interface;
+	hw-caps-temp-alert;
+};
Signature · 1 line
1.7.1
author
#3 lore raw

PATCH 3/4 arm: dts: EMIF and LPDDR2 device tree data for OMAP4 boards

In reply to Aneesh V

Device tree data for the EMIF sdram controllers in OMAP4 and LPDDR2 memory devices attached to OMAP4 boards.

Cc Rajendra Nayak <[email protected]>
Cc Benoit Cousson <[email protected]>

---
Changes since RFC v4:
- Removed DDR3 only parameters from
elpida_ecb240abacn.dtsi

---
 arch/arm/boot/dts/elpida_ecb240abacn.dtsi |   67 +++++++++++++++++++++++++++++
 arch/arm/boot/dts/omap4-panda.dts         |   13 ++++++
 arch/arm/boot/dts/omap4-sdp.dts           |   13 ++++++
 arch/arm/boot/dts/omap4.dtsi              |   18 ++++++++
 4 files changed, 111 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/boot/dts/elpida_ecb240abacn.dtsi
arch/arm/boot/dts/elpida_ecb240abacn.dtsi new file +67 -0
new file mode 100644
index 0000000..34b9f72
@@ -0,0 +1,67 @@
+/*
+ * Common devices used in different OMAP boards
+ */
+
+/ {
+	elpida_ECB240ABACN: lpddr2 {
+		compatible 	= "Elpida,ECB240ABACN","jedec,lpddr2-s4";
+		density		= <2048>;
+		io-width	= <32>;
+
+		tRPab-min-tck	= <3>;
+		tRCD-min-tck	= <3>;
+		tWR-min-tck	= <3>;
+		tRASmin-min-tck	= <3>;
+		tRRD-min-tck	= <2>;
+		tWTR-min-tck	= <2>;
+		tXP-min-tck	= <2>;
+		tRTP-min-tck	= <2>;
+		tCKE-min-tck	= <3>;
+		tCKESR-min-tck	= <3>;
+		tFAW-min-tck	= <8>;
+
+		timings_elpida_ECB240ABACN_400mhz: lpddr2-timings at 0 {
+			compatible	= "jedec,lpddr2-timings";
+			min-freq	= <10000000>;
+			max-freq	= <400000000>;
+			tRPab		= <21000>;
+			tRCD		= <18000>;
+			tWR		= <15000>;
+			tRAS-min	= <42000>;
+			tRRD		= <10000>;
+			tWTR		= <7500>;
+			tXP		= <7500>;
+			tRTP		= <7500>;
+			tCKESR		= <15000>;
+			tDQSCK-max 	= <5500>;
+			tFAW		= <50000>;
+			tZQCS		= <90000>;
+			tZQCL		= <360000>;
+			tZQinit		= <1000000>;
+			tRAS-max-ns	= <70000>;
+			tDQSCK-max-derated = <6000>;
+		};
+
+		timings_elpida_ECB240ABACN_200mhz: lpddr2-timings at 1 {
+			compatible	= "jedec,lpddr2-timings";
+			min-freq	= <10000000>;
+			max-freq	= <200000000>;
+			tRPab		= <21000>;
+			tRCD		= <18000>;
+			tWR		= <15000>;
+			tRAS-min	= <42000>;
+			tRRD		= <10000>;
+			tWTR		= <10000>;
+			tXP		= <7500>;
+			tRTP		= <7500>;
+			tCKESR		= <15000>;
+			tDQSCK-max 	= <5500>;
+			tFAW		= <50000>;
+			tZQCS		= <90000>;
+			tZQCL		= <360000>;
+			tZQinit		= <1000000>;
+			tRAS-max-ns	= <70000>;
+			tDQSCK-max-derated = <6000>;
+		};
+	};
+};
arch/arm/boot/dts/omap4-panda.dts +13 -0
index 9755ad5..f548523 100644
@@ -8,6 +8,7 @@
 /dts-v1/;
 
 /include/ "omap4.dtsi"
+/include/ "elpida_ecb240abacn.dtsi"
 
 / {
 	model = "TI OMAP4 PandaBoard";
@@ -17,4 +18,16 @@
 		device_type = "memory";
 		reg = <0x80000000 0x40000000>; /* 1 GB */
 	};
+
+	ocp {
+		emif1: emif at 0x4c000000 {
+			cs1-used;
+			device-handle = <&elpida_ECB240ABACN>;
+		};
+
+		emif2: emif at 0x4d000000 {
+			cs1-used;
+			device-handle = <&elpida_ECB240ABACN>;
+		};
+	};
 };
arch/arm/boot/dts/omap4-sdp.dts +13 -0
index 63c6b2b..6dc08bc 100644
@@ -8,6 +8,7 @@
 /dts-v1/;
 
 /include/ "omap4.dtsi"
+/include/ "elpida_ecb240abacn.dtsi"
 
 / {
 	model = "TI OMAP4 SDP board";
@@ -17,4 +18,16 @@
 		device_type = "memory";
 		reg = <0x80000000 0x40000000>; /* 1 GB */
 	};
+
+	ocp {
+		emif1: emif at 0x4c000000 {
+			cs1-used;
+			device-handle = <&elpida_ECB240ABACN>;
+		};
+
+		emif2: emif at 0x4d000000 {
+			cs1-used;
+			device-handle = <&elpida_ECB240ABACN>;
+		};
+	};
 };
arch/arm/boot/dts/omap4.dtsi +18 -0
index 70a44f6..fe3664a 100644
@@ -127,5 +127,23 @@
 			ti,hwmods = "uart4";
 			clock-frequency = <48000000>;
 		};
+
+		emif1: emif at 0x4c000000 {
+			compatible	= "ti,emif-4d";
+			ti,hwmods	= "emif1";
+			phy-type	= <1>;
+			hw-caps-read-idle-ctrl;
+			hw-caps-ll-interface;
+			hw-caps-temp-alert;
+		};
+
+		emif2: emif at 0x4d000000 {
+			compatible	= "ti,emif-4d";
+			ti,hwmods	= "emif2";
+			phy-type	= <1>;
+			hw-caps-read-idle-ctrl;
+			hw-caps-ll-interface;
+			hw-caps-temp-alert;
+		};
 	};
 };
Signature · 1 line
1.7.1
author
#4 lore raw

PATCH 4/4 misc: emif: add device tree support to emif driver

In reply to Aneesh V
Cc Rajendra Nayak <[email protected]>
Cc Benoit Cousson <[email protected]>

---
Changes since RFC v4:
- Rebased to the latest version of EMIF series
- Replace kzalloc()/kfree() with devm_* variants

---
 drivers/misc/emif.c |  289 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 288 insertions(+), 1 deletions(-)
drivers/misc/emif.c +288 -1
index 79fb161..0aaa61e 100644
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <linux/module.h>
@@ -49,6 +50,7 @@
  *				frequency in effect at the moment)
  * @plat_data:			Pointer to saved platform data.
  * @debugfs_root:		dentry to the root folder for EMIF in debugfs
+ * @np_ddr:			Pointer to ddr device tree node
  */
 struct emif_data {
 	u8				duplicate;
@@ -63,6 +65,9 @@ struct emif_data {
 	struct emif_regs		*curr_regs;
 	struct emif_platform_data	*plat_data;
 	struct dentry			*debugfs_root;
+#if defined(CONFIG_OF)
+	struct device_node		*np_ddr;
+#endif
 };
 
 static struct emif_data *emif1;
@@ -1147,6 +1152,270 @@ static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs,
 	return valid;
 }
 
+#if defined(CONFIG_OF)
+static void __init of_get_custom_configs(struct device_node *np_emif,
+		struct emif_data *emif)
+{
+	struct emif_custom_configs	*cust_cfgs = NULL;
+	int				len;
+	const int			*lpmode, *poll_intvl;
+
+	lpmode = of_get_property(np_emif, "low-power-mode", &len);
+	poll_intvl = of_get_property(np_emif, "temp-alert-poll-interval", &len);
+
+	if (lpmode || poll_intvl)
+		cust_cfgs = devm_kzalloc(emif->dev, sizeof(*cust_cfgs),
+			GFP_KERNEL);
+
+	if (!cust_cfgs)
+		return;
+
+	if (lpmode) {
+		cust_cfgs->mask |= EMIF_CUSTOM_CONFIG_LPMODE;
+		cust_cfgs->lpmode = *lpmode;
+		of_property_read_u32(np_emif,
+				"low-power-mode-timeout-performance",
+				&cust_cfgs->lpmode_timeout_performance);
+		of_property_read_u32(np_emif,
+				"low-power-mode-timeout-power",
+				&cust_cfgs->lpmode_timeout_power);
+		of_property_read_u32(np_emif,
+				"low-power-mode-freq-threshold",
+				&cust_cfgs->lpmode_freq_threshold);
+	}
+
+	if (poll_intvl) {
+		cust_cfgs->mask |=
+				EMIF_CUSTOM_CONFIG_TEMP_ALERT_POLL_INTERVAL;
+		cust_cfgs->temp_alert_poll_interval_ms = *poll_intvl;
+	}
+
+	if (!is_custom_config_valid(cust_cfgs, emif->dev)) {
+		devm_kfree(emif->dev, cust_cfgs);
+		return;
+	}
+
+	emif->plat_data->custom_configs = cust_cfgs;
+}
+
+static void __init of_get_min_tck(struct device_node *np,
+		struct emif_data *emif)
+{
+	int			ret = 0;
+	struct lpddr2_min_tck	*min;
+
+	min = devm_kzalloc(emif->dev, sizeof(*min), GFP_KERNEL);
+	if (!min)
+		goto default_min_tck;
+
+	ret |= of_property_read_u32(np, "tRPab-min-tck", &min->tRPab);
+	ret |= of_property_read_u32(np, "tRCD-min-tck", &min->tRCD);
+	ret |= of_property_read_u32(np, "tWR-min-tck", &min->tWR);
+	ret |= of_property_read_u32(np, "tRASmin-min-tck", &min->tRASmin);
+	ret |= of_property_read_u32(np, "tRRD-min-tck", &min->tRRD);
+	ret |= of_property_read_u32(np, "tWTR-min-tck", &min->tWTR);
+	ret |= of_property_read_u32(np, "tXP-min-tck", &min->tXP);
+	ret |= of_property_read_u32(np, "tRTP-min-tck", &min->tRTP);
+	ret |= of_property_read_u32(np, "tCKE-min-tck", &min->tCKE);
+	ret |= of_property_read_u32(np, "tCKESR-min-tck", &min->tCKESR);
+	ret |= of_property_read_u32(np, "tFAW-min-tck", &min->tFAW);
+
+	if (ret) {
+		devm_kfree(emif->dev, min);
+		goto default_min_tck;
+	}
+
+	emif->plat_data->min_tck = min;
+	return;
+
+default_min_tck:
+	dev_warn(emif->dev, "%s: using default min-tck values\n", __func__);
+	emif->plat_data->min_tck = &lpddr2_min_tck;
+}
+
+static int __init of_do_get_timings(struct device_node *np,
+		struct lpddr2_timings *tim)
+{
+	int ret;
+
+	ret = of_property_read_u32(np, "max-freq", &tim->max_freq);
+	ret |= of_property_read_u32(np, "min-freq", &tim->min_freq);
+	ret |= of_property_read_u32(np, "tRPab", &tim->tRPab);
+	ret |= of_property_read_u32(np, "tRCD", &tim->tRCD);
+	ret |= of_property_read_u32(np, "tWR", &tim->tWR);
+	ret |= of_property_read_u32(np, "tRAS-min", &tim->tRAS_min);
+	ret |= of_property_read_u32(np, "tRRD", &tim->tRRD);
+	ret |= of_property_read_u32(np, "tWTR", &tim->tWTR);
+	ret |= of_property_read_u32(np, "tXP", &tim->tXP);
+	ret |= of_property_read_u32(np, "tRTP", &tim->tRTP);
+	ret |= of_property_read_u32(np, "tCKESR", &tim->tCKESR);
+	ret |= of_property_read_u32(np, "tDQSCK-max", &tim->tDQSCK_max);
+	ret |= of_property_read_u32(np, "tFAW", &tim->tFAW);
+	ret |= of_property_read_u32(np, "tZQCS", &tim->tZQCS);
+	ret |= of_property_read_u32(np, "tZQCL", &tim->tZQCL);
+	ret |= of_property_read_u32(np, "tZQinit", &tim->tZQinit);
+	ret |= of_property_read_u32(np, "tRAS-max-ns", &tim->tRAS_max_ns);
+	ret |= of_property_read_u32(np, "tDQSCK-max-derated",
+		&tim->tDQSCK_max_derated);
+
+	return ret;
+}
+
+static void __init of_get_ddr_timings(struct device_node *np_ddr,
+		struct emif_data *emif)
+{
+	struct lpddr2_timings	*timings = NULL;
+	u32			arr_sz = 0, i = 0;
+	struct device_node	*np_tim;
+	char			*tim_compat;
+
+	switch (emif->plat_data->device_info->type) {
+	case DDR_TYPE_LPDDR2_S2:
+	case DDR_TYPE_LPDDR2_S4:
+		tim_compat = "jedec,lpddr2-timings";
+		break;
+	default:
+		dev_warn(emif->dev, "%s: un-supported memory type\n", __func__);
+	}
+
+	for_each_child_of_node(np_ddr, np_tim)
+		if (of_device_is_compatible(np_tim, tim_compat))
+			arr_sz++;
+
+	if (arr_sz)
+		timings = devm_kzalloc(emif->dev, sizeof(*timings) * arr_sz,
+			GFP_KERNEL);
+
+	if (!timings)
+		goto default_timings;
+
+	for_each_child_of_node(np_ddr, np_tim) {
+		if (of_device_is_compatible(np_tim, tim_compat)) {
+			if (of_do_get_timings(np_tim, &timings[i])) {
+				devm_kfree(emif->dev, timings);
+				goto default_timings;
+			}
+			i++;
+		}
+	}
+
+	emif->plat_data->timings = timings;
+	emif->plat_data->timings_arr_size = arr_sz;
+
+	return;
+
+default_timings:
+	get_default_timings(emif);
+
+	return;
+}
+
+static void __init of_get_ddr_info(struct device_node *np_emif,
+		struct device_node *np_ddr,
+		struct ddr_device_info *dev_info)
+{
+	u32 density = 0, io_width = 0;
+	int len;
+
+	if (of_find_property(np_emif, "cs1-used", &len))
+		dev_info->cs1_used = true;
+
+	if (of_find_property(np_emif, "cal-resistor-per-cs", &len))
+		dev_info->cal_resistors_per_cs = true;
+
+	if (of_device_is_compatible(np_ddr , "jedec,lpddr2-s4"))
+		dev_info->type = DDR_TYPE_LPDDR2_S4;
+	else if (of_device_is_compatible(np_ddr , "jedec,lpddr2-s2"))
+		dev_info->type = DDR_TYPE_LPDDR2_S2;
+
+	of_property_read_u32(np_ddr, "density", &density);
+	of_property_read_u32(np_ddr, "io-width", &io_width);
+
+	/* Convert from density in Mb to the density encoding in jedc_ddr.h */
+	if (density & (density - 1))
+		dev_info->density = 0;
+	else
+		dev_info->density = __fls(density) - 5;
+
+	/* Convert from io_width in bits to io_width encoding in jedc_ddr.h */
+	if (io_width & (io_width - 1))
+		dev_info->io_width = 0;
+	else
+		dev_info->io_width = __fls(io_width) - 1;
+}
+
+static struct emif_data * __init of_get_device_details(
+		struct device_node *np_emif, struct device *dev)
+{
+	struct emif_data		*emif = NULL;
+	struct ddr_device_info		*dev_info = NULL;
+	struct emif_platform_data	*pd = NULL;
+	struct device_node		*np_ddr;
+	int				len;
+
+	np_ddr = of_parse_phandle(np_emif, "device-handle", 0);
+	if (!np_ddr)
+		goto error;
+	emif	= devm_kzalloc(dev, sizeof(struct emif_data), GFP_KERNEL);
+	pd	= devm_kzalloc(dev, sizeof(*pd), GFP_KERNEL);
+	dev_info = devm_kzalloc(dev, sizeof(*dev_info), GFP_KERNEL);
+
+	if (!emif || !pd || !dev_info) {
+		dev_err(dev, "%s: of_get_device_details() failure!!\n",
+			__func__);
+		goto error;
+	}
+
+	emif->plat_data		= pd;
+	pd->device_info		= dev_info;
+	emif->dev		= dev;
+	emif->np_ddr		= np_ddr;
+	emif->temperature_level	= SDRAM_TEMP_NOMINAL;
+
+	if (of_device_is_compatible(np_emif, "ti,emif-4d"))
+		emif->plat_data->ip_rev = EMIF_4D;
+	else if (of_device_is_compatible(np_emif, "ti,emif-4d5"))
+		emif->plat_data->ip_rev = EMIF_4D5;
+
+	of_property_read_u32(np_emif, "phy-type", &pd->phy_type);
+
+	if (of_find_property(np_emif, "hw-caps-ll-interface", &len))
+		pd->hw_caps |= EMIF_HW_CAPS_LL_INTERFACE;
+
+	of_get_ddr_info(np_emif, np_ddr, dev_info);
+	if (!is_dev_data_valid(pd->device_info->type, pd->device_info->density,
+			pd->device_info->io_width, pd->phy_type, pd->ip_rev,
+			emif->dev)) {
+		dev_err(dev, "%s: invalid device data!!\n", __func__);
+		goto error;
+	}
+	/*
+	 * For EMIF instances other than EMIF1 see if the devices connected
+	 * are exactly same as on EMIF1(which is typically the case). If so,
+	 * mark it as a duplicate of EMIF1. This will save some memory and
+	 * computation.
+	 */
+	if (emif1 && emif1->np_ddr == np_ddr) {
+		emif->duplicate = true;
+		goto out;
+	} else if (emif1) {
+		dev_warn(emif->dev, "%s: Non-symmetric DDR geometry\n",
+			__func__);
+	}
+
+
+	of_get_custom_configs(np_emif, emif);
+	of_get_ddr_timings(np_ddr, emif);
+	of_get_min_tck(np_ddr, emif);
+	goto out;
+
+error:
+	return NULL;
+out:
+	return emif;
+}
+#endif
+
 static struct emif_data * __init get_device_details(
 		struct platform_device *pdev)
 {
@@ -1266,7 +1535,13 @@ static int __init emif_probe(struct platform_device *pdev)
 	struct resource		*res;
 	int			irq;
 
-	emif = get_device_details(pdev);
+#if defined(CONFIG_OF)
+	if (pdev->dev.of_node)
+		emif = of_get_device_details(pdev->dev.of_node, &pdev->dev);
+	else
+#endif
+		emif = get_device_details(pdev);
+
 	if (!emif) {
 		pr_err("%s: error getting device data\n", __func__);
 		goto error;
@@ -1643,11 +1918,23 @@ static void __attribute__((unused)) freq_post_notify_handling(void)
 	spin_unlock_irqrestore(&emif_lock, irq_state);
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id emif_of_match[] = {
+		{ .compatible = "ti,emif-4d" },
+		{ .compatible = "ti,emif-4d5" },
+		{},
+};
+MODULE_DEVICE_TABLE(of, emif_of_match);
+#endif
+
 static struct platform_driver emif_driver = {
 	.remove		= __exit_p(emif_remove),
 	.shutdown	= emif_shutdown,
 	.driver = {
 		.name = "emif",
+#if defined(CONFIG_OF)
+		.of_match_table = of_match_ptr(emif_of_match),
+#endif
 	},
 };
 
Signature · 1 line
1.7.1
#5 lore raw

PATCH 4/4 misc: emif: add device tree support to emif driver

Quoted 32 lines · Aneesh V
Cc Rajendra Nayak <[email protected]>
Cc Benoit Cousson <[email protected]>

---
Changes since RFC v4:
- Rebased to the latest version of EMIF series
- Replace kzalloc()/kfree() with devm_* variants

---
 drivers/misc/emif.c |  289 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 288 insertions(+), 1 deletions(-)
drivers/misc/emif.c +2 -0
index 79fb161..0aaa61e 100644
@@ -18,6 +18,7 @@
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
+#include <linux/of.h>
 #include <linux/debugfs.h>
 #include <linux/seq_file.h>
 #include <linux/module.h>
@@ -49,6 +50,7 @@
  *				frequency in effect at the moment)
  * @plat_data:			Pointer to saved platform data.
  * @debugfs_root:		dentry to the root folder for EMIF in debugfs
+ * @np_ddr:			Pointer to ddr device tree node
  */
 struct emif_data {
 	u8				duplicate;

drivers/misc/emif.c struct emif_data { · line 70

Quoted 6 lines
drivers/misc/emif.c +3 -0
@@ -63,6 +65,9 @@ struct emif_data {
 	struct emif_regs		*curr_regs;
 	struct emif_platform_data	*plat_data;
 	struct dentry			*debugfs_root;
+#if defined(CONFIG_OF)
+	struct device_node		*np_ddr;
+#endif

Don't bother with the #if/#endif wrapper here.

drivers/misc/emif.c static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs, · line 1156

Quoted 9 lines
drivers/misc/emif.c +2 -0
 };
 
 static struct emif_data *emif1;
@@ -1147,6 +1152,270 @@ static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs,
 	return valid;
 }
 
+#if defined(CONFIG_OF)
+static void __init of_get_custom_configs(struct device_node *np_emif,

__devinit. Same through the rest of the file.

[...]

drivers/misc/emif.c static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs, · line 1419

Quoted 7 lines
drivers/misc/emif.c +6 -0
+	return NULL;
+out:
+	return emif;
+}
+#endif
+
 static struct emif_data * __init get_device_details(

This function also must be __devinit

Quoted 22 lines
drivers/misc/emif.c +8 -1
 		struct platform_device *pdev)
 {
@@ -1266,7 +1535,13 @@ static int __init emif_probe(struct platform_device *pdev)
 	struct resource		*res;
 	int			irq;
 
-	emif = get_device_details(pdev);
+#if defined(CONFIG_OF)
+	if (pdev->dev.of_node)
+		emif = of_get_device_details(pdev->dev.of_node, &pdev->dev);
+	else
+#endif
+		emif = get_device_details(pdev);
+
 	if (!emif) {
 		pr_err("%s: error getting device data\n", __func__);
 		goto error;
@@ -1643,11 +1918,23 @@ static void __attribute__((unused)) freq_post_notify_handling(void)
 	spin_unlock_irqrestore(&emif_lock, irq_state);
 }
 
+#if defined(CONFIG_OF)

drivers/misc/emif.c static void __attribute__((unused)) freq_post_notify_handling(void) · line 1931

Quoted 10 lines
drivers/misc/emif.c +8 -0
+static const struct of_device_id emif_of_match[] = {
+		{ .compatible = "ti,emif-4d" },
+		{ .compatible = "ti,emif-4d5" },
+		{},
+};
+MODULE_DEVICE_TABLE(of, emif_of_match);
+#endif
+
 static struct platform_driver emif_driver = {
 	.remove		= __exit_p(emif_remove),

Not part of this patch I realize, but this is a bug. .remove must be in the __devexit section and the __devexit_p() wrapper must be used. Similarly, emif_probe must be in the __devinit section.

drivers/misc/emif.c static void __attribute__((unused)) freq_post_notify_handling(void) · line 1937

Quoted 6 lines
drivers/misc/emif.c +3 -0
 	.shutdown	= emif_shutdown,
 	.driver = {
 		.name = "emif",
+#if defined(CONFIG_OF)
+		.of_match_table = of_match_ptr(emif_of_match),
+#endif

The of_match_ptr() macro makes the #if/#endif wrapper redundant.

Otherwise, on brief review this patch looks right.

g.

Quoted 11 lines
drivers/misc/emif.c +0 -0
 	},
 };
 
Signature · 7 lines
1.7.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 2 lines
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies,Ltd.
author
#6 lore raw

PATCH 4/4 misc: emif: add device tree support to emif driver

Hi Grant,

Quoted 33 lines · Grant Likely
Quoted 32 lines · Aneesh V
Cc Rajendra Nayak<[email protected]>
Cc Benoit Cousson<[email protected]>

---
Changes since RFC v4:
- Rebased to the latest version of EMIF series
- Replace kzalloc()/kfree() with devm_* variants

---
  drivers/misc/emif.c |  289 ++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 files changed, 288 insertions(+), 1 deletions(-)
drivers/misc/emif.c +2 -0
index 79fb161..0aaa61e 100644
@@ -18,6 +18,7 @@
  #include<linux/platform_device.h>
  #include<linux/interrupt.h>
  #include<linux/slab.h>
+#include<linux/of.h>
  #include<linux/debugfs.h>
  #include<linux/seq_file.h>
  #include<linux/module.h>
@@ -49,6 +50,7 @@
   *				frequency in effect at the moment)
   * @plat_data:			Pointer to saved platform data.
   * @debugfs_root:		dentry to the root folder for EMIF in debugfs
+ * @np_ddr:			Pointer to ddr device tree node
   */
  struct emif_data {
  	u8				duplicate;
@@ -63,6 +65,9 @@ struct emif_data {
Quoted 8 lines

drivers/misc/emif.c struct emif_data { · line 70

Quoted 6 lines
drivers/misc/emif.c +3 -0
  	struct emif_regs		*curr_regs;
  	struct emif_platform_data	*plat_data;
  	struct dentry			*debugfs_root;
+#if defined(CONFIG_OF)
+	struct device_node		*np_ddr;
+#endif

Don't bother with the #if/#endif wrapper here.

Ok.

Quoted 8 lines
Quoted 8 lines
drivers/misc/emif.c +0 -0
  };
 
  static struct emif_data *emif1;
@@ -1147,6 +1152,270 @@ static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs,
  	return valid;
  }
 
Quoted 4 lines

drivers/misc/emif.c static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs, · line 1156

Quoted 2 lines
drivers/misc/emif.c +2 -0
+#if defined(CONFIG_OF)
+static void __init of_get_custom_configs(struct device_node *np_emif,

__devinit. Same through the rest of the file.

I am not sure if we need __devinit. I see that __devinit will not have any effect if CONFIG_HOTPLUG is enabled and CONFIG_HOTPLUG is always enabled in our configuration. EMIF devices are not hot-pluggable devices and are always added at boot-time from the board file. They are on-chip IP modules and dynamic discovery and addition doesn't make sense for them. So, can't we save some memory by making them __init.

AFAIU, __init doesn't have any effect in a module. However, I can make that more explicit by using '__init_or_module'. Is that ok?

Quoted 33 lines

[...]

drivers/misc/emif.c static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs, · line 1419

Quoted 7 lines
drivers/misc/emif.c +6 -0
+	return NULL;
+out:
+	return emif;
+}
+#endif
+
  static struct emif_data * __init get_device_details(

This function also must be __devinit

Quoted 21 lines
  		struct platform_device *pdev)
  {
drivers/misc/emif.c +7 -1
@@ -1266,7 +1535,13 @@ static int __init emif_probe(struct platform_device *pdev)
  	struct resource		*res;
  	int			irq;
 
-	emif = get_device_details(pdev);
+#if defined(CONFIG_OF)
+	if (pdev->dev.of_node)
+		emif = of_get_device_details(pdev->dev.of_node,&pdev->dev);
+	else
+#endif
+		emif = get_device_details(pdev);
+
  	if (!emif) {
  		pr_err("%s: error getting device data\n", __func__);
  		goto error;
@@ -1643,11 +1918,23 @@ static void __attribute__((unused)) freq_post_notify_handling(void)
  	spin_unlock_irqrestore(&emif_lock, irq_state);
  }
 
Quoted 15 lines

drivers/misc/emif.c static void __attribute__((unused)) freq_post_notify_handling(void) · line 1931

Quoted 11 lines
drivers/misc/emif.c +9 -0
+#if defined(CONFIG_OF)
+static const struct of_device_id emif_of_match[] = {
+		{ .compatible = "ti,emif-4d" },
+		{ .compatible = "ti,emif-4d5" },
+		{},
+};
+MODULE_DEVICE_TABLE(of, emif_of_match);
+#endif
+
  static struct platform_driver emif_driver = {
  	.remove		= __exit_p(emif_remove),

Not part of this patch I realize, but this is a bug. .remove must be in the __devexit section and the __devexit_p() wrapper must be used. Similarly, emif_probe must be in the __devinit section.

Again, in our case remove() is not needed unless the driver is built as a module because the devices will never be un-registered. When it is built as a module the effect is same for both:

#if defined(MODULE) || defined(CONFIG_HOTPLUG)
#define __devexit_p(x) x
#else
#define __devexit_p(x) NULL
#endif

#ifdef MODULE
#define __exit_p(x) x
#else
#define __exit_p(x) NULL
#endif

Quoted 9 lines

drivers/misc/emif.c static void __attribute__((unused)) freq_post_notify_handling(void) · line 1937

Quoted 7 lines
drivers/misc/emif.c +3 -0
  	.shutdown	= emif_shutdown,
  	.driver = {
  		.name = "emif",
+#if defined(CONFIG_OF)
+		.of_match_table = of_match_ptr(emif_of_match),
+#endif

The of_match_ptr() macro makes the #if/#endif wrapper redundant.

Ok. I will remove it.

Quoted 2 lines

Otherwise, on brief review this patch looks right.

Thanks for the review. Are you ok with the bindings too?

br,
Aneesh

#7 lore raw

PATCH 4/4 misc: emif: add device tree support to emif driver

Quoted 119 lines · Aneesh V

Hi Grant,

Quoted 33 lines · Grant Likely
Quoted 32 lines · Aneesh V
Cc Rajendra Nayak<[email protected]>
Cc Benoit Cousson<[email protected]>

---
Changes since RFC v4:
- Rebased to the latest version of EMIF series
- Replace kzalloc()/kfree() with devm_* variants

---
  drivers/misc/emif.c |  289 ++++++++++++++++++++++++++++++++++++++++++++++++++-
  1 files changed, 288 insertions(+), 1 deletions(-)
drivers/misc/emif.c +2 -0
index 79fb161..0aaa61e 100644
@@ -18,6 +18,7 @@
  #include<linux/platform_device.h>
  #include<linux/interrupt.h>
  #include<linux/slab.h>
+#include<linux/of.h>
  #include<linux/debugfs.h>
  #include<linux/seq_file.h>
  #include<linux/module.h>
@@ -49,6 +50,7 @@
   *				frequency in effect at the moment)
   * @plat_data:			Pointer to saved platform data.
   * @debugfs_root:		dentry to the root folder for EMIF in debugfs
+ * @np_ddr:			Pointer to ddr device tree node
   */
  struct emif_data {
  	u8				duplicate;
@@ -63,6 +65,9 @@ struct emif_data {
Quoted 8 lines

drivers/misc/emif.c struct emif_data { · line 70

Quoted 6 lines
drivers/misc/emif.c +3 -0
  	struct emif_regs		*curr_regs;
  	struct emif_platform_data	*plat_data;
  	struct dentry			*debugfs_root;
+#if defined(CONFIG_OF)
+	struct device_node		*np_ddr;
+#endif

Don't bother with the #if/#endif wrapper here.

Ok.

Quoted 8 lines
Quoted 8 lines
drivers/misc/emif.c +0 -0
  };
 
  static struct emif_data *emif1;
@@ -1147,6 +1152,270 @@ static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs,
  	return valid;
  }
 
Quoted 4 lines

drivers/misc/emif.c static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs, · line 1156

Quoted 2 lines
drivers/misc/emif.c +2 -0
+#if defined(CONFIG_OF)
+static void __init of_get_custom_configs(struct device_node *np_emif,

__devinit. Same through the rest of the file.

I am not sure if we need __devinit. I see that __devinit will not have any effect if CONFIG_HOTPLUG is enabled and CONFIG_HOTPLUG is always enabled in our configuration. EMIF devices are not hot-pluggable devices and are always added at boot-time from the board file. They are on-chip IP modules and dynamic discovery and addition doesn't make sense for them. So, can't we save some memory by making them __init.

AFAIU, __init doesn't have any effect in a module. However, I can make that more explicit by using '__init_or_module'. Is that ok?

Quoted 33 lines

[...]

drivers/misc/emif.c static int is_custom_config_valid(struct emif_custom_configs *cust_cfgs, · line 1419

Quoted 7 lines
drivers/misc/emif.c +6 -0
+	return NULL;
+out:
+	return emif;
+}
+#endif
+
  static struct emif_data * __init get_device_details(

This function also must be __devinit

Quoted 21 lines
  		struct platform_device *pdev)
  {
drivers/misc/emif.c +7 -1
@@ -1266,7 +1535,13 @@ static int __init emif_probe(struct platform_device *pdev)
  	struct resource		*res;
  	int			irq;
 
-	emif = get_device_details(pdev);
+#if defined(CONFIG_OF)
+	if (pdev->dev.of_node)
+		emif = of_get_device_details(pdev->dev.of_node,&pdev->dev);
+	else
+#endif
+		emif = get_device_details(pdev);
+
  	if (!emif) {
  		pr_err("%s: error getting device data\n", __func__);
  		goto error;
@@ -1643,11 +1918,23 @@ static void __attribute__((unused)) freq_post_notify_handling(void)
  	spin_unlock_irqrestore(&emif_lock, irq_state);
  }
 
Quoted 15 lines

drivers/misc/emif.c static void __attribute__((unused)) freq_post_notify_handling(void) · line 1931

Quoted 11 lines
drivers/misc/emif.c +9 -0
+#if defined(CONFIG_OF)
+static const struct of_device_id emif_of_match[] = {
+		{ .compatible = "ti,emif-4d" },
+		{ .compatible = "ti,emif-4d5" },
+		{},
+};
+MODULE_DEVICE_TABLE(of, emif_of_match);
+#endif
+
  static struct platform_driver emif_driver = {
  	.remove		= __exit_p(emif_remove),

Not part of this patch I realize, but this is a bug. .remove must be in the __devexit section and the __devexit_p() wrapper must be used. Similarly, emif_probe must be in the __devinit section.

Quoted 3 lines

Again, in our case remove() is not needed unless the driver is built as a module because the devices will never be un-registered. When it is built as a module the effect is same for both:

It's just outside the pattern expected for device drivers. I like to be careful about things like this. That's why I recommend __devinit also, but I see now that the probe hook isn't added to the platform_device so it isn't required.

Quoted 7 lines

#if defined(MODULE) || defined(CONFIG_HOTPLUG)
#define __devexit_p(x) x
#else
#define __devexit_p(x) NULL
#endif

Quoted 5 lines

#ifdef MODULE
#define __exit_p(x) x
#else
#define __exit_p(x) NULL
#endif

Don't go with what is written in the code here; go with what the meaning of the sections are. The code can be changed, but the drivers are expected to honor what the sections mean.

__init can be freed after module init
__devinit in reality never gets freed because devices can show up later __devexit can be freed if hotplug is not enabled
__exit can be freed after module unload

g.