Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1400286 > unrolled thread

Re: [PATCH RFT 1/2] phylib: add device reset GPIO support

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2016-05-12 20:50 +0200
    Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-12 23:40 +0200
      Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Andrew Lunn <andrew@lunn.ch> - 2016-05-13 06:10 +0200
        Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-13 23:20 +0200
      Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2016-05-13 09:10 +0200
        Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-13 23:50 +0200
      Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-13 21:20 +0200
        Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-16 04:10 +0200
    Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Roger Quadros <rogerq@ti.com> - 2016-05-13 11:10 +0200
      Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-13 21:40 +0200
        Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Andrew Lunn <andrew@lunn.ch> - 2016-05-13 22:50 +0200
          Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-13 23:00 +0200
            Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Andrew Lunn <andrew@lunn.ch> - 2016-05-14 01:50 +0200
              Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-14 21:40 +0200
                Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Andrew Lunn <andrew@lunn.ch> - 2016-05-14 22:00 +0200
                  Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Andrew Lunn <andrew@lunn.ch> - 2016-05-16 04:10 +0200
                    Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-19 15:50 +0200
                  Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2016-05-16 04:10 +0200
        Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Roger Quadros <rogerq@ti.com> - 2016-05-16 11:00 +0200
    Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Linus Walleij <linus.walleij@linaro.org> - 2016-05-26 11:10 +0200
      Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2016-05-26 21:10 +0200
        Re: [PATCH RFT 1/2] phylib: add device reset GPIO support Linus Walleij <linus.walleij@linaro.org> - 2016-05-30 17:00 +0200

Page 1 of 2  [1] 2  Next page →

#1400286 — Re: [PATCH RFT 1/2] phylib: add device reset GPIO support

Hello Sergei,

[we already talked about this patch in #armlinux, I'm now just
forwarding my comments on the list. Background was that I sent an easier
and less complete patch with the same idea. See
http://patchwork.ozlabs.org/patch/621418/]

[added Linus Walleij to Cc, there is a question for you/him below]

On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
> +++ net-next/Documentation/devicetree/bindings/net/phy.txt
> @@ -35,6 +35,8 @@ Optional Properties:
>  - broken-turn-around: If set, indicates the PHY device does not correctly
>    release the turn around line low at the end of a MDIO transaction.
>  
> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
> +
>  Example:
>  
>  ethernet-phy@0 {

This is great.

> Index: net-next/drivers/net/phy/at803x.c
> ===================================================================
> --- net-next.orig/drivers/net/phy/at803x.c
> +++ net-next/drivers/net/phy/at803x.c
> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL");
> [...]

My patch breaks this driver. I wasn't aware of it.

> [...]
> Index: net-next/drivers/net/phy/mdio_device.c
> ===================================================================
> --- net-next.orig/drivers/net/phy/mdio_device.c
> +++ net-next/drivers/net/phy/mdio_device.c
> [...]
> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
>  }
>  EXPORT_SYMBOL(mdio_device_remove);
>  
> +void mdio_device_reset(struct mdio_device *mdiodev, int value)
> +{
> +	if (mdiodev->reset)
> +		gpiod_set_value(mdiodev->reset, value);

Before v4.6-rc1~108^2~91 it was not necessary to check for the first
parameter being non-NULL before calling gpiod_set_value. Linus, did you
change this on purpose?

> +}
> +EXPORT_SYMBOL(mdio_device_reset);
> +
>  /**
>   * mdio_probe - probe an MDIO device
>   * @dev: device to probe
> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>  	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>  	int err = 0;
>  
> -	if (mdiodrv->probe)
> +	if (mdiodrv->probe) {
> +		/* Deassert the reset signal */
> +		mdio_device_reset(mdiodev, 0);
> +
>  		err = mdiodrv->probe(mdiodev);
>  
> +		/* Assert the reset signal */
> +		mdio_device_reset(mdiodev, 1);

I wonder if it's safe to do this in general. What if ->probe does
something with the phy that is lost by resetting but that is relied on
later?

> +	}
> +
>  	return err;
>  }
> [...]
> Index: net-next/drivers/of/of_mdio.c
> ===================================================================
> --- net-next.orig/drivers/of/of_mdio.c
> +++ net-next/drivers/of/of_mdio.c
> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>  				    struct device_node *child, u32 addr)
>  {
> +	struct gpio_desc *gpiod;
>  	struct phy_device *phy;
>  	bool is_c45;
>  	int rc;
> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>  	is_c45 = of_device_is_compatible(child,
>  					 "ethernet-phy-ieee802.3-c45");
>  
> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> +	/* Deassert the reset signal */
> +	if (!IS_ERR(gpiod))
> +		gpiod_direction_output(gpiod, 0);

This is wrong I think. You must only ignore -ENODEV, all other error
codes should be passed to the caller. (I see that's not trivial because
of_mdiobus_register_phy returns void.)

In my patch I used devm_gpiod_get_array which has the nice property that
I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
of the gpio to the device which is nice and IMHO the right direction for
the phylib (i.e. better embracing of the device model).

This cannot be used here easily however because there is no struct
device yet and this is only created after the phy id is determined. The
phy id is either read from the device tree or must be read from the phy
which might fail if reset is not deasserted.

Principally there is no reason however that the phy_id must be known
before the struct device is created however.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[toc] | [next] | [standalone]

#1400382

Hello.

On 05/12/2016 09:42 PM, Uwe Kleine-König wrote:

> [we already talked about this patch in #armlinux, I'm now just
> forwarding my comments on the list. Background was that I sent an easier
> and less complete patch with the same idea. See
> http://patchwork.ozlabs.org/patch/621418/]
>
> [added Linus Walleij to Cc, there is a question for you/him below]
>
> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
>> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
>> +++ net-next/Documentation/devicetree/bindings/net/phy.txt
>> @@ -35,6 +35,8 @@ Optional Properties:
>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>    release the turn around line low at the end of a MDIO transaction.
>>
>> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
>> +
>>  Example:
>>
>>  ethernet-phy@0 {
>
> This is great.
>
>> Index: net-next/drivers/net/phy/at803x.c
>> ===================================================================
>> --- net-next.orig/drivers/net/phy/at803x.c
>> +++ net-next/drivers/net/phy/at803x.c
>> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL");
>> [...]
>
> My patch breaks this driver. I wasn't aware of it.

    I tried to be as careful as I could but still it looks that I didn't 
succeed at that too...

[...]
>> Index: net-next/drivers/net/phy/mdio_device.c
>> ===================================================================
>> --- net-next.orig/drivers/net/phy/mdio_device.c
>> +++ net-next/drivers/net/phy/mdio_device.c
[...]
>> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>>  	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>>  	int err = 0;
>>
>> -	if (mdiodrv->probe)
>> +	if (mdiodrv->probe) {
>> +		/* Deassert the reset signal */
>> +		mdio_device_reset(mdiodev, 0);
>> +
>>  		err = mdiodrv->probe(mdiodev);
>>
>> +		/* Assert the reset signal */
>> +		mdio_device_reset(mdiodev, 1);
>
> I wonder if it's safe to do this in general. What if ->probe does
> something with the phy that is lost by resetting but that is relied on
> later?

    Well, I thought that config_init() method is designed for that but indeed 
the LXT driver writes to BMCR in its probe() method and hence is broken. Thank 
you for noticing...

[...]
>> Index: net-next/drivers/of/of_mdio.c
>> ===================================================================
>> --- net-next.orig/drivers/of/of_mdio.c
>> +++ net-next/drivers/of/of_mdio.c
>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>>  				    struct device_node *child, u32 addr)
>>  {
>> +	struct gpio_desc *gpiod;
>>  	struct phy_device *phy;
>>  	bool is_c45;
>>  	int rc;
>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>>  	is_c45 = of_device_is_compatible(child,
>>  					 "ethernet-phy-ieee802.3-c45");
>>
>> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>> +	/* Deassert the reset signal */
>> +	if (!IS_ERR(gpiod))
>> +		gpiod_direction_output(gpiod, 0);
>
> This is wrong I think. You must only ignore -ENODEV, all other error

    At least -ENOSYS should also be ignored (it's returned when gpiolib is not 
configured), right? When does -ENODEV gets returned (it's not easy to follow)?

> codes should be passed to the caller.

    The caller doesn't care anyway...

> (I see that's not trivial because
> of_mdiobus_register_phy returns void.)

    I've made this function *void* in net-next.

> In my patch I used devm_gpiod_get_array which has the nice property that
> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> of the gpio to the device which is nice and IMHO the right direction for
> the phylib (i.e. better embracing of the device model).
>
> This cannot be used here easily however because there is no struct
> device yet and this is only created after the phy id is determined.

    Your last patch [1] didn't make use of the managed device API (devm) 
either, I didn't quite get to the bottom of that...

> The
> phy id is either read from the device tree or must be read from the phy
> which might fail if reset is not deasserted.

> Principally there is no reason however that the phy_id must be known
> before the struct device is created however.

    It's just that the code is cleaner that way...

[1] http://paste.debian.net/683630/

> Best regards
> Uwe

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1400492

> >>+	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> >>+	/* Deassert the reset signal */
> >>+	if (!IS_ERR(gpiod))
> >>+		gpiod_direction_output(gpiod, 0);
> >
> >This is wrong I think. You must only ignore -ENODEV, all other error
> 
>    At least -ENOSYS should also be ignored (it's returned when
> gpiolib is not configured), right? When does -ENODEV gets returned
> (it's not easy to follow)?
> 
> >codes should be passed to the caller.
> 
>    The caller doesn't care anyway...

It should do. What if fwnode_get_named_gpiod() returns -EPROBE_DEFER
because the GPIO driver has not been loaded yet?

	Andrew

[toc] | [prev] | [next] | [standalone]

#1400989

Hello.

On 05/13/2016 07:06 AM, Andrew Lunn wrote:

>>>> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>>>> +	/* Deassert the reset signal */
>>>> +	if (!IS_ERR(gpiod))
>>>> +		gpiod_direction_output(gpiod, 0);
>>>
>>> This is wrong I think. You must only ignore -ENODEV, all other error
>>
>>    At least -ENOSYS should also be ignored (it's returned when
>> gpiolib is not configured), right? When does -ENODEV gets returned
>> (it's not easy to follow)?
>>
>>> codes should be passed to the caller.
>>
>>    The caller doesn't care anyway...
>
> It should do.

    Tell that to Florian. So far, everybody has been happy with 
of_mdiobus_register(). Until I had to touch this code. :-)

> What if fwnode_get_named_gpiod() returns -EPROBE_DEFER
> because the GPIO driver has not been loaded yet?

    Bad luck. :-)
    Seriously, I'll see what I can do but it's not a trivial case.

> 	Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1400544

Hello Sergei,

On Fri, May 13, 2016 at 12:35:50AM +0300, Sergei Shtylyov wrote:
> On 05/12/2016 09:42 PM, Uwe Kleine-König wrote:
> >>Index: net-next/drivers/net/phy/mdio_device.c
> >>===================================================================
> >>--- net-next.orig/drivers/net/phy/mdio_device.c
> >>+++ net-next/drivers/net/phy/mdio_device.c
> [...]
> >>@@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
> >> 	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
> >> 	int err = 0;
> >>
> >>-	if (mdiodrv->probe)
> >>+	if (mdiodrv->probe) {
> >>+		/* Deassert the reset signal */
> >>+		mdio_device_reset(mdiodev, 0);
> >>+
> >> 		err = mdiodrv->probe(mdiodev);
> >>
> >>+		/* Assert the reset signal */
> >>+		mdio_device_reset(mdiodev, 1);
> >
> >I wonder if it's safe to do this in general. What if ->probe does
> >something with the phy that is lost by resetting but that is relied on
> >later?
> 
>    Well, I thought that config_init() method is designed for that but indeed
> the LXT driver writes to BMCR in its probe() method and hence is broken.
> Thank you for noticing...
> 
> [...]
> >>Index: net-next/drivers/of/of_mdio.c
> >>===================================================================
> >>--- net-next.orig/drivers/of/of_mdio.c
> >>+++ net-next/drivers/of/of_mdio.c
> >>@@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
> >> static void of_mdiobus_register_phy(struct mii_bus *mdio,
> >> 				    struct device_node *child, u32 addr)
> >> {
> >>+	struct gpio_desc *gpiod;
> >> 	struct phy_device *phy;
> >> 	bool is_c45;
> >> 	int rc;
> >>@@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
> >> 	is_c45 = of_device_is_compatible(child,
> >> 					 "ethernet-phy-ieee802.3-c45");
> >>
> >>+	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
> >>+	/* Deassert the reset signal */
> >>+	if (!IS_ERR(gpiod))
> >>+		gpiod_direction_output(gpiod, 0);
> >
> >This is wrong I think. You must only ignore -ENODEV, all other error
> 
>    At least -ENOSYS should also be ignored (it's returned when gpiolib is
> not configured), right?

No, that's a common misconception. If GPIOLIB is off you cannot
determine if dt specified a reset gpio. So you have the choice between:

 a) ignore -ENOSYS and so don't handle the reset line in the cases where
    it's necessary probably yielding an "Error: phy not found" message.
 b) fail to probe even if a reset handling isn't necessary, yielding
    "Error: failed to get hold of reset gpio".

I say b) is the better one. It's easier to debug because the error
message is better, GPIOLIB is enabled in most cases anyhow (still maybe
select it?) and it's ensured that we're operating in the limits of the
hardware specs (maybe a phy returns a random value when a register is
read while reset is applied?).

> When does -ENODEV gets returned (it's not easy to follow)?

I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*()
family returns -ENODEV if the node doesn't have a reset-gpio property.

> >codes should be passed to the caller.
> 
>    The caller doesn't care anyway...
> 
> >(I see that's not trivial because
> >of_mdiobus_register_phy returns void.)
> 
>    I've made this function *void* in net-next.

I'd say this is a step in the wrong direction. For example this makes it
impossible to handle the case where the phy should be probed, the gpio
driver isn't available yet, though. Normally you'd want to return
-EPROBE_DEFER in this case and retry probing later.

> >In my patch I used devm_gpiod_get_array which has the nice property that
> >I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> >of the gpio to the device which is nice and IMHO the right direction for
> >the phylib (i.e. better embracing of the device model).
> >
> >This cannot be used here easily however because there is no struct
> >device yet and this is only created after the phy id is determined.
> 
>    Your last patch [1] didn't make use of the managed device API (devm)
> either, I didn't quite get to the bottom of that...

Right, devm didn't work. My patch was a prototype for a way that allowed
to bind the lifetime of the gpio to the device. This might be longer
than the call to mdiobus_unregister. See the problems that i2c handles
because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c
at the end of i2c_del_adapter.

> >The phy id is either read from the device tree or must be read from
> >the phy which might fail if reset is not deasserted.
> 
> >Principally there is no reason however that the phy_id must be known
> >before the struct device is created however.
> 
>    It's just that the code is cleaner that way...

I don't agree, I don't see that

	determine_phyid()
	allocate_device()

is better than

	allocate_device()
	determine_phyid()

. The former is maybe easier because that's the status quo and it
doesn't need patching. But IMHO the result is on a similar level of
cleanliness.

Best regards
Uwe

> [1] http://paste.debian.net/683630/

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[toc] | [prev] | [next] | [standalone]

#1400997

Hello.

On 05/13/2016 10:06 AM, Uwe Kleine-König wrote:

[...]
>>>> Index: net-next/drivers/of/of_mdio.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/of/of_mdio.c
>>>> +++ net-next/drivers/of/of_mdio.c
>>>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>>>> static void of_mdiobus_register_phy(struct mii_bus *mdio,
>>>> 				    struct device_node *child, u32 addr)
>>>> {
>>>> +	struct gpio_desc *gpiod;
>>>> 	struct phy_device *phy;
>>>> 	bool is_c45;
>>>> 	int rc;
>>>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>>>> 	is_c45 = of_device_is_compatible(child,
>>>> 					 "ethernet-phy-ieee802.3-c45");
>>>>
>>>> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>>>> +	/* Deassert the reset signal */
>>>> +	if (!IS_ERR(gpiod))
>>>> +		gpiod_direction_output(gpiod, 0);
>>>
>>> This is wrong I think. You must only ignore -ENODEV, all other error
>>
>>    At least -ENOSYS should also be ignored (it's returned when gpiolib is
>> not configured), right?
>
> No, that's a common misconception. If GPIOLIB is off you cannot
> determine if dt specified a reset gpio. So you have the choice between:
>
>  a) ignore -ENOSYS and so don't handle the reset line in the cases where
>     it's necessary probably yielding an "Error: phy not found" message.
>  b) fail to probe even if a reset handling isn't necessary, yielding
>     "Error: failed to get hold of reset gpio".
>
> I say b) is the better one. It's easier to debug because the error
> message is better, GPIOLIB is enabled in most cases anyhow (still maybe
> select it?) and it's ensured that we're operating in the limits of the
> hardware specs (maybe a phy returns a random value when a register is
> read while reset is applied?).

    It returns all ones in my case.

>> When does -ENODEV gets returned (it's not easy to follow)?
>
> I don't know for sure for fwnode_get_named_gpiod, but the gpiod_get*()
> family returns -ENODEV if the node doesn't have a reset-gpio property.

    Are you sure it's not -ENOENT?

>>> codes should be passed to the caller.
>>
>>    The caller doesn't care anyway...
>>
>>> (I see that's not trivial because
>>> of_mdiobus_register_phy returns void.)
>>
>>    I've made this function *void* in net-next.
>
> I'd say this is a step in the wrong direction. For example this makes it
> impossible to handle the case where the phy should be probed, the gpio
> driver isn't available yet, though. Normally you'd want to return
> -EPROBE_DEFER in this case and retry probing later.

    Well, of_mdiobus_register() is not an easy function to add the checks 
whiuch were never there (and undo the done stuff on failure). I'll see what I 
can do but no promises...

>>> In my patch I used devm_gpiod_get_array which has the nice property that
>>> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
>>> of the gpio to the device which is nice and IMHO the right direction for
>>> the phylib (i.e. better embracing of the device model).
>>>
>>> This cannot be used here easily however because there is no struct
>>> device yet and this is only created after the phy id is determined.
>>
>>    Your last patch [1] didn't make use of the managed device API (devm)
>> either, I didn't quite get to the bottom of that...
>
> Right, devm didn't work. My patch was a prototype for a way that allowed
> to bind the lifetime of the gpio to the device. This might be longer
> than the call to mdiobus_unregister.

    Ah, that was the reason... Well, then you hardly achieved anything with 
rehashing the code...

> See the problems that i2c handles
> because it doesn't handle lifetimes correctly in drivers/i2c/i2c-core.c
> at the end of i2c_del_adapter.
>
>>> The phy id is either read from the device tree or must be read from
>>> the phy which might fail if reset is not deasserted.
>>
>>> Principally there is no reason however that the phy_id must be known
>>> before the struct device is created however.
>>
>>    It's just that the code is cleaner that way...
>
> I don't agree, I don't see that
>
> 	determine_phyid()
> 	allocate_device()
>
> is better than
>
> 	allocate_device()
> 	determine_phyid()

    This is an oversimplified view. Actually, it is:

	error = determine_phyid()
	if (error)
		return
	allocate_device()

versus

	allocate_device()
	error = determine_phyid()
	if (error)
		free_device()

> . The former is maybe easier because that's the status quo and it
> doesn't need patching. But IMHO the result is on a similar level of
> cleanliness.

    I disagree. And I don't see why it's necessary at all. Just to use another 
gpiolib API?

> Best regards
> Uwe

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1400948

Hello.

On 05/13/2016 12:35 AM, Sergei Shtylyov wrote:

>> [we already talked about this patch in #armlinux, I'm now just
>> forwarding my comments on the list. Background was that I sent an easier
>> and less complete patch with the same idea. See
>> http://patchwork.ozlabs.org/patch/621418/]
>>
>> [added Linus Walleij to Cc, there is a question for you/him below]
>>
>> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
>>> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
>>> +++ net-next/Documentation/devicetree/bindings/net/phy.txt
>>> @@ -35,6 +35,8 @@ Optional Properties:
>>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>>    release the turn around line low at the end of a MDIO transaction.
>>>
>>> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
>>> +
>>>  Example:
>>>
>>>  ethernet-phy@0 {
>>
>> This is great.
>>
>>> Index: net-next/drivers/net/phy/at803x.c
>>> ===================================================================
>>> --- net-next.orig/drivers/net/phy/at803x.c
>>> +++ net-next/drivers/net/phy/at803x.c
>>> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL");
>>> [...]
>>
>> My patch breaks this driver. I wasn't aware of it.
>
>    I tried to be as careful as I could but still it looks that I didn't
> succeed at that too...

    Hm, I'm starting to forget the vital details about my patch...

> [...]
>>> Index: net-next/drivers/net/phy/mdio_device.c
>>> ===================================================================
>>> --- net-next.orig/drivers/net/phy/mdio_device.c
>>> +++ net-next/drivers/net/phy/mdio_device.c
> [...]
>>> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>>>      struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>>>      int err = 0;
>>>
>>> -    if (mdiodrv->probe)
>>> +    if (mdiodrv->probe) {
>>> +        /* Deassert the reset signal */
>>> +        mdio_device_reset(mdiodev, 0);
>>> +
>>>          err = mdiodrv->probe(mdiodev);
>>>
>>> +        /* Assert the reset signal */
>>> +        mdio_device_reset(mdiodev, 1);
>>
>> I wonder if it's safe to do this in general. What if ->probe does
>> something with the phy that is lost by resetting but that is relied on
>> later?
>
>    Well, I thought that config_init() method is designed for that but indeed
> the LXT driver writes to BMCR in its probe() method and hence is broken.
 > Thank you for noticing...

    It's broken even without my patch. The phylib will cause a PHY soft reset 
when attaching to the PHY device, so all BMCR programming dpone in the probe() 
method will be lost. My patch does make sense as is. Looks like I should also 
look into fixing lxt.c. Florian, what do you think?

[...]

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1401211

Hello.

On 05/13/2016 10:18 PM, Sergei Shtylyov wrote:

>>> [we already talked about this patch in #armlinux, I'm now just
>>> forwarding my comments on the list. Background was that I sent an easier
>>> and less complete patch with the same idea. See
>>> http://patchwork.ozlabs.org/patch/621418/]
>>>
>>> [added Linus Walleij to Cc, there is a question for you/him below]
>>>
>>> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
>>>> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
>>>> +++ net-next/Documentation/devicetree/bindings/net/phy.txt
>>>> @@ -35,6 +35,8 @@ Optional Properties:
>>>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>>>    release the turn around line low at the end of a MDIO transaction.
>>>>
>>>> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
>>>> +
>>>>  Example:
>>>>
>>>>  ethernet-phy@0 {
>>>
>>> This is great.
>>>
>>>> Index: net-next/drivers/net/phy/at803x.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/phy/at803x.c
>>>> +++ net-next/drivers/net/phy/at803x.c
>>>> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL");
>>>> [...]
>>>
>>> My patch breaks this driver. I wasn't aware of it.
>>
>>    I tried to be as careful as I could but still it looks that I didn't
>> succeed at that too...
>
>    Hm, I'm starting to forget the vital details about my patch...
>
>> [...]
>>>> Index: net-next/drivers/net/phy/mdio_device.c
>>>> ===================================================================
>>>> --- net-next.orig/drivers/net/phy/mdio_device.c
>>>> +++ net-next/drivers/net/phy/mdio_device.c
>> [...]
>>>> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>>>>      struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>>>>      int err = 0;
>>>>
>>>> -    if (mdiodrv->probe)
>>>> +    if (mdiodrv->probe) {
>>>> +        /* Deassert the reset signal */
>>>> +        mdio_device_reset(mdiodev, 0);
>>>> +
>>>>          err = mdiodrv->probe(mdiodev);
>>>>
>>>> +        /* Assert the reset signal */
>>>> +        mdio_device_reset(mdiodev, 1);
>>>
>>> I wonder if it's safe to do this in general. What if ->probe does
>>> something with the phy that is lost by resetting but that is relied on
>>> later?
>>
>>    Well, I thought that config_init() method is designed for that but indeed
>> the LXT driver writes to BMCR in its probe() method and hence is broken.
>> Thank you for noticing...
>
>    It's broken even without my patch. The phylib will cause a PHY soft reset

    Only iff the config_init() method exists in the PHY driver...

> when attaching to the PHY device, so all BMCR programming dpone in the probe()
> method will be lost. My patch does make sense as is.

    No, actually it doesn't. :-(

> Looks like I should alsolook into fixing lxt.c.

    It took me to actually do a patch to understand my fault. Sigh... :-/

> Florian, what do you think?

    Florian, is phy_init_hw() logic correct?

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1400624

Hi Sergei,

On 12/05/16 21:42, Uwe Kleine-König wrote:
> Hello Sergei,
> 
> [we already talked about this patch in #armlinux, I'm now just
> forwarding my comments on the list. Background was that I sent an easier
> and less complete patch with the same idea. See
> http://patchwork.ozlabs.org/patch/621418/]
> 
> [added Linus Walleij to Cc, there is a question for you/him below]
> 
> On Fri, Apr 29, 2016 at 01:12:54AM +0300, Sergei Shtylyov wrote:
>> --- net-next.orig/Documentation/devicetree/bindings/net/phy.txt
>> +++ net-next/Documentation/devicetree/bindings/net/phy.txt
>> @@ -35,6 +35,8 @@ Optional Properties:
>>  - broken-turn-around: If set, indicates the PHY device does not correctly
>>    release the turn around line low at the end of a MDIO transaction.
>>  
>> +- reset-gpios: The GPIO phandle and specifier for the PHY reset signal.
>> +
>>  Example:
>>  
>>  ethernet-phy@0 {
> 
> This is great.
> 
>> Index: net-next/drivers/net/phy/at803x.c
>> ===================================================================
>> --- net-next.orig/drivers/net/phy/at803x.c
>> +++ net-next/drivers/net/phy/at803x.c
>> @@ -65,7 +65,6 @@ MODULE_LICENSE("GPL");
>> [...]
> 
> My patch breaks this driver. I wasn't aware of it.
> 
>> [...]
>> Index: net-next/drivers/net/phy/mdio_device.c
>> ===================================================================
>> --- net-next.orig/drivers/net/phy/mdio_device.c
>> +++ net-next/drivers/net/phy/mdio_device.c
>> [...]
>> @@ -103,6 +105,13 @@ void mdio_device_remove(struct mdio_devi
>>  }
>>  EXPORT_SYMBOL(mdio_device_remove);
>>  
>> +void mdio_device_reset(struct mdio_device *mdiodev, int value)
>> +{
>> +	if (mdiodev->reset)
>> +		gpiod_set_value(mdiodev->reset, value);
> 
> Before v4.6-rc1~108^2~91 it was not necessary to check for the first
> parameter being non-NULL before calling gpiod_set_value. Linus, did you
> change this on purpose?
> 
>> +}
>> +EXPORT_SYMBOL(mdio_device_reset);
>> +
>>  /**
>>   * mdio_probe - probe an MDIO device
>>   * @dev: device to probe
>> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>>  	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>>  	int err = 0;
>>  
>> -	if (mdiodrv->probe)
>> +	if (mdiodrv->probe) {
>> +		/* Deassert the reset signal */
>> +		mdio_device_reset(mdiodev, 0);
>> +
>>  		err = mdiodrv->probe(mdiodev);
>>  
>> +		/* Assert the reset signal */
>> +		mdio_device_reset(mdiodev, 1);
> 
> I wonder if it's safe to do this in general. What if ->probe does
> something with the phy that is lost by resetting but that is relied on
> later?

mdio_probe is called for non PHY devices only, right?

I'm a bit lost as to why we're de-asserting reset at multiple places. i.e.
mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy().

Isn't it simpler to just de-assert it once at the topmost level?
i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()?

Also, how about issuing a reset pulse instead of just de-asserting it.
This would tackle the case where PHY is in invalid state with reset already
de-asserted.

Another issue is that on some boards we have one reset line tied to
multiple PHYs. How do we prevent multiple resets being taking place when each of
the PHYs are registered? Do we just specify the reset line only for one PHY in
the DT or can we have the reset gpio in the mdio_bus node for such case?

cheers,
-roger

> 
>> +	}
>> +
>>  	return err;
>>  }
>> [...]
>> Index: net-next/drivers/of/of_mdio.c
>> ===================================================================
>> --- net-next.orig/drivers/of/of_mdio.c
>> +++ net-next/drivers/of/of_mdio.c
>> @@ -44,6 +44,7 @@ static int of_get_phy_id(struct device_n
>>  static void of_mdiobus_register_phy(struct mii_bus *mdio,
>>  				    struct device_node *child, u32 addr)
>>  {
>> +	struct gpio_desc *gpiod;
>>  	struct phy_device *phy;
>>  	bool is_c45;
>>  	int rc;
>> @@ -52,10 +53,17 @@ static void of_mdiobus_register_phy(stru
>>  	is_c45 = of_device_is_compatible(child,
>>  					 "ethernet-phy-ieee802.3-c45");
>>  
>> +	gpiod = fwnode_get_named_gpiod(&child->fwnode, "reset-gpios");
>> +	/* Deassert the reset signal */
>> +	if (!IS_ERR(gpiod))
>> +		gpiod_direction_output(gpiod, 0);
> 
> This is wrong I think. You must only ignore -ENODEV, all other error
> codes should be passed to the caller. (I see that's not trivial because
> of_mdiobus_register_phy returns void.)
> 
> In my patch I used devm_gpiod_get_array which has the nice property that
> I can already pass GPIOD_OUT_LOW in flags. Also this binds the lifetime
> of the gpio to the device which is nice and IMHO the right direction for
> the phylib (i.e. better embracing of the device model).
> 
> This cannot be used here easily however because there is no struct
> device yet and this is only created after the phy id is determined. The
> phy id is either read from the device tree or must be read from the phy
> which might fail if reset is not deasserted.
> 
> Principally there is no reason however that the phy_id must be known
> before the struct device is created however.
> 
> Best regards
> Uwe
> 

[toc] | [prev] | [next] | [standalone]

#1400952

Hello.

On 05/13/2016 12:07 PM, Roger Quadros wrote:

[...]

>>> +}
>>> +EXPORT_SYMBOL(mdio_device_reset);
>>> +
>>>  /**
>>>   * mdio_probe - probe an MDIO device
>>>   * @dev: device to probe
>>> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>>>  	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>>>  	int err = 0;
>>>
>>> -	if (mdiodrv->probe)
>>> +	if (mdiodrv->probe) {
>>> +		/* Deassert the reset signal */
>>> +		mdio_device_reset(mdiodev, 0);
>>> +
>>>  		err = mdiodrv->probe(mdiodev);
>>>
>>> +		/* Assert the reset signal */
>>> +		mdio_device_reset(mdiodev, 1);
>>
>> I wonder if it's safe to do this in general. What if ->probe does
>> something with the phy that is lost by resetting but that is relied on
>> later?
>
> mdio_probe is called for non PHY devices only, right?

    Yes, those also can have a reset signal.

> I'm a bit lost as to why we're de-asserting reset at multiple places. i.e.
> mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy().

> Isn't it simpler to just de-assert it once at the topmost level?

    I wasn't after simplicity here. The intent was to save some power putting 
the device at reset when it's not needed. Florian Fainelly (the phylib 
maintainer) actually wanted me to go even further with that and assert reset 
in phy_suspend() but it was too much for me.

> i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()?
>
> Also, how about issuing a reset pulse instead of just de-asserting it.

    If it's already held at reset, my assumption is that it's enough time 
passed already.

> This would tackle the case where PHY is in invalid state with reset already
> de-asserted.

     Good question. I haven't yet met such cases though.

> Another issue is that on some boards we have one reset line tied to
> multiple PHYs.How do we prevent multiple resets being taking place when each of
> the PHYs are registered?

    My patch just doesn't address this case -- it's about the individual 
resets only.

> Do we just specify the reset line only for one PHY in
> the DT or can we have the reset gpio in the mdio_bus node for such case?

    I think there's mii_bus::reset() method for that case. Some Ethernet 
drivers even use it
(mostly instead of the code being suggested here).

> cheers,
> -roger

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1400970

> >Another issue is that on some boards we have one reset line tied to
> >multiple PHYs.How do we prevent multiple resets being taking place when each of
> >the PHYs are registered?
> 
>    My patch just doesn't address this case -- it's about the
> individual resets only.

This actually needs to be addresses a layer above. What you have is a
bus reset, not a device reset. So the gpio line is associated to the
mdio bus, not a PHY. Either your MDIO driver needs to handle the gpio
line, or in __mdio_register(), before it starts looking at the
children.

	Andrew

[toc] | [prev] | [next] | [standalone]

#1400977

On 05/13/2016 11:44 PM, Andrew Lunn wrote:

>>> Another issue is that on some boards we have one reset line tied to
>>> multiple PHYs.How do we prevent multiple resets being taking place when each of
>>> the PHYs are registered?
>>
>>    My patch just doesn't address this case -- it's about the
>> individual resets only.
>
> This actually needs to be addresses a layer above. What you have is a
> bus reset, not a device reset.

    No.
    There's simply no such thing as a bus reset for the xMII/MDIO busses, 
there's simply no reset signaling on them. Every device has its own reset 
signal and its own timing requirements.

> So the gpio line is associated to the mdio bus, not a PHY.

    No.

> Either your MDIO driver needs to handle the gpio
> line,  or in __mdio_register(),

    __mdiobus_register(), you mean?

> before it starts looking at the
> children.

    It's basically the same thing.
    The MDIO bus reset is a misconception.

>
> 	Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1401037

On Fri, May 13, 2016 at 11:56:12PM +0300, Sergei Shtylyov wrote:
> On 05/13/2016 11:44 PM, Andrew Lunn wrote:
> 
> >>>Another issue is that on some boards we have one reset line tied to
> >>>multiple PHYs.How do we prevent multiple resets being taking place when each of
> >>>the PHYs are registered?
> >>
> >>   My patch just doesn't address this case -- it's about the
> >>individual resets only.
> >
> >This actually needs to be addresses a layer above. What you have is a
> >bus reset, not a device reset.
> 
>    No.
>    There's simply no such thing as a bus reset for the xMII/MDIO
> busses, there's simply no reset signaling on them. Every device has
> its own reset signal and its own timing requirements.

Except in the case above, where two phys are sharing the same reset
signal. So although it is not part of the mdio standard to have a bus
reset, this is in effect what the gpio line is doing, resetting all
devices on the bus. If you don't model that as a bus reset, how do you
model it?

      Andrew

[toc] | [prev] | [next] | [standalone]

#1401138

Hello.

On 05/14/2016 02:44 AM, Andrew Lunn wrote:

>>>>> Another issue is that on some boards we have one reset line tied to
>>>>> multiple PHYs.How do we prevent multiple resets being taking place when each of
>>>>> the PHYs are registered?
>>>>
>>>>   My patch just doesn't address this case -- it's about the
>>>> individual resets only.
>>>
>>> This actually needs to be addresses a layer above. What you have is a
>>> bus reset, not a device reset.
>>
>>    No.
>>    There's simply no such thing as a bus reset for the xMII/MDIO
>> busses, there's simply no reset signaling on them. Every device has
>> its own reset signal and its own timing requirements.
>
> Except in the case above, where two phys are sharing the same reset
> signal. So although it is not part of the mdio standard to have a bus
> reset, this is in effect what the gpio line is doing, resetting all
> devices on the bus. If you don't model that as a bus reset, how do you
> model it?

    I'm not suggesting that the shared reset should be handled by my patch. 
Contrariwise, I suggested to use the mii_bus::reset() method -- I see it as a 
necessary evil. However, in the more common case of a single PHY, this method 
simply doesn't scale -- you'd have to teach each and every individual MAC/ 
MDIO driver to do the GPIO reset trick.

>       Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1401141

On Sat, May 14, 2016 at 10:36:38PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 05/14/2016 02:44 AM, Andrew Lunn wrote:
> 
> >>>>>Another issue is that on some boards we have one reset line tied to
> >>>>>multiple PHYs.How do we prevent multiple resets being taking place when each of
> >>>>>the PHYs are registered?
> >>>>
> >>>>  My patch just doesn't address this case -- it's about the
> >>>>individual resets only.
> >>>
> >>>This actually needs to be addresses a layer above. What you have is a
> >>>bus reset, not a device reset.
> >>
> >>   No.
> >>   There's simply no such thing as a bus reset for the xMII/MDIO
> >>busses, there's simply no reset signaling on them. Every device has
> >>its own reset signal and its own timing requirements.
> >
> >Except in the case above, where two phys are sharing the same reset
> >signal. So although it is not part of the mdio standard to have a bus
> >reset, this is in effect what the gpio line is doing, resetting all
> >devices on the bus. If you don't model that as a bus reset, how do you
> >model it?
> 
>    I'm not suggesting that the shared reset should be handled by my
> patch. Contrariwise, I suggested to use the mii_bus::reset() method

I think we miss understood each other somewhere.

Your code is great for one gpio reset line for one phy.

I think there could be similar code one layer above to handle one gpio
line for multiple phys.

     Andrew

[toc] | [prev] | [next] | [standalone]

#1401192

> >I think there could be similar code one layer above to handle one gpio
> >line for multiple phys.
> 
>    Ah, you want me to recognize some MAC/MDIO bound prop (e.g.
> "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it
> now that my patch needs fixing anyway...

Hi Sergi

It does not need to be you implementing this, your hardware does not
need it. However, if you do feel like doing it, great.

     Andrew

[toc] | [prev] | [next] | [standalone]

#1403753

Hello.

On 5/15/2016 6:23 PM, Andrew Lunn wrote:

>>> I think there could be similar code one layer above to handle one gpio
>>> line for multiple phys.
>>
>>    Ah, you want me to recognize some MAC/MDIO bound prop (e.g.
>> "mdio-reset-gpios") in of_mdiobus_register()? I'll think about it
>> now that my patch needs fixing anyway...
>
> Hi Sergi
>
> It does not need to be you implementing this, your hardware does not
> need it. However, if you do feel like doing it, great.

    It would cover my "single PHY" case anyway.

>      Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1401215

Hello.

On 05/14/2016 10:50 PM, Andrew Lunn wrote:

>>>>>>> Another issue is that on some boards we have one reset line tied to
>>>>>>> multiple PHYs.How do we prevent multiple resets being taking place when each of
>>>>>>> the PHYs are registered?
>>>>>>
>>>>>>  My patch just doesn't address this case -- it's about the
>>>>>> individual resets only.
>>>>>
>>>>> This actually needs to be addresses a layer above. What you have is a
>>>>> bus reset, not a device reset.
>>>>
>>>>   No.
>>>>   There's simply no such thing as a bus reset for the xMII/MDIO
>>>> busses, there's simply no reset signaling on them. Every device has
>>>> its own reset signal and its own timing requirements.
>>>
>>> Except in the case above, where two phys are sharing the same reset
>>> signal. So although it is not part of the mdio standard to have a bus
>>> reset, this is in effect what the gpio line is doing, resetting all
>>> devices on the bus. If you don't model that as a bus reset, how do you
>>> model it?
>>
>>    I'm not suggesting that the shared reset should be handled by my
>> patch. Contrariwise, I suggested to use the mii_bus::reset() method
>
> I think we miss understood each other somewhere.
>
> Your code is great for one gpio reset line for one phy.
>
> I think there could be similar code one layer above to handle one gpio
> line for multiple phys.

    Ah, you want me to recognize some MAC/MDIO bound prop (e.g. 
"mdio-reset-gpios") in of_mdiobus_register()? I'll think about it now that my 
patch needs fixing anyway...

>      Andrew

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1401379

On 13/05/16 22:36, Sergei Shtylyov wrote:
> Hello.
> 
> On 05/13/2016 12:07 PM, Roger Quadros wrote:
> 
> [...]
> 
>>>> +}
>>>> +EXPORT_SYMBOL(mdio_device_reset);
>>>> +
>>>>  /**
>>>>   * mdio_probe - probe an MDIO device
>>>>   * @dev: device to probe
>>>> @@ -117,9 +126,16 @@ static int mdio_probe(struct device *dev
>>>>      struct mdio_driver *mdiodrv = to_mdio_driver(drv);
>>>>      int err = 0;
>>>>
>>>> -    if (mdiodrv->probe)
>>>> +    if (mdiodrv->probe) {
>>>> +        /* Deassert the reset signal */
>>>> +        mdio_device_reset(mdiodev, 0);
>>>> +
>>>>          err = mdiodrv->probe(mdiodev);
>>>>
>>>> +        /* Assert the reset signal */
>>>> +        mdio_device_reset(mdiodev, 1);
>>>
>>> I wonder if it's safe to do this in general. What if ->probe does
>>> something with the phy that is lost by resetting but that is relied on
>>> later?
>>
>> mdio_probe is called for non PHY devices only, right?
> 
>    Yes, those also can have a reset signal.
> 
>> I'm a bit lost as to why we're de-asserting reset at multiple places. i.e.
>> mdio_probe(), phy_device_register(), phy_init_hw(), phy_probe(), of_mdiobus_register_phy().
> 
>> Isn't it simpler to just de-assert it once at the topmost level?
> 
>    I wasn't after simplicity here. The intent was to save some power putting the device at reset when it's not needed. Florian Fainelly (the phylib maintainer) actually wanted me to go even further with that and assert reset in phy_suspend() but it was too much for me.

Is using RESET for power saving a standard practice? Isn't there some register interface for power saving?
My concern here is that RESET does a number of things that might be undesired for normal operation.
e.g. PHY's will use bootstrap settings on RESET and we need to ensure that bootstrap pins are always in
the right setting before issuing a RESET.

What happens to the PHY link? Is it lost while PHY RESET is asserted?
Is this really desirable?


> 
>> i.e. of_mdiobus_register_device() f and of_mdiobus_register_phy()?
>>
>> Also, how about issuing a reset pulse instead of just de-asserting it.
> 
>    If it's already held at reset, my assumption is that it's enough time passed already.
> 
>> This would tackle the case where PHY is in invalid state with reset already
>> de-asserted.
> 
>     Good question. I haven't yet met such cases though.
> 
>> Another issue is that on some boards we have one reset line tied to
>> multiple PHYs.How do we prevent multiple resets being taking place when each of
>> the PHYs are registered?
> 
>    My patch just doesn't address this case -- it's about the individual resets only.
> 
>> Do we just specify the reset line only for one PHY in
>> the DT or can we have the reset gpio in the mdio_bus node for such case?
> 
>    I think there's mii_bus::reset() method for that case. Some Ethernet drivers even use it
> (mostly instead of the code being suggested here).
> 

--
cheers,
-roger

[toc] | [prev] | [next] | [standalone]

#1407465

On Thu, May 12, 2016 at 8:42 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> [added Linus Walleij to Cc, there is a question for you/him below]
(...)
>> +void mdio_device_reset(struct mdio_device *mdiodev, int value)
>> +{
>> +     if (mdiodev->reset)
>> +             gpiod_set_value(mdiodev->reset, value);
>
> Before v4.6-rc1~108^2~91 it was not necessary to check for the first
> parameter being non-NULL before calling gpiod_set_value. Linus, did you
> change this on purpose?

Not really. And AFAICT it is still not necessary: what changed is that
an error message will be printed by VALIDATE_DESC() if you do that.
And that is proper I guess? I think it's sloppy code to randomly pass in
NULL to a call and just expect it to bail out, it seems more like
exercising the error path than something you'd normally rely on.

Or am I getting things wrong?

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web