by Lei Wei

[permalink] [raw]

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

On 1/10/2024 8:09 PM, Russell King (Oracle) wrote:
> 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.
>
Sorry for this. I will update the driver to have appropriate error codes
where required.

>> +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.
>
Currently it is called outside this file in the following patch:
https://lore.kernel.org/netdev/[email protected]/

Yes, if inband autoneg is used, .pcs_get_state should report the link
status. Then this function should not be needed and should be removed.
I will update the .pcs_get_state method for USXGMII if using inband autoneg.

>> +
>> +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.
>

When we consider the sequence of operations expected from the driver by
the hardware, the MACand PCSoperations are interleaved. So we were not
able to clearly separate the MACand PCS operations during link up into
pcs_link_up() and .mac_link_up() ops. So we have avoided using
pcs_link_up() and included the entire sequence in mac_link_up() op.
This function is called by the PPE MAC support patch below.

https://lore.kernel.org/netdev/[email protected]/

The sequence expected by PPE HW from driver for link up is as below:
1. disable uniphy interface clock. (PCS operation)
2. configure the PPE port speed clock. (MAC operation)
3. configure the uniphy pcs speed for usxgmii (PCS operation).
4. configure PPE MAC speed (MAC operation).
5. enable uniphy interface clock (PCS operation).
6. reset uniphy pcs adapter (PCS operation).
7. enable mac (MAC operation).

I will also check whole patch to rework the
one-function-to-control-one-parameter-in-a-register style being used
here, thanks for the suggestion.

>> +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);
>

Sure, Thanks for the suggestion, I will update it.

>> + state->duplex = DUPLEX_FULL;
>> + state->speed = SPEED_10000;
>> + state->pause |= (MLO_PAUSE_RX | MLO_PAUSE_TX);
>
> Excessive parens.
>
Will update it.

>> + 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.
>
Will update it.

>> + state->duplex = DUPLEX_FULL;
>> + state->speed = SPEED_2500;
>> + state->pause |= (MLO_PAUSE_RX | MLO_PAUSE_TX);
>
> Ditto.
>
Will update it.

>> + 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" ?
>
Sorry for the confusion, I will translate the register to meaningful name.

>> + state->pause |= (MLO_PAUSE_RX | MLO_PAUSE_TX);
>
> Ditto.
>
Will update it.

> 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.
>
Our hardware supports both 1000base-x and SGMII auto-neg, the hardware
can resolve and decode the autoneg result of 1000base-x C37 word format
and SGMII auto-neg word format. This information after autoneg
resolution is stored in the same register exported to software. This is
the reason the same code works for both cases.

>> +/* [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" ?
>
Will rename to use proper name.

>> +/* [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.
>
As above, will rename the register to proper 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.
>
Will update and rename it.

>> +
>> +/* [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
>

This is the uniphy xpcs autoneg enable control bit, our uniphy is not
MDIO accessed, I will rename it to a meaningful name.

>> +#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.
>

Sure, will add comment for it in code and documentation files, thanks.

>> +
>> +/* 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.
>

This is used in the following patch as I explained above for the MAC/PCS
related comment:
https://lore.kernel.org/netdev/[email protected]/

>> +
>> +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 for the good suggestion, will follow this.

> Thanks.
>

2024-01-22 15:31:52

by Lei Wei

[permalink] [raw]

Subject: Re: [PATCH net-next 18/20] net: ethernet: qualcomm: Add PPE MAC support for phylink

On 1/10/2024 8:18 PM, Russell King (Oracle) wrote:
> On Wed, Jan 10, 2024 at 07:40:30PM +0800, Luo Jie wrote:
>> +static void ppe_phylink_mac_link_up(struct ppe_device *ppe_dev, int port,
>> + struct phy_device *phy,
>> + unsigned int mode, phy_interface_t interface,
>> + int speed, int duplex, bool tx_pause, bool rx_pause)
>> +{
>> + struct phylink_pcs *pcs = ppe_phylink_mac_select_pcs(ppe_dev, port, interface);
>> + struct ppe_uniphy *uniphy = pcs_to_ppe_uniphy(pcs);
>> + struct ppe_port *ppe_port = ppe_port_get(ppe_dev, port);
>> +
>> + /* Wait uniphy auto-negotiation completion */
>> + ppe_uniphy_autoneg_complete_check(uniphy, port);
>
> Way too late...
>

Yes agree, this will be removed. If inband autoneg is used,
pcs_get_state should report the link status. Then this function call
should not be needed and should be removed.

>> @@ -352,6 +1230,12 @@ static int ppe_port_maxframe_set(struct ppe_device *ppe_dev,
>> }
>>
>> static struct ppe_device_ops qcom_ppe_ops = {
>> + .phylink_setup = ppe_phylink_setup,
>> + .phylink_destroy = ppe_phylink_destroy,
>> + .phylink_mac_config = ppe_phylink_mac_config,
>> + .phylink_mac_link_up = ppe_phylink_mac_link_up,
>> + .phylink_mac_link_down = ppe_phylink_mac_link_down,
>> + .phylink_mac_select_pcs = ppe_phylink_mac_select_pcs,
>> .set_maxframe = ppe_port_maxframe_set,
>> };
>
> Why this extra layer of abstraction? If you need separate phylink
> operations, why not implement separate phylink_mac_ops structures?
>

This PPE driver will serve as the base driver for higher level drivers
such as the ethernet DMA (EDMA) driver and the DSA switch driver. The
ppe_device_ops is exported to these higher level drivers, to allow
access to PPE operations. For example, the EDMA driver (ethernet
netdevice driver to be pushed for review after the PPE driver) will use
the phylink_setup/destroy ops for managing netdevice to PHY linkage. The
set_maxframe op is also to be used by the EDMA driver during MTU change
operation on the ethernet port.

I also mentioned it in the section "Exported PPE Device Operations" in
PPE driver documentation:
https://lore.kernel.org/netdev/[email protected]/

Whereas the PPE DSA switch driver is expected to use the phylink_mac
ops. However，we will remove the phylink_mac ops from this patch now
since it is currently unused.