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(®s->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