linux-kernel - Re: [PATCH] net: phy: add support for Motorcomm yt8531C phy

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]

Message-ID: <Y0TMIMPJbOETFQ3f@lunn.ch>
Date:   Tue, 11 Oct 2022 03:51:28 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Furkan Kardame <f.kardame@...jaro.org>
Cc:     pgwipeout@...il.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: add support for Motorcomm yt8531C phy

> +#define YT8531_RGMII_CONFIG1	0xa003
> +
> +/* TX Gig-E Delay is bits 3:0, default 0x1
> + * TX Fast-E Delay is bits 7:4, default 0xf
> + * RX Delay is bits 13:10, default 0x0
> + * Delay = 150ps * N
> + * On = 2000ps, off = 50ps
> + */
> +#define YT8531_DELAY_GE_TX_EN	(0xd << 0)
> +#define YT8531_DELAY_GE_TX_DIS	(0x0 << 0)

The comments above and the value here don't correspond.  These seem to
be enable/disable, which is usually a single bit. Here you have 3 bits
set? And what about the default 0x1?

0xd is 13. 13*150 = 1950, which is about 2000ps?

So YT8531_DELAY_GE_TX_EN is not really enable, it is
YT8531_DELAY_GE_TX_1950_PS, and YT8531_DELAY_GE_TX_DIS should be
YT8531_DELAY_GE_TX_0_PS.

> +#define YT8531_DELAY_FE_TX_EN	(0xd << 4)
> +#define YT8531_DELAY_FE_TX_DIS	(0x0 << 4)

Default is 0xf?

> +#define YT8531_DELAY_RX_EN	(0xd << 10)
> +#define YT8531_DELAY_RX_DIS	(0x0 << 10)
> +#define YT8531_DELAY_MASK	(GENMASK(13, 10) | GENMASK(7, 0))

Please rework these.

> +	ret = __phy_write(phydev, YT8511_PAGE, YT8531_CLKCFG_125M);
> +	if (ret < 0)
> +		goto err_restore_page;

This if statement is pointless.

> +
> +err_restore_page:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +