[U-Boot] [PATCH 06/10] usb:g_dnl:f_thor: USB download function to support TIZEN's THOR protocol

[Marek Vasut <marex@denx.de>]

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.

Also, I suspect you want to use DEFINE_CACHE_ALIGN_BUFFER here, no ?

> +/* ********************************************************** */
> +/*         THOR protocol - transmission handling	      */
> +/* ********************************************************** */
> +DEFINE_CACHE_ALIGN_BUFFER(char, f_name, F_NAME_BUF_SIZE);

Ditto

> +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.

> +	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.

> +
> +	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 ?

> +	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 ?

> +	.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 ...

> +		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;
> +		else
> +			dev->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.

> +}
> +
> +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()

> +		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 ...

> +		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

> +		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