linux-kernel - Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMdYzYrSB0G7jfG9fo85X0DxVG_r-qaWUyVAa5paAW0ugLvoxw@mail.gmail.com>
Date:   Fri, 14 May 2021 10:14:27 -0400
From:   Peter Geis <pgwipeout@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@...ts.infradead.org>
Subject: Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

On Fri, May 14, 2021 at 9:24 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> On Fri, May 14, 2021 at 07:58:26AM -0400, Peter Geis wrote:
> > Add a driver for the Motorcomm yt8511 phy that will be used in the
> > production Pine64 rk3566-quartz64 development board.
> > It supports gigabit transfer speeds, rgmii, and 125mhz clk output.
>
> Thanks for adding RGMII support.
>
> > +#define PHY_ID_YT8511                0x0000010a
>
> No OUI in the PHY ID?
>
> Humm, the datasheet says it defaults to zero. That is not very
> good. This could be a source of problems in the future, if some other
> manufacture also does not use an OUI.
>
> > +/* RX Delay enabled = 1.8ns 1000T, 8ns 10/100T */
> > +#define YT8511_DELAY_RX              BIT(0)
> > +
> > +/* TX Delay is bits 7:4, default 0x5
> > + * Delay = 150ps * N - 250ps, Default = 500ps
> > + */
> > +#define YT8511_DELAY_TX              (0x5 << 4)
>
> > +
> > +     switch (phydev->interface) {
> > +     case PHY_INTERFACE_MODE_RGMII:
> > +             val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> > +             break;
>
> This is not correct. YT8511_DELAY_TX will only mask the 2 bits in 0x5,
> not all the bits in 7:4. And since the formula is
>
> Delay = 150ps * N - 250ps
>
> setting N to 0 is not what you want. You probably want N=2, so you end up with 50ps

The manufacturer's driver set this to <0> to disable, but I agree the
datasheet disagrees with this.

>
> > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > +             val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +             val &= ~(YT8511_DELAY_TX);
> > +             val |= YT8511_DELAY_RX;
>
> The delay should be around 2ns. For RX you only have 1.8ns, which is
> probably good enough. But for TX you have more flexibility. You are
> setting it to the default of 500ps which is too small. I would suggest
> 1.85ns, N=14, so it is the same as RX.

Makes sense, these are the insights I was hoping for!

>
> I also wonder about bits 15:12 of PHY EXT ODH: Delay and driver
> strength CFG register.

The default value *works*, and from an emi perspective we want the
lowest strength single that is reliable.
Unfortunately I don't have the equipment to *test* the actual output
strengths and the datasheet isn't explicitly clear about them either.
This is one of the values I was considering having defined in the devicetree.

>
>          Andrew