| Message ID | 1306481908-7386-3-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 * > *
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
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
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/
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
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
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
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 * *
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(-)