lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 11 Jan 2013 18:10:48 +0530
From:	Vivek Gautam <gautamvivek1987@...il.com>
To:	Doug Anderson <dianders@...omium.org>
Cc:	Vivek Gautam <gautam.vivek@...sung.com>, linux-usb@...r.kernel.org,
	yulgon.kim@...sung.com, linux-samsung-soc@...r.kernel.org,
	Praveen Paneri <p.paneri@...sung.com>,
	gregkh@...uxfoundation.org, devicetree-discuss@...ts.ozlabs.org,
	jg1.han@...sung.com,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	balbi@...com, kishon@...com, Kukjin Kim <kgene.kim@...sung.com>,
	stern@...land.harvard.edu, rob.herring@...xeda.com,
	sylvester.nawrocki@...il.com
Subject: Re: [PATCH v5 2/4] usb: phy: samsung: Add host phy support to
 samsung-phy driver

Hi Doug,

Sorry!!  for the delayed response.


On Thu, Dec 20, 2012 at 4:31 AM, Doug Anderson <dianders@...omium.org> wrote:
> Vivek,
>
> I don't really have a good 10000 foot view about how all of the USB
> bits fit together, but a few detail-oriented comments below.
>
>
> On Tue, Dec 18, 2012 at 6:43 AM, Vivek Gautam <gautam.vivek@...sung.com> wrote:
>> This patch adds host phy support to samsung-usbphy.c and
>> further adds support for samsung's exynos5250 usb-phy.
>>
>> Signed-off-by: Praveen Paneri <p.paneri@...sung.com>
>> Signed-off-by: Vivek Gautam <gautam.vivek@...sung.com>
>> ---
>>  .../devicetree/bindings/usb/samsung-usbphy.txt     |   25 +-
>>  drivers/usb/phy/Kconfig                            |    2 +-
>>  drivers/usb/phy/samsung-usbphy.c                   |  465 ++++++++++++++++++--
>>  include/linux/usb/samsung_usb_phy.h                |   13 +
>>  4 files changed, 454 insertions(+), 51 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> index a7b28b2..2ec5400 100644
>> --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
>> @@ -1,23 +1,38 @@
>>  * Samsung's usb phy transceiver
>>
>> -The Samsung's phy transceiver is used for controlling usb otg phy for
>> -s3c-hsotg usb device controller.
>> +The Samsung's phy transceiver is used for controlling usb phy for
>> +s3c-hsotg as well as ehci-s5p and ohci-exynos usb controllers
>> +across Samsung SOCs.
>>  TODO: Adding the PHY binding with controller(s) according to the under
>>  developement generic PHY driver.
>>
>>  Required properties:
>> +
>> +Exynos4210:
>>  - compatible : should be "samsung,exynos4210-usbphy"
>>  - reg : base physical address of the phy registers and length of memory mapped
>>         region.
>>
>> +Exynos5250:
>> +- compatible : should be "samsung,exynos5250-usbphy"
>> +- reg : base physical address of the phy registers and length of memory mapped
>> +       region.
>> +
>>  Optional properties:
>>  - samsung,usb-phyhandle : should point to usb-phyhandle sub-node which provides
>>                         binding data to enable/disable device PHY handled by
>> -                       PMU register.
>> +                       PMU register; or to configure usb2.0 phy handled by
>> +                       SYSREG.
>>
>>                         Required properties:
>>                         - compatible : should be "samsung,usbdev-phyctrl" for
>> -                                       DEVICE type phy.
>> +                                      DEVICE type phy; or
>> +                                      should be "samsung,usbhost-phyctrl" for
>> +                                      HOST type phy; or
>> +                                      should be "samsung,usb-phycfg" for
>> +                                      USB2.0 PHY_CFG.
>>                         - samsung,phyhandle-reg: base physical address of
>> -                                               PHY_CONTROL register in PMU.
>> +                                                PHY_CONTROL register in PMU;
>> +                                                or USB2.0 PHY_CFG register
>> +                                                in SYSREG.
>>  - samsung,enable-mask : should be '1'
>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>> index 17ad743..13c0eaf 100644
>> --- a/drivers/usb/phy/Kconfig
>> +++ b/drivers/usb/phy/Kconfig
>> @@ -47,7 +47,7 @@ config USB_RCAR_PHY
>>
>>  config SAMSUNG_USBPHY
>>         bool "Samsung USB PHY controller Driver"
>> -       depends on USB_S3C_HSOTG
>> +       depends on USB_S3C_HSOTG || USB_EHCI_S5P || USB_OHCI_EXYNOS
>>         select USB_OTG_UTILS
>>         help
>>           Enable this to support Samsung USB phy controller for samsung
>> diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
>> index 4ceabe3..621348a 100644
>> --- a/drivers/usb/phy/samsung-usbphy.c
>> +++ b/drivers/usb/phy/samsung-usbphy.c
>> @@ -5,7 +5,8 @@
>>   *
>>   * Author: Praveen Paneri <p.paneri@...sung.com>
>>   *
>> - * Samsung USB2.0 High-speed OTG transceiver, talks to S3C HS OTG controller
>> + * Samsung USB-PHY transceiver; talks to S3C HS OTG controller, EHCI-S5P and
>> + * OHCI-EXYNOS controllers.
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the GNU General Public License version 2 as
>> @@ -24,7 +25,7 @@
>>  #include <linux/err.h>
>>  #include <linux/io.h>
>>  #include <linux/of.h>
>> -#include <linux/usb/otg.h>
>> +#include <linux/usb/samsung_usb_phy.h>
>>  #include <linux/platform_data/samsung-usbphy.h>
>>
>>  /* Register definitions */
>> @@ -56,6 +57,103 @@
>>  #define RSTCON_HLINK_SWRST                     (0x1 << 1)
>>  #define RSTCON_SWRST                           (0x1 << 0)
>>
>> +/* EXYNOS5 */
>> +#define EXYNOS5_PHY_HOST_CTRL0                 (0x00)
>> +
>> +#define HOST_CTRL0_PHYSWRSTALL                 (0x1 << 31)
>> +
>> +#define HOST_CTRL0_REFCLKSEL_MASK              (0x3)
>> +#define HOST_CTRL0_REFCLKSEL_XTAL              (0x0 << 19)
>> +#define HOST_CTRL0_REFCLKSEL_EXTL              (0x1 << 19)
>> +#define HOST_CTRL0_REFCLKSEL_CLKCORE           (0x2 << 19)
>> +
>> +#define HOST_CTRL0_FSEL_MASK                   (0x7 << 16)
>> +#define HOST_CTRL0_FSEL(_x)                    ((_x) << 16)
>> +#define HOST_CTRL0_FSEL_CLKSEL_50M             (0x7)
>> +#define HOST_CTRL0_FSEL_CLKSEL_24M             (0x5)
>> +#define HOST_CTRL0_FSEL_CLKSEL_20M             (0x4)
>> +#define HOST_CTRL0_FSEL_CLKSEL_19200K          (0x3)
>> +#define HOST_CTRL0_FSEL_CLKSEL_12M             (0x2)
>> +#define HOST_CTRL0_FSEL_CLKSEL_10M             (0x1)
>> +#define HOST_CTRL0_FSEL_CLKSEL_9600K           (0x0)
>
> Add the shifts to the #defines and remove HOST_CTRL0_FSEL(_x).  That
> makes it match the older phy more closely.
>
Wouldn't it hamper the readability when shifts are used ??
I mean if we have something like this -

phyhost |= HOST_CTRL0_FSEL(phyclk)

so, if we are using the shifts then
phyhost |= (HOST_CTRL0_FSEL_CLKSEL_24M << HOST_CTRL0_FSEL_SHIFT)

If you say i will add the shifts here.

>> +
>> +#define HOST_CTRL0_TESTBURNIN                  (0x1 << 11)
>> +#define HOST_CTRL0_RETENABLE                   (0x1 << 10)
>> +#define HOST_CTRL0_COMMONON_N                  (0x1 << 9)
>> +#define HOST_CTRL0_SIDDQ                       (0x1 << 6)
>> +#define HOST_CTRL0_FORCESLEEP                  (0x1 << 5)
>> +#define HOST_CTRL0_FORCESUSPEND                        (0x1 << 4)
>> +#define HOST_CTRL0_WORDINTERFACE               (0x1 << 3)
>> +#define HOST_CTRL0_UTMISWRST                   (0x1 << 2)
>> +#define HOST_CTRL0_LINKSWRST                   (0x1 << 1)
>> +#define HOST_CTRL0_PHYSWRST                    (0x1 << 0)
>> +
>> +#define EXYNOS5_PHY_HOST_TUNE0                 (0x04)
>> +
>> +#define EXYNOS5_PHY_HSIC_CTRL1                 (0x10)
>> +
>> +#define EXYNOS5_PHY_HSIC_TUNE1                 (0x14)
>> +
>> +#define EXYNOS5_PHY_HSIC_CTRL2                 (0x20)
>> +
>> +#define EXYNOS5_PHY_HSIC_TUNE2                 (0x24)
>> +
>> +#define HSIC_CTRL_REFCLKSEL_MASK               (0x3)
>> +#define HSIC_CTRL_REFCLKSEL                    (0x2 << 23)
>> +
>> +#define HSIC_CTRL_REFCLKDIV_MASK               (0x7f)
>> +#define HSIC_CTRL_REFCLKDIV(_x)                        ((_x) << 16)
>> +#define HSIC_CTRL_REFCLKDIV_12                 (0x24 << 16)
>> +#define HSIC_CTRL_REFCLKDIV_15                 (0x1c << 16)
>> +#define HSIC_CTRL_REFCLKDIV_16                 (0x1a << 16)
>> +#define HSIC_CTRL_REFCLKDIV_19_2               (0x15 << 16)
>> +#define HSIC_CTRL_REFCLKDIV_20                 (0x14)
>> +
>> +#define HSIC_CTRL_SIDDQ                                (0x1 << 6)
>> +#define HSIC_CTRL_FORCESLEEP                   (0x1 << 5)
>> +#define HSIC_CTRL_FORCESUSPEND                 (0x1 << 4)
>> +#define HSIC_CTRL_WORDINTERFACE                        (0x1 << 3)
>> +#define HSIC_CTRL_UTMISWRST                    (0x1 << 2)
>> +#define HSIC_CTRL_PHYSWRST                     (0x1 << 0)
>> +
>> +#define EXYNOS5_PHY_HOST_EHCICTRL              (0x30)
>> +
>> +#define HOST_EHCICTRL_ENAINCRXALIGN            (0x1 << 29)
>> +#define HOST_EHCICTRL_ENAINCR4                 (0x1 << 28)
>> +#define HOST_EHCICTRL_ENAINCR8                 (0x1 << 27)
>> +#define HOST_EHCICTRL_ENAINCR16                        (0x1 << 26)
>> +
>> +#define EXYNOS5_PHY_HOST_OHCICTRL              (0x34)
>> +
>> +#define HOST_OHCICTRL_SUSPLGCY                 (0x1 << 3)
>> +#define HOST_OHCICTRL_APPSTARTCLK              (0x1 << 2)
>> +#define HOST_OHCICTRL_CNTSEL                   (0x1 << 1)
>> +#define HOST_OHCICTRL_CLKCKTRST                        (0x1 << 0)
>> +
>> +#define EXYNOS5_PHY_OTG_SYS                    (0x38)
>> +
>> +#define OTG_SYS_PHYLINK_SWRESET                        (0x1 << 14)
>> +#define OTG_SYS_LINKSWRST_UOTG                 (0x1 << 13)
>> +#define OTG_SYS_PHY0_SWRST                     (0x1 << 12)
>> +
>> +#define OTG_SYS_REFCLKSEL_MASK                 (0x3 << 9)
>> +#define OTG_SYS_REFCLKSEL_XTAL                 (0x0 << 9)
>> +#define OTG_SYS_REFCLKSEL_EXTL                 (0x1 << 9)
>> +#define OTG_SYS_REFCLKSEL_CLKCORE              (0x2 << 9)
>> +
>> +#define OTG_SYS_IDPULLUP_UOTG                  (0x1 << 8)
>> +#define OTG_SYS_COMMON_ON                      (0x1 << 7)
>> +
>> +#define OTG_SYS_FSEL_MASK                      (0x7 << 4)
>> +#define OTG_SYS_FSEL(_x)                       ((_x) << 4)
>> +
>> +#define OTG_SYS_FORCESLEEP                     (0x1 << 3)
>> +#define OTG_SYS_OTGDISABLE                     (0x1 << 2)
>> +#define OTG_SYS_SIDDQ_UOTG                     (0x1 << 1)
>> +#define OTG_SYS_FORCESUSPEND                   (0x1 << 0)
>> +
>> +#define EXYNOS5_PHY_OTG_TUNE                   (0x40)
>> +
>>  #ifndef MHZ
>>  #define MHZ (1000*1000)
>>  #endif
>> @@ -63,6 +161,7 @@
>>  enum samsung_cpu_type {
>>         TYPE_S3C64XX,
>>         TYPE_EXYNOS4210,
>> +       TYPE_EXYNOS5250,
>>  };
>>
>>  /*
>> @@ -73,9 +172,13 @@ enum samsung_cpu_type {
>>   * @clk: usb phy clock
>>   * @regs: usb phy register memory base
>>   * @devctrl_reg: usb device phy-control pmu register memory base
>> + * @hostctrl_reg: usb host phy-control pmu register memory base
>> + * @phycfg_reg: usb2.0 phy-cfg system register memory base
>>   * @en_mask: enable mask
>>   * @ref_clk_freq: reference clock frequency selection
>>   * @cpu_type: machine identifier
>> + * @phy_type: It keeps track of the PHY type.
>> + * @host_usage: host_phy usage count.
>>   */
>>  struct samsung_usbphy {
>>         struct usb_phy  phy;
>> @@ -84,9 +187,13 @@ struct samsung_usbphy {
>>         struct clk      *clk;
>>         void __iomem    *regs;
>>         void __iomem    *devctrl_reg;
>> +       void __iomem    *hostctrl_reg;
>> +       void __iomem    *phycfg_reg;
>>         u32             en_mask;
>>         int             ref_clk_freq;
>>         int             cpu_type;
>> +       enum samsung_usb_phy_type phy_type;
>> +       atomic_t        host_usage;
>>  };
>>
>>  #define phy_to_sphy(x)         container_of((x), struct samsung_usbphy, phy)
>> @@ -96,26 +203,49 @@ static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)
>>         struct device_node *usb_phyctrl;
>>         u32 reg;
>>         int lenp;
>> +       int count = 0;
>>
>>         if (!sphy->dev->of_node) {
>>                 sphy->devctrl_reg = NULL;
>
> Remove init to NULL here (and in other error cases).
> ...I was going to suggest instead to add setting of hostctrl_reg and
> phycfg_reg to NULL, but then realized that we should just rely on the
> fact that the devm_kzalloc() will zero things.
>

this will change in accordance with v6 patch for pmu_isolation.

>>                 return -ENODEV;
>>         }
>>
>> -       if (of_get_property(sphy->dev->of_node, "samsung,usb-phyhandle", &lenp)) {
>> -               usb_phyctrl = of_parse_phandle(sphy->dev->of_node,
>> -                                               "samsung,usb-phyhandle", 0);
>> -               if (!usb_phyctrl) {
>> -                       dev_warn(sphy->dev, "Can't get usb-phy handle\n");
>> -                       sphy->devctrl_reg = NULL;
>> -               }
>> -
>> -               of_property_read_u32(usb_phyctrl, "samsung,phyhandle-reg", &reg);
>> +       if (of_get_property(sphy->dev->of_node,
>> +                       "samsung,usb-phyhandle", &lenp)) {
>> +               do {
>
> Indentation gets excessive here.  Can you break this into its own function?
>
ditto.

>> +                       usb_phyctrl = of_parse_phandle(sphy->dev->of_node,
>> +                                                       "samsung,usb-phyhandle",
>> +                                                       count++);
>> +                       if (!usb_phyctrl) {
>> +                               dev_warn(sphy->dev,
>> +                                       "Can't get usb-phy handle\n");
>> +                               sphy->devctrl_reg = NULL;
>
> This is a confusing way to handle the error.  Essentially you want to
> print an error if you get to the end of the function and devctrl_reg,
> hostctrl_reg, and phycfg_reg are all NULL, right?  Just check for
> that.
>
sure, will amend this.

>> +                       }
>> +
>> +                       of_property_read_u32(usb_phyctrl,
>> +                                       "samsung,phyhandle-reg", &reg);
>> +
>> +                       /*
>> +                        * Considering here the maximum number of configurable
>> +                        * register settings: DEVICE_PHY_CONTROL
>> +                        *                    HOST_PHY_CONTROL
>> +                        *                    PHY20_CFG
>> +                        */
>> +                       if (of_device_is_compatible(usb_phyctrl,
>> +                                               "samsung,usbdev-phyctrl"))
>> +                               sphy->devctrl_reg = ioremap(reg, SZ_4);
>> +                       else if (of_device_is_compatible(usb_phyctrl,
>> +                                               "samsung,usbhost-phyctrl"))
>> +                               sphy->hostctrl_reg = ioremap(reg, SZ_4);
>> +                       else if (of_device_is_compatible(usb_phyctrl,
>> +                                               "samsung,usb-phycfg"))
>> +                               sphy->phycfg_reg = ioremap(reg, SZ_4);
>> +
>> +                       of_property_read_u32(sphy->dev->of_node,
>> +                                       "samsung,enable-mask", &sphy->en_mask);
>> +               } while (of_parse_phandle(sphy->dev->of_node,
>> +                               "samsung,usb-phyhandle", count));
>
> Can't you use (*lenp) to figure out how many times to loop?  Seems
> like that would be simpler.
>

this will change in accordance with v6 patch for pmu_isolation.

>>
>> -               sphy->devctrl_reg = ioremap(reg, SZ_4);
>> -
>> -               of_property_read_u32(sphy->dev->of_node, "samsung,enable-mask",
>> -                                                       &sphy->en_mask);
>>                 of_node_put(usb_phyctrl);
>>         } else {
>>                 dev_warn(sphy->dev, "Can't get usb-phy handle\n");
>> @@ -129,13 +259,18 @@ static int samsung_usbphy_parse_dt_param(struct samsung_usbphy *sphy)
>>   * Set isolation here for phy.
>>   * SOCs control this by controlling corresponding PMU registers
>>   */
>> -static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on)
>> +static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy,
>> +                                                       int on, int type)
>
> Why add type to this function.  It's always sphy->type, isn't it?
>

Valid, :-) will change this.

>>  {
>> -       void __iomem *usb_phyctrl_reg;
>> +       void __iomem *usb_phyctrl_reg = NULL;
>>         u32 en_mask = sphy->en_mask;
>>         u32 reg;
>>
>> -       usb_phyctrl_reg = sphy->devctrl_reg;
>> +       if (type == USB_PHY_TYPE_DEVICE) {
>
> nit: no braces for single line "if".  Please run checkpatch.
>

this will change in accordance with v6 patch for pmu_isolation.

>> +               usb_phyctrl_reg = sphy->devctrl_reg;
>> +       } else if (type == USB_PHY_TYPE_HOST) {
>> +               usb_phyctrl_reg = sphy->hostctrl_reg;
>> +       }
>>
>>         if (!usb_phyctrl_reg) {
>>                 dev_warn(sphy->dev, "Can't set pmu isolation\n");
>> @@ -151,6 +286,47 @@ static void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, int on)
>>  }
>>
>>  /*
>> + * Configure the mode of working og usb-phy here: HOST/DEVICE.
>
> s/og/otg?
>

s/og/of here

>> + * SOCs control this by controlling corresponding system register
>> + */
>> +static void samsung_usbphy_cfg_sel(struct samsung_usbphy *sphy, int type)
>
> Why add type to this function.  It's always sphy->type, isn't it?
>

Sure, no need of type argument here. will change this.

>> +{
>> +       void __iomem *usb_phycfg_reg;
>> +       u32 en_mask = sphy->en_mask;
>> +       u32 reg;
>> +
>> +       usb_phycfg_reg = sphy->phycfg_reg;
>> +
>> +       if (!usb_phycfg_reg) {
>> +               dev_warn(sphy->dev, "Can't select specified phy mode\n");
>> +               return;
>> +       }
>> +
>> +       reg = readl(usb_phycfg_reg);
>> +
>> +       if (type == USB_PHY_TYPE_DEVICE)
>
> This is really confusing.  You disable for device and enable for host?  Eh?
>

This is written badly :-(
Actually resetting the bit puts it to device mode and setting the bit to Host.
Should have used a separate mask for phycfg. Will amend this.

>> +               writel(reg & ~en_mask, usb_phycfg_reg);
>> +       else if (type == USB_PHY_TYPE_HOST)
>> +               writel(reg | en_mask, usb_phycfg_reg);
>> +}
>> +
>> +/*
>> + * PHYs are different for USB Device and USB Host. Controllers can make
>> + * sure that the correct PHY type is selected by calling this function
>> + * before any PHY operation.
>> + */
>> +int samsung_usbphy_set_type(struct usb_phy *phy,
>> +                               enum samsung_usb_phy_type phy_type)
>> +{
>> +       struct samsung_usbphy *sphy = phy_to_sphy(phy);
>> +
>> +       if (sphy->phy_type != phy_type)
>> +               sphy->phy_type = phy_type;
>
> Kill useless "if"
>
Ok

>> +
>> +       return 0;
>> +}
>> +
>> +/*
>>   * Returns reference clock frequency selection value
>>   */
>>  static int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy)
>> @@ -158,34 +334,178 @@ static int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy)
>>         struct clk *ref_clk;
>>         int refclk_freq = 0;
>>
>> -       ref_clk = clk_get(sphy->dev, "xusbxti");
>> +       if (sphy->cpu_type == TYPE_EXYNOS5250)
>> +               ref_clk = clk_get(sphy->dev, "ext_xtal");
>> +       else
>> +               ref_clk = clk_get(sphy->dev, "xusbxti");
>
> Seems strange that this clock has a special different name on 5250.
> Maybe that should be fixed rather than hacking it here?
> clk_add_alias()?
>

Actually, SoC untill exynos4 were using clock "xusbxti" for USB-PHY
which has a fixed rate of 24MHz, now exynos5250 uses "ext_xtal" clock
for USB-PHY whose rate is determined at the time of clk_init (no doubt it's
also 24MHz for exynos5250).

>>         if (IS_ERR(ref_clk)) {
>>                 dev_err(sphy->dev, "Failed to get reference clock\n");
>>                 return PTR_ERR(ref_clk);
>>         }
>>
>> -       switch (clk_get_rate(ref_clk)) {
>> -       case 12 * MHZ:
>> -               refclk_freq = PHYCLK_CLKSEL_12M;
>> -               break;
>> -       case 24 * MHZ:
>> -               refclk_freq = PHYCLK_CLKSEL_24M;
>> -               break;
>> -       case 48 * MHZ:
>> -               refclk_freq = PHYCLK_CLKSEL_48M;
>> -               break;
>> -       default:
>> -               if (sphy->cpu_type == TYPE_S3C64XX)
>> -                       refclk_freq = PHYCLK_CLKSEL_48M;
>> -               else
>> +       if (sphy->cpu_type == TYPE_EXYNOS5250) {
>> +               /* set clock frequency for PLL */
>> +               switch (clk_get_rate(ref_clk)) {
>> +               case 96 * 100000:
>
> nit: change to 9600 * KHZ to match; below too.
>
sure.

>> +                       refclk_freq |= HOST_CTRL0_FSEL_CLKSEL_9600K;
>
> Why |= with 0?
>
kept this just to keep things look similar :-). will remove this line,

>> +                       break;
>> +               case 10 * MHZ:
>> +                       refclk_freq |= HOST_CTRL0_FSEL_CLKSEL_10M;
>> +                       break;
>> +               case 12 * MHZ:
>> +                       refclk_freq |= HOST_CTRL0_FSEL_CLKSEL_12M;
>> +                       break;
>> +               case 192 * 100000:
>> +                       refclk_freq |= HOST_CTRL0_FSEL_CLKSEL_19200K;
>> +                       break;
>> +               case 20 * MHZ:
>> +                       refclk_freq |= HOST_CTRL0_FSEL_CLKSEL_20M;
>> +                       break;
>> +               case 50 * MHZ:
>> +                       refclk_freq |= HOST_CTRL0_FSEL_CLKSEL_50M;
>> +                       break;
>> +               case 24 * MHZ:
>> +               default:
>> +                       /* default reference clock */
>> +                       refclk_freq |= HOST_CTRL0_FSEL_CLKSEL_24M;
>> +                       break;
>> +               }
>> +       } else {
>> +               switch (clk_get_rate(ref_clk)) {
>> +               case 12 * MHZ:
>> +                       refclk_freq = PHYCLK_CLKSEL_12M;
>> +                       break;
>> +               case 24 * MHZ:
>>                         refclk_freq = PHYCLK_CLKSEL_24M;
>> -               break;
>> +                       break;
>> +               case 48 * MHZ:
>> +                       refclk_freq = PHYCLK_CLKSEL_48M;
>> +                       break;
>> +               default:
>> +                       if (sphy->cpu_type == TYPE_S3C64XX)
>> +                               refclk_freq = PHYCLK_CLKSEL_48M;
>> +                       else
>> +                               refclk_freq = PHYCLK_CLKSEL_24M;
>> +                       break;
>> +               }
>>         }
>>         clk_put(ref_clk);
>>
>>         return refclk_freq;
>>  }
>>
>> +static int exynos5_phyhost_is_on(void *regs)
>
> return bool?
>
Yeah, sure.

>> +{
>> +       u32 reg;
>> +
>> +       reg = readl(regs + EXYNOS5_PHY_HOST_CTRL0);
>> +
>> +       return !(reg & HOST_CTRL0_SIDDQ);
>> +}
>> +
>> +static void samsung_exynos5_usbphy_enable(struct samsung_usbphy *sphy)
>> +{
>> +       void __iomem *regs = sphy->regs;
>> +       u32 phyclk = sphy->ref_clk_freq;
>> +       u32 phyhost;
>> +       u32 phyotg;
>> +       u32 phyhsic;
>> +       u32 ehcictrl;
>> +       u32 ohcictrl;
>> +
>> +       atomic_inc(&sphy->host_usage);
>
> Can you explain what situations you expect host_usage to be > 1?  Why
> is that special for exynos5?
>
this host_usage keeps a track of initialization and de-initialization of PHY.
since both ehci and ohci are customers for this phy, host_usage keeps
either of them from disabling the phy when other is still using it.

> If you really expect multiple users of this, it seems like you need
> better locking.  Without better locking one process could be in the
> middle of disabling at the same time another process is enabling,
> couldn't it?
>

Yes, sure we need to add better locking. Will check on this more.

>> +
>> +       if (exynos5_phyhost_is_on(regs)) {
>> +               dev_info(sphy->dev, "Already power on PHY\n");
>> +               return;
>> +       }
>> +
>> +       /* Selecting Host/OTG mode; After reset USB2.0PHY_CFG: HOST */
>> +       samsung_usbphy_cfg_sel(sphy, USB_PHY_TYPE_HOST);
>> +
>> +       /* Host configuration */
>> +       phyhost = readl(regs + EXYNOS5_PHY_HOST_CTRL0);
>> +
>> +       /* phy reference clock configuration */
>> +       phyhost &= ~HOST_CTRL0_FSEL_MASK;
>> +       phyhost |= HOST_CTRL0_FSEL(phyclk);
>> +
>> +       /* host phy reset */
>> +       phyhost &= ~(HOST_CTRL0_PHYSWRST |
>> +                       HOST_CTRL0_PHYSWRSTALL |
>> +                       HOST_CTRL0_SIDDQ |
>> +                       /* Enable normal mode of operation */
>> +                       HOST_CTRL0_FORCESUSPEND |
>> +                       HOST_CTRL0_FORCESLEEP);
>> +
>> +       /* Link reset */
>> +       phyhost |= (HOST_CTRL0_LINKSWRST |
>> +                       HOST_CTRL0_UTMISWRST |
>> +                       /* COMMON Block configuration during suspend */
>> +                       HOST_CTRL0_COMMONON_N);
>> +       writel(phyhost, regs + EXYNOS5_PHY_HOST_CTRL0);
>> +       udelay(10);
>> +       phyhost &= ~(HOST_CTRL0_LINKSWRST |
>> +                       HOST_CTRL0_UTMISWRST);
>> +       writel(phyhost, regs + EXYNOS5_PHY_HOST_CTRL0);
>> +
>> +       /* OTG configuration */
>> +       phyotg = readl(regs + EXYNOS5_PHY_OTG_SYS);
>> +
>> +       /* phy reference clock configuration */
>> +       phyotg &= ~OTG_SYS_FSEL_MASK;
>> +       phyotg |= OTG_SYS_FSEL(phyclk);
>> +
>> +       /* Enable normal mode of operation */
>> +       phyotg &= ~(OTG_SYS_FORCESUSPEND |
>> +                       OTG_SYS_SIDDQ_UOTG |
>> +                       OTG_SYS_FORCESLEEP |
>> +                       OTG_SYS_REFCLKSEL_MASK |
>> +                       /* COMMON Block configuration during suspend */
>> +                       OTG_SYS_COMMON_ON);
>> +
>> +       /* OTG phy & link reset */
>> +       phyotg |= (OTG_SYS_PHY0_SWRST |
>> +                       OTG_SYS_LINKSWRST_UOTG |
>> +                       OTG_SYS_PHYLINK_SWRESET |
>> +                       OTG_SYS_OTGDISABLE |
>> +                       /* Set phy refclk */
>> +                       OTG_SYS_REFCLKSEL_CLKCORE);
>> +
>> +       writel(phyotg, regs + EXYNOS5_PHY_OTG_SYS);
>> +       udelay(10);
>> +       phyotg &= ~(OTG_SYS_PHY0_SWRST |
>> +                       OTG_SYS_LINKSWRST_UOTG |
>> +                       OTG_SYS_PHYLINK_SWRESET);
>> +       writel(phyotg, regs + EXYNOS5_PHY_OTG_SYS);
>> +
>> +       /* HSIC phy configuration */
>> +       phyhsic = (HSIC_CTRL_REFCLKDIV_12 |
>> +                       HSIC_CTRL_REFCLKSEL |
>> +                       HSIC_CTRL_PHYSWRST);
>> +       writel(phyhsic, regs + EXYNOS5_PHY_HSIC_CTRL1);
>> +       writel(phyhsic, regs + EXYNOS5_PHY_HSIC_CTRL2);
>> +       udelay(10);
>> +       phyhsic &= ~HSIC_CTRL_PHYSWRST;
>> +       writel(phyhsic, regs + EXYNOS5_PHY_HSIC_CTRL1);
>> +       writel(phyhsic, regs + EXYNOS5_PHY_HSIC_CTRL2);
>> +
>> +       udelay(80);
>> +
>> +       /* enable EHCI DMA burst */
>> +       ehcictrl = readl(regs + EXYNOS5_PHY_HOST_EHCICTRL);
>> +       ehcictrl |= (HOST_EHCICTRL_ENAINCRXALIGN |
>> +                               HOST_EHCICTRL_ENAINCR4 |
>> +                               HOST_EHCICTRL_ENAINCR8 |
>> +                               HOST_EHCICTRL_ENAINCR16);
>> +       writel(ehcictrl, regs + EXYNOS5_PHY_HOST_EHCICTRL);
>> +
>> +       /* set ohci_suspend_on_n */
>> +       ohcictrl = readl(regs + EXYNOS5_PHY_HOST_OHCICTRL);
>> +       ohcictrl |= HOST_OHCICTRL_SUSPLGCY;
>> +       writel(ohcictrl, regs + EXYNOS5_PHY_HOST_OHCICTRL);
>> +}
>> +
>>  static void samsung_usbphy_enable(struct samsung_usbphy *sphy)
>>  {
>>         void __iomem *regs = sphy->regs;
>> @@ -221,6 +541,41 @@ static void samsung_usbphy_enable(struct samsung_usbphy *sphy)
>>         writel(rstcon, regs + SAMSUNG_RSTCON);
>>  }
>>
>> +static void samsung_exynos5_usbphy_disable(struct samsung_usbphy *sphy)
>> +{
>> +       void __iomem *regs = sphy->regs;
>> +       u32 phyhost;
>> +       u32 phyotg;
>> +       u32 phyhsic;
>> +
>> +       if (atomic_dec_return(&sphy->host_usage) > 0) {
>> +               dev_info(sphy->dev, "still being used\n");
>> +               return;
>> +       }
>> +
>> +       phyhsic = (HSIC_CTRL_REFCLKDIV_12 |
>> +                       HSIC_CTRL_REFCLKSEL |
>> +                       HSIC_CTRL_SIDDQ |
>> +                       HSIC_CTRL_FORCESLEEP |
>> +                       HSIC_CTRL_FORCESUSPEND);
>> +       writel(phyhsic, regs + EXYNOS5_PHY_HSIC_CTRL1);
>> +       writel(phyhsic, regs + EXYNOS5_PHY_HSIC_CTRL2);
>> +
>> +       phyhost = readl(regs + EXYNOS5_PHY_HOST_CTRL0);
>> +       phyhost |= (HOST_CTRL0_SIDDQ |
>> +                       HOST_CTRL0_FORCESUSPEND |
>> +                       HOST_CTRL0_FORCESLEEP |
>> +                       HOST_CTRL0_PHYSWRST |
>> +                       HOST_CTRL0_PHYSWRSTALL);
>> +       writel(phyhost, regs + EXYNOS5_PHY_HOST_CTRL0);
>> +
>> +       phyotg = readl(regs + EXYNOS5_PHY_OTG_SYS);
>> +       phyotg |= (OTG_SYS_FORCESUSPEND |
>> +                       OTG_SYS_SIDDQ_UOTG |
>> +                       OTG_SYS_FORCESLEEP);
>> +       writel(phyotg, regs + EXYNOS5_PHY_OTG_SYS);
>> +}
>> +
>>  static void samsung_usbphy_disable(struct samsung_usbphy *sphy)
>>  {
>>         void __iomem *regs = sphy->regs;
>> @@ -263,10 +618,13 @@ static int samsung_usbphy_init(struct usb_phy *phy)
>>         if (sphy->plat && sphy->plat->pmu_isolation)
>>                 sphy->plat->pmu_isolation(false);
>>         else
>> -               samsung_usbphy_set_isolation(sphy, false);
>> +               samsung_usbphy_set_isolation(sphy, false, sphy->phy_type);
>>
>>         /* Initialize usb phy registers */
>> -       samsung_usbphy_enable(sphy);
>> +       if (sphy->cpu_type == TYPE_EXYNOS5250)
>> +               samsung_exynos5_usbphy_enable(sphy);
>> +       else
>> +               samsung_usbphy_enable(sphy);
>>
>>         /* Disable the phy clock */
>>         clk_disable_unprepare(sphy->clk);
>> @@ -288,13 +646,16 @@ static void samsung_usbphy_shutdown(struct usb_phy *phy)
>>         }
>>
>>         /* De-initialize usb phy registers */
>> -       samsung_usbphy_disable(sphy);
>> +       if (sphy->cpu_type == TYPE_EXYNOS5250)
>> +               samsung_exynos5_usbphy_disable(sphy);
>> +       else
>> +               samsung_usbphy_disable(sphy);
>>
>>         /* Enable phy isolation */
>>         if (sphy->plat && sphy->plat->pmu_isolation)
>>                 sphy->plat->pmu_isolation(true);
>>         else
>> -               samsung_usbphy_set_isolation(sphy, true);
>> +               samsung_usbphy_set_isolation(sphy, true, sphy->phy_type);
>>
>>         clk_disable_unprepare(sphy->clk);
>>  }
>> @@ -339,12 +700,6 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
>>         if (!sphy)
>>                 return -ENOMEM;
>>
>> -       clk = devm_clk_get(dev, "otg");
>> -       if (IS_ERR(clk)) {
>> -               dev_err(dev, "Failed to get otg clock\n");
>> -               return PTR_ERR(clk);
>> -       }
>> -
>>         sphy->dev = &pdev->dev;
>>
>>         ret = samsung_usbphy_parse_dt_param(sphy);
>> @@ -361,7 +716,6 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
>>
>>         sphy->plat              = pdata;
>>         sphy->regs              = phy_base;
>> -       sphy->clk               = clk;
>>         sphy->phy.dev           = sphy->dev;
>>         sphy->phy.label         = "samsung-usbphy";
>>         sphy->phy.init          = samsung_usbphy_init;
>> @@ -371,6 +725,17 @@ static int __devinit samsung_usbphy_probe(struct platform_device *pdev)
>>
>>         platform_set_drvdata(pdev, sphy);
>>
>> +       if (sphy->cpu_type == TYPE_EXYNOS5250)
>> +               clk = devm_clk_get(dev, "usbhost");
>
> Why is 5250 special in that it is always host?  Something doesn't seem
> right here.
>

exynos5250 onward usb-phy block gets its clock from 'usb host' unlike earlier
SoCs where it was getting the clock from 'otg'
So this change was required.

> ...also, are you sure you want to move this down to after
> platform_set_drvdata()?  I don't think it will hurt anything, but
> seems better to leave it where it was and just make call
> samsung_usbphy_get_driver_data(pdev) earlier in the function.
>
Ok, will amend this as suggested.


-- 
Thanks & Regards
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists