| Message ID | 1277960546-32336-4-git-send-email-bryan.wu@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
Tim, This patch fix the module building failure issue which is similar to the patches from you. And it wrapped those pointers with access helper functions, this solve the loop dependencies issue. I think it is OK for me. Acked-by: Bryan Wu <bryan.wu@canonical.com> On 07/01/2010 01:02 PM, Bryan Wu wrote: > From: Miguel Vadillo <vadillo@ti.com> > > Changes to fix circular dependencies when compiling ipu_pm in > modules mode. > > Signed-off-by: Miguel Vadillo <vadillo@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; > > /** ============================================================================ > * 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); > + > +/* > + 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 */ >
On 06/30/2010 11:02 PM, Bryan Wu wrote: > From: Miguel Vadillo<vadillo@ti.com> > > Changes to fix circular dependencies when compiling ipu_pm in > modules mode. > > Signed-off-by: Miguel Vadillo<vadillo@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. > /** ============================================================================ > * 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() ? > +/* > + 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. rtg
Answers below. > -----Original Message----- > From: Tim Gardner [mailto:tim.gardner@canonical.com] > Sent: Thursday, July 01, 2010 9:47 AM > To: Vadillo, Miguel > Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@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@ti.com> > > > > Changes to fix circular dependencies when compiling ipu_pm in > > modules mode. > > > > Signed-off-by: Miguel Vadillo<vadillo@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. > > > /** > ========================================================================== > == > > * 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. > > rtg > -- > Tim Gardner tim.gardner@canonical.com Thanks a lot for the feedback Regards... Miguel Vadillo Cel. 214-587-2910
On 07/01/2010 11:53 PM, Vadillo, Miguel wrote: > Answers below. > >> -----Original Message----- >> From: Tim Gardner [mailto:tim.gardner@canonical.com] >> Sent: Thursday, July 01, 2010 9:47 AM >> To: Vadillo, Miguel >> Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@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@ti.com> >>> >>> Changes to fix circular dependencies when compiling ipu_pm in >>> modules mode. >>> >>> Signed-off-by: Miguel Vadillo<vadillo@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
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
On 07/01/2010 09:53 AM, Vadillo, Miguel wrote: >>> /* >>> + 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. So, you're saying this is all different in 2.6.35 (which I assume is the 'next release') ? That could be awhile. As far as I can tell from looking at the repositories at git://dev.omapzoom.org/pub/scm/integration you wrote these patches just for the Ubuntu branch. Since we may have to live with this 2.6.34 based kernel for a few weeks more, please take the time to write correct code and resend this patch. At least put some BUG_ON()s in it so we catch allocation failures. rtg
> -----Original Message----- > From: Tim Gardner [mailto:tim.gardner@canonical.com] > Sent: Friday, July 02, 2010 8:17 AM > To: Vadillo, Miguel > Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@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 09:53 AM, Vadillo, Miguel wrote: > > >>> /* > >>> + 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. > > So, you're saying this is all different in 2.6.35 (which I assume is the > 'next release') ? That could be awhile. > > As far as I can tell from looking at the repositories at > git://dev.omapzoom.org/pub/scm/integration you wrote these patches just > for the Ubuntu branch. Since we may have to live with this 2.6.34 based > kernel for a few weeks more, please take the time to write correct code > and resend this patch. At least put some BUG_ON()s in it so we catch > allocation failures. > Ok in that case I will rework the patches following your suggestions and resend them ASAP, probably next week. > rtg > -- > Tim Gardner tim.gardner@canonical.com Regards... Miguel Vadillo
Tim, Bryan, Please find attached the patch that solves the error with the return values in the ipu_pm_setup function. Regarding the semaphore and the static declaration both changes will be included in the next release. Please let us know what you think. As always your feedback is welcome. Regards... Miguel Vadillo Cel. 214-587-2910 > -----Original Message----- > From: Vadillo, Miguel > Sent: Friday, July 02, 2010 10:39 AM > To: 'tim.gardner@canonical.com' > Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@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 > > > -----Original Message----- > > From: Tim Gardner [mailto:tim.gardner@canonical.com] > > Sent: Friday, July 02, 2010 8:17 AM > > To: Vadillo, Miguel > > Cc: Bryan Wu; kernel-team@lists.ubuntu.com; ogra@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 09:53 AM, Vadillo, Miguel wrote: > > > > >>> /* > > >>> + 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. > > > > So, you're saying this is all different in 2.6.35 (which I assume is the > > 'next release') ? That could be awhile. > > > > As far as I can tell from looking at the repositories at > > git://dev.omapzoom.org/pub/scm/integration you wrote these patches just > > for the Ubuntu branch. Since we may have to live with this 2.6.34 based > > kernel for a few weeks more, please take the time to write correct code > > and resend this patch. At least put some BUG_ON()s in it so we catch > > allocation failures. > > > > > Ok in that case I will rework the patches following your suggestions and > resend them ASAP, probably next week. > > > rtg > > -- > > Tim Gardner tim.gardner@canonical.com > > Regards... > Miguel Vadillo
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; /** ============================================================================ * 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); + +/* + 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 */