Re: [PATCH net-next v3 10/11] phy: add driver for Microsemi Ocelot SerDes muxing

[Quentin Schulz]

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