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

[Tony Dinh]

Hi Stefan,

On Sun, Aug 1, 2021 at 11:39 PM Stefan Roese <sr at denx.de> wrote:
>
> Hi Tony,
>
> On 01.08.21 00:37, Tony Dinh wrote:
> > Hi Stefan,
> >
> > On Sat, Jul 31, 2021 at 4:50 AM Stefan Roese <sr at denx.de> wrote:
> >>
> >> 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 for the suggestion. I agree that would work, too. I did
> > consider going down the MDIO path and looking up "reg" a bit quicker
> > because we know what the PHY Base address is. But in the scenario that
> > we have 2 or more active Ethernet ports (eg. Dreamplug board has 2),
> > the lookup function is not as elegant.  It seems more straightforward
> > if we go down the Eth0 or Eth1 path from the DTSI and then follow the
> > "phy-handle" to the MDIO ethernet-phy node.
>
> ... and also "phy" to make this compatible with this deprecated property
> as well?
>
> But you could still use the already available functions to read the
> "reg" property instead of coding it once again in this new function?
>
> > Regarding your previous question about  "phy" vs. "phy-handle",
> > according to the current device tree guideline, "phy" is deprecated
> > and "phy-handle" is the one we should use. And there are some Armada
> > 37x/38x boards that use the deprecated name "phy" currently.
>
> Thanks. Please see my comment above.

Thanks for your review! I'll send in a patch to propose a new home (in
/common) for this function, and also update it to look up for the
deprecated "phy" also. Once we have that done, I'll go back and submit
patches for the Kirkwood boards that have this function hardcoded in.

Thanks,
Tony

>
> Thanks,
> Stefan