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

login
register
mail settings
Submitter Mark Langsdorf
Date June 3, 2013, 1:35 p.m.
Message ID <1370266538-14211-1-git-send-email-mark.langsdorf@calxeda.com>
Download mbox | patch
Permalink /patch/248269/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Mark Langsdorf - June 3, 2013, 1:35 p.m.
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(-)
Tejun Heo - June 3, 2013, 8:37 p.m.
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? :(
Mark Langsdorf - June 4, 2013, 3:09 p.m.
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.


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - June 4, 2013, 8:32 p.m.
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.
Mark Langsdorf - June 5, 2013, 8:19 p.m.
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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];