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'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.
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.
(...)
> +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.
Did you test these protocols?
I would be much more comfortable if we only add to the supported
protocols list what was tested.
(...)
> + /* 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?
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?
> + } 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.
If you really need this, maybe you can move it in the phy_init callback.
> +
> + /* 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.
(...)
> +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.
Ioana
