On 2/22/23 22:08, Vladimir Oltean wrote:
Please summarize in the commit title what is the user-visible impact of
the problem that is being fixed. Short and to the point.
Can you suggest a Subject which is acceptable ?
On Wed, Feb 22, 2023 at 04:17:38AM +0100, Marek Vasut wrote:
Per KSZ8794 [1] datasheet DS00002134D page 54 TABLE 4-4: PORT REGISTERS,
it is Register 86 (0x56): Port 4 Interface Control 6 which contains the
Is_1Gbps field.
Good thing you mention Is_1Gbps (even though it's irrelevant to the
change you're proposing, since ksz_port_set_xmii_speed() is only called
by ksz9477_phylink_mac_link_up()).
That is actually what I want to bring up. If you change the speed in
your fixed-link nodes (CPU port and DSA master) to 100 Mbps on KSZ87xx,
does it work? No, right? Because P_GMII_1GBIT_M always remains at its
hardware default value, which is selected based on pin strapping.
That's a bug, and should be fixed too.
Sure, separate patch. The system I use has gigabit link to the switch.
Good thing you brought this up, I wouldn't have mentioned it if it
wasn't in the commit message.
Currently, the driver uses PORT read function on register P_XMII_CTRL_1
to access the P_GMII_1GBIT_M, i.e. Is_1Gbps, bit.
Provably false. The driver does do that, but not for KSZ87xx.
The driver uses port read function with register value 0x56 instead of
0x06 , which means the remapping happens twice, which provably breaks
the driver since commit Fixes below .
Please delete red herrings from the commit message, they do not help
assess users if they care about backporting a patch to a custom tree
or not.
The problem is, the register P_XMII_CTRL_1 address is already 0x56,
which is the converted PORT register address instead of the offset
within PORT register space that PORT read function expects and
converts into the PORT register address internally. The incorrectly
double-converted register address becomes 0xa6, which is what the PORT
read function ultimatelly accesses, and which is a non-existent
~~~~~~~~~~~
ultimately
register on the KSZ8794/KSZ8795 .
The correct value for P_XMII_CTRL_1 is 0x6, which gets converted into
port address 0x56, which is Register 86 (0x56): Port 4 Interface Control 6
per KSZ8794 datasheet, i.e. the correct register address.
To make this worse, there are multiple other call sites which read and
~~~~~~~~
multiple implies more than 1.
There is no call site other than ksz_set_xmii(). Please delete false
information from the commit message.
$ git grep P_XMII_CTRL_1 drivers/net/dsa/microchip/
drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1]
= 0x06,
drivers/net/dsa/microchip/ksz_common.c: [P_XMII_CTRL_1]
= 0x0301,
drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port,
regs[P_XMII_CTRL_1], &data8);
drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port,
regs[P_XMII_CTRL_1], data8);
drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port,
regs[P_XMII_CTRL_1], &data8);
drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port,
regs[P_XMII_CTRL_1], &data8);
drivers/net/dsa/microchip/ksz_common.c: ksz_pread8(dev, port,
regs[P_XMII_CTRL_1], &data8);
drivers/net/dsa/microchip/ksz_common.c: ksz_pwrite8(dev, port,
regs[P_XMII_CTRL_1], data8);
drivers/net/dsa/microchip/ksz_common.h: P_XMII_CTRL_1,
I count 6.
even write the P_XMII_CTRL_1 register, one of them is ksz_set_xmii(),
which is responsible for configuration of RGMII delays. These delays
are incorrectly configured and a non-existent register is written
without this change.
Not only RGMII delays, but also P_MII_SEL_M (interface mode selection).
The implication of writing the value at an undocumented address is that
the real register 0x56 remains with the value decided by pin strapping
(which may or may not be adequate for Linux runtime). This is absolutely
the same class of bug as what happens with Is_1Gbps.
Fix the P_XMII_CTRL_1 register offset to resolve these problems.
[1] https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/KSZ8794CNX-Data-Sheet-DS00002134.pdf
Fixes: 46f80fa8981b ("net: dsa: microchip: add common gigabit set and get function")
Technically, the problem was introduced by:
Fixes: c476bede4b0f ("net: dsa: microchip: ksz8795: use common xmii function")
because that's when ksz87xx was transitioned from the old logic (which
also used to set Is_1Gbps) to the new one.
And that same commit is also to blame for the Is_1Gbps bug, because the
new logic from ksz8795_cpu_interface_select() should have called not
only ksz_set_xmii(), but also ksz_set_gbit() for code-wise identical
behavior. It didn't do that. Then with commit f3d890f5f90e ("net: dsa:
microchip: add support for phylink mac config"), this incomplete
configuration just got moved around.
Signed-off-by: Marek Vasut <marex@xxxxxxx>
The contents of the patch is not wrong, but the commit message that
describes it misses a lot of points which make non-zero difference to
someone trying to assess whether a patch fixes a problem he's seeing or not.
OK, to make this simple, can you write a commit message which you
consider acceptable, to close this discussion ?
