On Tue, Jun 23, 2020 at 12:53:40AM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jun 16, 2020 at 10:31:39AM +0200, Sascha Hauer wrote:
> > The MVNETA_SERDES_CFG register is only available on older SoCs like the
> > Armada XP. On newer SoCs like the Armada 38x the fields are moved to
> > comphy. This patch moves the writes to this register next to the comphy
> > initialization, so that depending on the SoC either comphy or
> > MVNETA_SERDES_CFG is configured.
> > With this we no longer write to the MVNETA_SERDES_CFG on SoCs where it
> > doesn't exist.
> >
> > Suggested-by: Russell King <rmk+ker...@armlinux.org.uk>
> > Signed-off-by: Sascha Hauer <s.ha...@pengutronix.de>
> > ---
> > drivers/net/ethernet/marvell/mvneta.c | 80 +++++++++++++++------------
> > 1 file changed, 44 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index 51889770958d8..9933eb4577d43 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> > @@ -106,6 +106,7 @@
> > #define MVNETA_TX_IN_PRGRS BIT(1)
> > #define MVNETA_TX_FIFO_EMPTY BIT(8)
> > #define MVNETA_RX_MIN_FRAME_SIZE 0x247c
> > +/* Only exists on Armada XP and Armada 370 */
> > #define MVNETA_SERDES_CFG 0x24A0
> > #define MVNETA_SGMII_SERDES_PROTO 0x0cc7
> > #define MVNETA_QSGMII_SERDES_PROTO 0x0667
> > @@ -3514,26 +3515,55 @@ static int mvneta_setup_txqs(struct mvneta_port *pp)
> > return 0;
> > }
> >
> > -static int mvneta_comphy_init(struct mvneta_port *pp)
> > +static int mvneta_comphy_init(struct mvneta_port *pp, phy_interface_t
> > interface)
> > {
> > int ret;
> >
> > - if (!pp->comphy)
> > - return 0;
> > -
> > - ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET,
> > - pp->phy_interface);
> > + ret = phy_set_mode_ext(pp->comphy, PHY_MODE_ETHERNET, interface);
> > if (ret)
> > return ret;
> >
> > return phy_power_on(pp->comphy);
> > }
> >
> > +static int mvneta_config_interface(struct mvneta_port *pp,
> > + phy_interface_t interface)
> > +{
> > + int ret = 0;
> > +
> > + if (pp->comphy) {
> > + if (interface == PHY_INTERFACE_MODE_SGMII ||
> > + interface == PHY_INTERFACE_MODE_1000BASEX ||
> > + interface == PHY_INTERFACE_MODE_2500BASEX) {
> > + ret = mvneta_comphy_init(pp, interface);
> > + }
> > + } else {
> > + switch (interface) {
> > + case PHY_INTERFACE_MODE_QSGMII:
> > + mvreg_write(pp, MVNETA_SERDES_CFG,
> > + MVNETA_QSGMII_SERDES_PROTO);
> > + break;
> > +
> > + case PHY_INTERFACE_MODE_SGMII:
> > + case PHY_INTERFACE_MODE_1000BASEX:
> > + mvreg_write(pp, MVNETA_SERDES_CFG,
> > + MVNETA_SGMII_SERDES_PROTO);
> > + break;
> > + default:
> > + return -EINVAL;
>
> I've just noticed that you made changes to the patch I sent, such as
> adding this default case that errors out, and by doing so, you have
> caused a regression by causing a WARN_ON() splat.
>
> It was not accidental that my patch had "break;" here instead of an
> error return, and I left the interface mode checking in
> mvneta_port_power_up() that you also removed.
>
> mvneta supports RGMII, and since RGMII doesn't use the serdes, there
> is no need to write to MVNETA_SGMII_SERDES_PROTO, and so we want to
> ignore those, not return -EINVAL.
>
> Since the interface type was already validated both by phylink when
> the interface is brought up, and also by the driver at probe time
> through mvneta_port_power_up(), which performs early validation of
> the mode given in DT this was not a problem... there is no need to
> consider anything but the RGMII case in the "default" case here.
>
> So, please fix this... at minimum fixing this switch() statement not
> to error out in the RGMII cases. However, I think actually following
> what was in my patch (which was there for good reason) rather than
> randomly changing it would have been better.
>
> This will have made the kernel on the SolidRun Clearfog platform
> trigger the WARN_ON()s for the dedicated gigabit port, which uses
> RGMII, and doesn't have a comphy specified in DT... and having
> waited for the compile to finish and the resulting kernel to boot...
I'll send a fixup shortly
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
