[PATCH v2] powerpc: add setmaskedbits macros

[PATCH v2] powerpc: add setmaskedbits macros

[Timur Tabi]

This patch adds the setmaskedbits_xxx() macros, which are used to set a
multiple-bit bit pattern in a register.  The macros include a mask, which
zeros the respective bits before applying the value via a bitwise-OR.
There are big-endian and little-endian versions for 8, 16, 32, and 64 bits.

These new macros are useful because the setbits macros can only be used 
to set single-bit fields.  For example, if you have a 32-bit register 
where bits 17-20 need to be set to 0100, you would do  
setmaskedbits_be32(p, 4 << 11, 0xF << 11).

Signed-off-by: Timur Tabi <timur@freescale.com>
---

Updated the changelog to include a reason why you'd want these macros.
I have a number of new SOC device drivers coming up that will use these
macros, if this patch is accepted.

 include/asm-powerpc/io.h |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/include/asm-powerpc/io.h b/include/asm-powerpc/io.h
index bb8d965..ac3defb 100644
--- a/include/asm-powerpc/io.h
+++ b/include/asm-powerpc/io.h
@@ -734,6 +734,19 @@ static inline void * bus_to_virt(unsigned long address)
 #define setbits16(_addr, _v) out_be16((_addr), in_be16(_addr) |  (_v))
 #define clrbits16(_addr, _v) out_be16((_addr), in_be16(_addr) & ~(_v))
 
+#ifdef __powerpc64__
+#define setmaskedbits_be64(a, v, m) out_be64((a), (in_be64(a) & ~(m)) | (v))
+#define setmaskedbits_le64(a, v, m) out_le64((a), (in_le64(a) & ~(m)) | (v))
+#endif
+
+#define setmaskedbits_be32(a, v, m) out_be32((a), (in_be32(a) & ~(m)) | (v))
+#define setmaskedbits_be16(a, v, m) out_be16((a), (in_be16(a) & ~(m)) | (v))
+
+#define setmaskedbits_le32(a, v, m) out_le32((a), (in_le32(a) & ~(m)) | (v))
+#define setmaskedbits_le16(a, v, m) out_le16((a), (in_le16(a) & ~(m)) | (v))
+
+#define setmaskedbits_8(a, v, m) out_8((a), (in_8(a) & ~(m)) | (v))
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_POWERPC_IO_H */
-- 
1.5.2.4

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Kumar Gala]

On Aug 15, 2007, at 4:30 PM, Timur Tabi wrote:

> This patch adds the setmaskedbits_xxx() macros, which are used to  
> set a
> multiple-bit bit pattern in a register.  The macros include a mask,  
> which
> zeros the respective bits before applying the value via a bitwise-OR.
> There are big-endian and little-endian versions for 8, 16, 32, and  
> 64 bits.
>
> These new macros are useful because the setbits macros can only be  
> used
> to set single-bit fields.  For example, if you have a 32-bit register
> where bits 17-20 need to be set to 0100, you would do
> setmaskedbits_be32(p, 4 << 11, 0xF << 11).
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> Updated the changelog to include a reason why you'd want these macros.
> I have a number of new SOC device drivers coming up that will use  
> these
> macros, if this patch is accepted.

Can you post a driver that uses this.  I'm interested in seeing what  
the readability is really like using these macros.

- k

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2687 bytes --]

On Wed, 2007-08-15 at 16:30 -0500, Timur Tabi wrote:
> This patch adds the setmaskedbits_xxx() macros, which are used to set a
> multiple-bit bit pattern in a register.  The macros include a mask, which
> zeros the respective bits before applying the value via a bitwise-OR.
> There are big-endian and little-endian versions for 8, 16, 32, and 64 bits.
> 
> These new macros are useful because the setbits macros can only be used 
> to set single-bit fields.  For example, if you have a 32-bit register 
> where bits 17-20 need to be set to 0100, you would do  
> setmaskedbits_be32(p, 4 << 11, 0xF << 11).
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> 
> Updated the changelog to include a reason why you'd want these macros.
> I have a number of new SOC device drivers coming up that will use these
> macros, if this patch is accepted.
> 
>  include/asm-powerpc/io.h |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/include/asm-powerpc/io.h b/include/asm-powerpc/io.h
> index bb8d965..ac3defb 100644
> --- a/include/asm-powerpc/io.h
> +++ b/include/asm-powerpc/io.h
> @@ -734,6 +734,19 @@ static inline void * bus_to_virt(unsigned long address)
>  #define setbits16(_addr, _v) out_be16((_addr), in_be16(_addr) |  (_v))
>  #define clrbits16(_addr, _v) out_be16((_addr), in_be16(_addr) & ~(_v))
>  
> +#ifdef __powerpc64__
> +#define setmaskedbits_be64(a, v, m) out_be64((a), (in_be64(a) & ~(m)) | (v))
> +#define setmaskedbits_le64(a, v, m) out_le64((a), (in_le64(a) & ~(m)) | (v))
> +#endif
> +
> +#define setmaskedbits_be32(a, v, m) out_be32((a), (in_be32(a) & ~(m)) | (v))
> +#define setmaskedbits_be16(a, v, m) out_be16((a), (in_be16(a) & ~(m)) | (v))
> +
> +#define setmaskedbits_le32(a, v, m) out_le32((a), (in_le32(a) & ~(m)) | (v))
> +#define setmaskedbits_le16(a, v, m) out_le16((a), (in_le16(a) & ~(m)) | (v))
> +
> +#define setmaskedbits_8(a, v, m) out_8((a), (in_8(a) & ~(m)) | (v))

Can you extract the masking logic, rather than repeating it 7 times:

#define maskbits(a, v, m)	((a) & ~(m) | (v))

And if you're going to the trouble of making a macro, why not make it a
bit more useful and have it check that the value and the mask match, ie:

(v & ~m == 0)

Final random thought, you could make the size/endian an argument:

#define setmaskedbits(a, v, m, s)  out_##s((a), (in_##s(a) & ~(m) | (v))

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1282 bytes --]

On Thu, 16 Aug 2007, Michael Ellerman wrote:
> Can you extract the masking logic, rather than repeating it 7 times:
> 
> #define maskbits(a, v, m)	((a) & ~(m) | (v))
> 
> And if you're going to the trouble of making a macro, why not make it a
> bit more useful and have it check that the value and the mask match, ie:
> 
> (v & ~m == 0)
> 
> Final random thought, you could make the size/endian an argument:
> 
> #define setmaskedbits(a, v, m, s)  out_##s((a), (in_##s(a) & ~(m) | (v))

For readability, you can change the parameter names `v' and `m' to e.g. `set'
and `clear' (or `clr'), so people don't have to look through the actual
definitions to know which one comes first?

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Timur Tabi]

Kumar Gala wrote:

> Can you post a driver that uses this.  I'm interested in seeing what the 
> readability is really like using these macros.

I do not yet have any drivers ready, but I can give you an example:

	setmaskedbits_be32(&uccp->gumr_l,
		UCC_SLOW_GUMR_L_MODE_QMC | UCC_SLOW_GUMR_L_TDCR_1 |
		UCC_SLOW_GUMR_L_RDCR_16,
                 UCC_SLOW_GUMR_L_MODE_MASK | UCC_SLOW_GUMR_L_TDCR_MASK |
		UCC_SLOW_GUMR_L_RDCR_MASK);

	setmaskedbits_be32(&uccp->gumr_h,
		UCC_SLOW_GUMR_H_SUART | UCC_SLOW_GUMR_H_TRX |
		UCC_SLOW_GUMR_H_TTX, UCC_SLOW_GUMR_H_RFW);

The alternative to using a macro like this is to do this:

	reg = in_be32(p);
	reg &= ~some-mask-bits;
	reg |= some-other-bits;
	out_be32(p, reg);

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Kumar Gala]

On Aug 16, 2007, at 10:18 AM, Timur Tabi wrote:

> Kumar Gala wrote:
>
>> Can you post a driver that uses this.  I'm interested in seeing  
>> what the readability is really like using these macros.
>
> I do not yet have any drivers ready, but I can give you an example:
>
> 	setmaskedbits_be32(&uccp->gumr_l,
> 		UCC_SLOW_GUMR_L_MODE_QMC | UCC_SLOW_GUMR_L_TDCR_1 |
> 		UCC_SLOW_GUMR_L_RDCR_16,
>                 UCC_SLOW_GUMR_L_MODE_MASK |  
> UCC_SLOW_GUMR_L_TDCR_MASK |
> 		UCC_SLOW_GUMR_L_RDCR_MASK);
>
> 	setmaskedbits_be32(&uccp->gumr_h,
> 		UCC_SLOW_GUMR_H_SUART | UCC_SLOW_GUMR_H_TRX |
> 		UCC_SLOW_GUMR_H_TTX, UCC_SLOW_GUMR_H_RFW);

Can you also show what the UCC_SLOW* values look like.

> The alternative to using a macro like this is to do this:
>
> 	reg = in_be32(p);
> 	reg &= ~some-mask-bits;
> 	reg |= some-other-bits;
> 	out_be32(p, reg);
>
> -- 
> Timur Tabi
> Linux Kernel Developer @ Freescale

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Timur Tabi]

Kumar Gala wrote:

>>     setmaskedbits_be32(&uccp->gumr_l,
>>         UCC_SLOW_GUMR_L_MODE_QMC | UCC_SLOW_GUMR_L_TDCR_1 |
>>         UCC_SLOW_GUMR_L_RDCR_16,
>>                 UCC_SLOW_GUMR_L_MODE_MASK | UCC_SLOW_GUMR_L_TDCR_MASK |
>>         UCC_SLOW_GUMR_L_RDCR_MASK);
>>
>>     setmaskedbits_be32(&uccp->gumr_h,
>>         UCC_SLOW_GUMR_H_SUART | UCC_SLOW_GUMR_H_TRX |
>>         UCC_SLOW_GUMR_H_TTX, UCC_SLOW_GUMR_H_RFW);
> 
> Can you also show what the UCC_SLOW* values look like.

The second example is actually a trick that lets me set some bits and clear 
others in one shot, so for the gumr_h register, all of the above values are 
single bits.  I guess that's not a good example.

For gumr_l, we have:

#define UCC_SLOW_GUMR_L_TDCR_MASK	0x00030000
#define UCC_SLOW_GUMR_L_TDCR_32	        0x00030000
#define UCC_SLOW_GUMR_L_TDCR_16	        0x00020000
#define UCC_SLOW_GUMR_L_TDCR_8	        0x00010000
#define UCC_SLOW_GUMR_L_TDCR_1	        0x00000000
#define UCC_SLOW_GUMR_L_RDCR_MASK	0x0000c000
#define UCC_SLOW_GUMR_L_RDCR_32		0x0000c000
#define UCC_SLOW_GUMR_L_RDCR_16	        0x00008000
#define UCC_SLOW_GUMR_L_RDCR_8	        0x00004000
#define UCC_SLOW_GUMR_L_RDCR_1		0x00000000
#define UCC_SLOW_GUMR_L_MODE_MASK	0x0000000F
#define UCC_SLOW_GUMR_L_MODE_BISYNC	0x00000008
#define UCC_SLOW_GUMR_L_MODE_AHDLC	0x00000006
#define UCC_SLOW_GUMR_L_MODE_UART	0x00000004
#define UCC_SLOW_GUMR_L_MODE_QMC	0x00000002

So for example, I could not use setbits32 to set UCC_SLOW_GUMR_L_RDCR_16 (10) 
if it was currently UCC_SLOW_GUMR_L_RDCR_32 (11).


-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Timur Tabi]

Michael Ellerman wrote:

> And if you're going to the trouble of making a macro, why not make it a
> bit more useful and have it check that the value and the mask match, ie:
> 
> (v & ~m == 0)

What should I do if it fails this check?

> Final random thought, you could make the size/endian an argument:
> 
> #define setmaskedbits(a, v, m, s)  out_##s((a), (in_##s(a) & ~(m) | (v))

Hmmm.... the only thing wrong with that is that it would allow be64 on a 
32-bit platform (i.e. no __power64__ protection).  But that's minor.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Benjamin Herrenschmidt]

On Thu, 2007-08-16 at 10:26 -0500, Timur Tabi wrote:
> Kumar Gala wrote:
> 
> >>     setmaskedbits_be32(&uccp->gumr_l,
> >>         UCC_SLOW_GUMR_L_MODE_QMC | UCC_SLOW_GUMR_L_TDCR_1 |
> >>         UCC_SLOW_GUMR_L_RDCR_16,
> >>                 UCC_SLOW_GUMR_L_MODE_MASK | UCC_SLOW_GUMR_L_TDCR_MASK |
> >>         UCC_SLOW_GUMR_L_RDCR_MASK);
> >>
> >>     setmaskedbits_be32(&uccp->gumr_h,
> >>         UCC_SLOW_GUMR_H_SUART | UCC_SLOW_GUMR_H_TRX |
> >>         UCC_SLOW_GUMR_H_TTX, UCC_SLOW_GUMR_H_RFW);
> > 
> > Can you also show what the UCC_SLOW* values look like.
> 
> The second example is actually a trick that lets me set some bits and clear 
> others in one shot, so for the gumr_h register, all of the above values are 
> single bits.  I guess that's not a good example.

Such tricks deserve at least a comment.

Ben.

Re: [PATCH v2] powerpc: add setmaskedbits macros

[Timur Tabi]

Benjamin Herrenschmidt wrote:

>> The second example is actually a trick that lets me set some bits and clear 
>> others in one shot, so for the gumr_h register, all of the above values are 
>> single bits.  I guess that's not a good example.
> 
> Such tricks deserve at least a comment.

Would it be better if the function were called setclrbits_xxx(), and advertise 
that it can be used to set multi-bit fields *and* set and clear bits at the 
same time?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale