| Message ID | 1370266538-14211-1-git-send-email-mark.langsdorf@calxeda.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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? :(
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
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.
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
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];
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(-)