Message-ID: <20220718193521.ap3fc7mzkpstw727@skbuf>
Date:   Mon, 18 Jul 2022 22:35:21 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Christian Marangi <ansuelsmth@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Russell King <linux@...linux.org.uk>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jens Axboe <axboe@...nel.dk>, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [net-next RFC PATCH 1/4] net: dsa: qca8k: drop
 qca8k_read/write/rmw for regmap variant

On Mon, Jul 18, 2022 at 08:40:14PM +0200, Christian Marangi wrote:
> > I don't really have a preference, I just want to understand why you want
> > to call regmap_read(priv->regmap) directly every time as opposed to
> > qca8k_read(priv) which is shorter to type and allows more stuff to fit
> > on one line.
> 
> The main reason is that it's one less function. qca8k_read calls
> directly the regmap ops so it seems a good time to drop it.

This is before applying your patch 1/4, with an armv7 compiler:
make drivers/net/dsa/qca/qca8k.lst

I'm looking at the qca8k_read() call from qca8k_pcs_get_state():

000009d8 <qca8k_pcs_get_state>:
{
     9d8:	e92d4030 	push	{r4, r5, lr}
     9dc:	e3005000 	movw	r5, #0
			9dc: R_ARM_MOVW_ABS_NC	__stack_chk_guard
	ret = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg);
     9e0:	e590300c 	ldr	r3, [r0, #12]
{
     9e4:	e3405000 	movt	r5, #0
			9e4: R_ARM_MOVT_ABS	__stack_chk_guard
     9e8:	e24dd00c 	sub	sp, sp, #12
     9ec:	e1a04001 	mov	r4, r1
	return regmap_read(priv->regmap, reg, val);
     9f0:	e5900008 	ldr	r0, [r0, #8]
     9f4:	e1a0200d 	mov	r2, sp
{
     9f8:	e595c000 	ldr	ip, [r5]
	ret = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port), &reg);
     9fc:	e283101f 	add	r1, r3, #31
	return regmap_read(priv->regmap, reg, val);
     a00:	e1a01101 	lsl	r1, r1, #2
     a04:	e5900010 	ldr	r0, [r0, #16]
{
     a08:	e58dc004 	str	ip, [sp, #4]
	return regmap_read(priv->regmap, reg, val);
     a0c:	ebfffffe 	bl	0 <regmap_read>
			a0c: R_ARM_CALL	regmap_read
(portions irrelevant to regmap cut out)

And this is how it looks like after applying your patch 1/4:

000009d8 <qca8k_pcs_get_state>:
{
     9d8:	e92d4030 	push	{r4, r5, lr}
     9dc:	e3005000 	movw	r5, #0
			9dc: R_ARM_MOVW_ABS_NC	__stack_chk_guard
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     9e0:	e590300c 	ldr	r3, [r0, #12]
{
     9e4:	e3405000 	movt	r5, #0
			9e4: R_ARM_MOVT_ABS	__stack_chk_guard
     9e8:	e24dd00c 	sub	sp, sp, #12
     9ec:	e1a04001 	mov	r4, r1
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     9f0:	e5900008 	ldr	r0, [r0, #8]
     9f4:	e1a0200d 	mov	r2, sp
{
     9f8:	e595c000 	ldr	ip, [r5]
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     9fc:	e283101f 	add	r1, r3, #31
     a00:	e1a01101 	lsl	r1, r1, #2
     a04:	e5900010 	ldr	r0, [r0, #16]
{
     a08:	e58dc004 	str	ip, [sp, #4]
	ret = regmap_read(priv->regmap, QCA8K_REG_PORT_STATUS(port), &reg);
     a0c:	ebfffffe 	bl	0 <regmap_read>
			a0c: R_ARM_CALL	regmap_read

You don't even need to recognize the instructions or calling conventions
to figure out that the generated assembly code is identical.

> > 
> > I think if you run "make drivers/net/dsa/qca/qca8k.lst" and you look at
> > the generated code listing before and after, you'll find it is identical
> > (note, I haven't actually done that).
> > 
> > > An alternative is to keep them for qca8k specific code and migrate the
> > > common function to regmap api.
> > 
> > No, that's silly and I can't even find a reason to do that.
> > It's not like you're trying to create a policy to not call qca8k-common.c
> > functions from qca8k-8xxx.c, right? That should work just fine (in this
> > case, qca8k_read etc).
> 
> The idea of qca8k-common is to keep them as generilized as possible.
> Considering ipq4019 will have a different way to write/read regs we can't
> lock common function to specific implementation.

Wait a minute, what's the difference between having this in common.c:

	qca8k_read(priv)

vs this:

	regmap_read(priv->regmap)

when qca8k_read is implemented *exactly* as a call to regmap_read(priv->regmap)?
There's nothing *specific* to a switch in the implementation of qca8k_read().
But rather, all differences lie in the regmap_config structure and in
the way the regmap was created. But the common code operates with a
pointer to a generic regmap structure, regardless of how that was created.

So no, sorry, there is no technical argument for which you cannot have
calls to qca8k_read() in common.c. I can work with "that's the way I prefer",
but let's not try to invent technical arguments when there aren't any.

> > In fact, while typing this I realized that in your code structure,
> > you'll have one struct dsa_switch_ops in qca8k-8xxx.c and another one in
> > qca8k-ipq4019.c. But the vast majority of dsa_switch_ops are common,
> > with the exception of .setup() which is switch-specific, correct?
> 
> Phylink ops will also be different as ipq4019 will have qsgmii and will
> require some calibration logic.

Ok, phylink too, the point is that they aren't radically different switches
for the majority of operations.

> qca8k_setup will require major investigation and I think it would be
> better to do do a qca8k_setup generalization when ipq4019 will be
> proposed.

Ok, "major investigation" sounds about right, that's what I was looking
to hear. The alternative would have been to plop a separate ipq4019_setup(),
leave qca8k_setup() alone, and call it a day. FWIW, that's essentially
where the microchip ksz set of drivers were, before Arun Ramadoss
started doing some major cleanup through them. After some point, this
strategy simply stops scaling.

> On the other hand I like the idea of putting the qca8k ops in common.c
> and make the driver adds the relevant specific options.
> Think I will also move that to common.c. That would permit to keep
> function static aka even less delta and less bloat in the header file.
> 
> (is it a problem if it won't be const?)

yeah, it's a problem if it won't be const, why wouldn't it?

> > If I were to summarize your reason, it would be "because I prefer it
> > that way and because now is a good time", right? That's fine with me,
> > but I honestly didn't understand that while reading the commit message.
> 
> I have to be honest... Yes you are right... This is really my opinion
> and I don't have a particular strong reason on why dropping them.
> 
> It's really that I don't like keeping function that are just leftover of
> an old implementation. But my target here is not argue and find a
> solution so it's OK for me if I should keep these compat function and
> migrate them to common.c.

I know that the revolutionary spirit can be strong, but it's good to keep
in mind that "older/newer" is not always synonymous with "worse/better" ;)

Again, I don't have a strong objection against the change and I'm not
going to argue about it either. My comment was simply because I didn't
physically UNDERSTAND you. My expectations were also a bit confused,
because I initially thought it's a necessary change (that's why I
replied to it last), and I just didn't understand what's so necessary
about it.

Powered by blists - more mailing lists