Re: [PATCH v13 3/4] phy: Add Sparx5 ethernet serdes PHY driver

Hi Vinod,

On Thu, 2021-02-04 at 13:31 +0530, Vinod Koul wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

On 29-01-21, 14:07, Steen Hegelund wrote:

quoted

Add the Microchip Sparx5 ethernet serdes PHY driver for the 6G, 10G
and 25G
interfaces available in the Sparx5 SoC.

Signed-off-by: Bjarni Jonasson <redacted>
Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---

...

quoted

sdx5_rmw(SD25G_LANE_LANE_1E_LN_CFG_RXLB_EN_SET(params-

quoted

cfg_rxlb_en),

+              SD25G_LANE_LANE_1E_LN_CFG_RXLB_EN,
+              priv,
+              SD25G_LANE_LANE_1E(sd_index));
+
+     sdx5_rmw(SD25G_LANE_LANE_19_LN_CFG_TXLB_EN_SET(params-

quoted

cfg_txlb_en),

+              SD25G_LANE_LANE_19_LN_CFG_TXLB_EN,
+              priv,
+              SD25G_LANE_LANE_19(sd_index));
+
+     sdx5_rmw(SD25G_LANE_LANE_2E_LN_CFG_RSTN_DFEDIG_SET(0),
+              SD25G_LANE_LANE_2E_LN_CFG_RSTN_DFEDIG,
+              priv,
+              SD25G_LANE_LANE_2E(sd_index));
+
+     sdx5_rmw(SD25G_LANE_LANE_2E_LN_CFG_RSTN_DFEDIG_SET(1),
+              SD25G_LANE_LANE_2E_LN_CFG_RSTN_DFEDIG,
+              priv,
+              SD25G_LANE_LANE_2E(sd_index));
+
+     sdx5_rmw(SD_LANE_25G_SD_LANE_CFG_MACRO_RST_SET(0),
+              SD_LANE_25G_SD_LANE_CFG_MACRO_RST,
+              priv,
+              SD_LANE_25G_SD_LANE_CFG(sd_index));
+
+     sdx5_rmw(SD25G_LANE_LANE_1C_LN_CFG_CDR_RSTN_SET(0),
+              SD25G_LANE_LANE_1C_LN_CFG_CDR_RSTN,
+              priv,
+              SD25G_LANE_LANE_1C(sd_index));

This looks quite terrible :(

Can we do a table here for these and then write the configuration
table,
that may look better and easy to maintain ?

I will restructure this.

quoted

+
+     usleep_range(1000, 2000);
+
+     sdx5_rmw(SD25G_LANE_LANE_1C_LN_CFG_CDR_RSTN_SET(1),
+              SD25G_LANE_LANE_1C_LN_CFG_CDR_RSTN,
+              priv,
+              SD25G_LANE_LANE_1C(sd_index));
+
+     usleep_range(10000, 20000);
+
+     sdx5_rmw(SD25G_LANE_CMU_FF_REGISTER_TABLE_INDEX_SET(0xff),
+              SD25G_LANE_CMU_FF_REGISTER_TABLE_INDEX,
+              priv,
+              SD25G_LANE_CMU_FF(sd_index));
+
+     value = sdx5_rd(priv, SD25G_LANE_CMU_C0(sd_index));
+     value = SD25G_LANE_CMU_C0_PLL_LOL_UDL_GET(value);
+
+     if (value) {
+             dev_err(macro->priv->dev, "25G PLL Loss of Lock:
0x%x\n", value);
+             ret = -EINVAL;
+     }
+
+     value = sdx5_rd(priv, SD_LANE_25G_SD_LANE_STAT(sd_index));
+     value = SD_LANE_25G_SD_LANE_STAT_PMA_RST_DONE_GET(value);
+
+     if (value != 0x1) {
+             dev_err(macro->priv->dev, "25G PMA Reset failed:
0x%x\n", value);
+             ret = -EINVAL;

continue on error..?

I will change that.

quoted

+     }
+
+     sdx5_rmw(SD25G_LANE_CMU_2A_R_DBG_LOL_STATUS_SET(0x1),
+              SD25G_LANE_CMU_2A_R_DBG_LOL_STATUS,
+              priv,
+              SD25G_LANE_CMU_2A(sd_index));
+
+     sdx5_rmw(SD_LANE_25G_SD_SER_RST_SER_RST_SET(0x0),
+              SD_LANE_25G_SD_SER_RST_SER_RST,
+              priv,

...

quoted

sdx5_inst_rmw(SD10G_LANE_LANE_0E_CFG_RXLB_EN_SET(params-

quoted

cfg_rxlb_en) |

+                   SD10G_LANE_LANE_0E_CFG_TXLB_EN_SET(params-

quoted

cfg_txlb_en),

+                   SD10G_LANE_LANE_0E_CFG_RXLB_EN |
+                   SD10G_LANE_LANE_0E_CFG_TXLB_EN,
+                   sd_inst,
+                   SD10G_LANE_LANE_0E(sd_index));
+
+     sdx5_rmw(SD_LANE_SD_LANE_CFG_MACRO_RST_SET(0),
+              SD_LANE_SD_LANE_CFG_MACRO_RST,
+              priv,
+              SD_LANE_SD_LANE_CFG(sd_lane_tgt));
+
+     sdx5_inst_rmw(SD10G_LANE_LANE_50_CFG_SSC_RESETB_SET(1),
+                   SD10G_LANE_LANE_50_CFG_SSC_RESETB,
+                   sd_inst,
+                   SD10G_LANE_LANE_50(sd_index));
+
+     sdx5_rmw(SD10G_LANE_LANE_50_CFG_SSC_RESETB_SET(1),
+              SD10G_LANE_LANE_50_CFG_SSC_RESETB,
+              priv,
+              SD10G_LANE_LANE_50(sd_index));
+
+     sdx5_rmw(SD_LANE_MISC_SD_125_RST_DIS_SET(params->fx_100),
+              SD_LANE_MISC_SD_125_RST_DIS,
+              priv,
+              SD_LANE_MISC(sd_lane_tgt));
+
+     sdx5_rmw(SD_LANE_MISC_RX_ENA_SET(params->fx_100),
+              SD_LANE_MISC_RX_ENA,
+              priv,
+              SD_LANE_MISC(sd_lane_tgt));
+
+     sdx5_rmw(SD_LANE_MISC_MUX_ENA_SET(params->fx_100),
+              SD_LANE_MISC_MUX_ENA,
+              priv,
+              SD_LANE_MISC(sd_lane_tgt));

Table for this set as well as other places please

I will restructure the code here as well.

--
~Vinod

Thank you for your comments.

BR
Steen




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel