| 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
| ||
|
Message-ID: <547EC24E.6050905@rock-chips.com>
Date: Wed, 03 Dec 2014 15:57:02 +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
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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists