Re: [PATCH] net: dsa: microchip: Fix gigabit set and get function for KSZ87xx

Marek Vasut <marex@xxxxxxx> · Thu, 23 Feb 2023 06:17:28 +0100




On 2/23/23 01:22, Vladimir Oltean wrote:
Hi Marek,

On Thu, Feb 23, 2023 at 12:55:08AM +0100, Marek Vasut wrote:
The old code, removed in:
c476bede4b0f0 ("net: dsa: microchip: ksz8795: use common xmii function")
used ksz_write8() (this part is important):
ksz_write8(dev, REG_PORT_5_CTRL_6, data8);
where:
drivers/net/dsa/microchip/ksz8795_reg.h:#define REG_PORT_5_CTRL_6
0x56

The new code, where the relevant part is added in (see Fixes tag)
46f80fa8981bc ("net: dsa: microchip: add common gigabit set and get
function")
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -257,6 +257,7 @@ static const u16 ksz8795_regs[] = {
+       [P_XMII_CTRL_1]                 = 0x56,
uses ksz_pwrite8() (with p in the function name, p means PORT):
ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8);
which per drivers/net/dsa/microchip/ksz_common.h translates to
ksz_write8(dev, dev->dev_ops->get_port_addr(port, offset), data);
and that dev->dev_ops->get_port_addr(port, offset) remapping function is per
drivers/net/dsa/microchip/ksz8795.c really call to the following macro:
PORT_CTRL_ADDR(port, offset)
which in turn from drivers/net/dsa/microchip/ksz8795_reg.h becomes
#define PORT_CTRL_ADDR(port, addr) ((addr) + REG_PORT_1_CTRL_0 + (port) * (REG_PORT_2_CTRL_0 - REG_PORT_1_CTRL_0))

That means:
ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
writes register 0xa6 instead of register 0x56, because it calls the
PORT_CTRL_ADDR(port, 0x56)=0xa6, but in reality it should call
PORT_CTRL_ADDR(port, 0x06)=0x56, i.e. the remapping should happen ONCE, the
value 0x56 is already remapped .

I never had any objection to this part.

All the call-sites which do
ksz_pwrite8(dev, port, regs[P_XMII_CTRL_1], data8)
or
ksz_pread8(dev, port, regs[P_XMII_CTRL_1], &data8)
are affected by this, all six, that means the ksz_[gs]et_xmii() and the
ksz_[gs]et_gbit().

I'm going to say this with a very calm tone, please tell me where it's wrong
and we can go from there.

  Not for the ksz_switch_chips[] elements which point to ksz8795_regs (which
  had the incorrect value you're fixing), it isn't. You're making an argument
  for code which never executes (5 out of 6 call paths), and basing your commit
  message off of it. Your commit message reads as if you didn't even notice
  that ksz_set_gbit() isn't called for ksz87xx, and that this is a bug in itself.
  Moreover, the problem you're seeing (I may speculate what that is, but
  I don't know) is surely not due to ksz_set_gbit() being called on the
  wrong register, because it's not called at all *for that hardware*.

That gigabit bug was pointed out to you by reviewers, and you refuse to
acknowledge this and keep bringing forth some other stuff which was never
debated by anyone. The lack of acknowledgement plus your continuation to
randomly keep singing another tune in another key completely is irritating
to me on a very personal (non-technical) level. To respond to you, I am
exercising some mental muscles which I wish I wouldn't have needed to,
here, in this context. The same muscles I use when I need to identify
manipulation on tass.com.

[ in case the message above is misinterpreted: I did not say that you
   willingly manipulate. I said that your lack of acknowledgement to what
   is being said to you is giving me the same kind of frustration ]

This is my feedback to the tone in your replies. I want you to give your
feedback to my tone now too. You disregarded that.

...

If all that should be changed in the commit message is "to access the
P_GMII_1GBIT_M, i.e. Is_1Gbps, bit" to something from the "ksz_set_xmii()"
function instead, then just say so.

[...]

No, this is not all that I want.

The gigabit bug changes things in ways in which I'm curious how you're
going to try to defend, with this attitude of responding to anything
except to what was asked. Your commit says it fixes gigabit on KSZ87xx

No, it does not say it fixes gigabit on KSZ87xx, the commit message says 
it fixes gigabit accessor functions, but what it really fixes (and what 
is the bulk of the commit message) is the incorrectly double-remapped 
register 0x56 used in those gigabit accessor functions and which is also 
used in the ksz_[gs]et_xmii function.

but the gigabit bug which *was pointed out to you by others* is still
there. Your patch fixes something else, but *it says* it fixes a gigabit
bug. What I want is for you to describe exactly what it fixes, or if you
just don't know, say you noticed the bug during code review and you
don't know what is its real life impact (considering pin strapping).

I believe I wrote what the problem is in my previous email, the 
incorrectly double-remapped XMII_1 register . The register wasn't 
updated in my case in ksz_set_xmii() with RGMII delays.

Why I picked the is_1Gbit bit for the commit message, probably was tired 
after lengthy debugging session of this problem.

I don't want a patch to be merged which says it fixes something it doesn't
fix, while leaving the exact thing it says it fixes unfixed.

I also don't want to entertain this game of "if it's just this small
thing, why didn't you say so". I would be setting myself up for an
endless time waste if I were to micromanage your commit message writing.

I am looking forward to a productive conversation with you, but if your
next reply is going to have the same strategy of avoiding my key message
and focusing on some other random thing, then I'm very sorry, but I'll
just have to focus my attention somewhere else.

[...]






