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.=A0 Strange, because I
thought I had hand-inspected that patch...=A0 Ah, I see: I copied the
wrong patch, which was done against linux-2.6, rather than my internal
tree.=A0 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:

quoted

Andy,

 >
 > Some feedback:
 >
 > * Any reason to get ride of the 604 code now,w/o replacing it with

quoted

something?

Well, it doesn't really do much, and I was told that no one was using
it.=A0 If people want, I can add it back.

quoted

* Why have you introduce two CPU features (CPU_FTR_CAN_USE_PMON_INTR=20=

&

quoted

CPU_FTR_FSL_BOOKE_PMON) you dont use ?

Ah, this is for the near future, when I implement support for 74xx
processors.=A0 The non-Freescale-Book-E parts use a different perfmon
scheme, and I wanted a way to distinguish.=A0 It may not be useful,
though, I agree, since I am using CONFIG_FSL_BOOKE to do this.=A0
CAN_USE_PMON_INTR is important, though, to detect the errata which
could send the processor into never-neverland.=A0 Eventually, some =

74xx

processors will have this bit set, while some will not.

Ok, add this when you add the code, not before then.

quoted

* 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.=A0 I will change this if no one thinks of a good =

reason

to do otherwise.

quoted

* Does arch/ppc/kernel/perfmon.c really need all the headers you are
including?

Probably not.=A0 Does anyone know an easy way to determine which =

headers

 are needed?

quoted

* 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.=A0 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=20
separating the files. We are not going to have a single kernel image=20
that supports both book-e and classic PPC so we can make this a=20
compile/makefile decision instead of #ifdef'd.

quoted

* 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?=A0 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?=A0 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=20=

comments.

quoted

* 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=20=

are a number of SMP systems there.

[snip]

- kumar=