[PATCH 2/3] arm: kirkwood: Dreamplug: Use Ethernet PHY name and address from device tree

[Stefan Roese]

On 31.07.21 12:27, Stefan Roese wrote:
> Hi Tony,
> 
> (added Joe & Ramon as network custodians)
> 
> On 31.07.21 11:55, Tony Dinh wrote:
>> Hi Stefan,
>>
>> On Sat, Jul 31, 2021 at 12:41 AM Stefan Roese <sr at denx.de> wrote:
>>>
>>> On 26.07.21 08:01, Tony Dinh wrote:
>>>> In DM Ethernet, the old "egiga0" and 'egiga1" names are no longer 
>>>> valid,
>>>> so replace these with Ethernet PHY names from device tree. Also, read
>>>> Ethernet PHY address for each port from device tree.
>>>>
>>>> Signed-off-by: Tony Dinh <mibodhi at gmail.com>
>>>> ---
>>>>
>>>>    board/Marvell/dreamplug/dreamplug.c | 62 
>>>> ++++++++++++++++++++++-------
>>>>    1 file changed, 48 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/board/Marvell/dreamplug/dreamplug.c 
>>>> b/board/Marvell/dreamplug/dreamplug.c
>>>> index e1c64b5224..d5b6b22ddf 100644
>>>> --- a/board/Marvell/dreamplug/dreamplug.c
>>>> +++ b/board/Marvell/dreamplug/dreamplug.c
>>>> @@ -1,5 +1,6 @@
>>>>    // SPDX-License-Identifier: GPL-2.0+
>>>>    /*
>>>> + * Copyright (C) 2021  Tony Dinh <mibodhi at gmail.com>
>>>>     * (C) Copyright 2011
>>>>     * Jason Cooper <u-boot at lakedaemon.net>
>>>>     *
>>>> @@ -97,42 +98,75 @@ int board_init(void)
>>>>        return 0;
>>>>    }
>>>>
>>>> +static int fdt_get_phy_addr(const char *path)
>>>> +{
>>>> +     const void *fdt = gd->fdt_blob;
>>>> +     const u32 *reg;
>>>> +     const u32 *val;
>>>> +     int node, phandle, addr;
>>>> +
>>>> +     /* Find the node by its full path */
>>>> +     node = fdt_path_offset(fdt, path);
>>>> +     if (node >= 0) {
>>>> +             /* Look up phy-handle */
>>>> +             val = fdt_getprop(fdt, node, "phy-handle", NULL);
>>>> +             if (val) {
>>>> +                     phandle = fdt32_to_cpu(*val);
>>>> +                     if (!phandle)
>>>> +                             return -1;
>>>> +                     /* Follow it to its node */
>>>> +                     node = fdt_node_offset_by_phandle(fdt, phandle);
>>>> +                     if (node) {
>>>> +                             /* Look up reg */
>>>> +                             reg = fdt_getprop(fdt, node, "reg", 
>>>> NULL);
>>>> +                             if (reg) {
>>>> +                                     addr = fdt32_to_cpu(*reg);
>>>> +                                     return addr;
>>>> +                             }
>>>> +                     }
>>>> +             }
>>>> +     }
>>>> +     return -1;
>>>> +}
>>>
>>> You've added this exact some function now for the 3rd time IIRC. Could
>>> please please consolidate this function into one of the fdt / dt common
>>> functions instead? Perhaps there is already something similar for
>>> reading PHY addresses?
>>>
>>> Please do this in a follow-up patch, after I've pulled this one.
>>
>> Yes, I realized this function needs consolidation too, and mentioned
>> it in the patch for the Sheevaplug. Currently, it is hard to find a
>> home for this. ./common/fdt_support.c is one level of abstraction
>> below this fdt_get_phy_addr(). So it is not appropriate for this
>> function to be in that file.
> 
> Joe, Ramon, do you know of any DT / FDT helper function already
> available to read the PHY address from the device-tree?

It just now came to my mind that the "normal" functions for base address
reading like dev_read_addr(), fdt_get_base_address() and friends can
most likely be used for PHY MDIO address reading as well.

Thanks,
Stefan

> Or if not,
> where would you like to have this new function being placed in the
> U-Boot source tree?
> 
>> I've looked at various Armada 37x/38x SoC DTS, the binding is a bit
>> different, i.e. the "phy-handle" is not defined as consistently as in
>> Kirkwood DTSs, so this function would not work for Marvell SoCs, in
>> general.
> 
> Not sure here (sorry for not looking deeper into the docs), but what is
> the difference between the "phy-handle" and "phy" property here in the
> ethernet DT nodes? Do we perhaps need to handle both properties?
> 
>> How about if we put this function in a new area in
>> arch/arm/mach-kirkwood/? such as arch/arm/mach-kirkwood/fdt/ ?
> 
> I'm not a fan of using a kirkwood specific place here. This sounds like
> a common (platform independent) issue and should be able to be used
> by all boards.
> 
> Thanks,
> Stefan
> 
>> I'll send in a separate patch for this, which will apply to the
>> Kirkwood boards which have already been converted to DM Ethernet.
>>
>> Thanks,
>> Tony
>>
>>> Other that this:
>>>
>>> Reviewed-by: Stefan Roese <sr at denx.de>
>>>
>>> Thanks,
>>> Stefan
>>>
>>>
>>>> +
>>>>    #ifdef CONFIG_RESET_PHY_R
>>>> -void mv_phy_88e1116_init(char *name)
>>>> +void mv_phy_88e1116_init(const char *name, const char *path)
>>>>    {
>>>>        u16 reg;
>>>> -     u16 devadr;
>>>> +     int phyaddr;
>>>>
>>>>        if (miiphy_set_current_dev(name))
>>>>                return;
>>>>
>>>> -     /* command to read PHY dev address */
>>>> -     if (miiphy_read(name, 0xEE, 0xEE, (u16 *) &devadr)) {
>>>> -             printf("Err..%s could not read PHY dev address\n",
>>>> -                     __func__);
>>>> +     phyaddr = fdt_get_phy_addr(path);
>>>> +     if (phyaddr < 0)
>>>>                return;
>>>> -     }
>>>>
>>>>        /*
>>>>         * Enable RGMII delay on Tx and Rx for CPU port
>>>>         * Ref: sec 4.7.2 of chip datasheet
>>>>         */
>>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 2);
>>>> -     miiphy_read(name, devadr, MV88E1116_MAC_CTRL2_REG, &reg);
>>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 2);
>>>> +     miiphy_read(name, phyaddr, MV88E1116_MAC_CTRL2_REG, &reg);
>>>>        reg |= (MV88E1116_RGMII_RXTM_CTRL | MV88E1116_RGMII_TXTM_CTRL);
>>>> -     miiphy_write(name, devadr, MV88E1116_MAC_CTRL2_REG, reg);
>>>> -     miiphy_write(name, devadr, MV88E1116_PGADR_REG, 0);
>>>> +     miiphy_write(name, phyaddr, MV88E1116_MAC_CTRL2_REG, reg);
>>>> +     miiphy_write(name, phyaddr, MV88E1116_PGADR_REG, 0);
>>>>
>>>>        /* reset the phy */
>>>> -     miiphy_reset(name, devadr);
>>>> +     miiphy_reset(name, phyaddr);
>>>>
>>>>        printf("88E1116 Initialized on %s\n", name);
>>>>    }
>>>>
>>>>    void reset_phy(void)
>>>>    {
>>>> +     char *eth0_name = "ethernet-controller at 72000";
>>>> +     char *eth0_path = 
>>>> "/ocp at f1000000/ethernet-controller at 72000/ethernet0-port at 0";
>>>> +     char *eth1_name = "ethernet-controller at 76000";
>>>> +     char *eth1_path = 
>>>> "/ocp at f1000000/ethernet-controller at 72000/ethernet1-port at 0";
>>>> +
>>>>        /* configure and initialize both PHY's */
>>>> -     mv_phy_88e1116_init("egiga0");
>>>> -     mv_phy_88e1116_init("egiga1");
>>>> +     mv_phy_88e1116_init(eth0_name, eth0_path);
>>>> +     mv_phy_88e1116_init(eth1_name, eth1_path);
>>>>    }
>>>>    #endif /* CONFIG_RESET_PHY_R */
>>>>
>>>
>>>
>>> Viele Grüße,
>>> Stefan
>>>
>>> -- 
>>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
> 
> 
> Viele Grüße,
> Stefan
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de