On Thursday, February 06, 2014 at 06:27:01 AM, Stephen Warren wrote:

Tom, Wolfgang, can you please check the license of the drivers/usb/host/dwc2_otg_core_if.h below ? Thanks!

[...]

+void handle_error(int line, uint32_t d) +{

• hcint_data_t hcint;
• hcint.d32 = d;
• printf("Error condition at line %d: ", line);
• if (hcint.b.ahberr)

• printf(" AHBERR");
  

puts(), please fix globally. Is such verbosity needed anyway?

• if (hcint.b.stall)

• printf(" STALL");
  

• if (hcint.b.bblerr)

• printf(" NAK");
  

• if (hcint.b.ack)

• printf(" ACK");
  

• if (hcint.b.nyet)

• printf(" NYET");
  

• if (hcint.b.xacterr)

• printf(" XACTERR");
  

• if (hcint.b.datatglerr)

• printf(" DATATGLERR");
  

• printf("\n");

+}

+/*

• • U-Boot USB interface
• */

+int usb_lowlevel_init(int index, enum usb_init_type init, void **controller) +{

• /*

• * We need doubleword-aligned buffers for DMA transfers
  

• */
  

• allocated_buffer = (uint8_t *)malloc(DWC_OTG_HCD_STATUS_BUF_SIZE +

DWC_OTG_HCD_DATA_BUF_SIZE + 8); + uint32_t addr = (uint32_t)allocated_buffer;

• aligned_buffer = (uint8_t *) ((addr + 7) & ~7);
• status_buffer = (uint8_t *)((uint32_t)aligned_buffer +

DWC_OTG_HCD_DATA_BUF_SIZE); + int i, j;

• hprt0_data_t hprt0 = {.d32 = 0 };

Use DEFINE_CACHE_ALIGN_BUFFER or similar from include/common.h instead .

• root_hub_devnum = 0;
• memset(&g_core_if, 0, sizeof(g_core_if));

This would then be moot.

• dwc_otg_cil_init(&g_core_if, (uint32_t *)0x20980000);
• if ((g_core_if.snpsid & 0xFFFFF000) !=

• 0x4F542000) {
  

• printf("SNPSID is invalid (not DWC OTG device): %08x\n",
  

g_core_if.snpsid); + return -1;

• }

Get rid of the magic constants please. Use regular errno.h definitions for return values please.

• dwc_otg_core_init(&g_core_if);
• dwc_otg_core_host_init(&g_core_if);
• hprt0.d32 = dwc_otg_read_hprt0(&g_core_if);
• hprt0.b.prtrst = 1;
• dwc_write_reg32(g_core_if.host_if->hprt0, hprt0.d32);
• udelay(50000);

That's quite a delay you have here ... is it needed? Also, use mdelay(50) instead ?

• hprt0.b.prtrst = 0;
• dwc_write_reg32(g_core_if.host_if->hprt0, hprt0.d32);
• udelay(50000);
• hprt0.d32 = dwc_read_reg32(g_core_if.host_if->hprt0);
• for (i = 0; i < MAX_DEVICE; i++) {

• for (j = 0; j < MAX_ENDPOINT; j++) {
  

• 	control_data_toggle[i][j] = DWC_OTG_HC_PID_DATA1;
  

• 	bulk_data_toggle[i][j] = DWC_OTG_HC_PID_DATA0;
  

• }
  

• }
• return 0;

+}

+int usb_lowlevel_stop(int index) +{

• free(allocated_buffer);
• return 0;

+}

+/* based on usb_ohci.c */ +#define min_t(type, x, y) \

• ({ type __x = (x); type __y = (y); __x < __y ? __x : __y; })
  

+/*------------------------------------------------------------------------ -* + * Virtual Root Hub

*------------------------------------------------------------------------- */ + +/* Device descriptor */ +static __u8 root_hub_dev_des[] = {

How many copies of these descriptors do we need until we will have enough of them? Can you not pull them out of the other drivers so we would have only one set of them ? [...]

+static int dwc_otg_submit_rh_msg(struct usb_device *dev, unsigned long pipe, + void *buffer, int transfer_len,

• 		 struct devrequest *cmd)
  

+{

• int leni = transfer_len;
• int len = 0;
• int stat = 0;
• __u16 bmRType_bReq;
• __u16 wValue;
• __u16 wLength;
• unsigned char data[32];
• hprt0_data_t hprt0 = {.d32 = 0 };
• if (usb_pipeint(pipe)) {

• printf("Root-Hub submit IRQ: NOT implemented");
  

• return 0;
  

• }
• bmRType_bReq = cmd->requesttype | (cmd->request << 8);
• wValue = cpu_to_le16 (cmd->value);
• wLength = cpu_to_le16 (cmd->length);
• switch (bmRType_bReq) {
• case RH_GET_STATUS:

• *(__u16 *)buffer = cpu_to_le16(1);
  

What makes you believe that the pointer will have an aligned value and you won't trigger alignment fault by storing data like so ? :) Use put_unaligned.*()

• len = 2;
  

• break;
  

• case RH_GET_STATUS | RH_INTERFACE:

• *(__u16 *)buffer = cpu_to_le16(0);
  

• len = 2;
  

• break;
  

• case RH_GET_STATUS | RH_ENDPOINT:

• *(__u16 *)buffer = cpu_to_le16(0);
  

• len = 2;
  

• break;
  

• case RH_GET_STATUS | RH_CLASS:

• *(__u32 *)buffer = cpu_to_le32(0);
  

• len = 4;
  

• break;
  

• case RH_GET_STATUS | RH_OTHER | RH_CLASS:

case FOO: ; uint32_t bar;

The way to go is tuo place that semicolon there, then you would be able to declare vars. This would trim down the weird indent here. [...]

• case RH_GET_DESCRIPTOR:

• switch ((wValue & 0xff00) >> 8) {
  

• case (0x01): /* device descriptor */
  

Magic value.

[...]

diff --git a/drivers/usb/host/dwc2_otg.c b/drivers/usb/host/dwc2_otg.c new file mode 100644 index 0000000..f80a505 --- /dev/null +++ b/drivers/usb/host/dwc2_otg.c @@ -0,0 +1,2179 @@ +#include <common.h> +#include <usb.h> +#include <malloc.h> +#include "dwc2_otg.h" +#include "dwc2_otg_regs.h" +#include "dwc2_otg_core_if.h"

+#undef DWC_OTG_DEBUG

+#ifdef DWC_OTG_DEBUG

• #define PDEBUG(fmt, args...) \

• printf("[%s:%d] " fmt, \
  

• 	__PRETTY_FUNCTION__, __LINE__ , ## args)
  

+#else

• #define PDEBUG(fmt, args...) do {} while (0)

+#endif

Use debug_cond() instead of reinventing the wheel please.

+void dwc_write_reg32(volatile uint32_t *addr, uint32_t value) +{

• *addr = value;

We call a safer and more correct implementation of this writel()

+}

+uint32_t dwc_read_reg32(volatile uint32_t *addr) +{

• return *addr;

And readl() here, so scrap this please.

+}

+void dwc_modify_reg32(volatile uint32_t *addr, uint32_t clear_mask, uint32_t set_mask) +{

• uint32_t v = *addr;
• *addr = (v & ~clear_mask) | set_mask;

+}

clrsetbits_le32() ?

+/**

• • This function returns the mode of the operation, host or device.
• • @return 0 - Device Mode, 1 - Host Mode
• */

+static inline uint32_t dwc_otg_mode(dwc_otg_core_if_t *_core_if) +{

• return dwc_read_reg32(&_core_if->core_global_regs->gintsts) & 0x1;

Magic value ;)

+}

+uint8_t dwc_otg_is_host_mode(dwc_otg_core_if_t *_core_if) +{

• return dwc_otg_mode(_core_if) == DWC_HOST_MODE;

+}

+static void dwc_otg_set_uninitialized(int32_t *p, int size) +{

• int i;
• for (i = 0; i < size; i++) {

• p[i] = -1;
  

• }

Use memset() here? [...]

+uint32_t dwc_otg_read_hprt0(dwc_otg_core_if_t *_core_if) +{

• hprt0_data_t hprt0;
• hprt0.d32 = dwc_read_reg32(_core_if->host_if->hprt0);
• hprt0.b.prtena = 0;
• hprt0.b.prtconndet = 0;
• hprt0.b.prtenchng = 0;
• hprt0.b.prtovrcurrchng = 0;

I am very much not fond of these kinds of obscure bit accesses. Please define regular bits and their position and use that.

• return hprt0.d32;

+}

+/* Checks if the parameter is outside of its valid range of values */ +#define DWC_OTG_PARAM_TEST(_param_, _low_, _high_) \

• (((_param_) < (_low_)) || \
  

• ((_param_) > (_high_)))
  

Please make this into a function due to typechecking. [...]

+int32_t dwc_otg_get_param_opt(dwc_otg_core_if_t *core_if) +{

• return core_if->core_params->opt;

Do you really need to have gazillion of such accessor functions all around ?

+}

+int dwc_otg_set_param_dma_enable(dwc_otg_core_if_t *core_if, int32_t val) +{

• int retval = 0;
• if (DWC_OTG_PARAM_TEST(val, 0, 1)) {

• printf("warning: Wrong value for dma enable\n");
  

puts(), please fix globally.

[...]

diff --git a/drivers/usb/host/dwc2_otg_core_if.h b/drivers/usb/host/dwc2_otg_core_if.h new file mode 100644 index 0000000..ee85fe5 --- /dev/null +++ b/drivers/usb/host/dwc2_otg_core_if.h @@ -0,0 +1,1001 @@

+/*

• • $File: //dwh/usb_iip/dev/software/otg/linux/drivers/dwc_otg_core_if.h

$ + * $Revision: #4 $

• • $Date: 2008/12/18 $
• • $Change: 1155299 $
• • Synopsys HS OTG Linux Software Driver and documentation (hereinafter,
• • "Software") is an Unsupported proprietary work of Synopsys, Inc. unless
• • otherwise expressly agreed to in writing between Synopsys and you.
• • The Software IS NOT an item of Licensed Software or Licensed Product

under + * any End User Software License Agreement or Agreement for Licensed Product + * with Synopsys or any supplement thereto. You are permitted to use and + * redistribute this Software in source and binary forms, with or without + * modification, provided that redistributions of source code must retain this + * notice. You may not view, use, disclose, copy or distribute this file or + * any information contained herein except pursuant to this license grant from + * Synopsys. If you do not agree with this notice, including the disclaimer + * below, then you are not authorized to use the Software.

This really makes no sense, but I sense licensing issue. Tom, Wolfgang ?

• • THIS SOFTWARE IS BEING DISTRIBUTED BY SYNOPSYS SOLELY ON AN "AS IS"

BASIS + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE HEREBY DISCLAIMED. IN NO EVENT SHALL SYNOPSYS BE LIABLE FOR ANY DIRECT, + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER + * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH + * DAMAGE.

==========================================================================

[...]

+typedef struct dwc_otg_core_if dwc_otg_core_if_t;

+extern void dwc_otg_cil_init(dwc_otg_core_if_t *_core_if, const uint32_t

Why use those 'extern' keywords here ?

[...]

+typedef union gusbcfg_data {

• /** raw register data */
• uint32_t d32;
• /** register bits */
• struct {

• unsigned toutcal:3;
  

• unsigned phyif:1;
  

• unsigned ulpi_utmi_sel:1;
  

• unsigned fsintf:1;
  

• unsigned physel:1;
  

• unsigned ddrsel:1;
  

• unsigned srpcap:1;
  

• unsigned hnpcap:1;
  

• unsigned usbtrdtim:4;
  

• unsigned nptxfrwnden:1;
  

• unsigned phylpwrclksel:1;
  

• unsigned otgutmifssel:1;
  

• unsigned ulpi_fsls:1;
  

• unsigned ulpi_auto_res:1;
  

• unsigned ulpi_clk_sus_m:1;
  

• unsigned ulpi_ext_vbus_drv:1;
  

• unsigned ulpi_int_vbus_indicator:1;
  

• unsigned term_sel_dl_pulse:1;
  

• unsigned reserved23_25:3;
  

• unsigned ic_usb_cap:1;
  

• unsigned ic_traffic_pull_remove:1;
  

• unsigned tx_end_delay:1;
  

• unsigned reserved29_31:3;
  

I really don't like this kind of bit-access , please see my rant above. [...]

Cheers!