From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S943291AbdDTH64 (ORCPT ); Thu, 20 Apr 2017 03:58:56 -0400 Received: from lelnx194.ext.ti.com ([198.47.27.80]:38701 "EHLO lelnx194.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S937259AbdDTH6y (ORCPT ); Thu, 20 Apr 2017 03:58:54 -0400 Subject: Re: [PATCH] mdio_bus: Issue GPIO RESET to PHYs. To: Andrew Lunn References: <1491381237-24635-1-git-send-email-rogerq@ti.com> <64d6494d-41d2-0faf-a434-057f796637fe@ti.com> <20170419113951.GA31908@lunn.ch> <20170419133853.GA2696@lunn.ch> CC: , Florian Fainelli , , , , , , From: Roger Quadros Message-ID: <47d66f57-d477-4cde-9ac5-26d5397861a6@ti.com> Date: Thu, 20 Apr 2017 10:58:36 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170419133853.GA2696@lunn.ch> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19/04/17 16:38, Andrew Lunn wrote: > On Wed, Apr 19, 2017 at 02:56:48PM +0300, Roger Quadros wrote: >> Hi, >> >> On 19/04/17 14:39, Andrew Lunn wrote: >>> On Wed, Apr 19, 2017 at 12:24:26PM +0300, Roger Quadros wrote: >>>> Some boards [1] leave the PHYs at an invalid state >>>> during system power-up or reset thus causing unreliability >>>> issues with the PHY which manifests as PHY not being detected >>>> or link not functional. To fix this, these PHYs need to be RESET >>>> via a GPIO connected to the PHY's RESET pin. >>>> >>>> Some boards have a single GPIO controlling the PHY RESET pin of all >>>> PHYs on the bus whereas some others have separate GPIOs controlling >>>> individual PHY RESETs. >>>> >>>> In both cases, the RESET de-assertion cannot be done in the PHY driver >>>> as the PHY will not probe till its reset is de-asserted. >>>> So do the RESET de-assertion in the MDIO bus driver. >>>> >>>> [1] - am572x-idk, am571x-idk, a437x-idk >>>> >>>> Signed-off-by: Roger Quadros >>>> --- >>>> drivers/net/phy/mdio_bus.c | 26 ++++++++++++++++++++++++++ >>>> drivers/of/of_mdio.c | 4 ++++ >>>> include/linux/phy.h | 5 +++++ >>>> 3 files changed, 35 insertions(+) >>> >>> Hi Roger >>> >>> Thanks for making this generic. >>> >>> Please add device tree binding documentation. I think that actually >>> means you have to document MDIO in general, since there currently is >>> not a binding document. >> >> OK. >> >>> >>>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c >>>> index fa7d51f..25fda2b 100644 >>>> --- a/drivers/net/phy/mdio_bus.c >>>> +++ b/drivers/net/phy/mdio_bus.c >>>> @@ -22,8 +22,11 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> +#include >>>> #include >>>> #include >>>> +#include >>>> #include >>>> #include >>>> #include >>>> @@ -43,6 +46,8 @@ >>>> >>>> #include "mdio-boardinfo.h" >>>> >>>> +#define DEFAULT_GPIO_RESET_DELAY 10 /* in microseconds */ >>>> + >>>> int mdiobus_register_device(struct mdio_device *mdiodev) >>>> { >>>> if (mdiodev->bus->mdio_map[mdiodev->addr]) >>>> @@ -307,6 +312,7 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) >>>> { >>>> struct mdio_device *mdiodev; >>>> int i, err; >>>> + struct gpio_desc *gpiod; >>>> >>>> if (NULL == bus || NULL == bus->name || >>>> NULL == bus->read || NULL == bus->write) >>>> @@ -333,6 +339,26 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) >>>> if (bus->reset) >>>> bus->reset(bus); >>>> >>>> + /* de-assert bus level PHY GPIO resets */ >>>> + for (i = 0; i < bus->num_reset_gpios; i++) { >>>> + gpiod = devm_gpiod_get_index(&bus->dev, "reset", i, >>>> + GPIOD_OUT_LOW); >>>> + if (IS_ERR(gpiod)) { >>>> + err = PTR_ERR(gpiod); >>>> + if (err != -ENOENT) { >>>> + pr_err("mii_bus %s couldn't get reset GPIO\n", >>>> + bus->id); >>>> + return err; >>>> + } >>>> + } else { >>>> + gpiod_set_value_cansleep(gpiod, 1); >>> >>> >>>> + if (!bus->reset_delay_us) >>>> + bus->reset_delay_us = DEFAULT_GPIO_RESET_DELAY; >>> >>> Maybe do this once, where you read the device tree property. >> >> I was thinking from point of view that GPIO RESET code should work even without >> device tree. Although I'm not sure if there would be any users or not. > > Hi Roger > > I don't see how this would work. What would devm_gpiod_get_index() > return? Something from ACPI? But then there would be something Yes or just lookup tables based on platform data. > equivalent for getting the delay. I'm not sure about delay part. > > Lets keep it simple and OF only. If there is a real need for something > in addition to OF, it can be added later. OK. I'll revise the patch based on your comments. cheers, -roger