| Message ID | 20230721135501.1464455-7-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: microchip: provide Wake on LAN support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
> The switch MAC address is used for sending pause frames and for Wake on Magic > Packet. So, make use of local-mac-address property in the switch node > root and configure it in the HW. > > @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds) > if (dev->wakeup_source) > ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE); > > + if (is_valid_ether_addr(dev->mac_addr)) { > + int i; > + > + for (i = 0; i < ETH_ALEN; i++) > + ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, > + dev->mac_addr[i]); > + } > + > + > + mac = of_get_property(dev->dev->of_node, "local-mac-address", > + NULL); > + if (mac) > + memcpy(dev->mac_addr, mac, ETH_ALEN); So the Magic Packet uses the same MAC address for all KSZ9477 switches if local-mac-address is not defined. Is that practical in real operating environment? The DSA driver used to have an API to set the MAC address to the switch, but it was removed because nobody used it.
On Fri, Jul 21, 2023 at 10:09:57PM +0000, Tristram.Ha@microchip.com wrote: > > The switch MAC address is used for sending pause frames and for Wake on Magic > > Packet. So, make use of local-mac-address property in the switch node > > root and configure it in the HW. > > > > > @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds) > > if (dev->wakeup_source) > > ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE); > > > > + if (is_valid_ether_addr(dev->mac_addr)) { > > + int i; > > + > > + for (i = 0; i < ETH_ALEN; i++) > > + ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, > > + dev->mac_addr[i]); > > + } > > + > > > + > > + mac = of_get_property(dev->dev->of_node, "local-mac-address", > > + NULL); > > + if (mac) > > + memcpy(dev->mac_addr, mac, ETH_ALEN); > > So the Magic Packet uses the same MAC address for all KSZ9477 switches > if local-mac-address is not defined. Is that practical in real operating > environment? "same address" which is 00:00:00:00:00:00 AFAIU. Do you mean to ask "what do we do for systems without this device tree property, don't we want to support Magic Packet for them too?". If so, I agree, it is a valid concern (although we need to modify it anyway, to add "wakeup-source", apparently). Additionally, "local-mac-address" would be overloaded with a secondary meaning compared to what ethernet-controller.yaml says: Specifies the MAC address that was assigned to the network device. ...which this is not. Since the switch hardware doesn't have a register per user port that could be kept in sync with the MAC address of the Linux interface, it means that the address for WOL and for flow control will always require a special way for the user to find out about it. I'm thinking, would it be simpler to just program the hardware with the MAC address of the DSA master? Every DSA master has one; we can track changes to it; no special device tree properties are needed; it also kinda makes sense as a MAC SA for flow control as well; DSA user ports also inherit it if they don't have a local-mac-address of their own. If there needs to be an override for that auto-selected MAC address, a device tree property can be added later, in the "microchip," namespace. But "how does the user find out what MAC address to use for WOL" still remains a concern, when it's stuffed in the device tree. Is there going to be a BIOS customized for the KSZ9477 which always fixes up the local-mac-address of the switch node with what's set there? > The DSA driver used to have an API to set the MAC address to the switch, > but it was removed because nobody used it. > Which API is that? "git log -GREG_SW_MAC_ADDR_0 drivers/net/dsa/" came up with nothing.
On 7/21/2023 5:19 PM, Vladimir Oltean wrote: > On Fri, Jul 21, 2023 at 10:09:57PM +0000, Tristram.Ha@microchip.com wrote: >>> The switch MAC address is used for sending pause frames and for Wake on Magic >>> Packet. So, make use of local-mac-address property in the switch node >>> root and configure it in the HW. >>> >> >>> @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds) >>> if (dev->wakeup_source) >>> ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE); >>> >>> + if (is_valid_ether_addr(dev->mac_addr)) { >>> + int i; >>> + >>> + for (i = 0; i < ETH_ALEN; i++) >>> + ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, >>> + dev->mac_addr[i]); >>> + } >>> + >> >>> + >>> + mac = of_get_property(dev->dev->of_node, "local-mac-address", >>> + NULL); >>> + if (mac) >>> + memcpy(dev->mac_addr, mac, ETH_ALEN); >> >> So the Magic Packet uses the same MAC address for all KSZ9477 switches >> if local-mac-address is not defined. Is that practical in real operating >> environment? > > "same address" which is 00:00:00:00:00:00 AFAIU. > > Do you mean to ask "what do we do for systems without this device tree > property, don't we want to support Magic Packet for them too?". If so, > I agree, it is a valid concern (although we need to modify it anyway, > to add "wakeup-source", apparently). > > Additionally, "local-mac-address" would be overloaded with a secondary > meaning compared to what ethernet-controller.yaml says: > > Specifies the MAC address that was assigned to the network device. > > ...which this is not. > > Since the switch hardware doesn't have a register per user port that > could be kept in sync with the MAC address of the Linux interface, it > means that the address for WOL and for flow control will always require > a special way for the user to find out about it. > > I'm thinking, would it be simpler to just program the hardware with the > MAC address of the DSA master? Every DSA master has one; we can track > changes to it; no special device tree properties are needed; it also > kinda makes sense as a MAC SA for flow control as well; DSA user ports > also inherit it if they don't have a local-mac-address of their own. > If there needs to be an override for that auto-selected MAC address, a > device tree property can be added later, in the "microchip," namespace. Yes the MAC address of the DSA master should be used, that is what we would naturally want to target magic packets with. > > But "how does the user find out what MAC address to use for WOL" still > remains a concern, when it's stuffed in the device tree. Is there going > to be a BIOS customized for the KSZ9477 which always fixes up the > local-mac-address of the switch node with what's set there? Agreed, I don't think we should be accepting that 'local-mac-address' be specified, it does not serve any good and only makes everything more confusing because this is not the MAC address that is set into the port's net_device::dev_addr at all. > >> The DSA driver used to have an API to set the MAC address to the switch, >> but it was removed because nobody used it. >> > > Which API is that? "git log -GREG_SW_MAC_ADDR_0 drivers/net/dsa/" came > up with nothing. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=841f4f24053acad69240c6ab7427a1d24bc29491
> The DSA driver used to have an API to set the MAC address to the switch, > but it was removed because nobody used it. That is a long time ago, when Marvell was about the only supported vendor. As far as i understood, it was used to set the MAC source address used when sending pause frames. But since pause frames are link local by definition, and the switches had a reasonable default, it was removed. Andrew
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c index 69909241b1f44..427176c1441f7 100644 --- a/drivers/net/dsa/microchip/ksz9477.c +++ b/drivers/net/dsa/microchip/ksz9477.c @@ -1211,6 +1211,14 @@ int ksz9477_setup(struct dsa_switch *ds) if (dev->wakeup_source) ksz_write8(dev, REG_SW_PME_CTRL, PME_ENABLE); + if (is_valid_ether_addr(dev->mac_addr)) { + int i; + + for (i = 0; i < ETH_ALEN; i++) + ksz_write8(dev, REG_SW_MAC_ADDR_0 + i, + dev->mac_addr[i]); + } + return 0; } diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c index a7b298838932c..4de2456b5a349 100644 --- a/drivers/net/dsa/microchip/ksz_common.c +++ b/drivers/net/dsa/microchip/ksz_common.c @@ -3684,6 +3684,8 @@ int ksz_switch_register(struct ksz_device *dev) for (port_num = 0; port_num < dev->info->port_cnt; ++port_num) dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA; if (dev->dev->of_node) { + const u8 *mac; + ret = of_get_phy_mode(dev->dev->of_node, &interface); if (ret == 0) dev->compat_interface = interface; @@ -3718,6 +3720,11 @@ int ksz_switch_register(struct ksz_device *dev) dev->wakeup_source = of_property_read_bool(dev->dev->of_node, "wakeup-source"); + + mac = of_get_property(dev->dev->of_node, "local-mac-address", + NULL); + if (mac) + memcpy(dev->mac_addr, mac, ETH_ALEN); } ret = dsa_register_switch(dev->ds); diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h index 589f8b582a415..bdfcd1dc02ee9 100644 --- a/drivers/net/dsa/microchip/ksz_common.h +++ b/drivers/net/dsa/microchip/ksz_common.h @@ -171,6 +171,8 @@ struct ksz_device { struct mutex lock_irq; /* IRQ Access */ struct ksz_irq girq; struct ksz_ptp_data ptp_data; + + u8 mac_addr[ETH_ALEN]; }; /* List of supported models */
The switch MAC address is used for sending pause frames and for Wake on Magic Packet. So, make use of local-mac-address property in the switch node root and configure it in the HW. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- drivers/net/dsa/microchip/ksz9477.c | 8 ++++++++ drivers/net/dsa/microchip/ksz_common.c | 7 +++++++ drivers/net/dsa/microchip/ksz_common.h | 2 ++ 3 files changed, 17 insertions(+)