genphy_read_status() cannot report correct link speed when BCM54616S PHY
is configured in RGMII->1000Base-KX mode (for example, on Facebook CMM
BMC platform), and it is because speed-related fields in MII registers
are assigned different meanings in 1000X register set. Actually there
is no speed field in 1000X register set because link speed is always
1000 Mb/s.
The patch adds "probe" callback to detect PHY's operation mode based on
INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
Control register. Besides, link speed is manually set to 1000 Mb/s in
"read_status" callback if PHY-switch link is 1000Base-X.
Signed-off-by: Tao Ren <[email protected]>
---
Changes in v3:
- rename bcm5482_read_status to bcm54xx_read_status so the callback can
be shared by BCM5482 and BCM54616S.
Changes in v2:
- Auto-detect PHY operation mode instead of passing DT node.
- move PHY mode auto-detect logic from config_init to probe callback.
- only set speed (not including duplex) in read_status callback.
- update patch description with more background to avoid confusion.
- patch #1 in the series ("net: phy: broadcom: set features explicitly
for BCM54616") is dropped: the fix should go to get_features callback
which may potentially depend on this patch.
drivers/net/phy/broadcom.c | 41 +++++++++++++++++++++++++++++++++-----
include/linux/brcmphy.h | 10 ++++++++--
2 files changed, 44 insertions(+), 7 deletions(-)
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 937d0059e8ac..ecad8a201a09 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
/*
* Select 1000BASE-X register set (primary SerDes)
*/
- reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
- bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
- reg | BCM5482_SHD_MODE_1000BX);
+ reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+ bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
+ reg | BCM54XX_SHD_MODE_1000BX);
/*
* LED1=ACTIVITYLED, LED3=LINKSPD[2]
@@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
return err;
}
-static int bcm5482_read_status(struct phy_device *phydev)
+static int bcm54xx_read_status(struct phy_device *phydev)
{
int err;
@@ -464,6 +464,35 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
return ret;
}
+static int bcm54616s_probe(struct phy_device *phydev)
+{
+ int val, intf_sel;
+
+ val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
+ if (val < 0)
+ return val;
+
+ /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
+ * is 01b.
+ */
+ intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
+ if (intf_sel == 1) {
+ val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
+ if (val < 0)
+ return val;
+
+ /* Bit 0 of the SerDes 100-FX Control register, when set
+ * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
+ * When this bit is set to 0, it sets the GMII/RGMII ->
+ * 1000BASE-X configuration.
+ */
+ if (!(val & BCM54616S_100FX_MODE))
+ phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+ }
+
+ return 0;
+}
+
static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
{
int val;
@@ -655,6 +684,8 @@ static struct phy_driver broadcom_drivers[] = {
.config_aneg = bcm54616s_config_aneg,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
+ .read_status = bcm54xx_read_status,
+ .probe = bcm54616s_probe,
}, {
.phy_id = PHY_ID_BCM5464,
.phy_id_mask = 0xfffffff0,
@@ -689,7 +720,7 @@ static struct phy_driver broadcom_drivers[] = {
.name = "Broadcom BCM5482",
/* PHY_GBIT_FEATURES */
.config_init = bcm5482_config_init,
- .read_status = bcm5482_read_status,
+ .read_status = bcm54xx_read_status,
.ack_interrupt = bcm_phy_ack_intr,
.config_intr = bcm_phy_config_intr,
}, {
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 6db2d9a6e503..b475e7f20d28 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -200,9 +200,15 @@
#define BCM5482_SHD_SSD 0x14 /* 10100: Secondary SerDes control */
#define BCM5482_SHD_SSD_LEDM 0x0008 /* SSD LED Mode enable */
#define BCM5482_SHD_SSD_EN 0x0001 /* SSD enable */
-#define BCM5482_SHD_MODE 0x1f /* 11111: Mode Control Register */
-#define BCM5482_SHD_MODE_1000BX 0x0001 /* Enable 1000BASE-X registers */
+/* 10011: SerDes 100-FX Control Register */
+#define BCM54616S_SHD_100FX_CTRL 0x13
+#define BCM54616S_100FX_MODE BIT(0) /* 100-FX SerDes Enable */
+
+/* 11111: Mode Control Register */
+#define BCM54XX_SHD_MODE 0x1f
+#define BCM54XX_SHD_INTF_SEL_MASK GENMASK(2, 1) /* INTERF_SEL[1:0] */
+#define BCM54XX_SHD_MODE_1000BX BIT(0) /* Enable 1000-X registers */
/*
* EXPANSION SHADOW ACCESS REGISTERS. (PHY REG 0x15, 0x16, and 0x17)
--
2.17.1
Hi Tao,
On Sat, 3 Aug 2019 at 00:56, Tao Ren <[email protected]> wrote:
>
> genphy_read_status() cannot report correct link speed when BCM54616S PHY
> is configured in RGMII->1000Base-KX mode (for example, on Facebook CMM
> BMC platform), and it is because speed-related fields in MII registers
> are assigned different meanings in 1000X register set. Actually there
> is no speed field in 1000X register set because link speed is always
> 1000 Mb/s.
>
> The patch adds "probe" callback to detect PHY's operation mode based on
> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
> Control register. Besides, link speed is manually set to 1000 Mb/s in
> "read_status" callback if PHY-switch link is 1000Base-X.
>
> Signed-off-by: Tao Ren <[email protected]>
> ---
> Changes in v3:
> - rename bcm5482_read_status to bcm54xx_read_status so the callback can
> be shared by BCM5482 and BCM54616S.
> Changes in v2:
> - Auto-detect PHY operation mode instead of passing DT node.
> - move PHY mode auto-detect logic from config_init to probe callback.
> - only set speed (not including duplex) in read_status callback.
> - update patch description with more background to avoid confusion.
> - patch #1 in the series ("net: phy: broadcom: set features explicitly
> for BCM54616") is dropped: the fix should go to get_features callback
> which may potentially depend on this patch.
>
> drivers/net/phy/broadcom.c | 41 +++++++++++++++++++++++++++++++++-----
> include/linux/brcmphy.h | 10 ++++++++--
> 2 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 937d0059e8ac..ecad8a201a09 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
> /*
> * Select 1000BASE-X register set (primary SerDes)
> */
> - reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
> - bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
> - reg | BCM5482_SHD_MODE_1000BX);
> + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
> + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
> + reg | BCM54XX_SHD_MODE_1000BX);
>
> /*
> * LED1=ACTIVITYLED, LED3=LINKSPD[2]
> @@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
> return err;
> }
>
> -static int bcm5482_read_status(struct phy_device *phydev)
> +static int bcm54xx_read_status(struct phy_device *phydev)
> {
> int err;
>
> @@ -464,6 +464,35 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
> return ret;
> }
>
> +static int bcm54616s_probe(struct phy_device *phydev)
> +{
> + int val, intf_sel;
> +
> + val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
> + if (val < 0)
> + return val;
> +
> + /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
> + * is 01b.
> + */
> + intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
> + if (intf_sel == 1) {
> + val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
> + if (val < 0)
> + return val;
> +
> + /* Bit 0 of the SerDes 100-FX Control register, when set
> + * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
> + * When this bit is set to 0, it sets the GMII/RGMII ->
> + * 1000BASE-X configuration.
> + */
> + if (!(val & BCM54616S_100FX_MODE))
> + phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
> + }
> +
> + return 0;
> +}
> +
> static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
> {
> int val;
> @@ -655,6 +684,8 @@ static struct phy_driver broadcom_drivers[] = {
> .config_aneg = bcm54616s_config_aneg,
> .ack_interrupt = bcm_phy_ack_intr,
> .config_intr = bcm_phy_config_intr,
> + .read_status = bcm54xx_read_status,
> + .probe = bcm54616s_probe,
> }, {
> .phy_id = PHY_ID_BCM5464,
> .phy_id_mask = 0xfffffff0,
> @@ -689,7 +720,7 @@ static struct phy_driver broadcom_drivers[] = {
> .name = "Broadcom BCM5482",
> /* PHY_GBIT_FEATURES */
> .config_init = bcm5482_config_init,
> - .read_status = bcm5482_read_status,
> + .read_status = bcm54xx_read_status,
> .ack_interrupt = bcm_phy_ack_intr,
> .config_intr = bcm_phy_config_intr,
> }, {
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 6db2d9a6e503..b475e7f20d28 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -200,9 +200,15 @@
> #define BCM5482_SHD_SSD 0x14 /* 10100: Secondary SerDes control */
> #define BCM5482_SHD_SSD_LEDM 0x0008 /* SSD LED Mode enable */
> #define BCM5482_SHD_SSD_EN 0x0001 /* SSD enable */
> -#define BCM5482_SHD_MODE 0x1f /* 11111: Mode Control Register */
> -#define BCM5482_SHD_MODE_1000BX 0x0001 /* Enable 1000BASE-X registers */
>
> +/* 10011: SerDes 100-FX Control Register */
> +#define BCM54616S_SHD_100FX_CTRL 0x13
> +#define BCM54616S_100FX_MODE BIT(0) /* 100-FX SerDes Enable */
> +
> +/* 11111: Mode Control Register */
> +#define BCM54XX_SHD_MODE 0x1f
> +#define BCM54XX_SHD_INTF_SEL_MASK GENMASK(2, 1) /* INTERF_SEL[1:0] */
> +#define BCM54XX_SHD_MODE_1000BX BIT(0) /* Enable 1000-X registers */
>
> /*
> * EXPANSION SHADOW ACCESS REGISTERS. (PHY REG 0x15, 0x16, and 0x17)
> --
> 2.17.1
>
The patchset looks better now. But is it ok, I wonder, to keep
PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
phy_attach_direct is overwriting it?
Regards,
-Vladimir
Hi Vladimir,
On 8/3/19 6:49 AM, Vladimir Oltean wrote:
> Hi Tao,
>
> On Sat, 3 Aug 2019 at 00:56, Tao Ren <[email protected]> wrote:
>>
>> genphy_read_status() cannot report correct link speed when BCM54616S PHY
>> is configured in RGMII->1000Base-KX mode (for example, on Facebook CMM
>> BMC platform), and it is because speed-related fields in MII registers
>> are assigned different meanings in 1000X register set. Actually there
>> is no speed field in 1000X register set because link speed is always
>> 1000 Mb/s.
>>
>> The patch adds "probe" callback to detect PHY's operation mode based on
>> INTERF_SEL[1:0] pins and 1000X/100FX selection bit in SerDES 100-FX
>> Control register. Besides, link speed is manually set to 1000 Mb/s in
>> "read_status" callback if PHY-switch link is 1000Base-X.
>>
>> Signed-off-by: Tao Ren <[email protected]>
>> ---
>> Changes in v3:
>> - rename bcm5482_read_status to bcm54xx_read_status so the callback can
>> be shared by BCM5482 and BCM54616S.
>> Changes in v2:
>> - Auto-detect PHY operation mode instead of passing DT node.
>> - move PHY mode auto-detect logic from config_init to probe callback.
>> - only set speed (not including duplex) in read_status callback.
>> - update patch description with more background to avoid confusion.
>> - patch #1 in the series ("net: phy: broadcom: set features explicitly
>> for BCM54616") is dropped: the fix should go to get_features callback
>> which may potentially depend on this patch.
>>
>> drivers/net/phy/broadcom.c | 41 +++++++++++++++++++++++++++++++++-----
>> include/linux/brcmphy.h | 10 ++++++++--
>> 2 files changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 937d0059e8ac..ecad8a201a09 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -383,9 +383,9 @@ static int bcm5482_config_init(struct phy_device *phydev)
>> /*
>> * Select 1000BASE-X register set (primary SerDes)
>> */
>> - reg = bcm_phy_read_shadow(phydev, BCM5482_SHD_MODE);
>> - bcm_phy_write_shadow(phydev, BCM5482_SHD_MODE,
>> - reg | BCM5482_SHD_MODE_1000BX);
>> + reg = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> + bcm_phy_write_shadow(phydev, BCM54XX_SHD_MODE,
>> + reg | BCM54XX_SHD_MODE_1000BX);
>>
>> /*
>> * LED1=ACTIVITYLED, LED3=LINKSPD[2]
>> @@ -409,7 +409,7 @@ static int bcm5482_config_init(struct phy_device *phydev)
>> return err;
>> }
>>
>> -static int bcm5482_read_status(struct phy_device *phydev)
>> +static int bcm54xx_read_status(struct phy_device *phydev)
>> {
>> int err;
>>
>> @@ -464,6 +464,35 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
>> return ret;
>> }
>>
>> +static int bcm54616s_probe(struct phy_device *phydev)
>> +{
>> + int val, intf_sel;
>> +
>> + val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
>> + if (val < 0)
>> + return val;
>> +
>> + /* The PHY is strapped in RGMII to fiber mode when INTERF_SEL[1:0]
>> + * is 01b.
>> + */
>> + intf_sel = (val & BCM54XX_SHD_INTF_SEL_MASK) >> 1;
>> + if (intf_sel == 1) {
>> + val = bcm_phy_read_shadow(phydev, BCM54616S_SHD_100FX_CTRL);
>> + if (val < 0)
>> + return val;
>> +
>> + /* Bit 0 of the SerDes 100-FX Control register, when set
>> + * to 1, sets the MII/RGMII -> 100BASE-FX configuration.
>> + * When this bit is set to 0, it sets the GMII/RGMII ->
>> + * 1000BASE-X configuration.
>> + */
>> + if (!(val & BCM54616S_100FX_MODE))
>> + phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
>> {
>> int val;
>> @@ -655,6 +684,8 @@ static struct phy_driver broadcom_drivers[] = {
>> .config_aneg = bcm54616s_config_aneg,
>> .ack_interrupt = bcm_phy_ack_intr,
>> .config_intr = bcm_phy_config_intr,
>> + .read_status = bcm54xx_read_status,
>> + .probe = bcm54616s_probe,
>> }, {
>> .phy_id = PHY_ID_BCM5464,
>> .phy_id_mask = 0xfffffff0,
>> @@ -689,7 +720,7 @@ static struct phy_driver broadcom_drivers[] = {
>> .name = "Broadcom BCM5482",
>> /* PHY_GBIT_FEATURES */
>> .config_init = bcm5482_config_init,
>> - .read_status = bcm5482_read_status,
>> + .read_status = bcm54xx_read_status,
>> .ack_interrupt = bcm_phy_ack_intr,
>> .config_intr = bcm_phy_config_intr,
>> }, {
>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
>> index 6db2d9a6e503..b475e7f20d28 100644
>> --- a/include/linux/brcmphy.h
>> +++ b/include/linux/brcmphy.h
>> @@ -200,9 +200,15 @@
>> #define BCM5482_SHD_SSD 0x14 /* 10100: Secondary SerDes control */
>> #define BCM5482_SHD_SSD_LEDM 0x0008 /* SSD LED Mode enable */
>> #define BCM5482_SHD_SSD_EN 0x0001 /* SSD enable */
>> -#define BCM5482_SHD_MODE 0x1f /* 11111: Mode Control Register */
>> -#define BCM5482_SHD_MODE_1000BX 0x0001 /* Enable 1000BASE-X registers */
>>
>> +/* 10011: SerDes 100-FX Control Register */
>> +#define BCM54616S_SHD_100FX_CTRL 0x13
>> +#define BCM54616S_100FX_MODE BIT(0) /* 100-FX SerDes Enable */
>> +
>> +/* 11111: Mode Control Register */
>> +#define BCM54XX_SHD_MODE 0x1f
>> +#define BCM54XX_SHD_INTF_SEL_MASK GENMASK(2, 1) /* INTERF_SEL[1:0] */
>> +#define BCM54XX_SHD_MODE_1000BX BIT(0) /* Enable 1000-X registers */
>>
>> /*
>> * EXPANSION SHADOW ACCESS REGISTERS. (PHY REG 0x15, 0x16, and 0x17)
>> --
>> 2.17.1
>>
>
> The patchset looks better now. But is it ok, I wonder, to keep
> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
> phy_attach_direct is overwriting it?
I checked ftgmac100 driver (used on my machine) and it calls phy_connect_direct which passes phydev->dev_flags when calling phy_attach_direct: that explains why the flag is not cleared in my case.
Can you give me some suggestions since dev_flags may be override in other calling paths? For example, is it good idea to move the auto-detect logic from probe to config_init? Or are there other fields in phy_device structure that can be used to store PHY-switch link type? Or maybe I should just include the auto-detect logic in read_status callback?
Thanks,
Tao
> > The patchset looks better now. But is it ok, I wonder, to keep
> > PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
> > phy_attach_direct is overwriting it?
>
> I checked ftgmac100 driver (used on my machine) and it calls
> phy_connect_direct which passes phydev->dev_flags when calling
> phy_attach_direct: that explains why the flag is not cleared in my
> case.
Yes, that is the way it is intended to be used. The MAC driver can
pass flags to the PHY. It is a fragile API, since the MAC needs to
know what PHY is being used, since the flags are driver specific.
One option would be to modify the assignment in phy_attach_direct() to
OR in the flags passed to it with flags which are already in
phydev->dev_flags.
Andrew
On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <[email protected]> wrote:
>
> > > The patchset looks better now. But is it ok, I wonder, to keep
> > > PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
> > > phy_attach_direct is overwriting it?
> >
>
> > I checked ftgmac100 driver (used on my machine) and it calls
> > phy_connect_direct which passes phydev->dev_flags when calling
> > phy_attach_direct: that explains why the flag is not cleared in my
> > case.
>
> Yes, that is the way it is intended to be used. The MAC driver can
> pass flags to the PHY. It is a fragile API, since the MAC needs to
> know what PHY is being used, since the flags are driver specific.
>
> One option would be to modify the assignment in phy_attach_direct() to
> OR in the flags passed to it with flags which are already in
> phydev->dev_flags.
>
> Andrew
Even if that were the case (patching phy_attach_direct to apply a
logical-or to dev_flags), it sounds fishy to me that the genphy code
is unable to determine that this PHY is running in 1000Base-X mode.
In my opinion it all boils down to this warning:
"PHY advertising (0,00000200,000062c0) more modes than genphy
supports, some modes not advertised".
You see, the 0x200 in the above advertising mask corresponds exactly
to this definition from ethtool.h:
ETHTOOL_LINK_MODE_1000baseX_Full_BIT = 41,
But it gets truncated and hence lost.
Regards,
-Vladimir
On 04.08.2019 17:59, Vladimir Oltean wrote:
> On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <[email protected]> wrote:
>>
>>>> The patchset looks better now. But is it ok, I wonder, to keep
>>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
>>>> phy_attach_direct is overwriting it?
>>>
>>
>>> I checked ftgmac100 driver (used on my machine) and it calls
>>> phy_connect_direct which passes phydev->dev_flags when calling
>>> phy_attach_direct: that explains why the flag is not cleared in my
>>> case.
>>
>> Yes, that is the way it is intended to be used. The MAC driver can
>> pass flags to the PHY. It is a fragile API, since the MAC needs to
>> know what PHY is being used, since the flags are driver specific.
>>
>> One option would be to modify the assignment in phy_attach_direct() to
>> OR in the flags passed to it with flags which are already in
>> phydev->dev_flags.
>>
>> Andrew
>
> Even if that were the case (patching phy_attach_direct to apply a
> logical-or to dev_flags), it sounds fishy to me that the genphy code
> is unable to determine that this PHY is running in 1000Base-X mode.
>
> In my opinion it all boils down to this warning:
>
> "PHY advertising (0,00000200,000062c0) more modes than genphy
> supports, some modes not advertised".
>
The genphy code deals with Clause 22 + Gigabit BaseT only.
Question is whether you want aneg at all in 1000Base-X mode and
what you want the config_aneg callback to do.
There may be some inspiration in the Marvel PHY drivers.
> You see, the 0x200 in the above advertising mask corresponds exactly
> to this definition from ethtool.h:
> ETHTOOL_LINK_MODE_1000baseX_Full_BIT = 41,
>
> But it gets truncated and hence lost.
>
> Regards,
> -Vladimir
>
Heiner
> > Even if that were the case (patching phy_attach_direct to apply a
> > logical-or to dev_flags), it sounds fishy to me that the genphy code
> > is unable to determine that this PHY is running in 1000Base-X mode.
> >
> > In my opinion it all boils down to this warning:
> >
> > "PHY advertising (0,00000200,000062c0) more modes than genphy
> > supports, some modes not advertised".
> >
> The genphy code deals with Clause 22 + Gigabit BaseT only.
> Question is whether you want aneg at all in 1000Base-X mode and
> what you want the config_aneg callback to do.
> There may be some inspiration in the Marvel PHY drivers.
As far as i know, you cannot actually advertise 1000Base-X. So we
probably should not be setting the bit in advertise, only having it in
supported?
Andrew
On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <[email protected]> wrote:
>
> On 04.08.2019 17:59, Vladimir Oltean wrote:
> > On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <[email protected]> wrote:
> >>
> >>>> The patchset looks better now. But is it ok, I wonder, to keep
> >>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
> >>>> phy_attach_direct is overwriting it?
> >>>
> >>
> >>> I checked ftgmac100 driver (used on my machine) and it calls
> >>> phy_connect_direct which passes phydev->dev_flags when calling
> >>> phy_attach_direct: that explains why the flag is not cleared in my
> >>> case.
> >>
> >> Yes, that is the way it is intended to be used. The MAC driver can
> >> pass flags to the PHY. It is a fragile API, since the MAC needs to
> >> know what PHY is being used, since the flags are driver specific.
> >>
> >> One option would be to modify the assignment in phy_attach_direct() to
> >> OR in the flags passed to it with flags which are already in
> >> phydev->dev_flags.
> >>
> >> Andrew
> >
> > Even if that were the case (patching phy_attach_direct to apply a
> > logical-or to dev_flags), it sounds fishy to me that the genphy code
> > is unable to determine that this PHY is running in 1000Base-X mode.
> >
> > In my opinion it all boils down to this warning:
> >
> > "PHY advertising (0,00000200,000062c0) more modes than genphy
> > supports, some modes not advertised".
> >
> The genphy code deals with Clause 22 + Gigabit BaseT only.
> Question is whether you want aneg at all in 1000Base-X mode and
> what you want the config_aneg callback to do.
> There may be some inspiration in the Marvel PHY drivers.
>
AN for 1000Base-X still gives you duplex and pause frame settings. I
thought the base page format for exchanging that info is standardized
in clause 37.
Does genphy cover only copper media by design, or is it desirable to
augment genphy_read_status?
> > You see, the 0x200 in the above advertising mask corresponds exactly
> > to this definition from ethtool.h:
> > ETHTOOL_LINK_MODE_1000baseX_Full_BIT = 41,
> >
> > But it gets truncated and hence lost.
> >
> > Regards,
> > -Vladimir
> >
> Heiner
Hi Andrew,
On 8/4/19 7:51 AM, Andrew Lunn wrote:
>>> The patchset looks better now. But is it ok, I wonder, to keep
>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
>>> phy_attach_direct is overwriting it?
>>
>
>> I checked ftgmac100 driver (used on my machine) and it calls
>> phy_connect_direct which passes phydev->dev_flags when calling
>> phy_attach_direct: that explains why the flag is not cleared in my
>> case.
>
> Yes, that is the way it is intended to be used. The MAC driver can
> pass flags to the PHY. It is a fragile API, since the MAC needs to
> know what PHY is being used, since the flags are driver specific.
>
> One option would be to modify the assignment in phy_attach_direct() to
> OR in the flags passed to it with flags which are already in
> phydev->dev_flags.
It sounds like a reasonable fix/enhancement to replace overriding with OR, no matter which direction we are going to (either adding 1000bx aneg in genphy or providing phy-specific aneg callback).
Do you want me to send out the patch (I feel it's better to be in a separate patch?) or someone else will take care of it?
Thanks,
Tao
On Mon, Aug 05, 2019 at 06:38:16AM +0000, Tao Ren wrote:
> Hi Andrew,
>
> On 8/4/19 7:51 AM, Andrew Lunn wrote:
> >>> The patchset looks better now. But is it ok, I wonder, to keep
> >>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
> >>> phy_attach_direct is overwriting it?
> >>
> >
> >> I checked ftgmac100 driver (used on my machine) and it calls
> >> phy_connect_direct which passes phydev->dev_flags when calling
> >> phy_attach_direct: that explains why the flag is not cleared in my
> >> case.
> >
> > Yes, that is the way it is intended to be used. The MAC driver can
> > pass flags to the PHY. It is a fragile API, since the MAC needs to
> > know what PHY is being used, since the flags are driver specific.
> >
> > One option would be to modify the assignment in phy_attach_direct() to
> > OR in the flags passed to it with flags which are already in
> > phydev->dev_flags.
>
> It sounds like a reasonable fix/enhancement to replace overriding
> with OR, no matter which direction we are going to (either adding
> 1000bx aneg in genphy or providing phy-specific aneg callback).
> Do you want me to send out the patch (I feel it's better to be in a
> separate patch?) or someone else will take care of it?
Hi Tao
Please send a patch.
Thanks
Andrew
On 04.08.2019 21:22, Vladimir Oltean wrote:
> On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <[email protected]> wrote:
>>
>> On 04.08.2019 17:59, Vladimir Oltean wrote:
>>> On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <[email protected]> wrote:
>>>>
>>>>>> The patchset looks better now. But is it ok, I wonder, to keep
>>>>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
>>>>>> phy_attach_direct is overwriting it?
>>>>>
>>>>
>>>>> I checked ftgmac100 driver (used on my machine) and it calls
>>>>> phy_connect_direct which passes phydev->dev_flags when calling
>>>>> phy_attach_direct: that explains why the flag is not cleared in my
>>>>> case.
>>>>
>>>> Yes, that is the way it is intended to be used. The MAC driver can
>>>> pass flags to the PHY. It is a fragile API, since the MAC needs to
>>>> know what PHY is being used, since the flags are driver specific.
>>>>
>>>> One option would be to modify the assignment in phy_attach_direct() to
>>>> OR in the flags passed to it with flags which are already in
>>>> phydev->dev_flags.
>>>>
>>>> Andrew
>>>
>>> Even if that were the case (patching phy_attach_direct to apply a
>>> logical-or to dev_flags), it sounds fishy to me that the genphy code
>>> is unable to determine that this PHY is running in 1000Base-X mode.
>>>
>>> In my opinion it all boils down to this warning:
>>>
>>> "PHY advertising (0,00000200,000062c0) more modes than genphy
>>> supports, some modes not advertised".
>>>
>> The genphy code deals with Clause 22 + Gigabit BaseT only.
>> Question is whether you want aneg at all in 1000Base-X mode and
>> what you want the config_aneg callback to do.
>> There may be some inspiration in the Marvel PHY drivers.
>>
>
> AN for 1000Base-X still gives you duplex and pause frame settings. I
> thought the base page format for exchanging that info is standardized
> in clause 37.
> Does genphy cover only copper media by design, or is it desirable to
> augment genphy_read_status?
>
So far we care about copper only in phylib. Some constants needed for
Clause 37 support are defined, but used by few drivers only.
ADVERTISE_1000XHALF
ADVERTISE_1000XFULL
ADVERTISE_1000XPAUSE
ADVERTISE_1000XPSE_ASYM
I think it would make sense to have something like genphy_c37_config_aneg.
Similar for read_status.
>>> You see, the 0x200 in the above advertising mask corresponds exactly
>>> to this definition from ethtool.h:
>>> ETHTOOL_LINK_MODE_1000baseX_Full_BIT = 41,
>>>
>>> But it gets truncated and hence lost.
>>>
>>> Regards,
>>> -Vladimir
>>>
>> Heiner
>
Heiner
On 8/5/19 1:45 PM, Heiner Kallweit wrote:
> On 04.08.2019 21:22, Vladimir Oltean wrote:
>> On Sun, 4 Aug 2019 at 19:07, Heiner Kallweit <[email protected]> wrote:
>>>
>>> On 04.08.2019 17:59, Vladimir Oltean wrote:
>>>> On Sun, 4 Aug 2019 at 17:52, Andrew Lunn <[email protected]> wrote:
>>>>>
>>>>>>> The patchset looks better now. But is it ok, I wonder, to keep
>>>>>>> PHY_BCM_FLAGS_MODE_1000BX in phydev->dev_flags, considering that
>>>>>>> phy_attach_direct is overwriting it?
>>>>>>
>>>>>
>>>>>> I checked ftgmac100 driver (used on my machine) and it calls
>>>>>> phy_connect_direct which passes phydev->dev_flags when calling
>>>>>> phy_attach_direct: that explains why the flag is not cleared in my
>>>>>> case.
>>>>>
>>>>> Yes, that is the way it is intended to be used. The MAC driver can
>>>>> pass flags to the PHY. It is a fragile API, since the MAC needs to
>>>>> know what PHY is being used, since the flags are driver specific.
>>>>>
>>>>> One option would be to modify the assignment in phy_attach_direct() to
>>>>> OR in the flags passed to it with flags which are already in
>>>>> phydev->dev_flags.
>>>>>
>>>>> Andrew
>>>>
>>>> Even if that were the case (patching phy_attach_direct to apply a
>>>> logical-or to dev_flags), it sounds fishy to me that the genphy code
>>>> is unable to determine that this PHY is running in 1000Base-X mode.
>>>>
>>>> In my opinion it all boils down to this warning:
>>>>
>>>> "PHY advertising (0,00000200,000062c0) more modes than genphy
>>>> supports, some modes not advertised".
>>>>
>>> The genphy code deals with Clause 22 + Gigabit BaseT only.
>>> Question is whether you want aneg at all in 1000Base-X mode and
>>> what you want the config_aneg callback to do.
>>> There may be some inspiration in the Marvel PHY drivers.
>>>
>>
>> AN for 1000Base-X still gives you duplex and pause frame settings. I
>> thought the base page format for exchanging that info is standardized
>> in clause 37.
>> Does genphy cover only copper media by design, or is it desirable to
>> augment genphy_read_status?
>>
> So far we care about copper only in phylib. Some constants needed for
> Clause 37 support are defined, but used by few drivers only.
>
> ADVERTISE_1000XHALF
> ADVERTISE_1000XFULL
> ADVERTISE_1000XPAUSE
> ADVERTISE_1000XPSE_ASYM
>
> I think it would make sense to have something like genphy_c37_config_aneg.
> Similar for read_status.
Thank you all for the inputs on this patch.
If I understand correctly, we are going to create a set of genphy_c37_* functions for 1000x support so it can be used by phy drivers? Or are we considering other options? What's your recommendation on this specific patch?
Thanks,
Tao