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@xxxxxxxxx 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 |
