Re: [net-next PATCH v11 5/9] mfd: an8855: Add support for Airoha AN8855 Switch MFD

Christian Marangi
2024-12-14T15:13:30Z

On Wed, Dec 11, 2024 at 01:48:03AM +0200, Vladimir Oltean wrote:
> On Tue, Dec 10, 2024 at 11:32:17PM +0100, Christian Marangi wrote:
> > Doesn't regmap add lots of overhead tho? Maybe I should really change
> > the switch regmap to apply a save/restore logic?
> >
> > With an implementation like that current_page is not needed anymore.
> > And I feel additional read/write are ok for switch OP.
> >
> > On mdio I can use the parent-mdio-bus property to get the bus directly
> > without using MFD priv.
> >
> > What do you think?
>
> If performance is a relevant factor at all, it will be hard to measure it, other
> than with synthetic tests (various mixes of switch and PHY register access).
> Though since you mention it, it would be interesting to see a comparison of the
> 3 arbitration methods. This could probably be all done from the an8855_mfd_probe()
> calling context: read a switch register and a PHY register 100K times and see how
> long it took, then read 2 switch registers and a PHY register 100K times, then
> 3 switch registers.... At some point, we should start seeing the penalty of the
> page restoration in Andrew's proposal, because that will be done after each switch
> register read. Just curious to put it into perspective and see how soon it starts
> to make a difference. And this test will also answer the regmap overhead issue.

Ok sorry for the delay as I had to tackle an annoying crypto driver...

I was also curious about this and I hope I tested this correctly...

The testing code is this. Following Vladimir testing and simple time
comparision before and after. I used 100 times as 100k was very big.
From the results we can derive our conclusions.

static void test(struct an8855_mfd_priv *priv, struct regmap *regmap, struct regmap *regmap_phy)
{
ktime_t start_time, end_time;
// struct mii_bus *bus = priv->bus;
s64 elapsed_ns;
u32 val;
int times = 1;
int i, j;

start_time = ktime_get();
for (i = 0; i < 100; i++) {
for (j = 0; j < times; j++) {
regmap_read(regmap, 0x10005000, &val);
}
// mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
// // an8855_mii_set_page(priv, priv->switch_addr, 0);
// __mdiobus_read(bus, priv->switch_addr, 0x1);
// mutex_unlock(&bus->mdio_lock);
regmap_read(regmap_phy,
FIELD_PREP(GENMASK(20, 16), priv->switch_addr) |
FIELD_PREP(GENMASK(15, 0), 0x1),
&val);
times++;
}

end_time = ktime_get();

elapsed_ns = ktime_to_ns(ktime_sub(end_time, start_time));

pr_info("Time spent in the code block: %lld ns\n", elapsed_ns);
}

With the code changed accordingly.

switch regmap + page (proposed implementation)
Time spent in the code block: 866179846 ns

switch regmap + phy regmap (proposed implementation + PHY regmap)
Time spent in the code block: 861326846 ns

switch regmap restore (switch regmap read/set/restore page)
Time spent in the code block: 1151011308 ns

switch regmap restore + phy regmap (switch regmap read/set/restore pgae
+ PHY regmap)
Time spent in the code block: 1147400462 ns

We can see that:
- as suggested regmap doesn't cause any performance penality. It does
even produce better result.
- the read/set/restore implementation gives worse performance.

So I guess I will follow the path of regmap + cache page. What do you
think?

--
Ansuel