* [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding
@ 2018-08-08 8:03 Tony Lindgren
2018-08-08 11:58 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2018-08-08 8:03 UTC (permalink / raw)
To: David Miller
Cc: netdev, linux-omap, devicetree, Grygorii Strashko,
Ivan Khoronzhuk, Mark Rutland, Murali Karicheri, Rob Herring
The cpsw-phy-sel device is not a child of the cpsw interconnect target
module. It lives in the system control module.
Let's fix this issue by trying to use cpsw-phy-sel phandle first if it
exists and if not fall back to current usage of trying to find the
cpsw-phy-sel child. That way the phy sel driver can be a child of the
system control module where it belongs in the device tree.
Without this fix, we cannot have a proper interconnect target module
hierarchy in device tree for things like genpd.
For documenting it, looks like the cpsw-phy-sel is used for all cpsw device
tree nodes. It's missing the related binding documentation, so let's also
update the binding documentation accordingly.
Cc: devicetree@vger.kernel.org
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Murali Karicheri <m-karicheri2@ti.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
Documentation/devicetree/bindings/net/cpsw.txt | 5 +++++
drivers/net/ethernet/ti/cpsw-phy-sel.c | 9 ++++++---
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/cpsw.txt b/Documentation/devicetree/bindings/net/cpsw.txt
--- a/Documentation/devicetree/bindings/net/cpsw.txt
+++ b/Documentation/devicetree/bindings/net/cpsw.txt
@@ -19,6 +19,9 @@ Required properties:
- slaves : Specifies number for slaves
- active_slave : Specifies the slave to use for time stamping,
ethtool and SIOCGMIIPHY
+- cpsw-phy-sel : Specifies the phandle to the CPSW phy mode selection
+ device. Note that in legacy cases cpsw-phy-sel may be
+ a child device instead of a phandle.
Optional properties:
- ti,hwmods : Must be "cpgmac0"
@@ -75,6 +78,7 @@ Examples:
cpts_clock_mult = <0x80000000>;
cpts_clock_shift = <29>;
syscon = <&cm>;
+ cpsw-phy-sel = <&phy_sel>;
cpsw_emac0: slave@0 {
phy_id = <&davinci_mdio>, <0>;
phy-mode = "rgmii-txid";
@@ -103,6 +107,7 @@ Examples:
cpts_clock_mult = <0x80000000>;
cpts_clock_shift = <29>;
syscon = <&cm>;
+ cpsw-phy-sel = <&phy_sel>;
cpsw_emac0: slave@0 {
phy_id = <&davinci_mdio>, <0>;
phy-mode = "rgmii-txid";
diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c b/drivers/net/ethernet/ti/cpsw-phy-sel.c
--- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
+++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
@@ -170,10 +170,13 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
struct device_node *node;
struct cpsw_phy_sel_priv *priv;
- node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
+ node = of_parse_phandle(dev->of_node, "cpsw-phy-sel", 0);
if (!node) {
- dev_err(dev, "Phy mode driver DT not found\n");
- return;
+ node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
+ if (!node) {
+ dev_err(dev, "Phy mode driver DT not found\n");
+ return;
+ }
}
dev = bus_find_device(&platform_bus_type, NULL, node, match);
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding
2018-08-08 8:03 [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding Tony Lindgren
@ 2018-08-08 11:58 ` Andrew Lunn
2018-08-08 13:48 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2018-08-08 11:58 UTC (permalink / raw)
To: Tony Lindgren
Cc: David Miller, netdev, linux-omap, devicetree, Grygorii Strashko,
Ivan Khoronzhuk, Mark Rutland, Murali Karicheri, Rob Herring
> +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> @@ -19,6 +19,9 @@ Required properties:
> - slaves : Specifies number for slaves
> - active_slave : Specifies the slave to use for time stamping,
> ethtool and SIOCGMIIPHY
> +- cpsw-phy-sel : Specifies the phandle to the CPSW phy mode selection
> + device. Note that in legacy cases cpsw-phy-sel may be
> + a child device instead of a phandle.
Hi Tony
It would be good to reference cpsw-phy-sel.txt.
> Optional properties:
> - ti,hwmods : Must be "cpgmac0"
> @@ -75,6 +78,7 @@ Examples:
> cpts_clock_mult = <0x80000000>;
> cpts_clock_shift = <29>;
> syscon = <&cm>;
> + cpsw-phy-sel = <&phy_sel>;
> cpsw_emac0: slave@0 {
> phy_id = <&davinci_mdio>, <0>;
> phy-mode = "rgmii-txid";
> @@ -103,6 +107,7 @@ Examples:
> cpts_clock_mult = <0x80000000>;
> cpts_clock_shift = <29>;
> syscon = <&cm>;
> + cpsw-phy-sel = <&phy_sel>;
> cpsw_emac0: slave@0 {
> phy_id = <&davinci_mdio>, <0>;
> phy-mode = "rgmii-txid";
> diff --git a/drivers/net/ethernet/ti/cpsw-phy-sel.c b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> --- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
> +++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> @@ -170,10 +170,13 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
> struct device_node *node;
> struct cpsw_phy_sel_priv *priv;
>
> - node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
> + node = of_parse_phandle(dev->of_node, "cpsw-phy-sel", 0);
> if (!node) {
Do you need to handle EPROBE_DEFER here? The phandle points to a
device which has not yet been loaded? I'm not sure exactly where it
will be returned, maybe it is bus_find_device(), but i expect to see
some handling of it somewhere in this function.
> - dev_err(dev, "Phy mode driver DT not found\n");
> - return;
> + node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
> + if (!node) {
> + dev_err(dev, "Phy mode driver DT not found\n");
> + return;
> + }
> }
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding
2018-08-08 11:58 ` Andrew Lunn
@ 2018-08-08 13:48 ` Tony Lindgren
2018-08-09 10:46 ` Tony Lindgren
0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2018-08-08 13:48 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, netdev, linux-omap, devicetree, Grygorii Strashko,
Ivan Khoronzhuk, Mark Rutland, Murali Karicheri, Rob Herring
* Andrew Lunn <andrew@lunn.ch> [180808 12:02]:
> > +++ b/Documentation/devicetree/bindings/net/cpsw.txt
> > @@ -19,6 +19,9 @@ Required properties:
> > - slaves : Specifies number for slaves
> > - active_slave : Specifies the slave to use for time stamping,
> > ethtool and SIOCGMIIPHY
> > +- cpsw-phy-sel : Specifies the phandle to the CPSW phy mode selection
> > + device. Note that in legacy cases cpsw-phy-sel may be
> > + a child device instead of a phandle.
>
> Hi Tony
>
> It would be good to reference cpsw-phy-sel.txt.
OK will add.
> > --- a/drivers/net/ethernet/ti/cpsw-phy-sel.c
> > +++ b/drivers/net/ethernet/ti/cpsw-phy-sel.c
> > @@ -170,10 +170,13 @@ void cpsw_phy_sel(struct device *dev, phy_interface_t phy_mode, int slave)
> > struct device_node *node;
> > struct cpsw_phy_sel_priv *priv;
> >
> > - node = of_get_child_by_name(dev->of_node, "cpsw-phy-sel");
> > + node = of_parse_phandle(dev->of_node, "cpsw-phy-sel", 0);
> > if (!node) {
>
> Do you need to handle EPROBE_DEFER here? The phandle points to a
> device which has not yet been loaded? I'm not sure exactly where it
> will be returned, maybe it is bus_find_device(), but i expect to see
> some handling of it somewhere in this function.
With the proper interconnect hierarchy in the device tree there should be
no EPROBE_DEFER happening here as the interconnects are probed in the
right order with the always on interrupt with system control module first :)
But then again, adding support for EPROBE_DEFER here won't hurt either,
will take a look.
Regards,
Tony
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding
2018-08-08 13:48 ` Tony Lindgren
@ 2018-08-09 10:46 ` Tony Lindgren
2018-08-09 22:47 ` Grygorii Strashko
0 siblings, 1 reply; 6+ messages in thread
From: Tony Lindgren @ 2018-08-09 10:46 UTC (permalink / raw)
To: Andrew Lunn
Cc: David Miller, netdev, linux-omap, devicetree, Grygorii Strashko,
Ivan Khoronzhuk, Mark Rutland, Murali Karicheri, Rob Herring
* Tony Lindgren <tony@atomide.com> [180808 13:52]:
> * Andrew Lunn <andrew@lunn.ch> [180808 12:02]:
> >
> > Do you need to handle EPROBE_DEFER here? The phandle points to a
> > device which has not yet been loaded? I'm not sure exactly where it
> > will be returned, maybe it is bus_find_device(), but i expect to see
> > some handling of it somewhere in this function.
If no device is found the driver just produces a warning currently.
And in that case cpsw attempts to continue with bootloader settings.
And looking at the caller function cpsw_slave_open() it also just
produces warnings for phy_connect() too..
I agree that in general this this whole pile of cpsw related drivers sure
could use some better error handling. Starting with making cpsw_slave_open()
and cpsw_phy_sel() return errors instead of just ignoring them might be a
good start.
Grygorii, care to add that note of things to do into your cpsw maintainer
hat?
> With the proper interconnect hierarchy in the device tree there should be
> no EPROBE_DEFER happening here as the interconnects are probed in the
> right order with the always on interrupt with system control module first :)
>
> But then again, adding support for EPROBE_DEFER here won't hurt either,
> will take a look.
I'll just add some notes about that to the patch description considering
the above.
Regards,
Tony
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding
2018-08-09 10:46 ` Tony Lindgren
@ 2018-08-09 22:47 ` Grygorii Strashko
0 siblings, 0 replies; 6+ messages in thread
From: Grygorii Strashko @ 2018-08-09 22:47 UTC (permalink / raw)
To: Tony Lindgren, Andrew Lunn
Cc: David Miller, netdev, linux-omap, devicetree, Ivan Khoronzhuk,
Mark Rutland, Murali Karicheri, Rob Herring
On 08/09/2018 05:46 AM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180808 13:52]:
>> * Andrew Lunn <andrew@lunn.ch> [180808 12:02]:
>>>
>>> Do you need to handle EPROBE_DEFER here? The phandle points to a
>>> device which has not yet been loaded? I'm not sure exactly where it
>>> will be returned, maybe it is bus_find_device(), but i expect to see
>>> some handling of it somewhere in this function.
>
> If no device is found the driver just produces a warning currently.
> And in that case cpsw attempts to continue with bootloader settings.
>
> And looking at the caller function cpsw_slave_open() it also just
> produces warnings for phy_connect() too..
>
> I agree that in general this this whole pile of cpsw related drivers sure
> could use some better error handling. Starting with making cpsw_slave_open()
> and cpsw_phy_sel() return errors instead of just ignoring them might be a
> good start.
>
> Grygorii, care to add that note of things to do into your cpsw maintainer
> hat?
Right. EPROBE_DEFER not supported for this module as of now.
>
>> With the proper interconnect hierarchy in the device tree there should be
>> no EPROBE_DEFER happening here as the interconnects are probed in the
>> right order with the always on interrupt with system control module first :)
>>
>> But then again, adding support for EPROBE_DEFER here won't hurt either,
>> will take a look.
>
> I'll just add some notes about that to the patch description considering
> the above.
thanks Tony.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding
@ 2018-08-09 22:47 ` Grygorii Strashko
0 siblings, 0 replies; 6+ messages in thread
From: Grygorii Strashko @ 2018-08-09 22:47 UTC (permalink / raw)
To: Tony Lindgren, Andrew Lunn
Cc: David Miller, netdev, linux-omap, devicetree, Ivan Khoronzhuk,
Mark Rutland, Murali Karicheri, Rob Herring
On 08/09/2018 05:46 AM, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [180808 13:52]:
>> * Andrew Lunn <andrew@lunn.ch> [180808 12:02]:
>>>
>>> Do you need to handle EPROBE_DEFER here? The phandle points to a
>>> device which has not yet been loaded? I'm not sure exactly where it
>>> will be returned, maybe it is bus_find_device(), but i expect to see
>>> some handling of it somewhere in this function.
>
> If no device is found the driver just produces a warning currently.
> And in that case cpsw attempts to continue with bootloader settings.
>
> And looking at the caller function cpsw_slave_open() it also just
> produces warnings for phy_connect() too..
>
> I agree that in general this this whole pile of cpsw related drivers sure
> could use some better error handling. Starting with making cpsw_slave_open()
> and cpsw_phy_sel() return errors instead of just ignoring them might be a
> good start.
>
> Grygorii, care to add that note of things to do into your cpsw maintainer
> hat?
Right. EPROBE_DEFER not supported for this module as of now.
>
>> With the proper interconnect hierarchy in the device tree there should be
>> no EPROBE_DEFER happening here as the interconnects are probed in the
>> right order with the always on interrupt with system control module first :)
>>
>> But then again, adding support for EPROBE_DEFER here won't hurt either,
>> will take a look.
>
> I'll just add some notes about that to the patch description considering
> the above.
thanks Tony.
--
regards,
-grygorii
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-10 1:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-08 8:03 [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding Tony Lindgren
2018-08-08 11:58 ` Andrew Lunn
2018-08-08 13:48 ` Tony Lindgren
2018-08-09 10:46 ` Tony Lindgren
2018-08-09 22:47 ` Grygorii Strashko
2018-08-09 22:47 ` Grygorii Strashko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.