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

[Christian Marangi <ansuelsmth@gmail.com>]

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