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@xxxxxxxxxxx>
> > ---
>
> [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:
signature.asc
Description: PGP signature
