[PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies ipu_pm
Bryan Wu
bryan.wu at canonical.com
Fri Jul 2 04:23:33 UTC 2010
On 07/01/2010 11:53 PM, Vadillo, Miguel wrote:
> Answers below.
>
>> -----Original Message-----
>> From: Tim Gardner [mailto:tim.gardner at canonical.com]
>> Sent: Thursday, July 01, 2010 9:47 AM
>> To: Vadillo, Miguel
>> Cc: Bryan Wu; kernel-team at lists.ubuntu.com; ogra at canonical.com; Jan,
>> Sebastien
>> Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies
>> ipu_pm
>>
>> On 06/30/2010 11:02 PM, Bryan Wu wrote:
>>> From: Miguel Vadillo<vadillo at ti.com>
>>>
>>> Changes to fix circular dependencies when compiling ipu_pm in
>>> modules mode.
>>>
>>> Signed-off-by: Miguel Vadillo<vadillo at ti.com>
>>> ---
>>> arch/arm/plat-omap/clock.c | 1 +
>>> drivers/dsp/syslink/ipu_pm/ipu_pm.c | 60
>> ++++++++++++++++++++++++--
>>> drivers/dsp/syslink/ipu_pm/ipu_pm.h | 10 +++--
>>> drivers/dsp/syslink/multicore_ipc/platform.c | 31 ++------------
>>> 4 files changed, 67 insertions(+), 35 deletions(-)
>>> mode change 100644 => 100755 arch/arm/plat-omap/clock.c
>>> mode change 100644 => 100755 drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>> mode change 100644 => 100755 drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>> mode change 100644 => 100755
>> drivers/dsp/syslink/multicore_ipc/platform.c
>>>
>>> diff --git a/arch/arm/plat-omap/clock.c b/arch/arm/plat-omap/clock.c
>>> old mode 100644
>>> new mode 100755
>>> index 701e4ea..104eaec
>>> --- a/arch/arm/plat-omap/clock.c
>>> +++ b/arch/arm/plat-omap/clock.c
>>> @@ -336,6 +336,7 @@ struct clk *omap_clk_get_by_name(const char *name)
>>>
>>> return ret;
>>> }
>>> +EXPORT_SYMBOL(omap_clk_get_by_name);
>>>
>>> /*
>>> * Low level helpers
>>> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>> b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>> old mode 100644
>>> new mode 100755
>>> index 3806f5a..f27512f
>>> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.c
>>> @@ -85,6 +85,9 @@ int ipu_timer_list[NUM_IPU_TIMERS] = {
>>>
>>> struct omap_dm_timer *p_gpt;
>>> struct clk *p_i2c_clk;
>>> +struct sms *rcb_table;
>>> +void *ipu_pm_notifydrv_handle;
>>> +struct pm_event *pm_event;
>>>
>>
>> Is there any reason these aren't static? Their scope appears to be
>> purely local to this file.
>
>
> In the next release we are getting rid of the ipu_pm_notifydrv_handle and we are just providing a handle to access both rcb_table and pm_event.
>
Actually, not only these 3 pointers, but also lots of varibles and functions are
locally in drivers/dsp/syslink/ipu_pm/ipu_pm.c. They can be set as static.
But that is out of this patch.
>>
>>> /**
>> ==========================================================================
>> ==
>>> * Forward declarations of internal functions
>>> @@ -221,7 +224,7 @@ void ipu_pm_callback(short int procId,
>>>
>>> /* send the ACK to DUCATI*/
>>> return_val = notify_sendevent(
>>> - platform_notifydrv_handle,
>>> + ipu_pm_notifydrv_handle,
>>> SYS_M3,/*DUCATI_PROC*/
>>> PM_RESOURCE,/*PWR_MGMT_EVENT*/
>>> payload,
>>> @@ -280,7 +283,7 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>> pm_msg.fields.parm = PM_SUCCESS;
>>> /* send the ACK to DUCATI*/
>>> return_val = notify_sendevent(
>>> - platform_notifydrv_handle,
>>> + ipu_pm_notifydrv_handle,
>>> SYS_M3,/*DUCATI_PROC*/
>>> PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>>> (unsigned int)pm_msg.whole,
>>> @@ -301,7 +304,7 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>> pm_msg.fields.parm = PM_SUCCESS;
>>> /* send the ACK to DUCATI*/
>>> return_val = notify_sendevent(
>>> - platform_notifydrv_handle,
>>> + ipu_pm_notifydrv_handle,
>>> SYS_M3,/*DUCATI_PROC*/
>>> PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>>> (unsigned int)pm_msg.whole,
>>> @@ -322,7 +325,7 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>> pm_msg.fields.parm = PM_SUCCESS;
>>> /* send the ACK to DUCATI*/
>>> return_val = notify_sendevent(
>>> - platform_notifydrv_handle,
>>> + ipu_pm_notifydrv_handle,
>>> SYS_M3,/*DUCATI_PROC*/
>>> PM_NOTIFICATION,/*PWR_MGMT_EVENT*/
>>> (unsigned int)pm_msg.whole,
>>> @@ -344,6 +347,53 @@ int ipu_pm_notifications(enum pm_event_type
>> event_type)
>>> EXPORT_SYMBOL(ipu_pm_notifications);
>>>
>>> /*
>>> + Function for setup ipu_pm module
>>> + *
>>> + */
>>> +int ipu_pm_setup(void *notify_driver_handle)
>>> +{
>>> + u32 i = 0;
>>> + ipu_pm_notifydrv_handle = notify_driver_handle;
>>> + /* Get the shared RCB */
>>> + rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
>>> + sizeof(struct sms));
>>> +
>>> + pm_event = kzalloc(sizeof(struct pm_event) * NUMBER_PM_EVENTS,
>>> + GFP_KERNEL);
>>> +
>>> + /* Each event has it own sem */
>>> + for (i = 0; i< NUMBER_PM_EVENTS; i++) {
>>> + pm_event[i].sem_handle = kzalloc(sizeof(struct semaphore),
>>> + GFP_KERNEL);
>>> + sema_init(pm_event[i].sem_handle, 0);
>>> + pm_event[i].event_type = i;
>>> + }
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(ipu_pm_setup);
>>> +
>>
>> Why bother returning a status code if its just a constant? How about
>> checking for failures when calling ioremap() and kzalloc() ?
>
> This part is now fixed and will be available for the next release too. We are checking the kzalloc. Also the ioremap is not done anymore since we are requesting a heap from the share memory.
>>
>>> +/*
>>> + Function for finish ipu_pm module
>>> + *
>>> + */
>>> +int ipu_pm_finish()
>>> +{
>>> + u32 i = 0;
>>> + /* Release the shared RCB */
>>> + for (i = 0; i< NUMBER_PM_EVENTS; i++) {
>>> + kfree(pm_event[i].sem_handle);
>>> + pm_event[i].event_type = 0;
>>> + }
>>> + kfree(pm_event);
>>> + pm_event = NULL;
>>> + iounmap(rcb_table);
>>> + rcb_table = NULL;
>>> + ipu_pm_notifydrv_handle = NULL;
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL(ipu_pm_finish);
>>> +
>>> +/*
>>> Function for get sdma channels from PRCM
>>> *
>>> */
>>> @@ -518,3 +568,5 @@ inline void ipu_pm_rel_i2c_bus(unsigned rcb_num)
>>> pm_i2c_bus_counter--;
>>> }
>>>
>>> +MODULE_LICENSE("GPL");
>>> +
>>> diff --git a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>> b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>> old mode 100644
>>> new mode 100755
>>> index e9106fb..4481c6d
>>> --- a/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>> +++ b/drivers/dsp/syslink/ipu_pm/ipu_pm.h
>>> @@ -185,10 +185,6 @@ struct rcb_block {
>>>
>>> };
>>>
>>> -extern struct sms *rcb_table;
>>> -extern void *platform_notifydrv_handle;
>>> -extern struct pm_event *pm_event;
>>> -
>>> struct sms {
>>> unsigned rat;
>>> struct rcb_block rcb[RCB_MAX];
>>> @@ -214,5 +210,11 @@ void ipu_pm_notify_callback(short int procId,
>>> /* Function for send PM Notifications */
>>> int ipu_pm_notifications(enum pm_event_type event_type);
>>>
>>> +/* Function for setup ipu_pm module */
>>> +int ipu_pm_setup(void *notify_driver_handle);
>>> +
>>> +/* Function for finish ipu_pm module */
>>> +int ipu_pm_finish(void);
>>> +
>>> #endif
>>>
>>> diff --git a/drivers/dsp/syslink/multicore_ipc/platform.c
>> b/drivers/dsp/syslink/multicore_ipc/platform.c
>>> old mode 100644
>>> new mode 100755
>>> index d1c62e7..1cdd686
>>> --- a/drivers/dsp/syslink/multicore_ipc/platform.c
>>> +++ b/drivers/dsp/syslink/multicore_ipc/platform.c
>>> @@ -233,9 +233,6 @@
>>> */
>>> void *platform_notifydrv_handle;
>>>
>>> -struct pm_event *pm_event;
>>> -struct sms *rcb_table;
>>> -
>>> /* Handles for SysM3 */
>>> void *platform_nsrn_gate_handle_sysm3;
>>> void *platform_nsrn_handle_sysm3;
>>> @@ -982,22 +979,8 @@ void platform_start_callback(void *arg)
>>> goto pm_register_fail;
>>> }
>>>
>>> - /* Get the shared RCB */
>>> - rcb_table = (struct sms *) ioremap(PM_SHM_BASE_ADDR,
>>> - sizeof(struct sms));
>>> -
>>> - pm_event =
>>> - kzalloc(sizeof(struct pm_event)
>>> - * NUMBER_PM_EVENTS, GFP_KERNEL);
>>> -
>>> - /* Each event has it own sem */
>>> - for (i = 0; i< NUMBER_PM_EVENTS; i++) {
>>> - pm_event[i].sem_handle =
>>> - kzalloc(sizeof(struct semaphore),
>>> - GFP_KERNEL);
>>> - sema_init(pm_event[i].sem_handle, 0);
>>> - pm_event[i].event_type = i;
>>> - }
>>> + ipu_pm_setup(platform_notifydrv_handle);
>>> +
>>> }
>>> /* END PM */
>>>
>>> @@ -1334,7 +1317,6 @@ void platform_stop_callback(void *arg)
>>> u16 proc_id = (u32) arg;
>>> int index = 0;
>>> u32 nread = 0;
>>> - u32 i = 0;
>>>
>>> if (proc_id == multiproc_get_id("SysM3"))
>>> index = SMHEAP_SRINDEX_SYSM3;
>>> @@ -1460,13 +1442,8 @@ void platform_stop_callback(void *arg)
>>> (void *)NULL);
>>> if (status< 0)
>>> printk(KERN_INFO "ERROR UNREGISTERING PM
>> EVENT\n");
>>> - for (i = 0; i< NUMBER_PM_EVENTS; i++) {
>>> - kfree(pm_event[i].sem_handle);
>>> - pm_event[i].event_type = 0;
>>> - }
>>> - kfree(pm_event);
>>> - /* Release the shared RCB */
>>> - iounmap(rcb_table);
>>> +
>>> + ipu_pm_finish();
>>> }
>>> /* END PM */
>>>
>>
>> I realize this is outside the scope of this patch, but why are the
>> semaphore events handles (pm_event[i].sem_handle) allocated? In other
>> words, why isn't the structure definition:
>>
>> struct pm_event {
>> enum pm_event_type event_type;
>> struct semaphore sem;
>> };
>>
>> Then the initialization wouldn't have to allocate memory and could just
>> call sema_init(&pm_event[i].sem, 0), which makes init and de-init a bit
>> simpler.
>
> This one is a very good point thanks for the tip I will try this and it will probably be included for the next release too.
>>
And will we got an incremental patch to fix this according to the review. Or we
have to wait for next release.
Thanks,
-Bryan
More information about the kernel-team
mailing list