phy: ti-pipe3: Add set_mode callback to configure usb3 phy as pcie phy

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +39 -8

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;
+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {
+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)
+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {
 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 
 	return 0;

drivers/phy/ti/phy-ti-pipe3.c +5 -4

+
+	return 0;
+}
+
 static const struct phy_ops ops = {
 	.init		= ti_pipe3_init,
 	.exit		= ti_pipe3_exit,
 	.power_on	= ti_pipe3_power_on,
 	.power_off	= ti_pipe3_power_off,
+	.set_mode	= ti_pipe3_set_mode,
 	.owner		= THIS_MODULE,
 };
 

@@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)
 {
 	struct resource *res;
 	struct device *dev = phy->dev;
-	struct device_node *node = dev->of_node;
 	struct platform_device *pdev = to_platform_device(dev);
 
-	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))
-		return 0;
-
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
 					   "phy_rx");
 	phy->phy_rx = devm_ioremap_resource(dev, res);

Hi Kishon,

Quoted 5 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Quoted 2 lines

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

What is the use case of this? At what point and who decides the phy_set_mode() to be called with USB vs PCIe?

Quoted 115 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +39 -8

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;
+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {
+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)
+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {
 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 11 lines

drivers/phy/ti/phy-ti-pipe3.c +9 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;
+
+	phy->mode = mode;

You are only saving the mode. But not really switching modes here. How is this intended to work?

Hi Roger,

Quoted 8 lines · Roger Quadros

Hi Kishon,

Quoted 5 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Quoted 5 lines

Quoted 2 lines

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

What is the use case of this? At what point and who decides the phy_set_mode() to be called with USB vs PCIe?

The PHY by default is configured to be used with USB controller (That's why it has a compatible of ti,omap-usb3). However there is a special case where this PHY has to be used PCIe controller. So the set_mode() will help the PHY driver to configure the USB PHY to be used with PCIe controller.

This is not a shared PHY, so the PHY as such can be used only with USB or PCIe and is determined at designtime.

Quoted 115 lines

Quoted 115 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +39 -8

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;
+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {
+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)
+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {
 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 14 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 11 lines

drivers/phy/ti/phy-ti-pipe3.c +9 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;
+
+	phy->mode = mode;

You are only saving the mode. But not really switching modes here. How is this intended to work?

The PHY doesn't have a specific mode. Rather it is a set of configurations required to operate in a particular mode.

This mode value will be used in other init, power_on callback functions.

Thanks
Kishon

Kishon,

Quoted 22 lines · Kishon Vijay Abraham I

Hi Roger,

Quoted 8 lines · Roger Quadros

Hi Kishon,

Quoted 5 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Quoted 5 lines

Quoted 2 lines

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

What is the use case of this? At what point and who decides the phy_set_mode() to be called with USB vs PCIe?

The PHY by default is configured to be used with USB controller (That's why it has a compatible of ti,omap-usb3). However there is a special case where this PHY has to be used PCIe controller. So the set_mode() will help the PHY driver to configure the USB PHY to be used with PCIe controller.

Quoted 2 lines

This is not a shared PHY, so the PHY as such can be used only with USB or PCIe and is determined at designtime

If type is defined at design time then the design using the PHY for PCIe shouldn't be using "ti,omap-usb3" compatible. Right?

Maybe we should fix the DT? Which board/PHY is this about?

cheers,
-roger

Quoted 138 lines

Quoted 115 lines

Quoted 115 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +39 -8

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;
+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {
+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)
+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {
 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 14 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 11 lines

drivers/phy/ti/phy-ti-pipe3.c +9 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;
+
+	phy->mode = mode;

You are only saving the mode. But not really switching modes here. How is this intended to work?

The PHY doesn't have a specific mode. Rather it is a set of configurations required to operate in a particular mode.

This mode value will be used in other init, power_on callback functions.

Thanks
Kishon

Hi Roger,

Quoted 25 lines · Roger Quadros

Kishon,

Quoted 22 lines · Kishon Vijay Abraham I

Hi Roger,

Quoted 8 lines · Roger Quadros

Hi Kishon,

Quoted 5 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Quoted 5 lines

Quoted 2 lines

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

What is the use case of this? At what point and who decides the phy_set_mode() to be called with USB vs PCIe?

The PHY by default is configured to be used with USB controller (That's why it has a compatible of ti,omap-usb3). However there is a special case where this PHY has to be used PCIe controller. So the set_mode() will help the PHY driver to configure the USB PHY to be used with PCIe controller.

Quoted 5 lines

Quoted 2 lines

This is not a shared PHY, so the PHY as such can be used only with USB or PCIe and is determined at designtime

If type is defined at design time then the design using the PHY for PCIe shouldn't be using "ti,omap-usb3" compatible. Right?

The second instance of PIPE3 PHY in dra72x SoC can be used either with USB3 or as a second lane of the 1st PCIe instance. How it is connected is based on board design. DRA72x EVM will need board modification for the PHY to be used with PCIe (by default it is connected to USB). AM571x IDK doesn't have a USB3 port so the PIPE3 PHY is used with PCIe (USB2 also doesn't work with this configuration but that is a different HW issue).

I'm not sure if we should change the compatible in board dts file as it is based on board design. Previously I added a dt property to indicate the PHY should be configured for PCIe but Rob Herring suggested to use set_mode [1].

Thanks
Kishon

[1] -> https://lore.kernel.org/patchwork/patch/865398/

Quoted 145 lines

Maybe we should fix the DT? Which board/PHY is this about?

cheers,
-roger

Quoted 138 lines

Quoted 115 lines

Quoted 115 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +39 -8

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;
+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {
+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)
+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {
 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 14 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 11 lines

drivers/phy/ti/phy-ti-pipe3.c +9 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;
+
+	phy->mode = mode;

You are only saving the mode. But not really switching modes here. How is this intended to work?

The PHY doesn't have a specific mode. Rather it is a set of configurations required to operate in a particular mode.

This mode value will be used in other init, power_on callback functions.

Thanks
Kishon

+Rob,

Quoted 40 lines · Kishon Vijay Abraham I

Hi Roger,

Quoted 25 lines · Roger Quadros

Kishon,

Quoted 22 lines · Kishon Vijay Abraham I

Hi Roger,

Quoted 8 lines · Roger Quadros

Hi Kishon,

Quoted 5 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Quoted 5 lines

Quoted 2 lines

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

What is the use case of this? At what point and who decides the phy_set_mode() to be called with USB vs PCIe?

The PHY by default is configured to be used with USB controller (That's why it has a compatible of ti,omap-usb3). However there is a special case where this PHY has to be used PCIe controller. So the set_mode() will help the PHY driver to configure the USB PHY to be used with PCIe controller.

Quoted 5 lines

Quoted 2 lines

This is not a shared PHY, so the PHY as such can be used only with USB or PCIe and is determined at designtime

If type is defined at design time then the design using the PHY for PCIe shouldn't be using "ti,omap-usb3" compatible. Right?

The second instance of PIPE3 PHY in dra72x SoC can be used either with USB3 or as a second lane of the 1st PCIe instance. How it is connected is based on board design. DRA72x EVM will need board modification for the PHY to be used with PCIe (by default it is connected to USB). AM571x IDK doesn't have a USB3 port so the PIPE3 PHY is used with PCIe (USB2 also doesn't work with this configuration but that is a different HW issue).

Quoted 3 lines

I'm not sure if we should change the compatible in board dts file as it is based on board design. Previously I added a dt property to indicate the PHY should be configured for PCIe but Rob Herring suggested to use set_mode [1].

That is because you tried to add a new property "ti,configure-as-pcie" which does seem unnecessary.

Wouldn't it work if you just use "ti,phy-pipe3-pcie" in the relevant device tree?

cheers,
-roger

Quoted 150 lines

Thanks
Kishon

[1] -> https://lore.kernel.org/patchwork/patch/865398/

Quoted 145 lines

Maybe we should fix the DT? Which board/PHY is this about?

cheers,
-roger

Quoted 138 lines

Quoted 115 lines

Quoted 115 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +39 -8

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;
+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {
+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)
+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {
 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 14 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 11 lines

drivers/phy/ti/phy-ti-pipe3.c +9 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;
+
+	phy->mode = mode;

You are only saving the mode. But not really switching modes here. How is this intended to work?

The PHY doesn't have a specific mode. Rather it is a set of configurations required to operate in a particular mode.

This mode value will be used in other init, power_on callback functions.

Thanks
Kishon

Hi,

Quoted 47 lines · Roger Quadros

+Rob,

Quoted 43 lines · Kishon Vijay Abraham I

Hi Roger,

Quoted 25 lines · Roger Quadros

Kishon,

Quoted 22 lines · Kishon Vijay Abraham I

Hi Roger,

Quoted 8 lines · Roger Quadros

Hi Kishon,

Quoted 5 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Quoted 5 lines

Quoted 2 lines

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

What is the use case of this? At what point and who decides the phy_set_mode() to be called with USB vs PCIe?

The PHY by default is configured to be used with USB controller (That's why it has a compatible of ti,omap-usb3). However there is a special case where this PHY has to be used PCIe controller. So the set_mode() will help the PHY driver to configure the USB PHY to be used with PCIe controller.

Quoted 5 lines

Quoted 2 lines

This is not a shared PHY, so the PHY as such can be used only with USB or PCIe and is determined at designtime

If type is defined at design time then the design using the PHY for PCIe shouldn't be using "ti,omap-usb3" compatible. Right?

The second instance of PIPE3 PHY in dra72x SoC can be used either with USB3 or as a second lane of the 1st PCIe instance. How it is connected is based on board design. DRA72x EVM will need board modification for the PHY to be used with PCIe (by default it is connected to USB). AM571x IDK doesn't have a USB3 port so the PIPE3 PHY is used with PCIe (USB2 also doesn't work with this configuration but that is a different HW issue).

I'm not sure if we should change the compatible in board dts file as it is based on board design. Previously I added a dt property to indicate the PHY should be configured for PCIe but Rob Herring suggested to use set_mode [1].

Quoted 4 lines

That is because you tried to add a new property "ti,configure-as-pcie" which does seem unnecessary.

Wouldn't it work if you just use "ti,phy-pipe3-pcie" in the relevant device tree?

No, it won't. The configuration required for a PCIe PHY is different from a USB PHY to be used for PCIe.

Thanks
Kishon

Quoted 155 lines

cheers,
-roger

Quoted 150 lines

Thanks
Kishon

[1] -> https://lore.kernel.org/patchwork/patch/865398/

Quoted 145 lines

Maybe we should fix the DT? Which board/PHY is this about?

cheers,
-roger

Quoted 138 lines

Quoted 115 lines

Quoted 115 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +39 -8

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;
+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {
+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)
+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 
 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {
 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 14 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 11 lines

drivers/phy/ti/phy-ti-pipe3.c +9 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;
+
+	phy->mode = mode;

You are only saving the mode. But not really switching modes here. How is this intended to work?

The PHY doesn't have a specific mode. Rather it is a set of configurations required to operate in a particular mode.

This mode value will be used in other init, power_on callback functions.

Thanks
Kishon

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +9 -0

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 
 static struct pipe3_dpll_map dpll_map_usb[] = {

drivers/phy/ti/phy-ti-pipe3.c +3 -4

+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

drivers/phy/ti/phy-ti-pipe3.c +10 -0

+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 
+	return 0;
+}

drivers/phy/ti/phy-ti-pipe3.c +0 -0

 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 
 	return 0;

drivers/phy/ti/phy-ti-pipe3.c +5 -0

+
+	return 0;
+}
+
 static const struct phy_ops ops = {
 	.init		= ti_pipe3_init,
 	.exit		= ti_pipe3_exit,
 	.power_on	= ti_pipe3_power_on,
 	.power_off	= ti_pipe3_power_off,
+	.set_mode	= ti_pipe3_set_mode,
 	.owner		= THIS_MODULE,
 };
 

Kishon,

Quoted 7 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

How about rewording this to,

"On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed. Provide a way for the PHY user to choose the appropriate mode via the .set_mode hook"

More comments below.

Quoted 39 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +9 -0

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_power_on(struct phy *x) · line 247

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -1

 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

Is this required only for the USB3 PHY being used as PCIe lane or can it be done for the PCIe PHY as well?

I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that currently.

Maybe this is safer?

	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

Quoted 24 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -4

+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 366

Quoted 4 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -2

 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {

How about

	else if (of_device_is_compatible(node, "ti,phy-usb3")) {

		/* USB3 PHY being used as PCIe Lane */

Quoted 10 lines

drivers/phy/ti/phy-ti-pipe3.c +8 -0

+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 388

Quoted 12 lines

drivers/phy/ti/phy-ti-pipe3.c +12 -0

+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)

Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE. Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +2 -0

+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 429

Quoted 3 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -1

 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {

But this might be a USB3 PHY, which has DPLL.

Quoted 7 lines

drivers/phy/ti/phy-ti-pipe3.c +0 -0

 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 
 	return 0;

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 10 lines

drivers/phy/ti/phy-ti-pipe3.c +9 -0

 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;
+
+	phy->mode = mode;

We need to do a sanity check here and prevent unsupported modes being set.

e.g. we don't want PCIe mode being set on omap-usb3 PHY or SATA PHY.

Which makes me wonder, if all PHYs with compatible "ti,phy-usb3" don't support PCIe mode then we might need to add a new compatible for USB3 PHY's that do support PCIe mode?

Or maybe a DT-property of supported PHY modes?

Quoted 21 lines

drivers/phy/ti/phy-ti-pipe3.c +5 -1

+
+	return 0;
+}
+
 static const struct phy_ops ops = {
 	.init		= ti_pipe3_init,
 	.exit		= ti_pipe3_exit,
 	.power_on	= ti_pipe3_power_on,
 	.power_off	= ti_pipe3_power_off,
+	.set_mode	= ti_pipe3_set_mode,
 	.owner		= THIS_MODULE,
 };
 

@@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)
 {
 	struct resource *res;
 	struct device *dev = phy->dev;
-	struct device_node *node = dev->of_node;
 	struct platform_device *pdev = to_platform_device(dev);
 

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy) · line 597

Quoted 3 lines

drivers/phy/ti/phy-ti-pipe3.c +0 -3

-	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))
-		return 0;
-

Do all USB3 and SATA PHYs have "phy_rx" and "phy_tx" regions? If not it will break them.

Roger,

Quoted 11 lines · Roger Quadros

Kishon,

Quoted 7 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

Quoted 7 lines

How about rewording this to,

"On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed. Provide a way for the PHY user to choose the appropriate mode via the .set_mode hook"

hmm okay

Quoted 42 lines

More comments below.

Quoted 39 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +9 -0

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 

Quoted 12 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_power_on(struct phy *x) · line 247

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -1

 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

Is this required only for the USB3 PHY being used as PCIe lane or can it be done for the PCIe PHY as well?

This setting was given only for USB3 PHY. I did check PCIe PHY (1-lane) to see if this is causing issues before posting the patch.

Thinking again, it might be safer to just use this setting for USB3 PHY.

Quoted 8 lines

I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that currently.

Maybe this is safer?

	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

yeah..

Quoted 31 lines

Quoted 22 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -4

+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 366

Quoted 6 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -2

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {

How about

Quoted 1 line

	else if (of_device_is_compatible(node, "ti,phy-usb3")) {

This check is implicit no?

Quoted 12 lines

		/* USB3 PHY being used as PCIe Lane */

Quoted 10 lines

drivers/phy/ti/phy-ti-pipe3.c +8 -0

+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 

Quoted 15 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 388

Quoted 12 lines

drivers/phy/ti/phy-ti-pipe3.c +12 -0

+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)

Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE. Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

The set_mode callback checks if the phy->mode is INVALID before setting the mode. The phy_set_mode in the PCIe consumer driver will return error if it is already set. The phy_set_mode will be called for both the lanes by the PCIe driver.

Quoted 9 lines

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +2 -0

+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 

Quoted 5 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 429

Quoted 3 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -1

 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {

But this might be a USB3 PHY, which has DPLL.

hmm. I have to check this. We should also then reset PLL_IDLE bit in phy_init.

Quoted 6 lines

Quoted 6 lines

drivers/phy/ti/phy-ti-pipe3.c +0 -0

 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 469

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +7 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;

Quoted 5 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 471

Quoted 2 lines

drivers/phy/ti/phy-ti-pipe3.c +2 -0

+
+	phy->mode = mode;

We need to do a sanity check here and prevent unsupported modes being set.

Quoted 7 lines

e.g. we don't want PCIe mode being set on omap-usb3 PHY or SATA PHY.

Which makes me wonder, if all PHYs with compatible "ti,phy-usb3" don't support PCIe mode then we might need to add a new compatible for USB3 PHY's that do support PCIe mode?

Or maybe a DT-property of supported PHY modes?

hmm yeah, makes sense.

Quoted 21 lines

Quoted 21 lines

drivers/phy/ti/phy-ti-pipe3.c +5 -1

+
+	return 0;
+}
+
 static const struct phy_ops ops = {
 	.init		= ti_pipe3_init,
 	.exit		= ti_pipe3_exit,
 	.power_on	= ti_pipe3_power_on,
 	.power_off	= ti_pipe3_power_off,
+	.set_mode	= ti_pipe3_set_mode,
 	.owner		= THIS_MODULE,
 };
 

@@ -589,12 +634,8 @@ static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy)
 {
 	struct resource *res;
 	struct device *dev = phy->dev;
-	struct device_node *node = dev->of_node;
 	struct platform_device *pdev = to_platform_device(dev);
 

Quoted 6 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_get_tx_rx_base(struct ti_pipe3 *phy) · line 597

Quoted 3 lines

drivers/phy/ti/phy-ti-pipe3.c +0 -3

-	if (!of_device_is_compatible(node, "ti,phy-pipe3-pcie"))
-		return 0;
-

Do all USB3 and SATA PHYs have "phy_rx" and "phy_tx" regions? If not it will break them.

yes they have.

Quoted 123 lines · Kishon Vijay Abraham I

Roger,

Quoted 11 lines · Roger Quadros

Kishon,

Quoted 7 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

Quoted 7 lines

How about rewording this to,

"On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed. Provide a way for the PHY user to choose the appropriate mode via the .set_mode hook"

hmm okay

Quoted 42 lines

More comments below.

Quoted 39 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +9 -0

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 

Quoted 12 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_power_on(struct phy *x) · line 247

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -1

 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

Is this required only for the USB3 PHY being used as PCIe lane or can it be done for the PCIe PHY as well?

This setting was given only for USB3 PHY. I did check PCIe PHY (1-lane) to see if this is causing issues before posting the patch.

Thinking again, it might be safer to just use this setting for USB3 PHY.

Quoted 8 lines

I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that currently.

Maybe this is safer?

	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

yeah..

Quoted 31 lines

Quoted 22 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -4

+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 366

Quoted 6 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -2

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {

How about

Quoted 3 lines

Quoted 1 line

	else if (of_device_is_compatible(node, "ti,phy-usb3")) {

This check is implicit no?

For now yes. But phy_set_mode can be called for SATA PHY as well and that might need different settings than USB3 PHY?

Quoted 29 lines

Quoted 13 lines

		/* USB3 PHY being used as PCIe Lane */

Quoted 10 lines

drivers/phy/ti/phy-ti-pipe3.c +8 -0

+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 

Quoted 15 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 388

Quoted 12 lines

drivers/phy/ti/phy-ti-pipe3.c +12 -0

+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)

Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE. Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

Quoted 3 lines

The set_mode callback checks if the phy->mode is INVALID before setting the mode. The phy_set_mode in the PCIe consumer driver will return error if it is already set. The phy_set_mode will be called for both the lanes by the PCIe driver.

see below.

Quoted 22 lines

Quoted 9 lines

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +2 -0

+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 

Quoted 5 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 429

Quoted 3 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -1

 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {

But this might be a USB3 PHY, which has DPLL.

hmm. I have to check this. We should also then reset PLL_IDLE bit in phy_init.

Quoted 6 lines

Quoted 6 lines

drivers/phy/ti/phy-ti-pipe3.c +0 -0

 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 9 lines

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 469

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +7 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;

I think you are referring to this. Why is this necessary? This will restrict the PHY to one mode per boot. We don't want to impose that limitation.

Hi,

Quoted 126 lines · Roger Quadros

Quoted 123 lines · Kishon Vijay Abraham I

Roger,

Quoted 11 lines · Roger Quadros

Kishon,

Quoted 7 lines · Kishon Vijay Abraham I

DRA72 platform has the second instance of PHY shared between USB3 controller and PCIe controller with default as USB3 controller. Since it is used with USB3 controller by default, it uses the compatible specific to USB (ti,omap-usb3).

Populate set_mode callback so that the USB3 PHY can be configured to be used with PCIe controller.

Quoted 7 lines

How about rewording this to,

"On DRA72x SoCs, the USB3 PHY can be used either as USB Super-Speed lane or as PCIe Lane (i.e. second lane for PCIe_SS1 in 2 lane mode or single lane for PCIe_SS2). The default mode for the USB3 PHY is USB Super-Speed. Provide a way for the PHY user to choose the appropriate mode via the .set_mode hook"

hmm okay

Quoted 42 lines

More comments below.

Quoted 39 lines

Signed-off-by Kishon Vijay Abraham I <[email protected]>

---
 drivers/phy/ti/phy-ti-pipe3.c | 66 ++++++++++++++++++++++++++++-------
 1 file changed, 54 insertions(+), 12 deletions(-)

drivers/phy/ti/phy-ti-pipe3.c +9 -0

index 68ce4a082b9b..8c98f366416d 100644
@@ -56,6 +56,12 @@
 
 #define SATA_PLL_SOFT_RESET	BIT(18)
 
+#define PHY_RX_ANA_PRGRAMMABILITY_REG	0xC
+#define MEM_EN_PLLBYP			BIT(7)
+
+#define PHY_TX_TEST_CONFIG		0x2C
+#define MEM_ENTESTCLK			BIT(31)
+
 #define PIPE3_PHY_PWRCTL_CLK_CMD_MASK	0x003FC000
 #define PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT	14
 

@@ -110,6 +116,8 @@
 #define PLL_IDLE_TIME	100	/* in milliseconds */
 #define PLL_LOCK_TIME	100	/* in milliseconds */
 
+#define PIPE3_PHY_DISABLE_SYNC_POWER	BIT(4)
+
 struct pipe3_dpll_params {
 	u16	m;
 	u8	n;

@@ -141,6 +149,7 @@ struct ti_pipe3 {
 	unsigned int		power_reg; /* power reg. index within syscon */
 	unsigned int		pcie_pcs_reg; /* pcs reg. index in syscon */
 	bool			sata_refclk_enabled;
+	u32			mode;
 };
 

Quoted 12 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_power_on(struct phy *x) · line 247

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -1

 static struct pipe3_dpll_map dpll_map_usb[] = {

@@ -233,7 +242,10 @@ static int ti_pipe3_power_on(struct phy *x)
 	rate = rate / 1000000;
 	mask = OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_CMD_MASK |
 		  OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_MASK;
-	val = PIPE3_PHY_TX_RX_POWERON << PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
+	val = PIPE3_PHY_TX_RX_POWERON;
+	if (phy->mode == PHY_MODE_PCIE)
+		val |= PIPE3_PHY_DISABLE_SYNC_POWER;

Is this required only for the USB3 PHY being used as PCIe lane or can it be done for the PCIe PHY as well?

This setting was given only for USB3 PHY. I did check PCIe PHY (1-lane) to see if this is causing issues before posting the patch.

Thinking again, it might be safer to just use this setting for USB3 PHY.

Quoted 8 lines

I ask this because phy->mode can be PHY_MODE_PCIE for both USB3 PHY and PCIe PHY and it might break PCIe PHY as we don't do PIPE3_PHY_DISABLE_SYNC_POWER for that currently.

Maybe this is safer?

	if (phy->mode == PHY_MODE_PCIE && of_device_is_compatible(node, "ti,phy-usb3"))

yeah..

Quoted 31 lines

Quoted 22 lines

drivers/phy/ti/phy-ti-pipe3.c +3 -4

+	val <<= PIPE3_PHY_PWRCTL_CLK_CMD_SHIFT;
 	val |= rate << OMAP_CTRL_PIPE3_PHY_PWRCTL_CLK_FREQ_SHIFT;
 
 	ret = regmap_update_bits(phy->phy_power_syscon, phy->power_reg,

@@ -328,13 +340,11 @@ static void ti_pipe3_calibrate(struct ti_pipe3 *phy)
 	ti_pipe3_writel(phy->phy_rx, PCIEPHYRX_EQUALIZER, val);
 }
 
-static int ti_pipe3_init(struct phy *x)
+static int ti_pipe3_pcie_init(struct ti_pipe3 *phy)
 {
-	struct ti_pipe3 *phy = phy_get_drvdata(x);
-	u32 val;
 	int ret = 0;
+	u32 val;
 
-	ti_pipe3_enable_clocks(phy);
 	/*
 	 * Set pcie_pcs register to 0x96 for proper functioning of phy
 	 * as recommended in AM572x TRM SPRUHZ6, section 18.5.2.2, table

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 366

Quoted 6 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -2

@@ -353,10 +363,31 @@ static int ti_pipe3_init(struct phy *x)
 			return ret;
 
 		ti_pipe3_calibrate(phy);
-
-		return 0;
+	} else {

How about

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

	else if (of_device_is_compatible(node, "ti,phy-usb3")) {

This check is implicit no?

For now yes. But phy_set_mode can be called for SATA PHY as well and that might need different settings than USB3 PHY?

This function will be invoked only for PCIe.

Quoted 57 lines

Quoted 29 lines

Quoted 13 lines

		/* USB3 PHY being used as PCIe Lane */

Quoted 10 lines

drivers/phy/ti/phy-ti-pipe3.c +8 -0

+		val = ti_pipe3_readl(phy->phy_rx,
+				     PHY_RX_ANA_PRGRAMMABILITY_REG);
+		val |= MEM_EN_PLLBYP;
+		ti_pipe3_writel(phy->phy_rx, PHY_RX_ANA_PRGRAMMABILITY_REG,
+				val);
+		val = ti_pipe3_readl(phy->phy_tx, PHY_TX_TEST_CONFIG);
+		val |= MEM_ENTESTCLK;
+		ti_pipe3_writel(phy->phy_tx, PHY_TX_TEST_CONFIG, val);
 	}
 

Quoted 15 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_init(struct phy *x) · line 388

Quoted 12 lines

drivers/phy/ti/phy-ti-pipe3.c +12 -0

+	return 0;
+}
+
+static int ti_pipe3_init(struct phy *x)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+	u32 val;
+	int ret = 0;
+
+	ti_pipe3_enable_clocks(phy);
+
+	if (phy->mode == PHY_MODE_PCIE)

Here you expect PHY "ti,phy-pipe3-pcie" to have mode = PHY_MODE_PCIE. Should we set the mode to PCI_MODE_PCIE explicitly it in probe?

Quoted 3 lines

The set_mode callback checks if the phy->mode is INVALID before setting the mode. The phy_set_mode in the PCIe consumer driver will return error if it is already set. The phy_set_mode will be called for both the lanes by the PCIe driver.

see below.

Quoted 22 lines

Quoted 9 lines

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +2 -0

+		return ti_pipe3_pcie_init(phy);
+
 	/* Bring it out of IDLE if it is IDLE */
 	val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 	if (val & PLL_IDLE) {

@@ -395,7 +426,7 @@ static int ti_pipe3_exit(struct phy *x)
 		return 0;
 

Quoted 5 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 429

Quoted 3 lines

drivers/phy/ti/phy-ti-pipe3.c +1 -1

 	/* PCIe doesn't have internal DPLL */
-	if (!of_device_is_compatible(phy->dev->of_node, "ti,phy-pipe3-pcie")) {
+	if (!(phy->mode == PHY_MODE_PCIE)) {

But this might be a USB3 PHY, which has DPLL.

hmm. I have to check this. We should also then reset PLL_IDLE bit in phy_init.

Quoted 6 lines

Quoted 6 lines

drivers/phy/ti/phy-ti-pipe3.c +0 -0

 		/* Put DPLL in IDLE mode */
 		val = ti_pipe3_readl(phy->pll_ctrl_base, PLL_CONFIGURATION2);
 		val |= PLL_IDLE;

@@ -429,11 +460,25 @@ static int ti_pipe3_exit(struct phy *x)
 

Quoted 13 lines

Quoted 9 lines

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c static int ti_pipe3_exit(struct phy *x) · line 469

Quoted 9 lines

drivers/phy/ti/phy-ti-pipe3.c +7 -0

 	return 0;
 }
+
+static int ti_pipe3_set_mode(struct phy *x, enum phy_mode mode, int submode)
+{
+	struct ti_pipe3 *phy = phy_get_drvdata(x);
+
+	if (phy->mode != PHY_MODE_INVALID)
+		return -EBUSY;

I think you are referring to this. Why is this necessary? This will restrict the PHY to one mode per boot. We don't want to impose that limitation.

Isn't that true? Our PHYs can work only in one mode right?

Maybe we can add a check like below but there too we can't set the PHY mode of USB PHY to USB in probe, for the PHY that is shared with PCIe.

if (phy->mode != PHY_MODE_INVALID) && (phy->mode != mode)
return -EBUSY.