Re: [PATCH net-next 03/28] phy: fsl: Add QorIQ SerDes driver

[Sean Anderson]

On 6/18/22 11:52 AM, Sean Anderson wrote:
> Hi Ioana,
> 
> On 6/18/22 8:39 AM, Ioana Ciornei wrote:
>>>> Subject: [PATCH net-next 03/28] phy: fsl: Add QorIQ SerDes driver
>>>>
>>
>> Sorry for the previous HTML formatted email...
>>
>>>
>>> Hi Sean,
>>>
>>> I am very much interested in giving this driver a go on other SoCs as well
>>> but at the moment I am in vacation until mid next week.
> 
> Please let me know your results. I have documented how to add support for
> additional SoCs, so hopefully it should be fairly straightforward.
> 
>>>> This adds support for the "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.
>>>>
>>>> There is wide hardware support for this SerDes. I have not done extensive
>>>> digging, but it seems to be used on almost every QorIQ device, including
>>>> the AMP and Layerscape series. Because each SoC typically has specific
>>>> instructions and exceptions for its SerDes, I have limited the initial
>>>> scope of this module to just the LS1046A. Additionally, I have only added
>>>> support for Ethernet protocols. There is not a great need for dynamic
>>>> reconfiguration for other protocols (SATA and PCIe handle rate changes in
>>>> hardware), so support for them may never be added.>
>>>> Nevertheless, I have tried to provide an obvious path for adding support
>>>> for other SoCs as well as other protocols. SATA just needs support for
>>>> configuring LNmSSCR0. PCIe may need to configure the equalization
>>>> registers. It also uses multiple lanes. I have tried to write the driver
>>>> with multi-lane support in mind, so there should not need to be any large
>>>> changes. Although there are 6 protocols supported, I have only tested SGMII
>>>> and XFI. The rest have been implemented as described in the datasheet.
>>>>
>>>> The PLLs are modeled as clocks proper. This lets us take advantage of the
>>>> existing clock infrastructure. I have not given the same treatment to the
>>>> lane "clocks" (dividers) because they need to be programmed in-concert with
>>>> the rest of the lane settings. One tricky thing is that the VCO (pll) rate
>>>> exceeds 2^32 (maxing out at around 5GHz). This will be a problem on 32-bit
>>>> platforms, since clock rates are stored as unsigned longs. To work around
>>>> this, the pll clock rate is generally treated in units of kHz.>
>>>> The PLLs are configured rather interestingly. Instead of the usual direct
>>>> programming of the appropriate divisors, the input and output clock rates
>>>> are selected directly. Generally, the only restriction is that the input
>>>> and output must be integer multiples of each other. This suggests some kind
>>>> of internal look-up table. The datasheets generally list out the supported
>>>> combinations explicitly, and not all input/output combinations are
>>>> documented. I'm not sure if this is due to lack of support, or due to an
>>>> oversight. If this becomes an issue, then some combinations can be
>>>> blacklisted (or whitelisted). This may also be necessary for other SoCs
>>>> which have more stringent clock requirements.
>>>
>>>
>>> I didn't get a change to go through the driver like I would like, but are you
>>> changing the PLL's rate at runtime?
> 
> Yes.
> 
>>> Do you take into consideration that a PLL might still be used by a PCIe or SATA
>>> lane (which is not described in the DTS) and deny its rate reconfiguration
>>> if this happens?
> 
> Yes.
> 
> When the device is probed, we go through the PCCRs and reserve any lane which is in
> use for a protocol we don't support (PCIe, SATA). We also get both PLL's rates
> exclusively and mark them as enabled.
> 
>>> I am asking this because when I added support for the Lynx 28G SerDes block what
>>> I did in order to support rate change depending of the plugged SFP module was
>>> just to change the PLL used by the lane, not the PLL rate itself.
>>> This is because I was afraid of causing more harm then needed for all the
>>> non-Ethernet lanes.
> 
> Yes. Since There is not much need for dynamic reconfiguration for other protocols,
> I suspect that non-ethernet support will not be added soon (or perhaps ever).
> 
>>>>
>>>> The general API call list for this PHY is documented under the driver-api
>>>> docs. I think this is rather standard, except that most driverts configure
>>>> the mode (protocol) at xlate-time. Unlike some other phys where e.g. PCIe
>>>> x4 will use 4 separate phys all configured for PCIe, this driver uses one
>>>> phy configured to use 4 lanes. This is because while the individual lanes
>>>> may be configured individually, the protocol selection acts on all lanes at
>>>> once. Additionally, the order which lanes should be configured in is
>>>> specified by the datasheet.  To coordinate this, lanes are reserved in
>>>> phy_init, and released in phy_exit.
>>>>
>>>> When getting a phy, if a phy already exists for those lanes, it is reused.
>>>> This is to make things like QSGMII work. Four MACs will all want to ensure
>>>> that the lane is configured properly, and we need to ensure they can all
>>>> call phy_init, etc. There is refcounting for phy_init and phy_power_on, so
>>>> the phy will only be powered on once. However, there is no refcounting for
>>>> phy_set_mode. A "rogue" MAC could set the mode to something non-QSGMII and
>>>> break the other MACs. Perhaps there is an opportunity for future
>>>> enhancement here.
>>>>
>>>> This driver was written with reference to the LS1046A reference manual.
>>>> However, it was informed by reference manuals for all processors with
>>>> MEMACs, especially the T4240 (which appears to have a "maxed-out"
>>>> configuration).
>>>>
>>>> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxx>
>>>> ---
>>>> This appears to be the same underlying hardware as the Lynx 28G phy
>>>> added in 8f73b37cf3fb ("phy: add support for the Layerscape SerDes
>>>> 28G").
>>>
>>> The SerDes block used on L1046A (and a lot of other SoCs) is not the same
>>> one as the Lynx 28G that I submitted. The Lynx 28G block is only included
>>> on the LX2160A SoC and its variants.
> 
> OK. I looked over it quickly and it seemed to share many of the same registers.

I looked at the LX2160ARM today and it seems like the 28g phy is mostly a superset
of the 10g phy. With some careful attention to detail, I think these drivers could
be merged. At the very least, I think it should be possible to create some helper
functions for programming the common registers.

--Sean

>>> The SerDes block that you are adding a driver for is the Lynx 10G SerDes,
>>> which is why I would suggest renaming it to phy-fsl-lynx-10g.c.
> 
> Ah, thanks. Is this documented anywhere?
> 
> --Sean
>