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?
[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 :/
> + SERDES_MUX(SERDES1G_2, 2, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_4_MODE, 0),
> + SERDES_MUX(SERDES1G_2, 4, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> + HSIO_HW_CFG_DEV1G_4_MODE, HSIO_HW_CFG_DEV1G_4_MODE),
> + SERDES_MUX(SERDES1G_3, 3, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_6_MODE, 0),
> + SERDES_MUX(SERDES1G_3, 6, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> + HSIO_HW_CFG_DEV1G_6_MODE, HSIO_HW_CFG_DEV1G_6_MODE),
> + SERDES_MUX(SERDES1G_4, 4, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> + HSIO_HW_CFG_DEV1G_4_MODE | HSIO_HW_CFG_DEV1G_9_MODE, 0),
> + SERDES_MUX(SERDES1G_4, 9, PHY_MODE_SGMII, HSIO_HW_CFG_DEV1G_4_MODE |
> + HSIO_HW_CFG_DEV1G_9_MODE, HSIO_HW_CFG_DEV1G_4_MODE |
> + HSIO_HW_CFG_DEV1G_9_MODE),
> + SERDES_MUX(SERDES1G_5, 5, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA |
> + HSIO_HW_CFG_DEV1G_5_MODE | HSIO_HW_CFG_DEV2G5_10_MODE, 0),
> + SERDES_MUX(SERDES1G_5, 10, PHY_MODE_SGMII, HSIO_HW_CFG_PCIE_ENA |
> + HSIO_HW_CFG_DEV1G_5_MODE | HSIO_HW_CFG_DEV2G5_10_MODE,
> + HSIO_HW_CFG_DEV1G_5_MODE | HSIO_HW_CFG_DEV2G5_10_MODE),
> + SERDES_MUX(SERDES6G_0, 4, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> + HSIO_HW_CFG_QSGMII_ENA),
> + SERDES_MUX(SERDES6G_0, 5, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> + HSIO_HW_CFG_QSGMII_ENA),
> + SERDES_MUX(SERDES6G_0, 6, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> + HSIO_HW_CFG_QSGMII_ENA),
> + SERDES_MUX(SERDES6G_0, 7, PHY_MODE_SGMII, HSIO_HW_CFG_QSGMII_ENA, 0),
> + SERDES_MUX(SERDES6G_0, 7, PHY_MODE_QSGMII, HSIO_HW_CFG_QSGMII_ENA,
> + HSIO_HW_CFG_QSGMII_ENA),
> + SERDES_MUX(SERDES6G_1, 8, PHY_MODE_SGMII, 0, 0),
> + SERDES_MUX(SERDES6G_2, 10, PHY_MODE_SGMII, HSIO_HW_CFG_PCIE_ENA |
> + HSIO_HW_CFG_DEV2G5_10_MODE, 0),
> + SERDES_MUX(SERDES6G_2, 10, PHY_MODE_PCIE, HSIO_HW_CFG_PCIE_ENA,
> + HSIO_HW_CFG_PCIE_ENA),
> +};
> +
> +static int serdes_set_mode(struct phy *phy, enum phy_mode mode)
> +{
> + struct serdes_macro *macro = phy_get_drvdata(phy);
> + int ret, i;
unsigned int i;
> +
> + 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)?
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct phy_ops serdes_ops = {
> + .set_mode = serdes_set_mode,
> + .owner = THIS_MODULE,
> +};
> +
> +static struct phy *serdes_simple_xlate(struct device *dev,
> + struct of_phandle_args *args)
> +{
> + struct serdes_ctrl *ctrl = dev_get_drvdata(dev);
> + int port, idx, i;
unsigned int port, idx, i;
[snip]
> +
> +static int serdes_probe(struct platform_device *pdev)
> +{
> + struct phy_provider *provider;
> + struct serdes_ctrl *ctrl;
> + int i, ret;
unsigned int i;
> +
> + 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?
--
Florian
