From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtpout-03.galae.net (smtpout-03.galae.net [185.246.85.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 5A1422D23A4 for ; Mon, 8 Jun 2026 14:23:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=185.246.85.4 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780928624; cv=none; b=amcoXISjXI0NFU85wOTIcDOowA3/UAxXFGI5Tq10kA0rrlfwvLog1BXzk3pSLfNuPV8mwSZv9jdK7252HAErwRriLVvLPCu3nT8NtTCWoqWoDi3AxD6KbE4bbDcikNfMjbpmKvjqs+QRWVCEr+6UIcYYK4rej72SltHiMB43t7I= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780928624; c=relaxed/simple; bh=xG/h0yiLY7kKhERjflmJpWKl2eHPNJz95wXZRDGbjaw=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=MZRPvtV5iSPhxqSOUm6gHXUkr+G3LMwDipPxlDn0H+bGtlEKXJ2ZOZWv143WWekJU/lqTVdsOxEPIg9DAqJ95VwA07JpMQQIZOX02b+9+R6OA2PhTTc/fo+wkWvs0soXz6DBDP/TN9i770yjtCA0M/TEoRynNOgDiiIL5NFJ7jA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com; spf=pass smtp.mailfrom=bootlin.com; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b=DSXLaZBB; arc=none smtp.client-ip=185.246.85.4 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=bootlin.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=bootlin.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=bootlin.com header.i=@bootlin.com header.b="DSXLaZBB" Received: from smtpout-01.galae.net (smtpout-01.galae.net [212.83.139.233]) by smtpout-03.galae.net (Postfix) with ESMTPS id 015174E4164C; Mon, 8 Jun 2026 14:23:41 +0000 (UTC) Received: from mail.galae.net (mail.galae.net [212.83.136.155]) by smtpout-01.galae.net (Postfix) with ESMTPS id C10F25FFB7; Mon, 8 Jun 2026 14:23:40 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) by localhost (Mailerdaemon) with ESMTPSA id D55C1106A295D; Mon, 8 Jun 2026 16:23:37 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=dkim; t=1780928620; h=from:subject:date:message-id:to:cc:mime-version:content-type: content-transfer-encoding:content-language:in-reply-to:references; bh=4aYIYlMAWcvFfdd8owfFa2HC1tQ1UBlKgz/uYgabcE4=; b=DSXLaZBB0Yo4RlfGRuHgxJMH1r9sqnayn4OhC+Wu43oCDXKdbo8Me7RRMpEzmmjGvW12N4 12gKHoLnN0glEg815oyTv23BUj3JZm91KLg9EDB4/hLyigNT9ERNu10m6OfCu7pUVrWOuY Cgs9AmW8cfdjo0r1VdKq7dhXPj7k9mb3XzxZtqD8Yca4dWybWLEwnl3AZIVFLGRMgbnHgm Rp2i2HWlcITW8QHhUL9dvNpNKMcR14eBCybY4USeNHq0RSJuchzQC/k3TTaxo6VuAOY8Y5 Lgvc9UkAXnDHgrWBnH7bkjY/V6RcIsee2v/a3JUQvrE7WgIQHRQzaAZ+WqBwDw== Message-ID: Date: Mon, 8 Jun 2026 16:23:36 +0200 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH net-next v1 2/6] r8169: add support for phylink To: Javen , "hkallweit1@gmail.com" , "nic_swsd@realtek.com" , "andrew+netdev@lunn.ch" , "davem@davemloft.net" , "edumazet@google.com" , "kuba@kernel.org" , "pabeni@redhat.com" , "horms@kernel.org" Cc: "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" References: <20260605103906.1445-1-javen_xu@realsil.com.cn> <20260605103906.1445-3-javen_xu@realsil.com.cn> <75cee79b-78d5-4a64-bbfe-8ba89f4cfb7d@bootlin.com> Content-Language: en-US From: Maxime Chevallier In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Last-TLS-Session-Version: TLSv1.3 On 6/8/26 09:40, Javen wrote: >> >> Hi, >> >> On 6/5/26 12:39, javen wrote: >>> From: Javen Xu >>> >>> Transfer old framework to phylink. Phylink can support fiber mode card >>> which can not get link status or link speed from standard phy registers. >> >> This is a good start, but you need to go deeper than that. Looking at the end >> result, you still access tp->phydev a lot in this driver : >> >> Looking at r8169_mdio_register() for example : >> >> -> don't configure the PHY eee support with phy_support_eee() and >> phy_disable_eee_mode(), let phylink to that for you >> >> All over the driver, there's still a lot of manual control of the PHY with phylib, >> look at the calls for phy_init_hw(), phy_resume(), and so on, you need to >> assume that with phylink you may not have a PHY. >> >> The problem then is that there are places in the code where PHY registers are >> directly accessed : >> >> static void rtl8169_init_phy(struct rtl8169_private *tp) { [...] >> >> if (tp->mac_version == RTL_GIGA_MAC_VER_05 && >> tp->pci_dev->subsystem_vendor == PCI_VENDOR_ID_GIGABYTE && >> tp->pci_dev->subsystem_device == 0xe000) >> phy_write_paged(tp->phydev, 0x0001, 0x10, 0xf01b); [...] >> genphy_soft_reset(tp->phydev); >> } >> >> In the end, you shouldn't even need to use tp->phydev at all. >> >> It's hard to know what this is all about, but it seems like something a PHY >> driver should have to do, not a MAC driver :( >> >> Also, I'd merge the next commit with this one to have one single commit doing >> the phylink conversion. >> >> Maxime >> > Hi Maxime, > > I have one question about the existing link-change interrupt model. > > For most r8169 chips, the PHY exists and the current driver uses the MAC LinkChg interrupt to notify phylib: > if (status & LinkChg) > phy_mac_interrupt(tp->phydev); This is the first time there's an attempt to port a driver that typically deals with an embedded PHY to phylink, so we need to sort this out. The main issue is that this assumes the phy device is an integrated PHY. But what if the PHY isn't integrated and can deal itself with the interrupt ? or there's just no PHY ? I think we could imagine adding a phylink helper that would call phy_mac_interrupt(pl->phydev), however we need to make sure that this is only done when the MAC is handling the interrupt on behalf of the integrated PHY, and not any PHY. > > After converting the driver to phylink, my understanding is that the MAC driver should avoid keeping a private tp->phydev pointer. The PHY can still be found during setup and passed to phylink_connect_phy(), but then I am not sure how the MAC-routed PHY interrupt should be handled. > > Would switching the internal PHY to PHY_POLL be acceptable , or would you prefer to keep the existing interrupt-driven behaviour? If the latter, what would be the preferred way to notify phylib without storing tp->phydev in the driver? I think we need to address this instead of falling to POLL :) > > I also have a related question about the existing firmware handling and sw_cnt_1ms_ini logic. Some of this code touches MAC MMIO registers, so it does not seem suitable to move it entirely into the Realtek PHY driver. The current code uses tp->phydev in this area. I'll continue looking at this deeper in the upcoming days :) Maxime > > What would be the preferred phylink-compatible way to handle this? Should the driver still keep the tp->phydev, or is there another recommended approach? > > Link: https://lore.kernel.org/netdev/b54b5f3a6703498fbc17e64548550ba0@realsil.com.cn/ > > Thanks, > BRs, > Javen > >