[PATCH] sata_mv: enable active LED blink mode for SoC

[PATCH] sata_mv: enable active LED blink mode for SoC

[Saeed Bishara]

Enabling blink mode makes the SATA active LED more human visible.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 drivers/ata/sata_mv.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index 13c1d2a..302e1f8 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -208,6 +208,10 @@ enum {
 	DMA_IRQ			= (1 << 0),	/* shift by port # */
 	HC_COAL_IRQ		= (1 << 4),	/* IRQ coalescing */
 	DEV_IRQ			= (1 << 8),	/* shift by port # */
+	SOC_LED_CTRL_OFS	= 0x2c,
+	SOC_LED_CTRL_BLINK	= (1 << 0),	/* Active LED blink */
+	SOC_LED_CTRL_ACT_PRESENCE = (1 << 2),	/* Multiplex presence with */
+						/* the active LED */
 
 	/* Shadow block registers */
 	SHD_BLK_OFS		= 0x100,
@@ -2616,7 +2620,11 @@ static void mv6_phy_errata(struct mv_host_priv *hpriv, void __iomem *mmio,
 static void mv_soc_enable_leds(struct mv_host_priv *hpriv,
 				      void __iomem *mmio)
 {
-	return;
+	void __iomem *hc_mmio = mv_hc_base(mmio, 0);
+	u32 tmp = readl(hc_mmio + SOC_LED_CTRL_OFS);
+
+	/* enable blinking mode */
+	writel(tmp | SOC_LED_CTRL_BLINK, hc_mmio + SOC_LED_CTRL_OFS);
 }
 
 static void mv_soc_read_preamp(struct mv_host_priv *hpriv, int idx,
-- 
1.5.2.5

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[saeed bishara]

> +       SOC_LED_CTRL_ACT_PRESENCE = (1 << 2),   /* Multiplex presence with */
> +                                               /* the active LED */
meanwhile this mode (multiplexing presence with active LED) is not
enabled, but I believe some boards may need it. if your board already
have this mode then it's probably done by HW by multiplexing the two
wires into one LED.

saeed

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[Frans Pop]

Hello Saeed,

On Sunday 07 September 2008, Saeed Bishara wrote:
> Enabling blink mode makes the SATA active LED more human visible.

The patch does make the led responsive, but it does *not* restore it to 
the old behavior.

With the 2.6.25 kernel (and also during early boot of the device) the
HDD led flashes with an intensity that reflects the amount of disk 
activity (possibly the amount of data transferred), very similar to the 
LAN led.
The flashing was subtle, but very well visible both with low and with high 
disk activity.

With 2.6.26.3 without this patch there is no flashing visible at all.

With this patch applied to 2.6.26.3 I only get a very slow and steady 
blinking of the HDD led. A very regular, almost lazy on/off.
The flashing is related to disk activity in that the led stays on when 
there is no activity, but any relationship to the *intensity* of the disk 
activity is lost.

So, IMO it is an improvement, but it is definitely not a fix for the 
regression.

Cheers,
FJP

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[Mark Lord]

Frans Pop wrote:
..
> So, IMO it is an improvement, but it is definitely not a fix for the 
> regression.
..

With a new copy of hdparm, use "hdparm -Q1 /dev/sd?"

Does that fix your "regression" ?

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[Frans Pop]

On Monday 08 September 2008, Mark Lord wrote:
> Frans Pop wrote:
> > So, IMO it is an improvement, but it is definitely not a fix for the
> > regression.
>
> With a new copy of hdparm, use "hdparm -Q1 /dev/sd?"
> Does that fix your "regression" ?

$ sudo hdparm -Q1 /dev/sda

/dev/sda:
 setting queue_depth to 1
 queue_depth   =  1

Yes, that does result in the original behavior of the led.
(Using version 8.9 of hdparm.)

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[Frans Pop]

On Monday 08 September 2008, Frans Pop wrote:
> On Monday 08 September 2008, Mark Lord wrote:
> > Frans Pop wrote:
> > > So, IMO it is an improvement, but it is definitely not a fix for
> > > the regression.
> >
> > With a new copy of hdparm, use "hdparm -Q1 /dev/sd?"
> > Does that fix your "regression" ?
>
> $ sudo hdparm -Q1 /dev/sda
>
> /dev/sda:
>  setting queue_depth to 1
>  queue_depth   =  1
>
> Yes, that does result in the original behavior of the led.
> (Using version 8.9 of hdparm.)

But that's not new information: we'd already determined some time ago that 
disabling NCQ "fixed" the problem, both if it is done at runtime and when 
it is disabled in the driver:
http://www.spinics.net/lists/linux-ide/msg25642.html
http://www.spinics.net/lists/linux-ide/msg25645.html

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[Simon Farnsworth]

Frans Pop wrote:
> On Monday 08 September 2008, Frans Pop wrote:
>> On Monday 08 September 2008, Mark Lord wrote:
>>> Frans Pop wrote:
>>>> So, IMO it is an improvement, but it is definitely not a fix for
>>>> the regression.
>>> With a new copy of hdparm, use "hdparm -Q1 /dev/sd?"
>>> Does that fix your "regression" ?
>> $ sudo hdparm -Q1 /dev/sda
>>
>> /dev/sda:
>>  setting queue_depth to 1
>>  queue_depth   =  1
>>
>> Yes, that does result in the original behavior of the led.
>> (Using version 8.9 of hdparm.)
> 
> But that's not new information: we'd already determined some time ago that 
> disabling NCQ "fixed" the problem, both if it is done at runtime and when 
> it is disabled in the driver:
> http://www.spinics.net/lists/linux-ide/msg25642.html
> http://www.spinics.net/lists/linux-ide/msg25645.html

Setting queue_depth to 1 is not disabling NCQ; it's NCQ enabled, no more
than one command outstanding at a time. A queue depth of 0 disables NCQ
completely.

While I don't have the relevant hardware, this sounds like a hardware
limitation, not a regression. I suspect that the LED is active whenever
a command is outstanding; with NCQ allowed to queue more than 1 command
at a time, Linux is keeping the drive fed with commands so that there
are always outstanding commands, and thus the light never goes inactive
during disk activity.

As a result, you never see the LED intensity vary, as the drive never
idles waiting for a new command, but is constantly processing commands
as fast as it can. Allowing the drive to idle would get you the
"flickering" you want, at the expense of the performance gain NCQ provides.
-- 
I'm sure Saeed or Mark will correct me if I'm completely wrong,

Simon Farnsworth

RE: [PATCH] sata_mv: enable active LED blink mode for SoC

[Saeed Bishara]

> 
> While I don't have the relevant hardware, this sounds like a hardware
> limitation, not a regression. I suspect that the LED is active
whenever
> a command is outstanding; with NCQ allowed to queue more than 1
command
> at a time, Linux is keeping the drive fed with commands so that there
> are always outstanding commands, and thus the light never goes
inactive
> during disk activity.
> 
> As a result, you never see the LED intensity vary, as the drive never
> idles waiting for a new command, but is constantly processing commands
> as fast as it can. Allowing the drive to idle would get you the
> "flickering" you want, at the expense of the performance gain NCQ
> provides.
Frans reported that the LED stays on during disk activity, so it looks
like the drive always unbusy. I need to check with the HW guys how the
activity LED behaves in NCQ mode. I suspect that this disk responds
immediately to new commands telling the host "I got the command and it
will be handled later", this response clears the ports ATA busy bit. If
the activity LED reflects only the state of that bit in NCQ mode, then
that could explain the behavior that Frans described.

- saeed

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[Mark Lord]

Simon Farnsworth wrote:
>..
> Setting queue_depth to 1 is not disabling NCQ; it's NCQ enabled, no more
> than one command outstanding at a time. A queue depth of 0 disables NCQ
> completely.
..

Unfortunately, in the libata implementation, a queue depth of 1
actually *does* disable NCQ.  I wish it didn't (for easier testing
of features), but it does. 

Cheers

Re: [PATCH] sata_mv: enable active LED blink mode for SoC

[Mark Lord]

Frans Pop wrote:
> On Monday 08 September 2008, Frans Pop wrote:
>> On Monday 08 September 2008, Mark Lord wrote:
>>> Frans Pop wrote:
>>>> So, IMO it is an improvement, but it is definitely not a fix for
>>>> the regression.
>>> With a new copy of hdparm, use "hdparm -Q1 /dev/sd?"
>>> Does that fix your "regression" ?
>> $ sudo hdparm -Q1 /dev/sda
>>
>> /dev/sda:
>>  setting queue_depth to 1
>>  queue_depth   =  1
>>
>> Yes, that does result in the original behavior of the led.
>> (Using version 8.9 of hdparm.)
> 
> But that's not new information: we'd already determined some time ago that 
> disabling NCQ "fixed" the problem, both if it is done at runtime and when 
> it is disabled in the driver:
> http://www.spinics.net/lists/linux-ide/msg25642.html
> http://www.spinics.net/lists/linux-ide/msg25645.html
..

Still, that's proof that there is NO REGRESSION here,
so please stop using that term.

Older kernels had a working LED without NCQ,
and so do newer ones.  No regression.

In the meanwhile, it appears that Saeed is working to determine
for sure that this is a hardware limitation, as it appears to be.

Cheers