[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Thu, Apr 12, 2012 at 4:26 AM, Chris Wilson wrote: > On Thu, 12 Apr 2012 02:17:46 +0800, Daniel Kurtz > wrote: >> On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson >> wrote: >> > The last major item on the wishlist is solving how to drive the SDVO i2c >> > over gmbus. I think it is just a matter of massaging in the channel >> > switch as msg[0]. >> >> I noticed that random other i2c adapter, but haven't looked at it to >> know why it is so different than the others. ?What is it? > > Every command sequence over SDVO is preceded by an instruction to the > controller as to which function to send the command to. Originally I > tried to reuse the GPIO method of simply sending that switch command > immediately followed by the real message, but I never succeeded in > setting up the SDVO devices in that manner. I suspect what is actually > required is to rebuild the message to include the preceeding bus switch. > > However, if you don't have any SDVO devices to experiment with we can > find somebody who does. Sorry, I don't have any SDVO hardware to play with. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Thu, Apr 12, 2012 at 4:26 AM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Thu, 12 Apr 2012 02:17:46 +0800, Daniel Kurtz djku...@chromium.org wrote: On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson ch...@chris-wilson.co.uk wrote: The last major item on the wishlist is solving how to drive the SDVO i2c over gmbus. I think it is just a matter of massaging in the channel switch as msg[0]. I noticed that random other i2c adapter, but haven't looked at it to know why it is so different than the others. What is it? Every command sequence over SDVO is preceded by an instruction to the controller as to which function to send the command to. Originally I tried to reuse the GPIO method of simply sending that switch command immediately followed by the real message, but I never succeeded in setting up the SDVO devices in that manner. I suspect what is actually required is to rebuild the message to include the preceeding bus switch. However, if you don't have any SDVO devices to experiment with we can find somebody who does. Sorry, I don't have any SDVO hardware to play with. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson wrote: > On Tue, 10 Apr 2012 17:03:04 +0200, Daniel Vetter wrote: >> On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: >> > On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter wrote: >> > > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: >> > >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: >> > >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c >> > >> > transaction) during a DATA or WAIT phase. ?In other words, the >> > >> > controller rejects a STOP requested as part of the first transaction >> > >> > in a >> > >> > sequence. >> > >> > >> > >> > Thus, for the first transaction we must always use a WAIT cycle, >> > >> > detect >> > >> > when the device has finished (and is in a WAIT phase), and then either >> > >> > start the next transaction, or, if there are no more transactions, >> > >> > generate a STOP cycle. >> > >> > >> > >> > Note: Theoretically, the last transaction of a multi-transaction >> > >> > sequence >> > >> > could initiate a STOP cycle. ?However, this slight optimization is >> > >> > left >> > >> > for another patch. ?We return -ETIMEDOUT if the hardware doesn't >> > >> > deactivate after the STOP cycle. >> > >> > >> > >> > Signed-off-by: Daniel Kurtz >> > >> >> > >> I've re-read gmbus register spec and STOP seems to be allowed even in >> > >> the >> > >> first cycle. Does this patch solve an issue for you? If not, I prefer we >> > >> just drop it. >> > >> > STOP does not work in the first cycle, hence the patch. >> >> Ok, I've picked this patch up and extended the comment a bit to that >> effect. Just to avoid anyone else trying to 'fix' things because bspec >> sounds like it should work. >> >> I've also picked up the other patches safe for the last one, thanks a lot >> for digging through the gmbus code and fixing it all up. >> >> Now can I volunteer you for a (hopefully) last set of gmbus patches? >> Afaics there are a few small things left to fix: >> - zero-length reads can blow up the kernel, like zero-length writes could. >> ? See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 >> - Chris Wilson suggested on irc that we should wait for HW_READY even for >> ? zero-length writes (and also reads), currently we don't. >> - atm the debug output is too noisy. I think we can leave the fallback to >> ? gpio bitbanging at info (or maybe error) level, but all the other >> ? messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit >> ? when userspace tries to probe the i2c with nothing connected or if the >> ? driver code tries to do the same. See: >> ? https://bugs.freedesktop.org/show_bug.cgi?id=48248 >> >> Chris, anything you want to add to the wishlist? > > The last major item on the wishlist is solving how to drive the SDVO i2c > over gmbus. I think it is just a matter of massaging in the channel > switch as msg[0]. I noticed that random other i2c adapter, but haven't looked at it to know why it is so different than the others. What is it? > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter wrote: > On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: >> On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter wrote: >> > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: >> >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: >> >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c >> >> > transaction) during a DATA or WAIT phase. ?In other words, the >> >> > controller rejects a STOP requested as part of the first transaction in >> >> > a >> >> > sequence. >> >> > >> >> > Thus, for the first transaction we must always use a WAIT cycle, detect >> >> > when the device has finished (and is in a WAIT phase), and then either >> >> > start the next transaction, or, if there are no more transactions, >> >> > generate a STOP cycle. >> >> > >> >> > Note: Theoretically, the last transaction of a multi-transaction >> >> > sequence >> >> > could initiate a STOP cycle. ?However, this slight optimization is left >> >> > for another patch. ?We return -ETIMEDOUT if the hardware doesn't >> >> > deactivate after the STOP cycle. >> >> > >> >> > Signed-off-by: Daniel Kurtz >> >> >> >> I've re-read gmbus register spec and STOP seems to be allowed even in the >> >> first cycle. Does this patch solve an issue for you? If not, I prefer we >> >> just drop it. >> >> STOP does not work in the first cycle, hence the patch. > > Ok, I've picked this patch up and extended the comment a bit to that > effect. Just to avoid anyone else trying to 'fix' things because bspec > sounds like it should work. > > I've also picked up the other patches safe for the last one, thanks a lot > for digging through the gmbus code and fixing it all up. > > Now can I volunteer you for a (hopefully) last set of gmbus patches? > Afaics there are a few small things left to fix: > - zero-length reads can blow up the kernel, like zero-length writes could. > ?See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 Got it. Will Fix. > - Chris Wilson suggested on irc that we should wait for HW_READY even for > ?zero-length writes (and also reads), currently we don't. I don't think so. We just need to wait for (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE). Why would we wait for HW_READY, too? > - atm the debug output is too noisy. I think we can leave the fallback to > ?gpio bitbanging at info (or maybe error) level, but all the other > ?messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit > ?when userspace tries to probe the i2c with nothing connected or if the > ?driver code tries to do the same. See: > ?https://bugs.freedesktop.org/show_bug.cgi?id=48248 OK... we can change the logging level. However, the log in the bug to which you link seems to indicate a more serious issue in this case. It says to me that something on his system is trying to talk to the disabled dpc i2c port 5 times every 10 seconds. Each time it fails due with a time out, and each timeout takes 50ms. I would argue that the INFO message here is pointing out that the hotplug code might want to check the corresponding PORT_ENABLED bit before attempting a read over a particular DP/HDMI gmbus port. Perhaps I am mistaken, but if there was really nothing on the bus, shouldn't that be a NAK, not a timeout? > > Chris, anything you want to add to the wishlist? > > Thanks, Daniel > -- > Daniel Vetter > Mail: daniel at ffwll.ch > Mobile: +41 (0)79 365 57 48
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Thu, Apr 12, 2012 at 02:16:45AM +0800, Daniel Kurtz wrote: > On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter wrote: > > - atm the debug output is too noisy. I think we can leave the fallback to > > ?gpio bitbanging at info (or maybe error) level, but all the other > > ?messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit > > ?when userspace tries to probe the i2c with nothing connected or if the > > ?driver code tries to do the same. See: > > ?https://bugs.freedesktop.org/show_bug.cgi?id=48248 > > OK... we can change the logging level. > However, the log in the bug to which you link seems to indicate a more > serious issue in this case. It says to me that something on his > system is trying to talk to the disabled dpc i2c port 5 times every 10 > seconds. Each time it fails due with a time out, and each timeout > takes 50ms. I would argue that the INFO message here is pointing out > that the hotplug code might want to check the corresponding > PORT_ENABLED bit before attempting a read over a particular DP/HDMI > gmbus port. Perhaps I am mistaken, but if there was really nothing on > the bus, shouldn't that be a NAK, not a timeout? The issue is that there's no hotplug, so we run a polling loop which checks every 10s whether anything is connected. Part of that is trying to read an edid. I dunno exactly why we don't get a NAK but a timeout. -Daniel -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Thu, 12 Apr 2012 02:16:45 +0800, Daniel Kurtz wrote: > On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter wrote: > > - Chris Wilson suggested on irc that we should wait for HW_READY even for > > ??zero-length writes (and also reads), currently we don't. > > I don't think so. We just need to wait for (GMBUS_SATOER | > GMBUS_HW_WAIT_PHASE). > Why would we wait for HW_READY, too? Just paranoia when looking at the read/write sequences and wondering how safe they were with 0-length read/writes. No real reason to suspect that the code is incorrect in any way. -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Thu, 12 Apr 2012 02:17:46 +0800, Daniel Kurtz wrote: > On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson > wrote: > > The last major item on the wishlist is solving how to drive the SDVO i2c > > over gmbus. I think it is just a matter of massaging in the channel > > switch as msg[0]. > > I noticed that random other i2c adapter, but haven't looked at it to > know why it is so different than the others. What is it? Every command sequence over SDVO is preceded by an instruction to the controller as to which function to send the command to. Originally I tried to reuse the GPIO method of simply sending that switch command immediately followed by the real message, but I never succeeded in setting up the SDVO devices in that manner. I suspect what is actually required is to rebuild the message to include the preceeding bus switch. However, if you don't have any SDVO devices to experiment with we can find somebody who does. -Chris -- Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: The i915 is only able to generate a STOP cycle (i.e. finalize an i2c transaction) during a DATA or WAIT phase. In other words, the controller rejects a STOP requested as part of the first transaction in a sequence. Thus, for the first transaction we must always use a WAIT cycle, detect when the device has finished (and is in a WAIT phase), and then either start the next transaction, or, if there are no more transactions, generate a STOP cycle. Note: Theoretically, the last transaction of a multi-transaction sequence could initiate a STOP cycle. However, this slight optimization is left for another patch. We return -ETIMEDOUT if the hardware doesn't deactivate after the STOP cycle. Signed-off-by: Daniel Kurtz djku...@chromium.org I've re-read gmbus register spec and STOP seems to be allowed even in the first cycle. Does this patch solve an issue for you? If not, I prefer we just drop it. STOP does not work in the first cycle, hence the patch. Ok, I've picked this patch up and extended the comment a bit to that effect. Just to avoid anyone else trying to 'fix' things because bspec sounds like it should work. I've also picked up the other patches safe for the last one, thanks a lot for digging through the gmbus code and fixing it all up. Now can I volunteer you for a (hopefully) last set of gmbus patches? Afaics there are a few small things left to fix: - zero-length reads can blow up the kernel, like zero-length writes could. See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 Got it. Will Fix. - Chris Wilson suggested on irc that we should wait for HW_READY even for zero-length writes (and also reads), currently we don't. I don't think so. We just need to wait for (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE). Why would we wait for HW_READY, too? - atm the debug output is too noisy. I think we can leave the fallback to gpio bitbanging at info (or maybe error) level, but all the other messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit when userspace tries to probe the i2c with nothing connected or if the driver code tries to do the same. See: https://bugs.freedesktop.org/show_bug.cgi?id=48248 OK... we can change the logging level. However, the log in the bug to which you link seems to indicate a more serious issue in this case. It says to me that something on his system is trying to talk to the disabled dpc i2c port 5 times every 10 seconds. Each time it fails due with a time out, and each timeout takes 50ms. I would argue that the INFO message here is pointing out that the hotplug code might want to check the corresponding PORT_ENABLED bit before attempting a read over a particular DP/HDMI gmbus port. Perhaps I am mistaken, but if there was really nothing on the bus, shouldn't that be a NAK, not a timeout? Chris, anything you want to add to the wishlist? Thanks, Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson ch...@chris-wilson.co.uk wrote: On Tue, 10 Apr 2012 17:03:04 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: The i915 is only able to generate a STOP cycle (i.e. finalize an i2c transaction) during a DATA or WAIT phase. In other words, the controller rejects a STOP requested as part of the first transaction in a sequence. Thus, for the first transaction we must always use a WAIT cycle, detect when the device has finished (and is in a WAIT phase), and then either start the next transaction, or, if there are no more transactions, generate a STOP cycle. Note: Theoretically, the last transaction of a multi-transaction sequence could initiate a STOP cycle. However, this slight optimization is left for another patch. We return -ETIMEDOUT if the hardware doesn't deactivate after the STOP cycle. Signed-off-by: Daniel Kurtz djku...@chromium.org I've re-read gmbus register spec and STOP seems to be allowed even in the first cycle. Does this patch solve an issue for you? If not, I prefer we just drop it. STOP does not work in the first cycle, hence the patch. Ok, I've picked this patch up and extended the comment a bit to that effect. Just to avoid anyone else trying to 'fix' things because bspec sounds like it should work. I've also picked up the other patches safe for the last one, thanks a lot for digging through the gmbus code and fixing it all up. Now can I volunteer you for a (hopefully) last set of gmbus patches? Afaics there are a few small things left to fix: - zero-length reads can blow up the kernel, like zero-length writes could. See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 - Chris Wilson suggested on irc that we should wait for HW_READY even for zero-length writes (and also reads), currently we don't. - atm the debug output is too noisy. I think we can leave the fallback to gpio bitbanging at info (or maybe error) level, but all the other messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit when userspace tries to probe the i2c with nothing connected or if the driver code tries to do the same. See: https://bugs.freedesktop.org/show_bug.cgi?id=48248 Chris, anything you want to add to the wishlist? The last major item on the wishlist is solving how to drive the SDVO i2c over gmbus. I think it is just a matter of massaging in the channel switch as msg[0]. I noticed that random other i2c adapter, but haven't looked at it to know why it is so different than the others. What is it? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Thu, Apr 12, 2012 at 02:16:45AM +0800, Daniel Kurtz wrote: On Tue, Apr 10, 2012 at 11:03 PM, Daniel Vetter dan...@ffwll.ch wrote: - atm the debug output is too noisy. I think we can leave the fallback to gpio bitbanging at info (or maybe error) level, but all the other messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit when userspace tries to probe the i2c with nothing connected or if the driver code tries to do the same. See: https://bugs.freedesktop.org/show_bug.cgi?id=48248 OK... we can change the logging level. However, the log in the bug to which you link seems to indicate a more serious issue in this case. It says to me that something on his system is trying to talk to the disabled dpc i2c port 5 times every 10 seconds. Each time it fails due with a time out, and each timeout takes 50ms. I would argue that the INFO message here is pointing out that the hotplug code might want to check the corresponding PORT_ENABLED bit before attempting a read over a particular DP/HDMI gmbus port. Perhaps I am mistaken, but if there was really nothing on the bus, shouldn't that be a NAK, not a timeout? The issue is that there's no hotplug, so we run a polling loop which checks every 10s whether anything is connected. Part of that is trying to read an edid. I dunno exactly why we don't get a NAK but a timeout. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Thu, 12 Apr 2012 02:17:46 +0800, Daniel Kurtz djku...@chromium.org wrote: On Wed, Apr 11, 2012 at 5:34 AM, Chris Wilson ch...@chris-wilson.co.uk wrote: The last major item on the wishlist is solving how to drive the SDVO i2c over gmbus. I think it is just a matter of massaging in the channel switch as msg[0]. I noticed that random other i2c adapter, but haven't looked at it to know why it is so different than the others. What is it? Every command sequence over SDVO is preceded by an instruction to the controller as to which function to send the command to. Originally I tried to reuse the GPIO method of simply sending that switch command immediately followed by the real message, but I never succeeded in setting up the SDVO devices in that manner. I suspect what is actually required is to rebuild the message to include the preceeding bus switch. However, if you don't have any SDVO devices to experiment with we can find somebody who does. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, 10 Apr 2012 17:03:04 +0200, Daniel Vetter wrote: > On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: > > On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter wrote: > > > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: > > >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: > > >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c > > >> > transaction) during a DATA or WAIT phase. ??In other words, the > > >> > controller rejects a STOP requested as part of the first transaction > > >> > in a > > >> > sequence. > > >> > > > >> > Thus, for the first transaction we must always use a WAIT cycle, detect > > >> > when the device has finished (and is in a WAIT phase), and then either > > >> > start the next transaction, or, if there are no more transactions, > > >> > generate a STOP cycle. > > >> > > > >> > Note: Theoretically, the last transaction of a multi-transaction > > >> > sequence > > >> > could initiate a STOP cycle. ??However, this slight optimization is > > >> > left > > >> > for another patch. ??We return -ETIMEDOUT if the hardware doesn't > > >> > deactivate after the STOP cycle. > > >> > > > >> > Signed-off-by: Daniel Kurtz > > >> > > >> I've re-read gmbus register spec and STOP seems to be allowed even in the > > >> first cycle. Does this patch solve an issue for you? If not, I prefer we > > >> just drop it. > > > > STOP does not work in the first cycle, hence the patch. > > Ok, I've picked this patch up and extended the comment a bit to that > effect. Just to avoid anyone else trying to 'fix' things because bspec > sounds like it should work. > > I've also picked up the other patches safe for the last one, thanks a lot > for digging through the gmbus code and fixing it all up. > > Now can I volunteer you for a (hopefully) last set of gmbus patches? > Afaics there are a few small things left to fix: > - zero-length reads can blow up the kernel, like zero-length writes could. > See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 > - Chris Wilson suggested on irc that we should wait for HW_READY even for > zero-length writes (and also reads), currently we don't. > - atm the debug output is too noisy. I think we can leave the fallback to > gpio bitbanging at info (or maybe error) level, but all the other > messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit > when userspace tries to probe the i2c with nothing connected or if the > driver code tries to do the same. See: > https://bugs.freedesktop.org/show_bug.cgi?id=48248 > > Chris, anything you want to add to the wishlist? The last major item on the wishlist is solving how to drive the SDVO i2c over gmbus. I think it is just a matter of massaging in the channel switch as msg[0]. -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter wrote: > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c >> > transaction) during a DATA or WAIT phase. ?In other words, the >> > controller rejects a STOP requested as part of the first transaction in a >> > sequence. >> > >> > Thus, for the first transaction we must always use a WAIT cycle, detect >> > when the device has finished (and is in a WAIT phase), and then either >> > start the next transaction, or, if there are no more transactions, >> > generate a STOP cycle. >> > >> > Note: Theoretically, the last transaction of a multi-transaction sequence >> > could initiate a STOP cycle. ?However, this slight optimization is left >> > for another patch. ?We return -ETIMEDOUT if the hardware doesn't >> > deactivate after the STOP cycle. >> > >> > Signed-off-by: Daniel Kurtz >> >> I've re-read gmbus register spec and STOP seems to be allowed even in the >> first cycle. Does this patch solve an issue for you? If not, I prefer we >> just drop it. STOP does not work in the first cycle, hence the patch. -Daniel > > Actually I'd like to keep the -ETIMEDOUT return value, so maybe we should > keeep that hunk. I've picked up the previous 3 patches of this series, the > once after this one here conflict (without this patch here). > -Daniel > -- > Daniel Vetter > Mail: daniel at ffwll.ch > Mobile: +41 (0)79 365 57 48
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: > On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter wrote: > > On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: > >> On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: > >> > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c > >> > transaction) during a DATA or WAIT phase. ?In other words, the > >> > controller rejects a STOP requested as part of the first transaction in a > >> > sequence. > >> > > >> > Thus, for the first transaction we must always use a WAIT cycle, detect > >> > when the device has finished (and is in a WAIT phase), and then either > >> > start the next transaction, or, if there are no more transactions, > >> > generate a STOP cycle. > >> > > >> > Note: Theoretically, the last transaction of a multi-transaction sequence > >> > could initiate a STOP cycle. ?However, this slight optimization is left > >> > for another patch. ?We return -ETIMEDOUT if the hardware doesn't > >> > deactivate after the STOP cycle. > >> > > >> > Signed-off-by: Daniel Kurtz > >> > >> I've re-read gmbus register spec and STOP seems to be allowed even in the > >> first cycle. Does this patch solve an issue for you? If not, I prefer we > >> just drop it. > > STOP does not work in the first cycle, hence the patch. Ok, I've picked this patch up and extended the comment a bit to that effect. Just to avoid anyone else trying to 'fix' things because bspec sounds like it should work. I've also picked up the other patches safe for the last one, thanks a lot for digging through the gmbus code and fixing it all up. Now can I volunteer you for a (hopefully) last set of gmbus patches? Afaics there are a few small things left to fix: - zero-length reads can blow up the kernel, like zero-length writes could. See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 - Chris Wilson suggested on irc that we should wait for HW_READY even for zero-length writes (and also reads), currently we don't. - atm the debug output is too noisy. I think we can leave the fallback to gpio bitbanging at info (or maybe error) level, but all the other messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit when userspace tries to probe the i2c with nothing connected or if the driver code tries to do the same. See: https://bugs.freedesktop.org/show_bug.cgi?id=48248 Chris, anything you want to add to the wishlist? Thanks, Daniel -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: > On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: > > The i915 is only able to generate a STOP cycle (i.e. finalize an i2c > > transaction) during a DATA or WAIT phase. In other words, the > > controller rejects a STOP requested as part of the first transaction in a > > sequence. > > > > Thus, for the first transaction we must always use a WAIT cycle, detect > > when the device has finished (and is in a WAIT phase), and then either > > start the next transaction, or, if there are no more transactions, > > generate a STOP cycle. > > > > Note: Theoretically, the last transaction of a multi-transaction sequence > > could initiate a STOP cycle. However, this slight optimization is left > > for another patch. We return -ETIMEDOUT if the hardware doesn't > > deactivate after the STOP cycle. > > > > Signed-off-by: Daniel Kurtz > > I've re-read gmbus register spec and STOP seems to be allowed even in the > first cycle. Does this patch solve an issue for you? If not, I prefer we > just drop it. Actually I'd like to keep the -ETIMEDOUT return value, so maybe we should keeep that hunk. I've picked up the previous 3 patches of this series, the once after this one here conflict (without this patch here). -Daniel -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
> The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
> transaction) during a DATA or WAIT phase. In other words, the
> controller rejects a STOP requested as part of the first transaction in a
> sequence.
>
> Thus, for the first transaction we must always use a WAIT cycle, detect
> when the device has finished (and is in a WAIT phase), and then either
> start the next transaction, or, if there are no more transactions,
> generate a STOP cycle.
>
> Note: Theoretically, the last transaction of a multi-transaction sequence
> could initiate a STOP cycle. However, this slight optimization is left
> for another patch. We return -ETIMEDOUT if the hardware doesn't
> deactivate after the STOP cycle.
>
> Signed-off-by: Daniel Kurtz
I've re-read gmbus register spec and STOP seems to be allowed even in the
first cycle. Does this patch solve an issue for you? If not, I prefer we
just drop it.
-Daniel
> ---
> drivers/gpu/drm/i915/intel_i2c.c | 30 +++---
> 1 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> b/drivers/gpu/drm/i915/intel_i2c.c
> index 9707868..25f2c9e 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -204,8 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
> }
>
> static int
> -gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
> - bool last)
> +gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
> {
> int reg_offset = dev_priv->gpio_mmio_base;
> u16 len = msg->len;
> @@ -213,7 +212,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
> i2c_msg *msg,
>
> I915_WRITE(GMBUS1 + reg_offset,
> GMBUS_CYCLE_WAIT |
> -(last ? GMBUS_CYCLE_STOP : 0) |
> (len << GMBUS_BYTE_COUNT_SHIFT) |
> (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
> GMBUS_SLAVE_READ | GMBUS_SW_RDY);
> @@ -239,8 +237,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
> i2c_msg *msg,
> }
>
> static int
> -gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
> - bool last)
> +gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
> {
> int reg_offset = dev_priv->gpio_mmio_base;
> u16 len = msg->len;
> @@ -256,7 +253,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv,
> struct i2c_msg *msg,
> I915_WRITE(GMBUS3 + reg_offset, val);
> I915_WRITE(GMBUS1 + reg_offset,
> GMBUS_CYCLE_WAIT |
> -(last ? GMBUS_CYCLE_STOP : 0) |
> (msg->len << GMBUS_BYTE_COUNT_SHIFT) |
> (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
> GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> @@ -289,7 +285,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
> struct intel_gmbus,
> adapter);
> struct drm_i915_private *dev_priv = bus->dev_priv;
> - int i, reg_offset, ret;
> + int i, reg_offset;
> + int ret = 0;
>
> mutex_lock(_priv->gmbus_mutex);
>
> @@ -303,20 +300,17 @@ gmbus_xfer(struct i2c_adapter *adapter,
> I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>
> for (i = 0; i < num; i++) {
> - bool last = i + 1 == num;
> -
> if (msgs[i].flags & I2C_M_RD)
> - ret = gmbus_xfer_read(dev_priv, [i], last);
> + ret = gmbus_xfer_read(dev_priv, [i]);
> else
> - ret = gmbus_xfer_write(dev_priv, [i], last);
> + ret = gmbus_xfer_write(dev_priv, [i]);
>
> if (ret == -ETIMEDOUT)
> goto timeout;
> if (ret == -ENXIO)
> goto clear_err;
>
> - if (!last &&
> - wait_for(I915_READ(GMBUS2 + reg_offset) &
> + if (wait_for(I915_READ(GMBUS2 + reg_offset) &
>(GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
>50))
> goto timeout;
> @@ -324,15 +318,21 @@ gmbus_xfer(struct i2c_adapter *adapter,
> goto clear_err;
> }
>
> + /* Generate a STOP condition on the bus */
> + I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
> +
> /* Mark the GMBUS interface as disabled after waiting for idle.
>* We will re-enable it at the start of the next xfer,
>* till then let it sleep.
>*/
> - if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
> + if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
> + 10)) {
> DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
>adapter->name);
> +
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote:
The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
transaction) during a DATA or WAIT phase. In other words, the
controller rejects a STOP requested as part of the first transaction in a
sequence.
Thus, for the first transaction we must always use a WAIT cycle, detect
when the device has finished (and is in a WAIT phase), and then either
start the next transaction, or, if there are no more transactions,
generate a STOP cycle.
Note: Theoretically, the last transaction of a multi-transaction sequence
could initiate a STOP cycle. However, this slight optimization is left
for another patch. We return -ETIMEDOUT if the hardware doesn't
deactivate after the STOP cycle.
Signed-off-by: Daniel Kurtz djku...@chromium.org
I've re-read gmbus register spec and STOP seems to be allowed even in the
first cycle. Does this patch solve an issue for you? If not, I prefer we
just drop it.
-Daniel
---
drivers/gpu/drm/i915/intel_i2c.c | 30 +++---
1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c
b/drivers/gpu/drm/i915/intel_i2c.c
index 9707868..25f2c9e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,8 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
}
static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
- bool last)
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
{
int reg_offset = dev_priv-gpio_mmio_base;
u16 len = msg-len;
@@ -213,7 +212,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
I915_WRITE(GMBUS1 + reg_offset,
GMBUS_CYCLE_WAIT |
-(last ? GMBUS_CYCLE_STOP : 0) |
(len GMBUS_BYTE_COUNT_SHIFT) |
(msg-addr GMBUS_SLAVE_ADDR_SHIFT) |
GMBUS_SLAVE_READ | GMBUS_SW_RDY);
@@ -239,8 +237,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
}
static int
-gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
- bool last)
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
{
int reg_offset = dev_priv-gpio_mmio_base;
u16 len = msg-len;
@@ -256,7 +253,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv,
struct i2c_msg *msg,
I915_WRITE(GMBUS3 + reg_offset, val);
I915_WRITE(GMBUS1 + reg_offset,
GMBUS_CYCLE_WAIT |
-(last ? GMBUS_CYCLE_STOP : 0) |
(msg-len GMBUS_BYTE_COUNT_SHIFT) |
(msg-addr GMBUS_SLAVE_ADDR_SHIFT) |
GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
@@ -289,7 +285,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = bus-dev_priv;
- int i, reg_offset, ret;
+ int i, reg_offset;
+ int ret = 0;
mutex_lock(dev_priv-gmbus_mutex);
@@ -303,20 +300,17 @@ gmbus_xfer(struct i2c_adapter *adapter,
I915_WRITE(GMBUS0 + reg_offset, bus-reg0);
for (i = 0; i num; i++) {
- bool last = i + 1 == num;
-
if (msgs[i].flags I2C_M_RD)
- ret = gmbus_xfer_read(dev_priv, msgs[i], last);
+ ret = gmbus_xfer_read(dev_priv, msgs[i]);
else
- ret = gmbus_xfer_write(dev_priv, msgs[i], last);
+ ret = gmbus_xfer_write(dev_priv, msgs[i]);
if (ret == -ETIMEDOUT)
goto timeout;
if (ret == -ENXIO)
goto clear_err;
- if (!last
- wait_for(I915_READ(GMBUS2 + reg_offset)
+ if (wait_for(I915_READ(GMBUS2 + reg_offset)
(GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
50))
goto timeout;
@@ -324,15 +318,21 @@ gmbus_xfer(struct i2c_adapter *adapter,
goto clear_err;
}
+ /* Generate a STOP condition on the bus */
+ I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
+
/* Mark the GMBUS interface as disabled after waiting for idle.
* We will re-enable it at the start of the next xfer,
* till then let it sleep.
*/
- if (wait_for((I915_READ(GMBUS2 + reg_offset) GMBUS_ACTIVE) == 0, 10))
+ if (wait_for((I915_READ(GMBUS2 + reg_offset) GMBUS_ACTIVE) == 0,
+ 10)) {
DRM_INFO(GMBUS [%s] timed out waiting for idle\n,
adapter-name);
+ ret = -ETIMEDOUT;
+ }
I915_WRITE(GMBUS0 + reg_offset, 0);
- ret = i;
+ ret = ret
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: The i915 is only able to generate a STOP cycle (i.e. finalize an i2c transaction) during a DATA or WAIT phase. In other words, the controller rejects a STOP requested as part of the first transaction in a sequence. Thus, for the first transaction we must always use a WAIT cycle, detect when the device has finished (and is in a WAIT phase), and then either start the next transaction, or, if there are no more transactions, generate a STOP cycle. Note: Theoretically, the last transaction of a multi-transaction sequence could initiate a STOP cycle. However, this slight optimization is left for another patch. We return -ETIMEDOUT if the hardware doesn't deactivate after the STOP cycle. Signed-off-by: Daniel Kurtz djku...@chromium.org I've re-read gmbus register spec and STOP seems to be allowed even in the first cycle. Does this patch solve an issue for you? If not, I prefer we just drop it. Actually I'd like to keep the -ETIMEDOUT return value, so maybe we should keeep that hunk. I've picked up the previous 3 patches of this series, the once after this one here conflict (without this patch here). -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: The i915 is only able to generate a STOP cycle (i.e. finalize an i2c transaction) during a DATA or WAIT phase. In other words, the controller rejects a STOP requested as part of the first transaction in a sequence. Thus, for the first transaction we must always use a WAIT cycle, detect when the device has finished (and is in a WAIT phase), and then either start the next transaction, or, if there are no more transactions, generate a STOP cycle. Note: Theoretically, the last transaction of a multi-transaction sequence could initiate a STOP cycle. However, this slight optimization is left for another patch. We return -ETIMEDOUT if the hardware doesn't deactivate after the STOP cycle. Signed-off-by: Daniel Kurtz djku...@chromium.org I've re-read gmbus register spec and STOP seems to be allowed even in the first cycle. Does this patch solve an issue for you? If not, I prefer we just drop it. STOP does not work in the first cycle, hence the patch. -Daniel Actually I'd like to keep the -ETIMEDOUT return value, so maybe we should keeep that hunk. I've picked up the previous 3 patches of this series, the once after this one here conflict (without this patch here). -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: The i915 is only able to generate a STOP cycle (i.e. finalize an i2c transaction) during a DATA or WAIT phase. In other words, the controller rejects a STOP requested as part of the first transaction in a sequence. Thus, for the first transaction we must always use a WAIT cycle, detect when the device has finished (and is in a WAIT phase), and then either start the next transaction, or, if there are no more transactions, generate a STOP cycle. Note: Theoretically, the last transaction of a multi-transaction sequence could initiate a STOP cycle. However, this slight optimization is left for another patch. We return -ETIMEDOUT if the hardware doesn't deactivate after the STOP cycle. Signed-off-by: Daniel Kurtz djku...@chromium.org I've re-read gmbus register spec and STOP seems to be allowed even in the first cycle. Does this patch solve an issue for you? If not, I prefer we just drop it. STOP does not work in the first cycle, hence the patch. Ok, I've picked this patch up and extended the comment a bit to that effect. Just to avoid anyone else trying to 'fix' things because bspec sounds like it should work. I've also picked up the other patches safe for the last one, thanks a lot for digging through the gmbus code and fixing it all up. Now can I volunteer you for a (hopefully) last set of gmbus patches? Afaics there are a few small things left to fix: - zero-length reads can blow up the kernel, like zero-length writes could. See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 - Chris Wilson suggested on irc that we should wait for HW_READY even for zero-length writes (and also reads), currently we don't. - atm the debug output is too noisy. I think we can leave the fallback to gpio bitbanging at info (or maybe error) level, but all the other messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit when userspace tries to probe the i2c with nothing connected or if the driver code tries to do the same. See: https://bugs.freedesktop.org/show_bug.cgi?id=48248 Chris, anything you want to add to the wishlist? Thanks, Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
On Tue, 10 Apr 2012 17:03:04 +0200, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 06:56:15PM +0800, Daniel Kurtz wrote: On Tue, Apr 10, 2012 at 6:41 PM, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Apr 10, 2012 at 12:37:46PM +0200, Daniel Vetter wrote: On Fri, Mar 30, 2012 at 07:46:39PM +0800, Daniel Kurtz wrote: The i915 is only able to generate a STOP cycle (i.e. finalize an i2c transaction) during a DATA or WAIT phase. Â In other words, the controller rejects a STOP requested as part of the first transaction in a sequence. Thus, for the first transaction we must always use a WAIT cycle, detect when the device has finished (and is in a WAIT phase), and then either start the next transaction, or, if there are no more transactions, generate a STOP cycle. Note: Theoretically, the last transaction of a multi-transaction sequence could initiate a STOP cycle. Â However, this slight optimization is left for another patch. Â We return -ETIMEDOUT if the hardware doesn't deactivate after the STOP cycle. Signed-off-by: Daniel Kurtz djku...@chromium.org I've re-read gmbus register spec and STOP seems to be allowed even in the first cycle. Does this patch solve an issue for you? If not, I prefer we just drop it. STOP does not work in the first cycle, hence the patch. Ok, I've picked this patch up and extended the comment a bit to that effect. Just to avoid anyone else trying to 'fix' things because bspec sounds like it should work. I've also picked up the other patches safe for the last one, thanks a lot for digging through the gmbus code and fixing it all up. Now can I volunteer you for a (hopefully) last set of gmbus patches? Afaics there are a few small things left to fix: - zero-length reads can blow up the kernel, like zero-length writes could. See: https://bugs.freedesktop.org/show_bug.cgi?id=48269 - Chris Wilson suggested on irc that we should wait for HW_READY even for zero-length writes (and also reads), currently we don't. - atm the debug output is too noisy. I think we can leave the fallback to gpio bitbanging at info (or maybe error) level, but all the other messages should be tuned down to DRM_DEBUG_KMS - these can easily be hit when userspace tries to probe the i2c with nothing connected or if the driver code tries to do the same. See: https://bugs.freedesktop.org/show_bug.cgi?id=48248 Chris, anything you want to add to the wishlist? The last major item on the wishlist is solving how to drive the SDVO i2c over gmbus. I think it is just a matter of massaging in the channel switch as msg[0]. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
transaction) during a DATA or WAIT phase. In other words, the
controller rejects a STOP requested as part of the first transaction in a
sequence.
Thus, for the first transaction we must always use a WAIT cycle, detect
when the device has finished (and is in a WAIT phase), and then either
start the next transaction, or, if there are no more transactions,
generate a STOP cycle.
Note: Theoretically, the last transaction of a multi-transaction sequence
could initiate a STOP cycle. However, this slight optimization is left
for another patch. We return -ETIMEDOUT if the hardware doesn't
deactivate after the STOP cycle.
Signed-off-by: Daniel Kurtz
---
drivers/gpu/drm/i915/intel_i2c.c | 30 +++---
1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 9707868..25f2c9e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,8 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
}
static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
- bool last)
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
{
int reg_offset = dev_priv->gpio_mmio_base;
u16 len = msg->len;
@@ -213,7 +212,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
I915_WRITE(GMBUS1 + reg_offset,
GMBUS_CYCLE_WAIT |
- (last ? GMBUS_CYCLE_STOP : 0) |
(len << GMBUS_BYTE_COUNT_SHIFT) |
(msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
GMBUS_SLAVE_READ | GMBUS_SW_RDY);
@@ -239,8 +237,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
}
static int
-gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
- bool last)
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
{
int reg_offset = dev_priv->gpio_mmio_base;
u16 len = msg->len;
@@ -256,7 +253,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
I915_WRITE(GMBUS3 + reg_offset, val);
I915_WRITE(GMBUS1 + reg_offset,
GMBUS_CYCLE_WAIT |
- (last ? GMBUS_CYCLE_STOP : 0) |
(msg->len << GMBUS_BYTE_COUNT_SHIFT) |
(msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
@@ -289,7 +285,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = bus->dev_priv;
- int i, reg_offset, ret;
+ int i, reg_offset;
+ int ret = 0;
mutex_lock(_priv->gmbus_mutex);
@@ -303,20 +300,17 @@ gmbus_xfer(struct i2c_adapter *adapter,
I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
for (i = 0; i < num; i++) {
- bool last = i + 1 == num;
-
if (msgs[i].flags & I2C_M_RD)
- ret = gmbus_xfer_read(dev_priv, [i], last);
+ ret = gmbus_xfer_read(dev_priv, [i]);
else
- ret = gmbus_xfer_write(dev_priv, [i], last);
+ ret = gmbus_xfer_write(dev_priv, [i]);
if (ret == -ETIMEDOUT)
goto timeout;
if (ret == -ENXIO)
goto clear_err;
- if (!last &&
- wait_for(I915_READ(GMBUS2 + reg_offset) &
+ if (wait_for(I915_READ(GMBUS2 + reg_offset) &
(GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
50))
goto timeout;
@@ -324,15 +318,21 @@ gmbus_xfer(struct i2c_adapter *adapter,
goto clear_err;
}
+ /* Generate a STOP condition on the bus */
+ I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
+
/* Mark the GMBUS interface as disabled after waiting for idle.
* We will re-enable it at the start of the next xfer,
* till then let it sleep.
*/
- if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
+ if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
+10)) {
DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
adapter->name);
+ ret = -ETIMEDOUT;
+ }
I915_WRITE(GMBUS0 + reg_offset, 0);
- ret = i;
+ ret = ret ?: i;
goto out;
clear_err:
--
1.7.7.3
[PATCH 4/8 v7] drm/i915/intel_i2c: use WAIT cycle, not STOP
The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
transaction) during a DATA or WAIT phase. In other words, the
controller rejects a STOP requested as part of the first transaction in a
sequence.
Thus, for the first transaction we must always use a WAIT cycle, detect
when the device has finished (and is in a WAIT phase), and then either
start the next transaction, or, if there are no more transactions,
generate a STOP cycle.
Note: Theoretically, the last transaction of a multi-transaction sequence
could initiate a STOP cycle. However, this slight optimization is left
for another patch. We return -ETIMEDOUT if the hardware doesn't
deactivate after the STOP cycle.
Signed-off-by: Daniel Kurtz djku...@chromium.org
---
drivers/gpu/drm/i915/intel_i2c.c | 30 +++---
1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 9707868..25f2c9e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,8 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
}
static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
- bool last)
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
{
int reg_offset = dev_priv-gpio_mmio_base;
u16 len = msg-len;
@@ -213,7 +212,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
I915_WRITE(GMBUS1 + reg_offset,
GMBUS_CYCLE_WAIT |
- (last ? GMBUS_CYCLE_STOP : 0) |
(len GMBUS_BYTE_COUNT_SHIFT) |
(msg-addr GMBUS_SLAVE_ADDR_SHIFT) |
GMBUS_SLAVE_READ | GMBUS_SW_RDY);
@@ -239,8 +237,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
}
static int
-gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
- bool last)
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
{
int reg_offset = dev_priv-gpio_mmio_base;
u16 len = msg-len;
@@ -256,7 +253,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct
i2c_msg *msg,
I915_WRITE(GMBUS3 + reg_offset, val);
I915_WRITE(GMBUS1 + reg_offset,
GMBUS_CYCLE_WAIT |
- (last ? GMBUS_CYCLE_STOP : 0) |
(msg-len GMBUS_BYTE_COUNT_SHIFT) |
(msg-addr GMBUS_SLAVE_ADDR_SHIFT) |
GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
@@ -289,7 +285,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
struct intel_gmbus,
adapter);
struct drm_i915_private *dev_priv = bus-dev_priv;
- int i, reg_offset, ret;
+ int i, reg_offset;
+ int ret = 0;
mutex_lock(dev_priv-gmbus_mutex);
@@ -303,20 +300,17 @@ gmbus_xfer(struct i2c_adapter *adapter,
I915_WRITE(GMBUS0 + reg_offset, bus-reg0);
for (i = 0; i num; i++) {
- bool last = i + 1 == num;
-
if (msgs[i].flags I2C_M_RD)
- ret = gmbus_xfer_read(dev_priv, msgs[i], last);
+ ret = gmbus_xfer_read(dev_priv, msgs[i]);
else
- ret = gmbus_xfer_write(dev_priv, msgs[i], last);
+ ret = gmbus_xfer_write(dev_priv, msgs[i]);
if (ret == -ETIMEDOUT)
goto timeout;
if (ret == -ENXIO)
goto clear_err;
- if (!last
- wait_for(I915_READ(GMBUS2 + reg_offset)
+ if (wait_for(I915_READ(GMBUS2 + reg_offset)
(GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
50))
goto timeout;
@@ -324,15 +318,21 @@ gmbus_xfer(struct i2c_adapter *adapter,
goto clear_err;
}
+ /* Generate a STOP condition on the bus */
+ I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
+
/* Mark the GMBUS interface as disabled after waiting for idle.
* We will re-enable it at the start of the next xfer,
* till then let it sleep.
*/
- if (wait_for((I915_READ(GMBUS2 + reg_offset) GMBUS_ACTIVE) == 0, 10))
+ if (wait_for((I915_READ(GMBUS2 + reg_offset) GMBUS_ACTIVE) == 0,
+10)) {
DRM_INFO(GMBUS [%s] timed out waiting for idle\n,
adapter-name);
+ ret = -ETIMEDOUT;
+ }
I915_WRITE(GMBUS0 + reg_offset, 0);
- ret = i;
+ ret = ret ?: i;
goto out;
clear_err:
--
1.7.7.3
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
