Re: new PCI quirk for Toshiba Satellite?

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

Jesse Barnes wrote:
> Stefan, is a PCI quirk addition possible or do we have to use 
> dmi_check_system in the ohci driver itself (since we have to reprogram 
> the cache line size in addition to the other registers)?

I am not familiar with the PCI subsystem, thus cannot advise how to 
handle it best nor wanted to post a patch myself (yet).

[...]
>>		.callback = ohci1394_toshiba_reprogram_config,
>>		.ident = "Toshiba PSM4 based laptop",
>>		.matches = {
>>			DMI_MATCH(DMI_SYS_VENDOR, "TOSHIBA"),
>>			DMI_MATCH(DMI_PRODUCT_VERSION, "PSM4"),
>>		},
>>		.driver_data = &tosh_data;

It seems to me, using the .callback and .driver_data doesn't make it 
cleaner and leaner.

> But then what about the dev->current_state = 4?  Is that necessary?

It is necessary; at least if the workaround resides in ohci1394. 
Otherwise the controller won't come back after a suspend/ resume cycle. 
(See Rob's post from February, 
http://marc.theaimsgroup.com/?m=110786495210243 ) Maybe there is another 
way to do that if the workaround was moved to pci/quirks.c.

[...]
> +	if (toshiba) {
> +		dev->current_state = 4;
> +		pci_read_config_word(dev, PCI_CACHE_LINE_SIZE, &toshiba_data);
> +	}
> +
>          if (pci_enable_device(dev))
>  		FAIL(-ENXIO, "Failed to enable OHCI hardware");
>          pci_set_master(dev);
>  
> +	if (toshiba) {
> +		mdelay(10);
> +		pci_write_config_word(dev, PCI_CACHE_LINE_SIZE, toshiba_data);
[...]

pci_set_master(dev) can be moved below the second part of the Toshiba 
workaround. That means AFAIU, the 2nd part of the Toshiba workaround can 
be moved out of ohci1394 into pci_fixup_device() which is called from 
pci_enable_device(), to be called as a DECLARE_PCI_FIXUP_ENABLE hook.

The first part of the workaround, i.e. caching the cache line size, for 
example by means of a static variable, would have to go into an 
_FIXUP_EARLY, _FIXUP_HEADER, or _FIXUP_FINAL hook. I am not sure yet 
about which type of hook to use.

Furthermore, everything which belongs to the workaround should IMO be 
enclosed by #ifdef SOME_SENSIBLE_MACRO. This avoids kernel bloat for any 
target which is surely not a Toshiba laptop. Rob used an #if 
defined(__i386__).
-- 
Stefan Richter
-=====-=-=-= =-=- =-=-=
http://arcgraph.de/sr/