phy_connect_dev calling phy_reset???

phy_connect_dev calling phy_reset???

[Tim Harvey]

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

Re: phy_connect_dev calling phy_reset???

[Tim Harvey]

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

Re: phy_connect_dev calling phy_reset???

[Michael Walle]

> 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

Re: phy_connect_dev calling phy_reset???

[Tim Harvey]

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/?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
>