Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Tomasz, On Fri, Jun 20, 2014 at 4:59 PM, Tomasz Figa tomasz.f...@gmail.com wrote: On 21.06.2014 01:53, Doug Anderson wrote: Kevin, On Fri, Jun 20, 2014 at 4:13 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: Kevin, On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman khil...@linaro.org wrote: Hi Doug, Doug Anderson diand...@chromium.org writes: On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Yes, it appears it does. Objection withdrawn. I just wanted to be sure because since the introduction of late/early, the need for noirq should be pretty rare, but there certainly are needs. tangent In this case though, the need for it has more to do with the lack of a way for us to describe non parent-child device dependencies than whether or not IRQs are enabled or not. /tangent Actually, I'm not sure that's true, but I'll talk through it and you can point to where I'm wrong (I often am!) If you're a wakeup device then you need to be ready to handle interrupts as soon as the noirq phase of resume is done, right? As soon as the noirq phase of your own driver is done, correct. Said another way: you need to be ready to handle interrupts _before_ the normal resume code is called and be ready to handle interrupts even _before_ the early resume code is called. Correct. That means if you are implementing a bus that's needed by any devices with wakeup interrupts then it's your responsibility to also be prepared to run this early. In this particular case the max77686 driver doesn't need to do anything at all to be ready to handle interrupts. It's suspend and resume code is just boilerplate enable wakeups / disable wakeups and it has no noirq code. The max77686 driver doesn't have any noirq wake call because it would just be empty. Said another way: the problem isn't that the max77686 wakeup gets called before the i2c wakeup. The problem is that i2c is needed ASAP once IRQs are enabled and thus needs to be run noirq. Does that sound semi-correct? Yes that's correct. My point above was (trying to be) that ultimately this is an ordering issue. e.g. the bus device needs to be ready before wakeup devices on that bus can handle wakeup interrupts etc. The way we're handling that ordering is by the implied ordering of noirq, late/early and normal callbacks. That's convenient, but not exactly obvious. It works because we dont' typically need too many layers here, but it would be much more understandable if we could describe this kind of dependency in a way that the suspend/resume code would suspend/resume things in the right order rather than by tinkering with callback levels (since otherwise suspend/resume ordering just depends on probe order.) This issue then usually gets me headed down my usual rant path about how I think runtime PM is much better suited for handling ordering and dependencies becuase it automatically handles parent/child dependencies and non parent/child dependencies can be handled by taking advantage of the get/put APIs which are refcounted, ect etc. but that's another can worms. Ah, I gotcha. Yes, I'm a fan of having explicit dependency orderings too. So I guess in this case the truly correct way to handle it is: 1. i2c controller should have Runtime PM even though (as per the code now) there's nothing you can do to it to save power under normal circumstances. So the runtime suspend code would be a no-op. 2. When the i2c controller is told to runtime resume, it should double-check if a full SoC poweroff has happened since the last time it checked. In this case it should reinit its hardware. 3. If the i2c controller gets a full resume callback then it should also reinit the hardware just so it's not sitting in a half-configured state until the first peripheral uses it. If later someone finds a way to power gate the i2c controller when no active transfers are going (and we actually save non-trivial power doing this) then we've got a nice place to put that code. NOTE: Unless we can actually save power by power gating the i2c peripheral when there are no active transfers, we
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Doug Anderson diand...@chromium.org writes: [...] On Fri, Jun 20, 2014 at 4:59 PM, Tomasz Figa tomasz.f...@gmail.com wrote: I'm not sure noirq is going to work correctly, at least not with current callbacks. I can see a call to clk_prepare_enable() there which needs to acquire a mutex. Nice catch, thanks! :) OK, looking at that now. Interestingly this doesn't seem to cause us problems in our ChromeOS 3.8 tree. I just tried enabling: CONFIG_DEBUG_ATOMIC_SLEEP=y ...and confirmed that I got it on right: # zgrep -i atomic /proc/config.gz CONFIG_DEBUG_ATOMIC_SLEEP=y I can suspend/resume with no problems. My bet is that it works fine because: * resume_noirq is not considered atomic in the sense enforced by CONFIG_DEBUG_ATOMIC_SLEEP (at least not in 3.8--I haven't tried on ToT) The reason is because noirq in the suspend/resume path actually means no *device* IRQs for that specific device. It's often assumed that the noirq callbacks are called with *all* interrupts disabled, but that's not the case. Only the IRQs for that specific device are disabled when its noirq callbacks run. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Doug Anderson diand...@chromium.org writes: Kevin, On Fri, Jun 20, 2014 at 4:13 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: Kevin, On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman khil...@linaro.org wrote: Hi Doug, Doug Anderson diand...@chromium.org writes: On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Yes, it appears it does. Objection withdrawn. I just wanted to be sure because since the introduction of late/early, the need for noirq should be pretty rare, but there certainly are needs. tangent In this case though, the need for it has more to do with the lack of a way for us to describe non parent-child device dependencies than whether or not IRQs are enabled or not. /tangent Actually, I'm not sure that's true, but I'll talk through it and you can point to where I'm wrong (I often am!) If you're a wakeup device then you need to be ready to handle interrupts as soon as the noirq phase of resume is done, right? As soon as the noirq phase of your own driver is done, correct. Said another way: you need to be ready to handle interrupts _before_ the normal resume code is called and be ready to handle interrupts even _before_ the early resume code is called. Correct. That means if you are implementing a bus that's needed by any devices with wakeup interrupts then it's your responsibility to also be prepared to run this early. In this particular case the max77686 driver doesn't need to do anything at all to be ready to handle interrupts. It's suspend and resume code is just boilerplate enable wakeups / disable wakeups and it has no noirq code. The max77686 driver doesn't have any noirq wake call because it would just be empty. Said another way: the problem isn't that the max77686 wakeup gets called before the i2c wakeup. The problem is that i2c is needed ASAP once IRQs are enabled and thus needs to be run noirq. Does that sound semi-correct? Yes that's correct. My point above was (trying to be) that ultimately this is an ordering issue. e.g. the bus device needs to be ready before wakeup devices on that bus can handle wakeup interrupts etc. The way we're handling that ordering is by the implied ordering of noirq, late/early and normal callbacks. That's convenient, but not exactly obvious. It works because we dont' typically need too many layers here, but it would be much more understandable if we could describe this kind of dependency in a way that the suspend/resume code would suspend/resume things in the right order rather than by tinkering with callback levels (since otherwise suspend/resume ordering just depends on probe order.) This issue then usually gets me headed down my usual rant path about how I think runtime PM is much better suited for handling ordering and dependencies becuase it automatically handles parent/child dependencies and non parent/child dependencies can be handled by taking advantage of the get/put APIs which are refcounted, ect etc. but that's another can worms. Ah, I gotcha. Yes, I'm a fan of having explicit dependency orderings too. So I guess in this case the truly correct way to handle it is: 1. i2c controller should have Runtime PM even though (as per the code now) there's nothing you can do to it to save power under normal circumstances. So the runtime suspend code would be a no-op. 2. When the i2c controller is told to runtime resume, it should double-check if a full SoC poweroff has happened since the last time it checked. In this case it should reinit its hardware. 3. If the i2c controller gets a full resume callback then it should also reinit the hardware just so it's not sitting in a half-configured state until the first peripheral uses it. If later someone finds a way to power gate the i2c controller when no active transfers are going (and we actually save non-trivial power doing this) then we've got a nice place to put that code. NOTE: Unless we can actually save power by power gating the i2c peripheral when there are no active transfers, we would also just have the i2c_xfer() init the hardware if needed. Maybe that's kinda
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
On 24.06.2014 00:19, Kevin Hilman wrote: Doug Anderson diand...@chromium.org writes: [...] On Fri, Jun 20, 2014 at 4:59 PM, Tomasz Figa tomasz.f...@gmail.com wrote: I'm not sure noirq is going to work correctly, at least not with current callbacks. I can see a call to clk_prepare_enable() there which needs to acquire a mutex. Nice catch, thanks! :) OK, looking at that now. Interestingly this doesn't seem to cause us problems in our ChromeOS 3.8 tree. I just tried enabling: CONFIG_DEBUG_ATOMIC_SLEEP=y ...and confirmed that I got it on right: # zgrep -i atomic /proc/config.gz CONFIG_DEBUG_ATOMIC_SLEEP=y I can suspend/resume with no problems. My bet is that it works fine because: * resume_noirq is not considered atomic in the sense enforced by CONFIG_DEBUG_ATOMIC_SLEEP (at least not in 3.8--I haven't tried on ToT) The reason is because noirq in the suspend/resume path actually means no *device* IRQs for that specific device. It's often assumed that the noirq callbacks are called with *all* interrupts disabled, but that's not the case. Only the IRQs for that specific device are disabled when its noirq callbacks run. Thanks for clarifying this. This means that we should be fine with the noirq variant then. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Kevin, On Mon, Jun 23, 2014 at 3:19 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: [...] On Fri, Jun 20, 2014 at 4:59 PM, Tomasz Figa tomasz.f...@gmail.com wrote: I'm not sure noirq is going to work correctly, at least not with current callbacks. I can see a call to clk_prepare_enable() there which needs to acquire a mutex. Nice catch, thanks! :) OK, looking at that now. Interestingly this doesn't seem to cause us problems in our ChromeOS 3.8 tree. I just tried enabling: CONFIG_DEBUG_ATOMIC_SLEEP=y ...and confirmed that I got it on right: # zgrep -i atomic /proc/config.gz CONFIG_DEBUG_ATOMIC_SLEEP=y I can suspend/resume with no problems. My bet is that it works fine because: * resume_noirq is not considered atomic in the sense enforced by CONFIG_DEBUG_ATOMIC_SLEEP (at least not in 3.8--I haven't tried on ToT) The reason is because noirq in the suspend/resume path actually means no *device* IRQs for that specific device. It's often assumed that the noirq callbacks are called with *all* interrupts disabled, but that's not the case. Only the IRQs for that specific device are disabled when its noirq callbacks run. Ah, so even with my fix of moving to noirq we could still be broken if the system decided to enable interrupts for the device before the i2c controller get resumed then we'd still be SOL. ...oh, but if it matches probe order then maybe we're guaranteed for that not to happen? We know that we will probe the i2c bus before the devices on it, right? -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
On 24.06.2014 00:27, Doug Anderson wrote: Kevin, On Mon, Jun 23, 2014 at 3:19 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: [...] On Fri, Jun 20, 2014 at 4:59 PM, Tomasz Figa tomasz.f...@gmail.com wrote: I'm not sure noirq is going to work correctly, at least not with current callbacks. I can see a call to clk_prepare_enable() there which needs to acquire a mutex. Nice catch, thanks! :) OK, looking at that now. Interestingly this doesn't seem to cause us problems in our ChromeOS 3.8 tree. I just tried enabling: CONFIG_DEBUG_ATOMIC_SLEEP=y ...and confirmed that I got it on right: # zgrep -i atomic /proc/config.gz CONFIG_DEBUG_ATOMIC_SLEEP=y I can suspend/resume with no problems. My bet is that it works fine because: * resume_noirq is not considered atomic in the sense enforced by CONFIG_DEBUG_ATOMIC_SLEEP (at least not in 3.8--I haven't tried on ToT) The reason is because noirq in the suspend/resume path actually means no *device* IRQs for that specific device. It's often assumed that the noirq callbacks are called with *all* interrupts disabled, but that's not the case. Only the IRQs for that specific device are disabled when its noirq callbacks run. Ah, so even with my fix of moving to noirq we could still be broken if the system decided to enable interrupts for the device before the i2c controller get resumed then we'd still be SOL. ...oh, but if it matches probe order then maybe we're guaranteed for that not to happen? We know that we will probe the i2c bus before the devices on it, right? If the mentioned device is a child of the I2C controller then the parent-child relation determines the order. Otherwise (e.g. another, non-I2C interrupt source that just triggers some operation on an I2C device like voltage regulator) we're doomed. ;) Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Kevin, On Mon, Jun 23, 2014 at 3:23 PM, Kevin Hilman khil...@linaro.org wrote: So I guess in this case the truly correct way to handle it is: 1. i2c controller should have Runtime PM even though (as per the code now) there's nothing you can do to it to save power under normal circumstances. So the runtime suspend code would be a no-op. 2. When the i2c controller is told to runtime resume, it should double-check if a full SoC poweroff has happened since the last time it checked. In this case it should reinit its hardware. 3. If the i2c controller gets a full resume callback then it should also reinit the hardware just so it's not sitting in a half-configured state until the first peripheral uses it. If later someone finds a way to power gate the i2c controller when no active transfers are going (and we actually save non-trivial power doing this) then we've got a nice place to put that code. NOTE: Unless we can actually save power by power gating the i2c peripheral when there are no active transfers, we would also just have the i2c_xfer() init the hardware if needed. Maybe that's kinda gross, though. Yes, this is how we manage the i2c controller on OMAP. Essentially, between every xfer, the hw is disabled and can potentially lose context, so eveery xfer requires a hw init. We use the runtime PM autosuspend feature so that it stays alive for X milliseconds so bursty i2c xfers are not punished. Have a look at drivers/i2c/busses/i2c-omap.c. You'll notice there are not callbacks for system suspend/resume, it's only doing runtime PM. OK, cool! That might be a bit too aggressive of a change for what I can take on right now. I've filed http://crbug.com/388007 to see if Samsung can take a look at this. -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Tomasz, On Mon, Jun 23, 2014 at 3:31 PM, Tomasz Figa tomasz.f...@gmail.com wrote: On 24.06.2014 00:27, Doug Anderson wrote: Kevin, On Mon, Jun 23, 2014 at 3:19 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: [...] On Fri, Jun 20, 2014 at 4:59 PM, Tomasz Figa tomasz.f...@gmail.com wrote: I'm not sure noirq is going to work correctly, at least not with current callbacks. I can see a call to clk_prepare_enable() there which needs to acquire a mutex. Nice catch, thanks! :) OK, looking at that now. Interestingly this doesn't seem to cause us problems in our ChromeOS 3.8 tree. I just tried enabling: CONFIG_DEBUG_ATOMIC_SLEEP=y ...and confirmed that I got it on right: # zgrep -i atomic /proc/config.gz CONFIG_DEBUG_ATOMIC_SLEEP=y I can suspend/resume with no problems. My bet is that it works fine because: * resume_noirq is not considered atomic in the sense enforced by CONFIG_DEBUG_ATOMIC_SLEEP (at least not in 3.8--I haven't tried on ToT) The reason is because noirq in the suspend/resume path actually means no *device* IRQs for that specific device. It's often assumed that the noirq callbacks are called with *all* interrupts disabled, but that's not the case. Only the IRQs for that specific device are disabled when its noirq callbacks run. Ah, so even with my fix of moving to noirq we could still be broken if the system decided to enable interrupts for the device before the i2c controller get resumed then we'd still be SOL. ...oh, but if it matches probe order then maybe we're guaranteed for that not to happen? We know that we will probe the i2c bus before the devices on it, right? If the mentioned device is a child of the I2C controller then the parent-child relation determines the order. Otherwise (e.g. another, non-I2C interrupt source that just triggers some operation on an I2C device like voltage regulator) we're doomed. ;) Wow, that would be seriously screwed up. OK, so to summarize my current plans: I won't spin this patch and we can see what Wolfram thinks. It may not be as beautiful as Kevin's suggestion to use Runtime PM but I also don't think it's insane. ...and I've got a request in to Samsung to use Runtime PM in the long run. If anyone at Samsung working on suspend/resume on exynos5420-pit or exynos5800-pi wants to add their Tested-by (or bug reports) I'm sure that would be appreciated. -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Doug Anderson diand...@chromium.org writes: Kevin, On Mon, Jun 23, 2014 at 3:23 PM, Kevin Hilman khil...@linaro.org wrote: So I guess in this case the truly correct way to handle it is: 1. i2c controller should have Runtime PM even though (as per the code now) there's nothing you can do to it to save power under normal circumstances. So the runtime suspend code would be a no-op. 2. When the i2c controller is told to runtime resume, it should double-check if a full SoC poweroff has happened since the last time it checked. In this case it should reinit its hardware. 3. If the i2c controller gets a full resume callback then it should also reinit the hardware just so it's not sitting in a half-configured state until the first peripheral uses it. If later someone finds a way to power gate the i2c controller when no active transfers are going (and we actually save non-trivial power doing this) then we've got a nice place to put that code. NOTE: Unless we can actually save power by power gating the i2c peripheral when there are no active transfers, we would also just have the i2c_xfer() init the hardware if needed. Maybe that's kinda gross, though. Yes, this is how we manage the i2c controller on OMAP. Essentially, between every xfer, the hw is disabled and can potentially lose context, so eveery xfer requires a hw init. We use the runtime PM autosuspend feature so that it stays alive for X milliseconds so bursty i2c xfers are not punished. Have a look at drivers/i2c/busses/i2c-omap.c. You'll notice there are not callbacks for system suspend/resume, it's only doing runtime PM. OK, cool! That might be a bit too aggressive of a change for what I can take on right now. I've filed http://crbug.com/388007 to see if Samsung can take a look at this. Sure. While I think moving to runtime PM is the right thing to do, that alone shouldn't block this patch. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Tomasz Figa tomasz.f...@gmail.com writes: On 24.06.2014 00:27, Doug Anderson wrote: Kevin, On Mon, Jun 23, 2014 at 3:19 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: [...] On Fri, Jun 20, 2014 at 4:59 PM, Tomasz Figa tomasz.f...@gmail.com wrote: I'm not sure noirq is going to work correctly, at least not with current callbacks. I can see a call to clk_prepare_enable() there which needs to acquire a mutex. Nice catch, thanks! :) OK, looking at that now. Interestingly this doesn't seem to cause us problems in our ChromeOS 3.8 tree. I just tried enabling: CONFIG_DEBUG_ATOMIC_SLEEP=y ...and confirmed that I got it on right: # zgrep -i atomic /proc/config.gz CONFIG_DEBUG_ATOMIC_SLEEP=y I can suspend/resume with no problems. My bet is that it works fine because: * resume_noirq is not considered atomic in the sense enforced by CONFIG_DEBUG_ATOMIC_SLEEP (at least not in 3.8--I haven't tried on ToT) The reason is because noirq in the suspend/resume path actually means no *device* IRQs for that specific device. It's often assumed that the noirq callbacks are called with *all* interrupts disabled, but that's not the case. Only the IRQs for that specific device are disabled when its noirq callbacks run. Ah, so even with my fix of moving to noirq we could still be broken if the system decided to enable interrupts for the device before the i2c controller get resumed then we'd still be SOL. ...oh, but if it matches probe order then maybe we're guaranteed for that not to happen? We know that we will probe the i2c bus before the devices on it, right? If the mentioned device is a child of the I2C controller then the parent-child relation determines the order. Otherwise (e.g. another, non-I2C interrupt source that just triggers some operation on an I2C device like voltage regulator) we're doomed. ;) Exactly. There are lots of dragons hiding here. Runtime PM is your friend. ;) Kevin -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Hi Doug, Doug Anderson diand...@chromium.org writes: On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Yes, it appears it does. Objection withdrawn. I just wanted to be sure because since the introduction of late/early, the need for noirq should be pretty rare, but there certainly are needs. tangent In this case though, the need for it has more to do with the lack of a way for us to describe non parent-child device dependencies than whether or not IRQs are enabled or not. /tangent Kevin -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Kevin, On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman khil...@linaro.org wrote: Hi Doug, Doug Anderson diand...@chromium.org writes: On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Yes, it appears it does. Objection withdrawn. I just wanted to be sure because since the introduction of late/early, the need for noirq should be pretty rare, but there certainly are needs. tangent In this case though, the need for it has more to do with the lack of a way for us to describe non parent-child device dependencies than whether or not IRQs are enabled or not. /tangent Actually, I'm not sure that's true, but I'll talk through it and you can point to where I'm wrong (I often am!) If you're a wakeup device then you need to be ready to handle interrupts as soon as the noirq phase of resume is done, right? Said another way: you need to be ready to handle interrupts _before_ the normal resume code is called and be ready to handle interrupts even _before_ the early resume code is called. That means if you are implementing a bus that's needed by any devices with wakeup interrupts then it's your responsibility to also be prepared to run this early. In this particular case the max77686 driver doesn't need to do anything at all to be ready to handle interrupts. It's suspend and resume code is just boilerplate enable wakeups / disable wakeups and it has no noirq code. The max77686 driver doesn't have any noirq wake call because it would just be empty. Said another way: the problem isn't that the max77686 wakeup gets called before the i2c wakeup. The problem is that i2c is needed ASAP once IRQs are enabled and thus needs to be run noirq. Does that sound semi-correct? -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Doug Anderson diand...@chromium.org writes: Kevin, On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman khil...@linaro.org wrote: Hi Doug, Doug Anderson diand...@chromium.org writes: On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Yes, it appears it does. Objection withdrawn. I just wanted to be sure because since the introduction of late/early, the need for noirq should be pretty rare, but there certainly are needs. tangent In this case though, the need for it has more to do with the lack of a way for us to describe non parent-child device dependencies than whether or not IRQs are enabled or not. /tangent Actually, I'm not sure that's true, but I'll talk through it and you can point to where I'm wrong (I often am!) If you're a wakeup device then you need to be ready to handle interrupts as soon as the noirq phase of resume is done, right? As soon as the noirq phase of your own driver is done, correct. Said another way: you need to be ready to handle interrupts _before_ the normal resume code is called and be ready to handle interrupts even _before_ the early resume code is called. Correct. That means if you are implementing a bus that's needed by any devices with wakeup interrupts then it's your responsibility to also be prepared to run this early. In this particular case the max77686 driver doesn't need to do anything at all to be ready to handle interrupts. It's suspend and resume code is just boilerplate enable wakeups / disable wakeups and it has no noirq code. The max77686 driver doesn't have any noirq wake call because it would just be empty. Said another way: the problem isn't that the max77686 wakeup gets called before the i2c wakeup. The problem is that i2c is needed ASAP once IRQs are enabled and thus needs to be run noirq. Does that sound semi-correct? Yes that's correct. My point above was (trying to be) that ultimately this is an ordering issue. e.g. the bus device needs to be ready before wakeup devices on that bus can handle wakeup interrupts etc. The way we're handling that ordering is by the implied ordering of noirq, late/early and normal callbacks. That's convenient, but not exactly obvious. It works because we dont' typically need too many layers here, but it would be much more understandable if we could describe this kind of dependency in a way that the suspend/resume code would suspend/resume things in the right order rather than by tinkering with callback levels (since otherwise suspend/resume ordering just depends on probe order.) This issue then usually gets me headed down my usual rant path about how I think runtime PM is much better suited for handling ordering and dependencies becuase it automatically handles parent/child dependencies and non parent/child dependencies can be handled by taking advantage of the get/put APIs which are refcounted, ect etc. but that's another can worms. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Kevin, On Fri, Jun 20, 2014 at 4:13 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: Kevin, On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman khil...@linaro.org wrote: Hi Doug, Doug Anderson diand...@chromium.org writes: On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Yes, it appears it does. Objection withdrawn. I just wanted to be sure because since the introduction of late/early, the need for noirq should be pretty rare, but there certainly are needs. tangent In this case though, the need for it has more to do with the lack of a way for us to describe non parent-child device dependencies than whether or not IRQs are enabled or not. /tangent Actually, I'm not sure that's true, but I'll talk through it and you can point to where I'm wrong (I often am!) If you're a wakeup device then you need to be ready to handle interrupts as soon as the noirq phase of resume is done, right? As soon as the noirq phase of your own driver is done, correct. Said another way: you need to be ready to handle interrupts _before_ the normal resume code is called and be ready to handle interrupts even _before_ the early resume code is called. Correct. That means if you are implementing a bus that's needed by any devices with wakeup interrupts then it's your responsibility to also be prepared to run this early. In this particular case the max77686 driver doesn't need to do anything at all to be ready to handle interrupts. It's suspend and resume code is just boilerplate enable wakeups / disable wakeups and it has no noirq code. The max77686 driver doesn't have any noirq wake call because it would just be empty. Said another way: the problem isn't that the max77686 wakeup gets called before the i2c wakeup. The problem is that i2c is needed ASAP once IRQs are enabled and thus needs to be run noirq. Does that sound semi-correct? Yes that's correct. My point above was (trying to be) that ultimately this is an ordering issue. e.g. the bus device needs to be ready before wakeup devices on that bus can handle wakeup interrupts etc. The way we're handling that ordering is by the implied ordering of noirq, late/early and normal callbacks. That's convenient, but not exactly obvious. It works because we dont' typically need too many layers here, but it would be much more understandable if we could describe this kind of dependency in a way that the suspend/resume code would suspend/resume things in the right order rather than by tinkering with callback levels (since otherwise suspend/resume ordering just depends on probe order.) This issue then usually gets me headed down my usual rant path about how I think runtime PM is much better suited for handling ordering and dependencies becuase it automatically handles parent/child dependencies and non parent/child dependencies can be handled by taking advantage of the get/put APIs which are refcounted, ect etc. but that's another can worms. Ah, I gotcha. Yes, I'm a fan of having explicit dependency orderings too. So I guess in this case the truly correct way to handle it is: 1. i2c controller should have Runtime PM even though (as per the code now) there's nothing you can do to it to save power under normal circumstances. So the runtime suspend code would be a no-op. 2. When the i2c controller is told to runtime resume, it should double-check if a full SoC poweroff has happened since the last time it checked. In this case it should reinit its hardware. 3. If the i2c controller gets a full resume callback then it should also reinit the hardware just so it's not sitting in a half-configured state until the first peripheral uses it. If later someone finds a way to power gate the i2c controller when no active transfers are going (and we actually save non-trivial power doing this) then we've got a nice place to put that code. NOTE: Unless we can actually save power by power gating the i2c peripheral when there are no active transfers, we would also just have the i2c_xfer() init the hardware if needed. Maybe that's kinda gross, though. -Doug P.S. Just to confirm: you're not
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
On 21.06.2014 01:53, Doug Anderson wrote: Kevin, On Fri, Jun 20, 2014 at 4:13 PM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: Kevin, On Fri, Jun 20, 2014 at 2:48 PM, Kevin Hilman khil...@linaro.org wrote: Hi Doug, Doug Anderson diand...@chromium.org writes: On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Yes, it appears it does. Objection withdrawn. I just wanted to be sure because since the introduction of late/early, the need for noirq should be pretty rare, but there certainly are needs. tangent In this case though, the need for it has more to do with the lack of a way for us to describe non parent-child device dependencies than whether or not IRQs are enabled or not. /tangent Actually, I'm not sure that's true, but I'll talk through it and you can point to where I'm wrong (I often am!) If you're a wakeup device then you need to be ready to handle interrupts as soon as the noirq phase of resume is done, right? As soon as the noirq phase of your own driver is done, correct. Said another way: you need to be ready to handle interrupts _before_ the normal resume code is called and be ready to handle interrupts even _before_ the early resume code is called. Correct. That means if you are implementing a bus that's needed by any devices with wakeup interrupts then it's your responsibility to also be prepared to run this early. In this particular case the max77686 driver doesn't need to do anything at all to be ready to handle interrupts. It's suspend and resume code is just boilerplate enable wakeups / disable wakeups and it has no noirq code. The max77686 driver doesn't have any noirq wake call because it would just be empty. Said another way: the problem isn't that the max77686 wakeup gets called before the i2c wakeup. The problem is that i2c is needed ASAP once IRQs are enabled and thus needs to be run noirq. Does that sound semi-correct? Yes that's correct. My point above was (trying to be) that ultimately this is an ordering issue. e.g. the bus device needs to be ready before wakeup devices on that bus can handle wakeup interrupts etc. The way we're handling that ordering is by the implied ordering of noirq, late/early and normal callbacks. That's convenient, but not exactly obvious. It works because we dont' typically need too many layers here, but it would be much more understandable if we could describe this kind of dependency in a way that the suspend/resume code would suspend/resume things in the right order rather than by tinkering with callback levels (since otherwise suspend/resume ordering just depends on probe order.) This issue then usually gets me headed down my usual rant path about how I think runtime PM is much better suited for handling ordering and dependencies becuase it automatically handles parent/child dependencies and non parent/child dependencies can be handled by taking advantage of the get/put APIs which are refcounted, ect etc. but that's another can worms. Ah, I gotcha. Yes, I'm a fan of having explicit dependency orderings too. So I guess in this case the truly correct way to handle it is: 1. i2c controller should have Runtime PM even though (as per the code now) there's nothing you can do to it to save power under normal circumstances. So the runtime suspend code would be a no-op. 2. When the i2c controller is told to runtime resume, it should double-check if a full SoC poweroff has happened since the last time it checked. In this case it should reinit its hardware. 3. If the i2c controller gets a full resume callback then it should also reinit the hardware just so it's not sitting in a half-configured state until the first peripheral uses it. If later someone finds a way to power gate the i2c controller when no active transfers are going (and we actually save non-trivial power doing this) then we've got a nice place to put that code. NOTE: Unless we can actually save power by power gating the i2c peripheral when there are no active transfers, we would also just have the i2c_xfer() init the hardware if needed. Maybe that's
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
Kevin, On Thu, Jun 19, 2014 at 11:43 AM, Kevin Hilman khil...@linaro.org wrote: Doug Anderson diand...@chromium.org writes: The original code for the exynos i2c controller registered for the noirq variants. However during review feedback it was moved to SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no longer actually noirq (despite functions named exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq). i2c controllers that might have wakeup sources on them seem to need to resume at noirq time so that the individual drivers can actually read the i2c bus to handle their wakeup. I suspect usage of the noirq variants pre-dates the existence of the late/early callbacks in the PM core, but based on the description above, I suspect what you actually want is the late/early callbacks. I think it actually really needs noirq. ;) Specifically as soon as IRQs are enabled one of our children might get an interrupt. To respond to that interrupt it might want to do an i2c transfer. Here's a snippet from me moving it to resume early. You can see that max77686 failed pretty much right away (the dummy is just the max77686 RTC for various reasons). Note that the max77686 isn't doing anything tricky. It uses regmap_irq (as per Javier's patches) and calls enable_irq_wake() at suspend time. It uses a simple threaded IRQ: [ 117.968055] PM: noirq resume of devices complete after 1.391 msecs [ 117.968200] max77686 0-0009: Failed to read IRQ status: -5 [ 117.968697] dummy 0-0006: Failed to read IRQ status: -5 [ 117.968793] calling 12c6.i2c+ @ 5891, parent: none [ 117.968805] s3c-i2c 12c6.i2c: slave address 0x00 [ 117.968814] s3c-i2c 12c6.i2c: bus frequency set to 378 KHz [ 117.968824] call 12c6.i2c+ returned 0 after 23 usecs [ 117.968831] calling 12c7.i2c+ @ 5891, parent: none [ 117.968841] s3c-i2c 12c7.i2c: slave address 0x00 [ 117.968849] s3c-i2c 12c7.i2c: bus frequency set to 378 KHz [ 117.968858] call 12c7.i2c+ returned 0 after 20 usecs [ 117.968864] calling 12c8.i2c+ @ 5891, parent: none [ 117.968874] s3c-i2c 12c8.i2c: slave address 0x00 [ 117.968882] s3c-i2c 12c8.i2c: bus frequency set to 65 KHz [ 117.968891] call 12c8.i2c+ returned 0 after 20 usecs [ 117.968897] calling 12c9.i2c+ @ 5891, parent: none [ 117.968907] s3c-i2c 12c9.i2c: slave address 0x00 [ 117.968915] s3c-i2c 12c9.i2c: bus frequency set to 65 KHz [ 117.968923] call 12c9.i2c+ returned 0 after 19 usecs [ 117.968930] calling 12ca.i2c+ @ 5891, parent: none [ 117.968940] s3c-i2c 12ca.i2c: slave address 0x00 [ 117.968948] s3c-i2c 12ca.i2c: bus frequency set to 65 KHz [ 117.968957] call 12ca.i2c+ returned 0 after 20 usecs [ 117.968964] calling 12cb.i2c+ @ 5891, parent: none [ 117.968974] s3c-i2c 12cb.i2c: slave address 0x00 [ 117.968982] s3c-i2c 12cb.i2c: bus frequency set to 65 KHz [ 117.968990] call 12cb.i2c+ returned 0 after 20 usecs [ 117.968997] calling 12cd.i2c+ @ 5891, parent: none [ 117.969006] s3c-i2c 12cd.i2c: slave address 0x00 [ 117.969014] s3c-i2c 12cd.i2c: bus frequency set to 65 KHz [ 117.969022] call 12cd.i2c+ returned 0 after 19 usecs [ 117.969029] calling 12ce.i2c+ @ 5891, parent: none [ 117.969039] s3c-i2c 12ce.i2c: slave address 0x00 [ 117.969048] s3c-i2c 12ce.i2c: bus frequency set to 378 KHz [ 117.969056] call 12ce.i2c+ returned 0 after 20 usecs [ 117.969625] PM: early resume of devices complete after 1.469 msecs -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: exynos5: Properly use the noirq variants of suspend/resume
The original code for the exynos i2c controller registered for the
noirq variants. However during review feedback it was moved to
SIMPLE_DEV_PM_OPS without anyone noticing that it meant we were no
longer actually noirq (despite functions named
exynos5_i2c_suspend_noirq and exynos5_i2c_resume_noirq).
i2c controllers that might have wakeup sources on them seem to need to
resume at noirq time so that the individual drivers can actually read
the i2c bus to handle their wakeup.
NOTE: I took the original review feedback from Wolfram and added
poweroff, thaw, freeze, restore.
This patch has only been compile-tested since I don't have all the
patches needed to make my machine using this i2c driver actually
suspend/resume.
Signed-off-by: Doug Anderson diand...@chromium.org
---
drivers/i2c/busses/i2c-exynos5.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 63d2292..cba740c 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -789,8 +789,14 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
}
#endif
-static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, exynos5_i2c_suspend_noirq,
-exynos5_i2c_resume_noirq);
+const struct dev_pm_ops exynos5_i2c_dev_pm_ops = {
+ .suspend_noirq = exynos5_i2c_suspend_noirq,
+ .resume_noirq = exynos5_i2c_resume_noirq,
+ .freeze_noirq = exynos5_i2c_suspend_noirq,
+ .thaw_noirq = exynos5_i2c_resume_noirq,
+ .poweroff_noirq = exynos5_i2c_suspend_noirq,
+ .restore_noirq = exynos5_i2c_resume_noirq,
+};
static struct platform_driver exynos5_i2c_driver = {
.probe = exynos5_i2c_probe,
--
2.0.0.526.g5318336
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
