Hi Florian,

On Sat, Sep 15, 2018 at 02:20:25PM -0700, Florian Fainelli wrote:

On 09/14/18 01:16, Quentin Schulz wrote:

quoted

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 <redacted>
---

[snip]

quoted

+
+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]

quoted

+#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?

[...]

quoted

+
+	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.

[...]

quoted

+
+	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