Hi Marek,
Dear Lukasz Majewski,
[...]
+static struct f_thor *thor_func; +static inline struct f_thor *func_to_thor(struct usb_function *f) +{
- return container_of(f, struct f_thor, usb_function);
+}
+DEFINE_CACHE_ALIGN_BUFFER(char, thor_tx_data_buf, sizeof(struct rsp_box)); +DEFINE_CACHE_ALIGN_BUFFER(char, thor_rx_data_buf, sizeof(struct rqt_box));
This should either be uint8_t or unsigned char. A buffer shall not be (signed) char.
Yes. I agree. This buffer shall be unsigned char. I will correct that.
Also, I suspect you want to use DEFINE_CACHE_ALIGN_BUFFER here, no ?
I'm a bit confused.... I do use DEFINE_CACHE_ALIGN_BUFFER for those buffers.
+/* ********************************************************** */ +/* THOR protocol - transmission handling */ +/* ********************************************************** */ +DEFINE_CACHE_ALIGN_BUFFER(char, f_name, F_NAME_BUF_SIZE);
Ditto
I believe that buffer for storing file name (f_name) shall be defined as char.
+static unsigned long long int thor_file_size; +static int alt_setting_num;
+static void send_rsp(const struct rsp_box *rsp) +{
- /* should be copy on dma duffer */
- memcpy(thor_tx_data_buf, rsp, sizeof(struct rsp_box));
- thor_tx_data(thor_tx_data_buf, sizeof(struct rsp_box));
- debug("-RSP: %d, %d\n", rsp->rsp, rsp->rsp_data);
+}
+static void send_data_rsp(s32 ack, s32 count) +{
- ALLOC_CACHE_ALIGN_BUFFER(struct data_rsp_box, rsp,
sizeof(struct data_rsp_box));- rsp->ack = ack;
- rsp->count = count;
- /* should be copy on dma duffer */
This comment really makes no sense to me.
Yes. It's pretty obvious, what I intent to do in this function. I will remove it.
- memcpy(thor_tx_data_buf, rsp, sizeof(struct data_rsp_box));
- thor_tx_data(thor_tx_data_buf, sizeof(struct
data_rsp_box)); +
- debug("-DATA RSP: %d, %d\n", ack, count);
+}
+static int process_rqt_info(const struct rqt_box *rqt) +{
- ALLOC_CACHE_ALIGN_BUFFER(struct rsp_box, rsp,
sizeof(struct rsp_box));
- memset(rsp, '\0', sizeof(struct rsp_box));
- rsp->rsp = rqt->rqt;
- rsp->rsp_data = rqt->rqt_data;
- switch (rqt->rqt_data) {
- case RQT_INFO_VER_PROTOCOL:
rsp->int_data[0] = VER_PROTOCOL_MAJOR;rsp->int_data[1] = VER_PROTOCOL_MINOR;break;- case RQT_INIT_VER_HW:
snprintf(rsp->str_data[0],sizeof(rsp->str_data[0]),
"%x", checkboard());break;- case RQT_INIT_VER_BOOT:
sprintf(rsp->str_data[0], "%s", U_BOOT_VERSION);break;- case RQT_INIT_VER_KERNEL:
sprintf(rsp->str_data[0], "%s", "k unknown");break;- case RQT_INIT_VER_PLATFORM:
sprintf(rsp->str_data[0], "%s", "p unknown");break;- case RQT_INIT_VER_CSC:
sprintf(rsp->str_data[0], "%s", "c unknown");break;- default:
return -EINVAL;- }
- send_rsp(rsp);
- return true;
+}
+static int process_rqt_cmd(const struct rqt_box *rqt) +{
- ALLOC_CACHE_ALIGN_BUFFER(struct rsp_box, rsp,
sizeof(struct rsp_box));
- memset(rsp, '\0', sizeof(struct rsp_box));
memset(rsp, 0, sizeof() ... this '\0' is unneeded, fix globally.
Good point. I will correct this.
- rsp->rsp = rqt->rqt;
- rsp->rsp_data = rqt->rqt_data;
- switch (rqt->rqt_data) {
- case RQT_CMD_REBOOT:
debug("TARGET RESET\n");send_rsp(rsp);g_dnl_unregister();dfu_free_entities();run_command("reset", 0);break;- case RQT_CMD_POWEROFF:
- case RQT_CMD_EFSCLEAR:
send_rsp(rsp);This case fallthrough is intentional here ?
Yes. Thor protocol requires to receive response from device even when HOST PC ordered it to power off.
Also, on the target only reboot command is supported.
- default:
printf("Command not supported -> cmd: %d\n",rqt->rqt_data);
return -EINVAL;- }
- return true;
+}
[...] [...]
+static struct usb_cdc_call_mgmt_descriptor thor_downloader_cdc_call = {
- .bLength = sizeof(thor_downloader_cdc_call),
- .bDescriptorType = 0x24, /* CS_INTERFACE */
- .bDescriptorSubType = 0x01,
- .bmCapabilities = 0x00,
- .bDataInterface = 0x01,
+};
+struct usb_cdc_acm_descriptor thor_downloader_cdc_abstract = {
Why is this and the rest not static ?
They shall be static. I will fix that.
- .bLength = sizeof(thor_downloader_cdc_abstract),
- .bDescriptorType = 0x24, /* CS_INTERFACE */
- .bDescriptorSubType = 0x02,
- .bmCapabilities = 0x00,
+};
+struct usb_cdc_union_desc thor_downloader_cdc_union = {
- .bLength = sizeof(thor_downloader_cdc_union),
- .bDescriptorType = 0x24, /* CS_INTERFACE */
- .bDescriptorSubType = USB_CDC_UNION_TYPE,
+};
+static struct usb_endpoint_descriptor fs_int_desc = {
- .bLength = USB_DT_ENDPOINT_SIZE,
- .bDescriptorType = USB_DT_ENDPOINT,
- .bEndpointAddress = 3 | USB_DIR_IN,
- .bmAttributes = USB_ENDPOINT_XFER_INT,
- .wMaxPacketSize = __constant_cpu_to_le16(16),
- .bInterval = 0x9,
+};
[...]
+/*------------------------------------------------------------------------ -*/ +static struct usb_request *alloc_ep_req(struct usb_ep *ep, unsigned length) +{
- struct usb_request *req;
- req = usb_ep_alloc_request(ep, 0);
- if (req) {
if (!req) return ... ... rest of the code ...
Thanks for spotting. I fully agree.
req->length = length;req->buf = memalign(CONFIG_SYS_CACHELINE_SIZE,length);
if (!req->buf) {usb_ep_free_request(ep, req);req = NULL;}- }
- return req;
+}
[...]
+static void thor_rx_tx_complete(struct usb_ep *ep, struct usb_request *req) +{
- struct thor_dev *dev = thor_func->dev;
- int status = req->status;
- debug("%s: ep_ptr:%p, req_ptr:%p\n", __func__, ep, req);
- switch (status) {
- case 0: /* normal
completion? */
if (ep == dev->out_ep)dev->rxdata = 1;elsedev->txdata = 1;break;- /* this endpoint is normally active while we're configured
*/
- case -ECONNABORTED: /* hardware forced ep
reset */
- case -ECONNRESET: /* request dequeued */
- case -ESHUTDOWN: /* disconnect from host */
- /* Exeptional situation - print error message */
- case -EOVERFLOW:
error("ERROR:%d", status);- default:
debug("%s complete --> %d, %d/%d\n", ep->name,status, req->actual, req->length);- case -EREMOTEIO: /* short read */
break;- }
You might want to fix the order of these cases here.
Ok. I will reorder it.
+}
+static struct usb_request *thor_start_ep(struct usb_ep *ep) +{
- struct usb_request *req;
- req = alloc_ep_req(ep, ep->maxpacket);
- debug("%s: ep:%p req:%p\n", __func__, ep, req);
- if (!req)
return NULL;- memset(req->buf, 0, req->length);
- req->complete = thor_rx_tx_complete;
- memset(req->buf, 0x55, req->length);
- return req;
+}
[...]
+int thor_init(void) +{
- struct thor_dev *dev = thor_func->dev;
- /* Wait for a device enumeration and configuration
settings */
- debug("THOR enumeration/configuration setting....\n");
- while (!dev->configuration_done)
usb_gadget_handle_interrupts();- thor_set_dma(thor_rx_data_buf, strlen(recv_key));
- /* detect the download request from Host PC */
- if (thor_rx_data() < 0) {
printf("%s: Data not received!\n", __func__);return -1;- }
- if (strncmp(thor_rx_data_buf, recv_key, strlen(recv_key))
== 0) {
Use min() of upper bound on the recv_key length and this strlen()
But the recv_key is defined as const char *recv_key = "THOR";
I will rewrite this as:
if (!strcmp(thor_rx_data_buf, "THOR"))
puts("Download request from the Host PC\n");udelay(30 * 1000); /* 30 ms */strncpy(thor_tx_data_buf, tx_key, strlen(tx_key));thor_tx_data(thor_tx_data_buf, strlen(tx_key));- } else {
puts("Wrong reply information\n");return -1;- }
- return 0;
+}
[...]
+static int thor_eps_setup(struct usb_function *f) +{
- struct usb_composite_dev *cdev = f->config->cdev;
- struct usb_gadget *gadget = cdev->gadget;
- struct thor_dev *dev = thor_func->dev;
- struct usb_endpoint_descriptor *d;
- struct usb_request *req;
- struct usb_ep *ep;
- int result;
- ep = dev->in_ep;
- d = ep_desc(gadget, &hs_in_desc, &fs_in_desc);
- debug("(d)bEndpointAddress: 0x%x\n", d->bEndpointAddress);
- result = usb_ep_enable(ep, d);
- if (result == 0) {
if (result) goto exit;
... the rest ...
Yes, correct. I will change this.
ep->driver_data = cdev; /* claim */req = thor_start_ep(ep);if (req != NULL) {dev->in_req = req;} else {usb_ep_disable(ep);result = -EIO;}- } else {
goto exit;- }
- ep = dev->out_ep;
- d = ep_desc(gadget, &hs_out_desc, &fs_out_desc);
- debug("(d)bEndpointAddress: 0x%x\n", d->bEndpointAddress);
- result = usb_ep_enable(ep, d);
- if (result == 0) {
DTTO
Yes, this will be also changed.
ep->driver_data = cdev; /* claim */req = thor_start_ep(ep);if (req != NULL) {dev->out_req = req;} else {usb_ep_disable(ep);result = -EIO;}- } else {
goto exit;- }
- /* ACM control EP */
- ep = dev->int_ep;
- ep->driver_data = cdev; /* claim */
- exit:
- return result;
+}
[...]
Best regards, Marek Vasut