On 2/23/23 00:21, Vladimir Oltean wrote:
[...]
, and I really have nothing else to base my judgement
on, than your hint that there is a bug there, and the code. But the
driver might behave in much more subtle ways which I may be completely
missing, and I may think that I'm fixing something when I'm not. I have
no way to know that except by booting a board, which I do not have (but
you do).
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 .
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().
...
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.
[...]
