Hi all,
This patch series reduces the duplication among 10G PHY drivers that just
essentially stub most functions, but do that while replicating what the existing
generic functions do.
Florian Fainelli (5):
net: phy: aquantia: Utilize genphy_c45_aneg_done()
net: phy: Export gen10g_* functions
net: phy: teranetics: Utilize generic functions
net: phy: cortina: Utilize generic functions
net: phy: marvell10g: Utilize gen10g_soft_reset()
drivers/net/phy/aquantia.c | 20 ++++++--------------
drivers/net/phy/cortina.c | 18 +++---------------
drivers/net/phy/marvell10g.c | 7 +------
drivers/net/phy/phy-c45.c | 18 ++++++++++++------
drivers/net/phy/teranetics.c | 30 +++++-------------------------
include/linux/phy.h | 8 ++++++++
6 files changed, 35 insertions(+), 66 deletions(-)
--
2.14.1
cortina_soft_reset() does the same thing as gen10g_soft_reset(), and
cortina_config_aneg() is actually doing what gen10g_config_init() does
for 10G capable PHYs.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/cortina.c | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/net/phy/cortina.c b/drivers/net/phy/cortina.c
index 9442db221834..9afbeb8b3667 100644
--- a/drivers/net/phy/cortina.c
+++ b/drivers/net/phy/cortina.c
@@ -30,14 +30,6 @@ static int cortina_read_reg(struct phy_device *phydev, u16 regnum)
MII_ADDR_C45 | regnum);
}
-static int cortina_config_aneg(struct phy_device *phydev)
-{
- phydev->supported = SUPPORTED_10000baseT_Full;
- phydev->advertising = SUPPORTED_10000baseT_Full;
-
- return 0;
-}
-
static int cortina_read_status(struct phy_device *phydev)
{
int gpio_int_status, ret = 0;
@@ -61,11 +53,6 @@ static int cortina_read_status(struct phy_device *phydev)
return ret;
}
-static int cortina_soft_reset(struct phy_device *phydev)
-{
- return 0;
-}
-
static int cortina_probe(struct phy_device *phydev)
{
u32 phy_id = 0;
@@ -101,9 +88,10 @@ static struct phy_driver cortina_driver[] = {
.phy_id = PHY_ID_CS4340,
.phy_id_mask = 0xffffffff,
.name = "Cortina CS4340",
- .config_aneg = cortina_config_aneg,
+ .config_init = gen10g_config_init,
+ .config_aneg = gen10g_config_aneg,
.read_status = cortina_read_status,
- .soft_reset = cortina_soft_reset,
+ .soft_reset = gen10g_soft_reset,
.probe = cortina_probe,
},
};
--
2.14.1
We do the same thing as the generic function: nothing, so utilize it.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/marvell10g.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 8a0bd98fdec7..da014eae1476 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -75,11 +75,6 @@ static int mv3310_probe(struct phy_device *phydev)
* Resetting the MV88X3310 causes it to become non-responsive. Avoid
* setting the reset bit(s).
*/
-static int mv3310_soft_reset(struct phy_device *phydev)
-{
- return 0;
-}
-
static int mv3310_config_init(struct phy_device *phydev)
{
__ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
@@ -377,7 +372,7 @@ static struct phy_driver mv3310_drivers[] = {
SUPPORTED_10000baseT_Full |
SUPPORTED_Backplane,
.probe = mv3310_probe,
- .soft_reset = mv3310_soft_reset,
+ .soft_reset = gen10g_soft_reset,
.config_init = mv3310_config_init,
.config_aneg = mv3310_config_aneg,
.aneg_done = mv3310_aneg_done,
--
2.14.1
Update teranetics_aneg_done() to use genphy_c45_aneg_done() instead of
duplicating that code, and switch to gen10g_* functions where
appropriate instead of maintaining identical copies doing nothing.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/teranetics.c | 30 +++++-------------------------
1 file changed, 5 insertions(+), 25 deletions(-)
diff --git a/drivers/net/phy/teranetics.c b/drivers/net/phy/teranetics.c
index fb2cef764e9a..09f6829cfcc8 100644
--- a/drivers/net/phy/teranetics.c
+++ b/drivers/net/phy/teranetics.c
@@ -34,19 +34,6 @@ MODULE_LICENSE("GPL v2");
MDIO_PHYXS_LNSTAT_SYNC3 | \
MDIO_PHYXS_LNSTAT_ALIGN)
-static int teranetics_config_init(struct phy_device *phydev)
-{
- phydev->supported = SUPPORTED_10000baseT_Full;
- phydev->advertising = SUPPORTED_10000baseT_Full;
-
- return 0;
-}
-
-static int teranetics_soft_reset(struct phy_device *phydev)
-{
- return 0;
-}
-
static int teranetics_aneg_done(struct phy_device *phydev)
{
int reg;
@@ -54,19 +41,12 @@ static int teranetics_aneg_done(struct phy_device *phydev)
/* auto negotiation state can only be checked when using copper
* port, if using fiber port, just lie it's done.
*/
- if (!phy_read_mmd(phydev, MDIO_MMD_VEND1, 93)) {
- reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
- return (reg < 0) ? reg : (reg & BMSR_ANEGCOMPLETE);
- }
+ if (!phy_read_mmd(phydev, MDIO_MMD_VEND1, 93))
+ return genphy_c45_aneg_done(phydev);
return 1;
}
-static int teranetics_config_aneg(struct phy_device *phydev)
-{
- return 0;
-}
-
static int teranetics_read_status(struct phy_device *phydev)
{
int reg;
@@ -102,10 +82,10 @@ static struct phy_driver teranetics_driver[] = {
.phy_id = PHY_ID_TN2020,
.phy_id_mask = 0xffffffff,
.name = "Teranetics TN2020",
- .soft_reset = teranetics_soft_reset,
+ .soft_reset = gen10g_soft_reset,
.aneg_done = teranetics_aneg_done,
- .config_init = teranetics_config_init,
- .config_aneg = teranetics_config_aneg,
+ .config_init = gen10g_config_init,
+ .config_aneg = gen10g_config_aneg,
.read_status = teranetics_read_status,
.match_phy_device = teranetics_match_phy_device,
},
--
2.14.1
The driver duplicates what the generic function does, so use the generic
function intead.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/aquantia.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/net/phy/aquantia.c b/drivers/net/phy/aquantia.c
index e8ae50e1255e..319edc9c8ec7 100644
--- a/drivers/net/phy/aquantia.c
+++ b/drivers/net/phy/aquantia.c
@@ -38,14 +38,6 @@ static int aquantia_config_aneg(struct phy_device *phydev)
return 0;
}
-static int aquantia_aneg_done(struct phy_device *phydev)
-{
- int reg;
-
- reg = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
- return (reg < 0) ? reg : (reg & BMSR_ANEGCOMPLETE);
-}
-
static int aquantia_config_intr(struct phy_device *phydev)
{
int err;
@@ -125,7 +117,7 @@ static struct phy_driver aquantia_driver[] = {
.name = "Aquantia AQ1202",
.features = PHY_AQUANTIA_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .aneg_done = aquantia_aneg_done,
+ .aneg_done = genphy_c45_aneg_done,
.config_aneg = aquantia_config_aneg,
.config_intr = aquantia_config_intr,
.ack_interrupt = aquantia_ack_interrupt,
@@ -137,7 +129,7 @@ static struct phy_driver aquantia_driver[] = {
.name = "Aquantia AQ2104",
.features = PHY_AQUANTIA_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .aneg_done = aquantia_aneg_done,
+ .aneg_done = genphy_c45_aneg_done,
.config_aneg = aquantia_config_aneg,
.config_intr = aquantia_config_intr,
.ack_interrupt = aquantia_ack_interrupt,
@@ -149,7 +141,7 @@ static struct phy_driver aquantia_driver[] = {
.name = "Aquantia AQR105",
.features = PHY_AQUANTIA_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .aneg_done = aquantia_aneg_done,
+ .aneg_done = genphy_c45_aneg_done,
.config_aneg = aquantia_config_aneg,
.config_intr = aquantia_config_intr,
.ack_interrupt = aquantia_ack_interrupt,
@@ -161,7 +153,7 @@ static struct phy_driver aquantia_driver[] = {
.name = "Aquantia AQR106",
.features = PHY_AQUANTIA_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .aneg_done = aquantia_aneg_done,
+ .aneg_done = genphy_c45_aneg_done,
.config_aneg = aquantia_config_aneg,
.config_intr = aquantia_config_intr,
.ack_interrupt = aquantia_ack_interrupt,
@@ -173,7 +165,7 @@ static struct phy_driver aquantia_driver[] = {
.name = "Aquantia AQR107",
.features = PHY_AQUANTIA_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .aneg_done = aquantia_aneg_done,
+ .aneg_done = genphy_c45_aneg_done,
.config_aneg = aquantia_config_aneg,
.config_intr = aquantia_config_intr,
.ack_interrupt = aquantia_ack_interrupt,
@@ -185,7 +177,7 @@ static struct phy_driver aquantia_driver[] = {
.name = "Aquantia AQR405",
.features = PHY_AQUANTIA_FEATURES,
.flags = PHY_HAS_INTERRUPT,
- .aneg_done = aquantia_aneg_done,
+ .aneg_done = genphy_c45_aneg_done,
.config_aneg = aquantia_config_aneg,
.config_intr = aquantia_config_intr,
.ack_interrupt = aquantia_ack_interrupt,
--
2.14.1
In order to remove a fair amount of duplication in the different 10G PHY
drivers, export all gen10g_* functions to be able to make use of those.
Signed-off-by: Florian Fainelli <[email protected]>
---
drivers/net/phy/phy-c45.c | 18 ++++++++++++------
include/linux/phy.h | 8 ++++++++
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index a4576859afae..f882dfae8e41 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -268,12 +268,13 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
/* The gen10g_* functions are the old Clause 45 stub */
-static int gen10g_config_aneg(struct phy_device *phydev)
+int gen10g_config_aneg(struct phy_device *phydev)
{
return 0;
}
+EXPORT_SYMBOL_GPL(gen10g_config_aneg);
-static int gen10g_read_status(struct phy_device *phydev)
+int gen10g_read_status(struct phy_device *phydev)
{
u32 mmd_mask = phydev->c45_ids.devices_in_package;
int ret;
@@ -291,14 +292,16 @@ static int gen10g_read_status(struct phy_device *phydev)
return 0;
}
+EXPORT_SYMBOL_GPL(gen10g_read_status);
-static int gen10g_soft_reset(struct phy_device *phydev)
+int gen10g_soft_reset(struct phy_device *phydev)
{
/* Do nothing for now */
return 0;
}
+EXPORT_SYMBOL_GPL(gen10g_soft_reset);
-static int gen10g_config_init(struct phy_device *phydev)
+int gen10g_config_init(struct phy_device *phydev)
{
/* Temporarily just say we support everything */
phydev->supported = SUPPORTED_10000baseT_Full;
@@ -306,16 +309,19 @@ static int gen10g_config_init(struct phy_device *phydev)
return 0;
}
+EXPORT_SYMBOL_GPL(gen10g_config_init);
-static int gen10g_suspend(struct phy_device *phydev)
+int gen10g_suspend(struct phy_device *phydev)
{
return 0;
}
+EXPORT_SYMBOL_GPL(gen10g_suspend);
-static int gen10g_resume(struct phy_device *phydev)
+int gen10g_resume(struct phy_device *phydev)
{
return 0;
}
+EXPORT_SYMBOL_GPL(gen10g_resume);
struct phy_driver genphy_10g_driver = {
.phy_id = 0xffffffff,
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 5a0c3e53e7c2..612637e7584a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -994,6 +994,14 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev);
int genphy_c45_an_disable_aneg(struct phy_device *phydev);
int genphy_c45_read_mdix(struct phy_device *phydev);
+/* The gen10g_* functions are the old Clause 45 stub */
+int gen10g_config_aneg(struct phy_device *phydev);
+int gen10g_read_status(struct phy_device *phydev);
+int gen10g_soft_reset(struct phy_device *phydev);
+int gen10g_config_init(struct phy_device *phydev);
+int gen10g_suspend(struct phy_device *phydev);
+int gen10g_resume(struct phy_device *phydev);
+
static inline int phy_read_status(struct phy_device *phydev)
{
if (!phydev->drv)
--
2.14.1
On Wed, Feb 28, 2018 at 11:36:09AM -0800, Florian Fainelli wrote:
> In order to remove a fair amount of duplication in the different 10G PHY
> drivers, export all gen10g_* functions to be able to make use of those.
The gen10g functions tend to be barely functional - for example,
gen10g_soft_reset() doesn't do the soft-reset thing, which could be
argued to be a bug, as there's a generic way to do that.
However, doing that for the Marvell 10G driver would be a big mistake.
The PHY crashes if you try to set the reset bit in any of the control
register 1s.
--
Russell King
On Wed, Feb 28, 2018 at 11:36:12AM -0800, Florian Fainelli wrote:
> We do the same thing as the generic function: nothing, so utilize it.
>
> Signed-off-by: Florian Fainelli <[email protected]>
> ---
> drivers/net/phy/marvell10g.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 8a0bd98fdec7..da014eae1476 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -75,11 +75,6 @@ static int mv3310_probe(struct phy_device *phydev)
> * Resetting the MV88X3310 causes it to become non-responsive. Avoid
> * setting the reset bit(s).
> */
> -static int mv3310_soft_reset(struct phy_device *phydev)
> -{
> - return 0;
> -}
> -
You do realise that getting rid of that function makes a nonsense of the
comment above it - and removing the comment along with the function gets
rid of the very important reason _why_ we have en empty reset method?
> static int mv3310_config_init(struct phy_device *phydev)
> {
> __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
> @@ -377,7 +372,7 @@ static struct phy_driver mv3310_drivers[] = {
> SUPPORTED_10000baseT_Full |
> SUPPORTED_Backplane,
> .probe = mv3310_probe,
> - .soft_reset = mv3310_soft_reset,
> + .soft_reset = gen10g_soft_reset,
> .config_init = mv3310_config_init,
> .config_aneg = mv3310_config_aneg,
> .aneg_done = mv3310_aneg_done,
> --
> 2.14.1
>
--
Russell King
ARM architecture Linux Kernel maintainer
Florian,
On Wed, Feb 28, 2018 at 11:44 AM, Russell King <[email protected]> wrote:
> On Wed, Feb 28, 2018 at 11:36:12AM -0800, Florian Fainelli wrote:
>> We do the same thing as the generic function: nothing, so utilize it.
>>
>> Signed-off-by: Florian Fainelli <[email protected]>
>> ---
>> drivers/net/phy/marvell10g.c | 7 +------
>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>> index 8a0bd98fdec7..da014eae1476 100644
>> --- a/drivers/net/phy/marvell10g.c
>> +++ b/drivers/net/phy/marvell10g.c
>> @@ -75,11 +75,6 @@ static int mv3310_probe(struct phy_device *phydev)
>> * Resetting the MV88X3310 causes it to become non-responsive. Avoid
>> * setting the reset bit(s).
>> */
>> -static int mv3310_soft_reset(struct phy_device *phydev)
>> -{
>> - return 0;
>> -}
>> -
>
> You do realise that getting rid of that function makes a nonsense of the
> comment above it - and removing the comment along with the function gets
> rid of the very important reason _why_ we have en empty reset method?
>
>> static int mv3310_config_init(struct phy_device *phydev)
>> {
>> __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
>> @@ -377,7 +372,7 @@ static struct phy_driver mv3310_drivers[] = {
>> SUPPORTED_10000baseT_Full |
>> SUPPORTED_Backplane,
>> .probe = mv3310_probe,
>> - .soft_reset = mv3310_soft_reset,
>> + .soft_reset = gen10g_soft_reset,
>> .config_init = mv3310_config_init,
>> .config_aneg = mv3310_config_aneg,
>> .aneg_done = mv3310_aneg_done,
>> --
>> 2.14.1
>>
>
> --
> Russell King
> ARM architecture Linux Kernel maintainer
FWIW I have a local patch that goes something like that, which I meant to send
at one point and forgot
Something like that:
static int gen10g_soft_reset(struct phy_device *phydev)
{
+ int val;
+
+ val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+ if (val < 0)
+ return val;
+
+ val |= MDIO_CTRL1_RESET;
+ phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, val);
+
- /* Do nothing for now */
return 0;
}
If that looks reasonable I can properly submit a patch,
Moritz
On 02/28/2018 11:49 AM, Moritz Fischer wrote:
> Florian,
>
> On Wed, Feb 28, 2018 at 11:44 AM, Russell King <[email protected]> wrote:
>> On Wed, Feb 28, 2018 at 11:36:12AM -0800, Florian Fainelli wrote:
>>> We do the same thing as the generic function: nothing, so utilize it.
>>>
>>> Signed-off-by: Florian Fainelli <[email protected]>
>>> ---
>>> drivers/net/phy/marvell10g.c | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
>>> index 8a0bd98fdec7..da014eae1476 100644
>>> --- a/drivers/net/phy/marvell10g.c
>>> +++ b/drivers/net/phy/marvell10g.c
>>> @@ -75,11 +75,6 @@ static int mv3310_probe(struct phy_device *phydev)
>>> * Resetting the MV88X3310 causes it to become non-responsive. Avoid
>>> * setting the reset bit(s).
>>> */
>>> -static int mv3310_soft_reset(struct phy_device *phydev)
>>> -{
>>> - return 0;
>>> -}
>>> -
>>
>> You do realise that getting rid of that function makes a nonsense of the
>> comment above it - and removing the comment along with the function gets
>> rid of the very important reason _why_ we have en empty reset method?
>>
>>> static int mv3310_config_init(struct phy_device *phydev)
>>> {
>>> __ETHTOOL_DECLARE_LINK_MODE_MASK(supported) = { 0, };
>>> @@ -377,7 +372,7 @@ static struct phy_driver mv3310_drivers[] = {
>>> SUPPORTED_10000baseT_Full |
>>> SUPPORTED_Backplane,
>>> .probe = mv3310_probe,
>>> - .soft_reset = mv3310_soft_reset,
>>> + .soft_reset = gen10g_soft_reset,
>>> .config_init = mv3310_config_init,
>>> .config_aneg = mv3310_config_aneg,
>>> .aneg_done = mv3310_aneg_done,
>>> --
>>> 2.14.1
>>>
>>
>> --
>> Russell King
>> ARM architecture Linux Kernel maintainer
>
> FWIW I have a local patch that goes something like that, which I meant to send
> at one point and forgot
>
> Something like that:
>
> static int gen10g_soft_reset(struct phy_device *phydev)
> {
> + int val;
> +
> + val = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
> + if (val < 0)
> + return val;
> +
> + val |= MDIO_CTRL1_RESET;
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, val);
> +
> - /* Do nothing for now */
> return 0;
> }
>
> If that looks reasonable I can properly submit a patch,
Sure, I would have to check the specification document to make sure
whether this does not need to be coded similarly to what
genphy_soft_reset() does, which is polling for the reset bit to clear
itself.
v2 also renamed the current gen10g_soft_reset() into
gen10g_no_soft_reset() so your implementation makes even more sense now.
--
Florian