sata_mv errata function

sata_mv errata function

[Sylver Bruneau]

Hello,
during my analysis of some recent patches [1] from Marvell for several
ARM based (88f5182 & 88f5281) NAS devices, i've found that some
errata stuff are a little bit different from what is done in sata_mv.c
 mainline code.

I don't know if there is a reason for the code not to be the same, but
I think that it could be useful to bring that to you.

Here are the facts ...
First, the fix value for phyMode 3 is a little bit different :
 while sata_mv.c is applying a
        tmp &= ~0x7F800000;
the patch from Marvell (check _fixPhyParams function in
arch/arm/mach-feroceon/Board
/SATA/CoreDriver/mvSata.c file) is applying a
        regVal &= ~0x7F900000;

The fix value for phyMode 4 is also different for GEN IIE (6042 &
7042) devices :
in Marvell patch, there is an additional fix for this case (where nothing
seems to be done for this in sata_mv.c) :
        /* phy mode 4 register of Gen IIE devices has some restriction */
         if (pAdapter->sataAdapterGeneration >= MV_SATA_GEN_IIE)
        {
            phyMode4Value &= ~0x5DE3FFFC;
            phyMode4Value |= MV_BIT2;
        }

I understand that what is done in this function is not well documented,
 but as it's what is done by Marvell in their patches, do you think that it
should be merged in sata_mv.c ?

If so, I will post a proper patch to add this !

Thanks,
Sylver Bruneau

[1] Marvell/QNAP patch for the TS-409 NAS (88f5281 based + PCIe
88SX7042 SATA controller)
      http://qnap.free.fr/kernel/stock/linux-2.6.21.1.patch

RE: sata_mv errata function

[Morrison, Tom]

Interesting - I was just looking at this code this morning - and
comparing with the reference driver Marvell has given us - this was
one of the differences I was looking at because with this new driver
and the old reference driver we were having performance problems
due to vibrations or something that is causing the disk write
performance
to go very low (<5Mbytes/second)...

I'd be very interested what Mark has to say about this - he just 
made some update to the driver - perhaps he has updated these values...

t

-----Original Message-----
From: linux-ide-owner@vger.kernel.org
[mailto:linux-ide-owner@vger.kernel.org] On Behalf Of Sylver Bruneau
Sent: Monday, April 21, 2008 8:43 AM
To: linux-ide@vger.kernel.org
Cc: jeff@garzik.org
Subject: sata_mv errata function

Hello,
during my analysis of some recent patches [1] from Marvell for several
ARM based (88f5182 & 88f5281) NAS devices, i've found that some
errata stuff are a little bit different from what is done in sata_mv.c
 mainline code.

I don't know if there is a reason for the code not to be the same, but
I think that it could be useful to bring that to you.

Here are the facts ...
First, the fix value for phyMode 3 is a little bit different :
 while sata_mv.c is applying a
        tmp &= ~0x7F800000;
the patch from Marvell (check _fixPhyParams function in
arch/arm/mach-feroceon/Board
/SATA/CoreDriver/mvSata.c file) is applying a
        regVal &= ~0x7F900000;

The fix value for phyMode 4 is also different for GEN IIE (6042 &
7042) devices :
in Marvell patch, there is an additional fix for this case (where
nothing
seems to be done for this in sata_mv.c) :
        /* phy mode 4 register of Gen IIE devices has some restriction
*/
         if (pAdapter->sataAdapterGeneration >= MV_SATA_GEN_IIE)
        {
            phyMode4Value &= ~0x5DE3FFFC;
            phyMode4Value |= MV_BIT2;
        }

I understand that what is done in this function is not well documented,
 but as it's what is done by Marvell in their patches, do you think that
it
should be merged in sata_mv.c ?

If so, I will post a proper patch to add this !

Thanks,
Sylver Bruneau

[1] Marvell/QNAP patch for the TS-409 NAS (88f5281 based + PCIe
88SX7042 SATA controller)
      http://qnap.free.fr/kernel/stock/linux-2.6.21.1.patch
--
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

Re: sata_mv errata function

[Mark Lord]

Morrison, Tom wrote:
> Interesting - I was just looking at this code this morning - and
> comparing with the reference driver Marvell has given us - this was
> one of the differences I was looking at because with this new driver
> and the old reference driver we were having performance problems
> due to vibrations or something that is causing the disk write
> performance
> to go very low (<5Mbytes/second)...
> 
> I'd be very interested what Mark has to say about this - he just 
> made some update to the driver - perhaps he has updated these values...
..

I've replied separately to Sylver's original post,
but just to keep me on all branches of this thread I'm
replying again now to you.  :)

Most errata fixes are on my menu here for the next few weeks,
so the ones pointed out by Sylver will get fixed as part of that.

As to the current state of sata_mv, it is *still* deservedly marked
as "HIGHLY EXPERIMENTAL".  I would not trust my own data to it yet.

In practice, I believe it is actually working well now,
on non-PCIX systems at least.

But, as you see, there are still missing workarounds for various errata,
and I am not yet totally finished with the error-handling logic.
That still needs just a little more TLC to be reliable/correct with NCQ.

So the "HIGHLY EXPERIMENTAL" label remains, for now, but I do plan to 
strip it away entirely within a few weeks of now.

Cheers

RE: sata_mv errata function

[Morrison, Tom]

Understood Mark - I wasn't expecting that this was complete! I think
we'll all clap very hard when you say - DONE!

Also, I've got to say, that you've been so helpful with my problems
with this chip in the past, I've really appreciated all your help!

As you saw in my post - we have a vibration problem - that seems to
cause some type of performance problem without any errors occurring.

Very weird! IFF we completely isolate the disk from the chassis - 
the write performances goes up to what we expect ~75Mbs - but with it
in the box - based upon how much vibration (directly related to 
heat - you can figure out what is causing vibration) - the performance
goes down to the tube. We see NO errors at the sata level - and
using SMART/smartctl we do not seem to see any errors that point to
something we can fix easily...

Our hardware engineer is looking into the affect of changing these
values:

   SQ (squelch detector threshold) in PHY Mode 3 register.
   TxPRE (transmitter pre-emphasis) in PHY Mode 2 register.
   TXAmp (transmitter differential Amplitude) in PHY mode 2 register.

I believe he has gotten some suggestions from the disk manufacturer and
Marvell - so we shall see what comes of it!

I'll keep you all updated...

Tom

Ps: this is also related to the Western Digital Green thread...:-)...


-----Original Message-----
From: Mark Lord [mailto:liml@rtr.ca] 
Sent: Monday, April 21, 2008 9:42 AM
To: Morrison, Tom
Cc: Sylver Bruneau; linux-ide@vger.kernel.org; jeff@garzik.org
Subject: Re: sata_mv errata function

Morrison, Tom wrote:
> Interesting - I was just looking at this code this morning - and
> comparing with the reference driver Marvell has given us - this was
> one of the differences I was looking at because with this new driver
> and the old reference driver we were having performance problems
> due to vibrations or something that is causing the disk write
> performance
> to go very low (<5Mbytes/second)...
> 
> I'd be very interested what Mark has to say about this - he just 
> made some update to the driver - perhaps he has updated these
values...
..

I've replied separately to Sylver's original post,
but just to keep me on all branches of this thread I'm
replying again now to you.  :)

Most errata fixes are on my menu here for the next few weeks,
so the ones pointed out by Sylver will get fixed as part of that.

As to the current state of sata_mv, it is *still* deservedly marked
as "HIGHLY EXPERIMENTAL".  I would not trust my own data to it yet.

In practice, I believe it is actually working well now,
on non-PCIX systems at least.

But, as you see, there are still missing workarounds for various errata,
and I am not yet totally finished with the error-handling logic.
That still needs just a little more TLC to be reliable/correct with NCQ.

So the "HIGHLY EXPERIMENTAL" label remains, for now, but I do plan to 
strip it away entirely within a few weeks of now.

Cheers

Re: sata_mv errata function

[Grant Grundler]

On Mon, Apr 21, 2008 at 6:51 AM, Morrison, Tom <tmorrison@empirix.com> wrote:
> Understood Mark - I wasn't expecting that this was complete! I think
>  we'll all clap very hard when you say - DONE!
>
>  Also, I've got to say, that you've been so helpful with my problems
>  with this chip in the past, I've really appreciated all your help!
>
>  As you saw in my post - we have a vibration problem - that seems to
>  cause some type of performance problem without any errors occurring.
>
>  Very weird! IFF we completely isolate the disk from the chassis -
>  the write performances goes up to what we expect ~75Mbs - but with it
>  in the box - based upon how much vibration (directly related to
>  heat - you can figure out what is causing vibration)

heat and vibration are separate issues. Please don't confuse them.
Each vendor responds to "overtemp" differently but they all result
in some sort of performance degradation since they (a) attempt
to validate data and (b) conserve power.

Drives report their internal temp via SMART data. See "ID" 194 as
documented in wikipedia:
    http://en.wikipedia.org/wiki/Self-Monitoring,_Analysis,_and_Reporting_Technology

Use smartctl (part of smartmontools package) to read that data.

> - the performance
>  goes down to the tube. We see NO errors at the sata level - and
>  using SMART/smartctl we do not seem to see any errors that point to
>  something we can fix easily...

 AFAIK, there is no access to the
accelerometers that are built in to "near line" and "enterprise"
disk drives.  SMART data _might_ tell you why. Check "reallocated
sectors count" and "Seek Error Count"  before and after your test runs.
They might correlate slightly with vibration but I've not heard any
vendor confirm that.

>  Our hardware engineer is looking into the affect of changing these
>  values:
>
>    SQ (squelch detector threshold) in PHY Mode 3 register.
>    TxPRE (transmitter pre-emphasis) in PHY Mode 2 register.
>    TXAmp (transmitter differential Amplitude) in PHY mode 2 register.

These have nothing to do with vibration.
This has to do with TX signal strength and RX filtering.
If the drives perform fine outside the disk enclosure while
sitting on foam, then this isn't the problem.

grant

>
>  I believe he has gotten some suggestions from the disk manufacturer and
>  Marvell - so we shall see what comes of it!
>
>  I'll keep you all updated...
>
>  Tom
>
>  Ps: this is also related to the Western Digital Green thread...:-)...
>
>
>
>
>  -----Original Message-----
>  From: Mark Lord [mailto:liml@rtr.ca]
>  Sent: Monday, April 21, 2008 9:42 AM
>  To: Morrison, Tom
>  Cc: Sylver Bruneau; linux-ide@vger.kernel.org; jeff@garzik.org
>  Subject: Re: sata_mv errata function
>
>  Morrison, Tom wrote:
>  > Interesting - I was just looking at this code this morning - and
>  > comparing with the reference driver Marvell has given us - this was
>  > one of the differences I was looking at because with this new driver
>  > and the old reference driver we were having performance problems
>  > due to vibrations or something that is causing the disk write
>  > performance
>  > to go very low (<5Mbytes/second)...
>  >
>  > I'd be very interested what Mark has to say about this - he just
>  > made some update to the driver - perhaps he has updated these
>  values...
>  ..
>
>  I've replied separately to Sylver's original post,
>  but just to keep me on all branches of this thread I'm
>  replying again now to you.  :)
>
>  Most errata fixes are on my menu here for the next few weeks,
>  so the ones pointed out by Sylver will get fixed as part of that.
>
>  As to the current state of sata_mv, it is *still* deservedly marked
>  as "HIGHLY EXPERIMENTAL".  I would not trust my own data to it yet.
>
>  In practice, I believe it is actually working well now,
>  on non-PCIX systems at least.
>
>  But, as you see, there are still missing workarounds for various errata,
>  and I am not yet totally finished with the error-handling logic.
>  That still needs just a little more TLC to be reliable/correct with NCQ.
>
>  So the "HIGHLY EXPERIMENTAL" label remains, for now, but I do plan to
>  strip it away entirely within a few weeks of now.
>
>  Cheers
>  --
>  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
>

Re: sata_mv errata function

[Mark Lord]

Sylver Bruneau wrote:
> Hello,
> during my analysis of some recent patches [1] from Marvell for several
> ARM based (88f5182 & 88f5281) NAS devices, i've found that some
> errata stuff are a little bit different from what is done in sata_mv.c
>  mainline code.
> 
> I don't know if there is a reason for the code not to be the same, but
> I think that it could be useful to bring that to you.
> 
> Here are the facts ...
> First, the fix value for phyMode 3 is a little bit different :
>  while sata_mv.c is applying a
>         tmp &= ~0x7F800000;
> the patch from Marvell (check _fixPhyParams function in
> arch/arm/mach-feroceon/Board
> /SATA/CoreDriver/mvSata.c file) is applying a
>         regVal &= ~0x7F900000;
> 
> The fix value for phyMode 4 is also different for GEN IIE (6042 &
> 7042) devices :
> in Marvell patch, there is an additional fix for this case (where nothing
> seems to be done for this in sata_mv.c) :
>         /* phy mode 4 register of Gen IIE devices has some restriction */
>          if (pAdapter->sataAdapterGeneration >= MV_SATA_GEN_IIE)
>         {
>             phyMode4Value &= ~0x5DE3FFFC;
>             phyMode4Value |= MV_BIT2;
>         }
> 
> I understand that what is done in this function is not well documented,
>  but as it's what is done by Marvell in their patches, do you think that it
> should be merged in sata_mv.c ?
> 
> If so, I will post a proper patch to add this !
..

Thanks, Sylver.

I am currently working on sata_mv on behalf of Marvell,
and a full errata overhaul is in the works for the next
2-3 weeks here.  So those bits will get fixed by me as
part of this effort.

But I will keep your description above in my inbox as a reminder
to double-check those specific items, just in case it differs from
the detailed errata information Marvell has provided to me (under NDA).

Thanks again.

Mark