netdev - Re: [PATCH] net: thunderx: add support for rgmii internal delay

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] [day] [month] [year] [list]

Date:   Mon, 18 Dec 2017 14:10:14 -0800
From:   Tim Harvey <tharvey@...eworks.com>
To:     Andrew Lunn <andrew@...n.ch>, Sunil Goutham <sgoutham@...ium.com>
Cc:     netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] net: thunderx: add support for rgmii internal delay

On Thu, Dec 14, 2017 at 12:45 AM, Andrew Lunn <andrew@...n.ch> wrote:
> On Wed, Dec 13, 2017 at 03:28:33PM -0800, Tim Harvey wrote:
>> On Wed, Dec 13, 2017 at 3:10 AM, Andrew Lunn <andrew@...n.ch> wrote:
>> >> +void xcv_init_hw(int phy_mode)
>> >>  {
>> >>       u64  cfg;
>> >>
>> >> @@ -81,12 +81,31 @@ void xcv_init_hw(void)
>> >>       /* Wait for DLL to lock */
>> >>       msleep(1);
>> >>
>> >> -     /* Configure DLL - enable or bypass
>> >> -      * TX no bypass, RX bypass
>> >> -      */
>> >> +     /* enable/bypass DLL providing MAC based internal TX/RX delays */
>> >>       cfg = readq_relaxed(xcv->reg_base + XCV_DLL_CTL);
>> >> -     cfg &= ~0xFF03;
>> >> -     cfg |= CLKRX_BYP;
>> >> +     cfg &= ~0xffff00;
>> >> +     switch (phy_mode) {
>> >> +     /* RX and TX delays are added by the MAC */
>> >> +     case PHY_INTERFACE_MODE_RGMII:
>> >> +             break;
>> >> +     /* internal RX and TX delays provided by the PHY */
>> >> +     case PHY_INTERFACE_MODE_RGMII_ID:
>> >> +             cfg |= CLKRX_BYP;
>> >> +             cfg |= CLKTX_BYP;
>> >> +             break;
>> >> +     /* internal RX delay provided by the PHY, the MAC
>> >> +      * should not add an RX delay in this case
>> >> +      */
>> >> +     case PHY_INTERFACE_MODE_RGMII_RXID:
>> >> +             cfg |= CLKRX_BYP;
>> >> +             break;
>> >> +     /* internal TX delay provided by the PHY, the MAC
>> >> +      * should not add an TX delay in this case
>> >> +      */
>> >> +     case PHY_INTERFACE_MODE_RGMII_TXID:
>> >> +             cfg |= CLKRX_BYP;
>> >> +             break;
>> >> +     }
>> >
>> > Hi Tim
>> >
>> > This i don't get. Normally, you leave the PHY to handle delays, if
>> > needed. The MAC should not add any. Here you seem to assume a delay is
>> > always needed, and if the PHY is not providing it, the MAC should.
>> >
>> >        Andrew
>>
>> Andrew,
>>
>> The thunder RGX inserts a delay via an on-board DLL. The 'bypass'
>> register will bypass this DLL and not insert a delay from the MAC
>> side. By default out of reset CLKTX_BYP=1 causing the RGX transmit
>> interface to not introduce a delay and CLKRX_BYP=0 causing the RGX
>> receive interface to introduce a delay.
>
> Hi Tim
>
> So the MAC by default is doing PHY_INTERFACE_MODE_RGMII_RXID.  And it
> calls phy_connect_direct() passing PHY_INTERFACE_MODE_RGMII. It does
> not get anything from device tree. So it looks like we have a chance
> to clean this up.
>
> So the correct thing to do is set the MAC to PHY_INTERFACE_MODE_RGMII,
> i.e. no delays. By default call phy_connect_direct()
> PHY_INTERFACE_MODE_RGMII_RXID. That should give you the same behaviour
> as today.

I don't understand - PHY_INTERFACE_MODE_RGMII means delays are added by the MAC

The way I see it today the driver is making an assumption that is not
always correct. What is the right way to configure a MAC when no
phy-mode is present in the dts? I assumed it would be RGMII_ID such
that the MAC introduces no delay.

>
> Then add code to look in device tree, to find a per board setting. In
> your case, you want PHY_INTERFACE_MODE_RGMII_ID. And make sure the PHY
> driver respects the value passed.
>
>        Andrew
>

Should I be attempting to make the default if no phy-mode is in the
dts be PHY_INTERFACE_MODE_RGMII_RXID so that existing boards do not
break (as I assume they configure the phy's that way in firmware).

My original goal was to make the bgx driver flexible for different
delay configurations as well as allow phy drivers to be used. However
I found that the dp83867 driver doesn't work with my board anyway as
it issues a soft reset that disables CLKOUT which I setup in firmware
and require. Is it standard for phy drivers to issue hard or soft
resets during init and if so how do boards deal with custom LED or
CLKOUT configs as those don't seem to be supported by phy drivers? I
only have experience with phy drivers that support an optional hard
reset and if you don't want to reset any custom regs you simply don't
expose the phy_rst gpio (assuming there is on) to the driver by not
defining it in device-tree.

Tim