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:
> > 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 <bjarni.jonas...@microchip.com>
> > Signed-off-by: Steen Hegelund <steen.hegel...@microchip.com>
> > Reviewed-by: Andrew Lunn <and...@lunn.ch>
> > Reviewed-by: Alexandre Belloni <alexandre.bell...@bootlin.com>
> > ---
> >
...
> > sdx5_rmw(SD25G_LANE_LANE_1E_LN_CFG_RXLB_EN_SET(params-
> > >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-
> > >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.
>
> > +
> > + 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.
>
> > + }
> > +
> > + 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,
> >
...
> > sdx5_inst_rmw(SD10G_LANE_LANE_0E_CFG_RXLB_EN_SET(params-
> > >cfg_rxlb_en) |
> > + SD10G_LANE_LANE_0E_CFG_TXLB_EN_SET(params-
> > >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
