You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/07/27 00:52:24 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
a-lunev opened a new pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236
## Summary
bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
This fixes the following errors:
- "stm32_dmacapable: stm32_dmacapable: burst crosses 1KiB
up_assert: Assertion failed at file:chip/stm32_sdio.c line: 2890 task: init"
- "stm32_dmacapable: stm32_dmacapable: burst crosses 1KiB
up_assert: Assertion failed at file:chip/stm32_sdio.c line: 2808 task: bcmf"
## Impact
bcm43xxx driver
Photon board
EMW3162 board
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#issuecomment-888434714
> The fail due to CI check defconfig in the normalized form, please try this command to see the difference:
>
> ```
> ./tools/refresh.sh --silent photon/wlan
> ```
Thank you very much! This helped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r678283965
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
Your suggestion is good. Thanks. Just now I've applied it and tested it on Photon board.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677504222
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
I'm referencing bcmf_dma_buf struct I've introduced. first_word[] and remaining_aligned_buf[] parts should not have a gap between each other as they form the whole bcmf frame. Thus I've added "attribute((packed))" to the struct to prevent a gap inside of the struct (now I've wrapped it into begin_packed_struct / end_packed_struct for compiler independence).
However, the whole bcmf_dma_buf struct should be aligned to the address boundary of 16.
As the struct is packed inside and aligned to 16 outside, then remaining_aligned_buf[] is also aligned to the address boundary of 16, that is the purpose as it's used in DMA transfer.
There are two conditions at the same time:
- remaining_aligned_buf[] should be aligned;
- remaining_aligned_buf[] should follow first_word[] w/o a gap in the RAM.
The main purpose of first_word[] is to receive the length (the first phase) of the remaining frame to be read (the second phase). DMA receive transfer can not be invoked w/o knowing the length of the frame first, thus it can not be a single transfer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#issuecomment-887374311
Hi Xiang,
Alternatively, I've added CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT parameter in menuconfig for bcm43xxx driver.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677653089
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
Ok, after reviewing your change more carefully, you need read the first 4bytes and then the rest packet:
1. the first word align on 12bytes
2. then the rest packet align on 16bytes
So, you declare the buffer is 16bytes alignment and skip the 12bytes manually. But, why not directly align the buffer 12bytes directly liket this:
```
uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
CONFIG_NET_GUARDSIZE] aligned_data(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT - 4);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677514319
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
It's more simple to attach aligned_data(16) to the data field directly:
```
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
CONFIG_NET_GUARDSIZE] aligned_data(16);
};
```
And ld.script may don't need change too. There are many examples in #4238.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677955436
##########
File path: drivers/wireless/ieee80211/bcm43xxx/Kconfig
##########
@@ -101,4 +101,12 @@ config IEEE80211_BROADCOM_NINTERFACES
default 1
depends on EXPERIMENTAL
+config IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT
Review comment:
how about change to IEEE80211_BROADCOM_DMABUF_ALIGNMENT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677653089
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
Ok, after reviewing your change more carefully, you need read the first 4bytes and then the rest packet:
1. the first word align on 12bytes
2. then the rest packet align on 16bytes
So, you declare the buffer is 16bytes alignment and skip the 12bytes manually. But, why not directly align the buffer 12bytes directly liket this:
```
uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE]
aligned_data(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT - 4);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677954749
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
Ok, but from your change, align_padding and remaining_aligned_buf is never referenced. How about we change to:
```
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
uint8_t pad[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT - FIRST_WORD_SIZE] aligned_data(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT);
uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE];
};
```
Which could reduce the rest change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677350110
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
+ FIRST_WORD_SIZE];
+ uint8_t first_word[FIRST_WORD_SIZE];
+ uint8_t remaining_aligned_buf[HEADER_SIZE - FIRST_WORD_SIZE +
+ MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE];
+};
+
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
- uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
- CONFIG_NET_GUARDSIZE];
+ struct bcmf_dma_buf data
+ __attribute__((aligned(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT)));
Review comment:
Good. I will use the new aligned_data as soon as #4238 merged into the master branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677463503
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
+ FIRST_WORD_SIZE];
+ uint8_t first_word[FIRST_WORD_SIZE];
+ uint8_t remaining_aligned_buf[HEADER_SIZE - FIRST_WORD_SIZE +
+ MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE];
+};
+
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
- uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
- CONFIG_NET_GUARDSIZE];
+ struct bcmf_dma_buf data
+ __attribute__((aligned(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT)));
Review comment:
Hi @patacongo, good point. Thanks.
@xiaoxiang781216, initially I've misunderstood, I thought you've introduced the aligned_data macro just today in #4238.
As the aligned_data macro is already in the master branch, now I've replaced __ attribute __((aligned(x)))" by aligned_data(x).
Concerning compiler independence, now I've found "__ attribute __ ((packed))" occurrences in bcm43xxx driver.
Thus I've replaced "__ attribute __((packed))" by "begin_packed_struct / end_packed_struct" everywhere in bcm43xxx driver as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677348955
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
I tried to use __attribute__((aligned(16))) attached directly to "uint8_t remaining_aligned_buf", however the alignment did not work inside of struct __attribute__((packed)). (first_word[] and remaining_aligned_buf[] parts are required to be adjacent). Thus I've added align_padding[] before first_word[] and attached __attribute__((aligned(16))) to the whole structure.
One more alternative approach would be to use a monolitic data array and use offsets to point to imaginary first_word[] and remaining_aligned_buf[] parts. Is the alternative preferable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] patacongo commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
patacongo commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677396508
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
+ FIRST_WORD_SIZE];
+ uint8_t first_word[FIRST_WORD_SIZE];
+ uint8_t remaining_aligned_buf[HEADER_SIZE - FIRST_WORD_SIZE +
+ MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE];
+};
+
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
- uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
- CONFIG_NET_GUARDSIZE];
+ struct bcmf_dma_buf data
+ __attribute__((aligned(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT)));
Review comment:
GCC attributes are forbidden in common code. This should not be merged until this fixed. There are compiler-independent macros in include/nuttx/compiler.h that should be used to make the alignment compiler independent.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677661977
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_bdc.c
##########
@@ -48,30 +48,30 @@
* Private Types
****************************************************************************/
-struct __attribute__((packed)) bcmf_bdc_header
+begin_packed_struct struct bcmf_bdc_header
Review comment:
No problem, let's do it in #4238. I've already replaced __ attribute __ occurrences here in #4236 PR (in my branch) for bcm43xxx.
As I understand, it should be normally merged anyway. If I should remove the changes in my branch, please let me know.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677649214
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_bdc.c
##########
@@ -48,30 +48,30 @@
* Private Types
****************************************************************************/
-struct __attribute__((packed)) bcmf_bdc_header
+begin_packed_struct struct bcmf_bdc_header
Review comment:
let's #4238 fix this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677653089
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
Ok, after reviewing your change more carefully, you need read the first 4bytes and then the rest packet:
1. the first word align on 12bytes
2. then the rest packet align on 16bytes
So, you declare the buffer is 16bytes alignment and skip the 12bytes manually. But, why not directly align the buffer 12bytes directly liket this:
```
uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE]
aligned_data(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT - 4);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677653089
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
Ok, after reviewing your change more carefully, you need read the first 4bytes and then the rest packet:
1. the first word align on 12bytes
2. then the rest packet align on 16bytes
So, you declare the buffer is 16bytes alignment and skip the 12bytes manually. But, why not directly align the buffer 12bytes directly liket this:
```
uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE]
aligned_data(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT - 4);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677460368
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
but bcmf_sdio_frame don't have attribute((packed)), which struct you reference has this attribute?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677073264
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -42,11 +42,40 @@
****************************************************************************/
#define HEADER_SIZE 0x12 /* Default sdpcm + bdc header size */
+#define FIRST_WORD_SIZE 4
/* TODO move to Kconfig */
#define BCMF_PKT_POOL_SIZE 4 /* Frame pool size */
+#if defined(CONFIG_STM32_STM32F10XX)
Review comment:
it isn't good to reference SOC specific config in a common driver.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677504222
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
I'm referencing bcmf_dma_buf struct I've introduced. first_word[] and remaining_aligned_buf[] parts should not have a gap between each other as they form the whole bcmf frame. Thus I've added "attribute((packed))" to the struct to prevent a gap inside of the struct (now I've wrapped it into begin_packed_struct / end_packed_struct for compiler independence).
However, the whole bcmf_dma_buf struct should be aligned to the address boundary of 16.
As the struct is packed inside and aligned to 16 outside, then remaining_aligned_buf[] is also aligned to the address boundary of 16, that is the purpose as it's used in DMA transfer.
There are two conditions at the same time:
- remaining_aligned_buf[] should be aligned;
- remaining_aligned_buf[] should follow first_word[] w/o a gap.
The main purpose of first_word[] is to receive the length (the first phase) of the remaining frame to be read (the second phase). DMA receive transfer can not be invoked w/o knowing the length of the frame first, thus it can not be a single transfer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677348955
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
I tried to use __attribute__((aligned(16))) attached directly to "uint8_t remaining_aligned_buf", however the alignment did not work inside of struct __attribute__((packed)). (first_word[] and remaining_aligned_buf[] parts are required to be adjacent). Thus I've added align_padding[] before first_word[] and attached __attribute__((aligned(16))) to the whole structure.
One more alternative approach would be to use a monolithic data array and use offsets to point to imaginary first_word[] and remaining_aligned_buf[] parts. Is the alternative preferable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677488280
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
+ FIRST_WORD_SIZE];
+ uint8_t first_word[FIRST_WORD_SIZE];
+ uint8_t remaining_aligned_buf[HEADER_SIZE - FIRST_WORD_SIZE +
+ MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE];
+};
+
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
- uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
- CONFIG_NET_GUARDSIZE];
+ struct bcmf_dma_buf data
+ __attribute__((aligned(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT)));
Review comment:
#4238 will fix all ```__attribute__``` in a batch, so you can drop the related change from your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677648641
##########
File path: boards/arm/stm32/emw3162/scripts/ld.script
##########
@@ -45,11 +45,16 @@ SECTIONS
*(.fixup)
*(.gnu.warning)
- . = ALIGN(0x4);
+ /* SDIO_RXDMA32_CONFIG and SDIO_TXDMA32_CONFIG values in stm32_sdio.c
+ * give the DMA burst length of 16 bytes
+ * (DMA_SCR_MSIZE_32BITS x DMA_SCR_MBURST_INCR4).
+ * Thus the following alignment should be 0x10.
+ */
+ . = ALIGN(0x10);
wlan_firmware_image_location = .;
*(.wlan_firmware_image .wlan_firmware_image.*)
wlan_firmware_image_end = .;
- . = ALIGN(0x4);
+ . = ALIGN(0x10);
Review comment:
Ok, this is for WiFi mac firmware, not the network packet send to device, look good.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#issuecomment-888344906
The fail due to CI check defconfig in the normalized form, please try this command to see the difference:
```
./tools/refresh.sh --silent photon/wlan
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#issuecomment-888344906
The fail due to CI check defconfig in the normalized form, please try this command to say the difference:
```
./tools/refresh.sh --silent photon/wlan
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677546437
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
In my opinion, the approach by using bcmf_dma_buf struct is better as it's self-documented and gives a better understanding compared to a monolithic buffer with offsets in the programming code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677653089
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
Ok, after reviewing your change more carefully, you need read the first 4bytes and then the rest packet:
1. the first word align on 12bytes
2. then the rest packet align on 16bytes
So, you declare the buffer is 16bytes alignment and skip the 12bytes manually. But, why not directly align the buffer 12bytes directly liket this:
```
uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE] aligned_data(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT - 4);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677733304
##########
File path: boards/arm/stm32/emw3162/scripts/ld.script
##########
@@ -45,11 +45,16 @@ SECTIONS
*(.fixup)
*(.gnu.warning)
- . = ALIGN(0x4);
+ /* SDIO_RXDMA32_CONFIG and SDIO_TXDMA32_CONFIG values in stm32_sdio.c
+ * give the DMA burst length of 16 bytes
+ * (DMA_SCR_MSIZE_32BITS x DMA_SCR_MBURST_INCR4).
+ * Thus the following alignment should be 0x10.
+ */
+ . = ALIGN(0x10);
wlan_firmware_image_location = .;
*(.wlan_firmware_image .wlan_firmware_image.*)
wlan_firmware_image_end = .;
- . = ALIGN(0x4);
+ . = ALIGN(0x10);
Review comment:
Concerning firmware images, now I've removed ALIGN from the linker scripts, and added "aligned_data" to stm32_wlan_firmware.c instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677537261
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
> It's more simple to attach aligned_data(16) to the data field directly:
In your example I need data[] buffer to be aligned to the boundary of 16 at the offset of 4 (not at the offset of 0). Please, check my previous message.
And as I previously wrote, one more alternative approach would be to use a monolitic data array (like in your example) and use offsets to point to imaginary first_word[] and remaining_aligned_buf[] parts. Is the alternative preferable?
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
> It's more simple to attach aligned_data(16) to the data field directly:
In your example I need data[] buffer to be aligned to the boundary of 16 at the offset of 4 (not at the offset of 0). Please, check my previous message.
And as I previously wrote, one more alternative approach would be to use a monolitic data array (like in your example) and use offsets to point to imaginary first_word[] and remaining_aligned_buf[] parts. Is the alternative preferable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677463503
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
+ FIRST_WORD_SIZE];
+ uint8_t first_word[FIRST_WORD_SIZE];
+ uint8_t remaining_aligned_buf[HEADER_SIZE - FIRST_WORD_SIZE +
+ MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE];
+};
+
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
- uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
- CONFIG_NET_GUARDSIZE];
+ struct bcmf_dma_buf data
+ __attribute__((aligned(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT)));
Review comment:
Hi @patacongo, good point. Thanks.
@xiaoxiang781216, initially I've misunderstood, I thought you've introduced the aligned_data macro just today in #4238.
As the aligned_data macro is already in the master branch, now I've replaced __attribute__((aligned(x))) by aligned_data(x).
Concerning compiler independence, now I've found "__attribute__((packed))" occurrences in bcm43xxx driver.
Thus I've replaced "__attribute__((packed))" by "begin_packed_struct / end_packed_struct" everywhere in bcm43xxx driver as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#issuecomment-888334080
@xiaoxiang781216, possibly you know, for some reason CI tests on github sometimes fail despite the code is good and normally is built locally. Sometimes, I just sent the new commit (by adding a space character to the commit memo) just to restart CI tests on github (I do not know how else I can restart them). Usually just the restarting this way helped. However, now it always fails on arm-09 step. Possibly some build cache should be cleared? I do not know how I can do this, I do not have access.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677504222
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
I'm referencing bcmf_dma_buf struct I've introduced. first_word[] and remaining_aligned_buf[] parts should not have a gap between each other as they form the whole bcmf frame. Thus I've added "attribute((packed))" to the struct to prevent a gap inside of the struct (now I've wrapped it into begin_packed_struct / end_packed_struct for compiler independence).
However, the whole bcmf_dma_buf struct should be aligned to the address boundary of 16.
As the struct is packed inside and aligned to 16 outside, then remaining_aligned_buf[] is also aligned to the address boundary of 16, that is the purpose as it's used in DMA transfer.
There are two conditions at the same time:
- remaining_aligned_buf[] should be aligned;
- remaining_aligned_buf[] should follow first_word[] w/o a gap in the RAM.
The main purpose of first_word[] is to receive the length (the first phase) of the remaining frame to be read (the second phase). DMA receive transfer can not be invoked w/o knowing the length of the frame first, thus it can not be a single transfer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r678222917
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
How about this suggestion, @a-lunev ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677698367
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
> So, you declare the buffer is 16bytes alignment and skip the 12bytes manually. But, why not directly align the buffer 12bytes directly liket this:
>
> ```
> uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE]
> aligned_data(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT - 4);
> ```
Any alignment must be power of 2 (it's the gcc standard requirement):
```
wireless/ieee80211/bcm43xxx/bcmf_sdio.h:130:3: error: requested alignment is not a positive power of 2
uint8_t data2[32] aligned_data(12);
^~~~~~~
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r678174082
##########
File path: drivers/wireless/ieee80211/bcm43xxx/Kconfig
##########
@@ -101,4 +101,12 @@ config IEEE80211_BROADCOM_NINTERFACES
default 1
depends on EXPERIMENTAL
+config IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT
Review comment:
Good. I've renamed it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677537261
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
> It's more simple to attach aligned_data(16) to the data field directly:
In your example I need data[] buffer to be aligned to the boundary of 16 at the offset of 4 (not at the offset of 0). Please, check my previous message.
And as I previously wrote, one more alternative approach would be to use a monolithic data array (like in your example) and use offsets to point to imaginary first_word[] and remaining_aligned_buf[] parts. Is the alternative preferable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #4236: bcm43xxx: fixed issues with unaligned buffers for DMA transfers.
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4236:
URL: https://github.com/apache/incubator-nuttx/pull/4236#discussion_r677327171
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
+ FIRST_WORD_SIZE];
+ uint8_t first_word[FIRST_WORD_SIZE];
+ uint8_t remaining_aligned_buf[HEADER_SIZE - FIRST_WORD_SIZE +
+ MAX_NETDEV_PKTSIZE + CONFIG_NET_GUARDSIZE];
+};
+
struct bcmf_sdio_frame
{
struct bcmf_frame_s header;
bool tx;
dq_entry_t list_entry;
- uint8_t data[HEADER_SIZE + MAX_NETDEV_PKTSIZE +
- CONFIG_NET_GUARDSIZE];
+ struct bcmf_dma_buf data
+ __attribute__((aligned(CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT)));
Review comment:
directly use aligned_data:
https://github.com/apache/incubator-nuttx/pull/4238
##########
File path: drivers/wireless/ieee80211/bcm43xxx/bcmf_sdio.h
##########
@@ -110,13 +111,22 @@ struct bcmf_sdio_dev_s
/* Structure used to manage SDIO frames */
+struct __attribute__((packed)) bcmf_dma_buf
+{
+ uint8_t align_padding[CONFIG_IEEE80211_BROADCOM_SDIO_DMA_BUF_ALIGNMENT -
Review comment:
why not attach aligned_data to the char buffer directly?
##########
File path: boards/arm/stm32/emw3162/scripts/ld.script
##########
@@ -45,11 +45,16 @@ SECTIONS
*(.fixup)
*(.gnu.warning)
- . = ALIGN(0x4);
+ /* SDIO_RXDMA32_CONFIG and SDIO_TXDMA32_CONFIG values in stm32_sdio.c
+ * give the DMA burst length of 16 bytes
+ * (DMA_SCR_MSIZE_32BITS x DMA_SCR_MBURST_INCR4).
+ * Thus the following alignment should be 0x10.
+ */
+ . = ALIGN(0x10);
wlan_firmware_image_location = .;
*(.wlan_firmware_image .wlan_firmware_image.*)
wlan_firmware_image_end = .;
- . = ALIGN(0x4);
+ . = ALIGN(0x10);
Review comment:
is it enough to add __attribute__((aligned(16))) on the global data?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org