* [PATCH] phy: nxp-c45: add driver for tja1103
@ 2021-04-09 18:41 Radu Pirea (NXP OSS)
2021-04-09 19:18 ` Heiner Kallweit
` (5 more replies)
0 siblings, 6 replies; 21+ messages in thread
From: Radu Pirea (NXP OSS) @ 2021-04-09 18:41 UTC (permalink / raw)
To: andrew, hkallweit1, linux, davem, kuba
Cc: netdev, linux-kernel, Radu Pirea (NXP OSS)
Add driver for tja1103 driver and for future NXP C45 PHYs.
Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
---
MAINTAINERS | 6 +
drivers/net/phy/Kconfig | 6 +
drivers/net/phy/Makefile | 1 +
drivers/net/phy/nxp-c45.c | 622 ++++++++++++++++++++++++++++++++++++++
4 files changed, 635 insertions(+)
create mode 100644 drivers/net/phy/nxp-c45.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a008b70f3c16..082a5eca8913 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12518,6 +12518,12 @@ F: drivers/nvmem/
F: include/linux/nvmem-consumer.h
F: include/linux/nvmem-provider.h
+NXP C45 PHY DRIVER
+M: Radu Pirea <radu-nicolae.pirea@oss.nxp.com>
+L: netdev@vger.kernel.org
+S: Maintained
+F: drivers/net/phy/nxp-c45.c
+
NXP FSPI DRIVER
M: Ashish Kumar <ashish.kumar@nxp.com>
R: Yogesh Gaur <yogeshgaur.83@gmail.com>
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 698bea312adc..fd2da80b5339 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -228,6 +228,12 @@ config NATIONAL_PHY
help
Currently supports the DP83865 PHY.
+config NXP_C45_PHY
+ tristate "NXP C45 PHYs"
+ help
+ Enable support for NXP C45 PHYs.
+ Currently supports only the TJA1103 PHY.
+
config NXP_TJA11XX_PHY
tristate "NXP TJA11xx PHYs support"
depends on HWMON
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf..a18f095748b5 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -70,6 +70,7 @@ obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o
obj-$(CONFIG_MICROSEMI_PHY) += mscc/
obj-$(CONFIG_NATIONAL_PHY) += national.o
+obj-$(CONFIG_NXP_C45_PHY) += nxp-c45.o
obj-$(CONFIG_NXP_TJA11XX_PHY) += nxp-tja11xx.o
obj-$(CONFIG_QSEMI_PHY) += qsemi.o
obj-$(CONFIG_REALTEK_PHY) += realtek.o
diff --git a/drivers/net/phy/nxp-c45.c b/drivers/net/phy/nxp-c45.c
new file mode 100644
index 000000000000..2961799f7d05
--- /dev/null
+++ b/drivers/net/phy/nxp-c45.c
@@ -0,0 +1,622 @@
+// SPDX-License-Identifier: GPL-2.0
+/* NXP C45 PHY driver
+ * Copyright (C) 2021 NXP
+ * Copyright (C) 2021 Radu Pirea <radu-nicolae.pirea@oss.nxp.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/ethtool.h>
+#include <linux/ethtool_netlink.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+#include <linux/processor.h>
+#include <linux/property.h>
+
+#define PHY_ID_BASE_T1 0x001BB010
+
+#define B100T1_PMAPMD_CTL 0x0834
+#define B100T1_PMAPMD_CONFIG_EN BIT(15)
+#define B100T1_PMAPMD_MASTER BIT(14)
+#define MASTER_MODE (B100T1_PMAPMD_CONFIG_EN | B100T1_PMAPMD_MASTER)
+#define SLAVE_MODE (B100T1_PMAPMD_CONFIG_EN)
+
+#define DEVICE_CONTROL 0x0040
+#define DEVICE_CONTROL_RESET BIT(15)
+#define DEVICE_CONTROL_CONFIG_GLOBAL_EN BIT(14)
+#define DEVICE_CONTROL_CONFIG_ALL_EN BIT(13)
+#define RESET_POLL_NS (250 * NSEC_PER_MSEC)
+
+#define PHY_CONTROL 0x8100
+#define PHY_CONFIG_EN BIT(14)
+#define PHY_START_OP BIT(0)
+
+#define PHY_CONFIG 0x8108
+#define PHY_CONFIG_AUTO BIT(0)
+
+#define SIGNAL_QUALITY 0x8320
+#define SQI_VALID BIT(14)
+#define SQI_MASK GENMASK(2, 0)
+#define MAX_SQI SQI_MASK
+
+#define CABLE_TEST 0x8330
+#define CABLE_TEST_ENABLE BIT(15)
+#define CABLE_TEST_START BIT(14)
+#define CABLE_TEST_VALID BIT(13)
+#define CABLE_TEST_OK 0x00
+#define CABLE_TEST_SHORTED 0x01
+#define CABLE_TEST_OPEN 0x02
+#define CABLE_TEST_UNKNOWN 0x07
+
+#define PORT_CONTROL 0x8040
+#define PORT_CONTROL_EN BIT(14)
+
+#define PORT_INFRA_CONTROL 0xAC00
+#define PORT_INFRA_CONTROL_EN BIT(14)
+
+#define VND1_RXID 0xAFCC
+#define VND1_TXID 0xAFCD
+#define ID_ENABLE BIT(15)
+
+#define ABILITIES 0xAFC4
+#define RGMII_ID_ABILITY BIT(15)
+#define RGMII_ABILITY BIT(14)
+#define RMII_ABILITY BIT(10)
+#define REVMII_ABILITY BIT(9)
+#define MII_ABILITY BIT(8)
+#define SGMII_ABILITY BIT(0)
+
+#define MII_BASIC_CONFIG 0xAFC6
+#define MII_BASIC_CONFIG_REV BIT(8)
+#define MII_BASIC_CONFIG_SGMII 0x9
+#define MII_BASIC_CONFIG_RGMII 0x7
+#define MII_BASIC_CONFIG_RMII 0x5
+#define MII_BASIC_CONFIG_MII 0x4
+
+#define SYMBOL_ERROR_COUNTER 0x8350
+#define LINK_DROP_COUNTER 0x8352
+#define LINK_LOSSES_AND_FAILURES 0x8353
+#define R_GOOD_FRAME_CNT 0xA950
+#define R_BAD_FRAME_CNT 0xA952
+#define R_RXER_FRAME_CNT 0xA954
+#define RX_PREAMBLE_COUNT 0xAFCE
+#define TX_PREAMBLE_COUNT 0xAFCF
+#define RX_IPG_LENGTH 0xAFD0
+#define TX_IPG_LENGTH 0xAFD1
+#define COUNTERS_EN BIT(15)
+
+#define CLK_25MHZ_PS_PERIOD 40000UL
+#define PS_PER_DEGREE (CLK_25MHZ_PS_PERIOD / 360)
+#define MIN_ID_PS 8222U
+#define MAX_ID_PS 11300U
+
+struct nxp_c45_phy {
+ u32 tx_delay;
+ u32 rx_delay;
+};
+
+struct nxp_c45_phy_stats {
+ const char *name;
+ u8 mmd;
+ u16 reg;
+ u8 off;
+ u16 mask;
+};
+
+static const struct nxp_c45_phy_stats nxp_c45_hw_stats[] = {
+ { "phy_symbol_error_cnt", MDIO_MMD_VEND1, SYMBOL_ERROR_COUNTER, 0, GENMASK(15, 0) },
+ { "phy_link_status_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 8, GENMASK(13, 8) },
+ { "phy_link_availability_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 0, GENMASK(5, 0) },
+ { "phy_link_loss_cnt", MDIO_MMD_VEND1, LINK_LOSSES_AND_FAILURES, 10, GENMASK(15, 10) },
+ { "phy_link_failure_cnt", MDIO_MMD_VEND1, LINK_LOSSES_AND_FAILURES, 0, GENMASK(9, 0) },
+ { "r_good_frame_cnt", MDIO_MMD_VEND1, R_GOOD_FRAME_CNT, 0, GENMASK(15, 0) },
+ { "r_bad_frame_cnt", MDIO_MMD_VEND1, R_BAD_FRAME_CNT, 0, GENMASK(15, 0) },
+ { "r_rxer_frame_cnt", MDIO_MMD_VEND1, R_RXER_FRAME_CNT, 0, GENMASK(15, 0) },
+ { "rx_preamble_count", MDIO_MMD_VEND1, RX_PREAMBLE_COUNT, 0, GENMASK(5, 0) },
+ { "tx_preamble_count", MDIO_MMD_VEND1, TX_PREAMBLE_COUNT, 0, GENMASK(5, 0) },
+ { "rx_ipg_length", MDIO_MMD_VEND1, RX_IPG_LENGTH, 0, GENMASK(8, 0) },
+ { "tx_ipg_length", MDIO_MMD_VEND1, TX_IPG_LENGTH, 0, GENMASK(8, 0) },
+};
+
+static int nxp_c45_get_sset_count(struct phy_device *phydev)
+{
+ return ARRAY_SIZE(nxp_c45_hw_stats);
+}
+
+static void nxp_c45_get_strings(struct phy_device *phydev, u8 *data)
+{
+ size_t i;
+
+ for (i = 0; i < ARRAY_SIZE(nxp_c45_hw_stats); i++) {
+ strncpy(data + i * ETH_GSTRING_LEN,
+ nxp_c45_hw_stats[i].name, ETH_GSTRING_LEN);
+ }
+}
+
+static void nxp_c45_get_stats(struct phy_device *phydev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ size_t i;
+ int ret;
+
+ for (i = 0; i < ARRAY_SIZE(nxp_c45_hw_stats); i++) {
+ ret = phy_read_mmd(phydev, nxp_c45_hw_stats[i].mmd, nxp_c45_hw_stats[i].reg);
+ if (ret < 0) {
+ data[i] = U64_MAX;
+ } else {
+ data[i] = ret & nxp_c45_hw_stats[i].mask;
+ data[i] >>= nxp_c45_hw_stats[i].off;
+ }
+ }
+}
+
+static int nxp_c45_config_enable(struct phy_device *phydev)
+{
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL, DEVICE_CONTROL_CONFIG_GLOBAL_EN |
+ DEVICE_CONTROL_CONFIG_ALL_EN);
+ usleep_range(400, 450);
+
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, PORT_CONTROL, PORT_CONTROL_EN);
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL, PHY_CONFIG_EN);
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, PORT_INFRA_CONTROL, PORT_INFRA_CONTROL_EN);
+
+ return 0;
+}
+
+static int nxp_c45_start_op(struct phy_device *phydev)
+{
+ int reg;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL);
+ reg |= PHY_START_OP;
+
+ return phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL, reg);
+}
+
+static bool nxp_c45_can_sleep(struct phy_device *phydev)
+{
+ int reg;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);
+ if (reg < 0)
+ return false;
+
+ return !!(reg & MDIO_STAT1_LPOWERABLE);
+}
+
+static int nxp_c45_resume(struct phy_device *phydev)
+{
+ int reg;
+
+ if (!nxp_c45_can_sleep(phydev))
+ return -EOPNOTSUPP;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
+ reg &= ~MDIO_CTRL1_LPOWER;
+ phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
+
+ return 0;
+}
+
+static int nxp_c45_suspend(struct phy_device *phydev)
+{
+ int reg;
+
+ if (!nxp_c45_can_sleep(phydev))
+ return -EOPNOTSUPP;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
+ reg |= MDIO_CTRL1_LPOWER;
+ phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
+
+ return 0;
+}
+
+static int nxp_c45_reset_done(struct phy_device *phydev)
+{
+ return !(phy_read_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL) & DEVICE_CONTROL_RESET);
+}
+
+static int nxp_c45_reset_done_or_timeout(struct phy_device *phydev,
+ ktime_t timeout)
+{
+ ktime_t cur = ktime_get();
+
+ return nxp_c45_reset_done(phydev) || ktime_after(cur, timeout);
+}
+
+static int nxp_c45_soft_reset(struct phy_device *phydev)
+{
+ ktime_t timeout;
+ int ret;
+
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL, DEVICE_CONTROL_RESET);
+ if (ret)
+ return ret;
+
+ timeout = ktime_add_ns(ktime_get(), RESET_POLL_NS);
+ spin_until_cond(nxp_c45_reset_done_or_timeout(phydev, timeout));
+ if (!nxp_c45_reset_done(phydev)) {
+ phydev_err(phydev, "reset fail\n");
+ return -EIO;
+ }
+ return 0;
+}
+
+static int nxp_c45_cable_test_start(struct phy_device *phydev)
+{
+ return phy_write_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST,
+ CABLE_TEST_ENABLE | CABLE_TEST_START);
+}
+
+static int nxp_c45_cable_test_get_status(struct phy_device *phydev,
+ bool *finished)
+{
+ int ret;
+ u8 cable_test_result;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST);
+ if (!(ret & CABLE_TEST_VALID)) {
+ *finished = false;
+ return 0;
+ }
+
+ *finished = true;
+ cable_test_result = ret & GENMASK(2, 0);
+
+ switch (cable_test_result) {
+ case CABLE_TEST_OK:
+ ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+ ETHTOOL_A_CABLE_RESULT_CODE_OK);
+ break;
+ case CABLE_TEST_SHORTED:
+ ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+ ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT);
+ break;
+ case CABLE_TEST_OPEN:
+ ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+ ETHTOOL_A_CABLE_RESULT_CODE_OPEN);
+ break;
+ default:
+ ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
+ ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC);
+ }
+
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST, 0);
+
+ return nxp_c45_start_op(phydev);
+}
+
+static int nxp_c45_setup_master_slave(struct phy_device *phydev)
+{
+ switch (phydev->master_slave_set) {
+ case MASTER_SLAVE_CFG_MASTER_FORCE:
+ case MASTER_SLAVE_CFG_MASTER_PREFERRED:
+ phy_write_mmd(phydev, MDIO_MMD_PMAPMD, B100T1_PMAPMD_CTL, MASTER_MODE);
+ break;
+ case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
+ case MASTER_SLAVE_CFG_SLAVE_FORCE:
+ phy_write_mmd(phydev, MDIO_MMD_PMAPMD, B100T1_PMAPMD_CTL, SLAVE_MODE);
+ break;
+ case MASTER_SLAVE_CFG_UNKNOWN:
+ case MASTER_SLAVE_CFG_UNSUPPORTED:
+ return 0;
+ default:
+ phydev_warn(phydev, "Unsupported Master/Slave mode\n");
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static int nxp_c45_read_master_slave(struct phy_device *phydev)
+{
+ int reg;
+
+ phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
+ phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, B100T1_PMAPMD_CTL);
+ if (reg < 0)
+ return reg;
+
+ if (reg & B100T1_PMAPMD_MASTER) {
+ phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
+ phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
+ } else {
+ phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
+ phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
+ }
+
+ return 0;
+}
+
+static int nxp_c45_config_aneg(struct phy_device *phydev)
+{
+ return nxp_c45_setup_master_slave(phydev);
+}
+
+static int nxp_c45_read_status(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = genphy_c45_read_status(phydev);
+ if (ret)
+ return ret;
+
+ ret = nxp_c45_read_master_slave(phydev);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int nxp_c45_set_loopback(struct phy_device *phydev, bool enable)
+{
+ int reg;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
+ if (reg < 0)
+ return reg;
+
+ if (enable)
+ reg |= MDIO_PCS_CTRL1_LOOPBACK;
+ else
+ reg &= ~MDIO_PCS_CTRL1_LOOPBACK;
+
+ phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, reg);
+
+ phydev->loopback_enabled = enable;
+
+ phydev_dbg(phydev, "Loopback %s\n", enable ? "enabled" : "disabled");
+
+ return 0;
+}
+
+static int nxp_c45_get_sqi(struct phy_device *phydev)
+{
+ int reg;
+
+ reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, SIGNAL_QUALITY);
+ if (!(reg & SQI_VALID))
+ return -EINVAL;
+
+ reg &= SQI_MASK;
+
+ return reg;
+}
+
+static int nxp_c45_get_sqi_max(struct phy_device *phydev)
+{
+ return MAX_SQI;
+}
+
+static int nxp_c45_check_delay(struct phy_device *phydev, u32 delay)
+{
+ if (delay < MIN_ID_PS) {
+ phydev_err(phydev, "delay value smaller than %u\n", MIN_ID_PS);
+ return -EINVAL;
+ }
+
+ if (delay > MAX_ID_PS) {
+ phydev_err(phydev, "delay value higher than %u\n", MAX_ID_PS);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static u64 nxp_c45_get_phase_shift(u64 phase_offset_raw)
+{
+ /* The delay in degree phase is 73.8 + phase_offset_raw * 0.9.
+ * To avoid floating point operations we'll multiply by 10
+ * and get 1 decimal point precision.
+ */
+ phase_offset_raw *= 10;
+ return (phase_offset_raw - 738) / 9;
+}
+
+static void nxp_c45_set_delays(struct phy_device *phydev)
+{
+ struct nxp_c45_phy *priv = phydev->priv;
+ u64 tx_delay = priv->tx_delay;
+ u64 rx_delay = priv->rx_delay;
+ u64 degree;
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+ degree = tx_delay / PS_PER_DEGREE;
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_TXID,
+ ID_ENABLE | nxp_c45_get_phase_shift(degree));
+ }
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+ degree = rx_delay / PS_PER_DEGREE;
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_RXID,
+ ID_ENABLE | nxp_c45_get_phase_shift(degree));
+ }
+}
+
+static int nxp_c45_get_delays(struct phy_device *phydev)
+{
+ struct nxp_c45_phy *priv = phydev->priv;
+ int ret;
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+ ret = device_property_read_u32(&phydev->mdio.dev, "tx-internal-delay-ps",
+ &priv->tx_delay);
+ if (ret) {
+ phydev_err(phydev, "tx-internal-delay-ps property missing\n");
+ return ret;
+ }
+ ret = nxp_c45_check_delay(phydev, priv->tx_delay);
+ if (ret) {
+ phydev_err(phydev, "tx-internal-delay-ps invalid value\n");
+ return ret;
+ }
+ }
+
+ if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+ phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+ ret = device_property_read_u32(&phydev->mdio.dev, "rx-internal-delay-ps",
+ &priv->rx_delay);
+ if (ret) {
+ phydev_err(phydev, "rx-internal-delay-ps property missing\n");
+ return ret;
+ }
+ ret = nxp_c45_check_delay(phydev, priv->rx_delay);
+ if (ret) {
+ phydev_err(phydev, "rx-internal-delay-ps invalid value\n");
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int nxp_c45_set_phy_mode(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ABILITIES);
+ phydev_dbg(phydev, "Clause 45 managed PHY abilities 0x%x\n", ret);
+
+ switch (phydev->interface) {
+ case PHY_INTERFACE_MODE_RGMII:
+ if (!(ret & RGMII_ABILITY)) {
+ phydev_err(phydev, "rgmii mode not supported\n");
+ return -EINVAL;
+ }
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
+ break;
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ if (!(ret & RGMII_ID_ABILITY)) {
+ phydev_err(phydev, "rgmii-id, rgmii-txid, rgmii-rxid modes are not supported\n");
+ return -EINVAL;
+ }
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
+ ret = nxp_c45_get_delays(phydev);
+ if (ret)
+ return ret;
+
+ nxp_c45_set_delays(phydev);
+ break;
+ case PHY_INTERFACE_MODE_MII:
+ if (!(ret & MII_ABILITY)) {
+ phydev_err(phydev, "mii mode not supported\n");
+ return -EINVAL;
+ }
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_MII);
+ break;
+ case PHY_INTERFACE_MODE_REVMII:
+ if (!(ret & REVMII_ABILITY)) {
+ phydev_err(phydev, "rev-mii mode not supported\n");
+ return -EINVAL;
+ }
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_MII |
+ MII_BASIC_CONFIG_REV);
+ break;
+ case PHY_INTERFACE_MODE_RMII:
+ if (!(ret & RMII_ABILITY)) {
+ phydev_err(phydev, "rmii mode not supported\n");
+ return -EINVAL;
+ }
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RMII);
+ break;
+ case PHY_INTERFACE_MODE_SGMII:
+ if (!(ret & SGMII_ABILITY)) {
+ phydev_err(phydev, "sgmii mode not supported\n");
+ return -EINVAL;
+ }
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_SGMII);
+ break;
+ case PHY_INTERFACE_MODE_INTERNAL:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int nxp_c45_config_init(struct phy_device *phydev)
+{
+ int ret;
+
+ ret = nxp_c45_config_enable(phydev);
+ if (ret) {
+ phydev_err(phydev, "Failed to enable config\n");
+ return ret;
+ }
+
+ ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, PHY_CONFIG);
+ ret &= ~PHY_CONFIG_AUTO;
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONFIG, ret);
+
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, LINK_DROP_COUNTER, COUNTERS_EN);
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, RX_PREAMBLE_COUNT, COUNTERS_EN);
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, TX_PREAMBLE_COUNT, COUNTERS_EN);
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, RX_IPG_LENGTH, COUNTERS_EN);
+ phy_write_mmd(phydev, MDIO_MMD_VEND1, TX_IPG_LENGTH, COUNTERS_EN);
+
+ ret = nxp_c45_set_phy_mode(phydev);
+ if (ret)
+ return ret;
+
+ phydev->autoneg = AUTONEG_DISABLE;
+
+ return nxp_c45_start_op(phydev);
+}
+
+static int nxp_c45_probe(struct phy_device *phydev)
+{
+ struct nxp_c45_phy *priv;
+
+ priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ phydev->priv = priv;
+
+ return 0;
+}
+
+static struct phy_driver nxp_c45_driver[] = {
+ {
+ PHY_ID_MATCH_MODEL(PHY_ID_BASE_T1),
+ .name = "NXP C45 BASE-T1",
+ .features = PHY_BASIC_T1_FEATURES,
+ .probe = nxp_c45_probe,
+ .soft_reset = nxp_c45_soft_reset,
+ .config_aneg = nxp_c45_config_aneg,
+ .config_init = nxp_c45_config_init,
+ .read_status = nxp_c45_read_status,
+ .suspend = nxp_c45_suspend,
+ .resume = nxp_c45_resume,
+ .get_sset_count = nxp_c45_get_sset_count,
+ .get_strings = nxp_c45_get_strings,
+ .get_stats = nxp_c45_get_stats,
+ .cable_test_start = nxp_c45_cable_test_start,
+ .cable_test_get_status = nxp_c45_cable_test_get_status,
+ .set_loopback = nxp_c45_set_loopback,
+ .get_sqi = nxp_c45_get_sqi,
+ .get_sqi_max = nxp_c45_get_sqi_max,
+ },
+};
+
+module_phy_driver(nxp_c45_driver);
+
+static struct mdio_device_id __maybe_unused nxp_c45_tbl[] = {
+ { PHY_ID_MATCH_MODEL(PHY_ID_BASE_T1) }
+};
+
+MODULE_DEVICE_TABLE(mdio, nxp_c45_tbl);
+
+MODULE_AUTHOR("Radu Pirea <radu-nicolae.pirea@oss.nxp.com>");
+MODULE_DESCRIPTION("NXP C45 PHY driver");
+MODULE_LICENSE("GPL v2");
--
2.31.1
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 18:41 [PATCH] phy: nxp-c45: add driver for tja1103 Radu Pirea (NXP OSS)
@ 2021-04-09 19:18 ` Heiner Kallweit
2021-04-12 9:10 ` Radu Nicolae Pirea (NXP OSS)
2021-04-09 19:31 ` Jakub Kicinski
` (4 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Heiner Kallweit @ 2021-04-09 19:18 UTC (permalink / raw)
To: Radu Pirea (NXP OSS), andrew, linux, davem, kuba; +Cc: netdev, linux-kernel
On 09.04.2021 20:41, Radu Pirea (NXP OSS) wrote:
> Add driver for tja1103 driver and for future NXP C45 PHYs.
>
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> ---
> MAINTAINERS | 6 +
> drivers/net/phy/Kconfig | 6 +
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/nxp-c45.c | 622 ++++++++++++++++++++++++++++++++++++++
> 4 files changed, 635 insertions(+)
> create mode 100644 drivers/net/phy/nxp-c45.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a008b70f3c16..082a5eca8913 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12518,6 +12518,12 @@ F: drivers/nvmem/
> F: include/linux/nvmem-consumer.h
> F: include/linux/nvmem-provider.h
>
> +NXP C45 PHY DRIVER
> +M: Radu Pirea <radu-nicolae.pirea@oss.nxp.com>
> +L: netdev@vger.kernel.org
> +S: Maintained
> +F: drivers/net/phy/nxp-c45.c
> +
> NXP FSPI DRIVER
> M: Ashish Kumar <ashish.kumar@nxp.com>
> R: Yogesh Gaur <yogeshgaur.83@gmail.com>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 698bea312adc..fd2da80b5339 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -228,6 +228,12 @@ config NATIONAL_PHY
> help
> Currently supports the DP83865 PHY.
>
> +config NXP_C45_PHY
> + tristate "NXP C45 PHYs"
> + help
> + Enable support for NXP C45 PHYs.
> + Currently supports only the TJA1103 PHY.
> +
> config NXP_TJA11XX_PHY
> tristate "NXP TJA11xx PHYs support"
> depends on HWMON
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index a13e402074cf..a18f095748b5 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
> obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o
> obj-$(CONFIG_MICROSEMI_PHY) += mscc/
> obj-$(CONFIG_NATIONAL_PHY) += national.o
> +obj-$(CONFIG_NXP_C45_PHY) += nxp-c45.o
> obj-$(CONFIG_NXP_TJA11XX_PHY) += nxp-tja11xx.o
> obj-$(CONFIG_QSEMI_PHY) += qsemi.o
> obj-$(CONFIG_REALTEK_PHY) += realtek.o
> diff --git a/drivers/net/phy/nxp-c45.c b/drivers/net/phy/nxp-c45.c
> new file mode 100644
> index 000000000000..2961799f7d05
> --- /dev/null
> +++ b/drivers/net/phy/nxp-c45.c
> @@ -0,0 +1,622 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* NXP C45 PHY driver
> + * Copyright (C) 2021 NXP
> + * Copyright (C) 2021 Radu Pirea <radu-nicolae.pirea@oss.nxp.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/ethtool.h>
> +#include <linux/ethtool_netlink.h>
> +#include <linux/kernel.h>
> +#include <linux/mii.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +#include <linux/processor.h>
> +#include <linux/property.h>
> +
> +#define PHY_ID_BASE_T1 0x001BB010
> +
> +#define B100T1_PMAPMD_CTL 0x0834
> +#define B100T1_PMAPMD_CONFIG_EN BIT(15)
> +#define B100T1_PMAPMD_MASTER BIT(14)
> +#define MASTER_MODE (B100T1_PMAPMD_CONFIG_EN | B100T1_PMAPMD_MASTER)
> +#define SLAVE_MODE (B100T1_PMAPMD_CONFIG_EN)
> +
> +#define DEVICE_CONTROL 0x0040
> +#define DEVICE_CONTROL_RESET BIT(15)
> +#define DEVICE_CONTROL_CONFIG_GLOBAL_EN BIT(14)
> +#define DEVICE_CONTROL_CONFIG_ALL_EN BIT(13)
> +#define RESET_POLL_NS (250 * NSEC_PER_MSEC)
> +
> +#define PHY_CONTROL 0x8100
> +#define PHY_CONFIG_EN BIT(14)
> +#define PHY_START_OP BIT(0)
> +
> +#define PHY_CONFIG 0x8108
> +#define PHY_CONFIG_AUTO BIT(0)
> +
> +#define SIGNAL_QUALITY 0x8320
> +#define SQI_VALID BIT(14)
> +#define SQI_MASK GENMASK(2, 0)
> +#define MAX_SQI SQI_MASK
> +
> +#define CABLE_TEST 0x8330
> +#define CABLE_TEST_ENABLE BIT(15)
> +#define CABLE_TEST_START BIT(14)
> +#define CABLE_TEST_VALID BIT(13)
> +#define CABLE_TEST_OK 0x00
> +#define CABLE_TEST_SHORTED 0x01
> +#define CABLE_TEST_OPEN 0x02
> +#define CABLE_TEST_UNKNOWN 0x07
> +
> +#define PORT_CONTROL 0x8040
> +#define PORT_CONTROL_EN BIT(14)
> +
> +#define PORT_INFRA_CONTROL 0xAC00
> +#define PORT_INFRA_CONTROL_EN BIT(14)
> +
> +#define VND1_RXID 0xAFCC
> +#define VND1_TXID 0xAFCD
> +#define ID_ENABLE BIT(15)
> +
> +#define ABILITIES 0xAFC4
> +#define RGMII_ID_ABILITY BIT(15)
> +#define RGMII_ABILITY BIT(14)
> +#define RMII_ABILITY BIT(10)
> +#define REVMII_ABILITY BIT(9)
> +#define MII_ABILITY BIT(8)
> +#define SGMII_ABILITY BIT(0)
> +
> +#define MII_BASIC_CONFIG 0xAFC6
> +#define MII_BASIC_CONFIG_REV BIT(8)
> +#define MII_BASIC_CONFIG_SGMII 0x9
> +#define MII_BASIC_CONFIG_RGMII 0x7
> +#define MII_BASIC_CONFIG_RMII 0x5
> +#define MII_BASIC_CONFIG_MII 0x4
> +
> +#define SYMBOL_ERROR_COUNTER 0x8350
> +#define LINK_DROP_COUNTER 0x8352
> +#define LINK_LOSSES_AND_FAILURES 0x8353
> +#define R_GOOD_FRAME_CNT 0xA950
> +#define R_BAD_FRAME_CNT 0xA952
> +#define R_RXER_FRAME_CNT 0xA954
> +#define RX_PREAMBLE_COUNT 0xAFCE
> +#define TX_PREAMBLE_COUNT 0xAFCF
> +#define RX_IPG_LENGTH 0xAFD0
> +#define TX_IPG_LENGTH 0xAFD1
> +#define COUNTERS_EN BIT(15)
> +
> +#define CLK_25MHZ_PS_PERIOD 40000UL
> +#define PS_PER_DEGREE (CLK_25MHZ_PS_PERIOD / 360)
> +#define MIN_ID_PS 8222U
> +#define MAX_ID_PS 11300U
> +
> +struct nxp_c45_phy {
> + u32 tx_delay;
> + u32 rx_delay;
> +};
> +
> +struct nxp_c45_phy_stats {
> + const char *name;
> + u8 mmd;
> + u16 reg;
> + u8 off;
> + u16 mask;
> +};
> +
> +static const struct nxp_c45_phy_stats nxp_c45_hw_stats[] = {
> + { "phy_symbol_error_cnt", MDIO_MMD_VEND1, SYMBOL_ERROR_COUNTER, 0, GENMASK(15, 0) },
> + { "phy_link_status_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 8, GENMASK(13, 8) },
> + { "phy_link_availability_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 0, GENMASK(5, 0) },
> + { "phy_link_loss_cnt", MDIO_MMD_VEND1, LINK_LOSSES_AND_FAILURES, 10, GENMASK(15, 10) },
> + { "phy_link_failure_cnt", MDIO_MMD_VEND1, LINK_LOSSES_AND_FAILURES, 0, GENMASK(9, 0) },
> + { "r_good_frame_cnt", MDIO_MMD_VEND1, R_GOOD_FRAME_CNT, 0, GENMASK(15, 0) },
> + { "r_bad_frame_cnt", MDIO_MMD_VEND1, R_BAD_FRAME_CNT, 0, GENMASK(15, 0) },
> + { "r_rxer_frame_cnt", MDIO_MMD_VEND1, R_RXER_FRAME_CNT, 0, GENMASK(15, 0) },
> + { "rx_preamble_count", MDIO_MMD_VEND1, RX_PREAMBLE_COUNT, 0, GENMASK(5, 0) },
> + { "tx_preamble_count", MDIO_MMD_VEND1, TX_PREAMBLE_COUNT, 0, GENMASK(5, 0) },
> + { "rx_ipg_length", MDIO_MMD_VEND1, RX_IPG_LENGTH, 0, GENMASK(8, 0) },
> + { "tx_ipg_length", MDIO_MMD_VEND1, TX_IPG_LENGTH, 0, GENMASK(8, 0) },
> +};
> +
> +static int nxp_c45_get_sset_count(struct phy_device *phydev)
> +{
> + return ARRAY_SIZE(nxp_c45_hw_stats);
> +}
> +
> +static void nxp_c45_get_strings(struct phy_device *phydev, u8 *data)
> +{
> + size_t i;
> +
> + for (i = 0; i < ARRAY_SIZE(nxp_c45_hw_stats); i++) {
> + strncpy(data + i * ETH_GSTRING_LEN,
> + nxp_c45_hw_stats[i].name, ETH_GSTRING_LEN);
> + }
> +}
> +
> +static void nxp_c45_get_stats(struct phy_device *phydev,
> + struct ethtool_stats *stats, u64 *data)
> +{
> + size_t i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(nxp_c45_hw_stats); i++) {
> + ret = phy_read_mmd(phydev, nxp_c45_hw_stats[i].mmd, nxp_c45_hw_stats[i].reg);
> + if (ret < 0) {
> + data[i] = U64_MAX;
> + } else {
> + data[i] = ret & nxp_c45_hw_stats[i].mask;
> + data[i] >>= nxp_c45_hw_stats[i].off;
> + }
> + }
> +}
> +
> +static int nxp_c45_config_enable(struct phy_device *phydev)
> +{
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL, DEVICE_CONTROL_CONFIG_GLOBAL_EN |
> + DEVICE_CONTROL_CONFIG_ALL_EN);
> + usleep_range(400, 450);
> +
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, PORT_CONTROL, PORT_CONTROL_EN);
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL, PHY_CONFIG_EN);
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, PORT_INFRA_CONTROL, PORT_INFRA_CONTROL_EN);
> +
> + return 0;
> +}
> +
> +static int nxp_c45_start_op(struct phy_device *phydev)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL);
> + reg |= PHY_START_OP;
> +
> + return phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL, reg);
You may want to use phy_set_bits_mmd() here. Similar comment
applies to other places in the driver where phy_clear_bits_mmd()
and phy_modify_mmd() could be used.
> +}
> +
> +static bool nxp_c45_can_sleep(struct phy_device *phydev)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);
> + if (reg < 0)
> + return false;
> +
> + return !!(reg & MDIO_STAT1_LPOWERABLE);
> +}
> +
> +static int nxp_c45_resume(struct phy_device *phydev)
> +{
> + int reg;
> +
> + if (!nxp_c45_can_sleep(phydev))
> + return -EOPNOTSUPP;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> + reg &= ~MDIO_CTRL1_LPOWER;
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> +
> + return 0;
> +}
> +
> +static int nxp_c45_suspend(struct phy_device *phydev)
> +{
> + int reg;
> +
> + if (!nxp_c45_can_sleep(phydev))
> + return -EOPNOTSUPP;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> + reg |= MDIO_CTRL1_LPOWER;
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> +
> + return 0;
> +}
> +
> +static int nxp_c45_reset_done(struct phy_device *phydev)
> +{
> + return !(phy_read_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL) & DEVICE_CONTROL_RESET);
> +}
> +
> +static int nxp_c45_reset_done_or_timeout(struct phy_device *phydev,
> + ktime_t timeout)
> +{
> + ktime_t cur = ktime_get();
> +
> + return nxp_c45_reset_done(phydev) || ktime_after(cur, timeout);
> +}
> +
> +static int nxp_c45_soft_reset(struct phy_device *phydev)
> +{
> + ktime_t timeout;
> + int ret;
> +
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL, DEVICE_CONTROL_RESET);
> + if (ret)
> + return ret;
> +
> + timeout = ktime_add_ns(ktime_get(), RESET_POLL_NS);
> + spin_until_cond(nxp_c45_reset_done_or_timeout(phydev, timeout));
> + if (!nxp_c45_reset_done(phydev)) {
> + phydev_err(phydev, "reset fail\n");
> + return -EIO;
> + }
> + return 0;
> +}
> +
> +static int nxp_c45_cable_test_start(struct phy_device *phydev)
> +{
> + return phy_write_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST,
> + CABLE_TEST_ENABLE | CABLE_TEST_START);
> +}
> +
> +static int nxp_c45_cable_test_get_status(struct phy_device *phydev,
> + bool *finished)
> +{
> + int ret;
> + u8 cable_test_result;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST);
> + if (!(ret & CABLE_TEST_VALID)) {
> + *finished = false;
> + return 0;
> + }
> +
> + *finished = true;
> + cable_test_result = ret & GENMASK(2, 0);
> +
> + switch (cable_test_result) {
> + case CABLE_TEST_OK:
> + ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
> + ETHTOOL_A_CABLE_RESULT_CODE_OK);
> + break;
> + case CABLE_TEST_SHORTED:
> + ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
> + ETHTOOL_A_CABLE_RESULT_CODE_SAME_SHORT);
> + break;
> + case CABLE_TEST_OPEN:
> + ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
> + ETHTOOL_A_CABLE_RESULT_CODE_OPEN);
> + break;
> + default:
> + ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A,
> + ETHTOOL_A_CABLE_RESULT_CODE_UNSPEC);
> + }
> +
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST, 0);
> +
> + return nxp_c45_start_op(phydev);
> +}
> +
> +static int nxp_c45_setup_master_slave(struct phy_device *phydev)
> +{
> + switch (phydev->master_slave_set) {
> + case MASTER_SLAVE_CFG_MASTER_FORCE:
> + case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, B100T1_PMAPMD_CTL, MASTER_MODE);
> + break;
> + case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> + case MASTER_SLAVE_CFG_SLAVE_FORCE:
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, B100T1_PMAPMD_CTL, SLAVE_MODE);
> + break;
> + case MASTER_SLAVE_CFG_UNKNOWN:
> + case MASTER_SLAVE_CFG_UNSUPPORTED:
> + return 0;
> + default:
> + phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> + return -EOPNOTSUPP;
> + }
> +
> + return 0;
> +}
> +
> +static int nxp_c45_read_master_slave(struct phy_device *phydev)
> +{
> + int reg;
> +
> + phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
> + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, B100T1_PMAPMD_CTL);
> + if (reg < 0)
> + return reg;
> +
> + if (reg & B100T1_PMAPMD_MASTER) {
> + phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE;
> + phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER;
> + } else {
> + phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE;
> + phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE;
> + }
> +
> + return 0;
> +}
> +
> +static int nxp_c45_config_aneg(struct phy_device *phydev)
> +{
> + return nxp_c45_setup_master_slave(phydev);
> +}
> +
> +static int nxp_c45_read_status(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = genphy_c45_read_status(phydev);
> + if (ret)
> + return ret;
> +
> + ret = nxp_c45_read_master_slave(phydev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int nxp_c45_set_loopback(struct phy_device *phydev, bool enable)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
> + if (reg < 0)
> + return reg;
> +
> + if (enable)
> + reg |= MDIO_PCS_CTRL1_LOOPBACK;
> + else
> + reg &= ~MDIO_PCS_CTRL1_LOOPBACK;
> +
> + phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, reg);
> +
> + phydev->loopback_enabled = enable;
> +
> + phydev_dbg(phydev, "Loopback %s\n", enable ? "enabled" : "disabled");
> +
> + return 0;
> +}
> +
> +static int nxp_c45_get_sqi(struct phy_device *phydev)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, SIGNAL_QUALITY);
> + if (!(reg & SQI_VALID))
> + return -EINVAL;
> +
> + reg &= SQI_MASK;
> +
> + return reg;
> +}
> +
> +static int nxp_c45_get_sqi_max(struct phy_device *phydev)
> +{
> + return MAX_SQI;
> +}
> +
> +static int nxp_c45_check_delay(struct phy_device *phydev, u32 delay)
> +{
> + if (delay < MIN_ID_PS) {
> + phydev_err(phydev, "delay value smaller than %u\n", MIN_ID_PS);
> + return -EINVAL;
> + }
> +
> + if (delay > MAX_ID_PS) {
> + phydev_err(phydev, "delay value higher than %u\n", MAX_ID_PS);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static u64 nxp_c45_get_phase_shift(u64 phase_offset_raw)
> +{
> + /* The delay in degree phase is 73.8 + phase_offset_raw * 0.9.
> + * To avoid floating point operations we'll multiply by 10
> + * and get 1 decimal point precision.
> + */
> + phase_offset_raw *= 10;
> + return (phase_offset_raw - 738) / 9;
> +}
> +
> +static void nxp_c45_set_delays(struct phy_device *phydev)
> +{
> + struct nxp_c45_phy *priv = phydev->priv;
> + u64 tx_delay = priv->tx_delay;
> + u64 rx_delay = priv->rx_delay;
> + u64 degree;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + degree = tx_delay / PS_PER_DEGREE;
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_TXID,
> + ID_ENABLE | nxp_c45_get_phase_shift(degree));
> + }
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> + degree = rx_delay / PS_PER_DEGREE;
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_RXID,
> + ID_ENABLE | nxp_c45_get_phase_shift(degree));
> + }
> +}
> +
> +static int nxp_c45_get_delays(struct phy_device *phydev)
> +{
> + struct nxp_c45_phy *priv = phydev->priv;
> + int ret;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + ret = device_property_read_u32(&phydev->mdio.dev, "tx-internal-delay-ps",
> + &priv->tx_delay);
> + if (ret) {
> + phydev_err(phydev, "tx-internal-delay-ps property missing\n");
> + return ret;
> + }
> + ret = nxp_c45_check_delay(phydev, priv->tx_delay);
> + if (ret) {
> + phydev_err(phydev, "tx-internal-delay-ps invalid value\n");
> + return ret;
> + }
> + }
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + ret = device_property_read_u32(&phydev->mdio.dev, "rx-internal-delay-ps",
> + &priv->rx_delay);
> + if (ret) {
> + phydev_err(phydev, "rx-internal-delay-ps property missing\n");
> + return ret;
> + }
> + ret = nxp_c45_check_delay(phydev, priv->rx_delay);
> + if (ret) {
> + phydev_err(phydev, "rx-internal-delay-ps invalid value\n");
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int nxp_c45_set_phy_mode(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ABILITIES);
> + phydev_dbg(phydev, "Clause 45 managed PHY abilities 0x%x\n", ret);
> +
> + switch (phydev->interface) {
> + case PHY_INTERFACE_MODE_RGMII:
> + if (!(ret & RGMII_ABILITY)) {
> + phydev_err(phydev, "rgmii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> + break;
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + if (!(ret & RGMII_ID_ABILITY)) {
> + phydev_err(phydev, "rgmii-id, rgmii-txid, rgmii-rxid modes are not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> + ret = nxp_c45_get_delays(phydev);
> + if (ret)
> + return ret;
> +
> + nxp_c45_set_delays(phydev);
> + break;
> + case PHY_INTERFACE_MODE_MII:
> + if (!(ret & MII_ABILITY)) {
> + phydev_err(phydev, "mii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_MII);
> + break;
> + case PHY_INTERFACE_MODE_REVMII:
> + if (!(ret & REVMII_ABILITY)) {
> + phydev_err(phydev, "rev-mii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_MII |
> + MII_BASIC_CONFIG_REV);
> + break;
> + case PHY_INTERFACE_MODE_RMII:
> + if (!(ret & RMII_ABILITY)) {
> + phydev_err(phydev, "rmii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RMII);
> + break;
> + case PHY_INTERFACE_MODE_SGMII:
> + if (!(ret & SGMII_ABILITY)) {
> + phydev_err(phydev, "sgmii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_SGMII);
> + break;
> + case PHY_INTERFACE_MODE_INTERNAL:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int nxp_c45_config_init(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = nxp_c45_config_enable(phydev);
> + if (ret) {
> + phydev_err(phydev, "Failed to enable config\n");
> + return ret;
> + }
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, PHY_CONFIG);
> + ret &= ~PHY_CONFIG_AUTO;
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONFIG, ret);
> +
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, LINK_DROP_COUNTER, COUNTERS_EN);
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, RX_PREAMBLE_COUNT, COUNTERS_EN);
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, TX_PREAMBLE_COUNT, COUNTERS_EN);
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, RX_IPG_LENGTH, COUNTERS_EN);
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, TX_IPG_LENGTH, COUNTERS_EN);
> +
> + ret = nxp_c45_set_phy_mode(phydev);
> + if (ret)
> + return ret;
> +
> + phydev->autoneg = AUTONEG_DISABLE;
> +
> + return nxp_c45_start_op(phydev);
> +}
> +
> +static int nxp_c45_probe(struct phy_device *phydev)
> +{
> + struct nxp_c45_phy *priv;
> +
> + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + phydev->priv = priv;
> +
> + return 0;
> +}
> +
> +static struct phy_driver nxp_c45_driver[] = {
> + {
> + PHY_ID_MATCH_MODEL(PHY_ID_BASE_T1),
> + .name = "NXP C45 BASE-T1",
> + .features = PHY_BASIC_T1_FEATURES,
> + .probe = nxp_c45_probe,
> + .soft_reset = nxp_c45_soft_reset,
> + .config_aneg = nxp_c45_config_aneg,
> + .config_init = nxp_c45_config_init,
> + .read_status = nxp_c45_read_status,
> + .suspend = nxp_c45_suspend,
> + .resume = nxp_c45_resume,
> + .get_sset_count = nxp_c45_get_sset_count,
> + .get_strings = nxp_c45_get_strings,
> + .get_stats = nxp_c45_get_stats,
> + .cable_test_start = nxp_c45_cable_test_start,
> + .cable_test_get_status = nxp_c45_cable_test_get_status,
> + .set_loopback = nxp_c45_set_loopback,
> + .get_sqi = nxp_c45_get_sqi,
> + .get_sqi_max = nxp_c45_get_sqi_max,
How about interrupt support?
> + },
> +};
> +
> +module_phy_driver(nxp_c45_driver);
> +
> +static struct mdio_device_id __maybe_unused nxp_c45_tbl[] = {
> + { PHY_ID_MATCH_MODEL(PHY_ID_BASE_T1) }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, nxp_c45_tbl);
> +
> +MODULE_AUTHOR("Radu Pirea <radu-nicolae.pirea@oss.nxp.com>");
> +MODULE_DESCRIPTION("NXP C45 PHY driver");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 18:41 [PATCH] phy: nxp-c45: add driver for tja1103 Radu Pirea (NXP OSS)
2021-04-09 19:18 ` Heiner Kallweit
@ 2021-04-09 19:31 ` Jakub Kicinski
2021-04-09 19:36 ` Andrew Lunn
` (3 subsequent siblings)
5 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2021-04-09 19:31 UTC (permalink / raw)
To: Radu Pirea (NXP OSS)
Cc: andrew, hkallweit1, linux, davem, netdev, linux-kernel
On Fri, 9 Apr 2021 21:41:06 +0300 Radu Pirea (NXP OSS) wrote:
> Add driver for tja1103 driver and for future NXP C45 PHYs.
>
> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
drivers/net/phy/nxp-c45: struct mdio_device_id is 8 bytes. The last of 1 is:
0x10 0xb0 0x1b 0x00 0xf0 0xff 0xff 0xff
FATAL: modpost: drivers/net/phy/nxp-c45: struct mdio_device_id is not terminated with a NULL entry!
make[2]: *** [Module.symvers] Error 1
make[1]: *** [modules] Error 2
make: *** [__sub-make] Error 2
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 18:41 [PATCH] phy: nxp-c45: add driver for tja1103 Radu Pirea (NXP OSS)
2021-04-09 19:18 ` Heiner Kallweit
2021-04-09 19:31 ` Jakub Kicinski
@ 2021-04-09 19:36 ` Andrew Lunn
2021-04-12 10:02 ` Radu Nicolae Pirea (NXP OSS)
2021-04-11 2:33 ` kernel test robot
` (2 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Andrew Lunn @ 2021-04-09 19:36 UTC (permalink / raw)
To: Radu Pirea (NXP OSS); +Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote:
> Add driver for tja1103 driver and for future NXP C45 PHYs.
So apart from c45 vs c22, how does this differ to nxp-tja11xx.c?
Do we really want two different drivers for the same hardware?
Can we combine them somehow?
> +config NXP_C45_PHY
> + tristate "NXP C45 PHYs"
This is also very vague. So in the future it will support PHYs other
than the TJA series?
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 18:41 [PATCH] phy: nxp-c45: add driver for tja1103 Radu Pirea (NXP OSS)
` (2 preceding siblings ...)
2021-04-09 19:36 ` Andrew Lunn
@ 2021-04-11 2:33 ` kernel test robot
2021-04-12 9:50 ` Russell King - ARM Linux admin
2021-04-12 18:04 ` Andrew Lunn
5 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2021-04-11 2:33 UTC (permalink / raw)
To: Radu Pirea (NXP OSS), andrew, hkallweit1, linux, davem, kuba
Cc: kbuild-all, netdev, linux-kernel, Radu Pirea (NXP OSS)
[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]
Hi "Radu,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.12-rc6 next-20210409]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Radu-Pirea-NXP-OSS/phy-nxp-c45-add-driver-for-tja1103/20210410-024203
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 17e7124aad766b3f158943acb51467f86220afe9
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/436e508ba7a4bbe24891acf430d7722ed1f5e128
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Radu-Pirea-NXP-OSS/phy-nxp-c45-add-driver-for-tja1103/20210410-024203
git checkout 436e508ba7a4bbe24891acf430d7722ed1f5e128
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: drivers/net/phy/nxp-c45.o: in function `nxp_c45_config_init':
>> nxp-c45.c:(.text+0xe2c): undefined reference to `__aeabi_uldivmod'
>> arm-linux-gnueabi-ld: nxp-c45.c:(.text+0xe64): undefined reference to `__aeabi_uldivmod'
arm-linux-gnueabi-ld: nxp-c45.c:(.text+0xec0): undefined reference to `__aeabi_uldivmod'
arm-linux-gnueabi-ld: nxp-c45.c:(.text+0xef8): undefined reference to `__aeabi_uldivmod'
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 77806 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 19:18 ` Heiner Kallweit
@ 2021-04-12 9:10 ` Radu Nicolae Pirea (NXP OSS)
0 siblings, 0 replies; 21+ messages in thread
From: Radu Nicolae Pirea (NXP OSS) @ 2021-04-12 9:10 UTC (permalink / raw)
To: Heiner Kallweit, andrew, linux, davem, kuba; +Cc: netdev, linux-kernel
On Fri, 2021-04-09 at 21:18 +0200, Heiner Kallweit wrote:
> On 09.04.2021 20:41, Radu Pirea (NXP OSS) wrote:
> > Add driver for tja1103 driver and for future NXP C45 PHYs.
> >
> > Signed-off-by: Radu Pirea (NXP OSS)
> > <radu-nicolae.pirea@oss.nxp.com>
> > ---
> > MAINTAINERS | 6 +
> > drivers/net/phy/Kconfig | 6 +
> > drivers/net/phy/Makefile | 1 +
> > drivers/net/phy/nxp-c45.c | 622
> > ++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 635 insertions(+)
> > create mode 100644 drivers/net/phy/nxp-c45.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a008b70f3c16..082a5eca8913 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12518,6 +12518,12 @@ F: drivers/nvmem/
> > F: include/linux/nvmem-consumer.h
> > F: include/linux/nvmem-provider.h
> >
> > +NXP C45 PHY DRIVER
> > +M: Radu Pirea <radu-nicolae.pirea@oss.nxp.com>
> > +L: netdev@vger.kernel.org
> > +S: Maintained
> > +F: drivers/net/phy/nxp-c45.c
> > +
> > NXP FSPI DRIVER
> > M: Ashish Kumar <ashish.kumar@nxp.com>
> > R: Yogesh Gaur <yogeshgaur.83@gmail.com>
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 698bea312adc..fd2da80b5339 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -228,6 +228,12 @@ config NATIONAL_PHY
> > help
> > Currently supports the DP83865 PHY.
> >
> > +config NXP_C45_PHY
> > + tristate "NXP C45 PHYs"
> > + help
> > + Enable support for NXP C45 PHYs.
> > + Currently supports only the TJA1103 PHY.
> > +
> > config NXP_TJA11XX_PHY
> > tristate "NXP TJA11xx PHYs support"
> > depends on HWMON
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index a13e402074cf..a18f095748b5 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -70,6 +70,7 @@ obj-$(CONFIG_MICROCHIP_PHY) += microchip.o
> > obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o
> > obj-$(CONFIG_MICROSEMI_PHY) += mscc/
> > obj-$(CONFIG_NATIONAL_PHY) += national.o
> > +obj-$(CONFIG_NXP_C45_PHY) += nxp-c45.o
> > obj-$(CONFIG_NXP_TJA11XX_PHY) += nxp-tja11xx.o
> > obj-$(CONFIG_QSEMI_PHY) += qsemi.o
> > obj-$(CONFIG_REALTEK_PHY) += realtek.o
> > diff --git a/drivers/net/phy/nxp-c45.c b/drivers/net/phy/nxp-c45.c
> > new file mode 100644
> > index 000000000000..2961799f7d05
> > --- /dev/null
> > +++ b/drivers/net/phy/nxp-c45.c
> > @@ -0,0 +1,622 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* NXP C45 PHY driver
> > + * Copyright (C) 2021 NXP
> > + * Copyright (C) 2021 Radu Pirea <radu-nicolae.pirea@oss.nxp.com>
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/ethtool_netlink.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mii.h>
> > +#include <linux/module.h>
> > +#include <linux/phy.h>
> > +#include <linux/processor.h>
> > +#include <linux/property.h>
> > +
> > +#define PHY_ID_BASE_T1 0x001BB010
> > +
> > +#define B100T1_PMAPMD_CTL 0x0834
> > +#define B100T1_PMAPMD_CONFIG_EN BIT(15)
> > +#define B100T1_PMAPMD_MASTER BIT(14)
> > +#define MASTER_MODE (B100T1_PMAPMD_CONFIG_EN |
> > B100T1_PMAPMD_MASTER)
> > +#define SLAVE_MODE (B100T1_PMAPMD_CONFIG_EN)
> > +
> > +#define DEVICE_CONTROL 0x0040
> > +#define DEVICE_CONTROL_RESET BIT(15)
> > +#define DEVICE_CONTROL_CONFIG_GLOBAL_EN BIT(14)
> > +#define DEVICE_CONTROL_CONFIG_ALL_EN BIT(13)
> > +#define RESET_POLL_NS (250 * NSEC_PER_MSEC)
> > +
> > +#define PHY_CONTROL 0x8100
> > +#define PHY_CONFIG_EN BIT(14)
> > +#define PHY_START_OP BIT(0)
> > +
> > +#define PHY_CONFIG 0x8108
> > +#define PHY_CONFIG_AUTO BIT(0)
> > +
> > +#define SIGNAL_QUALITY 0x8320
> > +#define SQI_VALID BIT(14)
> > +#define SQI_MASK GENMASK(2, 0)
> > +#define MAX_SQI SQI_MASK
> > +
> > +#define CABLE_TEST 0x8330
> > +#define CABLE_TEST_ENABLE BIT(15)
> > +#define CABLE_TEST_START BIT(14)
> > +#define CABLE_TEST_VALID BIT(13)
> > +#define CABLE_TEST_OK 0x00
> > +#define CABLE_TEST_SHORTED 0x01
> > +#define CABLE_TEST_OPEN 0x02
> > +#define CABLE_TEST_UNKNOWN 0x07
> > +
> > +#define PORT_CONTROL 0x8040
> > +#define PORT_CONTROL_EN BIT(14)
> > +
> > +#define PORT_INFRA_CONTROL 0xAC00
> > +#define PORT_INFRA_CONTROL_EN BIT(14)
> > +
> > +#define VND1_RXID 0xAFCC
> > +#define VND1_TXID 0xAFCD
> > +#define ID_ENABLE BIT(15)
> > +
> > +#define ABILITIES 0xAFC4
> > +#define RGMII_ID_ABILITY BIT(15)
> > +#define RGMII_ABILITY BIT(14)
> > +#define RMII_ABILITY BIT(10)
> > +#define REVMII_ABILITY BIT(9)
> > +#define MII_ABILITY BIT(8)
> > +#define SGMII_ABILITY BIT(0)
> > +
> > +#define MII_BASIC_CONFIG 0xAFC6
> > +#define MII_BASIC_CONFIG_REV BIT(8)
> > +#define MII_BASIC_CONFIG_SGMII 0x9
> > +#define MII_BASIC_CONFIG_RGMII 0x7
> > +#define MII_BASIC_CONFIG_RMII 0x5
> > +#define MII_BASIC_CONFIG_MII 0x4
> > +
> > +#define SYMBOL_ERROR_COUNTER 0x8350
> > +#define LINK_DROP_COUNTER 0x8352
> > +#define LINK_LOSSES_AND_FAILURES 0x8353
> > +#define R_GOOD_FRAME_CNT 0xA950
> > +#define R_BAD_FRAME_CNT 0xA952
> > +#define R_RXER_FRAME_CNT 0xA954
> > +#define RX_PREAMBLE_COUNT 0xAFCE
> > +#define TX_PREAMBLE_COUNT 0xAFCF
> > +#define RX_IPG_LENGTH 0xAFD0
> > +#define TX_IPG_LENGTH 0xAFD1
> > +#define COUNTERS_EN BIT(15)
> > +
> > +#define CLK_25MHZ_PS_PERIOD 40000UL
> > +#define PS_PER_DEGREE (CLK_25MHZ_PS_PERIOD / 360)
> > +#define MIN_ID_PS 8222U
> > +#define MAX_ID_PS 11300U
> > +
> > +struct nxp_c45_phy {
> > + u32 tx_delay;
> > + u32 rx_delay;
> > +};
> > +
> > +struct nxp_c45_phy_stats {
> > + const char *name;
> > + u8 mmd;
> > + u16 reg;
> > + u8 off;
> > + u16 mask;
> > +};
> > +
> > +static const struct nxp_c45_phy_stats nxp_c45_hw_stats[] = {
> > + { "phy_symbol_error_cnt", MDIO_MMD_VEND1,
> > SYMBOL_ERROR_COUNTER, 0, GENMASK(15, 0) },
> > + { "phy_link_status_drop_cnt", MDIO_MMD_VEND1,
> > LINK_DROP_COUNTER, 8, GENMASK(13, 8) },
> > + { "phy_link_availability_drop_cnt", MDIO_MMD_VEND1,
> > LINK_DROP_COUNTER, 0, GENMASK(5, 0) },
> > + { "phy_link_loss_cnt", MDIO_MMD_VEND1,
> > LINK_LOSSES_AND_FAILURES, 10, GENMASK(15, 10) },
> > + { "phy_link_failure_cnt", MDIO_MMD_VEND1,
> > LINK_LOSSES_AND_FAILURES, 0, GENMASK(9, 0) },
> > + { "r_good_frame_cnt", MDIO_MMD_VEND1, R_GOOD_FRAME_CNT, 0,
> > GENMASK(15, 0) },
> > + { "r_bad_frame_cnt", MDIO_MMD_VEND1, R_BAD_FRAME_CNT, 0,
> > GENMASK(15, 0) },
> > + { "r_rxer_frame_cnt", MDIO_MMD_VEND1, R_RXER_FRAME_CNT, 0,
> > GENMASK(15, 0) },
> > + { "rx_preamble_count", MDIO_MMD_VEND1, RX_PREAMBLE_COUNT,
> > 0, GENMASK(5, 0) },
> > + { "tx_preamble_count", MDIO_MMD_VEND1, TX_PREAMBLE_COUNT,
> > 0, GENMASK(5, 0) },
> > + { "rx_ipg_length", MDIO_MMD_VEND1, RX_IPG_LENGTH, 0,
> > GENMASK(8, 0) },
> > + { "tx_ipg_length", MDIO_MMD_VEND1, TX_IPG_LENGTH, 0,
> > GENMASK(8, 0) },
> > +};
> > +
> > +static int nxp_c45_get_sset_count(struct phy_device *phydev)
> > +{
> > + return ARRAY_SIZE(nxp_c45_hw_stats);
> > +}
> > +
> > +static void nxp_c45_get_strings(struct phy_device *phydev, u8
> > *data)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(nxp_c45_hw_stats); i++) {
> > + strncpy(data + i * ETH_GSTRING_LEN,
> > + nxp_c45_hw_stats[i].name, ETH_GSTRING_LEN);
> > + }
> > +}
> > +
> > +static void nxp_c45_get_stats(struct phy_device *phydev,
> > + struct ethtool_stats *stats, u64
> > *data)
> > +{
> > + size_t i;
> > + int ret;
> > +
> > + for (i = 0; i < ARRAY_SIZE(nxp_c45_hw_stats); i++) {
> > + ret = phy_read_mmd(phydev, nxp_c45_hw_stats[i].mmd,
> > nxp_c45_hw_stats[i].reg);
> > + if (ret < 0) {
> > + data[i] = U64_MAX;
> > + } else {
> > + data[i] = ret & nxp_c45_hw_stats[i].mask;
> > + data[i] >>= nxp_c45_hw_stats[i].off;
> > + }
> > + }
> > +}
> > +
> > +static int nxp_c45_config_enable(struct phy_device *phydev)
> > +{
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL,
> > DEVICE_CONTROL_CONFIG_GLOBAL_EN |
> > + DEVICE_CONTROL_CONFIG_ALL_EN);
> > + usleep_range(400, 450);
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, PORT_CONTROL,
> > PORT_CONTROL_EN);
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL,
> > PHY_CONFIG_EN);
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, PORT_INFRA_CONTROL,
> > PORT_INFRA_CONTROL_EN);
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_start_op(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL);
> > + reg |= PHY_START_OP;
> > +
> > + return phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONTROL,
> > reg);
>
> You may want to use phy_set_bits_mmd() here. Similar comment
> applies to other places in the driver where phy_clear_bits_mmd()
> and phy_modify_mmd() could be used.
Thank you for suggestion. I will consider this.
>
> > +}
> > +
> > +static bool nxp_c45_can_sleep(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);
> > + if (reg < 0)
> > + return false;
> > +
> > + return !!(reg & MDIO_STAT1_LPOWERABLE);
> > +}
> > +
> > +static int nxp_c45_resume(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + if (!nxp_c45_can_sleep(phydev))
> > + return -EOPNOTSUPP;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> > + reg &= ~MDIO_CTRL1_LPOWER;
> > + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_suspend(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + if (!nxp_c45_can_sleep(phydev))
> > + return -EOPNOTSUPP;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> > + reg |= MDIO_CTRL1_LPOWER;
> > + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_reset_done(struct phy_device *phydev)
> > +{
> > + return !(phy_read_mmd(phydev, MDIO_MMD_VEND1,
> > DEVICE_CONTROL) & DEVICE_CONTROL_RESET);
> > +}
> > +
> > +static int nxp_c45_reset_done_or_timeout(struct phy_device
> > *phydev,
> > + ktime_t timeout)
> > +{
> > + ktime_t cur = ktime_get();
> > +
> > + return nxp_c45_reset_done(phydev) || ktime_after(cur,
> > timeout);
> > +}
> > +
> > +static int nxp_c45_soft_reset(struct phy_device *phydev)
> > +{
> > + ktime_t timeout;
> > + int ret;
> > +
> > + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, DEVICE_CONTROL,
> > DEVICE_CONTROL_RESET);
> > + if (ret)
> > + return ret;
> > +
> > + timeout = ktime_add_ns(ktime_get(), RESET_POLL_NS);
> > + spin_until_cond(nxp_c45_reset_done_or_timeout(phydev,
> > timeout));
> > + if (!nxp_c45_reset_done(phydev)) {
> > + phydev_err(phydev, "reset fail\n");
> > + return -EIO;
> > + }
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_cable_test_start(struct phy_device *phydev)
> > +{
> > + return phy_write_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST,
> > + CABLE_TEST_ENABLE | CABLE_TEST_START);
> > +}
> > +
> > +static int nxp_c45_cable_test_get_status(struct phy_device
> > *phydev,
> > + bool *finished)
> > +{
> > + int ret;
> > + u8 cable_test_result;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST);
> > + if (!(ret & CABLE_TEST_VALID)) {
> > + *finished = false;
> > + return 0;
> > + }
> > +
> > + *finished = true;
> > + cable_test_result = ret & GENMASK(2, 0);
> > +
> > + switch (cable_test_result) {
> > + case CABLE_TEST_OK:
> > + ethnl_cable_test_result(phydev,
> > ETHTOOL_A_CABLE_PAIR_A,
> > + ETHTOOL_A_CABLE_RESULT_CODE
> > _OK);
> > + break;
> > + case CABLE_TEST_SHORTED:
> > + ethnl_cable_test_result(phydev,
> > ETHTOOL_A_CABLE_PAIR_A,
> > + ETHTOOL_A_CABLE_RESULT_CODE
> > _SAME_SHORT);
> > + break;
> > + case CABLE_TEST_OPEN:
> > + ethnl_cable_test_result(phydev,
> > ETHTOOL_A_CABLE_PAIR_A,
> > + ETHTOOL_A_CABLE_RESULT_CODE
> > _OPEN);
> > + break;
> > + default:
> > + ethnl_cable_test_result(phydev,
> > ETHTOOL_A_CABLE_PAIR_A,
> > + ETHTOOL_A_CABLE_RESULT_CODE
> > _UNSPEC);
> > + }
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, CABLE_TEST, 0);
> > +
> > + return nxp_c45_start_op(phydev);
> > +}
> > +
> > +static int nxp_c45_setup_master_slave(struct phy_device *phydev)
> > +{
> > + switch (phydev->master_slave_set) {
> > + case MASTER_SLAVE_CFG_MASTER_FORCE:
> > + case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> > + phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
> > B100T1_PMAPMD_CTL, MASTER_MODE);
> > + break;
> > + case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> > + case MASTER_SLAVE_CFG_SLAVE_FORCE:
> > + phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
> > B100T1_PMAPMD_CTL, SLAVE_MODE);
> > + break;
> > + case MASTER_SLAVE_CFG_UNKNOWN:
> > + case MASTER_SLAVE_CFG_UNSUPPORTED:
> > + return 0;
> > + default:
> > + phydev_warn(phydev, "Unsupported Master/Slave
> > mode\n");
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_read_master_slave(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN;
> > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD,
> > B100T1_PMAPMD_CTL);
> > + if (reg < 0)
> > + return reg;
> > +
> > + if (reg & B100T1_PMAPMD_MASTER) {
> > + phydev->master_slave_get =
> > MASTER_SLAVE_CFG_MASTER_FORCE;
> > + phydev->master_slave_state =
> > MASTER_SLAVE_STATE_MASTER;
> > + } else {
> > + phydev->master_slave_get =
> > MASTER_SLAVE_CFG_SLAVE_FORCE;
> > + phydev->master_slave_state =
> > MASTER_SLAVE_STATE_SLAVE;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_config_aneg(struct phy_device *phydev)
> > +{
> > + return nxp_c45_setup_master_slave(phydev);
> > +}
> > +
> > +static int nxp_c45_read_status(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = genphy_c45_read_status(phydev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = nxp_c45_read_master_slave(phydev);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_set_loopback(struct phy_device *phydev, bool
> > enable)
> > +{
> > + int reg;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1);
> > + if (reg < 0)
> > + return reg;
> > +
> > + if (enable)
> > + reg |= MDIO_PCS_CTRL1_LOOPBACK;
> > + else
> > + reg &= ~MDIO_PCS_CTRL1_LOOPBACK;
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_PCS, MDIO_CTRL1, reg);
> > +
> > + phydev->loopback_enabled = enable;
> > +
> > + phydev_dbg(phydev, "Loopback %s\n", enable ? "enabled" :
> > "disabled");
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_get_sqi(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, SIGNAL_QUALITY);
> > + if (!(reg & SQI_VALID))
> > + return -EINVAL;
> > +
> > + reg &= SQI_MASK;
> > +
> > + return reg;
> > +}
> > +
> > +static int nxp_c45_get_sqi_max(struct phy_device *phydev)
> > +{
> > + return MAX_SQI;
> > +}
> > +
> > +static int nxp_c45_check_delay(struct phy_device *phydev, u32
> > delay)
> > +{
> > + if (delay < MIN_ID_PS) {
> > + phydev_err(phydev, "delay value smaller than %u\n",
> > MIN_ID_PS);
> > + return -EINVAL;
> > + }
> > +
> > + if (delay > MAX_ID_PS) {
> > + phydev_err(phydev, "delay value higher than %u\n",
> > MAX_ID_PS);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static u64 nxp_c45_get_phase_shift(u64 phase_offset_raw)
> > +{
> > + /* The delay in degree phase is 73.8 + phase_offset_raw *
> > 0.9.
> > + * To avoid floating point operations we'll multiply by 10
> > + * and get 1 decimal point precision.
> > + */
> > + phase_offset_raw *= 10;
> > + return (phase_offset_raw - 738) / 9;
> > +}
> > +
> > +static void nxp_c45_set_delays(struct phy_device *phydev)
> > +{
> > + struct nxp_c45_phy *priv = phydev->priv;
> > + u64 tx_delay = priv->tx_delay;
> > + u64 rx_delay = priv->rx_delay;
> > + u64 degree;
> > +
> > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > + degree = tx_delay / PS_PER_DEGREE;
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_TXID,
> > + ID_ENABLE |
> > nxp_c45_get_phase_shift(degree));
> > + }
> > +
> > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> > + degree = rx_delay / PS_PER_DEGREE;
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_RXID,
> > + ID_ENABLE |
> > nxp_c45_get_phase_shift(degree));
> > + }
> > +}
> > +
> > +static int nxp_c45_get_delays(struct phy_device *phydev)
> > +{
> > + struct nxp_c45_phy *priv = phydev->priv;
> > + int ret;
> > +
> > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > + ret = device_property_read_u32(&phydev->mdio.dev,
> > "tx-internal-delay-ps",
> > + &priv->tx_delay);
> > + if (ret) {
> > + phydev_err(phydev, "tx-internal-delay-ps
> > property missing\n");
> > + return ret;
> > + }
> > + ret = nxp_c45_check_delay(phydev, priv->tx_delay);
> > + if (ret) {
> > + phydev_err(phydev, "tx-internal-delay-ps
> > invalid value\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> > + ret = device_property_read_u32(&phydev->mdio.dev,
> > "rx-internal-delay-ps",
> > + &priv->rx_delay);
> > + if (ret) {
> > + phydev_err(phydev, "rx-internal-delay-ps
> > property missing\n");
> > + return ret;
> > + }
> > + ret = nxp_c45_check_delay(phydev, priv->rx_delay);
> > + if (ret) {
> > + phydev_err(phydev, "rx-internal-delay-ps
> > invalid value\n");
> > + return ret;
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_set_phy_mode(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ABILITIES);
> > + phydev_dbg(phydev, "Clause 45 managed PHY abilities
> > 0x%x\n", ret);
> > +
> > + switch (phydev->interface) {
> > + case PHY_INTERFACE_MODE_RGMII:
> > + if (!(ret & RGMII_ABILITY)) {
> > + phydev_err(phydev, "rgmii mode not
> > supported\n");
> > + return -EINVAL;
> > + }
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> > + break;
> > + case PHY_INTERFACE_MODE_RGMII_ID:
> > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > + if (!(ret & RGMII_ID_ABILITY)) {
> > + phydev_err(phydev, "rgmii-id, rgmii-txid,
> > rgmii-rxid modes are not supported\n");
> > + return -EINVAL;
> > + }
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> > + ret = nxp_c45_get_delays(phydev);
> > + if (ret)
> > + return ret;
> > +
> > + nxp_c45_set_delays(phydev);
> > + break;
> > + case PHY_INTERFACE_MODE_MII:
> > + if (!(ret & MII_ABILITY)) {
> > + phydev_err(phydev, "mii mode not
> > supported\n");
> > + return -EINVAL;
> > + }
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > MII_BASIC_CONFIG, MII_BASIC_CONFIG_MII);
> > + break;
> > + case PHY_INTERFACE_MODE_REVMII:
> > + if (!(ret & REVMII_ABILITY)) {
> > + phydev_err(phydev, "rev-mii mode not
> > supported\n");
> > + return -EINVAL;
> > + }
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > MII_BASIC_CONFIG, MII_BASIC_CONFIG_MII |
> > + MII_BASIC_CONFIG_REV);
> > + break;
> > + case PHY_INTERFACE_MODE_RMII:
> > + if (!(ret & RMII_ABILITY)) {
> > + phydev_err(phydev, "rmii mode not
> > supported\n");
> > + return -EINVAL;
> > + }
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > MII_BASIC_CONFIG, MII_BASIC_CONFIG_RMII);
> > + break;
> > + case PHY_INTERFACE_MODE_SGMII:
> > + if (!(ret & SGMII_ABILITY)) {
> > + phydev_err(phydev, "sgmii mode not
> > supported\n");
> > + return -EINVAL;
> > + }
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > MII_BASIC_CONFIG, MII_BASIC_CONFIG_SGMII);
> > + break;
> > + case PHY_INTERFACE_MODE_INTERNAL:
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_config_init(struct phy_device *phydev)
> > +{
> > + int ret;
> > +
> > + ret = nxp_c45_config_enable(phydev);
> > + if (ret) {
> > + phydev_err(phydev, "Failed to enable config\n");
> > + return ret;
> > + }
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, PHY_CONFIG);
> > + ret &= ~PHY_CONFIG_AUTO;
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, PHY_CONFIG, ret);
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, LINK_DROP_COUNTER,
> > COUNTERS_EN);
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, RX_PREAMBLE_COUNT,
> > COUNTERS_EN);
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, TX_PREAMBLE_COUNT,
> > COUNTERS_EN);
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, RX_IPG_LENGTH,
> > COUNTERS_EN);
> > + phy_write_mmd(phydev, MDIO_MMD_VEND1, TX_IPG_LENGTH,
> > COUNTERS_EN);
> > +
> > + ret = nxp_c45_set_phy_mode(phydev);
> > + if (ret)
> > + return ret;
> > +
> > + phydev->autoneg = AUTONEG_DISABLE;
> > +
> > + return nxp_c45_start_op(phydev);
> > +}
> > +
> > +static int nxp_c45_probe(struct phy_device *phydev)
> > +{
> > + struct nxp_c45_phy *priv;
> > +
> > + priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv),
> > GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + phydev->priv = priv;
> > +
> > + return 0;
> > +}
> > +
> > +static struct phy_driver nxp_c45_driver[] = {
> > + {
> > + PHY_ID_MATCH_MODEL(PHY_ID_BASE_T1),
> > + .name = "NXP C45 BASE-T1",
> > + .features = PHY_BASIC_T1_FEATURES,
> > + .probe = nxp_c45_probe,
> > + .soft_reset = nxp_c45_soft_reset,
> > + .config_aneg = nxp_c45_config_aneg,
> > + .config_init = nxp_c45_config_init,
> > + .read_status = nxp_c45_read_status,
> > + .suspend = nxp_c45_suspend,
> > + .resume = nxp_c45_resume,
> > + .get_sset_count = nxp_c45_get_sset_count,
> > + .get_strings = nxp_c45_get_strings,
> > + .get_stats = nxp_c45_get_stats,
> > + .cable_test_start = nxp_c45_cable_test_start,
> > + .cable_test_get_status =
> > nxp_c45_cable_test_get_status,
> > + .set_loopback = nxp_c45_set_loopback,
> > + .get_sqi = nxp_c45_get_sqi,
> > + .get_sqi_max = nxp_c45_get_sqi_max,
>
> How about interrupt support?
Unfortunately my setup is a bit limited and I can't test the
interrupts. However, I am planning to add interrupt support in a
further patch.
>
> > + },
> > +};
> > +
> > +module_phy_driver(nxp_c45_driver);
> > +
> > +static struct mdio_device_id __maybe_unused nxp_c45_tbl[] = {
> > + { PHY_ID_MATCH_MODEL(PHY_ID_BASE_T1) }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(mdio, nxp_c45_tbl);
> > +
> > +MODULE_AUTHOR("Radu Pirea <radu-nicolae.pirea@oss.nxp.com>");
> > +MODULE_DESCRIPTION("NXP C45 PHY driver");
> > +MODULE_LICENSE("GPL v2");
> >
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 18:41 [PATCH] phy: nxp-c45: add driver for tja1103 Radu Pirea (NXP OSS)
` (3 preceding siblings ...)
2021-04-11 2:33 ` kernel test robot
@ 2021-04-12 9:50 ` Russell King - ARM Linux admin
2021-04-13 13:44 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 18:04 ` Andrew Lunn
5 siblings, 1 reply; 21+ messages in thread
From: Russell King - ARM Linux admin @ 2021-04-12 9:50 UTC (permalink / raw)
To: Radu Pirea (NXP OSS)
Cc: andrew, hkallweit1, davem, kuba, netdev, linux-kernel
On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote:
> +#define B100T1_PMAPMD_CTL 0x0834
> +#define B100T1_PMAPMD_CONFIG_EN BIT(15)
> +#define B100T1_PMAPMD_MASTER BIT(14)
> +#define MASTER_MODE (B100T1_PMAPMD_CONFIG_EN | B100T1_PMAPMD_MASTER)
> +#define SLAVE_MODE (B100T1_PMAPMD_CONFIG_EN)
> +
> +#define DEVICE_CONTROL 0x0040
> +#define DEVICE_CONTROL_RESET BIT(15)
> +#define DEVICE_CONTROL_CONFIG_GLOBAL_EN BIT(14)
> +#define DEVICE_CONTROL_CONFIG_ALL_EN BIT(13)
> +#define RESET_POLL_NS (250 * NSEC_PER_MSEC)
> +
> +#define PHY_CONTROL 0x8100
> +#define PHY_CONFIG_EN BIT(14)
> +#define PHY_START_OP BIT(0)
> +
> +#define PHY_CONFIG 0x8108
> +#define PHY_CONFIG_AUTO BIT(0)
> +
> +#define SIGNAL_QUALITY 0x8320
> +#define SQI_VALID BIT(14)
> +#define SQI_MASK GENMASK(2, 0)
> +#define MAX_SQI SQI_MASK
> +
> +#define CABLE_TEST 0x8330
> +#define CABLE_TEST_ENABLE BIT(15)
> +#define CABLE_TEST_START BIT(14)
> +#define CABLE_TEST_VALID BIT(13)
> +#define CABLE_TEST_OK 0x00
> +#define CABLE_TEST_SHORTED 0x01
> +#define CABLE_TEST_OPEN 0x02
> +#define CABLE_TEST_UNKNOWN 0x07
> +
> +#define PORT_CONTROL 0x8040
> +#define PORT_CONTROL_EN BIT(14)
> +
> +#define PORT_INFRA_CONTROL 0xAC00
> +#define PORT_INFRA_CONTROL_EN BIT(14)
> +
> +#define VND1_RXID 0xAFCC
> +#define VND1_TXID 0xAFCD
> +#define ID_ENABLE BIT(15)
> +
> +#define ABILITIES 0xAFC4
> +#define RGMII_ID_ABILITY BIT(15)
> +#define RGMII_ABILITY BIT(14)
> +#define RMII_ABILITY BIT(10)
> +#define REVMII_ABILITY BIT(9)
> +#define MII_ABILITY BIT(8)
> +#define SGMII_ABILITY BIT(0)
> +
> +#define MII_BASIC_CONFIG 0xAFC6
> +#define MII_BASIC_CONFIG_REV BIT(8)
> +#define MII_BASIC_CONFIG_SGMII 0x9
> +#define MII_BASIC_CONFIG_RGMII 0x7
> +#define MII_BASIC_CONFIG_RMII 0x5
> +#define MII_BASIC_CONFIG_MII 0x4
> +
> +#define SYMBOL_ERROR_COUNTER 0x8350
> +#define LINK_DROP_COUNTER 0x8352
> +#define LINK_LOSSES_AND_FAILURES 0x8353
> +#define R_GOOD_FRAME_CNT 0xA950
> +#define R_BAD_FRAME_CNT 0xA952
> +#define R_RXER_FRAME_CNT 0xA954
> +#define RX_PREAMBLE_COUNT 0xAFCE
> +#define TX_PREAMBLE_COUNT 0xAFCF
> +#define RX_IPG_LENGTH 0xAFD0
> +#define TX_IPG_LENGTH 0xAFD1
> +#define COUNTERS_EN BIT(15)
> +
> +#define CLK_25MHZ_PS_PERIOD 40000UL
> +#define PS_PER_DEGREE (CLK_25MHZ_PS_PERIOD / 360)
> +#define MIN_ID_PS 8222U
> +#define MAX_ID_PS 11300U
Maybe include some prefix as to which MMD each of these registers is
located?
> +static bool nxp_c45_can_sleep(struct phy_device *phydev)
> +{
> + int reg;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);
> + if (reg < 0)
> + return false;
> +
> + return !!(reg & MDIO_STAT1_LPOWERABLE);
> +}
This looks like it could be useful as a generic helper function -
nothing in this function is specific to this PHY.
> +static int nxp_c45_resume(struct phy_device *phydev)
> +{
> + int reg;
> +
> + if (!nxp_c45_can_sleep(phydev))
> + return -EOPNOTSUPP;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> + reg &= ~MDIO_CTRL1_LPOWER;
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> +
> + return 0;
> +}
> +
> +static int nxp_c45_suspend(struct phy_device *phydev)
> +{
> + int reg;
> +
> + if (!nxp_c45_can_sleep(phydev))
> + return -EOPNOTSUPP;
> +
> + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> + reg |= MDIO_CTRL1_LPOWER;
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> +
> + return 0;
> +}
These too look like potential generic helper functions.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 19:36 ` Andrew Lunn
@ 2021-04-12 10:02 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 12:57 ` Andrew Lunn
0 siblings, 1 reply; 21+ messages in thread
From: Radu Nicolae Pirea (NXP OSS) @ 2021-04-12 10:02 UTC (permalink / raw)
To: Andrew Lunn; +Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
On Fri, 2021-04-09 at 21:36 +0200, Andrew Lunn wrote:
> On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote:
> > Add driver for tja1103 driver and for future NXP C45 PHYs.
>
> So apart from c45 vs c22, how does this differ to nxp-tja11xx.c?
> Do we really want two different drivers for the same hardware?
> Can we combine them somehow?
It looks like the PHYs are the same hardware, but that's not entirely
true. Just the naming is the same. TJA1103 is using a different IP and
is having timestamping support(I will add it later).
TJA is also not an Ethernet PHY series, but a general prefix for media
interfaces including also CAN, LIN, etc.
>
> > +config NXP_C45_PHY
> > + tristate "NXP C45 PHYs"
>
> This is also very vague. So in the future it will support PHYs other
> than the TJA series?
Yes, in the future this driver will support other PHYs too.
>
> Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-12 10:02 ` Radu Nicolae Pirea (NXP OSS)
@ 2021-04-12 12:57 ` Andrew Lunn
2021-04-12 14:11 ` Radu Nicolae Pirea (NXP OSS)
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Lunn @ 2021-04-12 12:57 UTC (permalink / raw)
To: Radu Nicolae Pirea (NXP OSS)
Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
On Mon, Apr 12, 2021 at 01:02:07PM +0300, Radu Nicolae Pirea (NXP OSS) wrote:
> On Fri, 2021-04-09 at 21:36 +0200, Andrew Lunn wrote:
> > On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote:
> > > Add driver for tja1103 driver and for future NXP C45 PHYs.
> >
> > So apart from c45 vs c22, how does this differ to nxp-tja11xx.c?
> > Do we really want two different drivers for the same hardware?
> > Can we combine them somehow?
> It looks like the PHYs are the same hardware, but that's not entirely
> true. Just the naming is the same. TJA1103 is using a different IP and
> is having timestamping support(I will add it later).
Is the IP very different? You often see different generations of a PHY
supported by the same driver, if the generations are similar.
Does it support C22 or it is purely a C45 device?
> TJA is also not an Ethernet PHY series, but a general prefix for media
> interfaces including also CAN, LIN, etc.
> >
> > > +config NXP_C45_PHY
> > > + tristate "NXP C45 PHYs"
> >
> > This is also very vague. So in the future it will support PHYs other
> > than the TJA series?
> Yes, in the future this driver will support other PHYs too.
Based on the same IP? Or different IP? Are we talking about 2 more
PHYs, so like the nxp-tja11xx.c will support 3 PHYs. And then the
tja1106 will come along and need a new driver? What will you call
that? I just don't like 'NXP C45 PHYs", it gives no clue as to what it
actually supports, and it gives you problems when you need to add yet
another driver.
At minimum, there needs to be a patch to add tja1102 to the help for
the nxp-tja11xx.c driver. And this driver needs to list tja1103.
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-12 12:57 ` Andrew Lunn
@ 2021-04-12 14:11 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 14:23 ` Andrew Lunn
0 siblings, 1 reply; 21+ messages in thread
From: Radu Nicolae Pirea (NXP OSS) @ 2021-04-12 14:11 UTC (permalink / raw)
To: Andrew Lunn; +Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
On Mon, 2021-04-12 at 14:57 +0200, Andrew Lunn wrote:
> On Mon, Apr 12, 2021 at 01:02:07PM +0300, Radu Nicolae Pirea (NXP
> OSS) wrote:
> > On Fri, 2021-04-09 at 21:36 +0200, Andrew Lunn wrote:
> > > On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS)
> > > wrote:
> > > > Add driver for tja1103 driver and for future NXP C45 PHYs.
> > >
> > > So apart from c45 vs c22, how does this differ to nxp-tja11xx.c?
> > > Do we really want two different drivers for the same hardware?
> > > Can we combine them somehow?
> > It looks like the PHYs are the same hardware, but that's not
> > entirely
> > true. Just the naming is the same. TJA1103 is using a different IP
> > and
> > is having timestamping support(I will add it later).
>
> Is the IP very different? You often see different generations of a
> PHY
> supported by the same driver, if the generations are similar.
Yes. It's very different. I know what you mean, but that's not the
case. That's why we decided to write a new driver from scratch.
>
> Does it support C22 or it is purely a C45 device?
It is purely a C45 device.
>
> > TJA is also not an Ethernet PHY series, but a general prefix for
> > media
> > interfaces including also CAN, LIN, etc.
> > >
> > > > +config NXP_C45_PHY
> > > > + tristate "NXP C45 PHYs"
> > >
> > > This is also very vague. So in the future it will support PHYs
> > > other
> > > than the TJA series?
> > Yes, in the future this driver will support other PHYs too.
>
> Based on the same IP?
> Or different IP? Are we talking about 2 more
> PHYs, so like the nxp-tja11xx.c will support 3 PHYs. And then the
> tja1106 will come along and need a new driver?
> What will you call
> that?
> I just don't like 'NXP C45 PHYs", it gives no clue as to what it
> actually supports, and it gives you problems when you need to add yet
> another driver.
Even if the PHY will be based on the same IP or not, if it is a C45
PHY, it will be supported by this driver. We are not talking about 2 or
3 PHYs. This driver will support all future C45 PHYs. That's why we
named it "NXP C45".
>
> At minimum, there needs to be a patch to add tja1102 to the help for
> the nxp-tja11xx.c driver. And this driver needs to list tja1103.
I will make the changes then.
Thank you.
>
> Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-12 14:11 ` Radu Nicolae Pirea (NXP OSS)
@ 2021-04-12 14:23 ` Andrew Lunn
2021-04-12 14:49 ` Radu Nicolae Pirea (NXP OSS)
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Lunn @ 2021-04-12 14:23 UTC (permalink / raw)
To: Radu Nicolae Pirea (NXP OSS)
Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
> It is purely a C45 device.
> Even if the PHY will be based on the same IP or not, if it is a C45
> PHY, it will be supported by this driver. We are not talking about 2 or
> 3 PHYs. This driver will support all future C45 PHYs. That's why we
> named it "NXP C45".
So if in future you produce C45 multi-gige PHYs, which have nothing in
common with the T1 automative PHY, it will still be in this driver?
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-12 14:23 ` Andrew Lunn
@ 2021-04-12 14:49 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 16:52 ` Andrew Lunn
0 siblings, 1 reply; 21+ messages in thread
From: Radu Nicolae Pirea (NXP OSS) @ 2021-04-12 14:49 UTC (permalink / raw)
To: Andrew Lunn; +Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
On Mon, 2021-04-12 at 16:23 +0200, Andrew Lunn wrote:
> > It is purely a C45 device.
>
> > Even if the PHY will be based on the same IP or not, if it is a C45
> > PHY, it will be supported by this driver. We are not talking about
> > 2 or
> > 3 PHYs. This driver will support all future C45 PHYs. That's why we
> > named it "NXP C45".
>
> So if in future you produce C45 multi-gige PHYs, which have nothing
> in
> common with the T1 automative PHY, it will still be in this driver?
Yes. C45 is robust and, if the PHY interface is standard, you can
support Base-T, Base-T1, and so on in the same register interface.
>
> Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-12 14:49 ` Radu Nicolae Pirea (NXP OSS)
@ 2021-04-12 16:52 ` Andrew Lunn
2021-04-13 6:56 ` Christian Herber
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Lunn @ 2021-04-12 16:52 UTC (permalink / raw)
To: Radu Nicolae Pirea (NXP OSS)
Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
On Mon, Apr 12, 2021 at 05:49:04PM +0300, Radu Nicolae Pirea (NXP OSS) wrote:
> On Mon, 2021-04-12 at 16:23 +0200, Andrew Lunn wrote:
> > > It is purely a C45 device.
> >
> > > Even if the PHY will be based on the same IP or not, if it is a C45
> > > PHY, it will be supported by this driver. We are not talking about
> > > 2 or
> > > 3 PHYs. This driver will support all future C45 PHYs. That's why we
> > > named it "NXP C45".
> >
> > So if in future you produce C45 multi-gige PHYs, which have nothing
> > in
> > common with the T1 automative PHY, it will still be in this driver?
> Yes. C45 is robust and, if the PHY interface is standard, you can
> support Base-T, Base-T1, and so on in the same register interface.
So what you are say is, you don't care if the IP is completely
different, it all goes in one driver. So lets put this driver into
nxp-tja11xx.c. And then we avoid all the naming issues.
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-09 18:41 [PATCH] phy: nxp-c45: add driver for tja1103 Radu Pirea (NXP OSS)
` (4 preceding siblings ...)
2021-04-12 9:50 ` Russell King - ARM Linux admin
@ 2021-04-12 18:04 ` Andrew Lunn
5 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2021-04-12 18:04 UTC (permalink / raw)
To: Radu Pirea (NXP OSS); +Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
> +static const struct nxp_c45_phy_stats nxp_c45_hw_stats[] = {
> + { "phy_symbol_error_cnt", MDIO_MMD_VEND1, SYMBOL_ERROR_COUNTER, 0, GENMASK(15, 0) },
> + { "phy_link_status_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 8, GENMASK(13, 8) },
> + { "phy_link_availability_drop_cnt", MDIO_MMD_VEND1, LINK_DROP_COUNTER, 0, GENMASK(5, 0) },
netdev tries to keep with the old 80 character limit. Please wrap the
long lines.
> +static void nxp_c45_set_delays(struct phy_device *phydev)
> +{
> + struct nxp_c45_phy *priv = phydev->priv;
> + u64 tx_delay = priv->tx_delay;
> + u64 rx_delay = priv->rx_delay;
> + u64 degree;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + degree = tx_delay / PS_PER_DEGREE;
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, VND1_TXID,
> + ID_ENABLE | nxp_c45_get_phase_shift(degree));
> + }
You are missing an else clause. You need to ensure the delay is 0 if
delays are not required. You have no idea what the bootloader has
done.
> +static int nxp_c45_get_delays(struct phy_device *phydev)
> +{
> + struct nxp_c45_phy *priv = phydev->priv;
> + int ret;
> +
> + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> + phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> + ret = device_property_read_u32(&phydev->mdio.dev, "tx-internal-delay-ps",
> + &priv->tx_delay);
> + if (ret) {
> + phydev_err(phydev, "tx-internal-delay-ps property missing\n");
This is not normally mandatory. Default to 2ns if not specified in DT.
> +static int nxp_c45_set_phy_mode(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, ABILITIES);
> + phydev_dbg(phydev, "Clause 45 managed PHY abilities 0x%x\n", ret);
> +
> + switch (phydev->interface) {
> + case PHY_INTERFACE_MODE_RGMII:
> + if (!(ret & RGMII_ABILITY)) {
> + phydev_err(phydev, "rgmii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> + break;
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + if (!(ret & RGMII_ID_ABILITY)) {
> + phydev_err(phydev, "rgmii-id, rgmii-txid, rgmii-rxid modes are not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_RGMII);
> + ret = nxp_c45_get_delays(phydev);
> + if (ret)
> + return ret;
> +
> + nxp_c45_set_delays(phydev);
> + break;
Again, for PHY_INTERFACE_MODE_RGMII you need to ensure the hardware is
not inserting a delay.
> + case PHY_INTERFACE_MODE_SGMII:
> + if (!(ret & SGMII_ABILITY)) {
> + phydev_err(phydev, "sgmii mode not supported\n");
> + return -EINVAL;
> + }
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, MII_BASIC_CONFIG, MII_BASIC_CONFIG_SGMII);
> + break;
Interested. What gets reported over the inband signalling?
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-12 16:52 ` Andrew Lunn
@ 2021-04-13 6:56 ` Christian Herber
2021-04-13 13:30 ` Andrew Lunn
0 siblings, 1 reply; 21+ messages in thread
From: Christian Herber @ 2021-04-13 6:56 UTC (permalink / raw)
To: Andrew Lunn, Radu-nicolae Pirea (OSS)
Cc: hkallweit1, linux, davem, kuba, netdev, linux-kernel
Hi Andrew,
On 4/12/2021 6:52 PM, Andrew Lunn wrote:
>
> So what you are say is, you don't care if the IP is completely
> different, it all goes in one driver. So lets put this driver into
> nxp-tja11xx.c. And then we avoid all the naming issues.
>
> Andrew
>
As this seems to be a key question, let me try and shed some more light
on this.
The original series of BASE-T1 PHYs includes TJA110, TJA1101, and
TJA1102. They are covered by the existing driver, which has the
unfortunate naming TJA11xx. Unfortunate, because the use of wildcards is
a bit to generous. E.g. the naming would also include a TJA1145, which
is a high-speed CAN transceiver. The truth is, extrapolating wildcards
in product names doesn't work as there is not guarantee of future
product names.
The mentioned TJA1100/1/2 are *fairly* software-compatible, which is why
it makes sense to have a shared driver. When it gets to TJA1103, there
is no SW compatibility, which is why we decided to create a new driver.
We want to support all future Ethernet PHY devices with this codebase,
and that is why the naming is that generic. The common denominator of
the devices is that they are NXP products and use clause 45 addressing.
When you say we don't care that the IP is different, that doesn't quite
fit. Just because the MDI is different, the register map does not need
to change much, so it will be easy to support future PHYs also when
using different PHY technology.
Moving the code into TJA11xx is creating more issues, as it assumes that
the devices which are managed by the driver are always TJA... devices
which may not be true.
Christian
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-13 6:56 ` Christian Herber
@ 2021-04-13 13:30 ` Andrew Lunn
2021-04-13 13:44 ` Christian Herber
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Lunn @ 2021-04-13 13:30 UTC (permalink / raw)
To: Christian Herber
Cc: Radu-nicolae Pirea (OSS),
hkallweit1, linux, davem, kuba, netdev, linux-kernel
On Tue, Apr 13, 2021 at 08:56:30AM +0200, Christian Herber wrote:
> Hi Andrew,
>
> On 4/12/2021 6:52 PM, Andrew Lunn wrote:
> >
> > So what you are say is, you don't care if the IP is completely
> > different, it all goes in one driver. So lets put this driver into
> > nxp-tja11xx.c. And then we avoid all the naming issues.
> >
> > Andrew
> >
>
> As this seems to be a key question, let me try and shed some more light on
> this.
> The original series of BASE-T1 PHYs includes TJA110, TJA1101, and TJA1102.
> They are covered by the existing driver, which has the unfortunate naming
> TJA11xx. Unfortunate, because the use of wildcards is a bit to generous.
Yes, that does happen.
Naming is difficult. But i really think nxp-c45.c is much worse. It
gives no idea at all what it supports. Or in the future, what it does
not support, and you actually need nxp-c45-ng.c.
Developers are going to look at a board, see a tja1XYZ chip, see the
nxp-tja11xx.c and enable it. Does the chip have a big C45 symbol on
it? Anything to give the idea it should use the nxp-c45 driver?
Maybe we should actually swing the complete opposite direction. Name
it npx-tja1103.c. There are lots of drivers which have a specific
name, but actually support a lot more devices. The developer sees they
have an tja1XYZ, there are two drivers which look about right, and
enable them both?
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-13 13:30 ` Andrew Lunn
@ 2021-04-13 13:44 ` Christian Herber
2021-04-13 13:57 ` Andrew Lunn
0 siblings, 1 reply; 21+ messages in thread
From: Christian Herber @ 2021-04-13 13:44 UTC (permalink / raw)
To: Andrew Lunn
Cc: Radu-nicolae Pirea (OSS),
hkallweit1, linux, davem, kuba, netdev, linux-kernel
On 4/13/2021 3:30 PM, Andrew Lunn wrote:
> On Tue, Apr 13, 2021 at 08:56:30AM +0200, Christian Herber wrote:
>> Hi Andrew,
>>
>> On 4/12/2021 6:52 PM, Andrew Lunn wrote:
>>>
>>> So what you are say is, you don't care if the IP is completely
>>> different, it all goes in one driver. So lets put this driver into
>>> nxp-tja11xx.c. And then we avoid all the naming issues.
>>>
>>> Andrew
>>>
>>
>> As this seems to be a key question, let me try and shed some more light on
>> this.
>> The original series of BASE-T1 PHYs includes TJA110, TJA1101, and TJA1102.
>> They are covered by the existing driver, which has the unfortunate naming
>> TJA11xx. Unfortunate, because the use of wildcards is a bit to generous.
>
> Yes, that does happen.
>
> Naming is difficult. But i really think nxp-c45.c is much worse. It
> gives no idea at all what it supports. Or in the future, what it does
> not support, and you actually need nxp-c45-ng.c.
>
> Developers are going to look at a board, see a tja1XYZ chip, see the
> nxp-tja11xx.c and enable it. Does the chip have a big C45 symbol on
> it? Anything to give the idea it should use the nxp-c45 driver?
>
> Maybe we should actually swing the complete opposite direction. Name
> it npx-tja1103.c. There are lots of drivers which have a specific
> name, but actually support a lot more devices. The developer sees they
> have an tja1XYZ, there are two drivers which look about right, and
> enable them both?
>
> Andrew
>
Ok, we can agree that there will not be a perfect naming. Would it be a
possibility to rename the existing TJA11xx driver to TJA1100-1-2 or is
that unwanted?
I agree that it should be easy to find the right driver. Right now,
there is another device called the SJA1110, which has a very similar IP
integrated. It would be possible to use the driver for that device also,
even if this is outside of the scope of this submission. Going for
wildcards, we get to xJA11xx, which is really undesirable to me.
In the end my hope was that people will look up the correct driver
through LKDDb or similar and will find the matching devices from there.
I am open to any suggestion that leads to users finding the right driver
and that also work for future devices.
Using your example of an NG device: My assumption is that the things
that change are covered by IEEE standardized registers, and thus should
be implemented as part of generic helper functions. The things that are
outside of the IEEE scope, e.g xMII interface configuration are generic
and can be contained in a single driver if the registers are kept
software compatible which we intend to do.
If nxp-c45.c is to generic (I take from your comments that' your
conclusion), we could at least lean towards nxp-c45-bt1.c?
Unfortunately, the product naming schemes are not sufficiently
methodical to have a a good driver name based on product names.
Christian
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-12 9:50 ` Russell King - ARM Linux admin
@ 2021-04-13 13:44 ` Radu Nicolae Pirea (NXP OSS)
0 siblings, 0 replies; 21+ messages in thread
From: Radu Nicolae Pirea (NXP OSS) @ 2021-04-13 13:44 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: andrew, hkallweit1, davem, kuba, netdev, linux-kernel
On Mon, 2021-04-12 at 10:50 +0100, Russell King - ARM Linux admin
wrote:
> On Fri, Apr 09, 2021 at 09:41:06PM +0300, Radu Pirea (NXP OSS) wrote:
> > +#define B100T1_PMAPMD_CTL 0x0834
> > +#define B100T1_PMAPMD_CONFIG_EN BIT(15)
> > +#define B100T1_PMAPMD_MASTER BIT(14)
> > +#define MASTER_MODE (B100T1_PMAPMD_CONFIG_EN |
> > B100T1_PMAPMD_MASTER)
> > +#define SLAVE_MODE (B100T1_PMAPMD_CONFIG_EN)
> > +
> > +#define DEVICE_CONTROL 0x0040
> > +#define DEVICE_CONTROL_RESET BIT(15)
> > +#define DEVICE_CONTROL_CONFIG_GLOBAL_EN BIT(14)
> > +#define DEVICE_CONTROL_CONFIG_ALL_EN BIT(13)
> > +#define RESET_POLL_NS (250 * NSEC_PER_MSEC)
> > +
> > +#define PHY_CONTROL 0x8100
> > +#define PHY_CONFIG_EN BIT(14)
> > +#define PHY_START_OP BIT(0)
> > +
> > +#define PHY_CONFIG 0x8108
> > +#define PHY_CONFIG_AUTO BIT(0)
> > +
> > +#define SIGNAL_QUALITY 0x8320
> > +#define SQI_VALID BIT(14)
> > +#define SQI_MASK GENMASK(2, 0)
> > +#define MAX_SQI SQI_MASK
> > +
> > +#define CABLE_TEST 0x8330
> > +#define CABLE_TEST_ENABLE BIT(15)
> > +#define CABLE_TEST_START BIT(14)
> > +#define CABLE_TEST_VALID BIT(13)
> > +#define CABLE_TEST_OK 0x00
> > +#define CABLE_TEST_SHORTED 0x01
> > +#define CABLE_TEST_OPEN 0x02
> > +#define CABLE_TEST_UNKNOWN 0x07
> > +
> > +#define PORT_CONTROL 0x8040
> > +#define PORT_CONTROL_EN BIT(14)
> > +
> > +#define PORT_INFRA_CONTROL 0xAC00
> > +#define PORT_INFRA_CONTROL_EN BIT(14)
> > +
> > +#define VND1_RXID 0xAFCC
> > +#define VND1_TXID 0xAFCD
> > +#define ID_ENABLE BIT(15)
> > +
> > +#define ABILITIES 0xAFC4
> > +#define RGMII_ID_ABILITY BIT(15)
> > +#define RGMII_ABILITY BIT(14)
> > +#define RMII_ABILITY BIT(10)
> > +#define REVMII_ABILITY BIT(9)
> > +#define MII_ABILITY BIT(8)
> > +#define SGMII_ABILITY BIT(0)
> > +
> > +#define MII_BASIC_CONFIG 0xAFC6
> > +#define MII_BASIC_CONFIG_REV BIT(8)
> > +#define MII_BASIC_CONFIG_SGMII 0x9
> > +#define MII_BASIC_CONFIG_RGMII 0x7
> > +#define MII_BASIC_CONFIG_RMII 0x5
> > +#define MII_BASIC_CONFIG_MII 0x4
> > +
> > +#define SYMBOL_ERROR_COUNTER 0x8350
> > +#define LINK_DROP_COUNTER 0x8352
> > +#define LINK_LOSSES_AND_FAILURES 0x8353
> > +#define R_GOOD_FRAME_CNT 0xA950
> > +#define R_BAD_FRAME_CNT 0xA952
> > +#define R_RXER_FRAME_CNT 0xA954
> > +#define RX_PREAMBLE_COUNT 0xAFCE
> > +#define TX_PREAMBLE_COUNT 0xAFCF
> > +#define RX_IPG_LENGTH 0xAFD0
> > +#define TX_IPG_LENGTH 0xAFD1
> > +#define COUNTERS_EN BIT(15)
> > +
> > +#define CLK_25MHZ_PS_PERIOD 40000UL
> > +#define PS_PER_DEGREE (CLK_25MHZ_PS_PERIOD / 360)
> > +#define MIN_ID_PS 8222U
> > +#define MAX_ID_PS 11300U
>
> Maybe include some prefix as to which MMD each of these registers is
> located?
I will add the MMD as prefix. Thank you.
>
> > +static bool nxp_c45_can_sleep(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_STAT1);
> > + if (reg < 0)
> > + return false;
> > +
> > + return !!(reg & MDIO_STAT1_LPOWERABLE);
> > +}
>
> This looks like it could be useful as a generic helper function -
> nothing in this function is specific to this PHY.
>
> > +static int nxp_c45_resume(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + if (!nxp_c45_can_sleep(phydev))
> > + return -EOPNOTSUPP;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> > + reg &= ~MDIO_CTRL1_LPOWER;
> > + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> > +
> > + return 0;
> > +}
> > +
> > +static int nxp_c45_suspend(struct phy_device *phydev)
> > +{
> > + int reg;
> > +
> > + if (!nxp_c45_can_sleep(phydev))
> > + return -EOPNOTSUPP;
> > +
> > + reg = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
> > + reg |= MDIO_CTRL1_LPOWER;
> > + phy_write_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1, reg);
> > +
> > + return 0;
> > +}
>
> These too look like potential generic helper functions.
That's true.
Should I implement them as genphy_c45_pma_suspend/resume? Given that we
can also have PCS suspend/resume too.
However, in my case, PMA low power bit will enable low power for PCS as
well.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-13 13:44 ` Christian Herber
@ 2021-04-13 13:57 ` Andrew Lunn
2021-04-13 14:02 ` Christian Herber
0 siblings, 1 reply; 21+ messages in thread
From: Andrew Lunn @ 2021-04-13 13:57 UTC (permalink / raw)
To: Christian Herber
Cc: Radu-nicolae Pirea (OSS),
hkallweit1, linux, davem, kuba, netdev, linux-kernel
> Ok, we can agree that there will not be a perfect naming. Would it be a
> possibility to rename the existing TJA11xx driver to TJA1100-1-2 or is that
> unwanted?
It is generally a bad idea. It makes back porting fixing harder if the
file changes name.
> If nxp-c45.c is to generic (I take from your comments that' your
> conclusion), we could at least lean towards nxp-c45-bt1.c? Unfortunately,
> the product naming schemes are not sufficiently methodical to have a a good
> driver name based on product names.
And what does bt1 stand for?
How about nxp-c45-tja11xx.c. It is not ideal, but it does at least
give an indication of what devices it does cover, even if there is a
big overlap with nxp-tja11xx.c, in terms of pattern matching. And if
you do decide to have a major change of registers, your can call the
device tja1201 and have a new driver nxp-c45-tja12xx.
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-13 13:57 ` Andrew Lunn
@ 2021-04-13 14:02 ` Christian Herber
2021-04-13 14:04 ` Andrew Lunn
0 siblings, 1 reply; 21+ messages in thread
From: Christian Herber @ 2021-04-13 14:02 UTC (permalink / raw)
To: Andrew Lunn
Cc: Radu-nicolae Pirea (OSS),
hkallweit1, linux, davem, kuba, netdev, linux-kernel
On 4/13/2021 3:57 PM, Andrew Lunn wrote:
>> Ok, we can agree that there will not be a perfect naming. Would it be a
>> possibility to rename the existing TJA11xx driver to TJA1100-1-2 or is that
>> unwanted?
>
> It is generally a bad idea. It makes back porting fixing harder if the
> file changes name.
>
>> If nxp-c45.c is to generic (I take from your comments that' your
>> conclusion), we could at least lean towards nxp-c45-bt1.c? Unfortunately,
>> the product naming schemes are not sufficiently methodical to have a a good
>> driver name based on product names.
>
> And what does bt1 stand for?
>
> How about nxp-c45-tja11xx.c. It is not ideal, but it does at least
> give an indication of what devices it does cover, even if there is a
> big overlap with nxp-tja11xx.c, in terms of pattern matching. And if
> you do decide to have a major change of registers, your can call the
> device tja1201 and have a new driver nxp-c45-tja12xx.
>
> Andrew
>
bt1 standing for BASE-T1.
As you can see from the current situation, it could well happen that a
future PHY is SW incompatible (right now I would say it is unlikely, but
ok), and the device is still a TJA11xx.
nxp-c45-tja11xx is acceptable from my point of view.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] phy: nxp-c45: add driver for tja1103
2021-04-13 14:02 ` Christian Herber
@ 2021-04-13 14:04 ` Andrew Lunn
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2021-04-13 14:04 UTC (permalink / raw)
To: Christian Herber
Cc: Radu-nicolae Pirea (OSS),
hkallweit1, linux, davem, kuba, netdev, linux-kernel
> nxp-c45-tja11xx is acceptable from my point of view.
Great. Enough bike shedding, nxp-c45-tja11xx it is.
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-04-13 14:05 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09 18:41 [PATCH] phy: nxp-c45: add driver for tja1103 Radu Pirea (NXP OSS)
2021-04-09 19:18 ` Heiner Kallweit
2021-04-12 9:10 ` Radu Nicolae Pirea (NXP OSS)
2021-04-09 19:31 ` Jakub Kicinski
2021-04-09 19:36 ` Andrew Lunn
2021-04-12 10:02 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 12:57 ` Andrew Lunn
2021-04-12 14:11 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 14:23 ` Andrew Lunn
2021-04-12 14:49 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 16:52 ` Andrew Lunn
2021-04-13 6:56 ` Christian Herber
2021-04-13 13:30 ` Andrew Lunn
2021-04-13 13:44 ` Christian Herber
2021-04-13 13:57 ` Andrew Lunn
2021-04-13 14:02 ` Christian Herber
2021-04-13 14:04 ` Andrew Lunn
2021-04-11 2:33 ` kernel test robot
2021-04-12 9:50 ` Russell King - ARM Linux admin
2021-04-13 13:44 ` Radu Nicolae Pirea (NXP OSS)
2021-04-12 18:04 ` Andrew Lunn
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).