Re: [PATCH net-next v2 04/35] [RFC] phy: fsl: Add Lynx 10G SerDes driver

[Sean Anderson]

On 6/30/22 11:56 AM, Ioana Ciornei wrote:
> 
> Hi Sean,
> 
> I am in the process of adding the necessary configuration for this
> driver to work on a LS1088A based board. At the moment, I can see that
> the lane's PLL is changed depending on the SFP module plugged, I have a
> CDR lock but no PCS link.

I have a LS1088A board which I can test on.

> I'll let you know when I get to the bottom of this.
> 
> I didn't go through the driver in detail but added some comments below.
> 
> On Tue, Jun 28, 2022 at 06:13:33PM -0400, Sean Anderson wrote:
>> This adds support for the Lynx 10G "SerDes" devices found on various NXP
>> QorIQ SoCs. There may be up to four SerDes devices on each SoC, each
>> supporting up to eight lanes. Protocol support for each SerDes is highly
>> heterogeneous, with each SoC typically having a totally different
>> selection of supported protocols for each lane. Additionally, the SerDes
>> devices on each SoC also have differing support. One SerDes will
>> typically support Ethernet on most lanes, while the other will typically
>> support PCIe on most lanes.
>> 
> 
> (...)
> 
>> +For example, the configuration for SerDes1 of the LS1046A is::
>> +
>> +    static const struct lynx_mode ls1046a_modes1[] = {
>> +        CONF_SINGLE(1, PCIE, 0x0, 1, 0b001),
>> +        CONF_1000BASEKX(0, 0x8, 0, 0b001),
>> +        CONF_SGMII25KX(1, 0x8, 1, 0b001),
>> +        CONF_SGMII25KX(2, 0x8, 2, 0b001),
>> +        CONF_SGMII25KX(3, 0x8, 3, 0b001),
>> +        CONF_SINGLE(1, QSGMII, 0x9, 2, 0b001),
>> +        CONF_XFI(2, 0xB, 0, 0b010),
>> +        CONF_XFI(3, 0xB, 1, 0b001),
>> +    };
>> +
>> +    static const struct lynx_conf ls1046a_conf1 = {
>> +        .modes = ls1046a_modes1,
>> +        .mode_count = ARRAY_SIZE(ls1046a_modes1),
>> +        .lanes = 4,
>> +        .endian = REGMAP_ENDIAN_BIG,
>> +    };
>> +
>> +There is an additional set of configuration for SerDes2, which supports a
>> +different set of modes. Both configurations should be added to the match
>> +table::
>> +
>> +    { .compatible = "fsl,ls1046-serdes-1", .data = &ls1046a_conf1 },
>> +    { .compatible = "fsl,ls1046-serdes-2", .data = &ls1046a_conf2 },
> 
> I am not 100% sure that different compatible strings are needed for each
> SerDes block. I know that in the 'supported SerDes options' tables only
> a certain list of combinations are present, different for each block.
> Even with this, I find it odd to believe that, for example, SerDes block
> 2 from LS1046A was instantiated so that it does not support any Ethernet
> protocols.

As it happens, it does support SGMII on lane B, but it mainly supports
SATA/PCIe.

If you happen to have some additional info about the internal structure of
the SerDes, I'd be very interested. However, as far as I can tell from the
public documentation the protocols supported are different for each SerDes
on each SoC.

E.g. the LS1043A has a completely different set of supported protocols on its SerDes.

> I'll ask around to see if indeed this happens.
> 
>> +
>> +Supporting Protocols
>> +--------------------
>> +
>> +Each protocol is a combination of values which must be programmed into the lane
>> +registers. To add a new protocol, first add it to :c:type:`enum lynx_protocol
>> +<lynx_protocol>`. If it is in ``UNSUPPORTED_PROTOS``, remove it. Add a new
>> +entry to `lynx_proto_params`, and populate the appropriate fields. You may need
>> +to add some new members to support new fields. Modify `lynx_lookup_proto` to
>> +map the :c:type:`enum phy_mode <phy_mode>` to :c:type:`enum lynx_protocol
>> +<lynx_protocol>`. Ensure that :c:func:`lynx_proto_mode_mask` and
>> +:c:func:`lynx_proto_mode_shift` have been updated with support for your
>> +protocol.
>> +
>> +You may need to modify :c:func:`lynx_set_mode` in order to support your
>> +procotol. This can happen when you have added members to :c:type:`struct
>> +lynx_proto_params <lynx_proto_params>`. It can also happen if you have specific
>> +clocking requirements, or protocol-specific registers to program.
>> +
>> +Internal API Reference
>> +----------------------
>> +
>> +.. kernel-doc:: drivers/phy/freescale/phy-fsl-lynx-10g.c
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index ca95b1833b97..ef65e2acdb48 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7977,6 +7977,12 @@ F:	drivers/ptp/ptp_qoriq.c
>>  F:	drivers/ptp/ptp_qoriq_debugfs.c
>>  F:	include/linux/fsl/ptp_qoriq.h
>>  
>> +FREESCALE QORIQ SERDES DRIVER
>> +M:	Sean Anderson <sean.anderson@xxxxxxxx>
>> +S:	Maintained
>> +F:	Documentation/driver-api/phy/qoriq.rst
>> +F:	drivers/phy/freescale/phy-qoriq.c
>> +
> 
> These file names have to be changed as well.
> 
> (...)

Will fix.

>> +enum lynx_protocol {
>> +	LYNX_PROTO_NONE = 0,
>> +	LYNX_PROTO_SGMII,
>> +	LYNX_PROTO_SGMII25,
>> +	LYNX_PROTO_1000BASEKX,
>> +	LYNX_PROTO_QSGMII,
>> +	LYNX_PROTO_XFI,
>> +	LYNX_PROTO_10GKR,
>> +	LYNX_PROTO_PCIE, /* Not implemented */
>> +	LYNX_PROTO_SATA, /* Not implemented */
>> +	LYNX_PROTO_LAST,
>> +};
>> +
>> +static const char lynx_proto_str[][16] = {
>> +	[LYNX_PROTO_NONE] = "unknown",
>> +	[LYNX_PROTO_SGMII] = "SGMII",
>> +	[LYNX_PROTO_SGMII25] = "2.5G SGMII",
>> +	[LYNX_PROTO_1000BASEKX] = "1000Base-KX",
>> +	[LYNX_PROTO_QSGMII] = "QSGMII",
>> +	[LYNX_PROTO_XFI] = "XFI",
>> +	[LYNX_PROTO_10GKR] = "10GBase-KR",
>> +	[LYNX_PROTO_PCIE] = "PCIe",
>> +	[LYNX_PROTO_SATA] = "SATA",
>> +};
> 
>> +
>> +#define PROTO_MASK(proto) BIT(LYNX_PROTO_##proto)
>> +#define UNSUPPORTED_PROTOS (PROTO_MASK(SATA) | PROTO_MASK(PCIE))
> 
> From what I know, -KX and -KR need software level link training.

There was no mention of that in the datasheet, but I suspect that's
a PCS issue.

> Did you test these protocols?

No, as noted in the commit message.

> I would be much more comfortable if we only add to the supported
> protocols list what was tested.

Fine by me.

>> +	/* Deselect anything configured by the RCW/bootloader */
>> +	for (i = 0; i < conf->mode_count; i++) {
>> +		const struct lynx_mode *mode = &conf->modes[i];
>> +		u32 pccr = lynx_read(serdes, PCCRn(mode->pccr));
>> +
>> +		if (lynx_proto_mode_get(mode, pccr) == mode->cfg) {
>> +			if (mode->protos & UNSUPPORTED_PROTOS) {
>> +				/* Don't mess with modes we don't support */
>> +				serdes->used_lanes |= mode->lanes;
>> +				if (grabbed_clocks)
>> +					continue;
>> +
>> +				grabbed_clocks = true;
>> +				clk_prepare_enable(serdes->pll[0].hw.clk);
>> +				clk_prepare_enable(serdes->pll[1].hw.clk);
>> +				clk_rate_exclusive_get(serdes->pll[0].hw.clk);
>> +				clk_rate_exclusive_get(serdes->pll[1].hw.clk);
> 
> Am I understanding correctly that if you encounter a protocol which is
> not supported (PCIe, SATA) both PLLs will not be capable of changing,
> right?

Correct.

> Why aren't you just getting exclusivity on the PLL that is actually used
> by a lane configured with a protocol which the driver does not support?

PCIe will automatically switch between PLLs in order to switch speeds. So
we can't change either, because the currently-used PLL could change at any
time. SATA doesn't have this restriction. Its rates have power-of-two
relationships with each other, so it can just change the divider. However,
I've chosen to get things exclusively in both cases for simplicity.

>> +			} else {
>> +				/* Otherwise, clear out the existing config */
>> +				pccr = lynx_proto_mode_prep(mode, pccr,
>> +							    LYNX_PROTO_NONE);
>> +				lynx_write(serdes, pccr, PCCRn(mode->pccr));
>> +			}
> 
> Hmmm, do you need this?
> 
> Wouldn't it be better to just leave the lane untouched (as it was setup
> by the RCW) just in case the lane is not requested by a consumer driver
> but actually used in practice. I am referring to the case in which some
> ethernet nodes have the 'phys' property, some don't.

The reason why I do this is to make sure that no other protocols are selected.
We only clear out the protocol configuration registers for a protocol that we've
configured (e.g when we go from SGMII to XFI we clear out the SGMII register).
But if the RCW e.g. configured QSGMII, we need to disable it because otherwise we
will accidentally leave it enabled.

> If you really need this, maybe you can move it in the phy_init callback.

That's fine by me.

>> +
>> +			/* Disable the SGMII PCS until we're ready for it */
>> +			if (mode->protos & LYNX_PROTO_SGMII) {
>> +				u32 cr1;
>> +
>> +				cr1 = lynx_read(serdes, SGMIIaCR1(mode->idx));
>> +				cr1 &= ~SGMIIaCR1_SGPCS_EN;
>> +				lynx_write(serdes, cr1, SGMIIaCR1(mode->idx));
>> +			}
>> +		}
>> +	}
>> +
>> +	/* Power off all lanes; used ones will be powered on later */
>> +	for (i = 0; i < conf->lanes; i++)
>> +		lynx_power_off_lane(serdes, i);
> 
> This means that you are powering-off any lane, PCIe/SATA lanes
> which are not integrated with this driver at all, right?.
> I don't think we want to break stuff that used to be working.

You're right. This should really check used_lanes first.

> (...)
> 
>> +MODULE_DEVICE_TABLE(of, lynx_of_match);
>> +
>> +static struct platform_driver lynx_driver = {
>> +	.probe = lynx_probe,
>> +	.driver = {
>> +		.name = "qoriq_serdes",
> 
> Please change the driver's name as well.

Will do.

--Sean