RE: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle
>-Original Message- >From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com] >Sent: 20 January, 2010 02:48 >To: Kristo Tero (Nokia-D/Tampere) >Cc: linux-omap@vger.kernel.org >Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for >checking if pwrdm/clkdm can idle > > writes: > >> >> >>>-Original Message- >>>From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com] >>>Sent: 18 January, 2010 20:41 >>>To: Kristo Tero (Nokia-D/Tampere) >>>Cc: linux-omap@vger.kernel.org >>>Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for >>>checking if pwrdm/clkdm can idle >>> >>>Tero Kristo writes: >>> >>>> From: Tero Kristo >>>> >>>> pwrdm_can_idle(pwrdm) will check if the specified >>>powerdomain can enter >>>> idle. This is done by checking all clockdomains under the >powerdomain >>>> if they can idle also. >>>> >>>> omap2_clkdm_can_idle(clkdm) will check if the specified >>>clockdomain can >>>> enter idle. This checks the functional clocks and idle >>>status bits of the >>>> domain according to following rules: >>>> 1) get inverse of idlest and mask against idle_def.mask >>>> * this gives a bitmask with non-idle bits high >>>> 2) bitwise OR active functional clocks from the domain to >the result >>>> * in some cases FCLK can be active but idlest still shows >>>module in idle >>>> 3) disable bits defined in idle_def.mask >>> >>>disable? it looks like they're just being ignored. >> >> Oh yes, just a small typo here in patch description. This >should indeed say that they are ignored, as the bit-field also >is named as ignore. >> >>> >>>> * some bits should be ignored, like UART clocks which are >>>dynamically >>>> switched inside sleep loop >>>> >>>> These calls can be used e.g. inside cpuidle to decide which >>>power states >>>> core and mpu should enter during idle, as there are certain >>>dependencies >>>> between wakeup capabilities and reset logic. >>>> >>>> Signed-off-by: Tero Kristo >>> >>>Thanks for adding the IDLEST checks, I think that will help. >> >> Yes, and this is actually required for the secure side clock status >> handling, this information is not available anywhere else, thus it >> is impossible to get the idle status from the clock framework >> usecounts. >> >>> >>>In the .mask field, instead of using a hard-coded mask, >probably using >>>the existing bitfields headers would be a bit more readable, and help >>>ensure correctness. >> >> Well, the bitfield definitions for those do not exist at the >> moment. > >We could use the EN bit definitions for the CM_FCLKEN* since they're >the same for IDLEST. Some bits do not exist as EN bit definitions, but they do exist as ST. E.g secure clocks do not have EN bit, and some CORE domain EN bits are not definied either as they do not exist. I will update the masks in the code according to this, and just use ST bits. > >> The idea of the mask is just to get all bits in that are >> needed, e.g. PER domain does not have GPIO bits in the mask, as >> these appear to always be in a state that they are accessible >> (i.e. IDLEST shows accessible for them.) > >And I think if we used the #defines here, it would be much clearer >what bits are included and which are left out. > >Some comments as to which ones are needed or not would be helpful too. Ok, I'll inline the comments with the masks. > >> I can write all IDLEST bits into the masks (as was done before) if >> you like this approach better. This will generate rather long >> definitions though, and the probability of errors in those is >> actually higher than in this version (I just write a number of low >> bits to one.) > >I guess you didn't see the rest of my comments further down in the >original review. There was at least one error already in the SGX >since it's bit is not bit 0 but but 1. Using the existing #defines >would've solved that. True, I will update the masks according to this. > >On that note, please check out the other inline comments in the >original review. > >I should've mentioned that there were further comments inline in the >original review. Sorry for the confusion. Yeah, sorry missed those, will comment now. > >Kevin > >>> >>&g
Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle
writes: > > >>-Original Message- >>From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com] >>Sent: 18 January, 2010 20:41 >>To: Kristo Tero (Nokia-D/Tampere) >>Cc: linux-omap@vger.kernel.org >>Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for >>checking if pwrdm/clkdm can idle >> >>Tero Kristo writes: >> >>> From: Tero Kristo >>> >>> pwrdm_can_idle(pwrdm) will check if the specified >>powerdomain can enter >>> idle. This is done by checking all clockdomains under the powerdomain >>> if they can idle also. >>> >>> omap2_clkdm_can_idle(clkdm) will check if the specified >>clockdomain can >>> enter idle. This checks the functional clocks and idle >>status bits of the >>> domain according to following rules: >>> 1) get inverse of idlest and mask against idle_def.mask >>> * this gives a bitmask with non-idle bits high >>> 2) bitwise OR active functional clocks from the domain to the result >>> * in some cases FCLK can be active but idlest still shows >>module in idle >>> 3) disable bits defined in idle_def.mask >> >>disable? it looks like they're just being ignored. > > Oh yes, just a small typo here in patch description. This should indeed say > that they are ignored, as the bit-field also is named as ignore. > >> >>> * some bits should be ignored, like UART clocks which are >>dynamically >>> switched inside sleep loop >>> >>> These calls can be used e.g. inside cpuidle to decide which >>power states >>> core and mpu should enter during idle, as there are certain >>dependencies >>> between wakeup capabilities and reset logic. >>> >>> Signed-off-by: Tero Kristo >> >>Thanks for adding the IDLEST checks, I think that will help. > > Yes, and this is actually required for the secure side clock status > handling, this information is not available anywhere else, thus it > is impossible to get the idle status from the clock framework > usecounts. > >> >>In the .mask field, instead of using a hard-coded mask, probably using >>the existing bitfields headers would be a bit more readable, and help >>ensure correctness. > > Well, the bitfield definitions for those do not exist at the > moment. We could use the EN bit definitions for the CM_FCLKEN* since they're the same for IDLEST. > The idea of the mask is just to get all bits in that are > needed, e.g. PER domain does not have GPIO bits in the mask, as > these appear to always be in a state that they are accessible > (i.e. IDLEST shows accessible for them.) And I think if we used the #defines here, it would be much clearer what bits are included and which are left out. Some comments as to which ones are needed or not would be helpful too. > I can write all IDLEST bits into the masks (as was done before) if > you like this approach better. This will generate rather long > definitions though, and the probability of errors in those is > actually higher than in this version (I just write a number of low > bits to one.) I guess you didn't see the rest of my comments further down in the original review. There was at least one error already in the SGX since it's bit is not bit 0 but but 1. Using the existing #defines would've solved that. On that note, please check out the other inline comments in the original review. I should've mentioned that there were further comments inline in the original review. Sorry for the confusion. Kevin >> >>Hopefully there's a way to auto-generate these for OMAP4. >> >>> --- >>> arch/arm/mach-omap2/clockdomain.c | 27 >>> arch/arm/mach-omap2/clockdomains.h| 54 >>+ >>> arch/arm/mach-omap2/powerdomain.c | 25 +++ >>> arch/arm/plat-omap/include/plat/clockdomain.h | 17 >>> arch/arm/plat-omap/include/plat/powerdomain.h |1 + >>> 5 files changed, 124 insertions(+), 0 deletions(-) >>> >>> diff --git a/arch/arm/mach-omap2/clockdomain.c >>b/arch/arm/mach-omap2/clockdomain.c >>> index dd285f0..f42111a 100644 >>> --- a/arch/arm/mach-omap2/clockdomain.c >>> +++ b/arch/arm/mach-omap2/clockdomain.c >>> @@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct >>clockdomain *clkdm) >>> return 0; >>> } >>> >>> + >>> +/** >>> + * omap2_clkdm_can_idle - check if clockdomain has any >>active clocks or not >>> + *
RE: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle
>-Original Message-
>From: ext Kevin Hilman [mailto:khil...@deeprootsystems.com]
>Sent: 18 January, 2010 20:41
>To: Kristo Tero (Nokia-D/Tampere)
>Cc: linux-omap@vger.kernel.org
>Subject: Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for
>checking if pwrdm/clkdm can idle
>
>Tero Kristo writes:
>
>> From: Tero Kristo
>>
>> pwrdm_can_idle(pwrdm) will check if the specified
>powerdomain can enter
>> idle. This is done by checking all clockdomains under the powerdomain
>> if they can idle also.
>>
>> omap2_clkdm_can_idle(clkdm) will check if the specified
>clockdomain can
>> enter idle. This checks the functional clocks and idle
>status bits of the
>> domain according to following rules:
>> 1) get inverse of idlest and mask against idle_def.mask
>> * this gives a bitmask with non-idle bits high
>> 2) bitwise OR active functional clocks from the domain to the result
>> * in some cases FCLK can be active but idlest still shows
>module in idle
>> 3) disable bits defined in idle_def.mask
>
>disable? it looks like they're just being ignored.
Oh yes, just a small typo here in patch description. This should indeed say
that they are ignored, as the bit-field also is named as ignore.
>
>> * some bits should be ignored, like UART clocks which are
>dynamically
>> switched inside sleep loop
>>
>> These calls can be used e.g. inside cpuidle to decide which
>power states
>> core and mpu should enter during idle, as there are certain
>dependencies
>> between wakeup capabilities and reset logic.
>>
>> Signed-off-by: Tero Kristo
>
>Thanks for adding the IDLEST checks, I think that will help.
Yes, and this is actually required for the secure side clock status handling,
this information is not available anywhere else, thus it is impossible to get
the idle status from the clock framework usecounts.
>
>In the .mask field, instead of using a hard-coded mask, probably using
>the existing bitfields headers would be a bit more readable, and help
>ensure correctness.
Well, the bitfield definitions for those do not exist at the moment. The idea
of the mask is just to get all bits in that are needed, e.g. PER domain does
not have GPIO bits in the mask, as these appear to always be in a state that
they are accessible (i.e. IDLEST shows accessible for them.)
I can write all IDLEST bits into the masks (as was done before) if you like
this approach better. This will generate rather long definitions though, and
the probability of errors in those is actually higher than in this version (I
just write a number of low bits to one.)
>
>Hopefully there's a way to auto-generate these for OMAP4.
>
>> ---
>> arch/arm/mach-omap2/clockdomain.c | 27
>> arch/arm/mach-omap2/clockdomains.h| 54
>+
>> arch/arm/mach-omap2/powerdomain.c | 25 +++
>> arch/arm/plat-omap/include/plat/clockdomain.h | 17
>> arch/arm/plat-omap/include/plat/powerdomain.h |1 +
>> 5 files changed, 124 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/clockdomain.c
>b/arch/arm/mach-omap2/clockdomain.c
>> index dd285f0..f42111a 100644
>> --- a/arch/arm/mach-omap2/clockdomain.c
>> +++ b/arch/arm/mach-omap2/clockdomain.c
>> @@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct
>clockdomain *clkdm)
>> return 0;
>> }
>>
>> +
>> +/**
>> + * omap2_clkdm_can_idle - check if clockdomain has any
>active clocks or not
>> + * @clkdm: struct clockdomain *
>> + *
>> + * Checks if the clockdomain has any active clock or not,
>i.e. whether it
>> + * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if
>unable to idle;
>> + * 1 if can idle.
>> + */
>> +int omap2_clkdm_can_idle(struct clockdomain *clkdm)
>> +{
>> +int i;
>> +
>> +if (!clkdm)
>> +return -EINVAL;
>> +
>> +for (i = 0; i < clkdm->clk_reg_amt; i++)
>> +if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>> +CM_IDLEST + 4 * i) &
>> +clkdm->idle_def[i].mask) |
>> +
>cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
>> +CM_FCLKEN + 4 * i)) &
>> +~clkdm->idle_def[i].ignore)
>
>This has some readability/indent issues.
>
>Also, could check fclk first, if active fclks, no reason to check
>idlest. How about something like this (untested, but I *think* I kept
>the sa
Re: [PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle
Tero Kristo writes:
> From: Tero Kristo
>
> pwrdm_can_idle(pwrdm) will check if the specified powerdomain can enter
> idle. This is done by checking all clockdomains under the powerdomain
> if they can idle also.
>
> omap2_clkdm_can_idle(clkdm) will check if the specified clockdomain can
> enter idle. This checks the functional clocks and idle status bits of the
> domain according to following rules:
> 1) get inverse of idlest and mask against idle_def.mask
> * this gives a bitmask with non-idle bits high
> 2) bitwise OR active functional clocks from the domain to the result
> * in some cases FCLK can be active but idlest still shows module in idle
> 3) disable bits defined in idle_def.mask
disable? it looks like they're just being ignored.
> * some bits should be ignored, like UART clocks which are dynamically
> switched inside sleep loop
>
> These calls can be used e.g. inside cpuidle to decide which power states
> core and mpu should enter during idle, as there are certain dependencies
> between wakeup capabilities and reset logic.
>
> Signed-off-by: Tero Kristo
Thanks for adding the IDLEST checks, I think that will help.
In the .mask field, instead of using a hard-coded mask, probably using
the existing bitfields headers would be a bit more readable, and help
ensure correctness.
Hopefully there's a way to auto-generate these for OMAP4.
> ---
> arch/arm/mach-omap2/clockdomain.c | 27
> arch/arm/mach-omap2/clockdomains.h| 54
> +
> arch/arm/mach-omap2/powerdomain.c | 25 +++
> arch/arm/plat-omap/include/plat/clockdomain.h | 17
> arch/arm/plat-omap/include/plat/powerdomain.h |1 +
> 5 files changed, 124 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clockdomain.c
> b/arch/arm/mach-omap2/clockdomain.c
> index dd285f0..f42111a 100644
> --- a/arch/arm/mach-omap2/clockdomain.c
> +++ b/arch/arm/mach-omap2/clockdomain.c
> @@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct clockdomain *clkdm)
> return 0;
> }
>
> +
> +/**
> + * omap2_clkdm_can_idle - check if clockdomain has any active clocks or not
> + * @clkdm: struct clockdomain *
> + *
> + * Checks if the clockdomain has any active clock or not, i.e. whether it
> + * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if unable to idle;
> + * 1 if can idle.
> + */
> +int omap2_clkdm_can_idle(struct clockdomain *clkdm)
> +{
> + int i;
> +
> + if (!clkdm)
> + return -EINVAL;
> +
> + for (i = 0; i < clkdm->clk_reg_amt; i++)
> + if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
> + CM_IDLEST + 4 * i) &
> + clkdm->idle_def[i].mask) |
> + cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
> + CM_FCLKEN + 4 * i)) &
> + ~clkdm->idle_def[i].ignore)
This has some readability/indent issues.
Also, could check fclk first, if active fclks, no reason to check
idlest. How about something like this (untested, but I *think* I kept
the same logic):
for (i = 0; i < clkdm->clk_reg_amt; i++) {
u32 idlest, fclk;
fclk = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
CM_FCLKEN + 4 * i);
if (fclk & ~clkdm->idle_def[i].ignore)
return 0;
idlest = cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
CM_IDLEST + 4 * i);
if (~idlest & clkdm->idle_def[i].mask)
return 0;
}
> + return 0;
> + return 1;
> +}
> +
> /**
> * omap2_clkdm_allow_idle - enable hwsup idle transitions for clkdm
> * @clkdm: struct clockdomain *
> diff --git a/arch/arm/mach-omap2/clockdomains.h
> b/arch/arm/mach-omap2/clockdomains.h
> index c4ee076..2c1f2eb 100644
> --- a/arch/arm/mach-omap2/clockdomains.h
> +++ b/arch/arm/mach-omap2/clockdomains.h
> @@ -167,6 +167,12 @@ static struct clockdomain iva2_clkdm = {
> .flags = CLKDM_CAN_HWSUP_SWSUP,
> .clktrctrl_mask = OMAP3430_CLKTRCTRL_IVA2_MASK,
> .omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
> + .clk_reg_amt= 1,
> + .idle_def = {
> + [0] = {
> + .mask = 0x1,
Use OMAP3430_CM_FCLKEN_IVA2_EN_IVA2_MASK
> + },
> + },
> };
>
> static struct clockdomain gfx_3430es1_clkdm = {
> @@ -183,6 +189,12 @@ static struct clockdomain sgx_clkdm = {
> .flags = CLKDM_CAN_HWSUP_SWSUP,
> .clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
> .omap_chip = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
> + .clk_reg_amt= 1,
> + .idle_def = {
> + [0] = {
> + .mask = 0x1,
Use OMAP3430ES2_CM_FCLKEN_SGX_EN_SGX_MASK
And here's a a good reason for using the pre-
[PATCHv3 5/6] OMAP: Powerdomains: Add support for checking if pwrdm/clkdm can idle
From: Tero Kristo
pwrdm_can_idle(pwrdm) will check if the specified powerdomain can enter
idle. This is done by checking all clockdomains under the powerdomain
if they can idle also.
omap2_clkdm_can_idle(clkdm) will check if the specified clockdomain can
enter idle. This checks the functional clocks and idle status bits of the
domain according to following rules:
1) get inverse of idlest and mask against idle_def.mask
* this gives a bitmask with non-idle bits high
2) bitwise OR active functional clocks from the domain to the result
* in some cases FCLK can be active but idlest still shows module in idle
3) disable bits defined in idle_def.mask
* some bits should be ignored, like UART clocks which are dynamically
switched inside sleep loop
These calls can be used e.g. inside cpuidle to decide which power states
core and mpu should enter during idle, as there are certain dependencies
between wakeup capabilities and reset logic.
Signed-off-by: Tero Kristo
---
arch/arm/mach-omap2/clockdomain.c | 27
arch/arm/mach-omap2/clockdomains.h| 54 +
arch/arm/mach-omap2/powerdomain.c | 25 +++
arch/arm/plat-omap/include/plat/clockdomain.h | 17
arch/arm/plat-omap/include/plat/powerdomain.h |1 +
5 files changed, 124 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-omap2/clockdomain.c
b/arch/arm/mach-omap2/clockdomain.c
index dd285f0..f42111a 100644
--- a/arch/arm/mach-omap2/clockdomain.c
+++ b/arch/arm/mach-omap2/clockdomain.c
@@ -472,6 +472,33 @@ int omap2_clkdm_wakeup(struct clockdomain *clkdm)
return 0;
}
+
+/**
+ * omap2_clkdm_can_idle - check if clockdomain has any active clocks or not
+ * @clkdm: struct clockdomain *
+ *
+ * Checks if the clockdomain has any active clock or not, i.e. whether it
+ * can enter idle. Returns -EINVAL if clkdm is NULL; 0 if unable to idle;
+ * 1 if can idle.
+ */
+int omap2_clkdm_can_idle(struct clockdomain *clkdm)
+{
+ int i;
+
+ if (!clkdm)
+ return -EINVAL;
+
+ for (i = 0; i < clkdm->clk_reg_amt; i++)
+ if (((~cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
+ CM_IDLEST + 4 * i) &
+ clkdm->idle_def[i].mask) |
+ cm_read_mod_reg(clkdm->pwrdm.ptr->prcm_offs,
+ CM_FCLKEN + 4 * i)) &
+ ~clkdm->idle_def[i].ignore)
+ return 0;
+ return 1;
+}
+
/**
* omap2_clkdm_allow_idle - enable hwsup idle transitions for clkdm
* @clkdm: struct clockdomain *
diff --git a/arch/arm/mach-omap2/clockdomains.h
b/arch/arm/mach-omap2/clockdomains.h
index c4ee076..2c1f2eb 100644
--- a/arch/arm/mach-omap2/clockdomains.h
+++ b/arch/arm/mach-omap2/clockdomains.h
@@ -167,6 +167,12 @@ static struct clockdomain iva2_clkdm = {
.flags = CLKDM_CAN_HWSUP_SWSUP,
.clktrctrl_mask = OMAP3430_CLKTRCTRL_IVA2_MASK,
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+ .clk_reg_amt= 1,
+ .idle_def = {
+ [0] = {
+ .mask = 0x1,
+ },
+ },
};
static struct clockdomain gfx_3430es1_clkdm = {
@@ -183,6 +189,12 @@ static struct clockdomain sgx_clkdm = {
.flags = CLKDM_CAN_HWSUP_SWSUP,
.clktrctrl_mask = OMAP3430ES2_CLKTRCTRL_SGX_MASK,
.omap_chip = OMAP_CHIP_INIT(CHIP_GE_OMAP3430ES2),
+ .clk_reg_amt= 1,
+ .idle_def = {
+ [0] = {
+ .mask = 0x1,
+ },
+ },
};
/*
@@ -206,6 +218,23 @@ static struct clockdomain core_l3_34xx_clkdm = {
.flags = CLKDM_CAN_HWSUP,
.clktrctrl_mask = OMAP3430_CLKTRCTRL_L3_MASK,
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+ .clk_reg_amt= 3,
+ .idle_def = {
+ [0] = {
+ .ignore = OMAP3430_ST_UART2_MASK |
+ OMAP3430_ST_UART1_MASK |
+ OMAP3430_ST_SDRC_MASK |
+ OMAP3430_ST_OMAPCTRL_MASK |
+ OMAP3430ES2_ST_HSOTGUSB_IDLE_MASK,
+ .mask = 0x,
+ },
+ [1] = {
+ .mask = 0x1f,
+ },
+ [2] = {
+ .mask = 0x4,
+ },
+ },
};
static struct clockdomain core_l4_34xx_clkdm = {
@@ -222,6 +251,12 @@ static struct clockdomain dss_34xx_clkdm = {
.flags = CLKDM_CAN_HWSUP_SWSUP,
.clktrctrl_mask = OMAP3430_CLKTRCTRL_DSS_MASK,
.omap_chip = OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
+ .clk_reg_amt= 1,
+ .idle_def = {
+ [0] = {
+ .mask = 0x1,
+ },
+ },
};
static struct clockdomain cam_clk
