Re: [Patch net-next v2 0/9] net: dsa: microchip: add support for phylink mac config and link up

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Wed, Aug 31, 2022 at 06:18:59PM +0300, Vladimir Oltean wrote:
> On Wed, Aug 31, 2022 at 09:43:24AM +0200, Oleksij Rempel wrote:
> > > > 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.

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