netdev - Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

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: <Yd4cgGZ2tHzjBLqS@lunn.ch>
Date:   Wed, 12 Jan 2022 01:10:40 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Robert Hancock <robert.hancock@...ian.com>
Cc:     netdev@...r.kernel.org, hkallweit1@...il.com,
        linux@...linux.org.uk, davem@...emloft.net, kuba@...nel.org
Subject: Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

>  #define AT803X_MODE_CFG_MASK			0x0F
> -#define AT803X_MODE_CFG_SGMII			0x01
> +#define AT803X_MODE_CFG_BASET_RGMII		0x00
> +#define AT803X_MODE_CFG_BASET_SGMII		0x01
> +#define AT803X_MODE_CFG_BX1000_RGMII_50		0x02
> +#define AT803X_MODE_CFG_BX1000_RGMII_75		0x03
> +#define AT803X_MODE_CFG_BX1000_CONV_50		0x04
> +#define AT803X_MODE_CFG_BX1000_CONV_75		0x05
> +#define AT803X_MODE_CFG_FX100_RGMII_50		0x06
> +#define AT803X_MODE_CFG_FX100_CONV_50		0x07
> +#define AT803X_MODE_CFG_RGMII_AUTO_MDET		0x0B
> +#define AT803X_MODE_CFG_FX100_RGMII_75		0x0E
> +#define AT803X_MODE_CFG_FX100_CONV_75		0x0F

Hi Robert

What do these _50, and _75 mean?

 
>  #define AT803X_PSSR				0x11	/*PHY-Specific Status Register*/
>  #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
> @@ -283,6 +295,8 @@ struct at803x_priv {
>  	u16 clk_25m_mask;
>  	u8 smarteee_lpi_tw_1g;
>  	u8 smarteee_lpi_tw_100m;
> +	bool is_fiber;

Is maybe is_100basefx a better name? It makes it clearer it represents
a link mode?

> +	bool is_1000basex;
>  	struct regulator_dev *vddio_rdev;
>  	struct regulator_dev *vddh_rdev;
>  	struct regulator *vddio;
> @@ -784,7 +798,33 @@ static int at803x_probe(struct phy_device *phydev)
>  			return ret;
>  	}
>  
> +	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> +		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> +		int mode_cfg;
> +
> +		if (ccr < 0)
> +			goto err;
> +		mode_cfg = ccr & AT803X_MODE_CFG_MASK;
> +
> +		switch (mode_cfg) {
> +		case AT803X_MODE_CFG_BX1000_RGMII_50:
> +		case AT803X_MODE_CFG_BX1000_RGMII_75:
> +			priv->is_1000basex = true;
> +			fallthrough;
> +		case AT803X_MODE_CFG_FX100_RGMII_50:
> +		case AT803X_MODE_CFG_FX100_RGMII_75:
> +			priv->is_fiber = true;

O.K, now i'm wondering what AT803X_MODE_CFG_FX100_* actually means. I
was thinking it indicated 100BaseFX? But the fall through suggests
otherwise.

>  static int at803x_config_init(struct phy_device *phydev)
>  {
> +	struct at803x_priv *priv = phydev->priv;
>  	int ret;
>  
>  	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> -	       /* Some bootloaders leave the fiber page selected.
> -		* Switch to the copper page, as otherwise we read
> -		* the PHY capabilities from the fiber side.
> -		*/
> +		/* Some bootloaders leave the fiber page selected.

Looks like you have a tab vs space problem with the previous patch?
Otherwise this first line should not of changed.

	  Andrew