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

[Sean Anderson <sean.anderson@xxxxxxxx>]

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@xxxxxxxxxx/

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