Re: [PATCH 1/6] MIPS: BMIPS: Disable pref 30 for buggy CPUs

["Maciej W. Rozycki" <macro@imgtec.com>]

On Tue, 9 Feb 2016, Florian Fainelli wrote:

> >> +static void bmips5000_pref30_quirk(void)
> >> +{
> >> +	__asm__ __volatile__(
> >> +	"	.word	0x4008b008\n"	/* mfc0 $8, $22, 8 */
> >> +	"	lui	$9, 0x0100\n"
> >> +	"	or	$8, $9\n"
> >> +	/* disable "pref 30" on buggy CPUs */
> >> +	"	lui	$9, 0x0800\n"
> >> +	"	or	$8, $9\n"
> >> +	"	.word	0x4088b008\n"	/* mtc0 $8, $22, 8 */
> >> +	: : : "$8", "$9");
> >> +}
> > 
> >  Ouch, why not using our standard accessors and avoid magic numbers, e.g.:
> 
> Are you positive the assembler will not barf on these custom cp0 reg 22
> selectors?

 Indeed, I missed that this is beyond the usual select range of 0-7.  
Sorry about that.

 That does not mean it shouldn't be done in a cleaner way, stashing the 
obscurity in one place only.  I did notice a similar piece in one of your 
other patches, which is a strong argument for abstracting it.

 So first, are you aware of support for these Broadcom instruction 
encoding extensions being considered for inclusion in binutils?  I'll be 
happy to accept a patch and AFAICT it's a simple extension of the `sel' 
field within the existing MFC0/MTC0 instruction definitions.

 Second, regardless we need to abstract this in a reusable way while we 
don't have such support in the assembler.  So here:

> > #define read_c0_brcm_whateverthisiscalled() \
> > 	__read_32bit_c0_register($22, 8)
> > #define write_c0_brcm_whateverthisiscalled(val) \
> > 	__write_32bit_c0_register($22, 8, val)

rather than using `__read_32bit_c0_register' and 
`__write_32bit_c0_register' we can define special 
`__read_32bit_c0_brcm_register' and `__write_32bit_c0_brcm_register' 
helpers like:

#define __read_32bit_c0_brcm_register(reg, sel)				\
({									\
	register unsigned int __res asm("t0");				\
									\
	__asm__ __volatile__(						\
		/* mfc0 t0, $reg, sel */				\
		".word	0x40080000 | ((%1 & 0x1f) << 11) | (%2 & 0xf)"	\
		: "=r" (__res) : "i" (reg), "i" (sel));			\
	__res;								\
})

#define __write_32bit_c0_brcm_register(reg, sel, value)			\
do {									\
	register unsigned int __val asm("t0") = value;			\
									\
	__asm__ __volatile__(						\
		/* mtc0 t0, $reg, sel */				\
		".word  0x40880000 | ((%1 & 0x1f) << 11) | (%2 & 0xf)"	\
		: : "r" (__val), "i" (reg), "i" (sel));			\
} while (0)

(if 0xf is indeed the mask for `sel').  This is untested, but should work, 
perhaps with a minor fix somewhere if I made a typo.  It should also 
produce a little bit better code, although I realise this is a corner case 
hardly worth optimising for.  What is important is maintainability.

> > #define BMIPS5000_WHATEVERTHISISCALLED_BIT_X 0x0100
> > #define BMIPS5000_WHATEVERTHISISCALLED_BIT_Y 0x0800
> > 
> > static void bmips5000_pref30_quirk(void)
> > {
> > 	unsigned int whateverthisiscalled;
> > 
> > 	whateverthisiscalled = read_c0_brcm_whateverthisiscalled();
> > 	whateverthisiscalled |= BMIPS_WHATEVERTHISISCALLED_BIT_X;
> > 	/* disable "pref 30" on buggy CPUs */
> > 	whateverthisiscalled |= BMIPS_WHATEVERTHISISCALLED_BIT_Y;
> > 	write_c0_brcm_whateverthisiscalled(whateverthisiscalled);
> > }
> > 
> > ?  I'm leaving it up to you to select the right names here -- just about 
> > anything will be better than the halfway-binary piece you have proposed.
> 
> These are not standardized registers in any form, which is why these are
> in a halfway-binary form, they are not meant to be re-used outside of
> two known places: disabling pref30 and enabling "rotr".

 Well, if Broadcom didn't give this register any name, then `reg22_sel8' 
will be as good as any.  We don't need to invent things here, just to keep 
them maintainable.  If you call something `8', then you can't easily 
search through the tree to find references.  If you use something uniquely 
identifiable, then you can.  So:

#define read_c0_brcm_reg22_sel8()					\
	__read_32bit_c0_brcm_register(22, 8)
#define write_c0_brcm_reg22_sel8(val)					\
	__write_32bit_c0_brcm_register(22, 8, val)

(note the dropped `$' because we can't pass it along in this form).

 As to the bit names -- you already gave them: NO_PREF30 (since this is 
negated) and ROTR will be just fine, with a BMIPS5000_REG22_SEL8_ prefix.  
So:

#define BMIPS5000_REG22_SEL8_ROTR	0x0100
#define BMIPS5000_REG22_SEL8_NO_PREF30	0x0800

(why is ROTR set along NO_PREF30 BTW? -- it does not seem related).

 I hope you agree this all is reasonable from a maintainer's point of 
view.  I'll leave it up to you to make a patch out of it.  You'll then be 
able to use this stuff in 2/6 too.

 Please try to give meaningful names to the other magic numbers you 
introduce too.

  Maciej