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.

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!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel