This patchset addresses a couple of issues with the i915 gmbus implementation: * fixes misassigned pin port pair for HDMI-D * fixes write transactions when they are the only transaction requested (including large >4-byte writes) by terminating every transaction with a WAIT cycle. * returns -ENXIO and -ETIMEDOUT as appropriate so upper layers can handled i2c transaction failures * optimizes the typical read transaction case by using the INDEX cycle v3: * rebased onto Daniel Vetter's drm-intel-next-queued branch at git://people.freedesktop.org/~danvet/drm-intel * replace intel_i2c_quirk_xfer with pre/post_xfer i2c routines * pre-allocate gmbus array * drop interrupt approach since I could not make it stable, probably due to difficulty in clearing and resetting the GMBUS interrupt which is buffered behind the SDE's PCH interrupt. * Fix zero-length writes * Wait for IDLE before clearing NAK
Daniel Kurtz (11): drm/i915/intel_i2c: cleanup drm/i915/intel_i2c: assign HDMI port D to pin pair 6 drm/i915/intel_i2c: use i2c pre/post_xfer functions to setup gpio xfers drm/i915/intel_i2c: cleanup gmbus/gpio pin assignments drm/i915/intel_i2c: allocate gmbus array as part of drm_i915_private drm/i915/intel_i2c: refactor using intel_gmbus_get_adapter drm/i915/intel_i2c: handle zero-length writes drm/i915/intel_i2c: always wait for IDLE before clearing NAK drm/i915/intel_i2c: use WAIT cycle, not STOP drm/i915/intel_i2c: use INDEX cycles for i2c read transactions drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop
drivers/gpu/drm/i915/i915_drv.h | 10 +- drivers/gpu/drm/i915/i915_reg.h | 6 +- drivers/gpu/drm/i915/intel_bios.c | 4 +- drivers/gpu/drm/i915/intel_crt.c | 14 +- drivers/gpu/drm/i915/intel_dvo.c | 6 +- drivers/gpu/drm/i915/intel_hdmi.c | 9 +- drivers/gpu/drm/i915/intel_i2c.c | 278 ++++++++++++++++++++++-------------- drivers/gpu/drm/i915/intel_lvds.c | 7 +- drivers/gpu/drm/i915/intel_modes.c | 3 +- drivers/gpu/drm/i915/intel_sdvo.c | 9 +- 10 files changed, 213 insertions(+), 133 deletions(-)
80 col, spaces around operators and other basic cleanup. Some info message cleanup.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 32 +++++++++++++++++++++----------- 1 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 0713cc2..86b1861 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -230,13 +230,16 @@ gmbus_xfer(struct i2c_adapter *adapter, (len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_READ | GMBUS_SW_RDY); - POSTING_READ(GMBUS2+reg_offset); + POSTING_READ(GMBUS2 + reg_offset); do { u32 val, loop = 0;
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) + if (wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) + if (I915_READ(GMBUS2 + reg_offset) & + GMBUS_SATOER) goto clear_err;
val = I915_READ(GMBUS3 + reg_offset); @@ -260,12 +263,15 @@ gmbus_xfer(struct i2c_adapter *adapter, (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_WRITE | GMBUS_SW_RDY); - POSTING_READ(GMBUS2+reg_offset); + POSTING_READ(GMBUS2 + reg_offset);
while (len) { - if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) + if (wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) + if (I915_READ(GMBUS2 + reg_offset) & + GMBUS_SATOER) goto clear_err;
val = loop = 0; @@ -274,11 +280,14 @@ gmbus_xfer(struct i2c_adapter *adapter, } while (--len && ++loop < 4);
I915_WRITE(GMBUS3 + reg_offset, val); - POSTING_READ(GMBUS2+reg_offset); + POSTING_READ(GMBUS2 + reg_offset); } }
- if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50)) + if (i + 1 < num && + wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), + 50)) goto timeout; if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) goto clear_err; @@ -300,14 +309,15 @@ done: * till then let it sleep. */ if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10)) - DRM_INFO("GMBUS timed out waiting for idle\n"); + DRM_INFO("GMBUS [%s] timed out waiting for idle\n", + bus->adapter.name); I915_WRITE(GMBUS0 + reg_offset, 0); ret = i; goto out;
timeout: - DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n", - bus->reg0 & 0xff, bus->adapter.name); + DRM_INFO("GMBUS [%s] timed out, falling back to bit banging on pin %d\n", + bus->adapter.name, bus->reg0 & 0xff); I915_WRITE(GMBUS0 + reg_offset, 0);
/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
On Mon, Mar 26, 2012 at 10:26:40PM +0800, Daniel Kurtz wrote:
80 col, spaces around operators and other basic cleanup. Some info message cleanup.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
On reconsideration, nacked. You desperately try to squeeze a function with already 5 indent levels (not counting indentation due to function arguments and overflow) with already 100+ lines of code into 80 columns.
Please do the right thing and split out a few of helper functions. I suggest you split out the read and write code, i.e.
if (msgs[i].flags & I2C_M_RD) gmbus_xfer_read() else gmbus_xfer_write
This way you get 3 levels of tab space of room and I won't have to complain as much that some of your later patches make gmbus_xfer way too bug.
Yours, Daniel
drivers/gpu/drm/i915/intel_i2c.c | 32 +++++++++++++++++++++----------- 1 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 0713cc2..86b1861 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -230,13 +230,16 @@ gmbus_xfer(struct i2c_adapter *adapter, (len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_READ | GMBUS_SW_RDY);
POSTING_READ(GMBUS2+reg_offset);
POSTING_READ(GMBUS2 + reg_offset); do { u32 val, loop = 0;
if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
if (wait_for(I915_READ(GMBUS2 + reg_offset) &(GMBUS_SATOER | GMBUS_HW_RDY),50)) goto timeout;
if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
if (I915_READ(GMBUS2 + reg_offset) &GMBUS_SATOER) goto clear_err; val = I915_READ(GMBUS3 + reg_offset);@@ -260,12 +263,15 @@ gmbus_xfer(struct i2c_adapter *adapter, (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
POSTING_READ(GMBUS2+reg_offset);
POSTING_READ(GMBUS2 + reg_offset); while (len) {
if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
if (wait_for(I915_READ(GMBUS2 + reg_offset) &(GMBUS_SATOER | GMBUS_HW_RDY),50)) goto timeout;
if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
if (I915_READ(GMBUS2 + reg_offset) &GMBUS_SATOER) goto clear_err; val = loop = 0;@@ -274,11 +280,14 @@ gmbus_xfer(struct i2c_adapter *adapter, } while (--len && ++loop < 4);
I915_WRITE(GMBUS3 + reg_offset, val);
POSTING_READ(GMBUS2+reg_offset);
POSTING_READ(GMBUS2 + reg_offset); }
if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
if (i + 1 < num &&wait_for(I915_READ(GMBUS2 + reg_offset) &(GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),50)) goto timeout;@@ -300,14 +309,15 @@ done: * till then let it sleep. */ if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
DRM_INFO("GMBUS timed out waiting for idle\n");
DRM_INFO("GMBUS [%s] timed out waiting for idle\n",bus->adapter.name);timeout:
- DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
bus->reg0 & 0xff, bus->adapter.name);
DRM_INFO("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
bus->adapter.name, bus->reg0 & 0xff);I915_WRITE(GMBUS0 + reg_offset, 0);
/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
-- 1.7.7.3
According to i915 documentation [1], "Port D" (DP/HDMI Port D) is actually gmbus pin pair 6 (gmbus0.2:0 == 110b GPIOF), not 7 (111b). Pin pair 7 is a reserved pair.
[1] Documentation for [DevSNB+] and [DevIBX], as found on http://intellinuxgraphics.org
Note: the "reserved" and "disabled" pairs do not actually map to a physical pair of pins, nor GPIO regs and shouldn't be initialized or used. Fixing this is left for a later patch.
This bug has not been noticed for two reasons: 1) "gmbus" mode is currently disabled - all transfers are actually using "bit-bang" mode which uses the GPIO port 5 (the "HDMI/DPD CTLDATA/CLK" pair), at register 0x5024 (defined as GPIOF i915_reg.h). Since this is the correct pair of pins for HDMI1, transfers succeed.
2) Even if gmbus mode is re-enabled, the first attempted transaction will fail because it tries to use the wrong ("Reserved") pin pair. However, the driver immediately falls back again to the bit-bang method, which correctly uses GPIOF, so again, transfers succeed.
However, if gmbus mode is re-enabled and the GPIO fall-back mode is disabled, then reading an attached monitor's EDID fail.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_reg.h | 6 +++--- drivers/gpu/drm/i915/intel_i2c.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f3609f2..accd8ee 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -742,9 +742,9 @@ #define GMBUS_PORT_PANEL 3 #define GMBUS_PORT_DPC 4 /* HDMIC */ #define GMBUS_PORT_DPB 5 /* SDVO, HDMIB */ - /* 6 reserved */ -#define GMBUS_PORT_DPD 7 /* HDMID */ -#define GMBUS_NUM_PORTS 8 +#define GMBUS_PORT_DPD 6 /* HDMID */ +#define GMBUS_PORT_RESERVED 7 /* 7 reserved */ +#define GMBUS_NUM_PORTS 8 #define GMBUS1 0x5104 /* command/status */ #define GMBUS_SW_CLR_INT (1<<31) #define GMBUS_SW_RDY (1<<30) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 86b1861..54f85a1 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -148,8 +148,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) GPIOC, GPIOD, GPIOE, - 0, GPIOF, + 0, }; struct i2c_algo_bit_data *algo;
@@ -359,8 +359,8 @@ int intel_setup_gmbus(struct drm_device *dev) "panel", "dpc", "dpb", - "reserved", "dpd", + "reserved", }; struct drm_i915_private *dev_priv = dev->dev_private; int ret, i;
On Mon, Mar 26, 2012 at 10:26:41PM +0800, Daniel Kurtz wrote:
According to i915 documentation [1], "Port D" (DP/HDMI Port D) is actually gmbus pin pair 6 (gmbus0.2:0 == 110b GPIOF), not 7 (111b). Pin pair 7 is a reserved pair.
[1] Documentation for [DevSNB+] and [DevIBX], as found on http://intellinuxgraphics.org
Note: the "reserved" and "disabled" pairs do not actually map to a physical pair of pins, nor GPIO regs and shouldn't be initialized or used. Fixing this is left for a later patch.
This bug has not been noticed for two reasons:
- "gmbus" mode is currently disabled - all transfers are actually using "bit-bang" mode which uses the GPIO port 5 (the "HDMI/DPD CTLDATA/CLK" pair), at register 0x5024 (defined as GPIOF i915_reg.h). Since this is the correct pair of pins for HDMI1, transfers succeed.
... this is no longer true on drm-intel-next.
- Even if gmbus mode is re-enabled, the first attempted transaction will fail because it tries to use the wrong ("Reserved") pin pair. However, the driver immediately falls back again to the bit-bang method, which correctly uses GPIOF, so again, transfers succeed.
However, if gmbus mode is re-enabled and the GPIO fall-back mode is disabled, then reading an attached monitor's EDID fail.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
Otherwise this looks ok to me - I've checked with gen3 Bspec and we seem to indeed have a 1:1 mapping, see "Display Registers", 1.5.3 "GPIO Control Registers", the list right below the heading.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
When resending, can you please add the Bspec reference above?
Thanks, Daniel
drivers/gpu/drm/i915/i915_reg.h | 6 +++--- drivers/gpu/drm/i915/intel_i2c.c | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index f3609f2..accd8ee 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -742,9 +742,9 @@ #define GMBUS_PORT_PANEL 3 #define GMBUS_PORT_DPC 4 /* HDMIC */ #define GMBUS_PORT_DPB 5 /* SDVO, HDMIB */
/* 6 reserved */-#define GMBUS_PORT_DPD 7 /* HDMID */ -#define GMBUS_NUM_PORTS 8 +#define GMBUS_PORT_DPD 6 /* HDMID */ +#define GMBUS_PORT_RESERVED 7 /* 7 reserved */ +#define GMBUS_NUM_PORTS 8 #define GMBUS1 0x5104 /* command/status */ #define GMBUS_SW_CLR_INT (1<<31) #define GMBUS_SW_RDY (1<<30) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 86b1861..54f85a1 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -148,8 +148,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) GPIOC, GPIOD, GPIOE,
0,
0,@@ -359,8 +359,8 @@ int intel_setup_gmbus(struct drm_device *dev) "panel", "dpc", "dpb",
"reserved",
"reserved",-- 1.7.7.3
On Mon, Mar 26, 2012 at 04:47:11PM +0200, Daniel Vetter wrote:
On Mon, Mar 26, 2012 at 10:26:41PM +0800, Daniel Kurtz wrote:
According to i915 documentation [1], "Port D" (DP/HDMI Port D) is actually gmbus pin pair 6 (gmbus0.2:0 == 110b GPIOF), not 7 (111b). Pin pair 7 is a reserved pair.
[1] Documentation for [DevSNB+] and [DevIBX], as found on http://intellinuxgraphics.org
Note: the "reserved" and "disabled" pairs do not actually map to a physical pair of pins, nor GPIO regs and shouldn't be initialized or used. Fixing this is left for a later patch.
This bug has not been noticed for two reasons:
- "gmbus" mode is currently disabled - all transfers are actually using "bit-bang" mode which uses the GPIO port 5 (the "HDMI/DPD CTLDATA/CLK" pair), at register 0x5024 (defined as GPIOF i915_reg.h). Since this is the correct pair of pins for HDMI1, transfers succeed.
... this is no longer true on drm-intel-next.
- Even if gmbus mode is re-enabled, the first attempted transaction will fail because it tries to use the wrong ("Reserved") pin pair. However, the driver immediately falls back again to the bit-bang method, which correctly uses GPIOF, so again, transfers succeed.
However, if gmbus mode is re-enabled and the GPIO fall-back mode is disabled, then reading an attached monitor's EDID fail.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
Otherwise this looks ok to me - I've checked with gen3 Bspec and we seem to indeed have a 1:1 mapping, see "Display Registers", 1.5.3 "GPIO Control Registers", the list right below the heading.
well, scrap that, I've confused myself here a bit. Afaict we don't actually use these gmbus ports on earlier chips.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
When resending, can you please add the Bspec reference above?
Can you you instead add a clear reference (Volume, Full Section plus Heading Title) for the Snb/Ibx public Bspec.
Thanks, Daneil
On Mon, Mar 26, 2012 at 11:08 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Mar 26, 2012 at 04:47:11PM +0200, Daniel Vetter wrote:
On Mon, Mar 26, 2012 at 10:26:41PM +0800, Daniel Kurtz wrote:
According to i915 documentation [1], "Port D" (DP/HDMI Port D) is actually gmbus pin pair 6 (gmbus0.2:0 == 110b GPIOF), not 7 (111b). Pin pair 7 is a reserved pair.
[1] Documentation for [DevSNB+] and [DevIBX], as found on http://intellinuxgraphics.org
Note: the "reserved" and "disabled" pairs do not actually map to a physical pair of pins, nor GPIO regs and shouldn't be initialized or used. Fixing this is left for a later patch.
This bug has not been noticed for two reasons:  1) "gmbus" mode is currently disabled - all transfers are actually using   "bit-bang" mode which uses the GPIO port 5 (the "HDMI/DPD CTLDATA/CLK"   pair), at register 0x5024 (defined as GPIOF i915_reg.h).   Since this is the correct pair of pins for HDMI1, transfers succeed.
... this is no longer true on drm-intel-next.
2) Even if gmbus mode is re-enabled, the first attempted transaction   will fail because it tries to use the wrong ("Reserved") pin pair.   However, the driver immediately falls back again to the bit-bang   method, which correctly uses GPIOF, so again, transfers succeed.
However, if gmbus mode is re-enabled and the GPIO fall-back mode is disabled, then reading an attached monitor's EDID fail.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
Otherwise this looks ok to me - I've checked with gen3 Bspec and we seem to indeed have a 1:1 mapping, see "Display Registers", 1.5.3 "GPIO Control Registers", the list right below the heading.
well, scrap that, I've confused myself here a bit. Afaict we don't actually use these gmbus ports on earlier chips.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
When resending, can you please add the Bspec reference above?
Can you you instead add a clear reference (Volume, Full Section plus Heading Title) for the Snb/Ibx public Bspec.
Thanks, Daneil
Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48
I'm not sure what a "Bspec" is, but here is the documents I used:
[DevSNB+]: http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf Section 2.2.2 lists the 6 gmbus ports (gpio pin pairs): [ 5: HDMI/DPD, 4: HDMIB, 3: HDMI/DPC, 2: LVDS, 1: SSC, 0: VGA ] 2.2.2.1 lists the GPIO registers to control these 6 ports. 2.2.3.1 lists the mapping between 5 of these gmbus ports and the 3 Pin_Pair_Select bits (of the GMBUS0 register). This table is missing HDMIB (port 101).
[DevIBX]: http://intellinuxgraphics.org/IHD_OS_Vol3_Part3r2.pdf Section 2.2.2 lists the same 6 gmbus ports plus two 'reserved' gpio ports. 2.2.2.1 lists 8 GPIO registers... however, it says the size of the block is 6x32, which implies that those 2 reserved GPIO registers (GPIO_6 & GPIO_7) don't actually exist (or are irrelevant). 2.2.3.1 lists the mapping between the 6 named gmbus ports and the 3 Pin_Pair_Select bits (of the GMBUS0 register). This table has HDMIB.
HTH, -Daniel
On Tue, Mar 27, 2012 at 01:49:51AM +0800, Daniel Kurtz wrote:
I'm not sure what a "Bspec" is, but here is the documents I used:
Section 2.2.2 lists the 6 gmbus ports (gpio pin pairs): [ 5: HDMI/DPD, 4: HDMIB, 3: HDMI/DPC, 2: LVDS, 1: SSC, 0: VGA ] 2.2.2.1 lists the GPIO registers to control these 6 ports. 2.2.3.1 lists the mapping between 5 of these gmbus ports and the 3 Pin_Pair_Select bits (of the GMBUS0 register). This table is missing HDMIB (port 101).
Section 2.2.2 lists the same 6 gmbus ports plus two 'reserved' gpio ports. 2.2.2.1 lists 8 GPIO registers... however, it says the size of the block is 6x32, which implies that those 2 reserved GPIO registers (GPIO_6 & GPIO_7) don't actually exist (or are irrelevant). 2.2.3.1 lists the mapping between the 6 named gmbus ports and the 3 Pin_Pair_Select bits (of the GMBUS0 register). This table has HDMIB.
Exactly this. Bspec is the internal name for what these public docs are based on. When fixing up an inconsistency wrt the docs it's always good to cite it like this, so that later on people can cross check. And we need section numbers because they are the most stable - Internal Bspec is just a collection of individual sections as html pages. And hw people also tend to use the same sections for the same stuff in newer/older chips.
Cheers, Daniel
Instead of rolling our own custom quirk_xfer function, use the bit_algo pre_xfer and post_xfer functions to setup and teardown bit-banged i2c transactions.
gmbus_xfer uses .force_bit to determine which i2c_algorithm to use, either i2c_bit_algo.master_xfer or its own. So, Similarly, let gmbus_func use .force_bit to determine which i2c functionalities are available, either i2c_bit_algo.functionality, or its own.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 72 +++++++++++++++++++++++-------------- 1 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 54f85a1..ae08a08 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -137,6 +137,35 @@ static void set_data(void *data, int state_high) POSTING_READ(bus->gpio_reg); }
+static int +intel_gpio_pre_xfer(struct i2c_adapter *adapter) +{ + struct intel_gmbus *bus = container_of(adapter, + struct intel_gmbus, + adapter); + struct drm_i915_private *dev_priv = bus->dev_priv; + + intel_i2c_reset(dev_priv->dev); + intel_i2c_quirk_set(dev_priv, true); + set_data(bus, 1); + set_clock(bus, 1); + udelay(I2C_RISEFALL_TIME); + return 0; +} + +static void +intel_gpio_post_xfer(struct i2c_adapter *adapter) +{ + struct intel_gmbus *bus = container_of(adapter, + struct intel_gmbus, + adapter); + struct drm_i915_private *dev_priv = bus->dev_priv; + + set_data(bus, 1); + set_clock(bus, 1); + intel_i2c_quirk_set(dev_priv, false); +} + static bool intel_gpio_setup(struct intel_gmbus *bus, u32 pin) { @@ -166,6 +195,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) algo->setscl = set_clock; algo->getsda = get_data; algo->getscl = get_clock; + algo->pre_xfer = intel_gpio_pre_xfer; + algo->post_xfer = intel_gpio_post_xfer; algo->udelay = I2C_RISEFALL_TIME; algo->timeout = usecs_to_jiffies(2200); algo->data = bus; @@ -174,30 +205,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) }
static int -intel_i2c_quirk_xfer(struct intel_gmbus *bus, - struct i2c_msg *msgs, - int num) -{ - struct drm_i915_private *dev_priv = bus->dev_priv; - int ret; - - intel_i2c_reset(dev_priv->dev); - - intel_i2c_quirk_set(dev_priv, true); - set_data(bus, 1); - set_clock(bus, 1); - udelay(I2C_RISEFALL_TIME); - - ret = i2c_bit_algo.master_xfer(&bus->adapter, msgs, num); - - set_data(bus, 1); - set_clock(bus, 1); - intel_i2c_quirk_set(dev_priv, false); - - return ret; -} - -static int gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) @@ -211,7 +218,7 @@ gmbus_xfer(struct i2c_adapter *adapter, mutex_lock(&dev_priv->gmbus_mutex);
if (bus->force_bit) { - ret = intel_i2c_quirk_xfer(bus, msgs, num); + ret = i2c_bit_algo.master_xfer(adapter, msgs, num); goto out; }
@@ -325,8 +332,9 @@ timeout: ret = -EIO; } else { bus->force_bit = true; - ret = intel_i2c_quirk_xfer(bus, msgs, num); + ret = i2c_bit_algo.master_xfer(adapter, msgs, num); } + out: mutex_unlock(&dev_priv->gmbus_mutex); return ret; @@ -334,11 +342,21 @@ out:
static u32 gmbus_func(struct i2c_adapter *adapter) { - return i2c_bit_algo.functionality(adapter) & + struct intel_gmbus *bus = container_of(adapter, + struct intel_gmbus, + adapter); + struct drm_i915_private *dev_priv = bus->dev_priv; + u32 func; + + mutex_lock(&dev_priv->gmbus_mutex); + func = bus->force_bit ? i2c_bit_algo.functionality(adapter) : (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | /* I2C_FUNC_10BIT_ADDR | */ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL); + mutex_unlock(&dev_priv->gmbus_mutex); + + return func; }
static const struct i2c_algorithm gmbus_algorithm = {
On Mon, Mar 26, 2012 at 10:26:42PM +0800, Daniel Kurtz wrote:
Instead of rolling our own custom quirk_xfer function, use the bit_algo pre_xfer and post_xfer functions to setup and teardown bit-banged i2c transactions.
gmbus_xfer uses .force_bit to determine which i2c_algorithm to use, either i2c_bit_algo.master_xfer or its own. So, Similarly, let gmbus_func use .force_bit to determine which i2c functionalities are available, either i2c_bit_algo.functionality, or its own.
Please split this part of the patch into a separate patch. Furthermore I'm not sure what this should buy us, given that we might magically changes our i2c feature set once with gone to fallback mode. Can you please elaborate why we need this?
The pre_xfer/post_xfer stuff looks good to me, that part along is Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Yours, Daniel
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
drivers/gpu/drm/i915/intel_i2c.c | 72 +++++++++++++++++++++++-------------- 1 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 54f85a1..ae08a08 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -137,6 +137,35 @@ static void set_data(void *data, int state_high) POSTING_READ(bus->gpio_reg); }
+static int +intel_gpio_pre_xfer(struct i2c_adapter *adapter) +{
- struct intel_gmbus *bus = container_of(adapter,
struct intel_gmbus,adapter);- struct drm_i915_private *dev_priv = bus->dev_priv;
- intel_i2c_reset(dev_priv->dev);
- intel_i2c_quirk_set(dev_priv, true);
- set_data(bus, 1);
- set_clock(bus, 1);
- udelay(I2C_RISEFALL_TIME);
- return 0;
+}
+static void +intel_gpio_post_xfer(struct i2c_adapter *adapter) +{
- struct intel_gmbus *bus = container_of(adapter,
struct intel_gmbus,adapter);- struct drm_i915_private *dev_priv = bus->dev_priv;
- set_data(bus, 1);
- set_clock(bus, 1);
- intel_i2c_quirk_set(dev_priv, false);
+}
static bool intel_gpio_setup(struct intel_gmbus *bus, u32 pin) { @@ -166,6 +195,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) algo->setscl = set_clock; algo->getsda = get_data; algo->getscl = get_clock;
- algo->pre_xfer = intel_gpio_pre_xfer;
- algo->post_xfer = intel_gpio_post_xfer; algo->udelay = I2C_RISEFALL_TIME; algo->timeout = usecs_to_jiffies(2200); algo->data = bus;
@@ -174,30 +205,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) }
static int -intel_i2c_quirk_xfer(struct intel_gmbus *bus,
struct i2c_msg *msgs,int num)-{
- struct drm_i915_private *dev_priv = bus->dev_priv;
- int ret;
- intel_i2c_reset(dev_priv->dev);
- intel_i2c_quirk_set(dev_priv, true);
- set_data(bus, 1);
- set_clock(bus, 1);
- udelay(I2C_RISEFALL_TIME);
- ret = i2c_bit_algo.master_xfer(&bus->adapter, msgs, num);
- set_data(bus, 1);
- set_clock(bus, 1);
- intel_i2c_quirk_set(dev_priv, false);
- return ret;
-}
-static int gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num) @@ -211,7 +218,7 @@ gmbus_xfer(struct i2c_adapter *adapter, mutex_lock(&dev_priv->gmbus_mutex);
if (bus->force_bit) {
ret = intel_i2c_quirk_xfer(bus, msgs, num);
ret = i2c_bit_algo.master_xfer(adapter, msgs, num);@@ -325,8 +332,9 @@ timeout: ret = -EIO; } else { bus->force_bit = true;
ret = intel_i2c_quirk_xfer(bus, msgs, num);
ret = i2c_bit_algo.master_xfer(adapter, msgs, num);out: mutex_unlock(&dev_priv->gmbus_mutex); return ret; @@ -334,11 +342,21 @@ out:
static u32 gmbus_func(struct i2c_adapter *adapter) {
- return i2c_bit_algo.functionality(adapter) &
- struct intel_gmbus *bus = container_of(adapter,
struct intel_gmbus,adapter);- struct drm_i915_private *dev_priv = bus->dev_priv;
- u32 func;
- mutex_lock(&dev_priv->gmbus_mutex);
- func = bus->force_bit ? i2c_bit_algo.functionality(adapter) : (I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | /* I2C_FUNC_10BIT_ADDR | */ I2C_FUNC_SMBUS_READ_BLOCK_DATA | I2C_FUNC_SMBUS_BLOCK_PROC_CALL);
- mutex_unlock(&dev_priv->gmbus_mutex);
- return func;
}
static const struct i2c_algorithm gmbus_algorithm = {
1.7.7.3
On Mon, Mar 26, 2012 at 10:49 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Mar 26, 2012 at 10:26:42PM +0800, Daniel Kurtz wrote:
Instead of rolling our own custom quirk_xfer function, use the bit_algo pre_xfer and post_xfer functions to setup and teardown bit-banged i2c transactions.
gmbus_xfer uses .force_bit to determine which i2c_algorithm to use, either i2c_bit_algo.master_xfer or its own. Â So, Similarly, let gmbus_func use .force_bit to determine which i2c functionalities are available, either i2c_bit_algo.functionality, or its own.
Please split this part of the patch into a separate patch. Furthermore I'm not sure what this should buy us, given that we might magically changes our i2c feature set once with gone to fallback mode. Can you please elaborate why we need this?
An i2c adapter's functionality is provided by its algorithm. Since these gmbus adapters can [for now] change their algorithm at runtime, I thought the functionality returned should match the currently selected algorithm at any given moment.
Arguably, the adapter actually sort of provides the union of the two functionalities since if a particular transfer fails using gmbus, it gets retried using bit-banged. But then again, this is a one-shot permanent switch, so perhaps we should return the union of the functionalities if force_bit == 0, and then only the bit-algo functionality after the switch?
The pre_xfer/post_xfer stuff looks good to me, that part along is Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Yours, Daniel
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
drivers/gpu/drm/i915/intel_i2c.c | Â 72 +++++++++++++++++++++++-------------- Â 1 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 54f85a1..ae08a08 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -137,6 +137,35 @@ static void set_data(void *data, int state_high) Â Â Â POSTING_READ(bus->gpio_reg); Â }
+static int +intel_gpio_pre_xfer(struct i2c_adapter *adapter) +{
- struct intel_gmbus *bus = container_of(adapter,
- struct intel_gmbus,
- adapter);
- struct drm_i915_private *dev_priv = bus->dev_priv;
- intel_i2c_reset(dev_priv->dev);
- intel_i2c_quirk_set(dev_priv, true);
- set_data(bus, 1);
- set_clock(bus, 1);
- udelay(I2C_RISEFALL_TIME);
- return 0;
+}
+static void +intel_gpio_post_xfer(struct i2c_adapter *adapter) +{
- struct intel_gmbus *bus = container_of(adapter,
- struct intel_gmbus,
- adapter);
- struct drm_i915_private *dev_priv = bus->dev_priv;
- set_data(bus, 1);
- set_clock(bus, 1);
- intel_i2c_quirk_set(dev_priv, false);
+}
static bool  intel_gpio_setup(struct intel_gmbus *bus, u32 pin)  { @@ -166,6 +195,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)    algo->setscl = set_clock;    algo->getsda = get_data;    algo->getscl = get_clock;
- algo->pre_xfer = intel_gpio_pre_xfer;
- algo->post_xfer = intel_gpio_post_xfer;
algo->udelay = I2C_RISEFALL_TIME; Â Â Â algo->timeout = usecs_to_jiffies(2200); Â Â Â algo->data = bus; @@ -174,30 +205,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) Â }
static int -intel_i2c_quirk_xfer(struct intel_gmbus *bus,
- struct i2c_msg *msgs,
- int num)
-{
- struct drm_i915_private *dev_priv = bus->dev_priv;
- int ret;
- intel_i2c_reset(dev_priv->dev);
- intel_i2c_quirk_set(dev_priv, true);
- set_data(bus, 1);
- set_clock(bus, 1);
- udelay(I2C_RISEFALL_TIME);
- ret = i2c_bit_algo.master_xfer(&bus->adapter, msgs, num);
- set_data(bus, 1);
- set_clock(bus, 1);
- intel_i2c_quirk_set(dev_priv, false);
- return ret;
-}
-static int  gmbus_xfer(struct i2c_adapter *adapter,      struct i2c_msg *msgs,      int num) @@ -211,7 +218,7 @@ gmbus_xfer(struct i2c_adapter *adapter,    mutex_lock(&dev_priv->gmbus_mutex);
if (bus->force_bit) {
- ret = intel_i2c_quirk_xfer(bus, msgs, num);
- ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
goto out; Â Â Â }
@@ -325,8 +332,9 @@ timeout: Â Â Â Â Â Â Â ret = -EIO; Â Â Â } else { Â Â Â Â Â Â Â bus->force_bit = true;
- ret = intel_i2c_quirk_xfer(bus, msgs, num);
- ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
}
out: Â Â Â mutex_unlock(&dev_priv->gmbus_mutex); Â Â Â return ret; @@ -334,11 +342,21 @@ out:
static u32 gmbus_func(struct i2c_adapter *adapter) Â {
- return i2c_bit_algo.functionality(adapter) &
- struct intel_gmbus *bus = container_of(adapter,
- struct intel_gmbus,
- adapter);
- struct drm_i915_private *dev_priv = bus->dev_priv;
- u32 func;
- mutex_lock(&dev_priv->gmbus_mutex);
- func = bus->force_bit ? i2c_bit_algo.functionality(adapter) :
(I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | Â Â Â Â Â Â Â /* I2C_FUNC_10BIT_ADDR | */ Â Â Â Â Â Â Â I2C_FUNC_SMBUS_READ_BLOCK_DATA | Â Â Â Â Â Â Â I2C_FUNC_SMBUS_BLOCK_PROC_CALL);
- mutex_unlock(&dev_priv->gmbus_mutex);
- return func;
}
static const struct i2c_algorithm gmbus_algorithm = {
1.7.7.3
-- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48
On Tue, Mar 27, 2012 at 01:58:47AM +0800, Daniel Kurtz wrote:
On Mon, Mar 26, 2012 at 10:49 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Mon, Mar 26, 2012 at 10:26:42PM +0800, Daniel Kurtz wrote:
Instead of rolling our own custom quirk_xfer function, use the bit_algo pre_xfer and post_xfer functions to setup and teardown bit-banged i2c transactions.
gmbus_xfer uses .force_bit to determine which i2c_algorithm to use, either i2c_bit_algo.master_xfer or its own. Â So, Similarly, let gmbus_func use .force_bit to determine which i2c functionalities are available, either i2c_bit_algo.functionality, or its own.
Please split this part of the patch into a separate patch. Furthermore I'm not sure what this should buy us, given that we might magically changes our i2c feature set once with gone to fallback mode. Can you please elaborate why we need this?
An i2c adapter's functionality is provided by its algorithm. Since these gmbus adapters can [for now] change their algorithm at runtime, I thought the functionality returned should match the currently selected algorithm at any given moment.
Arguably, the adapter actually sort of provides the union of the two functionalities since if a particular transfer fails using gmbus, it gets retried using bit-banged. But then again, this is a one-shot permanent switch, so perhaps we should return the union of the functionalities if force_bit == 0, and then only the bit-algo functionality after the switch?
In that case I guess we can drop it - current edid reading seems to work and without a good reason I'd like not to play clever tricks because it doesn't seem to be worth it. -Daniel
There is no GMBUS "disabled" port 0, nor "reserved" port 7. For the other 6 ports there is a fixed 1:1 mapping between pin pairs and gmbus ports, which means every real gmbus port has a gpio pin.
Given these realizations, clean up gmbus initialization.
Tested on Sandybridge (gen 6, PCH == CougarPoint) hardware.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_reg.h | 2 +- drivers/gpu/drm/i915/intel_i2c.c | 63 ++++++++++++++----------------------- 3 files changed, 25 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 9c75a7b..312dd4e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -303,7 +303,6 @@ struct intel_fbc_work; struct intel_gmbus { struct i2c_adapter adapter; bool force_bit; - bool has_gpio; u32 reg0; u32 gpio_reg; struct i2c_algo_bit_data bit_algo; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index accd8ee..a8d218c 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -744,7 +744,7 @@ #define GMBUS_PORT_DPB 5 /* SDVO, HDMIB */ #define GMBUS_PORT_DPD 6 /* HDMID */ #define GMBUS_PORT_RESERVED 7 /* 7 reserved */ -#define GMBUS_NUM_PORTS 8 +#define GMBUS_NUM_PORTS (GMBUS_PORT_DPD - GMBUS_PORT_SSC + 1) #define GMBUS1 0x5104 /* command/status */ #define GMBUS_SW_CLR_INT (1<<31) #define GMBUS_SW_RDY (1<<30) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index ae08a08..7396d7c 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -35,6 +35,20 @@ #include "i915_drm.h" #include "i915_drv.h"
+struct gmbus_port { + const char *name; + int reg; +}; + +static const struct gmbus_port gmbus_ports[] = { + { "ssc", GPIOB }, + { "vga", GPIOA }, + { "panel", GPIOC }, + { "dpc", GPIOD }, + { "dpb", GPIOE }, + { "dpd", GPIOF }, +}; + /* Intel GPIO access functions */
#define I2C_RISEFALL_TIME 10 @@ -166,29 +180,16 @@ intel_gpio_post_xfer(struct i2c_adapter *adapter) intel_i2c_quirk_set(dev_priv, false); }
-static bool +static void intel_gpio_setup(struct intel_gmbus *bus, u32 pin) { struct drm_i915_private *dev_priv = bus->dev_priv; - static const int map_pin_to_reg[] = { - 0, - GPIOB, - GPIOA, - GPIOC, - GPIOD, - GPIOE, - GPIOF, - 0, - }; struct i2c_algo_bit_data *algo;
- if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin]) - return false; - algo = &bus->bit_algo;
- bus->gpio_reg = map_pin_to_reg[pin]; - bus->gpio_reg += dev_priv->gpio_mmio_base; + /* -1 to map pin pair to gmbus index */ + bus->gpio_reg = dev_priv->gpio_mmio_base + gmbus_ports[pin - 1].reg;
bus->adapter.algo_data = algo; algo->setsda = set_data; @@ -200,8 +201,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin) algo->udelay = I2C_RISEFALL_TIME; algo->timeout = usecs_to_jiffies(2200); algo->data = bus; - - return true; }
static int @@ -328,12 +327,8 @@ timeout: I915_WRITE(GMBUS0 + reg_offset, 0);
/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */ - if (!bus->has_gpio) { - ret = -EIO; - } else { - bus->force_bit = true; - ret = i2c_bit_algo.master_xfer(adapter, msgs, num); - } + bus->force_bit = true; + ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
out: mutex_unlock(&dev_priv->gmbus_mutex); @@ -370,16 +365,6 @@ static const struct i2c_algorithm gmbus_algorithm = { */ int intel_setup_gmbus(struct drm_device *dev) { - static const char *names[GMBUS_NUM_PORTS] = { - "disabled", - "ssc", - "vga", - "panel", - "dpc", - "dpb", - "dpd", - "reserved", - }; struct drm_i915_private *dev_priv = dev->dev_private; int ret, i;
@@ -397,13 +382,14 @@ int intel_setup_gmbus(struct drm_device *dev)
for (i = 0; i < GMBUS_NUM_PORTS; i++) { struct intel_gmbus *bus = &dev_priv->gmbus[i]; + u32 port = i + 1; /* +1 to map gmbus index to pin pair */
bus->adapter.owner = THIS_MODULE; bus->adapter.class = I2C_CLASS_DDC; snprintf(bus->adapter.name, sizeof(bus->adapter.name), "i915 gmbus %s", - names[i]); + gmbus_ports[i].name);
bus->adapter.dev.parent = &dev->pdev->dev; bus->dev_priv = dev_priv; @@ -414,9 +400,9 @@ int intel_setup_gmbus(struct drm_device *dev) goto err;
/* By default use a conservative clock rate */ - bus->reg0 = i | GMBUS_RATE_100KHZ; + bus->reg0 = port | GMBUS_RATE_100KHZ;
- bus->has_gpio = intel_gpio_setup(bus, i); + intel_gpio_setup(bus, port); }
intel_i2c_reset(dev_priv->dev); @@ -444,8 +430,7 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit) { struct intel_gmbus *bus = to_intel_gmbus(adapter);
- if (bus->has_gpio) - bus->force_bit = force_bit; + bus->force_bit = force_bit; }
void intel_teardown_gmbus(struct drm_device *dev)
On Mon, Mar 26, 2012 at 10:26:43PM +0800, Daniel Kurtz wrote:
There is no GMBUS "disabled" port 0, nor "reserved" port 7. For the other 6 ports there is a fixed 1:1 mapping between pin pairs and gmbus ports, which means every real gmbus port has a gpio pin.
Given these realizations, clean up gmbus initialization.
Tested on Sandybridge (gen 6, PCH == CougarPoint) hardware.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
Nice cleanup. Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
On Mon, Mar 26, 2012 at 10:26:43PM +0800, Daniel Kurtz wrote:
There is no GMBUS "disabled" port 0, nor "reserved" port 7. For the other 6 ports there is a fixed 1:1 mapping between pin pairs and gmbus ports, which means every real gmbus port has a gpio pin.
Given these realizations, clean up gmbus initialization.
Tested on Sandybridge (gen 6, PCH == CougarPoint) hardware.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_reg.h | 2 +- drivers/gpu/drm/i915/intel_i2c.c | 63 ++++++++++++++----------------------- 3 files changed, 25 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h @@ -370,16 +365,6 @@ static const struct i2c_algorithm gmbus_algorithm = { */ int intel_setup_gmbus(struct drm_device *dev) {
- static const char *names[GMBUS_NUM_PORTS] = {
"disabled","ssc","vga","panel","dpc","dpb","dpd","reserved",- }; struct drm_i915_private *dev_priv = dev->dev_private; int ret, i;
@@ -397,13 +382,14 @@ int intel_setup_gmbus(struct drm_device *dev)
for (i = 0; i < GMBUS_NUM_PORTS; i++) { struct intel_gmbus *bus = &dev_priv->gmbus[i];
u32 port = i + 1; /* +1 to map gmbus index to pin pair */
On reconsideration you move around gmbus ports here, which will break things horribly. I've wondered a bit how that could possible work and noticed that you fix things up in the a later patch when introducing get_adapter.
This ordering breaks bisecting is a complete no-go. If you want to do this (I still think the refactor is nice) you need to introduce get_adapter first, then change the meaning of the array index in this patch while adjusting the lookup in the new get_adapter function.
Yours, Daniel
This memory is always allocated, and it is always a fixed size, so just allocate it along with the rest of the driver state.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/intel_i2c.c | 10 ---------- 2 files changed, 1 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 312dd4e..b132677 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -326,7 +326,7 @@ typedef struct drm_i915_private { /** gt_lock is also taken in irq contexts. */ struct spinlock gt_lock;
- struct intel_gmbus *gmbus; + struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
/** gmbus_mutex protects against concurrent usage of the single hw gmbus * controller on different i2c buses. */ diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 7396d7c..aa0670e 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -373,11 +373,6 @@ int intel_setup_gmbus(struct drm_device *dev) else dev_priv->gpio_mmio_base = 0;
- dev_priv->gmbus = kcalloc(GMBUS_NUM_PORTS, sizeof(struct intel_gmbus), - GFP_KERNEL); - if (dev_priv->gmbus == NULL) - return -ENOMEM; - mutex_init(&dev_priv->gmbus_mutex);
for (i = 0; i < GMBUS_NUM_PORTS; i++) { @@ -414,8 +409,6 @@ err: struct intel_gmbus *bus = &dev_priv->gmbus[i]; i2c_del_adapter(&bus->adapter); } - kfree(dev_priv->gmbus); - dev_priv->gmbus = NULL; return ret; }
@@ -445,7 +438,4 @@ void intel_teardown_gmbus(struct drm_device *dev) struct intel_gmbus *bus = &dev_priv->gmbus[i]; i2c_del_adapter(&bus->adapter); } - - kfree(dev_priv->gmbus); - dev_priv->gmbus = NULL; }
On Mon, Mar 26, 2012 at 10:26:44PM +0800, Daniel Kurtz wrote:
This memory is always allocated, and it is always a fixed size, so just allocate it along with the rest of the driver state.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
On Mon, Mar 26, 2012 at 10:26:44PM +0800, Daniel Kurtz wrote:
This memory is always allocated, and it is always a fixed size, so just allocate it along with the rest of the driver state.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
Instead of letting other modules directly access the ->gmbus array, introduce a new API, intel_gmbus_get_adapter(), to lookup an i2c_adapter for a given gmbus pin pair identifier. This API enables later refactoring of the gmbus pin pair list.
Note: The gmbus pin must be checked for validity before requesting the corresponding gmbus port. This is especially true when using a pin that has been read from VBIOS, as an improperly or uninitialied BIOS might indicate '0' for the 'disabled' pin pair, which doesn't actually exist. In the case of an invalid pin, the driver falls back to using a safer default pin pair.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ drivers/gpu/drm/i915/intel_bios.c | 4 ++-- drivers/gpu/drm/i915/intel_crt.c | 14 ++++++++------ drivers/gpu/drm/i915/intel_dvo.c | 6 +++--- drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++--- drivers/gpu/drm/i915/intel_i2c.c | 8 ++++++++ drivers/gpu/drm/i915/intel_lvds.c | 7 ++++--- drivers/gpu/drm/i915/intel_modes.c | 3 ++- drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- 9 files changed, 45 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b132677..64e0ae1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1346,6 +1346,13 @@ extern int i915_restore_state(struct drm_device *dev); /* intel_i2c.c */ extern int intel_setup_gmbus(struct drm_device *dev); extern void intel_teardown_gmbus(struct drm_device *dev); +extern inline bool intel_gmbus_is_port_valid(unsigned port) +{ + return (port >= GMBUS_PORT_SSC && port <= GMBUS_PORT_DPD); +} + +extern struct i2c_adapter *intel_gmbus_get_adapter( + struct drm_i915_private *dev_priv, unsigned port); extern void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed); extern void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit); extern inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index e4317da..871aa27 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -372,11 +372,11 @@ parse_general_definitions(struct drm_i915_private *dev_priv, if (block_size >= sizeof(*general)) { int bus_pin = general->crt_ddc_gmbus_pin; DRM_DEBUG_KMS("crt_ddc_bus_pin: %d\n", bus_pin); - if (bus_pin >= 1 && bus_pin <= 6) + if (intel_gmbus_is_port_valid(bus_pin)) dev_priv->crt_ddc_pin = bus_pin; } else { DRM_DEBUG_KMS("BDB_GD too small (%d). Invalid.\n", - block_size); + block_size); } } } diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 4d3d736..d54d232 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -278,9 +278,10 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector) if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) { struct edid *edid; bool is_digital = false; + struct i2c_adapter *i2c;
- edid = drm_get_edid(connector, - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); + i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin); + edid = drm_get_edid(connector, i2c); /* * This may be a DVI-I connector with a shared DDC * link between analog and digital outputs, so we @@ -483,15 +484,16 @@ static int intel_crt_get_modes(struct drm_connector *connector) struct drm_device *dev = connector->dev; struct drm_i915_private *dev_priv = dev->dev_private; int ret; + struct i2c_adapter *i2c;
- ret = intel_ddc_get_modes(connector, - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); + i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin); + ret = intel_ddc_get_modes(connector, i2c); if (ret || !IS_G4X(dev)) return ret;
/* Try to probe digital port for output in DVI-I -> VGA mode. */ - return intel_ddc_get_modes(connector, - &dev_priv->gmbus[GMBUS_PORT_DPB].adapter); + i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB); + return intel_ddc_get_modes(connector, i2c); }
static int intel_crt_set_property(struct drm_connector *connector, diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c index 020a7d7..60ba50b9 100644 --- a/drivers/gpu/drm/i915/intel_dvo.c +++ b/drivers/gpu/drm/i915/intel_dvo.c @@ -243,7 +243,7 @@ static int intel_dvo_get_modes(struct drm_connector *connector) * that's not the case. */ intel_ddc_get_modes(connector, - &dev_priv->gmbus[GMBUS_PORT_DPC].adapter); + intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPC)); if (!list_empty(&connector->probed_modes)) return 1;
@@ -375,7 +375,7 @@ void intel_dvo_init(struct drm_device *dev) * special cases, but otherwise default to what's defined * in the spec. */ - if (dvo->gpio != 0) + if (intel_gmbus_is_port_valid(dvo->gpio)) gpio = dvo->gpio; else if (dvo->type == INTEL_DVO_CHIP_LVDS) gpio = GMBUS_PORT_SSC; @@ -386,7 +386,7 @@ void intel_dvo_init(struct drm_device *dev) * It appears that everything is on GPIOE except for panels * on i830 laptops, which are on GPIOB (DVOA). */ - i2c = &dev_priv->gmbus[gpio].adapter; + i2c = intel_gmbus_get_adapter(dev_priv, gpio);
intel_dvo->dev = *dvo; if (!dvo->dev_ops->init(&intel_dvo->dev, i2c)) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index cae3e5f..1d00f61 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -334,7 +334,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) intel_hdmi->has_hdmi_sink = false; intel_hdmi->has_audio = false; edid = drm_get_edid(connector, - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); + intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus));
if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) { @@ -367,7 +368,8 @@ static int intel_hdmi_get_modes(struct drm_connector *connector) */
return intel_ddc_get_modes(connector, - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); + intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus)); }
static bool @@ -379,7 +381,8 @@ intel_hdmi_detect_audio(struct drm_connector *connector) bool has_audio = false;
edid = drm_get_edid(connector, - &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); + intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus)); if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) has_audio = drm_detect_monitor_audio(edid); diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index aa0670e..a04f773 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -412,6 +412,14 @@ err: return ret; }
+struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv, + unsigned port) +{ + BUG_ON(!intel_gmbus_is_port_valid(port)); + /* -1 to map pin pair to gmbus index */ + return &dev_priv->gmbus[port - 1].adapter; +} + void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed) { struct intel_gmbus *bus = to_intel_gmbus(adapter); diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index c5c0973..4f92a11 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -837,8 +837,8 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev, child->device_type != DEVICE_TYPE_LFP) continue;
- if (child->i2c_pin) - *i2c_pin = child->i2c_pin; + if (intel_gmbus_is_port_valid(child->i2c_pin)) + *i2c_pin = child->i2c_pin;
/* However, we cannot trust the BIOS writers to populate * the VBT correctly. Since LVDS requires additional @@ -979,7 +979,8 @@ bool intel_lvds_init(struct drm_device *dev) * preferred mode is the right one. */ intel_lvds->edid = drm_get_edid(connector, - &dev_priv->gmbus[pin].adapter); + intel_gmbus_get_adapter(dev_priv, + pin)); if (intel_lvds->edid) { if (drm_add_edid_modes(connector, intel_lvds->edid)) { diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index 2978a3f..cc682a0 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -55,7 +55,8 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus) } };
- return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 2) == 2; + return i2c_transfer(intel_gmbus_get_adapter(dev_priv, ddc_bus), + msgs, 2) == 2; }
/** diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 70fb275..830c8b5 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1254,7 +1254,8 @@ intel_sdvo_get_analog_edid(struct drm_connector *connector) struct drm_i915_private *dev_priv = connector->dev->dev_private;
return drm_get_edid(connector, - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); + intel_gmbus_get_adapter(dev_priv, + dev_priv->crt_ddc_pin)); }
enum drm_connector_status @@ -1922,12 +1923,12 @@ intel_sdvo_select_i2c_bus(struct drm_i915_private *dev_priv, if (mapping->initialized) pin = mapping->i2c_pin;
- if (pin < GMBUS_NUM_PORTS) { - sdvo->i2c = &dev_priv->gmbus[pin].adapter; + if (intel_gmbus_is_port_valid(pin)) { + sdvo->i2c = intel_gmbus_get_adapter(dev_priv, pin); intel_gmbus_set_speed(sdvo->i2c, GMBUS_RATE_1MHZ); intel_gmbus_force_bit(sdvo->i2c, true); } else { - sdvo->i2c = &dev_priv->gmbus[GMBUS_PORT_DPB].adapter; + sdvo->i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB); } }
On Mon, Mar 26, 2012 at 10:26:45PM +0800, Daniel Kurtz wrote:
Instead of letting other modules directly access the ->gmbus array, introduce a new API, intel_gmbus_get_adapter(), to lookup an i2c_adapter for a given gmbus pin pair identifier. This API enables later refactoring of the gmbus pin pair list.
Note: The gmbus pin must be checked for validity before requesting the corresponding gmbus port. This is especially true when using a pin that has been read from VBIOS, as an improperly or uninitialied BIOS might indicate '0' for the 'disabled' pin pair, which doesn't actually exist. In the case of an invalid pin, the driver falls back to using a safer default pin pair.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
With the cavet of the patch ordering issue, this looks ok. One comment below.
drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ drivers/gpu/drm/i915/intel_bios.c | 4 ++-- drivers/gpu/drm/i915/intel_crt.c | 14 ++++++++------ drivers/gpu/drm/i915/intel_dvo.c | 6 +++--- drivers/gpu/drm/i915/intel_hdmi.c | 9 ++++++--- drivers/gpu/drm/i915/intel_i2c.c | 8 ++++++++ drivers/gpu/drm/i915/intel_lvds.c | 7 ++++--- drivers/gpu/drm/i915/intel_modes.c | 3 ++- drivers/gpu/drm/i915/intel_sdvo.c | 9 +++++---- 9 files changed, 45 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b132677..64e0ae1 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1346,6 +1346,13 @@ extern int i915_restore_state(struct drm_device *dev); /* intel_i2c.c */ extern int intel_setup_gmbus(struct drm_device *dev); extern void intel_teardown_gmbus(struct drm_device *dev); +extern inline bool intel_gmbus_is_port_valid(unsigned port) +{
- return (port >= GMBUS_PORT_SSC && port <= GMBUS_PORT_DPD);
+}
+extern struct i2c_adapter *intel_gmbus_get_adapter(
struct drm_i915_private *dev_priv, unsigned port);extern void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed); extern void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit); extern inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index e4317da..871aa27 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -372,11 +372,11 @@ parse_general_definitions(struct drm_i915_private *dev_priv, if (block_size >= sizeof(*general)) { int bus_pin = general->crt_ddc_gmbus_pin; DRM_DEBUG_KMS("crt_ddc_bus_pin: %d\n", bus_pin);
if (bus_pin >= 1 && bus_pin <= 6)
if (intel_gmbus_is_port_valid(bus_pin)) dev_priv->crt_ddc_pin = bus_pin;
block_size);
block_size);} diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 4d3d736..d54d232 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -278,9 +278,10 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector) if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) { struct edid *edid; bool is_digital = false;
struct i2c_adapter *i2c;
edid = drm_get_edid(connector,&dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);edid = drm_get_edid(connector, i2c);
- This may be a DVI-I connector with a shared DDC
- link between analog and digital outputs, so we
@@ -483,15 +484,16 @@ static int intel_crt_get_modes(struct drm_connector *connector) struct drm_device *dev = connector->dev; struct drm_i915_private *dev_priv = dev->dev_private; int ret;
- struct i2c_adapter *i2c;
- ret = intel_ddc_get_modes(connector,
&dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);
ret = intel_ddc_get_modes(connector, i2c); if (ret || !IS_G4X(dev)) return ret;
/* Try to probe digital port for output in DVI-I -> VGA mode. */
- return intel_ddc_get_modes(connector,
&dev_priv->gmbus[GMBUS_PORT_DPB].adapter);
- i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB);
- return intel_ddc_get_modes(connector, i2c);
}
static int intel_crt_set_property(struct drm_connector *connector, diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c index 020a7d7..60ba50b9 100644 --- a/drivers/gpu/drm/i915/intel_dvo.c +++ b/drivers/gpu/drm/i915/intel_dvo.c @@ -243,7 +243,7 @@ static int intel_dvo_get_modes(struct drm_connector *connector) * that's not the case. */ intel_ddc_get_modes(connector,
&dev_priv->gmbus[GMBUS_PORT_DPC].adapter);
intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPC));@@ -375,7 +375,7 @@ void intel_dvo_init(struct drm_device *dev) * special cases, but otherwise default to what's defined * in the spec. */
if (dvo->gpio != 0)
if (intel_gmbus_is_port_valid(dvo->gpio)) gpio = dvo->gpio;@@ -386,7 +386,7 @@ void intel_dvo_init(struct drm_device *dev) * It appears that everything is on GPIOE except for panels * on i830 laptops, which are on GPIOB (DVOA). */
i2c = &dev_priv->gmbus[gpio].adapter;
i2c = intel_gmbus_get_adapter(dev_priv, gpio);intel_dvo->dev = *dvo; if (!dvo->dev_ops->init(&intel_dvo->dev, i2c))
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index cae3e5f..1d00f61 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -334,7 +334,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) intel_hdmi->has_hdmi_sink = false; intel_hdmi->has_audio = false; edid = drm_get_edid(connector,
&dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
intel_gmbus_get_adapter(dev_priv,intel_hdmi->ddc_bus));if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) {
@@ -367,7 +368,8 @@ static int intel_hdmi_get_modes(struct drm_connector *connector) */
return intel_ddc_get_modes(connector,
&dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
intel_gmbus_get_adapter(dev_priv,intel_hdmi->ddc_bus));}
static bool @@ -379,7 +381,8 @@ intel_hdmi_detect_audio(struct drm_connector *connector) bool has_audio = false;
edid = drm_get_edid(connector,
&dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
intel_gmbus_get_adapter(dev_priv,intel_hdmi->ddc_bus));diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index aa0670e..a04f773 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -412,6 +412,14 @@ err: return ret; }
+struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
unsigned port)+{
- BUG_ON(!intel_gmbus_is_port_valid(port));
WARN_ON highly preferred, safe when it's guaranteed that the code will later on blow up anyway (e.g. due to a NULL pointer deref).
- /* -1 to map pin pair to gmbus index */
- return &dev_priv->gmbus[port - 1].adapter;
+}
void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed) { struct intel_gmbus *bus = to_intel_gmbus(adapter); diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index c5c0973..4f92a11 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -837,8 +837,8 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev, child->device_type != DEVICE_TYPE_LFP) continue;
if (child->i2c_pin)*i2c_pin = child->i2c_pin;
if (intel_gmbus_is_port_valid(child->i2c_pin))*i2c_pin = child->i2c_pin;/* However, we cannot trust the BIOS writers to populate
- the VBT correctly. Since LVDS requires additional
@@ -979,7 +979,8 @@ bool intel_lvds_init(struct drm_device *dev) * preferred mode is the right one. */ intel_lvds->edid = drm_get_edid(connector,
&dev_priv->gmbus[pin].adapter);
intel_gmbus_get_adapter(dev_priv,pin));diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index 2978a3f..cc682a0 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -55,7 +55,8 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus) } };
- return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 2) == 2;
- return i2c_transfer(intel_gmbus_get_adapter(dev_priv, ddc_bus),
msgs, 2) == 2;}
/** diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 70fb275..830c8b5 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1254,7 +1254,8 @@ intel_sdvo_get_analog_edid(struct drm_connector *connector) struct drm_i915_private *dev_priv = connector->dev->dev_private;
return drm_get_edid(connector,
&dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
intel_gmbus_get_adapter(dev_priv,dev_priv->crt_ddc_pin));}
enum drm_connector_status @@ -1922,12 +1923,12 @@ intel_sdvo_select_i2c_bus(struct drm_i915_private *dev_priv, if (mapping->initialized) pin = mapping->i2c_pin;
- if (pin < GMBUS_NUM_PORTS) {
sdvo->i2c = &dev_priv->gmbus[pin].adapter;
- if (intel_gmbus_is_port_valid(pin)) {
sdvo->i2c = intel_gmbus_get_adapter(dev_priv, pin);
sdvo->i2c = &dev_priv->gmbus[GMBUS_PORT_DPB].adapter;
sdvo->i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB);}
-- 1.7.7.3
A common method of probing an i2c bus is trying to do a zero-length write. Handle this case by checking the length first before decrementing it.
This is actually important, since attempting a zero-length write is one of the ways that i2cdetect and i2c_new_probed_device detect whether there is device present on the bus with a given address.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index a04f773..c467a2e 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -258,9 +258,10 @@ gmbus_xfer(struct i2c_adapter *adapter, u32 val, loop;
val = loop = 0; - do { - val |= *buf++ << (8 * loop); - } while (--len && ++loop < 4); + while (len && loop < 4) { + val |= *buf++ << (8 * loop++); + len -= 1; + }
I915_WRITE(GMBUS3 + reg_offset, val); I915_WRITE(GMBUS1 + reg_offset,
The GMBUS controller can report a NAK condition while a transaction is still active. If the driver is fast enough, and the bus is slow enough, the driver may clear the NAK condition while the controller is still busy, resulting in a confused GMBUS controller. This will leave the controller in a bad state such that the next transaction may fail.
Also, return -ENXIO if a device NAKs a transaction.
Note: this patch also refactors gmbus_xfer to remove the "done" label.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 41 ++++++++++++++++++++++++++++--------- 1 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index c467a2e..9d5d286 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -300,26 +300,47 @@ gmbus_xfer(struct i2c_adapter *adapter, goto clear_err; }
- goto done; + /* 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)) + DRM_INFO("GMBUS [%s] timed out waiting for idle\n", + adapter->name); + I915_WRITE(GMBUS0 + reg_offset, 0); + ret = i; + goto out;
clear_err: + /* + * Wait for bus to IDLE before clearing NAK. + * If we clear the NAK while bus is still active, then it will stay + * active and the next transaction may fail. + */ + if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, + 10)) + DRM_INFO("GMBUS [%s] timed out after NAK\n", adapter->name); + /* Toggle the Software Clear Interrupt bit. This has the effect * of resetting the GMBUS controller and so clearing the * BUS_ERROR raised by the slave's NAK. */ I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT); I915_WRITE(GMBUS1 + reg_offset, 0); + I915_WRITE(GMBUS0 + reg_offset, 0);
-done: - /* 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. + DRM_DEBUG_DRIVER("GMBUS [%s] NAK for addr: %04x %c(%d)\n", + adapter->name, msgs[i].addr, + (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len); + + /* + * If no ACK is received during the address phase of a transaction, + * the adapter must report -ENXIO. + * It is not clear what to return if no ACK is received at other times. + * So, we always return -ENXIO in all NAK cases, to ensure we send + * it at least during the one case that is specified. */ - if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10)) - DRM_INFO("GMBUS [%s] timed out waiting for idle\n", - bus->adapter.name); - I915_WRITE(GMBUS0 + reg_offset, 0); - ret = i; + ret = -ENXIO; goto out;
timeout:
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.
This patch also takes advantage (in the write path) of the double-buffered GMBUS3 data register by writing two 4-byte words before the first wait for HW_RDY.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 34 +++++++++++++++++++--------------- 1 files changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 9d5d286..7396d6e 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -212,7 +212,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);
@@ -232,7 +233,6 @@ gmbus_xfer(struct i2c_adapter *adapter, if (msgs[i].flags & I2C_M_RD) { I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_WAIT | - (i + 1 == num ? GMBUS_CYCLE_STOP : 0) | (len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_READ | GMBUS_SW_RDY); @@ -266,21 +266,12 @@ gmbus_xfer(struct i2c_adapter *adapter, I915_WRITE(GMBUS3 + reg_offset, val); I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_WAIT | - (i + 1 == num ? GMBUS_CYCLE_STOP : 0) | (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_WRITE | GMBUS_SW_RDY); POSTING_READ(GMBUS2 + reg_offset);
while (len) { - if (wait_for(I915_READ(GMBUS2 + reg_offset) & - (GMBUS_SATOER | GMBUS_HW_RDY), - 50)) - goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & - GMBUS_SATOER) - goto clear_err; - val = loop = 0; do { val |= *buf++ << (8 * loop); @@ -288,11 +279,18 @@ gmbus_xfer(struct i2c_adapter *adapter,
I915_WRITE(GMBUS3 + reg_offset, val); POSTING_READ(GMBUS2 + reg_offset); + + if (wait_for(I915_READ(GMBUS2 + reg_offset) & + (GMBUS_SATOER | GMBUS_HW_RDY), + 50)) + goto timeout; + if (I915_READ(GMBUS2 + reg_offset) & + GMBUS_SATOER) + goto clear_err; } }
- if (i + 1 < num && - wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50)) goto timeout; @@ -300,15 +298,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:
It is very common for an i2c device to require a small 1 or 2 byte write followed by a read. For example, when reading from an i2c EEPROM it is common to write and address, offset or index followed by a reading some values.
The i915 gmbus controller provides a special "INDEX" cycle for performing such a small write followed by a read. The INDEX can be either one or two bytes long. The advantage of using such a cycle is that the CPU has slightly less work to do once the read with INDEX cycle is started.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org --- drivers/gpu/drm/i915/intel_i2c.c | 32 ++++++++++++++++++++++++++++++-- 1 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 7396d6e..9619bde 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -227,12 +227,40 @@ gmbus_xfer(struct i2c_adapter *adapter, I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
for (i = 0; i < num; i++) { - u16 len = msgs[i].len; - u8 *buf = msgs[i].buf; + u16 len; + u8 *buf; + u32 gmbus5 = 0; + u32 gmbus1_index = 0; + + /* + * The gmbus controller can combine a 1 or 2 byte write with a + * read that immediately follows it by using an "INDEX" cycle. + */ + if (i + 1 < num && + !(msgs[i].flags & I2C_M_RD) && + (msgs[i + 1].flags & I2C_M_RD) && + msgs[i].len <= 2) { + if (msgs[i].len == 2) + gmbus5 = GMBUS_2BYTE_INDEX_EN | + msgs[i].buf[1] | + (msgs[i].buf[0] << 8); + if (msgs[i].len == 1) + gmbus1_index = GMBUS_CYCLE_INDEX | + (msgs[i].buf[0] << + GMBUS_SLAVE_INDEX_SHIFT); + i += 1; /* set i to the index of the read xfer */ + } + + len = msgs[i].len; + buf = msgs[i].buf; + + /* GMBUS5 holds 16-bit index, but must be 0 if not used */ + I915_WRITE(GMBUS5 + reg_offset, gmbus5);
if (msgs[i].flags & I2C_M_RD) { I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_WAIT | + gmbus1_index | (len << GMBUS_BYTE_COUNT_SHIFT) | (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) | GMBUS_SLAVE_READ | GMBUS_SW_RDY);
Save the GMBUS2 value read while polling for state changes, and then reuse this value when determining for which reason the loops were exited. This is a small optimization which saves a couple of bus accesses for memory mapped IO registers.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org Reviewed-by: Chris Wilson chris@chris-wilson.co.uk --- drivers/gpu/drm/i915/intel_i2c.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index 9619bde..7674c89 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -214,6 +214,7 @@ gmbus_xfer(struct i2c_adapter *adapter, struct drm_i915_private *dev_priv = bus->dev_priv; int i, reg_offset; int ret = 0; + u32 gmbus2 = 0;
mutex_lock(&dev_priv->gmbus_mutex);
@@ -268,12 +269,12 @@ gmbus_xfer(struct i2c_adapter *adapter, do { u32 val, loop = 0;
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for((gmbus2 = I915_READ(GMBUS2 + + reg_offset)) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & - GMBUS_SATOER) + if (gmbus2 & GMBUS_SATOER) goto clear_err;
val = I915_READ(GMBUS3 + reg_offset); @@ -308,21 +309,21 @@ gmbus_xfer(struct i2c_adapter *adapter, I915_WRITE(GMBUS3 + reg_offset, val); POSTING_READ(GMBUS2 + reg_offset);
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for((gmbus2 = I915_READ(GMBUS2 + + reg_offset)) & (GMBUS_SATOER | GMBUS_HW_RDY), 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & - GMBUS_SATOER) + if (gmbus2 & GMBUS_SATOER) goto clear_err; } }
- if (wait_for(I915_READ(GMBUS2 + reg_offset) & + if (wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50)) goto timeout; - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) + if (gmbus2 & GMBUS_SATOER) goto clear_err; }
On Mon, Mar 26, 2012 at 10:26:39PM +0800, Daniel Kurtz wrote:
This patchset addresses a couple of issues with the i915 gmbus implementation:
- fixes misassigned pin port pair for HDMI-D
- fixes write transactions when they are the only transaction requested (including large >4-byte writes) by terminating every transaction with a WAIT cycle.
- returns -ENXIO and -ETIMEDOUT as appropriate so upper layers can handled i2c transaction failures
- optimizes the typical read transaction case by using the INDEX cycle
v3:
- rebased onto Daniel Vetter's drm-intel-next-queued branch at git://people.freedesktop.org/~danvet/drm-intel
- replace intel_i2c_quirk_xfer with pre/post_xfer i2c routines
- pre-allocate gmbus array
- drop interrupt approach since I could not make it stable, probably due to difficulty in clearing and resetting the GMBUS interrupt which is buffered behind the SDE's PCH interrupt.
- Fix zero-length writes
- Wait for IDLE before clearing NAK
Ok, I've found a few more things to complain about ;-) I haven't looked to closely add the later patches that fix up the actual gmbus code, but on a quick read I don't see any issues there - I'll cross check the code somewhen later with Bspec.
Yours, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Kurtz -
Daniel Vetter