[PATCH v2 0/4] phy: zynqmp: Fixes and debugfs support

[PATCH v2 0/4] phy: zynqmp: Fixes and debugfs support

[Sean Anderson]

This has a few fixes and cleanups cleaning up the driver. Additionally,
the last patch adds useful debugfs info.

Changes in v2:
- Enable reference clock correctly
- Expand the icm_matrix comment
- Move the logic for waiting on PLL lock to xpsgtr_wait_pll_lock
- Use debugfs_create_devm_seqfile

Sean Anderson (4):
  phy: zynqmp: Enable reference clock correctly
  phy: zynqmp: Store instance instead of type
  phy: zynqmp: Only wait for PLL lock "primary" instances
  phy: zynqmp: Add debugfs support

 drivers/phy/xilinx/phy-zynqmp.c | 197 ++++++++++++++++----------------
 1 file changed, 100 insertions(+), 97 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] phy: zynqmp: Enable reference clock correctly

[Sean Anderson]

Lanes can use other lanes' reference clocks, as determined by refclk.
Use refclk to determine the clock to enable/disable instead of always
using the lane's own reference clock. This ensures the clock selected in
xpsgtr_configure_pll is the one enabled.

For the other half of the equation, always program REF_CLK_SEL even when
we are selecting the lane's own clock. This ensures that Linux's idea of
the reference clock matches the hardware. We use the "local" clock mux
for this instead of going through the ref clock network.

Fixes: 25d700833513 ("phy: xilinx: phy-zynqmp: dynamic clock support for power-save")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- New

 drivers/phy/xilinx/phy-zynqmp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index f72c5257d712..5a434382356c 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -80,7 +80,8 @@
 
 /* Reference clock selection parameters */
 #define L0_Ln_REF_CLK_SEL(n)		(0x2860 + (n) * 4)
-#define L0_REF_CLK_SEL_MASK		0x8f
+#define L0_REF_CLK_LCL_SEL		BIT(7)
+#define L0_REF_CLK_SEL_MASK		0x9f
 
 /* Calibration digital logic parameters */
 #define L3_TM_CALIB_DIG19		0xec4c
@@ -349,11 +350,14 @@ static void xpsgtr_configure_pll(struct xpsgtr_phy *gtr_phy)
 		       PLL_FREQ_MASK, ssc->pll_ref_clk);
 
 	/* Enable lane clock sharing, if required */
-	if (gtr_phy->refclk != gtr_phy->lane) {
+	if (gtr_phy->refclk == gtr_phy->lane)
+		/* Lane3 Ref Clock Selection Register */
+		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy->lane),
+			       L0_REF_CLK_SEL_MASK, L0_REF_CLK_LCL_SEL);
+	else
 		/* Lane3 Ref Clock Selection Register */
 		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy->lane),
 			       L0_REF_CLK_SEL_MASK, 1 << gtr_phy->refclk);
-	}
 
 	/* SSC step size [7:0] */
 	xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_0_LSB,
@@ -573,7 +577,7 @@ static int xpsgtr_phy_init(struct phy *phy)
 	mutex_lock(&gtr_dev->gtr_mutex);
 
 	/* Configure and enable the clock when peripheral phy_init call */
-	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->lane]))
+	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
 		goto out;
 
 	/* Skip initialization if not required. */
@@ -625,7 +629,7 @@ static int xpsgtr_phy_exit(struct phy *phy)
 	gtr_phy->skip_phy_init = false;
 
 	/* Ensure that disable clock only, which configure for lane */
-	clk_disable_unprepare(gtr_dev->clk[gtr_phy->lane]);
+	clk_disable_unprepare(gtr_dev->clk[gtr_phy->refclk]);
 
 	return 0;
 }
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] phy: zynqmp: Store instance instead of type

[Sean Anderson]

The phy "type" is just the combination of protocol and instance, and is
never used apart from that. Store the instance directly, instead of
converting to a type first. No functional change intended.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- Expand the icm_matrix comment

 drivers/phy/xilinx/phy-zynqmp.c | 115 +++++++++-----------------------
 1 file changed, 31 insertions(+), 84 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index 5a434382356c..5a8f81bbeb8d 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -147,22 +147,6 @@
 /* Total number of controllers */
 #define CONTROLLERS_PER_LANE		5
 
-/* Protocol Type parameters */
-#define XPSGTR_TYPE_USB0		0  /* USB controller 0 */
-#define XPSGTR_TYPE_USB1		1  /* USB controller 1 */
-#define XPSGTR_TYPE_SATA_0		2  /* SATA controller lane 0 */
-#define XPSGTR_TYPE_SATA_1		3  /* SATA controller lane 1 */
-#define XPSGTR_TYPE_PCIE_0		4  /* PCIe controller lane 0 */
-#define XPSGTR_TYPE_PCIE_1		5  /* PCIe controller lane 1 */
-#define XPSGTR_TYPE_PCIE_2		6  /* PCIe controller lane 2 */
-#define XPSGTR_TYPE_PCIE_3		7  /* PCIe controller lane 3 */
-#define XPSGTR_TYPE_DP_0		8  /* Display Port controller lane 0 */
-#define XPSGTR_TYPE_DP_1		9  /* Display Port controller lane 1 */
-#define XPSGTR_TYPE_SGMII0		10 /* Ethernet SGMII controller 0 */
-#define XPSGTR_TYPE_SGMII1		11 /* Ethernet SGMII controller 1 */
-#define XPSGTR_TYPE_SGMII2		12 /* Ethernet SGMII controller 2 */
-#define XPSGTR_TYPE_SGMII3		13 /* Ethernet SGMII controller 3 */
-
 /* Timeout values */
 #define TIMEOUT_US			1000
 
@@ -185,7 +169,8 @@ struct xpsgtr_ssc {
 /**
  * struct xpsgtr_phy - representation of a lane
  * @phy: pointer to the kernel PHY device
- * @type: controller which uses this lane
+ * @instance: instance of the protocol type (such as the lane within a
+ *            protocol, or the USB/Ethernet controller)
  * @lane: lane number
  * @protocol: protocol in which the lane operates
  * @skip_phy_init: skip phy_init() if true
@@ -194,7 +179,7 @@ struct xpsgtr_ssc {
  */
 struct xpsgtr_phy {
 	struct phy *phy;
-	u8 type;
+	u8 instance;
 	u8 lane;
 	u8 protocol;
 	bool skip_phy_init;
@@ -331,8 +316,8 @@ static int xpsgtr_wait_pll_lock(struct phy *phy)
 
 	if (ret == -ETIMEDOUT)
 		dev_err(gtr_dev->dev,
-			"lane %u (type %u, protocol %u): PLL lock timeout\n",
-			gtr_phy->lane, gtr_phy->type, gtr_phy->protocol);
+			"lane %u (protocol %u, instance %u): PLL lock timeout\n",
+			gtr_phy->lane, gtr_phy->protocol, gtr_phy->instance);
 
 	return ret;
 }
@@ -647,8 +632,7 @@ static int xpsgtr_phy_power_on(struct phy *phy)
 	 * cumulating waits for both lanes. The user is expected to initialize
 	 * lane 0 last.
 	 */
-	if (gtr_phy->protocol != ICM_PROTOCOL_DP ||
-	    gtr_phy->type == XPSGTR_TYPE_DP_0)
+	if (gtr_phy->protocol != ICM_PROTOCOL_DP || !gtr_phy->instance)
 		ret = xpsgtr_wait_pll_lock(phy);
 
 	return ret;
@@ -678,73 +662,33 @@ static const struct phy_ops xpsgtr_phyops = {
  * OF Xlate Support
  */
 
-/* Set the lane type and protocol based on the PHY type and instance number. */
+/* Set the lane protocol and instance based on the PHY type and instance number. */
 static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
 				unsigned int phy_instance)
 {
 	unsigned int num_phy_types;
-	const int *phy_types;
 
 	switch (phy_type) {
-	case PHY_TYPE_SATA: {
-		static const int types[] = {
-			XPSGTR_TYPE_SATA_0,
-			XPSGTR_TYPE_SATA_1,
-		};
-
-		phy_types = types;
-		num_phy_types = ARRAY_SIZE(types);
+	case PHY_TYPE_SATA:
+		num_phy_types = 2;
 		gtr_phy->protocol = ICM_PROTOCOL_SATA;
 		break;
-	}
-	case PHY_TYPE_USB3: {
-		static const int types[] = {
-			XPSGTR_TYPE_USB0,
-			XPSGTR_TYPE_USB1,
-		};
-
-		phy_types = types;
-		num_phy_types = ARRAY_SIZE(types);
+	case PHY_TYPE_USB3:
+		num_phy_types = 2;
 		gtr_phy->protocol = ICM_PROTOCOL_USB;
 		break;
-	}
-	case PHY_TYPE_DP: {
-		static const int types[] = {
-			XPSGTR_TYPE_DP_0,
-			XPSGTR_TYPE_DP_1,
-		};
-
-		phy_types = types;
-		num_phy_types = ARRAY_SIZE(types);
+	case PHY_TYPE_DP:
+		num_phy_types = 2;
 		gtr_phy->protocol = ICM_PROTOCOL_DP;
 		break;
-	}
-	case PHY_TYPE_PCIE: {
-		static const int types[] = {
-			XPSGTR_TYPE_PCIE_0,
-			XPSGTR_TYPE_PCIE_1,
-			XPSGTR_TYPE_PCIE_2,
-			XPSGTR_TYPE_PCIE_3,
-		};
-
-		phy_types = types;
-		num_phy_types = ARRAY_SIZE(types);
+	case PHY_TYPE_PCIE:
+		num_phy_types = 4;
 		gtr_phy->protocol = ICM_PROTOCOL_PCIE;
 		break;
-	}
-	case PHY_TYPE_SGMII: {
-		static const int types[] = {
-			XPSGTR_TYPE_SGMII0,
-			XPSGTR_TYPE_SGMII1,
-			XPSGTR_TYPE_SGMII2,
-			XPSGTR_TYPE_SGMII3,
-		};
-
-		phy_types = types;
-		num_phy_types = ARRAY_SIZE(types);
+	case PHY_TYPE_SGMII:
+		num_phy_types = 4;
 		gtr_phy->protocol = ICM_PROTOCOL_SGMII;
 		break;
-	}
 	default:
 		return -EINVAL;
 	}
@@ -752,22 +696,25 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
 	if (phy_instance >= num_phy_types)
 		return -EINVAL;
 
-	gtr_phy->type = phy_types[phy_instance];
+	gtr_phy->instance = phy_instance;
 	return 0;
 }
 
 /*
- * Valid combinations of controllers and lanes (Interconnect Matrix).
+ * Valid combinations of controllers and lanes (Interconnect Matrix). Each
+ * "instance" represents one controller for a lane. For PCIe and DP, the
+ * "instance" is the logical lane in the link. For SATA, USB, and SGMII,
+ * the instance is the index of the controller.
+ *
+ * This information is only used to validate the devicetree reference, and is
+ * not used when programming the hardware.
  */
 static const unsigned int icm_matrix[NUM_LANES][CONTROLLERS_PER_LANE] = {
-	{ XPSGTR_TYPE_PCIE_0, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
-		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII0 },
-	{ XPSGTR_TYPE_PCIE_1, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB0,
-		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII1 },
-	{ XPSGTR_TYPE_PCIE_2, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
-		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII2 },
-	{ XPSGTR_TYPE_PCIE_3, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB1,
-		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII3 }
+	/* PCIe, SATA, USB, DP, SGMII */
+	{ 0, 0, 0, 1, 0 }, /* Lane 0 */
+	{ 1, 1, 0, 0, 1 }, /* Lane 1 */
+	{ 2, 0, 0, 1, 2 }, /* Lane 2 */
+	{ 3, 1, 1, 0, 3 }, /* Lane 3 */
 };
 
 /* Translate OF phandle and args to PHY instance. */
@@ -822,7 +769,7 @@ static struct phy *xpsgtr_xlate(struct device *dev,
 	 * is allowed to operate on the lane.
 	 */
 	for (i = 0; i < CONTROLLERS_PER_LANE; i++) {
-		if (icm_matrix[phy_lane][i] == gtr_phy->type)
+		if (icm_matrix[phy_lane][i] == gtr_phy->instance)
 			return gtr_phy->phy;
 	}
 
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] phy: zynqmp: Only wait for PLL lock "primary" instances

[Sean Anderson]

For PCIe and DisplayPort, the phy instance represents the controller's
logical lane. Wait for the instance 0 phy's PLL to lock as other
instances will never lock. We do this in xpsgtr_wait_pll_lock so callers
don't have to determine the correct lane themselves.

The original comment is wrong about cumulative wait times. Since we are
just polling a bit, all subsequent waiters will finish immediately.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- Move the logic for waiting on PLL lock to xpsgtr_wait_pll_lock

 drivers/phy/xilinx/phy-zynqmp.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index 5a8f81bbeb8d..b86fe2a249a8 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -294,10 +294,30 @@ static int xpsgtr_wait_pll_lock(struct phy *phy)
 	struct xpsgtr_phy *gtr_phy = phy_get_drvdata(phy);
 	struct xpsgtr_dev *gtr_dev = gtr_phy->dev;
 	unsigned int timeout = TIMEOUT_US;
+	u8 protocol = gtr_phy->protocol;
 	int ret;
 
 	dev_dbg(gtr_dev->dev, "Waiting for PLL lock\n");
 
+	/*
+	 * For DP and PCIe, only the instance 0 PLL is used. Switch to that phy
+	 * so we wait on the right PLL.
+	 */
+	if ((protocol == ICM_PROTOCOL_DP || protocol == ICM_PROTOCOL_PCIE) &&
+	    gtr_phy->instance) {
+		int i;
+
+		for (i = 0; i < NUM_LANES; i++) {
+			gtr_phy = &gtr_dev->phys[i];
+
+			if (gtr_phy->protocol == protocol && !gtr_phy->instance)
+				goto got_phy;
+		}
+
+		return -EBUSY;
+	}
+
+got_phy:
 	while (1) {
 		u32 reg = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1);
 
@@ -627,15 +647,7 @@ static int xpsgtr_phy_power_on(struct phy *phy)
 	/* Skip initialization if not required. */
 	if (!xpsgtr_phy_init_required(gtr_phy))
 		return ret;
-	/*
-	 * Wait for the PLL to lock. For DP, only wait on DP0 to avoid
-	 * cumulating waits for both lanes. The user is expected to initialize
-	 * lane 0 last.
-	 */
-	if (gtr_phy->protocol != ICM_PROTOCOL_DP || !gtr_phy->instance)
-		ret = xpsgtr_wait_pll_lock(phy);
-
-	return ret;
+	return xpsgtr_wait_pll_lock(phy);
 }
 
 static int xpsgtr_phy_configure(struct phy *phy, union phy_configure_opts *opts)
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] phy: zynqmp: Add debugfs support

[Sean Anderson]

Add support for printing some basic status information to debugfs. This
is helpful when debugging phy consumers to make sure they are configuring
the phy appropriately.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

Changes in v2:
- Use debugfs_create_devm_seqfile

 drivers/phy/xilinx/phy-zynqmp.c | 40 +++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index b86fe2a249a8..2520c75a4a77 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -13,6 +13,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -123,6 +124,15 @@
 #define ICM_PROTOCOL_DP			0x4
 #define ICM_PROTOCOL_SGMII		0x5
 
+static const char *const xpsgtr_icm_str[] = {
+	[ICM_PROTOCOL_PD] = "powered down",
+	[ICM_PROTOCOL_PCIE] = "PCIe",
+	[ICM_PROTOCOL_SATA] = "SATA",
+	[ICM_PROTOCOL_USB] = "USB",
+	[ICM_PROTOCOL_DP] = "DisplayPort",
+	[ICM_PROTOCOL_SGMII] = "SGMII",
+};
+
 /* Test Mode common reset control  parameters */
 #define TM_CMN_RST			0x10018
 #define TM_CMN_RST_EN			0x1
@@ -788,6 +798,34 @@ static struct phy *xpsgtr_xlate(struct device *dev,
 	return ERR_PTR(-EINVAL);
 }
 
+/*
+ * DebugFS
+ */
+
+static int xpsgtr_status_read(struct seq_file *seq, void *data)
+{
+	struct device *dev = seq->private;
+	struct xpsgtr_phy *gtr_phy = dev_get_drvdata(dev);
+	struct clk *clk;
+	u32 pll_status;
+
+	mutex_lock(&gtr_phy->phy->mutex);
+	pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1);
+	clk = gtr_phy->dev->clk[gtr_phy->refclk];
+
+	seq_printf(seq, "Lane:            %u\n", gtr_phy->lane);
+	seq_printf(seq, "Protocol:        %s\n",
+		   xpsgtr_icm_str[gtr_phy->protocol]);
+	seq_printf(seq, "Instance:        %u\n", gtr_phy->instance);
+	seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk);
+	seq_printf(seq, "Reference rate:  %lu\n", clk_get_rate(clk));
+	seq_printf(seq, "PLL locked:      %s\n",
+		   pll_status & PLL_STATUS_LOCKED ? "yes" : "no");
+
+	mutex_unlock(&gtr_phy->phy->mutex);
+	return 0;
+}
+
 /*
  * Power Management
  */
@@ -937,6 +975,8 @@ static int xpsgtr_probe(struct platform_device *pdev)
 
 		gtr_phy->phy = phy;
 		phy_set_drvdata(phy, gtr_phy);
+		debugfs_create_devm_seqfile(&phy->dev, "status", phy->debugfs,
+					    xpsgtr_status_read);
 	}
 
 	/* Register the PHY provider. */
-- 
2.35.1.1320.gc452695387.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2 4/4] phy: zynqmp: Add debugfs support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Monday, May 6, 2024 10:31 PM
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
> phy@lists.infradead.org
> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
> <sean.anderson@linux.dev>
> Subject: [PATCH v2 4/4] phy: zynqmp: Add debugfs support
> 
> Add support for printing some basic status information to debugfs. This
> is helpful when debugging phy consumers to make sure they are configuring
> the phy appropriately.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v2:
> - Use debugfs_create_devm_seqfile
> 
>  drivers/phy/xilinx/phy-zynqmp.c | 40
> +++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
> zynqmp.c
> index b86fe2a249a8..2520c75a4a77 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -13,6 +13,7 @@
>   */
> 
>  #include <linux/clk.h>
> +#include <linux/debugfs.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
>  #include <linux/kernel.h>
> @@ -123,6 +124,15 @@
>  #define ICM_PROTOCOL_DP			0x4
>  #define ICM_PROTOCOL_SGMII		0x5
> 
> +static const char *const xpsgtr_icm_str[] = {
> +	[ICM_PROTOCOL_PD] = "powered down",

Protocol saying powered down seems confusing.
Should we say None or None(power down)?

> +	[ICM_PROTOCOL_PCIE] = "PCIe",
> +	[ICM_PROTOCOL_SATA] = "SATA",
> +	[ICM_PROTOCOL_USB] = "USB",
> +	[ICM_PROTOCOL_DP] = "DisplayPort",
> +	[ICM_PROTOCOL_SGMII] = "SGMII",
> +};
> +
>  /* Test Mode common reset control  parameters */
>  #define TM_CMN_RST			0x10018
>  #define TM_CMN_RST_EN			0x1
> @@ -788,6 +798,34 @@ static struct phy *xpsgtr_xlate(struct device *dev,
>  	return ERR_PTR(-EINVAL);
>  }
> 
> +/*
> + * DebugFS
> + */
> +
> +static int xpsgtr_status_read(struct seq_file *seq, void *data)
> +{
> +	struct device *dev = seq->private;
> +	struct xpsgtr_phy *gtr_phy = dev_get_drvdata(dev);
> +	struct clk *clk;
> +	u32 pll_status;
> +
> +	mutex_lock(&gtr_phy->phy->mutex);

Do we see any need for this lock? This function simply read hw register 
/phy members and decodes it.

> +	pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1);
> +	clk = gtr_phy->dev->clk[gtr_phy->refclk];
> +
> +	seq_printf(seq, "Lane:            %u\n", gtr_phy->lane);
> +	seq_printf(seq, "Protocol:        %s\n",
> +		   xpsgtr_icm_str[gtr_phy->protocol]);
> +	seq_printf(seq, "Instance:        %u\n", gtr_phy->instance);
> +	seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk);
> +	seq_printf(seq, "Reference rate:  %lu\n", clk_get_rate(clk));
> +	seq_printf(seq, "PLL locked:      %s\n",
> +		   pll_status & PLL_STATUS_LOCKED ? "yes" : "no");
> +
> +	mutex_unlock(&gtr_phy->phy->mutex);
> +	return 0;
> +}
> +
>  /*
>   * Power Management
>   */
> @@ -937,6 +975,8 @@ static int xpsgtr_probe(struct platform_device *pdev)
> 
>  		gtr_phy->phy = phy;
>  		phy_set_drvdata(phy, gtr_phy);
> +		debugfs_create_devm_seqfile(&phy->dev, "status", phy-
> >debugfs,
> +					    xpsgtr_status_read);
>  	}
> 
>  	/* Register the PHY provider. */
> --
> 2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 4/4] phy: zynqmp: Add debugfs support

[Sean Anderson]

On 6/13/24 05:20, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Monday, May 6, 2024 10:31 PM
>> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
>> phy@lists.infradead.org
>> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
>> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
>> <sean.anderson@linux.dev>
>> Subject: [PATCH v2 4/4] phy: zynqmp: Add debugfs support
>> 
>> Add support for printing some basic status information to debugfs. This
>> is helpful when debugging phy consumers to make sure they are configuring
>> the phy appropriately.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>> Changes in v2:
>> - Use debugfs_create_devm_seqfile
>> 
>>  drivers/phy/xilinx/phy-zynqmp.c | 40
>> +++++++++++++++++++++++++++++++++
>>  1 file changed, 40 insertions(+)
>> 
>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
>> zynqmp.c
>> index b86fe2a249a8..2520c75a4a77 100644
>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>> @@ -13,6 +13,7 @@
>>   */
>> 
>>  #include <linux/clk.h>
>> +#include <linux/debugfs.h>
>>  #include <linux/delay.h>
>>  #include <linux/io.h>
>>  #include <linux/kernel.h>
>> @@ -123,6 +124,15 @@
>>  #define ICM_PROTOCOL_DP			0x4
>>  #define ICM_PROTOCOL_SGMII		0x5
>> 
>> +static const char *const xpsgtr_icm_str[] = {
>> +	[ICM_PROTOCOL_PD] = "powered down",
> 
> Protocol saying powered down seems confusing.
> Should we say None or None(power down)?

Either works I guess. I'm just matching the define.

>> +	[ICM_PROTOCOL_PCIE] = "PCIe",
>> +	[ICM_PROTOCOL_SATA] = "SATA",
>> +	[ICM_PROTOCOL_USB] = "USB",
>> +	[ICM_PROTOCOL_DP] = "DisplayPort",
>> +	[ICM_PROTOCOL_SGMII] = "SGMII",
>> +};
>> +
>>  /* Test Mode common reset control  parameters */
>>  #define TM_CMN_RST			0x10018
>>  #define TM_CMN_RST_EN			0x1
>> @@ -788,6 +798,34 @@ static struct phy *xpsgtr_xlate(struct device *dev,
>>  	return ERR_PTR(-EINVAL);
>>  }
>> 
>> +/*
>> + * DebugFS
>> + */
>> +
>> +static int xpsgtr_status_read(struct seq_file *seq, void *data)
>> +{
>> +	struct device *dev = seq->private;
>> +	struct xpsgtr_phy *gtr_phy = dev_get_drvdata(dev);
>> +	struct clk *clk;
>> +	u32 pll_status;
>> +
>> +	mutex_lock(&gtr_phy->phy->mutex);
> 
> Do we see any need for this lock? This function simply read hw register 
> /phy members and decodes it.

It's to protect against modifications to gtr_phy->refclk and ->instance.

These are accessed under lock elsewhere; this is not a fast path and I don't
want to have to reason about the semantics when using a mutex is definitely
correct.

--Sean

>> +	pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1);
>> +	clk = gtr_phy->dev->clk[gtr_phy->refclk];
>> +
>> +	seq_printf(seq, "Lane:            %u\n", gtr_phy->lane);
>> +	seq_printf(seq, "Protocol:        %s\n",
>> +		   xpsgtr_icm_str[gtr_phy->protocol]);
>> +	seq_printf(seq, "Instance:        %u\n", gtr_phy->instance);
>> +	seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk);
>> +	seq_printf(seq, "Reference rate:  %lu\n", clk_get_rate(clk));
>> +	seq_printf(seq, "PLL locked:      %s\n",
>> +		   pll_status & PLL_STATUS_LOCKED ? "yes" : "no");
>> +
>> +	mutex_unlock(&gtr_phy->phy->mutex);
>> +	return 0;
>> +}
>> +
>>  /*
>>   * Power Management
>>   */
>> @@ -937,6 +975,8 @@ static int xpsgtr_probe(struct platform_device *pdev)
>> 
>>  		gtr_phy->phy = phy;
>>  		phy_set_drvdata(phy, gtr_phy);
>> +		debugfs_create_devm_seqfile(&phy->dev, "status", phy-
>> >debugfs,
>> +					    xpsgtr_status_read);
>>  	}
>> 
>>  	/* Register the PHY provider. */
>> --
>> 2.35.1.1320.gc452695387.dirty
> 

RE: [PATCH v2 4/4] phy: zynqmp: Add debugfs support

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Thursday, June 13, 2024 8:32 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Laurent
> Pinchart <laurent.pinchart@ideasonboard.com>; linux-
> phy@lists.infradead.org
> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>;
> Kishon Vijay Abraham I <kishon@kernel.org>
> Subject: Re: [PATCH v2 4/4] phy: zynqmp: Add debugfs support
> 
> On 6/13/24 05:20, Pandey, Radhey Shyam wrote:
> >> -----Original Message-----
> >> From: Sean Anderson <sean.anderson@linux.dev>
> >> Sent: Monday, May 6, 2024 10:31 PM
> >> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
> >> phy@lists.infradead.org
> >> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
> >> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
> >> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
> >> <sean.anderson@linux.dev>
> >> Subject: [PATCH v2 4/4] phy: zynqmp: Add debugfs support
> >>
> >> Add support for printing some basic status information to debugfs. This
> >> is helpful when debugging phy consumers to make sure they are
> configuring
> >> the phy appropriately.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >>
> >> Changes in v2:
> >> - Use debugfs_create_devm_seqfile
> >>
> >>  drivers/phy/xilinx/phy-zynqmp.c | 40
> >> +++++++++++++++++++++++++++++++++
> >>  1 file changed, 40 insertions(+)
> >>
> >> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
> >> zynqmp.c
> >> index b86fe2a249a8..2520c75a4a77 100644
> >> --- a/drivers/phy/xilinx/phy-zynqmp.c
> >> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> >> @@ -13,6 +13,7 @@
> >>   */
> >>
> >>  #include <linux/clk.h>
> >> +#include <linux/debugfs.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/io.h>
> >>  #include <linux/kernel.h>
> >> @@ -123,6 +124,15 @@
> >>  #define ICM_PROTOCOL_DP			0x4
> >>  #define ICM_PROTOCOL_SGMII		0x5
> >>
> >> +static const char *const xpsgtr_icm_str[] = {
> >> +	[ICM_PROTOCOL_PD] = "powered down",
> >
> > Protocol saying powered down seems confusing.
> > Should we say None or None(power down)?
> 
> Either works I guess. I'm just matching the define.

None seems fine - we can respin if there are no objections.
> 
> >> +	[ICM_PROTOCOL_PCIE] = "PCIe",
> >> +	[ICM_PROTOCOL_SATA] = "SATA",
> >> +	[ICM_PROTOCOL_USB] = "USB",
> >> +	[ICM_PROTOCOL_DP] = "DisplayPort",
> >> +	[ICM_PROTOCOL_SGMII] = "SGMII",
> >> +};
> >> +
> >>  /* Test Mode common reset control  parameters */
> >>  #define TM_CMN_RST			0x10018
> >>  #define TM_CMN_RST_EN			0x1
> >> @@ -788,6 +798,34 @@ static struct phy *xpsgtr_xlate(struct device *dev,
> >>  	return ERR_PTR(-EINVAL);
> >>  }
> >>
> >> +/*
> >> + * DebugFS
> >> + */
> >> +
> >> +static int xpsgtr_status_read(struct seq_file *seq, void *data)
> >> +{
> >> +	struct device *dev = seq->private;
> >> +	struct xpsgtr_phy *gtr_phy = dev_get_drvdata(dev);
> >> +	struct clk *clk;
> >> +	u32 pll_status;
> >> +
> >> +	mutex_lock(&gtr_phy->phy->mutex);
> >
> > Do we see any need for this lock? This function simply read hw register
> > /phy members and decodes it.
> 
> It's to protect against modifications to gtr_phy->refclk and ->instance.

Refclock and instance is assigned in xlate which is not protected by any
Lock.  Also xpsgtr_phy_init () uses a different gtr_mutex. So want
to understand this phy->mutex need.

> 
> These are accessed under lock elsewhere; this is not a fast path and I don't
> want to have to reason about the semantics when using a mutex is definitely
> correct.
> 
> --Sean
> 
> >> +	pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1);
> >> +	clk = gtr_phy->dev->clk[gtr_phy->refclk];
> >> +
> >> +	seq_printf(seq, "Lane:            %u\n", gtr_phy->lane);
> >> +	seq_printf(seq, "Protocol:        %s\n",
> >> +		   xpsgtr_icm_str[gtr_phy->protocol]);
> >> +	seq_printf(seq, "Instance:        %u\n", gtr_phy->instance);
> >> +	seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk);
> >> +	seq_printf(seq, "Reference rate:  %lu\n", clk_get_rate(clk));
> >> +	seq_printf(seq, "PLL locked:      %s\n",
> >> +		   pll_status & PLL_STATUS_LOCKED ? "yes" : "no");
> >> +
> >> +	mutex_unlock(&gtr_phy->phy->mutex);
> >> +	return 0;
> >> +}
> >> +
> >>  /*
> >>   * Power Management
> >>   */
> >> @@ -937,6 +975,8 @@ static int xpsgtr_probe(struct platform_device
> *pdev)
> >>
> >>  		gtr_phy->phy = phy;
> >>  		phy_set_drvdata(phy, gtr_phy);
> >> +		debugfs_create_devm_seqfile(&phy->dev, "status", phy-
> >> >debugfs,
> >> +					    xpsgtr_status_read);
> >>  	}
> >>
> >>  	/* Register the PHY provider. */
> >> --
> >> 2.35.1.1320.gc452695387.dirty
> >

RE: [PATCH v2 1/4] phy: zynqmp: Enable reference clock correctly

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Monday, May 6, 2024 10:31 PM
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
> phy@lists.infradead.org
> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
> <sean.anderson@linux.dev>
> Subject: [PATCH v2 1/4] phy: zynqmp: Enable reference clock correctly
> 
> Lanes can use other lanes' reference clocks, as determined by refclk.
> Use refclk to determine the clock to enable/disable instead of always
> using the lane's own reference clock. This ensures the clock selected in
> xpsgtr_configure_pll is the one enabled.
> 
> For the other half of the equation, always program REF_CLK_SEL even when
> we are selecting the lane's own clock. This ensures that Linux's idea of
> the reference clock matches the hardware. We use the "local" clock mux
> for this instead of going through the ref clock network.
> 
> Fixes: 25d700833513 ("phy: xilinx: phy-zynqmp: dynamic clock support for
> power-save")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v2:
> - New
> 
>  drivers/phy/xilinx/phy-zynqmp.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
> zynqmp.c
> index f72c5257d712..5a434382356c 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -80,7 +80,8 @@
> 
>  /* Reference clock selection parameters */
>  #define L0_Ln_REF_CLK_SEL(n)		(0x2860 + (n) * 4)
> -#define L0_REF_CLK_SEL_MASK		0x8f
> +#define L0_REF_CLK_LCL_SEL		BIT(7)
> +#define L0_REF_CLK_SEL_MASK		0x9f
> 
>  /* Calibration digital logic parameters */
>  #define L3_TM_CALIB_DIG19		0xec4c
> @@ -349,11 +350,14 @@ static void xpsgtr_configure_pll(struct xpsgtr_phy
> *gtr_phy)
>  		       PLL_FREQ_MASK, ssc->pll_ref_clk);
> 
>  	/* Enable lane clock sharing, if required */
> -	if (gtr_phy->refclk != gtr_phy->lane) {
> +	if (gtr_phy->refclk == gtr_phy->lane)
> +		/* Lane3 Ref Clock Selection Register */

This is common ref clock selection and not lane 3?

> +		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy-
> >lane),
> +			       L0_REF_CLK_SEL_MASK, L0_REF_CLK_LCL_SEL);
> +	else
>  		/* Lane3 Ref Clock Selection Register */
>  		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy-
> >lane),
>  			       L0_REF_CLK_SEL_MASK, 1 << gtr_phy->refclk);
> -	}
> 
>  	/* SSC step size [7:0] */
>  	xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_0_LSB,
> @@ -573,7 +577,7 @@ static int xpsgtr_phy_init(struct phy *phy)
>  	mutex_lock(&gtr_dev->gtr_mutex);
> 
>  	/* Configure and enable the clock when peripheral phy_init call */
> -	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->lane]))
> +	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
>  		goto out;
> 
>  	/* Skip initialization if not required. */
> @@ -625,7 +629,7 @@ static int xpsgtr_phy_exit(struct phy *phy)
>  	gtr_phy->skip_phy_init = false;
> 
>  	/* Ensure that disable clock only, which configure for lane */
> -	clk_disable_unprepare(gtr_dev->clk[gtr_phy->lane]);
> +	clk_disable_unprepare(gtr_dev->clk[gtr_phy->refclk]);
> 
>  	return 0;
>  }
> --
> 2.35.1.1320.gc452695387.dirty

RE: [PATCH v2 2/4] phy: zynqmp: Store instance instead of type

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Sean Anderson <sean.anderson@linux.dev>
> Sent: Monday, May 6, 2024 10:31 PM
> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
> phy@lists.infradead.org
> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
> <sean.anderson@linux.dev>
> Subject: [PATCH v2 2/4] phy: zynqmp: Store instance instead of type
> 
> The phy "type" is just the combination of protocol and instance, and is
> never used apart from that. Store the instance directly, instead of
> converting to a type first. No functional change intended.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
> Changes in v2:
> - Expand the icm_matrix comment
> 
>  drivers/phy/xilinx/phy-zynqmp.c | 115 +++++++++-----------------------
>  1 file changed, 31 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
> zynqmp.c
> index 5a434382356c..5a8f81bbeb8d 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -147,22 +147,6 @@
>  /* Total number of controllers */
>  #define CONTROLLERS_PER_LANE		5
> 
> -/* Protocol Type parameters */
> -#define XPSGTR_TYPE_USB0		0  /* USB controller 0 */
> -#define XPSGTR_TYPE_USB1		1  /* USB controller 1 */
> -#define XPSGTR_TYPE_SATA_0		2  /* SATA controller lane 0 */
> -#define XPSGTR_TYPE_SATA_1		3  /* SATA controller lane 1 */
> -#define XPSGTR_TYPE_PCIE_0		4  /* PCIe controller lane 0 */
> -#define XPSGTR_TYPE_PCIE_1		5  /* PCIe controller lane 1 */
> -#define XPSGTR_TYPE_PCIE_2		6  /* PCIe controller lane 2 */
> -#define XPSGTR_TYPE_PCIE_3		7  /* PCIe controller lane 3 */
> -#define XPSGTR_TYPE_DP_0		8  /* Display Port controller lane 0 */
> -#define XPSGTR_TYPE_DP_1		9  /* Display Port controller lane 1 */
> -#define XPSGTR_TYPE_SGMII0		10 /* Ethernet SGMII controller 0 */
> -#define XPSGTR_TYPE_SGMII1		11 /* Ethernet SGMII controller 1 */
> -#define XPSGTR_TYPE_SGMII2		12 /* Ethernet SGMII controller 2 */
> -#define XPSGTR_TYPE_SGMII3		13 /* Ethernet SGMII controller 3 */
> -
>  /* Timeout values */
>  #define TIMEOUT_US			1000
> 
> @@ -185,7 +169,8 @@ struct xpsgtr_ssc {
>  /**
>   * struct xpsgtr_phy - representation of a lane
>   * @phy: pointer to the kernel PHY device
> - * @type: controller which uses this lane
> + * @instance: instance of the protocol type (such as the lane within a
> + *            protocol, or the USB/Ethernet controller)
>   * @lane: lane number
>   * @protocol: protocol in which the lane operates
>   * @skip_phy_init: skip phy_init() if true
> @@ -194,7 +179,7 @@ struct xpsgtr_ssc {
>   */
>  struct xpsgtr_phy {
>  	struct phy *phy;
> -	u8 type;
> +	u8 instance;
>  	u8 lane;
>  	u8 protocol;
>  	bool skip_phy_init;
> @@ -331,8 +316,8 @@ static int xpsgtr_wait_pll_lock(struct phy *phy)
> 
>  	if (ret == -ETIMEDOUT)
>  		dev_err(gtr_dev->dev,
> -			"lane %u (type %u, protocol %u): PLL lock timeout\n",
> -			gtr_phy->lane, gtr_phy->type, gtr_phy->protocol);
> +			"lane %u (protocol %u, instance %u): PLL lock
> timeout\n",
> +			gtr_phy->lane, gtr_phy->protocol, gtr_phy-
> >instance);
> 
>  	return ret;
>  }
> @@ -647,8 +632,7 @@ static int xpsgtr_phy_power_on(struct phy *phy)
>  	 * cumulating waits for both lanes. The user is expected to initialize
>  	 * lane 0 last.
>  	 */
> -	if (gtr_phy->protocol != ICM_PROTOCOL_DP ||
> -	    gtr_phy->type == XPSGTR_TYPE_DP_0)
> +	if (gtr_phy->protocol != ICM_PROTOCOL_DP || !gtr_phy->instance)
>  		ret = xpsgtr_wait_pll_lock(phy);
> 
>  	return ret;
> @@ -678,73 +662,33 @@ static const struct phy_ops xpsgtr_phyops = {
>   * OF Xlate Support
>   */
> 
> -/* Set the lane type and protocol based on the PHY type and instance
> number. */
> +/* Set the lane protocol and instance based on the PHY type and instance
> number. */
>  static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
>  				unsigned int phy_instance)
>  {
>  	unsigned int num_phy_types;
> -	const int *phy_types;
> 
>  	switch (phy_type) {
> -	case PHY_TYPE_SATA: {
> -		static const int types[] = {
> -			XPSGTR_TYPE_SATA_0,
> -			XPSGTR_TYPE_SATA_1,
> -		};
> -
> -		phy_types = types;
> -		num_phy_types = ARRAY_SIZE(types);
> +	case PHY_TYPE_SATA:
> +		num_phy_types = 2;
>  		gtr_phy->protocol = ICM_PROTOCOL_SATA;
>  		break;
> -	}
> -	case PHY_TYPE_USB3: {
> -		static const int types[] = {
> -			XPSGTR_TYPE_USB0,
> -			XPSGTR_TYPE_USB1,
> -		};
> -
> -		phy_types = types;
> -		num_phy_types = ARRAY_SIZE(types);
> +	case PHY_TYPE_USB3:
> +		num_phy_types = 2;
>  		gtr_phy->protocol = ICM_PROTOCOL_USB;
>  		break;
> -	}
> -	case PHY_TYPE_DP: {
> -		static const int types[] = {
> -			XPSGTR_TYPE_DP_0,
> -			XPSGTR_TYPE_DP_1,
> -		};
> -
> -		phy_types = types;
> -		num_phy_types = ARRAY_SIZE(types);
> +	case PHY_TYPE_DP:
> +		num_phy_types = 2;
>  		gtr_phy->protocol = ICM_PROTOCOL_DP;
>  		break;
> -	}
> -	case PHY_TYPE_PCIE: {
> -		static const int types[] = {
> -			XPSGTR_TYPE_PCIE_0,
> -			XPSGTR_TYPE_PCIE_1,
> -			XPSGTR_TYPE_PCIE_2,
> -			XPSGTR_TYPE_PCIE_3,
> -		};
> -
> -		phy_types = types;
> -		num_phy_types = ARRAY_SIZE(types);
> +	case PHY_TYPE_PCIE:
> +		num_phy_types = 4;
>  		gtr_phy->protocol = ICM_PROTOCOL_PCIE;
>  		break;
> -	}
> -	case PHY_TYPE_SGMII: {
> -		static const int types[] = {
> -			XPSGTR_TYPE_SGMII0,
> -			XPSGTR_TYPE_SGMII1,
> -			XPSGTR_TYPE_SGMII2,
> -			XPSGTR_TYPE_SGMII3,
> -		};
> -
> -		phy_types = types;
> -		num_phy_types = ARRAY_SIZE(types);
> +	case PHY_TYPE_SGMII:
> +		num_phy_types = 4;
>  		gtr_phy->protocol = ICM_PROTOCOL_SGMII;
>  		break;
> -	}
>  	default:
>  		return -EINVAL;
>  	}
> @@ -752,22 +696,25 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy
> *gtr_phy, u8 phy_type,
>  	if (phy_instance >= num_phy_types)
>  		return -EINVAL;
> 
> -	gtr_phy->type = phy_types[phy_instance];
> +	gtr_phy->instance = phy_instance;
>  	return 0;
>  }
> 
>  /*
> - * Valid combinations of controllers and lanes (Interconnect Matrix).
> + * Valid combinations of controllers and lanes (Interconnect Matrix). Each
> + * "instance" represents one controller for a lane. For PCIe and DP, the
> + * "instance" is the logical lane in the link. For SATA, USB, and SGMII,
> + * the instance is the index of the controller.
> + *
> + * This information is only used to validate the devicetree reference, and is
> + * not used when programming the hardware.
>   */
>  static const unsigned int
> icm_matrix[NUM_LANES][CONTROLLERS_PER_LANE] = {
> -	{ XPSGTR_TYPE_PCIE_0, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
> -		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII0 },
> -	{ XPSGTR_TYPE_PCIE_1, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB0,
> -		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII1 },
> -	{ XPSGTR_TYPE_PCIE_2, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
> -		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII2 },
> -	{ XPSGTR_TYPE_PCIE_3, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB1,
> -		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII3 }
> +	/* PCIe, SATA, USB, DP, SGMII */
> +	{ 0, 0, 0, 1, 0 }, /* Lane 0 */
> +	{ 1, 1, 0, 0, 1 }, /* Lane 1 */
> +	{ 2, 0, 0, 1, 2 }, /* Lane 2 */
> +	{ 3, 1, 1, 0, 3 }, /* Lane 3 */
>  };

Feel this change reduces readability and introduce magic values
for icm_matrix and num_phy_types. At times decoding these 
numbers can be hard. 

Can we retain deriving num_phy_types from ARRAY_SIZE(types);
and also defines for ICM matrix?

> 
>  /* Translate OF phandle and args to PHY instance. */
> @@ -822,7 +769,7 @@ static struct phy *xpsgtr_xlate(struct device *dev,
>  	 * is allowed to operate on the lane.
>  	 */
>  	for (i = 0; i < CONTROLLERS_PER_LANE; i++) {
> -		if (icm_matrix[phy_lane][i] == gtr_phy->type)
> +		if (icm_matrix[phy_lane][i] == gtr_phy->instance)
>  			return gtr_phy->phy;
>  	}
> 
> --
> 2.35.1.1320.gc452695387.dirty

Re: [PATCH v2 2/4] phy: zynqmp: Store instance instead of type

[Sean Anderson]

On 6/14/24 02:02, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Monday, May 6, 2024 10:31 PM
>> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
>> phy@lists.infradead.org
>> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
>> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
>> <sean.anderson@linux.dev>
>> Subject: [PATCH v2 2/4] phy: zynqmp: Store instance instead of type
>> 
>> The phy "type" is just the combination of protocol and instance, and is
>> never used apart from that. Store the instance directly, instead of
>> converting to a type first. No functional change intended.
>> 
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>> Changes in v2:
>> - Expand the icm_matrix comment
>> 
>>  drivers/phy/xilinx/phy-zynqmp.c | 115 +++++++++-----------------------
>>  1 file changed, 31 insertions(+), 84 deletions(-)
>> 
>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
>> zynqmp.c
>> index 5a434382356c..5a8f81bbeb8d 100644
>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>> @@ -147,22 +147,6 @@
>>  /* Total number of controllers */
>>  #define CONTROLLERS_PER_LANE		5
>> 
>> -/* Protocol Type parameters */
>> -#define XPSGTR_TYPE_USB0		0  /* USB controller 0 */
>> -#define XPSGTR_TYPE_USB1		1  /* USB controller 1 */
>> -#define XPSGTR_TYPE_SATA_0		2  /* SATA controller lane 0 */
>> -#define XPSGTR_TYPE_SATA_1		3  /* SATA controller lane 1 */
>> -#define XPSGTR_TYPE_PCIE_0		4  /* PCIe controller lane 0 */
>> -#define XPSGTR_TYPE_PCIE_1		5  /* PCIe controller lane 1 */
>> -#define XPSGTR_TYPE_PCIE_2		6  /* PCIe controller lane 2 */
>> -#define XPSGTR_TYPE_PCIE_3		7  /* PCIe controller lane 3 */
>> -#define XPSGTR_TYPE_DP_0		8  /* Display Port controller lane 0 */
>> -#define XPSGTR_TYPE_DP_1		9  /* Display Port controller lane 1 */
>> -#define XPSGTR_TYPE_SGMII0		10 /* Ethernet SGMII controller 0 */
>> -#define XPSGTR_TYPE_SGMII1		11 /* Ethernet SGMII controller 1 */
>> -#define XPSGTR_TYPE_SGMII2		12 /* Ethernet SGMII controller 2 */
>> -#define XPSGTR_TYPE_SGMII3		13 /* Ethernet SGMII controller 3 */
>> -
>>  /* Timeout values */
>>  #define TIMEOUT_US			1000
>> 
>> @@ -185,7 +169,8 @@ struct xpsgtr_ssc {
>>  /**
>>   * struct xpsgtr_phy - representation of a lane
>>   * @phy: pointer to the kernel PHY device
>> - * @type: controller which uses this lane
>> + * @instance: instance of the protocol type (such as the lane within a
>> + *            protocol, or the USB/Ethernet controller)
>>   * @lane: lane number
>>   * @protocol: protocol in which the lane operates
>>   * @skip_phy_init: skip phy_init() if true
>> @@ -194,7 +179,7 @@ struct xpsgtr_ssc {
>>   */
>>  struct xpsgtr_phy {
>>  	struct phy *phy;
>> -	u8 type;
>> +	u8 instance;
>>  	u8 lane;
>>  	u8 protocol;
>>  	bool skip_phy_init;
>> @@ -331,8 +316,8 @@ static int xpsgtr_wait_pll_lock(struct phy *phy)
>> 
>>  	if (ret == -ETIMEDOUT)
>>  		dev_err(gtr_dev->dev,
>> -			"lane %u (type %u, protocol %u): PLL lock timeout\n",
>> -			gtr_phy->lane, gtr_phy->type, gtr_phy->protocol);
>> +			"lane %u (protocol %u, instance %u): PLL lock
>> timeout\n",
>> +			gtr_phy->lane, gtr_phy->protocol, gtr_phy-
>> >instance);
>> 
>>  	return ret;
>>  }
>> @@ -647,8 +632,7 @@ static int xpsgtr_phy_power_on(struct phy *phy)
>>  	 * cumulating waits for both lanes. The user is expected to initialize
>>  	 * lane 0 last.
>>  	 */
>> -	if (gtr_phy->protocol != ICM_PROTOCOL_DP ||
>> -	    gtr_phy->type == XPSGTR_TYPE_DP_0)
>> +	if (gtr_phy->protocol != ICM_PROTOCOL_DP || !gtr_phy->instance)
>>  		ret = xpsgtr_wait_pll_lock(phy);
>> 
>>  	return ret;
>> @@ -678,73 +662,33 @@ static const struct phy_ops xpsgtr_phyops = {
>>   * OF Xlate Support
>>   */
>> 
>> -/* Set the lane type and protocol based on the PHY type and instance
>> number. */
>> +/* Set the lane protocol and instance based on the PHY type and instance
>> number. */
>>  static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
>>  				unsigned int phy_instance)
>>  {
>>  	unsigned int num_phy_types;
>> -	const int *phy_types;
>> 
>>  	switch (phy_type) {
>> -	case PHY_TYPE_SATA: {
>> -		static const int types[] = {
>> -			XPSGTR_TYPE_SATA_0,
>> -			XPSGTR_TYPE_SATA_1,
>> -		};
>> -
>> -		phy_types = types;
>> -		num_phy_types = ARRAY_SIZE(types);
>> +	case PHY_TYPE_SATA:
>> +		num_phy_types = 2;
>>  		gtr_phy->protocol = ICM_PROTOCOL_SATA;
>>  		break;
>> -	}
>> -	case PHY_TYPE_USB3: {
>> -		static const int types[] = {
>> -			XPSGTR_TYPE_USB0,
>> -			XPSGTR_TYPE_USB1,
>> -		};
>> -
>> -		phy_types = types;
>> -		num_phy_types = ARRAY_SIZE(types);
>> +	case PHY_TYPE_USB3:
>> +		num_phy_types = 2;
>>  		gtr_phy->protocol = ICM_PROTOCOL_USB;
>>  		break;
>> -	}
>> -	case PHY_TYPE_DP: {
>> -		static const int types[] = {
>> -			XPSGTR_TYPE_DP_0,
>> -			XPSGTR_TYPE_DP_1,
>> -		};
>> -
>> -		phy_types = types;
>> -		num_phy_types = ARRAY_SIZE(types);
>> +	case PHY_TYPE_DP:
>> +		num_phy_types = 2;
>>  		gtr_phy->protocol = ICM_PROTOCOL_DP;
>>  		break;
>> -	}
>> -	case PHY_TYPE_PCIE: {
>> -		static const int types[] = {
>> -			XPSGTR_TYPE_PCIE_0,
>> -			XPSGTR_TYPE_PCIE_1,
>> -			XPSGTR_TYPE_PCIE_2,
>> -			XPSGTR_TYPE_PCIE_3,
>> -		};
>> -
>> -		phy_types = types;
>> -		num_phy_types = ARRAY_SIZE(types);
>> +	case PHY_TYPE_PCIE:
>> +		num_phy_types = 4;
>>  		gtr_phy->protocol = ICM_PROTOCOL_PCIE;
>>  		break;
>> -	}
>> -	case PHY_TYPE_SGMII: {
>> -		static const int types[] = {
>> -			XPSGTR_TYPE_SGMII0,
>> -			XPSGTR_TYPE_SGMII1,
>> -			XPSGTR_TYPE_SGMII2,
>> -			XPSGTR_TYPE_SGMII3,
>> -		};
>> -
>> -		phy_types = types;
>> -		num_phy_types = ARRAY_SIZE(types);
>> +	case PHY_TYPE_SGMII:
>> +		num_phy_types = 4;
>>  		gtr_phy->protocol = ICM_PROTOCOL_SGMII;
>>  		break;
>> -	}
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -752,22 +696,25 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy
>> *gtr_phy, u8 phy_type,
>>  	if (phy_instance >= num_phy_types)
>>  		return -EINVAL;
>> 
>> -	gtr_phy->type = phy_types[phy_instance];
>> +	gtr_phy->instance = phy_instance;
>>  	return 0;
>>  }
>> 
>>  /*
>> - * Valid combinations of controllers and lanes (Interconnect Matrix).
>> + * Valid combinations of controllers and lanes (Interconnect Matrix). Each
>> + * "instance" represents one controller for a lane. For PCIe and DP, the
>> + * "instance" is the logical lane in the link. For SATA, USB, and SGMII,
>> + * the instance is the index of the controller.
>> + *
>> + * This information is only used to validate the devicetree reference, and is
>> + * not used when programming the hardware.
>>   */
>>  static const unsigned int
>> icm_matrix[NUM_LANES][CONTROLLERS_PER_LANE] = {
>> -	{ XPSGTR_TYPE_PCIE_0, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
>> -		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII0 },
>> -	{ XPSGTR_TYPE_PCIE_1, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB0,
>> -		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII1 },
>> -	{ XPSGTR_TYPE_PCIE_2, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0,
>> -		XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII2 },
>> -	{ XPSGTR_TYPE_PCIE_3, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB1,
>> -		XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII3 }
>> +	/* PCIe, SATA, USB, DP, SGMII */
>> +	{ 0, 0, 0, 1, 0 }, /* Lane 0 */
>> +	{ 1, 1, 0, 0, 1 }, /* Lane 1 */
>> +	{ 2, 0, 0, 1, 2 }, /* Lane 2 */
>> +	{ 3, 1, 1, 0, 3 }, /* Lane 3 */
>>  };
> 
> Feel this change reduces readability and introduce magic values
> for icm_matrix and num_phy_types. At times decoding these 
> numbers can be hard. 

> Can we retain deriving num_phy_types from ARRAY_SIZE(types);
> and also defines for ICM matrix?

There is no point. The value of e.g. XPSGTR_TYPE_PCIE_3 would be 3.
The value of XPSGTR_TYPE_SATA_1 would be 1. The names are already
given by the structure of the matrix.

--Sean

>> 
>>  /* Translate OF phandle and args to PHY instance. */
>> @@ -822,7 +769,7 @@ static struct phy *xpsgtr_xlate(struct device *dev,
>>  	 * is allowed to operate on the lane.
>>  	 */
>>  	for (i = 0; i < CONTROLLERS_PER_LANE; i++) {
>> -		if (icm_matrix[phy_lane][i] == gtr_phy->type)
>> +		if (icm_matrix[phy_lane][i] == gtr_phy->instance)
>>  			return gtr_phy->phy;
>>  	}
>> 
>> --
>> 2.35.1.1320.gc452695387.dirty
> 

Re: [PATCH v2 1/4] phy: zynqmp: Enable reference clock correctly

[Sean Anderson]

On 6/14/24 01:30, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Monday, May 6, 2024 10:31 PM
>> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
>> phy@lists.infradead.org
>> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
>> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
>> <sean.anderson@linux.dev>
>> Subject: [PATCH v2 1/4] phy: zynqmp: Enable reference clock correctly
>> 
>> Lanes can use other lanes' reference clocks, as determined by refclk.
>> Use refclk to determine the clock to enable/disable instead of always
>> using the lane's own reference clock. This ensures the clock selected in
>> xpsgtr_configure_pll is the one enabled.
>> 
>> For the other half of the equation, always program REF_CLK_SEL even when
>> we are selecting the lane's own clock. This ensures that Linux's idea of
>> the reference clock matches the hardware. We use the "local" clock mux
>> for this instead of going through the ref clock network.
>> 
>> Fixes: 25d700833513 ("phy: xilinx: phy-zynqmp: dynamic clock support for
>> power-save")
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>> 
>> Changes in v2:
>> - New
>> 
>>  drivers/phy/xilinx/phy-zynqmp.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
>> zynqmp.c
>> index f72c5257d712..5a434382356c 100644
>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>> @@ -80,7 +80,8 @@
>> 
>>  /* Reference clock selection parameters */
>>  #define L0_Ln_REF_CLK_SEL(n)		(0x2860 + (n) * 4)
>> -#define L0_REF_CLK_SEL_MASK		0x8f
>> +#define L0_REF_CLK_LCL_SEL		BIT(7)
>> +#define L0_REF_CLK_SEL_MASK		0x9f
>> 
>>  /* Calibration digital logic parameters */
>>  #define L3_TM_CALIB_DIG19		0xec4c
>> @@ -349,11 +350,14 @@ static void xpsgtr_configure_pll(struct xpsgtr_phy
>> *gtr_phy)
>>  		       PLL_FREQ_MASK, ssc->pll_ref_clk);
>> 
>>  	/* Enable lane clock sharing, if required */
>> -	if (gtr_phy->refclk != gtr_phy->lane) {
>> +	if (gtr_phy->refclk == gtr_phy->lane)
>> +		/* Lane3 Ref Clock Selection Register */
> 
> This is common ref clock selection and not lane 3?

This is copied from the existing comment. I will remove it.

--Sean

>> +		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy-
>> >lane),
>> +			       L0_REF_CLK_SEL_MASK, L0_REF_CLK_LCL_SEL);
>> +	else
>>  		/* Lane3 Ref Clock Selection Register */
>>  		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy-
>> >lane),
>>  			       L0_REF_CLK_SEL_MASK, 1 << gtr_phy->refclk);
>> -	}
>> 
>>  	/* SSC step size [7:0] */
>>  	xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_0_LSB,
>> @@ -573,7 +577,7 @@ static int xpsgtr_phy_init(struct phy *phy)
>>  	mutex_lock(&gtr_dev->gtr_mutex);
>> 
>>  	/* Configure and enable the clock when peripheral phy_init call */
>> -	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->lane]))
>> +	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
>>  		goto out;
>> 
>>  	/* Skip initialization if not required. */
>> @@ -625,7 +629,7 @@ static int xpsgtr_phy_exit(struct phy *phy)
>>  	gtr_phy->skip_phy_init = false;
>> 
>>  	/* Ensure that disable clock only, which configure for lane */
>> -	clk_disable_unprepare(gtr_dev->clk[gtr_phy->lane]);
>> +	clk_disable_unprepare(gtr_dev->clk[gtr_phy->refclk]);
>> 
>>  	return 0;
>>  }
>> --
>> 2.35.1.1320.gc452695387.dirty
> 

Re: [PATCH v2 4/4] phy: zynqmp: Add debugfs support

[Sean Anderson]

On 6/14/24 01:16, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@linux.dev>
>> Sent: Thursday, June 13, 2024 8:32 PM
>> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>; Laurent
>> Pinchart <laurent.pinchart@ideasonboard.com>; linux-
>> phy@lists.infradead.org
>> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
>> linux-kernel@vger.kernel.org; Simek, Michal <michal.simek@amd.com>;
>> Kishon Vijay Abraham I <kishon@kernel.org>
>> Subject: Re: [PATCH v2 4/4] phy: zynqmp: Add debugfs support
>> 
>> On 6/13/24 05:20, Pandey, Radhey Shyam wrote:
>> >> -----Original Message-----
>> >> From: Sean Anderson <sean.anderson@linux.dev>
>> >> Sent: Monday, May 6, 2024 10:31 PM
>> >> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>; linux-
>> >> phy@lists.infradead.org
>> >> Cc: Vinod Koul <vkoul@kernel.org>; linux-arm-kernel@lists.infradead.org;
>> >> linux-kernel@vger.kernel.org; Michal Simek <michal.simek@amd.com>;
>> >> Kishon Vijay Abraham I <kishon@kernel.org>; Sean Anderson
>> >> <sean.anderson@linux.dev>
>> >> Subject: [PATCH v2 4/4] phy: zynqmp: Add debugfs support
>> >>
>> >> Add support for printing some basic status information to debugfs. This
>> >> is helpful when debugging phy consumers to make sure they are
>> configuring
>> >> the phy appropriately.
>> >>
>> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> ---
>> >>
>> >> Changes in v2:
>> >> - Use debugfs_create_devm_seqfile
>> >>
>> >>  drivers/phy/xilinx/phy-zynqmp.c | 40
>> >> +++++++++++++++++++++++++++++++++
>> >>  1 file changed, 40 insertions(+)
>> >>
>> >> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
>> >> zynqmp.c
>> >> index b86fe2a249a8..2520c75a4a77 100644
>> >> --- a/drivers/phy/xilinx/phy-zynqmp.c
>> >> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>> >> @@ -13,6 +13,7 @@
>> >>   */
>> >>
>> >>  #include <linux/clk.h>
>> >> +#include <linux/debugfs.h>
>> >>  #include <linux/delay.h>
>> >>  #include <linux/io.h>
>> >>  #include <linux/kernel.h>
>> >> @@ -123,6 +124,15 @@
>> >>  #define ICM_PROTOCOL_DP			0x4
>> >>  #define ICM_PROTOCOL_SGMII		0x5
>> >>
>> >> +static const char *const xpsgtr_icm_str[] = {
>> >> +	[ICM_PROTOCOL_PD] = "powered down",
>> >
>> > Protocol saying powered down seems confusing.
>> > Should we say None or None(power down)?
>> 
>> Either works I guess. I'm just matching the define.
> 
> None seems fine - we can respin if there are no objections.
>> 
>> >> +	[ICM_PROTOCOL_PCIE] = "PCIe",
>> >> +	[ICM_PROTOCOL_SATA] = "SATA",
>> >> +	[ICM_PROTOCOL_USB] = "USB",
>> >> +	[ICM_PROTOCOL_DP] = "DisplayPort",
>> >> +	[ICM_PROTOCOL_SGMII] = "SGMII",
>> >> +};
>> >> +
>> >>  /* Test Mode common reset control  parameters */
>> >>  #define TM_CMN_RST			0x10018
>> >>  #define TM_CMN_RST_EN			0x1
>> >> @@ -788,6 +798,34 @@ static struct phy *xpsgtr_xlate(struct device *dev,
>> >>  	return ERR_PTR(-EINVAL);
>> >>  }
>> >>
>> >> +/*
>> >> + * DebugFS
>> >> + */
>> >> +
>> >> +static int xpsgtr_status_read(struct seq_file *seq, void *data)
>> >> +{
>> >> +	struct device *dev = seq->private;
>> >> +	struct xpsgtr_phy *gtr_phy = dev_get_drvdata(dev);
>> >> +	struct clk *clk;
>> >> +	u32 pll_status;
>> >> +
>> >> +	mutex_lock(&gtr_phy->phy->mutex);
>> >
>> > Do we see any need for this lock? This function simply read hw register
>> > /phy members and decodes it.
>> 
>> It's to protect against modifications to gtr_phy->refclk and ->instance.
> 
> Refclock and instance is assigned in xlate which is not protected by any
> Lock.  Also xpsgtr_phy_init () uses a different gtr_mutex. So want
> to understand this phy->mutex need.

Ah, well then we should take this lock in xlate.

--Sean

>> 
>> These are accessed under lock elsewhere; this is not a fast path and I don't
>> want to have to reason about the semantics when using a mutex is definitely
>> correct.
>> 
>> --Sean
>> 
>> >> +	pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1);
>> >> +	clk = gtr_phy->dev->clk[gtr_phy->refclk];
>> >> +
>> >> +	seq_printf(seq, "Lane:            %u\n", gtr_phy->lane);
>> >> +	seq_printf(seq, "Protocol:        %s\n",
>> >> +		   xpsgtr_icm_str[gtr_phy->protocol]);
>> >> +	seq_printf(seq, "Instance:        %u\n", gtr_phy->instance);
>> >> +	seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk);
>> >> +	seq_printf(seq, "Reference rate:  %lu\n", clk_get_rate(clk));
>> >> +	seq_printf(seq, "PLL locked:      %s\n",
>> >> +		   pll_status & PLL_STATUS_LOCKED ? "yes" : "no");
>> >> +
>> >> +	mutex_unlock(&gtr_phy->phy->mutex);
>> >> +	return 0;
>> >> +}
>> >> +
>> >>  /*
>> >>   * Power Management
>> >>   */
>> >> @@ -937,6 +975,8 @@ static int xpsgtr_probe(struct platform_device
>> *pdev)
>> >>
>> >>  		gtr_phy->phy = phy;
>> >>  		phy_set_drvdata(phy, gtr_phy);
>> >> +		debugfs_create_devm_seqfile(&phy->dev, "status", phy-
>> >> >debugfs,
>> >> +					    xpsgtr_status_read);
>> >>  	}
>> >>
>> >>  	/* Register the PHY provider. */
>> >> --
>> >> 2.35.1.1320.gc452695387.dirty
>> >
>