phy_connect_dev calling phy_reset???
Greetings,
I'm wondering if it is proper in U-Boot for phy_connect_dev() to always call phy_reset() which generates a soft reset via BMCR_RESET.
For some (or most?) PHY's this resets specific PHY config such as RGMII delays and LED configuration that may have been configured by firmware running prior to U-Boot (SPL/TPL).
I believe there was some discussion and thrash about this in the Linux kernel in the past and while I can't find the discussion or patches I see that for the current kernel BMCR_RESET is in genphy_soft_reset which() is not called in the generic phy_connect() but instead only called by a handful of phy drivers which I would expect is ok as those phy drivers would also be re-configuring the PHY.
Specifically I have an issue with this with a board that has custom firmware code that runs prior to U-Boot and the BMCR reset is undoing specific PHY config that I've done in this firmware causing me to look at implementing PHY drivers in U-Boot that otherwise would not be needed.
Best Regards,
Tim
On Mon, Feb 28, 2022 at 12:01 PM Tim Harvey tharvey@gateworks.com wrote:
Greetings,
I'm wondering if it is proper in U-Boot for phy_connect_dev() to always call phy_reset() which generates a soft reset via BMCR_RESET.
For some (or most?) PHY's this resets specific PHY config such as RGMII delays and LED configuration that may have been configured by firmware running prior to U-Boot (SPL/TPL).
I believe there was some discussion and thrash about this in the Linux kernel in the past and while I can't find the discussion or patches I see that for the current kernel BMCR_RESET is in genphy_soft_reset which() is not called in the generic phy_connect() but instead only called by a handful of phy drivers which I would expect is ok as those phy drivers would also be re-configuring the PHY.
Specifically I have an issue with this with a board that has custom firmware code that runs prior to U-Boot and the BMCR reset is undoing specific PHY config that I've done in this firmware causing me to look at implementing PHY drivers in U-Boot that otherwise would not be needed.
Joe and Ramon,
Do you have any comment on removing the call to phy_reset in phy_connect_dev? Linux delegates calling genphy_soft_reset to the phy drivers that need to whereas U-Boot seems to take the opposite approach requireing a phy driver to set PHY_FLAG_BROKEN_RESET to skip the reset. I think U-Boot should follow Linux and not perform a reset without a PHY driver specifically needing it.
Best regards,
Tim
On Mon, Feb 28, 2022 at 12:01 PM Tim Harvey tharvey@gateworks.com wrote:
Greetings,
I'm wondering if it is proper in U-Boot for phy_connect_dev() to always call phy_reset() which generates a soft reset via BMCR_RESET.
FWIW, a PHY might also get a hardware reset prior to probing in eth_phy_pre_probe() if the device tree contains a reset gpio line.
-michael
For some (or most?) PHY's this resets specific PHY config such as RGMII delays and LED configuration that may have been configured by firmware running prior to U-Boot (SPL/TPL).
I believe there was some discussion and thrash about this in the Linux kernel in the past and while I can't find the discussion or patches I see that for the current kernel BMCR_RESET is in genphy_soft_reset which() is not called in the generic phy_connect() but instead only called by a handful of phy drivers which I would expect is ok as those phy drivers would also be re-configuring the PHY.
Specifically I have an issue with this with a board that has custom firmware code that runs prior to U-Boot and the BMCR reset is undoing specific PHY config that I've done in this firmware causing me to look at implementing PHY drivers in U-Boot that otherwise would not be needed.
Joe and Ramon,
Do you have any comment on removing the call to phy_reset in phy_connect_dev? Linux delegates calling genphy_soft_reset to the phy drivers that need to whereas U-Boot seems to take the opposite approach requireing a phy driver to set PHY_FLAG_BROKEN_RESET to skip the reset. I think U-Boot should follow Linux and not perform a reset without a PHY driver specifically needing it.
Best regards,
Tim
On Mon, Mar 7, 2022 at 12:51 AM Michael Walle michael@walle.cc wrote:
On Mon, Feb 28, 2022 at 12:01 PM Tim Harvey tharvey@gateworks.com wrote:
Greetings,
I'm wondering if it is proper in U-Boot for phy_connect_dev() to always call phy_reset() which generates a soft reset via BMCR_RESET.
FWIW, a PHY might also get a hardware reset prior to probing in eth_phy_pre_probe() if the device tree contains a reset gpio line.
Michael,
Yes, but the hardware reset is configurable via dt allowing me to opt-out of a reset. In my particular case I have a GPY111 (intel-xway) PHY which is hardware reset and configured in software prior to U-Boot and the soft reset in U-Boot undoes this configuration requiring me to add a driver for the PHY in U-Boot which seems silly as I wouldn't need it if the forced soft reset was not done. I'm sure there are other PHY's that act as mine.
I found the following patches I was looking for in regards to removing general PHY soft reset's in Linux: https://lore.kernel.org/lkml/20180925182846.30042-2-f.fainelli@gmail.com/ - net: phy: Stop with excessive soft reset - Fixes: 87aa9f9c61ad ("net: phy: consolidate PHY reset in phy_init_hw()") https://lore.kernel.org/lkml/20170809202156.106449339@linuxfoundation.org/ - net: phy: Do not perform software reset for Generic PHY - Fixes: 87aa9f9c61ad ("net: phy: consolidate PHY reset in phy_init_hw()") https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... - 87aa9f9c61ad ("net: phy: consolidate PHY reset in phy_init_hw()")
I've added some people on Cc that were involved in those patches so they can hopefully weigh in on their thoughts of always forcing a BMCR reset in the bootloader.
My worry is that removing the generic PHY reset in U-Boot may cause some issues with various PHY's that may require the soft reset forcing existing PHY drivers to need to add an explicit call to phy_reset() (like the Linux PHY drivers that need a reset do) or cause someone to have to add a new PHY driver for those PHYs just to call phy_reset().
There already exists a phydev flag 'PHY_FLAG_BROKEN_RESET' that allows PHY drivers to skip the reset indicating people have run into this before but again that requires adding PHY drivers for PHY's just to remove a reset that I don't feel should be there anyway. I could submit a patch that adds a Kconfig to skip the reset much like was done for CONFIG_PHY_RESET_DELAY which apparently was created to add a post soft-reset delay that is only used in 2 configs (bmips_common.h and stv0991.h). Interestingly enough the comment in CONFIG_PHY_RESET_DELAY says it was needed for the LXT971A PHY and such a post-delay could also be handled by moving the phy_reset into the PHY drivers that need them.
Best regards,
Tim
For some (or most?) PHY's this resets specific PHY config such as RGMII delays and LED configuration that may have been configured by firmware running prior to U-Boot (SPL/TPL).
I believe there was some discussion and thrash about this in the Linux kernel in the past and while I can't find the discussion or patches I see that for the current kernel BMCR_RESET is in genphy_soft_reset which() is not called in the generic phy_connect() but instead only called by a handful of phy drivers which I would expect is ok as those phy drivers would also be re-configuring the PHY.
Specifically I have an issue with this with a board that has custom firmware code that runs prior to U-Boot and the BMCR reset is undoing specific PHY config that I've done in this firmware causing me to look at implementing PHY drivers in U-Boot that otherwise would not be needed.
Joe and Ramon,
Do you have any comment on removing the call to phy_reset in phy_connect_dev? Linux delegates calling genphy_soft_reset to the phy drivers that need to whereas U-Boot seems to take the opposite approach requireing a phy driver to set PHY_FLAG_BROKEN_RESET to skip the reset. I think U-Boot should follow Linux and not perform a reset without a PHY driver specifically needing it.
Best regards,
Tim
participants (2)
-
Michael Walle -
Tim Harvey