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

[Sean Anderson <sean.anderson@seco.com>]

Hi Ioana,

On 7/1/22 6:03 AM, Ioana Ciornei wrote:
> On Thu, Jun 30, 2022 at 02:11:17PM -0400, Sean Anderson wrote:
>> 
>> 
>> 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.
> 
> If it's a LS1088ARDB one, you have to bypass / disable the retimer which
> is between the SerDes lane and the SFP cage. I have some i2cset commands
> which do this, let me know if you need them.

I'd appreciate that.

> By the way, I think the LS1046ARDB also has a retimer. What are you
> doing with that when you switch to an SFP module (SGMII/1000Base-X)?

I haven't tested that so far... In fact, I'd forgotten about that retimer.
Perhaps it can be modeled as an additional "phy". Although according to
the datasheet,

> Each channel of the DS110DF111 will, by default operate at 10.3125 Gbps
> and 1.25 Gbps

so it seems like it shouldn't need reconfiguration to switch between SGMII
and XFI.

>> >> +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.
> 
> Yes, between the SoCs there are differences and having SoC specific
> compatible helps there.
> 
> What I am not sure of is if there are different instantiations of the
> SerDes in the same SoC. Will let you know when I find out more myself.
> 
>> >> +
>> >> +#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.
> 
> 
> No, not just the PCS is involved in the backplane (-KR, -KX) link
> training.
> Depending on the what the link partner requests, the pre- and post-tap
> coefficients (the TECR0 register) need to be changed. Those default
> values presented in the RM may well work in some situations, but not all
> of them. They are usually just used as a starting point for the link
> training algorithm which will try to get the link to an optimal point.
> 
> Here is an application note which describes in more details what I just
> said: https://www.nxp.com/docs/en/application-note/AN12572.pdf

Well the linked repo [1] certainly is interesting, as it contains around 1/3
of a general phy driver. To support KX/KR it definitely seems like some kind
of iterative process is necessary, probably using phy_configure. Such a process
is most naturally driven using the PCS... it might make sense to reference the
SerDes from the PCS node instead of the MAC. E.g.

	mdio@e9000 {
		#address-cells = <1>;
		#size-cells = <0>;
		compatible = "fsl,fman-memac-mdio", "fsl,fman-xmdio";
		reg = <0xe7000 0x1000>;

		pcsphy4: ethernet-phy@0 {
			reg = <0x0>;
			phys = <&serdes1 1>;
		};
	};

This of course would be easier with a more normal probing process.

That said, I do agree with you that KX/KR would probably not function as-is.

[1] https://source.codeaurora.org/external/qoriq/qoriq-components/linux-extras/

>> > 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.
> 
> Oh, ok. I didn't know that PCIe does this automatic switchover between
> PLLs. Thanks!
> 
>> 
>> >> +			} 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.
>> 
> 
> I am not sure if the used_lanes indication will cover the case in which
> just some, for example, SGMII lanes have a 'phys' property pointing to
> them but not all of them.

This is why I've disabled the SerDes by default. Boards which enable it
will need to ensure that all the Ethernet interfaces have had their phys
property added.

> Again, powering off the lane can be done in the phy_init.

Not if no one ever uses the lane. Unlike the clock subsystem, unused phys
are not automatically powered off. We could of course wait until sometime
after probe, but doing it now is easiest.

--Sean