Message-ID: <ZZ6JCIQSimPy0TVY@shell.armlinux.org.uk>
Date: Wed, 10 Jan 2024 12:09:44 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Luo Jie <quic_luoj@...cinc.com>
Cc: agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, robh+dt@...nel.org,
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
	corbet@....net, catalin.marinas@....com, will@...nel.org,
	p.zabel@...gutronix.de, shannon.nelson@....com,
	anthony.l.nguyen@...el.com, jasowang@...hat.com,
	brett.creeley@....com, rrameshbabu@...dia.com,
	joshua.a.hay@...el.com, arnd@...db.de, geert+renesas@...der.be,
	neil.armstrong@...aro.org, dmitry.baryshkov@...aro.org,
	nfraprado@...labora.com, m.szyprowski@...sung.com, u-kumar1@...com,
	jacob.e.keller@...el.com, andrew@...n.ch, netdev@...r.kernel.org,
	linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, ryazanov.s.a@...il.com,
	ansuelsmth@...il.com, quic_kkumarcs@...cinc.com,
	quic_suruchia@...cinc.com, quic_soni@...cinc.com,
	quic_pavir@...cinc.com, quic_souravp@...cinc.com,
	quic_linchen@...cinc.com, quic_leiwei@...cinc.com
Subject: Re: [PATCH net-next 17/20] net: ethernet: qualcomm: Add PPE UNIPHY
 support for phylink

On Wed, Jan 10, 2024 at 07:40:29PM +0800, Luo Jie wrote:
> +static int clk_uniphy_set_rate(struct clk_hw *hw, unsigned long rate,
> +			       unsigned long parent_rate)
> +{
> +	struct clk_uniphy *uniphy = to_clk_uniphy(hw);
> +
> +	if (rate != UNIPHY_CLK_RATE_125M && rate != UNIPHY_CLK_RATE_312P5M)
> +		return -1;

Sigh. I get very annoyed off by stuff like this. It's lazy programming,
and makes me wonder why I should be bothered to spend time reviewing if
the programmer can't be bothered to pay attention to details. It makes
me wonder what else is done lazily in the patch.

-1 is -EPERM "Operation not permitted". This is highly likely not an
appropriate error code for this code.

> +int ppe_uniphy_autoneg_complete_check(struct ppe_uniphy *uniphy, int port)
> +{
> +	u32 reg, val;
> +	int channel, ret;
> +
> +	if (uniphy->interface == PHY_INTERFACE_MODE_USXGMII ||
> +	    uniphy->interface == PHY_INTERFACE_MODE_QUSGMII) {
> +		/* Only uniphy0 may have multi channels */
> +		channel = (uniphy->index == 0) ? (port - 1) : 0;
> +		reg = (channel == 0) ? VR_MII_AN_INTR_STS_ADDR :
> +		       VR_MII_AN_INTR_STS_CHANNEL_ADDR(channel);
> +
> +		/* Wait auto negotiation complete */
> +		ret = read_poll_timeout(ppe_uniphy_read, val,
> +					(val & CL37_ANCMPLT_INTR),
> +					1000, 100000, true,
> +					uniphy, reg);
> +		if (ret) {
> +			dev_err(uniphy->ppe_dev->dev,
> +				"uniphy %d auto negotiation timeout\n", uniphy->index);
> +			return ret;
> +		}
> +
> +		/* Clear auto negotiation complete interrupt */
> +		ppe_uniphy_mask(uniphy, reg, CL37_ANCMPLT_INTR, 0);
> +	}
> +
> +	return 0;
> +}

Why is this necessary? Why is it callable outside this file? Shouldn't
this be done in the .pcs_get_state method? If negotiation hasn't
completed (and negotiation is being used) then .pcs_get_state should not
report that the link is up.

> +
> +int ppe_uniphy_speed_set(struct ppe_uniphy *uniphy, int port, int speed)
> +{
> +	u32 reg, val;
> +	int channel;
> +
> +	if (uniphy->interface == PHY_INTERFACE_MODE_USXGMII ||
> +	    uniphy->interface == PHY_INTERFACE_MODE_QUSGMII) {
> +		/* Only uniphy0 may have multiple channels */
> +		channel = (uniphy->index == 0) ? (port - 1) : 0;
> +
> +		reg = (channel == 0) ? SR_MII_CTRL_ADDR :
> +		       SR_MII_CTRL_CHANNEL_ADDR(channel);
> +
> +		switch (speed) {
> +		case SPEED_100:
> +			val = USXGMII_SPEED_100;
> +			break;
> +		case SPEED_1000:
> +			val = USXGMII_SPEED_1000;
> +			break;
> +		case SPEED_2500:
> +			val = USXGMII_SPEED_2500;
> +			break;
> +		case SPEED_5000:
> +			val = USXGMII_SPEED_5000;
> +			break;
> +		case SPEED_10000:
> +			val = USXGMII_SPEED_10000;
> +			break;
> +		case SPEED_10:
> +			val = USXGMII_SPEED_10;
> +			break;
> +		default:
> +			val = 0;
> +			break;
> +		}
> +
> +		ppe_uniphy_mask(uniphy, reg, USXGMII_SPEED_MASK, val);
> +	}
> +
> +	return 0;
> +}
> +
> +int ppe_uniphy_duplex_set(struct ppe_uniphy *uniphy, int port, int duplex)
> +{
> +	u32 reg;
> +	int channel;
> +
> +	if (uniphy->interface == PHY_INTERFACE_MODE_USXGMII &&
> +	    uniphy->interface == PHY_INTERFACE_MODE_QUSGMII) {
> +		/* Only uniphy0 may have multiple channels */
> +		channel = (uniphy->index == 0) ? (port - 1) : 0;
> +
> +		reg = (channel == 0) ? SR_MII_CTRL_ADDR :
> +		       SR_MII_CTRL_CHANNEL_ADDR(channel);
> +
> +		ppe_uniphy_mask(uniphy, reg, USXGMII_DUPLEX_FULL,
> +				(duplex == DUPLEX_FULL) ? USXGMII_DUPLEX_FULL : 0);
> +	}
> +
> +	return 0;
> +}

What calls the above two functions? Surely this should be called from
the .pcs_link_up method? I would also imagine that you call each of
these consecutively. So why not modify the register in one go rather
than piecemeal like this. I'm not a fan of one-function-to-control-one-
parameter-in-a-register style when it results in more register accesses
than are really necessary.

> +static void ppe_pcs_get_state(struct phylink_pcs *pcs,
> +			      struct phylink_link_state *state)
> +{
> +	struct ppe_uniphy *uniphy = pcs_to_ppe_uniphy(pcs);
> +	u32 val;
> +
> +	switch (state->interface) {
> +	case PHY_INTERFACE_MODE_10GBASER:
> +		val = ppe_uniphy_read(uniphy, SR_XS_PCS_KR_STS1_ADDR);
> +		state->link = (val & SR_XS_PCS_KR_STS1_PLU) ? 1 : 0;

Unnecessary tenary operation.

		state->link = !!(val & SR_XS_PCS_KR_STS1_PLU);

> +		state->duplex = DUPLEX_FULL;
> +		state->speed = SPEED_10000;
> +		state->pause |= (MLO_PAUSE_RX | MLO_PAUSE_TX);

Excessive parens.

> +		break;
> +	case PHY_INTERFACE_MODE_2500BASEX:
> +		val = ppe_uniphy_read(uniphy, UNIPHY_CHANNEL0_INPUT_OUTPUT_6_ADDR);
> +		state->link = (val & NEWADDEDFROMHERE_CH0_LINK_MAC) ? 1 : 0;

Ditto.

> +		state->duplex = DUPLEX_FULL;
> +		state->speed = SPEED_2500;
> +		state->pause |= (MLO_PAUSE_RX | MLO_PAUSE_TX);

Ditto.

> +		break;
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +	case PHY_INTERFACE_MODE_SGMII:
> +		val = ppe_uniphy_read(uniphy, UNIPHY_CHANNEL0_INPUT_OUTPUT_6_ADDR);
> +		state->link = (val & NEWADDEDFROMHERE_CH0_LINK_MAC) ? 1 : 0;
> +		state->duplex = (val & NEWADDEDFROMHERE_CH0_DUPLEX_MODE_MAC) ?
> +			DUPLEX_FULL : DUPLEX_HALF;
> +		if (FIELD_GET(NEWADDEDFROMHERE_CH0_SPEED_MODE_MAC, val) == UNIPHY_SPEED_10M)
> +			state->speed = SPEED_10;
> +		else if (FIELD_GET(NEWADDEDFROMHERE_CH0_SPEED_MODE_MAC, val) == UNIPHY_SPEED_100M)
> +			state->speed = SPEED_100;
> +		else if (FIELD_GET(NEWADDEDFROMHERE_CH0_SPEED_MODE_MAC, val) == UNIPHY_SPEED_1000M)
> +			state->speed = SPEED_1000;

Looks like a switch(FIELD_GET(NEWADDEDFROMHERE_CH0_SPEED_MODE_MAC, val)
would be better here. Also "NEWADDEDFROMHERE" ?

> +		state->pause |= (MLO_PAUSE_RX | MLO_PAUSE_TX);

Ditto.

As you make no differentiation between 1000base-X and SGMII, I question
whether your hardware supports 1000base-X. I seem to recall in previous
discussions that it doesn't. So, that means it doesn't support the
inband negotiation word format for 1000base-X. Thus, 1000base-X should
not be included in any of these switch statements, and 1000base-X won't
be usable.

> +/* [register] UNIPHY_MODE_CTRL */
> +#define UNIPHY_MODE_CTRL_ADDR				0x46c
> +#define NEWADDEDFROMHERE_CH0_AUTONEG_MODE		BIT(0)
> +#define NEWADDEDFROMHERE_CH1_CH0_SGMII			BIT(1)
> +#define NEWADDEDFROMHERE_CH4_CH1_0_SGMII		BIT(2)
> +#define NEWADDEDFROMHERE_SGMII_EVEN_LOW			BIT(3)
> +#define NEWADDEDFROMHERE_CH0_MODE_CTRL_25M		GENMASK(6, 4)
> +#define NEWADDEDFROMHERE_CH0_QSGMII_SGMII		BIT(8)
> +#define NEWADDEDFROMHERE_CH0_PSGMII_QSGMII		BIT(9)
> +#define NEWADDEDFROMHERE_SG_MODE			BIT(10)
> +#define NEWADDEDFROMHERE_SGPLUS_MODE			BIT(11)
> +#define NEWADDEDFROMHERE_XPCS_MODE			BIT(12)
> +#define NEWADDEDFROMHERE_USXG_EN			BIT(13)
> +#define NEWADDEDFROMHERE_SW_V17_V18			BIT(15)

Again, why "NEWADDEDFROMHERE" ?

> +/* [register] VR_XS_PCS_EEE_MCTRL0 */
> +#define VR_XS_PCS_EEE_MCTRL0_ADDR			0x38006
> +#define LTX_EN						BIT(0)
> +#define LRX_EN						BIT(1)
> +#define SIGN_BIT					BIT(6)

"SIGN_BIT" is likely too generic a name.

> +#define MULT_FACT_100NS					GENMASK(11, 8)
> +
> +/* [register] VR_XS_PCS_KR_CTRL */
> +#define VR_XS_PCS_KR_CTRL_ADDR	0x38007
> +#define USXG_MODE					GENMASK(12, 10)
> +#define QUXGMII_MODE					(FIELD_PREP(USXG_MODE, 0x5))
> +
> +/* [register] VR_XS_PCS_EEE_TXTIMER */
> +#define VR_XS_PCS_EEE_TXTIMER_ADDR			0x38008
> +#define TSL_RES						GENMASK(5, 0)
> +#define T1U_RES						GENMASK(7, 6)
> +#define TWL_RES						GENMASK(12, 8)
> +#define UNIPHY_XPCS_TSL_TIMER				(FIELD_PREP(TSL_RES, 0xa))
> +#define UNIPHY_XPCS_T1U_TIMER				(FIELD_PREP(TSL_RES, 0x3))
> +#define UNIPHY_XPCS_TWL_TIMER				(FIELD_PREP(TSL_RES, 0x16))
> +
> +/* [register] VR_XS_PCS_EEE_RXTIMER  */
> +#define VR_XS_PCS_EEE_RXTIMER_ADDR			0x38009
> +#define RES_100U					GENMASK(7, 0)
> +#define TWR_RES						GENMASK(13, 8)
> +#define UNIPHY_XPCS_100US_TIMER				(FIELD_PREP(RES_100U, 0xc8))
> +#define UNIPHY_XPCS_TWR_TIMER				(FIELD_PREP(RES_100U, 0x1c))
> +
> +/* [register] VR_XS_PCS_DIG_STS */
> +#define VR_XS_PCS_DIG_STS_ADDR				0x3800a
> +#define AM_COUNT					GENMASK(14, 0)
> +#define QUXGMII_AM_COUNT				(FIELD_PREP(AM_COUNT, 0x6018))
> +
> +/* [register] VR_XS_PCS_EEE_MCTRL1 */
> +#define VR_XS_PCS_EEE_MCTRL1_ADDR			0x3800b
> +#define TRN_LPI						BIT(0)
> +#define TRN_RXLPI					BIT(8)
> +
> +/* [register] VR_MII_1_DIG_CTRL1 */
> +#define VR_MII_DIG_CTRL1_CHANNEL1_ADDR			0x1a8000
> +#define VR_MII_DIG_CTRL1_CHANNEL2_ADDR			0x1b8000
> +#define VR_MII_DIG_CTRL1_CHANNEL3_ADDR			0x1c8000
> +#define VR_MII_DIG_CTRL1_CHANNEL_ADDR(x)		(0x1a8000 + 0x10000 * ((x) - 1))
> +#define CHANNEL_USRA_RST				BIT(5)
> +
> +/* [register] VR_MII_AN_CTRL */
> +#define VR_MII_AN_CTRL_ADDR				0x1f8001
> +#define VR_MII_AN_CTRL_CHANNEL1_ADDR			0x1a8001
> +#define VR_MII_AN_CTRL_CHANNEL2_ADDR			0x1b8001
> +#define VR_MII_AN_CTRL_CHANNEL3_ADDR			0x1c8001
> +#define VR_MII_AN_CTRL_CHANNEL_ADDR(x)			(0x1a8001 + 0x10000 * ((x) - 1))
> +#define MII_AN_INTR_EN					BIT(0)
> +#define MII_CTRL					BIT(8)

Too generic a name.

> +
> +/* [register] VR_MII_AN_INTR_STS */
> +#define VR_MII_AN_INTR_STS_ADDR				0x1f8002
> +#define VR_MII_AN_INTR_STS_CHANNEL1_ADDR		0x1a8002
> +#define VR_MII_AN_INTR_STS_CHANNEL2_ADDR		0x1b8002
> +#define VR_MII_AN_INTR_STS_CHANNEL3_ADDR		0x1c8002
> +#define VR_MII_AN_INTR_STS_CHANNEL_ADDR(x)		(0x1a8002 + 0x10000 * ((x) - 1))
> +#define CL37_ANCMPLT_INTR				BIT(0)
> +
> +/* [register] VR_XAUI_MODE_CTRL */
> +#define VR_XAUI_MODE_CTRL_ADDR				0x1f8004
> +#define VR_XAUI_MODE_CTRL_CHANNEL1_ADDR			0x1a8004
> +#define VR_XAUI_MODE_CTRL_CHANNEL2_ADDR			0x1b8004
> +#define VR_XAUI_MODE_CTRL_CHANNEL3_ADDR			0x1c8004
> +#define VR_XAUI_MODE_CTRL_CHANNEL_ADDR(x)		(0x1a8004 + 0x10000 * ((x) - 1))
> +#define IPG_CHECK					BIT(0)
> +
> +/* [register] SR_MII_CTRL */
> +#define SR_MII_CTRL_ADDR				0x1f0000
> +#define SR_MII_CTRL_CHANNEL1_ADDR			0x1a0000
> +#define SR_MII_CTRL_CHANNEL2_ADDR			0x1b0000
> +#define SR_MII_CTRL_CHANNEL3_ADDR			0x1c0000
> +#define SR_MII_CTRL_CHANNEL_ADDR(x)			(0x1a0000 + 0x10000 * ((x) - 1))


> +#define AN_ENABLE					BIT(12)

Looks like MDIO_AN_CTRL1_ENABLE

> +#define USXGMII_DUPLEX_FULL				BIT(8)
> +#define USXGMII_SPEED_MASK				(BIT(13) | BIT(6) | BIT(5))
> +#define USXGMII_SPEED_10000				(BIT(13) | BIT(6))
> +#define USXGMII_SPEED_5000				(BIT(13) | BIT(5))
> +#define USXGMII_SPEED_2500				BIT(5)
> +#define USXGMII_SPEED_1000				BIT(6)
> +#define USXGMII_SPEED_100				BIT(13)
> +#define USXGMII_SPEED_10				0

Looks rather like the standard IEEE 802.3 definitions except for the
2.5G and 5G speeds. Probably worth a comment stating that they're
slightly different.

> +
> +/* PPE UNIPHY data type */
> +struct ppe_uniphy {
> +	void __iomem *base;
> +	struct ppe_device *ppe_dev;
> +	unsigned int index;
> +	phy_interface_t interface;
> +	struct phylink_pcs pcs;
> +};
> +
> +#define pcs_to_ppe_uniphy(_pcs)				container_of(_pcs, struct ppe_uniphy, pcs)

As this should only be used in the .c file, I suggest making this a
static function in the .c file. There should be no requirement to use
it outside of the .c file.

> +
> +struct ppe_uniphy *ppe_uniphy_setup(struct platform_device *pdev);
> +
> +int ppe_uniphy_speed_set(struct ppe_uniphy *uniphy,
> +			 int port, int speed);
> +
> +int ppe_uniphy_duplex_set(struct ppe_uniphy *uniphy,
> +			  int port, int duplex);
> +
> +int ppe_uniphy_adapter_reset(struct ppe_uniphy *uniphy,
> +			     int port);
> +
> +int ppe_uniphy_autoneg_complete_check(struct ppe_uniphy *uniphy,
> +				      int port);
> +
> +int ppe_uniphy_port_gcc_clock_en_set(struct ppe_uniphy *uniphy,
> +				     int port, bool enable);
> +
> +#endif /* _PPE_UNIPHY_H_ */
> diff --git a/include/linux/soc/qcom/ppe.h b/include/linux/soc/qcom/ppe.h
> index 268109c823ad..d3cb18df33fa 100644
> --- a/include/linux/soc/qcom/ppe.h
> +++ b/include/linux/soc/qcom/ppe.h
> @@ -20,6 +20,7 @@ struct ppe_device {
>  	struct dentry *debugfs_root;
>  	bool is_ppe_probed;
>  	void *ppe_priv;
> +	void *uniphy;

Not struct ppe_uniphy *uniphy? You can declare the struct before use
via:

struct ppe_uniphy;

so you don't need to include ppe_uniphy.h in this header.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists