[PATCH v3] dp83640: Ensure against premature access to PHY registers after reset

[Esben Haabendal]

From: Esben Haabendal <e...@deif.com>

The datasheet specifies a 3uS pause after performing a software
reset. The default implementation of genphy_soft_reset() does not
provide this, so implement soft_reset with the needed pause.

Signed-off-by: Esben Haabendal <e...@deif.com>
Reviewed-by: Andrew Lunn <and...@lunn.ch>
---
 drivers/net/phy/dp83640.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 654f42d00092..a6c87793d899 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1207,6 +1207,23 @@ static void dp83640_remove(struct phy_device *phydev)
kfree(dp83640);
 }
 
+static int dp83640_soft_reset(struct phy_device *phydev)
+{
+   int ret;
+
+   ret = genphy_soft_reset(phydev);
+   if (ret < 0)
+   return ret;
+
+   /* From DP83640 datasheet: "Software driver code must wait 3 us
+* following a software reset before allowing further serial MII
+* operations with the DP83640."
+*/
+   udelay(10); /* Taking udelay inaccuracy into account */
+
+   return 0;
+}
+
 static int dp83640_config_init(struct phy_device *phydev)
 {
struct dp83640_private *dp83640 = phydev->priv;
@@ -1501,6 +1518,7 @@ static struct phy_driver dp83640_driver = {
.flags  = PHY_HAS_INTERRUPT,
.probe  = dp83640_probe,
.remove = dp83640_remove,
+   .soft_reset = dp83640_soft_reset,
.config_init= dp83640_config_init,
.ack_interrupt  = dp83640_ack_interrupt,
.config_intr= dp83640_config_intr,
-- 
2.16.3

Re: [PATCH] net: phy: marvell: Enable interrupt function on LED2 pin

[Esben Haabendal]

Andrew Lunn  writes:

>> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c index
>> 0e0978d8a0eb..f03a510f1247 100644
>> --- a/drivers/net/phy/marvell.c
>> +++ b/drivers/net/phy/marvell.c
>> @@ -457,6 +457,21 @@ static int marvell_of_reg_init(struct phy_device
>> *phydev) } #endif /* CONFIG_OF_MDIO */
>>  
>> +static int m88e1318_config_intr(struct phy_device *phydev) {
>> +int err;
>> +
>> +err = marvell_config_intr(phydev);
>> +if (err)
>> +return err;
>> +
>> +/* Setup LED[2] as interrupt pin (active low) */
>> +return phy_modify(phydev, MII_88E1318S_PHY_LED_TCR,
>> +  MII_88E1318S_PHY_LED_TCR_FORCE_INT,
>> +  MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
>> +  MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);
>> 
>> Can we move this part of the code to m88e1121_config_init() ?
>> 
>> Every time whether we disable or enable the interrupts this part of code
>> will execute.
>
> Yes, doing this once would be better. But please allow the LED pin to
> be used as an LED when not using interrupts. phy_interrupt_is_valid()
> should be involved somehow.

This should be addressed in v2 of the patch which I have already posted.

/Esben

[PATCH v2] dp83640: Ensure against premature access to PHY registers after reset

[Esben Haabendal]

From: Esben Haabendal <e...@deif.com>

Signed-off-by: Esben Haabendal <e...@deif.com>
---
 drivers/net/phy/dp83640.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 654f42d00092..a6c87793d899 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1207,6 +1207,23 @@ static void dp83640_remove(struct phy_device *phydev)
kfree(dp83640);
 }
 
+static int dp83640_soft_reset(struct phy_device *phydev)
+{
+   int ret;
+
+   ret = genphy_soft_reset(phydev);
+   if (ret < 0)
+   return ret;
+
+   /* From DP83640 datasheet: "Software driver code must wait 3 us
+* following a software reset before allowing further serial MII
+* operations with the DP83640."
+*/
+   udelay(10); /* Taking udelay inaccuracy into account */
+
+   return 0;
+}
+
 static int dp83640_config_init(struct phy_device *phydev)
 {
struct dp83640_private *dp83640 = phydev->priv;
@@ -1501,6 +1518,7 @@ static struct phy_driver dp83640_driver = {
.flags  = PHY_HAS_INTERRUPT,
.probe  = dp83640_probe,
.remove = dp83640_remove,
+   .soft_reset = dp83640_soft_reset,
.config_init= dp83640_config_init,
.ack_interrupt  = dp83640_ack_interrupt,
.config_intr= dp83640_config_intr,
-- 
2.16.3

Re: [PATCH] dp83640: Ensure against premature access to PHY registers after reset

[Esben Haabendal]

David Miller <da...@davemloft.net> writes:

> From: Andrew Lunn <and...@lunn.ch>
> Date: Fri, 6 Apr 2018 16:14:10 +0200
>
>> On Fri, Apr 06, 2018 at 04:05:40PM +0200, Esben Haabendal wrote:
>>> From: Esben Haabendal <e...@deif.com>
>>> 
>>> Signed-off-by: Esben Haabendal <e...@deif.com>
>>> ---
>>>  drivers/net/phy/dp83640.c | 17 +
>>>  1 file changed, 17 insertions(+)
>>> 
>>> diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
>>> index 654f42d00092..48403170096a 100644
>>> --- a/drivers/net/phy/dp83640.c
>>> +++ b/drivers/net/phy/dp83640.c
>>> @@ -1207,6 +1207,22 @@ static void dp83640_remove(struct phy_device *phydev)
>>> kfree(dp83640);
>>>  }
>>>  
>>> +static int dp83640_soft_reset(struct phy_device *phydev)
>>> +{
>>> +   int ret;
>>> +
>>> +   ret = genphy_soft_reset(phydev);
>>> +   if (ret < 0)
>>> +   return ret;
>>> +
>>> +   /* From DP83640 datasheet: "Software driver code must wait 3 us
>>> +* following a software reset before allowing further serial MII
>>> +* operations with the DP83640." */
>>> +   udelay(3);
>> 
>> Hi Esben
>> 
>> The accuracy of udelay() is not guaranteed. So you probably want to be
>> a bit pessimistic, and use 10.

Ok, will do.

/Esben

[PATCH] dp83640: Ensure against premature access to PHY registers after reset

[Esben Haabendal]

From: Esben Haabendal <e...@deif.com>

Signed-off-by: Esben Haabendal <e...@deif.com>
---
 drivers/net/phy/dp83640.c | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 654f42d00092..48403170096a 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1207,6 +1207,22 @@ static void dp83640_remove(struct phy_device *phydev)
kfree(dp83640);
 }
 
+static int dp83640_soft_reset(struct phy_device *phydev)
+{
+   int ret;
+
+   ret = genphy_soft_reset(phydev);
+   if (ret < 0)
+   return ret;
+
+   /* From DP83640 datasheet: "Software driver code must wait 3 us
+* following a software reset before allowing further serial MII
+* operations with the DP83640." */
+   udelay(3);
+
+   return 0;
+}
+
 static int dp83640_config_init(struct phy_device *phydev)
 {
struct dp83640_private *dp83640 = phydev->priv;
@@ -1501,6 +1517,7 @@ static struct phy_driver dp83640_driver = {
.flags  = PHY_HAS_INTERRUPT,
.probe  = dp83640_probe,
.remove = dp83640_remove,
+   .soft_reset = dp83640_soft_reset,
.config_init= dp83640_config_init,
.ack_interrupt  = dp83640_ack_interrupt,
.config_intr= dp83640_config_intr,
-- 
2.16.3

[PATCH] ARM: dts: ls1021a: Specify TBIPA register address

[Esben Haabendal]

From: Esben Haabendal <e...@deif.com>

The current (mildly evil) fsl_pq_mdio code uses an undocumented shadow of
the TBIPA register on LS1021A, which happens to be read-only.
Changing TBI PHY address therefore does not work on LS1021A.

The real (and documented) address of the TBIPA registere lies in the eTSEC
block and not in MDIO/MII, which is read/write, so using that fixes
the problem.

Signed-off-by: Esben Haabendal <e...@deif.com>
---
 arch/arm/boot/dts/ls1021a.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/ls1021a.dtsi b/arch/arm/boot/dts/ls1021a.dtsi
index c31dad98f989..728e206009ea 100644
--- a/arch/arm/boot/dts/ls1021a.dtsi
+++ b/arch/arm/boot/dts/ls1021a.dtsi
@@ -587,7 +587,8 @@
device_type = "mdio";
#address-cells = <1>;
#size-cells = <0>;
-   reg = <0x0 0x2d24000 0x0 0x4000>;
+   reg = <0x0 0x2d24000 0x0 0x4000>,
+ <0x0 0x2d10030 0x0 0x4>;
};
 
ptp_clock@2d10e00 {
-- 
2.16.3

[PATCH 1/2] net/fsl_pq_mdio: Allow explicit speficition of TBIPA address

[Esben Haabendal]

From: Esben Haabendal <e...@deif.com>

This introduces a simpler and generic method for for finding (and mapping)
the TBIPA register.

Instead of relying of complicated logic for finding the TBIPA register
address based on the MDIO or MII register block base
address, which even in some cases relies on undocumented shadow registers,
a second "reg" entry for the mdio bus devicetree node specifies the TBIPA
register.

Backwards compatibility is kept, as the existing logic is applied when
only a single "reg" mapping is specified.

Signed-off-by: Esben Haabendal <e...@deif.com>
---
 .../devicetree/bindings/net/fsl-tsec-phy.txt   |  6 ++-
 drivers/net/ethernet/freescale/fsl_pq_mdio.c   | 50 +++---
 2 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt 
b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
index 594982c6b9f9..79bf352e659c 100644
--- a/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
+++ b/Documentation/devicetree/bindings/net/fsl-tsec-phy.txt
@@ -6,7 +6,11 @@ the definition of the PHY node in booting-without-of.txt for 
an example
 of how to define a PHY.
 
 Required properties:
-  - reg : Offset and length of the register set for the device
+  - reg : Offset and length of the register set for the device, and optionally
+  the offset and length of the TBIPA register (TBI PHY address
+ register).  If TBIPA register is not specified, the driver will
+ attempt to infer it from the register set specified (your mileage may
+ vary).
   - compatible : Should define the compatible device type for the
 mdio. Currently supported strings/devices are:
- "fsl,gianfar-tbi"
diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c 
b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 80ad16acf0f1..ac2c3f6a12bc 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -377,6 +377,38 @@ static const struct of_device_id fsl_pq_mdio_match[] = {
 };
 MODULE_DEVICE_TABLE(of, fsl_pq_mdio_match);
 
+static void set_tbipa(const u32 tbipa_val, struct platform_device *pdev,
+ uint32_t __iomem * (*get_tbipa)(void __iomem *),
+ void __iomem *reg_map, struct resource *reg_res)
+{
+   struct device_node *np = pdev->dev.of_node;
+   uint32_t __iomem *tbipa;
+   bool tbipa_mapped;
+
+   tbipa = of_iomap(np, 1);
+   if (tbipa) {
+   tbipa_mapped = true;
+   } else {
+   tbipa_mapped = false;
+   tbipa = (*get_tbipa)(reg_map);
+
+   /*
+* Add consistency check to make sure TBI is contained within
+* the mapped range (not because we would get a segfault,
+* rather to catch bugs in computing TBI address). Print error
+* message but continue anyway.
+*/
+   if ((void *)tbipa > reg_map + resource_size(reg_res) - 4)
+   dev_err(>dev, "invalid register map (should be at 
least 0x%04zx to contain TBI address)\n",
+   ((void *)tbipa - reg_map) + 4);
+   }
+
+   iowrite32be(be32_to_cpu(tbipa_val), tbipa);
+
+   if (tbipa_mapped)
+   iounmap(tbipa);
+}
+
 static int fsl_pq_mdio_probe(struct platform_device *pdev)
 {
const struct of_device_id *id =
@@ -450,8 +482,6 @@ static int fsl_pq_mdio_probe(struct platform_device *pdev)
 
if (tbi) {
const u32 *prop = of_get_property(tbi, "reg", NULL);
-   uint32_t __iomem *tbipa;
-
if (!prop) {
dev_err(>dev,
"missing 'reg' property in node %pOF\n",
@@ -459,20 +489,8 @@ static int fsl_pq_mdio_probe(struct platform_device *pdev)
err = -EBUSY;
goto error;
}
-
-   tbipa = data->get_tbipa(priv->map);
-
-   /*
-* Add consistency check to make sure TBI is contained
-* within the mapped range (not because we would get a
-* segfault, rather to catch bugs in computing TBI
-* address). Print error message but continue anyway.
-*/
-   if ((void *)tbipa > priv->map + resource_size() - 4)
-   dev_err(>dev, "invalid register map 
(should be at least 0x%04zx to contain TBI address)\n",
-   ((void *)tbipa - priv->map) + 4);
-
-   iowrite32be(be32_to_cpup(prop), tbipa);
+   set_tbipa(*prop, pdev,
+ data->get_tbipa, priv->map, );
}
}
 
-- 
2.16.3

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Esben Haabendal]

David Miller  writes:

> From: Andrew Lunn 
> Date: Thu, 5 Apr 2018 22:40:49 +0200
>
>> Or could it still contain whatever state the last boot of Linux, or
>> maybe the bootloader, left the PHY in?
>
> Right, this is my concern as well.

I don't think that should happen.
With config_init() being called (in phy_init_hw()) after soft_reset(),
any state set by software should be cleared.

>From DP83620 datasheet description of what happens when BMCR_RESET is
set:

The software reset will reset the device such that all registers
will be reset to default values and the hardware configuration
values will be maintained.

But something else that could be a concern is the risk that there is
boards out there with wrong hardware configuration, which works with
current Linux (because it ignores hardware configuration).  Such designs
could break with this patch.

If we need to safeguard against that, maybe we could just keep the
genphy_read_config() function in the kernel, and let board specific code
use it as a phy_fixup where hardware configuration is to be respected.

Would that be a better approach?

/Esben

[PATCH v2] net: phy: marvell: Enable interrupt function on LED2 pin

[Esben Haabendal]

From: Esben Haabendal <e...@deif.com>

The LED2[2]/INTn pin on Marvell 88E1318S as well as 88E1510/12/14/18 needs
to be configured to be usable as interrupt not only when WOL is enabled,
but whenever we rely on interrupts from the PHY.

Signed-off-by: Esben Haabendal <e...@deif.com>
Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk>
---
 drivers/net/phy/marvell.c | 20 ++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 0e0978d8a0eb..a6ad0255c512 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -828,6 +828,22 @@ static int m88e1121_config_init(struct phy_device *phydev)
return marvell_config_init(phydev);
 }
 
+static int m88e1318_config_init(struct phy_device *phydev)
+{
+   if (phy_interrupt_is_valid(phydev)) {
+   int err = phy_modify_paged(
+   phydev, MII_MARVELL_LED_PAGE,
+   MII_88E1318S_PHY_LED_TCR,
+   MII_88E1318S_PHY_LED_TCR_FORCE_INT,
+   MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
+   MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);
+   if (err < 0)
+   return err;
+   }
+
+   return m88e1121_config_init(phydev);
+}
+
 static int m88e1510_config_init(struct phy_device *phydev)
 {
int err;
@@ -870,7 +886,7 @@ static int m88e1510_config_init(struct phy_device *phydev)
phydev->advertising &= ~pause;
}
 
-   return m88e1121_config_init(phydev);
+   return m88e1318_config_init(phydev);
 }
 
 static int m88e1118_config_aneg(struct phy_device *phydev)
@@ -2086,7 +2102,7 @@ static struct phy_driver marvell_drivers[] = {
.features = PHY_GBIT_FEATURES,
.flags = PHY_HAS_INTERRUPT,
.probe = marvell_probe,
-   .config_init = _config_init,
+   .config_init = _config_init,
.config_aneg = _config_aneg,
.read_status = _read_status,
.ack_interrupt = _ack_interrupt,
-- 
2.16.3

Re: [PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[Esben Haabendal]

Florian Fainelli <f.faine...@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haaben...@gmail.com wrote:
>> From: Esben Haabendal <e...@deif.com>
>> 
>> Add a function for use in PHY driver probe functions, reading current
>> autoneg, speed and duplex configuration from BMCR register.
>> 
>> Useful for PHY that supports hardware strapped configuration, enabling
>> Linux to respect that configuration (i.e. strapped non-autoneg
>> configuration).
>> 
>> Signed-off-by: Esben Haabendal <e...@deif.com>
>> Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk>
>> ---
>>  drivers/net/phy/phy_device.c | 41 +
>>  include/linux/phy.h  |  1 +
>>  2 files changed, 42 insertions(+)
>> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 74664a6c0cdc..cc52ff2a2344 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1673,6 +1673,47 @@ int genphy_config_init(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL(genphy_config_init);
>>  
>> +/**
>> + * genphy_read_config - read configuration from PHY
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: Reads MII_BMCR and sets phydev autoneg, speed and duplex
>> + * accordingly.  For use in driver probe functions, to respect strapped
>> + * configuration settings.
>> + */
>> +int genphy_read_config(struct phy_device *phydev)
>
> This duplicates what already exists, in part at least within
> genphy_read_status() can you find a way to use that?

Make a small static function for updating duplex and speed fields from a
BMCR value.  It will not be big re-use, but it would make sense.  I will
do that in next patch version.

/Esben

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Esben Haabendal]

Florian Fainelli <f.faine...@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haaben...@gmail.com wrote:
>> From: Esben Haabendal <e...@deif.com>
>> 
>> Read configration settings, to allow automatic forced speed/duplex setup
>> by hardware strapping.
>
> OK but why? What problem is this solving for you?

It is ensuring that the PHY is configured according to the HW design.
If the HW design has set the strap configuration to fx. fixed 100 Mbit
full-duplex, this avoids Linux configuring it for auto-negotiation.

> In general, we do not really want to preserve too much of what the PHY
> has been previously configured with,

Even when this "previous" configuration is the configuration selected by
the board configuration?

> provided that the PHY driver can re-instate these configuration
> values.

This is sort of what I try to do here.  The PHY driver needs to check
the BMCR register to figure out what the strapped configuration was.
Without that, it is necessary for software configuration to duplicate
the exact same configuration as is encoded in the strap configuration in
order for the PHY to be configured as it is supposed to.

> I just wonder how this can be robust when you connect this PHY with
> auto-negotiation disabled to a peer that expects a set of link
> parameters not covered by the default advertisement values?

I assume it will fail just as it will if you use ethtool to configure
the PHY wrongly.

> This really looks like a recipe for disaster when you could just
> disable auto-negotiation with ethtool.

The current PHY driver approach to always default to enable
auto-negotation, and then allow user-space to configure it differently
with ethtool.

With this patch, the default configuration is chosen based on the strap
configuration, but user-space can still change the configuration with
ethtool if needed / desired.

I don't think it is such a big change.

Bringing up the PHY with a default configuration according to HW
strapping instead of an in-kernel SW hard-coded value sounds like a good
idea to me.

/Esben

Re: [PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[Esben Haabendal]

Florian Fainelli <f.faine...@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haaben...@gmail.com wrote:
>> From: Esben Haabendal <e...@deif.com>
>> 
>> Read configration settings, to allow automatic forced speed/duplex setup
>> by hardware strapping.
>
> OK but why? What problem is this solving for you?

It is ensuring that the PHY is configured according to the HW design.
If the HW design has set the strap configuration to fx. fixed 100 Mbit
full-duplex, this avoids Linux configuring it for auto-negotiation.

> In general, we do not really want to preserve too much of what the PHY
> has been previously configured with,

Even when this "previous" configuration is the configuration selected by
the board configuration?

> provided that the PHY driver can re-instate these configuration
> values.

This is sort of what I try to do here.  The PHY driver needs to check
the BMCR register to figure out what the strapped configuration was.
Without that, it is necessary for software configuration to duplicate
the exact same configuration as is encoded in the strap configuration in
order for the PHY to be configured as it is supposed to.

> I just wonder how this can be robust when you connect this PHY with
> auto-negotiation disabled to a peer that expects a set of link
> parameters not covered by the default advertisement values?

I assume it will fail just as it will if you use ethtool to configure
the PHY wrongly.

> This really looks like a recipe for disaster when you could just
> disable auto-negotiation with ethtool.

The current PHY driver approach to always default to enable
auto-negotation, and then allow user-space to configure it differently
with ethtool.

With this patch, the default configuration is chosen based on the strap
configuration, but user-space can still change the configuration with
ethtool if needed / desired.

I don't think it is such a big change.

Bringing up the PHY with a default configuration according to HW
strapping instead of an in-kernel SW hard-coded value sounds like a good
idea to me.

/Esben

Re: [PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[Esben Haabendal]

Florian Fainelli <f.faine...@gmail.com> writes:

> On 04/05/2018 04:44 AM, esben.haaben...@gmail.com wrote:
>> From: Esben Haabendal <e...@deif.com>
>> 
>> Add a function for use in PHY driver probe functions, reading current
>> autoneg, speed and duplex configuration from BMCR register.
>> 
>> Useful for PHY that supports hardware strapped configuration, enabling
>> Linux to respect that configuration (i.e. strapped non-autoneg
>> configuration).
>> 
>> Signed-off-by: Esben Haabendal <e...@deif.com>
>> Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk>
>> ---
>>  drivers/net/phy/phy_device.c | 41 +
>>  include/linux/phy.h  |  1 +
>>  2 files changed, 42 insertions(+)
>> 
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 74664a6c0cdc..cc52ff2a2344 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1673,6 +1673,47 @@ int genphy_config_init(struct phy_device *phydev)
>>  }
>>  EXPORT_SYMBOL(genphy_config_init);
>>  
>> +/**
>> + * genphy_read_config - read configuration from PHY
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: Reads MII_BMCR and sets phydev autoneg, speed and duplex
>> + * accordingly.  For use in driver probe functions, to respect strapped
>> + * configuration settings.
>> + */
>> +int genphy_read_config(struct phy_device *phydev)
>
> This duplicates what already exists, in part at least within
> genphy_read_status() can you find a way to use that?

Make a small static function for updating duplex and speed fields from a
BMCR value.  It will not be big re-use, but it would make sense.  I will
do that in next patch version.

/Esben

[PATCH] net: phy: marvell: Enable interrupt function on LED2 pin

[Esben Haabendal]

From: Esben Haabendal <e...@deif.com>

The LED2[2]/INTn pin on Marvell 88E1318S as well as 88E1510/12/14/18 needs
to be configured to be usable as interrupt not only when WOL is enabled,
but whenever we rely on interrupts from the PHY.

Signed-off-by: Esben Haabendal <e...@deif.com>
Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk>
---
 drivers/net/phy/marvell.c | 19 +--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 0e0978d8a0eb..f03a510f1247 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -457,6 +457,21 @@ static int marvell_of_reg_init(struct phy_device *phydev)
 }
 #endif /* CONFIG_OF_MDIO */
 
+static int m88e1318_config_intr(struct phy_device *phydev)
+{
+   int err;
+
+   err = marvell_config_intr(phydev);
+   if (err)
+   return err;
+
+   /* Setup LED[2] as interrupt pin (active low) */
+   return phy_modify(phydev, MII_88E1318S_PHY_LED_TCR,
+ MII_88E1318S_PHY_LED_TCR_FORCE_INT,
+ MII_88E1318S_PHY_LED_TCR_INTn_ENABLE |
+ MII_88E1318S_PHY_LED_TCR_INT_ACTIVE_LOW);
+}
+
 static int m88e1121_config_aneg_rgmii_delays(struct phy_device *phydev)
 {
int mscr;
@@ -2090,7 +2105,7 @@ static struct phy_driver marvell_drivers[] = {
.config_aneg = _config_aneg,
.read_status = _read_status,
.ack_interrupt = _ack_interrupt,
-   .config_intr = _config_intr,
+   .config_intr = _config_intr,
.did_interrupt = _did_interrupt,
.get_wol = _get_wol,
.set_wol = _set_wol,
@@ -2189,7 +2204,7 @@ static struct phy_driver marvell_drivers[] = {
.config_aneg = _config_aneg,
.read_status = _read_status,
.ack_interrupt = _ack_interrupt,
-   .config_intr = _config_intr,
+   .config_intr = _config_intr,
.did_interrupt = _did_interrupt,
.get_wol = _get_wol,
.set_wol = _set_wol,
-- 
2.16.3

[PATCH 1/2] net: phy: Helper function for reading strapped configuration values

[esben . haabendal]

From: Esben Haabendal <e...@deif.com>

Add a function for use in PHY driver probe functions, reading current
autoneg, speed and duplex configuration from BMCR register.

Useful for PHY that supports hardware strapped configuration, enabling
Linux to respect that configuration (i.e. strapped non-autoneg
configuration).

Signed-off-by: Esben Haabendal <e...@deif.com>
Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk>
---
 drivers/net/phy/phy_device.c | 41 +
 include/linux/phy.h  |  1 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 74664a6c0cdc..cc52ff2a2344 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1673,6 +1673,47 @@ int genphy_config_init(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(genphy_config_init);
 
+/**
+ * genphy_read_config - read configuration from PHY
+ * @phydev: target phy_device struct
+ *
+ * Description: Reads MII_BMCR and sets phydev autoneg, speed and duplex
+ * accordingly.  For use in driver probe functions, to respect strapped
+ * configuration settings.
+ */
+int genphy_read_config(struct phy_device *phydev)
+{
+   int bmcr;
+
+   bmcr = phy_read(phydev, MII_BMCR);
+   if (bmcr < 0)
+   return bmcr;
+
+   if (BMCR_ANENABLE & bmcr) {
+   phydev->autoneg = AUTONEG_ENABLE;
+
+   phydev->speed = 0;
+   phydev->duplex = -1;
+   } else {
+   phydev->autoneg = AUTONEG_DISABLE;
+
+   if (bmcr & BMCR_FULLDPLX)
+   phydev->duplex = DUPLEX_FULL;
+   else
+   phydev->duplex = DUPLEX_HALF;
+
+   if (bmcr & BMCR_SPEED1000)
+   phydev->speed = SPEED_1000;
+   else if (bmcr & BMCR_SPEED100)
+   phydev->speed = SPEED_100;
+   else
+   phydev->speed = SPEED_10;
+   }
+
+   return 0;
+}
+EXPORT_SYMBOL(genphy_read_config);
+
 /* This is used for the phy device which doesn't support the MMD extended
  * register access, but it does have side effect when we are trying to access
  * the MMD register via indirect method.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7c4c2379e010..5a8821c8105d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -970,6 +970,7 @@ void phy_attached_info(struct phy_device *phydev);
 
 /* Clause 22 PHY */
 int genphy_config_init(struct phy_device *phydev);
+int genphy_read_config(struct phy_device *phydev);
 int genphy_setup_forced(struct phy_device *phydev);
 int genphy_restart_aneg(struct phy_device *phydev);
 int genphy_config_aneg(struct phy_device *phydev);
-- 
2.16.3

[PATCH 2/2] net: phy: dp83640: Read strapped configuration settings

[esben . haabendal]

From: Esben Haabendal <e...@deif.com>

Read configration settings, to allow automatic forced speed/duplex setup
by hardware strapping.

Signed-off-by: Esben Haabendal <e...@deif.com>
Cc: Rasmus Villemoes <rasmus.villem...@prevas.dk>
---
 drivers/net/phy/dp83640.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 654f42d00092..01e21b4998ad 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1134,6 +1134,10 @@ static int dp83640_probe(struct phy_device *phydev)
if (!dp83640)
goto no_memory;
 
+   err = genphy_read_config(phydev);
+   if (err)
+   goto no_config;
+
dp83640->phydev = phydev;
INIT_DELAYED_WORK(>ts_work, rx_timestamp_work);
 
@@ -1166,6 +1170,7 @@ static int dp83640_probe(struct phy_device *phydev)
 
 no_register:
clock->chosen = NULL;
+no_config:
kfree(dp83640);
 no_memory:
dp83640_clock_put(clock);
-- 
2.16.3

RE: [PATCH 6/9] fs_enet: Be an of_platform device when CONFIG_PPC_CPM_NEW_BINDING is set.

[Esben Haabendal]

Hi Scott,

A minor error handling bug

 + const u32 *data = of_get_property(np, phy-handle, len);
 + if (!data || len != 4)
 + return -EINVAL;
 +
 + phynode = of_find_node_by_phandle(*data);
 + if (!phynode)
 + return -EINVAL;
 +
 + mdionode = of_get_parent(phynode);
 + if (!phynode)

if (!mdionode)

 + goto out_put_phy;

Best regards,
Esben

-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html