[PATCH 0/9] drm/i915/intel_i2c: fix gmbus writes and related issues

[PATCH 0/9] drm/i915/intel_i2c: fix gmbus writes and related issues

[Daniel Kurtz]

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)
  * 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

The patchset should apply cleanly onto linus/master.
It is inspired by, but completely supercedes, a similar patch submitted
recently by Benson Leung (bleung@chromium.org).
I'd be happy to rebase on a different tree if necessary.

Note: 
There is one more patch in the set which I have not included here.  It enables
the PCH GMBUS interrupt and makes use of it to eliminate the very slow
'wait_for' polling loop.  This patch was not included, however, because there
is at least one case I have discovered where the i915 driver issues a GMBUS
transaction AFTER its interrupts have been initialized, but BEFORE it they have
been enabled.  Both the gmbus transaction and irq initialization occur during
i915_load_modeset_init(), but the gmbus transaction happens first, during:
  intel_modeset_init()
    intel_setup_outputs()
      intel_lvds_init()
        drm_get_edid()
Is there a way to switch the order of these to events?
Or does it make more sense for the gmbus driver to check a "bool irq_enabled",
and choose whether to poll or use the GMBUS interrupt?
... or both?


Daniel Kurtz (9):
  drm/i915/intel_i2c: cleanup
  drm/i915/intel_i2c: assign HDMI port D to pin pair 6
  drm/i915/intel_i2c: refactor using intel_gmbus_get_bus
  drm/i915/intel_i2c: cleanup gmbus/gpio pin assignments
  drm/i915/intel_i2c: add locking around i2c algorithm accesses
  drm/i915/intel_i2c: return -ENXIO for device 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    |    5 +-
 drivers/gpu/drm/i915/i915_reg.h    |    5 +-
 drivers/gpu/drm/i915/intel_bios.c  |   10 ++-
 drivers/gpu/drm/i915/intel_crt.c   |   13 ++--
 drivers/gpu/drm/i915/intel_drv.h   |    3 +-
 drivers/gpu/drm/i915/intel_dvo.c   |    4 +-
 drivers/gpu/drm/i915/intel_hdmi.c  |   29 +++---
 drivers/gpu/drm/i915/intel_i2c.c   |  175 +++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_lvds.c  |    2 +-
 drivers/gpu/drm/i915/intel_modes.c |    6 +-
 drivers/gpu/drm/i915/intel_sdvo.c  |   10 +--
 11 files changed, 165 insertions(+), 97 deletions(-)

-- 
1.7.7.3

[PATCH 1/9] drm/i915/intel_i2c: cleanup

[Daniel Kurtz]

80 col, spaces around operators and other basic cleanup.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30cccc..a94e383 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -236,8 +236,9 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	int i, reg_offset;
 
 	if (bus->force_bit)
-		return intel_i2c_quirk_xfer(dev_priv,
-					    bus->force_bit, msgs, num);
+		return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs,
+					    num);
+
 
 	reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
 
@@ -253,11 +254,13 @@ 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)
 					goto clear_err;
@@ -282,10 +285,12 @@ 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)
 					goto clear_err;
@@ -296,11 +301,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;
-- 
1.7.7.3

Re: [PATCH 1/9] drm/i915/intel_i2c: cleanup

[Chris Wilson]

On Wed,  7 Mar 2012 19:50:42 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 80 col, spaces around operators and other basic cleanup.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   24 ++++++++++++++++--------
>  1 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d30cccc..a94e383 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -236,8 +236,9 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	int i, reg_offset;
>  
>  	if (bus->force_bit)
> -		return intel_i2c_quirk_xfer(dev_priv,
> -					    bus->force_bit, msgs, num);
> +		return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs,
> +					    num);
> +
NAK just on that around. The new line was put there for a reason as
there is a distinction in the types of parameters, it visually grouped
the control arguments from the object.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/9] drm/i915/intel_i2c: assign HDMI port D to pin pair 6

[Daniel Kurtz]

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 03c53fc..56af0df 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -697,9 +697,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 a94e383..45ce1d2 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -157,8 +157,8 @@ intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin)
 		GPIOC,
 		GPIOD,
 		GPIOE,
-		0,
 		GPIOF,
+		0,
 	};
 	struct intel_gpio *gpio;
 
@@ -377,8 +377,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;
-- 
1.7.7.3

Re: [PATCH 2/9] drm/i915/intel_i2c: assign HDMI port D to pin pair 6

[Chris Wilson]

On Wed,  7 Mar 2012 19:50:43 +0800, Daniel Kurtz <djkurtz@chromium.org> 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

The problem is I took those definitions from the gen2 specs, and munged
in the obvious changes with gen3... And it appears that the GPIO pin
assignment versus GMBUS has changed over the years. And so the can of
worms is opened!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/9] drm/i915/intel_i2c: assign HDMI port D to pin pair 6

[Daniel Kurtz]

On Wed, Mar 7, 2012 at 9:23 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> On Wed,  7 Mar 2012 19:50:43 +0800, Daniel Kurtz <djkurtz@chromium.org>
> 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
>
> The problem is I took those definitions from the gen2 specs, and munged
> in the obvious changes with gen3... And it appears that the GPIO pin
> assignment versus GMBUS has changed over the years. And so the can of
> worms is opened!

I'm not sure what exactly you mean by gen2 and gen3 specs, but, as far
as I can tell, the GPIO pin assignment is the same for both Cougar
Point (Sandy Bridge) [1] and Ibex Peak (?) [2].  Although, the
documentation is a bit confusing and it is difficult to tell.

[1] http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf
[2] http://intellinuxgraphics.org/IHD_OS_Vol3_Part3r2.pdf

What do you mean exactly by gen2 & gen3?  Can you point to docs?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/9] drm/i915/intel_i2c: assign HDMI port D to pin pair 6

[Chris Wilson]

On Thu, 8 Mar 2012 02:02:03 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> I'm not sure what exactly you mean by gen2 and gen3 specs, but, as far as I
> can tell, the GPIO pin assignment is the same for both Cougar Point (Sandy
> Bridge) [1] and Ibex Peak (?) [2].  Although, the documentation is a bit
> confusing and it is difficult to tell.
> 
> [1] http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf
> [2] http://intellinuxgraphics.org/IHD_OS_Vol3_Part3r2.pdf
> 
> What do you mean exactly by gen2 & gen3?  Can you point to docs?
gen2 == 830gm,845g,855gm,865g
gen3 == 915g,945g,PineView

Those docs have not been completely scrubbed and made public yet.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 3/9] drm/i915/intel_i2c: refactor using intel_gmbus_get_bus

[Daniel Kurtz]

Instead of letting other modules directly access the ->gmbus array,
introduce a new API, intel_gmbus_get_bus(), to lookup an i2c_adapter for
a given gmbus pin pair identifier.  This API enables later refactoring
of the gmbus pin pair list.

Note: It is critical that intel_setup_gmbus() gets called before
intel_gmbus_get_bus().

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/i915_drv.h    |    4 +++-
 drivers/gpu/drm/i915/intel_bios.c  |   10 ++++++----
 drivers/gpu/drm/i915/intel_crt.c   |   13 ++++++-------
 drivers/gpu/drm/i915/intel_drv.h   |    3 ++-
 drivers/gpu/drm/i915/intel_dvo.c   |    4 ++--
 drivers/gpu/drm/i915/intel_hdmi.c  |   29 ++++++++++++++---------------
 drivers/gpu/drm/i915/intel_i2c.c   |    6 ++++++
 drivers/gpu/drm/i915/intel_lvds.c  |    2 +-
 drivers/gpu/drm/i915/intel_modes.c |    6 +++---
 drivers/gpu/drm/i915/intel_sdvo.c  |   10 ++++------
 10 files changed, 47 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9689ca3..64d1276 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -391,7 +391,7 @@ typedef struct drm_i915_private {
 
 	struct notifier_block lid_notifier;
 
-	int crt_ddc_pin;
+	struct i2c_adapter *crt_ddc;
 	struct drm_i915_fence_reg fence_regs[I915_MAX_NUM_FENCES]; /* assume 965 */
 	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */
 	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
@@ -1274,6 +1274,8 @@ 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 struct i2c_adapter *intel_gmbus_get_bus(
+		struct drm_i915_private *dev_priv, int pin);
 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 63880e2..16456f7 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -330,12 +330,14 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
 		u16 block_size = get_blocksize(general);
 		if (block_size >= sizeof(*general)) {
 			int bus_pin = general->crt_ddc_gmbus_pin;
+			struct i2c_adapter *i2c;
 			DRM_DEBUG_KMS("crt_ddc_bus_pin: %d\n", bus_pin);
-			if (bus_pin >= 1 && bus_pin <= 6)
-				dev_priv->crt_ddc_pin = bus_pin;
+			i2c = intel_gmbus_get_bus(dev_priv, bus_pin);
+			if (i2c)
+				dev_priv->crt_ddc = i2c;
 		} else {
 			DRM_DEBUG_KMS("BDB_GD too small (%d). Invalid.\n",
-				  block_size);
+				      block_size);
 		}
 	}
 }
@@ -599,7 +601,7 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
-	dev_priv->crt_ddc_pin = GMBUS_PORT_VGADDC;
+	dev_priv->crt_ddc = intel_gmbus_get_bus(dev_priv, GMBUS_PORT_VGADDC);
 
 	/* LFP panel data */
 	dev_priv->lvds_dither = 1;
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index dd729d4..71d6a50 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -275,12 +275,11 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 	if (crt->base.type != INTEL_OUTPUT_ANALOG)
 		return false;
 
-	if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) {
+	if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc)) {
 		struct edid *edid;
 		bool is_digital = false;
 
-		edid = drm_get_edid(connector,
-			&dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
+		edid = drm_get_edid(connector, dev_priv->crt_ddc);
 		/*
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
@@ -484,14 +483,14 @@ static int intel_crt_get_modes(struct drm_connector *connector)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
-	ret = intel_ddc_get_modes(connector,
-				 &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
+	ret = intel_ddc_get_modes(connector, dev_priv->crt_ddc);
 	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);
+	return intel_ddc_get_modes(
+			connector,
+			intel_gmbus_get_bus(dev_priv, GMBUS_PORT_DPB));
 }
 
 static int intel_crt_set_property(struct drm_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1348705..7538f2e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -285,7 +285,8 @@ struct intel_fbc_work {
 };
 
 int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
-extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus);
+extern bool intel_ddc_probe(struct intel_encoder *intel_encoder,
+			    struct i2c_adapter *adapter);
 
 extern void intel_attach_force_audio_property(struct drm_connector *connector);
 extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 6eda1b5..5c2174b 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -244,7 +244,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_bus(dev_priv, GMBUS_PORT_DPC));
 	if (!list_empty(&connector->probed_modes))
 		return 1;
 
@@ -387,7 +387,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_bus(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 64541f7..87015bf 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -40,7 +40,7 @@
 struct intel_hdmi {
 	struct intel_encoder base;
 	u32 sdvox_reg;
-	int ddc_bus;
+	struct i2c_adapter *ddc_bus;
 	uint32_t color_range;
 	bool has_hdmi_sink;
 	bool has_audio;
@@ -327,14 +327,12 @@ static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 	struct edid *edid;
 	enum drm_connector_status status = connector_status_disconnected;
 
 	intel_hdmi->has_hdmi_sink = false;
 	intel_hdmi->has_audio = false;
-	edid = drm_get_edid(connector,
-			    &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
+	edid = drm_get_edid(connector, intel_hdmi->ddc_bus);
 
 	if (edid) {
 		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
@@ -357,26 +355,22 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 static int intel_hdmi_get_modes(struct drm_connector *connector)
 {
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 
 	/* We should parse the EDID data and find out if it's an HDMI sink so
 	 * we can send audio to it.
 	 */
 
-	return intel_ddc_get_modes(connector,
-				   &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
+	return intel_ddc_get_modes(connector, intel_hdmi->ddc_bus);
 }
 
 static bool
 intel_hdmi_detect_audio(struct drm_connector *connector)
 {
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 	struct edid *edid;
 	bool has_audio = false;
 
-	edid = drm_get_edid(connector,
-			    &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
+	edid = drm_get_edid(connector, intel_hdmi->ddc_bus);
 	if (edid) {
 		if (edid->input & DRM_EDID_INPUT_DIGITAL)
 			has_audio = drm_detect_monitor_audio(edid);
@@ -521,23 +515,28 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 	/* Set up the DDC bus. */
 	if (sdvox_reg == SDVOB) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIB_CLONE_BIT);
-		intel_hdmi->ddc_bus = GMBUS_PORT_DPB;
+		intel_hdmi->ddc_bus = intel_gmbus_get_bus(dev_priv,
+							  GMBUS_PORT_DPB);
 		dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS;
 	} else if (sdvox_reg == SDVOC) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIC_CLONE_BIT);
-		intel_hdmi->ddc_bus = GMBUS_PORT_DPC;
+		intel_hdmi->ddc_bus = intel_gmbus_get_bus(dev_priv,
+							  GMBUS_PORT_DPC);
 		dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS;
 	} else if (sdvox_reg == HDMIB) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMID_CLONE_BIT);
-		intel_hdmi->ddc_bus = GMBUS_PORT_DPB;
+		intel_hdmi->ddc_bus = intel_gmbus_get_bus(dev_priv,
+							  GMBUS_PORT_DPB);
 		dev_priv->hotplug_supported_mask |= HDMIB_HOTPLUG_INT_STATUS;
 	} else if (sdvox_reg == HDMIC) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIE_CLONE_BIT);
-		intel_hdmi->ddc_bus = GMBUS_PORT_DPC;
+		intel_hdmi->ddc_bus = intel_gmbus_get_bus(dev_priv,
+							  GMBUS_PORT_DPC);
 		dev_priv->hotplug_supported_mask |= HDMIC_HOTPLUG_INT_STATUS;
 	} else if (sdvox_reg == HDMID) {
 		intel_encoder->clone_mask = (1 << INTEL_HDMIF_CLONE_BIT);
-		intel_hdmi->ddc_bus = GMBUS_PORT_DPD;
+		intel_hdmi->ddc_bus = intel_gmbus_get_bus(dev_priv,
+							  GMBUS_PORT_DPD);
 		dev_priv->hotplug_supported_mask |= HDMID_HOTPLUG_INT_STATUS;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 45ce1d2..dd8c699 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -427,6 +427,12 @@ err:
 	return ret;
 }
 
+struct i2c_adapter *intel_gmbus_get_bus(struct drm_i915_private *dev_priv,
+					int pin)
+{
+	return (pin < GMBUS_NUM_PORTS) ? &dev_priv->gmbus[pin].adapter : NULL;
+}
+
 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 aa84832..735e024 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -948,7 +948,7 @@ 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_bus(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 be2c6fe..095c1ae 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -35,9 +35,9 @@
  * intel_ddc_probe
  *
  */
-bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus)
+bool intel_ddc_probe(struct intel_encoder *intel_encoder,
+		     struct i2c_adapter *ddc_bus)
 {
-	struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private;
 	u8 out_buf[] = { 0x0, 0x0};
 	u8 buf[2];
 	struct i2c_msg msgs[] = {
@@ -55,7 +55,7 @@ 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(ddc_bus, msgs, 2) == 2;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index e334ec3..41f2a66 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1251,9 +1251,7 @@ static struct edid *
 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);
+	return drm_get_edid(connector, dev_priv->crt_ddc);
 }
 
 enum drm_connector_status
@@ -1921,12 +1919,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;
+	sdvo->i2c = intel_gmbus_get_bus(dev_priv, pin);
+	if (sdvo->i2c) {
 		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_bus(dev_priv, GMBUS_PORT_DPB);
 	}
 }
 
-- 
1.7.7.3

Re: [PATCH 3/9] drm/i915/intel_i2c: refactor using intel_gmbus_get_bus

[Chris Wilson]

On Wed,  7 Mar 2012 19:50:44 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> +struct i2c_adapter *intel_gmbus_get_bus(struct drm_i915_private *dev_priv,
> +					int pin)
> +{
BUG_ON(pin >= GMBUS_NUM_PORTS);

> +	return (pin < GMBUS_NUM_PORTS) ? &dev_priv->gmbus[pin].adapter : NULL;
You just turned a programming error into a nearly undecipherable OOPS.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/9] drm/i915/intel_i2c: refactor using intel_gmbus_get_bus

[Chris Wilson]

On Wed,  7 Mar 2012 19:50:44 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Instead of letting other modules directly access the ->gmbus array,
> introduce a new API, intel_gmbus_get_bus(), to lookup an i2c_adapter for
> a given gmbus pin pair identifier.  This API enables later refactoring
> of the gmbus pin pair list.
> 
> Note: It is critical that intel_setup_gmbus() gets called before
> intel_gmbus_get_bus().

If you are going to rename it, perhaps a better choice than
intel_gmbus_get_bus(int gpio_pin) ?

intel_i2c_get_adapter(int gpio_pin) since the rest of the code really
does not care how the bits are transferred but that it interfaces using
i2c.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 4/9] drm/i915/intel_i2c: cleanup gmbus/gpio pin assignments

[Daniel Kurtz]

There is no "disabled" port 0.  So, don't even try to initialize/scan
it, etc.  This saves a bit of time when initializing the driver, since
the we can avoid a 50ms timeout waiting for a device to respond on
a port that doesn't even exist.

Similarly, don't initialize the reserved port, either.

Tested on Sandybridge (gen 6, PCH == CougarPoint) hardware.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/i915_reg.h  |    1 -
 drivers/gpu/drm/i915/intel_i2c.c |   64 ++++++++++++++++++--------------------
 2 files changed, 30 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 56af0df..89cace2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -699,7 +699,6 @@
 #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 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 dd8c699..b2cc7f2 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 20
@@ -150,32 +164,23 @@ static void set_data(void *data, int state_high)
 static struct i2c_adapter *
 intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin)
 {
-	static const int map_pin_to_reg[] = {
-		0,
-		GPIOB,
-		GPIOA,
-		GPIOC,
-		GPIOD,
-		GPIOE,
-		GPIOF,
-		0,
-	};
 	struct intel_gpio *gpio;
 
-	if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin])
+	pin -= 1;  /* NB: -1 to map pin pair to gmbus array index */
+	if (pin >= ARRAY_SIZE(gmbus_ports))
 		return NULL;
 
 	gpio = kzalloc(sizeof(struct intel_gpio), GFP_KERNEL);
 	if (gpio == NULL)
 		return NULL;
 
-	gpio->reg = map_pin_to_reg[pin];
+	gpio->reg = gmbus_ports[pin].reg;
 	if (HAS_PCH_SPLIT(dev_priv->dev))
 		gpio->reg += PCH_GPIOA - GPIOA;
 	gpio->dev_priv = dev_priv;
 
 	snprintf(gpio->adapter.name, sizeof(gpio->adapter.name),
-		 "i915 GPIO%c", "?BACDE?F"[pin]);
+		 "i915 GPIO%c", "BACDEF"[pin]);
 	gpio->adapter.owner = THIS_MODULE;
 	gpio->adapter.algo_data	= &gpio->algo;
 	gpio->adapter.dev.parent = &dev_priv->dev->pdev->dev;
@@ -370,33 +375,22 @@ 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;
 
-	dev_priv->gmbus = kcalloc(sizeof(struct intel_gmbus), GMBUS_NUM_PORTS,
-				  GFP_KERNEL);
+	dev_priv->gmbus = kcalloc(sizeof(struct intel_gmbus),
+				  ARRAY_SIZE(gmbus_ports), GFP_KERNEL);
 	if (dev_priv->gmbus == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
+	for (i = 0; i < ARRAY_SIZE(gmbus_ports); i++) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
+		int 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]);
+		snprintf(bus->adapter.name, sizeof(bus->adapter.name),
+			 "i915 gmbus %s", gmbus_ports[i].name);
 
 		bus->adapter.dev.parent = &dev->pdev->dev;
 		bus->adapter.algo_data	= dev_priv;
@@ -407,10 +401,10 @@ 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;
 
 		/* XXX force bit banging until GMBUS is fully debugged */
-		bus->force_bit = intel_gpio_create(dev_priv, i);
+		bus->force_bit = intel_gpio_create(dev_priv, port);
 	}
 
 	intel_i2c_reset(dev_priv->dev);
@@ -430,7 +424,9 @@ err:
 struct i2c_adapter *intel_gmbus_get_bus(struct drm_i915_private *dev_priv,
 					int pin)
 {
-	return (pin < GMBUS_NUM_PORTS) ? &dev_priv->gmbus[pin].adapter : NULL;
+	pin -= 1;  /* NB: -1 to map pin pair to gmbus array index */
+	return (pin < ARRAY_SIZE(gmbus_ports)) ?
+			&dev_priv->gmbus[pin].adapter : NULL;
 }
 
 void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
@@ -467,7 +463,7 @@ void intel_teardown_gmbus(struct drm_device *dev)
 	if (dev_priv->gmbus == NULL)
 		return;
 
-	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
+	for (i = 0; i < ARRAY_SIZE(gmbus_ports); i++) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
 		if (bus->force_bit) {
 			i2c_del_adapter(bus->force_bit);
-- 
1.7.7.3

Re: [PATCH 4/9] drm/i915/intel_i2c: cleanup gmbus/gpio pin assignments

[Chris Wilson]

On Wed,  7 Mar 2012 19:50:45 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> There is no "disabled" port 0.  So, don't even try to initialize/scan
> it, etc.  This saves a bit of time when initializing the driver, since
> the we can avoid a 50ms timeout waiting for a device to respond on
> a port that doesn't even exist.
> 
> Similarly, don't initialize the reserved port, either.

> @@ -150,32 +164,23 @@ static void set_data(void *data, int state_high)
>  static struct i2c_adapter *
>  intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin)
>  {
> -	static const int map_pin_to_reg[] = {
> -		0,
> -		GPIOB,
> -		GPIOA,
> -		GPIOC,
> -		GPIOD,
> -		GPIOE,
> -		GPIOF,
> -		0,
> -	};
>  	struct intel_gpio *gpio;
>  
> -	if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin])

And that doesn't do what your changelog proposes? Why?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/9] drm/i915/intel_i2c: cleanup gmbus/gpio pin assignments

[Daniel Kurtz]

On Wed, Mar 7, 2012 at 8:17 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed,  7 Mar 2012 19:50:45 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> There is no "disabled" port 0.  So, don't even try to initialize/scan
>> it, etc.  This saves a bit of time when initializing the driver, since
>> the we can avoid a 50ms timeout waiting for a device to respond on
>> a port that doesn't even exist.
>>
>> Similarly, don't initialize the reserved port, either.
>
>> @@ -150,32 +164,23 @@ static void set_data(void *data, int state_high)
>>  static struct i2c_adapter *
>>  intel_gpio_create(struct drm_i915_private *dev_priv, u32 pin)
>>  {
>> -     static const int map_pin_to_reg[] = {
>> -             0,
>> -             GPIOB,
>> -             GPIOA,
>> -             GPIOC,
>> -             GPIOD,
>> -             GPIOE,
>> -             GPIOF,
>> -             0,
>> -     };
>>       struct intel_gpio *gpio;
>>
>> -     if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin])
>
> And that doesn't do what your changelog proposes? Why?

This changelog proposes to optimize initialization of the gmbus ports,
not the gpio-bit-bang ports.

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre

[PATCH 5/9] drm/i915/intel_i2c: add locking around i2c algorithm accesses

[Daniel Kurtz]

The i915 has multiple i2c adapters.  However, they all share a single
single set of i2c control registers (algorithm).  Thus, different threads
trying to access different adapters could interfere with each other.

Note: different threads trying to access the same channel is already
handled in the i2c-core using the i2c adapter lock.

This patch adds a mutex to serialize access to the gmbus_xfer routine.
Note: the same mutex serializes both bit banged and native xfers.

Signed-off-by: Yufeng Shen <miletus@chromium.org>
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/i915_drv.h  |    1 +
 drivers/gpu/drm/i915/intel_i2c.c |   12 ++++++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 64d1276..c7f0809 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -301,6 +301,7 @@ typedef struct drm_i915_private {
 		struct i2c_adapter *force_bit;
 		u32 reg0;
 	} *gmbus;
+	struct mutex gmbus_mutex;
 
 	struct pci_dev *bridge_dev;
 	struct intel_ring_buffer ring[I915_NUM_RINGS];
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b2cc7f2..b97392c 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -213,6 +213,8 @@ intel_i2c_quirk_xfer(struct drm_i915_private *dev_priv,
 					       adapter);
 	int ret;
 
+	mutex_lock(&dev_priv->gmbus_mutex);
+
 	intel_i2c_reset(dev_priv->dev);
 
 	intel_i2c_quirk_set(dev_priv, true);
@@ -226,6 +228,8 @@ intel_i2c_quirk_xfer(struct drm_i915_private *dev_priv,
 	set_clock(gpio, 1);
 	intel_i2c_quirk_set(dev_priv, false);
 
+	mutex_unlock(&dev_priv->gmbus_mutex);
+
 	return ret;
 }
 
@@ -244,6 +248,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs,
 					    num);
 
+	mutex_lock(&dev_priv->gmbus_mutex);
 
 	reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
 
@@ -334,6 +339,9 @@ done:
 	 * start of the next xfer, till then let it sleep.
 	 */
 	I915_WRITE(GMBUS0 + reg_offset, 0);
+
+	mutex_unlock(&dev_priv->gmbus_mutex);
+
 	return i;
 
 timeout:
@@ -341,6 +349,8 @@ timeout:
 		 bus->reg0 & 0xff, bus->adapter.name);
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 
+	mutex_unlock(&dev_priv->gmbus_mutex);
+
 	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
 	bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff);
 	if (!bus->force_bit)
@@ -383,6 +393,8 @@ int intel_setup_gmbus(struct drm_device *dev)
 	if (dev_priv->gmbus == NULL)
 		return -ENOMEM;
 
+	mutex_init(&dev_priv->gmbus_mutex);
+
 	for (i = 0; i < ARRAY_SIZE(gmbus_ports); i++) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
 		int port = i + 1; /* +1 to map gmbus index to pin pair */
-- 
1.7.7.3

Re: [PATCH 5/9] drm/i915/intel_i2c: add locking around i2c algorithm accesses

[Daniel Vetter]

On Wed, Mar 07, 2012 at 07:50:46PM +0800, Daniel Kurtz wrote:
> The i915 has multiple i2c adapters.  However, they all share a single
> single set of i2c control registers (algorithm).  Thus, different threads
> trying to access different adapters could interfere with each other.
> 
> Note: different threads trying to access the same channel is already
> handled in the i2c-core using the i2c adapter lock.
> 
> This patch adds a mutex to serialize access to the gmbus_xfer routine.
> Note: the same mutex serializes both bit banged and native xfers.
> 
> Signed-off-by: Yufeng Shen <miletus@chromium.org>
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

This is already fixed in

commit 8a8ed1f5143b3df312e436ab15290e4a7ca6a559
Author: Yufeng Shen <miletus@chromium.org>
Date:   Mon Feb 13 17:36:54 2012 -0500

    drm/i915: Fix race condition in accessing GMBUS

There are also a few gmbus changes already queued up in my -next tree at 

http://cgit.freedesktop.org/~danvet/drm-intel/log/?h=drm-intel-next-queued

Please rebase your patch series on top of that (with Chris' suggestion
incorporated).

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 6/9] drm/i915/intel_i2c: return -ENXIO for device NAK

[Daniel Kurtz]

Return -ENXIO if a device NAKs a transaction.

Note: We should return -ETIMEDOUT, too if the transaction times out,
however, that error path is currently handled by the 'bit-bang fallback'.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b97392c..2cdd531 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -243,6 +243,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 					       adapter);
 	struct drm_i915_private *dev_priv = adapter->algo_data;
 	int i, reg_offset;
+	int ret = 0;
 
 	if (bus->force_bit)
 		return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs,
@@ -333,6 +334,7 @@ clear_err:
 	 */
 	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
 	I915_WRITE(GMBUS1 + reg_offset, 0);
+	ret = -ENXIO;
 
 done:
 	/* Mark the GMBUS interface as disabled. We will re-enable it at the
@@ -342,7 +344,7 @@ done:
 
 	mutex_unlock(&dev_priv->gmbus_mutex);
 
-	return i;
+	return ret ?: i;
 
 timeout:
 	DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
-- 
1.7.7.3

Re: [PATCH 6/9] drm/i915/intel_i2c: return -ENXIO for device NAK

[Chris Wilson]

On Wed,  7 Mar 2012 19:50:47 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Return -ENXIO if a device NAKs a transaction.
> 
> Note: We should return -ETIMEDOUT, too if the transaction times out,
> however, that error path is currently handled by the 'bit-bang fallback'.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Can you clarify what the rule is if an error is detected part-way
through a xfer?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 6/9] drm/i915/intel_i2c: return -ENXIO for device NAK

[Daniel Kurtz]

On Wed, Mar 7, 2012 at 8:12 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed,  7 Mar 2012 19:50:47 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> Return -ENXIO if a device NAKs a transaction.
>>
>> Note: We should return -ETIMEDOUT, too if the transaction times out,
>> however, that error path is currently handled by the 'bit-bang fallback'.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>
> Can you clarify what the rule is if an error is detected part-way
> through a xfer?

A priceless comment from drivers/i2c/i2c-core::i2c_transfer...

/* REVISIT the fault reporting model here is weak:
 *
 *  - When we get an error after receiving N bytes from a slave,
 *    there is no way to report "N".
 *
 *  - When we get a NAK after transmitting N bytes to a slave,
 *    there is no way to report "N" ... or to let the master
 *    continue executing the rest of this combined message, if
 *    that's the appropriate response.
 *
 *  - When for example "num" is two and we successfully complete
 *    the first message but get an error part way through the
 *    second, it's unclear whether that should be reported as
 *    one (discarding status on the second message) or errno
 *    (discarding status on the first one).
 */

As for which error code to use, all I have found is this:

Documentation/i2c/fault-codes:
ENXIO
	Returned by I2C adapters to indicate that the address phase
	of a transfer didn't get an ACK.  While it might just mean
	an I2C device was temporarily not responding, usually it
	means there's nothing listening at that address.

This doesn't specify what to do if the transfer doesn't get an ACK
during another phase of the transfer.
However, it does say to send -ENXIO "if no ACK during address phase",
which is a subset of the possible no-ACK conditions during a transfer.
 Thus, I choose to return ENXIO in all no-ACK cases, to ensure we send
it during the one case that is specified.

For different i2c adapters i've seen wildly different behavior:
   -ENXIO (i2c-algo-pca)
   -EIO (i2c-algo-bit)
   -EREMOTEIO (i2c-algo-pcf).

-Dan

Re: [PATCH 6/9] drm/i915/intel_i2c: return -ENXIO for device NAK

[Chris Wilson]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Wed, 7 Mar 2012 20:41:03 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Wed, Mar 7, 2012 at 8:12 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed,  7 Mar 2012 19:50:47 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> >> Return -ENXIO if a device NAKs a transaction.
> >>
> >> Note: We should return -ETIMEDOUT, too if the transaction times out,
> >> however, that error path is currently handled by the 'bit-bang fallback'.
> >>
> >> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> >
> > Can you clarify what the rule is if an error is detected part-way
> > through a xfer?
> 
> A priceless comment from drivers/i2c/i2c-core::i2c_transfer...

Thanks, that is about as consistent as one expects with i2c. ;)
 
> This doesn't specify what to do if the transfer doesn't get an ACK
> during another phase of the transfer.
> However, it does say to send -ENXIO "if no ACK during address phase",
> which is a subset of the possible no-ACK conditions during a transfer.
>  Thus, I choose to return ENXIO in all no-ACK cases, to ensure we send
> it during the one case that is specified.

This (and a summary of the rest) deserves to be captured as a comment in
the code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 7/9] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

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 |   42 +++++++++++++++++++++++++------------
 1 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 2cdd531..c3d8c70 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -261,7 +261,7 @@ 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) |
+				   GMBUS_CYCLE_WAIT |
 				   (len << GMBUS_BYTE_COUNT_SHIFT) |
 				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
 				   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
@@ -273,7 +273,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 					     (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);
@@ -292,20 +293,13 @@ gmbus_xfer(struct i2c_adapter *adapter,
 
 			I915_WRITE(GMBUS3 + reg_offset, val);
 			I915_WRITE(GMBUS1 + reg_offset,
-				   (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) |
+				   GMBUS_CYCLE_WAIT |
 				   (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);
@@ -313,11 +307,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;
@@ -337,9 +338,22 @@ clear_err:
 	ret = -ENXIO;
 
 done:
-	/* Mark the GMBUS interface as disabled. We will re-enable it at the
-	 * start of the next xfer, till then let it sleep.
+	if (I915_READ(GMBUS2 + reg_offset) & GMBUS_HW_WAIT_PHASE) {
+		I915_WRITE(GMBUS1 + reg_offset,
+			   GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
+		POSTING_READ(GMBUS2 + reg_offset);
+	}
+
+	/* 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);
+		ret = -ETIMEDOUT;
+	}
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 
 	mutex_unlock(&dev_priv->gmbus_mutex);
-- 
1.7.7.3

[PATCH 8/9] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Daniel Kurtz]

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 c3d8c70..41f9ae2 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -256,12 +256,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);
-- 
1.7.7.3

[PATCH 9/9] drm/i915/intel_i2c: reuse GMBUS2 value from polling loop

[Daniel Kurtz]

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.

Note: checkpatch doesn't like this ('assigning in if condition'), but it seems
like the cleanest implementation.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 41f9ae2..450f2b8 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -244,6 +244,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	struct drm_i915_private *dev_priv = adapter->algo_data;
 	int i, reg_offset;
 	int ret = 0;
+	u32 gmbus2 = 0;
 
 	if (bus->force_bit)
 		return intel_i2c_quirk_xfer(dev_priv, bus->force_bit, msgs,
@@ -297,12 +298,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);
@@ -336,21 +337,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;
 	}
 
@@ -366,7 +367,7 @@ clear_err:
 	ret = -ENXIO;
 
 done:
-	if (I915_READ(GMBUS2 + reg_offset) & GMBUS_HW_WAIT_PHASE) {
+	if (gmbus2 & GMBUS_HW_WAIT_PHASE) {
 		I915_WRITE(GMBUS1 + reg_offset,
 			   GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
 		POSTING_READ(GMBUS2 + reg_offset);
-- 
1.7.7.3

Re: [PATCH 9/9] drm/i915/intel_i2c: reuse GMBUS2 value from polling loop

[Chris Wilson]

On Wed,  7 Mar 2012 19:50:50 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> 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.
> 
> Note: checkpatch doesn't like this ('assigning in if condition'), but it seems
> like the cleanest implementation.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre