linux-kernel - Re: [PATCH net-next v2 3/3] net: phy: mscc: RGMII skew delay configuration

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: <20200228172616.GG29979@lunn.ch>
Date:   Fri, 28 Feb 2020 18:26:16 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Antoine Tenart <antoine.tenart@...tlin.com>
Cc:     davem@...emloft.net, f.fainelli@...il.com, hkallweit1@...il.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        foss@...il.net
Subject: Re: [PATCH net-next v2 3/3] net: phy: mscc: RGMII skew delay
 configuration

> I did not know that, thanks for the explanation! So if ID/TXID/RXID is
> used, I should configure the Rx and/or Tx skew with
> VSC8584_RGMII_SKEW_2_0, except if the dt says otherwise.

Yes.

> > What is not clearly defined, is how you combine
> > PHY_INTERFACE_MODE_RGMII* with DT properties. I guess i would enforce
> > that phydev->interface is PHY_INTERFACE_MODE_RGMII and then the delays
> > in DT are absolute values.
> 
> So, if there's a value in the device tree, and the mode corresponds
> (RXID for Rx skew), we do use the dt value. That should look like what's
> in the patch (except for the default value used when no configuration is
> provided in the dt).

No. I would not do that. PHY_INTERFACE_MODE_RGMII_RXID means 2ns delay
for RX. So how do you then interpret the DT property. Is it 2ns + the
DT delay? That would then mean you need negative values in DT if you
want short delays than 2ns.

Which is why i suggest PHY_INTERFACE_MODE_RGMII. It is then clear you
have a base delay of 0ns, and the DT property then has the absolute
value of the delay.

> > There is also some discussion about what should go in DT. #defines
> > like you have, or time in pS, and the driver converts to register
> > values. I generally push towards pS.
> 
> That would allow a more generic dt binding, and could be used by other
> PHY drivers. The difficulty would be to map the pS to allowed values in
> the h/w. Should we round them to the upper or lower bound?

I would document the accepted values in DT, and return -EINVAL if DT
does not exactly match one of the listed values. Plus a phydev_err()
message to help debug.

> I also saw the micrel-ksz90x1 dt documentation describes many skews, not
> only one for Rx and one for Tx. How would the generic dt binding would
> look like then?

It is a balancing act. Do you actually need dt properties for your
hardware? Are the standard 2ns delays sufficient. For many designs it
is. Just because the hardware supports all sorts of configurations,
are they actually needed? It seems like adding delays are needed for a
few boards. But do all the properties exposed for the Micrel PHY every
get used, or is it a case of, the hardware has it, lets make it
available, even if nobody ever uses it?

So i think a standardized DT for delays is good, but i would not go
any further.

    Andrew