| 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
| ||
|
Message-ID: <68b3dfbf-9bab-2554-254e-bffd280ba97e@gmail.com>
Date: Sun, 9 Oct 2022 01:38:15 -0400
From: Sean Anderson <seanga2@...il.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>,
davem@...emloft.net, Rob Herring <robh+dt@...nel.org>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
thomas.petazzoni@...tlin.com, Andrew Lunn <andrew@...n.ch>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>,
linux-arm-kernel@...ts.infradead.org,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v3 3/5] net: pcs: add new PCS driver for altera
TSE PCS
On 9/1/22 10:35, Maxime Chevallier wrote:
> The Altera Triple Speed Ethernet has a SGMII/1000BaseC PCS that can be
> integrated in several ways. It can either be part of the TSE MAC's
> address space, accessed through 32 bits accesses on the mapped mdio
> device 0, or through a dedicated 16 bits register set.
>
> This driver allows using the TSE PCS outside of altera TSE's driver,
> since it can be used standalone by other MACs.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
> ---
> V2->V3 :
> - No changes
> V1->V2 :
> - Added a pcs_validate() callback to filter interface modes
> - Added comments on the need for a soft reset at an_restart
>
> MAINTAINERS | 7 ++
> drivers/net/pcs/Kconfig | 6 ++
> drivers/net/pcs/Makefile | 1 +
> drivers/net/pcs/pcs-altera-tse.c | 171 +++++++++++++++++++++++++++++++
> include/linux/pcs-altera-tse.h | 17 +++
> 5 files changed, 202 insertions(+)
> create mode 100644 drivers/net/pcs/pcs-altera-tse.c
> create mode 100644 include/linux/pcs-altera-tse.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fe484e7d36dc..576c01576a5f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -878,6 +878,13 @@ L: netdev@...r.kernel.org
> S: Maintained
> F: drivers/net/ethernet/altera/
>
> +ALTERA TSE PCS
> +M: Maxime Chevallier <maxime.chevallier@...tlin.com>
> +L: netdev@...r.kernel.org
> +S: Supported
> +F: drivers/net/pcs/pcs-altera-tse.c
> +F: include/linux/pcs-altera-tse.h
> +
> ALTERA UART/JTAG UART SERIAL DRIVERS
> M: Tobias Klauser <tklauser@...tanz.ch>
> L: linux-serial@...r.kernel.org
> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> index 6289b7c765f1..6e7e6c346a3e 100644
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
> @@ -26,4 +26,10 @@ config PCS_RZN1_MIIC
> on RZ/N1 SoCs. This PCS converts MII to RMII/RGMII or can be set in
> pass-through mode for MII.
>
> +config PCS_ALTERA_TSE
> + tristate
> + help
> + This module provides helper functions for the Altera Triple Speed
> + Ethernet SGMII PCS, that can be found on the Intel Socfpga family.
> +
> endmenu
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 0ff5388fcdea..4c780d8f2e98 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -6,3 +6,4 @@ pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-nxp.o
> obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o
> obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o
> obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o
> +obj-$(CONFIG_PCS_ALTERA_TSE) += pcs-altera-tse.o
> diff --git a/drivers/net/pcs/pcs-altera-tse.c b/drivers/net/pcs/pcs-altera-tse.c
> new file mode 100644
> index 000000000000..ae7688ffc4d7
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-altera-tse.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Bootlin
> + *
> + * Maxime Chevallier <maxime.chevallier@...tlin.com>
> + */
> +
> +#include <linux/netdevice.h>
> +#include <linux/phy.h>
> +#include <linux/phylink.h>
> +#include <linux/pcs-altera-tse.h>
> +
> +/* SGMII PCS register addresses
> + */
> +#define SGMII_PCS_SCRATCH 0x10
> +#define SGMII_PCS_REV 0x11
> +#define SGMII_PCS_LINK_TIMER_0 0x12
> +#define SGMII_PCS_LINK_TIMER_REG(x) (0x12 + (x))
Not used.
> +#define SGMII_PCS_LINK_TIMER_1 0x13
> +#define SGMII_PCS_IF_MODE 0x14
> +#define PCS_IF_MODE_SGMII_ENA BIT(0)
> +#define PCS_IF_MODE_USE_SGMII_AN BIT(1)
> +#define PCS_IF_MODE_SGMI_SPEED_MASK GENMASK(3, 2)
> +#define PCS_IF_MODE_SGMI_SPEED_10 (0 << 2)
> +#define PCS_IF_MODE_SGMI_SPEED_100 (1 << 2)
> +#define PCS_IF_MODE_SGMI_SPEED_1000 (2 << 2)
You can use FIELD_PREP if you're so inclined. I assume SGMI is from the datasheet.
> +#define PCS_IF_MODE_SGMI_HALF_DUPLEX BIT(4)
> +#define PCS_IF_MODE_SGMI_PHY_AN BIT(5)
> +#define SGMII_PCS_DIS_READ_TO 0x15
> +#define SGMII_PCS_READ_TO 0x16
> +#define SGMII_PCS_SW_RESET_TIMEOUT 100 /* usecs */
> +
> +struct altera_tse_pcs {
> + struct phylink_pcs pcs;
> + void __iomem *base;
> + int reg_width;
> +};
> +
> +static struct altera_tse_pcs *phylink_pcs_to_tse_pcs(struct phylink_pcs *pcs)
> +{
> + return container_of(pcs, struct altera_tse_pcs, pcs);
> +}
> +
> +static u16 tse_pcs_read(struct altera_tse_pcs *tse_pcs, int regnum)
> +{
> + if (tse_pcs->reg_width == 4)
> + return readl(tse_pcs->base + regnum * 4);
> + else
> + return readw(tse_pcs->base + regnum * 2);
> +}
> +
> +static void tse_pcs_write(struct altera_tse_pcs *tse_pcs, int regnum,
> + u16 value)
> +{
> + if (tse_pcs->reg_width == 4)
> + writel(value, tse_pcs->base + regnum * 4);
> + else
> + writew(value, tse_pcs->base + regnum * 2);
> +}
> +
> +static int tse_pcs_reset(struct altera_tse_pcs *tse_pcs)
> +{
> + int i = 0;
> + u16 bmcr;
> +
> + /* Reset PCS block */
> + bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
> + bmcr |= BMCR_RESET;
> + tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
> +
> + for (i = 0; i < SGMII_PCS_SW_RESET_TIMEOUT; i++) {
> + if (!(tse_pcs_read(tse_pcs, MII_BMCR) & BMCR_RESET))
> + return 0;
> + udelay(1);
> + }
read_poll_timeout?
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int alt_tse_pcs_validate(struct phylink_pcs *pcs,
> + unsigned long *supported,
> + const struct phylink_link_state *state)
> +{
> + if (state->interface == PHY_INTERFACE_MODE_SGMII ||
> + state->interface == PHY_INTERFACE_MODE_1000BASEX)
> + return 1;
> +
> + return -EINVAL;
> +}
> +
> +static int alt_tse_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
> + struct altera_tse_pcs *tse_pcs = phylink_pcs_to_tse_pcs(pcs);
> + u32 ctrl, if_mode;
> +
> + ctrl = tse_pcs_read(tse_pcs, MII_BMCR);
> + if_mode = tse_pcs_read(tse_pcs, SGMII_PCS_IF_MODE);
> +
> + /* Set link timer to 1.6ms, as per the MegaCore Function User Guide */
> + tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_0, 0x0D40);
> + tse_pcs_write(tse_pcs, SGMII_PCS_LINK_TIMER_1, 0x03);
Shouldn't this be different for SGMII vs 1000BASE-X?
> +
> + if (interface == PHY_INTERFACE_MODE_SGMII) {
> + if_mode |= PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA;
I think PCS_IF_MODE_USE_SGMII_AN should be cleared if mode=MLO_AN_FIXED.
> + } else if (interface == PHY_INTERFACE_MODE_1000BASEX) {
> + if_mode &= ~(PCS_IF_MODE_USE_SGMII_AN | PCS_IF_MODE_SGMII_ENA);
> + if_mode |= PCS_IF_MODE_SGMI_SPEED_1000;
I don't think you need to set this for 1000BASE-X.
> + }
> +
> + ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);
BMCR_FULLDPLX is read-only, so you don't have to set it. Same for the
speed.
> +
> + tse_pcs_write(tse_pcs, MII_BMCR, ctrl);
> + tse_pcs_write(tse_pcs, SGMII_PCS_IF_MODE, if_mode);
> +
> + return tse_pcs_reset(tse_pcs);
> +}
> +
> +static void alt_tse_pcs_get_state(struct phylink_pcs *pcs,
> + struct phylink_link_state *state)
> +{
> + struct altera_tse_pcs *tse_pcs = phylink_pcs_to_tse_pcs(pcs);
> + u16 bmsr, lpa;
> +
> + bmsr = tse_pcs_read(tse_pcs, MII_BMSR);
> + lpa = tse_pcs_read(tse_pcs, MII_LPA);
> +
> + phylink_mii_c22_pcs_decode_state(state, bmsr, lpa);
> +}
> +
> +static void alt_tse_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> + struct altera_tse_pcs *tse_pcs = phylink_pcs_to_tse_pcs(pcs);
> + u16 bmcr;
> +
> + bmcr = tse_pcs_read(tse_pcs, MII_BMCR);
> + bmcr |= BMCR_ANRESTART;
> + tse_pcs_write(tse_pcs, MII_BMCR, bmcr);
> +
> + /* This PCS seems to require a soft reset to re-sync the AN logic */
> + tse_pcs_reset(tse_pcs);
This is kinda strange since c22 phys are supposed to reset the other
registers to default values when BMCR_RESET is written. Good thing this
is a PCS...
> +}
> +
> +static const struct phylink_pcs_ops alt_tse_pcs_ops = {
> + .pcs_validate = alt_tse_pcs_validate,
> + .pcs_get_state = alt_tse_pcs_get_state,
> + .pcs_config = alt_tse_pcs_config,
> + .pcs_an_restart = alt_tse_pcs_an_restart,
> +};
Don't you need link_up to set the speed/duplex for MLO_AN_FIXED?
> +
> +struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
> + void __iomem *pcs_base, int reg_width)
> +{
> + struct altera_tse_pcs *tse_pcs;
> +
> + if (reg_width != 4 && reg_width != 2)
> + return ERR_PTR(-EINVAL);
> +
> + tse_pcs = devm_kzalloc(&ndev->dev, sizeof(*tse_pcs), GFP_KERNEL);
> + if (!tse_pcs)
> + return ERR_PTR(-ENOMEM);
> +
> + tse_pcs->pcs.ops = &alt_tse_pcs_ops;
> + tse_pcs->base = pcs_base;
> + tse_pcs->reg_width = reg_width;
> +
> + return &tse_pcs->pcs;
> +}
> +EXPORT_SYMBOL_GPL(alt_tse_pcs_create);
> diff --git a/include/linux/pcs-altera-tse.h b/include/linux/pcs-altera-tse.h
> new file mode 100644
> index 000000000000..92ab9f08e835
> --- /dev/null
> +++ b/include/linux/pcs-altera-tse.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2022 Bootlin
> + *
> + * Maxime Chevallier <maxime.chevallier@...tlin.com>
> + */
> +
> +#ifndef __LINUX_PCS_ALTERA_TSE_H
> +#define __LINUX_PCS_ALTERA_TSE_H
> +
> +struct phylink_pcs;
> +struct net_device;
> +
> +struct phylink_pcs *alt_tse_pcs_create(struct net_device *ndev,
> + void __iomem *pcs_base, int reg_width);
> +
> +#endif /* __LINUX_PCS_ALTERA_TSE_H */
--Sean
Powered by blists - more mailing lists