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).

>> +}

[...]

Reply via email to