On Nov 1, 2004, at 11:03 AM, Fleming Andy-afleming wrote: > Well, it looks like I messed up the patch, and included some stuff I'm > working on for generalizing the PHY interface.? Strange, because I > thought I had hand-inspected that patch...? Ah, I see: I copied the > wrong patch, which was done against linux-2.6, rather than my internal > tree.? Please ignore the patch, since much of that code is obsolete. > > Anyway, I will respond to Kumar's comments, and have attached the > CORRECT patch: > > On Oct 31, 2004, at 23:43, Kumar Gala wrote: > > > Andy, > > > > Some feedback: > > > > * Any reason to get ride of the 604 code now,w/o replacing it with > > something? > > Well, it doesn't really do much, and I was told that no one was using > it.? If people want, I can add it back. > > > * Why have you introduce two CPU features (CPU_FTR_CAN_USE_PMON_INTR > & > > CPU_FTR_FSL_BOOKE_PMON) you dont use ? > > Ah, this is for the near future, when I implement support for 74xx > processors.? The non-Freescale-Book-E parts use a different perfmon > scheme, and I wanted a way to distinguish.? It may not be useful, > though, I agree, since I am using CONFIG_FSL_BOOKE to do this.? > CAN_USE_PMON_INTR is important, though, to detect the errata which > could send the processor into never-neverland.? Eventually, some 74xx > processors will have this bit set, while some will not.
Ok, add this when you add the code, not before then. > > * In the PerformanceMonitor can't you use regs->nip for the same > > purpose of regs->sia? > > Hmmm.... somehow I missed this when I was looking for a way to get the > sampled address.? I will change this if no one thinks of a good reason > to do otherwise. > > > * Does arch/ppc/kernel/perfmon.c really need all the headers you are > > including? > > Probably not.? Does anyone know an easy way to determine which headers > are needed? > > > * Does perfmon.c make more sense as perfmon_fsl_booke.c > > Well, I thought about that, but I figured that it was ok to put all of > the code into one file, and use #ifdef to distinguish.? I'm willing to > divide it into two (one for FSL booke, and one for classic), though. Unless there is a fair amount of shared code, I would recommend separating the files. We are not going to have a single kernel image that supports both book-e and classic PPC so we can make this a compile/makefile decision instead of #ifdef'd. > > * op_ppc32_setup, I'd prefer the saving & restoring of the perf_irq > > like arch/ppc64/oprofile/common.c does > > Well, does this really make sense?? If one driver grabs the interrupt, > and a subsequent driver tries to grab it, do we want the second driver > to win, but nicely restore the handler later, or do we want the second > driver to fail, and give the user the chance to disable the other > driver?? I definitely won't use the method in ppc64, since that isn't > thread-safe if more than one driver wants the perfmon interrupt. Hmm, ok. I'm not sure what we should do here. Maybe others have some comments. > > * a number of issues related to SMP > > I admit, I haven't carefully worked this out, as there aren't any SMP > e500 systems out there. True, but if you are going share infrastructure with PPC classic, there are a number of SMP systems there. [snip] - kumar
