Received-SPF: pass (google.com: domain of linux-kernel+bounces-43698-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 30 Jan 2024 01:53:32 +0100
From: Andrew Lunn <andrew@lunn.ch>
To: Christian Marangi <ansuelsmth@gmail.com>
Cc: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Russell King <linux@armlinux.org.uk>,
	Robert Marko <robert.marko@sartura.hr>,
	linux-arm-msm@vger.kernel.org, netdev@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	Jie Luo <quic_luoj@quicinc.com>
Subject: Re: [net-next PATCH v2 2/3] net: mdio: ipq4019: add support for
 clock-frequency property
Message-ID: <27b6701b-f3f1-4277-8720-b436dc8cc75a@lunn.ch>
References: <20240130003546.1546-1-ansuelsmth@gmail.com>
 <20240130003546.1546-3-ansuelsmth@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130003546.1546-3-ansuelsmth@gmail.com>

On Tue, Jan 30, 2024 at 01:35:21AM +0100, Christian Marangi wrote:
> The IPQ4019 MDIO internally divide the clock feed by AHB based on the
> MDIO_MODE reg. On reset or power up, the default value for the
> divider is 0xff that reflect the divider set to /256.
> 
> This makes the MDC run at a very low rate, that is, considering AHB is
> always fixed to 100Mhz, a value of 390KHz.
> 
> This hasn't have been a problem as MDIO wasn't used for time sensitive
> operation, it is now that on IPQ807x is usually mounted with PHY that
> requires MDIO to load their firmware (example Aquantia PHY).
> 
> To handle this problem and permit to set the correct designed MDC
> frequency for the SoC add support for the standard "clock-frequency"
> property for the MDIO node.
> 
> The divider supports value from /1 to /256 and the common value are to
> set it to /16 to reflect 6.25Mhz or to /8 on newer platform to reflect
> 12.5Mhz.
> 
> To scan if the requested rate is supported by the divider, loop with
> each supported divider and stop when the requested rate match the final
> rate with the current divider. An error is returned if the rate doesn't
> match any value.
> 
> On MDIO reset, the divider is restored to the requested value to prevent
> any kind of downclocking caused by the divider reverting to a default
> value.
> 
> To follow 802.3 spec of 2.5MHz of default value, if divider is set at
> /256 and "clock-frequency" is not set in DT, assume nobody set the
> divider and try to find the closest MDC rate to 2.5MHz. (in the case of
> AHB set to 100MHz, it's 1.5625MHz)
> 
> While at is also document other bits of the MDIO_MODE reg to have a
> clear idea of what is actually applied there.
> 
> Documentation of some BITs is skipped as they are marked as reserved and
> their usage is not clear (RES 11:9 GENPHY 16:13 RES1 19:17)
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/mdio/mdio-ipq4019.c | 109 ++++++++++++++++++++++++++++++--
>  1 file changed, 103 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/mdio/mdio-ipq4019.c b/drivers/net/mdio/mdio-ipq4019.c
> index abd8b508ec16..61638f25c184 100644
> --- a/drivers/net/mdio/mdio-ipq4019.c
> +++ b/drivers/net/mdio/mdio-ipq4019.c
> @@ -14,6 +14,20 @@
>  #include <linux/clk.h>
>  
>  #define MDIO_MODE_REG				0x40
> +#define   MDIO_MODE_MDC_MODE			BIT(12)
> +/* 0 = Clause 22, 1 = Clause 45 */
> +#define   MDIO_MODE_C45				BIT(8)
> +#define   MDIO_MODE_DIV_MASK			GENMASK(7, 0)
> +#define     MDIO_MODE_DIV(x)			FIELD_PREP(MDIO_MODE_DIV_MASK, (x) - 1)
> +#define     MDIO_MODE_DIV_1			0x0
> +#define     MDIO_MODE_DIV_2			0x1
> +#define     MDIO_MODE_DIV_4			0x3
> +#define     MDIO_MODE_DIV_8			0x7
> +#define     MDIO_MODE_DIV_16			0xf
> +#define     MDIO_MODE_DIV_32			0x1f
> +#define     MDIO_MODE_DIV_64			0x3f
> +#define     MDIO_MODE_DIV_128			0x7f
> +#define     MDIO_MODE_DIV_256			0xff
>  #define MDIO_ADDR_REG				0x44
>  #define MDIO_DATA_WRITE_REG			0x48
>  #define MDIO_DATA_READ_REG			0x4c
> @@ -26,9 +40,6 @@
>  #define MDIO_CMD_ACCESS_CODE_C45_WRITE	1
>  #define MDIO_CMD_ACCESS_CODE_C45_READ	2
>  
> -/* 0 = Clause 22, 1 = Clause 45 */
> -#define MDIO_MODE_C45				BIT(8)
> -
>  #define IPQ4019_MDIO_TIMEOUT	10000
>  #define IPQ4019_MDIO_SLEEP		10
>  
> @@ -41,6 +52,7 @@ struct ipq4019_mdio_data {
>  	void __iomem	*membase;
>  	void __iomem *eth_ldo_rdy;
>  	struct clk *mdio_clk;
> +	unsigned int mdc_rate;
>  };
>  
>  static int ipq4019_mdio_wait_busy(struct mii_bus *bus)
> @@ -203,6 +215,38 @@ static int ipq4019_mdio_write_c22(struct mii_bus *bus, int mii_id, int regnum,
>  	return 0;
>  }
>  
> +static int ipq4019_mdio_set_div(struct ipq4019_mdio_data *priv)
> +{
> +	unsigned long ahb_rate;
> +	int div;
> +	u32 val;
> +
> +	/* If we don't have a clock for AHB use the fixed value */
> +	ahb_rate = IPQ_MDIO_CLK_RATE;
> +	if (priv->mdio_clk)
> +		ahb_rate = clk_get_rate(priv->mdio_clk);
> +
> +	/* MDC rate is ahb_rate/(MDIO_MODE_DIV + 1)
> +	 * While supported, internal documentation doesn't
> +	 * assure correct functionality of the MDIO bus
> +	 * with divider of 1, 2 or 4.
> +	 */
> +	for (div = 8; div <= 256; div *= 2) {
> +		/* The requested rate is supported by the div */
> +		if (priv->mdc_rate == DIV_ROUND_UP(ahb_rate, div)) {
> +			val = readl(priv->membase + MDIO_MODE_REG);
> +			val &= ~MDIO_MODE_DIV_MASK;
> +			val |= MDIO_MODE_DIV(div);
> +			writel(val, priv->membase + MDIO_MODE_REG);
> +
> +			return 0;
> +		}
> +	}
> +
> +	/* The requested rate is not supported */
> +	return -EINVAL;
> +}
> +
>  static int ipq_mdio_reset(struct mii_bus *bus)
>  {
>  	struct ipq4019_mdio_data *priv = bus->priv;
> @@ -225,10 +269,58 @@ static int ipq_mdio_reset(struct mii_bus *bus)
>  		return ret;
>  
>  	ret = clk_prepare_enable(priv->mdio_clk);
> -	if (ret == 0)
> -		mdelay(10);
> +	if (ret)
> +		return ret;
>  
> -	return ret;
> +	mdelay(10);
> +
> +	/* Restore MDC rate */
> +	return ipq4019_mdio_set_div(priv);
> +}
> +
> +static void ipq4019_mdio_select_mdc_rate(struct platform_device *pdev,
> +					 struct ipq4019_mdio_data *priv)
> +{
> +	unsigned long ahb_rate;
> +	int div;
> +	u32 val;
> +
> +	/* MDC rate defined in DT, we don't have to decide a default value */
> +	if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +				  &priv->mdc_rate))
> +		return;
> +
> +	/* If we don't have a clock for AHB use the fixed value */
> +	ahb_rate = IPQ_MDIO_CLK_RATE;
> +	if (priv->mdio_clk)
> +		ahb_rate = clk_get_rate(priv->mdio_clk);
> +
> +	/* Check what is the current div set */
> +	val = readl(priv->membase + MDIO_MODE_REG);
> +	div = FIELD_GET(MDIO_MODE_DIV_MASK, val);
> +
> +	/* div is not set to the default value of /256
> +	 * Probably someone changed that (bootloader, other drivers)
> +	 * Keep this and doesn't overwrite it.

doesn't -> don't 

Otherwise please add my Reviewed-by on the next version.

	Andrew