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

[Chaoyi Chen <chaoyi.chen@rock-chips.com>]

On 8/26/2025 5:34 PM, Russell King (Oracle) wrote:

> 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?

Hmm, I'm not sure if it will be the same for the devices that come later. I'll try adding those checks.