* Re: [PATCHv2] omap:mailbox: resolve hang issue
From: Guzman Lugo, Fernando @ 2011-03-03 2:51 UTC (permalink / raw)
To: Tony Lindgren
Cc: Armando Uribe, hiroshi.doyu, ohad, Russell King, linux-omap,
linux-arm-kernel, linux-kernel, Hari Kanigeri
In-Reply-To: <20110303014416.GL20560@atomide.com>
On Wed, Mar 2, 2011 at 7:44 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Armando Uribe <x0095078@ti.com> [110302 13:54]:
>> From: Hari Kanigeri <h-kanigeri2@ti.com>
>>
>> omap4 interrupt disable bits is different. On rx kfifo full, the mbox rx
>> interrupts wasn't getting disabled, and this is causing the rcm stress tests
>> to hang.
>>
>> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
>> Signed-off-by: Armando Uribe <x0095078@ti.com>
>> Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
>
> Should we merge this as a fix for the 2.6.38 still?
yeah, if it can still be merged in 2.6.38 because it is a fix it would be great.
Regards,
Fernando.
>
> Tony
>
>> ---
>> arch/arm/mach-omap2/mailbox.c | 10 ++++++----
>> 1 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
>> index 394413d..011ca50 100644
>> --- a/arch/arm/mach-omap2/mailbox.c
>> +++ b/arch/arm/mach-omap2/mailbox.c
>> @@ -193,10 +193,12 @@ static void omap2_mbox_disable_irq(struct omap_mbox *mbox,
>> omap_mbox_type_t irq)
>> {
>> struct omap_mbox2_priv *p = mbox->priv;
>> - u32 l, bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
>> - l = mbox_read_reg(p->irqdisable);
>> - l &= ~bit;
>> - mbox_write_reg(l, p->irqdisable);
>> + u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
>> +
>> + if (!cpu_is_omap44xx())
>> + bit = mbox_read_reg(p->irqdisable) & ~bit;
>> +
>> + mbox_write_reg(bit, p->irqdisable);
>> }
>>
>> static void omap2_mbox_ack_irq(struct omap_mbox *mbox,
>> --
>> 1.7.0.4
>>
>
^ permalink raw reply
* Re: [PATCHv2] omap:mailbox: resolve hang issue
From: Tony Lindgren @ 2011-03-03 1:44 UTC (permalink / raw)
To: Armando Uribe
Cc: hiroshi.doyu, ohad, Russell King, linux-omap, linux-arm-kernel,
linux-kernel, Hari Kanigeri, Fernando Guzman Lugo
In-Reply-To: <1299104058-28565-1-git-send-email-x0095078@ti.com>
* Armando Uribe <x0095078@ti.com> [110302 13:54]:
> From: Hari Kanigeri <h-kanigeri2@ti.com>
>
> omap4 interrupt disable bits is different. On rx kfifo full, the mbox rx
> interrupts wasn't getting disabled, and this is causing the rcm stress tests
> to hang.
>
> Signed-off-by: Hari Kanigeri <h-kanigeri2@ti.com>
> Signed-off-by: Armando Uribe <x0095078@ti.com>
> Signed-off-by: Fernando Guzman Lugo <fernando.lugo@ti.com>
Should we merge this as a fix for the 2.6.38 still?
Tony
> ---
> arch/arm/mach-omap2/mailbox.c | 10 ++++++----
> 1 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/mailbox.c b/arch/arm/mach-omap2/mailbox.c
> index 394413d..011ca50 100644
> --- a/arch/arm/mach-omap2/mailbox.c
> +++ b/arch/arm/mach-omap2/mailbox.c
> @@ -193,10 +193,12 @@ static void omap2_mbox_disable_irq(struct omap_mbox *mbox,
> omap_mbox_type_t irq)
> {
> struct omap_mbox2_priv *p = mbox->priv;
> - u32 l, bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
> - l = mbox_read_reg(p->irqdisable);
> - l &= ~bit;
> - mbox_write_reg(l, p->irqdisable);
> + u32 bit = (irq == IRQ_TX) ? p->notfull_bit : p->newmsg_bit;
> +
> + if (!cpu_is_omap44xx())
> + bit = mbox_read_reg(p->irqdisable) & ~bit;
> +
> + mbox_write_reg(bit, p->irqdisable);
> }
>
> static void omap2_mbox_ack_irq(struct omap_mbox *mbox,
> --
> 1.7.0.4
>
^ permalink raw reply
* Re: [PATCH v6 0/8] OMAP: HSMMC: hwmod adaptation
From: Tony Lindgren @ 2011-03-03 1:42 UTC (permalink / raw)
To: Kevin Hilman
Cc: Kishore Kadiyala, linux-mmc, linux-omap, cjb, madhu.cr, paul,
b-cousson
In-Reply-To: <878vwx9qah.fsf@ti.com>
* Kevin Hilman <khilman@ti.com> [110302 13:14]:
> Kishore Kadiyala <kishore.kadiyala@ti.com> writes:
>
> > Adding hwmod data for hsmmc device on OMAP2430/OMAP3/OMAP4.
> > Adapting the omap_hsmmc driver to hwmod framework.
>
> Looks OK to me.
>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
Sorry just missed it, got it all merged into omap-for-linus already
and rather not start messing with it any longer.
Tony
^ permalink raw reply
* Re: [PATCH] cbus-retu-wdt: Make the watchdog driver fully preemptible
From: Tony Lindgren @ 2011-03-03 1:33 UTC (permalink / raw)
To: Michael Buesch; +Cc: Felipe Balbi, linux-omap
In-Reply-To: <1299082307.13604.12.camel@marge>
* Michael Buesch <mb@bu3sch.de> [110302 08:09]:
> Make the WDT driver fully preemptible.
> This is required in order to apply a locking fix to retu_write_reg.
> It also improves power management by using round_relative_jiffies().
Thanks, seems to stay running, so applying all your remaining patches
to the cbus branch.
Tony
^ permalink raw reply
* Re: [PATCH 00/19] OMAP3+: introduce SR class 1.5
From: Nishanth Menon @ 2011-03-03 1:30 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <8762s1t3v4.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 06:30 AM:
> Nishanth Menon<nm@ti.com> writes:
>
>>>
>>> - Please Cc linux-arm-kernel for patches intended for mainline. Because
>>> of this, I didn't (yet) queue the ones I said I would queue.
>> done that with v2 which I had happened to have posted yesterday :(:
>> http://marc.info/?l=linux-omap&m=129906334611444&w=2
>
> Doh, sorry.
>
> My backlog is deep enough that I hadn't seen your v2 yet. :/
nothing lost ;).. I guess we can skip it for the moment and look at v3
later this week..
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 12/19] omap3+: sr: disable interrupt by default
From: Nishanth Menon @ 2011-03-03 1:23 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <87ei6pt3ws.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 06:29 AM:
> Nishanth Menon<nm@ti.com> writes:
>
>> Kevin Hilman wrote, on 03/03/2011 05:45 AM:
>>> Nishanth Menon<nm@ti.com> writes:
>>>
>>>> We will enable and disable interrupt on a need basis in the class
>>>> driver. we need to keep the irq disabled by default else the
>>>> forceupdate or vcbypass events could trigger events that we dont
>>>> need/expect to handle.
>>>
>>> It's not clear from the patch where the IRQ is re-enabled. For example,
>>> without knowing better, I would expect a corresponding change to the
>>> Class 3 driver to enable/disable the IRQ as needed.
>>
>> Why would that be?
>> a) class 3 driver does not request for any notifiers
>> b) class 3 does'nt need interrupts.
>> c) each class driver can choose to enable when it needs it - class3 does'nt.
>>
>> is it fine if I add a "this is a preperation for class drivers such as
>> class 2 and class 1.5 which would need to use interrupts" in commit
>> message?
>
> Yes, also stating/summarizing that existing class driver (e.g. class 3)
> does not use interrupts would be helpful.
Ack. will do in v3.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 0/6] omap3: pm: Fixes for low power code
From: Kevin Hilman @ 2011-03-03 1:22 UTC (permalink / raw)
To: Santosh Shilimkar; +Cc: linux-omap, linux-arm-kernel
In-Reply-To: <1298294365-30770-1-git-send-email-santosh.shilimkar@ti.com>
Hi Santosh,
Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
> The series does below fixes to the omap3 low power code.
> 1. Use supported ARMv7 instructions instead of the legacy ones
> 2. Fix the MMU on sequence
> 3. Fix the cache flush scenario when only L1 lost.
> 4. Remove all un-necessary context save registers
> 5. Disable C-bit before cache clean
> 6. Use set_cr() exported API instead of custom one.
Look like good cleanups, thanks.
> It's generated against mainline and tested with OMAP3630 ZOOM3.
> 1. Renetion and off-mode mode in suspend - ok.
> 2. Retention in idle - ok
Testing this series along with other PM changes queued up for mainline
(my pm-core branch), this doesn't work for me on 3630/Zoom3 (but works
fine on 3430/n900.)
Here's what I did using omap2plus_defconfig + enabling CPUidle
First tested suspend:
echo 4 > /debug/pm_debug/wakeup_timer_seconds
echo mem > /sys/power/state
then, setup UART idle/wakeup:
echo 5 > /sys/devices/platform/omap/omap_uart.0/sleep_timeout
echo 5 > /sys/devices/platform/omap/omap_uart.1/sleep_timeout
echo 5 > /sys/devices/platform/omap/omap_uart.2/sleep_timeout
echo 5 > /sys/devices/platform/omap/omap_uart.3/sleep_timeout
echo enabled > /sys/devices/platform/serial8250.0/tty/ttyS0/power/wakeup
then enabled idle:
echo 1 > /debug/pm_debug/sleep_while_idle
As soon as I do this, it hangs.
Without your series, it's working fine for me. Only after merging your
series it hangs.
Kevin
> The following changes since commit 85e2efbb1db9a18d218006706d6e4fbeb0216213:
> Linus Torvalds (1):
> Linux 2.6.38-rc5
>
> Santosh Shilimkar (6):
> omap3: pm: Use amrv7 supported instructions instead of legacy cp15
> ones
> omap3: pm: Fix the mmu on sequence in the asm code
> omap3: pm: Allow the cache clean when L1 is lost.
> omap3: pm: Remove un-necessary cp15 registers form low power cpu
> context
> omap3: pm: Clear the SCTLR C bit in asm code to prevent data cache
> allocation
> omap3: pm: Use exported set_cr() instead of a custom one.
>
> arch/arm/mach-omap2/pm34xx.c | 7 +-
> arch/arm/mach-omap2/sleep34xx.S | 223 ++++++++++++++-------------------------
> 2 files changed, 78 insertions(+), 152 deletions(-)
^ permalink raw reply
* Re: [PATCH 09/19] omap3+: sr: introduce class init,deinit and priv data
From: Nishanth Menon @ 2011-03-03 1:22 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <87k4ght402.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 06:27 AM:
> Nishanth Menon<nm@ti.com> writes:
>
>> Kevin Hilman wrote, on 03/03/2011 05:38 AM:
>>> Nishanth Menon<nm@ti.com> writes:
>>>
>>>> Certain class drivers such as class 1.5 drivers, will need specific
>>>> notification that they have to be started up or stopped independent
>>>> of smart reflex operation. They also may need private data to be
>>>> used for operations of their own, provide the same.
>>>>
>>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>>
>>> Basic principle looks fine, but some naming comments below...
>> k, thx.
>>
>>>
>>> [...]
>>>
>>>> diff --git a/arch/arm/plat-omap/include/plat/smartreflex.h b/arch/arm/plat-omap/include/plat/smartreflex.h
>>>> index 6568c88..8b6ecd9 100644
>>>> --- a/arch/arm/plat-omap/include/plat/smartreflex.h
>>>> +++ b/arch/arm/plat-omap/include/plat/smartreflex.h
>>>> @@ -167,6 +167,8 @@ struct omap_sr_pmic_data {
>>>> *
>>>> * @enable: API to enable a particular class smaartreflex.
>>>> * @disable: API to disable a particular class smartreflex.
>>>> + * @class_init: API to do class specific initialization (optional)
>>>> + * @class_deinit: API to do class specific initialization (optional)
>>>
>>> The 'class_' prefix here is not needed.
>> ack.
>>
>>>
>>> The changelog uses 'start' and 'stop' instead of init& deinit. I
>>> prefer those names to [de]init.
>>
>> Would'nt start and stop cause confusion when mixed with existing
>> enable/disable? does enable/start actually start the SR? intent here
>> with init/deinit is to do class specific initialization or
>> deinitialization.
>
> Well, one way or another, make the changelog consistent with the
> function names.
>
> To me though, start/stop has more meaning. They are called from
> sr_[start|stop]_vddautocomp, so using start/stop is consistent there.
> Also, init is usually something done once (and doesn't have a de-init
> counterpart) but here we're talking about something done for each
> sr_[start|stop]_vddautocomp()
fair enough - unless there are other dissenting voices, will switch to
start/stop for the api name and add documentation in code to explain the
need.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH] omap: rx51: Add support for vibra
From: Tony Lindgren @ 2011-03-03 1:02 UTC (permalink / raw)
To: Ilkka Koskinen; +Cc: linux-omap
In-Reply-To: <1298623583-9743-1-git-send-email-ilkka.koskinen@nokia.com>
* Ilkka Koskinen <ilkka.koskinen@nokia.com> [110225 00:48]:
Adding for the merge window with updated description
"Add support for vibra".
Tony
> Signed-off-by: Ilkka Koskinen <ilkka.koskinen@nokia.com>
> ---
> arch/arm/mach-omap2/board-rx51-peripherals.c | 9 +++++++++
> 1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-rx51-peripherals.c b/arch/arm/mach-omap2/board-rx51-peripherals.c
> index e75e240..dcd98fa 100644
> --- a/arch/arm/mach-omap2/board-rx51-peripherals.c
> +++ b/arch/arm/mach-omap2/board-rx51-peripherals.c
> @@ -699,6 +699,14 @@ static struct twl4030_power_data rx51_t2scripts_data __initdata = {
> .resource_config = twl4030_rconfig,
> };
>
> +struct twl4030_codec_vibra_data rx51_vibra_data __initdata = {
> + .coexist = 0,
> +};
> +
> +struct twl4030_codec_data rx51_codec_data __initdata = {
> + .audio_mclk = 26000000,
> + .vibra = &rx51_vibra_data,
> +};
>
> static struct twl4030_platform_data rx51_twldata __initdata = {
> .irq_base = TWL4030_IRQ_BASE,
> @@ -710,6 +718,7 @@ static struct twl4030_platform_data rx51_twldata __initdata = {
> .madc = &rx51_madc_data,
> .usb = &rx51_usb_data,
> .power = &rx51_t2scripts_data,
> + .codec = &rx51_codec_data,
>
> .vaux1 = &rx51_vaux1,
> .vaux2 = &rx51_vaux2,
> --
> 1.7.0.4
>
^ permalink raw reply
* Re: [PATCH] omap: panda: Add TI-ST driver support
From: Tony Lindgren @ 2011-03-03 1:00 UTC (permalink / raw)
To: Guy Eilam; +Cc: linux-omap
In-Reply-To: <1298616755-10192-1-git-send-email-guy@wizery.com>
* Guy Eilam <guy@wizery.com> [110224 22:52]:
> Added the KIM (Kernel initialization module for the
> Shared Transport driver) device entry in the board file
>
> Only the Blutooth enable GPIO is set for now
Adding for the merge window.
Tony
^ permalink raw reply
* Re: [PATCH 00/19] OMAP3+: introduce SR class 1.5
From: Kevin Hilman @ 2011-03-03 1:00 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <4D6EE2D5.7040900@ti.com>
Nishanth Menon <nm@ti.com> writes:
>>
>> - Please Cc linux-arm-kernel for patches intended for mainline. Because
>> of this, I didn't (yet) queue the ones I said I would queue.
> done that with v2 which I had happened to have posted yesterday :(:
> http://marc.info/?l=linux-omap&m=129906334611444&w=2
Doh, sorry.
My backlog is deep enough that I hadn't seen your v2 yet. :/
Kevin
^ permalink raw reply
* Re: [PATCH] ldp: Fix regulator mapping for ads7846 TS controller
From: Tony Lindgren @ 2011-03-03 1:00 UTC (permalink / raw)
To: Rajendra Nayak; +Cc: linux-omap, linux-arm-kernel
In-Reply-To: <1298552202-28307-1-git-send-email-rnayak@ti.com>
* Rajendra Nayak <rnayak@ti.com> [110224 04:54]:
> On the OMAP3430LDP board, the ads7846 touchscreen controller
> is powered by VAUX1 regulator (supplying 3.0v).
> Fix this mapping in the board file, and hence prevent
> the ads7846 driver init to fail with the below error..
>
> ads7846 spi1.0: unable to get regulator: -19
Applying for the merge window.
Tony
^ permalink raw reply
* Re: [PATCH 12/19] omap3+: sr: disable interrupt by default
From: Kevin Hilman @ 2011-03-03 0:59 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <4D6EE04D.600@ti.com>
Nishanth Menon <nm@ti.com> writes:
> Kevin Hilman wrote, on 03/03/2011 05:45 AM:
>> Nishanth Menon<nm@ti.com> writes:
>>
>>> We will enable and disable interrupt on a need basis in the class
>>> driver. we need to keep the irq disabled by default else the
>>> forceupdate or vcbypass events could trigger events that we dont
>>> need/expect to handle.
>>
>> It's not clear from the patch where the IRQ is re-enabled. For example,
>> without knowing better, I would expect a corresponding change to the
>> Class 3 driver to enable/disable the IRQ as needed.
>
> Why would that be?
> a) class 3 driver does not request for any notifiers
> b) class 3 does'nt need interrupts.
> c) each class driver can choose to enable when it needs it - class3 does'nt.
>
> is it fine if I add a "this is a preperation for class drivers such as
> class 2 and class 1.5 which would need to use interrupts" in commit
> message?
Yes, also stating/summarizing that existing class driver (e.g. class 3)
does not use interrupts would be helpful.
Kevin
^ permalink raw reply
* Re: [PATCH 09/19] omap3+: sr: introduce class init,deinit and priv data
From: Kevin Hilman @ 2011-03-03 0:57 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <4D6EE3C0.8060202@ti.com>
Nishanth Menon <nm@ti.com> writes:
> Kevin Hilman wrote, on 03/03/2011 05:38 AM:
>> Nishanth Menon<nm@ti.com> writes:
>>
>>> Certain class drivers such as class 1.5 drivers, will need specific
>>> notification that they have to be started up or stopped independent
>>> of smart reflex operation. They also may need private data to be
>>> used for operations of their own, provide the same.
>>>
>>> Signed-off-by: Nishanth Menon<nm@ti.com>
>>
>> Basic principle looks fine, but some naming comments below...
> k, thx.
>
>>
>> [...]
>>
>>> diff --git a/arch/arm/plat-omap/include/plat/smartreflex.h b/arch/arm/plat-omap/include/plat/smartreflex.h
>>> index 6568c88..8b6ecd9 100644
>>> --- a/arch/arm/plat-omap/include/plat/smartreflex.h
>>> +++ b/arch/arm/plat-omap/include/plat/smartreflex.h
>>> @@ -167,6 +167,8 @@ struct omap_sr_pmic_data {
>>> *
>>> * @enable: API to enable a particular class smaartreflex.
>>> * @disable: API to disable a particular class smartreflex.
>>> + * @class_init: API to do class specific initialization (optional)
>>> + * @class_deinit: API to do class specific initialization (optional)
>>
>> The 'class_' prefix here is not needed.
> ack.
>
>>
>> The changelog uses 'start' and 'stop' instead of init& deinit. I
>> prefer those names to [de]init.
>
> Would'nt start and stop cause confusion when mixed with existing
> enable/disable? does enable/start actually start the SR? intent here
> with init/deinit is to do class specific initialization or
> deinitialization.
Well, one way or another, make the changelog consistent with the
function names.
To me though, start/stop has more meaning. They are called from
sr_[start|stop]_vddautocomp, so using start/stop is consistent there.
Also, init is usually something done once (and doesn't have a de-init
counterpart) but here we're talking about something done for each
sr_[start|stop]_vddautocomp()
Kevin
^ permalink raw reply
* Re: [PATCH 03/19] omap3+: voltage: remove initial voltage
From: Kevin Hilman @ 2011-03-03 0:53 UTC (permalink / raw)
To: Nishanth Menon; +Cc: Vishwanath Sripathy, linux-omap, Tony Lindgren
In-Reply-To: <4D6EE248.3010205@ti.com>
Nishanth Menon <nm@ti.com> writes:
[...]
>
> Does the following sound any better?:
Yes, thanks.
> Blindly setting a 1.2v setting in the initial structure may not even
> match the default voltages stored in the voltage table which are
> supported for the domain. For example, OMAP3430 core domain does not
> use 1.2v and ends up generating a warning on the first transition.
>
> Further, since omap2_set_init_voltage is called as part of the pm
> framework's initialization sequence to configure the voltage required
> for the current OPP, the call does(and has to) setup the system
> voltage(curr_volt as a result) using the right mechanisms appropriate
> for the system at that point of time. This also overrides
> initialization we are currently doing in voltage.c making it
> redundant. So remove the wrong and useless initialization.
^ permalink raw reply
* Re: [RFC 3/3] omap3+: sr: Reuse sr_[start|stop]_vddautocomp functions
From: Nishanth Menon @ 2011-03-03 0:52 UTC (permalink / raw)
To: Kevin Hilman; +Cc: Jarkko Nikula, linux-omap, Thara Gopinath
In-Reply-To: <87vd01t4ex.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 06:18 AM:
> Nishanth Menon<nm@ti.com> writes:
>
>> Jarkko Nikula wrote, on 03/02/2011 09:27 PM:
>>> sr_start_vddautocomp and sr_stop_autocomp functions can be reused from
>>> omap_sr_enable, omap_sr_disable and omap_sr_disable_reset_volt and by
>>> adding one additional argument sr_stop_autocomp.
>>>
>>> Signed-off-by: Jarkko Nikula<jhnikula@gmail.com>
>>> ---
>>> arch/arm/mach-omap2/smartreflex.c | 41 ++++++------------------------------
>>> 1 files changed, 7 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>>> index 11741d8..7e6002f 100644
>>> --- a/arch/arm/mach-omap2/smartreflex.c
>>> +++ b/arch/arm/mach-omap2/smartreflex.c
>>> @@ -227,7 +227,7 @@ static void sr_start_vddautocomp(struct omap_sr *sr)
>>> sr->autocomp_active = true;
>>> }
>>>
>>> -static void sr_stop_vddautocomp(struct omap_sr *sr)
>>> +static void sr_stop_vddautocomp(struct omap_sr *sr, int is_volt_reset)
>>> {
>>> if (!sr->autocomp_active)
>>> return;
>>> @@ -239,7 +239,7 @@ static void sr_stop_vddautocomp(struct omap_sr *sr)
>>> return;
>>> }
>>>
>>> - if (!sr_class->disable(sr->voltdm, 1))
>>> + if (!sr_class->disable(sr->voltdm, is_volt_reset))
>>> sr->autocomp_active = false;
>>> }
>>>
>>> @@ -681,16 +681,7 @@ void omap_sr_enable(struct voltagedomain *voltdm)
>>> return;
>>> }
>>>
>>> - if (!sr->autocomp_active)
>>> - return;
>>> -
>>> - if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) {
>>> - dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
>>> - "registered\n", __func__);
>>> - return;
>>> - }
>>> -
>>> - sr_class->enable(voltdm);
>>> + sr_start_vddautocomp(sr);
>>> }
>>>
>>> /**
>>> @@ -714,16 +705,7 @@ void omap_sr_disable(struct voltagedomain *voltdm)
>>> return;
>>> }
>>>
>>> - if (!sr->autocomp_active)
>>> - return;
>>> -
>>> - if (!sr_class || !(sr_class->disable)) {
>>> - dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
>>> - "registered\n", __func__);
>>> - return;
>>> - }
>>> -
>>> - sr_class->disable(voltdm, 0);
>>> + sr_stop_vddautocomp(sr, 0);
>>> }
>>>
>>> /**
>>> @@ -747,16 +729,7 @@ void omap_sr_disable_reset_volt(struct voltagedomain *voltdm)
>>> return;
>>> }
>>>
>>> - if (!sr->autocomp_active)
>>> - return;
>>> -
>>> - if (!sr_class || !(sr_class->disable)) {
>>> - dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
>>> - "registered\n", __func__);
>>> - return;
>>> - }
>>> -
>>> - sr_class->disable(voltdm, 1);
>>> + sr_stop_vddautocomp(sr, 1);
>>> }
>>>
>>> /**
>>> @@ -809,7 +782,7 @@ static int omap_sr_autocomp_store(void *data, u64 val)
>>> }
>>>
>>> if (!val)
>>> - sr_stop_vddautocomp(sr_info);
>>> + sr_stop_vddautocomp(sr_info, 1);
>>> else
>>> sr_start_vddautocomp(sr_info);
>>>
>>> @@ -976,7 +949,7 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
>>> }
>>>
>>> if (sr_info->autocomp_active)
>>> - sr_stop_vddautocomp(sr_info);
>>> + sr_stop_vddautocomp(sr_info, 1);
>>>
>>> list_del(&sr_info->node);
>>> iounmap(sr_info->base);
>>
>> Looks like a nice cleanup to me.
>>
>> if the motivation of patch 1/3 is to do this cleanup, I am altogether
>> for it (lesser code == lesser bugs ;) ). btw, this should not impact
>> class1.5 either - core sr.c sequencing has not been modified in sr1.5
>> :)
>>
>> Depending on Kevin's views, either you or I or both of us can rebase
>> depending on whose ever series gets pulled in first.. (either should
>> be a very minimal effort).
>
> Nishanth,
>
> I'd prefer if you handle these. You can add them to the fixup/cleanup
> part of your SR1.5 series.
ok will pull this in as part of sr1.5 series if no further review comments.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [RFC 3/3] omap3+: sr: Reuse sr_[start|stop]_vddautocomp functions
From: Kevin Hilman @ 2011-03-03 0:48 UTC (permalink / raw)
To: Nishanth Menon; +Cc: Jarkko Nikula, linux-omap, Thara Gopinath
In-Reply-To: <4D6E775D.7040107@ti.com>
Nishanth Menon <nm@ti.com> writes:
> Jarkko Nikula wrote, on 03/02/2011 09:27 PM:
>> sr_start_vddautocomp and sr_stop_autocomp functions can be reused from
>> omap_sr_enable, omap_sr_disable and omap_sr_disable_reset_volt and by
>> adding one additional argument sr_stop_autocomp.
>>
>> Signed-off-by: Jarkko Nikula<jhnikula@gmail.com>
>> ---
>> arch/arm/mach-omap2/smartreflex.c | 41 ++++++------------------------------
>> 1 files changed, 7 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>> index 11741d8..7e6002f 100644
>> --- a/arch/arm/mach-omap2/smartreflex.c
>> +++ b/arch/arm/mach-omap2/smartreflex.c
>> @@ -227,7 +227,7 @@ static void sr_start_vddautocomp(struct omap_sr *sr)
>> sr->autocomp_active = true;
>> }
>>
>> -static void sr_stop_vddautocomp(struct omap_sr *sr)
>> +static void sr_stop_vddautocomp(struct omap_sr *sr, int is_volt_reset)
>> {
>> if (!sr->autocomp_active)
>> return;
>> @@ -239,7 +239,7 @@ static void sr_stop_vddautocomp(struct omap_sr *sr)
>> return;
>> }
>>
>> - if (!sr_class->disable(sr->voltdm, 1))
>> + if (!sr_class->disable(sr->voltdm, is_volt_reset))
>> sr->autocomp_active = false;
>> }
>>
>> @@ -681,16 +681,7 @@ void omap_sr_enable(struct voltagedomain *voltdm)
>> return;
>> }
>>
>> - if (!sr->autocomp_active)
>> - return;
>> -
>> - if (!sr_class || !(sr_class->enable) || !(sr_class->configure)) {
>> - dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
>> - "registered\n", __func__);
>> - return;
>> - }
>> -
>> - sr_class->enable(voltdm);
>> + sr_start_vddautocomp(sr);
>> }
>>
>> /**
>> @@ -714,16 +705,7 @@ void omap_sr_disable(struct voltagedomain *voltdm)
>> return;
>> }
>>
>> - if (!sr->autocomp_active)
>> - return;
>> -
>> - if (!sr_class || !(sr_class->disable)) {
>> - dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
>> - "registered\n", __func__);
>> - return;
>> - }
>> -
>> - sr_class->disable(voltdm, 0);
>> + sr_stop_vddautocomp(sr, 0);
>> }
>>
>> /**
>> @@ -747,16 +729,7 @@ void omap_sr_disable_reset_volt(struct voltagedomain *voltdm)
>> return;
>> }
>>
>> - if (!sr->autocomp_active)
>> - return;
>> -
>> - if (!sr_class || !(sr_class->disable)) {
>> - dev_warn(&sr->pdev->dev, "%s: smartreflex class driver not"
>> - "registered\n", __func__);
>> - return;
>> - }
>> -
>> - sr_class->disable(voltdm, 1);
>> + sr_stop_vddautocomp(sr, 1);
>> }
>>
>> /**
>> @@ -809,7 +782,7 @@ static int omap_sr_autocomp_store(void *data, u64 val)
>> }
>>
>> if (!val)
>> - sr_stop_vddautocomp(sr_info);
>> + sr_stop_vddautocomp(sr_info, 1);
>> else
>> sr_start_vddautocomp(sr_info);
>>
>> @@ -976,7 +949,7 @@ static int __devexit omap_sr_remove(struct platform_device *pdev)
>> }
>>
>> if (sr_info->autocomp_active)
>> - sr_stop_vddautocomp(sr_info);
>> + sr_stop_vddautocomp(sr_info, 1);
>>
>> list_del(&sr_info->node);
>> iounmap(sr_info->base);
>
> Looks like a nice cleanup to me.
>
> if the motivation of patch 1/3 is to do this cleanup, I am altogether
> for it (lesser code == lesser bugs ;) ). btw, this should not impact
> class1.5 either - core sr.c sequencing has not been modified in sr1.5
> :)
>
> Depending on Kevin's views, either you or I or both of us can rebase
> depending on whose ever series gets pulled in first.. (either should
> be a very minimal effort).
Nishanth,
I'd prefer if you handle these. You can add them to the fixup/cleanup
part of your SR1.5 series.
Thanks,
Kevin
^ permalink raw reply
* Re: [PATCH 14/19] omap3+: sr: introduce notifiers flags
From: Nishanth Menon @ 2011-03-03 0:47 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <87vd01ukek.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 05:47 AM:
[..]
>
>> diff --git a/arch/arm/plat-omap/include/plat/smartreflex.h b/arch/arm/plat-omap/include/plat/smartreflex.h
>> index 8b6ecd9..ff07d1e 100644
>> --- a/arch/arm/plat-omap/include/plat/smartreflex.h
>> +++ b/arch/arm/plat-omap/include/plat/smartreflex.h
>> @@ -141,6 +141,12 @@
>> #define OMAP3430_SR_ERRWEIGHT 0x04
>> #define OMAP3430_SR_ERRMAXLIMIT 0x02
>>
>> +/* Smart reflex notifiers for class drivers to use */
>> +#define SR_NOTIFY_MCUACCUM (0x1<< 0)
>> +#define SR_NOTIFY_MCUVALID (0x1<< 1)
>> +#define SR_NOTIFY_MCUBOUND (0x1<< 2)
>> +#define SR_NOTIFY_MCUDISACK (0x1<< 3)
>> +
>
> Please use BIT(x)
Ack.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 11/19] omap3+: sr: call handler with interrupt disabled
From: Nishanth Menon @ 2011-03-03 0:46 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <87fwr5vzaj.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 05:41 AM:
> Nishanth Menon<nm@ti.com> writes:
>
>> Request the handler irq such that there is no nesting for calls.
>> the notifiers are not expected to be nested, further the interrupt
>> events for status change should be handled prior to the next event
>> else there is a risk of loosing events.
>>
>> Signed-off-by: Nishanth Menon<nm@ti.com>
>
> IRQs disabled interrupt handlers are soon to disappear in the kernel.
>
> This kind of thing should be handled using locking, most likely IRQs
> disabled spinlocks.
http://marc.info/?t=129906354500012&r=1&w=2
Ack.. painfully made clear in l-a as well.. dropping this :(
>
> Kevin
>
>> ---
>> arch/arm/mach-omap2/smartreflex.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
>> index 1fe8b95..7931fcd 100644
>> --- a/arch/arm/mach-omap2/smartreflex.c
>> +++ b/arch/arm/mach-omap2/smartreflex.c
>> @@ -278,7 +278,7 @@ static int sr_late_init(struct omap_sr *sr_info)
>> goto error;
>> }
>> ret = request_irq(sr_info->irq, sr_interrupt,
>> - 0, name, (void *)sr_info);
>> + IRQF_DISABLED, name, (void *)sr_info);
>> if (ret)
>> goto error;
>> }
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH] perf: add OMAP support for the new power events
From: Kevin Hilman @ 2011-03-03 0:43 UTC (permalink / raw)
To: jean.pihet; +Cc: Thomas Renninger, linux-omap, linux-arm-kernel, Jean Pihet
In-Reply-To: <1298052645-4164-1-git-send-email-j-pihet@ti.com>
Hi Jean,
jean.pihet@newoldbits.com writes:
> From: Jean Pihet <j-pihet@ti.com>
>
> The patch adds the new power management trace points for
> the OMAP architecture.
There are some other core clock/powerdomain changes queued for 2.6.39
ahead of this that conflict with your patch.
Could you rebase this against my pm-core branch where these other
changes are queued?
Thanks,
Kevin
> The trace points are for:
> - default idle handler. Since the cpuidle framework is
> instrumented in the generic way there is no need to
> add trace points in the OMAP specific cpuidle handler;
> - cpufreq (DVFS),
> - SoC clocks changes (enable, disable, set_rate),
> - power domain states: the desired target state and -if different-
> the actually hit state.
>
> Because of the generic nature of the changes, OMAP3 and OMAP4 are supported.
>
> Tested on OMAP3 with suspend/resume, cpuidle, basic DVFS.
>
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
> arch/arm/mach-omap2/clock.c | 8 +++++++-
> arch/arm/mach-omap2/pm34xx.c | 7 +++++++
> arch/arm/mach-omap2/powerdomain.c | 26 +++++++++++++++++++++++---
> 3 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clock.c b/arch/arm/mach-omap2/clock.c
> index 2a2f152..72af75d 100644
> --- a/arch/arm/mach-omap2/clock.c
> +++ b/arch/arm/mach-omap2/clock.c
> @@ -22,7 +22,9 @@
> #include <linux/clk.h>
> #include <linux/io.h>
> #include <linux/bitops.h>
> +#include <trace/events/power.h>
>
> +#include <asm/cpu.h>
> #include <plat/clock.h>
> #include "clockdomain.h"
> #include <plat/cpu.h>
> @@ -261,6 +263,7 @@ void omap2_clk_disable(struct clk *clk)
>
> pr_debug("clock: %s: disabling in hardware\n", clk->name);
>
> + trace_clock_disable(clk->name, 0, smp_processor_id());
> clk->ops->disable(clk);
>
> if (clk->clkdm)
> @@ -312,6 +315,7 @@ int omap2_clk_enable(struct clk *clk)
> }
> }
>
> + trace_clock_enable(clk->name, 1, smp_processor_id());
> ret = clk->ops->enable(clk);
> if (ret) {
> WARN(1, "clock: %s: could not enable: %d\n", clk->name, ret);
> @@ -349,8 +353,10 @@ int omap2_clk_set_rate(struct clk *clk, unsigned long rate)
> pr_debug("clock: set_rate for clock %s to rate %ld\n", clk->name, rate);
>
> /* dpll_ck, core_ck, virt_prcm_set; plus all clksel clocks */
> - if (clk->set_rate)
> + if (clk->set_rate) {
> + trace_clock_set_rate(clk->name, rate, smp_processor_id());
> ret = clk->set_rate(clk, rate);
> + }
>
> return ret;
> }
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 2f864e4..d1cc3f4 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -29,6 +29,7 @@
> #include <linux/delay.h>
> #include <linux/slab.h>
> #include <linux/console.h>
> +#include <trace/events/power.h>
>
> #include <plat/sram.h>
> #include "clockdomain.h"
> @@ -519,8 +520,14 @@ static void omap3_pm_idle(void)
> if (omap_irq_pending() || need_resched())
> goto out;
>
> + trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> + trace_cpu_idle(1, smp_processor_id());
> +
> omap_sram_idle();
>
> + trace_power_end(smp_processor_id());
> + trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
> +
> out:
> local_fiq_enable();
> local_irq_enable();
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index eaed0df..1495eed 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -19,12 +19,15 @@
> #include <linux/list.h>
> #include <linux/errno.h>
> #include <linux/string.h>
> +#include <trace/events/power.h>
> +
> #include "cm2xxx_3xxx.h"
> #include "prcm44xx.h"
> #include "cm44xx.h"
> #include "prm2xxx_3xxx.h"
> #include "prm44xx.h"
>
> +#include <asm/cpu.h>
> #include <plat/cpu.h>
> #include "powerdomain.h"
> #include "clockdomain.h"
> @@ -32,6 +35,8 @@
>
> #include "pm.h"
>
> +#define PWRDM_TRACE_STATES_FLAG (1<<31)
> +
> enum {
> PWRDM_STATE_NOW = 0,
> PWRDM_STATE_PREV,
> @@ -130,8 +135,7 @@ static void _update_logic_membank_counters(struct powerdomain *pwrdm)
> static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
> {
>
> - int prev;
> - int state;
> + int prev, state, trace_state = 0;
>
> if (pwrdm == NULL)
> return -EINVAL;
> @@ -148,6 +152,17 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
> pwrdm->state_counter[prev]++;
> if (prev == PWRDM_POWER_RET)
> _update_logic_membank_counters(pwrdm);
> + /*
> + * If the power domain did not hit the desired state,
> + * generate a trace event with both the desired and hit states
> + */
> + if (state != prev) {
> + trace_state = (PWRDM_TRACE_STATES_FLAG |
> + ((state & OMAP_POWERSTATE_MASK) << 8) |
> + ((prev & OMAP_POWERSTATE_MASK) << 0));
> + trace_power_domain_target(pwrdm->name, trace_state,
> + smp_processor_id());
> + }
> break;
> default:
> return -EINVAL;
> @@ -406,8 +421,13 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
> pr_debug("powerdomain: setting next powerstate for %s to %0x\n",
> pwrdm->name, pwrst);
>
> - if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst)
> + if (arch_pwrdm && arch_pwrdm->pwrdm_set_next_pwrst) {
> + /* Trace the pwrdm desired target state */
> + trace_power_domain_target(pwrdm->name, pwrst,
> + smp_processor_id());
> + /* Program the pwrdm desired target state */
> ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
> + }
>
> return ret;
> }
^ permalink raw reply
* Re: [PATCH 01/19] omap3: hwmod: add smartreflex irqs
From: Nishanth Menon @ 2011-03-03 0:43 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <87bp1txewg.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 05:18 AM:
> Nishanth Menon<nm@ti.com> writes:
>
>> OMAP3 smartreflex irqs in hwmod structures with the same naming as
>> present in OMAP4. Without these IRQs being registered, SmartReflex
>> driver will be unable to get the irq numbers to handle notifications
>>
>> Signed-off-by: Nishanth Menon<nm@ti.com>
>
> minor: IRQ is an acronym, please capitalize consistenly throughout.
>
> I know we haven't been too consitent about this but OMAP is also an
> acronym, please capitialize (c.f. $SUBJECT). This goes for the whole
> series.
here and elsewhere - will do with v3.. thanks for catching..
>
> Otherwise patch looks fine.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 09/19] omap3+: sr: introduce class init,deinit and priv data
From: Nishanth Menon @ 2011-03-03 0:41 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <87r5apvzfc.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 05:38 AM:
> Nishanth Menon<nm@ti.com> writes:
>
>> Certain class drivers such as class 1.5 drivers, will need specific
>> notification that they have to be started up or stopped independent
>> of smart reflex operation. They also may need private data to be
>> used for operations of their own, provide the same.
>>
>> Signed-off-by: Nishanth Menon<nm@ti.com>
>
> Basic principle looks fine, but some naming comments below...
k, thx.
>
> [...]
>
>> diff --git a/arch/arm/plat-omap/include/plat/smartreflex.h b/arch/arm/plat-omap/include/plat/smartreflex.h
>> index 6568c88..8b6ecd9 100644
>> --- a/arch/arm/plat-omap/include/plat/smartreflex.h
>> +++ b/arch/arm/plat-omap/include/plat/smartreflex.h
>> @@ -167,6 +167,8 @@ struct omap_sr_pmic_data {
>> *
>> * @enable: API to enable a particular class smaartreflex.
>> * @disable: API to disable a particular class smartreflex.
>> + * @class_init: API to do class specific initialization (optional)
>> + * @class_deinit: API to do class specific initialization (optional)
>
> The 'class_' prefix here is not needed.
ack.
>
> The changelog uses 'start' and 'stop' instead of init& deinit. I
> prefer those names to [de]init.
Would'nt start and stop cause confusion when mixed with existing
enable/disable? does enable/start actually start the SR? intent here
with init/deinit is to do class specific initialization or deinitialization.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 00/19] OMAP3+: introduce SR class 1.5
From: Nishanth Menon @ 2011-03-03 0:37 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <87aahdujox.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 06:03 AM:
> - Please capitalize acronyms throughout the
> subjects/comments/changelogs. This series tends to mix lower-case and
> upper case acronyms
Ack. thanks for catching it.
>
> - Please Cc linux-arm-kernel for patches intended for mainline. Because
> of this, I didn't (yet) queue the ones I said I would queue.
done that with v2 which I had happened to have posted yesterday :(:
http://marc.info/?l=linux-omap&m=129906334611444&w=2
>
> - Please rebase this on top of my pm-core branch (or Paul's
> integration-2.6.39 branch if you prefer.) There are some important
> voltage related re-organization happening there that affects your code.
ok. will do that and integrate comments from v1 and v2 for v3..
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 03/19] omap3+: voltage: remove initial voltage
From: Nishanth Menon @ 2011-03-03 0:35 UTC (permalink / raw)
To: Kevin Hilman; +Cc: Vishwanath Sripathy, linux-omap, Tony Lindgren
In-Reply-To: <8762s1xep6.fsf@ti.com>
Kevin Hilman wrote, on 03/03/2011 05:22 AM:
> "Menon, Nishanth"<nm@ti.com> writes:
>
>> On Wed, Feb 23, 2011 at 14:29, Vishwanath Sripathy<vishwanath.bs@ti.com> wrote:
>>>> -----Original Message-----
>>>> From: Menon, Nishanth [mailto:nm@ti.com]
>>>> Sent: Wednesday, February 23, 2011 1:48 PM
>>>> To: Vishwanath Sripathy
>>>> Cc: linux-omap; Tony Lindgren; Kevin Hilman
>>>> Subject: Re: [PATCH 03/19] omap3+: voltage: remove initial voltage
>>>>
>>>> Wed, Feb 23, 2011 at 12:24, Vishwanath Sripathy
>>>> <vishwanath.bs@ti.com> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Nishanth Menon [mailto:nm@ti.com]
>>>>>> Sent: Sunday, February 20, 2011 10:42 AM
>>>>>> To: Vishwanath Sripathy
>>>>>> Cc: linux-omap; Tony Lindgren; Kevin Hilman
>>>>>> Subject: Re: [PATCH 03/19] omap3+: voltage: remove initial voltage
>>>>>>
>>>>>> Vishwanath Sripathy wrote, on 02/19/2011 06:54 PM:
>>>>>> [..]
>>>>>>>> omap2_set_init_voltage should setup the curr_volt based on
>>>> which
>>>>>> OPP
>>>>>>>> the system is functioning at. Blindly setting a 1.2v setting in
>>> the
>>>>>>>> initial structure may not even match the default voltages stored
>>>> in
>>>>>>>> the voltage table which are supported for the domain.
>>>>>>>>
>>>>>>>> For example, OMAP3430 core domain does not use 1.2v and ends
>>>> up
>>>>>>>> generating a warning on the first transition.
>>>>>>>>
>>>>>> [..]
>>>>>>>> diff --git a/arch/arm/mach-omap2/voltage.c b/arch/arm/mach-
>>>>>>>> omap2/voltage.c
>>>>>>>> index 12be525..280ee12 100644
>>>>>>>> --- a/arch/arm/mach-omap2/voltage.c
>>>>>>>> +++ b/arch/arm/mach-omap2/voltage.c
>>>>>> [..]
>>>>>>>> /* Generic voltage parameters */
>>>>>>>> - vdd->curr_volt = 1200000;
>>>>>>> Where do you update this parameter upon initialization? Shouldn't
>>>> you
>>>>>> read
>>>>>>> the VP register and find the actual current voltage and update this
>>>>>> param?
>>>>>>
>>>>>> The sequence is as follows:
>>>>>> a) omapx_vdd_data configure is called as part of sr init sequence.
>>>>>> And the curr_volt with this patch is not updated at this stage.
>>>>>> b) somewhere down in the boot sequence, pm.c's
>>>>>> omap2_set_init_voltage
>>>>>> starts up. This looks up the current clk frequency from clock layer
>>> of
>>>>>> the parent device for the domain, picks up the nominal voltages
>>>> stored
>>>>>> in the opp layer, then does a omap_voltage_scale_vdd to that
>>>> voltage. In
>>>>>> omap_voltage_scale_vdd, The current voltage is merely picked off
>>>> the vp
>>>>>> (in _pre_volt_scale). the last step it does is to setup
>>> vdd->curr_volt.
>>>>>> This can then be used by dvfs layer etc to make appropriate
>>>> decisions.
>>>>>>
>>>>>> So, No, I dont think I need to update it here, it should happen as
>>>> part
>>>>>> of the pm init sequence.
>>>>>>
>>>>>> Could you explain what problem do you foresee by doing this?
>>>>> What if omap_voltage_scale_vdd fails when called from
>>>>> omap2_set_init_voltage? Or omap2_set_init_voltage returns error
>>>> even
>>>>> before calling omap_voltage_scale_vdd because it could not find the
>>>>> matching voltage for the frequency set by bootloader?
>>>>>
>>>>> Your assumption that curr_volt is always set by
>>>> omap2_set_init_voltage
>>>>> need not be true always.
>>>>
>>>> set_init_voltage's job is to set the initial voltage. if it is
>>>> incapable of doing it, I say fix that instead of hacking in some
>>>> random number as curr_volt.
>>> I never said to use random numbers. All I suggested was that instead of
>>> relying on some others function's behavior, read the VP register to fill
>>> in the right values. If set_init_voltage succeeds, it will anyway update
>>> with right voltage.
>> OK, lets take this argument for a moment.
>> Q: which voltage to set as curr_volt?
>> a) what if the update was done over vcbypass by bootloader? Cannot
>> trust the vp register for the value.
>> b) what if the voltage was updated over i2c1 to the PMIC by
>> bootloader? cant trust vp register again.
>> c) use some number like 1.2v - which we are aligned is wrong.
>>
>> in short, what should we do for a accurate curr_volt? vp register may
>> not be correct, instead, the right steps to do:
>> find my current freq, check the OPP table for the voltage needed,
>> setup the voltage for it, update curr_volt for it. this is trust
>> worthy, rest are not.
>>
>> This is what set_init_voltage does, replicating that logic in
>> voltage.c makes no sense again to me.
>
> Agreed.
>
> Please update the changelog with a summary of how curr_volt is properly
> updated so it's clear that you're not removing something that isn't
> updated elsewhere.
>
Does the following sound any better?:
Blindly setting a 1.2v setting in the initial structure may not even
match the default voltages stored in the voltage table which are
supported for the domain. For example, OMAP3430 core domain does not use
1.2v and ends up generating a warning on the first transition.
Further, since omap2_set_init_voltage is called as part of the pm
framework's initialization sequence to configure the voltage required
for the current OPP, the call does(and has to) setup the system
voltage(curr_volt as a result) using the right mechanisms appropriate
for the system at that point of time. This also overrides initialization
we are currently doing in voltage.c making it redundant. So remove the
wrong and useless initialization.
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [PATCH 00/19] OMAP3+: introduce SR class 1.5
From: Kevin Hilman @ 2011-03-03 0:33 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap, Tony Lindgren
In-Reply-To: <1298116918-30744-1-git-send-email-nm@ti.com>
Hi Nishanth,
Nishanth Menon <nm@ti.com> writes:
> Hi,
> This series intends to introduce SmartReflex AVS Class 1.5 support which
> is now the recommended AVS class for usage in OMAP3630, OMAP4 an potentially
> in later generation of silicon as well. Smartreflex class 1.5 is a software
> controlled hardware calibration mechanism designed to improve dvfs latencies
> and system performance as well as helping bring in additional benefits to the
> system from h/w perspective. The corresponding patch has details on this class
> and the implementation as well.
In addition to some specific comments given on individual patches, I
a couple of general, nit-picky comments:
- Please capitalize acronyms throughout the
subjects/comments/changelogs. This series tends to mix lower-case and
upper case acronyms
- Please Cc linux-arm-kernel for patches intended for mainline. Because
of this, I didn't (yet) queue the ones I said I would queue.
- Please rebase this on top of my pm-core branch (or Paul's
integration-2.6.39 branch if you prefer.) There are some important
voltage related re-organization happening there that affects your code.
Kevin
> The series eventually results in OMAP343x based platforms using class3 and
> OMAP3630, OMAP4 platforms using class1.5 automatically without modifications
> or additions to board files.
>
> This series is Based on:
> a) k.org 2.6.38-rc5 (b2.6.38-rc5)
> b) The following branches Kevin Hilman's tree: (pm-base)
> 'pm/for_2.6.38/pm-fixes', 'pm/for_2.6.39/pm-misc' and 'pm/pm-wip/cpufreq'
> http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=summary
> c) sr-fixes: (sr-baseline)
> http://marc.info/?l=linux-omap&m=129783708019505&w=2
> http://marc.info/?l=linux-omap&m=129679846322563&w=2
>
> This series is also available at:
> git://gitorious.org/linux-omap-nm-sr/linux-omap-sr.git
> Branch: sr-1.5-v1
>
> Note: There is also a branch sr-dvfs-1.5 in my tree which contains the test
> version of code which is based off Vishwa's DVFS series which is currently
> being revamped. it may need few handtweaking for testing (esp selecting class
> at menuconfig level or by commenting out appropriate late_init).
>
> The series contains a bunch of bugfixes and improvements needed to introduce
> Smartreflex class 1.5.
> Nishanth Menon (19):
> omap3: hwmod: add smartreflex irqs
> omap3630: hwmod: sr: enable for higher ES
> omap3+: voltage: remove initial voltage
> omap3+: voltage: remove spurious pr_notice for debugfs
> omap3+: voltage: use IS_ERR_OR_NULL
> omap3+: voltage: use volt_data pointer instead values
> omap3+: voltage: add transdone apis
> omap3+: sr: make notify independent of class
> omap3+: sr: introduce class init,deinit and priv data
> omap3+: sr: fix cosmetic indentation
> omap3+: sr: call handler with interrupt disabled
> omap3+: sr: disable interrupt by default
> omap3+: sr: enable/disable SR only on need
> omap3+: sr: introduce notifiers flags
> omap3+: sr: introduce notifier_control
> omap3+: sr: disable spamming interrupts
> omap3+: sr: make enable path use volt_data pointer
> omap3630+: sr: add support for class 1.5
> omap3430: sr: class3: restrict cpu to run on
>
> arch/arm/mach-omap2/Makefile | 1 +
> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 25 +-
> arch/arm/mach-omap2/pm.c | 3 +-
> arch/arm/mach-omap2/smartreflex-class1p5.c | 556 +++++++++++++++++++++++++
> arch/arm/mach-omap2/smartreflex-class3.c | 21 +-
> arch/arm/mach-omap2/smartreflex.c | 249 ++++++++++--
> arch/arm/mach-omap2/voltage.c | 236 ++++++++---
> arch/arm/plat-omap/Kconfig | 17 +
> arch/arm/plat-omap/include/plat/smartreflex.h | 42 ++-
> arch/arm/plat-omap/include/plat/voltage.h | 36 ++-
> 10 files changed, 1062 insertions(+), 124 deletions(-)
> create mode 100644 arch/arm/mach-omap2/smartreflex-class1p5.c
>
> Testing performed:
> - basic boot tests on SDP3630 and SDP3430 - with bare series
> - Detailed dvfs tests with Viswha's series on SDP3430 and SDP3630
> - OMAP4 was'nt supported yet by dvfs series, so very restricted tests.
> - Build tests with and without each of SR classes, SMARTREFLEX enabled.
>
> Test Script: http://pastebin.mozilla.org/1080985
>
> I request any additional tests on Panda, BeagleXM, EVM platforms that folks
> may have. please feel free to comment and provide tested-by/Acked-by feedback
> before I take this ahead including l-a.
>
> Regards,
> Nishanth Menon
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox