| Message ID | 1380122868-5306-1-git-send-email-abrodkin@synopsys.com |
|---|---|
| State | Accepted |
| Delegated to: | Joe Hershberger |
| Headers | show |
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
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
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
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
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(-)