[U-Boot] [RFC PATCH 05/11] ehci: Add support for Qualcomm EHCI

[Marek Vasut <marex@denx.de>]

On Thursday, December 10, 2015 at 10:41:41 PM, Mateusz Kulikowski wrote:
> This driver is able to reconfigure OTG controller into HOST mode.
> Board can add board-specific initialization as board_prepare_usb().
> It requires USB_ULPI_VIEWPORT enabled in board configuration.
> 
> Signed-off-by: Mateusz Kulikowski <mateusz.kulikowski@gmail.com>

Hi,

minor nits below.

[...]

> diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
> new file mode 100644
> index 0000000..d17a29a
> --- /dev/null
> +++ b/drivers/usb/host/ehci-msm.c
> @@ -0,0 +1,198 @@
> +/*
> + * Qualcomm EHCI driver
> + *
> + * (C) Copyright 2015 Mateusz Kulikowski <mateusz.kulikowski@gmail.com>
> + *
> + * Based on Linux driver
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <asm/gpio.h>
> +#include <asm-generic/errno.h>
> +#include <linux/compat.h>
> +#include <dm.h>
> +#include <fdtdec.h>
> +#include <libfdt.h>
> +#include <asm/io.h>
> +#include <usb.h>
> +#include <usb/ulpi.h>
> +#include "ehci.h"
> +
> +#ifndef CONFIG_USB_ULPI_VIEWPORT
> +#error Please enable CONFIG_USB_ULPI_VIEWPORT
> +#endif

The driver should select this in Kconfig instead of this check, right ?

> +#define MSM_USB_ULPI_OFFSET 0x170 /* ULPI viewport (PHY) */
> +#define MSM_USB_EHCI_OFFSET 0x100 /* Start of EHCI registers */
> +
> +/* PHY viewport regs */
> +#define ULPI_MISC_A_READ         0x96
> +#define ULPI_MISC_A_SET          0x97
> +#define ULPI_MISC_A_CLEAR        0x98
> +#define ULPI_MISC_A_VBUSVLDEXTSEL    (1 << 1)
> +#define ULPI_MISC_A_VBUSVLDEXT       (1 << 0)
> +
> +/* qcom specific registers (OTG) */
> +#define USB_GENCONFIG_2      0x00A0
> +#define GEN2_SESS_VLD_CTRL_EN (1 << 7)
> +
> +#define USB_USBCMD           (0x0140)

Please drop the parenthesis.

btw. this register layout looks very similar to struct usb_ehci in
include/usb/ehci-fsl.h , can the header be made more universal to
cover your driver as well ? Then these macros here won't be needed.

> +#define SESS_VLD_CTRL         (1 << 25)
> +#define USBCMD_RESET   2
> +#define USBCMD_ATTACH  1
> +
> +/* USB2_HSIC_USB_OTG_HS_BASE_USB_OTG_HS_PORTSC */
> +#define USB_PORTSC           0x0184
> +#define USB_SBUSCFG          0x0090
> +#define USB_AHB_MODE         0x0098
> +
> +#define USB_USBMODE          0x01A8
> +#define USBMODE_DEVICE 2
> +#define USBMODE_HOST   3
> +
> +struct msm_ehci_priv {
> +	struct ehci_ctrl ctrl; /* Needed by EHCI */
> +	phys_addr_t base;
> +	phys_addr_t ehci_base;
> +	u32 ulpi_base;
> +	u32 ulpi_port;
> +};
> +
> +int __weak board_prepare_usb(enum usb_init_type type)
> +{
> +	return 0;
> +}
> +
> +static void setup_usb_phy(struct msm_ehci_priv *priv)
> +{
> +	struct ulpi_viewport ulpi_vp = {.port_num = priv->ulpi_port,
> +					.viewport_addr = priv->ulpi_base};
> +
> +	/* Select and enable external configuration with USB PHY */
> +	ulpi_write(&ulpi_vp, (u8 *)ULPI_MISC_A_SET,
> +		   ULPI_MISC_A_VBUSVLDEXTSEL | ULPI_MISC_A_VBUSVLDEXT);
> +}
> +
> +static void reset_usb_phy(struct msm_ehci_priv *priv)
> +{
> +	struct ulpi_viewport ulpi_vp = {.port_num = priv->ulpi_port,
> +					.viewport_addr = priv->ulpi_base};
> +
> +	/* Disable VBUS mimicing in the controller. */
> +	ulpi_write(&ulpi_vp, (u8 *)ULPI_MISC_A_CLEAR,

This should be a pointer to a field in struct ulpi_regs, so the (u8 *)
cast does not seem right. See for example ehci-zynq.c

> +		   ULPI_MISC_A_VBUSVLDEXTSEL | ULPI_MISC_A_VBUSVLDEXT);
> +}


[...]

> +static int ehci_usb_remove(struct udevice *dev)
> +{
> +	struct msm_ehci_priv *p = dev_get_priv(dev);
> +	phys_addr_t reg = p->base + USB_USBCMD;
> +	int ret;
> +
> +	ret = ehci_deregister(dev);
> +	if (ret)
> +		return ret;
> +
> +	/* Stop controller. */
> +	writel(readl(reg) & ~USBCMD_ATTACH, reg);

This should use clrbits_le32() instead.

> +	reset_usb_phy(p);
> +
> +	ret = board_prepare_usb(USB_INIT_DEVICE); /* Board specific hook */
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Reset controller */
> +	writel(0x00080002, reg); /* reset usb */
> +	mdelay(20);
> +	/* Wait for completion */
> +	while (readl(reg) & 2)
> +		;

Look at wait_for_bit() implementations in the U-Boot tree and avoid the 
unbounded waiting here please.

> +	return 0;
> +}
> +
> +static int ehci_usb_ofdata_to_platdata(struct udevice *dev)
> +{
> +	struct msm_ehci_priv *priv = dev_get_priv(dev);
> +
> +	priv->base = dev_get_addr(dev);
> +	priv->ehci_base = priv->base + MSM_USB_EHCI_OFFSET;
> +	priv->ulpi_base = priv->base + MSM_USB_ULPI_OFFSET;
> +	priv->ulpi_port = 0;
> +	return 0;
> +}
> +
> +static const struct udevice_id ehci_usb_ids[] = {
> +	{ .compatible = "qcom,ehci-host", },
> +	{ }
> +};
> +
> +U_BOOT_DRIVER(usb_ehci) = {
> +	.name	= "ehci_msm",
> +	.id	= UCLASS_USB,
> +	.of_match = ehci_usb_ids,
> +	.ofdata_to_platdata = ehci_usb_ofdata_to_platdata,
> +	.probe = ehci_usb_probe,
> +	.remove = ehci_usb_remove,
> +	.ops	= &ehci_usb_ops,
> +	.priv_auto_alloc_size = sizeof(struct msm_ehci_priv),
> +	.flags	= DM_FLAG_ALLOC_PRIV_DMA,
> +};