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

[Ioana Ciornei <ioana.ciornei@xxxxxxx>]

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://lore.kernel.org/all/db9d9455-37af-1616-8f7f-3d752e7930f1@xxxxxxxxxx/

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://lore.kernel.org/netdev/20220628221404.1444200-5-sean.anderson@xxxxxxxx/

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

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

Ioana