| Message ID | a613b66b4872b5f3f09544138d03d5326a8f6f8b.1675407169.git.daniel@makrotopia.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net: ethernet: mtk_eth_soc: various enhancements | expand |
> static int mtk_mdio_init(struct mtk_eth *eth) > { > struct device_node *mii_np; > + int clk = 25000000, max_clk = 2500000, divider = 1; > int ret; > + u32 val; Reverse Christmas tree please. > + > + if (!of_property_read_u32(mii_np, "clock-frequency", &val)) > + max_clk = val; > + > + while (clk / divider > max_clk) { > + if (divider >= 63) > + break; > + > + divider++; > + }; Please add some range checks here. Return -EINVAL if val > max_clock. Also, if divider = 63 indicating the requested clock is too slow. > + > + val = mtk_r32(eth, MTK_PPSC); > + val |= PPSC_MDC_TURBO; > + mtk_w32(eth, val, MTK_PPSC); > + > + /* Configure MDC Divider */ > + val = mtk_r32(eth, MTK_PPSC); > + val &= ~PPSC_MDC_CFG; > + val |= FIELD_PREP(PPSC_MDC_CFG, divider); > + mtk_w32(eth, val, MTK_PPSC); Can these two writes to MTK_PPSC be combined into one? val |= FIELD_PREP(PPSC_MDC_CFG, divider) | PPSC_MDC_TURBO; Andrew
On Fri, Feb 03, 2023 at 07:01:01AM +0000, Daniel Golle wrote: > Set MDIO bus clock frequency and allow setting a custom maximum > frequency from device tree. > > Signed-off-by: Daniel Golle <daniel@makrotopia.org> > --- > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 25 +++++++++++++++++++++ > drivers/net/ethernet/mediatek/mtk_eth_soc.h | 5 +++++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index a44ffff48c7b..9050423821dc 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > @@ -790,7 +790,9 @@ static const struct phylink_mac_ops mtk_phylink_ops = { > static int mtk_mdio_init(struct mtk_eth *eth) > { > struct device_node *mii_np; > + int clk = 25000000, max_clk = 2500000, divider = 1; Would be good if constant values (clk) weren't put in variables. > int ret; > + u32 val; > > mii_np = of_get_child_by_name(eth->dev->of_node, "mdio-bus"); > if (!mii_np) { > @@ -818,6 +820,29 @@ static int mtk_mdio_init(struct mtk_eth *eth) > eth->mii_bus->parent = eth->dev; > > snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%pOFn", mii_np); > + > + if (!of_property_read_u32(mii_np, "clock-frequency", &val)) > + max_clk = val; Checking for valid range? There should also probably be a dt-bindings patch for this. > + > + while (clk / divider > max_clk) { > + if (divider >= 63) > + break; > + > + divider++; > + }; uhm, "divider = min(DIV_ROUND_UP(25000000, max_clk), 63);"? I don't think the compiler is smart enough to optimize away this loop. > + > + val = mtk_r32(eth, MTK_PPSC); > + val |= PPSC_MDC_TURBO; > + mtk_w32(eth, val, MTK_PPSC); What does "TURBO" do and why do you set it unconditionally? > + > + /* Configure MDC Divider */ > + val = mtk_r32(eth, MTK_PPSC); > + val &= ~PPSC_MDC_CFG; > + val |= FIELD_PREP(PPSC_MDC_CFG, divider); > + mtk_w32(eth, val, MTK_PPSC); > + > + dev_dbg(eth->dev, "MDC is running on %d Hz\n", clk / divider); > + > ret = of_mdiobus_register(eth->mii_bus, mii_np); > > err_put_node: > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > index 7230dcb29315..724815ae18a0 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h > @@ -363,6 +363,11 @@ > #define RX_DMA_VTAG_V2 BIT(0) > #define RX_DMA_L4_VALID_V2 BIT(2) > > +/* PHY Polling and SMI Master Control registers */ > +#define MTK_PPSC 0x10000 > +#define PPSC_MDC_CFG GENMASK(29, 24) > +#define PPSC_MDC_TURBO BIT(20) > + > /* PHY Indirect Access Control registers */ > #define MTK_PHY_IAC 0x10004 > #define PHY_IAC_ACCESS BIT(31) > -- > 2.39.1 >
> Checking for valid range? There should also probably be a dt-bindings > patch for this. This is a common mdio property in mdio.yaml. So there should not be any need for a driver specific dt-binding. Andrew
On Fri, Feb 03, 2023 at 11:15:15PM +0100, Andrew Lunn wrote: > > Checking for valid range? There should also probably be a dt-bindings > > patch for this. > > This is a common mdio property in mdio.yaml. So there should not be > any need for a driver specific dt-binding. I meant it would probably be informative if the dt schema for this MDIO controller had a "default: $val", "minimum: $val" and "maximum: $val".
diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index a44ffff48c7b..9050423821dc 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -790,7 +790,9 @@ static const struct phylink_mac_ops mtk_phylink_ops = { static int mtk_mdio_init(struct mtk_eth *eth) { struct device_node *mii_np; + int clk = 25000000, max_clk = 2500000, divider = 1; int ret; + u32 val; mii_np = of_get_child_by_name(eth->dev->of_node, "mdio-bus"); if (!mii_np) { @@ -818,6 +820,29 @@ static int mtk_mdio_init(struct mtk_eth *eth) eth->mii_bus->parent = eth->dev; snprintf(eth->mii_bus->id, MII_BUS_ID_SIZE, "%pOFn", mii_np); + + if (!of_property_read_u32(mii_np, "clock-frequency", &val)) + max_clk = val; + + while (clk / divider > max_clk) { + if (divider >= 63) + break; + + divider++; + }; + + val = mtk_r32(eth, MTK_PPSC); + val |= PPSC_MDC_TURBO; + mtk_w32(eth, val, MTK_PPSC); + + /* Configure MDC Divider */ + val = mtk_r32(eth, MTK_PPSC); + val &= ~PPSC_MDC_CFG; + val |= FIELD_PREP(PPSC_MDC_CFG, divider); + mtk_w32(eth, val, MTK_PPSC); + + dev_dbg(eth->dev, "MDC is running on %d Hz\n", clk / divider); + ret = of_mdiobus_register(eth->mii_bus, mii_np); err_put_node: diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h index 7230dcb29315..724815ae18a0 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h @@ -363,6 +363,11 @@ #define RX_DMA_VTAG_V2 BIT(0) #define RX_DMA_L4_VALID_V2 BIT(2) +/* PHY Polling and SMI Master Control registers */ +#define MTK_PPSC 0x10000 +#define PPSC_MDC_CFG GENMASK(29, 24) +#define PPSC_MDC_TURBO BIT(20) + /* PHY Indirect Access Control registers */ #define MTK_PHY_IAC 0x10004 #define PHY_IAC_ACCESS BIT(31)
Set MDIO bus clock frequency and allow setting a custom maximum frequency from device tree. Signed-off-by: Daniel Golle <daniel@makrotopia.org> --- drivers/net/ethernet/mediatek/mtk_eth_soc.c | 25 +++++++++++++++++++++ drivers/net/ethernet/mediatek/mtk_eth_soc.h | 5 +++++ 2 files changed, 30 insertions(+)