netdev - Re: [RFC PATCH net-next v3 13/20] net: dsa: qca8k: make rgmii delay configurable

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]

Date:   Wed, 5 May 2021 03:07:15 +0200
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH net-next v3 13/20] net: dsa: qca8k: make rgmii delay
 configurable

On Wed, May 05, 2021 at 03:00:45AM +0200, Andrew Lunn wrote:
> On Wed, May 05, 2021 at 12:29:07AM +0200, Ansuel Smith wrote:
> > The legacy qsdk code used a different delay instead of the max value.
> > Qsdk use 1 ps for rx and 2 ps for tx. Make these values configurable
> > using the standard rx/tx-internal-delay-ps ethernet binding and apply
> > qsdk values by default. The connected gmac doesn't add any delay so no
> > additional delay is added to tx/rx.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@...il.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 51 +++++++++++++++++++++++++++++++++++++++--
> >  drivers/net/dsa/qca8k.h | 11 +++++----
> >  2 files changed, 55 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 22334d416f53..cb9b44769e92 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -779,6 +779,47 @@ qca8k_setup_mdio_bus(struct qca8k_priv *priv)
> >  	return 0;
> >  }
> >  
> > +static int
> > +qca8k_setup_of_rgmii_delay(struct qca8k_priv *priv)
> > +{
> > +	struct device_node *ports, *port;
> > +	u32 val;
> > +
> > +	ports = of_get_child_by_name(priv->dev->of_node, "ports");
> > +	if (!ports)
> > +		return -EINVAL;
> > +
> > +	/* Assume only one port with rgmii-id mode */
> > +	for_each_available_child_of_node(ports, port) {
> 
> Are delays global? Or per port? They really should be per port.  If it
> is global, one value that applies to all ports, i would not use
> it. Have the PHY apply the delay, not the MAC.
> 
>     Andrew

It's expected to set the delay in the port node. But yes the value is
global and assigned to every port (in reality this is only assigned to
the unique rgmii cpu port). The switch has only one rgmii port
and one sgmii, that's why I skipped any logic about handling more than
one case with internal delay. If you want I can try to add some logic to
handle this value per port. But again on the switch there is only one
reg to set the delay and the qca8k_phylink_mac_config function use this
only for the rgmii cpu port.