From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E672AC433E2 for ; Thu, 3 Sep 2020 17:17:34 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id A62E2208B3 for ; Thu, 3 Sep 2020 17:17:34 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="pHTi/O9n" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728529AbgICRRd (ORCPT ); Thu, 3 Sep 2020 13:17:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33880 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728353AbgICRRc (ORCPT ); Thu, 3 Sep 2020 13:17:32 -0400 Received: from mail-pf1-x443.google.com (mail-pf1-x443.google.com [IPv6:2607:f8b0:4864:20::443]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2183CC061244; Thu, 3 Sep 2020 10:17:31 -0700 (PDT) Received: by mail-pf1-x443.google.com with SMTP id o68so2860264pfg.2; Thu, 03 Sep 2020 10:17:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=oW0uoQUvpldpc+Iv2HAzfNV0xV+yANzTTg8yRNQopps=; b=pHTi/O9n82na5FygUhxsUcvFnvNzO/CJW2SdlTSUoXcn/h1hgW5KB08jE78wv2uS/I /d1gZs63oxW1NBOosCfMgslgQrZ0WBEy05VpSgO/dvJxNL1kZK3U0uUvWTr+ykkLL/ij YLU+pvPEQ22zrBg+zQ619Tt8T8uwSbvbPb+kNWs2ckr5uMIyWa5nMpllCgBkZhBMKNFe 0CnIqTJX7y4+m1J+IiQXuL6gGTloLaCmTspUVD50y0LjtD8+/7zEKAtjzAjk3wWMT7fU Jc5yInpthi6QnV+j2RaNrTsDW0jlMAw/ebOLpMkSg8xX5+v6c29uVqUebvUC9ULeBgiw eYqg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=oW0uoQUvpldpc+Iv2HAzfNV0xV+yANzTTg8yRNQopps=; b=IC3kQ4om+iVSoskWcvFExOFnBKDYgnx4CR4alGJ3Gq+TsF17ijtWvYBAysqSb/Kokw 56J8jV59YvKm8u6O4FOnJpg+oGDZnt4jApLxbA8TOJxbMCuHEeV5Nlb8C6yIKxPoVWWs 1la9oGWSFfxTO8lbzly57OHxpsMWFpANL1nmAr7YDIKN/65iuZQvoF0dLl7yoZ3j0tQt Zuh/nOUjptEZdx+w1GI/6W8Mn5E0/y8NSBOu9X8drcO61TWZSDkqAOuD/SYmPnmveYce fwjQMsrCLcsMIKEBmtd+asTEXEnXXUadwOGM5jTgrqlE36ARDvRXVSvPs66ihz76QOv/ KwOQ== X-Gm-Message-State: AOAM530PQryK3cxCiG21YIV+18B0fAFbG2dgWK8rEUERKPoNdKby4f0K B3LqeHkfHHAgZgCfftnTd8poWK4PAa0= X-Google-Smtp-Source: ABdhPJxYu9hsAAVvAegM6osB7UtnSvl/g8lQXR6SMAL4Jyf6IXelLPHB+4pNL4QnAn8ihWzE/PHfdA== X-Received: by 2002:a65:6093:: with SMTP id t19mr3731079pgu.13.1599153450407; Thu, 03 Sep 2020 10:17:30 -0700 (PDT) Received: from [10.230.30.107] ([192.19.223.252]) by smtp.gmail.com with ESMTPSA id a24sm3066303pju.25.2020.09.03.10.17.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 03 Sep 2020 10:17:29 -0700 (PDT) Subject: Re: [RFC net-next 2/2] net: phy: bcm7xxx: request and manage GPHY clock To: =?UTF-8?Q?Adam_Rudzi=c5=84ski?= , Andrew Lunn Cc: netdev@vger.kernel.org, m.felsch@pengutronix.de, hkallweit1@gmail.com, richard.leitner@skidata.com, zhengdejin5@gmail.com, devicetree@vger.kernel.org, kernel@pengutronix.de, kuba@kernel.org, robh+dt@kernel.org References: <20200902213347.3177881-1-f.fainelli@gmail.com> <20200902213347.3177881-3-f.fainelli@gmail.com> <20200902222030.GJ3050651@lunn.ch> <7696bf30-9d7b-ecc9-041d-7d899dd07915@gmail.com> <77088212-ac93-9454-d3a0-c2eb61b5c3e0@arf.net.pl> <26a8a508-6108-035a-1416-01cff51a930a@gmail.com> From: Florian Fainelli Message-ID: <7b38d9fe-7c01-a658-fddd-c32e5a0b6f0d@gmail.com> Date: Thu, 3 Sep 2020 10:17:27 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Firefox/78.0 Thunderbird/78.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org On 9/3/2020 10:13 AM, Adam Rudziński wrote: > > W dniu 2020-09-03 o 17:21, Florian Fainelli pisze: >> >> >> On 9/2/2020 11:00 PM, Adam Rudziński wrote: >>> >>> W dniu 2020-09-03 o 04:13, Florian Fainelli pisze: >>>> >>>> >>>> On 9/2/2020 3:20 PM, Andrew Lunn wrote: >>>>>> +    priv->clk = devm_clk_get_optional(&phydev->mdio.dev, "sw_gphy"); >>>>>> +    if (IS_ERR(priv->clk)) >>>>>> +        return PTR_ERR(priv->clk); >>>>>> + >>>>>> +    /* To get there, the mdiobus registration logic already >>>>>> enabled our >>>>>> +     * clock otherwise we would not have probed this device since >>>>>> we would >>>>>> +     * not be able to read its ID. To avoid artificially bumping >>>>>> up the >>>>>> +     * clock reference count, only do the clock enable from a >>>>>> phy_remove -> >>>>>> +     * phy_probe path (driver unbind, then rebind). >>>>>> +     */ >>>>>> +    if (!__clk_is_enabled(priv->clk)) >>>>>> +        ret = clk_prepare_enable(priv->clk); >>>>> >>>>> This i don't get. The clock subsystem does reference counting. So what >>>>> i would expect to happen is that during scanning of the bus, phylib >>>>> enables the clock and keeps it enabled until after probe. To keep >>>>> things balanced, phylib would disable the clock after probe. >>>> >>>> That would be fine, although it assumes that the individual PHY >>>> drivers have obtained the clocks and called clk_prepare_enable(), >>>> which is a fair assumption I suppose. >>>> >>>>> >>>>> If the driver wants the clock enabled all the time, it can enable it >>>>> in the probe method. The common clock framework will then have two >>>>> reference counts for the clock, so that when the probe exists, and >>>>> phylib disables the clock, the CCF keeps the clock ticking. The PHY >>>>> driver can then disable the clock in .remove. >>>> >>>> But then the lowest count you will have is 1, which will lead to the >>>> clock being left on despite having unbound the PHY driver from the >>>> device (->remove was called). This does not allow saving any power >>>> unfortunately. >>>> >>>>> >>>>> There are some PHYs which will enumerate with the clock disabled. They >>>>> only need it ticking for packet transfer. Such PHY drivers can enable >>>>> the clock only when needed in order to save some power when the >>>>> interface is administratively down. >>>> >>>> Then the best approach would be for the OF scanning code to enable >>>> all clocks reference by the Ethernet PHY node (like it does in the >>>> proposed patch), since there is no knowledge of which clock is >>>> necessary and all must be assumed to be critical for MDIO bus >>>> scanning. Right before drv->probe() we drop all resources reference >>>> counts, and from there on ->probe() is assumed to manage the >>>> necessary clocks. >>>> >>>> It looks like another solution may be to use the assigned-clocks >>>> property which will take care of assigning clock references to >>>> devices and having those applied as soon as the clock provider is >>>> available. >>> >>> Hi Guys, >>> >>> I've just realized that a PHY may also have a reset signal connected. >>> The reset signal may be controlled by the dedicated peripheral or by >>> GPIO. >> >> There is already support for such a thing within >> drivers/net/phy/mdio_bus.c though it assumes we could bind the PHY >> device to its driver already. >> >>> >>> In general terms, there might be a set of control signals needed to >>> enable the PHY. It seems that the clock and the reset would be the >>> typical useful options. >>> >>> Going further with my imagination of how evil the hardware design >>> could be, in general the signals for the PHY may have some relations >>> to other control signals. >>> >>> I think that from the software point of view this comes down to >>> assumption that the PHY is to be controlled "driver only knows how". >> >> That is all well and good as long as we can actually bind the PHY >> device which its driver, and right now this means that we either have: >> >> - a compatible string in Device Tree which is of the form >> ethernet-phy-id%4x.%4x (see of_get_phy_id) which means that we *know* >> already which PHY we have and we avoid doing reads of MII_PHYSID1 and >> MII_PHYSID2. This is a Linux implementation detail that should not >> have to be known to systems designer IMHO >> >> - a successful read of MII_PHYSID1 and MII_PHYSID2 (or an equivalent >> for the clause 45 PHYs) that allows us to know what PHY device we >> have, which is something that needs to happen eventually. >> >> The problem is when there are essential resources such as clocks, >> regulators, reset signals that must be enabled, respectively >> de-asserted in order for a successful MDIO read of MII_PHYSID1 and >> MII_PHYSID2 to succeed. >> >> There is no driver involvement at that stage because we have no >> phy_device to bind it to *yet*. Depending on what we read from >> MII_PHYSID1/PHY_ID2 we will either successfully bind to the Generic >> PHY driver (assuming we did not read all Fs) or not and we will return >> -ENODEV and then it is game over. >> >> This is the chicken and egg problem that this patch series is >> addressing, for clocks, because we can retrieve clock devices with >> just a device_node reference. > > I have an impression that here the effort goes in the wrong direction. > If I understand correctly, the goal is to have the kernel find out what > will the driver need to use the phy. But, the kernel anyway calls a > probe function of the driver, doesn't it? To me it looks as if you were > trying to do something that the driver will/would/might do later, and > possibly "undo" it in the meantime. In this regard, this becomes kind of > a workaround, not solution of the problem. Also, having taken a glance > at your previous messages, I can tell that this is all becoming even > more complex. What is the problem according to you, and what would an acceptable solution look like then? > > I think that the effort should be to allow any node in the device tree > to take care about its child nodes by itself, and just "report" to the > kernel, or "install" in the kernel whatever is necessary, but without > any initiative of the kernel. Let the drivers be as complicated as > necessary, not the kernel. The Device Tree representation is already correct in that it lists clocks/regulators/reset signals that are needed by the PHY. The problem is its implementation with the Linux device driver model. Please read again what I wrote. -- Florian