Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

2018-08-09 Thread Grygorii Strashko 




On 08/09/2018 05:46 AM, Tony Lindgren wrote:

* Tony Lindgren  [180808 13:52]:

* Andrew Lunn  [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

Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

2018-08-09 Thread Tony Lindgren 
* Tony Lindgren  [180808 13:52]:
> * Andrew Lunn  [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

Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

2018-08-08 Thread Tony Lindgren 
* Andrew Lunn  [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

Re: [PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

2018-08-08 Thread Andrew Lunn 
> +++ 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 = <0x8000>;
>   cpts_clock_shift = <29>;
>   syscon = <>;
> + cpsw-phy-sel = <_sel>;
>   cpsw_emac0: slave@0 {
>   phy_id = <_mdio>, <0>;
>   phy-mode = "rgmii-txid";
> @@ -103,6 +107,7 @@ Examples:
>   cpts_clock_mult = <0x8000>;
>   cpts_clock_shift = <29>;
>   syscon = <>;
> + cpsw-phy-sel = <_sel>;
>   cpsw_emac0: slave@0 {
>   phy_id = <_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

[PATCH] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel and update binding

2018-08-08 Thread Tony Lindgren 
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: devicet...@vger.kernel.org
Cc: Grygorii Strashko 
Cc: Ivan Khoronzhuk 
Cc: Mark Rutland 
Cc: Murali Karicheri 
Cc: Rob Herring 
Signed-off-by: Tony Lindgren 
---
 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 = <0x8000>;
cpts_clock_shift = <29>;
syscon = <>;
+   cpsw-phy-sel = <_sel>;
cpsw_emac0: slave@0 {
phy_id = <_mdio>, <0>;
phy-mode = "rgmii-txid";
@@ -103,6 +107,7 @@ Examples:
cpts_clock_mult = <0x8000>;
cpts_clock_shift = <29>;
syscon = <>;
+   cpsw-phy-sel = <_sel>;
cpsw_emac0: slave@0 {
phy_id = <_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(_bus_type, NULL, node, match);
-- 
2.17.1