| Message ID | 20220517142706.2570696-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Rejected |
| Delegated to: | Ramon Fried |
| Headers | show |
| Series | phy: dp83867: add dp83867_{read,write}_mmd helpers | expand |
Hi Rasmus, On Tue, May 17, 2022 at 04:27:06PM +0200, Rasmus Villemoes wrote: > Since the phy_{read,write}_mmd functions are static inlines using > other static inline functions, they cause code using them to explode. > > Defining local wrappers cuts the size of the generated code by 50%: > > $ size drivers/net/phy/dp83867.o.{before,after} > text data bss dec hex filename > 4873 112 0 4985 1379 drivers/net/phy/dp83867.o.before > 2413 112 0 2525 9dd drivers/net/phy/dp83867.o.after > > Of course, most of that improvement could also be had by making the > phy_*_mmd functions out-of-line, and they probably should be, but this > still has the advantage of avoiding passing the DP83867_DEVADDR > argument at all call sites, which allows lines to be unwrapped (and > probably also gives a little .text reduction by itself). > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- Have you considered making phy_read_mmd() and phy_write_mmd() non-inline? There are few users, but it looks like they would all benefit from this.
On 19/05/2022 16.38, Vladimir Oltean wrote: > Hi Rasmus, > > On Tue, May 17, 2022 at 04:27:06PM +0200, Rasmus Villemoes wrote: >> Since the phy_{read,write}_mmd functions are static inlines using >> other static inline functions, they cause code using them to explode. >> >> Defining local wrappers cuts the size of the generated code by 50%: >> >> $ size drivers/net/phy/dp83867.o.{before,after} >> text data bss dec hex filename >> 4873 112 0 4985 1379 drivers/net/phy/dp83867.o.before >> 2413 112 0 2525 9dd drivers/net/phy/dp83867.o.after >> >> Of course, most of that improvement could also be had by making the >> phy_*_mmd functions out-of-line, and they probably should be, but this ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> still has the advantage of avoiding passing the DP83867_DEVADDR >> argument at all call sites, which allows lines to be unwrapped (and >> probably also gives a little .text reduction by itself). >> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> >> --- > > Have you considered making phy_read_mmd() and phy_write_mmd() non-inline? > There are few users, but it looks like they would all benefit from this. Yes, I wrote precisely that in the commit message. But the problem with that is finding some TU to put them in which is guaranteed to be built and included by all current users. This localized change gives just about the same benefit in .text, plus the line unwrapping. And nothing prevents somebody later from figuring out a proper place to put out-of-line versions of these phy_*_mmd functions. Rasmus
On Tue, May 24, 2022 at 01:31:41PM +0200, Rasmus Villemoes wrote: > On 19/05/2022 16.38, Vladimir Oltean wrote: > > Hi Rasmus, > > > > On Tue, May 17, 2022 at 04:27:06PM +0200, Rasmus Villemoes wrote: > >> Since the phy_{read,write}_mmd functions are static inlines using > >> other static inline functions, they cause code using them to explode. > >> > >> Defining local wrappers cuts the size of the generated code by 50%: > >> > >> $ size drivers/net/phy/dp83867.o.{before,after} > >> text data bss dec hex filename > >> 4873 112 0 4985 1379 drivers/net/phy/dp83867.o.before > >> 2413 112 0 2525 9dd drivers/net/phy/dp83867.o.after > >> > >> Of course, most of that improvement could also be had by making the > >> phy_*_mmd functions out-of-line, and they probably should be, but this > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > >> still has the advantage of avoiding passing the DP83867_DEVADDR > >> argument at all call sites, which allows lines to be unwrapped (and > >> probably also gives a little .text reduction by itself). > >> > >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > >> --- > > > > Have you considered making phy_read_mmd() and phy_write_mmd() non-inline? > > There are few users, but it looks like they would all benefit from this. > > Yes, I wrote precisely that in the commit message. But the problem with > that is finding some TU to put them in which is guaranteed to be built > and included by all current users. This localized change gives just > about the same benefit in .text, plus the line unwrapping. And nothing > prevents somebody later from figuring out a proper place to put > out-of-line versions of these phy_*_mmd functions. I don't see why the translation unit you mention cannot be drivers/net/phy/phy.c. For phy_read_mmd() and phy_write_mmd() this is even pretty easy to see, as all the callers are either PHY drivers or cmd/mdio.c which itself depends on CONFIG_PHYLIB. The phy_read() and phy_write() calls themselves can also be probably be uninlined as a further exercise, but I didn't request that. But yeah, ok, whatever.
On Tue, May 24, 2022 at 3:08 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Tue, May 24, 2022 at 01:31:41PM +0200, Rasmus Villemoes wrote: > > On 19/05/2022 16.38, Vladimir Oltean wrote: > > > Hi Rasmus, > > > > > > On Tue, May 17, 2022 at 04:27:06PM +0200, Rasmus Villemoes wrote: > > >> Since the phy_{read,write}_mmd functions are static inlines using > > >> other static inline functions, they cause code using them to explode. > > >> > > >> Defining local wrappers cuts the size of the generated code by 50%: > > >> > > >> $ size drivers/net/phy/dp83867.o.{before,after} > > >> text data bss dec hex filename > > >> 4873 112 0 4985 1379 drivers/net/phy/dp83867.o.before > > >> 2413 112 0 2525 9dd drivers/net/phy/dp83867.o.after > > >> > > >> Of course, most of that improvement could also be had by making the > > >> phy_*_mmd functions out-of-line, and they probably should be, but this > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > >> still has the advantage of avoiding passing the DP83867_DEVADDR > > >> argument at all call sites, which allows lines to be unwrapped (and > > >> probably also gives a little .text reduction by itself). > > >> > > >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > > >> --- > > > > > > Have you considered making phy_read_mmd() and phy_write_mmd() non-inline? > > > There are few users, but it looks like they would all benefit from this. > > > > Yes, I wrote precisely that in the commit message. But the problem with > > that is finding some TU to put them in which is guaranteed to be built > > and included by all current users. This localized change gives just > > about the same benefit in .text, plus the line unwrapping. And nothing > > prevents somebody later from figuring out a proper place to put > > out-of-line versions of these phy_*_mmd functions. > > I don't see why the translation unit you mention cannot be drivers/net/phy/phy.c. > For phy_read_mmd() and phy_write_mmd() this is even pretty easy to see, > as all the callers are either PHY drivers or cmd/mdio.c which itself > depends on CONFIG_PHYLIB. > > The phy_read() and phy_write() calls themselves can also be probably be > uninlined as a further exercise, but I didn't request that. > > But yeah, ok, whatever. Nack. Let's fix the root problem, phy_write_mmd/phy_read_mmd should have never been static inline. They are too big for that. I'll create a fix for that.
diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c index 3d862636b6..766457dfd2 100644 --- a/drivers/net/phy/dp83867.c +++ b/drivers/net/phy/dp83867.c @@ -126,20 +126,30 @@ struct dp83867_private { bool sgmii_ref_clk_en; }; +static int dp83867_read_mmd(struct phy_device *phydev, int regnum) +{ + return phy_read_mmd(phydev, DP83867_DEVADDR, regnum); +} + +static int dp83867_write_mmd(struct phy_device *phydev, int regnum, u16 val) +{ + return phy_write_mmd(phydev, DP83867_DEVADDR, regnum, val); +} + static int dp83867_config_port_mirroring(struct phy_device *phydev) { struct dp83867_private *dp83867 = (struct dp83867_private *)phydev->priv; u16 val; - val = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4); + val = dp83867_read_mmd(phydev, DP83867_CFG4); if (dp83867->port_mirroring == DP83867_PORT_MIRRORING_EN) val |= DP83867_CFG4_PORT_MIRROR_EN; else val &= ~DP83867_CFG4_PORT_MIRROR_EN; - phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_CFG4, val); + dp83867_write_mmd(phydev, DP83867_CFG4, val); return 0; } @@ -191,8 +201,7 @@ static int dp83867_of_init(struct phy_device *phydev) * mode, but rgmii should have meant no delay. Warn existing users. */ if (phydev->interface == PHY_INTERFACE_MODE_RGMII) { - u16 val = phy_read_mmd(phydev, DP83867_DEVADDR, - DP83867_STRAP_STS2); + u16 val = dp83867_read_mmd(phydev, DP83867_STRAP_STS2); u16 txskew = (val & DP83867_STRAP_STS2_CLK_SKEW_TX_MASK) >> DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT; u16 rxskew = (val & DP83867_STRAP_STS2_CLK_SKEW_RX_MASK) >> @@ -282,11 +291,9 @@ static int dp83867_config(struct phy_device *phydev) /* Mode 1 or 2 workaround */ if (dp83867->rxctrl_strap_quirk) { - val = phy_read_mmd(phydev, DP83867_DEVADDR, - DP83867_CFG4); + val = dp83867_read_mmd(phydev, DP83867_CFG4); val &= ~BIT(7); - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_CFG4, val); + dp83867_write_mmd(phydev, DP83867_CFG4, val); } if (phy_interface_is_rgmii(phydev)) { @@ -312,15 +319,14 @@ static int dp83867_config(struct phy_device *phydev) * register's bit 11 (marked as RESERVED). */ - bs = phy_read_mmd(phydev, DP83867_DEVADDR, DP83867_STRAP_STS1); + bs = dp83867_read_mmd(phydev, DP83867_STRAP_STS1); if (bs & DP83867_STRAP_STS1_RESERVED) val &= ~DP83867_PHYCR_RESERVED_MASK; ret = phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL, val); - val = phy_read_mmd(phydev, DP83867_DEVADDR, - DP83867_RGMIICTL); + val = dp83867_read_mmd(phydev, DP83867_RGMIICTL); val &= ~(DP83867_RGMII_TX_CLK_DELAY_EN | DP83867_RGMII_RX_CLK_DELAY_EN); @@ -334,20 +340,18 @@ static int dp83867_config(struct phy_device *phydev) if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) val |= DP83867_RGMII_RX_CLK_DELAY_EN; - phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_RGMIICTL, val); + dp83867_write_mmd(phydev, DP83867_RGMIICTL, val); delay = (dp83867->rx_id_delay | (dp83867->tx_id_delay << DP83867_RGMII_TX_CLK_DELAY_SHIFT)); - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_RGMIIDCTL, delay); + dp83867_write_mmd(phydev, DP83867_RGMIIDCTL, delay); } if (phy_interface_is_sgmii(phydev)) { if (dp83867->sgmii_ref_clk_en) - phy_write_mmd(phydev, DP83867_DEVADDR, DP83867_SGMIICTL, - DP83867_SGMII_TYPE); + dp83867_write_mmd(phydev, DP83867_SGMIICTL, DP83867_SGMII_TYPE); phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, (BMCR_ANENABLE | BMCR_FULLDPLX | BMCR_SPEED1000)); @@ -361,8 +365,7 @@ static int dp83867_config(struct phy_device *phydev) MII_DP83867_CFG2_SPEEDOPT_INTLOW); phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_CFG2, cfg2); - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_RGMIICTL, 0x0); + dp83867_write_mmd(phydev, DP83867_RGMIICTL, 0x0); phy_write(phydev, MDIO_DEVAD_NONE, MII_DP83867_PHYCTRL, DP83867_PHYCTRL_SGMIIEN | @@ -374,14 +377,11 @@ static int dp83867_config(struct phy_device *phydev) } if (dp83867->io_impedance >= 0) { - val = phy_read_mmd(phydev, - DP83867_DEVADDR, - DP83867_IO_MUX_CFG); + val = dp83867_read_mmd(phydev, DP83867_IO_MUX_CFG); val &= ~DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL; val |= dp83867->io_impedance & DP83867_IO_MUX_CFG_IO_IMPEDANCE_CTRL; - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_IO_MUX_CFG, val); + dp83867_write_mmd(phydev, DP83867_IO_MUX_CFG, val); } if (dp83867->port_mirroring != DP83867_PORT_MIRRORING_KEEP) @@ -389,8 +389,7 @@ static int dp83867_config(struct phy_device *phydev) /* Clock output selection if muxing property is set */ if (dp83867->set_clk_output) { - val = phy_read_mmd(phydev, DP83867_DEVADDR, - DP83867_IO_MUX_CFG); + val = dp83867_read_mmd(phydev, DP83867_IO_MUX_CFG); if (dp83867->clk_output_sel == DP83867_CLK_O_SEL_OFF) { val |= DP83867_IO_MUX_CFG_CLK_O_DISABLE; @@ -400,8 +399,7 @@ static int dp83867_config(struct phy_device *phydev) val |= dp83867->clk_output_sel << DP83867_IO_MUX_CFG_CLK_O_SEL_SHIFT; } - phy_write_mmd(phydev, DP83867_DEVADDR, - DP83867_IO_MUX_CFG, val); + dp83867_write_mmd(phydev, DP83867_IO_MUX_CFG, val); } genphy_config_aneg(phydev);
Since the phy_{read,write}_mmd functions are static inlines using other static inline functions, they cause code using them to explode. Defining local wrappers cuts the size of the generated code by 50%: $ size drivers/net/phy/dp83867.o.{before,after} text data bss dec hex filename 4873 112 0 4985 1379 drivers/net/phy/dp83867.o.before 2413 112 0 2525 9dd drivers/net/phy/dp83867.o.after Of course, most of that improvement could also be had by making the phy_*_mmd functions out-of-line, and they probably should be, but this still has the advantage of avoiding passing the DP83867_DEVADDR argument at all call sites, which allows lines to be unwrapped (and probably also gives a little .text reduction by itself). Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/net/phy/dp83867.c | 52 +++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 27 deletions(-)