Hi Kevin/Paul, > -----Original Message----- > From: Paul Walmsley [mailto:paul@xxxxxxxxx] > Sent: Friday, February 11, 2011 7:36 AM > To: Rajendra Nayak > Cc: linux-omap@xxxxxxxxxxxxxxx; khilman@xxxxxx; b-cousson@xxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 3/3] OMAP4: clockdomain: Add wkup/sleep dependency support > > Hi > > On Mon, 7 Feb 2011, Rajendra Nayak wrote: > > > Add OMAP4 platform specific implementation to support clkdm > > wkup and sleep dependencies a.k.a static dependencies. > > > > Signed-off-by: Rajendra Nayak <rnayak@xxxxxx> > > This patch had a similar bug to the OMAP2/3 variants; updated patch > follows - Thanks for fixing this. I have one comment below... > > > - Paul > > From: Rajendra Nayak <rnayak@xxxxxx> > Date: Thu, 10 Feb 2011 18:58:04 -0700 > Subject: [PATCH] OMAP4: clockdomain: Add wkup/sleep dependency support > > Add OMAP4 platform specific implementation to support clkdm > wkup and sleep dependencies a.k.a static dependencies. > > Signed-off-by: Rajendra Nayak <rnayak@xxxxxx> > [paul@xxxxxxxxx: removed comment about PRM; zero-prefixed STATICDEP > register offset; fixed loop termination condition in > omap4_clkdm_clear_all_wkup_sleep_deps(); thanks to Kevin Hilman for finding > and helping fix this bug] > Cc: Kevin Hilman <khilman@xxxxxxxxxxxxxxxxxxx> > Signed-off-by: Paul Walmsley <paul@xxxxxxxxx> > --- > arch/arm/mach-omap2/clockdomain44xx.c | 58 +++++++++++++++++++++++++++++++++ > arch/arm/mach-omap2/cm44xx.h | 1 + > 2 files changed, 59 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/mach-omap2/clockdomain44xx.c b/arch/arm/mach-omap2/clockdomain44xx.c > index c0ccc47..e6fd8da 100644 > --- a/arch/arm/mach-omap2/clockdomain44xx.c > +++ b/arch/arm/mach-omap2/clockdomain44xx.c > @@ -12,8 +12,58 @@ > * published by the Free Software Foundation. > */ > > +#include <linux/kernel.h> > #include "clockdomain.h" > #include "cminst44xx.h" > +#include "cm44xx.h" > + > +static int omap4_clkdm_add_wkup_sleep_dep(struct clockdomain *clkdm1, > + struct clockdomain *clkdm2) > +{ > + omap4_cminst_set_inst_reg_bits((1 << clkdm2->dep_bit), > + clkdm1->prcm_partition, > + clkdm1->cm_inst, clkdm1->clkdm_offs + > + OMAP4_CM_STATICDEP); > + return 0; > +} > + > +static int omap4_clkdm_del_wkup_sleep_dep(struct clockdomain *clkdm1, > + struct clockdomain *clkdm2) > +{ > + omap4_cminst_clear_inst_reg_bits((1 << clkdm2->dep_bit), > + clkdm1->prcm_partition, > + clkdm1->cm_inst, clkdm1->clkdm_offs + > + OMAP4_CM_STATICDEP); > + return 0; > +} > + > +static int omap4_clkdm_read_wkup_sleep_dep(struct clockdomain *clkdm1, > + struct clockdomain *clkdm2) > +{ > + return omap4_cminst_read_inst_reg_bits(clkdm1->prcm_partition, > + clkdm1->cm_inst, clkdm1->clkdm_offs + > + OMAP4_CM_STATICDEP, > + (1 << clkdm2->dep_bit)); > +} > + > +static int omap4_clkdm_clear_all_wkup_sleep_deps(struct clockdomain *clkdm) > +{ > + struct clkdm_dep *cd; > + u32 mask = 0; > + > + for (cd = clkdm->wkdep_srcs; cd && cd->clkdm_name; cd++) { My initial version actually did have a check for cd->clkdm_name instead of cd->clkdm, and then I ran into aborts when a clkdm, though belonging to the right chip version, failed lookup (in clkdm_init) and left the cd->clkdm pointer NULL. This however was due to the fact that the clkdm_name populated was'nt matching the actual name, but that got me thinking if its better to look for populated pointers instead of names. I however seemed to have missed the point that there also could be a cd->clkdm populated as NULL for non supported CHIP rev's. > + if (!omap_chip_is(cd->omap_chip)) > + continue; > + Would it make sense to add an additional check here to avoid an abort in case of mismatches in clkdm_name populated and lookup's failing in clkdm_init? Something like... If (cd->clkdm) { |= 1 << cd->clkdm->dep_bit; atomic_set(&cd->wkdep_usecount, 0); } Regards Rajendra > + mask |= 1 << cd->clkdm->dep_bit; > + atomic_set(&cd->wkdep_usecount, 0); > + } > + > + omap4_cminst_clear_inst_reg_bits(mask, clkdm->prcm_partition, > + clkdm->cm_inst, clkdm->clkdm_offs + > + OMAP4_CM_STATICDEP); > + return 0; > +} > > static int omap4_clkdm_sleep(struct clockdomain *clkdm) > { > @@ -68,6 +118,14 @@ static int omap4_clkdm_clk_disable(struct clockdomain *clkdm) > } > > struct clkdm_ops omap4_clkdm_operations = { > + .clkdm_add_wkdep = omap4_clkdm_add_wkup_sleep_dep, > + .clkdm_del_wkdep = omap4_clkdm_del_wkup_sleep_dep, > + .clkdm_read_wkdep = omap4_clkdm_read_wkup_sleep_dep, > + .clkdm_clear_all_wkdeps = omap4_clkdm_clear_all_wkup_sleep_deps, > + .clkdm_add_sleepdep = omap4_clkdm_add_wkup_sleep_dep, > + .clkdm_del_sleepdep = omap4_clkdm_del_wkup_sleep_dep, > + .clkdm_read_sleepdep = omap4_clkdm_read_wkup_sleep_dep, > + .clkdm_clear_all_sleepdeps = omap4_clkdm_clear_all_wkup_sleep_deps, > .clkdm_sleep = omap4_clkdm_sleep, > .clkdm_wakeup = omap4_clkdm_wakeup, > .clkdm_allow_idle = omap4_clkdm_allow_idle, > diff --git a/arch/arm/mach-omap2/cm44xx.h b/arch/arm/mach-omap2/cm44xx.h > index 48fc3f4..0b87ec8 100644 > --- a/arch/arm/mach-omap2/cm44xx.h > +++ b/arch/arm/mach-omap2/cm44xx.h > @@ -21,6 +21,7 @@ > #include "cm.h" > > #define OMAP4_CM_CLKSTCTRL 0x0000 > +#define OMAP4_CM_STATICDEP 0x0004 > > /* Function prototypes */ > # ifndef __ASSEMBLER__ > -- > 1.7.2.3 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
