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 <[email protected]>
---
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
> -----Original Message-----
> From: Sean Anderson <[email protected]>
> Sent: Monday, May 6, 2024 10:31 PM
> To: Laurent Pinchart <[email protected]>; linux-
> [email protected]
> Cc: Vinod Koul <[email protected]>; [email protected];
> [email protected]; Michal Simek <[email protected]>;
> Kishon Vijay Abraham I <[email protected]>; Sean Anderson
> <[email protected]>
> 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 <[email protected]>
> ---
>
> 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
On 6/14/24 02:02, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <[email protected]>
>> Sent: Monday, May 6, 2024 10:31 PM
>> To: Laurent Pinchart <[email protected]>; linux-
>> [email protected]
>> Cc: Vinod Koul <[email protected]>; [email protected];
>> [email protected]; Michal Simek <[email protected]>;
>> Kishon Vijay Abraham I <[email protected]>; Sean Anderson
>> <[email protected]>
>> 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 <[email protected]>
>> ---
>>
>> 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
>