Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 15 Sep 2021 17:29:57 -0700
From:   Colin Foster <colin.foster@in-advantage.com>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     Claudiu Manoil <claudiu.manoil@nxp.com>,
        Alexandre Belloni <alexandre.belloni@bootlin.com>,
        "UNGLinuxDriver@microchip.com" <UNGLinuxDriver@microchip.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Russell King <linux@armlinux.org.uk>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing
 MAC speed
Message-ID: <20210916002957.GA513411@euler>
References: <20210915062103.149287-1-colin.foster@in-advantage.com>
 <20210915122551.kol3f5jz4634nvrm@skbuf>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210915122551.kol3f5jz4634nvrm@skbuf>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?sScwZlb2O8J52ijUXtZP2y9zu8riOsN6pUKJXDL2GXDqz5CLiMoD0WAr3PCp?=
 =?us-ascii?Q?/a8YUM+xJ+7CB/vaSeg2YBXAicCdu6XUduC7cRS7m7kixL1aKyIJyRpd1WKO?=
 =?us-ascii?Q?G/9x2g2o+jqGjJIiFxCso2cmu8yxWJrhvve+4V+0qnmoEbqqMi7FlpoOBUZW?=
 =?us-ascii?Q?zqxjoaQCfxQVcNHjQq3tqRdyrRKhTnhWdprJ0bumimviPwODPASbuSlODZ4B?=
 =?us-ascii?Q?V2pDpFtdNh+n1jyqcpWkisj7lrADcy0wQPKzBAjG+i9P8hqDXpluFIYfWQwU?=
 =?us-ascii?Q?dqIYiZwiNkAgsx6pbuaCGNBZ+fDJYDcZ63wRj6bEr4nXgTJCcmsyfd3YJ/J5?=
 =?us-ascii?Q?5Hi1g7oKs69aab//AX1Xz6zcfdASv1NhoMWEY6D8OczYUli09Qrt1tGbDICH?=
 =?us-ascii?Q?/tWok/gDUWgq2VH3jnKMhGTUfXGaAWwR6p4lvWlG98xRS0Kd8l1CbPsY2VUF?=
 =?us-ascii?Q?Xow2BTRgUrmJidSSVrJSzTxes3jBdpfGtJOoTxuxW/XFv2p5YTY/USPeNNAx?=
 =?us-ascii?Q?zUyDCX2QfSXRFNjOynxqKONdf3f9wJMcwS/HCEa13bA9oKGvDsD16FeZYjxT?=
 =?us-ascii?Q?z8HN/vPrdH5SwtsIwD8i+q4mS7MkPxbJDp1NCfEI7TA4QKwomJM3T5kEN9k1?=
 =?us-ascii?Q?AcHzxCItrZ5MTo9ROU1sqMtXbK29kpfjhaBiwqSEIhKtJTNJ4tEYLKx0xXVn?=
 =?us-ascii?Q?csvaS2D9agWpX1geu6JGV/2DwfL6HggKMnqzcEk0b1eqkEdjE2DLpdSj1ObD?=
 =?us-ascii?Q?8305fe0xbmIX6wFDHdyYV99POg1tXUl99C9G+i5vCgPvcw2Es1ujJtfygirl?=
 =?us-ascii?Q?sks4CM4P9nchUqtKBRXOAG0UZOhftrvYoKgzI5SJvty4MzpIo5dQPvILoleu?=
 =?us-ascii?Q?RKZM0Z+k0feDxVqxY5iIN40+Qb7nJK4pkeYEKXd+EY/LmUCjj26/WRNKEsL4?=
 =?us-ascii?Q?KC4f8cc7un2UsmxkhBJX89eR5ZeiWNJknpnqt8Hjt/ineqQhOfxsRjZQoIM+?=
 =?us-ascii?Q?hhR7jW49wfLbyQRDg4r1Wed2XGau0NJBKv4D65anATiMAQ9QRtfX54jVQhHL?=
 =?us-ascii?Q?Qjl6jqSvbwLKHlDSNHOzblzOKxJXlAKLGLB0ZHPENBSx1gpaOUziRiK7vQrF?=
 =?us-ascii?Q?xQ+6+8iMbHu7+hdMK+2/OWto4CfPxJHHgsIzUk/Vyi9wUIOCY2M3ewyWWUEV?=
 =?us-ascii?Q?1R1KoQCmy/mKm7sXIv5yspJybrSjwxltaIiE0jTUhPVqxcgglrNXt+RHZH/M?=
 =?us-ascii?Q?dYPFKlv+XZJDyyToNWcltQ1F9ONr1iRpPh5R7Q47H+YQgaHpEurj/ydqsxP3?=
 =?us-ascii?Q?aWrU64xwkH7v1ucjhh2abHeK?=
X-OriginatorOrg: in-advantage.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 208b0cff-e5ec-47e9-96ce-08d978a91fdd
X-MS-Exchange-CrossTenant-AuthSource: DM5PR1001MB2345.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Sep 2021 00:30:03.3335
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 48e842ca-fbd8-4633-a79d-0c955a7d3aae
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: l2cjzTGe3h7Dj8+Fwo213Lu0vY6071xFghu+M4YevxbjYwmxpkBQXlXMokJaDPtH9MGt4hPbs+FH5QjRiq/fzxiG4f7l8txhAegNfPIgNkQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR10MB3146
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.