Re: [PATCH v2] net: mdiobus: Add a function to deassert reset

[Yan Wang]

On 5/10/2023 7:55 PM, Alexander Stein wrote:
> Am Mittwoch, 10. Mai 2023, 10:02:52 CEST schrieb Yan Wang:
> > It is possible to mount multiple sub-devices on the mido bus.
> mdio bus
Yes, misspelled.
> > The hardware power-on does not necessarily reset these devices.
> > The device may be in an uncertain state, causing the device's ID
> > to be scanned.
> >
> > So, before adding a reset to the scan, make sure the device is in
> > normal working mode.
> >
> > I found that the subsequent drive registers the reset pin into the
> > structure of the sub-device to prevent conflicts, so release the
> > reset pin.
> >
> > Signed-off-by: Yan Wang <rk.code@xxxxxxxxxxx>
> We had similar cases where the (single) PHY was in reset during Linux boot.
> Should you be able to make this work by using a "ethernet-phy-id%4x.%4x"
> compatible? See also [1]
>
> [1] https://lkml.org/lkml/2020/10/28/1139
Well, I've seen the [1] before, this method may mask some issues. For 
example ,if I use
another type of phy ,I have to modify the DT, Is it very cumbersome?
> > ---
> > v2:
> >    - fixed commit message
> >    - Using gpiod_ replace gpio_
> > v1:
> > https://lore.kernel.org/all/KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR01M
> > B5448.apcprd01.prod.exchangelabs.com/ - Incorrect description of commit
> > message.
> >    - The gpio-api too old
> > ---
> >   drivers/net/mdio/fwnode_mdio.c | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index 1183ef5e203e..6695848b8ef2 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -57,6 +57,20 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
> > return register_mii_timestamper(arg.np, arg.args[0]);
> >   }
> >
> > +static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
> > +{
> > +	struct gpio_desc *reset;
> > +
> > +	reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_HIGH,
> NULL);
> > +	if (IS_ERR(reset) && PTR_ERR(reset) != -EPROBE_DEFER)
> How are you dealing with EPROBE_DEFER if the reset line is e.g. attached to an
> i2c expander, which is to be probed later on?
Thank you ,The logic is wrong,trying to fix it.
> > +		return;
> > +
> > +	usleep_range(100, 200);
> How do you know a PHY's reset pulse width?
>
> > +	gpiod_set_value_cansleep(reset, 0);
> What about post-reset stabilization times before MDIO access is allowed?
yes,I need to get reset pulse width and post-reset stabilization times 
from reset-assert-us and  reset-deassert-us. right?
> > +	/*Release the reset pin,it needs to be registered with the PHY.*/
> /* Release [...] PHY. */
>
> Best regards,
> Alexander
Thank you for your support.
> > +	gpiod_put(reset);
> > +}
> > +
> >   int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
> >   				       struct phy_device *phy,
> >   				       struct fwnode_handle *child,
> u32 addr)
> > @@ -119,6 +133,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> >   	u32 phy_id;
> >   	int rc;
> >
> > +	fwnode_mdiobus_pre_enable_phy(child);
> > +
> >   	psec = fwnode_find_pse_control(child);
> >   	if (IS_ERR(psec))
> >   		return PTR_ERR(psec);