Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-25 Thread Mark Lord

Tejun Heo wrote:

Mark Lord wrote:

..

But no big deal.  I can clone code and not bother you any more.
In fact, some of the cloned code was already in sata_mv, and I removed
it this past week in my local working copy.  I'll just restore that,
along with another big blob so that we can select pm port where needed.

What a shame.


The order is somewhat reversed here and I can understand why you're
frustrated but I'm just trying to make things look right in long term,
so feel free to bother me. :-) For temporary solution, I'm okay either
way.  I'll clean things up later when the necessary core changes are made.

..

MMmm.. maybe the vendor unique FIS mechanism of the chipset
can save the scenario here.  It would seem to be a reasonable
way to direct a FIS (anything up to 8KB) at a specific pmp,
without changing the default pmp on the channel.

I can have qc_issue use that mechanism for anything destined
for pmp==15.  If it works.  The Marvell proprietary driver
has some kludgey status polling wrapped around their own use of it.

One of the chip errata apparently requires this anyway for doing
the READ_LOG_EXT commands after a device error, so perhaps it will
work out to be useful in more ways.

Speaking of which.. I would like to sort out the freeze stuff,
so that the sata_mv EH doesn't lock out the PMP commands as it
does today.

Can you recap what a LLD should be doing in the presence of a PM
for EH purposes?  Eg. media error on a PMP drive, so what core
ATA functions should the sata_mv EH interrupt handler be calling,
and in what sequence.. so that the libata-pmp/eh code can still
succeed in it's queries to the PM?

I think James is also interested in a decent explanation of
how to tie into the libata-eh stuff.

Like I noted before, sata_mv will need to eventually hard reset
the channel regardless, but it does seem permitted to use PIO
or vendor-unique-FIS PIO (with polling for either) in the interim.

Thanks.
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-25 Thread Tejun Heo
Hello, Mark.

Mark Lord wrote:
 MMmm.. maybe the vendor unique FIS mechanism of the chipset
 can save the scenario here.  It would seem to be a reasonable
 way to direct a FIS (anything up to 8KB) at a specific pmp,
 without changing the default pmp on the channel.
 
 I can have qc_issue use that mechanism for anything destined
 for pmp==15.  If it works.  The Marvell proprietary driver
 has some kludgey status polling wrapped around their own use of it.
 
 One of the chip errata apparently requires this anyway for doing
 the READ_LOG_EXT commands after a device error, so perhaps it will
 work out to be useful in more ways.

Hmmm...

 Speaking of which.. I would like to sort out the freeze stuff,
 so that the sata_mv EH doesn't lock out the PMP commands as it
 does today.
 
 Can you recap what a LLD should be doing in the presence of a PM
 for EH purposes?  Eg. media error on a PMP drive, so what core
 ATA functions should the sata_mv EH interrupt handler be calling,
 and in what sequence.. so that the libata-pmp/eh code can still
 succeed in it's queries to the PM?

libata doesn't really put much restrictions on what a LLD should do on
entering EH and if the controller's behavior is predictable, there's no
reason to freeze the port.  If the problem is that the DMA engine isn't
usable after PMP error but it's known that the controller isn't gonna
cause irq storm, no need to freeze.  The only command EH issues before
resetting which can use DMA protocol is READ_LOG_EXT.  Maybe there needs
to be a way to force PIO protocol for READ_LOG_EXT.  Other than that, if
no-data and PIO-only commands work fine, EH autopsy should work fine.

 Like I noted before, sata_mv will need to eventually hard reset
 the channel regardless, but it does seem permitted to use PIO
 or vendor-unique-FIS PIO (with polling for either) in the interim.

Just set ATA_EH_HARDRESET (which will soon become ATA_EH_RESET with
prefer-COMRESET patchset) from the interrupt handler before requesting EH.

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-25 Thread Mark Lord

Tejun Heo wrote:


libata doesn't really put much restrictions on what a LLD should do on
entering EH and if the controller's behavior is predictable, there's no
reason to freeze the port.  If the problem is that the DMA engine isn't
usable after PMP error but it's known that the controller isn't gonna
cause irq storm, no need to freeze.  The only command EH issues before
resetting which can use DMA protocol is READ_LOG_EXT.  Maybe there needs
to be a way to force PIO protocol for READ_LOG_EXT.  Other than that, if
no-data and PIO-only commands work fine, EH autopsy should work fine.

..

Eh?  READ LOG EXT *is* a PIO command, as opposed to READ LOG DMA EXT
which uses DMA.

And the EH also issues PIO READ_BUFFER commands for PM ports, if a PM is 
present.

???
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-25 Thread Tejun Heo
Hello, Mark.

Mark Lord wrote:
 libata doesn't really put much restrictions on what a LLD should do on
 entering EH and if the controller's behavior is predictable, there's no
 reason to freeze the port.  If the problem is that the DMA engine isn't
 usable after PMP error but it's known that the controller isn't gonna
 cause irq storm, no need to freeze.  The only command EH issues before
 resetting which can use DMA protocol is READ_LOG_EXT.  Maybe there needs
 to be a way to force PIO protocol for READ_LOG_EXT.  Other than that, if
 no-data and PIO-only commands work fine, EH autopsy should work fine.
 ..
 
 Eh?  READ LOG EXT *is* a PIO command, as opposed to READ LOG DMA EXT
 which uses DMA.

Ah.. right.  My bad.

 And the EH also issues PIO READ_BUFFER commands for PM ports, if a PM is
 present.

H Where?

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-24 Thread Mark Lord

Tejun Heo wrote:

Hello, Mark.

Mark Lord wrote:

Mark Lord wrote:

An alternative to all this, might be to expose the select_pmp()
function shown in the sample code, and have libata-pmp.c call that,
instead of having the new new .pmp_{read,write} functions. 

..

I wonder if this might be more viable than first thought.

Say the LLD, be it ata_piix or sata_mv or sata_svw, were to provide
an option ata_operations method for select_pmp_port(pmp),
which the core could invoke prior to any direct manipulation
of the shadow registers.


I don't really think that's a good solution.  That's the quickest
solution for sata_mv but it just works around more fundamental problem
of assuming SFF behavior in core layer which we need to drop anyway.


I really would like to keep the LLD code small, and have good solid
core routines for non-hardware specific functionality.  All of this stuff
I'm beginning to do with sata_mv would be trivial if I wanted to bloat
the LLD, but really.. only a tiny bit of it need be custom to sata_mv.

The existing SFF reset/probe/pmp stuff is just about exactly what
sata_mv needs.. and I feel a strong desire to not clone/duplicate
that hard-won functionality.


I strongly agree but am having difficult time agreeing with the proposed
approach.

..

Okay, then.  MASSIVE code-duplication, here I come!

-ml
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-24 Thread Mark Lord

Jeff Garzik wrote:

Tejun Heo wrote:

Yeah, exactly.  I think what needs to be done is to separate out SFF
assumptions from core layer, factor out SFF-proper helpers and use them
to implement LLDs for quasi-SFF controllers.



Thinking long term, I continue to hope that SFF support can eventually 
be separate into a sub-API, allowing people to disable (at compile time) 
SFF support completely if they don't need it.

..

I can think of a few more pressing things to disable before that.

Like ACPI and PMP.  Modularizing those (or more crudely de-selecting them
at build time) would save a lot of memory on small embedded boxes.

Cheers
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-24 Thread Jeff Garzik

Mark Lord wrote:

Jeff Garzik wrote:

Tejun Heo wrote:

Yeah, exactly.  I think what needs to be done is to separate out SFF
assumptions from core layer, factor out SFF-proper helpers and use them
to implement LLDs for quasi-SFF controllers.



Thinking long term, I continue to hope that SFF support can eventually 
be separate into a sub-API, allowing people to disable (at compile 
time) SFF support completely if they don't need it.

..

I can think of a few more pressing things to disable before that.

Like ACPI and PMP.  Modularizing those (or more crudely de-selecting them
at build time) would save a lot of memory on small embedded boxes.


Sure.  Make these options dependent on CONFIG_EMBEDDED, and go to town 
from there...


Jeff



-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-24 Thread Mark Lord

Tejun Heo wrote:

Hello, Mark.

Mark Lord wrote:

Mark Lord wrote:

An alternative to all this, might be to expose the select_pmp()
function shown in the sample code, and have libata-pmp.c call that,
instead of having the new new .pmp_{read,write} functions. 

..

I wonder if this might be more viable than first thought.

Say the LLD, be it ata_piix or sata_mv or sata_vw, were to provide
an option ata_operations method for select_pmp_port(pmp),
which the core could invoke prior to any direct manipulation
of the shadow registers.


I don't really think that's a good solution.  That's the quickest
solution for sata_mv but it just works around more fundamental problem
of assuming SFF behavior in core layer which we need to drop anyway.

...

No, the quickest solution for sata_mv, the one I apparently will now be using,
is to just clone about 250 lines of reset/debouce/probe code from libata-core
and change perhaps five lines of it to work around this issue plus some
chipset errata.

What I was thinking of, before, is the SATA specification which details
use of bits in the SCR to select a PMP port.  Pretty much exactly as
this chipset does it, except in the SCR rather than a proprieary port.

But no big deal.  I can clone code and not bother you any more.
In fact, some of the cloned code was already in sata_mv, and I removed
it this past week in my local working copy.  I'll just restore that,
along with another big blob so that we can select pm port where needed.

What a shame.

-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-24 Thread Jeff Garzik

To repeat myself from threads past...

In particular with the Marvell 6440 (SATA/SAS, drivers/scsi/mvsas.c) 
when SATA PMP support is completed, we will want to look at passing 
things other than normal ATA commands via -qc_issue.


Although selection isn't necessarily, talking to a PMP is fundamentally 
an asynchronous, event-driven operation just like any other FIS 
conversation.


We don't need to be adding hooks for each new and interesting type of 
FIS sent across the wire.  That just leads to an awful API.


Jeff



-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-24 Thread Tejun Heo
Mark Lord wrote:
 No, the quickest solution for sata_mv, the one I apparently will now be
 using,
 is to just clone about 250 lines of reset/debouce/probe code from
 libata-core
 and change perhaps five lines of it to work around this issue plus some
 chipset errata.
 
 What I was thinking of, before, is the SATA specification which details
 use of bits in the SCR to select a PMP port.  Pretty much exactly as
 this chipset does it, except in the SCR rather than a proprieary port.
 
 But no big deal.  I can clone code and not bother you any more.
 In fact, some of the cloned code was already in sata_mv, and I removed
 it this past week in my local working copy.  I'll just restore that,
 along with another big blob so that we can select pm port where needed.
 
 What a shame.

The order is somewhat reversed here and I can understand why you're
frustrated but I'm just trying to make things look right in long term,
so feel free to bother me. :-) For temporary solution, I'm okay either
way.  I'll clean things up later when the necessary core changes are made.

* Adding -pmp_select or -scr_pmp_rw or whatever callback to work
around the current problem.

* Duplicate code in sata_mv.

Whichever way you go, can you please mark where it's different and why?

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-23 Thread Tejun Heo
Hello, Mark.

Mark Lord wrote:
 Mark Lord wrote:
 An alternative to all this, might be to expose the select_pmp()
 function shown in the sample code, and have libata-pmp.c call that,
 instead of having the new new .pmp_{read,write} functions. 
 ..
 
 I wonder if this might be more viable than first thought.
 
 Say the LLD, be it ata_piix or sata_mv or sata_svw, were to provide
 an option ata_operations method for select_pmp_port(pmp),
 which the core could invoke prior to any direct manipulation
 of the shadow registers.

I don't really think that's a good solution.  That's the quickest
solution for sata_mv but it just works around more fundamental problem
of assuming SFF behavior in core layer which we need to drop anyway.

 I really would like to keep the LLD code small, and have good solid
 core routines for non-hardware specific functionality.  All of this stuff
 I'm beginning to do with sata_mv would be trivial if I wanted to bloat
 the LLD, but really.. only a tiny bit of it need be custom to sata_mv.
 
 The existing SFF reset/probe/pmp stuff is just about exactly what
 sata_mv needs.. and I feel a strong desire to not clone/duplicate
 that hard-won functionality.

I strongly agree but am having difficult time agreeing with the proposed
approach.

 Much of it I can see being shared with other half-and-half chipset drivers
 as we add PMP functionality to those.

Do we have other chips which have PMP support but still uses mixed SFF
interface?

 Maybe that's just the embedded side me showing through?
 
 It is tricky to define the right interfaces, though, as each chipset
 does throw its own unique curve balls at us.

Yeah, exactly.  I think what needs to be done is to separate out SFF
assumptions from core layer, factor out SFF-proper helpers and use them
to implement LLDs for quasi-SFF controllers.

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-23 Thread Jeff Garzik

Tejun Heo wrote:

Yeah, exactly.  I think what needs to be done is to separate out SFF
assumptions from core layer, factor out SFF-proper helpers and use them
to implement LLDs for quasi-SFF controllers.



Thinking long term, I continue to hope that SFF support can eventually 
be separate into a sub-API, allowing people to disable (at compile time) 
SFF support completely if they don't need it.


Jeff


-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-22 Thread Mark Lord

Mark Lord wrote:

Tejun,

Here's a first cut at this for discussion.

You may prefer different names for the invoking functions
inside libata-pmp.c, rather than simply pmp_read() and pmp_write(),
but I've been up too long and couldn't think of a better name.

An alternative to all this, might be to expose the select_pmp()
function shown in the sample code, and have libata-pmp.c call that,
instead of having the new new .pmp_{read,write} functions.

What do you think?

* * *

SATA host controllers which are not purely FIS-based require setup
of a special register to select the active pmp for taskfile accesses.

This can be done in the low-level driver for regular command issue
on a link.  But commands directed at a port-multiplier cause problems,
because they leave the original link pmp de-selected afterwards.

To fix this in a sane fashion, without tons of code duplication
from libata into the low-level driver, it is necessary to be able
to wrap the sata_pmp_read() and sata_pmp_write() functions.

This patch provides two new struct ata_port_operations methods for this,
and modifies the code in libata-pmp to use them if provided.

...

Note that, while this does work for sata_mv, I'm still thinking about it.

I'm not totally clear yet (more reading to do) as to how/when
the ATA shadow/taskfile registers get updated to reflect those
for the currently selected pmp..

It would seem that with other parts of libata-sff directly reading
from the ctl, status, and altstatus registers during polling,
command setup, and probing, that there might (?) be a loophole
somewhere in this strategy.

Is this scenario going to be possible:  somebody calls sata_pmp_read()
as part of, say, hotplug polling, and after that operation completes
we then have code that calls ata_check_status() prior to the next
tf_load / command issue ?  If so, they'll see the wrong cached 
shadow status register.


And for that matter, is it possible for sata_pmp_read() to be called
while the link is active with another command ?  Not today, it seems,
but what about when hotplug polling gets implemented ?

Mmm.. an amazing amount of complexity there for something that
ought to be very simple.

More reading to do, but comments from Tejun/others are welcomed.

Thanks.
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-22 Thread Mark Lord

Mark Lord wrote:


Note that, while this does work for sata_mv, I'm still thinking about it.

I'm not totally clear yet (more reading to do) as to how/when
the ATA shadow/taskfile registers get updated to reflect those
for the currently selected pmp..

It would seem that with other parts of libata-sff directly reading
from the ctl, status, and altstatus registers during polling,
command setup, and probing, that there might (?) be a loophole
somewhere in this strategy.

Is this scenario going to be possible:  somebody calls sata_pmp_read()
as part of, say, hotplug polling, and after that operation completes
we then have code that calls ata_check_status() prior to the next
tf_load / command issue ?  If so, they'll see the wrong cached shadow 
status register.

..

Answering my own question here: the above scenario really doesn't matter.
They'll see 0x50 in the status register, and be happy regardless.
Because that's what should have been there before the command issue
by sata_pmp_read() in the first place.  So not-a-problem.


And for that matter, is it possible for sata_pmp_read() to be called
while the link is active with another command ?  Not today, it seems,
but what about when hotplug polling gets implemented ?

..

That's the one I'm most concerned about.  Should I be?

-ml
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-22 Thread Mark Lord

Mark Lord wrote:

Mark Lord wrote:

...

And for that matter, is it possible for sata_pmp_read() to be called
while the link is active with another command ?  Not today, it seems,
but what about when hotplug polling gets implemented ?

..
That's the one I'm most concerned about.  Should I be?

...

Tejun,

On a related note, I'm now looking into PMP error handling in the driver.
The obvious thing I see that I want to fix, is that after a media error
on any PMP attached drive, I get this:

ata20.00: failed to read SCR 1 (Emask=0x40)
ata20.01: failed to read SCR 1 (Emask=0x40)
ata20.02: failed to read SCR 1 (Emask=0x40)
ata20.03: failed to read SCR 1 (Emask=0x40)
ata20.04: failed to read SCR 1 (Emask=0x40)

Okay, so those are from sata_pmp_read(), which cannot even
issue it's commands because the port was frozen by the EH.

Is this expected?  I'm not entirely clear what to do in
the EH for this driver.  The chipset docs say that
after just about any kind of error software must do
a hard reset of the channel to make it usable again.

But I suspect that PIO commands may be okay before that,
and sata_pmp_read() is trying to issue a PIO command.

How to resolve this?
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-22 Thread Tejun Heo
Hello, Mark.

Mark Lord wrote:
 This patch provides two new struct ata_port_operations methods for this,
 and modifies the code in libata-pmp to use them if provided.
 ...
 
 Note that, while this does work for sata_mv, I'm still thinking about it.
 
 I'm not totally clear yet (more reading to do) as to how/when
 the ATA shadow/taskfile registers get updated to reflect those
 for the currently selected pmp..
 
 It would seem that with other parts of libata-sff directly reading
 from the ctl, status, and altstatus registers during polling,
 command setup, and probing, that there might (?) be a loophole
 somewhere in this strategy.

Yeah, this is an interesting problem.  There are basically multiple sets
of TF registers and the SFF way of assuming single set doesn't really
work well and I don't really think adding -pmp_read/write is the
correct long term solution to the problem.  We need to confine direct TF
register accesses to SFF layer only as controllers which don't implement
SFF interface may or may not emulate TF registers and even when they do,
it sometimes can't really match the SFF behavior.

For command execution, TF write is already performed by qc_prep and
issue and TF read back should be performed by LLD if RESULT_TF is set.
For EH, the reset methods should be responsible whether or how the TF
registers are accessed.

Mark, for your case, I think the correct thing to do is to factor out
the necessary bare-bone part from SFF implementation and use it with
necessary PMP register setup once the necessary change is made to the
core layer.  The controller is NOT a SFF one and I don't think
stretching SFF implementation to fit mv's PMP-aware emulation of it is a
good idea.  Maybe we can fit libata to it but it's likely we'll need to
modify it again to fit different emulation from different vendor.

 Is this scenario going to be possible:  somebody calls sata_pmp_read()
 as part of, say, hotplug polling, and after that operation completes
 we then have code that calls ata_check_status() prior to the next
 tf_load / command issue ?  If so, they'll see the wrong cached shadow
 status register.

I think what should happen is that any of TF registers isn't accessed
behind LLD's back.  Then, you don't have nothing to worry about.  If the
controller emulates some of SFF interface, you can always properly wrap
SFF helpers with necessary setup and teardown added.

 Mmm.. an amazing amount of complexity there for something that
 ought to be very simple.

I wish things are a bit clearer now.  I think the problem here is that
we're assuming SFF TF access on controllers which aren't really SFF.
For sil24 and ahci, the driver emulates it and it isn't too difficult.
The picture gets more interesting for sata_mv as its hardware interface
much closer to SFF than sil24 or ahci and TF registers matter much more.
 For ahci and sil24, LLD can just fool libata core layer which assumes
ubiquitous TF access.  TF registers don't really matter to controller
operation anyway and feeding bogus values work well.  For sata_mv, it's
different.  Those registers are integral part of controller operation
and sata_mv can't really tolerate core layer stepping in w/o notifying LLD.

Thanks.

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-22 Thread Tejun Heo
Hello, Mark.

Mark Lord wrote:
 And for that matter, is it possible for sata_pmp_read() to be called
 while the link is active with another command ?  Not today, it seems,
 but what about when hotplug polling gets implemented ?
 ..
 That's the one I'm most concerned about.  Should I be?
 ...
 
 Tejun,
 
 On a related note, I'm now looking into PMP error handling in the driver.
 The obvious thing I see that I want to fix, is that after a media error
 on any PMP attached drive, I get this:
 
 ata20.00: failed to read SCR 1 (Emask=0x40)
 ata20.01: failed to read SCR 1 (Emask=0x40)
 ata20.02: failed to read SCR 1 (Emask=0x40)
 ata20.03: failed to read SCR 1 (Emask=0x40)
 ata20.04: failed to read SCR 1 (Emask=0x40)
 
 Okay, so those are from sata_pmp_read(), which cannot even
 issue it's commands because the port was frozen by the EH.

Hmm.. media error causes freeze?  Anyways, yeah, those are expected if
the port is frozen.  Those are from link autopsy.  Maybe it's better to
skip SCR access on fan-out ports if host port is frozen or at least
suppress the messages.

 Is this expected?  I'm not entirely clear what to do in
 the EH for this driver.  The chipset docs say that
 after just about any kind of error software must do
 a hard reset of the channel to make it usable again.
 
 But I suspect that PIO commands may be okay before that,
 and sata_pmp_read() is trying to issue a PIO command.

If the controller needs to be frozen after any kind of error, I don't
think there's much left to do other than suppressing those annoying
messages.  Hmmm.. how does the controller handle ATAPI check sense?

-- 
tejun
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-22 Thread Jeff Garzik

Tejun Heo wrote:

Yeah, this is an interesting problem.  There are basically multiple sets
of TF registers and the SFF way of assuming single set doesn't really
work well and I don't really think adding -pmp_read/write is the
correct long term solution to the problem.  We need to confine direct TF
register accesses to SFF layer only as controllers which don't implement
SFF interface may or may not emulate TF registers and even when they do,
it sometimes can't really match the SFF behavior.


Strongly agreed.



I wish things are a bit clearer now.  I think the problem here is that
we're assuming SFF TF access on controllers which aren't really SFF.
For sil24 and ahci, the driver emulates it and it isn't too difficult.
The picture gets more interesting for sata_mv as its hardware interface
much closer to SFF than sil24 or ahci and TF registers matter much more.
 For ahci and sil24, LLD can just fool libata core layer which assumes
ubiquitous TF access.  TF registers don't really matter to controller
operation anyway and feeding bogus values work well.  For sata_mv, it's
different.  Those registers are integral part of controller operation
and sata_mv can't really tolerate core layer stepping in w/o notifying LLD.


I would definitely like to move away from the model where non-SFF 
drivers have to emulate SFF in any way.


Re-reviewing the code, I don't see a lot of TF accesses outside of 
SFF-specific code, so we are already in pretty good shape.


I think the picture gets more complicated with sata_mv and similar 
drivers (soon sata_svw, too), because they use the SFF code but only for 
certain TF protocols.  The emulation question is not as clear, with 
those drivers.


Jeff


-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: new ata_port_operations for .pmp_{read,write} ?

2008-02-22 Thread Mark Lord

Mark Lord wrote:

An alternative to all this, might be to expose the select_pmp()
function shown in the sample code, and have libata-pmp.c call that,
instead of having the new new .pmp_{read,write} functions. 

..

I wonder if this might be more viable than first thought.

Say the LLD, be it ata_piix or sata_mv or sata_svw, were to provide
an option ata_operations method for select_pmp_port(pmp),
which the core could invoke prior to any direct manipulation
of the shadow registers.

It would really only be needed in perhaps three or four places total
(eg. sata_pmp_read/write, and before writes to the ata ctl register).
This could be a reasonably tidy way for the core to keep the LLD
abreast of its intent when accessing things.

I really would like to keep the LLD code small, and have good solid
core routines for non-hardware specific functionality.  All of this stuff
I'm beginning to do with sata_mv would be trivial if I wanted to bloat
the LLD, but really.. only a tiny bit of it need be custom to sata_mv.

The existing SFF reset/probe/pmp stuff is just about exactly what
sata_mv needs.. and I feel a strong desire to not clone/duplicate
that hard-won functionality.

Much of it I can see being shared with other half-and-half chipset drivers
as we add PMP functionality to those.

Maybe that's just the embedded side me showing through?

It is tricky to define the right interfaces, though, as each chipset
does throw its own unique curve balls at us.

I may prototype this select_pmp_port() concept in sata_mv(),
and see how it works out (or not).

Cheers
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html