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
