| Message ID | 20201201083408.51006-1-jean.pihet@newoldbits.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] net: dsa: ksz8795: adjust CPU link to host interface | expand |
On Tue, Dec 01, 2020 at 09:34:08AM +0100, Jean Pihet wrote: > Add support for RGMII in 100 and 1000 Mbps. > > Adjust the CPU port settings from the host interface settings: interface > MII type, speed, duplex. Hi Jean You have still not explained why this is needed. Why? is always the important question to answer in the commit message. The What? is obvious from reading the patch. Why does you board need this, when no over board does? Thanks Andrew
Hi Andrew, On Tue, Dec 1, 2020 at 7:41 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Dec 01, 2020 at 09:34:08AM +0100, Jean Pihet wrote: > > Add support for RGMII in 100 and 1000 Mbps. > > > > Adjust the CPU port settings from the host interface settings: interface > > MII type, speed, duplex. > > Hi Jean > > You have still not explained why this is needed. Why? is always the > important question to answer in the commit message. The What? is > obvious from reading the patch. Why does you board need this, when no > over board does? I reworked the commit description about the What and thought it was enough. Do you need a cover letter to describe it more? The Why is: " Configure the host port of the switch to match the host interface settings. This is useful when the switch is directly connected to the host MAC interface. " Thank you for reviewing the patch. BR, Jean > > Thanks > Andrew
Hi Jean, On Tue, Dec 01, 2020 at 07:58:01PM +0100, Jean Pihet wrote: > Hi Andrew, > > On Tue, Dec 1, 2020 at 7:41 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > On Tue, Dec 01, 2020 at 09:34:08AM +0100, Jean Pihet wrote: > > > Add support for RGMII in 100 and 1000 Mbps. > > > > > > Adjust the CPU port settings from the host interface settings: interface > > > MII type, speed, duplex. > > > > Hi Jean > > > > You have still not explained why this is needed. Why? is always the > > important question to answer in the commit message. The What? is > > obvious from reading the patch. Why does you board need this, when no > > over board does? > > I reworked the commit description about the What and thought it was > enough. Do you need a cover letter to describe it more? > > The Why is: > " > Configure the host port of the switch to match the host interface > settings. This is useful when the switch is directly connected to the > host MAC interface. > " > Thank you for reviewing the patch. First of all, I am not clear if you want the patch merged or not. If you do, then I don't understand why you did not use the ./scripts/get_maintainer.pl tool to get the email addresses of the people who can help you with that. No one from Microchip, not the DSA maintainers, not the networking maintainer. Secondly, don't you get an annoying warning that you should not use .adjust_link and should migrate to .phylink_mac_link_up? Why do you ignore it? Did you even see it? Thirdly, your patch is opaque and has three changes folded into one. You refactor some code from ksz8795_port_setup into a separate function, you add logic for the speeds of 100 and 10 for RGMII, and you call this function from .adjust_link. You must justify why you need all of this, and cannot just add 3 lines to ksz8795_port_setup. You must explain that the ksz8795_port_setup function does not use information from device tree. Then you must explain why the patch is correct. The code refactored out of ksz8795_port_setup, plus the changes you've added to it, looks now super weird. Half of ksz8795_mii_config treats p->phydev.speed as an output variable, and half of it as an input variable. To the untrained eye this looks like a hack. I'm sure you can clarify. This is what Andrew wants to see. Fourth, seriously now, could you just copy Microchip people to your patches? The phylink conversion was done this summer, I'm sure they can help with some suggestions.
> Configure the host port of the switch to match the host interface > settings. This is useful when the switch is directly connected to the > host MAC interface. Why do you need this when no other board does? Why is your board special? As i said before, i'm guessing your board has back to back PHYs between the SoC and the switch and nobody else does. Is that the reason why? Without this, nothing is configuring the switch MAC to the results of the auto-neg between the two PHYs? Or am i completely wrong? Andrew
Hi Vladimir, On Tue, Dec 1, 2020 at 8:48 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > Hi Jean, > > On Tue, Dec 01, 2020 at 07:58:01PM +0100, Jean Pihet wrote: > > Hi Andrew, > > > > On Tue, Dec 1, 2020 at 7:41 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > > On Tue, Dec 01, 2020 at 09:34:08AM +0100, Jean Pihet wrote: > > > > Add support for RGMII in 100 and 1000 Mbps. > > > > > > > > Adjust the CPU port settings from the host interface settings: interface > > > > MII type, speed, duplex. > > > > > > Hi Jean > > > > > > You have still not explained why this is needed. Why? is always the > > > important question to answer in the commit message. The What? is > > > obvious from reading the patch. Why does you board need this, when no > > > over board does? > > > > I reworked the commit description about the What and thought it was > > enough. Do you need a cover letter to describe it more? > > > > The Why is: > > " > > Configure the host port of the switch to match the host interface > > settings. This is useful when the switch is directly connected to the > > host MAC interface. > > " > > Thank you for reviewing the patch. > > First of all, I am not clear if you want the patch merged or not. If you > do, then I don't understand why you did not use the ./scripts/get_maintainer.pl > tool to get the email addresses of the people who can help you with > that. No one from Microchip, not the DSA maintainers, not the networking > maintainer. My bad, I thought that sending to both LKML and netdev was enough. > > Secondly, don't you get an annoying warning that you should not use > .adjust_link and should migrate to .phylink_mac_link_up? Why do you > ignore it? Did you even see it? No there is no warning using my arm config, both with linux and netdev kernels. > > Thirdly, your patch is opaque and has three changes folded into one. You > refactor some code from ksz8795_port_setup into a separate function, you > add logic for the speeds of 100 and 10 for RGMII, and you call this > function from .adjust_link. You must justify why you need all of this, > and cannot just add 3 lines to ksz8795_port_setup. You must explain that > the ksz8795_port_setup function does not use information from device > tree. Then you must explain why the patch is correct. > The code refactored out of ksz8795_port_setup, plus the changes you've > added to it, looks now super weird. Half of ksz8795_mii_config treats > p->phydev.speed as an output variable, and half of it as an input > variable. To the untrained eye this looks like a hack. I'm sure you can > clarify. This is what Andrew wants to see. Ok taking notes here, thanks for the valuable input. > > Fourth, seriously now, could you just copy Microchip people to your > patches? The phylink conversion was done this summer, I'm sure they can > help with some suggestions. Ok will do, and check the phylink conversion code. Thank you for reviewing. BR, Jean
Hi Andrew, On Tue, Dec 1, 2020 at 9:45 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Configure the host port of the switch to match the host interface > > settings. This is useful when the switch is directly connected to the > > host MAC interface. > > Why do you need this when no other board does? Why is your board > special? > > As i said before, i'm guessing your board has back to back PHYs > between the SoC and the switch and nobody else does. Is that the > reason why? Without this, nothing is configuring the switch MAC to the > results of the auto-neg between the two PHYs? Yes that is the case. From here I see this patch is too specific to our setup, and so cannot be considered for merging. > > Or am i completely wrong? No, this is completely right. I will drop this patch then. Thank you very much for reviewing and for the suggestions. BR, Jean > > Andrew >
On Mon, Dec 07, 2020 at 08:43:32PM +0100, Jean Pihet wrote: > Hi Andrew, > > On Tue, Dec 1, 2020 at 9:45 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > Configure the host port of the switch to match the host interface > > > settings. This is useful when the switch is directly connected to the > > > host MAC interface. > > > > Why do you need this when no other board does? Why is your board > > special? > > > > As i said before, i'm guessing your board has back to back PHYs > > between the SoC and the switch and nobody else does. Is that the > > reason why? Without this, nothing is configuring the switch MAC to the > > results of the auto-neg between the two PHYs? > > Yes that is the case. From here I see this patch is too specific to > our setup, and so cannot be considered for merging. Hi Jean I never said i was too specific to your board. There are other boards using different switches like this. This is where the commit message is so important. Without understanding Why? it is hard to point you in the right direction. So you setup is: SoC - MAC - PHY - PHY - MAC - Switch. The SoC MAC driver is looking after the first PHY? This patch is about the Switch MAC and PHY. You need the results of auto-neg from the PHY to be programmed into the MAC of the switch. If i remember correctly you just need a phy-handle in the CPU node, pointing at the PHY. See for example imx6q-b450v3.dts Andrew
> > Hi Jean > > > > I never said i was too specific to your board. There are other boards > > using different switches like this. This is where the commit message > > is so important. Without understanding Why? it is hard to point you in > > the right direction. > > > > So you setup is: > > > > SoC - MAC - PHY - PHY - MAC - Switch. > > > > The SoC MAC driver is looking after the first PHY? > > No, the connection is at the MAC level, via RGMII but it is missing the MDC/ > MDIO signals. That means we have to fix the auto-neg parameters from the DT. So the PHY is there, but you cannot talk to it? It has strapping resisters to make it auto-neg to the other PHY? Some switches default their CPU port to the maximum speed the port can do. But not all do. It is worth checking that. > On the 4.14 LTS kernel we are working with, the setup of the parameters is done > via adjust_link. Since the phylink conversion adjust_link is not required > anymore, is this correct? 4.14 is dead in terms of development. Anything you contribute needs to be for net-next, and then you need to figure out how to backport it. Using v5.4 will help with that, since it is much closer, and v5.10 will be LTS. Given the change to phylink, you probably want as new a kernel as possible. If you put a fixed-link property in the "CPU" node, phylink should do the right thing for you. Andrew
diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c index 1e101ab56cea..e76390bba3a0 100644 --- a/drivers/net/dsa/microchip/ksz8795.c +++ b/drivers/net/dsa/microchip/ksz8795.c @@ -916,10 +916,53 @@ static void ksz8795_port_mirror_del(struct dsa_switch *ds, int port, PORT_MIRROR_SNIFFER, false); } +static void ksz8795_mii_config(struct ksz_device *dev, struct ksz_port *p) +{ + u8 data8; + + /* Configure MII interface for proper network communication. */ + ksz_read8(dev, REG_PORT_5_CTRL_6, &data8); + data8 &= ~PORT_INTERFACE_TYPE; + data8 &= ~PORT_GMII_1GPS_MODE; + switch (p->interface) { + case PHY_INTERFACE_MODE_MII: + p->phydev.speed = SPEED_100; + break; + case PHY_INTERFACE_MODE_RMII: + data8 |= PORT_INTERFACE_RMII; + p->phydev.speed = SPEED_100; + break; + case PHY_INTERFACE_MODE_GMII: + data8 |= PORT_GMII_1GPS_MODE; + data8 |= PORT_INTERFACE_GMII; + p->phydev.speed = SPEED_1000; + break; + default: + data8 &= ~PORT_RGMII_ID_IN_ENABLE; + data8 &= ~PORT_RGMII_ID_OUT_ENABLE; + if (p->interface == PHY_INTERFACE_MODE_RGMII_ID || + p->interface == PHY_INTERFACE_MODE_RGMII_RXID) + data8 |= PORT_RGMII_ID_IN_ENABLE; + if (p->interface == PHY_INTERFACE_MODE_RGMII_ID || + p->interface == PHY_INTERFACE_MODE_RGMII_TXID) + data8 |= PORT_RGMII_ID_OUT_ENABLE; + /* Support RGMII in 100 and 1000 Mbps */ + if (p->phydev.speed == SPEED_1000) { + data8 |= PORT_GMII_1GPS_MODE; + } else { + p->phydev.speed = SPEED_100; + } + data8 |= PORT_INTERFACE_RGMII; + break; + } + ksz_write8(dev, REG_PORT_5_CTRL_6, data8); + p->phydev.duplex = 1; +} + static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port) { struct ksz_port *p = &dev->ports[port]; - u8 data8, member; + u8 member; /* enable broadcast storm limit */ ksz_port_cfg(dev, port, P_BCAST_STORM_CTRL, PORT_BROADCAST_STORM, true); @@ -943,41 +986,7 @@ static void ksz8795_port_setup(struct ksz_device *dev, int port, bool cpu_port) port); p->interface = dev->compat_interface; } - - /* Configure MII interface for proper network communication. */ - ksz_read8(dev, REG_PORT_5_CTRL_6, &data8); - data8 &= ~PORT_INTERFACE_TYPE; - data8 &= ~PORT_GMII_1GPS_MODE; - switch (p->interface) { - case PHY_INTERFACE_MODE_MII: - p->phydev.speed = SPEED_100; - break; - case PHY_INTERFACE_MODE_RMII: - data8 |= PORT_INTERFACE_RMII; - p->phydev.speed = SPEED_100; - break; - case PHY_INTERFACE_MODE_GMII: - data8 |= PORT_GMII_1GPS_MODE; - data8 |= PORT_INTERFACE_GMII; - p->phydev.speed = SPEED_1000; - break; - default: - data8 &= ~PORT_RGMII_ID_IN_ENABLE; - data8 &= ~PORT_RGMII_ID_OUT_ENABLE; - if (p->interface == PHY_INTERFACE_MODE_RGMII_ID || - p->interface == PHY_INTERFACE_MODE_RGMII_RXID) - data8 |= PORT_RGMII_ID_IN_ENABLE; - if (p->interface == PHY_INTERFACE_MODE_RGMII_ID || - p->interface == PHY_INTERFACE_MODE_RGMII_TXID) - data8 |= PORT_RGMII_ID_OUT_ENABLE; - data8 |= PORT_GMII_1GPS_MODE; - data8 |= PORT_INTERFACE_RGMII; - p->phydev.speed = SPEED_1000; - break; - } - ksz_write8(dev, REG_PORT_5_CTRL_6, data8); - p->phydev.duplex = 1; - + ksz8795_mii_config(dev, p); member = dev->port_mask; } else { member = dev->host_mask | p->vid_member; @@ -1102,11 +1111,23 @@ static int ksz8795_setup(struct dsa_switch *ds) return 0; } +static void ksz8795_adjust_link(struct dsa_switch *ds, int port, + struct phy_device *phydev) +{ + struct ksz_device *dev = ds->priv; + struct ksz_port *p = &dev->ports[port]; + + /* Adjust the link interface mode and speed for the CPU port */ + if (dsa_is_cpu_port(ds, port)) + ksz8795_mii_config(dev, p); +} + static const struct dsa_switch_ops ksz8795_switch_ops = { .get_tag_protocol = ksz8795_get_tag_protocol, .setup = ksz8795_setup, .phy_read = ksz_phy_read16, .phy_write = ksz_phy_write16, + .adjust_link = ksz8795_adjust_link, .phylink_mac_link_down = ksz_mac_link_down, .port_enable = ksz_enable_port, .get_strings = ksz8795_get_strings,
Add support for RGMII in 100 and 1000 Mbps. Adjust the CPU port settings from the host interface settings: interface MII type, speed, duplex. Signed-off-by: Jean Pihet <jean.pihet@newoldbits.com> --- drivers/net/dsa/microchip/ksz8795.c | 93 ++++++++++++++++++----------- 1 file changed, 57 insertions(+), 36 deletions(-)