| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <48e124d7-94d3-47b2-82c3-db6d0d6fdefd@kernel.org>
Date: Fri, 22 Dec 2023 12:07:31 +0200
From: Roger Quadros <rogerq@...nel.org>
To: Swapnil Jakhade <sjakhade@...ence.com>, vkoul@...nel.org,
kishon@...nel.org, robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, linux-phy@...ts.infradead.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Cc: mparab@...ence.com, s-vadapalli@...com
Subject: Re: [PATCH v3 2/5] phy: cadence-torrent: Add PCIe(100MHz) +
USXGMII(156.25MHz) multilink configuration
Hi Swapnil,
On 21/12/2023 18:20, Swapnil Jakhade wrote:
> Torrent PHY can have separate input reference clocks for PLL0 and PLL1.
> Add support for dual reference clock multilink configurations.
>
> Add register sequences for PCIe(100MHz) + USXGMII(156.25MHz) multilink
> configuration. PCIe uses PLL0 and USXGMII uses PLL1.
>
> Signed-off-by: Swapnil Jakhade <sjakhade@...ence.com>
> ---
> drivers/phy/cadence/phy-cadence-torrent.c | 194 +++++++++++++++++++++-
> 1 file changed, 191 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
> index a75c96385c57..18ec49c9a76e 100644
> --- a/drivers/phy/cadence/phy-cadence-torrent.c
> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> @@ -355,7 +355,9 @@ struct cdns_torrent_phy {
> struct reset_control *apb_rst;
> struct device *dev;
> struct clk *clk;
> + struct clk *clk1;
> enum cdns_torrent_ref_clk ref_clk_rate;
> + enum cdns_torrent_ref_clk ref_clk1_rate;
> struct cdns_torrent_inst phys[MAX_NUM_LANES];
> int nsubnodes;
> const struct cdns_torrent_data *init_data;
> @@ -2460,9 +2462,11 @@ int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy)
> {
> const struct cdns_torrent_data *init_data = cdns_phy->init_data;
> struct cdns_torrent_vals *cmn_vals, *tx_ln_vals, *rx_ln_vals;
> + enum cdns_torrent_ref_clk ref_clk1 = cdns_phy->ref_clk1_rate;
> enum cdns_torrent_ref_clk ref_clk = cdns_phy->ref_clk_rate;
> struct cdns_torrent_vals *link_cmn_vals, *xcvr_diag_vals;
> enum cdns_torrent_phy_type phy_t1, phy_t2;
> + struct cdns_torrent_vals *phy_pma_cmn_vals;
> struct cdns_torrent_vals *pcs_cmn_vals;
> int i, j, node, mlane, num_lanes, ret;
> struct cdns_reg_pairs *reg_pairs;
> @@ -2489,6 +2493,7 @@ int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy)
> * Get the array values as [phy_t2][phy_t1][ssc].
> */
> swap(phy_t1, phy_t2);
> + swap(ref_clk, ref_clk1);
> }
>
> mlane = cdns_phy->phys[node].mlane;
> @@ -2552,9 +2557,22 @@ int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy)
> 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 common registers configurations */
> cmn_vals = cdns_torrent_get_tbl_vals(&init_data->cmn_vals_tbl,
> - ref_clk, ref_clk,
> + ref_clk, ref_clk1,
> phy_t1, phy_t2, ssc);
> if (cmn_vals) {
> reg_pairs = cmn_vals->reg_pairs;
> @@ -2567,7 +2585,7 @@ int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy)
>
> /* PMA TX lane registers configurations */
> tx_ln_vals = cdns_torrent_get_tbl_vals(&init_data->tx_ln_vals_tbl,
> - ref_clk, ref_clk,
> + ref_clk, ref_clk1,
> phy_t1, phy_t2, ssc);
> if (tx_ln_vals) {
> reg_pairs = tx_ln_vals->reg_pairs;
> @@ -2582,7 +2600,7 @@ int cdns_torrent_phy_configure_multilink(struct cdns_torrent_phy *cdns_phy)
>
> /* PMA RX lane registers configurations */
> rx_ln_vals = cdns_torrent_get_tbl_vals(&init_data->rx_ln_vals_tbl,
> - ref_clk, ref_clk,
> + ref_clk, ref_clk1,
> phy_t1, phy_t2, ssc);
> if (rx_ln_vals) {
> reg_pairs = rx_ln_vals->reg_pairs;
> @@ -2684,9 +2702,11 @@ static int cdns_torrent_reset(struct cdns_torrent_phy *cdns_phy)
> static int cdns_torrent_clk(struct cdns_torrent_phy *cdns_phy)
> {
> struct device *dev = cdns_phy->dev;
> + unsigned long ref_clk1_rate;
> unsigned long ref_clk_rate;
> int ret;
>
> + /* refclk: Input reference clock for PLL0 */
> cdns_phy->clk = devm_clk_get(dev, "refclk");
> if (IS_ERR(cdns_phy->clk)) {
> dev_err(dev, "phy ref clock not found\n");
> @@ -2725,7 +2745,54 @@ static int cdns_torrent_clk(struct cdns_torrent_phy *cdns_phy)
> return -EINVAL;
> }
>
> + /* refclk1: Input reference clock for PLL1 */
> + cdns_phy->clk1 = devm_clk_get_optional(dev, "pll1_refclk");
> + if (IS_ERR(cdns_phy->clk1)) {
> + dev_err(dev, "phy pll1 ref clock not found\n");
> + return PTR_ERR(cdns_phy->clk1);
Don't you need to disable cdns_phy->clk before exititing?
So
ret = PTR_ERR(cdns_phy->clk1);
goto disbale_clk;
> + }
> +
> + if (cdns_phy->clk1) {
> + ret = clk_prepare_enable(cdns_phy->clk1);
> + if (ret) {
> + dev_err(cdns_phy->dev, "Failed to prepare pll1 ref clock\n");
nit, pll should be all uppercase in error messages. so "PLL1"?
Do you wan't to dump the return error code as well so it is easier to report/debug?
> + clk_disable_unprepare(cdns_phy->clk);
> + return ret;
Instead of above 2 lines, just:
goto disable_clk.
> + }
> +
> + ref_clk1_rate = clk_get_rate(cdns_phy->clk1);
> + if (!ref_clk1_rate) {
> + dev_err(cdns_phy->dev, "Failed to get pll1 ref clock rate\n");
> + goto refclk1_err;
For consistency let's call this label disable_clk1
> + }
> +
> + switch (ref_clk1_rate) {
> + case REF_CLK_19_2MHZ:
> + cdns_phy->ref_clk1_rate = CLK_19_2_MHZ;
> + break;
> + case REF_CLK_25MHZ:
> + cdns_phy->ref_clk1_rate = CLK_25_MHZ;
> + break;
> + case REF_CLK_100MHZ:
> + cdns_phy->ref_clk1_rate = CLK_100_MHZ;
> + break;
> + case REF_CLK_156_25MHZ:
> + cdns_phy->ref_clk1_rate = CLK_156_25_MHZ;
> + break;
> + default:
> + dev_err(cdns_phy->dev, "Invalid pll1 ref clock rate\n");
> + goto refclk1_err;
> + }
> + } else {
> + cdns_phy->ref_clk1_rate = cdns_phy->ref_clk_rate;
> + }
> +
> return 0;
> +
> +refclk1_err:
> + clk_disable_unprepare(cdns_phy->clk1);
disable_clk:
> + clk_disable_unprepare(cdns_phy->clk);
> + return -EINVAL;
return ret. We need to preserve why the failure
happened so it is easier to debug.
> }
>
> static int cdns_torrent_phy_probe(struct platform_device *pdev)
> @@ -2981,6 +3048,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
> of_node_put(child);
> reset_control_assert(cdns_phy->apb_rst);
> clk_disable_unprepare(cdns_phy->clk);
> + clk_disable_unprepare(cdns_phy->clk1);
No sure if it matters, but for symmetry sake should we disable clk1 before clk?
> clk_cleanup:
> cdns_torrent_clk_cleanup(cdns_phy);
> return ret;
> @@ -2999,6 +3067,7 @@ static void cdns_torrent_phy_remove(struct platform_device *pdev)
> }
>
> clk_disable_unprepare(cdns_phy->clk);
> + clk_disable_unprepare(cdns_phy->clk1);
Here too.
> cdns_torrent_clk_cleanup(cdns_phy);
> }
>
> @@ -3034,6 +3103,100 @@ static struct cdns_torrent_vals dp_usb_xcvr_diag_ln_vals = {
> .num_regs = ARRAY_SIZE(dp_usb_xcvr_diag_ln_regs),
> };
<snip>
--
cheers,
-roger
Powered by blists - more mailing lists