Re: [PATCH net-next v3 12/12] net: stmmac: dwmac-qcom-ethqos: Add EMAC3 support

[Paolo Abeni <pabeni@xxxxxxxxxx>]

Hi,

On Mon, 2023-04-03 at 11:52 -0500, Andrew Halaney wrote:
> @@ -327,9 +370,17 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>  			      RGMII_CONFIG2_RX_PROG_SWAP,
>  			      RGMII_IO_MACRO_CONFIG2);
>  
> -		/* Set PRG_RCLK_DLY to 57 for 1.8 ns delay */
> -		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
> -			      57, SDCC_HC_REG_DDR_CONFIG);
> +		/* PRG_RCLK_DLY = TCXO period * TCXO_CYCLES_CNT / 2 * RX delay ns,
> +		 * in practice this becomes PRG_RCLK_DLY = 52 * 4 / 2 * RX delay ns
> +		 */
> +		if (ethqos->has_emac3)
> +			/* 0.9 ns */
> +			rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
> +				      115, SDCC_HC_REG_DDR_CONFIG);
> +		else
> +			/* 1.8 ns */
> +			rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
> +				      57, SDCC_HC_REG_DDR_CONFIG);

The only (very minor) comment I have is that AFAIK the preferred style
for the above block is: 

		if (ethqos->has_emac3) {
			/* 0.9 ns */
			rgmii_updatel(ethqos, SDCC_DDR_CONFIG_PRG_RCLK_DLY,
				      115, SDCC_HC_REG_DDR_CONFIG);
		} else {
			...

due to the comment presence this should be threaded as a multi-line statement.
(even if checkpatch does not complain).

Cheers,

Paolo

>  			      SDCC_HC_REG_DDR_CONFIG);
> @@ -355,8 +406,15 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>  			      BIT(6), RGMII_IO_MACRO_CONFIG);
>  		rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
>  			      0, RGMII_IO_MACRO_CONFIG2);
> -		rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
> -			      0, RGMII_IO_MACRO_CONFIG2);
> +
> +		if (ethqos->has_emac3)
> +			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
> +				      RGMII_CONFIG2_RX_PROG_SWAP,
> +				      RGMII_IO_MACRO_CONFIG2);
> +		else
> +			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
> +				      0, RGMII_IO_MACRO_CONFIG2);
> +
>  		/* Write 0x5 to PRG_RCLK_DLY_CODE */
>  		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
>  			      (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG);
> @@ -389,8 +447,13 @@ static int ethqos_rgmii_macro_init(struct qcom_ethqos *ethqos)
>  			      RGMII_IO_MACRO_CONFIG);
>  		rgmii_updatel(ethqos, RGMII_CONFIG2_RSVD_CONFIG15,
>  			      0, RGMII_IO_MACRO_CONFIG2);
> -		rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
> -			      0, RGMII_IO_MACRO_CONFIG2);
> +		if (ethqos->has_emac3)
> +			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
> +				      RGMII_CONFIG2_RX_PROG_SWAP,
> +				      RGMII_IO_MACRO_CONFIG2);
> +		else
> +			rgmii_updatel(ethqos, RGMII_CONFIG2_RX_PROG_SWAP,
> +				      0, RGMII_IO_MACRO_CONFIG2);
>  		/* Write 0x5 to PRG_RCLK_DLY_CODE */
>  		rgmii_updatel(ethqos, SDCC_DDR_CONFIG_EXT_PRG_RCLK_DLY_CODE,
>  			      (BIT(29) | BIT(27)), SDCC_HC_REG_DDR_CONFIG);
> @@ -433,6 +496,17 @@ static int ethqos_configure(struct qcom_ethqos *ethqos)
>  	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_PDN,
>  		      SDCC_DLL_CONFIG_PDN, SDCC_HC_REG_DLL_CONFIG);
>  
> +	if (ethqos->has_emac3) {
> +		if (ethqos->speed == SPEED_1000) {
> +			rgmii_writel(ethqos, 0x1800000, SDCC_TEST_CTL);
> +			rgmii_writel(ethqos, 0x2C010800, SDCC_USR_CTL);
> +			rgmii_writel(ethqos, 0xA001, SDCC_HC_REG_DLL_CONFIG2);
> +		} else {
> +			rgmii_writel(ethqos, 0x40010800, SDCC_USR_CTL);
> +			rgmii_writel(ethqos, 0xA001, SDCC_HC_REG_DLL_CONFIG2);
> +		}
> +	}
> +
>  	/* Clear DLL_RST */
>  	rgmii_updatel(ethqos, SDCC_DLL_CONFIG_DLL_RST, 0,
>  		      SDCC_HC_REG_DLL_CONFIG);
> @@ -452,7 +526,9 @@ static int ethqos_configure(struct qcom_ethqos *ethqos)
>  			      SDCC_HC_REG_DLL_CONFIG);
>  
>  		/* Set USR_CTL bit 26 with mask of 3 bits */
> -		rgmii_updatel(ethqos, GENMASK(26, 24), BIT(26), SDCC_USR_CTL);
> +		if (!ethqos->has_emac3)
> +			rgmii_updatel(ethqos, GENMASK(26, 24), BIT(26),
> +				      SDCC_USR_CTL);
>  
>  		/* wait for DLL LOCK */
>  		do {
> @@ -547,6 +623,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	ethqos->por = data->por;
>  	ethqos->num_por = data->num_por;
>  	ethqos->rgmii_config_loopback_en = data->rgmii_config_loopback_en;
> +	ethqos->has_emac3 = data->has_emac3;
>  
>  	ethqos->rgmii_clk = devm_clk_get(&pdev->dev, "rgmii");
>  	if (IS_ERR(ethqos->rgmii_clk)) {
> @@ -566,6 +643,7 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  	plat_dat->fix_mac_speed = ethqos_fix_mac_speed;
>  	plat_dat->dump_debug_regs = rgmii_dump;
>  	plat_dat->has_gmac4 = 1;
> +	plat_dat->dwmac4_addrs = &data->dwmac4_addrs;
>  	plat_dat->pmt = 1;
>  	plat_dat->tso_en = of_property_read_bool(np, "snps,tso");
>  	if (of_device_is_compatible(np, "qcom,qcs404-ethqos"))
> @@ -603,6 +681,7 @@ static int qcom_ethqos_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id qcom_ethqos_match[] = {
>  	{ .compatible = "qcom,qcs404-ethqos", .data = &emac_v2_3_0_data},
> +	{ .compatible = "qcom,sc8280xp-ethqos", .data = &emac_v3_0_0_data},
>  	{ .compatible = "qcom,sm8150-ethqos", .data = &emac_v2_1_0_data},
>  	{ }
>  };