Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 Aug 2022 17:45:13 +0100
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Maxime Chevallier <maxime.chevallier@bootlin.com>
Cc:     davem@davemloft.net, Rob Herring <robh+dt@kernel.org>,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        thomas.petazzoni@bootlin.com, Andrew Lunn <andrew@lunn.ch>,
        Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org
Subject: Re: [PATCH net-next 3/5] net: pcs: add new PCS driver for altera TSE
 PCS
Message-ID: <Ywj4mQDyLzwbvxt8@shell.armlinux.org.uk>
References: <20220826135451.526756-1-maxime.chevallier@bootlin.com>
 <20220826135451.526756-4-maxime.chevallier@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220826135451.526756-4-maxime.chevallier@bootlin.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

On Fri, Aug 26, 2022 at 03:54:49PM +0200, Maxime Chevallier wrote:
> +
> +/* 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))
> +#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)
> +#define   PCS_IF_MODE_SGMI_HALF_DUPLEX	BIT(4)
> +#define   PCS_IF_MODE_SGMI_PHY_AN	BIT(5)

This looks very similar to pcs-lynx's register layout. I wonder if it's
the same underlying hardware.

> +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;
> +
> +	if (interface != PHY_INTERFACE_MODE_SGMII &&
> +	    interface != PHY_INTERFACE_MODE_1000BASEX)
> +		return 0;

I would suggest doing this check in .pcs_validate() to catch anyone
attaching the PCS with an unsupported interface mode.

> +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);
> +
> +	tse_pcs_reset(tse_pcs);

Any ideas why a reset is necessary after setting BMCR_ANRESTART?
Normally, this is not required.

> diff --git a/include/linux/pcs-altera-tse.h b/include/linux/pcs-altera-tse.h
> new file mode 100644
> index 000000000000..9c85e7c8ef70
> --- /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@bootlin.com>
> + */
> +
> +#ifndef __LINUX_PCS_ALTERA_TSE_H
> +#define __LINUX_PCS_ALTERA_TSE_H
> +
> +struct phylink;

Don't you want "struct phylink_pcs;" here?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!