[PATCH 4/5] usb: xhci: Add Qualcomm DWC3 xHCI driver

[Marek Vasut]

On 8/19/20 8:22 PM, Tom Rini wrote:
[...]
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <usb.h>
>> +#include <linux/compat.h>
>> +#include <linux/errno.h>
>> +#include <linux/delay.h>
>> +#include <linux/usb/dwc3.h>
>> +#include <usb/xhci.h>

Please keep the list sorted.

>> +/* Declare global data pointer */
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +struct ipq_xhci_platdata {
>> +	fdt_addr_t hcd_base;
>> +	unsigned int rst_ctrl;
>> +	unsigned int hs_only;

bool ...

>> +};
>> +
>> +struct ipq_xhci {
>> +	struct ipq_xhci_platdata usb_plat;
>> +	struct xhci_ctrl ctrl;
>> +	struct udevice* dev;
>> +	struct xhci_hccr *hcd;
>> +	struct dwc3 *dwc3_reg;
>> +};
>> +
>> +void ipq_reset_usb_phy(void *data)
>> +{
>> +	unsigned int gcc_rst_ctrl;
>> +	struct ipq_xhci_platdata *platdata;
>> +	struct ipq_xhci *ipq = (struct ipq_xhci *)data;

Pass struct ipg_xhci pointer in instead of void *.

>> +	platdata = dev_get_platdata(ipq->dev);
>> +	if (platdata == NULL) {
>> +		printf("Error: %s Failed\n", __func__);

dev_err() here.

>> +		return;
>> +	}

Shouldn't this be part of a PHY driver ?

>> +	gcc_rst_ctrl = platdata->rst_ctrl;
>> +
>> +	if (gcc_rst_ctrl) {
>> +		/* assert HS PHY POR reset */
>> +		setbits_le32(gcc_rst_ctrl, 0x10);
>> +		mdelay(10);

Does it really need such lengthy delays here ?

>> +		/* assert HS PHY SRIF reset */
>> +		setbits_le32(gcc_rst_ctrl, 0x4);
>> +		mdelay(10);
>> +
>> +		/* deassert HS PHY SRIF reset and program HS PHY registers */
>> +		clrbits_le32(gcc_rst_ctrl, 0x4);
>> +		mdelay(10);
>> +
>> +		/* de-assert USB3 HS PHY POR reset */
>> +		clrbits_le32(gcc_rst_ctrl, 0x10);

This BIT(4) should likely be #define'd as a macro , same for the others.

>> +		mdelay(10);
>> +
>> +		if (!platdata->hs_only) {
>> +			/* assert SS PHY POR reset */
>> +			setbits_le32(gcc_rst_ctrl, 0x20);
>> +			mdelay(10);
>> +
>> +			/* deassert SS PHY POR reset */
>> +			clrbits_le32(gcc_rst_ctrl, 0x20);
>> +		}
>> +	}
>> +}
>> +
>> +static int ipq_xhci_core_init(struct ipq_xhci *ipq)
>> +{
>> +	int ret = 0;
>> +
>> +	ipq_reset_usb_phy((void *)ipq);
>> +
>> +	ret = dwc3_core_init(ipq->dwc3_reg);
>> +	if (ret) {
>> +		debug("%s:failed to initialize core\n", __func__);

dev_dbg()

>> +		return ret;
>> +	}
>> +
>> +	/* We are hard-coding DWC3 core to Host Mode */
>> +	dwc3_set_mode(ipq->dwc3_reg, DWC3_GCTL_PRTCAP_HOST);
>> +
>> +	return ret;
>> +}
>> +
>> +static void ipq_xhci_core_exit(struct ipq_xhci *ipq)
>> +{

Is some code missing here ?

>> +}
>> +
>> +static int xhci_usb_remove(struct udevice *dev)
>> +{
>> +	int ret;
>> +	ret = xhci_deregister(dev);
>> +
>> +	if (ret != 0) {
>> +		debug("%s:xhci deregistration failed\n", __func__);

dev_dbg()

>> +		return ret;
>> +	}
>> +
>> +	ipq_xhci_core_exit(dev_get_priv(dev));
>> +
>> +	return 0;

return ipg_xhci_core_exit() to propagate the error value (if any).

>> +}

[...]