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

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.


> 			...
> 		else if (!IS_ERR)
> 			bsp_priv->clk_phy = clk;
>
> I don't have a free terminal to work out what "..." should be.
>