Re: [PATCH 2/3] net: stmmac: add a glue driver for the Amlogic Meson 8b / GXBB DWMAC
From: Kevin Hilman <khilman@baylibre.com>
Date: 2016-08-19 21:40:00
Also in:
linux-amlogic, linux-arm-kernel, linux-devicetree
Martin Blumenstingl [off-list ref] writes:
The Ethernet controller available in Meson8b and GXBB SoCs is a Synopsys DesignWare MAC IP core which is already supported by the stmmac driver. In addition to the standard stmmac driver some Meson8b / GXBB specific registers have to be configured for the PHY clocks. These SoC specific registers are called PRG_ETHERNET_ADDR0 and PRG_ETHERNET_ADDR1 in the datasheet. These registers are not backwards compatible with those on Meson 6b, which is why a new glue driver is introduced. This worked for many boards because the bootloader programs the PRG_ETHERNET registers correctly. Additionally the meson6-dwmac driver only sets bit 1 of PRG_ETHERNET_ADDR0 which (according to the datasheet) is only used during reset. Currently all configuration values can be determined automatically, based on the configured phy-mode (which is mandatory for the stmmac driver). If required the tx-delay and the mux clock (so it supports the MPLL2 clock as well) can be made configurable in the future. Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
[...]
quoted hunk
+static int meson8b_init_clk(struct meson8b_dwmac *dwmac) +{ + struct clk_init_data init; + int i, ret; + struct device *dev = &dwmac->pdev->dev; + char clk_name[32]; + const char *clk_div_parents[1]; + const char *mux_parent_names[MUX_CLK_NUM_PARENTS]; + unsigned int mux_parent_count = 0; + static struct clk_div_table clk_25m_div_table[] = { + { .val = 0, .div = 5 }, + { .val = 1, .div = 10 }, + { /* sentinel */ }, + }; + + /* get the mux parents from DT */ + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { + char name[16]; + + snprintf(name, sizeof(name), "clkin%d", i); + dwmac->m250_mux_parent[i] = devm_clk_get(dev, name); + if (IS_ERR(dwmac->m250_mux_parent[i])) { + /* NOTE: the second clock (MP2) is unused on all known
nit: multi-line comment style (c.f. Documentation/CodingStyle search for "multi-line")
quoted hunk
+ * boards, thus we're making it optional here. + */ + if (i > 0) + continue;
IMO, this is a bit confusing. I think this should either be coded to deal with an optional "clkin1" (preferred), or it should be coded to without a mux and clkin0 is directly the parent of the divider. The current way of always bailing out of this loop early is a bit confusing.
quoted hunk
+ ret = PTR_ERR(dwmac->m250_mux_parent[i]); + if (ret != -EPROBE_DEFER) + dev_err(dev, "Missing clock %s\n", name); + dwmac->m250_mux_parent[i] = NULL; + return ret; + } + + mux_parent_names[i] = + __clk_get_name(dwmac->m250_mux_parent[i]); + mux_parent_count++; + } + + /* create the m250_mux */ + snprintf(clk_name, sizeof(clk_name), "%s#m250_sel", dev_name(dev)); + init.name = clk_name; + init.ops = &clk_mux_ops; + init.flags = CLK_IS_BASIC; + init.parent_names = mux_parent_names; + init.num_parents = mux_parent_count; + + dwmac->m250_mux.reg = dwmac->regs + PRG_ETH0; + dwmac->m250_mux.shift = PRG_ETH0_CLK_M250_SEL_SHIFT; + dwmac->m250_mux.mask = PRG_ETH0_CLK_M250_SEL_MASK; + dwmac->m250_mux.flags = 0; + dwmac->m250_mux.table = NULL; + dwmac->m250_mux.hw.init = &init; + + dwmac->m250_mux_clk = devm_clk_register(dev, &dwmac->m250_mux.hw); + if (WARN_ON(PTR_ERR_OR_ZERO(dwmac->m250_mux_clk))) + return PTR_ERR(dwmac->m250_mux_clk); + + /* create the m250_div */ + snprintf(clk_name, sizeof(clk_name), "%s#m250_div", dev_name(dev)); + init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL); + init.ops = &clk_divider_ops; + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT;
hmm, with CLK_SET_RATE_PARENT that implies that it might switch the mux, but it's hard-coded above so the mux only ever has one parent, so that can never happen.
quoted hunk
+ clk_div_parents[0] = __clk_get_name(dwmac->m250_mux_clk); + init.parent_names = clk_div_parents; + init.num_parents = ARRAY_SIZE(clk_div_parents); + + dwmac->m250_div.reg = dwmac->regs + PRG_ETH0; + dwmac->m250_div.shift = PRG_ETH0_CLK_M250_DIV_SHIFT; + dwmac->m250_div.width = PRG_ETH0_CLK_M250_DIV_WIDTH; + dwmac->m250_div.hw.init = &init; + dwmac->m250_div.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO; + + dwmac->m250_div_clk = devm_clk_register(dev, &dwmac->m250_div.hw); + if (WARN_ON(PTR_ERR_OR_ZERO(dwmac->m250_div_clk))) + return PTR_ERR(dwmac->m250_div_clk); + + /* create the m25_div */ + snprintf(clk_name, sizeof(clk_name), "%s#m25_div", dev_name(dev)); + init.name = devm_kstrdup(dev, clk_name, GFP_KERNEL); + init.ops = &clk_divider_ops; + init.flags = CLK_IS_BASIC | CLK_SET_RATE_PARENT; + clk_div_parents[0] = __clk_get_name(dwmac->m250_div_clk); + init.parent_names = clk_div_parents; + init.num_parents = ARRAY_SIZE(clk_div_parents); + + dwmac->m25_div.reg = dwmac->regs + PRG_ETH0; + dwmac->m25_div.shift = PRG_ETH0_CLK_M25_DIV_SHIFT; + dwmac->m25_div.width = PRG_ETH0_CLK_M25_DIV_WIDTH; + dwmac->m25_div.table = clk_25m_div_table; + dwmac->m25_div.hw.init = &init; + dwmac->m25_div.flags = CLK_DIVIDER_ALLOW_ZERO; + + dwmac->m25_div_clk = devm_clk_register(dev, &dwmac->m25_div.hw); + if (WARN_ON(PTR_ERR_OR_ZERO(dwmac->m25_div_clk))) + return PTR_ERR(dwmac->m25_div_clk); + + return 0; +} + +static int meson8b_init_prg_eth(struct meson8b_dwmac *dwmac) +{ + int ret; + unsigned long clk_rate; + + switch (dwmac->phy_mode) { + case PHY_INTERFACE_MODE_RGMII: + case PHY_INTERFACE_MODE_RGMII_ID: + case PHY_INTERFACE_MODE_RGMII_RXID: + case PHY_INTERFACE_MODE_RGMII_TXID: + /* Generate a 25MHz clock for the PHY */ + clk_rate = 25 * 1000 * 1000; + + /* enable RGMII mode */ + meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE, + PRG_ETH0_RGMII_MODE); + + /* only relevant for RMII mode -> disable in RGMII mode */ + meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, + PRG_ETH0_INVERTED_RMII_CLK, 0); + + /* TX clock delay - all known boards use a 1/4 cycle delay */ + meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK, + PRG_ETH0_TXDLY_QUARTER); + break; + + case PHY_INTERFACE_MODE_RMII: + /* Use the rate of the mux clock for the internal RMII PHY */ + clk_rate = clk_get_rate(dwmac->m250_mux_clk); + + /* disable RGMII mode -> enables RMII mode */ + meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_RGMII_MODE, + 0); + + /* invert internal clk_rmii_i to generate 25/2.5 tx_rx_clk */ + meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, + PRG_ETH0_INVERTED_RMII_CLK, + PRG_ETH0_INVERTED_RMII_CLK); + + /* TX clock delay cannot be configured in RMII mode */ + meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TXDLY_MASK, + 0); + + break; + + default: + dev_err(&dwmac->pdev->dev, "unsupported phy-mode %s\n", + phy_modes(dwmac->phy_mode)); + return -EINVAL; + } + + ret = clk_prepare_enable(dwmac->m25_div_clk); + if (ret) { + dev_err(&dwmac->pdev->dev, "failed to enable the PHY clock\n"); + return ret; + } + + ret = clk_set_rate(dwmac->m25_div_clk, clk_rate); + if (ret) { + clk_disable_unprepare(dwmac->m25_div_clk); + + dev_err(&dwmac->pdev->dev, "failed to set PHY clock\n"); + return ret; + }
In the case of success, the clock is never disabled/unprepared. You probably need a .remove function which disables the clock and then calls stmmac_pltfr_remove.
quoted hunk
+ /* enable TX_CLK and PHY_REF_CLK generator */ + meson8b_dwmac_mask_bits(dwmac, PRG_ETH0, PRG_ETH0_TX_AND_PHY_REF_CLK, + PRG_ETH0_TX_AND_PHY_REF_CLK); + + return 0; +}
[...] Otherwise, this is looking good. Also, I tested on meson-gxbb-odroidc2 and meson-gxbb-p200 by booting a debian root filesystem over NFS and all was well. Tested-by: Kevin Hilman <khilman@baylibre.com> Kevin