[PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Tony Lindgren]

The current cpsw usage for cpsw-phy-sel is undocumented but is used for
all the boards using cpsw. And cpsw-phy-sel is not really a child of
the cpsw device, it lives in the system control module instead.

Let's document the existing usage, and improve it a bit where we prefer
to use a phandle instead of a child device for it. That way we can
properly describe the hardware in dts files for things like genpd.

Cc: devicetree@vger.kernel.org
Cc: Andrew Lunn <andrew@lunn.ch>
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 | 6 ++++++
 1 file changed, 6 insertions(+)

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,10 @@ 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. See also cpsw-phy-sel.txt for it's binding.
+			  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 +79,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 +108,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";
-- 
2.18.0

[PATCH 2/2] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel

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

Note that deferred probe is mostly not supported by cpsw and this patch
does not attempt to fix that. In case deferred probe support is needed,
this could be added to cpsw_slave_open() and phy_connect() so they start
handling and returning errors.

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: Andrew Lunn <andrew@lunn.ch>
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>
---
 drivers/net/ethernet/ti/cpsw-phy-sel.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

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

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Grygorii Strashko]

Hi Tony,

On 08/29/2018 10:00 AM, Tony Lindgren wrote:
> The current cpsw usage for cpsw-phy-sel is undocumented but is used for
> all the boards using cpsw. And cpsw-phy-sel is not really a child of
> the cpsw device, it lives in the system control module instead.
> 
> Let's document the existing usage, and improve it a bit where we prefer
> to use a phandle instead of a child device for it. That way we can
> properly describe the hardware in dts files for things like genpd.

I'm ok with this series, but I really don't like cpsw-phy-sel in general.

It was introduced long time back and now I'm thinking about possibility to replace it with
one of current generic interfaces - for example mux-controller. 
Each port will control up to 3 muxes (port mode, idmode and rmii_ext_clk) and  
transform phy-mode => mux states.
What do you think?

Another option is to use phy, but it'd be complicated.

> 
> Cc: devicetree@vger.kernel.org
> Cc: Andrew Lunn <andrew@lunn.ch>
> 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 | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> 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,10 @@ 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. See also cpsw-phy-sel.txt for it's binding.
> +			  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 +79,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 +108,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";
> 

-- 
regards,
-grygorii

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Grygorii Strashko]

Hi Tony,

On 08/29/2018 10:00 AM, Tony Lindgren wrote:
> The current cpsw usage for cpsw-phy-sel is undocumented but is used for
> all the boards using cpsw. And cpsw-phy-sel is not really a child of
> the cpsw device, it lives in the system control module instead.
> 
> Let's document the existing usage, and improve it a bit where we prefer
> to use a phandle instead of a child device for it. That way we can
> properly describe the hardware in dts files for things like genpd.

I'm ok with this series, but I really don't like cpsw-phy-sel in general.

It was introduced long time back and now I'm thinking about possibility to replace it with
one of current generic interfaces - for example mux-controller. 
Each port will control up to 3 muxes (port mode, idmode and rmii_ext_clk) and  
transform phy-mode => mux states.
What do you think?

Another option is to use phy, but it'd be complicated.

> 
> Cc: devicetree@vger.kernel.org
> Cc: Andrew Lunn <andrew@lunn.ch>
> 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 | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> 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,10 @@ 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. See also cpsw-phy-sel.txt for it's binding.
> +			  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 +79,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 +108,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";
> 

-- 
regards,
-grygorii

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [180830 00:12]:
> Hi Tony,
> 
> On 08/29/2018 10:00 AM, Tony Lindgren wrote:
> > The current cpsw usage for cpsw-phy-sel is undocumented but is used for
> > all the boards using cpsw. And cpsw-phy-sel is not really a child of
> > the cpsw device, it lives in the system control module instead.
> > 
> > Let's document the existing usage, and improve it a bit where we prefer
> > to use a phandle instead of a child device for it. That way we can
> > properly describe the hardware in dts files for things like genpd.
> 
> I'm ok with this series, but I really don't like cpsw-phy-sel in general.

Yeah this binding predates any standards. This series
only fixes the nasty issue of cpsw claiming a module as a
child that's outside it's IO range.

> It was introduced long time back and now I'm thinking about possibility to replace it with
> one of current generic interfaces - for example mux-controller. 
> Each port will control up to 3 muxes (port mode, idmode and rmii_ext_clk) and  
> transform phy-mode => mux states.
> What do you think?

Sure a mux-controller here makes sense.

> Another option is to use phy, but it'd be complicated.

For the port muxes, how about a phy driver just using
a pinctrl driver?

In general, it seems cpsw is just an interconnect instance
(L4_FAST) with a control module (CPSW_WR) and a pile of
independent other modules. That's described nicely in
am437x TRM chapter "2.1.4 L4 Fast Peripheral Memory Map".
So from that point of view the binding reg entries right
now are all wrong :)

In the long run cpsw should be really treated as an
interconnect instance with it's control module providing
standard Linux framework services such as clock /
regulator / phy / pinctrl / iio whatever for the other
modules.

Just my 2c based on looking at the interconnect, I'm
not too familiar with cpsw otherwise.

Regards,

Tony

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Andrew Lunn]

> In the long run cpsw should be really treated as an
> interconnect instance with it's control module providing
> standard Linux framework services such as clock /
> regulator / phy / pinctrl / iio whatever for the other
> modules.

Some of us have been applying pressure for a new driver. This sounds
like another argument for such a re-write.

	   Andrew

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Grygorii Strashko]

On 08/29/2018 07:47 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [180830 00:12]:
>> Hi Tony,
>>
>> On 08/29/2018 10:00 AM, Tony Lindgren wrote:
>>> The current cpsw usage for cpsw-phy-sel is undocumented but is used for
>>> all the boards using cpsw. And cpsw-phy-sel is not really a child of
>>> the cpsw device, it lives in the system control module instead.
>>>
>>> Let's document the existing usage, and improve it a bit where we prefer
>>> to use a phandle instead of a child device for it. That way we can
>>> properly describe the hardware in dts files for things like genpd.
>>
>> I'm ok with this series, but I really don't like cpsw-phy-sel in general.
> 
> Yeah this binding predates any standards. This series
> only fixes the nasty issue of cpsw claiming a module as a
> child that's outside it's IO range.
> 
>> It was introduced long time back and now I'm thinking about possibility to replace it with
>> one of current generic interfaces - for example mux-controller.
>> Each port will control up to 3 muxes (port mode, idmode and rmii_ext_clk) and
>> transform phy-mode => mux states.
>> What do you think?
> 
> Sure a mux-controller here makes sense.
> 
>> Another option is to use phy, but it'd be complicated.
> 
> For the port muxes, how about a phy driver just using
> a pinctrl driver?
> 
> In general, it seems cpsw is just an interconnect instance
> (L4_FAST) with a control module (CPSW_WR) and a pile of
> independent other modules. That's described nicely in
> am437x TRM chapter "2.1.4 L4 Fast Peripheral Memory Map".
> So from that point of view the binding reg entries right
> now are all wrong :)

TRM not consistent - for am5 it's one MMIO region.

> 
> In the long run cpsw should be really treated as an
> interconnect instance with it's control module providing
> standard Linux framework services such as clock /
> regulator / phy / pinctrl / iio whatever for the other
> modules.
> 
> Just my 2c based on looking at the interconnect, I'm
> not too familiar with cpsw otherwise.

It's not separate modules. this is composite module which have only one 
fck/ick and most of blocks can't even function without each other.
Above might be the case for Keystone 2, but not omap CPSW.
Keystone 2 - has packet processor, security accelerator, queue manager 
in addition to its basic switch block.

-- 
regards,
-grygorii

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Grygorii Strashko]

On 08/29/2018 07:47 PM, Tony Lindgren wrote:
> * Grygorii Strashko <grygorii.strashko@ti.com> [180830 00:12]:
>> Hi Tony,
>>
>> On 08/29/2018 10:00 AM, Tony Lindgren wrote:
>>> The current cpsw usage for cpsw-phy-sel is undocumented but is used for
>>> all the boards using cpsw. And cpsw-phy-sel is not really a child of
>>> the cpsw device, it lives in the system control module instead.
>>>
>>> Let's document the existing usage, and improve it a bit where we prefer
>>> to use a phandle instead of a child device for it. That way we can
>>> properly describe the hardware in dts files for things like genpd.
>>
>> I'm ok with this series, but I really don't like cpsw-phy-sel in general.
> 
> Yeah this binding predates any standards. This series
> only fixes the nasty issue of cpsw claiming a module as a
> child that's outside it's IO range.
> 
>> It was introduced long time back and now I'm thinking about possibility to replace it with
>> one of current generic interfaces - for example mux-controller.
>> Each port will control up to 3 muxes (port mode, idmode and rmii_ext_clk) and
>> transform phy-mode => mux states.
>> What do you think?
> 
> Sure a mux-controller here makes sense.
> 
>> Another option is to use phy, but it'd be complicated.
> 
> For the port muxes, how about a phy driver just using
> a pinctrl driver?
> 
> In general, it seems cpsw is just an interconnect instance
> (L4_FAST) with a control module (CPSW_WR) and a pile of
> independent other modules. That's described nicely in
> am437x TRM chapter "2.1.4 L4 Fast Peripheral Memory Map".
> So from that point of view the binding reg entries right
> now are all wrong :)

TRM not consistent - for am5 it's one MMIO region.

> 
> In the long run cpsw should be really treated as an
> interconnect instance with it's control module providing
> standard Linux framework services such as clock /
> regulator / phy / pinctrl / iio whatever for the other
> modules.
> 
> Just my 2c based on looking at the interconnect, I'm
> not too familiar with cpsw otherwise.

It's not separate modules. this is composite module which have only one 
fck/ick and most of blocks can't even function without each other.
Above might be the case for Keystone 2, but not omap CPSW.
Keystone 2 - has packet processor, security accelerator, queue manager 
in addition to its basic switch block.

-- 
regards,
-grygorii

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[Tony Lindgren]

* Grygorii Strashko <grygorii.strashko@ti.com> [180830 17:08]:
> On 08/29/2018 07:47 PM, Tony Lindgren wrote:
> > In general, it seems cpsw is just an interconnect instance
> > (L4_FAST) with a control module (CPSW_WR) and a pile of
> > independent other modules. That's described nicely in
> > am437x TRM chapter "2.1.4 L4 Fast Peripheral Memory Map".
> > So from that point of view the binding reg entries right
> > now are all wrong :)
> 
> TRM not consistent - for am5 it's one MMIO region.

Well that same information is there in 57xx TRM in chapter
"Table 26-1454. GMAC_SW Instance Summary". But yeah, all
the cpsw internal devices are stuffed into a single
interconnect target module.

> > In the long run cpsw should be really treated as an
> > interconnect instance with it's control module providing
> > standard Linux framework services such as clock /
> > regulator / phy / pinctrl / iio whatever for the other
> > modules.
> > 
> > Just my 2c based on looking at the interconnect, I'm
> > not too familiar with cpsw otherwise.
> 
> It's not separate modules. this is composite module which have only one
> fck/ick and most of blocks can't even function without each other.
> Above might be the case for Keystone 2, but not omap CPSW.
> Keystone 2 - has packet processor, security accelerator, queue manager in
> addition to its basic switch block.

Yeah there's just one fck/ick as it's all in a single
interconnect module. But you might want to look at the
CPSW_WR device registers and see what gate clocks and other
Linux generic subsystem services CPSW_WR could provide for
the other cpsw internal devices. It might just make your
life easier maintaining all these variants ;)

Regards,

Tony

Re: [PATCH 1/2] dt-bindings: net: cpsw: Document cpsw-phy-sel usage but prefer phandle

[David Miller]

From: Tony Lindgren <tony@atomide.com>
Date: Wed, 29 Aug 2018 08:00:23 -0700

> The current cpsw usage for cpsw-phy-sel is undocumented but is used for
> all the boards using cpsw. And cpsw-phy-sel is not really a child of
> the cpsw device, it lives in the system control module instead.
> 
> Let's document the existing usage, and improve it a bit where we prefer
> to use a phandle instead of a child device for it. That way we can
> properly describe the hardware in dts files for things like genpd.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Applied.

Re: [PATCH 2/2] net: ethernet: cpsw-phy-sel: prefer phandle for phy sel

[David Miller]

From: Tony Lindgren <tony@atomide.com>
Date: Wed, 29 Aug 2018 08:00:24 -0700

> 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.
> 
> Note that deferred probe is mostly not supported by cpsw and this patch
> does not attempt to fix that. In case deferred probe support is needed,
> this could be added to cpsw_slave_open() and phy_connect() so they start
> handling and returning errors.
> 
> 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.
> 
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Applied.