[PATCH 3/5] SYSLINK:IPU-PM: solve circular dependencies ipu_pm
Vadillo, Miguel
vadillo at ti.com
Thu Jul 1 15:53:39 UTC 2010
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.
>
> > /**
> ==========================================================================
> ==
> > * 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 at canonical.com
Thanks a lot for the feedback
Regards...
Miguel Vadillo
Cel. 214-587-2910
More information about the kernel-team
mailing list