[U-Boot] [PATCH 7/9] net: tsec: Use portable types and accessors for BDs

[Claudiu Manoil]

On 10/1/2013 2:22 AM, Scott Wood wrote:
> On Mon, 2013-09-30 at 12:44 +0300, Claudiu Manoil wrote:
>> +static RTXBD rtx __aligned(8);
>> +#define RXBD(i) rtx.rxbd[i]
>> +#define TXBD(i) rtx.txbd[i]
>> +#define GET_BD_STAT(T, i) be16_to_cpu((__force __be16)T##BD(i).status)
>> +#define SET_BD_STAT(T, i, v) T##BD(i).status = (__force __u16)cpu_to_be16(v)
>> +#define GET_BD_BLEN(T, i) be16_to_cpu((__force __be16)T##BD(i).length)
>> +#define SET_BD_BLEN(T, i, v) T##BD(i).length = (__force __u16)cpu_to_be16(v)
>> +#define GET_BD_BPTR(T, i) be32_to_cpu((__force __be32)T##BD(i).bufptr)
>> +#define SET_BD_BPTR(T, i, v) T##BD(i).bufptr = (__force __u32)cpu_to_be32(v)
>
> Why the forcing?  Can't you declare the data with __be16/__be32?

The __force annotation is obviously needed to suppress the sparse
warnings due to BD data declaration type not being __bexx, but the
generic uintxx_t, as you noticed as well.
Now, why I didn't use __bexx instead?  The main reason would be
maintainability: the DMA descriptors may not be in big endian format
exclusively.  The eTSEC hw may actually have an option to interpret
the DMA descriptors in little endian format.  If we decide to use
that option for future platforms, then the __bexx type wouldn't be
correct anymore.
Besides, I noticed that most drivers prefer to use the generic type
uintxx_t for buffer descriptors, instead of the annotated __bexx/__lexx
types.

>
>>   #else
>>   #error "rtx must be 64-bit aligned"
>>   #endif
>> @@ -275,10 +283,11 @@ void redundant_init(struct eth_device *dev)
>>   	clrbits_be32(&regs->dmactrl, DMACTRL_GRS | DMACTRL_GTS);
>>
>>   	do {
>> +		uint16_t status;
>>   		tsec_send(dev, (void *)pkt, sizeof(pkt));
>>
>>   		/* Wait for buffer to be received */
>> -		for (t = 0; rtx.rxbd[rx_idx].status & RXBD_EMPTY; t++) {
>> +		for (t = 0; GET_BD_STAT(RX, rx_idx) & RXBD_EMPTY; t++) {
>
> What's wrong with:
>
> for (t = 0; in_be16(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
>
> Or if you don't want to use I/O accessors on the DMA descriptors (Is
> synchronization ensured some other way?  We had problems with this in
> the Linux driver before...):
>

Synchronization here is is insured by declaring the RTXBD structure
type as "volatile" (see RTXBD declaration, a couple of lines above).
The existing code has been working this way for quite a while on the
mpc85xx platforms, so I thought it would be better not to change this
approach.  Using i/o accessors for the Buffer Descriptors would be a
significant change, and I don't see how to argue such a change: why
would the I/O accessors be better than the current approach - i.e.
declaring the DMA descriptors as volatile?  Is there something wrong
with about volatile usage in this case? (afaik, this is a case were
the volatile declaration is permitted)

Also, there doesn't seem to be other net drivers using I/O accessors
for the BD rings.

> for (t = 0; be16_to_cpup(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
>

I opted for using macros instead due to code maintainability, and to
avoid overly long lines (80) and to better control these changes.
For instance, if etsec were to access it's BDs in little endian format
in the future, then it would be enough to update the implementation
of the macro accessors without touching the whole code.

> -Scott
>

Thanks for reviewing this.

Regards,
Claudiu