netdev - Re: [v7, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for mt2712

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: <20181213123346.GF1605@lunn.ch>
Date:   Thu, 13 Dec 2018 13:33:46 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Biao Huang <biao.huang@...iatek.com>
Cc:     davem@...emloft.net, robh+dt@...nel.org,
        honghui.zhang@...iatek.com, yt.shen@...iatek.com,
        liguo.zhang@...iatek.com, mark.rutland@....com,
        nelson.chang@...iatek.com, matthias.bgg@...il.com,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, joabreu@...opsys.com
Subject: Re: [v7, PATCH 1/2] net:stmmac: dwmac-mediatek: add support for
 mt2712

Hi Biao

> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* the PHY is not responsible for inserting any internal
> +		 * delay by itself in PHY_INTERFACE_MODE_RGMII case,
> +		 * so Ethernet MAC will insert delays for both transmit
> +		 * and receive path here.
> +		 */

What if the PCB designed has decided to do a kink in the clock to add
the delays? I don't think any of these delays should depend on the PHY
interface mode. It is up to the device tree writer to set both the PHY
delay and the MAC delay, based on knowledge of the board, including
any kicks in the tracks. The driver should then do what it is told.

> +	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
> +		if (tx_delay_ps < plat->variant->tx_delay_max) {
> +			mac_delay->tx_delay = tx_delay_ps;
> +		} else {
> +			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (!of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps)) {
> +		if (rx_delay_ps < plat->variant->rx_delay_max) {
> +			mac_delay->rx_delay = rx_delay_ps;
> +		} else {
> +			dev_err(plat->dev, "Invalid RX clock delay: %dps\n", rx_delay_ps);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	mac_delay->tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse");
> +	mac_delay->rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse");
> +	mac_delay->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune");

Why is fine tune needed? If the requested delay can be done using fine
tune, it should use fine tune. If not, it should use rough tune. The
driver can work this out itself.

       Thanks
	Andrew