[PATCH 1/3] ARM: zynq: DT: Fix Ethernet phy modes

[PATCH 1/3] ARM: zynq: DT: Fix Ethernet phy modes

[Soren Brinkmann]

The used PHYs should be qualified as 'rgmii-id' instead of just 'rgmii'.
For the Zed board this seems to make a difference between working and
broken Ethernet.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/boot/dts/zynq-zc702.dts | 2 +-
 arch/arm/boot/dts/zynq-zc706.dts | 2 +-
 arch/arm/boot/dts/zynq-zed.dts   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
index 835c3089c61c..30bcfe20f0bc 100644
--- a/arch/arm/boot/dts/zynq-zc702.dts
+++ b/arch/arm/boot/dts/zynq-zc702.dts
@@ -35,7 +35,7 @@
 
 &gem0 {
 	status = "okay";
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 };
 
 &i2c0 {
diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
index 4cc9913078cd..3d9cb023e158 100644
--- a/arch/arm/boot/dts/zynq-zc706.dts
+++ b/arch/arm/boot/dts/zynq-zc706.dts
@@ -32,7 +32,7 @@
 
 &gem0 {
 	status = "okay";
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 };
 
 &i2c0 {
diff --git a/arch/arm/boot/dts/zynq-zed.dts b/arch/arm/boot/dts/zynq-zed.dts
index 82d7ef1a9a9c..42deb1b7dbc8 100644
--- a/arch/arm/boot/dts/zynq-zed.dts
+++ b/arch/arm/boot/dts/zynq-zed.dts
@@ -32,7 +32,7 @@
 
 &gem0 {
 	status = "okay";
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 };
 
 &sdhci0 {
-- 
2.0.1.1.gfbfc394

[PATCH 2/3] ARM: zynq: DT: Move size/address properties to dtsi

[Soren Brinkmann]

Move the GEM's size and address cells properties to the common
dtsi file.

Cc: Andreas Färber <afaerber@suse.de>
Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/boot/dts/zynq-7000.dtsi      | 4 ++++
 arch/arm/boot/dts/zynq-parallella.dts | 2 --
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 6cc83d4c6c76..4e6e96ef7bd0 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -195,6 +195,8 @@
 			interrupts = <0 22 4>;
 			clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>;
 			clock-names = "pclk", "hclk", "tx_clk";
+			#address-cells = <1>;
+			#size-cells = <0>;
 		};
 
 		gem1: ethernet@e000c000 {
@@ -204,6 +206,8 @@
 			interrupts = <0 45 4>;
 			clocks = <&clkc 31>, <&clkc 31>, <&clkc 14>;
 			clock-names = "pclk", "hclk", "tx_clk";
+			#address-cells = <1>;
+			#size-cells = <0>;
 		};
 
 		sdhci0: sdhci@e0100000 {
diff --git a/arch/arm/boot/dts/zynq-parallella.dts b/arch/arm/boot/dts/zynq-parallella.dts
index 41afd9da6876..690e6206beac 100644
--- a/arch/arm/boot/dts/zynq-parallella.dts
+++ b/arch/arm/boot/dts/zynq-parallella.dts
@@ -38,8 +38,6 @@
 	status = "okay";
 	phy-mode = "rgmii-id";
 	phy-handle = <&ethernet_phy>;
-	#address-cells = <1>;
-	#size-cells = <0>;
 
 	ethernet_phy: ethernet-phy@0 {
 		/* Marvell 88E1318 */
-- 
2.0.1.1.gfbfc394

Re: [PATCH 2/3] ARM: zynq: DT: Move size/address properties to dtsi

[Andreas Färber]

Am 20.08.2014 17:56, schrieb Soren Brinkmann:
> Move the GEM's size and address cells properties to the common
> dtsi file.
> 
> Cc: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  arch/arm/boot/dts/zynq-7000.dtsi      | 4 ++++
>  arch/arm/boot/dts/zynq-parallella.dts | 2 --
>  2 files changed, 4 insertions(+), 2 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH 2/3] ARM: zynq: DT: Move size/address properties to dtsi

[Michal Simek]

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

On 08/20/2014 06:00 PM, Andreas Färber wrote:
> Am 20.08.2014 17:56, schrieb Soren Brinkmann:
>> Move the GEM's size and address cells properties to the common
>> dtsi file.
>>
>> Cc: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> ---
>>  arch/arm/boot/dts/zynq-7000.dtsi      | 4 ++++
>>  arch/arm/boot/dts/zynq-parallella.dts | 2 --
>>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Andreas Färber <afaerber@suse.de>

Applied.

Thanks,
Michal



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Soren Brinkmann]

Add missing Ethernet phys to Zynq DTs.

Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
---
 arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++
 arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++
 arch/arm/boot/dts/zynq-zed.dts   | 6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
index 30bcfe20f0bc..fa810505ab8f 100644
--- a/arch/arm/boot/dts/zynq-zc702.dts
+++ b/arch/arm/boot/dts/zynq-zc702.dts
@@ -36,6 +36,12 @@
 &gem0 {
 	status = "okay";
 	phy-mode = "rgmii-id";
+	phy-handle = <&ethernet_phy>;
+
+	ethernet_phy: ethernet-phy@7 {
+		compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22";
+		reg = <7>;
+	};
 };
 
 &i2c0 {
diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
index 3d9cb023e158..d446b44d45a2 100644
--- a/arch/arm/boot/dts/zynq-zc706.dts
+++ b/arch/arm/boot/dts/zynq-zc706.dts
@@ -33,6 +33,12 @@
 &gem0 {
 	status = "okay";
 	phy-mode = "rgmii-id";
+	phy-handle = <&ethernet_phy>;
+
+	ethernet_phy: ethernet-phy@7 {
+		compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22";
+		reg = <7>;
+	};
 };
 
 &i2c0 {
diff --git a/arch/arm/boot/dts/zynq-zed.dts b/arch/arm/boot/dts/zynq-zed.dts
index 42deb1b7dbc8..92662eb078ce 100644
--- a/arch/arm/boot/dts/zynq-zed.dts
+++ b/arch/arm/boot/dts/zynq-zed.dts
@@ -33,6 +33,12 @@
 &gem0 {
 	status = "okay";
 	phy-mode = "rgmii-id";
+	phy-handle = <&ethernet_phy>;
+
+	ethernet_phy: ethernet-phy@0 {
+		compatible = "marvell,88e1518", "ethernet-phy-ieee802.3-c22";
+		reg = <0>;
+	};
 };
 
 &sdhci0 {
-- 
2.0.1.1.gfbfc394

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Michal Simek]

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

On 08/20/2014 05:56 PM, Soren Brinkmann wrote:
> Add missing Ethernet phys to Zynq DTs.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> ---
>  arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++
>  arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++
>  arch/arm/boot/dts/zynq-zed.dts   | 6 ++++++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
> index 30bcfe20f0bc..fa810505ab8f 100644
> --- a/arch/arm/boot/dts/zynq-zc702.dts
> +++ b/arch/arm/boot/dts/zynq-zc702.dts
> @@ -36,6 +36,12 @@
>  &gem0 {
>  	status = "okay";
>  	phy-mode = "rgmii-id";
> +	phy-handle = <&ethernet_phy>;
> +
> +	ethernet_phy: ethernet-phy@7 {
> +		compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22";

c22 is completely unused by the kernel and also c22 is default option anyway.
Any advantage to have c22 specified here?

Thanks,
Michal


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Andreas Färber]

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

Am 21.08.2014 10:41, schrieb Michal Simek:
> On 08/20/2014 05:56 PM, Soren Brinkmann wrote:
>> Add missing Ethernet phys to Zynq DTs.
>>
>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
>> ---
>>  arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++
>>  arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++
>>  arch/arm/boot/dts/zynq-zed.dts   | 6 ++++++
>>  3 files changed, 18 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
>> index 30bcfe20f0bc..fa810505ab8f 100644
>> --- a/arch/arm/boot/dts/zynq-zc702.dts
>> +++ b/arch/arm/boot/dts/zynq-zc702.dts
>> @@ -36,6 +36,12 @@
>>  &gem0 {
>>  	status = "okay";
>>  	phy-mode = "rgmii-id";
>> +	phy-handle = <&ethernet_phy>;
>> +
>> +	ethernet_phy: ethernet-phy@7 {
>> +		compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22";
> 
> c22 is completely unused by the kernel and also c22 is default option anyway.
> Any advantage to have c22 specified here?

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt

Sören's marvell,* entries do not seem documented, therefore I used the
documented "ethernet-phy-idAAAA.BBBB" based syntax. The documented
example specifically uses -c22, too.

Either it's okay to prepend unrecognized model strings, then you should
update zynq-parallella.dts as well (I put the model in a comment there)
or use the official strings like I used and keep the readable models as
comments. Documenting all those "marvell,88e1116r", "marvell,88e1518",
"marvell,88e1318" PHY bindings and possibly prepending them to the ID
based strings would be another option, of course.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Sören Brinkmann]

On Thu, 2014-08-21 at 01:32PM +0200, Andreas Färber wrote:
> Am 21.08.2014 10:41, schrieb Michal Simek:
> > On 08/20/2014 05:56 PM, Soren Brinkmann wrote:
> >> Add missing Ethernet phys to Zynq DTs.
> >>
> >> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> >> ---
> >>  arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++
> >>  arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++
> >>  arch/arm/boot/dts/zynq-zed.dts   | 6 ++++++
> >>  3 files changed, 18 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
> >> index 30bcfe20f0bc..fa810505ab8f 100644
> >> --- a/arch/arm/boot/dts/zynq-zc702.dts
> >> +++ b/arch/arm/boot/dts/zynq-zc702.dts
> >> @@ -36,6 +36,12 @@
> >>  &gem0 {
> >>  	status = "okay";
> >>  	phy-mode = "rgmii-id";
> >> +	phy-handle = <&ethernet_phy>;
> >> +
> >> +	ethernet_phy: ethernet-phy@7 {
> >> +		compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22";
> > 
> > c22 is completely unused by the kernel and also c22 is default option anyway.
> > Any advantage to have c22 specified here?
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt
> 
> Sören's marvell,* entries do not seem documented, therefore I used the
> documented "ethernet-phy-idAAAA.BBBB" based syntax. The documented
> example specifically uses -c22, too.
> 
> Either it's okay to prepend unrecognized model strings, then you should
> update zynq-parallella.dts as well (I put the model in a comment there)
> or use the official strings like I used and keep the readable models as
> comments. Documenting all those "marvell,88e1116r", "marvell,88e1518",
> "marvell,88e1318" PHY bindings and possibly prepending them to the ID
> based strings would be another option, of course.

These phy-bindings are everything than obvious to me. It seems the docs
are spread across a couple of different files and not fully up to date
either. I basically tried to get something working out of the docs, the
parallela and our vendor DT files.

So my thinkings:
 - the compatible string with the -c22 is used and documented in the phy
   bindings, it should be there.
 - the ID based strings seem to be not needed since, IIUC, the core
   reads the ID from the PHY and uses it, so I just left it out not
   trying to figure out how to obtain the correct ID
 - the marvell compatible strings are used in our vendor tree. They
   aren't used anywhere but in our vendor tree. I though keeping them is
   nice since it identifies the PHY fully. And in case that level of
   detail is needed at some point it is already there.

Assuming that we wanna keep things this way, I'm happy to re-spin this
patch and also add a similar compatible string to the parallela DT.

	Thanks,
	Sören

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Jason Gunthorpe]

On Thu, Aug 21, 2014 at 08:49:19AM -0700, Sören Brinkmann wrote:

> So my thinkings:
>  - the compatible string with the -c22 is used and documented in the phy
>    bindings, it should be there.

Yes

>  - the ID based strings seem to be not needed since, IIUC, the core
>    reads the ID from the PHY and uses it, so I just left it out not
>    trying to figure out how to obtain the correct ID

It is certainly optional, I added the property to solve some obscure
problems with probing, but I've noticed people using it more broadly.
I suspect it also speeds up booting a tiny bit.

>  - the marvell compatible strings are used in our vendor tree. They
>    aren't used anywhere but in our vendor tree. I though keeping them is
>    nice since it identifies the PHY fully. And in case that level of
>    detail is needed at some point it is already there.

DT is complex enough already, don't include useless compatible strings
in mainline - people will see and tend to blindly copy. Ideally your
vendor tree would use the standard property :|

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Sören Brinkmann]

On Fri, 2014-08-22 at 10:20AM -0600, Jason Gunthorpe wrote:
> On Thu, Aug 21, 2014 at 08:49:19AM -0700, Sören Brinkmann wrote:
> 
> > So my thinkings:
> >  - the compatible string with the -c22 is used and documented in the phy
> >    bindings, it should be there.
> 
> Yes
> 
> >  - the ID based strings seem to be not needed since, IIUC, the core
> >    reads the ID from the PHY and uses it, so I just left it out not
> >    trying to figure out how to obtain the correct ID
> 
> It is certainly optional, I added the property to solve some obscure
> problems with probing, but I've noticed people using it more broadly.
> I suspect it also speeds up booting a tiny bit.
> 
> >  - the marvell compatible strings are used in our vendor tree. They
> >    aren't used anywhere but in our vendor tree. I though keeping them is
> >    nice since it identifies the PHY fully. And in case that level of
> >    detail is needed at some point it is already there.
> 
> DT is complex enough already, don't include useless compatible strings
> in mainline - people will see and tend to blindly copy. Ideally your
> vendor tree would use the standard property :|

There are different opinions on this. I also heard to just add such
strings, so in case this level of differentiation becomes necessary at
some later point, it is already existing.

	Sören

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Jason Gunthorpe]

On Fri, Aug 22, 2014 at 09:31:20AM -0700, Sören Brinkmann wrote:

> > >  - the marvell compatible strings are used in our vendor tree. They
> > >    aren't used anywhere but in our vendor tree. I though keeping them is
> > >    nice since it identifies the PHY fully. And in case that level of
> > >    detail is needed at some point it is already there.
> > 
> > DT is complex enough already, don't include useless compatible strings
> > in mainline - people will see and tend to blindly copy. Ideally your
> > vendor tree would use the standard property :|
> 
> There are different opinions on this. I also heard to just add such
> strings, so in case this level of differentiation becomes necessary at
> some later point, it is already existing.

In some cases, yes, and they should be docuemented in the binding, of
course.

But there is already a standard compatible string format to uniquely
identify a phy, so in this case another string is not justified, and
just creates confusion about what is correct.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Florian Fainelli]

2014-08-21 8:49 GMT-07:00 Sören Brinkmann <soren.brinkmann@xilinx.com>:
> On Thu, 2014-08-21 at 01:32PM +0200, Andreas Färber wrote:
>> Am 21.08.2014 10:41, schrieb Michal Simek:
>> > On 08/20/2014 05:56 PM, Soren Brinkmann wrote:
>> >> Add missing Ethernet phys to Zynq DTs.
>> >>
>> >> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> >> ---
>> >>  arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++
>> >>  arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++
>> >>  arch/arm/boot/dts/zynq-zed.dts   | 6 ++++++
>> >>  3 files changed, 18 insertions(+)
>> >>
>> >> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
>> >> index 30bcfe20f0bc..fa810505ab8f 100644
>> >> --- a/arch/arm/boot/dts/zynq-zc702.dts
>> >> +++ b/arch/arm/boot/dts/zynq-zc702.dts
>> >> @@ -36,6 +36,12 @@
>> >>  &gem0 {
>> >>    status = "okay";
>> >>    phy-mode = "rgmii-id";
>> >> +  phy-handle = <&ethernet_phy>;
>> >> +
>> >> +  ethernet_phy: ethernet-phy@7 {
>> >> +          compatible = "marvell,88e1116r", "ethernet-phy-ieee802.3-c22";
>> >
>> > c22 is completely unused by the kernel and also c22 is default option anyway.
>> > Any advantage to have c22 specified here?
>>
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt
>>
>> Sören's marvell,* entries do not seem documented, therefore I used the
>> documented "ethernet-phy-idAAAA.BBBB" based syntax. The documented
>> example specifically uses -c22, too.
>>
>> Either it's okay to prepend unrecognized model strings, then you should
>> update zynq-parallella.dts as well (I put the model in a comment there)
>> or use the official strings like I used and keep the readable models as
>> comments. Documenting all those "marvell,88e1116r", "marvell,88e1518",
>> "marvell,88e1318" PHY bindings and possibly prepending them to the ID
>> based strings would be another option, of course.
>
> These phy-bindings are everything than obvious to me. It seems the docs
> are spread across a couple of different files and not fully up to date
> either. I basically tried to get something working out of the docs, the
> parallela and our vendor DT files.

There is one file that describes everything that is needed in
Documentation/devicetree/bindings/net/phy.txt, if it is not clear,
patches are welcome.

>
> So my thinkings:
>  - the compatible string with the -c22 is used and documented in the phy
>    bindings, it should be there.

The clause 22 compatible string is not used, but we do need it to know
it is not a clause 45 PHY, which implicitly makes the clause 22
compatible string used.

>  - the ID based strings seem to be not needed since, IIUC, the core
>    reads the ID from the PHY and uses it, so I just left it out not
>    trying to figure out how to obtain the correct ID

It is not needed, but it is one way to specify a PHY device if you do
not know what compatible string to use instead.

>  - the marvell compatible strings are used in our vendor tree. They
>    aren't used anywhere but in our vendor tree. I though keeping them is
>    nice since it identifies the PHY fully. And in case that level of
>    detail is needed at some point it is already there.

And this is the recommended way to do it in case we ever need to key a
software decision based on the hardware.

>
> Assuming that we wanna keep things this way, I'm happy to re-spin this
> patch and also add a similar compatible string to the parallela DT.

Your initial patchset is correct with repesct to how the Device Tree
binding for Ethernet PHY nodes is defined.

>
>         Thanks,
>         Sören
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Jason Gunthorpe]

On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote:

> >  - the ID based strings seem to be not needed since, IIUC, the core
> >    reads the ID from the PHY and uses it, so I just left it out not
> >    trying to figure out how to obtain the correct ID
> 
> It is not needed, but it is one way to specify a PHY device if you do
> not know what compatible string to use instead.

No, it is a way to specify a PHY device if the kernel can't auto probe
the Phy ID.

Last I checked, the kernel doesn't support plain text compatible
strings for phys - everything is driven on the phy id, either auto
probed or specified in the DT.

> >  - the marvell compatible strings are used in our vendor tree. They
> >    aren't used anywhere but in our vendor tree. I though keeping them is
> >    nice since it identifies the PHY fully. And in case that level of
> >    detail is needed at some point it is already there.
> 
> And this is the recommended way to do it in case we ever need to key a
> software decision based on the hardware.

All compatible strings need to be documented.

.. and they need to encode more information than you get from the phy
id - die revsision, package option, functional options, voltage
codes. Etc.

.. and they actually need to be *right*

An example: The kernel reports 88E1318S for all four chips in that
family, AFAIK you have to read the package marking to figure out which
you have (it is the same die, with options switched on/off at
packaging time). People have already posted patches trying to
helpfully add a 'marvell,88E1318S' compatible string based on kernel
output. Except it is wrong, it isn't actually the '8S version in the
HW.

Even worse, Marvell has a whole series of socket compatible phys. Just
because the board the DT author looked at has a '318, doesn't mean
that every board ever made will. We've actually already been switching
between the 318 and 318S for production depending on which has part
availability.

Basically: don't try to override self-discoverable hardware in DT
without a really good reason.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Florian Fainelli]

On 08/25/2014 10:46 AM, Jason Gunthorpe wrote:
> On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote:
> 
>>>  - the ID based strings seem to be not needed since, IIUC, the core
>>>    reads the ID from the PHY and uses it, so I just left it out not
>>>    trying to figure out how to obtain the correct ID
>>
>> It is not needed, but it is one way to specify a PHY device if you do
>> not know what compatible string to use instead.
> 
> No, it is a way to specify a PHY device if the kernel can't auto probe
> the Phy ID.
> 
> Last I checked, the kernel doesn't support plain text compatible
> strings for phys - everything is driven on the phy id, either auto
> probed or specified in the DT.

That's right. Some PHY drivers might be relying on specific compatible
strings though, but not the core PHY library that probes and maps a
driver to a PHY node.

> 
>>>  - the marvell compatible strings are used in our vendor tree. They
>>>    aren't used anywhere but in our vendor tree. I though keeping them is
>>>    nice since it identifies the PHY fully. And in case that level of
>>>    detail is needed at some point it is already there.
>>
>> And this is the recommended way to do it in case we ever need to key a
>> software decision based on the hardware.
> 
> All compatible strings need to be documented.
> 
> .. and they need to encode more information than you get from the phy
> id - die revsision, package option, functional options, voltage
> codes. Etc.
> 
> .. and they actually need to be *right*

Agreed.

> 
> An example: The kernel reports 88E1318S for all four chips in that
> family, AFAIK you have to read the package marking to figure out which
> you have (it is the same die, with options switched on/off at
> packaging time). People have already posted patches trying to
> helpfully add a 'marvell,88E1318S' compatible string based on kernel
> output. Except it is wrong, it isn't actually the '8S version in the
> HW.
> 
> Even worse, Marvell has a whole series of socket compatible phys. Just
> because the board the DT author looked at has a '318, doesn't mean
> that every board ever made will. We've actually already been switching
> between the 318 and 318S for production depending on which has part
> availability.
> 
> Basically: don't try to override self-discoverable hardware in DT
> without a really good reason.

I think that's a very good point, at the very least let's use a
compatible string that contains the full 32-bits PHY OUI.

Thanks
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 3242 bytes --]

On 08/25/2014 10:21 PM, Florian Fainelli wrote:
> On 08/25/2014 10:46 AM, Jason Gunthorpe wrote:
>> On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote:
>>
>>>>  - the ID based strings seem to be not needed since, IIUC, the core
>>>>    reads the ID from the PHY and uses it, so I just left it out not
>>>>    trying to figure out how to obtain the correct ID
>>>
>>> It is not needed, but it is one way to specify a PHY device if you do
>>> not know what compatible string to use instead.
>>
>> No, it is a way to specify a PHY device if the kernel can't auto probe
>> the Phy ID.
>>
>> Last I checked, the kernel doesn't support plain text compatible
>> strings for phys - everything is driven on the phy id, either auto
>> probed or specified in the DT.
> 
> That's right. Some PHY drivers might be relying on specific compatible
> strings though, but not the core PHY library that probes and maps a
> driver to a PHY node.
> 
>>
>>>>  - the marvell compatible strings are used in our vendor tree. They
>>>>    aren't used anywhere but in our vendor tree. I though keeping them is
>>>>    nice since it identifies the PHY fully. And in case that level of
>>>>    detail is needed at some point it is already there.
>>>
>>> And this is the recommended way to do it in case we ever need to key a
>>> software decision based on the hardware.
>>
>> All compatible strings need to be documented.
>>
>> .. and they need to encode more information than you get from the phy
>> id - die revsision, package option, functional options, voltage
>> codes. Etc.
>>
>> .. and they actually need to be *right*
> 
> Agreed.
> 
>>
>> An example: The kernel reports 88E1318S for all four chips in that
>> family, AFAIK you have to read the package marking to figure out which
>> you have (it is the same die, with options switched on/off at
>> packaging time). People have already posted patches trying to
>> helpfully add a 'marvell,88E1318S' compatible string based on kernel
>> output. Except it is wrong, it isn't actually the '8S version in the
>> HW.
>>
>> Even worse, Marvell has a whole series of socket compatible phys. Just
>> because the board the DT author looked at has a '318, doesn't mean
>> that every board ever made will. We've actually already been switching
>> between the 318 and 318S for production depending on which has part
>> availability.
>>
>> Basically: don't try to override self-discoverable hardware in DT
>> without a really good reason.
> 
> I think that's a very good point, at the very least let's use a
> compatible string that contains the full 32-bits PHY OUI.

I think resolution is:
1. Do not use marvell,88e1518 because it is not listed anywhere
2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection
if there is different phy on the board and we shouldn't restrict us in this.
In spite of autodetection takes some time.
3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added
4. Any listed compatible string has to be parsed which takes time

That's why I think make sense not to use any compatible string.
This should give us all flexibility which we want to have.

Thanks,
Michal



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Andreas Färber]

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

Am 29.08.2014 16:08, schrieb Michal Simek:
> On 08/25/2014 10:21 PM, Florian Fainelli wrote:
>> On 08/25/2014 10:46 AM, Jason Gunthorpe wrote:
>>> On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote:
>>>
>>>>>  - the ID based strings seem to be not needed since, IIUC, the core
>>>>>    reads the ID from the PHY and uses it, so I just left it out not
>>>>>    trying to figure out how to obtain the correct ID
>>>>
>>>> It is not needed, but it is one way to specify a PHY device if you do
>>>> not know what compatible string to use instead.
>>>
>>> No, it is a way to specify a PHY device if the kernel can't auto probe
>>> the Phy ID.
>>>
>>> Last I checked, the kernel doesn't support plain text compatible
>>> strings for phys - everything is driven on the phy id, either auto
>>> probed or specified in the DT.
>>
>> That's right. Some PHY drivers might be relying on specific compatible
>> strings though, but not the core PHY library that probes and maps a
>> driver to a PHY node.
>>
>>>
>>>>>  - the marvell compatible strings are used in our vendor tree. They
>>>>>    aren't used anywhere but in our vendor tree. I though keeping them is
>>>>>    nice since it identifies the PHY fully. And in case that level of
>>>>>    detail is needed at some point it is already there.
>>>>
>>>> And this is the recommended way to do it in case we ever need to key a
>>>> software decision based on the hardware.
>>>
>>> All compatible strings need to be documented.
>>>
>>> .. and they need to encode more information than you get from the phy
>>> id - die revsision, package option, functional options, voltage
>>> codes. Etc.
>>>
>>> .. and they actually need to be *right*
>>
>> Agreed.
>>
>>>
>>> An example: The kernel reports 88E1318S for all four chips in that
>>> family, AFAIK you have to read the package marking to figure out which
>>> you have (it is the same die, with options switched on/off at
>>> packaging time). People have already posted patches trying to
>>> helpfully add a 'marvell,88E1318S' compatible string based on kernel
>>> output. Except it is wrong, it isn't actually the '8S version in the
>>> HW.
>>>
>>> Even worse, Marvell has a whole series of socket compatible phys. Just
>>> because the board the DT author looked at has a '318, doesn't mean
>>> that every board ever made will. We've actually already been switching
>>> between the 318 and 318S for production depending on which has part
>>> availability.
>>>
>>> Basically: don't try to override self-discoverable hardware in DT
>>> without a really good reason.
>>
>> I think that's a very good point, at the very least let's use a
>> compatible string that contains the full 32-bits PHY OUI.
> 
> I think resolution is:
> 1. Do not use marvell,88e1518 because it is not listed anywhere
> 2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection
> if there is different phy on the board and we shouldn't restrict us in this.
> In spite of autodetection takes some time.
> 3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added
> 4. Any listed compatible string has to be parsed which takes time
> 
> That's why I think make sense not to use any compatible string.
> This should give us all flexibility which we want to have.

Sorry, but we do need some node that we can reference via phy-handle
from the gem node, don't we?

In that case, is not specifying any compatible string a valid option?

If you don't want to specify the IDs, then I would've assumed we need to
specify the -c22 in order to have *some* compatible string in order to
trigger loading of the appropriate driver module.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Sören Brinkmann]

On Fri, 2014-08-29 at 05:18PM +0200, Andreas Färber wrote:
> Am 29.08.2014 16:08, schrieb Michal Simek:
> > On 08/25/2014 10:21 PM, Florian Fainelli wrote:
> >> On 08/25/2014 10:46 AM, Jason Gunthorpe wrote:
> >>> On Fri, Aug 22, 2014 at 01:47:09PM -0700, Florian Fainelli wrote:
> >>>
> >>>>>  - the ID based strings seem to be not needed since, IIUC, the core
> >>>>>    reads the ID from the PHY and uses it, so I just left it out not
> >>>>>    trying to figure out how to obtain the correct ID
> >>>>
> >>>> It is not needed, but it is one way to specify a PHY device if you do
> >>>> not know what compatible string to use instead.
> >>>
> >>> No, it is a way to specify a PHY device if the kernel can't auto probe
> >>> the Phy ID.
> >>>
> >>> Last I checked, the kernel doesn't support plain text compatible
> >>> strings for phys - everything is driven on the phy id, either auto
> >>> probed or specified in the DT.
> >>
> >> That's right. Some PHY drivers might be relying on specific compatible
> >> strings though, but not the core PHY library that probes and maps a
> >> driver to a PHY node.
> >>
> >>>
> >>>>>  - the marvell compatible strings are used in our vendor tree. They
> >>>>>    aren't used anywhere but in our vendor tree. I though keeping them is
> >>>>>    nice since it identifies the PHY fully. And in case that level of
> >>>>>    detail is needed at some point it is already there.
> >>>>
> >>>> And this is the recommended way to do it in case we ever need to key a
> >>>> software decision based on the hardware.
> >>>
> >>> All compatible strings need to be documented.
> >>>
> >>> .. and they need to encode more information than you get from the phy
> >>> id - die revsision, package option, functional options, voltage
> >>> codes. Etc.
> >>>
> >>> .. and they actually need to be *right*
> >>
> >> Agreed.
> >>
> >>>
> >>> An example: The kernel reports 88E1318S for all four chips in that
> >>> family, AFAIK you have to read the package marking to figure out which
> >>> you have (it is the same die, with options switched on/off at
> >>> packaging time). People have already posted patches trying to
> >>> helpfully add a 'marvell,88E1318S' compatible string based on kernel
> >>> output. Except it is wrong, it isn't actually the '8S version in the
> >>> HW.
> >>>
> >>> Even worse, Marvell has a whole series of socket compatible phys. Just
> >>> because the board the DT author looked at has a '318, doesn't mean
> >>> that every board ever made will. We've actually already been switching
> >>> between the 318 and 318S for production depending on which has part
> >>> availability.
> >>>
> >>> Basically: don't try to override self-discoverable hardware in DT
> >>> without a really good reason.
> >>
> >> I think that's a very good point, at the very least let's use a
> >> compatible string that contains the full 32-bits PHY OUI.
> > 
> > I think resolution is:
> > 1. Do not use marvell,88e1518 because it is not listed anywhere
> > 2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection
> > if there is different phy on the board and we shouldn't restrict us in this.
> > In spite of autodetection takes some time.
> > 3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added
> > 4. Any listed compatible string has to be parsed which takes time
> > 
> > That's why I think make sense not to use any compatible string.
> > This should give us all flexibility which we want to have.
> 
> Sorry, but we do need some node that we can reference via phy-handle
> from the gem node, don't we?
> 
> In that case, is not specifying any compatible string a valid option?
> 
> If you don't want to specify the IDs, then I would've assumed we need to
> specify the -c22 in order to have *some* compatible string in order to
> trigger loading of the appropriate driver module.

The compatible string is listed as optional property for PHYs. So, not
having one is an option, I guess. But, I'd also prefer to at least keep
the -c22 one, since I saw problems when I tried using -c45 (the Zed phy
should support it...).
Also, so far, we haven't had any phy nodes in our Zynq dts files and
Ethernet worked, so the auto-detection there works pretty well
apparently. But it may be problematic if more than a single PHY is on
the MDIO bus, I'd assume.

	Sören
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Andreas Färber]

Am 29.08.2014 17:35, schrieb Sören Brinkmann:
> On Fri, 2014-08-29 at 05:18PM +0200, Andreas Färber wrote:
>> Am 29.08.2014 16:08, schrieb Michal Simek:
>>> I think resolution is:
>>> 1. Do not use marvell,88e1518 because it is not listed anywhere
>>> 2. Do not add ethernet-phy-idAAAA.BBBB because it breaks autodetection
>>> if there is different phy on the board and we shouldn't restrict us in this.
>>> In spite of autodetection takes some time.
>>> 3. "ethernet-phy-ieee802.3-c22" is optional that's why doesn't need to be added
>>> 4. Any listed compatible string has to be parsed which takes time
>>>
>>> That's why I think make sense not to use any compatible string.
>>> This should give us all flexibility which we want to have.
>>
>> Sorry, but we do need some node that we can reference via phy-handle
>> from the gem node, don't we?
>>
>> In that case, is not specifying any compatible string a valid option?
>>
>> If you don't want to specify the IDs, then I would've assumed we need to
>> specify the -c22 in order to have *some* compatible string in order to
>> trigger loading of the appropriate driver module.
> 
> The compatible string is listed as optional property for PHYs. So, not
> having one is an option, I guess. But, I'd also prefer to at least keep
> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy
> should support it...).
> Also, so far, we haven't had any phy nodes in our Zynq dts files and
> Ethernet worked, so the auto-detection there works pretty well
> apparently. But it may be problematic if more than a single PHY is on
> the MDIO bus, I'd assume.

Hm, it didn't work for me initially, but maybe some config option was
missing in multi_v7 or something else. I'll re-test next week.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Jason Gunthorpe]

On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote:

> The compatible string is listed as optional property for PHYs. So, not
> having one is an option, I guess. But, I'd also prefer to at least keep
> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy
> should support it...).

-c45 and -c22 use a completely different MDIO protocol, Zed doesn't
have a 10GE port, so it certainly doesn't use -c45.

If there is no compatible string at all the defined default is to use
-c22.

> Also, so far, we haven't had any phy nodes in our Zynq dts files and
> Ethernet worked, so the auto-detection there works pretty well
> apparently. But it may be problematic if more than a single PHY is on
> the MDIO bus, I'd assume.

Phy autodetection has always worked in some cases, but for DT ethernet
bindings it is expected that there is an option to specify an explicit
MDIO bus, and a phy-handle phandle to point to the phy. The phy should
be explicitly called out with the fixed MDIO address specified to
avoid sketchy MDIO address auto probing.

This provides the framework to specify interrupts and register
overrides for the phy in the DT.

Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Florian Fainelli]

On 08/29/2014 10:31 AM, Jason Gunthorpe wrote:
> On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote:
> 
>> The compatible string is listed as optional property for PHYs. So, not
>> having one is an option, I guess. But, I'd also prefer to at least keep
>> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy
>> should support it...).
> 
> -c45 and -c22 use a completely different MDIO protocol, Zed doesn't
> have a 10GE port, so it certainly doesn't use -c45.

Most recent 1GbE PHYs should also implement clause 45. It is a nice
improvement if you are using lot of transactions, otherwise clause 45
over clause 22 is suitable and supported by the PHY library (for EEE in
particular).

> 
> If there is no compatible string at all the defined default is to use
> -c22.

Absolutely, the current binding makes that property optional, but I
think we should make it mandatory instead.

> 
>> Also, so far, we haven't had any phy nodes in our Zynq dts files and
>> Ethernet worked, so the auto-detection there works pretty well
>> apparently. But it may be problematic if more than a single PHY is on
>> the MDIO bus, I'd assume.
> 
> Phy autodetection has always worked in some cases, but for DT ethernet
> bindings it is expected that there is an option to specify an explicit
> MDIO bus, and a phy-handle phandle to point to the phy. The phy should
> be explicitly called out with the fixed MDIO address specified to
> avoid sketchy MDIO address auto probing.
> 
> This provides the framework to specify interrupts and register
> overrides for the phy in the DT.

Absolutely, on the plus side, avoiding auto-detection also allows you to
have a shorter boot time (usually by a factor of few hundreds of msecs
depending on how the MDIO buses handle errors).
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Jason Gunthorpe]

On Fri, Aug 29, 2014 at 11:23:57AM -0700, Florian Fainelli wrote:
> On 08/29/2014 10:31 AM, Jason Gunthorpe wrote:
> > On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote:
> > 
> >> The compatible string is listed as optional property for PHYs. So, not
> >> having one is an option, I guess. But, I'd also prefer to at least keep
> >> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy
> >> should support it...).
> > 
> > -c45 and -c22 use a completely different MDIO protocol, Zed doesn't
> > have a 10GE port, so it certainly doesn't use -c45.
> 
> Most recent 1GbE PHYs should also implement clause 45. It is a nice
> improvement if you are using lot of transactions, otherwise clause 45
> over clause 22 is suitable and supported by the PHY library (for EEE in
> particular).

Oh, that is interesting, I haven't actually seen one of those yet..

Hum. So that is messy, even if the Zed phy supports the C45 format,
the macb driver (and by my reading, the Zynq hardware) lacks the
capability to generate C45 frames.

It could be supported, but you'd have to use the GPIO bitbang MDIO
driver to talk to the phy.

So... that makes the compatible string for the phy even more
confusing. 'Describe The HW' says it should have both c22 and c45
listed - however we don't have software support in Linux to negotiate
c22 and c45 support between the phy bus driver and attached phy :(

Jason

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Florian Fainelli]

On 08/29/2014 04:22 PM, Jason Gunthorpe wrote:
> On Fri, Aug 29, 2014 at 11:23:57AM -0700, Florian Fainelli wrote:
>> On 08/29/2014 10:31 AM, Jason Gunthorpe wrote:
>>> On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote:
>>>
>>>> The compatible string is listed as optional property for PHYs. So, not
>>>> having one is an option, I guess. But, I'd also prefer to at least keep
>>>> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy
>>>> should support it...).
>>>
>>> -c45 and -c22 use a completely different MDIO protocol, Zed doesn't
>>> have a 10GE port, so it certainly doesn't use -c45.
>>
>> Most recent 1GbE PHYs should also implement clause 45. It is a nice
>> improvement if you are using lot of transactions, otherwise clause 45
>> over clause 22 is suitable and supported by the PHY library (for EEE in
>> particular).
> 
> Oh, that is interesting, I haven't actually seen one of those yet..
> 
> Hum. So that is messy, even if the Zed phy supports the C45 format,
> the macb driver (and by my reading, the Zynq hardware) lacks the
> capability to generate C45 frames.

We should restrict ourselves to clause 45 over clause 22 compatibility
mode, which requires no MDIO bus driver changes.

> 
> It could be supported, but you'd have to use the GPIO bitbang MDIO
> driver to talk to the phy.
> 
> So... that makes the compatible string for the phy even more
> confusing. 'Describe The HW' says it should have both c22 and c45
> listed - however we don't have software support in Linux to negotiate
> c22 and c45 support between the phy bus driver and attached phy :(

Right now, if you set the c45 compatible string, the MDIO bus driver and
the PHY must support native c45 transactions to set phydev->is_c45, if
one or the other, or neither of those work, we will fallback to c22.

The part that is not figured out properly yet is how do we want to
handle functions (e.g: EEE) that are only accessible using c45 (native
or in compatible mode), since the PHY library uses two pairs of
accessors, with the native accessors not falling back to the indirect
accessor...
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Michal Simek]

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

On 08/30/2014 02:43 AM, Florian Fainelli wrote:
> On 08/29/2014 04:22 PM, Jason Gunthorpe wrote:
>> On Fri, Aug 29, 2014 at 11:23:57AM -0700, Florian Fainelli wrote:
>>> On 08/29/2014 10:31 AM, Jason Gunthorpe wrote:
>>>> On Fri, Aug 29, 2014 at 08:35:36AM -0700, Sören Brinkmann wrote:
>>>>
>>>>> The compatible string is listed as optional property for PHYs. So, not
>>>>> having one is an option, I guess. But, I'd also prefer to at least keep
>>>>> the -c22 one, since I saw problems when I tried using -c45 (the Zed phy
>>>>> should support it...).
>>>>
>>>> -c45 and -c22 use a completely different MDIO protocol, Zed doesn't
>>>> have a 10GE port, so it certainly doesn't use -c45.
>>>
>>> Most recent 1GbE PHYs should also implement clause 45. It is a nice
>>> improvement if you are using lot of transactions, otherwise clause 45
>>> over clause 22 is suitable and supported by the PHY library (for EEE in
>>> particular).
>>
>> Oh, that is interesting, I haven't actually seen one of those yet..
>>
>> Hum. So that is messy, even if the Zed phy supports the C45 format,
>> the macb driver (and by my reading, the Zynq hardware) lacks the
>> capability to generate C45 frames.
> 
> We should restrict ourselves to clause 45 over clause 22 compatibility
> mode, which requires no MDIO bus driver changes.
> 
>>
>> It could be supported, but you'd have to use the GPIO bitbang MDIO
>> driver to talk to the phy.
>>
>> So... that makes the compatible string for the phy even more
>> confusing. 'Describe The HW' says it should have both c22 and c45
>> listed - however we don't have software support in Linux to negotiate
>> c22 and c45 support between the phy bus driver and attached phy :(
> 
> Right now, if you set the c45 compatible string, the MDIO bus driver and
> the PHY must support native c45 transactions to set phydev->is_c45, if
> one or the other, or neither of those work, we will fallback to c22.
> 
> The part that is not figured out properly yet is how do we want to
> handle functions (e.g: EEE) that are only accessible using c45 (native
> or in compatible mode), since the PHY library uses two pairs of
> accessors, with the native accessors not falling back to the indirect
> accessor...

ok. I have read all responses listed here and still IMHO the best resolution
is not to add any compatible string.
I agree with Florian that we can have shorten boot time when idAAAA.BBBB is used
but also we are not checking that phy can be detect.

I believe all my points in my response are still valid and we have 3 options.
1. not to add any compatible string and use autodetection
2. Add idAAAA.BBBB to shorten boot up time
3. Add c22
4. Add c22 and idAAAA.BBBB

My preference is 1. because there could be problem with MII setting
and autodetection is good proof that everything is working.
If any user wants to have short boot up time it can specify ID in DTS
and also I believe DTS will be much longer because of some IPs in PL.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] ARM: zynq: DT: Add Ethernet phys

[Florian Fainelli]

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

On Aug 21, 2014 3:42 AM, "Michal Simek" <michal.simek@xilinx.com> wrote:
>
> On 08/20/2014 05:56 PM, Soren Brinkmann wrote:
> > Add missing Ethernet phys to Zynq DTs.
> >
> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> > ---
> >  arch/arm/boot/dts/zynq-zc702.dts | 6 ++++++
> >  arch/arm/boot/dts/zynq-zc706.dts | 6 ++++++
> >  arch/arm/boot/dts/zynq-zed.dts   | 6 ++++++
> >  3 files changed, 18 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/zynq-zc702.dts
b/arch/arm/boot/dts/zynq-zc702.dts
> > index 30bcfe20f0bc..fa810505ab8f 100644
> > --- a/arch/arm/boot/dts/zynq-zc702.dts
> > +++ b/arch/arm/boot/dts/zynq-zc702.dts
> > @@ -36,6 +36,12 @@
> >  &gem0 {
> >       status = "okay";
> >       phy-mode = "rgmii-id";
> > +     phy-handle = <&ethernet_phy>;
> > +
> > +     ethernet_phy: ethernet-phy@7 {
> > +             compatible = "marvell,88e1116r",
"ethernet-phy-ieee802.3-c22";
>
> c22 is completely unused by the kernel and also c22 is default option
anyway.

Which does not preclude you from following the binding defined in phy.txt,
and that binding mandates a valid compatible property.

> Any advantage to have c22 specified here?

We need to differentiate a clause 22 only PHY from a clause 45 compatible
PHY.

Soren's changes are good and are the recommended way to do it.

>
> Thanks,
> Michal
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[-- Attachment #2: Type: text/html, Size: 2395 bytes --]

Re: [PATCH 1/3] ARM: zynq: DT: Fix Ethernet phy modes

[Michal Simek]

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

On 08/20/2014 05:56 PM, Soren Brinkmann wrote:
> The used PHYs should be qualified as 'rgmii-id' instead of just 'rgmii'.
> For the Zed board this seems to make a difference between working and
> broken Ethernet.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
>  arch/arm/boot/dts/zynq-zc702.dts | 2 +-
>  arch/arm/boot/dts/zynq-zc706.dts | 2 +-
>  arch/arm/boot/dts/zynq-zed.dts   | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts
> index 835c3089c61c..30bcfe20f0bc 100644
> --- a/arch/arm/boot/dts/zynq-zc702.dts
> +++ b/arch/arm/boot/dts/zynq-zc702.dts
> @@ -35,7 +35,7 @@
>  
>  &gem0 {
>  	status = "okay";
> -	phy-mode = "rgmii";
> +	phy-mode = "rgmii-id";
>  };
>  
>  &i2c0 {
> diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts
> index 4cc9913078cd..3d9cb023e158 100644
> --- a/arch/arm/boot/dts/zynq-zc706.dts
> +++ b/arch/arm/boot/dts/zynq-zc706.dts
> @@ -32,7 +32,7 @@
>  
>  &gem0 {
>  	status = "okay";
> -	phy-mode = "rgmii";
> +	phy-mode = "rgmii-id";
>  };
>  
>  &i2c0 {
> diff --git a/arch/arm/boot/dts/zynq-zed.dts b/arch/arm/boot/dts/zynq-zed.dts
> index 82d7ef1a9a9c..42deb1b7dbc8 100644
> --- a/arch/arm/boot/dts/zynq-zed.dts
> +++ b/arch/arm/boot/dts/zynq-zed.dts
> @@ -32,7 +32,7 @@
>  
>  &gem0 {
>  	status = "okay";
> -	phy-mode = "rgmii";
> +	phy-mode = "rgmii-id";
>  };
>  
>  &sdhci0 {
> 

Applied.

M



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]