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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
