Skip to content

fix for endpoint bitfield problem in big endian mode#1114

Closed
Wini-Buh wants to merge 1 commit into
hathach:masterfrom
Wini-Buh:CCRX_Port
Closed

fix for endpoint bitfield problem in big endian mode#1114
Wini-Buh wants to merge 1 commit into
hathach:masterfrom
Wini-Buh:CCRX_Port

Conversation

@Wini-Buh
Copy link
Copy Markdown
Contributor

I could track down the problem with the bitfield "wMaxPacketSize" in the file "usb_types.h"

struct TU_ATTR_PACKED {
#if defined(__CCRX__)
//FIXME the original defined bit field has a problem with the CCRX toolchain, so only a size field is defined
uint16_t size;
#else
uint16_t size : 11; ///< Maximum packet size this endpoint is capable of sending or receiving when this configuration is selected. \n For isochronous endpoints, this value is used to reserve the bus time in the schedule, required for the per-(micro)frame data payloads. The pipe may, on an ongoing basis, actually use less bandwidth than that reserved. The device reports, if necessary, the actual bandwidth used via its normal, non-USB defined mechanisms. \n For all endpoints, bits 10..0 specify the maximum packet size (in bytes). \n For high-speed isochronous and interrupt endpoints: \n Bits 12..11 specify the number of additional transaction opportunities per microframe: \n- 00 = None (1 transaction per microframe) \n- 01 = 1 additional (2 per microframe) \n- 10 = 2 additional (3 per microframe) \n- 11 = Reserved \n Bits 15..13 are reserved and must be set to zero.
uint16_t hs_period_mult : 2;
uint16_t TU_RESERVED : 3;
#endif
}wMaxPacketSize;

and the CCRX toolchain building in big endian mode. The problem does not occurs if the CCRX toolchain is building in little endian mode.

The bitfield "wMaxPacketSize" is a member of the "tusb_desc_endpoint_t" struct definition, which is used to make the access to the USB descriptor easier. The USB descriptor is a constant and it's descriptor fields must be always in little endian and the bitfield order must be always lower-bit (or right). An access with a controller working in big endian is possible as long as the endian change on an access to the descriptor fields is possible.

It is not possible to tell the CCRX that the constant USB descriptor (which is an array of bytes) is defined in little endian and the struct definition that is mapped over it must force the access in little endian.

Have we look at the situation how it looks in the memory (only the bitfield "wMaxPacketSize") on a little endian configuration:

Allocation of the bitfields in "right" (or lower-bit) :

Bit: 15   13 12  11 10           0
     +------+------+-------------+
     |  :3  |  :2  |     :11     |
     +------+------+-------------+
typedef struct {
  uint16_t size           : 11;  // bits 0 - 10
  uint16_t hs_period_mult : 2;   // bits 11 - 12
  uint16_t TU_RESERVED    : 3;   // bits 13 - 15
} wMaxPacketSize;

The U16_TO_U8S_LE macro (which is used in the TUD_CDC_DESCRIPTOR macro) creates the the following content in the memory at "Adr" if a value of 0x40 (01000000b) is used

     +-----+----------+----------+
     | Adr |     n    |    n+1   |
     +-----+----------+----------+ 
           |   0x40   |   0x00   |
           +----------+----------+
           | 01000000 | 00000000 |
           +----------+----------+           

Loading this content as a 16 bit word into a register of the RX controller gives the value of 0x40 (01000000b) because of the little endian configuration of the RX CPU.

Mapping the struct "wMaxPacketSize" over it looks like this

 Bit: 15   13 12  11 10           0
      +------+------+-------------+
      |  :3  |  :2  |     :11     |
      +------+------+-------------+
      | 000  |  00  | 00001000000 | <- value of size field 0x40 
      +------+------+-------------+

Doing the same in big endian configuration
Loading this content as a 16 bit word into a register of the RX controller gives the value of 0x4000 (01000000 00000000b) because of the big endian configuration of the RX CPU.

Mapping the struct "wMaxPacketSize" over it looks like this

 Bit: 15   13 12  11 10           0
      +------+------+-------------+
      |  :3  |  :2  |     :11     |
      +------+------+-------------+
      | 010  |  00  | 00000000000 | <- value of size field 0x0 
      +------+------+-------------+

Conclusion
This PR solves this issue with the use of a helper struct, which is used to change the endian if required and then access the bitfields correctly.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Sep 30, 2021

Thank you @Wini-Buh for detail breakout, I think I understand the issue, indeed the bitfield doesn't work well when swapping the endian. USB descriptors are always in endian --> need swap, which will cause incorrect mapping of bitfield. I think the best solution is just drop the bitfield within multi-byte field such as wMaxPacketSize entirely. The highspeed mult isn't used much anyway, in case of usage, it could be done simply by bitwise operation. I will make any PR to rename/change it, although it is used across drivers, it is very straight forward change.

@Wini-Buh
Copy link
Copy Markdown
Contributor Author

Wini-Buh commented Oct 1, 2021

@hathach Ok, I agree with you to replace the bitfield wMaxPacketSize as a normal unsigned short field. A simple solution is always the best. I let this draft PR open until the modifications is done (as a reminder).

@hathach
Copy link
Copy Markdown
Owner

hathach commented Oct 1, 2021

@hathach Ok, I agree with you to replace the bitfield wMaxPacketSize as a normal unsigned short field. A simple solution is always the best. I let this draft PR open until the modifications is done (as a reminder).

will do it after PRs adding video and cdc-ncm, since it can complicate those on-going PR.

@hathach
Copy link
Copy Markdown
Owner

hathach commented Oct 24, 2021

@Wini-Buh bit-field in the wMaxPacketSize is removed by #1161 , please check it out to see if that works for you.

7FM pushed a commit to 7FM/tinyusb that referenced this pull request Aug 23, 2025
In arm-gnu-toolchain-12.2 we see this warning, that's not relevant to
pico. Disable it.

warning: blink.elf has a LOAD segment with RWX permissions

Fixes hathach#1029
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants