Messages in this thread | | | Date | Thu, 10 Dec 2020 13:57:06 +0100 | From | Steen Hegelund <> | Subject | Re: [PATCH v9 3/4] phy: Add Sparx5 ethernet serdes PHY driver |
| |
On 10.12.2020 03:11, Andrew Lunn wrote: >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >> index 01b53f86004c..f6a094c81e86 100644 >> --- a/drivers/phy/Kconfig >> +++ b/drivers/phy/Kconfig >> @@ -66,9 +66,11 @@ source "drivers/phy/broadcom/Kconfig" >> source "drivers/phy/cadence/Kconfig" >> source "drivers/phy/freescale/Kconfig" >> source "drivers/phy/hisilicon/Kconfig" >> +source "drivers/phy/intel/Kconfig" > >That looks odd. > >> source "drivers/phy/lantiq/Kconfig" >> source "drivers/phy/marvell/Kconfig" >> source "drivers/phy/mediatek/Kconfig" >> +source "drivers/phy/microchip/Kconfig" >> source "drivers/phy/motorola/Kconfig" >> source "drivers/phy/mscc/Kconfig" >> source "drivers/phy/qualcomm/Kconfig" >> @@ -80,7 +82,6 @@ source "drivers/phy/socionext/Kconfig" >> source "drivers/phy/st/Kconfig" >> source "drivers/phy/tegra/Kconfig" >> source "drivers/phy/ti/Kconfig" >> -source "drivers/phy/intel/Kconfig" >> source "drivers/phy/xilinx/Kconfig" > >Ah. Please make that a separate patch.
Yes - it was really a separate change as a result of my sorting... > >> + 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, "CMU_C0 pll_lol_udl: 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, "sd_lane_stat pma_rst_done: 0x%x\n", value); >> + ret = -EINVAL; >> + } > >These error messages are not very helpful. Could you be a bit more >descriptive. Or do you think there is sufficient black magic in the >hardware that nobody outside of Microchip will be able to debug it?
I will dig up some better descriptions...
> >> +static int sparx5_serdes_get_serdesmode(phy_interface_t portmode, >> + struct phy_configure_opts_eth_serdes *conf) >> +{ >> + switch (portmode) { >> + case PHY_INTERFACE_MODE_1000BASEX: >> + if (conf->speed == SPEED_2500) >> + return SPX5_SD_MODE_2G5; >> + if (conf->speed == SPEED_100) >> + return SPX5_SD_MODE_100FX; >> + return SPX5_SD_MODE_1000BASEX; > >Please could you explain this. Why different speeds for 1000BaseX?
I will remove this. It was taken from our bare-metal API (MESA) and only relevant in that context because it did not have an explicit 2500G mode.
> >> + case PHY_INTERFACE_MODE_SGMII: >> + return SPX5_SD_MODE_1000BASEX; > >Here there could be some oddities, depending on how 10Mbps and 100Mbps >is implemented. But 1000BASEX only supports 1Gbps. > The same Serdes mode is used for SGMII and 1000BaseX. Speeds 10M/100M is handled by repeating the byte sequence 100/10 times to get to 1G serdes speed.
>> +static int sparx5_serdes_validate(struct phy *phy, enum phy_mode mode, >> + int submode, >> + union phy_configure_opts *opts) >> +{ >> + struct sparx5_serdes_macro *macro = phy_get_drvdata(phy); >> + struct sparx5_serdes_private *priv = macro->priv; >> + u32 value, analog_sd; >> + >> + if (mode != PHY_MODE_ETHERNET) >> + return -EINVAL; >> + >> + switch (submode) { >> + case PHY_INTERFACE_MODE_1000BASEX: >> + case PHY_INTERFACE_MODE_SGMII: >> + case PHY_INTERFACE_MODE_QSGMII: >> + case PHY_INTERFACE_MODE_10GBASER: >> + break; >> + default: >> + return -EINVAL; >> + } >> + if (macro->serdestype == SPX5_SDT_6G) { >> + value = sdx5_rd(priv, SD6G_LANE_LANE_DF(macro->stpidx)); >> + analog_sd = SD6G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value); >> + } else if (macro->serdestype == SPX5_SDT_10G) { >> + value = sdx5_rd(priv, SD10G_LANE_LANE_DF(macro->stpidx)); >> + analog_sd = SD10G_LANE_LANE_DF_PMA2PCS_RXEI_FILTERED_GET(value); >> + } else { >> + value = sdx5_rd(priv, SD25G_LANE_LANE_DE(macro->stpidx)); >> + analog_sd = SD25G_LANE_LANE_DE_LN_PMA_RXEI_GET(value); >> + } >> + /* Link up is when analog_sd == 0 */ >> + return analog_sd; > >The documentation says: > > /** > * @validate: > * > * Optional. > * > * Used to check that the current set of parameters can be > * handled by the phy. Implementations are free to tune the > * parameters passed as arguments if needed by some > * implementation detail or constraints. It must not change > * any actual configuration of the PHY, so calling it as many > * times as deemed fit by the consumer must have no side > * effect. > * > * Returns: 0 if the configuration can be applied, an negative > * error code otherwise > */ > >So why are returning link up information?
Yes that was a bit of a hijacking of the function. I will remove that. I also removed the dependency on this behaviour in the client driver in the meantime.
I think a status function on the generic phy would be useful, but I will take that as separate issue.
> > Andrew
Thanks for the comments.
BR Steen
--------------------------------------- Steen Hegelund steen.hegelund@microchip.com
| |