[PATCH v14 00/15] phy: Add support for Lynx 10G SerDes

[Sean Anderson]

On 4/26/23 06:51, Vladimir Oltean wrote:
> On Tue, Apr 25, 2023 at 04:22:32PM -0400, Sean Anderson wrote:
>> The features which do not work (major protocol changes) are disabled :)
>>
>> If it would cause this series to be immediately merged, I would remove
>> KX/KR and 2.5G which are the only untested link modes.
>>
>> That said, PCS support is necessary for these modes, so it is not even
>> possible to select them.
>>
>>> If you do not have the time to fix up the loose ends
>>> for this patch submission now
>>
>> I have time to fix up any loose ends preventing this series from being
>> applied. However, I am not very sympathetic to larger requests, since
>> there has been extensive time to supply feedback already.
>>
>>> , 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 gotten no substantive feedback on this driver. I have been
>> working on this series since June last year, and no one has commented on
>> the core algotithms thus far. My capacity for making large changes has
>> decreased because the project funding the development has ended. I
>> appreciate that you are taking interest now, but frankly I think it is
>> rather past time...
>>
>>> 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.
>>
>> This patchset has been ready to merge for several revisions now. I do
>> not consider it problematic. However, I do consider the (nonexistant)
>> review process for this subsystem extremely problematic.
> 
> To be very clear, the "larger request" which you are unsympathetic to is
> to wait. I didn't ask you to change anything.

The maintainers in this subsystem refuse to review any patches which have
any kind of feedback. I have been trying to get them to look at this series
for literal months. So saying things like "I don't like this series, have
a NACK" seriously delays the process of getting feedback...

> 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
- The alternative is polling once a second (not terribly intensive)

I think it's very reasonable to make this change. Anyway, it's in a separate
patch so that it can be applied independently.

and "doesn't changing PLL frequencies on the
> fly affect other lanes that use those PLLs, like PCIe?".

This driver is not enable on any boards with PCIe on the same serdes. Therefore,
this is irrelevant for the initial submission.

> The cognitive
> dissonance between this and you saying that the review process for this
> subsystem is absent is mind boggling. You are sufficiently averse to
> feedback that's not the feedback you want to hear, that it's hard to
> find a common ground.

I'm opposed to nebulous 11th hour objections.

> It's naive to expect that the silicon vendor will respond positively to
> a change of such magnitude as this one, which was written using the
> "works for me" work ethics, but which the silicon vendor will have to
> work with, afterwards. The only reason I sent my previous email was to
> announce you in advance that I have managed to carve out a sufficient
> amount of time to explore the topic in detail, and to stop you from
> pushing this forward in this state. I thought you would understand the
> concept of engineers being unable to easily reserve large chunks of time
> for a given project, after all, you brought this argument with your own
> company...
> 
> 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.

> https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2flinux%2darm%2dkernel%2fd3163201%2d2012%2d6cf9%2dc798%2d916bab9c7f72%40seco.com%2f&umid=b7a538d4-355b-4a0a-bac8-26f0fa0a74c0&auth=d807158c60b7d2502abde8a2fc01f40662980862-e02727a0e4438c72b5e854cb20b9de5379470fd9
> Even so, I have not said anything definitive, I have just requested time
> to take your proposal at face value, and understand whether there is any
> other alternative.
> 
> I would advise you to consider whether your follow-up emails on this
> topic encourage a collaborative atmosphere.

Sorry, I was a bit standoffish, but frankly I don't think leading off with
"NACK, no feedback for you today" is particularly collaborative either.

--Sean