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

[Michal Simek <michal.simek@amd.com>]

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