Prev: staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c
Next: staging:ti dspbridge: replace simple_strtoul by strict_strtoul
From: Nishanth Menon on 28 Jul 2010 11:10
Ramos Falcon, Ernesto had written, on 07/28/2010 09:40 AM, the following:
> Remove unnecessary volatile variables; use accessor
> functions __raw_readl/__raw_writel instead when applicable.
>
> Signed-off-by: Ernesto Ramos <ernesto(a)ti.com>
> ---
> drivers/staging/tidspbridge/core/tiomap3430.c | 8 ++++----
> drivers/staging/tidspbridge/dynload/tramp.c | 4 ++--
> drivers/staging/tidspbridge/pmgr/cmm.c | 7 ++++---
> 3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/staging/tidspbridge/core/tiomap3430.c b/drivers/staging/tidspbridge/core/tiomap3430.c
> index 08a2f5f..ae1f394 100644
> --- a/drivers/staging/tidspbridge/core/tiomap3430.c
> +++ b/drivers/staging/tidspbridge/core/tiomap3430.c
> @@ -404,7 +404,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
> pr_err("%s: Illegal SM base\n", __func__);
> status = -EPERM;
> } else
> - *((volatile u32 *)dw_sync_addr) = 0xffffffff;
> + __raw_writel(0xffffffff, dw_sync_addr);
curious question: any reason not to use writel instead of __raw_writel?

>
> if (DSP_SUCCEEDED(status)) {
> resources = dev_context->resources;
> @@ -584,7 +584,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
> dev_dbg(bridge, "Waiting for Sync @ 0x%x\n", dw_sync_addr);
> dev_dbg(bridge, "DSP c_int00 Address = 0x%x\n", dsp_addr);
> if (dsp_debug)
> - while (*((volatile u16 *)dw_sync_addr))
> + while (__raw_readw(dw_sync_addr))
> ;;

may be for a later on patch -> we seem to have a potential infinite loop
here..

>
> /* Wait for DSP to clear word in shared memory */
> @@ -602,7 +602,7 @@ static int bridge_brd_start(struct bridge_dev_context *dev_ctxt,
> /* Write the synchronization bit to indicate the
> * completion of OPP table update to DSP
> */
> - *((volatile u32 *)dw_sync_addr) = 0XCAFECAFE;
> + __raw_writel(0XCAFECAFE, dw_sync_addr);
>
> /* update board state */
> dev_context->dw_brd_state = BRD_RUNNING;
> @@ -1852,7 +1852,7 @@ bool wait_for_start(struct bridge_dev_context *dev_context, u32 dw_sync_addr)
> u16 timeout = TIHELEN_ACKTIMEOUT;
>
> /* Wait for response from board */
> - while (*((volatile u16 *)dw_sync_addr) && --timeout)
> + while (__raw_readw(dw_sync_addr) && --timeout)
> udelay(10);
>
> /* If timed out: return false */
> diff --git a/drivers/staging/tidspbridge/dynload/tramp.c b/drivers/staging/tidspbridge/dynload/tramp.c
> index 81314d2..60d22ea 100644
> --- a/drivers/staging/tidspbridge/dynload/tramp.c
> +++ b/drivers/staging/tidspbridge/dynload/tramp.c
> @@ -86,8 +86,8 @@ static u8 priv_h2a(u8 value)
> static void priv_tramp_sym_gen_name(u32 value, char *dst)
> {
> u32 i;
> - volatile char *prefix = TRAMP_SYM_PREFIX;
> - volatile char *dst_local = dst;
> + char *prefix = TRAMP_SYM_PREFIX;
> + char *dst_local = dst;
> u8 tmp;
>
> /* Clear out the destination, including the ending NULL */
> diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c b/drivers/staging/tidspbridge/pmgr/cmm.c
> index 874ed64..b7cba1b 100644
> --- a/drivers/staging/tidspbridge/pmgr/cmm.c
> +++ b/drivers/staging/tidspbridge/pmgr/cmm.c
> @@ -1008,6 +1008,7 @@ void *cmm_xlator_alloc_buf(struct cmm_xlatorobject *xlator, void *va_buf,
> {
> struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
> void *pbuf = NULL;
> + void *tmp_va_buff;
> struct cmm_attrs attrs;
>
> DBC_REQUIRE(refs > 0);
> @@ -1019,16 +1020,16 @@ void *cmm_xlator_alloc_buf(struct cmm_xlatorobject *xlator, void *va_buf,
>
> if (xlator_obj) {
> attrs.ul_seg_id = xlator_obj->ul_seg_id;
> - *(volatile u32 *)va_buf = 0;
> + __raw_writel(0, va_buf);
> /* Alloc SM */
> pbuf =
> cmm_calloc_buf(xlator_obj->hcmm_mgr, pa_size, &attrs, NULL);
> if (pbuf) {
> /* convert to translator(node/strm) process Virtual
> * address */
> - *(volatile u32 **)va_buf =
> - (u32 *) cmm_xlator_translate(xlator,
> + tmp_va_buff = cmm_xlator_translate(xlator,
> pbuf, CMM_PA2VA);
> + __raw_writel((u32)tmp_va_buff, va_buf);

a) is the u32 cast of tmp_va_buff really necessary?
b) tmp_va_buff is used only in this branch, why not reduce the scope of
the variable to just to this if branch
or an optional way could be:
--- a/drivers/staging/tidspbridge/pmgr/cmm.c
+++ b/drivers/staging/tidspbridge/pmgr/cmm.c
@@ -1022,18 +1022,17 @@ void *cmm_xlator_alloc_buf(struct
cmm_xlatorobject *xlator, void *va_buf,
DBC_REQUIRE(xlator_obj->ul_seg_id > 0);

if (xlator_obj) {
+ void *t_buf = 0;
attrs.ul_seg_id = xlator_obj->ul_seg_id;
- *(volatile u32 *)va_buf = 0;
/* Alloc SM */
pbuf =
cmm_calloc_buf(xlator_obj->hcmm_mgr, pa_size, &attrs, NULL);
if (pbuf) {
/* convert to translator(node/strm) process Virtual
* address */
- *(volatile u32 **)va_buf =
- (u32 *) cmm_xlator_translate(xlator,
- pbuf, CMM_PA2VA);
+ t_buf = cmm_xlator_translate(xlator, pbuf, CMM_PA2VA);
}
+ writel(t_buf, va_buf);
}
return pbuf;
}


> }
> }
> return pbuf;


--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ramos Falcon, Ernesto on 2 Aug 2010 15:50
Hi,

>-----Original Message-----
>From: Menon, Nishanth
>Sent: Wednesday, July 28, 2010 10:04 AM
>To: Ramos Falcon, Ernesto
>Cc: gregkh(a)suse.de; Ramirez Luna, Omar; ohad(a)wizery.com;
>ameya.palande(a)nokia.com; felipe.contreras(a)nokia.com; Guzman Lugo, Fernando;
>linux-kernel(a)vger.kernel.org; andy.shevchenko(a)gmail.com; linux-
>omap(a)vger.kernel.org
>Subject: Re: [PATCH 4/5] staging:ti dspbridge: remove unnecessary volatile
>variables

<snip>

>> index 08a2f5f..ae1f394 100644
>> --- a/drivers/staging/tidspbridge/core/tiomap3430.c
>> +++ b/drivers/staging/tidspbridge/core/tiomap3430.c
>> @@ -404,7 +404,7 @@ static int bridge_brd_start(struct bridge_dev_context
>*dev_ctxt,
>> pr_err("%s: Illegal SM base\n", __func__);
>> status = -EPERM;
>> } else
>> - *((volatile u32 *)dw_sync_addr) = 0xffffffff;
>> + __raw_writel(0xffffffff, dw_sync_addr);
>curious question: any reason not to use writel instead of __raw_writel?
>
I was not aware of the existence of writel, but I think I should use writel instead.

>>
>> if (DSP_SUCCEEDED(status)) {
>> resources = dev_context->resources;
>> @@ -584,7 +584,7 @@ static int bridge_brd_start(struct bridge_dev_context
>*dev_ctxt,
>> dev_dbg(bridge, "Waiting for Sync @ 0x%x\n", dw_sync_addr);
>> dev_dbg(bridge, "DSP c_int00 Address = 0x%x\n", dsp_addr);
>> if (dsp_debug)
>> - while (*((volatile u16 *)dw_sync_addr))
>> + while (__raw_readw(dw_sync_addr))
>> ;;
>
>may be for a later on patch -> we seem to have a potential infinite loop
>here..
>
This is done intentionally and it is used to debug the dsp. The system will hang here, and then Lautherback or jtag can be connected to take control of the program execution from this point.

>> diff --git a/drivers/staging/tidspbridge/pmgr/cmm.c
>b/drivers/staging/tidspbridge/pmgr/cmm.c
>> index 874ed64..b7cba1b 100644
>> --- a/drivers/staging/tidspbridge/pmgr/cmm.c
>> +++ b/drivers/staging/tidspbridge/pmgr/cmm.c
>> @@ -1008,6 +1008,7 @@ void *cmm_xlator_alloc_buf(struct cmm_xlatorobject
>*xlator, void *va_buf,
>> {
>> struct cmm_xlator *xlator_obj = (struct cmm_xlator *)xlator;
>> void *pbuf = NULL;
>> + void *tmp_va_buff;
>> struct cmm_attrs attrs;
>>
>> DBC_REQUIRE(refs > 0);
>> @@ -1019,16 +1020,16 @@ void *cmm_xlator_alloc_buf(struct
>cmm_xlatorobject *xlator, void *va_buf,
>>
>> if (xlator_obj) {
>> attrs.ul_seg_id = xlator_obj->ul_seg_id;
>> - *(volatile u32 *)va_buf = 0;
>> + __raw_writel(0, va_buf);
>> /* Alloc SM */
>> pbuf =
>> cmm_calloc_buf(xlator_obj->hcmm_mgr, pa_size, &attrs,
>NULL);
>> if (pbuf) {
>> /* convert to translator(node/strm) process Virtual
>> * address */
>> - *(volatile u32 **)va_buf =
>> - (u32 *) cmm_xlator_translate(xlator,
>> + tmp_va_buff = cmm_xlator_translate(xlator,
>> pbuf, CMM_PA2VA);
>> + __raw_writel((u32)tmp_va_buff, va_buf);
>
>a) is the u32 cast of tmp_va_buff really necessary?
>b) tmp_va_buff is used only in this branch, why not reduce the scope of
>the variable to just to this if branch

Agree!


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 | 
Pages: 1
Prev: staging:ti dspbridge: remove unnecessary check for NULL pointer in cmm.c
Next: staging:ti dspbridge: replace simple_strtoul by strict_strtoul