Message-ID: 
 <LV3PR07MB1036463AB8AB5D38D003175E6C5A52@LV3PR07MB10364.namprd07.prod.outlook.com>
Date: Thu, 11 Jul 2024 06:43:01 +0000
From: Swapnil Kashinath Jakhade <sjakhade@...ence.com>
To: Siddharth Vadapalli <s-vadapalli@...com>,
        "vkoul@...nel.org"
	<vkoul@...nel.org>,
        "kishon@...nel.org" <kishon@...nel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "rogerq@...nel.org"
	<rogerq@...nel.org>,
        "thomas.richard@...tlin.com"
	<thomas.richard@...tlin.com>,
        "theo.lebrun@...tlin.com"
	<theo.lebrun@...tlin.com>,
        "robh@...nel.org" <robh@...nel.org>
CC: "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>,
        "srk@...com" <srk@...com>, Milind
 Parab <mparab@...ence.com>
Subject: RE: [PATCH v2] phy: cadence-torrent: add support for three or more
 links using 2 protocols

Hi Siddharth,

> -----Original Message-----
> From: Siddharth Vadapalli <s-vadapalli@...com>
> Sent: Wednesday, July 10, 2024 5:26 PM
> To: vkoul@...nel.org; kishon@...nel.org; p.zabel@...gutronix.de; Swapnil
> Kashinath Jakhade <sjakhade@...ence.com>; rogerq@...nel.org;
> thomas.richard@...tlin.com; theo.lebrun@...tlin.com; robh@...nel.org
> Cc: linux-phy@...ts.infradead.org; linux-kernel@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; srk@...com; s-vadapalli@...com
> Subject: [PATCH v2] phy: cadence-torrent: add support for three or more links
> using 2 protocols
> 
> EXTERNAL MAIL
> 
> 
> The Torrent SERDES can support at most two different protocols. This only
> mandates that the device-tree sub-nodes expressing the configuration should
> describe links with at-most two different protocols.
> 
> The existing implementation however imposes an artificial constraint that
> allows only two links (device-tree sub-nodes). As long as at-most two
> protocols are chosen, using more than two links to describe them in an
> alternating configuration is still a valid configuration of the Torrent
> SERDES.
> 
> A 3-Link 2-Protocol configuration of the 4-Lane SERDES can be:
> Lane 0 => Protocol 1 => Link 1
> Lane 1 => Protocol 1 => Link 1
> Lane 2 => Protocol 2 => Link 2
> Lane 3 => Protocol 1 => Link 3
> 
> A 4-Link 2-Protocol configuration of the 4-Lane SERDES can be:
> Lane 0 => Protocol 1 => Link 1
> Lane 1 => Protocol 2 => Link 2
> Lane 2 => Protocol 1 => Link 3
> Lane 3 => Protocol 2 => Link 4
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@...com>
> ---
> 
> Hello,
> 
> This patch is based on linux-next tagged next-20240710.
> Patch has been sanity tested and tested for functionality in the following
> configurations with the Torrent SERDES0 on J7200-EVM:
> 1. PCIe (Lanes 0 and 1) + QSGMII (Lane 2)
>    => 2 protocols, 2 links
> 2. PCIe (Lane0 as 1 Link) + PCIe (Lane 1 as 1 Link)
>    => 1 protocol, 2 links
> 3. PCIe (Lanes 0 and 1) + QSGMII (Lane 2) + PCIe (Lane 3)
>    => 2 protocols, 3 links
> 
> v1:
> https://urldefense.com/v3/__https://lore.kernel.org/r/20240709120703.27163
> 97-1-s-vadapalli@...com/__;!!EHscmS1ygiU1lA!HVLtdWbkUa1JK0NIXiJM7F-
> Db2kR5c-mgeDMtqa4i7c8-efmAsDWYAloP0KmI6OOx2NKr7v39FZx5hG8bg$
> Changes since v1:
> - A multilink configuration doesn't necessarily imply two protocols
>   since a single protocol may be split across links as follows:
>   Lane 0 => Protocol 1
>   Lane 1 => Unused
>   Lane 2 => Protocol 1
>   Lane 3 => Unused
>   which corresponds to two links and therefore two sub-nodes. In such a
>   case, treat it as two single-link configurations performed sequentially
>   which happens to be the case prior to this patch. To address this,
>   handle the case where cdns_torrent_phy_configure_multilink() can be
>   invoked for a single protocol with multiple sub-nodes (links) by
>   erroring out only when the number of protocols is strictly greater
>   than two, followed by handling the configuration similar to how it was
>   done prior to this patch.

The assumption here that "a single protocol when split across links is to be
considered as single-link configurations performed sequentially" is not always
correct.
e.g. If there are 2 PCIe links, then this is a case of 'Multilink PCIe' and not 2 separate
'single link PCIe'. Multilink PCIe has different PLL configurations than for single link
PCIe resulting in different register configurations.
Also, for multi-protocol case, in cdns_torrent_phy_configure_multilink() function,
the existing implementation considers this as multilink case of combination of 2
protocols which has different register config than single link of each protocol.

> 
> Regards,
> Siddharth.
> 
>  drivers/phy/cadence/phy-cadence-torrent.c | 252 ++++++++++++----------
>  1 file changed, 136 insertions(+), 116 deletions(-)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> b/drivers/phy/cadence/phy-cadence-torrent.c
> index 56ce82a47f88..a6d0082e448d 100644
> --- a/drivers/phy/cadence/phy-cadence-torrent.c
> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> @@ -351,6 +351,7 @@ struct cdns_torrent_phy {
>  	void __iomem *sd_base; /* SD0801 registers base */
>  	u32 max_bit_rate; /* Maximum link bit rate to use (in Mbps) */
>  	u32 dp_pll;
> +	u32 protocol_bitmask;
>  	struct reset_control *phy_rst;
>  	struct reset_control *apb_rst;
>  	struct device *dev;
> @@ -2475,21 +2476,32 @@ int cdns_torrent_phy_configure_multilink(struct
> cdns_torrent_phy *cdns_phy)
>  	struct cdns_reg_pairs *reg_pairs;
>  	enum cdns_torrent_ssc_mode ssc;
>  	struct regmap *regmap;
> -	u32 num_regs;
> +	u32 num_regs, num_protocols, protocol;
> 
> -	/* Maximum 2 links (subnodes) are supported */
> -	if (cdns_phy->nsubnodes != 2)
> +	num_protocols = hweight32(cdns_phy->protocol_bitmask);
> +	/* Maximum 2 protocols are supported */
> +	if (num_protocols > 2) {
>  		return -EINVAL;
> -
> -	phy_t1 = cdns_phy->phys[0].phy_type;
> -	phy_t2 = cdns_phy->phys[1].phy_type;
> +	} else if (num_protocols == 2) {
> +		phy_t1 = fns(cdns_phy->protocol_bitmask, 0);
> +		phy_t2 = fns(cdns_phy->protocol_bitmask, 1);
> +	} else {
> +		phy_t1 = fns(cdns_phy->protocol_bitmask, 0);
> +		/**
> +		 * For a single protocol split across multiple links,
> +		 * assign TYPE_NONE to phy_t2 for configuring each link
> +		 * identical to a single-link configuration with a single
> +		 * protocol.
> +		 */
> +		phy_t2 = TYPE_NONE;

As mentioned above, assuming phy_t2 = TYPE_NONE is not correct here.

FYI. I have shared few patches to TI earlier removing the constraint of Maximum 2 links (subnodes)
in the driver specifically for Multilink PCIe cases.
Please check first 4 patches in link below.
https://github.com/t-c-collab/linux/commits/ti-linux-6.1.y-torrent-multi-pcie-sgmii-v1

Thanks & regards,
Swapnil

> +	}
> 
>  	/**
>  	 * First configure the PHY for first link with phy_t1. Get the array
>  	 * values as [phy_t1][phy_t2][ssc].
>  	 */
> -	for (node = 0; node < cdns_phy->nsubnodes; node++) {
> -		if (node == 1) {
> +	for (protocol = 0; protocol < num_protocols; protocol++) {
> +		if (protocol == 1) {
>  			/**
>  			 * If first link with phy_t1 is configured, then
>  			 * configure the PHY for second link with phy_t2.
> @@ -2499,130 +2511,136 @@ int cdns_torrent_phy_configure_multilink(struct
> cdns_torrent_phy *cdns_phy)
>  			swap(ref_clk, ref_clk1);
>  		}
> 
> -		mlane = cdns_phy->phys[node].mlane;
> -		ssc = cdns_phy->phys[node].ssc_mode;
> -		num_lanes = cdns_phy->phys[node].num_lanes;
> +		for (node = 0; node < cdns_phy->nsubnodes; node++) {
> +			if (cdns_phy->phys[node].phy_type != phy_t1)
> +				continue;
> 
> -		/**
> -		 * PHY configuration specific registers:
> -		 * link_cmn_vals depend on combination of PHY types being
> -		 * configured and are common for both PHY types, so array
> -		 * values should be same for [phy_t1][phy_t2][ssc] and
> -		 * [phy_t2][phy_t1][ssc].
> -		 * xcvr_diag_vals also depend on combination of PHY types
> -		 * being configured, but these can be different for particular
> -		 * PHY type and are per lane.
> -		 */
> -		link_cmn_vals = cdns_torrent_get_tbl_vals(&init_data-
> >link_cmn_vals_tbl,
> -							  CLK_ANY, CLK_ANY,
> -							  phy_t1, phy_t2,
> ANY_SSC);
> -		if (link_cmn_vals) {
> -			reg_pairs = link_cmn_vals->reg_pairs;
> -			num_regs = link_cmn_vals->num_regs;
> -			regmap = cdns_phy->regmap_common_cdb;
> +			mlane = cdns_phy->phys[node].mlane;
> +			ssc = cdns_phy->phys[node].ssc_mode;
> +			num_lanes = cdns_phy->phys[node].num_lanes;
> 
>  			/**
> -			 * First array value in link_cmn_vals must be of
> -			 * PHY_PLL_CFG register
> +			 * PHY configuration specific registers:
> +			 * link_cmn_vals depend on combination of PHY types
> being
> +			 * configured and are common for both PHY types, so
> array
> +			 * values should be same for [phy_t1][phy_t2][ssc] and
> +			 * [phy_t2][phy_t1][ssc].
> +			 * xcvr_diag_vals also depend on combination of PHY
> types
> +			 * being configured, but these can be different for
> particular
> +			 * PHY type and are per lane.
>  			 */
> -			regmap_field_write(cdns_phy->phy_pll_cfg,
> -					   reg_pairs[0].val);
> +			link_cmn_vals = cdns_torrent_get_tbl_vals(&init_data-
> >link_cmn_vals_tbl,
> +								  CLK_ANY,
> CLK_ANY,
> +								  phy_t1,
> phy_t2, ANY_SSC);
> +			if (link_cmn_vals) {
> +				reg_pairs = link_cmn_vals->reg_pairs;
> +				num_regs = link_cmn_vals->num_regs;
> +				regmap = cdns_phy->regmap_common_cdb;
> 
> -			for (i = 1; i < num_regs; i++)
> -				regmap_write(regmap, reg_pairs[i].off,
> -					     reg_pairs[i].val);
> -		}
> +				/**
> +				 * First array value in link_cmn_vals must be of
> +				 * PHY_PLL_CFG register
> +				 */
> +				regmap_field_write(cdns_phy->phy_pll_cfg,
> +						   reg_pairs[0].val);
> 
> -		xcvr_diag_vals = cdns_torrent_get_tbl_vals(&init_data-
> >xcvr_diag_vals_tbl,
> -							   CLK_ANY, CLK_ANY,
> -							   phy_t1, phy_t2,
> ANY_SSC);
> -		if (xcvr_diag_vals) {
> -			reg_pairs = xcvr_diag_vals->reg_pairs;
> -			num_regs = xcvr_diag_vals->num_regs;
> -			for (i = 0; i < num_lanes; i++) {
> -				regmap = cdns_phy->regmap_tx_lane_cdb[i +
> mlane];
> -				for (j = 0; j < num_regs; j++)
> -					regmap_write(regmap,
> reg_pairs[j].off,
> -						     reg_pairs[j].val);
> +				for (i = 1; i < num_regs; i++)
> +					regmap_write(regmap,
> reg_pairs[i].off,
> +						     reg_pairs[i].val);
>  			}
> -		}
> 
> -		/* PHY PCS common registers configurations */
> -		pcs_cmn_vals = cdns_torrent_get_tbl_vals(&init_data-
> >pcs_cmn_vals_tbl,
> -							 CLK_ANY, CLK_ANY,
> -							 phy_t1, phy_t2,
> ANY_SSC);
> -		if (pcs_cmn_vals) {
> -			reg_pairs = pcs_cmn_vals->reg_pairs;
> -			num_regs = pcs_cmn_vals->num_regs;
> -			regmap = cdns_phy->regmap_phy_pcs_common_cdb;
> -			for (i = 0; i < num_regs; i++)
> -				regmap_write(regmap, reg_pairs[i].off,
> -					     reg_pairs[i].val);
> -		}
> +			xcvr_diag_vals = cdns_torrent_get_tbl_vals(&init_data-
> >xcvr_diag_vals_tbl,
> +								   CLK_ANY,
> CLK_ANY,
> +								   phy_t1,
> phy_t2, ANY_SSC);
> +			if (xcvr_diag_vals) {
> +				reg_pairs = xcvr_diag_vals->reg_pairs;
> +				num_regs = xcvr_diag_vals->num_regs;
> +				for (i = 0; i < num_lanes; i++) {
> +					regmap = cdns_phy-
> >regmap_tx_lane_cdb[i + mlane];
> +					for (j = 0; j < num_regs; j++)
> +						regmap_write(regmap,
> reg_pairs[j].off,
> +							     reg_pairs[j].val);
> +				}
> +			}
> 
> -		/* PHY PMA common registers configurations */
> -		phy_pma_cmn_vals = cdns_torrent_get_tbl_vals(&init_data-
> >phy_pma_cmn_vals_tbl,
> -							     CLK_ANY, CLK_ANY,
> -							     phy_t1, phy_t2,
> ANY_SSC);
> -		if (phy_pma_cmn_vals) {
> -			reg_pairs = phy_pma_cmn_vals->reg_pairs;
> -			num_regs = phy_pma_cmn_vals->num_regs;
> -			regmap = cdns_phy->regmap_phy_pma_common_cdb;
> -			for (i = 0; i < num_regs; i++)
> -				regmap_write(regmap, reg_pairs[i].off,
> -					     reg_pairs[i].val);
> -		}
> +			/* PHY PCS common registers configurations */
> +			pcs_cmn_vals = cdns_torrent_get_tbl_vals(&init_data-
> >pcs_cmn_vals_tbl,
> +								 CLK_ANY,
> CLK_ANY,
> +								 phy_t1,
> phy_t2, ANY_SSC);
> +			if (pcs_cmn_vals) {
> +				reg_pairs = pcs_cmn_vals->reg_pairs;
> +				num_regs = pcs_cmn_vals->num_regs;
> +				regmap = cdns_phy-
> >regmap_phy_pcs_common_cdb;
> +				for (i = 0; i < num_regs; i++)
> +					regmap_write(regmap,
> reg_pairs[i].off,
> +						     reg_pairs[i].val);
> +			}
> 
> -		/* PMA common registers configurations */
> -		cmn_vals = cdns_torrent_get_tbl_vals(&init_data-
> >cmn_vals_tbl,
> -						     ref_clk, ref_clk1,
> -						     phy_t1, phy_t2, ssc);
> -		if (cmn_vals) {
> -			reg_pairs = cmn_vals->reg_pairs;
> -			num_regs = cmn_vals->num_regs;
> -			regmap = cdns_phy->regmap_common_cdb;
> -			for (i = 0; i < num_regs; i++)
> -				regmap_write(regmap, reg_pairs[i].off,
> -					     reg_pairs[i].val);
> -		}
> +			/* PHY PMA common registers configurations */
> +			phy_pma_cmn_vals =
> +				cdns_torrent_get_tbl_vals(&init_data-
> >phy_pma_cmn_vals_tbl,
> +							  CLK_ANY, CLK_ANY,
> phy_t1, phy_t2,
> +							  ANY_SSC);
> +			if (phy_pma_cmn_vals) {
> +				reg_pairs = phy_pma_cmn_vals->reg_pairs;
> +				num_regs = phy_pma_cmn_vals->num_regs;
> +				regmap = cdns_phy-
> >regmap_phy_pma_common_cdb;
> +				for (i = 0; i < num_regs; i++)
> +					regmap_write(regmap,
> reg_pairs[i].off,
> +						     reg_pairs[i].val);
> +			}
> 
> -		/* PMA TX lane registers configurations */
> -		tx_ln_vals = cdns_torrent_get_tbl_vals(&init_data-
> >tx_ln_vals_tbl,
> -						       ref_clk, ref_clk1,
> -						       phy_t1, phy_t2, ssc);
> -		if (tx_ln_vals) {
> -			reg_pairs = tx_ln_vals->reg_pairs;
> -			num_regs = tx_ln_vals->num_regs;
> -			for (i = 0; i < num_lanes; i++) {
> -				regmap = cdns_phy->regmap_tx_lane_cdb[i +
> mlane];
> -				for (j = 0; j < num_regs; j++)
> -					regmap_write(regmap,
> reg_pairs[j].off,
> -						     reg_pairs[j].val);
> +			/* PMA common registers configurations */
> +			cmn_vals = cdns_torrent_get_tbl_vals(&init_data-
> >cmn_vals_tbl,
> +							     ref_clk, ref_clk1,
> +							     phy_t1, phy_t2,
> ssc);
> +			if (cmn_vals) {
> +				reg_pairs = cmn_vals->reg_pairs;
> +				num_regs = cmn_vals->num_regs;
> +				regmap = cdns_phy->regmap_common_cdb;
> +				for (i = 0; i < num_regs; i++)
> +					regmap_write(regmap,
> reg_pairs[i].off,
> +						     reg_pairs[i].val);
>  			}
> -		}
> 
> -		/* PMA RX lane registers configurations */
> -		rx_ln_vals = cdns_torrent_get_tbl_vals(&init_data-
> >rx_ln_vals_tbl,
> -						       ref_clk, ref_clk1,
> -						       phy_t1, phy_t2, ssc);
> -		if (rx_ln_vals) {
> -			reg_pairs = rx_ln_vals->reg_pairs;
> -			num_regs = rx_ln_vals->num_regs;
> -			for (i = 0; i < num_lanes; i++) {
> -				regmap = cdns_phy->regmap_rx_lane_cdb[i +
> mlane];
> -				for (j = 0; j < num_regs; j++)
> -					regmap_write(regmap,
> reg_pairs[j].off,
> -						     reg_pairs[j].val);
> +			/* PMA TX lane registers configurations */
> +			tx_ln_vals = cdns_torrent_get_tbl_vals(&init_data-
> >tx_ln_vals_tbl,
> +							       ref_clk, ref_clk1,
> +							       phy_t1, phy_t2,
> ssc);
> +			if (tx_ln_vals) {
> +				reg_pairs = tx_ln_vals->reg_pairs;
> +				num_regs = tx_ln_vals->num_regs;
> +				for (i = 0; i < num_lanes; i++) {
> +					regmap = cdns_phy-
> >regmap_tx_lane_cdb[i + mlane];
> +					for (j = 0; j < num_regs; j++)
> +						regmap_write(regmap,
> reg_pairs[j].off,
> +							     reg_pairs[j].val);
> +				}
>  			}
> -		}
> 
> -		if (phy_t1 == TYPE_DP) {
> -			ret = cdns_torrent_dp_get_pll(cdns_phy, phy_t2);
> -			if (ret)
> -				return ret;
> -		}
> +			/* PMA RX lane registers configurations */
> +			rx_ln_vals = cdns_torrent_get_tbl_vals(&init_data-
> >rx_ln_vals_tbl,
> +							       ref_clk, ref_clk1,
> +							       phy_t1, phy_t2,
> ssc);
> +			if (rx_ln_vals) {
> +				reg_pairs = rx_ln_vals->reg_pairs;
> +				num_regs = rx_ln_vals->num_regs;
> +				for (i = 0; i < num_lanes; i++) {
> +					regmap = cdns_phy-
> >regmap_rx_lane_cdb[i + mlane];
> +					for (j = 0; j < num_regs; j++)
> +						regmap_write(regmap,
> reg_pairs[j].off,
> +							     reg_pairs[j].val);
> +				}
> +			}
> 
> -		reset_control_deassert(cdns_phy->phys[node].lnk_rst);
> +			if (phy_t1 == TYPE_DP) {
> +				ret = cdns_torrent_dp_get_pll(cdns_phy,
> phy_t2);
> +				if (ret)
> +					return ret;
> +			}
> +
> +			reset_control_deassert(cdns_phy->phys[node].lnk_rst);
> +		}
>  	}
> 
>  	/* Take the PHY out of reset */
> @@ -2826,6 +2844,7 @@ static int cdns_torrent_phy_probe(struct
> platform_device *pdev)
>  	dev_set_drvdata(dev, cdns_phy);
>  	cdns_phy->dev = dev;
>  	cdns_phy->init_data = data;
> +	cdns_phy->protocol_bitmask = 0;
> 
>  	cdns_phy->sd_base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(cdns_phy->sd_base))
> @@ -3010,6 +3029,7 @@ static int cdns_torrent_phy_probe(struct
> platform_device *pdev)
>  		}
> 
>  		cdns_phy->phys[node].phy = gphy;
> +		cdns_phy->protocol_bitmask |= BIT(cdns_phy-
> >phys[node].phy_type);
>  		phy_set_drvdata(gphy, &cdns_phy->phys[node]);
> 
>  		node++;
> --
> 2.40.1

Powered by blists - more mailing lists