linux-kernel - Re: [PATCH v5 1/2] stmmac: dwmac-mediatek: enable 2ns delay only for special cases

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:   Mon, 26 Dec 2022 03:50:39 +0000
From:   Biao Huang (黄彪) <Biao.Huang@...iatek.com>
To:     "andrew@...n.ch" <andrew@...n.ch>
CC:     "linux-mediatek@...ts.infradead.org" 
        <linux-mediatek@...ts.infradead.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "joabreu@...opsys.com" <joabreu@...opsys.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "alexandre.torgue@...s.st.com" <alexandre.torgue@...s.st.com>,
        "mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "richardcochran@...il.com" <richardcochran@...il.com>,
        "krzysztof.kozlowski+dt@...aro.org" 
        <krzysztof.kozlowski+dt@...aro.org>,
        Macpaul Lin (林智斌) 
        <Macpaul.Lin@...iatek.com>,
        "matthias.bgg@...il.com" <matthias.bgg@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "angelogioacchino.delregno@...labora.com" 
        <angelogioacchino.delregno@...labora.com>
Subject: Re: [PATCH v5 1/2] stmmac: dwmac-mediatek: enable 2ns delay only for
 special cases

Dear Andrew,
	Thanks for your comments.

On Fri, 2022-12-23 at 15:18 +0100, Andrew Lunn wrote:
> On Fri, Dec 23, 2022 at 09:50:28AM +0800, Biao Huang wrote:
> > In current driver, MAC will always enable 2ns delay in RGMII mode,
> > but that will lead to transmission failures for "rgmii-id"/"rgmii-
> > txid"
> > cases.
> > 
> > Modify the implementation of fix_mac_speed() to ensure the 2ns
> > delay
> > will only take effect for "rgmii-rxid"/"rgmii" cases, then user can
> > choose phy-mode freely.
> 
> This does not seem correct. There are three ways the delays can be
> added:
> 
> 1) The MAC
> 2) The PHY
> 3) Extra long lines on the board.
> 
> What the four RGMII modes tell you is what is needed in addition to
> whatever the board provides. So it describes the combination of 1)
> and
> 2). Your board does not appear to be applying any delays, so you
> should be using rgmii-id.
> 
> The MAC and PHY driver then need to decide how to add these delays,
> and in most cases, the MAC does nothing, and passes phy-mode to the
> PHY and the PHY adds the delay.
> 
> The MAC can add delays, but if it does, it need to mask out the
> delays
> it added to the value passed to the PHY. Otherwise the PHY will add
> the delay as well.
in the ethernet-controller.yaml,
 77       # RX and TX delays are added by the MAC when required
 78       - rgmii
 79
 80       # RGMII with internal RX and TX delays provided by the PHY,
 81       # the MAC should not add the RX or TX delays in this case
 82       - rgmii-id
 83
 84       # RGMII with internal RX delay provided by the PHY, the MAC
 85       # should not add an RX delay in this case
 86       - rgmii-rxid
 87
 88       # RGMII with internal TX delay provided by the PHY, the MAC
 89       # should not add an TX delay in this case
 90       - rgmii-txid

so, why don't MAC set delay according to rgmii-** ?
rgmii-rxid   --> mac set tx, but not set rx delay
rgmii-txid   --> mac set rx, but not set tx delay
...

and PHY seems set delay according to rgmii-** in their dirver.

> 
> >  
> > -	if ((phy_interface_mode_is_rgmii(priv_plat->phy_mode))) {
> > +	switch (priv_plat->phy_mode) {
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> >  		/* prefer 2ns fixed delay which is controlled by
> > TXC_PHASE_CTRL,
> >  		 * when link speed is 1Gbps with RGMII interface,
> >  		 * Fall back to delay macro circuit for 10/100Mbps link
> > speed.
> 
> So this is wrong. PHY_INTERFACE_MODE_RGMII means the board is adding
> the delay via long lines. You should not be added any delay at all.
> 
> For PHY_INTERFACE_MODE_RGMII_RXID, you need to mask the RXID bit from
> phy_mode when connecting the MAC to the PHY. Otherwise the PHY is
> going to add this delay as well.

These lines only set tx delay when phy_mode is RGMII/RGMI_RXID, when
phy will not add tx delay internally.
We don't want to restrict phy-mode to rgmii-id only, so I don't think
we need mask RXID bit, then, User can use rgmii-rxid/-txid/-id as their
will.

If I misunderstood, please correct me. Thanks.
> 
> 	 Andrew
Best Regards!
Biao