[PATCH 1/2] ahci: make ahci_transmit_led_message into a function pointer

[PATCH 1/2] ahci: make ahci_transmit_led_message into a function pointer

[Mark Langsdorf]

Create a new ata_port_operations function pointer called
transmit_led_message and give it the default value of
ahci_transmit_led_message. This allows AHCI controllers with
non-standard LED interfaces to use the existing em_ interface.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
 drivers/ata/libahci.c  | 11 ++++++-----
 include/linux/libata.h |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 34c8216..0c36360 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -173,6 +173,7 @@ struct ata_port_operations ahci_ops = {
 	.em_store		= ahci_led_store,
 	.sw_activity_show	= ahci_activity_show,
 	.sw_activity_store	= ahci_activity_store,
+	.transmit_led_message	= ahci_transmit_led_message,
 #ifdef CONFIG_PM
 	.port_suspend		= ahci_port_suspend,
 	.port_resume		= ahci_port_resume,
@@ -774,7 +775,7 @@ static void ahci_start_port(struct ata_port *ap)
 
 			/* EM Transmit bit maybe busy during init */
 			for (i = 0; i < EM_MAX_RETRY; i++) {
-				rc = ahci_transmit_led_message(ap,
+				rc = ap->ops->transmit_led_message(ap,
 							       emp->led_state,
 							       4);
 				if (rc == -EBUSY)
@@ -915,7 +916,7 @@ static void ahci_sw_activity_blink(unsigned long arg)
 			led_message |= (1 << 16);
 	}
 	spin_unlock_irqrestore(ap->lock, flags);
-	ahci_transmit_led_message(ap, led_message, 4);
+	ap->ops->transmit_led_message(ap, led_message, 4);
 }
 
 static void ahci_init_sw_activity(struct ata_link *link)
@@ -1044,7 +1045,7 @@ static ssize_t ahci_led_store(struct ata_port *ap, const char *buf,
 	if (emp->blink_policy)
 		state &= ~EM_MSG_LED_VALUE_ACTIVITY;
 
-	return ahci_transmit_led_message(ap, state, size);
+	return ap->ops->transmit_led_message(ap, state, size);
 }
 
 static ssize_t ahci_activity_store(struct ata_device *dev, enum sw_activity val)
@@ -1063,7 +1064,7 @@ static ssize_t ahci_activity_store(struct ata_device *dev, enum sw_activity val)
 		/* set the LED to OFF */
 		port_led_state &= EM_MSG_LED_VALUE_OFF;
 		port_led_state |= (ap->port_no | (link->pmp << 8));
-		ahci_transmit_led_message(ap, port_led_state, 4);
+		ap->ops->transmit_led_message(ap, port_led_state, 4);
 	} else {
 		link->flags |= ATA_LFLAG_SW_ACTIVITY;
 		if (val == BLINK_OFF) {
@@ -1071,7 +1072,7 @@ static ssize_t ahci_activity_store(struct ata_device *dev, enum sw_activity val)
 			port_led_state &= EM_MSG_LED_VALUE_OFF;
 			port_led_state |= (ap->port_no | (link->pmp << 8));
 			port_led_state |= EM_MSG_LED_VALUE_ON; /* check this */
-			ahci_transmit_led_message(ap, port_led_state, 4);
+			ap->ops->transmit_led_message(ap, port_led_state, 4);
 		}
 	}
 	emp->blink_policy = val;
diff --git a/include/linux/libata.h b/include/linux/libata.h
index eae7a05..1d09b65 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -908,6 +908,9 @@ struct ata_port_operations {
 	ssize_t (*sw_activity_show)(struct ata_device *dev, char *buf);
 	ssize_t (*sw_activity_store)(struct ata_device *dev,
 				     enum sw_activity val);
+	ssize_t (*transmit_led_message)(struct ata_port *ap, u32 state,
+					ssize_t size);
+
 	/*
 	 * Obsolete
 	 */
-- 
1.8.1.4

[PATCH 2/2] sata highbank: add bit-banged SGPIO driver support

[Mark Langsdorf]

Highbank supports SGPIO by bit-banging out the SGPIO signals over
three GPIO pins defined in the DTB. Add support for this SGPIO
functionality.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
 .../devicetree/bindings/ata/ahci-platform.txt      |   9 ++
 arch/arm/boot/dts/ecx-common.dtsi                  |   1 +
 drivers/ata/sata_highbank.c                        | 128 ++++++++++++++++++++-
 3 files changed, 133 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index b519f9b..123b7ae 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -12,6 +12,15 @@ Optional properties:
 - calxeda,port-phys: phandle-combophy and lane assignment, which maps each
 			SATA port to a combophy and a lane within that
 			combophy
+- calxeda,sgpio-gpio: phandle-gpio bank, bit offset, and default on or off,
+			which indicates that the driver supports SGPIO 
+			indicator lights using the indicated GPIOs
+- calxeda,led-order : a u32 array that map port numbers to offsets within the
+			SGPIO bitstream. By default, port 0 has offset 4
+			and the other ports are offset by their port number
+			less 1. if calxeda,sgpio-gpio is used and that port
+			to led mapping is incorrect, this array should have
+			each port's offset within the bitstream.
 - dma-coherent      : Present if dma operations are coherent
 
 Example:
diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index d61b535..8c1d643 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -33,6 +33,7 @@
 			calxeda,port-phys = <&combophy5 0 &combophy0 0
 					     &combophy0 1 &combophy0 2
 					     &combophy0 3>;
+			calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
 		};
 
 		sdhci@ffe0e000 {
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index b20aa96..d48f708 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -33,6 +33,9 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
 #include "ahci.h"
 
 #define CPHY_MAP(dev, addr) ((((dev) & 0x1f) << 7) | (((addr) >> 9) & 0x7f))
@@ -66,6 +69,124 @@ struct phy_lane_info {
 };
 static struct phy_lane_info port_data[CPHY_PORT_COUNT];
 
+static DEFINE_SPINLOCK(sgpio_lock);
+#define SCLOCK				0
+#define SLOAD				1
+#define SDATA				2
+static unsigned ecx_sgpio[3];
+static unsigned long ecx_sgpio_pattern;
+static int port_to_sgpio[] = { 4, 0, 1, 2, 3};
+static int n_ports;
+
+#define ACTIVITY_BITS			0x300000 // 0x7
+#define ACTIVITY_SHIFT			2
+#define LOCATE_BITS 			0x80000  // 0x38
+#define LOCATE_SHIFT			1
+#define FAULT_BITS 			0x400000 // 0x1c0
+#define FAULT_SHIFT			1
+#define SGPIO_BIT(port, shift)		1 << (3 * port_to_sgpio[(port)] + \
+							(shift))
+static void ecx_parse_sgpio(u32 port, u32 state)
+{
+	if (state == 0) {
+		ecx_sgpio_pattern &= ~(SGPIO_BIT(port, ACTIVITY_SHIFT) |
+					SGPIO_BIT(port, LOCATE_SHIFT) |
+					SGPIO_BIT(port, FAULT_SHIFT));
+		return;
+	}
+	if (state & ACTIVITY_BITS)
+		ecx_sgpio_pattern |= SGPIO_BIT(port, ACTIVITY_SHIFT);
+	if (state & LOCATE_BITS)
+		ecx_sgpio_pattern |= SGPIO_BIT(port, LOCATE_SHIFT);
+	if (state & FAULT_BITS)
+		ecx_sgpio_pattern |= SGPIO_BIT(port, FAULT_SHIFT);
+}
+
+/*
+ * Tell the LED controller that the signal has changed by raising the clock
+ * line for 50 uS and then lowering it for 50 uS.
+ */
+static void ecx_led_cycle_clock(void)
+{
+       gpio_set_value(ecx_sgpio[SCLOCK], 1);
+       udelay(50);
+       gpio_set_value(ecx_sgpio[SCLOCK], 0);
+       udelay(50);
+}
+
+static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
+					ssize_t size)
+{
+	struct ahci_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	unsigned long flags;
+	int pmp, i;
+	struct ahci_em_priv *emp;
+	u32 sgpio_out;
+
+	/* get the slot number from the message */
+	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
+	if (pmp < EM_MAX_SLOTS)
+		emp = &pp->em_priv[pmp];
+	else
+		return -EINVAL;
+
+	if (!hpriv->em_msg_type & EM_MSG_TYPE_LED) 
+		return size;
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+	ecx_parse_sgpio(ap->port_no, state);
+	sgpio_out = ecx_sgpio_pattern;
+	gpio_set_value(ecx_sgpio[SLOAD], 1);
+	ecx_led_cycle_clock();
+	gpio_set_value(ecx_sgpio[SLOAD], 0);
+	for (i = 0; i < (3 * n_ports); i++) {
+		gpio_set_value(ecx_sgpio[SDATA], sgpio_out & 1);
+		sgpio_out >>= 1;
+		ecx_led_cycle_clock();
+	}
+
+	/* save off new led state for port/slot */
+	emp->led_state = state;
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+	return size;
+}
+
+void highbank_set_em_messages(struct device *dev, struct ahci_host_priv *hpriv,
+			  struct ata_port_info *pi)
+{
+	struct device_node *np = dev->of_node;
+	int i;
+	int err;
+
+	for (i = 0;i < 3; i++) {
+		ecx_sgpio[i] = of_get_named_gpio(np, "calxeda,sgpio-gpio", i);
+		if (IS_ERR_VALUE(ecx_sgpio[i]))
+		       return;
+
+	        err = gpio_request(ecx_sgpio[i], "CX SGPIO");
+		if (err) {
+			printk(KERN_ERR
+				"sata_highbank gpio_request %d failed: %d\n",
+					i, err);
+		                return;
+		}
+		gpio_direction_output(ecx_sgpio[i], 1);
+	}
+	/* there's a default ordering, but give it an optional override */
+	for (i = 0; i < n_ports; i++) {
+		of_property_read_u32_array(np, "calxeda,led-order", 
+						port_to_sgpio, i);
+	}
+
+	/* store em_loc */
+	hpriv->em_loc = 0;
+	hpriv->em_buf_sz = 4;
+	hpriv->em_msg_type = EM_MSG_TYPE_LED;
+	pi->flags |= ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY;
+}
+
 static u32 __combo_phy_reg_read(u8 sata_port, u32 addr)
 {
 	u32 data;
@@ -241,6 +362,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 static struct ata_port_operations ahci_highbank_ops = {
 	.inherits		= &ahci_ops,
 	.hardreset		= ahci_highbank_hardreset,
+	.transmit_led_message   = ecx_transmit_led_message,
 };
 
 static const struct ata_port_info ahci_highbank_port_info = {
@@ -267,7 +389,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	struct ata_host *host;
 	struct resource *mem;
 	int irq;
-	int n_ports;
 	int i;
 	int rc;
 	struct ata_port_info pi = ahci_highbank_port_info;
@@ -313,7 +434,7 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (hpriv->cap & HOST_CAP_PMP)
 		pi.flags |= ATA_FLAG_PMP;
 
-	ahci_set_em_messages(hpriv, &pi);
+	highbank_set_em_messages(dev, hpriv, &pi);
 
 	/* CAP.NP sometimes indicate the index of the last enabled
 	 * port, at other times, that of the last possible port, so
@@ -333,9 +454,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
 		host->flags |= ATA_HOST_PARALLEL_SCAN;
 
-	if (pi.flags & ATA_FLAG_EM)
-		ahci_reset_em(host);
-
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-- 
1.8.1.4

Re: [PATCH 2/2] sata highbank: add bit-banged SGPIO driver support

[Tejun Heo]

On Thu, May 30, 2013 at 03:17:31PM -0500, Mark Langsdorf wrote:
> +static DEFINE_SPINLOCK(sgpio_lock);
> +#define SCLOCK				0
> +#define SLOAD				1
> +#define SDATA				2
> +static unsigned ecx_sgpio[3];
> +static unsigned long ecx_sgpio_pattern;
> +static int port_to_sgpio[] = { 4, 0, 1, 2, 3};
> +static int n_ports;

Please use more specific names for global constants and variables.
Defining something like n_ports as a global variable is silly and
can lead to silly bugs later on.

> +#define ACTIVITY_BITS			0x300000 // 0x7
> +#define ACTIVITY_SHIFT			2
> +#define LOCATE_BITS 			0x80000  // 0x38
> +#define LOCATE_SHIFT			1
> +#define FAULT_BITS 			0x400000 // 0x1c0

No //s please and please pick and use a common prefix.  It doesn't
have to be long or complete.  Even something as short as HB_ is fine.

> +#define FAULT_SHIFT			1
> +#define SGPIO_BIT(port, shift)		1 << (3 * port_to_sgpio[(port)] + \
> +							(shift))

Please make it a function.

> +static void ecx_parse_sgpio(u32 port, u32 state)
> +{
> +	if (state == 0) {
> +		ecx_sgpio_pattern &= ~(SGPIO_BIT(port, ACTIVITY_SHIFT) |
> +					SGPIO_BIT(port, LOCATE_SHIFT) |
> +					SGPIO_BIT(port, FAULT_SHIFT));
> +		return;
> +	}
> +	if (state & ACTIVITY_BITS)
> +		ecx_sgpio_pattern |= SGPIO_BIT(port, ACTIVITY_SHIFT);
> +	if (state & LOCATE_BITS)
> +		ecx_sgpio_pattern |= SGPIO_BIT(port, LOCATE_SHIFT);
> +	if (state & FAULT_BITS)
> +		ecx_sgpio_pattern |= SGPIO_BIT(port, FAULT_SHIFT);
> +}

Maybe I'm misunderstanding the code but things are cleared iff @state
is zero?  Is that intentional?

....
> +static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
> +					ssize_t size)
> +{
> +	struct ahci_host_priv *hpriv = ap->host->private_data;
> +	struct ahci_port_priv *pp = ap->private_data;
> +	unsigned long flags;
> +	int pmp, i;
> +	struct ahci_em_priv *emp;
> +	u32 sgpio_out;
> +
> +	/* get the slot number from the message */
> +	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
> +	if (pmp < EM_MAX_SLOTS)
> +		emp = &pp->em_priv[pmp];
> +	else
> +		return -EINVAL;
> +
> +	if (!hpriv->em_msg_type & EM_MSG_TYPE_LED) 

Huh?  This can't be right.

> +		return size;
> +
> +	spin_lock_irqsave(&sgpio_lock, flags);
> +	ecx_parse_sgpio(ap->port_no, state);
> +	sgpio_out = ecx_sgpio_pattern;

Hmmm... global data.  Can we at least move it into host private data?

> +	gpio_set_value(ecx_sgpio[SLOAD], 1);
> +	ecx_led_cycle_clock();
> +	gpio_set_value(ecx_sgpio[SLOAD], 0);
> +	for (i = 0; i < (3 * n_ports); i++) {
> +		gpio_set_value(ecx_sgpio[SDATA], sgpio_out & 1);
> +		sgpio_out >>= 1;
> +		ecx_led_cycle_clock();
> +	}

What's the above loop achieving?  Care to add a comment?

> +
> +	/* save off new led state for port/slot */
> +	emp->led_state = state;
> +
> +	spin_unlock_irqrestore(&sgpio_lock, flags);
> +	return size;
> +}
> +
> +void highbank_set_em_messages(struct device *dev, struct ahci_host_priv *hpriv,
> +			  struct ata_port_info *pi)
> +{
> +	struct device_node *np = dev->of_node;
> +	int i;
> +	int err;
> +
> +	for (i = 0;i < 3; i++) {

Missing space after ; and what's this magic 3 I keep seeing?

> +		ecx_sgpio[i] = of_get_named_gpio(np, "calxeda,sgpio-gpio", i);
> +		if (IS_ERR_VALUE(ecx_sgpio[i]))
> +		       return;
> +
> +	        err = gpio_request(ecx_sgpio[i], "CX SGPIO");
> +		if (err) {
> +			printk(KERN_ERR
> +				"sata_highbank gpio_request %d failed: %d\n",
> +					i, err);
> +		                return;

pr_err()?

> +		}
> +		gpio_direction_output(ecx_sgpio[i], 1);
> +	}
> +	/* there's a default ordering, but give it an optional override */
> +	for (i = 0; i < n_ports; i++) {
> +		of_property_read_u32_array(np, "calxeda,led-order", 
> +						port_to_sgpio, i);
> +	}

{ } unnecessary.

Thanks.

-- 
tejun

[PATCH 2/2 v2] sata highbank: add bit-banged SGPIO driver support

[Mark Langsdorf]

Highbank supports SGPIO by bit-banging out the SGPIO signals over
three GPIO pins defined in the DTB. Add support for this SGPIO
functionality.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
Changes from v1:
	Moved all global variables to a private structure
	Replaced all magic numbers with defined symbols
	Added some comments
	Removed the default ordering of ports to LEDs
	Fixed several bugs the code to read led-order
	Made highbank_set_em_messages static

 .../devicetree/bindings/ata/ahci-platform.txt      |   5 +
 arch/arm/boot/dts/ecx-common.dtsi                  |   2 +
 drivers/ata/sata_highbank.c                        | 167 +++++++++++++++++++--
 3 files changed, 165 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index b519f9b..b271712 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -12,6 +12,11 @@ Optional properties:
 - calxeda,port-phys: phandle-combophy and lane assignment, which maps each
 			SATA port to a combophy and a lane within that
 			combophy
+- calxeda,sgpio-gpio: phandle-gpio bank, bit offset, and default on or off,
+			which indicates that the driver supports SGPIO
+			indicator lights using the indicated GPIOs
+- calxeda,led-order : a u32 array that map port numbers to offsets within the
+			SGPIO bitstream.
 - dma-coherent      : Present if dma operations are coherent
 
 Example:
diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index d61b535..e8559b7 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -33,6 +33,8 @@
 			calxeda,port-phys = <&combophy5 0 &combophy0 0
 					     &combophy0 1 &combophy0 2
 					     &combophy0 3>;
+			calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
+			calxeda,led-order = <4 0 1 2 3>;
 		};
 
 		sdhci@ffe0e000 {
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index b20aa96..64f2acf 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -33,6 +33,9 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
 #include "ahci.h"
 
 #define CPHY_MAP(dev, addr) ((((dev) & 0x1f) << 7) | (((addr) >> 9) & 0x7f))
@@ -66,6 +69,154 @@ struct phy_lane_info {
 };
 static struct phy_lane_info port_data[CPHY_PORT_COUNT];
 
+static DEFINE_SPINLOCK(sgpio_lock);
+#define SCLOCK				0
+#define SLOAD				1
+#define SDATA				2
+#define SGPIO_PINS			3
+#define SGPIO_PORTS			8
+
+/* can be cast as an ahci_host_priv for compatibility with most functions */
+struct ecx_host_priv {
+	void __iomem	*mmio;		/* bus-independent mem map */
+	unsigned int	flags;		/* AHCI_HFLAG_* */
+	u32		cap;		/* cap to use */
+	u32		cap2;		/* cap2 to use */
+	u32		port_map;	/* port map to use */
+	u32		saved_cap;	/* saved initial cap */
+	u32		saved_cap2;	/* saved initial cap2 */
+	u32		saved_port_map;	/* saved initial port_map */
+	u32		em_loc;		/* enclosure management location */
+	u32		em_buf_sz;      /* EM buffer size in byte */
+	u32		em_msg_type;    /* EM message type */
+	struct clk	*clk;		/* Only for platforms supporting clk */
+	u32		n_ports;
+	unsigned	sgpio_gpio[SGPIO_PINS];
+	u32		sgpio_pattern;
+	u32		port_to_sgpio[SGPIO_PORTS];
+};
+
+#define SGPIO_SIGNALS			3
+#define ECX_ACTIVITY_BITS		0x300000
+#define ECX_ACTIVITY_SHIFT		2
+#define ECX_LOCATE_BITS		0x80000
+#define ECX_LOCATE_SHIFT		1
+#define ECX_FAULT_BITS			0x400000
+#define ECX_FAULT_SHIFT			0
+static inline int sgpio_bit_shift(struct ecx_host_priv *hpriv, u32 port,
+				u32 shift)
+{
+	return 1 << (3 * hpriv->port_to_sgpio[port] + shift);
+}
+
+static void ecx_parse_sgpio(struct ecx_host_priv *hpriv, u32 port, u32 state)
+{
+	if (state == 0) {
+		hpriv->sgpio_pattern &= ~(sgpio_bit_shift(hpriv, port,
+						ECX_ACTIVITY_SHIFT) |
+			sgpio_bit_shift(hpriv, port, ECX_LOCATE_SHIFT) |
+			sgpio_bit_shift(hpriv, port, ECX_FAULT_SHIFT));
+		return;
+	}
+	if (state & ECX_ACTIVITY_BITS)
+		hpriv->sgpio_pattern |= sgpio_bit_shift(hpriv, port,
+						ECX_ACTIVITY_SHIFT);
+	if (state & ECX_LOCATE_BITS)
+		hpriv->sgpio_pattern |= sgpio_bit_shift(hpriv, port,
+						ECX_LOCATE_SHIFT);
+	if (state & ECX_FAULT_BITS)
+		hpriv->sgpio_pattern |= sgpio_bit_shift(hpriv, port,
+						ECX_FAULT_SHIFT);
+}
+
+/*
+ * Tell the LED controller that the signal has changed by raising the clock
+ * line for 50 uS and then lowering it for 50 uS.
+ */
+static void ecx_led_cycle_clock(struct ecx_host_priv *hpriv)
+{
+	gpio_set_value(hpriv->sgpio_gpio[SCLOCK], 1);
+	udelay(50);
+	gpio_set_value(hpriv->sgpio_gpio[SCLOCK], 0);
+	udelay(50);
+}
+
+static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
+					ssize_t size)
+{
+	struct ecx_host_priv *hpriv = ap->host->private_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	unsigned long flags;
+	int pmp, i;
+	struct ahci_em_priv *emp;
+	u32 sgpio_out;
+
+	/* get the slot number from the message */
+	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
+	if (pmp < EM_MAX_SLOTS)
+		emp = &pp->em_priv[pmp];
+	else
+		return -EINVAL;
+
+	if (!hpriv->em_msg_type & EM_MSG_TYPE_LED)
+		return size;
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+	ecx_parse_sgpio(hpriv, ap->port_no, state);
+	sgpio_out = hpriv->sgpio_pattern;
+	gpio_set_value(hpriv->sgpio_gpio[SLOAD], 1);
+	ecx_led_cycle_clock(hpriv);
+	gpio_set_value(hpriv->sgpio_gpio[SLOAD], 0);
+	/*
+	 * bit-bang out the SGPIO pattern, by consuming a bit and then
+	 * clocking it out.
+	 */
+	for (i = 0; i < (SGPIO_SIGNALS * hpriv->n_ports); i++) {
+		gpio_set_value(hpriv->sgpio_gpio[SDATA], sgpio_out & 1);
+		sgpio_out >>= 1;
+		ecx_led_cycle_clock(hpriv);
+	}
+
+	/* save off new led state for port/slot */
+	emp->led_state = state;
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+	return size;
+}
+
+static void highbank_set_em_messages(struct device *dev,
+					struct ecx_host_priv *hpriv,
+					struct ata_port_info *pi)
+{
+	struct device_node *np = dev->of_node;
+	int i;
+	int err;
+
+	for (i = 0; i < SGPIO_PINS; i++) {
+		err = of_get_named_gpio(np, "calxeda,sgpio-gpio", i);
+		if (IS_ERR_VALUE(err))
+			return;
+
+		hpriv->sgpio_gpio[i] = err;
+		err = gpio_request(hpriv->sgpio_gpio[i], "CX SGPIO");
+		if (err) {
+			pr_err("sata_highbank gpio_request %d failed: %d\n",
+					i, err);
+			return;
+		}
+		gpio_direction_output(hpriv->sgpio_gpio[i], 1);
+	}
+	of_property_read_u32_array(np, "calxeda,led-order",
+						hpriv->port_to_sgpio,
+						hpriv->n_ports);
+
+	/* store em_loc */
+	hpriv->em_loc = 0;
+	hpriv->em_buf_sz = 4;
+	hpriv->em_msg_type = EM_MSG_TYPE_LED;
+	pi->flags |= ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY;
+}
+
 static u32 __combo_phy_reg_read(u8 sata_port, u32 addr)
 {
 	u32 data;
@@ -241,6 +392,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 static struct ata_port_operations ahci_highbank_ops = {
 	.inherits		= &ahci_ops,
 	.hardreset		= ahci_highbank_hardreset,
+	.transmit_led_message   = ecx_transmit_led_message,
 };
 
 static const struct ata_port_info ahci_highbank_port_info = {
@@ -263,13 +415,13 @@ MODULE_DEVICE_TABLE(of, ahci_of_match);
 static int ahci_highbank_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct ahci_host_priv *hpriv;
+	struct ecx_host_priv *hpriv;
 	struct ata_host *host;
 	struct resource *mem;
 	int irq;
-	int n_ports;
 	int i;
 	int rc;
+	u32 n_ports;
 	struct ata_port_info pi = ahci_highbank_port_info;
 	const struct ata_port_info *ppi[] = { &pi, NULL };
 
@@ -303,8 +455,7 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-
-	ahci_save_initial_config(dev, hpriv, 0, 0);
+	ahci_save_initial_config(dev, (struct ahci_host_priv *) hpriv, 0, 0);
 
 	/* prepare host */
 	if (hpriv->cap & HOST_CAP_NCQ)
@@ -313,8 +464,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (hpriv->cap & HOST_CAP_PMP)
 		pi.flags |= ATA_FLAG_PMP;
 
-	ahci_set_em_messages(hpriv, &pi);
-
 	/* CAP.NP sometimes indicate the index of the last enabled
 	 * port, at other times, that of the last possible port, so
 	 * determining the maximum port number requires looking at
@@ -322,6 +471,9 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	 */
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
+	hpriv->n_ports = n_ports;
+	highbank_set_em_messages(dev, hpriv, &pi);
+
 	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
 	if (!host) {
 		rc = -ENOMEM;
@@ -333,9 +485,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
 		host->flags |= ATA_HOST_PARALLEL_SCAN;
 
-	if (pi.flags & ATA_FLAG_EM)
-		ahci_reset_em(host);
-
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-- 
1.8.1.4

Re: [PATCH 2/2 v2] sata highbank: add bit-banged SGPIO driver support

[Tejun Heo]

Hello, Mark.

In general, please try to reply to reviews addressing each point.  It
gives much better sense of what's going on to the reviewer and also
helps the reviewee to avoid misunderstandings or missing points.

> +static DEFINE_SPINLOCK(sgpio_lock);
> +#define SCLOCK				0
> +#define SLOAD				1
> +#define SDATA				2
> +#define SGPIO_PINS			3
> +#define SGPIO_PORTS			8
> +
> +/* can be cast as an ahci_host_priv for compatibility with most functions */

This sounds awfully scary.  What's going on here?  Are you actually
overriding ahci_host_priv?  If so, please don't ever do things like
that.  Do it properly.  Add host_priv->platform_priv or whatever and
chain the pointer there.  If you're worried about the extra deref,
update ahci core such that it allows specifying extra size and you can
embed ahci_host_priv in your own priv.  ie.

	struct ecx_host_priv {
		struct ahci_host_priv	ahci_priv;	/* must be the first field */
		/* your own stuff */
	};

And tell ahci core sizeof(ecx_host_priv) some way, but really, just
having a plain pointer should be enough, I think.

> +struct ecx_host_priv {
> +	void __iomem	*mmio;		/* bus-independent mem map */
> +	unsigned int	flags;		/* AHCI_HFLAG_* */
> +	u32		cap;		/* cap to use */
> +	u32		cap2;		/* cap2 to use */
> +	u32		port_map;	/* port map to use */
> +	u32		saved_cap;	/* saved initial cap */
> +	u32		saved_cap2;	/* saved initial cap2 */
> +	u32		saved_port_map;	/* saved initial port_map */
> +	u32		em_loc;		/* enclosure management location */
> +	u32		em_buf_sz;      /* EM buffer size in byte */
> +	u32		em_msg_type;    /* EM message type */
> +	struct clk	*clk;		/* Only for platforms supporting clk */
> +	u32		n_ports;
> +	unsigned	sgpio_gpio[SGPIO_PINS];
> +	u32		sgpio_pattern;
> +	u32		port_to_sgpio[SGPIO_PORTS];
> +};
> +
> +#define SGPIO_SIGNALS			3
> +#define ECX_ACTIVITY_BITS		0x300000
> +#define ECX_ACTIVITY_SHIFT		2
> +#define ECX_LOCATE_BITS		0x80000
> +#define ECX_LOCATE_SHIFT		1
> +#define ECX_FAULT_BITS			0x400000
> +#define ECX_FAULT_SHIFT			0
> +static inline int sgpio_bit_shift(struct ecx_host_priv *hpriv, u32 port,
> +				u32 shift)
> +{
> +	return 1 << (3 * hpriv->port_to_sgpio[port] + shift);
> +}
> +
> +static void ecx_parse_sgpio(struct ecx_host_priv *hpriv, u32 port, u32 state)

Would be kinda nice to have comment explaining what the function does
as @state == 0 turns off everything while anything else would just
turn things on.

> +static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
> +					ssize_t size)
> +{
...
> +	if (!hpriv->em_msg_type & EM_MSG_TYPE_LED)
> +		return size;

Is this really correct?  You first negate and convert it to bool and
then bit-wise and it with a mask?  How is supposed to work?

> -	ahci_save_initial_config(dev, hpriv, 0, 0);
> +	ahci_save_initial_config(dev, (struct ahci_host_priv *) hpriv, 0, 0);

Ugh....... how is this supposed to work?  What if ahci_host_priv grows
larger than ecx one in the future? :(

-- 
tejun

Re: [PATCH 2/2 v2] sata highbank: add bit-banged SGPIO driver support

[Mark Langsdorf]

On 06/03/2013 03:37 PM, Tejun Heo wrote:
> Hello, Mark.
> 
> In general, please try to reply to reviews addressing each point.  It
> gives much better sense of what's going on to the reviewer and also
> helps the reviewee to avoid misunderstandings or missing points.
> 
>> +static DEFINE_SPINLOCK(sgpio_lock);
>> +#define SCLOCK				0
>> +#define SLOAD				1
>> +#define SDATA				2
>> +#define SGPIO_PINS			3
>> +#define SGPIO_PORTS			8
>> +
>> +/* can be cast as an ahci_host_priv for compatibility with most functions */
> 
> This sounds awfully scary.  What's going on here?  Are you actually
> overriding ahci_host_priv?  If so, please don't ever do things like
> that.  Do it properly.  Add host_priv->platform_priv or whatever and
> chain the pointer there.  If you're worried about the extra deref,
> update ahci core such that it allows specifying extra size and you can
> embed ahci_host_priv in your own priv.  ie.
> 
> 	struct ecx_host_priv {
> 		struct ahci_host_priv	ahci_priv;	/* must be the first field */
> 		/* your own stuff */
> 	};
> 
> And tell ahci core sizeof(ecx_host_priv) some way, but really, just
> having a plain pointer should be enough, I think.

I think I want to do the opposite. For 90% of the AHCI EM functions,
I want ecx_host_priv to be an ahci_host_priv so that I can use those
functions without having to keep a local copy of them.

Would something like this:
struct ahci_host_priv {
	/* standard AHCI existing stuff */
	void *private_data;
};

I shied away from that because a private data structure having a private
data structure doesn't seem right.

>> +static inline int sgpio_bit_shift(struct ecx_host_priv *hpriv, u32 port,
>> +				u32 shift)
>> +{
>> +	return 1 << (3 * hpriv->port_to_sgpio[port] + shift);
>> +}
>> +
>> +static void ecx_parse_sgpio(struct ecx_host_priv *hpriv, u32 port, u32 state)
> 
> Would be kinda nice to have comment explaining what the function does
> as @state == 0 turns off everything while anything else would just
> turn things on.

Okay.

>> +static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
>> +					ssize_t size)
>> +{
> ...
>> +	if (!hpriv->em_msg_type & EM_MSG_TYPE_LED)
>> +		return size;
> 
> Is this really correct?  You first negate and convert it to bool and
> then bit-wise and it with a mask?  How is supposed to work?

Am I confused about the order of operations? It's meant to be "continue
if hpriv->em_msg_type doesn't have EM_MSG_TYPE_LED set".

>> -	ahci_save_initial_config(dev, hpriv, 0, 0);
>> +	ahci_save_initial_config(dev, (struct ahci_host_priv *) hpriv, 0, 0);
> 
> Ugh....... how is this supposed to work?  What if ahci_host_priv grows
> larger than ecx one in the future? :(

For functions like ahci_save_initial_config, I just want to use the
already defined ahci_ functions with my extra data along for the ride.
What's the best way to do that?

--Mark Langsdorf
Calxeda, Inc.

Re: [PATCH 2/2 v2] sata highbank: add bit-banged SGPIO driver support

[Tejun Heo]

Hello, Mark.

On Tue, Jun 04, 2013 at 10:09:41AM -0500, Mark Langsdorf wrote:
> > And tell ahci core sizeof(ecx_host_priv) some way, but really, just
> > having a plain pointer should be enough, I think.
> 
> I think I want to do the opposite. For 90% of the AHCI EM functions,
> I want ecx_host_priv to be an ahci_host_priv so that I can use those
> functions without having to keep a local copy of them.
> 
> Would something like this:
> struct ahci_host_priv {
> 	/* standard AHCI existing stuff */
> 	void *private_data;
> };
> 
> I shied away from that because a private data structure having a private
> data structure doesn't seem right.

Aren't we talking about the same thing?  I'm perfectly fine with
adding a pointer to ahci_host_priv.  Maybe you can name it slightly
differently - say, *impl_data, *platform_data, whatever.

> >> +static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
> >> +					ssize_t size)
> >> +{
> > ...
> >> +	if (!hpriv->em_msg_type & EM_MSG_TYPE_LED)
> >> +		return size;
> > 
> > Is this really correct?  You first negate and convert it to bool and
> > then bit-wise and it with a mask?  How is supposed to work?
> 
> Am I confused about the order of operations? It's meant to be "continue
> if hpriv->em_msg_type doesn't have EM_MSG_TYPE_LED set".

Shouldn't that be

	if (!(hpriv->em_msg_type & EM_MSG_TYPE_LED))

! has higher priority than &.  You're converting em_msg_type to 1 or 0
  and then and'ing EM_MSG_TYPE_LED to it.

> >> -	ahci_save_initial_config(dev, hpriv, 0, 0);
> >> +	ahci_save_initial_config(dev, (struct ahci_host_priv *) hpriv, 0, 0);
> > 
> > Ugh....... how is this supposed to work?  What if ahci_host_priv grows
> > larger than ecx one in the future? :(
> 
> For functions like ahci_save_initial_config, I just want to use the
> already defined ahci_ functions with my extra data along for the ride.
> What's the best way to do that?

Please don't override different types on the same area.  Having the
driver specific data in a separate struct pointed to by ahci_host_priv
should work fine, right?

Thanks.

-- 
tejun

Re: [PATCH 2/2 v2] sata highbank: add bit-banged SGPIO driver support

[Mark Langsdorf]

On 06/04/2013 03:32 PM, Tejun Heo wrote:
> Hello, Mark.
> 
> On Tue, Jun 04, 2013 at 10:09:41AM -0500, Mark Langsdorf wrote:
>>> And tell ahci core sizeof(ecx_host_priv) some way, but really, just
>>> having a plain pointer should be enough, I think.
>>
>> I think I want to do the opposite. For 90% of the AHCI EM functions,
>> I want ecx_host_priv to be an ahci_host_priv so that I can use those
>> functions without having to keep a local copy of them.
>>
>> Would something like this:
>> struct ahci_host_priv {
>> 	/* standard AHCI existing stuff */
>> 	void *private_data;
>> };
>>
>> I shied away from that because a private data structure having a private
>> data structure doesn't seem right.
> 
> Aren't we talking about the same thing?  I'm perfectly fine with
> adding a pointer to ahci_host_priv.  Maybe you can name it slightly
> differently - say, *impl_data, *platform_data, whatever.

I guess we are talking about the same thing. I'll do that.

>>>> +static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
>>>> +					ssize_t size)
>>>> +{
>>> ...
>>>> +	if (!hpriv->em_msg_type & EM_MSG_TYPE_LED)
>>>> +		return size;
>>>
>>> Is this really correct?  You first negate and convert it to bool and
>>> then bit-wise and it with a mask?  How is supposed to work?
>>
>> Am I confused about the order of operations? It's meant to be "continue
>> if hpriv->em_msg_type doesn't have EM_MSG_TYPE_LED set".
> 
> Shouldn't that be
> 
> 	if (!(hpriv->em_msg_type & EM_MSG_TYPE_LED))
> 
> ! has higher priority than &.  You're converting em_msg_type to 1 or 0
>   and then and'ing EM_MSG_TYPE_LED to it.

I'll fix it then.

--Mark Langsdorf
Calxeda, Inc.

[PATCH 2/2] sata highbank: add bit-banged SGPIO driver support

[Mark Langsdorf]

Highbank supports SGPIO by bit-banging out the SGPIO signals over
three GPIO pins defined in the DTB. Add support for this SGPIO
functionality.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
Changes from v2:
	Added plat_data to ahci_host_priv
	Moved driver specific data to plat_data and got rid of ecx_host_priv
	Corrected order of operations in ecx_transmit_led_message
	Standardized bit operations in ecx_parse_sgpio
Changes from v1:
	Moved all global variables to a private structure
	Replaced all magic numbers with defined symbols
	Added some comments
	Removed the default ordering of ports to LEDs
	Fixed several bugs in the code to read led-order
	Made highbank_set_em_messages static

 .../devicetree/bindings/ata/ahci-platform.txt      |   5 +
 arch/arm/boot/dts/ecx-common.dtsi                  |   2 +
 drivers/ata/ahci.h                                 |   1 +
 drivers/ata/sata_highbank.c                        | 161 ++++++++++++++++++++-
 4 files changed, 163 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index b519f9b..3ec0c5c 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -12,6 +12,11 @@ Optional properties:
 - calxeda,port-phys: phandle-combophy and lane assignment, which maps each
 			SATA port to a combophy and a lane within that
 			combophy
+- calxeda,sgpio-gpio: phandle-gpio bank, bit offset, and default on or off,
+			which indicates that the driver supports SGPIO
+			indicator lights using the indicated GPIOs
+- calxeda,led-order : a u32 array that map port numbers to offsets within the
+			SGPIO bitstream.
 - dma-coherent      : Present if dma operations are coherent
 
 Example:
diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index d61b535..e8559b7 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -33,6 +33,8 @@
 			calxeda,port-phys = <&combophy5 0 &combophy0 0
 					     &combophy0 1 &combophy0 2
 					     &combophy0 3>;
+			calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
+			calxeda,led-order = <4 0 1 2 3>;
 		};
 
 		sdhci@ffe0e000 {
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 10b14d4..6144b93 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -321,6 +321,7 @@ struct ahci_host_priv {
 	u32			em_buf_sz;	/* EM buffer size in byte */
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clk;		/* Only for platforms supporting clk */
+	void			*plat_data;	/* Other platform data */
 };
 
 extern int ahci_ignore_sss;
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index b20aa96..5b13f12 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -33,6 +33,9 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
 #include "ahci.h"
 
 #define CPHY_MAP(dev, addr) ((((dev) & 0x1f) << 7) | (((addr) >> 9) & 0x7f))
@@ -66,6 +69,146 @@ struct phy_lane_info {
 };
 static struct phy_lane_info port_data[CPHY_PORT_COUNT];
 
+static DEFINE_SPINLOCK(sgpio_lock);
+#define SCLOCK				0
+#define SLOAD				1
+#define SDATA				2
+#define SGPIO_PINS			3
+#define SGPIO_PORTS			8
+
+/* can be cast as an ahci_host_priv for compatibility with most functions */
+struct ecx_plat_data {
+	u32		n_ports;
+	unsigned	sgpio_gpio[SGPIO_PINS];
+	u32		sgpio_pattern;
+	u32		port_to_sgpio[SGPIO_PORTS];
+};
+
+#define SGPIO_SIGNALS			3
+#define ECX_ACTIVITY_BITS		0x300000
+#define ECX_ACTIVITY_SHIFT		2
+#define ECX_LOCATE_BITS			0x80000
+#define ECX_LOCATE_SHIFT		1
+#define ECX_FAULT_BITS			0x400000
+#define ECX_FAULT_SHIFT			0
+static inline int sgpio_bit_shift(struct ecx_plat_data *pdata, u32 port,
+				u32 shift)
+{
+	return 1 << (3 * pdata->port_to_sgpio[port] + shift);
+}
+
+static void ecx_parse_sgpio(struct ecx_plat_data *pdata, u32 port, u32 state)
+{
+	if (state & ECX_ACTIVITY_BITS)
+		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
+						ECX_ACTIVITY_SHIFT);
+	else
+		pdata->sgpio_pattern &= sgpio_bit_shift(pdata, port,
+						ECX_ACTIVITY_SHIFT);
+	if (state & ECX_LOCATE_BITS)
+		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
+						ECX_LOCATE_SHIFT);
+	else
+		pdata->sgpio_pattern &= sgpio_bit_shift(pdata, port,
+						ECX_LOCATE_SHIFT);
+	if (state & ECX_FAULT_BITS)
+		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
+						ECX_FAULT_SHIFT);
+	else
+		pdata->sgpio_pattern &= sgpio_bit_shift(pdata, port,
+						ECX_FAULT_SHIFT);
+}
+
+/*
+ * Tell the LED controller that the signal has changed by raising the clock
+ * line for 50 uS and then lowering it for 50 uS.
+ */
+static void ecx_led_cycle_clock(struct ecx_plat_data *pdata)
+{
+	gpio_set_value(pdata->sgpio_gpio[SCLOCK], 1);
+	udelay(50);
+	gpio_set_value(pdata->sgpio_gpio[SCLOCK], 0);
+	udelay(50);
+}
+
+static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
+					ssize_t size)
+{
+	struct ahci_host_priv *hpriv =  ap->host->private_data;
+	struct ecx_plat_data *pdata = (struct ecx_plat_data *) hpriv->plat_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	unsigned long flags;
+	int pmp, i;
+	struct ahci_em_priv *emp;
+	u32 sgpio_out;
+
+	/* get the slot number from the message */
+	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
+	if (pmp < EM_MAX_SLOTS)
+		emp = &pp->em_priv[pmp];
+	else
+		return -EINVAL;
+
+	if (!(hpriv->em_msg_type & EM_MSG_TYPE_LED))
+		return size;
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+	ecx_parse_sgpio(pdata, ap->port_no, state);
+	sgpio_out = pdata->sgpio_pattern;
+	gpio_set_value(pdata->sgpio_gpio[SLOAD], 1);
+	ecx_led_cycle_clock(pdata);
+	gpio_set_value(pdata->sgpio_gpio[SLOAD], 0);
+	/*
+	 * bit-bang out the SGPIO pattern, by consuming a bit and then
+	 * clocking it out.
+	 */
+	for (i = 0; i < (SGPIO_SIGNALS * pdata->n_ports); i++) {
+		gpio_set_value(pdata->sgpio_gpio[SDATA], sgpio_out & 1);
+		sgpio_out >>= 1;
+		ecx_led_cycle_clock(pdata);
+	}
+
+	/* save off new led state for port/slot */
+	emp->led_state = state;
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+	return size;
+}
+
+static void highbank_set_em_messages(struct device *dev,
+					struct ahci_host_priv *hpriv,
+					struct ata_port_info *pi)
+{
+	struct device_node *np = dev->of_node;
+	struct ecx_plat_data *pdata = hpriv->plat_data;
+	int i;
+	int err;
+
+	for (i = 0; i < SGPIO_PINS; i++) {
+		err = of_get_named_gpio(np, "calxeda,sgpio-gpio", i);
+		if (IS_ERR_VALUE(err))
+			return;
+
+		pdata->sgpio_gpio[i] = err;
+		err = gpio_request(pdata->sgpio_gpio[i], "CX SGPIO");
+		if (err) {
+			pr_err("sata_highbank gpio_request %d failed: %d\n",
+					i, err);
+			return;
+		}
+		gpio_direction_output(pdata->sgpio_gpio[i], 1);
+	}
+	of_property_read_u32_array(np, "calxeda,led-order",
+						pdata->port_to_sgpio,
+						pdata->n_ports);
+
+	/* store em_loc */
+	hpriv->em_loc = 0;
+	hpriv->em_buf_sz = 4;
+	hpriv->em_msg_type = EM_MSG_TYPE_LED;
+	pi->flags |= ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY;
+}
+
 static u32 __combo_phy_reg_read(u8 sata_port, u32 addr)
 {
 	u32 data;
@@ -241,6 +384,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 static struct ata_port_operations ahci_highbank_ops = {
 	.inherits		= &ahci_ops,
 	.hardreset		= ahci_highbank_hardreset,
+	.transmit_led_message   = ecx_transmit_led_message,
 };
 
 static const struct ata_port_info ahci_highbank_port_info = {
@@ -264,12 +408,13 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
+	struct ecx_plat_data *pdata;
 	struct ata_host *host;
 	struct resource *mem;
 	int irq;
-	int n_ports;
 	int i;
 	int rc;
+	u32 n_ports;
 	struct ata_port_info pi = ahci_highbank_port_info;
 	const struct ata_port_info *ppi[] = { &pi, NULL };
 
@@ -290,6 +435,11 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 		dev_err(dev, "can't alloc ahci_host_priv\n");
 		return -ENOMEM;
 	}
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "can't alloc ecx_plat_data\n");
+		return -ENOMEM;
+	}
 
 	hpriv->flags |= (unsigned long)pi.private_data;
 
@@ -313,8 +463,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (hpriv->cap & HOST_CAP_PMP)
 		pi.flags |= ATA_FLAG_PMP;
 
-	ahci_set_em_messages(hpriv, &pi);
-
 	/* CAP.NP sometimes indicate the index of the last enabled
 	 * port, at other times, that of the last possible port, so
 	 * determining the maximum port number requires looking at
@@ -322,6 +470,10 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	 */
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
+	pdata->n_ports = n_ports;
+	hpriv->plat_data = pdata;
+	highbank_set_em_messages(dev, hpriv, &pi);
+
 	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
 	if (!host) {
 		rc = -ENOMEM;
@@ -333,9 +485,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
 		host->flags |= ATA_HOST_PARALLEL_SCAN;
 
-	if (pi.flags & ATA_FLAG_EM)
-		ahci_reset_em(host);
-
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-- 
1.8.1.4

Re: [PATCH 2/2] sata highbank: add bit-banged SGPIO driver support

[Tejun Heo]

Hello,

On Wed, Jun 05, 2013 at 05:31:48PM -0500, Mark Langsdorf wrote:
> +static void ecx_parse_sgpio(struct ecx_plat_data *pdata, u32 port, u32 state)
> +{
> +	if (state & ECX_ACTIVITY_BITS)
> +		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
> +						ECX_ACTIVITY_SHIFT);
> +	else
> +		pdata->sgpio_pattern &= sgpio_bit_shift(pdata, port,
> +						ECX_ACTIVITY_SHIFT);

Shouldn't it be "&= ~" instead of "&="?  You're trying to turn the bit
off, right?

Looks okay to me otherwise.

Thanks.

-- 
tejun

Re: [PATCH 2/2] sata highbank: add bit-banged SGPIO driver support

[Mark Langsdorf]

,On 06/05/2013 06:15 PM, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jun 05, 2013 at 05:31:48PM -0500, Mark Langsdorf wrote:
>> +static void ecx_parse_sgpio(struct ecx_plat_data *pdata, u32 port, u32 state)
>> +{
>> +	if (state & ECX_ACTIVITY_BITS)
>> +		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
>> +						ECX_ACTIVITY_SHIFT);
>> +	else
>> +		pdata->sgpio_pattern &= sgpio_bit_shift(pdata, port,
>> +						ECX_ACTIVITY_SHIFT);
> 
> Shouldn't it be "&= ~" instead of "&="?  You're trying to turn the bit
> off, right?

Oops. Thanks!

--Mark Langsdorf
Calxeda, Inc.

[PATCH 2/2 v4] sata highbank: add bit-banged SGPIO driver support

[Mark Langsdorf]

Highbank supports SGPIO by bit-banging out the SGPIO signals over
three GPIO pins defined in the DTB. Add support for this SGPIO
functionality.

Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>
---
Changes from v3:
	Correctly mask the activity bits to clear bits in ecx_parse_sgpio()
Changes from v2:
	Added plat_data to ahci_host_priv
	Moved driver specific data to plat_data and got rid of ecx_host_priv
	Corrected order of operations in ecx_transmit_led_message
	Standardized bit operations in ecx_parse_sgpio
Changes from v1:
	Moved all global variables to a private structure
	Replaced all magic numbers with defined symbols
	Added some comments
	Removed the default ordering of ports to LEDs
	Fixed several bugs in the code to read led-order
	Made highbank_set_em_messages static

 .../devicetree/bindings/ata/ahci-platform.txt      |   5 +
 arch/arm/boot/dts/ecx-common.dtsi                  |   2 +
 drivers/ata/ahci.h                                 |   1 +
 drivers/ata/sata_highbank.c                        | 161 ++++++++++++++++++++-
 4 files changed, 163 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/ata/ahci-platform.txt b/Documentation/devicetree/bindings/ata/ahci-platform.txt
index b519f9b..3ec0c5c 100644
--- a/Documentation/devicetree/bindings/ata/ahci-platform.txt
+++ b/Documentation/devicetree/bindings/ata/ahci-platform.txt
@@ -12,6 +12,11 @@ Optional properties:
 - calxeda,port-phys: phandle-combophy and lane assignment, which maps each
 			SATA port to a combophy and a lane within that
 			combophy
+- calxeda,sgpio-gpio: phandle-gpio bank, bit offset, and default on or off,
+			which indicates that the driver supports SGPIO
+			indicator lights using the indicated GPIOs
+- calxeda,led-order : a u32 array that map port numbers to offsets within the
+			SGPIO bitstream.
 - dma-coherent      : Present if dma operations are coherent
 
 Example:
diff --git a/arch/arm/boot/dts/ecx-common.dtsi b/arch/arm/boot/dts/ecx-common.dtsi
index d61b535..e8559b7 100644
--- a/arch/arm/boot/dts/ecx-common.dtsi
+++ b/arch/arm/boot/dts/ecx-common.dtsi
@@ -33,6 +33,8 @@
 			calxeda,port-phys = <&combophy5 0 &combophy0 0
 					     &combophy0 1 &combophy0 2
 					     &combophy0 3>;
+			calxeda,sgpio-gpio =<&gpioh 5 1 &gpioh 6 1 &gpioh 7 1>;
+			calxeda,led-order = <4 0 1 2 3>;
 		};
 
 		sdhci@ffe0e000 {
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 10b14d4..6144b93 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -321,6 +321,7 @@ struct ahci_host_priv {
 	u32			em_buf_sz;	/* EM buffer size in byte */
 	u32			em_msg_type;	/* EM message type */
 	struct clk		*clk;		/* Only for platforms supporting clk */
+	void			*plat_data;	/* Other platform data */
 };
 
 extern int ahci_ignore_sss;
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index b20aa96..8de8ac8 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -33,6 +33,9 @@
 #include <linux/interrupt.h>
 #include <linux/delay.h>
 #include <linux/export.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+
 #include "ahci.h"
 
 #define CPHY_MAP(dev, addr) ((((dev) & 0x1f) << 7) | (((addr) >> 9) & 0x7f))
@@ -66,6 +69,146 @@ struct phy_lane_info {
 };
 static struct phy_lane_info port_data[CPHY_PORT_COUNT];
 
+static DEFINE_SPINLOCK(sgpio_lock);
+#define SCLOCK				0
+#define SLOAD				1
+#define SDATA				2
+#define SGPIO_PINS			3
+#define SGPIO_PORTS			8
+
+/* can be cast as an ahci_host_priv for compatibility with most functions */
+struct ecx_plat_data {
+	u32		n_ports;
+	unsigned	sgpio_gpio[SGPIO_PINS];
+	u32		sgpio_pattern;
+	u32		port_to_sgpio[SGPIO_PORTS];
+};
+
+#define SGPIO_SIGNALS			3
+#define ECX_ACTIVITY_BITS		0x300000
+#define ECX_ACTIVITY_SHIFT		2
+#define ECX_LOCATE_BITS			0x80000
+#define ECX_LOCATE_SHIFT		1
+#define ECX_FAULT_BITS			0x400000
+#define ECX_FAULT_SHIFT			0
+static inline int sgpio_bit_shift(struct ecx_plat_data *pdata, u32 port,
+				u32 shift)
+{
+	return 1 << (3 * pdata->port_to_sgpio[port] + shift);
+}
+
+static void ecx_parse_sgpio(struct ecx_plat_data *pdata, u32 port, u32 state)
+{
+	if (state & ECX_ACTIVITY_BITS)
+		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
+						ECX_ACTIVITY_SHIFT);
+	else
+		pdata->sgpio_pattern &= ~sgpio_bit_shift(pdata, port,
+						ECX_ACTIVITY_SHIFT);
+	if (state & ECX_LOCATE_BITS)
+		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
+						ECX_LOCATE_SHIFT);
+	else
+		pdata->sgpio_pattern &= ~sgpio_bit_shift(pdata, port,
+						ECX_LOCATE_SHIFT);
+	if (state & ECX_FAULT_BITS)
+		pdata->sgpio_pattern |= sgpio_bit_shift(pdata, port,
+						ECX_FAULT_SHIFT);
+	else
+		pdata->sgpio_pattern &= ~sgpio_bit_shift(pdata, port,
+						ECX_FAULT_SHIFT);
+}
+
+/*
+ * Tell the LED controller that the signal has changed by raising the clock
+ * line for 50 uS and then lowering it for 50 uS.
+ */
+static void ecx_led_cycle_clock(struct ecx_plat_data *pdata)
+{
+	gpio_set_value(pdata->sgpio_gpio[SCLOCK], 1);
+	udelay(50);
+	gpio_set_value(pdata->sgpio_gpio[SCLOCK], 0);
+	udelay(50);
+}
+
+static ssize_t ecx_transmit_led_message(struct ata_port *ap, u32 state,
+					ssize_t size)
+{
+	struct ahci_host_priv *hpriv =  ap->host->private_data;
+	struct ecx_plat_data *pdata = (struct ecx_plat_data *) hpriv->plat_data;
+	struct ahci_port_priv *pp = ap->private_data;
+	unsigned long flags;
+	int pmp, i;
+	struct ahci_em_priv *emp;
+	u32 sgpio_out;
+
+	/* get the slot number from the message */
+	pmp = (state & EM_MSG_LED_PMP_SLOT) >> 8;
+	if (pmp < EM_MAX_SLOTS)
+		emp = &pp->em_priv[pmp];
+	else
+		return -EINVAL;
+
+	if (!(hpriv->em_msg_type & EM_MSG_TYPE_LED))
+		return size;
+
+	spin_lock_irqsave(&sgpio_lock, flags);
+	ecx_parse_sgpio(pdata, ap->port_no, state);
+	sgpio_out = pdata->sgpio_pattern;
+	gpio_set_value(pdata->sgpio_gpio[SLOAD], 1);
+	ecx_led_cycle_clock(pdata);
+	gpio_set_value(pdata->sgpio_gpio[SLOAD], 0);
+	/*
+	 * bit-bang out the SGPIO pattern, by consuming a bit and then
+	 * clocking it out.
+	 */
+	for (i = 0; i < (SGPIO_SIGNALS * pdata->n_ports); i++) {
+		gpio_set_value(pdata->sgpio_gpio[SDATA], sgpio_out & 1);
+		sgpio_out >>= 1;
+		ecx_led_cycle_clock(pdata);
+	}
+
+	/* save off new led state for port/slot */
+	emp->led_state = state;
+
+	spin_unlock_irqrestore(&sgpio_lock, flags);
+	return size;
+}
+
+static void highbank_set_em_messages(struct device *dev,
+					struct ahci_host_priv *hpriv,
+					struct ata_port_info *pi)
+{
+	struct device_node *np = dev->of_node;
+	struct ecx_plat_data *pdata = hpriv->plat_data;
+	int i;
+	int err;
+
+	for (i = 0; i < SGPIO_PINS; i++) {
+		err = of_get_named_gpio(np, "calxeda,sgpio-gpio", i);
+		if (IS_ERR_VALUE(err))
+			return;
+
+		pdata->sgpio_gpio[i] = err;
+		err = gpio_request(pdata->sgpio_gpio[i], "CX SGPIO");
+		if (err) {
+			pr_err("sata_highbank gpio_request %d failed: %d\n",
+					i, err);
+			return;
+		}
+		gpio_direction_output(pdata->sgpio_gpio[i], 1);
+	}
+	of_property_read_u32_array(np, "calxeda,led-order",
+						pdata->port_to_sgpio,
+						pdata->n_ports);
+
+	/* store em_loc */
+	hpriv->em_loc = 0;
+	hpriv->em_buf_sz = 4;
+	hpriv->em_msg_type = EM_MSG_TYPE_LED;
+	pi->flags |= ATA_FLAG_EM | ATA_FLAG_SW_ACTIVITY;
+}
+
 static u32 __combo_phy_reg_read(u8 sata_port, u32 addr)
 {
 	u32 data;
@@ -241,6 +384,7 @@ static int ahci_highbank_hardreset(struct ata_link *link, unsigned int *class,
 static struct ata_port_operations ahci_highbank_ops = {
 	.inherits		= &ahci_ops,
 	.hardreset		= ahci_highbank_hardreset,
+	.transmit_led_message   = ecx_transmit_led_message,
 };
 
 static const struct ata_port_info ahci_highbank_port_info = {
@@ -264,12 +408,13 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct ahci_host_priv *hpriv;
+	struct ecx_plat_data *pdata;
 	struct ata_host *host;
 	struct resource *mem;
 	int irq;
-	int n_ports;
 	int i;
 	int rc;
+	u32 n_ports;
 	struct ata_port_info pi = ahci_highbank_port_info;
 	const struct ata_port_info *ppi[] = { &pi, NULL };
 
@@ -290,6 +435,11 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 		dev_err(dev, "can't alloc ahci_host_priv\n");
 		return -ENOMEM;
 	}
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "can't alloc ecx_plat_data\n");
+		return -ENOMEM;
+	}
 
 	hpriv->flags |= (unsigned long)pi.private_data;
 
@@ -313,8 +463,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (hpriv->cap & HOST_CAP_PMP)
 		pi.flags |= ATA_FLAG_PMP;
 
-	ahci_set_em_messages(hpriv, &pi);
-
 	/* CAP.NP sometimes indicate the index of the last enabled
 	 * port, at other times, that of the last possible port, so
 	 * determining the maximum port number requires looking at
@@ -322,6 +470,10 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	 */
 	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
 
+	pdata->n_ports = n_ports;
+	hpriv->plat_data = pdata;
+	highbank_set_em_messages(dev, hpriv, &pi);
+
 	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
 	if (!host) {
 		rc = -ENOMEM;
@@ -333,9 +485,6 @@ static int ahci_highbank_probe(struct platform_device *pdev)
 	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
 		host->flags |= ATA_HOST_PARALLEL_SCAN;
 
-	if (pi.flags & ATA_FLAG_EM)
-		ahci_reset_em(host);
-
 	for (i = 0; i < host->n_ports; i++) {
 		struct ata_port *ap = host->ports[i];
 
-- 
1.8.1.4

Re: [PATCH 2/2 v4] sata highbank: add bit-banged SGPIO driver support

[Tejun Heo]

On Thu, Jun 06, 2013 at 07:52:41AM -0500, Mark Langsdorf wrote:
> Highbank supports SGPIO by bit-banging out the SGPIO signals over
> three GPIO pins defined in the DTB. Add support for this SGPIO
> functionality.
> 
> Signed-off-by: Mark Langsdorf <mark.langsdorf@calxeda.com>

Applied to libata/for-3.11.  Thanks!

-- 
tejun