Patchwork [U-Boot,v2] drivers/net/designware - fix alignment of buffer descriptors

login
register
mail settings
Submitter Alexey Brodkin
Date Sept. 25, 2013, 3:27 p.m.
Message ID <1380122868-5306-1-git-send-email-abrodkin@synopsys.com>
Download mbox | patch
Permalink /patch/277902/
State Accepted
Delegated to: Joe Hershberger
Headers show

Comments

Alexey Brodkin - Sept. 25, 2013, 3:27 p.m.
It's important that buffer descriptors are aligned in accordance to GMAC
data bus width (32/64/128-bit). It's safe to align to 128-bit (16-bytes)
for every bus width type.

If buffer descriptor is improperly aligned GMAC discards lower bits of
provided address and as a result reads from improper location that
doesn't match expected fields.

Commit ef76025a99247cdb8f927a2c9f15400678dfb599 "net: Multiple
updates/enhancements to designware.c" introduced another structure
member "link_printed" right before buffer descriptors while "padding"
member was left untouched. This together with alignment of structure
itself to 16-byte boundary forces buffer descriptoprs always to be
4-byte aligned that causes driver complete disfunction if GMAC bus width
is 64 or 128-bit.

Proposed change makes sure all buffer descriptors are 16-byte (128-bit)
aligned.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Compared to V1 "dw_eth_dev" structure was left aligned, because I
realized that it's address will be used as a base for all the members.
And only this way I may be sure arrays of buffer descriptors are
properly aligned.
---
 drivers/net/designware.h | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)
Vipin Kumar - Sept. 26, 2013, 4:09 a.m.
On 9/25/2013 8:57 PM, Alexey Brodkin wrote:
> It's important that buffer descriptors are aligned in accordance to GMAC
> data bus width (32/64/128-bit). It's safe to align to 128-bit (16-bytes)
> for every bus width type.
>
> If buffer descriptor is improperly aligned GMAC discards lower bits of
> provided address and as a result reads from improper location that
> doesn't match expected fields.
>
> Commit ef76025a99247cdb8f927a2c9f15400678dfb599 "net: Multiple
> updates/enhancements to designware.c" introduced another structure
> member "link_printed" right before buffer descriptors while "padding"
> member was left untouched. This together with alignment of structure
> itself to 16-byte boundary forces buffer descriptoprs always to be
> 4-byte aligned that causes driver complete disfunction if GMAC bus width
> is 64 or 128-bit.
>

I have also faced this problem before. May be a better solution is to 
place all the struct and buffer declarations at the very start of 
dw_eth_dev structure (off-course with a comment that these should not be 
moved). It may avoid the problem in later modifications

Regards
Vipin
Mischa Jonker - Sept. 26, 2013, 5:26 a.m.
Vipin wrote:
> I have also faced this problem before. May be a better solution is to
> place all the struct and buffer declarations at the very start of
> dw_eth_dev structure (off-course with a comment that these should not
> be moved). It may avoid the problem in later modifications

I think that's why Alexey added the alignment to the struct dmamacdescr declaration, to make sure that it always aligned on a boundary of 16 bytes (so even 128-bit busses don't face this issue).

I don't know though whether the __aligned attribute should be at the type definition of the struct or at the declaration of the "struct dmamacdescr" inside "struct dw_eth_dev". I'm guessing the declaration inside "struct dw_eth_dev" will inherit the alignment requirements of the type def though, but not sure.

Mischa
Alexey Brodkin - Sept. 26, 2013, 8:30 a.m.
On 09/26/2013 09:26 AM, Mischa Jonker wrote:
> Vipin wrote:
>> I have also faced this problem before. May be a better solution is to
>> place all the struct and buffer declarations at the very start of
>> dw_eth_dev structure (off-course with a comment that these should not
>> be moved). It may avoid the problem in later modifications
>
> I think that's why Alexey added the alignment to the struct dmamacdescr declaration, to make sure that it always aligned on a boundary of 16 bytes (so even 128-bit busses don't face this issue).
>
> I don't know though whether the __aligned attribute should be at the type definition of the struct or at the declaration of the "struct dmamacdescr" inside "struct dw_eth_dev". I'm guessing the declaration inside "struct dw_eth_dev" will inherit the alignment requirements of the type def though, but not sure.

At least from what I see during my trials "__aligned" in definition of 
BD structure works as well. Both arrays of buffer descriptors were 
properly aligned compared to "dw_eth_dev" base address.
And since "dw_eth_dev" structure is already aligned (it is allocated 
with "memalign") both BD arrays are also properly aligned.

Initially I thought about move of BD arrays on top of the mother 
structure but then decided to go with "__aligned" specifiers. IMHO it is 
safe enough while still very clear - no need to worry about order of 
structure members and no need to add any comments on how to keep members 
in order.

-Alexey

Patch

diff --git a/drivers/net/designware.h b/drivers/net/designware.h
index d668f8f..137acb0 100644
--- a/drivers/net/designware.h
+++ b/drivers/net/designware.h
@@ -128,7 +128,7 @@  struct dmamacdescr {
 	u32 dmamac_cntl;
 	void *dmamac_addr;
 	struct dmamacdescr *dmamac_next;
-};
+} __aligned(16);
 
 /*
  * txrx_status definitions
@@ -240,8 +240,7 @@  struct dw_eth_dev {
 	u32 tx_currdescnum;
 	u32 rx_currdescnum;
 	u32 phy_configured;
-	int link_printed;
-	u32 padding;
+	u32 link_printed;
 
 	struct dmamacdescr tx_mac_descrtable[CONFIG_TX_DESCR_NUM];
 	struct dmamacdescr rx_mac_descrtable[CONFIG_RX_DESCR_NUM];
@@ -253,7 +252,7 @@  struct dw_eth_dev {
 	struct eth_dma_regs *dma_regs_p;
 
 	struct eth_device *dev;
-} __attribute__ ((aligned(8)));
+};
 
 /* Speed specific definitions */
 #define SPEED_10M		1