Re: [PATCH 2/2] phy: armada-38x: fix NETA lockup when repeatedly switching speeds

[Vinod Koul <vkoul@kernel.org>]

On 30-06-20, 17:05, Russell King wrote:
> The mvneta hardware appears to lock up in various random ways when
> repeatedly switching speeds between 1G and 2.5G, which involves
> reprogramming the COMPHY.  It is not entirely clear why this happens,
> but best guess is that reprogramming the COMPHY glitches mvneta clocks
> causing the hardware to fail.  It seems that rebooting resolves the
> failure, but not down/up cycling the interface alone.
> 
> Various other approaches have been tried, such as trying to cleanly
> power down the COMPHY and then take it back through the power up
> initialisation, but this does not seem to help.
> 
> It was finally noticed that u-boot's last step when configuring a
> COMPHY for "SGMII" mode was to poke at a register described as
> "GBE_CONFIGURATION_REG", which is undocumented in any external
> documentation.  All that we have is the fact that u-boot sets a bit
> corresponding to the "SGMII" lane at the end of COMPHY initialisation.
> 
> Experimentation shows that if we clear this bit prior to changing the
> speed, and then set it afterwards, mvneta does not suffer this problem
> on the SolidRun Clearfog when switching speeds between 1G and 2.5G.
> 
> This problem was found while script-testing phylink.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  arch/arm/boot/dts/armada-38x.dtsi          |  3 +-

lgtm, i need ack for dts parts before I can apply this

>  drivers/phy/marvell/phy-armada38x-comphy.c | 45 ++++++++++++++++++----
>  2 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi
> index e038abc0c6b4..420ae26e846b 100644
> --- a/arch/arm/boot/dts/armada-38x.dtsi
> +++ b/arch/arm/boot/dts/armada-38x.dtsi
> @@ -344,7 +344,8 @@
>  
>  			comphy: phy@18300 {
>  				compatible = "marvell,armada-380-comphy";
> -				reg = <0x18300 0x100>;
> +				reg-names = "comphy", "conf";
> +				reg = <0x18300 0x100>, <0x18460 4>;
>  				#address-cells = <1>;
>  				#size-cells = <0>;
>  
> diff --git a/drivers/phy/marvell/phy-armada38x-comphy.c b/drivers/phy/marvell/phy-armada38x-comphy.c
> index 6960dfd8ad8c..0fe408964334 100644
> --- a/drivers/phy/marvell/phy-armada38x-comphy.c
> +++ b/drivers/phy/marvell/phy-armada38x-comphy.c
> @@ -41,6 +41,7 @@ struct a38x_comphy_lane {
>  
>  struct a38x_comphy {
>  	void __iomem *base;
> +	void __iomem *conf;
>  	struct device *dev;
>  	struct a38x_comphy_lane lane[MAX_A38X_COMPHY];
>  };
> @@ -54,6 +55,21 @@ static const u8 gbe_mux[MAX_A38X_COMPHY][MAX_A38X_PORTS] = {
>  	{ 0, 0, 3 },
>  };
>  
> +static void a38x_set_conf(struct a38x_comphy_lane *lane, bool enable)
> +{
> +	struct a38x_comphy *priv = lane->priv;
> +	u32 conf;
> +
> +	if (priv->conf) {
> +		conf = readl_relaxed(priv->conf);
> +		if (enable)
> +			conf |= BIT(lane->port);
> +		else
> +			conf &= ~BIT(lane->port);
> +		writel(conf, priv->conf);
> +	}
> +}
> +
>  static void a38x_comphy_set_reg(struct a38x_comphy_lane *lane,
>  				unsigned int offset, u32 mask, u32 value)
>  {
> @@ -97,6 +113,7 @@ static int a38x_comphy_set_mode(struct phy *phy, enum phy_mode mode, int sub)
>  {
>  	struct a38x_comphy_lane *lane = phy_get_drvdata(phy);
>  	unsigned int gen;
> +	int ret;
>  
>  	if (mode != PHY_MODE_ETHERNET)
>  		return -EINVAL;
> @@ -115,13 +132,20 @@ static int a38x_comphy_set_mode(struct phy *phy, enum phy_mode mode, int sub)
>  		return -EINVAL;
>  	}
>  
> +	a38x_set_conf(lane, false);
> +
>  	a38x_comphy_set_speed(lane, gen, gen);
>  
> -	return a38x_comphy_poll(lane, COMPHY_STAT1,
> -				COMPHY_STAT1_PLL_RDY_TX |
> -				COMPHY_STAT1_PLL_RDY_RX,
> -				COMPHY_STAT1_PLL_RDY_TX |
> -				COMPHY_STAT1_PLL_RDY_RX);
> +	ret = a38x_comphy_poll(lane, COMPHY_STAT1,
> +			       COMPHY_STAT1_PLL_RDY_TX |
> +			       COMPHY_STAT1_PLL_RDY_RX,
> +			       COMPHY_STAT1_PLL_RDY_TX |
> +			       COMPHY_STAT1_PLL_RDY_RX);
> +
> +	if (ret == 0)
> +		a38x_set_conf(lane, true);
> +
> +	return ret;
>  }
>  
>  static const struct phy_ops a38x_comphy_ops = {
> @@ -174,14 +198,21 @@ static int a38x_comphy_probe(struct platform_device *pdev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	base = devm_ioremap_resource(&pdev->dev, res);
> +	base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
>  	priv->dev = &pdev->dev;
>  	priv->base = base;
>  
> +	/* Optional */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf");
> +	if (res) {
> +		priv->conf = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(priv->conf))
> +			return PTR_ERR(priv->conf);
> +	}
> +
>  	for_each_available_child_of_node(pdev->dev.of_node, child) {
>  		struct phy *phy;
>  		int ret;
> -- 
> 2.20.1

-- 
~Vinod

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