Re: [PATCH 2/4] net: dsa: microchip: ksz8795: add Wake on LAN support

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

Hi Pieter,

If I see it correctly, the only difference between KSZ9477 and KSZ8795
code is the register access. Even bit offsets are identical. I do not
think indirect register access is good justification for duplication
this amount of code.

On Wed, Jul 17, 2024 at 09:37:23PM +0200, vtpieter@gmail.com wrote:
>  
> +static int ksz8_ind_read8(struct ksz_device *dev, u8 table, u16 addr, u8 *val)

Will be good to add comment for this function.

....
> +/**
> + * ksz8_handle_wake_reason - Handle wake reason on a specified port.
> + * @dev: The device structure.
> + * @port: The port number.
> + *
> + * This function reads the PME (Power Management Event) status register of a
> + * specified port to determine the wake reason. If there is no wake event, it
> + * returns early. Otherwise, it logs the wake reason which could be due to a
> + * "Magic Packet", "Link Up", or "Energy Detect" event. The PME status register
> + * is then cleared to acknowledge the handling of the wake event; followed by
> + * clearing the global Interrupt Status Register.
> + *
> + * Return: 0 on success, or an error code on failure.
> + */
> +static int ksz8_handle_wake_reason(struct ksz_device *dev, int port)
> +{
> +	u8 pme_status;
> +	int ret;
> +
> +	ret = ksz8_ind_read8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_STATUS, &pme_status);
> +	if (ret)
> +		return ret;
> +
> +	if (!pme_status)
> +		return 0;
> +
> +	dev_dbg(dev->dev, "Wake event on port %d due to:%s%s%s\n", port,
> +		pme_status & PME_WOL_MAGICPKT ? " \"Magic Packet\"" : "",
> +		pme_status & PME_WOL_LINKUP ? " \"Link Up\"" : "",
> +		pme_status & PME_WOL_ENERGY ? " \"Energy detect\"" : "");
> +
> +	ret = ksz8_ind_write8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_STATUS, pme_status);
> +	if (ret)
> +		return ret;
> +
> +	ksz_read8(dev, REG_INT_STATUS, &pme_status);

Recycling a variable with use case specific name, make the code more
confusing. Use "var" far all or "int_status" for this case.

> +	return ksz_write8(dev, REG_INT_STATUS, pme_status && INT_PME);

"pme_status && INT_PME" will write BIT(0) instead of BIT(4) which should
be written in this case - So, it should be "pme_status & INT_PME".

Instead of ksz_read8(dev, REG_INT_STATUS + ksz_write8, you can use one
ksz_rmw8()

> +}
> +
> +/**
> + * ksz8_get_wol - Get Wake-on-LAN settings for a specified port.
> + * @dev: The device structure.
> + * @port: The port number.
> + * @wol: Pointer to ethtool Wake-on-LAN settings structure.
> + *
> + * This function checks the PME 'wakeup-source' property from the
> + * device tree. If enabled, it sets the supported and active WoL
> + * flags.

This is a bit confusing - this function do not checks devicetree properly.
It only checks if the wakeup_source flag is set. In current code state, it is
set from devicetree properly.

> + */
> +void ksz8_get_wol(struct ksz_device *dev, int port,
> +		  struct ethtool_wolinfo *wol)
> +{
> +	u8 pme_ctrl;
> +	int ret;
> +
> +	if (!dev->wakeup_source)
> +		return;
> +
> +	wol->supported = WAKE_PHY;
> +
> +	/* Check if the current MAC address on this port can be set
> +	 * as global for WAKE_MAGIC support. The result may vary
> +	 * dynamically based on other ports configurations.
> +	 */
> +	if (ksz_is_port_mac_global_usable(dev->ds, port))
> +		wol->supported |= WAKE_MAGIC;
> +
> +	ret = ksz8_ind_read8(dev, TABLE_PME_PORT(port), REG_IND_PORT_PME_CTRL, &pme_ctrl);
> +	if (ret)
> +		return;
> +
> +	if (pme_ctrl & PME_WOL_MAGICPKT)
> +		wol->wolopts |= WAKE_MAGIC;
> +	if (pme_ctrl & (PME_WOL_LINKUP | PME_WOL_ENERGY))
> +		wol->wolopts |= WAKE_PHY;
> +}
> +
> +/**
> + * ksz8_set_wol - Set Wake-on-LAN settings for a specified port.
> + * @dev: The device structure.
> + * @port: The port number.
> + * @wol: Pointer to ethtool Wake-on-LAN settings structure.
> + *
> + * This function configures Wake-on-LAN (WoL) settings for a specified port.
> + * It validates the provided WoL options, checks if PME is enabled via the
> + * switch's device tree property, clears any previous wake reasons,

Same here, the "device tree" related part of comment can bit rot with
time.

> + * and sets the Magic Packet flag in the port's PME control register if
> + * specified.
> + *
> + * Return: 0 on success, or other error codes on failure.
> + */
> +int ksz8_set_wol(struct ksz_device *dev, int port,
> +/**
> + * ksz9477_wol_pre_shutdown - Prepares the switch device for shutdown while

s/ksz9477_wol_pre_shutdown/ksz8_wol_pre_shutdown

> + *                            considering Wake-on-LAN (WoL) settings.
> + * @dev: The switch device structure.
> + * @wol_enabled: Pointer to a boolean which will be set to true if WoL is
> + *               enabled on any port.
> + *
> + * This function prepares the switch device for a safe shutdown while taking
> + * into account the Wake-on-LAN (WoL) settings on the user ports. It updates
> + * the wol_enabled flag accordingly to reflect whether WoL is active on any
> + * port. It also sets the PME output pin enable with the polarity specified
> + * through the device-tree.
> + */
> +void ksz8_wol_pre_shutdown(struct ksz_device *dev, bool *wol_enabled)
> +{

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index b074b4bb0629..61403898c1f4 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -307,6 +307,9 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
>  	.init = ksz8_switch_init,
>  	.exit = ksz8_switch_exit,
>  	.change_mtu = ksz8_change_mtu,
> +	.get_wol = ksz8_get_wol,
> +	.set_wol = ksz8_set_wol,
> +	.wol_pre_shutdown = ksz8_wol_pre_shutdown,

This part will break on KSZ8830 variants. There is no WoL support.

Regards,
Oleksij
-- 
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 |