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

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

[Rasmus Villemoes]

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@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);
 }
-- 
2.31.1

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

[Ramon Fried]

On Tue, Aug 9, 2022 at 2:53 PM Rasmus Villemoes
<rasmus.villemoes@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@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);
>  }
> --
> 2.31.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

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

[Rasmus Villemoes]

On 18/09/2022 08.13, Ramon Fried wrote:
> On Tue, Aug 9, 2022 at 2:53 PM Rasmus Villemoes
> <rasmus.villemoes@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@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);
>>  }
>> --
>> 2.31.1
>>
> Reviewed-by: Ramon Fried <rfried.dev@gmail.com>

This seems to have fallen through the cracks. Can it be picked up please?

Rasmus

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]

On Thu, Jan 26, 2023 at 09:29:29AM +0100, Rasmus Villemoes wrote:
> On 18/09/2022 08.13, Ramon Fried wrote:
> > On Tue, Aug 9, 2022 at 2:53 PM Rasmus Villemoes
> > <rasmus.villemoes@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@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);
> >>  }
> >> --
> >> 2.31.1
> >>
> > Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
> 
> This seems to have fallen through the cracks. Can it be picked up please?

Thanks for spotting, yes, I'll pick this up for real soon.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Simon Glass]

Hi Rasmus,

On Tue, 9 Aug 2022 at 05:53, Rasmus Villemoes
<rasmus.villemoes@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@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);
>  }
> --
> 2.31.1
>

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?

Regards,
Simon

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2568 bytes --]

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@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@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);
> >  }
> > --
> > 2.31.1
> >
> 
> 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?

As written, the patch also needs a #ifdef or similar around OF_CONTROL
or this fails to build on a platform or two. I was about to apply with
that change made when I saw Simon's question, to which I'm waiting for
an answer / follow-up on.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Rasmus Villemoes]

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@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@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.

> 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".

Rasmus

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

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3274 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]