arm: omap: move more HWMOD data to DT

In order to get rid of some more hwmod data, we
introduce a few extra properties to OMAP DT
data in order to be able to pass the needed
information through DT.

Signed-off-by Felipe Balbi <[email protected]>

---
 Documentation/devicetree/bindings/arm/omap/omap.txt | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/arm/omap/omap.txt +0 -0

index 4f6a82c..c6b9515 100644

Documentation/devicetree/bindings/arm/omap/omap.txt +0 -0

 Example:
 
 

Hi Felipe,

Quoted 15 lines · Felipe Balbi

In order to get rid of some more hwmod data, we
introduce a few extra properties to OMAP DT
data in order to be able to pass the needed
information through DT.

Signed-off-by Felipe Balbi <[email protected]>

---
 Documentation/devicetree/bindings/arm/omap/omap.txt | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/arm/omap/omap.txt +0 -0

index 4f6a82c..c6b9515 100644

Documentation/devicetree/bindings/arm/omap/omap.txt Optional properties: · line 32

Quoted 10 lines

Documentation/devicetree/bindings/arm/omap/omap.txt +7 -0

@@ -23,6 +23,13 @@ Optional properties:
   during suspend.
 - ti,no-reset-on-init: When present, the module should not be reset at init
 - ti,no-idle-on-init: When present, the module should not be idled at init
+- ti,rev_offs: IP block revision register offset (defaults to 0)
+- ti,sysc_offs: OCP_SYSCONFIG register offset (defaults to 0)
+- ti,syss_offs: OCP_SYSSTATUS register offset (defaults to 0)
+- ti,srst_udelay: Delay needed after a softreset in usecs (defaults to 0)
+- ti,idlemodes: Slave and Master supported idling modes
+- ti,clockact: default value of clock activity bits (defaults to 0)
+- ti,sysc_type: OCP_SYSCONFIG type. Valid types are 1, 2 or 3 (defaults to 1)

Once if all the hwmod data comes to dt it will be very dufficult to see all the data together. Can we separate out hwmod class data like clocks data is separated out into different node or something of the sort like below?

	ti, hwmod_class {
		ti,class_name = <>;

Quoted 30 lines · Lokesh Vutla

Quoted 15 lines · Felipe Balbi

In order to get rid of some more hwmod data, we
introduce a few extra properties to OMAP DT
data in order to be able to pass the needed
information through DT.

Signed-off-by Felipe Balbi <[email protected]>

---
 Documentation/devicetree/bindings/arm/omap/omap.txt | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/arm/omap/omap.txt +0 -0

index 4f6a82c..c6b9515 100644

Documentation/devicetree/bindings/arm/omap/omap.txt Optional properties: · line 32

Quoted 10 lines

Documentation/devicetree/bindings/arm/omap/omap.txt +7 -0

@@ -23,6 +23,13 @@ Optional properties:
   during suspend.
 - ti,no-reset-on-init: When present, the module should not be reset at init
 - ti,no-idle-on-init: When present, the module should not be idled at init
+- ti,rev_offs: IP block revision register offset (defaults to 0)
+- ti,sysc_offs: OCP_SYSCONFIG register offset (defaults to 0)
+- ti,syss_offs: OCP_SYSSTATUS register offset (defaults to 0)
+- ti,srst_udelay: Delay needed after a softreset in usecs (defaults to 0)
+- ti,idlemodes: Slave and Master supported idling modes
+- ti,clockact: default value of clock activity bits (defaults to 0)
+- ti,sysc_type: OCP_SYSCONFIG type. Valid types are 1, 2 or 3 (defaults to 1)

Once if all the hwmod data comes to dt it will be very dufficult to see all the data together. Can we separate out hwmod class data like clocks data is separated out into different node or something of the sort like below?

Quoted 2 lines

	ti, hwmod_class {
		ti,class_name = <>;

this can be done, but I don't want to pass a class name. It's anyways just the hwmod name without the digits. For those which are not, I would rather change the class name to match.

Quoted 31 lines · Felipe Balbi

Quoted 30 lines · Lokesh Vutla

Quoted 15 lines · Felipe Balbi

In order to get rid of some more hwmod data, we
introduce a few extra properties to OMAP DT
data in order to be able to pass the needed
information through DT.

Signed-off-by Felipe Balbi <[email protected]>

---
 Documentation/devicetree/bindings/arm/omap/omap.txt | 7 +++++++
 1 file changed, 7 insertions(+)

Documentation/devicetree/bindings/arm/omap/omap.txt +0 -0

index 4f6a82c..c6b9515 100644

Documentation/devicetree/bindings/arm/omap/omap.txt Optional properties: · line 32

Quoted 10 lines

Documentation/devicetree/bindings/arm/omap/omap.txt +7 -0

@@ -23,6 +23,13 @@ Optional properties:
   during suspend.
 - ti,no-reset-on-init: When present, the module should not be reset at init
 - ti,no-idle-on-init: When present, the module should not be idled at init
+- ti,rev_offs: IP block revision register offset (defaults to 0)
+- ti,sysc_offs: OCP_SYSCONFIG register offset (defaults to 0)
+- ti,syss_offs: OCP_SYSSTATUS register offset (defaults to 0)
+- ti,srst_udelay: Delay needed after a softreset in usecs (defaults to 0)
+- ti,idlemodes: Slave and Master supported idling modes
+- ti,clockact: default value of clock activity bits (defaults to 0)
+- ti,sysc_type: OCP_SYSCONFIG type. Valid types are 1, 2 or 3 (defaults to 1)

Once if all the hwmod data comes to dt it will be very dufficult to see all the data together. Can we separate out hwmod class data like clocks data is separated out into different node or something of the sort like below?

Quoted 6 lines

Quoted 2 lines

	ti, hwmod_class {
		ti,class_name = <>;

this can be done, but I don't want to pass a class name. It's anyways just the hwmod name without the digits. For those which are not, I would rather change the class name to match.

I did some initial thinking on this some weeks ago, too (and then was short of time, so I did not continue). My suggestion for the binding would be:

* OCP_SYSCONFIG (power-management of IP-Cores connected to OMAP processors)

Each IP-Core connected to the bus of OMAP processors has three registers, which specify the IP-Core's version, its status and setup of PM features.

Required Properties:
- ti,prcm-type: must be one of the following:
1 for OMAP2+ register style,
2 for OMAP4+ register style,
3 for AM33xx register style
- reg: offset to revision, config and status registers relative to module base address

Optional Properties:
- ti,idlemodes: bit field of flags (SIDLE)

   PRCM_IDLE_FORCE               (1 << 0)
   PRCM_IDLE_NO                  (1 << 1)
   PRCM_IDLE_SMART               (1 << 2)
   PRCM_IDLE_SMART_WKUP          (1 << 3)
- ti,standbymodes:             bit field of flags (MIDLE)
   PRCM_STANDBY_FORCE            (1 << 0)
   PRCM_STANDBY_NO               (1 << 1)
   PRCM_STANDBY_SMART            (1 << 2)
   PRCM_STANDBY_SMART_WKUP       (1 << 3)

- ti,sysc-has-autoidle:        config register has AUTOIDLE bit
- ti,sysc-has-softreset:       config register has SOFTRESET bit
- ti,sysc-has-enawakeup:       config register has ENAWAKEUP bit
- ti,sysc-has-emufree:         config register has EMUFREE bit
- ti,sysc-has-clock-activity:  config register has CLOCKACTIVITY bit
- ti,sysc-has-dma-disable:     config register has DMADISABLE bit
- ti,sysc-has-reset-status:    config register has RESETDONE bit
- ti,syss-has-reset-status:    status register has RESETDONE bit

- ti,reset-delay-us: reset delay in us

Example:

ocp {
	gpio1: gpio at 48310000 {
		compatible = "ti,omap3-gpio";

		... /* IP-Core specific properties */

		ti,sysconfig {

Quoted 10 lines · Sebastian Reichel

Each IP-Core connected to the bus of OMAP processors has three registers, which specify the IP-Core's version, its status and setup of PM features.

Required Properties:
- ti,prcm-type: must be one of the following:
1 for OMAP2+ register style,
2 for OMAP4+ register style,
3 for AM33xx register style

Quoted 2 lines

- reg: offset to revision, config and status registers relative to module base address

Quoted 11 lines

Optional Properties:
- ti,idlemodes: bit field of flags (SIDLE)

   PRCM_IDLE_FORCE               (1 << 0)
   PRCM_IDLE_NO                  (1 << 1)
   PRCM_IDLE_SMART               (1 << 2)
   PRCM_IDLE_SMART_WKUP          (1 << 3)
- ti,standbymodes:             bit field of flags (MIDLE)
   PRCM_STANDBY_FORCE            (1 << 0)
   PRCM_STANDBY_NO               (1 << 1)
   PRCM_STANDBY_SMART            (1 << 2)
   PRCM_STANDBY_SMART_WKUP       (1 << 3)

Quoted 8 lines

- ti,sysc-has-autoidle:        config register has AUTOIDLE bit
- ti,sysc-has-softreset:       config register has SOFTRESET bit
- ti,sysc-has-enawakeup:       config register has ENAWAKEUP bit
- ti,sysc-has-emufree:         config register has EMUFREE bit
- ti,sysc-has-clock-activity:  config register has CLOCKACTIVITY bit
- ti,sysc-has-dma-disable:     config register has DMADISABLE bit
- ti,sysc-has-reset-status:    config register has RESETDONE bit
- ti,syss-has-reset-status:    status register has RESETDONE bit

I thought about having boolean flags like this but after talking to Tony we agreed that it would just increase the amount of string parsing during early initialization. Besides, why have two properties using an integer bitfield and another huge number of boolean properties ?

Quoted 8 lines

- ti,reset-delay-us: reset delay in us

Example:

ocp {
	gpio1: gpio at 48310000 {
		compatible = "ti,omap3-gpio";

Quoted 3 lines

		... /* IP-Core specific properties */

		ti,sysconfig {

this is what I'm still waiting for comments from Tony. I'm not convinced we need this extra indentation level. It really brings nothing of value.

After moving sysconfig data to DT, we need to make sure we can generate the same data we have today in C files out of the DT provided properties.

This patch adds support for all optional properties documented by a previous commit.

Signed-off-by Felipe Balbi <[email protected]>

---
 arch/arm/mach-omap2/omap_device.c | 115 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 115 insertions(+)

arch/arm/mach-omap2/omap_device.c +0 -0

index abd622b..ea729aa 100644
@@ -25,6 +25,7 @@
  */
 #undef DEBUG

arch/arm/mach-omap2/omap_device.c +71 -0

+
+static int omap_hwmod_init_sysc(struct device *dev,
+		struct omap_hwmod *oh, int index)
+{
+	struct device_node *node = dev->of_node;
+	struct omap_hwmod_class *class = oh->class;
+	struct omap_hwmod_class_sysconfig *sysc;
+	int ret;
+	int i;
+	char name[128];
+	const char *tmp = oh->name;
+	u32 prop;
+
+	/* if data isn't provided by DT, skip */
+	if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(dev))
+		return 0;
+
+	class = kzalloc(sizeof(*class), GFP_KERNEL);
+	if (!class)
+		return -ENOMEM;
+
+	i = 0;
+	while (*tmp) {
+		if (isalpha(*tmp))
+			name[i++] = *tmp;
+		tmp++;
+	}
+	name[i] = '\0';
+
+	class->name = kzalloc(sizeof(name), GFP_KERNEL);
+	if (!class->name)
+		return -ENOMEM;
+	strncpy(class->name, name, sizeof(name) - 1);
+
+	sysc = kzalloc(sizeof(*sysc), GFP_KERNEL);
+	if (!sysc)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop);
+	if (!ret)
+		sysc->rev_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop);
+	if (!ret)
+		sysc->sysc_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop);
+	if (!ret)
+		sysc->syss_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop);
+	if (!ret)
+		sysc->sysc_flags = prop & 0xffff;
+
+	ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop);
+	if (!ret)
+		sysc->srst_udelay = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop);
+	if (!ret)
+		sysc->idlemodes = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,clockact", index, &prop);
+	if (!ret)
+		sysc->clockact = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop);
+	if (ret)
+		prop = 1;
+
+	switch (prop) {

arch/arm/mach-omap2/omap_device.c +8 -0

+
+	return 0;
+}
 
 /**
  * omap_device_build_from_dt - build an omap_device with multiple hwmods

@@ -154,6 +264,11 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
 			goto odbfd_exit1;
 		}
 		hwmods[i] = oh;
+
+		ret = omap_hwmod_init_sysc(&pdev->dev, oh, i);
+		if (ret)
+			goto odbfd_exit1;
+
 		if (oh->flags & HWMOD_INIT_NO_IDLE)
 			device_active = true;
 

arch/arm/mach-omap2/omap_device.c static void _add_hwmod_clocks_clkdev(struct omap_device *od, · line 218

Quoted 11 lines

arch/arm/mach-omap2/omap_device.c +11 -0

+	switch (prop) {
+	case 2:
+		sysc->sysc_fields = &omap_hwmod_sysc_type2;
+		break;
+	case 3:
+		sysc->sysc_fields = &omap_hwmod_sysc_type3;
+		break;
+	case 1:
+	default:
+		sysc->sysc_fields = &omap_hwmod_sysc_type1;
+	}

Missed hooking the class to oh?

	class->sysc = sysc;

Before this patch, HWMOD requires the existence of a struct omap_hwmod_class very early.

so happens that we're moving that data to DeviceTree in a follow-up commit so we need to allow the registration of HWMODs with a valid omap_hwmod_class pointer.

When the device is built from DT, we will allocate and populate struct omap_hwmod_class based on DT data.

Signed-off-by Felipe Balbi <[email protected]>

---
 arch/arm/mach-omap2/omap_hwmod.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

arch/arm/mach-omap2/omap_hwmod.c +18 -12

index cbb908d..2355764 100644
@@ -504,6 +504,9 @@ static int _set_dmadisable(struct omap_hwmod *oh)
 	u32 v;
 	u32 dmadisable_mask;
 
+	if (!oh->class)
+		return -EINVAL;
+
 	if (!oh->class->sysc ||
 	    !(oh->class->sysc->sysc_flags & SYSC_HAS_DMADISABLE))
 		return -EINVAL;

@@ -1843,7 +1846,7 @@ static int _ocp_softreset(struct omap_hwmod *oh)
 	int c = 0;
 	int ret = 0;
 
-	if (!oh->class->sysc ||
+	if (!oh->class || !oh->class->sysc ||
 	    !(oh->class->sysc->sysc_flags & SYSC_HAS_SOFTRESET))
 		return -ENOENT;
 

@@ -1937,12 +1940,13 @@ static int _reset(struct omap_hwmod *oh)
 
 	pr_debug("omap_hwmod: %s: resetting\n", oh->name);
 
-	if (oh->class->reset) {
+	if (oh->class && oh->class->reset) {
 		r = oh->class->reset(oh);
 	} else {
 		if (oh->rst_lines_cnt > 0) {
-			for (i = 0; i < oh->rst_lines_cnt; i++)
+			for (i = 0; i < oh->rst_lines_cnt; i++) {
 				_assert_hardreset(oh, oh->rst_lines[i].name);
+			}
 			return 0;
 		} else {
 			r = _ocp_softreset(oh);

@@ -1958,7 +1962,7 @@ static int _reset(struct omap_hwmod *oh)
 	 * softreset.  The _enable() function should be split to avoid
 	 * the rewrite of the OCP_SYSCONFIG register.
 	 */
-	if (oh->class->sysc) {
+	if (oh->class && oh->class->sysc) {
 		_update_sysc_cache(oh);
 		_enable_sysc(oh);
 	}

@@ -2036,7 +2040,7 @@ static int _omap4_get_context_lost(struct omap_hwmod *oh)
  */
 static int _enable_preprogram(struct omap_hwmod *oh)
 {
-	if (!oh->class->enable_preprogram)
+	if (!oh->class || !oh->class->enable_preprogram)
 		return 0;
 
 	return oh->class->enable_preprogram(oh);

@@ -2145,7 +2149,7 @@ static int _enable(struct omap_hwmod *oh)
 		oh->_state = _HWMOD_STATE_ENABLED;
 
 		/* Access the sysconfig only if the target is ready */
-		if (oh->class->sysc) {
+		if (oh->class && oh->class->sysc) {
 			if (!(oh->_int_flags & _HWMOD_SYSCONFIG_LOADED))
 				_update_sysc_cache(oh);
 			_enable_sysc(oh);

@@ -2186,7 +2190,7 @@ static int _idle(struct omap_hwmod *oh)
 	if (_are_all_hardreset_lines_asserted(oh))
 		return 0;
 
-	if (oh->class->sysc)
+	if (oh->class && oh->class->sysc)
 		_idle_sysc(oh);
 	_del_initiator_dep(oh, mpu_oh);
 

@@ -2244,7 +2248,7 @@ static int _shutdown(struct omap_hwmod *oh)
 
 	pr_debug("omap_hwmod: %s: disabling\n", oh->name);
 
-	if (oh->class->pre_shutdown) {
+	if (oh->class && oh->class->pre_shutdown) {
 		prev_state = oh->_state;
 		if (oh->_state == _HWMOD_STATE_IDLE)
 			_enable(oh);

@@ -2256,7 +2260,7 @@ static int _shutdown(struct omap_hwmod *oh)
 		}
 	}
 
-	if (oh->class->sysc) {
+	if (oh->class && oh->class->sysc) {
 		if (oh->_state == _HWMOD_STATE_IDLE)
 			_enable(oh);
 		_shutdown_sysc(oh);

@@ -2451,7 +2455,7 @@ static int __init _init(struct omap_hwmod *oh, void *data)
 				oh->name, np->name);
 	}
 
-	if (oh->class->sysc) {
+	if (oh->class && oh->class->sysc) {
 		r = _init_mpu_rt_base(oh, NULL, index, np);
 		if (r < 0) {
 			WARN(1, "omap_hwmod: %s: doesn't have mpu register target base\n",

@@ -2684,8 +2688,7 @@ static int __init _setup(struct omap_hwmod *oh, void *data)
  */
 static int __init _register(struct omap_hwmod *oh)
 {
-	if (!oh || !oh->name || !oh->class || !oh->class->name ||
-	    (oh->_state != _HWMOD_STATE_UNKNOWN))
+	if (!oh || !oh->name || (oh->_state != _HWMOD_STATE_UNKNOWN))
 		return -EINVAL;
 
 	pr_debug("omap_hwmod: %s: registering\n", oh->name);

@@ -3942,6 +3945,9 @@ int omap_hwmod_for_each_by_class(const char *classname,
 		 __func__, classname);
 
 	list_for_each_entry(temp_oh, &omap_hwmod_list, node) {
+		if (!temp_oh->class)
+			continue;
+
 		if (!strcmp(temp_oh->class->name, classname)) {
 			pr_debug("omap_hwmod: %s: %s: calling callback fn\n",
 				 __func__, temp_oh->name);

Hi Felipe,

Quoted 2 lines · Felipe Balbi

Before this patch, HWMOD requires the existence
of a struct omap_hwmod_class very early.

Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod.

With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. Mostly making it unusable.
IMO this patch just masks the problem.

In order to register hwmod we need to populate class data very early. We can populate at the same place as how reg property is being parsed. Call omap_hwmod_init_sysc() in _init() of the particular hwmod:
The below diff will help:

arch/arm/mach-omap2/omap_hwmod.c +110 -0

index cbb908d..05ecf8a 100644
@@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data,
 	return 0;
 }
 
+static int omap_hwmod_has_sysc_bindings(struct device_node *node)
+{
+	char *properties[] = {
+		"ti,rev_offs",
+		"ti,sysc_offs",
+		"ti,syss_offs",
+		"ti,sysc_flags",
+		"ti,srst_udelay",
+		"ti,idlemodes",
+		"ti,clockact",
+		"ti,sysc_type",
+		NULL
+	};
+	char **tmp = properties;
+
+	while (*tmp) {
+		if (of_property_read_bool(node, *tmp)) {
+			return true;
+		}
+		tmp++;
+	}
+
+	return 0;
+}
+
+static int omap_hwmod_init_sysc(struct device_node *node,
+		struct omap_hwmod *oh, int index)
+{
+	struct omap_hwmod_class *class = oh->class;
+	struct omap_hwmod_class_sysconfig *sysc;
+	int ret;
+	int i;
+	char name[128];
+	const char *tmp = oh->name;
+	u32 prop;
+
+	/* if data isn't provided by DT, skip */
+	if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node))
+		return 0;
+
+	class = kzalloc(sizeof(*class), GFP_KERNEL);
+	if (!class)
+		return -ENOMEM;
+
+	i = 0;
+	while (*tmp) {
+		if (isalpha(*tmp))
+			name[i++] = *tmp;
+		tmp++;
+	}
+	name[i] = '\0';
+
+	class->name = kzalloc(sizeof(name), GFP_KERNEL);
+	if (!class->name)
+		return -ENOMEM;
+	strncpy(class->name, name, sizeof(name) - 1);
+
+	sysc = kzalloc(sizeof(*sysc), GFP_KERNEL);
+	if (!sysc)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop);
+	if (!ret)
+		sysc->rev_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop);
+	if (!ret)
+		sysc->sysc_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop);
+	if (!ret)
+		sysc->syss_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop);
+	if (!ret)
+		sysc->sysc_flags = prop & 0xffff;
+
+	ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop);
+	if (!ret)
+		sysc->srst_udelay = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop);
+	if (!ret)
+		sysc->idlemodes = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,clockact", index, &prop);
+	if (!ret)
+		sysc->clockact = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop);
+	if (ret)
+		prop = 1;
+
+	switch (prop) {
+	case 2:
+		sysc->sysc_fields = &omap_hwmod_sysc_type2;
+		break;
+	case 3:
+		sysc->sysc_fields = &omap_hwmod_sysc_type3;
+		break;
+	case 1:
+	default:
+		sysc->sysc_fields = &omap_hwmod_sysc_type1;
+	}
+	class->sysc = sysc;
+	oh->class = class;
+
+	return 0;
+}
+
 /**
  * _init - initialize internal data for the hwmod @oh
  * @oh: struct omap_hwmod *

Hi,

(adding linux-omap back to the loop)

Quoted 140 lines · Lokesh Vutla

Hi Felipe,

Quoted 2 lines · Felipe Balbi

Before this patch, HWMOD requires the existence
of a struct omap_hwmod_class very early.

Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod.

With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. Mostly making it unusable.
IMO this patch just masks the problem.

In order to register hwmod we need to populate class data very early. We can populate at the same place as how reg property is being parsed. Call omap_hwmod_init_sysc() in _init() of the particular hwmod:
The below diff will help:

arch/arm/mach-omap2/omap_hwmod.c +110 -0

index cbb908d..05ecf8a 100644
@@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data,
 	return 0;
 }
 
+static int omap_hwmod_has_sysc_bindings(struct device_node *node)
+{
+	char *properties[] = {
+		"ti,rev_offs",
+		"ti,sysc_offs",
+		"ti,syss_offs",
+		"ti,sysc_flags",
+		"ti,srst_udelay",
+		"ti,idlemodes",
+		"ti,clockact",
+		"ti,sysc_type",
+		NULL
+	};
+	char **tmp = properties;
+
+	while (*tmp) {
+		if (of_property_read_bool(node, *tmp)) {
+			return true;
+		}
+		tmp++;
+	}
+
+	return 0;
+}
+
+static int omap_hwmod_init_sysc(struct device_node *node,
+		struct omap_hwmod *oh, int index)
+{
+	struct omap_hwmod_class *class = oh->class;
+	struct omap_hwmod_class_sysconfig *sysc;
+	int ret;
+	int i;
+	char name[128];
+	const char *tmp = oh->name;
+	u32 prop;
+
+	/* if data isn't provided by DT, skip */
+	if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node))
+		return 0;
+
+	class = kzalloc(sizeof(*class), GFP_KERNEL);
+	if (!class)
+		return -ENOMEM;
+
+	i = 0;
+	while (*tmp) {
+		if (isalpha(*tmp))
+			name[i++] = *tmp;
+		tmp++;
+	}
+	name[i] = '\0';
+
+	class->name = kzalloc(sizeof(name), GFP_KERNEL);
+	if (!class->name)
+		return -ENOMEM;
+	strncpy(class->name, name, sizeof(name) - 1);
+
+	sysc = kzalloc(sizeof(*sysc), GFP_KERNEL);
+	if (!sysc)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop);
+	if (!ret)
+		sysc->rev_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop);
+	if (!ret)
+		sysc->sysc_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop);
+	if (!ret)
+		sysc->syss_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop);
+	if (!ret)
+		sysc->sysc_flags = prop & 0xffff;
+
+	ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop);
+	if (!ret)
+		sysc->srst_udelay = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop);
+	if (!ret)
+		sysc->idlemodes = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,clockact", index, &prop);
+	if (!ret)
+		sysc->clockact = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop);
+	if (ret)
+		prop = 1;
+
+	switch (prop) {
+	case 2:
+		sysc->sysc_fields = &omap_hwmod_sysc_type2;
+		break;
+	case 3:
+		sysc->sysc_fields = &omap_hwmod_sysc_type3;
+		break;
+	case 1:
+	default:
+		sysc->sysc_fields = &omap_hwmod_sysc_type1;
+	}
+	class->sysc = sysc;
+	oh->class = class;
+
+	return 0;
+}
+
 /**
  * _init - initialize internal data for the hwmod @oh
  * @oh: struct omap_hwmod *

@@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data)
 		else if (np && index)

arch/arm/mach-omap2/omap_hwmod.c static int __init _init(struct omap_hwmod *oh, void *data) · line 2564

Quoted 5 lines

arch/arm/mach-omap2/omap_hwmod.c +3 -0

 			pr_warn("omap_hwmod: %s using broken dt data from %s\n",
 				oh->name, np->name);
+
+		if (np) {
+			r = omap_hwmod_init_sysc(np, oh, 0);

this won't handle any binding which lists more than one hwmod on its ti,hwmods property.

Quoted 145 lines · Felipe Balbi

Hi,

(adding linux-omap back to the loop)

Quoted 140 lines · Lokesh Vutla

Hi Felipe,

Quoted 2 lines · Felipe Balbi

Before this patch, HWMOD requires the existence
of a struct omap_hwmod_class very early.

Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod.

With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. Mostly making it unusable.
IMO this patch just masks the problem.

In order to register hwmod we need to populate class data very early. We can populate at the same place as how reg property is being parsed. Call omap_hwmod_init_sysc() in _init() of the particular hwmod:
The below diff will help:

arch/arm/mach-omap2/omap_hwmod.c +110 -0

index cbb908d..05ecf8a 100644
@@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data,
 	return 0;
 }
 
+static int omap_hwmod_has_sysc_bindings(struct device_node *node)
+{
+	char *properties[] = {
+		"ti,rev_offs",
+		"ti,sysc_offs",
+		"ti,syss_offs",
+		"ti,sysc_flags",
+		"ti,srst_udelay",
+		"ti,idlemodes",
+		"ti,clockact",
+		"ti,sysc_type",
+		NULL
+	};
+	char **tmp = properties;
+
+	while (*tmp) {
+		if (of_property_read_bool(node, *tmp)) {
+			return true;
+		}
+		tmp++;
+	}
+
+	return 0;
+}
+
+static int omap_hwmod_init_sysc(struct device_node *node,
+		struct omap_hwmod *oh, int index)
+{
+	struct omap_hwmod_class *class = oh->class;
+	struct omap_hwmod_class_sysconfig *sysc;
+	int ret;
+	int i;
+	char name[128];
+	const char *tmp = oh->name;
+	u32 prop;
+
+	/* if data isn't provided by DT, skip */
+	if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node))
+		return 0;
+
+	class = kzalloc(sizeof(*class), GFP_KERNEL);
+	if (!class)
+		return -ENOMEM;
+
+	i = 0;
+	while (*tmp) {
+		if (isalpha(*tmp))
+			name[i++] = *tmp;
+		tmp++;
+	}
+	name[i] = '\0';
+
+	class->name = kzalloc(sizeof(name), GFP_KERNEL);
+	if (!class->name)
+		return -ENOMEM;
+	strncpy(class->name, name, sizeof(name) - 1);
+
+	sysc = kzalloc(sizeof(*sysc), GFP_KERNEL);
+	if (!sysc)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop);
+	if (!ret)
+		sysc->rev_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop);
+	if (!ret)
+		sysc->sysc_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop);
+	if (!ret)
+		sysc->syss_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop);
+	if (!ret)
+		sysc->sysc_flags = prop & 0xffff;
+
+	ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop);
+	if (!ret)
+		sysc->srst_udelay = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop);
+	if (!ret)
+		sysc->idlemodes = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,clockact", index, &prop);
+	if (!ret)
+		sysc->clockact = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop);
+	if (ret)
+		prop = 1;
+
+	switch (prop) {
+	case 2:
+		sysc->sysc_fields = &omap_hwmod_sysc_type2;
+		break;
+	case 3:
+		sysc->sysc_fields = &omap_hwmod_sysc_type3;
+		break;
+	case 1:
+	default:
+		sysc->sysc_fields = &omap_hwmod_sysc_type1;
+	}
+	class->sysc = sysc;
+	oh->class = class;
+
+	return 0;
+}
+
 /**
  * _init - initialize internal data for the hwmod @oh
  * @oh: struct omap_hwmod *

@@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data)
 		else if (np && index)

Quoted 8 lines

arch/arm/mach-omap2/omap_hwmod.c static int __init _init(struct omap_hwmod *oh, void *data) · line 2564

Quoted 5 lines

arch/arm/mach-omap2/omap_hwmod.c +3 -0

 			pr_warn("omap_hwmod: %s using broken dt data from %s\n",
 				oh->name, np->name);
+
+		if (np) {
+			r = omap_hwmod_init_sysc(np, oh, 0);

this won't handle any binding which lists more than one hwmod on its ti,hwmods property.

I think Tony planned to remove this kind of multi hwmod references.

IMHO instead of DT referencing the hwmod stuff using ti,hwmods the hwmod database should reference the compatible values. This depends on omap3 being DT only of course.

Hi,

Quoted 155 lines · Sebastian Reichel

Quoted 153 lines · Felipe Balbi

Hi,

(adding linux-omap back to the loop)

Quoted 139 lines · Lokesh Vutla

Hi Felipe,

Quoted 2 lines · Felipe Balbi

Before this patch, HWMOD requires the existence
of a struct omap_hwmod_class very early.

Yes, hwmod code looks for omap_hwmod_class entry before registering any hwmod.

With the patch 4/7 omap_hwmod_class gets populated from dt very late after registration of the hwmod. So all the hwmod which gets class data from dt never gets registered and state is always UNKNOWN. Mostly making it unusable.
IMO this patch just masks the problem.

In order to register hwmod we need to populate class data very early. We can populate at the same place as how reg property is being parsed. Call omap_hwmod_init_sysc() in _init() of the particular hwmod:
The below diff will help:

arch/arm/mach-omap2/omap_hwmod.c +110 -0

index cbb908d..05ecf8a 100644
@@ -2415,6 +2415,116 @@ static int __init _init_mpu_rt_base(struct omap_hwmod *oh, void *data,
 	return 0;
 }
 
+static int omap_hwmod_has_sysc_bindings(struct device_node *node)
+{
+	char *properties[] = {
+		"ti,rev_offs",
+		"ti,sysc_offs",
+		"ti,syss_offs",
+		"ti,sysc_flags",
+		"ti,srst_udelay",
+		"ti,idlemodes",
+		"ti,clockact",
+		"ti,sysc_type",
+		NULL
+	};
+	char **tmp = properties;
+
+	while (*tmp) {
+		if (of_property_read_bool(node, *tmp)) {
+			return true;
+		}
+		tmp++;
+	}
+
+	return 0;
+}
+
+static int omap_hwmod_init_sysc(struct device_node *node,
+		struct omap_hwmod *oh, int index)
+{
+	struct omap_hwmod_class *class = oh->class;
+	struct omap_hwmod_class_sysconfig *sysc;
+	int ret;
+	int i;
+	char name[128];
+	const char *tmp = oh->name;
+	u32 prop;
+
+	/* if data isn't provided by DT, skip */
+	if ((class && class->sysc) || !omap_hwmod_has_sysc_bindings(node))
+		return 0;
+
+	class = kzalloc(sizeof(*class), GFP_KERNEL);
+	if (!class)
+		return -ENOMEM;
+
+	i = 0;
+	while (*tmp) {
+		if (isalpha(*tmp))
+			name[i++] = *tmp;
+		tmp++;
+	}
+	name[i] = '\0';
+
+	class->name = kzalloc(sizeof(name), GFP_KERNEL);
+	if (!class->name)
+		return -ENOMEM;
+	strncpy(class->name, name, sizeof(name) - 1);
+
+	sysc = kzalloc(sizeof(*sysc), GFP_KERNEL);
+	if (!sysc)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_index(node, "ti,rev_offs", index, &prop);
+	if (!ret)
+		sysc->rev_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_offs", index, &prop);
+	if (!ret)
+		sysc->sysc_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,syss_offs", index, &prop);
+	if (!ret)
+		sysc->syss_offs = prop;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_flags", index, &prop);
+	if (!ret)
+		sysc->sysc_flags = prop & 0xffff;
+
+	ret = of_property_read_u32_index(node, "ti,srst_udelay", index, &prop);
+	if (!ret)
+		sysc->srst_udelay = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,idlemodes", index, &prop);
+	if (!ret)
+		sysc->idlemodes = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,clockact", index, &prop);
+	if (!ret)
+		sysc->clockact = prop & 0xff;
+
+	ret = of_property_read_u32_index(node, "ti,sysc_type", index, &prop);
+	if (ret)
+		prop = 1;
+
+	switch (prop) {
+	case 2:
+		sysc->sysc_fields = &omap_hwmod_sysc_type2;
+		break;
+	case 3:
+		sysc->sysc_fields = &omap_hwmod_sysc_type3;
+		break;
+	case 1:
+	default:
+		sysc->sysc_fields = &omap_hwmod_sysc_type1;
+	}
+	class->sysc = sysc;
+	oh->class = class;
+
+	return 0;
+}
+
 /**
  * _init - initialize internal data for the hwmod @oh
  * @oh: struct omap_hwmod *

arch/arm/mach-omap2/omap_hwmod.c static int __init _init(struct omap_hwmod *oh, void *data) · line 2564

Quoted 6 lines

arch/arm/mach-omap2/omap_hwmod.c +3 -0

@@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data)
 		else if (np && index)
 			pr_warn("omap_hwmod: %s using broken dt data from %s\n",
 				oh->name, np->name);
+
+		if (np) {
+			r = omap_hwmod_init_sysc(np, oh, 0);

this won't handle any binding which lists more than one hwmod on its ti,hwmods property.

Quoted 1 line

I think Tony planned to remove this kind of multi hwmod references.

true, but I'm still a bit iffy wrt that. ABI compatibility breaks and all

Quoted 3 lines

IMHO instead of DT referencing the hwmod stuff using ti,hwmods the hwmod database should reference the compatible values. This depends on omap3 being DT only of course.

don't you think it's too late for that ? We need to support the current form of dts files forever. It's an ABI afterall.

Quoted 2 lines · Felipe Balbi

true, but I'm still a bit iffy wrt that. ABI compatibility breaks and all

Moving the hwmod data to device tree will break the ABI compatibility anyways. You wont be able to use an old DT with the new kernel, since then you are missing the hwmod data (assuming there won't be a fallback hwmod data kept in the kernel source).

Quoted 6 lines

Quoted 3 lines

IMHO instead of DT referencing the hwmod stuff using ti,hwmods the hwmod database should reference the compatible values. This depends on omap3 being DT only of course.

don't you think it's too late for that ? We need to support the current form of dts files forever. It's an ABI afterall.

As I described above the ABI *will* break if hwmod data is removed from the kernel.

OTOH where is the ABI breakage when hwmod database in kernel is built from the compatible value? The compatible values are already part of the ABI, so there are no new properties needed. The ti,hwmod property can be marked as deprecated (and maybe removed after some years).

Quoted 10 lines · Felipe Balbi

arch/arm/mach-omap2/omap_hwmod.c static int __init _init(struct omap_hwmod *oh, void *data) · line 2564

Quoted 7 lines

arch/arm/mach-omap2/omap_hwmod.c +3 -0

@@ -2449,6 +2559,12 @@ static int __init _init(struct omap_hwmod *oh, void *data)
 		else if (np && index)
 			pr_warn("omap_hwmod: %s using broken dt data from %s\n",
 				oh->name, np->name);
+
+		if (np) {
+			r = omap_hwmod_init_sysc(np, oh, 0);

this won't handle any binding which lists more than one hwmod on its ti,hwmods property.

I think Tony planned to remove this kind of multi hwmod references.

IMHO instead of DT referencing the hwmod stuff using ti,hwmods the hwmod database should reference the compatible values. This depends on omap3 being DT only of course.

Hi,

here's a more complete series showing the out of debugfs interface.

So far only AM43xx has been converted and only those HWMODs which play well with DT and/or are not common with other OMAPs/AM33xx.

I plan to work on AM33xx tomorrow, but this RFC shows what we can achieve WRT HWMOD static data reduction.

As you can see, with AM43xx, data reduction is quite minimal, since it reuses so much from AM33xx, but well, had to start somewhere.

All patches were boot tested with AM437x SK on top of:

commit 1ca7c606de868d172afb4eb65e04e290dbdb51ff

Author: Stephen Rothwell <[email protected]>
Date:   Thu Dec 4 19:49:10 2014 +1100

Add linux-next specific files for 20141204

Signed-off-by Stephen Rothwell <[email protected]>

cheers

Felipe Balbi (7):
arm: omap: hwmod: add debugfs interface
arm: omap: devicetree: add new properties for OMAP devices
arm: omap: hwmod: drop 'const' qualifier from omap_hwmod_class name
arm: omap: device: add support for generating sysconfig data from DT
arm: omap: hwmod: allow for registration of class-less hwmods
arm: boot: dts: am4372: add sysconfig data to all HWMODs
arm: omap: hwmod: 43xx: remove sysc and class data

 .../devicetree/bindings/arm/omap/omap.txt          |   7 +
 arch/arm/boot/dts/am4372.dtsi                      | 251 +++++++++++++++++++
 arch/arm/mach-omap2/Makefile                       |   2 +-
 arch/arm/mach-omap2/omap_device.c                  | 117 +++++++++
 arch/arm/mach-omap2/omap_hwmod.c                   |  30 ++-
 arch/arm/mach-omap2/omap_hwmod.h                   |  11 +-
 arch/arm/mach-omap2/omap_hwmod_43xx_data.c         |  61 -----
 arch/arm/mach-omap2/omap_hwmod_debugfs.c           | 269 +++++++++++++++++++++
 8 files changed, 673 insertions(+), 75 deletions(-)
 create mode 100644 arch/arm/mach-omap2/omap_hwmod_debugfs.c