fix the problem where pcix node is probed again as pci node.

From: Feng Kan <fkan@appliedmicro.com>

The current matching scheme make the pci node match to pcix or pciex node.
To avoid the match, change the method so only one type of initialization
is called per node.

Signed-off-by: Feng Kan <fkan@appliedmicro.com>
Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
---
 arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
> From: Feng Kan <fkan@appliedmicro.com>
> 
> The current matching scheme make the pci node match to pcix or pciex node.
> To avoid the match, change the method so only one type of initialization
> is called per node.

No, your patch is not right. The problem was introduced by a patch from
Grant that incorrectly made of_device_is_compatible do a substring
match. Grant should have fixed that now. Grant ? Is your fix upstream
yet ? If not, can you send that ASAP ?

Cheers,
Ben.


> Signed-off-by: Feng Kan <fkan@appliedmicro.com>
> Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
> ---
>  arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
> index 8aa3302..1e67c74 100644
> --- a/arch/powerpc/sysdev/ppc4xx_pci.c
> +++ b/arch/powerpc/sysdev/ppc4xx_pci.c
> @@ -1842,14 +1842,16 @@ static int __init ppc4xx_pci_find_bridges(void)
>  
>  	ppc_pci_flags |= PPC_PCI_ENABLE_PROC_DOMAINS | PPC_PCI_COMPAT_DOMAIN_0;
>  
> +	for_each_compatible_node(np, NULL, "ibm,plb-pci") {
> +		if (of_device_is_compatible(np, "ibm,plb-pcix"))
> +			ppc4xx_probe_pcix_bridge(np);
>  #ifdef CONFIG_PPC4xx_PCI_EXPRESS
> -	for_each_compatible_node(np, NULL, "ibm,plb-pciex")
> -		ppc4xx_probe_pciex_bridge(np);
> +		else if (of_device_is_compatible(np, "ibm,plb-pciex"))
> +			ppc4xx_probe_pciex_bridge(np);
>  #endif
> -	for_each_compatible_node(np, NULL, "ibm,plb-pcix")
> -		ppc4xx_probe_pcix_bridge(np);
> -	for_each_compatible_node(np, NULL, "ibm,plb-pci")
> -		ppc4xx_probe_pci_bridge(np);
> +		else
> +			ppc4xx_probe_pci_bridge(np);
> +	}
>  
>  	return 0;
>  }

On Wed, 2010-03-31 at 07:48 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
> > From: Feng Kan <fkan@appliedmicro.com>
> > 
> > The current matching scheme make the pci node match to pcix or pciex node.
> > To avoid the match, change the method so only one type of initialization
> > is called per node.
> 
> No, your patch is not right. The problem was introduced by a patch from
> Grant that incorrectly made of_device_is_compatible do a substring
> match. Grant should have fixed that now. Grant ? Is your fix upstream
> yet ? If not, can you send that ASAP ?

Better if I CC him too :-)

Cheers,
Ben.

> Cheers,
> Ben.
> 
> 
> > Signed-off-by: Feng Kan <fkan@appliedmicro.com>
> > Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
> > ---
> >  arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
> >  1 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
> > index 8aa3302..1e67c74 100644
> > --- a/arch/powerpc/sysdev/ppc4xx_pci.c
> > +++ b/arch/powerpc/sysdev/ppc4xx_pci.c
> > @@ -1842,14 +1842,16 @@ static int __init ppc4xx_pci_find_bridges(void)
> >  
> >  	ppc_pci_flags |= PPC_PCI_ENABLE_PROC_DOMAINS | PPC_PCI_COMPAT_DOMAIN_0;
> >  
> > +	for_each_compatible_node(np, NULL, "ibm,plb-pci") {
> > +		if (of_device_is_compatible(np, "ibm,plb-pcix"))
> > +			ppc4xx_probe_pcix_bridge(np);
> >  #ifdef CONFIG_PPC4xx_PCI_EXPRESS
> > -	for_each_compatible_node(np, NULL, "ibm,plb-pciex")
> > -		ppc4xx_probe_pciex_bridge(np);
> > +		else if (of_device_is_compatible(np, "ibm,plb-pciex"))
> > +			ppc4xx_probe_pciex_bridge(np);
> >  #endif
> > -	for_each_compatible_node(np, NULL, "ibm,plb-pcix")
> > -		ppc4xx_probe_pcix_bridge(np);
> > -	for_each_compatible_node(np, NULL, "ibm,plb-pci")
> > -		ppc4xx_probe_pci_bridge(np);
> > +		else
> > +			ppc4xx_probe_pci_bridge(np);
> > +	}
> >  
> >  	return 0;
> >  }
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Ok thanks. This short string match may be useful in some cases, but I  
agree it plays havoc with the current code.

Feng Kan

On Mar 30, 2010, at 14:14, "Benjamin Herrenschmidt" <benh@kernel.crashing.org 
 > wrote:

> On Wed, 2010-03-31 at 07:48 +1100, Benjamin Herrenschmidt wrote:
>> On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
>>> From: Feng Kan <fkan@appliedmicro.com>
>>>
>>> The current matching scheme make the pci node match to pcix or  
>>> pciex node.
>>> To avoid the match, change the method so only one type of  
>>> initialization
>>> is called per node.
>>
>> No, your patch is not right. The problem was introduced by a patch  
>> from
>> Grant that incorrectly made of_device_is_compatible do a substring
>> match. Grant should have fixed that now. Grant ? Is your fix upstream
>> yet ? If not, can you send that ASAP ?
>
> Better if I CC him too :-)
>
> Cheers,
> Ben.
>
>> Cheers,
>> Ben.
>>
>>
>>> Signed-off-by: Feng Kan <fkan@appliedmicro.com>
>>> Signed-off-by: Tirumala R Marri <tmarri@appliedmicro.com>
>>> ---
>>> arch/powerpc/sysdev/ppc4xx_pci.c |   14 ++++++++------
>>> 1 files changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/ 
>>> sysdev/ppc4xx_pci.c
>>> index 8aa3302..1e67c74 100644
>>> --- a/arch/powerpc/sysdev/ppc4xx_pci.c
>>> +++ b/arch/powerpc/sysdev/ppc4xx_pci.c
>>> @@ -1842,14 +1842,16 @@ static int __init ppc4xx_pci_find_bridges 
>>> (void)
>>>
>>>    ppc_pci_flags |= PPC_PCI_ENABLE_PROC_DOMAINS |  
>>> PPC_PCI_COMPAT_DOMAIN_0;
>>>
>>> +    for_each_compatible_node(np, NULL, "ibm,plb-pci") {
>>> +        if (of_device_is_compatible(np, "ibm,plb-pcix"))
>>> +            ppc4xx_probe_pcix_bridge(np);
>>> #ifdef CONFIG_PPC4xx_PCI_EXPRESS
>>> -    for_each_compatible_node(np, NULL, "ibm,plb-pciex")
>>> -        ppc4xx_probe_pciex_bridge(np);
>>> +        else if (of_device_is_compatible(np, "ibm,plb-pciex"))
>>> +            ppc4xx_probe_pciex_bridge(np);
>>> #endif
>>> -    for_each_compatible_node(np, NULL, "ibm,plb-pcix")
>>> -        ppc4xx_probe_pcix_bridge(np);
>>> -    for_each_compatible_node(np, NULL, "ibm,plb-pci")
>>> -        ppc4xx_probe_pci_bridge(np);
>>> +        else
>>> +            ppc4xx_probe_pci_bridge(np);
>>> +    }
>>>
>>>    return 0;
>>> }
>>
>>
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>

On Tue, Mar 30, 2010 at 3:14 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2010-03-31 at 07:48 +1100, Benjamin Herrenschmidt wrote:
>> On Tue, 2010-03-30 at 10:41 -0700, Feng Kan wrote:
>> > From: Feng Kan <fkan@appliedmicro.com>
>> >
>> > The current matching scheme make the pci node match to pcix or pciex node.
>> > To avoid the match, change the method so only one type of initialization
>> > is called per node.
>>
>> No, your patch is not right. The problem was introduced by a patch from
>> Grant that incorrectly made of_device_is_compatible do a substring
>> match. Grant should have fixed that now. Grant ? Is your fix upstream
>> yet ? If not, can you send that ASAP ?
>
> Better if I CC him too :-)

The fix is in upstream.  Commit 1976152fd8e706135deed6cf333e347c08416056

g.

On Tue, 2010-03-30 at 15:02 -0700, Feng Kan wrote:
> Ok thanks. This short string match may be useful in some cases, but I
> agree it plays havoc with the current code.

Yeah well, it's not actually -that- useful and is definitely broken :-)

If you want multiples matches, then it's the compatible property itself
that should contain multiple entries since it's a list. The string match
should be exact.

Cheers,
Ben.