The 11/24/2021 10:20, Russell King (Oracle) wrote:
Hi,
Hi Russel,
On Wed, Nov 24, 2021 at 09:39:12AM +0100, Horatiu Vultur wrote:
quoted
+static int lan966x_port_open(struct net_device *dev)
+{
+ struct lan966x_port *port = netdev_priv(dev);
+ struct lan966x *lan966x = port->lan966x;
+ int err;
+
+ if (port->serdes) {
+ err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET,
+ port->config.phy_mode);
+ if (err) {
+ netdev_err(dev, "Could not set mode of SerDes\n");
+ return err;
+ }
+ }
This could be done in the mac_prepare() method.
Yes, I will move this in mac_prepare()
quoted
+static void lan966x_cleanup_ports(struct lan966x *lan966x)
+{
+ struct lan966x_port *port;
+ int portno;
+
+ for (portno = 0; portno < lan966x->num_phys_ports; portno++) {
+ port = lan966x->ports[portno];
+ if (!port)
+ continue;
+
+ if (port->phylink) {
+ rtnl_lock();
+ lan966x_port_stop(port->dev);
+ rtnl_unlock();
+ phylink_destroy(port->phylink);
+ port->phylink = NULL;
+ }
+
+ if (port->fwnode)
+ fwnode_handle_put(port->fwnode);
+
+ if (port->dev)
+ unregister_netdev(port->dev);
This doesn't look like the correct sequence to me. Shouldn't the net
device be unregistered first, which will take the port down by doing
so and make it unavailable to userspace to further manipulate. Then
we should start tearing other stuff down such as destroying phylink
and disabling interrupts (in the caller of this.)
I can change the order as you suggested.
Regarding the interrupts, shouldn't they be first disable and then do
all the teardown?
Don't you need to free the netdev as well at some point?
It is not needed because they are allocated using devm_alloc_etherdev_mqs
quoted
static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
- phy_interface_t phy_mode)
+ phy_interface_t phy_mode,
+ struct fwnode_handle *portnp)
{
...
quoted
+ port->phylink_config.dev = &port->dev->dev;
+ port->phylink_config.type = PHYLINK_NETDEV;
+ port->phylink_config.pcs_poll = true;
+ port->phylink_pcs.poll = true;
You don't need to set both of these - please omit
port->phylink_config.pcs_poll.
I will remove it.
quoted
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index 7a1ff9d19fbf..ce2798db0449 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
...
quoted
@@ -44,15 +58,48 @@ struct lan966x {
void __iomem *regs[NUM_TARGETS];
int shared_queue_sz;
+
+ /* interrupts */
+ int xtr_irq;
+};
+
+struct lan966x_port_config {
+ phy_interface_t portmode;
+ phy_interface_t phy_mode;
What is the difference between "portmode" and "phy_mode"? Does it matter
if port->config.phy_mode get zeroed when lan966x_port_pcs_set() is
called from lan966x_pcs_config()? It looks to me like the first call
will clear phy_mode, setting it to PHY_INTERFACE_MODE_NA from that point
on.
The purpose was to use portmode to configure the MAC and the phy_mode
to configure the serdes. There are small issues regarding this which
will be fix in the next series also I will add some comments just to
make it clear.
Actually, port->config.phy_mode will not get zeroed. Because right after
the memset it follows: 'config = port->config'.
quoted
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
new file mode 100644
index 000000000000..ca1b0c8d1bf5
--- /dev/null
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
...
quoted
+void lan966x_port_status_get(struct lan966x_port *port,
+ struct phylink_link_state *state)
+{
+ struct lan966x *lan966x = port->lan966x;
+ u16 lp_adv, ld_adv;
+ bool link_down;
+ u16 bmsr = 0;
+ u32 val;
+
+ val = lan_rd(lan966x, DEV_PCS1G_STICKY(port->chip_port));
+ link_down = DEV_PCS1G_STICKY_LINK_DOWN_STICKY_GET(val);
+ if (link_down)
+ lan_wr(val, lan966x, DEV_PCS1G_STICKY(port->chip_port));
+
+ /* Get both current Link and Sync status */
+ val = lan_rd(lan966x, DEV_PCS1G_LINK_STATUS(port->chip_port));
+ state->link = DEV_PCS1G_LINK_STATUS_LINK_STATUS_GET(val) &&
+ DEV_PCS1G_LINK_STATUS_SYNC_STATUS_GET(val);
+ state->link &= !link_down;
+
+ if (port->config.portmode == PHY_INTERFACE_MODE_1000BASEX)
+ state->speed = SPEED_1000;
+ else if (port->config.portmode == PHY_INTERFACE_MODE_2500BASEX)
+ state->speed = SPEED_2500;
Why not use state->interface? state->interface will be the currently
configured interface mode (which should be the same as your
port->config.portmode.)
I will use state->interface.
quoted
+
+ state->duplex = DUPLEX_FULL;
Also, what is the purpose of initialising state->speed and state->duplex
here? phylink_mii_c22_pcs_decode_state() will do that for you when
decoding the advertisements.
If it's to deal with autoneg disabled, then it ought to be conditional on
autoneg being disabled and the link being up.
It was the case for autoneg disabled.
quoted
+
+ /* Get PCS ANEG status register */
+ val = lan_rd(lan966x, DEV_PCS1G_ANEG_STATUS(port->chip_port));
+
+ /* Aneg complete provides more information */
+ if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) {
+ lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val);
+ state->an_complete = true;
+
+ bmsr |= state->link ? BMSR_LSTATUS : 0;
+ bmsr |= state->an_complete;
Shouldn't this be setting BMSR_ANEGCOMPLETE?
That was a silly mistake from my side.
quoted
+
+ if (port->config.portmode == PHY_INTERFACE_MODE_SGMII) {
+ phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv);
+ } else {
+ val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port));
+ ld_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val);
+ phylink_mii_c22_pcs_decode_state(state, bmsr, ld_adv);
+ }
This looks like it can be improved:
if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) {
state->an_complete = true;
bmsr |= state->link ? BMSR_LSTATUS : 0;
bmsr |= BMSR_ANEGCOMPLETE;
if (state->interface == PHY_INTERFACE_MODE_SGMII) {
lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val);
} else {
val = lan_rd(lan966x, DEV_PCS1G_ANEG_CFG(port->chip_port));
lp_adv = DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET(val);
}
phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv);
}
I'm not sure that the non-SGMII code is actually correct though. Which
advertisement are you extracting by reading the DEV_PCS1G_ANEG_CFG
register and extracting DEV_PCS1G_ANEG_CFG_ADV_ABILITY_GET ? From the
code in lan966x_port_pcs_set(), it suggests this is our advertisement,
but it's supposed to always be the link partner's advertisement being
passed to phylink_mii_c22_pcs_decode_state().
Actually, like you mentioned it needs to be link partner's advertisement
so that code can be simplified more:
if (DEV_PCS1G_ANEG_STATUS_ANEG_COMPLETE_GET(val)) {
state->an_complete = true;
bmsr |= state->link ? BMSR_LSTATUS : 0;
bmsr |= BMSR_ANEGCOMPLETE;
lp_adv = DEV_PCS1G_ANEG_STATUS_LP_ADV_GET(val);
phylink_mii_c22_pcs_decode_state(state, bmsr, lp_adv);
}
Because inside phylink_mii_c22_pcs_decode_state, more precisely in
phylink_decode_c37_work, state->advertising will have the local
advertising.
quoted
+int lan966x_port_pcs_set(struct lan966x_port *port,
+ struct lan966x_port_config *config)
+{
...
quoted
+ port->config = *config;
As mentioned elsewhere, "config" won't have phy_mode set, so this clears
port->config.phymode to PHY_INTERFACE_MODE_NA, which I think will cause
e.g. lan966x_port_link_up() not to behave as intended.
Actually, the "config" will still have the phy_mode because config will
get teh value of port->config and after that some fields are changed.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
--
/Horatiu