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.
[...]