| Message ID | 1314024454-14278-1-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hello. On 22-08-2011 18:47, m-karicheri2@ti.com wrote: > From: Murali Karicheri<m-karicheri2@ti.com> > Compared to initial version here are the changes:- > - Rebased to linux-davinci v3.0-rc7 master branch > - Added missing changes to psc files. This passage should follow the --- tear line. > In one of the new SoC that we are working on, there are multiple power > domains similar to that in C6670. Currently clock module assumes > that there are only two power domains (ARM and DSP). This patch is > added to allow porting of Linux on to the above SoC. > Boot tested this change on DM365 and OMAP L137 > Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> [...] > diff --git a/arch/arm/mach-davinci/psc.c b/arch/arm/mach-davinci/psc.c > index 1fb6bdf..2445050 100644 > --- a/arch/arm/mach-davinci/psc.c > +++ b/arch/arm/mach-davinci/psc.c [...] > @@ -80,10 +80,10 @@ void davinci_psc_config(unsigned int domain, unsigned int ctlr, > __raw_writel(mdctl, psc_base + MDCTL + 4 * id); > > pdstat = __raw_readl(psc_base + PDSTAT); > - if ((pdstat& 0x00000001) == 0) { > - pdctl1 = __raw_readl(psc_base + PDCTL1); > - pdctl1 |= 0x1; > - __raw_writel(pdctl1, psc_base + PDCTL1); > + if ((pdstat& (1<< domain)) == 0) { Hm, are you sure? I'm looking into the "TMS320DM644x DMSoC ARM Subsystem Reference Guide" and "OMAP-L137 Applications Processor System Reference Guide" and the PDSTAT.STATE field is in bits 0-4 there, and it's not laid out as bit per domain (PTCMD is laid out as bit per domain. Shouldn't we read PDSTAT0 for the ARM domain and PDSTAT1 for the DSP domain instead? I.e. the current code does not seem correct (why we always change PDCTL1 and PDCTL0 for ARM domain instead?). > + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain); > + pdctl |= 0x1; > + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain); This seems correct... WBR, Sergei
On 08/23/2011 02:47 PM, Sergei Shtylyov wrote: >> - Rebased to linux-davinci v3.0-rc7 master branch >> - Added missing changes to psc files. > This passage should follow the --- tear line. >> In one of the new SoC that we are working on, there are multiple power >> domains similar to that in C6670. Currently clock module assumes >> that there are only two power domains (ARM and DSP). This patch is >> added to allow porting of Linux on to the above SoC. >> Boot tested this change on DM365 and OMAP L137 >> Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> > [...] >> diff --git a/arch/arm/mach-davinci/psc.c b/arch/arm/mach-davinci/psc.c >> index 1fb6bdf..2445050 100644 >> --- a/arch/arm/mach-davinci/psc.c >> +++ b/arch/arm/mach-davinci/psc.c > [...] >> @@ -80,10 +80,10 @@ void davinci_psc_config(unsigned int domain, unsigned int >> ctlr, >> __raw_writel(mdctl, psc_base + MDCTL + 4 * id); >> >> pdstat = __raw_readl(psc_base + PDSTAT); >> - if ((pdstat & 0x00000001) == 0) { The current code doesn't make much sense to me -- Kevin, as the original author, could you explain why this cross-dependency between domains 0 and 1? The code below should never be executed in my opinion, since domain 0 is always-on, so 'pdstat & 1' should never be 0 -- although my DA830 manual says it's 0 by default even though PDSTAT0.NEXT is initially 1... I could understand if we had: if (domain) { instead if this check... And the bit mask certainly isn't right as it excludes the "intermediate state" bit 4. >> - pdctl1 = __raw_readl(psc_base + PDCTL1); >> - pdctl1 |= 0x1; >> - __raw_writel(pdctl1, psc_base + PDCTL1); >> + if ((pdstat& (1<< domain)) == 0) { > Hm, are you sure? I'm looking into the "TMS320DM644x DMSoC ARM Subsystem > Reference Guide" and "OMAP-L137 Applications Processor System Reference Guide" > and the PDSTAT.STATE field is in bits 0-4 there, and it's not laid out as bit > per domain (PTCMD is laid out as bit per domain. So, this part certainly isn't right. > Shouldn't we read PDSTAT0 for > the ARM domain and PDSTAT1 for the DSP domain instead? I.e. the current code > does not seem correct (why we always change PDCTL1 and PDCTL0 for ARM domain > instead?). It seems I missed 'not' before PDCTL0... Well, actually there's no need to touch PDCTL0 as domain 0 seems to always be always-on one. >> + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain); >> + pdctl |= 0x1; >> + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain); > This seems correct... ... and should switch on non always-on domains. Although I'm not sure about the following bit twiddling concerning the external power source... WBR, Sergei
Sergei, Thanks for your comments. See my response inline with a [MK] prefix. -----Original Message----- From: Sergei Shtylyov [mailto:sshtylyov@mvista.com] Sent: Tuesday, August 23, 2011 6:48 AM To: Karicheri, Muralidharan Cc: davinci-linux-open-source@linux.davincidsp.com; Nori, Sekhar Subject: Re: [PATCH v1] ARM: davinci: enhancement to support multiple power domains Hello. On 22-08-2011 18:47, m-karicheri2@ti.com wrote: > From: Murali Karicheri<m-karicheri2@ti.com> > Compared to initial version here are the changes:- > - Rebased to linux-davinci v3.0-rc7 master branch > - Added missing changes to psc files. This passage should follow the --- tear line. [MK] Will do in the next revision. > In one of the new SoC that we are working on, there are multiple power > domains similar to that in C6670. Currently clock module assumes > that there are only two power domains (ARM and DSP). This patch is > added to allow porting of Linux on to the above SoC. > Boot tested this change on DM365 and OMAP L137 > Signed-off-by: Murali Karicheri<m-karicheri2@ti.com> [...] > diff --git a/arch/arm/mach-davinci/psc.c b/arch/arm/mach-davinci/psc.c > index 1fb6bdf..2445050 100644 > --- a/arch/arm/mach-davinci/psc.c > +++ b/arch/arm/mach-davinci/psc.c [...] > @@ -80,10 +80,10 @@ void davinci_psc_config(unsigned int domain, unsigned int ctlr, > __raw_writel(mdctl, psc_base + MDCTL + 4 * id); > > pdstat = __raw_readl(psc_base + PDSTAT); > - if ((pdstat& 0x00000001) == 0) { > - pdctl1 = __raw_readl(psc_base + PDCTL1); > - pdctl1 |= 0x1; > - __raw_writel(pdctl1, psc_base + PDCTL1); > + if ((pdstat& (1<< domain)) == 0) { Hm, are you sure? I'm looking into the "TMS320DM644x DMSoC ARM Subsystem Reference Guide" and "OMAP-L137 Applications Processor System Reference Guide" and the PDSTAT.STATE field is in bits 0-4 there, and it's not laid out as bit per domain (PTCMD is laid out as bit per domain. Shouldn't we read PDSTAT0 for the ARM domain and PDSTAT1 for the DSP domain instead? I.e. the current code does not seem correct (why we always change PDCTL1 and PDCTL0 for ARM domain instead?). [MK] Good catch! The existing code uses bitmask 1 though 5 bits are allocated. For DM644x, value Can be 0 or 1 and hence will be fine. But I think we need to change the mask to 0x1F. Also the PDSTAT is available per domain, and not bit per domain (again you are correct). So should be changed to pdstat = __raw_readl(psc_base + PDSTAT + 4 * domain); > + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain); > + pdctl |= 0x1; > + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain); This seems correct... WBR, Sergei
Sergei, I had replied to your earlier comment. >> __raw_writel(mdctl, psc_base + MDCTL + 4 * id); >> >> pdstat = __raw_readl(psc_base + PDSTAT); >> - if ((pdstat & 0x00000001) == 0) { The current code doesn't make much sense to me -- Kevin, as the original author, could you explain why this cross-dependency between domains 0 and 1? The code below should never be executed in my opinion, since domain 0 is always-on, so 'pdstat & 1' should never be 0 -- although my DA830 manual says it's 0 by default even though PDSTAT0.NEXT is initially 1... I could understand if we had: if (domain) { instead if this check... And the bit mask certainly isn't right as it excludes the "intermediate state" bit 4. [MK] This will be corrected in my next revision of the patch. But would wait to hear response from Kevin as well. WBR, Sergei
Hello. On 25-08-2011 21:04, Karicheri, Muralidharan wrote: > I had replied to your earlier comment. Would be good if you fixed your mail agent to properly quote the messages that you reply to. I did that manually. >>>> __raw_writel(mdctl, psc_base + MDCTL + 4 * id); >>>> pdstat = __raw_readl(psc_base + PDSTAT); >>>> - if ((pdstat& 0x00000001) == 0) { >> The current code doesn't make much sense to me -- Kevin, as the original >> author, could you explain why this cross-dependency between domains 0 and 1? >> The code below should never be executed in my opinion, since domain 0 is >> always-on, so 'pdstat& 1' should never be 0 -- although my DA830 manual says >> it's 0 by default even though PDSTAT0.NEXT is initially 1... >> I could understand if we had: >> if (domain) { >> instead if this check... >> And the bit mask certainly isn't right as it excludes the "intermediate >> state" bit 4. > [MK] This will be corrected in my next revision of the patch. But would wait to > hear response from Kevin as well. The mask should be corrected by a separate patch. WBR, Sergei
diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c index 0086113..008772e 100644 --- a/arch/arm/mach-davinci/clock.c +++ b/arch/arm/mach-davinci/clock.c @@ -31,19 +31,12 @@ static LIST_HEAD(clocks); static DEFINE_MUTEX(clocks_mutex); static DEFINE_SPINLOCK(clockfw_lock); -static unsigned psc_domain(struct clk *clk) -{ - return (clk->flags & PSC_DSP) - ? DAVINCI_GPSC_DSPDOMAIN - : DAVINCI_GPSC_ARMDOMAIN; -} - static void __clk_enable(struct clk *clk) { if (clk->parent) __clk_enable(clk->parent); if (clk->usecount++ == 0 && (clk->flags & CLK_PSC)) - davinci_psc_config(psc_domain(clk), clk->gpsc, clk->lpsc, + davinci_psc_config(clk->domain, clk->gpsc, clk->lpsc, true, clk->flags); } @@ -53,7 +46,7 @@ static void __clk_disable(struct clk *clk) return; if (--clk->usecount == 0 && !(clk->flags & CLK_PLL) && (clk->flags & CLK_PSC)) - davinci_psc_config(psc_domain(clk), clk->gpsc, clk->lpsc, + davinci_psc_config(clk->domain, clk->gpsc, clk->lpsc, false, clk->flags); if (clk->parent) __clk_disable(clk->parent); @@ -237,7 +230,7 @@ static int __init clk_disable_unused(void) pr_debug("Clocks: disable unused %s\n", ck->name); - davinci_psc_config(psc_domain(ck), ck->gpsc, ck->lpsc, + davinci_psc_config(ck->domain, ck->gpsc, ck->lpsc, false, ck->flags); } spin_unlock_irq(&clockfw_lock); diff --git a/arch/arm/mach-davinci/clock.h b/arch/arm/mach-davinci/clock.h index a705f36..46f0f1b 100644 --- a/arch/arm/mach-davinci/clock.h +++ b/arch/arm/mach-davinci/clock.h @@ -93,6 +93,7 @@ struct clk { u8 usecount; u8 lpsc; u8 gpsc; + u8 domain; u32 flags; struct clk *parent; struct list_head children; /* list of children */ @@ -107,11 +108,10 @@ struct clk { /* Clock flags: SoC-specific flags start at BIT(16) */ #define ALWAYS_ENABLED BIT(1) #define CLK_PSC BIT(2) -#define PSC_DSP BIT(3) /* PSC uses DSP domain, not ARM */ -#define CLK_PLL BIT(4) /* PLL-derived clock */ -#define PRE_PLL BIT(5) /* source is before PLL mult/div */ -#define PSC_SWRSTDISABLE BIT(6) /* Disable state is SwRstDisable */ -#define PSC_FORCE BIT(7) /* Force module state transtition */ +#define CLK_PLL BIT(3) /* PLL-derived clock */ +#define PRE_PLL BIT(4) /* source is before PLL mult/div */ +#define PSC_SWRSTDISABLE BIT(5) /* Disable state is SwRstDisable */ +#define PSC_FORCE BIT(6) /* Force module state transtition */ #define CLK(dev, con, ck) \ { \ diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c index 555ff5b..f38c4bb 100644 --- a/arch/arm/mach-davinci/dm644x.c +++ b/arch/arm/mach-davinci/dm644x.c @@ -130,7 +130,7 @@ static struct clk dsp_clk = { .name = "dsp", .parent = &pll1_sysclk1, .lpsc = DAVINCI_LPSC_GEM, - .flags = PSC_DSP, + .domain = DAVINCI_GPSC_DSPDOMAIN, .usecount = 1, /* REVISIT how to disable? */ }; @@ -145,7 +145,7 @@ static struct clk vicp_clk = { .name = "vicp", .parent = &pll1_sysclk2, .lpsc = DAVINCI_LPSC_IMCOP, - .flags = PSC_DSP, + .domain = DAVINCI_GPSC_DSPDOMAIN, .usecount = 1, /* REVISIT how to disable? */ }; diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c index 552031e..5bec8b6 100644 --- a/arch/arm/mach-davinci/dm646x.c +++ b/arch/arm/mach-davinci/dm646x.c @@ -160,7 +160,7 @@ static struct clk dsp_clk = { .name = "dsp", .parent = &pll1_sysclk1, .lpsc = DM646X_LPSC_C64X_CPU, - .flags = PSC_DSP, + .domain = DAVINCI_GPSC_DSPDOMAIN, .usecount = 1, /* REVISIT how to disable? */ }; diff --git a/arch/arm/mach-davinci/include/mach/psc.h b/arch/arm/mach-davinci/include/mach/psc.h index 47fd0bc..63c4366 100644 --- a/arch/arm/mach-davinci/include/mach/psc.h +++ b/arch/arm/mach-davinci/include/mach/psc.h @@ -233,7 +233,7 @@ #define PTCMD 0x120 #define PTSTAT 0x128 #define PDSTAT 0x200 -#define PDCTL1 0x304 +#define PDCTL 0x300 #define MDSTAT 0x800 #define MDCTL 0xA00 diff --git a/arch/arm/mach-davinci/psc.c b/arch/arm/mach-davinci/psc.c index 1fb6bdf..2445050 100644 --- a/arch/arm/mach-davinci/psc.c +++ b/arch/arm/mach-davinci/psc.c @@ -52,7 +52,7 @@ int __init davinci_psc_is_clk_active(unsigned int ctlr, unsigned int id) void davinci_psc_config(unsigned int domain, unsigned int ctlr, unsigned int id, bool enable, u32 flags) { - u32 epcpr, ptcmd, ptstat, pdstat, pdctl1, mdstat, mdctl; + u32 epcpr, ptcmd, ptstat, pdstat, pdctl, mdstat, mdctl; void __iomem *psc_base; struct davinci_soc_info *soc_info = &davinci_soc_info; u32 next_state = PSC_STATE_ENABLE; @@ -80,10 +80,10 @@ void davinci_psc_config(unsigned int domain, unsigned int ctlr, __raw_writel(mdctl, psc_base + MDCTL + 4 * id); pdstat = __raw_readl(psc_base + PDSTAT); - if ((pdstat & 0x00000001) == 0) { - pdctl1 = __raw_readl(psc_base + PDCTL1); - pdctl1 |= 0x1; - __raw_writel(pdctl1, psc_base + PDCTL1); + if ((pdstat & (1 << domain)) == 0) { + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain); + pdctl |= 0x1; + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain); ptcmd = 1 << domain; __raw_writel(ptcmd, psc_base + PTCMD); @@ -92,9 +92,10 @@ void davinci_psc_config(unsigned int domain, unsigned int ctlr, epcpr = __raw_readl(psc_base + EPCPR); } while ((((epcpr >> domain) & 1) == 0)); - pdctl1 = __raw_readl(psc_base + PDCTL1); - pdctl1 |= 0x100; - __raw_writel(pdctl1, psc_base + PDCTL1); + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain); + pdctl |= 0x100; + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain); + } else { ptcmd = 1 << domain; __raw_writel(ptcmd, psc_base + PTCMD);