[U-Boot] [PATCH 1/5] Tegra2: Add bitfield access macros

[Wolfgang Denk]

Dear Simon Glass,

In message <1306973675-8411-2-git-send-email-sjg at chromium.org> you wrote:
> To use these, set things up like this:
> 
> struct uart_ctlr *uart = (struct uart_ctlr *)UART_PA_START;
> 
>  #define UART_PA_START 0x67000000       /* Physical address of UART */
>  #define UART_FBCON_RANGE  5:3          /* Bit range for the FBCON field */
> enum {                                  /* An enum with allowed values */
>         UART_FBCON_OFF,
>         UART_FBCON_ON,
>         UART_FBCON_MULTI,
>         UART_FBCON_SLAVE,
> };
> 
> This defines a bit field of 3 bits starting at bit 5 and extending down
> to bit 3, i.e. 5:3

Sorry, but this is a highly unportable, and completely unreadable.


> Then:
> bf_unpack(UART_FBCON)
>         - return the value of bits 5:3 (shifted down to bits 2:0)

OK.  Assuming reading from UART_FBCON would return the value
0x16000038 or 0b0001.0110.0000.0000.0000.0000.0011.1000.
What is the correct and expected return value from your macro?

You say 5:3 means "3 bits starting at bit 5 and extending down to bit 3".

With your ARM view of things you might expect the outcome to be
0x7 = 0b111 because for you bit 0 is the LSB, and your counting "from
left to right", and "down" for you means "in direction of less
significant bits".  But please note, that for the Power Architecture,
bit 0 denotes the MSB, so the _expected_ (from the documentation)
result of the macro would be 0x5 = 0b101, and your notion of "down to
bit 3" is completely unexpected as we are movin in the direction or
more significan bits, where "down" is simply wrong in my
understanding.

Your design probably does not consider that different architectures
have different understandings of how to number bits.

In addition, in portable C code there is no standardizartion for bit
fields: nothing about alignment, layout, padding, maximum bit
numbers, or anything. The whole implementationm is compiler-specific.


> + * Why have bitfield macros?
> + *   1. Reability
> + *   2. Maintainability
> + *   3. Less error prone

It would be nice if this worked, alas it doesn't. At least not as you
expect it.

> + * For example, this:
> + *
> + *	int RegVal = 0;
> + *	RegVal= readl(UsbBase+USB_SUSP_CTRL);
> + *	RegVal |= Bit11;
> + *	writel(RegVal, UsbBase+USB_SUSP_CTRL);
> + *	if(UsbBase == NV_ADDRESS_MAP_USB3_BASE)
> + *	{
> + *		RegVal = readl(UsbBase+USB_SUSP_CTRL);
> + *		RegVal |= Bit12;
> + *		writel(RegVal, UsbBase+USB_SUSP_CTRL);
> + *	}

Well, such code would be rejected right from the beginning.  We don;t
allow the use of "base address plus offset" to access structured data
like mapped device registers, instead we require the use of proper I/O
accessors.

> + * becomes this:
> + *
> + *	bitfield_writel(UTMIP_RESET, 1, &usbctlr->susp_ctrl);
> + *	if (id == PERIPH_ID_USB3)
> + *		bitfield_writel(UTMIP_PHY_ENB, 1, &usbctlr->susp_ctrl);

And in which way is this more readable, better maintainable, and/or
less error prone then when using the proper standard macros which are
ready available in U-Boot:

	setbits_le32(&usbctlr->susp_ctrl, UTMIP_RESET);
	if (id == PERIPH_ID_USB3)
		setbits_le32(&usbctlr->susp_ctrl, UTMIP_PHY_ENB)

Yes, you may need slightly different definitions of UTMIP_RESET and
UTMIP_PHY_ENB, but so what?



Sorry, but I am not going to accept this.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is impractical for  the  standard  to  attempt  to  constrain  the
behavior  of code that does not obey the constraints of the standard.
                                                          - Doug Gwyn