[PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies ipu_pm
Vadillo, Miguel
vadillo at ti.com
Fri Jul 2 05:27:29 UTC 2010
The changes will be on next release if they are easy to do since we are close to the release but we take consideration of all suggestions even if they are out of the scope so thanks in advance for the feedback.
Best Regards.
Miguel Vadillo
________________________________________
From: Bryan Wu [bryan.wu at canonical.com]
Sent: Thursday, July 01, 2010 11:23 PM
To: Vadillo, Miguel
Cc: tim.gardner at canonical.com; kernel-team at lists.ubuntu.com; ogra at canonical.com; Jan, Sebastien; Hunt, Paul; Shah, Bhavin; Liu, Stanley; Gutierrez, Juan
Subject: Re: [PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies ipu_pm
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