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;
From:   Vladimir Oltean <vladimir.oltean@nxp.com>
To:     Colin Foster <colin.foster@in-advantage.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
Thread-Topic: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing
 MAC speed
Thread-Index: AQHXqfnm86w4MNSgi02/MgrVZkbyjqulBbCAgADKT4CAALpvgA==
Date:   Thu, 16 Sep 2021 11:37:14 +0000
Message-ID: <20210916113713.eaquhezzoxheqkxg@skbuf>
References: <20210915062103.149287-1-colin.foster@in-advantage.com>
 <20210915122551.kol3f5jz4634nvrm@skbuf> <20210916002957.GA513411@euler>
In-Reply-To: <20210916002957.GA513411@euler>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?G8ADj0dDUNpuNAp4fv9+ze+mKm9WEuFFku0BTzOwoyWryq/sSXVGhmv6qkH3?=
 =?us-ascii?Q?1ZIOB2Mlfi9BpPuS1hC7PWwJvHUMuBM+vcZcFU6AfEG7+WQtWSMfgo1zivtq?=
 =?us-ascii?Q?8JfzVQDV4XAHRqYW9FvdH40/Od0U89KSmrx5hVIpDZiC9J++rs3tr3kE/NwL?=
 =?us-ascii?Q?R1QC9Y2ipCMs0wqHf4/N3LEjcpFyl3PMVI6VAohG1WnZwJzw+2zqwo/yBt6r?=
 =?us-ascii?Q?zR/iKGiq5Xxp1oc6aYFP2N7jjt1CQkPiKb8+Wfn0XMmqdAA9VHpX9aQ051zv?=
 =?us-ascii?Q?C3N/wsY/Yj0rI9JRcBztJuRGCYIsfYRH5T9JKG0Stlb4Fa1EPMkr/pn+TNNm?=
 =?us-ascii?Q?8p1pIPLD/G1L8PmDmYCv/d17gOdBkXvSR1LWxjhHgOe6DS1LrZfadgZfQEow?=
 =?us-ascii?Q?Xy+BZ2cLinZokz3ELBLNtYNFnV5yEYPrY4CNU0XjTASVbu8t9lHcfZgGQuaI?=
 =?us-ascii?Q?e3u+a1bMeC+ikYseu63xXIRQhMuan4FRPPvIp6vWKe9hoTZcFFQOIsQmcX3r?=
 =?us-ascii?Q?DspzElLPKxCJNU63HACV3DDIoOLTrO2qFcVWDprTE4XgwahL4VyFRKFcMDEV?=
 =?us-ascii?Q?b+hqqP8BDzP2Tn6hI+RK4eqgqkTCYFDlut0cjBW/P8OSuU4FrZEuwIpPzYBY?=
 =?us-ascii?Q?vkg3MAGQJD43bWfwk596vjkK9wB08IAltEjvOPFWrdB/kignx4u6p5maBKcm?=
 =?us-ascii?Q?dhJYIUe8OSOSEgFIJyyXFr5fIiivlfhy7JTcZiHSP7mEZbS+VOtVEasKQ2HQ?=
 =?us-ascii?Q?x/eKGbWSlrblEIW7iNs/mE4wvbRdsHOne6gSMWKt8ZHEjq4vwXutC49eGcuW?=
 =?us-ascii?Q?wldNBZiyBL6mXzJo8qkMb8voNeLrrPSKRywMq6TigY69Wcj0sFdSf9iv2Hi0?=
 =?us-ascii?Q?aA51yBGtLvkN+/+6zhWh6V7ErrMwK9l7/040JXsfgMFZgIXH4r1V7THpi1Nw?=
 =?us-ascii?Q?jzzmlEwNt7YyuUYJnIc2JGGgJeGVBxYZsxegZDvqIZRsXAqMNmLCUb8yGPK8?=
 =?us-ascii?Q?eqYPOxJKVzgHmXRyJLEcZ1xrYY5UbHXu5xGvOjOQwUE5ovXK7/pevoQVX1NY?=
 =?us-ascii?Q?P+orsdcxkFJDpYf8W+2+W+Obe71iAS99EsjY1B6zkbQMwdec3oNPPL2IsbGQ?=
 =?us-ascii?Q?3DuS4OGe0ylpGB90Xc6YzjQ64ZEDyTMxo9x580gwZhngA1jdrxsD1l2vZ5iT?=
 =?us-ascii?Q?AZK9NDnTjfZ8z6SmH4A+tgbcvoAiSGqb8gIzZphj/7/3R7UqPQOJT4QDDf2u?=
 =?us-ascii?Q?HhvM5CEv20sOLSBvWRSurU87a0KkrQ0Td6Zsb/w4kxn/WGWaTLK0wtL9EdLv?=
 =?us-ascii?Q?ymdjEwpKTvDpTN0lLPg0fnnN?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <07B895F9A57C3243BFF429A8F4EE019A@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: VI1PR04MB5136.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 80810fa5-4acd-4559-288a-08d979065467
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Sep 2021 11:37:14.1287
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: SMgVcNKNqSxzYQexUnj0WgSORjT7KkVRDqNxlPmuLDPQhUHnQpvKO6jXQ1K09MZLLzw/WCiFmBx5h8aGDzK7wg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5856
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wed, Sep 15, 2021 at 05:29:57PM -0700, Colin Foster wrote:
> 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, use=
s mac_speed
> > > in ocelot_phylink_mac_link_up instead of the local variable speed. St=
ale
> > > references to the old variable were missed, and so were always perfor=
ming
> > > invalid second writes to the DEV_CLOCK_CFG and ANA_PFC_CFG registers.
> > >=20
> > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > ---
> > >  drivers/net/ethernet/mscc/ocelot.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >=20
> > > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/etherne=
t/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);
> >=20
> > 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.
>=20
> 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
>=20
> 11. Set up the switch port to the new mode of operation. Keep the reset b=
its in CLOCK_CFG set.
> 12. Release the switch port from reset by clearing the reset bits in CLOC=
K_CFG.
>=20
> 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),=20
>                  DEV_CLOCK_CFG_LINK_SPEED_M, DEV_CLK_CFG);
> ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed),
>                    DEV_CLK_CFG);

Yes, that cycle is supposed to be completed by ocelot_phylink_mac_link_down
which does put the port in reset, and ocelot_phylink_mac_link_up puts it
back out of it. The third write to DEV_CLOCK_CFG is simply nonsensical.

> 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.

Observation: as long as Alex's original code is correct, then the
PCS_RATE_ADAPTATION quirk is only needed for an _NXP_ PCS. The VSC7514
family uses a Microchip PCS1G block and the code unconditionally changes
the DEV_CLOCK_CFG_LINK_SPEED, so if this is correct, it means that
different PCS blocks have different requirements.=