Re: [PATCH net-next 17/20] net: ethernet: qualcomm: Add PPE UNIPHY support for phylink

From: Russell King (Oracle)
Date: Wed Jan 10 2024 - 07:11:11 EST

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!