| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Dec 2014 08:46:08 +0800
From: Roger <roger.chen@...k-chips.com>
To: Heiko Stübner <heiko@...ech.de>
CC: peppe.cavallaro@...com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org,
kever.yang@...k-chips.com, eddie.cai@...k-chips.com
Subject: Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated
GMAC
Hi! Heiko
Any suggestion?
On 2014/12/3 15:57, Roger wrote:
> Hi! Heiko
>
> about regulator, power gpio, reset gpio and irq gpio
> please refer to my comment inline, tks.
>
> On 2014/12/2 7:44, Heiko Stübner wrote:
>> Hi Roger,
>>
>> the comments inline are a rough first review. I hope to get a clearer
>> picture
>> for the stuff I'm not sure about in v3 once the big issues are fixed.
>>
>>
>> Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
>>> This driver is based on stmmac driver.
>>>
>>> modification based on Giuseppe CAVALLARO's suggestion:
>>> 1. use BIT()
>>>
>>> > +/*RK3288_GRF_SOC_CON3*/
>>> > +#define GMAC_TXCLK_DLY_ENABLE ((0x4000 << 16) | (0x4000))
>>> > +#define GMAC_TXCLK_DLY_DISABLE ((0x4000 << 16) | (0x0000))
>>>
>>> ...
>>>
>>> why do not use BIT and BIT_MASK where possible?
>>>
>>> ===>after modification:
>>>
>>> #define GRF_BIT(nr) (BIT(nr) | BIT(nr+16))
>>> #define GRF_CLR_BIT(nr) (BIT(nr+16))
>>> #define GMAC_TXCLK_DLY_ENABLE GRF_BIT(14)
>>> #define GMAC_TXCLK_DLY_DISABLE GRF_CLR_BIT(14)
>>> ...
>>> 2.
>>>
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> > + GMAC_PHY_INTF_SEL_RGMII);
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> > + GMAC_RMII_MODE_CLR);
>>>
>>> maybe you could perform just one write unless there is some HW
>>> constraint.
>>>
>>> ===>after modification:
>>>
>>> regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>>
>>> 3. use macros
>>>
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E,
>>> 0xFFFFFFFF);
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
>>> > + 0x3<<2<<16 | 0x3<<2);
>>>
>>> pls use macros, these shift sequence is really help to decode
>>>
>>> ===>after modification:
>>>
>>> regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>>
>>> 4. remove grf fail check in rk_gmac_setup()
>>>
>>> > + if (IS_ERR(bsp_priv->grf))
>>> > + dev_err(&pdev->dev, "Missing rockchip,grf property\n");
>>>
>>> I wonder if you can fail on here and save all the check in
>>> set_rgmii_speed etc.
>>> Maybe this can be considered a mandatory property for the
>>> glue-logic.
>>>
>>> 5. remove .tx_coe=1
>>>
>>> > +const struct stmmac_of_data rk_gmac_data = {
>>> > + .has_gmac = 1,
>>> > + .tx_coe = 1,
>>>
>>> FYI, on new gmac there is the HW capability register to dinamically
>>> provide you if coe is supported.
>>>
>>> IMO you should add the OF "compatible" string and in case of mac
>>> newer than the 3.50a you can remove coe.
>> changelogs like these, should be compact and also not be in the
>> commit message
>> itself, but in the "comment"-section below the "---" and before the
>> diffstat.
>>
>>
>>> Signed-off-by: Roger Chen <roger.chen@...k-chips.com>
>>> ---
>> changelog here ... the commonly used pattern is something like
>>
>> changes since v2:
>> - ...
>> - ...
>>
>> changes since v1:
>> - ...
>>
>>> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
>>> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 636
>>> ++++++++++++++++++++
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.c |
>>> 3 +
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.h | 1 +
>>> 4 files changed, 641 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
>>> 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o
>>> stmmac_mdio.o
>>> ring_mode.o \
>>>
>>> obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>>> stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o
>>> dwmac-sunxi.o \
>>> - dwmac-sti.o dwmac-socfpga.o
>>> + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
>>>
>>> obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>>> stmmac-pci-objs:= stmmac_pci.o
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
>>> index 0000000..870563f
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> @@ -0,0 +1,636 @@
>>> +/**
>>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
>>> + *
>>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
>>> + *
>>> + * Chen-Zhi (Roger Chen) <roger.chen@...k-chips.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>> + * it under the terms of the GNU General Public License as
>>> published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/stmmac.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/phy.h>
>>> +#include <linux/of_net.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +struct rk_priv_data {
>>> + struct platform_device *pdev;
>>> + int phy_iface;
>>> + bool power_ctrl_by_pmu;
>>> + char pmu_regulator[32];
>>> + int pmu_enable_level;
>>> +
>>> + int power_io;
>>> + int power_io_level;
>>> + int reset_io;
>>> + int reset_io_level;
>>> + int phyirq_io;
>>> + int phyirq_io_level;
>>> +
>>> + bool clk_enabled;
>>> + bool clock_input;
>>> +
>>> + struct clk *clk_mac;
>>> + struct clk *clk_mac_pll;
>>> + struct clk *gmac_clkin;
>>> + struct clk *mac_clk_rx;
>>> + struct clk *mac_clk_tx;
>>> + struct clk *clk_mac_ref;
>>> + struct clk *clk_mac_refout;
>>> + struct clk *aclk_mac;
>>> + struct clk *pclk_mac;
>>> +
>>> + int tx_delay;
>>> + int rx_delay;
>>> +
>>> + struct regmap *grf;
>>> +};
>>> +
>>> +#define RK3288_GRF_SOC_CON1 0x0248
>>> +#define RK3288_GRF_SOC_CON3 0x0250
>>> +#define RK3288_GRF_GPIO3D_E 0x01ec
>>> +#define RK3288_GRF_GPIO4A_E 0x01f0
>>> +#define RK3288_GRF_GPIO4B_E 0x01f4
>> here you're using a space instead of a tab, please select one pattern
>> either
>> tabs or space but do not mix them.
>>
>>
>>> +#define GPIO3D_2MA 0xFFFF0000
>>> +#define GPIO3D_4MA 0xFFFF5555
>>> +#define GPIO3D_8MA 0xFFFFAAAA
>>> +#define GPIO3D_12MA 0xFFFFFFFF
>>> +
>>> +#define GPIO4A_2MA 0xFFFF0000
>>> +#define GPIO4A_4MA 0xFFFF5555
>>> +#define GPIO4A_8MA 0xFFFFAAAA
>>> +#define GPIO4A_12MA 0xFFFFFFFF
>> see comment about pin settings below
>>
>>
>>> +
>>> +#define GRF_BIT(nr) (BIT(nr) | BIT(nr+16))
>>> +#define GRF_CLR_BIT(nr) (BIT(nr+16))
>>> +
>>> +#define GPIO4B_2_2MA (GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_4MA (GRF_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_8MA (GRF_CLR_BIT(2) | GRF_BIT(3))
>>> +#define GPIO4B_2_12MA (GRF_BIT(2) | GRF_BIT(3))
>>> +
>>> +/*RK3288_GRF_SOC_CON1*/
>>> +#define GMAC_PHY_INTF_SEL_RGMII (GRF_BIT(6) | GRF_CLR_BIT(7) |
>>> GRF_CLR_BIT(8))
>>> +#define GMAC_PHY_INTF_SEL_RMII (GRF_CLR_BIT(6) |
>>> GRF_CLR_BIT(7) | GRF_BIT(8))
>>> +#define GMAC_FLOW_CTRL GRF_BIT(9)
>>> +#define GMAC_FLOW_CTRL_CLR GRF_CLR_BIT(9)
>>> +#define GMAC_SPEED_10M GRF_CLR_BIT(10)
>>> +#define GMAC_SPEED_100M GRF_BIT(10)
>>> +#define GMAC_RMII_CLK_25M GRF_BIT(11)
>>> +#define GMAC_RMII_CLK_2_5M GRF_CLR_BIT(11)
>>> +#define GMAC_CLK_125M (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
>>> +#define GMAC_CLK_25M (GRF_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_CLK_2_5M (GRF_CLR_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_RMII_MODE GRF_BIT(14)
>>> +#define GMAC_RMII_MODE_CLR GRF_CLR_BIT(14)
>>> +
>>> +/*RK3288_GRF_SOC_CON3*/
>>> +#define GMAC_TXCLK_DLY_ENABLE GRF_BIT(14)
>>> +#define GMAC_TXCLK_DLY_DISABLE GRF_CLR_BIT(14)
>>> +#define GMAC_RXCLK_DLY_ENABLE GRF_BIT(15)
>>> +#define GMAC_RXCLK_DLY_DISABLE GRF_CLR_BIT(15)
>>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
>>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
>> again mixed tabs and spaces as delimiters.
>>
>> Also the _CFG macros are not well abstracted. You could take a look
>> at the
>> HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:
>>
>> #define GMAC_CLK_DL_MASK 0x7f
>> #define GMAC_CLK_RX_DL_CFG(val) HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)
>> #define GMAC_CLK_TX_DL_CFG(val) HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)
>>
>>
>>> +
>>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
>>> + int tx_delay, int rx_delay)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> + GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
>>> + GMAC_CLK_RX_DL_CFG(rx_delay) |
>>> + GMAC_CLK_TX_DL_CFG(tx_delay));
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>> please don't write to parts controlled by other drivers - here the drive
>> strength settings of pins is controlled by the pinctrl driver.
>> Instead you can
>> just set the drive-strength in the pinctrl settings.
>>
>>
>>> +
>>> + pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
>>> + __func__, tx_delay, rx_delay);
>>> +}
>>> +
>>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>> you have a device-reference in rk_priv_data, so you could use dev_err
>> here.
>> Same for all other pr_err/pr_debug/pr_* calls in this file.
>>
>>
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RMII);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_MODE);
>> these two could be combined?
>>
>>
>>> +}
>>> +
>>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_2_5M);
>>> + else if (speed == 100)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_25M);
>>> + else if (speed == 1000)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_125M);
>>> + else
>>> + pr_err("unknown speed value for RGMII! speed=%d", speed);
>>> +}
>>> +
>>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_2_5M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_10M);
>> combine into one write?
>>
>>
>>> + } else if (speed == 100) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_25M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_100M);
>> combine into one write?
>>
>>
>>> + } else {
>>> + pr_err("unknown speed value for RMII! speed=%d", speed);
>>> + }
>>> +}
>>> +
>>> +#define MAC_CLK_RX "mac_clk_rx"
>>> +#define MAC_CLK_TX "mac_clk_tx"
>>> +#define CLK_MAC_REF "clk_mac_ref"
>>> +#define CLK_MAC_REF_OUT "clk_mac_refout"
>>> +#define CLK_MAC_PLL "clk_mac_pll"
>>> +#define ACLK_MAC "aclk_mac"
>>> +#define PCLK_MAC "pclk_mac"
>>> +#define MAC_CLKIN "ext_gmac"
>>> +#define CLK_MAC "stmmaceth"
>> why the need to extra constants for the clock names and not use the
>> real names
>> directly like most other drivers do?
>>
>>
>>> +
>>> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
>>> +{
>>> + struct device *dev = &bsp_priv->pdev->dev;
>>> +
>>> + bsp_priv->clk_enabled = false;
>>> +
>>> + bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
>>> + if (IS_ERR(bsp_priv->mac_clk_rx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_RX);
>>> +
>>> + bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
>>> + if (IS_ERR(bsp_priv->mac_clk_tx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_TX);
>>> +
>>> + bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
>>> + if (IS_ERR(bsp_priv->clk_mac_ref))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF);
>>> +
>>> + bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
>>> + if (IS_ERR(bsp_priv->clk_mac_refout))
>>> + pr_warn("%s: warning:cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF_OUT);
>>> +
>>> + bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
>>> + if (IS_ERR(bsp_priv->aclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, ACLK_MAC);
>>> +
>>> + bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
>>> + if (IS_ERR(bsp_priv->pclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, PCLK_MAC);
>>> +
>>> + bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
>>> + if (IS_ERR(bsp_priv->clk_mac_pll))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_PLL);
>>> +
>>> + bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
>>> + if (IS_ERR(bsp_priv->gmac_clkin))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLKIN);
>>> +
>>> + bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
>>> + if (IS_ERR(bsp_priv->clk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC);
>> there is not clk_put in the _remove case ... maybe you could simply use
>> devm_clk_get here so that all clocks are put on device removal.
>>
>> Also you're warning on every missing clock. Below it looks like you
>> need a
>> different set of them for rgmii and rmii, so maybe you should simply
>> error out
>> when core clocks for the selected phy-mode are missing.
>>
>>
>>> +
>>> + if (bsp_priv->clock_input) {
>>> + pr_info("%s: clock input from PHY\n", __func__);
>>> + } else {
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
>>> +
>>> + clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
>> why the explicit reparenting. The common clock-framework is
>> intelligent enough
>> to select the best suitable parent.
>>
>> In general I'm thinking the clock-handling inside this driver should be
>> simplyfied, as the common-clock framework can handle most cases
>> itself. I.e. if
>> a 125MHz external clock is present and so on. But haven't looked to
>> deep yet.
>>
>>
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int phy_iface = phy_iface = bsp_priv->phy_iface;
>>> +
>>> + if (enable) {
>>> + if (!bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_prepare_enable(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_prepare_enable(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_prepare_enable(
>>> + bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> + clk_prepare_enable(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> + clk_prepare_enable(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_prepare_enable(bsp_priv->mac_clk_tx);
>>> +
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_prepare_enable(bsp_priv->clk_mac);
>>> + */
>>> + mdelay(5);
>>> + bsp_priv->clk_enabled = true;
>>> + }
>>> + } else {
>>> + if (bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_disable_unprepare(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_disable_unprepare(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_disable_unprepare(
>>> + bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> + clk_disable_unprepare(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> + clk_disable_unprepare(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_disable_unprepare(bsp_priv->mac_clk_tx);
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_disable_unprepare(bsp_priv->clk_mac);
>>> + */
>>> + bsp_priv->clk_enabled = false;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + struct regulator *ldo;
>>> + char *ldostr = bsp_priv->pmu_regulator;
>>> + int ret;
>>> +
>>> + if (!ldostr) {
>>> + pr_err("%s: no ldo found\n", __func__);
>>> + return -1;
>>> + }
>>> +
>>> + ldo = regulator_get(NULL, ldostr);
>>> + if (!ldo) {
>>> + pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
>>> + } else {
>>> + if (enable) {
>>> + if (!regulator_is_enabled(ldo)) {
>>> + regulator_set_voltage(ldo, 3300000, 3300000);
>>> + ret = regulator_enable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to enable %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn on ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is enabled before enable", ldostr);
>>> + }
>>> + } else {
>>> + if (regulator_is_enabled(ldo)) {
>>> + ret = regulator_disable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to disable %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn off ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is disabled before disable",
>>> + ldostr);
>>> + }
>>> + }
>>> + regulator_put(ldo);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool
>>> enable)
>>> +{
>>> + if (enable) {
>>> + /*power on*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + bsp_priv->power_io_level);
>>> + } else {
>>> + /*power off*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + !bsp_priv->power_io_level);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int ret = -1;
>>> +
>>> + pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
>>> +
>>> + if (bsp_priv->power_ctrl_by_pmu)
>>> + ret = power_on_by_pmu(bsp_priv, enable);
>>> + else
>>> + ret = power_on_by_gpio(bsp_priv, enable);
>> this looks wrong. This should always be a regulator. Even a regulator
>> + switch
>> controlled by a gpio can still be modelled as regulator, so that you
>> don't
>> need this switch and assorted special handling - so just use the
>> regulator API
>>
> In some case, it would be a switching circuit to control the power for
> PHY.
> All I need to do is to control a GPIO to make switch on/off. So...
>>> +
>>> + if (enable) {
>>> + /*reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + mdelay(5);
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + !bsp_priv->reset_io_level);
>>> + }
>>> + mdelay(30);
>>> +
>>> + } else {
>>> + /*pull down reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + }
>>> + }
>> I'm not sure yet if it would be better to use the reset framework for
>> this.
>> While it says it is also meant for reset-gpios, there does not seem a
>> driver
>> for this to exist yet.
>>
> What should I do?
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +#define GPIO_PHY_POWER "gmac_phy_power"
>>> +#define GPIO_PHY_RESET "gmac_phy_reset"
>>> +#define GPIO_PHY_IRQ "gmac_phy_irq"
>> again I don't understand why these constants are necessary
>>
>>> +
>>> +static void *rk_gmac_setup(struct platform_device *pdev)
>>> +{
>>> + struct rk_priv_data *bsp_priv;
>>> + struct device *dev = &pdev->dev;
>>> + enum of_gpio_flags flags;
>>> + int ret;
>>> + const char *strings = NULL;
>>> + int value;
>>> + int irq;
>>> +
>>> + bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
>>> + if (!bsp_priv)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>>> +
>>> + ret = of_property_read_string(dev->of_node, "pmu_regulator",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_regulator.\n",
>>> __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
>>> + __func__, strings);
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + strcpy(bsp_priv->pmu_regulator, strings);
>>> + }
>> There is a generic regulator-dt-binding for regulator-consumers
>> available
>> which you should of course use.
>>
> The same explanation as above
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "pmu_enable_level",
>>> &value);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_enable_level.\n",
>>> + __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: PHY power controlled by pmu(level = %s).\n",
>>> + __func__, (value == 1) ? "HIGH" : "LOW");
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + bsp_priv->pmu_enable_level = value;
>>> + }
>> What is this used for? Enabling should of course be done via
>> regulator_enable
>> and disabling using regulator_disable.
>>
>>
>>> +
>>> + ret = of_property_read_string(dev->of_node, "clock_in_out",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: clock_in_out.\n",
>>> __func__);
>>> + bsp_priv->clock_input = true;
>>> + } else {
>>> + pr_info("%s: clock input or output? (%s).\n",
>>> + __func__, strings);
>>> + if (!strcmp(strings, "input"))
>>> + bsp_priv->clock_input = true;
>>> + else
>>> + bsp_priv->clock_input = false;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->tx_delay = 0x30;
>>> + pr_err("%s: Can not read property: tx_delay.", __func__);
>>> + pr_err("%s: set tx_delay to 0x%x\n",
>>> + __func__, bsp_priv->tx_delay);
>>> + } else {
>>> + pr_info("%s: TX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->tx_delay = value;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->rx_delay = 0x10;
>>> + pr_err("%s: Can not read property: rx_delay.", __func__);
>>> + pr_err("%s: set rx_delay to 0x%x\n",
>>> + __func__, bsp_priv->rx_delay);
>>> + } else {
>>> + pr_info("%s: RX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->rx_delay = value;
>>> + }
>>> +
>>> + bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> + "rockchip,grf");
>>> + bsp_priv->phyirq_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "phyirq-gpio", 0, &flags);
>>> + bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + bsp_priv->reset_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "reset-gpio", 0, &flags);
>>> + bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + bsp_priv->power_io =
>>> + of_get_named_gpio_flags(dev->of_node, "power-gpio", 0,
>>> &flags);
>>> + bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + /*power*/
>>> + if (!gpio_is_valid(bsp_priv->power_io)) {
>>> + pr_err("%s: Failed to get GPIO %s.\n",
>>> + __func__, "power-gpio");
>>> + } else {
>>> + ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_POWER);
>>> + }
>> When everything power-related is handled using the regulator api, you
>> don't
>> need this
> The same explanation as above
>>
>>> +
>>> + if (!gpio_is_valid(bsp_priv->reset_io)) {
>>> + pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
>>> + } else {
>>> + ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_RESET);
>>> + }
>>> +
>>> + if (bsp_priv->phyirq_io > 0) {
>> This is more for my understanding: why does the mac driver need to
>> handle the
>> phy interrupt - but I might be overlooking something.
>>
> phy interrupt is not mandatory. In most of the time, in order to
> find something happen in PHY, for example,
> link is up or down, we just use polling method to read the phy's
> register in a timer.
> Buf if phy interrupt is in use, when link is up or down, phy
> interrupt pin will be assert to inform the CPU.
> I just implement the driver for phy interrupt gpio, not enable it as
> default.
>
>>> + struct plat_stmmacenet_data *plat_dat;
>>> +
>>> + pr_info("PHY irq in use\n");
>>> + ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
>>> + if (ret < 0) {
>>> + pr_warn("%s: Failed to request GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + goto goon;
>>> + }
>>> +
>>> + ret = gpio_direction_input(bsp_priv->phyirq_io);
>>> + if (ret < 0) {
>>> + pr_err("%s, Failed to set input for GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + irq = gpio_to_irq(bsp_priv->phyirq_io);
>>> + if (irq < 0) {
>>> + ret = irq;
>>> + pr_err("Failed to set irq for %s\n",
>>> + GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + plat_dat = dev_get_platdata(&pdev->dev);
>>> + if (plat_dat)
>>> + plat_dat->mdio_bus_data->probed_phy_irq = irq;
>>> + else
>>> + pr_err("%s: plat_data is NULL\n", __func__);
>>> + }
>>> +
>>> +goon:
>>> + /*rmii or rgmii*/
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
>>> + pr_info("%s: init for RGMII\n", __func__);
>>> + set_to_rgmii(bsp_priv, bsp_priv->tx_delay,
>>> bsp_priv->rx_delay);
>>> + } else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + pr_info("%s: init for RMII\n", __func__);
>>> + set_to_rmii(bsp_priv);
>>> + } else {
>>> + pr_err("%s: ERROR: NO interface defined!\n", __func__);
>>> + }
>>> +
>>> + bsp_priv->pdev = pdev;
>>> +
>>> + gmac_clk_init(bsp_priv);
>>> +
>>> + return bsp_priv;
>>> +}
>>> +
>>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> + int ret;
>>> +
>>> + ret = phy_power_on(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = gmac_clk_enable(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *gmac = priv;
>>> +
>>> + phy_power_on(gmac, false);
>>> + gmac_clk_enable(gmac, false);
>>> +}
>>> +
>>> +static void rk_fix_speed(void *priv, unsigned int speed)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> +
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
>>> + set_rgmii_speed(bsp_priv, speed);
>>> + else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + set_rmii_speed(bsp_priv, speed);
>>> + else
>>> + pr_err("unsupported interface %d", bsp_priv->phy_iface);
>>> +}
>>> +
>>> +const struct stmmac_of_data rk_gmac_data = {
>>> + .has_gmac = 1,
>>> + .fix_mac_speed = rk_fix_speed,
>>> + .setup = rk_gmac_setup,
>>> + .init = rk_gmac_init,
>>> + .exit = rk_gmac_exit,
>>> +};
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
>>> 15814b7..b4dee96 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -33,6 +33,7 @@
>>>
>>> static const struct of_device_id stmmac_dt_ids[] = {
>>> /* SoC specific glue layers should come before generic
>>> bindings */
>>> + { .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
>> please name that rk3288_gmac_data [of course the other occurences too]
>> It makes it easier to see which soc it is meant for and it's also not
>> save to
>> assume that the next one will use the same register + bit positions
>> in the
>> grf.
>>
>>
>>> { .compatible = "amlogic,meson6-dwmac", .data =
>>> &meson6_dwmac_data},
>>> { .compatible = "allwinner,sun7i-a20-gmac", .data =
>>> &sun7i_gmac_data},
>>> { .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
>>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct
>>> platform_device
>>> *pdev) return -ENOMEM;
>>> }
>>>
>>> + pdev->dev.platform_data = plat_dat;
>>> +
>>> ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>>> if (ret) {
>>> pr_err("%s: main dt probe failed", __func__);
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
>>> 25dd1f7..32a0516 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>>> extern const struct stmmac_of_data stih4xx_dwmac_data;
>>> extern const struct stmmac_of_data stid127_dwmac_data;
>>> extern const struct stmmac_of_data socfpga_gmac_data;
>>> +extern const struct stmmac_of_data rk_gmac_data;
>>>
>>> #endif /* __STMMAC_PLATFORM_H__ */
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists