fsi/gpio: Include command build in locked section

PATCH In review Benjamin Herrenschmidt 2 participants 14 messages May 24, 2018

Subsystems (3)

Tracing Hardware monitoring FSI

lore

Share & tools

lore

Benjamin Herrenschmidt

author

2018-05-24 05:14 UTC #0 lore raw

PATCH 1/10 fsi/gpio: Include command build in locked section

From: Jeremy Kerr <[email protected]>

For implementing relative addressing mode, we'll need to build a command that is coherent with CFAM state. To do that, include the build_command_* functions in the locked section of read/write/term.

Signed-off-by Jeremy Kerr <[email protected]>

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/fsi/fsi-master-gpio.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

drivers/fsi/fsi-master-gpio.c +18 -7

index e1bde9e3f855..d50ea4849377 100644
@@ -463,14 +463,10 @@ static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
 {
 	int rc;
 
-	mutex_lock(&master->cmd_lock);
-
 	rc = send_request(master, cmd);
 	if (!rc)
 		rc = poll_for_response(master, slave, resp_len, resp);
 
-	mutex_unlock(&master->cmd_lock);
-
 	return rc;
 }

@@ -479,12 +475,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
 {
 	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
 	struct fsi_gpio_msg cmd;
+	int rc;
 
 	if (link != 0)
 		return -ENODEV;
 
+	mutex_lock(&master->cmd_lock);
 	build_abs_ar_command(&cmd, id, addr, size, NULL);
-	return fsi_master_gpio_xfer(master, id, &cmd, size, val);
+	rc = fsi_master_gpio_xfer(master, id, &cmd, size, val);
+	mutex_unlock(&master->cmd_lock);
+
+	return rc;
 }
 
 static int fsi_master_gpio_write(struct fsi_master *_master, int link,

@@ -492,12 +493,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
 {
 	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
 	struct fsi_gpio_msg cmd;
+	int rc;
 
 	if (link != 0)
 		return -ENODEV;
 
+	mutex_lock(&master->cmd_lock);
 	build_abs_ar_command(&cmd, id, addr, size, val);
-	return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	mutex_unlock(&master->cmd_lock);
+
+	return rc;
 }
 
 static int fsi_master_gpio_term(struct fsi_master *_master,

@@ -505,12 +511,17 @@ static int fsi_master_gpio_term(struct fsi_master *_master,
 {
 	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
 	struct fsi_gpio_msg cmd;
+	int rc;
 
 	if (link != 0)
 		return -ENODEV;
 
+	mutex_lock(&master->cmd_lock);
 	build_term_command(&cmd, id);
-	return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	mutex_unlock(&master->cmd_lock);
+
+	return rc;
 }
 
 static int fsi_master_gpio_break(struct fsi_master *_master, int link)

Signature · 1 line

2.17.0

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #1 lore raw

PATCH 2/10 fsi/gpio: Use relative-addressing commands

From: Jeremy Kerr <[email protected]>

FSI CFAMs support shorter commands that use a relative (or same) address as the last. This change introduces a last_addr to the master state, and uses it for subsequent reads/writes, and performs relative addressing when a subsequent read/write is in range.

Signed-off-by Jeremy Kerr <[email protected]>

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/fsi/fsi-master-gpio.c | 102 ++++++++++++++++++++++++++++++----
 1 file changed, 91 insertions(+), 11 deletions(-)

drivers/fsi/fsi-master-gpio.c +91 ~~-11~~

index d50ea4849377..0a6799bda294 100644
@@ -28,6 +28,8 @@
 #define	FSI_GPIO_CMD_DPOLL      0x2
 #define	FSI_GPIO_CMD_TERM	0x3f
 #define FSI_GPIO_CMD_ABS_AR	0x4
+#define FSI_GPIO_CMD_REL_AR	0x5
+#define FSI_GPIO_CMD_SAME_AR	0x3	/* but only a 2-bit opcode... */
 
 
 #define	FSI_GPIO_DPOLL_CLOCKS	50      /* < 21 will cause slave to hang */

@@ -52,6 +54,8 @@
 #define	FSI_GPIO_MSG_RESPID_SIZE	2
 #define	FSI_GPIO_PRIME_SLAVE_CLOCKS	20
 
+#define LAST_ADDR_INVALID		0x1
+
 struct fsi_master_gpio {
 	struct fsi_master	master;
 	struct device		*dev;

@@ -64,6 +68,7 @@ struct fsi_master_gpio {
 	struct gpio_desc	*gpio_mux;	/* Mux control */
 	bool			external_mode;
 	bool			no_delays;
+	uint32_t		last_addr;
 };
 
 #define CREATE_TRACE_POINTS

@@ -205,22 +210,89 @@ static void msg_push_crc(struct fsi_gpio_msg *msg)
 	msg_push_bits(msg, crc, 4);
 }
 
+static bool check_same_address(struct fsi_master_gpio *master, int id,
+		uint32_t addr)
+{
+	/* this will also handle LAST_ADDR_INVALID */
+	return master->last_addr == (((id & 0x3) << 21) | (addr & ~0x3));
+}
+
+static bool check_relative_address(struct fsi_master_gpio *master, int id,
+		uint32_t addr, uint32_t *rel_addrp)
+{
+	uint32_t last_addr = master->last_addr;
+	int32_t rel_addr;
+
+	if (last_addr == LAST_ADDR_INVALID)
+		return false;
+
+	/* We may be in 23-bit addressing mode, which uses the id as the
+	 * top two address bits. So, if we're referencing a different ID,
+	 * use absolute addresses.
+	 */
+	if (((last_addr >> 21) & 0x3) != id)
+		return false;
+
+	/* remove the top two bits from any 23-bit addressing */
+	last_addr &= (1 << 21) - 1;
+
+	/* We know that the addresses are limited to 21 bits, so this won't
+	 * overflow the signed rel_addr */
+	rel_addr = addr - last_addr;
+	if (rel_addr > 255 || rel_addr < -256)
+		return false;
+
+	*rel_addrp = (uint32_t)rel_addr;
+
+	return true;
+}
+
+static void last_address_update(struct fsi_master_gpio *master,
+		int id, bool valid, uint32_t addr)
+{
+	if (!valid)
+		master->last_addr = LAST_ADDR_INVALID;
+	else
+		master->last_addr = ((id & 0x3) << 21) | (addr & ~0x3);
+}
+
 /*
- * Encode an Absolute Address command
+ * Encode an Absolute/Relative/Same Address command
  */
-static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
-		uint8_t id, uint32_t addr, size_t size, const void *data)
+static void build_ar_command(struct fsi_master_gpio *master,
+		struct fsi_gpio_msg *cmd, uint8_t id,
+		uint32_t addr, size_t size, const void *data)
 {
+	int i, addr_bits, opcode_bits;
 	bool write = !!data;
-	uint8_t ds;
-	int i;
+	uint8_t ds, opcode;
+	uint32_t rel_addr;
 
 	cmd->bits = 0;
 	cmd->msg = 0;
 
-	msg_push_bits(cmd, id, 2);
-	msg_push_bits(cmd, FSI_GPIO_CMD_ABS_AR, 3);
-	msg_push_bits(cmd, write ? 0 : 1, 1);
+	/* we have 21 bits of address max */
+	addr &= ((1 << 21) - 1);
+
+	/* cmd opcodes are variable length - SAME_AR is only two bits */
+	opcode_bits = 3;
+
+	if (check_same_address(master, id, addr)) {
+		/* we still address the byte offset within the word */
+		addr_bits = 2;
+		opcode_bits = 2;
+		opcode = FSI_GPIO_CMD_SAME_AR;
+
+	} else if (check_relative_address(master, id, addr, &rel_addr)) {
+		/* 8 bits plus sign */
+		addr_bits = 9;
+		addr = rel_addr;
+		opcode = FSI_GPIO_CMD_REL_AR;
+
+	} else {
+		addr_bits = 21;
+		opcode = FSI_GPIO_CMD_ABS_AR;
+	}
 
 	/*
 	 * The read/write size is encoded in the lower bits of the address

@@ -237,7 +309,10 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
 	if (size == 4)
 		addr |= 1;
 
-	msg_push_bits(cmd, addr & ((1 << 21) - 1), 21);
+	msg_push_bits(cmd, id, 2);
+	msg_push_bits(cmd, opcode, opcode_bits);
+	msg_push_bits(cmd, write ? 0 : 1, 1);
+	msg_push_bits(cmd, addr, addr_bits);
 	msg_push_bits(cmd, ds, 1);
 	for (i = 0; write && i < size; i++)
 		msg_push_bits(cmd, ((uint8_t *)data)[i], 8);

@@ -481,8 +556,9 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
 		return -ENODEV;
 
 	mutex_lock(&master->cmd_lock);
-	build_abs_ar_command(&cmd, id, addr, size, NULL);
+	build_ar_command(master, &cmd, id, addr, size, NULL);
 	rc = fsi_master_gpio_xfer(master, id, &cmd, size, val);
+	last_address_update(master, id, rc == 0, addr);
 	mutex_unlock(&master->cmd_lock);
 
 	return rc;

@@ -499,8 +575,9 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
 		return -ENODEV;
 
 	mutex_lock(&master->cmd_lock);
-	build_abs_ar_command(&cmd, id, addr, size, val);
+	build_ar_command(master, &cmd, id, addr, size, val);
 	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	last_address_update(master, id, rc == 0, addr);
 	mutex_unlock(&master->cmd_lock);
 
 	return rc;

@@ -519,6 +596,7 @@ static int fsi_master_gpio_term(struct fsi_master *_master,
 	mutex_lock(&master->cmd_lock);
 	build_term_command(&cmd, id);
 	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	last_address_update(master, id, false, 0);
 	mutex_unlock(&master->cmd_lock);
 
 	return rc;

@@ -552,6 +630,7 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
 	clock_toggle(master, FSI_POST_BREAK_CLOCKS);
 
 	spin_unlock_irqrestore(&master->bit_lock, flags);
+	last_address_update(master, 0, false, 0);
 	mutex_unlock(&master->cmd_lock);
 
 	/* Wait for logic reset to take effect */

@@ -662,6 +741,7 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 	master->dev = &pdev->dev;
 	master->master.dev.parent = master->dev;
 	master->master.dev.of_node = of_node_get(dev_of_node(master->dev));
+	master->last_addr = LAST_ADDR_INVALID;
 
 	gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
 	if (IS_ERR(gpio)) {

Signature · 1 line

2.17.0

Christopher Bostic

Reviewed-by on v1 2/10

2018-05-24 14:34 UTC #2 lore raw

Re: [PATCH linux dev-4.13 02/10] fsi/gpio: Use relative-addressing commands

Reviewed-by Christopher Bostic <[email protected]>

Quoted 199 lines · Benjamin Herrenschmidt

From: Jeremy Kerr <[email protected]>

Signed-off-by Jeremy Kerr <[email protected]>

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
  drivers/fsi/fsi-master-gpio.c | 102 ++++++++++++++++++++++++++++++----
  1 file changed, 91 insertions(+), 11 deletions(-)

drivers/fsi/fsi-master-gpio.c +91 ~~-11~~

index d50ea4849377..0a6799bda294 100644
@@ -28,6 +28,8 @@
  #define	FSI_GPIO_CMD_DPOLL      0x2
  #define	FSI_GPIO_CMD_TERM	0x3f
  #define FSI_GPIO_CMD_ABS_AR	0x4
+#define FSI_GPIO_CMD_REL_AR	0x5
+#define FSI_GPIO_CMD_SAME_AR	0x3	/* but only a 2-bit opcode... */
 
 
  #define	FSI_GPIO_DPOLL_CLOCKS	50      /* < 21 will cause slave to hang */

@@ -52,6 +54,8 @@
  #define	FSI_GPIO_MSG_RESPID_SIZE	2
  #define	FSI_GPIO_PRIME_SLAVE_CLOCKS	20
 
+#define LAST_ADDR_INVALID		0x1
+
  struct fsi_master_gpio {
  	struct fsi_master	master;
  	struct device		*dev;

@@ -64,6 +68,7 @@ struct fsi_master_gpio {
  	struct gpio_desc	*gpio_mux;	/* Mux control */
  	bool			external_mode;
  	bool			no_delays;
+	uint32_t		last_addr;
  };
 
  #define CREATE_TRACE_POINTS

@@ -205,22 +210,89 @@ static void msg_push_crc(struct fsi_gpio_msg *msg)
  	msg_push_bits(msg, crc, 4);
  }
 
+static bool check_same_address(struct fsi_master_gpio *master, int id,
+		uint32_t addr)
+{
+	/* this will also handle LAST_ADDR_INVALID */
+	return master->last_addr == (((id & 0x3) << 21) | (addr & ~0x3));
+}
+
+static bool check_relative_address(struct fsi_master_gpio *master, int id,
+		uint32_t addr, uint32_t *rel_addrp)
+{
+	uint32_t last_addr = master->last_addr;
+	int32_t rel_addr;
+
+	if (last_addr == LAST_ADDR_INVALID)
+		return false;
+
+	/* We may be in 23-bit addressing mode, which uses the id as the
+	 * top two address bits. So, if we're referencing a different ID,
+	 * use absolute addresses.
+	 */
+	if (((last_addr >> 21) & 0x3) != id)
+		return false;
+
+	/* remove the top two bits from any 23-bit addressing */
+	last_addr &= (1 << 21) - 1;
+
+	/* We know that the addresses are limited to 21 bits, so this won't
+	 * overflow the signed rel_addr */
+	rel_addr = addr - last_addr;
+	if (rel_addr > 255 || rel_addr < -256)
+		return false;
+
+	*rel_addrp = (uint32_t)rel_addr;
+
+	return true;
+}
+
+static void last_address_update(struct fsi_master_gpio *master,
+		int id, bool valid, uint32_t addr)
+{
+	if (!valid)
+		master->last_addr = LAST_ADDR_INVALID;
+	else
+		master->last_addr = ((id & 0x3) << 21) | (addr & ~0x3);
+}
+
  /*
- * Encode an Absolute Address command
+ * Encode an Absolute/Relative/Same Address command
   */
-static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
-		uint8_t id, uint32_t addr, size_t size, const void *data)
+static void build_ar_command(struct fsi_master_gpio *master,
+		struct fsi_gpio_msg *cmd, uint8_t id,
+		uint32_t addr, size_t size, const void *data)
  {
+	int i, addr_bits, opcode_bits;
  	bool write = !!data;
-	uint8_t ds;
-	int i;
+	uint8_t ds, opcode;
+	uint32_t rel_addr;
 
  	cmd->bits = 0;
  	cmd->msg = 0;
 
-	msg_push_bits(cmd, id, 2);
-	msg_push_bits(cmd, FSI_GPIO_CMD_ABS_AR, 3);
-	msg_push_bits(cmd, write ? 0 : 1, 1);
+	/* we have 21 bits of address max */
+	addr &= ((1 << 21) - 1);
+
+	/* cmd opcodes are variable length - SAME_AR is only two bits */
+	opcode_bits = 3;
+
+	if (check_same_address(master, id, addr)) {
+		/* we still address the byte offset within the word */
+		addr_bits = 2;
+		opcode_bits = 2;
+		opcode = FSI_GPIO_CMD_SAME_AR;
+
+	} else if (check_relative_address(master, id, addr, &rel_addr)) {
+		/* 8 bits plus sign */
+		addr_bits = 9;
+		addr = rel_addr;
+		opcode = FSI_GPIO_CMD_REL_AR;
+
+	} else {
+		addr_bits = 21;
+		opcode = FSI_GPIO_CMD_ABS_AR;
+	}
 
  	/*
  	 * The read/write size is encoded in the lower bits of the address

@@ -237,7 +309,10 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
  	if (size == 4)
  		addr |= 1;
 
-	msg_push_bits(cmd, addr & ((1 << 21) - 1), 21);
+	msg_push_bits(cmd, id, 2);
+	msg_push_bits(cmd, opcode, opcode_bits);
+	msg_push_bits(cmd, write ? 0 : 1, 1);
+	msg_push_bits(cmd, addr, addr_bits);
  	msg_push_bits(cmd, ds, 1);
  	for (i = 0; write && i < size; i++)
  		msg_push_bits(cmd, ((uint8_t *)data)[i], 8);

@@ -481,8 +556,9 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
  		return -ENODEV;
 
  	mutex_lock(&master->cmd_lock);
-	build_abs_ar_command(&cmd, id, addr, size, NULL);
+	build_ar_command(master, &cmd, id, addr, size, NULL);
  	rc = fsi_master_gpio_xfer(master, id, &cmd, size, val);
+	last_address_update(master, id, rc == 0, addr);
  	mutex_unlock(&master->cmd_lock);
 
  	return rc;

@@ -499,8 +575,9 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
  		return -ENODEV;
 
  	mutex_lock(&master->cmd_lock);
-	build_abs_ar_command(&cmd, id, addr, size, val);
+	build_ar_command(master, &cmd, id, addr, size, val);
  	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	last_address_update(master, id, rc == 0, addr);
  	mutex_unlock(&master->cmd_lock);
 
  	return rc;

@@ -519,6 +596,7 @@ static int fsi_master_gpio_term(struct fsi_master *_master,
  	mutex_lock(&master->cmd_lock);
  	build_term_command(&cmd, id);
  	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	last_address_update(master, id, false, 0);
  	mutex_unlock(&master->cmd_lock);
 
  	return rc;

@@ -552,6 +630,7 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
  	clock_toggle(master, FSI_POST_BREAK_CLOCKS);
 
  	spin_unlock_irqrestore(&master->bit_lock, flags);
+	last_address_update(master, 0, false, 0);
  	mutex_unlock(&master->cmd_lock);
 
  	/* Wait for logic reset to take effect */

@@ -662,6 +741,7 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
  	master->dev = &pdev->dev;
  	master->master.dev.parent = master->dev;
  	master->master.dev.of_node = of_node_get(dev_of_node(master->dev));
+	master->last_addr = LAST_ADDR_INVALID;
 
  	gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
  	if (IS_ERR(gpio)) {

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #3 lore raw

PATCH 3/10 fsi/fsi-master-gpio: Implement CRC error recovery

In reply to Benjamin Herrenschmidt

The FSI protocol defines two modes of recovery from CRC errors, this implements both:

- If the device returns an ECRC (it detected a CRC error in the command), then we simply issue the command again.

- If the master detects a CRC error in the response, we send an E_POLL command which requests a resend of the response without actually re-executing the command (which could otherwise have unwanted side effects such as dequeuing a FIFO twice).

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

Note: This was actually tested by removing some of my fixes, thus causing us to hit occasional CRC errors during high LPC activity.

---
 drivers/fsi/fsi-master-gpio.c          | 90 ++++++++++++++++++++------
 include/trace/events/fsi_master_gpio.h | 27 ++++++++
 2 files changed, 99 insertions(+), 18 deletions(-)

drivers/fsi/fsi-master-gpio.c +72 ~~-18~~

index 0a6799bda294..351c12f2ac55 100644
@@ -22,20 +22,23 @@
 #define	FSI_BREAK_CLOCKS	256	/* Number of clocks to issue break */
 #define	FSI_POST_BREAK_CLOCKS	16000	/* Number clocks to set up cfam */
 #define	FSI_INIT_CLOCKS		5000	/* Clock out any old data */
+#define	FSI_GPIO_DPOLL_CLOCKS	50      /* < 21 will cause slave to hang */
+#define	FSI_GPIO_EPOLL_CLOCKS	50      /* Number of clocks for E_POLL retry */
 #define	FSI_GPIO_STD_DELAY	10	/* Standard GPIO delay in nS */
 					/* todo: adjust down as low as */
 					/* possible or eliminate */
+#define FSI_CRC_ERR_RETRIES	10
+
 #define	FSI_GPIO_CMD_DPOLL      0x2
+#define	FSI_GPIO_CMD_EPOLL      0x3
 #define	FSI_GPIO_CMD_TERM	0x3f
 #define FSI_GPIO_CMD_ABS_AR	0x4
 #define FSI_GPIO_CMD_REL_AR	0x5
 #define FSI_GPIO_CMD_SAME_AR	0x3	/* but only a 2-bit opcode... */
 
-
-#define	FSI_GPIO_DPOLL_CLOCKS	50      /* < 21 will cause slave to hang */
-
-/* Bus errors */
-#define	FSI_GPIO_ERR_BUSY	1	/* Slave stuck in busy state */
+/* Slave responses */
+#define	FSI_GPIO_RESP_ACK	0	/* Success */
+#define	FSI_GPIO_RESP_BUSY	1	/* Slave busy */
 #define	FSI_GPIO_RESP_ERRA	2	/* Any (misc) Error */
 #define	FSI_GPIO_RESP_ERRC	3	/* Slave reports master CRC error */
 #define	FSI_GPIO_MTOE		4	/* Master time out error */

@@ -330,6 +333,16 @@ static void build_dpoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
 	msg_push_crc(cmd);
 }
 
+static void build_epoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
+{
+	cmd->bits = 0;
+	cmd->msg = 0;
+
+	msg_push_bits(cmd, slave_id, 2);
+	msg_push_bits(cmd, FSI_GPIO_CMD_EPOLL, 3);
+	msg_push_crc(cmd);
+}
+
 static void echo_delay(struct fsi_master_gpio *master)
 {
 	set_sda_output(master, 1);

@@ -355,6 +368,12 @@ static void fsi_master_gpio_error(struct fsi_master_gpio *master, int error)
 
 }
 
+/*
+ * Note: callers rely specifically on this returning -EAGAIN for
+ * a CRC error detected in the response. Use other error code
+ * for other situations. It will be converted to something else
+ * higher up the stack before it reaches userspace.
+ */
 static int read_one_response(struct fsi_master_gpio *master,
 		uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
 {

@@ -379,7 +398,7 @@ static int read_one_response(struct fsi_master_gpio *master,
 			"Master time out waiting for response\n");
 		fsi_master_gpio_error(master, FSI_GPIO_MTOE);
 		spin_unlock_irqrestore(&master->bit_lock, flags);
-		return -EIO;
+		return -ETIMEDOUT;
 	}
 
 	msg.bits = 0;

@@ -405,7 +424,7 @@ static int read_one_response(struct fsi_master_gpio *master,
 	if (crc) {
 		dev_dbg(master->dev, "ERR response CRC\n");
 		fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
-		return -EIO;
+		return -EAGAIN;
 	}
 
 	if (msgp)

@@ -451,11 +470,33 @@ static int poll_for_response(struct fsi_master_gpio *master,
 	unsigned long flags;
 	uint8_t tag;
 	uint8_t *data_byte = data;
-
+	int crc_err_retries = 0;
 retry:
 	rc = read_one_response(master, size, &response, &tag);
-	if (rc)
-		return rc;
+
+	/* Handle retries on CRC errors */
+	if (rc == -EAGAIN) {
+		/* Too many retries ? */
+		if (crc_err_retries++ > FSI_CRC_ERR_RETRIES) {
+			/*
+			 * Pass it up as a -EIO otherwise upper level will retry
+			 * the whole command which isn't what we want here.
+			 */
+			rc = -EIO;
+			goto fail;
+		}
+		dev_dbg(master->dev,
+			 "CRC error retry %d\n", crc_err_retries);
+		trace_fsi_master_gpio_crc_rsp_error(master);
+		build_epoll_command(&cmd, slave);
+		spin_lock_irqsave(&master->bit_lock, flags);
+		clock_zeros(master, FSI_GPIO_EPOLL_CLOCKS);
+		serial_out(master, &cmd);
+		echo_delay(master);
+		spin_unlock_irqrestore(&master->bit_lock, flags);
+		goto retry;
+	} else if (rc)
+		goto fail;
 
 	switch (tag) {
 	case FSI_GPIO_RESP_ACK:

@@ -496,18 +537,21 @@ static int poll_for_response(struct fsi_master_gpio *master,
 		break;
 
 	case FSI_GPIO_RESP_ERRA:
-	case FSI_GPIO_RESP_ERRC:
-		dev_dbg(master->dev, "ERR%c received: 0x%x\n",
-			tag == FSI_GPIO_RESP_ERRA ? 'A' : 'C',
-			(int)response.msg);
+		dev_dbg(master->dev, "ERRA received: 0x%x\n", (int)response.msg);
 		fsi_master_gpio_error(master, response.msg);
 		rc = -EIO;
 		break;
+	case FSI_GPIO_RESP_ERRC:
+		dev_dbg(master->dev, "ERRC received: 0x%x\n", (int)response.msg);
+		fsi_master_gpio_error(master, response.msg);
+		trace_fsi_master_gpio_crc_cmd_error(master);
+		rc = -EAGAIN;
+		break;
 	}
 
 	if (busy_count > 0)
 		trace_fsi_master_gpio_poll_response_busy(master, busy_count);
-
+ fail:
 	/* Clock the slave enough to be ready for next operation */
 	spin_lock_irqsave(&master->bit_lock, flags);
 	clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);

@@ -536,11 +580,21 @@ static int send_request(struct fsi_master_gpio *master,
 static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
 		struct fsi_gpio_msg *cmd, size_t resp_len, void *resp)
 {
-	int rc;
+	int rc = -EAGAIN, retries = 0;
 
-	rc = send_request(master, cmd);
-	if (!rc)
+	while ((retries++) < FSI_CRC_ERR_RETRIES) {
+		rc = send_request(master, cmd);
+		if (rc)
+			break;
 		rc = poll_for_response(master, slave, resp_len, resp);
+		if (rc != -EAGAIN)
+			break;
+		rc = -EIO;
+		dev_warn(master->dev, "ECRC retry %d\n", retries);
+
+		/* Pace it a bit before retry */
+		msleep(1);
+	}
 
 	return rc;
 }

include/trace/events/fsi_master_gpio.h +27 -0

index 48e83e5755f4..e18b94ce85b2 100644
@@ -63,6 +63,33 @@ TRACE_EVENT(fsi_master_gpio_break,
 	)
 );
 
+TRACE_EVENT(fsi_master_gpio_crc_cmd_error,
+	TP_PROTO(const struct fsi_master_gpio *master),
+	TP_ARGS(master),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+	),
+	TP_printk("fsi-gpio%d ----CRC command retry---",
+		__entry->master_idx
+	)
+);
+
+TRACE_EVENT(fsi_master_gpio_crc_rsp_error,
+	TP_PROTO(const struct fsi_master_gpio *master),
+	TP_ARGS(master),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+	),
+	TP_printk("fsi-gpio%d ----CRC response---",
+		__entry->master_idx
+	)
+);
 
 TRACE_EVENT(fsi_master_gpio_poll_response_busy,
 	TP_PROTO(const struct fsi_master_gpio *master, int busy),

Signature · 1 line

2.17.0

Christopher Bostic

Reviewed-by on v1 3/10

2018-05-24 15:05 UTC #4 lore raw

Re: [PATCH linux dev-4.13 03/10] fsi/fsi-master-gpio: Implement CRC error recovery

Reviewed-by Christopher Bostic <[email protected]>

Quoted 229 lines · Benjamin Herrenschmidt

The FSI protocol defines two modes of recovery from CRC errors, this implements both:

- If the device returns an ECRC (it detected a CRC error in the command), then we simply issue the command again.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

Note: This was actually tested by removing some of my fixes, thus causing us to hit occasional CRC errors during high LPC activity.

---
  drivers/fsi/fsi-master-gpio.c          | 90 ++++++++++++++++++++------
  include/trace/events/fsi_master_gpio.h | 27 ++++++++
  2 files changed, 99 insertions(+), 18 deletions(-)

drivers/fsi/fsi-master-gpio.c +72 ~~-18~~

index 0a6799bda294..351c12f2ac55 100644
@@ -22,20 +22,23 @@
  #define	FSI_BREAK_CLOCKS	256	/* Number of clocks to issue break */
  #define	FSI_POST_BREAK_CLOCKS	16000	/* Number clocks to set up cfam */
  #define	FSI_INIT_CLOCKS		5000	/* Clock out any old data */
+#define	FSI_GPIO_DPOLL_CLOCKS	50      /* < 21 will cause slave to hang */
+#define	FSI_GPIO_EPOLL_CLOCKS	50      /* Number of clocks for E_POLL retry */
  #define	FSI_GPIO_STD_DELAY	10	/* Standard GPIO delay in nS */
  					/* todo: adjust down as low as */
  					/* possible or eliminate */
+#define FSI_CRC_ERR_RETRIES	10
+
  #define	FSI_GPIO_CMD_DPOLL      0x2
+#define	FSI_GPIO_CMD_EPOLL      0x3
  #define	FSI_GPIO_CMD_TERM	0x3f
  #define FSI_GPIO_CMD_ABS_AR	0x4
  #define FSI_GPIO_CMD_REL_AR	0x5
  #define FSI_GPIO_CMD_SAME_AR	0x3	/* but only a 2-bit opcode... */
 
-
-#define	FSI_GPIO_DPOLL_CLOCKS	50      /* < 21 will cause slave to hang */
-
-/* Bus errors */
-#define	FSI_GPIO_ERR_BUSY	1	/* Slave stuck in busy state */
+/* Slave responses */
+#define	FSI_GPIO_RESP_ACK	0	/* Success */
+#define	FSI_GPIO_RESP_BUSY	1	/* Slave busy */
  #define	FSI_GPIO_RESP_ERRA	2	/* Any (misc) Error */
  #define	FSI_GPIO_RESP_ERRC	3	/* Slave reports master CRC error */
  #define	FSI_GPIO_MTOE		4	/* Master time out error */

@@ -330,6 +333,16 @@ static void build_dpoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
  	msg_push_crc(cmd);
  }
 
+static void build_epoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
+{
+	cmd->bits = 0;
+	cmd->msg = 0;
+
+	msg_push_bits(cmd, slave_id, 2);
+	msg_push_bits(cmd, FSI_GPIO_CMD_EPOLL, 3);
+	msg_push_crc(cmd);
+}
+
  static void echo_delay(struct fsi_master_gpio *master)
  {
  	set_sda_output(master, 1);

@@ -355,6 +368,12 @@ static void fsi_master_gpio_error(struct fsi_master_gpio *master, int error)
 
  }
 
+/*
+ * Note: callers rely specifically on this returning -EAGAIN for
+ * a CRC error detected in the response. Use other error code
+ * for other situations. It will be converted to something else
+ * higher up the stack before it reaches userspace.
+ */
  static int read_one_response(struct fsi_master_gpio *master,
  		uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
  {

@@ -379,7 +398,7 @@ static int read_one_response(struct fsi_master_gpio *master,
  			"Master time out waiting for response\n");
  		fsi_master_gpio_error(master, FSI_GPIO_MTOE);
  		spin_unlock_irqrestore(&master->bit_lock, flags);
-		return -EIO;
+		return -ETIMEDOUT;
  	}
 
  	msg.bits = 0;

@@ -405,7 +424,7 @@ static int read_one_response(struct fsi_master_gpio *master,
  	if (crc) {
  		dev_dbg(master->dev, "ERR response CRC\n");
  		fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
-		return -EIO;
+		return -EAGAIN;
  	}
 
  	if (msgp)

@@ -451,11 +470,33 @@ static int poll_for_response(struct fsi_master_gpio *master,
  	unsigned long flags;
  	uint8_t tag;
  	uint8_t *data_byte = data;
-
+	int crc_err_retries = 0;
  retry:
  	rc = read_one_response(master, size, &response, &tag);
-	if (rc)
-		return rc;
+
+	/* Handle retries on CRC errors */
+	if (rc == -EAGAIN) {
+		/* Too many retries ? */
+		if (crc_err_retries++ > FSI_CRC_ERR_RETRIES) {
+			/*
+			 * Pass it up as a -EIO otherwise upper level will retry
+			 * the whole command which isn't what we want here.
+			 */
+			rc = -EIO;
+			goto fail;
+		}
+		dev_dbg(master->dev,
+			 "CRC error retry %d\n", crc_err_retries);
+		trace_fsi_master_gpio_crc_rsp_error(master);
+		build_epoll_command(&cmd, slave);
+		spin_lock_irqsave(&master->bit_lock, flags);
+		clock_zeros(master, FSI_GPIO_EPOLL_CLOCKS);
+		serial_out(master, &cmd);
+		echo_delay(master);
+		spin_unlock_irqrestore(&master->bit_lock, flags);
+		goto retry;
+	} else if (rc)
+		goto fail;
 
  	switch (tag) {
  	case FSI_GPIO_RESP_ACK:

@@ -496,18 +537,21 @@ static int poll_for_response(struct fsi_master_gpio *master,
  		break;
 
  	case FSI_GPIO_RESP_ERRA:
-	case FSI_GPIO_RESP_ERRC:
-		dev_dbg(master->dev, "ERR%c received: 0x%x\n",
-			tag == FSI_GPIO_RESP_ERRA ? 'A' : 'C',
-			(int)response.msg);
+		dev_dbg(master->dev, "ERRA received: 0x%x\n", (int)response.msg);
  		fsi_master_gpio_error(master, response.msg);
  		rc = -EIO;
  		break;
+	case FSI_GPIO_RESP_ERRC:
+		dev_dbg(master->dev, "ERRC received: 0x%x\n", (int)response.msg);
+		fsi_master_gpio_error(master, response.msg);
+		trace_fsi_master_gpio_crc_cmd_error(master);
+		rc = -EAGAIN;
+		break;
  	}
 
  	if (busy_count > 0)
  		trace_fsi_master_gpio_poll_response_busy(master, busy_count);
-
+ fail:
  	/* Clock the slave enough to be ready for next operation */
  	spin_lock_irqsave(&master->bit_lock, flags);
  	clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);

@@ -536,11 +580,21 @@ static int send_request(struct fsi_master_gpio *master,
  static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
  		struct fsi_gpio_msg *cmd, size_t resp_len, void *resp)
  {
-	int rc;
+	int rc = -EAGAIN, retries = 0;
 
-	rc = send_request(master, cmd);
-	if (!rc)
+	while ((retries++) < FSI_CRC_ERR_RETRIES) {
+		rc = send_request(master, cmd);
+		if (rc)
+			break;
  		rc = poll_for_response(master, slave, resp_len, resp);
+		if (rc != -EAGAIN)
+			break;
+		rc = -EIO;
+		dev_warn(master->dev, "ECRC retry %d\n", retries);
+
+		/* Pace it a bit before retry */
+		msleep(1);
+	}
 
  	return rc;
  }

include/trace/events/fsi_master_gpio.h +27 -0

index 48e83e5755f4..e18b94ce85b2 100644
@@ -63,6 +63,33 @@ TRACE_EVENT(fsi_master_gpio_break,
  	)
  );
 
+TRACE_EVENT(fsi_master_gpio_crc_cmd_error,
+	TP_PROTO(const struct fsi_master_gpio *master),
+	TP_ARGS(master),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+	),
+	TP_printk("fsi-gpio%d ----CRC command retry---",
+		__entry->master_idx
+	)
+);
+
+TRACE_EVENT(fsi_master_gpio_crc_rsp_error,
+	TP_PROTO(const struct fsi_master_gpio *master),
+	TP_ARGS(master),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+	),
+	TP_printk("fsi-gpio%d ----CRC response---",
+		__entry->master_idx
+	)
+);
 
  TRACE_EVENT(fsi_master_gpio_poll_response_busy,
  	TP_PROTO(const struct fsi_master_gpio *master, int busy),

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #5 lore raw

PATCH 4/10 fsi/fsi-master-gpio: More error handling cleanup

In reply to Benjamin Herrenschmidt

Remove calls to the empty and useless fsi_master_gpio_error() function, and report CRC errors as "FSI_ERR_NO_SLAVE" when reading an all 1's response.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/fsi/fsi-master-gpio.c | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

drivers/fsi/fsi-master-gpio.c +5 ~~-21~~

index 351c12f2ac55..da556da62846 100644
@@ -41,13 +41,6 @@
 #define	FSI_GPIO_RESP_BUSY	1	/* Slave busy */
 #define	FSI_GPIO_RESP_ERRA	2	/* Any (misc) Error */
 #define	FSI_GPIO_RESP_ERRC	3	/* Slave reports master CRC error */
-#define	FSI_GPIO_MTOE		4	/* Master time out error */
-#define	FSI_GPIO_CRC_INVAL	5	/* Master reports slave CRC error */
-
-/* Normal slave responses */
-#define	FSI_GPIO_RESP_BUSY	1
-#define	FSI_GPIO_RESP_ACK	0
-#define	FSI_GPIO_RESP_ACKD	4
 
 #define	FSI_GPIO_MAX_BUSY	200
 #define	FSI_GPIO_MTOE_COUNT	1000

@@ -359,15 +352,6 @@ static void build_term_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
 	msg_push_crc(cmd);
 }
 
-/*
- * Store information on master errors so handler can detect and clean
- * up the bus
- */
-static void fsi_master_gpio_error(struct fsi_master_gpio *master, int error)
-{
-
-}
-
 /*
  * Note: callers rely specifically on this returning -EAGAIN for
  * a CRC error detected in the response. Use other error code

@@ -396,7 +380,6 @@ static int read_one_response(struct fsi_master_gpio *master,
 	if (i == FSI_GPIO_MTOE_COUNT) {
 		dev_dbg(master->dev,
 			"Master time out waiting for response\n");
-		fsi_master_gpio_error(master, FSI_GPIO_MTOE);
 		spin_unlock_irqrestore(&master->bit_lock, flags);
 		return -ETIMEDOUT;
 	}

@@ -422,8 +405,11 @@ static int read_one_response(struct fsi_master_gpio *master,
 	crc = crc4(0, 1, 1);
 	crc = crc4(crc, msg.msg, msg.bits);
 	if (crc) {
-		dev_dbg(master->dev, "ERR response CRC\n");
-		fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
+		/* Check if it's all 1's, that probably means the host is off */
+		if (((~msg.msg) & ((1ull << msg.bits) - 1)) == 0)
+			return -ENODEV;
+		dev_dbg(master->dev, "ERR response CRC msg: 0x%016llx (%d bits)\n",
+			msg.msg, msg.bits);
 		return -EAGAIN;
 	}

@@ -538,12 +524,10 @@ static int poll_for_response(struct fsi_master_gpio *master,
 
 	case FSI_GPIO_RESP_ERRA:
 		dev_dbg(master->dev, "ERRA received: 0x%x\n", (int)response.msg);
-		fsi_master_gpio_error(master, response.msg);
 		rc = -EIO;
 		break;
 	case FSI_GPIO_RESP_ERRC:
 		dev_dbg(master->dev, "ERRC received: 0x%x\n", (int)response.msg);
-		fsi_master_gpio_error(master, response.msg);
 		trace_fsi_master_gpio_crc_cmd_error(master);
 		rc = -EAGAIN;
 		break;

Signature · 1 line

2.17.0

Christopher Bostic

Reviewed-by on v1 4/10

2018-05-24 18:50 UTC #6 lore raw

Re: [PATCH linux dev-4.13 04/10] fsi/fsi-master-gpio: More error handling cleanup

Reviewed-by Christopher Bostic <[email protected]>

Quoted 78 lines · Benjamin Herrenschmidt

Remove calls to the empty and useless fsi_master_gpio_error() function, and report CRC errors as "FSI_ERR_NO_SLAVE" when reading an all 1's response.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
  drivers/fsi/fsi-master-gpio.c | 26 +++++---------------------
  1 file changed, 5 insertions(+), 21 deletions(-)

drivers/fsi/fsi-master-gpio.c +5 ~~-21~~

index 351c12f2ac55..da556da62846 100644
@@ -41,13 +41,6 @@
  #define	FSI_GPIO_RESP_BUSY	1	/* Slave busy */
  #define	FSI_GPIO_RESP_ERRA	2	/* Any (misc) Error */
  #define	FSI_GPIO_RESP_ERRC	3	/* Slave reports master CRC error */
-#define	FSI_GPIO_MTOE		4	/* Master time out error */
-#define	FSI_GPIO_CRC_INVAL	5	/* Master reports slave CRC error */
-
-/* Normal slave responses */
-#define	FSI_GPIO_RESP_BUSY	1
-#define	FSI_GPIO_RESP_ACK	0
-#define	FSI_GPIO_RESP_ACKD	4
 
  #define	FSI_GPIO_MAX_BUSY	200
  #define	FSI_GPIO_MTOE_COUNT	1000

@@ -359,15 +352,6 @@ static void build_term_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
  	msg_push_crc(cmd);
  }
 
-/*
- * Store information on master errors so handler can detect and clean
- * up the bus
- */
-static void fsi_master_gpio_error(struct fsi_master_gpio *master, int error)
-{
-
-}
-
  /*
   * Note: callers rely specifically on this returning -EAGAIN for
   * a CRC error detected in the response. Use other error code

@@ -396,7 +380,6 @@ static int read_one_response(struct fsi_master_gpio *master,
  	if (i == FSI_GPIO_MTOE_COUNT) {
  		dev_dbg(master->dev,
  			"Master time out waiting for response\n");
-		fsi_master_gpio_error(master, FSI_GPIO_MTOE);
  		spin_unlock_irqrestore(&master->bit_lock, flags);
  		return -ETIMEDOUT;
  	}

@@ -422,8 +405,11 @@ static int read_one_response(struct fsi_master_gpio *master,
  	crc = crc4(0, 1, 1);
  	crc = crc4(crc, msg.msg, msg.bits);
  	if (crc) {
-		dev_dbg(master->dev, "ERR response CRC\n");
-		fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
+		/* Check if it's all 1's, that probably means the host is off */
+		if (((~msg.msg) & ((1ull << msg.bits) - 1)) == 0)
+			return -ENODEV;
+		dev_dbg(master->dev, "ERR response CRC msg: 0x%016llx (%d bits)\n",
+			msg.msg, msg.bits);
  		return -EAGAIN;
  	}

@@ -538,12 +524,10 @@ static int poll_for_response(struct fsi_master_gpio *master,
 
  	case FSI_GPIO_RESP_ERRA:
  		dev_dbg(master->dev, "ERRA received: 0x%x\n", (int)response.msg);
-		fsi_master_gpio_error(master, response.msg);
  		rc = -EIO;
  		break;
  	case FSI_GPIO_RESP_ERRC:
  		dev_dbg(master->dev, "ERRC received: 0x%x\n", (int)response.msg);
-		fsi_master_gpio_error(master, response.msg);
  		trace_fsi_master_gpio_crc_cmd_error(master);
  		rc = -EAGAIN;
  		break;

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #7 lore raw

PATCH 5/10 fsi/master-gpio: Replace bit_bit lock with IRQ disable/enable

In reply to Benjamin Herrenschmidt

From: Jeremy Kerr <[email protected]>

We currently use a spinlock (bit_lock) around operations that clock bits out of the FSI bus, and a mutex to protect against simultaneous access to the master.

This means that bit_lock isn't needed for mutual exlusion, only to prevent timing issues when clocking bits out.

To reflect this, this change converts bit_lock to just the local_irq_save/restore operation.

Signed-off-by Jeremy Kerr <[email protected]>

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/fsi/fsi-master-gpio.c | 48 +++++++++++++++++------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

drivers/fsi/fsi-master-gpio.c +23 ~~-25~~

index da556da62846..084e9da8d151 100644
@@ -8,11 +8,11 @@
 #include <linux/fsi.h>
 #include <linux/gpio/consumer.h>
 #include <linux/io.h>
+#include <linux/irqflags.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
-#include <linux/spinlock.h>
 
 #include "fsi-master.h"

@@ -56,7 +56,6 @@ struct fsi_master_gpio {
 	struct fsi_master	master;
 	struct device		*dev;
 	struct mutex		cmd_lock;	/* mutex for command ordering */
-	spinlock_t		bit_lock;	/* lock for clocking bits out */
 	struct gpio_desc	*gpio_clk;
 	struct gpio_desc	*gpio_data;
 	struct gpio_desc	*gpio_trans;	/* Voltage translator */

@@ -367,7 +366,7 @@ static int read_one_response(struct fsi_master_gpio *master,
 	uint8_t tag;
 	int i;
 
-	spin_lock_irqsave(&master->bit_lock, flags);
+	local_irq_save(flags);
 
 	/* wait for the start bit */
 	for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) {

@@ -380,7 +379,7 @@ static int read_one_response(struct fsi_master_gpio *master,
 	if (i == FSI_GPIO_MTOE_COUNT) {
 		dev_dbg(master->dev,
 			"Master time out waiting for response\n");
-		spin_unlock_irqrestore(&master->bit_lock, flags);
+		local_irq_restore(flags);
 		return -ETIMEDOUT;
 	}

@@ -399,7 +398,7 @@ static int read_one_response(struct fsi_master_gpio *master,
 	/* read CRC */
 	serial_in(master, &msg, FSI_GPIO_CRC_SIZE);
 
-	spin_unlock_irqrestore(&master->bit_lock, flags);
+	local_irq_restore(flags);
 
 	/* we have a whole message now; check CRC */
 	crc = crc4(0, 1, 1);

@@ -430,10 +429,10 @@ static int issue_term(struct fsi_master_gpio *master, uint8_t slave)
 
 	build_term_command(&cmd, slave);
 
-	spin_lock_irqsave(&master->bit_lock, flags);
+	local_irq_save(flags);
 	serial_out(master, &cmd);
 	echo_delay(master);
-	spin_unlock_irqrestore(&master->bit_lock, flags);
+	local_irq_restore(flags);
 
 	rc = read_one_response(master, 0, NULL, &tag);
 	if (rc < 0) {

@@ -475,11 +474,11 @@ static int poll_for_response(struct fsi_master_gpio *master,
 			 "CRC error retry %d\n", crc_err_retries);
 		trace_fsi_master_gpio_crc_rsp_error(master);
 		build_epoll_command(&cmd, slave);
-		spin_lock_irqsave(&master->bit_lock, flags);
+		local_irq_save(flags);
 		clock_zeros(master, FSI_GPIO_EPOLL_CLOCKS);
 		serial_out(master, &cmd);
 		echo_delay(master);
-		spin_unlock_irqrestore(&master->bit_lock, flags);
+		local_irq_restore(flags);
 		goto retry;
 	} else if (rc)
 		goto fail;

@@ -506,18 +505,18 @@ static int poll_for_response(struct fsi_master_gpio *master,
 		 */
 		if (busy_count++ < FSI_GPIO_MAX_BUSY) {
 			build_dpoll_command(&cmd, slave);
-			spin_lock_irqsave(&master->bit_lock, flags);
+			local_irq_save(flags);
 			clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
 			serial_out(master, &cmd);
 			echo_delay(master);
-			spin_unlock_irqrestore(&master->bit_lock, flags);
+			local_irq_restore(flags);
 			goto retry;
 		}
 		dev_warn(master->dev,
 			"ERR slave is stuck in busy state, issuing TERM\n");
-		spin_lock_irqsave(&master->bit_lock, flags);
+		local_irq_save(flags);
 		clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
-		spin_unlock_irqrestore(&master->bit_lock, flags);
+		local_irq_restore(flags);
 		issue_term(master, slave);
 		rc = -EIO;
 		break;

@@ -537,9 +536,10 @@ static int poll_for_response(struct fsi_master_gpio *master,
 		trace_fsi_master_gpio_poll_response_busy(master, busy_count);
  fail:
 	/* Clock the slave enough to be ready for next operation */
-	spin_lock_irqsave(&master->bit_lock, flags);
+	local_irq_save(flags);
 	clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
-	spin_unlock_irqrestore(&master->bit_lock, flags);
+	local_irq_restore(flags);
+
 	return rc;
 }

@@ -548,15 +548,13 @@ static int send_request(struct fsi_master_gpio *master,
 {
 	unsigned long flags;
 
-	spin_lock_irqsave(&master->bit_lock, flags);
-	if (master->external_mode) {
-		spin_unlock_irqrestore(&master->bit_lock, flags);
+	if (master->external_mode)
 		return -EBUSY;
-	}
 
+	local_irq_save(flags);
 	serial_out(master, cmd);
 	echo_delay(master);
-	spin_unlock_irqrestore(&master->bit_lock, flags);
+	local_irq_restore(flags);
 
 	return 0;
 }

@@ -656,7 +654,7 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
 		return -EBUSY;
 	}
 
-	spin_lock_irqsave(&master->bit_lock, flags);
+	local_irq_save(flags);
 
 	set_sda_output(master, 1);
 	sda_out(master, 1);

@@ -667,7 +665,8 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
 	sda_out(master, 1);
 	clock_toggle(master, FSI_POST_BREAK_CLOCKS);
 
-	spin_unlock_irqrestore(&master->bit_lock, flags);
+	local_irq_restore(flags);
+
 	last_address_update(master, 0, false, 0);
 	mutex_unlock(&master->cmd_lock);

@@ -688,9 +687,9 @@ static void fsi_master_gpio_init(struct fsi_master_gpio *master)
 	gpiod_direction_output(master->gpio_data, 1);
 
 	/* todo: evaluate if clocks can be reduced */
-	spin_lock_irqsave(&master->bit_lock, flags);
+	local_irq_save(flags);
 	clock_zeros(master, FSI_INIT_CLOCKS);
-	spin_unlock_irqrestore(&master->bit_lock, flags);
+	local_irq_restore(flags);
 }
 
 static void fsi_master_gpio_init_external(struct fsi_master_gpio *master)

@@ -832,7 +831,6 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 	master->master.send_break = fsi_master_gpio_break;
 	master->master.link_enable = fsi_master_gpio_link_enable;
 	platform_set_drvdata(pdev, master);
-	spin_lock_init(&master->bit_lock);
 	mutex_init(&master->cmd_lock);
 
 	fsi_master_gpio_init(master);

Signature · 1 line

2.17.0

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #8 lore raw

PATCH 6/10 fsi: Remove old sbefifo driver

In reply to Benjamin Herrenschmidt

And remporarily disable build of fsi-occ

---
 drivers/fsi/Makefile        |    4 +-
 drivers/fsi/fsi-sbefifo.c   | 1071 -----------------------------------
 drivers/hwmon/occ/Makefile  |    2 +-
 include/linux/fsi-sbefifo.h |   30 -
 4 files changed, 3 insertions(+), 1104 deletions(-)
 delete mode 100644 drivers/fsi/fsi-sbefifo.c
 delete mode 100644 include/linux/fsi-sbefifo.h

drivers/fsi/Makefile +2 -2

index 75fdc6d8cfc4..61e8e420c7ed 100644
@@ -3,5 +3,5 @@ obj-$(CONFIG_FSI) += fsi-core.o
 obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
 obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
 obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
-obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
-obj-$(CONFIG_FSI_OCC) += fsi-occ.o
+#obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
+#obj-$(CONFIG_FSI_OCC) += fsi-occ.o

drivers/fsi/fsi-sbefifo.c deleted +0 ~~-1071~~

1,071 lines not on this page · raw message

drivers/hwmon/occ/Makefile +1 -1

index ab5c3e965ccb..ca6d25ae9da8 100644
@@ -1,7 +1,7 @@
 occ-hwmon-objs := common.o
 
 ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y)
-occ-hwmon-objs += p9_sbe.o
+#occ-hwmon-objs += p9_sbe.o
 endif
 
 ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y)

include/linux/fsi-sbefifo.h deleted +0 ~~-30~~

deleted file mode 100644
index 8e55891963a5..000000000000
@@ -1,30 +0,0 @@
-/*
- * SBEFIFO FSI Client device driver
- *
- * Copyright (C) IBM Corporation 2017
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef LINUX_FSI_SBEFIFO_H
-#define LINUX_FSI_SBEFIFO_H
-
-struct device;
-struct sbefifo_client;
-
-extern struct sbefifo_client *sbefifo_drv_open(struct device *dev,
-					       unsigned long flags);
-extern int sbefifo_drv_read(struct sbefifo_client *client, char *buf,
-			    size_t len);
-extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
-			     size_t len);
-extern void sbefifo_drv_release(struct sbefifo_client *client);
-
-#endif /* LINUX_FSI_SBEFIFO_H */

Signature · 1 line

2.17.0

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #9 lore raw

PATCH 7/10 fsi/sbefifo: Add driver for the SBE FIFO

In reply to Benjamin Herrenschmidt

This driver provides an in-kernel and a user API for accessing the command FIFO of the SBE (Self Boot Engine) of the POWER9 processor, via the FSI bus.

It provides an in-kernel interface to submit command and receive responses, along with a helper to locate and analyse the response status block. It's a simple synchronous submit() type API.

The user interface uses the write/read interface that an earlier version of this driver already provided, however it has some specific limitations in order to keep the driver simple and avoid using up a lot of kernel memory:

- The user should perform a single write() with the command and a single read() to get the response (with a buffer big enough to hold the entire response).

- On a write() the command is simply "stored" into a kernel buffer, it is submitted as one operation on the subsequent read(). This allows to have the code write directly from the FIFO into the user buffer and avoid hogging the SBE between the write() and read() syscall as it's critical that the SBE be freed asap to respond to the host. An extra write() will simply replace the previously written command.

- A write of a single 4 bytes containing the value 0x52534554 in big endian will trigger a reset request. No read is necessary, the write() call will return when the reset has been acknowledged or times out.

- The command is limited to 4K bytes.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---

v2. Return -EAGAIN on read() when there's no pending message as
to be compatible with existing userspace (cronus) expectations

v3. Don't print "Initial HW cleanup failed" when the error is
-ESHUTDOWN (is the host is not booted).

v4. Print FFDC packages on command errors and collect & print
async FFDC when available.
Use symbolic constants for commands
Use an iovec iterator instead of set_fs/put_user

---
 drivers/fsi/Makefile        |    2 +-
 drivers/fsi/fsi-sbefifo.c   | 1007 +++++++++++++++++++++++++++++++++++
 include/linux/fsi-sbefifo.h |   33 ++
 3 files changed, 1041 insertions(+), 1 deletion(-)
 create mode 100644 drivers/fsi/fsi-sbefifo.c
 create mode 100644 include/linux/fsi-sbefifo.h

drivers/fsi/Makefile +1 -1

index 61e8e420c7ed..f331e929423c 100644
@@ -3,5 +3,5 @@ obj-$(CONFIG_FSI) += fsi-core.o
 obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
 obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
 obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
-#obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
+obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
 #obj-$(CONFIG_FSI_OCC) += fsi-occ.o

drivers/fsi/fsi-sbefifo.c new file +1007 -0

1,007 lines not on this page · raw message

include/linux/fsi-sbefifo.h new file +33 -0

new file mode 100644
index 000000000000..13f9ebeaa25e
@@ -0,0 +1,33 @@
+/*
+ * SBEFIFO FSI Client device driver
+ *
+ * Copyright (C) IBM Corporation 2017
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef LINUX_FSI_SBEFIFO_H
+#define LINUX_FSI_SBEFIFO_H
+
+#define SBEFIFO_CMD_PUT_OCC_SRAM	0xa404
+#define SBEFIFO_CMD_GET_OCC_SRAM	0xa403
+#define SBEFIFO_CMD_GET_SBE_FFDC	0xa801
+
+#define SBEFIFO_MAX_FFDC_SIZE		0x2000
+
+struct device;
+
+int sbefifo_submit(struct device *dev, const __be32 *command, size_t cmd_len,
+		   __be32 *response, size_t *resp_len);
+
+int sbefifo_parse_status(struct device *dev, u16 cmd, __be32 *response,
+			 size_t resp_len, size_t *data_len);
+
+#endif /* LINUX_FSI_SBEFIFO_H */

Signature · 1 line

2.17.0

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #10 lore raw

PATCH 8/10 fsi/fsi-occ: Simple conversion to new sbefifo driver

In reply to Benjamin Herrenschmidt

Replace open/close/write/read API with the simple submit() API and the helper to parse status.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/fsi/Makefile       |   2 +-
 drivers/fsi/fsi-occ.c      | 209 +++++++++++++++++--------------------
 drivers/hwmon/occ/Makefile |   2 +-
 3 files changed, 99 insertions(+), 114 deletions(-)

drivers/fsi/Makefile +1 -1

index f331e929423c..75fdc6d8cfc4 100644
@@ -4,4 +4,4 @@ obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
 obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
 obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
 obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
-#obj-$(CONFIG_FSI_OCC) += fsi-occ.o
+obj-$(CONFIG_FSI_OCC) += fsi-occ.o

drivers/fsi/fsi-occ.c +97 ~~-112~~

285 lines not on this page · raw message

drivers/hwmon/occ/Makefile +1 -1

index ca6d25ae9da8..ab5c3e965ccb 100644
@@ -1,7 +1,7 @@
 occ-hwmon-objs := common.o
 
 ifeq ($(CONFIG_SENSORS_OCC_P9_SBE), y)
-#occ-hwmon-objs += p9_sbe.o
+occ-hwmon-objs += p9_sbe.o
 endif
 
 ifeq ($(CONFIG_SENSORS_OCC_P8_I2C), y)

Signature · 1 line

2.17.0

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #11 lore raw

PATCH 9/10 fsi/occ: Don't set driver data late

In reply to Benjamin Herrenschmidt

Until now, the OCC driver was setting the driver data after registering the character device and the hwmon device.

This might have been intentional, as doing so makes the initial probe of the OCC by the hwmon device fail while the data is NULL (provided you are lucky and the hwmon driver doesn't get bound asynchronously). That failure used to be necessary, otherwise the driver would try to access the SBE fifo at a time when it's not ready, causing all sort of problems.

The new SBE fifo driver is much more robust and will return an appropriate error code, so that (fragile) tweak is no longer necessary.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/fsi/fsi-occ.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/fsi/fsi-occ.c +2 -2

index 3f96d7ddea91..15f41f45fb31 100644
@@ -794,6 +794,8 @@ static int occ_probe(struct platform_device *pdev)
 		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
 	}
 
+	platform_set_drvdata(pdev, occ);
+
 	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
 	occ->mdev.fops = &occ_fops;
 	occ->mdev.minor = MISC_DYNAMIC_MINOR;

@@ -812,8 +814,6 @@ static int occ_probe(struct platform_device *pdev)
 	if (!hwmon_dev)
 		dev_warn(dev, "failed to create hwmon device\n");
 
-	platform_set_drvdata(pdev, occ);
-
 	return 0;
 }

Signature · 1 line

2.17.0

Benjamin Herrenschmidt

author

2018-05-24 05:15 UTC #12 lore raw

PATCH 10/10 hwmon/occ: Silence probe error message when host is shutdown

In reply to Benjamin Herrenschmidt

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 drivers/hwmon/occ/p9_sbe.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/hwmon/occ/p9_sbe.c +7 -1

index 27886897a3d6..9a1d3ae56d69 100644
@@ -110,6 +110,7 @@ static int p9_sbe_occ_send_cmd(struct occ *occ, u8 *cmd)
 static int p9_sbe_occ_probe(struct platform_device *pdev)
 {
 	struct occ *occ;
+	int rc;
 	struct p9_sbe_occ *ctx = devm_kzalloc(&pdev->dev,
 						     sizeof(*ctx),
 						     GFP_KERNEL);

@@ -126,7 +127,12 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 	occ->poll_cmd_data = 0x20;		/* P9 OCC poll data */
 	occ->send_cmd = p9_sbe_occ_send_cmd;
 
-	return occ_setup(occ, "p9_occ");
+	rc = occ_setup(occ, "p9_occ");
+
+	/* Host is shutdown, don't spew errors */
+	if (rc == -ESHUTDOWN)
+		rc = -ENODEV;
+	return rc;
 }
 
 static int p9_sbe_occ_remove(struct platform_device *pdev)

Signature · 1 line

2.17.0

Christopher Bostic

Reviewed-by on v1 1/10

2018-05-24 13:40 UTC #13 lore raw

In reply to Benjamin Herrenschmidt

Reviewed-by Christopher Bostic <[email protected]>

Quoted 88 lines · Benjamin Herrenschmidt

From: Jeremy Kerr <[email protected]>

For implementing relative addressing mode, we'll need to build a command that is coherent with CFAM state. To do that, include the build_command_* functions in the locked section of read/write/term.

Signed-off-by Jeremy Kerr <[email protected]>

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
  drivers/fsi/fsi-master-gpio.c | 25 ++++++++++++++++++-------
  1 file changed, 18 insertions(+), 7 deletions(-)

drivers/fsi/fsi-master-gpio.c +18 -7

index e1bde9e3f855..d50ea4849377 100644
@@ -463,14 +463,10 @@ static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
  {
  	int rc;
 
-	mutex_lock(&master->cmd_lock);
-
  	rc = send_request(master, cmd);
  	if (!rc)
  		rc = poll_for_response(master, slave, resp_len, resp);
 
-	mutex_unlock(&master->cmd_lock);
-
  	return rc;
  }

@@ -479,12 +475,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
  {
  	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
  	struct fsi_gpio_msg cmd;
+	int rc;
 
  	if (link != 0)
  		return -ENODEV;
 
+	mutex_lock(&master->cmd_lock);
  	build_abs_ar_command(&cmd, id, addr, size, NULL);
-	return fsi_master_gpio_xfer(master, id, &cmd, size, val);
+	rc = fsi_master_gpio_xfer(master, id, &cmd, size, val);
+	mutex_unlock(&master->cmd_lock);
+
+	return rc;
  }
 
  static int fsi_master_gpio_write(struct fsi_master *_master, int link,

@@ -492,12 +493,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
  {
  	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
  	struct fsi_gpio_msg cmd;
+	int rc;
 
  	if (link != 0)
  		return -ENODEV;
 
+	mutex_lock(&master->cmd_lock);
  	build_abs_ar_command(&cmd, id, addr, size, val);
-	return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	mutex_unlock(&master->cmd_lock);
+
+	return rc;
  }
 
  static int fsi_master_gpio_term(struct fsi_master *_master,

@@ -505,12 +511,17 @@ static int fsi_master_gpio_term(struct fsi_master *_master,
  {
  	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
  	struct fsi_gpio_msg cmd;
+	int rc;
 
  	if (link != 0)
  		return -ENODEV;
 
+	mutex_lock(&master->cmd_lock);
  	build_term_command(&cmd, id);
-	return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	rc = fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+	mutex_unlock(&master->cmd_lock);
+
+	return rc;
  }
 
  static int fsi_master_gpio_break(struct fsi_master *_master, int link)