| Message ID | 20230413160607.4128315-1-sean.anderson@seco.com |
|---|---|
| Headers | show |
| Series | phy: Add support for Lynx 10G SerDes | expand |
Hello, On Thu, 13 Apr 2023 12:05:52 -0400, Sean Anderson wrote: > This series is ready for review by the phy maintainers. I have addressed > all known feedback and there are no outstanding issues. > > Major reconfiguration of baud rate (e.g. 1G->10G) does not work. > > There are several stand-alone commits in this series. Please feel free > to pick them as appropriate. In particular, commits 1, 3, 4, 12, 13, and > 14 are all good candidates for picking. > > - Although this is untested, it should support 2.5G SGMII as well as > 1000BASE-KX. The latter needs MAC and PCS support, but the former > should work out of the box. > - It allows for clock configurations not supported by the RCW. This is > very useful if you want to use e.g. SRDS_PRTCL_S1=0x3333 and =0x1133 > on the same board. This is because the former setting will use PLL1 > as the 1G reference, but the latter will use PLL1 as the 10G > reference. Because we can reconfigure the PLLs, it is possible to > always use PLL1 as the 1G reference. I am an engineer working for NXP and I have access to the majority of hardware that includes the Lynx 10G SERDES, as well as to block guides that are not visible to customers, and to people from the hardware design and validation teams. I have an interest in adding a driver for this SERDES to support dynamic Ethernet protocol reconfiguration, and perhaps the internal PHY for copper backplane modes (1000Base-KX, 10GBase-KR) and its link training procedure - although the latter will need more work. I would like to thank you for starting the effort, but please, stop submitting code for modes that are untested, and do not submit features that do not work. If you do not have the time to fix up the loose ends for this patch submission now, you won't have the time to debug regressions on boards you might not even have access to, which worked fine previously due to the static RCW/PBL configuration. I have downloaded your patches, and although I have objections to some of them already, I will be in the process of evaluating, testing, changing them, for the coming weeks, perhaps even more. Please consider this a NACK for the current patch set due to the SERDES related material, although the unrelated patches (like "dt-bindings: Convert gpio-mmio to yaml") can and should have been submitted separately, so they can be analyzed by their respective maintainers based on their own merit and not as part of an overall problematic set.
On Wed, Apr 26, 2023 at 10:50:17AM -0400, Sean Anderson wrote: > > I need to catch up with 14 rounds of patches from you and with the > > discussions that took place on each version, and understand how you > > responded to feedback like "don't remove PHY interrupts without finding > > out why they don't work" > > All I can say is that > > - It doesn't work on my board > - The traces are on the bottom of the PCB > - The signal goes through an FPGA which (unlike the LS1046ARDB) is closed-source I don't understand the distinction you are making here. Are the sources for QIXIS bit streams public for any Layerscape board? > - The alternative is polling once a second (not terribly intensive) It makes a difference to performance (forwarded packets per second), believe it or not. > > I think it's very reasonable to make this change. Anyway, it's in a separate > patch so that it can be applied independently. Perhaps better phrased: "discussed separately"... > > Even if the SERDES and PLL drivers "work for you" in the current form, > > I doubt the usefulness of a PLL driver if you have to disconnect the > > SoC's reset request signal on the board to not be stuck in a reboot loop. > > I would like to emphasize that this has *nothing to do with this driver*. > This behavior is part of the boot ROM (or something like it) and occurs before > any user code has ever executed. The problem of course is that certain RCWs > expect the reference clocks to be in certain (incompatible) configurations, > and will fail the boot without a lock. I think this is rather silly (since > you only need PLL lock when you actually want to use the serdes), but that's > how it is. And of course, this is only necessary because I was unable to get > major reconfiguration to work. In an ideal world, you could always boot with > the same RCW (with PLL config matching the board) and choose the major protocol > at runtime. Could you please tell me what are the reference clock frequencies that your board provides at boot time to the 2 PLLs, and which SERDES protocol out of those 2 (1133 and 3333) boots correctly (no RESET_REQ hacks necessary) with those refclks? I will try to get a LS1046A-QDS where I boot from the same refclk + SERDES protocol configuration as you, and use PBI commands in the RCW to reconfigure the lanes (PLL selection and protocol registers) for the other mode, while keeping the FRATE_SEL of the PLLs unmodified.
Hello Sean, On Fri, Jun 09, 2023 at 03:19:22PM -0400, Sean Anderson wrote: > On 5/22/23 11:00, Vladimir Oltean wrote: > > On Mon, May 22, 2023 at 10:42:04AM -0400, Sean Anderson wrote: > >> Have you had a chance to review this driver? > > > > Partially / too little (and no, I don't have an answer yet). I am > > debugging a SERDES protocol change procedure from XFI to SGMII. > > I'd just like to reiterate that, like I said in the cover letter, I > believe this driver still has value even if it cannot yet perform > protocol switching. > > Please send me your feedback, and I will try and incorporate it into the > next revision. Previously, you said you had major objections to the > contents of this series, but you still have not listed them. And if SERDES protocol switching was not physically possible, would this patch set still have value?
On 6/12/23 12:33, Vladimir Oltean wrote: > On Mon, Jun 12, 2023 at 10:35:21AM -0400, Sean Anderson wrote: >> > And if SERDES protocol switching was not physically possible, would this >> > patch set still have value? More to the point, did you make any progress in this area? >> Yes. To e.g. set up SGMII25 or to fix the clocking situation. > > Let me analyze the reasons one by one. > > The clocking situation only exists due to a hope that protocol changing > would be possible. If you were sure it wasn't possible, your board would > have had refclks set up for protocol 3333 via the supported PLL mapping 2222. > In essence, I consider this almost a non-argument, as it fixes a > self-inflicted problem. 2222 also does not work, as outlined above. The clocking is incompatible, and must be fixed by software. The only thing self-inflicted is NXP's conflicting design. > Have you actually tested changing individual lanes from SGMII to SGMII 2.5? > I know it works on LS1028A, but I don't know if that's also true on LS1046A. > Your comments do say "LYNX_PROTO_SGMII25, /* Not tested */". Not yet. This driver would also be a good place to add the KR link training with NXP tried to upstream a few years ago. > Assuming a start from SERDES protocol 3333 with PLL mapping 2222, > this protocol change implies powering on PLL 1 (reset state machine > wants it to be disabled) and moving those lanes from PLL 2 to PLL 1. > > At first sight you might appear to have a point related to the fact that > PLL register writes are necessary, and thus this whole shebang is necessary. > But this can all be done using PBI commands, with the added benefit that > U-Boot can also use those SERDES networking ports, and not just Linux. > You can use the RCW+PBL specific to your board to inform the SoC that > your platform's refclk 1 is 156 MHz (something which the reset state > machine seems unable to learn, with protocol 0x3333). You don't have to > put that in the device tree. You don't have to push code to any open > source project to expose your platform specific details. Then, just like > in the case of the Lynx 28G driver on LX2160, the SERDES driver could > just treat the PLL configuration as read-only, which would greatly > simplify things and eliminate the need for a clk driver. > > Here is an illustrative example (sorry, I don't have a board with the > right refclk on that PLL, to verify all the way): > > ... snip ... (which of course complicates the process of building the PBIs...) > In short, I believe the reasons you have cited do not justify the > complexity of the solution that you propose. The work is done, and it is certainly more flexible than what you propose. E.g. imagine a clock which needs to be configured before it has the correct frequency. Such a thing is difficult to do in a PBI solution, but is trivial in Linux. --Sean
On Fri, Aug 11, 2023 at 11:43:01AM -0400, Sean Anderson wrote: > >> > > Here is an illustrative example (sorry, I don't have a board with the > >> > > right refclk on that PLL, to verify all the way): > >> > > > >> > > ... snip ... > >> > > >> > (which of course complicates the process of building the PBIs...) > >> > >> Maybe this is the language barrier, but what are you trying to say here? > > > > I said that I don't understand. Can you please clarify what you were > > trying to transmit with this comment? > > Well, right now I produce my RCWs by generating a second RCW with e.g. > 1133 replaced by 3333. But doing this is a more involved change. Just a > minor issue, really. Ok, so a comment related to your private build environment, and not a counter-argument to the solution. It wasn't clear. > That said, I don't think this is the best approach moving forward. > Much like many other platforms, users should be able to plug in an SFP > module and Linux should configure things appropriately. An RCW > approach requires some kind of configuration tool to swap out the > bootloaders, which isn't as good of a user experience. I am quite familiar with the SFP use case you are talking about. SolidRun requested that on LX2160 with the Lynx 28G driver, and that works fine there. What I am proposing is for the exact same approach to be used with Lynx 10G as well. Let me explain that approach, because your mention of "swapping out the bootloaders" makes it appear as if you are not visualising what I am proposing. The Lynx SerDes family has 2 PLLs, and more lanes (4 or 8). Each lane uses one PLL or the other, to derive its protocol frequency. Through the RCW, you provision the 2 PLL frequencies that may be used by the lanes at runtime. The Lynx 28G SerDes driver reads the PLL frequencies in lynx_28g_pll_read_configuration(), and determines the interface modes supportable by each PLL (this is used by phylink). But it never changes those PLL frequencies, since that operation is practically impossible in the general sense (PLLs are shared by multiple lanes, so changing a PLL frequency disrupts all lanes that use it). So, you see, the RCW approach that I am proposing is not the RCW approach that you are thinking about. I was never suggesting to swap the bootloader when the user plugs in a different SFP. I am just suggesting that the RCW is provisioned for 2 different PLL frequencies, and those PLL frequencies give you 2 networking protocol speed options. Depending on what each lane needs (the information comes from phylink), you can move each lane to one PLL or the other, at runtime, in the Linux SerDes driver, to support the speeds of 1G and 10G, or 10G and 25G. You can't get more than 2 speeds at the same time with just 2 PLLs, and this is an intrinsic limitation of the fact that there are just 2 PLLs. If the RCW and board do not provision the PLL frequencies for more than 1 lane speed, then no dynamic protocol switching is possible, and the supported_interfaces of each lane contains a single bit: the reset-time protocol.
On 8/21/23 08:49, Vladimir Oltean wrote: > Hi Sean, > > On Fri, Aug 11, 2023 at 07:36:37PM +0300, Vladimir Oltean wrote: >> Let me explain that approach, because your mention of "swapping out the >> bootloaders" makes it appear as if you are not visualising what I am >> proposing. >> >> The Lynx SerDes family has 2 PLLs, and more lanes (4 or 8). Each lane >> uses one PLL or the other, to derive its protocol frequency. Through the >> RCW, you provision the 2 PLL frequencies that may be used by the lanes >> at runtime. >> >> The Lynx 28G SerDes driver reads the PLL frequencies in >> lynx_28g_pll_read_configuration(), and determines the interface modes >> supportable by each PLL (this is used by phylink). But it never changes >> those PLL frequencies, since that operation is practically impossible in >> the general sense (PLLs are shared by multiple lanes, so changing a PLL >> frequency disrupts all lanes that use it). > > Is my high-level feedback clear and actionable to you? I am suggesting > to keep the look and feel the same between the lynx-10g and lynx-28g > drivers, and to not use "fsl,type" protocols listed in the device tree > as the immutable source of information for populating mode->protos, but > instead the current PLL frequency configuration. So this implies that I > am requesting that the dt-bindings should not contain a listing of the > supported protocols. Well, we have two pieces of information we need - What values do we need to program in the PCCRs to select a particular mode? This includes whether to e.g. set the KX bits. - Implied by the above, what protocols are supported on which lanes? This is not strictly necessary, but will certainly solve a lot of headscratching. This information varies between different socs, and different serdes on the same socs. We can't really look at the RCW or the clocks and figure out what we need to program. So what are our options? - We can have a separate compatible for each serdes on each SoC (e.g. "fsl,lynx-10g-a"). This was rejected by the devicetree maintainers. - We can have one compatible for each SoC, and determine the serdes based on the address. I would like to avoid this... - We can stick all the details which vary between serdes/socs into the device tree. This is very flexible, since supporting new SoCs is mostly a matter of adding a new compatible and writing a new devicetree. On the other hand, if you have a bug in your devicetree, it's not easy to fix it in the kernel. - Just don't support protocol switching. The 28G driver does this, which is why it only has one compatible. However, supporting protocol switching is a core goal of this driver, so dropping support is not an option. I'm open to any other suggestions you have, but this was the best way I could figure out to get this information to the driver in a way which would be acceptable to the devicetree folks. --Sean
On Mon, Aug 21, 2023 at 01:45:44PM -0400, Sean Anderson wrote: > Well, we have two pieces of information we need > > - What values do we need to program in the PCCRs to select a particular > mode? This includes whether to e.g. set the KX bits. > - Implied by the above, what protocols are supported on which lanes? > This is not strictly necessary, but will certainly solve a lot of > headscratching. > > This information varies between different socs, and different serdes on > the same socs. We can't really look at the RCW or the clocks and figure > out what we need to program. So what are our options? > > - We can have a separate compatible for each serdes on each SoC (e.g. > "fsl,lynx-10g-a"). This was rejected by the devicetree maintainers. > - We can have one compatible for each SoC, and determine the serdes > based on the address. I would like to avoid this... To me this really seems like a straightforward approach. > - We can stick all the details which vary between serdes/socs into the > device tree. This is very flexible, since supporting new SoCs is > mostly a matter of adding a new compatible and writing a new > devicetree. On the other hand, if you have a bug in your devicetree, > it's not easy to fix it in the kernel. > - Just don't support protocol switching. The 28G driver does this, which > is why it only has one compatible. However, supporting protocol > switching is a core goal of this driver, so dropping support is not an > option. > The Lynx 28G SerDes driver does support protocol switching. How did you arrive at the opposite conclusion? The initial commit on the driver is even part of a patch set named "dpaa2-mac: add support for changing the protocol at runtime". In upstream it only supports the 1G <-> 10G transition but I have some patches on the way to also support 25G. Ioana
On Mon, Aug 21, 2023 at 09:13:49PM +0300, Ioana Ciornei wrote: > > - We can have one compatible for each SoC, and determine the serdes > > based on the address. I would like to avoid this... > > To me this really seems like a straightforward approach. +1
On 8/21/23 15:58, Vladimir Oltean wrote: > On Mon, Aug 21, 2023 at 02:46:53PM -0400, Sean Anderson wrote: >> After further review, it seems the reason 28g can get away without this >> is because there's a one-to-one mapping between protocol controllers and >> lanes. Unfortunately, that regularity is not present for 10g. >> >> --Sean > > There are some things I saw in your phy-fsl-lynx-10g.c driver and device > tree bindings that I don't understand (the concept of lane groups) Each lane group corresponds to a phy device (struct phy). For protocols like PCIe or XAUI which can use multiple lanes, this lets the driver coordinate configuring all lanes at once in the correct order. > and > I'm not sure if they're related with what you're saying here, so if you > could elaborate a bit more (ideally with an example) on the one-to-one > mapping and the specific problems it causes, it would be great. For e.g. the LS2088A, SerDes1 lanes H-A use SG1-8 and XFI1-8. SerDes2 lanes A-H use SG9-16 and XFI9-16. Each lane has its own controller, and the mapping is 1-to-1. In the PCCRs, each controller uses the same value, and is mapped in a regular way. So you can go directly from the lane number to the right value to mask in the PCCR, with a very simple translation scheme. For e.g. the LS1046A, SerDes1 lane D uses XFI.9 (aka XFIA) and lane C uses XFI.10 (aka XFIB). This is the opposite of how SerDes1 lanes A-D use SGMII.9, .10, .5, and .6 (aka SGMIIA-D). For e.g. the T4240, SerDes1 lanes A-H use sg1.5, .6, .10, .9, .1, .2, .3, .4 (aka SGMII E, F, H, G, A, B, C, D). For e.g. the B4860, SerDes lanes A uses sgmii1 or sgmii5 and B uses sgmii2 or sgmii6. The MAC selected is determined based on the value programmed into PCCR2. While I appreciate that your hardware engineers did a better job for 28g, many 10g serdes arbitrarily map lanes to protocol controllers. I think the mapping is too irregular to tame, and it is better to say "if you want this configuration, program this value". > I may be off with my understanding of the regularity you are talking about, > but the LX2160 (and Lynx 28G block) also has multi-lane protocols like 40G, > 100G, assuming that's what you are talking about. I haven't started yet > working on those for the mtip_backplane driver, but I'm not currently > seeing a problem with the architecture where a phy_device represents a > single lane that's part of a multi-lane port, and not an entire group. Resetting one lane in a group will reset the rest, which could confuse the driver. Additionally, treating the lanes as one phy lets us set the reset direction and first lane bits correctly. > In my imagination, there are 2 cases: > - all 4 lanes are managed by the single dpaa2-mac consumer (which has 4 > phandles, and iterates over them with a "for" loop) > - each of the 4 lanes is managed by the respective backplane AN/LT core, > and thus, there's one phandle to each lane By doing the grouping in the driver, we also simplify the consumer implementation. The MAC can always use a single phy, without worrying about the actual number of lanes. This matches the hardware, since the MAC is going to talk XGMII (or whatever) to the protocol controller anyway. I think it will be a lot easier to add multi-lane support with this method because it gives the driver more information about what's going on. The driver can control the whole configuration/reset process and the timing. > I sketched some dt-bindings for the second case here, so I guess it must > be the first scenario that's somehow problematic? > https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fpatchwork.kernel.org%2fproject%2fnetdevbpf%2fpatch%2f20230817150644.3605105%2d9%2dvladimir.oltean%40nxp.com%2f&umid=9e644233-009e-4197-a266-5d9a85eb1148&auth=d807158c60b7d2502abde8a2fc01f40662980862-cc1d5330d84af8fa40745b165a44849db50f8a67 Yes, no issues with the second case. --Sean
On 8/21/23 18:48, Vladimir Oltean wrote: > On Mon, Aug 21, 2023 at 05:06:46PM -0400, Sean Anderson wrote: >> On 8/21/23 15:58, Vladimir Oltean wrote: >> > On Mon, Aug 21, 2023 at 02:46:53PM -0400, Sean Anderson wrote: >> >> After further review, it seems the reason 28g can get away without this >> >> is because there's a one-to-one mapping between protocol controllers and >> >> lanes. Unfortunately, that regularity is not present for 10g. >> >> >> >> --Sean >> > >> > There are some things I saw in your phy-fsl-lynx-10g.c driver and device >> > tree bindings that I don't understand (the concept of lane groups) >> >> Each lane group corresponds to a phy device (struct phy). For protocols >> like PCIe or XAUI which can use multiple lanes, this lets the driver >> coordinate configuring all lanes at once in the correct order. > > For PCIe I admit that I don't know. I don't even know if there's any > valid (current or future) use case for having the PCIe controller have a > phandle to the SerDes. Everything else below in my reply assumes Ethernet. One use case could be selecting PCIe/SATA as appropriate for an M.2 card. Another use case could be allocating lanes to different slots based on card presence (e.g. 1 card use x4, 2+ cards use x1). I recently bought a motherboard whose manual says | PCI_E4 [normally x4] will run x1 speed when installing devices in PCI_E2/ | PCI_E3/ PCI_E5 slot [all x1]. which implies exactly this sort of design. >> > and >> > I'm not sure if they're related with what you're saying here, so if you >> > could elaborate a bit more (ideally with an example) on the one-to-one >> > mapping and the specific problems it causes, it would be great. >> >> For e.g. the LS2088A, SerDes1 lanes H-A use SG1-8 and XFI1-8. SerDes2 >> lanes A-H use SG9-16 and XFI9-16. Each lane has its own controller, and >> the mapping is 1-to-1. In the PCCRs, each controller uses the same >> value, and is mapped in a regular way. So you can go directly from the >> lane number to the right value to mask in the PCCR, with a very simple >> translation scheme. >> >> For e.g. the LS1046A, SerDes1 lane D uses XFI.9 (aka XFIA) and lane C >> uses XFI.10 (aka XFIB). This is the opposite of how SerDes1 lanes A-D >> use SGMII.9, .10, .5, and .6 (aka SGMIIA-D). >> >> For e.g. the T4240, SerDes1 lanes A-H use sg1.5, .6, .10, .9, .1, .2, >> .3, .4 (aka SGMII E, F, H, G, A, B, C, D). >> >> For e.g. the B4860, SerDes lanes A uses sgmii1 or sgmii5 and B uses >> sgmii2 or sgmii6. The MAC selected is determined based on the value >> programmed into PCCR2. > > It's so exceedingly unlikely that B4860 will gain support for anything > new, that it's not even worth talking about it, or even considering it > in the design of a new driver. Just forget about it. > > Let's concentrate on the Layerscapes, and on the T series to the extent > that we're not going out of our way to support them with a fairly simple > design. Well, these are just easy ways to show the oddities in the PCCRs. I could have made the same points with the various Layerscapes. > In the Lynx 10G block guide that I have, PCCR2 is a register that does > something completely different from Ethernet. I'm not sure if B4860 has > a Lynx 10G block and not something else. T-series SoCs use a different PCCR layout than Layerscape, despite using the same registers in the rest of the SerDes. The LS1021 also uses this scheme, so it's not just T-series (but it's close enough). This is why I had an option for different PCCR configuration functions in earlier versions of this series, so if someone wanted they could easily add T-series support. >> While I appreciate that your hardware engineers did a better job for >> 28g, many 10g serdes arbitrarily map lanes to protocol controllers. >> I think the mapping is too irregular to tame, and it is better to say >> "if you want this configuration, program this value". > > Ok, but that's a lateral argument (or I'm not understanding the connection). To restate, there's no systemic organization to the PCCRs. The driver configuration should reflect this and allow arbitrary mappings. > Some maintainers (Mark Brown for sure, from my personal experience) prefer > that expert-level knowledge of hardware should be hardcoded into the kernel > driver based on known stuff such as the SoC-specific compatible string. > I certainly share the same view. > > With your case, I think that argument is even more pertinent, because > IIUC, the lane group protocols won't be put in the SoC .dtsi (so as to > be written only once), but in the board device trees (since the > available protocols invariably depend upon the board provisioning). > So, non-expert board device tree writers will have to know what's with > the PCCR stuff. Pretty brain-intensive. The idea is to stick all the PCCR stuff in the SoC devicetree, and the board-level devicetrees just set up the clocks and pick which MACs use which phys. Have a look at patches 10 and 15 of this series for some typical board configurations. I think it's pretty simple, since all the complexity is in the SoC dt. That said, I originally had the driver set up so that everything was based on the compatible, but I had to change that because it was nacked by the devicetree people. >> > I may be off with my understanding of the regularity you are talking about, >> > but the LX2160 (and Lynx 28G block) also has multi-lane protocols like 40G, >> > 100G, assuming that's what you are talking about. I haven't started yet >> > working on those for the mtip_backplane driver, but I'm not currently >> > seeing a problem with the architecture where a phy_device represents a >> > single lane that's part of a multi-lane port, and not an entire group. >> >> Resetting one lane in a group will reset the rest, which could confuse >> the driver. Additionally, treating the lanes as one phy lets us set the >> reset direction and first lane bits correctly. > > Yeah, in theory that is probably correct, but in practice can't we hide > our head in the sand and say that the "phys" phandles have to have the > lanes in the same order as LNmGCR0[1STLANE] expects them (which is also > the "natural" order as the SoC RM describes it)? With a "for" loop > implementation in the MAC, that would work just fine 100% of the time. > Doing more intricate massaging of the "phys" in the consumer, and you're > just asking for trouble. My 2 cents. Yeah, but from the perspective of the driver, if we have one phy per lane we don't get any indication from the subsystem that we are doing a multi-lane configuration. So we need to stick this sort of thing into the phy handle. But IMO we can do all this in the driver and the board integrator never has to worry about it. > Sure, it's the same kind of ask of a board device tree writer as "hey, > please give me a good PCCR value", but I honestly think that the head > scratching will be much less severe. > >> > In my imagination, there are 2 cases: >> > - all 4 lanes are managed by the single dpaa2-mac consumer (which has 4 >> > phandles, and iterates over them with a "for" loop) >> > - each of the 4 lanes is managed by the respective backplane AN/LT core, >> > and thus, there's one phandle to each lane >> >> By doing the grouping in the driver, we also simplify the consumer >> implementation. The MAC can always use a single phy, without worrying >> about the actual number of lanes. This matches the hardware, since the >> MAC is going to talk XGMII (or whatever) to the protocol controller >> anyway. > > XGMII is the link between the MAC and the PCS, but the "phys" phandle to > the SerDes gives insight to the MAC driver way beyond the PCS layer. > That kinda invalidates the idea that "you don't need to worry about the > actual number of lanes". When you're a MAC driver with an XLAUI link and > you need insight into the PMA/PMD layer, you'd better not be surprised > about the fact that there are 4 lanes, at the very least? Well, this is why they are condensed into a "lane group". From the MAC's perspective the whole thing is opaque, and gets handled by the phy subsystem. >> I think it will be a lot easier to add multi-lane support with this >> method because it gives the driver more information about what's going >> on. The driver can control the whole configuration/reset process and the >> timing. > > Also, I'm thinking that if you support multi-lane (which dpaa2-mac currently > doesn't, even in LSDK), you can't avoid multiple "phys" phandles in dpaa2-mac, > and a "for" loop, eventually, anyway. That's because if your lanes have protocol > retimers on them, those are going to be modeled as generic PHYs too. And those > will not be bundled in these "groups", because they might be one chip per lane. > > The retimer thing isn't theoretical, but, due to reasons independent of NXP, > we lack the ability to provide an upstream user of the "lane retimer as > generic PHY" functionality in dpaa2-mac. So it stays downstream for now. > https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2fgithub.com%2fnxp%2dqoriq%2flinux%2fcommit%2f627c5f626a13657f46f68b90882f329310e0e22f&umid=54bd2b00-07e7-4f55-8e2a-ed7b7cf7d142&auth=d807158c60b7d2502abde8a2fc01f40662980862-a6780f8e227e850b7785d5afbae1abed18ded9d9 > > So, if you're thinking of easing the work of the consumer side, I'm not > sure that the gains will be that high. Well, FWIW those serdes don't need good coordination. That said, I think it might be interesting to have some subsystem-level support for this, much like clock groups. > Otherwise, I will repeat the idea that lynx-10g and lynx-28g should be > treated in unison, because some drivers (dpaa2-mac, mtip_backplane) will > have to interface with both, and I don't really believe that major deviations > in software architecture between the 2 SerDes drivers are justifiable in > any way (multi-protocol handled differently, for example). Well, I think we should take the opportunity to think about the hardware which exists and how we plan to model it. IMO grouping lanes into a single phy simplifies both the phy driver and the mac driver. --Sean
On Mon, Aug 21, 2023 at 07:39:15PM -0400, Sean Anderson wrote: > Well, I think we should take the opportunity to think about the hardware > which exists and how we plan to model it. IMO grouping lanes into a > single phy simplifies both the phy driver and the mac driver. Ok, but ungrouped for backplane and grouped for !backplane? For the KR link modes, parallel link training, with separate consumers per lanes in a group, will be needed per lane.
On 8/22/23 10:55, Ioana Ciornei wrote: > On Mon, Aug 21, 2023 at 02:46:53PM -0400, Sean Anderson wrote: >> On 8/21/23 14:13, Ioana Ciornei wrote: >> > On Mon, Aug 21, 2023 at 01:45:44PM -0400, Sean Anderson wrote: >> >> Well, we have two pieces of information we need >> >> >> >> - What values do we need to program in the PCCRs to select a particular >> >> mode? This includes whether to e.g. set the KX bits. >> >> - Implied by the above, what protocols are supported on which lanes? >> >> This is not strictly necessary, but will certainly solve a lot of >> >> headscratching. >> >> >> >> This information varies between different socs, and different serdes on >> >> the same socs. We can't really look at the RCW or the clocks and figure >> >> out what we need to program. So what are our options? >> >> >> >> - We can have a separate compatible for each serdes on each SoC (e.g. >> >> "fsl,lynx-10g-a"). This was rejected by the devicetree maintainers. > > I previously took this statement at face value and didn't further > investigate. After a bit of digging through the first versions of this > patch set it's evident that you left out a big piece of information. > > The devicetree maintainers have indeed rejected compatible strings of > the "fsl,<soc-name>-serdes-<instance>" form but they also suggested to > move the numbering to a property instead: > > https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2fall%2fdb9d9455%2d37af%2d1616%2d8f7f%2d3d752e7930f1%40linaro.org%2f&umid=2d629417-3b95-49e4-8cdb-34737cc93582&auth=d807158c60b7d2502abde8a2fc01f40662980862-895c2dfe1c33719569d44ae2b51e21f626f39d39 > > But instead of doing that, you chose to move all the different details > that vary between SerDes blocks/SoCs from the driver to the DTS. I don't > see that this was done in response to explicit feedback. > >> >> - We can have one compatible for each SoC, and determine the serdes >> >> based on the address. I would like to avoid this... >> > >> > To me this really seems like a straightforward approach. >> >> Indeed it would be straightforward, but what's the point of having a >> devicetree in the first place then? We could just go back to being a >> (non-dt) platform device. >> > > I am confused why you are now so adamant to have these details into the > DTS. Your first approach was to put them into the driver, not the DTS: > > https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2fnetdev%2f20220628221404.1444200%2d5%2dsean.anderson%40seco.com%2f&umid=2d629417-3b95-49e4-8cdb-34737cc93582&auth=d807158c60b7d2502abde8a2fc01f40662980862-64ea8fa45172282f676b7463a5401e8a7c5bdcbf > > And this approach could still work now and get accepted by the device > tree maintainers. The only change that would be needed is to add a > property like "fsl,serdes-block-id = <1>". https://lore.kernel.org/linux-phy/1c2bbc12-0aa5-6d2a-c701-577ce70f7502@linaro.org/ Despite what he says in your link, I explicitly proposed doing exactly that and he rejected it. I suspect that despite accusing me of "twisting" the conversation, he did not clearly remember that exchange... That said, maybe we could do something like serdes: phy@1ea0000 { compatible = "fsl,ls1046a-serdes"; reg = <0x1ea0000 0x1000>, <0x1eb0000 0x1000>; }; --Sean
On 8/21/23 19:59, Vladimir Oltean wrote: > On Mon, Aug 21, 2023 at 07:39:15PM -0400, Sean Anderson wrote: >> Well, I think we should take the opportunity to think about the hardware >> which exists and how we plan to model it. IMO grouping lanes into a >> single phy simplifies both the phy driver and the mac driver. > > Ok, but ungrouped for backplane and grouped for !backplane? For the KR > link modes, parallel link training, with separate consumers per lanes in > a group, will be needed per lane. Hm, this is the sort of thing I hadn't considered since separate link training isn't necessary for lynx 10g. But couldn't this be done by adding a "lane" parameter to phy_configure_opts_xgkr? Although, I am not sure how the driver is supposed to figure out what coefficients to use. c73 implies that the training frame should be sent on each lane. So I expected that there would be four copies of the link coefficient registers. However, when reading the LX2160ARM, I only saw one set of registers (e.g. 26.6.3.3). So is link training done serially? I didn't see anything like a "lane select" field. --Sean
On Thu, Aug 24, 2023 at 06:09:52PM -0400, Sean Anderson wrote: > On 8/21/23 19:59, Vladimir Oltean wrote: > > On Mon, Aug 21, 2023 at 07:39:15PM -0400, Sean Anderson wrote: > >> Well, I think we should take the opportunity to think about the hardware > >> which exists and how we plan to model it. IMO grouping lanes into a > >> single phy simplifies both the phy driver and the mac driver. > > > > Ok, but ungrouped for backplane and grouped for !backplane? For the KR > > link modes, parallel link training, with separate consumers per lanes in > > a group, will be needed per lane. > > Hm, this is the sort of thing I hadn't considered since separate link > training isn't necessary for lynx 10g. But couldn't this be done by > adding a "lane" parameter to phy_configure_opts_xgkr? > > Although, I am not sure how the driver is supposed to figure out what > coefficients to use. c73 implies that the training frame should be sent > on each lane. So I expected that there would be four copies of the > link coefficient registers. However, when reading the LX2160ARM, I only > saw one set of registers (e.g. 26.6.3.3). So is link training done > serially? I didn't see anything like a "lane select" field. > > --Sean There is one AN/LT block replicated for each lane, even for multi-lane backplane protocols. The primary (first) AN/LT block handles C73 autoneg + C73 link training on that lane, and the secondary AN/LT blocks handle just link training on their respective lanes. In other words, each AN/LT block needs to interact with just its lane (SerDes PHY). A "lane" parameter could be added to phy_configure_opts_xgkr to work around the "grouped lanes" design, but it would complicate the consumer implementation, since the AN/LT block does not otherwise need to know what is the index of the SerDes lane it is attached to (so it would need something like an extra device tree property). C72 link training is independent on each lane, has independent AN/LT block MDIO registers, independent SerDes lane registers, and independent training frame exchanges. There is no "lane select" field. You can see the "LX2160A lanes A, B, C, D with SerDes 1 protocol 19: dpmac2 uses 40GBase-KR4" example in my backplane dt-bindings patch, which shows how on dpmac2's internal MDIO bus, there are AN/LT devices at MDIO addresses 7, 6, 5 and 4, one for each lane. I know that Lynx 10G doesn't do multi-lane backplane, but I wouldn't want Lynx 10G and Lynx 28G to have different designs when it comes to their handling of multi-lane. A single design that works for both would be great.
This adds support for the Lynx 10G SerDes found on the QorIQ T-series and Layerscape series. Due to limited time and hardware, only support for the LS1046ARDB and LS1088ARDB is added in this initial series. This series is ready for review by the phy maintainers. I have addressed all known feedback and there are no outstanding issues. There are several stand-alone commits in this series. Please feel free to pick them as appropriate. In particular, commits 1, 3, 4, 12, 13, and 14 are all good candidates for picking. Major reconfiguration of baud rate (e.g. 1G->10G) does not work. From my testing, SerDes register settings appear identical. The issue appears to be between the PCS and the MAC. The link itself comes up at both ends, and a mac loopback succeeds. However, a PCS loopback results in dropped packets. Perhaps there is some undocumented register in the PCS? I suspect this driver is around 95% complete, but I don't have the documentation to make it work completely. At the very least it is useful for two cases: - Although this is untested, it should support 2.5G SGMII as well as 1000BASE-KX. The latter needs MAC and PCS support, but the former should work out of the box. - It allows for clock configurations not supported by the RCW. This is very useful if you want to use e.g. SRDS_PRTCL_S1=0x3333 and =0x1133 on the same board. This is because the former setting will use PLL1 as the 1G reference, but the latter will use PLL1 as the 10G reference. Because we can reconfigure the PLLs, it is possible to always use PLL1 as the 1G reference. Changes in v14: - Fix incorrect $id - Add note about (lack of) use of FIELD_GET/PREP Changes in v13: - Fix references to brcm,bcm63xx-gpio.yaml (neeĢ brcm,bcm6345-gpio) - Split interrupt changes off from serdes support - Split off SFP addition from serdes support Changes in v12: - Put compatible first - Keep gpio-controller to one line - Add little-endian property - Alphabetize compatibles - Remove some comments - Remove some examples with insufficient novelty Changes in v11: - Keep empty (or almost-empty) properties on a single line - Don't use | unnecessarily - Use gpio as the node name for examples - Rename brcm,bcm6345-gpio.yaml to brcm,bcm63xx-gpio.yaml Changes in v10: - Convert gpio-mmio to yaml - Add compatible for QIXIS - Remove unnecessary inclusion of clk.h - Don't gate clocks in compatibility mode - Fix debugging print with incorrect error variable - Move serdes bindings to SoC dtsi - Add support for all (ethernet) serdes modes - Refer to "nodes" instead of "bindings" - Move compatible/reg first Changes in v9: - Add fsl,unused-lanes-reserved to allow for a gradual transition between firmware and Linux control of the SerDes - Change phy-type back to fsl,type, as I was getting the error '#phy-cells' is a dependency of 'phy-type' - Convert some u32s to unsigned long to match arguments - Switch from round_rate to determine_rate - Drop explicit reference to reference clock - Use .parent_names when requesting parents - Use devm_clk_hw_get_clk to pass clocks back to serdes - Fix indentation - Split off clock "driver" into its own patch to allow for better review. - Add ability to defer lane initialization to phy_init. This allows for easier transitioning between firmware-managed serdes and Linux- managed serdes, as the consumer (such as dpaa2, which knows what the firmware is doing) has the last say on who gets control. - Fix name of phy mode node - Add fsl,unused-lanes-reserved to allow a gradual transition, depending on the mac link type. - Remove unused clocks - Fix some phy mode node names Changes in v8: - Remove unused variable from lynx_ls_mode_init - Rename serdes phy handles to use _A, _B, etc. instead of _0, _1, etc. This should help remind readers that the numbering corresponds to the physical layout of the registers, and not the lane (pin) number. - Prevent PCSs from probing as phys - Rename serdes phy handles like the LS1046A - Add SFP slot binding - Fix incorrect lane ordering (it's backwards on the LS1088A just like it is in the LS1046A). - Fix duplicated lane 2 (it should have been lane 3). - Fix incorrectly-documented value for XFI1. - Remove interrupt for aquantia phy. It never fired for whatever reason, preventing the link from coming up. - Add GPIOs for QIXIS FPGA. - Enable MAC1 PCS - Remove si5341 binding Changes in v7: - Use double quotes everywhere in yaml - Break out call order into generic documentation - Refuse to switch "major" protocols - Update Kconfig to reflect restrictions - Remove set/clear of "pcs reset" bit, since it doesn't seem to fix anything. Changes in v6: - Bump PHY_TYPE_2500BASEX to 13, since PHY_TYPE_USXGMII was added in the meantime - fsl,type -> phy-type - frequence -> frequency - Update MAINTAINERS to include new files - Include bitfield.h and slab.h to allow compilation on non-arm64 arches. - Depend on COMMON_CLK and either layerscape/ppc - XGI.9 -> XFI.9 Changes in v5: - Update commit description - Dual id header - Remove references to PHY_INTERFACE_MODE_1000BASEKX to allow this series to be applied directly to linux/master. - Add fsl,lynx-10g.h to MAINTAINERS Changes in v4: - Add 2500BASE-X and 10GBASE-R phy types - Use subnodes to describe lane configuration, instead of describing PCCRs. This is the same style used by phy-cadence-sierra et al. - Add ids for Lynx 10g PLLs - Rework all debug statements to remove use of __func__. Additional information has been provided as necessary. - Consider alternative parent rates in round_rate and not in set_rate. Trying to modify out parent's rate in set_rate will deadlock. - Explicitly perform a stop/reset sequence in set_rate. This way we always ensure that the PLL is properly stopped. - Set the power-down bit when disabling the PLL. We can do this now that enable/disable aren't abused during the set rate sequence. - Fix typos in QSGMII_OFFSET and XFI_OFFSET - Rename LNmTECR0_TEQ_TYPE_PRE to LNmTECR0_TEQ_TYPE_POST to better reflect its function (adding post-cursor equalization). - Use of_clk_hw_onecell_get instead of a custom function. - Return struct clks from lynx_clks_init instead of embedding lynx_clk in lynx_priv. - Rework PCCR helper functions; T-series SoCs differ from Layerscape SoCs primarily in the layout and offset of the PCCRs. This will help bring a cleaner abstraction layer. The caps have been removed, since this handles the only current usage. - Convert to use new binding format. As a result of this, we no longer need to have protocols for PCIe or SATA. Additionally, modes now live in lynx_group instead of lynx_priv. - Remove teq from lynx_proto_params, since it can be determined from preq_ratio/postq_ratio. - Fix an early return from lynx_set_mode not releasing serdes->lock. - Rename lynx_priv.conf to .cfg, since I kept mistyping it. Changes in v3: - Manually expand yaml references - Add mode configuration to device tree - Rename remaining references to QorIQ SerDes to Lynx 10G - Fix PLL enable sequence by waiting for our reset request to be cleared before continuing. Do the same for the lock, even though it isn't as critical. Because we will delay for 1.5ms on average, use prepare instead of enable so we can sleep. - Document the status of each protocol - Fix offset of several bitfields in RECR0 - Take into account PLLRST_B, SDRST_B, and SDEN when considering whether a PLL is "enabled." - Only power off unused lanes. - Split mode lane mask into first/last lane (like group) - Read modes from device tree - Use caps to determine whether KX/KR are supported - Move modes to lynx_priv - Ensure that the protocol controller is not already in-use when we try to configure a new mode. This should only occur if the device tree is misconfigured (e.g. when QSGMII is selected on two lanes but there is only one QSGMII controller). - Split PLL drivers off into their own file - Add clock for "ext_dly" instead of writing the bit directly (and racing with any clock code). - Use kasprintf instead of open-coding the snprintf dance - Support 1000BASE-KX in lynx_lookup_proto. This still requires PCS support, so nothing is truly "enabled" yet. - Describe modes in device tree - ls1088a: Add serdes bindings Changes in v2: - Rename to fsl,lynx-10g.yaml - Refer to the device in the documentation, rather than the binding - Move compatible first - Document phy cells in the description - Allow a value of 1 for phy-cells. This allows for compatibility with the similar (but according to Ioana Ciornei different enough) lynx-28g binding. - Remove minItems - Use list for clock-names - Fix example binding having too many cells in regs - Add #clock-cells. This will allow using assigned-clocks* to configure the PLLs. - Document the structure of the compatible strings - Rename driver to Lynx 10G (etc.) - Fix not clearing group->pll after disabling it - Support 1 and 2 phy-cells - Power off lanes during probe - Clear SGMIIaCR1_PCS_EN during probe - Rename LYNX_PROTO_UNKNOWN to LYNX_PROTO_NONE - Handle 1000BASE-KX in lynx_proto_mode_prep - Use one phy cell for SerDes1, since no lanes can be grouped - Disable SerDes by default to prevent breaking boards inadvertently. Sean Anderson (15): dt-bindings: phy: Add 2500BASE-X and 10GBASE-R dt-bindings: phy: Add Lynx 10G phy binding dt-bindings: Convert gpio-mmio to yaml dt-bindings: gpio-mmio: Add compatible for QIXIS dt-bindings: clock: Add ids for Lynx 10g PLLs clk: Add Lynx 10G SerDes PLL driver phy: fsl: Add Lynx 10G SerDes driver phy: lynx10g: Enable by default on Layerscape arm64: dts: ls1046a: Add serdes nodes arm64: dts: ls1046ardb: Add serdes descriptions arm64: dts: ls1088a: Add serdes nodes arm64: dts: ls1088a: Prevent PCSs from probing as phys arm64: dts: ls1088ardb: Remove aquantia interrupt arm64: dts: ls1088ardb: Add SFP cage arm64: dts: ls1088ardb: Add serdes descriptions ...m6345-gpio.yaml => brcm,bcm63xx-gpio.yaml} | 18 +- .../devicetree/bindings/gpio/gpio-mmio.yaml | 123 ++ .../bindings/gpio/ni,169445-nand-gpio.txt | 38 - .../devicetree/bindings/gpio/wd,mbl-gpio.txt | 38 - .../mfd/brcm,bcm6318-gpio-sysctl.yaml | 4 +- .../mfd/brcm,bcm63268-gpio-sysctl.yaml | 4 +- .../mfd/brcm,bcm6328-gpio-sysctl.yaml | 4 +- .../mfd/brcm,bcm6358-gpio-sysctl.yaml | 4 +- .../mfd/brcm,bcm6362-gpio-sysctl.yaml | 4 +- .../mfd/brcm,bcm6368-gpio-sysctl.yaml | 4 +- .../devicetree/bindings/phy/fsl,lynx-10g.yaml | 248 ++++ Documentation/driver-api/phy/index.rst | 1 + Documentation/driver-api/phy/lynx_10g.rst | 58 + MAINTAINERS | 9 + .../boot/dts/freescale/fsl-ls1046a-rdb.dts | 26 + .../arm64/boot/dts/freescale/fsl-ls1046a.dtsi | 111 ++ .../boot/dts/freescale/fsl-ls1088a-rdb.dts | 82 +- .../arm64/boot/dts/freescale/fsl-ls1088a.dtsi | 156 ++- drivers/clk/Makefile | 1 + drivers/clk/clk-fsl-lynx-10g.c | 510 +++++++ drivers/phy/freescale/Kconfig | 23 + drivers/phy/freescale/Makefile | 1 + drivers/phy/freescale/phy-fsl-lynx-10g.c | 1224 +++++++++++++++++ include/dt-bindings/clock/fsl,lynx-10g.h | 14 + include/dt-bindings/phy/phy.h | 2 + include/linux/phy/lynx-10g.h | 16 + 26 files changed, 2607 insertions(+), 116 deletions(-) rename Documentation/devicetree/bindings/gpio/{brcm,bcm6345-gpio.yaml => brcm,bcm63xx-gpio.yaml} (75%) create mode 100644 Documentation/devicetree/bindings/gpio/gpio-mmio.yaml delete mode 100644 Documentation/devicetree/bindings/gpio/ni,169445-nand-gpio.txt delete mode 100644 Documentation/devicetree/bindings/gpio/wd,mbl-gpio.txt create mode 100644 Documentation/devicetree/bindings/phy/fsl,lynx-10g.yaml create mode 100644 Documentation/driver-api/phy/lynx_10g.rst create mode 100644 drivers/clk/clk-fsl-lynx-10g.c create mode 100644 drivers/phy/freescale/phy-fsl-lynx-10g.c create mode 100644 include/dt-bindings/clock/fsl,lynx-10g.h create mode 100644 include/linux/phy/lynx-10g.h