| Message ID | 20240130003546.1546-1-ansuelsmth@gmail.com |
|---|---|
| Headers | show |
| Series | net: mdio-ipq4019: fix wrong default MDC rate | expand |
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
On Tue, Jan 30, 2024 at 01:35:22AM +0100, Christian Marangi wrote: > Add clock-frequency to MDIO node to set the MDC rate to 6.25Mhz instead > of using the default value of 390KHz from MDIO default divider. > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > --- > arch/arm64/boot/dts/qcom/ipq8074.dtsi | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi > index 2f275c84e566..08ddfeece043 100644 > --- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi > +++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi > @@ -264,6 +264,8 @@ mdio: mdio@90000 { > clocks = <&gcc GCC_MDIO_AHB_CLK>; > clock-names = "gcc_mdio_ahb_clk"; > > + clock-frequency = <6250000>; > + > status = "disabled"; If we merge this via netdev, is a merge conflict likely? Any other changes expected in this area, given the changes which might happen to GCC soon, etc? Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
On Tue, Jan 30, 2024 at 01:55:28AM +0100, Andrew Lunn wrote: > On Tue, Jan 30, 2024 at 01:35:22AM +0100, Christian Marangi wrote: > > Add clock-frequency to MDIO node to set the MDC rate to 6.25Mhz instead > > of using the default value of 390KHz from MDIO default divider. > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > --- > > arch/arm64/boot/dts/qcom/ipq8074.dtsi | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/ipq8074.dtsi b/arch/arm64/boot/dts/qcom/ipq8074.dtsi > > index 2f275c84e566..08ddfeece043 100644 > > --- a/arch/arm64/boot/dts/qcom/ipq8074.dtsi > > +++ b/arch/arm64/boot/dts/qcom/ipq8074.dtsi > > @@ -264,6 +264,8 @@ mdio: mdio@90000 { > > clocks = <&gcc GCC_MDIO_AHB_CLK>; > > clock-names = "gcc_mdio_ahb_clk"; > > > > + clock-frequency = <6250000>; > > + > > status = "disabled"; > > If we merge this via netdev, is a merge conflict likely? Any other > changes expected in this area, given the changes which might happen to > GCC soon, etc? > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Honestly I don't expect much to change here in the mdio node. If it's a problem I can submit in a separate patch in linux-msm.
On Tue, 30 Jan 2024 01:57:36 +0100 Christian Marangi wrote: > > If we merge this via netdev, is a merge conflict likely? Any other > > changes expected in this area, given the changes which might happen to > > GCC soon, etc? > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Honestly I don't expect much to change here in the mdio node. > > If it's a problem I can submit in a separate patch in linux-msm. The arch maintainers usually prefer to take the DTS patches, so if there isn't anything special here we should probably default to that.
On Tue, Jan 30, 2024 at 05:40:41PM -0800, Jakub Kicinski wrote: > On Tue, 30 Jan 2024 01:57:36 +0100 Christian Marangi wrote: > > > If we merge this via netdev, is a merge conflict likely? Any other > > > changes expected in this area, given the changes which might happen to > > > GCC soon, etc? > > > > > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > > > Honestly I don't expect much to change here in the mdio node. > > > > If it's a problem I can submit in a separate patch in linux-msm. > > The arch maintainers usually prefer to take the DTS patches, > so if there isn't anything special here we should probably > default to that. Thanks for the suggestion I sent v3 with the DTS patch dropped and I sent that patch separately.
On 1/30/2024 8:35 AM, Christian Marangi wrote: > + > + /* If div is /256 assume nobody have set this value and > + * try to find one MDC rate that is close the 802.3 spec of > + * 2.5MHz > + */ > + for (div = 256; div >= 8; div /= 2) { > + /* Stop as soon as we found a divider that > + * reached the closest value to 2.5MHz > + */ > + if (DIV_ROUND_UP(ahb_rate, div) > 2500000) > + break; Hi Christian, Sorry for the delayed review. The MDIO hardware block supports higher frequency 6.25M and 12.5M, Would you remove this 2.5MHZ limitation? On the IPQ platform, we normally use 6.25MHZ. > + > + priv->mdc_rate = DIV_ROUND_UP(ahb_rate, div); > + } > }
On Sun, Feb 04, 2024 at 05:59:10PM +0800, Jie Luo wrote: > > > On 1/30/2024 8:35 AM, Christian Marangi wrote: > > + > > + /* If div is /256 assume nobody have set this value and > > + * try to find one MDC rate that is close the 802.3 spec of > > + * 2.5MHz > > + */ > > + for (div = 256; div >= 8; div /= 2) { > > + /* Stop as soon as we found a divider that > > + * reached the closest value to 2.5MHz > > + */ > > + if (DIV_ROUND_UP(ahb_rate, div) > 2500000) > > + break; > > Hi Christian, > Sorry for the delayed review. > > The MDIO hardware block supports higher frequency 6.25M and 12.5M, > Would you remove this 2.5MHZ limitation? On the IPQ platform, we > normally use 6.25MHZ. 802.3 says the clock has a maximum of 2.5MHz. So this code is correct. It is however O.K. to go faster, but since that breaks the standard, you need each board to indicate it knows all the devices on the bus do support higher speeds and its O.K. to break the standard. You indicate this by using the DT property in its .dts file. For an MDIO bus which is totally internal, you could however put the DT property in the SoC .dtsi file. Andrew
On 2/4/2024 11:22 PM, Andrew Lunn wrote: > On Sun, Feb 04, 2024 at 05:59:10PM +0800, Jie Luo wrote: >> >> >> On 1/30/2024 8:35 AM, Christian Marangi wrote: >>> + >>> + /* If div is /256 assume nobody have set this value and >>> + * try to find one MDC rate that is close the 802.3 spec of >>> + * 2.5MHz >>> + */ >>> + for (div = 256; div >= 8; div /= 2) { >>> + /* Stop as soon as we found a divider that >>> + * reached the closest value to 2.5MHz >>> + */ >>> + if (DIV_ROUND_UP(ahb_rate, div) > 2500000) >>> + break; >> >> Hi Christian, >> Sorry for the delayed review. >> >> The MDIO hardware block supports higher frequency 6.25M and 12.5M, >> Would you remove this 2.5MHZ limitation? On the IPQ platform, we >> normally use 6.25MHZ. > > 802.3 says the clock has a maximum of 2.5MHz. So this code is correct. > > It is however O.K. to go faster, but since that breaks the standard, > you need each board to indicate it knows all the devices on the bus do > support higher speeds and its O.K. to break the standard. You indicate > this by using the DT property in its .dts file. For an MDIO bus which > is totally internal, you could however put the DT property in the SoC > .dtsi file. > > Andrew Understand it, Thanks Andrew.