RE: [PATCH 02/14] dspbridge: deh: trivial cleanups
If "disable" means cutting off the clock of the timer, then no. The clock for the timer is needed so DSP can clear the timer IRQ status register. If it is to stop the timer from counting, then it is fine. -Original Message- From: Jansson, Cris Sent: Monday, May 17, 2010 4:53 PM To: Felipe Contreras Cc: Guzman Lugo, Fernando; linux-omap; Liu, Stanley; Ramirez Luna, Omar Subject: RE: [PATCH 02/14] dspbridge: deh: trivial cleanups The safe thing to do is disable the timer after the DSP has completed the dump. During the dump processing, the DSP will clear the timer interrupt. I don't know what would happen if the timer were disabled prior to that. Best Regards, Cris -Original Message- From: Felipe Contreras [mailto:felipe.contre...@gmail.com] Sent: Monday, May 17, 2010 4:14 PM To: Jansson, Cris Cc: Guzman Lugo, Fernando; linux-omap; Liu, Stanley; Ramirez Luna, Omar Subject: Re: [PATCH 02/14] dspbridge: deh: trivial cleanups Hi Cris, On Mon, May 17, 2010 at 10:39 PM, Jansson, Cris wrote: > Thanks for forwarding the thread. > > In wait_for_timer(), should return be used inside the loop? It will result > in omap_dm_timer_disable(timer) not being called. I believe it should be > called in all outcomes. Yes, that's a mistake I already acknowledged. The question is: can we disable the timer _before_ the DSP has finished dumping the stack? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/14] dspbridge: deh: trivial cleanups
The safe thing to do is disable the timer after the DSP has completed the dump. During the dump processing, the DSP will clear the timer interrupt. I don't know what would happen if the timer were disabled prior to that. Best Regards, Cris -Original Message- From: Felipe Contreras [mailto:felipe.contre...@gmail.com] Sent: Monday, May 17, 2010 4:14 PM To: Jansson, Cris Cc: Guzman Lugo, Fernando; linux-omap; Liu, Stanley; Ramirez Luna, Omar Subject: Re: [PATCH 02/14] dspbridge: deh: trivial cleanups Hi Cris, On Mon, May 17, 2010 at 10:39 PM, Jansson, Cris wrote: > Thanks for forwarding the thread. > > In wait_for_timer(), should return be used inside the loop? It will result > in omap_dm_timer_disable(timer) not being called. I believe it should be > called in all outcomes. Yes, that's a mistake I already acknowledged. The question is: can we disable the timer _before_ the DSP has finished dumping the stack? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] dspbridge: deh: trivial cleanups
Hi Cris, On Mon, May 17, 2010 at 10:39 PM, Jansson, Cris wrote: > Thanks for forwarding the thread. > > In wait_for_timer(), should return be used inside the loop? It will result > in omap_dm_timer_disable(timer) not being called. I believe it should be > called in all outcomes. Yes, that's a mistake I already acknowledged. The question is: can we disable the timer _before_ the DSP has finished dumping the stack? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/14] dspbridge: deh: trivial cleanups
Fernando,
Thanks for forwarding the thread.
In wait_for_timer(), should return be used inside the loop? It will result in
omap_dm_timer_disable(timer) not being called. I believe it should be called
in all outcomes.
> + for (c = 0; c < GPTIMER_IRQ_WAIT_MAX_CNT; c++)
> + if ((omap_dm_timer_read_status(timer) & GPTIMER_IRQ_OVERFLOW))
> + return;
> +
> + pr_err("%s: GPTimer interrupt failed\n", __func__);
>
> omap_dm_timer_disable(timer);
> }
Best Regards,
Cris
-Original Message-
From: Guzman Lugo, Fernando
Sent: Monday, May 17, 2010 2:09 PM
To: Felipe Contreras; linux-omap; Liu, Stanley; Jansson, Cris
Cc: Ramirez Luna, Omar
Subject: RE: [PATCH 02/14] dspbridge: deh: trivial cleanups
+Cris
+Stanley
Loop in DSP guys in case they would have something to add.
> -Original Message-
> From: Felipe Contreras [mailto:felipe.contre...@gmail.com]
> Sent: Sunday, May 16, 2010 10:46 AM
> To: linux-omap
> Cc: Ramirez Luna, Omar; Guzman Lugo, Fernando; Felipe Contreras
> Subject: [PATCH 02/14] dspbridge: deh: trivial cleanups
>
> No functional changes.
>
> Signed-off-by: Felipe Contreras
> ---
> drivers/dsp/bridge/core/ue_deh.c | 112 ++---
> -
> 1 files changed, 41 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/dsp/bridge/core/ue_deh.c
> b/drivers/dsp/bridge/core/ue_deh.c
> index 48c11e9..605cec7 100644
> --- a/drivers/dsp/bridge/core/ue_deh.c
> +++ b/drivers/dsp/bridge/core/ue_deh.c
> @@ -60,11 +60,6 @@
> /* Max time to check for GP Timer IRQ */
> #define GPTIMER_IRQ_WAIT_MAX_CNT 1000
>
> -static struct hw_mmu_map_attrs_t map_attrs = { HW_LITTLE_ENDIAN,
> - HW_ELEM_SIZE16BIT,
> - HW_MMU_CPUES
> -};
> -
> static void *dummy_va_addr;
>
> static struct omap_dm_timer *timer;
> @@ -72,7 +67,7 @@ static struct omap_dm_timer *timer;
> dsp_status bridge_deh_create(struct deh_mgr **ret_deh_mgr,
> struct dev_object *hdev_obj)
> {
> - dsp_status status = DSP_SOK;
> + dsp_status status;
> struct deh_mgr *deh_mgr;
> struct bridge_dev_context *hbridge_context = NULL;
>
> @@ -81,23 +76,20 @@ dsp_status bridge_deh_create(struct deh_mgr
> **ret_deh_mgr,
>* the base image. */
> /* Get WMD context info. */
> dev_get_bridge_context(hdev_obj, &hbridge_context);
> - DBC_ASSERT(hbridge_context);
> - dummy_va_addr = NULL;
> /* Allocate IO manager object: */
> deh_mgr = kzalloc(sizeof(struct deh_mgr), GFP_KERNEL);
> if (!deh_mgr) {
> status = -ENOMEM;
> - goto leave;
> + goto err;
> }
>
> /* Create an NTFY object to manage notifications */
> deh_mgr->ntfy_obj = kmalloc(sizeof(struct ntfy_object), GFP_KERNEL);
> - if (deh_mgr->ntfy_obj) {
> - ntfy_init(deh_mgr->ntfy_obj);
> - } else {
> + if (!deh_mgr->ntfy_obj) {
> status = -ENOMEM;
> goto err;
> }
> + ntfy_init(deh_mgr->ntfy_obj);
>
> /* Create a MMUfault DPC */
> tasklet_init(&deh_mgr->dpc_tasklet, mmu_fault_dpc, (u32) deh_mgr);
> @@ -110,32 +102,25 @@ dsp_status bridge_deh_create(struct deh_mgr
> **ret_deh_mgr,
> deh_mgr->err_info.dw_val3 = 0L;
>
> /* Install ISR function for DSP MMU fault */
> - if ((request_irq(INT_DSP_MMU_IRQ, mmu_fault_isr, 0,
> - "DspBridge\tiommu fault",
> - (void *)deh_mgr)) == 0)
> - status = DSP_SOK;
> - else
> - status = -EPERM;
> + status = request_irq(INT_DSP_MMU_IRQ, mmu_fault_isr, 0,
> + "DspBridge\tiommu fault", deh_mgr);
> + if (status < 0)
> + goto err;
>
> -err:
> - if (DSP_FAILED(status)) {
> - /* If create failed, cleanup */
> - bridge_deh_destroy(deh_mgr);
> - deh_mgr = NULL;
> - } else {
> - timer = omap_dm_timer_request_specific(
> - GPTIMER_FOR_DSP_MMU_FAULT);
> - if (timer) {
> - omap_dm_timer_disable(timer);
> - } else {
> - pr_err("%s: GPTimer not available\n", __func__);
> - return -ENODEV;
> - }
> + timer = omap_dm_timer_request_specific(GPTIMER_FOR_DSP_MMU_FAULT);
> + if (!timer) {
> + pr_err("%s: GPTimer not available\n", __func__);
> + status = -ENODEV;
> + goto err
RE: [PATCH 02/14] dspbridge: deh: trivial cleanups
+Cris
+Stanley
Loop in DSP guys in case they would have something to add.
> -Original Message-
> From: Felipe Contreras [mailto:felipe.contre...@gmail.com]
> Sent: Sunday, May 16, 2010 10:46 AM
> To: linux-omap
> Cc: Ramirez Luna, Omar; Guzman Lugo, Fernando; Felipe Contreras
> Subject: [PATCH 02/14] dspbridge: deh: trivial cleanups
>
> No functional changes.
>
> Signed-off-by: Felipe Contreras
> ---
> drivers/dsp/bridge/core/ue_deh.c | 112 ++---
> -
> 1 files changed, 41 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/dsp/bridge/core/ue_deh.c
> b/drivers/dsp/bridge/core/ue_deh.c
> index 48c11e9..605cec7 100644
> --- a/drivers/dsp/bridge/core/ue_deh.c
> +++ b/drivers/dsp/bridge/core/ue_deh.c
> @@ -60,11 +60,6 @@
> /* Max time to check for GP Timer IRQ */
> #define GPTIMER_IRQ_WAIT_MAX_CNT 1000
>
> -static struct hw_mmu_map_attrs_t map_attrs = { HW_LITTLE_ENDIAN,
> - HW_ELEM_SIZE16BIT,
> - HW_MMU_CPUES
> -};
> -
> static void *dummy_va_addr;
>
> static struct omap_dm_timer *timer;
> @@ -72,7 +67,7 @@ static struct omap_dm_timer *timer;
> dsp_status bridge_deh_create(struct deh_mgr **ret_deh_mgr,
> struct dev_object *hdev_obj)
> {
> - dsp_status status = DSP_SOK;
> + dsp_status status;
> struct deh_mgr *deh_mgr;
> struct bridge_dev_context *hbridge_context = NULL;
>
> @@ -81,23 +76,20 @@ dsp_status bridge_deh_create(struct deh_mgr
> **ret_deh_mgr,
>* the base image. */
> /* Get WMD context info. */
> dev_get_bridge_context(hdev_obj, &hbridge_context);
> - DBC_ASSERT(hbridge_context);
> - dummy_va_addr = NULL;
> /* Allocate IO manager object: */
> deh_mgr = kzalloc(sizeof(struct deh_mgr), GFP_KERNEL);
> if (!deh_mgr) {
> status = -ENOMEM;
> - goto leave;
> + goto err;
> }
>
> /* Create an NTFY object to manage notifications */
> deh_mgr->ntfy_obj = kmalloc(sizeof(struct ntfy_object), GFP_KERNEL);
> - if (deh_mgr->ntfy_obj) {
> - ntfy_init(deh_mgr->ntfy_obj);
> - } else {
> + if (!deh_mgr->ntfy_obj) {
> status = -ENOMEM;
> goto err;
> }
> + ntfy_init(deh_mgr->ntfy_obj);
>
> /* Create a MMUfault DPC */
> tasklet_init(&deh_mgr->dpc_tasklet, mmu_fault_dpc, (u32) deh_mgr);
> @@ -110,32 +102,25 @@ dsp_status bridge_deh_create(struct deh_mgr
> **ret_deh_mgr,
> deh_mgr->err_info.dw_val3 = 0L;
>
> /* Install ISR function for DSP MMU fault */
> - if ((request_irq(INT_DSP_MMU_IRQ, mmu_fault_isr, 0,
> - "DspBridge\tiommu fault",
> - (void *)deh_mgr)) == 0)
> - status = DSP_SOK;
> - else
> - status = -EPERM;
> + status = request_irq(INT_DSP_MMU_IRQ, mmu_fault_isr, 0,
> + "DspBridge\tiommu fault", deh_mgr);
> + if (status < 0)
> + goto err;
>
> -err:
> - if (DSP_FAILED(status)) {
> - /* If create failed, cleanup */
> - bridge_deh_destroy(deh_mgr);
> - deh_mgr = NULL;
> - } else {
> - timer = omap_dm_timer_request_specific(
> - GPTIMER_FOR_DSP_MMU_FAULT);
> - if (timer) {
> - omap_dm_timer_disable(timer);
> - } else {
> - pr_err("%s: GPTimer not available\n", __func__);
> - return -ENODEV;
> - }
> + timer = omap_dm_timer_request_specific(GPTIMER_FOR_DSP_MMU_FAULT);
> + if (!timer) {
> + pr_err("%s: GPTimer not available\n", __func__);
> + status = -ENODEV;
> + goto err;
> }
> + omap_dm_timer_disable(timer);
>
> -leave:
> *ret_deh_mgr = deh_mgr;
> + return 0;
>
> +err:
> + bridge_deh_destroy(deh_mgr);
> + *ret_deh_mgr = NULL;
> return status;
> }
>
> @@ -164,36 +149,31 @@ dsp_status bridge_deh_destroy(struct deh_mgr
> *deh_mgr)
> omap_dm_timer_free(timer);
> timer = NULL;
>
> - return DSP_SOK;
> + return 0;
> }
>
> dsp_status bridge_deh_register_notify(struct deh_mgr *deh_mgr, u32
> event_mask,
> u32 notify_type,
> struct dsp_notification *hnotification)
> {
> - dsp_status status = DSP_SOK;
> -
> if (!deh_mgr)
> return -EFAULT;
>
> if (event_mask)
> - status = ntfy_register(deh_mgr->ntfy_obj, hnotification,
> - event_mask, notify_type);
> + return ntfy_register(deh_mgr->ntfy_obj, hnotification,
> + event_mask, notify_type);
> else
> - status = ntfy_unregister(deh_mgr->ntfy_obj, hnotification);
> -
> - return status;
> + return ntfy_unregister(deh_mgr->ntfy_obj, hnotification);
> }
>
> static void wait_for_timer(
RE: [PATCH 02/14] dspbridge: deh: trivial cleanups
Hi,
> -Original Message-
> From: Felipe Contreras [mailto:felipe.contre...@gmail.com]
> Sent: Sunday, May 16, 2010 5:13 PM
> To: Guzman Lugo, Fernando
> Cc: linux-omap; Ramirez Luna, Omar
> Subject: Re: [PATCH 02/14] dspbridge: deh: trivial cleanups
>
> On Mon, May 17, 2010 at 1:02 AM, Guzman Lugo, Fernando
> wrote:
>
> >> + for (c = 0; c < GPTIMER_IRQ_WAIT_MAX_CNT; c++)
> >> + if ((omap_dm_timer_read_status(timer) &
> GPTIMER_IRQ_OVERFLOW))
> >> + return;
> >> +
> >> + pr_err("%s: GPTimer interrupt failed\n", __func__);
> >
> > In case of not timeout the timer is not disabled, that could prevent
> power transition to off. It should be disable as soon as it is not needed
> which is after calling dump_dsp_stack.
>
> Right, I originally had timer_disable() outside of wait_for_timeout(),
> and only later decided to put it in. However, do we still need to let
> the timer on? Can't we disable the timer always before dumping the
> stack? The rest of the code seems to run fine with the timer disabled.
If there is no f/i clock the DSP cannot ack the interruption, maybe reading the
register without the clocks not cause a fatal in the DSP. But you will have
issues if the ack is not done.
Regards,
Fernando.
>
> >> omap_dm_timer_disable(timer);
> >> }
>
> Cheers.
>
> --
> Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/14] dspbridge: deh: trivial cleanups
On Mon, May 17, 2010 at 1:02 AM, Guzman Lugo, Fernando
wrote:
>> + for (c = 0; c < GPTIMER_IRQ_WAIT_MAX_CNT; c++)
>> + if ((omap_dm_timer_read_status(timer) & GPTIMER_IRQ_OVERFLOW))
>> + return;
>> +
>> + pr_err("%s: GPTimer interrupt failed\n", __func__);
>
> In case of not timeout the timer is not disabled, that could prevent power
> transition to off. It should be disable as soon as it is not needed which is
> after calling dump_dsp_stack.
Right, I originally had timer_disable() outside of wait_for_timeout(),
and only later decided to put it in. However, do we still need to let
the timer on? Can't we disable the timer always before dumping the
stack? The rest of the code seems to run fine with the timer disabled.
>> omap_dm_timer_disable(timer);
>> }
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 02/14] dspbridge: deh: trivial cleanups
Hi,
> -Original Message-
> From: Felipe Contreras [mailto:felipe.contre...@gmail.com]
> Sent: Sunday, May 16, 2010 10:46 AM
> To: linux-omap
> Cc: Ramirez Luna, Omar; Guzman Lugo, Fernando; Felipe Contreras
> Subject: [PATCH 02/14] dspbridge: deh: trivial cleanups
>
> No functional changes.
>
> Signed-off-by: Felipe Contreras
> ---
> drivers/dsp/bridge/core/ue_deh.c | 112 ++---
> -
> 1 files changed, 41 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/dsp/bridge/core/ue_deh.c
> b/drivers/dsp/bridge/core/ue_deh.c
> index 48c11e9..605cec7 100644
> --- a/drivers/dsp/bridge/core/ue_deh.c
> +++ b/drivers/dsp/bridge/core/ue_deh.c
> @@ -60,11 +60,6 @@
> /* Max time to check for GP Timer IRQ */
> #define GPTIMER_IRQ_WAIT_MAX_CNT 1000
>
> -static struct hw_mmu_map_attrs_t map_attrs = { HW_LITTLE_ENDIAN,
> - HW_ELEM_SIZE16BIT,
> - HW_MMU_CPUES
> -};
> -
> static void *dummy_va_addr;
>
> static struct omap_dm_timer *timer;
> @@ -72,7 +67,7 @@ static struct omap_dm_timer *timer;
> dsp_status bridge_deh_create(struct deh_mgr **ret_deh_mgr,
> struct dev_object *hdev_obj)
> {
> - dsp_status status = DSP_SOK;
> + dsp_status status;
> struct deh_mgr *deh_mgr;
> struct bridge_dev_context *hbridge_context = NULL;
>
> @@ -81,23 +76,20 @@ dsp_status bridge_deh_create(struct deh_mgr
> **ret_deh_mgr,
>* the base image. */
> /* Get WMD context info. */
> dev_get_bridge_context(hdev_obj, &hbridge_context);
> - DBC_ASSERT(hbridge_context);
> - dummy_va_addr = NULL;
> /* Allocate IO manager object: */
> deh_mgr = kzalloc(sizeof(struct deh_mgr), GFP_KERNEL);
> if (!deh_mgr) {
> status = -ENOMEM;
> - goto leave;
> + goto err;
> }
>
> /* Create an NTFY object to manage notifications */
> deh_mgr->ntfy_obj = kmalloc(sizeof(struct ntfy_object), GFP_KERNEL);
> - if (deh_mgr->ntfy_obj) {
> - ntfy_init(deh_mgr->ntfy_obj);
> - } else {
> + if (!deh_mgr->ntfy_obj) {
> status = -ENOMEM;
> goto err;
> }
> + ntfy_init(deh_mgr->ntfy_obj);
>
> /* Create a MMUfault DPC */
> tasklet_init(&deh_mgr->dpc_tasklet, mmu_fault_dpc, (u32) deh_mgr);
> @@ -110,32 +102,25 @@ dsp_status bridge_deh_create(struct deh_mgr
> **ret_deh_mgr,
> deh_mgr->err_info.dw_val3 = 0L;
>
> /* Install ISR function for DSP MMU fault */
> - if ((request_irq(INT_DSP_MMU_IRQ, mmu_fault_isr, 0,
> - "DspBridge\tiommu fault",
> - (void *)deh_mgr)) == 0)
> - status = DSP_SOK;
> - else
> - status = -EPERM;
> + status = request_irq(INT_DSP_MMU_IRQ, mmu_fault_isr, 0,
> + "DspBridge\tiommu fault", deh_mgr);
> + if (status < 0)
> + goto err;
>
> -err:
> - if (DSP_FAILED(status)) {
> - /* If create failed, cleanup */
> - bridge_deh_destroy(deh_mgr);
> - deh_mgr = NULL;
> - } else {
> - timer = omap_dm_timer_request_specific(
> - GPTIMER_FOR_DSP_MMU_FAULT);
> - if (timer) {
> - omap_dm_timer_disable(timer);
> - } else {
> - pr_err("%s: GPTimer not available\n", __func__);
> - return -ENODEV;
> - }
> + timer = omap_dm_timer_request_specific(GPTIMER_FOR_DSP_MMU_FAULT);
> + if (!timer) {
> + pr_err("%s: GPTimer not available\n", __func__);
> + status = -ENODEV;
> + goto err;
> }
> + omap_dm_timer_disable(timer);
>
> -leave:
> *ret_deh_mgr = deh_mgr;
> + return 0;
>
> +err:
> + bridge_deh_destroy(deh_mgr);
> + *ret_deh_mgr = NULL;
> return status;
> }
>
> @@ -164,36 +149,31 @@ dsp_status bridge_deh_destroy(struct deh_mgr
> *deh_mgr)
> omap_dm_timer_free(timer);
> timer = NULL;
>
> - return DSP_SOK;
> + return 0;
> }
>
> dsp_status bridge_deh_register_notify(struct deh_mgr *deh_mgr, u32
> event_mask,
> u32 notify_type,
> struct dsp_notification *hnotification)
> {
> - dsp_status status = DSP_SOK;
> -
> if (!deh_mgr)
> return -EFAULT;
>
> if (event_mask)
> - status = ntfy_register(deh_mgr->ntfy_obj, hnotification,
> - event_mask, notify_type);
> + return ntfy_register(deh_mgr->ntfy_obj, hnotification,
> + event_mask, notify_type);
> else
> - status = ntfy_unregister(deh_mgr->ntfy_obj, hnotification);
> -
> - return status;
> + return ntfy_unregister(deh_mgr->ntfy_obj, hnotification);
> }
>
> static void wait_for_timer(void)
> {
> - u32 cnt = 0;
> + int c;
>
> omap_dm_time
