Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dcd_lpc_ip3511: isochronous support and endpoint accidental write fix #1676

Merged
merged 8 commits into from Oct 24, 2022
Merged

dcd_lpc_ip3511: isochronous support and endpoint accidental write fix #1676

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
16f1554
lpc55s69 isochronous, dummy address for endpoint buffers to prevent a…
tswan-quasi Oct 11, 2022
930c682
double cast of pointer to remove error
tswan-quasi Oct 12, 2022
2c1ff26
(void) rhport for unused parameter
tswan-quasi Oct 13, 2022
3f45f37
minor rename
hathach Oct 17, 2022
fe42785
dummy buffer only on EP0 OUT ZLPs
tswan-quasi Oct 20, 2022
0b55047
typo fix
tswan-quasi Oct 20, 2022
35e1a27
unused (void) cast
tswan-quasi Oct 21, 2022
e3b7ed9
use dummy for all ZLP for ip3511, fix lpc55 build with DEBUG=1
hathach Oct 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
103 changes: 64 additions & 39 deletions src/portable/nxp/lpc_ip3511/dcd_lpc_ip3511.c
View file
Open in desktop
Expand Up @@ -78,8 +78,10 @@ typedef struct {

// Max nbytes for each control/bulk/interrupt transfer
enum {
NBYTES_CBI_FULLSPEED_MAX = 64,
NBYTES_CBI_HIGHSPEED_MAX = 32767 // can be up to all 15-bit, but only tested with 4096
NBYTES_ISO_FS_MAX = 1023, // FS ISO
NBYTES_ISO_HS_MAX = 1024, // HS ISO
NBYTES_CBI_FS_MAX = 64, // FS control/bulk/interrupt
NBYTES_CBI_HS_MAX = 32767 // can be up to all 15-bit, but only tested with 4096
};

enum {
Expand Down Expand Up @@ -112,6 +114,8 @@ enum {
typedef union TU_ATTR_PACKED
{
// Full and High speed has different bit layout for buffer_offset and nbytes
// TODO FS/HS layout depends on the max speed of controller e.g
// lpc55s69 PORT0 is only FS but actually has the same layout as HS on port1

// Buffer (aligned 64) = DATABUFSTART [31:22] | buffer_offset [21:6]
volatile struct {
Expand Down Expand Up @@ -174,6 +178,7 @@ typedef struct
// For example: LPC55s69 port1 Highspeed must be USB_RAM (0x40100000)
// Use CFG_TUSB_MEM_SECTION to place it accordingly.
CFG_TUSB_MEM_SECTION TU_ATTR_ALIGNED(256) static dcd_data_t _dcd;
CFG_TUSB_MEM_SECTION TU_ATTR_ALIGNED(256) static volatile uint8_t dummy[64]; // TODO temp fix to prevent accidental overwrite due to ep[][].buffer_xx.offset being 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need to have this. Have you experience issue with dcd data is overwritten. USB data shouldn't be accepted when active bit is not set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is likely a better solution, but it was causing me some issues not assigning the offsets to another location. I am however using a different build setup than the one provided in the repo, I tried to follow the defines as they are in the family makefile, hopefully this does not cause any discrepancies.

In functions such as dcd_init() and others, the USB would write to the offset in _dcd.ep[...][...].buffer_xx.offset and I think because the value of the DATABUFSTART register was 0x2000000, the USB was overwriting some other values I had in SRAM. The reason I noticed it was because the LED blink_interval_ms in main.c was not changing correctly and it happened to be placed at 0x20000000 in my build. First place it happened was after dcd_reg->DEVCMDSTAT |= ... in dcd_init()

A roundabout solution could be to always have the USB operate in USB_SRAM regardless of FS or HS, then in edpt_reset() and edpt_reset_all() have the offset value be set based on the start of USB_SRAM (0x40100000) since setting to zero could potentially cause issues with the APB bus which start at 0x40000000 which is what the value of DATABUFSTART would be. I could see this potentially having other issues though.

Copy link
Owner

@hathach hathach Oct 19, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not clear enough that USB overwrite the data, even if that is the case, I think we should properly fix it e.g explicitly set SKIP/DISABLE bit for endpoint first before zero it out. Otherwise we could run into issue that host may think we already received data (overwritten to zero), while it is ended up in dummy. Maybe we should do it in a separated PR.

PS: Do you have an easy/reliable way to reproduce this issue, I will try my luck to troubleshoot it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I noticed it on the video example when I was testing because blink_interval_ms was getting overwritten at address 0x20000000. I will take a look again as well as seeing if I can find proper solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A repeatable setup for me:

  • example: video_capture
  • modifications: revert all lines using dummy to before
  • OS: Windows
  • when USB is asleep it blinks as intended. open the camera app, once the device is awake, the value of blink_interval_ms is 0 (address 0x20000000) when it should be 1000

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have narrowed it down to dcd_edpt_xfer() when buffer is NULL, occurring for EP0 OUT ZLPs. The code below makes it modify dummy, and the other previous uses of dummy can be removed. From what I have seen, the USB writes 4 bytes of zeros to its offset location in this scenario. I am not sure if there is a proper solution to this aside from a dummy buffer?

bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t total_bytes)
{
  uint8_t const ep_id = ep_addr2id(ep_addr);

  tu_memclr(&_dcd.dma[ep_id], sizeof(xfer_dma_t));
  _dcd.dma[ep_id].total_bytes = total_bytes;

  if (!buffer && !ep_id)
  {
    buffer = (uint8_t*)(uint32_t)dummy;
  }
  prepare_ep_xfer(rhport, ep_id, get_buf_offset(buffer), total_bytes);

  return true;
}


//--------------------------------------------------------------------+
// Multiple Controllers
Expand Down Expand Up @@ -225,8 +230,44 @@ static inline uint8_t ep_addr2id(uint8_t ep_addr)
//--------------------------------------------------------------------+
// CONTROLLER API
//--------------------------------------------------------------------+

static void prepare_setup_packet(uint8_t rhport)
{
if (_dcd_controller[rhport].max_speed == TUSB_SPEED_FULL )
{
_dcd.ep[0][1].buffer_fs.offset = get_buf_offset(_dcd.setup_packet);
}else
{
_dcd.ep[0][1].buffer_hs.offset = get_buf_offset(_dcd.setup_packet);
}
}

static void edpt_reset(uint8_t rhport, uint8_t ep_id)
{
const uint32_t offset = get_buf_offset((void*)(uint32_t)dummy);
tu_memclr(&_dcd.ep[ep_id], sizeof(_dcd.ep[ep_id]));
if (_dcd_controller[rhport].max_speed == TUSB_SPEED_FULL )
{
_dcd.ep[ep_id][0].buffer_fs.offset = _dcd.ep[ep_id][1].buffer_fs.offset = offset;
}
else
{
_dcd.ep[ep_id][0].buffer_hs.offset = _dcd.ep[ep_id][1].buffer_hs.offset = offset;
}
}

static void edpt_reset_all(uint8_t rhport)
{
for (uint8_t ep_id = 0; ep_id < 2*_dcd_controller[rhport].ep_pairs; ++ep_id)
{
edpt_reset(rhport, ep_id);
}
prepare_setup_packet(rhport);
}
void dcd_init(uint8_t rhport)
{
edpt_reset_all(rhport);

dcd_registers_t* dcd_reg = _dcd_controller[rhport].regs;

dcd_reg->EPLISTSTART = (uint32_t) _dcd.ep;
Expand Down Expand Up @@ -310,18 +351,13 @@ void dcd_edpt_clear_stall(uint8_t rhport, uint8_t ep_addr)

bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc)
{
(void) rhport;

// TODO not support ISO yet
TU_VERIFY(p_endpoint_desc->bmAttributes.xfer != TUSB_XFER_ISOCHRONOUS);

//------------- Prepare Queue Head -------------//
uint8_t ep_id = ep_addr2id(p_endpoint_desc->bEndpointAddress);

// Check if endpoint is available
TU_ASSERT( _dcd.ep[ep_id][0].disable && _dcd.ep[ep_id][1].disable );

tu_memclr(_dcd.ep[ep_id], 2*sizeof(ep_cmd_sts_t));
edpt_reset(rhport, ep_id);
_dcd.ep[ep_id][0].is_iso = (p_endpoint_desc->bmAttributes.xfer == TUSB_XFER_ISOCHRONOUS);

// Enable EP interrupt
Expand All @@ -333,19 +369,20 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc)

void dcd_edpt_close_all (uint8_t rhport)
{
(void) rhport;
// TODO implement dcd_edpt_close_all()
for (uint8_t ep_id = 0; ep_id < 2*_dcd_controller[rhport].ep_pairs; ++ep_id)
{
_dcd.ep[ep_id][0].active = _dcd.ep[ep_id][0].active = 0; // TODO proper way is to EPSKIP then wait ep[][].active then write ep[][].disable (see table 778 in LPC55S69 Use Manual)
_dcd.ep[ep_id][0].disable = _dcd.ep[ep_id][1].disable = 1;
}
}

static void prepare_setup_packet(uint8_t rhport)
void dcd_edpt_close(uint8_t rhport, uint8_t ep_addr)
{
if (_dcd_controller[rhport].max_speed == TUSB_SPEED_FULL )
{
_dcd.ep[0][1].buffer_fs.offset = get_buf_offset(_dcd.setup_packet);;
}else
{
_dcd.ep[0][1].buffer_hs.offset = get_buf_offset(_dcd.setup_packet);;
}
(void) rhport;

uint8_t ep_id = ep_addr2id(ep_addr);
_dcd.ep[ep_id][0].active = _dcd.ep[ep_id][0].active = 0; // TODO proper way is to EPSKIP then wait ep[][].active then write ep[][].disable (see table 778 in LPC55S69 Use Manual)
_dcd.ep[ep_id][0].disable = _dcd.ep[ep_id][1].disable = 1;
}

static void prepare_ep_xfer(uint8_t rhport, uint8_t ep_id, uint16_t buf_offset, uint16_t total_bytes)
Expand All @@ -354,13 +391,12 @@ static void prepare_ep_xfer(uint8_t rhport, uint8_t ep_id, uint16_t buf_offset,

if (_dcd_controller[rhport].max_speed == TUSB_SPEED_FULL )
{
// TODO ISO FullSpeed can have up to 1023 bytes
nbytes = tu_min16(total_bytes, NBYTES_CBI_FULLSPEED_MAX);
nbytes = tu_min16(total_bytes, _dcd.ep[ep_id][0].is_iso ? NBYTES_ISO_FS_MAX : NBYTES_CBI_FS_MAX);
_dcd.ep[ep_id][0].buffer_fs.offset = buf_offset;
_dcd.ep[ep_id][0].buffer_fs.nbytes = nbytes;
}else
{
nbytes = tu_min16(total_bytes, NBYTES_CBI_HIGHSPEED_MAX);
nbytes = tu_min16(total_bytes, NBYTES_CBI_HS_MAX);
_dcd.ep[ep_id][0].buffer_hs.offset = buf_offset;
_dcd.ep[ep_id][0].buffer_hs.nbytes = nbytes;
}
Expand All @@ -372,14 +408,12 @@ static void prepare_ep_xfer(uint8_t rhport, uint8_t ep_id, uint16_t buf_offset,

bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t total_bytes)
{
(void) rhport;

uint8_t const ep_id = ep_addr2id(ep_addr);

tu_memclr(&_dcd.dma[ep_id], sizeof(xfer_dma_t));
_dcd.dma[ep_id].total_bytes = total_bytes;

prepare_ep_xfer(rhport, ep_id, get_buf_offset(buffer), total_bytes);
prepare_ep_xfer(rhport, ep_id, buffer ? get_buf_offset(buffer) : get_buf_offset((void*)(uint32_t)dummy), total_bytes);

return true;
}
Expand All @@ -390,15 +424,14 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t* buffer, uint16_t to
static void bus_reset(uint8_t rhport)
{
tu_memclr(&_dcd, sizeof(dcd_data_t));
edpt_reset_all(rhport);

// disable all non-control endpoints on bus reset
for(uint8_t ep_id = 2; ep_id < 2*MAX_EP_PAIRS; ep_id++)
{
_dcd.ep[ep_id][0].disable = _dcd.ep[ep_id][1].disable = 1;
}

prepare_setup_packet(rhport);

dcd_registers_t* dcd_reg = _dcd_controller[rhport].regs;

dcd_reg->EPINUSE = 0;
Expand Down Expand Up @@ -506,23 +539,15 @@ void dcd_int_handler(uint8_t rhport)
}
}

// TODO support suspend & resume
if (cmd_stat & CMDSTAT_SUSPEND_CHANGE_MASK)
{
if (cmd_stat & CMDSTAT_DEVICE_SUSPEND_MASK)
{ // suspend signal, bus idle for more than 3ms
// Note: Host may delay more than 3 ms before and/or after bus reset before doing enumeration.
if (cmd_stat & CMDSTAT_DEVICE_ADDR_MASK)
{
dcd_event_bus_signal(rhport, DCD_EVENT_SUSPEND, true);
}
// suspend signal, bus idle for more than 3ms
// Note: Host may delay more than 3 ms before and/or after bus reset before doing enumeration.
if (cmd_stat & CMDSTAT_DEVICE_ADDR_MASK)
{
dcd_event_bus_signal(rhport, (cmd_stat & CMDSTAT_DEVICE_SUSPEND_MASK) ? DCD_EVENT_SUSPEND : DCD_EVENT_RESUME, true);
}
}
// else
// { // resume signal
// dcd_event_bus_signal(rhport, DCD_EVENT_RESUME, true);
// }
// }
}

// Setup Receive
Expand Down