Date: Thu, 13 Jun 2013 11:49:17 -0500
From: Scott Wood <scottwood@freescale.com>
Subject: Re: [BUG] PCI related panic on powerpc based board with 3.10-rcX
To: Rojhalat Ibrahim <imr@rtschenk.de>
CC: Michael Guntsche <michael.guntsche@it-loops.com>,
        <linuxppc-dev@lists.ozlabs.org>, <linux-kernel@vger.kernel.org>
References: <1370971739.18413.27@snotra> <34279395.MbRViMjbAR@pcimr>
	<1371073826.18413.52@snotra> <7706620.VNI3PE9pqO@pcimr>
In-Reply-To: <7706620.VNI3PE9pqO@pcimr> (from imr@rtschenk.de on Thu Jun 13
	02:21:24 2013)
Message-ID: <1371142157.2028.9@snotra>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; delsp=Yes; format=Flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2562
Lines: 72

On 06/13/2013 02:21:24 AM, Rojhalat Ibrahim wrote:
> On Wednesday 12 June 2013 16:50:26 Scott Wood wrote:
> > On 06/12/2013 03:19:30 AM, Rojhalat Ibrahim wrote:
> > > On Tuesday 11 June 2013 12:28:59 Scott Wood wrote:
> > > > Yes, I figured it was non-PCIe because the code change that you  
> said
> > > > helped was on the non-PCIe branch of the if/else.  Generally  
> it's
> > >
> > > good
> > >
> > > > to explicitly mention the chip you're using, though.
> > > >
> > > > fsl_setup_indirect_pci should be renamed to  
> fsl_setup_indirect_pcie.
> > > > Your patch above should be applied, and fsl_setup_indirect_pcie
> > >
> > > should
> > >
> > > > be moved into the booke/86xx ifdef to avoid an unused function
> > >
> > > warning.
> > >
> > > > -Scott
> > >
> > > How about this patch? It uses setup_indirect_pci for the PCI case  
> in
> > > 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
> > 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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/