Re: [PATCH 04/10] pm: domains: sync runtime PM status with PM domains after probe
Russell King rmk+ker...@arm.linux.org.uk writes: Synchronise the PM domain status with runtime PM's status after a platform device has been probed. This augments the solution in commit 2ed127697eb1 (PM / Domains: Power on the PM domain right after attach completes). The above commit added a call to power up the PM domain when a device attaches to the domain in order to match the behaviour required by drivers that make no use of runtime PM. The assumption is that the device driver will cause a runtime PM transition, which will synchronise the PM domain state with the runtime PM state. However, by default, runtime PM will assume that the device is initially suspended, and some drivers may make use of this should they not need to touch the hardware during probe. In order to allow such drivers, trigger the PM domain code to check whether the PM domain can be suspended after the probe function, undoing the effect of the power-on prior to the probe. Signed-off-by: Russell King rmk+ker...@arm.linux.org.uk [ Since I hadn't seen this version yet, repeating comment from the previous version so it doesn't get lost. ] I think this is a good fix to the existing problem. One minor nit on a comment below, otherwise: Acked-by: Kevin Hilman khil...@linaro.org +/** + * dev_pm_domain_sync - synchronise the PM domain state with its devices + * @dev: device corresponding with domain + * + * Synchronise the PM domain state with the recently probed device, which + * may be in a variety of PM states. This ensures that a device which + * enables runtime PM in suspended state, and never transitions to active + * in its probe handler is properly suspended after the probe. + */ It's not the *device* tha needs to be properly suspended after the probe (since it's already/still runtime suspended), but the pm_domain that would be potentially powered down. Hence, I'd reword the last sentence slightly: This ensures that a device which enables runtime PM in suspended state, and never transitions to active in its probe handler gives an opportunity for the PM domain to be powered down after the probe. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 0/2] i2c: omap: new fixes for driver
Alexander Kochetkov al.koc...@gmail.com writes: The first patch fix i2c-omap driver for omap2420, found by code review and later reported by Tony Lindgren t...@atomide.com here[1]. Candidate for stable? The second patch unhide the reson of system lockup. The patch is rebased on branch 'i2c/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux (6e79807443cba7397cd855ed29d6faba51d4c893) Tony, could you check, what the patches fix the problem reported[1]? Kevin, could you run tests for patches on all omap boards? Done. Built v3.18-rc7 + these 2 patches using omap2plus_defconfig. Boot logs here for your amusement: http://people.linaro.org/~khilman/tmp/v3.18-rc7-2-gdf84e23f2864/arm-omap2plus_defconfig/ Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC] i2c: omap: TEST: do IP reset during probe.
Alexander Kochetkov al.koc...@gmail.com writes: NOT FOR UPSTREAM The patch checks if IP reset during probe could bring I2C bus to a free state on omap2430 - omap3530 boards. I guess, IP hold one of I2C lines in a low state. I guess, u-boot haven't sent a stop condition. The patch is rebased on branch 'i2c/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux (6e79807443cba7397cd855ed29d6faba51d4c893) Signed-off-by: Alexander Kochetkov al.koc...@gmail.com Reported-by: Kevin Hilman khil...@kernel.org Tested-by: Kevin Hilman khil...@kernel.org Built for omap2plus_defconfig and tested on all my OMAP boards. Results here: http://people.linaro.org/~khilman/tmp/next-20141126-1-g760388ee02e4/arm-omap2plus_defconfig/ -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier
Alexander Kochetkov al.koc...@gmail.com writes: 25 нояб. 2014 г., в 22:13, Kevin Hilman khil...@kernel.org написал(а): I'll test your patch on all my OMAP boards. Put whatever debug output you want, and I'll send you links to all the boot output. Hello, Kevin! I've sent the patch[1]. Could you be so kind to run it on all your OMAP boards? Thank you very much! It is not urgent at all. Done. Built for omap2plus_defconfig, boot reports for all my OMAP boards here: http://people.linaro.org/~khilman/tmp/next-20141126-1-g760388ee02e4/arm-omap2plus_defconfig/ What is the preferred way for giving patches for you (for future)? Email is fine. I have things fully automated for primary upstream trees (mainline, linux-next, stable, etc.) but for stuff like this, I can trigger one-off tests. However, if Tony wants to have a branch (besides the one already goes to linux-next) which I would add to the automation cycle, I'm willing to do that. I have one more fixes for i2c-omap (I think final). I don't want to break tests anymore. And I found, that n900 boot test PASS, but in fact it doesn't[2]. [2] http://status.armcloud.us/boot/omap3-n900/job/next/kernel/next-20141124/defconfig/arm-omap2plus_defconfig/ Right. For these boot tests, PASS means it got to a userspace shell, which it did. The kernel got some warnings etc. during boot, but it still booted up to a shell. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier
Alexander Kochetkov al.koc...@gmail.com writes: Commit 903c3859f77f9b0aace551da03267ef7a211dbc4 (i2c: omap: implement workaround for handling invalid BB-bit values) introduce the error result in boot test fault on OMAP3530 boards The patch fix the error (disable i2c bus test for OMAP3530). Signed-off-by: Alexander Kochetkov al.koc...@gmail.com Fixes: 903c3859f77f9b0aace551da03267ef7a211dbc4 Reported-by: Kevin Hilman khil...@kernel.org Tested-by: Tony Lindgren t...@atomide.com Tested-by: Kevin Hilman khil...@linaro.org I tested DT and legacy boot on 3430/n900, 3530/beagle and 3530/overo-tobi. All boot fine. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] omap: i2c: don't check bus state IP rev3.3 and earlier
Alexander Kochetkov al.koc...@gmail.com writes: 25 нояб. 2014 г., в 17:19, Wolfram Sang w...@the-dreams.de написал(а): I'll push out this evening to make the boot tests work again. If there is more to be investigated, either hurry up and post v3 ;) or let me know that you need more time. Ok, thank you. Let the fix go to the kernel-next. Maybe small fix to subject omap: i2c: to i2c: omap: I still guessing what some boards have broken i2c pull-ups. And real fix must go in the board file. http://www.spinics.net/lists/linux-i2c/msg17750.html I could create a patch to confirm this. But I don't have omap3530 boards to run. I'll be very appreciated if someone could run the patch. I'll test your patch on all my OMAP boards. Put whatever debug output you want, and I'll send you links to all the boot output. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/4] i2c: omap: implement workaround for handling invalid BB-bit values
On Sat, Nov 22, 2014 at 11:47 AM, Alexander Kochetkov al.koc...@gmail.com wrote: In a multimaster environment, after IP software reset, BB-bit value doesn't correspond to the current bus state. It may happen what BB-bit will be 0, while the bus is busy due to another I2C master activity. Any transfer started when BB=0 and bus is busy wouldn't be completed by IP and results in controller timeout. More over, in some cases IP could interrupt another master's transfer and corrupt data on wire. The commit implement method allowing to prevent IP from entering into controller timeout state and from data corruption state. The one drawback is the need to wait for 10ms before the first transfer. Tested on Beagleboard XM C. Tested on BBB and AM437x Starter Kit by Felipe Balbi. Signed-off-by: Alexander Kochetkov al.koc...@gmail.com Tested-by: Felipe Balbi ba...@ti.com Reviewed-by: Felipe Balbi ba...@ti.com This patch recently hit linux-next (as commit 903c3859f77f) and boot breakage[1] in next-20141124 on OMAP3530 Beagle and Overo/Tobi boards was bisected down to this commit. Kevin [1] http://status.armcloud.us/boot/?next-20141124omap -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND/PATCH] i2c: pxa: Use suspend() and resume() instead of the _noirq hooks
Ezequiel Garcia ezequ...@vanguardiasur.com.ar writes:
On 10/20/2014 06:07 PM, Kevin Hilman wrote:
Ezequiel Garcia ezequ...@vanguardiasur.com.ar writes:
The _noirq were previously chosen to make sure all the users of the
adapter were suspended by the time the adapter itself enters the
suspended state.
The {suspend,resume}_noirq usage was converted from an earlier
implementation based on suspend_late and resume_early on this commit:
commit 57f4d4f1b72983f8c76e2f232e064730aeffe599
Author: Magnus Damm d...@igel.co.jp
Date: Wed Jul 8 13:22:39 2009 +0200
I2C: Rework i2c-pxa suspend_late()/resume_early()
However, all the I2C devices are probed as children of its I2C adapter,
and hence the device model guarantees they are suspended before its parent,
and
resumed after it.
In other words, there's no need to use the _noirq hooks to get a
suspend/resume
device/adapter order.
Are you sure *really* about this?
Hm, now you made me doubt.
Good. :)
It's usally not the children that are the problem here. It's usually
some other driver trying to use an I2C device e.g. MMC changing voltage
using an I2C-based PMIC during its suspend process.
So we can't know for sure if using suspend/resume is going to work OK for
a I2C adapter?
There might be some use of it that can't be accurately modeled in the
device model.
Correct.
I.e., the above example is a dependency of MMC on the I2C being suspended
after him, but right now there's no way to model such relationship.
That's right. That's why many i2c drivers are using the late/early
callbacks for suspend/resume.
suspend/resume order depends on probe ordering, so with deferred probe,
the probe ordering might be right, but it should be thoroughly tested.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] i2c/at91: add support for system PM
Wenyou Yang wenyou.y...@atmel.com writes:
Add a changelog here describing what you're doing, and why.
Signed-off-by: Wenyou Yang wenyou.y...@atmel.com
---
drivers/i2c/busses/i2c-at91.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 03b9f48..8f408f8 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -845,6 +845,35 @@ static int at91_twi_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
+static int at91_twi_suspend(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ if (!pm_runtime_suspended(dev))
+ clk_disable_unprepare(twi_dev-clk);
I would just call at91_twi_runtime_suspend() here.
Then, if you need to add additional steps, you only have to add them in
once place. This also makes it obvious that -suspend and
-runtime_suspend are doing the exact same thing.
NOTE: you'll need to wrap the runtime_suspend|resume functions in just
CONFIG_PM instead of CONFIG_PM_RUNTIME for this to work.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] i2c/at91: adopt pinctrl support
Wenyou Yang wenyou.y...@atmel.com writes: Amend the i2c at91 pin controller to optionally take a pin control handle and set the state of the pins to: - default on boot, resume and before performing an transfer - sleep on suspend() This should make it possible to optimize energy usage for the pins both for the suspend/resume cycle Signed-off-by: Wenyou Yang wenyou.y...@atmel.com This patch is a good example of why you should just have the -suspend function call the same function as -runtime_suspend. If you do that, rather than having to add the pinctrl_pm* calls bo both system PM and runtime PM functions, you could've just added them to the runtime PM functions. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: [...] 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-i2c 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
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-i2c 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-i2c 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-i2c 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-i2c 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: 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-i2c 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: mv64xxx: Fix locked bus when offload is selected but not used on a message
Jason Cooper ja...@lakedaemon.net writes: On Fri, Feb 07, 2014 at 11:55:54AM +0100, Gregory CLEMENT wrote: Offload can be used only on regular transactions and for 1 to byte transfers. In the other cases we switch back to usual work flow. In this case we need to call mv64xxx_i2c_prepare_for_io() as this function is not used when we try to use offloading. This commit adds this missing call when offloading have failed in the MV64XXX_I2C_ACTION_OFFLOAD_SEND_START case. This fix the timeout seen when the the i2c driver try to access an address where the device is absent on the Armada XP bases board. Cc: sta...@vger.kernel.org # v3.12+ Fixes: 930ab3d403ae (i2c: mv64xxx: Add I2C Transaction Generator support) Signed-off-by: Gregory CLEMENT gregory.clem...@free-electrons.com --- drivers/i2c/busses/i2c-mv64xxx.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) I'd like to get a few tested-by's on this before this is pushed. We've had quite a bit of fixes this round :( Please test both multi_v7 and mvebu defconfigs. Kevin, I know you're busy with a lot more than us, but if you could confirm that this fixes the bus hangs you reported, that would be great. I applied this patch on top of next-20140207 and tested this on the armada-xp openblocks ax3, which is where I was seeing the I2C timeouts. I confirm it fixes the timeouts I was seeing. Tested-by: Kevin Hilman khil...@linaro.org Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/17] mmc: mmci: Put the device into low power state at system suspend
Linus Walleij linus.wall...@linaro.org writes: On Tue, Feb 4, 2014 at 8:22 PM, Kevin Hilman khil...@linaro.org wrote: Ulf Hansson ulf.hans...@linaro.org writes: Due to the available runtime PM callbacks, we are now able to put our device into low power state at system suspend. (...) I'm trying to thing of a good reason to not make PM_SLEEP depend on PM_RUNTIME for platforms like this. Isn't the typical Android platform using PM_SLEEP without using PM_RUNTIME? No, most Android platforms that I'm aware of use both extensively. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/17] mmc: mmci: Put the device into low power state at system suspend
Ulf Hansson ulf.hans...@linaro.org writes:
Due to the available runtime PM callbacks, we are now able to put our
device into low power state at system suspend.
Earlier we could not accomplish this without trusting a power domain
for the device to take care of it. Now we are able to cope with
scenarios both with and without a power domain.
Cc: Russell King li...@arm.linux.org.uk
Signed-off-by: Ulf Hansson ulf.hans...@linaro.org
---
drivers/mmc/host/mmci.c | 45 +
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index c88da1c..074e0cb 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -1723,33 +1723,38 @@ static int mmci_remove(struct amba_device *dev)
return 0;
}
-#ifdef CONFIG_SUSPEND
-static int mmci_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+static int mmci_suspend_late(struct device *dev)
{
- struct amba_device *adev = to_amba_device(dev);
- struct mmc_host *mmc = amba_get_drvdata(adev);
+ int ret = 0;
- if (mmc) {
- struct mmci_host *host = mmc_priv(mmc);
- pm_runtime_get_sync(dev);
- writel(0, host-base + MMCIMASK0);
- }
+ if (pm_runtime_status_suspended(dev))
+ return 0;
- return 0;
+ if (dev-pm_domain dev-pm_domain-ops.runtime_suspend)
+ ret = dev-pm_domain-ops.runtime_suspend(dev);
+ else
+ ret = dev-bus-pm-runtime_suspend(dev);
+
+ if (!ret)
+ pm_runtime_set_suspended(dev);
Isn't this basically open-coding pm_runtime_suspend()...
+ return ret;
}
-static int mmci_resume(struct device *dev)
+static int mmci_resume_early(struct device *dev)
{
- struct amba_device *adev = to_amba_device(dev);
- struct mmc_host *mmc = amba_get_drvdata(adev);
+ int ret = 0;
- if (mmc) {
- struct mmci_host *host = mmc_priv(mmc);
- writel(MCI_IRQENABLE, host-base + MMCIMASK0);
- pm_runtime_put(dev);
- }
+ if (pm_runtime_status_suspended(dev))
+ return 0;
- return 0;
+ if (dev-pm_domain dev-pm_domain-ops.runtime_resume)
+ ret = dev-pm_domain-ops.runtime_resume(dev);
+ else
+ ret = dev-bus-pm-runtime_resume(dev);
+
+ return ret;
...and this is pm_runtime_resume()? (though both terribly simplified.)
This is starting to show that building with PM_SLEEP but not PM_RUNTIME
is going to force open-coding a lot of stuff that the runtime PM
framework already provides. So either we need some helper functions so
we're not sprinkling manual calls to bus/pm_domain callbacks all over
the place, or maybe where we need to go is have a way for platforms that
really are runtime PM centric to declare that even PM_SLEEP depends on
PM_RUNTIME.
I'm trying to thing of a good reason to not make PM_SLEEP depend on
PM_RUNTIME for platforms like this.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg mika.westerb...@linux.intel.com writes:
On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index f32ca29..44374b4 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
client-flags I2C_CLIENT_WAKE);
dev_dbg(dev, probe\n);
+ /* Make sure the adapter is active */
+ pm_runtime_get_sync(client-adapter-dev);
+
+ /*
+ * Enable runtime PM for the client device. If the client wants to
+ * participate on runtime PM it should call pm_runtime_put() in its
+ * probe() callback.
+ */
+ pm_runtime_get_noresume(client-dev);
+ pm_runtime_set_active(client-dev);
Why the set_active here?
For hardware that is disabled/powered-off on startup, there will now be
a mismatch between the hardware state an the RPM core state.
The call to pm_runtime_get_noresume() should make sure that the device is
in active state (at least in state where it can access the bus) if I'm
understanding this right.
No, after _get_noresume(), nothing happens to the hardware. It simply
increments the usecount. From pm_runtime.h:
static inline void pm_runtime_get_noresume(struct device *dev)
{
atomic_inc(dev-power.usage_count);
}
So after the _get_noresume() and _set_active() you're very likely to
have a disconnect between the hardware state and what state RPM thinks
the hardware is in.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg mika.westerb...@linux.intel.com writes:
On Fri, Sep 13, 2013 at 05:50:22PM +0300, Mika Westerberg wrote:
On Fri, Sep 13, 2013 at 07:30:55AM -0700, Kevin Hilman wrote:
Mika Westerberg mika.westerb...@linux.intel.com writes:
On Thu, Sep 12, 2013 at 02:34:21PM -0700, Kevin Hilman wrote:
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index f32ca29..44374b4 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev)
client-flags
I2C_CLIENT_WAKE);
dev_dbg(dev, probe\n);
+ /* Make sure the adapter is active */
+ pm_runtime_get_sync(client-adapter-dev);
+
+ /*
+ * Enable runtime PM for the client device. If the client wants
to
+ * participate on runtime PM it should call pm_runtime_put() in
its
+ * probe() callback.
+ */
+ pm_runtime_get_noresume(client-dev);
+ pm_runtime_set_active(client-dev);
Why the set_active here?
For hardware that is disabled/powered-off on startup, there will now be
a mismatch between the hardware state an the RPM core state.
The call to pm_runtime_get_noresume() should make sure that the device is
in active state (at least in state where it can access the bus) if I'm
understanding this right.
No, after _get_noresume(), nothing happens to the hardware. It simply
increments the usecount. From pm_runtime.h:
static inline void pm_runtime_get_noresume(struct device *dev)
{
atomic_inc(dev-power.usage_count);
}
So after the _get_noresume() and _set_active() you're very likely to
have a disconnect between the hardware state and what state RPM thinks
the hardware is in.
Good point.
I suppose calling pm_runtime_get() here would work (and make the state
active in all case)? I used _noresume() here because at this point the
driver itself hasn't had change to install it's RPM hooks.
I take that back.
[ It has been some time since this code was originally written so I can't
remember all the details :-/ ]
The pm_runtime_get_noresume() is there just to increment the refcount. It
has nothing to do to activate the device in question. Sorry about the
confusion from my part.
This is what happens in case the device is enumerated from ACPI:
// This makes sure that the controller itself is powered on
// (adapter device follows its parent which is the controller). The
// controller is attached to the ACPI power domain so it is
// brought to D0 now.
pm_runtime_get_sync(client-adapter-dev);
// This binds the client device to the ACPI power domain, and in
// addition to that brings the client device to D0.
OK, then here is where the problem is, because you're building ACPI
assumptions into the core. For non-ACPI devices, this part is a nop, so
the client device is still powered off, which breaks the assumptions
below.
if (ACPI_HANDLE(client-dev))
acpi_dev_pm_attach(client-dev, true);
// Increase the refcount so that client can start runtime PM
// transitions when it calls _put().
pm_runtime_get_noresume(client-dev);
// Mark the device being active as
// 1) In ACPI case we know that is true as we just powered the
// device on.
// 2) We treat the device by default to be runtime PM active and
// powered on (that's in the changelog and should follow what
// the PCI bus did).
pm_runtime_set_active(client-dev);
// Enable runtime PM but nothing happens yet as long as the client
// driver doesn't call _put().
pm_runtime_enable(client-dev);
So, yes there might be a disconnect between the runtime PM state and the
device HW state now (same is with default to suspended).
Yes, but until now, default to suspended has been assumed, so any
changes to that will likely require more thorough auditing of other drivers.
Kevin
When the driver -probe() is called, it needs to power on the device (which
it probably needs to do anyway to be able to talk to the device and probe
its registers, etc.). Once the driver calls _put() the device is eventually
moved to suspended state (and its RPM hooks are called). I might be missing
something but is there a case where this is not beneficial?
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND 1/2] i2c: prepare runtime PM support for I2C client devices
Aaron Lu aaron...@intel.com writes: On 09/11/2013 06:32 AM, Rafael J. Wysocki wrote: On Tuesday, September 10, 2013 10:35:22 PM Mark Brown wrote: On Tue, Sep 10, 2013 at 10:04:21PM +0200, Rafael J. Wysocki wrote: On Tuesday, September 10, 2013 05:13:21 PM Mark Brown wrote: OK, that is very much not the model which embedded systems follow, in embedded systems the driver for the device is fully in control of its own power. It gets resources like GPIOs and regulators which allow it to make fine grained decisions. There are platforms where those resources are simply not available for direct manipulation and we need to use ACPI methods for power management. Now, since those methods are used in pretty much the same way for all I2C devices, we add a PM domain for that to avoid duplicating the same code in all of the drivers in question (patch [2/2]). Does that make sense to you? It doesn't seem like a particular problem, but the existing usage does need to be preserved for the systems that use it so things like having auto as the default and updating the drivers seem like they're needed. If we're starting to get a reasonable number of buses following the same pattern it seems like we're in a position to start We need that for exactly 3 buses: platform (already done), I2C and SPI. No other bus types are going to use ACPI this way for PM, at least for the time being, simply because PCI, USB and SATA/IDE have their own ways of doing this (which are bus-specific) and the spec doesn't cover other bus types directly (it defines support for UART, but we don't have a UART bus type). Moreover, because PCI and USB use ACPI for PM in their own ways, moving that thing up to the driver core would be rather inconvenient. That only applies to the power domains though, what Mika was saying was that the process for enabling runtime PM (just drop a reference) also becomes easier with this method regardless of anything else - that makes sense to me as something we might want to end up with so do we want to just move towards making that a default? Possibly. :-) At least I don't see any fundamental obstacles ... Looks like, it all boils down to how many I2C devices should be allowed for runtime PM by default and how many I2C devices should be forbidden. , and then we allow/forbid runtime PM for the majority case in I2C core while individual driver can still forbid/allow it in their own code. So if the majority case is runtime PM should be allowed by default, I'm also OK to not forbid runtime PM for I2C client device in I2C core. My original intention to forbid runtime PM by default is to make sure no adverse effect would occur to some I2C devices that used to work well before runtime PM. IMO, this decision belongs to the PM domain, not to the core. We have an established legacy with the current core default (auto) and changing that means lots of breakage. The forbid by default can just as easily be handled in the PM domain for the group of devices that need it, so why not do it there? Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
Mika Westerberg mika.westerb...@linux.intel.com writes: From: Aaron Lu aaron...@intel.com This patch adds runtime PM support for the I2C bus in a similar way that has been done for PCI bus already. This means that the I2C bus core prepares runtime PM for a client device just before a driver is about to be bound to it. Devices that are not bound to any driver are not prepared for runtime PM. In order to take advantage of this runtime PM support, the client device driver needs drop the device runtime PM reference count by calling pm_runtime_put() in its -probe() callback and possibly implement rest of the runtime PM callbacks. If the driver doesn't support runtime PM (like most of the existing I2C client drivers), the device in question is regarded as being runtime PM active and powered on. But for existing drivers which already support runtime PM (at least 7 by a quick grep), they will be stuck runtime enabled and stop hitting low-power states after this patch. The patch adds also runtime PM support for the adapter device because it is needed to be able to runtime power manage the I2C controller device. The adapter device is handled along with the I2C controller device (it uses pm_runtime_no_callbacks()). Signed-off-by: Aaron Lu aaron...@intel.com Signed-off-by: Mika Westerberg mika.westerb...@linux.intel.com --- drivers/i2c/i2c-core.c | 44 +++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c index f32ca29..44374b4 100644 --- a/drivers/i2c/i2c-core.c +++ b/drivers/i2c/i2c-core.c @@ -248,11 +248,30 @@ static int i2c_device_probe(struct device *dev) client-flags I2C_CLIENT_WAKE); dev_dbg(dev, probe\n); + /* Make sure the adapter is active */ + pm_runtime_get_sync(client-adapter-dev); + + /* + * Enable runtime PM for the client device. If the client wants to + * participate on runtime PM it should call pm_runtime_put() in its + * probe() callback. + */ + pm_runtime_get_noresume(client-dev); + pm_runtime_set_active(client-dev); Why the set_active here? For hardware that is disabled/powered-off on startup, there will now be a mismatch between the hardware state an the RPM core state. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/9] i2c: prepare runtime PM support for I2C client devices
On Thu, Sep 12, 2013 at 2:34 PM, Kevin Hilman khil...@linaro.org wrote: Mika Westerberg mika.westerb...@linux.intel.com writes: From: Aaron Lu aaron...@intel.com This patch adds runtime PM support for the I2C bus in a similar way that has been done for PCI bus already. This means that the I2C bus core prepares runtime PM for a client device just before a driver is about to be bound to it. Devices that are not bound to any driver are not prepared for runtime PM. In order to take advantage of this runtime PM support, the client device driver needs drop the device runtime PM reference count by calling pm_runtime_put() in its -probe() callback and possibly implement rest of the runtime PM callbacks. If the driver doesn't support runtime PM (like most of the existing I2C client drivers), the device in question is regarded as being runtime PM active and powered on. But for existing drivers which already support runtime PM (at least 7 by a quick grep), they will be stuck runtime enabled and stop hitting low-power states after this patch. Oops, nevermind. I was mixing this up with runtime PM on the i2c adapter but this is for the i2c client. Sorry for the noise. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c: iop3xxx: fix build failure after waitqueue changes
There has long been a syntax problem in iop3xx_i2c_wait_event() which
has been somehow hidden by the macros in linux/wait.h. After some
recent cleanup/rework of the wait_event_* helpers, the bug has come
out from hiding and now results in build failure:
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c: In function
'iop3xx_i2c_wait_event':
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:143: error: expected ')'
before ';' token
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:157: error: expected ')'
before ';' token
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:213: error: expected ')'
before ';' token
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:291: warning: ISO C90
forbids mixed declarations and code [-Wdeclaration-after-statement]
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:551: error: expected ')'
before ';' token
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:565: error: expected ')'
before ';' token
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:764: error: expected ')'
before ';' token
/work/kernel/next/drivers/i2c/busses/i2c-iop3xx.c:176:778: error: expected ')' b
Fix by removing stray ';'
Signed-off-by: Kevin Hilman khil...@linaro.org
---
Discovered in new build failure of iop32xx_defconfig with next-20130628
drivers/i2c/busses/i2c-iop3xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-iop3xx.c b/drivers/i2c/busses/i2c-iop3xx.c
index bc99333..dd24aa0 100644
--- a/drivers/i2c/busses/i2c-iop3xx.c
+++ b/drivers/i2c/busses/i2c-iop3xx.c
@@ -176,7 +176,7 @@ iop3xx_i2c_wait_event(struct i2c_algo_iop3xx_data
*iop3xx_adap,
interrupted = wait_event_interruptible_timeout (
iop3xx_adap-waitq,
(done = compare( sr =
iop3xx_i2c_get_srstat(iop3xx_adap) ,flags )),
- 1 * HZ;
+ 1 * HZ
);
if ((rc = iop3xx_i2c_error(sr)) 0) {
*status = sr;
--
1.8.3
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
Felipe Balbi ba...@ti.com writes: [...] If you have 200 pm_runtime_get() followed by 200 pm_runtime_put() (put is called only after 200 gets, no put-get ping-pong), your -runtime_resume() gets called once, your -runtime_suspend() gets called once but your -runtime_idle() will get called 200 times. No. The driver's -runtime_idle() will only be called when the usecount goes to zero. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
Grygorii Strashko grygorii.stras...@ti.com writes: From: Kevin Hilman khil...@deeprootsystems.com Currently, runtime PM is used to keep the device enabled only during active transfers and for a configurable runtime PM autosuspend timout after an xfer. In addition to idling the device, driver's -runtime_suspend() method currently disables device interrupts when idle. However, on some SoCs (notably OMAP4+), the I2C hardware may shared with other coprocessors. This means that the MPU will still recieve interrupts if a coprocessor is using the I2C device. To avoid this, also disable interrupts at the MPU INTC when idling the device in -runtime_suspend() (and re-enable them in -runtime_resume().) This part based on an original patch from Shubhrajyoti Datta. NOTE: for proper sharing the I2C with a coprocessor, this driver still needs hwspinlock support added. More over, this patch fixes the kernel boot failure which happens when CONFIG_SENSORS_LM75=y: [2.251220] WARNING: at drivers/bus/omap_l3_noc.c:113 l3_interrupt_handler+0x140/0x184() [2.257781] L3 custom error: MASTER:MPU TARGET:L4 PER2 [2.264373] Modules linked in: [2.268463] CPU: 0 PID: 2 Comm: kthreadd Not tainted 3.10.0-rc4-00034-g042dd60-dirty #64 [2.270385] [c001a80c] (unwind_backtrace+0x0/0xf0) from [c0017238] (show_stack+0x10/0x14) [2.286102] [c0017238] (show_stack+0x10/0x14) from [c0040fd0] (warn_slowpath_common+0x4c/0x68) [2.295593] [c0040fd0] (warn_slowpath_common+0x4c/0x68) from [c0041080] (warn_slowpath_fmt+0x30/0x40) [2.299987] [c0041080] (warn_slowpath_fmt+0x30/0x40) from [c02c5ed0] (l3_interrupt_handler+0x140/0x184) [2.315582] [c02c5ed0] (l3_interrupt_handler+0x140/0x184) from [c009ffb8] (handle_irq_event_percpu+0x58/0x258) [2.323364] [c009ffb8] (handle_irq_event_percpu+0x58/0x258) from [c00a01f4] (handle_irq_event+0x3c/0x5c) [2.327819] [c00a01f4] (handle_irq_event+0x3c/0x5c) from [c00a2f7c] (handle_fasteoi_irq+0xbc/0x164) [2.337829] [c00a2f7c] (handle_fasteoi_irq+0xbc/0x164) from [c009f978] (generic_handle_irq+0x20/0x30) [2.357513] [c009f978] (generic_handle_irq+0x20/0x30) from [c0014168] (handle_IRQ+0x4c/0xac) [2.366821] [c0014168] (handle_IRQ+0x4c/0xac) from [c00085b4] (gic_handle_irq+0x28/0x5c) [2.375762] [c00085b4] (gic_handle_irq+0x28/0x5c) from [c04f08a4] (__irq_svc+0x44/0x5c) [2.379821] Exception stack(0xdb085ec0 to 0xdb085f08) [2.389953] 5ec0: 0001 0001 db07ea00 4113 db2317a8 db084000 02f1 [2.389953] 5ee0: db07ea00 c04fd990 db085f08 c009170c c04f03e8 [2.405609] 5f00: 2113 [2.408355] [c04f08a4] (__irq_svc+0x44/0x5c) from [c04f03e8] (_raw_spin_unlock_irqrestore+0x34/0x44) [2.418304] [c04f03e8] (_raw_spin_unlock_irqrestore+0x34/0x44) from [c00403c0] (do_fork+0xa4/0x2d4) [2.427795] [c00403c0] (do_fork+0xa4/0x2d4) from [c0040620] (kernel_thread+0x30/0x38) [2.437805] [c0040620] (kernel_thread+0x30/0x38) from [c0063888] (kthreadd+0xd4/0x13c) [2.448364] [c0063888] (kthreadd+0xd4/0x13c) from [c0013308] (ret_from_fork+0x14/0x2c) [2.448364] ---[ end trace da8cf92c433d1616 ]--- The root casue of crash is race between omap_i2c_runtime_suspend() and omap_i2c_isr_thread() If there's a race here, then it's not the same problem which is described above, unless the CPU2 IRQ is happening because of a shared user of I2C, in which case it doesn't need any additional explanation. CPU1: CPU2: |-omap_i2c_xfer | |- pm_runtime_put_autosuspend() | |-timeout |-omap_i2c_isr() |-return IRQ_WAKE_THREAD; |-omap_i2c_runtime_suspend() | |-omap_i2c_isr_thread() |- oops is here - I2C module is in idle state If this is happening, I don't think it's related to $SUBJECT patch (but is probably hidden by it.) Instead, what probably needs to happen in this is case is that omap_i2c_isr() should be doing a mark last busy to reset the autosuspend timeout. And, that should be done in a separate patch. Cc: Nishanth Menon n...@ti.com Signed-off-by: Kevin Hilman khil...@linaro.org Both the From: and Signed-off should be from my TI address since the work was done while I was working for TI. Also, if you change the original patch/changelog, you should add a line here like: [grygorii.stras...@ti.com]: updated changlog Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: omap: convert to module_platform_driver()
Grygorii Strashko grygorii.stras...@ti.com writes:
Hi Kevin,
On 06/03/2013 11:59 PM, Kevin Hilman wrote:
Grygorii Strashko grygorii.stras...@ti.com writes:
The OMAP I2C driver has a relation to pinctrl-single driver. As result,
its probe will be deferred during system boot until late init time,
because the pinctrl-single is initizalized as moudle/device init time.
This, in turn, will delay initialization of all I2C devices (like mfd,
I2C regulators and etc.) and cause boot delay (more over, it can broken
initialization of drivers which are not ready to use deferred probe
mechanism yet, for example DSS).
There are no sense to keep OMAP I2C initialization on subsys init layer
any more, hence shift it to module/device layer where the i2c --
pinctrl-single dependency is resolved in drivers/Makefile now.
Cc: Wolfram Sang w...@the-dreams.de
Cc: Ben Dooks (embedded platforms) ben-li...@fluff.org
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: linux-o...@vger.kernel.org
Cc: linux-i2c@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com
Testing this patch with PATCH 1/2, the twl_rtc driver fails to correctly
initialize on OMAP3:
twl_rtc rtc.22: hctosys: invalid date/time
instead of the expected result:
twl_rtc rtc.22: setting system clock to 2000-01-01 00:00:00 UTC
(946684800)
so something is still not right for the init sequence.
Kevin
I think, the root cause of the problem isn't this patch - it's just
yet another side effect
of using deferred probes :). I've taken a look on twl-rtc code and
found possible error place
static int __init twl_rtc_init(void)
{
if (twl_class_is_4030()) -- here,
rtc_reg_map = (u8 *) twl4030_rtc_reg_map;
else
rtc_reg_map = (u8 *) twl6030_rtc_reg_map;
return platform_driver_register(twl4030rtc_driver);
}
In drivers/Makefile:
obj-$(CONFIG_RTC_LIB)+= rtc/ -- RTC is placed before I2C
obj-y+= i2c/ media/ - and only here I2C bus
instantiates TWL-core device
and configures twl_priv-twl_id
Thats why it's working on my OMAP4/twl6030 board. Could you check if
below fix will work on OMAP3:
Yes, your fix works. Thanks for digging into it.
Care to send a proper patch? Please be sure to send to Andrew Morton
also, since he'll be the one to queue the RTC patch.
diff --git a/drivers/rtc/rtc-twl.c b/drivers/rtc/rtc-twl.c
index 8751a52..aaa5015 100644
--- a/drivers/rtc/rtc-twl.c
+++ b/drivers/rtc/rtc-twl.c
@@ -469,6 +469,11 @@ static int twl_rtc_probe(struct platform_device *pdev)
if (irq = 0)
goto out1;
+ if (twl_class_is_4030())
+ rtc_reg_map = (u8 *) twl4030_rtc_reg_map;
+ else
+ rtc_reg_map = (u8 *) twl6030_rtc_reg_map;
+
ret = twl_rtc_read_u8(rd_reg, REG_RTC_STATUS_REG);
if (ret 0)
goto out1;
@@ -610,11 +615,6 @@ static struct platform_driver twl4030rtc_driver = {
static int __init twl_rtc_init(void)
{
- if (twl_class_is_4030())
- rtc_reg_map = (u8 *) twl4030_rtc_reg_map;
- else
- rtc_reg_map = (u8 *) twl6030_rtc_reg_map;
-
return platform_driver_register(twl4030rtc_driver);
}
module_init(twl_rtc_init);
Unfortunately, I have no OMAP3 HW and can't check it.
I suggest you find a Beagle or Gumstix/Overo type board someplace.
There is an abundance of cheap OMAP3 hardware available.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 09/42] drivers/i2c/busses: don't check resource with devm_ioremap_resource
Wolfram Sang w...@the-dreams.de writes: devm_ioremap_resource does sanity checks on the given resource. No need to duplicate this in the driver. Signed-off-by: Wolfram Sang w...@the-dreams.de Acked-by: Kevin Hilman khil...@linaro.org # for i2c-omap.c -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: omap: convert to module_platform_driver()
Grygorii Strashko grygorii.stras...@ti.com writes: The OMAP I2C driver has a relation to pinctrl-single driver. As result, its probe will be deferred during system boot until late init time, because the pinctrl-single is initizalized as moudle/device init time. This, in turn, will delay initialization of all I2C devices (like mfd, I2C regulators and etc.) and cause boot delay (more over, it can broken initialization of drivers which are not ready to use deferred probe mechanism yet, for example DSS). There are no sense to keep OMAP I2C initialization on subsys init layer any more, hence shift it to module/device layer where the i2c -- pinctrl-single dependency is resolved in drivers/Makefile now. Cc: Wolfram Sang w...@the-dreams.de Cc: Ben Dooks (embedded platforms) ben-li...@fluff.org Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: linux-o...@vger.kernel.org Cc: linux-i2c@vger.kernel.org Cc: linux-ker...@vger.kernel.org Signed-off-by: Grygorii Strashko grygorii.stras...@ti.com Testing this patch with PATCH 1/2, the twl_rtc driver fails to correctly initialize on OMAP3: twl_rtc rtc.22: hctosys: invalid date/time instead of the expected result: twl_rtc rtc.22: setting system clock to 2000-01-01 00:00:00 UTC (946684800) so something is still not right for the init sequence. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] i2c: omap: correct usage of the interrupt enable register
Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com writes:
On 05/30/2013 07:46 PM, Kevin Hilman wrote:
Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com writes:
If the i2c controller during suspend will generate an interrupt, it
can lead to unpredictable behaviour in the kernel.
Based on the logic of the kernel code interrupts from i2c should be
prohibited during suspend. Kernel writes 0 to the I2C_IE register in
the omap_i2c_runtime_suspend() function. In the other side kernel
writes saved interrupt flags to the I2C_IE register in
omap_i2c_runtime_resume() function. I.e. interrupts should be disabled
during suspend.
This works for chips with version1 registers scheme. Interrupts are
disabled during suspend. For chips with version2 scheme registers
writting 0 to the I2C_IE register does nothing (because now the
I2C_IRQENABLE_SET register is located at this address). This register
is used to enable interrupts. For disabling interrupts
I2C_IRQENABLE_CLR register should be used.
Because the registers I2C_IRQENABLE_SET and I2C_IE have the same
addresses, the interrupt enabling procedure is unchanged.
Signed-off-by: Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com
Much better, but still doesn't explain how/why this has been working up
until now. Have we just been lucky?
Yes, this has been working up until now because we've just been lucky.
What I'm trying to say is you need to describe that in the changelog,
with more details. e.g. we've been lucky not to have any interrupts
fire during the suspend path, otherwise we would have
Kevin
---
drivers/i2c/busses/i2c-omap.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e02f9e3..2419899 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -180,6 +180,8 @@ enum {
#define I2C_OMAP_ERRATA_I207 (1 0)
#define I2C_OMAP_ERRATA_I462 (1 1)
+#define OMAP_I2C_INTERRUPTS_MASK 0x6FFF
To be more clear, this should probably have v2 in the name.
I'll rename this mask in the patch-set v3
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/11] i2c: omap: enhance pinctrl support
Hebbar Gururaja gururaja.heb...@ti.com writes: Amend the I2C omap pin controller to optionally take a pin control handle and set the state of the pins to: - default on boot, resume and before performing an i2c transfer - idle after initial default, after resume default, and after each i2c xfer - sleep on suspend() By optionally putting the pins into sleep state in the suspend callback we can accomplish two things. - One is to minimize current leakage from pins and thus save power, - second, we can prevent the IP from driving pins output in an uncontrolled manner, which may happen if the power domain drops the domain regulator. Note: A .suspend .resume callback is added which simply puts the pins to sleep state upon suspend are moved to default idle state upon resume. If any of the above pin states are missing in dt, a warning message about the missing state is displayed. If certain pin-states are not available, to remove this warning message pass respective state name with null phandler. (Changes based on i2c-nomadik.c) Signed-off-by: Hebbar Gururaja gururaja.heb...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Wolfram Sang w...@the-dreams.de Cc: linux-o...@vger.kernel.org Cc: linux-i2c@vger.kernel.org [...] @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) out: pm_runtime_mark_last_busy(dev-dev); + pm_runtime_put_autosuspend(dev-dev); + /* Optionally let pins go into idle state */ + if (!IS_ERR(dev-pins_idle)) + if (pinctrl_select_state(dev-pinctrl, dev-pins_idle)) + dev_err(dev-dev, could not set pins to idle state\n); This is wrong. You're changing the muxing before the device actually goes idle. Anything you want to happen when the device actually idles for real has to be in runtime PM callbacks. Looking below, I see it's already in the callbacks, so why is it here also? [...] @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev) omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); } + if (!IS_ERR(_dev-pins_idle)) + if (pinctrl_select_state(_dev-pinctrl, _dev-pins_idle)) + dev_err(dev, could not set pins to idle state\n); + return 0; } Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/1] i2c: omap: correct usage of the interrupt enable register
Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com writes: I've just removed unnecessary Change-Id line. If the i2c controller during suspend will generate an interrupt, it can lead to unpredictable behaviour in the kernel. Based on the logic of the kernel code interrupts from i2c should be prohibited during suspend. Kernel writes 0 to the I2C_IE register in the omap_i2c_runtime_suspend() function. In the other side kernel writes saved interrupt flags to the I2C_IE register in omap_i2c_runtime_resume() function. I.e. interrupts should be disabled during suspend. This works for chips with version1 registers scheme. Interrupts are disabled during suspend. For chips with version2 scheme registers writting 0 to the I2C_IE register does nothing (because now the I2C_IRQENABLE_SET register is located at this address). This register is used to enable interrupts. For disabling interrupts I2C_IRQENABLE_CLR register should be used. Because the registers I2C_IRQENABLE_SET and I2C_IE have the same addresses, the interrupt enabling procedure is unchanged. I've checked that interrupts in the i2c controller are still enabled after writting 0 to the I2C_IE register. But with my patch interrupts are disabled in the omap_i2c_runtime_suspend() function. This has been working up until now because we've just been lucky. This belongs in the changelog of the patch, not the cover letter and needs more detail. Also, you don't need a cover letter on a 1-patch series. Kevin Next patch fixes it. Patch is based on: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git tag: v3.10-rc2 Verified on OMAP4430. Oleksandr Dmytryshyn (1): i2c: omap: correct usage of the interrupt enable register drivers/i2c/busses/i2c-omap.c | 15 +++ 1 file changed, 11 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] i2c: omap: correct usage of the interrupt enable register
Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com writes: On 05/29/2013 08:22 PM, Kevin Hilman wrote: Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com writes: Starting from the OMAP chips with version2 registers scheme there are 2 registers (I2C_IRQENABLE_SET and I2C_IRQENABLE_CLR) to manage interrupts instead of the older OMAP chips with old scheme which have only one register (I2C_IE). Now we should use I2C_IRQENABLE_SET register for enabling interrupts and I2C_IRQENABLE_CLR register for disabling interrupts. Why? (changelogs should always answer the why question) IOW, what is broken without this change, how does it fail? And equally important, how is it currently working? Kevin Hi, Kevin. If the i2c controller during suspend will generate an interrupt, it can lead to unpredictable behaviour in the kernel. Based on the logic of the kernel code interrupts from i2c should be prohibited during suspend. Kernel writes 0 to the I2C_IE register in the omap_i2c_runtime_suspend() function. In the other side kernel writes saved interrupt flags to the I2C_IE register in omap_i2c_runtime_resume() function. I.e. interrupts should be disabled during suspend. This works for chips with version1 registers scheme. Interrupts are disabled during suspend. For chips with version2 scheme registers writting 0 to the I2C_IE register does nothing (because now the I2C_IRQENABLE_SET register is located at this address ). This register is used to enable interrupts. For disabling interrupts I2C_IRQENABLE_CLR register should be used. I've checked that interrupts in the i2c controller are still enabled after writting 0 to the I2C_IE register. But with my patch interrupts are disabled in the omap_i2c_runtime_suspend() function. Yes, I understand why your patch works, and it looks correct to me. My main concern is that the changelog is missing a detailed description of the problem that is being solved, as well as a summary of why this has ever worked. I guess we've just been lucky and not seen interrupts during suspend? Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/1] i2c: omap: correct usage of the interrupt enable register
Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com writes:
If the i2c controller during suspend will generate an interrupt, it
can lead to unpredictable behaviour in the kernel.
Based on the logic of the kernel code interrupts from i2c should be
prohibited during suspend. Kernel writes 0 to the I2C_IE register in
the omap_i2c_runtime_suspend() function. In the other side kernel
writes saved interrupt flags to the I2C_IE register in
omap_i2c_runtime_resume() function. I.e. interrupts should be disabled
during suspend.
This works for chips with version1 registers scheme. Interrupts are
disabled during suspend. For chips with version2 scheme registers
writting 0 to the I2C_IE register does nothing (because now the
I2C_IRQENABLE_SET register is located at this address). This register
is used to enable interrupts. For disabling interrupts
I2C_IRQENABLE_CLR register should be used.
Because the registers I2C_IRQENABLE_SET and I2C_IE have the same
addresses, the interrupt enabling procedure is unchanged.
Signed-off-by: Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com
Much better, but still doesn't explain how/why this has been working up
until now. Have we just been lucky?
---
drivers/i2c/busses/i2c-omap.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e02f9e3..2419899 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -180,6 +180,8 @@ enum {
#define I2C_OMAP_ERRATA_I207 (1 0)
#define I2C_OMAP_ERRATA_I462 (1 1)
+#define OMAP_I2C_INTERRUPTS_MASK 0x6FFF
To be more clear, this should probably have v2 in the name.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] i2c: omap: correct usage of the interrupt enable register
Oleksandr Dmytryshyn oleksandr.dmytrys...@ti.com writes: Starting from the OMAP chips with version2 registers scheme there are 2 registers (I2C_IRQENABLE_SET and I2C_IRQENABLE_CLR) to manage interrupts instead of the older OMAP chips with old scheme which have only one register (I2C_IE). Now we should use I2C_IRQENABLE_SET register for enabling interrupts and I2C_IRQENABLE_CLR register for disabling interrupts. Why? (changelogs should always answer the why question) IOW, what is broken without this change, how does it fail? And equally important, how is it currently working? Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: omap: ensure writes to dev-buf_len are ordered
+Paul
Felipe Balbi ba...@ti.com writes:
if we allow compiler reorder our writes, we could
fall into a situation where dev-buf_len is reset
for no apparent reason.
Any chance this would help with the bug Paul found with I2C timeouts on
beagle userspace startup?
Kevin
This bug was found with a simple script which would
transfer data to an i2c client from 1 to 1024 bytes
(a simple for loop), when we got to transfer sizes
bigger than the fifo size, dev-buf_len was reset
to zero before we had an oportunity to handle XDR
Interrupt. Because dev-buf_len was zero, we entered
omap_i2c_transmit_data() to transfer zero bytes,
which would mean we would just silently exit
omap_i2c_transmit_data() without actually writing
anything to DATA register. That would cause XDR
IRQ to trigger forever and we would never transfer
the remaining bytes.
After adding the memory barrier, we also drop resetting
dev-buf_len to zero in omap_i2c_xfer_msg() because
both omap_i2c_transmit_data() and omap_i2c_receive_data()
will act until dev-buf_len reaches zero, rendering the
other write in omap_i2c_xfer_msg() redundant.
This patch has been tested with pandaboard for a few
iterations of the script mentioned above.
Signed-off-by: Felipe Balbi ba...@ti.com
---
This bug has been there forever, but it's quite annoying.
I think it deserves being pushed upstream during this -rc
cycle, but if Wolfram decides to wait until v3.8, I don't
mind.
drivers/i2c/busses/i2c-omap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..1ec4e6e 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -521,6 +521,7 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
/* REVISIT: Could the STB bit of I2C_CON be used with probing? */
dev-buf = msg-buf;
dev-buf_len = msg-len;
+ wmb();
omap_i2c_write_reg(dev, OMAP_I2C_CNT_REG, dev-buf_len);
@@ -579,7 +580,6 @@ static int omap_i2c_xfer_msg(struct i2c_adapter *adap,
*/
timeout = wait_for_completion_timeout(dev-cmd_complete,
OMAP_I2C_TIMEOUT);
- dev-buf_len = 0;
if (timeout == 0) {
dev_err(dev-dev, controller timed out\n);
omap_i2c_init(dev);
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RT throttling and suspend/resume (was Re: [PATCH] i2c: omap: revert i2c: omap: switch to threaded IRQ support)
Peter Zijlstra pet...@infradead.org writes: On Thu, 2012-10-18 at 08:51 +0300, Felipe Balbi wrote: So the primary question remains: is RT runtime supposed to include the time spent suspended? I suspect not. you might be right there, though we need Thomas or Peter to answer :-s re, sorry both tglx and I have been traveling, he still is, I'm trying to play catch-up :-) No worries, thanks for the help. Anyway, yeah I'm somewhat surprised the clock is 'running' when the machine isn't. From what I could gather, this is !x86 hardware, right? x86 explicitly makes sure our clocks are 'stopped' during suspend, see commit cd7240c0b900eb6d690ccee088a6c9b46dae815a. Can you do something similar for ARM? A quick look at arch/arm/kernel/sched_clock.c shows there's already suspend/resume hooks, do they do the wrong thing? No, they do the right thing, but only if they're asked by the SoC-specific code that registers a sched_clock. Changing the SoC specific code to use the 'needs_suspend' API gets things working perfectly. Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: RT throttling and suspend/resume (was Re: [PATCH] i2c: omap: revert i2c: omap: switch to threaded IRQ support)
Felipe Balbi ba...@ti.com writes:
On Wed, Oct 17, 2012 at 05:00:02PM +0300, Felipe Balbi wrote:
On Tue, Oct 16, 2012 at 02:39:50PM -0700, Kevin Hilman wrote:
+ peterz, tglx
Felipe Balbi ba...@ti.com writes:
[...]
The problem I see is that even though we properly return IRQ_WAKE_THREAD
and wake_up_process() manages to wakeup the IRQ thread (it returns 1),
the thread is never scheduled. To make things even worse, ouw irq thread
runs once, but doesn't run on a consecutive call. Here's some (rather
nasty) debug prints showing the problem:
[...]
[ 88.721923] try_to_wake_up 1411
[ 88.725189] === irq_wake_thread 139: IRQ 72 wake_up_process 0
[ 88.731292] [sched_delayed] sched: RT throttling activated
This throttling message is the key one.
With RT throttling activated, the IRQ thread will not be run (it
eventually will be allowed much later on, but by then, the I2C xfers
have timed out.)
As a quick hack, the throttling can be disabled by seeting the
sched_rt_runtime to RUNTIME_INF:
# sysctl -w kernel.sched_rt_runtime_us=-1
and a quick test shows that things go back to working as expected. But
we still need to figure out why the throttling is hapenning...
So I started digging into why the RT runtime was so high, and noticed
that time spent in suspend was being counted as RT runtime!
So spending time in suspend anywhere near sched_rt_runtime (0.95s) will
cause the RT throttling to always be triggered, and thus prevent IRQ
threads from running in the resume path. Ouch.
I think I'm already in over my head in the RT runtime stuff, but
counting the time spent in suspend as RT runtime smells like a bug to
me. no?
Peter? Thomas?
it looks like removing console output completely (echo 0
/proc/sysrq-trigger) I don't see the issue anymore. Let me just run for
a few more iterations to make sure what I'm saying is correct.
Yeah, really looks like removing console output makes the problem go
away. Ran a few iterations and it always worked fine. Full logs attached
Removing console output during resume is going to significantly change
the timing of what is happening during suspend/resume, so I suspect that
combined with all your other debug prints is somehow masking the
problem. How log are you letting the system stay in suspend?
That being said, I can still easily reproduce the problem, even with
console output disabled.
With vanilla v3.7-rc1 + the debug patch below[1], with and without
console output, I see RT throttling kicking in on resume, and the RT
runtime on resume corresponds to the time spent in suspend. Here's an
example of debug output of my patch below after ~3 sec in suspend:
[ 43.198028] sched_rt_runtime_exceeded: rt_time 2671752930 runtime 95000
[ 43.198028] update_curr_rt: RT runtime exceeded: irq/72-omap_i2c
[ 43.198059] update_curr_rt: RT runtime exceeded: irq/72-omap_i2c
[ 43.203704] [sched_delayed] sched: RT throttling activated
I see this rather consistently, and the rt_time value is always roughly the
time I spent in suspend.
So the primary question remains: is RT runtime supposed to include the
time spent suspended? I suspect not.
Kevin
[1]
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 418feb0..39de750 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -891,6 +891,8 @@ static int sched_rt_runtime_exceeded(struct rt_rq *rt_rq)
if (!once) {
once = true;
printk_sched(sched: RT throttling
activated\n);
+ pr_warn(%s: rt_time %llu runtime %llu\n,
+ __func__, rt_rq-rt_time, runtime);
}
} else {
/*
@@ -948,8 +950,11 @@ static void update_curr_rt(struct rq *rq)
if (sched_rt_runtime(rt_rq) != RUNTIME_INF) {
raw_spin_lock(rt_rq-rt_runtime_lock);
rt_rq-rt_time += delta_exec;
- if (sched_rt_runtime_exceeded(rt_rq))
+ if (sched_rt_runtime_exceeded(rt_rq)) {
+ pr_warn(%s: RT runtime exceeded: %s\n,
+ __func__, curr-comm);
resched_task(curr);
+ }
raw_spin_unlock(rt_rq-rt_runtime_lock);
}
}
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] twl4030: Fix chained irq handling on resume from suspend
Kalle Jokiniemi kalle.jokini...@jollamobile.com writes: The irqs are enabled one-by-one in pm core resume_noirq phase. This leads to situation where the twl4030 primary interrupt handler (PIH) is enabled before the chained secondary handlers (SIH). As the PIH cannot clear the pending interrupt, and SIHs have not been enabled yet, a flood of interrupts hangs the device. Fixed the issue by setting the SIH irqs with IRQF_EARLY_RESUME flags, so they get enabled before the PIH. Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com Acked-by: Kevin Hilman khil...@ti.com Thanks, I like this better than the I2C driver fix. Samuel, feel free to take this for v3.7 Kevin --- drivers/mfd/twl4030-irq.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/mfd/twl4030-irq.c b/drivers/mfd/twl4030-irq.c index ad733d7..cdd1173 100644 --- a/drivers/mfd/twl4030-irq.c +++ b/drivers/mfd/twl4030-irq.c @@ -672,7 +672,8 @@ int twl4030_sih_setup(struct device *dev, int module, int irq_base) irq = sih_mod + twl4030_irq_base; irq_set_handler_data(irq, agent); agent-irq_name = kasprintf(GFP_KERNEL, twl4030_%s, sih-name); - status = request_threaded_irq(irq, NULL, handle_twl4030_sih, 0, + status = request_threaded_irq(irq, NULL, handle_twl4030_sih, + IRQF_EARLY_RESUME, agent-irq_name ?: sih-name, NULL); dev_info(dev, %s (irq %d) chaining IRQs %d..%d\n, sih-name, -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
Kalle Jokiniemi kalle.jokini...@jollamobile.com writes: Hi, la, 2012-10-13 kello 01:00 +0530, Shubhrajyoti Datta kirjoitti: On Sat, Oct 13, 2012 at 12:10 AM, Kevin Hilman khil...@deeprootsystems.com wrote: From: Kevin Hilman khil...@ti.com Currently, runtime PM is used to keep the device enabled only during active transfers and for a configurable runtime PM autosuspend timout after an xfer. In addition to idling the device, driver's -runtime_suspend() method currently disables device interrupts when idle. However, on some SoCs (notably OMAP4+), the I2C hardware may shared with other coprocessors. This means that the MPU will still recieve interrupts if a coprocessor is using the I2C device. To avoid this, also disable interrupts at the MPU INTC when idling the device in -runtime_suspend() (and re-enable them in -runtime_resume().) This part based on an original patch from Shubhrajyoti Datta. NOTE: for proper sharing the I2C with a coprocessor, this driver still needs hwspinlock support added. This change is also meant to address an issue reported by Kalle Jokiniemi where I2C bus interrupt may be enabled before an I2C device interrupt handler (e.g. just after noirq resume phase) causing an It is actually in middle of resume_noirq. interrupt flood on the I2C bus interrupt before the device interrupt is enabled (e.g. interrupts coming from devices on I2C connected PMIC before the PMIC chained hanlder is enabled.) This problem is addresed by ensuring that the I2C bus interrupt left disabled until an I2C xfer is requested. Looks good to me. Will wait for Kalle though. Does not work for me :( As I said, the issue occurs for me when I enter static suspend (echo mem /sys/power/autosleep or /sys/power/state). I don't think doing this just in runtime pm will fix my issue. Or do those handlers get run in the normal suspend path as well? If the I2C device is still active during the suspend path, these handlers will get run by the PM domain code (in omap_device.) However, now that I think about it, the current omap_device PM domain code calls these at the noirq level, not the late/early level, so it does not address your original problem. :( I suspect we'll need this and your original patch. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Strashko, Grygorii grygorii.stras...@ti.com writes: Hi All, Sorry, for the late reply. + CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4. Hi Grygorii, thanks for reviewing. I was hoping you would have some ideas here as this was sounding familiar to something you had mentioned elsewhere. Regarding this patch - from my point of view, it fixes corner case and not an issue in general. Let take a look on resume sequence: - platform resume - syscore resume - resume_noirq - enable IRQs - resume_device_irqs() |- at this point IRQ handler will be invoked if IRQ state is IRQS_PENDING. |- so, the I2C device IRQ handler may be called at time when I2C adapter IRQ is still disabled and, as result, the I2C device IRQ-handler may fail. (I2C device and I2C adapter may use different physical IRQ lines) - resume_late |- enable I2C bus IRQ Possibly, the better way is to enable/disable I2C bus IRQ when needed - in our case in omap_i2c_xfer(). We use such approach in Android kernel 3.4 (http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e) I agree, that should work and cover the cases where I2C is used by other processors also. Shubhrajyoti already posted something similar[1] but it needed some rework (comments from Russell and myself.) Huzefa, Shubhrajyoti, who can rework this idea for the upstream driver and submit a patch? Wolfram, I guess for now lets hold off on $SUBJECT patch. Seems we can come up with a broader solution. Thanks. Kevin [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2012-October/124427.html -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: omap: Do not enable the irq always
+Kalle, Grygorii, Huzefa Shubhrajyoti D shubhrajy...@ti.com writes: Enable the irq in the transfer and disable it after the transfer is done. Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com --- drivers/i2c/busses/i2c-omap.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index b6c6b95..ce41596 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) if (IS_ERR_VALUE(r)) goto out; + enable_irq(dev-irq); r = omap_i2c_wait_for_bb(dev); if (r 0) goto out; @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) omap_i2c_wait_for_bb(dev); out: + disable_irq(dev-irq); When using runtime PM with auto-suspend timeouts, why would you disable the IRQ before the runtime suspend handlers have run? If you really want to do this, you probably should have these in the runtime PM callbacks. But I'll wait until you add a more descriptive changelog before I can really tell why this is being done. Based on the discussion in the patch from Kalle, I'm assuming this is to prevent interrups when I2C is being used by co-processors. If so, plese describe in the changelog. That being said, doesn't the runtime suspend callback already disable IRQs at the device level instead of the INTC level? Kevin pm_runtime_mark_last_busy(dev-dev); pm_runtime_put_autosuspend(dev-dev); return r; @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev) goto err_unuse_clocks; } + disable_irq(dev-irq); adap = dev-adapter; i2c_set_adapdata(adap, dev); adap-owner = THIS_MODULE; -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] i2c: omap: Do not enable the irq always
+Kalle, Grygorii, Huzefa Shubhrajyoti D shubhrajy...@ti.com writes: Enable the irq in the transfer and disable it after the transfer is done. Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com --- drivers/i2c/busses/i2c-omap.c |3 +++ 1 files changed, 3 insertions(+), 0 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index b6c6b95..ce41596 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -625,6 +625,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) if (IS_ERR_VALUE(r)) goto out; + enable_irq(dev-irq); r = omap_i2c_wait_for_bb(dev); if (r 0) goto out; @@ -654,6 +655,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) omap_i2c_wait_for_bb(dev); out: + disable_irq(dev-irq); When using runtime PM with auto-suspend timeouts, why would you disable the IRQ before the runtime suspend handlers have run? If you really want to do this, you probably should have these in the runtime PM callbacks. But I'll wait until you add a more descriptive changelog before I can really tell why this is being done. Based on the discussion in the patch from Kalle, I'm assuming this is to prevent interrups when I2C is being used by co-processors. If so, plese describe in the changelog. That being said, doesn't the runtime suspend callback already disable IRQs at the device level instead of the INTC level? Kevin pm_runtime_mark_last_busy(dev-dev); pm_runtime_put_autosuspend(dev-dev); return r; @@ -1179,6 +1181,7 @@ omap_i2c_probe(struct platform_device *pdev) goto err_unuse_clocks; } + disable_irq(dev-irq); adap = dev-adapter; i2c_set_adapdata(adap, dev); adap-owner = THIS_MODULE; -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Strashko, Grygorii grygorii.stras...@ti.com writes:
Hi Kevin,
yep, [1] is the same fix - thanks.
Hi Kalle,
i've applied these changes and PM runtime fix on top of
git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
(omap2plus_defconfig)
Could you check it with your use case, pls? (just to be sure that idea is
right)
I think the ideas is right, but [1] introduces more potential problems
since it disables the IRQ at the INTC well before being disabled at the
device. With runtime PM autosuspend timeouts, that means any IRQs
firing during the autosuspend delay will be lost, which basically
nullifies the use of autosuspend.
Since Shubhrajyoti didn't respond to my runtime PM comments on the
original patch, I'll respin this patch by moving the INTC disable/enable
into the runtime PM callbacks and make the changelog a bit more clear.
Grygorii, that pm_runtime_resume() change is needed to. Can you spin a
patch with just that change with a nice descriptive changelog about why
it is needed? Thanks.
Kevin
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..cb09e20 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -586,6 +586,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
if (IS_ERR_VALUE(r))
goto out;
+ /* We have the bus, enable IRQ */
+ enable_irq(dev-irq);
+
r = omap_i2c_wait_for_bb(dev);
if (r 0)
goto out;
@@ -606,6 +609,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
r = num;
omap_i2c_wait_for_bb(dev);
+ disable_irq(dev-irq);
out:
pm_runtime_put(dev-dev);
return r;
@@ -1060,6 +1064,9 @@ omap_i2c_probe(struct platform_device *pdev)
omap_i2c_isr;
r = request_irq(dev-irq, isr, IRQF_NO_SUSPEND, pdev-name, dev);
+ /* We enable IRQ only when request for I2C from master */
+disable_irq(dev-irq);
+
if (r) {
dev_err(dev-dev, failure requesting irq %i\n, dev-irq);
goto err_unuse_clocks;
@@ -1182,7 +1189,23 @@ static int omap_i2c_runtime_resume(struct device *dev)
}
#endif /* CONFIG_PM_RUNTIME */
+static int omap_i2c_suspend(struct device *dev)
+{
+ int ret;
+
+ /*
+* Enable I2C device, so it will be accessible during
+* later stages of suspending when device Runtime PM is disabled.
+* I2C device will be turned off at noirq suspend stage.
+*/
+ ret = pm_runtime_resume(dev);
+ if (ret 0)
+ return ret;
+ return 0;
+}
+
static struct dev_pm_ops omap_i2c_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(omap_i2c_suspend, NULL)
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
- Grygorii
From: Kevin Hilman [khil...@deeprootsystems.com]
Sent: Friday, October 12, 2012 5:34 PM
To: Strashko, Grygorii
Cc: Kalle Jokiniemi; linux-i2c@vger.kernel.org; w.s...@pengutronix.de;
ben-li...@fluff.org; t...@atomide.com; linux-o...@vger.kernel.org; Datta,
Shubhrajyoti; Kankroliwala, Huzefa
Subject: Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Strashko, Grygorii grygorii.stras...@ti.com writes:
Hi All,
Sorry, for the late reply.
+ CC Huzefa Kankroliwala - who is I2C driver owner on Android Kernel 3.4.
Hi Grygorii, thanks for reviewing. I was hoping you would have some
ideas here as this was sounding familiar to something you had
mentioned elsewhere.
Regarding this patch - from my point of view, it fixes corner case and not
an issue in general.
Let take a look on resume sequence:
- platform resume
- syscore resume
- resume_noirq
- enable IRQs - resume_device_irqs()
|- at this point IRQ handler will be invoked if IRQ state is
IRQS_PENDING.
|- so, the I2C device IRQ handler may be called at time when I2C
adapter IRQ is still disabled and, as result, the I2C device IRQ-handler may
fail. (I2C device and I2C adapter may use different physical IRQ lines)
- resume_late
|- enable I2C bus IRQ
Possibly, the better way is to enable/disable I2C bus IRQ when needed - in
our case in omap_i2c_xfer().
We use such approach in Android kernel 3.4
(http://git.omapzoom.org/?p=kernel/omap.git;a=commitdiff;h=1445a4d3b587c164bd30d108b6176007365e)
I agree, that should work and cover the cases where I2C is used by other
processors also. Shubhrajyoti already posted something similar[1] but
it needed some rework (comments from Russell and myself.)
Huzefa, Shubhrajyoti, who can rework this idea for the upstream and/or
follow up with the earlier patch[1]?
Wolfram, I guess for now lets hold off on $SUBJECT patch
[PATCH] i2c: omap: fix spurious IRQs: disable/enable IRQ at INTC when idle
From: Kevin Hilman khil...@ti.com Currently, runtime PM is used to keep the device enabled only during active transfers and for a configurable runtime PM autosuspend timout after an xfer. In addition to idling the device, driver's -runtime_suspend() method currently disables device interrupts when idle. However, on some SoCs (notably OMAP4+), the I2C hardware may shared with other coprocessors. This means that the MPU will still recieve interrupts if a coprocessor is using the I2C device. To avoid this, also disable interrupts at the MPU INTC when idling the device in -runtime_suspend() (and re-enable them in -runtime_resume().) This part based on an original patch from Shubhrajyoti Datta. NOTE: for proper sharing the I2C with a coprocessor, this driver still needs hwspinlock support added. This change is also meant to address an issue reported by Kalle Jokiniemi where I2C bus interrupt may be enabled before an I2C device interrupt handler (e.g. just after noirq resume phase) causing an interrupt flood on the I2C bus interrupt before the device interrupt is enabled (e.g. interrupts coming from devices on I2C connected PMIC before the PMIC chained hanlder is enabled.) This problem is addresed by ensuring that the I2C bus interrupt left disabled until an I2C xfer is requested. Cc: Kalle Jokiniemi kalle.jokini...@jollamobile.com Cc: Grygorii Strashko grygorii.stras...@ti.com Cc: Shubhrajyoti Datta shubhrajy...@ti.com, Cc: Huzefa Kankroliwala huzef...@ti.com Cc: Nishanth Menon n...@ti.com Signed-off-by: Kevin Hilman khil...@ti.com --- drivers/i2c/busses/i2c-omap.c |3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index db31eae..e6413e8 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1255,6 +1255,7 @@ static int omap_i2c_runtime_suspend(struct device *dev) /* Flush posted write */ omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG); } + disable_irq(_dev-irq); return 0; } @@ -1275,6 +1276,8 @@ static int omap_i2c_runtime_resume(struct device *dev) omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN); } + enable_irq(_dev-irq); + /* * Don't write to this register if the IE state is 0 as it can * cause deadlock. -- 1.7.9.2 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Shubhrajyoti Datta omaplinuxker...@gmail.com writes: On Fri, Oct 12, 2012 at 10:13 PM, Kevin Hilman khil...@deeprootsystems.com wrote: Strashko, Grygorii grygorii.stras...@ti.com writes: Hi Kevin, yep, [1] is the same fix - thanks. Hi Kalle, i've applied these changes and PM runtime fix on top of git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git (omap2plus_defconfig) Could you check it with your use case, pls? (just to be sure that idea is right) I think the ideas is right, but [1] introduces more potential problems since it disables the IRQ at the INTC well before being disabled at the device. I fail to see this point. Current driver supports master mode only. So unless there is a msg queued by the core. May be no interrupts should fire. what I mean msg - conf - intr - completion/error - autosuspend. With runtime PM autosuspend timeouts, that means any IRQs firing during the autosuspend delay will be lost, which basically nullifies the use of autosuspend. so I do not expect any interrupts between completion/error - autosuspend. Runtime PM is designed for concurrent users (hence the usecounting.) The I2C driver may not fully support this, since there is a single bus to share, but the usage of runtime PM currently allows multiple users to do runtime PM get/put (though in this driver they will block in the wait_for_bb.) So the fundamental problem in doing the enable/disable IRQ in the xfer function, and not the runtime PM callbacks is that you're ignoring the runtime PM usecount. You're assuming that the 'get' is *always* incrementing the usecount from zero to 1, and the 'put' is *always* a transition from 1 to zero. This may be the case in current usage and tests, but does not have to be the case. Even if that never happens in practice, it can in theory, and to me leads to confusing code. It simply doesn't make sense in terms of readability to disable the IRQ at the INTC in xfer, and disable IRQs at the device level in the runtime PM callback. To put it more simply: anything that needs to be done when the I2C is about to be idled/enabled should be done in the runtime PM callbacks. Please have a look at the patch I just sent[1]. In addition to making it more readable (IMO), it elminates the need for an extra disable_irq() in probe. Kevin [1] http://marc.info/?l=linux-omapm=135006723121147w=2 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] ARM: OMAP: i2c: fix interrupt flood during resume
Hi Kalle,
Kalle Jokiniemi kalle.jokini...@jollamobile.com writes:
The resume_noirq enables interrupts one-by-one starting from
first one. Now if the wake up event for suspend came from i2c
device, the i2c bus irq gets enabled before the threaded
i2c device irq, causing a flood of i2c bus interrupts as the
threaded irq that should clear the event is not enabled yet.
Fixed the issue by adding suspend_noirq and resume_early
functions that keep i2c bus interrupts disabled until
resume_noirq has run completely.
Issue was detected doing a wake up from autosleep with
twl4030 power key on N9. Patch tested on N9.
Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com
This version looks good, thanks for the extra comments.
Reviewed-by: Kevin Hilman khil...@ti.com
Tested-by: Kevin Hilman khil...@ti.com
Wolfram, This should also probably be Cc'd to stable since it affects
earlier kernels as well. Thanks,
Kevin
---
drivers/i2c/busses/i2c-omap.c | 34 ++
1 files changed, 34 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a0e49f6..991341b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1132,6 +1132,36 @@ static int __devexit omap_i2c_remove(struct
platform_device *pdev)
}
#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
+static int omap_i2c_suspend_noirq(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /* Disabling irq here to balance the enable in resume_early */
+ disable_irq(_dev-irq);
+ return 0;
+}
+
+static int omap_i2c_resume_early(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /*
+ * The noirq_resume enables the interrupts one by one,
+ * this causes a interrupt flood if the SW irq actually reading
+ * event from i2c device is enabled only after i2c bus irq, as the
+ * irq that should clear the event is still disabled. We have to
+ * keep the bus irq disabled until all other irqs have been enabled.
+ */
+ enable_irq(_dev-irq);
+
+ return 0;
+}
+#endif
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
@@ -1183,6 +1213,10 @@ static int omap_i2c_runtime_resume(struct device *dev)
#endif /* CONFIG_PM_RUNTIME */
static struct dev_pm_ops omap_i2c_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
+ .suspend_noirq = omap_i2c_suspend_noirq,
+ .resume_early = omap_i2c_resume_early,
+#endif
SET_RUNTIME_PM_OPS(omap_i2c_runtime_suspend,
omap_i2c_runtime_resume, NULL)
};
--
To unsubscribe from this list: send the line unsubscribe linux-i2c 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: i2c-omap: fix interrupt flood during resume
+Grygorii (who's been working on various I2C related suspend/resume
issues also)
Hi Kalle,
Kalle Jokiniemi kalle.jokini...@jollamobile.com writes:
The resume_noirq enables interrupts one-by-one starting from
first one. Now if the wake up event for suspend came from i2c
device, the i2c bus irq gets enabled before the threaded
i2c device irq, causing a flood of i2c bus interrupts as the
threaded irq that should clear the event is not enabled yet.
Ugh, another reason we need some sort of driver dependency tracking in the
driver model.
Fixed the issue by adding suspend_late and resume_early
functions that keep i2c bus interrupts disabled until
resume_noirq has run completely.
Hmm, any reason we couldn't put the disable in the .suspend_noirq
callback? The reason being is that other drivers might use I2C in their
suspend or late_suspend callbacks. I'm not aware of any using I2C in
their late_suspend callbacks in mainline, but in theory I2C should work
all the way through the late callbacks.
I know it might look strange to have a disable_irq() in the noirq
callback, since IRQs are already disabled at that point, but a comment
stating that it's just there to balance the (re)enable in the
early_resume callback should be clear.
Some other minor comments below...
Issue was detected doing a wake up from autosleep with
twl4030 power key on N9. Patch tested on N9.
Signed-off-by: Kalle Jokiniemi kalle.jokini...@jollamobile.com
---
drivers/i2c/busses/i2c-omap.c | 37 +
1 files changed, 37 insertions(+), 0 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 801df60..b77b0c2 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1158,6 +1158,35 @@ omap_i2c_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_SUSPEND
This should be CONFIG_PM_SLEEP in order to cover hibernation also.
+static int omap_i2c_suspend_late(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ /*
+ * The noirq_resume enables the interrupts one by one,
+ * this causes a interrupt flood if the SW irq actually reading
+ * event from i2c device is enabled only after i2c bus irq, as the
+ * irq that should clear the event is still disabled. We have to
+ * disable the bus irq until all other irqs have been enabled.
+ */
This comment probably belongs in the resume handler.
+ disable_irq(_dev-irq);
+ return 0;
+}
+
+static int omap_i2c_resume_early(struct device *dev)
+{
+
+ struct platform_device *pdev = to_platform_device(dev);
+ struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+ enable_irq(_dev-irq);
+
+ return 0;
+}
+#endif
#ifdef CONFIG_PM_RUNTIME
static int omap_i2c_runtime_suspend(struct device *dev)
{
@@ -1178,10 +1207,18 @@ static int omap_i2c_runtime_resume(struct device *dev)
return 0;
}
+#endif
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_PM_RUNTIME)
#ifdef CONFIG_PM will cover this
static struct dev_pm_ops omap_i2c_pm_ops = {
+#ifdef CONFIG_SUSPEND
again, CONFIG_PM_SLEEP here
+ .suspend_late = omap_i2c_suspend_late,
+ .resume_early = omap_i2c_resume_early,
+#endif
+#ifdef CONFIG_PM_RUNTIME
.runtime_suspend = omap_i2c_runtime_suspend,
.runtime_resume = omap_i2c_runtime_resume,
+#endif
};
#define OMAP_I2C_PM_OPS (omap_i2c_pm_ops)
#else
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP: convert I2C driver to PM QoS for MPU latency constraints
Jean Pihet jean.pi...@newoldbits.com writes: Convert the driver from the outdated omap_pm_set_max_mpu_wakeup_lat API to the new PM QoS API. Since the constraint is on the MPU subsystem, use the PM_QOS_CPU_DMA_LATENCY class of PM QoS. The resulting MPU constraints are used by cpuidle to decide the next power state of the MPU subsystem. The I2C device latency timing is derived from the FIFO size and the clock speed and so is applicable to all OMAP SoCs. Signed-off-by: Jean Pihet j-pi...@ti.com Acked-by: Kevin Hilman khil...@ti.com -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 00/23]I2C big cleanup
Kevin Hilman khil...@deeprootsystems.com writes: Kevin Hilman khil...@deeprootsystems.com writes: [...] Sorry to be late to the party (again), but still catching up after some time off. Unfortunately, this series causes PM regressions on several OMAP platforms. I hope we can hold off on this until those issues are addressed. I tracked the regression down to [PATCHv8 21/22] (see reply there.) Since this series is already merged, I suggest that the problem patch be reverted, at least for v3.7 and until the problem is better understood and tested. With that patch reverted, all my PM tests are passing. Feel free to add: OK, the i2c series is off the hook. Felipe and I spent a little time tracking this down. Felipe suggested that there might be a driver with periodic i2c activity keeping I2C awake, and thus preventing CORE retention. He was right. On the boards where I'm seeing the failure, the RTC is firing every second, and since the RTC is on the I2C connected PMIC, the PMIC IRQ reads and the RTC reads are causing lots of I2C activity every second. With the new autosuspend feature, that is enough to keep the I2C active continually and prevent CORE retention. So, all that to say, from my PoV, this series can go in as is. The PM problem is caused by the RTC driver someplace. Thanks Felipe, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 00/23]I2C big cleanup
Felipe Balbi ba...@ti.com writes: Hi, On Thu, Sep 13, 2012 at 11:04:42AM -0700, Kevin Hilman wrote: Kevin Hilman khil...@deeprootsystems.com writes: Kevin Hilman khil...@deeprootsystems.com writes: [...] Sorry to be late to the party (again), but still catching up after some time off. Unfortunately, this series causes PM regressions on several OMAP platforms. I hope we can hold off on this until those issues are addressed. I tracked the regression down to [PATCHv8 21/22] (see reply there.) Since this series is already merged, I suggest that the problem patch be reverted, at least for v3.7 and until the problem is better understood and tested. With that patch reverted, all my PM tests are passing. Feel free to add: OK, the i2c series is off the hook. Felipe and I spent a little time tracking this down. Felipe suggested that there might be a driver with periodic i2c activity keeping I2C awake, and thus preventing CORE retention. He was right. FYI, the original idea came from Shubhro. We agreed that would be the only way i2c would be prevented from idling. Great, thanks Shubhro! Also, FYI, I just submitted a patch to the TWL RTC driver which was the source of all the I2C activity since it's on the I2C-connected PMIC. Thanks for the help and suggestions, Kevin [1] https://groups.google.com/forum/#!topic/rtc-linux/sFbYmAzCRLQ -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 00/23]I2C big cleanup
Shubhrajyoti D shubhrajy...@ti.com writes:
[...]
This is the cleanup only series.
Tested on omap4sdp and 3430sdp.
It would be extremely helpful if you would describe how this was tested.
And for me, it would be a source of extreme joy if you described any PM
testing.
I gave this some additional PM testing on 3430/n900, 3530/Overo,
3730/OveroSTORM, 3730/Beagle-xM and 4430/Panda.
I tested v3.6-rc5 which passed all PM tests and then added this series
(by merging the i2c-embedded/i2c-next branch.) PM tests then fail.
At least on 3530/Overo and 3730/Overo, CORE no longer hits retenion (or
off) during idle.
The easy way to notice this is to see that even when no i2c transactions
are happening, the runtime PM status for omap_i2c.1 is remains 'active':
# cat omap_i2c.*/power/runtime_status
active
suspended
Of course that means that clocks are never gated during idle, and CORE
will never hit retention.
I noticed it because my PM tests detected that the CORE powerdomain was
not transitioning to retention (or off) during idle.
To reproduce, simply enable UART timeouts so CORE will hit retention:
echo 3000 /sys/devices/platform/omap_uart.0/power/autosuspend_delay_ms
echo 3000 /sys/devices/platform/omap_uart.1/power/autosuspend_delay_ms
echo 3000 /sys/devices/platform/omap_uart.2/power/autosuspend_delay_ms
echo 3000 /sys/devices/platform/omap_uart.3/power/autosuspend_delay_ms
and check the core_pwrm state counters:
cat /debug/pm_debug/count
wait 3 seconds for the UART autosuspend timers to kick in. At this
point, CORE should be transitioning to retenion in idle (determined by
noticing that the RET counter for core_pwrdm is increasing.)
With $SUBJECT series applied, you'll notice that CORE is not
transitioning.
Kevin
The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217:
Linux 3.6-rc5 (2012-09-08 16:43:45 -0700)
are available in the git repository at:
git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups
Felipe Balbi (22):
i2c: omap: switch to devm_* API
i2c: omap: simplify num_bytes handling
i2c: omap: decrease indentation level on data handling
i2c: omap: add blank lines
i2c: omap: simplify omap_i2c_ack_stat()
i2c: omap: split out [XR]DR and [XR]RDY
i2c: omap: improve i462 errata handling
i2c: omap: re-factor receive/transmit data loop
i2c: omap: switch over to do {} while loop
i2c: omap: ack IRQ in parts
i2c: omap: switch to platform_get_irq()
i2c: omap: bus: add a receiver flag
i2c: omap: simplify errata check
i2c: omap: always return IRQ_HANDLED
i2c: omap: simplify IRQ exit path
i2c: omap: resize fifos before each message
i2c: omap: get rid of the complete label
i2c: omap: always return IRQ_HANDLED
i2c: omap: switch to threaded IRQ support
i2c: omap: remove unnecessary pm_runtime_suspended check
i2c: omap: switch over to autosuspend API
i2c: omap: sanitize exit path
Shubhrajyoti D (1):
i2c: omap: remove redundant status read
drivers/i2c/busses/i2c-omap.c | 442
+
1 files changed, 271 insertions(+), 171 deletions(-)
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 00/23]I2C big cleanup
Wolfram Sang w.s...@pengutronix.de writes: On Wed, Sep 12, 2012 at 04:27:54PM +0530, Shubhrajyoti D wrote: [...] This is the cleanup only series. Tested on omap4sdp and 3430sdp. The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217: Linux 3.6-rc5 (2012-09-08 16:43:45 -0700) are available in the git repository at: git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups Pushed to -next. Thanks to all involved! Sorry to be late to the party (again), but still catching up after some time off. Unfortunately, this series causes PM regressions on several OMAP platforms. I hope we can hold off on this until those issues are addressed. We *really* need to have some more thorough testing (especially PM) before merging large series like this. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 21/22] i2c: omap: switch over to autosuspend API
Shubhrajyoti D shubhrajy...@ti.com writes:
From: Felipe Balbi ba...@ti.com
this helps us reduce unnecessary pm transitions
in case we have another i2c message starting soon.
Signed-off-by: Felipe Balbi ba...@ti.com
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
I tracked the PM regression down to this patch.
---
drivers/i2c/busses/i2c-omap.c | 12 ++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 6d38a57..122f517 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -55,6 +55,9 @@
/* timeout waiting for the controller to respond */
#define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))
+/* timeout for pm runtime autosuspend */
+#define OMAP_I2C_PM_TIMEOUT 1000/* ms */
+
/* For OMAP3 I2C_IV has changed to I2C_WE (wakeup enable) */
enum {
OMAP_I2C_REV_REG = 0,
@@ -645,7 +648,8 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
omap_i2c_wait_for_bb(dev);
out:
- pm_runtime_put(dev-dev);
+ pm_runtime_mark_last_busy(dev-dev);
+ pm_runtime_put_autosuspend(dev-dev);
Reverting this change allows CORE to hit retention again.
I didn't debug this any further, so I'm not sure exactly why the async
suspend works but not the autosuspend.
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 00/23]I2C big cleanup
Kevin Hilman khil...@deeprootsystems.com writes: Wolfram Sang w.s...@pengutronix.de writes: On Wed, Sep 12, 2012 at 04:27:54PM +0530, Shubhrajyoti D wrote: [...] This is the cleanup only series. Tested on omap4sdp and 3430sdp. The following changes since commit 55d512e245bc7699a8800e23df1a24195dd08217: Linux 3.6-rc5 (2012-09-08 16:43:45 -0700) are available in the git repository at: git://gitorious.org/linus-tree/linus-tree.git for_3.7/i2c/big_cleanups Pushed to -next. Thanks to all involved! Sorry to be late to the party (again), but still catching up after some time off. Unfortunately, this series causes PM regressions on several OMAP platforms. I hope we can hold off on this until those issues are addressed. I tracked the regression down to [PATCHv8 21/22] (see reply there.) Since this series is already merged, I suggest that the problem patch be reverted, at least for v3.7 and until the problem is better understood and tested. With that patch reverted, all my PM tests are passing. Feel free to add: Tested-by: Kevin Hilman khil...@ti.com Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: OMAP: xfer: fix runtime PM get/put balance on error
Hi Wolfram, Kevin Hilman khil...@ti.com writes: In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure. Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle. Cc: Shubhrajyoti D shubhrajy...@ti.com Signed-off-by: Kevin Hilman khil...@ti.com --- This patch applies to current i2c-embedded/for-next branch This one is needed for v3.6. Can you queue this up as a fix for v3.6-rc? Thanks, Kevin drivers/i2c/busses/i2c-omap.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 9895fa7..b105733 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) r = pm_runtime_get_sync(dev-dev); if (IS_ERR_VALUE(r)) - return r; + goto out; r = omap_i2c_wait_for_bb(dev); if (r 0) -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: OMAP: fix runtime PM get/put balance on error
Shubhrajyoti D shubhrajy...@ti.com writes: ensure pm_runtime_put() is called, on pm_runtime_get_sync() failure. Without this, after a failed call, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. Thanks to Kevin for educating about it. While at it also fix a missing pm_runtime_disable in the probe error path. This is the same subject and changelog as the patch I sent, but is a different patch. Please write a new subject and a changelog specific to your patch. As this changes the error/failure path, please be specific about how the failure modes were tested, and on which platforms. Cc: Kevin Hilman khil...@ti.com Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com --- drivers/i2c/busses/i2c-omap.c |7 +++ 1 files changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 2500f19..c8e5c76 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -1113,10 +1113,10 @@ err_free_irq: free_irq(dev-irq, dev); err_unuse_clocks: omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); +err_free_mem: pm_runtime_put(dev-dev); iounmap(dev-base); This doesn't look right. At least one of the gotos for this label, the ioremap has failed. pm_runtime_disable(pdev-dev); -err_free_mem: platform_set_drvdata(pdev, NULL); kfree(dev); err_release_region: @@ -1136,10 +1136,9 @@ static int __devexit omap_i2c_remove(struct platform_device *pdev) free_irq(dev-irq, dev); i2c_del_adapter(dev-adapter); ret = pm_runtime_get_sync(pdev-dev); - if (IS_ERR_VALUE(ret)) - return ret; + if (!IS_ERR_VALUE(ret)) + omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); this change isn't described in changelog - omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0); pm_runtime_put(pdev-dev); pm_runtime_disable(pdev-dev); iounmap(dev-base); Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: OMAP: fix runtime PM get/put balance on error
Shubhrajyoti shubhrajy...@ti.com writes: On Friday 29 June 2012 05:32 PM, Kevin Hilman wrote: Shubhrajyoti D shubhrajy...@ti.com writes: ensure pm_runtime_put() is called, on pm_runtime_get_sync() failure. Without this, after a failed call, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. Thanks to Kevin for educating about it. While at it also fix a missing pm_runtime_disable in the probe error path. This is the same subject and changelog as the patch I sent, but is a different patch. Please write a new subject and a changelog specific to your patch. OK. Actually I did that on purpose your patch fixed the xfer call only. I thought that since get_sync increments the count always we could extend the patch to all the callers. As this changes the error/failure path, please be specific about how the failure modes were tested, and on which platforms. I found and fixed by review only. Didnt really hit the failure case. Which is why you should't just add stuff to other peoples work. My patch was targetted at the XFER problem during suspend/resume which I found by testing and and proved the fix by testing. Your patch adds several additional functional changes that were not well described and not tested. That calls for a separate patch with seprate subject/changelog etc. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: OMAP: xfer: fix runtime PM get/put balance on error
Hi Shubhrajyoti, Shubhrajyoti Datta omaplinuxker...@gmail.com writes: Hi Kevin, Thanks for the patch , a doubt below Thanks for the review. On Wed, Jun 27, 2012 at 7:15 AM, Kevin Hilman khil...@ti.com wrote: In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure. So the failure means that the usecount is incremented. However the device was not enabled. Correct. In that case could we consider void pm_runtime_put_noidle(struct device *dev); - decrement the device's usage counter Which will only decrement the counter and does not try to disable it. No, that is not needed. However I am not sure what happens if you try to disable an already disabled device. The runtime PM core already knows that the device is not enabled so will not need to call the disable hooks. It already needs this functionality to support the _get_noresume() and _put_noidle() calls. I tested this on 3730/OveroSTORM where I was seeing this i2c xfer failure (and thus a failure in MMC suspend, which uses I2C to control the regulator.) Hope that helps clarify, If so, Wolfram, can you queue this up in your i2c-embedded/for-next branch since this problem was introduced there. Thanks, Kevin Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented Agree. causing the usecount to never reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle. Cc: Shubhrajyoti D shubhrajy...@ti.com Signed-off-by: Kevin Hilman khil...@ti.com --- This patch applies to current i2c-embedded/for-next branch drivers/i2c/busses/i2c-omap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 9895fa7..b105733 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) r = pm_runtime_get_sync(dev-dev); if (IS_ERR_VALUE(r)) - return r; + goto out; r = omap_i2c_wait_for_bb(dev); if (r 0) -- 1.7.9.2 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/11] I2C: OMAP: Handle error check for pm runtime
Shubhrajyoti D shubhrajy...@ti.com writes: If PM runtime get_sync fails return with the error so that no further reads/writes goes through the interface. This will avoid possible abort. Add a error message in case of failure with the cause of the failure. Reviewed-by: Kevin Hilman khil...@ti.com Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com This patch introduced a regression where the runtime PM usecount will never reach zero and so CORE retention is prevented after any xfer failures... --- -v10 Use IS_ERR_VALUE -v9 Fix the error handling drivers/i2c/busses/i2c-omap.c | 14 +++--- 1 files changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 44e8cfa..c39b72f 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -585,7 +585,9 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) int i; int r; - pm_runtime_get_sync(dev-dev); + r = pm_runtime_get_sync(dev-dev); + if (IS_ERR_VALUE(r)) + return r; This return should be 'goto out' so the runtime PM usecount is decremented by the 'put'. Otherwise, after failure, the usecount remains non-zero, so the device is considered 'active' and keeps the containing power domain active. I found this on a 3730/OveroSTORM where the suspend/resume of MMC fails because I2C is already suspended. After the suspend though, the CORE powerdomain never again hits retention, and I tracked it down to this. I'll send a separate patch to fix this shortly. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] I2C: OMAP: xfer: fix runtime PM get/put balance on error
In omap_i2c_xfer(), ensure pm_runtime_put() is called, even on failure. Without this, after a failed xfer, the runtime PM usecount will have been incremented, but not decremented causing the usecount to never reach zero after a failure. This keeps the device always runtime PM enabled which keeps the enclosing power domain active, and prevents full-chip retention/off from happening during idle. Cc: Shubhrajyoti D shubhrajy...@ti.com Signed-off-by: Kevin Hilman khil...@ti.com --- This patch applies to current i2c-embedded/for-next branch drivers/i2c/busses/i2c-omap.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 9895fa7..b105733 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -582,7 +582,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) r = pm_runtime_get_sync(dev-dev); if (IS_ERR_VALUE(r)) - return r; + goto out; r = omap_i2c_wait_for_bb(dev); if (r 0) -- 1.7.9.2 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 00/10] I2C fixes
Shubhrajyoti D shubhrajy...@ti.com writes: The patch series does the following - Warn fixes if CONFIG_PM_RUNTIME is not selected. - In case of i2c remove register access was done without any get_sync fix the same. - Folds a patch from Tasslehoff to prevent any merge conflicts. - Prevents the XDUF flag to be set if the underflow condition is not met. - As per discussion in [1] .Adds a patch to rename the 1p153 errata and use the unique id instead as the section number in the recent errata docs has changed. v9: Fix the comments from Wolfram Sang [1] http://www.spinics.net/lists/linux-i2c/msg07607.html Tested on omap4sdp and omap3sdp. Can you also describe how it was tested? With the runtime PM changes, does it still hit full-chip retention in idle and suspend after these changes? I had a few minor comments on this version, otherwise feel free add Reviewed-by: Kevin Hilman khil...@ti.com That being said, before this is merged, I woudl like to see some more non-author Tested-bys. We've been having lots of regressions of late from OMAP drivers that are not being sufficiently tested before merging. We need to ensure proper testing before merge. Other testers should also report what platforms they tested on, and how it was tested. Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv9 00/10] I2C fixes
+Neil Brown
Shubhrajyoti D shubhrajy...@ti.com writes:
The patch series does the following
- Warn fixes if CONFIG_PM_RUNTIME is not selected.
- In case of i2c remove register access was done without any
get_sync fix the same.
- Folds a patch from Tasslehoff to prevent any merge conflicts.
- Prevents the XDUF flag to be set if the underflow condition is not met.
- As per discussion in [1] .Adds a patch to rename the 1p153 errata and
use the unique id instead as the section number in the recent errata
docs has changed.
Shubhrajyoti,
Can you add one more patch to this series.
The patch below from Neil Brown has been circulating for awhile, and
I've been using it locally for awhile now too. It would help if it got
into this series and got some broader testing.
Thanks,
Kevin
From 0c6effd8356e6273c294490a576551ef37ae6799 Mon Sep 17 00:00:00 2001
From: NeilBrown ne...@suse.de
Date: Fri, 30 Dec 2011 12:40:30 +1100
Subject: [PATCH] OMAP/I2C - Fix timeout problem during suspend.
On a board with OMAP3 processor and TWL4030 Power management,
we need to talk to the TWL4030 during late suspend but cannot
because the I2C interrupt is disabled (as late suspend disables
interrupt).
e.g. I get messages like:
[ 62.161102] musb-omap2430 musb-omap2430: LATE power domain suspend
[ 63.167205] omap_i2c omap_i2c.1: controller timed out
[ 63.183044] twl: i2c_read failed to transfer all messages
[ 64.182861] omap_i2c omap_i2c.1: controller timed out
[ 64.198455] twl: i2c_write failed to transfer all messages
[ 65.198455] omap_i2c omap_i2c.1: controller timed out
[ 65.203765] twl: i2c_write failed to transfer all messages
The stack shows omap2430_runtime_suspend calling twl4030_set_suspend
which tries to power-down the USB PHY (twl4030_phy_suspend -
twl4030_phy_power - __twl4030_phy_power which as a nice WARN_ON
that helps).
Then we get the same in resume:
[ 69.603912] musb-omap2430 musb-omap2430: EARLY power domain resume
[ 70.610473] omap_i2c omap_i2c.1: controller timed out
[ 70.626129] twl: i2c_write failed to transfer all messages
etc.
So don't disable interrupts for I2C.
Acked-by: Kevin Hilman khil...@ti.com
Tested-by: Kevin Hilman khil...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
---
drivers/i2c/busses/i2c-omap.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 801df60..e024c50 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1092,7 +1092,7 @@ omap_i2c_probe(struct platform_device *pdev)
isr = (dev-rev OMAP_I2C_OMAP1_REV_2) ? omap_i2c_omap1_isr :
omap_i2c_isr;
- r = request_irq(dev-irq, isr, 0, pdev-name, dev);
+ r = request_irq(dev-irq, isr, IRQF_NO_SUSPEND, pdev-name, dev);
if (r) {
dev_err(dev-dev, failure requesting irq %i\n, dev-irq);
--
1.7.9.2
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 04/18] I2C: OMAP: I2C register restore only if context is lost
Datta, Shubhrajyoti shubhrajy...@ti.com writes:
Hi Kevin,
Hi Kevin,
Thanks for your review,
On Tue, Apr 17, 2012 at 7:15 PM, Kevin Hilman khil...@ti.com wrote:
Shubhrajyoti D shubhrajy...@ti.com writes:
Currently i2c register restore is done always.
Adding conditional restore.
The i2c register restore is done only if the context is lost.
OK, minor comment below.
Thanks for the suggestion of the error case restore.
Updated the patch. Also added Tony's ack for the plat part.
From 1ca222f6f50868e07d9a46575e73dd66fd2d542e Mon Sep 17 00:00:00 2001
From: Shubhrajyoti D shubhrajy...@ti.com
Date: Tue, 17 Jan 2012 16:13:03 +0530
Subject: [PATCH] I2C: OMAP: I2C register restore only if context is lost
Currently i2c register restore is done always.
Adding conditional restore.
The i2c register restore is done only if the context is lost
or in case of error to be on the safe side.
Also remove the definition of SYSS_RESETDONE_MASK and use the
one in omap_hwmod.h.
Cc: Kevin Hilman khil...@ti.com
[For the arch/arm/plat-omap/i2c.c part]
Acked-by: Tony Lindgren t...@atomide.com
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
---
arch/arm/plat-omap/i2c.c |3 ++
drivers/i2c/busses/i2c-omap.c | 53
+++--
include/linux/i2c-omap.h |1 +
3 files changed, 39 insertions(+), 18 deletions(-)
diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
index db071bc..4ccab07 100644
--- a/arch/arm/plat-omap/i2c.c
+++ b/arch/arm/plat-omap/i2c.c
@@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
*/
if (cpu_is_omap34xx())
pdata-set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
+
+ pdata-get_context_loss_count = omap_pm_get_dev_context_loss_count;
+
pdev = omap_device_build(name, bus_id, oh, pdata,
sizeof(struct omap_i2c_bus_platform_data),
NULL, 0, 0);
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a882558..76cf066 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -43,6 +43,7 @@
#include linux/slab.h
#include linux/i2c-omap.h
#include linux/pm_runtime.h
+#include plat/omap_device.h
/* I2C controller revisions */
#define OMAP_I2C_OMAP1_REV_2 0x20
@@ -157,9 +158,6 @@ enum {
#define OMAP_I2C_SYSTEST_SDA_I (1 1)/* SDA line
sense in */
#define OMAP_I2C_SYSTEST_SDA_O (1 0)/* SDA line
drive out */
-/* OCP_SYSSTATUS bit definitions */
-#define SYSS_RESETDONE_MASK (1 0)
-
Unrelated to this patch.
/* OCP_SYSCONFIG bit definitions */
#define SYSC_CLOCKACTIVITY_MASK (0x3 8)
#define SYSC_SIDLEMODE_MASK (0x3 3)
@@ -184,6 +182,7 @@ struct omap_i2c_dev {
u32 latency;/* maximum mpu wkup latency */
void(*set_mpu_wkup_lat)(struct device *dev,
long latency);
+ int (*get_context_loss_count)(struct device *dev);
u32 speed; /* Speed of bus in kHz */
u32 dtrev; /* extra revision from DT */
u32 flags;
@@ -206,6 +205,7 @@ struct omap_i2c_dev {
u16 syscstate;
u16 westate;
u16 errata;
+ int dev_lost_count;
};
static const u8 reg_map_ip_v1[] = {
@@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev)
dev-speed = pdata-clkrate;
dev-flags = pdata-flags;
dev-set_mpu_wkup_lat = pdata-set_mpu_wkup_lat;
+ dev-get_context_loss_count = pdata-get_context_loss_count;
dev-dtrev = pdata-rev;
}
@@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_RUNTIME
+static void omap_i2c_restore(struct omap_i2c_dev *dev)
+{
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+ omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev-pscstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev-scllstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev-sclhstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, dev-bufstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev-westate);
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
+ /*
+ * Don't write to this register if the IE state is 0 as it can
+ * cause deadlock.
+ */
+ if (dev-iestate)
+ omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev-iestate);
+
+}
static int omap_i2c_runtime_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
u16 iv;
+ if (_dev
Re: [PATCHv8 04/18] I2C: OMAP: I2C register restore only if context is lost
Shubhrajyoti D shubhrajy...@ti.com writes:
Currently i2c register restore is done always.
Adding conditional restore.
The i2c register restore is done only if the context is lost.
OK, minor comment below.
Also remove the definition of SYSS_RESETDONE_MASK and use the
one in omap_hwmod.h.
The definition is removed, but I don't see any of the users removed.
If the users were cleaned up earlier in the series, then the removal of
this should be done with that, or as a separate patch. I don't see why
it belongs with this patch.
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
---
arch/arm/plat-omap/i2c.c |3 ++
drivers/i2c/busses/i2c-omap.c | 52 ++--
include/linux/i2c-omap.h |1 +
3 files changed, 38 insertions(+), 18 deletions(-)
diff --git a/arch/arm/plat-omap/i2c.c b/arch/arm/plat-omap/i2c.c
index db071bc..4ccab07 100644
--- a/arch/arm/plat-omap/i2c.c
+++ b/arch/arm/plat-omap/i2c.c
@@ -179,6 +179,9 @@ static inline int omap2_i2c_add_bus(int bus_id)
*/
if (cpu_is_omap34xx())
pdata-set_mpu_wkup_lat = omap_pm_set_max_mpu_wakeup_lat_compat;
+
+ pdata-get_context_loss_count = omap_pm_get_dev_context_loss_count;
+
pdev = omap_device_build(name, bus_id, oh, pdata,
sizeof(struct omap_i2c_bus_platform_data),
NULL, 0, 0);
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a882558..45389db 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -43,6 +43,7 @@
#include linux/slab.h
#include linux/i2c-omap.h
#include linux/pm_runtime.h
+#include plat/omap_device.h
/* I2C controller revisions */
#define OMAP_I2C_OMAP1_REV_2 0x20
@@ -157,9 +158,6 @@ enum {
#define OMAP_I2C_SYSTEST_SDA_I (1 1)/* SDA line
sense in */
#define OMAP_I2C_SYSTEST_SDA_O (1 0)/* SDA line
drive out */
-/* OCP_SYSSTATUS bit definitions */
-#define SYSS_RESETDONE_MASK (1 0)
-
/* OCP_SYSCONFIG bit definitions */
#define SYSC_CLOCKACTIVITY_MASK (0x3 8)
#define SYSC_SIDLEMODE_MASK (0x3 3)
@@ -184,6 +182,7 @@ struct omap_i2c_dev {
u32 latency;/* maximum mpu wkup latency */
void(*set_mpu_wkup_lat)(struct device *dev,
long latency);
+ int (*get_context_loss_count)(struct device *dev);
u32 speed; /* Speed of bus in kHz */
u32 dtrev; /* extra revision from DT */
u32 flags;
@@ -206,6 +205,7 @@ struct omap_i2c_dev {
u16 syscstate;
u16 westate;
u16 errata;
+ int dev_lost_count;
};
static const u8 reg_map_ip_v1[] = {
@@ -1025,6 +1025,7 @@ omap_i2c_probe(struct platform_device *pdev)
dev-speed = pdata-clkrate;
dev-flags = pdata-flags;
dev-set_mpu_wkup_lat = pdata-set_mpu_wkup_lat;
+ dev-get_context_loss_count = pdata-get_context_loss_count;
dev-dtrev = pdata-rev;
}
@@ -1151,12 +1152,32 @@ omap_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_RUNTIME
+static void omap_i2c_restore(struct omap_i2c_dev *dev)
+{
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
+ omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev-pscstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLL_REG, dev-scllstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_SCLH_REG, dev-sclhstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, dev-bufstate);
+ omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev-westate);
+ omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
+ /*
+ * Don't write to this register if the IE state is 0 as it can
+ * cause deadlock.
+ */
+ if (dev-iestate)
+ omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, dev-iestate);
+
+}
static int omap_i2c_runtime_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
u16 iv;
+ if (_dev-get_context_loss_count)
+ _dev-dev_lost_count = _dev-get_context_loss_count(dev);
+
_dev-iestate = omap_i2c_read_reg(_dev, OMAP_I2C_IE_REG);
if (_dev-dtrev == OMAP_I2C_IP_VERSION_2)
omap_i2c_write_reg(_dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1);
@@ -1179,24 +1200,19 @@ static int omap_i2c_runtime_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+ int loss_cnt;
- if (_dev-flags OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
-
Re: [PATCH RFC] I2C : OMAP : make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME
Shubhrajyoti shubhrajy...@ti.com writes: On Thursday 12 January 2012 03:46 AM, Kevin Hilman wrote: Shubhrajyoti D shubhrajy...@ti.com writes: The functions omap_i2c_unidle/idle are called from omap_i2c_runtime_resume and omap_i2c_runtime_suspend which is compiled for CONFIG_PM_RUNTIME. Make the omap_i2c_unidle/idle also depend on CONFIG_PM_RUNTIME flag. I probably should've done this when I initially cleaned up the callbacks, but since you're doing it... rather than move the functions within the file, just remove the functions and move the code into the runtime callbacks. That may break the modularity of the code since the omap_i2c_unidle is responsible for restore. I would have preferred to keep it separate. Do you think I could rename omap_i2c_unidle to omap_i2c_restore instead ? that doesn't help: It does restore and (re)enable interrupts. However don't feel strongly about it either dif you still feel that it should be moved into runtime callbacks directly I am ok. Yes, just move it into the runtime callbacks as it's the only place they are used. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RFC] I2C : OMAP : make omap_i2c_unidle/idle functions depend on CONFIG_PM_RUNTIME
Shubhrajyoti D shubhrajy...@ti.com writes: The functions omap_i2c_unidle/idle are called from omap_i2c_runtime_resume and omap_i2c_runtime_suspend which is compiled for CONFIG_PM_RUNTIME. Make the omap_i2c_unidle/idle also depend on CONFIG_PM_RUNTIME flag. I probably should've done this when I initially cleaned up the callbacks, but since you're doing it... rather than move the functions within the file, just remove the functions and move the code into the runtime callbacks. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] I2C: OMAP: Recover from Bus Busy condition
Shubhrajyoti D shubhrajy...@ti.com writes: From: Vikram Pandita vikram.pand...@ti.com In case a peripheral is driving SDA bus low (ie. a start condition), provide a constant clock output using the test mode of the OMAP I2C controller to try and clear the bus. Soft reset I2C controller after attempting the bus clear to ensure that controller is in a good state. Based upon Vikram Pandita's patch from TI Android 3.0 kernel and modified for mainline by Jon Hunter. A couple differences from the original patch ... 1. Add a new function for bus clear 2. Ensure that the CON.I2C_EN bit is set when using the SYSTEST feature to output a permanent clock. This bit needs to be set and tpyically it would be set by the unidle function but this is not the case for all OMAP generations. 3. Program the SYSTEST setting only the bits we care about. However, restore SYSTEST registers to there original state as some OMAP generations do not implement perform a soft-reset. 4. Clear the CON register after performing the bus clear, so when we call the init function the controller is disabled and the init function will re-enable later. Cc: Kevin Hilman khil...@ti.com Signed-off-by: Vikram Pandita vikram.pand...@ti.com Signed-off-by: Jon Hunter jon-hun...@ti.com Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com --- Original patch can be found here: http://git.omapzoom.org/?p=kernel/omap.git;a=commit;h=a2ab04192ba25e60f95ba1ff3af5601a2d7b5bd1 applies on Kevin's for_3.3/i2c/misc Please also explain how this was tested, and on what platforms. Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8 4/5] OMAP: I2C: Remove the reset in the init path
Datta, Shubhrajyoti shubhrajy...@ti.com writes: On Tue, Jan 10, 2012 at 11:53 AM, Datta, Shubhrajyoti shubhrajy...@ti.com wrote: On Fri, Dec 16, 2011 at 2:29 PM, Shubhrajyoti shubhrajy...@ti.com wrote: Ben, On Friday 16 December 2011 02:10 PM, Paul Walmsley wrote: Hi On Tue, 13 Dec 2011, Shubhrajyoti D wrote: - The reset in the driver at init is not needed anymore as the hwmod framework takes care of reseting it. - Reset is removed from omap_i2c_init, which was called not only during probe, but also after time out and error handling. device_reset were added in those places to effect the reset. - Earlier the hwmod SYSC settings were over-written in the driver. Removing the same and letting the hwmod take care of the settings. - Clean up the SYSS_RESETDONE_MASK macro as it is no longer needed. - Clean up the SYSCONFIG SYSC bit definition macros. - Fix the typos in wakeup. Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com --- drivers/i2c/busses/i2c-omap.c | 83 +++- 1 files changed, 23 insertions(+), 60 deletions(-) This patch either needs to be acked by Ben with a note that it's okay for us to merge through the OMAP tree, or needs to be merged by Ben during the 3.4 merge window, after patches 1-3 have reached the mainline tree. I agree. Ben do you have any comments . If there are no further comments can this be merged also ? As Benoit mentioned earlier, the addition of new pdata fields will cause problems with the DT support already queued for v3.3. This series should be reworked on top of the DT support which Ben has now queued for v3.3 (my branch: for_3.3/i2c/misc) Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP/I2C - Fix timeout problem during suspend.
NeilBrown ne...@suse.de writes: On Wed, 04 Jan 2012 14:19:48 -0800 Kevin Hilman khil...@ti.com wrote: +Felipe NeilBrown ne...@suse.de writes: On a board with OMAP3 processor and TWL4030 Power management, we need to talk to the TWL4030 during late suspend but cannot because the I2C interrupt is disabled (as late suspend disables interrupt). I'm not convinced this is the right solution to this problem. IMO, this problem is caused by the MUSB driver being broken for suspend/resume. I've reported this problem (and an RFC/PATCH) already[1], but I don't think the driver has been fixed. Can you try my patch[1] to see if it fixes your problem as well? Kevin [1] http://marc.info/?l=linux-omapm=132252827112721w=2 Yes... and no. I reverted my patch to confirm that the timeout messages come back which they did. I then applied your patch and the suspend was nice and smooth again with no timeouts. So that is good. However immediately after I wake it up I get: [ 109.193054] Powerdomain (core_pwrdm) didn't enter target state 1 [ 109.199310] Could not enter target state in pm_suspend whereas with my patch in place I get: [ 123.666046] Successfully put all powerdomains to target state Following this hint I looked into current draw. With my patch I get a suspend-time current draw of 60-80mA (which is still too high..). With your patch in place I get 120mA or more (about 4 tests, definitely at least 30mA difference). I measure this by checking the 'charge_now' reported by the bq27000 in the battery. This is without the usb cable plugged in. Strange... without the usb cable plugged in, runtime PM should kick in and suspend the device long before system suspend happens so that there shouldn't be anything for the MUSB driver to do during system suspend. With usb plugged in your version has the positive effect that charging continues while in suspend while with mine it doesn't. But I don't think that justifies the extra current drain :-) The reason I implemented it that way was because I understood that with a cable plugged in, you cant reliabaly suspend (or resume) MUSB due to HW bugs/limitations. Hopefully Felipe can chime in here to clarify. So I'll be sticking with my patch for now. My next problem I need to resolve relates to i2c and the charger as well. When entering suspend the various twl4030 interrupts are disabled by not masked. This means they can still fire but are not handled until after suspend. This effectively blocks other interrupts from the twl4030 that I actually want (like the RTC alarm). I think there are at least 3 or 4 bugs in here making it rather hard to sort out. However I think I will want to mask interrupt sources when they are disabled. To mask the interrupts I need to talk to the twl4030 over i2c. And this means that I need the i2c interrupt to still be working. So I feel that my patch might be more generally useful. However I confess that there is a lot here that I don't completely understand, and I might have a different opinion tomorrow. I think your patch is probably useful also, but would like to see the MUSB driver issues better clarified and fixed as well. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] I2C: OMAP: misc. updates for v3.3
Hi Ben, Here's a few OMAP I2C updates for the v3.3 merge window. Thanks, Kevin The following changes since commit 3f6b2a8bd6e4ff43269d89066a9fe06a0e5ba961: I2C: OMAP: correct SYSC register offset for OMAP4 (2011-12-13 11:35:56 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.3/i2c/misc Benoit Cousson (1): i2c: OMAP: Add DT support for i2c controller Jan Weitzel (1): I2C: OMAP: NACK without STP Documentation/devicetree/bindings/i2c/omap-i2c.txt | 30 ++ drivers/i2c/busses/i2c-omap.c | 108 +--- 2 files changed, 99 insertions(+), 39 deletions(-) create mode 100644 Documentation/devicetree/bindings/i2c/omap-i2c.txt -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP/I2C - Fix timeout problem during suspend.
+Felipe NeilBrown ne...@suse.de writes: On a board with OMAP3 processor and TWL4030 Power management, we need to talk to the TWL4030 during late suspend but cannot because the I2C interrupt is disabled (as late suspend disables interrupt). I'm not convinced this is the right solution to this problem. IMO, this problem is caused by the MUSB driver being broken for suspend/resume. I've reported this problem (and an RFC/PATCH) already[1], but I don't think the driver has been fixed. Can you try my patch[1] to see if it fixes your problem as well? Kevin [1] http://marc.info/?l=linux-omapm=132252827112721w=2 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] I2C: OMAP fixes for v3.2-rc
Jean, Could you pull the following I2C fixes for the OMAP platform for v3.2? The first one belongs in stable (change log already Cc's sta...@vger.kernel.org) Since this is an embedded platform, I have asked Ben Dooks a few times but gotten no response, so am hoping you can queue these before it's too late for v3.2-rc. Thanks, Kevin The following changes since commit 5611cc4572e889b62a7b4c72a413536bf6a9c416: Linux 3.2-rc4 (2011-12-01 14:56:01 -0800) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.2/fixes/i2c Alexander Aring (1): I2C: OMAP: correct SYSC register offset for OMAP4 Shubhrajyoti D (1): I2C: OMAP: fix FIFO usage for OMAP4 drivers/i2c/busses/i2c-omap.c | 13 +++-- 1 files changed, 7 insertions(+), 6 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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_omap: correct OMAP_I2C_SYSC_REG offset
Shubhrajyoti shubhrajy...@ti.com writes: On Friday 09 December 2011 01:01 AM, Jon Hunter wrote: Hi Kevin, On 12/8/2011 12:12, Kevin Hilman wrote: Alexander Aringa.ar...@phytec.de writes: Correct OMAP_I2C_SYSC_REG offset in omap4 register map. Offset 0x20 is reserved and OMAP_I2C_SYSC_REG has 0x10 as offset. Signed-off-by: Alexander Aringa.ar...@phytec.de Thanks for the patch! A different approach to fix this is being done by Shubhrajyoti[1] as part of his reset series. However, I think we should probably apply this patch for v3.2 and also send to the stable kernel for v3.0 v3.1. Shubhrajyoti/Jon, any objections to me queuing this fix for v3.2 (and stable.) It would just mean rebasing your other fixes and cleanup series on top of this. I have no issue with that and I am in favour of getting the fix in now. I too have no issues. OK, I've added this to my for_3.2/fixes/i2c branch. Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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_omap: correct OMAP_I2C_SYSC_REG offset
Ben, [...] Shubhrajyoti/Jon, any objections to me queuing this fix for v3.2 (and stable.) It would just mean rebasing your other fixes and cleanup series on top of this. I have no issue with that and I am in favour of getting the fix in now. I too have no issues. I've queued this fix in my i2c fixes branch[1], and added a Cc: sta...@kernel.org so it makes it into stable as well. Note, This branch also includes the other fix I sent a couple days ago[2], so pulling this branch will get you all the OMAP I2C fixes for v3.2. Thanks, Kevin [1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.2/fixes/i2c [2] Subject: [PATCH] I2C: OMAP: fix FIFO usage for OMAP4. -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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_omap: correct OMAP_I2C_SYSC_REG offset
Alexander Aring a.ar...@phytec.de writes:
Correct OMAP_I2C_SYSC_REG offset in omap4 register map.
Offset 0x20 is reserved and OMAP_I2C_SYSC_REG has 0x10 as offset.
Signed-off-by: Alexander Aring a.ar...@phytec.de
Thanks for the patch!
A different approach to fix this is being done by Shubhrajyoti[1] as
part of his reset series.
However, I think we should probably apply this patch for v3.2 and also
send to the stable kernel for v3.0 v3.1.
Shubhrajyoti/Jon, any objections to me queuing this fix for v3.2 (and
stable.) It would just mean rebasing your other fixes and cleanup
series on top of this.
Thanks,
Kevin
[1] http://marc.info/?l=linux-omapm=132281779526640w=2
---
drivers/i2c/busses/i2c-omap.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a43d002..fc3bb37 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -235,7 +235,7 @@ static const u8 reg_map_ip_v2[] = {
[OMAP_I2C_BUF_REG] = 0x94,
[OMAP_I2C_CNT_REG] = 0x98,
[OMAP_I2C_DATA_REG] = 0x9c,
- [OMAP_I2C_SYSC_REG] = 0x20,
+ [OMAP_I2C_SYSC_REG] = 0x10,
[OMAP_I2C_CON_REG] = 0xa4,
[OMAP_I2C_OA_REG] = 0xa8,
[OMAP_I2C_SA_REG] = 0xac,
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] I2C: OMAP: fix FIFO usage for OMAP4
From: Shubhrajyoti D shubhrajy...@ti.com
Currently the fifo depth is set to zero for OMAP4 which disables
the FIFO usage. This patch enables the FIFO usage for I2C transactions
on OMAP4 also.
Tested on omap4430 and 3430.
Tested-and-Reported-by: Nishanth Menon n...@ti.com
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
Signed-off-by: Kevin Hilman khil...@ti.com
---
Ben, can you queue this fix for v3.2-rc?
Patch applies to v3.2-rc4.
drivers/i2c/busses/i2c-omap.c | 11 ++-
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a43d002..fa23faa 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1047,13 +1047,14 @@ omap_i2c_probe(struct platform_device *pdev)
* size. This is to ensure that we can handle the status on int
* call back latencies.
*/
- if (dev-rev = OMAP_I2C_REV_ON_3530_4430) {
- dev-fifo_size = 0;
+
+ dev-fifo_size = (dev-fifo_size / 2);
+
+ if (dev-rev = OMAP_I2C_REV_ON_3530_4430)
dev-b_hw = 0; /* Disable hardware fixes */
- } else {
- dev-fifo_size = (dev-fifo_size / 2);
+ else
dev-b_hw = 1; /* Enable hardware fixes */
- }
+
/* calculate wakeup latency constraint for MPU */
if (dev-set_mpu_wkup_lat != NULL)
dev-latency = (100 * dev-fifo_size) /
--
1.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] OMAP: I2C: Fix the interrupt clearing in OMAP4
Shubhrajyoti D shubhrajy...@ti.com writes:
On OMAP4 we were writing 1 to IRQENABLE_CLR which cleared only
the arbitration lost interrupt however for other ips (not
OMAP_I2C_IP_VERSION_2)
we clear all the interrupts at idle. The patch intends to fix the same by
writing 0 to the
IE register.
Please also explain why you're using a different register to clear interrupts.
IMO, you don't need to explain how it works on other IPs.
The changelog should just explain what's broken in the handling of the
v2 interrupts (only arbitration lost interrupt is cleared) and how
you're fixing it (writing zero to IE REG, which clears all interrupts.)
Signed-off-by: Vikram Pandita vikram.pand...@ti.com
Has Vikram really signed off on this? This is a different
implementation from his original version. Unless he is on the delivery
path, there should not be a signed-off from him. Instead, as I've
suggested already, I'd like to see a comment in the changelog mentioning
that this patch is based on an original patch from Vikram, and then
ideally a reply from Vikram on the list with his Acked-by or Reviewed-by.
Kevin
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
---
drivers/i2c/busses/i2c-omap.c |6 ++
1 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index a43d002..abadc13 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -297,10 +297,8 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
pdata = dev-dev-platform_data;
dev-iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
- if (pdata-rev == OMAP_I2C_IP_VERSION_2)
- omap_i2c_write_reg(dev, OMAP_I2C_IP_V2_IRQENABLE_CLR, 1);
- else
- omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
+
+ omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);
if (dev-rev OMAP_I2C_OMAP1_REV_2) {
iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG); /* Read clears */
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] OMAP: I2C: Fix the interrupt clearing in OMAP4
Shubhrajyoti D shubhrajy...@ti.com writes: For OMAP4 Interrupt enable register is a legacy register. I don't see anything in the docs mentioning this is legacy. In fact, that register is used extensivly throughout the driver, even for OMAP4. I think the CLR/SET registers were added to aid atomically setting/clearing specific interrupts, but when disabling all, I don't see why I2C_IE cannot be used. For that reason, any reason why the 4430-specific check cannot simply be removed to fix this interrupt clearing bug? To clear the interrupts we were writing 0 to it. This patch still writes 0 to it, so I'm not seeing the point of this comment. However on OMAP4 we were writing 1 to IRQENABLE_CLR which clears only the arbitration lost interrupt. The patch intends to fix the same by writing 1 to all the bits. This is the bug, and should come first in the changelog so readers know what the problem is. Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com I believe this patch was originally from a fix by Vikram Pandita. Even if you've now changed the implementation, please credit the original author (and Cc them) in the changelog. It's common practice (and common courtesy) to say something like Based on an a patch originally from Author email. Thanks. Also, this patch doesn't apply to mainline or linux-omap master. Can you please update? Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c 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: OMAP: fix the clearing of interrupts
Shubhrajyoti D shubhrajy...@ti.com writes: The register IRQENABLE_CLR is a bit map of interrupt events. All the bits have to be cleared to clear the interrupts. Signed-off-by: Vikram Pandita vikram.pand...@ti.com Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com --- drivers/i2c/busses/i2c-omap.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 2dfb631..c7c6bb2 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -309,7 +309,7 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) dev-iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG); if (dev-rev = OMAP_I2C_REV_ON_4430) - omap_i2c_write_reg(dev, OMAP_I2C_IRQENABLE_CLR, 1); + omap_i2c_write_reg(dev, OMAP_I2C_IRQENABLE_CLR, 0x6FFF); While fixing, please #define this mask, and add a comment to why all the bits set in the mask. Thanks, Kevin else omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0); -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] omap4: i2c: add post idle reset registers flag
Shubhrajyoti shubhrajy...@ti.com writes: On Tuesday 08 November 2011 03:01 AM, David Anders wrote: omap44xx i2c devices need to have the registers reset post idle similar to omap3xxx devices. this adds the additional flag for OMAP_I2C_FLAG_RESET_REGS_POSTIDLE to the omap44xx i2c_dev_attr. Hello Anders a similar patch is already posted http://www.spinics.net/lists/linux-i2c/msg05913.html Benoit/Paul, Can one of you queue the original hwmod data fix from Shubhrajyoti (link above) for v3.2-rc1? Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP4: I2C: Enable FIFO usage for OMAP4
+Ben Dooks Shubhrajyoti D shubhrajy...@ti.com writes: Currently the fifo depth is set to zero for OMAP4 which disables the FIFO usage. This patch enables the FIFO usage for I2C transactions on OMAP4 also. Tested on omap4430 and 3430. Tested-and-Reported-by: Nishanth Menon n...@ti.com Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com Acked-by: Kevin Hilman khil...@ti.com Ben, please queue as a fix for the v3.2-rc. It applies cleanly to v3.2-rc1. If you prefer a branch: https://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.2/fixes/i2c Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: 10-bit address support in i2c-omap and i2c-davinci
Adding linux-omap and linux-davinci lists Jean Delvare kh...@linux-fr.org writes: Both bus drivers i2c-omap and i2c-davinci apparently handle 10-bit addresses: (i2c-omap.c) if (msg-flags I2C_M_TEN) w |= OMAP_I2C_CON_XA; (i2c-davinci.c) /* if the slave address is ten bit address, enable XA bit */ if (msg-flags I2C_M_TEN) flag |= DAVINCI_I2C_MDR_XA; However neither driver declares functionality flag I2C_FUNC_10BIT_ADDR, so chip drivers would normally refuse to bind to these buses. If 10-bit address support is incomplete or broken then it should be removed completely. If it works then these drivers should declare so by adding I2C_FUNC_10BIT_ADDR to the functionality flags they return. -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] OMAP4: I2C: Enable FIFO usage for OMAP4
Shubhrajyoti shubhrajy...@ti.com writes: On Thursday 14 July 2011 12:50 AM, Ben Dooks wrote: On Tue, Jul 05, 2011 at 05:01:01PM -0700, Kevin Hilman wrote: Shubhrajyoti D shubhrajy...@ti.com writes: Currently the fifo depth is set to zero for OMAP4 which disables the FIFO usage. This patch enables the FIFO usage for I2C transactions on OMAP4 also. Tested on omap4430 and 3430. Reported-By: Nishanth Menon n...@ti.com Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com --- Rebased on top of the series by Andy Green http://www.spinics.net/lists/linux-i2c/msg05632.html Thanks. This is v3.1 material, but would be nice to see a couple tested-by or acked-by tags from folks that are more actively using the I2C driver before merging Kevin guess you'll be picking these up? Ben Could this patch be lined up as well? Please update this patch against my for_3.2/i2c-cleanup branch, and add the Tested-by from Nishanth Menon. Then I will push to get this in as a fix for v3.2-rc. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] I2C: OMAP: misc. cleanup for v3.2
Ben Dooks ben-...@fluff.org writes: On Mon, Sep 26, 2011 at 03:30:50PM -0700, Kevin Hilman wrote: ping On 09/06/2011 03:31 PM, Kevin Hilman wrote: Hi Ben, On 08/23/2011 05:10 PM, Kevin Hilman wrote: Ben, Here's one more I2C cleanup series for v3.2. It applies on top of my for_3.2/i2c-fixes branch just submitted. Please pull into your tree for linux-next. I see you pulled the other two, can you pull this one as well? I've tried, but it seems to note that everything is up to date. Is this a suitable branch to pull onto latest so I can reset my next tree? Yes. The i2c-cleanup branch[1] is based on top of the previous two (i2c-andy and i2c-fixes) so if you reset your next-i2c branch and just pull i2c-cleanup, you'll get all three. Kevin [1] git://github.com/khilman/linux-omap-pm.git for_3.2/i2c-cleanup -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] I2C: OMAP: misc. cleanup for v3.2
ping On 09/06/2011 03:31 PM, Kevin Hilman wrote: Hi Ben, On 08/23/2011 05:10 PM, Kevin Hilman wrote: Ben, Here's one more I2C cleanup series for v3.2. It applies on top of my for_3.2/i2c-fixes branch just submitted. Please pull into your tree for linux-next. I see you pulled the other two, can you pull this one as well? Since master.kernel.org is still down, I pushed a (backup) copy of these branches to my gitorious repo[1]. It should pull cleanly into what you've already pulled since this branch is based on my for_3.2/i2c-fixes which you've already pulled. Thanks, Kevin [1] git://gitorious.org/khilman/linux-omap-pm.git -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] I2C: OMAP: misc. cleanup for v3.2
Hi Ben, On 08/23/2011 05:10 PM, Kevin Hilman wrote: Ben, Here's one more I2C cleanup series for v3.2. It applies on top of my for_3.2/i2c-fixes branch just submitted. Please pull into your tree for linux-next. I see you pulled the other two, can you pull this one as well? Since master.kernel.org is still down, I pushed a (backup) copy of these branches to my gitorious repo[1]. It should pull cleanly into what you've already pulled since this branch is based on my for_3.2/i2c-fixes which you've already pulled. Thanks, Kevin [1] git://gitorious.org/khilman/linux-omap-pm.git -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6 1/3] OMAP: I2C: Reset support
Shubhrajyoti D shubhrajy...@ti.com writes:
Under some error conditions the i2c driver may do a reset.
Adding a reset field and support in the device-specific code.
Signed-off-by: Shubhrajyoti D shubhrajy...@ti.com
Needs update/rebase to apply against my for_3.2/i2c-cleanup branch...
[...]
diff --git a/include/linux/i2c-omap.h b/include/linux/i2c-omap.h
index 98ae49b..8aa91b6 100644
--- a/include/linux/i2c-omap.h
+++ b/include/linux/i2c-omap.h
@@ -38,6 +38,7 @@ struct omap_i2c_bus_platform_data {
int (*device_enable) (struct platform_device *pdev);
int (*device_shutdown) (struct platform_device *pdev);
int (*device_idle) (struct platform_device *pdev);
The above functions no longer exist in the platform_data struct.
+ int (*device_reset) (struct device *dev);
};
#endif
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] OMAP4: hwmod data: I2C: add flag for context restore
Shubhrajyoti shubhrajy...@ti.com writes:
On Sunday 17 July 2011 06:13 PM, Shubhrajyoti D wrote:
Restore of context is not done for OMAP4. This patch
adds the OMAP_I2C_FLAG_RESET_REGS_POSTIDLE in the OMAP4
hwmod data which activates the restore for OMAP4.
Currently the OMAP4 does not hit device off still the
driver may have support for it.
Could this patch be queued as well?
Paul is the maintainer for the hwmod data. It is usually helpful to
ensure the maintainer is specifically added to the To line.
Kevin
Reviewed-by: Kevin Hilmankhil...@ti.com
Signed-off-by: Shubhrajyoti Dshubhrajy...@ti.com
---
Applies on top of patches from Andy Green
http://www.spinics.net/lists/linux-i2c/msg05632.html
Tested on OMAP4430
arch/arm/mach-omap2/omap_hwmod_44xx_data.c |3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 0fe9556..5e2c748 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -2130,7 +2130,8 @@ static struct omap_hwmod_class
omap44xx_i2c_hwmod_class = {
};
static struct omap_i2c_dev_attr i2c_dev_attr = {
-.flags = OMAP_I2C_FLAG_BUS_SHIFT_NONE,
+.flags = OMAP_I2C_FLAG_BUS_SHIFT_NONE |
+OMAP_I2C_FLAG_RESET_REGS_POSTIDLE,
};
/* i2c1 */
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] I2C: OMAP: misc. fixes for v3.2
Ben Dooks ben-...@fluff.org writes: On Tue, Aug 23, 2011 at 05:07:56PM -0700, Kevin Hilman wrote: Hi Ben, Initially, these fixes were planned to be queued for v3.1-rc, but since the previous series from Andy didn't make it into v3.1, it should be queued for v3.2. This branch is based on the for_3.2/i2c-andy branch (previous pull request.) Please pull into your tree for linux-next. Thanks, Kevin The following changes since commit 48cc1edc8f6b65a9500c8f1d93db6038167da8f0: I2C: OMAP1/OMAP2+: prepend I2C IP version to probed version shown in dev_info (2011-08-23 10:51:45 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.2/i2c-fixes Kevin Hilman (1): Revert i2c-omap: fix static suspend vs. runtime suspend I've got this one, I assume it can be submitted without being based on the Andy Green series. Well, the original version posted had a bug (thanks Felipe), so you need to be sure you have the one from this branch. Actually, this one (the revert commit) should also be submitted for v3.1-rc, since this is a known bug/race in v3.1. To that end, I reworked the branches slightly. The patch above now lives in my for_3.1/i2c-fixes branch (based on v3.1-rc3.) This should be submitted for v3.1-rc. The other patch in this series is still in the v3.2/i2c-fixes branch (based on Andy's branch, and merges for_3.1/i2c-fixes.) The 2nd cleanup series (for_3.2/i2c-cleanup) was then simply rebased onto for_3.2/i2c-fixes. That should help clarify the dependences and also make it easy to get the revert in for v3.1-rc as well. Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] I2C: OMAP: misc. fixes for v3.2
Hi Ben, Initially, these fixes were planned to be queued for v3.1-rc, but since the previous series from Andy didn't make it into v3.1, it should be queued for v3.2. This branch is based on the for_3.2/i2c-andy branch (previous pull request.) Please pull into your tree for linux-next. Thanks, Kevin The following changes since commit 48cc1edc8f6b65a9500c8f1d93db6038167da8f0: I2C: OMAP1/OMAP2+: prepend I2C IP version to probed version shown in dev_info (2011-08-23 10:51:45 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.2/i2c-fixes Kevin Hilman (1): Revert i2c-omap: fix static suspend vs. runtime suspend Shubhrajyoti D (1): OMAP4: I2C: Enable the wakeup in I2C_WE drivers/i2c/busses/i2c-omap.c | 34 ++ 1 files changed, 2 insertions(+), 32 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL] I2C: OMAP: misc. cleanup for v3.2
Ben, Here's one more I2C cleanup series for v3.2. It applies on top of my for_3.2/i2c-fixes branch just submitted. Please pull into your tree for linux-next. Thanks, Kevin The following changes since commit f3cb1e11c4e54f4dc7e60f9513d43ffa0bbebc25: Revert i2c-omap: fix static suspend vs. runtime suspend (2011-08-23 10:51:58 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git for_3.2/i2c-cleanup Kevin Hilman (3): I2C: OMAP: remove unneccesary use of pdev I2C: OMAP: remove dev-idle, use usage counting provided by runtime PM I2C: OMAP: remove unused function pointers from pdata drivers/i2c/busses/i2c-omap.c | 77 +++- include/linux/i2c-omap.h |3 -- 2 files changed, 44 insertions(+), 36 deletions(-) -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert i2c-omap: fix static suspend vs. runtime suspend
Felipe Balbi ba...@ti.com writes: On Wed, Aug 03, 2011 at 10:59:39AM -0700, Kevin Hilman wrote: This reverts commit adf6e07922255937c8bfeea777d19502b4c9a2be. Remove system PM methods which can race with runtime PM methods. Also, as of v3.1, the PM domain level code for OMAP handles device power state transistions automatically for devices, so calling them from from where ? I didn't quite get this ;-) heh, looks like i got distracted before finishing the changelog. Will respin. Thanks, Kevin -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Revert i2c-omap: fix static suspend vs. runtime suspend
This reverts commit adf6e07922255937c8bfeea777d19502b4c9a2be.
Remove system PM methods which can race with runtime PM methods.
Also, as of v3.1, the PM domain level code for OMAP handles device
power state transistions automatically for devices, so drivers no
longer need to specifically call the bus/pm_domain methods themselves.
Signed-off-by: Kevin Hilman khil...@ti.com
---
v2: updated changelog to remove cliff-hanger ending
drivers/i2c/busses/i2c-omap.c | 29 -
1 files changed, 0 insertions(+), 29 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 84df53f..e854be0 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1148,41 +1148,12 @@ omap_i2c_remove(struct platform_device *pdev)
return 0;
}
-#ifdef CONFIG_SUSPEND
-static int omap_i2c_suspend(struct device *dev)
-{
- if (!pm_runtime_suspended(dev))
- if (dev-bus dev-bus-pm dev-bus-pm-runtime_suspend)
- dev-bus-pm-runtime_suspend(dev);
-
- return 0;
-}
-
-static int omap_i2c_resume(struct device *dev)
-{
- if (!pm_runtime_suspended(dev))
- if (dev-bus dev-bus-pm dev-bus-pm-runtime_resume)
- dev-bus-pm-runtime_resume(dev);
-
- return 0;
-}
-
-static struct dev_pm_ops omap_i2c_pm_ops = {
- .suspend = omap_i2c_suspend,
- .resume = omap_i2c_resume,
-};
-#define OMAP_I2C_PM_OPS (omap_i2c_pm_ops)
-#else
-#define OMAP_I2C_PM_OPS NULL
-#endif
-
static struct platform_driver omap_i2c_driver = {
.probe = omap_i2c_probe,
.remove = omap_i2c_remove,
.driver = {
.name = omap_i2c,
.owner = THIS_MODULE,
- .pm = OMAP_I2C_PM_OPS,
},
};
--
1.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] I2C: OMAP: remove dev-idle, use usage counting provided by runtime PM
Felipe Balbi ba...@ti.com writes:
Hi,
On Wed, Aug 03, 2011 at 11:09:20AM -0700, Kevin Hilman wrote:
Current usage of runtime PM is not quite correct. The actual
idle/unidle of the I2C hardware should not happen until the runtime PM
callbacks are called. Therefore, change omap_i2c_[un]idle() functions
to only be called from the runtime PM callbacks (when usage count
transitions to/from zero.)
Also, the runtime PM core does usage counting and replaces
functionality currently managed by the dev-idle flag. Remove usage
of dev-idle in favor of using runtime PM, and checking status using
pm_runtime_suspended().
Signed-off-by: Kevin Hilman khil...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 58
++--
1 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 12d0cbc..1b5325b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -194,7 +194,6 @@ struct omap_i2c_dev {
*/
u8 rev;
unsignedb_hw:1; /* bad h/w fixes */
-unsignedidle:1;
u16 iestate;/* Saved interrupt register */
u16 pscstate;
u16 scllstate;
@@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
{
struct omap_i2c_bus_platform_data *pdata;
-WARN_ON(!dev-idle);
-
pdata = dev-dev-platform_data;
-pm_runtime_get_sync(dev-dev);
-
if (pdata-flags OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev-pscstate);
@@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev-westate);
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
}
-dev-idle = 0;
/*
* Don't write to this register if the IE state is 0 as it can
@@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
struct omap_i2c_bus_platform_data *pdata;
u16 iv;
-WARN_ON(dev-idle);
-
pdata = dev-dev-platform_data;
dev-iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
} else {
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev-iestate);
-/* Flush posted write before the dev-idle store occurs */
+/* Flush posted write */
omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
}
-dev-idle = 1;
-
-pm_runtime_put_sync(dev-dev);
}
static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
int i;
int r;
-omap_i2c_unidle(dev);
+pm_runtime_get_sync(dev-dev);
r = omap_i2c_wait_for_bb(dev);
if (r 0)
@@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
omap_i2c_wait_for_bb(dev);
out:
-omap_i2c_idle(dev);
+pm_runtime_put_sync(dev-dev);
I wonder if these pm_runtime_put need to be synchronous ? Could we just
call pm_runtime_put() instead ? Ditto to all other.
Yes, will use asynchronous versions.
@@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM_RUNTIME
+static int omap_i2c_runtime_suspend(struct device *dev)
+{
+struct platform_device *pdev = to_platform_device(dev);
+struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
what happened to dev_get_drvdata(dev) ??
Yes, that would work too since:
static inline void *platform_get_drvdata(const struct platform_device *pdev)
{
return dev_get_drvdata(pdev-dev);
}
but IMO, readability is better if the driver does platform_set_drvdata()
and then the corresponding platform_get_drvdata()
+omap_i2c_idle(_dev);
+
+return 0;
+}
+
+static int omap_i2c_runtime_resume(struct device *dev)
+{
+struct platform_device *pdev = to_platform_device(dev);
+struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
ditto
+omap_i2c_unidle(_dev);
+
+return 0;
+}
+
+static struct dev_pm_ops omap_i2c_pm_ops = {
+.runtime_suspend = omap_i2c_runtime_suspend,
+.runtime_resume = omap_i2c_runtime_resume,
+};
+#define OMAP_I2C_PM_OPS (omap_i2c_pm_ops)
+#else
+#define OMAP_I2C_PM_OPS NULL
+#endif
OMAP_I2C_PM_OPS isn't used anywhere ??
doh
thanks for catching
Kevin
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/2] I2C: OMAP: remove unneccesary use of pdev
A pointer to the struct device associated with the i2c device is
already kept in the struct omap_i2c_dev, so use omap_i2c_device to
find the pointer to struct device.
Reviewed-by: Felipe Balbi ba...@ti.com
Signed-off-by: Kevin Hilman khil...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 22 +++---
1 files changed, 7 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e854be0..12d0cbc 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -267,15 +267,13 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev
*i2c_dev, int reg)
static void omap_i2c_unidle(struct omap_i2c_dev *dev)
{
- struct platform_device *pdev;
struct omap_i2c_bus_platform_data *pdata;
WARN_ON(!dev-idle);
- pdev = to_platform_device(dev-dev);
- pdata = pdev-dev.platform_data;
+ pdata = dev-dev-platform_data;
- pm_runtime_get_sync(pdev-dev);
+ pm_runtime_get_sync(dev-dev);
if (pdata-flags OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
@@ -299,14 +297,12 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
static void omap_i2c_idle(struct omap_i2c_dev *dev)
{
- struct platform_device *pdev;
struct omap_i2c_bus_platform_data *pdata;
u16 iv;
WARN_ON(dev-idle);
- pdev = to_platform_device(dev-dev);
- pdata = pdev-dev.platform_data;
+ pdata = dev-dev-platform_data;
dev-iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
if (pdata-rev == OMAP_I2C_IP_VERSION_2)
@@ -324,7 +320,7 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
}
dev-idle = 1;
- pm_runtime_put_sync(pdev-dev);
+ pm_runtime_put_sync(dev-dev);
}
static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -335,11 +331,9 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
unsigned long timeout;
unsigned long internal_clk = 0;
struct clk *fclk;
- struct platform_device *pdev;
struct omap_i2c_bus_platform_data *pdata;
- pdev = to_platform_device(dev-dev);
- pdata = pdev-dev.platform_data;
+ pdata = dev-dev-platform_data;
if (dev-rev = OMAP_I2C_OMAP1_REV_2) {
/* Disable I2C controller before soft reset */
@@ -821,11 +815,9 @@ omap_i2c_isr(int this_irq, void *dev_id)
u16 bits;
u16 stat, w;
int err, count = 0;
- struct platform_device *pdev;
struct omap_i2c_bus_platform_data *pdata;
- pdev = to_platform_device(dev-dev);
- pdata = pdev-dev.platform_data;
+ pdata = dev-dev-platform_data;
if (dev-idle)
return IRQ_NONE;
@@ -1047,7 +1039,7 @@ omap_i2c_probe(struct platform_device *pdev)
else
dev-regs = (u8 *)reg_map_ip_v1;
- pm_runtime_enable(pdev-dev);
+ pm_runtime_enable(dev-dev);
omap_i2c_unidle(dev);
dev-rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) 0xff;
--
1.7.6
--
To unsubscribe from this list: send the line unsubscribe linux-i2c in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] I2C: OMAP: remove dev-idle, use usage counting provided by runtime PM
Current usage of runtime PM is not quite correct. The actual
idle/unidle of the I2C hardware should not happen until the runtime PM
callbacks are called. Therefore, change omap_i2c_[un]idle() functions
to only be called from the runtime PM callbacks (when usage count
transitions to/from zero.)
Also, the runtime PM core does usage counting and replaces
functionality currently managed by the dev-idle flag. Remove usage
of dev-idle in favor of using runtime PM, and checking status using
pm_runtime_suspended().
Signed-off-by: Kevin Hilman khil...@ti.com
---
drivers/i2c/busses/i2c-omap.c | 59 +++--
1 files changed, 39 insertions(+), 20 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 12d0cbc..1c75d13 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -194,7 +194,6 @@ struct omap_i2c_dev {
*/
u8 rev;
unsignedb_hw:1; /* bad h/w fixes */
- unsignedidle:1;
u16 iestate;/* Saved interrupt register */
u16 pscstate;
u16 scllstate;
@@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
{
struct omap_i2c_bus_platform_data *pdata;
- WARN_ON(!dev-idle);
-
pdata = dev-dev-platform_data;
- pm_runtime_get_sync(dev-dev);
-
if (pdata-flags OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev-pscstate);
@@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev-westate);
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
}
- dev-idle = 0;
/*
* Don't write to this register if the IE state is 0 as it can
@@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
struct omap_i2c_bus_platform_data *pdata;
u16 iv;
- WARN_ON(dev-idle);
-
pdata = dev-dev-platform_data;
dev-iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
} else {
omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev-iestate);
- /* Flush posted write before the dev-idle store occurs */
+ /* Flush posted write */
omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
}
- dev-idle = 1;
-
- pm_runtime_put_sync(dev-dev);
}
static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
int i;
int r;
- omap_i2c_unidle(dev);
+ pm_runtime_get_sync(dev-dev);
r = omap_i2c_wait_for_bb(dev);
if (r 0)
@@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg
msgs[], int num)
omap_i2c_wait_for_bb(dev);
out:
- omap_i2c_idle(dev);
+ pm_runtime_put(dev-dev);
return r;
}
@@ -727,7 +716,7 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
struct omap_i2c_dev *dev = dev_id;
u16 iv, w;
- if (dev-idle)
+ if (pm_runtime_suspended(dev-dev))
return IRQ_NONE;
iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
@@ -819,7 +808,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
pdata = dev-dev-platform_data;
- if (dev-idle)
+ if (pm_runtime_suspended(dev-dev))
return IRQ_NONE;
bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -1021,7 +1010,6 @@ omap_i2c_probe(struct platform_device *pdev)
}
dev-speed = speed;
- dev-idle = 1;
dev-dev = pdev-dev;
dev-irq = irq-start;
dev-base = ioremap(mem-start, resource_size(mem));
@@ -1040,7 +1028,7 @@ omap_i2c_probe(struct platform_device *pdev)
dev-regs = (u8 *)reg_map_ip_v1;
pm_runtime_enable(dev-dev);
- omap_i2c_unidle(dev);
+ pm_runtime_get_sync(dev-dev);
dev-rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) 0xff;
@@ -1087,7 +1075,7 @@ omap_i2c_probe(struct platform_device *pdev)
dev_info(dev-dev, bus %d rev%d.%d.%d at %d kHz\n, pdev-id,
pdata-rev, dev-rev 4, dev-rev 0xf, dev-speed);
- omap_i2c_idle(dev);
+ pm_runtime_put(dev-dev);
adap = dev-adapter;
i2c_set_adapdata(adap, dev);
@@ -,7 +1099,7 @@ err_free_irq:
free_irq(dev-irq, dev);
err_unuse_clocks:
omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
- omap_i2c_idle(dev);
+ pm_runtime_put(dev-dev);
iounmap(dev-base);
err_free_mem:
platform_set_drvdata(pdev, NULL);
@@ -1140,12 +1128,43
[PATCH v2 0/2] I2C: OMAP: misc. PM-related cleanups
Do some PM-related cleanup on this driver and make the runtime PM usage more standard. Series applies on Andy's I2C cleanup series (for_3.1/i2c-andy-2 branch in my tree[1]) plus the misc. i2c fixes queued for v3.1-rc (for_3.1/i2c-fixes branch) and are available in the for_3.2/i2c-cleanup branch of my tree. [1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git Kevin Hilman (2): I2C: OMAP: remove unneccesary use of pdev I2C: OMAP: remove dev-idle, use usage counting provided by runtime PM drivers/i2c/busses/i2c-omap.c | 77 +++- 1 files changed, 44 insertions(+), 33 deletions(-) -- 1.7.6 -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
