On Mon, Mar 7, 2022 at 12:51 AM Michael Walle <mich...@walle.cc> wrote: > > > On Mon, Feb 28, 2022 at 12:01 PM Tim Harvey <thar...@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.faine...@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.106449...@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/?id=87aa9f9c61ad - 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 >