diff mbox

[2/8] ARM: tegra: config the polarity of the request of sys clock

Message ID 1374830110-30685-3-git-send-email-josephl@nvidia.com (mailing list archive)
State New, archived
Headers show

Commit Message

Joseph Lo July 26, 2013, 9:15 a.m. UTC
When suspending to LP1 mode, the SYSCLK will be clock gated. And different
board may have different polarity of the request of SYSCLK, this patch
configure the polarity from the DT for the board.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/pmc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Stephen Warren July 29, 2013, 10:47 p.m. UTC | #1
On 07/26/2013 03:15 AM, Joseph Lo wrote:
> When suspending to LP1 mode, the SYSCLK will be clock gated. And different
> board may have different polarity of the request of SYSCLK, this patch
> configure the polarity from the DT for the board.

> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c

> @@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
>  	reg = tegra_pmc_readl(PMC_CTRL);
>  	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
>  	tegra_pmc_writel(reg, PMC_CTRL);
> +
> +	reg = tegra_pmc_readl(PMC_CTRL);
> +
> +	if (!pmc_pm_data.sysclkreq_high)
> +		reg |= TEGRA_POWER_SYSCLK_POLARITY;
> +	else
> +		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
> +
> +	/* configure the output inverts while the request is tristated */
> +	tegra_pmc_writel(reg, PMC_CTRL);

I think s/inverts/polarity/ in that comment would make a lot more sense.

Must _OE be disabled for the code to work correctly? If so, should the
code explicitly clear _OE first, since who knows what state it was
originally in? Can't we just set the new polarity and enable _OE in a
single register write?

> +	/* now enable the request */
> +	reg |= TEGRA_POWER_SYSCLK_OE;
> +	tegra_pmc_writel(reg, PMC_CTRL);
Joseph Lo Aug. 2, 2013, 7:48 a.m. UTC | #2
On Tue, 2013-07-30 at 06:47 +0800, Stephen Warren wrote:
> On 07/26/2013 03:15 AM, Joseph Lo wrote:
> > When suspending to LP1 mode, the SYSCLK will be clock gated. And different
> > board may have different polarity of the request of SYSCLK, this patch
> > configure the polarity from the DT for the board.
> 
> > diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> 
> > @@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
> >  	reg = tegra_pmc_readl(PMC_CTRL);
> >  	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
> >  	tegra_pmc_writel(reg, PMC_CTRL);
> > +
> > +	reg = tegra_pmc_readl(PMC_CTRL);
> > +
> > +	if (!pmc_pm_data.sysclkreq_high)
> > +		reg |= TEGRA_POWER_SYSCLK_POLARITY;
> > +	else
> > +		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
> > +
> > +	/* configure the output inverts while the request is tristated */
> > +	tegra_pmc_writel(reg, PMC_CTRL);
> 
> I think s/inverts/polarity/ in that comment would make a lot more sense.
> 
Yes, thanks.

> Must _OE be disabled for the code to work correctly? If so, should the
> code explicitly clear _OE first, since who knows what state it was
> originally in? Can't we just set the new polarity and enable _OE in a
> single register write?
> 
The SYSCLK is super clock that was connected to COP subsystem. It can't
be disabled when system is running. The boot loader had initialized it
and brought it to kernel. We follow the HW description in DT of the
polarity of SCLK to re-configure and re-init again. Then the PMC can
clock gate it when system go into suspend state.

And it can't be set it up by a single register write. It's a HW control
sequence. (Actually lots of PMC code have similar sequence.)

> > +	/* now enable the request */
> > +	reg |= TEGRA_POWER_SYSCLK_OE;
> > +	tegra_pmc_writel(reg, PMC_CTRL);
>
Stephen Warren Aug. 2, 2013, 8:24 p.m. UTC | #3
On 08/02/2013 01:48 AM, Joseph Lo wrote:
> On Tue, 2013-07-30 at 06:47 +0800, Stephen Warren wrote:
>> On 07/26/2013 03:15 AM, Joseph Lo wrote:
>>> When suspending to LP1 mode, the SYSCLK will be clock gated. And different
>>> board may have different polarity of the request of SYSCLK, this patch
>>> configure the polarity from the DT for the board.
>>
>>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
>>
>>> @@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
>>>  	reg = tegra_pmc_readl(PMC_CTRL);
>>>  	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
>>>  	tegra_pmc_writel(reg, PMC_CTRL);
>>> +
>>> +	reg = tegra_pmc_readl(PMC_CTRL);
>>> +
>>> +	if (!pmc_pm_data.sysclkreq_high)
>>> +		reg |= TEGRA_POWER_SYSCLK_POLARITY;
>>> +	else
>>> +		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
>>> +
>>> +	/* configure the output inverts while the request is tristated */
>>> +	tegra_pmc_writel(reg, PMC_CTRL);
>>
>> I think s/inverts/polarity/ in that comment would make a lot more sense.
>>
> Yes, thanks.
> 
>> Must _OE be disabled for the code to work correctly? If so, should the
>> code explicitly clear _OE first, since who knows what state it was
>> originally in? Can't we just set the new polarity and enable _OE in a
>> single register write?
>>
> The SYSCLK is super clock that was connected to COP subsystem. It can't
> be disabled when system is running. The boot loader had initialized it
> and brought it to kernel. We follow the HW description in DT of the
> polarity of SCLK to re-configure and re-init again. Then the PMC can
> clock gate it when system go into suspend state.

So it sounds like the bootloader has already configured the clock to a
certain polarity. If so, why do we need to reconfigure it again?

Or, is this code not duplicating something the bootloader must have
done, but simply informing some HW in the PMC that's receiving SYSCLK
how the clock is configured elsewhere?
Joseph Lo Aug. 5, 2013, 8:42 a.m. UTC | #4
On Sat, 2013-08-03 at 04:24 +0800, Stephen Warren wrote:
> On 08/02/2013 01:48 AM, Joseph Lo wrote:
> > On Tue, 2013-07-30 at 06:47 +0800, Stephen Warren wrote:
> >> On 07/26/2013 03:15 AM, Joseph Lo wrote:
> >>> When suspending to LP1 mode, the SYSCLK will be clock gated. And different
> >>> board may have different polarity of the request of SYSCLK, this patch
> >>> configure the polarity from the DT for the board.
> >>
> >>> diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
> >>
> >>> @@ -238,6 +240,20 @@ void tegra_pmc_suspend_init(void)
> >>>  	reg = tegra_pmc_readl(PMC_CTRL);
> >>>  	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
> >>>  	tegra_pmc_writel(reg, PMC_CTRL);
> >>> +
> >>> +	reg = tegra_pmc_readl(PMC_CTRL);
> >>> +
> >>> +	if (!pmc_pm_data.sysclkreq_high)
> >>> +		reg |= TEGRA_POWER_SYSCLK_POLARITY;
> >>> +	else
> >>> +		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
> >>> +
> >>> +	/* configure the output inverts while the request is tristated */
> >>> +	tegra_pmc_writel(reg, PMC_CTRL);
> >>
> >> I think s/inverts/polarity/ in that comment would make a lot more sense.
> >>
> > Yes, thanks.
> > 
> >> Must _OE be disabled for the code to work correctly? If so, should the
> >> code explicitly clear _OE first, since who knows what state it was
> >> originally in? Can't we just set the new polarity and enable _OE in a
> >> single register write?
> >>
> > The SYSCLK is super clock that was connected to COP subsystem. It can't
> > be disabled when system is running. The boot loader had initialized it
> > and brought it to kernel. We follow the HW description in DT of the
> > polarity of SCLK to re-configure and re-init again. Then the PMC can
> > clock gate it when system go into suspend state.
> 
> So it sounds like the bootloader has already configured the clock to a
> certain polarity. If so, why do we need to reconfigure it again?
> 
> Or, is this code not duplicating something the bootloader must have
> done, but simply informing some HW in the PMC that's receiving SYSCLK
> how the clock is configured elsewhere?

I can't always guarantee the bootloader does the stuffs that kernel
needs. For this reason, it's better to do what the kernel needs in the
kernel.
diff mbox

Patch

diff --git a/arch/arm/mach-tegra/pmc.c b/arch/arm/mach-tegra/pmc.c
index 8345fcd..018bc87 100644
--- a/arch/arm/mach-tegra/pmc.c
+++ b/arch/arm/mach-tegra/pmc.c
@@ -27,6 +27,8 @@ 
 #include "pmc.h"
 #include "sleep.h"
 
+#define TEGRA_POWER_SYSCLK_POLARITY	(1 << 10)  /* sys clk polarity */
+#define TEGRA_POWER_SYSCLK_OE		(1 << 11)  /* system clock enable */
 #define TEGRA_POWER_EFFECT_LP0		(1 << 14)  /* LP0 when CPU pwr gated */
 #define TEGRA_POWER_CPU_PWRREQ_POLARITY	(1 << 15)  /* CPU pwr req polarity */
 #define TEGRA_POWER_CPU_PWRREQ_OE	(1 << 16)  /* CPU pwr req enable */
@@ -238,6 +240,20 @@  void tegra_pmc_suspend_init(void)
 	reg = tegra_pmc_readl(PMC_CTRL);
 	reg |= TEGRA_POWER_CPU_PWRREQ_OE;
 	tegra_pmc_writel(reg, PMC_CTRL);
+
+	reg = tegra_pmc_readl(PMC_CTRL);
+
+	if (!pmc_pm_data.sysclkreq_high)
+		reg |= TEGRA_POWER_SYSCLK_POLARITY;
+	else
+		reg &= ~TEGRA_POWER_SYSCLK_POLARITY;
+
+	/* configure the output inverts while the request is tristated */
+	tegra_pmc_writel(reg, PMC_CTRL);
+
+	/* now enable the request */
+	reg |= TEGRA_POWER_SYSCLK_OE;
+	tegra_pmc_writel(reg, PMC_CTRL);
 }
 #endif