Re: [U-Boot] [PATCH 2/5] sunxi: add USB EHCI driver

[Ian Campbell]

On Tue, 2014-07-08 at 22:21 +0200, Roman Byshko wrote:
> diff --git a/drivers/usb/host/ehci-sunxi.c b/drivers/usb/host/ehci-sunxi.c
> new file mode 100644
> index 000..5817fc7
> --- /dev/null
> +++ b/drivers/usb/host/ehci-sunxi.c
> @@ -0,0 +1,236 @@
> +/*
> + * Copyright (C) 2014 arokux
> + *
> + * arokux 

The authorship info here contradicts the authorship suggested by the
commit email metadata and the S-o-b.

If arokux wrote this patch then the commit message should begin with 

From: Arokux X 

on the first line and arokux needs to have Signed-off on the patch.

I'm not sure what u-boot policy says about pseudonyms.

> + *
> + * Based on code from
> + * Allwinner Technology Co., Ltd. 
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.

Please use the SPDX tag instead of the full license.

> + dat = readl(&pio->dat);
> + if (val)
> + dat |= 0x1 << num;
> + else
> + dat &= ~(0x1 << num);
> +
> + writel(dat, &pio->dat);

This is 
if (val)
setbits_le32(&pio->dat, 1 +}
> +
> +static void usb_phy_write(struct sunxi_ehci_hcd *sunxi_ehci, int addr,
> +   int data, int len)
> +{
> + int temp = 0, j = 0, usbc_bit = 0;
> + void *dest = sunxi_ehci->csr;
> +
> + usbc_bit = BIT(sunxi_ehci->id * 2);
> + for (j = 0; j < len; j++) {
> + /* set the bit address to be written */
> + temp = readl(dest);
> + temp &= ~(0xff << 8);
> + temp |= ((addr + j) << 8);
> + writel(temp, dest);

clrsetbits?

> +
> + clrbits_le32(dest, usbc_bit);
> + /* set data bit */
> + if (data & 0x1)
> + temp |= BIT(7);
> + else
> + temp &= ~BIT(7);
> + writeb(temp, dest);

AFAICT this will clobber the clearing of usbc_bit which you just did,
since temp was read before that, is that right?

This should probably use the clr/setbits_le32 stuff and if it really is
deliberately clobbering usbc_bit I think it needs a comment.

> +
> + setbits_le32(dest, usbc_bit);
> +
> + clrbits_le32( dest, usbc_bit);

Extra space in this line.

> + /* gpio_direction_output(sunxi_ehci->gpio_vbus, 1); */

???

> +static void sunxi_ehci_disable(struct sunxi_ehci_hcd *sunxi_ehci)
> +{
> + struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +
> + /* gpio_direction_output(sunxi_ehci->gpio_vbus, 0); */

???

Ian.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/5] sunxi: add USB EHCI driver

[Roman Byshko]

Signed-off-by: Roman Byshko 
---
 drivers/usb/host/Makefile |   1 +
 drivers/usb/host/ehci-sunxi.c | 236 ++
 2 files changed, 237 insertions(+)
 create mode 100644 drivers/usb/host/ehci-sunxi.c

diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 04c1a64..c4f5157 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_USB_EHCI_PPC4XX) += ehci-ppc4xx.o
 obj-$(CONFIG_USB_EHCI_MARVELL) += ehci-marvell.o
 obj-$(CONFIG_USB_EHCI_PCI) += ehci-pci.o
 obj-$(CONFIG_USB_EHCI_SPEAR) += ehci-spear.o
+obj-$(CONFIG_USB_EHCI_SUNXI) += ehci-sunxi.o
 obj-$(CONFIG_USB_EHCI_TEGRA) += ehci-tegra.o
 obj-$(CONFIG_USB_EHCI_VCT) += ehci-vct.o
 obj-$(CONFIG_USB_EHCI_RMOBILE) += ehci-rmobile.o
diff --git a/drivers/usb/host/ehci-sunxi.c b/drivers/usb/host/ehci-sunxi.c
new file mode 100644
index 000..5817fc7
--- /dev/null
+++ b/drivers/usb/host/ehci-sunxi.c
@@ -0,0 +1,236 @@
+/*
+ * Copyright (C) 2014 arokux
+ *
+ * arokux 
+ *
+ * Based on code from
+ * Allwinner Technology Co., Ltd. 
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "ehci.h"
+
+#define BIT(x) (1 << (x))
+
+#define SUNXI_USB1_IO_BASE 0x01c14000
+#define SUNXI_USB2_IO_BASE 0x01c1c000
+
+#define SUNXI_USB_PMU_IRQ_ENABLE   0x800
+#define SUNXI_USB_CSR  0x01c13404
+#define SUNXI_USB_PASSBY_EN1
+
+#define SUNXI_EHCI_AHB_ICHR8_ENBIT(10)
+#define SUNXI_EHCI_AHB_INCR4_BURST_EN  BIT(9)
+#define SUNXI_EHCI_AHB_INCRX_ALIGN_EN  BIT(8)
+#define SUNXI_EHCI_ULPI_BYPASS_EN  BIT(0)
+
+static struct sunxi_ehci_hcd {
+   void *ehci_base;
+   struct usb_hcd *hcd;
+   int usb_rst_mask;
+   int ahb_clk_mask;
+   int gpio_vbus;
+   void *csr;
+   int irq;
+   int id;
+} sunxi_echi_hcd[CONFIG_USB_MAX_CONTROLLER_COUNT] = {
+   [0] = {
+   .ehci_base = (void *) SUNXI_USB1_IO_BASE,
+   .usb_rst_mask = CCM_USB_CTRL_PHY1_RST,
+   .ahb_clk_mask = BIT(AHB_GATE_OFFSET_USB_EHCI0),
+   .gpio_vbus = CONFIG_SUNXI_USB_VBUS0_GPIO,
+   .csr = (void*) SUNXI_USB_CSR,
+   .irq = 39,
+   .id = 1,
+   },
+#if (CONFIG_USB_MAX_CONTROLLER_COUNT > 1)
+   [1] = {
+   .ehci_base = (void *) SUNXI_USB2_IO_BASE,
+   .usb_rst_mask = CCM_USB_CTRL_PHY2_RST,
+   .ahb_clk_mask = BIT(AHB_GATE_OFFSET_USB_EHCI1),
+   .gpio_vbus = CONFIG_SUNXI_USB_VBUS1_GPIO,
+   .csr = (void*) SUNXI_USB_CSR,
+   .irq = 40,
+   .id = 2,
+   }
+#endif
+};
+
+static int sunxi_gpio_output(u32 pin, u32 val)
+{
+   u32 dat;
+   u32 bank = GPIO_BANK(pin);
+   u32 num = GPIO_NUM(pin);
+   struct sunxi_gpio *pio =
+   &((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[bank];
+
+   dat = readl(&pio->dat);
+   if (val)
+   dat |= 0x1 << num;
+   else
+   dat &= ~(0x1 << num);
+
+   writel(dat, &pio->dat);
+
+   return 0;
+}
+
+static void usb_phy_write(struct sunxi_ehci_hcd *sunxi_ehci, int addr,
+ int data, int len)
+{
+   int temp = 0, j = 0, usbc_bit = 0;
+   void *dest = sunxi_ehci->csr;
+
+   usbc_bit = BIT(sunxi_ehci->id * 2);
+   for (j = 0; j < len; j++) {
+   /* set the bit address to be written */
+   temp = readl(dest);
+   temp &= ~(0xff << 8);
+   temp |= ((addr + j) << 8);
+   writel(temp, dest);
+
+   clrbits_le32(dest, usbc_bit);
+   /* set data bit */
+   if (data & 0x1)
+   temp |= BIT(7);
+   else
+   temp &= ~BIT(7);
+   writeb(temp, dest);
+
+   setbits_le32(dest, usbc_bit);
+
+   clrbits_le32( dest, usbc_bit);
+
+   data >>= 1;
+   }
+}
+
+static void sunxi_usb_phy_init(struct sunxi_ehci_hcd *sunxi_ehci)
+{
+   /* The following comments are machine
+* translated from Chinese, you hav