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

Lukasz Majewski l.majewski at samsung.com
Fri Oct 4 10:30:53 CEST 2013


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

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



-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group


More information about the U-Boot mailing list