[PATCH 0/3] phy: zynqmp: A PCIe fix and debugfs support

[PATCH 0/3] phy: zynqmp: A PCIe fix and debugfs support

[Sean Anderson]

This has a few small patches cleaning up the driver, adding a fix for
PCIe, and adding some useful debugfs info.


Sean Anderson (3):
  phy: zynqmp: Store instance instead of type
  phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes
  phy: zynqmp: Add debugfs support

 drivers/phy/xilinx/phy-zynqmp.c | 169 ++++++++++++++++----------------
 1 file changed, 83 insertions(+), 86 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 1/3] 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>
---

 drivers/phy/xilinx/phy-zynqmp.c | 107 +++++++-------------------------
 1 file changed, 24 insertions(+), 83 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index f72c5257d712..b507ed4c3053 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -146,22 +146,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
 
@@ -184,7 +168,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
@@ -193,7 +178,7 @@ struct xpsgtr_ssc {
  */
 struct xpsgtr_phy {
 	struct phy *phy;
-	u8 type;
+	u8 instance;
 	u8 lane;
 	u8 protocol;
 	bool skip_phy_init;
@@ -330,8 +315,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;
 }
@@ -643,8 +628,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;
@@ -674,73 +658,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;
 	}
@@ -748,7 +692,7 @@ 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;
 }
 
@@ -756,14 +700,11 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
  * Valid combinations of controllers and lanes (Interconnect Matrix).
  */
 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 },
+	{ 1, 1, 0, 0, 1 },
+	{ 2, 0, 0, 1, 2 },
+	{ 3, 1, 1, 0, 3 },
 };
 
 /* Translate OF phandle and args to PHY instance. */
@@ -818,7 +759,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 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes

[Sean Anderson]

Similarly to DisplayPort, nonzero PCIe lanes never achieve PLL lock [1].
Don't wait for them.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---
Maybe it's actually that only the final lane configured locks? I haven't
tested this out.

 drivers/phy/xilinx/phy-zynqmp.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index b507ed4c3053..08c88dcd7799 100644
--- a/drivers/phy/xilinx/phy-zynqmp.c
+++ b/drivers/phy/xilinx/phy-zynqmp.c
@@ -624,11 +624,13 @@ static int xpsgtr_phy_power_on(struct phy *phy)
 	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.
+	 * Wait for the PLL to lock. For DP and PCIe, only wait on instance 0
+	 * to avoid cumulative waits for both lanes. The user is expected to
+	 * initialize lane 0 last.
 	 */
-	if (gtr_phy->protocol != ICM_PROTOCOL_DP || !gtr_phy->instance)
+	if ((gtr_phy->protocol != ICM_PROTOCOL_DP &&
+	     gtr_phy->protocol != ICM_PROTOCOL_PCIE) ||
+	    !gtr_phy->instance)
 		ret = xpsgtr_wait_pll_lock(phy);
 
 	return ret;
-- 
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 3/3] 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>
---

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

diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
index 08c88dcd7799..e2e86943e9f3 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>
@@ -122,6 +123,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
@@ -768,6 +778,48 @@ 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 xpsgtr_phy *gtr_phy = seq->private;
+	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;
+}
+
+static int xpsgtr_status_open(struct inode *inode, struct file *f)
+{
+	struct xpsgtr_phy *gtr_phy = inode->i_private;
+
+	return single_open(f, xpsgtr_status_read, gtr_phy);
+}
+
+static const struct file_operations xpsgtr_status_ops = {
+	.owner = THIS_MODULE,
+	.open = xpsgtr_status_open,
+	.release = single_release,
+	.read = seq_read,
+	.llseek = seq_lseek
+};
+
 /*
  * Power Management
  */
@@ -917,6 +969,8 @@ static int xpsgtr_probe(struct platform_device *pdev)
 
 		gtr_phy->phy = phy;
 		phy_set_drvdata(phy, gtr_phy);
+		debugfs_create_file("status", 0444, phy->debugfs, gtr_phy,
+				    &xpsgtr_status_ops);
 	}
 
 	/* 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 1/3] phy: zynqmp: Store instance instead of type

[Michal Simek]

On 4/22/24 20:58, Sean Anderson wrote:
> 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>
> ---
> 
>   drivers/phy/xilinx/phy-zynqmp.c | 107 +++++++-------------------------
>   1 file changed, 24 insertions(+), 83 deletions(-)
> 
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index f72c5257d712..b507ed4c3053 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -146,22 +146,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
>   
> @@ -184,7 +168,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
> @@ -193,7 +178,7 @@ struct xpsgtr_ssc {
>    */
>   struct xpsgtr_phy {
>   	struct phy *phy;
> -	u8 type;
> +	u8 instance;
>   	u8 lane;
>   	u8 protocol;
>   	bool skip_phy_init;
> @@ -330,8 +315,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;
>   }
> @@ -643,8 +628,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;
> @@ -674,73 +658,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;
>   	}
> @@ -748,7 +692,7 @@ 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;
>   }
>   
> @@ -756,14 +700,11 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
>    * Valid combinations of controllers and lanes (Interconnect Matrix).
>    */
>   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 },
> +	{ 1, 1, 0, 0, 1 },
> +	{ 2, 0, 0, 1, 2 },
> +	{ 3, 1, 1, 0, 3 },


Do you think that this is more understandable than was before?

Thanks,
Michal

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

Re: [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes

[Michal Simek]

On 4/22/24 20:58, Sean Anderson wrote:
> Similarly to DisplayPort, nonzero PCIe lanes never achieve PLL lock [1].

What is this [1] for?

M

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

Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type

[Sean Anderson]

On 4/23/24 02:25, Michal Simek wrote:
> 
> 
> On 4/22/24 20:58, Sean Anderson wrote:
>> 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>
>> ---
>>
>>   drivers/phy/xilinx/phy-zynqmp.c | 107 +++++++-------------------------
>>   1 file changed, 24 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
>> index f72c5257d712..b507ed4c3053 100644
>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>> @@ -146,22 +146,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
>>   @@ -184,7 +168,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
>> @@ -193,7 +178,7 @@ struct xpsgtr_ssc {
>>    */
>>   struct xpsgtr_phy {
>>       struct phy *phy;
>> -    u8 type;
>> +    u8 instance;
>>       u8 lane;
>>       u8 protocol;
>>       bool skip_phy_init;
>> @@ -330,8 +315,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;
>>   }
>> @@ -643,8 +628,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;
>> @@ -674,73 +658,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;
>>       }
>> @@ -748,7 +692,7 @@ 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;
>>   }
>>   @@ -756,14 +700,11 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
>>    * Valid combinations of controllers and lanes (Interconnect Matrix).
>>    */
>>   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 },
>> +    { 1, 1, 0, 0, 1 },
>> +    { 2, 0, 0, 1, 2 },
>> +    { 3, 1, 1, 0, 3 },
> 
> 
> Do you think that this is more understandable than was before?

Yes. This better matches the documentation. And now it is easier to
programmatically access the index. Before we might have

protocol = ICM_PROTOCOL_USB
type = XPSGTR_TYPE_USB0

and the instance is there in the type name, but we can't access it. Now
we have

instance = 0

and it is easy to determine which instance we have.

--Sean

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

Re: [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes

[Sean Anderson]

On 4/23/24 02:25, Michal Simek wrote:
> 
> 
> On 4/22/24 20:58, Sean Anderson wrote:
>> Similarly to DisplayPort, nonzero PCIe lanes never achieve PLL lock [1].
> 
> What is this [1] for?

I was originally going to have the comment below the fold in the commit
message as a footnote. I forgot to remove this after editing.

--Sean

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

Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type

[Michal Simek]

On 4/23/24 17:02, Sean Anderson wrote:
> On 4/23/24 02:25, Michal Simek wrote:
>>
>>
>> On 4/22/24 20:58, Sean Anderson wrote:
>>> 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>
>>> ---
>>>
>>>    drivers/phy/xilinx/phy-zynqmp.c | 107 +++++++-------------------------
>>>    1 file changed, 24 insertions(+), 83 deletions(-)
>>>
>>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
>>> index f72c5257d712..b507ed4c3053 100644
>>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>>> @@ -146,22 +146,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
>>>    @@ -184,7 +168,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
>>> @@ -193,7 +178,7 @@ struct xpsgtr_ssc {
>>>     */
>>>    struct xpsgtr_phy {
>>>        struct phy *phy;
>>> -    u8 type;
>>> +    u8 instance;
>>>        u8 lane;
>>>        u8 protocol;
>>>        bool skip_phy_init;
>>> @@ -330,8 +315,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;
>>>    }
>>> @@ -643,8 +628,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;
>>> @@ -674,73 +658,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;
>>>        }
>>> @@ -748,7 +692,7 @@ 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;
>>>    }
>>>    @@ -756,14 +700,11 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
>>>     * Valid combinations of controllers and lanes (Interconnect Matrix).
>>>     */
>>>    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 },
>>> +    { 1, 1, 0, 0, 1 },
>>> +    { 2, 0, 0, 1, 2 },
>>> +    { 3, 1, 1, 0, 3 },
>>
>>
>> Do you think that this is more understandable than was before?
> 
> Yes. This better matches the documentation. And now it is easier to
> programmatically access the index. Before we might have
> 
> protocol = ICM_PROTOCOL_USB
> type = XPSGTR_TYPE_USB0
> 
> and the instance is there in the type name, but we can't access it. Now
> we have
> 
> instance = 0
> 
> and it is easy to determine which instance we have.

Numbers itself means in PCIE, SATA and DP number of lanes used but there is only 
single controller.
In USB and SGMII case it is one lane per controller which is different 
information pointing to controller ID.

I didn't look at this driver for a while but at least for this array I would 
prefer to extend comment above to describe position of controllers and connected 
lanes and explain what that numbers really means.

Thanks,
Michal


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

Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type

[Sean Anderson]

On 4/24/24 02:38, Michal Simek wrote:
> 
> 
> On 4/23/24 17:02, Sean Anderson wrote:
>> On 4/23/24 02:25, Michal Simek wrote:
>>>
>>>
>>> On 4/22/24 20:58, Sean Anderson wrote:
>>>> 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>
>>>> ---
>>>>
>>>>    drivers/phy/xilinx/phy-zynqmp.c | 107 +++++++-------------------------
>>>>    1 file changed, 24 insertions(+), 83 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
>>>> index f72c5257d712..b507ed4c3053 100644
>>>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>>>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>>>> @@ -146,22 +146,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
>>>>    @@ -184,7 +168,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
>>>> @@ -193,7 +178,7 @@ struct xpsgtr_ssc {
>>>>     */
>>>>    struct xpsgtr_phy {
>>>>        struct phy *phy;
>>>> -    u8 type;
>>>> +    u8 instance;
>>>>        u8 lane;
>>>>        u8 protocol;
>>>>        bool skip_phy_init;
>>>> @@ -330,8 +315,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;
>>>>    }
>>>> @@ -643,8 +628,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;
>>>> @@ -674,73 +658,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;
>>>>        }
>>>> @@ -748,7 +692,7 @@ 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;
>>>>    }
>>>>    @@ -756,14 +700,11 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type,
>>>>     * Valid combinations of controllers and lanes (Interconnect Matrix).
>>>>     */
>>>>    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 },
>>>> +    { 1, 1, 0, 0, 1 },
>>>> +    { 2, 0, 0, 1, 2 },
>>>> +    { 3, 1, 1, 0, 3 },
>>>
>>>
>>> Do you think that this is more understandable than was before?
>>
>> Yes. This better matches the documentation. And now it is easier to
>> programmatically access the index. Before we might have
>>
>> protocol = ICM_PROTOCOL_USB
>> type = XPSGTR_TYPE_USB0
>>
>> and the instance is there in the type name, but we can't access it. Now
>> we have
>>
>> instance = 0
>>
>> and it is easy to determine which instance we have.
> 
> Numbers itself means in PCIE, SATA and DP number of lanes used but
> there is only single controller. In USB and SGMII case it is one lane
> per controller which is different information pointing to controller
> ID.

Yeah, the instance is really just to validate the devicetree binding.
It's not used in the driver except to determine the lane to wait for PLL
lock on in a multi-lane configuration.

> I didn't look at this driver for a while but at least for this array I
> would prefer to extend comment above to describe position of
> controllers and connected lanes and explain what that numbers really
> means.

OK, I will expand the comment for v2.

--Sean

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

Re: [PATCH 3/3] phy: zynqmp: Add debugfs support

[Vinod Koul]

On 22-04-24, 14:58, Sean Anderson wrote:
> 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>
> ---
> 
>  drivers/phy/xilinx/phy-zynqmp.c | 54 +++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index 08c88dcd7799..e2e86943e9f3 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>
> @@ -122,6 +123,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
> @@ -768,6 +778,48 @@ 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 xpsgtr_phy *gtr_phy = seq->private;
> +	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;
> +}
> +
> +static int xpsgtr_status_open(struct inode *inode, struct file *f)
> +{
> +	struct xpsgtr_phy *gtr_phy = inode->i_private;
> +
> +	return single_open(f, xpsgtr_status_read, gtr_phy);
> +}
> +
> +static const struct file_operations xpsgtr_status_ops = {
> +	.owner = THIS_MODULE,
> +	.open = xpsgtr_status_open,
> +	.release = single_release,
> +	.read = seq_read,
> +	.llseek = seq_lseek
> +};

There are debugfs simple helpers which should help you avoid all this
open coding and just have the read call

> +
>  /*
>   * Power Management
>   */
> @@ -917,6 +969,8 @@ static int xpsgtr_probe(struct platform_device *pdev)
>  
>  		gtr_phy->phy = phy;
>  		phy_set_drvdata(phy, gtr_phy);
> +		debugfs_create_file("status", 0444, phy->debugfs, gtr_phy,
> +				    &xpsgtr_status_ops);

generic status does not make sense, how about device-name-status

-- 
~Vinod

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

Re: [PATCH 3/3] phy: zynqmp: Add debugfs support

[Sean Anderson]

On 5/4/24 07:58, Vinod Koul wrote:
> On 22-04-24, 14:58, Sean Anderson wrote:
>> 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>
>> ---
>> 
>>  drivers/phy/xilinx/phy-zynqmp.c | 54 +++++++++++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>> 
>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
>> index 08c88dcd7799..e2e86943e9f3 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>
>> @@ -122,6 +123,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
>> @@ -768,6 +778,48 @@ 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 xpsgtr_phy *gtr_phy = seq->private;
>> +	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;
>> +}
>> +
>> +static int xpsgtr_status_open(struct inode *inode, struct file *f)
>> +{
>> +	struct xpsgtr_phy *gtr_phy = inode->i_private;
>> +
>> +	return single_open(f, xpsgtr_status_read, gtr_phy);
>> +}
>> +
>> +static const struct file_operations xpsgtr_status_ops = {
>> +	.owner = THIS_MODULE,
>> +	.open = xpsgtr_status_open,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +	.llseek = seq_lseek
>> +};
> 
> There are debugfs simple helpers which should help you avoid all this
> open coding and just have the read call

I assume you mean debugfs_create_devm_seqfile.

>> +
>>  /*
>>   * Power Management
>>   */
>> @@ -917,6 +969,8 @@ static int xpsgtr_probe(struct platform_device *pdev)
>>  
>>  		gtr_phy->phy = phy;
>>  		phy_set_drvdata(phy, gtr_phy);
>> +		debugfs_create_file("status", 0444, phy->debugfs, gtr_phy,
>> +				    &xpsgtr_status_ops);
> 
> generic status does not make sense, how about device-name-status

This file lives in the phy's debugfs directory already. The full path is

/sys/kernel/debug/phy/phy-fd400000.phy.0/status

so there is no need to add the phy name yet again.

--Sean

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