[PATCH,v2][0/2] sata_mv: harddisk activity led no longer responsive on QNAP TS-109

[PATCH,v2][0/2] sata_mv: harddisk activity led no longer responsive on QNAP TS-109

[Frans Pop]

On Tuesday 10 March 2009, Mark Lord wrote:
> So if you want to rework the first patch so that the blink fix
> works for all GEN_IIE chips, then I'll test it here with a few
> different drives from different vendors and see how the LEDs behave.

Here's version 2 of the patches.

Changes:
- make the quirk valid for all GenIIe chips and not only for SoC
- use a flag to indicate a particular chip needs the quirk
- use a hardware op to enable/disable blink mode

Open issues:
- as I've worked without any specs and only have SoC to test on, please
  check that mv_iie_enable_led_blink is correct for all GenIIe chips
- if mv_iie_enable_led_blink is also valid for GenII chips, the
  first patch can be simplified a bit (no need for mv6xxx_iie_ops)

To test:
* load module with ncq_blink_led=0 (default)
  - check if NCQ is enabled (/sys/block/sda/device/queue_depth > 1)
    if not -> chip does not support NCQ and is thus not affected?
  - cause some disk activity; if led stays on without blinking, the
    chip needs the quirk
  - disable NCQ (echo 1 >/sys/block/sda/device/queue_depth)
  - cause some disk activity; led blinking should be very responsive
    (pre-2.6.26 behavior on my QNAP TS-109)
* load module with ncq_blink_led=1
  - check if NCQ is enabled (/sys/block/sda/device/queue_depth > 1)
  - cause some disk activity; led should now blink in a lazy frequency
    (quirk enabled)
  - disable NCQ (echo 1 >/sys/block/sda/device/queue_depth)
  - cause some disk activity; led blinking should be very responsive
    again (pre-2.6.26 behavior)
  - re-enable NCQ (echo 31 >/sys/block/sda/device/queue_depth)
  - cause some disk activity; led blinking should be lazy again

Cheers,
FJP

[PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

For some Marvell chips the HDD led does not blink when there is
disk I/O if NCQ is enabled. Add a quirk that enables blink mode for
the led when NCQ is used on any of the ports of a host controller.

The code to enable the blink mode is based on an earlier patch
proposed by Saeed Bishara.

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Mark Lord <liml@rtr.ca>
Cc: Saeed Bishara <saeed.bishara@gmail.com>

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 7007edd..4057647 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -205,6 +205,11 @@ enum {
 	HC_COAL_IRQ		= (1 << 4),	/* IRQ coalescing */
 	DEV_IRQ			= (1 << 8),	/* shift by port # */
 
+	IIE_LED_CTRL_OFS	= 0x2c,
+	IIE_LED_CTRL_BLINK	= (1 << 0),	/* Active LED blink */
+	IIE_LED_CTRL_ACT_PRESENCE = (1 << 2),	/* Multiplex presence with */
+						/* the active LED */
+
 	/* Shadow block registers */
 	SHD_BLK_OFS		= 0x100,
 	SHD_CTL_AST_OFS		= 0x20,		/* ofs from SHD_BLK_OFS */
@@ -359,6 +364,8 @@ enum {
 	MV_HP_PCIE		= (1 << 9),	/* PCIe bus/regs: 7042 */
 	MV_HP_CUT_THROUGH	= (1 << 10),	/* can use EDMA cut-through */
 	MV_HP_FLAG_SOC		= (1 << 11),	/* SystemOnChip, no PCI */
+	MV_HP_NCQ_LED_QUIRK	= (1 << 12),
+	MV_HP_LED_BL_EN		= (1 << 13),	/* is led blinking enabled? */
 
 	/* Port private flags (pp_flags) */
 	MV_PP_FLAG_EDMA_EN	= (1 << 0),	/* is EDMA engine enabled? */
@@ -480,11 +487,19 @@ struct mv_hw_ops {
 			   unsigned int port);
 	void (*enable_leds)(struct mv_host_priv *hpriv, void __iomem *mmio);
 	void (*read_preamp)(struct mv_host_priv *hpriv, int idx,
-			   void __iomem *mmio);
+			    void __iomem *mmio);
 	int (*reset_hc)(struct mv_host_priv *hpriv, void __iomem *mmio,
 			unsigned int n_hc);
 	void (*reset_flash)(struct mv_host_priv *hpriv, void __iomem *mmio);
 	void (*reset_bus)(struct ata_host *host, void __iomem *mmio);
+	void (*enable_led_blink)(struct mv_host_priv *hpriv,
+				 void __iomem *mmio, int enable_blink);
+
+	/*
+	 * ->inherits must be the last field and all the preceding
+	 * fields must be pointers.
+	 */
+	const struct mv_hw_ops	*inherits;
 };
 
 static int mv_scr_read(struct ata_link *link, unsigned int sc_reg_in, u32 *val);
@@ -530,6 +545,8 @@ static int mv_soc_reset_hc(struct mv_host_priv *hpriv,
 static void mv_soc_reset_flash(struct mv_host_priv *hpriv,
 				      void __iomem *mmio);
 static void mv_soc_reset_bus(struct ata_host *host, void __iomem *mmio);
+static void mv_iie_enable_led_blink(struct mv_host_priv *hpriv,
+					void __iomem *mmio, int enable_blink);
 static void mv_reset_pci_bus(struct ata_host *host, void __iomem *mmio);
 static void mv_reset_channel(struct mv_host_priv *hpriv, void __iomem *mmio,
 			     unsigned int port_no);
@@ -705,6 +722,11 @@ static const struct mv_hw_ops mv6xxx_ops = {
 	.reset_bus		= mv_reset_pci_bus,
 };
 
+static const struct mv_hw_ops mv6xxx_iie_ops = {
+	.inherits		= &mv6xxx_ops,
+	.enable_led_blink	= mv_iie_enable_led_blink,
+};
+
 static const struct mv_hw_ops mv_soc_ops = {
 	.phy_errata		= mv6_phy_errata,
 	.enable_leds		= mv_soc_enable_leds,
@@ -712,6 +734,7 @@ static const struct mv_hw_ops mv_soc_ops = {
 	.reset_hc		= mv_soc_reset_hc,
 	.reset_flash		= mv_soc_reset_flash,
 	.reset_bus		= mv_soc_reset_bus,
+	.enable_led_blink	= mv_iie_enable_led_blink,
 };
 
 /*
@@ -852,6 +875,55 @@ static void mv_enable_port_irqs(struct ata_port *ap,
 	mv_set_main_irq_mask(ap->host, disable_bits, enable_bits);
 }
 
+static int mv_has_port_using_ncq(struct ata_host *host)
+{
+	int port;
+	struct mv_host_priv *hpriv = host->private_data;
+
+	for (port = 0; port < hpriv->n_ports; port++) {
+		struct ata_port *ap = host->ports[port];
+		struct mv_port_priv *pp = ap->private_data;
+
+		if ((pp->pp_flags & MV_PP_FLAG_EDMA_EN) &&
+		    (pp->pp_flags & MV_PP_FLAG_NCQ_EN))
+			return 1;
+	}
+
+	return 0;
+}
+
+/**
+ * Some chips have an erratum which causes the HDD led not to blink
+ * during I/O when NCQ is enabled. Enabling the blink mode of the
+ * led makes activity visible in that case.
+ */
+static void mv_quirk_blink_led_when_ncq(struct ata_port *ap,
+					int enable)
+{
+	struct mv_host_priv *hpriv = ap->host->private_data;
+	void __iomem *mmio = hpriv->base;
+
+	if (!hpriv->ops->enable_led_blink)
+		return;
+
+	if (enable) {
+		if (!(hpriv->hp_flags & MV_HP_LED_BL_EN)) {
+			hpriv->ops->enable_led_blink(hpriv, mmio, true);
+			hpriv->hp_flags |= MV_HP_LED_BL_EN;
+		}
+		return;
+	} else {
+		if (!(hpriv->hp_flags & MV_HP_LED_BL_EN))
+			return;
+	}
+
+	/* Does any other port still use NCQ? */
+	if (!mv_has_port_using_ncq(ap->host)) {
+		hpriv->ops->enable_led_blink(hpriv, mmio, false);
+		hpriv->hp_flags &= ~MV_HP_LED_BL_EN;
+	}
+}
+
 /**
  *      mv_start_dma - Enable eDMA engine
  *      @base: port base address
@@ -952,6 +1024,7 @@ static int mv_stop_edma(struct ata_port *ap)
 {
 	void __iomem *port_mmio = mv_ap_base(ap);
 	struct mv_port_priv *pp = ap->private_data;
+	struct mv_host_priv *hpriv = ap->host->private_data;
 
 	if (!(pp->pp_flags & MV_PP_FLAG_EDMA_EN))
 		return 0;
@@ -961,6 +1034,10 @@ static int mv_stop_edma(struct ata_port *ap)
 		ata_port_printk(ap, KERN_ERR, "Unable to stop eDMA\n");
 		return -EIO;
 	}
+
+	if (hpriv->hp_flags & MV_HP_NCQ_LED_QUIRK)
+		mv_quirk_blink_led_when_ncq(ap, false);
+
 	return 0;
 }
 
@@ -1222,6 +1299,9 @@ static void mv_edma_cfg(struct ata_port *ap, int want_ncq)
 	}
 
 	if (want_ncq) {
+		if (hpriv->hp_flags & MV_HP_NCQ_LED_QUIRK)
+			mv_quirk_blink_led_when_ncq(ap, true);
+
 		cfg |= EDMA_CFG_NCQ;
 		pp->pp_flags |=  MV_PP_FLAG_NCQ_EN;
 	} else
@@ -2690,6 +2770,19 @@ static void mv_soc_reset_bus(struct ata_host *host, void __iomem *mmio)
 	return;
 }
 
+static void mv_iie_enable_led_blink(struct mv_host_priv *hpriv,
+				      void __iomem *mmio, int enable_blink)
+{
+	void __iomem *hc_mmio = mv_hc_base(mmio, 0);
+	u32 tmp = readl(hc_mmio + IIE_LED_CTRL_OFS);
+
+	/* enable/disable blinking mode */
+	if (enable_blink)
+		writel(tmp | IIE_LED_CTRL_BLINK, hc_mmio + IIE_LED_CTRL_OFS);
+	else
+		writel(tmp & ~IIE_LED_CTRL_BLINK, hc_mmio + IIE_LED_CTRL_OFS);
+}
+
 static void mv_setup_ifcfg(void __iomem *port_mmio, int want_gen2i)
 {
 	u32 ifcfg = readl(port_mmio + SATA_INTERFACE_CFG_OFS);
@@ -2997,8 +3090,8 @@ static int mv_chip_id(struct ata_host *host, unsigned int board_idx)
 		}
 		/* drop through */
 	case chip_6042:
-		hpriv->ops = &mv6xxx_ops;
-		hp_flags |= MV_HP_GEN_IIE;
+		hpriv->ops = &mv6xxx_iie_ops;
+		hp_flags |= MV_HP_GEN_IIE | MV_HP_NCQ_LED_QUIRK;
 		if (board_idx == chip_6042 && mv_pci_cut_through_okay(host))
 			hp_flags |= MV_HP_CUT_THROUGH;
 
@@ -3016,7 +3109,7 @@ static int mv_chip_id(struct ata_host *host, unsigned int board_idx)
 	case chip_soc:
 		hpriv->ops = &mv_soc_ops;
 		hp_flags |= MV_HP_FLAG_SOC | MV_HP_GEN_IIE |
-			MV_HP_ERRATA_60X1C0;
+			MV_HP_ERRATA_60X1C0 | MV_HP_NCQ_LED_QUIRK;
 		break;
 
 	default:

[PATCH,v2][2/2] sata-mv: add module parameter msq_blink_led to enable quirk for GenIIe

[Frans Pop]

For some Marvell chips the HDD led does not blink when there is disk
I/O if NCQ is enabled. Only enable the quirk that works around this
(by enabling blink mode for the led) if parameter msq_blink_led is
set as it is not clear whether this is a general erratum or related
to the type of hard disk connected to the controller.

Signed-off-by: Frans Pop <elendil@planet.nl>
Cc: Mark Lord <liml@rtr.ca>
Cc: Saeed Bishara <saeed.bishara@gmail.com>

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 4057647..86f8047 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -738,6 +738,15 @@ static const struct mv_hw_ops mv_soc_ops = {
 };
 
 /*
+ * module options
+ */
+static int msi;			/* Use PCI msi; either zero (off, default)
+				 * or non-zero */
+static int ncq_blink_led;	/* Use blink mode for HDD activity led when
+				 * NCQ is enabled (GenIIe quirk);
+				 * either zero (off, default) or non-zero */
+
+/*
  * Functions
  */
 
@@ -903,7 +912,7 @@ static void mv_quirk_blink_led_when_ncq(struct 
ata_port *ap,
 	struct mv_host_priv *hpriv = ap->host->private_data;
 	void __iomem *mmio = hpriv->base;
 
-	if (!hpriv->ops->enable_led_blink)
+	if (!ncq_blink_led || !hpriv->ops->enable_led_blink)
 		return;
 
 	if (enable) {
@@ -3379,12 +3388,6 @@ static struct pci_driver mv_pci_driver = {
 	.remove			= ata_pci_remove_one,
 };
 
-/*
- * module options
- */
-static int msi;	      /* Use PCI msi; either zero (off, default) or 
non-zero */
-
-
 /* move to PCI layer or libata core? */
 static int pci_go_64(struct pci_dev *pdev)
 {
@@ -3570,6 +3573,10 @@ MODULE_ALIAS("platform:" DRV_NAME);
 module_param(msi, int, 0444);
 MODULE_PARM_DESC(msi, "Enable use of PCI MSI (0=off, 1=on)");
 #endif
+/* ncq_blink_led only has effect for GenIIe chip variants */
+module_param(ncq_blink_led, int, 0444);
+MODULE_PARM_DESC(ncq_blink_led,
+	"Use blink mode quirk for HDD led when NCQ is enabled (0=off, 1=on)");
 
 module_init(mv_init);
 module_exit(mv_exit);

Re: [PATCH,v2][0/2] sata_mv: harddisk activity led no longer responsive on QNAP TS-109

[Mark Lord]

Frans,

How many HDD activity LEDs does your SoC system have?

Just one, for all channels?
Or several -- one for *each* channel?

Thanks

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Frans Pop wrote:
..
> @@ -480,11 +487,19 @@ struct mv_hw_ops {
>  			   unsigned int port);
>  	void (*enable_leds)(struct mv_host_priv *hpriv, void __iomem *mmio);
>  	void (*read_preamp)(struct mv_host_priv *hpriv, int idx,
> -			   void __iomem *mmio);
> +			    void __iomem *mmio);
>  	int (*reset_hc)(struct mv_host_priv *hpriv, void __iomem *mmio,
>  			unsigned int n_hc);
>  	void (*reset_flash)(struct mv_host_priv *hpriv, void __iomem *mmio);
>  	void (*reset_bus)(struct ata_host *host, void __iomem *mmio);
> +	void (*enable_led_blink)(struct mv_host_priv *hpriv,
> +				 void __iomem *mmio, int enable_blink);
> +
> +	/*
> +	 * ->inherits must be the last field and all the preceding
> +	 * fields must be pointers.
> +	 */
> +	const struct mv_hw_ops	*inherits;
>  };
..

>  
> +static const struct mv_hw_ops mv6xxx_iie_ops = {
> +	.inherits		= &mv6xxx_ops,
> +	.enable_led_blink	= mv_iie_enable_led_blink,
> +};
> +


I'm afraid I just don't understand the purpose of "inherits" above.
This field appears to never be referenced anywhere.

(or did I miss an update to the C programming language at some point?  :)

???

Re: [PATCH,v2][0/2] sata_mv: harddisk activity led no longer responsive on QNAP TS-109

[Mark Lord]

Mark Lord wrote:
> Frans,
> 
> How many HDD activity LEDs does your SoC system have?
> 
> Just one, for all channels?
> Or several -- one for *each* channel?
..

Oh, nevermind, as that doesn't really matter,
because there's only one global "blink mode" enable bit
used for all ports.

-ml

Re: [PATCH,v2][0/2] sata_mv: harddisk activity led no longer responsive on QNAP TS-109

[Frans Pop]

On Wednesday 11 March 2009, Mark Lord wrote:
> How many HDD activity LEDs does your SoC system have?
>
> Just one, for all channels?

Just one as my TS-109 [0] can hold only one disk. Although, it does have a 
separate LED for its eSATA port on the back, but I've never used that.
So the answer could also be 2.

During boot I get:
sata_mv sata_mv.0: version 1.25
sata_mv sata_mv.0: slots 32 ports 2
scsi0 : sata_mv
scsi1 : sata_mv
ata1: SATA max UDMA/133 irq 29
ata2: SATA max UDMA/133 irq 29
ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300)
ata1.00: ATA-7: HDT722516DLA380, V43OA91A, max UDMA/133
ata1.00: 321672960 sectors, multi 0: LBA48 NCQ (depth 31/32)
ata1.00: configured for UDMA/133
scsi 0:0:0:0: Direct-Access   ATA    HDT722516DLA380  V43O PQ: 0 ANSI: 5

I guess scsi0/ata1 is the internal disk and scsi1/ata2 the eSATA?

There are other QNAP variants that can hold multiple disks.
Looking at [1] the TS-209 has 2 LEDs, one for each disk. I cannot make out 
on [2] how many LEDs the TS-409 has.

Cheers,
FJP

[0] http://www.cyrius.com/debian/orion/qnap/ts-109/images/img_0013.jpg
[1] http://www.cyrius.com/debian/orion/qnap/ts-209/images/img_0004.jpg
[2] http://www.cyrius.com/debian/orion/qnap/ts-409/images/img_0201.jpg

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

On Wednesday 11 March 2009, Mark Lord wrote:
> > +static const struct mv_hw_ops mv6xxx_iie_ops = {
> > +	.inherits		= &mv6xxx_ops,
> > +	.enable_led_blink	= mv_iie_enable_led_blink,
> > +};
> > +
>
> I'm afraid I just don't understand the purpose of "inherits" above.
> This field appears to never be referenced anywhere.

Fun, as it's also used in mv6_ops and mv_iie_ops. That's where I copied it 
from. I really don't have anywhere near the C fu to think up such 
constructs all by myself :-D

What it does (I've been assuming) is include any ops defined in the struct 
that is being referred to. So mv6xxx_iie_ops gets all the ops defined for 
mv6xxx_ops plus mv_iie_enable_led_blink.

> (or did I miss an update to the C programming language at some point? 
> :)
>

You're still way ahead of me.

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Frans Pop wrote:
> On Wednesday 11 March 2009, Mark Lord wrote:
>>> +static const struct mv_hw_ops mv6xxx_iie_ops = {
>>> +	.inherits		= &mv6xxx_ops,
>>> +	.enable_led_blink	= mv_iie_enable_led_blink,
>>> +};
>>> +
>> I'm afraid I just don't understand the purpose of "inherits" above.
>> This field appears to never be referenced anywhere.
> 
> Fun, as it's also used in mv6_ops and mv_iie_ops. That's where I copied it 
> from. I really don't have anywhere near the C fu to think up such 
> constructs all by myself :-D
> 
> What it does (I've been assuming) is include any ops defined in the struct 
> that is being referred to. So mv6xxx_iie_ops gets all the ops defined for 
> mv6xxx_ops plus mv_iie_enable_led_blink.
..

Heh.. that's the idea.

Except it is not a C-language feature, but rather a simple/clever
implementation for those specific data structures, as can be seen
in libata-core.c :: ata_finalize_port_ops().

So, skip that.

I've looked through the patches, and through the various Marvell datasheets
that I have access to here.  And it seems that the "LED blink" controls are
in different bits of different registers for the non-SOC chips.

So enough: we'll just do this the original way, for SOC only.
All of the SOC chips that I know about appear to have the correct bits
in the same places, so perhaps that's what Saeed's cryptic commment was about.

Here's my hacked version of your original patch,
against the latest libata-dev#upstream tree + IRQ coalescing patches.

Can you test this there and confirm that it still works for you?
Please try switching NCQ on/off etc.. just to make sure.

After I hear back, I'll submit this for #upstream.

Thanks

--- old/drivers/ata/sata_mv.c	2009-03-11 00:50:48.000000000 -0400
+++ new/drivers/ata/sata_mv.c	2009-03-11 10:13:29.000000000 -0400
@@ -251,6 +251,11 @@
 	HC_IRQ_COAL_IO_THRESHOLD_OFS	= 0x000c,
 	HC_IRQ_COAL_TIME_THRESHOLD_OFS	= 0x0010,
 
+	SOC_LED_CTRL_OFS	= 0x2c,
+	SOC_LED_CTRL_BLINK	= (1 << 0),	/* Active LED blink */
+	SOC_LED_CTRL_ACT_PRESENCE = (1 << 2),	/* Multiplex dev presence */
+						/*  with dev activity LED */
+
 	/* Shadow block registers */
 	SHD_BLK_OFS		= 0x100,
 	SHD_CTL_AST_OFS		= 0x20,		/* ofs from SHD_BLK_OFS */
@@ -411,6 +416,7 @@
 	MV_HP_PCIE		= (1 << 9),	/* PCIe bus/regs: 7042 */
 	MV_HP_CUT_THROUGH	= (1 << 10),	/* can use EDMA cut-through */
 	MV_HP_FLAG_SOC		= (1 << 11),	/* SystemOnChip, no PCI */
+	MV_HP_QUIRK_LED_BLINK_EN = (1 << 12),	/* is led blinking enabled? */
 
 	/* Port private flags (pp_flags) */
 	MV_PP_FLAG_EDMA_EN	= (1 << 0),	/* is EDMA engine enabled? */
@@ -1404,6 +1410,61 @@
 	mv_write_cached_reg(mv_ap_base(ap) + EDMA_UNKNOWN_RSVD_OFS, old, new);
 }
 
+/*
+ * SOC chips have an issue whereby the HDD LEDs don't always blink
+ * during I/O when NCQ is enabled. Enabling a special "LED blink" mode
+ * of the SOC takes care of it, generating a steady blink rate when
+ * any drive on the chip is active.
+ *
+ * Unfortunately, the blink mode is a global hardware setting for the SOC,
+ * so we must use it whenever at least one port on the SOC has NCQ enabled.
+ *
+ * We turn "LED blink" off when NCQ is not in use anywhere, because the normal
+ * LED operation works then, and provides better (more accurate) feedback.
+ *
+ * Note that this code assumes that an SOC never has more than one HC onboard.
+ */
+static void mv_soc_led_blink_enable(struct ata_port *ap)
+{
+	struct ata_host *host = ap->host;
+	struct mv_host_priv *hpriv = host->private_data;
+	void __iomem *hc_mmio;
+	u32 led_ctrl;
+
+	if (hpriv->hp_flags & MV_HP_QUIRK_LED_BLINK_EN)
+		return;
+	hpriv->hp_flags |= MV_HP_QUIRK_LED_BLINK_EN;
+	hc_mmio = mv_hc_base_from_port(mv_host_base(host), ap->port_no);
+	led_ctrl = readl(hc_mmio + SOC_LED_CTRL_OFS);
+	writel(led_ctrl | SOC_LED_CTRL_BLINK, hc_mmio + SOC_LED_CTRL_OFS);
+}
+
+static void mv_soc_led_blink_disable(struct ata_port *ap)
+{
+	struct ata_host *host = ap->host;
+	struct mv_host_priv *hpriv = host->private_data;
+	void __iomem *hc_mmio;
+	u32 led_ctrl;
+	unsigned int port;
+
+	if (!(hpriv->hp_flags & MV_HP_QUIRK_LED_BLINK_EN))
+		return;
+
+	/* disable led-blink only if no ports are using NCQ */
+	for (port = 0; port < hpriv->n_ports; port++) {
+		struct ata_port *this_ap = host->ports[port];
+		struct mv_port_priv *pp = this_ap->private_data;
+
+		if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
+			return;
+	}
+
+	hpriv->hp_flags &= ~MV_HP_QUIRK_LED_BLINK_EN;
+	hc_mmio = mv_hc_base_from_port(mv_host_base(host), ap->port_no);
+	led_ctrl = readl(hc_mmio + SOC_LED_CTRL_OFS);
+	writel(led_ctrl & ~SOC_LED_CTRL_BLINK, hc_mmio + SOC_LED_CTRL_OFS);
+}
+
 static void mv_edma_cfg(struct ata_port *ap, int want_ncq, int want_edma)
 {
 	u32 cfg;
@@ -1451,6 +1512,13 @@
 		if (hpriv->hp_flags & MV_HP_CUT_THROUGH)
 			cfg |= (1 << 17); /* enab cut-thru (dis stor&forwrd) */
 		mv_bmdma_enable_iie(ap, !want_edma);
+
+		if (IS_SOC(hpriv)) {
+			if (want_ncq)
+				mv_soc_led_blink_enable(ap);
+			else
+				mv_soc_led_blink_disable(ap);
+		}
 	}
 
 	if (want_ncq) {

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

On Wednesday 11 March 2009, Mark Lord wrote:
> Frans Pop wrote:
> > On Wednesday 11 March 2009, Mark Lord wrote:
> >> I'm afraid I just don't understand the purpose of "inherits" above.
> >> This field appears to never be referenced anywhere.
> >
> > What it does (I've been assuming) is include any ops defined in the
> > struct that is being referred to. So mv6xxx_iie_ops gets all the ops
> > defined for mv6xxx_ops plus mv_iie_enable_led_blink.
>
> Except it is not a C-language feature, but rather a simple/clever
> implementation for those specific data structures, as can be seen
> in libata-core.c :: ata_finalize_port_ops().
>
> So, skip that.

Oops. So I implemented part of what's needed, but not all. Well, that just 
proves my level of C knowledge. Should have just copied the set of ops 
instead of trying to do it clean :-/

> So enough: we'll just do this the original way, for SOC only.
> All of the SOC chips that I know about appear to have the correct bits
> in the same places, so perhaps that's what Saeed's cryptic commment was
> about.

OK. Fine by me. It can always be extended if the need for that is proven.

> Can you test this there and confirm that it still works for you?
> Please try switching NCQ on/off etc.. just to make sure.

The LED behavior is correct with NCQ enabled: slow blink.
But if I disable NCQ blink mode is not turned off, the led continues the 
slow blinking. So it looks as if just calling mv_soc_led_blink_disable in 
mv_edma_cfg is not sufficient.

> After I hear back, I'll submit this for #upstream.

Thanks Mark.

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Frans Pop wrote:
> On Wednesday 11 March 2009, Mark Lord wrote:
..
>> Can you test this there and confirm that it still works for you?
>> Please try switching NCQ on/off etc.. just to make sure.
> 
> The LED behavior is correct with NCQ enabled: slow blink.
> But if I disable NCQ blink mode is not turned off, the led continues the 
> slow blinking. So it looks as if just calling mv_soc_led_blink_disable in 
> mv_edma_cfg is not sufficient.
..

Exactly, *how*, did you "disable NCQ blink mode".

thanks

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Mark Lord wrote:
> Frans Pop wrote:
>> On Wednesday 11 March 2009, Mark Lord wrote:
> ..
>>> Can you test this there and confirm that it still works for you?
>>> Please try switching NCQ on/off etc.. just to make sure.
>>
>> The LED behavior is correct with NCQ enabled: slow blink.
>> But if I disable NCQ blink mode is not turned off, the led continues 
>> the slow blinking. So it looks as if just calling 
>> mv_soc_led_blink_disable in mv_edma_cfg is not sufficient.
> ..
> 
> Exactly, *how*, did you "disable NCQ blink mode".
..

Or rather, exactly *how* did you "disable NCQ" ?

Thanks

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Mark Lord wrote:
..
> Or rather, exactly *how* did you "disable NCQ" ?
..

And after you answer that, then does this revised patch work?
If not, then perhaps you could try and figure out why,
since I just don't have your hardware available to me here.

Thanks

--- old/drivers/ata/sata_mv.c	2009-03-11 00:50:48.000000000 -0400
+++ new/drivers/ata/sata_mv.c	2009-03-11 10:13:29.000000000 -0400
@@ -251,6 +251,11 @@
 	HC_IRQ_COAL_IO_THRESHOLD_OFS	= 0x000c,
 	HC_IRQ_COAL_TIME_THRESHOLD_OFS	= 0x0010,
 
+	SOC_LED_CTRL_OFS	= 0x2c,
+	SOC_LED_CTRL_BLINK	= (1 << 0),	/* Active LED blink */
+	SOC_LED_CTRL_ACT_PRESENCE = (1 << 2),	/* Multiplex dev presence */
+						/*  with dev activity LED */
+
 	/* Shadow block registers */
 	SHD_BLK_OFS		= 0x100,
 	SHD_CTL_AST_OFS		= 0x20,		/* ofs from SHD_BLK_OFS */
@@ -411,6 +416,7 @@
 	MV_HP_PCIE		= (1 << 9),	/* PCIe bus/regs: 7042 */
 	MV_HP_CUT_THROUGH	= (1 << 10),	/* can use EDMA cut-through */
 	MV_HP_FLAG_SOC		= (1 << 11),	/* SystemOnChip, no PCI */
+	MV_HP_QUIRK_LED_BLINK_EN = (1 << 12),	/* is led blinking enabled? */
 
 	/* Port private flags (pp_flags) */
 	MV_PP_FLAG_EDMA_EN	= (1 << 0),	/* is EDMA engine enabled? */
@@ -1404,6 +1410,62 @@
 	mv_write_cached_reg(mv_ap_base(ap) + EDMA_UNKNOWN_RSVD_OFS, old, new);
 }
 
+/*
+ * SOC chips have an issue whereby the HDD LEDs don't always blink
+ * during I/O when NCQ is enabled. Enabling a special "LED blink" mode
+ * of the SOC takes care of it, generating a steady blink rate when
+ * any drive on the chip is active.
+ *
+ * Unfortunately, the blink mode is a global hardware setting for the SOC,
+ * so we must use it whenever at least one port on the SOC has NCQ enabled.
+ *
+ * We turn "LED blink" off when NCQ is not in use anywhere, because the normal
+ * LED operation works then, and provides better (more accurate) feedback.
+ *
+ * Note that this code assumes that an SOC never has more than one HC onboard.
+ */
+static void mv_soc_led_blink_enable(struct ata_port *ap)
+{
+	struct ata_host *host = ap->host;
+	struct mv_host_priv *hpriv = host->private_data;
+	void __iomem *hc_mmio;
+	u32 led_ctrl;
+
+	if (hpriv->hp_flags & MV_HP_QUIRK_LED_BLINK_EN)
+		return;
+	hpriv->hp_flags |= MV_HP_QUIRK_LED_BLINK_EN;
+	hc_mmio = mv_hc_base_from_port(mv_host_base(host), ap->port_no);
+	led_ctrl = readl(hc_mmio + SOC_LED_CTRL_OFS);
+	writel(led_ctrl | SOC_LED_CTRL_BLINK, hc_mmio + SOC_LED_CTRL_OFS);
+}
+
+static void mv_soc_led_blink_disable(struct ata_port *ap)
+{
+	struct ata_host *host = ap->host;
+	struct mv_host_priv *hpriv = host->private_data;
+	void __iomem *hc_mmio;
+	u32 led_ctrl;
+	unsigned int port;
+
+	if (!(hpriv->hp_flags & MV_HP_QUIRK_LED_BLINK_EN))
+		return;
+
+	/* disable led-blink only if no ports are using NCQ */
+	for (port = 0; port < hpriv->n_ports; port++) {
+		struct ata_port *this_ap = host->ports[port];
+		struct mv_port_priv *pp = this_ap->private_data;
+
+		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
+			if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
+				return;
+	}
+
+	hpriv->hp_flags &= ~MV_HP_QUIRK_LED_BLINK_EN;
+	hc_mmio = mv_hc_base_from_port(mv_host_base(host), ap->port_no);
+	led_ctrl = readl(hc_mmio + SOC_LED_CTRL_OFS);
+	writel(led_ctrl & ~SOC_LED_CTRL_BLINK, hc_mmio + SOC_LED_CTRL_OFS);
+}
+
 static void mv_edma_cfg(struct ata_port *ap, int want_ncq, int want_edma)
 {
 	u32 cfg;
@@ -1451,6 +1513,13 @@
 		if (hpriv->hp_flags & MV_HP_CUT_THROUGH)
 			cfg |= (1 << 17); /* enab cut-thru (dis stor&forwrd) */
 		mv_bmdma_enable_iie(ap, !want_edma);
+
+		if (IS_SOC(hpriv)) {
+			if (want_ncq)
+				mv_soc_led_blink_enable(ap);
+			else
+				mv_soc_led_blink_disable(ap);
+		}
 	}
 
 	if (want_ncq) {

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

On Thursday 12 March 2009, Mark Lord wrote:
> > The LED behavior is correct with NCQ enabled: slow blink.
> > But if I disable NCQ blink mode is not turned off, the led continues
> > the slow blinking. So it looks as if just calling
> > mv_soc_led_blink_disable in mv_edma_cfg is not sufficient.
>
> Exactly, *how*, did you "disable NCQ blink mode".

# echo 1 >/sys/block/sda/device/queue_depth

With my own version of the patch this worked.

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Frans Pop wrote:
> On Thursday 12 March 2009, Mark Lord wrote:
..
>> Exactly, *how*, did you "disable NCQ blink mode".
> 
> # echo 1 >/sys/block/sda/device/queue_depth
..

Okay, good.  Just making sure of things, thanks.

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

On Thursday 12 March 2009, Mark Lord wrote:
> And after you answer that, then does this revised patch work?
> If not, then perhaps you could try and figure out why,
> since I just don't have your hardware available to me here.

I've just tested (on top of .29-rc8) the new development version of the 
driver you sent me privately, which essentially contains this patch, and 
it worked perfectly!

In the final patch, please add my:
Reported-by: Frans Pop <elendil@planet.nl>
Tested-by: Frans Pop <elendil@planet.nl>

Thanks Mark.

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Frans Pop wrote:
> On Thursday 12 March 2009, Mark Lord wrote:
>> And after you answer that, then does this revised patch work?
>> If not, then perhaps you could try and figure out why,
>> since I just don't have your hardware available to me here.
> 
> I've just tested (on top of .29-rc8) the new development version of the 
> driver you sent me privately, which essentially contains this patch, and 
> it worked perfectly!
> 
> In the final patch, please add my:
> Reported-by: Frans Pop <elendil@planet.nl>
> Tested-by: Frans Pop <elendil@planet.nl>
> 
> Thanks Mark.
..

Super.  Could do one more test, perhaps:
Does it also still work (turn off, turn on) with this
slight change to the copy you were testing there?

Thanks!

--- old/drivers/ata/sata_mv.c	2009-03-12 10:23:41.000000000 -0400
+++ new/drivers/ata/sata_mv.c	2009-03-13 09:03:47.000000000 -0400
@@ -1455,9 +1455,8 @@
 		struct ata_port *this_ap = host->ports[port];
 		struct mv_port_priv *pp = this_ap->private_data;
 
-		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
-			if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
-				return;
+		if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
+			return;
 	}
 
 	hpriv->hp_flags &= ~MV_HP_QUIRK_LED_BLINK_EN;

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

On Friday 13 March 2009, Mark Lord wrote:
> --- old/drivers/ata/sata_mv.c	2009-03-12 10:23:41.000000000 -0400
> +++ new/drivers/ata/sata_mv.c	2009-03-13 09:03:47.000000000 -0400
> @@ -1455,9 +1455,8 @@
>  		struct ata_port *this_ap = host->ports[port];
>  		struct mv_port_priv *pp = this_ap->private_data;
>
> -		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
> -			if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
> -				return;
> +		if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
> +			return;

I expect that if NCQ is disabled using the method I've used so far, the 
led behavior would still be correct, but...

The reason I included the test for both in my patches is that if DMA gets 
disabled (which IIUC also disables NCQ) only the MV_PP_FLAG_EDMA_EN flag 
gets unset in mv_stop_edma, and not the NCQ flag (AFAICT). So that would 
result in blink mode remaining enabled while it shouldn't be.

I have so far not tested the led bahavior when DMA is disabled (not even 
sure if that's possible for a running system).

Note that e.g. the last if statement in mv_qc_defer also tests both.

Cheers,
FJP

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Frans Pop wrote:
> On Friday 13 March 2009, Mark Lord wrote:
>> --- old/drivers/ata/sata_mv.c	2009-03-12 10:23:41.000000000 -0400
>> +++ new/drivers/ata/sata_mv.c	2009-03-13 09:03:47.000000000 -0400
>> @@ -1455,9 +1455,8 @@
>>  		struct ata_port *this_ap = host->ports[port];
>>  		struct mv_port_priv *pp = this_ap->private_data;
>>
>> -		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
>> -			if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
>> -				return;
>> +		if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
>> +			return;
> 
> I expect that if NCQ is disabled using the method I've used so far, the 
> led behavior would still be correct, but...
..

I fully expect it to work, actually, but we do need confirmation
from you that it does work in real-life.  :)

> The reason I included the test for both in my patches is that if DMA gets 
> disabled (which IIUC also disables NCQ) only the MV_PP_FLAG_EDMA_EN flag 
> gets unset in mv_stop_edma, and not the NCQ flag (AFAICT). So that would 
> result in blink mode remaining enabled while it shouldn't be.
..

That is no longer true in the latest sata_mv.c that you are testing with.
It calls mv_edma_cfg() from mv_stop_edma() now, so it *should* be fine.
But we do need you to test it.

Thanks!

> Note that e.g. the last if statement in mv_qc_defer also tests both.
..

I might fix that one someday, too.

Cheers

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

I seem to have accidentally deleted your reply to this...

On Friday 13 March 2009, Frans Pop wrote:
> On Friday 13 March 2009, Mark Lord wrote:
> > --- old/drivers/ata/sata_mv.c	2009-03-12 10:23:41.000000000 -0400
> > +++ new/drivers/ata/sata_mv.c	2009-03-13 09:03:47.000000000 -0400
> > @@ -1455,9 +1455,8 @@
> >  		struct ata_port *this_ap = host->ports[port];
> >  		struct mv_port_priv *pp = this_ap->private_data;
> >
> > -		if (pp->pp_flags & MV_PP_FLAG_EDMA_EN)
> > -			if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
> > -				return;
> > +		if (pp->pp_flags & MV_PP_FLAG_NCQ_EN)
> > +			return;
>
> I expect that if NCQ is disabled using the method I've used so far, the
> led behavior would still be correct, but...

I did not mean I did not want to test the patch. Just wanted to verify 
that it was correct first.

> The reason I included the test for both in my patches is that if DMA
> gets disabled (which IIUC also disables NCQ) only the
> MV_PP_FLAG_EDMA_EN flag gets unset in mv_stop_edma, and not the NCQ
> flag (AFAICT). So that would result in blink mode remaining enabled
> while it shouldn't be.

I missed the fact that you now call mv_edma_cfg from mv_stop_edma, so the 
simplification is now indeed possible.

Tested and everything still works.

> I have so far not tested the led bahavior when DMA is disabled (not
> even sure if that's possible for a running system).

It seems that with libata-based drivers disabling dma is only possible at 
boot time and not dynamically, so this isn't possible anyway.
I don't know if access errors could cause the driver to fall back to PIO, 
but I don't really want to test that :-)

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Mark Lord]

Frans Pop wrote:
> I missed the fact that you now call mv_edma_cfg from mv_stop_edma, so the 
> simplification is now indeed possible.
> 
> Tested and everything still works.
> 
>> I have so far not tested the led bahavior when DMA is disabled (not
>> even sure if that's possible for a running system).
> 
> It seems that with libata-based drivers disabling dma is only possible at 
> boot time and not dynamically, so this isn't possible anyway.
> I don't know if access errors could cause the driver to fall back to PIO, 
> but I don't really want to test that :-)
..

Yeah, tricky to see that.  With sata_mv, though, DMA is disabled
for any irregular (non block layer R/W) commands.

So the code path is being exercised, just too quickly to see if
the LED actually blinks much in non-DMA modes.
I think you could test the non-DMA mode with this:

  #!/bin/bash
  i=0
  while ( sleep 0.3 ) ; do hdparm --read-sector $i --direct /dev/sda ; i=$((i + 1)) ; done

That should work.
you could even vary the sleep value to change the observed LED blinking rate.

I'll queue up the patch for 2.6.29 now, but if you could try the above
then that would also be cool.

Thanks

Re: [PATCH,v2][1/2] sata-mv: enable HDD led blinking when NCQ is active for GenIIe

[Frans Pop]

On Saturday 14 March 2009, Mark Lord wrote:
> > It seems that with libata-based drivers disabling dma is only
> > possible at boot time and not dynamically, so this isn't possible
> > anyway. I don't know if access errors could cause the driver to fall
> > back to PIO, but I don't really want to test that :-)
>
> Yeah, tricky to see that.  With sata_mv, though, DMA is disabled
> for any irregular (non block layer R/W) commands.
>
> So the code path is being exercised, just too quickly to see if
> the LED actually blinks much in non-DMA modes.
> I think you could test the non-DMA mode with this:
>
>   #!/bin/bash
>   i=0
>   while ( sleep 0.3 ) ; do hdparm --read-sector $i --direct /dev/sda ;
> i=$((i + 1)) ; done

Tried that. The blinking behavior is the same as with 2.6.25, so blink 
mode does get disabled correctly.

It's funny though that, as you expected, with that particular command you 
don't really get any blinking (varying sleep times don't make any real 
difference), so for this particular case it would actually be better to 
have blink mode enabled (I tested that also) even when not using DMA/NCQ. 
But I suggest not to go there.

> I'll queue up the patch for 2.6.29 now, but if you could try the above
> then that would also be cool.

Somehow I doubt you'll manage that, but .30 would be nice :-)

Cheers,
FJP