Re: [PATCH RESEND 1/4] ARM: EXYNOS: Add usb otg phy control for EXYNOS4210

[Lukasz Majewski <l.majewski@samsung.com>]

Hi Kukjin,

> Lukasz Majewski wrote:
> > 
> > This patch supports to control usb otg phy of EXYNOS4210.
> > 
> > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > [Rebased on the newest git/kgene/linux-samsung #for-next]
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > ---
> >  arch/arm/mach-exynos/include/mach/irqs.h     |    1 +
> >  arch/arm/mach-exynos/include/mach/map.h      |    4 +
> >  arch/arm/mach-exynos/include/mach/regs-pmu.h |    3 +
> >  arch/arm/mach-exynos/setup-usb-phy.c         |   95
> +++++++++++++++++++-----
> > --
> >  4 files changed, 78 insertions(+), 25 deletions(-)
> > 
> 
> [...]
> 
> Hi Lukasz,
> 
> I have small comments on this.
> 
> > +static int exynos4_usb_phy0_init(struct platform_device *pdev)
> > +{
> > +	u32 rstcon;
> > +
> > +	writel(readl(S5P_USBDEVICE_PHY_CONTROL) |
> > S5P_USBDEVICE_PHY_ENABLE,
> > +			S5P_USBDEVICE_PHY_CONTROL);
> > +
> > +	exynos4_usb_phy_clkset(pdev);
> > +
> > +	/* set to normal PHY0 */
> > +	writel((readl(EXYNOS4_PHYPWR) & ~PHY0_NORMAL_MASK),
> > EXYNOS4_PHYPWR); +
> > +	/* reset PHY0 and Link */
> > +	rstcon = readl(EXYNOS4_RSTCON) | PHY0_SWRST_MASK;
> > +	writel(rstcon, EXYNOS4_RSTCON);
> > +	udelay(10);
> > +
> > +	rstcon &= ~PHY0_SWRST_MASK;
> > +	writel(rstcon, EXYNOS4_RSTCON);
> > +	udelay(80);
> 
> Do we _really_ need above udelay(80)?
> 
> As I know, we only need it in the phy1_init() for EHCI and we don't
> need it here.

I will test if this removal not break anything. 

> 
> [...]
> 
> >  int s5p_usb_phy_init(struct platform_device *pdev, int type)
> >  {
> > -	if (type == S5P_USB_PHY_HOST)
> > +	if (type == S5P_USB_PHY_DEVICE)
> > +		return exynos4_usb_phy0_init(pdev);
> 
> I think, this should be exynos4210_usb_phy0_init(pdev), because this
> is available on only exynos4210 not 

> exynos4x12 and exynos5.

I will change the name and submit a patch.

> 
> > +	else if (type == S5P_USB_PHY_HOST)
> >  		return exynos4_usb_phy1_init(pdev);
> 
> Same as above.
> 
> > 
> >  	return -EINVAL;
> > @@ -144,7 +187,9 @@ int s5p_usb_phy_init(struct platform_device
> > *pdev, int type)
> > 
> >  int s5p_usb_phy_exit(struct platform_device *pdev, int type)
> >  {
> > -	if (type == S5P_USB_PHY_HOST)
> > +	if (type == S5P_USB_PHY_DEVICE)
> > +		return exynos4_usb_phy0_exit(pdev);
> 
> Same as above.
> 
> > +	else if (type == S5P_USB_PHY_HOST)
> >  		return exynos4_usb_phy1_exit(pdev);
> 
> Same as above.
> 
> If you're ok on my comments, could you please update it as soon as
> possible for v3.5?

I will prepare a patch.

> 
> Thanks.
> 
> Best regards,
> Kgene.
> --
> Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
> SW Solution Development Team, Samsung Electronics Co., Ltd.
> 



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group