Re: [PATCH v4 1/6] nova-core: bitfield: Move bitfield-specific code from register! into new macro

[Joel Fernandes <joelagnelf@nvidia.com>]

On 9/29/2025 9:37 PM, Greg KH wrote:
> On Mon, Sep 29, 2025 at 03:26:57PM -0400, Joel Fernandes wrote:
>> On 9/24/2025 12:40 PM, Greg KH wrote:
>>> On Tue, Sep 23, 2025 at 06:24:34PM -0400, Joel Fernandes wrote:
>> [..]
>>>
>>>> For the separate issue of enforcing endianness with respect to (across)
>>>> multiple fields, I agree with you that if the user's backend (the consumer of
>>>> the data) is not doing such conversion, say via regmap, then that becomes a
>>>> problem. But that problem is orthogonal/different and cannot be solved here.  
>>>
>>> But that is exactly what these macros are being defined here for, so to
>>> ignore that is going to cause problems :)
>>>
>>
>> If needed, happy to add endianness support as needed by providing additional
>> options to the macro. Based on this thread, it sounds like we want see if that
>> is really needed here or can be solved elsewhere (?). The mental model I kind of
>> have is this macro should only be dealing with CPU native endianness, much like
>> bitfields in C deal with CPU endianness. Hmm.
> 
> Just don't go down the old path like drivers/net/fddi/skfp/h/supern_2.h
> does with it's definition of:

But this is not a comparable example though because in our macro we specify
individual bit numbers, not just bit width. So tx_length for example would
always start at the lower 2 bytes since we'd specify "tx_length 0:15" regardless
of whether those 2 bytes are higher in memory or lower in memory.  Whether it is
higher or lower depends on CPU endianness AFAICS, and if interacting with HW is
needed, the user of the macro would do endianness conversion. Maybe we should
add a comment about such conversion requirements?

Then there is the issue of the byte ordering within a multi-byte field. That
again follows CPU endianness, and we could add a comment for the benefit of
macro user.
> 
> 	union tx_descr {
> 		struct {
> 	#ifdef  LITTLE_ENDIAN
> 		unsigned int    tx_length:16 ;  /* frame length lower/upper byte */
> 		unsigned int    tx_res   :8 ;   /* reserved      (bit 16..23) */
> 		unsigned int    tx_xmtabt:1 ;   /* transmit abort */
> 		unsigned int    tx_nfcs  :1 ;   /* no frame check sequence */
> 		unsigned int    tx_xdone :1 ;   /* give up token */
> 		unsigned int    tx_rpxm  :2 ;   /* byte offset */
> 		unsigned int    tx_pat1  :2 ;   /* must be TXP1 */
> 		unsigned int    tx_more  :1 ;   /* more frame in chain */
> 	#else
> 		unsigned int    tx_more  :1 ;   /* more frame in chain */
> 		unsigned int    tx_pat1  :2 ;   /* must be TXP1 */
> 		unsigned int    tx_rpxm  :2 ;   /* byte offset */
> 		unsigned int    tx_xdone :1 ;   /* give up token */
> 		unsigned int    tx_nfcs  :1 ;   /* no frame check sequence */
> 		unsigned int    tx_xmtabt:1 ;   /* transmit abort */
> 		unsigned int    tx_res   :8 ;   /* reserved      (bit 16..23) */
> 		unsigned int    tx_length:16 ;  /* frame length lower/upper byte */
> 	#endif
> 		} t ;
> 		long    i ;
> 	} ;
> 

This is indeed yuck though but afaics not what hopefully we'd be doing ;-)

thanks,

 - Joel