linux-kernel - Re: [PATCH 3/6] net: arc_emac: support the phy reset for emac driver

Open Source and information security mailing list archives

Message-ID: <56E4E53F.2060307@gmail.com>
Date:	Sun, 13 Mar 2016 11:57:51 +0800
From:	Caesar Wang <caesar.upstream@...il.com>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:	Caesar Wang <wxt@...k-chips.com>, Heiko Stuebner <heiko@...ech.de>,
	"David S. Miller" <davem@...emloft.net>,
	Rob Herring <robh+dt@...nel.org>,
	Alexander Kochetkov <al.kochet@...il.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-rockchip@...ts.infradead.org, keescook@...gle.com,
	leozwang@...gle.com
Subject: Re: [PATCH 3/6] net: arc_emac: support the phy reset for emac driver

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 <wxt@...k-chips.com>
>> ---
>>
>>   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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

-- 
Thanks,
Caesar

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