Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932465AbcCMD6P (ORCPT ); Sat, 12 Mar 2016 22:58:15 -0500 Received: from mail-pa0-f68.google.com ([209.85.220.68]:33631 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932381AbcCMD6I (ORCPT ); Sat, 12 Mar 2016 22:58:08 -0500 Subject: Re: [PATCH 3/6] net: arc_emac: support the phy reset for emac driver To: Sergei Shtylyov References: <1457693731-6966-1-git-send-email-wxt@rock-chips.com> <1457693731-6966-4-git-send-email-wxt@rock-chips.com> <56E30FE5.1080607@cogentembedded.com> Cc: Caesar Wang , Heiko Stuebner , "David S. Miller" , Rob Herring , Alexander Kochetkov , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, keescook@google.com, leozwang@google.com From: Caesar Wang Message-ID: <56E4E53F.2060307@gmail.com> Date: Sun, 13 Mar 2016 11:57:51 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <56E30FE5.1080607@cogentembedded.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3799 Lines: 130 Hi Sergei, 在 2016年03月12日 02:35, Sergei Shtylyov 写道: > On 03/11/2016 01:55 PM, Caesar Wang wrote: > >> This patch adds to support the emac phy reset. >> >> 1) phy-reset-gpios: >> The phy-reset-gpios is an optional property for arc emac device tree >> boot. >> Change the binding document to match the driver code. > > The binding document is apparently changed in another patch. Not > sure what you wanted to say here... > >> 2) phy-reset-duration: >> Different boards may require different phy reset duration. Add property >> phy-reset-duration for device tree probe, so that the boards that need >> a longer reset duration can specify it in their device tree. >> >> 3) phy-reset-active-high: >> We need that for a custom hardware that needs the reverse reset >> sequence. > > No, we don't really need that, "phy-reset-gpio" prop can contain > this data. > >> Of course, this patch will fix the issue on >> https://patchwork.kernel.org/patch/8186801/. >> >> In some cases, the emac couldn't work if you don't have reset the phy. >> Let's add it to happy work. >> >> Signed-off-by: Caesar Wang >> --- >> >> drivers/net/ethernet/arc/emac_main.c | 41 >> ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 41 insertions(+) >> >> diff --git a/drivers/net/ethernet/arc/emac_main.c >> b/drivers/net/ethernet/arc/emac_main.c >> index 6446af1..42384f6a 100644 >> --- a/drivers/net/ethernet/arc/emac_main.c >> +++ b/drivers/net/ethernet/arc/emac_main.c >> @@ -764,6 +764,45 @@ static const struct net_device_ops >> arc_emac_netdev_ops = { >> #endif >> }; >> >> +#ifdef CONFIG_OF >> +static void emac_reset_phy(struct net_device *pdev) >> +{ >> + int err, phy_reset; >> + bool active_high = false; >> + int msec = 10; >> + struct device *dev = pdev->dev.parent; >> + struct device_node *np = dev->of_node; >> + >> + if (!np) >> + return; >> + >> + of_property_read_u32(np, "phy-reset-duration", &msec); >> + /* A sane reset duration should not be longer than 1s */ >> + if (msec > 1000) >> + msec = 1; >> + >> + phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); > > Forgot to say that these old integer-base GPIO APIs shouldn't be > used by the new code, there's new 'struct gpio_desc' based APIs, like > devm_gpiod_get() etc. > >> + if (!gpio_is_valid(phy_reset)) >> + return; >> + >> + active_high = of_property_read_bool(np, "phy-reset-active-high"); > > Well, I still don't understand why this prop is needed, while the > GPIO sense is transparently handled by the gpiolib (at least when > using DT). Okay, I have to admit that reference the exist way to do... wxt@nb:~/kernel/drivers/net/ethernet$ ag reset-gpios micrel/ks8851.c 1427: gpio = of_get_named_gpio_flags(spi->dev.of_node, "reset-gpios", arc/emac_main.c 787: phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); 797: dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); arc/emac_main.c~ 784: phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); 794: dev_err(dev, "failed to get phy-reset-gpios: %d\n", err); davicom/dm9000.c 1451: reset_gpios = of_get_named_gpio_flags(dev->of_node, "reset-gpios", 0, freescale/fec_main.c 3206: phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0); 3216: dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err); cadence/macb.c 2958: int gpio = of_get_named_gpio(phy_node, "reset-gpios", 0); ... Yep, the gpiolib that can include the prop to work with the same ways. > > [...] > > MBR, Sergei > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip -- Thanks, Caesar