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
