| Message ID | 1375811376-49985-9-git-send-email-d-gerlach@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/06/2013 12:49 PM, Dave Gerlach wrote: [...] > + > +struct forced_standby_module am33xx_mod[] = { > + {.oh_name = "usb_otg_hs"}, > + {.oh_name = "tptc0"}, > + {.oh_name = "tptc1"}, > + {.oh_name = "tptc2"}, > + {.oh_name = "cpgmac0"}, > +}; > + [...] > + > +static int am33xx_pm_suspend(void) > +{ > + int i, j, ret = 0; > + > + int status = 0; > + struct platform_device *pdev; > + struct omap_device *od; > + > + /* > + * By default the following IPs do not have MSTANDBY asserted > + * which is necessary for PER domain transition. If the drivers > + * are not compiled into the kernel HWMOD code will not change the > + * state of the IPs if the IP was not never enabled. To ensure > + * that there no issues with or without the drivers being compiled > + * in the kernel, we forcefully put these IPs to idle. > + */ > + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { > + pdev = to_platform_device(am33xx_mod[i].dev); > + od = to_omap_device(pdev); > + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { > + omap_device_enable_hwmods(od); > + omap_device_idle_hwmods(od); > + } > + } Sorry, I dont like this one bit. this is the job of the suspend / resume handler dealing with the specific device. I recollect having a similar issue on OMAP5 where without USB driver, USB wont idle, hence we cant suspend either. is the solution to do a custom handling for specific nodes as above? is it even necessary - considering that in multiple suspend-resume iterations, do we need to "enable and idle" multiple times? Cant we do it just in hwmod/omap_device level where unbound devices are disabled? Is there absolutely no possible way to do this in a generic manner?
Nishanth, The issue here is that during a low-power transition the peripheral power domain loses context so the MSTANDBY config gets lost which is why the custom handling is needed on every cycle if there is no driver to handle it. I was unable to come up with a more generic way to handle this but I am certainly open for suggestions. Regards, Dave On 08/07/2013 11:22 AM, Nishanth Menon wrote: > On 08/06/2013 12:49 PM, Dave Gerlach wrote: > [...] > >> + >> +struct forced_standby_module am33xx_mod[] = { >> + {.oh_name = "usb_otg_hs"}, >> + {.oh_name = "tptc0"}, >> + {.oh_name = "tptc1"}, >> + {.oh_name = "tptc2"}, >> + {.oh_name = "cpgmac0"}, >> +}; >> + > [...] >> + >> +static int am33xx_pm_suspend(void) >> +{ >> + int i, j, ret = 0; >> + >> + int status = 0; >> + struct platform_device *pdev; >> + struct omap_device *od; >> + >> + /* >> + * By default the following IPs do not have MSTANDBY asserted >> + * which is necessary for PER domain transition. If the drivers >> + * are not compiled into the kernel HWMOD code will not change the >> + * state of the IPs if the IP was not never enabled. To ensure >> + * that there no issues with or without the drivers being compiled >> + * in the kernel, we forcefully put these IPs to idle. >> + */ >> + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { >> + pdev = to_platform_device(am33xx_mod[i].dev); >> + od = to_omap_device(pdev); >> + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { >> + omap_device_enable_hwmods(od); >> + omap_device_idle_hwmods(od); >> + } >> + } > > Sorry, I dont like this one bit. this is the job of the suspend / resume > handler dealing with the specific device. I recollect having a similar > issue on OMAP5 where without USB driver, USB wont idle, hence we cant > suspend either. is the solution to do a custom handling for specific > nodes as above? is it even necessary - considering that in multiple > suspend-resume iterations, do we need to "enable and idle" multiple > times? Cant we do it just in hwmod/omap_device level where unbound > devices are disabled? Is there absolutely no possible way to do this in > a generic manner? >
On 08/07/2013 01:12 PM, Dave Gerlach wrote: > On 08/07/2013 11:22 AM, Nishanth Menon wrote: >> On 08/06/2013 12:49 PM, Dave Gerlach wrote: >> [...] >> >>> + >>> +struct forced_standby_module am33xx_mod[] = { >>> + {.oh_name = "usb_otg_hs"}, >>> + {.oh_name = "tptc0"}, >>> + {.oh_name = "tptc1"}, >>> + {.oh_name = "tptc2"}, >>> + {.oh_name = "cpgmac0"}, >>> +}; >>> + >> [...] >>> + >>> +static int am33xx_pm_suspend(void) >>> +{ >>> + int i, j, ret = 0; >>> + >>> + int status = 0; >>> + struct platform_device *pdev; >>> + struct omap_device *od; >>> + >>> + /* >>> + * By default the following IPs do not have MSTANDBY asserted >>> + * which is necessary for PER domain transition. If the drivers >>> + * are not compiled into the kernel HWMOD code will not change the >>> + * state of the IPs if the IP was not never enabled. To ensure >>> + * that there no issues with or without the drivers being compiled >>> + * in the kernel, we forcefully put these IPs to idle. >>> + */ >>> + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { >>> + pdev = to_platform_device(am33xx_mod[i].dev); >>> + od = to_omap_device(pdev); >>> + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { >>> + omap_device_enable_hwmods(od); >>> + omap_device_idle_hwmods(od); >>> + } >>> + } >> >> Sorry, I dont like this one bit. this is the job of the suspend / resume >> handler dealing with the specific device. I recollect having a similar >> issue on OMAP5 where without USB driver, USB wont idle, hence we cant >> suspend either. is the solution to do a custom handling for specific >> nodes as above? is it even necessary - considering that in multiple >> suspend-resume iterations, do we need to "enable and idle" multiple >> times? Cant we do it just in hwmod/omap_device level where unbound >> devices are disabled? Is there absolutely no possible way to do this in >> a generic manner? >> > > The issue here is that during a low-power transition the peripheral > power domain loses context so the MSTANDBY config gets lost which is why > the custom handling is needed on every cycle if there is no driver to > handle it. I was unable to come up with a more generic way to handle > this but I am certainly open for suggestions. > If the dts entry for device status = "disabled" you should not have these even registered right? kinda makes me wonder if M3 could do something about it -> since they are minimal? When they are not, I think the generic omap_device_pm_domain ops does not apply for these devices due to the quirks? Making a flag for these improper devices should let omap_device abstraction setup different pm_domain configuration also shut them up at boot as well (if un-bound). that will let a generic device solution scale out to more SoCs without custom handling, perhaps? just an quick idea - not sure about validity about the approach without digging in more. usually, we try to ensure system is exactly the same state as it entered -> so at boot, we shut down everything, on wakeup, if unexpected things like these are present, we shut them down again (in .finish handler).
On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach <d-gerlach@ti.com> wrote: > From: Vaibhav Bedia <vaibhav.bedia@ti.com> > > AM335x supports various low power modes as documented > in section 8.1.4.3 of the AM335x TRM which is available > @ http://www.ti.com/litv/pdf/spruh73f > > DeepSleep0 mode offers the lowest power mode with limited > wakeup sources without a system reboot and is mapped as > the suspend state in the kernel. In this state, MPU and > PER domains are turned off with the internal RAM held in > retention to facilitate resume process. As part of the boot > process, the assembly code is copied over to OCMCRAM using > the OMAP SRAM code. > > AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU > in DeepSleep0 entry and exit. WKUP_M3 takes care of the > clockdomain and powerdomain transitions based on the > intended low power state. MPU needs to load the appropriate > WKUP_M3 binary onto the WKUP_M3 memory space before it can > leverage any of the PM features like DeepSleep. > > The IPC mechanism between MPU and WKUP_M3 uses a mailbox > sub-module and 8 IPC registers in the Control module. MPU > uses the assigned Mailbox for issuing an interrupt to > WKUP_M3 which then goes and checks the IPC registers for > the payload. WKUP_M3 has the ability to trigger on interrupt > to MPU by executing the "sev" instruction. > > In the current implementation when the suspend process > is initiated MPU interrupts the WKUP_M3 to let it know about > the intent of entering DeepSleep0 and waits for an ACK. When > the ACK is received MPU continues with its suspend process > to suspend all the drivers and then jumps to assembly in > OCMC RAM. The assembly code puts the PLLs in bypass, puts the > external RAM in self-refresh mode and then finally execute the > WFI instruction. Execution of the WFI instruction triggers another > interrupt to the WKUP_M3 which then continues wiht the power down > sequence wherein the clockdomain and powerdomain transition takes > place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt > lines for the wakeup sources. WFI execution on WKUP_M3 causes the > hardware to disable the main oscillator of the SoC. > > When a wakeup event occurs, WKUP_M3 starts the power-up > sequence by switching on the power domains and finally > enabling the clock to MPU. Since the MPU gets powered down > as part of the sleep sequence in the resume path ROM code > starts executing. The ROM code detects a wakeup from sleep > and then jumps to the resume location in OCMC which was > populated in one of the IPC registers as part of the suspend > sequence. > > The low level code in OCMC relocks the PLLs, enables access > to external RAM and then jumps to the cpu_resume code of > the kernel to finish the resume process. > > Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com> > Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > Cc: Tony Lingren <tony@atomide.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Benoit Cousson <benoit.cousson@linaro.org> > Cc: Paul Walmsley <paul@pwsan.com> > Cc: Kevin Hilman <khilman@linaro.org> Reviewed-by: Russ Dill <russ.dill@ti.com> In response to Nishanth's comments about the list of omap modules to idle, I saw that, but it looks like its only for devices that no device driver ever binds do, eg, the support is not built into the kernel or the module is currently not loaded. Hence I don't think there is any suspend/resume function that gets called. In reference to the M3 handling it, the M3 wouldn't know which devices have a driver bound and which don't. > --- > arch/arm/mach-omap2/pm33xx.c | 474 +++++++++++++++++++++++++++++++++++++++++ > arch/arm/mach-omap2/pm33xx.h | 77 +++++++ > arch/arm/mach-omap2/wkup_m3.c | 183 ++++++++++++++++ > 3 files changed, 734 insertions(+) > create mode 100644 arch/arm/mach-omap2/pm33xx.c > create mode 100644 arch/arm/mach-omap2/pm33xx.h > create mode 100644 arch/arm/mach-omap2/wkup_m3.c > > diff --git a/arch/arm/mach-omap2/pm33xx.c b/arch/arm/mach-omap2/pm33xx.c > new file mode 100644 > index 0000000..d291c76 > --- /dev/null > +++ b/arch/arm/mach-omap2/pm33xx.c > @@ -0,0 +1,474 @@ > +/* > + * AM33XX Power Management Routines > + * > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ > + * Vaibhav Bedia <vaibhav.bedia@ti.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/cpu.h> > +#include <linux/err.h> > +#include <linux/firmware.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/sched.h> > +#include <linux/suspend.h> > +#include <linux/completion.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/ti_emif.h> > +#include <linux/omap-mailbox.h> > + > +#include <asm/suspend.h> > +#include <asm/proc-fns.h> > +#include <asm/sizes.h> > +#include <asm/fncpy.h> > +#include <asm/system_misc.h> > + > +#include "pm.h" > +#include "cm33xx.h" > +#include "pm33xx.h" > +#include "control.h" > +#include "common.h" > +#include "clockdomain.h" > +#include "powerdomain.h" > +#include "omap_hwmod.h" > +#include "omap_device.h" > +#include "soc.h" > +#include "sram.h" > + > +static void __iomem *am33xx_emif_base; > +static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm; > +static struct clockdomain *gfx_l4ls_clkdm; > + > +struct wakeup_src wakeups[] = { > + {.irq_nr = 35, .src = "USB0_PHY"}, > + {.irq_nr = 36, .src = "USB1_PHY"}, > + {.irq_nr = 40, .src = "I2C0"}, > + {.irq_nr = 41, .src = "RTC Timer"}, > + {.irq_nr = 42, .src = "RTC Alarm"}, > + {.irq_nr = 43, .src = "Timer0"}, > + {.irq_nr = 44, .src = "Timer1"}, > + {.irq_nr = 45, .src = "UART"}, > + {.irq_nr = 46, .src = "GPIO0"}, > + {.irq_nr = 48, .src = "MPU_WAKE"}, > + {.irq_nr = 49, .src = "WDT0"}, > + {.irq_nr = 50, .src = "WDT1"}, > + {.irq_nr = 51, .src = "ADC_TSC"}, > +}; > + > +struct forced_standby_module am33xx_mod[] = { > + {.oh_name = "usb_otg_hs"}, > + {.oh_name = "tptc0"}, > + {.oh_name = "tptc1"}, > + {.oh_name = "tptc2"}, > + {.oh_name = "cpgmac0"}, > +}; > + > +static struct am33xx_pm_context *am33xx_pm; > + > +static DECLARE_COMPLETION(am33xx_pm_sync); > + > +static void (*am33xx_do_wfi_sram)(struct am33xx_suspend_params *); > + > +static struct am33xx_suspend_params susp_params; > + > +#ifdef CONFIG_SUSPEND > + > +static int am33xx_do_sram_idle(long unsigned int unused) > +{ > + am33xx_do_wfi_sram(&susp_params); > + return 0; > +} > + > +static int am33xx_pm_suspend(void) > +{ > + int i, j, ret = 0; > + > + int status = 0; > + struct platform_device *pdev; > + struct omap_device *od; > + > + /* > + * By default the following IPs do not have MSTANDBY asserted > + * which is necessary for PER domain transition. If the drivers > + * are not compiled into the kernel HWMOD code will not change the > + * state of the IPs if the IP was not never enabled. To ensure > + * that there no issues with or without the drivers being compiled > + * in the kernel, we forcefully put these IPs to idle. > + */ > + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { > + pdev = to_platform_device(am33xx_mod[i].dev); > + od = to_omap_device(pdev); > + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { > + omap_device_enable_hwmods(od); > + omap_device_idle_hwmods(od); > + } > + } > + > + /* Try to put GFX to sleep */ > + omap_set_pwrdm_state(gfx_pwrdm, PWRDM_POWER_OFF); > + ret = cpu_suspend(0, am33xx_do_sram_idle); > + > + status = pwrdm_read_prev_pwrst(gfx_pwrdm); > + if (status != PWRDM_POWER_OFF) > + pr_err("PM: GFX domain did not transition\n"); > + else > + pr_info("PM: GFX domain entered low power state\n"); > + > + /* > + * BUG: GFX_L4LS clock domain needs to be woken up to > + * ensure thet L4LS clock domain does not get stuck in transition > + * If that happens L3 module does not get disabled, thereby leading > + * to PER power domain transition failing > + */ > + clkdm_wakeup(gfx_l4ls_clkdm); > + clkdm_sleep(gfx_l4ls_clkdm); > + > + if (ret) { > + pr_err("PM: Kernel suspend failure\n"); > + } else { > + i = am33xx_pm_status(); > + switch (i) { > + case 0: > + pr_info("PM: Successfully put all powerdomains to target state\n"); > + > + /* > + * The PRCM registers on AM335x do not contain previous state > + * information like those present on OMAP4 so we must manually > + * indicate transition so state counters are properly incremented > + */ > + pwrdm_post_transition(mpu_pwrdm); > + pwrdm_post_transition(per_pwrdm); > + break; > + case 1: > + pr_err("PM: Could not transition all powerdomains to target state\n"); > + ret = -1; > + break; > + default: > + pr_err("PM: CM3 returned unknown result :(\nStatus = %d\n", i); > + ret = -1; > + } > + > + /* print the wakeup reason */ > + i = am33xx_pm_wake_src(); > + for (j = 0; j < ARRAY_SIZE(wakeups); j++) { > + if (wakeups[j].irq_nr == i) { > + pr_info("PM: Wakeup source %s\n", wakeups[j].src); > + break; > + } > + } > + > + if (j == ARRAY_SIZE(wakeups)) > + pr_info("PM: Unknown wakeup source %d!\n", i); > + } > + > + return ret; > +} > + > +static int am33xx_pm_enter(suspend_state_t suspend_state) > +{ > + int ret = 0; > + > + switch (suspend_state) { > + case PM_SUSPEND_STANDBY: > + case PM_SUSPEND_MEM: > + ret = am33xx_pm_suspend(); > + break; > + default: > + ret = -EINVAL; > + } > + > + return ret; > +} > + > +/* returns the error code from msg_send - 0 for success, failure otherwise */ > +static int am33xx_ping_wkup_m3(void) > +{ > + int ret = 0; > + > + /* > + * Write a dummy message to the mailbox in order to trigger the RX > + * interrupt to alert the M3 that data is available in the IPC > + * registers. > + */ > + ret = omap_mbox_msg_send(am33xx_pm->mbox, 0xABCDABCD); > + > + return ret; > +} > + > +static void am33xx_m3_state_machine_reset(void) > +{ > + int i; > + > + am33xx_pm->ipc.sleep_mode = IPC_CMD_RESET; > + > + am33xx_pm_ipc_cmd(&am33xx_pm->ipc); > + > + am33xx_pm->state = M3_STATE_MSG_FOR_RESET; > + > + pr_info("PM: Sending message for resetting M3 state machine\n"); > + > + if (!am33xx_ping_wkup_m3()) { > + i = wait_for_completion_timeout(&am33xx_pm_sync, > + msecs_to_jiffies(500)); > + if (WARN(i == 0, "PM: MPU<->CM3 sync failure\n")) > + am33xx_pm->state = M3_STATE_UNKNOWN; > + } else { > + pr_warn("PM: Unable to ping CM3\n"); > + } > +} > + > +static int am33xx_pm_begin(suspend_state_t state) > +{ > + int i; > + > + cpu_idle_poll_ctrl(true); > + > + am33xx_pm->ipc.sleep_mode = IPC_CMD_DS0; > + am33xx_pm->ipc.param1 = DS_IPC_DEFAULT; > + am33xx_pm->ipc.param2 = DS_IPC_DEFAULT; > + > + am33xx_pm_ipc_cmd(&am33xx_pm->ipc); > + > + am33xx_pm->state = M3_STATE_MSG_FOR_LP; > + > + pr_info("PM: Sending message for entering DeepSleep mode\n"); > + > + if (!am33xx_ping_wkup_m3()) { > + i = wait_for_completion_timeout(&am33xx_pm_sync, > + msecs_to_jiffies(500)); > + if (WARN(i == 0, "PM: MPU<->CM3 sync failure\n")) > + return -1; > + } else { > + pr_warn("PM: Unable to ping CM3\n"); > + } > + > + return 0; > +} > + > +static void am33xx_pm_end(void) > +{ > + am33xx_m3_state_machine_reset(); > + > + cpu_idle_poll_ctrl(false); > + > + return; > +} > + > +static struct platform_suspend_ops am33xx_pm_ops = { > + .begin = am33xx_pm_begin, > + .end = am33xx_pm_end, > + .enter = am33xx_pm_enter, > +}; > + > +/* > + * Dummy notifier for the mailbox > + */ > + > +static int wkup_mbox_msg(struct notifier_block *self, unsigned long len, > + void *msg) > +{ > + return 0; > +} > + > +static struct notifier_block wkup_mbox_notifier = { > + .notifier_call = wkup_mbox_msg, > +}; > + > +void am33xx_txev_handler(void) > +{ > + switch (am33xx_pm->state) { > + case M3_STATE_RESET: > + am33xx_pm->state = M3_STATE_INITED; > + am33xx_pm->ver = am33xx_pm_version_get(); > + if (am33xx_pm->ver == M3_VERSION_UNKNOWN || > + am33xx_pm->ver < M3_BASELINE_VERSION) { > + pr_warn("PM: CM3 Firmware Version %x not supported\n", > + am33xx_pm->ver); > + } else { > + pr_info("PM: CM3 Firmware Version = 0x%x\n", > + am33xx_pm->ver); > + am33xx_pm_ops.valid = suspend_valid_only_mem; > + } > + break; > + case M3_STATE_MSG_FOR_RESET: > + am33xx_pm->state = M3_STATE_INITED; > + complete(&am33xx_pm_sync); > + break; > + case M3_STATE_MSG_FOR_LP: > + complete(&am33xx_pm_sync); > + break; > + case M3_STATE_UNKNOWN: > + pr_warn("PM: Unknown CM3 State\n"); > + } > + > + return; > +} > + > +static void am33xx_pm_firmware_cb(const struct firmware *fw, void *context) > +{ > + struct am33xx_pm_context *am33xx_pm = context; > + int ret = 0; > + > + /* no firmware found */ > + if (!fw) { > + pr_err("PM: request_firmware failed\n"); > + return; > + } > + > + wkup_m3_copy_code(fw->data, fw->size); > + > + wkup_m3_register_txev_handler(am33xx_txev_handler); > + > + pr_info("PM: Copied the M3 firmware to UMEM\n"); > + > + /* > + * Invalidate M3 firmware version before hardreset. > + * Write invalid version in lower 4 nibbles of parameter > + * register (ipc_regs + 0x8). > + */ > + am33xx_pm_version_clear(); > + > + am33xx_pm->state = M3_STATE_RESET; > + > + ret = wkup_m3_prepare(); > + if (ret) { > + pr_err("PM: Could not prepare WKUP_M3\n"); > + return; > + } > + > + /* Physical resume address to be used by ROM code */ > + am33xx_pm->ipc.resume_addr = (AM33XX_OCMC_END - > + am33xx_do_wfi_sz + am33xx_resume_offset + 0x4); > + > + am33xx_pm->mbox = omap_mbox_get("wkup_m3", &wkup_mbox_notifier); > + > + if (IS_ERR(am33xx_pm->mbox)) { > + ret = -EBUSY; > + pr_err("PM: IPC Request for A8->M3 Channel failed!\n"); > + return; > + } else { > + suspend_set_ops(&am33xx_pm_ops); > + } > + > + return; > +} > + > +#endif /* CONFIG_SUSPEND */ > + > +/* > + * Push the minimal suspend-resume code to SRAM > + */ > +void am33xx_push_sram_idle(void) > +{ > + am33xx_do_wfi_sram = (void *)omap_sram_push > + (am33xx_do_wfi, am33xx_do_wfi_sz); > +} > + > +static int __init am33xx_map_emif(void) > +{ > + am33xx_emif_base = ioremap(AM33XX_EMIF_BASE, SZ_32K); > + > + if (!am33xx_emif_base) > + return -ENOMEM; > + > + return 0; > +} > + > +int __init am33xx_pm_init(void) > +{ > + int ret; > + u32 temp; > + struct device_node *np; > + int i; > + > + if (!soc_is_am33xx()) > + return -ENODEV; > + > + pr_info("Power Management for AM33XX family\n"); > + > + /* > + * By default the following IPs do not have MSTANDBY asserted > + * which is necessary for PER domain transition. If the drivers > + * are not compiled into the kernel HWMOD code will not change the > + * state of the IPs if the IP was not never enabled > + */ > + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) > + am33xx_mod[i].dev = omap_device_get_by_hwmod_name(am33xx_mod[i].oh_name); > + > + gfx_pwrdm = pwrdm_lookup("gfx_pwrdm"); > + per_pwrdm = pwrdm_lookup("per_pwrdm"); > + mpu_pwrdm = pwrdm_lookup("mpu_pwrdm"); > + > + gfx_l4ls_clkdm = clkdm_lookup("gfx_l4ls_gfx_clkdm"); > + > + if ((!gfx_pwrdm) || (!per_pwrdm) || (!mpu_pwrdm) || (!gfx_l4ls_clkdm)) { > + ret = -ENODEV; > + goto err; > + } > + > + am33xx_pm = kzalloc(sizeof(*am33xx_pm), GFP_KERNEL); > + if (!am33xx_pm) { > + pr_err("Memory allocation failed\n"); > + ret = -ENOMEM; > + goto err; > + } > + > + ret = am33xx_map_emif(); > + if (ret) { > + pr_err("PM: Could not ioremap EMIF\n"); > + goto err; > + } > + /* Determine Memory Type */ > + temp = readl(am33xx_emif_base + EMIF_SDRAM_CONFIG); > + temp = (temp & SDRAM_TYPE_MASK) >> SDRAM_TYPE_SHIFT; > + /* Parameters to pass to aseembly code */ > + susp_params.emif_addr_virt = am33xx_emif_base; > + susp_params.dram_sync = am33xx_dram_sync; > + susp_params.mem_type = temp; > + am33xx_pm->ipc.param3 = temp; > + > + np = of_find_compatible_node(NULL, NULL, "ti,am3353-wkup-m3"); > + if (np) { > + if (of_find_property(np, "ti,needs_vtt_toggle", NULL) && > + (!(of_property_read_u32(np, "vtt-gpio-pin", > + &temp)))) { > + if (temp >= 0 && temp <= 31) > + am33xx_pm->ipc.param3 |= > + ((1 << VTT_STAT_SHIFT) | > + (temp << VTT_GPIO_PIN_SHIFT)); > + } > + } > + > + (void) clkdm_for_each(omap_pm_clkdms_setup, NULL); > + > + /* CEFUSE domain can be turned off post bootup */ > + cefuse_pwrdm = pwrdm_lookup("cefuse_pwrdm"); > + if (cefuse_pwrdm) > + omap_set_pwrdm_state(cefuse_pwrdm, PWRDM_POWER_OFF); > + else > + pr_err("PM: Failed to get cefuse_pwrdm\n"); > + > +#ifdef CONFIG_SUSPEND > + pr_info("PM: Trying to load am335x-pm-firmware.bin"); > + > + /* We don't want to delay boot */ > + request_firmware_nowait(THIS_MODULE, 0, "am335x-pm-firmware.bin", > + NULL, GFP_KERNEL, am33xx_pm, > + am33xx_pm_firmware_cb); > +#endif /* CONFIG_SUSPEND */ > + > +err: > + return ret; > +} > diff --git a/arch/arm/mach-omap2/pm33xx.h b/arch/arm/mach-omap2/pm33xx.h > new file mode 100644 > index 0000000..befdd11 > --- /dev/null > +++ b/arch/arm/mach-omap2/pm33xx.h > @@ -0,0 +1,77 @@ > +/* > + * AM33XX Power Management Routines > + * > + * Copyright (C) 2012 Texas Instruments Inc. > + * Vaibhav Bedia <vaibhav.bedia@ti.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef __ARCH_ARM_MACH_OMAP2_PM33XX_H > +#define __ARCH_ARM_MACH_OMAP2_PM33XX_H > + > +#include "control.h" > + > +#ifndef __ASSEMBLER__ > + > +struct am33xx_pm_context { > + struct am33xx_ipc_data ipc; > + struct firmware *firmware; > + struct omap_mbox *mbox; > + u8 state; > + u32 ver; > +}; > + > +/* > + * Params passed to suspend routine > + * > + * Since these are used to load into registers by suspend code, > + * entries here must always be in sync with the suspend code > + * in arm/mach-omap2/sleep33xx.S > + */ > +struct am33xx_suspend_params { > + void __iomem *emif_addr_virt; > + u32 mem_type; > + void __iomem *dram_sync; > +}; > + > +struct wakeup_src { > + int irq_nr; > + char src[10]; > +}; > + > +struct forced_standby_module { > + char oh_name[15]; > + struct device *dev; > +}; > + > +int wkup_m3_copy_code(const u8 *data, size_t size); > +int wkup_m3_prepare(void); > +void wkup_m3_register_txev_handler(void (*txev_handler)(void)); > + > +#endif > + > +#define IPC_CMD_DS0 0x3 > +#define IPC_CMD_RESET 0xe > +#define DS_IPC_DEFAULT 0xffffffff > +#define M3_VERSION_UNKNOWN 0x0000ffff > +#define M3_BASELINE_VERSION 0x21 > + > +#define M3_STATE_UNKNOWN 0 > +#define M3_STATE_RESET 1 > +#define M3_STATE_INITED 2 > +#define M3_STATE_MSG_FOR_LP 3 > +#define M3_STATE_MSG_FOR_RESET 4 > + > +#define AM33XX_OCMC_END 0x40310000 > +#define AM33XX_EMIF_BASE 0x4C000000 > + > +#define MEM_TYPE_DDR2 2 > + > +#endif > diff --git a/arch/arm/mach-omap2/wkup_m3.c b/arch/arm/mach-omap2/wkup_m3.c > new file mode 100644 > index 0000000..8eaa7f3 > --- /dev/null > +++ b/arch/arm/mach-omap2/wkup_m3.c > @@ -0,0 +1,183 @@ > +/* > + * AM33XX Power Management Routines > + * > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ > + * Vaibhav Bedia <vaibhav.bedia@ti.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/cpu.h> > +#include <linux/err.h> > +#include <linux/firmware.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/of.h> > + > +#include "pm33xx.h" > +#include "control.h" > +#include "omap_device.h" > +#include "soc.h" > + > +struct wkup_m3_context { > + struct device *dev; > + void __iomem *code; > + void (*txev_handler)(void); > +}; > + > +struct wkup_m3_context *wkup_m3; > + > +int wkup_m3_copy_code(const u8 *data, size_t size) > +{ > + if (size > SZ_16K) > + return -ENOMEM; > + > + memcpy_toio(wkup_m3->code, data, size); > + > + return 0; > +} > + > + > +void wkup_m3_register_txev_handler(void (*txev_handler)(void)) > +{ > + wkup_m3->txev_handler = txev_handler; > +} > + > +/* have platforms do what they want in atomic context over here? */ > +static irqreturn_t wkup_m3_txev_handler(int irq, void *unused) > +{ > + am33xx_txev_eoi(); > + > + /* callback to be executed in atomic context */ > + /* return 0 implies IRQ_HANDLED else IRQ_NONE */ > + wkup_m3->txev_handler(); > + > + am33xx_txev_enable(); > + > + return IRQ_HANDLED; > +} > + > +int wkup_m3_prepare(void) > +{ > + struct platform_device *pdev = to_platform_device(wkup_m3->dev); > + > + /* check that the code is loaded */ > + omap_device_deassert_hardreset(pdev, "wkup_m3"); > + > + return 0; > +} > + > +static int wkup_m3_probe(struct platform_device *pdev) > +{ > + int irq, ret = 0; > + struct resource *mem; > + > + pm_runtime_enable(&pdev->dev); > + > + ret = pm_runtime_get_sync(&pdev->dev); > + if (IS_ERR_VALUE(ret)) { > + dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n"); > + return ret; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (!irq) { > + dev_err(wkup_m3->dev, "no irq resource\n"); > + ret = -ENXIO; > + goto err; > + } > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!mem) { > + dev_err(wkup_m3->dev, "no memory resource\n"); > + ret = -ENXIO; > + goto err; > + } > + > + wkup_m3 = kzalloc(sizeof(*wkup_m3), GFP_KERNEL); > + if (!wkup_m3) { > + pr_err("Memory allocation failed\n"); > + ret = -ENOMEM; > + goto err; > + } > + > + wkup_m3->dev = &pdev->dev; > + > + wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, mem); > + if (!wkup_m3->code) { > + dev_err(wkup_m3->dev, "could not ioremap\n"); > + ret = -EADDRNOTAVAIL; > + goto err; > + } > + > + ret = devm_request_irq(wkup_m3->dev, irq, wkup_m3_txev_handler, > + IRQF_DISABLED, "wkup_m3_txev", NULL); > + if (ret) { > + dev_err(wkup_m3->dev, "request_irq failed\n"); > + goto err; > + } > + > +err: > + return ret; > +} > + > +static int wkup_m3_remove(struct platform_device *pdev) > +{ > + return 0; > +} > + > +static struct of_device_id wkup_m3_dt_ids[] = { > + { .compatible = "ti,am3353-wkup-m3" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, wkup_m3_dt_ids); > + > +static int wkup_m3_rpm_suspend(struct device *dev) > +{ > + return -EBUSY; > +} > + > +static int wkup_m3_rpm_resume(struct device *dev) > +{ > + return 0; > +} > + > +static const struct dev_pm_ops wkup_m3_ops = { > + SET_RUNTIME_PM_OPS(wkup_m3_rpm_suspend, wkup_m3_rpm_resume, NULL) > +}; > + > +static struct platform_driver wkup_m3_driver = { > + .probe = wkup_m3_probe, > + .remove = wkup_m3_remove, > + .driver = { > + .name = "wkup_m3", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(wkup_m3_dt_ids), > + .pm = &wkup_m3_ops, > + }, > +}; > + > +static __init int wkup_m3_init(void) > +{ > + return platform_driver_register(&wkup_m3_driver); > +} > + > +static __exit void wkup_m3_exit(void) > +{ > + platform_driver_unregister(&wkup_m3_driver); > +} > +omap_postcore_initcall(wkup_m3_init); > +module_exit(wkup_m3_exit); > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/08/2013 03:45 AM, Russ Dill wrote: > In reference to > the M3 handling it, the M3 wouldn't know which devices have a driver > bound and which don't. Does it need to? M3 firmware can pretty much define "I will force the device into low power state, and if the drivers dont handle things properly, fix the darned driver". M3 behavior should be considered as a "hardware" as far as Linux running on MPU is concerned, and firmware helps change the behavior by accounting for SoC quirks. *if* we have ability to handle this in the firmware, there is no need to carry this in Linux.
$subject and patch don't match. On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: > On 08/08/2013 03:45 AM, Russ Dill wrote: >> In reference to >> the M3 handling it, the M3 wouldn't know which devices have a driver >> bound and which don't. > Does it need to? M3 firmware can pretty much define "I will force the device into low power state, and if the drivers dont handle things properly, fix the darned driver". M3 behavior should be considered as a "hardware" as far as Linux running on MPU is concerned, and firmware helps change the behavior by accounting for SoC quirks. *if* we have ability to handle this in the firmware, there is no need to carry this in Linux. > I agree with Nishant. I don't like this patch and IIRC, I gave same comment in the last version. Linux need not know about all such firmware quirks. Also all these M3 specific stuff, should be done somewhere else. Probably having a small M3 driver won't be a bad idea. Regards, Santosh
On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: > $subject and patch don't match. > > On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >> On 08/08/2013 03:45 AM, Russ Dill wrote: >>> In reference to >>> the M3 handling it, the M3 wouldn't know which devices have a driver >>> bound and which don't. >> Does it need to? M3 firmware can pretty much define "I will force the device into low power state, and if the drivers dont handle things properly, fix the darned driver". M3 behavior should be considered as a "hardware" as far as Linux running on MPU is concerned, and firmware helps change the behavior by accounting for SoC quirks. *if* we have ability to handle this in the firmware, there is no need to carry this in Linux. >> > I agree with Nishant. I don't like this patch and IIRC, I gave same > comment in the last version. Linux need not know about all such firmware > quirks. Also all these M3 specific stuff, should be done somewhere > else. Probably having a small M3 driver won't be a bad idea. > > Regards, > Santosh > I am not opposed to doing it this way and letting the M3 firmware handle idling these modules, however the one concern raised in the last series is that an approach that does not acknowledge drivers will hide driver PM bugs. I suppose as long as I make sure to document that the devices are being idled by the M3 firmware this may not be an issue. I will look into implementing this. Regards, Dave
On 08/08/2013 11:06 AM, Dave Gerlach wrote: > On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >> $subject and patch don't match. >> >> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>> In reference to >>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>> bound and which don't. >>> Does it need to? M3 firmware can pretty much define "I will force the >>> device into low power state, and if the drivers dont handle things >>> properly, fix the darned driver". M3 behavior should be considered as >>> a "hardware" as far as Linux running on MPU is concerned, and >>> firmware helps change the behavior by accounting for SoC quirks. *if* >>> we have ability to handle this in the firmware, there is no need to >>> carry this in Linux. >>> >> I agree with Nishant. I don't like this patch and IIRC, I gave same >> comment in the last version. Linux need not know about all such firmware >> quirks. Also all these M3 specific stuff, should be done somewhere >> else. Probably having a small M3 driver won't be a bad idea. >> >> Regards, >> Santosh >> > > I am not opposed to doing it this way and letting the M3 firmware handle > idling these modules, however the one concern raised in the last series > is that an approach that does not acknowledge drivers will hide driver > PM bugs. I suppose as long as I make sure to document that the devices > are being idled by the M3 firmware this may not be an issue. I will look > into implementing this. yes, but doing it in M3 will ensure it is a "hardware behavior" - from debug perspective, you could make M3 firmware "release mode" - which it is stringent in terms of forcing all down to target state, OR "debug" where it does nothing - thus helping pick up driver bugs. With M3 in the picture, we now have an awesome flexibility of "defining what the hardware behavior should be" - that allows us to have lesser code to carry in kernel- especially ones(like quirks) that does not really add value from power saving strategies.
Dave Gerlach <d-gerlach@ti.com> writes: > On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >> $subject and patch don't match. >> >> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>> In reference to >>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>> bound and which don't. >>> Does it need to? M3 firmware can pretty much define "I will force >>> the device into low power state, and if the drivers dont handle >>> things properly, fix the darned driver". M3 behavior should be >>> considered as a "hardware" as far as Linux running on MPU is >>> concerned, and firmware helps change the behavior by accounting for >>> SoC quirks. *if* we have ability to handle this in the firmware, >>> there is no need to carry this in Linux. >>> >> I agree with Nishant. I don't like this patch and IIRC, I gave same >> comment in the last version. Linux need not know about all such firmware >> quirks. Also all these M3 specific stuff, should be done somewhere >> else. Probably having a small M3 driver won't be a bad idea. >> >> Regards, >> Santosh >> > > I am not opposed to doing it this way and letting the M3 firmware > handle idling these modules, however the one concern raised in the > last series is that an approach that does not acknowledge drivers will > hide driver PM bugs. I suppose as long as I make sure to document that > the devices are being idled by the M3 firmware this may not be an > issue. I will look into implementing this. No, please don't start idling devices in firmware that are otherwise managed by Linux. Keep the firmware simple and dumb. Linux is managing these devices, it should manage their bugs too. This is not just about idling devices. This is about handling broken IP blocks whose power-on reset state does not allow the the powerdomain to reach its target state. That's just bad hardware design. That being said, IMO, the kernel (specifically omap_device) should handle this, and it should be rather easy to do in the omap_device layer and keep the SoC suspend/resume core code simple and ignorant of these "quirks." AFAICT, there's no reason these quirks need to be dealt with immediatly on suspend. A slight delay should be fine, as long as it's before the next suspend/idle attempt, right? Given that, what we need to do (and by we, I mean you) is to flag all broken IP blocks, and let omap_device handle them in a suspend/resume notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.) That will keep things contained to the omap_device/hwmod level and allow flexiblity for future broken SoCs where the list of broken IP blocks is different. Though surely this broken hardware doesn't exist in AM4xxx because someone noticed this early on and pointed out that it should be fixed in hardware, right? ;) Kevin
On 08/08/2013 04:14 PM, Kevin Hilman wrote: > Dave Gerlach <d-gerlach@ti.com> writes: > >> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>> $subject and patch don't match. >>> >>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>> In reference to >>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>> bound and which don't. >>>> Does it need to? M3 firmware can pretty much define "I will force >>>> the device into low power state, and if the drivers dont handle >>>> things properly, fix the darned driver". M3 behavior should be >>>> considered as a "hardware" as far as Linux running on MPU is >>>> concerned, and firmware helps change the behavior by accounting for >>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>> there is no need to carry this in Linux. >>>> >>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>> comment in the last version. Linux need not know about all such firmware >>> quirks. Also all these M3 specific stuff, should be done somewhere >>> else. Probably having a small M3 driver won't be a bad idea. >> >> I am not opposed to doing it this way and letting the M3 firmware >> handle idling these modules, however the one concern raised in the >> last series is that an approach that does not acknowledge drivers will >> hide driver PM bugs. I suppose as long as I make sure to document that >> the devices are being idled by the M3 firmware this may not be an >> issue. I will look into implementing this. > > No, please don't start idling devices in firmware that are otherwise > managed by Linux. Keep the firmware simple and dumb. Linux is managing > these devices, it should manage their bugs too. > > This is not just about idling devices. This is about handling broken IP > blocks whose power-on reset state does not allow the the powerdomain to > reach its target state. That's just bad hardware design. Right, this is where M3 can help -> provide a consistent state for linux kernel to work with. by the fact that we want to keep majority of the power code inside master CPU, we are just letting M3 help us with nothing major at all.. tiny stuff like these can help "fix" the hardware design quirks by hiding it behind the firmware and modifying the hardware behavior. I know it breaks the purity of role, but as the next evolution, we might want to consider M3 something like an "accelerator" for power management activity.. (not saying it is that fast.. but conceptually). > > That being said, IMO, the kernel (specifically omap_device) should > handle this, and it should be rather easy to do in the omap_device layer > and keep the SoC suspend/resume core code simple and ignorant of these > "quirks." > > AFAICT, there's no reason these quirks need to be dealt with immediatly > on suspend. A slight delay should be fine, as long as it's before the > next suspend/idle attempt, right? > > Given that, what we need to do (and by we, I mean you) is to flag all > broken IP blocks, and let omap_device handle them in a suspend/resume > notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.) yes - that is the alternate that comes to mind.
Nishanth Menon <nm@ti.com> writes: > On 08/08/2013 04:14 PM, Kevin Hilman wrote: >> Dave Gerlach <d-gerlach@ti.com> writes: >> >>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>>> $subject and patch don't match. >>>> >>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>>> In reference to >>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>>> bound and which don't. >>>>> Does it need to? M3 firmware can pretty much define "I will force >>>>> the device into low power state, and if the drivers dont handle >>>>> things properly, fix the darned driver". M3 behavior should be >>>>> considered as a "hardware" as far as Linux running on MPU is >>>>> concerned, and firmware helps change the behavior by accounting for >>>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>>> there is no need to carry this in Linux. >>>>> >>>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>>> comment in the last version. Linux need not know about all such firmware >>>> quirks. Also all these M3 specific stuff, should be done somewhere >>>> else. Probably having a small M3 driver won't be a bad idea. >>> >>> I am not opposed to doing it this way and letting the M3 firmware >>> handle idling these modules, however the one concern raised in the >>> last series is that an approach that does not acknowledge drivers will >>> hide driver PM bugs. I suppose as long as I make sure to document that >>> the devices are being idled by the M3 firmware this may not be an >>> issue. I will look into implementing this. >> >> No, please don't start idling devices in firmware that are otherwise >> managed by Linux. Keep the firmware simple and dumb. Linux is managing >> these devices, it should manage their bugs too. > >> >> This is not just about idling devices. This is about handling broken IP >> blocks whose power-on reset state does not allow the the powerdomain to >> reach its target state. That's just bad hardware design. > > Right, this is where M3 can help -> provide a consistent state for > linux kernel to work with. by the fact that we want to keep majority > of the power code inside master CPU, we are just letting M3 help us > with nothing major at all.. heh, I would say HW design bugs like this are more than "nothing major at all." :) > tiny stuff like these can help "fix" the hardware design quirks by > hiding it behind the firmware and modifying the hardware behavior. I disagree here. I'm a firmware minimalist, and hiding bugs like this in the firmware is wrong when Linux is otherwise managing these devices. It also imposes criteria on the firmware of future SoCs that doesn't belong there either. IMO, the only stuff the firmware should do is what Linux *cannot* do. Remember, this only needs to happen when there isn't a driver for these devices. Should we communicate to the firmware that the OS has no driver, so please enable the hack? I think not. > I know it breaks the purity of role, but as the > next evolution, we might want to consider M3 something like an > "accelerator" for power management activity.. (not saying it is that > fast.. but conceptually). Yes, it breaks the purity of role, and makes it hard to maintain and extend to future SoCs. As a maintainer, that's a red flag. IMO, the roles need to be kept clear. The M3 manages some devices and the interconnect that MPU/Linux cannot, the rest are managed by Linux. >> That being said, IMO, the kernel (specifically omap_device) should >> handle this, and it should be rather easy to do in the omap_device layer >> and keep the SoC suspend/resume core code simple and ignorant of these >> "quirks." >> >> AFAICT, there's no reason these quirks need to be dealt with immediatly >> on suspend. A slight delay should be fine, as long as it's before the >> next suspend/idle attempt, right? >> >> Given that, what we need to do (and by we, I mean you) is to flag all >> broken IP blocks, and let omap_device handle them in a suspend/resume >> notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.) > > yes - that is the alternate that comes to mind. In the earlier reviews of this series (many months ago now), I complained about the presence of this device specific handling in the core MPU PM code. I'm somewhat troubled by the fact that nobody explored alternatives that so easily come to mind. Kevin
On 08/08/2013 06:04 PM, Kevin Hilman wrote: > Nishanth Menon <nm@ti.com> writes: > >> On 08/08/2013 04:14 PM, Kevin Hilman wrote: >>> Dave Gerlach <d-gerlach@ti.com> writes: >>> >>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>>>> $subject and patch don't match. >>>>> >>>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>>>> In reference to >>>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>>>> bound and which don't. >>>>>> Does it need to? M3 firmware can pretty much define "I will force >>>>>> the device into low power state, and if the drivers dont handle >>>>>> things properly, fix the darned driver". M3 behavior should be >>>>>> considered as a "hardware" as far as Linux running on MPU is >>>>>> concerned, and firmware helps change the behavior by accounting for >>>>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>>>> there is no need to carry this in Linux. >>>>>> >>>>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>>>> comment in the last version. Linux need not know about all such firmware >>>>> quirks. Also all these M3 specific stuff, should be done somewhere >>>>> else. Probably having a small M3 driver won't be a bad idea. >>>> >>>> I am not opposed to doing it this way and letting the M3 firmware >>>> handle idling these modules, however the one concern raised in the >>>> last series is that an approach that does not acknowledge drivers will >>>> hide driver PM bugs. I suppose as long as I make sure to document that >>>> the devices are being idled by the M3 firmware this may not be an >>>> issue. I will look into implementing this. >>> >>> No, please don't start idling devices in firmware that are otherwise >>> managed by Linux. Keep the firmware simple and dumb. Linux is managing >>> these devices, it should manage their bugs too. >> >>> >>> This is not just about idling devices. This is about handling broken IP >>> blocks whose power-on reset state does not allow the the powerdomain to >>> reach its target state. That's just bad hardware design. >> >> Right, this is where M3 can help -> provide a consistent state for >> linux kernel to work with. by the fact that we want to keep majority >> of the power code inside master CPU, we are just letting M3 help us >> with nothing major at all.. > > heh, I would say HW design bugs like this are more than "nothing major > at all." :) > >> tiny stuff like these can help "fix" the hardware design quirks by >> hiding it behind the firmware and modifying the hardware behavior. > > I disagree here. I'm a firmware minimalist, and hiding bugs like this > in the firmware is wrong when Linux is otherwise managing these devices. > It also imposes criteria on the firmware of future SoCs that doesn't > belong there either. IMO, the only stuff the firmware should do is what > Linux *cannot* do. > > Remember, this only needs to happen when there isn't a driver for these > devices. Should we communicate to the firmware that the OS has no > driver, so please enable the hack? I think not. My view is that the M3 should *ignore* the presence/existence of MPU's drivers. M3 will do whatever to force the system to go to suspend once notified - this saves us the prehistoric perpetual trouble when drivers have bugs (which get exposed in weird usage scenarios) in production systems, we dont get any hardware help to fix them up while attempting low power states and system never really hits low power state. This was always because OMAP and it's derivatives have been "democratic" in power management - if every hardware block achieves proper state, then we achieve a system-wide low power state. > >> I know it breaks the purity of role, but as the >> next evolution, we might want to consider M3 something like an >> "accelerator" for power management activity.. (not saying it is that >> fast.. but conceptually). > > Yes, it breaks the purity of role, and makes it hard to maintain and > extend to future SoCs. As a maintainer, that's a red flag. IMO, the > roles need to be kept clear. The M3 manages some devices and the > interconnect that MPU/Linux cannot, the rest are managed by Linux. suspend is a very controlled state as against cpuidle where driver knowledge is necessary and in fact mandatory. drivers are supposed to release their resources - and even though we test the hell out of them, we do have paths untrodden when it comes to production systems. I think the insight we have about the hardware make us(linux folks) want to own the decision making process on the master MPU - I mean, *nobody*(including me) wants to trust a "firmware" - that word is almost synonymous with "unspeakable horror". If on the other hand, we had a non-programmable hardware which would force all systems to achieve off mode (imagine having a PRCM which was really capable of doing it), we would have probably not had to deal with those pesky "stuck-in-transition" and other variants of issues (where MPU went to low power state, but core refused to go down - resulting in 200mA+ power instead of the <1mA we expected to see). I consider M3 to power management similar to what Neon is to ARM. I mean, I would even love a PMIC which is completely reprogrammable (where I could define the registers in s/w)! My personal thought is that (if possible): a) we should try to make the source firmware visible to everyone who has a stake on it. b) If (a) is possible, then we should see how we can consider M3 as an extension to Linux power strategy, rather than a "necessary burden" to carry around. In this particular case. (a) is done see [1]. So, why not (b)? A synergy does not necessarily mean "purity of role" is broken. it is just another way of doing the job. While, I personally dont think [1] is public enough, we can try to work through those current constraints to ensure everything is synergistic. in other words, this is not a "Graphics" or "Multimedia" or even few "BIOS" kind of "hidden firmware you cannot do anything about" scenario - here, *we* have the choice. [1] http://arago-project.org/git/projects/?p=am33x-cm3.git;a=summary > >>> That being said, IMO, the kernel (specifically omap_device) should >>> handle this, and it should be rather easy to do in the omap_device layer >>> and keep the SoC suspend/resume core code simple and ignorant of these >>> "quirks." >>> >>> AFAICT, there's no reason these quirks need to be dealt with immediatly >>> on suspend. A slight delay should be fine, as long as it's before the >>> next suspend/idle attempt, right? >>> >>> Given that, what we need to do (and by we, I mean you) is to flag all >>> broken IP blocks, and let omap_device handle them in a suspend/resume >>> notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.) >> >> yes - that is the alternate that comes to mind. > > In the earlier reviews of this series (many months ago now), I > complained about the presence of this device specific handling in the > core MPU PM code. I'm somewhat troubled by the fact that nobody explored > alternatives that so easily come to mind. Just spoke to Dave in person a few mins back, and he is going to go through all the previous mail chains and attempt to be thorough again - seems like going through a written list of pending actions completely missed many key aspects of prior reviews :). Apologies on this.
Nishanth Menon <nm@ti.com> writes: > On 08/08/2013 06:04 PM, Kevin Hilman wrote: >> Nishanth Menon <nm@ti.com> writes: >> >>> On 08/08/2013 04:14 PM, Kevin Hilman wrote: >>>> Dave Gerlach <d-gerlach@ti.com> writes: >>>> >>>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>>>>> $subject and patch don't match. >>>>>> >>>>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>>>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>>>>> In reference to >>>>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>>>>> bound and which don't. >>>>>>> Does it need to? M3 firmware can pretty much define "I will force >>>>>>> the device into low power state, and if the drivers dont handle >>>>>>> things properly, fix the darned driver". M3 behavior should be >>>>>>> considered as a "hardware" as far as Linux running on MPU is >>>>>>> concerned, and firmware helps change the behavior by accounting for >>>>>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>>>>> there is no need to carry this in Linux. >>>>>>> >>>>>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>>>>> comment in the last version. Linux need not know about all such firmware >>>>>> quirks. Also all these M3 specific stuff, should be done somewhere >>>>>> else. Probably having a small M3 driver won't be a bad idea. >>>>> >>>>> I am not opposed to doing it this way and letting the M3 firmware >>>>> handle idling these modules, however the one concern raised in the >>>>> last series is that an approach that does not acknowledge drivers will >>>>> hide driver PM bugs. I suppose as long as I make sure to document that >>>>> the devices are being idled by the M3 firmware this may not be an >>>>> issue. I will look into implementing this. >>>> >>>> No, please don't start idling devices in firmware that are otherwise >>>> managed by Linux. Keep the firmware simple and dumb. Linux is managing >>>> these devices, it should manage their bugs too. >>> >>>> >>>> This is not just about idling devices. This is about handling broken IP >>>> blocks whose power-on reset state does not allow the the powerdomain to >>>> reach its target state. That's just bad hardware design. >>> >>> Right, this is where M3 can help -> provide a consistent state for >>> linux kernel to work with. by the fact that we want to keep majority >>> of the power code inside master CPU, we are just letting M3 help us >>> with nothing major at all.. >> >> heh, I would say HW design bugs like this are more than "nothing major >> at all." :) >> >>> tiny stuff like these can help "fix" the hardware design quirks by >>> hiding it behind the firmware and modifying the hardware behavior. >> >> I disagree here. I'm a firmware minimalist, and hiding bugs like this >> in the firmware is wrong when Linux is otherwise managing these devices. >> It also imposes criteria on the firmware of future SoCs that doesn't >> belong there either. IMO, the only stuff the firmware should do is what >> Linux *cannot* do. >> >> Remember, this only needs to happen when there isn't a driver for these >> devices. Should we communicate to the firmware that the OS has no >> driver, so please enable the hack? I think not. > > My view is that the M3 should *ignore* the presence/existence of MPU's > drivers. M3 will do whatever to force the system to go to suspend once > notified - this saves us the prehistoric perpetual trouble when > drivers have bugs (which get exposed in weird usage scenarios) in > production systems, we dont get any hardware help to fix them up while > attempting low power states and system never really hits low power > state. This was always because OMAP and it's derivatives have been > "democratic" in power management - if every hardware block achieves > proper state, then we achieve a system-wide low power state. > >> >>> I know it breaks the purity of role, but as the >>> next evolution, we might want to consider M3 something like an >>> "accelerator" for power management activity.. (not saying it is that >>> fast.. but conceptually). >> >> Yes, it breaks the purity of role, and makes it hard to maintain and >> extend to future SoCs. As a maintainer, that's a red flag. IMO, the >> roles need to be kept clear. The M3 manages some devices and the >> interconnect that MPU/Linux cannot, the rest are managed by Linux. > > suspend is a very controlled state as against cpuidle where driver > knowledge is necessary and in fact mandatory. drivers are supposed to > release their resources - and even though we test the hell out of > them, we do have paths untrodden when it comes to production systems. Since folks don't seem to care about idle for AM33xx (starting with the hw designers, from what I can tell), you have the luxury of thinking only about suspend, where firmware can be heavy handed and force things into submission. Unfortunately, with cpuidle, life is not that easy and you have to have cooperation of the device drivers. Coordinating that with firmware is not so simple, to put it mildly. Any SW/firmware design that does not account for *both* static PM (suspend/resume) and dynamic PM (runtime PM + CPUidle) is not long-term maintainable, and thus ready for mainline IMO. (BTW, this is another theme from previous reviews of this series.) > I think the insight we have about the hardware make us(linux folks) > want to own the decision making process on the master MPU - I mean, > *nobody*(including me) wants to trust a "firmware" - that word is > almost synonymous with "unspeakable horror". > > If on the other hand, we had a non-programmable hardware which would > force all systems to achieve off mode (imagine having a PRCM which was > really capable of doing it), we would have probably not had to deal > with those pesky "stuck-in-transition" and other variants of issues > (where MPU went to low power state, but core refused to go down - > resulting in 200mA+ power instead of the <1mA we expected to see). > > I consider M3 to power management similar to what Neon is to ARM. I > mean, I would even love a PMIC which is completely reprogrammable > (where I could define the registers in s/w)! > > My personal thought is that (if possible): > a) we should try to make the source firmware visible to everyone who > has a stake on it. > b) If (a) is possible, then we should see how we can consider M3 as an > extension to Linux power strategy, rather than a "necessary burden" to > carry around. > > In this particular case. (a) is done see [1]. So, why not (b)? A > synergy does not necessarily mean "purity of role" is broken. it is > just another way of doing the job. > > While, I personally dont think [1] is public enough, we can try to > work through those current constraints to ensure everything is > synergistic. > > in other words, this is not a "Graphics" or "Multimedia" or even few > "BIOS" kind of "hidden firmware you cannot do anything about" scenario > - > here, *we* have the choice. > > [1] http://arago-project.org/git/projects/?p=am33x-cm3.git;a=summary >> >>>> That being said, IMO, the kernel (specifically omap_device) should >>>> handle this, and it should be rather easy to do in the omap_device layer >>>> and keep the SoC suspend/resume core code simple and ignorant of these >>>> "quirks." >>>> >>>> AFAICT, there's no reason these quirks need to be dealt with immediatly >>>> on suspend. A slight delay should be fine, as long as it's before the >>>> next suspend/idle attempt, right? >>>> >>>> Given that, what we need to do (and by we, I mean you) is to flag all >>>> broken IP blocks, and let omap_device handle them in a suspend/resume >>>> notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.) >>> >>> yes - that is the alternate that comes to mind. >> >> In the earlier reviews of this series (many months ago now), I >> complained about the presence of this device specific handling in the >> core MPU PM code. I'm somewhat troubled by the fact that nobody explored >> alternatives that so easily come to mind. > > Just spoke to Dave in person a few mins back, and he is going to go > through all the previous mail chains and attempt to be thorough again > - > seems like going through a written list of pending actions completely > missed many key aspects of prior reviews :). Apologies on this. Thanks. Since he's inherited this series Dave gets a get out of jail free card... this time. Next time, I expect I might be grumpier. Kevin
On 08/09/2013 11:12 AM, Kevin Hilman wrote: > Nishanth Menon <nm@ti.com> writes: > >> On 08/08/2013 06:04 PM, Kevin Hilman wrote: >>> Nishanth Menon <nm@ti.com> writes: >>> >>>> On 08/08/2013 04:14 PM, Kevin Hilman wrote: >>>>> Dave Gerlach <d-gerlach@ti.com> writes: >>>>> >>>>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>>>>>> $subject and patch don't match. >>>>>>> >>>>>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>>>>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>>>>>> In reference to >>>>>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>>>>>> bound and which don't. >>>>>>>> Does it need to? M3 firmware can pretty much define "I will force >>>>>>>> the device into low power state, and if the drivers dont handle >>>>>>>> things properly, fix the darned driver". M3 behavior should be >>>>>>>> considered as a "hardware" as far as Linux running on MPU is >>>>>>>> concerned, and firmware helps change the behavior by accounting for >>>>>>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>>>>>> there is no need to carry this in Linux. >>>>>>>> >>>>>>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>>>>>> comment in the last version. Linux need not know about all such firmware >>>>>>> quirks. Also all these M3 specific stuff, should be done somewhere >>>>>>> else. Probably having a small M3 driver won't be a bad idea. >>>>>> >>>>>> I am not opposed to doing it this way and letting the M3 firmware >>>>>> handle idling these modules, however the one concern raised in the >>>>>> last series is that an approach that does not acknowledge drivers will >>>>>> hide driver PM bugs. I suppose as long as I make sure to document that >>>>>> the devices are being idled by the M3 firmware this may not be an >>>>>> issue. I will look into implementing this. >>>>> >>>>> No, please don't start idling devices in firmware that are otherwise >>>>> managed by Linux. Keep the firmware simple and dumb. Linux is managing >>>>> these devices, it should manage their bugs too. >>>> >>>>> >>>>> This is not just about idling devices. This is about handling broken IP >>>>> blocks whose power-on reset state does not allow the the powerdomain to >>>>> reach its target state. That's just bad hardware design. >>>> >>>> Right, this is where M3 can help -> provide a consistent state for >>>> linux kernel to work with. by the fact that we want to keep majority >>>> of the power code inside master CPU, we are just letting M3 help us >>>> with nothing major at all.. >>> >>> heh, I would say HW design bugs like this are more than "nothing major >>> at all." :) >>> >>>> tiny stuff like these can help "fix" the hardware design quirks by >>>> hiding it behind the firmware and modifying the hardware behavior. >>> >>> I disagree here. I'm a firmware minimalist, and hiding bugs like this >>> in the firmware is wrong when Linux is otherwise managing these devices. >>> It also imposes criteria on the firmware of future SoCs that doesn't >>> belong there either. IMO, the only stuff the firmware should do is what >>> Linux *cannot* do. >>> >>> Remember, this only needs to happen when there isn't a driver for these >>> devices. Should we communicate to the firmware that the OS has no >>> driver, so please enable the hack? I think not. >> >> My view is that the M3 should *ignore* the presence/existence of MPU's >> drivers. M3 will do whatever to force the system to go to suspend once >> notified - this saves us the prehistoric perpetual trouble when >> drivers have bugs (which get exposed in weird usage scenarios) in >> production systems, we dont get any hardware help to fix them up while >> attempting low power states and system never really hits low power >> state. This was always because OMAP and it's derivatives have been >> "democratic" in power management - if every hardware block achieves >> proper state, then we achieve a system-wide low power state. >> >>> >>>> I know it breaks the purity of role, but as the >>>> next evolution, we might want to consider M3 something like an >>>> "accelerator" for power management activity.. (not saying it is that >>>> fast.. but conceptually). >>> >>> Yes, it breaks the purity of role, and makes it hard to maintain and >>> extend to future SoCs. As a maintainer, that's a red flag. IMO, the >>> roles need to be kept clear. The M3 manages some devices and the >>> interconnect that MPU/Linux cannot, the rest are managed by Linux. >> >> suspend is a very controlled state as against cpuidle where driver >> knowledge is necessary and in fact mandatory. drivers are supposed to >> release their resources - and even though we test the hell out of >> them, we do have paths untrodden when it comes to production systems. > > Since folks don't seem to care about idle for AM33xx (starting with the > hw designers, from what I can tell), you have the luxury of thinking > only about suspend, where firmware can be heavy handed and force things > into submission. Unfortunately, with cpuidle, life is not that easy and > you have to have cooperation of the device drivers. Coordinating that > with firmware is not so simple, to put it mildly. > > Any SW/firmware design that does not account for *both* static PM > (suspend/resume) and dynamic PM (runtime PM + CPUidle) is not long-term > maintainable, and thus ready for mainline IMO. (BTW, this is another > theme from previous reviews of this series.) I completely agree with you. But is'nt the specific suspend state we are attempting to achieve on AM335x just tooo expensive latency wise for being even considered for cpuidle? I am sure you recollect the latencies involved in OMAP3 OFF mode Vs OMAP4+ OFF mode - which basically kicked out OFF mode from OMAP4 cpuidle C states? - it was practically useless in this *specific* power state we are attempting, we do a bunch of i2c operations, etc, in short something that cannot even be considered for cpuidle. Considering this, we can consider the same only for suspend path - hence allowing firmware to do more here. This does not conflict with cpuidle (which controls MPU) or runtime PM (which kicks in once you have drivers active, but if drivers get active, we dont need to deal with this crap). Dont you think this helps the specific case to move this into firmware rather than into omap_device? [...]
Nishanth Menon <nm@ti.com> writes: > On 08/09/2013 11:12 AM, Kevin Hilman wrote: >> Nishanth Menon <nm@ti.com> writes: >> >>> On 08/08/2013 06:04 PM, Kevin Hilman wrote: >>>> Nishanth Menon <nm@ti.com> writes: >>>> >>>>> On 08/08/2013 04:14 PM, Kevin Hilman wrote: >>>>>> Dave Gerlach <d-gerlach@ti.com> writes: >>>>>> >>>>>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>>>>>>> $subject and patch don't match. >>>>>>>> >>>>>>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>>>>>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>>>>>>> In reference to >>>>>>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>>>>>>> bound and which don't. >>>>>>>>> Does it need to? M3 firmware can pretty much define "I will force >>>>>>>>> the device into low power state, and if the drivers dont handle >>>>>>>>> things properly, fix the darned driver". M3 behavior should be >>>>>>>>> considered as a "hardware" as far as Linux running on MPU is >>>>>>>>> concerned, and firmware helps change the behavior by accounting for >>>>>>>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>>>>>>> there is no need to carry this in Linux. >>>>>>>>> >>>>>>>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>>>>>>> comment in the last version. Linux need not know about all such firmware >>>>>>>> quirks. Also all these M3 specific stuff, should be done somewhere >>>>>>>> else. Probably having a small M3 driver won't be a bad idea. >>>>>>> >>>>>>> I am not opposed to doing it this way and letting the M3 firmware >>>>>>> handle idling these modules, however the one concern raised in the >>>>>>> last series is that an approach that does not acknowledge drivers will >>>>>>> hide driver PM bugs. I suppose as long as I make sure to document that >>>>>>> the devices are being idled by the M3 firmware this may not be an >>>>>>> issue. I will look into implementing this. >>>>>> >>>>>> No, please don't start idling devices in firmware that are otherwise >>>>>> managed by Linux. Keep the firmware simple and dumb. Linux is managing >>>>>> these devices, it should manage their bugs too. >>>>> >>>>>> >>>>>> This is not just about idling devices. This is about handling broken IP >>>>>> blocks whose power-on reset state does not allow the the powerdomain to >>>>>> reach its target state. That's just bad hardware design. >>>>> >>>>> Right, this is where M3 can help -> provide a consistent state for >>>>> linux kernel to work with. by the fact that we want to keep majority >>>>> of the power code inside master CPU, we are just letting M3 help us >>>>> with nothing major at all.. >>>> >>>> heh, I would say HW design bugs like this are more than "nothing major >>>> at all." :) >>>> >>>>> tiny stuff like these can help "fix" the hardware design quirks by >>>>> hiding it behind the firmware and modifying the hardware behavior. >>>> >>>> I disagree here. I'm a firmware minimalist, and hiding bugs like this >>>> in the firmware is wrong when Linux is otherwise managing these devices. >>>> It also imposes criteria on the firmware of future SoCs that doesn't >>>> belong there either. IMO, the only stuff the firmware should do is what >>>> Linux *cannot* do. >>>> >>>> Remember, this only needs to happen when there isn't a driver for these >>>> devices. Should we communicate to the firmware that the OS has no >>>> driver, so please enable the hack? I think not. >>> >>> My view is that the M3 should *ignore* the presence/existence of MPU's >>> drivers. M3 will do whatever to force the system to go to suspend once >>> notified - this saves us the prehistoric perpetual trouble when >>> drivers have bugs (which get exposed in weird usage scenarios) in >>> production systems, we dont get any hardware help to fix them up while >>> attempting low power states and system never really hits low power >>> state. This was always because OMAP and it's derivatives have been >>> "democratic" in power management - if every hardware block achieves >>> proper state, then we achieve a system-wide low power state. >>> >>>> >>>>> I know it breaks the purity of role, but as the >>>>> next evolution, we might want to consider M3 something like an >>>>> "accelerator" for power management activity.. (not saying it is that >>>>> fast.. but conceptually). >>>> >>>> Yes, it breaks the purity of role, and makes it hard to maintain and >>>> extend to future SoCs. As a maintainer, that's a red flag. IMO, the >>>> roles need to be kept clear. The M3 manages some devices and the >>>> interconnect that MPU/Linux cannot, the rest are managed by Linux. >>> >>> suspend is a very controlled state as against cpuidle where driver >>> knowledge is necessary and in fact mandatory. drivers are supposed to >>> release their resources - and even though we test the hell out of >>> them, we do have paths untrodden when it comes to production systems. >> >> Since folks don't seem to care about idle for AM33xx (starting with the >> hw designers, from what I can tell), you have the luxury of thinking >> only about suspend, where firmware can be heavy handed and force things >> into submission. Unfortunately, with cpuidle, life is not that easy and >> you have to have cooperation of the device drivers. Coordinating that >> with firmware is not so simple, to put it mildly. >> >> Any SW/firmware design that does not account for *both* static PM >> (suspend/resume) and dynamic PM (runtime PM + CPUidle) is not long-term >> maintainable, and thus ready for mainline IMO. (BTW, this is another >> theme from previous reviews of this series.) > > I completely agree with you. But is'nt the specific suspend state we > are attempting to achieve on AM335x just tooo expensive latency wise > for being even considered for cpuidle? > > I am sure you recollect the latencies involved in OMAP3 OFF mode Vs > OMAP4+ OFF mode - which basically kicked out OFF mode from OMAP4 > cpuidle C states? - it was practically useless > > in this *specific* power state we are attempting, we do a bunch of i2c > operations, etc, in short something that cannot even be considered for > cpuidle. > > Considering this, we can consider the same only for suspend path - > hence allowing firmware to do more here. > > > This does not conflict with cpuidle (which controls MPU) or runtime PM > (which kicks in once you have drivers active, but if drivers get > active, we dont need to deal with this crap). > > Dont you think this helps the specific case to move this into firmware > rather than into omap_device? No, I don't. That means the firmware design is based on several assumptions about what Linux can and can't do in idle, and then imposing that on future Linux designs as well. I dont' buy it. Kevin
On 08/09/2013 03:34 PM, Kevin Hilman wrote: > Nishanth Menon <nm@ti.com> writes: > >> On 08/09/2013 11:12 AM, Kevin Hilman wrote: >>> Nishanth Menon <nm@ti.com> writes: >>> >>>> On 08/08/2013 06:04 PM, Kevin Hilman wrote: >>>>> Nishanth Menon <nm@ti.com> writes: >>>>> >>>>>> On 08/08/2013 04:14 PM, Kevin Hilman wrote: >>>>>>> Dave Gerlach <d-gerlach@ti.com> writes: >>>>>>> >>>>>>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>>>>>>>> $subject and patch don't match. >>>>>>>>> >>>>>>>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>>>>>>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>>>>>>>> In reference to >>>>>>>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>>>>>>>> bound and which don't. >>>>>>>>>> Does it need to? M3 firmware can pretty much define "I will force >>>>>>>>>> the device into low power state, and if the drivers dont handle >>>>>>>>>> things properly, fix the darned driver". M3 behavior should be >>>>>>>>>> considered as a "hardware" as far as Linux running on MPU is >>>>>>>>>> concerned, and firmware helps change the behavior by accounting for >>>>>>>>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>>>>>>>> there is no need to carry this in Linux. >>>>>>>>>> >>>>>>>>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>>>>>>>> comment in the last version. Linux need not know about all such firmware >>>>>>>>> quirks. Also all these M3 specific stuff, should be done somewhere >>>>>>>>> else. Probably having a small M3 driver won't be a bad idea. >>>>>>>> >>>>>>>> I am not opposed to doing it this way and letting the M3 firmware >>>>>>>> handle idling these modules, however the one concern raised in the >>>>>>>> last series is that an approach that does not acknowledge drivers will >>>>>>>> hide driver PM bugs. I suppose as long as I make sure to document that >>>>>>>> the devices are being idled by the M3 firmware this may not be an >>>>>>>> issue. I will look into implementing this. >>>>>>> >>>>>>> No, please don't start idling devices in firmware that are otherwise >>>>>>> managed by Linux. Keep the firmware simple and dumb. Linux is managing >>>>>>> these devices, it should manage their bugs too. >>>>>> >>>>>>> >>>>>>> This is not just about idling devices. This is about handling broken IP >>>>>>> blocks whose power-on reset state does not allow the the powerdomain to >>>>>>> reach its target state. That's just bad hardware design. >>>>>> >>>>>> Right, this is where M3 can help -> provide a consistent state for >>>>>> linux kernel to work with. by the fact that we want to keep majority >>>>>> of the power code inside master CPU, we are just letting M3 help us >>>>>> with nothing major at all.. >>>>> >>>>> heh, I would say HW design bugs like this are more than "nothing major >>>>> at all." :) >>>>> >>>>>> tiny stuff like these can help "fix" the hardware design quirks by >>>>>> hiding it behind the firmware and modifying the hardware behavior. >>>>> >>>>> I disagree here. I'm a firmware minimalist, and hiding bugs like this >>>>> in the firmware is wrong when Linux is otherwise managing these devices. >>>>> It also imposes criteria on the firmware of future SoCs that doesn't >>>>> belong there either. IMO, the only stuff the firmware should do is what >>>>> Linux *cannot* do. >>>>> >>>>> Remember, this only needs to happen when there isn't a driver for these >>>>> devices. Should we communicate to the firmware that the OS has no >>>>> driver, so please enable the hack? I think not. >>>> >>>> My view is that the M3 should *ignore* the presence/existence of MPU's >>>> drivers. M3 will do whatever to force the system to go to suspend once >>>> notified - this saves us the prehistoric perpetual trouble when >>>> drivers have bugs (which get exposed in weird usage scenarios) in >>>> production systems, we dont get any hardware help to fix them up while >>>> attempting low power states and system never really hits low power >>>> state. This was always because OMAP and it's derivatives have been >>>> "democratic" in power management - if every hardware block achieves >>>> proper state, then we achieve a system-wide low power state. >>>> >>>>> >>>>>> I know it breaks the purity of role, but as the >>>>>> next evolution, we might want to consider M3 something like an >>>>>> "accelerator" for power management activity.. (not saying it is that >>>>>> fast.. but conceptually). >>>>> >>>>> Yes, it breaks the purity of role, and makes it hard to maintain and >>>>> extend to future SoCs. As a maintainer, that's a red flag. IMO, the >>>>> roles need to be kept clear. The M3 manages some devices and the >>>>> interconnect that MPU/Linux cannot, the rest are managed by Linux. >>>> >>>> suspend is a very controlled state as against cpuidle where driver >>>> knowledge is necessary and in fact mandatory. drivers are supposed to >>>> release their resources - and even though we test the hell out of >>>> them, we do have paths untrodden when it comes to production systems. >>> >>> Since folks don't seem to care about idle for AM33xx (starting with the >>> hw designers, from what I can tell), you have the luxury of thinking >>> only about suspend, where firmware can be heavy handed and force things >>> into submission. Unfortunately, with cpuidle, life is not that easy and >>> you have to have cooperation of the device drivers. Coordinating that >>> with firmware is not so simple, to put it mildly. >>> >>> Any SW/firmware design that does not account for *both* static PM >>> (suspend/resume) and dynamic PM (runtime PM + CPUidle) is not long-term >>> maintainable, and thus ready for mainline IMO. (BTW, this is another >>> theme from previous reviews of this series.) >> >> I completely agree with you. But is'nt the specific suspend state we >> are attempting to achieve on AM335x just tooo expensive latency wise >> for being even considered for cpuidle? >> >> I am sure you recollect the latencies involved in OMAP3 OFF mode Vs >> OMAP4+ OFF mode - which basically kicked out OFF mode from OMAP4 >> cpuidle C states? - it was practically useless >> >> in this *specific* power state we are attempting, we do a bunch of i2c >> operations, etc, in short something that cannot even be considered for >> cpuidle. >> >> Considering this, we can consider the same only for suspend path - >> hence allowing firmware to do more here. >> >> >> This does not conflict with cpuidle (which controls MPU) or runtime PM >> (which kicks in once you have drivers active, but if drivers get >> active, we dont need to deal with this crap). >> >> Dont you think this helps the specific case to move this into firmware >> rather than into omap_device? > > No, I don't. > > That means the firmware design is based on several assumptions about > what Linux can and can't do in idle, and then imposing that on future > Linux designs as well. I dont' buy it. OK, I back out my proposal. Will let Dave try out a generic solution in kernel and comment back.
On Fri, Aug 9, 2013 at 1:34 PM, Kevin Hilman <khilman@linaro.org> wrote: > Nishanth Menon <nm@ti.com> writes: > >> On 08/09/2013 11:12 AM, Kevin Hilman wrote: >>> Nishanth Menon <nm@ti.com> writes: >>> >>>> On 08/08/2013 06:04 PM, Kevin Hilman wrote: >>>>> Nishanth Menon <nm@ti.com> writes: >>>>> >>>>>> On 08/08/2013 04:14 PM, Kevin Hilman wrote: >>>>>>> Dave Gerlach <d-gerlach@ti.com> writes: >>>>>>> >>>>>>>> On 08/08/2013 10:03 AM, Santosh Shilimkar wrote: >>>>>>>>> $subject and patch don't match. >>>>>>>>> >>>>>>>>> On Thursday 08 August 2013 08:26 AM, Nishanth Menon wrote: >>>>>>>>>> On 08/08/2013 03:45 AM, Russ Dill wrote: >>>>>>>>>>> In reference to >>>>>>>>>>> the M3 handling it, the M3 wouldn't know which devices have a driver >>>>>>>>>>> bound and which don't. >>>>>>>>>> Does it need to? M3 firmware can pretty much define "I will force >>>>>>>>>> the device into low power state, and if the drivers dont handle >>>>>>>>>> things properly, fix the darned driver". M3 behavior should be >>>>>>>>>> considered as a "hardware" as far as Linux running on MPU is >>>>>>>>>> concerned, and firmware helps change the behavior by accounting for >>>>>>>>>> SoC quirks. *if* we have ability to handle this in the firmware, >>>>>>>>>> there is no need to carry this in Linux. >>>>>>>>>> >>>>>>>>> I agree with Nishant. I don't like this patch and IIRC, I gave same >>>>>>>>> comment in the last version. Linux need not know about all such firmware >>>>>>>>> quirks. Also all these M3 specific stuff, should be done somewhere >>>>>>>>> else. Probably having a small M3 driver won't be a bad idea. >>>>>>>> >>>>>>>> I am not opposed to doing it this way and letting the M3 firmware >>>>>>>> handle idling these modules, however the one concern raised in the >>>>>>>> last series is that an approach that does not acknowledge drivers will >>>>>>>> hide driver PM bugs. I suppose as long as I make sure to document that >>>>>>>> the devices are being idled by the M3 firmware this may not be an >>>>>>>> issue. I will look into implementing this. >>>>>>> >>>>>>> No, please don't start idling devices in firmware that are otherwise >>>>>>> managed by Linux. Keep the firmware simple and dumb. Linux is managing >>>>>>> these devices, it should manage their bugs too. >>>>>> >>>>>>> >>>>>>> This is not just about idling devices. This is about handling broken IP >>>>>>> blocks whose power-on reset state does not allow the the powerdomain to >>>>>>> reach its target state. That's just bad hardware design. >>>>>> >>>>>> Right, this is where M3 can help -> provide a consistent state for >>>>>> linux kernel to work with. by the fact that we want to keep majority >>>>>> of the power code inside master CPU, we are just letting M3 help us >>>>>> with nothing major at all.. >>>>> >>>>> heh, I would say HW design bugs like this are more than "nothing major >>>>> at all." :) >>>>> >>>>>> tiny stuff like these can help "fix" the hardware design quirks by >>>>>> hiding it behind the firmware and modifying the hardware behavior. >>>>> >>>>> I disagree here. I'm a firmware minimalist, and hiding bugs like this >>>>> in the firmware is wrong when Linux is otherwise managing these devices. >>>>> It also imposes criteria on the firmware of future SoCs that doesn't >>>>> belong there either. IMO, the only stuff the firmware should do is what >>>>> Linux *cannot* do. >>>>> >>>>> Remember, this only needs to happen when there isn't a driver for these >>>>> devices. Should we communicate to the firmware that the OS has no >>>>> driver, so please enable the hack? I think not. >>>> >>>> My view is that the M3 should *ignore* the presence/existence of MPU's >>>> drivers. M3 will do whatever to force the system to go to suspend once >>>> notified - this saves us the prehistoric perpetual trouble when >>>> drivers have bugs (which get exposed in weird usage scenarios) in >>>> production systems, we dont get any hardware help to fix them up while >>>> attempting low power states and system never really hits low power >>>> state. This was always because OMAP and it's derivatives have been >>>> "democratic" in power management - if every hardware block achieves >>>> proper state, then we achieve a system-wide low power state. >>>> >>>>> >>>>>> I know it breaks the purity of role, but as the >>>>>> next evolution, we might want to consider M3 something like an >>>>>> "accelerator" for power management activity.. (not saying it is that >>>>>> fast.. but conceptually). >>>>> >>>>> Yes, it breaks the purity of role, and makes it hard to maintain and >>>>> extend to future SoCs. As a maintainer, that's a red flag. IMO, the >>>>> roles need to be kept clear. The M3 manages some devices and the >>>>> interconnect that MPU/Linux cannot, the rest are managed by Linux. >>>> >>>> suspend is a very controlled state as against cpuidle where driver >>>> knowledge is necessary and in fact mandatory. drivers are supposed to >>>> release their resources - and even though we test the hell out of >>>> them, we do have paths untrodden when it comes to production systems. >>> >>> Since folks don't seem to care about idle for AM33xx (starting with the >>> hw designers, from what I can tell), you have the luxury of thinking >>> only about suspend, where firmware can be heavy handed and force things >>> into submission. Unfortunately, with cpuidle, life is not that easy and >>> you have to have cooperation of the device drivers. Coordinating that >>> with firmware is not so simple, to put it mildly. >>> >>> Any SW/firmware design that does not account for *both* static PM >>> (suspend/resume) and dynamic PM (runtime PM + CPUidle) is not long-term >>> maintainable, and thus ready for mainline IMO. (BTW, this is another >>> theme from previous reviews of this series.) >> >> I completely agree with you. But is'nt the specific suspend state we >> are attempting to achieve on AM335x just tooo expensive latency wise >> for being even considered for cpuidle? >> >> I am sure you recollect the latencies involved in OMAP3 OFF mode Vs >> OMAP4+ OFF mode - which basically kicked out OFF mode from OMAP4 >> cpuidle C states? - it was practically useless >> >> in this *specific* power state we are attempting, we do a bunch of i2c >> operations, etc, in short something that cannot even be considered for >> cpuidle. >> >> Considering this, we can consider the same only for suspend path - >> hence allowing firmware to do more here. >> >> >> This does not conflict with cpuidle (which controls MPU) or runtime PM >> (which kicks in once you have drivers active, but if drivers get >> active, we dont need to deal with this crap). >> >> Dont you think this helps the specific case to move this into firmware >> rather than into omap_device? > > No, I don't. > > That means the firmware design is based on several assumptions about > what Linux can and can't do in idle, and then imposing that on future > Linux designs as well. I dont' buy it. > > Kevin I was taking a look at this situation, figuring that the suspend/resume callbacks in omap_device might be the right place to do it, and came across this: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=72bb6f9b51c82c820ddef892455a85b115460904 Under what situations would the driver callbacks be present if probe fails? I'm looking at really_probe in drivers/base/dd.c, and if probe fails, dev->driver is set to NULL. What was the condition this was protecting against? Also, by modifying _od_suspend_noirq, can we force idle unbound omap device? Would we need a new omap_hwmod flag to check which devices should be force idled if no driver is bound?
Hi Russ, Russ Dill <russ.dill@gmail.com> writes: [...] > I was taking a look at this situation, figuring that the > suspend/resume callbacks in omap_device might be the right place to do > it, and came across this: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=72bb6f9b51c82c820ddef892455a85b115460904 > > Under what situations would the driver callbacks be present if probe > fails? I'm looking at really_probe in drivers/base/dd.c, and if probe > fails, dev->driver is set to NULL. What was the condition this was > protecting against? The static suspend/resume methods for PM domains don't check for dev->driver. (c.f. device_resume_noirq()), so during system suspend, the PM domain callbacks are called whether or not there is a driver. A simlar fix could've been done by checking for the existence of dev->driver (and maybe that's a better solution), but I chose the latter so omap_device has a finer grained track of the driver status. > Also, by modifying _od_suspend_noirq, can we force idle unbound omap > device? Would we need a new omap_hwmod flag to check which devices > should be force idled if no driver is bound? I suppose you could, but that would probably mask driver bugs where the driver is not properly runtime suspending itself before being removed. Kevin
On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach <d-gerlach@ti.com> wrote: > From: Vaibhav Bedia <vaibhav.bedia@ti.com> > > AM335x supports various low power modes as documented > in section 8.1.4.3 of the AM335x TRM which is available > @ http://www.ti.com/litv/pdf/spruh73f > > DeepSleep0 mode offers the lowest power mode with limited > wakeup sources without a system reboot and is mapped as > the suspend state in the kernel. In this state, MPU and > PER domains are turned off with the internal RAM held in > retention to facilitate resume process. As part of the boot > process, the assembly code is copied over to OCMCRAM using > the OMAP SRAM code. > > AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU > in DeepSleep0 entry and exit. WKUP_M3 takes care of the > clockdomain and powerdomain transitions based on the > intended low power state. MPU needs to load the appropriate > WKUP_M3 binary onto the WKUP_M3 memory space before it can > leverage any of the PM features like DeepSleep. > > The IPC mechanism between MPU and WKUP_M3 uses a mailbox > sub-module and 8 IPC registers in the Control module. MPU > uses the assigned Mailbox for issuing an interrupt to > WKUP_M3 which then goes and checks the IPC registers for > the payload. WKUP_M3 has the ability to trigger on interrupt > to MPU by executing the "sev" instruction. > > In the current implementation when the suspend process > is initiated MPU interrupts the WKUP_M3 to let it know about > the intent of entering DeepSleep0 and waits for an ACK. When > the ACK is received MPU continues with its suspend process > to suspend all the drivers and then jumps to assembly in > OCMC RAM. The assembly code puts the PLLs in bypass, puts the > external RAM in self-refresh mode and then finally execute the > WFI instruction. Execution of the WFI instruction triggers another > interrupt to the WKUP_M3 which then continues wiht the power down > sequence wherein the clockdomain and powerdomain transition takes > place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt > lines for the wakeup sources. WFI execution on WKUP_M3 causes the > hardware to disable the main oscillator of the SoC. > > When a wakeup event occurs, WKUP_M3 starts the power-up > sequence by switching on the power domains and finally > enabling the clock to MPU. Since the MPU gets powered down > as part of the sleep sequence in the resume path ROM code > starts executing. The ROM code detects a wakeup from sleep > and then jumps to the resume location in OCMC which was > populated in one of the IPC registers as part of the suspend > sequence. > > The low level code in OCMC relocks the PLLs, enables access > to external RAM and then jumps to the cpu_resume code of > the kernel to finish the resume process. > > Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com> > Signed-off-by: Dave Gerlach <d-gerlach@ti.com> > Cc: Tony Lingren <tony@atomide.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Benoit Cousson <benoit.cousson@linaro.org> > Cc: Paul Walmsley <paul@pwsan.com> > Cc: Kevin Hilman <khilman@linaro.org> > --- > arch/arm/mach-omap2/pm33xx.c | 474 +++++++++++++++++++++++++++++++++++++++++ > arch/arm/mach-omap2/pm33xx.h | 77 +++++++ > arch/arm/mach-omap2/wkup_m3.c | 183 ++++++++++++++++ > 3 files changed, 734 insertions(+) > create mode 100644 arch/arm/mach-omap2/pm33xx.c > create mode 100644 arch/arm/mach-omap2/pm33xx.h > create mode 100644 arch/arm/mach-omap2/wkup_m3.c > > diff --git a/arch/arm/mach-omap2/pm33xx.c b/arch/arm/mach-omap2/pm33xx.c > new file mode 100644 > index 0000000..d291c76 > --- /dev/null > +++ b/arch/arm/mach-omap2/pm33xx.c > @@ -0,0 +1,474 @@ > +/* > + * AM33XX Power Management Routines > + * > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ > + * Vaibhav Bedia <vaibhav.bedia@ti.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/cpu.h> > +#include <linux/err.h> > +#include <linux/firmware.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/sched.h> > +#include <linux/suspend.h> > +#include <linux/completion.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/ti_emif.h> > +#include <linux/omap-mailbox.h> > + > +#include <asm/suspend.h> > +#include <asm/proc-fns.h> > +#include <asm/sizes.h> > +#include <asm/fncpy.h> > +#include <asm/system_misc.h> > + > +#include "pm.h" > +#include "cm33xx.h" > +#include "pm33xx.h" > +#include "control.h" > +#include "common.h" > +#include "clockdomain.h" > +#include "powerdomain.h" > +#include "omap_hwmod.h" > +#include "omap_device.h" > +#include "soc.h" > +#include "sram.h" > + > +static void __iomem *am33xx_emif_base; > +static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm; > +static struct clockdomain *gfx_l4ls_clkdm; > + > +struct wakeup_src wakeups[] = { > + {.irq_nr = 35, .src = "USB0_PHY"}, > + {.irq_nr = 36, .src = "USB1_PHY"}, > + {.irq_nr = 40, .src = "I2C0"}, > + {.irq_nr = 41, .src = "RTC Timer"}, > + {.irq_nr = 42, .src = "RTC Alarm"}, > + {.irq_nr = 43, .src = "Timer0"}, > + {.irq_nr = 44, .src = "Timer1"}, > + {.irq_nr = 45, .src = "UART"}, > + {.irq_nr = 46, .src = "GPIO0"}, > + {.irq_nr = 48, .src = "MPU_WAKE"}, > + {.irq_nr = 49, .src = "WDT0"}, > + {.irq_nr = 50, .src = "WDT1"}, > + {.irq_nr = 51, .src = "ADC_TSC"}, > +}; > + > +struct forced_standby_module am33xx_mod[] = { > + {.oh_name = "usb_otg_hs"}, > + {.oh_name = "tptc0"}, > + {.oh_name = "tptc1"}, > + {.oh_name = "tptc2"}, > + {.oh_name = "cpgmac0"}, > +}; > + > +static struct am33xx_pm_context *am33xx_pm; > + > +static DECLARE_COMPLETION(am33xx_pm_sync); > + > +static void (*am33xx_do_wfi_sram)(struct am33xx_suspend_params *); > + > +static struct am33xx_suspend_params susp_params; > + > +#ifdef CONFIG_SUSPEND > + > +static int am33xx_do_sram_idle(long unsigned int unused) > +{ > + am33xx_do_wfi_sram(&susp_params); > + return 0; > +} > + > +static int am33xx_pm_suspend(void) > +{ > + int i, j, ret = 0; > + > + int status = 0; > + struct platform_device *pdev; > + struct omap_device *od; > + > + /* > + * By default the following IPs do not have MSTANDBY asserted > + * which is necessary for PER domain transition. If the drivers > + * are not compiled into the kernel HWMOD code will not change the > + * state of the IPs if the IP was not never enabled. To ensure > + * that there no issues with or without the drivers being compiled > + * in the kernel, we forcefully put these IPs to idle. > + */ > + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { > + pdev = to_platform_device(am33xx_mod[i].dev); > + od = to_omap_device(pdev); > + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { > + omap_device_enable_hwmods(od); > + omap_device_idle_hwmods(od); > + } > + } > + > + /* Try to put GFX to sleep */ > + omap_set_pwrdm_state(gfx_pwrdm, PWRDM_POWER_OFF); > + ret = cpu_suspend(0, am33xx_do_sram_idle); > + > + status = pwrdm_read_prev_pwrst(gfx_pwrdm); > + if (status != PWRDM_POWER_OFF) > + pr_err("PM: GFX domain did not transition\n"); > + else > + pr_info("PM: GFX domain entered low power state\n"); > + > + /* > + * BUG: GFX_L4LS clock domain needs to be woken up to > + * ensure thet L4LS clock domain does not get stuck in transition > + * If that happens L3 module does not get disabled, thereby leading > + * to PER power domain transition failing > + */ > + clkdm_wakeup(gfx_l4ls_clkdm); > + clkdm_sleep(gfx_l4ls_clkdm); > + > + if (ret) { > + pr_err("PM: Kernel suspend failure\n"); > + } else { > + i = am33xx_pm_status(); > + switch (i) { > + case 0: > + pr_info("PM: Successfully put all powerdomains to target state\n"); > + > + /* > + * The PRCM registers on AM335x do not contain previous state > + * information like those present on OMAP4 so we must manually > + * indicate transition so state counters are properly incremented > + */ > + pwrdm_post_transition(mpu_pwrdm); > + pwrdm_post_transition(per_pwrdm); > + break; > + case 1: > + pr_err("PM: Could not transition all powerdomains to target state\n"); > + ret = -1; > + break; > + default: > + pr_err("PM: CM3 returned unknown result :(\nStatus = %d\n", i); > + ret = -1; > + } > + > + /* print the wakeup reason */ > + i = am33xx_pm_wake_src(); > + for (j = 0; j < ARRAY_SIZE(wakeups); j++) { > + if (wakeups[j].irq_nr == i) { > + pr_info("PM: Wakeup source %s\n", wakeups[j].src); > + break; > + } > + } > + > + if (j == ARRAY_SIZE(wakeups)) > + pr_info("PM: Unknown wakeup source %d!\n", i); > + } > + > + return ret; > +} > + > +static int am33xx_pm_enter(suspend_state_t suspend_state) > +{ > + int ret = 0; > + > + switch (suspend_state) { > + case PM_SUSPEND_STANDBY: > + case PM_SUSPEND_MEM: > + ret = am33xx_pm_suspend(); > + break; > + default: > + ret = -EINVAL; > + } > + > + return ret; > +} > + > +/* returns the error code from msg_send - 0 for success, failure otherwise */ > +static int am33xx_ping_wkup_m3(void) > +{ > + int ret = 0; > + > + /* > + * Write a dummy message to the mailbox in order to trigger the RX > + * interrupt to alert the M3 that data is available in the IPC > + * registers. > + */ > + ret = omap_mbox_msg_send(am33xx_pm->mbox, 0xABCDABCD); > + > + return ret; > +} > + > +static void am33xx_m3_state_machine_reset(void) > +{ > + int i; > + > + am33xx_pm->ipc.sleep_mode = IPC_CMD_RESET; > + > + am33xx_pm_ipc_cmd(&am33xx_pm->ipc); > + > + am33xx_pm->state = M3_STATE_MSG_FOR_RESET; > + > + pr_info("PM: Sending message for resetting M3 state machine\n"); > + > + if (!am33xx_ping_wkup_m3()) { > + i = wait_for_completion_timeout(&am33xx_pm_sync, > + msecs_to_jiffies(500)); > + if (WARN(i == 0, "PM: MPU<->CM3 sync failure\n")) > + am33xx_pm->state = M3_STATE_UNKNOWN; > + } else { > + pr_warn("PM: Unable to ping CM3\n"); > + } > +} > + > +static int am33xx_pm_begin(suspend_state_t state) > +{ > + int i; > + > + cpu_idle_poll_ctrl(true); > + > + am33xx_pm->ipc.sleep_mode = IPC_CMD_DS0; > + am33xx_pm->ipc.param1 = DS_IPC_DEFAULT; > + am33xx_pm->ipc.param2 = DS_IPC_DEFAULT; > + > + am33xx_pm_ipc_cmd(&am33xx_pm->ipc); > + > + am33xx_pm->state = M3_STATE_MSG_FOR_LP; > + > + pr_info("PM: Sending message for entering DeepSleep mode\n"); > + > + if (!am33xx_ping_wkup_m3()) { > + i = wait_for_completion_timeout(&am33xx_pm_sync, > + msecs_to_jiffies(500)); > + if (WARN(i == 0, "PM: MPU<->CM3 sync failure\n")) > + return -1; > + } else { > + pr_warn("PM: Unable to ping CM3\n"); > + } > + > + return 0; > +} > + > +static void am33xx_pm_end(void) > +{ > + am33xx_m3_state_machine_reset(); > + > + cpu_idle_poll_ctrl(false); > + > + return; > +} > + > +static struct platform_suspend_ops am33xx_pm_ops = { > + .begin = am33xx_pm_begin, > + .end = am33xx_pm_end, > + .enter = am33xx_pm_enter, > +}; > + > +/* > + * Dummy notifier for the mailbox > + */ > + > +static int wkup_mbox_msg(struct notifier_block *self, unsigned long len, > + void *msg) > +{ > + return 0; > +} > + > +static struct notifier_block wkup_mbox_notifier = { > + .notifier_call = wkup_mbox_msg, > +}; > + > +void am33xx_txev_handler(void) > +{ > + switch (am33xx_pm->state) { > + case M3_STATE_RESET: > + am33xx_pm->state = M3_STATE_INITED; > + am33xx_pm->ver = am33xx_pm_version_get(); > + if (am33xx_pm->ver == M3_VERSION_UNKNOWN || > + am33xx_pm->ver < M3_BASELINE_VERSION) { > + pr_warn("PM: CM3 Firmware Version %x not supported\n", > + am33xx_pm->ver); > + } else { > + pr_info("PM: CM3 Firmware Version = 0x%x\n", > + am33xx_pm->ver); > + am33xx_pm_ops.valid = suspend_valid_only_mem; > + } > + break; > + case M3_STATE_MSG_FOR_RESET: > + am33xx_pm->state = M3_STATE_INITED; > + complete(&am33xx_pm_sync); > + break; > + case M3_STATE_MSG_FOR_LP: > + complete(&am33xx_pm_sync); > + break; > + case M3_STATE_UNKNOWN: > + pr_warn("PM: Unknown CM3 State\n"); > + } > + > + return; > +} > + > +static void am33xx_pm_firmware_cb(const struct firmware *fw, void *context) > +{ > + struct am33xx_pm_context *am33xx_pm = context; > + int ret = 0; > + > + /* no firmware found */ > + if (!fw) { > + pr_err("PM: request_firmware failed\n"); > + return; > + } > + > + wkup_m3_copy_code(fw->data, fw->size); > + > + wkup_m3_register_txev_handler(am33xx_txev_handler); > + > + pr_info("PM: Copied the M3 firmware to UMEM\n"); > + > + /* > + * Invalidate M3 firmware version before hardreset. > + * Write invalid version in lower 4 nibbles of parameter > + * register (ipc_regs + 0x8). > + */ > + am33xx_pm_version_clear(); > + > + am33xx_pm->state = M3_STATE_RESET; > + > + ret = wkup_m3_prepare(); > + if (ret) { > + pr_err("PM: Could not prepare WKUP_M3\n"); > + return; > + } > + > + /* Physical resume address to be used by ROM code */ > + am33xx_pm->ipc.resume_addr = (AM33XX_OCMC_END - > + am33xx_do_wfi_sz + am33xx_resume_offset + 0x4); > + > + am33xx_pm->mbox = omap_mbox_get("wkup_m3", &wkup_mbox_notifier); > + > + if (IS_ERR(am33xx_pm->mbox)) { > + ret = -EBUSY; > + pr_err("PM: IPC Request for A8->M3 Channel failed!\n"); > + return; > + } else { > + suspend_set_ops(&am33xx_pm_ops); > + } > + > + return; > +} > + > +#endif /* CONFIG_SUSPEND */ > + > +/* > + * Push the minimal suspend-resume code to SRAM > + */ > +void am33xx_push_sram_idle(void) > +{ > + am33xx_do_wfi_sram = (void *)omap_sram_push > + (am33xx_do_wfi, am33xx_do_wfi_sz); > +} > + > +static int __init am33xx_map_emif(void) > +{ > + am33xx_emif_base = ioremap(AM33XX_EMIF_BASE, SZ_32K); > + > + if (!am33xx_emif_base) > + return -ENOMEM; > + > + return 0; > +} > + > +int __init am33xx_pm_init(void) > +{ > + int ret; > + u32 temp; > + struct device_node *np; > + int i; > + > + if (!soc_is_am33xx()) > + return -ENODEV; > + > + pr_info("Power Management for AM33XX family\n"); > + > + /* > + * By default the following IPs do not have MSTANDBY asserted > + * which is necessary for PER domain transition. If the drivers > + * are not compiled into the kernel HWMOD code will not change the > + * state of the IPs if the IP was not never enabled > + */ > + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) > + am33xx_mod[i].dev = omap_device_get_by_hwmod_name(am33xx_mod[i].oh_name); > + > + gfx_pwrdm = pwrdm_lookup("gfx_pwrdm"); > + per_pwrdm = pwrdm_lookup("per_pwrdm"); > + mpu_pwrdm = pwrdm_lookup("mpu_pwrdm"); > + > + gfx_l4ls_clkdm = clkdm_lookup("gfx_l4ls_gfx_clkdm"); > + > + if ((!gfx_pwrdm) || (!per_pwrdm) || (!mpu_pwrdm) || (!gfx_l4ls_clkdm)) { > + ret = -ENODEV; > + goto err; > + } > + > + am33xx_pm = kzalloc(sizeof(*am33xx_pm), GFP_KERNEL); > + if (!am33xx_pm) { > + pr_err("Memory allocation failed\n"); > + ret = -ENOMEM; > + goto err; > + } > + > + ret = am33xx_map_emif(); > + if (ret) { > + pr_err("PM: Could not ioremap EMIF\n"); > + goto err; > + } > + /* Determine Memory Type */ > + temp = readl(am33xx_emif_base + EMIF_SDRAM_CONFIG); > + temp = (temp & SDRAM_TYPE_MASK) >> SDRAM_TYPE_SHIFT; > + /* Parameters to pass to aseembly code */ > + susp_params.emif_addr_virt = am33xx_emif_base; > + susp_params.dram_sync = am33xx_dram_sync; > + susp_params.mem_type = temp; > + am33xx_pm->ipc.param3 = temp; > + > + np = of_find_compatible_node(NULL, NULL, "ti,am3353-wkup-m3"); > + if (np) { > + if (of_find_property(np, "ti,needs_vtt_toggle", NULL) && > + (!(of_property_read_u32(np, "vtt-gpio-pin", > + &temp)))) { > + if (temp >= 0 && temp <= 31) > + am33xx_pm->ipc.param3 |= > + ((1 << VTT_STAT_SHIFT) | > + (temp << VTT_GPIO_PIN_SHIFT)); > + } > + } > + > + (void) clkdm_for_each(omap_pm_clkdms_setup, NULL); > + > + /* CEFUSE domain can be turned off post bootup */ > + cefuse_pwrdm = pwrdm_lookup("cefuse_pwrdm"); > + if (cefuse_pwrdm) > + omap_set_pwrdm_state(cefuse_pwrdm, PWRDM_POWER_OFF); > + else > + pr_err("PM: Failed to get cefuse_pwrdm\n"); > + > +#ifdef CONFIG_SUSPEND > + pr_info("PM: Trying to load am335x-pm-firmware.bin"); > + > + /* We don't want to delay boot */ > + request_firmware_nowait(THIS_MODULE, 0, "am335x-pm-firmware.bin", > + NULL, GFP_KERNEL, am33xx_pm, > + am33xx_pm_firmware_cb); > +#endif /* CONFIG_SUSPEND */ > + > +err: > + return ret; > +} > diff --git a/arch/arm/mach-omap2/pm33xx.h b/arch/arm/mach-omap2/pm33xx.h > new file mode 100644 > index 0000000..befdd11 > --- /dev/null > +++ b/arch/arm/mach-omap2/pm33xx.h > @@ -0,0 +1,77 @@ > +/* > + * AM33XX Power Management Routines > + * > + * Copyright (C) 2012 Texas Instruments Inc. > + * Vaibhav Bedia <vaibhav.bedia@ti.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef __ARCH_ARM_MACH_OMAP2_PM33XX_H > +#define __ARCH_ARM_MACH_OMAP2_PM33XX_H > + > +#include "control.h" > + > +#ifndef __ASSEMBLER__ > + > +struct am33xx_pm_context { > + struct am33xx_ipc_data ipc; > + struct firmware *firmware; > + struct omap_mbox *mbox; > + u8 state; > + u32 ver; > +}; > + > +/* > + * Params passed to suspend routine > + * > + * Since these are used to load into registers by suspend code, > + * entries here must always be in sync with the suspend code > + * in arm/mach-omap2/sleep33xx.S > + */ > +struct am33xx_suspend_params { > + void __iomem *emif_addr_virt; > + u32 mem_type; > + void __iomem *dram_sync; > +}; > + > +struct wakeup_src { > + int irq_nr; > + char src[10]; > +}; > + > +struct forced_standby_module { > + char oh_name[15]; > + struct device *dev; > +}; > + > +int wkup_m3_copy_code(const u8 *data, size_t size); > +int wkup_m3_prepare(void); > +void wkup_m3_register_txev_handler(void (*txev_handler)(void)); > + > +#endif > + > +#define IPC_CMD_DS0 0x3 > +#define IPC_CMD_RESET 0xe > +#define DS_IPC_DEFAULT 0xffffffff > +#define M3_VERSION_UNKNOWN 0x0000ffff > +#define M3_BASELINE_VERSION 0x21 > + > +#define M3_STATE_UNKNOWN 0 > +#define M3_STATE_RESET 1 > +#define M3_STATE_INITED 2 > +#define M3_STATE_MSG_FOR_LP 3 > +#define M3_STATE_MSG_FOR_RESET 4 > + > +#define AM33XX_OCMC_END 0x40310000 > +#define AM33XX_EMIF_BASE 0x4C000000 > + > +#define MEM_TYPE_DDR2 2 > + > +#endif > diff --git a/arch/arm/mach-omap2/wkup_m3.c b/arch/arm/mach-omap2/wkup_m3.c > new file mode 100644 > index 0000000..8eaa7f3 > --- /dev/null > +++ b/arch/arm/mach-omap2/wkup_m3.c > @@ -0,0 +1,183 @@ > +/* > + * AM33XX Power Management Routines > + * > + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ > + * Vaibhav Bedia <vaibhav.bedia@ti.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/slab.h> > +#include <linux/cpu.h> > +#include <linux/err.h> > +#include <linux/firmware.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/of.h> > + > +#include "pm33xx.h" > +#include "control.h" > +#include "omap_device.h" > +#include "soc.h" > + > +struct wkup_m3_context { > + struct device *dev; > + void __iomem *code; > + void (*txev_handler)(void); > +}; > + > +struct wkup_m3_context *wkup_m3; > + > +int wkup_m3_copy_code(const u8 *data, size_t size) > +{ > + if (size > SZ_16K) > + return -ENOMEM; > + > + memcpy_toio(wkup_m3->code, data, size); > + > + return 0; > +} > + > + > +void wkup_m3_register_txev_handler(void (*txev_handler)(void)) > +{ > + wkup_m3->txev_handler = txev_handler; > +} > + > +/* have platforms do what they want in atomic context over here? */ > +static irqreturn_t wkup_m3_txev_handler(int irq, void *unused) > +{ > + am33xx_txev_eoi(); > + > + /* callback to be executed in atomic context */ > + /* return 0 implies IRQ_HANDLED else IRQ_NONE */ > + wkup_m3->txev_handler(); > + > + am33xx_txev_enable(); > + > + return IRQ_HANDLED; > +} > + > +int wkup_m3_prepare(void) > +{ > + struct platform_device *pdev = to_platform_device(wkup_m3->dev); > + > + /* check that the code is loaded */ > + omap_device_deassert_hardreset(pdev, "wkup_m3"); > + > + return 0; > +} > + > +static int wkup_m3_probe(struct platform_device *pdev) > +{ > + int irq, ret = 0; > + struct resource *mem; > + > + pm_runtime_enable(&pdev->dev); > + > + ret = pm_runtime_get_sync(&pdev->dev); > + if (IS_ERR_VALUE(ret)) { > + dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n"); > + return ret; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (!irq) { > + dev_err(wkup_m3->dev, "no irq resource\n"); &pdev->dev > + ret = -ENXIO; > + goto err; > + } > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!mem) { > + dev_err(wkup_m3->dev, "no memory resource\n"); &pdev->dev > + ret = -ENXIO; > + goto err; > + } > + > + wkup_m3 = kzalloc(sizeof(*wkup_m3), GFP_KERNEL); > + if (!wkup_m3) { > + pr_err("Memory allocation failed\n"); > + ret = -ENOMEM; > + goto err; > + } > + > + wkup_m3->dev = &pdev->dev; > + > + wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, mem); > + if (!wkup_m3->code) { > + dev_err(wkup_m3->dev, "could not ioremap\n"); > + ret = -EADDRNOTAVAIL; > + goto err; > + } > + > + ret = devm_request_irq(wkup_m3->dev, irq, wkup_m3_txev_handler, > + IRQF_DISABLED, "wkup_m3_txev", NULL); > + if (ret) { > + dev_err(wkup_m3->dev, "request_irq failed\n"); > + goto err; > + } > + > +err: > + return ret; > +} > + > +static int wkup_m3_remove(struct platform_device *pdev) > +{ > + return 0; > +} > + > +static struct of_device_id wkup_m3_dt_ids[] = { > + { .compatible = "ti,am3353-wkup-m3" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, wkup_m3_dt_ids); > + > +static int wkup_m3_rpm_suspend(struct device *dev) > +{ > + return -EBUSY; > +} > + > +static int wkup_m3_rpm_resume(struct device *dev) > +{ > + return 0; > +} > + > +static const struct dev_pm_ops wkup_m3_ops = { > + SET_RUNTIME_PM_OPS(wkup_m3_rpm_suspend, wkup_m3_rpm_resume, NULL) > +}; > + > +static struct platform_driver wkup_m3_driver = { > + .probe = wkup_m3_probe, > + .remove = wkup_m3_remove, > + .driver = { > + .name = "wkup_m3", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(wkup_m3_dt_ids), > + .pm = &wkup_m3_ops, > + }, > +}; > + > +static __init int wkup_m3_init(void) > +{ > + return platform_driver_register(&wkup_m3_driver); > +} > + > +static __exit void wkup_m3_exit(void) > +{ > + platform_driver_unregister(&wkup_m3_driver); > +} > +omap_postcore_initcall(wkup_m3_init); > +module_exit(wkup_m3_exit); > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Russ Dill <russ.dill@gmail.com> writes: > On Tue, Aug 6, 2013 at 10:49 AM, Dave Gerlach <d-gerlach@ti.com> wrote: [...] >> +static int wkup_m3_probe(struct platform_device *pdev) >> +{ >> + int irq, ret = 0; >> + struct resource *mem; >> + >> + pm_runtime_enable(&pdev->dev); >> + >> + ret = pm_runtime_get_sync(&pdev->dev); >> + if (IS_ERR_VALUE(ret)) { >> + dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n"); >> + return ret; >> + } >> + >> + irq = platform_get_irq(pdev, 0); >> + if (!irq) { >> + dev_err(wkup_m3->dev, "no irq resource\n"); > > &pdev->dev > >> + ret = -ENXIO; >> + goto err; >> + } >> + >> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!mem) { >> + dev_err(wkup_m3->dev, "no memory resource\n"); > > &pdev->dev For future reference, when reviewing, please trim to only relevant content/context, especially on large patches so maintainers/reviewers do not have to find 2 one-line comments in a huge amount of irrelevant context. Thanks, Kevin
Hi folks, catching up on this thread. On 08/06/2013 12:49 PM, Dave Gerlach wrote: > + > +static int am33xx_pm_suspend(void) > +{ > + int i, j, ret = 0; > + > + int status = 0; > + struct platform_device *pdev; > + struct omap_device *od; > + > + /* > + * By default the following IPs do not have MSTANDBY asserted > + * which is necessary for PER domain transition. If the drivers > + * are not compiled into the kernel HWMOD code will not change the > + * state of the IPs if the IP was not never enabled. To ensure > + * that there no issues with or without the drivers being compiled > + * in the kernel, we forcefully put these IPs to idle. > + */ > + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { > + pdev = to_platform_device(am33xx_mod[i].dev); > + od = to_omap_device(pdev); > + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { > + omap_device_enable_hwmods(od); > + omap_device_idle_hwmods(od); > + } > + } Does this have to be done for every suspend entry, or can it just be done once during kernel initialization? If the latter, shouldn't this be done by hwmod during the initial reset and idle of all of these devices, based on a flag? For example, we had this flag for OMAP3630: * HWMOD_FORCE_MSTANDBY: Always keep MIDLEMODE bits cleared so that device * is kept in force-standby mode. Failing to do so causes PM problems * with musb on OMAP3630 at least. Note that musb has a dedicated register * to control MSTANDBY signal when MIDLEMODE is set to force-standby. - Paul
On 08/20/2013 05:48 PM, Paul Walmsley wrote: > > Hi folks, > > catching up on this thread. > > On 08/06/2013 12:49 PM, Dave Gerlach wrote: > >> + >> +static int am33xx_pm_suspend(void) >> +{ >> + int i, j, ret = 0; >> + >> + int status = 0; >> + struct platform_device *pdev; >> + struct omap_device *od; >> + >> + /* >> + * By default the following IPs do not have MSTANDBY asserted >> + * which is necessary for PER domain transition. If the drivers >> + * are not compiled into the kernel HWMOD code will not change the >> + * state of the IPs if the IP was not never enabled. To ensure >> + * that there no issues with or without the drivers being compiled >> + * in the kernel, we forcefully put these IPs to idle. >> + */ >> + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { >> + pdev = to_platform_device(am33xx_mod[i].dev); >> + od = to_omap_device(pdev); >> + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { >> + omap_device_enable_hwmods(od); >> + omap_device_idle_hwmods(od); >> + } >> + } > > Does this have to be done for every suspend entry, or can it just be done > once during kernel initialization? > > If the latter, shouldn't this be done by hwmod during the initial reset > and idle of all of these devices, based on a flag? For example, we had > this flag for OMAP3630: > > * HWMOD_FORCE_MSTANDBY: Always keep MIDLEMODE bits cleared so that device > * is kept in force-standby mode. Failing to do so causes PM problems > * with musb on OMAP3630 at least. Note that musb has a dedicated > register > * to control MSTANDBY signal when MIDLEMODE is set to force-standby. > Hi, Unfortunately this does have to be done at some point after every suspend/resume cycle because while the IPs are idled during initial reset, after a suspend cycle the context loss when no driver is bound causes MSTANDBY to be unasserted again which as mentioned breaks the PER power domain transition during the next suspend attempt. The current plan for this is somewhat similar to what you mentioned, all of the troublesome modules will be flagged in hwmod and when the hwmods are loaded they are tracked if no driver gets bound and then idled post resume by a pm_notifier. Regards, Dave > > - Paul > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
Dave Gerlach <d-gerlach@ti.com> writes: > From: Vaibhav Bedia <vaibhav.bedia@ti.com> > > AM335x supports various low power modes as documented > in section 8.1.4.3 of the AM335x TRM which is available > @ http://www.ti.com/litv/pdf/spruh73f > > DeepSleep0 mode offers the lowest power mode with limited > wakeup sources without a system reboot and is mapped as > the suspend state in the kernel. In this state, MPU and > PER domains are turned off with the internal RAM held in > retention to facilitate resume process. As part of the boot > process, the assembly code is copied over to OCMCRAM using > the OMAP SRAM code. > > AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU > in DeepSleep0 entry and exit. WKUP_M3 takes care of the > clockdomain and powerdomain transitions based on the > intended low power state. MPU needs to load the appropriate > WKUP_M3 binary onto the WKUP_M3 memory space before it can > leverage any of the PM features like DeepSleep. > > The IPC mechanism between MPU and WKUP_M3 uses a mailbox > sub-module and 8 IPC registers in the Control module. MPU > uses the assigned Mailbox for issuing an interrupt to > WKUP_M3 which then goes and checks the IPC registers for > the payload. WKUP_M3 has the ability to trigger on interrupt s/trigger on interrupt/trigger an interrupt/ ?? > to MPU by executing the "sev" instruction. > > In the current implementation when the suspend process > is initiated MPU interrupts the WKUP_M3 to let it know about > the intent of entering DeepSleep0 and waits for an ACK. When > the ACK is received MPU continues with its suspend process > to suspend all the drivers and then jumps to assembly in > OCMC RAM. The assembly code puts the PLLs in bypass, puts the > external RAM in self-refresh mode and then finally execute the > WFI instruction. Execution of the WFI instruction triggers another > interrupt to the WKUP_M3 which then continues wiht the power down > sequence wherein the clockdomain and powerdomain transition takes > place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt > lines for the wakeup sources. WFI execution on WKUP_M3 causes the > hardware to disable the main oscillator of the SoC. > > When a wakeup event occurs, WKUP_M3 starts the power-up > sequence by switching on the power domains and finally > enabling the clock to MPU. Since the MPU gets powered down > as part of the sleep sequence in the resume path ROM code > starts executing. The ROM code detects a wakeup from sleep > and then jumps to the resume location in OCMC which was > populated in one of the IPC registers as part of the suspend > sequence. > > The low level code in OCMC relocks the PLLs, enables access > to external RAM and then jumps to the cpu_resume code of > the kernel to finish the resume process. [...] > arch/arm/mach-omap2/pm33xx.c | 474 +++++++++++++++++++++++++++++++++++++++++ > arch/arm/mach-omap2/pm33xx.h | 77 +++++++ > arch/arm/mach-omap2/wkup_m3.c | 183 ++++++++++++++++ Looking closer at this code as I'm trying to fully get my head around all the IPC, I have some more comments. I think the split between pm33xx.c and the M3 driver is still confusing here. For example, am33xx_ping_wkup_m3(), am33xx_m3_state_machine_reset() and the guts of am33xx_pm_begin() all belong inside the M3 driver, along with all the wakeup_src stuff, which is info coming from the M3. IOW, the communication with M3 should be abstracted from pm33xx by the M3 driver (or possibly an eventual remoteproc/rpmsg implementation) with a well defined API. In this implementation, the interface is pretty fuzzy and mixed between pm33xx.c and wkup_m3.c. Kevin P.S. I'd also suggest renaming wakeup_src to something else since it's close to wakeup_source which has a rather different meaning in the kernel (c.f. linux/pm_wakeup.h)
On 08/27/2013 04:45 PM, Kevin Hilman wrote: > Dave Gerlach <d-gerlach@ti.com> writes: > >> From: Vaibhav Bedia <vaibhav.bedia@ti.com> >> >> AM335x supports various low power modes as documented >> in section 8.1.4.3 of the AM335x TRM which is available >> @ http://www.ti.com/litv/pdf/spruh73f >> >> DeepSleep0 mode offers the lowest power mode with limited >> wakeup sources without a system reboot and is mapped as >> the suspend state in the kernel. In this state, MPU and >> PER domains are turned off with the internal RAM held in >> retention to facilitate resume process. As part of the boot >> process, the assembly code is copied over to OCMCRAM using >> the OMAP SRAM code. >> >> AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU >> in DeepSleep0 entry and exit. WKUP_M3 takes care of the >> clockdomain and powerdomain transitions based on the >> intended low power state. MPU needs to load the appropriate >> WKUP_M3 binary onto the WKUP_M3 memory space before it can >> leverage any of the PM features like DeepSleep. >> >> The IPC mechanism between MPU and WKUP_M3 uses a mailbox >> sub-module and 8 IPC registers in the Control module. MPU >> uses the assigned Mailbox for issuing an interrupt to >> WKUP_M3 which then goes and checks the IPC registers for >> the payload. WKUP_M3 has the ability to trigger on interrupt > > s/trigger on interrupt/trigger an interrupt/ ?? Oops I will fix that. > >> to MPU by executing the "sev" instruction. >> >> In the current implementation when the suspend process >> is initiated MPU interrupts the WKUP_M3 to let it know about >> the intent of entering DeepSleep0 and waits for an ACK. When >> the ACK is received MPU continues with its suspend process >> to suspend all the drivers and then jumps to assembly in >> OCMC RAM. The assembly code puts the PLLs in bypass, puts the >> external RAM in self-refresh mode and then finally execute the >> WFI instruction. Execution of the WFI instruction triggers another >> interrupt to the WKUP_M3 which then continues wiht the power down >> sequence wherein the clockdomain and powerdomain transition takes >> place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt >> lines for the wakeup sources. WFI execution on WKUP_M3 causes the >> hardware to disable the main oscillator of the SoC. >> >> When a wakeup event occurs, WKUP_M3 starts the power-up >> sequence by switching on the power domains and finally >> enabling the clock to MPU. Since the MPU gets powered down >> as part of the sleep sequence in the resume path ROM code >> starts executing. The ROM code detects a wakeup from sleep >> and then jumps to the resume location in OCMC which was >> populated in one of the IPC registers as part of the suspend >> sequence. >> >> The low level code in OCMC relocks the PLLs, enables access >> to external RAM and then jumps to the cpu_resume code of >> the kernel to finish the resume process. > > [...] > >> arch/arm/mach-omap2/pm33xx.c | 474 +++++++++++++++++++++++++++++++++++++++++ >> arch/arm/mach-omap2/pm33xx.h | 77 +++++++ >> arch/arm/mach-omap2/wkup_m3.c | 183 ++++++++++++++++ > > > Looking closer at this code as I'm trying to fully get my head around > all the IPC, I have some more comments. > > I think the split between pm33xx.c and the M3 driver is still confusing > here. For example, am33xx_ping_wkup_m3(), > am33xx_m3_state_machine_reset() and the guts of am33xx_pm_begin() all > belong inside the M3 driver, along with all the wakeup_src stuff, which > is info coming from the M3. > > IOW, the communication with M3 should be abstracted from pm33xx by the > M3 driver (or possibly an eventual remoteproc/rpmsg implementation) with > a well defined API. In this implementation, the interface is pretty > fuzzy and mixed between pm33xx.c and wkup_m3.c. I have since moved much more of the m3 functionality, including the ping and wakeup src code, into the wkup_m3 driver to make the split more clear but I haven't yet moved the state machine portion into the wkup_m3 driver. I feel that this is the portion of the IPC that could potentially be the most variant between different SoC implementations so leaving this in the pm code should allow for more flexibility. > > Kevin > > P.S. I'd also suggest renaming wakeup_src to something else since > it's close to wakeup_source which has a rather different meaning in the > kernel (c.f. linux/pm_wakeup.h) > I will rename this to tie it into the M3 code. Regards, Dave
Dave Gerlach <d-gerlach@ti.com> writes: > On 08/27/2013 04:45 PM, Kevin Hilman wrote: >> Dave Gerlach <d-gerlach@ti.com> writes: >> >>> From: Vaibhav Bedia <vaibhav.bedia@ti.com> >>> >>> AM335x supports various low power modes as documented >>> in section 8.1.4.3 of the AM335x TRM which is available >>> @ http://www.ti.com/litv/pdf/spruh73f >>> >>> DeepSleep0 mode offers the lowest power mode with limited >>> wakeup sources without a system reboot and is mapped as >>> the suspend state in the kernel. In this state, MPU and >>> PER domains are turned off with the internal RAM held in >>> retention to facilitate resume process. As part of the boot >>> process, the assembly code is copied over to OCMCRAM using >>> the OMAP SRAM code. >>> >>> AM335x has a Cortex-M3 (WKUP_M3) which assists the MPU >>> in DeepSleep0 entry and exit. WKUP_M3 takes care of the >>> clockdomain and powerdomain transitions based on the >>> intended low power state. MPU needs to load the appropriate >>> WKUP_M3 binary onto the WKUP_M3 memory space before it can >>> leverage any of the PM features like DeepSleep. >>> >>> The IPC mechanism between MPU and WKUP_M3 uses a mailbox >>> sub-module and 8 IPC registers in the Control module. MPU >>> uses the assigned Mailbox for issuing an interrupt to >>> WKUP_M3 which then goes and checks the IPC registers for >>> the payload. WKUP_M3 has the ability to trigger on interrupt >> >> s/trigger on interrupt/trigger an interrupt/ ?? > > Oops I will fix that. > >> >>> to MPU by executing the "sev" instruction. >>> >>> In the current implementation when the suspend process >>> is initiated MPU interrupts the WKUP_M3 to let it know about >>> the intent of entering DeepSleep0 and waits for an ACK. When >>> the ACK is received MPU continues with its suspend process >>> to suspend all the drivers and then jumps to assembly in >>> OCMC RAM. The assembly code puts the PLLs in bypass, puts the >>> external RAM in self-refresh mode and then finally execute the >>> WFI instruction. Execution of the WFI instruction triggers another >>> interrupt to the WKUP_M3 which then continues wiht the power down >>> sequence wherein the clockdomain and powerdomain transition takes >>> place. As part of the sleep sequence, WKUP_M3 unmasks the interrupt >>> lines for the wakeup sources. WFI execution on WKUP_M3 causes the >>> hardware to disable the main oscillator of the SoC. >>> >>> When a wakeup event occurs, WKUP_M3 starts the power-up >>> sequence by switching on the power domains and finally >>> enabling the clock to MPU. Since the MPU gets powered down >>> as part of the sleep sequence in the resume path ROM code >>> starts executing. The ROM code detects a wakeup from sleep >>> and then jumps to the resume location in OCMC which was >>> populated in one of the IPC registers as part of the suspend >>> sequence. >>> >>> The low level code in OCMC relocks the PLLs, enables access >>> to external RAM and then jumps to the cpu_resume code of >>> the kernel to finish the resume process. >> >> [...] >> >>> arch/arm/mach-omap2/pm33xx.c | 474 +++++++++++++++++++++++++++++++++++++++++ >>> arch/arm/mach-omap2/pm33xx.h | 77 +++++++ >>> arch/arm/mach-omap2/wkup_m3.c | 183 ++++++++++++++++ >> >> >> Looking closer at this code as I'm trying to fully get my head around >> all the IPC, I have some more comments. >> >> I think the split between pm33xx.c and the M3 driver is still confusing >> here. For example, am33xx_ping_wkup_m3(), >> am33xx_m3_state_machine_reset() and the guts of am33xx_pm_begin() all >> belong inside the M3 driver, along with all the wakeup_src stuff, which >> is info coming from the M3. >> >> IOW, the communication with M3 should be abstracted from pm33xx by the >> M3 driver (or possibly an eventual remoteproc/rpmsg implementation) with >> a well defined API. In this implementation, the interface is pretty >> fuzzy and mixed between pm33xx.c and wkup_m3.c. > > I have since moved much more of the m3 functionality, including the > ping and wakeup src code, into the wkup_m3 driver to make the split > more clear but I haven't yet moved the state machine portion into the > wkup_m3 driver. I feel that this is the portion of the IPC that could > potentially be the most variant between different SoC implementations > so leaving this in the pm code should allow for more flexibility. I still think this belogs in the M3 driver, because AFAICT, it's specific to the M3 firmware, not to the SoC. >> >> Kevin >> >> P.S. I'd also suggest renaming wakeup_src to something else since >> it's close to wakeup_source which has a rather different meaning in the >> kernel (c.f. linux/pm_wakeup.h) >> > > I will rename this to tie it into the M3 code. Great, thanks. Kevin
(picking up an old thread, again) On Thu, Aug 8, 2013 at 7:04 PM, Kevin Hilman <khilman@linaro.org> wrote: > > I disagree here. I'm a firmware minimalist, and hiding bugs like this > in the firmware is wrong when Linux is otherwise managing these devices. > It also imposes criteria on the firmware of future SoCs that doesn't > belong there either. IMO, the only stuff the firmware should do is what > Linux *cannot* do. > > Remember, this only needs to happen when there isn't a driver for these > devices. Should we communicate to the firmware that the OS has no > driver, so please enable the hack? I think not. > Agreed on not hiding the bugs in the firmware. Moreover, M3 can't access the IPs in PER domain which is where the bad modules are, so the h/w doesn't support such hackery (+1 for the h/w after all the -1's that it gets ;) [...] >>> That being said, IMO, the kernel (specifically omap_device) should >>> handle this, and it should be rather easy to do in the omap_device layer >>> and keep the SoC suspend/resume core code simple and ignorant of these >>> "quirks." >>> >>> AFAICT, there's no reason these quirks need to be dealt with immediatly >>> on suspend. A slight delay should be fine, as long as it's before the >>> next suspend/idle attempt, right? >>> >>> Given that, what we need to do (and by we, I mean you) is to flag all >>> broken IP blocks, and let omap_device handle them in a suspend/resume >>> notifier (c.f. register_pm_notifier() and PM_POST_SUSPEND.) >> >> yes - that is the alternate that comes to mind. > > In the earlier reviews of this series (many months ago now), I > complained about the presence of this device specific handling in the > core MPU PM code. I'm somewhat troubled by the fact that nobody explored > alternatives that so easily come to mind. > My bad. I was thinking along those lines [1] but after the suggestion on using the driver bound status i just went with that suggestion in the dumbest possible manner. Regards, Vaibhav [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-November/129676.html
On Tue, Aug 27, 2013 at 5:45 PM, Kevin Hilman <khilman@linaro.org> wrote: [...] > > Looking closer at this code as I'm trying to fully get my head around > all the IPC, I have some more comments. > > I think the split between pm33xx.c and the M3 driver is still confusing > here. For example, am33xx_ping_wkup_m3(), > am33xx_m3_state_machine_reset() and the guts of am33xx_pm_begin() all > belong inside the M3 driver, along with all the wakeup_src stuff, which > is info coming from the M3. > > IOW, the communication with M3 should be abstracted from pm33xx by the > M3 driver (or possibly an eventual remoteproc/rpmsg implementation) with > a well defined API. In this implementation, the interface is pretty > fuzzy and mixed between pm33xx.c and wkup_m3.c. > > The reason for the current split was to have the M3 driver just do the minimal that's needed to talk to get M3 and MPU talking. What made me do it this way was to attempt to address a previous comment on keeping options open for folks to use M3 for things other than PM stuff. The IPC stuff is how implementors of the firmware (anything other than the PM one that TI provides) want it to be. The top level idea was to have the users of the firmware (PM in this case) decide what functionality they want when talking to M3. They are also free to decide the register bitfield layout and other IPC details. This was also a feeble attempt to keep things extensible for AM437x where in addition to the broken mailbox usage there's now a control module bit to trigger the interrupt to M3 (what's worse? pick one that you hate more ;) The AM437x PM routines could then just register different callbacks that are triggered when the M3 interrupts the MPU. Hope this clears up some of the confusion. Regards, Vaibhav
Vaibhav Bedia <vaibhav.bedia@gmail.com> writes: > On Tue, Aug 27, 2013 at 5:45 PM, Kevin Hilman <khilman@linaro.org> wrote: > [...] >> >> Looking closer at this code as I'm trying to fully get my head around >> all the IPC, I have some more comments. >> >> I think the split between pm33xx.c and the M3 driver is still confusing >> here. For example, am33xx_ping_wkup_m3(), >> am33xx_m3_state_machine_reset() and the guts of am33xx_pm_begin() all >> belong inside the M3 driver, along with all the wakeup_src stuff, which >> is info coming from the M3. >> >> IOW, the communication with M3 should be abstracted from pm33xx by the >> M3 driver (or possibly an eventual remoteproc/rpmsg implementation) with >> a well defined API. In this implementation, the interface is pretty >> fuzzy and mixed between pm33xx.c and wkup_m3.c. >> >> > > The reason for the current split was to have the M3 driver just do the minimal > that's needed to talk to get M3 and MPU talking. What made me do it this way > was to attempt to address a previous comment on keeping options open for folks > to use M3 for things other than PM stuff. The IPC stuff is how > implementors of the > firmware (anything other than the PM one that TI provides) want it to be. IMO, there should actually be 3 levels. the SoC PM implementation (pm33xx.c), the M3 driver where the protocol, state-machine, etc. are handled, and the messaging layer. In the current proposal, the last 2 are combined, but I'd really like to see a generalized messaging layer that everyone else using an M3 coprocessor might use as well. As mentioned already, I think that should be rpmsg, but that still needs some exploration. > The top level idea was to have the users of the firmware (PM in this case) > decide what functionality they want when talking to M3. They are also free to > decide the register bitfield layout and other IPC details. > > This was also a feeble attempt to keep things extensible for AM437x where > in addition to the broken mailbox usage there's now a control module bit > to trigger the interrupt to M3 (what's worse? pick one that you hate more ;) Sounds like AM43xx is better. If you have a control module bit to trigger an interrupt, why do you need the mailbox at all? > The AM437x PM routines could then just register different callbacks that > are triggered when the M3 interrupts the MPU. > > Hope this clears up some of the confusion. Thanks, Kevin
diff --git a/arch/arm/mach-omap2/pm33xx.c b/arch/arm/mach-omap2/pm33xx.c new file mode 100644 index 0000000..d291c76 --- /dev/null +++ b/arch/arm/mach-omap2/pm33xx.c @@ -0,0 +1,474 @@ +/* + * AM33XX Power Management Routines + * + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * Vaibhav Bedia <vaibhav.bedia@ti.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/cpu.h> +#include <linux/err.h> +#include <linux/firmware.h> +#include <linux/io.h> +#include <linux/platform_device.h> +#include <linux/sched.h> +#include <linux/suspend.h> +#include <linux/completion.h> +#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/ti_emif.h> +#include <linux/omap-mailbox.h> + +#include <asm/suspend.h> +#include <asm/proc-fns.h> +#include <asm/sizes.h> +#include <asm/fncpy.h> +#include <asm/system_misc.h> + +#include "pm.h" +#include "cm33xx.h" +#include "pm33xx.h" +#include "control.h" +#include "common.h" +#include "clockdomain.h" +#include "powerdomain.h" +#include "omap_hwmod.h" +#include "omap_device.h" +#include "soc.h" +#include "sram.h" + +static void __iomem *am33xx_emif_base; +static struct powerdomain *cefuse_pwrdm, *gfx_pwrdm, *per_pwrdm, *mpu_pwrdm; +static struct clockdomain *gfx_l4ls_clkdm; + +struct wakeup_src wakeups[] = { + {.irq_nr = 35, .src = "USB0_PHY"}, + {.irq_nr = 36, .src = "USB1_PHY"}, + {.irq_nr = 40, .src = "I2C0"}, + {.irq_nr = 41, .src = "RTC Timer"}, + {.irq_nr = 42, .src = "RTC Alarm"}, + {.irq_nr = 43, .src = "Timer0"}, + {.irq_nr = 44, .src = "Timer1"}, + {.irq_nr = 45, .src = "UART"}, + {.irq_nr = 46, .src = "GPIO0"}, + {.irq_nr = 48, .src = "MPU_WAKE"}, + {.irq_nr = 49, .src = "WDT0"}, + {.irq_nr = 50, .src = "WDT1"}, + {.irq_nr = 51, .src = "ADC_TSC"}, +}; + +struct forced_standby_module am33xx_mod[] = { + {.oh_name = "usb_otg_hs"}, + {.oh_name = "tptc0"}, + {.oh_name = "tptc1"}, + {.oh_name = "tptc2"}, + {.oh_name = "cpgmac0"}, +}; + +static struct am33xx_pm_context *am33xx_pm; + +static DECLARE_COMPLETION(am33xx_pm_sync); + +static void (*am33xx_do_wfi_sram)(struct am33xx_suspend_params *); + +static struct am33xx_suspend_params susp_params; + +#ifdef CONFIG_SUSPEND + +static int am33xx_do_sram_idle(long unsigned int unused) +{ + am33xx_do_wfi_sram(&susp_params); + return 0; +} + +static int am33xx_pm_suspend(void) +{ + int i, j, ret = 0; + + int status = 0; + struct platform_device *pdev; + struct omap_device *od; + + /* + * By default the following IPs do not have MSTANDBY asserted + * which is necessary for PER domain transition. If the drivers + * are not compiled into the kernel HWMOD code will not change the + * state of the IPs if the IP was not never enabled. To ensure + * that there no issues with or without the drivers being compiled + * in the kernel, we forcefully put these IPs to idle. + */ + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) { + pdev = to_platform_device(am33xx_mod[i].dev); + od = to_omap_device(pdev); + if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER) { + omap_device_enable_hwmods(od); + omap_device_idle_hwmods(od); + } + } + + /* Try to put GFX to sleep */ + omap_set_pwrdm_state(gfx_pwrdm, PWRDM_POWER_OFF); + ret = cpu_suspend(0, am33xx_do_sram_idle); + + status = pwrdm_read_prev_pwrst(gfx_pwrdm); + if (status != PWRDM_POWER_OFF) + pr_err("PM: GFX domain did not transition\n"); + else + pr_info("PM: GFX domain entered low power state\n"); + + /* + * BUG: GFX_L4LS clock domain needs to be woken up to + * ensure thet L4LS clock domain does not get stuck in transition + * If that happens L3 module does not get disabled, thereby leading + * to PER power domain transition failing + */ + clkdm_wakeup(gfx_l4ls_clkdm); + clkdm_sleep(gfx_l4ls_clkdm); + + if (ret) { + pr_err("PM: Kernel suspend failure\n"); + } else { + i = am33xx_pm_status(); + switch (i) { + case 0: + pr_info("PM: Successfully put all powerdomains to target state\n"); + + /* + * The PRCM registers on AM335x do not contain previous state + * information like those present on OMAP4 so we must manually + * indicate transition so state counters are properly incremented + */ + pwrdm_post_transition(mpu_pwrdm); + pwrdm_post_transition(per_pwrdm); + break; + case 1: + pr_err("PM: Could not transition all powerdomains to target state\n"); + ret = -1; + break; + default: + pr_err("PM: CM3 returned unknown result :(\nStatus = %d\n", i); + ret = -1; + } + + /* print the wakeup reason */ + i = am33xx_pm_wake_src(); + for (j = 0; j < ARRAY_SIZE(wakeups); j++) { + if (wakeups[j].irq_nr == i) { + pr_info("PM: Wakeup source %s\n", wakeups[j].src); + break; + } + } + + if (j == ARRAY_SIZE(wakeups)) + pr_info("PM: Unknown wakeup source %d!\n", i); + } + + return ret; +} + +static int am33xx_pm_enter(suspend_state_t suspend_state) +{ + int ret = 0; + + switch (suspend_state) { + case PM_SUSPEND_STANDBY: + case PM_SUSPEND_MEM: + ret = am33xx_pm_suspend(); + break; + default: + ret = -EINVAL; + } + + return ret; +} + +/* returns the error code from msg_send - 0 for success, failure otherwise */ +static int am33xx_ping_wkup_m3(void) +{ + int ret = 0; + + /* + * Write a dummy message to the mailbox in order to trigger the RX + * interrupt to alert the M3 that data is available in the IPC + * registers. + */ + ret = omap_mbox_msg_send(am33xx_pm->mbox, 0xABCDABCD); + + return ret; +} + +static void am33xx_m3_state_machine_reset(void) +{ + int i; + + am33xx_pm->ipc.sleep_mode = IPC_CMD_RESET; + + am33xx_pm_ipc_cmd(&am33xx_pm->ipc); + + am33xx_pm->state = M3_STATE_MSG_FOR_RESET; + + pr_info("PM: Sending message for resetting M3 state machine\n"); + + if (!am33xx_ping_wkup_m3()) { + i = wait_for_completion_timeout(&am33xx_pm_sync, + msecs_to_jiffies(500)); + if (WARN(i == 0, "PM: MPU<->CM3 sync failure\n")) + am33xx_pm->state = M3_STATE_UNKNOWN; + } else { + pr_warn("PM: Unable to ping CM3\n"); + } +} + +static int am33xx_pm_begin(suspend_state_t state) +{ + int i; + + cpu_idle_poll_ctrl(true); + + am33xx_pm->ipc.sleep_mode = IPC_CMD_DS0; + am33xx_pm->ipc.param1 = DS_IPC_DEFAULT; + am33xx_pm->ipc.param2 = DS_IPC_DEFAULT; + + am33xx_pm_ipc_cmd(&am33xx_pm->ipc); + + am33xx_pm->state = M3_STATE_MSG_FOR_LP; + + pr_info("PM: Sending message for entering DeepSleep mode\n"); + + if (!am33xx_ping_wkup_m3()) { + i = wait_for_completion_timeout(&am33xx_pm_sync, + msecs_to_jiffies(500)); + if (WARN(i == 0, "PM: MPU<->CM3 sync failure\n")) + return -1; + } else { + pr_warn("PM: Unable to ping CM3\n"); + } + + return 0; +} + +static void am33xx_pm_end(void) +{ + am33xx_m3_state_machine_reset(); + + cpu_idle_poll_ctrl(false); + + return; +} + +static struct platform_suspend_ops am33xx_pm_ops = { + .begin = am33xx_pm_begin, + .end = am33xx_pm_end, + .enter = am33xx_pm_enter, +}; + +/* + * Dummy notifier for the mailbox + */ + +static int wkup_mbox_msg(struct notifier_block *self, unsigned long len, + void *msg) +{ + return 0; +} + +static struct notifier_block wkup_mbox_notifier = { + .notifier_call = wkup_mbox_msg, +}; + +void am33xx_txev_handler(void) +{ + switch (am33xx_pm->state) { + case M3_STATE_RESET: + am33xx_pm->state = M3_STATE_INITED; + am33xx_pm->ver = am33xx_pm_version_get(); + if (am33xx_pm->ver == M3_VERSION_UNKNOWN || + am33xx_pm->ver < M3_BASELINE_VERSION) { + pr_warn("PM: CM3 Firmware Version %x not supported\n", + am33xx_pm->ver); + } else { + pr_info("PM: CM3 Firmware Version = 0x%x\n", + am33xx_pm->ver); + am33xx_pm_ops.valid = suspend_valid_only_mem; + } + break; + case M3_STATE_MSG_FOR_RESET: + am33xx_pm->state = M3_STATE_INITED; + complete(&am33xx_pm_sync); + break; + case M3_STATE_MSG_FOR_LP: + complete(&am33xx_pm_sync); + break; + case M3_STATE_UNKNOWN: + pr_warn("PM: Unknown CM3 State\n"); + } + + return; +} + +static void am33xx_pm_firmware_cb(const struct firmware *fw, void *context) +{ + struct am33xx_pm_context *am33xx_pm = context; + int ret = 0; + + /* no firmware found */ + if (!fw) { + pr_err("PM: request_firmware failed\n"); + return; + } + + wkup_m3_copy_code(fw->data, fw->size); + + wkup_m3_register_txev_handler(am33xx_txev_handler); + + pr_info("PM: Copied the M3 firmware to UMEM\n"); + + /* + * Invalidate M3 firmware version before hardreset. + * Write invalid version in lower 4 nibbles of parameter + * register (ipc_regs + 0x8). + */ + am33xx_pm_version_clear(); + + am33xx_pm->state = M3_STATE_RESET; + + ret = wkup_m3_prepare(); + if (ret) { + pr_err("PM: Could not prepare WKUP_M3\n"); + return; + } + + /* Physical resume address to be used by ROM code */ + am33xx_pm->ipc.resume_addr = (AM33XX_OCMC_END - + am33xx_do_wfi_sz + am33xx_resume_offset + 0x4); + + am33xx_pm->mbox = omap_mbox_get("wkup_m3", &wkup_mbox_notifier); + + if (IS_ERR(am33xx_pm->mbox)) { + ret = -EBUSY; + pr_err("PM: IPC Request for A8->M3 Channel failed!\n"); + return; + } else { + suspend_set_ops(&am33xx_pm_ops); + } + + return; +} + +#endif /* CONFIG_SUSPEND */ + +/* + * Push the minimal suspend-resume code to SRAM + */ +void am33xx_push_sram_idle(void) +{ + am33xx_do_wfi_sram = (void *)omap_sram_push + (am33xx_do_wfi, am33xx_do_wfi_sz); +} + +static int __init am33xx_map_emif(void) +{ + am33xx_emif_base = ioremap(AM33XX_EMIF_BASE, SZ_32K); + + if (!am33xx_emif_base) + return -ENOMEM; + + return 0; +} + +int __init am33xx_pm_init(void) +{ + int ret; + u32 temp; + struct device_node *np; + int i; + + if (!soc_is_am33xx()) + return -ENODEV; + + pr_info("Power Management for AM33XX family\n"); + + /* + * By default the following IPs do not have MSTANDBY asserted + * which is necessary for PER domain transition. If the drivers + * are not compiled into the kernel HWMOD code will not change the + * state of the IPs if the IP was not never enabled + */ + for (i = 0; i < ARRAY_SIZE(am33xx_mod); i++) + am33xx_mod[i].dev = omap_device_get_by_hwmod_name(am33xx_mod[i].oh_name); + + gfx_pwrdm = pwrdm_lookup("gfx_pwrdm"); + per_pwrdm = pwrdm_lookup("per_pwrdm"); + mpu_pwrdm = pwrdm_lookup("mpu_pwrdm"); + + gfx_l4ls_clkdm = clkdm_lookup("gfx_l4ls_gfx_clkdm"); + + if ((!gfx_pwrdm) || (!per_pwrdm) || (!mpu_pwrdm) || (!gfx_l4ls_clkdm)) { + ret = -ENODEV; + goto err; + } + + am33xx_pm = kzalloc(sizeof(*am33xx_pm), GFP_KERNEL); + if (!am33xx_pm) { + pr_err("Memory allocation failed\n"); + ret = -ENOMEM; + goto err; + } + + ret = am33xx_map_emif(); + if (ret) { + pr_err("PM: Could not ioremap EMIF\n"); + goto err; + } + /* Determine Memory Type */ + temp = readl(am33xx_emif_base + EMIF_SDRAM_CONFIG); + temp = (temp & SDRAM_TYPE_MASK) >> SDRAM_TYPE_SHIFT; + /* Parameters to pass to aseembly code */ + susp_params.emif_addr_virt = am33xx_emif_base; + susp_params.dram_sync = am33xx_dram_sync; + susp_params.mem_type = temp; + am33xx_pm->ipc.param3 = temp; + + np = of_find_compatible_node(NULL, NULL, "ti,am3353-wkup-m3"); + if (np) { + if (of_find_property(np, "ti,needs_vtt_toggle", NULL) && + (!(of_property_read_u32(np, "vtt-gpio-pin", + &temp)))) { + if (temp >= 0 && temp <= 31) + am33xx_pm->ipc.param3 |= + ((1 << VTT_STAT_SHIFT) | + (temp << VTT_GPIO_PIN_SHIFT)); + } + } + + (void) clkdm_for_each(omap_pm_clkdms_setup, NULL); + + /* CEFUSE domain can be turned off post bootup */ + cefuse_pwrdm = pwrdm_lookup("cefuse_pwrdm"); + if (cefuse_pwrdm) + omap_set_pwrdm_state(cefuse_pwrdm, PWRDM_POWER_OFF); + else + pr_err("PM: Failed to get cefuse_pwrdm\n"); + +#ifdef CONFIG_SUSPEND + pr_info("PM: Trying to load am335x-pm-firmware.bin"); + + /* We don't want to delay boot */ + request_firmware_nowait(THIS_MODULE, 0, "am335x-pm-firmware.bin", + NULL, GFP_KERNEL, am33xx_pm, + am33xx_pm_firmware_cb); +#endif /* CONFIG_SUSPEND */ + +err: + return ret; +} diff --git a/arch/arm/mach-omap2/pm33xx.h b/arch/arm/mach-omap2/pm33xx.h new file mode 100644 index 0000000..befdd11 --- /dev/null +++ b/arch/arm/mach-omap2/pm33xx.h @@ -0,0 +1,77 @@ +/* + * AM33XX Power Management Routines + * + * Copyright (C) 2012 Texas Instruments Inc. + * Vaibhav Bedia <vaibhav.bedia@ti.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#ifndef __ARCH_ARM_MACH_OMAP2_PM33XX_H +#define __ARCH_ARM_MACH_OMAP2_PM33XX_H + +#include "control.h" + +#ifndef __ASSEMBLER__ + +struct am33xx_pm_context { + struct am33xx_ipc_data ipc; + struct firmware *firmware; + struct omap_mbox *mbox; + u8 state; + u32 ver; +}; + +/* + * Params passed to suspend routine + * + * Since these are used to load into registers by suspend code, + * entries here must always be in sync with the suspend code + * in arm/mach-omap2/sleep33xx.S + */ +struct am33xx_suspend_params { + void __iomem *emif_addr_virt; + u32 mem_type; + void __iomem *dram_sync; +}; + +struct wakeup_src { + int irq_nr; + char src[10]; +}; + +struct forced_standby_module { + char oh_name[15]; + struct device *dev; +}; + +int wkup_m3_copy_code(const u8 *data, size_t size); +int wkup_m3_prepare(void); +void wkup_m3_register_txev_handler(void (*txev_handler)(void)); + +#endif + +#define IPC_CMD_DS0 0x3 +#define IPC_CMD_RESET 0xe +#define DS_IPC_DEFAULT 0xffffffff +#define M3_VERSION_UNKNOWN 0x0000ffff +#define M3_BASELINE_VERSION 0x21 + +#define M3_STATE_UNKNOWN 0 +#define M3_STATE_RESET 1 +#define M3_STATE_INITED 2 +#define M3_STATE_MSG_FOR_LP 3 +#define M3_STATE_MSG_FOR_RESET 4 + +#define AM33XX_OCMC_END 0x40310000 +#define AM33XX_EMIF_BASE 0x4C000000 + +#define MEM_TYPE_DDR2 2 + +#endif diff --git a/arch/arm/mach-omap2/wkup_m3.c b/arch/arm/mach-omap2/wkup_m3.c new file mode 100644 index 0000000..8eaa7f3 --- /dev/null +++ b/arch/arm/mach-omap2/wkup_m3.c @@ -0,0 +1,183 @@ +/* + * AM33XX Power Management Routines + * + * Copyright (C) 2012 Texas Instruments Incorporated - http://www.ti.com/ + * Vaibhav Bedia <vaibhav.bedia@ti.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/slab.h> +#include <linux/cpu.h> +#include <linux/err.h> +#include <linux/firmware.h> +#include <linux/io.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/of.h> + +#include "pm33xx.h" +#include "control.h" +#include "omap_device.h" +#include "soc.h" + +struct wkup_m3_context { + struct device *dev; + void __iomem *code; + void (*txev_handler)(void); +}; + +struct wkup_m3_context *wkup_m3; + +int wkup_m3_copy_code(const u8 *data, size_t size) +{ + if (size > SZ_16K) + return -ENOMEM; + + memcpy_toio(wkup_m3->code, data, size); + + return 0; +} + + +void wkup_m3_register_txev_handler(void (*txev_handler)(void)) +{ + wkup_m3->txev_handler = txev_handler; +} + +/* have platforms do what they want in atomic context over here? */ +static irqreturn_t wkup_m3_txev_handler(int irq, void *unused) +{ + am33xx_txev_eoi(); + + /* callback to be executed in atomic context */ + /* return 0 implies IRQ_HANDLED else IRQ_NONE */ + wkup_m3->txev_handler(); + + am33xx_txev_enable(); + + return IRQ_HANDLED; +} + +int wkup_m3_prepare(void) +{ + struct platform_device *pdev = to_platform_device(wkup_m3->dev); + + /* check that the code is loaded */ + omap_device_deassert_hardreset(pdev, "wkup_m3"); + + return 0; +} + +static int wkup_m3_probe(struct platform_device *pdev) +{ + int irq, ret = 0; + struct resource *mem; + + pm_runtime_enable(&pdev->dev); + + ret = pm_runtime_get_sync(&pdev->dev); + if (IS_ERR_VALUE(ret)) { + dev_err(&pdev->dev, "pm_runtime_get_sync() failed\n"); + return ret; + } + + irq = platform_get_irq(pdev, 0); + if (!irq) { + dev_err(wkup_m3->dev, "no irq resource\n"); + ret = -ENXIO; + goto err; + } + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!mem) { + dev_err(wkup_m3->dev, "no memory resource\n"); + ret = -ENXIO; + goto err; + } + + wkup_m3 = kzalloc(sizeof(*wkup_m3), GFP_KERNEL); + if (!wkup_m3) { + pr_err("Memory allocation failed\n"); + ret = -ENOMEM; + goto err; + } + + wkup_m3->dev = &pdev->dev; + + wkup_m3->code = devm_request_and_ioremap(wkup_m3->dev, mem); + if (!wkup_m3->code) { + dev_err(wkup_m3->dev, "could not ioremap\n"); + ret = -EADDRNOTAVAIL; + goto err; + } + + ret = devm_request_irq(wkup_m3->dev, irq, wkup_m3_txev_handler, + IRQF_DISABLED, "wkup_m3_txev", NULL); + if (ret) { + dev_err(wkup_m3->dev, "request_irq failed\n"); + goto err; + } + +err: + return ret; +} + +static int wkup_m3_remove(struct platform_device *pdev) +{ + return 0; +} + +static struct of_device_id wkup_m3_dt_ids[] = { + { .compatible = "ti,am3353-wkup-m3" }, + { } +}; +MODULE_DEVICE_TABLE(of, wkup_m3_dt_ids); + +static int wkup_m3_rpm_suspend(struct device *dev) +{ + return -EBUSY; +} + +static int wkup_m3_rpm_resume(struct device *dev) +{ + return 0; +} + +static const struct dev_pm_ops wkup_m3_ops = { + SET_RUNTIME_PM_OPS(wkup_m3_rpm_suspend, wkup_m3_rpm_resume, NULL) +}; + +static struct platform_driver wkup_m3_driver = { + .probe = wkup_m3_probe, + .remove = wkup_m3_remove, + .driver = { + .name = "wkup_m3", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(wkup_m3_dt_ids), + .pm = &wkup_m3_ops, + }, +}; + +static __init int wkup_m3_init(void) +{ + return platform_driver_register(&wkup_m3_driver); +} + +static __exit void wkup_m3_exit(void) +{ + platform_driver_unregister(&wkup_m3_driver); +} +omap_postcore_initcall(wkup_m3_init); +module_exit(wkup_m3_exit);