| Message ID | 20240710115624.3232925-1-s-vadapalli@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] phy: cadence-torrent: add support for three or more links using 2 protocols | expand |
Hi Siddharth, On 10/07/2024 14:56, Siddharth Vadapalli wrote: > The Torrent SERDES can support at most two different protocols. This only Could you please point to where this is mentioned? Doesn't this SERDES support 4 lanes? So in theory each lane can be used as one protocol (or link) independed of the other. Also, from code struct cdns_torrent_phy { ... struct cdns_torrent_inst phys[MAX_NUM_LANES]; ... } and MAX_NUM_LANES is 4. And from Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml patternProperties: '^phy@[0-3]$': type: object description: Each group of PHY lanes with a single master lane should be represented as a sub-node. Which means it can have upto 4 phy nodes with different protocols. > 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 > Could you please give an example of device tree where existing implementation doesn't work for you. > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.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://lore.kernel.org/r/20240709120703.2716397-1-s-vadapalli@ti.com/ > 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.
On Wed, Jul 10, 2024 at 06:22:53PM +0300, Roger Quadros wrote: > Hi Siddharth, Hello Roger, Thank you for reviewing this patch. > > On 10/07/2024 14:56, Siddharth Vadapalli wrote: > > The Torrent SERDES can support at most two different protocols. This only > > Could you please point to where this is mentioned? Doesn't this SERDES support 4 lanes? > So in theory each lane can be used as one protocol (or link) independed of the other. The Torrent SERDES has two PLLs. So up to two different protocols can be supported. Please note that protocol is not the same as a link. I am defining the terms below for your convenience: Protocol Analogous to PHY_TYPE -> DP/PCIe/QSGMII/SGMII/USB/USXGMII/XAUI/XFI Lane A pair of differential signals for TX/RX. A Lane is configured to operate for a specified Protocol. Link A collection of one or more lanes configured for the same Protocol. Since there are two PLLs, at most two different Protocols can be supported with each PLL configured for the frequency corresponding to the respective Protocol. Each Lane can be configured to operate for any of the Protocols with the SERDES level constraint being that at most two different Protocols can be supported across all Lanes. > > Also, from code > > struct cdns_torrent_phy { > ... > struct cdns_torrent_inst phys[MAX_NUM_LANES]; > ... > } > > and MAX_NUM_LANES is 4. > > And from Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml > > patternProperties: > '^phy@[0-3]$': > type: object > description: > Each group of PHY lanes with a single master lane should be represented as a sub-node. > > Which means it can have upto 4 phy nodes with different protocols. I respectfully disagree with your conclusion. MAX_NUM_LANES is 4 since the Torrent SERDES has 4 Lanes. Additionally, the description: "Each group of PHY lanes with a single master lane should be represented as a sub-node." is referring to a Link. A sub-node is analogous to a Link. Based on what you have quoted above, the following statement: "Which means it can have upto 4 phy nodes with different protocols." doesn't seem obvious to me. Setting aside the Documentation for a moment, if we look at the SERDES driver, it will simply reject any configuration specified in the device-tree that has more than 2 sub-nodes i.e. Links. I am referring to the following section of the driver prior to this patch: static int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy) { .... /* Maximum 2 links (subnodes) are supported */ if (cdns_phy->nsubnodes != 2) return -EINVAL; .... } In other words, irrespective of what the Documentation says, more than two sub-nodes are not allowed. We cannot specify more than 2 Protocols with just two sub-nodes (Links). So we can configure all 4 Lanes of the SERDES for at-most two different protocols, which does match the SERDES Hardware limitation since it has 2 PLLs. > > > 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 > > > > Could you please give an example of device tree where existing implementation > doesn't work for you. As I have pointed in my response above, the existing driver rejects any configuration which has more than two sub-nodes in the device-tree. Each device-tree sub-node represents a Link. A Link can constitute of one or more lanes. The existing driver prior to this patch only allows specifying two Links. In the examples I have listed above in the commit message, though there are only 2 protocols, since 3 Links are necessary to represent the configuration, the SERDES driver will not configure the SERDES, though the SERDES hardware supports such a configuration as it is still only 2 protocols being configured. While I am not the author of this driver and therefore cannot be certain about it, my guess about the author's rationale behind the existing implementation is the following: Given that the SERDES supports at most two protocols, the SERDES driver needs to prevent the user from specifying more than two protocols and treat all such requests as INVALID. One way to do so, which the author seems to have chosen, is to limit the number of Links supported to 2. Since it is impossible to request more than 2 protocols with just 2 Links, such a constraint although more limiting than required, does the needful. This patch on the other hand tries to relax the artificial constraint imposed in this driver by redefining the constraint to match the SERDES Hardware limitation. So the constraint of at-most 2 Links is replaced with at-most 2 Protocols in this patch, thereby making the constraint reflect the true Hardware limitation. Also, apart from the configurations that I have tested below on J7200-EVM, on a custom board with the J784S4/TDA4AP SoC [1] which has 4 Instances of the 4-Lane Torrent SERDES, the following configurations have been verified simultaneously with the current patch: SERDES0 -> 1 Protocol, 2 Links Lane 0 -> PCIe, Lane 1 -> Unused, Lane 2 -> PCIe, Lane 3 -> Unused (Link1 -> Lane0, Link2 -> Lane 2) SERDES1 -> 1 Protocol, 2 Links Lane 0 -> PCIe, Lane 1 -> Unused, Lane 2 -> PCIe, Lane 3 -> Unused (Link1 -> Lane0, Link2 -> Lane 2) SERDES2 -> 2 Protocols, 3 Links Lanes 0 and 1 -> SGMII, Lane 2 -> QSGMII, Lane 3 -> SGMII (Link1 -> Lanes 0 and 1, Link2 -> Lane2, Link3 -> Lane 3) SERDES4 -> 2 Protocols, 2 Links Lanes 0 and 1 -> Unused, Lane 2 -> SGMII, Lane 3 -> USB (Link1 -> Lane2, Link2 -> Lane3) For more details regarding the above, please refer [2] [1] https://www.ti.com/product/TDA4AP-Q1 [2] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1383684/tda4ap-q1-limitations-for-configuration-for-serdes-lanes-when-using-qsgmii-sgmii-and-sgmii-usb3-mixed/ > > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.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 > > [...] Regards, Siddharth.
Hi Siddharth, > -----Original Message----- > From: Siddharth Vadapalli <s-vadapalli@ti.com> > Sent: Wednesday, July 10, 2024 5:26 PM > To: vkoul@kernel.org; kishon@kernel.org; p.zabel@pengutronix.de; Swapnil > Kashinath Jakhade <sjakhade@cadence.com>; rogerq@kernel.org; > thomas.richard@bootlin.com; theo.lebrun@bootlin.com; robh@kernel.org > Cc: linux-phy@lists.infradead.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; srk@ti.com; s-vadapalli@ti.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@ti.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@ti.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
On Thu, Jul 11, 2024 at 06:43:01AM +0000, Swapnil Kashinath Jakhade wrote: > Hi Siddharth, Hello Swapnil, Thank you for reviewing this patch. [...] > > - 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. I suppose that PCIe is the only such protocol since it can have different speeds despite a single protocol (Gen1, Gen2, Gen3...), unlike other protocols which have a fixed speed and therefore the PLL associated with them doesn't have to be reconfigured as the rate will never change. Please let me know if there are other protocols (maybe DP?) which also require such special handling. [...] > > + 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. I can update this patch to handle it differently for PCIe multi-link, but as of now I don't see any PCIe multi-link support in the current Torrent SERDES driver in Mainline Linux. > > 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 The patches you are referring to above seem to remove the constraint of a maximum of 2 links, __only__ when one of the protocols is PCIe [1]. However, that is not necessarily the only use-case for multiple links. Please consider the following valid use-case: SERDES Lane 0 -> SGMII SERDES Lane 1 -> SGMII SERDES Lane 2 -> QSGMII SERDES Lane 3 -> SGMII Representing the above in the device-tree will require 3 sub-nodes (links): &serdes { serdes_sgmii_link1: phy@0 { reg = <0>; cdns,num-lanes = <2>; #phy-cells = <0>; cdns,phy-type = <PHY_TYPE_SGMII>; resets = <&serdes_wiz 1>, <&serdes_wiz 2>; }; serdes_qsgmii_link2: phy@2 { reg = <2>; cdns,num-lanes = <1>; #phy-cells = <0>; cdns,phy-type = <PHY_TYPE_QSGMII>; resets = <&serdes_wiz 3>; }; serdes_sgmii_link3: phy@3 { reg = <3>; cdns,num-lanes = <1>; #phy-cells = <0>; cdns,phy-type = <PHY_TYPE_SGMII>; resets = <&serdes_wiz 4>; }; }; Such a configuration is valid since it is still using only 2 different protocols. But the existing driver doesn't allow splitting/alternating protocols. So any use-case is forced to conform to a consecutive allocation of protocols. This patch enables the aforementioned use-case and this has been validated for functionality on the J784S4 SoC with a custom board [2]. I believe that this patch can be extended further to support the PCIe Multi-link configuration as well. Please let me know your thoughts on this. [1] https://github.com/t-c-collab/linux/commit/fd87922da100b1ed30015333e037c506c510e932#diff-814f5e3b47c89595aa30310ec07ab7aa8ac96f2921f524c4f5cd536a2c3c5adbR2488 [2] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1383684/tda4ap-q1-limitations-for-configuration-for-serdes-lanes-when-using-qsgmii-sgmii-and-sgmii-usb3-mixed/
Siddharth On 11/07/2024 08:13, Siddharth Vadapalli wrote: > On Wed, Jul 10, 2024 at 06:22:53PM +0300, Roger Quadros wrote: >> Hi Siddharth, > > Hello Roger, > > Thank you for reviewing this patch. > >> >> On 10/07/2024 14:56, Siddharth Vadapalli wrote: >>> The Torrent SERDES can support at most two different protocols. This only >> >> Could you please point to where this is mentioned? Doesn't this SERDES support 4 lanes? >> So in theory each lane can be used as one protocol (or link) independed of the other. > > The Torrent SERDES has two PLLs. So up to two different protocols can be > supported. Please note that protocol is not the same as a link. I am > defining the terms below for your convenience: > > Protocol > Analogous to PHY_TYPE -> DP/PCIe/QSGMII/SGMII/USB/USXGMII/XAUI/XFI > > Lane > A pair of differential signals for TX/RX. A Lane is configured > to operate for a specified Protocol. > > Link > A collection of one or more lanes configured for the same Protocol. > > Since there are two PLLs, at most two different Protocols can be > supported with each PLL configured for the frequency corresponding to > the respective Protocol. > > Each Lane can be configured to operate for any of the Protocols with the > SERDES level constraint being that at most two different Protocols can > be supported across all Lanes. Thanks for the detailed explanation. > >> >> Also, from code >> >> struct cdns_torrent_phy { >> ... >> struct cdns_torrent_inst phys[MAX_NUM_LANES]; >> ... >> } >> >> and MAX_NUM_LANES is 4. >> >> And from Documentation/devicetree/bindings/phy/phy-cadence-torrent.yaml >> >> patternProperties: >> '^phy@[0-3]$': >> type: object >> description: >> Each group of PHY lanes with a single master lane should be represented as a sub-node. >> >> Which means it can have upto 4 phy nodes with different protocols. > > I respectfully disagree with your conclusion. MAX_NUM_LANES is 4 since > the Torrent SERDES has 4 Lanes. Additionally, the description: > "Each group of PHY lanes with a single master lane should be represented > as a sub-node." > is referring to a Link. A sub-node is analogous to a Link. Based on what > you have quoted above, the following statement: > "Which means it can have upto 4 phy nodes with different protocols." > doesn't seem obvious to me. in the pattern Properties: phy@[0-3] means phy@0, phy@1, phy@2, phy@3 That's why I said it can have 4 PHY nodes. But looks like code doesn't match the documentation. > > Setting aside the Documentation for a moment, if we look at the SERDES > driver, it will simply reject any configuration specified in the > device-tree that has more than 2 sub-nodes i.e. Links. > I am referring to the following section of the driver prior to this patch: > > static > int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy) > { > .... > /* Maximum 2 links (subnodes) are supported */ > if (cdns_phy->nsubnodes != 2) > return -EINVAL; > .... > } OK. now looking at hardware capability it looks like we can still have 4 subnodes (phys/links) as long as all of them don't need more than 2 PLLs. So Documentation is correct from that perspective. We will still need to update the documentation to reflect the 2 PLL/protocol limit? > > In other words, irrespective of what the Documentation says, more than > two sub-nodes are not allowed. We cannot specify more than 2 Protocols > with just two sub-nodes (Links). So we can configure all 4 Lanes of the > SERDES for at-most two different protocols, which does match the SERDES > Hardware limitation since it has 2 PLLs. > >> >>> 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 >>> >> >> Could you please give an example of device tree where existing implementation >> doesn't work for you. > > As I have pointed in my response above, the existing driver rejects any > configuration which has more than two sub-nodes in the device-tree. > Each device-tree sub-node represents a Link. A Link can constitute of > one or more lanes. The existing driver prior to this patch only allows > specifying two Links. In the examples I have listed above in the commit > message, though there are only 2 protocols, since 3 Links are necessary > to represent the configuration, the SERDES driver will not configure the > SERDES, though the SERDES hardware supports such a configuration as it > is still only 2 protocols being configured. > > While I am not the author of this driver and therefore cannot be certain > about it, my guess about the author's rationale behind the existing > implementation is the following: > Given that the SERDES supports at most two protocols, the SERDES driver > needs to prevent the user from specifying more than two protocols and > treat all such requests as INVALID. One way to do so, which the author > seems to have chosen, is to limit the number of Links supported to 2. > Since it is impossible to request more than 2 protocols with just 2 > Links, such a constraint although more limiting than required, does the > needful. > > This patch on the other hand tries to relax the artificial constraint > imposed in this driver by redefining the constraint to match the SERDES > Hardware limitation. So the constraint of at-most 2 Links is replaced > with at-most 2 Protocols in this patch, thereby making the constraint > reflect the true Hardware limitation. > > Also, apart from the configurations that I have tested below on > J7200-EVM, on a custom board with the J784S4/TDA4AP SoC [1] which > has 4 Instances of the 4-Lane Torrent SERDES, the following configurations > have been verified simultaneously with the current patch: > > SERDES0 -> 1 Protocol, 2 Links > Lane 0 -> PCIe, Lane 1 -> Unused, Lane 2 -> PCIe, Lane 3 -> Unused > (Link1 -> Lane0, Link2 -> Lane 2) > SERDES1 -> 1 Protocol, 2 Links > Lane 0 -> PCIe, Lane 1 -> Unused, Lane 2 -> PCIe, Lane 3 -> Unused > (Link1 -> Lane0, Link2 -> Lane 2) > SERDES2 -> 2 Protocols, 3 Links > Lanes 0 and 1 -> SGMII, Lane 2 -> QSGMII, Lane 3 -> SGMII > (Link1 -> Lanes 0 and 1, Link2 -> Lane2, Link3 -> Lane 3) > SERDES4 -> 2 Protocols, 2 Links > Lanes 0 and 1 -> Unused, Lane 2 -> SGMII, Lane 3 -> USB > (Link1 -> Lane2, Link2 -> Lane3) > > For more details regarding the above, please refer [2] > > [1] https://www.ti.com/product/TDA4AP-Q1 > [2] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1383684/tda4ap-q1-limitations-for-configuration-for-serdes-lanes-when-using-qsgmii-sgmii-and-sgmii-usb3-mixed/ > >> >>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.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 >>> > > [...] > > Regards, > Siddharth.
Siddharth, On 11/07/2024 10:53, Siddharth Vadapalli wrote: > On Thu, Jul 11, 2024 at 06:43:01AM +0000, Swapnil Kashinath Jakhade wrote: >> Hi Siddharth, > > Hello Swapnil, > > Thank you for reviewing this patch. > > [...] > >>> - 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. > > I suppose that PCIe is the only such protocol since it can have > different speeds despite a single protocol (Gen1, Gen2, Gen3...), unlike > other protocols which have a fixed speed and therefore the PLL > associated with them doesn't have to be reconfigured as the rate will > never change. Please let me know if there are other protocols (maybe DP?) > which also require such special handling. The constraint is PLL frequency and not protocols as such right? e.g. If there are 4 protocols that all use same PLL frequency then we should be able to support it? How about updating the patch to limit on number of PLL frequencies rather than number of protocols? This should deal with PCIe multi-link case as well. > > [...] > >>> + 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. > > I can update this patch to handle it differently for PCIe multi-link, but > as of now I don't see any PCIe multi-link support in the current Torrent > SERDES driver in Mainline Linux. > >> >> 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 > > The patches you are referring to above seem to remove the constraint of > a maximum of 2 links, __only__ when one of the protocols is PCIe [1]. > However, that is not necessarily the only use-case for multiple links. > > Please consider the following valid use-case: > SERDES Lane 0 -> SGMII > SERDES Lane 1 -> SGMII > SERDES Lane 2 -> QSGMII > SERDES Lane 3 -> SGMII > Representing the above in the device-tree will require 3 sub-nodes > (links): > &serdes { > serdes_sgmii_link1: phy@0 { > reg = <0>; > cdns,num-lanes = <2>; > #phy-cells = <0>; > cdns,phy-type = <PHY_TYPE_SGMII>; > resets = <&serdes_wiz 1>, <&serdes_wiz 2>; > }; > serdes_qsgmii_link2: phy@2 { > reg = <2>; > cdns,num-lanes = <1>; > #phy-cells = <0>; > cdns,phy-type = <PHY_TYPE_QSGMII>; > resets = <&serdes_wiz 3>; > }; > serdes_sgmii_link3: phy@3 { > reg = <3>; > cdns,num-lanes = <1>; > #phy-cells = <0>; > cdns,phy-type = <PHY_TYPE_SGMII>; > resets = <&serdes_wiz 4>; > }; > }; > > Such a configuration is valid since it is still using only 2 different > protocols. But the existing driver doesn't allow splitting/alternating > protocols. So any use-case is forced to conform to a consecutive allocation > of protocols. This patch enables the aforementioned use-case and this has > been validated for functionality on the J784S4 SoC with a custom board [2]. > > I believe that this patch can be extended further to support the PCIe > Multi-link configuration as well. Please let me know your thoughts on > this. > > [1] https://github.com/t-c-collab/linux/commit/fd87922da100b1ed30015333e037c506c510e932#diff-814f5e3b47c89595aa30310ec07ab7aa8ac96f2921f524c4f5cd536a2c3c5adbR2488 > [2] https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1383684/tda4ap-q1-limitations-for-configuration-for-serdes-lanes-when-using-qsgmii-sgmii-and-sgmii-usb3-mixed/
Roger, Swapnil, On Thu, Jul 11, 2024 at 11:04:57AM +0300, Roger Quadros wrote: > Siddharth, > > On 11/07/2024 10:53, Siddharth Vadapalli wrote: [...] > > I suppose that PCIe is the only such protocol since it can have > > different speeds despite a single protocol (Gen1, Gen2, Gen3...), unlike > > other protocols which have a fixed speed and therefore the PLL > > associated with them doesn't have to be reconfigured as the rate will > > never change. Please let me know if there are other protocols (maybe DP?) > > which also require such special handling. > > The constraint is PLL frequency and not protocols as such right? > e.g. If there are 4 protocols that all use same PLL frequency then we should > be able to support it? > > How about updating the patch to limit on number of PLL frequencies rather than > number of protocols? This should deal with PCIe multi-link case as well. I suppose that an indirect way of determining whether a configuration can be supported or not is by checking if an entry exists in the "tables" (link_cmn_vals_tbl). That should be accurate since it reflects what the driver supports. I will update this patch accordingly so that Swapnil's inputs regarding PCIe Multi-link are also addressed. I am describing the logic for the updated patch below. Please share your feedback. 1. All single-link configurations (1 sub-node) can have only one protocol and will be handled via the "phy_ops" callbacks namely: .init, .power_on, ... No change will be made to this existing implementation. 2. All multi-link configurations (2 or more sub-nodes) have to be configured via cdns_torrent_phy_configure_multilink(). CASE-1 (2 Links/Sub-nodes): Check if there is an entry in "link_cmn_vals_entries" for the requested configuration and configure accordingly. This should handle the PCIe Multi-link configuration as well as other similar configurations which have a single protocol but cannot be treated as two single link configurations performed successively for each link. CASE-2 (3 or more Links/Sub-nodes): The links shall be grouped together by the protocol. Since we eventually have to look for an entry in "link_cmn_vals_entries", it is safe to impose the constraint that there shouldn't be more than 2 Protocols as the table is of the form: (phy_type1, phy_type2) i.e. Protocol 1, Protocol 2. It is guaranteed to be the case that Protocol1 != Protocol2 due to the following reason: If Protocol 1 == Protocol 2, it could have been represented in the device-tree using either: a) single link (sub-node) b) double link (sub-node) -> Special cases like PCIe Multi-link So assuming the above, we can enforce the constraint that there should be only 2 Protocols when 3 or more Links are present in the device-tree. This also handles the cases of PCIe Multi-Link + USB, PCIe Multi-Link + Q/SGMII which Swapnil has pointed to at [1], since PCIe Multi-Link is now a new protocol in itself (PHY_TYPE_PCIE_ML) and shall be represented in that manner in the device-tree when it is expected to be combined with a second protocol. After grouping the links by protocol, we can check for the entry in "link_cmn_vals_entries" and proceed to configure it identical to the 2 Link case. [1] https://github.com/t-c-collab/linux/commits/ti-linux-6.1.y-torrent-multi-pcie-sgmii-v1 Regards, Siddharth.
Hi Siddhath, On 12/07/2024 13:38, Siddharth Vadapalli wrote: > Roger, Swapnil, > > On Thu, Jul 11, 2024 at 11:04:57AM +0300, Roger Quadros wrote: >> Siddharth, >> >> On 11/07/2024 10:53, Siddharth Vadapalli wrote: > > [...] > >>> I suppose that PCIe is the only such protocol since it can have >>> different speeds despite a single protocol (Gen1, Gen2, Gen3...), unlike >>> other protocols which have a fixed speed and therefore the PLL >>> associated with them doesn't have to be reconfigured as the rate will >>> never change. Please let me know if there are other protocols (maybe DP?) >>> which also require such special handling. >> >> The constraint is PLL frequency and not protocols as such right? >> e.g. If there are 4 protocols that all use same PLL frequency then we should >> be able to support it? >> >> How about updating the patch to limit on number of PLL frequencies rather than >> number of protocols? This should deal with PCIe multi-link case as well. > > I suppose that an indirect way of determining whether a configuration > can be supported or not is by checking if an entry exists in the "tables" > (link_cmn_vals_tbl). That should be accurate since it reflects what the > driver supports. > > I will update this patch accordingly so that Swapnil's inputs regarding > PCIe Multi-link are also addressed. > > I am describing the logic for the updated patch below. Please share your > feedback. > > 1. All single-link configurations (1 sub-node) can have only one > protocol and will be handled via the "phy_ops" callbacks namely: > .init, .power_on, ... > No change will be made to this existing implementation. > > 2. All multi-link configurations (2 or more sub-nodes) have to be > configured via cdns_torrent_phy_configure_multilink(). > > CASE-1 (2 Links/Sub-nodes): > Check if there is an entry in "link_cmn_vals_entries" for the requested > configuration and configure accordingly. This should handle the PCIe > Multi-link configuration as well as other similar configurations which > have a single protocol but cannot be treated as two single link > configurations performed successively for each link. > > CASE-2 (3 or more Links/Sub-nodes): > > The links shall be grouped together by the protocol. Since we eventually > have to look for an entry in "link_cmn_vals_entries", it is safe to impose > the constraint that there shouldn't be more than 2 Protocols as the table > is of the form: > (phy_type1, phy_type2) > i.e. Protocol 1, Protocol 2. > It is guaranteed to be the case that Protocol1 != Protocol2 due to the > following reason: > If Protocol 1 == Protocol 2, it could have been represented in the > device-tree using either: > a) single link (sub-node) > b) double link (sub-node) -> Special cases like PCIe Multi-link > > So assuming the above, we can enforce the constraint that there should > be only 2 Protocols when 3 or more Links are present in the device-tree. > This also handles the cases of > PCIe Multi-Link + USB, PCIe Multi-Link + Q/SGMII > which Swapnil has pointed to at [1], since PCIe Multi-Link is now a new > protocol in itself (PHY_TYPE_PCIE_ML) and shall be represented in that > manner in the device-tree when it is expected to be combined with a second > protocol. > > After grouping the links by protocol, we can check for the entry in > "link_cmn_vals_entries" and proceed to configure it identical to the > 2 Link case. > > [1] https://github.com/t-c-collab/linux/commits/ti-linux-6.1.y-torrent-multi-pcie-sgmii-v1 This proposal looks good to me. Thanks!
On Mon, Jul 15, 2024 at 07:08:30PM +0300, Roger Quadros wrote: > Hi Siddhath, > > On 12/07/2024 13:38, Siddharth Vadapalli wrote: > > Roger, Swapnil, > > [...] > > So assuming the above, we can enforce the constraint that there should > > be only 2 Protocols when 3 or more Links are present in the device-tree. > > This also handles the cases of > > PCIe Multi-Link + USB, PCIe Multi-Link + Q/SGMII > > which Swapnil has pointed to at [1], since PCIe Multi-Link is now a new > > protocol in itself (PHY_TYPE_PCIE_ML) and shall be represented in that > > manner in the device-tree when it is expected to be combined with a second > > protocol. > > > > After grouping the links by protocol, we can check for the entry in > > "link_cmn_vals_entries" and proceed to configure it identical to the > > 2 Link case. > > > > [1] https://github.com/t-c-collab/linux/commits/ti-linux-6.1.y-torrent-multi-pcie-sgmii-v1 > > This proposal looks good to me. Thanks! Thank you for the confirmation. I will implement the above in the v3 patch. Regards, Siddharth.
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; + } /** * 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++;
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@ti.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://lore.kernel.org/r/20240709120703.2716397-1-s-vadapalli@ti.com/ 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. Regards, Siddharth. drivers/phy/cadence/phy-cadence-torrent.c | 252 ++++++++++++---------- 1 file changed, 136 insertions(+), 116 deletions(-)