Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230713202123.231445-1-alex@shruggie.ro> <20230713202123.231445-2-alex@shruggie.ro>
 <20230714172444.GA4003281-robh@kernel.org> <CAH3L5Qoj+sue=QnR2Lp12x3Hz2t2BNnarZHJiqxL3Gtf6M=bsA@mail.gmail.com>
 <7fa2d457-4ae9-42f5-be73-80549aae558c@lunn.ch>
In-Reply-To: <7fa2d457-4ae9-42f5-be73-80549aae558c@lunn.ch>
From:   Alexandru Ardelean <alex@shruggie.ro>
Date:   Sat, 5 Aug 2023 23:25:31 +0300
Message-ID: <CAH3L5Qpd+6740SeQJh+1J8MjC1BjHE=EEReK9AOuJW_Ey3V4mA@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] dt-bindings: net: phy: vsc8531: document
 'vsc8531,clkout-freq-mhz' property
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Rob Herring <robh@kernel.org>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, hkallweit1@gmail.com, linux@armlinux.org.uk,
        olteanv@gmail.com, marius.muresan@mxt.ro
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Jul 16, 2023 at 6:07=E2=80=AFPM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > So, there's the adin.c PHY driver which has a similar functionality
> > with the adin_config_clk_out().
> > Something in the micrel.c PHY driver (with
> > micrel,rmii-reference-clock-select-25-mhz); hopefully I did not
> > misread the code about that one.
> > And the at803x.c PHY driver has a 'qca,clk-out-frequency' property too.
> >
> > Now with the mscc.c driver, there is a common-ality that could use a fr=
amework.
> >
> > @Rob are you suggesting something like registering a clock provider
> > (somewhere in the PHY framework) and let the PHY drivers use it?
> > Usually, these clock signals (once enabled on startup), don't get
> > turned off; but I've worked mostly on reference designs; somewhere
> > down the line some people get different requirements.
> > These clocks get connected back to the MAC (usually), and are usually
> > like a "fixed-clock" driver.
>
> They are not necessarily fixed clocks. The clock you are adding here
> has three frequencies. Two frequencies is common for PHY devices. So
> you need to use something more than clk-fixed-rate.c. Also, mostly
> PHYs allows the clock to be gated.
>
> > In our case, turning off the clock would be needed if the PHY
> > negotiates a non-gigabit link; i.e 100 or 10 Mbps; in that case, the
> > CLKOUT signal is not needed and it can be turned off.
>
> Who does not need it? The PHY, or the MAC? If it is the MAC, it should
> really be the MAC driver which uses the common clock API to turn it
> off. Just watch out for deadlocks with phydev->lock.

The MAC needs the clock in GMII mode, when going in gigabit mode.

>
> > Maybe start out with a hook in 'struct phy_driver'?
> > Like "int (*config_clk_out)(struct phy_device *dev);" or something?
> > And underneath, this delegates to the CLK framework?
>
> Yes, have phy_device.c implement that registration/unregister of the
> clock, deal with locking, and call into the PHY driver to actually
> manipulate the clock. You missed the requested frequency in the
> function prototype. I would also call it refclk. Three is sometimes
> confusion about the different clocks.

Ack.
Then something like:
int (*config_refclk)(struct phy_device *dev, uint32_t frequency);

>
> Traditionally, clk_enable() can be called in atomic context, but that
> is not allowed with phylib, it always assume thread context. I don't
> know if the clock framework has some helpers for that, but i also
> don't see there being a real need for MAC to enable the clock in
> atomic context.
>
>         Andrew