On Wed, 2022-08-31 at 18:10 +0200, Oleksij Rempel wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

On Wed, Aug 31, 2022 at 06:18:59PM +0300, Vladimir Oltean wrote:

quoted

On Wed, Aug 31, 2022 at 09:43:24AM +0200, Oleksij Rempel wrote:

quoted

quoted

quoted

Should we point them to ksz8795_xmii_ctrl0 and
ksz8795_xmii_ctrl1? I don't know.
Could you find out what these should be set to?

xmii_ctrl0/1 are missing and it make no sense to add it.
KSZ8873 switch is controlling CPU port MII configuration over
global,
not port based register.

I'll better define separate ops for this chip.

Hm, not only KSZ8830/KSZ8863/KSZ8873 are affected. ksz8795
compatible
series with defined .xmii_ctrl0/.xmii_ctrl1 are broken too.
Because it
is writing to the global config register over ksz_pwrite8
function. It
means, we are writing to 0xa6 instead of 0x06. And to 0xf6
instead of
0x56.

What do you mean by "global config register"? The Is_1Gbps bit is
still
part of a port register, it's just that this particular register is
only
defined for the 5th port (port #4, the only xMII port on KSZ7895
AFAIU).
That doesn't necessarily make it a "global" register.

Datasheet says:

Register 22 (0x16): Reserved
Register 38 (0x26): Reserved
Register 54 (0x36): Reserved
Register 70 (0x46): Reserved
Register 86 (0x56): Port 5 Interface Control 6

I wonder if it's ok to modify the regs table like this, because we
should then only touch P_XMII_CTRL_1 using port 4:

 static const u16 ksz8795_regs[] = {
      [REG_IND_CTRL_0]                = 0x6E,
      [REG_IND_DATA_8]                = 0x70,
      [REG_IND_DATA_CHECK]            = 0x72,
      [REG_IND_DATA_HI]               = 0x71,
      [REG_IND_DATA_LO]               = 0x75,
      [REG_IND_MIB_CHECK]             = 0x74,
      [REG_IND_BYTE]                  = 0xA0,
      [P_FORCE_CTRL]                  = 0x0C,
      [P_LINK_STATUS]                 = 0x0E,
      [P_LOCAL_CTRL]                  = 0x07,
      [P_NEG_RESTART_CTRL]            = 0x0D,
      [P_REMOTE_STATUS]               = 0x08,
      [P_SPEED_STATUS]                = 0x09,
      [S_TAIL_TAG_CTRL]               = 0x0C,
      [P_STP_CTRL]                    = 0x02,
      [S_START_CTRL]                  = 0x01,
      [S_BROADCAST_CTRL]              = 0x06,
      [S_MULTICAST_CTRL]              = 0x04,
      [P_XMII_CTRL_0]                 = 0x06,
-     [P_XMII_CTRL_1]                 = 0x56,
+     [P_XMII_CTRL_1]                 = 0x06,
 };

Speed configuration on ksz8795 is done over two registers:
Register 86 (0x56): Port 5 Interface Control 6: Is_1Gbps - BIT(6)
and
Register 6 (0x06): Global Control 4: Switch SW5-MII/RMII Speed
-BIT(4)

both are accessed on wrong offsets.

I would prefer to do following steps:
- remove everything from ksz_phylink_mac_link_up() except of
  dev->dev_ops->phylink_mac_link_up
- move ksz_duplex_flowctrl(), ksz_port_set_xmii_speed()... to
ksz9477.c
  and rename them. Assign ksz9477_phylink_mac_link_up()
  dev->dev_ops->phylink_mac_link_up
- create separate function ksz8795_phylink_mac_link_up()
- use documented, not generic register names.

In the original code, the ksz8795_cpu_interface_select (which does the
xmii and speed configuration) is called only ksz87xx switch not for the
ksz88x3.
During refactoring, I intentionally did not add the P_XMII_CTRL0/1 for
the ksz88xx in the chip_data structure.
I added check that if switch belong to ksz88x3 then return in the
phylink_mac_config function but I missed to add in the
phylink_mac_link_up. Thats the mistake I have done. So, for the ksz88xx
switch code breakage, it could be fixed by adding the check in the
phylink_mac_link_up as well.

For the code breakage in ksz8795/ksz8794, the original code has only
provision for configuring xmii mode and choosing the 100/1000Mbps speed
selection which could be selected using is_1Gbps bit of
port5_interface_control6 register (0x56). But it didn't have provision
to select speed between 10/100Mbps, flow control and duplex.

As per vladimir suggestion, P_XMII_CTRL1 can be changed from 0x56 to
0x06. It fixes the problem for ksz_set_xmii, ksz_set_gbit since this is
the port based register not the global register.

The global register 0x06 responsibilities are bit 4 for 10/100mbps
speed selection, bit 5 for flow control and bit 6  for duplex
operation. Since these three are new features added during refactoring
I overlooked it. 
To fix this, either I need to return from the ksz_set_100_10mbit &
ksz_duplex_flowctrl function if the chip_id is ksz87xx or add dev-

dev_ops for this alone. 

Kindly suggest on how to proceed.

--
Pengutronix
e.K.                           |                             |
Steuerwalder Str. 21                       | 
http://www.pengutronix.de/e/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-
0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-
5555 |