[PATCH 0/3] snd-firewire-lib: add handling CMP output connection

[PATCH 0/3] snd-firewire-lib: add handling CMP output connection

[Takashi Sakamoto]

Current implementation of snd-firewire-lib can handle only CMP input
connection. This series of patch enable it to handle both CMP output and input
connection according to IEC 61883-1.

Note that this series of patch still supports only a point-to-point connection
without overlaying. The overlaying and any broadcast connection are not
supported. Each connections gain its own isochronous resource.

Takashi Sakamoto (3):
  rename macros, variables and functions related to CMP plug
  Add "direction" member to cmp_connection structure
  Add handling CMP output connection

 sound/firewire/cmp.c      |  192 +++++++++++++++++++++++++++++++++------------
 sound/firewire/cmp.h      |   13 ++-
 sound/firewire/speakers.c |    2 +-
 3 files changed, 152 insertions(+), 55 deletions(-)

-- 
1.7.10.4


------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

[PATCH 1/3] rename macros, variables and functions related to CMP plug

[Takashi Sakamoto]

Referring to IEC 61883-1, oMPR and iMPR, oPCR and iPCR have some fields with
the same role in the same position. This patch renames some local macros, local
variables and function arguments which has "i" in its prefix to reuse them
between oMPR and iMPR, oPCR and iPCR.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/cmp.c |   91 +++++++++++++++++++++++++-------------------------
 sound/firewire/cmp.h |    6 ++--
 2 files changed, 49 insertions(+), 48 deletions(-)

diff --git a/sound/firewire/cmp.c b/sound/firewire/cmp.c
index 645cb0b..ec6d341 100644
--- a/sound/firewire/cmp.c
+++ b/sound/firewire/cmp.c
@@ -14,18 +14,20 @@
 #include "iso-resources.h"
 #include "cmp.h"
 
-#define IMPR_SPEED_MASK		0xc0000000
-#define IMPR_SPEED_SHIFT	30
-#define IMPR_XSPEED_MASK	0x00000060
-#define IMPR_XSPEED_SHIFT	5
-#define IMPR_PLUGS_MASK		0x0000001f
-
-#define IPCR_ONLINE		0x80000000
-#define IPCR_BCAST_CONN		0x40000000
-#define IPCR_P2P_CONN_MASK	0x3f000000
-#define IPCR_P2P_CONN_SHIFT	24
-#define IPCR_CHANNEL_MASK	0x003f0000
-#define IPCR_CHANNEL_SHIFT	16
+/* MPR common fields */
+#define MPR_SPEED_MASK		0xc0000000
+#define MPR_SPEED_SHIFT		30
+#define MPR_XSPEED_MASK		0x00000060
+#define MPR_XSPEED_SHIFT	5
+#define MPR_PLUGS_MASK		0x0000001f
+
+/* PCR common fields */
+#define PCR_ONLINE		0x80000000
+#define PCR_BCAST_CONN		0x40000000
+#define PCR_P2P_CONN_MASK	0x3f000000
+#define PCR_P2P_CONN_SHIFT	24
+#define PCR_CHANNEL_MASK	0x003f0000
+#define PCR_CHANNEL_SHIFT	16
 
 enum bus_reset_handling {
 	ABORT_ON_BUS_RESET,
@@ -96,24 +98,24 @@ bus_reset:
  * cmp_connection_init - initializes a connection manager
  * @c: the connection manager to initialize
  * @unit: a unit of the target device
- * @ipcr_index: the index of the iPCR on the target device
+ * @pcr_index: the index of the iPCR/oPCR on the target device
  */
 int cmp_connection_init(struct cmp_connection *c,
 			struct fw_unit *unit,
-			unsigned int ipcr_index)
+			unsigned int pcr_index)
 {
-	__be32 impr_be;
-	u32 impr;
+	__be32 mpr_be;
+	u32 mpr;
 	int err;
 
 	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
 				 CSR_REGISTER_BASE + CSR_IMPR,
-				 &impr_be, 4);
+				 &mpr_be, 4);
 	if (err < 0)
 		return err;
-	impr = be32_to_cpu(impr_be);
+	mpr = be32_to_cpu(mpr_be);
 
-	if (ipcr_index >= (impr & IMPR_PLUGS_MASK))
+	if (pcr_index >= (mpr & MPR_PLUGS_MASK))
 		return -EINVAL;
 
 	err = fw_iso_resources_init(&c->resources, unit);
@@ -123,10 +125,10 @@ int cmp_connection_init(struct cmp_connection *c,
 	c->connected = false;
 	mutex_init(&c->mutex);
 	c->last_pcr_value = cpu_to_be32(0x80000000);
-	c->pcr_index = ipcr_index;
-	c->max_speed = (impr & IMPR_SPEED_MASK) >> IMPR_SPEED_SHIFT;
+	c->pcr_index = pcr_index;
+	c->max_speed = (mpr & MPR_SPEED_MASK) >> MPR_SPEED_SHIFT;
 	if (c->max_speed == SCODE_BETA)
-		c->max_speed += (impr & IMPR_XSPEED_MASK) >> IMPR_XSPEED_SHIFT;
+		c->max_speed += (mpr & MPR_XSPEED_MASK) >> MPR_XSPEED_SHIFT;
 
 	return 0;
 }
@@ -147,23 +149,23 @@ EXPORT_SYMBOL(cmp_connection_destroy);
 
 static __be32 ipcr_set_modify(struct cmp_connection *c, __be32 ipcr)
 {
-	ipcr &= ~cpu_to_be32(IPCR_BCAST_CONN |
-			     IPCR_P2P_CONN_MASK |
-			     IPCR_CHANNEL_MASK);
-	ipcr |= cpu_to_be32(1 << IPCR_P2P_CONN_SHIFT);
-	ipcr |= cpu_to_be32(c->resources.channel << IPCR_CHANNEL_SHIFT);
+	ipcr &= ~cpu_to_be32(PCR_BCAST_CONN |
+			     PCR_P2P_CONN_MASK |
+			     PCR_CHANNEL_MASK);
+	ipcr |= cpu_to_be32(1 << PCR_P2P_CONN_SHIFT);
+	ipcr |= cpu_to_be32(c->resources.channel << PCR_CHANNEL_SHIFT);
 
 	return ipcr;
 }
 
-static int ipcr_set_check(struct cmp_connection *c, __be32 ipcr)
+static int pcr_set_check(struct cmp_connection *c, __be32 pcr)
 {
-	if (ipcr & cpu_to_be32(IPCR_BCAST_CONN |
-			       IPCR_P2P_CONN_MASK)) {
+	if (pcr & cpu_to_be32(PCR_BCAST_CONN |
+			       PCR_P2P_CONN_MASK)) {
 		cmp_error(c, "plug is already in use\n");
 		return -EBUSY;
 	}
-	if (!(ipcr & cpu_to_be32(IPCR_ONLINE))) {
+	if (!(pcr & cpu_to_be32(PCR_ONLINE))) {
 		cmp_error(c, "plug is not on-line\n");
 		return -ECONNREFUSED;
 	}
@@ -178,9 +180,9 @@ static int ipcr_set_check(struct cmp_connection *c, __be32 ipcr)
  *
  * This function establishes a point-to-point connection from the local
  * computer to the target by allocating isochronous resources (channel and
- * bandwidth) and setting the target's input plug control register.  When this
- * function succeeds, the caller is responsible for starting transmitting
- * packets.
+ * bandwidth) and setting the target's input/output plug control register.
+ * When this function succeeds, the caller is responsible for starting
+ * transmitting packets.
  */
 int cmp_connection_establish(struct cmp_connection *c,
 			     unsigned int max_payload_bytes)
@@ -201,7 +203,7 @@ retry_after_bus_reset:
 	if (err < 0)
 		goto err_mutex;
 
-	err = pcr_modify(c, ipcr_set_modify, ipcr_set_check,
+	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
 			 ABORT_ON_BUS_RESET);
 	if (err == -EAGAIN) {
 		fw_iso_resources_free(&c->resources);
@@ -229,8 +231,8 @@ EXPORT_SYMBOL(cmp_connection_establish);
  * cmp_connection_update - update the connection after a bus reset
  * @c: the connection manager
  *
- * This function must be called from the driver's .update handler to reestablish
- * any connection that might have been active.
+ * This function must be called from the driver's .update handler to
+ * reestablish any connection that might have been active.
  *
  * Returns zero on success, or a negative error code.  On an error, the
  * connection is broken and the caller must stop transmitting iso packets.
@@ -250,7 +252,7 @@ int cmp_connection_update(struct cmp_connection *c)
 	if (err < 0)
 		goto err_unconnect;
 
-	err = pcr_modify(c, ipcr_set_modify, ipcr_set_check,
+	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
 			 SUCCEED_ON_BUS_RESET);
 	if (err < 0)
 		goto err_resources;
@@ -269,19 +271,18 @@ err_unconnect:
 }
 EXPORT_SYMBOL(cmp_connection_update);
 
-
-static __be32 ipcr_break_modify(struct cmp_connection *c, __be32 ipcr)
+static __be32 pcr_break_modify(struct cmp_connection *c, __be32 pcr)
 {
-	return ipcr & ~cpu_to_be32(IPCR_BCAST_CONN | IPCR_P2P_CONN_MASK);
+	return pcr & ~cpu_to_be32(PCR_BCAST_CONN | PCR_P2P_CONN_MASK);
 }
 
 /**
  * cmp_connection_break - break the connection to the target
  * @c: the connection manager
  *
- * This function deactives the connection in the target's input plug control
- * register, and frees the isochronous resources of the connection.  Before
- * calling this function, the caller should cease transmitting packets.
+ * This function deactives the connection in the target's input/output plug
+ * control register, and frees the isochronous resources of the connection.
+ * Before calling this function, the caller should cease transmitting packets.
  */
 void cmp_connection_break(struct cmp_connection *c)
 {
@@ -294,7 +295,7 @@ void cmp_connection_break(struct cmp_connection *c)
 		return;
 	}
 
-	err = pcr_modify(c, ipcr_break_modify, NULL, SUCCEED_ON_BUS_RESET);
+	err = pcr_modify(c, pcr_break_modify, NULL, SUCCEED_ON_BUS_RESET);
 	if (err < 0)
 		cmp_error(c, "plug is still connected\n");
 
diff --git a/sound/firewire/cmp.h b/sound/firewire/cmp.h
index f47de08..2320cd4 100644
--- a/sound/firewire/cmp.h
+++ b/sound/firewire/cmp.h
@@ -11,8 +11,8 @@ struct fw_unit;
  * struct cmp_connection - manages an isochronous connection to a device
  * @speed: the connection's actual speed
  *
- * This structure manages (using CMP) an isochronous stream from the local
- * computer to a device's input plug (iPCR).
+ * This structure manages (using CMP) an isochronous stream between the local
+ * computer and a device's input plug (iPCR) and output plug (oPCR).
  *
  * There is no corresponding oPCR created on the local computer, so it is not
  * possible to overlay connections on top of this one.
@@ -30,7 +30,7 @@ struct cmp_connection {
 
 int cmp_connection_init(struct cmp_connection *connection,
 			struct fw_unit *unit,
-			unsigned int ipcr_index);
+			unsigned int pcr_index);
 void cmp_connection_destroy(struct cmp_connection *connection);
 
 int cmp_connection_establish(struct cmp_connection *connection,
-- 
1.7.10.4


------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

[PATCH 2/3] Add "direction" member to cmp_connection structure

[Takashi Sakamoto]

To indicate the direction of connection, this patch adds "direction" member to
cmp_connection structure. To determine the direction, this patch also adds
"direction" argument to cmp_connection_init() function. The
cmp_connection_init() function is exported and used in snd-firewire-speakers so
this patch also affect it.

This patch just add them. Actual implementation will be done by followed
patches.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/cmp.c      |    1 +
 sound/firewire/cmp.h      |    7 +++++++
 sound/firewire/speakers.c |    2 +-
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/firewire/cmp.c b/sound/firewire/cmp.c
index ec6d341..a8d3d5f 100644
--- a/sound/firewire/cmp.c
+++ b/sound/firewire/cmp.c
@@ -102,6 +102,7 @@ bus_reset:
  */
 int cmp_connection_init(struct cmp_connection *c,
 			struct fw_unit *unit,
+			enum cmp_direction direction,
 			unsigned int pcr_index)
 {
 	__be32 mpr_be;
diff --git a/sound/firewire/cmp.h b/sound/firewire/cmp.h
index 2320cd4..4d26cc9 100644
--- a/sound/firewire/cmp.h
+++ b/sound/firewire/cmp.h
@@ -7,6 +7,11 @@
 
 struct fw_unit;
 
+enum cmp_direction {
+	CMP_OUTPUT = 0,
+	CMP_INPUT
+};
+
 /**
  * struct cmp_connection - manages an isochronous connection to a device
  * @speed: the connection's actual speed
@@ -26,10 +31,12 @@ struct cmp_connection {
 	__be32 last_pcr_value;
 	unsigned int pcr_index;
 	unsigned int max_speed;
+	enum cmp_direction direction;
 };
 
 int cmp_connection_init(struct cmp_connection *connection,
 			struct fw_unit *unit,
+			enum cmp_direction direction,
 			unsigned int pcr_index);
 void cmp_connection_destroy(struct cmp_connection *connection);
 
diff --git a/sound/firewire/speakers.c b/sound/firewire/speakers.c
index d684655..1e1f003 100644
--- a/sound/firewire/speakers.c
+++ b/sound/firewire/speakers.c
@@ -723,7 +723,7 @@ static int fwspk_probe(struct device *unit_dev)
 		goto err_unit;
 	}
 
-	err = cmp_connection_init(&fwspk->connection, unit, 0);
+	err = cmp_connection_init(&fwspk->connection, unit, CMP_INPUT, 0);
 	if (err < 0)
 		goto err_unit;
 
-- 
1.7.10.4

[PATCH 3/3] Add handling CMP output connection

[Takashi Sakamoto]

To handle CMP output connection, this patch adds some macros, codes with
condition of direction and new functions. Once cmp_connection_init() is
executed with its direction, CMP input and output connection can be
handled by the same way.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/cmp.c |  106 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 97 insertions(+), 9 deletions(-)

diff --git a/sound/firewire/cmp.c b/sound/firewire/cmp.c
index a8d3d5f..334744d 100644
--- a/sound/firewire/cmp.c
+++ b/sound/firewire/cmp.c
@@ -29,6 +29,16 @@
 #define PCR_CHANNEL_MASK	0x003f0000
 #define PCR_CHANNEL_SHIFT	16
 
+/* oPCR specific fields */
+#define OPCR_XSPEED_MASK	0x00C00000
+#define OPCR_XSPEED_SHIFT	22
+#define OPCR_SPEED_MASK		0x0000C000
+#define OPCR_SPEED_SHIFT	14
+#define OPCR_OVERHEAD_ID_MASK	0x00003C00
+#define OPCR_OVERHEAD_ID_SHIFT	10
+#define OPCR_PAYLOAD_MASK	0x000003FF
+#define OPCR_PAYLOAD_SHIFT	0
+
 enum bus_reset_handling {
 	ABORT_ON_BUS_RESET,
 	SUCCEED_ON_BUS_RESET,
@@ -41,7 +51,8 @@ void cmp_error(struct cmp_connection *c, const char *fmt, ...)
 
 	va_start(va, fmt);
 	dev_err(&c->resources.unit->device, "%cPCR%u: %pV",
-		'i', c->pcr_index, &(struct va_format){ fmt, &va });
+		(c->direction == CMP_INPUT) ? 'i' : 'o',
+		c->pcr_index, &(struct va_format){ fmt, &va });
 	va_end(va);
 }
 
@@ -54,8 +65,14 @@ static int pcr_modify(struct cmp_connection *c,
 	int generation = c->resources.generation;
 	int rcode, errors = 0;
 	__be32 old_arg, buffer[2];
+	unsigned long long offset;
 	int err;
 
+	if (c->direction == CMP_INPUT)
+		offset = CSR_REGISTER_BASE + CSR_IPCR(c->pcr_index);
+	else
+		offset = CSR_REGISTER_BASE + CSR_OPCR(c->pcr_index);
+
 	buffer[0] = c->last_pcr_value;
 	for (;;) {
 		old_arg = buffer[0];
@@ -64,8 +81,7 @@ static int pcr_modify(struct cmp_connection *c,
 		rcode = fw_run_transaction(
 				device->card, TCODE_LOCK_COMPARE_SWAP,
 				device->node_id, generation, device->max_speed,
-				CSR_REGISTER_BASE + CSR_IPCR(c->pcr_index),
-				buffer, 8);
+				offset, buffer, 8);
 
 		if (rcode == RCODE_COMPLETE) {
 			if (buffer[0] == old_arg) /* success? */
@@ -107,11 +123,16 @@ int cmp_connection_init(struct cmp_connection *c,
 {
 	__be32 mpr_be;
 	u32 mpr;
+	unsigned long long offset;
 	int err;
 
+	if (c->direction == CMP_INPUT)
+		offset = CSR_REGISTER_BASE + CSR_IMPR;
+	else
+		offset = CSR_REGISTER_BASE + CSR_OMPR;
+
 	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
-				 CSR_REGISTER_BASE + CSR_IMPR,
-				 &mpr_be, 4);
+				 offset, &mpr_be, 4);
 	if (err < 0)
 		return err;
 	mpr = be32_to_cpu(mpr_be);
@@ -130,6 +151,7 @@ int cmp_connection_init(struct cmp_connection *c,
 	c->max_speed = (mpr & MPR_SPEED_MASK) >> MPR_SPEED_SHIFT;
 	if (c->max_speed == SCODE_BETA)
 		c->max_speed += (mpr & MPR_XSPEED_MASK) >> MPR_XSPEED_SHIFT;
+	c->direction = direction;
 
 	return 0;
 }
@@ -159,6 +181,62 @@ static __be32 ipcr_set_modify(struct cmp_connection *c, __be32 ipcr)
 	return ipcr;
 }
 
+static int get_overhead_id(struct cmp_connection *c)
+{
+	int id;
+
+	/*
+	 * apply "oPCR overhead ID encoding"
+	 * the encoding table can convert up to 512.
+	 * here the value over 512 is converted as the same way as 512.
+	 */
+	for (id = 1; id < 16; id += 1) {
+		if (c->resources.bandwidth_overhead < (id << 5))
+			break;
+	}
+	if (id == 16)
+		id = 0;
+
+	return id;
+}
+
+static __be32 opcr_set_modify(struct cmp_connection *c, __be32 opcr)
+{
+	unsigned int spd, xspd;
+
+	/* generate speed and extended speed field value */
+	if (c->speed > SCODE_400) {
+		spd  = SCODE_800;
+		xspd = c->speed - SCODE_800;
+	} else {
+		spd = c->speed;
+		xspd = 0;
+	}
+
+	opcr &= ~cpu_to_be32(PCR_BCAST_CONN |
+			     PCR_P2P_CONN_MASK |
+			     OPCR_XSPEED_MASK |
+			     PCR_CHANNEL_MASK |
+			     OPCR_SPEED_MASK |
+			     OPCR_OVERHEAD_ID_MASK |
+			     OPCR_PAYLOAD_MASK);
+	opcr |= cpu_to_be32(1 << PCR_P2P_CONN_SHIFT);
+	opcr |= cpu_to_be32(xspd << OPCR_XSPEED_SHIFT);
+	opcr |= cpu_to_be32(c->resources.channel << PCR_CHANNEL_SHIFT);
+	opcr |= cpu_to_be32(spd << OPCR_SPEED_SHIFT);
+	opcr |= cpu_to_be32(get_overhead_id(c) << OPCR_OVERHEAD_ID_SHIFT);
+	/*
+	 * here zero is applied to payload field.
+	 * it means the maximum number of quadlets in an isochronous packet is
+	 * 1024 when spd is less than three, 1024 * 2 * xspd + 1 when spd is
+	 * equal to three. An arbitrary value can be set here but 0 is enough
+	 * for our purpose.
+	 */
+	opcr |= cpu_to_be32(0 << OPCR_PAYLOAD_SHIFT);
+
+	return opcr;
+}
+
 static int pcr_set_check(struct cmp_connection *c, __be32 pcr)
 {
 	if (pcr & cpu_to_be32(PCR_BCAST_CONN |
@@ -204,8 +282,13 @@ retry_after_bus_reset:
 	if (err < 0)
 		goto err_mutex;
 
-	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
-			 ABORT_ON_BUS_RESET);
+	if (c->direction == CMP_OUTPUT)
+		err = pcr_modify(c, opcr_set_modify, pcr_set_check,
+				 ABORT_ON_BUS_RESET);
+	else
+		err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
+				 ABORT_ON_BUS_RESET);
+
 	if (err == -EAGAIN) {
 		fw_iso_resources_free(&c->resources);
 		goto retry_after_bus_reset;
@@ -253,8 +336,13 @@ int cmp_connection_update(struct cmp_connection *c)
 	if (err < 0)
 		goto err_unconnect;
 
-	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
-			 SUCCEED_ON_BUS_RESET);
+	if (c->direction == CMP_OUTPUT)
+		err = pcr_modify(c, opcr_set_modify, pcr_set_check,
+				 SUCCEED_ON_BUS_RESET);
+	else
+		err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
+				 SUCCEED_ON_BUS_RESET);
+
 	if (err < 0)
 		goto err_resources;
 
-- 
1.7.10.4


------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

Re: [PATCH 3/3] Add handling CMP output connection

[Clemens Ladisch]

Takashi Sakamoto wrote:
> To handle CMP output connection, this patch adds some macros, codes with
> condition of direction and new functions. Once cmp_connection_init() is
> executed with its direction, CMP input and output connection can be
> handled by the same way.

> +++ b/sound/firewire/cmp.c

> +	if (c->direction == CMP_INPUT)
> +		offset = CSR_REGISTER_BASE + CSR_IPCR(c->pcr_index);
> +	else
> +		offset = CSR_REGISTER_BASE + CSR_OPCR(c->pcr_index);

This code is used twice and could be moved into a helper function.

> +static int get_overhead_id(struct cmp_connection *c)

> +	/*
> +	 * apply "oPCR overhead ID encoding"
> +	 * the encoding table can convert up to 512.
> +	 * here the value over 512 is converted as the same way as 512.
> +	 */

	/*
	 * Apply "oPCR overhead ID encoding":
	 * The encoding table can convert up to 512.
	 * Here any value over 512 is converted in the same way as 512.
	 */

> +	for (id = 1; id < 16; id += 1) {
> +		if (c->resources.bandwidth_overhead < (id << 5))
> +			break;
> +	}
> +	if (id == 16)
> +		id = 0;

	id = DIV_ROUND_UP(c->resources.bandwidth_overhead, 32);
	if (id >= 16)
		id = 0;

> +static __be32 opcr_set_modify(struct cmp_connection *c, __be32 opcr)

> +	/* generate speed and extended speed field value */

This comment is superfluous; it does not tell anything non-obvious about
the code.

> +	/*
> +	 * here zero is applied to payload field.
> +	 * it means the maximum number of quadlets in an isochronous packet is
> +	 * 1024 when spd is less than three, 1024 * 2 * xspd + 1 when spd is
> +	 * equal to three. An arbitrary value can be set here but 0 is enough
> +	 * for our purpose.
> +	 */
> +	opcr |= cpu_to_be32(0 << OPCR_PAYLOAD_SHIFT);

In an oPCR, the payload field is changed only by the device.


Regards,
Clemens

Re: [PATCH 3/3] Add handling CMP output connection

[Takashi Sakamoto]

Clemens,

Thanks for your review. I arrange these issues to 5 items below.

cmp[PATCH 3/3]:
1. Calculating offset should be moved into a helper function because 
they are used twice.
OK. I push them into a helper function.

2. Comments should be start with capital letter in each sentenses.
OK. I rewrite it.

3. Use DIV_ROUND_UP macro instead of "for" loop.
OK. I rewrite with the macro.

4. This superfluous comment should say anything to be obvious about the 
code.
OK. I remove the comment.

5. In an oPCR, the payload field is changed only by the device.
OK. I misunderstand the specification. I check IEC 611883-1:2008 and I 
should follow the rules in "7.9 Plug control register modification 
rules". It menthions the fields which should be modified in the same 
compare_swap lock transaction and payload field is not included in it.

Then I have a question about handling payload field. In specification, 
"The payload field shall specify the maximum number of quadlets that may 
be transmitted in a single isochronous packet for this plug" but 
actually cheking this field seems to make no sense because there is no 
checking process in the procedure. I think I can do nothing for payload 
field.


Regards

Takashi Sakamoto
o-takashi@sakamocchi.jp

(Apr 28 2013 21:52), Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> To handle CMP output connection, this patch adds some macros, codes with
>> condition of direction and new functions. Once cmp_connection_init() is
>> executed with its direction, CMP input and output connection can be
>> handled by the same way.
>
>> +++ b/sound/firewire/cmp.c
>
>> +	if (c->direction == CMP_INPUT)
>> +		offset = CSR_REGISTER_BASE + CSR_IPCR(c->pcr_index);
>> +	else
>> +		offset = CSR_REGISTER_BASE + CSR_OPCR(c->pcr_index);
>
> This code is used twice and could be moved into a helper function.
>
>> +static int get_overhead_id(struct cmp_connection *c)
>
>> +	/*
>> +	 * apply "oPCR overhead ID encoding"
>> +	 * the encoding table can convert up to 512.
>> +	 * here the value over 512 is converted as the same way as 512.
>> +	 */
>
> 	/*
> 	 * Apply "oPCR overhead ID encoding":
> 	 * The encoding table can convert up to 512.
> 	 * Here any value over 512 is converted in the same way as 512.
> 	 */
>
>> +	for (id = 1; id < 16; id += 1) {
>> +		if (c->resources.bandwidth_overhead < (id << 5))
>> +			break;
>> +	}
>> +	if (id == 16)
>> +		id = 0;
>
> 	id = DIV_ROUND_UP(c->resources.bandwidth_overhead, 32);
> 	if (id >= 16)
> 		id = 0;
>
>> +static __be32 opcr_set_modify(struct cmp_connection *c, __be32 opcr)
>
>> +	/* generate speed and extended speed field value */
>
> This comment is superfluous; it does not tell anything non-obvious about
> the code.
>
>> +	/*
>> +	 * here zero is applied to payload field.
>> +	 * it means the maximum number of quadlets in an isochronous packet is
>> +	 * 1024 when spd is less than three, 1024 * 2 * xspd + 1 when spd is
>> +	 * equal to three. An arbitrary value can be set here but 0 is enough
>> +	 * for our purpose.
>> +	 */
>> +	opcr |= cpu_to_be32(0 << OPCR_PAYLOAD_SHIFT);
>
> In an oPCR, the payload field is changed only by the device.
>
>
> Regards,
> Clemens

Re: [PATCH 3/3] Add handling CMP output connection

[Clemens Ladisch]

Takashi Sakamoto wrote:
> Then I have a question about handling payload field. In specification,
> "The payload field shall specify the maximum number of quadlets that
> may be transmitted in a single isochronous packet for this plug" but
> actually cheking this field seems to make no sense because there is no
> checking process in the procedure. I think I can do nothing for
> payload field.

The specification was written for the general case where the device that
creates the connection might be different from the transmitting and
receiving devices.  In that case, the payload size is needed for correct
bandwidth allocation.

In our case, the Linux PC does not have plugs that could be accessed by
anybody else.  (And I'm not sure if the value set by the Fireworks
firmware is correct enough so that we'd want to use it.)


Regards,
Clemens

Re: [PATCH 3/3] Add handling CMP output connection

[Takashi Sakamoto]

Clemens,

 > The specification was written for the general case where the device
 > that creates the connection might be different from the transmitting
 > and receiving devices.  In that case, the payload size is needed for
 > correct bandwidth allocation.

I'm sure that the general case.

 > In our case, the Linux PC does not have plugs that could be accessed
 > by anybody else.

I'm sure that host system (Linux PC) doesn't have no plugs.

 >  (And I'm not sure if the value set by the Fireworks
 > firmware is correct enough so that we'd want to use it.)

With my Fireworks device, it always report 0, it means 1024 bytes for 
max payload size for output plug. But I'm not sure that it always report 
the same value. Especially at 192.0 kHz, the payload size is over 1024 
bytes but my device supports up to 96.0 kHz...

Actually the target device seems to update the max payload size after 
host connects to CMP output plug because its initial value is zero. Then 
we already allocate isochronous resources and isochronous packet to host 
system. I don't think it better to reallocate them after the connecting.

It's reasonable to ignore the payload field in any processing.


Regards

Takashi Sakamoto

(Apr 29 2013 15:30), Clemens Ladisch wrote:
> Takashi Sakamoto wrote:
>> Then I have a question about handling payload field. In specification,
>> "The payload field shall specify the maximum number of quadlets that
>> may be transmitted in a single isochronous packet for this plug" but
>> actually cheking this field seems to make no sense because there is no
>> checking process in the procedure. I think I can do nothing for
>> payload field.
>
> The specification was written for the general case where the device that
> creates the connection might be different from the transmitting and
> receiving devices.  In that case, the payload size is needed for correct
> bandwidth allocation.
>
> In our case, the Linux PC does not have plugs that could be accessed by
> anybody else.  (And I'm not sure if the value set by the Fireworks
> firmware is correct enough so that we'd want to use it.)
>
>
> Regards,
> Clemens


------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

[PATCH v2 0/3] snd-firewire-lib: add handling CMP output connection

[Takashi Sakamoto]

Current implementation of snd-firewire-lib can handle only CMP input
connection. This series of patch enable it to handle both CMP output and input
connection according to IEC 61883-1.

Note that this series of patch still supports only a point-to-point connection
without overlaying. The overlaying and any broadcast connection are not
supported. Each connections gain its own isochronous resource.

Takashi Sakamoto (3):
  rename macros, variables and functions related to CMP plug
  Add "direction" member to cmp_connection structure
  Add handling CMP output connection

 sound/firewire/cmp.c      |  188 +++++++++++++++++++++++++++++++++------------
 sound/firewire/cmp.h      |   13 +++-
 sound/firewire/speakers.c |    2 +-
 3 files changed, 148 insertions(+), 55 deletions(-)

-- 
1.7.10.4

[PATCH 1/3] rename macros, variables and functions related to CMP plug

[Takashi Sakamoto]

Referring to IEC 61883-1, oMPR and iMPR, oPCR and iPCR have some fields with
the same role in the same position. This patch renames some local macros, local
variables and function arguments which has "i" in its prefix to reuse them
between oMPR and iMPR, oPCR and iPCR.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/cmp.c |   91 +++++++++++++++++++++++++-------------------------
 sound/firewire/cmp.h |    6 ++--
 2 files changed, 49 insertions(+), 48 deletions(-)

diff --git a/sound/firewire/cmp.c b/sound/firewire/cmp.c
index 645cb0b..ec6d341 100644
--- a/sound/firewire/cmp.c
+++ b/sound/firewire/cmp.c
@@ -14,18 +14,20 @@
 #include "iso-resources.h"
 #include "cmp.h"
 
-#define IMPR_SPEED_MASK		0xc0000000
-#define IMPR_SPEED_SHIFT	30
-#define IMPR_XSPEED_MASK	0x00000060
-#define IMPR_XSPEED_SHIFT	5
-#define IMPR_PLUGS_MASK		0x0000001f
-
-#define IPCR_ONLINE		0x80000000
-#define IPCR_BCAST_CONN		0x40000000
-#define IPCR_P2P_CONN_MASK	0x3f000000
-#define IPCR_P2P_CONN_SHIFT	24
-#define IPCR_CHANNEL_MASK	0x003f0000
-#define IPCR_CHANNEL_SHIFT	16
+/* MPR common fields */
+#define MPR_SPEED_MASK		0xc0000000
+#define MPR_SPEED_SHIFT		30
+#define MPR_XSPEED_MASK		0x00000060
+#define MPR_XSPEED_SHIFT	5
+#define MPR_PLUGS_MASK		0x0000001f
+
+/* PCR common fields */
+#define PCR_ONLINE		0x80000000
+#define PCR_BCAST_CONN		0x40000000
+#define PCR_P2P_CONN_MASK	0x3f000000
+#define PCR_P2P_CONN_SHIFT	24
+#define PCR_CHANNEL_MASK	0x003f0000
+#define PCR_CHANNEL_SHIFT	16
 
 enum bus_reset_handling {
 	ABORT_ON_BUS_RESET,
@@ -96,24 +98,24 @@ bus_reset:
  * cmp_connection_init - initializes a connection manager
  * @c: the connection manager to initialize
  * @unit: a unit of the target device
- * @ipcr_index: the index of the iPCR on the target device
+ * @pcr_index: the index of the iPCR/oPCR on the target device
  */
 int cmp_connection_init(struct cmp_connection *c,
 			struct fw_unit *unit,
-			unsigned int ipcr_index)
+			unsigned int pcr_index)
 {
-	__be32 impr_be;
-	u32 impr;
+	__be32 mpr_be;
+	u32 mpr;
 	int err;
 
 	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
 				 CSR_REGISTER_BASE + CSR_IMPR,
-				 &impr_be, 4);
+				 &mpr_be, 4);
 	if (err < 0)
 		return err;
-	impr = be32_to_cpu(impr_be);
+	mpr = be32_to_cpu(mpr_be);
 
-	if (ipcr_index >= (impr & IMPR_PLUGS_MASK))
+	if (pcr_index >= (mpr & MPR_PLUGS_MASK))
 		return -EINVAL;
 
 	err = fw_iso_resources_init(&c->resources, unit);
@@ -123,10 +125,10 @@ int cmp_connection_init(struct cmp_connection *c,
 	c->connected = false;
 	mutex_init(&c->mutex);
 	c->last_pcr_value = cpu_to_be32(0x80000000);
-	c->pcr_index = ipcr_index;
-	c->max_speed = (impr & IMPR_SPEED_MASK) >> IMPR_SPEED_SHIFT;
+	c->pcr_index = pcr_index;
+	c->max_speed = (mpr & MPR_SPEED_MASK) >> MPR_SPEED_SHIFT;
 	if (c->max_speed == SCODE_BETA)
-		c->max_speed += (impr & IMPR_XSPEED_MASK) >> IMPR_XSPEED_SHIFT;
+		c->max_speed += (mpr & MPR_XSPEED_MASK) >> MPR_XSPEED_SHIFT;
 
 	return 0;
 }
@@ -147,23 +149,23 @@ EXPORT_SYMBOL(cmp_connection_destroy);
 
 static __be32 ipcr_set_modify(struct cmp_connection *c, __be32 ipcr)
 {
-	ipcr &= ~cpu_to_be32(IPCR_BCAST_CONN |
-			     IPCR_P2P_CONN_MASK |
-			     IPCR_CHANNEL_MASK);
-	ipcr |= cpu_to_be32(1 << IPCR_P2P_CONN_SHIFT);
-	ipcr |= cpu_to_be32(c->resources.channel << IPCR_CHANNEL_SHIFT);
+	ipcr &= ~cpu_to_be32(PCR_BCAST_CONN |
+			     PCR_P2P_CONN_MASK |
+			     PCR_CHANNEL_MASK);
+	ipcr |= cpu_to_be32(1 << PCR_P2P_CONN_SHIFT);
+	ipcr |= cpu_to_be32(c->resources.channel << PCR_CHANNEL_SHIFT);
 
 	return ipcr;
 }
 
-static int ipcr_set_check(struct cmp_connection *c, __be32 ipcr)
+static int pcr_set_check(struct cmp_connection *c, __be32 pcr)
 {
-	if (ipcr & cpu_to_be32(IPCR_BCAST_CONN |
-			       IPCR_P2P_CONN_MASK)) {
+	if (pcr & cpu_to_be32(PCR_BCAST_CONN |
+			       PCR_P2P_CONN_MASK)) {
 		cmp_error(c, "plug is already in use\n");
 		return -EBUSY;
 	}
-	if (!(ipcr & cpu_to_be32(IPCR_ONLINE))) {
+	if (!(pcr & cpu_to_be32(PCR_ONLINE))) {
 		cmp_error(c, "plug is not on-line\n");
 		return -ECONNREFUSED;
 	}
@@ -178,9 +180,9 @@ static int ipcr_set_check(struct cmp_connection *c, __be32 ipcr)
  *
  * This function establishes a point-to-point connection from the local
  * computer to the target by allocating isochronous resources (channel and
- * bandwidth) and setting the target's input plug control register.  When this
- * function succeeds, the caller is responsible for starting transmitting
- * packets.
+ * bandwidth) and setting the target's input/output plug control register.
+ * When this function succeeds, the caller is responsible for starting
+ * transmitting packets.
  */
 int cmp_connection_establish(struct cmp_connection *c,
 			     unsigned int max_payload_bytes)
@@ -201,7 +203,7 @@ retry_after_bus_reset:
 	if (err < 0)
 		goto err_mutex;
 
-	err = pcr_modify(c, ipcr_set_modify, ipcr_set_check,
+	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
 			 ABORT_ON_BUS_RESET);
 	if (err == -EAGAIN) {
 		fw_iso_resources_free(&c->resources);
@@ -229,8 +231,8 @@ EXPORT_SYMBOL(cmp_connection_establish);
  * cmp_connection_update - update the connection after a bus reset
  * @c: the connection manager
  *
- * This function must be called from the driver's .update handler to reestablish
- * any connection that might have been active.
+ * This function must be called from the driver's .update handler to
+ * reestablish any connection that might have been active.
  *
  * Returns zero on success, or a negative error code.  On an error, the
  * connection is broken and the caller must stop transmitting iso packets.
@@ -250,7 +252,7 @@ int cmp_connection_update(struct cmp_connection *c)
 	if (err < 0)
 		goto err_unconnect;
 
-	err = pcr_modify(c, ipcr_set_modify, ipcr_set_check,
+	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
 			 SUCCEED_ON_BUS_RESET);
 	if (err < 0)
 		goto err_resources;
@@ -269,19 +271,18 @@ err_unconnect:
 }
 EXPORT_SYMBOL(cmp_connection_update);
 
-
-static __be32 ipcr_break_modify(struct cmp_connection *c, __be32 ipcr)
+static __be32 pcr_break_modify(struct cmp_connection *c, __be32 pcr)
 {
-	return ipcr & ~cpu_to_be32(IPCR_BCAST_CONN | IPCR_P2P_CONN_MASK);
+	return pcr & ~cpu_to_be32(PCR_BCAST_CONN | PCR_P2P_CONN_MASK);
 }
 
 /**
  * cmp_connection_break - break the connection to the target
  * @c: the connection manager
  *
- * This function deactives the connection in the target's input plug control
- * register, and frees the isochronous resources of the connection.  Before
- * calling this function, the caller should cease transmitting packets.
+ * This function deactives the connection in the target's input/output plug
+ * control register, and frees the isochronous resources of the connection.
+ * Before calling this function, the caller should cease transmitting packets.
  */
 void cmp_connection_break(struct cmp_connection *c)
 {
@@ -294,7 +295,7 @@ void cmp_connection_break(struct cmp_connection *c)
 		return;
 	}
 
-	err = pcr_modify(c, ipcr_break_modify, NULL, SUCCEED_ON_BUS_RESET);
+	err = pcr_modify(c, pcr_break_modify, NULL, SUCCEED_ON_BUS_RESET);
 	if (err < 0)
 		cmp_error(c, "plug is still connected\n");
 
diff --git a/sound/firewire/cmp.h b/sound/firewire/cmp.h
index f47de08..2320cd4 100644
--- a/sound/firewire/cmp.h
+++ b/sound/firewire/cmp.h
@@ -11,8 +11,8 @@ struct fw_unit;
  * struct cmp_connection - manages an isochronous connection to a device
  * @speed: the connection's actual speed
  *
- * This structure manages (using CMP) an isochronous stream from the local
- * computer to a device's input plug (iPCR).
+ * This structure manages (using CMP) an isochronous stream between the local
+ * computer and a device's input plug (iPCR) and output plug (oPCR).
  *
  * There is no corresponding oPCR created on the local computer, so it is not
  * possible to overlay connections on top of this one.
@@ -30,7 +30,7 @@ struct cmp_connection {
 
 int cmp_connection_init(struct cmp_connection *connection,
 			struct fw_unit *unit,
-			unsigned int ipcr_index);
+			unsigned int pcr_index);
 void cmp_connection_destroy(struct cmp_connection *connection);
 
 int cmp_connection_establish(struct cmp_connection *connection,
-- 
1.7.10.4


------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

[PATCH 2/3] Add "direction" member to cmp_connection structure

[Takashi Sakamoto]

To indicate the direction of connection, this patch adds "direction" member to
cmp_connection structure. To determine the direction, this patch also adds
"direction" argument to cmp_connection_init() function. The
cmp_connection_init() function is exported and used in snd-firewire-speakers so
this patch also affect it.

This patch just add them. Actual implementation will be done by followed
patches.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/cmp.c      |    1 +
 sound/firewire/cmp.h      |    7 +++++++
 sound/firewire/speakers.c |    2 +-
 3 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/sound/firewire/cmp.c b/sound/firewire/cmp.c
index ec6d341..a8d3d5f 100644
--- a/sound/firewire/cmp.c
+++ b/sound/firewire/cmp.c
@@ -102,6 +102,7 @@ bus_reset:
  */
 int cmp_connection_init(struct cmp_connection *c,
 			struct fw_unit *unit,
+			enum cmp_direction direction,
 			unsigned int pcr_index)
 {
 	__be32 mpr_be;
diff --git a/sound/firewire/cmp.h b/sound/firewire/cmp.h
index 2320cd4..4d26cc9 100644
--- a/sound/firewire/cmp.h
+++ b/sound/firewire/cmp.h
@@ -7,6 +7,11 @@
 
 struct fw_unit;
 
+enum cmp_direction {
+	CMP_OUTPUT = 0,
+	CMP_INPUT
+};
+
 /**
  * struct cmp_connection - manages an isochronous connection to a device
  * @speed: the connection's actual speed
@@ -26,10 +31,12 @@ struct cmp_connection {
 	__be32 last_pcr_value;
 	unsigned int pcr_index;
 	unsigned int max_speed;
+	enum cmp_direction direction;
 };
 
 int cmp_connection_init(struct cmp_connection *connection,
 			struct fw_unit *unit,
+			enum cmp_direction direction,
 			unsigned int pcr_index);
 void cmp_connection_destroy(struct cmp_connection *connection);
 
diff --git a/sound/firewire/speakers.c b/sound/firewire/speakers.c
index d684655..1e1f003 100644
--- a/sound/firewire/speakers.c
+++ b/sound/firewire/speakers.c
@@ -723,7 +723,7 @@ static int fwspk_probe(struct device *unit_dev)
 		goto err_unit;
 	}
 
-	err = cmp_connection_init(&fwspk->connection, unit, 0);
+	err = cmp_connection_init(&fwspk->connection, unit, CMP_INPUT, 0);
 	if (err < 0)
 		goto err_unit;
 
-- 
1.7.10.4


------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

[PATCH 3/3] Add handling CMP output connection

[Takashi Sakamoto]

To handle CMP output connection, this patch adds some macros, codes with
condition of direction and new functions. Once cmp_connection_init() is
executed with its direction, CMP input and output connection can be
handled by the same way.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/firewire/cmp.c |  102 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 93 insertions(+), 9 deletions(-)

diff --git a/sound/firewire/cmp.c b/sound/firewire/cmp.c
index a8d3d5f..9e70077 100644
--- a/sound/firewire/cmp.c
+++ b/sound/firewire/cmp.c
@@ -29,6 +29,16 @@
 #define PCR_CHANNEL_MASK	0x003f0000
 #define PCR_CHANNEL_SHIFT	16
 
+/* oPCR specific fields */
+#define OPCR_XSPEED_MASK	0x00C00000
+#define OPCR_XSPEED_SHIFT	22
+#define OPCR_SPEED_MASK		0x0000C000
+#define OPCR_SPEED_SHIFT	14
+#define OPCR_OVERHEAD_ID_MASK	0x00003C00
+#define OPCR_OVERHEAD_ID_SHIFT	10
+#define OPCR_PAYLOAD_MASK	0x000003FF
+#define OPCR_PAYLOAD_SHIFT	0
+
 enum bus_reset_handling {
 	ABORT_ON_BUS_RESET,
 	SUCCEED_ON_BUS_RESET,
@@ -41,10 +51,30 @@ void cmp_error(struct cmp_connection *c, const char *fmt, ...)
 
 	va_start(va, fmt);
 	dev_err(&c->resources.unit->device, "%cPCR%u: %pV",
-		'i', c->pcr_index, &(struct va_format){ fmt, &va });
+		(c->direction == CMP_INPUT) ? 'i' : 'o',
+		c->pcr_index, &(struct va_format){ fmt, &va });
 	va_end(va);
 }
 
+static unsigned long long get_offset(struct cmp_connection *c, bool master)
+{
+	unsigned long long offset = CSR_REGISTER_BASE;
+
+	if (!master) {
+		if (c->direction == CMP_INPUT)
+			offset += CSR_IPCR(c->pcr_index);
+		else
+			offset += CSR_OPCR(c->pcr_index);
+	} else {
+		if (c->direction == CMP_INPUT)
+			offset += CSR_IMPR;
+		else
+			offset += CSR_OMPR;
+	}
+
+	return offset;
+}
+
 static int pcr_modify(struct cmp_connection *c,
 		      __be32 (*modify)(struct cmp_connection *c, __be32 old),
 		      int (*check)(struct cmp_connection *c, __be32 pcr),
@@ -64,8 +94,7 @@ static int pcr_modify(struct cmp_connection *c,
 		rcode = fw_run_transaction(
 				device->card, TCODE_LOCK_COMPARE_SWAP,
 				device->node_id, generation, device->max_speed,
-				CSR_REGISTER_BASE + CSR_IPCR(c->pcr_index),
-				buffer, 8);
+				get_offset(c, false), buffer, 8);
 
 		if (rcode == RCODE_COMPLETE) {
 			if (buffer[0] == old_arg) /* success? */
@@ -110,8 +139,7 @@ int cmp_connection_init(struct cmp_connection *c,
 	int err;
 
 	err = snd_fw_transaction(unit, TCODE_READ_QUADLET_REQUEST,
-				 CSR_REGISTER_BASE + CSR_IMPR,
-				 &mpr_be, 4);
+				 get_offset(c, true), &mpr_be, 4);
 	if (err < 0)
 		return err;
 	mpr = be32_to_cpu(mpr_be);
@@ -130,6 +158,7 @@ int cmp_connection_init(struct cmp_connection *c,
 	c->max_speed = (mpr & MPR_SPEED_MASK) >> MPR_SPEED_SHIFT;
 	if (c->max_speed == SCODE_BETA)
 		c->max_speed += (mpr & MPR_XSPEED_MASK) >> MPR_XSPEED_SHIFT;
+	c->direction = direction;
 
 	return 0;
 }
@@ -159,6 +188,51 @@ static __be32 ipcr_set_modify(struct cmp_connection *c, __be32 ipcr)
 	return ipcr;
 }
 
+static int get_overhead_id(struct cmp_connection *c)
+{
+	int id;
+
+	/*
+	 * Apply "oPCR overhead ID encoding"
+	 * The encoding table can convert up to 512.
+	 * Here the value over 512 is converted as the same way as 512.
+	 */
+	id = DIV_ROUND_UP(c->resources.bandwidth_overhead, 32);
+	if (id >= 16)
+		id = 0;
+
+	return id;
+}
+
+static __be32 opcr_set_modify(struct cmp_connection *c, __be32 opcr)
+{
+	unsigned int spd, xspd;
+
+	if (c->speed > SCODE_400) {
+		spd  = SCODE_800;
+		xspd = c->speed - SCODE_800;
+	} else {
+		spd = c->speed;
+		xspd = 0;
+	}
+
+	opcr &= ~cpu_to_be32(PCR_BCAST_CONN |
+			     PCR_P2P_CONN_MASK |
+			     OPCR_XSPEED_MASK |
+			     PCR_CHANNEL_MASK |
+			     OPCR_SPEED_MASK |
+			     OPCR_OVERHEAD_ID_MASK);
+	opcr |= cpu_to_be32(1 << PCR_P2P_CONN_SHIFT);
+	opcr |= cpu_to_be32(xspd << OPCR_XSPEED_SHIFT);
+	opcr |= cpu_to_be32(c->resources.channel << PCR_CHANNEL_SHIFT);
+	opcr |= cpu_to_be32(spd << OPCR_SPEED_SHIFT);
+	opcr |= cpu_to_be32(get_overhead_id(c) << OPCR_OVERHEAD_ID_SHIFT);
+
+	/* NOTE: payload field is set by target device */
+
+	return opcr;
+}
+
 static int pcr_set_check(struct cmp_connection *c, __be32 pcr)
 {
 	if (pcr & cpu_to_be32(PCR_BCAST_CONN |
@@ -204,8 +278,13 @@ retry_after_bus_reset:
 	if (err < 0)
 		goto err_mutex;
 
-	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
-			 ABORT_ON_BUS_RESET);
+	if (c->direction == CMP_OUTPUT)
+		err = pcr_modify(c, opcr_set_modify, pcr_set_check,
+				 ABORT_ON_BUS_RESET);
+	else
+		err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
+				 ABORT_ON_BUS_RESET);
+
 	if (err == -EAGAIN) {
 		fw_iso_resources_free(&c->resources);
 		goto retry_after_bus_reset;
@@ -253,8 +332,13 @@ int cmp_connection_update(struct cmp_connection *c)
 	if (err < 0)
 		goto err_unconnect;
 
-	err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
-			 SUCCEED_ON_BUS_RESET);
+	if (c->direction == CMP_OUTPUT)
+		err = pcr_modify(c, opcr_set_modify, pcr_set_check,
+				 SUCCEED_ON_BUS_RESET);
+	else
+		err = pcr_modify(c, ipcr_set_modify, pcr_set_check,
+				 SUCCEED_ON_BUS_RESET);
+
 	if (err < 0)
 		goto err_resources;
 
-- 
1.7.10.4


------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service 
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_apr

Re: [PATCH 3/3] Add handling CMP output connection

[Takashi Sakamoto]

I'm sorry but I correct what I said:

 > Actually the target device seems to update the max payload size after
 > host connects to CMP output plug because its initial value is zero.
 > Then we already allocate isochronous resources and isochronous packet
 > to host system. I don't think it better to reallocate them after the
 > connecting.

We don't allocate buffer for isochronous packet yet when connecting CMP 
output plug. Then we just gain isochronous resource.

Anyway I don't think it better to reallocate isochronous resource.


Regards

Takashi Sakamoto

(Apr 29 2013 18:25), Takashi Sakamoto wrote:
> Clemens,
>
>  > The specification was written for the general case where the device
>  > that creates the connection might be different from the transmitting
>  > and receiving devices.  In that case, the payload size is needed for
>  > correct bandwidth allocation.
>
> I'm sure that the general case.
>
>  > In our case, the Linux PC does not have plugs that could be accessed
>  > by anybody else.
>
> I'm sure that host system (Linux PC) doesn't have no plugs.
>
>  >  (And I'm not sure if the value set by the Fireworks
>  > firmware is correct enough so that we'd want to use it.)
>
> With my Fireworks device, it always report 0, it means 1024 bytes for
> max payload size for output plug. But I'm not sure that it always report
> the same value. Especially at 192.0 kHz, the payload size is over 1024
> bytes but my device supports up to 96.0 kHz...
>
> Actually the target device seems to update the max payload size after
> host connects to CMP output plug because its initial value is zero. Then
> we already allocate isochronous resources and isochronous packet to host
> system. I don't think it better to reallocate them after the connecting.
>
> It's reasonable to ignore the payload field in any processing.
>
>
> Regards
>
> Takashi Sakamoto
>
> (Apr 29 2013 15:30), Clemens Ladisch wrote:
>> Takashi Sakamoto wrote:
>>> Then I have a question about handling payload field. In specification,
>>> "The payload field shall specify the maximum number of quadlets that
>>> may be transmitted in a single isochronous packet for this plug" but
>>> actually cheking this field seems to make no sense because there is no
>>> checking process in the procedure. I think I can do nothing for
>>> payload field.
>>
>> The specification was written for the general case where the device that
>> creates the connection might be different from the transmitting and
>> receiving devices.  In that case, the payload size is needed for correct
>> bandwidth allocation.
>>
>> In our case, the Linux PC does not have plugs that could be accessed by
>> anybody else.  (And I'm not sure if the value set by the Fireworks
>> firmware is correct enough so that we'd want to use it.)
>>
>>
>> Regards,
>> Clemens