On 10/1/2013 9:50 PM, Scott Wood wrote:
On Tue, 2013-10-01 at 14:38 +0300, Claudiu Manoil wrote:
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.
May have or does have?
If it does have such a feature, do you plan to use it? Usually I have
not seen such features used for (e.g.) little-endian PCI devices on big
endian hosts.
I has that option, but I don't really plan to use it, clearly not for
big endian hosts. The "may have" was for future little endian hosts.
But I think this option is not really needed by the uboot driver
so doing the byte swapping in software should be ok (i.e. performance
wise).
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).
That does not achieve hardware synchronization, and even the effects on
the compiler are questionable due to volatile's vague semantics.
The existing code has been working this way for quite a while on the
mpc85xx platforms,
It was "working" for a while in Linux as well, until we encountered a
workload where it didn't (though granted, there was no volatile in that
case). See Linux commit 3b6330ce2a3e1f152f79a6203f73d23356e243a7
Good point, I guess it would be safer too use some memory barriers
around accesses to BD fields in the uboot driver too. However some
portable barriers would be needed, eieio() doesn't have an equivalent
for ARM.
FWIW, I see some other places in U-Boot's TSEC driver that use
out_be32() on the descriptors (e.g. startup_tsec after "Point to the
buffer descriptors").
That's only to program the tbase and rbase registers with the physical
address of the Tx/Rx BD rings' base.
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.
I picked some random examples, and the first driver in Linux in which I
could quickly find the BD rings uses I/O accessors
(drivers/net/ethernet/realtek/8319too.c). I then checked its U-Boot
eqivalent (drivers/net/rtl8139.c) and it also uses I/O accessors for the
descriptors.
I actually meant accessing buffer descriptor fields (like status,
length, dma address). As you can see in this example from linux
8319too.c's Rx routine, there's no I/O accessor involved:
/* read size+status of next frame from DMA ring buffer */
rx_status = le32_to_cpu (*(__le32 *) (rx_ring + ring_offset));
However the driver does use a rmb() just before this line.
for (t = 0; be16_to_cpup(&rtx.rxbd[rx_idx].status) & RXBD_EMPTY; t++) {
I opted for using macros instead due to code maintainability,
Obfuscatory macros do not help.
and to avoid overly long lines (80)
You could factor out an rxbd_empty() function, or factor that loop out
to be its own function, or have a local variable point to
rtx.rxbd[rx_idx]...
and to better control these changes.
For instance, if etsec were to access it's BDs in little endian format
in the future,
Either don't do that (preferred option), or at that point add
tsec16_to_cpup() and friends.
I'll have a try with the portable I/O accessors (in_be, out_be) to see
how it works that way.
Thanks,
Claudiu
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
