Skip to contentSkip to conversation

SmartReflex Patches

PATCH In review Felipe Balbi 4 participants 16 messages Nov 15, 2011 – Nov 23, 2011

Subsystems (3)
Arm (32-bit) Arm sub-architectures OMAP2+ support
lore
Share & tools
lore

Felipe Balbi

author
#0 lore raw

cover SmartReflex Patches

Hi Tony,

here's a bunch of patches to smartreflex which have been pending for months.

Compile tested with omap2plus_defconfig + CONFIG_OMAP_SMARTREFLEX + CONFIG_OMAP_SMARTREFLEX_CLASS3.

Felipe Balbi (6):
arm: omap: smartreflex: add missing platform_set_drvdata()
arm: omap: smartreflex: use dev_get_drvdata()
arm: omap: smartreflex: move late_initcall() closer to its argument
arm: omap: smartreflex: clean ups all over
arm: omap: smartreflex: fix IRQ handling bug
arm: omap: smartreflex: micro-optimization for sanity check

Nishanth Menon (1):
OMAP3+: PM: SR: add suspend/resume handlers

 arch/arm/mach-omap2/smartreflex.c |  309 ++++++++++++++++++++++++------------
 1 files changed, 206 insertions(+), 103 deletions(-)
Signature · 1 line
1.7.8.rc0
author
#1 lore raw

PATCH 1/7 OMAP3+: PM: SR: add suspend/resume handlers

From: Nishanth Menon <[email protected]>

SmartReflex should be disabled while entering low power mode due to the following reasons:
a) SmartReflex values are not defined for retention voltage.
b) With SmartReflex enabled, if the CPU enters low power state, FSM will try to bump the voltage to current OPP's voltage for which it has entered low power state, causing power loss and potential unknown states for the SoC.
Since we are sure to attempt entering the lowest possible power state during suspend operation, SmartReflex needs to be disabled for the voltage domains in suspend path before achieving auto retention voltage on the device.

Traditionally, this has been done with interrupts disabled as part of the common code which handles the idle sequence. Instead, by using the fact that we have to disable SmartReflex for sure during suspend operations, we can opportunistically disable SmartReflex in device standard pm ops, instead of disabling it as part of the code which executes with interrupts disabled and slave CPU{s} shutdown. This allows the system to do other parallel activities(such as suspending other devices in the system using slave CPU{s}) and save the time required to achieve suspend and resume from suspended state as a sequential activity.

However, by being opportunistic as described above, we also increase the likelihood of SmartReflex library access functions being invoked in parallel contexts *after* SmartReflex driver's suspend handler (during suspend operation) or *before* resume handler (during resume operation) have been invoked (Example: DVFS for dependent devices, cpufreq for MPU etc.). We prevent this by using a flag to reject the callers in the duration where SmartReflex has been disabled.

Signed-off-by Nishanth Menon <[email protected]>
---
 arch/arm/mach-omap2/smartreflex.c |   96 ++++++++++++++++++++++++++++++++++--
 1 files changed, 90 insertions(+), 6 deletions(-)
arch/arm/mach-omap2/smartreflex.c +90 -6
index 6a4f683..5763a71 100644
@@ -24,6 +24,7 @@
 #include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/pm.h>
 #include <linux/pm_runtime.h>
 
 #include <plat/common.h>

@@ -40,6 +41,7 @@ struct omap_sr {
 	int				ip_type;
 	int				nvalue_count;
 	bool				autocomp_active;
+	bool				is_suspended;
 	u32				clk_length;
 	u32				err_weight;
 	u32				err_minlimit;

@@ -685,6 +687,11 @@ void omap_sr_enable(struct voltagedomain *voltdm)
 	if (!sr->autocomp_active)
 		return;
 
+	if (sr->is_suspended) {
+		dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__);
+		return;
+	}
+
 	if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) {
 		dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
 			"registered\n", __func__);

@@ -718,6 +725,11 @@ void omap_sr_disable(struct voltagedomain *voltdm)
 	if (!sr->autocomp_active)
 		return;
 
+	if (sr->is_suspended) {
+		dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__);
+		return;
+	}
+
 	if (!sr_class || !(sr_class->disable)) {
 		dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
 			"registered\n", __func__);

@@ -751,6 +763,11 @@ void omap_sr_disable_reset_volt(struct voltagedomain *voltdm)
 	if (!sr->autocomp_active)
 		return;
 
+	if (sr->is_suspended) {
+		dev_dbg(&sr->pdev->dev, "%s: in suspended state\n", __func__);
+		return;
+	}
+
 	if (!sr_class || !(sr_class->disable)) {
 		dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
 			"registered\n", __func__);

@@ -809,14 +826,16 @@ static int omap_sr_autocomp_store(void *data, u64 val)
 		return -EINVAL;
 	}
 
-	/* control enable/disable only if there is a delta in value */
-	if (sr_info->autocomp_active != val) {
-		if (!val)
-			sr_stop_vddautocomp(sr_info);
-		else
-			sr_start_vddautocomp(sr_info);
+	if (sr_info->is_suspended) {
+		pr_warning("%s: in suspended state\n", __func__);
+		return -EBUSY;
 	}
 
+	if (!val)
+		sr_stop_vddautocomp(sr_info);
+	else
+		sr_start_vddautocomp(sr_info);
+
 	return 0;
 }
 

@@ -1011,10 +1030,75 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int omap_sr_suspend(struct device *dev)
+{
+	struct omap_sr_data *pdata;
+	struct omap_sr *sr_info;
+
+	pdata = dev_get_platdata(dev);
+	if (!pdata) {
+		dev_err(dev, "%s: platform data missing\n", __func__);
+		return -EINVAL;
+	}
+
+	sr_info = _sr_lookup(pdata->voltdm);
+	if (IS_ERR(sr_info)) {
+		dev_warn(dev, "%s: omap_sr struct not found\n", __func__);
+		return -EINVAL;
+	}
+
+	if (!sr_info->autocomp_active)
+		return 0;
+
+	if (sr_info->is_suspended)
+		return 0;
+
+	omap_sr_disable_reset_volt(pdata->voltdm);
+	sr_info->is_suspended = true;
+	/* Flag the same info to the other CPUs */
+	smp_wmb();
+
+	return 0;
+}
+
+static int omap_sr_resume(struct device *dev)
+{
+	struct omap_sr_data *pdata;
+	struct omap_sr *sr_info;
+
+	pdata = dev_get_platdata(dev);
+	if (!pdata) {
+		dev_err(dev, "%s: platform data missing\n", __func__);
+		return -EINVAL;
+	}
+
+	sr_info = _sr_lookup(pdata->voltdm);
+	if (IS_ERR(sr_info)) {
+		dev_warn(dev, "%s: omap_sr struct not found\n", __func__);
+		return -EINVAL;
+	}
+
+	if (!sr_info->autocomp_active)
+		return 0;
+
+	if (!sr_info->is_suspended)
+		return 0;
+
+	sr_info->is_suspended = false;
+	/* Flag the same info to the other CPUs */
+	smp_wmb();
+	omap_sr_enable(pdata->voltdm);
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(omap_sr_dev_pm_ops, omap_sr_suspend, omap_sr_resume);
+
 static struct platform_driver smartreflex_driver = {
 	.remove         = omap_sr_remove,
 	.driver		= {
 		.name	= "smartreflex",
+		.pm	= &omap_sr_dev_pm_ops,
 	},
 };
Signature · 1 line
1.7.8.rc0
#2 lore raw

PATCH 1/7 OMAP3+: PM: SR: add suspend/resume handlers

Quoted 4 lines · Felipe Balbi

From: Nishanth Menon <[email protected]>

SmartReflex should be disabled while entering low power mode due to the following reasons:

[...]

Nishanth, in the end, didn't you decide to drop this patch?

Kevin

#3 lore raw

Re: [PATCH/RESEND #3 1/7] OMAP3+: PM: SR: add suspend/resume handlers

Quoted 10 lines · Kevin Hilman
Quoted 4 lines · Felipe Balbi

From: Nishanth Menon <[email protected]>

SmartReflex should be disabled while entering low power mode due to the following reasons:

[...]

Nishanth, in the end, didn't you decide to drop this patch?

Yes, I did eventually, once we implemented DVFS for GPU, Ducati, HSI, and other drivers, there was no real way to ensure sequence of suspend sequencing even after moving this to suspend_noirq. some of the other reasons:

if I disabled Smartreflex and went to Nominal voltage on MPU, and say MPU was at "performance" mode of 1.5GHz or so, thermal scenarios got worse due on hot corner samples - these tend to have higher leakage and thermal characteristics tend to be more pronounced. The option of throttling frequency down while suspend was not really a good option in comparison to switching off smart reflex in the last possible moment - in pmxxx.c

Regards,
Nishanth Menon

#4 lore raw

Re: [PATCH/RESEND #3 1/7] OMAP3+: PM: SR: add suspend/resume handlers

Quoted 9 lines · Nishanth
Quoted 8 lines · Kevin Hilman
Quoted 4 lines · Felipe Balbi

From: Nishanth Menon <[email protected]>

SmartReflex should be disabled while entering low power mode due to the following reasons:

[...]

Quoted 7 lines
Quoted 2 lines

Nishanth, in the end, didn't you decide to drop this patch?

Yes, I did eventually, once we implemented DVFS for GPU, Ducati, HSI, and other drivers, there was no real way to ensure sequence of suspend sequencing even after moving this to suspend_noirq. some of the other reasons:

OK, thanks for the clarification.

Kevin

author
#5 lore raw

PATCH 2/7 arm: omap: smartreflex: add missing platform_set_drvdata()

In reply to Felipe Balbi

that's very useful to fetch the correct struct sr_info from PM handlers.

Signed-off-by Felipe Balbi <[email protected]>
---
 arch/arm/mach-omap2/smartreflex.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
arch/arm/mach-omap2/smartreflex.c +2 -0
index 5763a71..443591b 100644
@@ -858,6 +858,8 @@ static int __init omap_sr_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	platform_set_drvdata(pdev, sr_info);
+
 	if (!pdata) {
 		dev_err(&pdev->dev, "%s: platform data missing\n", __func__);
 		ret = -EINVAL;
Signature · 1 line
1.7.8.rc0
author
#6 lore raw

PATCH 3/7 arm: omap: smartreflex: use dev_get_drvdata()

In reply to Felipe Balbi

When we need to fetch struct omap_sr on PM handlers, there's no need to access platform_data.

Instead, we can use dev_get_drvdata(dev) like
other drivers do.

Signed-off-by Felipe Balbi <[email protected]>
---
 arch/arm/mach-omap2/smartreflex.c |   51 ++++++------------------------------
 1 files changed, 9 insertions(+), 42 deletions(-)
arch/arm/mach-omap2/smartreflex.c +9 -42
index 443591b..bb3bfc0 100644
@@ -1002,22 +1002,9 @@ err_free_devinfo:
 
 static int __devexit omap_sr_remove(struct platform_device *pdev)
 {
-	struct omap_sr_data *pdata = pdev->dev.platform_data;
-	struct omap_sr *sr_info;
+	struct omap_sr *sr_info = platform_get_drvdata(pdev);
 	struct resource *mem;
 
-	if (!pdata) {
-		dev_err(&pdev->dev, "%s: platform data missing\n", __func__);
-		return -EINVAL;
-	}
-
-	sr_info = _sr_lookup(pdata->voltdm);
-	if (IS_ERR(sr_info)) {
-		dev_warn(&pdev->dev, "%s: omap_sr struct not found\n",
-			__func__);
-		return -EINVAL;
-	}
-
 	if (sr_info->autocomp_active)
 		sr_stop_vddautocomp(sr_info);
 	if (sr_info->dbg_dir)

@@ -1034,20 +1021,10 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
 
 static int omap_sr_suspend(struct device *dev)
 {
-	struct omap_sr_data *pdata;
-	struct omap_sr *sr_info;
+	struct omap_sr *sr_info = dev_get_drvdata(dev);
 
-	pdata = dev_get_platdata(dev);
-	if (!pdata) {
-		dev_err(dev, "%s: platform data missing\n", __func__);
-		return -EINVAL;
-	}
-
-	sr_info = _sr_lookup(pdata->voltdm);
-	if (IS_ERR(sr_info)) {
-		dev_warn(dev, "%s: omap_sr struct not found\n", __func__);
-		return -EINVAL;
-	}
+	if (!sr_info)
+		return 0;
 
 	if (!sr_info->autocomp_active)
 		return 0;

@@ -1055,7 +1032,7 @@ static int omap_sr_suspend(struct device *dev)
 	if (sr_info->is_suspended)
 		return 0;
 
-	omap_sr_disable_reset_volt(pdata->voltdm);
+	omap_sr_disable_reset_volt(sr_info->voltdm);
 	sr_info->is_suspended = true;
 	/* Flag the same info to the other CPUs */
 	smp_wmb();

@@ -1065,20 +1042,10 @@ static int omap_sr_suspend(struct device *dev)
 
 static int omap_sr_resume(struct device *dev)
 {
-	struct omap_sr_data *pdata;
-	struct omap_sr *sr_info;
+	struct omap_sr *sr_info = dev_get_drvdata(dev);
 
-	pdata = dev_get_platdata(dev);
-	if (!pdata) {
-		dev_err(dev, "%s: platform data missing\n", __func__);
-		return -EINVAL;
-	}
-
-	sr_info = _sr_lookup(pdata->voltdm);
-	if (IS_ERR(sr_info)) {
-		dev_warn(dev, "%s: omap_sr struct not found\n", __func__);
-		return -EINVAL;
-	}
+	if (!sr_info)
+		return 0;
 
 	if (!sr_info->autocomp_active)
 		return 0;

@@ -1089,7 +1056,7 @@ static int omap_sr_resume(struct device *dev)
 	sr_info->is_suspended = false;
 	/* Flag the same info to the other CPUs */
 	smp_wmb();
-	omap_sr_enable(pdata->voltdm);
+	omap_sr_enable(sr_info->voltdm);
 
 	return 0;
 }
Signature · 1 line
1.7.8.rc0
author
#7 lore raw

PATCH 4/7 arm: omap: smartreflex: move late_initcall() closer to its argument

In reply to Felipe Balbi

no functional changes, trivial patch.

Signed-off-by Felipe Balbi <[email protected]>
---
 arch/arm/mach-omap2/smartreflex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/smartreflex.c +1 -1
index bb3bfc0..97375d2 100644
@@ -1095,12 +1095,12 @@ static int __init sr_init(void)
 
 	return 0;
 }
+late_initcall(sr_init);
 
 static void __exit sr_exit(void)
 {
 	platform_driver_unregister(&smartreflex_driver);
 }
-late_initcall(sr_init);
 module_exit(sr_exit);
 
 MODULE_DESCRIPTION("OMAP Smartreflex Driver");
Signature · 1 line
1.7.8.rc0
author
#8 lore raw

PATCH 5/7 arm: omap: smartreflex: clean ups all over

In reply to Felipe Balbi

There are no functional changes here, only misc cleanups in general: tabifying variable declarations, converting if {} else if {} else {} into switch statements, etc.

Signed-off-by Felipe Balbi <[email protected]>
---
 arch/arm/mach-omap2/smartreflex.c |  218 +++++++++++++++++++++++--------------
 1 files changed, 134 insertions(+), 84 deletions(-)
arch/arm/mach-omap2/smartreflex.c +134 -84
index 97375d2..eb88b36 100644
@@ -37,26 +37,32 @@
 #define SR_DISABLE_TIMEOUT	200
 
 struct omap_sr {
-	int				srid;
-	int				ip_type;
+	struct list_head		node;
+	struct platform_device		*pdev;
+	struct omap_sr_nvalue_table	*nvalue_table;
+	struct voltagedomain		*voltdm;
+
+	unsigned int			irq;
+
 	int				nvalue_count;
-	bool				autocomp_active;
-	bool				is_suspended;
-	u32				clk_length;
-	u32				err_weight;
+	int				ip_type;
+	int				srid;
+
+	u32				senn_avgweight;
+	u32				senp_avgweight;
 	u32				err_minlimit;
 	u32				err_maxlimit;
+	u32				clk_length;
+	u32				err_weight;
 	u32				accum_data;
-	u32				senn_avgweight;
-	u32				senp_avgweight;
 	u32				senp_mod;
 	u32				senn_mod;
-	unsigned int			irq;
+
+	bool				autocomp_active;
+	bool				is_suspended;
+
 	void __iomem			*base;
-	struct platform_device		*pdev;
-	struct list_head		node;
-	struct omap_sr_nvalue_table	*nvalue_table;
-	struct voltagedomain		*voltdm;
+
 	struct dentry			*dbg_dir;
 };
 

@@ -75,11 +81,9 @@ static inline void sr_write_reg(struct omap_sr *sr, unsigned offset, u32 value)
 static inline void sr_modify_reg(struct omap_sr *sr, unsigned offset, u32 mask,
 					u32 value)
 {
-	u32 reg_val;
-	u32 errconfig_offs = 0, errconfig_mask = 0;
-
-	reg_val = __raw_readl(sr->base + offset);
-	reg_val &= ~mask;
+	u32		reg_val;
+	u32		errconfig_offs = 0;
+	u32		errconfig_mask = 0;
 
 	/*
 	 * Smartreflex error config register is special as it contains

@@ -90,14 +94,23 @@ static inline void sr_modify_reg(struct omap_sr *sr, unsigned offset, u32 mask,
 	 * if they are currently set, but does allow the caller to write
 	 * those bits.
 	 */
-	if (sr->ip_type == SR_TYPE_V1) {
+	switch (sr->ip_type) {
+	case SR_TYPE_V1:
 		errconfig_offs = ERRCONFIG_V1;
 		errconfig_mask = ERRCONFIG_STATUS_V1_MASK;
-	} else if (sr->ip_type == SR_TYPE_V2) {
+		break;
+	case SR_TYPE_V2:
 		errconfig_offs = ERRCONFIG_V2;
 		errconfig_mask = ERRCONFIG_VPBOUNDINTST_V2;
+		break;
+	default: /* should never happen */
+		dev_err(&sr->pdev->dev, "UNKNOWN IP type %d\n", sr->ip_type);
+		return;
 	}
 
+	reg_val = __raw_readl(sr->base + offset);
+	reg_val &= ~mask;
+
 	if (offset == errconfig_offs)
 		reg_val &= ~errconfig_mask;
 

@@ -113,7 +126,7 @@ static inline u32 sr_read_reg(struct omap_sr *sr, unsigned offset)
 
 static struct omap_sr *_sr_lookup(struct voltagedomain *voltdm)
 {
-	struct omap_sr *sr_info;
+	struct omap_sr	*sr_info;
 
 	if (!voltdm) {
 		pr_err("%s: Null voltage domain passed!\n", __func__);

@@ -130,33 +143,39 @@ static struct omap_sr *_sr_lookup(struct voltagedomain *voltdm)
 
 static irqreturn_t sr_interrupt(int irq, void *data)
 {
-	struct omap_sr *sr_info = (struct omap_sr *)data;
-	u32 status = 0;
+	struct omap_sr	*sr_info = data;
+	u32		status = 0;
 
-	if (sr_info->ip_type == SR_TYPE_V1) {
+	switch (sr_info->ip_type) {
+	case SR_TYPE_V1:
 		/* Read the status bits */
 		status = sr_read_reg(sr_info, ERRCONFIG_V1);
 
 		/* Clear them by writing back */
 		sr_write_reg(sr_info, ERRCONFIG_V1, status);
-	} else if (sr_info->ip_type == SR_TYPE_V2) {
+		break;
+	case SR_TYPE_V2:
 		/* Read the status bits */
 		sr_read_reg(sr_info, IRQSTATUS);
 
 		/* Clear them by writing back */
 		sr_write_reg(sr_info, IRQSTATUS, status);
-	}
 
-	if (sr_class->notify)
-		sr_class->notify(sr_info->voltdm, status);
+		if (sr_class->notify)
+			sr_class->notify(sr_info->voltdm, status);
+		break;
+	default: /* should never happen */
+		dev_err(&sr_info->pdev->dev, "UNKNOWN IP type %d\n", sr_info->ip_type);
+		return IRQ_NONE;
+	}
 
 	return IRQ_HANDLED;
 }
 
 static void sr_set_clk_length(struct omap_sr *sr)
 {
-	struct clk *sys_ck;
-	u32 sys_clk_speed;
+	struct clk	*sys_ck;
+	u32		sys_clk_speed;
 
 	if (cpu_is_omap34xx())
 		sys_ck = clk_get(NULL, "sys_ck");

@@ -168,6 +187,7 @@ static void sr_set_clk_length(struct omap_sr *sr)
 			__func__);
 		return;
 	}
+
 	sys_clk_speed = clk_get_rate(sys_ck);
 	clk_put(sys_ck);
 

@@ -257,10 +277,10 @@ static void sr_stop_vddautocomp(struct omap_sr *sr)
  */
 static int sr_late_init(struct omap_sr *sr_info)
 {
-	char *name;
-	struct omap_sr_data *pdata = sr_info->pdev->dev.platform_data;
-	struct resource *mem;
-	int ret = 0;
+	struct omap_sr_data	*pdata = sr_info->pdev->dev.platform_data;
+	struct			resource *mem;
+	int			ret = 0;
+	char			*name;
 
 	if (sr_class->notify && sr_class->notify_flags && sr_info->irq) {
 		name = kasprintf(GFP_KERNEL, "sr_%s", sr_info->voltdm->name);

@@ -269,7 +289,7 @@ static int sr_late_init(struct omap_sr *sr_info)
 			goto error;
 		}
 		ret = request_irq(sr_info->irq, sr_interrupt,
-				0, name, (void *)sr_info);
+				0, name, sr_info);
 		if (ret)
 			goto error;
 		disable_irq(sr_info->irq);

@@ -290,12 +310,13 @@ error:
 		"not function as desired\n", __func__);
 	kfree(name);
 	kfree(sr_info);
+
 	return ret;
 }
 
 static void sr_v1_disable(struct omap_sr *sr)
 {
-	int timeout = 0;
+	int		timeout = 0;
 
 	/* Enable MCUDisableAcknowledge interrupt */
 	sr_modify_reg(sr, ERRCONFIG_V1,

@@ -331,7 +352,7 @@ static void sr_v1_disable(struct omap_sr *sr)
 
 static void sr_v2_disable(struct omap_sr *sr)
 {
-	int timeout = 0;
+	int		timeout = 0;
 
 	/* Enable MCUDisableAcknowledge interrupt */
 	sr_write_reg(sr, IRQENABLE_SET, IRQENABLE_MCUDISABLEACKINT);

@@ -368,7 +389,7 @@ static void sr_v2_disable(struct omap_sr *sr)
 
 static u32 sr_retrieve_nvalue(struct omap_sr *sr, u32 efuse_offs)
 {
-	int i;
+	int		i;
 
 	if (!sr->nvalue_table) {
 		dev_warn(&sr->pdev->dev, "%s: Missing ntarget value table\n",

@@ -400,10 +421,16 @@ static u32 sr_retrieve_nvalue(struct omap_sr *sr, u32 efuse_offs)
  */
 int sr_configure_errgen(struct voltagedomain *voltdm)
 {
-	u32 sr_config, sr_errconfig, errconfig_offs, vpboundint_en;
-	u32 vpboundint_st, senp_en = 0, senn_en = 0;
-	u8 senp_shift, senn_shift;
-	struct omap_sr *sr = _sr_lookup(voltdm);
+	struct omap_sr		*sr = _sr_lookup(voltdm);
+	u32			sr_config;
+	u32			sr_errconfig;
+	u32			errconfig_offs;
+	u32			vpboundint_en;
+	u32			vpboundint_st;
+	u32			senp_en = 0;
+	u32			senn_en = 0;
+	u8			senp_shift;
+	u8			senn_shift;
 
 	if (IS_ERR(sr)) {
 		pr_warning("%s: omap_sr struct for sr_%s not found\n",

@@ -420,22 +447,24 @@ int sr_configure_errgen(struct voltagedomain *voltdm)
 	sr_config = (sr->clk_length << SRCONFIG_SRCLKLENGTH_SHIFT) |
 		SRCONFIG_SENENABLE | SRCONFIG_ERRGEN_EN;
 
-	if (sr->ip_type == SR_TYPE_V1) {
+	switch (sr->ip_type) {
+	case SR_TYPE_V1:
 		sr_config |= SRCONFIG_DELAYCTRL;
 		senn_shift = SRCONFIG_SENNENABLE_V1_SHIFT;
 		senp_shift = SRCONFIG_SENPENABLE_V1_SHIFT;
 		errconfig_offs = ERRCONFIG_V1;
 		vpboundint_en = ERRCONFIG_VPBOUNDINTEN_V1;
 		vpboundint_st = ERRCONFIG_VPBOUNDINTST_V1;
-	} else if (sr->ip_type == SR_TYPE_V2) {
+		break;
+	case SR_TYPE_V2:
 		senn_shift = SRCONFIG_SENNENABLE_V2_SHIFT;
 		senp_shift = SRCONFIG_SENPENABLE_V2_SHIFT;
 		errconfig_offs = ERRCONFIG_V2;
 		vpboundint_en = ERRCONFIG_VPBOUNDINTEN_V2;
 		vpboundint_st = ERRCONFIG_VPBOUNDINTST_V2;
-	} else {
-		dev_err(&sr->pdev->dev, "%s: Trying to Configure smartreflex"
-			"module without specifying the ip\n", __func__);
+		break;
+	default:
+		dev_err(&sr->pdev->dev, "UNKNOWN IP type %d\n", sr->ip_type);
 		return -EINVAL;
 	}
 

@@ -469,10 +498,13 @@ int sr_configure_errgen(struct voltagedomain *voltdm)
  */
 int sr_configure_minmax(struct voltagedomain *voltdm)
 {
-	u32 sr_config, sr_avgwt;
-	u32 senp_en = 0, senn_en = 0;
-	u8 senp_shift, senn_shift;
-	struct omap_sr *sr = _sr_lookup(voltdm);
+	struct omap_sr		*sr = _sr_lookup(voltdm);
+	u32			sr_config;
+	u32			sr_avgwt;
+	u32			senp_en = 0;
+	u32			senn_en = 0;
+	u8			senp_shift;
+	u8			senn_shift;
 
 	if (IS_ERR(sr)) {
 		pr_warning("%s: omap_sr struct for sr_%s not found\n",

@@ -490,16 +522,18 @@ int sr_configure_minmax(struct voltagedomain *voltdm)
 		SRCONFIG_SENENABLE |
 		(sr->accum_data << SRCONFIG_ACCUMDATA_SHIFT);
 
-	if (sr->ip_type == SR_TYPE_V1) {
+	switch (sr->ip_type) {
+	case SR_TYPE_V1:
 		sr_config |= SRCONFIG_DELAYCTRL;
 		senn_shift = SRCONFIG_SENNENABLE_V1_SHIFT;
 		senp_shift = SRCONFIG_SENPENABLE_V1_SHIFT;
-	} else if (sr->ip_type == SR_TYPE_V2) {
+		break;
+	case SR_TYPE_V2:
 		senn_shift = SRCONFIG_SENNENABLE_V2_SHIFT;
 		senp_shift = SRCONFIG_SENPENABLE_V2_SHIFT;
-	} else {
-		dev_err(&sr->pdev->dev, "%s: Trying to Configure smartreflex"
-			"module without specifying the ip\n", __func__);
+		break;
+	default:
+		dev_err(&sr->pdev->dev, "UNKNOWN IP type %d\n", sr->ip_type);
 		return -EINVAL;
 	}
 

@@ -513,20 +547,26 @@ int sr_configure_minmax(struct voltagedomain *voltdm)
 	 * Enabling the interrupts if MINMAXAVG module is used.
 	 * TODO: check if all the interrupts are mandatory
 	 */
-	if (sr->ip_type == SR_TYPE_V1) {
+	switch (sr->ip_type) {
+	case SR_TYPE_V1:
 		sr_modify_reg(sr, ERRCONFIG_V1,
 			(ERRCONFIG_MCUACCUMINTEN | ERRCONFIG_MCUVALIDINTEN |
 			ERRCONFIG_MCUBOUNDINTEN),
 			(ERRCONFIG_MCUACCUMINTEN | ERRCONFIG_MCUACCUMINTST |
 			 ERRCONFIG_MCUVALIDINTEN | ERRCONFIG_MCUVALIDINTST |
 			 ERRCONFIG_MCUBOUNDINTEN | ERRCONFIG_MCUBOUNDINTST));
-	} else if (sr->ip_type == SR_TYPE_V2) {
+		break;
+	case SR_TYPE_V2:
 		sr_write_reg(sr, IRQSTATUS,
 			IRQSTATUS_MCUACCUMINT | IRQSTATUS_MCVALIDINT |
 			IRQSTATUS_MCBOUNDSINT | IRQSTATUS_MCUDISABLEACKINT);
 		sr_write_reg(sr, IRQENABLE_SET,
 			IRQENABLE_MCUACCUMINT | IRQENABLE_MCUVALIDINT |
 			IRQENABLE_MCUBOUNDSINT | IRQENABLE_MCUDISABLEACKINT);
+		break;
+	default:
+		dev_err(&sr->pdev->dev, "UNKNOWN IP type %d\n", sr->ip_type);
+		return -EINVAL;
 	}
 
 	return 0;

@@ -545,10 +585,10 @@ int sr_configure_minmax(struct voltagedomain *voltdm)
  */
 int sr_enable(struct voltagedomain *voltdm, unsigned long volt)
 {
-	u32 nvalue_reciprocal;
-	struct omap_volt_data *volt_data;
-	struct omap_sr *sr = _sr_lookup(voltdm);
-	int ret;
+	struct omap_sr		*sr = _sr_lookup(voltdm);
+	struct omap_volt_data	*volt_data;
+	u32			nvalue_reciprocal;
+	int			ret;
 
 	if (IS_ERR(sr)) {
 		pr_warning("%s: omap_sr struct for sr_%s not found\n",

@@ -602,7 +642,7 @@ int sr_enable(struct voltagedomain *voltdm, unsigned long volt)
  */
 void sr_disable(struct voltagedomain *voltdm)
 {
-	struct omap_sr *sr = _sr_lookup(voltdm);
+	struct omap_sr		*sr = _sr_lookup(voltdm);
 
 	if (IS_ERR(sr)) {
 		pr_warning("%s: omap_sr struct for sr_%s not found\n",

@@ -619,10 +659,17 @@ void sr_disable(struct voltagedomain *voltdm)
 	 * disable the clocks.
 	 */
 	if (sr_read_reg(sr, SRCONFIG) & SRCONFIG_SRENABLE) {
-		if (sr->ip_type == SR_TYPE_V1)
+		switch (sr->ip_type) {
+		case SR_TYPE_V1:
 			sr_v1_disable(sr);
-		else if (sr->ip_type == SR_TYPE_V2)
+			break;
+		case SR_TYPE_V2:
 			sr_v2_disable(sr);
+			break;
+		default:
+			dev_err(&sr->pdev->dev, "UNKNOWN IP type %d\n",
+					sr->ip_type);
+		}
 	}
 
 	pm_runtime_put_sync_suspend(&sr->pdev->dev);

@@ -638,7 +685,7 @@ void sr_disable(struct voltagedomain *voltdm)
  */
 int sr_register_class(struct omap_sr_class_data *class_data)
 {
-	struct omap_sr *sr_info;
+	struct omap_sr		*sr_info;
 
 	if (!class_data) {
 		pr_warning("%s:, Smartreflex class data passed is NULL\n",

@@ -676,7 +723,7 @@ int sr_register_class(struct omap_sr_class_data *class_data)
  */
 void omap_sr_enable(struct voltagedomain *voltdm)
 {
-	struct omap_sr *sr = _sr_lookup(voltdm);
+	struct omap_sr		*sr = _sr_lookup(voltdm);
 
 	if (IS_ERR(sr)) {
 		pr_warning("%s: omap_sr struct for sr_%s not found\n",

@@ -714,7 +761,7 @@ void omap_sr_enable(struct voltagedomain *voltdm)
  */
 void omap_sr_disable(struct voltagedomain *voltdm)
 {
-	struct omap_sr *sr = _sr_lookup(voltdm);
+	struct omap_sr		*sr = _sr_lookup(voltdm);
 
 	if (IS_ERR(sr)) {
 		pr_warning("%s: omap_sr struct for sr_%s not found\n",

@@ -752,7 +799,7 @@ void omap_sr_disable(struct voltagedomain *voltdm)
  */
 void omap_sr_disable_reset_volt(struct voltagedomain *voltdm)
 {
-	struct omap_sr *sr = _sr_lookup(voltdm);
+	struct omap_sr		*sr = _sr_lookup(voltdm);
 
 	if (IS_ERR(sr)) {
 		pr_warning("%s: omap_sr struct for sr_%s not found\n",

@@ -796,10 +843,10 @@ void omap_sr_register_pmic(struct omap_sr_pmic_data *pmic_data)
 	sr_pmic_data = pmic_data;
 }
 
-/* PM Debug Fs enteries to enable disable smartreflex. */
+/* PM Debug Fs entries to enable disable smartreflex. */
 static int omap_sr_autocomp_show(void *data, u64 *val)
 {
-	struct omap_sr *sr_info = (struct omap_sr *) data;
+	struct omap_sr		*sr_info = data;
 
 	if (!sr_info) {
 		pr_warning("%s: omap_sr struct not found\n", __func__);

@@ -813,7 +860,7 @@ static int omap_sr_autocomp_show(void *data, u64 *val)
 
 static int omap_sr_autocomp_store(void *data, u64 val)
 {
-	struct omap_sr *sr_info = (struct omap_sr *) data;
+	struct omap_sr		*sr_info = data;
 
 	if (!sr_info) {
 		pr_warning("%s: omap_sr struct not found\n", __func__);

@@ -844,14 +891,17 @@ DEFINE_SIMPLE_ATTRIBUTE(pm_sr_fops, omap_sr_autocomp_show,
 
 static int __init omap_sr_probe(struct platform_device *pdev)
 {
-	struct omap_sr *sr_info = kzalloc(sizeof(struct omap_sr), GFP_KERNEL);
-	struct omap_sr_data *pdata = pdev->dev.platform_data;
-	struct resource *mem, *irq;
-	struct dentry *nvalue_dir;
-	struct omap_volt_data *volt_data;
-	int i, ret = 0;
-	char *name;
-
+	struct omap_sr		*sr_info;
+	struct omap_sr_data	*pdata = pdev->dev.platform_data;
+	struct resource		*mem;
+	struct resource		*irq;
+	struct dentry		*nvalue_dir;
+	struct omap_volt_data	*volt_data;
+	int			ret = 0;
+	int			i;
+	char			*name;
+
+	sr_info = kzalloc(sizeof(*sr_info), GFP_KERNEL);
 	if (!sr_info) {
 		dev_err(&pdev->dev, "%s: unable to allocate sr_info\n",
 			__func__);

@@ -1002,8 +1052,8 @@ err_free_devinfo:
 
 static int __devexit omap_sr_remove(struct platform_device *pdev)
 {
-	struct omap_sr *sr_info = platform_get_drvdata(pdev);
-	struct resource *mem;
+	struct omap_sr		*sr_info = platform_get_drvdata(pdev);
+	struct resource		*mem;
 
 	if (sr_info->autocomp_active)
 		sr_stop_vddautocomp(sr_info);

@@ -1021,7 +1071,7 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
 
 static int omap_sr_suspend(struct device *dev)
 {
-	struct omap_sr *sr_info = dev_get_drvdata(dev);
+	struct omap_sr		*sr_info = dev_get_drvdata(dev);
 
 	if (!sr_info)
 		return 0;

@@ -1042,7 +1092,7 @@ static int omap_sr_suspend(struct device *dev)
 
 static int omap_sr_resume(struct device *dev)
 {
-	struct omap_sr *sr_info = dev_get_drvdata(dev);
+	struct omap_sr		*sr_info = dev_get_drvdata(dev);
 
 	if (!sr_info)
 		return 0;
Signature · 1 line
1.7.8.rc0
#9 lore raw

Re: [PATCH/RESEND #3 5/7] arm: omap: smartreflex: clean ups all over

arch/arm/mach-omap2/smartreflex.c line 43

Quoted 6 lines · Felipe Balbi
arch/arm/mach-omap2/smartreflex.c +4 -2
-	int				srid;
-	int				ip_type;
+	struct list_head		node;
+	struct platform_device		*pdev;
+	struct omap_sr_nvalue_table	*nvalue_table;
+	struct voltagedomain		*voltdm;

As long as you're de-tabifying the function names that caused the problem...

arch/arm/mach-omap2/smartreflex.c line 44

Quoted 3 lines
arch/arm/mach-omap2/smartreflex.c +0 -2
 	int				nvalue_count;
-	bool				autocomp_active;
-	bool				is_suspended;

Why are you leaveing the rest of the struct tabified out to East Podunk?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111115/e07b4607/attachment.sig>

author
#10 lore raw

Re: [PATCH/RESEND #3 5/7] arm: omap: smartreflex: clean ups all over

Quoted 11 lines · Valdis.Kletnieks at vt.edu

arch/arm/mach-omap2/smartreflex.c line 43

Quoted 6 lines · Felipe Balbi
arch/arm/mach-omap2/smartreflex.c +4 -2
-	int				srid;
-	int				ip_type;
+	struct list_head		node;
+	struct platform_device		*pdev;
+	struct omap_sr_nvalue_table	*nvalue_table;
+	struct voltagedomain		*voltdm;

As long as you're de-tabifying the function names that caused the problem...

Quoted 5 lines

arch/arm/mach-omap2/smartreflex.c line 44

Quoted 3 lines
arch/arm/mach-omap2/smartreflex.c +0 -2
 	int				nvalue_count;
-	bool				autocomp_active;
-	bool				is_suspended;

Why are you leaveing the rest of the struct tabified out to East Podunk?

de-tabifying ? I tabified the entire thing... Everything is aligned.

Signature · 8 lines
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111116/f611e4aa/attachment.sig>
author
#11 lore raw

PATCH 6/7 arm: omap: smartreflex: fix IRQ handling bug

In reply to Felipe Balbi

fix a bug which has been on this driver since
it was added by the original commit 984aa6db
which would never clear IRQSTATUS bits.

Signed-off-by Felipe Balbi <[email protected]>
---
 arch/arm/mach-omap2/smartreflex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/smartreflex.c +1 -1
index eb88b36..a5399df 100644
@@ -156,7 +156,7 @@ static irqreturn_t sr_interrupt(int irq, void *data)
 		break;
 	case SR_TYPE_V2:
 		/* Read the status bits */
-		sr_read_reg(sr_info, IRQSTATUS);
+		status = sr_read_reg(sr_info, IRQSTATUS);
 
 		/* Clear them by writing back */
 		sr_write_reg(sr_info, IRQSTATUS, status);
Signature · 1 line
1.7.8.rc0
#12 lore raw

Re: [PATCH/RESEND #3 6/7] arm: omap: smartreflex: fix IRQ handling bug

Felipe Balbi <[email protected]> writes:

F> fix a bug which has been on this driver since

Quoted 4 lines

it was added by the original commit 984aa6db
which would never clear IRQSTATUS bits.

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

Queuing this fix for v3.2-rc (branch: for_3.2/fixes/pm)

Kevin

author
#13 lore raw

Re: [PATCH/RESEND #3 6/7] arm: omap: smartreflex: fix IRQ handling bug

Quoted 9 lines · Kevin Hilman

Felipe Balbi <[email protected]> writes:

F> fix a bug which has been on this driver since

Quoted 4 lines

it was added by the original commit 984aa6db
which would never clear IRQSTATUS bits.

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

Queuing this fix for v3.2-rc (branch: for_3.2/fixes/pm)

should it go to stable too ??

Signature · 8 lines
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111117/340ef8ba/attachment.sig>
author
#14 lore raw

PATCH 7/7 arm: omap: smartreflex: micro-optimization for sanity check

In reply to Felipe Balbi

val && (val != 1) == val > 1

Signed-off-by Felipe Balbi <[email protected]>
---
 arch/arm/mach-omap2/smartreflex.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/arm/mach-omap2/smartreflex.c +1 -1
index a5399df..9fee277 100644
@@ -868,7 +868,7 @@ static int omap_sr_autocomp_store(void *data, u64 val)
 	}
 
 	/* Sanity check */
-	if (val && (val != 1)) {
+	if (val > 1) {
 		pr_warning("%s: Invalid argument %lld\n", __func__, val);
 		return -EINVAL;
 	}
Signature · 1 line
1.7.8.rc0
#15 lore raw
In reply to Felipe Balbi

+Jean

Quoted 2 lines · Felipe Balbi

here's a bunch of patches to smartreflex which have been pending for months.

The SmartReflex code is going through a major rework by Jean so it can be converted into a real driver and moved under drivers/*.

I'll leave it to Jean to incorporate these changes into his rework as he sees fit, although I've queued the IRQ fix for v3.2-rc.

Kevin