[U-Boot] [PATCH 3/5] exynos5: Add power Enable/Disable for USB-EHCI

[Rajeshwari Birje]

Hi Minkyu,

Thank you for the comments.

On Thu, May 3, 2012 at 1:03 PM, Minkyu Kang <promsoft at gmail.com> wrote:
> On 2 May 2012 22:52, Rajeshwari Shinde <rajeshwari.s at samsung.com> wrote:
>> This patch adds functions to enable/disable the power of USB
>> host controller for exynos5.
>>
>> This patch depends on the patch:
>> USB: S5P: Add ehci support.patch
>
> This is not a commit message.
> Please move it under the "---" line.
--ok
>
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek at samsung.com>
>> Signed-off-by: Che-Liang Chiou <clchiou at chromium.org>
>> Signed-off-by: Rajeshwari Shinde <rajeshwari.s at samsung.com>
>> ---
>>  arch/arm/cpu/armv7/exynos/power.c         |   59 +++++++++++++++++++++++++++++
>>  arch/arm/include/asm/arch-exynos/power.h  |    5 ++
>>  arch/arm/include/asm/arch-exynos/sysreg.h |    1 +
>>  drivers/usb/host/ehci-s5p.c               |    3 +
>>  4 files changed, 68 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/exynos/power.c b/arch/arm/cpu/armv7/exynos/power.c
>> index c765304..a943219 100644
>> --- a/arch/arm/cpu/armv7/exynos/power.c
>> +++ b/arch/arm/cpu/armv7/exynos/power.c
>> @@ -24,6 +24,8 @@
>>  #include <common.h>
>>  #include <asm/io.h>
>>  #include <asm/arch/power.h>
>> +#include <asm/arch/cpu.h>
>
> cpu.h is already included.
> please remove it.
--- will do so
>
>> +#include <asm/arch/sysreg.h>
>
> also you don't need this header file.. maybe.
-- will remove this header file.
>
>>
>>  static void exynos4_mipi_phy_control(unsigned int dev_index,
>>                                        unsigned int enable)
>> @@ -52,3 +54,60 @@ void set_mipi_phy_ctrl(unsigned int dev_index, unsigned int enable)
>>        if (cpu_is_exynos4())
>>                exynos4_mipi_phy_control(dev_index, enable);
>>  }
>> +
>> +void exynos5_ps_hold_setup(void)
>> +{
>> +       struct exynos5_power *power =
>> +               (struct exynos5_power *)samsung_get_base_power();
>> +
>> +       /* Set PS-Hold high */
>> +       setbits_le32(&power->ps_hold_control, POWER_PS_HOLD_CONTROL_DATA_HIGH);
>> +}
>> +
>> +void exynos5_enable_usb_phy(void)
>> +{
>> +       struct exynos5_sysreg *sysreg =
>> +               (struct exynos5_sysreg *)samsung_get_base_sysreg();
>> +       struct exynos5_power *power =
>> +               (struct exynos5_power *)samsung_get_base_power();
>> +       unsigned int phy_cfg;
>> +
>> +       /* Setting USB20PHY_CONFIG register to USB 2.0 HOST link */
>> +       phy_cfg = readl(&sysreg->usb20_phy_cfg);
>> +       if (phy_cfg & USB20_PHY_CFG_EN) {
>> +               debug("USB 2.0 HOST link already selected\n");
>
> Is it useful information for your debugging? just question :)
-- will remove the same :)

>
>> +       } else {
>> +               phy_cfg |= USB20_PHY_CFG_EN;
>> +               writel(phy_cfg, &sysreg->usb20_phy_cfg);
>
> NAK.
> The codes that are related with sysreg should be moved to system.c.
-- will correct this
>
>> +       }
>> +
>> +       /* Enabling USBHOST_PHY */
>> +       setbits_le32(&power->usbhost_phy_control, POWER_USB_HOST_PHY_CTRL_EN);
>> +}
>> +
>> +void exynos5_disable_usb_phy(void)
>> +{
>> +       struct exynos5_power *power =
>> +               (struct exynos5_power *)samsung_get_base_power();
>> +
>> +       /* Disabling USBHost_PHY */
>> +       clrbits_le32(&power->usbhost_phy_control, POWER_USB_HOST_PHY_CTRL_EN);
>> +}
>
> I think, you can combine enable/disable function to one.
-- ok
>
>> +
>> +void ps_hold_setup(void)
>> +{
>> +       if (cpu_is_exynos5())
>> +               exynos5_ps_hold_setup();
>> +}
>> +
>> +void power_enable_usb_phy(void)
>> +{
>> +       if (cpu_is_exynos5())
>> +               exynos5_enable_usb_phy();
>> +}
>> +
>> +void power_disable_usb_phy(void)
>> +{
>> +       if (cpu_is_exynos5())
>> +               exynos5_disable_usb_phy();
>> +}
>> diff --git a/arch/arm/include/asm/arch-exynos/power.h b/arch/arm/include/asm/arch-exynos/power.h
>> index 4236beb..4e2448b 100644
>> --- a/arch/arm/include/asm/arch-exynos/power.h
>> +++ b/arch/arm/include/asm/arch-exynos/power.h
>> @@ -855,4 +855,9 @@ void set_mipi_phy_ctrl(unsigned int dev_index, unsigned int enable);
>>  #define EXYNOS_MIPI_PHY_SRESETN                (1 << 1)
>>  #define EXYNOS_MIPI_PHY_MRESETN                (1 << 2)
>>
>> +#define POWER_USB_HOST_PHY_CTRL_EN             (1 << 0)
>> +#define POWER_PS_HOLD_CONTROL_DATA_HIGH                (1 << 8)
>> +void power_enable_usb_phy(void);
>> +void power_disable_usb_phy(void);
>> +
>>  #endif
>> diff --git a/arch/arm/include/asm/arch-exynos/sysreg.h b/arch/arm/include/asm/arch-exynos/sysreg.h
>> index aca4b2b..2d8d35a 100644
>> --- a/arch/arm/include/asm/arch-exynos/sysreg.h
>> +++ b/arch/arm/include/asm/arch-exynos/sysreg.h
>> @@ -40,4 +40,5 @@ struct exynos5_sysreg {
>>  };
>>  #endif
>>
>> +#define USB20_PHY_CFG_EN       (1 << 0)
>>  #endif
>> diff --git a/drivers/usb/host/ehci-s5p.c b/drivers/usb/host/ehci-s5p.c
>> index 4dd4ec1..e575c48 100644
>> --- a/drivers/usb/host/ehci-s5p.c
>> +++ b/drivers/usb/host/ehci-s5p.c
>> @@ -30,6 +30,7 @@
>>  /* Setup the EHCI host controller. */
>>  static void setup_usb_phy(struct s5p_usb_phy *usb)
>>  {
>> +       power_enable_usb_phy();
>
> need space here.
> and you should separate the function for power and system.
-- ok
>
>>        clrbits_le32(&usb->usbphyctrl0,
>>                        HOST_CTRL0_FSEL_MASK |
>>                        HOST_CTRL0_COMMONON_N |
>> @@ -70,6 +71,8 @@ static void reset_usb_phy(struct s5p_usb_phy *usb)
>>                        HOST_CTRL0_SIDDQ |
>>                        HOST_CTRL0_FORCESUSPEND |
>>                        HOST_CTRL0_FORCESLEEP);
>> +
>> +       power_disable_usb_phy();
>>  }
>>
>>  /*
>> --
>
> Thanks.
> Minkyu Kang.
> --
> from. prom.
> www.promsoft.net
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

Regards,
Rajeshwari Shinde.