[PATCH] reverse pci config space restore order

[PATCH] reverse pci config space restore order

[Yu, Luming]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]


According to Intel ICH spec, there are several rules that
Base Address should be programmed before IOSE 
(PCICMD register ) enabled. 

For example ICH7:
12.1.3  SATA : the base address register for the bus 
master register should be programmed before this bit is set.

11.1.3:  PCICMD (USB): The base address register for
USB should be programmed before this bit is set.
....
To make sure kernel code follow this rule 
, and prevent unnecessary confusion. I proposal this
patch. 

Note:
I'm sorry the patch is NOT in good format.
If you want to apply please use the attached patch file.
sorry for my email client.


Signed-off-by: Luming Yu <luming.yu@intel.com>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bea1ad1..19a71a6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -456,7 +456,7 @@ pci_restore_state(struct pci_dev *dev)
 {
 	int i;
 
-	for (i = 0; i < 16; i++)
+	for (i = 15; i >= 0 ; i--)
 		pci_write_config_dword(dev,i * 4,
dev->saved_config_space[i]);
 	return 0;
 }

[-- Attachment #2: reverse_pci_config_space_restore_order.patch --]
[-- Type: application/octet-stream, Size: 339 bytes --]

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bea1ad1..19a71a6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -456,7 +456,7 @@ pci_restore_state(struct pci_dev *dev)
 {
 	int i;
 
-	for (i = 0; i < 16; i++)
+	for (i = 15; i >= 0 ; i--)
 		pci_write_config_dword(dev,i * 4, dev->saved_config_space[i]);
 	return 0;
 }

Re: [PATCH] reverse pci config space restore order

[Matthew Garrett]

On Tue, Apr 25, 2006 at 02:50:57PM +0800, Yu, Luming wrote:

> -	for (i = 0; i < 16; i++)
> +	for (i = 15; i >= 0 ; i--)

We certainly need to do /something/ here, but I'm not sure this is it. 
Adam Belay has code to limit PCI state restoration to the PCI-specified 
registers, with the idea being that individual drivers fix things up 
properly. While this has the obvious drawback that almost every PCI 
driver in the tree would then need fixing up, it's also probably the 
right thing.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] reverse pci config space restore order

[Arjan van de Ven]

On Tue, 2006-04-25 at 11:48 +0100, Matthew Garrett wrote:
> On Tue, Apr 25, 2006 at 02:50:57PM +0800, Yu, Luming wrote:
> 
> > -	for (i = 0; i < 16; i++)
> > +	for (i = 15; i >= 0 ; i--)
> 
> We certainly need to do /something/ here, but I'm not sure this is it. 
> Adam Belay has code to limit PCI state restoration to the PCI-specified 
> registers, with the idea being that individual drivers fix things up 
> properly. While this has the obvious drawback that almost every PCI 
> driver in the tree would then need fixing up, it's also probably the 
> right thing.

it has a second drawback: it assumes all devices HAVE a driver, which
isn't normally the case...

Re: [PATCH] reverse pci config space restore order

[Matthew Garrett]

On Tue, Apr 25, 2006 at 12:51:01PM +0200, Arjan van de Ven wrote:

> it has a second drawback: it assumes all devices HAVE a driver, which
> isn't normally the case...

Yeah, I guess there's a call for keeping a pci_save_entire_state type 
call and getting pci_device_suspend to use that in the no-driver case 
(also for the "Make my driver work again quickly" case, but still)

-- 
Matthew Garrett | mjg59@srcf.ucam.org

RE: [PATCH] reverse pci config space restore order

[Brown, Len]

>On Tue, 2006-04-25 at 11:48 +0100, Matthew Garrett wrote:
>> On Tue, Apr 25, 2006 at 02:50:57PM +0800, Yu, Luming wrote:
>> 
>> > -	for (i = 0; i < 16; i++)
>> > +	for (i = 15; i >= 0 ; i--)
>> 
>> We certainly need to do /something/ here, but I'm not sure 
>> this is it. 
>> Adam Belay has code to limit PCI state restoration to the 
>> PCI-specified 
>> registers, with the idea being that individual drivers fix things up 
>> properly. While this has the obvious drawback that almost every PCI 
>> driver in the tree would then need fixing up, it's also probably the 
>> right thing.
>
>it has a second drawback: it assumes all devices HAVE a driver, which
>isn't normally the case...

Adam mentioned earlier, and I agree, that it is probably a bad
idea for this code to blindly scribble on the BIST field at i=3.
Probably we should clear that field before restoring it.

Re: this patch
I think that this patch is likely a positive forward step.
It seems logical to restore the BARs before the CMD/STATUS in general,
nothing specific to the ICH here.

But yes, this is a helper routine and devices where it hurts
instead of helps should have their own routine.  Complex devices
need to handle the device-specific config space state above these
1st 16 locations anyway.

-Len

RE: [PATCH] reverse pci config space restore order

[abelay]

Quoting "Brown, Len" <len.brown@intel.com>:

>
>> On Tue, 2006-04-25 at 11:48 +0100, Matthew Garrett wrote:
>>> On Tue, Apr 25, 2006 at 02:50:57PM +0800, Yu, Luming wrote:
>>>
>>> > -	for (i = 0; i < 16; i++)
>>> > +	for (i = 15; i >= 0 ; i--)
>>>
>>> We certainly need to do /something/ here, but I'm not sure
>>> this is it.
>>> Adam Belay has code to limit PCI state restoration to the
>>> PCI-specified
>>> registers, with the idea being that individual drivers fix things up
>>> properly. While this has the obvious drawback that almost every PCI
>>> driver in the tree would then need fixing up, it's also probably the
>>> right thing.
>>
>> it has a second drawback: it assumes all devices HAVE a driver, which
>> isn't normally the case...
>
> Adam mentioned earlier, and I agree, that it is probably a bad
> idea for this code to blindly scribble on the BIST field at i=3.
> Probably we should clear that field before restoring it.
>
> Re: this patch
> I think that this patch is likely a positive forward step.
> It seems logical to restore the BARs before the CMD/STATUS in general,
> nothing specific to the ICH here.
>
> But yes, this is a helper routine and devices where it hurts
> instead of helps should have their own routine.  Complex devices
> need to handle the device-specific config space state above these
> 1st 16 locations anyway.

Right, and because we only restore the first 0x40 or so registers (they're all
standard), I don't think implementing pci_save/restore_state() properly 
is going
to break much at all.  I'm planning to merge these changes with -mm, so
hopefully it won't be difficult to tell. :)

Re: devices without drivers

We can certainly call a generic save and restore function when there isn't a
driver available, but beyond handling standard registers mentioned in 
the spec,
if important hardware context is lost there's really no way around it.

-Adam