Re: [PATCH v8 6/6] usb: musb: Add support for MediaTek musb controller

[Min Guo]

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