Date:   Sat, 11 Dec 2021 15:57:54 +0100
From:   Francesco Dolcini <francesco.dolcini@toradex.com>
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     Francesco Dolcini <francesco.dolcini@toradex.com>,
        philippe.schenker@toradex.com, andrew@lunn.ch,
        qiangqing.zhang@nxp.com, davem@davemloft.net, festevam@gmail.com,
        kuba@kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org
Subject: Re: [PATCH net-next] net: phy: perform a PHY reset on resume
Message-ID: <20211211145754.GA360685@francesco-nb.int.toradex.com>
References: <7a4830b495e5e819a2b2b39dd01785aa3eba4ce7.camel@toradex.com>
 <20211211130146.357794-1-francesco.dolcini@toradex.com>
 <YbSymkxlslW2DqLW@shell.armlinux.org.uk>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YbSymkxlslW2DqLW@shell.armlinux.org.uk>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?8Nb7E7qGto76cRO/6A8tk+eE8gBpXe2w5JrGBFEqaZyppbXndq0Hn065MQ1M?=
 =?us-ascii?Q?gOCZrJXXdbR8FgSuTgKWesKfC4bMgronWQi8+hT74t7PlovYdUb9FDFAQQrh?=
 =?us-ascii?Q?JQ7gkPT7VSudwTHbCQsEgtZ/VL7iBN1ih3G2vVdtzIwW9eL3p3Uyr/3s+3fW?=
 =?us-ascii?Q?Xk4/m5EsP9qEhTj+Nv8nRhOOZ1vA004XU6L5jfpFPApjxAxw5XQN62gxqKfp?=
 =?us-ascii?Q?5YnjA8zgf9Wg8MyOBgcoA8fQx/TXTR9Lshrhqykq/FYefjqp9tR8Z/uShVFD?=
 =?us-ascii?Q?oLBzqGqvT5I7e+gWc6ZBGbp3/WhnLmmZ82zxLirkCrX+jjho/ja2awBLcYdo?=
 =?us-ascii?Q?Jr5Rnvr7bHNmH+h5V3iKXoAGrY2vcA2orv6eUdvMpTQsdSKKJEQNoSV0b95o?=
 =?us-ascii?Q?OLHNc/Pa6MlUGsHoC67uO1zKRxsJP1vntkmdAx9I5US9oN84b1f0VdXdOx4d?=
 =?us-ascii?Q?xHSubHzF1lKcvBmu7NSYkMkZisOqtSmQw3llfvAFHn8IJd8JSXHk2CBnRjON?=
 =?us-ascii?Q?6AJ6XsyMs8oTsOaBeFnEm9UdcLTHupxHqAGyK3mr/9J5G/Mw4koUDd7D6/ee?=
 =?us-ascii?Q?kUmTrsKUWBhfeb52ayPStgCmux+UGjVv7cY+G3ljX/YW6yRVovQAFUCtrBrk?=
 =?us-ascii?Q?dnYkgL39LGQGgf6NktRJASZAprdbb4uQqWQPY+fyo0j0nBZI/EgrJckplCcx?=
 =?us-ascii?Q?HR/9aWMmGYVqrt86/r8ffgc8TzlNETzK8hdT3Ia3e69/R09ursnFZi0JUpQn?=
 =?us-ascii?Q?kqGaunLUVuHyOdFm+Jc/XoqvghJ4JTzWzAoxmH7TJ0pmd/1MJkMCIODvGD7n?=
 =?us-ascii?Q?F1lGqFCXtWL0NgKTcXnCY+Tyl8Ujj31OAH8cKIGqnwEJhRR8rUXK5lttI3YV?=
 =?us-ascii?Q?gHTO4pFIw2HxZnN1qyWSqV2OYuNU84/Te2InoRL8VntbICVwAy/EQQ34hqhh?=
 =?us-ascii?Q?D8sixxS2I9T49tYnpLKN+BCn47r1+W56Ps0CxCdW2rdzp1/vNYljgqz+zDjg?=
 =?us-ascii?Q?ic5JjD20hlSoM3k5Ptuxc3ac6l0Zo7fwOVTlK0KCpv5pUxZDoJA0EkhO44TP?=
 =?us-ascii?Q?uXmqzteQbNsYSBDM++bPWsK45H+RO61Bb/xloG/yhcsziOZ0tEB9e11FxuXn?=
 =?us-ascii?Q?T/FORNTcgk1y+zvyujhxHRRhF+wPCYPuhk+lgpjzm+VSEXW77ycJCn/0ZDvK?=
 =?us-ascii?Q?j9IOZn/nSX56aC20vjFbJBeHxP2X3yMmGKRI9Sqjg5cqsM57VR2u5fDFhZgR?=
 =?us-ascii?Q?K9jNlwVrZCMXlkizJbVIw63NNVZGRd9GeWdUQFkoBKs+FeSWLyOrTrjd3Ql9?=
 =?us-ascii?Q?/F3EmWFKm6i8lHwwpxWTJ9KpdLCLh+0upmXVOj3qVSv1GtyFyNkPTmqIUFNg?=
 =?us-ascii?Q?N2jMK/IrDTSwuuWmcQoBw2g57U+Tiiyo10KgdLnubJUkK+eouYU5ukj8IRV8?=
 =?us-ascii?Q?yXhwYCW861TWfT/sKXNi+kvD3YS5Yqrr9i4cBX5LeDLWHmjivboy+t86c+rH?=
 =?us-ascii?Q?uVP/qAduAH1Tcg5xnmtrrfs6GXiRK74sTBSHN0n5pLEJS8s7lnsWzVmZc3rY?=
 =?us-ascii?Q?4Hfv6Ttd4nBUs9BO2vhJTW7PoRUJKfc3hOyRW1KoULeARjJPbfRi9A0+qJSR?=
 =?us-ascii?Q?U5FpC5QsPTKUNUKH5CRjJdwzMxuKoDEfnaTVEtqUXJaPGgWbYLjeCz4nWSGe?=
 =?us-ascii?Q?F/rk1g=3D=3D?=
X-OriginatorOrg: toradex.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 23d351fc-6d65-446f-f2e7-08d9bcb69d20
X-MS-Exchange-CrossTenant-AuthSource: ZRAP278MB0642.CHEP278.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Dec 2021 14:57:55.7500
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: d9995866-0d9b-4251-8315-093f062abab4
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: rzV4za2PjVBlWGqnbPde381W7MjndRYI8i7pUndMkzKmRVchF/B/Ln5nVcqwsXg2UxbKgmsieznQorHE705EmDucjtY6iZsrAN4ICyKC7q0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: ZR0P278MB0490
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello,

On Sat, Dec 11, 2021 at 02:15:54PM +0000, Russell King (Oracle) wrote:
> I don't particularly like this - this impacts everyone who is using
> phylib at this point, whereas no reset was happening if the reset was
> already deasserted here.

I understand your concern, but I do not believe that this can create any
issue. The code should be able to handle the situation in which the PHY
is getting out of reset at that time.

> In the opening remarks to this series, it is stated:
> 
>   If a hardware-design is able to control power to the Ethernet PHY and
>   relying on software to do a reset, the PHY does no longer work after
>   resuming from suspend, given the PHY does need a hardware-reset.
> 
> This requirement is conditional on the hardware design, it isn't a
> universal requirement and won't apply everywhere. I think it needs to
> be described in firmware that this action is required. That said...
> 
> Please check the datasheet on the PHY regarding application of power and
> reset. You may find that the PHY datasheet requires the reset to be held
> active from power up until the clock input is stable - this could mean
> you need some other arrangement to assert the PHY reset signal after
> re-applying power sooner than would happen by the proposed point in the
> kernel.

I checked before sending this patch, the phy is a KSZ9131 [1] and
according to the power sequence timing  the reset should be toggled after
the power-up. No requirement on the clock or other signals.

The HW design is quite simple, we have a regulator controlling the PHY
power, and a GPIO controlling the reset. On suspend we remove the power
(FEC driver), on resume after enabling the power the PHY require a
reset, but nobody is doing it.

The issue here is that the phy regulator is handled by the FEC driver,
while the RESET_N GPIO can be controlled by both the FEC driver or the
phylib.
The initial proposal was to handle this into the FEC driver, but it was
not considered a good idea, therefore this new proposal.

One more comment about that, I do not believe that asserting the reset
in the suspend path is a good idea, in the general situation in which
the PHY is powered in suspend the power-consumption is likely to be
higher if the device is in reset compared to software power-down using
the BMCR register.

> universal requirement and won't apply everywhere. I think it needs to
> be described in firmware that this action is required. That said...
Are you thinking at a DTS property? Not sure to understand how do you
envision this. At the moment the regulator is not handled by the phylib,
and the property should be something like reset-on-resume, I guess ...

Francesco

[1] https://ww1.microchip.com/downloads/en/DeviceDoc/00002841C.pdf