Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX

On 06/13/2013 02:21:24 AM, Rojhalat Ibrahim wrote:

On Wednesday 12 June 2013 16:50:26 Scott Wood wrote:

quoted

On 06/12/2013 03:19:30 AM, Rojhalat Ibrahim wrote:

quoted

On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:

quoted

Yes, I figured it was non-PCIe because the code change that you =20

said

quoted

quoted

quoted

helped was on the non-PCIe branch of the if/else.  Generally =20

it's

quoted

quoted

good

quoted

to explicitly mention the chip you're using, though.

fsl_setup_indirect_pci should be renamed to =20

fsl_setup_indirect_pcie.

quoted

quoted

quoted

Your patch above should be applied, and fsl_setup_indirect_pcie

should

quoted

be moved into the booke/86xx ifdef to avoid an unused function

warning.

quoted

-Scott

How about this patch? It uses setup_indirect_pci for the PCI case =20

in

quoted

quoted

mpc83xx_add_bridge. Additionally it adds a check in
fsl_setup_indirect_pci
to only use the modified read function in case of PCIe.

If we're adding the check to fsl_setup_indirect_pci, there's no =20

need to

quoted

change the 83xx call back to setup_indirect_pci.  I see that 85xx is
also callirng fsl_setup_indirect_pci for both; it'd be good to be
consistent.

In any case, can you send a proper patch with a signoff and commit
message?

-Scott

=20
Where is it called for 85xx? As far as I can tell =20
fsl_setup_indirect_pci is
called exactly once in fsl_add_bridge and nowhere else (after =20
applying the
proposed patch).

fsl_add_bridge() is where it's called for 85xx.

For 83xx the decision between PCI and PCIe has already been made at
the point where the setup function is called. So IMO it doesn't make =20
sense
to call fsl_setup_indirect_pci and do the check again. Moreover PCIe =20
on 83xx
uses a completely different set of functions.

My concern is consistency.  E.g. if 85xx is using =20
fsl_setup_indirect_pci for both, but 83xx isn't, then a developer using =20
83xx could end up breaking 85xx by introducing another PCIe dependency =20
in fsl_setup_indirect_pci.  Or an 85xx developer could put something =20
non-PCIe-related in fsl_setup_indirect_pci that 83xx would benefit from.

Alternatively, you could call it fsl_setup_indirect_pcie, and move the =20
PCIe check into fsl_add_bridge().

-Scott=

On Thursday 13 June 2013 11:49:17 Scott Wood wrote:

On 06/13/2013 02:21:24 AM, Rojhalat Ibrahim wrote:

quoted

On Wednesday 12 June 2013 16:50:26 Scott Wood wrote:

quoted

On 06/12/2013 03:19:30 AM, Rojhalat Ibrahim wrote:

quoted

On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:

quoted

Yes, I figured it was non-PCIe because the code change that you

said

quoted

quoted

quoted

helped was on the non-PCIe branch of the if/else.  Generally

it's

quoted

quoted

good

quoted

to explicitly mention the chip you're using, though.

fsl_setup_indirect_pci should be renamed to

fsl_setup_indirect_pcie.

quoted

quoted

quoted

Your patch above should be applied, and fsl_setup_indirect_pcie

should

quoted

be moved into the booke/86xx ifdef to avoid an unused function

warning.

quoted

-Scott

How about this patch? It uses setup_indirect_pci for the PCI case

in

quoted

quoted

mpc83xx_add_bridge. Additionally it adds a check in
fsl_setup_indirect_pci
to only use the modified read function in case of PCIe.

If we're adding the check to fsl_setup_indirect_pci, there's no

need to

quoted

change the 83xx call back to setup_indirect_pci.  I see that 85xx is
also callirng fsl_setup_indirect_pci for both; it'd be good to be
consistent.

In any case, can you send a proper patch with a signoff and commit
message?

-Scott

Where is it called for 85xx? As far as I can tell
fsl_setup_indirect_pci is
called exactly once in fsl_add_bridge and nowhere else (after
applying the
proposed patch).

fsl_add_bridge() is where it's called for 85xx.

quoted

For 83xx the decision between PCI and PCIe has already been made at
the point where the setup function is called. So IMO it doesn't make
sense
to call fsl_setup_indirect_pci and do the check again. Moreover PCIe
on 83xx
uses a completely different set of functions.

My concern is consistency.  E.g. if 85xx is using
fsl_setup_indirect_pci for both, but 83xx isn't, then a developer using
83xx could end up breaking 85xx by introducing another PCIe dependency
in fsl_setup_indirect_pci.  Or an 85xx developer could put something
non-PCIe-related in fsl_setup_indirect_pci that 83xx would benefit from.

Alternatively, you could call it fsl_setup_indirect_pcie, and move the
PCIe check into fsl_add_bridge().

-Scott

Ok. I'll post a v2 of the patch.

   Rojhalat