Re: [PATCH] ppc44x: Remove PVR_440G* and change usages

[Eugene Surovegin <ebs@ebshome.net>]

On Mon, Mar 21, 2005 at 09:40:49AM -0800, Eugene Surovegin wrote:

[snip]

> > -	u32 pvr = mfspr(PVR);
> > -	if (pvr == PVR_440GX_RA || pvr == PVR_440GX_RB ||
> > -	    (pvr == PVR_440GX_RC && p->cpu > 667000000))
> > +	if (strcmp(cur_cpu_spec[0]->cpu_name, "440GX Rev. A") == 0 ||
> > +			strcmp(cur_cpu_spec[0]->cpu_name, "440GX Rev. B") == 0
> > +			|| (strcmp(cur_cpu_spec[0]->cpu_name, "440GX Rev. C")
> > +				== 0 && p->cpu > 667000000))
> 
> 
> While I applaud Tom's quest to eliminate _useless_ PVR defines I think 
> this change looks strange.
> 
> It substitutes simple and clear (for those who are reading this code) 
> check to something more involved without good reason, IMHO.
> 
> Also, it adds additional "point of failure" making this code more 
> fragile. We never used those CPU ID strings anywhere in the kernel 
> code before, people are used to the fact that they don't matter much 
> (maybe only for user-mode) and it's possible that during some future 
> "cleanup" code will break. One could say that we aren't allowed to 
> change such strings because something in user-mode could break, and 
> while I agree with that, I don't think it's good argument to _add_ 
> additional point where code could break.

While talking with galak today on IRC, he mentioned one of the reason, 
why we may want to use string comparison instead of PVR.

For some chips, e.g. for 8272 family, there is NO way (even if we use 
IMMR[16-31] and REV_NUM in addition to PVR) to detect correctly CPU 
(which I think is pretty lame, but I digress :). For such cases, board 
port can "fixup" CPU name, if this information can be implied by the 
board type/revision/etc.

So, I think having a way to specify/name/detect CPU which is more 
general and even can solve some real problems, is beneficial. 
Therefore, I don't object anymore to such changes :).

--
Eugene