[PATCH v3 0/4] drm/dp: Introduce AUX channel infrastructure

[PATCH v3 0/4] drm/dp: Introduce AUX channel infrastructure

[Thierry Reding]

Hi,

This small series introduces some infrastructure to support AUX channels
in a generic way. Drivers make use of it by embedding and filling in a
struct drm_dp_aux. Various helpers can then be used to for example read
from or write to the DPCD.

Patch 1 adds the basic infrastructure as well as a couple of helpers to
access the DPCD.

The helper introduced in patch 2 can be used to obtain the link status
as expected by various existing DP helpers.

More convenience helpers are added in patch 3, which can come in handy
during DP initialization.

An AUX channel can also be used to implement I2C-over-AUX and patch 4
implements an I2C adapter that can be used with the DRM EDID helpers.

Changes in v3:
- address comments by Jani Nikula:
  - keep debug and error messages in AUX helpers
  - read/write back-to-back registers in chunks
  - separate link power up and configuration
  - do not power up for DPCD prior to 1.1
  - sleep after power up as per the spec
  - return number of bytes transferred
  - factor out some common code
  - reorder function arguments
  - fix typo in comment
- address comments by Daniel Vetter:
  - embed i2c_adapter within struct drm_dp_aux
  - describe error codes

Changes in v2:
- reimplement I2C-over-AUX functionality to get rid of the additional
  layer
- extract retry logic from existing drivers
- add more kerneldoc comments

Thierry

Thierry Reding (4):
  drm/dp: Add AUX channel infrastructure
  drm/dp: Add drm_dp_dpcd_read_link_status()
  drm/dp: Add DisplayPort link helpers
  drm/dp: Allow registering AUX channels as I2C busses

 drivers/gpu/drm/drm_dp_helper.c | 403 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |  92 +++++++++
 2 files changed, 495 insertions(+)

-- 
1.8.4.2

[PATCH v3 1/4] drm/dp: Add AUX channel infrastructure

[Thierry Reding]

This is a superset of the current i2c_dp_aux bus functionality and can
be used to transfer native AUX in addition to I2C-over-AUX messages.

Helpers are provided to read and write the DPCD, either blockwise or
byte-wise. Many of the existing helpers for DisplayPort take a copy of a
portion of the DPCD and operate on that, without a way to write data
back to the DPCD (e.g. for configuration of the link).

Subsequent patches will build upon this infrastructure to provide common
functionality in a generic way.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- reorder drm_dp_dpcd_writeb() arguments to be more intuitive
- return number of bytes transferred in drm_dp_dpcd_write()
- factor out drm_dp_dpcd_access()
- describe error codes

 drivers/gpu/drm/drm_dp_helper.c | 110 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |  67 ++++++++++++++++++++++++
 2 files changed, 177 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9e978aae8972..3641c5eb349b 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -346,3 +346,113 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
 	}
 }
 EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
+
+/**
+ * DOC: dp helpers
+ *
+ * The DisplayPort AUX channel is an abstraction to allow generic, driver-
+ * independent access to AUX functionality. Drivers can take advantage of
+ * this by filling in the fields of the drm_dp_aux sturcture.
+ *
+ * The .transfer() function is the hardware-specific implementation of how
+ * a transaction is executed on the AUX channel. A pointer to a struct
+ * drm_dp_aux_msg describing the transaction is passed into this function.
+ * Upon success, the implementation should return the number of bytes that
+ * were transferred, or a negative error-code on failure. Helpers propagate
+ * errors from the .transfer() function, with the exception of the -EBUSY
+ * error, which causes a transaction to be retried.
+ *
+ * The .dev field should be set to a pointer to the device that implements
+ * the AUX channel.
+ */
+
+static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
+			      unsigned int offset, void *buffer, size_t size)
+{
+	struct drm_dp_aux_msg msg;
+	unsigned int retry;
+	int err;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.address = offset;
+	msg.request = request;
+	msg.buffer = buffer;
+	msg.size = size;
+
+	/*
+	 * The specification doesn't give any recommendation on how often to
+	 * retry native transactions, so retry 7 times like for I2C-over-AUX
+	 * transactions.
+	 */
+	for (retry = 0; retry < 7; retry++) {
+		err = aux->transfer(aux, &msg);
+		if (err < 0) {
+			if (err == -EBUSY)
+				continue;
+
+			return err;
+		}
+
+		if (err == 0)
+			return -EPROTO;
+
+		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
+		case DP_AUX_NATIVE_REPLY_ACK:
+			return err;
+
+		case DP_AUX_NATIVE_REPLY_NACK:
+			return -EIO;
+
+		case DP_AUX_NATIVE_REPLY_DEFER:
+			usleep_range(400, 500);
+			break;
+		}
+	}
+
+	DRM_ERROR("too many retries, giving up\n");
+	return -EIO;
+}
+
+/**
+ * drm_dp_dpcd_read() - read a series of bytes from the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the (first) register to read
+ * @buffer: buffer to store the register values
+ * @size: number of bytes in @buffer
+ *
+ * Returns the number of bytes transferred on success, or a negative error
+ * code on failure. -EIO is returned if the request was NAKed by the sink or
+ * if the retry count was exceeded. If no bytes were transferred, -EPROTO is
+ * returned. Errors from the underlying AUX channel transfer function, with
+ * the exception of -EBUSY (upon which the transaction will be retried), are
+ * propagated to the caller.
+ */
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
+			 void *buffer, size_t size)
+{
+	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
+				  size);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_read);
+
+/**
+ * drm_dp_dpcd_write() - write a series of bytes to the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the (first) register to write
+ * @buffer: buffer containing the values to write
+ * @size: number of bytes in @buffer
+ *
+ * Returns the number of bytes transferred on success, or a negative error
+ * code on failure. -EIO is returned if the request was NAKed by the sink or
+ * if the retry count was exceeded. If no bytes were transferred, -EPROTO is
+ * returned. Errors from the underlying AUX channel transfer function, with
+ * the exception of -EBUSY (upon which the transaction will be retried), are
+ * propagated to the caller.
+ */
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
+			  void *buffer, size_t size)
+{
+	return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer,
+				  size);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_write);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1d09050a8c00..c69c1dc1b741 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -398,4 +398,71 @@ drm_dp_enhanced_frame_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 		(dpcd[DP_MAX_LANE_COUNT] & DP_ENHANCED_FRAME_CAP);
 }
 
+/*
+ * DisplayPort AUX channel
+ */
+
+/**
+ * struct drm_dp_aux_msg - DisplayPort AUX channel transaction
+ * @address: address of the (first) register to access
+ * @request: contains the type of transaction (see DP_AUX_* macros)
+ * @reply: upon completion, contains the reply type of the transaction
+ * @buffer: pointer to a transmission or reception buffer
+ * @size: size of @buffer
+ */
+struct drm_dp_aux_msg {
+	unsigned int address;
+	u8 request;
+	u8 reply;
+	void *buffer;
+	size_t size;
+};
+
+/**
+ * struct drm_dp_aux - DisplayPort AUX channel
+ * @dev: pointer to struct device that is the parent for this AUX channel
+ * @transfer: transfers a message representing a single AUX transaction
+ */
+struct drm_dp_aux {
+	struct device *dev;
+
+	ssize_t (*transfer)(struct drm_dp_aux *aux,
+			    struct drm_dp_aux_msg *msg);
+};
+
+ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
+			 void *buffer, size_t size);
+ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
+			  void *buffer, size_t size);
+
+/**
+ * drm_dp_dpcd_readb() - read a single byte from the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the register to read
+ * @valuep: location where the value of the register will be stored
+ *
+ * Returns the number of bytes transferred (1) on success, or a negative
+ * error code on failure.
+ */
+static inline ssize_t drm_dp_dpcd_readb(struct drm_dp_aux *aux,
+					unsigned int offset, u8 *valuep)
+{
+	return drm_dp_dpcd_read(aux, offset, valuep, 1);
+}
+
+/**
+ * drm_dp_dpcd_writeb() - write a single byte to the DPCD
+ * @aux: DisplayPort AUX channel
+ * @offset: address of the register to write
+ * @value: value to write to the register
+ *
+ * Returns the number of bytes transferred (1) on success, or a negative
+ * error code on failure.
+ */
+static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux,
+					 unsigned int offset, u8 value)
+{
+	return drm_dp_dpcd_write(aux, offset, &value, 1);
+}
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
1.8.4.2

[PATCH v3 2/4] drm/dp: Add drm_dp_dpcd_read_link_status()

[Thierry Reding]

The function reads the link status (6 bytes starting at offset 0x202)
from the DPCD so that it can be conveniently passed to other DPCD
helpers.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 16 ++++++++++++++++
 include/drm/drm_dp_helper.h     |  3 +++
 2 files changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3641c5eb349b..572637456713 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -456,3 +456,19 @@ ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset,
 				  size);
 }
 EXPORT_SYMBOL(drm_dp_dpcd_write);
+
+/**
+ * drm_dp_dpcd_read_link_status() - read DPCD link status (bytes 0x202-0x207)
+ * @aux: DisplayPort AUX channel
+ * @status: buffer to store the link status in (must be at least 6 bytes)
+ *
+ * Returns the number of bytes transferred on success or a negative error
+ * code on failure.
+ */
+int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
+				 u8 status[DP_LINK_STATUS_SIZE])
+{
+	return drm_dp_dpcd_read(aux, DP_LANE0_1_STATUS, status,
+				DP_LINK_STATUS_SIZE);
+}
+EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c69c1dc1b741..8af695277a84 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -465,4 +465,7 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux,
 	return drm_dp_dpcd_write(aux, offset, &value, 1);
 }
 
+int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
+				 u8 status[DP_LINK_STATUS_SIZE]);
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
1.8.4.2

[PATCH v3 3/4] drm/dp: Add DisplayPort link helpers

[Thierry Reding]

Add a helper to probe a DP link (read out the supported DPCD revision,
maximum rate, link count and capabilities) as well as power up the DP
link and configure it accordingly.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- split into drm_dp_link_power_up() and drm_dp_link_configure()
- do not change sink state for DPCD versions earlier than 1.1
- sleep for 1-2 ms after setting local sink to D0 state
- read and write consecutive registers where possible
- read DPCD revision when link is probed
- remove duplicate kerneldoc

 drivers/gpu/drm/drm_dp_helper.c | 94 +++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     | 17 ++++++++
 2 files changed, 111 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 572637456713..ac2e12789634 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -472,3 +472,97 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 				DP_LINK_STATUS_SIZE);
 }
 EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
+
+/**
+ * drm_dp_link_probe() - probe a DisplayPort link for capabilities
+ * @aux: DisplayPort AUX channel
+ * @link: pointer to structure in which to return link capabilities
+ *
+ * The structure filled in by this function can usually be passed directly
+ * into drm_dp_link_power_up() and drm_dp_link_configure() to power up and
+ * configure the link based on the link's capabilities.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
+{
+	u8 values[3];
+	int err;
+
+	memset(link, 0, sizeof(*link));
+
+	err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
+	if (err < 0)
+		return err;
+
+	link->revision = values[0];
+	link->rate = drm_dp_bw_code_to_link_rate(values[1]);
+	link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
+
+	if (values[2] & DP_ENHANCED_FRAME_CAP)
+		link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
+
+	return 0;
+}
+
+/**
+ * drm_dp_link_power_up() - power up a DisplayPort link
+ * @aux: DisplayPort AUX channel
+ * @link: pointer to a structure containing the link configuration
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
+{
+	u8 value;
+	int err;
+
+	/* DP_SET_POWER register is only available on DPCD v1.1 and later */
+	if (link->revision < 0x11)
+		return 0;
+
+	err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
+	if (err < 0)
+		return err;
+
+	value &= ~DP_SET_POWER_MASK;
+	value |= DP_SET_POWER_D0;
+
+	err = drm_dp_dpcd_writeb(aux, DP_SET_POWER, value);
+	if (err < 0)
+		return err;
+
+	/*
+	 * According to the DP 1.1 specification, a "Sink Device must exit the
+	 * power saving state within 1 ms" (Section 2.5.3.1, Table 5-52, "Sink
+	 * Control Field" (register 0x600).
+	 */
+	usleep_range(1000, 2000);
+
+	return 0;
+}
+
+/**
+ * drm_dp_link_configure() - configure a DisplayPort link
+ * @aux: DisplayPort AUX cahnnel
+ * @link: pointer to a structure containing the link configuration
+ *
+ * Returns 0 on seccuss or a negative error code on failure.
+ */
+int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
+{
+	u8 values[2];
+	int err;
+
+	values[0] = drm_dp_link_rate_to_bw_code(link->rate);
+	values[1] = link->num_lanes;
+
+	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
+		values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
+
+	err = drm_dp_dpcd_write(aux, DP_LINK_BW_SET, values, sizeof(values));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8af695277a84..c7b3c736c40a 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -291,6 +291,7 @@
 #define DP_SET_POWER                        0x600
 # define DP_SET_POWER_D0                    0x1
 # define DP_SET_POWER_D3                    0x2
+# define DP_SET_POWER_MASK                  0x3
 
 #define DP_PSR_ERROR_STATUS                 0x2006  /* XXX 1.2? */
 # define DP_PSR_LINK_CRC_ERROR              (1 << 0)
@@ -468,4 +469,20 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux,
 int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
 				 u8 status[DP_LINK_STATUS_SIZE]);
 
+/*
+ * DisplayPort link
+ */
+#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
+
+struct drm_dp_link {
+	unsigned char revision;
+	unsigned int rate;
+	unsigned int num_lanes;
+	unsigned long capabilities;
+};
+
+int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
+int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
+int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
1.8.4.2

[PATCH v3 4/4] drm/dp: Allow registering AUX channels as I2C busses

[Thierry Reding]

Implements an I2C-over-AUX I2C adapter on top of the generic drm_dp_aux
infrastructure. It extracts the retry logic from existing drivers, which
should help in porting those drivers to this new helper.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- add back DRM_DEBUG_KMS and DRM_ERROR messages
- embed i2c_adapter within struct drm_dp_aux
- fix typo in comment

 drivers/gpu/drm/drm_dp_helper.c | 183 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_dp_helper.h     |   5 ++
 2 files changed, 188 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index ac2e12789634..dc25f63bbb0b 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -364,6 +364,13 @@ EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
  *
  * The .dev field should be set to a pointer to the device that implements
  * the AUX channel.
+ *
+ * An AUX channel can also be used to transport I2C messages to a sink. A
+ * typical application of that is to access an EDID that's present in the
+ * sink device. The .transfer() function can also be used to execute such
+ * transactions. The drm_dp_aux_register_i2c_bus() function registers an
+ * I2C adapter that can be passed to drm_probe_ddc(). Upon removal, drivers
+ * should call drm_dp_aux_unregister_i2c_bus() to remove the I2C adapter.
  */
 
 static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
@@ -566,3 +573,179 @@ int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
 
 	return 0;
 }
+
+/*
+ * I2C-over-AUX implementation
+ */
+
+static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
+	       I2C_FUNC_SMBUS_READ_BLOCK_DATA |
+	       I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
+	       I2C_FUNC_10BIT_ADDR;
+}
+
+/*
+ * Transfer a single I2C-over-AUX message and handle various error conditions,
+ * retrying the transaction as appropriate.
+ */
+static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
+{
+	unsigned int retry;
+	int err;
+
+	/*
+	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device
+	 * is required to retry at least seven times upon receiving AUX_DEFER
+	 * before giving up the AUX transaction.
+	 */
+	for (retry = 0; retry < 7; retry++) {
+		err = aux->transfer(aux, msg);
+		if (err < 0) {
+			if (err == -EBUSY)
+				continue;
+
+			DRM_DEBUG_KMS("transaction failed: %d\n", err);
+			return err;
+		}
+
+		switch (msg->reply & DP_AUX_NATIVE_REPLY_MASK) {
+		case DP_AUX_NATIVE_REPLY_ACK:
+			/*
+			 * For I2C-over-AUX transactions this isn't enough, we
+			 * need to check for the I2C ACK reply.
+			 */
+			break;
+
+		case DP_AUX_NATIVE_REPLY_NACK:
+			DRM_DEBUG_KMS("native nack\n");
+			return -EREMOTEIO;
+
+		case DP_AUX_NATIVE_REPLY_DEFER:
+			DRM_DEBUG_KMS("native defer");
+			/*
+			 * We could check for I2C bitrate capabilities and if
+			 * available adjust this interval. We could also be
+			 * more careful with DP-to-legacy adapters where a
+			 * long legacy cable may force very low I2C bit rates.
+			 *
+			 * For now just defer for long enough to hopefully be
+			 * safe for all use-cases.
+			 */
+			usleep_range(500, 600);
+			continue;
+
+		default:
+			DRM_ERROR("invalid native reply %#04x\n", msg->reply);
+			return -EREMOTEIO;
+		}
+
+		switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {
+		case DP_AUX_I2C_REPLY_ACK:
+			/*
+			 * Both native ACK and I2C ACK replies received. We
+			 * can assume the transfer was successful.
+			 */
+			return 0;
+
+		case DP_AUX_I2C_REPLY_NACK:
+			DRM_DEBUG_KMS("I2C nack\n");
+			return -EREMOTEIO;
+
+		case DP_AUX_I2C_REPLY_DEFER:
+			DRM_DEBUG_KMS("I2C defer\n");
+			usleep_range(400, 500);
+			continue;
+
+		default:
+			DRM_ERROR("invalid I2C reply %#04x\n", msg->reply);
+			return -EREMOTEIO;
+		}
+	}
+
+	DRM_ERROR("too many retries, giving up\n");
+	return -EREMOTEIO;
+}
+
+static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
+			   int num)
+{
+	struct drm_dp_aux *aux = adapter->algo_data;
+	unsigned int i, j;
+
+	for (i = 0; i < num; i++) {
+		struct drm_dp_aux_msg msg;
+		int err;
+
+		/*
+		 * Many hardware implementations support FIFOs larger than a
+		 * single byte, but it has been empirically determined that
+		 * transferring data in larger chunks can actually lead to
+		 * decreased performance. Therefore each message is simply
+		 * transferred byte-by-byte.
+		 */
+		for (j = 0; j < msgs[i].len; j++) {
+			memset(&msg, 0, sizeof(msg));
+			msg.address = msgs[i].addr;
+
+			msg.request = (msgs[i].flags & I2C_M_RD) ?
+					DP_AUX_I2C_READ :
+					DP_AUX_I2C_WRITE;
+
+			/*
+			 * All messages except the last one are middle-of-
+			 * transfer messages.
+			 */
+			if (j < msgs[i].len - 1)
+				msg.request |= DP_AUX_I2C_MOT;
+
+			msg.buffer = msgs[i].buf + j;
+			msg.size = 1;
+
+			err = drm_dp_i2c_do_msg(aux, &msg);
+			if (err < 0)
+				return err;
+		}
+	}
+
+	return num;
+}
+
+static const struct i2c_algorithm drm_dp_i2c_algo = {
+	.functionality = drm_dp_i2c_functionality,
+	.master_xfer = drm_dp_i2c_xfer,
+};
+
+/**
+ * drm_dp_aux_register_i2c_bus() - register an I2C adapter for I2C-over-AUX
+ * @aux: DisplayPort AUX channel
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux)
+{
+	aux->ddc.algo = &drm_dp_i2c_algo;
+	aux->ddc.algo_data = aux;
+	aux->ddc.retries = 3;
+
+	aux->ddc.class = I2C_CLASS_DDC;
+	aux->ddc.owner = THIS_MODULE;
+	aux->ddc.dev.parent = aux->dev;
+	aux->ddc.dev.of_node = aux->dev->of_node;
+
+	strncpy(aux->ddc.name, dev_name(aux->dev), sizeof(aux->ddc.name));
+
+	return i2c_add_adapter(&aux->ddc);
+}
+EXPORT_SYMBOL(drm_dp_aux_register_i2c_bus);
+
+/**
+ * drm_dp_aux_unregister_i2c_bus() - unregister an I2C-over-AUX adapter
+ * @aux: DisplayPort AUX channel
+ */
+void drm_dp_aux_unregister_i2c_bus(struct drm_dp_aux *aux)
+{
+	i2c_del_adapter(&aux->ddc);
+}
+EXPORT_SYMBOL(drm_dp_aux_unregister_i2c_bus);
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c7b3c736c40a..4d67bd12089b 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -421,10 +421,12 @@ struct drm_dp_aux_msg {
 
 /**
  * struct drm_dp_aux - DisplayPort AUX channel
+ * @ddc: I2C adapter that can be used for I2C-over-AUX communication
  * @dev: pointer to struct device that is the parent for this AUX channel
  * @transfer: transfers a message representing a single AUX transaction
  */
 struct drm_dp_aux {
+	struct i2c_adapter ddc;
 	struct device *dev;
 
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
@@ -485,4 +487,7 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
 int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
 
+int drm_dp_aux_register_i2c_bus(struct drm_dp_aux *aux);
+void drm_dp_aux_unregister_i2c_bus(struct drm_dp_aux *aux);
+
 #endif /* _DRM_DP_HELPER_H_ */
-- 
1.8.4.2

Re: [PATCH v3 3/4] drm/dp: Add DisplayPort link helpers

[Alex Deucher]

On Tue, Jan 14, 2014 at 9:55 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> Add a helper to probe a DP link (read out the supported DPCD revision,
> maximum rate, link count and capabilities) as well as power up the DP
> link and configure it accordingly.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v3:
> - split into drm_dp_link_power_up() and drm_dp_link_configure()
> - do not change sink state for DPCD versions earlier than 1.1
> - sleep for 1-2 ms after setting local sink to D0 state
> - read and write consecutive registers where possible
> - read DPCD revision when link is probed
> - remove duplicate kerneldoc
>
>  drivers/gpu/drm/drm_dp_helper.c | 94 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     | 17 ++++++++
>  2 files changed, 111 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 572637456713..ac2e12789634 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -472,3 +472,97 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>                                 DP_LINK_STATUS_SIZE);
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
> +
> +/**
> + * drm_dp_link_probe() - probe a DisplayPort link for capabilities
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to structure in which to return link capabilities
> + *
> + * The structure filled in by this function can usually be passed directly
> + * into drm_dp_link_power_up() and drm_dp_link_configure() to power up and
> + * configure the link based on the link's capabilities.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +       u8 values[3];
> +       int err;
> +
> +       memset(link, 0, sizeof(*link));
> +
> +       err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
> +       if (err < 0)
> +               return err;
> +
> +       link->revision = values[0];
> +       link->rate = drm_dp_bw_code_to_link_rate(values[1]);
> +       link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
> +
> +       if (values[2] & DP_ENHANCED_FRAME_CAP)
> +               link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
> +
> +       return 0;
> +}
> +
> +/**
> + * drm_dp_link_power_up() - power up a DisplayPort link
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +       u8 value;
> +       int err;
> +
> +       /* DP_SET_POWER register is only available on DPCD v1.1 and later */
> +       if (link->revision < 0x11)
> +               return 0;
> +
> +       err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
> +       if (err < 0)
> +               return err;
> +
> +       value &= ~DP_SET_POWER_MASK;
> +       value |= DP_SET_POWER_D0;
> +
> +       err = drm_dp_dpcd_writeb(aux, DP_SET_POWER, value);
> +       if (err < 0)
> +               return err;
> +
> +       /*
> +        * According to the DP 1.1 specification, a "Sink Device must exit the
> +        * power saving state within 1 ms" (Section 2.5.3.1, Table 5-52, "Sink
> +        * Control Field" (register 0x600).
> +        */
> +       usleep_range(1000, 2000);
> +
> +       return 0;
> +}
> +
> +/**
> + * drm_dp_link_configure() - configure a DisplayPort link
> + * @aux: DisplayPort AUX cahnnel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on seccuss or a negative error code on failure.

spelling.  success.


Alex

Re: [PATCH v3 0/4] drm/dp: Introduce AUX channel infrastructure

[Alex Deucher]

On Tue, Jan 14, 2014 at 9:55 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> Hi,
>
> This small series introduces some infrastructure to support AUX channels
> in a generic way. Drivers make use of it by embedding and filling in a
> struct drm_dp_aux. Various helpers can then be used to for example read
> from or write to the DPCD.
>
> Patch 1 adds the basic infrastructure as well as a couple of helpers to
> access the DPCD.
>
> The helper introduced in patch 2 can be used to obtain the link status
> as expected by various existing DP helpers.
>
> More convenience helpers are added in patch 3, which can come in handy
> during DP initialization.
>
> An AUX channel can also be used to implement I2C-over-AUX and patch 4
> implements an I2C adapter that can be used with the DRM EDID helpers.
>
> Changes in v3:
> - address comments by Jani Nikula:
>   - keep debug and error messages in AUX helpers
>   - read/write back-to-back registers in chunks
>   - separate link power up and configuration
>   - do not power up for DPCD prior to 1.1
>   - sleep after power up as per the spec
>   - return number of bytes transferred
>   - factor out some common code
>   - reorder function arguments
>   - fix typo in comment
> - address comments by Daniel Vetter:
>   - embed i2c_adapter within struct drm_dp_aux
>   - describe error codes
>
> Changes in v2:
> - reimplement I2C-over-AUX functionality to get rid of the additional
>   layer
> - extract retry logic from existing drivers
> - add more kerneldoc comments
>

Just one small spelling fix in patch 3, with that fixed,

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Thanks for doing this!

Alex

> Thierry
>
> Thierry Reding (4):
>   drm/dp: Add AUX channel infrastructure
>   drm/dp: Add drm_dp_dpcd_read_link_status()
>   drm/dp: Add DisplayPort link helpers
>   drm/dp: Allow registering AUX channels as I2C busses
>
>  drivers/gpu/drm/drm_dp_helper.c | 403 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     |  92 +++++++++
>  2 files changed, 495 insertions(+)
>
> --
> 1.8.4.2
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 0/4] drm/dp: Introduce AUX channel infrastructure

[Alex Deucher]

On Tue, Jan 14, 2014 at 10:54 AM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Tue, Jan 14, 2014 at 9:55 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>> Hi,
>>
>> This small series introduces some infrastructure to support AUX channels
>> in a generic way. Drivers make use of it by embedding and filling in a
>> struct drm_dp_aux. Various helpers can then be used to for example read
>> from or write to the DPCD.
>>
>> Patch 1 adds the basic infrastructure as well as a couple of helpers to
>> access the DPCD.
>>
>> The helper introduced in patch 2 can be used to obtain the link status
>> as expected by various existing DP helpers.
>>
>> More convenience helpers are added in patch 3, which can come in handy
>> during DP initialization.
>>
>> An AUX channel can also be used to implement I2C-over-AUX and patch 4
>> implements an I2C adapter that can be used with the DRM EDID helpers.
>>
>> Changes in v3:
>> - address comments by Jani Nikula:
>>   - keep debug and error messages in AUX helpers
>>   - read/write back-to-back registers in chunks
>>   - separate link power up and configuration
>>   - do not power up for DPCD prior to 1.1
>>   - sleep after power up as per the spec
>>   - return number of bytes transferred
>>   - factor out some common code
>>   - reorder function arguments
>>   - fix typo in comment
>> - address comments by Daniel Vetter:
>>   - embed i2c_adapter within struct drm_dp_aux
>>   - describe error codes
>>
>> Changes in v2:
>> - reimplement I2C-over-AUX functionality to get rid of the additional
>>   layer
>> - extract retry logic from existing drivers
>> - add more kerneldoc comments
>>
>
> Just one small spelling fix in patch 3, with that fixed,
>

For the series:

> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> Thanks for doing this!
>
> Alex
>
>> Thierry
>>
>> Thierry Reding (4):
>>   drm/dp: Add AUX channel infrastructure
>>   drm/dp: Add drm_dp_dpcd_read_link_status()
>>   drm/dp: Add DisplayPort link helpers
>>   drm/dp: Allow registering AUX channels as I2C busses
>>
>>  drivers/gpu/drm/drm_dp_helper.c | 403 ++++++++++++++++++++++++++++++++++++++++
>>  include/drm/drm_dp_helper.h     |  92 +++++++++
>>  2 files changed, 495 insertions(+)
>>
>> --
>> 1.8.4.2
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/4] drm/dp: Add DisplayPort link helpers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 723 bytes --]

On Tue, Jan 14, 2014 at 10:52:55AM -0500, Alex Deucher wrote:
> On Tue, Jan 14, 2014 at 9:55 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > +/**
> > + * drm_dp_link_configure() - configure a DisplayPort link
> > + * @aux: DisplayPort AUX cahnnel
> > + * @link: pointer to a structure containing the link configuration
> > + *
> > + * Returns 0 on seccuss or a negative error code on failure.
> 
> spelling.  success.

Good catch. Thanks! There's also "cahnnel" above and when running a
spell checker it pointed out a few others. I've rolled fixes for all of
them into a v4, which I guess I'll resend anyway because I forgot to
attach the Tegra eDP implementation that I promised.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/4] drm/dp: Add DisplayPort link helpers

[Jani Nikula]

On Tue, 14 Jan 2014, Thierry Reding <thierry.reding@gmail.com> wrote:
> Add a helper to probe a DP link (read out the supported DPCD revision,
> maximum rate, link count and capabilities) as well as power up the DP
> link and configure it accordingly.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v3:
> - split into drm_dp_link_power_up() and drm_dp_link_configure()
> - do not change sink state for DPCD versions earlier than 1.1
> - sleep for 1-2 ms after setting local sink to D0 state
> - read and write consecutive registers where possible
> - read DPCD revision when link is probed
> - remove duplicate kerneldoc
>
>  drivers/gpu/drm/drm_dp_helper.c | 94 +++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_dp_helper.h     | 17 ++++++++
>  2 files changed, 111 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 572637456713..ac2e12789634 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -472,3 +472,97 @@ int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>  				DP_LINK_STATUS_SIZE);
>  }
>  EXPORT_SYMBOL(drm_dp_dpcd_read_link_status);
> +
> +/**
> + * drm_dp_link_probe() - probe a DisplayPort link for capabilities
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to structure in which to return link capabilities
> + *
> + * The structure filled in by this function can usually be passed directly
> + * into drm_dp_link_power_up() and drm_dp_link_configure() to power up and
> + * configure the link based on the link's capabilities.
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +	u8 values[3];
> +	int err;
> +
> +	memset(link, 0, sizeof(*link));
> +
> +	err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
> +	if (err < 0)
> +		return err;
> +
> +	link->revision = values[0];
> +	link->rate = drm_dp_bw_code_to_link_rate(values[1]);
> +	link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
> +
> +	if (values[2] & DP_ENHANCED_FRAME_CAP)
> +		link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;

Since DP_DPCD_REV == 0, you could use the #defines for the indexes (if
you're going to send another version anyway). Ditto below for
drm_dp_link_configure.

Other than that nitpick, the series looks good to me. If we face any
issues migrating i915 on top of this, we can iron them out later on.

On the series,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +
> +	return 0;
> +}
> +
> +/**
> + * drm_dp_link_power_up() - power up a DisplayPort link
> + * @aux: DisplayPort AUX channel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +	u8 value;
> +	int err;
> +
> +	/* DP_SET_POWER register is only available on DPCD v1.1 and later */
> +	if (link->revision < 0x11)
> +		return 0;
> +
> +	err = drm_dp_dpcd_readb(aux, DP_SET_POWER, &value);
> +	if (err < 0)
> +		return err;
> +
> +	value &= ~DP_SET_POWER_MASK;
> +	value |= DP_SET_POWER_D0;
> +
> +	err = drm_dp_dpcd_writeb(aux, DP_SET_POWER, value);
> +	if (err < 0)
> +		return err;
> +
> +	/*
> +	 * According to the DP 1.1 specification, a "Sink Device must exit the
> +	 * power saving state within 1 ms" (Section 2.5.3.1, Table 5-52, "Sink
> +	 * Control Field" (register 0x600).
> +	 */
> +	usleep_range(1000, 2000);
> +
> +	return 0;
> +}
> +
> +/**
> + * drm_dp_link_configure() - configure a DisplayPort link
> + * @aux: DisplayPort AUX cahnnel
> + * @link: pointer to a structure containing the link configuration
> + *
> + * Returns 0 on seccuss or a negative error code on failure.
> + */
> +int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link)
> +{
> +	u8 values[2];
> +	int err;
> +
> +	values[0] = drm_dp_link_rate_to_bw_code(link->rate);
> +	values[1] = link->num_lanes;
> +
> +	if (link->capabilities & DP_LINK_CAP_ENHANCED_FRAMING)
> +		values[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> +
> +	err = drm_dp_dpcd_write(aux, DP_LINK_BW_SET, values, sizeof(values));
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 8af695277a84..c7b3c736c40a 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -291,6 +291,7 @@
>  #define DP_SET_POWER                        0x600
>  # define DP_SET_POWER_D0                    0x1
>  # define DP_SET_POWER_D3                    0x2
> +# define DP_SET_POWER_MASK                  0x3
>  
>  #define DP_PSR_ERROR_STATUS                 0x2006  /* XXX 1.2? */
>  # define DP_PSR_LINK_CRC_ERROR              (1 << 0)
> @@ -468,4 +469,20 @@ static inline ssize_t drm_dp_dpcd_writeb(struct drm_dp_aux *aux,
>  int drm_dp_dpcd_read_link_status(struct drm_dp_aux *aux,
>  				 u8 status[DP_LINK_STATUS_SIZE]);
>  
> +/*
> + * DisplayPort link
> + */
> +#define DP_LINK_CAP_ENHANCED_FRAMING (1 << 0)
> +
> +struct drm_dp_link {
> +	unsigned char revision;
> +	unsigned int rate;
> +	unsigned int num_lanes;
> +	unsigned long capabilities;
> +};
> +
> +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +int drm_dp_link_power_up(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +int drm_dp_link_configure(struct drm_dp_aux *aux, struct drm_dp_link *link);
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> -- 
> 1.8.4.2
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v3 3/4] drm/dp: Add DisplayPort link helpers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1576 bytes --]

On Fri, Jan 17, 2014 at 03:22:08PM +0200, Jani Nikula wrote:
> On Tue, 14 Jan 2014, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > +int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
> > +{
> > +	u8 values[3];
> > +	int err;
> > +
> > +	memset(link, 0, sizeof(*link));
> > +
> > +	err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
> > +	if (err < 0)
> > +		return err;
> > +
> > +	link->revision = values[0];
> > +	link->rate = drm_dp_bw_code_to_link_rate(values[1]);
> > +	link->num_lanes = values[2] & DP_MAX_LANE_COUNT_MASK;
> > +
> > +	if (values[2] & DP_ENHANCED_FRAME_CAP)
> > +		link->capabilities |= DP_LINK_CAP_ENHANCED_FRAMING;
> 
> Since DP_DPCD_REV == 0, you could use the #defines for the indexes (if
> you're going to send another version anyway). Ditto below for
> drm_dp_link_configure.

We write to DP_LINK_BW_SET in drm_dp_link_configure() so I don't think
we can apply the same trick there. Also I'm not sure if it's really
worth having that here. Given that it only works if we actually read
DP_DPCD_REV, the number of locations where it can be done is fairly
small and they will look asymmetric with respect to other functions
using the drm_dp_dpcd_read/write() helpers.

So unless you feel strongly I'd prefer not to do this.

> Other than that nitpick, the series looks good to me. If we face any
> issues migrating i915 on top of this, we can iron them out later on.
> 
> On the series,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks!

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx