[PATCH v6 0/4] Fine tune USB 3.0 PHY on exynos5420

[PATCH v6 0/4] Fine tune USB 3.0 PHY on exynos5420

[Vivek Gautam]

This series is based on Heikki's patches for simpliefied phy lookup table:
[PATCHv3 0/6] phy: simplified phy lookup [1], applied against 'usb-next' branch
alongwith Sergei's patch for adding generic phy support to usb hcd [2].

NOTE: We need to get above dependencies merged first in order to merge this
      series.
      As of now patches @ [1] are posted with all the comments addressed
      and we are waiting for an ACK on them, but
      patch @ [2] needs to be re-spined as Sergie says in [3]:
            "Yes, I'm planning this since Greg has lost v4."

Changes since v5:
 - Assigned NULL to hcd->gen_phy in error path in xhci-plat.c, so that
   we don't need to check for IS_ERR() while calibrating the PHYs in
   core/hcd.c
 - Removed extra empty lines in register definitions in exynos5-usbdrd
   phy driver.
 - Added write access for EXYNOS5_DRD_PHYREG0 register before any
   crport_handshake() call as suggested by Jingoo Han.
 - Renamed member 'calibrate' to 'phy_exynos_calibrate' of
   struct exynos5_usbdrd_phy_drvdata.

Changes since v4:
 - Rebased on latest patches by Heikki.
 - Took care of handling -EPROBE_DEFER error number while getting PHY in
   xhci plat.

Changes from v3:
 - Modified error message as per review comments from Julius.

Changes since v2:
 - Removed any check for DWC3 in xhci-plat for getting usb2-phy and usb3-phy,
   in order to make it more generic.
 - Moved the phy_calibration calls to core/hcd.c to enable a more generic
   solution for issues of calibrating the PHYs.

Changes since v1:
 - Using 'gen_phy' member of 'hcd' instead of declaring more variables
   to hold phys.
 - Added a check for compatible match for 'Synopsys-dwc3' controller,
   since the 'gen_phy' member of 'hcd' already gets the 'usb' PHY
   in core/hcd.c; but XHCI on Synopsys-dwc3 doesn't need that,
   instead two separate PHYs for UTMI+ and PIPE3 for the two HCDs
   (main hcd and shared hcd).
 - Restructured the code in 'xhci_plat_setup()' and 'xhci_plat_resume()'
   to use hcd->gen_phy directly. Also added the check for Synopsys's DWC3
   controller while trying to calibrate the PHY.

Explanation for the need of this patch-series:
"The DWC3-exynos eXtensible host controller present on Exynos5420/5800
SoCs is quirky. The PHY serving this controller operates at High-Speed
by default, so it detects even Super-speed devices as high-speed ones.
Certain PHY parameters like Tx LOS levels and Boost levels need to be
calibrated further post initialization of xHCI controller, to get
SuperSpeed operations working."

[1] https://www.mail-archive.com/linux-kernel at vger.kernel.org/msg710094.html
[2] http://comments.gmane.org/gmane.linux.ports.sh.devel/35932
[3] http://www.spinics.net/lists/linux-usb/msg112300.html

Vivek Gautam (4):
  phy: Add provision for calibrating phy.
  usb: host: xhci-plat: Get PHYs for xhci's hcds
  usb: hcd: Calibrate PHY post hcd reset
  phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

 drivers/phy/phy-core.c           |   36 ++++++++
 drivers/phy/phy-exynos5-usbdrd.c |  185 ++++++++++++++++++++++++++++++++++----
 drivers/usb/core/hcd.c           |   18 ++++
 drivers/usb/host/xhci-plat.c     |   27 ++++++
 include/linux/phy/phy.h          |    8 ++
 5 files changed, 259 insertions(+), 15 deletions(-)

-- 
1.7.10.4

[PATCH v6 1/4] phy: Add provision for calibrating phy.

[Vivek Gautam]

Some PHY controllers may need to calibrate certain
PHY settings after initialization of the controller and
sometimes even after initializing the PHY-consumer too.
Add support for the same in order to let consumers do so in need.

Signed-off-by: vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/phy/phy-core.c  |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/phy/phy.h |    8 ++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 834b337..c8cb3de 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -393,6 +393,42 @@ int phy_power_off(struct phy *phy)
 EXPORT_SYMBOL_GPL(phy_power_off);
 
 /**
+ * phy_calibrate - calibrate a phy post initialization
+ * @phy: Pointer to 'phy' from consumer
+ *
+ * For certain PHYs, it may be needed to calibrate few phy parameters
+ * post initialization. The need to calibrate may arise after the
+ * initialization of consumer itself, in order to prevent further any
+ * loss of phy settings post consumer-initialization.
+ *	example: USB 3.0 DRD PHY on Exynos5420/5800 systems is one such
+ *	phy which needs calibration after the host controller reset
+ *	has happened.
+ */
+int phy_calibrate(struct phy *phy)
+{
+	int ret = -ENOTSUPP;
+
+	if (!phy)
+		return 0;
+
+	mutex_lock(&phy->mutex);
+	if (phy->ops->calibrate) {
+		ret =  phy->ops->calibrate(phy);
+		if (ret < 0) {
+			dev_err(&phy->dev,
+				"phy calibration failed --> %d\n", ret);
+			goto out;
+		}
+	}
+
+out:
+	mutex_unlock(&phy->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phy_calibrate);
+
+/**
  * _of_phy_get() - lookup and obtain a reference to a phy by phandle
  * @np: device_node for which to get the phy
  * @index: the index of the phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index d983051..c70a311 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -28,6 +28,7 @@ struct phy;
  * @exit: operation to be performed while exiting
  * @power_on: powering on the phy
  * @power_off: powering off the phy
+ * @calibrate: calibrate the phy post init
  * @owner: the module owner containing the ops
  */
 struct phy_ops {
@@ -35,6 +36,7 @@ struct phy_ops {
 	int	(*exit)(struct phy *phy);
 	int	(*power_on)(struct phy *phy);
 	int	(*power_off)(struct phy *phy);
+	int	(*calibrate)(struct phy *phy);
 	struct module *owner;
 };
 
@@ -126,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_calibrate(struct phy *phy);
 static inline int phy_get_bus_width(struct phy *phy)
 {
 	return phy->attrs.bus_width;
@@ -231,6 +234,11 @@ static inline int phy_power_off(struct phy *phy)
 	return -ENOSYS;
 }
 
+static inline int phy_calibrate(struct phy *phy)
+{
+	return -ENOSYS;
+}
+
 static inline int phy_get_bus_width(struct phy *phy)
 {
 	return -ENOSYS;
-- 
1.7.10.4

[PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds

[Vivek Gautam]

The host controller by itself may sometimes need to handle PHY
and/or calibrate some of the PHY settings to get full support out
of the PHY controller. The PHY core provides a calibration
funtionality now to do so.
Therefore, facilitate getting the two possible PHYs, viz.
USB 2.0 type (UTMI+) and USB 3.0 type (PIPE3).

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/host/xhci-plat.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 1a0cf9f..6afd508 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/phy/phy.h>
 #include <linux/slab.h>
 #include <linux/usb/xhci_pdriver.h>
 
@@ -180,6 +181,19 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			goto put_hcd;
 	}
 
+	/* Get possile USB 2.0 type PHY (UTMI+) available with xhci */
+	hcd->gen_phy = devm_phy_get(&pdev->dev, "usb2-phy");
+	if (IS_ERR(hcd->gen_phy)) {
+		ret = PTR_ERR(hcd->gen_phy);
+		if (ret == -EPROBE_DEFER) {
+			return ret;
+		} else if (ret != -ENOSYS && ret != -ENODEV) {
+			hcd->gen_phy = NULL;
+			dev_warn(&pdev->dev,
+				 "Error retrieving usb2 phy: %d\n", ret);
+		}
+	}
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto disable_clk;
@@ -209,6 +223,19 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
 
+	/* Get possile USB 3.0 type PHY (PIPE3) available with xhci */
+	xhci->shared_hcd->gen_phy = devm_phy_get(&pdev->dev, "usb3-phy");
+	if (IS_ERR(xhci->shared_hcd->gen_phy)) {
+		ret = PTR_ERR(xhci->shared_hcd->gen_phy);
+		if (ret == -EPROBE_DEFER) {
+			return ret;
+		} else if (ret != -ENOSYS && ret != -ENODEV) {
+			xhci->shared_hcd->gen_phy = NULL;
+			dev_warn(&pdev->dev,
+				 "Error retrieving usb3 phy: %d\n", ret);
+		}
+	}
+
 	ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto put_usb3_hcd;
-- 
1.7.10.4

[PATCH v6 3/4] usb: hcd: Calibrate PHY post hcd reset

[Vivek Gautam]

Some quirky PHYs may require to be calibrated post the
hcd initialization.
The USB 3.0 DRD PHY on Exynos5420/5800 systems, coming along
with Synopsys's DWC3 controller, is one such PHY which needs
to be calibrated post xhci's reset at initialization time and
at resume time, to get the controller work at SuperSpeed.
So facilitating the HCDs to calibrate the PHY.

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/usb/core/hcd.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index c4ed66c..6e01d09 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2202,6 +2202,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	struct usb_hcd	*hcd = container_of(rhdev->bus, struct usb_hcd, self);
 	int		status;
 	int		old_state = hcd->state;
+	int		ret;
 
 	dev_dbg(&rhdev->dev, "usb %sresume\n",
 			(PMSG_IS_AUTO(msg) ? "auto-" : ""));
@@ -2216,6 +2217,15 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 
 	hcd->state = HC_STATE_RESUMING;
 	status = hcd->driver->bus_resume(hcd);
+
+	/* calibrate the phy here */
+	ret = phy_calibrate(hcd->gen_phy);
+	if (ret < 0 && ret != -ENOTSUPP) {
+		dev_err(hcd->self.controller,
+			"failed to calibrate USB PHY\n");
+		return ret;
+	}
+
 	clear_bit(HCD_FLAG_WAKEUP_PENDING, &hcd->flags);
 	if (status == 0) {
 		struct usb_device *udev;
@@ -2738,6 +2748,14 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	}
 	hcd->rh_pollable = 1;
 
+	/* calibrate the phy here */
+	retval = phy_calibrate(hcd->gen_phy);
+	if (retval < 0 && retval != -ENOTSUPP) {
+		dev_err(hcd->self.controller,
+			"failed to calibrate USB PHY\n");
+		return retval;
+	}
+
 	/* NOTE: root hub and controller capabilities may not be the same */
 	if (device_can_wakeup(hcd->self.controller)
 			&& device_can_wakeup(&hcd->self.root_hub->dev))
-- 
1.7.10.4

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

Adding phy calibrate callback, which facilitates setting certain
PHY settings post initialization of the PHY controller.
Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
the Loss-of-Signal (LOS) Detector Threshold Level as well as
Tx-Vboost-Level should be controlled for Super-Speed operations.

Additionally set proper time to wait for RxDetect measurement,
for desired PHY reference clock, so as to solve issue with enumeration
of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
on the controller.
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, and peach-pit boards 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 1-1: new high-speed USB device number 2 using xhci-hcd

[Also removed unnecessary extra lines in the register macro definitions]

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 drivers/phy/phy-exynos5-usbdrd.c |  185 ++++++++++++++++++++++++++++++++++----
 1 file changed, 170 insertions(+), 15 deletions(-)

diff --git a/drivers/phy/phy-exynos5-usbdrd.c b/drivers/phy/phy-exynos5-usbdrd.c
index 47f47fe..85742b0 100644
--- a/drivers/phy/phy-exynos5-usbdrd.c
+++ b/drivers/phy/phy-exynos5-usbdrd.c
@@ -37,13 +37,11 @@
 
 /* EXYNOS5: USB 3.0 DRD PHY registers */
 #define EXYNOS5_DRD_LINKSYSTEM			0x04
-
 #define LINKSYSTEM_FLADJ_MASK			(0x3f << 1)
 #define LINKSYSTEM_FLADJ(_x)			((_x) << 1)
 #define LINKSYSTEM_XHCI_VERSION_CONTROL		BIT(27)
 
 #define EXYNOS5_DRD_PHYUTMI			0x08
-
 #define PHYUTMI_OTGDISABLE			BIT(6)
 #define PHYUTMI_FORCESUSPEND			BIT(1)
 #define PHYUTMI_FORCESLEEP			BIT(0)
@@ -51,26 +49,20 @@
 #define EXYNOS5_DRD_PHYPIPE			0x0c
 
 #define EXYNOS5_DRD_PHYCLKRST			0x10
-
 #define PHYCLKRST_EN_UTMISUSPEND		BIT(31)
-
 #define PHYCLKRST_SSC_REFCLKSEL_MASK		(0xff << 23)
 #define PHYCLKRST_SSC_REFCLKSEL(_x)		((_x) << 23)
-
 #define PHYCLKRST_SSC_RANGE_MASK		(0x03 << 21)
 #define PHYCLKRST_SSC_RANGE(_x)			((_x) << 21)
-
 #define PHYCLKRST_SSC_EN			BIT(20)
 #define PHYCLKRST_REF_SSP_EN			BIT(19)
 #define PHYCLKRST_REF_CLKDIV2			BIT(18)
-
 #define PHYCLKRST_MPLL_MULTIPLIER_MASK		(0x7f << 11)
 #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF	(0x19 << 11)
 #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF	(0x32 << 11)
 #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF	(0x68 << 11)
 #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF	(0x7d << 11)
 #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF	(0x02 << 11)
-
 #define PHYCLKRST_FSEL_UTMI_MASK		(0x7 << 5)
 #define PHYCLKRST_FSEL_PIPE_MASK		(0x7 << 8)
 #define PHYCLKRST_FSEL(_x)			((_x) << 5)
@@ -78,46 +70,68 @@
 #define PHYCLKRST_FSEL_PAD_24MHZ		(0x2a << 5)
 #define PHYCLKRST_FSEL_PAD_20MHZ		(0x31 << 5)
 #define PHYCLKRST_FSEL_PAD_19_2MHZ		(0x38 << 5)
-
 #define PHYCLKRST_RETENABLEN			BIT(4)
-
 #define PHYCLKRST_REFCLKSEL_MASK		(0x03 << 2)
 #define PHYCLKRST_REFCLKSEL_PAD_REFCLK		(0x2 << 2)
 #define PHYCLKRST_REFCLKSEL_EXT_REFCLK		(0x3 << 2)
-
 #define PHYCLKRST_PORTRESET			BIT(1)
 #define PHYCLKRST_COMMONONN			BIT(0)
 
 #define EXYNOS5_DRD_PHYREG0			0x14
+#define PHYREG0_SSC_REF_CLK_SEL			BIT(21)
+#define PHYREG0_SSC_RANGE			BIT(20)
+#define PHYREG0_CR_WRITE			BIT(19)
+#define PHYREG0_CR_READ				BIT(18)
+#define PHYREG0_CR_DATA_IN(_x)			((_x) << 2)
+#define PHYREG0_CR_CAP_DATA			BIT(1)
+#define PHYREG0_CR_CAP_ADDR			BIT(0)
+
 #define EXYNOS5_DRD_PHYREG1			0x18
+#define PHYREG1_CR_DATA_OUT(_x)			((_x) << 1)
+#define PHYREG1_CR_ACK				BIT(0)
 
 #define EXYNOS5_DRD_PHYPARAM0			0x1c
-
 #define PHYPARAM0_REF_USE_PAD			BIT(31)
 #define PHYPARAM0_REF_LOSLEVEL_MASK		(0x1f << 26)
 #define PHYPARAM0_REF_LOSLEVEL			(0x9 << 26)
 
 #define EXYNOS5_DRD_PHYPARAM1			0x20
-
 #define PHYPARAM1_PCS_TXDEEMPH_MASK		(0x1f << 0)
 #define PHYPARAM1_PCS_TXDEEMPH			(0x1c)
 
 #define EXYNOS5_DRD_PHYTERM			0x24
 
 #define EXYNOS5_DRD_PHYTEST			0x28
-
 #define PHYTEST_POWERDOWN_SSP			BIT(3)
 #define PHYTEST_POWERDOWN_HSP			BIT(2)
 
 #define EXYNOS5_DRD_PHYADP			0x2c
 
 #define EXYNOS5_DRD_PHYUTMICLKSEL		0x30
-
 #define PHYUTMICLKSEL_UTMI_CLKSEL		BIT(2)
 
 #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)
+
+#define EXYNOS5_DRD_PHYSS_LANE0_TX_DEBUG		(0x1010)
+#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M		(0x4 << 4)
+#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_24M		(0x8 << 4)
+#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_25M_26M		(0x8 << 4)
+#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M	(0x20 << 4)
+#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_62M5		(0x20 << 4)
+#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_96M_100M		(0x40 << 4)
+
 #define KHZ	1000
 #define MHZ	(KHZ * KHZ)
 
@@ -135,12 +149,14 @@ struct exynos5_usbdrd_phy_config {
 	void (*phy_isol)(struct phy_usb_instance *inst, u32 on);
 	void (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
 	unsigned int (*set_refclk)(struct phy_usb_instance *inst);
+	int (*phy_calibrate)(struct phy_usb_instance *inst);
 };
 
 struct exynos5_usbdrd_phy_drvdata {
 	const struct exynos5_usbdrd_phy_config *phy_cfg;
 	u32 pmu_offset_usbdrd0_phy;
 	u32 pmu_offset_usbdrd1_phy;
+	void (*phy_exynos_calibrate)(struct exynos5_usbdrd_phy *phy_drd);
 };
 
 /**
@@ -487,6 +503,142 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
 	return 0;
 }
 
+static void crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
+						u32 val, u32 cmd)
+{
+	u32 usec = 100;
+	u32 result;
+
+	writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
+
+	do {
+		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
+		if (result & PHYREG1_CR_ACK)
+			break;
+
+		udelay(1);
+	} while (usec-- > 0);
+
+	if (!usec)
+		dev_err(phy_drd->dev,
+			"CRPORT handshake timeout1 (0x%08x)\n", val);
+
+	usec = 100;
+
+	writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
+
+	do {
+		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
+		if (!(result & PHYREG1_CR_ACK))
+			break;
+
+		udelay(1);
+	} while (usec-- > 0);
+
+	if (!usec)
+		dev_err(phy_drd->dev,
+			"CRPORT handshake timeout2 (0x%08x)\n", val);
+}
+
+static void crport_ctrl_write(struct exynos5_usbdrd_phy *phy_drd,
+						u32 addr, u32 data)
+{
+	/* Write Address */
+	writel(PHYREG0_CR_DATA_IN(addr),
+			phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
+	crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(addr),
+			 PHYREG0_CR_CAP_ADDR);
+
+	/* Write Data */
+	writel(PHYREG0_CR_DATA_IN(data),
+			phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
+	crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
+			 PHYREG0_CR_CAP_DATA);
+	crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
+			 PHYREG0_CR_WRITE);
+}
+
+/*
+ * Override PHY paramaeters using CR_PORT register to calibrate settings
+ * to meet meet SuperSpeed requirements, on Exynos5420 and Exynos5800 systems,
+ * which have 28nm USB 3.0 DRD PHY.
+ */
+static void exynos5420_usbdrd_phy_calibrate(struct exynos5_usbdrd_phy *phy_drd)
+{
+	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(phy_drd,
+			  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(phy_drd,
+			  EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN,
+			  temp);
+
+	/*
+	 * Set proper time to wait for RxDetect measurement, for
+	 * desired reference clock of PHY, by tuning the CRPORT
+	 * register LANE0.TX_DEBUG which is internal to PHY.
+	 * This fixes issue with few USB 3.0 devices, which are
+	 * not detected (not even generate interrupts on the bus
+	 * on insertion) without this change.
+	 * e.g. Samsung SUM-TSB16S 3.0 USB drive.
+	 */
+	switch (phy_drd->extrefclk) {
+	case EXYNOS5_FSEL_50MHZ:
+		temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M;
+		break;
+	case EXYNOS5_FSEL_20MHZ:
+	case EXYNOS5_FSEL_19MHZ2:
+		temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M;
+		break;
+	case EXYNOS5_FSEL_24MHZ:
+	default:
+		temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_24M;
+		break;
+	}
+
+	crport_ctrl_write(phy_drd,
+			  EXYNOS5_DRD_PHYSS_LANE0_TX_DEBUG,
+			  temp);
+}
+
+/* Calibrate PIPE3 PHY settings, if any */
+static int exynos5_usbdrd_pipe3_calibrate(struct phy_usb_instance *inst)
+{
+	struct exynos5_usbdrd_phy *phy_drd = to_usbdrd_phy(inst);
+
+	/* Call respective phy_calibrate given by certain platform */
+	if (phy_drd->drv_data->phy_exynos_calibrate)
+		phy_drd->drv_data->phy_exynos_calibrate(phy_drd);
+
+	return 0;
+}
+
+static int exynos5_usbdrd_phy_calibrate(struct phy *phy)
+{
+	struct phy_usb_instance *inst = phy_get_drvdata(phy);
+
+	if (inst->phy_cfg->phy_calibrate)
+		inst->phy_cfg->phy_calibrate(inst);
+
+	return 0;
+}
+
 static struct phy *exynos5_usbdrd_phy_xlate(struct device *dev,
 					struct of_phandle_args *args)
 {
@@ -503,6 +655,7 @@ static struct phy_ops exynos5_usbdrd_phy_ops = {
 	.exit		= exynos5_usbdrd_phy_exit,
 	.power_on	= exynos5_usbdrd_phy_power_on,
 	.power_off	= exynos5_usbdrd_phy_power_off,
+	.calibrate	= exynos5_usbdrd_phy_calibrate,
 	.owner		= THIS_MODULE,
 };
 
@@ -518,6 +671,7 @@ static const struct exynos5_usbdrd_phy_config phy_cfg_exynos5[] = {
 		.phy_isol	= exynos5_usbdrd_phy_isol,
 		.phy_init	= exynos5_usbdrd_pipe3_init,
 		.set_refclk	= exynos5_usbdrd_pipe3_set_refclk,
+		.phy_calibrate	= exynos5_usbdrd_pipe3_calibrate,
 	},
 };
 
@@ -525,6 +679,7 @@ static const struct exynos5_usbdrd_phy_drvdata exynos5420_usbdrd_phy = {
 	.phy_cfg		= phy_cfg_exynos5,
 	.pmu_offset_usbdrd0_phy	= EXYNOS5_USBDRD_PHY_CONTROL,
 	.pmu_offset_usbdrd1_phy	= EXYNOS5420_USBDRD1_PHY_CONTROL,
+	.phy_exynos_calibrate	= exynos5420_usbdrd_phy_calibrate,
 };
 
 static const struct exynos5_usbdrd_phy_drvdata exynos5250_usbdrd_phy = {
-- 
1.7.10.4

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Tue, Sep 02, 2014 at 04:42:18PM +0530, Vivek Gautam wrote:
> Adding phy calibrate callback, which facilitates setting certain
> PHY settings post initialization of the PHY controller.
> Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
> the Loss-of-Signal (LOS) Detector Threshold Level as well as
> Tx-Vboost-Level should be controlled for Super-Speed operations.
> 
> Additionally set proper time to wait for RxDetect measurement,
> for desired PHY reference clock, so as to solve issue with enumeration
> of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
> on the controller.
> 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, and peach-pit boards 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 1-1: new high-speed USB device number 2 using xhci-hcd
> 
> [Also removed unnecessary extra lines in the register macro definitions]
> 
> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> ---
>  drivers/phy/phy-exynos5-usbdrd.c |  185 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 170 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/phy/phy-exynos5-usbdrd.c b/drivers/phy/phy-exynos5-usbdrd.c
> index 47f47fe..85742b0 100644
> --- a/drivers/phy/phy-exynos5-usbdrd.c
> +++ b/drivers/phy/phy-exynos5-usbdrd.c
> @@ -37,13 +37,11 @@
>  
>  /* EXYNOS5: USB 3.0 DRD PHY registers */
>  #define EXYNOS5_DRD_LINKSYSTEM			0x04
> -
>  #define LINKSYSTEM_FLADJ_MASK			(0x3f << 1)
>  #define LINKSYSTEM_FLADJ(_x)			((_x) << 1)
>  #define LINKSYSTEM_XHCI_VERSION_CONTROL		BIT(27)
>  
>  #define EXYNOS5_DRD_PHYUTMI			0x08
> -
>  #define PHYUTMI_OTGDISABLE			BIT(6)
>  #define PHYUTMI_FORCESUSPEND			BIT(1)
>  #define PHYUTMI_FORCESLEEP			BIT(0)
> @@ -51,26 +49,20 @@
>  #define EXYNOS5_DRD_PHYPIPE			0x0c
>  
>  #define EXYNOS5_DRD_PHYCLKRST			0x10
> -
>  #define PHYCLKRST_EN_UTMISUSPEND		BIT(31)
> -
>  #define PHYCLKRST_SSC_REFCLKSEL_MASK		(0xff << 23)
>  #define PHYCLKRST_SSC_REFCLKSEL(_x)		((_x) << 23)
> -
>  #define PHYCLKRST_SSC_RANGE_MASK		(0x03 << 21)
>  #define PHYCLKRST_SSC_RANGE(_x)			((_x) << 21)
> -
>  #define PHYCLKRST_SSC_EN			BIT(20)
>  #define PHYCLKRST_REF_SSP_EN			BIT(19)
>  #define PHYCLKRST_REF_CLKDIV2			BIT(18)
> -
>  #define PHYCLKRST_MPLL_MULTIPLIER_MASK		(0x7f << 11)
>  #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF	(0x19 << 11)
>  #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF	(0x32 << 11)
>  #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF	(0x68 << 11)
>  #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF	(0x7d << 11)
>  #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF	(0x02 << 11)
> -
>  #define PHYCLKRST_FSEL_UTMI_MASK		(0x7 << 5)
>  #define PHYCLKRST_FSEL_PIPE_MASK		(0x7 << 8)
>  #define PHYCLKRST_FSEL(_x)			((_x) << 5)
> @@ -78,46 +70,68 @@
>  #define PHYCLKRST_FSEL_PAD_24MHZ		(0x2a << 5)
>  #define PHYCLKRST_FSEL_PAD_20MHZ		(0x31 << 5)
>  #define PHYCLKRST_FSEL_PAD_19_2MHZ		(0x38 << 5)
> -
>  #define PHYCLKRST_RETENABLEN			BIT(4)
> -
>  #define PHYCLKRST_REFCLKSEL_MASK		(0x03 << 2)
>  #define PHYCLKRST_REFCLKSEL_PAD_REFCLK		(0x2 << 2)
>  #define PHYCLKRST_REFCLKSEL_EXT_REFCLK		(0x3 << 2)
> -
>  #define PHYCLKRST_PORTRESET			BIT(1)
>  #define PHYCLKRST_COMMONONN			BIT(0)
>  
>  #define EXYNOS5_DRD_PHYREG0			0x14
> +#define PHYREG0_SSC_REF_CLK_SEL			BIT(21)
> +#define PHYREG0_SSC_RANGE			BIT(20)
> +#define PHYREG0_CR_WRITE			BIT(19)
> +#define PHYREG0_CR_READ				BIT(18)
> +#define PHYREG0_CR_DATA_IN(_x)			((_x) << 2)
> +#define PHYREG0_CR_CAP_DATA			BIT(1)
> +#define PHYREG0_CR_CAP_ADDR			BIT(0)
> +
>  #define EXYNOS5_DRD_PHYREG1			0x18
> +#define PHYREG1_CR_DATA_OUT(_x)			((_x) << 1)
> +#define PHYREG1_CR_ACK				BIT(0)
>  
>  #define EXYNOS5_DRD_PHYPARAM0			0x1c
> -
>  #define PHYPARAM0_REF_USE_PAD			BIT(31)
>  #define PHYPARAM0_REF_LOSLEVEL_MASK		(0x1f << 26)
>  #define PHYPARAM0_REF_LOSLEVEL			(0x9 << 26)
>  
>  #define EXYNOS5_DRD_PHYPARAM1			0x20
> -
>  #define PHYPARAM1_PCS_TXDEEMPH_MASK		(0x1f << 0)
>  #define PHYPARAM1_PCS_TXDEEMPH			(0x1c)
>  
>  #define EXYNOS5_DRD_PHYTERM			0x24
>  
>  #define EXYNOS5_DRD_PHYTEST			0x28
> -
>  #define PHYTEST_POWERDOWN_SSP			BIT(3)
>  #define PHYTEST_POWERDOWN_HSP			BIT(2)
>  
>  #define EXYNOS5_DRD_PHYADP			0x2c
>  
>  #define EXYNOS5_DRD_PHYUTMICLKSEL		0x30
> -

this and all above are unrelated to $subject.

>  #define PHYUTMICLKSEL_UTMI_CLKSEL		BIT(2)
>  
>  #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)
> +
> +#define EXYNOS5_DRD_PHYSS_LANE0_TX_DEBUG		(0x1010)
> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M		(0x4 << 4)
> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_24M		(0x8 << 4)
> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_25M_26M		(0x8 << 4)
> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M	(0x20 << 4)
> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_62M5		(0x20 << 4)
> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_96M_100M		(0x40 << 4)
> +
>  #define KHZ	1000
>  #define MHZ	(KHZ * KHZ)
>  
> @@ -135,12 +149,14 @@ struct exynos5_usbdrd_phy_config {
>  	void (*phy_isol)(struct phy_usb_instance *inst, u32 on);
>  	void (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
>  	unsigned int (*set_refclk)(struct phy_usb_instance *inst);
> +	int (*phy_calibrate)(struct phy_usb_instance *inst);
>  };
>  
>  struct exynos5_usbdrd_phy_drvdata {
>  	const struct exynos5_usbdrd_phy_config *phy_cfg;
>  	u32 pmu_offset_usbdrd0_phy;
>  	u32 pmu_offset_usbdrd1_phy;
> +	void (*phy_exynos_calibrate)(struct exynos5_usbdrd_phy *phy_drd);
>  };
>  
>  /**
> @@ -487,6 +503,142 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
>  	return 0;
>  }
>  
> +static void crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
> +						u32 val, u32 cmd)
> +{
> +	u32 usec = 100;
> +	u32 result;
> +
> +	writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> +
> +	do {
> +		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> +		if (result & PHYREG1_CR_ACK)
> +			break;
> +
> +		udelay(1);
> +	} while (usec-- > 0);
> +
> +	if (!usec)
> +		dev_err(phy_drd->dev,
> +			"CRPORT handshake timeout1 (0x%08x)\n", val);

if handshake timed out, shouldn't you error out here ? IOW, make this
function return an int and return an error code from this point ? Why
would you even continue to the next step ?

> +
> +	usec = 100;
> +
> +	writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> +
> +	do {
> +		result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> +		if (!(result & PHYREG1_CR_ACK))
> +			break;
> +
> +		udelay(1);
> +	} while (usec-- > 0);
> +
> +	if (!usec)
> +		dev_err(phy_drd->dev,
> +			"CRPORT handshake timeout2 (0x%08x)\n", val);

return an error ?

> +}
> +
> +static void crport_ctrl_write(struct exynos5_usbdrd_phy *phy_drd,
> +						u32 addr, u32 data)
> +{
> +	/* Write Address */
> +	writel(PHYREG0_CR_DATA_IN(addr),
> +			phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> +	crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(addr),
> +			 PHYREG0_CR_CAP_ADDR);
> +
> +	/* Write Data */
> +	writel(PHYREG0_CR_DATA_IN(data),
> +			phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> +	crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
> +			 PHYREG0_CR_CAP_DATA);
> +	crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
> +			 PHYREG0_CR_WRITE);
> +}
> +
> +/*
> + * Override PHY paramaeters using CR_PORT register to calibrate settings
> + * to meet meet SuperSpeed requirements, on Exynos5420 and Exynos5800 systems,
> + * which have 28nm USB 3.0 DRD PHY.
> + */
> +static void exynos5420_usbdrd_phy_calibrate(struct exynos5_usbdrd_phy *phy_drd)
> +{
> +	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(phy_drd,
> +			  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(phy_drd,
> +			  EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN,
> +			  temp);
> +
> +	/*
> +	 * Set proper time to wait for RxDetect measurement, for
> +	 * desired reference clock of PHY, by tuning the CRPORT
> +	 * register LANE0.TX_DEBUG which is internal to PHY.
> +	 * This fixes issue with few USB 3.0 devices, which are
> +	 * not detected (not even generate interrupts on the bus
> +	 * on insertion) without this change.
> +	 * e.g. Samsung SUM-TSB16S 3.0 USB drive.
> +	 */
> +	switch (phy_drd->extrefclk) {
> +	case EXYNOS5_FSEL_50MHZ:
> +		temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M;
> +		break;
> +	case EXYNOS5_FSEL_20MHZ:
> +	case EXYNOS5_FSEL_19MHZ2:
> +		temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M;
> +		break;
> +	case EXYNOS5_FSEL_24MHZ:

why can't this be done during probe() ? Are you going to change external
reference clock frequency during runtime ? And even if you do,
wouldn't it be easier to subscribe to clock rate change notifiers
instead of adding this ->calibrate() ?

It still looks, to me, that this ->calibrate() thing is unnecessary. We
have a single user for it and this user could very well get this sorted
out during probe() or by subscribing to clock rate notifiers.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140902/f0df3ec5/attachment.sig>

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

Hi Felipe,


On Tue, Sep 2, 2014 at 8:04 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Tue, Sep 02, 2014 at 04:42:18PM +0530, Vivek Gautam wrote:
>> Adding phy calibrate callback, which facilitates setting certain
>> PHY settings post initialization of the PHY controller.
>> Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
>> the Loss-of-Signal (LOS) Detector Threshold Level as well as
>> Tx-Vboost-Level should be controlled for Super-Speed operations.
>>
>> Additionally set proper time to wait for RxDetect measurement,
>> for desired PHY reference clock, so as to solve issue with enumeration
>> of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
>> on the controller.
>> 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, and peach-pit boards 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 1-1: new high-speed USB device number 2 using xhci-hcd
>>
>> [Also removed unnecessary extra lines in the register macro definitions]
>>
>> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> ---
>>  drivers/phy/phy-exynos5-usbdrd.c |  185 ++++++++++++++++++++++++++++++++++----
>>  1 file changed, 170 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/phy/phy-exynos5-usbdrd.c b/drivers/phy/phy-exynos5-usbdrd.c
>> index 47f47fe..85742b0 100644
>> --- a/drivers/phy/phy-exynos5-usbdrd.c
>> +++ b/drivers/phy/phy-exynos5-usbdrd.c
>> @@ -37,13 +37,11 @@
>>
>>  /* EXYNOS5: USB 3.0 DRD PHY registers */
>>  #define EXYNOS5_DRD_LINKSYSTEM                       0x04
>> -
>>  #define LINKSYSTEM_FLADJ_MASK                        (0x3f << 1)
>>  #define LINKSYSTEM_FLADJ(_x)                 ((_x) << 1)
>>  #define LINKSYSTEM_XHCI_VERSION_CONTROL              BIT(27)
>>
>>  #define EXYNOS5_DRD_PHYUTMI                  0x08
>> -
>>  #define PHYUTMI_OTGDISABLE                   BIT(6)
>>  #define PHYUTMI_FORCESUSPEND                 BIT(1)
>>  #define PHYUTMI_FORCESLEEP                   BIT(0)
>> @@ -51,26 +49,20 @@
>>  #define EXYNOS5_DRD_PHYPIPE                  0x0c
>>
>>  #define EXYNOS5_DRD_PHYCLKRST                        0x10
>> -
>>  #define PHYCLKRST_EN_UTMISUSPEND             BIT(31)
>> -
>>  #define PHYCLKRST_SSC_REFCLKSEL_MASK         (0xff << 23)
>>  #define PHYCLKRST_SSC_REFCLKSEL(_x)          ((_x) << 23)
>> -
>>  #define PHYCLKRST_SSC_RANGE_MASK             (0x03 << 21)
>>  #define PHYCLKRST_SSC_RANGE(_x)                      ((_x) << 21)
>> -
>>  #define PHYCLKRST_SSC_EN                     BIT(20)
>>  #define PHYCLKRST_REF_SSP_EN                 BIT(19)
>>  #define PHYCLKRST_REF_CLKDIV2                        BIT(18)
>> -
>>  #define PHYCLKRST_MPLL_MULTIPLIER_MASK               (0x7f << 11)
>>  #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19 << 11)
>>  #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF    (0x32 << 11)
>>  #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF  (0x68 << 11)
>>  #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF  (0x7d << 11)
>>  #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF       (0x02 << 11)
>> -
>>  #define PHYCLKRST_FSEL_UTMI_MASK             (0x7 << 5)
>>  #define PHYCLKRST_FSEL_PIPE_MASK             (0x7 << 8)
>>  #define PHYCLKRST_FSEL(_x)                   ((_x) << 5)
>> @@ -78,46 +70,68 @@
>>  #define PHYCLKRST_FSEL_PAD_24MHZ             (0x2a << 5)
>>  #define PHYCLKRST_FSEL_PAD_20MHZ             (0x31 << 5)
>>  #define PHYCLKRST_FSEL_PAD_19_2MHZ           (0x38 << 5)
>> -
>>  #define PHYCLKRST_RETENABLEN                 BIT(4)
>> -
>>  #define PHYCLKRST_REFCLKSEL_MASK             (0x03 << 2)
>>  #define PHYCLKRST_REFCLKSEL_PAD_REFCLK               (0x2 << 2)
>>  #define PHYCLKRST_REFCLKSEL_EXT_REFCLK               (0x3 << 2)
>> -
>>  #define PHYCLKRST_PORTRESET                  BIT(1)
>>  #define PHYCLKRST_COMMONONN                  BIT(0)
>>
>>  #define EXYNOS5_DRD_PHYREG0                  0x14
>> +#define PHYREG0_SSC_REF_CLK_SEL                      BIT(21)
>> +#define PHYREG0_SSC_RANGE                    BIT(20)
>> +#define PHYREG0_CR_WRITE                     BIT(19)
>> +#define PHYREG0_CR_READ                              BIT(18)
>> +#define PHYREG0_CR_DATA_IN(_x)                       ((_x) << 2)
>> +#define PHYREG0_CR_CAP_DATA                  BIT(1)
>> +#define PHYREG0_CR_CAP_ADDR                  BIT(0)
>> +
>>  #define EXYNOS5_DRD_PHYREG1                  0x18
>> +#define PHYREG1_CR_DATA_OUT(_x)                      ((_x) << 1)
>> +#define PHYREG1_CR_ACK                               BIT(0)
>>
>>  #define EXYNOS5_DRD_PHYPARAM0                        0x1c
>> -
>>  #define PHYPARAM0_REF_USE_PAD                        BIT(31)
>>  #define PHYPARAM0_REF_LOSLEVEL_MASK          (0x1f << 26)
>>  #define PHYPARAM0_REF_LOSLEVEL                       (0x9 << 26)
>>
>>  #define EXYNOS5_DRD_PHYPARAM1                        0x20
>> -
>>  #define PHYPARAM1_PCS_TXDEEMPH_MASK          (0x1f << 0)
>>  #define PHYPARAM1_PCS_TXDEEMPH                       (0x1c)
>>
>>  #define EXYNOS5_DRD_PHYTERM                  0x24
>>
>>  #define EXYNOS5_DRD_PHYTEST                  0x28
>> -
>>  #define PHYTEST_POWERDOWN_SSP                        BIT(3)
>>  #define PHYTEST_POWERDOWN_HSP                        BIT(2)
>>
>>  #define EXYNOS5_DRD_PHYADP                   0x2c
>>
>>  #define EXYNOS5_DRD_PHYUTMICLKSEL            0x30
>> -
>
> this and all above are unrelated to $subject.

True, but i have mentioned about this change in the commit message.
It looked so trivial to me that i included it here in this patch.

Will separate it out in a separate patch if you like that. :-)

>
>>  #define PHYUTMICLKSEL_UTMI_CLKSEL            BIT(2)
>>
>>  #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)
>> +
>> +#define EXYNOS5_DRD_PHYSS_LANE0_TX_DEBUG             (0x1010)
>> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M              (0x4 << 4)
>> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_24M           (0x8 << 4)
>> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_25M_26M               (0x8 << 4)
>> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M   (0x20 << 4)
>> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_62M5          (0x20 << 4)
>> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_96M_100M              (0x40 << 4)
>> +
>>  #define KHZ  1000
>>  #define MHZ  (KHZ * KHZ)
>>
>> @@ -135,12 +149,14 @@ struct exynos5_usbdrd_phy_config {
>>       void (*phy_isol)(struct phy_usb_instance *inst, u32 on);
>>       void (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
>>       unsigned int (*set_refclk)(struct phy_usb_instance *inst);
>> +     int (*phy_calibrate)(struct phy_usb_instance *inst);
>>  };
>>
>>  struct exynos5_usbdrd_phy_drvdata {
>>       const struct exynos5_usbdrd_phy_config *phy_cfg;
>>       u32 pmu_offset_usbdrd0_phy;
>>       u32 pmu_offset_usbdrd1_phy;
>> +     void (*phy_exynos_calibrate)(struct exynos5_usbdrd_phy *phy_drd);
>>  };
>>
>>  /**
>> @@ -487,6 +503,142 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
>>       return 0;
>>  }
>>
>> +static void crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
>> +                                             u32 val, u32 cmd)
>> +{
>> +     u32 usec = 100;
>> +     u32 result;
>> +
>> +     writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> +
>> +     do {
>> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
>> +             if (result & PHYREG1_CR_ACK)
>> +                     break;
>> +
>> +             udelay(1);
>> +     } while (usec-- > 0);
>> +
>> +     if (!usec)
>> +             dev_err(phy_drd->dev,
>> +                     "CRPORT handshake timeout1 (0x%08x)\n", val);
>
> if handshake timed out, shouldn't you error out here ? IOW, make this
> function return an int and return an error code from this point ? Why
> would you even continue to the next step ?

Point taken, we should have error'ed out here once timeout occured.
Will update it accordingly.

>
>> +
>> +     usec = 100;
>> +
>> +     writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> +
>> +     do {
>> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
>> +             if (!(result & PHYREG1_CR_ACK))
>> +                     break;
>> +
>> +             udelay(1);
>> +     } while (usec-- > 0);
>> +
>> +     if (!usec)
>> +             dev_err(phy_drd->dev,
>> +                     "CRPORT handshake timeout2 (0x%08x)\n", val);
>
> return an error ?
sure

>
>> +}
>> +
>> +static void crport_ctrl_write(struct exynos5_usbdrd_phy *phy_drd,
>> +                                             u32 addr, u32 data)
>> +{
>> +     /* Write Address */
>> +     writel(PHYREG0_CR_DATA_IN(addr),
>> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(addr),
>> +                      PHYREG0_CR_CAP_ADDR);
>> +
>> +     /* Write Data */
>> +     writel(PHYREG0_CR_DATA_IN(data),
>> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
>> +                      PHYREG0_CR_CAP_DATA);
>> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
>> +                      PHYREG0_CR_WRITE);
>> +}
>> +
>> +/*
>> + * Override PHY paramaeters using CR_PORT register to calibrate settings
>> + * to meet meet SuperSpeed requirements, on Exynos5420 and Exynos5800 systems,
>> + * which have 28nm USB 3.0 DRD PHY.
>> + */
>> +static void exynos5420_usbdrd_phy_calibrate(struct exynos5_usbdrd_phy *phy_drd)
>> +{
>> +     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(phy_drd,
>> +                       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(phy_drd,
>> +                       EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN,
>> +                       temp);
>> +
>> +     /*
>> +      * Set proper time to wait for RxDetect measurement, for
>> +      * desired reference clock of PHY, by tuning the CRPORT
>> +      * register LANE0.TX_DEBUG which is internal to PHY.
>> +      * This fixes issue with few USB 3.0 devices, which are
>> +      * not detected (not even generate interrupts on the bus
>> +      * on insertion) without this change.
>> +      * e.g. Samsung SUM-TSB16S 3.0 USB drive.
>> +      */
>> +     switch (phy_drd->extrefclk) {
>> +     case EXYNOS5_FSEL_50MHZ:
>> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M;
>> +             break;
>> +     case EXYNOS5_FSEL_20MHZ:
>> +     case EXYNOS5_FSEL_19MHZ2:
>> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M;
>> +             break;
>> +     case EXYNOS5_FSEL_24MHZ:
>
> why can't this be done during probe() ? Are you going to change external
> reference clock frequency during runtime ? And even if you do,
> wouldn't it be easier to subscribe to clock rate change notifiers
> instead of adding this ->calibrate() ?

Wait, did this patch give an impression that we are changing the reference
clock settings ?

We are actually trying to override some of the PHY parameters, which is
possible only after the controller has been reset.
These PHY parameter overriding (on PHY registers which are not exposed outside)
is done post initialization, only through EXYNOS5_DRD_PHYREG0.

>
> It still looks, to me, that this ->calibrate() thing is unnecessary. We
> have a single user for it and this user could very well get this sorted
> out during probe() or by subscribing to clock rate notifiers.

We can't program these parameters during PHY probe(), since they reset to
their default value after the controller reset.
So we needed a way to do this post initialization, once the controller has been
reset.



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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Wed, Sep 03, 2014 at 12:59:27PM +0530, Vivek Gautam wrote:
> > On Tue, Sep 02, 2014 at 04:42:18PM +0530, Vivek Gautam wrote:
> >> Adding phy calibrate callback, which facilitates setting certain
> >> PHY settings post initialization of the PHY controller.
> >> Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
> >> the Loss-of-Signal (LOS) Detector Threshold Level as well as
> >> Tx-Vboost-Level should be controlled for Super-Speed operations.
> >>
> >> Additionally set proper time to wait for RxDetect measurement,
> >> for desired PHY reference clock, so as to solve issue with enumeration
> >> of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
> >> on the controller.
> >> 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, and peach-pit boards 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 1-1: new high-speed USB device number 2 using xhci-hcd
> >>
> >> [Also removed unnecessary extra lines in the register macro definitions]
> >>
> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
> >> ---
> >>  drivers/phy/phy-exynos5-usbdrd.c |  185 ++++++++++++++++++++++++++++++++++----
> >>  1 file changed, 170 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/phy/phy-exynos5-usbdrd.c b/drivers/phy/phy-exynos5-usbdrd.c
> >> index 47f47fe..85742b0 100644
> >> --- a/drivers/phy/phy-exynos5-usbdrd.c
> >> +++ b/drivers/phy/phy-exynos5-usbdrd.c
> >> @@ -37,13 +37,11 @@
> >>
> >>  /* EXYNOS5: USB 3.0 DRD PHY registers */
> >>  #define EXYNOS5_DRD_LINKSYSTEM                       0x04
> >> -
> >>  #define LINKSYSTEM_FLADJ_MASK                        (0x3f << 1)
> >>  #define LINKSYSTEM_FLADJ(_x)                 ((_x) << 1)
> >>  #define LINKSYSTEM_XHCI_VERSION_CONTROL              BIT(27)
> >>
> >>  #define EXYNOS5_DRD_PHYUTMI                  0x08
> >> -
> >>  #define PHYUTMI_OTGDISABLE                   BIT(6)
> >>  #define PHYUTMI_FORCESUSPEND                 BIT(1)
> >>  #define PHYUTMI_FORCESLEEP                   BIT(0)
> >> @@ -51,26 +49,20 @@
> >>  #define EXYNOS5_DRD_PHYPIPE                  0x0c
> >>
> >>  #define EXYNOS5_DRD_PHYCLKRST                        0x10
> >> -
> >>  #define PHYCLKRST_EN_UTMISUSPEND             BIT(31)
> >> -
> >>  #define PHYCLKRST_SSC_REFCLKSEL_MASK         (0xff << 23)
> >>  #define PHYCLKRST_SSC_REFCLKSEL(_x)          ((_x) << 23)
> >> -
> >>  #define PHYCLKRST_SSC_RANGE_MASK             (0x03 << 21)
> >>  #define PHYCLKRST_SSC_RANGE(_x)                      ((_x) << 21)
> >> -
> >>  #define PHYCLKRST_SSC_EN                     BIT(20)
> >>  #define PHYCLKRST_REF_SSP_EN                 BIT(19)
> >>  #define PHYCLKRST_REF_CLKDIV2                        BIT(18)
> >> -
> >>  #define PHYCLKRST_MPLL_MULTIPLIER_MASK               (0x7f << 11)
> >>  #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19 << 11)
> >>  #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF    (0x32 << 11)
> >>  #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF  (0x68 << 11)
> >>  #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF  (0x7d << 11)
> >>  #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF       (0x02 << 11)
> >> -
> >>  #define PHYCLKRST_FSEL_UTMI_MASK             (0x7 << 5)
> >>  #define PHYCLKRST_FSEL_PIPE_MASK             (0x7 << 8)
> >>  #define PHYCLKRST_FSEL(_x)                   ((_x) << 5)
> >> @@ -78,46 +70,68 @@
> >>  #define PHYCLKRST_FSEL_PAD_24MHZ             (0x2a << 5)
> >>  #define PHYCLKRST_FSEL_PAD_20MHZ             (0x31 << 5)
> >>  #define PHYCLKRST_FSEL_PAD_19_2MHZ           (0x38 << 5)
> >> -
> >>  #define PHYCLKRST_RETENABLEN                 BIT(4)
> >> -
> >>  #define PHYCLKRST_REFCLKSEL_MASK             (0x03 << 2)
> >>  #define PHYCLKRST_REFCLKSEL_PAD_REFCLK               (0x2 << 2)
> >>  #define PHYCLKRST_REFCLKSEL_EXT_REFCLK               (0x3 << 2)
> >> -
> >>  #define PHYCLKRST_PORTRESET                  BIT(1)
> >>  #define PHYCLKRST_COMMONONN                  BIT(0)
> >>
> >>  #define EXYNOS5_DRD_PHYREG0                  0x14
> >> +#define PHYREG0_SSC_REF_CLK_SEL                      BIT(21)
> >> +#define PHYREG0_SSC_RANGE                    BIT(20)
> >> +#define PHYREG0_CR_WRITE                     BIT(19)
> >> +#define PHYREG0_CR_READ                              BIT(18)
> >> +#define PHYREG0_CR_DATA_IN(_x)                       ((_x) << 2)
> >> +#define PHYREG0_CR_CAP_DATA                  BIT(1)
> >> +#define PHYREG0_CR_CAP_ADDR                  BIT(0)
> >> +
> >>  #define EXYNOS5_DRD_PHYREG1                  0x18
> >> +#define PHYREG1_CR_DATA_OUT(_x)                      ((_x) << 1)
> >> +#define PHYREG1_CR_ACK                               BIT(0)
> >>
> >>  #define EXYNOS5_DRD_PHYPARAM0                        0x1c
> >> -
> >>  #define PHYPARAM0_REF_USE_PAD                        BIT(31)
> >>  #define PHYPARAM0_REF_LOSLEVEL_MASK          (0x1f << 26)
> >>  #define PHYPARAM0_REF_LOSLEVEL                       (0x9 << 26)
> >>
> >>  #define EXYNOS5_DRD_PHYPARAM1                        0x20
> >> -
> >>  #define PHYPARAM1_PCS_TXDEEMPH_MASK          (0x1f << 0)
> >>  #define PHYPARAM1_PCS_TXDEEMPH                       (0x1c)
> >>
> >>  #define EXYNOS5_DRD_PHYTERM                  0x24
> >>
> >>  #define EXYNOS5_DRD_PHYTEST                  0x28
> >> -
> >>  #define PHYTEST_POWERDOWN_SSP                        BIT(3)
> >>  #define PHYTEST_POWERDOWN_HSP                        BIT(2)
> >>
> >>  #define EXYNOS5_DRD_PHYADP                   0x2c
> >>
> >>  #define EXYNOS5_DRD_PHYUTMICLKSEL            0x30
> >> -
> >
> > this and all above are unrelated to $subject.
> 
> True, but i have mentioned about this change in the commit message.
> It looked so trivial to me that i included it here in this patch.
> 
> Will separate it out in a separate patch if you like that. :-)

probably not necessary. Kishon's got the final word for drivers/phy,
though.

> 
> >
> >>  #define PHYUTMICLKSEL_UTMI_CLKSEL            BIT(2)
> >>
> >>  #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)
> >> +
> >> +#define EXYNOS5_DRD_PHYSS_LANE0_TX_DEBUG             (0x1010)
> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M              (0x4 << 4)
> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_24M           (0x8 << 4)
> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_25M_26M               (0x8 << 4)
> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M   (0x20 << 4)
> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_62M5          (0x20 << 4)
> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_96M_100M              (0x40 << 4)
> >> +
> >>  #define KHZ  1000
> >>  #define MHZ  (KHZ * KHZ)
> >>
> >> @@ -135,12 +149,14 @@ struct exynos5_usbdrd_phy_config {
> >>       void (*phy_isol)(struct phy_usb_instance *inst, u32 on);
> >>       void (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
> >>       unsigned int (*set_refclk)(struct phy_usb_instance *inst);
> >> +     int (*phy_calibrate)(struct phy_usb_instance *inst);
> >>  };
> >>
> >>  struct exynos5_usbdrd_phy_drvdata {
> >>       const struct exynos5_usbdrd_phy_config *phy_cfg;
> >>       u32 pmu_offset_usbdrd0_phy;
> >>       u32 pmu_offset_usbdrd1_phy;
> >> +     void (*phy_exynos_calibrate)(struct exynos5_usbdrd_phy *phy_drd);
> >>  };
> >>
> >>  /**
> >> @@ -487,6 +503,142 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
> >>       return 0;
> >>  }
> >>
> >> +static void crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
> >> +                                             u32 val, u32 cmd)
> >> +{
> >> +     u32 usec = 100;
> >> +     u32 result;
> >> +
> >> +     writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> >> +
> >> +     do {
> >> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> >> +             if (result & PHYREG1_CR_ACK)
> >> +                     break;
> >> +
> >> +             udelay(1);
> >> +     } while (usec-- > 0);
> >> +
> >> +     if (!usec)
> >> +             dev_err(phy_drd->dev,
> >> +                     "CRPORT handshake timeout1 (0x%08x)\n", val);
> >
> > if handshake timed out, shouldn't you error out here ? IOW, make this
> > function return an int and return an error code from this point ? Why
> > would you even continue to the next step ?
> 
> Point taken, we should have error'ed out here once timeout occured.
> Will update it accordingly.
> 
> >
> >> +
> >> +     usec = 100;
> >> +
> >> +     writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> >> +
> >> +     do {
> >> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
> >> +             if (!(result & PHYREG1_CR_ACK))
> >> +                     break;
> >> +
> >> +             udelay(1);
> >> +     } while (usec-- > 0);
> >> +
> >> +     if (!usec)
> >> +             dev_err(phy_drd->dev,
> >> +                     "CRPORT handshake timeout2 (0x%08x)\n", val);
> >
> > return an error ?
> sure
> 
> >
> >> +}
> >> +
> >> +static void crport_ctrl_write(struct exynos5_usbdrd_phy *phy_drd,
> >> +                                             u32 addr, u32 data)
> >> +{
> >> +     /* Write Address */
> >> +     writel(PHYREG0_CR_DATA_IN(addr),
> >> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(addr),
> >> +                      PHYREG0_CR_CAP_ADDR);
> >> +
> >> +     /* Write Data */
> >> +     writel(PHYREG0_CR_DATA_IN(data),
> >> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
> >> +                      PHYREG0_CR_CAP_DATA);
> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
> >> +                      PHYREG0_CR_WRITE);
> >> +}
> >> +
> >> +/*
> >> + * Override PHY paramaeters using CR_PORT register to calibrate settings
> >> + * to meet meet SuperSpeed requirements, on Exynos5420 and Exynos5800 systems,
> >> + * which have 28nm USB 3.0 DRD PHY.
> >> + */
> >> +static void exynos5420_usbdrd_phy_calibrate(struct exynos5_usbdrd_phy *phy_drd)
> >> +{
> >> +     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(phy_drd,
> >> +                       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(phy_drd,
> >> +                       EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN,
> >> +                       temp);
> >> +
> >> +     /*
> >> +      * Set proper time to wait for RxDetect measurement, for
> >> +      * desired reference clock of PHY, by tuning the CRPORT
> >> +      * register LANE0.TX_DEBUG which is internal to PHY.
> >> +      * This fixes issue with few USB 3.0 devices, which are
> >> +      * not detected (not even generate interrupts on the bus
> >> +      * on insertion) without this change.
> >> +      * e.g. Samsung SUM-TSB16S 3.0 USB drive.
> >> +      */
> >> +     switch (phy_drd->extrefclk) {
> >> +     case EXYNOS5_FSEL_50MHZ:
> >> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M;
> >> +             break;
> >> +     case EXYNOS5_FSEL_20MHZ:
> >> +     case EXYNOS5_FSEL_19MHZ2:
> >> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M;
> >> +             break;
> >> +     case EXYNOS5_FSEL_24MHZ:
> >
> > why can't this be done during probe() ? Are you going to change external
> > reference clock frequency during runtime ? And even if you do,
> > wouldn't it be easier to subscribe to clock rate change notifiers
> > instead of adding this ->calibrate() ?
> 
> Wait, did this patch give an impression that we are changing the reference
> clock settings ?

not, it gives the impression that you need to change RX Detect timeout
value based on external reference clock frequency :-)

> We are actually trying to override some of the PHY parameters, which
> is possible only after the controller has been reset.
> These PHY parameter overriding (on PHY registers which are not exposed
> outside) is done post initialization, only through
> EXYNOS5_DRD_PHYREG0.

so it _has_ to be done after dwc3's reset ? Don't we have phy_power_on()
for that ? It looks like you could just as well do this from
phy_power_on() ?

> > It still looks, to me, that this ->calibrate() thing is unnecessary. We
> > have a single user for it and this user could very well get this sorted
> > out during probe() or by subscribing to clock rate notifiers.
> 
> We can't program these parameters during PHY probe(), since they reset to
> their default value after the controller reset.
> So we needed a way to do this post initialization, once the controller
> has been reset.

right, and we already have a call to phy_power_on() after core has been
reset. Can't you use that ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140903/d4da229f/attachment-0001.sig>

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

On Wed, Sep 3, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Wed, Sep 03, 2014 at 12:59:27PM +0530, Vivek Gautam wrote:
>> > On Tue, Sep 02, 2014 at 04:42:18PM +0530, Vivek Gautam wrote:
>> >> Adding phy calibrate callback, which facilitates setting certain
>> >> PHY settings post initialization of the PHY controller.
>> >> Exynos5420 and Exynos5800 have 28nm USB 3.0 DRD PHY for which
>> >> the Loss-of-Signal (LOS) Detector Threshold Level as well as
>> >> Tx-Vboost-Level should be controlled for Super-Speed operations.
>> >>
>> >> Additionally set proper time to wait for RxDetect measurement,
>> >> for desired PHY reference clock, so as to solve issue with enumeration
>> >> of few USB 3.0 devices, like Samsung SUM-TSB16S 3.0 USB drive
>> >> on the controller.
>> >> 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, and peach-pit boards 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 1-1: new high-speed USB device number 2 using xhci-hcd
>> >>
>> >> [Also removed unnecessary extra lines in the register macro definitions]
>> >>
>> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
>> >> ---
>> >>  drivers/phy/phy-exynos5-usbdrd.c |  185 ++++++++++++++++++++++++++++++++++----
>> >>  1 file changed, 170 insertions(+), 15 deletions(-)
>> >>
>> >> diff --git a/drivers/phy/phy-exynos5-usbdrd.c b/drivers/phy/phy-exynos5-usbdrd.c
>> >> index 47f47fe..85742b0 100644
>> >> --- a/drivers/phy/phy-exynos5-usbdrd.c
>> >> +++ b/drivers/phy/phy-exynos5-usbdrd.c
>> >> @@ -37,13 +37,11 @@
>> >>
>> >>  /* EXYNOS5: USB 3.0 DRD PHY registers */
>> >>  #define EXYNOS5_DRD_LINKSYSTEM                       0x04
>> >> -
>> >>  #define LINKSYSTEM_FLADJ_MASK                        (0x3f << 1)
>> >>  #define LINKSYSTEM_FLADJ(_x)                 ((_x) << 1)
>> >>  #define LINKSYSTEM_XHCI_VERSION_CONTROL              BIT(27)
>> >>
>> >>  #define EXYNOS5_DRD_PHYUTMI                  0x08
>> >> -
>> >>  #define PHYUTMI_OTGDISABLE                   BIT(6)
>> >>  #define PHYUTMI_FORCESUSPEND                 BIT(1)
>> >>  #define PHYUTMI_FORCESLEEP                   BIT(0)
>> >> @@ -51,26 +49,20 @@
>> >>  #define EXYNOS5_DRD_PHYPIPE                  0x0c
>> >>
>> >>  #define EXYNOS5_DRD_PHYCLKRST                        0x10
>> >> -
>> >>  #define PHYCLKRST_EN_UTMISUSPEND             BIT(31)
>> >> -
>> >>  #define PHYCLKRST_SSC_REFCLKSEL_MASK         (0xff << 23)
>> >>  #define PHYCLKRST_SSC_REFCLKSEL(_x)          ((_x) << 23)
>> >> -
>> >>  #define PHYCLKRST_SSC_RANGE_MASK             (0x03 << 21)
>> >>  #define PHYCLKRST_SSC_RANGE(_x)                      ((_x) << 21)
>> >> -
>> >>  #define PHYCLKRST_SSC_EN                     BIT(20)
>> >>  #define PHYCLKRST_REF_SSP_EN                 BIT(19)
>> >>  #define PHYCLKRST_REF_CLKDIV2                        BIT(18)
>> >> -
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_MASK               (0x7f << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF (0x19 << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_50M_REF    (0x32 << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_24MHZ_REF  (0x68 << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_20MHZ_REF  (0x7d << 11)
>> >>  #define PHYCLKRST_MPLL_MULTIPLIER_19200KHZ_REF       (0x02 << 11)
>> >> -
>> >>  #define PHYCLKRST_FSEL_UTMI_MASK             (0x7 << 5)
>> >>  #define PHYCLKRST_FSEL_PIPE_MASK             (0x7 << 8)
>> >>  #define PHYCLKRST_FSEL(_x)                   ((_x) << 5)
>> >> @@ -78,46 +70,68 @@
>> >>  #define PHYCLKRST_FSEL_PAD_24MHZ             (0x2a << 5)
>> >>  #define PHYCLKRST_FSEL_PAD_20MHZ             (0x31 << 5)
>> >>  #define PHYCLKRST_FSEL_PAD_19_2MHZ           (0x38 << 5)
>> >> -
>> >>  #define PHYCLKRST_RETENABLEN                 BIT(4)
>> >> -
>> >>  #define PHYCLKRST_REFCLKSEL_MASK             (0x03 << 2)
>> >>  #define PHYCLKRST_REFCLKSEL_PAD_REFCLK               (0x2 << 2)
>> >>  #define PHYCLKRST_REFCLKSEL_EXT_REFCLK               (0x3 << 2)
>> >> -
>> >>  #define PHYCLKRST_PORTRESET                  BIT(1)
>> >>  #define PHYCLKRST_COMMONONN                  BIT(0)
>> >>
>> >>  #define EXYNOS5_DRD_PHYREG0                  0x14
>> >> +#define PHYREG0_SSC_REF_CLK_SEL                      BIT(21)
>> >> +#define PHYREG0_SSC_RANGE                    BIT(20)
>> >> +#define PHYREG0_CR_WRITE                     BIT(19)
>> >> +#define PHYREG0_CR_READ                              BIT(18)
>> >> +#define PHYREG0_CR_DATA_IN(_x)                       ((_x) << 2)
>> >> +#define PHYREG0_CR_CAP_DATA                  BIT(1)
>> >> +#define PHYREG0_CR_CAP_ADDR                  BIT(0)
>> >> +
>> >>  #define EXYNOS5_DRD_PHYREG1                  0x18
>> >> +#define PHYREG1_CR_DATA_OUT(_x)                      ((_x) << 1)
>> >> +#define PHYREG1_CR_ACK                               BIT(0)
>> >>
>> >>  #define EXYNOS5_DRD_PHYPARAM0                        0x1c
>> >> -
>> >>  #define PHYPARAM0_REF_USE_PAD                        BIT(31)
>> >>  #define PHYPARAM0_REF_LOSLEVEL_MASK          (0x1f << 26)
>> >>  #define PHYPARAM0_REF_LOSLEVEL                       (0x9 << 26)
>> >>
>> >>  #define EXYNOS5_DRD_PHYPARAM1                        0x20
>> >> -
>> >>  #define PHYPARAM1_PCS_TXDEEMPH_MASK          (0x1f << 0)
>> >>  #define PHYPARAM1_PCS_TXDEEMPH                       (0x1c)
>> >>
>> >>  #define EXYNOS5_DRD_PHYTERM                  0x24
>> >>
>> >>  #define EXYNOS5_DRD_PHYTEST                  0x28
>> >> -
>> >>  #define PHYTEST_POWERDOWN_SSP                        BIT(3)
>> >>  #define PHYTEST_POWERDOWN_HSP                        BIT(2)
>> >>
>> >>  #define EXYNOS5_DRD_PHYADP                   0x2c
>> >>
>> >>  #define EXYNOS5_DRD_PHYUTMICLKSEL            0x30
>> >> -
>> >
>> > this and all above are unrelated to $subject.
>>
>> True, but i have mentioned about this change in the commit message.
>> It looked so trivial to me that i included it here in this patch.
>>
>> Will separate it out in a separate patch if you like that. :-)
>
> probably not necessary. Kishon's got the final word for drivers/phy,
> though.

sure,

>
>>
>> >
>> >>  #define PHYUTMICLKSEL_UTMI_CLKSEL            BIT(2)
>> >>
>> >>  #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)
>> >> +
>> >> +#define EXYNOS5_DRD_PHYSS_LANE0_TX_DEBUG             (0x1010)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M              (0x4 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_24M           (0x8 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_25M_26M               (0x8 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M   (0x20 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_62M5          (0x20 << 4)
>> >> +#define LANE0_TX_DEBUG_RXDET_MEAS_TIME_96M_100M              (0x40 << 4)
>> >> +
>> >>  #define KHZ  1000
>> >>  #define MHZ  (KHZ * KHZ)
>> >>
>> >> @@ -135,12 +149,14 @@ struct exynos5_usbdrd_phy_config {
>> >>       void (*phy_isol)(struct phy_usb_instance *inst, u32 on);
>> >>       void (*phy_init)(struct exynos5_usbdrd_phy *phy_drd);
>> >>       unsigned int (*set_refclk)(struct phy_usb_instance *inst);
>> >> +     int (*phy_calibrate)(struct phy_usb_instance *inst);
>> >>  };
>> >>
>> >>  struct exynos5_usbdrd_phy_drvdata {
>> >>       const struct exynos5_usbdrd_phy_config *phy_cfg;
>> >>       u32 pmu_offset_usbdrd0_phy;
>> >>       u32 pmu_offset_usbdrd1_phy;
>> >> +     void (*phy_exynos_calibrate)(struct exynos5_usbdrd_phy *phy_drd);
>> >>  };
>> >>
>> >>  /**
>> >> @@ -487,6 +503,142 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
>> >>       return 0;
>> >>  }
>> >>
>> >> +static void crport_handshake(struct exynos5_usbdrd_phy *phy_drd,
>> >> +                                             u32 val, u32 cmd)
>> >> +{
>> >> +     u32 usec = 100;
>> >> +     u32 result;
>> >> +
>> >> +     writel(val | cmd, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +
>> >> +     do {
>> >> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
>> >> +             if (result & PHYREG1_CR_ACK)
>> >> +                     break;
>> >> +
>> >> +             udelay(1);
>> >> +     } while (usec-- > 0);
>> >> +
>> >> +     if (!usec)
>> >> +             dev_err(phy_drd->dev,
>> >> +                     "CRPORT handshake timeout1 (0x%08x)\n", val);
>> >
>> > if handshake timed out, shouldn't you error out here ? IOW, make this
>> > function return an int and return an error code from this point ? Why
>> > would you even continue to the next step ?
>>
>> Point taken, we should have error'ed out here once timeout occured.
>> Will update it accordingly.
>>
>> >
>> >> +
>> >> +     usec = 100;
>> >> +
>> >> +     writel(val, phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +
>> >> +     do {
>> >> +             result = readl(phy_drd->reg_phy + EXYNOS5_DRD_PHYREG1);
>> >> +             if (!(result & PHYREG1_CR_ACK))
>> >> +                     break;
>> >> +
>> >> +             udelay(1);
>> >> +     } while (usec-- > 0);
>> >> +
>> >> +     if (!usec)
>> >> +             dev_err(phy_drd->dev,
>> >> +                     "CRPORT handshake timeout2 (0x%08x)\n", val);
>> >
>> > return an error ?
>> sure
>>
>> >
>> >> +}
>> >> +
>> >> +static void crport_ctrl_write(struct exynos5_usbdrd_phy *phy_drd,
>> >> +                                             u32 addr, u32 data)
>> >> +{
>> >> +     /* Write Address */
>> >> +     writel(PHYREG0_CR_DATA_IN(addr),
>> >> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(addr),
>> >> +                      PHYREG0_CR_CAP_ADDR);
>> >> +
>> >> +     /* Write Data */
>> >> +     writel(PHYREG0_CR_DATA_IN(data),
>> >> +                     phy_drd->reg_phy + EXYNOS5_DRD_PHYREG0);
>> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
>> >> +                      PHYREG0_CR_CAP_DATA);
>> >> +     crport_handshake(phy_drd, PHYREG0_CR_DATA_IN(data),
>> >> +                      PHYREG0_CR_WRITE);
>> >> +}
>> >> +
>> >> +/*
>> >> + * Override PHY paramaeters using CR_PORT register to calibrate settings
>> >> + * to meet meet SuperSpeed requirements, on Exynos5420 and Exynos5800 systems,
>> >> + * which have 28nm USB 3.0 DRD PHY.
>> >> + */
>> >> +static void exynos5420_usbdrd_phy_calibrate(struct exynos5_usbdrd_phy *phy_drd)
>> >> +{
>> >> +     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(phy_drd,
>> >> +                       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(phy_drd,
>> >> +                       EXYNOS5_DRD_PHYSS_TX_VBOOSTLEVEL_OVRD_IN,
>> >> +                       temp);
>> >> +
>> >> +     /*
>> >> +      * Set proper time to wait for RxDetect measurement, for
>> >> +      * desired reference clock of PHY, by tuning the CRPORT
>> >> +      * register LANE0.TX_DEBUG which is internal to PHY.
>> >> +      * This fixes issue with few USB 3.0 devices, which are
>> >> +      * not detected (not even generate interrupts on the bus
>> >> +      * on insertion) without this change.
>> >> +      * e.g. Samsung SUM-TSB16S 3.0 USB drive.
>> >> +      */
>> >> +     switch (phy_drd->extrefclk) {
>> >> +     case EXYNOS5_FSEL_50MHZ:
>> >> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_48M_50M_52M;
>> >> +             break;
>> >> +     case EXYNOS5_FSEL_20MHZ:
>> >> +     case EXYNOS5_FSEL_19MHZ2:
>> >> +             temp = LANE0_TX_DEBUG_RXDET_MEAS_TIME_19M2_20M;
>> >> +             break;
>> >> +     case EXYNOS5_FSEL_24MHZ:
>> >
>> > why can't this be done during probe() ? Are you going to change external
>> > reference clock frequency during runtime ? And even if you do,
>> > wouldn't it be easier to subscribe to clock rate change notifiers
>> > instead of adding this ->calibrate() ?
>>
>> Wait, did this patch give an impression that we are changing the reference
>> clock settings ?
>
> not, it gives the impression that you need to change RX Detect timeout
> value based on external reference clock frequency :-)
>
>> We are actually trying to override some of the PHY parameters, which
>> is possible only after the controller has been reset.
>> These PHY parameter overriding (on PHY registers which are not exposed
>> outside) is done post initialization, only through
>> EXYNOS5_DRD_PHYREG0.
>
> so it _has_ to be done after dwc3's reset ?

By  controller reset i meant XHCI reset in fact.
What we see with this PHY controller on Exynos5420/5800 that it's only when
the host controller has been reset, the PHY parameters (which we are
calibrating here)
settings remain intact.
We have already tried earlier moving this calibration afte DWC3 core
reset occurs,
but that doesn't help.
That's the reason we moved the calibration stuff to usb_add_hcd() and
usb_bus_resume()
functions.

> Don't we have phy_power_on()
> for that ? It looks like you could just as well do this from
> phy_power_on() ?

No, unfortunately keeping these calibration settings in phy_power_on()
doesn't help, since we need to do this after XHCI reset has happened.

>
>> > It still looks, to me, that this ->calibrate() thing is unnecessary. We
>> > have a single user for it and this user could very well get this sorted
>> > out during probe() or by subscribing to clock rate notifiers.
>>
>> We can't program these parameters during PHY probe(), since they reset to
>> their default value after the controller reset.
>> So we needed a way to do this post initialization, once the controller
>> has been reset.
>
> right, and we already have a call to phy_power_on() after core has been
> reset. Can't you use that ?
>
> --
> balbi



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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
> > Don't we have phy_power_on()
> > for that ? It looks like you could just as well do this from
> > phy_power_on() ?
> 
> No, unfortunately keeping these calibration settings in phy_power_on()
> doesn't help, since we need to do this after XHCI reset has happened.

teach xHCI about PHYs ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140905/dc3b01dc/attachment.sig>

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
>> > Don't we have phy_power_on()
>> > for that ? It looks like you could just as well do this from
>> > phy_power_on() ?
>>
>> No, unfortunately keeping these calibration settings in phy_power_on()
>> doesn't help, since we need to do this after XHCI reset has happened.
>
> teach xHCI about PHYs ?

sorry i couldn't understand you here.
Aren't we trying to do the same with Heikki's patch about dwc3 :
[PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci

and the 2nd patch in this series :
[PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds

Is there something else that is expected ?




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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
> >> > Don't we have phy_power_on()
> >> > for that ? It looks like you could just as well do this from
> >> > phy_power_on() ?
> >>
> >> No, unfortunately keeping these calibration settings in phy_power_on()
> >> doesn't help, since we need to do this after XHCI reset has happened.
> >
> > teach xHCI about PHYs ?
> 
> sorry i couldn't understand you here.
> Aren't we trying to do the same with Heikki's patch about dwc3 :
> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
> 
> and the 2nd patch in this series :
> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
> 
> Is there something else that is expected ?

right, use that to call phy_init() at the right time, then you need to
add a new ->calibrate() method which, likely, will only be used by you
;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140908/e3ea50a9/attachment.sig>

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

Hi,


On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
> Hi,
>
> On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
>> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
>> >> > Don't we have phy_power_on()
>> >> > for that ? It looks like you could just as well do this from
>> >> > phy_power_on() ?
>> >>
>> >> No, unfortunately keeping these calibration settings in phy_power_on()
>> >> doesn't help, since we need to do this after XHCI reset has happened.
>> >
>> > teach xHCI about PHYs ?
>>
>> sorry i couldn't understand you here.
>> Aren't we trying to do the same with Heikki's patch about dwc3 :
>> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
>>
>> and the 2nd patch in this series :
>> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
>>
>> Is there something else that is expected ?
>
> right, use that to call phy_init() at the right time, then you need to
> add a new ->calibrate() method which, likely, will only be used by you
> ;-)

so you mean, the xhci should itself call phy_init() at a time suitable,
so that ->calibrate() is not required at all ?

i think you meant there - "then you __do not__ need to
> add a new ->calibrate() method which, likely,
will only be used by you", is it ?




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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
> Hi,
> 
> 
> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
> > Hi,
> >
> > On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
> >> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> >> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
> >> >> > Don't we have phy_power_on()
> >> >> > for that ? It looks like you could just as well do this from
> >> >> > phy_power_on() ?
> >> >>
> >> >> No, unfortunately keeping these calibration settings in phy_power_on()
> >> >> doesn't help, since we need to do this after XHCI reset has happened.
> >> >
> >> > teach xHCI about PHYs ?
> >>
> >> sorry i couldn't understand you here.
> >> Aren't we trying to do the same with Heikki's patch about dwc3 :
> >> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
> >>
> >> and the 2nd patch in this series :
> >> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
> >>
> >> Is there something else that is expected ?
> >
> > right, use that to call phy_init() at the right time, then you need to
> > add a new ->calibrate() method which, likely, will only be used by you
> > ;-)
> 
> so you mean, the xhci should itself call phy_init() at a time suitable,
> so that ->calibrate() is not required at all ?
> 
> i think you meant there - "then you __do not__ need to

right :-)

> > add a new ->calibrate() method which, likely,
> will only be used by you", is it ?

yup

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140909/cfa35ca9/attachment.sig>

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

adding Julius here,


On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
>> Hi,
>>
>>
>> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
>> > Hi,
>> >
>> > On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
>> >> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
>> >> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
>> >> >> > Don't we have phy_power_on()
>> >> >> > for that ? It looks like you could just as well do this from
>> >> >> > phy_power_on() ?
>> >> >>
>> >> >> No, unfortunately keeping these calibration settings in phy_power_on()
>> >> >> doesn't help, since we need to do this after XHCI reset has happened.
>> >> >
>> >> > teach xHCI about PHYs ?
>> >>
>> >> sorry i couldn't understand you here.
>> >> Aren't we trying to do the same with Heikki's patch about dwc3 :
>> >> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
>> >>
>> >> and the 2nd patch in this series :
>> >> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
>> >>
>> >> Is there something else that is expected ?
>> >
>> > right, use that to call phy_init() at the right time, then you need to
>> > add a new ->calibrate() method which, likely, will only be used by you
>> > ;-)
>>
>> so you mean, the xhci should itself call phy_init() at a time suitable,
>> so that ->calibrate() is not required at all ?
>>
>> i think you meant there - "then you __do not__ need to
>
> right :-)

alright, i will prepare a patch for the suggested change.

But AFAI remember we had discussion for this patch in earlier version,
and Julius suggested to use a generic approach for such
change wherein other users in future may be able to use the
facility.
Julius any thoughts about the change suggested by Felipe.

[snip]


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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> adding Julius here,

i think i had missed adding Julius for this entire series :-(
I should be more careful with the CC list in future.
Added his chromium id, since that seems to be more active.

>
>
> On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
>> On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
>>> Hi,
>>>
>>>
>>> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
>>> > Hi,
>>> >
>>> > On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
>>> >> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
>>> >> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
>>> >> >> > Don't we have phy_power_on()
>>> >> >> > for that ? It looks like you could just as well do this from
>>> >> >> > phy_power_on() ?
>>> >> >>
>>> >> >> No, unfortunately keeping these calibration settings in phy_power_on()
>>> >> >> doesn't help, since we need to do this after XHCI reset has happened.
>>> >> >
>>> >> > teach xHCI about PHYs ?
>>> >>
>>> >> sorry i couldn't understand you here.
>>> >> Aren't we trying to do the same with Heikki's patch about dwc3 :
>>> >> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
>>> >>
>>> >> and the 2nd patch in this series :
>>> >> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
>>> >>
>>> >> Is there something else that is expected ?
>>> >
>>> > right, use that to call phy_init() at the right time, then you need to
>>> > add a new ->calibrate() method which, likely, will only be used by you
>>> > ;-)
>>>
>>> so you mean, the xhci should itself call phy_init() at a time suitable,
>>> so that ->calibrate() is not required at all ?
>>>
>>> i think you meant there - "then you __do not__ need to
>>
>> right :-)
>
> alright, i will prepare a patch for the suggested change.
>
> But AFAI remember we had discussion for this patch in earlier version,
> and Julius suggested to use a generic approach for such
> change wherein other users in future may be able to use the
> facility.
> Julius any thoughts about the change suggested by Felipe.




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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

On Wed, Sep 10, 2014 at 09:09:57AM +0530, Vivek Gautam wrote:
> On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> > adding Julius here,
> 
> i think i had missed adding Julius for this entire series :-(
> I should be more careful with the CC list in future.
> Added his chromium id, since that seems to be more active.
> 
> >
> >
> > On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
> >> On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
> >>> Hi,
> >>>
> >>>
> >>> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
> >>> > Hi,
> >>> >
> >>> > On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
> >>> >> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> >>> >> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
> >>> >> >> > Don't we have phy_power_on()
> >>> >> >> > for that ? It looks like you could just as well do this from
> >>> >> >> > phy_power_on() ?
> >>> >> >>
> >>> >> >> No, unfortunately keeping these calibration settings in phy_power_on()
> >>> >> >> doesn't help, since we need to do this after XHCI reset has happened.
> >>> >> >
> >>> >> > teach xHCI about PHYs ?
> >>> >>
> >>> >> sorry i couldn't understand you here.
> >>> >> Aren't we trying to do the same with Heikki's patch about dwc3 :
> >>> >> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
> >>> >>
> >>> >> and the 2nd patch in this series :
> >>> >> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
> >>> >>
> >>> >> Is there something else that is expected ?
> >>> >
> >>> > right, use that to call phy_init() at the right time, then you need to
> >>> > add a new ->calibrate() method which, likely, will only be used by you
> >>> > ;-)
> >>>
> >>> so you mean, the xhci should itself call phy_init() at a time suitable,
> >>> so that ->calibrate() is not required at all ?
> >>>
> >>> i think you meant there - "then you __do not__ need to
> >>
> >> right :-)
> >
> > alright, i will prepare a patch for the suggested change.
> >
> > But AFAI remember we had discussion for this patch in earlier
> > version, and Julius suggested to use a generic approach for such
> > change wherein other users in future may be able to use the
> > facility.

right, and what's more generic than adding the support for PHYs straight
into xHCI ?

What I fear is that we end up opening the doors for every odd
platform-specific operation to be added to the framework without really
considering what needs to be done. That would defeat the idea of having
a generic framework altogether.

cheers

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140909/9d71f7f9/attachment-0001.sig>

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

On Wed, Sep 10, 2014 at 10:23 AM, Felipe Balbi <balbi@ti.com> wrote:
> On Wed, Sep 10, 2014 at 09:09:57AM +0530, Vivek Gautam wrote:
>> On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
>> > adding Julius here,
>>
>> i think i had missed adding Julius for this entire series :-(
>> I should be more careful with the CC list in future.
>> Added his chromium id, since that seems to be more active.
>>
>> >
>> >
>> > On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
>> >> On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
>> >>> Hi,
>> >>>
>> >>>
>> >>> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
>> >>> > Hi,
>> >>> >
>> >>> > On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
>> >>> >> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
>> >>> >> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
>> >>> >> >> > Don't we have phy_power_on()
>> >>> >> >> > for that ? It looks like you could just as well do this from
>> >>> >> >> > phy_power_on() ?
>> >>> >> >>
>> >>> >> >> No, unfortunately keeping these calibration settings in phy_power_on()
>> >>> >> >> doesn't help, since we need to do this after XHCI reset has happened.
>> >>> >> >
>> >>> >> > teach xHCI about PHYs ?
>> >>> >>
>> >>> >> sorry i couldn't understand you here.
>> >>> >> Aren't we trying to do the same with Heikki's patch about dwc3 :
>> >>> >> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
>> >>> >>
>> >>> >> and the 2nd patch in this series :
>> >>> >> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
>> >>> >>
>> >>> >> Is there something else that is expected ?
>> >>> >
>> >>> > right, use that to call phy_init() at the right time, then you need to
>> >>> > add a new ->calibrate() method which, likely, will only be used by you
>> >>> > ;-)
>> >>>
>> >>> so you mean, the xhci should itself call phy_init() at a time suitable,
>> >>> so that ->calibrate() is not required at all ?
>> >>>
>> >>> i think you meant there - "then you __do not__ need to
>> >>
>> >> right :-)
>> >
>> > alright, i will prepare a patch for the suggested change.
>> >
>> > But AFAI remember we had discussion for this patch in earlier
>> > version, and Julius suggested to use a generic approach for such
>> > change wherein other users in future may be able to use the
>> > facility.
>
> right, and what's more generic than adding the support for PHYs straight
> into xHCI ?
>
> What I fear is that we end up opening the doors for every odd
> platform-specific operation to be added to the framework without really
> considering what needs to be done. That would defeat the idea of having
> a generic framework altogether.

Ok, i will prepare the patch series as suggested and post it
to gather opinion from you. :-)

>
> cheers
>
> --
> balbi



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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

On Wed, Sep 10, 2014 at 10:53 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> On Wed, Sep 10, 2014 at 10:23 AM, Felipe Balbi <balbi@ti.com> wrote:
>> On Wed, Sep 10, 2014 at 09:09:57AM +0530, Vivek Gautam wrote:
>>> On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
>>> > adding Julius here,
>>>
>>> i think i had missed adding Julius for this entire series :-(
>>> I should be more careful with the CC list in future.
>>> Added his chromium id, since that seems to be more active.
>>>
>>> >
>>> >
>>> > On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
>>> >> On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
>>> >>> Hi,
>>> >>>
>>> >>>
>>> >>> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
>>> >>> > Hi,
>>> >>> >
>>> >>> > On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
>>> >>> >> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
>>> >>> >> > On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
>>> >>> >> >> > Don't we have phy_power_on()
>>> >>> >> >> > for that ? It looks like you could just as well do this from
>>> >>> >> >> > phy_power_on() ?
>>> >>> >> >>
>>> >>> >> >> No, unfortunately keeping these calibration settings in phy_power_on()
>>> >>> >> >> doesn't help, since we need to do this after XHCI reset has happened.
>>> >>> >> >
>>> >>> >> > teach xHCI about PHYs ?
>>> >>> >>
>>> >>> >> sorry i couldn't understand you here.
>>> >>> >> Aren't we trying to do the same with Heikki's patch about dwc3 :
>>> >>> >> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
>>> >>> >>
>>> >>> >> and the 2nd patch in this series :
>>> >>> >> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
>>> >>> >>
>>> >>> >> Is there something else that is expected ?
>>> >>> >
>>> >>> > right, use that to call phy_init() at the right time, then you need to
>>> >>> > add a new ->calibrate() method which, likely, will only be used by you
>>> >>> > ;-)
>>> >>>
>>> >>> so you mean, the xhci should itself call phy_init() at a time suitable,
>>> >>> so that ->calibrate() is not required at all ?

but wait, dwc3 does a phy_init() already, then how xhci will be able to
do that again. We can't do phy_init() multiple times right ?

>>> >>>
>>> >>> i think you meant there - "then you __do not__ need to
>>> >>
>>> >> right :-)
>>> >
>>> > alright, i will prepare a patch for the suggested change.
>>> >
>>> > But AFAI remember we had discussion for this patch in earlier
>>> > version, and Julius suggested to use a generic approach for such
>>> > change wherein other users in future may be able to use the
>>> > facility.
>>
>> right, and what's more generic than adding the support for PHYs straight
>> into xHCI ?
>>
>> What I fear is that we end up opening the doors for every odd
>> platform-specific operation to be added to the framework without really
>> considering what needs to be done. That would defeat the idea of having
>> a generic framework altogether.
>
> Ok, i will prepare the patch series as suggested and post it
> to gather opinion from you. :-)
>
>>
>> cheers
>>
>> --
>> balbi
>
>
>
> --
> Best Regards
> Vivek Gautam
> Samsung R&D Institute, Bangalore
> India



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

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Julius Werner]

>> right, use that to call phy_init() at the right time, then you need to
>> add a new ->calibrate() method which, likely, will only be used by you
>> ;-)

> so you mean, the xhci should itself call phy_init() at a time suitable,
> so that ->calibrate() is not required at all ?

I'm not sure if that's a good idea because it would require phy_init()
and calibrate() to always mean the same thing. The calibrate() for
Exynos5420 needs to be called both during boot and after system
resume, but there might be other platforms that don't want to
completely shutdown and reinit their PHYs during suspend/resume with
the same functions used for boot. For example, Tegra5 (proposed driver
at http://www.spinics.net/lists/linux-usb/msg113093.html) can
power-gate the PHY during suspend, but that's not running the same
code as in the phy_init()/phy_shutdown() methods (the posted patch
doesn't contain all power-gating code yet, but you can get an idea
about how it has to work from
https://chromium.googlesource.com/chromiumos/third_party/kernel-next/+/chromeos-3.10/drivers/usb/host/xhci-tegra.c).
It would also mean that the initial phy_init() call must always come
after the XHCI reset, and I am not sure if that would fit nicely with
all platforms.

> right, and what's more generic than adding the support for PHYs straight into xHCI ?

Well, if we are going to have a calibrate() method (as in "stuff the
PHY does after every controller reset if it needs to"), we have to add
it either to the XHCI stack or the USB core. I thought the USB core
would be more generic, because in theory it's possible that e.g. an
EHCI controller might have a similar requirement. (Also, we have the
gen_phy pointer in a USB core structure (struct usb_hcd), so I thought
making this feature generic to the USB core and thus available to all
kinds of host controllers would be more natural.)

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Vivek Gautam]

Hi,


On Thursday, September 11, 2014 1:52 AM, "Julius Werner" 
<jwerner@google.com> wrote


>>> right, use that to call phy_init() at the right time, then you need to
>>> add a new ->calibrate() method which, likely, will only be used by you
>>> ;-)
>
>> so you mean, the xhci should itself call phy_init() at a time suitable,
>> so that ->calibrate() is not required at all ?
>
> I'm not sure if that's a good idea because it would require phy_init()
> and calibrate() to always mean the same thing. The calibrate() for
> Exynos5420 needs to be called both during boot and after system
> resume, but there might be other platforms that don't want to
> completely shutdown and reinit their PHYs during suspend/resume with
> the same functions used for boot. For example, Tegra5 (proposed driver
> at http://www.spinics.net/lists/linux-usb/msg113093.html) can
> power-gate the PHY during suspend, but that's not running the same
> code as in the phy_init()/phy_shutdown() methods (the posted patch
> doesn't contain all power-gating code yet, but you can get an idea
> about how it has to work from
> https://chromium.googlesource.com/chromiumos/third_party/kernel-next/+/chromeos-3.10/drivers/usb/host/xhci-tegra.c).
> It would also mean that the initial phy_init() call must always come
> after the XHCI reset, and I am not sure if that would fit nicely with
> all platforms.
>
>> right, and what's more generic than adding the support for PHYs straight 
>> into xHCI ?
>
> Well, if we are going to have a calibrate() method (as in "stuff the
> PHY does after every controller reset if it needs to"), we have to add
> it either to the XHCI stack or the USB core.
> I thought the USB core would be more generic, because in theory it's 
> possible that e.g. an
> EHCI controller might have a similar requirement.

Atleast on Exynos systems I can see that unless PHY is initialized the 
controller
fails to reset. So taking a path wherein we initialize the PHYs after 
controller resets
is not going to help us generically across all controllers.
With XHCI though things are OK while resetting, and we can initialize the 
PHYs
after controller reset happens but here too DWC3 is already initializing the 
PHYs
for exynos systems too, so it may not be very wise to do that initialization 
stuff
once again. Plus the PHY core prevents multiple initialization anyways.

> (Also, we have the gen_phy pointer in a USB core structure (struct 
> usb_hcd), so I thought
> making this feature generic to the USB core and thus available to all
> kinds of host controllers would be more natural.) 

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Kishon Vijay Abraham I]

Hi,

On Wednesday 10 September 2014 01:26 PM, Vivek Gautam wrote:
> On Wed, Sep 10, 2014 at 10:53 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
>> On Wed, Sep 10, 2014 at 10:23 AM, Felipe Balbi <balbi@ti.com> wrote:
>>> On Wed, Sep 10, 2014 at 09:09:57AM +0530, Vivek Gautam wrote:
>>>> On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
>>>>> adding Julius here,
>>>>
>>>> i think i had missed adding Julius for this entire series :-(
>>>> I should be more careful with the CC list in future.
>>>> Added his chromium id, since that seems to be more active.
>>>>
>>>>>
>>>>>
>>>>> On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
>>>>>> On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
>>>>>>>>> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
>>>>>>>>>> On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
>>>>>>>>>>>> Don't we have phy_power_on()
>>>>>>>>>>>> for that ? It looks like you could just as well do this from
>>>>>>>>>>>> phy_power_on() ?
>>>>>>>>>>>
>>>>>>>>>>> No, unfortunately keeping these calibration settings in phy_power_on()
>>>>>>>>>>> doesn't help, since we need to do this after XHCI reset has happened.
>>>>>>>>>>
>>>>>>>>>> teach xHCI about PHYs ?
>>>>>>>>>
>>>>>>>>> sorry i couldn't understand you here.
>>>>>>>>> Aren't we trying to do the same with Heikki's patch about dwc3 :
>>>>>>>>> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
>>>>>>>>>
>>>>>>>>> and the 2nd patch in this series :
>>>>>>>>> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
>>>>>>>>>
>>>>>>>>> Is there something else that is expected ?
>>>>>>>>
>>>>>>>> right, use that to call phy_init() at the right time, then you need to
>>>>>>>> add a new ->calibrate() method which, likely, will only be used by you
>>>>>>>> ;-)
>>>>>>>
>>>>>>> so you mean, the xhci should itself call phy_init() at a time suitable,
>>>>>>> so that ->calibrate() is not required at all ?
> 
> but wait, dwc3 does a phy_init() already, then how xhci will be able to
> do that again. We can't do phy_init() multiple times right ?

right. I think we should split and do phy ops separately for dwc3 host and gadget?

Thanks
Kishon

[PATCH v6 4/4] phy: exynos5-usbdrd: Calibrate LOS levels for exynos5420/5800

[Felipe Balbi]

Hi,

On Thu, Sep 11, 2014 at 09:10:21PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Wednesday 10 September 2014 01:26 PM, Vivek Gautam wrote:
> > On Wed, Sep 10, 2014 at 10:53 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> >> On Wed, Sep 10, 2014 at 10:23 AM, Felipe Balbi <balbi@ti.com> wrote:
> >>> On Wed, Sep 10, 2014 at 09:09:57AM +0530, Vivek Gautam wrote:
> >>>> On Wed, Sep 10, 2014 at 9:07 AM, Vivek Gautam <gautam.vivek@samsung.com> wrote:
> >>>>> adding Julius here,
> >>>>
> >>>> i think i had missed adding Julius for this entire series :-(
> >>>> I should be more careful with the CC list in future.
> >>>> Added his chromium id, since that seems to be more active.
> >>>>
> >>>>>
> >>>>>
> >>>>> On Tue, Sep 9, 2014 at 8:12 PM, Felipe Balbi <balbi@ti.com> wrote:
> >>>>>> On Tue, Sep 09, 2014 at 07:19:50AM +0530, Vivek Gautam wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>>
> >>>>>>> On Mon, Sep 8, 2014 at 7:14 PM, Felipe Balbi <balbi@ti.com> wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> On Mon, Sep 08, 2014 at 09:53:09AM +0530, Vivek Gautam wrote:
> >>>>>>>>> On Fri, Sep 5, 2014 at 11:26 PM, Felipe Balbi <balbi@ti.com> wrote:
> >>>>>>>>>> On Thu, Sep 04, 2014 at 12:01:19PM +0530, Vivek Gautam wrote:
> >>>>>>>>>>>> Don't we have phy_power_on()
> >>>>>>>>>>>> for that ? It looks like you could just as well do this from
> >>>>>>>>>>>> phy_power_on() ?
> >>>>>>>>>>>
> >>>>>>>>>>> No, unfortunately keeping these calibration settings in phy_power_on()
> >>>>>>>>>>> doesn't help, since we need to do this after XHCI reset has happened.
> >>>>>>>>>>
> >>>>>>>>>> teach xHCI about PHYs ?
> >>>>>>>>>
> >>>>>>>>> sorry i couldn't understand you here.
> >>>>>>>>> Aren't we trying to do the same with Heikki's patch about dwc3 :
> >>>>>>>>> [PATCH 6/6] usb: dwc3: host: convey the PHYs to xhci
> >>>>>>>>>
> >>>>>>>>> and the 2nd patch in this series :
> >>>>>>>>> [PATCH v6 2/4] usb: host: xhci-plat: Get PHYs for xhci's hcds
> >>>>>>>>>
> >>>>>>>>> Is there something else that is expected ?
> >>>>>>>>
> >>>>>>>> right, use that to call phy_init() at the right time, then you need to
> >>>>>>>> add a new ->calibrate() method which, likely, will only be used by you
> >>>>>>>> ;-)
> >>>>>>>
> >>>>>>> so you mean, the xhci should itself call phy_init() at a time suitable,
> >>>>>>> so that ->calibrate() is not required at all ?
> > 
> > but wait, dwc3 does a phy_init() already, then how xhci will be able to
> > do that again. We can't do phy_init() multiple times right ?
> 
> right. I think we should split and do phy ops separately for dwc3 host
> and gadget?

no, don't do that. We need a better way of handling this. As of now we
don't support dual-role, so we can just reinitialize the PHY once we
reach xhci.

Once we start supporting dual-role, we will need more inteligence in the
algorithm.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140911/95110e05/attachment-0001.sig>