Re: [PATCH v2 1/2] dt-bindings: phy: mediatek,mt7988-xfi-tphy: add new bindings

[Daniel Golle <daniel@makrotopia.org>]

Hi Krzysztof,

On Tue, Feb 06, 2024 at 11:53:55AM +0100, Krzysztof Kozlowski wrote:
> On 05/02/2024 18:28, Daniel Golle wrote:
> > Add bindings for the MediaTek XFI Ethernet SerDes T-PHY found in the
> > MediaTek MT7988 SoC which can operate at various interfaces modes:
> > 
> > via USXGMII PCS:
> >  * USXGMII
> >  * 10GBase-R
> >  * 5GBase-R
> > 
> > via LynxI SGMII PCS:
> >  * 2500Base-X
> >  * 1000Base-X
> >  * Cisco SGMII (MAC side)
> > 
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> > v2: unify filename and compatible as requested
> 
> Several comments, from me and Rob, were ignored. Please respond to them.

I'm sorry if I have missed something. I just checked again on
patchwork, just in case I would have missed an email reply to this or
any of the preceding posts of this patch as part of the old series
going to netdev.

Comments you have made which I have addressed:
 - removed $nodename
 - use compatible as filename

And the only thing I found that I didn't either fix or reply to is this:
> Can you explain what is this issue and errata about (except performance)?

Not overwriting that (undocumented) value in that (undocumented)
register results in 10GBase-R having performance issues according to a
commit in MediaTek's SDK, see here:

https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/a500d94cd%5E%21/#F0

Maybe Bc or SkyLake of MediaTek (added to Cc) can explain this in more
detail?


What I did miss was Rob's comment at the very bottom of this reply:
> What is PEXTP?

I can again only answer by referencing to MediaTek's SDK sources:

https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/refs/heads/master/21.02/files/target/linux/mediatek/files-5.4/drivers/net/ethernet/mediatek/mtk_sgmii.c#96

Here this reset is called XFI_PEXTP0_GRST.

I personally find that name confusing (as this PHY has nothing to do with
_P_ci _EX_press) and have tried to get rid of it where it isn't either part
of official documentation or already merged drivers (like Sam's clock driver).

If there have been any other issues with this patch which I'm not aware
of, please point them out to me.


Thank you


Best regards


Daniel