[PATCH 4/7] net: dwc_eth_qos: Add glue driver for GMAC on Rockchip RK3568

Eugen Hristev eugen.hristev at collabora.com
Mon Aug 7 16:41:18 CEST 2023


Hi Jonas,

Thank you for your work,

On 8/7/23 03:08, Jonas Karlman wrote:
> Add a new glue driver for Rockchip SoCs, i.e RK3568, with a GMAC based
> on Synopsys DWC Ethernet QoS IP.
> 
> rk_gmac_ops was ported from linux commit:
> 3bb3d6b1c195 ("net: stmmac: Add RK3566/RK3568 SoC support")
> 
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
> ---
> Cc: David Wu <david.wu at rock-chips.com>
> Cc: Ezequiel Garcia <ezequiel at collabora.com>
> ---
>   drivers/net/Kconfig                |   8 +
>   drivers/net/Makefile               |   1 +
>   drivers/net/dwc_eth_qos.c          |   8 +-
>   drivers/net/dwc_eth_qos.h          |   2 +
>   drivers/net/dwc_eth_qos_rockchip.c | 348 +++++++++++++++++++++++++++++
>   5 files changed, 365 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/net/dwc_eth_qos_rockchip.c
> 
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 0ed39a61e4de..29304fd77759 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -225,6 +225,14 @@ config DWC_ETH_QOS_IMX
>   	  The Synopsys Designware Ethernet QOS IP block with the specific
>   	  configuration used in IMX soc.
>   
> +config DWC_ETH_QOS_ROCKCHIP
> +	bool "Synopsys DWC Ethernet QOS device support for Rockchip SoCs"
> +	depends on DWC_ETH_QOS
> +	select DM_ETH_PHY
> +	help
> +	  The Synopsys Designware Ethernet QOS IP block with specific
> +	  configuration used in Rockchip SoCs.
> +
>   config DWC_ETH_QOS_STM32
>   	bool "Synopsys DWC Ethernet QOS device support for STM32"
>   	depends on DWC_ETH_QOS
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index d4af253b6f28..1d444f5b4a69 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_DRIVER_DM9000) += dm9000x.o
>   obj-$(CONFIG_DSA_SANDBOX) += dsa_sandbox.o
>   obj-$(CONFIG_DWC_ETH_QOS) += dwc_eth_qos.o
>   obj-$(CONFIG_DWC_ETH_QOS_IMX) += dwc_eth_qos_imx.o
> +obj-$(CONFIG_DWC_ETH_QOS_ROCKCHIP) += dwc_eth_qos_rockchip.o
>   obj-$(CONFIG_DWC_ETH_QOS_QCOM) += dwc_eth_qos_qcom.o
>   obj-$(CONFIG_DWC_ETH_QOS_STARFIVE) += dwc_eth_qos_starfive.o
>   obj-$(CONFIG_E1000) += e1000.o
> diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c
> index 24fb3fac1f12..9fb98a2c3c74 100644
> --- a/drivers/net/dwc_eth_qos.c
> +++ b/drivers/net/dwc_eth_qos.c
> @@ -1707,7 +1707,12 @@ static const struct udevice_id eqos_ids[] = {
>   		.data = (ulong)&eqos_imx_config
>   	},
>   #endif
> -
> +#if IS_ENABLED(CONFIG_DWC_ETH_QOS_ROCKCHIP)
> +	{
> +		.compatible = "rockchip,rk3568-gmac",
> +		.data = (ulong)&eqos_rockchip_config
> +	},
> +#endif
>   #if IS_ENABLED(CONFIG_DWC_ETH_QOS_QCOM)
>   	{
>   		.compatible = "qcom,qcs404-ethqos",
> @@ -1720,7 +1725,6 @@ static const struct udevice_id eqos_ids[] = {
>   		.data = (ulong)&eqos_jh7110_config
>   	},
>   #endif
> -
Unintended change ?
>   	{ }
>   };
>   
> diff --git a/drivers/net/dwc_eth_qos.h b/drivers/net/dwc_eth_qos.h
> index 06a082da72ef..e3222e1e17e5 100644
> --- a/drivers/net/dwc_eth_qos.h
> +++ b/drivers/net/dwc_eth_qos.h
> @@ -82,6 +82,7 @@ struct eqos_mac_regs {
>   #define EQOS_MAC_MDIO_ADDRESS_PA_SHIFT			21
>   #define EQOS_MAC_MDIO_ADDRESS_RDA_SHIFT			16
>   #define EQOS_MAC_MDIO_ADDRESS_CR_SHIFT			8
> +#define EQOS_MAC_MDIO_ADDRESS_CR_100_150		1
>   #define EQOS_MAC_MDIO_ADDRESS_CR_20_35			2
>   #define EQOS_MAC_MDIO_ADDRESS_CR_250_300		5
>   #define EQOS_MAC_MDIO_ADDRESS_SKAP			BIT(4)
> @@ -287,5 +288,6 @@ void eqos_flush_buffer_generic(void *buf, size_t size);
>   int eqos_null_ops(struct udevice *dev);
>   
>   extern struct eqos_config eqos_imx_config;
> +extern struct eqos_config eqos_rockchip_config;
>   extern struct eqos_config eqos_qcom_config;
>   extern struct eqos_config eqos_jh7110_config;
> diff --git a/drivers/net/dwc_eth_qos_rockchip.c b/drivers/net/dwc_eth_qos_rockchip.c
> new file mode 100644
> index 000000000000..c8abe351fc3e
> --- /dev/null
> +++ b/drivers/net/dwc_eth_qos_rockchip.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <dm/device_compat.h>
> +#include <net.h>
> +#include <phy.h>
> +#include <regmap.h>
> +#include <reset.h>
> +#include <syscon.h>
> +#include <asm/gpio.h>
> +#include <linux/delay.h>
> +
> +#include "dwc_eth_qos.h"
> +
> +struct rk_gmac_ops {
> +	const char *compatible;
> +	int (*set_to_rgmii)(struct udevice *dev,
> +			    int tx_delay, int rx_delay);
> +	int (*set_to_rmii)(struct udevice *dev);
> +	int (*set_gmac_speed)(struct udevice *dev);
> +	u32 regs[3];

Can't these be somehow const and point to a dedicated array for a 
specific SoC ?

Also I believe the naming 'regs' is a bit unfortunate, as it does not 
hold the usual regmap, rather an array of addresses hardcoded used to 
figure out which of the GMAC instances we are referring to.

> +};
> +
> +struct rockchip_platform_data {
> +	struct reset_ctl_bulk resets;
> +	const struct rk_gmac_ops *ops;
> +	int id;
> +	struct regmap *grf;
> +};
> +
> +#define HIWORD_UPDATE(val, mask, shift) \
> +		((val) << (shift) | (mask) << ((shift) + 16))
> +
> +#define GRF_BIT(nr)	(BIT(nr) | BIT((nr) + 16))
> +#define GRF_CLR_BIT(nr)	(BIT((nr) + 16))

can't you use rk_clrsetreg and friends? it should already do this

> +
> +#define RK3568_GRF_GMAC0_CON0		0x0380
> +#define RK3568_GRF_GMAC0_CON1		0x0384
> +#define RK3568_GRF_GMAC1_CON0		0x0388
> +#define RK3568_GRF_GMAC1_CON1		0x038c

Can you add these as a struct in

arch/arm/include/asm/arch-rockchip/grf_rk3568.h

(e.g. I got this from downstream Uboot for 3588 :
https://gitlab.collabora.com/hardware-enablement/rockchip-3588/u-boot/-/blob/rk3568-88-gmac/arch/arm/include/asm/arch-rockchip/grf_rk3588.h
)



> +
> +/* RK3568_GRF_GMAC0_CON1 && RK3568_GRF_GMAC1_CON1 */
> +#define RK3568_GMAC_PHY_INTF_SEL_RGMII	\
> +		(GRF_BIT(4) | GRF_CLR_BIT(5) | GRF_CLR_BIT(6))
> +#define RK3568_GMAC_PHY_INTF_SEL_RMII	\
> +		(GRF_CLR_BIT(4) | GRF_CLR_BIT(5) | GRF_BIT(6))
> +#define RK3568_GMAC_FLOW_CTRL			GRF_BIT(3)
> +#define RK3568_GMAC_FLOW_CTRL_CLR		GRF_CLR_BIT(3)
> +#define RK3568_GMAC_RXCLK_DLY_ENABLE		GRF_BIT(1)
> +#define RK3568_GMAC_RXCLK_DLY_DISABLE		GRF_CLR_BIT(1)
> +#define RK3568_GMAC_TXCLK_DLY_ENABLE		GRF_BIT(0)
> +#define RK3568_GMAC_TXCLK_DLY_DISABLE		GRF_CLR_BIT(0)
> +
> +/* RK3568_GRF_GMAC0_CON0 && RK3568_GRF_GMAC1_CON0 */
> +#define RK3568_GMAC_CLK_RX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 8)
> +#define RK3568_GMAC_CLK_TX_DL_CFG(val)	HIWORD_UPDATE(val, 0x7F, 0)
> +
> +static int rk3568_set_to_rgmii(struct udevice *dev,
> +			       int tx_delay, int rx_delay)
> +{
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data = pdata->priv_pdata;
> +	u32 con0, con1;
> +
> +	con0 = (data->id == 1) ? RK3568_GRF_GMAC1_CON0 :
> +				 RK3568_GRF_GMAC0_CON0;
> +	con1 = (data->id == 1) ? RK3568_GRF_GMAC1_CON1 :
> +				 RK3568_GRF_GMAC0_CON1;
> +
> +	regmap_write(data->grf, con0,
> +		     RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
> +		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> +
> +	regmap_write(data->grf, con1,
> +		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
> +		     RK3568_GMAC_RXCLK_DLY_ENABLE |
> +		     RK3568_GMAC_TXCLK_DLY_ENABLE);

Shouldn't you set RK3568_GMAC_TXCLK_DLY_DISABLE here in case the delays 
are 0 ?

> +
> +	return 0;
> +}
> +
> +static int rk3568_set_to_rmii(struct udevice *dev)
> +{
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data = pdata->priv_pdata;
> +	u32 con1;
> +
> +	con1 = (data->id == 1) ? RK3568_GRF_GMAC1_CON1 :
> +				 RK3568_GRF_GMAC0_CON1;
> +	regmap_write(data->grf, con1, RK3568_GMAC_PHY_INTF_SEL_RMII);
> +
> +	return 0;
> +}
> +
> +static int rk3568_set_gmac_speed(struct udevice *dev)
> +{
> +	struct eqos_priv *eqos = dev_get_priv(dev);
> +	ulong rate;
> +	int ret;
> +
> +	switch (eqos->phy->speed) {
> +	case SPEED_10:
> +		rate = 2500000;
> +		break;
> +	case SPEED_100:
> +		rate = 25000000;
> +		break;
> +	case SPEED_1000:
> +		rate = 125000000;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = clk_set_rate(&eqos->clk_tx, rate);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static const struct rk_gmac_ops rk_gmac_ops[] = {
> +	{
> +		.compatible = "rockchip,rk3568-gmac",
> +		.set_to_rgmii = rk3568_set_to_rgmii,
> +		.set_to_rmii = rk3568_set_to_rmii,
> +		.set_gmac_speed = rk3568_set_gmac_speed,
> +		.regs = {
> +			0xfe2a0000, /* gmac0 */
> +			0xfe010000, /* gmac1 */
> +			0x0, /* sentinel */
> +		},
Can these be referenced to a const struct ?
> +	},
> +	{ }
> +};
> +
> +static const struct rk_gmac_ops *get_rk_gmac_ops(struct udevice *dev)
> +{
> +	const struct rk_gmac_ops *ops = rk_gmac_ops;
> +
> +	while (ops->compatible) {
> +		if (device_is_compatible(dev, ops->compatible))
> +			return ops;
> +		ops++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int eqos_probe_resources_rk(struct udevice *dev)
> +{
> +	struct eqos_priv *eqos = dev_get_priv(dev);
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data;
> +	int reset_flags = GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE;
> +	int ret;
> +
> +	data = calloc(1, sizeof(struct rockchip_platform_data));
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->ops = get_rk_gmac_ops(dev);
> +	if (!data->ops) {
> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	for (int i = 0; data->ops->regs[i]; i++) {
> +		if (data->ops->regs[i] == (u32)eqos->regs) {
> +			data->id = i;
> +			break;
> +		}
> +	}
> +
> +	pdata->priv_pdata = data;
> +	pdata->phy_interface = eqos->config->interface(dev);
> +	pdata->max_speed = eqos->max_speed;
> +
> +	if (pdata->phy_interface == PHY_INTERFACE_MODE_NA) {
> +		pr_err("Invalid PHY interface\n");
> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	data->grf = syscon_regmap_lookup_by_phandle(dev, "rockchip,grf");
> +	if (IS_ERR(data->grf)) {
> +		dev_err(dev, "Missing rockchip,grf property\n");
> +		ret = -EINVAL;
> +		goto err_free;
> +	}

 From my read of the binding, rockchip,grf is not mandatory. Is it 
possible to just ignore the grf if the property is missing ?

> +
> +	ret = reset_get_bulk(dev, &data->resets);
> +	if (ret < 0)
> +		goto err_free;
> +
> +	reset_assert_bulk(&data->resets);
> +

The same goes for the clocks below, the binding specifies min clocks is 
5, and maximum clocks is 8, but not which are the 5 minimum required clocks.
So , can we not stop if some of the clocks are missing ?

> +	ret = clk_get_by_name(dev, "stmmaceth", &eqos->clk_master_bus);
> +	if (ret) {
> +		dev_dbg(dev, "clk_get_by_name(stmmaceth) failed: %d", ret);
> +		goto err_release_resets;
> +	}
> +
> +	ret = clk_get_by_name(dev, "clk_mac_speed", &eqos->clk_tx);
> +	if (ret) {
> +		dev_dbg(dev, "clk_get_by_name(clk_mac_speed) failed: %d", ret);
> +		goto err_free_clk_master_bus;
> +	}
> +
> +	if (dev_read_bool(dev, "snps,reset-active-low"))
> +		reset_flags |= GPIOD_ACTIVE_LOW;
> +
> +	dev_read_u32_array(dev, "snps,reset-delays-us", eqos->reset_delays, 3);
> +
> +	gpio_request_by_name(dev, "snps,reset-gpio", 0,
> +			     &eqos->phy_reset_gpio, reset_flags);

reset-gpio appears to be deprecated according to the binding,
also, you do not check the error code and assume the property exists ?


Greetings,
Eugen
> +
> +	return 0;
> +
> +err_free_clk_master_bus:
> +	clk_free(&eqos->clk_master_bus);
> +err_release_resets:
> +	reset_release_bulk(&data->resets);
> +err_free:
> +	free(data);
> +
> +	return ret;
> +}
> +
> +static int eqos_remove_resources_rk(struct udevice *dev)
> +{
> +	struct eqos_priv *eqos = dev_get_priv(dev);
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data = pdata->priv_pdata;
> +
> +	if (dm_gpio_is_valid(&eqos->phy_reset_gpio))
> +		dm_gpio_free(dev, &eqos->phy_reset_gpio);
> +
> +	clk_free(&eqos->clk_tx);
> +	clk_free(&eqos->clk_master_bus);
> +	reset_release_bulk(&data->resets);
> +	free(data);
> +
> +	return 0;
> +}
> +
> +static int eqos_stop_resets_rk(struct udevice *dev)
> +{
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data = pdata->priv_pdata;
> +
> +	return reset_assert_bulk(&data->resets);
> +}
> +
> +static int eqos_start_resets_rk(struct udevice *dev)
> +{
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data = pdata->priv_pdata;
> +
> +	return reset_deassert_bulk(&data->resets);
> +}
> +
> +static int eqos_stop_clks_rk(struct udevice *dev)
> +{
> +	return 0;
> +}
> +
> +static int eqos_start_clks_rk(struct udevice *dev)
> +{
> +	struct eqos_priv *eqos = dev_get_priv(dev);
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data = pdata->priv_pdata;
> +	int tx_delay, rx_delay, ret;
> +
> +	if (dm_gpio_is_valid(&eqos->phy_reset_gpio)) {
> +		udelay(eqos->reset_delays[1]);
> +
> +		ret = dm_gpio_set_value(&eqos->phy_reset_gpio, 0);
> +		if (ret < 0)
> +			return ret;
> +
> +		udelay(eqos->reset_delays[2]);
> +	}
> +
> +	tx_delay = dev_read_u32_default(dev, "tx_delay", 0x30);
> +	rx_delay = dev_read_u32_default(dev, "rx_delay", 0x10);
> +
> +	switch (pdata->phy_interface) {
> +	case PHY_INTERFACE_MODE_RGMII:
> +		return data->ops->set_to_rgmii(dev, tx_delay, rx_delay);
> +	case PHY_INTERFACE_MODE_RGMII_ID:
> +		return data->ops->set_to_rgmii(dev, 0, 0);
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		return data->ops->set_to_rgmii(dev, tx_delay, 0);
> +	case PHY_INTERFACE_MODE_RGMII_TXID:
> +		return data->ops->set_to_rgmii(dev, 0, rx_delay);
> +	case PHY_INTERFACE_MODE_RMII:
> +		return data->ops->set_to_rmii(dev);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int eqos_set_tx_clk_speed_rk(struct udevice *dev)
> +{
> +	struct eth_pdata *pdata = dev_get_plat(dev);
> +	struct rockchip_platform_data *data = pdata->priv_pdata;
> +
> +	return data->ops->set_gmac_speed(dev);
> +}
> +
> +static ulong eqos_get_tick_clk_rate_rk(struct udevice *dev)
> +{
> +	struct eqos_priv *eqos = dev_get_priv(dev);
> +
> +	return clk_get_rate(&eqos->clk_master_bus);
> +}
> +
> +static struct eqos_ops eqos_rockchip_ops = {
> +	.eqos_inval_desc = eqos_inval_desc_generic,
> +	.eqos_flush_desc = eqos_flush_desc_generic,
> +	.eqos_inval_buffer = eqos_inval_buffer_generic,
> +	.eqos_flush_buffer = eqos_flush_buffer_generic,
> +	.eqos_probe_resources = eqos_probe_resources_rk,
> +	.eqos_remove_resources = eqos_remove_resources_rk,
> +	.eqos_stop_resets = eqos_stop_resets_rk,
> +	.eqos_start_resets = eqos_start_resets_rk,
> +	.eqos_stop_clks = eqos_stop_clks_rk,
> +	.eqos_start_clks = eqos_start_clks_rk,
> +	.eqos_calibrate_pads = eqos_null_ops,
> +	.eqos_disable_calibration = eqos_null_ops,
> +	.eqos_set_tx_clk_speed = eqos_set_tx_clk_speed_rk,
> +	.eqos_get_enetaddr = eqos_null_ops,
> +	.eqos_get_tick_clk_rate = eqos_get_tick_clk_rate_rk,
> +};
> +
> +struct eqos_config eqos_rockchip_config = {
> +	.reg_access_always_ok = false,
> +	.mdio_wait = 10,
> +	.swr_wait = 50,
> +	.config_mac = EQOS_MAC_RXQ_CTRL0_RXQ0EN_ENABLED_DCB,
> +	.config_mac_mdio = EQOS_MAC_MDIO_ADDRESS_CR_100_150,
> +	.axi_bus_width = EQOS_AXI_WIDTH_64,
> +	.interface = dev_read_phy_mode,
> +	.ops = &eqos_rockchip_ops,
> +};



More information about the U-Boot mailing list