On Thursday 13 June 2013 11:49:17 Scott Wood wrote: > 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
Ok. I'll post a v2 of the patch. Rojhalat _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
