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

[Marek Vasut]

On Tuesday, July 15, 2014 at 11:56:49 PM, Roman Byshko wrote:

Please start writing commit messages for the patches.
[...]
> diff --git a/drivers/usb/host/ehci-sunxi.c b/drivers/usb/host/ehci-sunxi.c
> new file mode 100644
> index 0000000..8e2baa9
> --- /dev/null
> +++ b/drivers/usb/host/ehci-sunxi.c
> @@ -0,0 +1,212 @@
> +/*
> + * Copyright (C) 2014 Roman Byshko
> + *
> + * Roman Byshko <rbyshko at gmail.com>
> + *
> + * Based on code from
> + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <asm/arch/clock.h>
> +#include <asm/arch/clock.h>
> +#include <asm/arch/cpu.h>
> +#include <asm/arch/gpio.h>
> +#include <asm/io.h>
> +#include <common.h>
> +#include "ehci.h"
> +
> +#define BIT(x)	(1 << (x))

Please remove this code obfuscation, just use (1 << n) in the definitions below.

> +#define SUNXI_USB1_IO_BASE		0x01c14000
> +#define SUNXI_USB2_IO_BASE		0x01c1c000

Please implement an get_io_base() or such function, since these addresses are 
likely to change eventually.

> +#define SUNXI_USB_PMU_IRQ_ENABLE	0x800
> +#define SUNXI_USB_CSR			0x01c13404
> +#define SUNXI_USB_PASSBY_EN		1
> +
> +#define SUNXI_EHCI_AHB_ICHR8_EN		BIT(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] = {

No need to use this [CONFIG...] , just use [] and the compiler will calculate 
the size.

> +	[0] = {

No need for such explicit enumeration.

> +		.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 bank = GPIO_BANK(pin);
> +	u32 num = GPIO_NUM(pin);
> +	struct sunxi_gpio *pio =
> +		&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[bank];

Is this still an USB driver or is this now a GPIO driver ?

> +	if (val)
> +		setbits_le32(&pio->dat, 0x1 << num);
> +	else
> +		clrbits_le32(&pio->dat, 0x1 << num);
> +
> +	return 0;
> +}

[...]

> +static void sunxi_ehci_enable(struct sunxi_ehci_hcd *sunxi_ehci)
> +{
> +	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +
> +	setbits_le32(&ccm->usb_clk_cfg, sunxi_ehci->usb_rst_mask);
> +	setbits_le32(&ccm->ahb_gate0, sunxi_ehci->ahb_clk_mask);
> +
> +	sunxi_usb_phy_init(sunxi_ehci);
> +
> +	sunxi_usb_passby(sunxi_ehci, SUNXI_USB_PASSBY_EN);
> +
> +	/* this should be used instead of next two lines if
> +	 * sunxi_gpio.c is merged upstream
> +	 * gpio_direction_output(sunxi_ehci->gpio_vbus, 1); */

Please fix the comment ( http://www.denx.de/wiki/U-Boot/CodingStyle )

> +	sunxi_gpio_set_cfgpin(sunxi_ehci->gpio_vbus, SUNXI_GPIO_OUTPUT);
> +	sunxi_gpio_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;
> +
> +	/* this should be used instead of next two lines if
> +	 * sunxi_gpio.c is merged upstream
> +	 * gpio_direction_output(sunxi_ehci->gpio_vbus, 0); */

DTTO.

> +	sunxi_gpio_set_cfgpin(sunxi_ehci->gpio_vbus, SUNXI_GPIO_OUTPUT);
> +	sunxi_gpio_output(sunxi_ehci->gpio_vbus, 0);
> +
> +	sunxi_usb_passby(sunxi_ehci, !SUNXI_USB_PASSBY_EN);
> +
> +	clrbits_le32(&ccm->ahb_gate0, sunxi_ehci->ahb_clk_mask);
> +	clrbits_le32(&ccm->usb_clk_cfg, sunxi_ehci->usb_rst_mask);
> +}
> +
> +int ehci_hcd_init(int index, enum usb_init_type init, struct ehci_hccr
> **hccr, +		struct ehci_hcor **hcor)
> +{
> +	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +	struct sunxi_ehci_hcd *sunxi_ehci = &sunxi_echi_hcd[index];
> +
> +	/* enable common PHY only once */
> +	if (index == 0)
> +		setbits_le32(&ccm->usb_clk_cfg, CCM_USB_CTRL_PHYGATE);

This would fail if I enabled only controller #1 , which is perfectly legal 
operation. Just add a counter here and disable the clock upon last call of 
ehci_hcd_stop() .

> +	sunxi_ehci_enable(sunxi_ehci);
> +
> +	*hccr = sunxi_ehci->ehci_base;
> +
> +	*hcor = (struct ehci_hcor *)((uint32_t) *hccr
> +				+ HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
> +
> +	debug("sunxi-ehci: init hccr %x and hcor %x hc_length %d\n",
> +		(uint32_t)*hccr, (uint32_t)*hcor,
> +		(uint32_t)HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));
> +
> +	return 0;
> +}
> +
> +int ehci_hcd_stop(int index)
> +{
> +	struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +	struct sunxi_ehci_hcd *sunxi_ehci = &sunxi_echi_hcd[index];
> +
> +	sunxi_ehci_disable(sunxi_ehci);
> +
> +	/* disable common PHY only once, for the last hcd */
> +	if (index == CONFIG_USB_MAX_CONTROLLER_COUNT - 1)
> +		clrbits_le32(&ccm->usb_clk_cfg, CCM_USB_CTRL_PHYGATE);
> +
> +	return 0;
> +}