From: Christian Herber <christian.herber@nxp.com>
To: Heiner Kallweit <hkallweit1@gmail.com>,
"davem@davemloft.net" <davem@davemloft.net>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: Re: [PATCH net-next 1/1] Added BASE-T1 PHY support to PHY Subsystem
Date: Mon, 19 Aug 2019 06:40:45 +0000 [thread overview]
Message-ID: <AM6PR0402MB3798B7FC9B1800C0E28211FB86A80@AM6PR0402MB3798.eurprd04.prod.outlook.com> (raw)
In-Reply-To: 95eda63a-8202-3f49-c86a-e418162b2811@gmail.com
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.
next prev parent reply other threads:[~2019-08-19 6:40 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AM6PR0402MB3798B7FC9B1800C0E28211FB86A80@AM6PR0402MB3798.eurprd04.prod.outlook.com \
--to=christian.herber@nxp.com \
--cc=davem@davemloft.net \
--cc=hkallweit1@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).