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

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

[Andrew Lunn]

> 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.

> +	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?

> +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?

> +	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.

> +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?

   Andrew

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

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

[Steen Hegelund]

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

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

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

[Andrew Lunn]

> > 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.

In this context of an Ethernet SERDES, do you actually need it? You
would normally look at the PCS link status to determine if the link is
up.  But it is useful debug information. If the PCS is down, but the
PHY indicates up, you can guess you have a protocol misconfiguration.

What exactly does link at this level mean? And thinking of the wider
uses of the PHY subsystem, what would link mean at this level for
SATA, PCIe, USB? Don't these all have some protocol level above
similar to Ethernet PCS which is the real determiner of link?

     Andrew

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

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

[Steen Hegelund]

On 10.12.2020 15:16, Andrew Lunn wrote:
>EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> > 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.
>
>In this context of an Ethernet SERDES, do you actually need it? You
>would normally look at the PCS link status to determine if the link is
>up.  But it is useful debug information. If the PCS is down, but the
>PHY indicates up, you can guess you have a protocol misconfiguration.

Yes - you are probably right about that.  I have been exposing this via
a procfs interface during the development phase, and it was really
useful to have, to track down the origin of the problem in certain situations.

But on a system level, the PCS link would have the final say anyway.
>
>What exactly does link at this level mean? And thinking of the wider
>uses of the PHY subsystem, what would link mean at this level for
>SATA, PCIe, USB? Don't these all have some protocol level above
>similar to Ethernet PCS which is the real determiner of link?

Yes - I think this is really only a debug feature.  No need to force
this on the other PHY categories.


>
>     Andrew

Thanks for your comments, Andrew.

BR
Steen

---------------------------------------
Steen Hegelund
steen.hegelund@microchip.com

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