[PATCHv2 0/2] Some omap_device/hwmod/pwrdomain patches

[PATCHv2 0/2] Some omap_device/hwmod/pwrdomain patches

[Tomi Valkeinen]

I came up with these patches while implementing pm runtime adaptation for DSS
driver. I'm not quite sure on who's turf they belong, or do they even belong
together, but here they are anyway.

get_context_loss_count patch was discussed on l-o with Kevin.

The omap_device_reset patch I made as some parts of DSS currently presume that
the HW module is reset when it is enabled, and the reset is better handled in
hwmod code.

Changes in v2
* Dropped can-ever-lose-context patch
* Use INT_MAX instead of ~(1 << 31) when masking the ctx loss count

 Tomi

Tomi Valkeinen (2):
  OMAP: change get_context_loss_count ret value to int
  OMAP: add omap_device_reset()

 arch/arm/mach-omap2/omap_hwmod.c              |    2 +-
 arch/arm/mach-omap2/powerdomain.c             |   14 ++++++++++----
 arch/arm/mach-omap2/powerdomain.h             |    2 +-
 arch/arm/plat-omap/include/plat/omap-pm.h     |    4 ++--
 arch/arm/plat-omap/include/plat/omap_device.h |    3 ++-
 arch/arm/plat-omap/include/plat/omap_hwmod.h  |    2 +-
 arch/arm/plat-omap/omap-pm-noop.c             |   24 +++++++++++++++++-------
 arch/arm/plat-omap/omap_device.c              |   25 ++++++++++++++++++++++++-
 8 files changed, 58 insertions(+), 18 deletions(-)

-- 
1.7.4.1

[PATCHv2 1/2] OMAP: change get_context_loss_count ret value to int

[Tomi Valkeinen]

get_context_loss_count functions return context loss count as u32, and
zero means an error. However, zero is also returned when context has
never been lost and could also be returned when the context loss count
has wrapped and goes to zero.

Change the functions to return an int, with negative value meaning an
error.

OMAP HSMMC code uses omap_pm_get_dev_context_loss_count(), but as the
hsmmc code handles the returned value as an int, with negative value
meaning an error, this patch actually fixes hsmmc code also.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Acked-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod.c              |    2 +-
 arch/arm/mach-omap2/powerdomain.c             |   14 ++++++++++----
 arch/arm/mach-omap2/powerdomain.h             |    2 +-
 arch/arm/plat-omap/include/plat/omap-pm.h     |    4 ++--
 arch/arm/plat-omap/include/plat/omap_device.h |    2 +-
 arch/arm/plat-omap/include/plat/omap_hwmod.h  |    2 +-
 arch/arm/plat-omap/omap-pm-noop.c             |   24 +++++++++++++++++-------
 arch/arm/plat-omap/omap_device.c              |    2 +-
 8 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index e034294..4f0d554 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2332,7 +2332,7 @@ ohsps_unlock:
  * Returns the context loss count of the powerdomain assocated with @oh
  * upon success, or zero if no powerdomain exists for @oh.
  */
-u32 omap_hwmod_get_context_loss_count(struct omap_hwmod *oh)
+int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh)
 {
 	struct powerdomain *pwrdm;
 	int ret = 0;
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 9af0847..9d53a34 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -935,16 +935,16 @@ int pwrdm_post_transition(void)
  * @pwrdm: struct powerdomain * to wait for
  *
  * Context loss count is the sum of powerdomain off-mode counter, the
- * logic off counter and the per-bank memory off counter.  Returns 0
+ * logic off counter and the per-bank memory off counter.  Returns negative
  * (and WARNs) upon error, otherwise, returns the context loss count.
  */
-u32 pwrdm_get_context_loss_count(struct powerdomain *pwrdm)
+int pwrdm_get_context_loss_count(struct powerdomain *pwrdm)
 {
 	int i, count;
 
 	if (!pwrdm) {
 		WARN(1, "powerdomain: %s: pwrdm is null\n", __func__);
-		return 0;
+		return -ENODEV;
 	}
 
 	count = pwrdm->state_counter[PWRDM_POWER_OFF];
@@ -953,7 +953,13 @@ u32 pwrdm_get_context_loss_count(struct powerdomain *pwrdm)
 	for (i = 0; i < pwrdm->banks; i++)
 		count += pwrdm->ret_mem_off_counter[i];
 
-	pr_debug("powerdomain: %s: context loss count = %u\n",
+	/*
+	 * Context loss count has to be a non-negative value. Clear the sign
+	 * bit to get a value range from 0 to INT_MAX.
+	 */
+	count &= INT_MAX;
+
+	pr_debug("powerdomain: %s: context loss count = %d\n",
 		 pwrdm->name, count);
 
 	return count;
diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
index d23d979..012827f 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -207,7 +207,7 @@ int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
 int pwrdm_pre_transition(void);
 int pwrdm_post_transition(void);
 int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm);
-u32 pwrdm_get_context_loss_count(struct powerdomain *pwrdm);
+int pwrdm_get_context_loss_count(struct powerdomain *pwrdm);
 bool pwrdm_can_ever_lose_context(struct powerdomain *pwrdm);
 
 extern void omap2xxx_powerdomains_init(void);
diff --git a/arch/arm/plat-omap/include/plat/omap-pm.h b/arch/arm/plat-omap/include/plat/omap-pm.h
index c0a7520..68df031 100644
--- a/arch/arm/plat-omap/include/plat/omap-pm.h
+++ b/arch/arm/plat-omap/include/plat/omap-pm.h
@@ -350,9 +350,9 @@ unsigned long omap_pm_cpu_get_freq(void);
  * driver must restore device context.   If the number of context losses
  * exceeds the maximum positive integer, the function will wrap to 0 and
  * continue counting.  Returns the number of context losses for this device,
- * or zero upon error.
+ * or negative value upon error.
  */
-u32 omap_pm_get_dev_context_loss_count(struct device *dev);
+int omap_pm_get_dev_context_loss_count(struct device *dev);
 
 void omap_pm_enable_off_mode(void);
 void omap_pm_disable_off_mode(void);
diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index e4c349f..70d31d0 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -107,7 +107,7 @@ void __iomem *omap_device_get_rt_va(struct omap_device *od);
 int omap_device_align_pm_lat(struct platform_device *pdev,
 			     u32 new_wakeup_lat_limit);
 struct powerdomain *omap_device_get_pwrdm(struct omap_device *od);
-u32 omap_device_get_context_loss_count(struct platform_device *pdev);
+int omap_device_get_context_loss_count(struct platform_device *pdev);
 
 /* Other */
 
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 1adea9c..8658e2d 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -598,7 +598,7 @@ int omap_hwmod_for_each_by_class(const char *classname,
 				 void *user);
 
 int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state);
-u32 omap_hwmod_get_context_loss_count(struct omap_hwmod *oh);
+int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh);
 
 int omap_hwmod_no_setup_reset(struct omap_hwmod *oh);
 
diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
index b0471bb2..3dc3801 100644
--- a/arch/arm/plat-omap/omap-pm-noop.c
+++ b/arch/arm/plat-omap/omap-pm-noop.c
@@ -27,7 +27,7 @@
 #include <plat/omap_device.h>
 
 static bool off_mode_enabled;
-static u32 dummy_context_loss_counter;
+static int dummy_context_loss_counter;
 
 /*
  * Device-driver-originated constraints (via board-*.c files)
@@ -311,22 +311,32 @@ void omap_pm_disable_off_mode(void)
 
 #ifdef CONFIG_ARCH_OMAP2PLUS
 
-u32 omap_pm_get_dev_context_loss_count(struct device *dev)
+int omap_pm_get_dev_context_loss_count(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
-	u32 count;
+	int count;
 
 	if (WARN_ON(!dev))
-		return 0;
+		return -ENODEV;
 
 	if (dev->parent == &omap_device_parent) {
 		count = omap_device_get_context_loss_count(pdev);
 	} else {
 		WARN_ONCE(off_mode_enabled, "omap_pm: using dummy context loss counter; device %s should be converted to omap_device",
 			  dev_name(dev));
-		if (off_mode_enabled)
-			dummy_context_loss_counter++;
+
 		count = dummy_context_loss_counter;
+
+		if (off_mode_enabled) {
+			count++;
+			/*
+			 * Context loss count has to be a non-negative value.
+			 * Clear the sign bit to get a value range from 0 to
+			 * INT_MAX.
+			 */
+			count &= INT_MAX;
+			dummy_context_loss_counter = count;
+		}
 	}
 
 	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
@@ -337,7 +347,7 @@ u32 omap_pm_get_dev_context_loss_count(struct device *dev)
 
 #else
 
-u32 omap_pm_get_dev_context_loss_count(struct device *dev)
+int omap_pm_get_dev_context_loss_count(struct device *dev)
 {
 	return dummy_context_loss_counter;
 }
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index 9bbda9a..9753f71 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -310,7 +310,7 @@ static void _add_optional_clock_clkdev(struct omap_device *od,
  * return the context loss counter for that hwmod, otherwise return
  * zero.
  */
-u32 omap_device_get_context_loss_count(struct platform_device *pdev)
+int omap_device_get_context_loss_count(struct platform_device *pdev)
 {
 	struct omap_device *od;
 	u32 ret = 0;
-- 
1.7.4.1

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Tomi Valkeinen]

Add omap_device_reset() function which can be used to reset the hwmods
associated with the given platform device.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 arch/arm/plat-omap/include/plat/omap_device.h |    1 +
 arch/arm/plat-omap/omap_device.c              |   23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index 70d31d0..bcf1b35 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -108,6 +108,7 @@ int omap_device_align_pm_lat(struct platform_device *pdev,
 			     u32 new_wakeup_lat_limit);
 struct powerdomain *omap_device_get_pwrdm(struct omap_device *od);
 int omap_device_get_context_loss_count(struct platform_device *pdev);
+int omap_device_reset(struct platform_device *pdev);
 
 /* Other */
 
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index 9753f71..4e6fc1b 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -324,6 +324,29 @@ int omap_device_get_context_loss_count(struct platform_device *pdev)
 }
 
 /**
+ * omap_device_reset - reset hwmods of given device
+ * @pdev: struct platform_device *
+ *
+ * Reset all hwmods associated with the given device. If a reset of a hwmod
+ * fails the rest of the hwmods are skipped and the error is returned.
+ */
+int omap_device_reset(struct platform_device *pdev)
+{
+	struct omap_device *od;
+	int i, r;
+
+	od = _find_by_pdev(pdev);
+
+	for (i = 0; i < od->hwmods_cnt; i++) {
+		r = omap_hwmod_reset(od->hwmods[i]);
+		if (r)
+			return r;
+	}
+
+	return 0;
+}
+
+/**
  * omap_device_count_resources - count number of struct resource entries needed
  * @od: struct omap_device *
  *
-- 
1.7.4.1

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Cousson, Benoit]

Hi Tomi,

On 5/27/2011 9:38 AM, Valkeinen, Tomi wrote:
> Add omap_device_reset() function which can be used to reset the hwmods
> associated with the given platform device.

We've never exposed it because we are trying to avoid that any driver 
play with asynchronous HW reset. That can lead to undefined HW behavior :-(

Do you have some strong need for that?

Regards,
Benoit

>
> Signed-off-by: Tomi Valkeinen<tomi.valkeinen@ti.com>
> ---
>   arch/arm/plat-omap/include/plat/omap_device.h |    1 +
>   arch/arm/plat-omap/omap_device.c              |   23 +++++++++++++++++++++++
>   2 files changed, 24 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
> index 70d31d0..bcf1b35 100644
> --- a/arch/arm/plat-omap/include/plat/omap_device.h
> +++ b/arch/arm/plat-omap/include/plat/omap_device.h
> @@ -108,6 +108,7 @@ int omap_device_align_pm_lat(struct platform_device *pdev,
>   			     u32 new_wakeup_lat_limit);
>   struct powerdomain *omap_device_get_pwrdm(struct omap_device *od);
>   int omap_device_get_context_loss_count(struct platform_device *pdev);
> +int omap_device_reset(struct platform_device *pdev);
>
>   /* Other */
>
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index 9753f71..4e6fc1b 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -324,6 +324,29 @@ int omap_device_get_context_loss_count(struct platform_device *pdev)
>   }
>
>   /**
> + * omap_device_reset - reset hwmods of given device
> + * @pdev: struct platform_device *
> + *
> + * Reset all hwmods associated with the given device. If a reset of a hwmod
> + * fails the rest of the hwmods are skipped and the error is returned.
> + */
> +int omap_device_reset(struct platform_device *pdev)
> +{
> +	struct omap_device *od;
> +	int i, r;
> +
> +	od = _find_by_pdev(pdev);
> +
> +	for (i = 0; i<  od->hwmods_cnt; i++) {
> +		r = omap_hwmod_reset(od->hwmods[i]);
> +		if (r)
> +			return r;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>    * omap_device_count_resources - count number of struct resource entries needed
>    * @od: struct omap_device *
>    *

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Tomi Valkeinen]

On Fri, 2011-05-27 at 14:38 +0200, Cousson, Benoit wrote:
> Hi Tomi,
> 
> On 5/27/2011 9:38 AM, Valkeinen, Tomi wrote:
> > Add omap_device_reset() function which can be used to reset the hwmods
> > associated with the given platform device.
> 
> We've never exposed it because we are trying to avoid that any driver 
> play with asynchronous HW reset. That can lead to undefined HW behavior :-(
> 
> Do you have some strong need for that?

DSS driver has been designed so that it resets the HW before it begins
programming it. That way we get the HW into known state. Otherwise we
need to be extra careful to program all possible registers to a sane
value. Not impossible, of course, but requires extra work.

I noticed the problem with DSI driver, it didn't work anymore if I
didn't reset it.

Why does it lead to undefined HW behaviour? Isn't it much better to
reset the HW before starting to use it to be 100% sure it's in known and
valid state?

Especially in error situations it may be difficult (even impossible) to
recover without reset. DISPC has been known to froze in some sync lost
situations, and, if I recall right, if DSI transfer is aborted the only
way to recover is to reset the DSI block (on OMAP3).

 Tomi

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Cousson, Benoit]

On 5/27/2011 2:46 PM, Valkeinen, Tomi wrote:
> On Fri, 2011-05-27 at 14:38 +0200, Cousson, Benoit wrote:
>> Hi Tomi,
>>
>> On 5/27/2011 9:38 AM, Valkeinen, Tomi wrote:
>>> Add omap_device_reset() function which can be used to reset the hwmods
>>> associated with the given platform device.
>>
>> We've never exposed it because we are trying to avoid that any driver
>> play with asynchronous HW reset. That can lead to undefined HW behavior :-(
>>
>> Do you have some strong need for that?
>
> DSS driver has been designed so that it resets the HW before it begins
> programming it. That way we get the HW into known state. Otherwise we
> need to be extra careful to program all possible registers to a sane
> value. Not impossible, of course, but requires extra work.
>
> I noticed the problem with DSI driver, it didn't work anymore if I
> didn't reset it.
>
> Why does it lead to undefined HW behaviour? Isn't it much better to
> reset the HW before starting to use it to be 100% sure it's in known and
> valid state?

In theory, but since your are resetting only the DSS IP, it can leads to 
side effect at SoC level. Especially wrt to clock management.

> Especially in error situations it may be difficult (even impossible) to
> recover without reset. DISPC has been known to froze in some sync lost
> situations, and, if I recall right, if DSI transfer is aborted the only
> way to recover is to reset the DSI block (on OMAP3).

In case of recovery error it makes sense. What we did with hardreset is 
to re-assert the reset upon disable of the module and then the next 
enable will de-assert it. Softreset does not do that today.

My only concern is that people might start abusing that kind of API to 
use that for funtionnal purpose instead of error recovery.

This other issue is that it will add another OMAP specific IP to the driver.

Benoit

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Santosh Shilimkar]

On 5/27/2011 8:13 PM, Cousson, Benoit wrote:
> On 5/27/2011 2:46 PM, Valkeinen, Tomi wrote:
>> On Fri, 2011-05-27 at 14:38 +0200, Cousson, Benoit wrote:
>>> Hi Tomi,
>>>
>>> On 5/27/2011 9:38 AM, Valkeinen, Tomi wrote:
>>>> Add omap_device_reset() function which can be used to reset the hwmods
>>>> associated with the given platform device.
>>>
>>> We've never exposed it because we are trying to avoid that any driver
>>> play with asynchronous HW reset. That can lead to undefined HW
>>> behavior :-(
>>>
>>> Do you have some strong need for that?
>>
>> DSS driver has been designed so that it resets the HW before it begins
>> programming it. That way we get the HW into known state. Otherwise we
>> need to be extra careful to program all possible registers to a sane
>> value. Not impossible, of course, but requires extra work.
>>
>> I noticed the problem with DSI driver, it didn't work anymore if I
>> didn't reset it.
>>
>> Why does it lead to undefined HW behaviour? Isn't it much better to
>> reset the HW before starting to use it to be 100% sure it's in known and
>> valid state?
>
> In theory, but since your are resetting only the DSS IP, it can leads to
> side effect at SoC level. Especially wrt to clock management.
>
>> Especially in error situations it may be difficult (even impossible) to
>> recover without reset. DISPC has been known to froze in some sync lost
>> situations, and, if I recall right, if DSI transfer is aborted the only
>> way to recover is to reset the DSI block (on OMAP3).
>
> In case of recovery error it makes sense. What we did with hardreset is
> to re-assert the reset upon disable of the module and then the next
> enable will de-assert it. Softreset does not do that today.
>
I didn't notice this patch but Paul reported an issue on beagle
which was making L3 error handling driver hang.

Later on after debugging we noticed, that DSS initiator
was throwing timeout error.

As a temporary fix, we removed the timeout error from
the handler since root-cause was not known. [1]

I am not sure but may be a proper DSS reset might fix
that issue as well.

Regards
Santosh
[1] https://patchwork.kernel.org/patch/769482/

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Cousson, Benoit]

On 5/27/2011 4:51 PM, Shilimkar, Santosh wrote:
> On 5/27/2011 8:13 PM, Cousson, Benoit wrote:
>> On 5/27/2011 2:46 PM, Valkeinen, Tomi wrote:
>>> On Fri, 2011-05-27 at 14:38 +0200, Cousson, Benoit wrote:
>>>> Hi Tomi,
>>>>
>>>> On 5/27/2011 9:38 AM, Valkeinen, Tomi wrote:
>>>>> Add omap_device_reset() function which can be used to reset the hwmods
>>>>> associated with the given platform device.
>>>>
>>>> We've never exposed it because we are trying to avoid that any driver
>>>> play with asynchronous HW reset. That can lead to undefined HW
>>>> behavior :-(
>>>>
>>>> Do you have some strong need for that?
>>>
>>> DSS driver has been designed so that it resets the HW before it begins
>>> programming it. That way we get the HW into known state. Otherwise we
>>> need to be extra careful to program all possible registers to a sane
>>> value. Not impossible, of course, but requires extra work.
>>>
>>> I noticed the problem with DSI driver, it didn't work anymore if I
>>> didn't reset it.
>>>
>>> Why does it lead to undefined HW behaviour? Isn't it much better to
>>> reset the HW before starting to use it to be 100% sure it's in known and
>>> valid state?
>>
>> In theory, but since your are resetting only the DSS IP, it can leads to
>> side effect at SoC level. Especially wrt to clock management.
>>
>>> Especially in error situations it may be difficult (even impossible) to
>>> recover without reset. DISPC has been known to froze in some sync lost
>>> situations, and, if I recall right, if DSI transfer is aborted the only
>>> way to recover is to reset the DSI block (on OMAP3).
>>
>> In case of recovery error it makes sense. What we did with hardreset is
>> to re-assert the reset upon disable of the module and then the next
>> enable will de-assert it. Softreset does not do that today.
>>
> I didn't notice this patch but Paul reported an issue on beagle
> which was making L3 error handling driver hang.
>
> Later on after debugging we noticed, that DSS initiator
> was throwing timeout error.
>
> As a temporary fix, we removed the timeout error from
> the handler since root-cause was not known. [1]
>
> I am not sure but may be a proper DSS reset might fix
> that issue as well.

Yeah, but this is the issue... people will start abusing that to fix any 
kind of problems instead of finding the root cause.

That should be use only to fix real HW issue that cannot be solve 
properly by SW.

Regards,
Benoit

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Santosh Shilimkar]

On 5/27/2011 8:30 PM, Cousson, Benoit wrote:
> On 5/27/2011 4:51 PM, Shilimkar, Santosh wrote:
>> On 5/27/2011 8:13 PM, Cousson, Benoit wrote:
>>> On 5/27/2011 2:46 PM, Valkeinen, Tomi wrote:
>>>> On Fri, 2011-05-27 at 14:38 +0200, Cousson, Benoit wrote:
>>>>> Hi Tomi,
>>>>>
>>>>> On 5/27/2011 9:38 AM, Valkeinen, Tomi wrote:
>>>>>> Add omap_device_reset() function which can be used to reset the
>>>>>> hwmods
>>>>>> associated with the given platform device.
>>>>>
>>>>> We've never exposed it because we are trying to avoid that any driver
>>>>> play with asynchronous HW reset. That can lead to undefined HW
>>>>> behavior :-(
>>>>>
>>>>> Do you have some strong need for that?
>>>>
>>>> DSS driver has been designed so that it resets the HW before it begins
>>>> programming it. That way we get the HW into known state. Otherwise we
>>>> need to be extra careful to program all possible registers to a sane
>>>> value. Not impossible, of course, but requires extra work.
>>>>
>>>> I noticed the problem with DSI driver, it didn't work anymore if I
>>>> didn't reset it.
>>>>
>>>> Why does it lead to undefined HW behaviour? Isn't it much better to
>>>> reset the HW before starting to use it to be 100% sure it's in known
>>>> and
>>>> valid state?
>>>
>>> In theory, but since your are resetting only the DSS IP, it can leads to
>>> side effect at SoC level. Especially wrt to clock management.
>>>
>>>> Especially in error situations it may be difficult (even impossible) to
>>>> recover without reset. DISPC has been known to froze in some sync lost
>>>> situations, and, if I recall right, if DSI transfer is aborted the only
>>>> way to recover is to reset the DSI block (on OMAP3).
>>>
>>> In case of recovery error it makes sense. What we did with hardreset is
>>> to re-assert the reset upon disable of the module and then the next
>>> enable will de-assert it. Softreset does not do that today.
>>>
>> I didn't notice this patch but Paul reported an issue on beagle
>> which was making L3 error handling driver hang.
>>
>> Later on after debugging we noticed, that DSS initiator
>> was throwing timeout error.
>>
>> As a temporary fix, we removed the timeout error from
>> the handler since root-cause was not known. [1]
>>
>> I am not sure but may be a proper DSS reset might fix
>> that issue as well.
>
> Yeah, but this is the issue... people will start abusing that to fix any
> kind of problems instead of finding the root cause.
>
May be you are right. But shouldn't we do a proper reset of the IP.
On this same topic, I have a patch for timer IP too. As per
TRM, soft reset is not enough to properly reset the timer IP
and expecting the additional register to be used for
reseting the timer IP.

> That should be use only to fix real HW issue that cannot be solve
> properly by SW.
>
I agree but TRM doesn't say the softreset is enough to ensure
the certain IP's are properly reseted.

Regards
Santosh

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Tomi Valkeinen]

On Fri, 2011-05-27 at 16:43 +0200, Cousson, Benoit wrote:
> On 5/27/2011 2:46 PM, Valkeinen, Tomi wrote:
> > On Fri, 2011-05-27 at 14:38 +0200, Cousson, Benoit wrote:
> >> Hi Tomi,
> >>
> >> On 5/27/2011 9:38 AM, Valkeinen, Tomi wrote:
> >>> Add omap_device_reset() function which can be used to reset the hwmods
> >>> associated with the given platform device.
> >>
> >> We've never exposed it because we are trying to avoid that any driver
> >> play with asynchronous HW reset. That can lead to undefined HW behavior :-(
> >>
> >> Do you have some strong need for that?
> >
> > DSS driver has been designed so that it resets the HW before it begins
> > programming it. That way we get the HW into known state. Otherwise we
> > need to be extra careful to program all possible registers to a sane
> > value. Not impossible, of course, but requires extra work.
> >
> > I noticed the problem with DSI driver, it didn't work anymore if I
> > didn't reset it.
> >
> > Why does it lead to undefined HW behaviour? Isn't it much better to
> > reset the HW before starting to use it to be 100% sure it's in known and
> > valid state?
> 
> In theory, but since your are resetting only the DSS IP, it can leads to 
> side effect at SoC level. Especially wrt to clock management.

Out of interest, can you tell more what problems it could cause? Can
they be solved in the hwmod reset code?

Also one thing to note, VENC needs a softreset after changing certain
configurations, as per TRM. However, VENC doesn't use the standard
syscontrol mechanism, so that cannot be done via omap_device interface
anyway.

> > Especially in error situations it may be difficult (even impossible) to
> > recover without reset. DISPC has been known to froze in some sync lost
> > situations, and, if I recall right, if DSI transfer is aborted the only
> > way to recover is to reset the DSI block (on OMAP3).
> 
> In case of recovery error it makes sense. What we did with hardreset is 
> to re-assert the reset upon disable of the module and then the next 
> enable will de-assert it. Softreset does not do that today.
> 
> My only concern is that people might start abusing that kind of API to 
> use that for funtionnal purpose instead of error recovery.

Why do you see it's abusing? I think it's a good driver design to reset
the HW before starting operation. Perhaps it's true that in some cases
reset could hide (or fix, in a way) a bug in the driver, but in the end
what we want is a driver that works in every situation. And doing a HW
reset sounds a good way to make the driver more robust.

> This other issue is that it will add another OMAP specific IP to the driver.

I presume IP == API. And that is true. But isn't reset functionality
present in most HW blocks, and thus would it make a good addition to the
generic API?

 Tomi

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Paul Walmsley]

On Fri, 27 May 2011, Tomi Valkeinen wrote:

> However, VENC doesn't use the standard syscontrol mechanism, so that 
> cannot be done via omap_device interface anyway.

struct omap_hwmod_class has a .reset function pointer for this purpose.  
It's already been necessary to deal with the I2C IP block:

   https://patchwork.kernel.org/patch/804072/

If all that's needed are the steps in 4430 TRM Rev. T Section 10.6.5.1.2.3 
"Subsequence--Video Encoder Software Reset", then it should be possible to 
handle this internal reset with hwmod code.

- Paul

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Tomi Valkeinen]

On Sun, 2011-05-29 at 20:15 -0600, Paul Walmsley wrote:
> On Fri, 27 May 2011, Tomi Valkeinen wrote:
> 
> > However, VENC doesn't use the standard syscontrol mechanism, so that 
> > cannot be done via omap_device interface anyway.
> 
> struct omap_hwmod_class has a .reset function pointer for this purpose.  
> It's already been necessary to deal with the I2C IP block:
> 
>    https://patchwork.kernel.org/patch/804072/
> 
> If all that's needed are the steps in 4430 TRM Rev. T Section 10.6.5.1.2.3 
> "Subsequence--Video Encoder Software Reset", then it should be possible to 
> handle this internal reset with hwmod code.

Yes, that should be enough. However, the VENC_F_CONTROL register also
contains bits that need to be programmed by the driver. I hope that
doesn't cause any problems. Probably not, as hwmod code should use that
register only at reset time.

 Tomi

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Cousson, Benoit]

On 5/30/2011 4:15 AM, Paul Walmsley wrote:
> On Fri, 27 May 2011, Tomi Valkeinen wrote:
>
>> However, VENC doesn't use the standard syscontrol mechanism, so that
>> cannot be done via omap_device interface anyway.
>
> struct omap_hwmod_class has a .reset function pointer for this purpose.
> It's already been necessary to deal with the I2C IP block:
>
>     https://patchwork.kernel.org/patch/804072/
>
> If all that's needed are the steps in 4430 TRM Rev. T Section 10.6.5.1.2.3
> "Subsequence--Video Encoder Software Reset", then it should be possible to
> handle this internal reset with hwmod code.

Mmm, but in that case, if this is purely handled inside the IP, it is up 
to the driver to manage that.

hwmod should not manage more than the SYSCONFIG ocpreset because this is 
part of the PRCM/OCP wrapper and not really the IP.

In the case of the VENC, the reset is part of the regular init sequence, 
so the driver will take care of it.

Don't you think so?

Regards,
Benoit

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Paul Walmsley]

Hi Tomi,

On Fri, 27 May 2011, Tomi Valkeinen wrote:

> DSS driver has been designed so that it resets the HW before it begins
> programming it. That way we get the HW into known state. Otherwise we
> need to be extra careful to program all possible registers to a sane
> value. Not impossible, of course, but requires extra work.
> 
> I noticed the problem with DSI driver, it didn't work anymore if I
> didn't reset it.

One thing that I don't quite understand about this thread is that the 
hwmod code should be resetting the DSS hwmods when the system boots.  Is 
something not working with this process?

That is a separate case from the error recovery case.  Sounds like we may 
need to expose a reset function for that purpose as well.  Tomi, I don't 
think your proposed patch is enough for this, though: we'd also need to 
reset the hwmod internal state for that module, and reprogram the 
SYSCONFIG bits, etc. as mach-omap2/omap_hwmod.c:_setup() does.


- Paul

[PATCHv2 2/2] OMAP: add omap_device_reset()

[Tomi Valkeinen]

On Sun, 2011-05-29 at 20:24 -0600, Paul Walmsley wrote:
> Hi Tomi,
> 
> On Fri, 27 May 2011, Tomi Valkeinen wrote:
> 
> > DSS driver has been designed so that it resets the HW before it begins
> > programming it. That way we get the HW into known state. Otherwise we
> > need to be extra careful to program all possible registers to a sane
> > value. Not impossible, of course, but requires extra work.
> > 
> > I noticed the problem with DSI driver, it didn't work anymore if I
> > didn't reset it.
> 
> One thing that I don't quite understand about this thread is that the 
> hwmod code should be resetting the DSS hwmods when the system boots.  Is 
> something not working with this process?

I believe that is working fine.

The problem appears when DSS is turned off (i.e. the screen is blanked)
and then started again. The driver has been written so that it will
reset the DSS HW before starting to use that particular DSS HW block, to
get a clean state. Remove that reset, and we've got all the old register
values lying around, causing the device to malfunction.

This should be fixable in the driver, of course, although my experience
with DSI (and DSS in general) shows that it's not always very easy to
stop and restart the HW without a reset.

However, I'd still argue that a driver is more robust if it does do a
reset when starting up the HW. Perhaps the driver should be developed
without that reset, but in the production device the reset should give
us extra protection for unexpected situations.

> That is a separate case from the error recovery case.  Sounds like we may 
> need to expose a reset function for that purpose as well.  Tomi, I don't 
> think your proposed patch is enough for this, though: we'd also need to 
> reset the hwmod internal state for that module, and reprogram the 
> SYSCONFIG bits, etc. as mach-omap2/omap_hwmod.c:_setup() does.

Ah, ok, sounds right. It worked for me, but I probably wouldn't notice
if the PM bits in SYSCONFIG are wrong, as I didn't do any PM testing.

 Tomi