[PATCH 01/22] dt/bindings: fix documentation of ethernet-phy compatible property

[PATCH 01/22] dt/bindings: fix documentation of ethernet-phy compatible property

[Johan Hovold]

A recent commit extended the documentation of the ethernet-phy
compatible property, but placed the new paragraph under the max-speed
property.

Fixes: f00e756ed12d ("dt: Document a compatible entry for MDIO ethernet
Phys")

Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/net/phy.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/phy.txt b/Documentation/devicetree/bindings/net/phy.txt
index 5b8c58903077..40831fbaff72 100644
--- a/Documentation/devicetree/bindings/net/phy.txt
+++ b/Documentation/devicetree/bindings/net/phy.txt
@@ -19,7 +19,6 @@ Optional Properties:
   specifications. If neither of these are specified, the default is to
   assume clause 22. The compatible list may also contain other
   elements.
-- max-speed: Maximum PHY supported speed (10, 100, 1000...)
 
   If the phy's identifier is known then the list may contain an entry
   of the form: "ethernet-phy-idAAAA.BBBB" where
@@ -29,6 +28,8 @@ Optional Properties:
             4 hex digits. This is the chip vendor OUI bits 19:24,
             followed by 10 bits of a vendor specific ID.
 
+- max-speed: Maximum PHY supported speed (10, 100, 1000...)
+
 Example:
 
 ethernet-phy@0 {
-- 
2.0.4

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

[PATCH 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Johan Hovold]

Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
documentation.

Cc: devicetree@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
index a1bab5eaae02..9b08dd6551dd 100644
--- a/Documentation/devicetree/bindings/net/micrel.txt
+++ b/Documentation/devicetree/bindings/net/micrel.txt
@@ -19,6 +19,11 @@ Optional properties:
 
               See the respective PHY datasheet for the mode values.
 
+ - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
+
+		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
+		when the rmii_ref_clk_sel bit is set.
+
  - clocks, clock-names: contains clocks according to the common clock bindings.
 
               supported clocks:
-- 
2.0.4

Re: [PATCH 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Mark Rutland]

On Tue, Nov 11, 2014 at 05:37:37PM +0000, Johan Hovold wrote:
> Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
> documentation.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>  Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> index a1bab5eaae02..9b08dd6551dd 100644
> --- a/Documentation/devicetree/bindings/net/micrel.txt
> +++ b/Documentation/devicetree/bindings/net/micrel.txt
> @@ -19,6 +19,11 @@ Optional properties:
>  
>                See the respective PHY datasheet for the mode values.
>  
> + - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
> +
> +		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
> +		when the rmii_ref_clk_sel bit is set.

s/_/-/ in property names please.

That said, I don't follow the meaning. Does this cause the kernel to do
something different, or is is simply that a 25MHz ref clock is wired up?

Surely that should be described via the common clock bindings? Or if
internal through a clock-frequency property?

Mark.

Re: [PATCH 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Johan Hovold]

On Tue, Nov 11, 2014 at 05:57:42PM +0000, Mark Rutland wrote:
> On Tue, Nov 11, 2014 at 05:37:37PM +0000, Johan Hovold wrote:
> > Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
> > documentation.
> > 
> > Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Signed-off-by: Johan Hovold <johan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> > index a1bab5eaae02..9b08dd6551dd 100644
> > --- a/Documentation/devicetree/bindings/net/micrel.txt
> > +++ b/Documentation/devicetree/bindings/net/micrel.txt
> > @@ -19,6 +19,11 @@ Optional properties:
> >  
> >                See the respective PHY datasheet for the mode values.
> >  
> > + - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
> > +
> > +		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
> > +		when the rmii_ref_clk_sel bit is set.
> 
> s/_/-/ in property names please.

Ouch, copied from variable name, sorry.

> That said, I don't follow the meaning. Does this cause the kernel to do
> something different, or is is simply that a 25MHz ref clock is wired up?

Yes, the driver currently sets this configuration bit based on a common
clock binding.

However, it turns out the meaning of the bit is reversed on some PHY
variants. On most PHYs 50 MHz mode is selected by setting this bit,
whereas on the PHYs that need this new property, setting it selects 25
MHz mode instead.

> Surely that should be described via the common clock bindings? Or if
> internal through a clock-frequency property?

The driver currently selects the mode using the common clock bindings,
but this new property is needed to properly handle those PHY variants on
which the clock configuration bit has the reverse meaning.

Thanks,
Johan
--
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 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Sascha Hauer]

On Tue, Nov 11, 2014 at 07:18:25PM +0100, Johan Hovold wrote:
> On Tue, Nov 11, 2014 at 05:57:42PM +0000, Mark Rutland wrote:
> > On Tue, Nov 11, 2014 at 05:37:37PM +0000, Johan Hovold wrote:
> > > Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
> > > documentation.
> > > 
> > > Cc: devicetree@vger.kernel.org
> > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > ---
> > >  Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> > > index a1bab5eaae02..9b08dd6551dd 100644
> > > --- a/Documentation/devicetree/bindings/net/micrel.txt
> > > +++ b/Documentation/devicetree/bindings/net/micrel.txt
> > > @@ -19,6 +19,11 @@ Optional properties:
> > >  
> > >                See the respective PHY datasheet for the mode values.
> > >  
> > > + - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
> > > +
> > > +		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
> > > +		when the rmii_ref_clk_sel bit is set.
> > 
> > s/_/-/ in property names please.
> 
> Ouch, copied from variable name, sorry.
> 
> > That said, I don't follow the meaning. Does this cause the kernel to do
> > something different, or is is simply that a 25MHz ref clock is wired up?
> 
> Yes, the driver currently sets this configuration bit based on a common
> clock binding.
> 
> However, it turns out the meaning of the bit is reversed on some PHY
> variants. On most PHYs 50 MHz mode is selected by setting this bit,
> whereas on the PHYs that need this new property, setting it selects 25
> MHz mode instead.

Maybe rename the property to something like rmii-ref-clk-25mhz-active-high
then? Also you should probably make it more explicit that this is a
hardware property and not for adjusting the clock.

It's not very nice from Micrel to create Phys with exactly the same
device id but the meaning of this bit reversed.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Johan Hovold]

On Wed, Nov 12, 2014 at 08:01:27AM +0100, Sascha Hauer wrote:
> On Tue, Nov 11, 2014 at 07:18:25PM +0100, Johan Hovold wrote:
> > On Tue, Nov 11, 2014 at 05:57:42PM +0000, Mark Rutland wrote:
> > > On Tue, Nov 11, 2014 at 05:37:37PM +0000, Johan Hovold wrote:
> > > > Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
> > > > documentation.
> > > > 
> > > > Cc: devicetree@vger.kernel.org
> > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > ---
> > > >  Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> > > > index a1bab5eaae02..9b08dd6551dd 100644
> > > > --- a/Documentation/devicetree/bindings/net/micrel.txt
> > > > +++ b/Documentation/devicetree/bindings/net/micrel.txt
> > > > @@ -19,6 +19,11 @@ Optional properties:
> > > >  
> > > >                See the respective PHY datasheet for the mode values.
> > > >  
> > > > + - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
> > > > +
> > > > +		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
> > > > +		when the rmii_ref_clk_sel bit is set.
> > > 
> > > s/_/-/ in property names please.
> > 
> > Ouch, copied from variable name, sorry.
> > 
> > > That said, I don't follow the meaning. Does this cause the kernel to do
> > > something different, or is is simply that a 25MHz ref clock is wired up?
> > 
> > Yes, the driver currently sets this configuration bit based on a common
> > clock binding.
> > 
> > However, it turns out the meaning of the bit is reversed on some PHY
> > variants. On most PHYs 50 MHz mode is selected by setting this bit,
> > whereas on the PHYs that need this new property, setting it selects 25
> > MHz mode instead.
> 
> Maybe rename the property to something like rmii-ref-clk-25mhz-active-high
> then? Also you should probably make it more explicit that this is a
> hardware property and not for adjusting the clock.

You're right, but how about calling it

	micrel,rmii-reference-clock-select-inverted

Then no one will set it believing it will change the clock mode and
without reading the binding doc first.

The description could then read something like

	micrel,rmii-reference-clock-select-inverted: RMII Reference
		Clock Select bit is inverted

	The RMII Reference Clock Select bit is inverted so that setting
	it selects 25 MHz rather than 50 MHz clock mode. 

	Note that this is only needed for PHY variants that has this bit
	inverted and that a clock reference ("rmii-ref" below) is always
	needed to select the actual mode.

> It's not very nice from Micrel to create Phys with exactly the same
> device id but the meaning of this bit reversed.

Not very nice at all.

Thanks,
Johan

Re: [PATCH 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Sascha Hauer]

On Wed, Nov 12, 2014 at 10:19:20AM +0100, Johan Hovold wrote:
> On Wed, Nov 12, 2014 at 08:01:27AM +0100, Sascha Hauer wrote:
> > On Tue, Nov 11, 2014 at 07:18:25PM +0100, Johan Hovold wrote:
> > > On Tue, Nov 11, 2014 at 05:57:42PM +0000, Mark Rutland wrote:
> > > > On Tue, Nov 11, 2014 at 05:37:37PM +0000, Johan Hovold wrote:
> > > > > Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
> > > > > documentation.
> > > > > 
> > > > > Cc: devicetree@vger.kernel.org
> > > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
> > > > >  1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> > > > > index a1bab5eaae02..9b08dd6551dd 100644
> > > > > --- a/Documentation/devicetree/bindings/net/micrel.txt
> > > > > +++ b/Documentation/devicetree/bindings/net/micrel.txt
> > > > > @@ -19,6 +19,11 @@ Optional properties:
> > > > >  
> > > > >                See the respective PHY datasheet for the mode values.
> > > > >  
> > > > > + - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
> > > > > +
> > > > > +		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
> > > > > +		when the rmii_ref_clk_sel bit is set.
> > > > 
> > > > s/_/-/ in property names please.
> > > 
> > > Ouch, copied from variable name, sorry.
> > > 
> > > > That said, I don't follow the meaning. Does this cause the kernel to do
> > > > something different, or is is simply that a 25MHz ref clock is wired up?
> > > 
> > > Yes, the driver currently sets this configuration bit based on a common
> > > clock binding.
> > > 
> > > However, it turns out the meaning of the bit is reversed on some PHY
> > > variants. On most PHYs 50 MHz mode is selected by setting this bit,
> > > whereas on the PHYs that need this new property, setting it selects 25
> > > MHz mode instead.
> > 
> > Maybe rename the property to something like rmii-ref-clk-25mhz-active-high
> > then? Also you should probably make it more explicit that this is a
> > hardware property and not for adjusting the clock.
> 
> You're right, but how about calling it
> 
> 	micrel,rmii-reference-clock-select-inverted
> 
> Then no one will set it believing it will change the clock mode and
> without reading the binding doc first.
> 
> The description could then read something like
> 
> 	micrel,rmii-reference-clock-select-inverted: RMII Reference
> 		Clock Select bit is inverted
> 
> 	The RMII Reference Clock Select bit is inverted so that setting
> 	it selects 25 MHz rather than 50 MHz clock mode. 
> 
> 	Note that this is only needed for PHY variants that has this bit
> 	inverted and that a clock reference ("rmii-ref" below) is always
> 	needed to select the actual mode.

"Inverted" only has a meaning when everybody agrees what's the normal
case. Since that not the case I really prefer talking about
"active-high" or "active-low".

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 19/22] dt/bindings: add micrel,rmii_ref_clk_sel_25_mhz to eth-phy binding

[Johan Hovold]

On Thu, Nov 13, 2014 at 09:09:27AM +0100, Sascha Hauer wrote:
> On Wed, Nov 12, 2014 at 10:19:20AM +0100, Johan Hovold wrote:
> > On Wed, Nov 12, 2014 at 08:01:27AM +0100, Sascha Hauer wrote:
> > > On Tue, Nov 11, 2014 at 07:18:25PM +0100, Johan Hovold wrote:
> > > > On Tue, Nov 11, 2014 at 05:57:42PM +0000, Mark Rutland wrote:
> > > > > On Tue, Nov 11, 2014 at 05:37:37PM +0000, Johan Hovold wrote:
> > > > > > Add "micrel,rmii_ref_clk_sel_25_mhz" to Micrel ethernet PHY binding
> > > > > > documentation.
> > > > > > 
> > > > > > Cc: devicetree@vger.kernel.org
> > > > > > Signed-off-by: Johan Hovold <johan@kernel.org>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/net/micrel.txt | 5 +++++
> > > > > >  1 file changed, 5 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> > > > > > index a1bab5eaae02..9b08dd6551dd 100644
> > > > > > --- a/Documentation/devicetree/bindings/net/micrel.txt
> > > > > > +++ b/Documentation/devicetree/bindings/net/micrel.txt
> > > > > > @@ -19,6 +19,11 @@ Optional properties:
> > > > > >  
> > > > > >                See the respective PHY datasheet for the mode values.
> > > > > >  
> > > > > > + - micrel,rmii_ref_clk_sel_25_mhz: rmii_ref_clk_sel bit selects 25 MHz mode
> > > > > > +
> > > > > > +		Whether 25 MHz (rather than 50 Mhz) clock mode is selected
> > > > > > +		when the rmii_ref_clk_sel bit is set.
> > > > > 
> > > > > s/_/-/ in property names please.
> > > > 
> > > > Ouch, copied from variable name, sorry.
> > > > 
> > > > > That said, I don't follow the meaning. Does this cause the kernel to do
> > > > > something different, or is is simply that a 25MHz ref clock is wired up?
> > > > 
> > > > Yes, the driver currently sets this configuration bit based on a common
> > > > clock binding.
> > > > 
> > > > However, it turns out the meaning of the bit is reversed on some PHY
> > > > variants. On most PHYs 50 MHz mode is selected by setting this bit,
> > > > whereas on the PHYs that need this new property, setting it selects 25
> > > > MHz mode instead.
> > > 
> > > Maybe rename the property to something like rmii-ref-clk-25mhz-active-high
> > > then? Also you should probably make it more explicit that this is a
> > > hardware property and not for adjusting the clock.
> > 
> > You're right, but how about calling it
> > 
> > 	micrel,rmii-reference-clock-select-inverted
> > 
> > Then no one will set it believing it will change the clock mode and
> > without reading the binding doc first.
> > 
> > The description could then read something like
> > 
> > 	micrel,rmii-reference-clock-select-inverted: RMII Reference
> > 		Clock Select bit is inverted
> > 
> > 	The RMII Reference Clock Select bit is inverted so that setting
> > 	it selects 25 MHz rather than 50 MHz clock mode. 
> > 
> > 	Note that this is only needed for PHY variants that has this bit
> > 	inverted and that a clock reference ("rmii-ref" below) is always
> > 	needed to select the actual mode.
> 
> "Inverted" only has a meaning when everybody agrees what's the normal
> case. Since that not the case I really prefer talking about
> "active-high" or "active-low".

Yes, but I'm reluctant to using "active-high" and "active-low" as this
is not a signal (or IO pin) we're dealing with.

It's a configuration bit, which (if set) selects 25 MHz mode. Hence I
still think something like 

	micrel,rmii_ref_clk_sel_25_mhz

or

	micrel,rmii_reference_clock_select_selects_25_mhz

is preferred. The binding documentation will still make it clear that a
clocks reference is also needed.

Johan