Re: [PATCH 05/12] staging:tidspbridge - set5 remove hungarian from structs
Hi Dan, On Wed, Dec 15, 2010 at 2:53 AM, Dan Carpenter erro...@gmail.com wrote: On Tue, Dec 14, 2010 at 02:33:11PM -0600, Rene Sapiens wrote: - pfn_unload(pnode-nldr_node_obj, + load(pnode-nldr_node_obj, NLDR_EXECUTE); Unload got swapped with load here. This might have been easier to with Coccinelle instead of by hand... I'm sorry for the delay, i did use some scripts to identify/replace the words, this last one should be because of my lasts coding style fixes after changes, i will fix it and send the patch again. Indeed Coccinelle is a very good option too. -- Regards, Rene Sapiens -- 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: DSP Bridge video decode of above VGA videos
Hi James, On Tue, Nov 23, 2010 at 8:34 AM, James Adams james.r.ad...@gmail.com wrote: A bit more information about why videos are not playing in a port of Zoom2 to gumstix: 2) If I use the OpenCORE unit tests I can successfully decode: VGA movies with software codec 1280*544 movie with software codec VGA movies with DSP codec but it fails for the 1280x544 movie with DSP codec. This is the error log: pvplayer_engine_test -test 1 1 -source wall720p.mp4 SDK Labeled: PVDEV_CORE_RELEASE_6.506.4.1 built on 20090312 Test Program for pvPlayer engine class. Input file name 'wall720p.mp4' Test case range 1 to 1 Compressed output Video(No) Audio(No) Log level 8; Log node 0 Log Text 0 Log Mem 0 Starting Test 1: Open-Play-Stop-Reset LCML ERROR : DSP Error: Create the Node : Err Num = 8000800c LCML ERROR : DSP It looks that your system is getting ride of memory and it is failing in the creation stage of a DSP node. I suggest you to free all the not needed resources before trying to decode the 1280x544 movie with the DSP decoders, There are a couple of minor allocations done during the node create stage. To debug it you can take a look to the bridge code: drivers/dsp/bridge/ Basically communication with the DSP is handled by such driver[1]. The error should be coming from drivers/dsp/bridge/rmgr/node.c in the node_create() function. 3) I can't find this 8000800c number defined anywhere in the Android tree. Any idea what it means? For this version of the bridge driver 0x8000800c should mean a DSP_EMEMORY error, find it in [2]. You can take a look in the driver mentioned above for the place where you are getting that error. Functions where you can get that error in the node's create phase are: 1) drivers/dsp/bridge/rmgr/Nldr.c LoadLib() 2) drivers/dsp/bridge/pmgr/Dbll.c DBLL_open() 3) drivers/dsp/bridge/rmgr/Dbdcd.c GetDepLibInfo() 4) drivers/dsp/bridge/pmgr/Dbll.c DBLL_load() 5) drivers/dsp/bridge/rmgr/rmm.c RMM_alloc() - When calling to allocBlock() - When allocate list element for new section. 4) Using the software codec from inside the Android Gallery application I still get the same errors as mentioned before about missing video buffer addresses. In general, OpenCore/Omx question should go to a different list, since it is not kernel specific, i would suggest you to contact your vendor to point you to the correct support team. Regards, Rene Sapiens -- [1] http://omapzoom.org/wiki/DSPBridge_Project#Documents [2] http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=arch/arm/plat-omap/include/dspbridge/errbase.h;h=f04c005c06adb253a3b2f8886ba38e4475b578b3;hb=refs/heads/p-android-omap-2.6.29-eclair#l130 -- 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: DSP Bridge video decode of above VGA videos
Hi James, The second error log is the stack trace when the watchdog timer calls SYNC_EnterCS. Actually is not the WDT calling the function to get into the critical section, it is the WDT being disabled when the DSP is going to OFF state. This should fine. The stack trace is shown below. 1) Is this trace printed after the 'if (in_interrupt())' statement? 2) Can you check under drivers/dsp/bridge/ to see where is or under which context io_dispatchpm called? To get the assertions failing you should be in the context of an interrupt, the code base that i have is not that way for that reason i ask for your io_dispatchpm function. [c0412864] (dump_stack+0x0/0x14) from [c0278c18] (SYNC_EnterCS+0x48/0xe4) [c0278bd0] (SYNC_EnterCS+0x0/0xe4) from [c0279e24] (REG_GetValue+0x60/0xa0) r5:c05ba390 r4:c04f092c [c0279dc4] (REG_GetValue+0x0/0xa0) from [c027987c] (CFG_GetObject+0x60/0xe0) r7:cfa87a80 r6:0002 r5:cf98fe94 r4:0002 [c027981c] (CFG_GetObject+0x0/0xe0) from [c02913c0] (DRV_GetFirstDevObject+0 x1c/0x4c) r5:c05ba3c0 r4: [c02913a4] (DRV_GetFirstDevObject+0x0/0x4c) from [c02858e4] (DEV_GetFirst+0x 10/0x44) [c02858d4] (DEV_GetFirst+0x0/0x44) from [c027d430] (dsp_wdt_enable+0x44/0xe0 ) r5:c05ba3c0 r4: [c027d3ec] (dsp_wdt_enable+0x0/0xe0) from [c02820fc] (handle_hibernation_fro mDSP+0x148/0x1ec) r5:cfaa r4:8000 [c0281fb4] (handle_hibernation_fromDSP+0x0/0x1ec) from [c02810c0] (WMD_DEV_C trl+0xfc/0x130) r7:cf98e000 r6:cf98ff34 r5:cc5f6a88 r4:cfb89e00 [c0280fc4] (WMD_DEV_Ctrl+0x0/0x130) from [c027dc24] (io_mbox_msg+0x70/0x1d8) r7:cf98e000 r6:c05496c4 r5:cc5f6a88 r4:cfb89e00 [c027dbb4] (io_mbox_msg+0x0/0x1d8) from [c0059308] (mbox_rx_work+0xf4/0x100) r5:cc5f6a88 r4:200a [c0059214] (mbox_rx_work+0x0/0x100) from [c007ebb0] (run_workqueue+0xc8/0x18 4) [c007eae8] (run_workqueue+0x0/0x184) from [c007f2dc] (worker_thread+0x104/0x 118) r9: r8: r7:cf811040 r6:cf94e140 r5:cf94e148 r4:cf98e000 [c007f1d8] (worker_thread+0x0/0x118) from [c0082cb4] (kthread+0x54/0x80) r7: r6: r5:c007f1d8 r4:cf94e140 [c0082c60] (kthread+0x0/0x80) from [c0070b1c] (do_exit+0x0/0x7bc) r5: r4: drivers/dsp/bridge/services/sync.c, line 358: Assertion (0) failed. Regards, Rene Sapiens -- 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: DSP Bridge video decode of above VGA videos
Hi James, On Mon, Nov 22, 2010 at 9:26 AM, James Adams james.r.ad...@gmail.com wrote: D/TI_Video_Decoder( 934): VIDDEC_InitDSP_H264Dec():6573 After Rounding: nFrameW idth = 1280, nFrameHeight = 544 drivers/dsp/bridge/services/sync.c, line 357: Assertion (0) failed. drivers/dsp/bridge/services/sync.c, line 357: Assertion (0) failed. Does the version of your sync.c file points to sync_entercs() and the the assert failed for in_interrupt() for the line 357? Do your dspbridge version uses the WDT implementation? D/TI_Video_Decoder( 934): VIDDEC_InitDSP_H264Dec():6619 LCML_InitMMCodec Failed !...80001009 D/TI_Video_Decoder( 934): VIDDEC_HandleCommand():1950 LCML Error 1 D/TI_Video_Decoder( 934): OMX_VidDec_Thread():165 Error in Select Regards, Rene Sapiens -- 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: DSP Bridge video decode of above VGA videos
James, On Mon, Nov 22, 2010 at 11:12 AM, James Adams james.r.ad...@gmail.com wrote: Hi Rene, We have got the watchdog timer enabled at the moment (interval 5 seconds). The code does indeed point to the line as you say. I tried using dump_stack and it was getting triggered by an interrupt calling handle_hibernation_fromDSP which then called functions eventually triggering the SYNC_entercs (via DEV_getfirst if I remember right) I certainly don't understand why this should happen, but it seems to happen quite a lot (probably every 5 seconds...) and the lower resolution videos didn't seem to mind so I have been ignoring it. Looking at the drivers/dsp/bridge/wmd/tiomap3430_pwr.c file tagged with L25.12 release at [1] I don't see feasible the path to have handle_hibernation_fromDSP() to get to SYNC_entercs(), so probably you have a newer or older version of the file. Basically what i was looking with those assertions is to see if those could be because of a DSP WDT overflow. Also those assertions can fail because of the calling of IO_DispatchMsg by io_dpc(), if that's the case there should not be a problem. On the other hand if there is a WDT overflow those assertions could also fail. Can you double check by printing the call stack again when the assertion fails, just to see why for your case handle_hibernation_fromDSP() is failing in the assertion? Is it a bad idea to use WDT? No, it is ok to use it. Regards, Rene Sapiens -- [1] http://git.omapzoom.org/?p=kernel/omap.git;a=blob;f=drivers/dsp/bridge/wmd/tiomap3430_pwr.c;h=f698b4475584e92467e03ecec96bc887948275f9;hb=64b404e9f457f19537972e7f2424f4c73a8a7789#l116 -- 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 v2 06/12] staging: tidspbridge: convert core to list_head
Hi Ionut,
On Sat, Nov 6, 2010 at 11:21 AM, Ionut Nicu ionut.n...@mindbit.ro wrote:
Hi Rene,
On Fri, 2010-11-05 at 15:07 -0600, Sapiens, Rene wrote:
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the core module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
snip
if (!status) {
/* Get a free chirp: */
- chnl_packet_obj =
- (struct chnl_irp
*)lst_get_head(pchnl-free_packets_list);
- if (chnl_packet_obj == NULL)
+ if (!list_empty(pchnl-free_packets_list)) {
+ chnl_packet_obj = list_first_entry(
+ pchnl-free_packets_list,
+ struct chnl_irp, link);
+ list_del(chnl_packet_obj-link);
+ } else
status = -EIO;
What do you think if we close the braces, since the first conditional
has more than one statement?
Can you clarify? I don't think I understand which brace we need to close
here.
I mean open and close the braces for the added 'else', I'm just mentioning
what the coding style says for placing braces when one of the branches
has more than one statement.
snip
@@ -286,18 +286,16 @@ int bridge_chnl_cancel_io(struct chnl_object
*chnl_obj)
}
}
/* Move all IOR's to IOC queue: */
- while (!LST_IS_EMPTY(pchnl-pio_requests)) {
- chnl_packet_obj =
- (struct chnl_irp *)lst_get_head(pchnl-pio_requests);
- if (chnl_packet_obj) {
- chnl_packet_obj-byte_size = 0;
- chnl_packet_obj-status |= CHNL_IOCSTATCANCEL;
- lst_put_tail(pchnl-pio_completions,
- (struct list_head *)chnl_packet_obj);
- pchnl-cio_cs++;
- pchnl-cio_reqs--;
- DBC_ASSERT(pchnl-cio_reqs = 0);
- }
+ while (!list_empty(pchnl-pio_requests)) {
+ chnl_packet_obj = list_first_entry(pchnl-pio_requests,
+ struct chnl_irp, link);
+ list_del(chnl_packet_obj-link);
+ chnl_packet_obj-byte_size = 0;
+ chnl_packet_obj-status |= CHNL_IOCSTATCANCEL;
+ list_add_tail(chnl_packet_obj-link,
pchnl-pio_completions);
+ pchnl-cio_cs++;
+ pchnl-cio_reqs--;
+ DBC_ASSERT(pchnl-cio_reqs = 0);
Why don't we use list_for_each_entry_safe() instead?
Agreed, it will look better.
}
func_cont:
spin_unlock_bh(chnl_mgr_obj-chnl_mgr_lock);
snip
@@ -818,9 +804,19 @@ int bridge_chnl_open(struct chnl_object **chnl,
/* Protect queues from io_dpc: */
pchnl-dw_state = CHNL_STATECANCEL;
/* Allocate initial IOR and IOC queues: */
- pchnl-free_packets_list = create_chirp_list(pattrs-uio_reqs);
- pchnl-pio_requests = create_chirp_list(0);
- pchnl-pio_completions = create_chirp_list(0);
+ status = create_chirp_list(pchnl-free_packets_list,
+ pattrs-uio_reqs);
+ if (status)
+ goto func_end;
+
+ status = create_chirp_list(pchnl-pio_requests, 0);
+ if (status)
+ goto func_end;
+
+ status = create_chirp_list(pchnl-pio_completions, 0);
+ if (status)
+ goto func_end;
+
With these goto you are not freeing the memory allocated for pchnl, please
free
it at func_end.
Thanks for catching this. Freeing it at func_end is not a very good
idea, because it's also freed above.
What do you think if I replace the last two calls for
create_chirp_list() with just INIT_LIST_HEAD()? This way we'll have only
one error check where we'll also kfree(pchnl) and we'll have 2 less
un-necessary function calls / error checks.
Agreed, this looks good.
Regards,
Rene
--
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 v2 06/12] staging: tidspbridge: convert core to list_head
Hi Ionut,
On Sat, Nov 6, 2010 at 11:31 AM, Ionut Nicu ionut.n...@mindbit.ro wrote:
Hi Rene,
On Fri, 2010-11-05 at 16:12 -0600, Sapiens, Rene wrote:
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the core module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
Signed-off-by: Ionut Nicu ionut.n...@mindbit.ro
snip
diff --git a/drivers/staging/tidspbridge/core/io_sm.c
b/drivers/staging/tidspbridge/core/io_sm.c
index 194bada..9851f32 100644
--- a/drivers/staging/tidspbridge/core/io_sm.c
+++ b/drivers/staging/tidspbridge/core/io_sm.c
snip
@@ -1106,47 +1103,38 @@ static void input_msg(struct io_mgr *pio_mgr,
struct msg_mgr *hmsg_mgr)
* queued.
*/
(*hmsg_mgr-on_exit) ((void *)
-
msg_queue_obj-arg,
-
msg.msg.dw_arg1);
+ msg_queue_obj-arg,
+ msg.msg.dw_arg1);
+ break;
+ }
+ /*
+ * Not an exit acknowledgement, queue
+ * the message.
+ */
+ if
(!list_empty(msg_queue_obj-msg_free_list)) {
You are going beyond the 80 chars.
I thought about it too when using scripts/checkpatch.pl on this patch.
The thing is that it's 81 chars and breaking it into two lines makes it
look uglier. Also, this gets fixed in patch 10/12 (core code cleanup).
I think that it would be better to see every patch as a single element which
would accomplish with all the standards, what if the patch 10/12 doesn't get
merged?...
Probably we can keep this line uglier in this patch and make it prettier in the
10/12 one or even better, make it prettier in this patch.
Regards,
Ionut.
--
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
--
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 v2 07/12] staging: tidspbridge: convert pmgr to list_head
Hi Ionut,
On Sun, Nov 7, 2010 at 7:39 AM, Ionut Nicu ionut.n...@mindbit.ro wrote:
Hi Rene,
On Fri, 2010-11-05 at 16:41 -0600, Sapiens, Rene wrote:
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the pmgr module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
snip
+ * Memory is coalesced back to the appropriate heap when a buffer is
What is being fixed here?
It was a typo (s/coelesced/coalesced/).
Ok, thanks
* freed.
*
* Notes:
snip
@@ -833,67 +768,44 @@ static void add_to_free_list(struct cmm_allocator
*allocator,
DBC_REQUIRE(allocator != NULL);
dw_this_pa = pnode-dw_pa;
dw_next_pa = NEXT_PA(pnode);
i think it would be good to return with error if !allocator or !pnode
and remove the resulting duplicated DBC_REQUIRE.
Yeah I think pnode should be checked for null. Can allocator ever be
null?
It seems that it can actually be NULL, by a possible bug that i just
saw (will send the patch), but taking a deeper look it seems that if
allocator is NULL this function would never be called.
- mnode_obj = (struct cmm_mnode
*)lst_first(allocator-free_list_head);
- while (mnode_obj) {
+ list_for_each_entry(mnode_obj, allocator-free_list, link) {
if (dw_this_pa == NEXT_PA(mnode_obj)) {
snip
@@ -748,18 +736,16 @@ bool dev_init(void)
*/
int dev_notify_clients(struct dev_object *hdev_obj, u32 ret)
{
- int status = 0;
-
struct dev_object *dev_obj = hdev_obj;
- void *proc_obj;
+ struct list_head *curr;
can we add a check for !dev_obj and !dev_obj-proc_list just to be
sure that we get always the correct pointer?
proc_list isn't a pointer. Can dev_obj ever be null?
I suppose that adding an extra check in all functions for the received
parameter would be redundant when those parameters come only from
our driver and were previously checked, also performance would be
affected; so relying on the having of the correct parameters in the path
that calls this function it seems that dev_notify_clients() is not called
with dev_obj with a NULL value.
snip
@@ -947,15 +933,17 @@ int dev_insert_proc_object(struct dev_object
*hdev_obj,
DBC_REQUIRE(refs 0);
DBC_REQUIRE(dev_obj);
DBC_REQUIRE(proc_obj != 0);
- DBC_REQUIRE(dev_obj-proc_list != NULL);
DBC_REQUIRE(already_attached != NULL);
can we check for !hdev_obj, !already_attached even if we have the
DBC_REQUIRE?, maybe we can actually remove the DBC_REQUIRE that could
be redundant after applying this.
Same question here.
The same comment.
- if (!LST_IS_EMPTY(dev_obj-proc_list))
+ if (!list_empty(dev_obj-proc_list))
*already_attached = true;
snip
@@ -986,15 +974,12 @@ int dev_remove_proc_object(struct dev_object
*hdev_obj, u32 proc_obj)
DBC_REQUIRE(dev_obj);
DBC_REQUIRE(proc_obj != 0);
- DBC_REQUIRE(dev_obj-proc_list != NULL);
- DBC_REQUIRE(!LST_IS_EMPTY(dev_obj-proc_list));
+ DBC_REQUIRE(!list_empty(dev_obj-proc_list));
The same comment as above.
Regards,
Ionut.
--
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 v2 08/12] staging: tidspbridge: convert rmgr to list_head
Hi Ionut,
On Sat, Nov 6, 2010 at 12:18 PM, Ionut Nicu ionut.n...@mindbit.ro wrote:
Hi Rene,
On Fri, 2010-11-05 at 18:07 -0600, Sapiens, Rene wrote:
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the rmgr module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
Signed-off-by: Ionut Nicu ionut.n...@mindbit.ro
snip
diff --git a/drivers/staging/tidspbridge/rmgr/drv.c
b/drivers/staging/tidspbridge/rmgr/drv.c
snip
@@ -492,16 +465,17 @@ u32 drv_get_next_dev_object(u32 hdev_obj)
u32 dw_next_dev_object = 0;
struct drv_object *pdrv_obj;
struct drv_data *drv_datap = dev_get_drvdata(bridge);
+ struct list_head *curr;
DBC_REQUIRE(hdev_obj != 0);
can we remove the DBC_REQUIRE and always check for !hdev_obj?
Sounds ok to me.
As a general remark, I personally think that the DBC_* macros should be
replaced with BUG_ON, WARN_ON, but that's a subject for other patches.
What do you think?
I think that the idea is to remove all DBC_REQUIRE/DBC_ASSERT and
use a real check of values when needed, or in the case use the Greg's
and Nishanth Menon's advices.
if (drv_datap drv_datap-drv_object) {
pdrv_obj = drv_datap-drv_object;
- if ((pdrv_obj-dev_list != NULL)
- !LST_IS_EMPTY(pdrv_obj-dev_list)) {
- dw_next_dev_object = (u32)
lst_next(pdrv_obj-dev_list,
- (struct
list_head *)
- hdev_obj);
+ if (!list_empty(pdrv_obj-dev_list)) {
+ curr = (struct list_head *)hdev_obj;
+ if (curr-next == pdrv_obj-dev_list)
Can we use list_is_last() instead?
Good point. I'll fix this.
+ return 0;
+ dw_next_dev_object = (u32) curr-next;
snip
@@ -573,11 +548,8 @@ int drv_insert_dev_object(struct drv_object
*driver_obj,
DBC_REQUIRE(refs 0);
DBC_REQUIRE(hdev_obj != NULL);
DBC_REQUIRE(pdrv_object);
- DBC_ASSERT(pdrv_object-dev_list);
-
As a comment for all the functions that are manipulating lists, can we
check the parameters that they receive?, this applies for some other
functions in these patches, old lst_* functions were internally validating
the having of a valid pointer, now i think that we have to add this to each
function.
I don't think we need to put extra checks. The list head pointer will
never be null and I don't see any use cases where the list_head
container structure can be null...
I mean use the 'checks' only where needed, for this case you are right
it seems that callers of this function ensure that the pointers
are not null.
I'm just worried about a hidden scenario where there could be a call to one
of this functions with a NULL pointer, i.e. lst_put_tail was checking first for
the having of a valid pointer and then it was accessing its element, now
we don' have this, so, care should be taken to do not dereference a NULL
pointer.
- lst_put_tail(pdrv_object-dev_list, (struct list_head *)hdev_obj);
- DBC_ENSURE(!LST_IS_EMPTY(pdrv_object-dev_list));
+ list_add_tail((struct list_head *)hdev_obj,
pdrv_object-dev_list);
snip
@@ -1571,15 +1566,9 @@ int node_enum_nodes(struct node_mgr *hnode_mgr,
void **node_tab,
*pu_num_nodes = 0;
status = -EINVAL;
} else {
- hnode = (struct node_object *)lst_first(hnode_mgr-
- node_list);
- for (i = 0; i hnode_mgr-num_nodes; i++) {
- DBC_ASSERT(hnode);
- node_tab[i] = hnode;
- hnode = (struct node_object *)lst_next
- (hnode_mgr-node_list,
- (struct list_head *)hnode);
- }
+ i = 0;
just a comment, what if we initialize this i when declared and
remove this line.
Ok, will do.
+ list_for_each_entry(hnode, hnode_mgr-node_list,
list_elem)
+ node_tab[i++] = hnode;
*pu_allocated = *pu_num_nodes = hnode_mgr-num_nodes;
}
snip
diff --git a/drivers/staging/tidspbridge/rmgr/rmm.c
b/drivers/staging/tidspbridge/rmgr/rmm.c
snip
@@ -145,20 +141,17 @@ int rmm_alloc(struct rmm_target_obj *target, u32
segid, u32 size,
if (new_sect == NULL) {
status = -ENOMEM;
} else {
- lst_init_elem((struct list_head *)new_sect);
new_sect-addr = addr;
new_sect-size = size;
new_sect-page = segid;
- if (sect == NULL
Re: [PATCH v2 06/12] staging: tidspbridge: convert core to list_head
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the core module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
snip
if (!status) {
/* Get a free chirp: */
- chnl_packet_obj =
- (struct chnl_irp *)lst_get_head(pchnl-free_packets_list);
- if (chnl_packet_obj == NULL)
+ if (!list_empty(pchnl-free_packets_list)) {
+ chnl_packet_obj = list_first_entry(
+ pchnl-free_packets_list,
+ struct chnl_irp, link);
+ list_del(chnl_packet_obj-link);
+ } else
status = -EIO;
What do you think if we close the braces, since the first conditional
has more than one statement?
snip
@@ -286,18 +286,16 @@ int bridge_chnl_cancel_io(struct chnl_object *chnl_obj)
}
}
/* Move all IOR's to IOC queue: */
- while (!LST_IS_EMPTY(pchnl-pio_requests)) {
- chnl_packet_obj =
- (struct chnl_irp *)lst_get_head(pchnl-pio_requests);
- if (chnl_packet_obj) {
- chnl_packet_obj-byte_size = 0;
- chnl_packet_obj-status |= CHNL_IOCSTATCANCEL;
- lst_put_tail(pchnl-pio_completions,
- (struct list_head *)chnl_packet_obj);
- pchnl-cio_cs++;
- pchnl-cio_reqs--;
- DBC_ASSERT(pchnl-cio_reqs = 0);
- }
+ while (!list_empty(pchnl-pio_requests)) {
+ chnl_packet_obj = list_first_entry(pchnl-pio_requests,
+ struct chnl_irp, link);
+ list_del(chnl_packet_obj-link);
+ chnl_packet_obj-byte_size = 0;
+ chnl_packet_obj-status |= CHNL_IOCSTATCANCEL;
+ list_add_tail(chnl_packet_obj-link,
pchnl-pio_completions);
+ pchnl-cio_cs++;
+ pchnl-cio_reqs--;
+ DBC_ASSERT(pchnl-cio_reqs = 0);
Why don't we use list_for_each_entry_safe() instead?
}
func_cont:
spin_unlock_bh(chnl_mgr_obj-chnl_mgr_lock);
snip
@@ -818,9 +804,19 @@ int bridge_chnl_open(struct chnl_object **chnl,
/* Protect queues from io_dpc: */
pchnl-dw_state = CHNL_STATECANCEL;
/* Allocate initial IOR and IOC queues: */
- pchnl-free_packets_list = create_chirp_list(pattrs-uio_reqs);
- pchnl-pio_requests = create_chirp_list(0);
- pchnl-pio_completions = create_chirp_list(0);
+ status = create_chirp_list(pchnl-free_packets_list,
+ pattrs-uio_reqs);
+ if (status)
+ goto func_end;
+
+ status = create_chirp_list(pchnl-pio_requests, 0);
+ if (status)
+ goto func_end;
+
+ status = create_chirp_list(pchnl-pio_completions, 0);
+ if (status)
+ goto func_end;
+
With these goto you are not freeing the memory allocated for pchnl, please free
it at func_end.
Regards,
Rene
--
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 v2 06/12] staging: tidspbridge: convert core to list_head
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the core module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
Signed-off-by: Ionut Nicu ionut.n...@mindbit.ro
snip
diff --git a/drivers/staging/tidspbridge/core/io_sm.c
b/drivers/staging/tidspbridge/core/io_sm.c
index 194bada..9851f32 100644
--- a/drivers/staging/tidspbridge/core/io_sm.c
+++ b/drivers/staging/tidspbridge/core/io_sm.c
snip
@@ -1106,47 +1103,38 @@ static void input_msg(struct io_mgr *pio_mgr, struct
msg_mgr *hmsg_mgr)
* queued.
*/
(*hmsg_mgr-on_exit) ((void *)
- msg_queue_obj-arg,
- msg.msg.dw_arg1);
+ msg_queue_obj-arg,
+ msg.msg.dw_arg1);
+ break;
+ }
+ /*
+ * Not an exit acknowledgement, queue
+ * the message.
+ */
+ if
(!list_empty(msg_queue_obj-msg_free_list)) {
You are going beyond the 80 chars.
Regards,
Rene
Re: [PATCH v2 07/12] staging: tidspbridge: convert pmgr to list_head
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the pmgr module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
snip
+ * Memory is coalesced back to the appropriate heap when a buffer is
What is being fixed here?
* freed.
*
* Notes:
snip
@@ -833,67 +768,44 @@ static void add_to_free_list(struct cmm_allocator
*allocator,
DBC_REQUIRE(allocator != NULL);
dw_this_pa = pnode-dw_pa;
dw_next_pa = NEXT_PA(pnode);
i think it would be good to return with error if !allocator or !pnode
and remove the resulting duplicated DBC_REQUIRE.
- mnode_obj = (struct cmm_mnode *)lst_first(allocator-free_list_head);
- while (mnode_obj) {
+ list_for_each_entry(mnode_obj, allocator-free_list, link) {
if (dw_this_pa == NEXT_PA(mnode_obj)) {
snip
@@ -748,18 +736,16 @@ bool dev_init(void)
*/
int dev_notify_clients(struct dev_object *hdev_obj, u32 ret)
{
- int status = 0;
-
struct dev_object *dev_obj = hdev_obj;
- void *proc_obj;
+ struct list_head *curr;
can we add a check for !dev_obj and !dev_obj-proc_list just to be
sure that we get always the correct pointer?
snip
@@ -947,15 +933,17 @@ int dev_insert_proc_object(struct dev_object *hdev_obj,
DBC_REQUIRE(refs 0);
DBC_REQUIRE(dev_obj);
DBC_REQUIRE(proc_obj != 0);
- DBC_REQUIRE(dev_obj-proc_list != NULL);
DBC_REQUIRE(already_attached != NULL);
can we check for !hdev_obj, !already_attached even if we have the
DBC_REQUIRE?, maybe we can actually remove the DBC_REQUIRE that could
be redundant after applying this.
- if (!LST_IS_EMPTY(dev_obj-proc_list))
+ if (!list_empty(dev_obj-proc_list))
*already_attached = true;
snip
@@ -986,15 +974,12 @@ int dev_remove_proc_object(struct dev_object *hdev_obj,
u32 proc_obj)
DBC_REQUIRE(dev_obj);
DBC_REQUIRE(proc_obj != 0);
- DBC_REQUIRE(dev_obj-proc_list != NULL);
- DBC_REQUIRE(!LST_IS_EMPTY(dev_obj-proc_list));
+ DBC_REQUIRE(!list_empty(dev_obj-proc_list));
The same comment as above.
Regards,
Rene
--
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 v2 08/12] staging: tidspbridge: convert rmgr to list_head
Hi Ionut,
On Fri, Nov 5, 2010 at 9:13 AM, Ionut Nicu ionut.n...@gmail.com wrote:
Convert the rmgr module of the tidspbridge driver
to use struct list_head instead of struct lst_list.
Signed-off-by: Ionut Nicu ionut.n...@mindbit.ro
snip
diff --git a/drivers/staging/tidspbridge/rmgr/drv.c
b/drivers/staging/tidspbridge/rmgr/drv.c
snip
@@ -492,16 +465,17 @@ u32 drv_get_next_dev_object(u32 hdev_obj)
u32 dw_next_dev_object = 0;
struct drv_object *pdrv_obj;
struct drv_data *drv_datap = dev_get_drvdata(bridge);
+ struct list_head *curr;
DBC_REQUIRE(hdev_obj != 0);
can we remove the DBC_REQUIRE and always check for !hdev_obj?
if (drv_datap drv_datap-drv_object) {
pdrv_obj = drv_datap-drv_object;
- if ((pdrv_obj-dev_list != NULL)
- !LST_IS_EMPTY(pdrv_obj-dev_list)) {
- dw_next_dev_object = (u32)
lst_next(pdrv_obj-dev_list,
- (struct list_head
*)
- hdev_obj);
+ if (!list_empty(pdrv_obj-dev_list)) {
+ curr = (struct list_head *)hdev_obj;
+ if (curr-next == pdrv_obj-dev_list)
Can we use list_is_last() instead?
+ return 0;
+ dw_next_dev_object = (u32) curr-next;
snip
@@ -573,11 +548,8 @@ int drv_insert_dev_object(struct drv_object *driver_obj,
DBC_REQUIRE(refs 0);
DBC_REQUIRE(hdev_obj != NULL);
DBC_REQUIRE(pdrv_object);
- DBC_ASSERT(pdrv_object-dev_list);
-
As a comment for all the functions that are manipulating lists, can we
check the parameters that they receive?, this applies for some other
functions in these patches, old lst_* functions were internally validating
the having of a valid pointer, now i think that we have to add this to each
function.
- lst_put_tail(pdrv_object-dev_list, (struct list_head *)hdev_obj);
- DBC_ENSURE(!LST_IS_EMPTY(pdrv_object-dev_list));
+ list_add_tail((struct list_head *)hdev_obj, pdrv_object-dev_list);
snip
@@ -1571,15 +1566,9 @@ int node_enum_nodes(struct node_mgr *hnode_mgr, void
**node_tab,
*pu_num_nodes = 0;
status = -EINVAL;
} else {
- hnode = (struct node_object *)lst_first(hnode_mgr-
- node_list);
- for (i = 0; i hnode_mgr-num_nodes; i++) {
- DBC_ASSERT(hnode);
- node_tab[i] = hnode;
- hnode = (struct node_object *)lst_next
- (hnode_mgr-node_list,
- (struct list_head *)hnode);
- }
+ i = 0;
just a comment, what if we initialize this i when declared and
remove this line.
+ list_for_each_entry(hnode, hnode_mgr-node_list, list_elem)
+ node_tab[i++] = hnode;
*pu_allocated = *pu_num_nodes = hnode_mgr-num_nodes;
}
snip
diff --git a/drivers/staging/tidspbridge/rmgr/rmm.c
b/drivers/staging/tidspbridge/rmgr/rmm.c
snip
@@ -145,20 +141,17 @@ int rmm_alloc(struct rmm_target_obj *target, u32 segid,
u32 size,
if (new_sect == NULL) {
status = -ENOMEM;
} else {
- lst_init_elem((struct list_head *)new_sect);
new_sect-addr = addr;
new_sect-size = size;
new_sect-page = segid;
- if (sect == NULL) {
+ if (sect == NULL)
I think that sect can't be NULL at this point... can be?
can we use: if (list_is_last(sect-list_elem, target-ovly_list)) instead?
/* Put new section at the end of the list */
- lst_put_tail(target-ovly_list,
- (struct list_head *)new_sect);
- } else {
+ list_add_tail(new_sect-list_elem,
+ target-ovly_list);
+ else
snip
@@ -333,24 +316,17 @@ bool rmm_free(struct rmm_target_obj *target, u32 segid,
u32 dsp_addr, u32 size,
} else {
/* Unreserve memory */
- sect = (struct rmm_ovly_sect *)lst_first(target-ovly_list);
- while (sect != NULL) {
+ list_for_each_entry_safe(sect, tmp, target-ovly_list,
+ list_elem) {
if (dsp_addr == sect-addr) {
DBC_ASSERT(size == sect-size);
/* Remove from list */
- lst_remove_elem(target-ovly_list,
- (struct
Re: [PATCH 1/8] staging: tidspbridge: overwrite DSP error codes
On Wed, Nov 3, 2010 at 5:36 PM, Rene Sapiens rene.sapi...@ti.com wrote: When calling the DSP's remote functions, the DSP returns error codes different from the ones managed by the kernel, the function's return value is shared with the MPU using a shared structure. This patch overwrites those error codes by kernel specifics and deletes unnecessary code. The series of patches that follow this one do not apply yet, so please omit this patch, i will send it again as a single one. Regards, Rene -- 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 3/4] staging: tidspbridge: remove gb bitmap implementation
Hi Ionut, On Friday, October 22, 2010 9:09 AM Ionut Nicu wrote: Most likely I will have to break patch 4/4 from this series (which I also believe is way too big) into multiple patches and re-submit. I'm expecting some advices from the maintainers/list on how to split patch 4 (lst_list removal). I have a version of this patch but yours did it first :). You can break it by functionality so that you don't break nor affect bridge's behavior i.e. Patch 1 can include the removing of the wrapper from: drivers/staging/tidspbridge/core/_msg_sm.h drivers/staging/tidspbridge/core/chnl_sm.c drivers/staging/tidspbridge/core/io_sm.c drivers/staging/tidspbridge/core/msg_sm.c drivers/staging/tidspbridge/include/dspbridge/_chnl_sm.h drivers/staging/tidspbridge/include/dspbridge/cmmdefs.h The above files share some of the lists. Patch 2: drivers/staging/tidspbridge/pmgr/cmm.c Patch 3: drivers/staging/tidspbridge/pmgr/dev.c drivers/staging/tidspbridge/rmgr/drv.c drivers/staging/tidspbridge/rmgr/node.c drivers/staging/tidspbridge/rmgr/proc.c Patch 4: drivers/staging/tidspbridge/rmgr/rmm.c Patch 5: Removing of the not needed files. -- Regards, Rene Sapiens-- 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 4/7] omap:mailbox-send message in process context
Hi Hari,
On Thursday, October 14, 2010 9:13 PM Kanigeri, Hari wrote:
Schedule the Tasklet to send only when mailbox fifo is full, else
send the message in the Process context. This would avoid
needless scheduling of Tasklet for every message transfer
Signed-off-by: Hari Kanigeri h-kanige...@ti.com
---
arch/arm/plat-omap/mailbox.c |9 +++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index ed960c1..a4170c7 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -92,20 +92,25 @@ int omap_mbox_msg_send(struct omap_mbox *mbox,
mbox_msg_t msg) struct omap_mbox_queue *mq = mbox-txq;
int ret = 0, len;
- spin_lock(mq-lock);
+ spin_lock_bh(mq-lock);
Please check if this scenario looks valid to you, as discussed and depicted
with Fernando:
Supposing that at this point the hw fifo is full and there are messages in
the kfifo.
if (kfifo_avail(mq-fifo) sizeof(msg)) {
ret = -ENOMEM;
goto out;
}
We reach this point.
In this scenario, the DSP or other Core reads a message from the mbox hw fifo.
The next happens:
1.- The not full interrupt is triggered to the MPU.
2.- The mbox's ISR schedules the tasklet to write the message.
3.- The ISR is left and returns to here (since we still have the bh lock the
tasklet won't run).
+ if (!__mbox_poll_for_space(mbox)) {\
4.- We check for mbox_fifo_full and we have space. so the message is written.
+ mbox_fifo_write(mbox, msg);
At this point it looks that the FIFO order is lost. We write this message
before the ones in the kfifo.
A solution for this could be checking if there are messages in the kfifo
before trying to write directly to the hw fifo, if so, just continue
scheduling the tasklet.
+ goto out;
+ }
+
len = kfifo_in(mq-fifo, (unsigned char *)msg, sizeof(msg));
WARN_ON(len != sizeof(msg));
tasklet_schedule(mbox-txq-tasklet);
out:
- spin_unlock(mq-lock);
+ spin_unlock_bh(mq-lock);
return ret;
}
EXPORT_SYMBOL(omap_mbox_msg_send);
--
1.7.0--
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 4/7] omap:mailbox-send message in process context
Hari,
On Thursday, October 21, 2010 3:49 PM Hari Kanigeri wrote:
Rene,
Thanks for your comment.
@@ -92,20 +92,25 @@ int omap_mbox_msg_send(struct omap_mbox *mbox,
mbox_msg_t msg) struct omap_mbox_queue *mq = mbox-txq;
int ret = 0, len;
- spin_lock(mq-lock);
+ spin_lock_bh(mq-lock);
Please check if this scenario looks valid to you, as discussed and depicted
with Fernando:
Supposing that at this point the hw fifo is full and there are messages in
the kfifo.
if (kfifo_avail(mq-fifo) sizeof(msg)) {
ret = -ENOMEM;
goto out;
}
We reach this point.
In this scenario, the DSP or other Core reads a message from the mbox hw
fifo. The next happens:
1.- The not full interrupt is triggered to the MPU.
2.- The mbox's ISR schedules the tasklet to write the message.
3.- The ISR is left and returns to here (since we still have the bh lock
the tasklet won't run).
+ if (!__mbox_poll_for_space(mbox)) {\
4.- We check for mbox_fifo_full and we have space. so the message is
written.
+ mbox_fifo_write(mbox, msg);
At this point it looks that the FIFO order is lost. We write this message
before the ones in the kfifo.
Good point. I agree.
A solution for this could be checking if there are messages in the kfifo
before trying to write directly to the hw fifo, if so, just continue
scheduling the tasklet.
A change something like this ?
- if (!__mbox_poll_for_space(mbox)) {
+ if (kfifo_is_empty() !__mbox_poll_for_space(mbox)) {
Yes, this looks good to me.
mbox_fifo_write(mbox, msg);
Regards,
Rene--
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: [PATCHv2] mailbox: change full flag per mailbox queue instead of global
Hi Ohad,
This patch already contains the missed changes due to previous rebases.
Thanks.
Regards,
Rene
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Guzman Lugo, Fernando
Sent: Tuesday, August 10, 2010 8:13 PM
To: linux-arm-ker...@lists.infradead.org; linux-omap@vger.kernel.org
Cc: hiroshi.d...@nokia.com; o...@wizery.com; Guzman Lugo, Fernando
Subject: [PATCHv2] mailbox: change full flag per mailbox queue instead of
global
As pointed by Ohad Ben-Cohen, the variable rq_full flag is a
global variable, so if there are multiple mailbox users
there will be conflics. Now there is a full flag per
mailbox queue.
Version 2:
- Rebase to the latest.
Reported-by: Ohad Ben-Cohen o...@wizery.com
Signed-off-by: Fernando Guzman Lugo x0095...@ti.com
---
arch/arm/plat-omap/include/plat/mailbox.h |1 +
arch/arm/plat-omap/mailbox.c | 10 +++---
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-
omap/include/plat/mailbox.h
index 9976565..261f6b8 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -48,6 +48,7 @@ struct omap_mbox_queue {
struct tasklet_struct tasklet;
int (*callback)(void *);
struct omap_mbox*mbox;
+ boolfull;
};
struct omap_mbox {
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index d2fafb8..c59c9c3 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -33,7 +33,6 @@
static struct workqueue_struct *mboxd;
static struct omap_mbox **mboxes;
-static bool rq_full;
static int mbox_configured;
static DEFINE_MUTEX(mbox_configured_lock);
@@ -145,7 +144,12 @@ static void mbox_rx_work(struct work_struct *work)
while (kfifo_len(mq-fifo) = sizeof(msg)) {
len = kfifo_out(mq-fifo, (unsigned char *)msg,
sizeof(msg));
WARN_ON(len != sizeof(msg));
-
+ spin_lock_irq(mq-lock);
+ if (mq-full) {
+ omap_mbox_enable_irq(mq-mbox, IRQ_RX);
+ mq-full = false;
+ }
+ spin_unlock_irq(mq-lock);
if (mq-callback)
mq-callback((void *)msg);
}
@@ -170,7 +174,7 @@ static void __mbox_rx_interrupt(struct omap_mbox
*mbox)
while (!mbox_fifo_empty(mbox)) {
if (unlikely(kfifo_avail(mq-fifo) sizeof(msg))) {
omap_mbox_disable_irq(mbox, IRQ_RX);
- rq_full = true;
+ mq-full = true;
goto nomem;
}
--
1.6.3.3
--
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
--
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 09/10] omap: mailbox: convert block api to kfifo
Hi Ohad, Sure I will do it. Regards, Rene -Original Message- From: Ohad Ben-Cohen [mailto:o...@wizery.com] Sent: Tuesday, August 10, 2010 9:43 AM To: Guzman Lugo, Fernando; Sapiens, Rene Cc: Hiroshi DOYU; linux-omap@vger.kernel.org; linux-arm- ker...@lists.infradead.org; Kanigeri, Hari Subject: Re: [PATCH 09/10] omap: mailbox: convert block api to kfifo Hi Rene, On Wed, Jun 9, 2010 at 8:38 AM, Guzman Lugo, Fernando fernando.l...@ti.com wrote: On Tue, Jun 8, 2010 at 7:16 PM, Sapiens, Rene rene.sapi...@ti.com wrote: In mbox_rx_work() you are removing the lines that enable back the mbox irq for the RX case, but inside __mbox_rx_interrupt() this interrupt is disabled in the case that the kfifo for Rx mailbox gets full. So I think that we need to enable it back as soon as there is space in this kfifo. Actually these irq on/off lines are not part of my patch; they are introduced by patch 05/10 on top of which my patches were rebased. Nevertheless I agree with you - the kfifo migration patch should not affect that irq on/off behavior. It's probably just a rebase gotcha. But now that you point me to this irq on/off thing, it looks a bit broken in terms of multiple concurrent mbox support since it relies on a global rq_full state. I guess it'd be better to hold that rq_full state in the relevant mbox queue state itself. Fernando what do you think ? Yes, you are right Ohad. Only should be disable the new message interrupt of the mailbox which kfifo is full. Once Fernando's fix will get thru, we will be able to fix the rebase error that you pointed out. Unfortunately I will not have any email access in the next 3 weeks, and I was hoping maybe you could submit a fix for this once Fernando's fix is accepted ? I would really like us to fix this early in the days of 2.6.36, maybe even during the merge window. Thanks a lot, Ohad. regards, Fernando. Thanks, Ohad. -- 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: Serial console not working after waking up from sleep
You can do a telnet to the device... you should be able to work with it but your serial session will show the garbage. Regards, Rene -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Michael Trimarchi Sent: Wednesday, June 16, 2010 9:47 AM To: Han Wang Cc: linux-omap@vger.kernel.org Subject: Re: Serial console not working after waking up from sleep Hi Han Wang wrote: Hi, michael, I have the no_console_suspend option in my boot command line, I am not sure if that is the option you were trying to point me to in the last email? I have said that i have no problem when I remove that option. Can you try to echo 0 to timeout of the serial device? Michael anyway, I added no_debug_console into my boot command arg, but that doesn't seem to help with my problem. any ideas? Thanks, Han On Wed, Jun 16, 2010 at 2:09 AM, Michael Trimarchi mich...@panicking.kicks-ass.org wrote: Han Wang wrote: Hi, I am testing the 2.6.35-rc1 pm branch code on Overo. The system boots ok. (I can provide booting log if that is necessary) However, when I use echo mem /sys/power/state to send overo to sleep and wake it up by enter a key into serial console. I got garbage characters in the serial console, and I can not enter anything into the console anymore. I wonder if anyone has encountered a similar problem, and please give me some suggestion. I have appended command log below. r...@overo:~# echo mem /sys/power/state PM: Syncing filesystems ... done. PM: Preparing system for mem sleep PM: Adding info for No Bus:vcs63 PM: Adding info for No Bus:vcsa63 Freezing user space processes ... (elapsed 0.02 seconds) done. Freezing remaining freezable tasks ... (elapsed 0.01 seconds) done. PM: Entering mem sleep i2c_omap i2c_omap.1: preparing suspend i2c_omap i2c_omap.3: preparing suspend platform overo_lcd: preparing suspend serial8250 serial8250.0: preparing suspend, may wakeup serial8250 serial8250.1: preparing suspend, may wakeup serial8250 serial8250.2: preparing suspend, may wakeup platform omap2-nand: preparing suspend platform musb_hdrc: preparing suspend platform smsc911x.0: preparing suspend platform smsc911x.1: preparing suspend platform omap2_mcspi.1: preparing suspend platform omap2_mcspi.2: preparing suspend platform omap2_mcspi.3: preparing suspend platform omap2_mcspi.4: preparing suspend arm-pmu arm-pmu.0: preparing suspend platform omap_rng: preparing suspend platform omapfb: preparing suspend twl4030_gpio twl4030_gpio: preparing suspend mmci-omap-hs mmci-omap-hs.0: preparing suspend mmci-omap-hs mmci-omap-hs.1: preparing suspend twl_reg twl_reg.17: preparing suspend twl_reg twl_reg.18: preparing suspend twl_reg twl_reg.19: preparing suspend twl4030_usb twl4030_usb: preparing suspend, may wakeup twl_reg twl_reg.6: preparing suspend serial8250 serial8250: preparing suspend mmcblk mmc0:fb2a: legacy suspend serial8250 serial8250: suspend i2c i2c-3: suspend twl_reg twl_reg.6: suspend twl4030_usb twl4030_usb: suspend, may wakeup twl_reg twl_reg.19: suspend twl_reg twl_reg.18: suspend twl_reg twl_reg.17: suspend mmci-omap-hs mmci-omap-hs.1: suspend mmci-omap-hs mmci-omap-hs.0: suspend twl4030_gpio twl4030_gpio: suspend dummy 1-004b: suspend dummy 1-004a: suspend dummy 1-0049: suspend twl 1-0048: suspend, may wakeup i2c i2c-1: suspend platform omapfb: suspend platform omap_rng: suspend arm-pmu arm-pmu.0: suspend platform omap2_mcspi.4: suspend platform omap2_mcspi.3: suspend platform omap2_mcspi.2: suspend platform omap2_mcspi.1: suspend platform smsc911x.1: suspend platform smsc911x.0: suspend platform musb_hdrc: suspend platform omap2-nand: suspend serial8250 serial8250.2: suspend, may wakeup serial8250 serial8250.1: suspend, may wakeup serial8250 serial8250.0: suspend, may wakeup platform overo_lcd: suspend i2c_omap i2c_omap.3: suspend i2c_omap i2c_omap.1: suspend PM: suspend of devices complete after 201.965 msecs serial8250 serial8250: LATE suspend i2c i2c-3: LATE suspend twl_reg twl_reg.6: LATE suspend twl4030_usb twl4030_usb: LATE suspend, may wakeup twl_reg twl_reg.19: LATE suspend twl_reg twl_reg.18: LATE suspend twl_reg twl_reg.17: LATE suspend mmci-omap-hs mmci-omap-hs.1: LATE suspend mmci-omap-hs mmci-omap-hs.0: LATE suspend twl4030_gpio twl4030_gpio: LATE suspend dummy 1-004b: LATE suspend dummy 1-004a: LATE suspend dummy 1-0049: LATE suspend twl 1-0048: LATE suspend, may wakeup i2c i2c-1: LATE suspend platform omapfb: LATE suspend platform omap_rng: LATE suspend arm-pmu arm-pmu.0: LATE suspend platform omap2_mcspi.4: LATE suspend platform omap2_mcspi.3: LATE suspend platform omap2_mcspi.2: LATE suspend platform omap2_mcspi.1: LATE suspend platform smsc911x.1: LATE suspend platform
RE: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo
Hi Hiroshi -Original Message- From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- ow...@vger.kernel.org] On Behalf Of Hiroshi DOYU Sent: Monday, June 14, 2010 3:58 AM To: Guzman Lugo, Fernando; o...@wizery.com Cc: Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Kanigeri, Hari; linux- o...@vger.kernel.org Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Hi Ohad, From: ext Ohad Ben-Cohen o...@wizery.com Subject: Re: [PATCH v3 4/4] omap: mailbox: convert block api to kfifo Date: Mon, 14 Jun 2010 01:52:16 +0200 On Wed, Jun 9, 2010 at 12:07 AM, Hiroshi DOYU hiroshi.d...@nokia.com wrote: diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat- omap/mailbox.c index 87e0cde..1b79b32 100644 --- a/arch/arm/plat-omap/mailbox.c +++ b/arch/arm/plat-omap/mailbox.c @@ -188,7 +188,7 @@ static void __mbox_rx_interrupt(struct omap_mbox *mbox) /* no more messages in the fifo. clear IRQ source. */ ack_mbox_irq(mbox, IRQ_RX); nomem: - queue_work(mboxd, mbox-rxq-work); + mbox-callback(mbox); } I like this ! It will allow us to easily plug in new IPC mechanisms in the future. Agree. (btw, if we do this, and also introduce omap_mbox_msg_send_sync, do we still need a mailbox queuing mechanism at all ? :) I think that queuing(can be called as S/W fifo/buffer?) is basically necessary it can compensate the shortage of H/W fifo slots under high load or emergency case. It has only 4 slots. I guess that, DSP usually responds quickly and 4 H/W slots may be enough, but it might be safer/more robust to avoid the assumption which depends on other entity/DSP. From latecy perspective, s/w fifo + tasklet would be enough short. omap_mbox_msg_send_sync() can handle the case for a special message, like PM, which has to respond at the higher priority than the normal ones. Having said that, this is not going to solve the lockdep warning reported by Deepak - that was caused because of dspbridge's sending context (and not because of the receiving context). To eliminate that Does dspbridge really need its own defered work for sending mailbox messages? For me, the problem here is the unneccesary duplication of tasklet, or can be said, the unnecessary use of tasklet for _sending_ mailbox messages in dspbridge. http://marc.info/?l=linux-omapm=127601655325416w=2 I thought that bridge_msg_put()/bridge_msg_get() can use omap mbox APIs directly, with getting rid of its use of its own defered work/tasklet as pointed out in the above link. Fernando? Actually the intention of that deferred work is not to send the Mbox messages. Bridge shares a section of physical memory with the DSP and communication is established through this one. Messages (not mailbox messages) coming from users go into bridge_msg_put(), those are more significant messages containing more arguments that are understandable by a DSP task. If the Shared memory(SHM) is occupied by a previous message, messages are queued inside bridge_msg_put() and the tasklet to send them is scheduled (so that we don't loose any) due that there can only be a single message going out and one coming back at a time in this Shared memory. The usage of the Mailbox messages is to notify to the DSP (actually to interrupt it and make it go to check the SHM) that there is a message with more information in that shared memory. The tasklet is needed to serialize the upper communication that is done in the SHM level and every time that we want to send out a message previously queued we need to interrupt the DSP (use the Mailbox messages). On the other hand PM messages are sent directly by using the Mailbox messages where we directly call omap_mbox_msg_send(). For recieving, its defered work(tasklet) can be trigered directly in the above proposed callback, that callback can triger its own workqueue if necessary, then. I think that, for recieving, some PM command may has to be sent back immedieately inside of tasklet. omap_mbox_msg_send_sync() may handle this case. What do you think? issue, I prefer fixing dspbridge to use work queues rather than using spin_lock_bh in omap_mbox_msg_send. Disabling system bottom halves just to send a mbox msg sounds unjustified (unless bridge really needs to use tasklets instead of work queues, which I slightly doubt). What do you think ? I think that workqueue is only necessary when it has to sleep, otherwise tasklet is prefered. For _sending_ a message inside of dspbridge, I haven't found any reasonable reason to use any defered work(softirq, tasklet, workqueue) so far. Speaking of mailbox I'd like to address some issues that are code related: * Let's add mailbox API to set the callback pointer (it feels wrong to let users directly manipulate the mbox structure). * We can also safely move the callback field to the main mbox structure, since it has no usage in the
RE: [PATCH 09/10] omap: mailbox: convert block api to kfifo
Hi Ohad,
In mbox_rx_work() you are removing the lines that enable back the mbox irq for
the RX case, but inside __mbox_rx_interrupt() this interrupt is disabled in
the case that the kfifo for Rx mailbox gets full. So I think that we need to
enable it back as soon as there is space in this kfifo.
Regards,
Rene
-Original Message-
From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
ow...@vger.kernel.org] On Behalf Of Hiroshi DOYU
Sent: Thursday, June 03, 2010 1:34 AM
To: linux-omap@vger.kernel.org; linux-arm-ker...@lists.infradead.org
Cc: Ohad Ben-Cohen; Kanigeri, Hari; Hiroshi DOYU
Subject: [PATCH 09/10] omap: mailbox: convert block api to kfifo
From: Ohad Ben-Cohen o...@wizery.com
The underlying buffering implementation of mailbox
is converted from block API to kfifo due to the simplicity
and speed of kfifo.
The default size of the kfifo buffer is set to 256 bytes.
This value is configurable at compile time (via
CONFIG_OMAP_MBOX_KFIFO_SIZE), and can be changed at
runtime (via the mbox_kfifo_size module parameter).
Signed-off-by: Ohad Ben-Cohen o...@wizery.com
Signed-off-by: Hari Kanigeri h-kanige...@ti.com
Signed-off-by: Hiroshi DOYU hiroshi.d...@nokia.com
---
arch/arm/plat-omap/Kconfig|9 ++
arch/arm/plat-omap/include/plat/mailbox.h |4 +-
arch/arm/plat-omap/mailbox.c | 119 +---
-
3 files changed, 64 insertions(+), 68 deletions(-)
diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
index 78b49a6..111d39a 100644
--- a/arch/arm/plat-omap/Kconfig
+++ b/arch/arm/plat-omap/Kconfig
@@ -106,6 +106,15 @@ config OMAP_MBOX_FWK
Say Y here if you want to use OMAP Mailbox framework support for
DSP, IVA1.0 and IVA2 in OMAP1/2/3.
+config OMAP_MBOX_KFIFO_SIZE
+ int Mailbox kfifo default buffer size (bytes)
+ depends on OMAP_MBOX_FWK
+ default 256
+ help
+ Specify the default size of mailbox's kfifo buffers (bytes).
+ This can also be changed at runtime (via the mbox_kfifo_size
+ module parameter).
+
config OMAP_IOMMU
tristate
diff --git a/arch/arm/plat-omap/include/plat/mailbox.h b/arch/arm/plat-
omap/include/plat/mailbox.h
index 729166b..0c3c4a5 100644
--- a/arch/arm/plat-omap/include/plat/mailbox.h
+++ b/arch/arm/plat-omap/include/plat/mailbox.h
@@ -5,8 +5,8 @@
#include linux/wait.h
#include linux/workqueue.h
-#include linux/blkdev.h
#include linux/interrupt.h
+#include linux/kfifo.h
typedef u32 mbox_msg_t;
struct omap_mbox;
@@ -42,7 +42,7 @@ struct omap_mbox_ops {
struct omap_mbox_queue {
spinlock_t lock;
- struct request_queue*queue;
+ struct kfifofifo;
struct work_struct work;
struct tasklet_struct tasklet;
int (*callback)(void *);
diff --git a/arch/arm/plat-omap/mailbox.c b/arch/arm/plat-omap/mailbox.c
index 81076b5..ec0e159 100644
--- a/arch/arm/plat-omap/mailbox.c
+++ b/arch/arm/plat-omap/mailbox.c
@@ -21,11 +21,14 @@
*
*/
+#include linux/kernel.h
#include linux/module.h
#include linux/interrupt.h
#include linux/device.h
#include linux/delay.h
#include linux/slab.h
+#include linux/kfifo.h
+#include linux/err.h
#include plat/mailbox.h
@@ -37,6 +40,10 @@ static bool rq_full;
static int mbox_configured;
static DEFINE_MUTEX(mbox_configured_lock);
+static unsigned int mbox_kfifo_size = CONFIG_OMAP_MBOX_KFIFO_SIZE;
+module_param(mbox_kfifo_size, uint, S_IRUGO);
+MODULE_PARM_DESC(mbox_kfifo_size, Size of omap's mailbox kfifo
(bytes));
+
/* Mailbox FIFO handle functions */
static inline mbox_msg_t mbox_fifo_read(struct omap_mbox *mbox)
{
@@ -69,7 +76,7 @@ static inline int is_mbox_irq(struct omap_mbox *mbox,
omap_mbox_irq_t irq)
/*
* message sender
*/
-static int __mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
+static int __mbox_poll_for_space(struct omap_mbox *mbox)
{
int ret = 0, i = 1000;
@@ -80,49 +87,50 @@ static int __mbox_msg_send(struct omap_mbox *mbox,
mbox_msg_t msg)
return -1;
udelay(1);
}
- mbox_fifo_write(mbox, msg);
return ret;
}
-
int omap_mbox_msg_send(struct omap_mbox *mbox, mbox_msg_t msg)
{
+ struct omap_mbox_queue *mq = mbox-txq;
+ int ret = 0, len;
- struct request *rq;
- struct request_queue *q = mbox-txq-queue;
+ spin_lock(mq-lock);
- rq = blk_get_request(q, WRITE, GFP_ATOMIC);
- if (unlikely(!rq))
- return -ENOMEM;
+ if (kfifo_avail(mq-fifo) sizeof(msg)) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ len = kfifo_in(mq-fifo, (unsigned char *)msg, sizeof(msg));
+ WARN_ON(len != sizeof(msg));
- blk_insert_request(q, rq, 0, (void *) msg);
tasklet_schedule(mbox-txq-tasklet);
- return 0;
+out:
+ spin_unlock(mq-lock);
+
