Re: [PATCH 2/2] ARM: Exynos5250: Enabling USB 3.0 phy for samsung-usbphy driver

[Sylwester Nawrocki <sylvester.nawrocki@gmail.com>]

Hi Vivek,

On 11/06/2012 04:36 PM, Vivek Gautam wrote:
> Adding base address information and required platform data
> support for enabling USB DRD phy on exynos5250 SOC.
>
> Signed-off-by: Vivek Gautam<gautam.vivek@samsung.com>
> ---
>   arch/arm/boot/dts/exynos5250.dtsi            |    3 ++-
>   arch/arm/mach-exynos/include/mach/regs-pmu.h |    4 ++++
>   arch/arm/mach-exynos/setup-usb-phy.c         |    9 +++++++++
>   3 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 82bf042..51693af 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -220,7 +220,8 @@
>
>   	usbphy {
>   		compatible = "samsung,exynos5250-usbphy";
> -		reg =<0x12130000 0x100>;
> +		reg =<0x12130000 0x100>,
> +		<0x12100000 0x100>;
>   	};
>
>   	amba {
> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> index d4e392b..67132b4 100644
> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> @@ -39,6 +39,10 @@
>   #define S5P_HDMI_PHY_CONTROL			S5P_PMUREG(0x0700)
>   #define S5P_HDMI_PHY_ENABLE			(1<<  0)
>
> +/* only for EXYNOS5250*/
> +#define S5P_USBDRD_PHY_CONTROL			S5P_PMUREG(0x0704)
> +#define S5P_USBDRD_PHY_ENABLE			(1<<  0)

Hmm, couldn't it be added to your usbphy node above and then this register
left for the usb phy driver to do ioremap and control it directly ? Rather
than relying on the platform data callback ? I hoped this static mapping
can be dropped once there is a proper usb phy driver in place. AFAIU
arch/arm/mach-exynos/setup-usb-phy.c is supposed to be a non-dt only thing.

> +
>   #define S5P_DAC_PHY_CONTROL			S5P_PMUREG(0x070C)
>   #define S5P_DAC_PHY_ENABLE			(1<<  0)
>
> diff --git a/arch/arm/mach-exynos/setup-usb-phy.c b/arch/arm/mach-exynos/setup-usb-phy.c
> index 6c768e0..5e46fdd 100644
> --- a/arch/arm/mach-exynos/setup-usb-phy.c
> +++ b/arch/arm/mach-exynos/setup-usb-phy.c
> @@ -238,6 +238,15 @@ void s5p_usb_phy_pmu_isolation(int on, int type)
>   			writel(readl(S5P_USBHOST_PHY_CONTROL)
>   				| S5P_USBHOST_PHY_ENABLE,
>   					S5P_USBHOST_PHY_CONTROL);
> +	} else if (type == USB_PHY_TYPE_DRD) {
> +		if (on)
> +			writel(readl(S5P_USBDRD_PHY_CONTROL)
> +				&  ~S5P_USBDRD_PHY_ENABLE,
> +					S5P_USBDRD_PHY_CONTROL);

This is horrible coding style IMHO BTW. Why not just do

		u32 reg = readl(S5P_USBDRD_PHY_CONTROL);
		if (on)
			reg &= ~S5P_USBDRD_PHY_ENABLE;
		else
			reg |= ~S5P_USBDRD_PHY_ENABLE;
		writel(reg, S5P_USBDRD_PHY_CONTROL);

Or to create some read/modify/write helper ? Anyway, I suppose this whole
setup-usb-phy.c file is going to be removed, once exynos is completely dt
only.

> +		else
> +			writel(readl(S5P_USBDRD_PHY_CONTROL)
> +				| S5P_USBDRD_PHY_ENABLE,
> +					S5P_USBDRD_PHY_CONTROL);
>   	} else {
>   		if (on)
>   			writel(readl(S5P_USBDEVICE_PHY_CONTROL)

--

Thanks,
Sylwester