Messages in this thread Patch in this message | | | Date | Wed, 15 Sep 2021 17:29:57 -0700 | From | Colin Foster <> | Subject | Re: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed |
| |
On Wed, Sep 15, 2021 at 12:25:52PM +0000, Vladimir Oltean wrote: > On Tue, Sep 14, 2021 at 11:21:02PM -0700, Colin Foster wrote: > > Converting the ocelot driver to use phylink, commit e6e12df625f2, uses mac_speed > > in ocelot_phylink_mac_link_up instead of the local variable speed. Stale > > references to the old variable were missed, and so were always performing > > invalid second writes to the DEV_CLOCK_CFG and ANA_PFC_CFG registers. > > > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com> > > --- > > drivers/net/ethernet/mscc/ocelot.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c > > index c581b955efb3..91a31523be8f 100644 > > --- a/drivers/net/ethernet/mscc/ocelot.c > > +++ b/drivers/net/ethernet/mscc/ocelot.c > > @@ -566,11 +566,11 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port, > > /* Take MAC, Port, Phy (intern) and PCS (SGMII/Serdes) clock out of > > * reset > > */ > > - ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(speed), > > + ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed), > > DEV_CLOCK_CFG); > > Oh wow, I don't know how this piece did not get deleted. We write twice > to DEV_CLOCK_CFG, once with a good value and once with a bad value. > Please delete the second write entirely.
It seemed odd to me at first, but I looked into the datasheet and saw that multiple writes to the DEV_CLOCK_CFG register in section 5.2.1 https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf
11. Set up the switch port to the new mode of operation. Keep the reset bits in CLOCK_CFG set. 12. Release the switch port from reset by clearing the reset bits in CLOCK_CFG.
From that it seems like maybe the routine must be two-fold. First a write to set up the link speed with: ocelot_port_rmwl(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed), DEV_CLOCK_CFG_LINK_SPEED_M, DEV_CLK_CFG); ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed), DEV_CLK_CFG);
Of note: I'm currently only using the VSC7512 ports 0-3, which don't have a PCS and therefore don't have the PCS_RATE_ADAPTATION requirement. So all of my ports should be good test candidates for this.
> > > > > /* No PFC */ > > - ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(speed), > > + ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(mac_speed), > > ANA_PFC_PFC_CFG, port); > > Both were supposed to be deleted in fact. > See, if priority flow control is disabled, it does not matter what is > the speed the port is operating at, so the write is useless. > > Also, setting the FC_LINK_SPEED in ANA_PFC_PFC_CFG to mac_speed is not > quite correct for Felix/Seville, even if we were to enable PFC. The > documentation says: > > Configures the link speed. This is used to > evaluate the time specifications in incoming > pause frames. > 0: 2500 Mbps > 1: 1000 Mbps > 2: 100 Mbps > 3: 10 Mbps > > But mac_speed is always 1000 Mbps for Felix/Seville (1), due to > OCELOT_QUIRK_PCS_PERFORMS_RATE_ADAPTATION. If we were to set the correct > speed for the PFC PAUSE quanta, we'd need to introduce yet a third > variable, fc_link_speed, which is set similar to how mac_fc_cfg is, but > using the ANA_PFC_PFC_CFG_FC_LINK_SPEED macro instead of SYS_MAC_FC_CFG_FC_LINK_SPEED. > In other words, DEV_CLOCK_CFG may be fixed at 1000 Mbps, but if the port > operates at 100 Mbps via PCS rate adaptation, the PAUSE quanta values > in the MAC still need to be adapted.
This makes sense. And I'm hopeful once I get around to the rest of the device's functionality (external phy / fiber) this level of understanding will be second nature for me.
> > > > > /* Core: Enable port for frame transfer */ > > -- > > 2.25.1 > > > > So please restructure the patch to delete both assignments (maybe even > create two patches, one for each), and rewrite your commit message and > title to a more canonical format.
I'll make a submission for the PFC with the suggested formats. Thank you for the feedback.
With CLOCK_CFG, there could be the fix for the duplicate write, and a second patch to do the reconfigure while the port is still in reset. Or a single patch that does both, since this behavior seems to have existed in ocelot_adjust_link. > > Example: > "net: mscc: ocelot: remove buggy duplicate write to DEV_CLOCK_CFG") > "net: mscc: ocelot: remove buggy and useless write to ANA_PFC_PFC_CFG") > > Be sure to include this at the end: > Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink") > > So that it catches the stable maintainers' eyes.
Thanks as always for your direction.
| |