Message-ID: <20220701100313.qjiwfqirnw2pgjqi@skbuf>
Date:   Fri, 1 Jul 2022 10:03:14 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Sean Anderson <sean.anderson@...o.com>
CC:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Russell King <linux@...linux.org.uk>,
        Paolo Abeni <pabeni@...hat.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Jonathan Corbet <corbet@....net>,
        Kishon Vijay Abraham I <kishon@...com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Vinod Koul <vkoul@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>
Subject: Re: [PATCH net-next v2 04/35] [RFC] phy: fsl: Add Lynx 10G SerDes
 driver

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.

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)?

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

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

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

Ioana

Powered by blists - more mailing lists