* [PATCH net-next 0/1] Add BASE-T1 PHY support
@ 2019-08-15 15:32 Christian Herber
2019-08-15 15:32 ` [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem Christian Herber
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Christian Herber @ 2019-08-15 15:32 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, Christian Herber
This patch adds basic support for BASE-T1 PHYs in the framework.
BASE-T1 PHYs main area of application are automotive and industrial.
BASE-T1 is standardized in IEEE 802.3, namely
- IEEE 802.3bw: 100BASE-T1
- IEEE 802.3bp 1000BASE-T1
- IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
There are no products which contain BASE-T1 and consumer type PHYs like
1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
PHYs with auto-negotiation.
The intention of this patch is to make use of the existing Clause 45 functions.
BASE-T1 adds some additional registers e.g. for aneg control, which follow a
similiar register layout as the existing devices. The bits which are used in
BASE-T1 specific registers are the same as in basic registers, thus the
existing functions can be resued, with get_aneg_ctrl() selecting the correct
register address.
The current version of ethtool has been prepared for 100/1000BASE-T1 and works
with this patch. 10BASE-T1 needs to be added to ethtool.
Christian Herber (1):
Added BASE-T1 PHY support to PHY Subsystem
drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
drivers/net/phy/phy-core.c | 4 +-
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/mdio.h | 21 +++++++
4 files changed, 129 insertions(+), 11 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
2019-08-15 15:32 [PATCH net-next 0/1] Add BASE-T1 PHY support Christian Herber
@ 2019-08-15 15:32 ` Christian Herber
2019-08-15 15:56 ` Andrew Lunn
2019-08-16 21:13 ` Heiner Kallweit
2019-08-15 15:43 ` [PATCH net-next 0/1] Add BASE-T1 PHY support Andrew Lunn
2019-08-16 20:59 ` Heiner Kallweit
2 siblings, 2 replies; 20+ messages in thread
From: Christian Herber @ 2019-08-15 15:32 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, Christian Herber
BASE-T1 is a category of Ethernet PHYs.
They use a single copper pair for transmission.
This patch add basic support for this category of PHYs.
It coveres the discovery of abilities and basic configuration.
It includes setting fixed speed and enabling auto-negotiation.
BASE-T1 devices should always Clause-45 managed.
Therefore, this patch extends phy-c45.c.
While for some functions like auto-neogtiation different registers are
used, the layout of these registers is the same for the used fields.
Thus, much of the logic of basic Clause-45 devices can be reused.
Signed-off-by: Christian Herber <christian.herber@nxp.com>
---
drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
drivers/net/phy/phy-core.c | 4 +-
include/uapi/linux/ethtool.h | 2 +
include/uapi/linux/mdio.h | 21 +++++++
4 files changed, 129 insertions(+), 11 deletions(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index b9d4145781ca..9ff0b8c785de 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -8,13 +8,23 @@
#include <linux/mii.h>
#include <linux/phy.h>
+#define IS_100BASET1(phy) (linkmode_test_bit( \
+ ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
+ (phy)->supported))
+#define IS_1000BASET1(phy) (linkmode_test_bit( \
+ ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
+ (phy)->supported))
+
+static u32 get_aneg_ctrl(struct phy_device *phydev);
+static u32 get_aneg_stat(struct phy_device *phydev);
+
/**
* genphy_c45_setup_forced - configures a forced speed
* @phydev: target phy_device struct
*/
int genphy_c45_pma_setup_forced(struct phy_device *phydev)
{
- int ctrl1, ctrl2, ret;
+ int ctrl1, ctrl2, base_t1_ctrl = 0, ret;
/* Half duplex is not supported */
if (phydev->duplex != DUPLEX_FULL)
@@ -28,6 +38,16 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
if (ctrl2 < 0)
return ctrl2;
+ if (IS_100BASET1(phydev) || IS_1000BASET1(phydev)) {
+ base_t1_ctrl = phy_read_mmd(phydev,
+ MDIO_MMD_PMAPMD,
+ MDIO_PMA_BASET1CTRL);
+ if (base_t1_ctrl < 0)
+ return base_t1_ctrl;
+
+ base_t1_ctrl &= ~(MDIO_PMA_BASET1CTRL_TYPE);
+ }
+
ctrl1 &= ~MDIO_CTRL1_SPEEDSEL;
/*
* PMA/PMD type selection is 1.7.5:0 not 1.7.3:0. See 45.2.1.6.1
@@ -41,12 +61,21 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
break;
case SPEED_100:
ctrl1 |= MDIO_PMA_CTRL1_SPEED100;
- ctrl2 |= MDIO_PMA_CTRL2_100BTX;
+ if (IS_100BASET1(phydev)) {
+ ctrl2 |= MDIO_PMA_CTRL2_BT1;
+ base_t1_ctrl |= MDIO_PMA_BASET1CTRL_TYPE_100BT1;
+ } else {
+ ctrl2 |= MDIO_PMA_CTRL2_100BTX;
+ }
break;
case SPEED_1000:
ctrl1 |= MDIO_PMA_CTRL1_SPEED1000;
- /* Assume 1000base-T */
- ctrl2 |= MDIO_PMA_CTRL2_1000BT;
+ if (IS_1000BASET1(phydev)) {
+ ctrl2 |= MDIO_PMA_CTRL2_BT1;
+ base_t1_ctrl |= MDIO_PMA_BASET1CTRL_TYPE_1000BT1;
+ } else {
+ ctrl2 |= MDIO_PMA_CTRL2_1000BT;
+ }
break;
case SPEED_2500:
ctrl1 |= MDIO_CTRL1_SPEED2_5G;
@@ -75,6 +104,14 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
if (ret < 0)
return ret;
+ if (IS_100BASET1(phydev) || IS_1000BASET1(phydev)) {
+ ret = phy_write_mmd(phydev,
+ MDIO_MMD_PMAPMD,
+ MDIO_PMA_BASET1CTRL,
+ base_t1_ctrl);
+ if (ret < 0)
+ return ret;
+ }
return genphy_c45_an_disable_aneg(phydev);
}
EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
@@ -135,8 +172,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_an_config_aneg);
*/
int genphy_c45_an_disable_aneg(struct phy_device *phydev)
{
-
- return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1,
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev),
MDIO_AN_CTRL1_ENABLE | MDIO_AN_CTRL1_RESTART);
}
EXPORT_SYMBOL_GPL(genphy_c45_an_disable_aneg);
@@ -151,7 +187,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_an_disable_aneg);
*/
int genphy_c45_restart_aneg(struct phy_device *phydev)
{
- return phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1,
+ return phy_set_bits_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev),
MDIO_AN_CTRL1_ENABLE | MDIO_AN_CTRL1_RESTART);
}
EXPORT_SYMBOL_GPL(genphy_c45_restart_aneg);
@@ -171,7 +207,7 @@ int genphy_c45_check_and_restart_aneg(struct phy_device *phydev, bool restart)
if (!restart) {
/* Configure and restart aneg if it wasn't set before */
- ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
+ ret = phy_read_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev));
if (ret < 0)
return ret;
@@ -199,7 +235,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_check_and_restart_aneg);
*/
int genphy_c45_aneg_done(struct phy_device *phydev)
{
- int val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
+ int val = phy_read_mmd(phydev, MDIO_MMD_AN, get_aneg_stat(phydev));
return val < 0 ? val : val & MDIO_AN_STAT1_COMPLETE ? 1 : 0;
}
@@ -385,7 +421,9 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
* PMA Extended Abilities (1.11) register, indicating 1000BASET an 10G related
* modes. If bit 1.11.14 is set, then the list is also extended with the modes
* in the 2.5G/5G PMA Extended register (1.21), indicating if 2.5GBASET and
- * 5GBASET are supported.
+ * 5GBASET are supported. If bit 1.11.11 is set, then the list is also extended
+ * with the modes in the BASE-T1 PMA Extended register (1.18), indicating if
+ * 10/100/1000BASET-1 are supported.
*/
int genphy_c45_pma_read_abilities(struct phy_device *phydev)
{
@@ -470,6 +508,29 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
phydev->supported,
val & MDIO_PMA_NG_EXTABLE_5GBT);
}
+
+ if (val & MDIO_PMA_EXTABLE_BASET1) {
+ val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
+ MDIO_PMA_BASET1_EXTABLE);
+ if (val < 0)
+ return val;
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
+ phydev->supported,
+ val & MDIO_PMA_BASET1_EXTABLE_100BT1);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
+ phydev->supported,
+ val & MDIO_PMA_BASET1_EXTABLE_1000BT1);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT,
+ phydev->supported,
+ val & MDIO_PMA_BASET1_EXTABLE_10BT1L);
+
+ linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1S_Full_BIT,
+ phydev->supported,
+ val & MDIO_PMA_BASET1_EXTABLE_10BT1S);
+ }
}
return 0;
@@ -509,6 +570,38 @@ int genphy_c45_read_status(struct phy_device *phydev)
}
EXPORT_SYMBOL_GPL(genphy_c45_read_status);
+/**
+ * get_aneg_ctrl - Get the register address for auto-
+ * negotiation control register
+ * @phydev: target phy_device struct
+ *
+ */
+static u32 get_aneg_ctrl(struct phy_device *phydev)
+{
+ u32 ctrl = MDIO_CTRL1;
+
+ if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
+ ctrl = MDIO_AN_BT1_CTRL;
+
+ return ctrl;
+}
+
+/**
+ * get_aneg_ctrl - Get the register address for auto-
+ * negotiation status register
+ * @phydev: target phy_device struct
+ *
+ */
+static u32 get_aneg_stat(struct phy_device *phydev)
+{
+ u32 stat = MDIO_STAT1;
+
+ if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
+ stat = MDIO_AN_BT1_STAT;
+
+ return stat;
+}
+
/* The gen10g_* functions are the old Clause 45 stub */
int gen10g_config_aneg(struct phy_device *phydev)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 369903d9b6ec..b50576f7709a 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -8,7 +8,7 @@
const char *phy_speed_to_str(int speed)
{
- BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 69,
+ BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 71,
"Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
"If a speed or mode has been added please update phy_speed_to_str "
"and the PHY settings array.\n");
@@ -140,6 +140,8 @@ static const struct phy_setting settings[] = {
/* 10M */
PHY_SETTING( 10, FULL, 10baseT_Full ),
PHY_SETTING( 10, HALF, 10baseT_Half ),
+ PHY_SETTING( 10, FULL, 10baseT1L_Full ),
+ PHY_SETTING( 10, FULL, 10baseT1S_Full ),
};
#undef PHY_SETTING
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index dd06302aa93e..e429cc8da31a 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1485,6 +1485,8 @@ enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66,
ETHTOOL_LINK_MODE_100baseT1_Full_BIT = 67,
ETHTOOL_LINK_MODE_1000baseT1_Full_BIT = 68,
+ ETHTOOL_LINK_MODE_10baseT1L_Full_BIT = 69,
+ ETHTOOL_LINK_MODE_10baseT1S_Full_BIT = 70,
/* must be last entry */
__ETHTOOL_LINK_MODE_MASK_NBITS
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index 0a552061ff1c..6fd5ff632b8e 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -43,6 +43,7 @@
#define MDIO_PKGID1 14 /* Package identifier */
#define MDIO_PKGID2 15
#define MDIO_AN_ADVERTISE 16 /* AN advertising (base page) */
+#define MDIO_PMA_BASET1_EXTABLE 18 /* BASE-T1 PMA/PMD extended ability */
#define MDIO_AN_LPA 19 /* AN LP abilities (base page) */
#define MDIO_PCS_EEE_ABLE 20 /* EEE Capability register */
#define MDIO_PMA_NG_EXTABLE 21 /* 2.5G/5G PMA/PMD extended ability */
@@ -57,11 +58,16 @@
#define MDIO_PMA_10GBT_SNR 133 /* 10GBASE-T SNR margin, lane A.
* Lanes B-D are numbered 134-136. */
#define MDIO_PMA_10GBR_FECABLE 170 /* 10GBASE-R FEC ability */
+#define MDIO_PMA_BASET1CTRL 2100 /* BASE-T1 PMA/PMD control */
#define MDIO_PCS_10GBX_STAT1 24 /* 10GBASE-X PCS status 1 */
#define MDIO_PCS_10GBRT_STAT1 32 /* 10GBASE-R/-T PCS status 1 */
#define MDIO_PCS_10GBRT_STAT2 33 /* 10GBASE-R/-T PCS status 2 */
#define MDIO_AN_10GBT_CTRL 32 /* 10GBASE-T auto-negotiation control */
#define MDIO_AN_10GBT_STAT 33 /* 10GBASE-T auto-negotiation status */
+#define MDIO_AN_BT1_CTRL 512 /* BASE-T1 auto-negotiation control */
+#define MDIO_AN_BT1_STAT 513 /* BASE-T1 auto-negotiation status */
+#define MDIO_AN_10BT1_CTRL 526 /* 10BASE-T1 auto-negotiation control */
+#define MDIO_AN_10BT1_STAT 527 /* 10BASE-T1 auto-negotiation status */
/* LASI (Link Alarm Status Interrupt) registers, defined by XENPAK MSA. */
#define MDIO_PMA_LASI_RXCTRL 0x9000 /* RX_ALARM control */
@@ -151,6 +157,7 @@
#define MDIO_PMA_CTRL2_100BTX 0x000e /* 100BASE-TX type */
#define MDIO_PMA_CTRL2_10BT 0x000f /* 10BASE-T type */
#define MDIO_PMA_CTRL2_2_5GBT 0x0030 /* 2.5GBaseT type */
+#define MDIO_PMA_CTRL2_BT1 0x003D /* BASE-T1 type */
#define MDIO_PMA_CTRL2_5GBT 0x0031 /* 5GBaseT type */
#define MDIO_PCS_CTRL2_TYPE 0x0003 /* PCS type selection */
#define MDIO_PCS_CTRL2_10GBR 0x0000 /* 10GBASE-R type */
@@ -205,8 +212,16 @@
#define MDIO_PMA_EXTABLE_1000BKX 0x0040 /* 1000BASE-KX ability */
#define MDIO_PMA_EXTABLE_100BTX 0x0080 /* 100BASE-TX ability */
#define MDIO_PMA_EXTABLE_10BT 0x0100 /* 10BASE-T ability */
+#define MDIO_PMA_EXTABLE_BASET1 0x0800 /* BASE-T1 ability */
#define MDIO_PMA_EXTABLE_NBT 0x4000 /* 2.5/5GBASE-T ability */
+/* PMA BASE-T1 control register. */
+#define MDIO_PMA_BASET1CTRL_TYPE 0x000f /* PMA/PMD BASE-T1 type sel. */
+#define MDIO_PMA_BASET1CTRL_TYPE_100BT1 0x0000 /* 100BASE-T1 */
+#define MDIO_PMA_BASET1CTRL_TYPE_1000BT1 0x0001 /* 1000BASE-T1 */
+#define MDIO_PMA_BASET1CTRL_TYPE_10BT1L 0x0002 /* 10BASE-T1L */
+#define MDIO_PMA_BASET1CTRL_TYPE_10BT1S 0x0003 /* 10BASE-T1S */
+
/* PHY XGXS lane state register. */
#define MDIO_PHYXS_LNSTAT_SYNC0 0x0001
#define MDIO_PHYXS_LNSTAT_SYNC1 0x0002
@@ -281,6 +296,12 @@
#define MDIO_PMA_NG_EXTABLE_2_5GBT 0x0001 /* 2.5GBASET ability */
#define MDIO_PMA_NG_EXTABLE_5GBT 0x0002 /* 5GBASET ability */
+/* BASE-T1 Extended abilities register. */
+#define MDIO_PMA_BASET1_EXTABLE_100BT1 0x0001 /* 100BASE-T1 ability */
+#define MDIO_PMA_BASET1_EXTABLE_1000BT1 0x0002 /* 1000BASE-T1 ability */
+#define MDIO_PMA_BASET1_EXTABLE_10BT1L 0x0004 /* 10BASE-T1L ability */
+#define MDIO_PMA_BASET1_EXTABLE_10BT1S 0x0008 /* 10BASE-T1S ability */
+
/* LASI RX_ALARM control/status registers. */
#define MDIO_PMA_LASI_RX_PHYXSLFLT 0x0001 /* PHY XS RX local fault */
#define MDIO_PMA_LASI_RX_PCSLFLT 0x0008 /* PCS RX local fault */
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-15 15:32 [PATCH net-next 0/1] Add BASE-T1 PHY support Christian Herber
2019-08-15 15:32 ` [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem Christian Herber
@ 2019-08-15 15:43 ` Andrew Lunn
2019-08-16 20:59 ` Heiner Kallweit
2 siblings, 0 replies; 20+ messages in thread
From: Andrew Lunn @ 2019-08-15 15:43 UTC (permalink / raw)
To: Christian Herber
Cc: davem, netdev, linux-kernel, Florian Fainelli, Heiner Kallweit
On Thu, Aug 15, 2019 at 03:32:27PM +0000, Christian Herber wrote:
> This patch adds basic support for BASE-T1 PHYs in the framework.
> BASE-T1 PHYs main area of application are automotive and industrial.
> BASE-T1 is standardized in IEEE 802.3, namely
> - IEEE 802.3bw: 100BASE-T1
> - IEEE 802.3bp 1000BASE-T1
> - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
Hi Christian
Please make sure you Cc: the PHY subsystem maintainers.
Andrew
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
2019-08-15 15:32 ` [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem Christian Herber
@ 2019-08-15 15:56 ` Andrew Lunn
2019-08-15 16:34 ` Heiner Kallweit
2019-08-16 11:56 ` Christian Herber
2019-08-16 21:13 ` Heiner Kallweit
1 sibling, 2 replies; 20+ messages in thread
From: Andrew Lunn @ 2019-08-15 15:56 UTC (permalink / raw)
To: Christian Herber; +Cc: davem, netdev, linux-kernel
On Thu, Aug 15, 2019 at 03:32:29PM +0000, Christian Herber wrote:
> BASE-T1 is a category of Ethernet PHYs.
> They use a single copper pair for transmission.
> This patch add basic support for this category of PHYs.
> It coveres the discovery of abilities and basic configuration.
> It includes setting fixed speed and enabling auto-negotiation.
> BASE-T1 devices should always Clause-45 managed.
> Therefore, this patch extends phy-c45.c.
> While for some functions like auto-neogtiation different registers are
> used, the layout of these registers is the same for the used fields.
> Thus, much of the logic of basic Clause-45 devices can be reused.
>
> Signed-off-by: Christian Herber <christian.herber@nxp.com>
> ---
> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
> drivers/net/phy/phy-core.c | 4 +-
> include/uapi/linux/ethtool.h | 2 +
> include/uapi/linux/mdio.h | 21 +++++++
> 4 files changed, 129 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index b9d4145781ca..9ff0b8c785de 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -8,13 +8,23 @@
> #include <linux/mii.h>
> #include <linux/phy.h>
>
> +#define IS_100BASET1(phy) (linkmode_test_bit( \
> + ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
> + (phy)->supported))
> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
> + ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
> + (phy)->supported))
Hi Christian
We already have the flag phydev->is_gigabit_capable. Maybe add a flag
phydev->is_t1_capable
> +
> +static u32 get_aneg_ctrl(struct phy_device *phydev);
> +static u32 get_aneg_stat(struct phy_device *phydev);
No forward declarations please. Put the code in the right order so
they are not needed.
Thanks
Andrew
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
2019-08-15 15:56 ` Andrew Lunn
@ 2019-08-15 16:34 ` Heiner Kallweit
2019-08-16 12:05 ` [EXT] " Christian Herber
2019-08-16 11:56 ` Christian Herber
1 sibling, 1 reply; 20+ messages in thread
From: Heiner Kallweit @ 2019-08-15 16:34 UTC (permalink / raw)
To: Andrew Lunn, Christian Herber
Cc: davem, netdev, linux-kernel, Florian Fainelli
On 15.08.2019 17:56, Andrew Lunn wrote:
> On Thu, Aug 15, 2019 at 03:32:29PM +0000, Christian Herber wrote:
>> BASE-T1 is a category of Ethernet PHYs.
>> They use a single copper pair for transmission.
>> This patch add basic support for this category of PHYs.
>> It coveres the discovery of abilities and basic configuration.
>> It includes setting fixed speed and enabling auto-negotiation.
>> BASE-T1 devices should always Clause-45 managed.
>> Therefore, this patch extends phy-c45.c.
>> While for some functions like auto-neogtiation different registers are
>> used, the layout of these registers is the same for the used fields.
>> Thus, much of the logic of basic Clause-45 devices can be reused.
>>
>> Signed-off-by: Christian Herber <christian.herber@nxp.com>
>> ---
>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>> drivers/net/phy/phy-core.c | 4 +-
>> include/uapi/linux/ethtool.h | 2 +
>> include/uapi/linux/mdio.h | 21 +++++++
>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index b9d4145781ca..9ff0b8c785de 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -8,13 +8,23 @@
>> #include <linux/mii.h>
>> #include <linux/phy.h>
>>
>> +#define IS_100BASET1(phy) (linkmode_test_bit( \
>> + ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
>> + (phy)->supported))
>> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
>> + ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
>> + (phy)->supported))
>
> Hi Christian
>
> We already have the flag phydev->is_gigabit_capable. Maybe add a flag
> phydev->is_t1_capable
>
>> +
>> +static u32 get_aneg_ctrl(struct phy_device *phydev);
>> +static u32 get_aneg_stat(struct phy_device *phydev);
>
> No forward declarations please. Put the code in the right order so
> they are not needed.
>
> Thanks
>
> Andrew
>
For whatever reason I don't have the original mail in my netdev inbox (yet).
+ if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
+ ctrl = MDIO_AN_BT1_CTRL;
Code like this could be problematic once a PHY supports one of the T1 modes
AND normal modes. Then normal modes would be unusable.
I think this scenario isn't completely hypothetical. See the Aquantia
AQCS109 that supports normal modes and (proprietary) 1000Base-T2.
Maybe we need separate versions of the generic functions for T1.
Then it would be up to the PHY driver to decide when to use which
version.
Heiner
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
2019-08-15 15:56 ` Andrew Lunn
2019-08-15 16:34 ` Heiner Kallweit
@ 2019-08-16 11:56 ` Christian Herber
1 sibling, 0 replies; 20+ messages in thread
From: Christian Herber @ 2019-08-16 11:56 UTC (permalink / raw)
To: Andrew Lunn; +Cc: davem, netdev, linux-kernel
On 15.08.2019 17:56, Andrew Lunn wrote:
> Caution: EXT Email
>
> On Thu, Aug 15, 2019 at 03:32:29PM +0000, Christian Herber wrote:
>> BASE-T1 is a category of Ethernet PHYs.
>> They use a single copper pair for transmission.
>> This patch add basic support for this category of PHYs.
>> It coveres the discovery of abilities and basic configuration.
>> It includes setting fixed speed and enabling auto-negotiation.
>> BASE-T1 devices should always Clause-45 managed.
>> Therefore, this patch extends phy-c45.c.
>> While for some functions like auto-neogtiation different registers are
>> used, the layout of these registers is the same for the used fields.
>> Thus, much of the logic of basic Clause-45 devices can be reused.
>>
>> Signed-off-by: Christian Herber <christian.herber@nxp.com>
>> ---
>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>> drivers/net/phy/phy-core.c | 4 +-
>> include/uapi/linux/ethtool.h | 2 +
>> include/uapi/linux/mdio.h | 21 +++++++
>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index b9d4145781ca..9ff0b8c785de 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -8,13 +8,23 @@
>> #include <linux/mii.h>
>> #include <linux/phy.h>
>>
>> +#define IS_100BASET1(phy) (linkmode_test_bit( \
>> + ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
>> + (phy)->supported))
>> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
>> + ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
>> + (phy)->supported))
>
> Hi Christian
>
> We already have the flag phydev->is_gigabit_capable. Maybe add a flag
> phydev->is_t1_capable
>
>> +
>> +static u32 get_aneg_ctrl(struct phy_device *phydev);
>> +static u32 get_aneg_stat(struct phy_device *phydev);
>
> No forward declarations please. Put the code in the right order so
> they are not needed.
>
> Thanks
>
> Andrew
>
Hi Andrew,
thanks for feedback. The use of an additional flag is a good proposal.
I was hesitant to touch the phydev structure.
I will add this along with removing the forward declaration in v2.
Regards,
Christian
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
2019-08-15 16:34 ` Heiner Kallweit
@ 2019-08-16 12:05 ` Christian Herber
0 siblings, 0 replies; 20+ messages in thread
From: Christian Herber @ 2019-08-16 12:05 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: davem, netdev, linux-kernel, Florian Fainelli
On 15.08.2019 18:34, Heiner Kallweit wrote:
> Caution: EXT Email
>
> On 15.08.2019 17:56, Andrew Lunn wrote:
>> On Thu, Aug 15, 2019 at 03:32:29PM +0000, Christian Herber wrote:
>>> BASE-T1 is a category of Ethernet PHYs.
>>> They use a single copper pair for transmission.
>>> This patch add basic support for this category of PHYs.
>>> It coveres the discovery of abilities and basic configuration.
>>> It includes setting fixed speed and enabling auto-negotiation.
>>> BASE-T1 devices should always Clause-45 managed.
>>> Therefore, this patch extends phy-c45.c.
>>> While for some functions like auto-neogtiation different registers are
>>> used, the layout of these registers is the same for the used fields.
>>> Thus, much of the logic of basic Clause-45 devices can be reused.
>>>
>>> Signed-off-by: Christian Herber <christian.herber@nxp.com>
>>> ---
>>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>>> drivers/net/phy/phy-core.c | 4 +-
>>> include/uapi/linux/ethtool.h | 2 +
>>> include/uapi/linux/mdio.h | 21 +++++++
>>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>>> index b9d4145781ca..9ff0b8c785de 100644
>>> --- a/drivers/net/phy/phy-c45.c
>>> +++ b/drivers/net/phy/phy-c45.c
>>> @@ -8,13 +8,23 @@
>>> #include <linux/mii.h>
>>> #include <linux/phy.h>
>>>
>>> +#define IS_100BASET1(phy) (linkmode_test_bit( \
>>> + ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
>>> + (phy)->supported))
>>> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
>>> + ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
>>> + (phy)->supported))
>>
>> Hi Christian
>>
>> We already have the flag phydev->is_gigabit_capable. Maybe add a flag
>> phydev->is_t1_capable
>>
>>> +
>>> +static u32 get_aneg_ctrl(struct phy_device *phydev);
>>> +static u32 get_aneg_stat(struct phy_device *phydev);
>>
>> No forward declarations please. Put the code in the right order so
>> they are not needed.
>>
>> Thanks
>>
>> Andrew
>>
>
> For whatever reason I don't have the original mail in my netdev inbox (yet).
>
> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
> + ctrl = MDIO_AN_BT1_CTRL;
>
> Code like this could be problematic once a PHY supports one of the T1 modes
> AND normal modes. Then normal modes would be unusable.
>
> I think this scenario isn't completely hypothetical. See the Aquantia
> AQCS109 that supports normal modes and (proprietary) 1000Base-T2.
>
> Maybe we need separate versions of the generic functions for T1.
> Then it would be up to the PHY driver to decide when to use which
> version.
>
> Heiner
>
Integrating this with the existing driver or creating a new on is an
interesting question. I came to the conclusion that it is most efficient
to integrate to avoid all to much copy paste code.
So far, I am not aware of any device that supports T1 and something else
at the same time. From a HW perspective, I also consider this quite
unlikely. In the unlikely case that such a device comes up, support from
the genphy driver would be limited to the BASE-T1 modes. But i would
rather create the special case for the special device and cater the
current mainstream support to the mainstream devices.
I think this boils down to a general strategy for the PHY framework, as
this can happen for other classes of devices also like NGBASE-T1,
MultiGBASE-T and future unknown devices. For now, I think the
architecture is sufficiently scalable with a single c45 genphy driver
Christian
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-15 15:32 [PATCH net-next 0/1] Add BASE-T1 PHY support Christian Herber
2019-08-15 15:32 ` [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem Christian Herber
2019-08-15 15:43 ` [PATCH net-next 0/1] Add BASE-T1 PHY support Andrew Lunn
@ 2019-08-16 20:59 ` Heiner Kallweit
2019-08-19 6:32 ` Christian Herber
2019-10-16 8:37 ` Lucas Stach
2 siblings, 2 replies; 20+ messages in thread
From: Heiner Kallweit @ 2019-08-16 20:59 UTC (permalink / raw)
To: Christian Herber, davem; +Cc: netdev, linux-kernel
On 15.08.2019 17:32, Christian Herber wrote:
> This patch adds basic support for BASE-T1 PHYs in the framework.
> BASE-T1 PHYs main area of application are automotive and industrial.
> BASE-T1 is standardized in IEEE 802.3, namely
> - IEEE 802.3bw: 100BASE-T1
> - IEEE 802.3bp 1000BASE-T1
> - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>
> There are no products which contain BASE-T1 and consumer type PHYs like
> 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
> PHYs with auto-negotiation.
Is this meant in a way that *currently* there are no PHY's combining Base-T1
with normal Base-T modes? Or are there reasons why this isn't possible in
general? I'm asking because we have PHY's combining copper and fiber, and e.g.
the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
>
> The intention of this patch is to make use of the existing Clause 45 functions.
> BASE-T1 adds some additional registers e.g. for aneg control, which follow a
> similiar register layout as the existing devices. The bits which are used in
> BASE-T1 specific registers are the same as in basic registers, thus the
> existing functions can be resued, with get_aneg_ctrl() selecting the correct
> register address.
>
If Base-T1 can't be combined with other modes then at a first glance I see no
benefit in defining new registers e.g. for aneg control, and the standard ones
are unused. Why not using the standard registers? Can you shed some light on that?
Are the new registers internally shadowed to the standard location?
That's something I've seen on other PHY's: one register appears in different
places in different devices.
> The current version of ethtool has been prepared for 100/1000BASE-T1 and works
> with this patch. 10BASE-T1 needs to be added to ethtool.
>
> Christian Herber (1):
> Added BASE-T1 PHY support to PHY Subsystem
>
> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
> drivers/net/phy/phy-core.c | 4 +-
> include/uapi/linux/ethtool.h | 2 +
> include/uapi/linux/mdio.h | 21 +++++++
> 4 files changed, 129 insertions(+), 11 deletions(-)
>
Heiner
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
2019-08-15 15:32 ` [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem Christian Herber
2019-08-15 15:56 ` Andrew Lunn
@ 2019-08-16 21:13 ` Heiner Kallweit
2019-08-19 6:40 ` Christian Herber
1 sibling, 1 reply; 20+ messages in thread
From: Heiner Kallweit @ 2019-08-16 21:13 UTC (permalink / raw)
To: Christian Herber, davem; +Cc: netdev, linux-kernel
On 15.08.2019 17:32, Christian Herber wrote:
> BASE-T1 is a category of Ethernet PHYs.
> They use a single copper pair for transmission.
> This patch add basic support for this category of PHYs.
> It coveres the discovery of abilities and basic configuration.
> It includes setting fixed speed and enabling auto-negotiation.
> BASE-T1 devices should always Clause-45 managed.
> Therefore, this patch extends phy-c45.c.
> While for some functions like auto-neogtiation different registers are
> used, the layout of these registers is the same for the used fields.
> Thus, much of the logic of basic Clause-45 devices can be reused.
>
> Signed-off-by: Christian Herber <christian.herber@nxp.com>
> ---
> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
> drivers/net/phy/phy-core.c | 4 +-
> include/uapi/linux/ethtool.h | 2 +
> include/uapi/linux/mdio.h | 21 +++++++
> 4 files changed, 129 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index b9d4145781ca..9ff0b8c785de 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -8,13 +8,23 @@
> #include <linux/mii.h>
> #include <linux/phy.h>
>
> +#define IS_100BASET1(phy) (linkmode_test_bit( \
> + ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
> + (phy)->supported))
> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
> + ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
> + (phy)->supported))
> +
Why is there no such macro for 10BaseT1?
> +static u32 get_aneg_ctrl(struct phy_device *phydev);
> +static u32 get_aneg_stat(struct phy_device *phydev);
> +
> /**
> * genphy_c45_setup_forced - configures a forced speed
> * @phydev: target phy_device struct
> */
> int genphy_c45_pma_setup_forced(struct phy_device *phydev)
> {
> - int ctrl1, ctrl2, ret;
> + int ctrl1, ctrl2, base_t1_ctrl = 0, ret;
>
> /* Half duplex is not supported */
> if (phydev->duplex != DUPLEX_FULL)
> @@ -28,6 +38,16 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
> if (ctrl2 < 0)
> return ctrl2;
>
> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev)) {
10BaseT1 doesn't need to be considered here?
And maybe it would be better to have a flag for BaseT1 at phy_device level
(based on bit MDIO_PMA_EXTABLE_BASET1?) instead of checking whether certain
T1 modes are supported. Then we would be more future-proof in case of new
T1 modes.
> + base_t1_ctrl = phy_read_mmd(phydev,
> + MDIO_MMD_PMAPMD,
> + MDIO_PMA_BASET1CTRL);
> + if (base_t1_ctrl < 0)
> + return base_t1_ctrl;
> +
> + base_t1_ctrl &= ~(MDIO_PMA_BASET1CTRL_TYPE);
> + }
> +
> ctrl1 &= ~MDIO_CTRL1_SPEEDSEL;
> /*
> * PMA/PMD type selection is 1.7.5:0 not 1.7.3:0. See 45.2.1.6.1
> @@ -41,12 +61,21 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
> break;
> case SPEED_100:
> ctrl1 |= MDIO_PMA_CTRL1_SPEED100;
> - ctrl2 |= MDIO_PMA_CTRL2_100BTX;
> + if (IS_100BASET1(phydev)) {
> + ctrl2 |= MDIO_PMA_CTRL2_BT1;
> + base_t1_ctrl |= MDIO_PMA_BASET1CTRL_TYPE_100BT1;
> + } else {
> + ctrl2 |= MDIO_PMA_CTRL2_100BTX;
> + }
> break;
> case SPEED_1000:
> ctrl1 |= MDIO_PMA_CTRL1_SPEED1000;
> - /* Assume 1000base-T */
> - ctrl2 |= MDIO_PMA_CTRL2_1000BT;
> + if (IS_1000BASET1(phydev)) {
> + ctrl2 |= MDIO_PMA_CTRL2_BT1;
> + base_t1_ctrl |= MDIO_PMA_BASET1CTRL_TYPE_1000BT1;
> + } else {
> + ctrl2 |= MDIO_PMA_CTRL2_1000BT;
> + }
> break;
> case SPEED_2500:
> ctrl1 |= MDIO_CTRL1_SPEED2_5G;
> @@ -75,6 +104,14 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
> if (ret < 0)
> return ret;
>
> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev)) {
> + ret = phy_write_mmd(phydev,
> + MDIO_MMD_PMAPMD,
> + MDIO_PMA_BASET1CTRL,
> + base_t1_ctrl);
> + if (ret < 0)
> + return ret;
> + }
> return genphy_c45_an_disable_aneg(phydev);
> }
> EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
> @@ -135,8 +172,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_an_config_aneg);
> */
> int genphy_c45_an_disable_aneg(struct phy_device *phydev)
> {
> -
> - return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1,
> + return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev),
> MDIO_AN_CTRL1_ENABLE | MDIO_AN_CTRL1_RESTART);
> }
> EXPORT_SYMBOL_GPL(genphy_c45_an_disable_aneg);
> @@ -151,7 +187,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_an_disable_aneg);
> */
> int genphy_c45_restart_aneg(struct phy_device *phydev)
> {
> - return phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1,
> + return phy_set_bits_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev),
> MDIO_AN_CTRL1_ENABLE | MDIO_AN_CTRL1_RESTART);
> }
> EXPORT_SYMBOL_GPL(genphy_c45_restart_aneg);
> @@ -171,7 +207,7 @@ int genphy_c45_check_and_restart_aneg(struct phy_device *phydev, bool restart)
>
> if (!restart) {
> /* Configure and restart aneg if it wasn't set before */
> - ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
> + ret = phy_read_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev));
> if (ret < 0)
> return ret;
>
> @@ -199,7 +235,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_check_and_restart_aneg);
> */
> int genphy_c45_aneg_done(struct phy_device *phydev)
> {
> - int val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> + int val = phy_read_mmd(phydev, MDIO_MMD_AN, get_aneg_stat(phydev));
>
> return val < 0 ? val : val & MDIO_AN_STAT1_COMPLETE ? 1 : 0;
> }
> @@ -385,7 +421,9 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
> * PMA Extended Abilities (1.11) register, indicating 1000BASET an 10G related
> * modes. If bit 1.11.14 is set, then the list is also extended with the modes
> * in the 2.5G/5G PMA Extended register (1.21), indicating if 2.5GBASET and
> - * 5GBASET are supported.
> + * 5GBASET are supported. If bit 1.11.11 is set, then the list is also extended
> + * with the modes in the BASE-T1 PMA Extended register (1.18), indicating if
> + * 10/100/1000BASET-1 are supported.
> */
> int genphy_c45_pma_read_abilities(struct phy_device *phydev)
> {
> @@ -470,6 +508,29 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
> phydev->supported,
> val & MDIO_PMA_NG_EXTABLE_5GBT);
> }
> +
> + if (val & MDIO_PMA_EXTABLE_BASET1) {
> + val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
> + MDIO_PMA_BASET1_EXTABLE);
> + if (val < 0)
> + return val;
> +
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
> + phydev->supported,
> + val & MDIO_PMA_BASET1_EXTABLE_100BT1);
> +
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
> + phydev->supported,
> + val & MDIO_PMA_BASET1_EXTABLE_1000BT1);
> +
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT,
> + phydev->supported,
> + val & MDIO_PMA_BASET1_EXTABLE_10BT1L);
> +
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1S_Full_BIT,
> + phydev->supported,
> + val & MDIO_PMA_BASET1_EXTABLE_10BT1S);
> + }
> }
>
> return 0;
> @@ -509,6 +570,38 @@ int genphy_c45_read_status(struct phy_device *phydev)
> }
> EXPORT_SYMBOL_GPL(genphy_c45_read_status);
>
> +/**
> + * get_aneg_ctrl - Get the register address for auto-
> + * negotiation control register
> + * @phydev: target phy_device struct
> + *
> + */
> +static u32 get_aneg_ctrl(struct phy_device *phydev)
> +{
> + u32 ctrl = MDIO_CTRL1;
> +
> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
> + ctrl = MDIO_AN_BT1_CTRL;
> +
AFAICS 10BaseT1 has separate aneg registers (526/527).
To be considered here?
> + return ctrl;
> +}
> +
> +/**
> + * get_aneg_ctrl - Get the register address for auto-
> + * negotiation status register
> + * @phydev: target phy_device struct
> + *
> + */
> +static u32 get_aneg_stat(struct phy_device *phydev)
> +{
> + u32 stat = MDIO_STAT1;
> +
> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
> + stat = MDIO_AN_BT1_STAT;
> +
> + return stat;
> +}
> +
> /* The gen10g_* functions are the old Clause 45 stub */
>
> int gen10g_config_aneg(struct phy_device *phydev)
> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
> index 369903d9b6ec..b50576f7709a 100644
> --- a/drivers/net/phy/phy-core.c
> +++ b/drivers/net/phy/phy-core.c
> @@ -8,7 +8,7 @@
>
> const char *phy_speed_to_str(int speed)
> {
> - BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 69,
> + BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 71,
> "Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
> "If a speed or mode has been added please update phy_speed_to_str "
> "and the PHY settings array.\n");
> @@ -140,6 +140,8 @@ static const struct phy_setting settings[] = {
> /* 10M */
> PHY_SETTING( 10, FULL, 10baseT_Full ),
> PHY_SETTING( 10, HALF, 10baseT_Half ),
> + PHY_SETTING( 10, FULL, 10baseT1L_Full ),
> + PHY_SETTING( 10, FULL, 10baseT1S_Full ),
> };
> #undef PHY_SETTING
>
> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> index dd06302aa93e..e429cc8da31a 100644
> --- a/include/uapi/linux/ethtool.h
> +++ b/include/uapi/linux/ethtool.h
> @@ -1485,6 +1485,8 @@ enum ethtool_link_mode_bit_indices {
> ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66,
> ETHTOOL_LINK_MODE_100baseT1_Full_BIT = 67,
> ETHTOOL_LINK_MODE_1000baseT1_Full_BIT = 68,
> + ETHTOOL_LINK_MODE_10baseT1L_Full_BIT = 69,
> + ETHTOOL_LINK_MODE_10baseT1S_Full_BIT = 70,
>
> /* must be last entry */
> __ETHTOOL_LINK_MODE_MASK_NBITS
> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
> index 0a552061ff1c..6fd5ff632b8e 100644
> --- a/include/uapi/linux/mdio.h
> +++ b/include/uapi/linux/mdio.h
> @@ -43,6 +43,7 @@
> #define MDIO_PKGID1 14 /* Package identifier */
> #define MDIO_PKGID2 15
> #define MDIO_AN_ADVERTISE 16 /* AN advertising (base page) */
> +#define MDIO_PMA_BASET1_EXTABLE 18 /* BASE-T1 PMA/PMD extended ability */
> #define MDIO_AN_LPA 19 /* AN LP abilities (base page) */
> #define MDIO_PCS_EEE_ABLE 20 /* EEE Capability register */
> #define MDIO_PMA_NG_EXTABLE 21 /* 2.5G/5G PMA/PMD extended ability */
> @@ -57,11 +58,16 @@
> #define MDIO_PMA_10GBT_SNR 133 /* 10GBASE-T SNR margin, lane A.
> * Lanes B-D are numbered 134-136. */
> #define MDIO_PMA_10GBR_FECABLE 170 /* 10GBASE-R FEC ability */
> +#define MDIO_PMA_BASET1CTRL 2100 /* BASE-T1 PMA/PMD control */
> #define MDIO_PCS_10GBX_STAT1 24 /* 10GBASE-X PCS status 1 */
> #define MDIO_PCS_10GBRT_STAT1 32 /* 10GBASE-R/-T PCS status 1 */
> #define MDIO_PCS_10GBRT_STAT2 33 /* 10GBASE-R/-T PCS status 2 */
> #define MDIO_AN_10GBT_CTRL 32 /* 10GBASE-T auto-negotiation control */
> #define MDIO_AN_10GBT_STAT 33 /* 10GBASE-T auto-negotiation status */
> +#define MDIO_AN_BT1_CTRL 512 /* BASE-T1 auto-negotiation control */
> +#define MDIO_AN_BT1_STAT 513 /* BASE-T1 auto-negotiation status */
> +#define MDIO_AN_10BT1_CTRL 526 /* 10BASE-T1 auto-negotiation control */
> +#define MDIO_AN_10BT1_STAT 527 /* 10BASE-T1 auto-negotiation status */
>
> /* LASI (Link Alarm Status Interrupt) registers, defined by XENPAK MSA. */
> #define MDIO_PMA_LASI_RXCTRL 0x9000 /* RX_ALARM control */
> @@ -151,6 +157,7 @@
> #define MDIO_PMA_CTRL2_100BTX 0x000e /* 100BASE-TX type */
> #define MDIO_PMA_CTRL2_10BT 0x000f /* 10BASE-T type */
> #define MDIO_PMA_CTRL2_2_5GBT 0x0030 /* 2.5GBaseT type */
> +#define MDIO_PMA_CTRL2_BT1 0x003D /* BASE-T1 type */
> #define MDIO_PMA_CTRL2_5GBT 0x0031 /* 5GBaseT type */
> #define MDIO_PCS_CTRL2_TYPE 0x0003 /* PCS type selection */
> #define MDIO_PCS_CTRL2_10GBR 0x0000 /* 10GBASE-R type */
> @@ -205,8 +212,16 @@
> #define MDIO_PMA_EXTABLE_1000BKX 0x0040 /* 1000BASE-KX ability */
> #define MDIO_PMA_EXTABLE_100BTX 0x0080 /* 100BASE-TX ability */
> #define MDIO_PMA_EXTABLE_10BT 0x0100 /* 10BASE-T ability */
> +#define MDIO_PMA_EXTABLE_BASET1 0x0800 /* BASE-T1 ability */
> #define MDIO_PMA_EXTABLE_NBT 0x4000 /* 2.5/5GBASE-T ability */
>
> +/* PMA BASE-T1 control register. */
> +#define MDIO_PMA_BASET1CTRL_TYPE 0x000f /* PMA/PMD BASE-T1 type sel. */
> +#define MDIO_PMA_BASET1CTRL_TYPE_100BT1 0x0000 /* 100BASE-T1 */
> +#define MDIO_PMA_BASET1CTRL_TYPE_1000BT1 0x0001 /* 1000BASE-T1 */
> +#define MDIO_PMA_BASET1CTRL_TYPE_10BT1L 0x0002 /* 10BASE-T1L */
> +#define MDIO_PMA_BASET1CTRL_TYPE_10BT1S 0x0003 /* 10BASE-T1S */
> +
> /* PHY XGXS lane state register. */
> #define MDIO_PHYXS_LNSTAT_SYNC0 0x0001
> #define MDIO_PHYXS_LNSTAT_SYNC1 0x0002
> @@ -281,6 +296,12 @@
> #define MDIO_PMA_NG_EXTABLE_2_5GBT 0x0001 /* 2.5GBASET ability */
> #define MDIO_PMA_NG_EXTABLE_5GBT 0x0002 /* 5GBASET ability */
>
> +/* BASE-T1 Extended abilities register. */
> +#define MDIO_PMA_BASET1_EXTABLE_100BT1 0x0001 /* 100BASE-T1 ability */
> +#define MDIO_PMA_BASET1_EXTABLE_1000BT1 0x0002 /* 1000BASE-T1 ability */
> +#define MDIO_PMA_BASET1_EXTABLE_10BT1L 0x0004 /* 10BASE-T1L ability */
> +#define MDIO_PMA_BASET1_EXTABLE_10BT1S 0x0008 /* 10BASE-T1S ability */
> +
> /* LASI RX_ALARM control/status registers. */
> #define MDIO_PMA_LASI_RX_PHYXSLFLT 0x0001 /* PHY XS RX local fault */
> #define MDIO_PMA_LASI_RX_PCSLFLT 0x0008 /* PCS RX local fault */
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-16 20:59 ` Heiner Kallweit
@ 2019-08-19 6:32 ` Christian Herber
2019-08-19 19:07 ` Heiner Kallweit
2019-10-16 8:37 ` Lucas Stach
1 sibling, 1 reply; 20+ messages in thread
From: Christian Herber @ 2019-08-19 6:32 UTC (permalink / raw)
To: Heiner Kallweit, davem; +Cc: netdev, linux-kernel
On 16.08.2019 22:59, Heiner Kallweit wrote:
> On 15.08.2019 17:32, Christian Herber wrote:
>> This patch adds basic support for BASE-T1 PHYs in the framework.
>> BASE-T1 PHYs main area of application are automotive and industrial.
>> BASE-T1 is standardized in IEEE 802.3, namely
>> - IEEE 802.3bw: 100BASE-T1
>> - IEEE 802.3bp 1000BASE-T1
>> - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>>
>> There are no products which contain BASE-T1 and consumer type PHYs like
>> 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
>> PHYs with auto-negotiation.
>
> Is this meant in a way that *currently* there are no PHY's combining Base-T1
> with normal Base-T modes? Or are there reasons why this isn't possible in
> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
>
>>
>> The intention of this patch is to make use of the existing Clause 45 functions.
>> BASE-T1 adds some additional registers e.g. for aneg control, which follow a
>> similiar register layout as the existing devices. The bits which are used in
>> BASE-T1 specific registers are the same as in basic registers, thus the
>> existing functions can be resued, with get_aneg_ctrl() selecting the correct
>> register address.
>>
> If Base-T1 can't be combined with other modes then at a first glance I see no
> benefit in defining new registers e.g. for aneg control, and the standard ones
> are unused. Why not using the standard registers? Can you shed some light on that?
>
> Are the new registers internally shadowed to the standard location?
> That's something I've seen on other PHY's: one register appears in different
> places in different devices.
>
>> The current version of ethtool has been prepared for 100/1000BASE-T1 and works
>> with this patch. 10BASE-T1 needs to be added to ethtool.
>>
>> Christian Herber (1):
>> Added BASE-T1 PHY support to PHY Subsystem
>>
>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>> drivers/net/phy/phy-core.c | 4 +-
>> include/uapi/linux/ethtool.h | 2 +
>> include/uapi/linux/mdio.h | 21 +++++++
>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>
>
> Heiner
>
Hi Heiner,
I do not think the Aquantia part you are describing is publicly
documented, so i cannot comment on that part.
There are multiple reasons why e.g. xBASE-T1 plus 1000BASE-T is
unlikely. First, the is no use-case known to me, where this would be
required. Second, there is no way that you can do an auto-negotiation
between the two, as these both have their own auto-neg defined (Clause
28/73 vs. Clause 98). Thirdly, if you would ever have a product with
both, I believe it would just include two full PHYs and a way to select
which flavor you want. Of course, this is the theory until proven
otherwise, but to me it is sufficient to use a single driver.
The registers are different in the fields they include. It is just that
the flags which are used by the Linux driver, like restarting auto-neg,
are at the same position.
Christian
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
2019-08-16 21:13 ` Heiner Kallweit
@ 2019-08-19 6:40 ` Christian Herber
0 siblings, 0 replies; 20+ messages in thread
From: Christian Herber @ 2019-08-19 6:40 UTC (permalink / raw)
To: Heiner Kallweit, davem; +Cc: netdev, linux-kernel
On 16.08.2019 23:13, Heiner Kallweit wrote:
>
> On 15.08.2019 17:32, Christian Herber wrote:
>> BASE-T1 is a category of Ethernet PHYs.
>> They use a single copper pair for transmission.
>> This patch add basic support for this category of PHYs.
>> It coveres the discovery of abilities and basic configuration.
>> It includes setting fixed speed and enabling auto-negotiation.
>> BASE-T1 devices should always Clause-45 managed.
>> Therefore, this patch extends phy-c45.c.
>> While for some functions like auto-neogtiation different registers are
>> used, the layout of these registers is the same for the used fields.
>> Thus, much of the logic of basic Clause-45 devices can be reused.
>>
>> Signed-off-by: Christian Herber <christian.herber@nxp.com>
>> ---
>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>> drivers/net/phy/phy-core.c | 4 +-
>> include/uapi/linux/ethtool.h | 2 +
>> include/uapi/linux/mdio.h | 21 +++++++
>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index b9d4145781ca..9ff0b8c785de 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -8,13 +8,23 @@
>> #include <linux/mii.h>
>> #include <linux/phy.h>
>>
>> +#define IS_100BASET1(phy) (linkmode_test_bit( \
>> + ETHTOOL_LINK_MODE_100baseT1_Full_BIT, \
>> + (phy)->supported))
>> +#define IS_1000BASET1(phy) (linkmode_test_bit( \
>> + ETHTOOL_LINK_MODE_1000baseT1_Full_BIT, \
>> + (phy)->supported))
>> +
> Why is there no such macro for 10BaseT1?
>
>> +static u32 get_aneg_ctrl(struct phy_device *phydev);
>> +static u32 get_aneg_stat(struct phy_device *phydev);
>> +
>> /**
>> * genphy_c45_setup_forced - configures a forced speed
>> * @phydev: target phy_device struct
>> */
>> int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>> {
>> - int ctrl1, ctrl2, ret;
>> + int ctrl1, ctrl2, base_t1_ctrl = 0, ret;
>>
>> /* Half duplex is not supported */
>> if (phydev->duplex != DUPLEX_FULL)
>> @@ -28,6 +38,16 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>> if (ctrl2 < 0)
>> return ctrl2;
>>
>> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev)) {
>
> 10BaseT1 doesn't need to be considered here?
> And maybe it would be better to have a flag for BaseT1 at phy_device level
> (based on bit MDIO_PMA_EXTABLE_BASET1?) instead of checking whether certain
> T1 modes are supported. Then we would be more future-proof in case of new
> T1 modes.
>
>> + base_t1_ctrl = phy_read_mmd(phydev,
>> + MDIO_MMD_PMAPMD,
>> + MDIO_PMA_BASET1CTRL);
>> + if (base_t1_ctrl < 0)
>> + return base_t1_ctrl;
>> +
>> + base_t1_ctrl &= ~(MDIO_PMA_BASET1CTRL_TYPE);
>> + }
>> +
>> ctrl1 &= ~MDIO_CTRL1_SPEEDSEL;
>> /*
>> * PMA/PMD type selection is 1.7.5:0 not 1.7.3:0. See 45.2.1.6.1
>> @@ -41,12 +61,21 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>> break;
>> case SPEED_100:
>> ctrl1 |= MDIO_PMA_CTRL1_SPEED100;
>> - ctrl2 |= MDIO_PMA_CTRL2_100BTX;
>> + if (IS_100BASET1(phydev)) {
>> + ctrl2 |= MDIO_PMA_CTRL2_BT1;
>> + base_t1_ctrl |= MDIO_PMA_BASET1CTRL_TYPE_100BT1;
>> + } else {
>> + ctrl2 |= MDIO_PMA_CTRL2_100BTX;
>> + }
>> break;
>> case SPEED_1000:
>> ctrl1 |= MDIO_PMA_CTRL1_SPEED1000;
>> - /* Assume 1000base-T */
>> - ctrl2 |= MDIO_PMA_CTRL2_1000BT;
>> + if (IS_1000BASET1(phydev)) {
>> + ctrl2 |= MDIO_PMA_CTRL2_BT1;
>> + base_t1_ctrl |= MDIO_PMA_BASET1CTRL_TYPE_1000BT1;
>> + } else {
>> + ctrl2 |= MDIO_PMA_CTRL2_1000BT;
>> + }
>> break;
>> case SPEED_2500:
>> ctrl1 |= MDIO_CTRL1_SPEED2_5G;
>> @@ -75,6 +104,14 @@ int genphy_c45_pma_setup_forced(struct phy_device *phydev)
>> if (ret < 0)
>> return ret;
>>
>> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev)) {
>> + ret = phy_write_mmd(phydev,
>> + MDIO_MMD_PMAPMD,
>> + MDIO_PMA_BASET1CTRL,
>> + base_t1_ctrl);
>> + if (ret < 0)
>> + return ret;
>> + }
>> return genphy_c45_an_disable_aneg(phydev);
>> }
>> EXPORT_SYMBOL_GPL(genphy_c45_pma_setup_forced);
>> @@ -135,8 +172,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_an_config_aneg);
>> */
>> int genphy_c45_an_disable_aneg(struct phy_device *phydev)
>> {
>> -
>> - return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1,
>> + return phy_clear_bits_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev),
>> MDIO_AN_CTRL1_ENABLE | MDIO_AN_CTRL1_RESTART);
>> }
>> EXPORT_SYMBOL_GPL(genphy_c45_an_disable_aneg);
>> @@ -151,7 +187,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_an_disable_aneg);
>> */
>> int genphy_c45_restart_aneg(struct phy_device *phydev)
>> {
>> - return phy_set_bits_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1,
>> + return phy_set_bits_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev),
>> MDIO_AN_CTRL1_ENABLE | MDIO_AN_CTRL1_RESTART);
>> }
>> EXPORT_SYMBOL_GPL(genphy_c45_restart_aneg);
>> @@ -171,7 +207,7 @@ int genphy_c45_check_and_restart_aneg(struct phy_device *phydev, bool restart)
>>
>> if (!restart) {
>> /* Configure and restart aneg if it wasn't set before */
>> - ret = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
>> + ret = phy_read_mmd(phydev, MDIO_MMD_AN, get_aneg_ctrl(phydev));
>> if (ret < 0)
>> return ret;
>>
>> @@ -199,7 +235,7 @@ EXPORT_SYMBOL_GPL(genphy_c45_check_and_restart_aneg);
>> */
>> int genphy_c45_aneg_done(struct phy_device *phydev)
>> {
>> - int val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
>> + int val = phy_read_mmd(phydev, MDIO_MMD_AN, get_aneg_stat(phydev));
>>
>> return val < 0 ? val : val & MDIO_AN_STAT1_COMPLETE ? 1 : 0;
>> }
>> @@ -385,7 +421,9 @@ EXPORT_SYMBOL_GPL(genphy_c45_read_mdix);
>> * PMA Extended Abilities (1.11) register, indicating 1000BASET an 10G related
>> * modes. If bit 1.11.14 is set, then the list is also extended with the modes
>> * in the 2.5G/5G PMA Extended register (1.21), indicating if 2.5GBASET and
>> - * 5GBASET are supported.
>> + * 5GBASET are supported. If bit 1.11.11 is set, then the list is also extended
>> + * with the modes in the BASE-T1 PMA Extended register (1.18), indicating if
>> + * 10/100/1000BASET-1 are supported.
>> */
>> int genphy_c45_pma_read_abilities(struct phy_device *phydev)
>> {
>> @@ -470,6 +508,29 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
>> phydev->supported,
>> val & MDIO_PMA_NG_EXTABLE_5GBT);
>> }
>> +
>> + if (val & MDIO_PMA_EXTABLE_BASET1) {
>> + val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
>> + MDIO_PMA_BASET1_EXTABLE);
>> + if (val < 0)
>> + return val;
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_100baseT1_Full_BIT,
>> + phydev->supported,
>> + val & MDIO_PMA_BASET1_EXTABLE_100BT1);
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT1_Full_BIT,
>> + phydev->supported,
>> + val & MDIO_PMA_BASET1_EXTABLE_1000BT1);
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1L_Full_BIT,
>> + phydev->supported,
>> + val & MDIO_PMA_BASET1_EXTABLE_10BT1L);
>> +
>> + linkmode_mod_bit(ETHTOOL_LINK_MODE_10baseT1S_Full_BIT,
>> + phydev->supported,
>> + val & MDIO_PMA_BASET1_EXTABLE_10BT1S);
>> + }
>> }
>>
>> return 0;
>> @@ -509,6 +570,38 @@ int genphy_c45_read_status(struct phy_device *phydev)
>> }
>> EXPORT_SYMBOL_GPL(genphy_c45_read_status);
>>
>> +/**
>> + * get_aneg_ctrl - Get the register address for auto-
>> + * negotiation control register
>> + * @phydev: target phy_device struct
>> + *
>> + */
>> +static u32 get_aneg_ctrl(struct phy_device *phydev)
>> +{
>> + u32 ctrl = MDIO_CTRL1;
>> +
>> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
>> + ctrl = MDIO_AN_BT1_CTRL;
>> +
> AFAICS 10BaseT1 has separate aneg registers (526/527).
> To be considered here?
>
>> + return ctrl;
>> +}
>> +
>> +/**
>> + * get_aneg_ctrl - Get the register address for auto-
>> + * negotiation status register
>> + * @phydev: target phy_device struct
>> + *
>> + */
>> +static u32 get_aneg_stat(struct phy_device *phydev)
>> +{
>> + u32 stat = MDIO_STAT1;
>> +
>> + if (IS_100BASET1(phydev) || IS_1000BASET1(phydev))
>> + stat = MDIO_AN_BT1_STAT;
>> +
>> + return stat;
>> +}
>> +
>> /* The gen10g_* functions are the old Clause 45 stub */
>>
>> int gen10g_config_aneg(struct phy_device *phydev)
>> diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
>> index 369903d9b6ec..b50576f7709a 100644
>> --- a/drivers/net/phy/phy-core.c
>> +++ b/drivers/net/phy/phy-core.c
>> @@ -8,7 +8,7 @@
>>
>> const char *phy_speed_to_str(int speed)
>> {
>> - BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 69,
>> + BUILD_BUG_ON_MSG(__ETHTOOL_LINK_MODE_MASK_NBITS != 71,
>> "Enum ethtool_link_mode_bit_indices and phylib are out of sync. "
>> "If a speed or mode has been added please update phy_speed_to_str "
>> "and the PHY settings array.\n");
>> @@ -140,6 +140,8 @@ static const struct phy_setting settings[] = {
>> /* 10M */
>> PHY_SETTING( 10, FULL, 10baseT_Full ),
>> PHY_SETTING( 10, HALF, 10baseT_Half ),
>> + PHY_SETTING( 10, FULL, 10baseT1L_Full ),
>> + PHY_SETTING( 10, FULL, 10baseT1S_Full ),
>> };
>> #undef PHY_SETTING
>>
>> diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
>> index dd06302aa93e..e429cc8da31a 100644
>> --- a/include/uapi/linux/ethtool.h
>> +++ b/include/uapi/linux/ethtool.h
>> @@ -1485,6 +1485,8 @@ enum ethtool_link_mode_bit_indices {
>> ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66,
>> ETHTOOL_LINK_MODE_100baseT1_Full_BIT = 67,
>> ETHTOOL_LINK_MODE_1000baseT1_Full_BIT = 68,
>> + ETHTOOL_LINK_MODE_10baseT1L_Full_BIT = 69,
>> + ETHTOOL_LINK_MODE_10baseT1S_Full_BIT = 70,
>>
>> /* must be last entry */
>> __ETHTOOL_LINK_MODE_MASK_NBITS
>> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
>> index 0a552061ff1c..6fd5ff632b8e 100644
>> --- a/include/uapi/linux/mdio.h
>> +++ b/include/uapi/linux/mdio.h
>> @@ -43,6 +43,7 @@
>> #define MDIO_PKGID1 14 /* Package identifier */
>> #define MDIO_PKGID2 15
>> #define MDIO_AN_ADVERTISE 16 /* AN advertising (base page) */
>> +#define MDIO_PMA_BASET1_EXTABLE 18 /* BASE-T1 PMA/PMD extended ability */
>> #define MDIO_AN_LPA 19 /* AN LP abilities (base page) */
>> #define MDIO_PCS_EEE_ABLE 20 /* EEE Capability register */
>> #define MDIO_PMA_NG_EXTABLE 21 /* 2.5G/5G PMA/PMD extended ability */
>> @@ -57,11 +58,16 @@
>> #define MDIO_PMA_10GBT_SNR 133 /* 10GBASE-T SNR margin, lane A.
>> * Lanes B-D are numbered 134-136. */
>> #define MDIO_PMA_10GBR_FECABLE 170 /* 10GBASE-R FEC ability */
>> +#define MDIO_PMA_BASET1CTRL 2100 /* BASE-T1 PMA/PMD control */
>> #define MDIO_PCS_10GBX_STAT1 24 /* 10GBASE-X PCS status 1 */
>> #define MDIO_PCS_10GBRT_STAT1 32 /* 10GBASE-R/-T PCS status 1 */
>> #define MDIO_PCS_10GBRT_STAT2 33 /* 10GBASE-R/-T PCS status 2 */
>> #define MDIO_AN_10GBT_CTRL 32 /* 10GBASE-T auto-negotiation control */
>> #define MDIO_AN_10GBT_STAT 33 /* 10GBASE-T auto-negotiation status */
>> +#define MDIO_AN_BT1_CTRL 512 /* BASE-T1 auto-negotiation control */
>> +#define MDIO_AN_BT1_STAT 513 /* BASE-T1 auto-negotiation status */
>> +#define MDIO_AN_10BT1_CTRL 526 /* 10BASE-T1 auto-negotiation control */
>> +#define MDIO_AN_10BT1_STAT 527 /* 10BASE-T1 auto-negotiation status */
>>
>> /* LASI (Link Alarm Status Interrupt) registers, defined by XENPAK MSA. */
>> #define MDIO_PMA_LASI_RXCTRL 0x9000 /* RX_ALARM control */
>> @@ -151,6 +157,7 @@
>> #define MDIO_PMA_CTRL2_100BTX 0x000e /* 100BASE-TX type */
>> #define MDIO_PMA_CTRL2_10BT 0x000f /* 10BASE-T type */
>> #define MDIO_PMA_CTRL2_2_5GBT 0x0030 /* 2.5GBaseT type */
>> +#define MDIO_PMA_CTRL2_BT1 0x003D /* BASE-T1 type */
>> #define MDIO_PMA_CTRL2_5GBT 0x0031 /* 5GBaseT type */
>> #define MDIO_PCS_CTRL2_TYPE 0x0003 /* PCS type selection */
>> #define MDIO_PCS_CTRL2_10GBR 0x0000 /* 10GBASE-R type */
>> @@ -205,8 +212,16 @@
>> #define MDIO_PMA_EXTABLE_1000BKX 0x0040 /* 1000BASE-KX ability */
>> #define MDIO_PMA_EXTABLE_100BTX 0x0080 /* 100BASE-TX ability */
>> #define MDIO_PMA_EXTABLE_10BT 0x0100 /* 10BASE-T ability */
>> +#define MDIO_PMA_EXTABLE_BASET1 0x0800 /* BASE-T1 ability */
>> #define MDIO_PMA_EXTABLE_NBT 0x4000 /* 2.5/5GBASE-T ability */
>>
>> +/* PMA BASE-T1 control register. */
>> +#define MDIO_PMA_BASET1CTRL_TYPE 0x000f /* PMA/PMD BASE-T1 type sel. */
>> +#define MDIO_PMA_BASET1CTRL_TYPE_100BT1 0x0000 /* 100BASE-T1 */
>> +#define MDIO_PMA_BASET1CTRL_TYPE_1000BT1 0x0001 /* 1000BASE-T1 */
>> +#define MDIO_PMA_BASET1CTRL_TYPE_10BT1L 0x0002 /* 10BASE-T1L */
>> +#define MDIO_PMA_BASET1CTRL_TYPE_10BT1S 0x0003 /* 10BASE-T1S */
>> +
>> /* PHY XGXS lane state register. */
>> #define MDIO_PHYXS_LNSTAT_SYNC0 0x0001
>> #define MDIO_PHYXS_LNSTAT_SYNC1 0x0002
>> @@ -281,6 +296,12 @@
>> #define MDIO_PMA_NG_EXTABLE_2_5GBT 0x0001 /* 2.5GBASET ability */
>> #define MDIO_PMA_NG_EXTABLE_5GBT 0x0002 /* 5GBASET ability */
>>
>> +/* BASE-T1 Extended abilities register. */
>> +#define MDIO_PMA_BASET1_EXTABLE_100BT1 0x0001 /* 100BASE-T1 ability */
>> +#define MDIO_PMA_BASET1_EXTABLE_1000BT1 0x0002 /* 1000BASE-T1 ability */
>> +#define MDIO_PMA_BASET1_EXTABLE_10BT1L 0x0004 /* 10BASE-T1L ability */
>> +#define MDIO_PMA_BASET1_EXTABLE_10BT1S 0x0008 /* 10BASE-T1S ability */
>> +
>> /* LASI RX_ALARM control/status registers. */
>> #define MDIO_PMA_LASI_RX_PHYXSLFLT 0x0001 /* PHY XS RX local fault */
>> #define MDIO_PMA_LASI_RX_PCSLFLT 0x0008 /* PCS RX local fault */
>>
>
>
I have already prepared my next patch with a global is_baset1_capable
property in the phy device. It is intentional that I did not introduce
IS_10BASET1 macro. I should have probably explained in the cover letter.
Will do for v2.
100 and 1000BASE-T1 are already more mature than 10BASE-T1. For
10BASE-T1, the only support added right now is the detection of the
capability, i.e. reporting to ethtool. I would suggest enabling more
support for 10BASE-T1 once there is silicon available on a larger scale
and usage is more clear.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-19 6:32 ` Christian Herber
@ 2019-08-19 19:07 ` Heiner Kallweit
2019-08-20 13:36 ` [EXT] " Christian Herber
0 siblings, 1 reply; 20+ messages in thread
From: Heiner Kallweit @ 2019-08-19 19:07 UTC (permalink / raw)
To: Christian Herber, davem; +Cc: netdev, linux-kernel
On 19.08.2019 08:32, Christian Herber wrote:
> On 16.08.2019 22:59, Heiner Kallweit wrote:
>> On 15.08.2019 17:32, Christian Herber wrote:
>>> This patch adds basic support for BASE-T1 PHYs in the framework.
>>> BASE-T1 PHYs main area of application are automotive and industrial.
>>> BASE-T1 is standardized in IEEE 802.3, namely
>>> - IEEE 802.3bw: 100BASE-T1
>>> - IEEE 802.3bp 1000BASE-T1
>>> - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>>>
>>> There are no products which contain BASE-T1 and consumer type PHYs like
>>> 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
>>> PHYs with auto-negotiation.
>>
>> Is this meant in a way that *currently* there are no PHY's combining Base-T1
>> with normal Base-T modes? Or are there reasons why this isn't possible in
>> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
>> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
>>
>>>
>>> The intention of this patch is to make use of the existing Clause 45 functions.
>>> BASE-T1 adds some additional registers e.g. for aneg control, which follow a
>>> similiar register layout as the existing devices. The bits which are used in
>>> BASE-T1 specific registers are the same as in basic registers, thus the
>>> existing functions can be resued, with get_aneg_ctrl() selecting the correct
>>> register address.
>>>
>> If Base-T1 can't be combined with other modes then at a first glance I see no
>> benefit in defining new registers e.g. for aneg control, and the standard ones
>> are unused. Why not using the standard registers? Can you shed some light on that?
>>
>> Are the new registers internally shadowed to the standard location?
>> That's something I've seen on other PHY's: one register appears in different
>> places in different devices.
>>
>>> The current version of ethtool has been prepared for 100/1000BASE-T1 and works
>>> with this patch. 10BASE-T1 needs to be added to ethtool.
>>>
>>> Christian Herber (1):
>>> Added BASE-T1 PHY support to PHY Subsystem
>>>
>>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>>> drivers/net/phy/phy-core.c | 4 +-
>>> include/uapi/linux/ethtool.h | 2 +
>>> include/uapi/linux/mdio.h | 21 +++++++
>>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>>
>>
>> Heiner
>>
>
> Hi Heiner,
>
> I do not think the Aquantia part you are describing is publicly
> documented, so i cannot comment on that part.
Right, datasheet isn't publicly available. All I wanted to say with
mentioning this PHY: It's not a rare exception that a PHY combines
standard BaseT modes with "non-consumer" modes for special purposes.
One practical use case of this proprietary 1000Base-T2 mode is
re-using existing 2-pair cabling in aircrafts.
> There are multiple reasons why e.g. xBASE-T1 plus 1000BASE-T is
> unlikely. First, the is no use-case known to me, where this would be
> required. Second, there is no way that you can do an auto-negotiation
> between the two, as these both have their own auto-neg defined (Clause
> 28/73 vs. Clause 98). Thirdly, if you would ever have a product with
> both, I believe it would just include two full PHYs and a way to select
> which flavor you want. Of course, this is the theory until proven
> otherwise, but to me it is sufficient to use a single driver.
>
I'm with you if you say it's unlikely. However your statement in the
commit message leaves the impression that there can't be such a device.
And that's a difference.
Regarding "including two full PHYs":
This case we have already, there are PHYs combining different IP blocks,
each one supporting a specific mode (e.g. copper and fiber). There you
also have the case of different autoneg methods, clause 28 vs. clause 37.
> The registers are different in the fields they include. It is just that
> the flags which are used by the Linux driver, like restarting auto-neg,
> are at the same position.
>
Good to know. Your commit description doesn't mention any specific PHY.
I suppose you have PHYs you'd like to operate with the genphy_c45 driver.
Could you give an example? And ideally, is a public datasheet available?
> Christian
>
>
Heiner
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-19 19:07 ` Heiner Kallweit
@ 2019-08-20 13:36 ` Christian Herber
2019-08-21 17:09 ` Heiner Kallweit
0 siblings, 1 reply; 20+ messages in thread
From: Christian Herber @ 2019-08-20 13:36 UTC (permalink / raw)
To: Heiner Kallweit, davem; +Cc: netdev, linux-kernel
On 19.08.2019 21:07, Heiner Kallweit wrote:
> Caution: EXT Email
>
> On 19.08.2019 08:32, Christian Herber wrote:
>> On 16.08.2019 22:59, Heiner Kallweit wrote:
>>> On 15.08.2019 17:32, Christian Herber wrote:
>>>> This patch adds basic support for BASE-T1 PHYs in the framework.
>>>> BASE-T1 PHYs main area of application are automotive and industrial.
>>>> BASE-T1 is standardized in IEEE 802.3, namely
>>>> - IEEE 802.3bw: 100BASE-T1
>>>> - IEEE 802.3bp 1000BASE-T1
>>>> - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>>>>
>>>> There are no products which contain BASE-T1 and consumer type PHYs like
>>>> 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
>>>> PHYs with auto-negotiation.
>>>
>>> Is this meant in a way that *currently* there are no PHY's combining Base-T1
>>> with normal Base-T modes? Or are there reasons why this isn't possible in
>>> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
>>> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
>>>
>>>>
>>>> The intention of this patch is to make use of the existing Clause 45 functions.
>>>> BASE-T1 adds some additional registers e.g. for aneg control, which follow a
>>>> similiar register layout as the existing devices. The bits which are used in
>>>> BASE-T1 specific registers are the same as in basic registers, thus the
>>>> existing functions can be resued, with get_aneg_ctrl() selecting the correct
>>>> register address.
>>>>
>>> If Base-T1 can't be combined with other modes then at a first glance I see no
>>> benefit in defining new registers e.g. for aneg control, and the standard ones
>>> are unused. Why not using the standard registers? Can you shed some light on that?
>>>
>>> Are the new registers internally shadowed to the standard location?
>>> That's something I've seen on other PHY's: one register appears in different
>>> places in different devices.
>>>
>>>> The current version of ethtool has been prepared for 100/1000BASE-T1 and works
>>>> with this patch. 10BASE-T1 needs to be added to ethtool.
>>>>
>>>> Christian Herber (1):
>>>> Added BASE-T1 PHY support to PHY Subsystem
>>>>
>>>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>>>> drivers/net/phy/phy-core.c | 4 +-
>>>> include/uapi/linux/ethtool.h | 2 +
>>>> include/uapi/linux/mdio.h | 21 +++++++
>>>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>>>
>>>
>>> Heiner
>>>
>>
>> Hi Heiner,
>>
>> I do not think the Aquantia part you are describing is publicly
>> documented, so i cannot comment on that part.
> Right, datasheet isn't publicly available. All I wanted to say with
> mentioning this PHY: It's not a rare exception that a PHY combines
> standard BaseT modes with "non-consumer" modes for special purposes.
> One practical use case of this proprietary 1000Base-T2 mode is
> re-using existing 2-pair cabling in aircrafts.
>
>> There are multiple reasons why e.g. xBASE-T1 plus 1000BASE-T is
>> unlikely. First, the is no use-case known to me, where this would be
>> required. Second, there is no way that you can do an auto-negotiation
>> between the two, as these both have their own auto-neg defined (Clause
>> 28/73 vs. Clause 98). Thirdly, if you would ever have a product with
>> both, I believe it would just include two full PHYs and a way to select
>> which flavor you want. Of course, this is the theory until proven
>> otherwise, but to me it is sufficient to use a single driver.
>>
> I'm with you if you say it's unlikely. However your statement in the
> commit message leaves the impression that there can't be such a device.
> And that's a difference.
>
> Regarding "including two full PHYs":
> This case we have already, there are PHYs combining different IP blocks,
> each one supporting a specific mode (e.g. copper and fiber). There you
> also have the case of different autoneg methods, clause 28 vs. clause 37.
>
>> The registers are different in the fields they include. It is just that
>> the flags which are used by the Linux driver, like restarting auto-neg,
>> are at the same position.
>>
> Good to know. Your commit description doesn't mention any specific PHY.
> I suppose you have PHYs you'd like to operate with the genphy_c45 driver.
> Could you give an example? And ideally, is a public datasheet available?
>
>> Christian
>>
>>
> Heiner
>
There are no public BASE-T1 devices on the market right now that use
Clause 45 standard registers. The first such products were developed
before the IEEE standard (BroadR-Reach) and used Clause 22 access (see
e.g. the support in the Kernel for TJA110x).
The most convenient way to test with a BASE-T1 device would be to use an
SFP (e.g.
https://technica-engineering.de/produkt/1000base-t1-sfp-module/).
Alternative source could be Goepel.
There are also a number of media-converters around where one could break
out the MDIO and connect to a processor. Of course, in all cases it
should be made sure that this is a Clause-45 device.
As all relevant parts are NDA-restricted, this is pretty much all the
information I can share.
Christian
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-20 13:36 ` [EXT] " Christian Herber
@ 2019-08-21 17:09 ` Heiner Kallweit
2019-08-21 18:57 ` Andrew Lunn
0 siblings, 1 reply; 20+ messages in thread
From: Heiner Kallweit @ 2019-08-21 17:09 UTC (permalink / raw)
To: Christian Herber, davem, Andrew Lunn, Florian Fainelli
Cc: netdev, linux-kernel
On 20.08.2019 15:36, Christian Herber wrote:
> On 19.08.2019 21:07, Heiner Kallweit wrote:
>> Caution: EXT Email
>>
>> On 19.08.2019 08:32, Christian Herber wrote:
>>> On 16.08.2019 22:59, Heiner Kallweit wrote:
>>>> On 15.08.2019 17:32, Christian Herber wrote:
>>>>> This patch adds basic support for BASE-T1 PHYs in the framework.
>>>>> BASE-T1 PHYs main area of application are automotive and industrial.
>>>>> BASE-T1 is standardized in IEEE 802.3, namely
>>>>> - IEEE 802.3bw: 100BASE-T1
>>>>> - IEEE 802.3bp 1000BASE-T1
>>>>> - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>>>>>
>>>>> There are no products which contain BASE-T1 and consumer type PHYs like
>>>>> 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
>>>>> PHYs with auto-negotiation.
>>>>
>>>> Is this meant in a way that *currently* there are no PHY's combining Base-T1
>>>> with normal Base-T modes? Or are there reasons why this isn't possible in
>>>> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
>>>> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
>>>>
>>>>>
>>>>> The intention of this patch is to make use of the existing Clause 45 functions.
>>>>> BASE-T1 adds some additional registers e.g. for aneg control, which follow a
>>>>> similiar register layout as the existing devices. The bits which are used in
>>>>> BASE-T1 specific registers are the same as in basic registers, thus the
>>>>> existing functions can be resued, with get_aneg_ctrl() selecting the correct
>>>>> register address.
>>>>>
>>>> If Base-T1 can't be combined with other modes then at a first glance I see no
>>>> benefit in defining new registers e.g. for aneg control, and the standard ones
>>>> are unused. Why not using the standard registers? Can you shed some light on that?
>>>>
>>>> Are the new registers internally shadowed to the standard location?
>>>> That's something I've seen on other PHY's: one register appears in different
>>>> places in different devices.
>>>>
>>>>> The current version of ethtool has been prepared for 100/1000BASE-T1 and works
>>>>> with this patch. 10BASE-T1 needs to be added to ethtool.
>>>>>
>>>>> Christian Herber (1):
>>>>> Added BASE-T1 PHY support to PHY Subsystem
>>>>>
>>>>> drivers/net/phy/phy-c45.c | 113 +++++++++++++++++++++++++++++++----
>>>>> drivers/net/phy/phy-core.c | 4 +-
>>>>> include/uapi/linux/ethtool.h | 2 +
>>>>> include/uapi/linux/mdio.h | 21 +++++++
>>>>> 4 files changed, 129 insertions(+), 11 deletions(-)
>>>>>
>>>>
>>>> Heiner
>>>>
>>>
>>> Hi Heiner,
>>>
>>> I do not think the Aquantia part you are describing is publicly
>>> documented, so i cannot comment on that part.
>> Right, datasheet isn't publicly available. All I wanted to say with
>> mentioning this PHY: It's not a rare exception that a PHY combines
>> standard BaseT modes with "non-consumer" modes for special purposes.
>> One practical use case of this proprietary 1000Base-T2 mode is
>> re-using existing 2-pair cabling in aircrafts.
>>
>>> There are multiple reasons why e.g. xBASE-T1 plus 1000BASE-T is
>>> unlikely. First, the is no use-case known to me, where this would be
>>> required. Second, there is no way that you can do an auto-negotiation
>>> between the two, as these both have their own auto-neg defined (Clause
>>> 28/73 vs. Clause 98). Thirdly, if you would ever have a product with
>>> both, I believe it would just include two full PHYs and a way to select
>>> which flavor you want. Of course, this is the theory until proven
>>> otherwise, but to me it is sufficient to use a single driver.
>>>
>> I'm with you if you say it's unlikely. However your statement in the
>> commit message leaves the impression that there can't be such a device.
>> And that's a difference.
>>
>> Regarding "including two full PHYs":
>> This case we have already, there are PHYs combining different IP blocks,
>> each one supporting a specific mode (e.g. copper and fiber). There you
>> also have the case of different autoneg methods, clause 28 vs. clause 37.
>>
>>> The registers are different in the fields they include. It is just that
>>> the flags which are used by the Linux driver, like restarting auto-neg,
>>> are at the same position.
>>>
>> Good to know. Your commit description doesn't mention any specific PHY.
>> I suppose you have PHYs you'd like to operate with the genphy_c45 driver.
>> Could you give an example? And ideally, is a public datasheet available?
>>
>>> Christian
>>>
>>>
>> Heiner
>>
>
> There are no public BASE-T1 devices on the market right now that use
> Clause 45 standard registers. The first such products were developed
> before the IEEE standard (BroadR-Reach) and used Clause 22 access (see
> e.g. the support in the Kernel for TJA110x).
>
> The most convenient way to test with a BASE-T1 device would be to use an
> SFP (e.g.
> https://technica-engineering.de/produkt/1000base-t1-sfp-module/).
> Alternative source could be Goepel.
>
> There are also a number of media-converters around where one could break
> out the MDIO and connect to a processor. Of course, in all cases it
> should be made sure that this is a Clause-45 device.
>
> As all relevant parts are NDA-restricted, this is pretty much all the
> information I can share.
>
If no such device is on the market yet, then I'd suggest:
- wait for such a device to see whether genphy_c45 driver is really
sufficient or whether other chip features require a dedicated driver
anyway. In the latter case it may be better to add dedicated T1
functions to phylib.
- add the missing 10BASE-T1L and 10BASE-T1S support meanwhile
The current patch set IMO is a little bit hacky. I'm not 100% happy
with the implicit assumption that there can't be devices supporting
T1 and classic BaseT modes or fiber modes.
Andrew: Do you have an opinion on that?
> Christian
>
>
Heiner
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [EXT] Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-21 17:09 ` Heiner Kallweit
@ 2019-08-21 18:57 ` Andrew Lunn
2019-08-22 7:18 ` Christian Herber
0 siblings, 1 reply; 20+ messages in thread
From: Andrew Lunn @ 2019-08-21 18:57 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Christian Herber, davem, Florian Fainelli, netdev, linux-kernel
> The current patch set IMO is a little bit hacky. I'm not 100% happy
> with the implicit assumption that there can't be devices supporting
> T1 and classic BaseT modes or fiber modes.
>
> Andrew: Do you have an opinion on that?
Hi Heiner
I would also like cleaner integration. I doubt here is anything in the
standard which says you cannot combine these modes. It is more a
marketing question if anybody would build such a device. Maybe not
directly into a vehicle, but you could imaging a mobile test device
which uses T1 to talk to the car and T4 to connect to the garage
network?
So i don't think we should limit ourselves. phylib should provide a
clean, simple set of helpers to perform standard operations for
various modes. Drivers can make use of those helpers. That much should
be clear. If we try to make genphy support them all simultaneously, is
less clear.
Andrew
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-21 18:57 ` Andrew Lunn
@ 2019-08-22 7:18 ` Christian Herber
2019-08-24 15:03 ` Heiner Kallweit
0 siblings, 1 reply; 20+ messages in thread
From: Christian Herber @ 2019-08-22 7:18 UTC (permalink / raw)
To: Andrew Lunn, Heiner Kallweit
Cc: davem, Florian Fainelli, netdev, linux-kernel
On 21.08.2019 20:57, Andrew Lunn wrote:
>
>> The current patch set IMO is a little bit hacky. I'm not 100% happy
>> with the implicit assumption that there can't be devices supporting
>> T1 and classic BaseT modes or fiber modes.
>
>> Andrew: Do you have an opinion on that?
>
> Hi Heiner
>
> I would also like cleaner integration. I doubt here is anything in the
> standard which says you cannot combine these modes. It is more a
> marketing question if anybody would build such a device. Maybe not
> directly into a vehicle, but you could imaging a mobile test device
> which uses T1 to talk to the car and T4 to connect to the garage
> network?
>
> So i don't think we should limit ourselves. phylib should provide a
> clean, simple set of helpers to perform standard operations for
> various modes. Drivers can make use of those helpers. That much should
> be clear. If we try to make genphy support them all simultaneously, is
> less clear.
>
> Andrew
>
If you want to go down this path, then i think we have to ask some more
questions. Clause 45 is a very scalable register scheme, it is not a
specific class of devices and will be extended and extended.
Currently, the phy-c45.c supports 10/100/1000/2500/5000/10000 Mbps
consumer/enterprise PHYs. This is also an implicit assumption. The
register set (e.g. on auto-neg) used for this will also only support
these modes and nothing more, as it is done scaling.
Currently not supported, but already present in IEEE 802.3:
- MultiGBASE-T (25/40 Gbps) (see e.g. MultiGBASE-T AN control 1 register)
- BASE-T1
- 10BASE-T1
- NGBASE-T1
And surely there are some on the way or already there that I am not
aware of.
To me, one architectural decision point is if you want to have generic
support for all C45 PHYs in one file, or if you want to split it by
device class. I went down the first path with my patch, as this is the
road gone also with the existing code.
If you want to split BASE-T1, i think you will need one basic C45
library (genphy_c45_pma_read_abilities() is a good example of a function
that is not specific to a device class). On the other hand,
genphy_c45_pma_setup_forced() is not a generic function at this point as
it supports only a subset of devices managed in C45.
I tend to agree with you that splitting is the best way to go in the
long run, but that also requires a split of the existing phy-c45.c into
two IMHO.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-22 7:18 ` Christian Herber
@ 2019-08-24 15:03 ` Heiner Kallweit
2019-08-26 7:57 ` Christian Herber
0 siblings, 1 reply; 20+ messages in thread
From: Heiner Kallweit @ 2019-08-24 15:03 UTC (permalink / raw)
To: Christian Herber, Andrew Lunn
Cc: davem, Florian Fainelli, netdev, linux-kernel
On 22.08.2019 09:18, Christian Herber wrote:
> On 21.08.2019 20:57, Andrew Lunn wrote:
>>
>>> The current patch set IMO is a little bit hacky. I'm not 100% happy
>>> with the implicit assumption that there can't be devices supporting
>>> T1 and classic BaseT modes or fiber modes.
>>
>>> Andrew: Do you have an opinion on that?
>>
>> Hi Heiner
>>
>> I would also like cleaner integration. I doubt here is anything in the
>> standard which says you cannot combine these modes. It is more a
>> marketing question if anybody would build such a device. Maybe not
>> directly into a vehicle, but you could imaging a mobile test device
>> which uses T1 to talk to the car and T4 to connect to the garage
>> network?
>>
>> So i don't think we should limit ourselves. phylib should provide a
>> clean, simple set of helpers to perform standard operations for
>> various modes. Drivers can make use of those helpers. That much should
>> be clear. If we try to make genphy support them all simultaneously, is
>> less clear.
>>
>> Andrew
>>
>
> If you want to go down this path, then i think we have to ask some more
> questions. Clause 45 is a very scalable register scheme, it is not a
> specific class of devices and will be extended and extended.
>
> Currently, the phy-c45.c supports 10/100/1000/2500/5000/10000 Mbps
> consumer/enterprise PHYs. This is also an implicit assumption. The
> register set (e.g. on auto-neg) used for this will also only support
> these modes and nothing more, as it is done scaling.
>
> Currently not supported, but already present in IEEE 802.3:
> - MultiGBASE-T (25/40 Gbps) (see e.g. MultiGBASE-T AN control 1 register)
> - BASE-T1
> - 10BASE-T1
> - NGBASE-T1
>
> And surely there are some on the way or already there that I am not
> aware of.
>
> To me, one architectural decision point is if you want to have generic
> support for all C45 PHYs in one file, or if you want to split it by
> device class. I went down the first path with my patch, as this is the
> road gone also with the existing code.
>
> If you want to split BASE-T1, i think you will need one basic C45
> library (genphy_c45_pma_read_abilities() is a good example of a function
> that is not specific to a device class). On the other hand,
> genphy_c45_pma_setup_forced() is not a generic function at this point as
> it supports only a subset of devices managed in C45.
>
> I tend to agree with you that splitting is the best way to go in the
> long run, but that also requires a split of the existing phy-c45.c into
> two IMHO.
>
BASE-T1 seems to be based on Clause 45 (at least Clause 45 MDIO),
but it's not fully compliant with Clause 45. Taking AN link status
as an example: 45.2.7.2.7 states that link-up is signaled in bit 7.1.2.
If BASE-T1 uses a different register, then it's not fully Clause 45
compatible.
Therefore also my question for the datasheet of an actual BASE-T1 PHY,
as I would be curious whether it shadows the link-up bit from 7.513.2
to 7.1.2 to be Clause 45 compliant. Definitely reading bit 7.513.2
is nothing that belongs into a genphy_c45_ function.
The extension to genphy_c45_pma_read_abilities() looks good to me,
for the other parts I'd like to see first how real world BASE-T1 PHYs
handle it. If they shadow the T1-specific bits to the Clause 45
standard ones, we should be fine. Otherwise IMO we have to add
separate T1 functions to phylib.
Heiner
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-24 15:03 ` Heiner Kallweit
@ 2019-08-26 7:57 ` Christian Herber
0 siblings, 0 replies; 20+ messages in thread
From: Christian Herber @ 2019-08-26 7:57 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn
Cc: davem, Florian Fainelli, netdev, linux-kernel
On 24.08.2019 17:03, Heiner Kallweit wrote:
>
> On 22.08.2019 09:18, Christian Herber wrote:
>> On 21.08.2019 20:57, Andrew Lunn wrote:
>>>
>>>> The current patch set IMO is a little bit hacky. I'm not 100% happy
>>>> with the implicit assumption that there can't be devices supporting
>>>> T1 and classic BaseT modes or fiber modes.
>>>
>>>> Andrew: Do you have an opinion on that?
>>>
>>> Hi Heiner
>>>
>>> I would also like cleaner integration. I doubt here is anything in the
>>> standard which says you cannot combine these modes. It is more a
>>> marketing question if anybody would build such a device. Maybe not
>>> directly into a vehicle, but you could imaging a mobile test device
>>> which uses T1 to talk to the car and T4 to connect to the garage
>>> network?
>>>
>>> So i don't think we should limit ourselves. phylib should provide a
>>> clean, simple set of helpers to perform standard operations for
>>> various modes. Drivers can make use of those helpers. That much should
>>> be clear. If we try to make genphy support them all simultaneously, is
>>> less clear.
>>>
>>> Andrew
>>>
>>
>> If you want to go down this path, then i think we have to ask some more
>> questions. Clause 45 is a very scalable register scheme, it is not a
>> specific class of devices and will be extended and extended.
>>
>> Currently, the phy-c45.c supports 10/100/1000/2500/5000/10000 Mbps
>> consumer/enterprise PHYs. This is also an implicit assumption. The
>> register set (e.g. on auto-neg) used for this will also only support
>> these modes and nothing more, as it is done scaling.
>>
>> Currently not supported, but already present in IEEE 802.3:
>> - MultiGBASE-T (25/40 Gbps) (see e.g. MultiGBASE-T AN control 1 register)
>> - BASE-T1
>> - 10BASE-T1
>> - NGBASE-T1
>>
>> And surely there are some on the way or already there that I am not
>> aware of.
>>
>> To me, one architectural decision point is if you want to have generic
>> support for all C45 PHYs in one file, or if you want to split it by
>> device class. I went down the first path with my patch, as this is the
>> road gone also with the existing code.
>>
>> If you want to split BASE-T1, i think you will need one basic C45
>> library (genphy_c45_pma_read_abilities() is a good example of a function
>> that is not specific to a device class). On the other hand,
>> genphy_c45_pma_setup_forced() is not a generic function at this point as
>> it supports only a subset of devices managed in C45.
>>
>> I tend to agree with you that splitting is the best way to go in the
>> long run, but that also requires a split of the existing phy-c45.c into
>> two IMHO.
>>
> BASE-T1 seems to be based on Clause 45 (at least Clause 45 MDIO),
> but it's not fully compliant with Clause 45. Taking AN link status
> as an example: 45.2.7.2.7 states that link-up is signaled in bit 7.1.2.
> If BASE-T1 uses a different register, then it's not fully Clause 45
> compatible.
Clause 45 defines e.g. bit 7.1.2 just like it defines the BASE-T1
auto-neg registers. Any bit that i have used in my patch is 100%
standardized in IEEE 802.3-2018, Clause 45. By definition, BASE-T1 PHYs
have to use the Clause 45 BASE-T1 registers, otherwise they are not IEEE
compliant.
> Therefore also my question for the datasheet of an actual BASE-T1 PHY,
> as I would be curious whether it shadows the link-up bit from 7.513.2
> to 7.1.2 to be Clause 45 compliant. Definitely reading bit 7.513.2
> is nothing that belongs into a genphy_c45_ function.
For now, there is no such public data sheet. However, IEEE 802.3-2018 is
public. This should be the basis for a generic driver. Datasheets are
needed for the device specific drivers. If Linux cares to support
BASE-T1, it should implement a driver that works with a standard
compliant PHY and that can be done on the basis of IEEE.
> The extension to genphy_c45_pma_read_abilities() looks good to me,
> for the other parts I'd like to see first how real world BASE-T1 PHYs
> handle it. If they shadow the T1-specific bits to the Clause 45
> standard ones, we should be fine. Otherwise IMO we have to add
> separate T1 functions to phylib.
>
> Heiner
>
There is not requirement in IEEE to shadow the BASE-T1 registers into
the "standard" ones. Thus, such an assumption should not be done for a
generic driver, as it will quite certainly not work with all devices.
Fyi, both registers are standard, just that the historically first ones
are for classic 10/100/1000/10000 PHYs and BASE-T1 registers are for the
single twisted pair PHYs.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
2019-08-16 20:59 ` Heiner Kallweit
2019-08-19 6:32 ` Christian Herber
@ 2019-10-16 8:37 ` Lucas Stach
1 sibling, 0 replies; 20+ messages in thread
From: Lucas Stach @ 2019-10-16 8:37 UTC (permalink / raw)
To: Heiner Kallweit, Christian Herber, davem; +Cc: netdev, linux-kernel
On Fr, 2019-08-16 at 22:59 +0200, Heiner Kallweit wrote:
> On 15.08.2019 17:32, Christian Herber wrote:
> > This patch adds basic support for BASE-T1 PHYs in the framework.
> > BASE-T1 PHYs main area of application are automotive and industrial.
> > BASE-T1 is standardized in IEEE 802.3, namely
> > - IEEE 802.3bw: 100BASE-T1
> > - IEEE 802.3bp 1000BASE-T1
> > - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
> >
> > There are no products which contain BASE-T1 and consumer type PHYs like
> > 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
> > PHYs with auto-negotiation.
>
> Is this meant in a way that *currently* there are no PHY's combining Base-T1
> with normal Base-T modes? Or are there reasons why this isn't possible in
> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
There are PHYs combining both Base-T1 and other Base-T capabilities.
E.g. the Broadcom BCM54811 support both Base-T1, as well as 1000BASE-T
and 100BASE-TX.
Regards,
Lucas
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: Re: [PATCH net-next 0/1] Add BASE-T1 PHY support
@ 2019-10-16 13:19 Christian Herber
0 siblings, 0 replies; 20+ messages in thread
From: Christian Herber @ 2019-10-16 13:19 UTC (permalink / raw)
To: Lucas Stach, Heiner Kallweit, davem; +Cc: netdev, linux-kernel
On October 16, 2019 10:37:30 AM Lucas Stach <l.stach@pengutronix.de> wrote:
> On Fr, 2019-08-16 at 22:59 +0200, Heiner Kallweit wrote:
>> On 15.08.2019 17:32, Christian Herber wrote:
>> > This patch adds basic support for BASE-T1 PHYs in the framework.
>> > BASE-T1 PHYs main area of application are automotive and industrial.
>> > BASE-T1 is standardized in IEEE 802.3, namely
>> > - IEEE 802.3bw: 100BASE-T1
>> > - IEEE 802.3bp 1000BASE-T1
>> > - IEEE 802.3cg: 10BASE-T1L and 10BASE-T1S
>> >
>> > There are no products which contain BASE-T1 and consumer type PHYs like
>> > 1000BASE-T. However, devices exist which combine 100BASE-T1 and 1000BASE-T1
>> > PHYs with auto-negotiation.
>>
>> Is this meant in a way that *currently* there are no PHY's combining Base-T1
>> with normal Base-T modes? Or are there reasons why this isn't possible in
>> general? I'm asking because we have PHY's combining copper and fiber, and e.g.
>> the mentioned Aquantia PHY that combines NBase-T with 1000Base-T2.
>
> There are PHYs combining both Base-T1 and other Base-T capabilities.
> E.g. the Broadcom BCM54811 support both Base-T1, as well as 1000BASE-T
> and 100BASE-TX.
>
> Regards,
> Lucas
Interesting. To be precise, the device supports BroadR-Reach and not 100BASE-T1 according to their product brief. Therefore I would assume that the registers also do not follow the IEEE defined Clause 45 layout. While we cannot learn much then how to handle such devices in a generic clause 45 driver, this suggests that such devices can appear.
Still, IEEE 802.3 does not really define a way that these devices would coexist, so I would assume it is pretty much two PHYs in one IC and you can chose which flavor you want. Is my assumption on that correct?
In that case, you would have to structure the drivers in a way the you have separate functions for the standalone flavors of Clause 45 managed PHYs, while making as much reuse as possible between them.
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-10-16 13:19 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-15 15:32 [PATCH net-next 0/1] Add BASE-T1 PHY support Christian Herber
2019-08-15 15:32 ` [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem Christian Herber
2019-08-15 15:56 ` Andrew Lunn
2019-08-15 16:34 ` Heiner Kallweit
2019-08-16 12:05 ` [EXT] " Christian Herber
2019-08-16 11:56 ` Christian Herber
2019-08-16 21:13 ` Heiner Kallweit
2019-08-19 6:40 ` Christian Herber
2019-08-15 15:43 ` [PATCH net-next 0/1] Add BASE-T1 PHY support Andrew Lunn
2019-08-16 20:59 ` Heiner Kallweit
2019-08-19 6:32 ` Christian Herber
2019-08-19 19:07 ` Heiner Kallweit
2019-08-20 13:36 ` [EXT] " Christian Herber
2019-08-21 17:09 ` Heiner Kallweit
2019-08-21 18:57 ` Andrew Lunn
2019-08-22 7:18 ` Christian Herber
2019-08-24 15:03 ` Heiner Kallweit
2019-08-26 7:57 ` Christian Herber
2019-10-16 8:37 ` Lucas Stach
2019-10-16 13:19 Christian Herber
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).