Hi,
On Thu, 2019-10-17 at 09:34 -0700, Tony Lindgren wrote:
> Hi,
>
> Just few comments for future changes that might help below.
>
> * min.guo@xxxxxxxxxxxx <min.guo@xxxxxxxxxxxx> [191017 09:42]:
> > --- /dev/null
> > +++ b/drivers/usb/musb/mediatek.c
> > +static int musb_usb_role_sx_set(struct device *dev, enum usb_role role)
> > +{
> > + struct mtk_glue *glue = dev_get_drvdata(dev);
> > + struct musb *musb = glue->musb;
> > + u8 devctl = readb(musb->mregs + MUSB_DEVCTL);
> > + enum usb_role new_role;
> > +
> > + if (role == glue->role)
> > + return 0;
> > +
> > + switch (role) {
> > + case USB_ROLE_HOST:
> > + musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;
> > + glue->phy_mode = PHY_MODE_USB_HOST;
> > + new_role = USB_ROLE_HOST;
> > + if (glue->role == USB_ROLE_NONE)
> > + phy_power_on(glue->phy);
> > +
> > + devctl |= MUSB_DEVCTL_SESSION;
> > + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > + MUSB_HST_MODE(musb);
> > + break;
> > + case USB_ROLE_DEVICE:
> > + musb->xceiv->otg->state = OTG_STATE_B_IDLE;
> > + glue->phy_mode = PHY_MODE_USB_DEVICE;
> > + new_role = USB_ROLE_DEVICE;
> > + devctl &= ~MUSB_DEVCTL_SESSION;
> > + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > + if (glue->role == USB_ROLE_NONE)
> > + phy_power_on(glue->phy);
> > +
> > + MUSB_DEV_MODE(musb);
> > + break;
> > + case USB_ROLE_NONE:
> > + glue->phy_mode = PHY_MODE_USB_OTG;
> > + new_role = USB_ROLE_NONE;
> > + devctl &= ~MUSB_DEVCTL_SESSION;
> > + musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > + if (glue->role != USB_ROLE_NONE)
> > + phy_power_off(glue->phy);
> > +
> > + break;
> > + default:
> > + dev_err(glue->dev, "Invalid State\n");
> > + return -EINVAL;
> > + }
> > +
> > + glue->role = new_role;
> > + phy_set_mode(glue->phy, glue->phy_mode);
> > +
> > + return 0;
> > +}
>
> For the role change, I recently posted a patch "[PATCH 4/7] usb: musb:
> Add musb_set_host and peripheral and use them for omap2430". That
> should work for you looking at the code above, so later on you might
> want to change to use that. Probably best done as a follow-up patch
> to avoid adding extra dependencies to your series.
OK, thanks. I will try this patch.
> Please also note that musb core attempts to do things automagically
> on it's own. So trying to force mode in general does not work reliably.
> This is because VBUS may not yet have risen for example.
VBUS control is through a GPIO on MediaTek musb controller. The host/device switching method is to use the driver of gpio-usb-b-connector, it sets the debounce of GPIO, the link of patch is as follows:
Usb: roles: add USB Type-B GPIO connector driver
Https://patchwork.kernel.org/patch/10966361/
> The role change is best done based on the USB PHY as then usually
> musb has already switched to the right mode automatically :)
Considering the use of type-c connector later, the foce phy method is used.
> > +static const struct musb_platform_ops mtk_musb_ops = {
> > + .quirks = MUSB_DMA_INVENTRA,
> > + .init = mtk_musb_init,
> > + .get_toggle = mtk_musb_get_toggle,
> > + .set_toggle = mtk_musb_set_toggle,
> > + .exit = mtk_musb_exit,
> > +#ifdef CONFIG_USB_INVENTRA_DMA
> > + .dma_init = musbhs_dma_controller_create_noirq,
> > + .dma_exit = musbhs_dma_controller_destroy,
> > +#endif
> > + .clearb = mtk_musb_clearb,
> > + .clearw = mtk_musb_clearw,
> > + .busctl_offset = mtk_musb_busctl_offset,
> > + .set_mode = mtk_musb_set_mode,
> > +};
>
> So you may want to consider getting rid of .set_mode completely
> and rely on USB PHY calls instead.
>
> In some cases you need to use struct phy_companion for set_vbus
> depending how things are wired.
>
> Regards,
>
> Tony
