Re: Problem with PCI bus rescan on 460EX

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

Felix Radensky wrote:
> Hello Kenji-san,
> 
> Kenji Kaneshige wrote:
>> Felix Radensky wrote:
>>> Hello Kenji-san,
>>>
>>> Kenji Kaneshige wrote:
>>>> I'm not sure, but I guess pci_setup_bridge() didn't update IO 
>>>> base/limit
>>>> and Mem base/limit of the bridge (0000:00:02.0) because of the 
>>>> following
>>>> lines.
>>>>
>>>> static void pci_setup_bridge(struct pci_bus *bus)
>>>> {
>>>>        struct pci_dev *bridge = bus->self;
>>>>        struct resource *res;
>>>>        struct pci_bus_region region;
>>>>        u32 l, bu, lu, io_upper16;
>>>>
>>>>        if (pci_is_enabled(bridge))    
>>>> <===============================                              
>>>> return;            <===============================
>>>>
>>>>        dev_info(&bridge->dev, "PCI bridge to [bus %02x-%02x]\n",
>>>>                 bus->secondary, bus->subordinate);
>>>>     ...
>>>>
>>>> It seems the bridge was already enabled by 
>>>> pci_assign_unassigned_resources()
>>>> at boot time. Does removing those lines make any change?
>>>>
>>>
>>> Yes, with these lines removed bridge memory window is properly 
>>> allocated.
>>
>> These lines are to prevent updating IO or memory windows when there are
>> some devices working behind the bridge. So please note that removing
>> these lines is just for debugging.
>>
>>
>>> For some reason bridge memory is disabled, but if I enable it via 
>>> setpci, and
>>> also enable device memory, then everything works fine. If the system 
>>> is booted
>>> when device behind the bridge is plugged in, bridge memory is enabled.
>>>
>>
>> Maybe because of the following lines, I guess.
>>
>> void pci_enable_bridges(struct pci_bus *bus)
>> {
>>        struct pci_dev *dev;
>>        int retval;
>>
>>        list_for_each_entry(dev, &bus->devices, bus_list) {
>>                if (dev->subordinate) {
>>                        if (!pci_is_enabled(dev)) {        <=======
>>                                retval = pci_enable_device(dev);<=======
>>                                pci_set_master(dev);        <=======
>>                        }
>>         ...
>>
>>
> 
> Yes, but removing this check is not enough. The bridge is enabled after 
> first scan, but it's
> memory is disabled. So simply calling  pci_enable_device() will not 
> help, it will return without
> enabling memory. I had to call  pci_disable_device() before 
> pci_enable_device() to make things
> work.
>> One possible cause is that pcibios_enable_device() for the bridge didn't
>> return any error even though it didn't work properly (ex. cannot access
>> to the command register, and so on) on your platform when there is no
>> device behind the bridge. But it is just my guess.
> Should pcibios_enable_device() return an error if it fails to enable 
> bridge memory ?
> 

I think the device is expected to be ready to work if pci_enable_device()
returns without error. So I think pci_enable_device() should return an
error if it fails to enable the device (device is not ready to work). In
this case, detecting your bridge's failure seems PPC specific to me. So I
thought pcibios_enable_device() was the right to return an error. If
pcibios_enable_device() returned an error, pci_dev->enable_cnt would
decremented by pci_enable_device() (like pci_disable_device() does) and
this problem would not happen.

On the other hand, as Ben suggested, handling this by specific hot-plug
driver would be one of the other candidate to fix the problem.

Thanks,
Kenji Kaneshige