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

[Steen Hegelund <steen.hegelund@microchip.com>]

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.jonasson@microchip.com>
> > Signed-off-by: Steen Hegelund <steen.hegelund@microchip.com>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > Reviewed-by: Alexandre Belloni <alexandre.belloni@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