From: Quentin Schulz <quentin.schulz@bootlin.com>
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: alexandre.belloni@bootlin.com, ralf@linux-mips.org,
paul.burton@mips.com, jhogan@kernel.org, robh+dt@kernel.org,
mark.rutland@arm.com, davem@davemloft.net, kishon@ti.com,
andrew@lunn.ch, allan.nielsen@microchip.com,
linux-mips@linux-mips.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
thomas.petazzoni@bootlin.com
Subject: Re: [PATCH net-next v3 10/11] phy: add driver for Microsemi Ocelot SerDes muxing
Date: Mon, 1 Oct 2018 12:02:16 +0200 [thread overview]
Message-ID: <20181001100216.fm3ojqx4zwnmyy5a@qschulz> (raw)
In-Reply-To: <1dc66b5d-e60e-dad2-eed9-e283de260dc3@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3475 bytes --]
Hi Florian,
On Sat, Sep 15, 2018 at 02:20:25PM -0700, Florian Fainelli wrote:
>
>
> On 09/14/18 01:16, Quentin Schulz wrote:
> > The Microsemi Ocelot can mux SerDes lanes (aka macros) to different
> > switch ports or even make it act as a PCIe interface.
> >
> > This adds support for the muxing of the SerDes.
> >
> > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com>
> > ---
>
> [snip]
>
> > +
> > +struct serdes_macro {
> > + u8 idx;
> > + /* Not used when in QSGMII or PCIe mode */
> > + int port;
>
> u8 port to be consistent with the mux table?
>
Not wanted in the current implementation.
In serdes_phy_create, I put the port to -1. In serdes_simple_xlate, I
make sure that once port is set to anything else than -1, it cannot be
set again (cannot have 2+ PHYs sharing the same SerDes (except for
SERDES6G_0 which is used for QSGMII)).
I cannot use u8 for this simple reason. However, I'm all ears for a
nicer solution :)
> [snip]
>
> > +#define SERDES_MUX(_idx, _port, _mode, _mask, _mux) { \
> > + .idx = _idx, \
> > + .port = _port, \
> > + .mode = _mode, \
> > + .mask = _mask, \
> > + .mux = _mux, \
> > +}
> > +
> > +static const struct serdes_mux ocelot_serdes_muxes[] = {
> > + SERDES_MUX(SERDES1G_0, 0, PHY_MODE_SGMII, 0, 0),
> > + SERDES_MUX(SERDES1G_1, 1, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_5_MODE, 0),
> > + SERDES_MUX(SERDES1G_1, 5, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> > + HSIO_HW_CFG_DEV1G_5_MODE, HSIO_HW_CFG_DEV1G_5_MODE),
>
> Why not go one step further and define a SERDES_MUX_SGMII() macro which
> automatically resolves the correct bit definitions to use?
>
> The current macro does not make this particularly easy to read :/
>
I agree on the readability. But SERDES_MUX_SGMII would basically just
abstract the third argument (mode) and that's it, right? That's still
one argument less but do you see something even more intuitive and more
readable?
[...]
> > +
> > + for (i = 0; i < ARRAY_SIZE(ocelot_serdes_muxes); i++) {
> > + if (macro->idx != ocelot_serdes_muxes[i].idx ||
> > + mode != ocelot_serdes_muxes[i].mode)
> > + continue;
> > +
> > + if (mode != PHY_MODE_QSGMII &&
> > + macro->port != ocelot_serdes_muxes[i].port)
> > + continue;
> > +
> > + ret = regmap_update_bits(macro->ctrl->regs, HSIO_HW_CFG,
> > + ocelot_serdes_muxes[i].mask,
> > + ocelot_serdes_muxes[i].mux);
> > + if (ret)
> > + return ret;
> > +
> > + if (macro->idx < SERDES1G_MAX)
> > + return serdes_init_s1g(macro->ctrl->regs, macro->idx);
> > +
> > + /* SERDES6G and PCIe not supported yet */
> > + return 0;
>
> Would not returning -EOPNOTSUPP be more helpful rather than leaving the
> PHY unconfigured (or did the bootloader somehow configure it before for us)?
>
Yup, you're right, if the SerDes needs to be configured by the kernel,
the user of the SerDes mux is "broken" anyway so it makes sense to
return -EOPNOTSUPP.
[...]
> > +
> > + ctrl = devm_kzalloc(&pdev->dev, sizeof(*ctrl), GFP_KERNEL);
> > + if (!ctrl)
> > + return -ENOMEM;
> > +
> > + ctrl->dev = &pdev->dev;
> > + ctrl->regs = syscon_node_to_regmap(pdev->dev.parent->of_node);
> > + if (!ctrl->regs)
> > + return -ENODEV;
> > +
> > + for (i = 0; i <= SERDES_MAX; i++) {
>
> Every other loop you have is using <, is this one off-by-one?
That is an error.
Thanks,
Quentin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2018-10-01 10:02 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-14 8:15 [PATCH net-next v3 00/11] mscc: ocelot: add support for SerDes muxing configuration Quentin Schulz
2018-09-14 8:15 ` [PATCH net-next v3 01/11] MIPS: mscc: ocelot: make HSIO registers address range a syscon Quentin Schulz
2018-09-14 8:16 ` [PATCH net-next v3 02/11] dt-bindings: net: ocelot: remove hsio from the list of register address spaces Quentin Schulz
2018-09-14 8:16 ` [PATCH net-next v3 03/11] net: mscc: ocelot: get HSIO regmap from syscon Quentin Schulz
2018-09-15 2:23 ` Florian Fainelli
2018-09-14 8:16 ` [PATCH net-next v3 04/11] net: mscc: ocelot: move the HSIO header to include/soc Quentin Schulz
2018-09-15 2:24 ` Florian Fainelli
2018-09-14 8:16 ` [PATCH net-next v3 05/11] net: mscc: ocelot: simplify register access for PLL5 configuration Quentin Schulz
2018-09-15 2:26 ` Florian Fainelli
2018-09-14 8:16 ` [PATCH net-next v3 06/11] phy: add QSGMII and PCIE modes Quentin Schulz
2018-09-15 2:27 ` Florian Fainelli
2018-09-14 8:16 ` [PATCH net-next v3 07/11] dt-bindings: phy: add DT binding for Microsemi Ocelot SerDes muxing Quentin Schulz
2018-09-15 2:29 ` Florian Fainelli
2018-09-26 21:35 ` Rob Herring
2018-10-01 12:46 ` Quentin Schulz
2018-10-01 17:10 ` Rob Herring
2018-09-14 8:16 ` [PATCH net-next v3 08/11] MIPS: mscc: ocelot: add SerDes mux DT node Quentin Schulz
2018-09-15 2:30 ` Florian Fainelli
2018-09-14 8:16 ` [PATCH net-next v3 09/11] dt-bindings: add constants for Microsemi Ocelot SerDes driver Quentin Schulz
2018-09-15 2:31 ` Florian Fainelli
2018-09-26 21:36 ` Rob Herring
2018-09-14 8:16 ` [PATCH net-next v3 10/11] phy: add driver for Microsemi Ocelot SerDes muxing Quentin Schulz
2018-09-15 21:20 ` Florian Fainelli
2018-10-01 10:02 ` Quentin Schulz [this message]
2018-09-14 8:16 ` [PATCH net-next v3 11/11] net: mscc: ocelot: make use of SerDes PHYs for handling their configuration Quentin Schulz
2018-09-15 21:25 ` Florian Fainelli
2018-10-01 9:42 ` Quentin Schulz
2018-10-01 16:29 ` Florian Fainelli
2018-10-04 12:20 ` Quentin Schulz
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=20181001100216.fm3ojqx4zwnmyy5a@qschulz \
--to=quentin.schulz@bootlin.com \
--cc=alexandre.belloni@bootlin.com \
--cc=allan.nielsen@microchip.com \
--cc=andrew@lunn.ch \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=f.fainelli@gmail.com \
--cc=jhogan@kernel.org \
--cc=kishon@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@linux-mips.org \
--cc=mark.rutland@arm.com \
--cc=netdev@vger.kernel.org \
--cc=paul.burton@mips.com \
--cc=ralf@linux-mips.org \
--cc=robh+dt@kernel.org \
--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).