| Message ID | 1542882521-18874-2-git-send-email-biao.huang@mediatek.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | add Ethernet driver support for mt2712 | expand |
On Thu, Nov 22, 2018 at 2:29 AM Biao Huang <biao.huang@mediatek.com> wrote: > > Add Ethernet support for MediaTek SoCs from the mt2712 family > > Signed-off-by: Biao Huang <biao.huang@mediatek.com> > --- > drivers/net/ethernet/stmicro/stmmac/Kconfig | 8 + > drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + > .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c | 364 ++++++++++++++++++++ > 3 files changed, 373 insertions(+) > create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig > index 324049e..6209cc1 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig > @@ -75,6 +75,14 @@ config DWMAC_LPC18XX > ---help--- > Support for NXP LPC18xx/43xx DWMAC Ethernet. > > +config DWMAC_MEDIATEK > + tristate "MediaTek MT27xx GMAC support" > + depends on OF && (ARCH_MEDIATEK || COMPILE_TEST) > + help > + Support for MediaTek GMAC Ethernet controller. > + > + This selects the MT2712 SoC support for the stmmac driver. > + > config DWMAC_MESON > tristate "Amlogic Meson dwmac support" > default ARCH_MESON > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile > index 99967a8..bf09701 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile > @@ -13,6 +13,7 @@ obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o > obj-$(CONFIG_DWMAC_ANARION) += dwmac-anarion.o > obj-$(CONFIG_DWMAC_IPQ806X) += dwmac-ipq806x.o > obj-$(CONFIG_DWMAC_LPC18XX) += dwmac-lpc18xx.o > +obj-$(CONFIG_DWMAC_MEDIATEK) += dwmac-mediatek.o > obj-$(CONFIG_DWMAC_MESON) += dwmac-meson.o dwmac-meson8b.o > obj-$(CONFIG_DWMAC_OXNAS) += dwmac-oxnas.o > obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c > new file mode 100644 > index 0000000..dd8d4cc > --- /dev/null > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c > @@ -0,0 +1,364 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018 MediaTek Inc. > + */ > +#include <linux/io.h> > +#include <linux/mfd/syscon.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_net.h> > +#include <linux/regmap.h> > +#include <linux/stmmac.h> > + > +#include "stmmac.h" > +#include "stmmac_platform.h" > + > +/* Peri Configuration register for mt2712 */ > +#define PERI_ETH_PHY_INTF_SEL 0x418 > +#define PHY_INTF_MII_GMII 0 > +#define PHY_INTF_RGMII 1 > +#define PHY_INTF_RMII 4 > +#define RMII_CLK_SRC_RXC BIT(4) > +#define RMII_CLK_SRC_INTERNAL BIT(5) > + > +#define PERI_ETH_PHY_DLY 0x428 > +#define PHY_DLY_GTXC_INV BIT(6) > +#define PHY_DLY_GTXC_ENABLE BIT(5) > +#define PHY_DLY_GTXC_STAGES GENMASK(4, 0) > +#define PHY_DLY_TXC_INV BIT(20) > +#define PHY_DLY_TXC_ENABLE BIT(19) > +#define PHY_DLY_TXC_STAGES GENMASK(18, 14) > +#define PHY_DLY_TXC_SHIFT 14 > +#define PHY_DLY_RXC_INV BIT(13) > +#define PHY_DLY_RXC_ENABLE BIT(12) > +#define PHY_DLY_RXC_STAGES GENMASK(11, 7) > +#define PHY_DLY_RXC_SHIFT 7 > + > +#define PERI_ETH_DLY_FINE 0x800 > +#define ETH_RMII_DLY_TX_INV BIT(2) > +#define ETH_FINE_DLY_GTXC BIT(1) > +#define ETH_FINE_DLY_RXC BIT(0) > + > +struct mac_delay_struct { > + u32 tx_delay; > + u32 rx_delay; > + u32 tx_inv; bool should be enough > + u32 rx_inv; bool should be enough > +}; > + > +struct mediatek_dwmac_plat_data { > + const struct mediatek_dwmac_variant *variant; > + struct mac_delay_struct mac_delay; > + struct clk_bulk_data *clks; > + struct device_node *np; > + struct regmap *peri_regmap; > + struct device *dev; > + int fine_tune; bool > + int phy_mode; > + int rmii_rxc; bool > +}; > + > +struct mediatek_dwmac_variant { > + int (*dwmac_set_phy_interface)(struct mediatek_dwmac_plat_data *plat); > + int (*dwmac_set_delay)(struct mediatek_dwmac_plat_data *plat); > + > + /* clock ids to be requested */ > + const char * const *clk_list; > + int num_clks; > + > + u32 dma_bit_mask; > + u32 rx_delay_max; > + u32 tx_delay_max; > +}; > + > +/* list of clocks required for mac */ > +static const char * const mt2712_dwmac_clk_l[] = { > + "axi", "apb", "mac_main", "ptp_ref" > +}; > + > +static int mt2712_set_interface(struct mediatek_dwmac_plat_data *plat) > +{ > + int rmii_rxc = plat->rmii_rxc ? RMII_CLK_SRC_RXC : 0; > + u32 intf_val = 0; > + > + /* select phy interface in top control domain */ > + switch (plat->phy_mode) { > + case PHY_INTERFACE_MODE_MII: > + intf_val |= PHY_INTF_MII_GMII; > + break; > + case PHY_INTERFACE_MODE_RMII: > + intf_val |= PHY_INTF_RMII; > + intf_val |= rmii_rxc; how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ? > + break; > + case PHY_INTERFACE_MODE_RGMII: > + case PHY_INTERFACE_MODE_RGMII_TXID: > + case PHY_INTERFACE_MODE_RGMII_RXID: > + case PHY_INTERFACE_MODE_RGMII_ID: > + intf_val |= PHY_INTF_RGMII; > + break; > + default: > + dev_err(plat->dev, "phy interface not supported\n"); > + return -EINVAL; > + } > + > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val); > + > + return 0; > +} > + > +static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat) > +{ > + struct mac_delay_struct *mac_delay = &plat->mac_delay; > + u32 delay_val = 0; > + u32 fine_val = 0; The same type declaration can be put into the one line > + > + switch (plat->phy_mode) { There exists some room for code optimization in the switch statement such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are almost the same and even the configuration for the other PHY modes can reuse their partial setup. It appears to be better using a common way to set up various PHY modes. > + case PHY_INTERFACE_MODE_MII: > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0; > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) & > + PHY_DLY_TXC_STAGES; > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0; > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > + PHY_DLY_RXC_STAGES; > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > + break; > + case PHY_INTERFACE_MODE_RMII: > + if (plat->rmii_rxc) { > + delay_val |= mac_delay->rx_delay ? > + PHY_DLY_RXC_ENABLE : 0; > + delay_val |= (mac_delay->rx_delay << > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES; > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > + fine_val |= mac_delay->tx_inv ? > + ETH_RMII_DLY_TX_INV : 0; why is fine_val got from tx_inv? > + } else { > + delay_val |= mac_delay->rx_delay ? > + PHY_DLY_TXC_ENABLE : 0; > + delay_val |= (mac_delay->rx_delay << > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES; > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0; > + fine_val |= mac_delay->tx_inv ? > + ETH_RMII_DLY_TX_INV : 0; ditto, why is fine_val got from tx_inv? > + } > + break; > + case PHY_INTERFACE_MODE_RGMII: > + fine_val = plat->fine_tune ? > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > + PHY_DLY_RXC_STAGES; > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > + break; > + case PHY_INTERFACE_MODE_RGMII_TXID: > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > + PHY_DLY_RXC_STAGES; > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register bits, not with *_TXC_* bits? I'm a little confused about what path the register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC? > + break; > + case PHY_INTERFACE_MODE_RGMII_RXID: > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; ditto, as the above quetion > + break; > + case PHY_INTERFACE_MODE_RGMII_ID: > + break; > + default: > + dev_err(plat->dev, "phy interface not supported\n"); > + return -EINVAL; > + } > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_DLY, delay_val); > + regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val); > + > + return 0; > +} > + > +static const struct mediatek_dwmac_variant mt2712_gmac_variant = { > + .dwmac_set_phy_interface = mt2712_set_interface, > + .dwmac_set_delay = mt2712_set_delay, > + .clk_list = mt2712_dwmac_clk_l, > + .num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l), > + .dma_bit_mask = 33, > + .rx_delay_max = 32, > + .tx_delay_max = 32, > +}; > + > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat) > +{ > + u32 tx_delay, rx_delay; > + > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg"); > + if (IS_ERR(plat->peri_regmap)) { > + dev_err(plat->dev, "Failed to get pericfg syscon\n"); > + return PTR_ERR(plat->peri_regmap); > + } > + > + plat->phy_mode = of_get_phy_mode(plat->np); > + if (plat->phy_mode < 0) { > + dev_err(plat->dev, "not find phy-mode\n"); > + return -EINVAL; > + } > + > + if (!of_property_read_u32(plat->np, "mediatek,tx-delay", &tx_delay)) { > + if (tx_delay < plat->variant->tx_delay_max) { > + plat->mac_delay.tx_delay = tx_delay; > + } else { > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay); > + return -EINVAL; > + } > + } > + > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) { > + if (rx_delay < plat->variant->rx_delay_max) { > + plat->mac_delay.rx_delay = rx_delay; > + } else { > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay); > + return -EINVAL; > + } > + } > + > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse"); > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse"); > + plat->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune"); > + plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc"); > + > + return 0; > +} > + > +static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat) > +{ > + const struct mediatek_dwmac_variant *variant = plat->variant; > + int num = variant->num_clks; > + int i; put into the same line seems good > + > + plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL); > + if (!plat->clks) > + return -ENOMEM; > + > + for (i = 0; i < num; i++) > + plat->clks[i].id = variant->clk_list[i]; > + > + return devm_clk_bulk_get(plat->dev, num, plat->clks); > +} > + > +static int mediatek_dwmac_init(struct platform_device *pdev, void *priv) > +{ > + struct mediatek_dwmac_plat_data *plat = priv; > + const struct mediatek_dwmac_variant *variant = plat->variant; > + int ret = 0; zero initialized seems unnecessary > + > + ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask)); > + if (ret) { > + dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret); > + return ret; > + } > + > + ret = variant->dwmac_set_phy_interface(plat); > + if (ret) { > + dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret); > + return ret; > + } > + > + ret = variant->dwmac_set_delay(plat); > + if (ret) { > + dev_err(plat->dev, "failed to set delay value, err = %d\n", ret); > + return ret; > + } > + > + ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks); > + if (ret) { > + dev_err(plat->dev, "failed to enable clks, err = %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv) > +{ > + struct mediatek_dwmac_plat_data *plat = priv; > + const struct mediatek_dwmac_variant *variant = plat->variant; > + > + clk_bulk_disable_unprepare(variant->num_clks, plat->clks); > +} > + > +static int mediatek_dwmac_probe(struct platform_device *pdev) > +{ > + struct mediatek_dwmac_plat_data *priv_plat; > + struct plat_stmmacenet_data *plat_dat; > + struct stmmac_resources stmmac_res; > + int ret = 0; zero initialized seems unnecessary > + > + priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL); > + if (!priv_plat) > + return -ENOMEM; > + > + priv_plat->variant = of_device_get_match_data(&pdev->dev); > + if (!priv_plat->variant) { > + dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n"); > + return -EINVAL; > + } > + > + priv_plat->dev = &pdev->dev; > + priv_plat->np = pdev->dev.of_node; > + > + ret = mediatek_dwmac_config_dt(priv_plat); > + if (ret) > + return ret; > + > + ret = mediatek_dwmac_clk_init(priv_plat); > + if (ret) > + return ret; > + > + ret = stmmac_get_platform_resources(pdev, &stmmac_res); > + if (ret) > + return ret; > + > + plat_dat = stmmac_probe_config_dt(pdev, &stmmac_res.mac); > + if (IS_ERR(plat_dat)) > + return PTR_ERR(plat_dat); > + > + plat_dat->interface = priv_plat->phy_mode; > + /* clk_csr_i = 250-300MHz & MDC = clk_csr_i/124 */ > + plat_dat->clk_csr = 5; > + plat_dat->has_gmac4 = 1; > + plat_dat->has_gmac = 0; > + plat_dat->pmt = 0; > + plat_dat->maxmtu = ETH_DATA_LEN; > + plat_dat->bsp_priv = priv_plat; > + plat_dat->init = mediatek_dwmac_init; > + plat_dat->exit = mediatek_dwmac_exit; > + mediatek_dwmac_init(pdev, priv_plat); > + > + ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); > + if (ret) { > + stmmac_remove_config_dt(pdev, plat_dat); > + return ret; > + } > + > + return 0; > +} > + > +static const struct of_device_id mediatek_dwmac_match[] = { > + { .compatible = "mediatek,mt2712-gmac", > + .data = &mt2712_gmac_variant }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, mediatek_dwmac_match); > + > +static struct platform_driver mediatek_dwmac_driver = { > + .probe = mediatek_dwmac_probe, > + .remove = stmmac_pltfr_remove, > + .driver = { > + .name = "dwmac-mediatek", > + .pm = &stmmac_pltfr_pm_ops, > + .of_match_table = mediatek_dwmac_match, > + }, > +}; > +module_platform_driver(mediatek_dwmac_driver); > -- > 1.7.9.5 > > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Dear Sean, Thanks for your comments. On Thu, 2018-11-22 at 16:54 -0800, Sean Wang wrote: > On Thu, Nov 22, 2018 at 2:29 AM Biao Huang <biao.huang@mediatek.com> wrote: > > > > Add Ethernet support for MediaTek SoCs from the mt2712 family > > > > Signed-off-by: Biao Huang <biao.huang@mediatek.com> > > --- > > drivers/net/ethernet/stmicro/stmmac/Kconfig | 8 + > > drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + > > .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c | 364 ++++++++++++++++++++ > > 3 files changed, 373 insertions(+) > > create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig > > index 324049e..6209cc1 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig > > +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig > > @@ -75,6 +75,14 @@ config DWMAC_LPC18XX > > ---help--- > > Support for NXP LPC18xx/43xx DWMAC Ethernet. > > > > +config DWMAC_MEDIATEK > > + tristate "MediaTek MT27xx GMAC support" > > + depends on OF && (ARCH_MEDIATEK || COMPILE_TEST) > > + help > > + Support for MediaTek GMAC Ethernet controller. > > + > > + This selects the MT2712 SoC support for the stmmac driver. > > + > > config DWMAC_MESON > > tristate "Amlogic Meson dwmac support" > > default ARCH_MESON > > diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile > > index 99967a8..bf09701 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/Makefile > > +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile > > @@ -13,6 +13,7 @@ obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o > > obj-$(CONFIG_DWMAC_ANARION) += dwmac-anarion.o > > obj-$(CONFIG_DWMAC_IPQ806X) += dwmac-ipq806x.o > > obj-$(CONFIG_DWMAC_LPC18XX) += dwmac-lpc18xx.o > > +obj-$(CONFIG_DWMAC_MEDIATEK) += dwmac-mediatek.o > > obj-$(CONFIG_DWMAC_MESON) += dwmac-meson.o dwmac-meson8b.o > > obj-$(CONFIG_DWMAC_OXNAS) += dwmac-oxnas.o > > obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c > > new file mode 100644 > > index 0000000..dd8d4cc > > --- /dev/null > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c > > @@ -0,0 +1,364 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2018 MediaTek Inc. > > + */ > > +#include <linux/io.h> > > +#include <linux/mfd/syscon.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/of_net.h> > > +#include <linux/regmap.h> > > +#include <linux/stmmac.h> > > + > > +#include "stmmac.h" > > +#include "stmmac_platform.h" > > + > > +/* Peri Configuration register for mt2712 */ > > +#define PERI_ETH_PHY_INTF_SEL 0x418 > > +#define PHY_INTF_MII_GMII 0 > > +#define PHY_INTF_RGMII 1 > > +#define PHY_INTF_RMII 4 > > +#define RMII_CLK_SRC_RXC BIT(4) > > +#define RMII_CLK_SRC_INTERNAL BIT(5) > > + > > +#define PERI_ETH_PHY_DLY 0x428 > > +#define PHY_DLY_GTXC_INV BIT(6) > > +#define PHY_DLY_GTXC_ENABLE BIT(5) > > +#define PHY_DLY_GTXC_STAGES GENMASK(4, 0) > > +#define PHY_DLY_TXC_INV BIT(20) > > +#define PHY_DLY_TXC_ENABLE BIT(19) > > +#define PHY_DLY_TXC_STAGES GENMASK(18, 14) > > +#define PHY_DLY_TXC_SHIFT 14 > > +#define PHY_DLY_RXC_INV BIT(13) > > +#define PHY_DLY_RXC_ENABLE BIT(12) > > +#define PHY_DLY_RXC_STAGES GENMASK(11, 7) > > +#define PHY_DLY_RXC_SHIFT 7 > > + > > +#define PERI_ETH_DLY_FINE 0x800 > > +#define ETH_RMII_DLY_TX_INV BIT(2) > > +#define ETH_FINE_DLY_GTXC BIT(1) > > +#define ETH_FINE_DLY_RXC BIT(0) > > + > > +struct mac_delay_struct { > > + u32 tx_delay; > > + u32 rx_delay; > > + u32 tx_inv; > bool should be enough yes > > > + u32 rx_inv; > bool should be enough yes > > > +}; > > + > > +struct mediatek_dwmac_plat_data { > > + const struct mediatek_dwmac_variant *variant; > > + struct mac_delay_struct mac_delay; > > + struct clk_bulk_data *clks; > > + struct device_node *np; > > + struct regmap *peri_regmap; > > + struct device *dev; > > + int fine_tune; > bool > yes > > + int phy_mode; > > + int rmii_rxc; > bool yes > > > +}; > > + > > +struct mediatek_dwmac_variant { > > + int (*dwmac_set_phy_interface)(struct mediatek_dwmac_plat_data *plat); > > + int (*dwmac_set_delay)(struct mediatek_dwmac_plat_data *plat); > > + > > + /* clock ids to be requested */ > > + const char * const *clk_list; > > + int num_clks; > > + > > + u32 dma_bit_mask; > > + u32 rx_delay_max; > > + u32 tx_delay_max; > > +}; > > + > > +/* list of clocks required for mac */ > > +static const char * const mt2712_dwmac_clk_l[] = { > > + "axi", "apb", "mac_main", "ptp_ref" > > +}; > > + > > +static int mt2712_set_interface(struct mediatek_dwmac_plat_data *plat) > > +{ > > + int rmii_rxc = plat->rmii_rxc ? RMII_CLK_SRC_RXC : 0; > > + u32 intf_val = 0; > > + > > + /* select phy interface in top control domain */ > > + switch (plat->phy_mode) { > > + case PHY_INTERFACE_MODE_MII: > > + intf_val |= PHY_INTF_MII_GMII; > > + break; > > + case PHY_INTERFACE_MODE_RMII: > > + intf_val |= PHY_INTF_RMII; > > + intf_val |= rmii_rxc; > how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ? > ok, will change in next version. > > + break; > > + case PHY_INTERFACE_MODE_RGMII: > > + case PHY_INTERFACE_MODE_RGMII_TXID: > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > + case PHY_INTERFACE_MODE_RGMII_ID: > > + intf_val |= PHY_INTF_RGMII; > > + break; > > + default: > > + dev_err(plat->dev, "phy interface not supported\n"); > > + return -EINVAL; > > + } > > + > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val); > > + > > + return 0; > > +} > > + > > +static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat) > > +{ > > + struct mac_delay_struct *mac_delay = &plat->mac_delay; > > + u32 delay_val = 0; > > + u32 fine_val = 0; > The same type declaration can be put into the one line > Got it. > > + > > + switch (plat->phy_mode) { > > There exists some room for code optimization in the switch statement > such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are > almost the same and even the configuration for the other PHY modes can > reuse their partial setup. It appears to be better using a common way > to set up various PHY modes. > I'll try define a function to handle it. And how about like this: static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32 clk_shift, u32 clk_mask, u32 inv_bit) { u32 value = 0 value |= delay ? en_bit : 0; value |= (delay << clk_shift) & clk_mask; value |= inv ? inv_bit : 0; } case PHY_INTERFACE_MODE_MII: delay_value |= delay_setting(mac_delay->tx_delay, mac_delay->tx_inv, PHY_DLY_TXC_ENABLE, PHY_DLY_TXC_SHIFT, PHY_DLY_TXC_STAGES, PHY_DLY_TXC_INV); delay_value |= delay_setting(mac_delay->rx_delay, mac_delay->rx_inv, PHY_DLY_RXC_ENABLE, PHY_DLY_RXC_SHIFT, PHY_DLY_RXC_STAGES, PHY_DLY_RXC_INV); case PHY_INTERFACE_MODE_RMII: if (plat->rmii_rxc) { delay_value |= delay_setting(mac_delay->rx_delay, mac_delay->rx_inv, PHY_DLY_RXC_ENABLE, PHY_DLY_RXC_SHIFT, PHY_DLY_RXC_STAGES, PHY_DLY_RXC_INV); fine_val |= mac_delay->tx_inv ? ETH_RMII_DLY_TX_INV : 0; } else { delay_value |= delay_setting(mac_delay->rx_delay, mac_delay->rx_inv, PHY_DLY_TXC_ENABLE, PHY_DLY_TXC_SHIFT, PHY_DLY_TXC_STAGES, PHY_DLY_TXC_INV); fine_val |= mac_delay->tx_inv ? ETH_RMII_DLY_TX_INV : 0; } case PHY_INTERFACE_MODE_RGMII: fine_val = plat->fine_tune ? (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; delay_value |= delay_setting(mac_delay->tx_delay, mac_delay->tx_inv, PHY_DLY_GTXC_ENABLE, PHY_DLY_GTXC_SHIFT, PHY_DLY_GTXC_STAGES, PHY_DLY_GTXC_INV); delay_value |= delay_setting(mac_delay->rx_delay, mac_delay->rx_inv, PHY_DLY_RXC_ENABLE, PHY_DLY_RXC_SHIFT, PHY_DLY_RXC_STAGES, PHY_DLY_RXC_INV); case PHY_INTERFACE_MODE_RGMII_TXID: fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; delay_value |= delay_setting(mac_delay->rx_delay, mac_delay->rx_inv, PHY_DLY_RXC_ENABLE, PHY_DLY_RXC_SHIFT, PHY_DLY_RXC_STAGES, PHY_DLY_RXC_INV); case PHY_INTERFACE_MODE_RGMII_RXID: fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; delay_value |= delay_setting(mac_delay->tx_delay, mac_delay->tx_inv, PHY_DLY_GTXC_ENABLE, PHY_DLY_GTXC_SHIFT, PHY_DLY_GTXC_STAGES, PHY_DLY_GTXC_INV); > > + case PHY_INTERFACE_MODE_MII: > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0; > > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) & > > + PHY_DLY_TXC_STAGES; > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0; > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > + PHY_DLY_RXC_STAGES; > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > + break; > > + case PHY_INTERFACE_MODE_RMII: > > + if (plat->rmii_rxc) { > > + delay_val |= mac_delay->rx_delay ? > > + PHY_DLY_RXC_ENABLE : 0; > > + delay_val |= (mac_delay->rx_delay << > > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES; > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > + fine_val |= mac_delay->tx_inv ? > > + ETH_RMII_DLY_TX_INV : 0; > why is fine_val got from tx_inv? > becase the tx inv will inverse the tx clock inside mac relative to reference clock from external phy, and this bit is located in the same register with fine-tune. maybe I should rename fine_val to avoid misunderstanding. > > + } else { > > + delay_val |= mac_delay->rx_delay ? > > + PHY_DLY_TXC_ENABLE : 0; > > + delay_val |= (mac_delay->rx_delay << > > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES; > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0; > > + fine_val |= mac_delay->tx_inv ? > > + ETH_RMII_DLY_TX_INV : 0; > ditto, why is fine_val got from tx_inv? > same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the same register with fine-tune. > > + } > > + break; > > + case PHY_INTERFACE_MODE_RGMII: > > + fine_val = plat->fine_tune ? > > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > + PHY_DLY_RXC_STAGES; > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > + break; > > + case PHY_INTERFACE_MODE_RGMII_TXID: > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > + PHY_DLY_RXC_STAGES; > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register > bits, not with *_TXC_* bits? I'm a little confused about what path the > register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC? > The PHY_INTERFACE_MODE_RGMII_TXID is defined in Documentation/networking/phy.txt means phy will handle delay in tx path, so mac need handle delay in rx path here. > > + break; > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; > ditto, as the above quetion > Similar answer as above. > > + break; > > + case PHY_INTERFACE_MODE_RGMII_ID: > > + break; > > + default: > > + dev_err(plat->dev, "phy interface not supported\n"); > > + return -EINVAL; > > + } > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_DLY, delay_val); > > + regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val); > > + > > + return 0; > > +} > > + > > +static const struct mediatek_dwmac_variant mt2712_gmac_variant = { > > + .dwmac_set_phy_interface = mt2712_set_interface, > > + .dwmac_set_delay = mt2712_set_delay, > > + .clk_list = mt2712_dwmac_clk_l, > > + .num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l), > > + .dma_bit_mask = 33, > > + .rx_delay_max = 32, > > + .tx_delay_max = 32, > > +}; > > + > > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat) > > +{ > > + u32 tx_delay, rx_delay; > > + > > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg"); > > + if (IS_ERR(plat->peri_regmap)) { > > + dev_err(plat->dev, "Failed to get pericfg syscon\n"); > > + return PTR_ERR(plat->peri_regmap); > > + } > > + > > + plat->phy_mode = of_get_phy_mode(plat->np); > > + if (plat->phy_mode < 0) { > > + dev_err(plat->dev, "not find phy-mode\n"); > > + return -EINVAL; > > + } > > + > > + if (!of_property_read_u32(plat->np, "mediatek,tx-delay", &tx_delay)) { > > + if (tx_delay < plat->variant->tx_delay_max) { > > + plat->mac_delay.tx_delay = tx_delay; > > + } else { > > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay); > > + return -EINVAL; > > + } > > + } > > + > > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) { > > + if (rx_delay < plat->variant->rx_delay_max) { > > + plat->mac_delay.rx_delay = rx_delay; > > + } else { > > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay); > > + return -EINVAL; > > + } > > + } > > + > > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse"); > > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse"); > > + plat->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune"); > > + plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc"); > > + > > + return 0; > > +} > > + > > +static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat) > > +{ > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > + int num = variant->num_clks; > > + int i; > put into the same line seems good > ok > > + > > + plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL); > > + if (!plat->clks) > > + return -ENOMEM; > > + > > + for (i = 0; i < num; i++) > > + plat->clks[i].id = variant->clk_list[i]; > > + > > + return devm_clk_bulk_get(plat->dev, num, plat->clks); > > +} > > + > > +static int mediatek_dwmac_init(struct platform_device *pdev, void *priv) > > +{ > > + struct mediatek_dwmac_plat_data *plat = priv; > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > + int ret = 0; > zero initialized seems unnecessary > ok, will not initialized here > > + > > + ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask)); > > + if (ret) { > > + dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret); > > + return ret; > > + } > > + > > + ret = variant->dwmac_set_phy_interface(plat); > > + if (ret) { > > + dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret); > > + return ret; > > + } > > + > > + ret = variant->dwmac_set_delay(plat); > > + if (ret) { > > + dev_err(plat->dev, "failed to set delay value, err = %d\n", ret); > > + return ret; > > + } > > + > > + ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks); > > + if (ret) { > > + dev_err(plat->dev, "failed to enable clks, err = %d\n", ret); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv) > > +{ > > + struct mediatek_dwmac_plat_data *plat = priv; > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > + > > + clk_bulk_disable_unprepare(variant->num_clks, plat->clks); > > +} > > + > > +static int mediatek_dwmac_probe(struct platform_device *pdev) > > +{ > > + struct mediatek_dwmac_plat_data *priv_plat; > > + struct plat_stmmacenet_data *plat_dat; > > + struct stmmac_resources stmmac_res; > > + int ret = 0; > zero initialized seems unnecessary > ok, will not initialized here > > + > > + priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL); > > + if (!priv_plat) > > + return -ENOMEM; > > + > > + priv_plat->variant = of_device_get_match_data(&pdev->dev); > > + if (!priv_plat->variant) { > > + dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n"); > > + return -EINVAL; > > + } > > + > > + priv_plat->dev = &pdev->dev; > > + priv_plat->np = pdev->dev.of_node; > > + > > + ret = mediatek_dwmac_config_dt(priv_plat); > > + if (ret) > > + return ret; > > + > > + ret = mediatek_dwmac_clk_init(priv_plat); > > + if (ret) > > + return ret; > > + > > + ret = stmmac_get_platform_resources(pdev, &stmmac_res); > > + if (ret) > > + return ret; > > + > > + plat_dat = stmmac_probe_config_dt(pdev, &stmmac_res.mac); > > + if (IS_ERR(plat_dat)) > > + return PTR_ERR(plat_dat); > > + > > + plat_dat->interface = priv_plat->phy_mode; > > + /* clk_csr_i = 250-300MHz & MDC = clk_csr_i/124 */ > > + plat_dat->clk_csr = 5; > > + plat_dat->has_gmac4 = 1; > > + plat_dat->has_gmac = 0; > > + plat_dat->pmt = 0; > > + plat_dat->maxmtu = ETH_DATA_LEN; > > + plat_dat->bsp_priv = priv_plat; > > + plat_dat->init = mediatek_dwmac_init; > > + plat_dat->exit = mediatek_dwmac_exit; > > + mediatek_dwmac_init(pdev, priv_plat); > > + > > + ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); > > + if (ret) { > > + stmmac_remove_config_dt(pdev, plat_dat); > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static const struct of_device_id mediatek_dwmac_match[] = { > > + { .compatible = "mediatek,mt2712-gmac", > > + .data = &mt2712_gmac_variant }, > > + { } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, mediatek_dwmac_match); > > + > > +static struct platform_driver mediatek_dwmac_driver = { > > + .probe = mediatek_dwmac_probe, > > + .remove = stmmac_pltfr_remove, > > + .driver = { > > + .name = "dwmac-mediatek", > > + .pm = &stmmac_pltfr_pm_ops, > > + .of_match_table = mediatek_dwmac_match, > > + }, > > +}; > > +module_platform_driver(mediatek_dwmac_driver); > > -- > > 1.7.9.5 > > > > > > _______________________________________________ > > Linux-mediatek mailing list > > Linux-mediatek@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
< ... > > > > + /* select phy interface in top control domain */ > > > + switch (plat->phy_mode) { > > > + case PHY_INTERFACE_MODE_MII: > > > + intf_val |= PHY_INTF_MII_GMII; > > > + break; > > > + case PHY_INTERFACE_MODE_RMII: > > > + intf_val |= PHY_INTF_RMII; > > > + intf_val |= rmii_rxc; > > how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ? > > > ok, will change in next version. > > > + break; > > > + case PHY_INTERFACE_MODE_RGMII: > > > + case PHY_INTERFACE_MODE_RGMII_TXID: > > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > > + case PHY_INTERFACE_MODE_RGMII_ID: > > > + intf_val |= PHY_INTF_RGMII; > > > + break; > > > + default: > > > + dev_err(plat->dev, "phy interface not supported\n"); > > > + return -EINVAL; > > > + } > > > + > > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val); > > > + > > > + return 0; > > > +} > > > + > > > +static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat) > > > +{ > > > + struct mac_delay_struct *mac_delay = &plat->mac_delay; > > > + u32 delay_val = 0; > > > + u32 fine_val = 0; > > The same type declaration can be put into the one line > > > Got it. > > > + > > > + switch (plat->phy_mode) { > > > > There exists some room for code optimization in the switch statement > > such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are > > almost the same and even the configuration for the other PHY modes can > > reuse their partial setup. It appears to be better using a common way > > to set up various PHY modes. > > > I'll try define a function to handle it. > And how about like this: > static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32 > clk_shift, u32 clk_mask, u32 inv_bit) { > u32 value = 0 > > value |= delay ? en_bit : 0; > value |= (delay << clk_shift) & clk_mask; > value |= inv ? inv_bit : 0; > } > > case PHY_INTERFACE_MODE_MII: > delay_value |= delay_setting(mac_delay->tx_delay, > mac_delay->tx_inv, > PHY_DLY_TXC_ENABLE, > PHY_DLY_TXC_SHIFT, > PHY_DLY_TXC_STAGES, > PHY_DLY_TXC_INV); We can reuse FIELD_PREP defined in include/linux/bitfield.h to make up of the value instead of creating your own function delay_setting here, and also PHY_DLY_TXC_SHIFT macro can be trimmed while you're using FIED_PREP > delay_value |= delay_setting(mac_delay->rx_delay, > mac_delay->rx_inv, > PHY_DLY_RXC_ENABLE, > PHY_DLY_RXC_SHIFT, > PHY_DLY_RXC_STAGES, > PHY_DLY_RXC_INV); > > case PHY_INTERFACE_MODE_RMII: > if (plat->rmii_rxc) { > delay_value |= delay_setting(mac_delay->rx_delay, > mac_delay->rx_inv, > PHY_DLY_RXC_ENABLE, > PHY_DLY_RXC_SHIFT, > PHY_DLY_RXC_STAGES, > PHY_DLY_RXC_INV); > fine_val |= mac_delay->tx_inv ? > ETH_RMII_DLY_TX_INV : 0; > } else { > delay_value |= delay_setting(mac_delay->rx_delay, > mac_delay->rx_inv, shoudn't the parametors be mac_delay->tx_delay and mac_delay->tx_inv? > PHY_DLY_TXC_ENABLE, > PHY_DLY_TXC_SHIFT, > PHY_DLY_TXC_STAGES, > PHY_DLY_TXC_INV); > fine_val |= mac_delay->tx_inv ? > ETH_RMII_DLY_TX_INV : 0; if (plat->tx_inv) fine_val = ETH_RMII_DLY_TX_INV; the default fine_val is zero so zero assignement can be trimmed when !plat-> tx_inv > } > case PHY_INTERFACE_MODE_RGMII: > fine_val = plat->fine_tune ? > (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; if (plat->fine_tune) fine_val = ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC; the default fine_val is zero so zero assignement can be trimmed when !plat->fine_tune > delay_value |= delay_setting(mac_delay->tx_delay, > mac_delay->tx_inv, > PHY_DLY_GTXC_ENABLE, > PHY_DLY_GTXC_SHIFT, > PHY_DLY_GTXC_STAGES, > PHY_DLY_GTXC_INV); > delay_value |= delay_setting(mac_delay->rx_delay, > mac_delay->rx_inv, > PHY_DLY_RXC_ENABLE, > PHY_DLY_RXC_SHIFT, > PHY_DLY_RXC_STAGES, > PHY_DLY_RXC_INV); > case PHY_INTERFACE_MODE_RGMII_TXID: > fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; > delay_value |= delay_setting(mac_delay->rx_delay, > mac_delay->rx_inv, > PHY_DLY_RXC_ENABLE, > PHY_DLY_RXC_SHIFT, > PHY_DLY_RXC_STAGES, > PHY_DLY_RXC_INV); > case PHY_INTERFACE_MODE_RGMII_RXID: > fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; > delay_value |= delay_setting(mac_delay->tx_delay, > mac_delay->tx_inv, > PHY_DLY_GTXC_ENABLE, > PHY_DLY_GTXC_SHIFT, > PHY_DLY_GTXC_STAGES, > PHY_DLY_GTXC_INV); > phy_mode is used to indicate what phy mode would be tweaked when mac is connected to the phy so I thought mac delay can be independent from phy internal delay that means PHY_INTERFACE_MODE_RGMII_RXID and PHY_INTERFACE_MODE_RGMII_TXID can apply the same setting as PHY_INTERFACE_MODE_RGMII does. > > > + case PHY_INTERFACE_MODE_MII: > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0; > > > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) & > > > + PHY_DLY_TXC_STAGES; > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0; > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > > + PHY_DLY_RXC_STAGES; > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > > + break; > > > + case PHY_INTERFACE_MODE_RMII: > > > + if (plat->rmii_rxc) { > > > + delay_val |= mac_delay->rx_delay ? > > > + PHY_DLY_RXC_ENABLE : 0; > > > + delay_val |= (mac_delay->rx_delay << > > > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES; > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > > + fine_val |= mac_delay->tx_inv ? > > > + ETH_RMII_DLY_TX_INV : 0; > > why is fine_val got from tx_inv? > > > becase the tx inv will inverse the tx clock inside mac relative to > reference clock from external phy, and this bit is located in the same > register with fine-tune. > maybe I should rename fine_val to avoid misunderstanding. If you add more comments to say that, fine_val remains would be okay > > > + } else { > > > + delay_val |= mac_delay->rx_delay ? > > > + PHY_DLY_TXC_ENABLE : 0; > > > + delay_val |= (mac_delay->rx_delay << > > > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES; > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0; > > > + fine_val |= mac_delay->tx_inv ? > > > + ETH_RMII_DLY_TX_INV : 0; > > ditto, why is fine_val got from tx_inv? > > > same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the > same register with fine-tune. adding a fewer comments helps to avoid some confusion > > > + } > > > + break; > > > + case PHY_INTERFACE_MODE_RGMII: > > > + fine_val = plat->fine_tune ? > > > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > > + PHY_DLY_RXC_STAGES; > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > > + break; > > > + case PHY_INTERFACE_MODE_RGMII_TXID: > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > > + PHY_DLY_RXC_STAGES; > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register > > bits, not with *_TXC_* bits? I'm a little confused about what path the > > register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC? > > > The PHY_INTERFACE_MODE_RGMII_TXID is defined in > Documentation/networking/phy.txt > means phy will handle delay in tx path, so mac need handle delay in rx > path here. See the above explains: phy_mode is used to indicate what phy mode would be tweaked when mac is connected to the phy so I thought mac delay can be independent of phy internal delay that means PHY_INTERFACE_MODE_RGMII_RXID and PHY_INTERFACE_MODE_RGMII_TXID can apply the same setting as PHY_INTERFACE_MODE_RGMII does. > > > + break; > > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; > > ditto, as the above quetion > > > Similar answer as above. > > > + break; > > > + case PHY_INTERFACE_MODE_RGMII_ID: > > > + break; > > > + default: > > > + dev_err(plat->dev, "phy interface not supported\n"); > > > + return -EINVAL; > > > + } > > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_DLY, delay_val); > > > + regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct mediatek_dwmac_variant mt2712_gmac_variant = { > > > + .dwmac_set_phy_interface = mt2712_set_interface, > > > + .dwmac_set_delay = mt2712_set_delay, > > > + .clk_list = mt2712_dwmac_clk_l, > > > + .num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l), > > > + .dma_bit_mask = 33, > > > + .rx_delay_max = 32, > > > + .tx_delay_max = 32, > > > +}; > > > + > > > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat) > > > +{ > > > + u32 tx_delay, rx_delay; > > > + > > > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg"); you're also missing the property definition in dt-binding. > > > + if (IS_ERR(plat->peri_regmap)) { > > > + dev_err(plat->dev, "Failed to get pericfg syscon\n"); > > > + return PTR_ERR(plat->peri_regmap); > > > + } > > > + > > > + plat->phy_mode = of_get_phy_mode(plat->np); > > > + if (plat->phy_mode < 0) { > > > + dev_err(plat->dev, "not find phy-mode\n"); > > > + return -EINVAL; > > > + } > > > + > > > + if (!of_property_read_u32(plat->np, "mediatek,tx-delay", &tx_delay)) { > > > + if (tx_delay < plat->variant->tx_delay_max) { > > > + plat->mac_delay.tx_delay = tx_delay; > > > + } else { > > > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) { > > > + if (rx_delay < plat->variant->rx_delay_max) { > > > + plat->mac_delay.rx_delay = rx_delay; > > > + } else { > > > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse"); > > > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse"); > > > + plat->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune"); > > > + plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc"); > > > + > > > + return 0; > > > +} > > > + > > > +static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat) > > > +{ > > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > > + int num = variant->num_clks; > > > + int i; > > put into the same line seems good > > > ok > > > + > > > + plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL); > > > + if (!plat->clks) > > > + return -ENOMEM; > > > + > > > + for (i = 0; i < num; i++) > > > + plat->clks[i].id = variant->clk_list[i]; > > > + > > > + return devm_clk_bulk_get(plat->dev, num, plat->clks); > > > +} > > > + > > > +static int mediatek_dwmac_init(struct platform_device *pdev, void *priv) > > > +{ > > > + struct mediatek_dwmac_plat_data *plat = priv; > > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > > + int ret = 0; > > zero initialized seems unnecessary > > > ok, will not initialized here > > > + > > > + ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask)); > > > + if (ret) { > > > + dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = variant->dwmac_set_phy_interface(plat); > > > + if (ret) { > > > + dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = variant->dwmac_set_delay(plat); > > > + if (ret) { > > > + dev_err(plat->dev, "failed to set delay value, err = %d\n", ret); > > > + return ret; > > > + } > > > + > > > + ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks); > > > + if (ret) { > > > + dev_err(plat->dev, "failed to enable clks, err = %d\n", ret); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv) > > > +{ > > > + struct mediatek_dwmac_plat_data *plat = priv; > > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > > + > > > + clk_bulk_disable_unprepare(variant->num_clks, plat->clks); > > > +} > > > + > > > +static int mediatek_dwmac_probe(struct platform_device *pdev) > > > +{ > > > + struct mediatek_dwmac_plat_data *priv_plat; > > > + struct plat_stmmacenet_data *plat_dat; > > > + struct stmmac_resources stmmac_res; > > > + int ret = 0; > > zero initialized seems unnecessary > > > ok, will not initialized here > > > + > > > + priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL); > > > + if (!priv_plat) > > > + return -ENOMEM; > > > + > > > + priv_plat->variant = of_device_get_match_data(&pdev->dev); > > > + if (!priv_plat->variant) { > > > + dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n"); > > > + return -EINVAL; > > > + } > > > + > > > + priv_plat->dev = &pdev->dev; > > > + priv_plat->np = pdev->dev.of_node; > > > + > > > + ret = mediatek_dwmac_config_dt(priv_plat); > > > + if (ret) > > > + return ret; > > > + > > > + ret = mediatek_dwmac_clk_init(priv_plat); > > > + if (ret) > > > + return ret; > > > + > > > + ret = stmmac_get_platform_resources(pdev, &stmmac_res); > > > + if (ret) > > > + return ret; > > > + < ... >
Dear Sean, Thanks for your comments. If any misunderstanding, please correct me. On Thu, 2018-11-22 at 23:04 -0800, Sean Wang wrote: > < ... > > > > > > + /* select phy interface in top control domain */ > > > > + switch (plat->phy_mode) { > > > > + case PHY_INTERFACE_MODE_MII: > > > > + intf_val |= PHY_INTF_MII_GMII; > > > > + break; > > > > + case PHY_INTERFACE_MODE_RMII: > > > > + intf_val |= PHY_INTF_RMII; > > > > + intf_val |= rmii_rxc; > > > how about putting into one line such as intf_val |= (PHY_INTF_RMII | rmii_rxc) ? > > > > > ok, will change in next version. > > > > + break; > > > > + case PHY_INTERFACE_MODE_RGMII: > > > > + case PHY_INTERFACE_MODE_RGMII_TXID: > > > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > > > + case PHY_INTERFACE_MODE_RGMII_ID: > > > > + intf_val |= PHY_INTF_RGMII; > > > > + break; > > > > + default: > > > > + dev_err(plat->dev, "phy interface not supported\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat) > > > > +{ > > > > + struct mac_delay_struct *mac_delay = &plat->mac_delay; > > > > + u32 delay_val = 0; > > > > + u32 fine_val = 0; > > > The same type declaration can be put into the one line > > > > > Got it. > > > > + > > > > + switch (plat->phy_mode) { > > > > > > There exists some room for code optimization in the switch statement > > > such as PHY_INTERFACE_MODE_MII and PHY_INTERFACE_MODE_RGMII both are > > > almost the same and even the configuration for the other PHY modes can > > > reuse their partial setup. It appears to be better using a common way > > > to set up various PHY modes. > > > > > I'll try define a function to handle it. > > And how about like this: > > static u32 delay_setting(u32 delay, bool inv, u32 en_bit, u32 > > clk_shift, u32 clk_mask, u32 inv_bit) { > > u32 value = 0 > > > > value |= delay ? en_bit : 0; > > value |= (delay << clk_shift) & clk_mask; > > value |= inv ? inv_bit : 0; > > } > > > > case PHY_INTERFACE_MODE_MII: > > delay_value |= delay_setting(mac_delay->tx_delay, > > mac_delay->tx_inv, > > PHY_DLY_TXC_ENABLE, > > PHY_DLY_TXC_SHIFT, > > PHY_DLY_TXC_STAGES, > > PHY_DLY_TXC_INV); > > We can reuse FIELD_PREP defined in include/linux/bitfield.h to make up > of the value instead of creating your own function delay_setting here, > and also PHY_DLY_TXC_SHIFT macro can be trimmed while you're using > FIED_PREP > ok, so it should like this: delay_value |= FIELD_PREP(PHY_DLY_TXC_ENABLE, !!mac_delay->delay); delay_value |= FIELD_PREP(PHY_DLY_TXC_STAGES, mac_delay->delay); delay_value |= FIELD_PREP(PHY_DLY_TXC_INV, mac_delay->inv); > > delay_value |= delay_setting(mac_delay->rx_delay, > > mac_delay->rx_inv, > > PHY_DLY_RXC_ENABLE, > > PHY_DLY_RXC_SHIFT, > > PHY_DLY_RXC_STAGES, > > PHY_DLY_RXC_INV); > > > > case PHY_INTERFACE_MODE_RMII: > > if (plat->rmii_rxc) { > > delay_value |= delay_setting(mac_delay->rx_delay, > > mac_delay->rx_inv, > > PHY_DLY_RXC_ENABLE, > > PHY_DLY_RXC_SHIFT, > > PHY_DLY_RXC_STAGES, > > PHY_DLY_RXC_INV); > > fine_val |= mac_delay->tx_inv ? > > ETH_RMII_DLY_TX_INV : 0; > > } else { > > delay_value |= delay_setting(mac_delay->rx_delay, > > mac_delay->rx_inv, > > shoudn't the parametors be mac_delay->tx_delay and mac_delay->tx_inv? > in this case, the rmii reference clk from external phy is connected to TXC pin, and property "rmii_rxc" will handle which pin the reference clk is connected to. after that, the reference clk is only a received clk from outside, so use rx_delay/rx_inv may be much better. > > PHY_DLY_TXC_ENABLE, > > PHY_DLY_TXC_SHIFT, > > PHY_DLY_TXC_STAGES, > > PHY_DLY_TXC_INV); > > fine_val |= mac_delay->tx_inv ? > > ETH_RMII_DLY_TX_INV : 0; > > if (plat->tx_inv) > fine_val = ETH_RMII_DLY_TX_INV; > the default fine_val is zero so zero assignement can be trimmed when > !plat-> tx_inv > > > } > > case PHY_INTERFACE_MODE_RGMII: > > fine_val = plat->fine_tune ? > > (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; > > if (plat->fine_tune) > fine_val = ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC; > the default fine_val is zero so zero assignement can be trimmed when > !plat->fine_tune > ok, I'll not initialize fine_val. > > delay_value |= delay_setting(mac_delay->tx_delay, > > mac_delay->tx_inv, > > PHY_DLY_GTXC_ENABLE, > > PHY_DLY_GTXC_SHIFT, > > PHY_DLY_GTXC_STAGES, > > PHY_DLY_GTXC_INV); > > delay_value |= delay_setting(mac_delay->rx_delay, > > mac_delay->rx_inv, > > PHY_DLY_RXC_ENABLE, > > PHY_DLY_RXC_SHIFT, > > PHY_DLY_RXC_STAGES, > > PHY_DLY_RXC_INV); > > case PHY_INTERFACE_MODE_RGMII_TXID: > > fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; > > delay_value |= delay_setting(mac_delay->rx_delay, > > mac_delay->rx_inv, > > PHY_DLY_RXC_ENABLE, > > PHY_DLY_RXC_SHIFT, > > PHY_DLY_RXC_STAGES, > > PHY_DLY_RXC_INV); > > case PHY_INTERFACE_MODE_RGMII_RXID: > > fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; > > delay_value |= delay_setting(mac_delay->tx_delay, > > mac_delay->tx_inv, > > PHY_DLY_GTXC_ENABLE, > > PHY_DLY_GTXC_SHIFT, > > PHY_DLY_GTXC_STAGES, > > PHY_DLY_GTXC_INV); > > > > phy_mode is used to indicate what phy mode would be tweaked when mac > is connected to the phy so I thought mac delay can be independent from > phy internal delay that means PHY_INTERFACE_MODE_RGMII_RXID and > PHY_INTERFACE_MODE_RGMII_TXID can apply the same setting as > PHY_INTERFACE_MODE_RGMII does. > no, when phy adjust the tx delay, mac should not adjust tx delay at the same time. ex: phy will delay 2ns, and mac delay 2ns, the total delay will be 2+2=4ns.the whole delay will not meet the rgmii requirement. And PHY_INTERFACE_MODE_RGMII_TXID/RXID/ID is clarified clearly in phy.txt, I think it should be ok here. More, I'll add comments here to avoid confusion. > > > > + case PHY_INTERFACE_MODE_MII: > > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0; > > > > + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) & > > > > + PHY_DLY_TXC_STAGES; > > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0; > > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > > > + PHY_DLY_RXC_STAGES; > > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > > > + break; > > > > + case PHY_INTERFACE_MODE_RMII: > > > > + if (plat->rmii_rxc) { > > > > + delay_val |= mac_delay->rx_delay ? > > > > + PHY_DLY_RXC_ENABLE : 0; > > > > + delay_val |= (mac_delay->rx_delay << > > > > + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES; > > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > > > + fine_val |= mac_delay->tx_inv ? > > > > + ETH_RMII_DLY_TX_INV : 0; > > > why is fine_val got from tx_inv? > > > > > becase the tx inv will inverse the tx clock inside mac relative to > > reference clock from external phy, and this bit is located in the same > > register with fine-tune. > > maybe I should rename fine_val to avoid misunderstanding. > > If you add more comments to say that, fine_val remains would be okay > OK, I'll add comments. > > > > + } else { > > > > + delay_val |= mac_delay->rx_delay ? > > > > + PHY_DLY_TXC_ENABLE : 0; > > > > + delay_val |= (mac_delay->rx_delay << > > > > + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES; > > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0; > > > > + fine_val |= mac_delay->tx_inv ? > > > > + ETH_RMII_DLY_TX_INV : 0; > > > ditto, why is fine_val got from tx_inv? > > > > > same as above. ETH_RMII_DLY_TX_INV is only for RMII, and located in the > > same register with fine-tune. > > adding a fewer comments helps to avoid some confusion > OK. > > > > + } > > > > + break; > > > > + case PHY_INTERFACE_MODE_RGMII: > > > > + fine_val = plat->fine_tune ? > > > > + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; > > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; > > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > > > + PHY_DLY_RXC_STAGES; > > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > > > + break; > > > > + case PHY_INTERFACE_MODE_RGMII_TXID: > > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; > > > > + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; > > > > + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & > > > > + PHY_DLY_RXC_STAGES; > > > > + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; > > > why is PHY_INTERFACE_MODE_RGMII_TXID applied with *_RXC_* register > > > bits, not with *_TXC_* bits? I'm a little confused about what path the > > > register PHY_DLY_RXC_* cause the effects to? MAC to PHY or PHY to MAC? > > > > > The PHY_INTERFACE_MODE_RGMII_TXID is defined in > > Documentation/networking/phy.txt > > means phy will handle delay in tx path, so mac need handle delay in rx > > path here. > > See the above explains: phy_mode is used to indicate what phy mode > would be tweaked when mac is connected to the phy so I thought mac > delay can be independent of phy internal delay that means > PHY_INTERFACE_MODE_RGMII_RXID and PHY_INTERFACE_MODE_RGMII_TXID can > apply the same setting as PHY_INTERFACE_MODE_RGMII does. > Maybe more comments will avoid confusion. > > > > + break; > > > > + case PHY_INTERFACE_MODE_RGMII_RXID: > > > > + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; > > > > + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; > > > > + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; > > > > + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; > > > ditto, as the above quetion > > > > > Similar answer as above. > > > > + break; > > > > + case PHY_INTERFACE_MODE_RGMII_ID: > > > > + break; > > > > + default: > > > > + dev_err(plat->dev, "phy interface not supported\n"); > > > > + return -EINVAL; > > > > + } > > > > + regmap_write(plat->peri_regmap, PERI_ETH_PHY_DLY, delay_val); > > > > + regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static const struct mediatek_dwmac_variant mt2712_gmac_variant = { > > > > + .dwmac_set_phy_interface = mt2712_set_interface, > > > > + .dwmac_set_delay = mt2712_set_delay, > > > > + .clk_list = mt2712_dwmac_clk_l, > > > > + .num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l), > > > > + .dma_bit_mask = 33, > > > > + .rx_delay_max = 32, > > > > + .tx_delay_max = 32, > > > > +}; > > > > + > > > > +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat) > > > > +{ > > > > + u32 tx_delay, rx_delay; > > > > + > > > > + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg"); > > you're also missing the property definition in dt-binding. > thanks for remind. I'll add in next patch. > > > > + if (IS_ERR(plat->peri_regmap)) { > > > > + dev_err(plat->dev, "Failed to get pericfg syscon\n"); > > > > + return PTR_ERR(plat->peri_regmap); > > > > + } > > > > + > > > > + plat->phy_mode = of_get_phy_mode(plat->np); > > > > + if (plat->phy_mode < 0) { > > > > + dev_err(plat->dev, "not find phy-mode\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + if (!of_property_read_u32(plat->np, "mediatek,tx-delay", &tx_delay)) { > > > > + if (tx_delay < plat->variant->tx_delay_max) { > > > > + plat->mac_delay.tx_delay = tx_delay; > > > > + } else { > > > > + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay); > > > > + return -EINVAL; > > > > + } > > > > + } > > > > + > > > > + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) { > > > > + if (rx_delay < plat->variant->rx_delay_max) { > > > > + plat->mac_delay.rx_delay = rx_delay; > > > > + } else { > > > > + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay); > > > > + return -EINVAL; > > > > + } > > > > + } > > > > + > > > > + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse"); > > > > + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse"); > > > > + plat->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune"); > > > > + plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc"); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat) > > > > +{ > > > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > > > + int num = variant->num_clks; > > > > + int i; > > > put into the same line seems good > > > > > ok > > > > + > > > > + plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL); > > > > + if (!plat->clks) > > > > + return -ENOMEM; > > > > + > > > > + for (i = 0; i < num; i++) > > > > + plat->clks[i].id = variant->clk_list[i]; > > > > + > > > > + return devm_clk_bulk_get(plat->dev, num, plat->clks); > > > > +} > > > > + > > > > +static int mediatek_dwmac_init(struct platform_device *pdev, void *priv) > > > > +{ > > > > + struct mediatek_dwmac_plat_data *plat = priv; > > > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > > > + int ret = 0; > > > zero initialized seems unnecessary > > > > > ok, will not initialized here > > > > + > > > > + ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask)); > > > > + if (ret) { > > > > + dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + ret = variant->dwmac_set_phy_interface(plat); > > > > + if (ret) { > > > > + dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + ret = variant->dwmac_set_delay(plat); > > > > + if (ret) { > > > > + dev_err(plat->dev, "failed to set delay value, err = %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks); > > > > + if (ret) { > > > > + dev_err(plat->dev, "failed to enable clks, err = %d\n", ret); > > > > + return ret; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv) > > > > +{ > > > > + struct mediatek_dwmac_plat_data *plat = priv; > > > > + const struct mediatek_dwmac_variant *variant = plat->variant; > > > > + > > > > + clk_bulk_disable_unprepare(variant->num_clks, plat->clks); > > > > +} > > > > + > > > > +static int mediatek_dwmac_probe(struct platform_device *pdev) > > > > +{ > > > > + struct mediatek_dwmac_plat_data *priv_plat; > > > > + struct plat_stmmacenet_data *plat_dat; > > > > + struct stmmac_resources stmmac_res; > > > > + int ret = 0; > > > zero initialized seems unnecessary > > > > > ok, will not initialized here > > > > + > > > > + priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL); > > > > + if (!priv_plat) > > > > + return -ENOMEM; > > > > + > > > > + priv_plat->variant = of_device_get_match_data(&pdev->dev); > > > > + if (!priv_plat->variant) { > > > > + dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n"); > > > > + return -EINVAL; > > > > + } > > > > + > > > > + priv_plat->dev = &pdev->dev; > > > > + priv_plat->np = pdev->dev.of_node; > > > > + > > > > + ret = mediatek_dwmac_config_dt(priv_plat); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = mediatek_dwmac_clk_init(priv_plat); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = stmmac_get_platform_resources(pdev, &stmmac_res); > > > > + if (ret) > > > > + return ret; > > > > + > > < ... >
diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig index 324049e..6209cc1 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig @@ -75,6 +75,14 @@ config DWMAC_LPC18XX ---help--- Support for NXP LPC18xx/43xx DWMAC Ethernet. +config DWMAC_MEDIATEK + tristate "MediaTek MT27xx GMAC support" + depends on OF && (ARCH_MEDIATEK || COMPILE_TEST) + help + Support for MediaTek GMAC Ethernet controller. + + This selects the MT2712 SoC support for the stmmac driver. + config DWMAC_MESON tristate "Amlogic Meson dwmac support" default ARCH_MESON diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile b/drivers/net/ethernet/stmicro/stmmac/Makefile index 99967a8..bf09701 100644 --- a/drivers/net/ethernet/stmicro/stmmac/Makefile +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile @@ -13,6 +13,7 @@ obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o obj-$(CONFIG_DWMAC_ANARION) += dwmac-anarion.o obj-$(CONFIG_DWMAC_IPQ806X) += dwmac-ipq806x.o obj-$(CONFIG_DWMAC_LPC18XX) += dwmac-lpc18xx.o +obj-$(CONFIG_DWMAC_MEDIATEK) += dwmac-mediatek.o obj-$(CONFIG_DWMAC_MESON) += dwmac-meson.o dwmac-meson8b.o obj-$(CONFIG_DWMAC_OXNAS) += dwmac-oxnas.o obj-$(CONFIG_DWMAC_ROCKCHIP) += dwmac-rk.o diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c new file mode 100644 index 0000000..dd8d4cc --- /dev/null +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c @@ -0,0 +1,364 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018 MediaTek Inc. + */ +#include <linux/io.h> +#include <linux/mfd/syscon.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/of_net.h> +#include <linux/regmap.h> +#include <linux/stmmac.h> + +#include "stmmac.h" +#include "stmmac_platform.h" + +/* Peri Configuration register for mt2712 */ +#define PERI_ETH_PHY_INTF_SEL 0x418 +#define PHY_INTF_MII_GMII 0 +#define PHY_INTF_RGMII 1 +#define PHY_INTF_RMII 4 +#define RMII_CLK_SRC_RXC BIT(4) +#define RMII_CLK_SRC_INTERNAL BIT(5) + +#define PERI_ETH_PHY_DLY 0x428 +#define PHY_DLY_GTXC_INV BIT(6) +#define PHY_DLY_GTXC_ENABLE BIT(5) +#define PHY_DLY_GTXC_STAGES GENMASK(4, 0) +#define PHY_DLY_TXC_INV BIT(20) +#define PHY_DLY_TXC_ENABLE BIT(19) +#define PHY_DLY_TXC_STAGES GENMASK(18, 14) +#define PHY_DLY_TXC_SHIFT 14 +#define PHY_DLY_RXC_INV BIT(13) +#define PHY_DLY_RXC_ENABLE BIT(12) +#define PHY_DLY_RXC_STAGES GENMASK(11, 7) +#define PHY_DLY_RXC_SHIFT 7 + +#define PERI_ETH_DLY_FINE 0x800 +#define ETH_RMII_DLY_TX_INV BIT(2) +#define ETH_FINE_DLY_GTXC BIT(1) +#define ETH_FINE_DLY_RXC BIT(0) + +struct mac_delay_struct { + u32 tx_delay; + u32 rx_delay; + u32 tx_inv; + u32 rx_inv; +}; + +struct mediatek_dwmac_plat_data { + const struct mediatek_dwmac_variant *variant; + struct mac_delay_struct mac_delay; + struct clk_bulk_data *clks; + struct device_node *np; + struct regmap *peri_regmap; + struct device *dev; + int fine_tune; + int phy_mode; + int rmii_rxc; +}; + +struct mediatek_dwmac_variant { + int (*dwmac_set_phy_interface)(struct mediatek_dwmac_plat_data *plat); + int (*dwmac_set_delay)(struct mediatek_dwmac_plat_data *plat); + + /* clock ids to be requested */ + const char * const *clk_list; + int num_clks; + + u32 dma_bit_mask; + u32 rx_delay_max; + u32 tx_delay_max; +}; + +/* list of clocks required for mac */ +static const char * const mt2712_dwmac_clk_l[] = { + "axi", "apb", "mac_main", "ptp_ref" +}; + +static int mt2712_set_interface(struct mediatek_dwmac_plat_data *plat) +{ + int rmii_rxc = plat->rmii_rxc ? RMII_CLK_SRC_RXC : 0; + u32 intf_val = 0; + + /* select phy interface in top control domain */ + switch (plat->phy_mode) { + case PHY_INTERFACE_MODE_MII: + intf_val |= PHY_INTF_MII_GMII; + break; + case PHY_INTERFACE_MODE_RMII: + intf_val |= PHY_INTF_RMII; + intf_val |= rmii_rxc; + break; + case PHY_INTERFACE_MODE_RGMII: + case PHY_INTERFACE_MODE_RGMII_TXID: + case PHY_INTERFACE_MODE_RGMII_RXID: + case PHY_INTERFACE_MODE_RGMII_ID: + intf_val |= PHY_INTF_RGMII; + break; + default: + dev_err(plat->dev, "phy interface not supported\n"); + return -EINVAL; + } + + regmap_write(plat->peri_regmap, PERI_ETH_PHY_INTF_SEL, intf_val); + + return 0; +} + +static int mt2712_set_delay(struct mediatek_dwmac_plat_data *plat) +{ + struct mac_delay_struct *mac_delay = &plat->mac_delay; + u32 delay_val = 0; + u32 fine_val = 0; + + switch (plat->phy_mode) { + case PHY_INTERFACE_MODE_MII: + delay_val |= mac_delay->tx_delay ? PHY_DLY_TXC_ENABLE : 0; + delay_val |= (mac_delay->tx_delay << PHY_DLY_TXC_SHIFT) & + PHY_DLY_TXC_STAGES; + delay_val |= mac_delay->tx_inv ? PHY_DLY_TXC_INV : 0; + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & + PHY_DLY_RXC_STAGES; + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; + break; + case PHY_INTERFACE_MODE_RMII: + if (plat->rmii_rxc) { + delay_val |= mac_delay->rx_delay ? + PHY_DLY_RXC_ENABLE : 0; + delay_val |= (mac_delay->rx_delay << + PHY_DLY_RXC_SHIFT) & PHY_DLY_RXC_STAGES; + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; + fine_val |= mac_delay->tx_inv ? + ETH_RMII_DLY_TX_INV : 0; + } else { + delay_val |= mac_delay->rx_delay ? + PHY_DLY_TXC_ENABLE : 0; + delay_val |= (mac_delay->rx_delay << + PHY_DLY_TXC_SHIFT) & PHY_DLY_TXC_STAGES; + delay_val |= mac_delay->rx_inv ? PHY_DLY_TXC_INV : 0; + fine_val |= mac_delay->tx_inv ? + ETH_RMII_DLY_TX_INV : 0; + } + break; + case PHY_INTERFACE_MODE_RGMII: + fine_val = plat->fine_tune ? + (ETH_FINE_DLY_GTXC | ETH_FINE_DLY_RXC) : 0; + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & + PHY_DLY_RXC_STAGES; + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; + break; + case PHY_INTERFACE_MODE_RGMII_TXID: + fine_val = plat->fine_tune ? ETH_FINE_DLY_RXC : 0; + delay_val |= mac_delay->rx_delay ? PHY_DLY_RXC_ENABLE : 0; + delay_val |= (mac_delay->rx_delay << PHY_DLY_RXC_SHIFT) & + PHY_DLY_RXC_STAGES; + delay_val |= mac_delay->rx_inv ? PHY_DLY_RXC_INV : 0; + break; + case PHY_INTERFACE_MODE_RGMII_RXID: + fine_val = plat->fine_tune ? ETH_FINE_DLY_GTXC : 0; + delay_val |= mac_delay->tx_delay ? PHY_DLY_GTXC_ENABLE : 0; + delay_val |= mac_delay->tx_delay & PHY_DLY_GTXC_STAGES; + delay_val |= mac_delay->tx_inv ? PHY_DLY_GTXC_INV : 0; + break; + case PHY_INTERFACE_MODE_RGMII_ID: + break; + default: + dev_err(plat->dev, "phy interface not supported\n"); + return -EINVAL; + } + regmap_write(plat->peri_regmap, PERI_ETH_PHY_DLY, delay_val); + regmap_write(plat->peri_regmap, PERI_ETH_DLY_FINE, fine_val); + + return 0; +} + +static const struct mediatek_dwmac_variant mt2712_gmac_variant = { + .dwmac_set_phy_interface = mt2712_set_interface, + .dwmac_set_delay = mt2712_set_delay, + .clk_list = mt2712_dwmac_clk_l, + .num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l), + .dma_bit_mask = 33, + .rx_delay_max = 32, + .tx_delay_max = 32, +}; + +static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat) +{ + u32 tx_delay, rx_delay; + + plat->peri_regmap = syscon_regmap_lookup_by_phandle(plat->np, "mediatek,pericfg"); + if (IS_ERR(plat->peri_regmap)) { + dev_err(plat->dev, "Failed to get pericfg syscon\n"); + return PTR_ERR(plat->peri_regmap); + } + + plat->phy_mode = of_get_phy_mode(plat->np); + if (plat->phy_mode < 0) { + dev_err(plat->dev, "not find phy-mode\n"); + return -EINVAL; + } + + if (!of_property_read_u32(plat->np, "mediatek,tx-delay", &tx_delay)) { + if (tx_delay < plat->variant->tx_delay_max) { + plat->mac_delay.tx_delay = tx_delay; + } else { + dev_err(plat->dev, "Invalid TX clock delay: %d\n", tx_delay); + return -EINVAL; + } + } + + if (!of_property_read_u32(plat->np, "mediatek,rx-delay", &rx_delay)) { + if (rx_delay < plat->variant->rx_delay_max) { + plat->mac_delay.rx_delay = rx_delay; + } else { + dev_err(plat->dev, "Invalid RX clock delay: %d\n", rx_delay); + return -EINVAL; + } + } + + plat->mac_delay.tx_inv = of_property_read_bool(plat->np, "mediatek,txc-inverse"); + plat->mac_delay.rx_inv = of_property_read_bool(plat->np, "mediatek,rxc-inverse"); + plat->fine_tune = of_property_read_bool(plat->np, "mediatek,fine-tune"); + plat->rmii_rxc = of_property_read_bool(plat->np, "mediatek,rmii-rxc"); + + return 0; +} + +static int mediatek_dwmac_clk_init(struct mediatek_dwmac_plat_data *plat) +{ + const struct mediatek_dwmac_variant *variant = plat->variant; + int num = variant->num_clks; + int i; + + plat->clks = devm_kcalloc(plat->dev, num, sizeof(*plat->clks), GFP_KERNEL); + if (!plat->clks) + return -ENOMEM; + + for (i = 0; i < num; i++) + plat->clks[i].id = variant->clk_list[i]; + + return devm_clk_bulk_get(plat->dev, num, plat->clks); +} + +static int mediatek_dwmac_init(struct platform_device *pdev, void *priv) +{ + struct mediatek_dwmac_plat_data *plat = priv; + const struct mediatek_dwmac_variant *variant = plat->variant; + int ret = 0; + + ret = dma_set_mask_and_coherent(plat->dev, DMA_BIT_MASK(variant->dma_bit_mask)); + if (ret) { + dev_err(plat->dev, "No suitable DMA available, err = %d\n", ret); + return ret; + } + + ret = variant->dwmac_set_phy_interface(plat); + if (ret) { + dev_err(plat->dev, "failed to set phy interface, err = %d\n", ret); + return ret; + } + + ret = variant->dwmac_set_delay(plat); + if (ret) { + dev_err(plat->dev, "failed to set delay value, err = %d\n", ret); + return ret; + } + + ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks); + if (ret) { + dev_err(plat->dev, "failed to enable clks, err = %d\n", ret); + return ret; + } + + return 0; +} + +static void mediatek_dwmac_exit(struct platform_device *pdev, void *priv) +{ + struct mediatek_dwmac_plat_data *plat = priv; + const struct mediatek_dwmac_variant *variant = plat->variant; + + clk_bulk_disable_unprepare(variant->num_clks, plat->clks); +} + +static int mediatek_dwmac_probe(struct platform_device *pdev) +{ + struct mediatek_dwmac_plat_data *priv_plat; + struct plat_stmmacenet_data *plat_dat; + struct stmmac_resources stmmac_res; + int ret = 0; + + priv_plat = devm_kzalloc(&pdev->dev, sizeof(*priv_plat), GFP_KERNEL); + if (!priv_plat) + return -ENOMEM; + + priv_plat->variant = of_device_get_match_data(&pdev->dev); + if (!priv_plat->variant) { + dev_err(&pdev->dev, "Missing dwmac-mediatek variant\n"); + return -EINVAL; + } + + priv_plat->dev = &pdev->dev; + priv_plat->np = pdev->dev.of_node; + + ret = mediatek_dwmac_config_dt(priv_plat); + if (ret) + return ret; + + ret = mediatek_dwmac_clk_init(priv_plat); + if (ret) + return ret; + + ret = stmmac_get_platform_resources(pdev, &stmmac_res); + if (ret) + return ret; + + plat_dat = stmmac_probe_config_dt(pdev, &stmmac_res.mac); + if (IS_ERR(plat_dat)) + return PTR_ERR(plat_dat); + + plat_dat->interface = priv_plat->phy_mode; + /* clk_csr_i = 250-300MHz & MDC = clk_csr_i/124 */ + plat_dat->clk_csr = 5; + plat_dat->has_gmac4 = 1; + plat_dat->has_gmac = 0; + plat_dat->pmt = 0; + plat_dat->maxmtu = ETH_DATA_LEN; + plat_dat->bsp_priv = priv_plat; + plat_dat->init = mediatek_dwmac_init; + plat_dat->exit = mediatek_dwmac_exit; + mediatek_dwmac_init(pdev, priv_plat); + + ret = stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res); + if (ret) { + stmmac_remove_config_dt(pdev, plat_dat); + return ret; + } + + return 0; +} + +static const struct of_device_id mediatek_dwmac_match[] = { + { .compatible = "mediatek,mt2712-gmac", + .data = &mt2712_gmac_variant }, + { } +}; + +MODULE_DEVICE_TABLE(of, mediatek_dwmac_match); + +static struct platform_driver mediatek_dwmac_driver = { + .probe = mediatek_dwmac_probe, + .remove = stmmac_pltfr_remove, + .driver = { + .name = "dwmac-mediatek", + .pm = &stmmac_pltfr_pm_ops, + .of_match_table = mediatek_dwmac_match, + }, +}; +module_platform_driver(mediatek_dwmac_driver);
Add Ethernet support for MediaTek SoCs from the mt2712 family Signed-off-by: Biao Huang <biao.huang@mediatek.com> --- drivers/net/ethernet/stmicro/stmmac/Kconfig | 8 + drivers/net/ethernet/stmicro/stmmac/Makefile | 1 + .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c | 364 ++++++++++++++++++++ 3 files changed, 373 insertions(+) create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c