Messages in this thread | | | Date | Mon, 18 Jul 2022 22:35:21 +0300 | From | Vladimir Oltean <> | 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), ®); 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), ®); 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), ®); 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), ®); 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), ®); 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), ®); 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.
| |