From: Maxime Chevallier <maxime.chevallier@bootlin.com>
To: Sean Anderson <seanga2@gmail.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>,
Russell King <linux@armlinux.org.uk>,
linux-arm-kernel@lists.infradead.org,
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
devicetree@vger.kernel.org
Subject: Re: [PATCH net-next v3 3/5] net: pcs: add new PCS driver for altera TSE PCS
Date: Wed, 26 Oct 2022 11:37:11 +0200 [thread overview]
Message-ID: <20221026113711.2b740c7a@pc-8.home> (raw)
In-Reply-To: <68b3dfbf-9bab-2554-254e-bffd280ba97e@gmail.com>
Hello Sean,
On Sun, 9 Oct 2022 01:38:15 -0400
Sean Anderson <seanga2@gmail.com> wrote:
> > +#define SGMII_PCS_LINK_TIMER_REG(x) (0x12 + (x))
>
> Not used.
Right, I'll remove that in a followup patch
> > +#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.
Will do ! thanks :)
> > +#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?
Oh yeah definitely, I didn't know about this helper.
> > +
> > + 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?
I've dug a bit and indeed you're right. The value of 1.6ms works for
SGMII, but for 1000BaseX it should be set to 10ms. I'll send a fix for
this too.
> > +
> > + 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.
Correct.
> > + } 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.
You're correct too.
> > + }
> > +
> > + ctrl |= (BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_ANENABLE);
>
> BMCR_FULLDPLX is read-only, so you don't have to set it. Same for the
> speed.
Thanks, that's true
> > +
> > + 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...
Indeed. This soft reset will not affect the register configuration, it
will only reset all internal state machines.
The datasheet actually recommends performing a reset after any
configuration change...
That's one thing with this IP, it tries to re-use the C22 register
layout but it's not fully consistent with it...
> > +}
> > +
> > +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?
I'll give it a test and confirm it
> > +
> > +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@bootlin.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
Thanks a lot for the review ! I'll do a round of tests with the
comments and send follow-up patches.
Best regards,
Maxime
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-10-26 9:41 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-01 14:35 [PATCH net-next v3 0/5] net: altera: tse: phylink conversion Maxime Chevallier
2022-09-01 14:35 ` [PATCH net-next v3 1/5] dt-bindings: net: Convert Altera TSE bindings to yaml Maxime Chevallier
2022-09-01 14:35 ` [PATCH net-next v3 2/5] net: altera: tse: cosmetic change to use reverse xmas tree ordering Maxime Chevallier
2022-09-01 14:35 ` [PATCH net-next v3 3/5] net: pcs: add new PCS driver for altera TSE PCS Maxime Chevallier
2022-10-09 5:38 ` Sean Anderson
2022-10-26 9:37 ` Maxime Chevallier [this message]
2022-10-26 12:05 ` Andrew Lunn
2022-10-26 12:47 ` Russell King (Oracle)
2022-09-01 14:35 ` [PATCH net-next v3 4/5] net: altera: tse: convert to phylink Maxime Chevallier
2022-09-02 4:10 ` Jakub Kicinski
2022-09-02 7:57 ` Maxime Chevallier
2022-09-01 14:35 ` [PATCH net-next v3 5/5] dt-bindings: net: altera: tse: add an optional pcs register range Maxime Chevallier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221026113711.2b740c7a@pc-8.home \
--to=maxime.chevallier@bootlin.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=f.fainelli@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=krzysztof.kozlowski@linaro.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=robh+dt@kernel.org \
--cc=seanga2@gmail.com \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).