Re: [PATCH net-next v3] net: ethernet: stmmac: dwmac-rk: Make the clk_phy could be used for external phy

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Aug 26, 2025 at 05:29:04PM +0800, Chaoyi Chen wrote:
> On 8/26/2025 5:25 PM, Russell King (Oracle) wrote:
> 
> > On Tue, Aug 26, 2025 at 04:08:40PM +0800, Chaoyi Chen wrote:
> > > Hi Russell,
> > > 
> > > On 8/25/2025 9:37 PM, Russell King (Oracle) wrote:
> > > > On Mon, Aug 25, 2025 at 12:53:37PM +0200, Marek Szyprowski wrote:
> > > > > On 25.08.2025 11:57, Chaoyi Chen wrote:
> > > > > > On 8/25/2025 3:23 PM, Marek Szyprowski wrote:
> > > > > > > On 15.08.2025 04:35, Chaoyi Chen wrote:
> > > > > > > > From: Chaoyi Chen <chaoyi.chen@rock-chips.com>
> > > > > > > > 
> > > > > > > > For external phy, clk_phy should be optional, and some external phy
> > > > > > > > need the clock input from clk_phy. This patch adds support for setting
> > > > > > > > clk_phy for external phy.
> > > > > > > > 
> > > > > > > > Signed-off-by: David Wu <david.wu@rock-chips.com>
> > > > > > > > Signed-off-by: Chaoyi Chen <chaoyi.chen@rock-chips.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Changes in v3:
> > > > > > > > - Link to V2:
> > > > > > > > https://lore.kernel.org/netdev/20250812012127.197-1-kernel@airkyi.com/
> > > > > > > > - Rebase to net-next/main
> > > > > > > > 
> > > > > > > > Changes in v2:
> > > > > > > > - Link to V1:
> > > > > > > > https://lore.kernel.org/netdev/20250806011405.115-1-kernel@airkyi.com/
> > > > > > > > - Remove get clock frequency from DT prop
> > > > > > > > 
> > > > > > > >      drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 11 +++++++----
> > > > > > > >      1 file changed, 7 insertions(+), 4 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > > > > b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > > > > index ac8288301994..5d921e62c2f5 100644
> > > > > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > > > > @@ -1412,12 +1412,15 @@ static int rk_gmac_clk_init(struct
> > > > > > > > plat_stmmacenet_data *plat)
> > > > > > > >              clk_set_rate(plat->stmmac_clk, 50000000);
> > > > > > > >          }
> > > > > > > >      -    if (plat->phy_node && bsp_priv->integrated_phy) {
> > > > > > > > +    if (plat->phy_node) {
> > > > > > > >              bsp_priv->clk_phy = of_clk_get(plat->phy_node, 0);
> > > > > > > >              ret = PTR_ERR_OR_ZERO(bsp_priv->clk_phy);
> > > > > > > > -        if (ret)
> > > > > > > > -            return dev_err_probe(dev, ret, "Cannot get PHY clock\n");
> > > > > > > > -        clk_set_rate(bsp_priv->clk_phy, 50000000);
> > > > > > > > +        /* If it is not integrated_phy, clk_phy is optional */
> > > > > > > > +        if (bsp_priv->integrated_phy) {
> > > > > > > > +            if (ret)
> > > > > > > > +                return dev_err_probe(dev, ret, "Cannot get PHY
> > > > > > > > clock\n");
> > > > > > > > +            clk_set_rate(bsp_priv->clk_phy, 50000000);
> > > > > > > > +        }
> > > > > > I think  we should set bsp_priv->clk_phy to NULL here if we failed to
> > > > > > get the clock.
> > > > > > 
> > > > > > Could you try this on your board? Thank you.
> > > > > Right, the following change also fixes this issue:
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > index 9fc41207cc45..2d19d48be01f 100644
> > > > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > > > > @@ -1415,6 +1415,8 @@ static int rk_gmac_clk_init(struct
> > > > > plat_stmmacenet_data *plat)
> > > > >            if (plat->phy_node) {
> > > > >                    bsp_priv->clk_phy = of_clk_get(plat->phy_node, 0);
> > > > >                    ret = PTR_ERR_OR_ZERO(bsp_priv->clk_phy);
> > > > > +               if (ret)
> > > > > +                       bsp_priv->clk_phy = NULL;
> > > > Or just:
> > > > 
> > > > 		clk = of_clk_get(plat->phy_node, 0);
> > > > 		if (clk == ERR_PTR(-EPROBE_DEFER))
> > > Do we actually need this? Maybe other devm_clk_get() before it would fail in advance.
> > Is it the same clock as devm_clk_get()? If it is, what's the point of
> > getting it a second time. If it isn't, then it could be a different
> > clock which may be yet to probe.
> 
> It's not the same clock, but it should be use the same clock controller driver, which is the CRU on the Rockchip platform.

Will it always be the same clock controller, including into the future
Rockchip devices?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!