[PATCH] phy: add of_set_phy_supported() helper, call from phy_config()

[Tom Rini]

On Mon, Jan 30, 2023 at 10:36:40PM +0100, Rasmus Villemoes wrote:
> On 30/01/2023 18.16, Tom Rini wrote:
> > On Thu, Jan 26, 2023 at 05:54:51PM -0700, Simon Glass wrote:
> >> Hi Rasmus,
> >>
> >> On Tue, 9 Aug 2022 at 05:53, Rasmus Villemoes
> >> <rasmus.villemoes at prevas.dk> wrote:
> >>>
> >>> Currently, U-Boot doesn't parse a "max-speed" DT property in a phy's
> >>> DT node. That property is a standard binding which should be honoured,
> >>> and in linux that is done by the core phy code via a call to an
> >>> of_set_phy_supported() helper. (Some ethernet drivers support a
> >>> max-speed property in their DT node, but that's orthogonal to what the
> >>> phy supports.)
> >>>
> >>> Add a similar helper in U-Boot, and call it from phy_config().
> >>>
> >>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes at prevas.dk>
> >>> ---
> >>> Resending, this time including the u-boot list in recipients. Sorry
> >>> for the duplicate.
> >>>
> >>>  drivers/net/phy/phy.c | 20 ++++++++++++++++++++
> >>>  1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> >>> index e6e1755518..ec690361e6 100644
> >>> --- a/drivers/net/phy/phy.c
> >>> +++ b/drivers/net/phy/phy.c
> >>> @@ -599,6 +599,20 @@ int phy_register(struct phy_driver *drv)
> >>>         return 0;
> >>>  }
> >>>
> >>> +static int of_set_phy_supported(struct phy_device *phydev)
> >>> +{
> >>> +       ofnode node = phy_get_ofnode(phydev);
> >>> +       u32 max_speed;
> >>> +
> >>> +       if (!ofnode_valid(node))
> >>> +               return 0;
> >>> +
> >>> +       if (!ofnode_read_u32(node, "max-speed", &max_speed))
> >>> +               return phy_set_supported(phydev, max_speed);
> >>> +
> >>> +       return 0;
> >>> +}
> >>> +
> >>>  int phy_set_supported(struct phy_device *phydev, u32 max_speed)
> >>>  {
> >>>         /* The default values for phydev->supported are provided by the PHY
> >>> @@ -1070,6 +1084,12 @@ __weak int board_phy_config(struct phy_device *phydev)
> >>>
> >>>  int phy_config(struct phy_device *phydev)
> >>>  {
> >>> +       int ret;
> >>> +
> >>> +       ret = of_set_phy_supported(phydev);
> >>> +       if (ret)
> >>> +               return ret;
> >>> +
> >>>         /* Invoke an optional board-specific helper */
> >>>         return board_phy_config(phydev);
> >>>  }
> >>>
> >>
> >> The only problem with this is that it is reading DT outside the
> >> of_to_plat() method. Is it possible to call it from there?
> 
> I didn't know that was verboten, and I certainly don't want to add the
> same code over and over to all phy drivers' methods, that was the point
> of doing it in a central place. If there's some other central place
> that's better I'm all ears.

Is this a good enough rationale, Simon?

> > As written, the patch also needs a #ifdef or similar around OF_CONTROL
> 
> Sigh. But I suppose it's just adding 'if
> (!IS_ENABLED(CONFIG_OF_CONTROL)) return 0;' to the top of
> of_set_phy_supported(). But perhaps it will end up somewhere where
> that's "automatic".

Doing the guard in phy_config itself was fine (and I'm coming down
harder on "do $this to avoid #ifdef" when it's not making code read
easier).

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230130/4ff75931/attachment.sig>