Re: [PATCH RFC net-next 9/9] net: stmmac: qcom-ethqos: add Qualcomm Nord support

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 13288 bytes --]

> Qualcomm Nord provides two ETHQOS controllers based on the Synopsys
> DesignWare 25GMAC IP.
> 
> The Nord integration uses platform-specific MTL queue, DMA channel and
> timestamp register addresses. It also requires wrapper programming for
> USXGMII operation and an EEE clock vote for PCS link-up.
> 
> Add the Nord platform data, select the DW25GMAC core, describe its
> register layout and configure the ETHQOS wrapper for USXGMII. Also add
> the platform-specific loopback control needed while the MAC is being
> initialized.
> 
> Signed-off-by: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

Hi Mohd,

some nits inline.

Regards,
Lorenzo

> ---
>  .../ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c    | 142 ++++++++++++++++++++-
>  1 file changed, 137 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index a99a78880c0fb7858e90b6212b37ebfc7fccaf84..a03c8e203920a70238deff9f1f970d29d1293893 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -23,8 +23,14 @@
>  #define RGMII_IO_MACRO_CONFIG2		0x1C
>  #define RGMII_IO_MACRO_DEBUG1		0x20
>  #define EMAC_SYSTEM_LOW_POWER_DEBUG	0x28
> +#define RGMII_IO_MACRO_SCRATCH_2	0x44
>  #define EMAC_WRAPPER_SGMII_PHY_CNTRL1	0xf4
>  
> +#define RGMII_IO_MACRO_BYPASS			0x16C
> +#define EMAC_WRAPPER_SGMII_PHY_CNTRL0		0x170
> +#define EMAC_WRAPPER_SGMII_PHY_CNTRL1_V4	0x174
> +#define EMAC_WRAPPER_USXGMII_MUX_SEL		0x1D0
> +
>  /* RGMII_IO_MACRO_CONFIG fields */
>  #define RGMII_CONFIG_FUNC_CLK_EN		BIT(30)
>  #define RGMII_CONFIG_POS_NEG_DATA_SEL		BIT(23)
> @@ -77,6 +83,21 @@
>  #define RGMII_CONFIG2_RX_PROG_SWAP		BIT(7)
>  #define RGMII_CONFIG2_DATA_DIVIDE_CLK_SEL	BIT(6)
>  #define RGMII_CONFIG2_TX_CLK_PHASE_SHIFT_EN	BIT(5)
> +#define RGMII_CONFIG2_MODE_EN_VIA_GMII		BIT(21)
> +
> +/* EMAC_WRAPPER_SGMII_PHY_CNTRL0 fields */
> +#define SGMII_PHY_CNTRL0_2P5G_1G_CLK_SEL	GENMASK(6, 5)
> +
> +/* EMAC_WRAPPER_SGMII_PHY_CNTRL1_V4 fields */
> +#define SGMII_PHY_CNTRL1_USXGMII_GMII_MASTER_CLK_MUX_SEL	BIT(4)
> +#define SGMII_PHY_CNTRL1_RGMII_SGMII_CLK_MUX_SEL		BIT(0)
> +
> +/* RGMII_IO_MACRO_BYPASS fields */
> +#define RGMII_BYPASS_EN				BIT(0)
> +
> +/* EMAC_WRAPPER_USXGMII_MUX_SEL fields */
> +#define USXGMII_CLK_BLK_GMII_CLK_BLK_SEL	BIT(1)
> +#define USXGMII_CLK_BLK_CLK_EN			BIT(0)
>  
>  /* EMAC_WRAPPER_SGMII_PHY_CNTRL1 bits */
>  #define SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN	BIT(3)
> @@ -88,6 +109,8 @@ struct ethqos_emac_por {
>  	unsigned int value;
>  };
>  
> +struct qcom_ethqos;
> +
>  struct ethqos_emac_driver_data {
>  	const struct ethqos_emac_por *rgmii_por;
>  	unsigned int num_rgmii_por;
> @@ -97,12 +120,16 @@ struct ethqos_emac_driver_data {
>  	const char *link_clk_name;
>  	struct dwmac4_addrs dwmac4_addrs;
>  	bool needs_sgmii_loopback;
> +	struct dwxgmac_addrs dwxgmac_addrs;
> +	enum dwmac_core_type core_type;
> +	void (*set_sgmii_loopback)(struct qcom_ethqos *ethqos, bool enable);

I guess introducing this function pointer you can drop needs_sgmii_loopback
field since you always check set_sgmii_loopback is not NULL, right?

>  };
>  
>  struct qcom_ethqos {
>  	struct platform_device *pdev;
>  	void __iomem *rgmii_base;
>  	struct clk *link_clk;
> +	struct clk *eee_clk;
>  	struct phy *serdes_phy;
>  	phy_interface_t phy_mode;
>  
> @@ -111,6 +138,7 @@ struct qcom_ethqos {
>  	bool rgmii_config_loopback_en;
>  	bool has_emac_ge_3;
>  	bool needs_sgmii_loopback;
> +	void (*set_sgmii_loopback)(struct qcom_ethqos *ethqos, bool enable);
>  };
>  
>  static u32 rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
> @@ -201,6 +229,15 @@ qcom_ethqos_set_sgmii_loopback(struct qcom_ethqos *ethqos, bool enable)
>  		      EMAC_WRAPPER_SGMII_PHY_CNTRL1);
>  }
>  
> +static void
> +qcom_ethqos_set_sgmii_loopback_nord(struct qcom_ethqos *ethqos, bool enable)
> +{
> +	rgmii_updatel(ethqos,
> +		      SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN,
> +		      enable ? SGMII_PHY_CNTRL1_SGMII_TX_TO_RX_LOOPBACK_EN : 0,
> +		      EMAC_WRAPPER_SGMII_PHY_CNTRL1_V4);
> +}
> +
>  static void ethqos_set_func_clk_en(struct qcom_ethqos *ethqos)
>  {
>  	rgmii_setmask(ethqos, RGMII_CONFIG_FUNC_CLK_EN, RGMII_IO_MACRO_CONFIG);
> @@ -220,6 +257,7 @@ static const struct ethqos_emac_driver_data emac_v2_3_0_data = {
>  	.num_rgmii_por = ARRAY_SIZE(emac_v2_3_0_por),
>  	.rgmii_config_loopback_en = true,
>  	.has_emac_ge_3 = false,
> +	.core_type = DWMAC_CORE_GMAC4,
>  };
>  
>  static const struct ethqos_emac_por emac_v2_1_0_por[] = {
> @@ -236,6 +274,7 @@ static const struct ethqos_emac_driver_data emac_v2_1_0_data = {
>  	.num_rgmii_por = ARRAY_SIZE(emac_v2_1_0_por),
>  	.rgmii_config_loopback_en = false,
>  	.has_emac_ge_3 = false,
> +	.core_type = DWMAC_CORE_GMAC4,
>  };
>  
>  static const struct ethqos_emac_por emac_v3_0_0_por[] = {
> @@ -252,6 +291,7 @@ static const struct ethqos_emac_driver_data emac_v3_0_0_data = {
>  	.num_rgmii_por = ARRAY_SIZE(emac_v3_0_0_por),
>  	.rgmii_config_loopback_en = false,
>  	.has_emac_ge_3 = true,
> +	.core_type = DWMAC_CORE_GMAC4,
>  	.dwmac4_addrs = {
>  		.dma_chan = 0x00008100,
>  		.dma_chan_offset = 0x1000,
> @@ -286,6 +326,8 @@ static const struct ethqos_emac_driver_data emac_v4_0_0_data = {
>  	.has_emac_ge_3 = true,
>  	.link_clk_name = "phyaux",
>  	.needs_sgmii_loopback = true,
> +	.core_type = DWMAC_CORE_GMAC4,
> +	.set_sgmii_loopback = qcom_ethqos_set_sgmii_loopback,
>  	.dma_addr_width = 36,
>  	.dwmac4_addrs = {
>  		.dma_chan = 0x00008100,
> @@ -485,6 +527,47 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos, int speed)
>  	return 0;
>  }
>  
> +static void ethqos_configure_usxgmii(struct qcom_ethqos *ethqos)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ethqos->num_rgmii_por; i++)
> +		rgmii_writel(ethqos, ethqos->rgmii_por[i].value,
> +			     ethqos->rgmii_por[i].offset);
> +
> +	ethqos_set_func_clk_en(ethqos);
> +
> +	rgmii_updatel(ethqos, RGMII_BYPASS_EN, RGMII_BYPASS_EN,
> +		      RGMII_IO_MACRO_BYPASS);
> +	rgmii_updatel(ethqos, RGMII_CONFIG2_MODE_EN_VIA_GMII, 0,
> +		      RGMII_IO_MACRO_CONFIG2);
> +	rgmii_updatel(ethqos, SGMII_PHY_CNTRL0_2P5G_1G_CLK_SEL, BIT(5),
> +		      EMAC_WRAPPER_SGMII_PHY_CNTRL0);
> +	rgmii_updatel(ethqos, SGMII_PHY_CNTRL1_RGMII_SGMII_CLK_MUX_SEL, 0,
> +		      EMAC_WRAPPER_SGMII_PHY_CNTRL1_V4);
> +	rgmii_updatel(ethqos, SGMII_PHY_CNTRL1_USXGMII_GMII_MASTER_CLK_MUX_SEL,
> +		      SGMII_PHY_CNTRL1_USXGMII_GMII_MASTER_CLK_MUX_SEL,
> +		      EMAC_WRAPPER_SGMII_PHY_CNTRL1_V4);
> +
> +	rgmii_updatel(ethqos, USXGMII_CLK_BLK_GMII_CLK_BLK_SEL, 0,
> +		      EMAC_WRAPPER_USXGMII_MUX_SEL);
> +	rgmii_updatel(ethqos, USXGMII_CLK_BLK_CLK_EN, 0,
> +		      EMAC_WRAPPER_USXGMII_MUX_SEL);
> +
> +	rgmii_updatel(ethqos, USXGMII_CLK_BLK_GMII_CLK_BLK_SEL,
> +		      USXGMII_CLK_BLK_GMII_CLK_BLK_SEL,
> +		      EMAC_WRAPPER_USXGMII_MUX_SEL);
> +}
> +
> +static void
> +ethqos_fix_mac_speed_usxgmii(void *bsp_priv, phy_interface_t interface,
> +			     int speed, unsigned int mode)
> +{
> +	struct qcom_ethqos *ethqos = bsp_priv;
> +
> +	ethqos_configure_usxgmii(ethqos);

I guess you can just open-code ethqos_configure_usxgmii() here.

> +}
> +
>  static void ethqos_fix_mac_speed_rgmii(void *bsp_priv,
>  				       phy_interface_t interface, int speed,
>  				       unsigned int mode)
> @@ -625,7 +708,8 @@ static int ethqos_mac_finish_serdes(struct net_device *ndev, void *priv,
>  	struct qcom_ethqos *ethqos = priv;
>  	int ret = 0;
>  
> -	qcom_ethqos_set_sgmii_loopback(ethqos, false);
> +	if (ethqos->set_sgmii_loopback)
> +		ethqos->set_sgmii_loopback(ethqos, false);
>  
>  	if (interface == PHY_INTERFACE_MODE_SGMII ||
>  	    interface == PHY_INTERFACE_MODE_2500BASEX)
> @@ -647,14 +731,24 @@ static int ethqos_clks_config(void *priv, bool enabled)
>  			return ret;
>  		}
>  
> +		/* PCS link-up depends on the EEE clock due to a hardware quirk. */
> +		ret = clk_prepare_enable(ethqos->eee_clk);
> +		if (ret) {
> +			dev_err(&ethqos->pdev->dev, "eee_clk enable failed\n");
> +			clk_disable_unprepare(ethqos->link_clk);
> +			return ret;
> +		}
> +
>  		/* Enable functional clock to prevent DMA reset to timeout due
>  		 * to lacking PHY clock after the hardware block has been power
>  		 * cycled. The actual configuration will be adjusted once
>  		 * ethqos' fix_mac_speed() method is invoked.
>  		 */
> -		qcom_ethqos_set_sgmii_loopback(ethqos, true);
> +		if (ethqos->set_sgmii_loopback)
> +			ethqos->set_sgmii_loopback(ethqos, true);

I guess you should move this above the comment.

>  		ethqos_set_func_clk_en(ethqos);
>  	} else {
> +		clk_disable_unprepare(ethqos->eee_clk);
>  		clk_disable_unprepare(ethqos->link_clk);
>  	}
>  
> @@ -738,6 +832,10 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		plat_dat->fix_mac_speed = ethqos_fix_mac_speed_sgmii;
>  		plat_dat->mac_finish = ethqos_mac_finish_serdes;
>  		break;
> +	case PHY_INTERFACE_MODE_USXGMII:
> +		plat_dat->fix_mac_speed = ethqos_fix_mac_speed_usxgmii;
> +		plat_dat->mac_finish = ethqos_mac_finish_serdes;
> +		break;
>  	default:
>  		dev_err(dev, "Unsupported phy mode %s\n",
>  			phy_modes(ethqos->phy_mode));
> @@ -756,12 +854,18 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos->rgmii_config_loopback_en = data->rgmii_config_loopback_en;
>  	ethqos->has_emac_ge_3 = data->has_emac_ge_3;
>  	ethqos->needs_sgmii_loopback = data->needs_sgmii_loopback;
> +	ethqos->set_sgmii_loopback = data->set_sgmii_loopback;
>  
>  	ethqos->link_clk = devm_clk_get(dev, data->link_clk_name ?: "rgmii");
>  	if (IS_ERR(ethqos->link_clk))
>  		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
>  				     "Failed to get link_clk\n");
>  
> +	ethqos->eee_clk = devm_clk_get_optional(dev, "eee");
> +	if (IS_ERR(ethqos->eee_clk))
> +		return dev_err_probe(dev, PTR_ERR(ethqos->eee_clk),
> +				     "Failed to get eee_clk\n");
> +
>  	ret = ethqos_clks_config(ethqos, true);
>  	if (ret)
>  		return ret;
> @@ -778,7 +882,8 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos_set_clk_tx_rate(ethqos, NULL, plat_dat->phy_interface,
>  			       SPEED_1000);
>  
> -	qcom_ethqos_set_sgmii_loopback(ethqos, true);
> +	if (ethqos->set_sgmii_loopback)
> +		ethqos->set_sgmii_loopback(ethqos, true);
>  	ethqos_set_func_clk_en(ethqos);
>  
>  	/* The clocks are controlled by firmware, so we don't know for certain
> @@ -791,9 +896,11 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	plat_dat->set_clk_tx_rate = ethqos_set_clk_tx_rate;
>  	plat_dat->dump_debug_regs = rgmii_dump;
>  	plat_dat->ptp_clk_freq_config = ethqos_ptp_clk_freq_config;
> -	plat_dat->core_type = DWMAC_CORE_GMAC4;
> +	plat_dat->core_type = data->core_type;
>  	if (ethqos->has_emac_ge_3)
>  		plat_dat->dwmac4_addrs = &data->dwmac4_addrs;
> +	if (data->dwxgmac_addrs.dma_even_chan_base)

it seems a bit odd to check dma_even_chan_base is set. Maybe it is better to
check core_type is DWMAC_CORE_25GMAC?

> +		plat_dat->dwxgmac_addrs = &data->dwxgmac_addrs;
>  	plat_dat->pmt = true;
>  	if (of_property_read_bool(np, "snps,tso"))
>  		plat_dat->flags |= STMMAC_FLAG_TSO_EN;
> @@ -817,8 +924,33 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	return devm_stmmac_pltfr_probe(pdev, plat_dat, &stmmac_res);
>  }
>  
> +static const struct ethqos_emac_por emac_nord_por[] = {
> +	{ .offset = RGMII_IO_MACRO_CONFIG,    .value = 0x00C04D03 },
> +	{ .offset = SDCC_HC_REG_DLL_CONFIG,   .value = 0x2004642C },
> +	{ .offset = RGMII_IO_MACRO_CONFIG2,   .value = 0x00222060 },
> +	{ .offset = RGMII_IO_MACRO_SCRATCH_2, .value = 0x4c },
> +};
> +
> +static const struct ethqos_emac_driver_data emac_nord_data = {
> +	.rgmii_por = emac_nord_por,
> +	.num_rgmii_por = ARRAY_SIZE(emac_nord_por),
> +	.dma_addr_width = 40,
> +	.link_clk_name = "phyaux",
> +	.core_type = DWMAC_CORE_25GMAC,
> +	.set_sgmii_loopback = qcom_ethqos_set_sgmii_loopback_nord,
> +	.dwxgmac_addrs = {
> +		.dma_even_chan_base = 0x00008500,
> +		.dma_odd_chan_base  = 0x00008580,
> +		.dma_chan_offset    = 0x00001000,
> +		.mtl_chan_base      = 0x00008000,
> +		.mtl_chan_offset    = 0x00001000,
> +		.timestamp_base     = 0x00007000,
> +	},
> +};

I would move emac_nord_por and emac_nord_data definitions close to the other
ones. I guess the code is more readable.

Regards,
Lorenzo

> +
>  static const struct of_device_id qcom_ethqos_match[] = {
> -	{ .compatible = "qcom,qcs404-ethqos", .data = &emac_v2_3_0_data},
> +	{ .compatible = "qcom,nord-ethqos",    .data = &emac_nord_data },
> +	{ .compatible = "qcom,qcs404-ethqos",  .data = &emac_v2_3_0_data },
>  	{ .compatible = "qcom,sa8775p-ethqos", .data = &emac_v4_0_0_data},
>  	{ .compatible = "qcom,sc8280xp-ethqos", .data = &emac_v3_0_0_data},
>  	{ .compatible = "qcom,sm8150-ethqos", .data = &emac_v2_1_0_data},
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]