[PATCH RFC 3/4] xhci: Tune PHY for the DWC3-Exynos host controller

[Vivek Gautam]

The DWC3-exynos eXtensible host controller on Exynos5420 SoC
is quirky in a way that the PHY needs to be tuned to get it
working at SuperSpeed.
Add relevant calls for tuning the PHY for DWC3-Exynos's
host controller, for that matter passing just USB3 PHY
from DWC3 core, which is saved in secondary HCD of XHCI.

Signed-off-by: Vivek Gautam <gautam...@samsung.com>
---
drivers/usb/dwc3/host.c | 7 ++++++
drivers/usb/host/xhci-plat.c | 43 ++++++++++++++++++++++++++++++++++++++++-
include/linux/usb/hcd.h | 1 +
3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 32db328..cc1f6ff 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -46,6 +46,13 @@ int dwc3_host_init(struct dwc3 *dwc)
goto err1;
}

+ ret = platform_device_add_data(xhci, &dwc->usb3_generic_phy,
+ sizeof(dwc->usb3_generic_phy));
+ if (ret) {
+ dev_err(dwc->dev, "failed to add platform data\n");
+ goto err1;
+ }
+
ret = platform_device_add(xhci);
if (ret) {
dev_err(dwc->dev, "failed to register xHCI device\n");
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 395c9e9..a0f3cbc 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -16,6 +16,7 @@
#include <linux/slab.h>
#include <linux/of.h>
#include <linux/dma-mapping.h>
+#include <linux/phy/phy.h>

#include "xhci.h"

@@ -51,7 +52,24 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
/* called during probe() after chip reset completes */
static int xhci_plat_setup(struct usb_hcd *hcd)
{
- return xhci_gen_setup(hcd, xhci_plat_quirks);
+ struct xhci_hcd *xhci;
+ int ret = 0;
+
+ ret = xhci_gen_setup(hcd, xhci_plat_quirks);
+ if (ret) {
+ dev_err(hcd->self.controller, "xhci setup failed\n");
+ goto err0;
+ }
+
+ /* Valid for secondary HCD only which gives SuperSpeed ports */
+ if (!usb_hcd_is_primary_hcd(hcd)) {
+ xhci = hcd_to_xhci(hcd);
+ if (xhci->quirks & XHCI_DWC3_EXYNOS)
+ phy_tune(hcd->phy_generic);
+ }
+
+err0:
+ return ret;
}

static const struct hc_driver xhci_plat_xhci_driver = {
@@ -111,6 +129,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
struct usb_hcd *hcd;
int ret;
int irq;
+ struct phy **phy_generic;

if (usb_disabled())
return -ENODEV;
@@ -170,6 +189,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
}

/*
+ * The parent of the xhci-plat device may pass in a PHY via
+ * platform data. If it exists, store it in our struct usb_hcd
+ * so that we can use it later.
+ */
+ phy_generic = dev_get_platdata(&pdev->dev);
+ if (phy_generic)
+ xhci->shared_hcd->phy_generic = *phy_generic;
+
+ /*
* Set the xHCI pointer before xhci_plat_setup() (aka hcd_driver.reset)
* is called by usb_add_hcd().
*/
@@ -229,8 +257,19 @@ static int xhci_plat_resume(struct device *dev)
{
struct usb_hcd *hcd = dev_get_drvdata(dev);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ int ret;
+
+ ret = xhci_resume(xhci, 0);
+ if (ret)
+ return ret;

- return xhci_resume(xhci, 0);
+ /* Valid for secondary HCD only which gives SuperSpeed ports */
+ if (!usb_hcd_is_primary_hcd(hcd)) {
+ if (xhci->quirks & XHCI_DWC3_EXYNOS)
+ phy_tune(hcd->phy_generic);
+ }
+
+ return 0;
}

static const struct dev_pm_ops xhci_plat_pm_ops = {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index b8aba19..241ed2b 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -107,6 +107,7 @@ struct usb_hcd {
* other external phys should be software-transparent
*/
struct usb_phy *phy;
+ struct phy *phy_generic;

/* Flags that need to be manipulated atomically because they can
* change while the host controller is running. Always use
--
1.7.6.5

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

[Vivek Gautam]

Adding phy tune callback, which facilitates tuning USB 3.0 PHY
present on Exynos5420.
Basically, Exynos5420 has 28nm PHY for which Loss-of-Signal (LOS)
Detector Threshold Level should be controlled for Super-Speed
operations. We are using CR_port for this purpose to send
required data to override the LOS values.

On testing with USB 3.0 devices on USB 3.0 port present on
SMDK5420, should see following message:
usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd

and without this patch, should see below shown message:
usb 2-1: new high-speed USB device number 2 using xhci-hcd

Signed-off-by: Vivek Gautam <gautam...@samsung.com>
---

drivers/phy/phy-exynos5-usb3.c | 107 ++++++++++++++++++++++++++++++++++++++++
1 files changed, 107 insertions(+), 0 deletions(-)

diff --git a/drivers/phy/phy-exynos5-usb3.c b/drivers/phy/phy-exynos5-usb3.c
index 2bafc9d..669f998 100644
--- a/drivers/phy/phy-exynos5-usb3.c
+++ b/drivers/phy/phy-exynos5-usb3.c
@@ -84,8 +84,20 @@
#define PHYCLKRST_COMMONONN (0x1 << 0)

#define EXYNOS5_DRD_PHYREG0 (0x14)
+
+#define EXYNOS5_DRD_PHYREG0_SSC_REF_CLK_SEL (1 << 21)
+#define EXYNOS5_DRD_PHYREG0_SSC_RANGE (1 << 20)
+#define EXYNOS5_DRD_PHYREG0_CR_WRITE (1 << 19)
+#define EXYNOS5_DRD_PHYREG0_CR_READ (1 << 18)
+#define EXYNOS5_DRD_PHYREG0_CR_DATA_IN(_x) ((_x) << 2)
+#define EXYNOS5_DRD_PHYREG0_CR_CAP_DATA (1 << 1)
+#define EXYNOS5_DRD_PHYREG0_CR_CAP_ADDR (1 << 0)
+
#define EXYNOS5_DRD_PHYREG1 (0x18)

+#define EXYNOS5_DRD_PHYREG1_CR_DATA_OUT(_x) ((_x) << 1)
+#define EXYNOS5_DRD_PHYREG1_CR_ACK (1 << 0)
+
#define EXYNOS5_DRD_PHYPARAM0 (0x1c)

#define PHYPARAM0_REF_USE_PAD (0x1 << 31)
@@ -113,6 +125,18 @@
#define EXYNOS5_DRD_PHYRESUME (0x34)
#define EXYNOS5_DRD_LINKPORT (0x44)

+/* USB 3.0 DRD PHY SS Function Control Reg; accessed by CR_PORT */
+#define EXYNOS5_DRD_PHYSS_LOSLEVEL_OVRD_IN (0x15)
+
+#define LOSLEVEL_OVRD_IN_LOS_BIAS_5420 (0x5 << 13)
+#define LOSLEVEL_OVRD_IN_LOS_BIAS_DEFAULT (0x0 << 13)
+#define LOSLEVEL_OVRD_IN_EN (0x1 << 10)
+#define LOSLEVEL_OVRD_IN_LOS_LEVEL_DEFAULT (0x9 << 0)
+
+#define EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN (0x12)
+#define TX_VBOOSTLEVEL_OVRD_IN_VBOOST_5420 (0x5 << 13)
+#define TX_VBOOSTLEVEL_OVRD_IN_VBOOST_DEFAULT (0x4 << 13)
+
/* Power isolation defined in power management unit */
#define EXYNOS5_USB3DRD_PHY_PMU_REG_OFFSET (0x704)
#define EXYNOS5_USB3DRD_PMU_ISOL (1 << 0)
@@ -124,6 +148,7 @@ struct usb3phy_config {
bool has_usb30_sclk;
u32 reg_pmu_offset;
bool has_multi_controller;
+ bool need_crport_tuning;
};

struct usb3phy_driver {
@@ -235,6 +260,53 @@ static u32 exynos5_usb3phy_set_refclk(struct usb3phy_driver *drv)
return reg;
}

+static void crport_handshake(struct usb3phy_driver *drv, u32 val, u32 cmd)
+{
+ u32 usec = 100;
+ u32 result;
+
+ writel(val | cmd, drv->reg_phy + EXYNOS5_DRD_PHYREG0);
+
+ do {
+ result = readl(drv->reg_phy + EXYNOS5_DRD_PHYREG1);
+ if (result & EXYNOS5_DRD_PHYREG1_CR_ACK)
+ break;
+
+ udelay(1);
+ } while (usec-- > 0);
+
+ if (!usec)
+ dev_err(drv->dev, "CRPORT handshake timeout1 (0x%08x)\n", val);
+
+ usec = 100;
+
+ writel(val, drv->reg_phy + EXYNOS5_DRD_PHYREG0);
+
+ do {
+ result = readl(drv->reg_phy + EXYNOS5_DRD_PHYREG1);
+ if (!(result & EXYNOS5_DRD_PHYREG1_CR_ACK))
+ break;
+
+ udelay(1);
+ } while (usec-- > 0);
+
+ if (!usec)
+ dev_err(drv->dev, "CRPORT handshake timeout2 (0x%08x)\n", val);
+}
+
+static void crport_ctrl_write(struct usb3phy_driver *drv, u32 addr, u32 data)
+{
+ /* Write Address */
+ crport_handshake(drv, EXYNOS5_DRD_PHYREG0_CR_DATA_IN(addr),
+ EXYNOS5_DRD_PHYREG0_CR_CAP_ADDR);
+
+ /* Write Data */
+ crport_handshake(drv, EXYNOS5_DRD_PHYREG0_CR_DATA_IN(data),
+ EXYNOS5_DRD_PHYREG0_CR_CAP_DATA);
+ crport_handshake(drv, EXYNOS5_DRD_PHYREG0_CR_DATA_IN(data),
+ EXYNOS5_DRD_PHYREG0_CR_WRITE);
+}
+
static int exynos5_usb3phy_init(struct phy *phy)
{
struct usb3phy_driver *drv = phy_get_drvdata(phy);
@@ -379,11 +451,44 @@ static int exynos5_usb3phy_power_off(struct phy *phy)
return 0;
}

+static int exynos5_usb3phy_tune(struct phy *phy)
+{
+ struct usb3phy_driver *drv = phy_get_drvdata(phy);
+
+ if (drv->cfg->need_crport_tuning) {
+ u32 temp;
+ /*
+ * Change los_bias to (0x5) for 28nm PHY from a
+ * default value (0x0); los_level is set as default
+ * (0x9) as also reflected in los_level[30:26] bits
+ * of PHYPARAM0 register.
+ */
+ temp = LOSLEVEL_OVRD_IN_LOS_BIAS_5420 |
+ LOSLEVEL_OVRD_IN_EN |
+ LOSLEVEL_OVRD_IN_LOS_LEVEL_DEFAULT;
+ crport_ctrl_write(drv,
+ EXYNOS5_DRD_PHYSS_LOSLEVEL_OVRD_IN,
+ temp);
+
+ /*
+ * Set tx_vboost_lvl to (0x5) for 28nm PHY Tuning,
+ * to raise Tx signal level from its default value of (0x4)
+ */
+ temp = TX_VBOOSTLEVEL_OVRD_IN_VBOOST_5420;
+ crport_ctrl_write(drv,
+ EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN,
+ temp);

+ }
+
+ return 0;

+}
+
static struct phy_ops exynos5_usb3phy_ops = {
.init = exynos5_usb3phy_init,
.exit = exynos5_usb3phy_exit,
.power_on = exynos5_usb3phy_power_on,
.power_off = exynos5_usb3phy_power_off,
+ .tune = exynos5_usb3phy_tune,
.owner = THIS_MODULE,
};

@@ -391,12 +496,14 @@ const struct usb3phy_config exynos5420_usb3phy_cfg = {
.has_usb30_sclk = true,
.reg_pmu_offset = EXYNOS5_USB3DRD_PHY_PMU_REG_OFFSET,
.has_multi_controller = true,
+ .need_crport_tuning = true,
};

const struct usb3phy_config exynos5250_usb3phy_cfg = {
.has_usb30_sclk = false,
.reg_pmu_offset = EXYNOS5_USB3DRD_PHY_PMU_REG_OFFSET,
.has_multi_controller = false,
+ .need_crport_tuning = false,
};

static const struct of_device_id exynos5_usb3phy_of_match[] = {

[Vivek Gautam]

The DWC3-exynos eXtensible host controller present on Exynos5420
SoC is quirky. The PHY serving this controller operates at High-Speed
by default, so it detects even Super-speed devices as high-speed ones.
This PHY needs to be tuned for its Tx LOS levels and Boost levels.

In this patch-set, we are registering a quirk for this DWC3-Exynos
controller with XHCI, so that once after xhci_reset(), the controller
can call to tune the associated PHY and achieve the SuperSpeed.

These patches are based on new USB 3.0 phy driver for Exynos SoC series. [1]

[1] Add Exynos5 USB 3.0 phy driver based on generic PHY framework
http://lwn.net/Articles/575586/

Vivek Gautam (4):
phy: Add provision for tuning phy.
xhci: Add quirk for DWC3-Exynos controller
xhci: Tune PHY for the DWC3-Exynos host controller
phy-exynos-usb3: Fine tune LOS levels for exynos5420

drivers/phy/phy-core.c | 20 +++++++
drivers/phy/phy-exynos5-usb3.c | 107 ++++++++++++++++++++++++++++++++++++++++
drivers/usb/dwc3/host.c | 7 +++
drivers/usb/host/xhci-plat.c | 62 ++++++++++++++++++++++-
drivers/usb/host/xhci.h | 1 +
include/linux/phy/phy.h | 7 +++
include/linux/usb/hcd.h | 1 +
7 files changed, 203 insertions(+), 2 deletions(-)

[Vivek Gautam]

The DWC3-exynos eXtensible host controller on Exynos5420 SoC
is quirky in a way that the PHY needs to be tuned to get it
working at SuperSpeed.

By default this PHY works as High-speed phy and therefore

detects even Super-speed devices as high-speed ones.

So, the PHY needs to be tuned after controller has been reset.

Adding a xHCI quirk for this purpose.

Signed-off-by: Vivek Gautam <gautam...@samsung.com>
---

drivers/usb/host/xhci-plat.c | 19 +++++++++++++++++++
drivers/usb/host/xhci.h | 1 +
2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index d9c169f..395c9e9 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -21,6 +21,25 @@

static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)

{
+ struct device *parent_dev;
+ struct device *vendor_parent_dev;
+
+ parent_dev = dev->parent;
+ vendor_parent_dev = parent_dev->parent;
+
+ /*
+ * The DWC3-exynos host controller on Exynos5420 SoC is quirky
+ * in a way that the PHY needs to be tuned to get it working
+ * at SuperSpeed. By default this PHY works as High-speed phy
+ * and so detects even Super-speed devices as high-speed ones.
+ * Therefor the PHY needs to be tuned after controller
+ * has been reset, since a controller reset actually forces the
+ * PHY to fall back to its default state wherein it works as
+ * High-Speed PHY only.
+ */
+ if (!strstr(dev_name(vendor_parent_dev), "exynos"))
+ xhci->quirks |= XHCI_DWC3_EXYNOS;
+
/*
* As of now platform drivers don't provide MSI support so we ensure
* here that the generic code does not try to make a pci_dev from our
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 7807f62..f69af8c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1560,6 +1560,7 @@ struct xhci_hcd {
#define XHCI_PLAT (1 << 16)
#define XHCI_SLOW_SUSPEND (1 << 17)
#define XHCI_SPURIOUS_WAKEUP (1 << 18)
+#define XHCI_DWC3_EXYNOS (1 << 19)
unsigned int num_active_eps;
unsigned int limit_active_eps;
/* There are two roothubs to keep track of bus suspend info for */

[Vivek Gautam]

Some PHY controllers may need to tune PHY post-initialization,
so that the PHY consumers can call phy-tuning at appropriate
point of time.

Signed-off-by: vivek Gautam <gautam...@samsung.com>
---
drivers/phy/phy-core.c | 20 ++++++++++++++++++++
include/linux/phy/phy.h | 7 +++++++
2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 03cf8fb..68dbb90 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -239,6 +239,26 @@ out:
}
EXPORT_SYMBOL_GPL(phy_power_off);

+int phy_tune(struct phy *phy)
+{
+ int ret = -ENOTSUPP;
+
+ mutex_lock(&phy->mutex);
+ if (phy->ops->tune) {
+ ret = phy->ops->tune(phy);
+ if (ret < 0) {
+ dev_err(&phy->dev, "phy tuning failed --> %d\n", ret);
+ goto out;
+ }
+ }
+
+out:
+ mutex_unlock(&phy->mutex);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(phy_tune);
+
/**
* of_phy_get() - lookup and obtain a reference to a phy by phandle
* @dev: device that requests this phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 6d72269..d3b32b0 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -34,6 +34,7 @@ struct phy_ops {
int (*exit)(struct phy *phy);
int (*power_on)(struct phy *phy);
int (*power_off)(struct phy *phy);
+ int (*tune)(struct phy *phy);
struct module *owner;
};

@@ -127,6 +128,7 @@ int phy_init(struct phy *phy);
int phy_exit(struct phy *phy);
int phy_power_on(struct phy *phy);
int phy_power_off(struct phy *phy);
+int phy_tune(struct phy *phy);
struct phy *phy_get(struct device *dev, const char *string);
struct phy *devm_phy_get(struct device *dev, const char *string);
void phy_put(struct phy *phy);
@@ -199,6 +201,11 @@ static inline int phy_power_off(struct phy *phy)
return -ENOSYS;
}

+static inline int phy_tune(struct phy *phy)
+{
+ return -ENOSYS;
+}
+
static inline struct phy *phy_get(struct device *dev, const char *string)
{
return ERR_PTR(-ENOSYS);

[Heikki Krogerus]

Hi,

On Tue, Dec 10, 2013 at 04:25:25PM +0530, Vivek Gautam wrote:
> @@ -170,6 +189,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
> }
>
> /*
> + * The parent of the xhci-plat device may pass in a PHY via
> + * platform data. If it exists, store it in our struct usb_hcd
> + * so that we can use it later.
> + */
> + phy_generic = dev_get_platdata(&pdev->dev);
> + if (phy_generic)
> + xhci->shared_hcd->phy_generic = *phy_generic;

Getting the handle to the phy from platform data like this is not
going to work for long. It should be possible to get it normally with
phy_get(). It's not going to be possible to get the handle from the
platform data like this if the xhci-hcd platform device is created
from ACPI or DT. You are also not considering case where you have two
phys.

Vivek, I have made a patch set for the phy framework allowing
associations between the phys and their users to be made in same way
gpios and clk make them. With those you should be able to create a
lookup entry to the phy framework in drivers/usb/dwc3/host.c. Then we
could use phy_get() here already. Please check them. Subject of the
thread:

"phy: remove the need for the phys to know about their users"

The lookup table can then be added in drivers/usb/dwc3/host.c with
something like this:

int dwc3_host_init(struct dwc3 *dwc)
{
struct platform_device *xhci;
struct phy_lookup_table *table;
...
table->dev_id = dev_name(&xhci->dev);
if (dwc->usb2_generic_phy)
table->table[0].phy_name = dev_name(&dwc->usb2_generic_phy->dev);
if (dwc->usb3_generic_phy)
table->table[1].phy_name = dev_name(&dwc->usb3_generic_phy->dev);
phy_add_lookup_table(table);
...

Br,

--
heikki

[Heikki Krogerus]

Hi,

I think "setup" instead of "tune" is much more clear and reusable.

Thanks,

--
heikki

[Julius Werner]

On Tue, Dec 10, 2013 at 2:55 AM, Vivek Gautam <gautam...@samsung.com> wrote:
> The DWC3-exynos eXtensible host controller on Exynos5420 SoC
> is quirky in a way that the PHY needs to be tuned to get it
> working at SuperSpeed.
> By default this PHY works as High-speed phy and therefore
> detects even Super-speed devices as high-speed ones.
> So, the PHY needs to be tuned after controller has been reset.
>
> Adding a xHCI quirk for this purpose.
>
> Signed-off-by: Vivek Gautam <gautam...@samsung.com>
> ---
> drivers/usb/host/xhci-plat.c | 19 +++++++++++++++++++
> drivers/usb/host/xhci.h | 1 +
> 2 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index d9c169f..395c9e9 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -21,6 +21,25 @@
>
> static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
> {
> + struct device *parent_dev;
> + struct device *vendor_parent_dev;
> +
> + parent_dev = dev->parent;
> + vendor_parent_dev = parent_dev->parent;

Is this guaranteed to exist for all current and future xhci-plat
controllers or should you maybe check one or both for NULL?

I think in general it's better to name quirks after what they do
rather than a specific device, that way you might be able to reuse
them. How about XHCI_TUNE_AFTER_RESET? (Or you could leave out the
quirk completely and just differentiate by whether the PHY implements
the tune() method or not.)

[Vivek Gautam]

Hi,


On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus
<heikki....@linux.intel.com> wrote:
> Hi,

Thanks for reviewing this.

I think "setup" will look more like first time setting up the phy,
which is rather served by "init" callback.
This i thought would serve the purpose of over-riding certain PHY
parameters, which would not have been
possible at "init" time.
Please correct my thinking if i am unable to understand your point here.

>
> Thanks,
>
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in

> the body of a message to majo...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

[Heikki Krogerus]

Hi,

On Wed, Dec 11, 2013 at 12:08:04PM +0530, Vivek Gautam wrote:
> On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus

> > I think "setup" instead of "tune" is much more clear and reusable.
>
> I think "setup" will look more like first time setting up the phy,
> which is rather served by "init" callback.
> This i thought would serve the purpose of over-riding certain PHY
> parameters, which would not have been
> possible at "init" time.
> Please correct my thinking if i am unable to understand your point here.

OK, sorry I was not clear on this. I'm thinking the same, that this is
something that is called later, for example when the controller is
ready. Some ULPI phys need to be initialized, but since the controller
provides the interface, it's usually not possible during init time.
This hook could be used in that case as well.

All I'm saying is that "tune" is a poor expression. You will need to
add a comment explaining what the hook does in any case, so you'll
have something like "this is something that is called when the
controller is ready" or something similar. That will make it clear
what it's meant for.

Thanks,

--
heikki
--

[Kishon Vijay Abraham I]

how about 'calibrate'?

Thanks
Kishon

>
>>
>> Thanks,
>>
>> --
>> heikki
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>> the body of a message to majo...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>

--

[Vivek Gautam]

Hi Kishon,

Hmm, seems like a better name :-)

>
> Thanks
> Kishon
>>
>>>
>>> Thanks,
>>>
>>> --
>>> heikki
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
>>> the body of a message to majo...@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
>>
>



--

Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

[Vivek Gautam]

Hi,

On Wed, Dec 11, 2013 at 1:39 PM, Heikki Krogerus
<heikki....@linux.intel.com> wrote:
> Hi,
>
> On Wed, Dec 11, 2013 at 12:08:04PM +0530, Vivek Gautam wrote:
>> On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus
>> > I think "setup" instead of "tune" is much more clear and reusable.
>>
>> I think "setup" will look more like first time setting up the phy,
>> which is rather served by "init" callback.
>> This i thought would serve the purpose of over-riding certain PHY
>> parameters, which would not have been
>> possible at "init" time.
>> Please correct my thinking if i am unable to understand your point here.
>
> OK, sorry I was not clear on this. I'm thinking the same, that this is
> something that is called later, for example when the controller is
> ready. Some ULPI phys need to be initialized, but since the controller
> provides the interface, it's usually not possible during init time.
> This hook could be used in that case as well.
>
> All I'm saying is that "tune" is a poor expression. You will need to
> add a comment explaining what the hook does in any case, so you'll
> have something like "this is something that is called when the
> controller is ready" or something similar. That will make it clear
> what it's meant for.

Ok, i think i should have kept a comment atleast :-(
I will add proper comments above, and as suggested in the mail by
Kishon, may be name it calibrate ?
What do you think ?

>
> Thanks,
>
> --
> heikki

--
Best Regards
Vivek Gautam
Samsung R&D Institute, Bangalore
India

[Heikki Krogerus]

Hi again,

Sure, I'm fine with that.

Thanks,

--
heikki

[Vivek Gautam]

Hi Heikki,


On Tue, Dec 10, 2013 at 7:25 PM, Heikki Krogerus
<heikki....@linux.intel.com> wrote:

Giving life to this thread after long time.

> Hi,
>
> On Tue, Dec 10, 2013 at 04:25:25PM +0530, Vivek Gautam wrote:
>> @@ -170,6 +189,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
>> }
>>
>> /*
>> + * The parent of the xhci-plat device may pass in a PHY via
>> + * platform data. If it exists, store it in our struct usb_hcd
>> + * so that we can use it later.
>> + */
>> + phy_generic = dev_get_platdata(&pdev->dev);
>> + if (phy_generic)
>> + xhci->shared_hcd->phy_generic = *phy_generic;
>
> Getting the handle to the phy from platform data like this is not
> going to work for long. It should be possible to get it normally with
> phy_get(). It's not going to be possible to get the handle from the
> platform data like this if the xhci-hcd platform device is created
> from ACPI or DT. You are also not considering case where you have two
> phys.
>
> Vivek, I have made a patch set for the phy framework allowing
> associations between the phys and their users to be made in same way
> gpios and clk make them. With those you should be able to create a
> lookup entry to the phy framework in drivers/usb/dwc3/host.c. Then we
> could use phy_get() here already. Please check them. Subject of the
> thread:
>
> "phy: remove the need for the phys to know about their users"

I had seen your patches in the mailing list, but i don't see any
updated version of these patches.
Are you planning to work on this above mentioned patch-series any time soon ?

Or, should i try to find a different approach for handling the phy
from the host controller (child of DWC3 in this case, which has the
phys).

>
> The lookup table can then be added in drivers/usb/dwc3/host.c with
> something like this:
>
> int dwc3_host_init(struct dwc3 *dwc)
> {
> struct platform_device *xhci;
> struct phy_lookup_table *table;
> ...
> table->dev_id = dev_name(&xhci->dev);
> if (dwc->usb2_generic_phy)
> table->table[0].phy_name = dev_name(&dwc->usb2_generic_phy->dev);
> if (dwc->usb3_generic_phy)
> table->table[1].phy_name = dev_name(&dwc->usb3_generic_phy->dev);
> phy_add_lookup_table(table);
> ...

[Heikki Krogerus]

Hi,

On Tue, Apr 15, 2014 at 06:24:11PM +0530, Vivek Gautam wrote:
> I had seen your patches in the mailing list, but i don't see any
> updated version of these patches.
> Are you planning to work on this above mentioned patch-series any time soon ?

I'm sorry, I forgot this completely. I have not prepared new version
of those patches as the drivers I need them for are not ready yet, but
I guess I can also use this case as justification for them.

> Or, should i try to find a different approach for handling the phy
> from the host controller (child of DWC3 in this case, which has the
> phys).

Well, there is now an issue with the lookup method I'm suggesting in
this case. Since the device ID is now generated automatically for
xhci-hcd in dwc3, we don't know the xhci-hcd device name before
platform_device_add(), and that is too late. I don't see why the
device could not be named when platform_device_alloc() is called, so I
think I'll suggest something like the attached patch to fix this
issue.

In any case, I'll send an updated version of the phy patches soon.

Thanks,

--
heikki

[Vivek Gautam]

Hi,

On Wed, Apr 16, 2014 at 7:42 PM, Heikki Krogerus
<heikki....@linux.intel.com> wrote:
> Hi,
>
> On Tue, Apr 15, 2014 at 06:24:11PM +0530, Vivek Gautam wrote:
>> I had seen your patches in the mailing list, but i don't see any
>> updated version of these patches.
>> Are you planning to work on this above mentioned patch-series any time soon ?
>
> I'm sorry, I forgot this completely. I have not prepared new version
> of those patches as the drivers I need them for are not ready yet, but
> I guess I can also use this case as justification for them.
>
>> Or, should i try to find a different approach for handling the phy
>> from the host controller (child of DWC3 in this case, which has the
>> phys).
>
> Well, there is now an issue with the lookup method I'm suggesting in
> this case. Since the device ID is now generated automatically for
> xhci-hcd in dwc3, we don't know the xhci-hcd device name before
> platform_device_add(), and that is too late.

True, the xhci-hcd are getting AUTO ID, so it might not be possible to
get their device
names in dwc3.

> I don't see why the
> device could not be named when platform_device_alloc() is called, so I
> think I'll suggest something like the attached patch to fix this
> issue

Ok, i had a look at the patch, and it looks promising.

>
> In any case, I'll send an updated version of the phy patches soon.

Thanks for your efforts.