| Message ID | 20220809115330.463849-1-rasmus.villemoes@prevas.dk |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | phy: add of_set_phy_supported() helper, call from phy_config() | expand |
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>
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
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.
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
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.
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
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).
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); }
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(+)