Hi Adam,
it's nice to include people who made review comments in the follow-up
patches. I had to pull this out of the mailinglist again.
> The imx8mm and imx8mn appear compatible with imx7d-usb
> flags in the OTG driver. If the dr_mode is defined as
> host or peripheral, the device appears to operate correctly,
> however the auto host/peripheral detection results in an error.
>
> The solution isn't just adding checks for imx8mm and imx8mn to
> the check for imx7, because the USB clock needs to be running
> to read from the USBNC_PHY_STATUS_OFFSET register or it will hang.
>
> Marek requested that I not enable the clocks in ehci_usb_of_to_plat,
> so I modified that function to return an unknown state if the
> device tree does not explicitly state whether it is a host
> or a peripheral.
>
> When the driver probes, it looks to see if it's in the unknown
> state, and only then will it read the register to auto-detect.
>
> Signed-off-by: Adam Ford <aford...@gmail.com>
> Tested-by: Tim Harvey <thar...@gateworks.com>
> ---
> V4: Fix 'err_clk' reference, so it is not encapsulated in
> an ifdef making it available at all times.
> V3: Keep ehci_usb_of_to_plat but add the ability to return
> and unknown state instead of reading the register.
> If the probe determines the states is unknown, it will
> query the register after the clocks have been enabled.
> Because of the slight behavior change, I removed any
> review or tested tags.
>
> V2: Rename ehci_usb_of_to_plat to ehci_usb_dr_mode and call it
> from the probe after t
>
> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c
> index 1bd6147c76..060b02accc 100644
> --- a/drivers/usb/host/ehci-mx6.c
> +++ b/drivers/usb/host/ehci-mx6.c
> @@ -543,7 +543,7 @@ static int ehci_usb_phy_mode(struct udevice *dev)
> plat->init_type = USB_INIT_DEVICE;
> else
> plat->init_type = USB_INIT_HOST;
> - } else if (is_mx7()) {
> + } else if (is_mx7() || is_imx8mm() || is_imx8mn()) {
> phy_status = (void __iomem *)(addr +
> USBNC_PHY_STATUS_OFFSET);
> val = readl(phy_status);
> @@ -573,9 +573,8 @@ static int ehci_usb_of_to_plat(struct udevice *dev)
> case USB_DR_MODE_PERIPHERAL:
> plat->init_type = USB_INIT_DEVICE;
> break;
> - case USB_DR_MODE_OTG:
> - case USB_DR_MODE_UNKNOWN:
Don't you want to propagate the OTG mode to the init_type field? Maybe
it will be useful sometime and IMHO makes it easier to read the code...
> - return ehci_usb_phy_mode(dev);
> + default:
> + plat->init_type = USB_INIT_UNKNOWN;
> };
>
> return 0;
> @@ -677,6 +676,20 @@ static int ehci_usb_probe(struct udevice *dev)
> mdelay(1);
> #endif
>
> + /*
> + * If the device tree didn't specify host or device,
> + * the default is USB_INIT_UNKNOWN, so we need to check
> + * the register. For imx8mm and imx8mn, the clocks need to be
> + * running first, so we defer the check until they are.
> + */
> + if (priv->init_type == USB_INIT_UNKNOWN) {
Because this implicitly also contains OTG mode, right? So despite
the comment, if the device tree includes OTG mode, this branch is
also taken.
Otherwise looks good to me:
Reviewed-by: Michael Walle <mich...@walle.cc>
> + ret = ehci_usb_phy_mode(dev);
> + if (ret)
> + goto err_clk;
> + else
> + priv->init_type = plat->init_type;
> + }
> +
> #if CONFIG_IS_ENABLED(DM_REGULATOR)
> ret = device_get_supply_regulator(dev, "vbus-supply",
> &priv->vbus_supply);
> @@ -741,8 +754,8 @@ err_regulator:
> #if CONFIG_IS_ENABLED(DM_REGULATOR)
> if (priv->vbus_supply)
> regulator_set_enable(priv->vbus_supply, false);
> -err_clk:
> #endif
> +err_clk:
> #if CONFIG_IS_ENABLED(CLK)
> clk_disable(&priv->clk);
> #else
> diff --git a/include/usb.h b/include/usb.h
> index f032de8af9..7e3796bd5b 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -163,7 +163,8 @@ struct int_queue;
> */
> enum usb_init_type {
> USB_INIT_HOST,
> - USB_INIT_DEVICE
> + USB_INIT_DEVICE,
> + USB_INIT_UNKNOWN,
> };
>
> /**********************************************************************
