linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* GPIO registration for external Ethernet PHY oscillator enable/disable
@ 2014-09-23 21:52 Michael Welling
  2014-09-25 19:17 ` Michael Welling
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Welling @ 2014-09-23 21:52 UTC (permalink / raw)
  To: linux-kernel

I have some questions that span multiple subsystems including
gpio/pinctrl, apm, and net subsystems.

On some of our system on module designs, we use a GPIO to toggle the
enable pin on external oscillators. In particular, we are using a 50Mhz
oscillator to drive a clock on a RMII Ethernet PHY.

Though I can configure the pin such that the Ethernet interface works we 
are looking to disable the oscillator during APM sleep but after the PHY is 
put into a low power mode.

How do I register a GPIO for use in the PHY suspend and resume code?
Can it be handled outside of the PHY driver?
If so how do ensure the appropriate suspend and resume sequencing?

For reference, we are using a Micrel KSZ8081 PHY connected to a
AT91SAMA5D35 processor.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-23 21:52 GPIO registration for external Ethernet PHY oscillator enable/disable Michael Welling
@ 2014-09-25 19:17 ` Michael Welling
  2014-09-25 19:56   ` Florian Fainelli
  2014-10-07 14:04   ` Linus Walleij
  0 siblings, 2 replies; 11+ messages in thread
From: Michael Welling @ 2014-09-25 19:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: mwelling, f.fainelli, grant.likely, linus.walleij, rjw, gregkh

Looks like my original message got buried in the mailing list.

Lets try this again with a few key developers CC'd.

Original Message:
I have some questions that span multiple subsystems including
gpio/pinctrl, apm, and net subsystems.

On some of our system on module designs, we use a GPIO to toggle the
enable pin on external oscillators. In particular, we are using a 50Mhz
oscillator to drive a clock on a RMII Ethernet PHY.

Though I can configure the pin such that the Ethernet interface works we
are looking to disable the oscillator during APM sleep but after the PHY
is put into a low power mode.

How do I register a GPIO for use in the PHY suspend and resume code?
Can it be handled outside of the PHY driver?
If so how do ensure the appropriate suspend and resume sequencing?

For reference, we are using a Micrel KSZ8081 PHY connected to a
AT91SAMA5D35 processor.

Addendum:
I ran into another situation where a GPIO enabled oscillator was used.
The oscillator in this case drives the master clock for a audio codec.
In the old days (before device tree), I could initialize the GPIO in the
platform board file. Now with device tree I can setup the pin multipler
but the initial state of the GPIO I am not sure how to set.

Is there a way to directly change the state of a GPIO pin from a
devicetree entry?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-25 19:17 ` Michael Welling
@ 2014-09-25 19:56   ` Florian Fainelli
  2014-09-26 16:59     ` Michael Welling
  2014-10-07 14:04   ` Linus Walleij
  1 sibling, 1 reply; 11+ messages in thread
From: Florian Fainelli @ 2014-09-25 19:56 UTC (permalink / raw)
  To: Michael Welling, linux-kernel
  Cc: grant.likely, linus.walleij, rjw, gregkh, Nicolas Ferre

On 09/25/2014 12:17 PM, Michael Welling wrote:
> Looks like my original message got buried in the mailing list.
> 
> Lets try this again with a few key developers CC'd.
> 
> Original Message:
> I have some questions that span multiple subsystems including
> gpio/pinctrl, apm, and net subsystems.
> 
> On some of our system on module designs, we use a GPIO to toggle the
> enable pin on external oscillators. In particular, we are using a 50Mhz
> oscillator to drive a clock on a RMII Ethernet PHY.
> 
> Though I can configure the pin such that the Ethernet interface works we
> are looking to disable the oscillator during APM sleep but after the PHY
> is put into a low power mode.
> 
> How do I register a GPIO for use in the PHY suspend and resume code?

So, PHY drivers are allowed to provide specialized implementations for
suspend/resume operations that are called by phy_suspend() and
phy_resume(), the current Micrel PHY driver uses the generic
suspend/resume implementation and it is best if we can keep doing that.

> Can it be handled outside of the PHY driver?

I see a few possible options:

- hook a pm_runtime callbacks for your platform, check the device
pointer to make sure this is the PHY device, and when that is the case,
toggle the GPIO accordingly

- add an additional "osc_gpio" configuration parameter passed to the
Ethernet MAC driver (presumably drivers/net/ethernet/cadence/macb.c?)
and toggle the GPIO before and after the calls to the PHY state machine
(phy_suspend, phy_resume, phy_start, phy_stop), that might be simpler

- last but not least, make the PHY driver aware of that optional GPIO,
create customized PHY suspend/resume/config_aneg callbacks


> If so how do ensure the appropriate suspend and resume sequencing?
> 
> For reference, we are using a Micrel KSZ8081 PHY connected to a
> AT91SAMA5D35 processor.
> 
> Addendum:
> I ran into another situation where a GPIO enabled oscillator was used.
> The oscillator in this case drives the master clock for a audio codec.
> In the old days (before device tree), I could initialize the GPIO in the
> platform board file. Now with device tree I can setup the pin multipler
> but the initial state of the GPIO I am not sure how to set.
> 
> Is there a way to directly change the state of a GPIO pin from a
> devicetree entry?
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-25 19:56   ` Florian Fainelli
@ 2014-09-26 16:59     ` Michael Welling
  2014-09-26 17:16       ` Florian Fainelli
  0 siblings, 1 reply; 11+ messages in thread
From: Michael Welling @ 2014-09-26 16:59 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, grant.likely, linus.walleij, rjw, gregkh, Nicolas Ferre

On Thu, Sep 25, 2014 at 12:56:34PM -0700, Florian Fainelli wrote:
> So, PHY drivers are allowed to provide specialized implementations for
> suspend/resume operations that are called by phy_suspend() and
> phy_resume(), the current Micrel PHY driver uses the generic
> suspend/resume implementation and it is best if we can keep doing that.
>

In my situation the defualt phy_suspend is not sufficient. We are
looking to use the board for an application that requires a low sleep
current. The KZS8081 has a slow oscillator low power mode that is 
required to meet the requirements.

So I have already overwritten the suspend/resume to send the required
commands to the PHY to achieve the slow clock mode.

If you are interested the sequence is explained in the datasheet pg 34:
http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ8081MNX-RNB.pdf

> > Can it be handled outside of the PHY driver?
> 
> I see a few possible options:
> 
> - hook a pm_runtime callbacks for your platform, check the device
> pointer to make sure this is the PHY device, and when that is the case,
> toggle the GPIO accordingly

Not too familiar with the pm_runtime callbacks.

Can you point me to a similar example that is already in the kernel?

> 
> - add an additional "osc_gpio" configuration parameter passed to the
> Ethernet MAC driver (presumably drivers/net/ethernet/cadence/macb.c?)
> and toggle the GPIO before and after the calls to the PHY state machine
> (phy_suspend, phy_resume, phy_start, phy_stop), that might be simpler
>

This seems the wrong place as the oscillator is specific to the PHY.

> - last but not least, make the PHY driver aware of that optional GPIO,
> create customized PHY suspend/resume/config_aneg callbacks
> 

This to me feels like the path of least resistance. Though the driver
does not appear to be a platform driver so I am not sure how to pass
GPIOs to it. Maybe I am missing something.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-26 16:59     ` Michael Welling
@ 2014-09-26 17:16       ` Florian Fainelli
  2014-09-26 17:32         ` Michael Welling
  2014-09-26 20:04         ` Michael Welling
  0 siblings, 2 replies; 11+ messages in thread
From: Florian Fainelli @ 2014-09-26 17:16 UTC (permalink / raw)
  To: Michael Welling
  Cc: linux-kernel, grant.likely, linus.walleij, rjw, gregkh, Nicolas Ferre

On 09/26/2014 09:59 AM, Michael Welling wrote:
> On Thu, Sep 25, 2014 at 12:56:34PM -0700, Florian Fainelli wrote:
>> So, PHY drivers are allowed to provide specialized implementations for
>> suspend/resume operations that are called by phy_suspend() and
>> phy_resume(), the current Micrel PHY driver uses the generic
>> suspend/resume implementation and it is best if we can keep doing that.
>>
> 
> In my situation the defualt phy_suspend is not sufficient. We are
> looking to use the board for an application that requires a low sleep
> current. The KZS8081 has a slow oscillator low power mode that is 
> required to meet the requirements.
> 
> So I have already overwritten the suspend/resume to send the required
> commands to the PHY to achieve the slow clock mode.
> 
> If you are interested the sequence is explained in the datasheet pg 34:
> http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ8081MNX-RNB.pdf
> 
>>> Can it be handled outside of the PHY driver?
>>
>> I see a few possible options:
>>
>> - hook a pm_runtime callbacks for your platform, check the device
>> pointer to make sure this is the PHY device, and when that is the case,
>> toggle the GPIO accordingly
> 
> Not too familiar with the pm_runtime callbacks.
> 
> Can you point me to a similar example that is already in the kernel?

drivers/sh/pm_runtime.c is a simple example, there might others in the
OMAP code.

> 
>>
>> - add an additional "osc_gpio" configuration parameter passed to the
>> Ethernet MAC driver (presumably drivers/net/ethernet/cadence/macb.c?)
>> and toggle the GPIO before and after the calls to the PHY state machine
>> (phy_suspend, phy_resume, phy_start, phy_stop), that might be simpler
>>
> 
> This seems the wrong place as the oscillator is specific to the PHY.

Yes and no, this might feel like the wrong place, but ultimately, the
Ethernet MAC is a consumer of the PHY device, and is in control, through
the PHY library of how and when the PHY gets to be powered off.

> 
>> - last but not least, make the PHY driver aware of that optional GPIO,
>> create customized PHY suspend/resume/config_aneg callbacks
>>
> 
> This to me feels like the path of least resistance. Though the driver
> does not appear to be a platform driver so I am not sure how to pass
> GPIOs to it. Maybe I am missing something.

If your platform uses Device Tree, you need to add a probe() and
remove() callbacks to the micrel PHY driver, and fetch the gpio resource
from there.

For non-Device Tree, we might have to find an another to specify such
auxiliary information, but traditionally, people have been using the
help of the Ethernet MAC driver to provide additional information down
to the PHY driver.
--
Florian


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-26 17:16       ` Florian Fainelli
@ 2014-09-26 17:32         ` Michael Welling
  2014-09-26 20:04         ` Michael Welling
  1 sibling, 0 replies; 11+ messages in thread
From: Michael Welling @ 2014-09-26 17:32 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, grant.likely, linus.walleij, rjw, gregkh, Nicolas Ferre

On Fri, Sep 26, 2014 at 10:16:51AM -0700, Florian Fainelli wrote:
> On 09/26/2014 09:59 AM, Michael Welling wrote:
> > On Thu, Sep 25, 2014 at 12:56:34PM -0700, Florian Fainelli wrote:
> >> So, PHY drivers are allowed to provide specialized implementations for
> >> suspend/resume operations that are called by phy_suspend() and
> >> phy_resume(), the current Micrel PHY driver uses the generic
> >> suspend/resume implementation and it is best if we can keep doing that.
> >>
> > 
> > In my situation the defualt phy_suspend is not sufficient. We are
> > looking to use the board for an application that requires a low sleep
> > current. The KZS8081 has a slow oscillator low power mode that is 
> > required to meet the requirements.
> > 
> > So I have already overwritten the suspend/resume to send the required
> > commands to the PHY to achieve the slow clock mode.
> > 
> > If you are interested the sequence is explained in the datasheet pg 34:
> > http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ8081MNX-RNB.pdf
> > 
> >>> Can it be handled outside of the PHY driver?
> >>
> >> I see a few possible options:
> >>
> >> - hook a pm_runtime callbacks for your platform, check the device
> >> pointer to make sure this is the PHY device, and when that is the case,
> >> toggle the GPIO accordingly
> > 
> > Not too familiar with the pm_runtime callbacks.
> > 
> > Can you point me to a similar example that is already in the kernel?
> 
> drivers/sh/pm_runtime.c is a simple example, there might others in the
> OMAP code.
> 
> > 
> >>
> >> - add an additional "osc_gpio" configuration parameter passed to the
> >> Ethernet MAC driver (presumably drivers/net/ethernet/cadence/macb.c?)
> >> and toggle the GPIO before and after the calls to the PHY state machine
> >> (phy_suspend, phy_resume, phy_start, phy_stop), that might be simpler
> >>
> > 
> > This seems the wrong place as the oscillator is specific to the PHY.
> 
> Yes and no, this might feel like the wrong place, but ultimately, the
> Ethernet MAC is a consumer of the PHY device, and is in control, through
> the PHY library of how and when the PHY gets to be powered off.

This is a good point. This driver also has the added advantage that it
is a platform driver so the GPIO could more easily be registered via
the device tree.

Lets try this option first and see how it works out.

> 
> > 
> >> - last but not least, make the PHY driver aware of that optional GPIO,
> >> create customized PHY suspend/resume/config_aneg callbacks
> >>
> > 
> > This to me feels like the path of least resistance. Though the driver
> > does not appear to be a platform driver so I am not sure how to pass
> > GPIOs to it. Maybe I am missing something.
> 
> If your platform uses Device Tree, you need to add a probe() and
> remove() callbacks to the micrel PHY driver, and fetch the gpio resource
> from there.
> 
> For non-Device Tree, we might have to find an another to specify such
> auxiliary information, but traditionally, people have been using the
> help of the Ethernet MAC driver to provide additional information down
> to the PHY driver.
> --
> Florian
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-26 17:16       ` Florian Fainelli
  2014-09-26 17:32         ` Michael Welling
@ 2014-09-26 20:04         ` Michael Welling
  2014-10-07 14:09           ` Linus Walleij
  1 sibling, 1 reply; 11+ messages in thread
From: Michael Welling @ 2014-09-26 20:04 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, grant.likely, linus.walleij, rjw, gregkh, Nicolas Ferre

On Fri, Sep 26, 2014 at 10:16:51AM -0700, Florian Fainelli wrote:
> 
> Yes and no, this might feel like the wrong place, but ultimately, the
> Ethernet MAC is a consumer of the PHY device, and is in control, through
> the PHY library of how and when the PHY gets to be powered off.
>

So here is the patch that I made that hooks into the macb driver.

Please look it over and tell me if we are on the same page.

The thing that bothers me about this solution is that it will only work with the macb driver.
So everytime I use the same PHY/OSC combo with a different SoC I will have to do another patch.

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index e4e34b6..8cd363f 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -28,6 +28,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_net.h>
+#include <linux/of_gpio.h>
 #include <linux/pinctrl/consumer.h>
 
 #include "macb.h"
@@ -1833,6 +1834,16 @@ static int __init macb_probe(struct platform_device *pdev)
                bp->phy_interface = err;
        }
 
+       bp->phy_osc_gpio = of_get_named_gpio(pdev->dev.of_node, "osc-gpio", 0);
+
+       if (gpio_is_valid(bp->phy_osc_gpio))
+       {
+               if (gpio_request(bp->phy_osc_gpio, "osc-gpio") != 0) {
+                       pr_info("Oscillator GPIO not available.\n");
+                       bp->phy_osc_gpio = 0;
+               }
+       }
+
        if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
                macb_or_gem_writel(bp, USRIO, GEM_BIT(RGMII));
        else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII)
@@ -1927,6 +1938,9 @@ static int macb_suspend(struct platform_device *pdev, pm_message_t state)
        netif_carrier_off(netdev);
        netif_device_detach(netdev);
 
+       if (gpio_is_valid(bp->phy_osc_gpio))
+               gpio_set_value_cansleep(bp->phy_osc_gpio, 0);
+
        clk_disable_unprepare(bp->hclk);
        clk_disable_unprepare(bp->pclk);
 
@@ -1941,6 +1955,9 @@ static int macb_resume(struct platform_device *pdev)
        clk_prepare_enable(bp->pclk);
        clk_prepare_enable(bp->hclk);
 
+       if (gpio_is_valid(bp->phy_osc_gpio))
+               gpio_set_value_cansleep(bp->phy_osc_gpio, 1);
+
        netif_device_attach(netdev);
 
        return 0;
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index f407615..8ca3dbd 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -596,6 +596,7 @@ struct macb {
        u32                     caps;
 
        phy_interface_t         phy_interface;
+       int                     phy_osc_gpio;
 
        /* AT91RM9200 transmit */
        struct sk_buff *skb;                    /* holds skb until xmit interrupt completes */

 

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-25 19:17 ` Michael Welling
  2014-09-25 19:56   ` Florian Fainelli
@ 2014-10-07 14:04   ` Linus Walleij
  2014-10-07 16:30     ` Michael Welling
  1 sibling, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2014-10-07 14:04 UTC (permalink / raw)
  To: Michael Welling
  Cc: linux-kernel, Florian Fainelli, Grant Likely, Rafael J. Wysocki, Greg KH

On Thu, Sep 25, 2014 at 9:17 PM, Michael Welling <mwelling@emacinc.com> wrote:

> How do I register a GPIO for use in the PHY suspend and resume code?
> Can it be handled outside of the PHY driver?

Nominally these days you should get a named GPIO using the
GPIO descriptor abstraction, putting a named GPIO reference in the
device tree node for the PHY, which should work fine
if you're using device tree for this system.
Documentation/gpio/consumer.txt

> If so how do ensure the appropriate suspend and resume sequencing?

AFAICT there is no good answer to this kind of questions. I guess
my best answer would be something like what has been said for
DRM drivers: handle all the sequence-sensitive hardware in one big
composite driver and handle sequencing in the driver.

> For reference, we are using a Micrel KSZ8081 PHY connected to a
> AT91SAMA5D35 processor.

I don't know how AT91 is progressing on the device tree side or if
it's strictly required to boot these days. If it is, you should be able
to proceed as indicated.

> Addendum:
> I ran into another situation where a GPIO enabled oscillator was used.
> The oscillator in this case drives the master clock for a audio codec.
> In the old days (before device tree), I could initialize the GPIO in the
> platform board file. Now with device tree I can setup the pin multipler
> but the initial state of the GPIO I am not sure how to set.

A driver needs to do this. Like a drivers/clk driver in this case I
guess?

> Is there a way to directly change the state of a GPIO pin from a
> devicetree entry?

I have suggested mechanisms like GPIO hogs to replace the need
for very basic drivers that would just take a GPIO during init,
set it and never do anything with it.

Like the gpiochip node should have some hog entries:

gpio-hog-high = <0>, <1>, <2>...;
gpio-hog-low = <...>;

Then they would be taken away from other consumers and not
possible to use for anything.

This has so far not been implemented though.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-09-26 20:04         ` Michael Welling
@ 2014-10-07 14:09           ` Linus Walleij
  2014-10-07 16:38             ` Michael Welling
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2014-10-07 14:09 UTC (permalink / raw)
  To: Michael Welling
  Cc: Florian Fainelli, linux-kernel, Grant Likely, Rafael J. Wysocki,
	Greg KH, Nicolas Ferre

On Fri, Sep 26, 2014 at 10:04 PM, Michael Welling <mwelling@emacinc.com> wrote:
> On Fri, Sep 26, 2014 at 10:16:51AM -0700, Florian Fainelli wrote:
>>
>> Yes and no, this might feel like the wrong place, but ultimately, the
>> Ethernet MAC is a consumer of the PHY device, and is in control, through
>> the PHY library of how and when the PHY gets to be powered off.
>>
>
> So here is the patch that I made that hooks into the macb driver.
>
> Please look it over and tell me if we are on the same page.
>
> The thing that bothers me about this solution is that it will only work with the macb driver.
> So everytime I use the same PHY/OSC combo with a different SoC I will have to do another patch.
>
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index e4e34b6..8cd363f 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -28,6 +28,7 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_net.h>
> +#include <linux/of_gpio.h>

No please,
#include <linux/gpio/consumer.h>

> @@ -1833,6 +1834,16 @@ static int __init macb_probe(struct platform_device *pdev)
>                 bp->phy_interface = err;
>         }
>
> +       bp->phy_osc_gpio = of_get_named_gpio(pdev->dev.of_node, "osc-gpio", 0);
> +
> +       if (gpio_is_valid(bp->phy_osc_gpio))
> +       {
> +               if (gpio_request(bp->phy_osc_gpio, "osc-gpio") != 0) {
> +                       pr_info("Oscillator GPIO not available.\n");
> +                       bp->phy_osc_gpio = 0;
> +               }
> +       }


No,
bp->phy_osc_gpiod = devm_gpiod_get(&pdev->dev, "osc-gpio", 0);

And error handling.

> +       if (gpio_is_valid(bp->phy_osc_gpio))
> +               gpio_set_value_cansleep(bp->phy_osc_gpio, 0);
> +

if (bp->phy_osc_gpiod)
  gpiod_set_...

> +       if (gpio_is_valid(bp->phy_osc_gpio))
> +               gpio_set_value_cansleep(bp->phy_osc_gpio, 1);

Dito.

>         phy_interface_t         phy_interface;
> +       int                     phy_osc_gpio;

struct gpio_desc *phy_osc_gpiod;

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-10-07 14:04   ` Linus Walleij
@ 2014-10-07 16:30     ` Michael Welling
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Welling @ 2014-10-07 16:30 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-kernel, Florian Fainelli, Grant Likely, Rafael J. Wysocki, Greg KH

On Tue, Oct 07, 2014 at 04:04:27PM +0200, Linus Walleij wrote:
> On Thu, Sep 25, 2014 at 9:17 PM, Michael Welling <mwelling@emacinc.com> wrote:
> 
> > How do I register a GPIO for use in the PHY suspend and resume code?
> > Can it be handled outside of the PHY driver?
> 
> Nominally these days you should get a named GPIO using the
> GPIO descriptor abstraction, putting a named GPIO reference in the
> device tree node for the PHY, which should work fine
> if you're using device tree for this system.
> Documentation/gpio/consumer.txt
> 
> > If so how do ensure the appropriate suspend and resume sequencing?
> 
> AFAICT there is no good answer to this kind of questions. I guess
> my best answer would be something like what has been said for
> DRM drivers: handle all the sequence-sensitive hardware in one big
> composite driver and handle sequencing in the driver.
> 
> > For reference, we are using a Micrel KSZ8081 PHY connected to a
> > AT91SAMA5D35 processor.
> 
> I don't know how AT91 is progressing on the device tree side or if
> it's strictly required to boot these days. If it is, you should be able
> to proceed as indicated.

AT91 is fairly up to snuff on the device tree implementation.

> 
> > Addendum:
> > I ran into another situation where a GPIO enabled oscillator was used.
> > The oscillator in this case drives the master clock for a audio codec.
> > In the old days (before device tree), I could initialize the GPIO in the
> > platform board file. Now with device tree I can setup the pin multipler
> > but the initial state of the GPIO I am not sure how to set.
> 
> A driver needs to do this. Like a drivers/clk driver in this case I
> guess?
> 
> > Is there a way to directly change the state of a GPIO pin from a
> > devicetree entry?
> 
> I have suggested mechanisms like GPIO hogs to replace the need
> for very basic drivers that would just take a GPIO during init,
> set it and never do anything with it.
> 
> Like the gpiochip node should have some hog entries:
> 
> gpio-hog-high = <0>, <1>, <2>...;
> gpio-hog-low = <...>;
> 
> Then they would be taken away from other consumers and not
> possible to use for anything.
> 
> This has so far not been implemented though.

I was thinking less of a hog and more of an initial configuration.
Specifically for GPIO unregistered otherwise until exported via the GPIO
class.

> 
> Yours,
> Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: GPIO registration for external Ethernet PHY oscillator enable/disable
  2014-10-07 14:09           ` Linus Walleij
@ 2014-10-07 16:38             ` Michael Welling
  0 siblings, 0 replies; 11+ messages in thread
From: Michael Welling @ 2014-10-07 16:38 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Florian Fainelli, linux-kernel, Grant Likely, Rafael J. Wysocki,
	Greg KH, Nicolas Ferre

On Tue, Oct 07, 2014 at 04:09:45PM +0200, Linus Walleij wrote:
> On Fri, Sep 26, 2014 at 10:04 PM, Michael Welling <mwelling@emacinc.com> wrote:
> > On Fri, Sep 26, 2014 at 10:16:51AM -0700, Florian Fainelli wrote:
> >>
> >> Yes and no, this might feel like the wrong place, but ultimately, the
> >> Ethernet MAC is a consumer of the PHY device, and is in control, through
> >> the PHY library of how and when the PHY gets to be powered off.
> >>
> >
> > So here is the patch that I made that hooks into the macb driver.
> >
> > Please look it over and tell me if we are on the same page.
> >
> > The thing that bothers me about this solution is that it will only work with the macb driver.
> > So everytime I use the same PHY/OSC combo with a different SoC I will have to do another patch.
> >
> > diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> > index e4e34b6..8cd363f 100644
> > --- a/drivers/net/ethernet/cadence/macb.c
> > +++ b/drivers/net/ethernet/cadence/macb.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_net.h>
> > +#include <linux/of_gpio.h>
> 
> No please,
> #include <linux/gpio/consumer.h>
> 
> > @@ -1833,6 +1834,16 @@ static int __init macb_probe(struct platform_device *pdev)
> >                 bp->phy_interface = err;
> >         }
> >
> > +       bp->phy_osc_gpio = of_get_named_gpio(pdev->dev.of_node, "osc-gpio", 0);
> > +
> > +       if (gpio_is_valid(bp->phy_osc_gpio))
> > +       {
> > +               if (gpio_request(bp->phy_osc_gpio, "osc-gpio") != 0) {
> > +                       pr_info("Oscillator GPIO not available.\n");
> > +                       bp->phy_osc_gpio = 0;
> > +               }
> > +       }
> 
> 
> No,
> bp->phy_osc_gpiod = devm_gpiod_get(&pdev->dev, "osc-gpio", 0);
> 
> And error handling.
> 
> > +       if (gpio_is_valid(bp->phy_osc_gpio))
> > +               gpio_set_value_cansleep(bp->phy_osc_gpio, 0);
> > +
> 
> if (bp->phy_osc_gpiod)
>   gpiod_set_...
> 
> > +       if (gpio_is_valid(bp->phy_osc_gpio))
> > +               gpio_set_value_cansleep(bp->phy_osc_gpio, 1);
> 
> Dito.
> 
> >         phy_interface_t         phy_interface;
> > +       int                     phy_osc_gpio;
> 
> struct gpio_desc *phy_osc_gpiod;

Thank you for clearifying the preferred/correct interface.

This version was not even functional and I had to update it 
such that it was.

I will, of course, follow the above suggestions if/when I submit an
actual patch.

I am still not sure the MAC driver is the best place for this
registration.

> 
> Yours,
> Linus Walleij

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2014-10-07 16:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-23 21:52 GPIO registration for external Ethernet PHY oscillator enable/disable Michael Welling
2014-09-25 19:17 ` Michael Welling
2014-09-25 19:56   ` Florian Fainelli
2014-09-26 16:59     ` Michael Welling
2014-09-26 17:16       ` Florian Fainelli
2014-09-26 17:32         ` Michael Welling
2014-09-26 20:04         ` Michael Welling
2014-10-07 14:09           ` Linus Walleij
2014-10-07 16:38             ` Michael Welling
2014-10-07 14:04   ` Linus Walleij
2014-10-07 16:30     ` Michael Welling

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).