| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 11 Apr 2023 18:49:42 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org
Cc: Andrew Lunn <andrew@...n.ch>, Russell King <linux@...linux.org.uk>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next] net: phy: add basic driver for NXP CBTX PHY
On 11.04.2023 17:57, Vladimir Oltean wrote:
> The CBTX PHY is a Fast Ethernet PHY integrated into the SJA1110 A/B/C
> automotive Ethernet switches.
>
> It was hoped it would work with the Generic PHY driver, but alas, it
> doesn't. The most important reason why is that the PHY is powered down
> by default, and it needs a vendor register to power it on.
>
> It has a linear memory map that is accessed over SPI by the SJA1110
> switch driver, which exposes a fake MDIO controller. It has the
> following (and only the following) standard clause 22 registers:
>
> 0x0: MII_BMCR
> 0x1: MII_BMSR
> 0x2: MII_PHYSID1
> 0x3: MII_PHYSID2
> 0x4: MII_ADVERTISE
> 0x5: MII_LPA
> 0x6: MII_EXPANSION
> 0x7: the missing MII_NPAGE for Next Page Transmit Register
>
> Every other register is vendor-defined.
>
> The register map expands the standard clause 22 5-bit address space of
> 0x20 registers, however the driver does not need to access the extra
> registers for now (and hopefully never). If it ever needs to do that, it
> is possible to implement a fake (software) page switching mechanism
> between the PHY driver and the SJA1110 MDIO controller driver.
>
> Also, Auto-MDIX is turned off by default in hardware, the driver turns
> it on by default and reports the current status. I've tested this with a
> VSC8514 link partner and a crossover cable, by forcing the mode on the
> link partner, and seeing that the CBTX PHY always sees the reverse of
> the mode forced on the VSC8514 (and that traffic works). The link
> doesn't come up (as expected) if MDI modes are forced on both ends in
> the same way (with the cross-over cable, that is).
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> drivers/net/phy/Kconfig | 6 +
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/nxp-cbtx.c | 251 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 258 insertions(+)
> create mode 100644 drivers/net/phy/nxp-cbtx.c
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 6b9525def973..eae6fc697ba3 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -265,6 +265,12 @@ config NATIONAL_PHY
> help
> Currently supports the DP83865 PHY.
>
> +config NXP_CBTX_PHY
> + tristate "NXP 100BASE-TX PHYs"
> + help
> + Support the 100BASE-TX PHY integrated on the SJA1110 automotive
> + switch family.
> +
> config NXP_C45_TJA11XX_PHY
> tristate "NXP C45 TJA11XX PHYs"
> depends on PTP_1588_CLOCK_OPTIONAL
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index b5138066ba04..ae11bf20b46e 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_MOTORCOMM_PHY) += motorcomm.o
> obj-$(CONFIG_NATIONAL_PHY) += national.o
> obj-$(CONFIG_NCN26000_PHY) += ncn26000.o
> obj-$(CONFIG_NXP_C45_TJA11XX_PHY) += nxp-c45-tja11xx.o
> +obj-$(CONFIG_NXP_CBTX_PHY) += nxp-cbtx.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-cbtx.c b/drivers/net/phy/nxp-cbtx.c
> new file mode 100644
> index 000000000000..936761ec516e
> --- /dev/null
> +++ b/drivers/net/phy/nxp-cbtx.c
> @@ -0,0 +1,251 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Driver for 100BASE-TX PHY embedded into NXP SJA1110 switch
> + *
> + * Copyright 2022-2023 NXP
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/mii.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +
> +#define PHY_ID_CBTX_SJA1110 0x001bb020
> +
> +/* Registers */
> +#define CBTX_MODE_CTRL_STAT 0x11
> +#define CBTX_PDOWN_CTRL 0x18
> +#define CBTX_RX_ERR_COUNTER 0x1a
> +#define CBTX_IRQ_STAT 0x1d
> +#define CBTX_IRQ_ENABLE 0x1e
> +
> +/* Fields */
> +#define CBTX_MODE_CTRL_STAT_AUTO_MDIX_EN BIT(7)
> +#define CBTX_MODE_CTRL_STAT_MDIX_MODE BIT(6)
> +
> +#define CBTX_PDOWN_CTL_TRUE_PDOWN BIT(0)
> +
> +#define CBTX_IRQ_ENERGYON BIT(7)
> +#define CBTX_IRQ_AN_COMPLETE BIT(6)
> +#define CBTX_IRQ_REM_FAULT BIT(5)
> +#define CBTX_IRQ_LINK_DOWN BIT(4)
> +#define CBTX_IRQ_AN_LP_ACK BIT(3)
> +#define CBTX_IRQ_PARALLEL_DETECT_FAULT BIT(2)
> +#define CBTX_IRQ_AN_PAGE_RECV BIT(1)
> +
> +static int cbtx_soft_reset(struct phy_device *phydev)
> +{
> + int ret;
> +
> + /* Can't soft reset unless we remove PHY from true power down mode */
> + ret = phy_clear_bits(phydev, CBTX_PDOWN_CTRL,
> + CBTX_PDOWN_CTL_TRUE_PDOWN);
> + if (ret)
> + return ret;
> +
> + return genphy_soft_reset(phydev);
> +}
> +
> +static int cbtx_config_init(struct phy_device *phydev)
> +{
> + /* Wait for cbtx_config_aneg() to kick in and apply this */
> + phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> +
> + return 0;
> +}
> +
> +static int cbtx_suspend(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = genphy_suspend(phydev);
> + if (ret)
> + return ret;
> +
> + return phy_set_bits(phydev, CBTX_PDOWN_CTRL,
> + CBTX_PDOWN_CTL_TRUE_PDOWN);
A comment may be helpful explaining how true_pdown mode
is different from power-down mode set by C22 standard
bit in BMCR as part of genphy_suspend().
> +}
> +
> +static int cbtx_resume(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_clear_bits(phydev, CBTX_PDOWN_CTRL,
> + CBTX_PDOWN_CTL_TRUE_PDOWN);
> + if (ret)
> + return ret;
> +
> + return genphy_resume(phydev);
> +}
> +
> +static int cbtx_mdix_status(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = phy_read(phydev, CBTX_MODE_CTRL_STAT);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & CBTX_MODE_CTRL_STAT_MDIX_MODE)
> + phydev->mdix = ETH_TP_MDI_X;
> + else
> + phydev->mdix = ETH_TP_MDI;
> +
> + return 0;
> +}
> +
> +static int cbtx_read_status(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = cbtx_mdix_status(phydev);
> + if (ret)
> + return ret;
> +
> + return genphy_read_status(phydev);
> +}
> +
> +static int cbtx_mdix_config(struct phy_device *phydev)
> +{
> + int ret;
> +
> + switch (phydev->mdix_ctrl) {
> + case ETH_TP_MDI_AUTO:
> + return phy_set_bits(phydev, CBTX_MODE_CTRL_STAT,
> + CBTX_MODE_CTRL_STAT_AUTO_MDIX_EN);
> + case ETH_TP_MDI:
> + ret = phy_clear_bits(phydev, CBTX_MODE_CTRL_STAT,
> + CBTX_MODE_CTRL_STAT_AUTO_MDIX_EN);
> + if (ret)
> + return ret;
> +
> + return phy_clear_bits(phydev, CBTX_MODE_CTRL_STAT,
> + CBTX_MODE_CTRL_STAT_MDIX_MODE);
> + case ETH_TP_MDI_X:
> + ret = phy_clear_bits(phydev, CBTX_MODE_CTRL_STAT,
> + CBTX_MODE_CTRL_STAT_AUTO_MDIX_EN);
> + if (ret)
> + return ret;
> +
> + return phy_set_bits(phydev, CBTX_MODE_CTRL_STAT,
> + CBTX_MODE_CTRL_STAT_MDIX_MODE);
> + }
> +
> + return 0;
> +}
> +
> +static int cbtx_config_aneg(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = cbtx_mdix_config(phydev);
> + if (ret)
> + return ret;
> +
> + return genphy_config_aneg(phydev);
> +}
> +
> +static int cbtx_ack_interrupts(struct phy_device *phydev)
> +{
> + return phy_read(phydev, CBTX_IRQ_STAT);
> +}
> +
> +static int cbtx_config_intr(struct phy_device *phydev)
> +{
> + int ret;
> +
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
> + ret = cbtx_ack_interrupts(phydev);
> + if (ret < 0)
> + return ret;
> +
> + ret = phy_write(phydev, CBTX_IRQ_ENABLE,
> + CBTX_IRQ_AN_COMPLETE | CBTX_IRQ_LINK_DOWN);
I think you need also CBTX_IRQ_ENERGYON. Otherwise you won't get a
link-up interrupt in forced mode.
> + if (ret)
> + return ret;
> + } else {
> + ret = phy_write(phydev, CBTX_IRQ_ENABLE, 0);
> + if (ret)
> + return ret;
> +
> + ret = cbtx_ack_interrupts(phydev);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static irqreturn_t cbtx_handle_interrupt(struct phy_device *phydev)
> +{
> + int irq_stat, irq_enabled;
> +
> + irq_stat = cbtx_ack_interrupts(phydev);
> + if (irq_stat < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + irq_enabled = phy_read(phydev, CBTX_IRQ_ENABLE);
> + if (irq_enabled < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + if (!(irq_enabled & irq_stat))
> + return IRQ_NONE;
> +
> + phy_trigger_machine(phydev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int cbtx_get_sset_count(struct phy_device *phydev)
> +{
> + return 1;
> +}
> +
> +static void cbtx_get_strings(struct phy_device *phydev, u8 *data)
> +{
> + strncpy(data, "100btx_rx_err", ETH_GSTRING_LEN);
> +}
> +
> +static void cbtx_get_stats(struct phy_device *phydev,
> + struct ethtool_stats *stats, u64 *data)
> +{
> + int ret;
> +
> + ret = phy_read(phydev, CBTX_RX_ERR_COUNTER);
> + data[0] = (ret < 0) ? U64_MAX : ret;
> +}
> +
> +static struct phy_driver cbtx_driver[] = {
> + {
> + PHY_ID_MATCH_MODEL(PHY_ID_CBTX_SJA1110),
> + .name = "NXP CBTX (SJA1110)",
> + /* PHY_BASIC_FEATURES */
> + .soft_reset = cbtx_soft_reset,
> + .config_init = cbtx_config_init,
> + .suspend = cbtx_suspend,
> + .resume = cbtx_resume,
> + .config_intr = cbtx_config_intr,
> + .handle_interrupt = cbtx_handle_interrupt,
> + .read_status = cbtx_read_status,
> + .config_aneg = cbtx_config_aneg,
> + .get_sset_count = cbtx_get_sset_count,
> + .get_strings = cbtx_get_strings,
> + .get_stats = cbtx_get_stats,
> + },
> +};
> +
> +module_phy_driver(cbtx_driver);
> +
> +static struct mdio_device_id __maybe_unused cbtx_tbl[] = {
> + { PHY_ID_MATCH_MODEL(PHY_ID_CBTX_SJA1110) },
> + { },
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, cbtx_tbl);
> +
> +MODULE_AUTHOR("Vladimir Oltean <vladimir.oltean@....com>");
> +MODULE_DESCRIPTION("NXP CBTX PHY driver");
> +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists