Re: [PATCH] ARM: dts: add phy-reset property for rk3066a-rayeager emac
On 09/11/17 10:28, Roger Quadros wrote:
> On 08/11/17 11:49, Chris Zhong wrote:
>> Hi Florian Fainelli
>>
>>
>> On 2017年11月08日 02:26, Florian Fainelli wrote:
>>> On 11/07/2017 01:51 AM, Chris Zhong wrote:
>>>>
>>>> On 2017年11月07日 15:54, Vladimir Zapolskiy wrote:
>>>>> Hello Chris,
>>>>>
>>>>> On 11/07/2017 04:49 AM, Chris Zhong wrote:
>>>>>> The ethernet phy of rk3066a-rayeager has a reset pin, it controlled by
>>>>>> GPIO1_D6, this pin should be pull down then pull up to reset the phy.
>>>>>> Add a phy-reset property in emac, make the phy can be reset when emac
>>>>>> power on.
>>>>> for PHY reset there are properties 'reset-gpios' and 'reset-delay-us',
>>>>> please reference to Documentation/devicetree/bindings/net/mdio.txt
>>>>>
>>>>> Can you try to reuse them instead of adding new custom properties?
>>>> This phy-reset is from Documentation/devicetree/bindings/net/arc_emac.txt.
>>>> And copy from arch/arm/boot/dts/rk3036-kylin.dts.
>>>> Can we just use these properties, they are not new.
>>> Because it already exists does not mean it's correct, in fact, it is not
>>> at all because it places the reset property for a PHY into the MAC node,
>>> which is just not what this is, what we should be using instead is the
>>> following patch series:
>>>
>>> http://patchwork.ozlabs.org/project/netdev/list/?series=9267
>>>
>>> http://patchwork.ozlabs.org/patch/828499/
>>> http://patchwork.ozlabs.org/patch/828505/
>>> http://patchwork.ozlabs.org/patch/828501/
>>> http://patchwork.ozlabs.org/patch/828502/
>> Okay, this series works for me, and I will push a new version patch
>> following it,
>> with a reset-gpios property under phy node.
>> And hope this series can be applied soon.
>>
>
> If 'reset-gpios' and 'reset-delay-us' as per [1] works for you
> then why not use that?
>
> [1] Documentation/devicetree/bindings/net/mdio.txt
I take back my statement.
You should use the above only if you have a single reset line connected to
multiple PHYs
on the same MDIO bus.
In all other cases the reset properties should be part of PHY device node.
However, we need to still ensure that the mdio_bus driver issues the reset.
This piece
is still missing in the code.
>
>> Thanks
>> Chris Zhong
>>
>>
>>>>> As a side question, which is mainly addressed to Sergei and Roger,
>>>>> I don't quite understand why PHY properties were initially added to
>>>>> MAC/MDIO bus device tree nodes, in my opinion they must be moved under
>>>>> PHY device tree nodes.
>>>>>
>>>>> --
>>>>> With best wishes,
>>>>> Vladimir
>>>>>
>>>>>> Signed-off-by: Chris Zhong
>>>>>> ---
>>>>>>
>>>>>> arch/arm/boot/dts/rk3066a-rayeager.dts | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts
>>>>>> b/arch/arm/boot/dts/rk3066a-rayeager.dts
>>>>>> index 570157f..6064a0a 100644
>>>>>> --- a/arch/arm/boot/dts/rk3066a-rayeager.dts
>>>>>> +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts
>>>>>> @@ -173,6 +173,8 @@
>>>>>> pinctrl-0 = <&emac_xfer>, <&emac_mdio>, <&rmii_rst>;
>>>>>> phy = <&phy0>;
>>>>>> phy-supply = <&vcc_rmii>;
>>>>>> + phy-reset-gpios = <&gpio1 RK_PD6 GPIO_ACTIVE_LOW>; /* PHY_RST */
>>>>>> + phy-reset-duration = <10>; /* millisecond */
>>>>>> status = "okay";
>>>>>> phy0: ethernet-phy@0 {
>>>>>>
>>>>>
>>>
>>
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] ARM: dts: add phy-reset property for rk3066a-rayeager emac
On 08/11/17 11:49, Chris Zhong wrote:
> Hi Florian Fainelli
>
>
> On 2017年11月08日 02:26, Florian Fainelli wrote:
>> On 11/07/2017 01:51 AM, Chris Zhong wrote:
>>>
>>> On 2017年11月07日 15:54, Vladimir Zapolskiy wrote:
Hello Chris,
On 11/07/2017 04:49 AM, Chris Zhong wrote:
> The ethernet phy of rk3066a-rayeager has a reset pin, it controlled by
> GPIO1_D6, this pin should be pull down then pull up to reset the phy.
> Add a phy-reset property in emac, make the phy can be reset when emac
> power on.
for PHY reset there are properties 'reset-gpios' and 'reset-delay-us',
please reference to Documentation/devicetree/bindings/net/mdio.txt
Can you try to reuse them instead of adding new custom properties?
>>> This phy-reset is from Documentation/devicetree/bindings/net/arc_emac.txt.
>>> And copy from arch/arm/boot/dts/rk3036-kylin.dts.
>>> Can we just use these properties, they are not new.
>> Because it already exists does not mean it's correct, in fact, it is not
>> at all because it places the reset property for a PHY into the MAC node,
>> which is just not what this is, what we should be using instead is the
>> following patch series:
>>
>> http://patchwork.ozlabs.org/project/netdev/list/?series=9267
>>
>> http://patchwork.ozlabs.org/patch/828499/
>> http://patchwork.ozlabs.org/patch/828505/
>> http://patchwork.ozlabs.org/patch/828501/
>> http://patchwork.ozlabs.org/patch/828502/
> Okay, this series works for me, and I will push a new version patch following
> it,
> with a reset-gpios property under phy node.
> And hope this series can be applied soon.
>
If 'reset-gpios' and 'reset-delay-us' as per [1] works for you
then why not use that?
[1] Documentation/devicetree/bindings/net/mdio.txt
> Thanks
> Chris Zhong
>
>
As a side question, which is mainly addressed to Sergei and Roger,
I don't quite understand why PHY properties were initially added to
MAC/MDIO bus device tree nodes, in my opinion they must be moved under
PHY device tree nodes.
--
With best wishes,
Vladimir
> Signed-off-by: Chris Zhong
> ---
>
> arch/arm/boot/dts/rk3066a-rayeager.dts | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts
> b/arch/arm/boot/dts/rk3066a-rayeager.dts
> index 570157f..6064a0a 100644
> --- a/arch/arm/boot/dts/rk3066a-rayeager.dts
> +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts
> @@ -173,6 +173,8 @@
> pinctrl-0 = <&emac_xfer>, <&emac_mdio>, <&rmii_rst>;
> phy = <&phy0>;
> phy-supply = <&vcc_rmii>;
> + phy-reset-gpios = <&gpio1 RK_PD6 GPIO_ACTIVE_LOW>; /* PHY_RST */
> + phy-reset-duration = <10>; /* millisecond */
> status = "okay";
> phy0: ethernet-phy@0 {
>
>>
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH] ARM: dts: add phy-reset property for rk3066a-rayeager emac
Hi,
On 07/11/17 09:54, Vladimir Zapolskiy wrote:
> Hello Chris,
>
> On 11/07/2017 04:49 AM, Chris Zhong wrote:
>> The ethernet phy of rk3066a-rayeager has a reset pin, it controlled by
>> GPIO1_D6, this pin should be pull down then pull up to reset the phy.
>> Add a phy-reset property in emac, make the phy can be reset when emac
>> power on.
>
> for PHY reset there are properties 'reset-gpios' and 'reset-delay-us',
> please reference to Documentation/devicetree/bindings/net/mdio.txt
>
> Can you try to reuse them instead of adding new custom properties?
>
> As a side question, which is mainly addressed to Sergei and Roger,
> I don't quite understand why PHY properties were initially added to
> MAC/MDIO bus device tree nodes, in my opinion they must be moved under
> PHY device tree nodes.
The PHY reset *has* to be performed at the MDIO bus driver level so that they
can be pin-strap configured correctly and be detected during MDIO bus scan.
- We have boards where PHYs won't be detected if RESET is not done before the
scan. (due to invalid h/w pin-strapping config)
- We have boards where a single GPIO line controls reset of all PHYs on the bus.
Due to these reasons the RESET control lies with the MDIO bus driver.
>
> --
> With best wishes,
> Vladimir
>
>>
>> Signed-off-by: Chris Zhong
>> ---
>>
>> arch/arm/boot/dts/rk3066a-rayeager.dts | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/rk3066a-rayeager.dts
>> b/arch/arm/boot/dts/rk3066a-rayeager.dts
>> index 570157f..6064a0a 100644
>> --- a/arch/arm/boot/dts/rk3066a-rayeager.dts
>> +++ b/arch/arm/boot/dts/rk3066a-rayeager.dts
>> @@ -173,6 +173,8 @@
>> pinctrl-0 = <&emac_xfer>, <&emac_mdio>, <&rmii_rst>;
>> phy = <&phy0>;
>> phy-supply = <&vcc_rmii>;
>> +phy-reset-gpios = <&gpio1 RK_PD6 GPIO_ACTIVE_LOW>; /* PHY_RST */
>> +phy-reset-duration = <10>; /* millisecond */
>> status = "okay";
>>
>> phy0: ethernet-phy@0 {
>>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Re: [PATCH net-next] dt-bindings: mdio: Clarify binding document
On 25/04/17 21:33, Florian Fainelli wrote:
> The described GPIO reset property is applicable to *all* child PHYs. If
> we have one reset line per PHY present on the MDIO bus, these
> automatically become properties of the child PHY nodes.
>
> Finally, indicate how the RESET pulse width must be defined, which is
> the maximum value of all individual PHYs RESET pulse widths determined
> by reading their datasheets.
>
> Fixes: 69226896ad63 ("mdio_bus: Issue GPIO RESET to PHYs.")
> Signed-off-by: Florian Fainelli
Reviewed-by: Roger Quadros
> ---
> Documentation/devicetree/bindings/net/mdio.txt | 12
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/mdio.txt
> b/Documentation/devicetree/bindings/net/mdio.txt
> index 4ffbbacebda1..96a53f89aa6e 100644
> --- a/Documentation/devicetree/bindings/net/mdio.txt
> +++ b/Documentation/devicetree/bindings/net/mdio.txt
> @@ -3,13 +3,17 @@ Common MDIO bus properties.
> These are generic properties that can apply to any MDIO bus.
>
> Optional properties:
> -- reset-gpios: List of one or more GPIOs that control the RESET lines
> - of the PHYs on that MDIO bus.
> -- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
> +- reset-gpios: One GPIO that control the RESET lines of all PHYs on that MDIO
> + bus.
> +- reset-delay-us: RESET pulse width in microseconds.
>
> A list of child nodes, one per device on the bus is expected. These
> should follow the generic phy.txt, or a device specific binding document.
>
> +The 'reset-delay-us' indicates the RESET signal pulse width in microseconds
> and
> +applies to all PHY devices. It must therefore be appropriately determined
> based
> +on all PHY requirements (maximum value of all per-PHY RESET pulse widths).
> +
> Example :
> This example shows these optional properties, plus other properties
> required for the TI Davinci MDIO driver.
> @@ -21,7 +25,7 @@ required for the TI Davinci MDIO driver.
> #size-cells = <0>;
>
> reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> - reset-delay-us = <2>; /* PHY datasheet states 1us min */
> + reset-delay-us = <2>;
>
> ethphy0: ethernet-phy@1 {
> reg = <1>;
>
--
cheers,
-roger
Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.
On 25/04/17 19:31, Florian Fainelli wrote:
> On 04/25/2017 09:22 AM, Lars-Peter Clausen wrote:
>> On 04/24/2017 11:04 AM, Roger Quadros wrote:
>>> On 24/04/17 02:35, Andrew Lunn wrote:
>>>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt
>>>>>> b/Documentation/devicetree/bindings/net/mdio.txt
>>>>>> new file mode 100644
>>>>>> index 000..4ffbbac
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>>>> @@ -0,0 +1,33 @@
>>>>>> +Common MDIO bus properties.
>>>>>> +
>>>>>> +These are generic properties that can apply to any MDIO bus.
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>>>> + of the PHYs on that MDIO bus.
>>>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY
>>>>>> datasheet.
>>>>>> +
>>>>>> +A list of child nodes, one per device on the bus is expected. These
>>>>>> +should follow the generic phy.txt, or a device specific binding
>>>>>> document.
>>>>>> +
>>>>>> +Example :
>>>>>> +This example shows these optional properties, plus other properties
>>>>>> +required for the TI Davinci MDIO driver.
>>>>>> +
>>>>>> +davinci_mdio: ethernet@0x5c03 {
>>>>>> +compatible = "ti,davinci_mdio";
>>>>>> +reg = <0x5c03 0x1000>;
>>>>>> +#address-cells = <1>;
>>>>>> +#size-cells = <0>;
>>>>>> +
>>>>>> +reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>>>> +reset-delay-us = <2>; /* PHY datasheet states 1us min
>>>>>> */
>>>>>
>>>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>>>> node rather than of the MDIO controller node (which might have a reset on
>>>>> its own)?
>>>>>> +
>>>>>> +ethphy0: ethernet-phy@1 {
>>>>>> +reg = <1>;
>>>>>> +};
>>>>>> +
>>>>>> +ethphy1: ethernet-phy@3 {
>>>>>> +reg = <3>;
>>>>>> +};
>>>>
>>>> Hi Lars-Peter
>>>>
>>>> We discussed this when the first proposal was made. There are two
>>>> cases, to consider.
>>>>
>>>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>>>> example, two PHYs.
>>>>
>>>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>>>> say, the reset line should probably be considered a PHY property, not
>>>> an MDIO property. However, it can be messy, since in order to probe
>>>> the MDIO bus, you probably need to take the PHY out of reset.
>>>>
>>
>> But the DT binding documentation says something else "List of one or more
>> GPIOs that control the RESET lines of the PHYs on that MDIO bus".
>
> I agree, it should be defined more strictly as:
>
> "One GPIO that controls the reset line of *all* PHYs populated on that
> MDIO bus"
Patch is already in net-next. How can we get this fixed? Should I send a v5?
>
> If there are separate lines, these automatically become properties of
> the PHY nodes.
>
>>
>>>> Anyway, this patch addresses the first case, so should be accepted. If
>>>> anybody wants to address the second case, they are free to do so.
>>
>> I think we all know that that's not going to happen. Once there is a working
>> kludge there is no incentive to do a proper implementation anymore.
>>
>>
>>> Thanks for the explanation Andrew.
>>>
>>> For the second case, even if the RESET GPIO property is specified
>>> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
>>> else the PHY might not be probed at all.
>>
>> I'm not arguing wi
Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.
On 25/04/17 19:22, Lars-Peter Clausen wrote:
> On 04/24/2017 11:04 AM, Roger Quadros wrote:
>> On 24/04/17 02:35, Andrew Lunn wrote:
>>> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>>>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt
>>>>> b/Documentation/devicetree/bindings/net/mdio.txt
>>>>> new file mode 100644
>>>>> index 000..4ffbbac
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>>>> @@ -0,0 +1,33 @@
>>>>> +Common MDIO bus properties.
>>>>> +
>>>>> +These are generic properties that can apply to any MDIO bus.
>>>>> +
>>>>> +Optional properties:
>>>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>>>> + of the PHYs on that MDIO bus.
>>>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>>>> +
>>>>> +A list of child nodes, one per device on the bus is expected. These
>>>>> +should follow the generic phy.txt, or a device specific binding document.
>>>>> +
>>>>> +Example :
>>>>> +This example shows these optional properties, plus other properties
>>>>> +required for the TI Davinci MDIO driver.
>>>>> +
>>>>> + davinci_mdio: ethernet@0x5c03 {
>>>>> + compatible = "ti,davinci_mdio";
>>>>> + reg = <0x5c03 0x1000>;
>>>>> + #address-cells = <1>;
>>>>> + #size-cells = <0>;
>>>>> +
>>>>> + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>>>> + reset-delay-us = <2>; /* PHY datasheet states 1us min */
>>>>
>>>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>>>> node rather than of the MDIO controller node (which might have a reset on
>>>> its own)?
>>>>> +
>>>>> + ethphy0: ethernet-phy@1 {
>>>>> + reg = <1>;
>>>>> + };
>>>>> +
>>>>> + ethphy1: ethernet-phy@3 {
>>>>> + reg = <3>;
>>>>> + };
>>>
>>> Hi Lars-Peter
>>>
>>> We discussed this when the first proposal was made. There are two
>>> cases, to consider.
>>>
>>> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
>>> example, two PHYs.
>>>
>>> 2) There is one GPIO line per PHY. That is a separate case, and as you
>>> say, the reset line should probably be considered a PHY property, not
>>> an MDIO property. However, it can be messy, since in order to probe
>>> the MDIO bus, you probably need to take the PHY out of reset.
>>>
>
> But the DT binding documentation says something else "List of one or more
> GPIOs that control the RESET lines of the PHYs on that MDIO bus".
>
>>> Anyway, this patch addresses the first case, so should be accepted. If
>>> anybody wants to address the second case, they are free to do so.
>
> I think we all know that that's not going to happen. Once there is a working
> kludge there is no incentive to do a proper implementation anymore.
>
>
>> Thanks for the explanation Andrew.
>>
>> For the second case, even if the RESET GPIO property is specified
>> in the PHY node, the RESET *will* have to be done by the MDIO bus driver
>> else the PHY might not be probed at all.
>
> I'm not arguing with that, just that the hardware description should be
> truthful to the hardware topology and not to the software topology, i.e. the
> implementation details of the Linux kernel in this case. Reset GPIOs are not
> the only resource that is connected to the PHY that needs to be enabled
> before they can be enumerated. E.g. clocks and regulators fall into the same
> realm. And while you might argue that with a on-SoC phy controller node
> there wont be any conflicts in regard to the reset-gpios property, this not
> so very true for the clocks property.
>
> And MDIO is not really special in this regard, other discoverable buses
> (like USB, SDIO, ULPI) have the very same issue. Having a standardized
> binding approach where the resources are declared as part as the child child
> is preferable in my opinion.
Good point. I agree now that if PHYs have individual RESET lines, they
should be part of the PHY node.
>
>>
>> Whether we need additional code to just to make the DT look prettier is
>> questionable and if required can come as a separate patch.
>
> Unfortunately not, once it is merged it can't be changed anymore.
>
cheers,
-roger
Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.
On 24/04/17 02:35, Andrew Lunn wrote:
> On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
>> On 04/21/2017 03:15 PM, Roger Quadros wrote:
>>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt
>>> b/Documentation/devicetree/bindings/net/mdio.txt
>>> new file mode 100644
>>> index 000..4ffbbac
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>>> @@ -0,0 +1,33 @@
>>> +Common MDIO bus properties.
>>> +
>>> +These are generic properties that can apply to any MDIO bus.
>>> +
>>> +Optional properties:
>>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>>> + of the PHYs on that MDIO bus.
>>> +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
>>> +
>>> +A list of child nodes, one per device on the bus is expected. These
>>> +should follow the generic phy.txt, or a device specific binding document.
>>> +
>>> +Example :
>>> +This example shows these optional properties, plus other properties
>>> +required for the TI Davinci MDIO driver.
>>> +
>>> + davinci_mdio: ethernet@0x5c03 {
>>> + compatible = "ti,davinci_mdio";
>>> + reg = <0x5c03 0x1000>;
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>>> + reset-delay-us = <2>; /* PHY datasheet states 1us min */
>>
>> If this is the reset line of the PHY shouldn't it be a property of the PHY
>> node rather than of the MDIO controller node (which might have a reset on
>> its own)?
>>> +
>>> + ethphy0: ethernet-phy@1 {
>>> + reg = <1>;
>>> + };
>>> +
>>> + ethphy1: ethernet-phy@3 {
>>> + reg = <3>;
>>> + };
>
> Hi Lars-Peter
>
> We discussed this when the first proposal was made. There are two
> cases, to consider.
>
> 1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
> example, two PHYs.
>
> 2) There is one GPIO line per PHY. That is a separate case, and as you
> say, the reset line should probably be considered a PHY property, not
> an MDIO property. However, it can be messy, since in order to probe
> the MDIO bus, you probably need to take the PHY out of reset.
>
> Anyway, this patch addresses the first case, so should be accepted. If
> anybody wants to address the second case, they are free to do so.
Thanks for the explanation Andrew.
For the second case, even if the RESET GPIO property is specified
in the PHY node, the RESET *will* have to be done by the MDIO bus driver
else the PHY might not be probed at all.
Whether we need additional code to just to make the DT look prettier is
questionable and if required can come as a separate patch.
cheers,
-roger
Re: [PATCH v1] net: phy: fix auto-negotiation stall due to unavailable interrupt
On 20/04/17 14:00, Alexander Kochetkov wrote:
> The Ethernet link on an interrupt driven PHY was not coming up if the Ethernet
> cable was plugged before the Ethernet interface was brought up.
>
> The patch trigger PHY state machine to update link state if PHY was requested
> to
> do auto-negotiation and auto-negotiation complete flag already set.
>
> During power-up cycle the PHY do auto-negotiation, generate interrupt and set
> auto-negotiation complete flag. Interrupt is handled by PHY state machine but
> doesn't update link state because PHY is in PHY_READY state. After some time
> MAC bring up, start and request PHY to do auto-negotiation. If there are no
> new
> settings to advertise genphy_config_aneg() doesn't start PHY auto-negotiation.
> PHY continue to stay in auto-negotiation complete state and doesn't fire
> interrupt. At the same time PHY state machine expect that PHY started
> auto-negotiation and is waiting for interrupt from PHY and it won't get it.
>
> Signed-off-by: Alexander Kochetkov
> Cc: stable # v4.9+
Tested-by: Roger Quadros
I think the following commit broke functionality with interrupt driven PHYs
3c293f4e08b5 ("net: phy: Trigger state machine on state change and not
polling.")
cheers,
-roger
> ---
> drivers/net/phy/phy.c | 40
> include/linux/phy.h |1 +
> 2 files changed, 37 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7cc1b7d..2d9975b 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -591,16 +591,18 @@ int phy_mii_ioctl(struct phy_device *phydev, struct
> ifreq *ifr, int cmd)
> EXPORT_SYMBOL(phy_mii_ioctl);
>
> /**
> - * phy_start_aneg - start auto-negotiation for this PHY device
> + * phy_start_aneg_priv - start auto-negotiation for this PHY device
> * @phydev: the phy_device struct
> + * @sync: indicate whether we should wait for the workqueue cancelation
> *
> * Description: Sanitizes the settings (if we're not autonegotiating
> * them), and then calls the driver's config_aneg function.
> * If the PHYCONTROL Layer is operating, we change the state to
> * reflect the beginning of Auto-negotiation or forcing.
> */
> -int phy_start_aneg(struct phy_device *phydev)
> +static int phy_start_aneg_priv(struct phy_device *phydev, bool sync)
> {
> + bool trigger = 0;
> int err;
>
> mutex_lock(&phydev->lock);
> @@ -625,10 +627,40 @@ int phy_start_aneg(struct phy_device *phydev)
> }
> }
>
> + /* Re-schedule a PHY state machine to check PHY status because
> + * negotiation may already be done and aneg interrupt may not be
> + * generated.
> + */
> + if (phy_interrupt_is_valid(phydev) && (phydev->state == PHY_AN)) {
> + err = phy_aneg_done(phydev);
> + if (err > 0) {
> + trigger = true;
> + err = 0;
> + }
> + }
> +
> out_unlock:
> mutex_unlock(&phydev->lock);
> +
> + if (trigger)
> + phy_trigger_machine(phydev, sync);
> +
> return err;
> }
> +
> +/**
> + * phy_start_aneg - start auto-negotiation for this PHY device
> + * @phydev: the phy_device struct
> + *
> + * Description: Sanitizes the settings (if we're not autonegotiating
> + * them), and then calls the driver's config_aneg function.
> + * If the PHYCONTROL Layer is operating, we change the state to
> + * reflect the beginning of Auto-negotiation or forcing.
> + */
> +int phy_start_aneg(struct phy_device *phydev)
> +{
> + return phy_start_aneg_priv(phydev, true);
> +}
> EXPORT_SYMBOL(phy_start_aneg);
>
> /**
> @@ -656,7 +688,7 @@ void phy_start_machine(struct phy_device *phydev)
> * state machine runs.
> */
>
> -static void phy_trigger_machine(struct phy_device *phydev, bool sync)
> +void phy_trigger_machine(struct phy_device *phydev, bool sync)
> {
> if (sync)
> cancel_delayed_work_sync(&phydev->state_queue);
> @@ -1151,7 +1183,7 @@ void phy_state_machine(struct work_struct *work)
> mutex_unlock(&phydev->lock);
>
> if (needs_aneg)
> - err = phy_start_aneg(phydev);
> + err = phy_start_aneg_priv(phydev, false);
> else if (do_suspend)
> phy_suspend(phydev);
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 7fc1105..b19ae66 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -840,6 +840,7 @@ int phy_drivers_register(struct ph
[PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.
Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.
Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.
In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.
[1] - am572x-idk, am571x-idk, a437x-idk
Signed-off-by: Roger Quadros
---
v4:
- use dev_err() instead of pr_err()
- put PHYs back in RESET on failure or mdiobus_unregister()
- typo fixes uS -> us
v3:
- added more information in the DT binding document.
v2:
- add device tree binding document (mdio.txt)
- specify default reset delay in of_mdio.c instead of mdio_bus.c
Documentation/devicetree/bindings/net/mdio.txt | 33 ++
drivers/net/phy/mdio_bus.c | 47 ++
drivers/of/of_mdio.c | 7
include/linux/phy.h| 7
4 files changed, 94 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
diff --git a/Documentation/devicetree/bindings/net/mdio.txt
b/Documentation/devicetree/bindings/net/mdio.txt
new file mode 100644
index 000..4ffbbac
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio.txt
@@ -0,0 +1,33 @@
+Common MDIO bus properties.
+
+These are generic properties that can apply to any MDIO bus.
+
+Optional properties:
+- reset-gpios: List of one or more GPIOs that control the RESET lines
+ of the PHYs on that MDIO bus.
+- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
+
+A list of child nodes, one per device on the bus is expected. These
+should follow the generic phy.txt, or a device specific binding document.
+
+Example :
+This example shows these optional properties, plus other properties
+required for the TI Davinci MDIO driver.
+
+ davinci_mdio: ethernet@0x5c03 {
+ compatible = "ti,davinci_mdio";
+ reg = <0x5c03 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+ reset-delay-us = <2>; /* PHY datasheet states 1us min */
+
+ ethphy0: ethernet-phy@1 {
+ reg = <1>;
+ };
+
+ ethphy1: ethernet-phy@3 {
+ reg = <3>;
+ };
+ };
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..de927f7 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
#include
#include
#include
+#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
{
struct mdio_device *mdiodev;
int i, err;
+ struct gpio_desc *gpiod;
if (NULL == bus || NULL == bus->name ||
NULL == bus->read || NULL == bus->write)
@@ -333,6 +337,35 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
if (bus->reset)
bus->reset(bus);
+ /* de-assert bus level PHY GPIO resets */
+ if (bus->num_reset_gpios > 0) {
+ bus->reset_gpiod = devm_kcalloc(&bus->dev,
+bus->num_reset_gpios,
+sizeof(struct gpio_desc *),
+GFP_KERNEL);
+ if (!bus->reset_gpiod)
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < bus->num_reset_gpios; i++) {
+ gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ if (err != -ENOENT) {
+ dev_err(&bus->dev,
+ "mii_bus %s couldn't get reset GPIO\n",
+ bus->id);
+ return err;
+ }
+ } else {
+ bus->reset_gpiod[i] = gpiod;
+ gpiod_set_value_cansleep(gpiod, 1);
+ udelay(bus->reset_delay_us);
+ gpiod_set_value_cansleep(gpiod, 0);
+ }
+ }
+
for (i = 0; i < PHY_MAX_ADDR; i++) {
if ((bus->phy_m
Re: [PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.
Hi Florian,
On 21/04/17 04:23, Florian Fainelli wrote:
> Hi Roger,
>
> On 04/20/2017 07:11 AM, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
>>
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros
>
> A few comments on the binding and the code, sorry for this late review.
No problem at all.
>
>> +Example :
>> +This example shows these optional properties, plus other properties
>> +required for the TI Davinci MDIO driver.
>> +
>> +davinci_mdio: ethernet@0x5c03 {
>> +compatible = "ti,davinci_mdio";
>> +reg = <0x5c03 0x1000>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +
>> +reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>> +reset-delay-us = <2>; /* PHY datasheet states 1uS min */
>
> us is micro seconds, uS is micro siemens.
OK.
>
>> +
>> +ethphy0: ethernet-phy@1 {
>> +reg = <1>;
>> +};
>> +
>> +ethphy1: ethernet-phy@3 {
>> +reg = <3>;
>> +};
>> +};
>
>>
>> +/* de-assert bus level PHY GPIO resets */
>> +for (i = 0; i < bus->num_reset_gpios; i++) {
>> +gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
>> + GPIOD_OUT_LOW);
>> +if (IS_ERR(gpiod)) {
>> +err = PTR_ERR(gpiod);
>> +if (err != -ENOENT) {
>> +pr_err("mii_bus %s couldn't get reset GPIO\n",
>> + bus->id);
>
> Could we use dev_err(&bus->dev) here to better identify which MDIO bus
> is returning the problem?
Sure.
>
>> +return err;
>
> Should we somehow "unwind" the reset lines we were able to successfully
> take out of reset and therefore put back into reset state? How about
> mdiobus_unregister()? Should we have similar code there, if not for
> correctness to be more power efficient?
Al right.
>
>> +}
>> +} else {
>> +gpiod_set_value_cansleep(gpiod, 1);
>> +udelay(bus->reset_delay_us);
>> +gpiod_set_value_cansleep(gpiod, 0);
>
> Does that work even if the polarity of the reset line is active low?
>
Yes. The polarity needs to be specified at DT as explained by Andrew already.
cheers,
-roger
[PATCH v3 net-next] mdio_bus: Issue GPIO RESET to PHYs.
Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.
Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.
In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.
[1] - am572x-idk, am571x-idk, a437x-idk
Signed-off-by: Roger Quadros
---
v3:
- added more information in the DT binding document.
v2:
- add device tree binding document (mdio.txt)
- specify default reset delay in of_mdio.c instead of mdio_bus.c
Documentation/devicetree/bindings/net/mdio.txt | 33 ++
drivers/net/phy/mdio_bus.c | 22 +
drivers/of/of_mdio.c | 7 ++
include/linux/phy.h| 5
4 files changed, 67 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
diff --git a/Documentation/devicetree/bindings/net/mdio.txt
b/Documentation/devicetree/bindings/net/mdio.txt
new file mode 100644
index 000..3517369
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio.txt
@@ -0,0 +1,33 @@
+Common MDIO bus properties.
+
+These are generic properties that can apply to any MDIO bus.
+
+Optional properties:
+- reset-gpios: List of one or more GPIOs that control the RESET lines
+ of the PHYs on that MDIO bus.
+- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
+
+A list of child nodes, one per device on the bus is expected. These
+should follow the generic phy.txt, or a device specific binding document.
+
+Example :
+This example shows these optional properties, plus other properties
+required for the TI Davinci MDIO driver.
+
+ davinci_mdio: ethernet@0x5c03 {
+ compatible = "ti,davinci_mdio";
+ reg = <0x5c03 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+ reset-delay-us = <2>; /* PHY datasheet states 1uS min */
+
+ ethphy0: ethernet-phy@1 {
+ reg = <1>;
+ };
+
+ ethphy1: ethernet-phy@3 {
+ reg = <3>;
+ };
+ };
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..b353d99 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
#include
#include
#include
+#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
{
struct mdio_device *mdiodev;
int i, err;
+ struct gpio_desc *gpiod;
if (NULL == bus || NULL == bus->name ||
NULL == bus->read || NULL == bus->write)
@@ -333,6 +337,24 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
if (bus->reset)
bus->reset(bus);
+ /* de-assert bus level PHY GPIO resets */
+ for (i = 0; i < bus->num_reset_gpios; i++) {
+ gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ if (err != -ENOENT) {
+ pr_err("mii_bus %s couldn't get reset GPIO\n",
+ bus->id);
+ return err;
+ }
+ } else {
+ gpiod_set_value_cansleep(gpiod, 1);
+ udelay(bus->reset_delay_us);
+ gpiod_set_value_cansleep(gpiod, 0);
+ }
+ }
+
for (i = 0; i < PHY_MAX_ADDR; i++) {
if ((bus->phy_mask & (1 << i)) == 0) {
struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..7e4c80f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,8 @@
#include
#include
+#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
+
MODULE_AUTHOR("Grant Likely ");
MODULE_LICENSE("GPL");
@@ -221,6 +223,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct
device_node *np)
mdio->dev.of_node = np;
+ /* Get bus level PHY reset GPIO details */
+ mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+ of
Re: [PATCH v2] mdio_bus: Issue GPIO RESET to PHYs.
On 20/04/17 16:13, Andrew Lunn wrote:
> On Thu, Apr 20, 2017 at 11:39:20AM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
>
> Hi Roger
>
> Networking patches should include in the subject line which tree they
> are for. In this case, net-next. So please make the Subject
>
> [patch v3 net-next] .
>
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros
>> ---
>> v2:
>> - add device tree binding document (mdio.txt)
>> - specify default reset delay in of_mdio.c instead of mdio_bus.c
>>
>> Documentation/devicetree/bindings/net/mdio.txt | 20
>> drivers/net/phy/mdio_bus.c | 22 ++
>> drivers/of/of_mdio.c | 7 +++
>> include/linux/phy.h| 5 +
>> 4 files changed, 54 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/mdio.txt
>> b/Documentation/devicetree/bindings/net/mdio.txt
>> new file mode 100644
>> index 000..6e703d7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/mdio.txt
>> @@ -0,0 +1,20 @@
>> +Common MDIO bus properties.
>> +
>> +These are generic properties that can apply to any MDIO bus.
>> +
>> +Optional properties:
>> +- reset-gpios: List of one or more GPIOs that control the RESET lines
>> + of the PHYs on that MDIO bus.
>> +- reset-delay-us: RESET pulse width as per PHY datasheet.
>
> It would be good to explicitly say that it is in uS as part of the
> comment.
>
> Also, please document that we expect a list of child nodes, one per
> device on the bus. These should follow the generic phy.txt, or a
> device specific binding document.
>
>> +
>> +Example :
>> +
>
> It would be good to say something like:
>
> This example shows these optional properties, plus other properties
> required for the TI Davinci MDIO driver.
>
> Pointing this out may stop people cut/past the ti,hwmods property.
>
>> +davinci_mdio: ethernet@0x5c03 {
>> +compatible = "ti,davinci_mdio";
>> +ti,hwmods = "davinci_mdio";
>> +reg = <0x5c03 0x1000>;
>> +#address-cells = <1>;
>> +#size-cells = <0>;
>> +reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
>> +reset-delay-us = <2>; /* PHY datasheet states 1uS min */
>> +};
>
> And please include at least one PHY on the bus.
>
> Sorry for asking for so much in the documentation. That is the problem
> with the documentation being missing to start with.
>
> The code looks good now.
>
OK Andrew. I'll post a v3 with your suggestions.
cheers,
-roger
Re: [PATCH] net: phy: fix auto-negotiation stall due to unavailable interrupt
Hi,
On 19/04/17 19:53, Alexander Kochetkov wrote:
>
>> 19 апр. 2017 г., в 19:32, Florian Fainelli написал(а):
>>
>> http://patchwork.ozlabs.org/patch/743773/
>>
>> Roger can you also test Alexander's patch?
This patch fixes my problem and doesn't have the 1 second delay that my patch
had.
So,
Tested-by: Roger Quadros
>
> If MAC use phy_start_aneg() instead of phy_start() my patch will not work as
> expected. Roger, if patch don’t work for you please check what MAC bring up
> PHY using
> phy_start():
Our MAC driver is using phy_start() and I didn't face any issues with your
patch.
>
> http://patchwork.ozlabs.org/patch/752308/
>
> Is it correct to start PHY inside MAC probe using phy_start_aneg()? Or
> phy_start() must be used?
>
> And probably this tags should be added for my patch:
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not
> polling.")
> Cc: stable # v4.9+
>
> Because I bisected to commit 529ed1275263 ("net: phy: phy drivers
> should not set SUPPORTED_[Asym_]Pause») that looks pretty good.
>
> Also, there is another issue I found. link_timeout doesn’t work for interrupt
> driven PHY.
> It is possible to implement timer to handle this case.
> Florian, what do you think? Should this be fixed?
>
> Alexander.
>
cheers,
-roger
[PATCH v2] mdio_bus: Issue GPIO RESET to PHYs.
Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.
Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.
In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.
[1] - am572x-idk, am571x-idk, a437x-idk
Signed-off-by: Roger Quadros
---
v2:
- add device tree binding document (mdio.txt)
- specify default reset delay in of_mdio.c instead of mdio_bus.c
Documentation/devicetree/bindings/net/mdio.txt | 20
drivers/net/phy/mdio_bus.c | 22 ++
drivers/of/of_mdio.c | 7 +++
include/linux/phy.h| 5 +
4 files changed, 54 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/mdio.txt
diff --git a/Documentation/devicetree/bindings/net/mdio.txt
b/Documentation/devicetree/bindings/net/mdio.txt
new file mode 100644
index 000..6e703d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/mdio.txt
@@ -0,0 +1,20 @@
+Common MDIO bus properties.
+
+These are generic properties that can apply to any MDIO bus.
+
+Optional properties:
+- reset-gpios: List of one or more GPIOs that control the RESET lines
+ of the PHYs on that MDIO bus.
+- reset-delay-us: RESET pulse width as per PHY datasheet.
+
+Example :
+
+ davinci_mdio: ethernet@0x5c03 {
+ compatible = "ti,davinci_mdio";
+ ti,hwmods = "davinci_mdio";
+ reg = <0x5c03 0x1000>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+ reset-delay-us = <2>; /* PHY datasheet states 1uS min */
+ };
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..b353d99 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
#include
#include
#include
+#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -307,6 +310,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
{
struct mdio_device *mdiodev;
int i, err;
+ struct gpio_desc *gpiod;
if (NULL == bus || NULL == bus->name ||
NULL == bus->read || NULL == bus->write)
@@ -333,6 +337,24 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
if (bus->reset)
bus->reset(bus);
+ /* de-assert bus level PHY GPIO resets */
+ for (i = 0; i < bus->num_reset_gpios; i++) {
+ gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ if (err != -ENOENT) {
+ pr_err("mii_bus %s couldn't get reset GPIO\n",
+ bus->id);
+ return err;
+ }
+ } else {
+ gpiod_set_value_cansleep(gpiod, 1);
+ udelay(bus->reset_delay_us);
+ gpiod_set_value_cansleep(gpiod, 0);
+ }
+ }
+
for (i = 0; i < PHY_MAX_ADDR; i++) {
if ((bus->phy_mask & (1 << i)) == 0) {
struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..7e4c80f 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -22,6 +22,8 @@
#include
#include
+#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
+
MODULE_AUTHOR("Grant Likely ");
MODULE_LICENSE("GPL");
@@ -221,6 +223,11 @@ int of_mdiobus_register(struct mii_bus *mdio, struct
device_node *np)
mdio->dev.of_node = np;
+ /* Get bus level PHY reset GPIO details */
+ mdio->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+ of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+ mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
/* Register the MDIO bus */
rc = mdiobus_register(mdio);
if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..80a6574 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,11 @@ struct mii_bus {
* matching its address
*/
Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.
On 19/04/17 16:38, Andrew Lunn wrote:
> On Wed, Apr 19, 2017 at 02:56:48PM +0300, Roger Quadros wrote:
>> Hi,
>>
>> On 19/04/17 14:39, Andrew Lunn wrote:
>>> On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
>>>> Some boards [1] leave the PHYs at an invalid state
>>>> during system power-up or reset thus causing unreliability
>>>> issues with the PHY which manifests as PHY not being detected
>>>> or link not functional. To fix this, these PHYs need to be RESET
>>>> via a GPIO connected to the PHY's RESET pin.
>>>>
>>>> Some boards have a single GPIO controlling the PHY RESET pin of all
>>>> PHYs on the bus whereas some others have separate GPIOs controlling
>>>> individual PHY RESETs.
>>>>
>>>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>>>> as the PHY will not probe till its reset is de-asserted.
>>>> So do the RESET de-assertion in the MDIO bus driver.
>>>>
>>>> [1] - am572x-idk, am571x-idk, a437x-idk
>>>>
>>>> Signed-off-by: Roger Quadros
>>>> ---
>>>> drivers/net/phy/mdio_bus.c | 26 ++
>>>> drivers/of/of_mdio.c | 4
>>>> include/linux/phy.h| 5 +
>>>> 3 files changed, 35 insertions(+)
>>>
>>> Hi Roger
>>>
>>> Thanks for making this generic.
>>>
>>> Please add device tree binding documentation. I think that actually
>>> means you have to document MDIO in general, since there currently is
>>> not a binding document.
>>
>> OK.
>>
>>>
>>>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>>>> index fa7d51f..25fda2b 100644
>>>> --- a/drivers/net/phy/mdio_bus.c
>>>> +++ b/drivers/net/phy/mdio_bus.c
>>>> @@ -22,8 +22,11 @@
>>>> #include
>>>> #include
>>>> #include
>>>> +#include
>>>> +#include
>>>> #include
>>>> #include
>>>> +#include
>>>> #include
>>>> #include
>>>> #include
>>>> @@ -43,6 +46,8 @@
>>>>
>>>> #include "mdio-boardinfo.h"
>>>>
>>>> +#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
>>>> +
>>>> int mdiobus_register_device(struct mdio_device *mdiodev)
>>>> {
>>>>if (mdiodev->bus->mdio_map[mdiodev->addr])
>>>> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct
>>>> module *owner)
>>>> {
>>>>struct mdio_device *mdiodev;
>>>>int i, err;
>>>> + struct gpio_desc *gpiod;
>>>>
>>>>if (NULL == bus || NULL == bus->name ||
>>>>NULL == bus->read || NULL == bus->write)
>>>> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct
>>>> module *owner)
>>>>if (bus->reset)
>>>>bus->reset(bus);
>>>>
>>>> + /* de-assert bus level PHY GPIO resets */
>>>> + for (i = 0; i < bus->num_reset_gpios; i++) {
>>>> + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
>>>> + GPIOD_OUT_LOW);
>>>> + if (IS_ERR(gpiod)) {
>>>> + err = PTR_ERR(gpiod);
>>>> + if (err != -ENOENT) {
>>>> + pr_err("mii_bus %s couldn't get reset GPIO\n",
>>>> + bus->id);
>>>> + return err;
>>>> + }
>>>> + } else {
>>>> + gpiod_set_value_cansleep(gpiod, 1);
>>>
>>>
>>>> + if (!bus->reset_delay_us)
>>>> + bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
>>>
>>> Maybe do this once, where you read the device tree property.
>>
>> I was thinking from point of view that GPIO RESET code should work even
>> without
>> device tree. Although I'm not sure if there would be any users or not.
>
> Hi Roger
>
> I don't see how this would work. What would devm_gpiod_get_index()
> return? Something from ACPI? But then there would be something
Yes or just lookup tables based on platform data.
> equivalent for getting the delay.
I'm not sure about delay part.
>
> Lets keep it simple and OF only. If there is a real need for something
> in addition to OF, it can be added later.
OK. I'll revise the patch based on your comments.
cheers,
-roger
Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs.
Hi,
On 19/04/17 14:39, Andrew Lunn wrote:
> On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY which manifests as PHY not being detected
>> or link not functional. To fix this, these PHYs need to be RESET
>> via a GPIO connected to the PHY's RESET pin.
>>
>> Some boards have a single GPIO controlling the PHY RESET pin of all
>> PHYs on the bus whereas some others have separate GPIOs controlling
>> individual PHY RESETs.
>>
>> In both cases, the RESET de-assertion cannot be done in the PHY driver
>> as the PHY will not probe till its reset is de-asserted.
>> So do the RESET de-assertion in the MDIO bus driver.
>>
>> [1] - am572x-idk, am571x-idk, a437x-idk
>>
>> Signed-off-by: Roger Quadros
>> ---
>> drivers/net/phy/mdio_bus.c | 26 ++
>> drivers/of/of_mdio.c | 4
>> include/linux/phy.h| 5 +
>> 3 files changed, 35 insertions(+)
>
> Hi Roger
>
> Thanks for making this generic.
>
> Please add device tree binding documentation. I think that actually
> means you have to document MDIO in general, since there currently is
> not a binding document.
OK.
>
>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
>> index fa7d51f..25fda2b 100644
>> --- a/drivers/net/phy/mdio_bus.c
>> +++ b/drivers/net/phy/mdio_bus.c
>> @@ -22,8 +22,11 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -43,6 +46,8 @@
>>
>> #include "mdio-boardinfo.h"
>>
>> +#define DEFAULT_GPIO_RESET_DELAY10 /* in microseconds */
>> +
>> int mdiobus_register_device(struct mdio_device *mdiodev)
>> {
>> if (mdiodev->bus->mdio_map[mdiodev->addr])
>> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct
>> module *owner)
>> {
>> struct mdio_device *mdiodev;
>> int i, err;
>> +struct gpio_desc *gpiod;
>>
>> if (NULL == bus || NULL == bus->name ||
>> NULL == bus->read || NULL == bus->write)
>> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct
>> module *owner)
>> if (bus->reset)
>> bus->reset(bus);
>>
>> +/* de-assert bus level PHY GPIO resets */
>> +for (i = 0; i < bus->num_reset_gpios; i++) {
>> +gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
>> + GPIOD_OUT_LOW);
>> +if (IS_ERR(gpiod)) {
>> +err = PTR_ERR(gpiod);
>> +if (err != -ENOENT) {
>> +pr_err("mii_bus %s couldn't get reset GPIO\n",
>> + bus->id);
>> +return err;
>> +}
>> +} else {
>> +gpiod_set_value_cansleep(gpiod, 1);
>
>
>> +if (!bus->reset_delay_us)
>> +bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
>
> Maybe do this once, where you read the device tree property.
I was thinking from point of view that GPIO RESET code should work even without
device tree. Although I'm not sure if there would be any users or not.
>
>
>> +udelay(bus->reset_delay_us);
>> +gpiod_set_value_cansleep(gpiod, 0);
>> +}
>> +}
>> +
>> for (i = 0; i < PHY_MAX_ADDR; i++) {
>> if ((bus->phy_mask & (1 << i)) == 0) {
>> struct phy_device *phydev;
>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>> index 0b29798..83a62e4 100644
>> --- a/drivers/of/of_mdio.c
>> +++ b/drivers/of/of_mdio.c
>> @@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct
>> device_node *np)
>>
>> mdio->dev.of_node = np;
>>
>> +/* Get bus level PHY reset GPIO details */
>> +of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
>
> If the property does not exist, it is guaranteed mdio->reset_delay_us
> is not changed. So you can set it to the default value before, then do
> this call.
>
> Andrew
>
cheers,
-roger
[PATCH] mdio_bus: Issue GPIO RESET to PHYs.
Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY which manifests as PHY not being detected
or link not functional. To fix this, these PHYs need to be RESET
via a GPIO connected to the PHY's RESET pin.
Some boards have a single GPIO controlling the PHY RESET pin of all
PHYs on the bus whereas some others have separate GPIOs controlling
individual PHY RESETs.
In both cases, the RESET de-assertion cannot be done in the PHY driver
as the PHY will not probe till its reset is de-asserted.
So do the RESET de-assertion in the MDIO bus driver.
[1] - am572x-idk, am571x-idk, a437x-idk
Signed-off-by: Roger Quadros
---
drivers/net/phy/mdio_bus.c | 26 ++
drivers/of/of_mdio.c | 4
include/linux/phy.h| 5 +
3 files changed, 35 insertions(+)
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index fa7d51f..25fda2b 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -22,8 +22,11 @@
#include
#include
#include
+#include
+#include
#include
#include
+#include
#include
#include
#include
@@ -43,6 +46,8 @@
#include "mdio-boardinfo.h"
+#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
+
int mdiobus_register_device(struct mdio_device *mdiodev)
{
if (mdiodev->bus->mdio_map[mdiodev->addr])
@@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
{
struct mdio_device *mdiodev;
int i, err;
+ struct gpio_desc *gpiod;
if (NULL == bus || NULL == bus->name ||
NULL == bus->read || NULL == bus->write)
@@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module
*owner)
if (bus->reset)
bus->reset(bus);
+ /* de-assert bus level PHY GPIO resets */
+ for (i = 0; i < bus->num_reset_gpios; i++) {
+ gpiod = devm_gpiod_get_index(&bus->dev, "reset", i,
+GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ err = PTR_ERR(gpiod);
+ if (err != -ENOENT) {
+ pr_err("mii_bus %s couldn't get reset GPIO\n",
+ bus->id);
+ return err;
+ }
+ } else {
+ gpiod_set_value_cansleep(gpiod, 1);
+ if (!bus->reset_delay_us)
+ bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY;
+ udelay(bus->reset_delay_us);
+ gpiod_set_value_cansleep(gpiod, 0);
+ }
+ }
+
for (i = 0; i < PHY_MAX_ADDR; i++) {
if ((bus->phy_mask & (1 << i)) == 0) {
struct phy_device *phydev;
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 0b29798..83a62e4 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -221,6 +221,10 @@ int of_mdiobus_register(struct mii_bus *mdio, struct
device_node *np)
mdio->dev.of_node = np;
+ /* Get bus level PHY reset GPIO details */
+ of_property_read_u32(np, "reset-delay-us", &mdio->reset_delay_us);
+ mdio->num_reset_gpios = of_gpio_named_count(np, "reset-gpios");
+
/* Register the MDIO bus */
rc = mdiobus_register(mdio);
if (rc)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 43a7748..80a6574 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -217,6 +217,11 @@ struct mii_bus {
* matching its address
*/
int irq[PHY_MAX_ADDR];
+
+ /* GPIO reset pulse width in uS */
+ int reset_delay_us;
+ /* Number of reset GPIOs */
+ int num_reset_gpios;
};
#define to_mii_bus(d) container_of(d, struct mii_bus, dev)
--
2.7.4
Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs
Hi,
On 28/03/17 13:05, Roger Quadros wrote:
> +Andrew Davis & Sekhar.
>
> Hi,
>
> Andrew Davis posted a few comments offline which I'm clarifying here.
>
> On 27/03/17 14:59, Roger Quadros wrote:
>> The Ethernet link on an interrupt driven PHY was not coming up if the
>> Ethernet cable was plugged before the Ethernet interface was brought up.
>>
>> The PHY state machine seems to be stuck from RUNNING to AN state
>> with no new interrupts from the PHY. So it doesn't know when the
>> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
>> state with ANEG done thus netif_carrier_on() is never called.
>>
>> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
>> advertisement parameters didn't change.
>
> Is phy->config_aneg expected to *always* restart auto-negotiation even if
> advertisement parameters didn't change?
> If so then we'll need to fix genphy_config_aneg().
>
>>
>> Fix this by scheduling the PHY state machine in phy_start_aneg().
>> There is no way of knowing in phy.c whether auto-negotiation was
>> restarted or not by the PHY driver so we just wait for the next
>> poll/interrupt to update the PHY state machine.
>>
>> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and
>> not polling.")
>> Cc: stable # v4.9+
>> Signed-off-by: Roger Quadros
>> ---
>> v3: Fix typo in commit message
>>
>> drivers/net/phy/phy.c | 4
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 1be69d8..49dedf8 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
>>
>> out_unlock:
>> mutex_unlock(&phydev->lock);
>> +if (!err && phy_interrupt_is_valid(phydev))
>> +queue_delayed_work(system_power_efficient_wq,
>> + &phydev->state_queue, HZ);
>> +
>> return err;
>> }
>> EXPORT_SYMBOL(phy_start_aneg);
>>
>
> There is still room for optimization for interrupt driven PHYs as I still
> see a delay of 1 second between "ifconfig ethx up" and link status coming up
> if cable was already plugged in. In fact if Auto-negotiation was already
> completed
> and not required to be restarted, the PHY state machine should have move from
> AN to RUNNING instantly without expecting a PHY interrupt.
>
> How can we get rid of the unnecessary delay in the case where auto-negotiation
> is not restarted?
> Should we check for phy_aneg_done() immediately after issuing a
> phy_start_aneg()
> in phy_state_machine() and switch from PHY_AN to PHY_RUNNING?
>
> This should avoid the need to re-schedule the state machine in
> phy_start_angeg().
Any comments on my questions?
cheers,
-roger
Re: [PATCH] net: davinci_mdio: add GPIO reset logic
On 08/04/17 18:18, Florian Fainelli wrote: > > > On 04/08/2017 08:10 AM, Andrew Lunn wrote: >> On Sat, Apr 08, 2017 at 06:55:45AM -0700, David Miller wrote: >>> From: Roger Quadros >>> Date: Wed, 5 Apr 2017 11:33:57 +0300 >>> >>>> Some boards [1] leave the PHYs at an invalid state >>>> during system power-up or reset thus causing unreliability >>>> issues with the PHY like not being detected by the mdio bus >>>> or link not functional. To work around these boards have >>>> a GPIO connected to the PHY's reset pin. >>>> >>>> Implement GPIO reset handling for such cases. >>>> >>>> [1] - am572x-idk, am571x-idk, a437x-idk. >>>> >>>> Signed-off-by: Roger Quadros >>>> Signed-off-by: Sekhar Nori >>> >>> I have not seen a resolution in this discussion. >>> >>> My understanding is that there are several cases (single MDIO bus whose >>> reset does a reset on all that MDIO bus's PHYs, etc.) and it's unclear >>> how to handle all such cases cleanly. >> >> I see it falling into two cases. >> >> 1) We have a GPIO which resets one PHY. In this case, the GPIO is a >> PHY property, it should be documented in >> Documentation/devicetree/bindings/net/phy.txt. Hopefully there is >> nothing PHY driver specific here, so all the handling can be placed in >> the core PHY code. > > I suspect we would have to release the PHY GPIO reset line within a > mii_bus::reset callback, which occurs before the PHY registers are read. > There is this chicken and egg problem where you can't probe for a PHY > unless you can successfully read from it, and you can't do the PHY reset > in the PHY drivers' probe function unless you were able to find a PHY > device. +1 This is the exact problem we were facing so the reset has to be done at MDIO bus level, before PHY probe. > > NB: you can work around that by using an Ethernet PHY device compatible > string in Device Tree that already has the PHY OUI specified, although > that usually does not scale to many different boards/designs. > >> >> 2) We have one or more GPIOs which reset more than one PHY. In this >> case, the GPIOs are MDIO bus properties. Again, there should not be >> anything which is MDIO bus driver specific, so all the handling can be >> placed in the core MDIO bus code. > > Agreed. > > I would do something like: > > - if the MDIO bus node has a "gpio" reset property, use it and release > the device from reset > - for each available child node: > - if the PHY/MDIO device's node have a "gpio" reset property use it and > release the PHYs from reset. > > All of this should probably be placed somewhere in drivers/of/of_mdio.c > and deal with conditional GPIO/RESET controller support. > I agree with this proposal. Just need to spend some time to rework this patch. cheers, -roger
Re: [PATCH] net: davinci_mdio: add GPIO reset logic
On 06/04/17 15:05, Andrew Lunn wrote: >>> Do you really need more than one GPIO? A single gpio would make all >>> this code a lot simpler. >>> >> >> Yes we need. Some of our boards have separate GPIO RESET lines for >> different PHYs on the same MDIO bus. > > If you have a one-to-one mapping of GPIO and PHY, you should really be > modelling that differently. You want to be able to reset just a single > PHY, i.e. make it part of the PHY driver, or maybe the PHY core. > > Andrew > I'm not sure how it would be modelled. We have never had the need to reset just one PHY on the MDIO bus. Some boards have a single RESET line to multiple PHYs whereas others have individual RESET lines. In all cases we just want to do the RESET once per boot. cheers, -roger
Re: [PATCH] net: davinci_mdio: add GPIO reset logic
Hi,
On 05/04/17 18:03, Andrew Lunn wrote:
> On Wed, Apr 05, 2017 at 11:33:57AM +0300, Roger Quadros wrote:
>> Some boards [1] leave the PHYs at an invalid state
>> during system power-up or reset thus causing unreliability
>> issues with the PHY like not being detected by the mdio bus
>> or link not functional. To work around these boards have
>> a GPIO connected to the PHY's reset pin.
>>
>> Implement GPIO reset handling for such cases.
>>
>> [1] - am572x-idk, am571x-idk, a437x-idk.
>>
>> Signed-off-by: Roger Quadros
>> Signed-off-by: Sekhar Nori
>> ---
>> .../devicetree/bindings/net/davinci-mdio.txt | 2 +
>> drivers/net/ethernet/ti/davinci_mdio.c | 68
>> +++---
>> 2 files changed, 62 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/davinci-mdio.txt
>> b/Documentation/devicetree/bindings/net/davinci-mdio.txt
>> index 621156c..fd6ebe7 100644
>> --- a/Documentation/devicetree/bindings/net/davinci-mdio.txt
>> +++ b/Documentation/devicetree/bindings/net/davinci-mdio.txt
>> @@ -12,6 +12,8 @@ Required properties:
>>
>> Optional properties:
>> - ti,hwmods : Must be "davinci_mdio"
>> +- reset-gpios : array of GPIO specifier for PHY hardware
>> reset control
>> +- reset-delay-us: reset assertion time [in microseconds]
>>
>> Note: "ti,hwmods" field is used to fetch the base address and irq
>> resources from TI, omap hwmod data base during device registration.
>> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c
>> b/drivers/net/ethernet/ti/davinci_mdio.c
>> index 33df340..c6f9e55 100644
>> --- a/drivers/net/ethernet/ti/davinci_mdio.c
>> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
>> @@ -40,6 +40,9 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>> +#include
>>
>> /*
>> * This timeout definition is a worst-case ultra defensive measure against
>> @@ -53,6 +56,8 @@
>>
>> #define DEF_OUT_FREQ220 /* 2.2 MHz */
>>
>> +#define DEFAULT_GPIO_RESET_DELAY10 /* in microseconds */
>> +
>> struct davinci_mdio_of_param {
>> int autosuspend_delay_ms;
>> };
>> @@ -104,6 +109,9 @@ struct davinci_mdio_data {
>> */
>> boolskip_scan;
>> u32 clk_div;
>> +struct gpio_desc **gpio_reset;
>> +int num_gpios;
>> +int reset_delay_us;
>> };
>>
>> static void davinci_mdio_init_clk(struct davinci_mdio_data *data)
>> @@ -142,6 +150,20 @@ static void davinci_mdio_enable(struct
>> davinci_mdio_data *data)
>> __raw_writel(data->clk_div | CONTROL_ENABLE, &data->regs->control);
>> }
>>
>> +static void __davinci_gpio_reset(struct davinci_mdio_data *data)
>> +{
>> +int i;
>> +
>> +for (i = 0; i < data->num_gpios; i++) {
>> +if (!data->gpio_reset[i])
>> +continue;
>> +
>> +gpiod_set_value_cansleep(data->gpio_reset[i], 1);
>> +udelay(data->reset_delay_us);
>> +gpiod_set_value_cansleep(data->gpio_reset[i], 0);
>> +}
>> +}
>
> Do you really need more than one GPIO? A single gpio would make all
> this code a lot simpler.
>
Yes we need. Some of our boards have separate GPIO RESET lines for
different PHYs on the same MDIO bus.
cheers,
-roger
Re: [PATCH 0/2] ARM: am335x-icev2: Add ethernet support
On 04/04/17 19:01, Tony Lindgren wrote: > * Roger Quadros [170330 05:37]: >> Hi Tony & Dave, >> >> On 13/03/17 15:42, Roger Quadros wrote: >>> Hi, >>> >>> This series adds ethernet support to am335x-icev2 board. >>> >>> The ethernet PHYs on the board need an explicit GPIO reset pulse >>> to ensure they bootstrap to the correct mode. Without the >>> GPIO reset they just don't work. >>> >>> cheers, >>> -roger >> >> Any comments on this series. Patch 1 is at version 2. > > I think you meant patch 2/3 is at version2. I've picked > patches 2 and 3 for v4.12 into dt and defconfig branches. Thanks Tony. > > You may need to resend the davinci_mdio.c patch alone > for Dave as he usually won't pick individual patches I > think. OK, I'll send that one separately. cheers, -roger > > Regards, > > Tony > >>> Roger Quadros (2): >>> net: davinci_mdio: add GPIO reset logic >>> ARM: dts: am335x-icev2: Add CPSW ethernet0 and ethernet1 >>> >>> .../devicetree/bindings/net/davinci-mdio.txt | 2 + >>> arch/arm/boot/dts/am335x-icev2.dts | 113 >>> + >>> drivers/net/ethernet/ti/davinci_mdio.c | 68 +++-- >>> 3 files changed, 175 insertions(+), 8 deletions(-) >>> >>
[PATCH] net: davinci_mdio: add GPIO reset logic
Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY like not being detected by the mdio bus
or link not functional. To work around these boards have
a GPIO connected to the PHY's reset pin.
Implement GPIO reset handling for such cases.
[1] - am572x-idk, am571x-idk, a437x-idk.
Signed-off-by: Roger Quadros
Signed-off-by: Sekhar Nori
---
.../devicetree/bindings/net/davinci-mdio.txt | 2 +
drivers/net/ethernet/ti/davinci_mdio.c | 68 +++---
2 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/davinci-mdio.txt
b/Documentation/devicetree/bindings/net/davinci-mdio.txt
index 621156c..fd6ebe7 100644
--- a/Documentation/devicetree/bindings/net/davinci-mdio.txt
+++ b/Documentation/devicetree/bindings/net/davinci-mdio.txt
@@ -12,6 +12,8 @@ Required properties:
Optional properties:
- ti,hwmods: Must be "davinci_mdio"
+- reset-gpios : array of GPIO specifier for PHY hardware reset control
+- reset-delay-us : reset assertion time [in microseconds]
Note: "ti,hwmods" field is used to fetch the base address and irq
resources from TI, omap hwmod data base during device registration.
diff --git a/drivers/net/ethernet/ti/davinci_mdio.c
b/drivers/net/ethernet/ti/davinci_mdio.c
index 33df340..c6f9e55 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -40,6 +40,9 @@
#include
#include
#include
+#include
+#include
+#include
/*
* This timeout definition is a worst-case ultra defensive measure against
@@ -53,6 +56,8 @@
#define DEF_OUT_FREQ 220 /* 2.2 MHz */
+#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
+
struct davinci_mdio_of_param {
int autosuspend_delay_ms;
};
@@ -104,6 +109,9 @@ struct davinci_mdio_data {
*/
boolskip_scan;
u32 clk_div;
+ struct gpio_desc **gpio_reset;
+ int num_gpios;
+ int reset_delay_us;
};
static void davinci_mdio_init_clk(struct davinci_mdio_data *data)
@@ -142,6 +150,20 @@ static void davinci_mdio_enable(struct davinci_mdio_data
*data)
__raw_writel(data->clk_div | CONTROL_ENABLE, &data->regs->control);
}
+static void __davinci_gpio_reset(struct davinci_mdio_data *data)
+{
+ int i;
+
+ for (i = 0; i < data->num_gpios; i++) {
+ if (!data->gpio_reset[i])
+ continue;
+
+ gpiod_set_value_cansleep(data->gpio_reset[i], 1);
+ udelay(data->reset_delay_us);
+ gpiod_set_value_cansleep(data->gpio_reset[i], 0);
+ }
+}
+
static int davinci_mdio_reset(struct mii_bus *bus)
{
struct davinci_mdio_data *data = bus->priv;
@@ -317,20 +339,50 @@ static int davinci_mdio_write(struct mii_bus *bus, int
phy_id,
}
#if IS_ENABLED(CONFIG_OF)
-static int davinci_mdio_probe_dt(struct mdio_platform_data *data,
-struct platform_device *pdev)
+static int davinci_mdio_probe_dt(struct davinci_mdio_data *data,
+struct platform_device *pdev)
{
struct device_node *node = pdev->dev.of_node;
u32 prop;
-
- if (!node)
- return -EINVAL;
+ int error;
+ int i;
+ struct gpio_desc *gpiod;
if (of_property_read_u32(node, "bus_freq", &prop)) {
dev_err(&pdev->dev, "Missing bus_freq property in the DT.\n");
- return -EINVAL;
+ data->pdata = default_pdata;
+ } else {
+ data->pdata.bus_freq = prop;
+ }
+
+ i = of_gpio_named_count(node, "reset-gpios");
+ if (i > 0) {
+ data->num_gpios = i;
+ data->gpio_reset = devm_kcalloc(&pdev->dev, i,
+ sizeof(struct gpio_desc *),
+ GFP_KERNEL);
+ if (!data->gpio_reset)
+ return -ENOMEM;
+
+ for (i = 0; i < data->num_gpios; i++) {
+ gpiod = devm_gpiod_get_index(&pdev->dev, "reset", i,
+GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ error = PTR_ERR(gpiod);
+ if (error == -ENOENT)
+ continue;
+ else
+ return error;
+ }
+ data->gpio_reset[i] = gpiod;
+ }
+
+ if (of_property_read_u32(node, "reset-delay-us",
+
Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs
Florian,
On 30/03/17 23:02, Florian Fainelli wrote:
> On 03/27/2017 04:59 AM, Roger Quadros wrote:
>> The Ethernet link on an interrupt driven PHY was not coming up if the
>> Ethernet cable was plugged before the Ethernet interface was brought up.
>>
>> The PHY state machine seems to be stuck from RUNNING to AN state
>> with no new interrupts from the PHY. So it doesn't know when the
>> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
>> state with ANEG done thus netif_carrier_on() is never called.
>>
>> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
>> advertisement parameters didn't change.
>>
>> Fix this by scheduling the PHY state machine in phy_start_aneg().
>> There is no way of knowing in phy.c whether auto-negotiation was
>> restarted or not by the PHY driver so we just wait for the next
>> poll/interrupt to update the PHY state machine.
>>
>> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and
>> not polling.")
>> Cc: stable # v4.9+
>> Signed-off-by: Roger Quadros
>
> Reviewed-by: Florian Fainelli
>
Thanks for the review, but there are a still few unanswered questions in the
parallel thread.
Can you please clarify those first before this patch gets picked? Thanks.
cheers,
-roger
Re: [PATCH 0/2] ARM: am335x-icev2: Add ethernet support
Hi Tony & Dave, On 13/03/17 15:42, Roger Quadros wrote: > Hi, > > This series adds ethernet support to am335x-icev2 board. > > The ethernet PHYs on the board need an explicit GPIO reset pulse > to ensure they bootstrap to the correct mode. Without the > GPIO reset they just don't work. > > cheers, > -roger Any comments on this series. Patch 1 is at version 2. > > Roger Quadros (2): > net: davinci_mdio: add GPIO reset logic > ARM: dts: am335x-icev2: Add CPSW ethernet0 and ethernet1 > > .../devicetree/bindings/net/davinci-mdio.txt | 2 + > arch/arm/boot/dts/am335x-icev2.dts | 113 > + > drivers/net/ethernet/ti/davinci_mdio.c | 68 +++-- > 3 files changed, 175 insertions(+), 8 deletions(-) > cheers, -roger
Re: [PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs
+Andrew Davis & Sekhar.
Hi,
Andrew Davis posted a few comments offline which I'm clarifying here.
On 27/03/17 14:59, Roger Quadros wrote:
> The Ethernet link on an interrupt driven PHY was not coming up if the
> Ethernet cable was plugged before the Ethernet interface was brought up.
>
> The PHY state machine seems to be stuck from RUNNING to AN state
> with no new interrupts from the PHY. So it doesn't know when the
> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
> state with ANEG done thus netif_carrier_on() is never called.
>
> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
> advertisement parameters didn't change.
Is phy->config_aneg expected to *always* restart auto-negotiation even if
advertisement parameters didn't change?
If so then we'll need to fix genphy_config_aneg().
>
> Fix this by scheduling the PHY state machine in phy_start_aneg().
> There is no way of knowing in phy.c whether auto-negotiation was
> restarted or not by the PHY driver so we just wait for the next
> poll/interrupt to update the PHY state machine.
>
> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not
> polling.")
> Cc: stable # v4.9+
> Signed-off-by: Roger Quadros
> ---
> v3: Fix typo in commit message
>
> drivers/net/phy/phy.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 1be69d8..49dedf8 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
>
> out_unlock:
> mutex_unlock(&phydev->lock);
> + if (!err && phy_interrupt_is_valid(phydev))
> + queue_delayed_work(system_power_efficient_wq,
> +&phydev->state_queue, HZ);
> +
> return err;
> }
> EXPORT_SYMBOL(phy_start_aneg);
>
There is still room for optimization for interrupt driven PHYs as I still
see a delay of 1 second between "ifconfig ethx up" and link status coming up
if cable was already plugged in. In fact if Auto-negotiation was already
completed
and not required to be restarted, the PHY state machine should have move from
AN to RUNNING instantly without expecting a PHY interrupt.
How can we get rid of the unnecessary delay in the case where auto-negotiation
is not restarted?
Should we check for phy_aneg_done() immediately after issuing a phy_start_aneg()
in phy_state_machine() and switch from PHY_AN to PHY_RUNNING?
This should avoid the need to re-schedule the state machine in
phy_start_angeg().
cheers,
-roger
[PATCH v3 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs
The Ethernet link on an interrupt driven PHY was not coming up if the
Ethernet cable was plugged before the Ethernet interface was brought up.
The PHY state machine seems to be stuck from RUNNING to AN state
with no new interrupts from the PHY. So it doesn't know when the
PHY Auto-negotiation has been completed and doesn't transition to RUNNING
state with ANEG done thus netif_carrier_on() is never called.
NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
advertisement parameters didn't change.
Fix this by scheduling the PHY state machine in phy_start_aneg().
There is no way of knowing in phy.c whether auto-negotiation was
restarted or not by the PHY driver so we just wait for the next
poll/interrupt to update the PHY state machine.
Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not
polling.")
Cc: stable # v4.9+
Signed-off-by: Roger Quadros
---
v3: Fix typo in commit message
drivers/net/phy/phy.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1be69d8..49dedf8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
out_unlock:
mutex_unlock(&phydev->lock);
+ if (!err && phy_interrupt_is_valid(phydev))
+ queue_delayed_work(system_power_efficient_wq,
+ &phydev->state_queue, HZ);
+
return err;
}
EXPORT_SYMBOL(phy_start_aneg);
--
2.7.4
Re: [PATCH v2 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs
On 23/03/17 11:52, Sergei Shtylyov wrote:
> Hello!
>
> On 3/22/2017 2:02 PM, Roger Quadros wrote:
>
>> he ethernet link on an interrupt driven PHY was not coming up if the
>
>s/he/The/?
>
>> ethernet cable was plugged before the ethernet interface was brought up.
>
>Also, my spell checker trips on "ethernet", perhaps should be capitalized?
Thanks. I'll fix both issues.
>
>> The PHY state machine seems to be stuck from RUNNING to AN state
>> with no new interrupts from the PHY. So it doesn't know when the
>> PHY Auto-negotiation has been completed and doesn't transition to RUNNING
>> state with ANEG done thus netif_carrier_on() is never called.
>>
>> NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
>> advertisement parameters didn't change.
>>
>> Fix this by scheduling the PHY state machine in phy_start_aneg().
>>
>> Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and
>> not polling.")
>> Cc: stable # v4.9+
>> Signed-off-by: Roger Quadros
> [...]
>
> MBR, Sergei
>
cheers,
-roger
[PATCH v2 1/2] net: phy: Fix PHY AN done state machine for interrupt driven PHYs
he ethernet link on an interrupt driven PHY was not coming up if the
ethernet cable was plugged before the ethernet interface was brought up.
The PHY state machine seems to be stuck from RUNNING to AN state
with no new interrupts from the PHY. So it doesn't know when the
PHY Auto-negotiation has been completed and doesn't transition to RUNNING
state with ANEG done thus netif_carrier_on() is never called.
NOTE: genphy_config_aneg() will not restart PHY Auto-negotiation of
advertisement parameters didn't change.
Fix this by scheduling the PHY state machine in phy_start_aneg().
Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not
polling.")
Cc: stable # v4.9+
Signed-off-by: Roger Quadros
---
drivers/net/phy/phy.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1be69d8..49dedf8 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
out_unlock:
mutex_unlock(&phydev->lock);
+ if (!err && phy_interrupt_is_valid(phydev))
+ queue_delayed_work(system_power_efficient_wq,
+ &phydev->state_queue, HZ);
+
return err;
}
EXPORT_SYMBOL(phy_start_aneg);
--
2.7.4
[PATCH v2 2/2] net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts
phy_suspend() doesn't get called as part of phy_stop() for PHYs
using interrupts because the phy state machine is never triggered
after a phy_stop().
Explicitly trigger the PHY state machine in phy_stop() so that it can
see the new PHY state (HALTED) and suspend the PHY.
As most PHYLIB consumers will call phy_stop() with rtnl_lock() held
from ndo_close() so we use don't wait for workqueue cancellation in
phy_trigger_machine() by passing false for the 'sync' argument.
Fixes: 3c293f4e08b5 ("net: phy: Trigger state machine on state change and not
polling.")
Cc: stable # v4.9+
Signed-off-by: Roger Quadros
---
drivers/net/phy/phy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 49dedf8..ab14e7b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -907,6 +907,7 @@ void phy_stop(struct phy_device *phydev)
* of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
* will not reenable interrupts.
*/
+ phy_trigger_machine(phydev, false);
}
EXPORT_SYMBOL(phy_stop);
--
2.7.4
[PATCH v2 0/2] net: phy: state machine fixes for interrupt driven PHYs
Hi, These 2 patches fix the following 2 issues with the PHY state machine when used with interrupt driven PHYs. - PHY link not coming up if Ethernet cable is plugged before Ethernet network interface is brought up. - PHY not being suspended when PHY is halted. cheers, -roger Roger Quadros (2): net: phy: Fix PHY AN done state machine for interrupt driven PHYs net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts drivers/net/phy/phy.c | 5 + 1 file changed, 5 insertions(+) -- 2.7.4
Re: [RFC PATCH] net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts
On 20/03/17 18:41, Florian Fainelli wrote:
> On 03/16/2017 12:46 AM, Roger Quadros wrote:
>> On 15/03/17 17:49, Andrew Lunn wrote:
>>> On Wed, Mar 15, 2017 at 05:00:08PM +0200, Roger Quadros wrote:
>>>> Andrew,
>>>>
>>>> On 15/03/17 16:08, Andrew Lunn wrote:
>>>>> On Wed, Mar 15, 2017 at 03:51:27PM +0200, Roger Quadros wrote:
>>>>>> Since commit 3c293f4e08b5 ("net: phy: Trigger state machine on state
>>>>>> change and not polling.")
>>>>>> phy_suspend() doesn't get called as part of phy_stop() for PHYs using
>>>>>> interrupts because the phy state machine is never triggered after a
>>>>>> phy_stop().
>>>>>>
>>>>>> Explicitly trigger the PHY state machine so that it can
>>>>>> see the new PHY state (HALTED) and suspend the PHY.
>>>>>>
>>>>>> Signed-off-by: Roger Quadros
>>>>>
>>>>> Hi Roger
>>>>>
>>>>> This seems sensible. It mirrors what phy_start() does.
>>>>>
>>>>> Reviewed-by: Andrew Lunn
>>>>
>>>> The reason for this being an RFC was the following comment just before
>>>> where I add the phy_trigger_machine()
>>>>
>>>> /* Cannot call flush_scheduled_work() here as desired because
>>>> * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
>>>> * will not reenable interrupts.
>>>> */
>>>>
>>>> Is this comment still applicable? If yes, is it OK to call
>>>> phy_trigger_machine() there?
>>>
>>> Humm, good question.
>>>
>>> My _guess_ would be, calling it with sync=True could
>>> deadlock. sync=False is probably safe. But lets see what Florian says.
>>
>> I agree that we should use phy_trigger_machine() with sync=False.
>>
>>>
>>>>
>>>>>
>>>>> It does however lead to a follow up question. Are there other places
>>>>> phydev->state is changed and it is missing a phy_trigger_machine()?
>>>>>
>>>>
>>>> One other place I think we should add phy_trigger_machine() is
>>>> phy_start_aneg().
>>>
>>> Humm, that might get us into a tight loop.
>>>
>>> phy_start_aneg() kicks the phy driver to start autoneg and sets
>>> phydev->state = PHY_AN.
>>>
>>> phy_trigger_machine() triggers the state machine immediately.
>>>
>>> In state PHY_AN, we check if aneg is done. If not, it sets needs_aneg
>>> = true. At the end of the state machine, this then calls
>>> phy_start_aneg(), and it all starts again.
>>>
>>> We are missing the 1s delay we have with polling. So for
>>> phy_start_aneg(), we might need a phy_delayed_trigger_machine(), which
>>> waits a second before doing anything?
>>
>> I think that should do the trick.
>>
>> How about this?
>
> This sounds like a possible fix indeed, however I would like to better
> assess the impact on non interrupt driven PHYs before rolling such a change.
Is it safer if I add a check to do this only for interrupt driven PHYs?
e.g.
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 4b855f2..e0f5755 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -630,6 +630,9 @@ int phy_start_aneg(struct phy_device *phydev)
out_unlock:
mutex_unlock(&phydev->lock);
+ if (!err && phy_interrupt_is_valid(phydev))
+ queue_delayed_work(system_power_efficient_wq,
&phydev->state_queue, HZ);
+
return err;
}
EXPORT_SYMBOL(phy_start_aneg);
--
2.7.4
--
cheers,
-roger
Re: [RFC PATCH] net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts
On 15/03/17 17:49, Andrew Lunn wrote:
> On Wed, Mar 15, 2017 at 05:00:08PM +0200, Roger Quadros wrote:
>> Andrew,
>>
>> On 15/03/17 16:08, Andrew Lunn wrote:
>>> On Wed, Mar 15, 2017 at 03:51:27PM +0200, Roger Quadros wrote:
>>>> Since commit 3c293f4e08b5 ("net: phy: Trigger state machine on state
>>>> change and not polling.")
>>>> phy_suspend() doesn't get called as part of phy_stop() for PHYs using
>>>> interrupts because the phy state machine is never triggered after a
>>>> phy_stop().
>>>>
>>>> Explicitly trigger the PHY state machine so that it can
>>>> see the new PHY state (HALTED) and suspend the PHY.
>>>>
>>>> Signed-off-by: Roger Quadros
>>>
>>> Hi Roger
>>>
>>> This seems sensible. It mirrors what phy_start() does.
>>>
>>> Reviewed-by: Andrew Lunn
>>
>> The reason for this being an RFC was the following comment just before
>> where I add the phy_trigger_machine()
>>
>> /* Cannot call flush_scheduled_work() here as desired because
>> * of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
>> * will not reenable interrupts.
>> */
>>
>> Is this comment still applicable? If yes, is it OK to call
>> phy_trigger_machine() there?
>
> Humm, good question.
>
> My _guess_ would be, calling it with sync=True could
> deadlock. sync=False is probably safe. But lets see what Florian says.
I agree that we should use phy_trigger_machine() with sync=False.
>
>>
>>>
>>> It does however lead to a follow up question. Are there other places
>>> phydev->state is changed and it is missing a phy_trigger_machine()?
>>>
>>
>> One other place I think we should add phy_trigger_machine() is
>> phy_start_aneg().
>
> Humm, that might get us into a tight loop.
>
> phy_start_aneg() kicks the phy driver to start autoneg and sets
> phydev->state = PHY_AN.
>
> phy_trigger_machine() triggers the state machine immediately.
>
> In state PHY_AN, we check if aneg is done. If not, it sets needs_aneg
> = true. At the end of the state machine, this then calls
> phy_start_aneg(), and it all starts again.
>
> We are missing the 1s delay we have with polling. So for
> phy_start_aneg(), we might need a phy_delayed_trigger_machine(), which
> waits a second before doing anything?
I think that should do the trick.
How about this?
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 8fef03b..162061c 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -630,6 +630,10 @@ int phy_start_aneg(struct phy_device *phydev)
out_unlock:
mutex_unlock(&phydev->lock);
+
+ if (!err)
+ queue_delayed_work(system_power_efficient_wq,
&phydev->state_queue, HZ);
+
return err;
}
EXPORT_SYMBOL(phy_start_aneg);
--
cheers,
-roger
Re: [RFC PATCH] net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts
Andrew,
On 15/03/17 16:08, Andrew Lunn wrote:
> On Wed, Mar 15, 2017 at 03:51:27PM +0200, Roger Quadros wrote:
>> Since commit 3c293f4e08b5 ("net: phy: Trigger state machine on state change
>> and not polling.")
>> phy_suspend() doesn't get called as part of phy_stop() for PHYs using
>> interrupts because the phy state machine is never triggered after a
>> phy_stop().
>>
>> Explicitly trigger the PHY state machine so that it can
>> see the new PHY state (HALTED) and suspend the PHY.
>>
>> Signed-off-by: Roger Quadros
>
> Hi Roger
>
> This seems sensible. It mirrors what phy_start() does.
>
> Reviewed-by: Andrew Lunn
The reason for this being an RFC was the following comment just before
where I add the phy_trigger_machine()
/* Cannot call flush_scheduled_work() here as desired because
* of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
* will not reenable interrupts.
*/
Is this comment still applicable? If yes, is it OK to call
phy_trigger_machine() there?
>
> It does however lead to a follow up question. Are there other places
> phydev->state is changed and it is missing a phy_trigger_machine()?
>
One other place I think we should add phy_trigger_machine() is phy_start_aneg().
Since commit 3c293f4e08b5 I was not getting my ethernet link to come up if
ethernet cable was plugged before the ethernet interface was brought up.
The PHY seems to be stuck from RUNNING to AN state with no new interrupts from
the
PHY.
I could workaround it on my board by doing either of the following:
1) explicitly halt the PHY at ethernet driver probe time. Then when
network interface is brought up it resumes the PHY and we see the
Link/ANEG done interrupt.
2) force BMCR_ANRESTART every time from .config_aneg in the PHY driver.
I will still keep approach 1 as it is desirable to put the PHY in low power mode
for us but I need a second opinion if we should call phy_trigger_machine()
from phy_start_aneg() or not.
--
cheers,
-roger
[RFC PATCH] net: phy: Don't miss phy_suspend() on PHY_HALTED for PHYs with interrupts
Since commit 3c293f4e08b5 ("net: phy: Trigger state machine on state change and
not polling.")
phy_suspend() doesn't get called as part of phy_stop() for PHYs using
interrupts because the phy state machine is never triggered after a phy_stop().
Explicitly trigger the PHY state machine so that it can
see the new PHY state (HALTED) and suspend the PHY.
Signed-off-by: Roger Quadros
---
drivers/net/phy/phy.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 1be69d8..8fef03b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -903,6 +903,7 @@ void phy_stop(struct phy_device *phydev)
* of rtnl_lock(), but PHY_HALTED shall guarantee phy_change()
* will not reenable interrupts.
*/
+ phy_trigger_machine(phydev, true);
}
EXPORT_SYMBOL(phy_stop);
--
2.7.4
[PATCH 3/3] ARM: omap2plus_defconfig: Enable TI Ethernet PHY
DP83848_PHY i.e. [TI TLK10X 10/100 Mbps PHY] is used on the am335x-icev2 board. Enable the PHY driver for it. Signed-off-by: Roger Quadros --- arch/arm/configs/omap2plus_defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm/configs/omap2plus_defconfig b/arch/arm/configs/omap2plus_defconfig index f2462a6..cfa5bac 100644 --- a/arch/arm/configs/omap2plus_defconfig +++ b/arch/arm/configs/omap2plus_defconfig @@ -167,6 +167,7 @@ CONFIG_TI_CPTS=y # CONFIG_NET_VENDOR_VIA is not set # CONFIG_NET_VENDOR_WIZNET is not set CONFIG_AT803X_PHY=y +CONFIG_DP83848_PHY=y CONFIG_MICREL_PHY=y CONFIG_SMSC_PHY=y CONFIG_USB_USBNET=m -- 2.7.4
[PATCH v2 2/2] ARM: dts: am335x-icev2: Add CPSW ethernet0 and ethernet1
Enable the 2 ethernet ports as CPSW ports in dual-mac mode
Signed-off-by: Roger Quadros
[nsek...@ti.com: use AM33XX_IOPAD()]
Signed-off-by: Sekhar Nori
---
v2:
- use phy-handle instead of phy_id
arch/arm/boot/dts/am335x-icev2.dts | 121 +
1 file changed, 121 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-icev2.dts
b/arch/arm/boot/dts/am335x-icev2.dts
index a2ad076..415cd46 100644
--- a/arch/arm/boot/dts/am335x-icev2.dts
+++ b/arch/arm/boot/dts/am335x-icev2.dts
@@ -201,6 +201,69 @@
AM33XX_IOPAD(0x938, PIN_OUTPUT_PULLUP | MUX_MODE1) /*
(L16) gmii1_rxd2.uart3_txd */
>;
};
+
+ cpsw_default: cpsw_default {
+ pinctrl-single,pins = <
+ /* Slave 1, RMII mode */
+ AM33XX_IOPAD(0x90c, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_crs.rmii1_crs_dv */
+ AM33XX_IOPAD(0x944, (PIN_INPUT_PULLUP | MUX_MODE0))
/* rmii1_refclk.rmii1_refclk */
+ AM33XX_IOPAD(0x940, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_rxd0.rmii1_rxd0 */
+ AM33XX_IOPAD(0x93c, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_rxd1.rmii1_rxd1 */
+ AM33XX_IOPAD(0x910, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_rxerr.rmii1_rxerr */
+ AM33XX_IOPAD(0x928, (PIN_OUTPUT_PULLDOWN | MUX_MODE1))
/* mii1_txd0.rmii1_txd0 */
+ AM33XX_IOPAD(0x924, (PIN_OUTPUT_PULLDOWN | MUX_MODE1))
/* mii1_txd1.rmii1_txd1 */
+ AM33XX_IOPAD(0x914, (PIN_OUTPUT_PULLDOWN | MUX_MODE1))
/* mii1_txen.rmii1_txen */
+ /* Slave 2, RMII mode */
+ AM33XX_IOPAD(0x870, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_wait0.rmii2_crs_dv */
+ AM33XX_IOPAD(0x908, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_col.rmii2_refclk */
+ AM33XX_IOPAD(0x86c, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_a11.rmii2_rxd0 */
+ AM33XX_IOPAD(0x868, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_a10.rmii2_rxd1 */
+ AM33XX_IOPAD(0x874, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_wpn.rmii2_rxerr */
+ AM33XX_IOPAD(0x854, (PIN_OUTPUT_PULLDOWN | MUX_MODE3))
/* gpmc_a5.rmii2_txd0 */
+ AM33XX_IOPAD(0x850, (PIN_OUTPUT_PULLDOWN | MUX_MODE3))
/* gpmc_a4.rmii2_txd1 */
+ AM33XX_IOPAD(0x840, (PIN_OUTPUT_PULLDOWN | MUX_MODE3))
/* gpmc_a0.rmii2_txen */
+ >;
+ };
+
+ cpsw_sleep: cpsw_sleep {
+ pinctrl-single,pins = <
+ /* Slave 1 reset value */
+ AM33XX_IOPAD(0x90c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x944, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x940, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x93c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x910, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x928, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x924, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x914, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+
+ /* Slave 2 reset value */
+ AM33XX_IOPAD(0x870, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x908, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x86c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x868, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x874, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x854, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x850, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x840, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ >;
+ };
+
+ davinci_mdio_default: davinci_mdio_default {
+ pinctrl-single,pins = <
+ /* MDIO */
+ AM33XX_IOPAD(0x948, (PIN_INPUT_PULLUP | SLEWCTRL_FAST |
MUX_MODE0)) /* mdio_data.mdio_data */
+ AM33XX_IOPAD(0x94c, (PIN_OUTPUT_PULLUP | MUX_MODE0))
/* mdio_clk.mdio_clk */
+ >;
+ };
+
+ davinci_mdio_sleep: davinci_mdio_sleep {
+ pinctrl-single,pins = <
+ /* MDIO reset value */
+ AM33XX_IOPAD(0x948, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x94c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ >;
+ };
};
&i2c0 {
@@ -350,3 +413,61 @@
pinctrl-0 = <&uart3_pins_default>;
status = "okay";
};
+
+&gpio3 {
+ p4 {
+
[PATCH 0/2] ARM: am335x-icev2: Add ethernet support
Hi, This series adds ethernet support to am335x-icev2 board. The ethernet PHYs on the board need an explicit GPIO reset pulse to ensure they bootstrap to the correct mode. Without the GPIO reset they just don't work. cheers, -roger Roger Quadros (2): net: davinci_mdio: add GPIO reset logic ARM: dts: am335x-icev2: Add CPSW ethernet0 and ethernet1 .../devicetree/bindings/net/davinci-mdio.txt | 2 + arch/arm/boot/dts/am335x-icev2.dts | 113 + drivers/net/ethernet/ti/davinci_mdio.c | 68 +++-- 3 files changed, 175 insertions(+), 8 deletions(-) -- 2.7.4
[PATCH 1/2] net: davinci_mdio: add GPIO reset logic
Some boards [1] leave the PHYs at an invalid state
during system power-up or reset thus causing unreliability
issues with the PHY like not being detected by the mdio bus
or link not functional. To work around these boards have
a GPIO connected to the PHY's reset pin.
Implement GPIO reset handling for such cases.
[1] - am572x-idk, am571x-idk, a437x-idk.
Signed-off-by: Roger Quadros
Signed-off-by: Sekhar Nori
---
.../devicetree/bindings/net/davinci-mdio.txt | 2 +
drivers/net/ethernet/ti/davinci_mdio.c | 68 +++---
2 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/davinci-mdio.txt
b/Documentation/devicetree/bindings/net/davinci-mdio.txt
index 621156c..fd6ebe7 100644
--- a/Documentation/devicetree/bindings/net/davinci-mdio.txt
+++ b/Documentation/devicetree/bindings/net/davinci-mdio.txt
@@ -12,6 +12,8 @@ Required properties:
Optional properties:
- ti,hwmods: Must be "davinci_mdio"
+- reset-gpios : array of GPIO specifier for PHY hardware reset control
+- reset-delay-us : reset assertion time [in microseconds]
Note: "ti,hwmods" field is used to fetch the base address and irq
resources from TI, omap hwmod data base during device registration.
diff --git a/drivers/net/ethernet/ti/davinci_mdio.c
b/drivers/net/ethernet/ti/davinci_mdio.c
index 33df340..c6f9e55 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -40,6 +40,9 @@
#include
#include
#include
+#include
+#include
+#include
/*
* This timeout definition is a worst-case ultra defensive measure against
@@ -53,6 +56,8 @@
#define DEF_OUT_FREQ 220 /* 2.2 MHz */
+#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */
+
struct davinci_mdio_of_param {
int autosuspend_delay_ms;
};
@@ -104,6 +109,9 @@ struct davinci_mdio_data {
*/
boolskip_scan;
u32 clk_div;
+ struct gpio_desc **gpio_reset;
+ int num_gpios;
+ int reset_delay_us;
};
static void davinci_mdio_init_clk(struct davinci_mdio_data *data)
@@ -142,6 +150,20 @@ static void davinci_mdio_enable(struct davinci_mdio_data
*data)
__raw_writel(data->clk_div | CONTROL_ENABLE, &data->regs->control);
}
+static void __davinci_gpio_reset(struct davinci_mdio_data *data)
+{
+ int i;
+
+ for (i = 0; i < data->num_gpios; i++) {
+ if (!data->gpio_reset[i])
+ continue;
+
+ gpiod_set_value_cansleep(data->gpio_reset[i], 1);
+ udelay(data->reset_delay_us);
+ gpiod_set_value_cansleep(data->gpio_reset[i], 0);
+ }
+}
+
static int davinci_mdio_reset(struct mii_bus *bus)
{
struct davinci_mdio_data *data = bus->priv;
@@ -317,20 +339,50 @@ static int davinci_mdio_write(struct mii_bus *bus, int
phy_id,
}
#if IS_ENABLED(CONFIG_OF)
-static int davinci_mdio_probe_dt(struct mdio_platform_data *data,
-struct platform_device *pdev)
+static int davinci_mdio_probe_dt(struct davinci_mdio_data *data,
+struct platform_device *pdev)
{
struct device_node *node = pdev->dev.of_node;
u32 prop;
-
- if (!node)
- return -EINVAL;
+ int error;
+ int i;
+ struct gpio_desc *gpiod;
if (of_property_read_u32(node, "bus_freq", &prop)) {
dev_err(&pdev->dev, "Missing bus_freq property in the DT.\n");
- return -EINVAL;
+ data->pdata = default_pdata;
+ } else {
+ data->pdata.bus_freq = prop;
+ }
+
+ i = of_gpio_named_count(node, "reset-gpios");
+ if (i > 0) {
+ data->num_gpios = i;
+ data->gpio_reset = devm_kcalloc(&pdev->dev, i,
+ sizeof(struct gpio_desc *),
+ GFP_KERNEL);
+ if (!data->gpio_reset)
+ return -ENOMEM;
+
+ for (i = 0; i < data->num_gpios; i++) {
+ gpiod = devm_gpiod_get_index(&pdev->dev, "reset", i,
+GPIOD_OUT_LOW);
+ if (IS_ERR(gpiod)) {
+ error = PTR_ERR(gpiod);
+ if (error == -ENOENT)
+ continue;
+ else
+ return error;
+ }
+ data->gpio_reset[i] = gpiod;
+ }
+
+ if (of_property_read_u32(node, "reset-delay-us",
+
[PATCH 2/2] ARM: dts: am335x-icev2: Add CPSW ethernet0 and ethernet1
Enable the 2 ethernet ports as CPSW ports in dual-mac mode
Signed-off-by: Roger Quadros
[nsek...@ti.com: use AM33XX_IOPAD()]
Signed-off-by: Sekhar Nori
---
arch/arm/boot/dts/am335x-icev2.dts | 113 +
1 file changed, 113 insertions(+)
diff --git a/arch/arm/boot/dts/am335x-icev2.dts
b/arch/arm/boot/dts/am335x-icev2.dts
index a2ad076..cc343b0 100644
--- a/arch/arm/boot/dts/am335x-icev2.dts
+++ b/arch/arm/boot/dts/am335x-icev2.dts
@@ -201,6 +201,69 @@
AM33XX_IOPAD(0x938, PIN_OUTPUT_PULLUP | MUX_MODE1) /*
(L16) gmii1_rxd2.uart3_txd */
>;
};
+
+ cpsw_default: cpsw_default {
+ pinctrl-single,pins = <
+ /* Slave 1, RMII mode */
+ AM33XX_IOPAD(0x90c, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_crs.rmii1_crs_dv */
+ AM33XX_IOPAD(0x944, (PIN_INPUT_PULLUP | MUX_MODE0))
/* rmii1_refclk.rmii1_refclk */
+ AM33XX_IOPAD(0x940, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_rxd0.rmii1_rxd0 */
+ AM33XX_IOPAD(0x93c, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_rxd1.rmii1_rxd1 */
+ AM33XX_IOPAD(0x910, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_rxerr.rmii1_rxerr */
+ AM33XX_IOPAD(0x928, (PIN_OUTPUT_PULLDOWN | MUX_MODE1))
/* mii1_txd0.rmii1_txd0 */
+ AM33XX_IOPAD(0x924, (PIN_OUTPUT_PULLDOWN | MUX_MODE1))
/* mii1_txd1.rmii1_txd1 */
+ AM33XX_IOPAD(0x914, (PIN_OUTPUT_PULLDOWN | MUX_MODE1))
/* mii1_txen.rmii1_txen */
+ /* Slave 2, RMII mode */
+ AM33XX_IOPAD(0x870, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_wait0.rmii2_crs_dv */
+ AM33XX_IOPAD(0x908, (PIN_INPUT_PULLUP | MUX_MODE1))
/* mii1_col.rmii2_refclk */
+ AM33XX_IOPAD(0x86c, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_a11.rmii2_rxd0 */
+ AM33XX_IOPAD(0x868, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_a10.rmii2_rxd1 */
+ AM33XX_IOPAD(0x874, (PIN_INPUT_PULLUP | MUX_MODE3))
/* gpmc_wpn.rmii2_rxerr */
+ AM33XX_IOPAD(0x854, (PIN_OUTPUT_PULLDOWN | MUX_MODE3))
/* gpmc_a5.rmii2_txd0 */
+ AM33XX_IOPAD(0x850, (PIN_OUTPUT_PULLDOWN | MUX_MODE3))
/* gpmc_a4.rmii2_txd1 */
+ AM33XX_IOPAD(0x840, (PIN_OUTPUT_PULLDOWN | MUX_MODE3))
/* gpmc_a0.rmii2_txen */
+ >;
+ };
+
+ cpsw_sleep: cpsw_sleep {
+ pinctrl-single,pins = <
+ /* Slave 1 reset value */
+ AM33XX_IOPAD(0x90c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x944, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x940, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x93c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x910, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x928, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x924, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x914, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+
+ /* Slave 2 reset value */
+ AM33XX_IOPAD(0x870, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x908, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x86c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x868, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x874, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x854, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x850, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x840, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ >;
+ };
+
+ davinci_mdio_default: davinci_mdio_default {
+ pinctrl-single,pins = <
+ /* MDIO */
+ AM33XX_IOPAD(0x948, (PIN_INPUT_PULLUP | SLEWCTRL_FAST |
MUX_MODE0)) /* mdio_data.mdio_data */
+ AM33XX_IOPAD(0x94c, (PIN_OUTPUT_PULLUP | MUX_MODE0))
/* mdio_clk.mdio_clk */
+ >;
+ };
+
+ davinci_mdio_sleep: davinci_mdio_sleep {
+ pinctrl-single,pins = <
+ /* MDIO reset value */
+ AM33XX_IOPAD(0x948, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ AM33XX_IOPAD(0x94c, (PIN_INPUT_PULLDOWN | MUX_MODE7))
+ >;
+ };
};
&i2c0 {
@@ -350,3 +413,53 @@
pinctrl-0 = <&uart3_pins_default>;
status = "okay";
};
+
+&gpio3 {
+ p4 {
+ gpio-hog;
+ gp
[PATCH] net: phy: dp83848: add dp83822 PHY support
This PHY has a compatible register set with DP83848x so
add support for it.
Acked-by: Andrew F. Davis
Signed-off-by: Roger Quadros
---
drivers/net/phy/dp83848.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/phy/dp83848.c b/drivers/net/phy/dp83848.c
index 03d54c4..800b39f 100644
--- a/drivers/net/phy/dp83848.c
+++ b/drivers/net/phy/dp83848.c
@@ -19,6 +19,7 @@
#define TI_DP83848C_PHY_ID 0x20005ca0
#define NS_DP83848C_PHY_ID 0x20005c90
#define TLK10X_PHY_ID 0x2000a210
+#define TI_DP83822_PHY_ID 0x2000a240
/* Registers */
#define DP83848_MICR 0x11 /* MII Interrupt Control Register
*/
@@ -77,6 +78,7 @@ static struct mdio_device_id __maybe_unused dp83848_tbl[] = {
{ TI_DP83848C_PHY_ID, 0xfff0 },
{ NS_DP83848C_PHY_ID, 0xfff0 },
{ TLK10X_PHY_ID, 0xfff0 },
+ { TI_DP83822_PHY_ID, 0xfff0 },
{ }
};
MODULE_DEVICE_TABLE(mdio, dp83848_tbl);
@@ -105,6 +107,7 @@ static struct phy_driver dp83848_driver[] = {
DP83848_PHY_DRIVER(TI_DP83848C_PHY_ID, "TI DP83848C 10/100 Mbps PHY"),
DP83848_PHY_DRIVER(NS_DP83848C_PHY_ID, "NS DP83848C 10/100 Mbps PHY"),
DP83848_PHY_DRIVER(TLK10X_PHY_ID, "TI TLK10X 10/100 Mbps PHY"),
+ DP83848_PHY_DRIVER(TI_DP83822_PHY_ID, "TI DP83822 10/100 Mbps PHY"),
};
module_phy_driver(dp83848_driver);
--
2.7.4
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH RFT 1/2] phylib: add device reset GPIO support
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
Re: [PATCH] phy: add support for a reset-gpio specification
On 12/05/16 17:02, Roger Quadros wrote:
> Hi,
>
> On 12/05/16 16:50, Nishanth Menon wrote:
>> On 05/12/2016 05:00 AM, Uwe Kleine-König wrote:
>>> The framework only asserts (for now) that the reset gpio is not active.
>>>
>>> Signed-off-by: Uwe Kleine-König
>>> ---
>>> Documentation/devicetree/bindings/net/phy.txt | 3 +++
>>> drivers/net/phy/phy_device.c | 8
>>> 2 files changed, 11 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/phy.txt
>>> b/Documentation/devicetree/bindings/net/phy.txt
>>> index bc1c3c8bf8fa..c00a9a894547 100644
>>> --- a/Documentation/devicetree/bindings/net/phy.txt
>>> +++ b/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: Reference to a GPIO used to reset the phy.
>>> +
>>> Example:
>>>
>>> ethernet-phy@0 {
>>> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>>> interrupt-parent = <4>;
>>> interrupts = <35 1>;
>>> reg = <0>;
>>> + reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>>> };
>>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>>> index e551f3a89cfd..7d666ab47271 100644
>>> --- a/drivers/net/phy/phy_device.c
>>> +++ b/drivers/net/phy/phy_device.c
>>> @@ -34,6 +34,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> #include
>>>
>>> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>>> struct device_driver *drv = phydev->mdio.dev.driver;
>>> struct phy_driver *phydrv = to_phy_driver(drv);
>>> int err = 0;
>>> + struct gpio_descs *reset_gpios;
>>>
>>> phydev->drv = phydrv;
>>>
>>> + /* take phy out of reset */
>>> + reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
>>> + GPIOD_OUT_LOW);
>>> + if (IS_ERR(reset_gpios))
>>> + return PTR_ERR(reset_gpios);
>>> +
>>> /* Disable the interrupt if the PHY doesn't support it
>>> * but the interrupt is still a valid one
>>> */
>>>
>>
>> This looks like the right approach to me at least: I see that TI EVMs
>> will also benefit with this approach.
>>
>
> Agreed. Although on some of our boards we actually need a RESET pulse
> to get the PHY in a sane state. I can send a patch on top for that.
>
I think I replied too early. Will phy_probe() be called at all if the
PHY is not detected on the MDIO bus?
If not then this approach is not sufficient for some of the TI boards.
e.g. am57xx-idk.
I think the reset needs to be done at MDIO bus level before it enumerates
the PHYs so that we know the PHY is in an enumerable state.
cheers,
-roger
Re: [PATCH] phy: add support for a reset-gpio specification
Hi,
On 12/05/16 16:50, Nishanth Menon wrote:
> On 05/12/2016 05:00 AM, Uwe Kleine-König wrote:
>> The framework only asserts (for now) that the reset gpio is not active.
>>
>> Signed-off-by: Uwe Kleine-König
>> ---
>> Documentation/devicetree/bindings/net/phy.txt | 3 +++
>> drivers/net/phy/phy_device.c | 8
>> 2 files changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/phy.txt
>> b/Documentation/devicetree/bindings/net/phy.txt
>> index bc1c3c8bf8fa..c00a9a894547 100644
>> --- a/Documentation/devicetree/bindings/net/phy.txt
>> +++ b/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: Reference to a GPIO used to reset the phy.
>> +
>> Example:
>>
>> ethernet-phy@0 {
>> @@ -42,4 +44,5 @@ ethernet-phy@0 {
>> interrupt-parent = <4>;
>> interrupts = <35 1>;
>> reg = <0>;
>> +reset-gpios = <&gpio1 17 GPIO_ACTIVE_LOW>;
>> };
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index e551f3a89cfd..7d666ab47271 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -34,6 +34,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include
>>
>> @@ -1569,9 +1570,16 @@ static int phy_probe(struct device *dev)
>> struct device_driver *drv = phydev->mdio.dev.driver;
>> struct phy_driver *phydrv = to_phy_driver(drv);
>> int err = 0;
>> +struct gpio_descs *reset_gpios;
>>
>> phydev->drv = phydrv;
>>
>> +/* take phy out of reset */
>> +reset_gpios = devm_gpiod_get_array_optional(dev, "reset",
>> +GPIOD_OUT_LOW);
>> +if (IS_ERR(reset_gpios))
>> +return PTR_ERR(reset_gpios);
>> +
>> /* Disable the interrupt if the PHY doesn't support it
>> * but the interrupt is still a valid one
>> */
>>
>
> This looks like the right approach to me at least: I see that TI EVMs
> will also benefit with this approach.
>
Agreed. Although on some of our boards we actually need a RESET pulse
to get the PHY in a sane state. I can send a patch on top for that.
Reviewed-by: Roger Quadros
--
cheers,
-roger
