libata: implement dev->acpi_init_gtm

Cable detection on Nvidia PATA hosts is pathetic. The current nv_cable_detect() assumes that the native cable detection only mistakes 80c as 40c but this isn't true. On ASUS A8N-E, cable register almost always says 80c is attached and it also manages to trick the drive that the cable is 80c.

Quoted 5 lines

Cable detection on Nvidia PATA hosts is pathetic. The current nv_cable_detect() assumes that the native cable detection only mistakes 80c as 40c but this isn't true. On ASUS A8N-E, cable register almost always says 80c is attached and it also manages to trick the drive that the cable is 80c.

According to the last Nvidia comments on Nvidia there is no cable detect bit. I've just never gotten around to implementing it because due to lack of documentation Nvidia PATA is very very low priority compared to problems with documented chipsets from helpful vendors.

all we can use is how the BIOS configured it. I suppose BIOS does it by issuing trial commands which I don't think adding to libata is a good idea.

Can you please lemme know what you don't like about the current implementation or what other approach you have in mind? I don't like Nvidia PATA either but there are a lot of people using it out there.

Quoted 2 lines

all we can use is how the BIOS configured it. I suppose BIOS does it by issuing trial commands which I don't think adding to libata is a good idea.

The BIOS does it by asking the hardware somehow. I traced one or two BIOSes that far. The info is there but its not documented in the slightest so only ACPI makes it visible via the BIOS.

Quoted 3 lines

Can you please lemme know what you don't like about the current implementation or what other approach you have in mind? I don't like Nvidia PATA either but there are a lot of people using it out there.

We seem to be able to trust the drives and BIOS ACPI data for Nvidia (at least what I have seen), so I guess we should simply declare the cable type unknown, 80 wire if ACPI says it is and then do the drive detect side ?

Quoted 2 lines

all we can use is how the BIOS configured it. I suppose BIOS does it by issuing trial commands which I don't think adding to libata is a good idea.

The BIOS does it by asking the hardware somehow. I traced one or two BIOSes that far. The info is there but its not documented in the slightest so only ACPI makes it visible via the BIOS.

Quoted 3 lines

Can you please lemme know what you don't like about the current implementation or what other approach you have in mind? I don't like Nvidia PATA either but there are a lot of people using it out there.

We seem to be able to trust the drives and BIOS ACPI data for Nvidia (at least what I have seen), so I guess we should simply declare the cable type unknown, 80 wire if ACPI says it is and then do the drive detect side ?

Using the initial GTM value is the right thing to do because both are trying to check firmware setting and by the time ->cable_detect runs the controller is already forced into PIO0 by reset.

+ * Determine cable type from UDMA configuration in @gtm. If UDMA is
+ * disabled, it returns ATA_CBL_PATA_UNK. If UDMA is enabled and
+ * faster than 33, PATA_80; otherwise, PATA_40.

Quoted 3 lines

Using the initial GTM value is the right thing to do because both are trying to check firmware setting and by the time ->cable_detect runs the controller is already forced into PIO0 by reset.

As we only look at the DMA bits that doesn't matter but I agree it would be cleaner if we were more careful. Also our mode setting outside pata_acpi doesn't touch the ACPI settings so its ok for that reason too.

Quoted 3 lines

+ * Determine cable type from UDMA configuration in @gtm. If UDMA is
+ * disabled, it returns ATA_CBL_PATA_UNK. If UDMA is enabled and
+ * faster than 33, PATA_80; otherwise, PATA_40.

NAK

Consider the case of a UDMA 33 capable device on an 80 wire cable. We simply can't use the cable detect methods to prove it is 40 wire this way.

What we actually know is

If UDMA > 33 selected -> UDMA > 33 works
80 wire
Short
or SATA

If UDMA > 33 not selected Unknown
40 wire
80 + Drive limit 80 + Controller limit 80 + BIOS is set to PIO etc

The only case I can see you could reasonably suggest 40 wire would be IFF the drive is capable of UDMA > 33 _and_ the ACPI mode is UDMA 33.

GTM results do change whether you call STM or not. GTM in many cases is implemented as reading the current configuration value from the controller so if you configure PIO0, it will return PIO0 timings. And, yeah, on ASUS A8N-E, if you configure PIO0, GTM answer says DMA is off.

What the function answers is not actually cable type but "according to the current configuration, using this cable type won't violate BIOS configuration" kind of answer. How the caller is to use that is the caller's responsibility.

Quoted 4 lines

What the function answers is not actually cable type but "according to the current configuration, using this cable type won't violate BIOS configuration" kind of answer. How the caller is to use that is the caller's responsibility.

How does the caller make use of it ? The only way I can think of to use it is that if either drive says UDMA > 33 then we know its 80 wire. Otherwise we can't really prove anything. And if neither drive says > 80 wire we have to go on what other info we have, which for NVidia appears to be "guess 40".

I'm not sure therefore we gain anything ?

2. UDMA mode is configured but equal to or under UDMA33. Dunno whether it's cable or device limit but anyways you better limit it to UDMA33 too.

Quoted 2 lines

2. UDMA mode is configured but equal to or under UDMA33. Dunno whether it's cable or device limit but anyways you better limit it to UDMA33 too.

If it is under UDMA33 you know it isn't a cable limit

If it is UDMA 33 and the device top is > UDMA 33 it might be

This matters as you've got to account for both master and slave, as well as hotplug in theory.

detection. We're basically just trying to follow what BIOS did. Maybe we should return ATA_CBL_PATA_MAYBE_80 unconditionally and implement

ata_acpi_more_filter()?

Quoted 2 lines

detection. We're basically just trying to follow what BIOS did. Maybe we should return ATA_CBL_PATA_MAYBE_80 unconditionally and implement

MAYBE_80 = UNK.

Quoted 1 line

ata_acpi_more_filter()?

We can also use UNK intelligently in the eh code to go straight from UDMA133/100/66 to 33 (if you don't already I've not read that logic recently)

Quoted 3 lines

Using the initial GTM value is the right thing to do because both are trying to check firmware setting and by the time ->cable_detect runs the controller is already forced into PIO0 by reset.

As we only look at the DMA bits that doesn't matter but I agree it would

libata: correct handling of SRST reset sequences

be cleaner if we were more careful. Also our mode setting outside pata_acpi doesn't touch the ACPI settings so its ok for that reason too.

Quoted 5 lines · Alan Cox

Quoted 3 lines

Using the initial GTM value is the right thing to do because both are trying to check firmware setting and by the time ->cable_detect runs the controller is already forced into PIO0 by reset.

As we only look at the DMA bits that doesn't matter but I agree it would

It does matter,

http://lkml.org/lkml/2007/10/12/537

"

Quoted 1 line

libata: correct handling of SRST reset sequences

This potentially adds a regression for nVidia boxes (but ACPI cable detection should compensate for it):

pata_amd's ->cable_detect checks UDMA timings to get the cable type (cable detection is done before the SRST) but pata_amd's ->set_piomode

messes with UDMA timings. It seems that both methods need fixing. "

[ Now we see that since ACPI can just dump the current settings
ACPI cable detection won't help a tiny bit. ]

AFAICS pata_via.c has the same problem w.r.t. ->set_piomode

Quoted 2 lines

be cleaner if we were more careful. Also our mode setting outside pata_acpi doesn't touch the ACPI settings so its ok for that reason too.

Thanks,
Bart

Add dev->acpi_init_gtm and store initial GTM values on host initialization. If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag is set. This is to remember BIOS/firmware programmed initial timing for later use before reset and mode configuration modify it.

Signed-off-by Tejun Heo <[email protected]>

Quoted 6 lines · Tejun Heo

Add dev->acpi_init_gtm and store initial GTM values on host initialization. If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag is set. This is to remember BIOS/firmware programmed initial timing for later use before reset and mode configuration modify it.

Signed-off-by Tejun Heo <[email protected]>

It sounds like pata_via and pata_amd need a foo_save_initial_config() much like AHCI, during which they would fill ppriv->init_gtm rather than ap->init_gtm. Thoughts? Both drivers are calling ata_acpi_cbl() themselves, permitting the possibility of ppriv->init_gtm.

This avoids forcing everyone else to bear the memory cost in ata_port for just these two drivers.

Quoted 6 lines · Jeff Garzik

Quoted 5 lines · Tejun Heo

Add dev->acpi_init_gtm and store initial GTM values on host initialization. If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag is set. This is to remember BIOS/firmware programmed initial timing for later use before reset and mode configuration modify it.

Quoted 8 lines

Quoted 1 line

Signed-off-by Tejun Heo <[email protected]>

It sounds like pata_via and pata_amd need a foo_save_initial_config() much like AHCI, during which they would fill ppriv->init_gtm rather than ap->init_gtm. Thoughts? Both drivers are calling ata_acpi_cbl() themselves, permitting the possibility of ppriv->init_gtm.

This avoids forcing everyone else to bear the memory cost in ata_port for just these two drivers.

The memory overhead is pretty small and more importantly there isn't a good place to load initial GTM. ACPI is associated with the host during host registration after all private host initialization is complete. When the EH gets invoked, the first thing which is done is forcing PIO0 before initial reset.

Unless we add another hook, the only place to put this is in private ->error_handler(). A driver can check whether it's being called for the first time and record it in private structure, which isn't too pretty, so I thought doing it this way was fair tradeoff.

Quoted 21 lines · Tejun Heo

Quoted 6 lines · Jeff Garzik

Quoted 5 lines · Tejun Heo

Add dev->acpi_init_gtm and store initial GTM values on host initialization. If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag is set. This is to remember BIOS/firmware programmed initial timing for later use before reset and mode configuration modify it.

Quoted 8 lines

Quoted 1 line

Signed-off-by Tejun Heo <[email protected]>

It sounds like pata_via and pata_amd need a foo_save_initial_config() much like AHCI, during which they would fill ppriv->init_gtm rather than ap->init_gtm. Thoughts? Both drivers are calling ata_acpi_cbl() themselves, permitting the possibility of ppriv->init_gtm.

This avoids forcing everyone else to bear the memory cost in ata_port for just these two drivers.

The memory overhead is pretty small and more importantly there isn't a good place to load initial GTM. ACPI is associated with the host during host registration after all private host initialization is complete. When the EH gets invoked, the first thing which is done is forcing PIO0 before initial reset.

Quoted 4 lines

Unless we add another hook, the only place to put this is in private ->error_handler(). A driver can check whether it's being called for the first time and record it in private structure, which isn't too pretty, so I thought doing it this way was fair tradeoff.

Jeff, do you agree or still think it's better done in LLDs? I'm okay either way.

Add dev->acpi_init_gtm and store initial GTM values on host initialization. If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag is set. This is to remember BIOS/firmware programmed initial timing for later use before reset and mode configuration modify it.

Quoted 4 lines · On Sat, 03 Nov 2007 00:20:10 +0900 Tejun Heo <[email protected]> wrote:

Add dev->acpi_init_gtm and store initial GTM values on host initialization. If the field is valid, ATA_PFLAG_INIT_GTM_VALID flag is set. This is to remember BIOS/firmware programmed initial timing for later use before reset and mode configuration modify it.

Don't you need to clear this on a hotplug event ?